fix: memoize Virtuoso diff-line items to stop rebuilding on every render - #357
Merged
Conversation
buildDiffVirtuosoItems ran unmemoized inside DiffVirtuosoList, so every unrelated re-render (search debounce, line hover, etc.) rebuilt the entire item list and its object identities for large diffs. Wrap it in a useDiffVirtuosoItems hook (useMemo keyed on the actual diff data) so the items array stays referentially stable across unrelated renders, which is what was making Virtuoso's virtualization feel unreliable on diffs with many lines. Also memoize visibleCommittedFiles for the same reason. Added a unit test proving the hook returns a stable array reference across unrelated re-renders and only recomputes when diff data changes, plus a screenshot spec that expands a 600-line file diff and confirms every line (including the last one) actually renders.
… spec The previous spec scrolled the "h-full px-4" wrapper DiffContentArea renders around the list, not the actual scrollable element -- that's the div <Virtuoso> itself renders (data-testid="diff-virtuoso"), the one a real user's mouse wheel/scrollbar would move. Target that element instead, and assert the scroll actually moved (scrollTop > 0) before checking the last line is still rendered. Also documents that react-virtuoso is mocked to render every item unconditionally in this test setup (test/setup.common.ts), so this spec proves the scroll interaction doesn't break line rendering but can't catch a real virtualization/windowing regression -- that needs a live browser against the real react-virtuoso scroll-range logic.
…zation Replace the app-wide react-virtuoso jsdom stub (which renders every row unconditionally, so it could never catch a windowing regression) with the real library for this spec. jsdom has no layout engine, so react-virtuoso's dynamic-height bootstrap never converges there; work around that with fixedItemHeight + a viewport-capped initialItemCount (both real, documented react-virtuoso props) plus minimal stubs for the layout signals (offsetHeight, scrollHeight, Element.scrollTo) and a MutationObserver-driven ResizeObserver proxy it reads off its own scroll container. The spec now proves real virtualization on a 600-line diff: only a window of lines near the top mounts, with the far-off last line absent from the DOM -- something the fully-mocked version could never assert. Clicking a later file in the sidebar is exercised as a real interaction (must not error), but its scroll outcome isn't asserted: traced deep enough to confirm react-virtuoso's scrollToIndex reactive pipeline doesn't converge to its own native scrollTo call under jsdom, which needs a live-browser (e.g. Playwright) check instead.
Contributor
|
🚀 Web preview: https://preview-357.treq-9zy.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
buildDiffVirtuosoItems ran unmemoized inside DiffVirtuosoList, so every
unrelated re-render (search debounce, line hover, etc.) rebuilt the
entire item list and its object identities for large diffs. Wrap it in
a useDiffVirtuosoItems hook (useMemo keyed on the actual diff data) so
the items array stays referentially stable across unrelated renders,
which is what was making Virtuoso's virtualization feel unreliable on
diffs with many lines. Also memoize visibleCommittedFiles for the same
reason.
Added a unit test proving the hook returns a stable array reference
across unrelated re-renders and only recomputes when diff data
changes, plus a screenshot spec that expands a 600-line file diff and
confirms every line (including the last one) actually renders.