BA-2081: chat rooms refetch issue #182
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CR as ChatRoomsList Component
participant V as Virtuoso Component
participant DF as Data Fetcher
U->>CR: Change tab / search input
CR->>CR: Set renderList = false
U->>CR: New data received
CR->>CR: useEffect resets renderList = true
CR->>V: Conditionally render Virtuoso (if renderList true)
CR->>DF: Call refetch({ fetchPolicy: 'network-only' })
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/components/modules/messages/ChatRoomsList/index.tsx (1)
Reset cursor when switching tabs to ensure proper pagination
The cursor should be reset when switching tabs because each tab (unread/archived) represents a distinct dataset. Using a cursor from one tab's context for another tab's query could lead to incorrect pagination results.
The suggested implementation is correct:
const handleChange = (event: React.SyntheticEvent, newTab: string) => { setTab(newTab as ChatTabValues) + cursor.current = null startRefetchTransition(() => { refetch( { unreadMessages: newTab === CHAT_TAB_VALUES.unread, archived: newTab === CHAT_TAB_VALUES.archived, - cursor: cursor.current, }, { fetchPolicy: 'store-and-network' }, ) }) }🔗 Analysis chain
Line range hint
70-80: Verify cursor handling during tab switches.While the cursor is correctly passed to refetch, consider if the cursor should be reset when switching tabs to ensure fresh data fetching.
Consider this implementation:
const handleChange = (event: React.SyntheticEvent, newTab: string) => { setTab(newTab as ChatTabValues) + cursor.current = null startRefetchTransition(() => { refetch( { unreadMessages: newTab === CHAT_TAB_VALUES.unread, archived: newTab === CHAT_TAB_VALUES.archived, - cursor: cursor.current, }, { fetchPolicy: 'store-and-network' }, ) }) }Let's verify the current behavior:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar pagination implementations in the codebase ast-grep --pattern 'refetch($_, { fetchPolicy: $_ })'Length of output: 937
Script:
#!/bin/bash # Search for cursor usage in state and refs ast-grep --pattern 'const cursor = useRef' # Search for pagination-related patterns ast-grep --pattern 'const [$_] = usePagination' # Look for cursor reset patterns rg "cursor.*null" -A 2Length of output: 7142
🧹 Nitpick comments (1)
packages/components/modules/messages/ChatRoomsList/index.tsx (1)
48-48: Consider maintaining separate cursors per tab.To improve the pagination state management, consider maintaining separate cursors for each tab. This would prevent pagination state interference between tabs and provide a better user experience.
Example implementation:
const cursors = useRef<Record<ChatTabValues, string | null>>({ [CHAT_TAB_VALUES.active]: null, [CHAT_TAB_VALUES.unread]: null, [CHAT_TAB_VALUES.archived]: null })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2692120 and 44e22f6a90330527cc0b2766dd7314b0b4c23b8e.
📒 Files selected for processing (1)
packages/components/modules/messages/ChatRoomsList/index.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/components/modules/messages/ChatRoomsList/index.tsx (2)
3-11: LGTM! Clean import organization.The addition of
useRefis properly grouped with other React hooks.
48-48: LGTM! Well-typed cursor reference.The cursor ref is properly typed to handle all possible states and appropriately initialized.
|
I am slightly confused, can you explain how this works? It looks to me like |
pt-tsl
force-pushed
the
BA-2081-dissapearing-chats
branch
2 times, most recently
from
cf402b1 to
ec67947
Compare
pt-tsl
force-pushed
the
BA-2081-dissapearing-chats
branch
from
ec67947 to
a96ed0f
Compare
pt-tsl
requested review from
deboracosilveira and
priscilladeroode
as code owners
pt-tsl
force-pushed
the
BA-2081-dissapearing-chats
branch
from
a96ed0f to
3635abb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/modules/messages/ChatRoomsList/index.tsx (1)
215-219: Consider memoizing the endReached callback.The endReached callback is recreated on every render. Consider memoizing it with useCallback to optimize performance.
+ const handleEndReached = useCallback(() => { + if (hasNext) { + loadNext(5) + } + }, [hasNext, loadNext]) return ( <ChatRoomListContainer> {/* ... */} {renderList && ( <Virtuoso {/* ... */} - endReached={() => { - if (hasNext) { - loadNext(5) - } - }} + endReached={handleEndReached} {...VirtuosoProps} /> )} </ChatRoomListContainer> )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/modules/messages/ChatRoomsList/index.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Lint
- GitHub Check: Component Test Applications and Packages
🔇 Additional comments (2)
packages/components/modules/messages/ChatRoomsList/index.tsx (2)
3-41: LGTM! Clean state management addition.The new
renderListstate is well-integrated and follows React conventions.
97-102: Consider tracking the Virtuoso bug.The workaround is well-documented, but it would be beneficial to track the upstream Virtuoso issue for a proper fix in the future.
Could you share the Virtuoso issue number? If not, consider creating one to track this workaround.
| const handleSearchChange: ChangeEventHandler<HTMLInputElement> = (e) => { | ||
| const value = e.target.value || '' | ||
| startTransition(() => { | ||
| refetch({ | ||
| q: value, | ||
| unreadMessages: isInUnreadTab, | ||
| archived: isInArchivedTab, | ||
| }) | ||
| refetch( | ||
| { | ||
| q: value, | ||
| unreadMessages: isInUnreadTab, | ||
| archived: isInArchivedTab, | ||
| }, | ||
| { fetchPolicy: 'network-only' }, | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| const handleSearchClear = () => { | ||
| startTransition(() => { | ||
| reset() | ||
| refetch({ | ||
| q: '', | ||
| unreadMessages: isInUnreadTab, | ||
| archived: isInArchivedTab, | ||
| }) | ||
| refetch( | ||
| { | ||
| q: '', | ||
| unreadMessages: isInUnreadTab, | ||
| archived: isInArchivedTab, | ||
| }, | ||
| { fetchPolicy: 'network-only' }, | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| const handleChange = (event: React.SyntheticEvent, newTab: string) => { | ||
| setTab(newTab as ChatTabValues) | ||
| startRefetchTransition(() => { | ||
| setRenderList(false) | ||
| refetch( | ||
| { | ||
| q: searchValue, | ||
| unreadMessages: newTab === CHAT_TAB_VALUES.unread, | ||
| archived: newTab === CHAT_TAB_VALUES.archived, | ||
| }, | ||
| { fetchPolicy: 'store-and-network' }, | ||
| { fetchPolicy: 'network-only' }, | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consistent renderList handling across all refetch scenarios.
While handleChange sets renderList to false before refetch, the search handlers don't. This could lead to inconsistent behavior when the Virtuoso component needs to be remounted.
Apply this diff to maintain consistency:
const handleSearchChange: ChangeEventHandler<HTMLInputElement> = (e) => {
const value = e.target.value || ''
startTransition(() => {
+ setRenderList(false)
refetch(
{
q: value,
unreadMessages: isInUnreadTab,
archived: isInArchivedTab,
},
{ fetchPolicy: 'network-only' },
)
})
}
const handleSearchClear = () => {
startTransition(() => {
reset()
+ setRenderList(false)
refetch(
{
q: '',
unreadMessages: isInUnreadTab,
archived: isInArchivedTab,
},
{ fetchPolicy: 'network-only' },
)
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSearchChange: ChangeEventHandler<HTMLInputElement> = (e) => { | |
| const value = e.target.value || '' | |
| startTransition(() => { | |
| refetch({ | |
| q: value, | |
| unreadMessages: isInUnreadTab, | |
| archived: isInArchivedTab, | |
| }) | |
| refetch( | |
| { | |
| q: value, | |
| unreadMessages: isInUnreadTab, | |
| archived: isInArchivedTab, | |
| }, | |
| { fetchPolicy: 'network-only' }, | |
| ) | |
| }) | |
| } | |
| const handleSearchClear = () => { | |
| startTransition(() => { | |
| reset() | |
| refetch({ | |
| q: '', | |
| unreadMessages: isInUnreadTab, | |
| archived: isInArchivedTab, | |
| }) | |
| refetch( | |
| { | |
| q: '', | |
| unreadMessages: isInUnreadTab, | |
| archived: isInArchivedTab, | |
| }, | |
| { fetchPolicy: 'network-only' }, | |
| ) | |
| }) | |
| } | |
| const handleChange = (event: React.SyntheticEvent, newTab: string) => { | |
| setTab(newTab as ChatTabValues) | |
| startRefetchTransition(() => { | |
| setRenderList(false) | |
| refetch( | |
| { | |
| q: searchValue, | |
| unreadMessages: newTab === CHAT_TAB_VALUES.unread, | |
| archived: newTab === CHAT_TAB_VALUES.archived, | |
| }, | |
| { fetchPolicy: 'store-and-network' }, | |
| { fetchPolicy: 'network-only' }, | |
| ) | |
| }) | |
| } | |
| const handleSearchChange: ChangeEventHandler<HTMLInputElement> = (e) => { | |
| const value = e.target.value || '' | |
| startTransition(() => { | |
| setRenderList(false) | |
| refetch( | |
| { | |
| q: value, | |
| unreadMessages: isInUnreadTab, | |
| archived: isInArchivedTab, | |
| }, | |
| { fetchPolicy: 'network-only' }, | |
| ) | |
| }) | |
| } | |
| const handleSearchClear = () => { | |
| startTransition(() => { | |
| reset() | |
| setRenderList(false) | |
| refetch( | |
| { | |
| q: '', | |
| unreadMessages: isInUnreadTab, | |
| archived: isInArchivedTab, | |
| }, | |
| { fetchPolicy: 'network-only' }, | |
| ) | |
| }) | |
| } | |
| const handleChange = (event: React.SyntheticEvent, newTab: string) => { | |
| setTab(newTab as ChatTabValues) | |
| startRefetchTransition(() => { | |
| setRenderList(false) | |
| refetch( | |
| { | |
| q: searchValue, | |
| unreadMessages: newTab === CHAT_TAB_VALUES.unread, | |
| archived: newTab === CHAT_TAB_VALUES.archived, | |
| }, | |
| { fetchPolicy: 'network-only' }, | |
| ) | |
| }) | |
| } |
pt-tsl
force-pushed
the
BA-2081-dissapearing-chats
branch
from
3635abb to
43f9264
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/messages/web/ChatRoomsList/index.tsx (1)
151-160: Add error boundary for Virtuoso component.While the rendering logic is cleaner, consider adding error handling to gracefully recover from potential Virtuoso rendering failures.
+import { ErrorBoundary } from 'react-error-boundary' + +const VirtuosoErrorFallback = () => ( + <Box p={2}> + <Typography>Failed to load chat rooms. Please try again.</Typography> + </Box> +) {renderList && ( + <ErrorBoundary FallbackComponent={VirtuosoErrorFallback}> <Virtuoso data={chatRooms} // ... other props /> + </ErrorBoundary> )}Also applies to: 204-221
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/modules/messages/web/ChatRoomsList/index.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (5)
packages/components/modules/messages/web/ChatRoomsList/index.tsx (5)
3-11: LGTM! State management changes look good.The new imports and state variable are well-organized and necessary for implementing the Virtuoso component workaround.
Also applies to: 39-39
53-65: Consider caching implications of network-only fetch policy.While using
fetchPolicy: 'network-only'ensures fresh data, it bypasses the cache completely. This might impact performance, especially during rapid search interactions.Consider these alternatives:
- Use
cache-and-networkto show cached results while fetching- Implement a debounce for search to reduce network requests
Would you like me to provide implementation examples for these alternatives?
Also applies to: 67-79
81-94:✅ Verification successful
LGTM! Tab change handling addresses the core issue.
The implementation ensures fresh data fetching when switching tabs while properly handling the Virtuoso component's state.
Monitor the behavior in production to ensure:
- Chat rooms remain visible when switching back to Active tab
- No unnecessary refetches occur
🏁 Script executed:
#!/bin/bash # Search for any existing error handling or monitoring code rg -A 5 "error|monitor|track" packages/components/modules/messages/Length of output: 35286
LGTM! Tab change handling and data refetch logic look solid.
The code correctly resets the render state and forces a network-only data fetch when switching tabs. I reviewed the surrounding modules and found that error monitoring and toast notifications are consistently implemented elsewhere, indicating that additional error handling in this component isn’t necessary. Please continue to monitor in production to ensure:
- Chat rooms remain visible when switching back to the Active tab.
- No redundant or unnecessary refetches occur.
42-44:❓ Verification inconclusive
Verify cursor management in useRoomsList hook.
Regarding the concern about
cursor.currentin the comments:
- The
network-onlyfetch policy ensures fresh data fetching, which should help maintain correct cursor state- However, we should verify the cursor management in the
useRoomsListhook
🏁 Script executed:
#!/bin/bash # Search for useRoomsList implementation rg -A 10 "useRoomsList.*=.*{" packages/components/modules/messages/Length of output: 169
Action Required: Verify
useRoomsListCursor ManagementThe previous search command encountered a regex error. Please run an updated command to inspect the implementation of
useRoomsListand confirm thatcursor.currentis managed correctly. For example:This should provide context on how the hook manages its internal state (including
cursor.current). Once you confirm that the fresh data fetching (via the network-only fetch policy) aligns with proper cursor management, we can be confident that this part of the implementation meets the PR objectives.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for the definition of the `useRoomsList` hook with a corrected regex. rg -A 10 'const useRoomsList\s*=' packages/components/modules/messages/Length of output: 752
Verify and Confirm Cursor Management in
usePaginationFragmentThe changes rely on the
network-onlyfetch policy to ensure fresh data is loaded. Note thatuseRoomsList—located atpackages/components/modules/messages/common/graphql/fragments/RoomsList.ts—is a thin wrapper aroundusePaginationFragment. There’s no explicit handling ofcursor.currentinuseRoomsList, so please ensure thatusePaginationFragmentcorrectly manages the pagination cursor when new data is fetched.
- Verify that the pagination state, including
cursor.current, resets or updates correctly with the network-only policy.- Double-check any internal documentation or tests for
usePaginationFragmentto confirm that its behavior meets the intended design.
96-101:✅ Verification successful
LGTM! Well-documented workaround for Virtuoso bug.
The workaround is clearly explained and effectively handles the component remounting.
🌐 Web query:
Is there an open issue for the Virtuoso endReached bug when data changes?💡 Result:
There is an open GitHub issue (#985) specifically addressing a miscalculation in the
endReachedindex when using GroupedVirtuoso, where group headers are incorrectly counted as list items[1]. This causes discrepancies between reported indexes (e.g., 522) and actual item indexes (e.g., 499). A CodeSandbox reproduction demonstrates the issue[1].For non-grouped lists using standard
Virtuoso, there's no confirmed open bug regardingendReachedduring data changes. However:
- Data Refresh Handling: A Lightrun thread indicates fixes were implemented to reset
endReachedstate when data refreshes[7]. This prevents stale indexes when appending/prepending items.- Troubleshooting Recommendations:
- Verify items aren't empty (Virtuoso requires measurable content)[4]
- Check for CSS margins causing measurement errors[4]
- Use
logLevel=DEBUGto inspect rendering behavior[4]- Documentation Guidance: The official docs recommend using
endReachedwith state management that appends new items while preserving scroll position[3][9].For grouped list scenarios, follow issue #985 for updates. In standard lists, ensure you're using react-virtuoso v3.3.0+ which includes fixes for edge cases in dynamic data handling[7].
Citations:
- 1: [BUG] Wrong endReached index passed from GroupedVirtuoso petyosi/react-virtuoso#985
- 2: https://dev.to/petyosi/why-i-wrote-my-own-state-management-for-react-virtuoso-1b54
- 3: https://virtuoso.dev
- 4: https://virtuoso.dev/troubleshooting/
- 5: https://www.npmjs.com/package/@fezvrasta/react-virtuoso
- 6: https://www.youtube.com/watch?v=tWaxFSWAXuw
- 7: https://lightrun.com/answers/petyosi-react-virtuoso-reset-endreached
- 8: https://community.cadence.com/cadence_technology_forums/f/custom-ic-design/43817/some-gui-windows-fail-to-launch-in-virtuoso
- 9: https://virtuoso.dev/endless-scrolling/
- 10: https://forum.openmandriva.org/t/issue-bug-tracking-system/2183
LGTM: Well-Documented Workaround for Virtuoso "endReached" Behavior
The workaround using
setRenderList(true)on data changes is clear and effective in forcing the component to remount—which addresses issues with Virtuoso'sendReachednot triggering properly. Although GitHub issue #985 highlights concerns for grouped lists, our usage appears to be for the standard (non-grouped) list where such issues have largely been resolved in recent versions (v3.3.0+). Please continue to monitor updates from react-virtuoso in case further refinements allow removal of this workaround.
pt-tsl
force-pushed
the
BA-2081-dissapearing-chats
branch
2 times, most recently
from
ceeab46 to
cccb2cd
Compare
pt-tsl
force-pushed
the
BA-2211-delete-chat-message
branch
2 times, most recently
from
25a1670 to
8fb8dfa
Compare
* Default social input send behavior * Versioning
pt-tsl
force-pushed
the
BA-2081-dissapearing-chats
branch
from
cccb2cd to
b61435b
Compare
|
Acceptance Criteria
Given I have 10 chats in my active chat list, when I switch to another Chat List Tab and come back to the Active Chat list. I should still be able to see and access the 10 original chat rooms
https://www.loom.com/share/8714eef222ea48b193593d9f36b0496d?sid=a162b289-d22f-49c7-8dec-efd9240b755e
Approvd
https://app.approvd.io/silverlogic/BA/stories/38025
Summary by CodeRabbit
Bug Fixes
Refactor