test(keymap): ask the keymap what is bound, and cover the reload that binds it (#582) (#609)

The keymap tests asserted against `bound_keystrokes`, a `#[cfg(test)]` mirror
of `action_bindings` that #571 left behind when it removed the `NoAction`
retire loop — the last production reader of it. A mirror is a second
implementation of the table, so those tests could agree with it while the app
bound something else.

Each one now builds a `gpui::Keymap` from `action_bindings` and performs the
lookup gpui performs on a keypress: typing this chord in that context runs that
action. The paste test gains the other half it never asserted — that a terminal
chord does nothing outside a terminal — and the context test is renamed for
what it now checks. `bound_keystrokes` is deleted.

The watcher had no coverage at all, so the one thing #548 is for — hand-editing
config.json and having the new chord fire without a restart — was pinned
nowhere. Its tick moves out of the closure into `apply_reloaded_config`: same
statements in the same order, taking the load result as an argument and
returning whether it rebound, which the watcher ignores. Three tests drive it
against a live keymap — a reload that binds Ctrl+Alt+9 to SplitRight makes that
chord dispatch it, a reload that moves no binding does not rebuild the map, and
a quarantined reload rebinds nothing, because the global config is deliberately
not replaced on that path and the user's keys have to survive a typo.

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

src/main.rs

@@ -84,65 +84,78 @@ fn spawn_config_watcher(cx: &mut App) {
             while rx.try_recv().is_ok() {}
 
             cx.update(|cx| {
-                let (config, outcome) = Config::load_with_outcome();
-                if outcome.failed() {
-                    // Keep the settings the app is running on: swapping the
-                    // stand-in defaults in would flash the whole UI onto
-                    // defaults, and the load already parked the broken file
-                    // beside the original. It reloads itself the moment the
-                    // file parses again.
-                    if !announced {
-                        announced = true;
-                        notify_config_load_failed(cx, outcome, false);
-                    }
-                    // The theme files this same watcher covers must keep
-                    // hot-reloading: a typo in config.json is no reason for
-                    // theme editing to go dead until the app restarts. They
-                    // read the global config, which is deliberately still the
-                    // one the app is running on.
-                    reload_themes(cx);
-                    cx.refresh_windows();
-                    return;
-                }
-                announced = false;
-                // The keymap is rebuilt only when a binding actually moved.
-                // This watcher fires for every write under the config dir —
-                // including the app's own `save()`, which a sidebar drag or a
-                // palette open triggers — so reloading bindings on each tick
-                // would rebuild the whole keymap for nothing, and (before
-                // `rebind` cleared first) leak a full table per tick (#548).
-                // Read past the early return above: a load that failed leaves
-                // the global alone, so there is nothing to compare and the
-                // user's keys stay in the keymap the app is dispatching on.
-                let keymap_before = crate::ui::keymap::keybinding_config(cx);
-                crate::ui::i18n::set_locale(&config.gui_language);
-                cx.set_global(config);
-                reload_themes(cx);
-                crate::ui::theme::apply_cursor_hide_mode(cx);
-                // The menu bar is built once from the current locale, so editing
-                // gui_language by hand has to rebuild it the same way the
-                // in-app language picker does.
-                crate::ui::theme::set_menus(cx);
-                crate::ui::windows::WindowRegistry::refresh_locale(cx, None);
-                // `custom_shells` is only ever hand-edited, so this file is the
-                // one place it can change from — and the inventory that carries
-                // it to the new-tab menu is cached per window.
-                crate::ui::windows::WindowRegistry::refresh_shells(cx);
-                // A hand-edited keybinding shows up in the settings list off the
-                // live global immediately; without this it never reaches the
-                // keymap gpui actually dispatches against, so the key looks
-                // bound and does nothing until restart. Gated on the triple so
-                // an unrelated save does not churn it.
-                if crate::ui::keymap::keybinding_config(cx) != keymap_before {
-                    crate::ui::keymap::rebind(cx);
-                }
-                cx.refresh_windows();
+                apply_reloaded_config(cx, Config::load_with_outcome(), &mut announced);
             });
         }
     })
     .detach();
 }
 
