Remote tmux mirrors: exact feed-forward sizing, verified pane geometry, faithful live pane headers, active-pane indicator, and drag-stable rendering - #7315
Conversation
|
@ejc3 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughThis PR adds remote tmux per-window sizing, feed-forward mirror geometry, debug sizing verbs, a real-tmux UI test harness, and supporting unit/docs updates. It also includes small workflow, reload, and watchdog script changes. ChangesRemote tmux per-window sizing feature
End-to-end UI tests, scripts, and docs
Infra tweaks
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RemoteTmuxWindowMirrorView
participant RemoteTmuxWindowMirror
participant RemoteTmuxControlConnection
participant TmuxServer
RemoteTmuxWindowMirrorView->>RemoteTmuxWindowMirror: noteContainerSize(pointSize, scale)
RemoteTmuxWindowMirror->>RemoteTmuxWindowMirror: currentGeometry().clientCells(...)
RemoteTmuxWindowMirror->>RemoteTmuxControlConnection: setWindowSize(windowId, columns, rows)
RemoteTmuxControlConnection->>TmuxServer: refresh-client -C '`@id`:WxH'
TmuxServer-->>RemoteTmuxControlConnection: success or %error
RemoteTmuxControlConnection-->>RemoteTmuxWindowMirror: size recorded or fallback applied
RemoteTmuxControlConnection-->>RemoteTmuxWindowMirror: notifyTopologyChanged() after layout/header updates
Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 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 |
a13b26c to
8a1212e
Compare
Greptile SummaryThis PR replaces a feedback-loop-prone proportional SwiftUI split layout with a feed-forward sizing model for remote tmux mirrors: the client size pushed to tmux is derived purely from container device pixels, the layout tree's structure, and measured ghostty cell/padding constants — never from tmux-assigned geometry or rendered grids — eliminating the one-column-short wrapping defect that appeared on multi-pane mirrored windows.
Confidence Score: 5/5The feed-forward sizing model is architecturally sound and thoroughly tested; no new defects were identified in this review pass. The core change — replacing proportional SwiftUI splits and grid-feedback sizing with a pure-function pixel→cells→frames pipeline — is correct by construction: clientCells never reads tmux-assigned geometry, so echo events are silently deduped and there is no feedback loop to manage. The quarantine + generation-tagged list-panes fetch prevents raw layout-string rects from ever reaching the render. Per-window sizing dedup and the hidden-mirror write-once gate handle the multi-window attach ordering correctly. The localization additions cover all 20 supported locales. The test suite exercises the key invariants. The one pre-existing concern the team is tracking — TerminalController+RemoteTmuxTestSupport.swift remaining in production Sources/ — was already flagged in a prior review round. No files require special attention beyond the pre-existing discussion of Sources/TerminalController+RemoteTmuxTestSupport.swift. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant View as RemoteTmuxWindowMirrorView
participant Mirror as RemoteTmuxWindowMirror
participant Geometry as RemoteTmuxMirrorGeometry
participant Conn as RemoteTmuxControlConnection
participant tmux as tmux server
View->>Mirror: noteContainerSize(pointSize, scale)
View->>Mirror: updateClientSize()
Mirror->>Geometry: clientCells(pixelWidth, pixelHeight, structure)
Note over Geometry: f(pixels, structure, constants) - never reads tmux geometry
Geometry-->>Mirror: (cols, rows)
Mirror->>Conn: setWindowSize(windowId, cols, rows)
Conn->>Conn: debounce 180ms
Conn->>tmux: "refresh-client -C '@id:WxH'"
tmux-->>Conn: %layout-change (echo)
Conn->>Conn: stagePendingLayout (quarantine)
Conn->>tmux: list-panes (generation-tagged)
tmux-->>Conn: real pane rects + titles + active
Conn->>Conn: handlePaneRectsReply - publish windowsByID
Conn-->>Mirror: reconcile(layout:) / apply(window:)
Mirror->>View: framesForRender(containerPt:)
Note over Mirror,View: imposed frames: exact device-pixel rails or proportional transient if layout mismatch
%%{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 View as RemoteTmuxWindowMirrorView
participant Mirror as RemoteTmuxWindowMirror
participant Geometry as RemoteTmuxMirrorGeometry
participant Conn as RemoteTmuxControlConnection
participant tmux as tmux server
View->>Mirror: noteContainerSize(pointSize, scale)
View->>Mirror: updateClientSize()
Mirror->>Geometry: clientCells(pixelWidth, pixelHeight, structure)
Note over Geometry: f(pixels, structure, constants) - never reads tmux geometry
Geometry-->>Mirror: (cols, rows)
Mirror->>Conn: setWindowSize(windowId, cols, rows)
Conn->>Conn: debounce 180ms
Conn->>tmux: "refresh-client -C '@id:WxH'"
tmux-->>Conn: %layout-change (echo)
Conn->>Conn: stagePendingLayout (quarantine)
Conn->>tmux: list-panes (generation-tagged)
tmux-->>Conn: real pane rects + titles + active
Conn->>Conn: handlePaneRectsReply - publish windowsByID
Conn-->>Mirror: reconcile(layout:) / apply(window:)
Mirror->>View: framesForRender(containerPt:)
Note over Mirror,View: imposed frames: exact device-pixel rails or proportional transient if layout mismatch
Reviews (16): Last reviewed commit: "Handle remote tmux mirror runtime-ready ..." | Re-trigger Greptile |
8a1212e to
6b04066
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@cmuxUITests/RemoteTmuxSizingUITests.swift`:
- Line 325: `startWidthProbes` is using a fixed sleep before sending keys, which
violates the test time-inversion policy; replace that delay with a real
readiness check for the target panes. Update the `startWidthProbes` flow used by
`buildLabSession` and `buildShapeZoo` to poll a concrete signal such as
`#{pane_current_command}` (or the same readiness condition used by
`remote-tmux-shape-zoo.sh`) before typing, so the test proceeds only when the
panes are actually ready. Remove the “let the shell finish starting” style wait
and keep the waiting logic localized to the pane-startup path.
- Line 47: The socket path in RemoteTmuxSizingUITests is being built with
NSHomeDirectory() and can exceed sun_path in long sandbox paths, causing the
guard to fail before lastSocketFailure is recorded. Update the socketPath setup
in the test’s socket creation flow to either preflight the full path length and
set lastSocketFailure with an explicit overflow message, or switch to a shorter
guaranteed socket root so the failure is surfaced clearly.
In `@Sources/RemoteTmuxControlConnection.swift`:
- Around line 508-577: Stale per-window sizing state in
RemoteTmuxControlConnection is leaking across closed windows: clear the closed
window’s entries from lastWindowSizes and windowSizeDebounceTasks when handling
.windowClose so reconnect/replay can’t use a dead `@id` target. Also update
notePerWindowSizeRejected() and the .perWindowSize error handling path to
distinguish “unsupported on old tmux” from “window not found,” so only the
former flips supportsPerWindowSize to false and falls back to
setClientSize(columns:rows:).
In `@Sources/RemoteTmuxWindowMirror.swift`:
- Around line 416-428: Remove the test-only observability seams from
RemoteTmuxWindowMirror: `lastWindowSizeForTesting` should be dropped and tests
should read `connection.lastWindowSizes[windowId]` directly via `@testable
import`, and `geometryOverrideForTesting` should not live in the production
type. Move the geometry stub behind a dedicated debug-only helper/file or
replace it with a real injectable dependency, and update `currentGeometry()` so
production code no longer branches on the test override.
In `@Sources/TerminalController.swift`:
- Line 2041: `system.capabilities` is advertising DEBUG-only tmux verbs in the
always-on methods list, causing Release builds to claim support for
`remote.tmux.test_exec` and `remote.tmux.test_set_frame` even though
`socketWorkerV2Response` only handles them under `#if DEBUG`. Update
`TerminalController` so these two tokens are removed from the unconditional
`methods` array and are added via `Self.v2DebugMethodNames` alongside the other
debug-only capability names, keeping the advertised capabilities aligned with
actual dispatch behavior.
🪄 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: 8857db97-f31a-4da2-9c53-0b47f6e83fad
📒 Files selected for processing (33)
.github/workflows/test-e2e.ymlPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Sizing.swiftSources/RemoteTmuxControlCommandKind.swiftSources/RemoteTmuxControlConnection.swiftSources/RemoteTmuxControlMessage.swiftSources/RemoteTmuxControlStreamParser.swiftSources/RemoteTmuxController.swiftSources/RemoteTmuxHost.swiftSources/RemoteTmuxLayoutContainer.swiftSources/RemoteTmuxMirrorFrames.swiftSources/RemoteTmuxMirrorGeometry.swiftSources/RemoteTmuxPaneHeader.swiftSources/RemoteTmuxSSHTransport.swiftSources/RemoteTmuxSessionMirror.swiftSources/RemoteTmuxWindow.swiftSources/RemoteTmuxWindowMirror.swiftSources/RemoteTmuxWindowMirrorView.swiftSources/TerminalController+RemoteTmux.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/RemoteTmuxAuthTests.swiftcmuxTests/RemoteTmuxControlParserTests.swiftcmuxTests/RemoteTmuxMirrorFeedForwardTests.swiftcmuxTests/RemoteTmuxMirrorGeometryTests.swiftcmuxUITests/RemoteTmuxSizingUITests.swiftscripts/reload.shscripts/remote-tmux-e2e-ssh-shim-check.shscripts/remote-tmux-e2e-ssh-shim.shscripts/remote-tmux-shape-zoo.shscripts/remote-tmux-width-probe.shskills/cmux-testing/SKILL.mdskills/cmux-testing/references/remote-tmux-sizing-e2e.md
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
Sources/TerminalController.swift (1)
2041-2041: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
system.capabilitiesstill unconditionally advertises DEBUG-only verbs.
remote.tmux.test_exec/remote.tmux.test_set_frameare appended to the always-onmethodsarray here, but the dispatch cases insocketWorkerV2Response(Lines 1140-1143) are#if DEBUG-gated. In Release,system.capabilitiesclaims support for these two verbs but calling them returnsmethod_not_found. This is the same gap flagged on a prior revision of this PR and remains unresolved.🐛 Proposed fix: move the DEBUG-only tokens into the DEBUG-gated append
- "workspace.remote.terminal_session_end", "remote.tmux.sessions", "remote.tmux.attach", "remote.tmux.detach", "remote.tmux.state", "remote.tmux.mirror", "remote.tmux.window", "remote.tmux.pane_grids", "remote.tmux.test_exec", "remote.tmux.test_set_frame", + "workspace.remote.terminal_session_end", "remote.tmux.sessions", "remote.tmux.attach", "remote.tmux.detach", "remote.tmux.state", "remote.tmux.mirror", "remote.tmux.window", "remote.tmux.pane_grids",`#if` DEBUG methods.append(contentsOf: Self.v2DebugMethodNames) methods.append(contentsOf: ["remote.tmux.test_exec", "remote.tmux.test_set_frame"]) `#endif`🤖 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 `@Sources/TerminalController.swift` at line 2041, `system.capabilities` is still advertising DEBUG-only tmux verbs in release builds; move `remote.tmux.test_exec` and `remote.tmux.test_set_frame` out of the always-on `methods` list in `TerminalController` and append them only inside the existing `#if DEBUG` block alongside `Self.v2DebugMethodNames`, so the advertised capabilities match the `socketWorkerV2Response` dispatch cases.
🤖 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 `@cmuxUITests/RemoteTmuxSizingUITests.swift`:
- Around line 70-71: The test cleanup order in the app-launching scenarios is
wrong: app.terminate() runs before tearDown() can use the app socket to call
tmux(["kill-server"]). Update the deferred cleanup in RemoteTmuxSizingUITests so
the tmux server is killed first and the app is terminated afterward, and apply
the same ordering to each launchApp() scenario referenced in the test file to
ensure the remote.tmux.test_exec path remains reachable during teardown.
In `@scripts/remote-tmux-e2e-ssh-shim-check.sh`:
- Around line 60-87: The shim check script is writing stderr to a predictable
shared path, which can collide across runs or be abused via symlink precreation.
Update the stderr capture in the remote tmux e2e shim checks to use the existing
private mktemp-based temp directory/lab directory instead of a global /tmp
filename, and make sure the checks around tmux_remote, check, and the temp
cleanup all reference that per-run private path.
In `@scripts/remote-tmux-shape-zoo.sh`:
- Around line 38-39: The probe payload path in the remote tmux script is
predictable, so update the PROBE assignment and its write/cleanup flow to use an
unpredictable temporary file created with mktemp instead of the current fixed
/tmp/remote-tmux-width-probe-$(id -un).sh pattern. Keep the logic centered
around the PROBE variable in scripts/remote-tmux-shape-zoo.sh, and ensure the
temporary file is removed on exit using the script’s cleanup/trap path.
- Around line 84-87: The tmux setup for the mainh window is missing part of the
e2e shape: update the remote-tmux-shape-zoo.sh window construction around the
mainh setup so it matches the UI test’s mainh layout, including the second
horizontal split and the main-horizontal arrangement. Use the existing tmux
commands in the mainh block as the place to mirror the e2e suite’s shape
exactly, preserving the intended window structure for the shape zoo.
- Around line 92-109: The probe startup in the tmux pane loop relies on fixed
sleeps and a pane_current_command check that can misclassify valid bash panes,
so replace the shell-typing path with a deterministic tmux-driven launch for the
probe. Update the logic around the pane iteration and probe launch in the
remote-tmux-shape-zoo.sh flow so the probe starts directly through tmux commands
rather than sending keystrokes into shells, and remove the retry/sleep-based
nudge loop entirely.
In `@Sources/RemoteTmuxLayoutContainer.swift`:
- Around line 62-71: Remove the direct RemoteTmuxWindowMirror dependency from
pane rows in RemoteTmuxLayoutContainer and RemoteTmuxProportionalSplit so each
RemoteTmuxPaneLeaf is driven by immutable pane snapshot data plus explicit
action closures. Update the ForEach content to pass only the pane state needed
for rendering and route focus/requestSplit through callbacks from the parent
instead of reading mirror.activePaneId inside the leaf. Keep the mirror owned at
a higher level and prevent it from being captured by every pane subtree.
In `@Sources/RemoteTmuxWindowMirrorView.swift`:
- Around line 72-91: The sizing retry in pushClientSize(pointSize:) is still
time-based via ContinuousClock.sleep, which should be replaced by the actual
cell-size update signal. Use the existing GhosttyTerminalView
ghosttyDidUpdateCellSize notification, or the sizing-snapshot update path, to
trigger mirror.updateClientSize() when cell metrics change instead of looping on
a fixed delay. Keep the retry cancellation in sizingRetryTask, and wire the
update through the mirror/client-size flow so the render path becomes
event-driven rather than timer-driven.
---
Duplicate comments:
In `@Sources/TerminalController.swift`:
- Line 2041: `system.capabilities` is still advertising DEBUG-only tmux verbs in
release builds; move `remote.tmux.test_exec` and `remote.tmux.test_set_frame`
out of the always-on `methods` list in `TerminalController` and append them only
inside the existing `#if DEBUG` block alongside `Self.v2DebugMethodNames`, so
the advertised capabilities match the `socketWorkerV2Response` dispatch cases.
🪄 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: c7eca02f-cf86-42a6-918a-24851f594bb0
📒 Files selected for processing (33)
.github/workflows/test-e2e.ymlPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Sizing.swiftSources/RemoteTmuxControlCommandKind.swiftSources/RemoteTmuxControlConnection.swiftSources/RemoteTmuxControlMessage.swiftSources/RemoteTmuxControlStreamParser.swiftSources/RemoteTmuxController.swiftSources/RemoteTmuxHost.swiftSources/RemoteTmuxLayoutContainer.swiftSources/RemoteTmuxMirrorFrames.swiftSources/RemoteTmuxMirrorGeometry.swiftSources/RemoteTmuxPaneHeader.swiftSources/RemoteTmuxSSHTransport.swiftSources/RemoteTmuxSessionMirror.swiftSources/RemoteTmuxWindow.swiftSources/RemoteTmuxWindowMirror.swiftSources/RemoteTmuxWindowMirrorView.swiftSources/TerminalController+RemoteTmux.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/RemoteTmuxAuthTests.swiftcmuxTests/RemoteTmuxControlParserTests.swiftcmuxTests/RemoteTmuxMirrorFeedForwardTests.swiftcmuxTests/RemoteTmuxMirrorGeometryTests.swiftcmuxUITests/RemoteTmuxSizingUITests.swiftscripts/reload.shscripts/remote-tmux-e2e-ssh-shim-check.shscripts/remote-tmux-e2e-ssh-shim.shscripts/remote-tmux-shape-zoo.shscripts/remote-tmux-width-probe.shskills/cmux-testing/SKILL.mdskills/cmux-testing/references/remote-tmux-sizing-e2e.md
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 `@cmuxUITests/RemoteTmuxSizingUITests.swift`:
- Around line 483-489: The readiness check in RemoteTmuxSizingUITests should not
rely only on perPane.allSatisfy because tmux(_) trims trailing empty output,
letting a trailing unset `@probe_alive` pane slip through. Update the gate around
the tmux("list-panes"... ) loop to verify the expected pane count as well as all
values being "1", using the existing tmux(_:) helper and the sessionName:`@0`
probe output so a missing final probe cannot pass early.
🪄 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: 1ce1d226-3a90-4caf-8762-e6c562e6821c
📒 Files selected for processing (27)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+RuntimeLifecycle.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Sizing.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftResources/Localizable.xcstringsSources/App/CmuxMainWindow.swiftSources/RemoteTmuxControlCommandKind.swiftSources/RemoteTmuxControlConnection.swiftSources/RemoteTmuxLayoutContainer.swiftSources/RemoteTmuxLayoutNode.swiftSources/RemoteTmuxMirrorGeometry.swiftSources/RemoteTmuxPaneHeader.swiftSources/RemoteTmuxSessionMirror.swiftSources/RemoteTmuxWindowMirror.swiftSources/RemoteTmuxWindowMirrorView.swiftSources/TerminalController+DebugMethodNames.swiftSources/TerminalController+RemoteTmux.swiftSources/TerminalController.swiftSources/TerminalWindowPortal.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/RemoteTmuxMirrorFeedForwardTests.swiftcmuxTests/RemoteTmuxMirrorGeometryTests.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxUITests/RemoteTmuxSizingUITests.swiftscripts/cmux-spin-watchdog.shscripts/remote-tmux-shape-zoo.shscripts/remote-tmux-width-probe.sh
💤 Files with no reviewable changes (2)
- Sources/RemoteTmuxPaneHeader.swift
- cmux.xcodeproj/project.pbxproj
|
Pre-merge check fixes in 53238f1: placement chrome is now a one-pass fold threaded through On the package-boundaries check: extracting the remote-tmux connection/geometry into a SwiftPM target is the right end state, but doing it inside this PR would mean loosening access control across a ~2000-line type immediately after end-to-end validation. Planned as the first follow-up PR. @codex review |
@ejc3 I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 27 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
|
I'll kick off a fresh review of the latest commit now. ✏️ Learnings added
✅ 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)
cmuxUITests/RemoteTmuxSizingUITests.swift (1)
794-808: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winChunk-level UTF-8 decode can drop response bytes.
The read loop decodes each 8192-byte chunk independently via
String(bytes:encoding:.utf8). If a response ever crosses the buffer boundary mid multi-byte sequence, that chunk decodes toniland is silently discarded, corruptingaccumulatorand yielding a spuriousnilfrom a socket call (a flaky-test source that's hard to trace). AccumulatingDataand decoding once past the newline avoids it.🐛 Proposed fix: accumulate bytes, decode after framing
- var buffer = [UInt8](repeating: 0, count: 8192) - var accumulator = "" - let deadline = Date().addingTimeInterval(65) - while Date() < deadline { - let count = Darwin.read(fd, &buffer, buffer.count) - guard count > 0 else { break } - if let chunk = String(bytes: buffer[0..<count], encoding: .utf8) { - accumulator.append(chunk) - if let newline = accumulator.firstIndex(of: "\n") { - return String(accumulator[..<newline]) - } - } - } - return accumulator.isEmpty ? nil : accumulator.trimmingCharacters(in: .whitespacesAndNewlines) + var buffer = [UInt8](repeating: 0, count: 8192) + var accumulator = Data() + let deadline = Date().addingTimeInterval(65) + while Date() < deadline { + let count = Darwin.read(fd, &buffer, buffer.count) + guard count > 0 else { break } + accumulator.append(contentsOf: buffer[0..<count]) + if let newline = accumulator.firstIndex(of: UInt8(ascii: "\n")) { + return String(decoding: accumulator[..<newline], as: UTF8.self) + } + } + return accumulator.isEmpty ? nil : String(decoding: accumulator, as: UTF8.self).trimmingCharacters(in: .whitespacesAndNewlines)🤖 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 `@cmuxUITests/RemoteTmuxSizingUITests.swift` around lines 794 - 808, The read loop in the helper that returns the first line from the socket is decoding each buffer chunk independently, which can drop bytes when a UTF-8 sequence spans a boundary. Update the logic in this read helper to accumulate raw bytes first, detect the newline framing on the byte buffer, and only then decode the complete response once; this will prevent silent chunk loss and flaky nil results from the socket read path.
🤖 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 `@cmuxUITests/RemoteTmuxSizingUITests.swift`:
- Around line 794-808: The read loop in the helper that returns the first line
from the socket is decoding each buffer chunk independently, which can drop
bytes when a UTF-8 sequence spans a boundary. Update the logic in this read
helper to accumulate raw bytes first, detect the newline framing on the byte
buffer, and only then decode the complete response once; this will prevent
silent chunk loss and flaky nil results from the socket read path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f969b95a-5e5a-4a76-85d0-33347b927121
📒 Files selected for processing (8)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+RuntimeLifecycle.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftSources/RemoteTmuxMirrorGeometry.swiftSources/RemoteTmuxSessionMirror.swiftSources/TerminalController+RemoteTmux.swiftSources/TerminalController+RemoteTmuxTestSupport.swiftcmux.xcodeproj/project.pbxprojcmuxUITests/RemoteTmuxSizingUITests.swift
💤 Files with no reviewable changes (1)
- Sources/TerminalController+RemoteTmux.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53238f18da
ℹ️ 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".
|
Architecture Rethink disposition (2a3088c): Retry loop + readiness observers: removed (earlier commits). Initial sizing rides two direct surface events — Redraw-kick gap timer: event-gated redesign was built, validated green end to end, and withdrawn under adversarial review. The SIGWINCH the kick forces is delivered by the pane PTY ioctl, which tmux defers behind internal resize coalescing that emits nothing observable to control clients — layout publications confirm the wrong fact (layout recomputation is immediate), land inside the coalescing window on fast links so the shrink/restore pair collapses to net-zero, and per-window confirmation predicates admit spurious matches from unrelated windows already at the shrunken height. The full analysis now lives on the constant so the timer can't be "cleaned up" into that trap later. Size-send debounces: rate limiters, not correctness dependencies — the ledger is written synchronously before any deferral, dedup makes late sends idempotent, and the reconnect reseed replays the ledger. Reply-gated coalescing self-clocks to the control channel RTT and would reinstate the SIGWINCH storm the debounce absorbs; the layout-settle oscillation has no terminating event to gate on. Also documented in place. Reconnect backoff: deliberate network retry policy, unchanged. Every timer that correctness rode on is gone; the two that remain are rate limiters with their non-substitutability documented at the declaration. |
|
Added docs/remote-tmux-sizing-timers.md (f92a5b5) documenting why the two remaining sizing timers are load-bearing — the redraw-kick gap (SIGWINCH is a pane PTY ioctl deferred behind tmux's own resize coalescing, which emits nothing observable, so no control event can gate the restore) and the size-send debounce (a rate limiter the ledger + dedup + reconnect reseed make correctness-neutral). Includes a by-hand exploration with its confounds spelled out. On the question of a regression test: I attempted one and concluded against it, corroborated by an independent review. The kick's effect is a SIGWINCH below cmux's observable control surface — an e2e that observes it reliably requires the recorder to be the foreground process in the exact displayed mirrored pane and is inherently flaky around OS signal delivery, and a cheap command-shape unit test would pass for the broken event-gated design too (shrink-then-restore is identical; only the sub-coalescing-window timing differs). Per the repo's test-determinism and no-test-seam policies, a flaky or false-confidence test is worse than the documented invariant. The protection is the doc + the constant comment that points at it. @codex review |
@ejc3 I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 27 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd remote-tmux-sizing-timers.md
echo "---"
fd remote-tmux-sizing-timers.md --exec cat {}Length of output: 5690 The image attached (labeled "PR description image: broken_master") shows the same tmux "sizing" mirrored-window debug UI as in the PR description: a dark terminal window titled "sizing" with tabs
All three checks show ✓, and a red "THIS IS A DEV BUILD" badge is visible bottom-left outside the terminal content. This confirms PTY dimensions match tmux's pane/window/client width calculations across all three panes in this "nested" shape.
I'll factor this into review of the current diff. ✏️ Learnings added
✅ Action performedReview finished.
|
|
The pre-merge panel re-expanded to 6 on this pass. Nothing in the code changed since it last showed 2 —
|
|
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". |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
…eting suites (#8427) * tests: drain all paneRects in programmaticMirrorReorder… (broken by #7315) #7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched. * tests: drain post-#7315 follow-up commands in RemoteTmuxWindowReorderTests #7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in #7315. * tests: address review findings on the mirror/reorder test fixups From the CodeRabbit/Greptile pass: - drainLeadingOther replied to every paneRects with a hardcoded `%0`; a re-published @2/@3 needs its own pane id (the `windowId * 10` convention publishWindows stages), or its pending layout can't publish. - reorderPending filtered incidentals globally, so a paneRects landing BETWEEN two list-windows (an ordering anomaly) would be elided and the equality assertion would still pass. Trim only TRAILING incidental follow-ups; an interleaved one now survives and fails the assertion. - The mirror-targeting rects drain iterated a stale snapshot while each reply consumes the FIFO head, so an incidental preceding a fetch could mis-correlate pane data. Drain strictly from the head and stop at the first correlated command. * tests: stop the reorder drains from swallowing correlated commands Both drain helpers replied to whatever sat at the FIFO head, so a `listWindows` or `windowReorder` arriving early was consumed with an empty reply and its later positional result mis-correlated — the failure the drains exist to prevent. Each now answers only the incidental follow-ups (`paneRects`, `.other`) and stops at the first correlated command. `drainLeadingOther` also gains the bounded guard the other drains already had. --------- Co-authored-by: ejc3 <ejc3@users.noreply.github.com> Co-authored-by: Austin Wang <austinwang115@gmail.com>
Summary
Multi-pane mirrored tmux windows can render panes a column narrower than the width tmux assigned them. Repro on main: mirror a session with a 3-pane split (
cmux ssh-tmux <host>), run a program that paints full-width lines, and resize the cmux window through a few widths — at many widths one pane's full-width lines wrap by one character, and some resizes leave a pane permanently mismatched until the next resize.Root cause: the client size reported to tmux is derived by dividing the mirror's outer pixel area by the cell size, which counts local divider and padding pixels as terminal columns, so tmux lays out more columns than the panes have pixels. Nothing then constrains a pane's rendered grid to the width tmux assigned, so where the extra columns land is layout-dependent.
Symptom of the bug
This shows how at certain window sizes, tmux believes a that the pane is one column bigger than it actually is. That results in the incorrect wrap you see on the left side pane. The pull request's purpose is to fix that and make the whole thing simpler and testable.
old_master_broken_trimmed.mov
The same defect frozen in a frame — main rendering a mirrored session with the left pane's full-width lines wrapping by one column:
Demo Video
A tour over an eight-shape layout zoo (every pane running a width probe that paints a full-width ruler, a bottom-row sentinel, and a live PTY-vs-tmux size check):
pane-border-status top, where each strip carries tmux's own header text.pane-border-statustoggled off and back on mid-session — panes reclaim and yield the title rows, re-settling exactly each time.cmux-ultimate-demo-trimmed.mov
Design
One authority per quantity, feed-forward in both directions:
%layout-changeecho of our own push recomputes to the identical value and dedups to silence — there is no feedback loop to manage.refresh-client -C '@id:WxH'), deduped per window, reseeded after reconnect, with a session-wide fallback for servers that reject the@id:form. Hidden tabs claim their size once at attach (the first per-window pin drops unclaimed windows to tmux's 80×24 default) and re-own it when selected. Zoom renders the visible tree without touching the pushed size or panel lifecycle.Alternative considered: reconciling the render after the fact — measure what each surface actually renders and bring tmux to it (report the summed rendered grid as the client size, and
resize-panewhenever tmux assigns a column a pane's pixels can't render). That direction loses on two grounds. (1) It creates a cycle with two independent rounding schemes inside it — the view's pixel division and tmux's integer cell division; at some pixel widths the two have no common fixed point, so any policy that re-reads renders after a reflow either oscillates by a column (SIGWINCH-storming every pane) or must be rate-limited into eventual silence at a wrong answer. (2) Per-pane corrections write tmux's layout ratios, which are user state shared with every client of the session; grids read mid-resize feed transient geometry back as permanent ratio changes. Sizing from pixels + structure only, and rendering tmux's layout verbatim, removes the cycle instead of managing it.How the system works
Data flow on main (before)
One loop, three writers, and measurements feeding back into inputs:
Three problems live in that picture. The pushed size counts non-terminal
pixels, so tmux lays out more columns than the panes can render. The render
divides space proportionally instead of using tmux's assignment, so where the
lost column lands is layout-dependent. And rendered grids feed back into
sizing, so the system can oscillate: push → tmux re-lays-out → render moves →
push again.
Data flow now
Two one-way paths that never read each other's outputs, plus a quarantine
that keeps unverified geometry away from the render:
Who drives what:
not tmux events, not rendered grids, not reconcile.
draws exactly the rectangles tmux reports; pane ratios are user state and
are never written back.
%layout-changeecho of our own push recomputes to the identical size anddedups to silence. No feedback loop exists to manage.
A window resize, end to end: pixels change →
frecomputes cols×rows frompixels + structure → one deduped
refresh-client -C '@id:WxH'per window →tmux re-divides its panes →
%layout-changearrives, is parsed andquarantined → one
list-panesfetch returns the real rects → the patchedtree is published atomically and the render moves once, to verified geometry.
If layouts arrive faster than fetches return, newer layouts coalesce onto the
pending entry (generation-tagged; stale replies are discarded) and observers
keep the last verified tree until the next verified one — the render never
shows an intermediate guess, so there is nothing to bounce.
Geometry: the two header modes
tmux may or may not draw a title row above each pane (
pane-border-status).Both modes must render faithfully and settle, so the vertical chrome is
tmux's own rows plus at most one synthetic band:
Mode 1 — tmux gives us the header (
pane-border-status topis set).tmux carves a real title row above every pane, inside the window's own cell
budget. cmux adds nothing: every pane sits at y ≥ 1, so no synthetic band is
reserved and the full row budget is pushed. The strips render tmux's title
rows as a hairline carrying each pane's
index "title"label and theactive-pane dot:
The trap in this mode: tmux's layout STRING still reports the pre-title tree
(a pane claimed at y=0 with 62 rows actually displays at y=1 with 61 rows).
That is why placement never trusts the string's rects and waits for
list-panes— placing panes off the string is visibly one row wrong, andrendering the string first and correcting after made panes bob. The
quarantine makes that impossible by construction rather than by timing.
Mode 2 — no tmux headers (default;
pane-border-statusoff).tmux's window is just panes plus separator rows — no title rows, and a stock
tmux displays no titles anywhere. cmux matches that: strips are bare
hairlines, and the ONE cell-high band it reserves across the top (subtracting
exactly one row from the pushed budget) exists so the active-pane dot always
has a home that can never overlap pane text:
The band is uniform across every branch of the split tree, so each branch
loses the same single row and the bottoms of adjacent columns stay aligned
regardless of how many panes stack in each — the row budget is a function of
the tree's structure only, not its depth (test-pinned).
In both modes the strips are the only chrome: nothing ever draws over pane
text and the dividers are one device pixel like tmux's own borders. Header
text is tmux's, verbatim: each label is the pane's EXPANDED
pane-border-format(custom formats included), seeded by the samelist-panesfetch that publishes the geometry and kept live by a per-panecontrol-mode subscription — a program retitling its pane updates the strip
at the same moment a native tmux client's border would redraw. When headers
are off, no text renders at all, because that is what tmux shows. During a
resize the transient render reserves the same strip rows with the last-known
labels pinned, so the chrome never blinks while panes re-divide.
Testing
The defect class here lives in the interaction between stages (pixels → pushed size → tmux's assignment → imposed frames → rendered grid), so the tests are layered: pure property tests on the math, contract tests on the state machine around it, and an end-to-end suite that drives the whole loop against a real tmux server and asserts on the app's own introspection — never on screenshots.
Unit and property suites (
cmux-unitscheme):RemoteTmuxMirrorGeometryTests— property tests over randomized layout trees. The client-size function is invariant under re-assigning different sizes onto the same structure (it may depend on structure only); computed frames sit on integer device-pixel rails with each pane's split-axis extent in[needed, needed+1]px (the +1 boundary bias); the chrome fold matches per-shape expectations; minimum floors hold.RemoteTmuxMirrorFeedForwardTests— the mirror's sizing contract: the push is a pure function of container pixels + structure (re-applying tmux layouts never changes it); hidden mirrors write exactly one initial claim; reconcile never pushes (tmux events are not push triggers — the invariant that keeps the system echo-silent); frames are imposed only when tmux's layout matches the computed size for the current pixels, otherwise the render falls back to the proportional transient; zoom changes neither panel lifecycle nor the pushed size.RemoteTmuxConnectionWindowSizingTests— per-window dedup on the connection, the%error→ session-wide fallback for servers without the@id:WxHform, and the reconnect reseed table.End-to-end suite (
cmuxUITests/RemoteTmuxSizingUITests) — a real tmux server, the real app, the real ssh transport, hermetically: the app builds its own throwaway tmux on an isolatedTMUX_TMPDIRthrough a DEBUG-only socket verb (the XCUITest runner is sandboxed and cannot touch/tmpitself), and ssh is replaced by the checked-inscripts/remote-tmux-e2e-ssh-shim.shviaCMUX_REMOTE_TMUX_SSH_FOR_TESTING— which reproduces the three ssh behaviors the transport depends on (the remote shell re-splits the quoted command, a pty exists only under-t/-tt, and stderr stays separate so probe-failure classification works). Window sizes and tab selection are driven over the control socket (NSWindow.setFramewith read-back,surface.focus) rather than AX mouse gestures, so the suite is deterministic on any desktop, including headless CI.Scenarios: attach settles stable and coherent; a shape sweep (even/nested/rows/grid/deep/six-column/main-horizontal) must render every pane per the contract — exact on the immediate split's axis, ≥ assigned on the fill axis — at every window size; a window-size sweep re-converges at each width with pane ratios preserved; a co-attached client's
resize-paneheals. The oracle reads theremote.tmux.pane_gridsdebug verb (per-pane assigned vs rendered grids plus the sizing inputs), on top of tmux-side stability and coherence checks. The suite also guards its own blind spots: every resize asserts the read-back window frame matched the request, the width sweep asserts pushed columns strictly grow with the window, and a settle check fails if the selected window has no mirror entry — so a regression that stops mirrors from existing cannot pass on tmux-side checks alone.bash scripts/remote-tmux-e2e-ssh-shim-check.shexercises the shim against every ssh invocation shape the transport makes (ControlMaster ops, one-shot probes with stderr classification, the-ttcontrol stream with a live stdin dialogue, SIGTERM cleanup) in seconds.On CI, dispatch
test-e2e.ymlwithtest_filter=RemoteTmuxSizingUITests(records video; each lab pane in the first window runsscripts/remote-tmux-width-probe.sh, which paints a PTY-wide ruler, a bottom-row sentinel, and a two-axis size check — a human-readable narration of the machine oracle).Manual verification:
scripts/remote-tmux-shape-zoo.sh <ssh-host>builds the same shape zoo on a real remote server's tmux over a single interactive ssh connection (probes running in every pane), ready to mirror withcmux ssh-tmux <ssh-host>. Or runscripts/remote-tmux-width-probe.shinside any mirrored pane: a wrapped ruler (surface narrower than the PTY), a clipped bottom sentinel, or a ✗ at rest is a sizing bug, and the probe's resize log gives the transition history for a report.Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by CodeRabbit
Summary
Note
High Risk
Large changes to remote tmux control connection, mirror sizing/rendering, and terminal surface resize reporting—core session mirroring behavior with complex timing and tmux protocol edge cases.
Overview
Fixes multi-pane remote tmux mirrors reporting too many columns to tmux (divider/padding counted as grid) and rendering panes with proportional splits instead of tmux’s assigned cells.
Sizing is now feed-forward:
RemoteTmuxMirrorGeometryderives the pushedrefresh-client -Csize from container device pixels, the base layout structure, and measured ghostty cell/padding constants—never from rendered grids or tmux-assigned geometry—so layout echoes dedupe instead of looping. Per-windowrefresh-client -C '@id:WxH'(with session-wide fallback on%error) replaces a single session size; hidden tabs get a one-time size claim at attach.Geometry no longer trusts layout strings:
%layout-change/list-windowstrees sit inpendingLayoutsuntil a generation-taggedlist-panesreply patches real rects (including zoom/visible layout andpane-border-statusoffsets). Initial attach batches all windows into one publish so tab order is deterministic.Rendering switches to exact imposed frames (
RemoteTmuxImposedFrameLayout) when layout matches the computed size, with a proportional transient mode during settle; chrome is tmux-like hairline strips with optional livepane-border-formatlabels and an active-pane dot (pane header toolbar removed). Manual-I/O surfaces reportTerminalSurfaceRawSizingSampleon every applied resize, with attach-time flush for off-window applies.Also adds
remote.tmux.pane_gridsintrospection, DEBUG sizing test socket verbs + optional SSH shim for hermetic e2e, main-window guard againstNSHostingViewcontent-driven resize, and CI tmux install for e2e.Reviewed by Cursor Bugbot for commit 0f6ac5a. Bugbot is set up for automated code reviews on this repo. Configure here.