Skip to content

fix(ui): keep transcript geometry stable during reader input - #5192

Open
Astro-Han wants to merge 14 commits into
mainfrom
test/5184-geometry-ablation
Open

fix(ui): keep transcript geometry stable during reader input#5192
Astro-Han wants to merge 14 commits into
mainfrom
test/5184-geometry-ablation

Conversation

@Astro-Han

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

Copy link
Copy Markdown
Contributor

Summary

Scrolling through resident transcript content can change the native scroll range when Chromium replaces lazy height estimates with real layout. Use real layout for Turns, timeline blocks and code chunks, retaining containment and the bounded transcript window from #5170.

Make the session-side publication bridge own the latest pending update. Both Main and WorkHub publish through the viewport's scroll authority: input holds publication; admission synchronously commits the DOM and preserves the reading anchor. Unmounting a viewport releases its geometry constraint without losing an accepted source update. Existing session/store/disposal guards reject obsolete data.

Publish messages, gap metadata and the dependent live/transient handoff together. WorkHub retains one-shot renderer completion acknowledgements until the matching assistant is published, so a held publication cannot lose the handoff. Host admission and stop facts continue immediately. Fill may fetch during input; trim and automatic fill chaining resume against the published geometry. Refresh no longer writes messages directly, and lineage uses the existing navigation command. Short-window upward input distinguishes available older history from a conversation with nothing older to read.

Remove obsolete boundary size categories and CSS ablation modes that had become equivalent to production. Performance comparisons now require reports from different code revisions; the navigation harness verifies the current #5170 UI instead of a removed gap notice.

Refs #5184.

Verification

Latest validation for f497f7975:

  • 2,494 Desktop tests passed, including the corrected WorkHub test with exactly one settlement notification. Removing the retention fix reproduces the handoff failure; the earlier test incorrectly sent a second notification.
  • Consecutive native-wheel paging passed twice in Electron. Active input asserts height, membership and upward monotonicity; settled boundaries assert reading-anchor preservation. Existing 1px tolerances remain.
  • All three fixed-range scenes passed the trimmed --assert-stable driver: zero height drift and zero reverse motion. Desktop typechecks, lint, format, renderer architecture, E2E budget and license headers passed.
  • Removed disconnected and repeated assertions, unasserted anchor/LoAF/heap diagnostics and warm return sweeps. Historical measurements remain linked through Git. Scroll timing now covers only the cold upward sweep and must not be compared directly with the earlier three-sweep workload.

Earlier validation for f9cd77cbe (unchanged UI library and layout code):

  • 432 UI tests and 2,494 Desktop tests passed; Desktop typechecks, UI/main/renderer/Storybook builds, lint, format, renderer architecture, E2E budget and license-header checks passed.
  • Five Main Electron/Host geometry and paging E2Es passed, plus two WorkHub window/rail E2Es. The geometry tolerances remain unchanged. WorkHub held-publication behavior is covered through its real controller hook; a dedicated real WorkHub held-thumb E2E has not been run.
  • A React hook regression verifies that a held publication survives viewport unmount/remount without another source update. Restoring pending-update loss makes it fail. The WorkHub regression checks transcript, transient prompt and live answer handoff; restoring the previous WorkHub publisher makes it fail.
  • The production navigation performance harness completed all three mount/older/latest trials locally. All three fixed-range scenes passed --assert-stable with one repetition each, recording zero height drift and zero reverse scroll.
  • Earlier refresh, durable-object identity, short-window intent, stale trim and asynchronous DOM admission regressions were observed failing before their fixes. A complete fail-without-the-fix check for every original geometry regression remains unconfirmed.

Performance evidence from earlier revisions: before, after. These limited samples do not establish statistical equivalence. The previous current-head run failed on the removed gap selector; it is not evidence of a performance regression. The production performance run at f9cd77cbe passed. Performance for the latest commit remains to be assessed. Acceptance is no meaningful regression, not a speedup.

Cold-start Markdown limitation

A separate plaintext-fallback-to-Markdown transition produced a 14.5px anchor shift in a cold-start probe. This PR does not fix that transition. The history-insertion test waits for initial Markdown and fonts before recording its baseline, then retains its 1px tolerance; fixed-content geometry assertions do not cover initial module loading. Inner boundary containment is retained pending a separate performance ablation.

AI use

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

Tool(s) and scope: Codex contributed architecture analysis, implementation, tests and this description. Kimi Coding K3 and Claude Fable High provided design consultations. Independent deep reviewers examined architecture, implementation and simplification; the local adversarial review found the WorkHub settlement race corrected above. Raft review conclusions remain bound to the commits each reviewer examined.

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/XL Under 2500 readable lines label Sep 11, 2026
@Astro-Han
Astro-Han force-pushed the test/5184-geometry-ablation branch from 99d24b6 to 6a8e418 Compare September 11, 2026 15:16

@me2seeks me2seeks 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 at exact head 6a8e418. No P0/P1/P2 code findings; approval is based on the verified transcript geometry, publication identity, and navigation boundaries.

@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.

Posting one confirmed finding now rather than holding it for a full synthesis, at the author's request. This is not an approval and not a complete review — a multi-reviewer pass on 61d1b7fcbbf834ad90f1f7962f12e9c8a25c1b2e is still in progress, and I will report the rest when the other lanes return.

The finding is inline. It was introduced at 6a8e418fec94dbc3841be659ea8b093ea6126941, the author reproduced it independently, and the fix at 61d1b7fcb is in this head. I verified the defect on the old head, the fix on the new head, and the new regression test by ablation — each with its own control, so the inline comment is a record of a closed issue rather than an open one.

Two things I checked that are not findings, recorded so they are not re-derived:

  • The cold-start Markdown transition is disclosed accurately. The description states that the plain-text-to-Markdown swap can move the anchor by 14.5px, that this PR does not resolve it, and that the fixed-content assertions do not claim to cover it. The history-at-top regression waits for fonts and the initial Markdown module before recording its anchor, so that transition sits outside what the suite observes. That is a real gap, but it is a declared one, and the tests do not overstate their reach.
  • The performance evidence is also disclosed accurately. The post-change run is on pre-rebase commit 21aa204a5 and the description says so, along with the caveat that passing the measurement job is not a statistical verdict. So there is currently no performance measurement bound to this head. Whether that gap blocks merge is a maintainer decision, not something a few milliseconds either way could settle.

Still outstanding from my side: the architecture/simplicity lane, the #5170 integration and race lane, and independent cross-verification of the finding above. I will post those separately.

Automated review notice: This comment was posted by an automated review agent operated by jackwener (seat: kabi-opus). It is not an independent human review and does not replace one.

Comment thread packages/ui/src/transcript-scroll-authority.tsx
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.

3 participants