Skip to content

fix(ui): preserve transcript intent and layout through submission - #5057

Merged
Astro-Han merged 11 commits into
mainfrom
fix/transcript-submit-layout
Sep 9, 2026
Merged

fix(ui): preserve transcript intent and layout through submission#5057
Astro-Han merged 11 commits into
mainfrom
fix/transcript-submit-layout

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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

  • Keep one transcript scroll authority and tail writer. Remove the unused Astryx/host mode selector and obsolete vendor unread-state patches; all Maka chat surfaces use host ownership. Replace geometry-based intent classification with wheel, key, scrollbar and touch provenance, respecting nested scrolling and prevented defaults. Consolidate history-loader wheel handling into that authority.
  • Patch Astryx's controlled-value synchronization from useEffect to useLayoutEffect, retaining its equality and caret guards. No additional synchronization state or DOM writes are introduced.
  • Put transient user messages inside their owning Turn. Reuse metadata for delivery status/actions and TurnFooter for running status/provider retry/completed actions, including the two pre-Turn fallback paths. Remove the separate status row and mismatched 32px placeholder.
  • Keep only the streaming answer eagerly laid out. Historical Turns and completed blocks retain lazy layout.

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 commit 103313954 passes the architecture checker and its 103 tests. Browser and Electron acceptance below were performed before this cleanup.

  • 422 UI tests and 2508 Desktop tests pass. Story typecheck, production renderer/Storybook builds, patch-package verification, scoped Biome and diff checks pass.
  • Full headless Storybook smoke passes: 336 stories, 363 theme renders at 1280×900, using the normal smoke runner and observer.
  • Final macOS Electron 43.4.1/CDP acceptance passes through real Host/SQLite with FakeBackend: short/eight/80-line submission (0px reversal), completion, native-wheel reader preservation/return to tail, and pending history loading followed by submit/session switching. The window remained unfocused. This verifies layout and state flow, not real-provider latency or pixel presentation timing.
  • Negative controls: the original authority fails the geometry-without-input case; replacing only editor synchronization with its old passive effect reproduces the 168px reversal; the old completion layout fails the new completion story by 69px. The fallback test also fails before the shared-footer correction.
  • Ablation: removing the streaming-answer eager rule still causes 74px reversals with the other fixes present. The outer-Turn eager exception was removed because it is unnecessary.
  • Independent adversarial review found Meta-wheel zoom intent handling and a missed fallback footer migration; both were fixed and verified. No unresolved P0/P1 findings remain.
  • Two ordering tests now protect user-before-answer and next-message-after-answer behavior rather than requiring a transient outside the Turn. The existing sticky-dock geometry tolerance is calibrated from 0.25px to Chromium's 0.5 CSS-pixel scroll-limit rounding: an isolated native two-div sticky layout reproduces the exact 0.4375px movement at DPR 1 and 2, without React or Maka. No production scroll compensation was added.

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 48d0503e9 revision with mixed small changes against existing baseline run 34253026432 (a21c104ad3d5af7e140c875e4048f2d9f5d24493). Performance run 34303112564 passed on ef74216b5f5b0eaa833c251333b3afb906101e5e, 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.

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 8, 2026
@Astro-Han Astro-Han changed the title fix(ui): preserve transcript reading intent during layout changes fix(ui): preserve transcript intent and layout through submission Sep 9, 2026

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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: OpposingResizesKeepFollowingTheTail carries over unchanged, and ContentThatOnlyRoundsIsNotTheReader is superseded by LayoutDoesNotCreateReaderIntent, 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 overflowAnchor assertion 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。两项托管检查(testwindows_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:81reachesTranscript 中重现,并新增了旧版没有的 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 重写会拆掉文本节点 —— 这在两种时机下完全相同。

你会关心的合并后语义两条都在:统一的 TurnFooterpendingRunningStartedAt 等待计时,在 chat-view.tsx 的两个实时状态点上同时出现。

两条经核查未能成立的怀疑,记录以免重开:① 我预期删除 transcript-scroll-rounding.stories.tsx 会损失亚像素覆盖 —— 并未:OpposingResizesKeepFollowingTheTail 原样保留,ContentThatOnlyRoundsIsNotTheReaderLayoutDoesNotCreateReaderIntent 取代,后者仍在真实布局引擎下使用分数高度(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-authorityuse-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.

Comment thread patches/@astryxdesign+core+0.5.2.patch Outdated
@Astro-Han
Astro-Han marked this pull request as ready for review September 9, 2026 11:26
@Astro-Han
Astro-Han merged commit 3725f2b into main Sep 9, 2026
2 checks passed
@Astro-Han
Astro-Han deleted the fix/transcript-submit-layout branch September 9, 2026 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants