From ac8968643d8806601a7712f21e033c7dbce0b23c Mon Sep 17 00:00:00 2001 From: l0ng-ai Date: Thu, 30 Jul 2026 21:28:44 +0800 Subject: [PATCH] fix: reattach the last workspace closed, and resupervise a reopened remote one (#267) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(session): come back to the last workspace closed, not the home page Launch only ever restored a workspace that still had a window at quit, so closing them one by one and relaunching came up on the empty home page — with no hint that four workspaces were sitting there detached. Closing a window here is a detach: its panes keep running in the daemon, which makes that workspace every bit as much "where you left off" as one that still had a window on screen. `workspace_to_restore` now falls back to the most recently active workspace of any kind, and since `close_window` touches it on the way out, that is the one closed last. An open workspace still outranks a more recently touched detached one, so a background agent cannot steal the restore from the window that was actually on screen. Deleting a workspace still drops it from the file — that is the one gesture meaning "done with this". `None` therefore means a genuine first run only, which retires the `FreshStart::HomePage` launch path along with the enum that threaded it through `windows::open` and `Tty7App::for_workspace`. * fix(remote): reopening a remote workspace starts the supervisor again The supervisor stops — and clears every MachineLink with it — as soon as no open workspace is on a remote machine, which closing the last remote window does. The connection itself stays: a closed window is a detach, so HostLinks keeps the socket for whatever opens next. Reopening that workspace then found a live connection with no link behind it. `reopen_remote_at_startup` read the surviving HostLinks entry as "another window got there first" and returned before starting the pump, so nothing ever put a MachineLink back and `status_of` answered Disconnected for good: a "Not connected to " strip and a dead keyboard over panes that were visibly still running on the far side. An existing HostLinks entry is not a reason to skip the supervisor. It answers "is there a socket"; what the window renders from is `machines`. Both now go through RemoteLinks::supervise, which is a no-op for a local workspace and an unconditional ensure_running for a remote one — idempotent, so a machine already supervised costs a flag check, and the first tick over a live socket marks it Attached without opening a second SSH session. Fixes the same hole on the switcher's path, which never told the supervisor anything at all: picking a remote workspace from it goes through `switch_workspace`, not the launch path. --------- Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> --- CHANGELOG.md | 10 +++ crates/tty7-core/src/core/session.rs | 95 ++++++++++++++++---- src/core/session.rs | 11 ++- src/main.rs | 29 ++---- src/ui/app.rs | 26 +++--- src/ui/remote_workspace.rs | 130 ++++++++++++++++++++++----- src/ui/windows.rs | 25 +----- 7 files changed, 229 insertions(+), 97 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07fb7387..c45e91b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -231,6 +231,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Closing every window before quitting no longer loses your place** — launch + only ever restored a workspace that still had a window at quit, so closing them + one by one and relaunching came up on the empty home page, with no hint that + four workspaces were sitting there. Closing a window here is a *detach*: its + panes keep running in the daemon, which makes that workspace every bit as much + "where you left off" as one that still had a window on screen. Launch now falls + back to the workspace closed last, and the only launch that comes up on a fresh + workspace is a genuine first run. Deleting a workspace still means deleting it — + that is the one gesture that drops it from the file. + - **An unsubscribed directory watch stops delivering immediately** — dropping a local watch handle now closes its delivery channel rather than only asking the OS backend to stand down. Tearing that backend down is not instantaneous, and diff --git a/crates/tty7-core/src/core/session.rs b/crates/tty7-core/src/core/session.rs index 97fc4ec1..ca9efde6 100644 --- a/crates/tty7-core/src/core/session.rs +++ b/crates/tty7-core/src/core/session.rs @@ -555,20 +555,41 @@ impl WindowViews { /// others are not lost by any measure that matters: their panes never /// stopped running in the daemon, and the switcher lists them a click away. /// - /// [`active`](Self::active) is the answer whenever it is still open, since - /// it is written on every focus change and so names the window that had the - /// user's attention last. `last_active` is the fallback for a store written - /// by a build that did not track focus, or one whose active workspace was - /// closed before quitting. + /// Three answers, in order: + /// + /// 1. [`active`](Self::active) while it is still open — written on every + /// focus change, so it names the window that had the user's attention + /// last. + /// 2. the most recently active *open* workspace, for a store written by a + /// build that did not track focus, or one whose active workspace was + /// closed before quitting. + /// 3. the most recently active workspace of any kind, open or not. + /// + /// That last one is why closing every window and quitting still comes back + /// somewhere. Closing a window here is a *detach*: the panes keep running in + /// the daemon, so the workspace behind them is every bit as much "where the + /// user left off" as one that still had a window — and `close_window` + /// touches it on the way out, which makes the most recent of them the one + /// closed last. Only the explicit *Close Workspace* drops an entry from the + /// file, and that is the one gesture that means "I am done with this". + /// + /// `None` therefore means one thing: no workspaces at all, i.e. a first run. pub fn workspace_to_restore(&self) -> Option { let focused = self .active .filter(|id| self.get(*id).is_some_and(|w| w.open)); - focused.or_else(|| { - self.open_views() - .max_by_key(|w| w.last_active) - .map(|w| w.id) - }) + focused + .or_else(|| { + self.open_views() + .max_by_key(|w| w.last_active) + .map(|w| w.id) + }) + .or_else(|| { + self.views + .iter() + .max_by_key(|w| w.last_active) + .map(|w| w.id) + }) } /// Persist as JSON, creating the parent directory if needed. Any @@ -938,14 +959,56 @@ mod tests { }; assert_eq!(all.workspace_to_restore(), Some(open_id)); - // Nothing open at all: launch has no workspace to come up on and shows - // the home page instead of inventing one. - let mut none_open = view(); - none_open.open = false; + // Nothing open at all — the user closed every window before quitting. + // Launch still comes back to the one closed last, because a detached + // workspace's panes are still running and `close_window` touches it on + // the way out. + let mut first_closed = view(); + first_closed.open = false; + first_closed.last_active = 100; + let mut closed_last = view(); + closed_last.open = false; + closed_last.last_active = 900; + let closed_last_id = closed_last.id; let all = WindowViews { active: None, - views: vec![none_open], + views: vec![first_closed, closed_last], }; - assert_eq!(all.workspace_to_restore(), None); + assert_eq!(all.workspace_to_restore(), Some(closed_last_id)); + + // A stale `active` naming a workspace that is gone from the file does + // not stop the fallback from answering. + let all = WindowViews { + active: Some(WorkspaceId::new()), + ..all + }; + assert_eq!(all.workspace_to_restore(), Some(closed_last_id)); + + // The only `None` left is a genuine first run. + assert_eq!(WindowViews::default().workspace_to_restore(), None); + } + + /// An open workspace outranks a detached one even when the detached one saw + /// activity more recently — the fallback is for when *nothing* is open, not + /// a recency race across the two states. + /// + /// Without this, a background agent touching a detached workspace after the + /// user's last keystroke would have launch reopen that one instead of the + /// window that was actually on screen at quit. + #[test] + fn an_open_workspace_outranks_a_more_recently_touched_detached_one() { + let mut open_one = view(); + open_one.open = true; + open_one.last_active = 100; + let open_id = open_one.id; + let mut detached = view(); + detached.open = false; + detached.last_active = 900; + + let all = WindowViews { + active: None, + views: vec![open_one, detached], + }; + assert_eq!(all.workspace_to_restore(), Some(open_id)); } } diff --git a/src/core/session.rs b/src/core/session.rs index c466a6cf..ea4e34d2 100644 --- a/src/core/session.rs +++ b/src/core/session.rs @@ -159,11 +159,14 @@ impl WorkspaceStore { /// Their panes are untouched — this is exactly the state /// [`close_window`](Self::close_window) leaves behind, reached in bulk. /// - /// Returns `None` when nothing was open, which launch reads as "come up on - /// the home page". + /// The workspace kept need not have been open at all: quitting with every + /// window closed comes back to the one closed last (see + /// [`WindowViews::workspace_to_restore`]). `None` means there are no saved + /// workspaces whatsoever — a first run. pub fn restore_one(cx: &mut gpui::App) -> Option { let store = Self::try_store(cx)?; let keep = store.views.workspace_to_restore()?; + let reattaching = store.views.get(keep).is_some_and(|view| !view.open); let mut detached = 0usize; for view in &mut store.views.views { if view.open && view.id != keep { @@ -173,7 +176,9 @@ impl WorkspaceStore { } store.views.active = Some(keep); store.views.save(); - if detached > 0 { + if reattaching { + log::info!("launch: no window was open at quit; reattaching the last one closed"); + } else if detached > 0 { log::info!("launch: restoring 1 workspace, left {detached} detached"); } Some(keep) diff --git a/src/main.rs b/src/main.rs index 7f5550af..b1669e7e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -426,28 +426,15 @@ fn main() { // quit: see `WindowViews::workspace_to_restore` for why, and // `WorkspaceStore::restore_one` for what happens to the others (they // are detached, not forgotten — panes keep running and the switcher - // lists them). Quitting with every window closed — or a first run — - // opens a single window on a fresh workspace. - let any_saved = { - let store = crate::core::session::WorkspaceStore::all(cx); - !store.views.is_empty() - }; + // lists them). Closing every window before quitting is *not* a + // reason to come up empty: those workspaces still hold running + // panes, so launch reattaches the one closed last. + // + // `None` is therefore a first run only, and it opens a single window + // on a fresh workspace holding one terminal — exactly as every + // pre-multi-window build did. let reopen = crate::core::session::WorkspaceStore::restore_one(cx); - // With nothing to reopen, what that one window should hold depends on - // whether there is anything to come back to: workspaces the user - // detached are listed by the home page's picker, so leave it empty - // for them. A genuine first run has no picker to show and no reason - // to greet the user with a blank page — it opens a terminal, exactly - // as every pre-multi-window build did. - let fresh = if any_saved { - crate::ui::windows::FreshStart::HomePage - } else { - crate::ui::windows::FreshStart::Shell - }; - match reopen { - Some(id) => crate::ui::windows::open(cx, Some(id)), - None => crate::ui::windows::open_with(cx, None, fresh), - } + crate::ui::windows::open(cx, reopen); }); } diff --git a/src/ui/app.rs b/src/ui/app.rs index 4d7b4f08..fc05b627 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -937,7 +937,6 @@ impl Tty7App { /// that is no longer on file. pub fn for_workspace( id: Option, - fresh: crate::ui::windows::FreshStart, window: &mut Window, cx: &mut Context, ) -> Self { @@ -960,19 +959,12 @@ impl Tty7App { // A remote workspace hydrates even with restore off: its panes are // running sessions on another machine, not a saved layout. let hydrate = known && (restore || is_remote); - // What the window opens holding is the caller's call for a *brand-new* - // workspace: `None` takes the first-run path in `with_session`, - // spawning a single default terminal — what `New Workspace` and a - // first run both want — while an empty session lands on the home page, - // for the launch that exists to show the workspace picker. A known - // workspace opens empty (the hydration fills it), or on a fresh shell - // when the user turned restore off. - let session = match (known, fresh) { - (true, _) if hydrate => Some(Session::default()), - (true, _) => None, - (false, crate::ui::windows::FreshStart::Shell) => None, - (false, crate::ui::windows::FreshStart::HomePage) => Some(Session::default()), - }; + // A brand-new workspace takes the first-run path in `with_session` + // (`None`), spawning a single default terminal — what `New Workspace` + // and a first run both want. A known workspace opens on an empty + // session that the hydration fills, or on a fresh shell when the user + // turned restore off. + let session = hydrate.then(Session::default); let app = Self::with_session(Some(workspace), session, window, cx); if hydrate { // No immediate save: the window is deliberately empty, and racing @@ -1701,6 +1693,12 @@ impl Tty7App { let claimed = WorkspaceStore::claim(cx, Some(id)); crate::ui::windows::WindowRegistry::rebind(cx, previous, claimed); + // A pick from the switcher is one of the ways a remote workspace comes + // back, so it owes the supervisor the same call the launch path makes — + // see `RemoteLinks::supervise` for what skipping it leaves on screen. + // The outgoing workspace needs no counterpart: `pump_tick` drops a + // machine the moment its last open workspace goes. + crate::ui::remote_workspace::RemoteLinks::supervise(cx, claimed); // The machine's tree is the layout's only home now, so an explicit // pick from the switcher always hydrates — restore-off governs what // *launch* comes back to, not what a deliberate open shows. The window diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 3847e00f..57ecd1e7 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -757,28 +757,11 @@ impl Tty7App { /// its window already built and its layout the last one this client pulled. /// What M6 adds is the connect itself plus the auth queue that keeps ten /// windows from raising ten password sheets at once. + /// + /// Nothing but a call to [`RemoteLinks::supervise`], and local workspaces + /// pass straight through it — the launch path stays ignorant of hosts. pub(crate) fn reopen_remote_at_startup(&self, cx: &mut Context) { - let Some(host) = WorkspaceStore::remote_ref(cx, self.workspace) else { - return; - }; - remote_connect::register(cx); - if remote_connect::HostLinks::get(cx, host.host_id()).is_some() { - // Another window on the same machine got there first. One connection - // per machine is the point — D7's "connect immediately" is about the - // *machine*, and a second link to it would be a second SSH session - // for no reason. - return; - } - log::info!("reconnecting to {} at startup", host.target); - // No per-window connect call: the supervisor already knows which - // machines have open workspaces, so starting it *is* the reconnect, and - // ten windows on one box produce one attempt rather than ten. - // - // Nothing here classifies the host as needing authentication or not - // (D7): every machine is attempted in parallel, and the ones that turn - // out to need a human queue for the sheet at the moment they ask — see - // [`AuthSheetQueue`]. - RemoteLinks::ensure_running(cx); + RemoteLinks::supervise(cx, self.workspace); } // ----- prompts ----------------------------------------------------------- @@ -1341,6 +1324,45 @@ impl RemoteLinks { .detach(); } + /// Put `workspace`'s machine under the supervisor, if it has one. A local + /// workspace is a no-op, which is what lets every "a window took over a + /// workspace" path call this without first asking whether it is remote. + /// + /// **Every such path must.** The supervisor is not a one-shot at start-up: + /// [`pump_tick`] stops it — and clears every [`MachineLink`] with it — as + /// soon as no *open* workspace is on a remote machine, which closing the + /// last remote window does. What that leaves behind is a live connection + /// with no link behind it, because a closed window is a detach and + /// [`remote_connect::HostLinks`] outlives it by design. Reopening the + /// workspace then reads as [`RemoteStatus::Disconnected`] — a "Not + /// connected" strip and a dead keyboard over panes that are visibly still + /// running (#issue: reopened remote workspace stays "not connected"). + /// + /// So an existing `HostLinks` entry is **not** a reason to skip this: it + /// answers "is there a socket", and the state the window renders from is + /// `machines`. `ensure_running` is idempotent, so the machine that really + /// is already supervised costs a flag check, and the first tick over a live + /// socket marks it `Attached` without opening a second SSH session. + pub(crate) fn supervise(cx: &mut gpui::App, workspace: WorkspaceId) { + let Some(host) = WorkspaceStore::remote_ref(cx, workspace) else { + return; + }; + remote_connect::register(cx); + log::info!( + "supervising {} for a workspace that just opened", + host.target + ); + // No per-window connect call: the supervisor already knows which + // machines have open workspaces, so starting it *is* the reconnect, and + // ten windows on one box produce one attempt rather than ten. + // + // Nothing here classifies the host as needing authentication or not + // (D7): every machine is attempted in parallel, and the ones that turn + // out to need a human queue for the sheet at the moment they ask — see + // [`AuthSheetQueue`]. + RemoteLinks::ensure_running(cx); + } + /// This workspace's state, or `None` when it is a local one. pub(crate) fn status_of(cx: &gpui::App, workspace: WorkspaceId) -> Option { let host = WorkspaceStore::remote_ref(cx, workspace)?; @@ -2205,6 +2227,72 @@ mod tests { }); } + /// **Stopping the supervisor is not a terminal state.** It stops whenever no + /// open workspace is remote — closing the last remote window does it — and a + /// workspace reopened afterwards has to start it again, or it sits under a + /// "Not connected" strip with a dead keyboard for ever while its panes run + /// on the far side. + /// + /// What this pins is that [`RemoteLinks::supervise`] is that restart, from a + /// pump that has genuinely stopped. It cannot reproduce the original bug in + /// full — that needed a live `HostLinks` entry, which takes a real control + /// connection to build — so the other half of the rule lives in + /// `supervise`'s own doc: never gate the `ensure_running` call on one. + #[gpui::test] + fn a_stopped_supervisor_restarts_when_a_remote_workspace_comes_back( + cx: &mut gpui::TestAppContext, + ) { + let id = cx.update(|cx| { + cx.set_global(crate::core::config::Config::default()); + // Nothing remote on file yet, so the first tick has no machine to + // supervise and shuts the pump down — the state a closed remote + // window leaves behind. + crate::core::session::WorkspaceStore::install_for_test( + cx, + crate::core::session::WindowViews::default(), + ); + RemoteLinks::ensure_running(cx); + assert!(cx.default_global::().pumping); + WorkspaceId::new() + }); + // The tick runs and returns `false` without ever reaching its timer, so + // this needs no clock of its own. + cx.background_executor.run_until_parked(); + cx.update(|cx| { + assert!( + !cx.default_global::().pumping, + "with no remote workspace open the pump is expected to stop" + ); + + // The workspace comes back — reopened from the switcher, or the + // launch path's window landing on it. + let host = RemoteRef::new( + RemoteTarget::Alias { + alias: "build-box".into(), + }, + WorkspaceId::new(), + ); + crate::core::session::WorkspaceStore::install_for_test( + cx, + crate::core::session::WindowViews { + views: vec![crate::core::session::WindowView { + id, + host: Some(host), + open: true, + ..Default::default() + }], + active: Some(id), + }, + ); + RemoteLinks::supervise(cx, id); + + assert!( + cx.default_global::().pumping, + "reopening a remote workspace has to start the supervisor again" + ); + }); + } + /// A plain reconnect (never preempted) must not be marked for a rebuild — /// its panes are alive and re-attachable, and a Replace would tear down /// views the relink was about to reuse. diff --git a/src/ui/windows.rs b/src/ui/windows.rs index 8946fee8..38dfdae3 100644 --- a/src/ui/windows.rs +++ b/src/ui/windows.rs @@ -178,34 +178,15 @@ impl WindowRegistry { } } -/// What a *brand-new* workspace's window starts with. Only consulted when the -/// window is opening on a freshly minted workspace — a known one opens empty -/// and is filled from its machine's tree. -#[derive(Clone, Copy, PartialEq, Eq)] -pub enum FreshStart { - /// A single default terminal, the way every previous launch of tty7 came - /// up. What `New Workspace` and a genuine first run want: a window whose - /// workspace has nothing in it yet is a window you asked for to work in. - Shell, - /// No tabs — the home page. Used at launch when there *are* saved - /// workspaces but none were open at quit: the picker listing them is the - /// whole point of that window, and a shell in front of it would bury it. - HomePage, -} - /// Open a window on `workspace` — or on a brand-new workspace when `None`, -/// which starts with a single terminal (see [`open_with`] for the other case). +/// which starts with a single terminal. A known workspace opens empty and is +/// filled from its machine's tree. /// /// When that workspace already has a window, this focuses it instead of /// opening a second one: two windows on one workspace would both attach the /// same daemon panes, and the daemon's single-subscriber model means the /// second attach silently kills the first window's terminal. pub fn open(cx: &mut App, workspace: Option) { - open_with(cx, workspace, FreshStart::Shell); -} - -/// [`open`], with a say in what a brand-new workspace comes up holding. -pub fn open_with(cx: &mut App, workspace: Option, fresh: FreshStart) { if let Some(id) = workspace && let Some(handle) = WindowRegistry::window_for(cx, id) { @@ -218,7 +199,7 @@ pub fn open_with(cx: &mut App, workspace: Option, fresh: FreshStart // only the root view — so capture it on the way past. let mut created: Option> = None; let opened = cx.open_window(options, |window, cx| { - let app = cx.new(|cx| Tty7App::for_workspace(workspace, fresh, window, cx)); + let app = cx.new(|cx| Tty7App::for_workspace(workspace, window, cx)); created = Some(app.clone()); // Root's own background is fully transparent: `Tty7App`'s root div is // the single owner of the window background (solid / gradient / image,