Repository navigation
mux: server/client control commands (ping, reload-config, window-title, scroll-changed) - #7604
Conversation
…e, scroll-changed)
Adds four control commands and one event to the cmux-mux protocol
(protocol stays v6, additive), clean-room:
- ping: { ok, version, protocol } liveness probe, distinct from identify.
- reload-config: re-reads mux.json via config::load() and live-applies
theme/colors, tabs, sidebar, scrollbar, and keybindings to the running
TUI through the existing event loop (no timers); headless is a no-op.
- set-window-title / clear-window-title: write OSC 0/2 to the local and
each attached client's own terminal (sanitized), no focus change.
- scroll-changed event: { surface, offset, at_bottom }, emitted from the
shared scroll/viewport helpers, coalesced; subscribe gets all, attach
gets its surface.
Server + CLI verbs + Python binding + conformance fixture + spec.
NOTE: local verification was blocked by a host zig/ghostty toolchain
regression (libSystem link failure, unrelated to this change); fmt is
clean and the fixtures/python parse, but the Rust build/tests must be
validated by CI (test-linux/macos/bindings-e2e).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds protocol v6 control commands, structured scroll-changed events, runtime surface-option updates, and corresponding changes in mux-tui, Python bindings, conformance fixtures, and protocol docs. ChangesProtocol v6 Commands and Scroll Events
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Mux
participant App
Client->>Server: Command::ReloadConfig
Server->>Mux: emit ConfigReloadRequested
Mux-->>App: MuxEvent::ConfigReloadRequested
App->>App: reload_config()
App->>App: write_window_title(title)
Client->>Server: Command::SetWindowTitle{title}
Server->>Mux: emit WindowTitleRequested
Mux-->>App: MuxEvent::WindowTitleRequested(title)
Possibly related PRs
🚥 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 SummaryThis PR adds new cmux mux control commands and scroll events. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "mux: fix clippy needless-bool in scroll-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Command::SetWindowTitle { title } => { | ||
| mux.emit(MuxEvent::WindowTitleRequested(title)); | ||
| Ok(json!({})) |
There was a problem hiding this comment.
When set-window-title is invoked from the non-streaming CLI against a headless server, this branch only broadcasts WindowTitleRequested and then returns {}. With no TUI or subscribe client present, no process writes the OSC sequence, so the command exits successfully while the terminal title never changes.
| .flatten() | ||
| .unwrap_or(0); | ||
| Some(before != after) | ||
| } | ||
| SurfaceHandle::Remote(surface, _) if surface.kind == SurfaceKind::Pty => { | ||
| let mut term = surface.term.lock().unwrap(); | ||
| let before = term.scrollbar().map(|sb| sb.offset).unwrap_or(0); |
There was a problem hiding this comment.
For remote PTY surfaces, this branch mutates only the client's local terminal mirror and never sends the scroll through the server. The new scroll-changed event is then missing for other subscribers, and the server-side viewport can differ from the viewport the remote TUI is showing.
| surface.scroll_to_bottom()?; | ||
| surface.try_with_terminal(|term| { |
There was a problem hiding this comment.
The send path now scrolls to bottom, releases the terminal lock, and reacquires it later for encoder.sync_from_terminal(term). On an active PTY, output can arrive between those two steps, so the viewport event and the encoded input can be based on different terminal snapshots.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@mux/bindings/conformance/runner.py`:
- Around line 192-195: The `clear-window-title` entry in `runner.py` uses a
redundant lambda wrapper around `client.clear_window_title`, which also
introduces an unused `kw` parameter warning. Update the mapping alongside
`ping`, `reload-config`, and `set-window-title` so `clear-window-title` points
directly to `client.clear_window_title` without any wrapper.
In `@mux/crates/mux-core/src/server.rs`:
- Around line 825-849: The `spawn_attach_notification_stream` and `Subscribe`
paths are duplicating the `MuxEvent` to JSON mapping for `Notification` and
`ScrollChanged`, which can drift over time. Extract a shared helper like
`mux_event_json(&MuxEvent) -> Option<Value>` (or per-variant helpers such as
`scroll_changed_json`/`notification_json`) in `server.rs`, and have both the
attach stream and the subscribe handler call it so the JSON shape stays
consistent; keep the attach stream’s `surface_id` filtering separate from the
shared serialization logic.
🪄 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: e30ce9c5-3cda-43b4-91c0-757363974078
📒 Files selected for processing (16)
mux/bindings/conformance/fixtures.jsonmux/bindings/conformance/runner.pymux/bindings/python/cmux/client.pymux/crates/mux-core/src/mux.rsmux/crates/mux-core/src/server.rsmux/crates/mux-core/src/surface.rsmux/crates/mux-tui/src/app.rsmux/crates/mux-tui/src/cli.rsmux/crates/mux-tui/src/config.rsmux/crates/mux-tui/src/main.rsmux/crates/mux-tui/src/session/mod.rsmux/crates/mux-tui/src/session/remote.rsmux/crates/mux-tui/tests/cli.rsmux/spec/cli.mdmux/spec/commands.mdmux/spec/events.md
| "ping": client.ping, | ||
| "reload-config": client.reload_config, | ||
| "set-window-title": client.set_window_title, | ||
| "clear-window-title": lambda **kw: client.clear_window_title(), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Drop the unnecessary lambda for clear-window-title.
clear_window_title() takes no arguments, so the lambda wrapper is redundant — ping and reload-config map their no-arg methods directly. The lambda also triggers a Ruff ARG005 warning for the unused kw parameter.
♻️ Proposed fix
- "clear-window-title": lambda **kw: client.clear_window_title(),
+ "clear-window-title": client.clear_window_title,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "ping": client.ping, | |
| "reload-config": client.reload_config, | |
| "set-window-title": client.set_window_title, | |
| "clear-window-title": lambda **kw: client.clear_window_title(), | |
| "ping": client.ping, | |
| "reload-config": client.reload_config, | |
| "set-window-title": client.set_window_title, | |
| "clear-window-title": client.clear_window_title, |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 195-195: Unused lambda argument: kw
(ARG005)
🤖 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 `@mux/bindings/conformance/runner.py` around lines 192 - 195, The
`clear-window-title` entry in `runner.py` uses a redundant lambda wrapper around
`client.clear_window_title`, which also introduces an unused `kw` parameter
warning. Update the mapping alongside `ping`, `reload-config`, and
`set-window-title` so `clear-window-title` points directly to
`client.clear_window_title` without any wrapper.
Source: Linters/SAST tools
| let value = match event { | ||
| MuxEvent::Notification(notification) | ||
| if notification.surface == Some(surface_id) => | ||
| { | ||
| json!({ | ||
| "event": "notification", | ||
| "notification": notification.notification, | ||
| "title": notification.title, | ||
| "body": notification.body, | ||
| "level": notification.level.as_str(), | ||
| "surface": notification.surface, | ||
| }) | ||
| } | ||
| MuxEvent::ScrollChanged { surface, offset, at_bottom } | ||
| if surface == surface_id => | ||
| { | ||
| json!({ | ||
| "event": "scroll-changed", | ||
| "surface": surface, | ||
| "offset": offset, | ||
| "at_bottom": at_bottom, | ||
| }) | ||
| } | ||
| _ => continue, | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated ScrollChanged/Notification event→JSON mapping.
The scroll-changed (and notification) JSON shape is now hand-duplicated between spawn_attach_notification_stream (lines 838-847) and the Subscribe handler (lines 1387-1392). As more MuxEvent variants are added, this duplication risks silent drift (e.g. one path gets a field added/renamed and the other doesn't).
Consider extracting a shared fn mux_event_json(event: &MuxEvent) -> Option<Value> (or similar) used by both streams, with the attach stream additionally filtering by surface_id.
♻️ Sketch of a shared helper
+fn scroll_changed_json(surface: SurfaceId, offset: u64, at_bottom: bool) -> Value {
+ json!({
+ "event": "scroll-changed",
+ "surface": surface,
+ "offset": offset,
+ "at_bottom": at_bottom,
+ })
+}Then call scroll_changed_json(surface, offset, at_bottom) from both call sites.
Also applies to: 1387-1392
🤖 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 `@mux/crates/mux-core/src/server.rs` around lines 825 - 849, The
`spawn_attach_notification_stream` and `Subscribe` paths are duplicating the
`MuxEvent` to JSON mapping for `Notification` and `ScrollChanged`, which can
drift over time. Extract a shared helper like `mux_event_json(&MuxEvent) ->
Option<Value>` (or per-variant helpers such as
`scroll_changed_json`/`notification_json`) in `server.rs`, and have both the
attach stream and the subscribe handler call it so the JSON shape stays
consistent; keep the attach stream’s `surface_id` filtering separate from the
shared serialization logic.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 07589e4. Configure here.
| offset, | ||
| at_bottom, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Live bottom scroll event spam
Medium Severity
The PTY reader treats any change to the (offset, at_bottom) pair as a scroll-changed emit. While the viewport stays pinned at the live bottom, new output still bumps offset with growing scrollback even though at_bottom stays true, so subscribers can get a scroll-changed line on every read chunk during active output.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 07589e4. Configure here.
| "offset": offset, | ||
| "at_bottom": at_bottom, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Duplicate scroll-changed on attach
Medium Severity
After attach-surface, the same connection already streams all mux events from subscribe, including scroll-changed. The attach notification thread also writes scroll-changed for that surface on the shared LineWriter, so typical attach clients receive two identical event lines per viewport change.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 07589e4. Configure here.
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)
mux/crates/mux-tui/src/app.rs (1)
849-856: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDon't let a cosmetic title write tear down the event loop.
write_window_titlereturnsResultandhandle()propagates it with?(Line 952), so a transient stdout write/flush error bubbles up toevent_loopand terminates the TUI. Every other OSC writer here (sync_pointer_shape,copy_text_to_clipboard) intentionally swallows such errors. Setting/clearing the window title is non-essential; a failed write shouldn't end the session.🛡️ Make title writes non-fatal (consistent with sibling OSC writers)
- fn write_window_title(&self, title: &str) -> anyhow::Result<()> { - let lock = self.stdout_lock.clone(); - let _guard = lock.lock().unwrap(); - let mut stdout = std::io::stdout(); - stdout.write_all(&mux_core::server::window_title_osc(title))?; - stdout.flush()?; - Ok(()) - } + fn write_window_title(&self, title: &str) { + let lock = self.stdout_lock.clone(); + let _guard = lock.lock().unwrap(); + let mut stdout = std::io::stdout(); + let _ = stdout.write_all(&mux_core::server::window_title_osc(title)); + let _ = stdout.flush(); + }And update the call site:
AppEvent::Mux(MuxEvent::WindowTitleRequested(title)) => { - self.write_window_title(&title)?; + self.write_window_title(&title); Ok(RenderAction::None) }🤖 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 `@mux/crates/mux-tui/src/app.rs` around lines 849 - 856, The write_window_title helper currently returns a Result and is propagated from handle(), so a transient stdout write or flush failure can tear down the TUI event loop. Make write_window_title non-fatal like sync_pointer_shape and copy_text_to_clipboard by swallowing stdout errors inside the method in app.rs, and update the handle() call site so title updates do not use ? or otherwise propagate failures.
🤖 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 `@mux/crates/mux-tui/src/app.rs`:
- Around line 849-856: The write_window_title helper currently returns a Result
and is propagated from handle(), so a transient stdout write or flush failure
can tear down the TUI event loop. Make write_window_title non-fatal like
sync_pointer_shape and copy_text_to_clipboard by swallowing stdout errors inside
the method in app.rs, and update the handle() call site so title updates do not
use ? or otherwise propagate failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8cf8a06-3c00-4edc-a778-79f63ac2b70a
📒 Files selected for processing (1)
mux/crates/mux-tui/src/app.rs
This branch was successfully deployed
1 participant
Adds four control commands and one event to the cmux-mux protocol (v6, additive), clean-room (no third-party source consulted).
{ ok, version, protocol }liveness probe, lighter thanidentify.mux.json(config::load()) and live-applies theme/colors, tab and sidebar settings, scrollbar, and keybindings to the running TUI through the existing event loop (no timers); a headless server treats it as a no-op. Fields only read at startup still need a restart (documented).{ surface, offset, at_bottom }, emitted from the shared scroll/viewport helpers and coalesced; subscribe streams get all, attach streams get their surface.Server + CLI verbs + Python binding (conformance dispatches through it) + a new conformance fixture + spec updates (
commands.md/cli.md/events.md).Verification note: local build was blocked by a host zig/ghostty toolchain regression (a libSystem link failure unrelated to this change — the same clean-cache build fails on
maintoo on this machine).cargo fmtis clean and the JSON/Python parse; the Rust build + tests are validated here by CI (test-linux/macos, bindings-e2e, valgrind). Reviewed by the /fable judge for code-level correctness.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches control-socket protocol, event fan-out, and scroll/input paths across server and TUI; changes are additive but affect attach/subscribe clients and live config reload behavior.
Overview
Protocol v6 (additive) adds
ping,reload-config,set-window-title, andclear-window-title, plus mux eventsconfig-reload-requested,window-title-requested, andscroll-changed(surface,offset,at_bottom).Server behavior: Control commands emit the corresponding mux events; window titles go out as sanitized OSC 0/2. PTY scroll moves are centralized on
Surface::scroll_delta/scroll_to_bottom(including PTY reader output), so subscribe and attach streams get consistent viewport notifications.surface_optionsis mutex-protected withupdate_surface_optionssoreload-configcan refresh browser launch settings for new surfaces.TUI: Handles reload and window-title events (reload via
config::load()andapply_config; title via stdout OSC). Scrolling and key input use the shared surface scroll helpers instead of direct terminal mutation.Bindings & docs: Python client and conformance runner dispatch the new commands; a
server-client-controlfixture exercises ping and title verbs. CLI verbs, integration tests, andcommands.md/events.md/cli.mdare updated for v6.Reviewed by Cursor Bugbot for commit 07589e4. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Add server/client control commands to the
cmux-muxprotocol (v6, additive):ping,reload-config, and set/clear window title, plus ascroll-changedevent. Includes server, CLI verbs, Python bindings, and spec updates for live config reload and viewport change notifications.pingreturns{ ok, version, protocol }.reload-configemitsconfig-reload-requestedso frontends re-read config.set-window-title/clear-window-titleemitwindow-title-requestedand update the terminal window title (OSC 0/2, sanitized). Newscroll-changedevent carries{ surface, offset, at_bottom }.reload-config. Applies browser settings to future surfaces. Writes sanitized OSC 0/2 on window title requests. Headless servers no-op.scroll-changedon user scroll, programmatic scroll, input snap-to-bottom, and output-driven moves. Subscribe streams get all; attach streams get their surface only.ping,reload-config,set-window-title,clear-window-title.pingprintscmux-mux version=<version> protocol=<protocol>.ping,reload_config,set_window_title,clear_window_titleand event fieldsoffset/at_bottom. Conformance fixture andcommands.md/events.md/cli.mdupdated to protocol v6.Written for commit 07589e4. Summary will update on new commits.
Summary by CodeRabbit
ping,reload-config,set-window-title, andclear-window-title.scroll-changed,config-reload-requested, andwindow-title-requestedfor attach/subscribe streams.offsetandat_bottomreporting.