Fixed virtual list windowing for large admin lists#26952
Fixed virtual list windowing for large admin lists#26952jonatansberg wants to merge 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a virtualized list window module and 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
jonatansberg
force-pushed
the
ber-3470-fix-massive-member-counts
branch
from
bab11b1 to
67f84d9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/posts/src/views/members/member-query-params.ts (1)
59-68: Prefer a shared constant for member page size across modules.
'100'is now hardcoded here and inapps/admin-x-framework/src/api/members.ts. A shared exported constant would reduce drift risk between list query builders and infinite-query defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/member-query-params.ts` around lines 59 - 68, The hardcoded page size '100' in buildMemberListSearchParams should be replaced with a shared exported constant to avoid drift; add a new exported constant (e.g. MEMBERS_PAGE_SIZE) in a common/shared module and import it here, then replace the literal '100' in buildMemberListSearchParams with String(MEMBERS_PAGE_SIZE); also update the other usage in apps/admin-x-framework (members list/infinite-query defaults) to import and use the same MEMBERS_PAGE_SIZE constant so both query builders and infinite-query defaults stay in sync.
🤖 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/posts/src/components/virtual-table/virtual-list-window.ts`:
- Around line 21-55: Guard against non-positive windowSize by normalizing it to
a positive integer wherever it can be passed in: in getNextUnlockedItemCount and
in useVirtualListWindow. Ensure the local windowSize used for state and for
fetchMore is replaced with a validated value (e.g., Math.max(1, windowSize) or
defaulting to VIRTUAL_LIST_WINDOW_SIZE) so calls to
getNextUnlockedItemCount(unlockedItemCount, windowSize) and the initial
useState(windowSize) cannot receive 0 or negative values; update the
normalization in the useVirtualListWindow parameter handling and before invoking
setUnlockedItemCount in fetchMore.
---
Nitpick comments:
In `@apps/posts/src/views/members/member-query-params.ts`:
- Around line 59-68: The hardcoded page size '100' in
buildMemberListSearchParams should be replaced with a shared exported constant
to avoid drift; add a new exported constant (e.g. MEMBERS_PAGE_SIZE) in a
common/shared module and import it here, then replace the literal '100' in
buildMemberListSearchParams with String(MEMBERS_PAGE_SIZE); also update the
other usage in apps/admin-x-framework (members list/infinite-query defaults) to
import and use the same MEMBERS_PAGE_SIZE constant so both query builders and
infinite-query defaults stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36b206ef-3685-411c-910d-2d2de0196aed
📒 Files selected for processing (11)
apps/admin-x-framework/src/api/members.tsapps/posts/src/components/virtual-table/virtual-list-window.test.tsapps/posts/src/components/virtual-table/virtual-list-window.tsapps/posts/src/views/Tags/components/tags-list.tsxapps/posts/src/views/comments/comments.tsxapps/posts/src/views/comments/components/comments-list.tsxapps/posts/src/views/members/components/members-layout.tsxapps/posts/src/views/members/components/members-list-item.tsxapps/posts/src/views/members/components/members-list.tsxapps/posts/src/views/members/member-query-params.test.tsapps/posts/src/views/members/member-query-params.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 455e8c3363
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) { | ||
| const {search} = useLocation(); | ||
| const effectiveResetKey = resetKey ?? search; | ||
| const [unlockedItemCount, setUnlockedItemCount] = useState(windowSize); |
There was a problem hiding this comment.
useVirtualListWindow always reinitializes unlockedItemCount to windowSize on mount, so any rows unlocked via “Fetch more” are lost when the list unmounts/remounts (for example, opening a member and then using Back). In that flow, useScrollRestoration can no longer restore positions past the first 1,000 rows because the restored scroll offset is clamped to the smaller window, forcing users to re-unlock data and losing their place in large lists.
Useful? React with 👍 / 👎.
rob-ghost
force-pushed
the
ber-3470-fix-massive-member-counts
branch
from
15c8870 to
455e8c3
Compare
rob-ghost
added
deploy-to-staging
rob-ghost
added
deploy-to-staging
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 198aa01a09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (entryScopedScrollPositionKey) { | ||
| scrollPositions.set(entryScopedScrollPositionKey, position); | ||
| } | ||
| setStoredScrollPosition(getCurrentHistoryState(), key, position); |
There was a problem hiding this comment.
In persistScrollPosition, the history state is read at write time (getCurrentHistoryState()), but this function is also called from the effect cleanup during navigation. At that point React Router has already moved to the destination entry, so the previous page's scroll position gets written into the new entry instead of the one being left. This means quick scroll-then-navigate flows (e.g. open a member right after scrolling) can restore to an older position when returning, because the source entry never receives the final flushed value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/posts/src/components/virtual-table/use-scroll-restoration.tsx (1)
230-244:⚠️ Potential issue | 🟠 MajorTrack and clear all retry timeouts, not just the initial one.
The cleanup function only cancels the initial 150ms timeout. If
attemptRestoreschedules a retry on line 232 before cleanup runs, that retry persists after navigation or unmount. When it fires, it executes with stalesavedPositionand location context, restoring incorrect scroll positions.Store retry timeout IDs in a ref (e.g.,
Set<number>) and clear them all during cleanup to prevent post-navigation scroll writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/components/virtual-table/use-scroll-restoration.tsx` around lines 230 - 244, The cleanup only clears the initial timeoutId so retries scheduled by attemptRestore can fire after unmount; modify use-scroll-restoration to track all retry timers (e.g., keep a ref holding a Set<number> of timeout IDs) and, whenever you call setTimeout in attemptRestore or for the initial schedule, add the returned id to that Set, and on cleanup iterate the Set calling clearTimeout for each id and clear the Set; ensure symbols mentioned (attemptRestore, timeoutId, savedPosition, maxAttempts, setTimeout, clearTimeout, scrollContainer) are updated accordingly so no retry timeout can run with stale savedPosition or location context.
🧹 Nitpick comments (2)
e2e/helpers/pages/admin/members/members-page.ts (1)
109-110: Expose the new locators aspublic readonly.These additions don't follow the repo's page-object locator convention. Making them explicit
public readonlykeeps the helper aligned with the rest of the shared page-object API.As per coding guidelines, "Page Objects should be located in
helpers/pages/and expose locators aspublic readonly".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/members/members-page.ts` around lines 109 - 110, The new locators fetchMoreButton and membersList are declared without the repo's page-object visibility convention; update their declarations to be public readonly so they match the rest of the page-object API (e.g., change the declarations for fetchMoreButton and membersList in the MembersPage helper to use "public readonly" visibility).e2e/tests/admin/members/virtual-window.test.ts (1)
94-114: Use a semantic locator for the main region instead ofdocument.querySelector('main').The test reaches into the page with raw CSS selectors twice here. That makes the scroll assertions more brittle than they need to be and breaks the repo's E2E locator rule. A
page.getByRole('main')locator can be reused for both thescrollTopread and the restoration poll.As per coding guidelines, "Prefer semantic locators over test IDs, and never use CSS selectors, XPath, nth-child, or class names".♻️ One way to rewrite it
+ const mainRegion = page.getByRole('main'); const targetRow = { index: Number(await targetRowLocator.getAttribute('data-index')), - scrollTop: await page.evaluate(() => document.querySelector('main')?.scrollTop || 0) + scrollTop: await mainRegion.evaluate(element => (element as HTMLElement).scrollTop) }; @@ - await page.waitForFunction(({scrollTop}) => { - const main = document.querySelector('main'); - - if (!main) { - return false; - } - - return Math.abs(main.scrollTop - scrollTop) < 500; - }, {scrollTop: targetRow.scrollTop}); + await expect.poll(async () => { + const scrollTop = await mainRegion.evaluate(element => (element as HTMLElement).scrollTop); + return Math.abs(scrollTop - targetRow.scrollTop); + }).toBeLessThan(500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/members/virtual-window.test.ts` around lines 94 - 114, Replace raw document.querySelector('main') usage with a semantic Playwright locator: create const mainLocator = page.getByRole('main') and use mainLocator.evaluate(...) instead of page.evaluate(...) to read the initial scrollTop (replace the page.evaluate call that produces targetRow.scrollTop), then change the restoration poll so it compares against mainLocator.evaluate(el => el.scrollTop) (either by calling mainLocator.evaluate inside the wait loop or by passing an evaluateHandle) instead of querying document.querySelector('main') inside page.waitForFunction; keep existing symbols targetRow, targetRowLocator, targetRow.scrollTop, page.waitForFunction and memberDetailsPage.nameInput when locating where to replace the selectors.
🤖 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/posts/src/components/virtual-table/use-scroll-restoration.tsx`:
- Around line 145-150: Restore currently reads history.state before the
in-memory Map, which can return a stale position after quick scroll-and-click;
update the restore logic (the effect that computes the restored position for
entryScopedScrollPositionKey) to check
scrollPositions.get(entryScopedScrollPositionKey) first and use that if present,
and only then fall back to reading getCurrentHistoryState() /
setStoredScrollPosition (or its stored value) — keep persistScrollPosition,
scrollPositions, entryScopedScrollPositionKey and getCurrentHistoryState usages
as-is but change the precedence so the in-memory Map wins within the same
navigation cycle.
---
Outside diff comments:
In `@apps/posts/src/components/virtual-table/use-scroll-restoration.tsx`:
- Around line 230-244: The cleanup only clears the initial timeoutId so retries
scheduled by attemptRestore can fire after unmount; modify
use-scroll-restoration to track all retry timers (e.g., keep a ref holding a
Set<number> of timeout IDs) and, whenever you call setTimeout in attemptRestore
or for the initial schedule, add the returned id to that Set, and on cleanup
iterate the Set calling clearTimeout for each id and clear the Set; ensure
symbols mentioned (attemptRestore, timeoutId, savedPosition, maxAttempts,
setTimeout, clearTimeout, scrollContainer) are updated accordingly so no retry
timeout can run with stale savedPosition or location context.
---
Nitpick comments:
In `@e2e/helpers/pages/admin/members/members-page.ts`:
- Around line 109-110: The new locators fetchMoreButton and membersList are
declared without the repo's page-object visibility convention; update their
declarations to be public readonly so they match the rest of the page-object API
(e.g., change the declarations for fetchMoreButton and membersList in the
MembersPage helper to use "public readonly" visibility).
In `@e2e/tests/admin/members/virtual-window.test.ts`:
- Around line 94-114: Replace raw document.querySelector('main') usage with a
semantic Playwright locator: create const mainLocator = page.getByRole('main')
and use mainLocator.evaluate(...) instead of page.evaluate(...) to read the
initial scrollTop (replace the page.evaluate call that produces
targetRow.scrollTop), then change the restoration poll so it compares against
mainLocator.evaluate(el => el.scrollTop) (either by calling mainLocator.evaluate
inside the wait loop or by passing an evaluateHandle) instead of querying
document.querySelector('main') inside page.waitForFunction; keep existing
symbols targetRow, targetRowLocator, targetRow.scrollTop, page.waitForFunction
and memberDetailsPage.nameInput when locating where to replace the selectors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07e70b0e-cab3-4dbb-9213-14aa7b1985b7
📒 Files selected for processing (5)
apps/posts/src/components/virtual-table/use-scroll-restoration.tsxapps/posts/src/components/virtual-table/virtual-list-window.test.tsapps/posts/src/components/virtual-table/virtual-list-window.tse2e/helpers/pages/admin/members/members-page.tse2e/tests/admin/members/virtual-window.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/posts/src/components/virtual-table/virtual-list-window.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/src/components/virtual-table/virtual-list-window.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/posts/src/components/virtual-table/use-scroll-restoration.tsx`:
- Around line 164-180: flushScrollPosition currently writes the final position
only to the in-memory map when persistToHistory is false, leaving history.state
stale; update flushScrollPosition so that after setting scrollPositions (and
updating lastPersistedAtRef/lastPersistedPositionRef) it also calls
persistScrollPosition(position) to ensure the final pre-navigation scroll
position is persisted into history.state (persistScrollPosition already guards
on sourceHistoryEntryKey so this call is safe); apply the same change to the
other similar block at the indicated location (lines 212-214) referencing
latestScrollPositionRef, entryScopedScrollPositionKey, scrollPositions,
lastPersistedAtRef and lastPersistedPositionRef.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ef22f4a-bb23-44bc-8255-a37eaa278ea8
📒 Files selected for processing (5)
apps/posts/src/components/virtual-table/use-scroll-restoration.tsxapps/posts/src/components/virtual-table/virtual-list-window.test.tsapps/posts/src/components/virtual-table/virtual-list-window.tse2e/helpers/pages/admin/members/members-page.tse2e/tests/admin/members/virtual-window.test.ts
✅ Files skipped from review due to trivial changes (2)
- apps/posts/src/components/virtual-table/virtual-list-window.ts
- e2e/tests/admin/members/virtual-window.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/helpers/pages/admin/members/members-page.ts
- apps/posts/src/components/virtual-table/virtual-list-window.test.ts
| const flushScrollPosition = ({persistToHistory = true}: {persistToHistory?: boolean} = {}) => { | ||
| clearPendingPersist(); | ||
|
|
||
| if (!persistToHistory) { | ||
| const position = latestScrollPositionRef.current; | ||
|
|
||
| if (entryScopedScrollPositionKey) { | ||
| scrollPositions.set(entryScopedScrollPositionKey, position); | ||
| } | ||
|
|
||
| lastPersistedAtRef.current = Date.now(); | ||
| lastPersistedPositionRef.current = position; | ||
| return; | ||
| } | ||
|
|
||
| persistScrollPosition(latestScrollPositionRef.current); | ||
| }; |
There was a problem hiding this comment.
Persist the final pre-navigation scroll position into history.state.
Cleanup currently flushes only to the in-memory map. A small scroll followed by immediate SPA navigation can therefore leave the source history entry stale, so after a refresh on the destination page, Back restores an older scrollTop. persistScrollPosition() already guards on sourceHistoryEntryKey, so the normal flush is still safe here.
🔧 Minimal fix
return () => {
- flushScrollPosition({persistToHistory: false});
+ flushScrollPosition();
scrollContainer.removeEventListener('scroll', handleScroll);
window.removeEventListener('pagehide', flushScrollPosition);
};Also applies to: 212-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/posts/src/components/virtual-table/use-scroll-restoration.tsx` around
lines 164 - 180, flushScrollPosition currently writes the final position only to
the in-memory map when persistToHistory is false, leaving history.state stale;
update flushScrollPosition so that after setting scrollPositions (and updating
lastPersistedAtRef/lastPersistedPositionRef) it also calls
persistScrollPosition(position) to ensure the final pre-navigation scroll
position is persisted into history.state (persistScrollPosition already guards
on sourceHistoryEntryKey so this call is safe); apply the same change to the
other similar block at the indicated location (lines 212-214) referencing
latestScrollPositionRef, entryScopedScrollPositionKey, scrollPositions,
lastPersistedAtRef and lastPersistedPositionRef.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bafdc6be46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| return ( | ||
| <div ref={parentRef} className="-mx-8 -mb-8 h-[calc(100%+32px)] max-w-[calc(100vw-300px)] overflow-auto"> | ||
| <div ref={parentRef} className="overflow-hidden"> |
There was a problem hiding this comment.
The new wrapper now clips overflow (overflow-hidden), which removes the horizontal scrolling behavior the members table previously relied on. In views with active dynamic columns (activeColumns), the configured column widths can exceed the available viewport width on common screen sizes, so the rightmost columns become inaccessible instead of scrollable. This is a user-facing regression for filtered member views where those extra columns are the primary context.
Useful? React with 👍 / 👎.
ref https://linear.app/ghost/issue/BER-3470/member-list-fails-to-render-in-chrome-for-massive-member-counts Large members, comments, and tags lists could misbehave as the virtualized window kept expanding with layout-specific reset behavior. This adds a bounded shared window with an explicit fetch-more path and keeps comments on an explicit reset key until its filters are refactored.
ref https://linear.app/ghost/issue/BER-3470/member-list-fails-to-render-in-chrome-for-massive-member-counts The larger virtual list window works better when each members fetch returns more rows, which reduces paging churn and makes the manual fetch-more flow less noisy for large datasets.
ref https://linear.app/ghost/issue/BER-3470/member-list-fails-to-render-in-chrome-for-massive-member-counts Members needed the same constrained layout shell as the other admin lists, and the table widths needed to be sized from the rendered cells so the load-more windowing did not break row layout or overflow behavior.
ref https://linear.app/ghost/issue/BER-3470/member-list-fails-to-render-in-chrome-for-massive-member-counts Preserve unlocked windows and scroll position on back navigation without stale restores or chatty history writes
ref https://linear.app/ghost/issue/BER-3470/member-list-fails-to-render-in-chrome-for-massive-member-counts Address follow-up review issues around history-entry scroll restoration and virtual window safety
ref https://linear.app/ghost/issue/BER-3470/member-list-fails-to-render-in-chrome-for-massive-member-counts Reapplied the virtual windowing changes on top of the current upstream members table structure
jonatansberg
force-pushed
the
ber-3470-fix-massive-member-counts
branch
from
bafdc6b to
f32b8be
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/posts/src/components/virtual-table/use-scroll-restoration.tsx (1)
212-214:⚠️ Potential issue | 🟠 MajorPersist the final unmount scroll position to
history.stateas well.Cleanup currently uses
persistToHistory: false, so quick scroll + SPA navigation can leave the source history entry stale for back-after-refresh restoration. Please persist on cleanup too.Suggested fix
return () => { - flushScrollPosition({persistToHistory: false}); + flushScrollPosition(); scrollContainer.removeEventListener('scroll', handleScroll); window.removeEventListener('pagehide', flushScrollPosition); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/components/virtual-table/use-scroll-restoration.tsx` around lines 212 - 214, The cleanup currently calls flushScrollPosition({persistToHistory: false}) which leaves history.state stale; update the unmount/cleanup callback to call flushScrollPosition({persistToHistory: true}) so the final scroll position is persisted into history.state before removing the scroll listener (the code around the return cleanup that also calls scrollContainer.removeEventListener('scroll', handleScroll) should be updated accordingly).
🤖 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/posts/src/components/virtual-table/virtual-list-window.ts`:
- Around line 7-9: The normalization currently allows NaN to pass through;
update getSafeVirtualListWindowSize to first validate the input with
Number.isFinite(windowSize) (or check !Number.isNaN and typeof 'number') and if
invalid use the default VIRTUAL_LIST_WINDOW_SIZE before applying Math.floor and
Math.max(1, ...); likewise, in the restore/parse path where persisted
window/count is read (the code that parses persisted values into
visibleItemCount), ensure you parse integers (e.g., parseInt or Number) and then
validate with Number.isFinite and >0, falling back to the default when invalid
so NaN or non-finite values cannot poison visibleItemCount.
In `@e2e/helpers/pages/admin/members/members-page.ts`:
- Around line 194-200: The loop in scrollUntilMaxRenderedIndexAtLeast currently
continues when maxRenderedIndex === targetIndex, causing an extra iteration;
change the loop condition from "maxRenderedIndex <= targetIndex" to
"maxRenderedIndex < targetIndex" so it only iterates while below the target,
keeping the existing calls to scrollScrollParentBy(...) and
this.page.waitForFunction(...) intact and using the same maxRenderedIndex and
targetIndex variables.
---
Duplicate comments:
In `@apps/posts/src/components/virtual-table/use-scroll-restoration.tsx`:
- Around line 212-214: The cleanup currently calls
flushScrollPosition({persistToHistory: false}) which leaves history.state stale;
update the unmount/cleanup callback to call
flushScrollPosition({persistToHistory: true}) so the final scroll position is
persisted into history.state before removing the scroll listener (the code
around the return cleanup that also calls
scrollContainer.removeEventListener('scroll', handleScroll) should be updated
accordingly).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2c49236-aeb1-4c40-96ab-ee0ed4204441
📒 Files selected for processing (12)
apps/admin-x-framework/src/api/members.tsapps/posts/src/components/virtual-table/use-scroll-restoration.tsxapps/posts/src/components/virtual-table/virtual-list-window.test.tsapps/posts/src/components/virtual-table/virtual-list-window.tsapps/posts/src/views/Tags/components/tags-list.tsxapps/posts/src/views/comments/comments.tsxapps/posts/src/views/comments/components/comments-list.tsxapps/posts/src/views/members/components/members-list.tsxapps/posts/src/views/members/member-query-params.test.tsapps/posts/src/views/members/member-query-params.tse2e/helpers/pages/admin/members/members-page.tse2e/tests/admin/members/virtual-window.test.ts
✅ Files skipped from review due to trivial changes (4)
- apps/posts/src/views/comments/comments.tsx
- apps/admin-x-framework/src/api/members.ts
- apps/posts/src/views/members/member-query-params.test.ts
- apps/posts/src/views/members/member-query-params.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/posts/src/components/virtual-table/virtual-list-window.test.ts
- e2e/tests/admin/members/virtual-window.test.ts
| export function getSafeVirtualListWindowSize(windowSize: number = VIRTUAL_LIST_WINDOW_SIZE) { | ||
| return Math.max(1, Math.floor(windowSize)); | ||
| } |
There was a problem hiding this comment.
Harden window-size/count normalization against NaN and invalid persisted values.
NaN passes typeof value === 'number' checks and currently flows through both normalization and restore paths, which can poison visibleItemCount.
Suggested fix
export function getSafeVirtualListWindowSize(windowSize: number = VIRTUAL_LIST_WINDOW_SIZE) {
- return Math.max(1, Math.floor(windowSize));
+ const normalized = Number.isFinite(windowSize) ? Math.floor(windowSize) : VIRTUAL_LIST_WINDOW_SIZE;
+ return Math.max(1, normalized);
}
@@
- if (typeof storedUnlockedItemCount !== 'number') {
+ if (typeof storedUnlockedItemCount !== 'number' || !Number.isFinite(storedUnlockedItemCount)) {
return defaultUnlockedItemCount;
}
- return storedUnlockedItemCount;
+ return Math.max(1, Math.floor(storedUnlockedItemCount));
}Also applies to: 50-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/posts/src/components/virtual-table/virtual-list-window.ts` around lines
7 - 9, The normalization currently allows NaN to pass through; update
getSafeVirtualListWindowSize to first validate the input with
Number.isFinite(windowSize) (or check !Number.isNaN and typeof 'number') and if
invalid use the default VIRTUAL_LIST_WINDOW_SIZE before applying Math.floor and
Math.max(1, ...); likewise, in the restore/parse path where persisted
window/count is read (the code that parses persisted values into
visibleItemCount), ensure you parse integers (e.g., parseInt or Number) and then
validate with Number.isFinite and >0, falling back to the default when invalid
so NaN or non-finite values cannot poison visibleItemCount.
| for (let i = 0; i < 30 && maxRenderedIndex <= targetIndex; i += 1) { | ||
| await this.scrollScrollParentBy(4000); | ||
| await this.page.waitForFunction((previousMaxIndex) => { | ||
| const rows = Array.from(document.querySelectorAll('[data-testid="members-list-item"]')); | ||
|
|
||
| return rows.some(row => Number(row.getAttribute('data-index') || '-1') > previousMaxIndex); | ||
| }, maxRenderedIndex); |
There was a problem hiding this comment.
Stop iterating once target index is reached.
scrollUntilMaxRenderedIndexAtLeast continues when maxRenderedIndex === targetIndex, which can cause an unnecessary extra wait and flaky timeout. The loop should continue only while below target.
Suggested fix
- for (let i = 0; i < 30 && maxRenderedIndex <= targetIndex; i += 1) {
+ for (let i = 0; i < 30 && maxRenderedIndex < targetIndex; i += 1) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (let i = 0; i < 30 && maxRenderedIndex <= targetIndex; i += 1) { | |
| await this.scrollScrollParentBy(4000); | |
| await this.page.waitForFunction((previousMaxIndex) => { | |
| const rows = Array.from(document.querySelectorAll('[data-testid="members-list-item"]')); | |
| return rows.some(row => Number(row.getAttribute('data-index') || '-1') > previousMaxIndex); | |
| }, maxRenderedIndex); | |
| for (let i = 0; i < 30 && maxRenderedIndex < targetIndex; i += 1) { | |
| await this.scrollScrollParentBy(4000); | |
| await this.page.waitForFunction((previousMaxIndex) => { | |
| const rows = Array.from(document.querySelectorAll('[data-testid="members-list-item"]')); | |
| return rows.some(row => Number(row.getAttribute('data-index') || '-1') > previousMaxIndex); | |
| }, maxRenderedIndex); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/helpers/pages/admin/members/members-page.ts` around lines 194 - 200, The
loop in scrollUntilMaxRenderedIndexAtLeast currently continues when
maxRenderedIndex === targetIndex, causing an extra iteration; change the loop
condition from "maxRenderedIndex <= targetIndex" to "maxRenderedIndex <
targetIndex" so it only iterates while below the target, keeping the existing
calls to scrollScrollParentBy(...) and this.page.waitForFunction(...) intact and
using the same maxRenderedIndex and targetIndex variables.
ref https://linear.app/ghost/issue/BER-3470/member-list-fails-to-render-in-chrome-for-massive-member-counts Wrapped the pagehide flush callback in an event-compatible handler so the posts build passes under CI TypeScript checks.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3280459cdd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, [historyKey, safeWindowSize]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
useVirtualListWindow only writes ghostVirtualListWindow when historyKey or unlockedItemCount changes, but not when the browser history entry changes with the same reset key. In comments, resetKey is intentionally derived from nql (not location.search), so opening ?thread=... creates a new history entry without changing historyKey; this effect never runs for that entry, and a later remount from that entry falls back to the default 1,000-row window instead of the unlocked size. This is a navigation regression for users who open a thread, leave the page, and return via Back/Forward.
Useful? React with 👍 / 👎.
3 participants
ref https://linear.app/ghost/issue/BER-3470/member-list-fails-to-render-in-chrome-for-massive-member-counts
This change adds a shared virtual list window for the large members, comments, and tags tables so they stop expanding their rendered range indefinitely. The lists now unlock additional rows in 1,000-item increments through an explicit Fetch more action, members use the same outer layout wrapper as comments and tags, and comments keep an explicit reset key so opening a thread sidebar does not collapse the unlocked window.