Repository navigation
Deliver durable cloud usage notices - #8942
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds durable provider notices across the machine-provider protocol, provider client, runtime update stream, and TUI. It defines delivery metadata and subscription/acknowledgement methods, retains and replays notices, renders and dismisses them, and acknowledges them after successful painting. ChangesDurable provider notices
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant ProviderClient
participant ProviderMachineRuntime
participant App
participant UI
Provider->>ProviderClient: publish notice with delivery metadata
ProviderClient->>ProviderMachineRuntime: deliver retained notice
ProviderMachineRuntime->>App: send MachineUpdate::DurableNotice
App->>UI: paint durable notice banner
UI->>App: record successful paint
App->>ProviderMachineRuntime: acknowledge durable notice
ProviderMachineRuntime->>ProviderClient: acknowledge_notice
ProviderClient->>Provider: advance acknowledgement cursor
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 SummaryAdds capability-gated durable provider notices across the machine protocol and TUI.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains within the scope of this follow-up review. Important Files Changed
Sequence DiagramsequenceDiagram
participant TUI
participant Client as Provider Client
participant Provider
TUI->>Client: Connect with stable consumer ID
Client->>Provider: hello
Provider-->>Client: durable-notices-v1 capability
Client->>Provider: subscribe_notices(consumer_id)
Provider-->>Client: resume sequence
Provider-->>Client: retained/live notice + delivery metadata
Client-->>TUI: DurableProviderNotice
TUI->>TUI: Paint notice banner successfully
TUI->>Client: acknowledge_notice(id, sequence)
Client->>Provider: acknowledge_notice(id, sequence)
Reviews (11): Last reviewed commit: "Coalesce provider snapshot invalidations" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/crates/cmux-tui/src/app.rs`:
- Around line 6239-6250: Update the AppEvent::Input mouse handling to consume
mouse presses that land on the durable notice banner while it is displayed:
dismiss the notice and return the appropriate redraw action without forwarding
the event to handle_mouse. Preserve normal mouse handling for events outside the
banner and keep non-press dismissal behavior unchanged.
- Around line 4462-4479: Separate durable-notice acknowledgement backoff from
shared provider reconnect state by adding and using a dedicated
durable_notice_ack_retry_at field in the acknowledgement failure and
submit_pending_durable_notice_ack paths. Ensure successful acknowledgement or
explicit acknowledgement-state reset clears both durable_notice_ack_retry_at and
durable_notice_ack_failures, while provider reconnect handling no longer affects
either value.
In `@cmux-tui/crates/cmux-tui/src/ui/mod.rs`:
- Around line 77-80: Update the style construction near the status-level
rendering to make the intended colors explicit: remove Modifier::REVERSED for
colored text on app.chrome.status_bg, or, if a solid color bar is intended, swap
the foreground and background to fg(app.chrome.status_bg).bg(color) without
reversal. Adjust the paired visibility test to validate the rendered color
arrangement rather than only the stored pre-reversal foreground.
🪄 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 Plus
Run ID: 34327e8d-30dd-4a8a-854a-80faffe2b2b5
📒 Files selected for processing (7)
cmux-tui/crates/cmux-tui-machine-protocol/src/lib.rscmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/machine.rscmux-tui/crates/cmux-tui/src/machine_provider_client.rscmux-tui/crates/cmux-tui/src/machine_provider_runtime.rscmux-tui/crates/cmux-tui/src/ui/mod.rscmux-tui/spec/machine-provider.md
# Conflicts: # cmux-tui/crates/cmux-tui-machine-protocol/src/lib.rs # cmux-tui/crates/cmux-tui/src/app.rs # cmux-tui/crates/cmux-tui/src/machine_provider_client.rs # cmux-tui/crates/cmux-tui/src/machine_provider_runtime.rs # cmux-tui/crates/cmux-tui/src/ui/mod.rs # cmux-tui/spec/machine-provider.md
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. |
…ud-usage-warning-events
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/crates/cmux-tui/src/machine_agent/identity.rs`:
- Around line 128-149: Update the concurrent fallback in load_or_create around
the existing load(path) calls to retry load once when verify_private_file
reports the specific transient “must have exactly one link” failure caused by
the hard-link window. Keep the hardlink validation unchanged, and propagate all
other errors immediately; after the bounded retry, preserve the existing
load-or-create result behavior.
In `@cmux-tui/crates/cmux-tui/src/machine_agent/mod.rs`:
- Around line 190-198: Update
cmux-tui/crates/cmux-tui/src/machine_agent/mod.rs:190-198 around
StderrReporter::new to defer opening the pairing terminal until a pairing code
actually needs to be printed, allowing supervised reconnects without /dev/tty;
alternatively preserve the fail-closed behavior only if it is explicitly
documented. Update cmux-tui/docs/getting-started.md:66-73 to state that npx cmux
machine-agent requires an interactive terminal. Update
cmux-tui/docs/machines.md:97-103 with the same prerequisite for sharing a local
machine, and move the ssh cmux.cloud rationale above the agent-start command.
- Around line 55-64: Update the run_agent error handling in run so the
user-facing result remains the localized messages.runtime_failed, while the
original error is retained and emitted through the existing
CMUX_MACHINE_AGENT_DEBUG path or logging sink when enabled. Do not expose raw
failure details in the normal user-facing error.
In `@cmux-tui/crates/cmux-tui/src/machine_agent/runtime.rs`:
- Around line 1127-1129: The cloud write path in the `send` method has no
timeout and can block the worker loop indefinitely. Guard
`protocol_io::write_frame` with a deadline or equivalent bounded write control,
reusing the existing `ReadDeadline`-style mechanism used during handshake, and
preserve the current error propagation when the deadline or write fails.
In `@cmux-tui/crates/cmux-tui/src/machine_agent/transport.rs`:
- Around line 89-97: Add the SSH option terminator before the destination in the
argument construction flow of the relevant transport method, placing “--”
immediately before destination and preserving the existing cmux machine register
arguments and ordering.
In `@cmux-tui/crates/cmux-tui/src/main.rs`:
- Line 250: Update the USAGE block’s machine_agent_usage placeholder handling in
main.rs so unsupported platforms remove the entire indented line, including its
surrounding whitespace, rather than substituting an empty string. Apply the same
trimming behavior to the additional occurrence referenced by the comment.
- Around line 707-716: Update the machine-agent error handling across
machine_agent::run and the main dispatch block so help text is emitted only for
argument-parse errors. Preserve the concise error-only stderr output for runtime
failures such as reconnect, lock, socket, or SSH errors, using a typed or
otherwise explicit distinction from run rather than matching the opaque message.
In `@cmux-tui/crates/cmux-tui/tests/cli.rs`:
- Around line 349-363: Update
machine_agent_runtime_failures_are_stable_and_localized to exercise an
argument-level parse failure that cannot be masked by StderrReporter or /dev/tty
setup, such as an invalid value handled by parse_args. Assert the command fails
with the expected localized argument error and does not rely on the generic
runtime_failed output or empty-session validation.
In `@cmux-tui/docs/getting-started.md`:
- Around line 66-73: Update the machine-agent instructions near the npx cmux
machine-agent command to explicitly state that it must be run from an
interactive terminal with /dev/tty available. Keep the existing session-sharing
command and pairing-code guidance unchanged.
In `@cmux-tui/docs/machines.md`:
- Around line 97-103: Update the setup instructions around the outbound agent
and interactive SSH steps: move the rationale for running the one-time
interactive ssh cmux.cloud command before the agent-start command, adjacent to
the initial SSH instructions, and explicitly state that the agent requires a
controlling terminal and hard-fails without one, including on restarts where no
pairing code is printed.
🪄 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 Plus
Run ID: 80e91707-bef8-48e7-8e67-bf1db428fee0
⛔ Files ignored due to path filters (1)
cmux-tui/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
cmux-tui/Cargo.tomlcmux-tui/README.mdcmux-tui/crates/cmux-tui-machine-agent-protocol/Cargo.tomlcmux-tui/crates/cmux-tui-machine-agent-protocol/src/lib.rscmux-tui/crates/cmux-tui-machine-protocol/Cargo.tomlcmux-tui/crates/cmux-tui-machine-protocol/src/lib.rscmux-tui/crates/cmux-tui/Cargo.tomlcmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/localization.rscmux-tui/crates/cmux-tui/src/machine.rscmux-tui/crates/cmux-tui/src/machine_agent/identity.rscmux-tui/crates/cmux-tui/src/machine_agent/mod.rscmux-tui/crates/cmux-tui/src/machine_agent/protocol_io.rscmux-tui/crates/cmux-tui/src/machine_agent/runtime.rscmux-tui/crates/cmux-tui/src/machine_agent/transport.rscmux-tui/crates/cmux-tui/src/machine_provider_client.rscmux-tui/crates/cmux-tui/src/machine_provider_runtime.rscmux-tui/crates/cmux-tui/src/machine_provider_transport.rscmux-tui/crates/cmux-tui/src/main.rscmux-tui/crates/cmux-tui/src/ui/mod.rscmux-tui/crates/cmux-tui/src/ui/rail.rscmux-tui/crates/cmux-tui/tests/cli.rscmux-tui/docs/README.mdcmux-tui/docs/configuration.mdcmux-tui/docs/getting-started.mdcmux-tui/docs/machines.mdcmux-tui/docs/mouse.mdcmux-tui/spec/README.mdcmux-tui/spec/cli.mdcmux-tui/spec/machine-agent.mdcmux-tui/spec/machine-provider.md
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
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/crates/cmux-tui/src/machine_agent/runtime.rs`:
- Line 28: Shorten the thread name assigned by spawn_local_connectors so it
remains within Linux’s 16-byte pthread name limit while retaining each worker’s
index. Replace the current “machine-agent-local-connector-{index}” naming format
with a compact format such as “mx-local-open-{index}”, and apply the same naming
change to the corresponding worker-spawn path.
🪄 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 Plus
Run ID: 0f394031-9027-4449-b272-a0ded143a74b
📒 Files selected for processing (1)
cmux-tui/crates/cmux-tui/src/machine_agent/runtime.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a1598efbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ud-usage-warning-events
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4699faabb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ud-usage-warning-events
…ud-usage-warning-events # Conflicts: # cmux-tui/crates/cmux-tui/src/app.rs # cmux-tui/crates/cmux-tui/src/machine_provider_runtime.rs
# Conflicts: # cmux-tui/crates/cmux-tui/src/machine_provider_runtime.rs
|
Exact post-PR8748 integration head is 906c5a2. It preserves provider-switch UI overlays while forwarding durable notice updates unchanged. The 602 unit tests, 14 CLI tests, focused durable-notice tests, and workspace clippy with warnings denied passed. One unrelated terminal-host saturation test timed out twice on this heavily loaded host; this branch does not modify its code. @codex review |
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Exact head is 642ac9e. The final documentation commit makes the contract match the reviewed click behavior: keyboard, paste, scroll, and outside-banner presses pass through, while a press on the banner row is consumed to block hidden status targets. Runtime code is unchanged from c2e413a. @codex review |
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 642ac9ee09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 (4)
cmux-tui/spec/machine-provider.md (1)
83-85: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftEstablish the resume cursor before accepting replayed deliveries.
A new control generation starts at cursor zero, while the current test permits a
noticeevent beforesubscribe_noticesresponds. After reconnecting a consumer already acknowledged through sequenceN, a valid pre-response replay atN + 1is rejected as expecting sequence1, causing a reconnect loop.
cmux-tui/spec/machine-provider.md#L83-L85: require a successfulsubscribe_noticesresponse to precede durable deliveries, or explicitly define pre-response buffering.cmux-tui/crates/cmux-tui/src/machine_provider_client.rs#L431-L440: do not apply contiguity validation against the default cursor while subscription initialization is pending; add a reconnect regression with cursorNand deliveryN + 1.🤖 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/spec/machine-provider.md` around lines 83 - 85, Ensure subscription initialization establishes the resume cursor before durable deliveries undergo contiguity validation: update cmux-tui/spec/machine-provider.md lines 83-85 to require a successful subscribe_notices response before deliveries, or define buffering, and update machine_provider_client.rs lines 431-440 to avoid validating against the default cursor while subscription is pending; add a reconnect regression covering an acknowledged cursor N followed by delivery N+1.cmux-tui/crates/cmux-tui/src/machine_provider_runtime.rs (2)
829-1108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated "disconnected/failed UI" construction in
subscribe_ui_updates.The refresh worker builds a near-identical
machine_ui_state(...)+ notice +MachineRequest::ReconnectProviderblock roughly five times (overflow check,ProviderRefreshSignal::Disconnected, channel-disconnected fallback, snapshot/lifecycle/workspace load failures). This is already flagged as acode_block_complexity_highsegment; extracting a small helper (e.g.fn disconnected_ui(...)/fn failed_ui(..., message)) would shrink duplication and make the four failure branches easier to keep in sync.🤖 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/machine_provider_runtime.rs` around lines 829 - 1108, Extract the repeated disconnected/failed UI construction from subscribe_ui_updates into a small helper near the surrounding UI helpers, accepting the current snapshots, keys, and optional notice text. Have it build machine_ui_state with the existing disconnected flags, set the notice and ReconnectProvider request, then replace the overflow, disconnect, and snapshot/lifecycle/workspace error branches with calls to that helper while preserving their existing messages and control flow.
114-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear
pending_provider_switchwhenever a pending replacement is aborted.
pending_provider_switchis set when a provider-directed pairing succeeds, butabort_replacement()only clearspending_active_local. If that replacement fails, is cancelled, or is replaced by a stale command, the flag stays true; future providerSwitch(...)updates can then clear the currently-active localactive_localviamerge_local_machine_ui_for_provider_switch.🐛 Proposed fix
fn abort_replacement(&mut self) { self.provider.abort_replacement(); self.pending_active_local = None; + self.pending_provider_switch = false; }🤖 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/machine_provider_runtime.rs` at line 114, Update abort_replacement() to clear pending_provider_switch alongside pending_active_local whenever a pending replacement is aborted. Ensure all failure, cancellation, and stale-command paths that invoke abort_replacement() leave the flag false so later provider Switch(...) updates cannot clear the active local state.cmux-tui/crates/cmux-tui/src/app.rs (1)
6628-6645: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScroll wheel on the banner row still reaches the covered rail's scroll handling.
Only
MouseEventKind::Down(_)on the banner row short-circuits beforehandle_mouse(consuming the click so coveredScreenEntry/NewScreen/rail hits can't fire — the fix from the earlier review). Scroll events (ScrollUp/ScrollDown/ScrollLeft/ScrollRight) on that same row only dismiss the notice via the genericdismissedflag but still fall through tohandle_mouse→handle_scroll, which can mutatemachine_rail_scroll/workspace_rail_scrollsince both sidebarRects are sized withcontent_area.height.saturating_add(1)and therefore extend into the banner row (durable_notice_banner_row()computes exactly that row). A mouse wheel scroll on the banner row can silently scroll a hidden rail while the user intends to interact with (or just dismiss) the notice.Consider including the scroll variants in the early-return condition, mirroring the
Downcase:🐛 Proposed fix
AppEvent::Input(Event::Mouse(mouse)) => { - if matches!(mouse.kind, MouseEventKind::Down(_)) + if matches!( + mouse.kind, + MouseEventKind::Down(_) + | MouseEventKind::ScrollUp + | MouseEventKind::ScrollDown + | MouseEventKind::ScrollLeft + | MouseEventKind::ScrollRight + ) && self.durable_notice_banner_row() == Some(mouse.row) && self.dismiss_painted_durable_notice() { return Ok(RenderAction::Draw); }🤖 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 6628 - 6645, Update the early-return condition in the AppEvent::Input mouse handler to treat ScrollUp, ScrollDown, ScrollLeft, and ScrollRight on durable_notice_banner_row() like Down events. When dismiss_painted_durable_notice() succeeds for any of these events, return RenderAction::Draw before calling handle_mouse, preventing covered rail scrolling while preserving normal handling elsewhere.
🤖 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/crates/cmux-tui/src/app.rs`:
- Around line 6628-6645: Update the early-return condition in the
AppEvent::Input mouse handler to treat ScrollUp, ScrollDown, ScrollLeft, and
ScrollRight on durable_notice_banner_row() like Down events. When
dismiss_painted_durable_notice() succeeds for any of these events, return
RenderAction::Draw before calling handle_mouse, preventing covered rail
scrolling while preserving normal handling elsewhere.
In `@cmux-tui/crates/cmux-tui/src/machine_provider_runtime.rs`:
- Around line 829-1108: Extract the repeated disconnected/failed UI construction
from subscribe_ui_updates into a small helper near the surrounding UI helpers,
accepting the current snapshots, keys, and optional notice text. Have it build
machine_ui_state with the existing disconnected flags, set the notice and
ReconnectProvider request, then replace the overflow, disconnect, and
snapshot/lifecycle/workspace error branches with calls to that helper while
preserving their existing messages and control flow.
- Line 114: Update abort_replacement() to clear pending_provider_switch
alongside pending_active_local whenever a pending replacement is aborted. Ensure
all failure, cancellation, and stale-command paths that invoke
abort_replacement() leave the flag false so later provider Switch(...) updates
cannot clear the active local state.
In `@cmux-tui/spec/machine-provider.md`:
- Around line 83-85: Ensure subscription initialization establishes the resume
cursor before durable deliveries undergo contiguity validation: update
cmux-tui/spec/machine-provider.md lines 83-85 to require a successful
subscribe_notices response before deliveries, or define buffering, and update
machine_provider_client.rs lines 431-440 to avoid validating against the default
cursor while subscription is pending; add a reconnect regression covering an
acknowledged cursor N followed by delivery N+1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 66225fc3-7393-43d3-8fdd-6f78de78289d
📒 Files selected for processing (7)
cmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/machine.rscmux-tui/crates/cmux-tui/src/machine_agent/runtime.rscmux-tui/crates/cmux-tui/src/machine_provider_client.rscmux-tui/crates/cmux-tui/src/machine_provider_runtime.rscmux-tui/crates/cmux-tui/src/ui/mod.rscmux-tui/spec/machine-provider.md
✅ Action performedReview finished.
|
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/crates/cmux-tui/src/machine_provider_runtime.rs (1)
857-916: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "provider disconnected" UI construction (3x) in the refresh worker.
The overflow-check branch,
ProviderRefreshSignal::Disconnectedarm, and theRecvTimeoutError::Disconnectedarm all build an identical disconnectedMachineUiState(samemachine_ui_state(...)call, same notice, sameReconnectProviderrequest, same send-and-break). Extract a small helper closure capturinglast_snapshot,last_machine_lifecycle_snapshot,last_workspace_snapshot,keys, andrefresh_outputto build+send this once.♻️ Proposed refactor sketch
+ let emit_disconnected = |last_snapshot: &protocol::SnapshotResult, + last_machine_lifecycle_snapshot: &protocol::MachineLifecycleSnapshotResult, + last_workspace_snapshot: Option<&protocol::WorkspaceSnapshotResult>| { + let mut ui = machine_ui_state( + last_snapshot, + last_machine_lifecycle_snapshot, + last_workspace_snapshot, + &keys, + false, + false, + ); + ui.notice = Some(localization::catalog().sidebar.machine_provider_disconnected.into()); + ui.request = Some(MachineRequest::ReconnectProvider); + let _ = refresh_output.send(MachineUpdate::Ui(Box::new(ui))); + };Then replace each of the three inline blocks with a call to
emit_disconnected(&last_snapshot, &last_machine_lifecycle_snapshot, last_workspace_snapshot.as_ref()).🤖 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/machine_provider_runtime.rs` around lines 857 - 916, Extract the duplicated disconnected-UI construction in the refresh worker into an emit_disconnected helper closure capturing keys and refresh_output, with snapshot arguments as needed. Have it build the machine_ui_state, set the disconnected notice and ReconnectProvider request, and send the UI update; replace the overflow, ProviderRefreshSignal::Disconnected, and conditional RecvTimeoutError::Disconnected blocks with calls to this helper while preserving their existing break and overflow behavior.
🤖 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/crates/cmux-tui/src/machine_provider_runtime.rs`:
- Around line 857-916: Extract the duplicated disconnected-UI construction in
the refresh worker into an emit_disconnected helper closure capturing keys and
refresh_output, with snapshot arguments as needed. Have it build the
machine_ui_state, set the disconnected notice and ReconnectProvider request, and
send the UI update; replace the overflow, ProviderRefreshSignal::Disconnected,
and conditional RecvTimeoutError::Disconnected blocks with calls to this helper
while preserving their existing break and overflow behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc7c90b2-a338-49f9-ae62-278b8d526d38
📒 Files selected for processing (7)
cmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/machine.rscmux-tui/crates/cmux-tui/src/machine_agent/runtime.rscmux-tui/crates/cmux-tui/src/machine_provider_client.rscmux-tui/crates/cmux-tui/src/machine_provider_runtime.rscmux-tui/crates/cmux-tui/src/ui/mod.rscmux-tui/spec/machine-provider.md
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. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d45ef4d948
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db49438b37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Verification
cargo clippy -p cmux-tui --all-targets -- -D warningscargo test -p cmux-tui-machine-protocolcargo test -p cmux-tui --no-fail-fast(493 unit, 10 CLI, and 3 host-mode tests passed; three unrelated timing cases passed on isolated rerun)uwntcrendered through/tmp/cmux-debug-uwntc.sockNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds durable cloud usage notices with a color‑coded bottom banner and paint‑gated, action‑aware acknowledgements, plus an outbound
machine-agentfor secure external pairing. Coalesces provider snapshot invalidations to cut redundant refreshes and reduce UI churn.New Features
durable-notices-v1withsubscribe_noticesandacknowledge_notice, FIFO delivery, bounded queues, and a bottom banner by level; capability negotiation and spec updated.machine-agentsubcommand andcmux machine registerflow viacmux-tui-machine-protocol, with one‑time pairing codes and strict SSH host verification.Bug Fixes
machine-agentCLI parsing.Written for commit c3a18d4. Summary will update on new commits.
Summary by CodeRabbit