Skip to content

fix(ui): bookmark the anchor a wheel pages from - #4911

Merged
Astro-Han merged 2 commits into
apache:mainfrom
orangeCatDeveloper:fix/ui-paging-reading-anchor
Sep 7, 2026
Merged

fix(ui): bookmark the anchor a wheel pages from#4911
Astro-Han merged 2 commits into
apache:mainfrom
orangeCatDeveloper:fix/ui-paging-reading-anchor

Conversation

@orangeCatDeveloper

@orangeCatDeveloper orangeCatDeveloper commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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.ts times out waiting for the first
Turn to change (Timeout 10000ms exceeded while waiting on the predicate,
3 runs in a five-hour window on main).

Root cause: useChatScroll reads the visible Turn as the load anchor but never
refreshes the reading bookmark on that path. At the edge the scroller
cannot move, so no scroll event fires, and releasePin() publishes nothing
because 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 load
succeeds, then gets overwritten.

The fix reports the reading anchor through the existing reporter in
requestHistory, right after the pin is released and before either direction
picks 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, the
regression case now lives in
packages/ui/src/__tests__/use-chat-scroll.test.tsx sharing a file-local
environment installer with the other scroll tests, and the standalone
paging-reading-anchor.test.tsx is deleted.

packages/ui: 399 tests pass; biome lint clean on both changed files. The
moved case still fails without the one-line source change:

$ node --test packages/ui/dist/__tests__/use-chat-scroll.test.js   # without the fix
✖ a wheel at the top edge reports its anchor before it loads earlier history
    AssertionError [ERR_ASSERTION]: the wheel bookmarks the Turn it anchors the load to
ℹ pass 2
ℹ fail 1

$ node --test "packages/ui/dist/__tests__/*.test.js"               # with the fix
ℹ tests 399
ℹ pass 399
ℹ fail 0

apps/desktop/e2e/transcript-scroll-cost.spec.ts:239, --repeat-each=5 --workers=1 on macOS, rerun on the rebased head:

  ✓  1 e2e/transcript-scroll-cost.spec.ts:239:1 › paging back through the whole history keeps the mounted range bounded (10.4s)
  ✓  2 e2e/transcript-scroll-cost.spec.ts:239:1 › paging back through the whole history keeps the mounted range bounded (9.1s)
  ✓  3 e2e/transcript-scroll-cost.spec.ts:239:1 › paging back through the whole history keeps the mounted range bounded (11.1s)
  ✓  4 e2e/transcript-scroll-cost.spec.ts:239:1 › paging back through the whole history keeps the mounted range bounded (7.7s)
  ✓  5 e2e/transcript-scroll-cost.spec.ts:239:1 › paging back through the whole history keeps the mounted range bounded (28.3s)

  5 passed (1.1m)

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 6, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ui-paging-reading-anchor branch 3 times, most recently from 684807a to 00b8b00 Compare September 6, 2026 10:25

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 授权批准并附此非阻塞建议。

Comment thread packages/ui/src/__tests__/paging-reading-anchor.test.tsx Outdated
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)
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ui-paging-reading-anchor branch from 00b8b00 to f501a2f Compare September 7, 2026 17:44

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han
Astro-Han merged commit daa6d3e into apache:main Sep 7, 2026
1 check passed
@orangeCatDeveloper
orangeCatDeveloper deleted the fix/ui-paging-reading-anchor branch September 7, 2026 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants