diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index 86d517dc..d7cf82ce 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -136,11 +136,24 @@ impl PaneWorkspace { RouteHeader::local_stdio(program.clone(), &argv) } (_, Some(spec)) => RouteHeader::ssh((**spec).clone()), - (target, None) => { - return Err(anyhow::anyhow!( - "this workspace has no SSH connection details ({target:?}), so its panes \ - cannot be routed" - )); + (_, None) => { + // Deliberately not the target: a `Profile` spells itself as + // its config UUID in `Display` and in `Debug` alike, and a + // deleted profile is exactly what empties `spec` here. This + // sentence is not only logged — `land_pane` hands it to the + // pending pane, which prints the reason verbatim under + // "could not reach {machine}", so the UUID reached the screen + // (#485). The workspace's own name is what every other + // surface calls this thing. + return Err(match self.label.as_deref() { + Some(label) => anyhow::anyhow!( + "{label} has no SSH connection details, so its panes cannot be routed" + ), + None => anyhow::anyhow!( + "this workspace has no SSH connection details, so its panes \ + cannot be routed" + ), + }); } }; Ok(header.for_pane()) @@ -3073,6 +3086,62 @@ mod windows_tests { } } +/// Ungated on purpose: what a workspace can build a route out of is the same +/// on every platform, and so is the name the refusal carries. +#[cfg(test)] +mod route_header_tests { + use super::*; + use crate::core::session::{RemoteTarget, WorkspaceId}; + + fn unroutable(target: RemoteTarget, label: Option<&str>) -> PaneWorkspace { + PaneWorkspace { + workspace: WorkspaceId::new(), + target, + spec: None, + label: label.map(str::to_string), + resize_echo: false, + } + } + + /// A deleted profile is what empties `spec`, and the refusal built here is + /// what the pending pane prints verbatim under "could not reach + /// {machine}" — so this is one of the screens #485 is about. It used to + /// carry `{target:?}`, which for a `Profile` is its config UUID and + /// nothing else. + #[test] + fn an_unroutable_workspace_is_not_named_by_its_profile_uuid() { + let id = uuid::Uuid::new_v4(); + let gone = RemoteTarget::Profile { id }; + + let named = unroutable(gone.clone(), Some("lager")); + let e = named + .route_header() + .expect_err("no spec, no route") + .to_string(); + assert!( + !e.contains(&id.to_string()), + "a bare profile UUID reached the UI: {e}" + ); + assert!( + e.contains("lager"), + "the entry's own name is what it is called: {e}" + ); + assert!(e.contains("cannot be routed"), "{e}"); + + // Nothing to call it by is still no reason to print the UUID. + let bare = unroutable(gone, None); + let e = bare + .route_header() + .expect_err("no spec, no route") + .to_string(); + assert!( + !e.contains(&id.to_string()), + "a bare profile UUID reached the UI: {e}" + ); + assert!(e.contains("cannot be routed"), "{e}"); + } +} + #[cfg(all(test, unix))] mod tests { use super::*; diff --git a/src/ui/remote_connect.rs b/src/ui/remote_connect.rs index 42d101b4..01e3d189 100644 --- a/src/ui/remote_connect.rs +++ b/src/ui/remote_connect.rs @@ -69,11 +69,18 @@ pub fn available_hosts(cx: &App) -> Vec { /// exists, the target's own spelling when that is human-readable, and the /// deleted-profile placeholder for the bare-UUID case (#485). pub fn target_label(cx: &App, target: &RemoteTarget) -> String { - if let Some(choice) = available_hosts(cx) - .into_iter() - .find(|h| h.target == *target) - { - return choice.label; + label_from_hosts(&available_hosts(cx), target) +} + +/// `target_label`'s rule applied to a listing the caller already has. The +/// switcher builds `available_hosts` once per frame and names several targets +/// from it; re-listing per target would re-read `~/.ssh/config` off the disk +/// on the render path, which is the cost `route_label` goes out of its way to +/// avoid. One rule, two entry points — so a name shown next to a pane and the +/// same name shown in the switcher cannot drift apart (#485). +pub fn label_from_hosts(hosts: &[HostChoice], target: &RemoteTarget) -> String { + if let Some(choice) = hosts.iter().find(|h| h.target == *target) { + return choice.label.clone(); } match target { RemoteTarget::Profile { .. } => t(L10nKey::RemoteProfileGone).to_string(), @@ -835,6 +842,36 @@ pub fn restart_server_blocking(header: RouteHeader, label: &str) -> Result<(), S mod tests { use super::*; + /// The one rule for "what do we call a target we cannot resolve" (#485), + /// pinned where both `target_label` and the switcher's group listing read + /// it from. + #[test] + fn an_unresolvable_profile_is_named_never_spelled_as_its_uuid() { + let id = uuid::Uuid::new_v4(); + let gone = RemoteTarget::Profile { id }; + + let label = label_from_hosts(&[], &gone); + assert!( + !label.contains(&id.to_string()), + "a bare profile UUID reached the UI: {label}" + ); + assert_eq!(label, t(L10nKey::RemoteProfileGone)); + + // While the profile is configured, its own name wins. + let listed = vec![HostChoice { + target: gone.clone(), + label: "lager".into(), + detail: "qhw@222.29.101.16".into(), + }]; + assert_eq!(label_from_hosts(&listed, &gone), "lager"); + + // Targets that spell themselves readably never need the placeholder. + let alias = RemoteTarget::Alias { + alias: "build-box".into(), + }; + assert_eq!(label_from_hosts(&[], &alias), "build-box"); + } + fn request() -> InstallRequest { InstallRequest { host: "me@build-box:22".into(), diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index aca45583..a379e595 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -716,6 +716,11 @@ impl Tty7App { } } + // Listed once for the whole frame: the pending groups below name + // themselves from it, and so does the pass that settles every group's + // link state further down. + let configured = remote_connect::available_hosts(cx); + for target in self.pending_machines() { let key = target.to_string(); if index.contains_key(&key) { @@ -723,7 +728,12 @@ impl Tty7App { } index.insert(key.clone(), groups.len()); groups.push(Group { - label: key.clone(), + // Not `key`: a `Profile` target spells itself as its config + // UUID, so a machine whose profile has been deleted would + // announce itself to the banners below by a raw UUID (#485). + // The pass below overwrites this while the profile is still + // configured; this is what is left when it is not. + label: remote_connect::label_from_hosts(&configured, &target), key, endpoint: String::new(), target: Some(target), @@ -838,7 +848,6 @@ impl Tty7App { // trouble banners under the list come out in a stable order. groups.sort_by(|a, b| a.key.is_empty().cmp(&b.key.is_empty()).reverse()); - let configured = remote_connect::available_hosts(cx); for group in &mut groups { let Some(target) = group.target.clone() else { group.link = Link::Local; @@ -3290,6 +3299,53 @@ fn glyph_col(w: f32, child: impl IntoElement) -> impl IntoElement { mod tests { use super::*; + /// #485 on the path #645 did not cover. A machine the switcher knows only + /// from a listing snapshot has no store entry to name it, so its group + /// used to be labelled by the target's own spelling — and a `Profile` + /// target spells itself as its config UUID. Delete the profile and every + /// banner under the list announced a raw UUID. + #[gpui::test] + fn a_pending_machine_whose_profile_is_gone_is_not_named_by_its_uuid( + cx: &mut gpui::TestAppContext, + ) { + use crate::core::session::RemoteTarget; + + let (app, _vcx) = crate::ui::app::test_window::harness(cx); + + // A profile id that is in no config: the state left behind when the + // profile a machine was reached through is deleted. + let id = uuid::Uuid::new_v4(); + let target = RemoteTarget::Profile { id }; + + app.update(cx, |app, _| { + app.host_snapshots.insert( + target.host_id(), + super::HostSnapshot { + target: target.clone(), + rows: Vec::new(), + }, + ); + }); + + app.update(cx, |app, cx| { + let groups = app.switcher_groups(cx); + let group = groups + .iter() + .find(|g| g.target.as_ref() == Some(&target)) + .expect("the snapshot puts its machine in the list"); + assert!( + !group.label.contains(&id.to_string()), + "the switcher named a machine by its raw profile UUID: {}", + group.label + ); + assert_eq!( + group.label, + t(L10nKey::RemoteProfileGone), + "a gone profile is named here the way a pane's route names it" + ); + }); + } + /// A wrong hostname or a stale password used to be fixable only by /// finding the same machine again in Settings (#438). The machine is on /// screen here, so its host row is too — worded for what the row can