perf(desktop): bound rendering within oversized turns - #4259
Conversation
Generated-by: OpenAI Codex
19cd319 to
73bb817
Compare
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
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
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
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
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.
| const rootRect = target.getBoundingClientRect(); | ||
| const outsideViewport = | ||
| focusedRect.bottom <= rootRect.top || focusedRect.top >= rootRect.bottom; | ||
| if (outsideViewport || distanceToTail() > PIN_THRESHOLD_PX) releaseTail(); |
There was a problem hiding this comment.
Thanks for handling focus-driven materialization. [P2] (category ② — normal focus concurrent with transcript growth) focusin is attached to the self-scrolling ChatLayout root, which also contains the composer/dock. After transcript or viewport growth changes scrollHeight but before its ResizeObserver callback writes the new tail, pinned is still true while distanceToTail() is temporarily large. Focusing an already-visible composer or transcript control in that window therefore releases the pin even though focus did not move the reader; subsequent streaming growth stops following the tail until the user manually returns. Could this distinguish focus that actually moved/materialized the transcript from an ordinary visible focus, rather than using the root distance alone? A regression with pending growth → focus visible composer/control → more growth should remain pinned. Please push back if ResizeObserver is guaranteed to run before every possible focus event here.
There was a problem hiding this comment.
Addressed in af3cebfec, with the hosted regression stabilized in 24a20941e. The authority now records the incoming focus target from focusout.relatedTarget on the document and measures its transcript containment boundary before Chromium performs focus-reveal scrolling. A composer/non-transcript focus is ignored; a transcript control that was already visible keeps the live-tail pin even when geometry growth is pending; a control that was outside the viewport before focus still releases the pin.
I added state-machine coverage for both pre-focus-visible reveal scrolling and genuinely offscreen focus, plus the Electron sequence requested here: pending transcript growth -> focus the visible composer -> let ResizeObserver follow -> grow again -> remain at the tail. Exact head 24a20941e passed the full hosted Linux CI, including all 105 Desktop E2E tests, and windows_recovery also passed. Thanks for identifying the delivery-order gap.
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.
| const rootRect = target.getBoundingClientRect(); | ||
| const outsideViewport = | ||
| focusedRect.bottom <= rootRect.top || focusedRect.top >= rootRect.bottom; | ||
| const readerMoved = |
There was a problem hiding this comment.
[P2] Do not classify focus-induced reveal scrolling as reader input
At focusin time, scrollTop !== lastWrittenTop does not prove that the reader scrolled. Chromium may synchronously move the scroller to fully expose a transcript control that was only partially visible before focus; with pending transcript growth, this makes readerMoved true and distanceToTail() large, so this branch releases the pin before ResizeObserver can restore the tail. The exact-head hosted test job reproduced that path in the new regression: focus remained inside the transcript, but the viewport finished 804 px from the tail. Please distinguish explicit reader navigation from the browser's focus-reveal adjustment (or capture the target's pre-focus visibility/scroll state) so ordinary visible focus during pending growth cannot drop live-tail following.
There was a problem hiding this comment.
Addressed in af3cebfec. You are right that scrollTop !== lastWrittenTop at focusin time is not evidence of reader input: Chromium may already have performed focus-reveal scrolling. The authority now captures focusout.relatedTarget and records the target containment boundary's visibility before that reveal. A target visible before focus preserves the pin regardless of the browser-owned offset change; a target outside the viewport before focus releases it. The old distance/offset heuristic remains only as a fallback when no pre-focus transition is observable.
The state-machine suite now covers both sides explicitly, and the real Electron regression covers pending growth -> visible composer focus -> ResizeObserver delivery -> further growth. The prior 96 px hosted failure was the older offscreen test asserting raw scrollTop; that offset can change while native anchoring keeps the focused card on the same pixels, so 24a20941e keeps the user-facing activeTop invariant and the released distance assertion instead. Exact head 24a20941e passed all hosted checks, including 105/105 Desktop E2E tests and windows_recovery.
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 before an effective upward wheel can materialize intrinsic geometry. The input is ignored when a nested tool output, terminal, or contained overscroll surface owns it, so independent inner scrolling does not release the transcript tail.
Release live-tail following when keyboard or assistive-technology focus enters an offscreen skipped activity card. Capture the incoming target before Chromium performs focus-reveal scrolling, so a control already inside the transcript viewport keeps tail-following unchanged even when transcript growth is awaiting ResizeObserver delivery.
Add a deterministic content-free fixture whose single Turn exceeds the 512 KiB transcript range budget, plus:
Fixes #4256
Fixes #4269
Verification
npm run format:check— passednpm run lint— passednpm run typecheck— passednpm run build— passednpm run check:renderer-architecture— 62 passed; architecture check passednpm --workspace @maka/ui test— 278 passednpx playwright test --config e2e/playwright.config.ts e2e/oversized-turn-render.spec.ts— 4 passede2e/transcript-scroll.spec.tscoverage for ordinary upward release, nested-scroller consumption, and the history boundary — 3 passedMAKA_TRANSCRIPT_PERF=1 npx playwright test --config e2e/playwright.config.ts e2e/native-transcript-perf.spec.ts --grep "oversized single Turn upward"— passed on native macOS arm64The 50 ms Long Animation Frame release gate is explicitly calibrated and enforced on native macOS arm64. The test reports an explicit skip on other environments; in particular, Linux/Xvfb's software/virtual display is not treated as a comparable native-compositor performance benchmark. Functional Electron coverage remains cross-platform.
Synthetic performance sample from five consecutive local native macOS arm64 runs: all five reported zero Long Animation Frames over 50 ms, with frame maxima of 25.1–33.6 ms. The fixture contains 145 records / 736,799 serialized characters in one Turn and preserves offscreen skipped segments during the traversal. These measurements are regression evidence for that explicitly scoped environment, not a general cross-platform release benchmark.
npm testwas also attempted with the machine-default Node 23.11.0 rather than the repository release runtime. Unrelated SQLite suites repeatedly failed withcannot start a transaction within a transaction, and the parallel runner was interrupted after it stopped making progress. The Electron E2E above ran with the repository-pinned Node 24.18.1 runtime.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex investigated sanitized performance and interaction evidence, implemented the containment and live-tail changes, authored synthetic fixtures and regressions, and ran verification under maintainer direction.
Checklist
Does this PR entail a change in behavior?