fix(render): four state-tracking fixes in the vertical virtual renderer - #4931
fix(render): four state-tracking fixes in the vertical virtual renderer#4931lukecotter wants to merge 5 commits into
Conversation
vDomScrollHeight was only assigned inside _virtualRenderFill's full-fill branch, never initialized. scrollRows reads it on the first scroll (before any full fill), where it was undefined and produced a NaN comparison that took the wrong add/remove branch. Initialize to 0 in the constructor and reset it in clearRows.
The rerenderRows anchor-scan fallback used `this.rows.length - 1`, but `this.rows` is the method (arity 0), so the expression was always -1. When the scan found no anchor row (stale or out-of-range rendered window), the renderer filled from position -1 and left vDomTop negative. Use the last display-row index instead.
rerenderRows scanned the pre-filter vDomTop..vDomBottom window for an anchor row, then filled against the post-filter rows. When that window pointed past the new (smaller) row count, the stale topOffset inflated vDomTopPad into a blank strip across the top. Fall back to a fresh fill (which resets vDomTopPad) when the pre-filter window is invalid or no anchor was found. Also derive vDomBottomPad in the position branch of _virtualRenderFill from the current row count (mirroring the full-fill branch) instead of the cached vDomScrollHeight, which goes stale when the row count shrinks and leaves an inflated blank strip below the last row; refresh vDomScrollHeight too. Adds a Playwright regression test filtering 2000 rows down to ~50 and asserting no top/bottom blank strip.
In _removeTopRow, accumulated actual row heights can push vDomTopPad slightly negative, which sets a negative paddingTop and lets rendered content drift above the viewport. Clamp to zero after the adjustment.
lukecotter
changed the title
|
@lukecotter love all these PRs, but it's hard for me to evaluate the impact of them all, or to know if you've had any hand in them, or tested them, beyond your AI agent. Any way you could add a little video to each one showing what each PR does, or add a Luke-written comment with context for the fix and how you've tested it? For example - was this just a fun activity for you? Are you using this in production? |
|
@rathboma Yes I have manually tested them all and run them through a chrome dev tools perf profile. They are actually all fixes I applied to a custom version of the renderer I wrote to fix some bugs. One example was no rows being shown when the scroll is dragged down then quickly scrolled up. That new renderer also improved performance quite a bit but it had gaps in behaviour and is obviously a sync and maintenance annoyance not to mention a potential upgrade nightmare so I thought it better to port the fixes so I can switch back to stock. I also opened some prs which improve performance and I have some more improvements that could be migrated from the custom version but I was hoping get some of these in first to avoid bombarding you with too much and make sure they other changes are stable first. Plus I want to re benchmark once these all land. |
2 participants
What this is
Four state-tracking fixes in
VirtualDomVertical, five commits, 29 added lines in one fileplus tests. They were four separate drafts (#4931–#4934); one review beats four tabs on the
same file. Happy to split them back out.
vDomScrollHeight(commit 1) —scrollRowsread it before the first fillset it, so an early scroll worked from
undefined.rerenderRowsfallback index (commits 2 and 4) — with no anchor row itfell back to an index other than the one just computed. Commit 4 then separates "no anchor
row found" from "window indices past the new row count", which had been conflated and so
discarded the fallback.
a short one left a blank strip.
vDomTopPadto zero when removing top rows (commit 5) — one line; it could gonegative.
Verification
npm run test:unit471 pass,npm run test:e2e22/22. New coverage:VirtualDomVertical.scrollHeight.spec.js,VirtualDomVertical.rerender.spec.js, and an e2ecase in
rerender-filter.spec.ts.Performance-neutral: mutation churn is byte-identical to master at 500k rows.
Part of #4938.