Repository navigation
cmux-next agent pane: no blank edge while flinging the transcript - #16454
Conversation
…ry gaps Covers the #16424 follow-ups: lag resync dropping the live permission, a failed lag fetch stuck on "resyncing", a reconnect that switches the missing selection without bumping the generation, the event gap after a reconnect re-attach, a watch lag dropping the selected session, close() leaving requests pending, failed prompt rows lost on rebuild, the open-at-latest jump on a height-only shrink, and register() re-rendering for an unchanged renderer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
- Lag resync keeps the live summary, queue and permission through the rebuild (shared with loadOlder) and replays the missed mux events on top, so a missed permission decision or status still lands. - A failed lag fetch ends "resyncing" with "failed", or "disconnected" when the socket is gone. - A connect that finds the selected session missing bumps the selection generation, so a lag notice before the new attach waits for it. - A reconnect re-attach of the same session fetches the events between the old cursor and the attach page. - A watch-lag refresh that no longer lists the selected session falls back to the most recent one, like a purge. - close() rejects pending requests; onclose ignores a released socket. - rebuild() keeps failed optimistic prompt rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
… no-op registers The open-at-latest effect also runs when the viewport height changes, so rows that fit and then overflow after a vertical shrink still jump to the latest row once. register() returns early when the kind already has that renderer and measure, so a user renderer registering from an effect does not loop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
… a scroll paints VirtualTranscript commits a scroll one animation frame after the scroll event and mounts only the viewport plus four rows of overscan. In the #16231 fling bench every frame showed a blank edge. The test scrolls up a viewport and a half per event and expects the rows two steps ahead to be mounted synchronously; it fails with the rows still at the old offset (min mounted top 8008, expected <= 5500). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYQHfug1ZVQDp4eWgwVUtD
The scroll handler now commits the offset with flushSync from the scroll event instead of a later animation frame, and the mounted range leads in the scroll direction by two scroll steps (capped at four viewports), so the rows a fling reaches next are already in the DOM. 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 |
|
Subagent review at 3b0dc6c: APPROVE.
Non-blocking, added to #16424:
|
…ext-agent-pane-scroll-lead # Conflicts: # Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/Resources/agent-pane/index.html # webviews/src/agent-session/acpmux/App.tsx # webviews/src/agent-session/acpmux/transcript.test.tsx
There was a problem hiding this comment.
3 issues found across 5 files
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:128">
P2: `scroll.delta` retains the last fling indefinitely, so later transcript updates keep mounting up to four extra viewports even after scrolling stops. Clear the directional lead after the scroll commit or scope it to the scroll render so virtualization returns to the normal range.</violation>
</file>
<file name="webviews/src/agent-session/acpmux/direct.ts">
<violation number="1" location="webviews/src/agent-session/acpmux/direct.ts:167">
P2: The reconnect path pages `_acpmux/events` past the attach snapshot, but with `replayLiveState=false` the fetched mux events' effects (queue, status, permission) are discarded: `rebuildKeepingLiveState(undefined)` restores the pre-fetch `summary`/`queue`/`pendingPermission` and replays nothing, and `apply()` then drops the eventStream notification for the same seq because `event.seq <= this.lastSeq`. Any `queued`/`queue_removed`/`status`/`permission_*` event that lands between the attach reply and the end of the fetch (the fetch pages to *now*, not to the attach page) is permanently lost from the live state, leaving a stale queue chip or status until an unrelated later event changes it. Pass `replayLiveState` as true (the lag path's behavior) so events after `resumeAfter` are replayed onto the attach snapshot in seq order; the guard `oldest > resumeAfter + 1` is computed only from transcript seqs, so it does not limit the mux events this fetch returns.</violation>
<violation number="2" location="webviews/src/agent-session/acpmux/direct.ts:277">
P2: This branch still publishes a stale session list when selection changes while `_acpmux/watch` is pending. Discard the response (or merge the current selection) before replacing `this.sessions`, otherwise the selected session can disappear from the picker until another watch update arrives.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Unanchored findings
2 findings could not be posted inline, so cubic included the feedback here instead.
1. webviews/src/agent-session/acpmux/direct.ts:277
P2: This branch still publishes a stale session list when selection changes while _acpmux/watch is pending. Discard the response (or merge the current selection) before replacing this.sessions, otherwise the selected session can disappear from the picker until another watch update arrives.
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/direct.ts, line 277:
<comment>This branch still publishes a stale session list when selection changes while `_acpmux/watch` is pending. Discard the response (or merge the current selection) before replacing `this.sessions`, otherwise the selected session can disappear from the picker until another watch update arrives.</comment>
<file context>
@@ -236,28 +244,46 @@ export class AcpmuxDirectClient {
- this.emit("session changed");
+ const missing = this.selectedSessionId !== undefined && !this.sessions.some((session) => session.sessionId === this.selectedSessionId);
+ if (missing && generation === this.selectionGeneration) this.selectFallbackSession("session changed");
+ else this.emit("session changed");
+ }
+
</file context>
2. webviews/src/agent-session/acpmux/direct.ts:167
P2: The reconnect path pages _acpmux/events past the attach snapshot, but with replayLiveState=false the fetched mux events' effects (queue, status, permission) are discarded: rebuildKeepingLiveState(undefined) restores the pre-fetch summary/queue/pendingPermission and replays nothing, and apply() then drops the eventStream notification for the same seq because event.seq <= this.lastSeq. Any queued/queue_removed/status/permission_* event that lands between the attach reply and the end of the fetch (the fetch pages to now, not to the attach page) is permanently lost from the live state, leaving a stale queue chip or status until an unrelated later event changes it. Pass replayLiveState as true (the lag path's behavior) so events after resumeAfter are replayed onto the attach snapshot in seq order; the guard oldest > resumeAfter + 1 is computed only from transcript seqs, so it does not limit the mux events this fetch returns.
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/direct.ts, line 167:
<comment>The reconnect path pages `_acpmux/events` past the attach snapshot, but with `replayLiveState=false` the fetched mux events' effects (queue, status, permission) are discarded: `rebuildKeepingLiveState(undefined)` restores the pre-fetch `summary`/`queue`/`pendingPermission` and replays nothing, and `apply()` then drops the eventStream notification for the same seq because `event.seq <= this.lastSeq`. Any `queued`/`queue_removed`/`status`/`permission_*` event that lands between the attach reply and the end of the fetch (the fetch pages to *now*, not to the attach page) is permanently lost from the live state, leaving a stale queue chip or status until an unrelated later event changes it. Pass `replayLiveState` as true (the lag path's behavior) so events after `resumeAfter` are replayed onto the attach snapshot in seq order; the guard `oldest > resumeAfter + 1` is computed only from transcript seqs, so it does not limit the mux events this fetch returns.</comment>
<file context>
@@ -154,10 +153,19 @@ export class AcpmuxDirectClient {
+ if (sessionId) {
+ const page = await this.attach(sessionId, generation);
+ const oldest = page.length > 0 ? Math.min(...page.map((event) => event.seq)) : 0;
+ if (resumeAfter > 0 && oldest > resumeAfter + 1) await this.fetchMissedEvents(sessionId, generation, resumeAfter, false);
+ }
this.hasConnected = true;
</file context>
if (resumeAfter > 0 && oldest > resumeAfter + 1) await this.fetchMissedEvents(sessionId, generation, resumeAfter);
| const layout = measured.layout; | ||
| const reportedLayout = useRef<typeof measured | null>(null); | ||
| const range = visibleLayoutRange(layout, scrollTop, height); | ||
| const lead = Math.min(Math.abs(scroll.delta) * SCROLL_LEAD_STEPS, height * MAX_SCROLL_LEAD_VIEWPORTS); |
There was a problem hiding this comment.
P2: scroll.delta retains the last fling indefinitely, so later transcript updates keep mounting up to four extra viewports even after scrolling stops. Clear the directional lead after the scroll commit or scope it to the scroll render so virtualization returns to the normal range.
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 128:
<comment>`scroll.delta` retains the last fling indefinitely, so later transcript updates keep mounting up to four extra viewports even after scrolling stops. Clear the directional lead after the scroll commit or scope it to the scroll render so virtualization returns to the normal range.</comment>
<file context>
@@ -118,7 +125,8 @@ export function VirtualTranscript({ rows, onToggleActivity, expanded, registry =
const layout = measured.layout;
const reportedLayout = useRef<typeof measured | null>(null);
- const range = visibleLayoutRange(layout, scrollTop, height);
+ const lead = Math.min(Math.abs(scroll.delta) * SCROLL_LEAD_STEPS, height * MAX_SCROLL_LEAD_VIEWPORTS);
+ const range = visibleLayoutRange(layout, scroll.delta < 0 ? scroll.top - lead : scroll.top, height + lead);
useLayoutEffect(() => {
</file context>
|
Merging. 3d9a69c is the approved head 3b0dc6c with feat-cmux-next (#16452) merged in. The conflicts were only this PR's own additions against base context plus the generated bundle, rebuilt with |
Summary
While you flung the React agent pane's transcript, every frame showed a blank strip at the edge. In the #16231 bench, all 240 of 240 frames were blank. Two things caused it:
VirtualTranscriptcommitted a scroll one animation frame after the scroll event.Now the scroll handler commits from the scroll event with
flushSync. The mounted range also leads in the scroll direction by two scroll steps, capped at four viewports, so the rows a fling reaches next are already in the DOM. When scrolling stops, the lead stays at the last step's size until the next scroll.Part of #16424. Builds on #16452 (merged). This PR's commits are 16463ee (test) and 3b0dc6c (fix), plus a merge of feat-cmux-next.
Verification
transcript.test.tsxgets a jsdom test that scrolls up a viewport and a half per scroll event and expects the rows two steps ahead to be mounted right away:min mounted top 8008, expected <= 5500, because rows stayed at the old offset.webviews/:bun run test(280 pass), typecheck andlint:cipass.build-agent-pane-web.sh --checkandbuild-webviews-app.sh --checkpass.nx-scroll-v1(fleet job f5bbdb3b, head 3b0dc6c): seeded 5,000 rows, then ran four 3 s flings. The display is 160 Hz and the page runs at 80 Hz, as in the earlier benches.The 80 Hz page cap remains (#16424).
Changelog
none
🤖 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
Fixes the blank edge strip that showed on every frame when flinging the agent pane's transcript, and pulls in the
feat-cmux-nextlag and reconnect fixes.Bug Fixes
flushSync, and the mounted range leads in the scroll direction by two scroll steps (capped at four viewports), so the rows a fling reaches next are already in the DOM.Written for commit 3d9a69c. Summary will update on new commits.