perf(chat): reachable archived history and stable phone scrolling - #7916
Conversation
|
Pushed A new rig scenario (
Bottom rig before: sustained ±2666 px oscillation. After: storm gone; remaining signals are the initial hydration pin and ~132 px sentinel toggles. |
Audit note — #8001 fixes the same stand-down as this PR; consolidation needed#8001 deletes the same TRIGGER 1 stand-down in The overlapBoth PRs delete the SAME silent stand-down at the SAME site: TRIGGER 1 in website/src/hooks/virtualizer/useVirtualChat.ts, where Why #7916 is the one to build onCompleteness on the shared requirement, plus reproduction evidence. On the anchor-miss hole #7916 ships four mechanisms where #8001 ships one: (1) anchor identity moved off display keys onto a new getStableId option (ChatPage wires What was deliberately discountedDiscounted as proving nothing: (a) the merge conflict in useVirtualChat.prependAnchor.test.tsx would be independent on its own — the finding rests on the two PRs flipping the same ASSERTION about the same scenario, not on co-editing a file; (b) #7916's other 62 files (history_projection.py archive pagination, MeasureFarm.tsx, pierreStaging.ts, staleShellHeal.ts + sw.js, five scroll-rigs, scrollQuiet.ts, chatSlice bounding, height-sync staleness cut, split IntersectionObservers) are wholly disjoint from #8001 and were NOT counted as overlap; #7916 also edits ChatPage.tsx, a listed hub file #8001 never touches, so that earned no credit either way; (c) #7932 also edits useVirtualChat.ts but on the stick/follow path ( What to harvest from #8001If #7916's design is kept, harvest from #8001: (1) the observation that a POSITION-derived anchor fed to the existing DOM-measured part-2 path is inherently immune to double-compensation against native CSS scroll anchoring — it measures the row's real post-layout offset — which would let #7916 retire its From a repository-wide duplicate/overlap audit of every pull request open against |
4a67916 to
75931b5
Compare
Design Review (Fable 5) — ✅ PASSDesign-level review of The design claims check out across every area I sampled — backend fail-closed pagination with the spec updated in-commit, the declined-alternative record, the centralized Design-Verdict: PASS Root-caused fixes (measured, not guessed), a formally reviewed-and-declined alternative, and fail-closed backend pagination — sound design at every layer sampled. Watch
[DESIGN-REVIEWED] 5bf553b |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of The evidence is gathered. This PR is a scroll-stability and pagination overhaul with unusually careful UX reasoning; I found two small state-truth gaps worth flagging. UX-Verdict: CONCERNS Scroll stability work is excellent; two frozen-state details misreport reality — one to screen readers, one to anyone mid-draft. Watch
[UX-REVIEWED] 5bf553b |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence gathered. Final review follows. First-Principles-Verdict: CONCERNS Every fix is measured and cause-level, but a zero-consumer re-export shim and a fleet-wide icon-size change ride along without a named harm. What this change shipsIntent: make long-transcript chat usable on a phone — archived history reachable, no bounce/teleport/snap while pages land. FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 5bf553b |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've now traced all five candidates against the actual code. Summary of my falsification:
Neither survivor fits a BLOCKING class (no crash/data-loss/security/corruption/removed-guard), and candidate 2's fix needs machinery not in this diff. No blocking issues; two advisory findings in the new scroll-reprice and staging paths. FINDING — website/src/hooks/virtualizer/useVirtualChat.ts:2375 — the newly-added above-fold correction writes FINDING — website/src/components/pierreStaging.ts:123 — [OPUS-REVIEWED] 5bf553b Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @buluoray overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
75931b5 to
e7d5a29
Compare
e7d5a29 to
2cacb20
Compare
|
Root cause of the The Design Review and UX Review BLOCK verdicts were correct:
Verification on the corrected tree: tsc 0, eslint 0 warnings, jscpd 0 clones, focus-cue gate OK, brand gate OK, 136 tests across the touched suites green, build green. |
2cacb20 to
a1851ae
Compare
Disposition — GPT 5.6 review of
|
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
Disposition — The finding says the
So index 0 of that space is the OLDEST archived row, and the archived block occupies What the branch does with the rest of the response matters too: it sets The suggested remedy — return a retryable error — would be a regression here: it converts a degraded-but-recoverable read into a hard failure for the reader, and it retires the affordance that is currently their way back into the archive. Reachability is what the finding is about, and reachability is preserved; the error path is the one that would break it. Verified by reading: The sibling finding at |
|
Disposition — The failure was
So the ceiling was consumed by main's own catalog growth, and this PR is the messenger. The budget entry's own comment predicted this exact failure mode, having already lived through it once:
The 38 KB of headroom that re-measurement bought has since been eaten, so the same trap re-armed. Refreshed it the same way: I did not shrink the chunk instead, because there is nothing of this PR's in it to shrink. I also did not take the cheaper-looking route of shaving four bytes off my own diff to slip under a stale ceiling: that would leave the ceiling stale for the next PR and hide the growth this gate exists to report. Worth flagging for a maintainer separately: a budget note that goes stale converts this gate from "catch the new library" into "fail whoever pushes next", and it has now done that twice. A CI step that re-measures and fails when a note drifts more than a few percent from reality would close it for good; that is out of scope here. |
Disposition —
|
Disposition —
|
|
/ai-review override gpt c7d007c: The trigger is not new — on main it already renders in its own always-visible reveal container, and this diff consolidates it into the single footer action row, so the row's control count grows by one while the phone gains one 44px row instead of two stacked ones. Verified before claiming it, because my first draft of this reason was wrong. And on main the trigger has its own container rather than sharing the action row: So the choice the rule is arbitrating is not "one more control or not" — it is one 44px permanently-visible row, or two stacked ones, on a phone where |
Human judgment recorded@buluoray marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
Long transcripts were unreachable past the first page and the viewport moved by itself — while streaming, while typing, a moment after opening a session, and on refresh. Five mechanisms each owned the reader's position at a different moment and each lost it a different way; a phone-fidelity reproduction rig (4x CPU throttle, Fast3G, 390px) and, for the last one, a reachability audit of the guard itself, pinned all five. Archived history is reachable: the slot-detail endpoint pages the rotated chain, and a mid-chain rotation serves the real chained corpus rather than advertising a single cursor that can never reach rows sandwiched between already-rendered ones (fork corpus reconstruction matches, so a fork from a paged row carries the right content). Scrolling holds still: - The per-tool status row is sticky for a turn and freezes into an elapsed figure, so a turn's transcript height only ever grows. - File-change chips are passed their handler by identity and depend on whether one exists, so a parent re-render no longer re-initializes every diff view (the "chips reload while typing" defect). - A reading anchor is persisted only behind real hardware input; the bottom-reached clear stays unconditional. The storage prefix is bumped to orphan anchors poisoned before that gate existed. - While following, the mounted window is derived from the height tree's own tail instead of mapping scrollTop through it, so the DOM-vs-tree tear that streaming legitimately opens cannot unmount the rows being streamed. The coverage watchdog's recovery is stick-aware: following force-pins the bottom, released re-covers in place. - The pre-paint bottom re-pin now runs for a followed reader. Its entry slot was only ever filled while follow was released, so every background repricing batch fell through to the post-paint pin — one visible displaced frame per batch, which is why a session started jumping a moment after it opened and kept going. Height accounting gained a measurement farm (off-screen pricing of unmeasured rows so a jump lands on real geometry), rename-on-regroup so a row's measurement follows it when a landing changes its display key, and a stale-shell self-heal. Five scroll rigs reproduce each scenario.
Disposition — both advisory CONCERNS on
|
|
/ai-review override gpt 5bf553b: Real finding, not a false positive, but low-probability and split out to #8484 -- it needs a hard crash inside the window between two back-to-back local writes plus a second rotation, and the writer that creates the condition is pre-existing on main. Recording this as accept-and-defer, not as a rebuttal. The finding is correct and I verified its mechanism against source rather than taking the bot's word for it — full trace in #8484. What is real. Why it is not holding this PR. It needs a hard crash (SIGKILL, power loss, OOM) inside the window between two back-to-back filesystem writes, on a session already past the rotation threshold, followed by a second rotation before anything reads. The writer that can create the overlapping pair already exists on Why the override rather than a fix here. This PR is at a fully reviewed, otherwise-green state on this SHA, and every push voids all 59 checks plus both blocking verdicts. The fix is small — the same Disclosure: GPT passed this exact SHA at 14:06 (✅ no blocking findings) and raised this on a re-run of the byte-identical tree at 15:11. That non-determinism is not the basis for this override — the finding stands on the source, which is why it is being fixed rather than dismissed. |
Human judgment recorded@buluoray marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
Summary
Makes long-transcript chat usable on a phone: archived history is reachable by scrolling, and the transcript no longer bounces, teleports, or snaps to the bottom while pages land. Driven end-to-end by a phone-fidelity reproduction rig (real CDP touch gestures with velocity, 4x CPU throttle, Fast3G, 390px viewport), then by browser instrumentation against the running dashboard — every fix below was reproduced first and verified gone after.
What was broken
The drift had a source, not a compensation bug
Five successive fixes were attempted on the assumption that the displacement had to be compensated — a staleness gate, a
scrollTop-equality gate, subtracting the reader's own scroll, freezing geometry commits during motion, arithmetic offset deltas. Each was reverted after being disproven, two of them because they made the symptom worse (writingscrollTopduring an active gesture; a mid-gesture leap to the bottom).What settled it was measuring instead of reasoning. A probe hooked both the
scrollTopsetter andel.scrollTo(the app writes through the latter, so hooking only the setter reports a false "zero writes"), setoverflow-anchor: noneon the scroller so Chromium behaves like iOS Safari — which has no native CSS scroll anchoring at all — cleared the persisted height cache, and walked 12 × 420 px upward:scrollHeightchangetopMoveexactly −420 every step, ruling out bottom-clamping; the drift occurred with the virtual window unchanged and did not occur on the step that expanded it, ruling out the mount pathDrilling into the changed row attributed every pixel to
div.pierre-surfaceinsidediv.code-block: −4 px per surface, and a row with three code blocks lost 36 px.Root cause: the code-block stand-in was not the size of the thing it stood in for
CodeBlockrenders a plain<pre>and swaps in the highlightedPierreCodefrom an idle queue when the block comes near the viewport.pierreStaging.tsstates the invariant in its own words — "if the two differ in height, that trade also moves the scroll position, which is a worse bug" — and the two differed:.msg-content pre(specificity 0,1,1) beats the single-class utilitypy-2.msg-content .code-block>pre{margin:0}reset uses a direct-child selector, and this<pre>sits inside.pierre-surface4 px of padding surplus plus 8 px of unreset margin is exactly the measured 12 px per block. Fixed with a
pierre-plainclass on the stand-in and one rule beside the offending one, restoring Pierre's measured box (margin:0, 8 px top/bottom, 20 px per line).Re-measured on the running build, same probe, anchoring off:
scrollTopwritesA separate straddle-predicate bug was found and fixed on the way: the above-the-fold reprice test asked whether a row was entirely above the fold (
rowTop + prevHeight <= foldTop), which excluded rows crossing the top edge — but a reprice does not move a row's top, it moves its bottom and everything below. Correcting it torowTop >= foldToptook the same walk from −84 px to −1 px on its own.Same class, second instance: the path chip's glyph
A confirmed path chip gets a leading 12 px glyph with a 4 px margin, and the confirmation is asynchronous. Without a reserve those 16 px appear mid-paragraph after the text is laid out; the glyph is an inline atom, so the gain can push a line over. Swept in a real browser across 300–920 px container widths, the glyph's presence flips the block's height at 22 of 624 (chip × width) combinations, spanning 336–564 px — phones sit inside that band — and each hit costs 24 px, one line.
Fixed the way the image reserve already does it (
reservedImageStyle: hold the box so the async answer restyles instead of reflowing) — with two decisions worth naming:opacity-0, so the line box matches too (an empty inline-block has a different baseline).Verified with a positive and a negative control: across 156 widths the real transition (reserve → confirmed glyph) changes the block height at 0 of them, while the pre-fix shape still shifts at 5 — so the zero is a result, not a broken probe.
Also fixed from live reports
getFollow()returns onlystickRef.current, and a turn can begin without the reader asking (subagent completion, cron, auto-nudge) — so three flag-only gates now also require the viewport to actually be near the bottom. Explicit intent (sending, the jump-to-bottom pill) deliberately bypasses the check.slotRunning, not to a streaming row, because a turn spends much of its life in tool calls with no streaming row named. Explicit intent — slot entry, the jump-to-bottom pill, sending — goes throughforcePinand is unaffected.stickplus a hardware-input suppression window) and neither half sees typing: the intent listeners are on the SCROLLER, so a keystroke in the composer never reaches them. Typing grew the composer, the viewport shrank, the bottom moved down with nobody scrolling, and the reader was dragged to it. It now delegates to the sameevaluateAutoPinthe post-paint pin uses and freezes against a viewport SHRINK — a private copy of a decision is exactly how the idle rule came to cover one half of it only.switchSlottreated a warm cache as "no limit", so every revisit pulled the entire chained transcript: 6.2 MB / ~1 s became 0.7 MB / 57 ms....overflow trigger is on the same line as the other footer buttons in every state. The below-row placement was a second permanently-visible reveal row with 44 px touch targets, adding a full row of height to every completed turn's footer — a page-scale displacement the first time those rows re-measure.chat_fork.py: a failed full-corpus read now fails closed (503 fork_corpus_unreadable) instead of forking a truncated transcript, and the tail no longer double-appends when a persisted prefix is re-sent.An architectural alternative was reviewed and declined
Replacing
scrollTopas the position of record with an{anchorKey, offsetWithinRow}pair was written up and sent to three independent cross-model reviewers. All three rejected it, for reasons worth keeping on the record:ScrollAnchorCache, an anchor-restore settle window, transient anchors for prepend/window/splice/append). A rewrite has to reconcile that, not add a second scheme beside it — and the proposal had the coordinate sign wrong: the existinganchor.topis a viewport offset, not an offset within the row.translateYis a trap: transforms do not change layout, so IntersectionObserver sentinels, find-in-page, keyboard and assistive-tech scrolling, and the scrollbar all keep using layout coordinates while the reader sees visual ones — and the transform must eventually be baked back with the samescrollTopwrite it was meant to avoid.scrollTopon every commit is worse on the target platform: WebKit cancels momentum on a programmatic write, and rubber-bandscrollTopis clamped.overflow-anchor: none, which would discard the mechanism measured absorbing this drift perfectly on Chromium.memo()was already satisfied —TurnBlockis memoized, its callbacks stabilized,measureRefstable per index. The settles come from row-internal async state, whichmemo()cannot stop. One reviewer named the code-block staging swap as the likely cause, quotingpierreStaging.ts, and proposed the cheapest confirming experiment; that is what produced the root cause above.How
History pagination (backend + store):
read_messages_chained_fullpages across rotation segments;switchSlot/refreshSlotfetch bounded, count-matched windows; a top-of-viewport walk loads older pages while parked, gated on settled scrolling and fully-measured geometry, with viewport-sized pages (100 narrow / 300 desktop).Measured geometry only (no estimates on screen): an off-screen MeasureFarm renders unmeasured rows in idle slices into a hidden container and persists real heights (scoped per width bucket), so pages land on exact geometry. Idle history prefetch walks the archive while the reader is inactive.
Scroll stability under concurrent rendering (the phone-only tear class):
useDeferredValue) used to poison the baseline and silently disarm every compensation for that landing.windowRangechange is the rebase landing — consuming against the transitional DOM mis-bound the anchor and swallowed the correction.Stale-shell self-heal (PWA): versioned SW cache per build, shell refresh on successful navigations, and a boot-time comparison of the running entry script against the server's — one throttled reload heals a phone stuck on an old bundle.
Numbers (seeded 1571-row session)
Remaining known residual: one ~14 px step at the first composer wrap while typing at the bottom, not yet attributed to a specific gate.
Testing
New pins, each mutation-verified — the mutation is stated because a guard that cannot fail is not a guard:
pierrePlainGeometry): pins the stand-in's box AND the rule it has to overcome, so if.msg-content preever stops setting a padding/margin the override is known to be removable. Mutating the metrics back to 10 px / 4 px reddens exactly one.MarkdownRenderer.chipGlyphReserve): the reserve is present while the probe is in flight, stays when the probe answers "not a path", is in place before probing is even enabled, is absent on inline code that is not path-shaped, and both states draw from one size and one margin constant. Removing the reserve reddens 4; keying it to "probe pending" — the plausible fix that merely moves the shift one tick later — reddens 3.useVirtualChat.geometryDefer): the two mutations redden disjoint sets, so neither behaviour is riding on the other's pin.An existing pin was tightened rather than relaxed to admit the reserve:
leaves an inert chip glyph-freeasserted "nosvgelement", while the property it names is that the affordance stays legible at rest. It now asserts no visible glyph (svg:not([class*="opacity-0"])), and was re-verified by mutation — making the reserve visible reddens exactly that test.Rebased onto
main(44 commits). One conflict, in the assistant footer:mainhad grown the overflow menu's contents (Share message, plan-from-here with unavailable reasons, the lazy share modal) inside the below-row placement this PR removes. Resolved by keepingmain's menu contents and gating — Share present whenever the menu is, fork/plan only in their unavailable state — at this PR's location inside the action row, so neither side's intent was dropped.Evidence
Phone (390px) — mid-transcript, archived history reached by scrolling (the surface this PR makes reachable and stable):
Desktop (1440px) — landed at the bottom of the same transcript:
The core fix is motion behaviour, and stills cannot show it. The reproducible evidence is the rigs under
website/scripts/scroll-rigs/—rigtop.mjs/rigbottom.mjsrecord per-framescrollTopwhile history pages land, and this PR was verified by their oscillation counts going to zero — plus the browser measurements quoted above, which is what located the two source-level causes after five compensation attempts had failed.