fix(desktop): keep "return to latest" at the tail - #4883
Conversation
Pin the scroller before the latest range loads so the reading anchor cannot pull the range back to the Turn the reader left, and replay a history request that arrived while another load was in flight. Generated-by: Claude Code
0b2df41 to
278beec
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 278beecd7ef0947ddbe266fe080703d59797ca25.
The patch fixes the same-session tail-pin race and replays a later same-session history request, but the shared replay gate loses session/controller ownership. The inline P2 below remains reachable during normal session switching, so this is not ready to merge.
Validation completed on Node 24.18.1: clean install, build:test, Desktop typecheck, Desktop dist tests (2211/2211), renderer build, architecture checks (101/101), Biome lint/format, locale hygiene, ASF headers, diff check, and the paging-back Electron scenario repeated 20 times (20/20). A deterministic helper-level probe reproduced the cross-session misrouting. The exact-head hosted test check and a local full Storybook smoke both fail only on the unrelated product-sidebar-session-list--project-groups focus assertion; that required check still needs to become green. I did not run the native macOS performance benchmark.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
A history load in flight on one Session held a shell-wide gate, so a navigation on the Session switched to next was queued behind it and then dropped when the first load settled under the old Session's currency check. Keying the gate by controller keeps each Session's in-flight load and queued request to itself. Generated-by: Claude Code
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 76692daa8550f5c6a5f641cd8b16dc082db064ec.
The follow-up keys history-load ownership by transcript controller, so a load for the newly active Session starts independently instead of being queued behind and discarded with the previous Session. The added regression cases also cover late settlement isolation and same-controller latest-request priority. The prior P2 is resolved, and I found no new P0-P3 issues in this incremental change.
Validation on Node 24.18.1: clean install, build:test, Desktop tests (2218/2218), focused session UI-state tests (27/27), Desktop typecheck, renderer build, architecture checks (101/101), Biome, locale hygiene, ASF headers, and diff check. The exact-head hosted test check was still in progress when this review was posted, so merge readiness remains gated on its terminal result and independent human review.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
The gate tests detached their loads and awaited undefined, so a late rejection could not fail them and "await stale" guaranteed nothing. scenario.load now returns the load's real promise, and every test awaits the settle it asserts on. The pin-before-load order in ChatView's return-to-latest command had no fast regression: return-to-latest-pin.test.tsx renders the view in linkedom, parks the reader away from the tail, and asserts the click pinned synchronously and the reported anchor cleared while the load was in flight. Generated-by: Claude Code
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f8603a025e993845e236c310b35852d82c875c3a.
The increment since 76692daa8550f5c6a5f641cd8b16dc082db064ec changes tests only. The production per-controller history gate is unchanged, and the prior cross-Session P2 remains resolved. I found no new P0-P2 issues, but the two inline P3 comments identify assertions that do not currently prove the behavior they claim.
Validation passed on Node 24.18.1: build:test, Desktop typecheck, UI tests (385/385), focused Desktop tests (27/27), renderer architecture checks (101/101), Biome, formatting, ASF headers, diff check, and a clean patch-preserving merge-tree against current main 2bfea3c32eb9eb3a907d1a5a04a8229156810b26. The required hosted test check is green.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| authority = useTranscriptScrollAuthority(); | ||
| return createElement(Fragment, null); | ||
| }; | ||
| const probe = createElement(TranscriptScrollAuthorityProvider, { |
There was a problem hiding this comment.
[P3] Observe the authority that ChatView actually uses. This probe is mounted as a sibling of ChatSurfaceLayout, under its own TranscriptScrollAuthorityProvider, while the layout creates a separate provider around the chat. As a result, view.authority starts and remains independently pinned, so the assertion at line 275 can pass even if the production pin-before-load ordering regresses. Mount the probe inside the layout-owned provider (or otherwise expose that exact authority) so the assertion observes the state under test.
| await inFlight; | ||
| assert.deepEqual(scenario.sides.a.calls, ['before', 'latest']); | ||
| scenario.sides.a.settleLatest(); | ||
| await Promise.allSettled([queuedEarlier, queuedLatest]); |
There was a problem hiding this comment.
[P3] Do not use these queued-call promises as evidence that replay completed. loadTranscriptHistory returns immediately from the gate.pending branch, so both promises are already settled before the first load finishes; this await only happens to follow the replay continuation in today's microtask ordering. Either return a promise tied to the queued replay or await an explicit completion signal before asserting the final pending/callback state.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at f8603a0 against main (2bfea3c32e), +564/−29, seven files, mostly tests.
Both problems are real and both fixes sit at the owner. The jump-back is two lines: chat-view.tsx:747-752 now calls pinToTail() before await onClick(), so the reader anchor is cleared (use-chat-scroll.ts reports undefined while pinned, captureTranscriptReadingAnchor clears it, restoreSessionTranscriptRange has nothing to restore) before the range load resolves. That is inside the #4269 pin-owns-anchoring model, not a patch around it. The stall is the in-flight gate moving from the shell's boolean into loadTranscriptHistory (features/conversation/controller/transcript-reading-position.ts) and becoming queue-and-replay, keyed by controller in a WeakMap; since app-shell-effects.ts:415-448 builds one controller per active Session, the gate dies with its controller and the cross-Session P2 from the earlier review is closed. No new state mirrors the scroll authority.
Nothing blocking. Three P3s and two tests that can go:
packages/ui/src/use-chat-scroll.ts:140-142still documents the old contract ("it refuses a request while one is in flight"); this PR is the reason that sentence is now wrong. Change it to "queues the newest request and replays it".- In
return-to-latest-pin.test.tsxtheAuthorityProbereads a siblingTranscriptScrollAuthorityProvider, not the onechat-surface-layout.tsx:106wraps the view in, and a never-attached authority startspinned = true, so thepinnedassertion passes onmaintoo. Theanchorsassertion is the one with teeth (onmainthe thirdundefinednever arrives). Drop the probe, the getter and the import. - The second test in that file scrolls onto the position
pinToTail()just wrote, which the authority's echo branch swallows before any reader decision, so it proves "own echo is not reported", not what its name says. Delete it. Likewisereplays a queued load through its own Session callbacksinapp-shell-session-ui-state.test.tsrestates two neighbouring cases. app-shell.tsx:458useRef(new WeakMap())allocates a map per render; harmless,??=on first use if you touch the line anyway.
Body: "leaves the shell smaller" is −7 tokens in the ledger; call it net flat, with the concurrency semantics moved to a helper that has tests. Also worth one sentence on why the gate lives beside the controller rather than in createDesktopTranscriptRangeController itself, which is where "one load per transcript, newest queued" naturally belongs; the answer is presumably that setPending / onError are shell concerns, and that is fine.
No file or semantic overlap with #4259; #4885 fixes the smoke flake that blocked this one's required check.
Evidence boundary: static read against main plus the authority, range controller and shell effects; the 60-run spec loop, the 27 desktop tests and the ledger number are the body's claims, not rerun here.
AI-assisted review: drafted with Maka; I verified the pin-before-await ordering, the per-Session controller lifetime and the probe's provider scope myself.
Summary
Fixes #4882
Clicking 返回最新消息 after paging far back flashed the tail and jumped back to the Turn the reader had been on; fast wheel paging occasionally stalled one page short. Together they are the
transcript-scroll-cost.spec.ts:239flake onmain.Root cause of the jump-back:
ChatViewpinned the scroller to the tail only afterawait onClick()resolved, but the tail batch reaches the store before that. In the gap the unpinned scroller reported the top Turn as the reading anchor, and therestoreRangeeffect (which re-runs on everymessageschange) found it missing from the new range and calledloadAroundon it. The fix pins first: a pinned scroller reports no anchor, so the range that arrives has nothing to be pulled back to.Root cause of the stall:
loadTranscriptHistoryreturned silently while a load was pending. The scroller asks on every reader movement, so the request behind an in-flight load was lost and nothing re-asked. The guard now lives intranscript-reading-position.tsasloadTranscriptHistory, keeps the last request that arrived mid-flight (latestoutranksearlier) and replays it when the current load settles, still gated on the session and controller being current. Moving it out ofapp-shell.tsxis what the renderer architecture ratchet asks for and leaves the shell smaller.The gate itself was one shell-wide object, shared by every Session. A navigation on the Session switched to next queued behind the first Session's in-flight load and was then dropped when that load settled under the old Session's currency check — the "Return to latest" click did nothing. The second commit keys the gate per controller (
WeakMap<object, TranscriptHistoryGate>); each Session's in-flight load and queued request stay to themselves, with the same-Session queue semantics unchanged.Overlaps with #4560, which rebuilds this area and removes the notice; that PR clears the anchor in
app-shell.tsxbefore the load, which locally still loses the race (the scroller re-reports the anchor 17 ms after the load starts).Verification
apps/desktop:npm run typecheck, renderer ratchet--writethen--base $(git merge-base HEAD origin/main),biome checkon the touched files,@maka/uitest:dist— all pass.transcript-scroll-cost.spec.ts -g "paging back" --repeat-each=60 --workers=4, macOS,main411512b vs this branch:The instrumented lines are from a throwaway build of
mainwithconsole.infoin the store, the restore path and the shell; the fix branch is un-instrumented.Second commit adds cross-session gate coverage to
apps/desktop/src/main/__tests__/app-shell-session-ui-state.test.tsand re-runs the paging-back regression on that head:AI use
Select exactly one:
Tool(s) and scope: Claude Code — trace analysis, instrumentation, root-cause bisection, the code change, and the per-controller gate follow-up with its regression tests; verified by the author.
Checklist
Does this PR entail a change in behavior?