Repository navigation
cmux-next agent pane: place drawn rows by their drawn height - #16491
Merged
Merged
Conversation
Red first. 300 seeded random rows of every kind (markdown messages,
activity, permission cards, summaries) draw at heights the estimator
can't know. A mounted row must end at or above the next row's top, at
the latest rows and after scrolls to the top and the middle. Today the
layout places rows only by estimate, so they overlap ("r292 ends at
30505, r293 starts at 30444").
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
The transcript estimates a row's height only for its first layout. - Each mounted row (RowFrame) reports its border-box height before the frame paints: when it mounts, and when its content version, the row width or its expansion changes. One shared ResizeObserver reports later resizes. - A commit's reports land in one state update. A drawn height counts while the row's version and width match; zero means not laid out. - The row at the viewport's top stays in place as rows above it settle. A view opened at the latest row stays at the bottom. - Permission cards, activity rows and custom renderers use the same path, and MeasuredCustomRow is gone. Row gaps move into CSS (border-box padding: 16px under messages, 8px under other rows), so drawn and estimated heights mean the same thing. Fallback estimates follow: permission cards 87px (they were 44px against ~77px drawn), summaries and notices 37px, collapsed activity 34px. The caches shrink as they go: drawn heights and prepared text are dropped for rows that leave the transcript, and a streaming row's prepared text is cleared past 64 versions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The fake viewport now clamps scrollTop to the content like a browser. Red: opened at latest, 300 rows drawn shorter than estimated end at scrollTop 63024 against a maximum of 64104. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
Rows that settle shorter shrink the spacer, and the browser clamps scrollTop before the layout effect reads it. The stick-to-latest check then failed, and the anchor branch subtracted the shift a second time. The effect now uses the offset and at-latest state recorded at the last scroll or commit whenever the live offset shows that clamp. Also, ResizeObserver reports flush synchronously, so a late size change paints no frame of overlap, and they find their row by aria-posinset instead of a linear search. Green: bun test src/agent-session/ passes 142 of 142. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="webviews/src/agent-session/acpmux/App.tsx">
<violation number="1" location="webviews/src/agent-session/acpmux/App.tsx:165">
P2: The drawn-height cache has no session identity: switching to a same-sized session can recreate IDs such as `user-1` at version 1 and retain the prior session's heights. Invalidate drawn heights on session changes or include a session/generation key in the cache validity check.</violation>
</file>
<file name="webviews/src/agent-session/acpmux/model.ts">
<violation number="1" location="webviews/src/agent-session/acpmux/model.ts:184">
P2: Once a streaming row passes 64 versions, every later version clears the entire `prepared` map and re-measures all blocks from scratch. Streaming rows change `text` on every chunk, so the `entry.text !== row.text` branch runs each version and the `prepared.size > 64` check stays true; the cap therefore destroys exactly the reuse it is meant to preserve — the first blocks of a long assistant message keep identical text strings across versions and would otherwise be reused from the cache. For a stream extending past ~64 chunks (common for agent responses, re-laid out each version), this turns every frame's layout into a full re-preparation of the message. Evict only entries not referenced by the current `entry.blocks` (or least-recently-used entries) instead of clearing the whole map.</violation>
</file>
<file name="webviews/src/agent-session/acpmux/transcript.test.tsx">
<violation number="1" location="webviews/src/agent-session/acpmux/transcript.test.tsx:257">
P3: This test fires every callback in the module-level `resizeCallbacks` array for one row-resize event. That array accumulates the scroll container's viewport ResizeObserver as well as the row observers of every component earlier tests mounted (the stub's `disconnect`/`unobserve` never remove callbacks), so the fired event reaches observers this test never created and its assertions depend on React's no-op-on-unmounted and identical-value-bailout semantics. The sibling "a height-only shrink" test isolates first with `resizeCallbacks.length = 0;`; do the same at the start of this test, or record the array index after mount and fire only the callbacks registered since.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Forget rows that left the transcript (a session switch, older history unloaded). | ||
| useEffect(() => { | ||
| const cache = measurementCache.current; | ||
| if (cache.size <= rows.length && drawn.size <= rows.length) return; |
There was a problem hiding this comment.
P2: The drawn-height cache has no session identity: switching to a same-sized session can recreate IDs such as user-1 at version 1 and retain the prior session's heights. Invalidate drawn heights on session changes or include a session/generation key in the cache validity check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/App.tsx, line 161:
<comment>The drawn-height cache has no session identity: switching to a same-sized session can recreate IDs such as `user-1` at version 1 and retain the prior session's heights. Invalidate drawn heights on session changes or include a session/generation key in the cache validity check.</comment>
<file context>
@@ -111,7 +119,54 @@ export function VirtualTranscript({ rows, onToggleActivity, expanded, registry =
+ // Forget rows that left the transcript (a session switch, older history unloaded).
+ useEffect(() => {
+ const cache = measurementCache.current;
+ if (cache.size <= rows.length && drawn.size <= rows.length) return;
+ const ids = new Set(rows.map((row) => row.id));
+ for (const id of cache.keys()) if (!ids.has(id)) cache.delete(id);
</file context>
| } else if (entry.text !== row.text) { | ||
| entry.text = row.text; | ||
| entry.blocks = markdownBlocks(row.text); | ||
| // A streaming row prepares a new last block on every version; keep the cache bounded. |
There was a problem hiding this comment.
P2: Once a streaming row passes 64 versions, every later version clears the entire prepared map and re-measures all blocks from scratch. Streaming rows change text on every chunk, so the entry.text !== row.text branch runs each version and the prepared.size > 64 check stays true; the cap therefore destroys exactly the reuse it is meant to preserve — the first blocks of a long assistant message keep identical text strings across versions and would otherwise be reused from the cache. For a stream extending past ~64 chunks (common for agent responses, re-laid out each version), this turns every frame's layout into a full re-preparation of the message. Evict only entries not referenced by the current entry.blocks (or least-recently-used entries) instead of clearing the whole map.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/model.ts, line 184:
<comment>Once a streaming row passes 64 versions, every later version clears the entire `prepared` map and re-measures all blocks from scratch. Streaming rows change `text` on every chunk, so the `entry.text !== row.text` branch runs each version and the `prepared.size > 64` check stays true; the cap therefore destroys exactly the reuse it is meant to preserve — the first blocks of a long assistant message keep identical text strings across versions and would otherwise be reused from the cache. For a stream extending past ~64 chunks (common for agent responses, re-laid out each version), this turns every frame's layout into a full re-preparation of the message. Evict only entries not referenced by the current `entry.blocks` (or least-recently-used entries) instead of clearing the whole map.</comment>
<file context>
@@ -173,6 +181,8 @@ function measuredRowHeight(row: AcpmuxRow, width: number, cache: Map<string, Pre
} else if (entry.text !== row.text) {
entry.text = row.text;
entry.blocks = markdownBlocks(row.text);
+ // A streaming row prepares a new last block on every version; keep the cache bounded.
+ if (entry.prepared.size > 64) entry.prepared.clear();
}
</file context>
| const offset = scroller.scrollTop - anchor.top; | ||
| const above = placed().filter((row) => row.index < anchor.index).sort((a, b) => a.index - b.index)[0]!; | ||
| drawn.set(conversation[above.index]!.id, drawn.get(conversation[above.index]!.id)! + 100); | ||
| await act(async () => { for (const callback of resizeCallbacks) (callback as (entries: { target: Element }[]) => void)([{ target: above.article }]); }); |
There was a problem hiding this comment.
P3: This test fires every callback in the module-level resizeCallbacks array for one row-resize event. That array accumulates the scroll container's viewport ResizeObserver as well as the row observers of every component earlier tests mounted (the stub's disconnect/unobserve never remove callbacks), so the fired event reaches observers this test never created and its assertions depend on React's no-op-on-unmounted and identical-value-bailout semantics. The sibling "a height-only shrink" test isolates first with resizeCallbacks.length = 0;; do the same at the start of this test, or record the array index after mount and fire only the callbacks registered since.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/transcript.test.tsx, line 255:
<comment>This test fires every callback in the module-level `resizeCallbacks` array for one row-resize event. That array accumulates the scroll container's viewport ResizeObserver as well as the row observers of every component earlier tests mounted (the stub's `disconnect`/`unobserve` never remove callbacks), so the fired event reaches observers this test never created and its assertions depend on React's no-op-on-unmounted and identical-value-bailout semantics. The sibling "a height-only shrink" test isolates first with `resizeCallbacks.length = 0;`; do the same at the start of this test, or record the array index after mount and fire only the callbacks registered since.</comment>
<file context>
@@ -182,6 +182,88 @@ describe("acpmux transcript accessibility", () => {
+ const offset = scroller.scrollTop - anchor.top;
+ const above = placed().filter((row) => row.index < anchor.index).sort((a, b) => a.index - b.index)[0]!;
+ drawn.set(conversation[above.index]!.id, drawn.get(conversation[above.index]!.id)! + 100);
+ await act(async () => { for (const callback of resizeCallbacks) (callback as (entries: { target: Element }[]) => void)([{ target: above.article }]); });
+ expect(atTop().index).toBe(anchor.index);
+ expect(scroller.scrollTop - atTop().top).toBe(offset);
</file context>
…sured-rows # Conflicts: # Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/Resources/agent-pane/index.html # webviews/src/agent-session/acpmux/App.tsx
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="webviews/src/agent-session/acpmux/App.tsx">
<violation number="1" location="webviews/src/agent-session/acpmux/App.tsx:186">
P2: Replacing a row renderer leaves its previous drawn height authoritative because `drawn` is checked before the new registry measurement. Invalidate drawn heights when the row registry changes, or associate each height with a renderer generation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const layout = layoutConversation(rows, transcriptRowWidth(width), measurementCache.current, (row, rowWidth) => registry[rowKind(row)]?.measure?.(row, rowWidth) ?? measuredHeights.get(row.id)); | ||
| const layout = layoutConversation(rows, transcriptRowWidth(width), measurementCache.current, (row, rowWidth) => { | ||
| const known = drawn.get(row.id); | ||
| if (known && known.version === row.version && known.width === rowWidth) return known.height; |
There was a problem hiding this comment.
P2: Replacing a row renderer leaves its previous drawn height authoritative because drawn is checked before the new registry measurement. Invalidate drawn heights when the row registry changes, or associate each height with a renderer generation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/App.tsx, line 185:
<comment>Replacing a row renderer leaves its previous drawn height authoritative because `drawn` is checked before the new registry measurement. Invalidate drawn heights when the row registry changes, or associate each height with a renderer generation.</comment>
<file context>
@@ -111,18 +122,71 @@ export function VirtualTranscript({ rows, onToggleActivity, expanded, registry =
- const layout = layoutConversation(rows, transcriptRowWidth(width), measurementCache.current, (row, rowWidth) => registry[rowKind(row)]?.measure?.(row, rowWidth) ?? measuredHeights.get(row.id));
+ const layout = layoutConversation(rows, transcriptRowWidth(width), measurementCache.current, (row, rowWidth) => {
+ const known = drawn.get(row.id);
+ if (known && known.version === row.version && known.width === rowWidth) return known.height;
+ return registry[rowKind(row)]?.measure?.(row, rowWidth);
+ });
</file context>
…them On the CI mini, the first fling through 5000 rows dropped 3 frames (max 46 ms, layout p95 4 ms) against 0 before measured rows, because each newly drawn row re-ran the full layout. Red: one report measures 199 rows again (399 measures against 200). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
…ing again The transcript keeps the estimated layout memoized on rows, width and registry. Drawn heights are applied in a separate prefix-sum pass, so a row that draws during a fling no longer re-measures every row. Green: bun test src/agent-session/ passes 145 of 145. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="webviews/src/agent-session/acpmux/model.ts">
<violation number="1" location="webviews/src/agent-session/acpmux/model.ts:218">
P2: `placeRows` accepts any positive value from `heightAt`, including `Infinity`. A non-finite height then propagates: every subsequent `tops` entry and `totalHeight` becomes `Infinity`, `upperBound` in `visibleLayoutRange` returns the array end for every boundary after that row, and the spacer's `height: Infinity` is invalid CSS, collapsing the virtualized list. The sibling path `layoutConversation` guards the same kind of input with `Number.isFinite(customHeight)`. Add the same check so a bad known height falls back to the estimate.</violation>
</file>
<file name="webviews/src/agent-session/acpmux/transcript.test.tsx">
<violation number="1" location="webviews/src/agent-session/acpmux/transcript.test.tsx:289">
P3: This loop fires every callback in the module-wide `resizeCallbacks` array, which accumulates observers from every earlier test (the array is only cleared in the "height-only shrink" test). The stale callbacks from unmounted trees run against this test's `latest` row, making the test depend on prior tests unmounting cleanly and on dead components' handlers never throwing or mutating live state. Clear `resizeCallbacks.length = 0;` before `root.render(...)` so only this test's observer fires, matching the precedent already in this file.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| for (let index = 0; index < heights.length; index += 1) { | ||
| tops[index] = top; | ||
| const known = heightAt(index); | ||
| const height = known !== undefined && known > 0 ? known : estimate.heights[index]; |
There was a problem hiding this comment.
P2: placeRows accepts any positive value from heightAt, including Infinity. A non-finite height then propagates: every subsequent tops entry and totalHeight becomes Infinity, upperBound in visibleLayoutRange returns the array end for every boundary after that row, and the spacer's height: Infinity is invalid CSS, collapsing the virtualized list. The sibling path layoutConversation guards the same kind of input with Number.isFinite(customHeight). Add the same check so a bad known height falls back to the estimate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/model.ts, line 218:
<comment>`placeRows` accepts any positive value from `heightAt`, including `Infinity`. A non-finite height then propagates: every subsequent `tops` entry and `totalHeight` becomes `Infinity`, `upperBound` in `visibleLayoutRange` returns the array end for every boundary after that row, and the spacer's `height: Infinity` is invalid CSS, collapsing the virtualized list. The sibling path `layoutConversation` guards the same kind of input with `Number.isFinite(customHeight)`. Add the same check so a bad known height falls back to the estimate.</comment>
<file context>
@@ -206,6 +206,22 @@ export function layoutConversation(rows: AcpmuxRow[], width: number, cache = new
+ for (let index = 0; index < heights.length; index += 1) {
+ tops[index] = top;
+ const known = heightAt(index);
+ const height = known !== undefined && known > 0 ? known : estimate.heights[index];
+ heights[index] = height;
+ top += height;
</file context>
Suggested change
| const height = known !== undefined && known > 0 ? known : estimate.heights[index]; | |
| const height = known !== undefined && Number.isFinite(known) && known > 0 ? known : estimate.heights[index]; |
| const afterOpen = measures; | ||
| drawnHeight = 90; | ||
| const latest = dom.window.document.querySelector<HTMLElement>('.acpmux-row[aria-posinset="200"]')!; | ||
| await act(async () => { for (const callback of resizeCallbacks) (callback as (entries: { target: Element }[]) => void)([{ target: latest }]); }); |
There was a problem hiding this comment.
P3: This loop fires every callback in the module-wide resizeCallbacks array, which accumulates observers from every earlier test (the array is only cleared in the "height-only shrink" test). The stale callbacks from unmounted trees run against this test's latest row, making the test depend on prior tests unmounting cleanly and on dead components' handlers never throwing or mutating live state. Clear resizeCallbacks.length = 0; before root.render(...) so only this test's observer fires, matching the precedent already in this file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/transcript.test.tsx, line 289:
<comment>This loop fires every callback in the module-wide `resizeCallbacks` array, which accumulates observers from every earlier test (the array is only cleared in the "height-only shrink" test). The stale callbacks from unmounted trees run against this test's `latest` row, making the test depend on prior tests unmounting cleanly and on dead components' handlers never throwing or mutating live state. Clear `resizeCallbacks.length = 0;` before `root.render(...)` so only this test's observer fires, matching the precedent already in this file.</comment>
<file context>
@@ -264,6 +264,38 @@ describe("acpmux measured rows", () => {
+ const afterOpen = measures;
+ drawnHeight = 90;
+ const latest = dom.window.document.querySelector<HTMLElement>('.acpmux-row[aria-posinset="200"]')!;
+ await act(async () => { for (const callback of resizeCallbacks) (callback as (entries: { target: Element }[]) => void)([{ target: latest }]); });
+ expect(parseFloat(spacer.style.height)).toBe(estimated + 40);
+ expect(measures).toBe(afterOpen);
</file context>
Collaborator
Author
|
Subagent review at 58ecdae: LGTM.
Fling bench (5000 mock rows, 3 flings, CI mini M4 at 60 Hz):
On a 120 Hz virtual display (CGVirtualDisplay on the same mini), 58ecdae runs at nominal 8 ms, p50 8 ms, about 360 frames per fling. The first fling drops 11 frames and later flings drop 1 to 3. A screenshot on the mini shows lists, code, summaries and bubbles with no overlap. CI: everything passes except |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follows #16476, per #16424. Four review rounds on #16476 kept finding new message shapes the estimator measured short, so estimating alone wasn't going to converge. With this change, a row the page has drawn is placed by its drawn height, and the estimate only serves the first layout of rows not yet drawn.
Change (page only).
RowFrame) reports its border-box height before the frame paints: when it mounts, and when its content version, the row width or its expansion changes. One sharedResizeObservercatches later resizes, such as a font that loads or a custom renderer that grows. A commit's reports land in one state update.MeasuredCustomRow.Testing
bun test src/agent-session/. The new test "drawn rows never overlap, whatever their shape" lays out 300 seeded random rows of every kind, drawn at heights the estimator can't know:651557a761c:r292 ends at 30505, r293 starts at 30444, and more.be647a0e721: 141 pass.tscis clean andbuild-agent-pane-web.sh --checkreports the bundle is current. A fleet-build visual check and the fling bench are to follow (see comments).Changelog
🤖 Generated with Claude Code
https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Places transcript rows by the height they were drawn at as soon as they're drawn, instead of the estimate, so permission cards and expanded tool activity no longer overlap the next message. The estimate now only positions rows before their first paint.
ResizeObservercatches later resizes like a loaded font and flushes synchronously so no frame shows overlap.MeasuredCustomRow.Written for commit 58ecdae. Summary will update on new commits.