fix: Refactor Starred Messages to use useInfiniteQuery for non-blockin…#39238
Open
iramsk02 wants to merge 1 commit intoRocketChat:developfrom
Open
fix: Refactor Starred Messages to use useInfiniteQuery for non-blockin…#39238iramsk02 wants to merge 1 commit intoRocketChat:developfrom
iramsk02 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
WalkthroughMessage list contextual components were updated to support paginated infinite queries: MessageListTab accepts either standard or infinite query results, flattens paginated data into a memoized messages array and exposes an end-reached handler; StarredMessagesTab was migrated from useQuery to useInfiniteQuery with page-based fetching. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as MessageListTab (Virtuoso)
participant Q as ReactQuery (useInfiniteQuery)
participant API as Server API
User->>UI: scrolls to end
UI->>Q: call fetchNextPage()
Q->>API: GET /messages?offset=pageParam&count=25
API-->>Q: { messages: [...], total: N }
Q-->>UI: new page appended (pages -> flattened messages)
UI->>User: render additional messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
iramsk02
changed the title
coderabbitai
bot
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 2 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![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
🧹 Nitpick comments (1)
apps/meteor/client/views/room/contextualBar/MessageListTab.tsx (1)
35-47: Replaceanywith typed page structure to ensure type safety in message flattening.Line 35 uses
UseInfiniteQueryResult<any>and line 46 usespage: any, which allows non-array objects into themessagesarray. Introduce a concrete page type that matches your query structure:♻️ Proposed typed flattening
+type InfiniteMessagePage = IMessage[] | { items?: IMessage[]; messages?: IMessage[] }; + type MessageListTabProps = { iconName: IconName; title: ReactNode; emptyResultMessage: string; context: MessageActionContext; - queryResult: UseQueryResult<IMessage[]> | UseInfiniteQueryResult<any>; + queryResult: UseQueryResult<IMessage[]> | UseInfiniteQueryResult<InfiniteMessagePage>; }; @@ - return queryResult.data.pages.flatMap((page: any) => page.items || page.messages || page); + return queryResult.data.pages.flatMap((page) => { + if (Array.isArray(page)) { + return page; + } + return page.items ?? page.messages ?? []; + });🤖 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 35 - 47, The messages flattening uses untyped infinite-query pages so non-array values can leak into messages; define a concrete page type (e.g., interface InfiniteMessagesPage { items?: IMessage[]; messages?: IMessage[]; data?: IMessage[] } or similar to match your API) and replace UseInfiniteQueryResult<any> with UseInfiniteQueryResult<InfiniteMessagesPage> in MessageListTabProps, then update the useMemo to type the page parameter as InfiniteMessagesPage when doing queryResult.data.pages.flatMap to ensure the fallback expressions (items/messages/data) are strongly typed and only IMessage[] are returned by MessageListTab's messages variable.
🤖 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/MessageListTab.tsx`:
- Around line 58-62: The handleEndReached handler should avoid firing duplicate
page loads by checking the queryResult.isFetchingNextPage flag before calling
queryResult.fetchNextPage; update the handleEndReached callback (the function
named handleEndReached that currently checks 'fetchNextPage' in queryResult and
queryResult.hasNextPage) to return early if queryResult.isFetchingNextPage is
true, and when invoking queryResult.fetchNextPage call it without awaiting
(marking the Promise as intentionally unawaited, e.g., void
queryResult.fetchNextPage()) so concurrent fetches are prevented.
In `@apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx`:
- Line 21: In StarredMessagesTab
(apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx) remove the
inline implementation comment "// Map and normalize messages for THIS page only"
from inside the component/handler where messages are being mapped; do not
replace it with another implementation-style comment—if documentation is needed,
move it to a top-level JSDoc or commit message instead.
---
Nitpick comments:
In `@apps/meteor/client/views/room/contextualBar/MessageListTab.tsx`:
- Around line 35-47: The messages flattening uses untyped infinite-query pages
so non-array values can leak into messages; define a concrete page type (e.g.,
interface InfiniteMessagesPage { items?: IMessage[]; messages?: IMessage[];
data?: IMessage[] } or similar to match your API) and replace
UseInfiniteQueryResult<any> with UseInfiniteQueryResult<InfiniteMessagesPage> in
MessageListTabProps, then update the useMemo to type the page parameter as
InfiniteMessagesPage when doing queryResult.data.pages.flatMap to ensure the
fallback expressions (items/messages/data) are strongly typed and only
IMessage[] are returned by MessageListTab's messages variable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/views/room/contextualBar/MessageListTab.tsxapps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/MessageListTab.tsxapps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
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').
📚 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/MessageListTab.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/MessageListTab.tsxapps/meteor/client/views/room/contextualBar/StarredMessagesTab.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/MessageListTab.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/room/contextualBar/MessageListTab.tsx (1)
packages/ui-contexts/src/index.ts (2)
useUserPreference(86-86)useRoomToolbox(62-62)
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx (1)
apps/meteor/client/lib/queryKeys.ts (1)
roomsQueryKeys(13-38)
🔇 Additional comments (2)
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx (1)
16-34: Good infinite pagination flow.The
useInfiniteQuerysetup (pagedqueryFn,initialPageParam, andgetNextPageParam) is clean and aligns with the non-blocking loading objective.apps/meteor/client/views/room/contextualBar/MessageListTab.tsx (1)
54-56: Close button callback wiring looks correct.This callback cleanly delegates to
closeTaband restores the expected contextual bar close behavior.
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
Outdated
Show resolved
Hide resolved
iramsk02
force-pushed
the
fix/refactor-starred-messages-infinite-query
branch
from
b044f15 to
884acab
Compare
coderabbitai
bot
added
type: bug
and removed
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/room/contextualBar/MessageListTab.tsx (1)
35-47: Tighten infinite-query typing and page flattening.Lines 35 and 46 rely on
anyand|| page, which can leak non-message objects intomessages. The only infinite-query consumer (StarredMessagesTab) returns{ messages, total }, but the fallback|| pagecould pass the entire page object (including metadata) if the structure diverges. Prefer explicit page typing and null-coalescing (??) over short-circuit operators.Proposed refactor
-type MessageListTabProps = { +type InfiniteMessagesPage = { messages: IMessage[]; total: number } | { items: IMessage[]; itemCount: number }; + +type MessageListTabProps = { iconName: IconName; title: ReactNode; emptyResultMessage: string; context: MessageActionContext; - queryResult: UseQueryResult<IMessage[]> | UseInfiniteQueryResult<any>; + queryResult: UseQueryResult<IMessage[]> | UseInfiniteQueryResult<InfiniteMessagesPage>; }; @@ - return queryResult.data.pages.flatMap((page: any) => page.items || page.messages || page); + return queryResult.data.pages.flatMap((page) => { + if (Array.isArray(page)) { + return page; + } + return (page.items ?? page.messages ?? []) as IMessage[]; + }); }, [queryResult.data]);🤖 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 35 - 47, The messages useMemo in MessageListTab accepts a loosely typed queryResult and flattens pages with "any" and a fallback "|| page", which can leak non-message objects; tighten the typing by specifying UseInfiniteQueryResult<{ messages: IMessage[]; total?: number }> (or the exact page shape used by StarredMessagesTab) instead of any, and change the flatten logic in the useMemo to explicitly pick known arrays and use null-coalescing (e.g., page.messages ?? page.items ?? []) when building messages; update the MessageListTabProps queryResult type and the useMemo flattening to rely on these explicit types and null-coalescing so only IMessage[] elements are returned.
🤖 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/StarredMessagesTab.tsx`:
- Line 19: The pagination call in StarredMessagesTab uses a test page size
(getStarredMessages({ roomId: room._id, offset, count: 2 })) which should be
replaced with the normal page size constant or value to avoid excessive
requests; update the call in the StarredMessagesTab component to use the shared
page-size constant (or 25) instead of hardcoded 2, ensuring any imports or
references to the constant (e.g., PAGE_SIZE or USERS_PER_PAGE) are
added/adjusted accordingly.
---
Nitpick comments:
In `@apps/meteor/client/views/room/contextualBar/MessageListTab.tsx`:
- Around line 35-47: The messages useMemo in MessageListTab accepts a loosely
typed queryResult and flattens pages with "any" and a fallback "|| page", which
can leak non-message objects; tighten the typing by specifying
UseInfiniteQueryResult<{ messages: IMessage[]; total?: number }> (or the exact
page shape used by StarredMessagesTab) instead of any, and change the flatten
logic in the useMemo to explicitly pick known arrays and use null-coalescing
(e.g., page.messages ?? page.items ?? []) when building messages; update the
MessageListTabProps queryResult type and the useMemo flattening to rely on these
explicit types and null-coalescing so only IMessage[] elements are returned.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/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/MessageListTab.tsx
🧠 Learnings (9)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
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').
📚 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: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.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-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: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.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.tsx
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/room/contextualBar/MessageListTab.tsx (3)
packages/ui-contexts/src/index.ts (2)
useUserPreference(86-86)useRoomToolbox(62-62)packages/ui-contexts/src/hooks/useRoomToolbox.ts (1)
useRoomToolbox(6-6)packages/ui-client/src/components/Contextualbar/index.ts (1)
ContextualbarEmptyContent(34-34)
🔇 Additional comments (1)
apps/meteor/client/views/room/contextualBar/MessageListTab.tsx (1)
58-62: Nice guard for boundary-triggered pagination.Line 59’s
!queryResult.isFetchingNextPagecheck plusvoid queryResult.fetchNextPage()is the right non-blocking pattern here.
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
Outdated
Show resolved
Hide resolved
iramsk02
force-pushed
the
fix/refactor-starred-messages-infinite-query
branch
from
884acab to
322ddd1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx (1)
21-21: Remove stray whitespace line.This line appears to be leftover from the removed inline comment. Consider removing the empty whitespace.
Suggested fix
const result = await getStarredMessages({ roomId: room._id, offset, count: 25 }); - - + const messages = await Promise.all(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx` at line 21, In the StarredMessagesTab.tsx component remove the stray empty whitespace line left from a removed inline comment (the blank line near the top of the StarredMessagesTab component file); simply delete that empty line so there are no unnecessary blank lines at the start of the file and ensure the surrounding imports/exports (e.g., the StarredMessagesTab component declaration/export) remain intact.apps/meteor/client/views/room/contextualBar/MessageListTab.tsx (1)
35-35: Consider adding a more specific type forUseInfiniteQueryResult.Using
anyhere loses type safety for paginated data. Consider defining a page shape type to improve inference and catch potential mismatches:type MessagePage = { messages: IMessage[]; total: number }; queryResult: UseQueryResult<IMessage[]> | UseInfiniteQueryResult<InfiniteData<MessagePage>>;This is optional since the current approach works and the memoized derivation handles the shape defensively.
🤖 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` at line 35, The union type for queryResult currently uses UseInfiniteQueryResult<any>, which loses type safety; define a specific page shape (e.g., MessagePage with messages: IMessage[] and total: number) and replace the any with UseInfiniteQueryResult<InfiniteData<MessagePage>> (or UseInfiniteQueryResult<MessagePage> depending on your imports) so paginated results are strongly typed; update the type of queryResult accordingly and adjust any downstream uses that assume the page shape to use the new MessagePage/InfiniteData structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/views/room/contextualBar/MessageListTab.tsx`:
- Line 35: The union type for queryResult currently uses
UseInfiniteQueryResult<any>, which loses type safety; define a specific page
shape (e.g., MessagePage with messages: IMessage[] and total: number) and
replace the any with UseInfiniteQueryResult<InfiniteData<MessagePage>> (or
UseInfiniteQueryResult<MessagePage> depending on your imports) so paginated
results are strongly typed; update the type of queryResult accordingly and
adjust any downstream uses that assume the page shape to use the new
MessagePage/InfiniteData structure.
In `@apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx`:
- Line 21: In the StarredMessagesTab.tsx component remove the stray empty
whitespace line left from a removed inline comment (the blank line near the top
of the StarredMessagesTab component file); simply delete that empty line so
there are no unnecessary blank lines at the start of the file and ensure the
surrounding imports/exports (e.g., the StarredMessagesTab component
declaration/export) remain intact.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/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/MessageListTab.tsx
🧠 Learnings (11)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
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').
📚 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: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
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-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: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.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-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx (2)
apps/meteor/client/lib/queryKeys.ts (1)
roomsQueryKeys(13-38)packages/livechat/src/lib/room.js (1)
messages(258-258)
🔇 Additional comments (5)
apps/meteor/client/views/room/contextualBar/StarredMessagesTab.tsx (2)
18-28: LGTM! Clean infinite query implementation.The pagination logic is correct:
pageParamas offset, proper page size (25), andgetNextPageParamcorrectly sums loaded messages to compute the next offset. The message processing pipeline withPromise.allhandles async normalization appropriately.
30-34: LGTM! Correct pagination boundary logic.The
getNextPageParamimplementation properly accumulates loaded message counts across all pages and returnsundefinedwhen all items are fetched, which correctly signals to React Query that no more pages exist.apps/meteor/client/views/room/contextualBar/MessageListTab.tsx (3)
39-47: LGTM! Well-structured type narrowing.The memoized derivation correctly handles both query result shapes: direct array for
UseQueryResultand flattened pages forUseInfiniteQueryResult. TheArray.isArraycheck is a clean runtime type guard.
58-62: LGTM! Proper infinite scroll guard implemented.The
isFetchingNextPagecheck prevents duplicate fetch calls whenendReachedfires multiple times at the scroll boundary. Thevoidprefix correctly marks the promise as intentionally unawaited.
88-94: LGTM! Clean Virtuoso integration.The
endReachedcallback is properly wired, and theitemContentcorrectly references the derivedmessagesarray for both current and previous message lookups, ensuring consistent behavior between standard and infinite query modes.
Author
|
@dougfabris Can you check this out?? |
KevLehman
added
the
valid
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
3 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.
refactor: Use useInfiniteQuery for non-blocking Starred Messages pagination
Proposed changes
Refactored the Starred Messages tab to use
@tanstack/react-query'suseInfiniteQueryfor non-blocking, streamed pagination.The big picture:
Previously, opening the Starred Messages sidebar triggered a sequential
awaitloop (waterfall) that fetched every single page of starred messages before rendering any results. This blocked the UI and caused significant delays for users with many starred items.Technical context:
UseQueryResultandUseInfiniteQueryResultusing type narrowing (Array.isArray).Virtuosocan still render a single continuous list while supporting background fetching.endReachedcallback inVirtuosoto trigger thefetchNextPagestreaming automatically.Issue(s)
Relates to performance goals for the Activity Hub project. #39237.
Steps to test or reproduce
offset=25is triggered.For testing, the offset is 2
https://github.com/user-attachments/assets/4f3c2baa-b783-4aaf-be83-c4a112343077
Summary by CodeRabbit
New Features
Improvements