fix(ui): preserve transcript intent and layout through submission - #5057
Conversation
jackwener
left a comment
There was a problem hiding this comment.
APPROVE, bound to b2af0187c2477dc4f03808558384124615626929. One P3 inline; it does not block. Both hosted checks (test, windows_recovery) are terminal green on this exact head — test was still running while I reviewed, and I did not use it as evidence for anything below.
The core invariant holds, and I checked that the tests can actually falsify it rather than just pass. The claim is that geometry only counts as reader intent when it belongs to a known input operation: onScroll changes pin state only inside if (gesture), and gesture is established solely by wheel, key, pointer or touch. Relaxing that guard to always-true fails 7 of 16 tests, including identical shrink/grow geometry follows only when no reader input intervened, content that grows under a pinned transcript keeps the tail on screen and only the reader's own movement reaches a reader-scroll listener. A green suite alone would not have shown that.
Nested scrolling is stricter than what it replaces, not a relocation of it. The composedPath() walk deleted from use-chat-scroll.ts reappears as reachesTranscript in transcript-scroll-authority.tsx:81, and it adds an overscroll-behavior-y: contain | none check the old version did not have. The old extra term scrollHeight > clientHeight is implied by remaining > 0 in both directions — upward, a non-zero scrollTop already requires a scrollable box; downward, an unscrollable box cannot have positive remaining — so dropping it is not a regression. Forcing reachesTranscript to return true fails pages only toward reader input, including wheels at a bounded edge.
The fallback condition change is a fix, and I verified the premise it depends on. !tailTurnId → !hasRenderedLiveTurn means the running-status fallback now renders exactly when no TurnView is rendering the tail Turn, instead of suppressing itself whenever a Turn id merely exists. That is only precise if turns.some(...) matches what is actually rendered, which matters here because the transcript is paged: projectTranscriptRows maps every turn to a row and only inserts gap rows, never dropping one, so the two sets agree. No double render, and no silent suppression while a send arm has named a Turn the transcript does not yet contain.
IME risk is unchanged by this PR. The vendor patch moves the controlled-value sync from a passive to a layout effect, which changes when the DOM is written, not whether. Its dependency array is [controlledValue], so an in-progress composition with unchanged host state never triggers it, and the component keeps its own guard that Enter never submits mid-composition. The textContent rewrite tears down the text node in both timings equally.
Post-merge semantics you would want preserved are both present: the unified TurnFooter and the pendingRunningStartedAt wait clock appear together at both live-status sites in chat-view.tsx.
Two suspicions of mine that did not survive checking, recorded so they are not re-opened:
- I expected the deleted
transcript-scroll-rounding.stories.tsx(-233) to cost sub-pixel coverage. It does not:OpposingResizesKeepFollowingTheTailcarries over unchanged, andContentThatOnlyRoundsIsNotTheReaderis superseded byLayoutDoesNotCreateReaderIntent, which still uses fractional heights (300.5 / 400.5 / 120.5 / 900.5 px, mutating to 800.2 px) in a real layout engine. The claim was generalised, not narrowed. - I expected the perf-harness change to be an undisclosed comparability problem. The PR body already states it — the driver now supplies wheel intent first, anchor metrics must be read with that correction, and cross-VM timings are not a controlled speedup claim. The only detail not mentioned is that the new
overflowAnchorassertion lets a functional regression fail the performance lane; that is minor and arguably useful.
On complexity: the authority grew because it now owns provenance for four input sources, which is what replacing geometry-based classification requires, and use-chat-scroll.ts shrank correspondingly. The double requestAnimationFrame in onScrollEnd is unusual but carries a concrete Chromium rationale and introduces no timer or time-based ignore window.
Scope and limits. Static review of all 17 files; a local packages/ui build used to run and mutate the transcript-scroll-authority and use-chat-scroll suites; three separate application tests for the vendor patch. I did not run the Desktop or CLI suites, Storybook, or e2e, and did not exercise a real browser or Electron window — so the IME, native-wheel and rendering conclusions above come from reading the code and the vendor sources plus unit-level mutation, not from live verification. I did not re-derive any performance number, and I treated the two runs as not comparable in either direction rather than as weak evidence. This is a single review line.
简体中文
绑定 b2af0187c2477dc4f03808558384124615626929,行内一条不阻塞的 P3。两项托管检查(test、windows_recovery)在此 exact head 终态为绿 —— test 在我审查期间尚在运行,以下任何结论都未以它为证据。
核心不变式成立,而且我核了测试是否真能证伪它,而不只是通过。 主张是「几何变化只有归属于已知输入操作才算读者意图」:onScroll 仅在 if (gesture) 内改变固定状态,而 gesture 只由 wheel/key/pointer/touch 建立。把该守卫放宽为恒真后,16 项中 7 项失败,含 identical shrink/grow geometry follows only when no reader input intervened 等。只跑一遍绿测试无法说明这一点。
嵌套滚动是变严,不是搬运。 从 use-chat-scroll.ts 删除的 composedPath() 判断,在 transcript-scroll-authority.tsx:81 的 reachesTranscript 中重现,并新增了旧版没有的 overscroll-behavior-y: contain | none 判断。旧版多出的 scrollHeight > clientHeight 在两个方向上都被 remaining > 0 蕴含,故删除不构成回归。把 reachesTranscript 强制为真,会让 pages only toward reader input, including wheels at a bounded edge 失败。
兜底条件的改动是修正,且我验证了它所依赖的前提。 !tailTurnId → !hasRenderedLiveTurn 使运行反馈当且仅当没有 TurnView 在渲染尾部 Turn 时才兜底。这只有在 turns.some(...) 与实际渲染集合一致时才精确 —— 由于转录是分页的,这一点必须验证:projectTranscriptRows 对每个 turn 都产出一行、只插入 gap、从不丢弃,故两个集合一致。既不会双渲染,也不会在"发送臂已命名 Turn 但转录尚未包含它"时被静默抑制。
IME 风险未被本 PR 改变。 该 vendor 补丁把受控值同步从被动 effect 移到布局 effect,改变的是何时写 DOM,而非是否写。其依赖为 [controlledValue],宿主状态不变的组词过程根本不会触发它;组件自身也保留了「Enter 在组词期间永不提交」的守卫。textContent 重写会拆掉文本节点 —— 这在两种时机下完全相同。
你会关心的合并后语义两条都在:统一的 TurnFooter 与 pendingRunningStartedAt 等待计时,在 chat-view.tsx 的两个实时状态点上同时出现。
两条经核查未能成立的怀疑,记录以免重开:① 我预期删除 transcript-scroll-rounding.stories.tsx 会损失亚像素覆盖 —— 并未:OpposingResizesKeepFollowingTheTail 原样保留,ContentThatOnlyRoundsIsNotTheReader 由 LayoutDoesNotCreateReaderIntent 取代,后者仍在真实布局引擎下使用分数高度(300.5 / 400.5 / 120.5 / 900.5px,变更至 800.2px),主张是被泛化而非收窄。② 我预期性能脚手架的变更是未披露的可比性问题 —— 正文已写明(driver 现先给出 wheel intent、锚点指标须结合该修正解读、跨 VM 计时不是受控加速主张)。唯一未提及的是新增的 overflowAnchor 断言会让功能回归导致性能通道失败;影响很小,甚至可算有益。
关于复杂度:authority 变长是因为它现在独占四类输入来源的溯源,这正是替换掉致错的几何分类法所必需的,而 use-chat-scroll.ts 相应缩减。onScrollEnd 中的双 requestAnimationFrame 写法不常见,但给出了具体的 Chromium 理由,且不引入定时器或基于时间的忽略窗口。
本次审查的边界:17 个文件的静态审查;本地构建 packages/ui 以实际运行并变异 transcript-scroll-authority 与 use-chat-scroll 两个套件;对 vendor 补丁做了三种独立的应用测试。未运行 Desktop 或 CLI 套件、未跑 Storybook 或 e2e、未在真实浏览器或 Electron 窗口中验证 —— 因此上述 IME、原生滚轮与渲染结论来自代码与库源码的静态推理加单元级变异,而非实机验证。未复算任何性能数值,并把两次运行视为双向都不可比,而不是弱证据。这是单独一条评审线。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Problem and behavior
Submitting a prompt could reverse the new Turn and cancel transcript following. Completion could then move the same Turn downward again. Three independent causes crossed ownership boundaries: geometry was mistaken for reader input, controlled editor DOM was cleared after paint, and transient/running UI occupied different layout rows from settled content.
Reading intent now comes from input or explicit navigation. The editor synchronizes controlled content before paint. Transient and durable user messages share the Turn's spacing, while delivery status uses the existing metadata row and running activity uses the same footer as completed actions.
Implementation
useEffecttouseLayoutEffect, retaining its equality and caret guards. No additional synchronization state or DOM writes are introduced.TurnFooterfor running status/provider retry/completed actions, including the two pre-Turn fallback paths. Remove the separate status row and mismatched 32px placeholder.Validation
Latest cleanup (
ad602f8a5): 424 UI tests, 32 targeted Desktop tests, full Desktop/story typechecks, and clean-package patch application pass. The subsequent ledger-only commit103313954passes the architecture checker and its 103 tests. Browser and Electron acceptance below were performed before this cleanup.Performance and scope
The controlled editor update moves existing work before paint; the footer change removes temporary layout rows. Only the streaming answer loses lazy layout, so its first render bears its actual layout cost. No uniform speedup is claimed.
Performance run 34295613161 passed on the earlier
48d0503e9revision with mixed small changes against existing baseline run 34253026432 (a21c104ad3d5af7e140c875e4048f2d9f5d24493). Performance run 34303112564 passed onef74216b5f5b0eaa833c251333b3afb906101e5e, before the later merge and cleanup commits. The baseline used AMD EPYC 7763 and this run used Intel Xeon 6973P-C; within-run confidence intervals do not isolate code effects from hardware differences. The baseline is reused, not rerun. The Storybook performance driver now supplies wheel intent before relative scrolling; distances, waits and sample counts remain unchanged. Anchor metrics must be interpreted with that driver correction in mind, and cross-VM timings are not a controlled speedup claim.Composer admission/clear ownership and durable session semantics are unchanged.