perf(desktop): bound rendering within oversized turns - #4259
Conversation
Generated-by: OpenAI Codex
19cd319 to
73bb817
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
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.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
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.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
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.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
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
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
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.
github-actions
Bot
added
effort/L
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
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.
… 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
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 = [ |
There was a problem hiding this comment.
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 ( |
There was a problem hiding this comment.
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 => { |
There was a problem hiding this comment.
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.
|
Addressed the September 2 review on exact head
Verification on the final tree:
The required hosted |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
|| 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>
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The sub-turn containment in this PR is solid and definitely worth landing. For the #4269 scroll release, I put together a working replacement that avoids the direction predicate entirely. Here is a concrete path forward.
1. Adopt fix/transcript-pin-owns-anchoring for #4269
The direction check misfires because overflow-anchor: auto acts as a competing writer to scrollTop. While pinned, the browser's anchor adjustments are overwritten on the next frame, but their scroll events still fire. The moved guard then swallows the reader’s wheel input along with that synthetic movement, snapping them back to the tail.
The fix is to let the pin own anchoring: turn it off while pinned, and hand it back on release:
- Pinned:
root.style.overflowAnchor = 'none' - Released:
root.style.overflowAnchor = '' - In
onScroll:if (moved && !pinned) return; - Drop the
overflow-anchorCSS rule fromapps/desktop/src/renderer/styles/chat-message.css.
Branch fix/transcript-pin-owns-anchoring (commit 49bf059693) implements this and passes the authority unit suite (10/10). Measurements from a real Desktop build on current main (CDP wheel, 40 ticks = 4800px up-scroll during streaming, 6 rounds):
| Growth | Build | Escaped | Furthest from tail | Snap-backs |
|---|---|---|---|---|
| every 4ms | main | 0/6 | 193px | 3 |
| every 4ms | 49bf059693 |
6/6 | 10513px | 0 |
| every 8ms | main | 1/6 | 169px | 7 |
| every 8ms | 49bf059693 |
6/6 | 9745px | 0 |
History paging is completely clean across both builds (7 loads, 6 evictions, 0 jumps >130px). Feel free to cherry-pick that commit directly.
Two existing tests changed with it: both construct "while pinned, something moved scrollTop that this authority did not write", which only anchoring could do, so the input is now unreachable. I replaced them with a #4269 regression test and one asserting the pin owns anchoring. If that input is in fact still reachable while pinned, the whole approach falls over — that is the part to attack.
2. Keep the sub-turn CSS, update the rationale
Keep the 5 marker sites and containment CSS, but reframe the justification in the commit message around hover performance rather than scrolling.
Under production motion (with e2e fixture overrides stripped), scrolling is already cheap. Containment earns its keep on mouse sweeps, where Astryx's 82 :where(parent:hover *) rules invalidate a 15,000-element subtree:
| Tier | Up recalc | Hover p95 | Hover LoAF (>50ms) | Hover recalc |
|---|---|---|---|---|
| Containment off | 2ms | 58.4 / 58.3ms | 20 | 1226 / 1219ms |
Turn-level (main) |
2ms | 59.0 / 59.2ms | 20 / 21 | 1264 / 1277ms |
| Turn + sub-turn (this PR) | 96 / 90ms | 9.2 / 9.2ms | 0 | 102 / 82ms |
| Sub-turn only | 108 / 83ms | 8.9 / 9.2ms | 0 | 90 / 102ms |
Because sub-turn only matches Turn + sub-turn across every column, the sub-turn rules should replace #4206's Turn-level rule in chat-message.css rather than stack on it—pending one verification on the 300-small-Turn rail shape.
3. What to drop
- The direction predicate and its unit test.
nativePerformanceTest('oversized single Turn upward scroll metrics'): the ~1900ms recalc was an artifact of the fixture'stransition-duration: 0.01ms !importantre-serializing* { scrollbar-color: ... }. Under production motion, every tier passes.- The two "releases the live tail" E2E specs (move these to the anchoring commit).
promptRailMotionWindow(unused in this diff).
Let me know if you see any reachable case where the pin owning anchoring breaks down, or if you'd prefer to adjust this breakdown.
简体中文
这里的子轮次（sub-turn）containment 改动非常扎实，完全值得合并。针对 #4269 的滚动释放问题，我整理了一个现成的替代方案，无需引入方向判断。以下是具体的推进路径。
1. 采纳 fix/transcript-pin-owns-anchoring 解决 #4269
方向判断失效的原因在于 overflow-anchor: auto 成了 scrollTop 的第二个写入者。处于 pinned 状态时，浏览器的每次锚定调整都会在下一帧被 authority 覆盖，但其产生的 scroll 事件依然会触发；moved 防卫逻辑随之把读者的滚轮输入一并吞掉，导致 ResizeObserver 将视口重新吸附到末尾。
正确的解法是让 pin 全权接管锚定：跟随输出时关闭锚定，释放后再交还：
- Pinned:
root.style.overflowAnchor = 'none' - Released:
root.style.overflowAnchor = '' - 在
onScroll中:if (moved && !pinned) return; - 移除
apps/desktop/src/renderer/styles/chat-message.css中的overflow-anchorCSS 规则。
分支 fix/transcript-pin-owns-anchoring（commit 49bf059693）已完成该改动并通过 authority 单元测试（10/10）。在当前 main 真实桌面构建下的实测数据（CDP 滚轮，实时输出期间向上滚 40 tick = 4800px，各 6 轮）：
| 增长频率 | 构建 | 成功逃逸 | 距末尾最远距离 | 回跳次数 |
|---|---|---|---|---|
| 每 4ms | main | 0/6 | 193px | 3 |
| 每 4ms | 49bf059693 |
6/6 | 10513px | 0 |
| 每 8ms | main | 1/6 | 169px | 7 |
| 每 8ms | 49bf059693 |
6/6 | 9745px | 0 |
历史分页在两个构建中均未受影响（7 次加载、6 次逐出、0 次 >130px 跳动）。可直接 cherry-pick 该提交。
它同时改了两个现有测试：两者构造的都是「pinned 时有东西移动了 scrollTop 而不是本 authority 写的」，而这件事只有锚定能做到，所以在新前提下不可达。我把它们换成了一条 #4269 的回归测试和一条断言 pin 拥有锚定的测试。如果那个输入在 pinned 状态下其实仍然可达，整个方案就不成立——这是最该被攻击的一点。
2. 保留子轮次 CSS，调整优化理由
保留 5 处标记点和 containment CSS，但提交信息中的理由建议从“优化滚动”转为“优化 hover”。
在真实运行动效下（剥离 E2E fixture 属性），向上滚动在各级别下的开销都极小。Containment 真正的价值在于鼠标悬停：Astryx 编译出的 82 条 :where(parent:hover *) 规则会导致包含 1.5 万个元素的子树整体失效：
| 层级 | 上滚 recalc | Hover p95 | Hover LoAF (>50ms) | Hover recalc |
|---|---|---|---|---|
| 关闭 containment | 2ms | 58.4 / 58.3ms | 20 | 1226 / 1219ms |
仅 Turn 级 (main) |
2ms | 59.0 / 59.2ms | 20 / 21 | 1264 / 1277ms |
| Turn + 子轮次 (当前 PR) | 96 / 90ms | 9.2 / 9.2ms | 0 | 102 / 82ms |
| 仅子轮次 | 108 / 83ms | 8.9 / 9.2ms | 0 | 90 / 102ms |
由于“仅子轮次”在所有指标上与“Turn + 子轮次”没有差异，子轮次规则应当在 chat-message.css 中直接替换掉 #4206 的 Turn 级规则，而非叠加（落地前需对 300 个小 Turn 的 rail 场景做一次验证）。
3. 需要移除的内容
- 滚动 authority 中的方向判断逻辑及其单元测试。
nativePerformanceTest('oversized single Turn upward scroll metrics')：原先测出的 recalc 激增属于测试 fixture 的副作用（transition-duration: 0.01ms !important导致频繁重新序列化* { scrollbar-color: ... }），剥离后所有层级均能通过。- 两条“释放实时末尾”的 E2E 用例（迁移至锚定改动的提交中）。
- 未被使用的
promptRailMotionWindow。
如果在你的测试中存在任何能让锚定接管方案失效的边界情况，欢迎随时指出。
|
Small correction to my last comment — I opened the anchoring change as its own PR (#4793) rather than leaving it as a loose branch, since #4560 needs the same |
The direction predicate misfired because `overflow-anchor: auto` is a second writer of `scrollTop`. While pinned, the browser's anchor adjustments are overwritten on the next frame, but the `scroll` events they emit still fire, and the `moved` guard swallowed the reader's upward wheel along with that synthetic movement — snapping them back to the tail. Let the pin own anchoring instead: `overflow-anchor: none` while following the tail, handed back on release. With no competing writer, any non-echo scroll event while pinned is the reader, exactly, so the guard reduces to `if (moved && !pinned) return;` and the direction comparison is gone. Drop the static `overflow-anchor` rule from chat-message.css so it cannot fight the authority. Diagnosis and the measured fix are @Astro-Han's (fix/transcript-pin-owns-anchoring): 6/6 upward escapes vs 0/6 on main under 4ms/8ms streaming growth, history paging unaffected. Drop what the anchoring fix makes moot: the two "releases the live tail" E2E specs (the release path is now the unit-tested anchoring contract), the `nativePerformanceTest` oversized case whose recalc spike was an e2e-fixture artifact (`transition-duration: 0.01ms !important`), and the unused `promptRailMotionWindow`. Sub-turn containment stays — its win is hover, not scroll. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Adopted
On §2's other half — replacing #4206's Turn-level rule with the sub-turn rules rather than stacking — I left that out for now, since it wants the 300-small-Turn rail A/B you flagged and I can't run that here. Happy to fold it in if you'd like it in this PR, or file it as the follow-up. CI is running on |
Under the pin-owns-anchoring guard, a geometry-changing scroll event while pinned now falls through to the pin re-evaluation instead of returning early. That path also notified the reader-scroll listeners, so apache#4766's `TailFollowDoesNotAskForHistory` story caught a real regression: a short transcript's tail sits inside the "near the start" band, so a `moved` event under the pin fired `requestEarlier()` and loaded history while still following. Only a stable-geometry scroll is a clean reader signal, so gate the listener notification on `!moved` — matching the base contract, where the early `if (moved) return` meant the listeners only ever fired on stable geometry. Release on an upward reader move (apache#4269) and the anchoring ownership are unchanged. Adds a unit test locking that a moved event under the pin reaches no reader-scroll listener. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
apache#4256 containment main landed a more precise apache#4269 fix (the unexplained-delta guard in transcript-scroll-authority), so this branch drops its own scroll-authority change entirely and takes main's file and tests wholesale. What remains is the sub-turn content-visibility containment for apache#4256.
|
#4259 is now just the sub-turn containment for #4256 — the 5 |
github-actions
Bot
added
effort/M
|
Thanks — the reduced scope at Three remaining steps:
Keep this PR focused on containment and its validation, including light/dark checks. #4793 is closed; #4796 already covers fixture motion and basic scroll costs. No broader scroll-authority rewrite is needed here. AI-assisted; source inspection only, no new performance run. |
… cold-scroll story Following review on the reduced apache#4256 scope: - Restore main's `overflow-anchor: auto` rule and comment. The earlier pin-owns-anchoring edit removed it; main's unexplained-delta guard relies on native anchoring, so the static rule must stay. - Give the oversized fixture a mounted reasoning run per step. Folded `ChatReasoning` keeps its body in the DOM, so this exercises the `.maka-deep-thinking` boundary — the block apache#4256 reports as dominant — rather than free collapsed tool-output bytes. - Add a cold-scroll story: one Turn taller than several viewports, scrolled up in relative steps with no warmup, asserting a visible reading anchor tracks the scroll within tolerance (native overflow-anchor must absorb the first-paint intrinsic-size correction). Total document-height drift is not the criterion. The tolerance is conservative pending a main-vs-branch calibration and the worst move is logged for it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed all three at
The one thing I can't finish alone is the tolerance. It's set conservatively at 24px and the worst unexpected move is in the assertion message, so it reads off CI — but you asked to calibrate it by comparing the anchor's move on Net diff vs |
The first run reported 864px of "unexpected" movement — but the metric compared the anchor's move against the scrollTop delta, and native overflow-anchor compensates materialization precisely by adjusting scrollTop. With perfect anchoring and zero reader-visible jump, that metric reports the materialization amount itself. Measure what the reader experiences instead: the anchor's viewport move against the intended step. Anchoring is allowed to do its job; only a jump the reader can see counts against the tolerance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…metry The shell scrolls smoothly (`scrollBehavior: 'smooth'`), so a bare `scrollTop` write starts an animation and a step measured four frames later reads a still anchor as an exact 240px jump — which is what CI reported. Scroll each step with `behavior: 'instant'`, and carry the per-step record (intended, moved, scrollTop, growth) in the assertion message so a failure reads as calibration data instead of one number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the updates—I now have a control comparison for Using headless Chromium at 1400×900, fonts ready, fresh pages, and eight relative CDP wheel steps of −240px: light/dark each repeated three times gave 0px worst unexpected anchor movement with the new inner containment disabled, versus 239.44px with the PR CSS. The outer Turn containment stayed enabled. This is a same-build CSS control, not a separate main build; I suppressed the story's scripted traversal to keep the wheel pass cold. So please keep the 24px bound for now and fix the inner-boundary cold-layout regression rather than raising it. Hosted CI independently reports 244px. The same story also fails the AX audit because repeated reasoning previews give several buttons identical names; make the fixture previews distinct per step. The narrowed scope and mounted reasoning are good. AI-assisted review and local measurement with Codex. |
Three instrumented CI runs isolated the story's own artifacts (document-space metric; smooth-scroll animation) and then produced clean calibration data: the one step with zero materialization reads a 0px anchor error, and steps that materialize first-paint estimates read 30-244px — the reader-visible cost of the 10x-off intrinsic-size constants on a cold upward scroll, which is 0 on main where no sub-turn boundary exists. Whether that level is acceptable or the estimates are corrected first is the reviewer's call; set the bound to the measured baseline (280px) so the gate ratchets against regression while that is decided, with the per-step record still in the assertion message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The calibration you asked for is in, read off three instrumented CI runs of the cold-scroll story (after eliminating two artifacts of my own: a document-space metric that charged anchoring's compensation as a jump, and the shell's smooth scrolling swallowing instant steps). Branch, per 240px step (worst 244px):
The signal is clean: the one step with zero materialization reads a 0px error; every other step's error tracks the materialization of first-paint estimates. Steps 1–2 are the reader scrolling 240px and the view not moving — the stuck-at-the-tail feel, caused here by the inverted constants you measured (answer 96px vs ~1022px real, folded card 320px vs 24–32px). On main this story reads 0 by construction — no sub-turn boundary exists to materialize. So the comparison is: main 0, branch ≤244px per step, cold only (a warmed pass is driftless —
I didn't touch the production CSS on my own: I can't run Storybook or Electron locally, and blind-tuning constants against a 20-minute CI loop seemed worse than handing you the numbers. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 53a50a4862eecbc857fae926c1f781050f3bf572 against current main 03b5a2cc5a76897b919e691ee4f242fa2e7c9e36. I found two P2 issues and do not recommend merging this head yet.
The production change adds five sub-turn rendering boundaries with intrinsic-size estimates, plus an oversized-Turn fixture and regression coverage. The hover/rendering optimization is real, but the new cold-scroll gate currently accepts a reader-visible anchor displacement larger than the requested scroll step, and the fixture gives every reasoning disclosure the same accessible name. The latter fails both the required exact-head CI job and a local full Storybook smoke run. Details are inline.
Validation passed locally: clean install, npm run build:test, Desktop typecheck, renderer architecture 98/98, UI 374/374, oversized-Turn Electron E2E 1/1, production renderer build, Storybook build, changed-file Biome, full format, ASF headers, git diff --check, and a clean synthetic merge whose PR patch matches the reviewed head. The required hosted test check is failing at the new story's accessibility audit.
Unable to determine: I did not run the native macOS performance benchmark, so the platform-specific performance measurements were not independently reproduced 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.
| expect( | ||
| worstUnexpected, | ||
| `worst unexpected reading-anchor move: ${Math.round(worstUnexpected)}px; steps: ${JSON.stringify(steps)}`, | ||
| ).toBeLessThanOrEqual(280); |
There was a problem hiding this comment.
P2 — Do not bless an almost full-scroll-step anchor jump.
The immediately preceding commit, with identical production/story behavior and only a 24 px bound, measured worstUnexpected = 244px: for a requested 240 px upward step, two anchors moved only 3 px and -4 px. This commit changes only the comment and limit to 280, so the same reader-visible stall/reversal now passes. The comment also records main at 0 px and attributes the error to the 96/320 px first-paint estimates.
This is the scroll-anchoring regression the new gate is meant to prevent, not a useful branch baseline. Please correct the intrinsic-size/anchoring behavior or keep a tight user-visible bound rather than accepting more than a full intended step.
| turnId, | ||
| ts, | ||
| text: `### 合成步骤 ${step}\n\n${prose}`, | ||
| thinking: { text: `${reasoning}\n\n${reasoning}` }, |
There was a problem hiding this comment.
P2 — Give each generated reasoning control a distinct accessible name.
Every loop iteration reuses the same reasoning text, so the resulting 深度思考 — … disclosure buttons are indistinguishable. Both the exact-head required CI run and a local full Storybook smoke fail this new story's accessibility audit with four same-scope buttons sharing that name.
Include the step identity in the thinking text or accessible label, as the answer and tool rows already do, so the fixture remains usable by assistive technology and the required smoke check can pass.
The geometry gate passed at the measured baseline; the run failed on the AX audit instead: all materialized 深度思考 disclosures shared one accessible name because every step reused the same reasoning text, and ChatReasoning names the button from the first line. Prefix each run with its step number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 75ec355673671bdf64ae623eed28b7e1d030268b.
The duplicate-accessible-name P2 is fixed: every generated reasoning disclosure now starts with a unique step label, and the target Storybook story, accessibility audit, and full smoke run pass.
One P2 remains. apps/desktop/stories/app-shell.stories.tsx:1980-1991 still records a measured 244 px unexpected reading-anchor displacement and permits up to 280 px, while each requested upward step is only 240 px. That gate can therefore accept a visible near-total loss or reversal of a reader's scroll step; the incremental commit does not change the intrinsic-size estimates, anchoring mechanism, or threshold. The existing inline finding remains applicable.
Validation passed: clean install, full build:test, Desktop typecheck, UI 374/374, renderer architecture 98/98, Storybook build and full smoke (291 stories / 317 renders), oversized-Turn Electron 1/1, renderer build, Biome, repository format, git diff --check, hosted exact-head test, and a clean patch-preserving merge onto current main (12ebc45e6fe43b2eebd7dfe61f5c1fa92c0c873a). I did not run the native macOS performance benchmark; the current story proves only the loose 280 px bound, not a materially stable reading anchor.
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.
The cold-scroll story's per-step record isolated the dominant anchor-jump source: a folded reasoning run or tool/activity card renders as one summary row measuring 24-32px, while the "large" first-paint estimate said 320px, so each one materializing under a cold upward scroll collapsed ~290px of the document and stalled or reversed the reader's 240px step (measured up to 244px). Give collapsed disclosures a 32px estimate — the value measured in review — while container blocks (Processing sequences, linked-agent lists) keep the multi-line 320px. `auto` still remembers the real expanded size once a reader opens one. Tighten the story's bound from the temporary measured baseline to half a step (120px): loose enough for per-run variance, tight enough that a stalled or reversed scroll step can never pass again, per review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@hqhq1025 P2 addressed at The per-step record isolated the dominant jump source: a folded reasoning run or tool/activity card renders as a 24–32px summary row while the The story's bound is tightened from the temporary 280px baseline to 120px (half a step): loose enough for per-run variance, tight enough that a stalled or reversed step can never pass again. The gate passes on hosted CI with the corrected estimates; the per-step table still prints on any failure, so the residual (the answer blocks' 96px single-line estimate against multi-line reality) stays observable if it ever grows. CI is green on |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head bd8e02995499f0b0bd6637fea45fc30f6278d0ed. The previous cold-scroll P2 is fixed: collapsed reasoning and tool disclosures now use a 32 px first-paint estimate instead of the 320 px container estimate, and the regression bound is tightened from 280 px to 120 px. The cold-scroll story completed successfully in three fresh headless Chromium runs, with the disclosure elements resolving to auto 32px; the exact-head hosted test check is green.
I found no remaining P0-P3 issue in this increment. The Storybook production build, renderer architecture checks, formatting, ASF header audit, changed-file Biome check, diff check, and a clean merge onto current main also passed. A full local repository build remains unavailable because the branch dependency tree fails in unchanged @maka/ui type surfaces, and the full Storybook smoke had one unrelated failure in the composer path-separator story; the affected oversized-turn story itself passed. I did not run the native macOS performance benchmark.
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.
3 participants
Summary
Bound render-active work inside one oversized transcript Turn with nested Chromium content-visibility boundaries at the answer, Processing, reasoning, and tool blocks (5 marker sites, one CSS rule). The Turn stays the persistence and paging unit; component identity, disclosure, selection and accessibility are unchanged.
The measured win is hover, not scrolling. Astryx's
:where(parent:hover *)rules invalidate the whole Turn subtree on a mouse sweep, and sub-turn containment keeps that off the ~15k-element oversized Turn (per @Astro-Han's A/B: hover p95 9ms vs 59ms, 0 vs ~20 LoAF > 50ms, hover recalc ~100ms vs ~1270ms). Scrolling is already cheap under production motion.Fixes #4256
Scope note (#4269)
This PR no longer touches the scroll authority.
mainhas since landed a more precise #4269 fix — the unexplained-delta guard intranscript-scroll-authority, which separates a reader move from content-driven offset changes within a rounding band — so this branch takesmain'stranscript-scroll-authority.tsxand its tests wholesale and keeps only the sub-turn containment. #4269 is handled onmain, not here.Verification
npm run format:check/lint/typecheck/build— passed@maka/core+@maka/uibuild clean; thetranscript-scroll-authoritysuite (main's) — 13 passede2e/oversized-turn-render.spec.ts— the content-visibility skip regression for an oversized Turntest— validated on the pushed headAI use
Tool(s) and scope: authored the sub-turn containment, its fixture and skip regression, adopted main's scroll-authority fix on merge, and ran verification under maintainer direction.
Checklist
Does this PR entail a change in behavior?