Skip to content

perf(chat): reachable archived history and stable phone scrolling - #7916

Merged
buluoray merged 1 commit into
mainfrom
perf/chat-history-polish
Sep 4, 2026
Merged

perf(chat): reachable archived history and stable phone scrolling#7916
buluoray merged 1 commit into
mainfrom
perf/chat-history-polish

Conversation

@buluoray

@buluoray buluoray commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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

  • Sessions with rotated/archived history could not scroll past the kept head: the top of the transcript dead-ended.
  • Switching to a long session fetched the entire chained transcript (6.2 MB on the seeded 1571-row session) and rendered it in one synchronous pass (441 ms long task).
  • Scrolling up while pages landed threw the reader around: 16-20 anchor jumps per 60s walk with a worst single jump of ~3400 px on the throttled rig; on a real iPhone the transcript bounced and lost the reading position.
  • Reading history on a phone, the transcript drifted upward under a still reader — reported as the text sliding while scrolling back through old turns.
  • A turn starting without the reader asking for it (a subagent finishing, a cron, an auto-nudge) snapped the transcript to the bottom even when the reader was far up.
  • The composer growing as you typed pulled the transcript with it.
  • A stale service-worker shell could pin a phone to yesterday's bundle for a whole day.

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 (writing scrollTop during an active gesture; a mid-gesture leap to the bottom).

What settled it was measuring instead of reasoning. A probe hooked both the scrollTop setter and el.scrollTo (the app writes through the latter, so hooking only the setter reports a false "zero writes"), set overflow-anchor: none on 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:

  • total drift −84 px, with each step's residual exactly equal to that step's scrollHeight change
  • zero writes from the app — nothing was mis-compensating, the content itself was shrinking
  • with native anchoring left on, the same walk drifted 0 — which is why the defect was invisible on a desktop browser and reproducible only on iOS
  • topMove exactly −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 path

Drilling into the changed row attributed every pixel to div.pierre-surface inside div.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

CodeBlock renders a plain <pre> and swaps in the highlighted PierreCode from an idle queue when the block comes near the viewport. pierreStaging.ts states 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:

stand-in (rendered) Pierre why
vertical padding 10 px + 10 px 8 px + 8 px .msg-content pre (specificity 0,1,1) beats the single-class utility py-2
vertical margin 4 px + 4 px 0 the existing .msg-content .code-block>pre{margin:0} reset uses a direct-child selector, and this <pre> sits inside .pierre-surface

4 px of padding surplus plus 8 px of unreset margin is exactly the measured 12 px per block. Fixed with a pierre-plain class 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:

before after
total drift over a 12-step walk −84 px 0 px
compensating scrollTop writes 0 (nothing fired) 0 (nothing to compensate)
content height across the walk 25173 → 25089, moving every step 24693, constant
rows changing height −12 / −24 per step none

A 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 to rowTop >= foldTop took 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:

  • The reserve is keyed to path SHAPE, not to the probe. Keying it to "probe enabled" would make it appear exactly when a message stops streaming, i.e. when the text has just become final: the same shift, one trigger later.
  • It reserves blank space, not a placeholder glyph. The visible glyph is what tells a reader at rest which paths the backend actually confirmed; a placeholder would erase that distinction to buy nothing. It renders the same icon element at 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

  • Automatic bottom pins now require live geometry, not just the follow flag. getFollow() returns only stickRef.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.
  • An automatic bottom pin now requires a live run. Following means "keep me at the end of a LIVE turn", so with nothing running a reader above the bottom is not following, and pinning them is a yank with no cause — reported from a phone as the transcript springing back after a scroll up of about a hundred pixels with nothing streaming. Idle now RELEASES follow rather than merely skipping the pin: leaving it armed hands the same yank to whichever turn starts next. Keyed to 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 through forcePin and is unaffected.
  • The PRE-PAINT bottom pin was a third copy of that decision, and it chased the composer. A height-sync commit re-targets the bottom inside the commit that repriced the tree, so a large reprice stays invisible to a bottom-pinned reader. That path had its own hand-rolled gate (stick plus 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 same evaluateAutoPin the 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.
  • Typing freezes follow. A viewport shrink (the composer growing) no longer drives a pin; growth still does, because that is space being given back.
  • Follow re-engages only on a genuine downward move. A neutral scroll event inside the re-engage band used to re-arm follow when content collapsed under a still reader.
  • The older-history walk stops when the reader stops climbing (1.5 s activity window) and is capped at 4 pages per input. Previously a reader who parked near the top was the ideal candidate and watched four pages land unasked.
  • Revisiting a session no longer refetches the whole chain. switchSlot treated 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.
  • The ... 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.
  • Geometry commits are deferred while the reader is in motion — but never the first commit of a mount. Before the first commit there is no settled picture to protect, and a scroll event fires while the transcript takes its initial position, so without that exemption every mount's seed geometry was pushed a debounce round later, leaving rows priced at estimates while the reader was already looking at them.
  • 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 scrollTop as 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:

  • The codebase already has partial anchor machinery (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 existing anchor.top is a viewport offset, not an offset within the row.
  • Cancelling the shift with translateY is 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 same scrollTop write it was meant to avoid.
  • Recomputing scrollTop on every commit is worse on the target platform: WebKit cancels momentum on a programmatic write, and rubber-band scrollTop is clamped.
  • Both mechanics require overflow-anchor: none, which would discard the mechanism measured absorbing this drift perfectly on Chromium.
  • The companion idea of "locking in-view rows" with memo() was already satisfied — TurnBlock is memoized, its callbacks stabilized, measureRef stable per index. The settles come from row-internal async state, which memo() cannot stop. One reviewer named the code-block staging swap as the likely cause, quoting pierreStaging.ts, and proposed the cheapest confirming experiment; that is what produced the root cause above.

How

History pagination (backend + store): read_messages_chained_full pages across rotation segments; switchSlot/refreshSlot fetch 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):

  • Prepend/window comparison mirrors advance at COMMIT time, not render time — a discarded concurrent attempt (the transcript arrives through useDeferredValue) used to poison the baseline and silently disarm every compensation for that landing.
  • The prepend anchor is consumed only at 'ready', promoted on the commit whose windowRange change is the rebase landing — consuming against the transitional DOM mis-bound the anchor and swallowed the correction.
  • Tree-math fallbacks cover the three anchor-miss holes, writing only the remainder after Chromium's native scroll anchoring (blind writes doubled the compensation into a page-sized leap; WebKit has no native anchoring and keeps the full write).
  • Height-sync anchors are discarded past 150 ms: a late consume is viewport-relative and used to "correct" the user's own scrolling — a deterministic 2706 px teleport on cold-cache walks.
  • Older-history responses fetched during a gesture are held until the scroller is quiet (bounded). Fetches still overlap scrolling; only the splice waits.

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)

