Skip to content

perf(desktop): bound rendering within oversized turns - #4259

Open
liugddx wants to merge 22 commits into
apache:mainfrom
liugddx:fix/oversized-turn-render-bounds
Open

perf(desktop): bound rendering within oversized turns#4259
liugddx wants to merge 22 commits into
apache:mainfrom
liugddx:fix/oversized-turn-render-bounds

Conversation

@liugddx

@liugddx liugddx commented Aug 30, 2026

Copy link
Copy Markdown
Member

Summary

Bound render-active work inside an oversized transcript Turn by adding nested Chromium content-visibility boundaries at existing answer, Processing, reasoning, and tool blocks. The Turn remains the persistence and paging unit; scroll anchoring, component identity, disclosure state, selection, and accessibility semantics stay unchanged.

Release live-tail following when the reader scrolls up, decided by direction rather than by enumerating input devices. This authority only ever writes the offset toward the tail, so a scroll event whose offset is now above its last write is the reader having moved up — the one signal growth cannot forge, even while content-visibility placeholders expand scrollHeight in the same frame. A geometry change whose offset sits at or past the last write is growth outrunning the follow and keeps the pin. Re-pinning still requires returning to the tail on stable geometry or an explicit pinToTail(). No wheel/keydown/pointerdown/focus listeners are added; the guard is one direction comparison in onScroll.

Add a deterministic content-free fixture whose single Turn exceeds the 512 KiB transcript range budget, plus:

  • an Electron regression proving offscreen timeline blocks are skipped and reactivate when scrolled into view;
  • an Electron regression proving upward wheel scrolling stays released while skipped geometry materializes and later content grows;
  • an Electron regression proving PageUp from a visible card header stays released through later content growth;
  • state-machine regressions distinguishing reader upward motion from growth that outran the write while the geometry changes in the same frame;
  • a manual CDP performance probe (native macOS arm64) for the pathological upward-scroll path.

Fixes #4256
Fixes #4269

Verification

  • npm run format:check — passed
  • npm run lint — passed
  • npm run typecheck — passed
  • npm run build — passed
  • node --test on the transcript-scroll-authority state-machine suite — 10 passed (incl. the restored "growth that outruns the write" contract and the new upward-under-changing-geometry release)
  • hosted GitHub CI test job — passed on the merged head
  • MAKA_TRANSCRIPT_PERF=1 npx playwright test … native-transcript-perf.spec.ts --grep "oversized single Turn upward" — native macOS arm64 only; skipped elsewhere

The 50 ms Long Animation Frame gate asserts that no long-animation-frame entry exceeds 50 ms (LoAF only emits entries above 50 ms, so this is "zero long animation frames", not a separately calibrated bound). It runs only on native macOS arm64; Linux/Xvfb's software display is not treated as a comparable native-compositor benchmark, and the probe declares an explicit skip there. Functional Electron coverage remains cross-platform.

Open review item (Astro-Han, pre-merge)

Moving the containment boundary inside the Turn newly destabilises one dimension no surviving gate covers: the anchored Turn's own height changes as the reader scrolls up through it, so scrollHeight drifts across an upward traversal (measured ~+63% on the fixture; ~0 with inner boundaries disabled). The turn-level anchoring gate that would have caught this lived in transcript-scroll.spec.ts, removed by #4741. A no-regression gate for geometry drift — sampling scrollHeight - clientHeight at both ends of the traversal and asserting it does not drift materially — belongs in the perf probe, with a per-instance measured intrinsic-size mechanism (cf. #2237's turn-size-index) rather than the current per-type estimates to make it pass. This needs native macOS arm64 measurement to validate.

AI use

  • Generative tooling made a substantive contribution

Tool(s) and scope: investigated sanitized performance and interaction evidence, implemented the containment and direction-based live-tail release, authored the fixture and regressions, and ran verification under maintainer direction.

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/M Under 500 readable lines label Aug 30, 2026
@liugddx
liugddx force-pushed the fix/oversized-turn-render-bounds branch from 19cd319 to 73bb817 Compare August 30, 2026 11:27

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

Found one important issue: the new oversized-Turn performance harness does not enforce the stated 50 ms limit, and the exact head exceeded that limit in two consecutive local runs while the test still passed. The containment behavior itself passed focused functional, accessibility, scroll, and build checks.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread apps/desktop/e2e/native-transcript-perf.spec.ts

@hqhq1025 hqhq1025 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 exact head c0f66317387a01a3151d3925296ede26dbb0a13b. The previous harness-coverage issue is fixed: the opt-in benchmark now asserts that no Long Animation Frame exceeds 50 ms. I found one remaining performance failure on the exact head and left it inline. The functional oversized-Turn and live-tail behavior passed focused tests.

Validation: npm run build:test, full workspace typecheck, UI 276/276, the two oversized-Turn functional Electron tests, changed-file Biome, git diff --check, and a synthetic merge with current main passed. The opt-in performance test failed on Linux/Xvfb with 9 frames over 50 ms and a 76.9 ms maximum. The hosted test check was still running at publication.

Unable to determine: I could not reproduce the author’s macOS arm64 environment, so the cross-platform performance envelope remains uncertain.

