feat: add shared VirtualList component; migrate DiscussionsList to virtua#39394
Open
srijnabhargav wants to merge 6 commits into
Open
feat: add shared VirtualList component; migrate DiscussionsList to virtua#39394srijnabhargav wants to merge 6 commits into
srijnabhargav wants to merge 6 commits into
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
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a new generic VirtualList component using virtua, introduces CustomVirtuaScrollbars, refactors DiscussionsList to use VirtualList (removing resize-observer and height/width logic, and changing loadMoreItems to a parameterless callback), adds tests for DiscussionsList, and adds the virtua dependency. ChangesVirtual list & app usage
Scrollbars integration
DiscussionsList refactor & tests
Dependency
Sequence DiagramsequenceDiagram
participant DiscussionsList
participant VirtualList
participant Virtua as "virtua (VList)"
participant Scrollbars as "CustomVirtuaScrollbars"
DiscussionsList->>VirtualList: render(items, totalCount, renderItem, onEndReached)
VirtualList->>Virtua: initialize virtualizer (count, estimateSize)
Virtua-->>VirtualList: provide virtual items, totalSize, virtualizer handle
VirtualList->>Scrollbars: attach scroll element / forward ref
Scrollbars->>VirtualList: emit scroll events (offset, viewportSize, scrollSize)
VirtualList->>Virtua: update scroll offset / request visible items
Virtua-->>VirtualList: visible virtual item indexes
VirtualList->>DiscussionsList: call renderItem for visible items
alt near-bottom and items.length < totalCount and key changed
VirtualList->>DiscussionsList: call onEndReached()
DiscussionsList->>DiscussionsList: loadMoreItems()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
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: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx (1)
80-106: Add one regression test for pagination callback behavior.Please add coverage that
loadMoreItemsis triggered when end is reached and suppressed whenloadingis true, since this is part of the migration’s critical behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx` around lines 80 - 106, Tests for DiscussionsList lack coverage for pagination callback behavior; add a regression test that renders <DiscussionsList> with a mock loadMoreItems function and asserts that calling the component's end-reached trigger (e.g., simulate the virtualized list reaching the end or invoke the prop/handler that the component uses to detect end) calls loadMoreItems once when loading is false, and does not call it when loading is true. Use the DiscussionsList component, the loadMoreItems prop, and the loading prop to create two cases: one where loading=false and you assert the mock was called after simulating end reached, and one where loading=true and you assert the mock was not called.
🤖 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/components/VirtualList/VirtualList.tsx`:
- Around line 44-48: The effect currently calls onEndReached whenever the
onEndReached reference changes even if lastVirtualItemIndex, items.length, and
totalCount are unchanged; fix by tracking the items.length value that last
triggered onEndReached with a ref (e.g., lastTriggeredItemsLengthRef) inside the
component, and in the useEffect (where lastVirtualItemIndex, items.length,
totalCount, onEndReached are dependencies) only invoke onEndReached when the
conditions match AND lastTriggeredItemsLengthRef.current !== items.length; after
calling, set lastTriggeredItemsLengthRef.current = items.length, and reset the
ref (set to undefined or 0) when items.length decreases or when totalCount
changes if you want to allow re-triggering.
---
Nitpick comments:
In
`@apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx`:
- Around line 80-106: Tests for DiscussionsList lack coverage for pagination
callback behavior; add a regression test that renders <DiscussionsList> with a
mock loadMoreItems function and asserts that calling the component's end-reached
trigger (e.g., simulate the virtualized list reaching the end or invoke the
prop/handler that the component uses to detect end) calls loadMoreItems once
when loading is false, and does not call it when loading is true. Use the
DiscussionsList component, the loadMoreItems prop, and the loading prop to
create two cases: one where loading=false and you assert the mock was called
after simulating end reached, and one where loading=true and you assert the mock
was not called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c29b315-f7cd-40c5-b784-2aa068136b9a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
apps/meteor/client/components/VirtualList/VirtualList.tsxapps/meteor/client/components/VirtualList/index.tsapps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsxapps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.tsxapps/meteor/package.json
📜 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/Discussions/DiscussionsList.tsxapps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsxapps/meteor/client/components/VirtualList/VirtualList.tsxapps/meteor/client/components/VirtualList/index.ts
🧠 Learnings (14)
📚 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/package.json
📚 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/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.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/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.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/Discussions/DiscussionsList.spec.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/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.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/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.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/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.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/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.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/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/components/VirtualList/index.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/client/components/VirtualList/index.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/client/components/VirtualList/index.ts
🔇 Additional comments (3)
apps/meteor/package.json (1)
160-160: Reviewed this dependency addition; no actionable issue in this segment.apps/meteor/client/components/VirtualList/index.ts (1)
1-1: Barrel export looks correct.This cleanly exposes the shared VirtualList entrypoint for consumers.
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.tsx (1)
26-26: Migration wiring is consistent.The
loadMoreItems: () => voidcontract andVirtualListintegration are aligned with the new shared virtual list API.Also applies to: 86-94
Contributor
Author
|
@KevLehman @MartinSchoeler Could you please review and let me know if any changes are needed? |
KevLehman
added
the
valid
coderabbitai
Bot
removed
the
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx (2)
62-67: ⚡ Quick winThe
@rocket.chat/ui-contextsmock replaces the entire module without spreadingjest.requireActual.If
DiscussionsList(or any component in its tree) imports anything else from@rocket.chat/ui-contextsbeyond the four hooks listed here (useSetting,useLayoutSizes,useLayoutContextualBarPosition,useRoomToolbox), those imports will silently beundefinedat runtime in the test, potentially causing cryptic failures.♻️ Proposed fix
jest.mock('@rocket.chat/ui-contexts', () => ({ + ...jest.requireActual('@rocket.chat/ui-contexts'), useSetting: () => false, useLayoutSizes: () => ({ contextualBar: 'wide' }), useLayoutContextualBarPosition: () => 'side', useRoomToolbox: () => ({ closeTab: jest.fn() }), }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx` around lines 62 - 67, The current jest.mock for '@rocket.chat/ui-contexts' overwrites the entire module and may make other imports undefined; update the mock to spread the real module using const actual = jest.requireActual('@rocket.chat/ui-contexts') and return { ...actual, useSetting: () => false, useLayoutSizes: () => ({ contextualBar: 'wide' }), useLayoutContextualBarPosition: () => 'side', useRoomToolbox: () => ({ closeTab: jest.fn() }) } so only the four hooks (useSetting, useLayoutSizes, useLayoutContextualBarPosition, useRoomToolbox) are overridden while preserving all other exports.
8-13: 💤 Low valueCode comments in implementation files violate the project guideline.
Lines 8–9 and 12 contain block comments explaining the mock rationale. As per coding guidelines, code comments should be avoided in
*.{ts,tsx,js}files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx` around lines 8 - 13, Remove the inline block comments surrounding the two jest.mock calls (the comments on DiscussionsListRow and goToRoomById) and leave only the mock declarations so the test file contains no implementation comments; keep the existing mocks for DiscussionsListRow (the JSX sentinel) and the goToRoomById mock (jest.fn()), and if you need to preserve the rationale, move it into the test description or the PR/commit message instead of keeping comments in the .tsx file.apps/meteor/client/components/VirtualList/VirtualList.tsx (2)
67-71: ⚡ Quick winMemoize the
items.map(...)element array to preserve VList's internal optimizations.In complex usage, especially if you re-render frequently the parent of virtual scroller or the children are tons of items, children element creation can be a performance bottleneck. Creating React elements is fast enough but not free, and new React element instances break some of memoization inside virtual scroller. One solution is memoization with
useMemoto reduce computation and keep the elements' instances the same. If you want to pass state from parent to the items, using context instead of props may be better because it doesn't break the memoization.♻️ Proposed refactor
+import { useCallback, useEffect, useMemo, useRef } from 'react'; + const rowElements = useMemo( + () => + items.map((item, index) => ( + <div key={item._id} style={{ minHeight: estimateSize(index) }}> + {renderItem(item, index)} + </div> + )), + [items, estimateSize, renderItem], + ); return ( <CustomVirtuaScrollbars> <VList ref={virtualizerRef} style={{ height: '100%' }} onScroll={handleScroll}> - {items.map((item, index) => ( - <div key={item._id} style={{ minHeight: estimateSize(index) }}> - {renderItem(item, index)} - </div> - ))} + {rowElements} </VList> </CustomVirtuaScrollbars> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/components/VirtualList/VirtualList.tsx` around lines 67 - 71, The items.map(...) JSX should be memoized to avoid recreating child elements on every render; inside the VirtualList component, create a useMemo (e.g., const renderedItems = useMemo(() => items.map((item, index) => <div key={item._id} style={{minHeight: estimateSize(index)}}>{renderItem(item, index)}</div>), [items, estimateSize, renderItem]) and then render {renderedItems} instead of calling items.map inline; this preserves element instances for the virtual scroller and references the unique symbols items, estimateSize, renderItem, and the VirtualList component.
9-15: 💤 Low value
estimateSizeprop name is misleading — it acts as a CSSminHeight, not a Virtua size estimator.Virtua's
VListhas noestimateSizeprop (unlike TanStack Virtual). The current code applies the return value asstyle={{ minHeight: estimateSize(index) }}on a wrapperdiv. Calling the propestimateSizeborrows TanStack Virtual terminology and will confuse readers into expecting Virtua-style measurement behavior. ConsideritemMinHeightorminItemSizeto accurately convey its role.♻️ Proposed rename
- estimateSize?: (index: number) => number; + minItemSize?: (index: number) => number; function VirtualList<T extends { _id: string }>({ items, totalCount, renderItem, - estimateSize = () => 120, + minItemSize = () => 120, onEndReached, }: VirtualListProps<T>) { <div key={item._id} style={{ minHeight: estimateSize(index) }}> + <div key={item._id} style={{ minHeight: minItemSize(index) }}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/components/VirtualList/VirtualList.tsx` around lines 9 - 15, Rename the misleading prop estimateSize in the VirtualList component to a clearer name like itemMinHeight (or minItemSize) across the VirtualListProps type and the VirtualList component implementation; update the prop signature in type VirtualListProps<T extends { _id: string }>, change any parameter and destructuring in the VirtualList function that references estimateSize, and replace uses where you set style={{ minHeight: estimateSize(index) }} to style={{ minHeight: itemMinHeight(index) }} (and adjust any default/optional handling). Also update all external call sites and tests that pass estimateSize to use the new name to keep the API consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/client/components/VirtualList/VirtualList.tsx`:
- Around line 32-62: The list never calls onEndReached when initial items don't
fill the viewport because handleScroll is only invoked on user scroll; add a
mount/visibility-time check using the virtualizer's onRangeChange callback (or
equivalent VList.onRangeChange) to run the same end-of-list logic used in
handleScroll. Reuse the same guards and key deduplication (lastEndReachKeyRef,
items.length, totalCount, NEAR_BOTTOM_THRESHOLD, virtualizerRef,
onEndReachedRef) when implementing the onRangeChange handler so it triggers
loadMore on mount if the rendered range is already near the bottom or the list
is underfilled; ensure you still keep the viewportSize <= 0 startup guard and
avoid duplicating behavior by delegating to the existing handleScroll logic or
extracting the common check into a helper invoked by both handlers.
In
`@packages/ui-client/src/components/CustomScrollbars/CustomVirtuaScrollbars.tsx`:
- Around line 14-29: The initialized event handler passed into
useOverlayScrollbars captures the incoming prop ref in a stale closure; update
the code so the handler always forwards the current ref instead of the one from
first render—e.g., create a stable mutable container (like latestRefRef =
useRef(ref)), update latestRefRef.current in a useEffect when ref changes, and
have the events.initialized callback read latestRefRef.current before forwarding
the viewport (the handler that calls ref(viewport) / ref.current = viewport and
accesses osInstance.elements()). This preserves the existing behavior but
ensures function refs passed from parents are invoked with the latest reference.
---
Nitpick comments:
In `@apps/meteor/client/components/VirtualList/VirtualList.tsx`:
- Around line 67-71: The items.map(...) JSX should be memoized to avoid
recreating child elements on every render; inside the VirtualList component,
create a useMemo (e.g., const renderedItems = useMemo(() => items.map((item,
index) => <div key={item._id} style={{minHeight:
estimateSize(index)}}>{renderItem(item, index)}</div>), [items, estimateSize,
renderItem]) and then render {renderedItems} instead of calling items.map
inline; this preserves element instances for the virtual scroller and references
the unique symbols items, estimateSize, renderItem, and the VirtualList
component.
- Around line 9-15: Rename the misleading prop estimateSize in the VirtualList
component to a clearer name like itemMinHeight (or minItemSize) across the
VirtualListProps type and the VirtualList component implementation; update the
prop signature in type VirtualListProps<T extends { _id: string }>, change any
parameter and destructuring in the VirtualList function that references
estimateSize, and replace uses where you set style={{ minHeight:
estimateSize(index) }} to style={{ minHeight: itemMinHeight(index) }} (and
adjust any default/optional handling). Also update all external call sites and
tests that pass estimateSize to use the new name to keep the API consistent.
In
`@apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx`:
- Around line 62-67: The current jest.mock for '@rocket.chat/ui-contexts'
overwrites the entire module and may make other imports undefined; update the
mock to spread the real module using const actual =
jest.requireActual('@rocket.chat/ui-contexts') and return { ...actual,
useSetting: () => false, useLayoutSizes: () => ({ contextualBar: 'wide' }),
useLayoutContextualBarPosition: () => 'side', useRoomToolbox: () => ({ closeTab:
jest.fn() }) } so only the four hooks (useSetting, useLayoutSizes,
useLayoutContextualBarPosition, useRoomToolbox) are overridden while preserving
all other exports.
- Around line 8-13: Remove the inline block comments surrounding the two
jest.mock calls (the comments on DiscussionsListRow and goToRoomById) and leave
only the mock declarations so the test file contains no implementation comments;
keep the existing mocks for DiscussionsListRow (the JSX sentinel) and the
goToRoomById mock (jest.fn()), and if you need to preserve the rationale, move
it into the test description or the PR/commit message instead of keeping
comments in the .tsx file.
🪄 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: 6848c230-ac11-4c11-a971-acc595010368
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
apps/meteor/client/components/VirtualList/VirtualList.tsxapps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsxapps/meteor/package.jsonpackages/ui-client/src/components/CustomScrollbars/CustomVirtuaScrollbars.tsxpackages/ui-client/src/components/CustomScrollbars/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/ui-client/src/components/CustomScrollbars/index.ts
📜 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:
packages/ui-client/src/components/CustomScrollbars/CustomVirtuaScrollbars.tsxapps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsxapps/meteor/client/components/VirtualList/VirtualList.tsx
🧠 Learnings (3)
📚 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:
packages/ui-client/src/components/CustomScrollbars/CustomVirtuaScrollbars.tsxapps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsxapps/meteor/client/components/VirtualList/VirtualList.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/ui-client/src/components/CustomScrollbars/CustomVirtuaScrollbars.tsxapps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsxapps/meteor/client/components/VirtualList/VirtualList.tsx
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/views/room/contextualBar/Discussions/DiscussionsList.spec.tsx
🔇 Additional comments (2)
apps/meteor/package.json (1)
278-278: ⚡ Quick win
react-virtuosois still needed — it remains widely used throughout the codebase (27+ files including ThreadList, RoomMembers, RoomFiles, MessageSearch, VideoConfList, OmniChannel components, and more). WhileDiscussionsListmay have been migrated tovirtua, this does not eliminate the dependency. The^0.49.0version range is appropriate and already covers patch version 0.49.1.> Likely an incorrect or invalid review comment.packages/ui-client/src/components/CustomScrollbars/CustomVirtuaScrollbars.tsx (1)
31-44: ⚡ Quick winBaseScrollbars does not wrap children in intermediate containers.
BaseScrollbars is a simple Box wrapper that directly spreads props (including children) to the underlying Box element. There are no padding divs, flex wrappers, or other intermediate elements between the Box root and its rendered children. Therefore,
root.firstElementChildcorrectly points to the first child element passed toCustomVirtuaScrollbars, not to an intermediate wrapper.If a potential issue exists, it would arise from callers of
CustomVirtuaScrollbarswrapping their VList in intermediate containers before passing it as children—but that is a consumer responsibility, not a problem with BaseScrollbars itself.> Likely an incorrect or invalid review comment.
srijnabhargav
changed the title
MartinSchoeler
requested changes
May 12, 2026
Member
MartinSchoeler
left a comment
MartinSchoeler
left a comment
There was a problem hiding this comment.
Check the style linting, as the CI did flag some errors.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39394 +/- ##
===========================================
+ Coverage 69.73% 69.84% +0.11%
===========================================
Files 3326 3330 +4
Lines 123056 123269 +213
Branches 21962 22016 +54
===========================================
+ Hits 85816 86101 +285
+ Misses 33880 33806 -74
- Partials 3360 3362 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
MartinSchoeler
requested changes
May 15, 2026
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
1 issue found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Contributor
There was a problem hiding this comment.
2 issues found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Introduces a generic VirtualList component backed by the virtua Virtualizer. Supports end-reach callbacks (with async lock), configurable overscan, and a semantic ul/li structure via VirtuaListContainer. Pairs with the existing CustomVirtuaScrollbars from @rocket.chat/ui-client.
Replaces the Virtuoso + VirtualizedScrollbars + useResizeObserver setup with the new VirtualList component. The loadMoreItems signature simplifies from (start, end) to () => void, deferring pagination logic to the data layer.
- New VirtualList.spec.tsx covers end-reach, overscan, and scroll behaviour
using a mocked Virtualizer handle
- DiscussionsList.spec.tsx updated for the virtua-backed VirtualList (new
snapshot generated)
- MessageList.spec.tsx: replace require('react') with jest.requireActual to
silence @typescript-eslint/no-require-imports
srijnabhargav
force-pushed
the
feat/tanstack-virtual-poc
branch
from
571a03f to
4f26f66
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
Author
|
@cubic-dev-ai review |
Contributor
@srijnabhargav I have started the AI code review. It will take a few minutes to complete. |
MartinSchoeler
requested changes
May 28, 2026
Member
MartinSchoeler
left a comment
MartinSchoeler
left a comment
There was a problem hiding this comment.
Left some comments and suggestions
Contributor
Author
|
Addressed all review comments |
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
4 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.
Proposed changes (including videos or screenshots)
This is a POC for migrating the codebase from react-virtuoso to @tanstack/react-virtual. The goal, as discussed in the channel, is to replace the scattered Virtuoso + VirtualizedScrollbars + useResizeObserver pattern with a single shared headless component.
What's in this PR
New
VirtualListcomponent (apps/meteor/client/components/VirtualList/)A generic
VirtualList<T>built onuseVirtualizerfrom@tanstack/react-virtual.It wraps
CustomScrollbars, handles the absolute-positioning pattern, dynamic row measurement viameasureElement, and infinite scroll viaonEndReached.It exposes the following options directly from the TanStack Virtual API:
estimateSizeoverscangappaddingStartpaddingEndDiscussionsListmigratedChosen as the first target because it is a pure presentational component with a clean, representative pattern (search + infinite scroll) without the additional complexity of real-time updates or grouped views.
Changes:
Virtuoso,VirtualizedScrollbars, anduseResizeObserverloadMoreItemsprop type from:to:
()=>voidsince the parent component already passes it this way.
Tests added for DiscussionsList
Added 4 unit tests covering:
Demo - UI looks unchanged! No Regression
Screen.Recording.2026-03-06.at.12.39.44.AM.mov
Issue(s)
Part of the TanStack Virtual migration discussed in the GSoC channel.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores