fix(web-shell): stabilize transcript history pagination - #8914
Merged
Conversation
Collaborator
Author
E2E verification report
|
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. The description documents concrete user-visible symptoms — viewport jumping into newly prepended history, briefly overlapping rows, fast wheel/trackpad reaching the top without paginating until a down-then-up retry, and attached sessions rendering empty when the newest persisted transcript page has no replayable events. No linked issue, but the before/after account plus the step-by-step reviewer test plan make the failure modes checkable. Direction: aligned — this is reliability of the Web Shell's own transcript surface. No CHANGELOG reference needed for an internal pagination bugfix. Size: the change spans two packages ( Approach: wide but coherent — every part maps to one of the stated instability modes (row-key scroll anchoring, measuring prepended virtual rows, rAF-based top checks for fast wheel/touch/keyboard, generation-scoped pagination state, bounded empty-page walk in the bridge). Two parts are logically separable from scroll stability — the collapse of older failed/answer-less turns and the bridge empty-page skip — but both carry their own tests and are described in the PR body, so keeping them together is acceptable; just noting it for revert granularity. Risk: no high-risk path matches; no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的问题，不是理论性加固。描述中记录了具体的用户可见症状——向上分页时视口跳进新加载的历史、短暂出现行重叠、快速滚轮/触控板到达顶部后不触发分页（需要先下再上）、以及最新持久化转录页没有可 replay 事件时已附加会话显示为空。虽然没有关联 issue，但 before/after 描述和逐步的 reviewer 测试计划让这些失败模式可以被验证。 方向：对齐——这是 Web Shell 自身转录界面的可靠性修复，内部 bugfix 无需 CHANGELOG 参照。 规模：改动跨两个包（ 方案：范围较宽但自洽——每一部分都对应描述中的一种不稳定模式（行键滚动锚定、测量新插入的虚拟行、快速滚轮/触摸/键盘的 rAF 顶部检查、按代隔离的分页状态、bridge 中有上限的空页遍历）。其中两部分与滚动稳定性在逻辑上可拆分——较早失败/无回答 turn 的收起、bridge 空页跳过——但两者都有独立测试且在 PR 正文中说明，合在一起可以接受；仅为回滚粒度做提示。 风险：未命中高风险路径，无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewBefore reading the diff, my baseline for this problem was: capture the first visible row's viewport-relative top before fetching, restore What I traced at
Two non-blocking notes:
No critical blockers found. Test evidence — the PR's own CI (unattended run; no PR code executed)CI on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle this: 中文说明代码审查：在读 diff 之前我先独立给出了方案——抓取首个可见行相对视口的 top、prepend 后按行键恢复 scrollTop（失败退化为 scrollHeight 差值）、恢复期间为新挂载的虚拟行预填尺寸缓存、用 rAF 调度滚轮/触摸/键盘的顶部检查而不是只依赖 scroll 事件、为所有异步分页接缝加代际守卫、bridge 的 cursor 遍历加环检测与页数上限。PR 的实现与这条路径一致，没有发现被忽略的更简方案。 逐点核对：代际守卫覆盖了所有异步接缝（取前锚点等待、settle 更新、catch/finally、rAF 恢复回调、转录切换丢弃路径），旧请求无法影响新转录的锚点，且有对应测试；新增的三个 rAF 句柄都在卸载时取消；各处都有明确上限（30 帧锚点等待、20 页空页预算、重复 cursor 立即回退），bridge 三种情况均有测试；空页跳过循环遇到 partial/replayError 页会刻意停下以保留这些标志，新增的 throw 落入既有 catch → 实时 replay 回退，bounded read 失败不会破坏健康的附加会话；follow 模式的重排（用户滚动意图优先于贴底跟随、cooldown 不再吞掉主动滚动）正是流式输出期间不与用户争抢的修复点，程序化滚动不设置意图，follow 行为本身不受影响。 两条非阻塞提示：1） 测试证据：无人值守 CI 运行，未执行任何 PR 代码。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean static trace with a test for every claimed behavior; the two reservations are a user-visible change not named in the description, and real-library behavior that CI/ Honest read: this is what a careful fix for this problem looks like. My independent proposal before reading the diff — row-key scroll anchoring with a scrollHeight fallback, size-cache seeding for newly mounted rows, rAF top checks for fast input, generation guards, a bounded cursor walk — is exactly what the PR does; I didn't find a materially simpler path it missed. The anchor machinery in Reservations, named so they don't get lost:
Approving because the code is correct as far as a static trace can establish, the author is a repo committer with a concrete reviewer test plan, and every remaining question has a designated way to be answered (CI below, 中文说明整体判断：4/5——静态审查干净，每个声明的行为都有对应测试；两个保留意见是描述中未提及的一处用户可见变化，以及尚待 CI/ 坦白说：这是这类问题应有的细致修法。我在读 diff 之前独立给出的方案——行键滚动锚定加 scrollHeight 回退、为新挂载行预填尺寸缓存、快速输入的 rAF 顶部检查、代际守卫、有上限的 cursor 遍历——与 PR 的做法完全一致，没有发现被忽略的更简路径。 保留意见，明确列出以免被忽略：1）加载指示器的抑制开关（ 批准的理由：就静态审查所能及的范围，代码是正确的；作者是仓库 committer，给出了具体的 reviewer 测试计划；每个遗留问题都有明确的解答途径（下方 CI、行为声明用 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
added
scope/web-shell
type/bug
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
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. |
qwen-code-ci-bot
previously approved these changes
Aug 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
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）为单个提交。 |
qwen-code-ci-bot
pushed a commit
that referenced
this pull request
Aug 11, 2026
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/react-virtual@3.14.2, which declares@tanstack/virtual-core@3.17.0. 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.
Collaborator
|
Released in v0.21.10. |
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
4 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
无