From 6d54f856c4dd21611fa3436a1d76efa170e80443 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:59:05 +0800 Subject: [PATCH] fix(switcher): make the error band's Try Again and Dismiss do something The band falls back to the reconnect supervisor's last error, which neither button cleared: Dismiss left it on the next frame, and Try Again started a second connect of its own while the stale reason kept hiding the Connecting state. - Try Again on a supervisor failure asks the supervisor to dial now (backoff reset) instead of racing it with another connect. - While a connect is in flight the band says Connecting, not the old reason. - Dismiss silences that exact failure on the supervisor; a different one is shown again, and the link coming up forgets it. - A dismissed parked-route notice stays quiet instead of resurfacing as the error band, and the band offers no Try Again on a parked route. - A failed Close on an orphan pane shows why on its own line. --- src/ui/remote_workspace.rs | 104 ++++++++++++++++++++++++++++- src/ui/switcher.rs | 133 ++++++++++++++++++++++++++----------- 2 files changed, 197 insertions(+), 40 deletions(-) diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 9ce3c18b..f18450f6 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -1280,6 +1280,10 @@ struct MachineLink { /// `Failed` on the very next tick, so this is the only place the reason /// survives long enough for anyone to read it. last_error: Option, + /// The failure the user dismissed in the switcher, word for word. The + /// band stays quiet while the machine keeps failing the same way and + /// speaks again on a different reason; the link coming up forgets it. + dismissed_error: Option, /// The workspaces this client has sent a `WorkspaceAttach` for over the /// link that is up right now, whether or not the far end took it. Scoped /// to one link on purpose: a new link has heard nothing from us. @@ -1455,6 +1459,33 @@ impl RemoteLinks { }) } + /// What the switcher's error band should say for this machine, if + /// anything: the supervisor's last failure, unless an attempt is on the + /// wire right now (the band says Connecting then, or a Try Again would + /// look like it did nothing) or the user already dismissed this very + /// failure. + pub(crate) fn machine_error(cx: &gpui::App, host: HostId) -> Option { + let link = cx.try_global::()?.machines.get(&host)?; + if link.attempting { + return None; + } + let error = match &link.state { + LinkState::Reconnecting => link.last_error.clone()?, + LinkState::Failed(e) | LinkState::Mismatched(e) => e.clone(), + LinkState::Connecting | LinkState::Attached => return None, + }; + (link.dismissed_error.as_ref() != Some(&error)).then_some(error) + } + + /// The switcher's Dismiss on a failure the supervisor reported. Only that + /// failure is silenced — the next different one is shown again. + pub(crate) fn dismiss_machine_error(cx: &mut gpui::App, host: HostId, error: &str) { + if let Some(link) = cx.default_global::().machines.get_mut(&host) { + link.dismissed_error = Some(error.to_string()); + } + cx.refresh_windows(); + } + /// The open workspaces on this machine that another client is holding. pub(crate) fn preempted_on(cx: &gpui::App, host: HostId) -> Vec { if host.is_local() { @@ -1478,13 +1509,24 @@ impl RemoteLinks { if let Some(by) = links.preempted.remove(&workspace) { links.reclaiming.insert(workspace, by); } - links.suspended.remove(&host.host_id()); - let link = links.machines.entry(host.host_id()).or_insert(MachineLink { + RemoteLinks::retry_machine_now(cx, host.host_id()); + } + + /// Ask the supervisor to dial this machine on the next tick, backoff + /// reset. The switcher's Try Again lands here when the failure on screen + /// is the supervisor's: a second connect of its own would race the + /// reconnect already scheduled, and the band would go on showing the + /// supervisor's old reason the whole time. + pub(crate) fn retry_machine_now(cx: &mut gpui::App, host: HostId) { + let links = cx.default_global::(); + links.suspended.remove(&host); + let link = links.machines.entry(host).or_insert(MachineLink { state: LinkState::Reconnecting, backoff: Backoff::default(), next_attempt: None, attempting: false, last_error: None, + dismissed_error: None, attach_sent: Default::default(), }); link.backoff.reset(); @@ -1531,6 +1573,7 @@ impl RemoteLinks { next_attempt: None, attempting: false, last_error: None, + dismissed_error: None, attach_sent: Default::default(), }); f(link); @@ -1579,6 +1622,7 @@ fn pump_tick(cx: &mut gpui::App) -> bool { link.backoff.reset(); link.next_attempt = None; link.last_error = None; + link.dismissed_error = None; }); // A live link is not an attachment. Preemption of a GUI client // leaves the control connection alone (only a `dedicated` client is @@ -3935,4 +3979,60 @@ mod tests { ); }); } + + /// The switcher's error band reads the supervisor through `machine_error`. + /// Dismiss has to silence exactly the failure on screen — the band used + /// to come straight back on the next frame — and Try Again has to turn + /// the band into Connecting while the dial is out. + #[gpui::test] + fn the_switcher_band_hears_dismiss_and_retry(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + let (host, _) = resolvable_machine("dismissed-box"); + RemoteLinks::mark(cx, host, |link| { + link.state = LinkState::Reconnecting; + link.last_error = Some("connection timed out".into()); + }); + assert_eq!( + RemoteLinks::machine_error(cx, host).as_deref(), + Some("connection timed out") + ); + + RemoteLinks::dismiss_machine_error(cx, host, "connection timed out"); + assert_eq!( + RemoteLinks::machine_error(cx, host), + None, + "the same failure again stays dismissed" + ); + + RemoteLinks::mark(cx, host, |link| { + link.last_error = Some("host key changed".into()); + }); + assert_eq!( + RemoteLinks::machine_error(cx, host).as_deref(), + Some("host key changed"), + "a different failure is news" + ); + + RemoteLinks::mark(cx, host, |link| link.attempting = true); + assert_eq!( + RemoteLinks::machine_error(cx, host), + None, + "an attempt on the wire is Connecting, not the old reason" + ); + + RemoteLinks::mark(cx, host, |link| { + link.attempting = false; + link.state = LinkState::Reconnecting; + link.backoff = Backoff::default(); + }); + RemoteLinks::retry_machine_now(cx, host); + let links = cx.default_global::(); + let link = links.machines.get(&host).expect("still supervised"); + assert!( + link.next_attempt.is_some_and(|at| at <= Instant::now()), + "Try Again dials on the next tick instead of waiting out the backoff" + ); + assert!(!links.suspended.contains(&host)); + }); + } } diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index ae1ad382..58e4cbf2 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -334,6 +334,10 @@ pub(crate) struct Switcher { /// asynchronously after the panel opens; empty both while the listing is /// in flight and when there is nothing to reap. orphans: Vec, + /// Why the last Close on an orphan did not take, by pane id. Said on the + /// orphan's own line: a Close that fails in silence reads as a dead + /// button. + orphan_failures: HashMap, column: Column, left_sel: usize, right_sel: usize, @@ -506,6 +510,7 @@ impl Tty7App { page: Page::List, renaming: None, orphans: Vec::new(), + orphan_failures: HashMap::new(), column, left_sel: 0, right_sel: 0, @@ -580,12 +585,19 @@ impl Tty7App { Ok(listed) => listed, Err(e) => { log::warn!(target: "tty7::switcher", "closing orphan %{pane_id} failed: {e}"); + let _ = this.update(cx, |this, cx| { + if let Some(sw) = this.switcher.as_mut() { + sw.orphan_failures.insert(pane_id, e.to_string()); + } + cx.notify(); + }); return; } }; let _ = this.update(cx, |this, cx| { let held = held_local_pane_ids(cx); if let Some(sw) = this.switcher.as_mut() { + sw.orphan_failures.remove(&pane_id); sw.orphans = orphan_panes_of(listed, &held); } cx.notify(); @@ -953,11 +965,14 @@ impl Tty7App { // the reconnect that has been failing for an hour, have nowhere at // all to be said. if group.error.is_none() { - group.error = match supervised { - Some(MachineStatus::Failed(e)) => Some(e), - Some(MachineStatus::Reconnecting { last_error, .. }) => last_error, - _ => None, - }; + group.error = RemoteLinks::machine_error(cx, id); + } + // A connect of this window's own is on the wire: whatever failed + // before it is the question it is answering, and the band has to + // say Connecting — an old reason left up reads as a button that + // did nothing. + if matches!(group.link, Link::Connecting) { + group.error = None; } let taken: HashSet = RemoteLinks::preempted_on(cx, id).into_iter().collect(); @@ -2004,11 +2019,13 @@ impl Tty7App { self.render_install_progress(&group.label, phase, cx) .into_any_element(), ); - } else if group.parked - && !self.parked_dismissed.contains(&group.key) - && !group.rows.is_empty() - { - out.push(self.render_parked_notice(group, cx).into_any_element()); + } else if group.parked && !group.rows.is_empty() { + // Dismissed means quiet. Falling through would put the + // failure that came with the parking straight back up as the + // error band, under a route nothing can retry. + if !self.parked_dismissed.contains(&group.key) { + out.push(self.render_parked_notice(group, cx).into_any_element()); + } } else if let Some(error) = group.error.clone() { out.push(self.render_error_band(group, &error, cx)); } else if matches!(group.link, Link::Connecting | Link::Reconnecting { .. }) { @@ -2052,6 +2069,7 @@ impl Tty7App { let replace_key = group.key.clone(); let dismiss_key = group.key.clone(); let dismiss_target = group.target.clone(); + let dismiss_error = error.to_string(); // The band no longer sits under a machine header, so plain errors // carry the machine's name themselves; the dialect restatement // already names it. @@ -2074,30 +2092,46 @@ impl Tty7App { .child( h_flex() .gap(px(4.)) - .child( - Button::new(gpui::SharedString::from(format!( - "switcher-retry:{}", - group.key - ))) - .label(t(L10nKey::TryAgain)) - .ghost() - .xsmall() - .on_click(cx.listener( - move |this, _, _window, cx| { - this.remote_host_errors.remove(&retry_key); - if let Some(target) = retry.target.clone() { - this.connect_to_host( - HostChoice { - target, - label: retry.label.clone(), - detail: String::new(), - }, - cx, - ); - } - }, - )), - ) + // A route that no longer resolves cannot be retried into + // working (#485); a button that can only fail again is + // worse than none. + .when(!group.parked, |row| { + row.child( + Button::new(gpui::SharedString::from(format!( + "switcher-retry:{}", + group.key + ))) + .label(t(L10nKey::TryAgain)) + .ghost() + .xsmall() + .on_click(cx.listener( + move |this, _, _window, cx| { + let ours = this.remote_host_errors.remove(&retry_key).is_some() + || matches!( + &this.connect, + Some(ConnectFlow::Failed { choice, .. }) + if Some(&choice.target) == retry.target.as_ref() + ); + if let Some(target) = retry.target.clone() { + let host = target.host_id(); + if !ours && RemoteLinks::machine_error(cx, host).is_some() { + RemoteLinks::retry_machine_now(cx, host); + cx.notify(); + return; + } + this.connect_to_host( + HostChoice { + target, + label: retry.label.clone(), + detail: String::new(), + }, + cx, + ); + } + }, + )), + ) + }) .when( crate::daemon::control::is_dialect_refusal(error) // Same gate as the workspace strip's: a machine @@ -2145,6 +2179,22 @@ impl Tty7App { .on_click(cx.listener( move |this, _, _window, cx| { this.remote_host_errors.remove(&dismiss_key); + // The supervisor keeps its own copy of the + // reason, and it is the one that outlives every + // other source: without this the band is back + // on the very next frame. + if let Some(target) = &dismiss_target { + let host = target.host_id(); + if RemoteLinks::machine_error(cx, host).as_ref() + == Some(&dismiss_error) + { + RemoteLinks::dismiss_machine_error( + cx, + host, + &dismiss_error, + ); + } + } // The other half of this block can come from a // failed connect. Retire that too, but only when // it is this host's failure — a connect to @@ -2183,7 +2233,7 @@ impl Tty7App { }; // Resolved up front, while `cx` can still be borrowed as an `App`: the // render loop below needs it mutably for the Close listener. - let lines: Vec<(u64, String)> = switcher + let lines: Vec<(u64, String, Option)> = switcher .orphans .iter() .map(|orphan| { @@ -2196,11 +2246,15 @@ impl Tty7App { } else if !orphan.title.is_empty() { bits.push(orphan.title.clone()); } - (orphan.pane_id, bits.join(" · ")) + ( + orphan.pane_id, + bits.join(" · "), + switcher.orphan_failures.get(&orphan.pane_id).cloned(), + ) }) .collect(); let mut list = v_flex().gap(px(2.)); - for (pane_id, line) in lines { + for (pane_id, line, failure) in lines { list = list.child( h_flex() .items_center() @@ -2218,7 +2272,10 @@ impl Tty7App { .truncate() .text_size(gpui::rems(11. / 16.)) .text_color(theme.foreground) - .child(line), + .child(line) + .when_some(failure, |el, failure| { + el.child(div().truncate().text_color(theme.danger).child(failure)) + }), ) .child( div().flex_shrink_0().child(