Repository navigation
fix(web-shell): stabilize transcript history pagination - #8914
Merged
Merged
Conversation
Collaborator
Author
E2E verification report
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
ytahdn
force-pushed
the
codex/fix-web-shell-scroll-jitter
branch
from
August 11, 2026 07:09
9a706c5 to
eb753c3
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
chiga0
reviewed
Aug 11, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #8914 fix(web-shell): stabilize transcript history pagination
Type: Bug fix (scroll/pagination stability + persisted-transcript robustness)
Change size: +889/-78 across 6 files — production logic in MessageList.tsx (+332/-64) and bridge.ts (+35/-5); the rest are tests and one CSS module.
Reviewed at HEAD: eb753c37 (single squashed commit, MERGEABLE, CI green: ubuntu tests, web-shell visuals, E2E smoke, Desktop Shell).
Findings Summary
- Critical/Major: 0 items
- Minor: 1 item
- Nit: 3 items
Key Observations
This is a careful, well-bounded fix. I independently traced the design before reading the existing bot triage: row-key scroll anchoring with a scrollHeight fallback, synchronous size-cache seeding (measureVirtualRow → resizeItem) for newly mounted rows during restore, rAF-scheduled top-proximity checks for wheel/touchmove/keydown, generation guards on every async pagination seam, a 30-frame anchor wait budget, and a cycle-detected, page-count-bounded cursor walk in the bridge. Every async seam (olderHistoryLoadGeneration) is checked or bumped in the pre-fetch wait, settle update, catch/finally, the rAF restore callbacks, and the transcript-change discard path; all three new rAF handles are cancelled on unmount. The restore math is self-correcting (it measures the anchor row's actual viewport position at settle time rather than adding a blind delta), so virtualizer-side compensation and the manual adjustment converge instead of double-counting. The bridge's new throws land in the pre-existing catch { break; } → live-replay fallback, and partial/replayError pages are deliberately never skipped, so bounded-read failure semantics are preserved. Tests map 1:1 to the claimed behaviors (skip-to-visible-page, stuck cursor, unique-cursor bound, anchor discard on transcript change, bounded anchor wait, fast-top pagination, collapse rules).
Nit Details (overview-only, no inline comments)
- N1 — listener-effect churn during streaming (
MessageList.tsx:3851-3859):loadOlderHistory's dep array now includesgetItemKeyandmergedMessages.length, both of which change identity on every streaming render. That re-runs the scroll-listener effect and the wheel/touchstart/touchmove/pointerdown/keydown effect on every message update (cheap, but previously stable during streaming). Narrower consequence: if a render commits betweenscheduleOlderHistoryTopCheck()and its rAF firing, the old effect's cleanup cancelsolderHistoryTopCheckFrameand the top check is dropped. Self-healing in practice — thescrollhandler is the primary trigger and fires wheneverscrollTopactually moves — so this is a liveness nit, not a correctness bug. If it's ever revisited, readingmergedMessages.length/getItemKeythrough refs would keep the callback stable. - N2 — 31 checks against a 30-frame budget (
MessageList.tsx:3769):remainingFrames-- <= 0withOLDER_HISTORY_ANCHOR_WAIT_FRAMES = 30evaluates the post-decrement, so the wait loop actually performs 31 frame checks. Harmless; naming and behavior are off by one. - N3 — attach latency ceiling in the empty-page walk (
bridge.ts:1422): the skip loop performs up toMAX_EMPTY_TRANSCRIPT_PAGES(20) sequentialsessionTranscriptround trips before falling back to the live replay; each call carries its own 60 s timeout. Healthy servers answer empty pages in milliseconds so this is theoretical, but a slow, misbehaving persistence layer could stretch an attached-session refresh noticeably. A smaller budget or a total deadline across the walk would bound the pathological case; fine to defer.
Cross-Validation
The only prior review is the automated qwen-triage (stages 2–3, at the pre-force-push SHA 9a706c58; the current HEAD is a rebase with no substantive change to the PR's own hunks — I diffed both SHAs file-by-file).
| Finding | Other Reviewer | My Assessment |
|---|---|---|
C1: setSuppressOlderHistoryLoadingStatus(!allowRetry) → (!force) hides the loading indicator during passive pagination; user-visible, not in description |
qwen-code-ci-bot (stage 2/3 note 1) | Confirmed — still present at MessageList.tsx:3746 in HEAD eb753c3 and still absent from the PR body; posted as inline Minor |
C2: anchorTo/directDomUpdates/containerRef options could not be statically verified against the locked virtualizer version |
qwen-code-ci-bot (stage 2/3 note 2, reservation) | Resolved — see Additional Audit Coverage; I verified against the published typings |
| C3: Generation guards close every async seam; rAF handles cancelled on unmount; bounds everywhere | qwen-code-ci-bot | Confirmed — re-traced each path at HEAD |
Unique-1: loadOlderHistory dep churn re-attaches scroll/wheel/touch/keydown listeners on every streaming render; a pending olderHistoryTopCheckFrame can be cancelled in effect cleanup |
— | New finding, Nit, overview-only |
| Unique-2: Anchor-wait budget is 31 frame checks, not 30 | — | New finding, Nit, overview-only |
| Unique-3: Bridge empty-page walk worst case is 20 sequential transcript round trips before the live-replay fallback | — | New finding, Nit, overview-only |
Additional Audit Coverage
Areas I independently checked beyond the existing findings:
- Virtualizer API compatibility (resolves C2): the lockfile pins
@tanstack/[email protected], which declares@tanstack/[email protected]. I fetched the published typings for both:directDomUpdates,directDomUpdatesMode, andcontainerRef(onReactVirtualizer) exist in react-virtual 3.14.2;anchorTo,resizeItem,itemSizeCache,shouldAdjustScrollPositionOnItemSizeChange, anduseAnimationFrameWithResizeObserverexist in virtual-core 3.17.0. The JSX satisfies the documenteddirectDomUpdatescontract: rows areposition: absoluteanchored attop: 0/left: 0with no main-axis inline style (the old React-driventranslateYwas removed), and the sizer receivesvirtualizer.containerRefwithout an inlineheight(the oldheight: totalVirtualSizewas removed;totalVirtualSizeis still consumed by the auto-scroll driver and underfill check, so no dead value). - Scroll-adjustment predicate vs library default: virtual-core 3.17.0's built-in default deliberately skips offset compensation while
scrollDirection === 'backward'(the documented source of "items jump while scrolling up" jank). The PR'sshouldAdjustVirtualScrollPosition(item.end, scrollOffset)re-enables compensation, but only for rows fully above the viewport — the correct escape-hatch usage, and it's what makes collapsed-turn shrinkage and prepended pages stable during upward scrolling. - Follow-mode interplay: traced
scrollCooldownrelease againstpendingBottomFollowAfterCooldownandfollowPausedByUserRef— a user who pauses follow during cooldown is not force-re-followed on release; the Rule 2/Rule 3 reorder makes user intent win over near-bottom re-follow, which is the streaming fix, and programmatic scrolls set no intent so they are unaffected. - Bridge arithmetic: verified the test expectations against the loop — repeated cursor aborts after exactly 2 ext calls, the unique-cursor case aborts at exactly 20 (
++emptyPageCount >= MAX_EMPTY_TRANSCRIPT_PAGES), and the skip case issuesdirection: 'backward'first thencursorwithoutdirection. - CSS margin→padding: virtual rows are measured by element height, which excludes margins — switching
turnStatusRow/turnAnswerRowspacing to padding keeps it inside the measured size; visually equivalent since the elements have no background. Confirmed by the green visuals job. - Turn-collapse behavior change (
(hasTurnError || answerIdx < 0) && isLastTurn): disclosed in the PR description and covered by two new unit tests; user toggles still win viacollapseOverrides, and error rows remain visible in collapsed turns since they are not hideable steps. - Anchor settle race: if
onLoadOlderHistoryresolves before the parent commits the prepended messages, the "unchanged" branch schedules a no-op clear, but the changed branch cancels any pending frame before rescheduling the adjustment, so a later commit still restores position correctly.
Final Verdict
Recommend merge once the loading-indicator suppression change (Minor, inline) is acknowledged in the PR description. No correctness, safety, or design blockers found; the remaining items are Nits.
This review was generated by QoderWork AI
| olderHistoryLoadInFlight.current = true; | ||
| setSuppressOlderHistoryLoadingStatus(!allowRetry); | ||
| const generation = ++olderHistoryLoadGeneration.current; | ||
| setSuppressOlderHistoryLoadingStatus(!force); |
Collaborator
There was a problem hiding this comment.
[Minor] Loading-indicator suppression semantics changed, but not mentioned in the PR description
This used to be setSuppressOlderHistoryLoadingStatus(!allowRetry); it is now (!force). As a result, passive scroll/wheel/keyboard-triggered pagination (loadOlderHistory(true), where force is false) no longer shows the "Loading earlier history…" status row — only the explicit retry button (force: true) does. This is a user-visible behavior change: on a slow backend the user gets no feedback that history is loading during automatic pagination.
The change is plausibly intentional — the status row is itself prepended content and thus a layout-shift source this PR is trying to eliminate — and I'm raising it as documentation rather than a revert request. Please add one sentence to the PR body stating the intent (this was also noted by the triage bot at the previous SHA and is still unaddressed at HEAD eb753c3).
This review was generated by QoderWork AI
Collaborator
Author
There was a problem hiding this comment.
Confirmed intentional. Passive pagination suppresses the top loading-status row because prepending that row changes the measured layout and can destabilize the viewport anchor this PR is preserving. Explicit retry remains user-initiated and keeps the visible loading status. I am keeping the implementation as-is; this review reply records that the behavior is deliberate rather than incidental.
Collaborator
Author
|
Follow-up on the overview-only nits from the latest review:
No correctness or merge-blocking code change is needed for these notes. |
yiliang114
approved these changes
Aug 11, 2026
yiliang114
approved these changes
Aug 11, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. The anchor rework checks out under trace: stable msg: content keys with viewport-relative rowTop deltas are idempotent with the virtualizer's own size-change adjustment, in-flight dedup plus the pendingOlderHistoryTopLoad retry effect gives reliable chained loads for fast wheel/touch/keyboard, generation guards drop stale anchors on transcript replacement, and the virtual-anchor wait releases without stuck state. The bridge skip loop terminates provably (cursor-advance check + cycle detection + 20-page cap) with the three fallback triggers each pinned by a bridge test, and the loader-resolves-before-state-commits and non-virtual blind-anchor cases are latent-contract notes only — current loaders batch, E2E passed. CI green on head including the E2E smoke; chiga0's minor (loading-status suppression) is intentional and the rationale holds (the status row would change measured layout and destabilize the anchor).
One P2 for follow-up, not a gate: refreshedReplayFieldsFor has no wall-clock budget across the skip loop — up to 20 sequential 60s per-page timeouts (twice if the entry mutates mid-read) means a degraded-but-responsive transcript backend can stretch an attach-refresh to ~20-40 min before the live-replay fallback; enforce one overall monotonic deadline across the walk. P3s: measureVirtualRow's ref-callback identity churn re-measures every mounted row per streaming render (capture getItemKey via a ref to keep the callback stable); the anchor-restore unchanged branch assumes the loader's state update commits with its promise resolution (pin with a loader that resolves one task early); non-virtual anchor without rowKey is blind to transcript switches (contrived). Ship it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This change stabilizes long Web Shell transcripts while users scroll upward through virtualized history. It preserves the visible message across prepended pages, measures newly mounted virtual rows before display, avoids fighting intentional upward scrolling while a response is streaming, and makes fast wheel, touch, and keyboard navigation reliably trigger pagination near the top.
It also keeps pagination state scoped to the active transcript, bounds virtual-anchor waiting, preserves consistent turn-output spacing after virtualization, and collapses incomplete or failed older turns once a newer turn exists while leaving the latest incomplete turn open for inspection.
Attached session refreshes now skip empty persisted transcript pages until a visible page is found. Repeated, missing, or excessively long empty-page cursor chains fall back to the live replay instead of returning an empty session or looping indefinitely.
Why it's needed
Long sessions could visibly jump into newly loaded history, briefly render overlapping rows, or lose the reader's position during the first and second upward pagination. Fast scrolling could also reach the top without starting another page until the user moved down and up again. Separately, an attached session could appear empty when its newest persisted page contained no replayable events even though older pages contained the conversation.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: upward pagination could move the viewport into the prepended page, briefly overlap old and current rows, miss fast top crossings, and return an empty attached session when the newest persisted page contained no visible replay events.
After: real-browser validation on macOS kept the viewport at the same visible content across repeated pagination without the recorded overlap flash. Automated coverage verifies anchor preservation, virtual-row measurement, fast top detection, stale-request isolation, bounded anchor waiting, turn-collapse behavior, and persisted empty-page recovery.
Tested on
Environment (optional)
Local Web Shell and daemon session on macOS, plus package-level Vitest and TypeScript checks.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 的修改
本次修改提升了 Web Shell 长会话在向上浏览虚拟化历史记录时的稳定性。历史分页向前插入内容后会保持当前可见消息的位置，在展示前测量新挂载的虚拟行，模型流式输出期间不会与用户主动向上滚动争抢位置，并且快速滚轮、触摸和键盘操作到达顶部附近时都能可靠触发分页。
分页状态现在只作用于当前转录，虚拟锚点等待有明确上限，虚拟化后的 turn 输出间距保持一致；较早的异常或缺少最终回答的 turn 在出现新 turn 后会自动收起，而最新的不完整 turn 仍保持展开，便于用户检查。
刷新已附加会话时，会跳过没有可展示事件的持久化转录页，直到找到有内容的页面。对于重复、缺失或连续过长的空页 cursor 链，会回退到实时 replay，避免返回空会话或无限循环。
修改原因
长会话向上分页时可能直接跳进新加载的历史内容、短暂出现行重叠，或在第一次和第二次分页时丢失阅读位置。快速滚动到顶部后，有时还必须先向下再向上才能继续分页。另外，如果最新的持久化页没有可 replay 的事件，即使更早页面存在对话，已附加会话也可能显示为空。
Reviewer 测试计划
验证方式
前后对比证据
修改前：向上分页可能把视口移动到新插入的历史页，短暂重叠旧内容与当前内容，快速到顶可能漏掉分页；最新持久化页没有可见 replay 事件时，已附加会话可能显示为空。
修改后：在 macOS 真机浏览器验证中，连续分页时视口保持在相同可见内容，录屏中出现的重叠闪动不再出现。自动化测试覆盖锚点保持、虚拟行测量、快速到顶检测、旧请求隔离、锚点等待边界、turn 收起规则和持久化空页恢复。
测试平台
环境（可选）
macOS 本地 Web Shell 与 daemon 会话，以及包级 Vitest 和 TypeScript 检查。
风险与范围
关联 Issue
无