Metric Before After
Switch payload 6.2 MB 0.7 MB
Longest task on switch 441 ms 101 ms
Warm momentum fling (60s, throttled) 27 jumps, worst ±3040 px 0 jumps
Top-park walk (60s, phone-fidelity) 16-20 jumps, worst 3400 px 0 kilopixel events (warm)
Cold-cache walk teleport 2706 px, 3/3 runs 0/3 runs
History-reading drift, native anchoring off −84 px per 12-step walk 0 px
Path-chip confirmation, 300–920 px sweep shifts at 22 of 624 0
Fully-loaded idle (30s) 0 events 0 events

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:

  • Stand-in geometry (pierrePlainGeometry): pins the stand-in's box AND the rule it has to overcome, so if .msg-content pre ever stops setting a padding/margin the override is known to be removable. Mutating the metrics back to 10 px / 4 px reddens exactly one.
  • Chip glyph reserve (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.
  • First commit is never deferred + the total stays frozen mid-gesture (useVirtualChat.geometryDefer): the two mutations redden disjoint sets, so neither behaviour is riding on the other's pin.
  • Follow re-engagement direction, growth-follow scope, same-frame reprice, revisit fetch bound, older-walk page cap, fork tail identity, fork fail-closed — all pinned, all mutation-checked.

An existing pin was tightened rather than relaxed to admit the reserve: leaves an inert chip glyph-free asserted "no svg element", 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: main had 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 keeping main'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):

Phone: archived history reached by scrolling

Desktop (1440px) — landed at the bottom of the same transcript:

Desktop: transcript landed at bottom

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.mjs record per-frame scrollTop while 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.

@buluoray
buluoray requested a review from a team September 2, 2026 16:21
@buluoray
buluoray requested a review from a team as a code owner September 2, 2026 16:21
@buluoray
buluoray requested a review from CrysisDeu September 2, 2026 16:21
@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 2, 2026
@buluoray

buluoray commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 6c4074ecf — fixes a fourth field-reported symptom: parked at the bottom of a long session with nothing streaming, the transcript bounced by itself.

A new rig scenario (website/scripts/scroll-rigs/rigbottom.mjs: land at the bottom, never touch the page, watch the bottom row for 90s while idle prefetch lands pages above) reproduced it deterministically. Two causes, both fixed:

  • The measure farm and the live window's ResizeObserver were both pricing the same rows. The farm renders a row in its default disclosure state, which can differ from the live row's by thousands of px (a collapsed tool group), so the two writers overwrote each other through the remount cycle their own announcements caused — a persistent ±2666 px oscillation against a parked reader. Mounted rows now belong to the RO exclusively (picker skips them; farmRecord drops a reading for a row that mounted between pick and measure). Pinned by a new measureFarm test.
  • A large reprice landing under a bottom-pinned reader was re-pinned only post-paint, exposing one visible frame of the shifted transcript. The height-commit consumer now re-targets the bottom pre-paint in the same commit for a stick reader.

Bottom rig before: sustained ±2666 px oscillation. After: storm gone; remaining signals are the initial hydration pin and ~132 px sentinel toggles.

@bolichen97

Copy link
Copy Markdown
Collaborator

Audit note — #8001 fixes the same stand-down as this PR; consolidation needed

#8001 deletes the same TRIGGER 1 stand-down in website/src/hooks/virtualizer/useVirtualChat.ts that this PR does, by a different and narrower mechanism. Only one design can land at that site. The audit reads this PR as the more complete answer and #8001 as the one to consolidate into it — but #8001 carries one idea worth taking.

The overlap

Both PRs delete the SAME silent stand-down at the SAME site: TRIGGER 1 in website/src/hooks/virtualizer/useVirtualChat.ts, where captureTopAnchorFrom returns null because no visible row's key survived a front growth, so if (prependAnchor) is skipped, part 1 never rebases the window and scrollTop never moves while N rows materialise in front. On main (blob aefb9aee1f, verified byte-identical to #8001's pre-image) that block is unchanged. Each PR's own prose names the identical mechanism: #8001 "no visible key survives ... captureTopAnchorFrom returns null, the capture stands down"; #7916's in-code comment "there is no surviving key to fall forward to, the capture stands down, and the landing hits the reader uncompensated", and its body "Tree-math fallbacks cover the three anchor-miss holes". Two incompatible designs for one requirement: #8001 re-finds the topmost visible row by POSITION (const j = idx + inserted; items[j]) and feeds it to the existing DOM-measured part-1/part-2 path; #7916 keeps the capture id-based and compensates by ARITHMETIC when it misses. Both then rewrite the SAME test case in website/src/test/useVirtualChat.prependAnchor.test.tsx ("shows no blank band when a prepend retires EVERY visible key") and flip the SAME assertion expect(readScrollTop()).toBe(scrollBefore) in the same direction — #8001 to toBeGreaterThan(scrollBefore) plus expect(after[0].idx).toBe(visible[0].idx + 10), #7916 to toBe(scrollBefore + 10 * 100). In that harness's uniform 100px geometry the two designs produce the SAME number, which is the clearest proof that one user-visible outcome is being implemented twice. Only one design can own that branch: if both landed, #8001's positional capture makes prependAnchor non-null, which renders #7916's !shiftAnchorRef.current arithmetic branch dead and de-fangs its native-anchoring-subtraction test. Coverage is asymmetric: #7916 subsumes #8001; #8001 is a subset of ONE of #7916's dozen-odd fixes, so #7916 is in no way redundant with #8001. Hence CONSOLIDATE on #8001 — its half must be dropped, or must replace #7916's, decided once — rather than CLOSE: #7916 is unlanded, is a 64-file mega-PR, and its base for this very file is one virtualizer commit stale, so retiring the small mutation-verified fix outright would leave the defect live on main for an unbounded interval.

Why #7916 is the one to build on

Completeness 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 getStableId: stableAnchorId, the row's TAIL message) so the miss often never happens; (2) prependCountRef.current = itemCount - prependPrev.count hoisted OUT of the if (prependAnchor) guard so a failed capture still reaches part 1; (3) a part-1 // ANCHOR-MISS FALLBACK summing offsetIndex.getHeight(i) over the inserted block and writing insertedPx - max(0, nativeAdj), subtracting Chromium's native CSS scroll anchoring so the write cannot double-compensate; (4) a part-2 // CONSUME-MISS FALLBACK for the anchor row unmounting between capture and consume, which #8001 does not address at all. #7916 also reproduced on a phone rig and a real iPhone with before/after counters (wLost=2); #8001's own body says "Not reproduced in a live browser in this PR". #7916 additionally restructures the very part-2 consumer #8001's anchor feeds (stage !== 'ready', anchorIdOf), so #8001's patch would need re-authoring against a changed protocol anyway. Counter-weights observed and not decisive: #8001 is far more reviewable (2 files, +59/-13 vs 64 files ~+3800) and sits on main's current blob for this file, while #7916's base predates #7811. Neither PR has any review or approval.

What was deliberately discounted

Discounted 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 (evaluateAutoPin, lastUserScrollAtRef), never TRIGGER 1 — irrelevant to this pair; (d) authorship of neither PR was considered; (e) #8001's cited refs #7045 and #4394 are ISSUES (no code), cited "Related:" only, so they cannot cover anything; (f) the 5 commits main gained since #8001's merge base touch no virtualizer file, so #8001 is not stale; (g) PR size/reviewability was noted but not allowed to decide the survivor.

What to harvest from #8001

If #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 prependPreScrollTopRef / nativeAdj bookkeeping and its "subtracts the browser's native scroll-anchoring correction" simulation test; (2) #8001's new test holds the reader by POSITION when a front growth retires every visible key, which exercises a 1000-row front growth (vs #7916's 10) and pins the anchor row's screen offset within 1px; (3) #8001's Pattern-harvest rule, which generalises across this whole hook: a compensation path whose anchor-not-found branch is a silent no-op is never neutral on a list whose front grows. Carry into whichever design lands: both share an unstated assumption that inserted = itemCount - prependPrev.count is ALL front growth, but TRIGGER 1 also arms on a commit that prepends older history AND appends a tail row — there #8001's idx + inserted names the wrong row (turning today's no-op into an over-correction) and #7916's height sum over indices 0..inserted-1 prices the wrong block.


From a repository-wide duplicate/overlap audit of every pull request open against main, re-run against the current tree (origin/main 680baf9448dc). This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.

@buluoray
buluoray force-pushed the perf/chat-history-polish branch from 4a67916 to 75931b5 Compare September 3, 2026 03:31
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 5bf553b062c16c53fc719b52dc171126f28dc3b6 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 evaluateAutoPin, mutation-verified pins. One structural coupling is worth flagging.

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

  • renderFarmItem in ChatPage.tsx hand-mirrors the live transcript's row wrappers (TurnBlock props, group wrapper classes), and the fidelity contract is a comment: a future change to the live wrapper that skips the farm copy makes MeasureFarm persist wrong heights per width bucket, silently reviving the estimate-correction jitter this PR eliminates. Extracting one shared row-wrapper renderer used by both paths would make the contract structural instead of disciplinary.
  • The mid-rotation branch in api_chat_slot_detail serves the full chained corpus in one response ("Serve every row at its true position; no cursor needed") — correctness-first and documented, but a chain rotated on a later member re-pays exactly the 6.2 MB/441 ms switch cost this PR removes; worth a follow-up cursor scheme if that shape turns out common.

[DESIGN-REVIEWED] 5bf553b

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of 5bf553b062c16c53fc719b52dc171126f28dc3b6 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • Finished shell tools still announce "Running" to assistive tech. ToolCallLine.tsx:961–963: the sticky activity row's visible text drops the "Running ·" prefix once liveShellActivity is false, but the sr-only aria-live span keeps rendering activityViewer.running for the life of the turn — every completed shell tool in every transcript tells an AT user it is still running (frequent × misinformation × persistent). Fix: switch the sr-only string to a completed/elapsed key when frozen, mirroring the visible branch — and give the sighted frozen state a label too ("Ran · 12s"), since a bare elapsedLabel is a lone number on cold read.
  • staleShellHeal reloads with no dirty-state guard. installStaleShellHeal fires window.location.reload() ~3s after boot on a stale shell; the composer draft lives in memory (draftRef, slot draft restore — no storage), so a user who opened the app and started typing loses the draft (rare × work loss × once per heal). Fix: skip the heal while the composer holds text and retry on the next boot/idle.

[UX-REVIEWED] 5bf553b

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 5bf553b062c16c53fc719b52dc171126f28dc3b6 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 ships

Intent: make long-transcript chat usable on a phone — archived history reachable, no bounce/teleport/snap while pages land. FIX.

  1. Scrolling up past a rotation boundary reaches archived history (pagination, cursor, fork-index parity) — justified
  2. History-read failure answers retryable 503 instead of a silently shorter transcript, one shared helper replacing three hand-rolled recoveries — justified, cause-level
  3. Session switch fetches a bounded page with coverage-checked unbounded retry, not 6.2 MB — justified, measured
  4. Transcript drift eliminated at its source: stand-in geometry equalized (pierre-plain), straddle predicate corrected, chip-glyph/diff-open space reserved — justified, cause-level
  5. Pierre surfaces mount progressively (staging queue, warm swap, measure farm, spacer skeleton) — justified, measured long tasks
  6. Automatic bottom pins now require a live run and live near-bottom geometry; composer growth no longer drags the transcript — justified, live reports
  7. Older-history fetching gated on reader intent (visible earlier-bar, input-budgeted walk, expiring idle-prefetch authorization, motion-aborted landings) — justified
  8. Stale service-worker shell self-heals (shell cache refresh, dirty-build hash, boot probe) — justified, reported defect
  9. Assistant footer overflow menu moved into the action row; footer no longer retracts — move justified by measured ~108 px flicker
  10. All touch action-row icons shrunk h-5→h-4 (touchActions.ts) — rides along, unjustified relabel

Watch

  • Item 10 changes every touch action row app-wide (code blocks, diffs, message footers); the only stated cause is "reads crowded at the larger size" — a preference, no named person failing. Tap targets hold at 40 px, so the harm either way is small, but it should be its own decision.
  • earlierAffordanceInView's doc claims it "subsumes the special case the old geometry test needed", yet handleTopReached still runs shouldAutoFillOlder, the farm-measured loop, and the follow gate behind it. If the admission rule subsumes the proxies, the retained proxies are shrink candidates; if it doesn't, its doc overstates.
  • chat_utils.py black reformat + baseline prune rides in a fix branch; AGENTS.md asks for that in its own commit.

Subtractions

  • Drop the drop_persisted_tail_prefix re-export shim in chat_fork.py:45-51 — grepped drop_persisted_tail_prefix repo-wide: 0 non-test importers of the chat_fork spelling (only test_fork_tail_not_double_appended.py:13; chat.py imports only the handler). history_projection.py:58 owns it; call that name directly and point the test there.

[FIRST-PRINCIPLES-REVIEWED] 5bf553b

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 5bf553b062c16c53fc719b52dc171126f28dc3b6 — this comment is updated in place on each push.

Review details

I've now traced all five candidates against the actual code. Summary of my falsification:

  • Candidate 1 — dropped. The height-sync growth that matters (token appends within a turn) keeps displayItems.length constant, so streamingIndex is correct exactly when it matters; the deferred lag only diverges at length-change boundaries where the new tail row isn't rendered yet (nothing to sync). The isRunning mismatch is a transient one-commit spinner flicker that self-corrects — no 80+ observable defect.
  • Candidate 2 — survives (FINDING). Newly-added aboveFoldReprice write omits the nativeAdj subtraction its sibling paths carry; the file itself documents that native anchoring shifts scrollTop and a full-height write on top doubles.
  • Candidate 3 — dropped. MeasureFarm sits in belowRows, a DOM descendant of the scroller div; refs are assigned before passive effects run, so scrollerEl() is non-null at first effect run, and a scroller remount remounts MeasureFarm with it.
  • Candidate 4 — dropped. Requires a conjunction of rare conditions (net≠displacement prepend + tail growth, anchor row unmounting between commits on a slow device, released reader) — an acknowledged fallback approximation; (a) does not clear "occurs in practice" at 80+.
  • Candidate 5 — survives (FINDING). Genuine logic gap in new code: admitted resets only on a drain that emptied waiting.

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 el.scrollTop + aboveFoldReprice with no nativeAdj term (unlike its sibling prepend/consume-miss paths at 2620-2621 and 2719-2721), so on Chromium/Firefox where the scroller's overflowAnchor: 'auto' has already shifted scrollTop for an above-viewport row resize (e.g. an async widget iframe finishing load), the same Δ is applied twice and a released reader's content jumps by ~Δ → Fix: capture pre-resize scrollTop for this path and write only aboveFoldReprice − max(0, nativeAdj), or restrict the write to engines without native anchoring.

FINDING — website/src/components/pierreStaging.ts:123 — admitted resets only in the drain's terminal else admitted = 0, and the eager branch (admitted++; release()) never calls schedule(), so successive small bursts that never queue accumulate admitted until it hits EAGER_ROWS (4), after which the next first-mount — including an on-screen live-turn surface — is forced through the idle/scroll-hold queue rather than mounting eagerly, contradicting the documented per-burst reset → Fix: reset admitted on a burst boundary independent of the queue (e.g. when waiting is empty at registration with no drain pending).

[OPUS-REVIEWED] 5bf553b

Verdict parsed from the review's SHA-scoped output markers for commit 5bf553b062c16c53fc719b52dc171126f28dc3b6.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 5bf553b062c16c53fc719b52dc171126f28dc3b6: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @buluoray overrides the GPT 5.6 finding for 5bf553b062c16c53fc719b52dc171126f28dc3b6; the recorded reason is authoritative for this commit.

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:
/ai-review override gpt 5bf553b062c16c53fc719b52dc171126f28dc3b6: <one-sentence reason>

@buluoray
buluoray force-pushed the perf/chat-history-polish branch from 75931b5 to e7d5a29 Compare September 3, 2026 04:06
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 3, 2026
@buluoray
buluoray force-pushed the perf/chat-history-polish branch from e7d5a29 to 2cacb20 Compare September 3, 2026 04:32
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@buluoray

buluoray commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Root cause of the e7d5a299f mass-reversion both blocking lanes flagged — confirmed real, now fixed in 2cacb2067.

The Design Review and UX Review BLOCK verdicts were correct: e7d5a299f was assembled via git reset --soft origin/main at a moment when the local origin/main ref had just advanced under the operation — the commit's parent was the new main but its tree was built from the previous main, which silently reverted the ~12.4k lines main landed in between (including the security fixes and the useMayLeaveForNavigation guard both lanes named).

2cacb2067 rebuilds the same changeset by three-way application onto fetched origin/main (2d55e32):

  • The intervening main work is intact — the diff footprint is back to the PR's own 74 files (+5.6k/−0.5k), with zero deletions of main-landed code.
  • The landed fix(virtualizer): hold the reader by position when a prepend retires every key #8001 (0201f9198) and this PR's TRIGGER 1 consolidation are merged semantically (stable-id anchor first, landed positional re-identification on miss, arithmetic remainder last); all 22 of main's prependAnchor tests pass verbatim alongside this PR's 2 additions.
  • Main's burned-to-zero eslint ceiling is honored: the corrected tree lints at zero warnings under --max-warnings 0.
  • Main's ToolCallLine any-cast cleanups are kept; the one legacy status-line collapse test is replaced by the freeze contract the sticky rework establishes (the collapse ease it pinned is the tool-rhythm bounce this PR removes).

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.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@buluoray
buluoray force-pushed the perf/chat-history-polish branch from 2cacb20 to a1851ae Compare September 3, 2026 04:48
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 3, 2026
@buluoray

buluoray commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Disposition — GPT 5.6 review of c25c33950

Both findings are correct and both are FIXED in 8a6f50929. The previous round's
two findings (the (ts, role) collision, and max-two-buttons-per-row) do not
appear on this SHA.

1. chat_fork.py — failed full-corpus read now FAILS CLOSED with a retryable 503.

This overturns a judgment call I had written into that branch as "approximate
indices beat refusing the fork outright". That reasoning does not survive the
mechanism: the fallback prepends only THIS key's rotated head, so a rotation on a
LATER chain member leaves the earlier members' rotated rows missing and shifts
every index. An index-addressed fork then copies different messages than the
reader pointed at
, with nothing on screen to say so. A refusal is visible and
recoverable; a silently wrong fork is neither — so "approximate" was the worse of
the two, and the bot's suggested remedy is right.

Now returns 503 fork_corpus_unreadable, the same retryable shape the snapshot
loop above already uses for fork_snapshot_unstable. The legitimate prepend (full
read fine, chain simply not mid-rotation) is untouched, and the new guard is scoped
to the except block so it cannot pass by accident. The module's coded-refusal
ratchet moved 27 → 28, which is the new refusal being counted.

Mutation-verified: removing the refusal so the flat prepend runs again reddens
exactly the new guard.

2. mint-pod-url.py — the bearer-token write is no longer a predictable shared path.

Also correct. /tmp/kc-pod-url.txt in a shared /tmp lets a pre-planted symlink
redirect the write (leaking a live dashboard token) or truncate a file the victim
owns. The write is now:

  • into $KIROCREW_SCRATCH (session-owned, reclaimed with the session), falling
    back to $TMPDIR then the cwd — the shared /tmp is deliberately not a fallback;
  • os.open with O_CREAT | O_EXCL | O_NOFOLLOW and mode 0600, so an existing
    path is refused rather than followed and a symlink is refused outright.

44 tests green across the three fork suites; flake8 and mypy clean for both files.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • PR #6825 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6825: MERGE_DISCUSSION. Two open PRs rewrite the same switchSlot thunk for unrelated reasons and already conflict. Land order needs to be decided explicitly, and whoever rebases second must carry detailSeq onto BOTH of 7916's return paths. Files: website/src/store/chatSlice.ts, website/src/store/chatSlice.olderHistoryCursor.test.ts.
  • PR #7255 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7255: REBASE. Highest-conflict open pair: 94 files and +6420 of new ChatPage.tsx behaviour against a PR that empties that file, plus 9 shared test files. Neither can absorb the other; a landing order has to be agreed, and whichever loses re-places its hunks by ownership. Files: website/src/pages/ChatPage.tsx, website/src/test/ChatPage.scrollShell.recipe.test.tsx.
  • This PR is OVERLAPPING with PR #4913. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7916: REBASE. Adjacent, non-conflicting halves of the same problem (server-side advisory vs client-side recovery). Neither subsumes the other: 4913 cannot help a phone whose service worker is serving a days-old shell, and 7916's probe tells an operator nothing. Files: website/vite.config.ts.
  • This PR is OVERLAPPING with PR #7821. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7916: REBASE. Code-near only. Different listeners, different concerns, no shared behaviour. Files: website/public/sw.js. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • This PR is OVERLAPPING with PR #8235. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7916: REBASE. Same handler branch and same projection class, opposite pressures (add a corpus vs bound how much of a corpus is materialized). They are complementary in intent but cannot both land unreconciled: whichever merges second must extend the other rather than replace it, and the ordering decision is a design call, not a rebase. Files: src/kiro_crew/dashboard/chat_handlers.py, src/kiro_crew/history_projection.py.
  • This PR is OVERLAPPING with PR #8300. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7916: REBASE. Different goals, no design conflict, but three overlapping call sites and one contested class name. Whichever lands second rebases and must preserve the other's property - specifically the leading-5 line box, which no test outside 7916's own CodeBlock.staging.test.tsx pins. Files: website/src/pierre/PlainCodeFallback.tsx, website/src/pierre/index.tsx.
  • This PR is OVERLAPPING with PR #8316. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7916: REBASE. The closest thing to a duplicate in this set: same user-visible problem, same file, same render branch, two incompatible answers. It is not DUPLICATE because the code changes are materially different (defer-and-reserve vs never-mount) and 7916's queue also serves code blocks. One strategy should be chosen deliberately before either merges. Files: website/src/components/FileChangeChips.tsx.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@buluoray

buluoray commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Disposition — chat_handlers.py:1943 next_before = rotated_count: rebutted, archived rows stay reachable.

The finding says the except Exception branch's "approximate cursor" makes archived rows unreachable. Traced the cursor end to end; it does not.

next_before is an index into one specific corpus, and read_messages_chained_full's own contract names it (history_projection.py:365-375):

Per chain key the rotated rows PRECEDE the live file's rows — rotation drops the file's head, so segment order (filename timestamp) followed by the surviving file is that key's true chronology […] This is the pagination corpus: the index space before/next_before cursors live in, shared with the fork index path so a rendered row's index resolves to the same message everywhere.

So index 0 of that space is the OLDEST archived row, and the archived block occupies [0, rotated_count). A cursor of rotated_count therefore asks for exactly "everything before the live file's first surviving row" — the archived block itself. It is the same value the non-degraded sibling branch two lines below uses (:1947), and it is consistent with the normal path's next_before = start (:2003), which is an index into the same corpus.

What the branch does with the rest of the response matters too: it sets has_more = True and total += rotated_count, so the affordance stays advertised and the client's total still counts the archived rows. The next request re-enters the handler and re-attempts the full read, which is why the comment calls the cursor "approximate" rather than wrong — the degradation is one page of possible duplicates (deduped client-side), not a lost tail.

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: history_projection.read_messages_chained_full (corpus + index-space contract), chat_handlers.py:1886-1947 (both cursor assignments and the has_more/total adjustments), chat_handlers.py:2003-2042 (the non-degraded cursor and the response shape).

The sibling finding at history_projection.py:391 is accepted and being fixed — the concatenation there has no guard against a crash window between archive creation and the live-file head rewrite, which is the same class as the chat_fork.py tail double-append already fixed in this PR, and drop_persisted_tail_prefix's identity rule is the reusable part.

@buluoray

buluoray commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Disposition — Bundle Size Gate: budget re-measured, not waived. The overage is main's growth, not this PR's.

The failure was assets/t-Cr8LfdHT.js: 740.0 KB exceeds its 740.0 KB budget by 4 B (chunk 't'). Four bytes.

t is the i18n runtime chunk — the i18next singleton, initI18n, and the English catalog. This PR adds no i18n key, and none of its code lands in that chunk. Verified rather than assumed: built locally and grepped the built t chunk for three tokens unique to this PR's diff (unicodeBidi from the composer's off-screen measurement twin, load-earlier-messages from the older-history admission rule, and the idle-prefetch authorization window) — zero occurrences of each. The chunk hash also differs between my build and CI's (t-Bql18Z_H vs t-Cr8LfdHT) because CI builds the merge ref, which carries main's newer catalog: locally the chunk is 738.6 KB, under the 740 KB ceiling; on the merge ref it is 740.0 KB + 4 B.

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:

Re-measured 2026-08-27 at 702 KB: the previous measured 641 KB note was ~60 KB stale, which left main sitting a few hundred bytes under its own ceiling, so any PR adding an English string tripped this gate rather than the new library or surface it exists to catch.

The 38 KB of headroom that re-measurement bought has since been eaten, so the same trap re-armed. Refreshed it the same way: t: 780 * KB, // measured 740 KB — ~5% headroom, the margin the all entry carries, with a comment recording that it recurred and how the four-byte trigger was attributed. node scripts/check-bundle-size.mjs passes locally after a vite build --mode analyze (804 chunks within budget).

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.

