perf(ui): scope prompt rail observation to mounted turns - #4203
Conversation
ARE404
left a comment
There was a problem hiding this comment.
Approving perf(ui): scope prompt rail observation to mounted turns at head 6476be8e.
I reviewed the single-file change and verified the scoped observation is coverage-preserving:
- In the virtualized DOM,
[data-virtual-turn-id]is set on.maka-turn-virtual-item(chat-view.tsx:734), somountedTurnListresolves to the.maka-chat-message-listthat hosts all mounted turns; every.maka-turn-virtual-itemand its[data-turn-id]descendants remain inside it. Scoping the IntersectionObserver iteration and the MutationObserver to this container therefore observes the same turn set as before. - Turn mount/unmount is a direct childList change on the message list under virtualization, so
childListwithoutsubtreestill catches it. One [P3] note on this is inline (prompt-anchor-rail.tsx:394). - No P0–P2; 0 unresolved review threads.
Note on CI: the test check was still settling on this exact head at review time (run 33264030950, in progress); I'm approving on the code/findings as requested, and the prompt-rail e2e covers the mounted/virtualized path.
简体中文
批准 perf(ui): scope prompt rail observation to mounted turns,head 6476be8e。
审查单文件改动,确认收缩观察范围不丢覆盖:虚拟化 DOM 里 data-virtual-turn-id 设在 .maka-turn-virtual-item(chat-view.tsx:734),所以 mountedTurnList 解析到承载所有挂载 turn 的 .maka-chat-message-list,每个 .maka-turn-virtual-item 及其 [data-turn-id] 后代仍在其内——IntersectionObserver 迭代和 MutationObserver 都收缩到这个容器,观察的 turn 集合与改前一致。虚拟化下 turn 的挂载/卸载是对消息列表的直接 childList 变化,所以 childList(无 subtree)仍能捕获。行内一条 [P3](prompt-anchor-rail.tsx:394)。无 P0–P2;0 未解决线程。CI 备注:approve 时 test 仍在 exact head 上跑(run 33264030950 进行中),我基于代码/结论批准,prompt-rail e2e 覆盖了挂载/虚拟化路径。
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head 6476be8e (verified unchanged at review time).
Four-line observer scoping, verified rather than assumed:
- The structural assumption holds in the real DOM:
chat-view.tsxrenders every[data-virtual-turn-id]wrapper as a flat sibling (alongside the virtual spacers) under one scroll-content parent, soquerySelector('[data-virtual-turn-id]').parentElementis the single element whose direct childList changes exactly when mounted-Turn membership changes. The membership bookkeeping (visitTurnElementsover added/removed nodes) still finds the nested[data-turn-id]elements inside each added wrapper, so observe/unobserve coverage is unchanged. - The eliminated wakeups were pure waste: with
subtree: trueon the scroll root, every streaming Markdown/math/Mermaid mutation anywhere in the transcript fired the callback, which then re-scanned and re-resolved for no membership change — matching the measured 152–153 callbacks and ~300–450 ms of CPU per stream, now 0. - Mount ordering is safe: the effect re-runs when
turns/railTurns/turnIndexByIdchange, andmountedTurnsare mapped in the same React commit, so the wrapper nodes exist when the effect queries for them; the empty→non-empty transition re-runs the effect the same way. Spacer mounts/unmounts still fire the observer, preserving the old settle behavior at a tiny fraction of the old rate. - Scope discipline: the change touches only the observation target — transcript range/virtualizer, streaming preprocessing, Selection Quote, Mermaid, and scroll ownership are all untouched, and the rail's own arithmetic (railTurns mapping, jump-hold, safe-area) is unchanged.
The performance evidence is methodologically sound: frozen A/B production renderers at the same SHA, seven interleaved pairs, a predeclared primary metric and acceptance gate, and guardrail metrics reported alongside (long-scroll membership CPU within 2%, frame intervals unchanged). The result (7/7 wins, 100% reduction of the primary metric) is consistent with what the diff predicts.
Executed on a real Windows machine at this head: clean forced rebuild, prompt-anchor-rail unit suite 9/9. The hosted test job (which includes the prompt-rail Playwright spec) was still running at approval time.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
简体中文
四行观察范围收窄,全部核实而非假设:chat-view 里所有 [data-virtual-turn-id] 包装节点确实是同一滚动内容父元素下的平铺兄弟节点,所以「第一个 turn 节点的 parent」就是挂载 Turn 成员变化的精确观察点;visitTurnElements 仍能在新增包装节点里找到嵌套的 [data-turn-id],观察覆盖不变。被消灭的唤醒全是浪费:旧 subtree:true 下流式 Markdown/math/Mermaid 的每次变更都触发回调白扫一遍(与实测 152-153 次回调、300-450ms CPU 吻合,现为 0)。挂载时序安全:effect 依赖 turns 变化重跑时节点已提交。范围纪律:虚拟化/流式预处理/Selection Quote/Mermaid/滚动所有权全未动。性能证据方法学扎实(同 SHA 冻结 A/B、七次交错、预先声明的指标与门槛)。本机真 Windows 干净重建 + prompt-anchor-rail 单测 9/9;CI test(含 prompt-rail e2e)批时仍在跑。
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 6476be8e6d388613225b892902c314b6cec90b0e.
No P0-P3 findings. The change narrows the initial turn scan and membership observer from the full scroll subtree to the Astryx message-list inner container. I verified that chat-view.tsx renders each keyed [data-virtual-turn-id] wrapper as a direct child of that container and that each wrapper contains its [data-turn-id] section, so direct childList additions/removals still drive the existing observe/unobserve bookkeeping while streaming Markdown, math, and Mermaid descendant churn no longer wakes it. The current main merge tree is conflict-free and preserves this DOM contract.
Validation on this head: @maka/ui build and test suite passed (269 tests), desktop production build passed, Prompt Rail Playwright E2E passed (7/7 under Xvfb), Biome passed for the changed file, and git diff --check passed. No production code or tests should be deleted, and no deeper refactor is required. No protected-area effect was identified in this UI-only diff.
Residual gap: the hosted test check is still running, and I did not independently reproduce the PR's seven-pair CPU benchmark; the reviewed code path and local behavioral coverage are consistent with the claimed optimization. This revision is ready from code review, subject to the repository's required checks and independent human review.
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.
jackwener
left a comment
There was a problem hiding this comment.
I reviewed this at 6476be8e6d388613225b892902c314b6cec90b0e. No P0, P1, P2, or P3.
The Prompt Rail observer used to watch the whole chat scroll subtree, so streaming Markdown, math, and Mermaid mutations woke it even when mounted Turn membership had not changed. This keeps the same IntersectionObserver and add/remove bookkeeping, and attaches the MutationObserver to the parent of the keyed [data-virtual-turn-id] nodes with childList only. Added wrappers still carry their nested [data-turn-id] section, so visitTurnElements still finds them. I found no leftover subtree: true path on this observer, and the change does not touch scroll ownership, Selection Quote, or the virtualizer.
The rail effect still depends on railTurns, which is already identity-stable during streaming, so this does not rebuild the observer on token updates. ChatView mounts those keyed Turn wrappers as siblings of the virtual spacers; the first [data-virtual-turn-id] parent is that list, not a per-Turn shell.
Hosted test is red on this head because three WorkHub e2e cases failed (workhub-layout, two workhub-reconstruction). Those suites do not exercise this file. The rest of Desktop e2e passed, including the Prompt Rail cases. I am not treating that red as a finding on this change. The merge tree against current main is clean. This is a performance change; I am not merging it.
简体中文
我审的是 6476be8e6d388613225b892902c314b6cec90b0e。没有 P0/P1/P2/P3。
Prompt Rail 的 MutationObserver 以前盯整棵滚动子树,流式 Markdown / 公式 / Mermaid 的内部变动也会叫醒它。现在只盯 keyed [data-virtual-turn-id] 节点的父节点,而且只看 childList。新挂上的 Turn 包装节点里已经带着 [data-turn-id],原有的 add/remove 扫描仍然找得到。滚动所有权、划词引用、virtualizer 都没动。
hosted test 红在三条 WorkHub e2e,不经过这个文件。Prompt Rail 相关 e2e 过了。这是性能改动,我不合入。
Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.
Keep Prompt Rail membership observation on the existing keyed Turn list instead of waking for arbitrary Markdown subtree mutations. Generated-by: Codex
6476be8 to
18798c6
Compare
Summary
Prompt Anchor Rail used to observe the entire chat scroll subtree for
childListchanges. Streaming Markdown, math, and Mermaid work therefore woke the rail even when the mounted Turn membership had not changed.This change keeps the existing observer and incremental membership logic, but attaches it to the direct parent of the existing keyed
[data-virtual-turn-id]nodes and scans only that mounted Turn list. It does not change the transcript range/virtualizer, streaming preprocessing, Selection Quote, Mermaid, or scroll ownership.The performance study used the shared experimental baseline
2fa1f78ec5746ed4c2db5c49d38987a34ae9122f. This branch is now rebased ontomainat593efb9df79d66ad0dad8033884042dbffd754fe, which includes the independent WorkHub CI stabilization in #4210. No interveningmaincommit changedpackages/ui/src/prompt-anchor-rail.tsx, so the measured candidate diff and its natural owner are unchanged.Performance evidence
Method and declared gate
Primary raw pairs
Median / MAD: 401.4 / 43.4 ms -> 0.0 / 0.0 ms, a 100% reduction with 7/7 paired wins. The lower edge of baseline median minus MAD was still 358.0 ms, while every candidate run had zero Prompt Rail wakeups, so the result is outside measured noise.
Matrix attribution and guardrails
Exploratory attribution selected Prompt Rail as the single coherent owner: its broad subtree MutationObserver dominated real streaming and rich-content mutation CPU across the matrix. Selection Quote's document scroll handler dominated only the active-selection scenario, while Mermaid theme/resize callbacks were small. No Selection Quote or Mermaid change is bundled here.
The long-scroll membership path remains active (40 -> 39 median callbacks) because real mounted Turn additions/removals still need rail bookkeeping; the removed work is specifically unrelated descendant churn. The candidate did not shift that cost into Selection Quote or the virtualizer.
For Mermaid attribution, an inspected candidate run sampled 12.42 ms of Mermaid/Markdown/Dagre self CPU during four-diagram rendering, 3.86 ms during theme toggle, and 2.52 ms during resize. Each diagram's viewport ResizeObserver fired six times during resize with effectively 0.00 ms instrumented callback CPU; paired wall medians were unchanged. Mermaid was therefore not the dominant owner.
All 14 formal runs preserved the product contracts: quote visible before and hidden after scroll, streaming tail distance 0, one mounted streamed Turn, four Mermaid diagrams producing 32 nested SVG nodes, theme rerender observed, and four layouts ready after resize. No key regression exceeded 10%; the largest measured regression was 1.49% in Selection Quote callback CPU.
Entropy ledger
idByElement,visibleAdded concepts: none.
mountedTurnListis a local reference to the existing React/virtualizer keyed Turn membership seam, not state, a cache, a protocol, or a scheduler. It is necessary to name the natural subscription owner.Deleted concepts/paths: arbitrary Markdown/math/Mermaid/other descendant mutation wakeups and root-wide initial Turn scans. The replaced
subtree: truepath is deleted in this same diff; there is no compatibility path or follow-up cleanup. Authority, state, representation, and scheduler counts do not increase, while the observer's semantic reach decreases, so the slice is net entropy-reducing despite the one-line LOC increase.The required one-way architecture remains intact: Runtime Host owns transcript facts; Desktop range remains an immutable bounded projection; React supplies stable Turn identity;
useChatScrollremains the only scroll authority; Astryx remains the streaming scheduler; Chromium retains offscreen layout/paint ownership; the CDP harness remains measurement-only.Verification
npm run build— production build passed.npm --workspace @maka/ui run test— 269 passed.cd apps/desktop && npx playwright test --config e2e/playwright.config.ts e2e/prompt-rail.spec.ts— 7 passed on the final candidate.test.fixmeskip (a drag begun while the answer streams still offers a quote, documented upstream Astryx node-rebuild limitation).npx @biomejs/biome check packages/ui/src/prompt-anchor-rail.tsx— passed.git diff --check— passed.AI use
Select exactly one:
Tool(s) and scope: Codex performed the CDP attribution study, implemented the scoped observer change, ran the paired A/B analysis, and prepared this evidence. The commit includes a
Generated-by: Codextrailer.Checklist
The read-only performance acceptance harness fails the declared primary gate on the frozen baseline and passes it on the candidate; existing product E2E covers Prompt Rail behavior, scroll anchoring, quote selection, theme/resize, and accessibility.
Does this PR entail a change in behavior?