fix(forwards): say why a forward was refused, and never lose the rule it replaces (#514)

The managed-forward form in the Forwards panel used to bare-`return` from Add
whenever the fields did not make a rule — an unparseable bind port, a target
port of 0, an empty target host for a Local or Remote rule. The button did
nothing, said nothing, and left the form exactly as it was. It now collects
the fields through the same conditions the settings sheet applies
(`ForwardRuleForm::collect`), keeps Add disabled while there is nothing to
add, and shows the settings sheet's own "needs both" / "needs a listen port"
line under the form once it has been touched.

Saving an edit removed the old forward and then added the new one, so a rule
that could not be started took a working forward with it. Remove-then-add is
still the order — the ordinary edit keeps the bind port, and adding first
would collide with the very rule being replaced — but the panel now looks at
the entry the add appended, and on `ForwardStatus::Error` it takes that entry
back out, puts the old rule back, and keeps the form open with the reason
under it. `mf_editing` carries the whole `ManagedForward` rather than an id so
there is something to put back, and it is re-pointed at the restored entry,
which comes back under a new id.

Adjacent, and the reason a failed request could not be told from a successful
one: `ForwardRoute::add`/`remove` and their `RemoteTerminal` fallbacks turned
every transport error into an empty `Vec`, which the caller then assigned
straight into the panel's list — so one unreachable round trip blanked a panel
full of live forwards. They return `Option` now, and the panel only takes a
list the far side actually sent.

Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com>

src/terminal/remote.rs

@@ -1494,7 +1494,10 @@ impl RemoteTerminal {
         query(pane_id).unwrap_or_else(|e| Err(e.to_string()))
     }
 
-    pub fn add_forward(pane_id: u64, rule: SshForwardRule) -> Vec<ManagedForward> {
+    /// `None` when the request never got a list back — which is not the same
+    /// as getting an empty one, because only the caller of a *failed* request
+    /// still has to keep showing what it had.
+    pub fn add_forward(pane_id: u64, rule: SshForwardRule) -> Option<Vec<ManagedForward>> {
         fn query(pane_id: u64, rule: SshForwardRule) -> anyhow::Result<Vec<ManagedForward>> {
             let mut stream = connect()?;
             ClientMsg::AddForward { pane_id, rule }.encode(&mut stream)?;
@@ -1504,10 +1507,13 @@ impl RemoteTerminal {
                 other => Err(anyhow::anyhow!("unexpected reply to AddForward: {other:?}")),
             }
         }
-        query(pane_id, rule).unwrap_or_default()
+        query(pane_id, rule)
+            .inspect_err(|e| log::warn!("AddForward failed: {e}"))
+            .ok()
     }
 
-    pub fn remove_forward(pane_id: u64, forward_id: u64) -> Vec<ManagedForward> {
+    /// `None` when the request never got a list back — see `add_forward`.
+    pub fn remove_forward(pane_id: u64, forward_id: u64) -> Option<Vec<ManagedForward>> {
         fn query(pane_id: u64, forward_id: u64) -> anyhow::Result<Vec<ManagedForward>> {
             let mut stream = connect()?;
             ClientMsg::RemoveForward {
@@ -1522,7 +1528,9 @@ impl RemoteTerminal {
                 )),
             }
         }
-        query(pane_id, forward_id).unwrap_or_default()
+        query(pane_id, forward_id)
+            .inspect_err(|e| log::warn!("RemoveForward failed: {e}"))
+            .ok()
     }
 
     pub fn list_forwards(pane_id: u64) -> Vec<ManagedForward> {

src/ui/app.rs

@@ -27,6 +27,7 @@ use crate::core::window_state::{WindowGeometry as _, WindowState};
 use crate::daemon::protocol::{RemoteContext, ShellSpec, ssh_option_takes_value};
 use crate::daemon::spawn::DaemonMismatch;
 use crate::terminal::view::{ChildExited, TerminalView};
+use crate::ui::forwards::{ForwardFields, added_forward, rule_of};
 use crate::ui::host_registry::HostId;
 use crate::ui::i18n::{L10nKey, set_locale, t, t_fmt, t_plural};
 use crate::ui::palette::{
@@ -398,7 +399,13 @@ pub(crate) struct LoopbackForwardPanelState {
     pub(crate) mf_target_host: Entity<InputState>,
     pub(crate) mf_target_port: Entity<InputState>,
     pub(crate) mf_description: Entity<InputState>,
-    pub(crate) mf_editing: Option<u64>,
+    /// The rule the form is editing, whole rather than by id: an edit that
+    /// fails has to be able to put back what it took out, and the id alone
+    /// cannot describe the rule it named.
+    pub(crate) mf_editing: Option<crate::daemon::protocol::ManagedForward>,
+    /// Why the last Add or Save did not take, in the far side's own words.
+    /// Cleared the moment the form is closed or the edit is abandoned.
+    pub(crate) mf_error: Option<String>,
 }
 
 pub struct Tty7App {
@@ -985,6 +992,7 @@ impl Tty7App {
                 mf_target_port,
                 mf_description,
                 mf_editing: None,
+                mf_error: None,
             },
             sftp_panel,
             right_panel: Default::default(),
@@ -2196,74 +2204,105 @@ impl Tty7App {
         cx.notify();
     }
 
+    /// The managed-forward form's fields as plain text, for the two callers
+    /// that have to agree on what they add up to.
+    pub(crate) fn managed_forward_fields(&self, cx: &gpui::App) -> ForwardFields {
+        let val = |input: &Entity<InputState>| input.read(cx).value().to_string();
+        ForwardFields {
+            kind: self.loopback_panel.mf_kind,
+            bind_host: val(&self.loopback_panel.mf_bind_host),
+            bind_port: val(&self.loopback_panel.mf_bind_port),
+            target_host: val(&self.loopback_panel.mf_target_host),
+            target_port: val(&self.loopback_panel.mf_target_port),
+            description: val(&self.loopback_panel.mf_description),
+        }
+    }
+
     pub(crate) fn add_managed_forward(
         &mut self,
         pane_id: u64,
         window: &mut Window,
         cx: &mut Context<Self>,
     ) {
-        use crate::daemon::protocol::{SshForwardKind, SshForwardRule};
-        let kind = self.loopback_panel.mf_kind;
-        let bind_host = self
-            .loopback_panel
-            .mf_bind_host
-            .read(cx)
-            .value()
-            .trim()
-            .to_string();
-        let bind_host = if bind_host.is_empty() {
-            "127.0.0.1".to_string()
-        } else {
-            bind_host
-        };
-        let Ok(bind_port) = self
-            .loopback_panel
-            .mf_bind_port
-            .read(cx)
-            .value()
-            .trim()
-            .parse::<u16>()
-        else {
+        use crate::daemon::protocol::ForwardStatus;
+
+        let Some(rule) = self.managed_forward_fields(cx).collect() else {
+            // Add is disabled while the fields do not make a rule and the form
+            // already says what is missing, so there is nothing to do here and
+            // nothing left to explain.
             return;
         };
-        let target_host = self
-            .loopback_panel
-            .mf_target_host
-            .read(cx)
-            .value()
-            .trim()
-            .to_string();
-        let target_port = self
-            .loopback_panel
-            .mf_target_port
-            .read(cx)
-            .value()
-            .trim()
-            .parse::<u16>()
-            .unwrap_or(0);
-        if kind != SshForwardKind::Dynamic && (target_host.is_empty() || target_port == 0) {
-            return;
-        }
-        let description = self
-            .loopback_panel
-            .mf_description
-            .read(cx)
-            .value()
-            .trim()
-            .to_string();
-        let rule = SshForwardRule {
-            kind,
-            bind_host,
-            bind_port,
-            target_host,
-            target_port,
-            description: (!description.is_empty()).then_some(description),
-        };
         let route = self.forward_route(pane_id, cx);
-        if let Some(old_id) = self.loopback_panel.mf_editing.take() {
-            let _ = route.remove(old_id);
+        let previous = self.loopback_panel.mf_editing.clone();
+        // A saved edit is a replace, and the rule being replaced has to come
+        // out first: the ordinary edit keeps the bind port, and the far side
+        // really does bind it, so adding first would collide with the very
+        // rule it is replacing and fail every edit that only renames a rule or
+        // moves its target.
+        if let Some(old) = &previous {
+            let Some(list) = route.remove(old.id) else {
+                // Nothing came back, so what the far side still has is
+                // unknown — most likely the old rule, still listening. Adding
+                // on top of that would collide with it, and putting it back
+                // afterwards would leave two of it. Stop while nothing has
+                // changed.
+                self.loopback_panel.mf_error = Some(t(L10nKey::ForwardRequestFailed).to_string());
+                cx.notify();
+                return;
+            };
+            self.loopback_panel.managed = list;
         }
-        self.loopback_panel.managed = route.add(rule);
+
+        let before: Vec<u64> = self.loopback_panel.managed.iter().map(|m| m.id).collect();
+        let mut failure = None;
+        match route.add(rule) {
+            // The request never got an answer. An empty list here is not "this
+            // pane has no forwards", it is "nobody said" — assigning it is what
+            // used to blank the panel on a dropped connection.
+            None => failure = Some(t(L10nKey::ForwardRequestFailed).to_string()),
+            Some(list) => {
+                // A rule that could not be started is registered all the same,
+                // with the reason in its status, so whether the add worked is a
+                // question about the entry it appended rather than about
+                // whether the call returned.
+                let broken = added_forward(&before, &list).and_then(|added| match &added.status {
+                    ForwardStatus::Error(msg) => Some((added.id, msg.clone())),
+                    ForwardStatus::Listening => None,
+                });
+                self.loopback_panel.managed = list;
+                if let Some((id, msg)) = broken {
+                    if let Some(list) = route.remove(id) {
+                        self.loopback_panel.managed = list;
+                    }
+                    failure = Some(msg);
+                }
+            }
+        }
+
+        if let Some(msg) = failure {
+            // Put back what the edit took out, so the worst a failed Save can
+            // do is leave everything exactly as it was — with the form still
+            // open on the rule and the reason underneath it.
+            if let Some(old) = &previous {
+                let before: Vec<u64> = self.loopback_panel.managed.iter().map(|m| m.id).collect();
+                if let Some(list) = route.add(rule_of(old)) {
+                    // The rule comes back under a new id and the form is still
+                    // editing it, so the form has to be pointed at the entry
+                    // that now exists — otherwise the next Save would remove
+                    // an id nobody has and add a second copy of the rule.
+                    if let Some(restored) = added_forward(&before, &list) {
+                        self.loopback_panel.mf_editing = Some(restored.clone());
+                    }
+                    self.loopback_panel.managed = list;
+                }
+            }
+            self.loopback_panel.mf_error = Some(msg);
+            cx.notify();
+            return;
+        }
+
+        self.loopback_panel.mf_editing = None;
+        self.loopback_panel.mf_error = None;
         self.loopback_panel.form_pane_id = None;
         for input in [
             &self.loopback_panel.mf_bind_port,
@@ -2283,8 +2322,8 @@ impl Tty7App {
         cx: &mut Context<Self>,
     ) {
         self.loopback_panel.mf_kind = forward.kind;
-        self.loopback_panel.mf_editing = Some(forward.id);
         self.loopback_panel.form_pane_id = Some(forward.pane_id);
+        self.loopback_panel.mf_error = None;
         let target_port = if forward.target_port == 0 {
             String::new()
         } else {
@@ -2309,6 +2348,7 @@ impl Tty7App {
         for (input, value) in fields {
             input.update(cx, |input, cx| input.set_value(&value, window, cx));
         }
+        self.loopback_panel.mf_editing = Some(forward);
         cx.notify();
     }
 
@@ -2318,6 +2358,7 @@ impl Tty7App {
         cx: &mut Context<Self>,
     ) {
         self.loopback_panel.mf_editing = None;
+        self.loopback_panel.mf_error = None;
         for input in [
             &self.loopback_panel.mf_bind_port,
             &self.loopback_panel.mf_target_host,
@@ -2338,7 +2379,12 @@ impl Tty7App {
         forward_id: u64,
         cx: &mut Context<Self>,
     ) {
-        self.loopback_panel.managed = self.forward_route(pane_id, cx).remove(forward_id);
+        // Only what the far side actually answered with. A request that never
+        // got a reply knows nothing about the remaining forwards, and writing
+        // its empty list into the panel would blank a list that is still there.
+        if let Some(list) = self.forward_route(pane_id, cx).remove(forward_id) {
+            self.loopback_panel.managed = list;
+        }
         cx.notify();
     }
 
@@ -5910,18 +5956,26 @@ impl ForwardRoute {
         )
     }
 
+    /// The list a forward request answered with, or `None` when it did not
+    /// answer at all.
+    ///
+    /// The two are not the same and the panel has to be able to tell them
+    /// apart: an empty list is a pane with no forwards left, while a request
+    /// that failed says nothing about what the far side still has. Reporting
+    /// the second as the first is what blanked the panel whenever the daemon
+    /// was briefly unreachable.
     fn forwards(
         reply: anyhow::Result<crate::daemon::protocol::DaemonMsg>,
-    ) -> Vec<crate::daemon::protocol::ManagedForward> {
+    ) -> Option<Vec<crate::daemon::protocol::ManagedForward>> {
         match reply {
-            Ok(crate::daemon::protocol::DaemonMsg::ForwardList(list)) => list,
+            Ok(crate::daemon::protocol::DaemonMsg::ForwardList(list)) => Some(list),
             Ok(other) => {
                 log::warn!("unexpected reply to a workspace forward request: {other:?}");
-                Vec::new()
+                None
             }
             Err(e) => {
                 log::warn!("a workspace forward request failed: {e}");
-                Vec::new()
+                None
             }
         }
     }
@@ -5931,13 +5985,13 @@ impl ForwardRoute {
         else {
             return crate::terminal::RemoteTerminal::list_forwards(self.pane_id);
         };
-        Self::forwards(crate::terminal::RemoteTerminal::on_workspace(req))
+        Self::forwards(crate::terminal::RemoteTerminal::on_workspace(req)).unwrap_or_default()
     }
 
     pub(crate) fn add(
         &self,
         rule: crate::daemon::protocol::SshForwardRule,
-    ) -> Vec<crate::daemon::protocol::ManagedForward> {
+    ) -> Option<Vec<crate::daemon::protocol::ManagedForward>> {
         let Some(req) = self
             .workspace_op(crate::daemon::protocol::WorkspaceOp::AddForward { rule: rule.clone() })
         else {
@@ -5951,10 +6005,13 @@ impl ForwardRoute {
         else {
             return Vec::new();
         };
-        Self::forwards(crate::terminal::RemoteTerminal::on_workspace(req))
+        Self::forwards(crate::terminal::RemoteTerminal::on_workspace(req)).unwrap_or_default()
     }
 
-    pub(crate) fn remove(&self, forward_id: u64) -> Vec<crate::daemon::protocol::ManagedForward> {
+    pub(crate) fn remove(
+        &self,
+        forward_id: u64,
+    ) -> Option<Vec<crate::daemon::protocol::ManagedForward>> {
         let Some(req) =
             self.workspace_op(crate::daemon::protocol::WorkspaceOp::RemoveForward { forward_id })
         else {
@@ -8148,3 +8205,97 @@ mod rename_gpui_tests {
         });
     }
 }
+
+// A test window has no daemon behind it — its socket path is under the pinned
+// test config dir and nothing is listening on it — so every forward request
+// fails. That is exactly the case these are about: what the panel and the form
+// are left holding when the far side does not answer.
+#[cfg(all(test, unix))]
+mod managed_forward_gpui_tests {
+    use gpui::TestAppContext;
+    use gpui_component::input::InputState;
+
+    use crate::daemon::protocol::{ForwardStatus, ManagedForward, SshForwardKind};
+    use crate::ui::app::test_window::harness_with_tabs;
+
+    fn listening(id: u64) -> ManagedForward {
+        ManagedForward {
+            id,
+            pane_id: 1,
+            kind: SshForwardKind::Local,
+            bind_host: "127.0.0.1".to_string(),
+            bind_port: 8080,
+            target_host: "10.0.0.5".to_string(),
+            target_port: 80,
+            description: None,
+            status: ForwardStatus::Listening,
+        }
+    }
+
+    #[gpui::test]
+    fn an_add_that_never_reaches_the_session_leaves_the_panel_as_it_was(cx: &mut TestAppContext) {
+        let (app, mut vcx, _streams) = harness_with_tabs(cx, 1);
+
+        app.update_in(&mut vcx, |app, window, cx| {
+            app.loopback_panel.managed = vec![listening(1)];
+            app.loopback_panel.form_pane_id = Some(1);
+            let typed: [(&gpui::Entity<InputState>, &str); 3] = [
+                (&app.loopback_panel.mf_bind_port, "9000"),
+                (&app.loopback_panel.mf_target_host, "127.0.0.1"),
+                (&app.loopback_panel.mf_target_port, "22"),
+            ];
+            for (input, value) in typed {
+                input.update(cx, |input, cx| input.set_value(value, window, cx));
+            }
+
+            app.add_managed_forward(1, window, cx);
+
+            assert_eq!(
+                app.loopback_panel.managed.len(),
+                1,
+                "a request that failed says nothing about the forwards that are up"
+            );
+            assert!(
+                app.loopback_panel.mf_error.is_some(),
+                "and the form has to say why the Add did nothing"
+            );
+            assert_eq!(
+                app.loopback_panel.form_pane_id,
+                Some(1),
+                "the form stays open on what was typed"
+            );
+        });
+    }
+
+    #[gpui::test]
+    fn a_save_that_cannot_be_made_leaves_the_rule_it_would_replace_alone(cx: &mut TestAppContext) {
+        let (app, mut vcx, _streams) = harness_with_tabs(cx, 1);
+
+        app.update_in(&mut vcx, |app, window, cx| {
+            app.loopback_panel.managed = vec![listening(1)];
+            app.loopback_panel.form_pane_id = Some(1);
+            app.loopback_panel.mf_editing = Some(listening(1));
+            let typed: [(&gpui::Entity<InputState>, &str); 3] = [
+                (&app.loopback_panel.mf_bind_port, "8080"),
+                (&app.loopback_panel.mf_target_host, "10.0.0.6"),
+                (&app.loopback_panel.mf_target_port, "80"),
+            ];
+            for (input, value) in typed {
+                input.update(cx, |input, cx| input.set_value(value, window, cx));
+            }
+
+            app.add_managed_forward(1, window, cx);
+
+            assert_eq!(
+                app.loopback_panel.managed,
+                vec![listening(1)],
+                "the rule being edited must survive an edit that could not be made"
+            );
+            assert!(
+                app.loopback_panel.mf_editing.is_some(),
+                "the form is still editing it"
+            );
+            assert!(app.loopback_panel.mf_error.is_some());
+        });
+    }
+}

src/ui/forwards.rs

@@ -1,9 +1,11 @@
 use gpui::{AnyElement, Context, Div, Entity, FontWeight, Stateful, div, prelude::*, px, rems};
 use gpui_component::button::{Button, ButtonVariants as _};
 use gpui_component::input::Input;
-use gpui_component::{ActiveTheme as _, Icon, IconName, Sizable as _, h_flex, v_flex};
+use gpui_component::{
+    ActiveTheme as _, Disableable as _, Icon, IconName, Sizable as _, h_flex, v_flex,
+};
 
-use crate::daemon::protocol::{ForwardStatus, ManagedForward, SshForwardKind};
+use crate::daemon::protocol::{ForwardStatus, ManagedForward, SshForwardKind, SshForwardRule};
 use crate::terminal::view::TerminalView;
 use crate::ui::app::{CONTENT_INSET, TILE_GLYPH_SM, TILE_SIZE_SM, Tty7App};
 use crate::ui::i18n::{L10nKey, t, t_fmt};
@@ -14,6 +16,103 @@ use crate::ui::right_panel::{META, TEXT, TEXT_MONO};
 /// draw the same row, so they fade it by the same amount.
 pub(crate) const NO_TARGET_FADE: f32 = 0.4;
 
+/// The managed-forward form's five text fields, read out of their inputs.
+///
+/// Split out so the question "do these make a rule?" can be asked without a
+/// `Window` and answered the same way twice: `add_managed_forward` needs the
+/// rule, and `forward_form` needs to know whether there is one yet — that is
+/// what decides whether Add is live and whether the form says what is missing.
+pub(crate) struct ForwardFields {
+    pub(crate) kind: SshForwardKind,
+    pub(crate) bind_host: String,
+    pub(crate) bind_port: String,
+    pub(crate) target_host: String,
+    pub(crate) target_port: String,
+    pub(crate) description: String,
+}
+
+impl ForwardFields {
+    /// The rule these fields describe, or `None` while they do not describe
+    /// one yet.
+    ///
+    /// The same conditions the settings sheet's `ForwardRuleForm::collect`
+    /// applies, so a rule typed here and a rule typed there are accepted or
+    /// refused alike — including port 0, which parses as a `u16` but asks the
+    /// OS to pick the port, and there is nowhere in either form to say which
+    /// one it picked.
+    pub(crate) fn collect(&self) -> Option<SshForwardRule> {
+        let bind_port: u16 = self.bind_port.trim().parse().ok().filter(|p| *p > 0)?;
+        let (target_host, target_port) = if self.kind == SshForwardKind::Dynamic {
+            (String::new(), 0)
+        } else {
+            let port: u16 = self.target_port.trim().parse().ok().filter(|p| *p > 0)?;
+            let host = self.target_host.trim();
+            if host.is_empty() {
+                return None;
+            }
+            (host.to_string(), port)
+        };
+        let bind_host = match self.bind_host.trim() {
+            // The panel's own default, and the one the strip's tooltip
+            // promises: an empty bind host is loopback, not every interface.
+            "" => "127.0.0.1".to_string(),
+            host => host.to_string(),
+        };
+        let description = self.description.trim();
+        Some(SshForwardRule {
+            kind: self.kind,
+            bind_host,
+            bind_port,
+            target_host,
+            target_port,
+            description: (!description.is_empty()).then(|| description.to_string()),
+        })
+    }
+
+    /// Whether the form is still empty enough that saying what is missing
+    /// would be nagging rather than helping — the same restraint the settings
+    /// sheet shows through `ForwardRuleForm::is_blank`.
+    pub(crate) fn is_blank(&self) -> bool {
+        [
+            &self.bind_host,
+            &self.bind_port,
+            &self.target_host,
+            &self.target_port,
+            &self.description,
+        ]
+        .iter()
+        .all(|v| v.trim().is_empty())
+    }
+}
+
+/// The entry a forward request just appended: the one the panel did not have
+/// before it asked.
+///
+/// Ids come from a counter that only goes up, so "none of the ids from before"
+/// names the new entry exactly — and it is the new entry that says whether the
+/// rule is listening or why it is not.
+pub(crate) fn added_forward<'a>(
+    before: &[u64],
+    list: &'a [ManagedForward],
+) -> Option<&'a ManagedForward> {
+    list.iter().find(|m| !before.contains(&m.id))
+}
+
+/// The rule a live forward was made from.
+///
+/// An edit removes the old forward before adding the new one, so when the new
+/// one will not come up this is what puts the old one back.
+pub(crate) fn rule_of(forward: &ManagedForward) -> SshForwardRule {
+    SshForwardRule {
+        kind: forward.kind,
+        bind_host: forward.bind_host.clone(),
+        bind_port: forward.bind_port,
+        target_host: forward.target_host.clone(),
+        target_port: forward.target_port,
+        description: forward.description.clone(),
+    }
+}
+
 impl Tty7App {
     pub(crate) fn render_ssh_status_strip(
         &self,
@@ -315,9 +414,17 @@ impl Tty7App {
     fn forward_form(&self, pane_id: u64, cx: &mut Context<Self>) -> Div {
         let theme = cx.theme();
         let muted = theme.muted_foreground;
+        let danger = theme.danger;
         let sf = cx.global::<crate::ui::presets::Surfaces>().sidebar;
         let kind = self.loopback_panel.mf_kind;
         let editing = self.loopback_panel.mf_editing.is_some();
+        let fields = self.managed_forward_fields(cx);
+        // The form used to accept a click on Add and then do nothing at all
+        // when the fields did not make a rule. Now Add is only live when there
+        // is something to add, and the line below the form says what is still
+        // missing — but not while the form has barely been touched.
+        let complete = fields.collect().is_some();
+        let incomplete = !complete && !fields.is_blank();
         let selected = match kind {
             SshForwardKind::Local => 0,
             SshForwardKind::Remote => 1,
@@ -387,6 +494,17 @@ impl Tty7App {
                     )),
             )
             .child(Input::new(&self.loopback_panel.mf_description).xsmall())
+            .when(incomplete, |form| {
+                form.child(div().text_size(rems(META)).text_color(danger).child(
+                    match needs_target {
+                        true => t(L10nKey::SettingsFwdNeedsBoth),
+                        false => t(L10nKey::SettingsFwdNeedsListen),
+                    },
+                ))
+            })
+            .when_some(self.loopback_panel.mf_error.clone(), |form, msg| {
+                form.child(div().text_size(rems(META)).text_color(danger).child(msg))
+            })
             .child(
                 h_flex()
                     .justify_end()
@@ -410,6 +528,7 @@ impl Tty7App {
                             })
                             .primary()
                             .xsmall()
+                            .disabled(!complete)
                             .on_click(cx.listener(move |this, _, window, cx| {
                                 this.add_managed_forward(pane_id, window, cx)
                             })),
@@ -417,3 +536,127 @@ impl Tty7App {
             )
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn fields(kind: SshForwardKind, bind_port: &str, host: &str, port: &str) -> ForwardFields {
+        ForwardFields {
+            kind,
+            bind_host: "127.0.0.1".to_string(),
+            bind_port: bind_port.to_string(),
+            target_host: host.to_string(),
+            target_port: port.to_string(),
+            description: String::new(),
+        }
+    }
+
+    fn managed(id: u64, bind_port: u16) -> ManagedForward {
+        ManagedForward {
+            id,
+            pane_id: 7,
+            kind: SshForwardKind::Local,
+            bind_host: "127.0.0.1".to_string(),
+            bind_port,
+            target_host: "10.0.0.5".to_string(),
+            target_port: 80,
+            description: Some("the staging box".to_string()),
+            status: ForwardStatus::Listening,
+        }
+    }
+
+    #[test]
+    fn a_complete_local_rule_is_collected() {
+        let rule = fields(SshForwardKind::Local, "8080", "10.0.0.5", "80")
+            .collect()
+            .expect("a bind port and a target make a rule");
+        assert_eq!(rule.bind_port, 8080);
+        assert_eq!(rule.target_host, "10.0.0.5");
+        assert_eq!(rule.target_port, 80);
+        assert_eq!(rule.description, None);
+    }
+
+    #[test]
+    fn a_half_typed_rule_is_not_a_rule() {
+        assert!(
+            fields(SshForwardKind::Local, "", "10.0.0.5", "80")
+                .collect()
+                .is_none()
+        );
+        assert!(
+            fields(SshForwardKind::Local, "8080", "", "80")
+                .collect()
+                .is_none()
+        );
+        assert!(
+            fields(SshForwardKind::Local, "8080", "10.0.0.5", "")
+                .collect()
+                .is_none()
+        );
+        assert!(
+            fields(SshForwardKind::Local, "http", "10.0.0.5", "80")
+                .collect()
+                .is_none(),
+            "a service name is not a port"
+        );
+    }
+
+    #[test]
+    fn port_zero_is_refused_rather_than_quietly_ephemeral() {
+        assert!(
+            fields(SshForwardKind::Local, "0", "10.0.0.5", "80")
+                .collect()
+                .is_none(),
+            "there is nowhere in this form to say which port the OS picked"
+        );
+        assert!(
+            fields(SshForwardKind::Local, "8080", "10.0.0.5", "0")
+                .collect()
+                .is_none()
+        );
+    }
+
+    #[test]
+    fn a_socks_proxy_needs_nothing_but_a_port_to_listen_on() {
+        let rule = fields(SshForwardKind::Dynamic, "1080", "", "")
+            .collect()
+            .expect("a dynamic forward has no target");
+        assert_eq!(rule.bind_port, 1080);
+        assert_eq!(rule.target_host, "");
+        assert_eq!(rule.target_port, 0);
+    }
+
+    #[test]
+    fn an_untouched_form_is_blank_and_a_touched_one_is_not() {
+        let mut form = fields(SshForwardKind::Local, "", "", "");
+        form.bind_host = String::new();
+        assert!(form.is_blank());
+        form.description = "  ".to_string();
+        assert!(form.is_blank(), "whitespace is not typing");
+        form.bind_port = "8".to_string();
+        assert!(!form.is_blank());
+    }
+
+    #[test]
+    fn a_rule_survives_the_round_trip_through_a_live_forward() {
+        let rule = rule_of(&managed(3, 8080));
+        assert_eq!(rule.kind, SshForwardKind::Local);
+        assert_eq!(rule.bind_host, "127.0.0.1");
+        assert_eq!(rule.bind_port, 8080);
+        assert_eq!(rule.target_host, "10.0.0.5");
+        assert_eq!(rule.target_port, 80);
+        assert_eq!(rule.description.as_deref(), Some("the staging box"));
+    }
+
+    #[test]
+    fn the_entry_an_add_appended_is_the_one_that_was_not_there_before() {
+        let list = vec![managed(1, 8080), managed(4, 9090)];
+        let added = added_forward(&[1], &list).expect("the new entry");
+        assert_eq!(added.id, 4);
+        assert!(
+            added_forward(&[1, 4], &list).is_none(),
+            "nothing was added, so there is nothing to point at"
+        );
+    }
+}

src/ui/i18n/en.rs

@@ -834,6 +834,7 @@ pub fn translate_en(key: L10nKey) -> &'static str {
         L10nKey::ForwardToLabel => "to",
         L10nKey::ForwardSocksLabel => "SOCKS",
         L10nKey::ForwardAdd => "Add",
+        L10nKey::ForwardRequestFailed => "Could not reach the session — nothing changed.",
         L10nKey::FileTreePlaceholderFileName => "file name",
         L10nKey::FileTreePlaceholderFolderName => "folder name",
         L10nKey::FileTreePlaceholderNewName => "new name",

src/ui/i18n/ja.rs

@@ -876,6 +876,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> {
         L10nKey::ForwardToLabel => "転送先",
         L10nKey::ForwardSocksLabel => "SOCKS",
         L10nKey::ForwardAdd => "追加",
+        L10nKey::ForwardRequestFailed => "セッションに届きませんでした。何も変更していません",
         L10nKey::FileTreePlaceholderFileName => "ファイル名",
         L10nKey::FileTreePlaceholderFolderName => "フォルダ名",
         L10nKey::FileTreePlaceholderNewName => "新しい名前",

src/ui/i18n/mod.rs

@@ -640,6 +640,7 @@ l10n_keys! {
     ForwardToLabel,
     ForwardSocksLabel,
     ForwardAdd,
+    ForwardRequestFailed,
     FileTreePlaceholderFileName,
     FileTreePlaceholderFolderName,
     FileTreePlaceholderNewName,

src/ui/i18n/zh.rs

@@ -801,6 +801,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> {
         L10nKey::ForwardToLabel => "到",
         L10nKey::ForwardSocksLabel => "SOCKS",
         L10nKey::ForwardAdd => "添加",
+        L10nKey::ForwardRequestFailed => "联系不上这个会话——什么都没有改动。",
         L10nKey::FileTreePlaceholderFileName => "文件名",
         L10nKey::FileTreePlaceholderFolderName => "文件夹名",
         L10nKey::FileTreePlaceholderNewName => "新名称",