diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index 82b4eaa1..af1d8f4c 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -1045,9 +1045,23 @@ impl<'a> Installer<'a> { } } -const RUNNING_EXE_COMMAND: &str = r#"for p in /proc/[0-9]*; do e=$(readlink "$p/exe" 2>/dev/null) || continue; case "$e" in */tty7-server-*) printf '%s' "${e% (deleted)}"; break;; esac; done; true"#; +/// Finding the running server takes two shapes because `/proc` is a Linux +/// thing. On Linux, `/proc//exe` is the honest answer: a symlink to the +/// file that is actually executing, whatever anyone did to `argv[0]`. macOS and +/// the BSDs have no `/proc` at all, so fall back to `ps`, whose `comm` is the +/// full path there — on Linux it would be the name truncated to 15 characters, +/// one short of `tty7-server-c7p5`, which is exactly why the fallback is a +/// fallback and not the only branch. +/// +/// The `/proc` glob lives *inside* the `[ -d /proc ]` arm on purpose. The far +/// end runs these through the user's login shell, and zsh — the default on +/// macOS — aborts the whole command line when a glob matches nothing, so with +/// the loop at top level the trailing `true` never ran and every one of these +/// was a silent no-op on every Mac. That is what left `restart_daemon` waiting +/// out its ten seconds for a daemon nobody had asked to stop. +const RUNNING_EXE_COMMAND: &str = r#"if [ -d /proc ]; then for p in /proc/[0-9]*; do e=$(readlink "$p/exe" 2>/dev/null) || continue; case "$e" in */tty7-server-*) printf '%s' "${e% (deleted)}"; break;; esac; done; else ps -xwwo pid=,comm= 2>/dev/null | while read -r pid e; do case "$e" in */tty7-server-*) printf '%s' "$e"; break;; esac; done; fi; true"#; -const TERMINATE_RUNNING_COMMAND: &str = r#"for p in /proc/[0-9]*; do e=$(readlink "$p/exe" 2>/dev/null) || continue; case "$e" in */tty7-server-*) kill -TERM "${p#/proc/}" 2>/dev/null; break;; esac; done; true"#; +const TERMINATE_RUNNING_COMMAND: &str = r#"if [ -d /proc ]; then for p in /proc/[0-9]*; do e=$(readlink "$p/exe" 2>/dev/null) || continue; case "$e" in */tty7-server-*) kill -TERM "${p#/proc/}" 2>/dev/null; break;; esac; done; else ps -xwwo pid=,comm= 2>/dev/null | while read -r pid e; do case "$e" in */tty7-server-*) kill -TERM "$pid" 2>/dev/null; break;; esac; done; fi; true"#; fn launch_command(binary: &str) -> String { let bin = shell_quote(binary); diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index 1f189a36..5c2c6610 100644 --- a/crates/tty7-core/src/daemon/install/tests.rs +++ b/crates/tty7-core/src/daemon/install/tests.rs @@ -1029,6 +1029,55 @@ fn the_running_exe_probe_cannot_fail_the_command() { assert!(TERMINATE_RUNNING_COMMAND.contains("*/tty7-server-*")); } +/// Both commands have to work on a machine with no `/proc`, which is every Mac +/// and every BSD, and the `/proc` glob has to stay inside the guard: zsh is the +/// login shell over there, and a top-level glob that matches nothing takes the +/// rest of the command line with it — including the trailing `true`. +#[test] +fn finding_the_running_server_survives_a_machine_without_proc() { + for cmd in [RUNNING_EXE_COMMAND, TERMINATE_RUNNING_COMMAND] { + assert!( + cmd.starts_with("if [ -d /proc ]; then"), + "the glob has to be unreachable before the guard passes: {cmd}" + ); + let (guarded, fallback) = cmd + .split_once("; else ") + .expect("a machine with no /proc still needs an answer"); + assert!( + guarded.contains("/proc/[0-9]*") && !fallback.contains("/proc/"), + "the fallback reads ps, not a filesystem that is not there: {cmd}" + ); + assert!( + fallback.contains("ps -xwwo pid=,comm="), + "unwrapped, unabbreviated, and this user's processes only: {cmd}" + ); + } +} + +/// Whichever branch the far end takes, the command has to parse — a syntax +/// error here is invisible in production, where the output is read as "no +/// server is running" and the failure is a ten-second timeout. +/// +/// Parsed, not run: the terminate command would kill this developer's own +/// server, and this test is not the place to find that out. +#[cfg(unix)] +#[test] +fn both_branches_of_the_probe_are_valid_shell() { + for cmd in [RUNNING_EXE_COMMAND, TERMINATE_RUNNING_COMMAND] { + let out = std::process::Command::new("/bin/sh") + .arg("-n") + .arg("-c") + .arg(cmd) + .output() + .expect("every unix has /bin/sh"); + assert!( + out.status.success(), + "{cmd}\nstderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + } +} + #[test] fn exec_failures_quote_stderr_when_there_is_any() { let with_stderr = ExecOutput { diff --git a/src/ui/app.rs b/src/ui/app.rs index 1bb1ed04..8833bf2e 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -6144,12 +6144,25 @@ impl Tty7App { let status = self.remote_status(cx)?; let machine = self.remote_machine_label(cx); let message = status.strip_message(&machine)?; - let action = self.remote_strip_action(&status, cx); + // While an install runs, the strip is that install's progress: the + // refusal it is answering is no longer the news, and the button that + // started it would only start a second one. + let installing = self.remote_strip_progress(cx); + let action = installing + .is_none() + .then(|| self.remote_strip_action(&status, cx)) + .flatten(); let theme = cx.theme(); - let bar = gpui_component::h_flex() + let message = match installing { + Some(phase) => format!( + "{machine} — {}", + crate::ui::remote_workspace::install_phase_caption(phase) + ), + None => message, + }; + let bar = gpui_component::v_flex() .occlude() - .items_center() - .gap_2() + .gap(px(6.)) .px_3() .py_1p5() .rounded_lg() @@ -6159,25 +6172,33 @@ impl Tty7App { .shadow_md() .text_xs() .text_color(theme.muted_foreground) - .child(gpui_component::Icon::new(gpui_component::IconName::Globe)) .child( - div() - .font_weight(gpui::FontWeight::MEDIUM) - .text_color(theme.foreground) - .child(message), + gpui_component::h_flex() + .items_center() + .gap_2() + .child(gpui_component::Icon::new(gpui_component::IconName::Globe)) + .child( + div() + .font_weight(gpui::FontWeight::MEDIUM) + .text_color(theme.foreground) + .child(message), + ) + .when_some(action, |this, (label, action)| { + use gpui_component::Sizable as _; + use gpui_component::button::ButtonVariants as _; + this.child( + gpui_component::button::Button::new("remote-status-action") + .label(label) + .primary() + .small() + .on_click(cx.listener(move |this, _, window, cx| { + this.run_strip_action(action.clone(), window, cx); + })), + ) + }), ) - .when_some(action, |this, (label, action)| { - use gpui_component::Sizable as _; - use gpui_component::button::ButtonVariants as _; - this.child( - gpui_component::button::Button::new("remote-status-action") - .label(label) - .primary() - .small() - .on_click(cx.listener(move |this, _, window, cx| { - this.run_strip_action(action.clone(), window, cx); - })), - ) + .when_some(installing, |this, phase| { + this.child(crate::ui::remote_workspace::install_progress_bar(phase, cx)) }); Some( div() diff --git a/src/ui/home.rs b/src/ui/home.rs index 81622386..f7df385b 100644 --- a/src/ui/home.rs +++ b/src/ui/home.rs @@ -259,12 +259,25 @@ impl Tty7App { let machine = self.remote_machine_label(cx); let status = self.remote_status(cx)?; let message = status.strip_message(&machine)?; - let action = self.remote_strip_action(&status, cx); + // An install in flight replaces both halves of the strip: its own line + // instead of the complaint that is being answered, and no button, since + // pressing Update Server again would start a second one on top of it. + let installing = self.remote_strip_progress(cx); + let action = installing + .is_none() + .then(|| self.remote_strip_action(&status, cx)) + .flatten(); let theme = cx.theme(); + let message = match installing { + Some(phase) => format!( + "{machine} — {}", + crate::ui::remote_workspace::install_phase_caption(phase) + ), + None => message, + }; Some( - h_flex() - .items_center() - .gap_2() + v_flex() + .gap(px(6.)) .px(px(12.)) .py(px(6.)) .rounded(px(10.)) @@ -273,19 +286,29 @@ impl Tty7App { .border_color(theme.border) .text_xs() .text_color(theme.muted_foreground) - .child(gpui_component::Icon::new(IconName::Globe)) - .child(message) - .when_some(action, |this, (label, action)| { - this.child( - Button::new("home-remote-status-action") - .label(label) - .ghost() - .small() - .on_click(cx.listener(move |this, _, window, cx| { - this.run_strip_action(action.clone(), window, cx); - })) - .on_mouse_down(MouseButton::Left, |_, _, cx| cx.stop_propagation()), - ) + .child( + h_flex() + .items_center() + .gap_2() + .child(gpui_component::Icon::new(IconName::Globe)) + .child(message) + .when_some(action, |this, (label, action)| { + this.child( + Button::new("home-remote-status-action") + .label(label) + .ghost() + .small() + .on_click(cx.listener(move |this, _, window, cx| { + this.run_strip_action(action.clone(), window, cx); + })) + .on_mouse_down(MouseButton::Left, |_, _, cx| { + cx.stop_propagation() + }), + ) + }), + ) + .when_some(installing, |this, phase| { + this.child(crate::ui::remote_workspace::install_progress_bar(phase, cx)) }), ) } diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 2a4f6012..b57c41ce 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -168,6 +168,56 @@ pub(crate) fn mismatch_action_key(refusal: &str) -> L10nKey { } } +/// One line for an install in flight — bytes moved, or the wait for the far end +/// to come back up. Shared by the switcher's progress bar and the strip's, so a +/// user watching both is not told two different things. +pub(crate) fn install_phase_caption(phase: crate::daemon::install::InstallPhase) -> String { + use crate::daemon::install::InstallPhase; + use crate::ui::remote_connect::human_bytes; + match phase { + InstallPhase::Restarting => t(L10nKey::SwitcherRestartingServer).to_string(), + InstallPhase::Downloading { done, total } => match total { + Some(total) => t_fmt( + L10nKey::SwitcherDownloadingServerWithTotal, + &[("done", &human_bytes(done)), ("total", &human_bytes(total))], + ), + None => t_fmt( + L10nKey::SwitcherDownloadingServerNoTotal, + &[("done", &human_bytes(done))], + ), + }, + InstallPhase::Uploading { done, total } => t_fmt( + L10nKey::SwitcherCopyingServer, + &[("done", &human_bytes(done)), ("total", &human_bytes(total))], + ), + } +} + +/// The thin filled bar under an install's caption. `Restarting` has no +/// fraction to show — the far end is either back or it is not — so it draws +/// empty, which is what the switcher has always done and is still better than +/// the bar vanishing for the last leg. +pub(crate) fn install_progress_bar( + phase: crate::daemon::install::InstallPhase, + cx: &gpui::App, +) -> impl gpui::IntoElement + use<> { + use gpui::{ParentElement as _, Styled as _}; + use gpui_component::ActiveTheme as _; + let theme = cx.theme(); + gpui::div() + .w_full() + .h(gpui::px(3.)) + .rounded_full() + .bg(theme.border) + .child( + gpui::div() + .h_full() + .w(gpui::relative(phase.fraction().unwrap_or(0.0))) + .rounded_full() + .bg(theme.warning), + ) +} + /// What the remote strip's button is for, once the status has been read. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum StripAction { @@ -401,6 +451,22 @@ impl Tty7App { }) } + /// What the install running against *this* window's machine has reached, if + /// one is running at all. + /// + /// The switcher has drawn this since installs got a progress bar, but the + /// strip never did — so pressing Update Server on a parked workspace with + /// no switcher open froze the screen for as long as the download took and + /// then produced a modal, with nothing in between. Same source, so + /// the two cannot disagree about how far along it is. + pub(crate) fn remote_strip_progress( + &self, + cx: &gpui::App, + ) -> Option { + let own = WorkspaceStore::remote_ref(cx, self.workspace)?; + remote_connect::install_progress_for(own.target.host_id()) + } + pub(crate) fn run_strip_action( &mut self, action: StripAction, diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index 057e53c5..55af44d3 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -18,7 +18,7 @@ use crate::daemon::install::InstallPhase; use crate::terminal::pane_liveness::Liveness; use crate::ui::app::Tty7App; use crate::ui::i18n::{L10nKey, t, t_fmt}; -use crate::ui::remote_connect::{self, HostChoice, RemoteWorkspaceRow, human_bytes}; +use crate::ui::remote_connect::{self, HostChoice, RemoteWorkspaceRow}; use crate::ui::remote_workspace::{ConnectFlow, MachineStatus, RemoteLinks}; const CARD_W: f32 = 840.0; @@ -2156,23 +2156,7 @@ impl Tty7App { let theme = cx.theme(); let accent = theme.warning; let fraction = phase.fraction().unwrap_or(0.0); - let caption = match phase { - InstallPhase::Restarting => t(L10nKey::SwitcherRestartingServer).to_string(), - InstallPhase::Downloading { done, total } => match total { - Some(total) => t_fmt( - L10nKey::SwitcherDownloadingServerWithTotal, - &[("done", &human_bytes(done)), ("total", &human_bytes(total))], - ), - None => t_fmt( - L10nKey::SwitcherDownloadingServerNoTotal, - &[("done", &human_bytes(done))], - ), - }, - InstallPhase::Uploading { done, total } => t_fmt( - L10nKey::SwitcherCopyingServer, - &[("done", &human_bytes(done)), ("total", &human_bytes(total))], - ), - }; + let caption = crate::ui::remote_workspace::install_phase_caption(phase); v_flex() .gap(px(6.))