Skip to content

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

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

perf(desktop): bound rendering within oversized turns#4259
liugddx wants to merge 3 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 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:

  • an Electron regression proving offscreen timeline blocks are skipped and reactivate when scrolled into view;
  • an Electron regression proving upward scrolling stays released while skipped geometry materializes and later content grows;
  • a state-machine regression for reader movement and geometry growth before the same scroll event;
  • a manual CDP performance probe 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
  • npm run check:renderer-architecture — 62 passed; architecture check passed
  • npm --workspace @maka/ui test — 276 passed
  • npx playwright test --config e2e/playwright.config.ts e2e/oversized-turn-render.spec.ts — 2 passed
  • targeted e2e/transcript-scroll.spec.ts coverage for ordinary upward release, nested-scroller consumption, and the history boundary — 3 passed
  • MAKA_TRANSCRIPT_PERF=1 npx playwright test --config e2e/playwright.config.ts e2e/native-transcript-perf.spec.ts --grep "oversized single Turn upward" — passed

Synthetic 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 test was also attempted with the machine-default Node 23.11.0 rather than the repository release runtime. Unrelated SQLite suites repeatedly failed with cannot 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:

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

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

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

expect(
result.loafOver50Ms,
`oversized-Turn upward scroll exceeded the 50 ms Long Animation Frame gate: ${JSON.stringify(result)}`,
).toBe(0);

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.

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

.maka-tool-activity-card
),
.maka-processing-sequence > * {
content-visibility: auto;

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.

[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?

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

Labels

effort/M Under 500 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