Result: NO-GO for the current stated 50 ms acceptance criterion until the benchmark passes on the supported target matrix or the intended benchmark environment is explicitly scoped.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread apps/desktop/e2e/native-transcript-perf.spec.ts

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

Thanks for the focused performance work, and for turning the 50 ms Long Animation Frame criterion into a real failing gate on the latest head.

I reviewed exact head c0f66317387a01a3151d3925296ede26dbb0a13b. The oversized-Turn problem and the nested rendering boundary are well motivated. I found one separate P2 inline in the normal keyboard/assistive-navigation path. It does not challenge the value of the performance change, but I think the tail-follow authority needs to account for focus-driven materialization.

Please do push back if focus navigation through transcript activity cards is intentionally outside the supported interaction contract; the finding is based on Chromium materializing skipped focus targets and the current ResizeObserver tail-write path.

中文对照

感谢把超大 Turn 的性能问题和 50 ms 门槛做成可执行验证。当前 head 的性能方向成立;另有一个独立 P2:键盘或辅助功能聚焦被跳过的 activity card 时会触发布局展开,而当前 tail-follow authority 仍可能把视图写回尾部。若 transcript 内的焦点导航不属于支持范围,也欢迎补充上下文。

AI-assisted review disclosure: Codex ran an independent analysis lane; Astro-Han independently verified the exact head, Chromium focus/materialization path, scroll-authority composition, and severity, and owns this review.

Comment thread apps/desktop/src/renderer/styles/chat-message.css

@hqhq1025 hqhq1025 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 exact head 895cb38bff24aaab82fc4fcab27c0b45174dc8c7. I found no remaining P0-P3 issues.

The two prior blockers are addressed on this head. The 50 ms Long Animation Frame assertion is now explicitly scoped to the calibrated native macOS arm64 environment, while unsupported Linux/Xvfb execution reports a skip instead of a misleading pass or failure. The focus-navigation path now releases live-tail following when focus enters a skipped offscreen transcript block, and the real keyboard regression preserves both the focused control and viewport position through subsequent transcript growth.

Validation: clean npm ci, npm run build:test, full workspace typecheck, UI 276/276, renderer architecture 62/62, oversized-Turn Electron tests 3/3, the keyboard-focus test repeated 3/3, transcript-scroll Electron tests 10/10, full lint and format checks, git diff --check, a clean synthetic merge with current main, and hosted test / windows_recovery all passed. The opt-in performance command on Linux/Xvfb produced the expected explicit skip.

Unable to determine: this Linux host cannot independently reproduce the native macOS arm64 50 ms benchmark, so the author's five-run macOS measurements remain platform-specific evidence rather than independently verified results here.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

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

Thanks for the focused follow-up. I confirmed that the platform-scoped performance gate and the offscreen-focus case from the earlier review are both addressed. I found one separate focus/growth interleaving below. This was an AI-assisted review using Codex; I independently checked the scroll authority, ChatLayout composition, regression coverage, and exact-head CI. Please push back if Chromium or the layout enforces an ordering I have overlooked.

Comment thread packages/ui/src/transcript-scroll-authority.tsx Outdated
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Aug 31, 2026

@hqhq1025 hqhq1025 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 exact head d12b06162ecb657ed8b88fd129793f42e5d695a7. I found one P2 and do not recommend merging this head yet.

This follow-up scopes focus handling to the transcript and compares scrollTop with the authority's last write before deciding that focus moved the reader. The intended pending-growth case passes in isolated Linux/Xvfb runs, but the exact-head hosted test job failed the new regression with the viewport 804 px from the tail. The remaining race is described inline.

Validation: clean install, npm run build:test, full workspace typecheck, UI 276/276, renderer architecture 62/62, the oversized-Turn file 4/4, the new pending-growth case 10/10 in isolated repeats, transcript-scroll Electron tests 10/10, lint, format, git diff --check, and a clean synthetic merge with current main passed locally. Hosted windows_recovery passed; hosted test failed this exact focus/growth regression. A known 96 px assertion fluctuation in the older offscreen-focus test reproduced on both 895cb38 and this head, so I did not attribute that separate flake to this delta.

Unable to determine: this Linux host cannot independently validate the native macOS arm64 performance gate, and the hosted-only focus failure did not reproduce in ten isolated local repeats.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread packages/ui/src/transcript-scroll-authority.tsx Outdated
liugddx and others added 4 commits August 31, 2026 14:19
… estimate

A 96px contain-intrinsic-block-size placeholder on multi-line terminal/tool
blocks forces a large scrollHeight correction as the reader approaches them,
which the live-tail-release layer then compensates for. A closer first-paint
estimate lets native overflow-anchor absorb most of the correction. Estimate
only (auto <n>px still grows to real size); no content-visibility change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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

Third pass, on efe3acb2. The new head only changes CSS (intrinsic-size estimate 96px to 320px), so the focus/growth interleaving from last round was closed on the previous head; I re-read it and agree it is closed in code. @maka/ui is green locally (278/278). Two things to fix before this can go in, one direction question, then a rebase note.

The regression I asked for last round is not what the E2E tests. visible composer focus during pending growth keeps the live tail focuses COMPOSER_INPUT. The composer lives in the layout's bottom dock, outside .maka-chat-message-list, so onFocusIn returns at the TRANSCRIPT_SELECTOR guard before any of the new logic runs. Delete onFocusOut, incomingFocus and the pre-focus visibility check and that test stays green. The half I asked about, an already-visible transcript control focused while growth is pending, is covered only by the fake-DOM unit test, whose closest() and contains() return true unconditionally. Point the E2E at a visible tool-card header in the fixture instead.

The boundary selector list now exists in four places, and two are already out of sync. chat-message.css (5 selectors), FOCUS_VISIBILITY_BOUNDARY in transcript-scroll-authority.tsx (same 5, copied across the package boundary), SEGMENT in oversized-turn-render.spec.ts (3, missing .maka-deep-thinking and .maka-tool-activity-card) and the perf spec (3). The next boundary someone adds in CSS silently escapes focus classification and the E2E. The nearest seam is a data-* attribute set where the blocks are rendered; CSS and TS both select on it and the four lists become one.

Direction: release is keyed to input devices. onScroll still returns on moved without touching pinned, which is the #4269 ambiguity, and this PR opens two bypasses for it, wheel and focusin. PageUp/Home/arrow keys with focus in the transcript (which the E2E itself sets up via Shift+Tab), scrollbar drags and scrollIntoView are still dragged back to the tail while streaming. This PR widens that window: one upward gesture used to trigger at most one Turn-level materialization, now it can trigger many inside one Turn (the E2E asserts 80+ boundaries). Rather than a third device listener, I would have onScroll account for its own writes within the delivery window and treat the remainder as reader movement. If you go that way, the wheel and focusin listeners, FOCUS_VISIBILITY_BOUNDARY, the incomingFocus state and two of the three new unit tests fold into that one ledger, and the production diff roughly halves. Happy to hear why not.

Smaller, take or leave: the deltaY > 0 branch in nestedScrollerConsumesWheel is unreachable (both callers return on deltaY >= 0); || target in focusEventRoot exists only for the fake root, give the fake an ownerDocument; the new overscroll-behavior: contain rule changes history paging for the pannable mermaid viewport (correct, but undeclared); isOutsideViewport uses the scroll container's border box, so a control hidden under the composer counts as visible; the second CSS block repeats four selectors to override one property, one rule plus one exception is enough; the perf gate needs MAKA_TRANSCRIPT_PERF=1 and macOS arm64, so CI never runs it, and skippedSegments in it is logged, not asserted.

Rebase: git merge-tree against main conflicts only in native-transcript-perf.spec.ts, where #4523 moved the perf skip to declaration time. Do the same for the platform gate, so the Electron window is not built just to skip.

Evidence boundary: static read of efe3acb2 against main cdb29399, @maka/ui unit suite run; Playwright not run; the non-wheel release path is derived from the onScroll control flow, not reproduced in Electron.

AI-assisted review: drafted with Maka; I verified the composer guard, the four selector lists and the merge conflict myself.

简体中文

新 head 只改了 CSS,上轮的 focus/growth 问题在前一个 head 已关。两条合并前要修:E2E 用 composer 做焦点目标,它在 .maka-chat-message-list 之外,onFocusIn 在守卫处就返回了,没测到被要求的那半边,改用 fixture 里可见的工具卡头;边界选择器列表现在有四份,两份已不同步,建议渲染处打 data-* 属性统一。方向问题:释放规则按输入设备开旁路,键盘、拖滚动条等仍会被拽回尾部,且本 PR 放大了这个窗口,建议在 onScroll 里做位移对账,能把生产 diff 减半。rebase 只在 perf spec 与 #4523 有一处机械冲突。

composerRect.bottom > rootRect.top && composerRect.top < rootRect.bottom,
distance: element.scrollHeight - element.scrollTop - element.clientHeight,
};
}, COMPOSER_INPUT);

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.

The composer is in the bottom dock, outside .maka-chat-message-list, so onFocusIn returns at its first guard and none of the new focus logic runs for this test. Focus a visible tool-card header in the fixture instead; that is the case from last round.

