From c6941b6c685a9c93183208106565dcf91df8af1b Mon Sep 17 00:00:00 2001 From: adehad <26027314+adehad@users.noreply.github.com> Date: Mon, 3 Aug 2026 12:18:34 +0100 Subject: [PATCH 1/2] docs: state what the code does, not what it replaced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several comments explained the change that introduced them — the DiffSource enum against the earlier staged/commit_id pair, the keymap registry against the any_overlay_active gate, shadow detection against the gap it filled. That reasoning belongs in the commit and PR that made the change; in the source it is stale the moment a reader arrives without memory of the prior code. Keep the facts a future maintainer needs, drop the comparisons. Co-Authored-By: Claude Fable 5 --- crates/rgitui_diff/src/lib.rs | 28 ++++++++----------- crates/rgitui_workspace/src/keymap/display.rs | 6 ++-- .../rgitui_workspace/src/keymap/registry.rs | 11 ++++---- crates/rgitui_workspace/src/keymap/shadow.rs | 12 ++++---- crates/rgitui_workspace/src/keymap/summary.rs | 4 +-- .../src/workspace/key_handler.rs | 8 +++--- 6 files changed, 31 insertions(+), 38 deletions(-) diff --git a/crates/rgitui_diff/src/lib.rs b/crates/rgitui_diff/src/lib.rs index 8d88710..89578b2 100644 --- a/crates/rgitui_diff/src/lib.rs +++ b/crates/rgitui_diff/src/lib.rs @@ -77,13 +77,9 @@ impl StagingAction { /// Where the content currently shown in the diff viewer came from. /// -/// This replaces an earlier `staged: bool` + `commit_id: Option<&str>` pair. -/// That pair could express the nonsensical state "a commit diff that is also -/// unstaged", and every historical call site did exactly that — which is what -/// made commit diffs render an "Unstaged" badge and offer live "Stage Hunk" -/// buttons. Modelling provenance as one enum makes that state unrepresentable: -/// staged/unstaged is only a property of the two mutable sources, and a commit -/// or stash carries its OID instead. +/// Staged versus unstaged is a property of the two mutable sources only. A +/// commit or stash has no such distinction — its content is already recorded — +/// so it carries its OID instead, and offers no staging. #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum DiffSource { /// Unstaged working-tree changes (index → workdir). Can be staged. @@ -4224,9 +4220,9 @@ mod tests { #[test] fn staging_a_commit_diff_is_rejected_with_an_actionable_message() { - // The pre-fix hazard: `s` over a commit diff reached - // `GitProject::stage_hunk_at`, which resolves the hunk index against - // the working tree — silently staging unrelated uncommitted edits. + // A staging request from a commit diff would reach + // `GitProject::stage_hunk_at`, which resolves the hunk index against the + // working tree — staging unrelated uncommitted edits. for action in [StagingAction::Stage, StagingAction::Unstage] { let message = DiffSource::Commit(OID.to_string()) .reject_staging(action) @@ -4417,8 +4413,8 @@ mod tests { fn is_change_line_accepts_additions_and_deletions_only() { // Addition: (None, Some) — staged on the new side. assert!(DiffViewer::is_change_line(&(None, Some(3)))); - // Deletion: (Some, None) — staged on the old side. The pre-fix viewer - // dropped these because it filtered to `new_num.is_some()`. + // Deletion: (Some, None) — staged on the old side. Filtering on + // `new_num.is_some()` would drop these. assert!(DiffViewer::is_change_line(&(Some(7), None))); // Context: (Some, Some) — carried by the git layer, never a change target. assert!(!DiffViewer::is_change_line(&(Some(7), Some(7)))); @@ -4957,10 +4953,10 @@ mod tests { /// /// The pure tests above prove `DiffSource` classifies content correctly. These /// drive a real `DiffViewer` in a headless GPUI window and press the actual -/// staging keys, which is the only way to show the *hazard* is closed rather -/// than just the label corrected: before the fix, `s` on a commit diff emitted -/// `HunkStageRequested`, and the workspace resolved that hunk index against the -/// working tree — silently staging unrelated uncommitted edits. +/// staging keys, which is the only way to show that no staging request escapes +/// a source that cannot be staged. A request that did would reach +/// `GitProject::stage_hunk_at`, which resolves the hunk index against the +/// working tree — staging unrelated uncommitted edits. #[cfg(test)] mod view_tests { use gpui::prelude::*; diff --git a/crates/rgitui_workspace/src/keymap/display.rs b/crates/rgitui_workspace/src/keymap/display.rs index 11b31c4..b326894 100644 --- a/crates/rgitui_workspace/src/keymap/display.rs +++ b/crates/rgitui_workspace/src/keymap/display.rs @@ -509,13 +509,13 @@ mod tests { } } - /// The scanner has to actually catch the string the old code contained, - /// otherwise it is a test that can never fail. + /// The scanner must catch a real hardcoded chord in each of the shapes one + /// can take, otherwise it is a test that can never fail. #[test] fn the_chord_scanner_catches_the_label_that_drifted() { let needles = chord_needles(); for offender in [ - // The literal the shortcut help used to carry for Fetch. + // A shortcut-help table entry. r#"("Ctrl+Shift+F", "Fetch"),"#, // The palette's hint field. r#"Some("Ctrl+Shift+F"),"#, diff --git a/crates/rgitui_workspace/src/keymap/registry.rs b/crates/rgitui_workspace/src/keymap/registry.rs index a51ac2e..50e3841 100644 --- a/crates/rgitui_workspace/src/keymap/registry.rs +++ b/crates/rgitui_workspace/src/keymap/registry.rs @@ -5,8 +5,7 @@ //! //! `Workspace` is set on the workspace root element. The root adds `modal` //! whenever an overlay or dialog is open, so `Workspace && !modal` is the -//! scope for shortcuts that must not fire while a modal is up — it replaces -//! the hand-rolled `any_overlay_active` gate for the commands listed here. +//! scope for shortcuts that must not fire while a modal is up. //! `TextInput` is set by [`rgitui_ui::TextInput`], so `!TextInput` keeps //! unmodified single-key shortcuts from stealing typed characters. //! @@ -878,10 +877,10 @@ mod tests { keystroke.key.chars().count() == 1 || keystroke.key == "space" } - /// The whole point of Phase B: gpui dispatches bindings deepest-context - /// first, so one letter can mean different things in different panels. Each - /// of these is bound several times over, and every binding must name a - /// distinct action in a distinct context — otherwise one of them is dead. + /// gpui dispatches bindings deepest-context first, so one letter can mean + /// different things in different panels. Each of these is bound several + /// times over, and every binding must name a distinct action in a distinct + /// context — otherwise one of them is dead. #[test] fn ambiguous_letters_resolve_to_one_action_per_context() { for keystrokes in ["d", "s", "p", "b", "h", "j", "k", "g", "y", "/", "[", "]"] { diff --git a/crates/rgitui_workspace/src/keymap/shadow.rs b/crates/rgitui_workspace/src/keymap/shadow.rs index c2d786a..f37da3e 100644 --- a/crates/rgitui_workspace/src/keymap/shadow.rs +++ b/crates/rgitui_workspace/src/keymap/shadow.rs @@ -2,9 +2,8 @@ //! //! [`super::conflict`] compares two bindings' context predicates for textual or //! [`is_superset`] overlap, which cannot see that `GraphView && !TextInput` sits -//! *inside* `Workspace && !modal` at dispatch time. So a binding scoped to a -//! panel taking a keystroke away from a global binding used to go completely -//! unreported. +//! *inside* `Workspace && !modal` at dispatch time — so that comparison alone +//! never catches a panel binding taking a keystroke away from a global one. //! //! It is reported here, and deliberately at a lower severity than a conflict: //! deeper-wins scoping is usually the whole point — the shipped defaults rely on @@ -205,10 +204,9 @@ mod tests { .collect() } - /// The case that motivated this: `SquashSelected` used to default to - /// `secondary-shift-s`, which the graph dispatched ahead of the workspace's - /// `UnstageAll` — invisible to conflict detection, because neither predicate - /// is a superset of the other. + /// A panel binding on the same keystroke as a global one: the graph + /// dispatches ahead of the workspace, and conflict detection cannot see it + /// because neither predicate is a superset of the other. #[test] fn a_panel_binding_masking_a_global_one_is_reported() { let bindings = [ diff --git a/crates/rgitui_workspace/src/keymap/summary.rs b/crates/rgitui_workspace/src/keymap/summary.rs index 6cc7a79..976d1c9 100644 --- a/crates/rgitui_workspace/src/keymap/summary.rs +++ b/crates/rgitui_workspace/src/keymap/summary.rs @@ -480,8 +480,8 @@ mod tests { "Ctrl+" }; - /// The drift that motivated this work: the help used to advertise - /// `Ctrl+Shift+F` for Fetch while the registry bound `Ctrl+Shift+R`. + /// Fetch is the canary: its label must be the keystroke the registry binds, + /// so a hand-written label cannot drift away from the binding again. #[test] fn a_default_label_comes_from_the_registry_keystroke() { let summary = defaults(); diff --git a/crates/rgitui_workspace/src/workspace/key_handler.rs b/crates/rgitui_workspace/src/workspace/key_handler.rs index 4ffe7cd..016fe43 100644 --- a/crates/rgitui_workspace/src/workspace/key_handler.rs +++ b/crates/rgitui_workspace/src/workspace/key_handler.rs @@ -1,9 +1,9 @@ //! How the workspace window resolves keystrokes. //! -//! There is no keyboard handler here any more. Every shortcut is declared by -//! `commands!` in [`crate::keymap::registry`], bound to a gpui action scoped to a -//! key context, and handled by an `on_action` listener on the element that owns -//! the behaviour. Two mechanisms replace what used to be hand-rolled here: +//! Every shortcut is declared by `commands!` in [`crate::keymap::registry`], +//! bound to a gpui action scoped to a key context, and handled by an +//! `on_action` listener on the element that owns the behaviour. Two mechanisms +//! carry the cases that are not a plain one-view binding: //! //! * **Esc and Enter.** `menu::Cancel` and `menu::Confirm` are each bound once. //! gpui dispatches an action outwards from the focused element and the first From c142e5bc9b32a0662d38ebd429e195865f0cfc57 Mon Sep 17 00:00:00 2001 From: Noah Clarkson Date: Sun, 9 Aug 2026 18:10:51 +1200 Subject: [PATCH 2/2] docs: clarify keymap guard rationale --- crates/rgitui_workspace/src/keymap/display.rs | 13 +++++-------- crates/rgitui_workspace/src/keymap/shadow.rs | 5 +++-- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/rgitui_workspace/src/keymap/display.rs b/crates/rgitui_workspace/src/keymap/display.rs index b326894..80fc82a 100644 --- a/crates/rgitui_workspace/src/keymap/display.rs +++ b/crates/rgitui_workspace/src/keymap/display.rs @@ -476,11 +476,8 @@ mod tests { .collect() } - /// The `Ctrl+Shift+F`-for-Fetch drift happened because a shortcut label was a - /// literal in a list nobody re-checked. Deleting the literals fixed it once; - /// this test is what stops the next one being written, by failing the build if - /// a user-facing surface spells a chord out again instead of asking the - /// keymap. + /// User-facing surfaces must derive shortcut labels from the keymap. This + /// test fails if a surface spells out a chord instead. /// /// Doc comments and the test modules are skipped — prose may name a chord, /// and an assertion has to spell out what it expects. @@ -512,15 +509,15 @@ mod tests { /// The scanner must catch a real hardcoded chord in each of the shapes one /// can take, otherwise it is a test that can never fail. #[test] - fn the_chord_scanner_catches_the_label_that_drifted() { + fn chord_scanner_catches_supported_hardcoded_forms() { let needles = chord_needles(); for offender in [ // A shortcut-help table entry. - r#"("Ctrl+Shift+F", "Fetch"),"#, + r#"("Ctrl+Shift+F", "Example command"),"#, // The palette's hint field. r#"Some("Ctrl+Shift+F"),"#, // A chord tucked inside a longer sentence. - r#"tooltip_text: "Fetch from remote (Ctrl+Shift+R)","#, + r#"tooltip_text: "Run command (Ctrl+Shift+R)","#, // The macOS spelling. r#"Label::new("⌘⇧R")"#, ] { diff --git a/crates/rgitui_workspace/src/keymap/shadow.rs b/crates/rgitui_workspace/src/keymap/shadow.rs index f37da3e..814d558 100644 --- a/crates/rgitui_workspace/src/keymap/shadow.rs +++ b/crates/rgitui_workspace/src/keymap/shadow.rs @@ -2,8 +2,9 @@ //! //! [`super::conflict`] compares two bindings' context predicates for textual or //! [`is_superset`] overlap, which cannot see that `GraphView && !TextInput` sits -//! *inside* `Workspace && !modal` at dispatch time — so that comparison alone -//! never catches a panel binding taking a keystroke away from a global one. +//! *inside* `Workspace && !modal` at dispatch time. That comparison does not +//! catch this nesting relationship when a panel binding takes a keystroke away +//! from a global one. //! //! It is reported here, and deliberately at a lower severity than a conflict: //! deeper-wins scoping is usually the whole point — the shipped defaults rely on