+/// One watcher tick, once the debounce is out: what a reload does to the
+/// running app.
+///
+/// `announced` is the one-toast-per-breakage latch, which lives across ticks
+/// in the watcher. Returns whether the keymap was rebuilt — the watcher has no
+/// use for that; it is how a test asks which branch ran.
+fn apply_reloaded_config(
+    cx: &mut App,
+    load: (Config, crate::core::config::LoadOutcome),
+    announced: &mut bool,
+) -> bool {
+    let (config, outcome) = load;
+    if outcome.failed() {
+        // Keep the settings the app is running on: swapping the stand-in
+        // defaults in would flash the whole UI onto defaults, and the load
+        // already parked the broken file beside the original. It reloads
+        // itself the moment the file parses again.
+        if !*announced {
+            *announced = true;
+            notify_config_load_failed(cx, outcome, false);
+        }
+        // The theme files this same watcher covers must keep hot-reloading: a
+        // typo in config.json is no reason for theme editing to go dead until
+        // the app restarts. They read the global config, which is deliberately
+        // still the one the app is running on.
+        reload_themes(cx);
+        cx.refresh_windows();
+        return false;
+    }
+    *announced = false;
+    // The keymap is rebuilt only when a binding actually moved. This watcher
+    // fires for every write under the config dir — including the app's own
+    // `save()`, which a sidebar drag or a palette open triggers — so reloading
+    // bindings on each tick would rebuild the whole keymap for nothing, and
+    // (before `rebind` cleared first) leak a full table per tick (#548).
+    // Read past the early return above: a load that failed leaves the global
+    // alone, so there is nothing to compare and the user's keys stay in the
+    // keymap the app is dispatching on.
+    let keymap_before = crate::ui::keymap::keybinding_config(cx);
+    crate::ui::i18n::set_locale(&config.gui_language);
+    cx.set_global(config);
+    reload_themes(cx);
+    crate::ui::theme::apply_cursor_hide_mode(cx);
+    // The menu bar is built once from the current locale, so editing
+    // gui_language by hand has to rebuild it the same way the in-app language
+    // picker does.
+    crate::ui::theme::set_menus(cx);
+    crate::ui::windows::WindowRegistry::refresh_locale(cx, None);
+    // `custom_shells` is only ever hand-edited, so this file is the one place
+    // it can change from — and the inventory that carries it to the new-tab
+    // menu is cached per window.
+    crate::ui::windows::WindowRegistry::refresh_shells(cx);
+    // A hand-edited keybinding shows up in the settings list off the live
+    // global immediately; without this it never reaches the keymap gpui
+    // actually dispatches against, so the key looks bound and does nothing
+    // until restart. Gated on the triple so an unrelated save does not churn
+    // it.
+    let rebound = crate::ui::keymap::keybinding_config(cx) != keymap_before;
+    if rebound {
+        crate::ui::keymap::rebind(cx);
+    }
+    cx.refresh_windows();
+    rebound
+}
+
 fn is_theme_file(p: &std::path::Path) -> bool {
     p.parent().and_then(|d| d.file_name()) == Some(std::ffi::OsStr::new("themes"))
         && p.extension().and_then(|e| e.to_str()).is_some_and(|e| {
@@ -556,6 +569,176 @@ fn main() {
         });
 }
 
+/// The watcher tick, from a reloaded file to the keys the app dispatches on.
+///
+/// The one thing #548 is for — hand-editing config.json and having the new
+/// chord fire without a restart — crosses a file watcher, a debounce and a
+/// keymap rebuild, and used to be covered nowhere. These drive
+/// `apply_reloaded_config` directly, which is the whole body of the watcher's
+/// tick, so the reload path is exercised without waiting on the filesystem.
+#[cfg(test)]
+mod config_reload_tests {
+    use super::{apply_reloaded_config, is_theme_file};
+    use crate::core::actions::SplitRight;
+    use crate::core::config::{Config, LoadOutcome};
+    use gpui::{Action as _, App, KeyContext, Keystroke, TestAppContext};
+
+    /// What the live keymap — the one gpui dispatches against — does with
+    /// `keys` typed in a terminal.
+    fn dispatched(cx: &App, keys: &str) -> Vec<&'static str> {
+        let typed = [Keystroke::parse(keys).expect("the typed keystroke parses")];
+        let context = [KeyContext::parse("Terminal").expect("the context parses")];
+        cx.key_bindings()
+            .borrow()
+            .bindings_for_input(&typed, &context)
+            .0
+            .iter()
+            .map(|b| b.action().name())
+            .collect()
+    }
+
+    /// An app running on `config`, as far as the config watcher can tell:
+    /// the globals its tick reads, and a keymap built from that config.
+    fn running_on(cx: &mut App, config: Config) {
+        let dir = std::env::temp_dir().join(format!("tty7-reload-{}", std::process::id()));
+        std::fs::create_dir_all(&dir).ok();
+        crate::core::config::set_config_dir(dir);
+
+        gpui_component::init(cx);
+        cx.set_global(config);
+        crate::ui::windows::WindowRegistry::init(cx);
+        crate::ui::presets::load_registry(cx);
+        crate::ui::keymap::init(cx);
+    }
+
+    fn bound_to_split_right(config: &mut Config, key: &str) {
+        config
+            .keybindings
+            .insert("SplitRight".to_string(), key.to_string());
+    }
+
+    #[gpui::test]
+    fn a_hand_edited_binding_fires_without_a_restart(cx: &mut TestAppContext) {
+        cx.update(|cx| {
+            running_on(cx, Config::default());
+            assert!(
+                dispatched(cx, "ctrl-alt-9").is_empty(),
+                "nothing is on the chord before the edit"
+            );
+
+            let mut edited = Config::default();
+            bound_to_split_right(&mut edited, "ctrl-alt-9");
+            let mut announced = false;
+            assert!(
+                apply_reloaded_config(cx, (edited, LoadOutcome::Parsed), &mut announced),
+                "a moved binding rebuilds the keymap"
+            );
+
+            assert_eq!(
+                dispatched(cx, "ctrl-alt-9"),
+                vec![SplitRight::name_for_type()],
+                "the hand-edited chord reaches the keymap gpui dispatches on"
+            );
+        });
+    }
+
+    #[gpui::test]
+    fn a_save_that_moves_no_binding_leaves_the_keymap_alone(cx: &mut TestAppContext) {
+        cx.update(|cx| {
+            running_on(cx, Config::default());
+
+            // The app's own `save()` fires this watcher on every sidebar drag.
+            let mut unrelated = Config::default();
+            unrelated.dim_inactive_panes = !unrelated.dim_inactive_panes;
+            let mut announced = false;
+            assert!(
+                !apply_reloaded_config(
+                    cx,
+                    (unrelated.clone(), LoadOutcome::Parsed),
+                    &mut announced
+                ),
+                "an unrelated save must not rebuild the keymap"
+            );
+            assert_eq!(
+                cx.global::<Config>().dim_inactive_panes,
+                unrelated.dim_inactive_panes,
+                "the reload still took effect; only the rebind was skipped"
+            );
+        });
+    }
+
+    #[gpui::test]
+    fn a_broken_config_does_not_take_the_users_keys_away(cx: &mut TestAppContext) {
+        cx.update(|cx| {
+            let mut user = Config::default();
+            bound_to_split_right(&mut user, "ctrl-alt-9");
+            running_on(cx, user);
+            assert_eq!(
+                dispatched(cx, "ctrl-alt-9"),
+                vec![SplitRight::name_for_type()]
+            );
+
+            // A quarantined load hands back stand-in defaults, and the global
+            // is deliberately left alone — so nothing may rebind off them.
+            let mut announced = false;
+            assert!(
+                !apply_reloaded_config(
+                    cx,
+                    (Config::default(), LoadOutcome::Quarantined),
+                    &mut announced
+                ),
+                "a load that failed has nothing to compare and must not rebind"
+            );
+            assert!(announced, "the breakage is announced");
+            assert_eq!(
+                cx.global::<Config>()
+                    .keybindings
+                    .get("SplitRight")
+                    .map(String::as_str),
+                Some("ctrl-alt-9"),
+                "the running config survives the broken file"
+            );
+            assert_eq!(
+                dispatched(cx, "ctrl-alt-9"),
+                vec![SplitRight::name_for_type()],
+                "and so do the keys the app is dispatching on"
+            );
+
+            // The file parses again: the latch clears, so a second breakage
+            // can speak up.
+            let mut fixed = Config::default();
+            bound_to_split_right(&mut fixed, "ctrl-alt-8");
+            assert!(apply_reloaded_config(
+                cx,
+                (fixed, LoadOutcome::Parsed),
+                &mut announced
+            ));
+            assert!(!announced);
+            assert_eq!(
+                dispatched(cx, "ctrl-alt-8"),
+                vec![SplitRight::name_for_type()]
+            );
+            assert!(
+                dispatched(cx, "ctrl-alt-9").is_empty(),
+                "the chord the fixed file dropped is retired"
+            );
+        });
+    }
+
+    /// The watcher covers the whole config directory, and the themes half of
+    /// it has to keep reloading even when config.json does not parse.
+    #[test]
+    fn only_theme_files_beside_the_config_count_as_themes() {
+        use std::path::Path;
+        assert!(is_theme_file(Path::new("/cfg/themes/solar.yaml")));
+        assert!(is_theme_file(Path::new("/cfg/themes/solar.YML")));
+        assert!(is_theme_file(Path::new("/cfg/themes/solar.itermcolors")));
+        assert!(!is_theme_file(Path::new("/cfg/themes/notes.txt")));
+        assert!(!is_theme_file(Path::new("/cfg/config.json")));
+        assert!(!is_theme_file(Path::new("/cfg/solar.yaml")));
+    }
+}
+
 #[cfg(all(test, unix))]
 mod tests {
     use super::merge_paths;

src/ui/keymap.rs

@@ -183,22 +183,6 @@ fn action_bindings(effective: &[(String, String)]) -> Vec<KeyBinding> {
     bindings
 }
 
-/// The keystroke and context of every binding `action_bindings` installs.
-/// The rebind used to keep this in a global to retire the previous set one
-/// `NoAction` at a time; a rebuild replaces the whole map instead, so this is
-/// now only the tests' way of asking what a config would install.
-#[cfg(test)]
-fn bound_keystrokes(effective: &[(String, String)]) -> Vec<(String, Option<&'static str>)> {
-    let extras = extra_keystrokes(effective);
-    effective
-        .iter()
-        .map(|(a, k)| (a.as_str(), k.as_str()))
-        .chain(extras.iter().map(|(a, k)| (*a, *k)))
-        .filter(|(_, k)| !k.is_empty() && keystroke_is_valid(k))
-        .map(|(a, k)| (k.to_string(), action_context(a)))
-        .collect()
-}
-
 fn per_platform(mac: &'static str, other: &'static str) -> &'static str {
     if cfg!(target_os = "macos") {
         mac
@@ -1057,6 +1041,30 @@ fn make_binding(action: &str, keystroke: &str) -> Option<KeyBinding> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use gpui::Action as _;
+
+    /// The actions a keymap built from `action_bindings` dispatches for `keys`
+    /// typed in `context`, in precedence order — the same lookup gpui performs
+    /// on a real keypress.
+    ///
+    /// Bindings are asserted through this rather than through a mirror of the
+    /// table, so a chord the app would not really install, or would install in
+    /// another context, cannot pass.
+    fn dispatched(effective: &[(String, String)], keys: &str, context: &str) -> Vec<&'static str> {
+        let mut keymap = gpui::Keymap::default();
+        keymap.add_bindings(action_bindings(effective));
+        let input: Vec<Keystroke> = keys
+            .split(' ')
+            .map(|k| Keystroke::parse(k).expect("the typed keystroke parses"))
+            .collect();
+        let context = [gpui::KeyContext::parse(context).expect("the context parses")];
+        keymap
+            .bindings_for_input(&input, &context)
+            .0
+            .iter()
+            .map(|b| b.action().name())
+            .collect()
+    }
 
     #[test]
     fn every_dispatchable_action_has_a_slot_to_bind_it_in() {
@@ -1226,10 +1234,9 @@ mod tests {
         assert!(extra_keystrokes(&effective).contains(&("InsertNewline", "alt-enter")));
         for key in ["shift-enter", "alt-enter"] {
             assert!(keystroke_is_valid(key), "{key} does not parse");
-            assert!(make_binding("InsertNewline", key).is_some());
             assert!(
-                bound_keystrokes(&effective).iter().any(|(k, _)| k == key),
-                "{key} is not remembered as installed"
+                dispatched(&effective, key, "Terminal").contains(&InsertNewline::name_for_type()),
+                "{key} does not reach InsertNewline in a terminal"
             );
         }
         assert_eq!(key_tokens("shift-enter"), vec![SHIFT, "⏎"]);
@@ -1313,10 +1320,12 @@ mod tests {
         assert!(extra_keystrokes(&effective).contains(&("PasteText", "shift-insert")));
         for key in ["ctrl-shift-v", "shift-insert"] {
             assert!(
-                bound_keystrokes(&effective)
-                    .iter()
-                    .any(|(k, c)| k == key && *c == Some("Terminal")),
-                "{key} must be installed in the Terminal context and remembered for rebind"
+                dispatched(&effective, key, "Terminal").contains(&PasteText::name_for_type()),
+                "{key} must paste in a terminal"
+            );
+            assert!(
+                !dispatched(&effective, key, "Workspace").contains(&PasteText::name_for_type()),
+                "{key} is a terminal chord and must not paste outside one"
             );
         }
         let rebound = vec![("PasteText".to_string(), "ctrl-alt-v".to_string())];
@@ -1333,9 +1342,16 @@ mod tests {
         let effective = vec![("InsertNewline".to_string(), "ctrl-o".to_string())];
         assert!(extra_keystrokes(&effective).is_empty());
         assert_eq!(
-            bound_keystrokes(&effective),
-            vec![("ctrl-o".to_string(), Some("Terminal"))]
+            dispatched(&effective, "ctrl-o", "Terminal"),
+            vec![InsertNewline::name_for_type()],
+            "the chord the config asks for is the one the keymap dispatches"
         );
+        for retired in ["shift-enter", "alt-enter"] {
+            assert!(
+                dispatched(&effective, retired, "Terminal").is_empty(),
+                "{retired} is off the action now and must dispatch nothing"
+            );
+        }
 
         let unbound = vec![("InsertNewline".to_string(), String::new())];
         assert!(extra_keystrokes(&unbound).is_empty());
@@ -1343,19 +1359,36 @@ mod tests {
     }
 
     #[test]
-    fn bound_keystrokes_remember_the_context_each_binding_was_installed_in() {
+    fn each_binding_lands_in_the_context_its_action_is_scoped_to() {
         let effective = vec![
             ("InsertNewline".to_string(), "shift-enter".to_string()),
             ("NewTab".to_string(), "secondary-t".to_string()),
         ];
+        // The newline is a terminal chord, and its default ships the fallback
+        // beside it; both are scoped, so neither reaches the rest of the app.
+        let mut newline = dispatched(&effective, "shift-enter", "Terminal");
+        newline.sort_unstable();
         assert_eq!(
-            bound_keystrokes(&effective),
+            newline,
             vec![
-                ("shift-enter".to_string(), Some("Terminal")),
-                ("secondary-t".to_string(), None),
-                ("alt-enter".to_string(), Some("Terminal")),
+                InsertNewline::name_for_type(),
+                InsertNewlineFallback::name_for_type(),
             ]
         );
+        assert!(dispatched(&effective, "shift-enter", "Workspace").is_empty());
+        assert_eq!(
+            dispatched(&effective, "alt-enter", "Terminal"),
+            vec![InsertNewline::name_for_type()],
+            "the extra chord follows its action's scope"
+        );
+        // A window action has no context: it has to work from a terminal too.
+        for context in ["Terminal", "Workspace"] {
+            assert_eq!(
+                dispatched(&effective, "secondary-t", context),
+                vec![NewTab::name_for_type()],
+                "NewTab must not be scoped to {context}"
+            );
+        }
     }
 
     #[test]