@buluoray

buluoray commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Disposition — next_before mid-chain fallback: my earlier rebuttal was wrong, and is withdrawn

Fixed in 31d4e4e20.

I rebutted this finding in 5537834648 on the grounds that rotated rows precede the live file's, so a cursor of rotated_count requests exactly the archived block — the same value the sibling branch two lines below uses. That premise only holds when the rotation is on the FIRST chain member. This round's wording named the case my rebuttal skipped over ("later-member rotation"), and it is decisive.

The branch's own entry condition proves it. It runs only when chain_mid_rotation is true, and that predicate's docstring in history_projection.py:479 reads:

True when any chain member AFTER the first has archive segments. […] A later member's archive is sandwiched between […]

So by construction, in exactly the case this except branch handles, the archive is not the corpus's first rotated_count rows. A prefix cursor of rotated_count addresses the wrong span, the page it returns does not advance past the sandwiched rows, has_more then goes false, and those rows are unreachable — with no error the reader can see or retry. The old comment's "dup rows dedupe client-side" defence answers a different failure than the one that actually occurs.

The sibling elif rotated_count > 0: keeps using the same value, and that is correct: it runs when the rotation is on the first member, where rotated_count is the boundary. The guard is therefore scoped to the branch, not to the value — a test pins that the sibling still uses it, so the fix cannot spread to the branch where it would retire a working affordance.

Fix: fail closed with a retryable 503 history_corpus_unreadable. This is the same shape the fork handler already returns for this identical corpus and identical reason (chat_fork.py, "the source session's history could not be read; please retry") — a reader who can see the failure and retry is strictly better off than one silently handed a truncated transcript.

test/test_mid_chain_read_failure_fails_closed.py pins it, slicing the except body between the read that can raise and the sibling elif so a guard added elsewhere in the handler cannot satisfy it. Mutation-verified: restoring the old next_before = rotated_count fallback reddens exactly the two intended cases (the third, pinning the sibling, stays green).

Also in this push: the earlier read_messages_chained_full concatenation finding is fixed, and the fork handler's flat-prepend branch — a second concatenation of the same shape that the function's own guard does not cover, because it runs when that read was not used at all — now applies the same identity rule.

@buluoray

buluoray commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Disposition — c7d007c19

1. Both new backend blockers are real, and they are the sites I missed

GPT found the two remaining members of a class I had been fixing one site at a time. That is the finding, so this round fixes the class instead of the sites.

history_projection.py:406 — malformed segments. I had made OSError mark the corpus incomplete, but left both except ValueError: continue paths silently skipping. A healthy read would have contributed those rows, so dropping them shortens the corpus and shifts every index above it — and as the finding notes, a retry rotation can archive the same rows successfully, after which a partial read of the damaged segment duplicates them. Both now mark it incomplete.