const PIN_THRESHOLD_PX = 10;
const BUTTON_THRESHOLD_PX = 100;
const TRANSCRIPT_SELECTOR = '.maka-chat-message-list';
const FOCUS_VISIBILITY_BOUNDARY = [

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.

Copied from chat-message.css across the package boundary, and the two E2E copies already dropped two of these. A data-* attribute where the blocks are rendered lets CSS and this select on one fact.

if (!['auto', 'scroll', 'overlay'].includes(overflowY)) continue;
if (target.scrollHeight <= target.clientHeight) continue;
if (event.deltaY < 0 && target.scrollTop > 0) return true;
if (

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.

Unreachable: both callers return on deltaY >= 0 before getting here.

// content movement, so release synchronously while the input ownership
// is still unambiguous. A wheel consumed by a nested scroller is not an
// outer-transcript gesture and leaves the pin untouched.
const onWheel = (event: WheelEvent): void => {

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.

This and focusin are the only two inputs that can release while geometry is moving; keyboard paging, scrollbar drags and scrollIntoView still hit the moved return in onScroll and get pulled back. If onScroll subtracted its own writes for the delivery window, both listeners could go.

@liugddx

liugddx commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Addressed the September 2 review on exact head ffc698b42, merged with current main 72eb982d5.

  • The pending-growth E2E now focuses an already-visible .maka-tool-activity-card header inside the transcript, rather than the composer outside .maka-chat-message-list.
  • Renderers now set data-maka-transcript-boundary at answer, Processing, reasoning, tool-card, and linked-agent boundaries. CSS, focus classification, functional E2E, and the perf probe all select that one fact.
  • Added a real PageUp + same-delivery geometry-growth Electron regression. Upward wheel, transcript keyboard navigation (ArrowUp, PageUp, Home, Shift+Space), non-overlay scrollbar interaction, and focus reveal now release while input ownership is known. Nested scrollers retain ownership.
  • I tested the suggested pure onScroll ledger direction. In real streaming E2E, intrinsic geometry correction moved both scrollTop and scrollHeight without reader input and was misclassified as reader movement, leaving the transcript 400+ px from the tail. The final implementation therefore keeps geometry-changing scroll events neutral and handles ambiguous reader inputs synchronously. The normal streaming-tail and nested-scroller regressions pass on this form.
  • Removed the unreachable downward branch, moved the macOS arm64 perf gate to declaration time, and made skippedSegments an assertion.
  • Rebased/merged current main; the perf-spec conflict follows ci: cut single-job validation time by up to 40% #4523's declaration-time skip shape.

Verification on the final tree:

  • npm --workspace @maka/desktop run build:with-deps passed, including renderer entry and third-party notice checks.
  • @maka/ui full suite: 310/310 passed.
  • oversized-Turn Electron suite: 5/5 passed.
  • targeted transcript-scroll regressions for ordinary upward release, nested-scroller consumption, and the history boundary: 3/3 passed.
  • UI and Desktop typechecks, changed-file Biome, and git diff --check passed.

The required hosted test check is currently running. @Astro-Han, please re-review this head when it completes.

@liugddx
liugddx requested a review from Astro-Han September 2, 2026 12:07

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

Good round. The boundary is now one fact (data-maka-transcript-boundary on five render sites, one CSS rule, one selector in the authority and both specs), the unreachable deltaY > 0 branch is gone, the perf spec skips at declaration time and asserts skippedSegments, and merge-tree against main is clean. @maka/ui 315/315, desktop typecheck, lint, format and the architecture check pass locally. Your case against the onScroll ledger, that intrinsic geometry corrections would read as reader movement, is concrete and I accept it.

What I cannot accept yet is the test evidence for the focus subsystem. Last round's two unit tests were the ones where closest() and contains() always returned true; this round deletes them and adds nothing in their place, and the new E2E (visible transcript focus during pending growth keeps the live tail) goes focus, blur, growth, focus on the same header. Blur to body carries no in-transcript relatedTarget, so incomingFocus is undefined at the second focus, readerMoved is false, and the handler does not release. Without onFocusIn and onFocusOut at all the pin also stays. Removing the whole focus block (about 28 lines) leaves every test green. The regression that would prove the early return is a Tab from one visible transcript control to the next visible one, so focusout carries a real relatedTarget, with growth appended in the same task.

The PageUp regression has a related problem: it sets tabIndex = -1 on the scroller and focuses it, which takes the event.target === target branch. The production scroller has no tabindex, so the only reachable path is focus on a card header inside the list, the way the Shift+Tab test already does it. Same fix.

Not blocking, but please say two things in the body. The keydown and pointerdown listeners make release an explicit enumeration of input devices, which is outside #4269 and leaves touch drag still pulled back by onScroll, a pre-existing behaviour whose window this PR widens. And the overscrollBehaviorY check changes history paging on pannable mermaid viewports: scrolling up at the top no longer reaches requestEarlier(). Correct, but undeclared and untested.

Two lines to drop: || target in focusEventRoot has no consumer now that the fake-DOM tests are gone, and nestedScrollerConsumesWheel only forwards composedPath(). And onWheel and onKeyDown still walk getComputedStyle over the composed path after the pin is already released; an early if (!pinned) return saves a forced style recalc per wheel event in exactly the gesture the perf gate measures.

Evidence boundary: static read of ffc698b4 against main 92fa5281; @maka/ui unit tests, desktop typecheck, lint, format and architecture check run locally; Playwright not run, the focus reasoning is from the handler control flow, not an Electron run.

AI-assisted review: drafted with Maka; I verified the boundary attribute, the E2E focus sequence and the PageUp focus target myself.

简体中文

这轮边界收敛成一个 data 属性、死分支删了、perf spec 声明期 skip 并断言,merge 干净,本地全绿;对 ledger 方案的反驳有具体依据,我接受。还差的是 focus 子系统的测试证据:上轮两个假 DOM 单测删了没有替代,新 E2E 的 focus→blur→growth→focus 序列在有无监听器时结果一样,整块删掉测试仍绿。需要一条 Tab 到下一个可见控件(带真实 relatedTarget)并在同一 task 追加 growth 的回归。PageUp 回归给 scroller 设 tabIndex=-1 走的是生产不可达分支,改成聚焦卡片 header。非阻塞:正文声明按输入设备枚举释放(触屏拖动仍被拽回)和 mermaid 可平移视口的历史分页变化;两处一行可删;释放后 wheel/keydown 早返回省一次 style recalc。

await root.evaluate((element) => {
const list = element.querySelector('.maka-chat-message-list');
if (!list) throw new Error('the transcript content box is missing');
element.tabIndex = -1;

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.

The production scroller has no tabindex, so event.target === target is unreachable there. Focus a visible card header as the Shift+Tab test does; that exercises the closest() branch users actually hit.

next.closest(TRANSCRIPT_SELECTOR) !== null && isOutsideViewport(next),
};
};
const focusEventRoot = target.ownerDocument || target;

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.

|| target existed for the fake-DOM tests that this round deleted; nothing consumes it now.

…rection

- oversized-turn PageUp regression now focuses a visible tool-card header
  inside the list, exercising the `closest('.maka-chat-message-list')` branch
  users actually hit; the production scroller carries no tabindex, so the old
  `event.target === root` path was unreachable.
- inline `nestedScrollerConsumesUpwardInput` at both wheel call sites and drop
  the `nestedScrollerConsumesWheel` forwarder that only wrapped `composedPath()`;
  drop the `|| target` fallback in `focusEventRoot` that only the removed
  fake-DOM tests needed.
- return early from `onWheel`/`onKeyDown` once released, skipping the
  composed-path `getComputedStyle` walk in the exact gesture the perf gate
  measures.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rowth

Steps focus between two visible tool-card headers in one task with growth
appended while the ResizeObserver is still pending, so `focusout` carries a
real in-transcript `relatedTarget` — the release path the blur-to-body
fixtures could not reach. Asserts the pin survives and the tail follows growth
back to the bottom.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@liugddx

liugddx commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Pushed 615bcce6 addressing the September 2 review.

Done

  • PageUp regression targets the production path. It no longer sets tabIndex = -1 on the scroller and focuses it; it focuses a visible tool-card header inside the list, so the keydown handler is reached through event.target.closest('.maka-chat-message-list') — the branch users actually hit. Same shape as the Shift+Tab test.
  • Dead code dropped. nestedScrollerConsumesWheel is gone; nestedScrollerConsumesUpwardInput is exported and inlined at both wheel call sites (transcript-scroll-authority.tsx, use-chat-scroll.ts). focusEventRoot is target.ownerDocument.
  • Perf. onWheel and onKeyDown return early when !pinned, so the composed-path getComputedStyle walk no longer runs after release — the exact gesture the perf gate measures.
  • Body. Added the two behaviour notes: the input-device enumeration (outside bug(ui): upward scroll can snap back to the live tail during geometry changes #4269; touch-drag still pulled back by onScroll) and the mermaid history-paging change behind the overscrollBehaviorY check.

Focus test — I want your read before calling it done. I added Tab between two visible transcript controls under pending growth keeps the live tail: focus moves between two visible headers in one task with growth appended while the ResizeObserver is still pending, so focusout carries a real in-transcript relatedTarget, and it asserts the pin survives.

What I could not do is make it fail by deleting the before?.outsideViewport === false early return. Tracing onFocusIn for a visible relatedTarget: onFocusOut records before.outsideViewport from isOutsideViewport(next), and onFocusIn recomputes isOutsideViewport(event.target) — both inside the same synchronous focus() call, on the same layout, so they agree. With before defined the readerMoved branch is gated off (before === undefined), and for a visible target the outsideViewport branch is false either way. So on paper the early return changes nothing for a visible relatedTarget, and the offscreen case is already pinned by the Shift+Tab test.

So it's one of two things and I can't tell which from the control flow alone: either the early return guards an async focus-reveal I'm not reproducing — tell me the shape and I'll drive the test to it — or it's redundant, and the honest move is to delete it rather than test it. Which did you have in mind?

Hosted test is running on 615bcce6.

@Astro-Han

Copy link
Copy Markdown
Contributor

The problem is real and I have it filed as #4269, so thanks for picking it up. What I want to push back on is the shape of the fix, not the diagnosis.

The question and the two shapes of fix

The moved guard is what broke, and it is still here. transcript-scroll-authority.tsx classifies a scroll event by whether the geometry changed, and reads a changed scrollHeight as proof that content moved rather than the reader. That held until #4206 put content-visibility: auto with contain-intrinsic-block-size: auto 280px on every turn: scrolling up is now itself the reason placeholders expand. I measured it with CDP on a 120-turn session, and one 4240px turn entering range moves scrollHeight by +3960 in a single frame. So on every upward scroll both causes hold at once, the guard answers "content", and the ResizeObserver re-pins the tail.

This PR leaves that guard as it is and adds five listeners beside it (wheel, keydown, pointerdown, focusin, focusout-capture) to detect the reader by enumeration. Two reasons the enumeration cannot close:

  • Touch-drag is not covered, which the PR body states itself.
  • The scrollbar branch in onPointerDown requires target.offsetWidth - target.clientWidth > 0. macOS overlay scrollbars take no width, so that difference is 0 and the branch has never executed on the platform most of us develop on. Dragging the scrollbar still does not release the pin.

I would also rather not rewrite the header comment for this. It currently says the design's value is that it holds by construction, and names scroll direction, height deltas and wheel events as the signals not to infer from, because each of them has more than one cause. That paragraph is the reason the file is shaped this way. The PR replaces it with a rationale for the opposite. If we do decide inference is the way, that argument belongs in the PR description and the commit message, not in a rewritten claim about what the file guarantees.

A smaller shape. The guard does not need to answer "who moved". Leaving the tail upward is monotonic and readable from position alone; only re-pinning needs proof of reader intent. Keep the moved branch from setting pinned = true, but let it release when distance > PIN_THRESHOLD_PX. That is a few lines and adds no listener.

Keep the CSS. The sub-turn containment is the only thing addressing bounded rendering inside a single oversized turn, and it is what shrinks that +3960 jump. I would land it on its own.

Two smaller notes:

  • use-chat-scroll.ts:170 already installs a wheel listener on the same element. This PR extracts a shared helper but does not merge the two, so there are still two wheel listeners with two sets of conditions.
  • The behaviour note about mermaid is a real regression this PR introduces. With overscrollBehaviorY: contain|none, scrolling up at the top of a pannable diagram no longer chains to the transcript, so a reader whose pointer happens to rest on a diagram cannot load earlier history.

What I measured and what I did not. CDP on a 120-turn session, driving with scrollTop writes and dispatched wheel events: content displacement never jumped (Chromium scroll anchoring absorbs the +3960), frame times p95 = 9ms, zero long-animation-frame entries over 50ms. What does reproduce as a felt problem is that the reader cannot advance upward past max(640, clientHeight * 2): every reader scroll below that threshold calls requestEarlier(), and the arriving history pushes scrollTop back to about 1500. I did not cover real trackpad scrolling on the compositor thread, so if you are seeing content actually jump rather than refuse to advance, that path is still open and wants a trace on a real machine rather than E2E.

Next step: split this into the CSS change on its own plus the guard fix above, and drop the input enumeration. If you think the guard fix is not sufficient, what would settle it is a case where the reader leaves the tail upward and position alone cannot tell. I could not construct one.

AI-assisted review: drafted with Maka. The CDP measurements, the scrollbar-width finding and the diagram are mine.

简体中文

问题是真的,我这边立在 #4269,谢谢你接。我要反对的是修法的形状,不是诊断。

坏掉的是 moved 那道守卫,而它还在。 分类器用「几何有没有变」来判断这次滚动是谁造成的,把 scrollHeight 变化当成「是内容在动、不是读者」的证据。这在 #4206 之前成立,但那次给每个 turn 加了 content-visibility: autocontain-intrinsic-block-size: auto 280px 之后,往上滚这个动作本身就是占位撑开的原因。我用 CDP 在 120 条消息的会话上量过:一条 4240px 的 turn 进入渲染范围,单帧 scrollHeight +3960。于是每次上滚两个成因同时成立,守卫每次都答「是内容」,ResizeObserver 紧接着把视口贴回底部。

这个 PR 把那道守卫原样留着,在旁边加了五个监听器(wheel、keydown、pointerdown、focusin、focusout-capture)靠枚举来识别读者。枚举关不上的两个理由:touch-drag 没覆盖,PR 正文自己写了;onPointerDown 里那条滚动条分支要求 offsetWidth - clientWidth > 0,而 macOS 的悬浮滚动条不占宽度,这个差值恒为 0,在我们大多数人开发的平台上那条分支从未执行过,拖滚动条仍然不释放。

另外我不太希望为此改写文件头的注释。它现在写的是这个设计的价值在于「by construction」,并且点名说不要从滚动方向、高度差和滚轮事件去推断,因为每个信号都不止一个成因。那段话正是这个文件写成这样的理由,而 PR 把它换成了相反结论的论证。如果我们确实决定要走推断这条路,那个论证应该放在 PR 正文和 commit message 里,而不是改写成「这个文件保证了什么」的声明。

更小的形状。 守卫不需要回答「是谁在动」。往上离开底部是单调的,光看位置就能判定;只有「回到底部重新贴住」才需要证明是读者的意思。让 moved 分支不再设置 pinned = true,但在 distance > PIN_THRESHOLD_PX 时允许释放。几行,不加任何监听器。

CSS 那半留着。 子 turn 的 containment 是目前唯一在处理「单条超大 turn 内部也要限界」的东西,也正是它把 +3960 那个跳变压下去的。我建议它单独落地。

两条零碎的:use-chat-scroll.ts:170 本来就在同一个元素上装了 wheel 监听器,这个 PR 抽了公共函数但没有合并,同一元素上仍然是两个 wheel 监听器、两套条件。mermaid 那条行为说明是这个 PR 真实引入的退化:overscrollBehaviorY: contain|none 之下,指针停在可平移的图上时,在图的顶部往上滚不再链到 transcript,也就加载不了更早的历史。

我量到的和没量到的。 CDP、120 条消息、用 scrollTop 写入和派发 wheel 驱动:内容位移一次都没跳(Chromium 的滚动锚定把 +3960 吸收了),帧耗时 p95 = 9ms,超过 50ms 的 long-animation-frame 条目为 0。真正复现成「能感觉到」的问题是读者往上推不过 max(640, clientHeight * 2):低于这个阈值的每次读者滚动都会调 requestEarlier(),补进来的历史又把 scrollTop 顶回 1500 左右。真实触控板在合成器线程滚动那条路我没有覆盖,所以如果你看到的是内容真的在跳、而不是推不动,那条路还开着,需要在真机上抓 trace 而不是靠 E2E。

下一步: 把它拆成单独的 CSS 改动,加上上面那个守卫修法,去掉输入枚举。如果你认为守卫修法不够,能定这件事的是一个「读者往上离开底部、而光看位置判定不了」的具体场景,我构造不出来。

liugddx and others added 7 commits September 4, 2026 07:08
…tion

Reshape the oversized-turn fix to the guard change apache#4269 actually needs,
per maintainer review. The regression is that a `moved` scroll event —
geometry changed since the last one — refused to touch the pin, so under a
patch storm the position-based release never ran and the ResizeObserver
re-pinned the reader to the tail every frame ("grabs the scrollbar").

Leaving the tail is monotonic and readable from position alone, so a moved
event now releases the pin when distance > PIN_THRESHOLD_PX; only re-pinning
still needs the stable-geometry path or an explicit pinToTail(). This drops
the wheel/keydown/pointerdown/focusin/focusout enumeration the previous shape
added (which could not close touch-drag or macOS overlay scrollbars, and left
two wheel listeners on the scroller), and restores the by-construction header.

Keep the sub-turn content-visibility containment and its boundary markers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ance

CI showed the pure-distance release regressed two base contracts in
transcript-scroll.spec.ts (content-grows-is-followed, gesture-nested-consumed
keeps-tail): benign growth fires a moved scroll event whose distance to the
now-larger tail exceeds the threshold before writeToTail catches up, and
releasing on distance alone dropped the follow the reader never left.

Distance cannot separate growth from the reader, but direction can. This
authority only ever writes the offset toward the tail, so an offset now above
its last write is the reader having moved up — the one thing growth alone never
produces. Release the pin only for a moved event whose offset is above
lastWrittenTop (and past PIN_THRESHOLD_PX); growth at or past the last write
keeps the pin. This still fixes apache#4269 (an upward gesture that also grows
scrollHeight is read by direction, not refused) without releasing on growth.

Restores the base "growth that outruns the write" unit contract and adds the
upward-under-churn release case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts:
#	apps/desktop/e2e/fixtures.ts

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

c3ad91387 is the right predicate. Distance cannot separate growth from the reader; direction can, and the property you wrote down — this authority only ever writes toward the tail, so an offset above its last write is the reader — is what makes it safe. Verified at e0a96652d: all 10 transcript-scroll-authority tests pass, including the two contracts you restored.

The rest of this is one point, plus the smaller things that follow from it.

1. #4206's gates do not cover what this PR changes, and this PR has to bring its own

#4206 landed the same class of change and set the bar for it: a 7-pair interleaved A/B on production builds, 7/7 wins with median/P95/P99/IQR, and — the part that matters here — predeclared secondary gates, with scroll anchoring named among them and shown not to regress.

This PR uses #4206's harness (native-transcript-perf.spec.ts, which #4206 created) but not its gate discipline. The new case asserts three things: LoAF is supported, loafOver50Ms === 0, skippedSegments > 0. There is no secondary gate at all.

That gap is not hypothetical. [measured] One upward traversal of your own fixture moves scrollHeight from 29,044 to 47,283 — +63%. Same traversal with inner boundaries disabled drifts 0. What the reader sees is the scrollbar thumb shrinking and jumping for the whole gesture.

Three reasons the existing gates let this through, and all three say the gate has to come from this PR:

  • #4206's anchoring gate is turn-level by construction. transcript-scroll.spec.ts asserted Math.abs(turnTop(anchor) - anchorTop) <= 4 for growth arriving below a stationary reader. This PR moves the boundary inside the Turn, so the anchored Turn's own height now changes while the reader scrolls up through it. Under #4206 that could not happen — one boundary per Turn, auto 280px — so nothing ever asserted on it.
  • The file that held those gates was deleted at 08:23 today by #4741, 28 minutes before c3ad91387. Your 07:59 predicate was caught by exactly two of its cases (content that grows outside the turn wrappers is followed too, a gesture a nested scroller consumed does not release the tail) — the gate worked, and then the 09:24 merge removed it from this branch. The Verification section still lists it as passing coverage.
  • The perf spec never runs in CI. MAKA_TRANSCRIPT_PERF appears nowhere under .github/. #4206's A/B lived in its PR body, not in code, so the only durable gates were the e2e ones that are now gone.

I do not think the answer is to correct the two estimates. [measured] They are 10× off in opposite directions — default gives assistant answers 96px against a real 1022px, large gives collapsed tool cards 320px against a real 24–32px, and the classification is inverted (Astryx renders tool detail as {isDetailOpen && (…)}, so collapsed it is not in the DOM at all). Better constants would shrink the drift, but they would still be four guesses where #4206 had one measured-harmless guess at a coarser grain, and the PR still would not be able to say the geometry is stable.

What I would want instead is the assertion: geometry does not drift materially across the traversal. The harness already computes distance = scrollHeight - clientHeight to size the gesture and then never looks at it again — sampling it at both ends is a couple of lines. How you then make that assertion pass is yours to choose; #2237 solved the same symptom ("the document grows chunk by chunk and the scrollbar keeps moving") with a measured turn-size-index rather than constants, before #2975 removed it, so the constants-free route has a precedent here too.

2. The win is real and bigger than the body claims

[measured] Same build, same fixture, same gesture; only variable is whether inner boundaries are active. Control arm overrides .maka-chat-message-list [data-maka-transcript-boundary] { content-visibility: visible }. Fixed gesture delta for both arms — at the tail scrollHeight - clientHeight reads 28,384 with boundaries on vs 51,116 off, so sizing the gesture from it would make the arms travel different distances. 480 frames × 47,000px.

ON OFF (≈main)
TaskDuration 1108 ms 8117 ms 7.3×
RecalcStyleDuration 304 ms 3729 ms 12.3×
LoAF > 50 ms 0 24
worst frame 25.1 ms 358.4 ms 14×

Single-armed, the body only supports "the post-change value is under a threshold", which badly undersells this. #4206's interleaved form would state it properly.

One wording note: loafOver50Ms: frames.loafDurations.filter((duration) => duration > 50).length — LoAF only emits entries above 50 ms, so the filter is near-tautological and the assertion is really "zero long animation frames". That is a fine gate; "explicitly calibrated" is not what it is. The Xvfb question is answered now, and the comment at native-transcript-perf.spec.ts:27 is enough.

3. The fixture does not contain the reported case

oversizedTurnMessages emits only user, assistant text, tool_call, tool_result — no reasoning records at all. #4256 reports ~98.8k reasoning characters, and reasoning is the opposite of tool detail: astryx-chat-reasoning.tsx keeps {children} mounted and only swaps the wrapper class, so a collapsed body stays in the DOM and keeps costing layout. The .maka-deep-thinking boundary — plausibly the largest win here — is never exercised.

Also toolOutput = 'synthetic output line\n'.repeat(600) × 48: most of the fixture's 736,799 characters is collapsed tool stdout that never enters the DOM, so "exceeds the 512 KiB budget" is calibrated largely on free bytes.

4. The body no longer matches the code

  • "Behaviour notes (September 2 follow-up)" still describes the wheel/keydown/pointerdown enumeration and the touch-drag gap; 496f2014b removed all of it.
  • Verification still lists targeted e2e/transcript-scroll.spec.ts coverage … 3 passed for a file that is no longer in this branch.

The containment is worth landing and the predicate is right, so this is not a request to change the approach. It is that a perf change of this shape has, in this repo, come with a declared no-regression gate, and the one dimension this PR newly destabilises is the one no existing gate covers. §4 is a quick fix; §1 is the one I would want settled before merge.

@liugddx
liugddx requested a review from Astro-Han September 4, 2026 10:06
@liugddx

liugddx commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

@Astro-Han thanks, and agreed on the predicate — direction, not distance, is what makes the release safe.

Since your review at e0a96652d:

  • The body now matches the code. Removed the September-2 "input-device enumeration" note and the touch-drag / mermaid overscrollBehaviorY paragraphs — all of that went with 496f2014b, so there is no longer a wheel/keydown/pointerdown/focus listener to describe. Dropped the stale transcript-scroll.spec.ts line from Verification (removed by test(desktop): remove the layout-tier E2E specs that block main #4741). Fixed the loafOver50Ms wording: LoAF only emits entries over 50 ms, so it is "zero long-animation-frames", not a separately calibrated bound.
  • Merged latest main; the only conflict was the two fixture entries. The hosted test job is green on the merged head, and the 10 transcript-scroll-authority tests pass — including the "growth that outruns the write" contract I restored and the new upward-under-changing-geometry release.

On §1 (the geometry-drift gate) you're right, and I don't want to hand-wave it. The honest constraint is that both the drift and any fix for it are only observable on native macOS arm64: the probe is nativePerformanceTest, and Linux/CI skips it, so I cannot validate a stabilisation mechanism from CI. The right shape is what you describe — sample scrollHeight - clientHeight at both ends of the traversal and assert no material drift, backed by a per-instance measured intrinsic-size (your #2237 turn-size-index precedent) rather than the current per-type estimates — and it needs your environment to confirm the drift actually lands under the gate. The same work should extend the fixture with reasoning records so the .maka-deep-thinking boundary (§3) is exercised, since that changes what the gate measures.

Given the containment win is real and independent (your A/B: 7.3× TaskDuration, 12.3× RecalcStyle, 0 vs 24 LoAF > 50 ms) and you noted this isn't a request to change the approach: would you be open to landing the containment + direction-based release now, with the geometry-drift gate + measured intrinsic-size + reasoning fixture tracked as a scoped follow-up (I'll file it and reference the #2237 / #4206 gate discipline)? If you'd rather it stay in this PR, I'll build the measured mechanism and lean on your macOS run to confirm the assertion, since CI can't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(ui): upward scroll can snap back to the live tail during geometry changes perf(ui): oversized single Turn defeats transcript render bounds

3 participants