Repository navigation
fix(react): a gate reopening at the same window redraws its spacer and extent - #569
Merged
Merged
Conversation
An estimated total turning exact at the window already on screen changes no rows, so `setRows` is not an effective write and no revision reaches the row layout controller — which reads `getWindowSpacers` only at plan time. Every prop-derived value moved on that render (`aria-rowindex`, `aria-rowcount`) while the drawn leading spacer and scroll extent stayed collapsed at the shut gate's geometry. On a grid whose loaded window fits its viewport that never heals: the collapsed extent leaves nothing to scroll, and a scroll is what would have replanned. `refreshWindowSpacers()` re-reads the getter and republishes when the spacer counts no longer match the ones the last plan drew. Anchored, so a leading spacer appearing under rows already on screen moves the GLOBAL offset rather than the rows. The comparison is the controller's, not the caller's — a caller cannot see which spacers a plan used — which is what makes it free to fire from a layout effect on every commit: an effective row change replans with the new counts and the call that follows finds nothing to do. A no-op rather than a throw when disposed, unlike `setColumns`/`setViewport`: those carry consumer intent, this one is fired unconditionally, and an explicit-model consumer that disposes its model mid-mount commits again afterwards. Closes #568 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Vercel preview readyPreview: https://pretable-qe6lgjy2k-cacheplane.vercel.app Updated automatically by the |
… three branch tests Review items from PR #569, none of them blockers: - The refresh predicate compared a composed key, which told `null` apart from `{leadingRows: 0, trailingRows: 0}`. Those draw the identical grid, so a gate flip at `windowStart === 0` — the commonest window there is — fired one anchored republish producing byte-identical geometry. Compares the two counts as numbers now, which collapses absent with zero and drops the per-commit string allocation. - The disposed no-op is the surprising half of `refreshWindowSpacers`' contract and the one place `RowLayoutController` breaks its own throws-after-dispose uniformity, so the rationale moved onto the interface where a caller will find it. - The React layout effect's position after the viewport-authority effect is load-bearing, not incidental: that effect's `setViewport` replans and a replan reads the spacers fresh, so running the refresh first would cost an extra anchored publish on every viewport commit. Said so at the call site. - The `windowGap` comment claimed the plan is behind for exactly one render. True only with no replacement in flight; with one running the refresh returns early and the plan stays behind until that replacement publishes. Three branches the first pass left unexercised now have tests, each mutation-checked: the in-flight return (its safety was an argument — that the replacement's own `prepareWindow` re-reads the getter — and is now an observation), the catch/`startReplacement` fallback reached by poisoning the estimator for one plan (`windowSpacerRefreshFallbackCount` was asserted nowhere), and the head-of-dataset gate flip that item one is about. The queued-action branch (`notifying || projecting || synchronizing`) is still untested; reaching it needs a reentrant call from inside a notification. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
blove
enabled auto-merge (squash)
September 3, 2026 16:43
This was referenced Sep 3, 2026
Merged
Merged
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.
Closes #568.
The mechanism, confirmed
All three links in the issue hold, at these lines on
0af4bc36:packages/row-model/src/create-local-row-model.ts:1169(setRows) and:1247/:1251(applyTransaction) both gate ondrafted.effective— the revision is only bumped when it is, andnotify: falseotherwise. TurningresultMeta.totalfromestimatetoexactat the same window changes no rows, so no revision is published.resultMetato the controller. The honesty-gatedwindowSpacersmemo (packages/react/src/pretable-surface.tsx:3827) is pushed to a mutable channel in auseInsertionEffect(:3916), and the controller reads that channel only throughgetWindowSpacers.setViewportshort-circuits on an unchanged viewport (row-layout-controller.ts:2394), so aresultMeta-only change had no event at all.packages/renderer-dom/src/row-layout-controller.ts:858—prepareWindowresolves them at the top and derivesleadingHeight/trailingHeightfrom them, andplanViewportbuildstotalHeightasleading + loaded + trailing.aria-rowindexandaria-rowcountare derived from props during render (pretable-surface.tsx:3874,:7168), so they moved on the reopen render while the drawn geometry did not.Reproduction (jsdom,
gate-reopen-spacer.test.tsx)20-row dataset,
resultMeta.window.start: 10, 10 loaded rows, 44px theme row height, 800px viewport.estimate)exact), before the fixaria-rowcountaria-rowindextopThe two announced values moved on the same tick as the prop; the two drawn values were still at the shut gate's geometry 15 seconds later, when the poll timed out.
Severity: a reachability bug, not cosmetic
The "next replan" in a real app is a scroll, a resize, or an effective row change — but the collapsed extent is what removes the first of those. In the reproduction above the extent is 440px against an 800px viewport, so there is no scroll to make: the grid announces 21 rows, draws rows 10..19 as though they were the head of the dataset, and offers the reader no gesture that would reach rows 0..9.
windowGaptelemetry does not rescue it either, since the "before" branch testsviewportTop < renderSnapshot.leadingHeightand that is the stale0, so the consumer is never told to load. A grid whose loaded window fits its viewport, with a backend that returns an estimated total first and an exact one after, is stuck there for the life of the mount.Decision: (a), a spacer-only replan
(b) required the stale window to be unreachable-by-a-user or self-healing in every realistic path, and it is neither. Chose (a).
The seam is the same one PR #522 established for
setColumns: the row sequence is untouched, so the height index is reused as-is, an anchor is captured and restored, and any doubt falls back to a full replacement.refreshWindowSpacers()is that, without theclearEstimates()(nothing about the estimator changed — only the counts of rows nobody is drawing):setRowson a windowed grid movestrailingRows, and the replacement it triggers plans against the new counts; the refresh that follows in the same commit then finds nothing to do. Pinned by two negative tests, one of them the streaming shape exactly.setColumns/setViewport. Those carry consumer intent, so swallowing one would hide a lifecycle bug; this one is fired unconditionally from a layout effect, and an explicit-model consumer that disposes its model mid-mount commits at least once more afterwards. The full react suite found that:column-helper-date-format.test.tsxtook the whole tree down on the throw.The
windowGapcomment atpretable-surface.tsxthat justified the non-replan is updated. Its conclusion survives — reading the plan's own geometry for the loaded-window boundary is still right — but for a sharper reason: the plan is now a commit behind for exactly one render (the layout effect that closes the gap has not run yet), and reading the plan keeps that render silent instead of reporting a gap off a pixel total nobody drew.Tests
packages/react/src/__tests__/gate-reopen-spacer.test.tsx— the transient end to end, at the level a user sees it.packages/renderer-dom/src/__tests__/window-spacer-refresh.test.ts— 5 tests: the redraw, the anchoring, and three that pin the cost (already-current, replan-absorbed, disposed).Mutation results
stores.controller.refreshWindowSpacers()from the layout effectgate-reopen-spacerfails (topstays0px, expected440px)gate-reopen-spacerfails identicallyrestoreAnchorRequest(...)forglobalScroll(viewport.scrollTop)Every assertion in both files is killed by removing what it claims to test.
Suites
@pretable/react: 129 files, 1779 tests, all pass.@pretable-internal/renderer-dom: 10 files, 173 tests, all pass.lint,typecheck,build,api:check,formatall clean.Bench control (the change touches the commit path)
pnpm bench:matrix --adapters=pretable --repeats=5 --scale=target --scenarios=S5 --scripts=group-updates,updates, port 4173 verified free, both sides measured in the same session on the same machine,origin/main(0af4bc36) rebuilt for the before side.scroll_frame_p95_ms, 5 repeats each:group-updatesupdateslong_tasks_ms0 andframe_budget_overruns_count0 on every run of both sides. No regression; the two distributions overlap entirely, which is what the guard predicts.🤖 Generated with Claude Code