Proposal H, item 3: flat vertical scroll + sheet-level virtualization - #115
Merged
WilfordGrimley merged 2 commits intoJul 19, 2026
Merged
Conversation
Replaces /display's one-page-at-a-time pager with a continuous vertical stack of every sheet (sheets derived from paginateSlotsForDisplay), each mounting its PagePreview only when on/near screen via the existing RenderIfVisible component (already proven in CardResultSet.tsx) rather than a new IntersectionObserver implementation. The old prev/next pager is gone; a passive "Sheet N of M" readout (its own, tighter-band IntersectionObserver, distinct from RenderIfVisible's broader mount/ unmount one) replaces it. Slot click/selection logic now operates across all sheets, not just the current page.
…c-flat-scroll-virtualization-04bam2 # Conflicts: # frontend/src/features/display/DisplayPage.tsx
WilfordGrimley
deleted the
claude/proposal-h-3c-flat-scroll-virtualization-04bam2
branch
July 19, 2026 02:38
This was referenced Jul 19, 2026
WilfordGrimley
pushed a commit
that referenced
this pull request
Jul 19, 2026
… script Per owner follow-up on #115: the manual scroll fps/heap/jank benchmark that gated the sheet-level-vs-row-granular virtualization decision is now under frontend/tests/perf/, runnable via `npx playwright test --config=playwright.perf.config.ts`. The main playwright.config.ts testIgnores tests/perf/ so it never runs in CI or a plain `npx playwright test`. Ahead of the pane migration, which restructures this exact surface, this turns "did scrolling regress?" into a number to compare against, not a feeling. Verified: `npx playwright test --config=playwright.perf.config.ts` reproduces the PR's original results (120 cards/15 sheets, ~59fps, peak heap ~257MB, 16/120 max simultaneously-mounted <img> tags, 0 long tasks); `npx playwright test --list` (default config) does not surface the perf spec.
2 tasks
WilfordGrimley
added a commit
that referenced
this pull request
Jul 19, 2026
…a commit orphaned by #115's merge) (#178) * Commit item 3's scroll/virtualization benchmark as a runnable, non-CI script Per owner follow-up on #115: the manual scroll fps/heap/jank benchmark that gated the sheet-level-vs-row-granular virtualization decision is now under frontend/tests/perf/, runnable via `npx playwright test --config=playwright.perf.config.ts`. The main playwright.config.ts testIgnores tests/perf/ so it never runs in CI or a plain `npx playwright test`. Ahead of the pane migration, which restructures this exact surface, this turns "did scrolling regress?" into a number to compare against, not a feeling. Verified: `npx playwright test --config=playwright.perf.config.ts` reproduces the PR's original results (120 cards/15 sheets, ~59fps, peak heap ~257MB, 16/120 max simultaneously-mounted <img> tags, 0 long tasks); `npx playwright test --list` (default config) does not surface the perf spec. * Fix prettier formatting in scroll benchmark spec --------- Co-authored-by: Claude <noreply@anthropic.com>
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
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.
Description
Replaces
/display's one-page-at-a-time pager with a continuous vertical stack of every sheet, per the owner's flat-scroll amendment. Each sheet'sPagePreviewmounts only when on/near screen via the existingRenderIfVisiblecomponent (already proven inCardResultSet.tsx) rather than a new IntersectionObserver implementation from scratch.sheets(derived fromdisplayPagination.ts'spaginateSlotsForDisplay) replaces the old single-pagecurrentPageEntries/currentPageSlotsderivation — every sheet's slot content is resolved (cheap bookkeeping), but only sheets within one sheet-height of the viewport actually mount real<img>tags.display-page-indicator) is gone. In its place: a passivedisplay-sheet-indicatorreadout ("Sheet N of M") driven by its own IntersectionObserver — a thin band near vertical centre, distinct fromRenderIfVisible's own broader mount/unmount rootMargin.handleSlotClick/selected-slot-index logic now operates across all sheets simultaneously, not just "the current page."origin/masterto reconcile with item 2 (Proposal H, item 2: inline PDF export runs the real pipeline on /display #109, inline PDF export) and item (c) (Item (c): requested-printing badge on editor slots #110,RequestedPrintingBadgeextraction), both of which landed on master after this branch was created and both touchDisplayPage.tsxsignificantly. The merge was hand-resolved (one import-line conflict) and re-verified in full post-merge — see the checklist below.Benchmark (owner-required gate)
Measured against a 120-card deck (15 sheets) under 4x CPU throttle, dev-mode flag-on build, two runs:
<img>tagsDecision: sheet-level virtualization is sufficient — shipping as-is, no row-granular fallback needed. Both runs meet the ~60fps threshold with zero long tasks and a bounded image-mount count (~2 sheets' worth regardless of deck size), confirming the anti-crash goal holds. Caveat: measured under Next dev mode (unminified, React dev overhead) via the project's own Playwright dev-server harness, not the literal built production bundle — a real production build would only improve on these numbers, so this is a conservative/pessimistic reading, not an optimistic one.
Follow-up (owner-requested): the benchmark script is now committed under
frontend/tests/perf/display-scroll.bench.spec.ts, runnable vianpx playwright test --config=playwright.perf.config.ts— a separate config so it's excluded from both a plainnpx playwright testand CI's sharded run (playwright.config.tsnowtestIgnorestests/perf/**). Re-running it against this branch reproduces the original numbers (120 cards/15 sheets, ~59fps, peak heap ~257MB, 16/120 max mounted<img>s, 0 long tasks). Intended to give the upcoming pane migration (which restructures this exact surface) a number to compare against instead of a feeling.Checklist
pre-commitand installed the hooks withpre-commit installbefore creating any commits.DisplayPage.spec.ts). Existingdisplay-page-indicatorassertion updated todisplay-sheet-indicator. Benchmark script added undertests/perf/(non-CI, manually run).npx tsc --noEmitcleannpx jest— 42/42 suites, 399/399 tests passingNEXT_PUBLIC_UNIFIED_DISPLAY_ENABLED=true npx next build— flag-on production build succeedsnpx playwright test tests/DisplayPage.spec.ts tests/DisplayPageExport.spec.ts— 19/19 passing on the merged tree (full-file runs, not filtered)npx playwright test --list(default config) confirmstests/perf/is invisible to normal/CI runs;npx playwright test --config=playwright.perf.config.tsruns it and reproduces the benchmark numbersKnown sandbox flakiness (documented per prior PRs in this sequence, not chased further): unfiltered full-file Playwright runs in this dev sandbox intermittently fail with "Playwright Test did not expect test.describe()/describe() to be called here," reproduced even on untouched spec files — confirmed environmental, not code-related. Not observed on the runs quoted above.