Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 76 additions & 9 deletions cmux-linux/src/socket.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1655,10 +1655,10 @@ fn handleSurfaceClearHistory(_: Allocator, _: json.Value) []const u8 {
/// surface.action / tab.action — apply a tab-level action to a surface.
///
/// Mirrors macOS `v2TabAction` (Sources/TerminalController.swift). Linux
/// implements the trivial property-mutation actions (rename, clear_name,
/// pin, unpin, mark_read, mark_unread). Tab-relative close/new actions
/// (close_left, close_right, close_others, new_terminal_right,
/// new_browser_right, reload, duplicate) are not yet wired and will return
/// implements property-mutation actions (rename, clear_name, pin, unpin,
/// mark_read, mark_unread) and relative-close actions (close_left,
/// close_right, close_others). Remaining actions (new_terminal_right,
/// new_browser_right, reload, duplicate) are not yet wired and return
/// an `unsupported` error so callers can detect parity gaps.
fn handleSurfaceAction(alloc: Allocator, params: json.Value) []const u8 {
const tm = getTabManager() orelse return "{\"error\":\"no tab manager\"}";
Expand Down Expand Up @@ -1756,13 +1756,80 @@ fn handleSurfaceAction(alloc: Allocator, params: json.Value) []const u8 {
) catch "{}";
}

// ── Close relative actions ──────────────────────────────────────
// close_left / close_right / close_others: close sibling surfaces
// relative to the anchor in ordered_panels. Pinned panels are
// skipped (matching macOS).
if (std.mem.eql(u8, action, "close_left") or
std.mem.eql(u8, action, "close_right") or
std.mem.eql(u8, action, "close_others"))
{
// Find anchor position in ordered list.
var anchor_idx: ?usize = null;
for (ws.ordered_panels.items, 0..) |id, i| {
if (id == target_id) {
anchor_idx = i;
break;
}
}
const idx = anchor_idx orelse return "{\"error\":\"surface not in panel order\"}";

// Collect IDs to remove (snapshot before mutation).
var to_close = std.ArrayList(u128).init(alloc);
defer to_close.deinit();
var skipped_pinned: usize = 0;

for (ws.ordered_panels.items, 0..) |id, i| {
if (id == target_id) continue; // never close anchor

const dominated = if (std.mem.eql(u8, action, "close_left"))
i < idx
else if (std.mem.eql(u8, action, "close_right"))
i > idx
else // close_others
true;

if (!dominated) continue;

if (ws.panels.get(id)) |p| {
if (p.is_pinned) {
skipped_pinned += 1;
continue;
}
}
to_close.append(id) catch continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent partial close on append failure

catch continue skips a panel that failed to be appended to to_close and lets the loop proceed, so the final closed count in the response will undercount however many panels were silently dropped. The workspace-level analogue on line 1282 uses catch break to at least stop collecting, which produces a cleaner (if still silent) failure. A more correct fix is to surface the error:

Suggested change
to_close.append(id) catch continue;
to_close.append(id) catch return "{\"error\":\"alloc failed\"}";

}

// Remove collected panels (split tree + panel map).
for (to_close.items) |id| {
if (ws.root_node) |root| {
ws.root_node = split_tree.closePane(ws.alloc, root, id);
}
ws.removePanel(id);
}

// Ensure focus stays on the anchor.
ws.focused_panel_id = target_id;

// Rebuild widget tree.
if (!isNoSurface()) {
if (ws.root_node) |new_root| {
ws.content_widget = split_tree.buildWidget(new_root);
}
}
if (window.getSidebar()) |sb| sb.refresh();

return std.fmt.allocPrint(
alloc,
"{{\"action\":\"{s}\",\"surface_id\":\"{s}\",\"closed\":{d},\"skipped_pinned\":{d}}}",
.{ action, panel_id_slice, to_close.items.len, skipped_pinned },
) catch "{}";
}

// Recognized but not yet implemented on Linux. Returning a structured
// error lets callers distinguish "wrong call" from "platform gap".
// `action` is user input — escape it via writeJsonString.
if (std.mem.eql(u8, action, "close_left") or
std.mem.eql(u8, action, "close_right") or
std.mem.eql(u8, action, "close_others") or
std.mem.eql(u8, action, "new_terminal_right") or
if (std.mem.eql(u8, action, "new_terminal_right") or
std.mem.eql(u8, action, "new_browser_right") or
std.mem.eql(u8, action, "reload") or
std.mem.eql(u8, action, "duplicate"))
Expand All @@ -1783,7 +1850,7 @@ fn handleSurfaceAction(alloc: Allocator, params: json.Value) []const u8 {
w.writeAll("{\"error\":\"unsupported action\",\"action\":") catch
return "{\"error\":\"unsupported action\"}";
writeJsonString(w, action) catch return "{\"error\":\"unsupported action\"}";
w.writeAll(",\"supported\":[\"rename\",\"clear_name\",\"pin\",\"unpin\",\"mark_read\",\"mark_unread\"]}") catch
w.writeAll(",\"supported\":[\"rename\",\"clear_name\",\"pin\",\"unpin\",\"mark_read\",\"mark_unread\",\"close_left\",\"close_right\",\"close_others\"]}") catch
return "{\"error\":\"unsupported action\"}";
return buf.toOwnedSlice(alloc) catch "{\"error\":\"unsupported action\"}";
}
Expand Down
159 changes: 159 additions & 0 deletions tests_v2/test_surface_action_close_variants.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
#!/usr/bin/env python3
"""Socket API: surface.action close_left / close_right / close_others.

Verifies relative-close actions remove the correct sibling surfaces,
preserve the anchor, skip pinned panels, and return the right counts.
Runs against the Linux daemon in CMUX_NO_SURFACE headless mode.
"""

import os
import sys
import time
from pathlib import Path

sys.path.insert(0, str(Path(__file__).parent))
from cmux import cmux, cmuxError

SOCKET_PATH = os.environ.get("CMUX_SOCKET", "/tmp/cmux-debug.sock")


def _surface_ids(c: cmux, ws: str) -> list[str]:
"""Return ordered surface IDs for the workspace."""
res = c._call("surface.list", {"workspace_id": ws}) or {}
surfaces = res.get("surfaces") or []
return [str(s["id"]) for s in surfaces]


def _add_surfaces(c: cmux, n: int) -> list[str]:
"""Split right N times and return the new surface IDs."""
ids = []
for _ in range(n):
sid = c.new_split("right")
time.sleep(0.05)
if sid:
ids.append(sid)
return ids


def _close_action(c: cmux, ws: str, surface_id: str, action: str) -> dict:
return c._call(
"surface.action",
{"workspace_id": ws, "surface_id": surface_id, "action": action},
) or {}


def main() -> int:
ws = None
with cmux(SOCKET_PATH) as c:
try:
ws = c.new_workspace()
c.select_workspace(ws)
time.sleep(0.1)

# ── Setup: 5 surfaces: [s0, s1, s2, s3, s4] ──────────────
ids_before = _surface_ids(c, ws)
s0 = ids_before[0]
_add_surfaces(c, 4) # creates s1..s4
all5 = _surface_ids(c, ws)
if len(all5) < 5:
raise cmuxError(f"expected >= 5 surfaces, got {len(all5)}")
s0, s1, s2, s3, s4 = all5[0], all5[1], all5[2], all5[3], all5[4]

# ── close_right from s2 → removes s3, s4 ─────────────────
res = _close_action(c, ws, s2, "close_right")
if res.get("action") != "close_right":
raise cmuxError(f"close_right: bad action echo: {res}")
if res.get("closed") != 2:
raise cmuxError(f"close_right: expected closed=2, got {res}")

remaining = _surface_ids(c, ws)
if s3 in remaining or s4 in remaining:
raise cmuxError(f"close_right: s3/s4 still present: {remaining}")
if s2 not in remaining:
raise cmuxError(f"close_right: anchor s2 removed: {remaining}")

# Now have [s0, s1, s2]. Add 2 more for next test.
_add_surfaces(c, 2)
after_add = _surface_ids(c, ws)
if len(after_add) < 5:
raise cmuxError(f"expected >= 5 surfaces after re-add, got {len(after_add)}")
s_a, s_b, s_c, s_d, s_e = (
after_add[0], after_add[1], after_add[2],
after_add[3], after_add[4],
)

# ── close_left from s_c → removes s_a, s_b ───────────────
res2 = _close_action(c, ws, s_c, "close_left")
if res2.get("action") != "close_left":
raise cmuxError(f"close_left: bad action echo: {res2}")
if res2.get("closed") != 2:
raise cmuxError(f"close_left: expected closed=2, got {res2}")

remaining2 = _surface_ids(c, ws)
if s_a in remaining2 or s_b in remaining2:
raise cmuxError(f"close_left: s_a/s_b still present: {remaining2}")
if s_c not in remaining2:
raise cmuxError(f"close_left: anchor s_c removed: {remaining2}")

# Now have [s_c, s_d, s_e]. Test close_others from s_d.
# ── close_others from s_d → removes s_c, s_e ─────────────
res3 = _close_action(c, ws, s_d, "close_others")
if res3.get("action") != "close_others":
raise cmuxError(f"close_others: bad action echo: {res3}")
if res3.get("closed") != 2:
raise cmuxError(f"close_others: expected closed=2, got {res3}")

remaining3 = _surface_ids(c, ws)
if s_c in remaining3 or s_e in remaining3:
raise cmuxError(f"close_others: siblings still present: {remaining3}")
if s_d not in remaining3:
raise cmuxError(f"close_others: anchor removed: {remaining3}")

# ── Pinned surfaces are skipped ───────────────────────────
# Add 2 more, pin one, then close_others should skip it.
_add_surfaces(c, 2)
pin_set = _surface_ids(c, ws)
if len(pin_set) < 3:
raise cmuxError(f"expected >= 3 surfaces for pin test, got {len(pin_set)}")

anchor_pin = pin_set[1] # middle
pinned_target = pin_set[0] # left

# Pin the left surface
c._call(
"surface.action",
{"workspace_id": ws, "surface_id": pinned_target, "action": "pin"},
)

res4 = _close_action(c, ws, anchor_pin, "close_others")
if res4.get("skipped_pinned", 0) < 1:
raise cmuxError(f"close_others should skip pinned, got {res4}")
remaining4 = _surface_ids(c, ws)
if pinned_target not in remaining4:
raise cmuxError(
f"pinned surface {pinned_target} was closed: {remaining4}"
)

# ── No-op close_left at position 0 ────────────────────────
leftmost = _surface_ids(c, ws)[0]
res5 = _close_action(c, ws, leftmost, "close_left")
if res5.get("closed") != 0:
raise cmuxError(f"close_left at idx 0 should close 0: {res5}")
Comment on lines +137 to +141

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing symmetric no-op test for close_right at last index

The no-op case for close_left at position 0 is tested (lines 137-141), but there is no corresponding test for close_right when the anchor is already the rightmost surface (expected closed: 0). Adding a symmetric case would fully mirror the existing no-op coverage and guard against an off-by-one if i > idx is ever changed to i >= idx.


finally:
if ws is not None:
try:
c.close_workspace(ws)
except Exception:
pass

print("PASS: surface.action close_left / close_right / close_others")
return 0


if __name__ == "__main__":
try:
raise SystemExit(main())
except cmuxError as exc:
print(f"FAIL: {exc}", file=sys.stderr)
raise SystemExit(1)
Loading