fix(virtualizer): hold the reader across a mid-list row splice - #7198
Conversation
github-actions
Bot
added
the
readiness: checking
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS Pure virtualizer-internals fix — no new strings, surfaces, or screenshots; the only user-visible effect is fewer viewport jumps during transient-row splices. [UX-REVIEWED] ce60c8a |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Root-cause fix at the right layer: retirement splits "influences the mean" from "retrievable," and every trigger reuses the existing single capture/consumer path. Watch
[DESIGN-REVIEWED] ce60c8a |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified: the rollback mechanism the retire-vs-delete decision rests on is real ( First-Principles-Verdict: PASS Every item traces to the one reported defect (#6076), lands at the cause, and reuses the existing capture slot instead of growing a parallel anchor path. What this change shipsIntent: stop the transcript jogging under a reader while transient rows mount/unmount mid-list — a FIX.
Watch
[FIRST-PRINCIPLES-REVIEWED] ce60c8a |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've examined the single candidate against the code in the diff. Candidate 1 analysis — the claim is that Falsifying the three requirements:
The only concrete artifact is that the comment's "overwhelmingly common case" premise is slightly optimistic for a swap-heavy streaming session — but that is a doc/comment quibble, a category this pipeline owns deterministically, not a reportable finding. The correctness of the retire/revive/evict/persist logic itself checks out (measuredSum stays in lockstep; revive is idempotent and only fires for keys resolving from live indices; retired keys are excluded from the mean and the persisted blob and evicted first). No survivor clears the 80 bar. No additional grounded defect surfaced under review. No findings. [OPUS-REVIEWED] ce60c8a Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
81dd5f5 to
9cf2342
Compare
github-actions
Bot
added
readiness: checking
9cf2342 to
fe93043
Compare
Legitimate, and reachable on two paths I confirmed on
Fixed, but not by the suggested revert —
Red-before proven by reverting New coverage: |
Dispositions — Design Review 🟡 CONCERNS and First Principles ✅ PASS (Watch)Both lanes named the same gap from opposite directions: the detectors key on a count delta while the harm keys on key departure, so the batched "thinking leaves + output arrives" commit falls through. Splitting it into the two halves it actually has: 1. The retirement half —
|
fe93043 to
abdc284
Compare
github-actions
Bot
added
readiness: checking
Disposition — First Principles Review 🔴 BLOCK on
|
github-actions
Bot
added
readiness: action required
abdc284 to
831f64e
Compare
Legitimate. Fixed as suggested — any departed position, not a changed last index — and the same proxy turned out to gate the GROW path too: an interior replacement alongside a tail append also kept the last index's key, so The scan is affordable because it is not a re-keying one. The overwhelmingly common commit is a token append, which rebuilds the array while reusing every element object except the streaming row's, so reference equality settles those positions with no Red-before proven by restoring the boundary proxy and keeping the tests: Three new cases: the two above, plus |
github-actions
Bot
removed
the
readiness: action required
github-actions
Bot
added
the
readiness: action required
74f85f7 to
e373e25
Compare
The finding holds. A prepend that regroups the top turn departs index 0's row while the count GROWS, and that departure satisfies both properties the previous round used to recognise head paging: the departures are a contiguous prefix, and survivors remain. So it was classified as a page-out and the regrouped row's measurement stayed in the mean. This is the fifth blocking finding on this span, and the count-fell requirement the finding names is adopted -- not as a special case for prepends, but because it completes a definition the previous round left underspecified. Head paging is the one departure that must not retire, and it is now identified by all THREE of its properties, since any two of them are also true of a departure that must retire:
Every other departure shape retires, and each is covered by one of the three: interior removal by the prefix test, equal-count swap and interior-replacement-plus-append by the count test, tail truncation by the prefix test, clear by the survivor test. A single row leaving the very head satisfies all three and is skipped, which is the behaviour that predates this branch. Red-before proven by reverting only the new conjunct: a prepend that regroups the top row away kept its 900px in the mean and priced the transcript at 19600 instead of 12000. Virtualizer family 210/210 green across 17 files, rebased onto |
github-actions
Bot
added
readiness: checking
e373e25 to
d1eb59b
Compare
Confirmed against the real caller rather than treated as hypothetical. The suggested basis is adopted, and it is the one that ends this finding class rather than adding a fourth shape test: paging is a CAPABILITY THE CALLER DECLARES, not something inferable from the diff between two item lists. Nothing pages unless the consumer asked to be told when the reader reaches the top, so a consumer without Within a paging consumer all three shape properties still apply, because any two of them also hold for a departure that must retire:
Red-before proven by reverting only the new conjunct: the filtered-out prefix kept pricing the survivors at 10000 instead of falling back to the flat estimate at 1600. The existing paging-exemption test now wires |
github-actions
Bot
added
readiness: checking
The tree is mutated BEFORE the epsilon check, unconditionally: this.reviveLiveRows(itemCount)
this.tree.sync(itemCount, this.getHeight) // always runs
const total = this.tree.totalHeight()
if (Math.abs(total - this.lastAnnouncedTotal) <= ANNOUNCE_EPSILON_PX) returnThe early return skips the baseline write, The proposed reset would regress two properties that are deliberate:
The offsetting-rows reading is also unreachable here. Retirement drops one key from the mean, so every unmeasured row re-prices in the SAME direction and the deltas cannot cancel; measured rows are untouched by retirement. A same-total outcome therefore means the per-row prices did not materially move either, so there is no hidden offset corruption behind an unchanged total. Span hit count: this is the 8th blocking finding on |
A transient "thinking" row that mounts and unmounts between rows that are
already on screen jogs the viewport, while token growth inside an existing
line is smooth. Whole-row add/remove is the difference, and the render-phase
anchor capture covered none of its shapes.
Everything below extends the ONE existing render-phase capture and its ONE
consumer -- no parallel anchor path, which would fight this one for scrollTop.
1. A mid-list INSERT was swallowed by TRIGGER 3's predicate. `tailAppended`
only tested "count grew while index 0 kept its key", which a splice
satisfies too -- so it took the append path, which resolves a mounted
node's PREVIOUS-commit index through the NEW `items` and therefore anchored
on a mis-keyed row. It now keys on whether any PRE-EXISTING position
changed hands, and the splice gets TRIGGER 1's old-items,
survivor-filtered mapping.
2. A mid-list REMOVE had no trigger at all. TRIGGER 5 covers it through the
same capture and the same consumer.
3. The estimate-vs-measured reprice, at its source. `getHeight` prices every
UNMEASURED row from the running MEAN of the measured ones, so a measurement
is never local to its own row: a transient row's height goes on pricing the
transcript after the row itself is gone.
One invariant governs the fix, in three axes:
A retirement must be COMPLETE, NON-DESTRUCTIVE, and EFFECTIVE in the
commit that dropped the row.
COMPLETE. Retirement keys on KEY DEPARTURE, not on the net count falling: a
commit that drops the ghost while adding output nets to growth or to zero
with its height still pricing the transcript. Departure is found by an
EXACT positional scan, so an interior replacement counts -- a boundary probe
reads an artifact card refreshed in place as a no-op. The scan is
affordable because a token append rebuilds the array while REUSING every
element object but the streaming row's, so reference equality settles those
positions with no `getKey` call and no allocation.
NON-DESTRUCTIVE. Removal is not always permanent: `handleRegenerate` and
`handleEditResend` both snapshot the transcript, optimistically truncate
it, and dispatch the snapshot back when the server refuses the press. So
`retire` drops the height from the MEAN and keeps the entry, and
`reviveIfRetired` puts it back when a live row index resolves that key
again -- the only available signal, because an optimistic TAIL truncation
comes back as a plain append and has no commit shape of its own. Two
orderings follow, both of which had it backwards before: eviction drains
the retired set FIRST (a transient row is measured just before it leaves,
so LRU order would drop a live row instead), and revival runs as a pass
BEFORE the tree walk (mid-walk it would leave every row priced earlier in
that same walk holding the stale mean, with no later sync guaranteed).
EFFECTIVE IN THAT COMMIT. The tree is re-synced at the retirement site, not
left to the `offsetIndex` memo: that memo is keyed on `itemCount`, and an
equal-count SWAP moves none of its dependencies, so its body would not run
and the spacers would keep prices the retirement just invalidated.
Retired entries are left out of the persisted blob while retired (a reload
has no snapshot to roll back to) and persisted again once revived.
RETIREMENT IS GATED ON DEPARTURE, NOT ON A COUNT SHAPE. Three separate commits
reached the retirement site with a row's measurement still pricing the
transcript -- an equal-count swap, an interior replacement, and a full-session
`/clear` -- because retirement borrowed the ANCHOR's triggers, and those are
built from count arithmetic plus `frontKeyHeld`. The anchor needs both: a
renamed index 0 means the mounted nodes' indices no longer name their own rows,
and a count that does not move leaves part 2 (invalidated by `windowRange` and
`itemCount`) not running to spend what was captured. Retirement needs neither,
and the clear is what made the borrowing indefensible: emptying the list
renames index 0 exactly as paging out the head does, so the proxy read a wipe
as a page-out and carried the old conversation's heights into the next one.
The gate is now stated once, at the level the harm lives on -- A ROW LEFT THIS
SESSION -- which requires either a shrinking count or a shared index changing
hands, so the streaming commit (same rows, one more at the tail) still does no
work.
Head paging is the ONE departure that must not retire, because its rows come
back when the reader scrolls up. Recognising it starts from the CALLER, not from
the data: nothing pages unless the consumer asked to be told when the reader
reaches the top, so a consumer with no `onTopReached` has no page-out to exempt
and every departure it makes is final. That is what separates the transcript
(ChatPage, which wires it) from a filtered list -- the artifacts gallery does
not wire it and feeds the virtualizer a SEARCH RESULT, so narrowing the box
drops a leading run of cards and keeps later ones, which no shape test can tell
from a page-out, and those cards are not coming back. Its `sessionId` is a
shared constant, so the stale heights would price every later gallery render.
Within a paging consumer all three shape properties are still required, since
any two of them are also true of a departure that must retire: the count FELL (a
prepend regroup also drops a prefix row while survivors remain, and it grows the
count), the departures are a contiguous PREFIX (a tail truncation or an interior
removal leaves a survivor above a departure), and a survivor REMAINS (a clear
departs a prefix and nothing else, and is not coming back). Every shape is
covered: interior removal by the prefix test, equal-count swap and
interior-replacement-plus-append by the count test, tail truncation by the
prefix test, clear by the survivor test, and any departure at all in a
non-paging consumer by the capability test.
That subsumes the equal-count trigger, which existed only to reach retirement,
so it is gone rather than left as a trigger with no anchor and no consumer. The
stranded-anchor hazard it would have carried is unchanged and still needs a new
invalidation key in the shared consumer, which wants its own regression pass:
filed as #7234.
Every trigger stays behind the `stickRef` guard: a reader pinned to the bottom
keeps following the output down.
Tests -- each red-before proven by reverting just its own production change:
- prependAnchor: splice-in above the reader drifted 100px; remove above the
reader drifted 400px. Two pinned-to-bottom counterparts pin the stick guard.
- heightSyncAnchor: after the ghost left, totalHeight stayed at 6386.67
instead of returning to 9000; a rolled-back row came back at the flat
estimate 80 instead of its own 900px measurement; an INTERIOR replacement
kept pricing the transcript at equal count (13050 instead of 9000) and
alongside an append (15100 instead of 9300); a swapped-out row left the tree
at its pre-swap 6200 instead of 9000 when the sync was left to the memo. A
commit that only rewrites one row in place -- the token-append shape -- must
retire nothing, and does not.
- virtualizerHeightOwner: with revival inside the read, the rows the walk
reaches first kept the stale mean (2100 instead of 2500).
- HeightCache: nine cases pin the retire/revive contract -- out of the mean,
still readable, revived on re-measure, revived when its row is live again,
idempotent revive, estimate fallback when every sample is retired, not
persisted while retired but re-persisted once revived, retired evicted ahead
of an older live row, and LRU order resumed once no retired entry is left.
Rebased onto #7207, which made a caller's `getKey` INDEX-ADDRESSED (ChatPage
resolves a per-render deduped key LIST by position). Every lookup this commit
adds that reads the PREVIOUS render's items -- the positional departure scan,
the splice anchor's key resolver, and the departed-key pass -- therefore prices
them with the `getKey` captured WITH them, the contract #7207 established for
the prepend capture; the current render's closure returns the NEW list's key at
an old index. Two tests pin the interaction, both red-before: an interior
removal under a positional getKey retired nothing and kept pricing the
transcript at 14100 instead of 8700, and a splice above the reader drifted the
full 100px row because the anchor was named one row off.
- heightSyncAnchor: a `/clear` in the same session priced the next
conversation's first row from the cleared transcript's mean (500 instead of
the flat 80 estimate); dropping the head-paging exclusion retired a paged-out
prefix and collapsed the region above from 10000 to 1600; and a prepend that
regroups the top row away kept its 900 in the mean, pricing the transcript at
19600 instead of 12000, because that departure satisfies head paging's other
two properties while growing the count; and a filtered-out prefix in a
consumer that cannot page kept pricing the survivors at 10000 instead of
falling back to the flat estimate at 1600.
Virtualizer family 211/211 green (anchorRestore, heightSyncAnchor, integration,
layoutShrink, observerBackfill, postStreamLurch, prependAnchor, railCollapse,
spacerLurch, viewportResize, UseVirtualChatCoverage, HeightCache,
HeightCache.multiInstance, heightOwner, ScrollAnchorCache), plus the 13 other
suites that import the virtualizer. No existing assertion weakened.
Fixes #6076
d1eb59b to
ce60c8a
Compare
github-actions
Bot
added
readiness: checking
github-actions
Bot
removed
the
readiness: passed
2 participants
Why no screenshot: scroll-position fix with no static visual delta — the change is where the viewport sits across a commit, which a before/after still cannot show. Reproduced numerically instead (100px / 400px / 200px of drift in the harness).
Problem / Motivation
While a turn streams, transient "thinking" rows mount and unmount above or between output that is already rendered. Each whole-row add/remove jogs the viewport under a reader who has scrolled up: the transcript is pushed down when the row appears and pulled up when it goes. Token growth inside an existing line is already smooth — it is the row-level insert/remove that jitters.
Not the two anchor fixes that landed this week: #6645 collapsed two captures onto one render-phase slot, and #6949 added TRIGGER 3 for a tail append. Not #4399's pinned-jump glide either, which is untouched.
Why it matters
Anyone reading back through a transcript while an agent is still working — the normal way a long turn is followed. The row they are reading walks off under them, repeatedly, for the duration of the turn. On engines with no native scroll anchoring (iOS Safari) nothing absorbs it.
What changed (motivation → approach → change)
Everything below extends the one existing render-phase capture and its one consumer. No parallel anchor path — a second path fights this one for
scrollTop.1. A mid-list INSERT was swallowed by TRIGGER 3's predicate.
tailAppendedonly tested "the count grew while index 0 kept its key", which a mid-list splice satisfies too — so it took the append path. That path resolves a mounted node's previous-commit index through the newitems, which is only correct when no index moved; after a splice it names the wrong row, so the anchor was mis-keyed and the correction measured a row nobody was reading.Split on the one O(1) fact that separates them: a tail append leaves the last pre-existing index answering to its own key, while a splice anywhere above it moves that row along. The splice case (TRIGGER 4) gets TRIGGER 1's mapping — previous items at the node's previous index, filtered to keys that survive the commit, so a retired key falls forward to the next survivor.
2. A mid-list REMOVE had no trigger at all. An unmounting "thinking" row is exactly that. TRIGGER 5 covers it through the same capture and the same consumer: the height above the reader changing is one event, and the correction does not care which direction it moved.
3. The estimate-vs-measured reprice, at its source.
getHeightprices every unmeasured row from the running MEAN of the measured ones, so a measurement is never local to its own row. A transient row is measured while mounted and then leaves — and its height goes on pricing the transcript after the row is gone, so everything above the reader stays under-priced until the entry is evicted, and past a reload once the blob is persisted. Compensating one commit cannot reach that: the reprice recurs on every later sync.New
HeightIndex.retire/HeightCache.retiredrop the height from the mean during the render that dropped the row, so the reprice lands in the commit the splice anchor already compensates. Two properties earned during review:handleRegenerateandhandleEditResendboth snapshot the transcript, optimistically truncate it, and dispatch the snapshot back when the server refuses the press — so rows leave the list and come back, and rows restored without their measurements would be re-priced from the mean (wrong spacer, viewport jump, and it persists for every row still off-window). A laterset()revives a retired key, retired entries keep their LRU position so they evict before live ones, and they are left out of the persisted blob so a reload does not re-admit them to the mean.Retirement is reversible, because removal is not always permanent. A retired key is un-retired by
HeightCache.reviveIfRetired, called from the resolved-height read inHeightIndex.heightAt. That read is the right site rather than a removal site: a restore has no commit shape of its own to hook (an optimistic tail truncation comes back as a plain append, which is not a splice at all), while a key resolving from a live row index is itself proof the retirement's premise no longer holds. Without it a restored row kept its own exact height but stayed out of the mean, so it never priced the unmeasured rows again — and an off-window row, which never re-measures, stayed out indefinitely.What TRIGGER 6 deliberately does NOT do: capture an anchor. The single consumer is invalidated by
windowRangeanditemCount, and an equal-count commit moves neither, so an anchor taken there would sit in the slot and be spent on an unrelated later commit — the stranded-anchor hazard the render-phase capture exists to remove. Compensating a swap needs a new invalidation key in that shared consumer, which changes a contract every trigger depends on; filed as #7234 with the measured 200px reproduction.Every trigger stays behind the
stickRefguard: a reader pinned to the bottom keeps following the output down.Diff is confined to
website/src/hooks/virtualizer/and its tests. No reformatting.Tests
Each case red-before proven by reverting only its own production change and keeping the tests:
prependAnchor: row spliced in above the readerprependAnchor: row removed above the readerheightSyncAnchor: ghost row leaves the listtotalHeightstuck at 6386.67heightSyncAnchor: rolled-back row restoredretire→ deleteheightSyncAnchor: swapped-out row at equal counttotalHeight7110virtualizerHeightOwner: restored row back in the meanreviveIfRetiredPlus seven
HeightCachecases pinning the retire/revive contract (out of the mean, still readable, revived on re-measure, revived when its row is live again, idempotent revive, estimate fallback when every sample is retired, not persisted while retired and re-persisted once revived), and two pinned-to-bottom counterparts —still follows to the bottom when a row is SPLICED IN while PINNEDandkeeps following after a row is REMOVED while PINNED— which pass onmainand stay green, so a fix that held position for a pinned reader would fail them.Non-vacuity is asserted in each case: the ghost really mounts, the removed row really is gone.
No existing assertion weakened. Virtualizer family 198/198 green (
anchorRestore,heightSyncAnchor,integration,layoutShrink,observerBackfill,postStreamLurch,prependAnchor,railCollapse,spacerLurch,viewportResize,UseVirtualChatCoverage,HeightCache,HeightCache.multiInstance,virtualizerHeightOwner,ScrollAnchorCache). Full website suite green;tsc -bandeslintclean on the changed files.Manual verification
N/A — the jitter is a scroll-position delta, and the harness reproduces it numerically: the deterministic layout engine in
prependAnchor.test.tsxmakes the rows genuinely move, so every number in the table above is measured, not asserted against source text.Pattern harvest
Rule candidate: review checklist (not mechanically greppable)
Pattern: a cache retraction keyed on "the row left the list" must not treat removal as permanent when the app has optimistic-then-rollback flows. The first version of the retirement deleted the measurement, which is correct only if a departure is final. It is not:
handleRegenerateandhandleEditResendboth snapshot, truncate, and restore the snapshot when the server refuses the press. The generalizable shape is that a derived-data store gets two independent questions — "should this still influence other entries?" (here: the mean) and "should this still be retrievable?" (here: the entry) — and collapsing them into one delete is what turns a correct invalidation into data loss. The same split appears in this file's own eviction, which is a capacity decision about rows that still exist and therefore is not a retraction at all. Two of the three review findings on this PR were instances of getting that split wrong in opposite directions (delete loses the restore; retire-without-revive loses the sample), which is why it is worth writing down rather than treating as a one-off.Not mechanically enforceable: the trigger is "this key's row may come back", which no pattern can see from the call site. The checkable proxy is narrower and worth asking on any PR that removes an entry from a persisted cache — what restores this key, and does that path re-establish everything the removal took away?
Related Issues
Fixes #6076
Checklist