Repository navigation
cmux-tui: preserve smallest sizing through web reattach - #8302
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesRender Attachment Recovery
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalHook as useRenderTerminal
participant Client as CmuxClient
participant Stream as attachSurface
participant Fit as terminal fit
TerminalHook->>Client: attachSurface(surface)
Client->>Stream: read attach events
Stream-->>TerminalHook: OverflowEvent
TerminalHook->>TerminalHook: waitForRetry(delayMs)
TerminalHook->>Client: reattach surface
Client->>Stream: read next attach events
TerminalHook->>Fit: republish fitted size
Fit-->>Client: resizeSurface(80, 24)
Possibly related PRs
🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes the web client losing its size contribution to the shared minimum after an overflow-recovery reattach. Both
Confidence Score: 5/5Safe to merge; the attach-recovery and size-release logic is well-tested and the server correctly handles all lifecycle transitions. The core fix — nulling Both Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Browser as Web Client
participant Server as cmux-tui-core
Note over Browser: Component mounts
Browser->>Server: attachSurface(surface)
Server-->>Browser: vt-state / render-state
Browser->>Server: resizeSurface(cols, rows) [reportedFit set]
alt Overflow event received
Server-->>Browser: "overflow{scope:"surface"}"
Browser->>Server: stream.close()
Note over Browser: reportedFit = null
Browser->>Server: attachSurface(surface) [retry]
Server-->>Browser: vt-state / render-state
Browser->>Server: resizeSurface(cols, rows) [re-publish unchanged fit]
end
alt detached event received
Server-->>Browser: detached
Note over Browser: IIFE returns (cancelled=false)
Browser->>Server: "releaseSurfaceSize(surface) [finally, call #1]"
Note over Browser: Component later unmounts
Browser->>Server: "releaseSurfaceSize(surface) [cleanup, call #2 - no-op]"
end
alt Component unmount (cancelled path)
Note over Browser: cancelled = true
Browser->>Server: releaseSurfaceSize(surface) [cleanup only, single call]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Browser as Web Client
participant Server as cmux-tui-core
Note over Browser: Component mounts
Browser->>Server: attachSurface(surface)
Server-->>Browser: vt-state / render-state
Browser->>Server: resizeSurface(cols, rows) [reportedFit set]
alt Overflow event received
Server-->>Browser: "overflow{scope:"surface"}"
Browser->>Server: stream.close()
Note over Browser: reportedFit = null
Browser->>Server: attachSurface(surface) [retry]
Server-->>Browser: vt-state / render-state
Browser->>Server: resizeSurface(cols, rows) [re-publish unchanged fit]
end
alt detached event received
Server-->>Browser: detached
Note over Browser: IIFE returns (cancelled=false)
Browser->>Server: "releaseSurfaceSize(surface) [finally, call #1]"
Note over Browser: Component later unmounts
Browser->>Server: "releaseSurfaceSize(surface) [cleanup, call #2 - no-op]"
end
alt Component unmount (cancelled path)
Note over Browser: cancelled = true
Browser->>Server: releaseSurfaceSize(surface) [cleanup only, single call]
end
Reviews (19): Last reviewed commit: "fix(tui): linearize subscription recover..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const waitForRetry = (delayMs: number) => | ||
| new Promise<void>((resolve) => { | ||
| wakeRetry = resolve; | ||
| retryTimer = setTimeout(() => { | ||
| retryTimer = undefined; | ||
| wakeRetry = null; | ||
| resolve(); | ||
| }, delayMs); | ||
| }); |
There was a problem hiding this comment.
waitForRetry duplicated from useAttachedTerminal
The waitForRetry closure (lines 147–155) is copied verbatim from useAttachedTerminal. Both hooks now own identical implementations of the retry-wait primitive. Moving it into attachRecovery.ts—as a standalone exported function alongside attachRecoveryDelay and ATTACH_RECOVERY_STABLE_MS—would keep the cancellation and delay logic in one place.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| it("reports an unchanged local fit again after overflow reattachment", async () => { | ||
| globalThis.ResizeObserver = class { | ||
| observe() {} | ||
| unobserve() {} | ||
| disconnect() {} | ||
| }; | ||
| const streams = [ | ||
| new TestStream([ | ||
| { event: "vt-state", surface: 7, cols: 100, rows: 30, data: new Uint8Array(), colors: {} }, | ||
| { event: "overflow", scope: "surface", surface: 7, error: "subscriber fell behind" }, | ||
| ]), | ||
| new TestStream([ | ||
| { event: "vt-state", surface: 7, cols: 100, rows: 30, data: new Uint8Array(), colors: {} }, | ||
| ]), | ||
| ]; | ||
| const client = { | ||
| attachSurface: vi.fn(async () => streams.shift()!), | ||
| resizeSurface: vi.fn(async () => ({ accepted: true, reservation_id: null })), | ||
| send: vi.fn(async () => ({})), | ||
| } as unknown as CmuxClient; | ||
|
|
||
| render(<Harness client={client} />); | ||
|
|
||
| await waitFor(() => expect(client.attachSurface).toHaveBeenCalledTimes(2)); | ||
| await waitFor(() => expect(client.resizeSurface).toHaveBeenCalledTimes(2)); | ||
| expect(client.resizeSurface).toHaveBeenNthCalledWith(1, 7, 80, 24); | ||
| expect(client.resizeSurface).toHaveBeenNthCalledWith(2, 7, 80, 24); | ||
| }); |
There was a problem hiding this comment.
After the overflow event the hook waits attachRecoveryDelay(0) = 100 ms before reconnecting. Because vi.useFakeTimers() is not installed, that timer runs on the real wall clock. The test passes but adds about 100 ms of real elapsed time to the suite. Adding vi.useFakeTimers() in beforeEach and vi.runAllTimersAsync() after render would make the test deterministic and instant.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/frontends/web/src/hooks/useRenderTerminal.ts`:
- Around line 512-518: Update the resize publication path around applyFit so it
also verifies that the attachment stream is active before registering or
publishing dimensions. Keep resize observers installed if needed, but suppress
viewport-driven reports while stream is null during retry backoff and after
cancellation or failed recovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84c63449-bd61-4c74-bbcb-b160f8f9f016
📒 Files selected for processing (8)
cmux-tui/bindings/typescript/src/protocol/events.tscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/frontends/web/src/hooks/useAttachedTerminal.tscmux-tui/frontends/web/src/hooks/useCmuxClient.tscmux-tui/frontends/web/src/hooks/useRenderTerminal.tscmux-tui/frontends/web/src/lib/fit.tscmux-tui/frontends/web/test/attached-terminal-sizing.test.tsxcmux-tui/frontends/web/test/fit.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmux-tui/frontends/web/src/hooks/useRenderTerminal.ts (1)
134-155: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove recovery timing into a dedicated cancellation-aware scheduler.
These locally managed
setTimeoutpaths materially add wall-clock retry synchronization. Put retry waiting, stability reset, and cancellation under the shared attachment-recovery abstraction with focused tests.As per coding guidelines, production retry timing must use a dedicated cancellation-aware abstraction with bounded deadlines and tests. As per path instructions, production runtime timers used for synchronization must be flagged.
Also applies to: 469-473, 534-536
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/frontends/web/src/hooks/useRenderTerminal.ts` around lines 134 - 155, Replace the locally managed retry and stability timers in the terminal render setup, including waitForRetry and the referenced timer paths, with the shared attachment-recovery cancellation-aware scheduler. Configure bounded deadlines, route cancellation through that abstraction, and flag the production synchronization timers as required; add focused tests covering retry waits, stability resets, and cancellation.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmux-tui/frontends/web/src/hooks/useRenderTerminal.ts`:
- Around line 134-155: Replace the locally managed retry and stability timers in
the terminal render setup, including waitForRetry and the referenced timer
paths, with the shared attachment-recovery cancellation-aware scheduler.
Configure bounded deadlines, route cancellation through that abstraction, and
flag the production synchronization timers as required; add focused tests
covering retry waits, stability resets, and cancellation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25966626-80c8-443a-b44a-08cc9618c7a8
📒 Files selected for processing (2)
cmux-tui/frontends/web/src/hooks/useRenderTerminal.tscmux-tui/frontends/web/test/render-terminal-sizing.test.tsx
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmux-tui/crates/cmux-tui/src/app.rs (1)
3084-3102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared state invalidation into a helper.
The plugin state reset block is duplicated four times across
sync_sidebar_pluginandapply_sidebar_plugin_status(in both theconfig.sidebar.plugin.is_none()and visibility-check branches). Extracting this block into aclear_sidebar_plugin_statehelper will DRY up the code and prevent missing a state field during future updates.
cmux-tui/crates/cmux-tui/src/app.rs#L3084-L3102: replace the inline state resets with calls toself.clear_sidebar_plugin_state()and add the helper definition above this method.cmux-tui/crates/cmux-tui/src/app.rs#L3135-L3153: replace the inline state resets with calls toself.clear_sidebar_plugin_state().♻️ Proposed refactor
In
cmux-tui/crates/cmux-tui/src/app.rs#L3084-L3102:+ fn clear_sidebar_plugin_state(&mut self) { + self.session.invalidate_sidebar_plugin_sync(); + self.sidebar_plugin_surface = None; + self.sidebar_plugin_error = None; + self.sidebar_plugin_retry_after_ms = None; + self.sidebar_plugin_retry_at = None; + self.sidebar_focus_pending = false; + } + fn sync_sidebar_plugin(&mut self, relaunch: bool) -> bool { if self.config.sidebar.plugin.is_none() { - self.session.invalidate_sidebar_plugin_sync(); - self.sidebar_plugin_surface = None; - self.sidebar_plugin_error = None; - self.sidebar_plugin_retry_after_ms = None; - self.sidebar_plugin_retry_at = None; - self.sidebar_focus_pending = false; + self.clear_sidebar_plugin_state(); return false; } if self.sidebar_width < 3 || !self.sidebar_visible { - self.session.invalidate_sidebar_plugin_sync(); - self.sidebar_plugin_surface = None; - self.sidebar_plugin_error = None; - self.sidebar_plugin_retry_after_ms = None; - self.sidebar_plugin_retry_at = None; + self.clear_sidebar_plugin_state(); self.sidebar_focused = false; - self.sidebar_focus_pending = false; return false; }In
cmux-tui/crates/cmux-tui/src/app.rs#L3135-L3153:fn apply_sidebar_plugin_status(&mut self, status: SidebarPluginSurface, relaunch: bool) { if self.config.sidebar.plugin.is_none() { - self.session.invalidate_sidebar_plugin_sync(); - self.sidebar_plugin_surface = None; - self.sidebar_plugin_error = None; - self.sidebar_plugin_retry_after_ms = None; - self.sidebar_plugin_retry_at = None; - self.sidebar_focus_pending = false; + self.clear_sidebar_plugin_state(); return; } if !self.sidebar_visible { - self.session.invalidate_sidebar_plugin_sync(); - self.sidebar_plugin_surface = None; - self.sidebar_plugin_error = None; - self.sidebar_plugin_retry_after_ms = None; - self.sidebar_plugin_retry_at = None; + self.clear_sidebar_plugin_state(); self.sidebar_focused = false; - self.sidebar_focus_pending = false; return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui/src/app.rs` around lines 3084 - 3102, Extract the duplicated sidebar plugin state reset into a clear_sidebar_plugin_state helper defined above sync_sidebar_plugin in cmux-tui/crates/cmux-tui/src/app.rs. Replace the inline resets in cmux-tui/crates/cmux-tui/src/app.rs lines 3084-3102 and lines 3135-3153 with calls to this helper, preserving each branch’s additional focus-state updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/scripts/smoke-tui.py`:
- Around line 220-228: Update wait_render_excludes to avoid accepting a
transient marker-free frame during reload: require the post-reload readiness
condition or keep observing until the absence of needle remains stable for a
short window before returning. Preserve the existing timeout and assertion
behavior, and ensure callers such as the exclusion check at line 417 validate
the finalized render state.
---
Outside diff comments:
In `@cmux-tui/crates/cmux-tui/src/app.rs`:
- Around line 3084-3102: Extract the duplicated sidebar plugin state reset into
a clear_sidebar_plugin_state helper defined above sync_sidebar_plugin in
cmux-tui/crates/cmux-tui/src/app.rs. Replace the inline resets in
cmux-tui/crates/cmux-tui/src/app.rs lines 3084-3102 and lines 3135-3153 with
calls to this helper, preserving each branch’s additional focus-state updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed2ff376-e364-4069-aca5-f645647fa6a6
📒 Files selected for processing (2)
cmux-tui/crates/cmux-tui/src/app.rscmux-tui/scripts/smoke-tui.py
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
…nnect # Conflicts: # cmux-tui/crates/cmux-tui-core/src/mux.rs
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
1 participant
Follow-up to #8031.
The web client now republishes its unchanged local viewport whenever an overflow recovery creates a new attach stream. Both byte and render attachments use bounded recovery, so the server never loses the client from the smallest-size calculation. The byte fallback also uses Menlo, matching the styled renderer and Ghostty default.
The first commit is the failing behavior test. The second commit fixes it.
Verified:
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Preserves the smallest shared terminal grid across recovery and visibility changes, matching tmux sizing semantics and bounds. Client presence now refreshes after remote recovery, and remote subscription recovery is linearized and coalesced so pane labels and menus stay accurate.
New Features
set-client-sizing(withexclusive) andrelease-surface-size;list-clientsaddssize_participatingandtransport: "local" | "unix" | "ws".CmuxClient#setClientSizing,useOnlyClientSizing,useAllClientSizing; Rustrelease_surface_size; pane border label shows clients and shared minimum; render attach events includeOverflowEvent.Bug Fixes
Written for commit 233eb06. Summary will update on new commits.
Summary by CodeRabbit