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.
| expect( | ||
| result.loafOver50Ms, | ||
| `oversized-Turn upward scroll exceeded the 50 ms Long Animation Frame gate: ${JSON.stringify(result)}`, | ||
| ).toBe(0); |
There was a problem hiding this comment.
[P2] The gate now works, but the implementation still exceeds it on this exact head
The new assertion correctly fixes the previous test-harness gap. However, an exact-head Linux/Xvfb run produced loafOver50Ms: 9 and loafMaxMs: 76.9, so this test now fails at the documented 50 ms criterion. Because the benchmark is opt-in and hosted CI does not exercise it, the current green checks would still not demonstrate the issue’s stated performance outcome. Please either make the implementation satisfy the threshold on the supported target matrix, or explicitly define and enforce the intended benchmark environment so this failure is not silently outside the release gate.
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.
| .maka-tool-activity-card | ||
| ), | ||
| .maka-processing-sequence > * { | ||
| content-visibility: auto; |
There was a problem hiding this comment.
[P2] Suggestion (category ① — normal keyboard/assistive-navigation path): content-visibility: auto also applies to the focusable reasoning and tool-card headers. When a user tabs or moves assistive-technology focus to an offscreen skipped card while the transcript is still pinned, Chromium materializes that block to satisfy focus, its geometry expansion reaches the transcript ResizeObserver, and transcript-scroll-authority.tsx writes the viewport back to the tail because no wheel event released pinned. The focused item can therefore be pushed away just as the user reaches it. Could focus-driven reader movement release the tail (or otherwise prevent this materialization from triggering the pinned write), with a real keyboard-focus regression covering an offscreen reasoning/tool header?
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.
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— 276 passednpx playwright test --config e2e/playwright.config.ts e2e/oversized-turn-render.spec.ts— 2 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"— passedSynthetic performance sample on one local macOS arm64 run: 145 records / 736,799 serialized characters in one Turn, 25,424 CSS px upward traversal over 480 animation frames, frame P95 16.6 ms, P99 32.5 ms, maximum 41.8 ms, and 0 Long Animation Frames over 50 ms. Chromium still reported 47 offscreen render segments skipped at the destination. This is regression evidence, not a general 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?