perf: Use useInfiniteQuery to prevent UI freeze in message tabs#40353
Open
TasfinMahmud wants to merge 1 commit intoRocketChat:developfrom
Open
perf: Use useInfiniteQuery to prevent UI freeze in message tabs#40353TasfinMahmud wants to merge 1 commit intoRocketChat:developfrom
TasfinMahmud wants to merge 1 commit intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
Contributor
WalkthroughReplaces eager, sequential pagination in Mentions, Starred, and Pinned message tabs with paginated streaming via Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tab as Mentions/Starred/Pinned Tab
participant Query as useInfiniteQuery
participant Server
participant List as MessageListTab/Virtuoso
User->>Tab: Mount
Tab->>Query: fetch page (pageParam=offset=0, count=COUNT)
Query->>Server: GET /api/messages?offset=0&count=COUNT
Server-->>Query: return { messages, total, count, offset }
Query-->>Tab: page data
Tab->>Tab: map & flatten pages → messages
Tab-->>List: render messages + onEndReached
User->>List: scroll to bottom
List->>Tab: onEndReached()
alt hasNextPage && !isFetchingNextPage
Tab->>Query: fetchNextPage()
Query->>Server: GET /api/messages?offset=nextOffset&count=COUNT
Server-->>Query: return next page
Query-->>Tab: page data
Tab->>List: render appended messages + isFetchingNextPage indicator
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TasfinMahmud
changed the title
coderabbitai
Bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/room/contextualBar/MessageListTab.tsx (1)
29-39:⚠️ Potential issue | 🟠 MajorMigrate PinnedMessagesTab to updated MessageListTab prop contract.
The prop contract change from
queryResultto explicit props (messages,isLoading,isSuccess,isFetchingNextPage,onEndReached) in MessageListTab requires migrating all callers. StarredMessagesTab and MentionsTab have been migrated, but PinnedMessagesTab still passesqueryResult={pinnedMessagesQueryResult}at line 43, which will cause a type error. Update PinnedMessagesTab to extract and pass individual props matching the new signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/contextualBar/MessageListTab.tsx` around lines 29 - 39, PinnedMessagesTab is still passing the old queryResult prop (pinnedMessagesQueryResult) to MessageListTab; update PinnedMessagesTab to unpack the needed fields from pinnedMessagesQueryResult and pass them individually to MessageListTab: extract messages, isLoading, isSuccess, isFetchingNextPage, and onEndReached (or a noop) from pinnedMessagesQueryResult and supply those props along with existing iconName/title/emptyResultMessage so the call matches MessageListTab's new prop contract (MessageListTab, pinnedMessagesQueryResult, PinnedMessagesTab).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/views/room/contextualBar/MentionsTab.tsx`:
- Around line 45-49: Update the guard in handleEndReached to prevent starting a
pagination fetch while any fetch is in progress: replace the check against
mentionedMessagesQueryResult.isFetchingNextPage with
mentionedMessagesQueryResult.isFetching so the condition becomes "hasNextPage &&
!isFetching" before calling mentionedMessagesQueryResult.fetchNextPage(); keep
the rest of the callback and dependency on mentionedMessagesQueryResult
unchanged.
In `@apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx`:
- Around line 46-50: The guard in handleEndReached currently checks
starredMessagesQueryResult.hasNextPage &&
!starredMessagesQueryResult.isFetchingNextPage which can allow concurrent
fetches during background refetches; change the condition to
starredMessagesQueryResult.hasNextPage && !starredMessagesQueryResult.isFetching
so fetchNextPage() is only called when no fetch (including background refetch)
is active—update the check inside the handleEndReached callback to use
isFetching and keep the call to starredMessagesQueryResult.fetchNextPage()
otherwise unchanged.
---
Outside diff comments:
In `@apps/meteor/client/views/room/contextualBar/MessageListTab.tsx`:
- Around line 29-39: PinnedMessagesTab is still passing the old queryResult prop
(pinnedMessagesQueryResult) to MessageListTab; update PinnedMessagesTab to
unpack the needed fields from pinnedMessagesQueryResult and pass them
individually to MessageListTab: extract messages, isLoading, isSuccess,
isFetchingNextPage, and onEndReached (or a noop) from pinnedMessagesQueryResult
and supply those props along with existing iconName/title/emptyResultMessage so
the call matches MessageListTab's new prop contract (MessageListTab,
pinnedMessagesQueryResult, PinnedMessagesTab).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccb99ae1-3291-4041-a754-605fe41a4e4c
📒 Files selected for processing (3)
apps/meteor/client/views/room/contextualBar/MentionsTab.tsxapps/meteor/client/views/room/contextualBar/MessageListTab.tsxapps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsxapps/meteor/client/views/room/contextualBar/MentionsTab.tsxapps/meteor/client/views/room/contextualBar/MessageListTab.tsx
🧠 Learnings (13)
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsxapps/meteor/client/views/room/contextualBar/MentionsTab.tsxapps/meteor/client/views/room/contextualBar/MessageListTab.tsx
📚 Learning: 2026-04-29T20:06:34.862Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40268
File: apps/meteor/client/startup/incomingMessages.ts:21-25
Timestamp: 2026-04-29T20:06:34.862Z
Learning: In `apps/meteor/client/startup/incomingMessages.ts`, the `Messages.state.update` predicate that strips `ignored` from records when `'ignored' in sub` is false (i.e., the subscription update has no `ignored` field) is intentional. Absence of `ignored` in a `subscriptions-changed` event means the user's ignore list is empty/reset, so clearing all existing `ignored` flags on messages for that room is the correct behavior. Do not flag this as an unintentional ignored-state reset on unrelated subscription updates.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsxapps/meteor/client/views/room/contextualBar/MessageListTab.tsx
📚 Learning: 2026-04-28T14:08:46.920Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 40105
File: apps/meteor/client/views/room/MessageList/hooks/useTryToJumpToMessage.ts:54-67
Timestamp: 2026-04-28T14:08:46.920Z
Learning: In `apps/meteor/client/views/room/MessageList/hooks/useTryToJumpToMessage.ts`, setting `isJumpingToMessage.current = true` before the guard clauses (RoomHistoryManager.isLoading check, message not found check) is intentional. The flag means "a jump is pending/in progress" and must stay `true` through all intermediate early-return paths (loading, unresolved message, etc.) so that downstream scroll and load behavior is suppressed while waiting for the jump conditions to be satisfied. Do not flag this as a "flag stuck true" bug.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsxapps/meteor/client/views/room/contextualBar/MessageListTab.tsx
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsxapps/meteor/client/views/room/contextualBar/MessageListTab.tsx
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsxapps/meteor/client/views/room/contextualBar/MessageListTab.tsx
📚 Learning: 2026-04-13T16:40:55.864Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40100
File: apps/meteor/client/views/root/MainLayout/EmbeddedPreload.tsx:18-35
Timestamp: 2026-04-13T16:40:55.864Z
Learning: In Rocket.Chat's embedded mode (`?layout=embedded`), route changes are driven by the parent frame reloading the iframe URL, which causes a full remount of the React tree (including `EmbeddedPreload` at `apps/meteor/client/views/root/MainLayout/EmbeddedPreload.tsx`). The component never survives a navigation, so `roomParams` (computed via `useMemo`) is always fresh on mount. A `[router]` singleton as the sole `useMemo` dependency is therefore correct and intentional — do not flag it as a stale-dependency bug in this component.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsxapps/meteor/client/views/room/contextualBar/MentionsTab.tsx
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsxapps/meteor/client/views/room/contextualBar/MessageListTab.tsx
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsxapps/meteor/client/views/room/contextualBar/MentionsTab.tsxapps/meteor/client/views/room/contextualBar/MessageListTab.tsx
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/client/views/room/contextualBar/MessageListTab.tsx
📚 Learning: 2026-04-17T17:38:15.994Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39858
File: packages/ui-kit/src/interactions/UserInteraction.ts:33-33
Timestamp: 2026-04-17T17:38:15.994Z
Learning: In RocketChat/Rocket.Chat (`packages/ui-kit/src/interactions/UserInteraction.ts`), `ViewSubmitUserInteraction` and `ViewClosedUserInteraction` intentionally do NOT include `rid` when the interaction originates from a **modal** surface. Modals are not scoped to a room, so no room id is available in that context. The `rid?: string` field is optional to support the contextual bar surface (where room context exists) while remaining absent for modals. Do not flag the absence of `rid` in modal viewSubmit/viewClosed interactions as a missing-context bug.
Applied to files:
apps/meteor/client/views/room/contextualBar/MessageListTab.tsx
coderabbitai
Bot
added
type: bug
and removed
type: feature
TasfinMahmud
added a commit
to TasfinMahmud/Rocket.Chat
that referenced
this pull request
Apr 30, 2026
…bbit review - Migrate PinnedMessagesTab from useQuery with blocking for-loop to useInfiniteQuery - Use isFetching instead of isFetchingNextPage in all handleEndReached guards to prevent concurrent fetches during background refetches - Addresses CodeRabbit review comments on PR RocketChat#40353
…to use useInfiniteQuery Replaces blocking sequential for-loop fetching with useInfiniteQuery for MentionsTab, StarredMessagesTab, and PinnedMessagesTab components. Before: All pages were fetched sequentially in a for loop inside useQuery, blocking the UI until every page was loaded. After: Only the first page is fetched initially. Additional pages are loaded on-demand as the user scrolls to the bottom of the list using Virtuoso's endReached callback. Changes: - MentionsTab: useQuery -> useInfiniteQuery with page-by-page fetching - StarredMessagesTab: useQuery -> useInfiniteQuery with page-by-page fetching - PinnedMessagesTab: useQuery -> useInfiniteQuery with page-by-page fetching - MessageListTab: Accept individual props instead of UseQueryResult, add endReached prop to Virtuoso, show loading indicator for next page - Use isFetching guard in handleEndReached to prevent concurrent fetches Fixes RocketChat#39237
TasfinMahmud
force-pushed
the
fix/infinite-query-mentions-starred
branch
from
3836982 to
341966e
Compare
TasfinMahmud
changed the title
TasfinMahmud
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #39237
Summary
Replaces the blocking sequential
for-loop fetching pattern in bothMentionsTabandStarredMessagesTabwithuseInfiniteQueryfrom TanStack Query, enabling page-by-page data loading triggered by user scroll.Problem
Both
MentionsTab.tsxandStarredMessagesTab.tsxused aforloop insideuseQueryto fetch all pages sequentially before rendering:This caused a blocking UI state - users couldn't see any results until every single page was loaded from the server.
Solution
MentionsTab.tsx: Migrated fromuseQuery->useInfiniteQuerywithgetNextPageParamfor cursor-based paginationStarredMessagesTab.tsx: Same migration, preservingonClientMessageReceivedprocessingMessageListTab.tsx: Updated props interface to acceptmessages[],isLoading,isSuccess,isFetchingNextPage, andonEndReachedinstead ofUseQueryResult<IMessage[]>. Added Virtuoso'sendReachedcallback for scroll-triggered loading and a bottom<Throbber>for next-page loading state.Behavior Change
useQuerywith blocking loopuseInfiniteQuerywithgetNextPageParamTesting
Summary by CodeRabbit
Performance Improvements
New Features
Bug Fixes