keymap_editor: Fix default binding deletion and override (#50699)

iam-liam and Ben Kunkle created

## Summary

Fixes #48725

When using the keymap editor to delete or re-bind default keybindings,
two issues caused stale entries to persist:

- **Deleting a default binding** created a `NoAction` suppression entry
in `keymap.json` (correct for GPUI-level suppression), but the keymap
editor displayed it as a phantom `<null>` row alongside the greyed-out
default. Searching by keystroke showed both entries, and clash detection
counted the phantom as a real binding.

- **Changing a default binding's keystroke** (e.g. `โŒฅโŒ˜T` โ†’ `โ‡งโŒƒโŒ˜T`) added
the new binding but never suppressed the original default under the old
keystroke. The old default continued appearing in keystroke searches and
clash warnings.

## Fix

1. **Filter `NoAction` suppression bindings from the keymap editor
display.** These remain in the internal binding list for conflict
detection (so overridden defaults are still correctly greyed out), but
are excluded from the visible match results.

2. **Append a `null` suppression for the old keystroke when replacing a
non-user binding with a different keystroke.** When `update_keybinding`
converts a `Replace` to `Add` for a non-user binding and the keystroke
changes, it now also writes `{"old-keystroke": null}` (with the original
context) to suppress the stale default.

## Reproduction steps (verified fixed)

1. Open the keymap editor
2. Click Search by Keystroke and press `โŒฅโŒ˜T`
3. You should see "agent: new thread" and "pane: close other items"
(both Default)
4. Right-click "agent: new thread" and choose Delete
5. Double-click "pane: close other items", change keystroke to `โ‡งโŒƒโŒ˜T`,
click Save
6. Verify no `<null>` phantom row appears, and no stale defaults remain
under `โŒฅโŒ˜T`
7. Close and reopen the keymap editor, search `โŒฅโŒ˜T` โ€” no results
8. Search for "editor: wrap selections in tag" and assign `โŒฅโŒ˜T` โ€” no
clash warnings

## Test plan

- [x] Existing `keymap_update` and `test_keymap_remove` tests pass
- [x] Added test: replacing a non-user binding **without** changing the
keystroke produces no suppression
- [x] Added test: replacing a non-user binding **with** context and
keystroke change produces a suppression that preserves the context
- [x] Manual verification of all reproduction steps above

Release Notes:

- Fixed an issue with the keymap editor where where the defaults would not be completely removed when deleting or overriding default keybindings

---------

Co-authored-by: Ben Kunkle <ben@zed.dev>

Change summary

crates/keymap_editor/src/keymap_editor.rs | 347 ++++++++++++++++--------
crates/settings/src/keymap_file.rs        | 128 +++++++++
2 files changed, 354 insertions(+), 121 deletions(-)

Detailed changes

crates/keymap_editor/src/keymap_editor.rs ๐Ÿ”—

@@ -30,8 +30,8 @@ use settings::{
     BaseKeymap, KeybindSource, KeymapFile, Settings as _, SettingsAssets, infer_json_indent_size,
 };
 use ui::{
-    ActiveTheme as _, App, Banner, BorrowAppContext, ContextMenu, IconButtonShape, Indicator,
-    Modal, ModalFooter, ModalHeader, ParentElement as _, PopoverMenu, Render, Section,
+    ActiveTheme as _, App, Banner, BorrowAppContext, ContextMenu, IconButtonShape, IconPosition,
+    Indicator, Modal, ModalFooter, ModalHeader, ParentElement as _, PopoverMenu, Render, Section,
     SharedString, Styled as _, Table, TableColumnWidths, TableInteractionState,
     TableResizeBehavior, Tooltip, Window, prelude::*,
 };
@@ -73,6 +73,8 @@ actions!(
         CopyContext,
         /// Toggles Conflict Filtering
         ToggleConflictFilter,
+        /// Toggles whether NoAction bindings are shown
+        ToggleNoActionBindings,
         /// Toggle Keystroke search
         ToggleKeystrokeSearch,
         /// Toggles exact matching for keystroke search
@@ -183,7 +185,7 @@ impl KeymapEventChannel {
     }
 }
 
-#[derive(Default, PartialEq)]
+#[derive(Default, PartialEq, Copy, Clone)]
 enum SearchMode {
     #[default]
     Normal,
@@ -224,6 +226,25 @@ impl FilterState {
     }
 }
 
+#[derive(Default, PartialEq, Eq, Copy, Clone)]
+struct SourceFilters {
+    user: bool,
+    zed_defaults: bool,
+    vim_defaults: bool,
+}
+
+impl SourceFilters {
+    fn allows(&self, source: Option<KeybindSource>) -> bool {
+        match source {
+            Some(KeybindSource::User) => self.user,
+            Some(KeybindSource::Vim) => self.vim_defaults,
+            Some(KeybindSource::Base | KeybindSource::Default | KeybindSource::Unknown) | None => {
+                self.zed_defaults
+            }
+        }
+    }
+}
+
 #[derive(Debug, Default, PartialEq, Eq, Clone, Hash)]
 struct ActionMapping {
     keystrokes: Rc<[KeybindingKeystroke]>,
@@ -412,6 +433,8 @@ struct KeymapEditor {
     keybindings: Vec<ProcessedBinding>,
     keybinding_conflict_state: ConflictState,
     filter_state: FilterState,
+    source_filters: SourceFilters,
+    show_no_action_bindings: bool,
     search_mode: SearchMode,
     search_query_debounce: Option<Task<()>>,
     // corresponds 1 to 1 with keybindings
@@ -539,6 +562,12 @@ impl KeymapEditor {
             keybindings: vec![],
             keybinding_conflict_state: ConflictState::default(),
             filter_state: FilterState::default(),
+            source_filters: SourceFilters {
+                user: true,
+                zed_defaults: true,
+                vim_defaults: true,
+            },
+            show_no_action_bindings: true,
             search_mode: SearchMode::default(),
             string_match_candidates: Arc::new(vec![]),
             matches: vec![],
@@ -637,6 +666,11 @@ impl KeymapEditor {
         )
         .await;
         this.update(cx, |this, cx| {
+            matches.retain(|candidate| {
+                this.source_filters
+                    .allows(this.keybindings[candidate.candidate_id].keybind_source())
+            });
+
             match this.filter_state {
                 FilterState::Conflicts => {
                     matches.retain(|candidate| {
@@ -695,6 +729,15 @@ impl KeymapEditor {
                 SearchMode::Normal => {}
             }
 
+            // Filter out NoAction suppression bindings by default. These are internal
+            // markers created when a user deletes a default binding (to suppress the
+            // default at the GPUI level), not real bindings the user should usually see.
+            if !this.show_no_action_bindings {
+                matches.retain(|item| {
+                    this.keybindings[item.candidate_id].action().name != gpui::NoAction.name()
+                });
+            }
+
             if action_query.is_empty() {
                 matches.sort_by(|item1, item2| {
                     let binding1 = &this.keybindings[item1.candidate_id];
@@ -1367,6 +1410,31 @@ impl KeymapEditor {
         self.set_filter_state(self.filter_state.invert(), cx);
     }
 
+    fn toggle_no_action_bindings(
+        &mut self,
+        _: &ToggleNoActionBindings,
+        _: &mut Window,
+        cx: &mut Context<Self>,
+    ) {
+        self.show_no_action_bindings = !self.show_no_action_bindings;
+        self.on_query_changed(cx);
+    }
+
+    fn toggle_user_bindings_filter(&mut self, cx: &mut Context<Self>) {
+        self.source_filters.user = !self.source_filters.user;
+        self.on_query_changed(cx);
+    }
+
+    fn toggle_zed_defaults_filter(&mut self, cx: &mut Context<Self>) {
+        self.source_filters.zed_defaults = !self.source_filters.zed_defaults;
+        self.on_query_changed(cx);
+    }
+
+    fn toggle_vim_defaults_filter(&mut self, cx: &mut Context<Self>) {
+        self.source_filters.vim_defaults = !self.source_filters.vim_defaults;
+        self.on_query_changed(cx);
+    }
+
     fn set_filter_state(&mut self, filter_state: FilterState, cx: &mut Context<Self>) {
         if self.filter_state != filter_state {
             self.filter_state = filter_state;
@@ -1442,6 +1510,127 @@ impl KeymapEditor {
             .filter(|kb| kb.keystrokes().is_some())
             .any(|kb| kb.action().name == action_name)
     }
+
+    fn render_filter_dropdown(
+        &self,
+        focus_handle: &FocusHandle,
+        cx: &mut Context<KeymapEditor>,
+    ) -> impl IntoElement {
+        let focus_handle = focus_handle.clone();
+        let keymap_editor = cx.entity();
+        return PopoverMenu::new("keymap-editor-filter-menu")
+            .menu(move |window, cx| {
+                Some(ContextMenu::build_persistent(window, cx, {
+                    let focus_handle = focus_handle.clone();
+                    let keymap_editor = keymap_editor.clone();
+                    move |mut menu, _window, cx| {
+                        let (filter_state, source_filters, show_no_action_bindings) = keymap_editor
+                            .read_with(cx, |editor, _| {
+                                (
+                                    editor.filter_state,
+                                    editor.source_filters,
+                                    editor.show_no_action_bindings,
+                                )
+                            });
+
+                        menu = menu
+                            .context(focus_handle.clone())
+                            .header("Filters")
+                            .map(add_filter(
+                                "Conflicts",
+                                matches!(filter_state, FilterState::Conflicts),
+                                Some(ToggleConflictFilter.boxed_clone()),
+                                &focus_handle,
+                                &keymap_editor,
+                                None,
+                            ))
+                            .map(add_filter(
+                                "No Action",
+                                show_no_action_bindings,
+                                Some(ToggleNoActionBindings.boxed_clone()),
+                                &focus_handle,
+                                &keymap_editor,
+                                None,
+                            ))
+                            .separator()
+                            .header("Categories")
+                            .map(add_filter(
+                                "User",
+                                source_filters.user,
+                                None,
+                                &focus_handle,
+                                &keymap_editor,
+                                Some(|editor, cx| {
+                                    editor.toggle_user_bindings_filter(cx);
+                                }),
+                            ))
+                            .map(add_filter(
+                                "Default",
+                                source_filters.zed_defaults,
+                                None,
+                                &focus_handle,
+                                &keymap_editor,
+                                Some(|editor, cx| {
+                                    editor.toggle_zed_defaults_filter(cx);
+                                }),
+                            ))
+                            .map(add_filter(
+                                "Vim",
+                                source_filters.vim_defaults,
+                                None,
+                                &focus_handle,
+                                &keymap_editor,
+                                Some(|editor, cx| {
+                                    editor.toggle_vim_defaults_filter(cx);
+                                }),
+                            ));
+                        menu
+                    }
+                }))
+            })
+            .anchor(gpui::Corner::TopRight)
+            .offset(gpui::Point {
+                x: px(0.0),
+                y: px(2.0),
+            })
+            .trigger_with_tooltip(
+                IconButton::new("KeymapEditorFilterMenuButton", IconName::Sliders)
+                    .icon_size(IconSize::Small)
+                    .when(
+                        self.keybinding_conflict_state.any_user_binding_conflicts(),
+                        |this| this.indicator(Indicator::dot().color(Color::Warning)),
+                    ),
+                Tooltip::text("Filters"),
+            );
+
+        fn add_filter(
+            name: &'static str,
+            toggled: bool,
+            action: Option<Box<dyn Action>>,
+            focus_handle: &FocusHandle,
+            keymap_editor: &Entity<KeymapEditor>,
+            cb: Option<fn(&mut KeymapEditor, &mut Context<KeymapEditor>)>,
+        ) -> impl FnOnce(ContextMenu) -> ContextMenu {
+            let focus_handle = focus_handle.clone();
+            let keymap_editor = keymap_editor.clone();
+            return move |menu: ContextMenu| {
+                menu.toggleable_entry(
+                    name,
+                    toggled,
+                    IconPosition::End,
+                    action.as_ref().map(|a| a.boxed_clone()),
+                    move |window, cx| {
+                        window.focus(&focus_handle, cx);
+                        if let Some(action) = &action {
+                            window.dispatch_action(action.boxed_clone(), cx);
+                        } else if let Some(cb) = cb {
+                            keymap_editor.update(cx, cb);
+                        }
+                    },
+                )
+            };
+        }
+    }
 }
 
 struct HumanizedActionNameCache {
@@ -1694,6 +1883,7 @@ impl Render for KeymapEditor {
         let row_count = self.matches.len();
         let focus_handle = &self.focus_handle;
         let theme = cx.theme();
+        let search_mode = self.search_mode;
 
         v_flex()
             .id("keymap-editor")
@@ -1711,6 +1901,7 @@ impl Render for KeymapEditor {
             .on_action(cx.listener(Self::copy_action_to_clipboard))
             .on_action(cx.listener(Self::copy_context_to_clipboard))
             .on_action(cx.listener(Self::toggle_conflict_filter))
+            .on_action(cx.listener(Self::toggle_no_action_bindings))
             .on_action(cx.listener(Self::toggle_keystroke_search))
             .on_action(cx.listener(Self::toggle_exact_keystroke_matching))
             .on_action(cx.listener(Self::show_matching_keystrokes))
@@ -1727,6 +1918,7 @@ impl Render for KeymapEditor {
                     .child(
                         h_flex()
                             .gap_2()
+                            .items_center()
                             .child(
                                 h_flex()
                                     .key_context({
@@ -1748,152 +1940,65 @@ impl Render for KeymapEditor {
                                 h_flex()
                                     .gap_1()
                                     .min_w_96()
+                                    .items_center()
                                     .child(
                                         IconButton::new(
-                                            "KeymapEditorToggleFiltersIcon",
+                                            "KeymapEditorKeystrokeSearchButton",
                                             IconName::Keyboard,
                                         )
                                         .icon_size(IconSize::Small)
+                                        .toggle_state(matches!(
+                                            search_mode,
+                                            SearchMode::KeyStroke { .. }
+                                        ))
                                         .tooltip({
                                             let focus_handle = focus_handle.clone();
-
                                             move |_window, cx| {
                                                 Tooltip::for_action_in(
-                                                    "Search by Keystroke",
+                                                    "Search by Keystrokes",
                                                     &ToggleKeystrokeSearch,
-                                                    &focus_handle.clone(),
+                                                    &focus_handle,
                                                     cx,
                                                 )
                                             }
                                         })
-                                        .toggle_state(matches!(
-                                            self.search_mode,
-                                            SearchMode::KeyStroke { .. }
-                                        ))
-                                        .on_click(|_, window, cx| {
+                                        .on_click(cx.listener(|_, _, window, cx| {
                                             window.dispatch_action(
                                                 ToggleKeystrokeSearch.boxed_clone(),
                                                 cx,
                                             );
-                                        }),
+                                        })),
                                     )
                                     .child(
-                                        IconButton::new("KeymapEditorConflictIcon", IconName::Warning)
-                                            .icon_size(IconSize::Small)
-                                            .when(
-                                                self.keybinding_conflict_state
-                                                    .any_user_binding_conflicts(),
-                                                |this| {
-                                                    this.indicator(
-                                                        Indicator::dot().color(Color::Warning),
-                                                    )
-                                                },
+                                        self.render_filter_dropdown(focus_handle, cx)
+                                    )
+                                    .child(
+                                        Button::new("edit-in-json", "Edit in JSON")
+                                            .style(ButtonStyle::Subtle)
+                                            .key_binding(
+                                                ui::KeyBinding::for_action_in(&zed_actions::OpenKeymapFile, &focus_handle, cx)
+                                                    .map(|kb| kb.size(rems_from_px(10.))),
                                             )
-                                            .tooltip({
-                                                let filter_state = self.filter_state;
-                                                let focus_handle = focus_handle.clone();
-
-                                                move |_window, cx| {
-                                                    Tooltip::for_action_in(
-                                                        match filter_state {
-                                                            FilterState::All => "Show Conflicts",
-                                                            FilterState::Conflicts => {
-                                                                "Hide Conflicts"
-                                                            }
-                                                        },
-                                                        &ToggleConflictFilter,
-                                                        &focus_handle.clone(),
-                                                        cx,
-                                                    )
-                                                }
-                                            })
-                                            .selected_icon_color(Color::Warning)
-                                            .toggle_state(matches!(
-                                                self.filter_state,
-                                                FilterState::Conflicts
-                                            ))
                                             .on_click(|_, window, cx| {
                                                 window.dispatch_action(
-                                                    ToggleConflictFilter.boxed_clone(),
+                                                    zed_actions::OpenKeymapFile.boxed_clone(),
                                                     cx,
                                                 );
-                                            }),
+                                            })
                                     )
                                     .child(
-                                        h_flex()
-                                            .w_full()
-                                            .px_1p5()
-                                            .gap_1()
-                                            .justify_end()
-                                            .child(
-                                                PopoverMenu::new("open-keymap-menu")
-                                                    .menu(move |window, cx| {
-                                                        Some(ContextMenu::build(window, cx, |menu, _, _| {
-                                                            menu.header("View Default...")
-                                                                .action(
-                                                                    "Zed Key Bindings",
-                                                                    zed_actions::OpenDefaultKeymap
-                                                                        .boxed_clone(),
-                                                                )
-                                                                .action(
-                                                                    "Vim Bindings",
-                                                                    zed_actions::vim::OpenDefaultKeymap.boxed_clone(),
-                                                                )
-                                                        }))
-                                                    })
-                                                    .anchor(gpui::Corner::TopRight)
-                                                    .offset(gpui::Point {
-                                                        x: px(0.0),
-                                                        y: px(2.0),
-                                                    })
-                                                    .trigger_with_tooltip(
-                                                        IconButton::new(
-                                                            "OpenKeymapJsonButton",
-                                                            IconName::Ellipsis,
-                                                        )
-                                                        .icon_size(IconSize::Small),
-                                                        {
-                                                            let focus_handle = focus_handle.clone();
-                                                            move |_window, cx| {
-                                                                Tooltip::for_action_in(
-                                                                    "View Default...",
-                                                                    &zed_actions::OpenKeymapFile,
-                                                                    &focus_handle,
-                                                                    cx,
-                                                                )
-                                                            }
-                                                        },
-                                                    ),
-                                            )
-                                            .child(
-                                                Button::new("edit-in-json", "Edit in JSON")
-                                                    .style(ButtonStyle::Subtle)
-                                                    .key_binding(
-                                                        ui::KeyBinding::for_action_in(&zed_actions::OpenKeymapFile, &focus_handle, cx)
-                                                            .map(|kb| kb.size(rems_from_px(10.))),
-                                                    )
-                                                    .on_click(|_, window, cx| {
-                                                        window.dispatch_action(
-                                                            zed_actions::OpenKeymapFile.boxed_clone(),
-                                                            cx,
-                                                        );
-                                                    })
+                                        Button::new("create", "Create Keybinding")
+                                            .style(ButtonStyle::Outlined)
+                                            .key_binding(
+                                                ui::KeyBinding::for_action_in(&OpenCreateKeybindingModal, &focus_handle, cx)
+                                                    .map(|kb| kb.size(rems_from_px(10.))),
                                             )
-                                            .child(
-                                                Button::new("create", "Create Keybinding")
-                                                    .style(ButtonStyle::Outlined)
-                                                    .key_binding(
-                                                        ui::KeyBinding::for_action_in(&OpenCreateKeybindingModal, &focus_handle, cx)
-                                                            .map(|kb| kb.size(rems_from_px(10.))),
-                                                    )
-                                                    .on_click(|_, window, cx| {
-                                                        window.dispatch_action(
-                                                            OpenCreateKeybindingModal.boxed_clone(),
-                                                            cx,
-                                                        );
-                                                    })
-                                            )
-
+                                            .on_click(|_, window, cx| {
+                                                window.dispatch_action(
+                                                    OpenCreateKeybindingModal.boxed_clone(),
+                                                    cx,
+                                                );
+                                            })
                                     )
                             ),
                     )

crates/settings/src/keymap_file.rs ๐Ÿ”—

@@ -701,6 +701,10 @@ impl KeymapFile {
         tab_size: usize,
         keyboard_mapper: &dyn gpui::PlatformKeyboardMapper,
     ) -> Result<String> {
+        // When replacing a non-user binding's keystroke, we need to also suppress the old
+        // default so it doesn't continue showing under the old keystroke.
+        let mut old_keystroke_suppression: Option<(Option<String>, String)> = None;
+
         match operation {
             // if trying to replace a keybinding that is not user-defined, treat it as an add operation
             KeybindUpdateOperation::Replace {
@@ -708,6 +712,12 @@ impl KeymapFile {
                 source,
                 target,
             } if target_source != KeybindSource::User => {
+                if target.keystrokes_unparsed() != source.keystrokes_unparsed() {
+                    old_keystroke_suppression = Some((
+                        target.context.map(String::from),
+                        target.keystrokes_unparsed(),
+                    ));
+                }
                 operation = KeybindUpdateOperation::Add {
                     source,
                     from: Some(target),
@@ -886,6 +896,28 @@ impl KeymapFile {
             );
             keymap_contents.replace_range(replace_range, &replace_value);
         }
+
+        // If we converted a Replace to Add because the target was a non-user binding,
+        // and the keystroke changed, suppress the old default keystroke with a NoAction
+        // binding so it doesn't continue appearing under the old keystroke.
+        if let Some((context, old_keystrokes)) = old_keystroke_suppression {
+            let mut value = serde_json::Map::with_capacity(2);
+            if let Some(context) = context {
+                value.insert("context".to_string(), context.into());
+            }
+            value.insert("bindings".to_string(), {
+                let mut bindings = serde_json::Map::new();
+                bindings.insert(old_keystrokes, Value::Null);
+                bindings.into()
+            });
+            let (replace_range, replace_value) = append_top_level_array_value_in_json_text(
+                &keymap_contents,
+                &value.into(),
+                tab_size,
+            );
+            keymap_contents.replace_range(replace_range, &replace_value);
+        }
+
         return Ok(keymap_contents);
 
         fn find_binding<'a, 'b>(
@@ -1479,6 +1511,102 @@ mod tests {
                             }
                         ]
                     }
+                },
+                {
+                    "bindings": {
+                        "ctrl-a": null
+                    }
+                }
+            ]"#
+            .unindent(),
+        );
+
+        // Replacing a non-user binding without changing the keystroke should
+        // not produce a NoAction suppression entry.
+        check_keymap_update(
+            r#"[
+                {
+                    "bindings": {
+                        "ctrl-a": "zed::SomeAction"
+                    }
+                }
+            ]"#
+            .unindent(),
+            KeybindUpdateOperation::Replace {
+                target: KeybindUpdateTarget {
+                    keystrokes: &parse_keystrokes("ctrl-a"),
+                    action_name: "zed::SomeAction",
+                    context: None,
+                    action_arguments: None,
+                },
+                source: KeybindUpdateTarget {
+                    keystrokes: &parse_keystrokes("ctrl-a"),
+                    action_name: "zed::SomeOtherAction",
+                    context: None,
+                    action_arguments: None,
+                },
+                target_keybind_source: KeybindSource::Base,
+            },
+            r#"[
+                {
+                    "bindings": {
+                        "ctrl-a": "zed::SomeAction"
+                    }
+                },
+                {
+                    "bindings": {
+                        "ctrl-a": "zed::SomeOtherAction"
+                    }
+                }
+            ]"#
+            .unindent(),
+        );
+
+        // Replacing a non-user binding with a context and a keystroke change
+        // should produce a suppression entry that preserves the context.
+        check_keymap_update(
+            r#"[
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "ctrl-a": "zed::SomeAction"
+                    }
+                }
+            ]"#
+            .unindent(),
+            KeybindUpdateOperation::Replace {
+                target: KeybindUpdateTarget {
+                    keystrokes: &parse_keystrokes("ctrl-a"),
+                    action_name: "zed::SomeAction",
+                    context: Some("SomeContext"),
+                    action_arguments: None,
+                },
+                source: KeybindUpdateTarget {
+                    keystrokes: &parse_keystrokes("ctrl-b"),
+                    action_name: "zed::SomeOtherAction",
+                    context: Some("SomeContext"),
+                    action_arguments: None,
+                },
+                target_keybind_source: KeybindSource::Default,
+            },
+            r#"[
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "ctrl-a": "zed::SomeAction"
+                    }
+                },
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "ctrl-b": "zed::SomeOtherAction"
+                    }
+                },
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "ctrl-a": null
+                    }
                 }
             ]"#
             .unindent(),