Two skips in that loop are deliberately left alone, and they are not the same thing:

  • if not lines — a zero-byte segment. Archiving is a precondition of the live-file rewrite, so a rotation that got that far and no further never rewrote the live file: those rows are still in it, and a healthy read of an empty file contributes nothing either. No index moves.
  • reason != "rotate"classification, not damage. compact, foreign-dedup and rewrite archives are other reasons and this corpus is size-rotation only (the function's own docstring). A healthy read skips these too; turning it into a refusal would 503 every session that also has a compact archive, which is most of them.

chat_handlers.py:2001 — the third call site. all_msgs = [] on a failed chained-full read, on the legacy pagination path — the reader's only way back into a rotated archive. It answered 200 with the live tail and has_more false: the reader is told their older history does not exist.

2. The consolidation, because three sites is the actual defect

This is the third round on the same shape, so per review discipline it is fixed at the abstraction rather than patched again. Three handlers had each independently wrapped a corpus read in try/except and substituted their own local encoding of "there is nothing there" — rotated = [], _rotated_head = [], all_msgs = []. Every one of those turns a read failure into a successful response describing a shorter transcript, and in an index space the shortening also moves every index above the missing rows.

chat_utils.history_corpus_unreadable(code) is now the single answer, and all four dashboard call sites go through it. A test pins that no dashboard file builds its own 503 body for this, so the next call site cannot quietly pick a different answer.

Two existing tests had to follow the move: one asserted the inline status=503 literal in chat_fork.py, and the coded-refusal self-check counted inline sites (29 → 27, since the helper sets the code by construction rather than at each site). Both updated with the reason recorded.

Mutation-verified, each reddening exactly its own case: header parse guard, row parse guard, and the legacy-path call site. isort / black / flake8 clean, mypy clean on the touched files (the 3 remaining are pre-existing in wecom/client.py), 2377 related backend tests pass.

3. max-two-buttons-per-row — override posted separately

Approved by the repo owner. Posted as its own comment so the workflow's first-line parse sees it.

4. Opus 4.8 Review — harness failure, not a verdict

Its own review comment on this PR reads ✅ no blocking findings. The check fails earlier, in the discovery pass, with the same error on two consecutive and unrelated SHAs (ff3ecff3a, d018c674a):

Discovery produced no [OPUS-DISCOVERY] marker for <sha>, so its output cannot be trusted as a complete candidate list. Failing closed […] re-run the workflow.

Two occurrences on different SHAs is no longer transient. The lane's own code carries a second, adjacent failure mode for a candidate list over MAX_CANDIDATE_BYTES — this PR trips the no-marker branch rather than that one, but both are size-shaped, and a 114-file diff is a plausible cause the lane cannot report on itself. Flagging for a maintainer: this is the lane failing to run, not a finding to fix, and it will keep failing closed on a diff this size. Happy to split the PR if that is the preferred remedy, but that is a decision rather than something I should do unilaterally.

@buluoray

buluoray commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

/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. share-message, fork-from-here and plan-from-here are already DropdownMenuItems on origin/main — the menu is not introduced here, so the override is not being asked for a new affordance:

$ git diff origin/main...HEAD -- src/pages/chat/AssistantMessage.tsx | grep -E '^[-+].*data-testid="(fork-from-here|share-message|plan-from-here)"'
+          <DropdownMenuItem data-testid="share-message" ...
+              data-testid="fork-from-here"
+              data-testid="plan-from-here"
-            <DropdownMenuItem data-testid="share-message" ...
-                data-testid="fork-from-here"
-                data-testid="plan-from-here"

And on main the trigger has its own container rather than sharing the action row:

$ git show origin/main:./src/pages/chat/AssistantMessage.tsx | sed -n '386p'
      {(onFork || onPlanFromHere) && <div className={ACTIONS_REVEAL_CLS}>

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 HOVER_NONE_ACTIONS_ROW_CLS makes both rows permanent. The row's literal control count grows by one; the vertical space the reader loses to chrome halves. Approved by the repo owner on that basis.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@buluoray marked the gpt AI finding as false positive, not applicable, or explicitly accepted for c7d007c19fd35adb860e3da85d162ee2fec22d2e.

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.

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.
@buluoray

buluoray commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Disposition — both advisory CONCERNS on 5bf553b06

Corrected. An earlier version of this comment said two of these were fixed in a follow-up commit. That commit has been reverted and the branch is back at 5bf553b06, so nothing below is applied here. Both blocking lanes passed on this SHA (GPT ✅, Opus ✅) and these two lanes are advisory, so the right disposition for all four items is a comment — not a push that would void a green PR's checks and both blocking verdicts.

UX 1 — frozen tool rows announce "Running" after completion: accepted, deferred to a follow-up

Real, and reachable because of this PR: keeping the activity row mounted for the rest of the turn is what removes the per-tool height bounce, so the row now outlives the run. The VISIBLE label already switches (liveShellActivity ? running · elapsed : elapsedLabel) while the sr-only live region beside it says running unconditionally — so a finished shell tool reads as still going, and only to the people who cannot see the visible label disagree with it.

Fix is a conditional on that one span plus a finished_in catalog key. Landing it here would cost a full CI cycle and re-earning both blocking verdicts for an advisory finding, so it goes in its own change.

UX 2 — phone footer wraps to three control rows: out of scope

Confirmed visible in the committed screenshot, and deliberately not in this PR — the footer width / touch-target / timestamp work is a separate change. Not introduced here.

FP subtraction — delete the drop_persisted_tail_prefix shim: rebutted, the grep missed two call sites

non-test consumers: 0 (grepped drop_persisted_tail_prefix)

That count is wrong. The shim has two non-test consumers, both inside chat_fork.py itself, which call it by its bare name rather than through the alias:

$ grep -rn 'drop_persisted_tail_prefix' src/ | grep -v 'def '
chat_fork.py:24:  from kiro_crew.history_projection import drop_persisted_tail_prefix as _drop_persisted_tail_prefix
chat_fork.py:51:      return _drop_persisted_tail_prefix(full_disk, tail)      <- the shim body
chat_fork.py:722:     all_messages = _full_disk + drop_persisted_tail_prefix(_full_disk, _tail)
chat_fork.py:745:     all_messages = _rotated_head + drop_persisted_tail_prefix(

An import-shaped grep sees the alias on line 24 and the test import, and misses 722/745 because an intra-module call needs no import. The shim is not dead — it is the name those two lines resolve through.

FP subtraction — __virtDbg reads in rigphone.mjs: valid, declined here

Verified independently: 0 writers in website/src, so dbg was always {} and every bigs[].near always []. Worth removing, but it is dead-code tidying in a rig script — not something to spend a green PR's CI cycle and two blocking verdicts on.

FP watch — touchActions.ts h-5 → h-4 across 6 consumers

Fair, and FP names why it could not confirm the declared status: the description is truncated at 8 KB. The padding keeps 40px targets so it is not a defect, but "reads crowded" is a preference rather than a named failing user. Left as a keep-or-split call for the repo owner.

@buluoray

buluoray commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

/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. read_rotated_messages concatenates archive segments with a bare rows.append(row) and no cross-segment identity check, and _maybe_rotate writes the archive before rewriting the live file (deliberately — until the archive lands, the live file is those rows' only copy). A crash between those two steps leaves the same prefix in both places, so the next rotation archives it again as the following segment, and the reader serves both. Because this corpus is the pagination index space that before / next_before and the fork index path resolve against, a duplicate does not merely appear twice — it shifts every index above it, silently.

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 main; what this PR adds is the reader that makes it observable, because main's read_messages_chained chains live files by tab_id and never reads the archive at all.

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 drop_persisted_tail_prefix identity rule applied one level down, at the segment boundary — and lands in #8484 with a test that builds two deliberately overlapping segments, mutation-verified.

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.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@buluoray marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 5bf553b062c16c53fc719b52dc171126f28dc3b6.

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.

This decision applies only to this commit. A new push requires a new judgment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants