fix(ui): bookmark the anchor a wheel pages from - #4911
Conversation
684807a to
00b8b00
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 00b8b00b48fc38ce3ca22f3c79051741cc93e77a.
I found no P0–P3 correctness, ownership, or concurrency issue in the two-file diff. The production change in packages/ui/src/use-chat-scroll.ts:151-166 reports the existing reading anchor before a bounded-edge wheel requests history, covering the no-scroll-event case while preserving the existing loader anchor and one-pixel native-anchor workaround. The new test exercises a restored unpinned position and verifies that the persisted reading anchor and load anchor are both current.
The exact-head hosted test check passed. I did not independently run the UI typecheck or tests because this checkout has no usable local TypeScript executable, and I did not run a browser/Electron smoke test.
Automated review notice: This is an AI-assisted review and does not replace independent human review.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. Approving exact head 00b8b00b48fc38ce3ca22f3c79051741cc93e77a: the production change reuses the existing reading-anchor authority, and the regression fails when the fix is removed. The previously completed focused tests (19/19) and native scrolling checks (3/3) support the behavior.
The remaining suggestion is P3: reduce duplicated test scaffolding while preserving the regression. It is optional maintenance cleanup, not a correctness blocker. No production redesign is requested. Refresh integration checks if the head changes before merge.
AI assistance disclosure: Codex agents performed the independent source/test/browser checks; Astro-Han authorized approval with this non-blocking suggestion.
中文
感谢这个范围明确的修复。批准当前 head:生产改动复用已有阅读书签权威,移除修复后回归测试确实失败;此前 19 项定向测试和 3 项原生滚动检查通过。
剩余建议定为 P3:精简重复测试夹具,保留回归保护。它是可选维护性整理,不是正确性阻塞,也不要求重构生产代码。若合并前 head 变化,需刷新集成检查。本次由 Codex 代理执行验证,Astro-Han 授权批准并附此非阻塞建议。
A wheel at the top edge moves nothing, so no scroll event refreshes the reading anchor. The bookmark keeps naming a Turn the new range evicted, and the restore effect loads around it over the range paging just published. Generated-by: Claude Code (Opus 5)
The standalone paging-reading-anchor file repeated the global save/restore, React root lifecycle, LinkeDOM setup and environment wiring already present in use-chat-scroll.test.tsx. The case now runs there under a shared file-local environment installer, keeping the no-scroll-event edge, the real hook and authority composition, and the ordering assertion that the anchor is reported before onLoadEarlierHistory runs (removing the fix still fails it). Generated-by: Claude Code (Opus 5)
00b8b00 to
f501a2f
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks, the requested fixture simplification is complete at f501a2f75. The new commit changes only tests: it moves the regression into the existing scroll suite and shares its setup. The production anchor fix is unchanged. A fresh review ran the three focused tests successfully; removing only reportReadingAnchor.current?.() made the relocated regression fail with the old anchor. It still composes the real hook and TranscriptScrollAuthorityProvider. No remaining actionable issues; refreshing the prior approval for this head.
中文
夹具精简已完成,生产修复未变。回归已并入既有滚动测试并共享搭架,3 个定向测试通过;只移除生产修复后,新回归因读到旧锚点而失败,保护未削弱。当前没有剩余问题,更新当前 head 的批准。
AI-assisted rereview by Codex; the earlier native scrolling acceptance remains applicable to the unchanged production fix.
Summary
Paging back through history stalls at the top edge: the transcript keeps
loading earlier ranges but the mounted range snaps back to the one the reader
just left, so
transcript-scroll-cost.spec.tstimes out waiting for the firstTurn to change (
Timeout 10000ms exceeded while waiting on the predicate,3 runs in a five-hour window on main).
Root cause:
useChatScrollreads the visible Turn as the load anchor but neverrefreshes the reading bookmark on that path. At the edge the scroller
cannot move, so no scroll event fires, and
releasePin()publishes nothingbecause the authority is already unpinned and away from the tail. The bookmark
therefore still names a Turn the previous range evicted, and the restore effect
answers the freshly published range with
loadAround(stale)— the earlier loadsucceeds, then gets overwritten.
The fix reports the reading anchor through the existing reporter in
requestHistory, right after the pin is released and before either directionpicks its anchor, so the bookmark tracks the range being paged into and the
restore effect has nothing to correct. Reusing the reporter keeps its session check, its deduplication and
its guard against in-flight navigation, instead of writing the bookmark a
second way.
This is the paging-back half of the same anchor-versus-navigation family as
#4883; that PR fixed return-to-latest and is unaffected here.
Verification
Rebased onto current
main(00f2c9e0e); per the review thread's P3, theregression case now lives in
packages/ui/src/__tests__/use-chat-scroll.test.tsxsharing a file-localenvironment installer with the other scroll tests, and the standalone
paging-reading-anchor.test.tsxis deleted.packages/ui: 399 tests pass;biome lintclean on both changed files. Themoved case still fails without the one-line source change:
apps/desktop/e2e/transcript-scroll-cost.spec.ts:239,--repeat-each=5 --workers=1on macOS, rerun on the rebased head:AI use
Select exactly one:
Tool(s) and scope: Claude Code (Opus 5) — traced the anchor/restore
interaction, wrote the source change and the regression test. Reviewed and run
locally by a human.
Checklist
Does this PR entail a change in behavior?