From 2cf3af780353cdb424e55f9e0a01e7c7a39ae2d8 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 9 Aug 2026 14:05:33 +0800 Subject: [PATCH] fix(cli): drop `tab group`, and shore up the workspace deletion path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tab group` wrote a field the GUI recomputes on every sidebar render from the tab's repository root, so a heading set from the CLI was overwritten before anyone could read it. The command is gone; the read-only GROUP column stays, and the docs now say where its value comes from. Alongside it: - `tty7 new --open` tells a failed request apart from a missing GUI instead of reporting both as "no GUI is running", and its warnings carry the `tty7:` prefix the rest of the CLI uses. Neither is an exit code: the workspace exists by the time we ask. - The switcher measures unclaimed workspaces against the rows already listed rather than against the store that put them there, so this window's own workspace cannot show up twice. - `on_workspace_deleted` gets a test for its destructive half — the one that erases state only this client holds. - The empty-tree write-back says why it covers remote workspaces too, and both unreachable `WorkspaceDeleted` arms say they are unreachable. - `TabView` is no longer serialisable: it is a reading of the machine tree, and both sides that want one have the tree already. - New tests for the `tab ls` table and its JSON. --- crates/tty7-cli/src/cli.rs | 11 --- crates/tty7-cli/src/commands.rs | 113 +++++++++++++++----------- crates/tty7-core/src/core/tab_view.rs | 7 +- skills/tty7/references/commands.md | 6 +- src/ui/switcher.rs | 9 ++ src/ui/tree_sync.rs | 79 +++++++++++++++++- 6 files changed, 158 insertions(+), 67 deletions(-) diff --git a/crates/tty7-cli/src/cli.rs b/crates/tty7-cli/src/cli.rs index 6edad54c..79bd3c7b 100644 --- a/crates/tty7-cli/src/cli.rs +++ b/crates/tty7-cli/src/cli.rs @@ -391,17 +391,6 @@ pub enum TabCmd { #[arg(value_name = "INDEX")] index: u64, }, - - #[command(about = "Put a tab in a sidebar group, or take it out of one")] - Group { - #[arg(value_name = "@TAB")] - tab: String, - #[arg( - value_name = "GROUP", - help = "The group to join; omit to leave whatever group it is in" - )] - group: Option, - }, } #[derive(Debug, Subcommand)] diff --git a/crates/tty7-cli/src/commands.rs b/crates/tty7-cli/src/commands.rs index f3561e8e..797c5ec0 100644 --- a/crates/tty7-cli/src/commands.rs +++ b/crates/tty7-cli/src/commands.rs @@ -85,7 +85,6 @@ pub fn execute(cli: Cli, ctx: &Context, backend: &mut dyn Backend) -> Result tab_close(&tab, backend), Some(Command::Tab(TabCmd::Rename { tab, name })) => tab_rename(&tab, name, backend), Some(Command::Tab(TabCmd::Move { tab, index })) => tab_move(&tab, index, backend), - Some(Command::Tab(TabCmd::Group { tab, group })) => tab_group(&tab, group, backend), Some(Command::Pane(PaneCmd::Ls { ws, all })) => pane_ls(ws.as_deref(), all, backend), Some(Command::Pane(PaneCmd::Close { target })) => { pane_close(target.as_deref(), ctx, backend) @@ -352,17 +351,34 @@ fn new_workspace(path: Option, open: bool, backend: &mut dyn Backend) -> })?; // Only when asked: a workspace made from a script has no business // stealing the screen, and the switcher lists it either way. - let opened = open - && matches!( - backend.control(ControlRequest::GuiOpen { - path: None, - workspace: Some(ws.id), - }), - Ok(ReplyOk::Bool(true)) - ); - if open && !opened { - eprintln!("no GUI is running on this machine; the workspace was made all the same"); - } + let opened = match open { + false => false, + true => match backend.control(ControlRequest::GuiOpen { + path: None, + workspace: Some(ws.id), + }) { + Ok(ReplyOk::Bool(opened)) => { + // The workspace exists by the time we ask, so an unreachable + // GUI is worth a word and not an exit code: failing here would + // read as "nothing was made". + if !opened { + eprintln!( + "tty7: no GUI is running on this machine; \ + the workspace was made all the same" + ); + } + opened + } + Ok(other) => bail!("the server answered GuiOpen with {other:?}"), + Err(error) => { + eprintln!( + "tty7: could not ask the GUI to open it ({error:#}); \ + the workspace was made all the same" + ); + false + } + }, + }; report( ws.id.to_string(), json!({ "id": ws.id.to_string(), "pane": pane, "opened": opened }), @@ -645,24 +661,6 @@ fn tab_move(tab: &str, index: u64, backend: &mut dyn Backend) -> Result report("", json!({ "tab": tab.to_string(), "to": index })) } -/// The GUI files tabs under headings in its sidebar, and that heading is a -/// field on the tab like any other. Until now only the GUI could write it, -/// so a tab the CLI made landed in the ungrouped pile with no way out. -fn tab_group(tab: &str, group: Option, backend: &mut dyn Backend) -> Result { - let addr = address::parse_tab(tab)?; - let machine = fetch_machine(backend)?; - let (workspace, tab) = resolve::tab(&machine, &addr)?; - let group = group - .map(|g| g.trim().to_string()) - .filter(|g| !g.is_empty()); - backend.control(ControlRequest::TabSetGroup { - workspace, - tab, - group: group.clone(), - })?; - report("", json!({ "tab": tab.to_string(), "group": group })) -} - fn pane_ls(explicit: Option<&str>, all: bool, backend: &mut dyn Backend) -> Result { if all { return pane_ls_all(backend); @@ -1510,28 +1508,47 @@ mod tests { to: 0, } ); + } - backend.control_calls.clear(); - run_cli(&["tty7", "tab", "group", "@1", " scm "], &ctx, &mut backend); - assert_eq!( - backend.control_calls[1], - ControlRequest::TabSetGroup { - workspace: api.id, - tab: api.tabs[0].id, - group: Some("scm".into()), - } + #[test] + fn tab_ls_names_an_unnamed_tab_and_shows_the_leaf_of_its_group() { + let mut backend = mock(); + backend.machine.workspaces[0].tabs[1].sidebar_group = Some("C:\\proj\\sub".into()); + + let out = run_cli( + &["tty7", "tab", "ls", "api"], + &Context::default(), + &mut backend, ); - backend.control_calls.clear(); - run_cli(&["tty7", "tab", "group", "@1"], &ctx, &mut backend); + // @1 was named; @2 was not, so it borrows the leaf of its cwd. The + // GROUP column is the heading's last segment, not the whole path. assert_eq!( - backend.control_calls[1], - ControlRequest::TabSetGroup { - workspace: api.id, - tab: api.tabs[0].id, - group: None, - }, - "no group named means leave the group" + human(out), + "TAB NAME GROUP PANES\n@1 build - 1\n@2 proj sub 2\n" + ); + } + + #[test] + fn tab_ls_json_keeps_the_literal_name_beside_the_label() { + let mut backend = mock(); + backend.machine.workspaces[0].tabs[1].sidebar_group = Some("C:\\proj\\sub".into()); + + let out = run_cli( + &["tty7", "tab", "ls", "api"], + &Context::default(), + &mut backend, + ); + + let Outcome::Report(report) = out else { + panic!("tab ls must report"); + }; + let tabs = report.json["tabs"].as_array().expect("tabs").clone(); + assert_eq!(tabs[1]["name"], Value::Null, "nobody named this tab"); + assert_eq!(tabs[1]["label"], "proj", "the table's stand-in travels too"); + assert_eq!( + tabs[1]["group"], "C:\\proj\\sub", + "the JSON keeps the whole heading the table abbreviates" ); } diff --git a/crates/tty7-core/src/core/tab_view.rs b/crates/tty7-core/src/core/tab_view.rs index f56f1fea..d98481bc 100644 --- a/crates/tty7-core/src/core/tab_view.rs +++ b/crates/tty7-core/src/core/tab_view.rs @@ -6,12 +6,13 @@ //! the machine tree. This is the reading of that tree, kept in one place so //! the CLI and the GUI name a tab the same way. -use serde::{Deserialize, Serialize}; - use crate::core::cli_agent::{AgentStatus, CLIAgent}; use crate::core::machine::{PaneRecord, TabId, Workspace}; -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +/// Deliberately not serialisable: it is a reading of the machine tree, and +/// both sides that want one have the tree already. Putting it on the wire +/// would be sending a conclusion where the evidence has already gone. +#[derive(Debug, Clone, PartialEq)] pub struct TabView { pub id: TabId, pub name: Option, diff --git a/skills/tty7/references/commands.md b/skills/tty7/references/commands.md index d68cdfd3..828694b2 100644 --- a/skills/tty7/references/commands.md +++ b/skills/tty7/references/commands.md @@ -205,7 +205,11 @@ resolve it immediately before use. A full tab UUID also works: `@`. | `tab close @TAB` | close the tab and every pane in it | `{"closed"}` | | `tab rename @TAB NAME` | name or rename | `{"tab","name"}` | | `tab move @TAB INDEX` | reposition within its workspace | `{"tab","to"}` | -| `tab group @TAB [GROUP]` | file it under a sidebar heading, or with no GROUP take it out of one | `{"tab","group"}` | + +GROUP is the heading the GUI's sidebar files the tab under, shown by its last +segment (`group` in the JSON is the whole value). Read-only from here: with the +default repo grouping the GUI recomputes it from the tab's working directory, +so anything written from outside would be overwritten on the next render. Almost no tab has a `name`: the GUI's tab strip reads OSC titles, which the machine tree never sees. So the NAME column — and `label` in the JSON — falls diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index 9852aee7..14109a1e 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -483,9 +483,18 @@ impl Tty7App { // person who ran `tty7 new` like nothing happened at all. They open // like any other row — the id in the tree is the id a window claims. if let Some(slot) = groups.iter().position(|g| g.key.is_empty()) { + // Measured against the rows already listed rather than against the + // store, which is what put them there: the block above lists this + // window's own workspace before the store has caught up with it, + // and two rows under one id would be two ways into one window. + let listed: Vec = groups + .iter() + .flat_map(|g| g.rows.iter().map(|r| r.id)) + .collect(); let app: &App = cx; let rows: Vec = crate::ui::machine_mirror::unclaimed_local_workspaces(app) .into_iter() + .filter(|ws| !listed.contains(&ws.id)) .map(|ws| Row { id: ws.id, name: ws.name, diff --git a/src/ui/tree_sync.rs b/src/ui/tree_sync.rs index 6a284f54..78aec760 100644 --- a/src/ui/tree_sync.rs +++ b/src/ui/tree_sync.rs @@ -1387,6 +1387,15 @@ fn finish_hydration( // A full window over an empty tree has to write itself back, whether // or not an edit was waiting: the machine is missing tabs this window // is showing, and nothing else would ever put them there. + // + // Deliberately not limited to this machine. An empty tree means one of + // two things and the answer is the same either way: locally the + // workspace was removed under the window (`ws rm`, or another client), + // and remotely the far end lost its records — a re-imaged box, a store + // that was wiped. Writing the window back is what a reattach is for. + // The panes it names may well be dead; the window already draws them + // that way, and a tab the user can close beats a tab that silently + // stops existing. if was_dirty || machine_was_empty { app.update(cx, |app, cx| sync_window(app, cx)); } @@ -1531,6 +1540,10 @@ pub(crate) fn on_layout_delta(cx: &mut App, host: HostId, key: &str, delta: Layo fn apply_to_mirror(mirror: &mut WsMirror, delta: &LayoutDelta) -> bool { match delta { + // Nothing here is about a workspace's tab list, so the mirror is + // already right. `WorkspaceDeleted` never reaches this far — + // `on_layout_delta` hands it to `on_workspace_deleted` and returns — + // and is listed only so a new delta cannot join this arm by accident. LayoutDelta::WorkspaceCreated { .. } | LayoutDelta::WorkspaceRenamed { .. } | LayoutDelta::WorkspaceTouched { .. } @@ -1617,10 +1630,10 @@ impl Tty7App { | LayoutDelta::WorkspaceTouched { .. } | LayoutDelta::WorkspaceRenamed { .. } | LayoutDelta::PaneFacts { .. } => true, - // Handled before the window is ever reached — a deletion is about - // whether this workspace still exists here at all, which is not a - // question one window's tab list can answer. See - // `on_workspace_deleted`. + // Unreachable: `on_layout_delta` hands a deletion to + // `on_workspace_deleted` and returns before any window is asked. A + // deletion is about whether this workspace still exists here at + // all, which is not a question one window's tab list can answer. LayoutDelta::WorkspaceDeleted => true, LayoutDelta::ActiveTabChanged { tab } => { if let Some(index) = index_of(&self.tabs, *tab) { @@ -1912,6 +1925,64 @@ mod tests { }); } + /// The destructive half of a deletion. It erases state only this client + /// holds — geometry, the label, a remote binding — so the fence in front of + /// it ("no window is showing this workspace") is the whole safety of it. + /// + /// The other half needs a live `Tty7App` in a real window to reach, so it + /// is not tested here; what it does is hydrate, which the hydration tests + /// cover, and it touches neither the store nor the registry. + #[gpui::test] + fn a_deletion_nothing_has_open_forgets_the_workspace_here_too(cx: &mut gpui::TestAppContext) { + use crate::core::session::{WindowView, WindowViews}; + + cx.update(|cx| { + // Removing a workspace saves the views, and a test has no business + // writing the real ones. + let _ = tty7_core::core::config::set_config_dir( + std::env::temp_dir().join(format!("tty7-deleted-test-{}", std::process::id())), + ); + crate::ui::windows::WindowRegistry::init(cx); + + let deleted = WindowView::default(); + let gone = deleted.id; + let untouched = WindowView::default(); + let survivor = untouched.id; + WorkspaceStore::install_for_test( + cx, + WindowViews { + views: vec![deleted, untouched], + active: Some(gone), + }, + ); + cx.default_global::() + .windows + .entry(gone) + .or_default() + .sync = SyncPhase::Primed(WsMirror::default()); + + on_workspace_deleted(cx, gone); + + assert!( + WorkspaceStore::all(cx).get(gone).is_none(), + "a row that opens onto nothing is worse than no row at all" + ); + assert_eq!( + WorkspaceStore::all(cx).active, + None, + "the active workspace cannot be one that no longer exists" + ); + assert!( + WorkspaceStore::all(cx).get(survivor).is_some(), + "a deletion is about one workspace, not about the store" + ); + assert!( + !cx.default_global::().windows.contains_key(&gone), + "its sync state has nothing left to be about" + ); + }); + } + /// The rule that stops a failed rebuild from being read as "empty". /// /// A window with no tabs may delete its workspace outright — tree and store