fix(macos): resolve empty chat display during tab switching #2
Conversation
- Add immediate state reset using useLayoutEffect when sessions change - Implement requestAnimationFrame for macOS-optimized rendering - Replace useEffect with useLayoutEffect for DOM operations - Add stable virtualizer keys to force proper React reconciliation - Enhanced error handling with fallback scroll mechanisms - Guard against undefined messages in virtual list rendering - Add debug visibility for message filtering states Fixes race conditions in @tanstack/react-virtual on macOS WebKit where chat messages would appear empty when switching tabs or reloading sessions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @KonsomeJona, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request fixes an issue on macOS where the chat display would appear empty after switching tabs. The changes involve significant modifications to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of changes to address a rendering issue on macOS related to tab switching. The strategy of forcing the virtualizer to remount using a key, combined with resetting component state via useLayoutEffect and improving scroll handling with requestAnimationFrame, is a solid approach for tackling such platform-specific UI bugs.
The code is well-commented, explaining the rationale behind the changes. The added try-catch blocks and fallback mechanisms significantly improve the robustness of the component. Overall, this is a high-quality fix for what appears to be a tricky problem.
| setTimeout(() => { | ||
| if (loadedMessages.length > 0) { | ||
| try { | ||
| rowVirtualizer.scrollToIndex(loadedMessages.length - 1, { align: 'end', behavior: 'auto' }); | ||
| } catch (error) { | ||
| console.warn('Failed to scroll to index after loading history:', error); | ||
| // Fallback scroll | ||
| if (parentRef.current) { | ||
| parentRef.current.scrollTop = parentRef.current.scrollHeight; | ||
| } | ||
| } | ||
| } | ||
| }, 100); // Reduced delay since we're using RAF |
There was a problem hiding this comment.
The use of requestAnimationFrame and try-catch here is a great improvement for robustness. However, the setTimeout with a 100ms delay introduces a magic number and can be brittle—potentially too long on fast devices and too short on slow ones.
If the goal is to wait for the next render cycle after requestAnimationFrame, a setTimeout with a 0 delay is usually sufficient and more idiomatic. This pushes the execution to the end of the browser's event queue, ensuring it runs after the current execution context (including rendering) is complete.
Consider using a 0 delay unless the 100ms wait is strictly necessary. If it is, it would be helpful to add a comment explaining why.
| setTimeout(() => { | |
| if (loadedMessages.length > 0) { | |
| try { | |
| rowVirtualizer.scrollToIndex(loadedMessages.length - 1, { align: 'end', behavior: 'auto' }); | |
| } catch (error) { | |
| console.warn('Failed to scroll to index after loading history:', error); | |
| // Fallback scroll | |
| if (parentRef.current) { | |
| parentRef.current.scrollTop = parentRef.current.scrollHeight; | |
| } | |
| } | |
| } | |
| }, 100); // Reduced delay since we're using RAF | |
| setTimeout(() => { | |
| if (loadedMessages.length > 0) { | |
| try { | |
| rowVirtualizer.scrollToIndex(loadedMessages.length - 1, { align: 'end', behavior: 'auto' }); | |
| } catch (error) { | |
| console.warn('Failed to scroll to index after loading history:', error); | |
| // Fallback scroll | |
| if (parentRef.current) { | |
| parentRef.current.scrollTop = parentRef.current.scrollHeight; | |
| } | |
| } | |
| } | |
| }, 0); // Use 0ms delay to schedule scroll after paint and other operations. |
| displayableMessages.length === 0 && messages.length > 0 && ( | ||
| <div className="absolute inset-x-4 py-8 text-center text-muted-foreground"> | ||
| <p>Messages are being processed...</p> | ||
| <p className="text-xs mt-2">Raw: {messages.length}, Filtered: {displayableMessages.length}</p> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
This "Messages are being processed..." state is a good idea for providing feedback instead of showing a blank screen. However, it currently includes debugging information (Raw: ..., Filtered: ...) that probably shouldn't be in a production build. Additionally, the text "Messages are being processed..." is a bit technical for end-users.
I suggest two small improvements:
- Make the user-facing text more friendly, such as "Updating chat...".
- Wrap the debugging statistics in a
process.env.NODE_ENV === 'development'check so they only appear during development.
displayableMessages.length === 0 && messages.length > 0 && (
<div className="absolute inset-x-4 py-8 text-center text-muted-foreground">
<p>Updating chat...</p>
{process.env.NODE_ENV === 'development' && (
<p className="text-xs mt-2">Raw: {messages.length}, Filtered: {displayableMessages.length}</p>
)}
</div>
)
Summary
Changes
Test plan