From e18a31075057a34b2cdb81d1e77395ada64d9ff0 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Wed, 2 Sep 2026 13:14:58 -0400 Subject: [PATCH 1/4] feat(review): multi-line annotation editor --- git-workon-review/src/editor.rs | 415 ++++++++++++++++++++++++++++++++ git-workon-review/src/lib.rs | 2 + git-workon-review/src/wrap.rs | 148 ++++++++++++ 3 files changed, 565 insertions(+) create mode 100644 git-workon-review/src/editor.rs create mode 100644 git-workon-review/src/wrap.rs diff --git a/git-workon-review/src/editor.rs b/git-workon-review/src/editor.rs new file mode 100644 index 00000000..c808e9c7 --- /dev/null +++ b/git-workon-review/src/editor.rs @@ -0,0 +1,415 @@ +//! Multi-line text buffer for the annotation editor modal (ADR-039 slice 3) — the multi-line +//! counterpart to [`crate::prompt::PromptState`]'s single line. `tui.rs`'s modal key-handling +//! arm drives this the same way it drives [`crate::prompt::PromptState`]: it decodes the shared +//! readline subset through its own `prompt_edit_for_key`, then layers `Up`/`Down`/`Enter` on top +//! for multi-line motion (`Ctrl-s` submits, `Esc` cancels — both stay in `tui.rs`/`app.rs`, this +//! module owns no keyboard policy). +//! +//! [`Self::col`] is a BYTE offset into the CURRENT line, the same discipline +//! [`crate::prompt::PromptState::cursor`] uses and for the same reason — `String::insert`/ +//! `remove`/slicing want byte offsets, so every edit stays a direct mutation with no index +//! translation; only screen placement ([`Self::cursor_screen_pos`]) needs a display column. + +use unicode_width::UnicodeWidthChar; + +use crate::wrap::wrap_text; + +/// A multi-line editable buffer: one or more lines of text, a cursor (line index + byte-offset +/// column within it), and a scroll offset for a viewport shorter than the buffer. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct EditorState { + lines: Vec, + line: usize, + col: usize, + scroll: usize, +} + +impl EditorState { + /// A fresh editor: one empty line, cursor at its start — what every slice-3 authoring flow + /// (create, reply) opens with; nothing seeds a draft yet. + pub fn new() -> Self { + Self { + lines: vec![String::new()], + line: 0, + col: 0, + scroll: 0, + } + } + + /// Seed the editor from existing text, cursor at the end — the natural counterpart to + /// [`Self::text`], for a future edit-in-place verb this slice doesn't add. + #[cfg(test)] + pub fn from_text(text: &str) -> Self { + let lines: Vec = if text.is_empty() { + vec![String::new()] + } else { + text.split('\n').map(str::to_string).collect() + }; + let line = lines.len() - 1; + let col = lines[line].len(); + Self { + lines, + line, + col, + scroll: 0, + } + } + + /// The buffer's lines, in source order — the render side reads this directly rather than + /// re-deriving it from [`Self::text`] every frame. + pub fn lines(&self) -> &[String] { + &self.lines + } + + /// The whole buffer, newline-joined — what a submit writes through the annotation store. + pub fn text(&self) -> String { + self.lines.join("\n") + } + + /// Whether the buffer holds anything worth confirming before a discard — a lone empty line + /// is the "just opened, nothing typed yet" state. + pub fn is_dirty(&self) -> bool { + self.lines.len() > 1 || !self.lines[0].is_empty() + } + + pub fn cursor_line(&self) -> usize { + self.line + } + + pub fn cursor_col(&self) -> usize { + self.col + } + + pub fn scroll(&self) -> usize { + self.scroll + } + + fn current(&self) -> &str { + &self.lines[self.line] + } + + fn prev_boundary(&self) -> Option { + self.current()[..self.col] + .char_indices() + .next_back() + .map(|(i, _)| i) + } + + fn next_boundary(&self) -> Option { + self.current()[self.col..] + .chars() + .next() + .map(|c| self.col + c.len_utf8()) + } + + /// Insert `c` at the cursor, then advance past it — same as + /// [`crate::prompt::PromptState::insert_char`], over the current line only. + pub fn insert_char(&mut self, c: char) { + let col = self.col; + self.lines[self.line].insert(col, c); + self.col += c.len_utf8(); + } + + /// `Enter`: split the current line at the cursor into two, cursor landing at the start of + /// the new (second) line. Plain-text authoring, no auto-indent — a comment body has no + /// syntax to indent against. + pub fn newline(&mut self) { + let tail = self.lines[self.line].split_off(self.col); + self.lines.insert(self.line + 1, tail); + self.line += 1; + self.col = 0; + } + + /// `Backspace`: delete the char before the cursor. At column 0 this joins the current line + /// onto the PREVIOUS one instead (the multi-line extra over + /// [`crate::prompt::PromptState::backspace`], which has no line to join into) — a no-op only + /// on the buffer's very first line. + pub fn backspace(&mut self) { + if let Some(prev) = self.prev_boundary() { + self.lines[self.line].drain(prev..self.col); + self.col = prev; + return; + } + if self.line == 0 { + return; + } + let tail = self.lines.remove(self.line); + self.line -= 1; + self.col = self.lines[self.line].len(); + self.lines[self.line].push_str(&tail); + } + + /// `Delete`: delete the char after the cursor. At the end of a line this joins the NEXT + /// line onto this one — the multi-line mirror of [`Self::backspace`]'s join. + pub fn delete(&mut self) { + if let Some(next) = self.next_boundary() { + self.lines[self.line].drain(self.col..next); + return; + } + if self.line + 1 >= self.lines.len() { + return; + } + let tail = self.lines.remove(self.line + 1); + self.lines[self.line].push_str(&tail); + } + + /// Move left one char; at column 0, wraps to the end of the previous line (a no-op on the + /// buffer's first line). + pub fn move_left(&mut self) { + if let Some(prev) = self.prev_boundary() { + self.col = prev; + } else if self.line > 0 { + self.line -= 1; + self.col = self.lines[self.line].len(); + } + } + + /// Move right one char; past the last char, wraps to the start of the next line (a no-op on + /// the buffer's last line). + pub fn move_right(&mut self) { + if let Some(next) = self.next_boundary() { + self.col = next; + } else if self.line + 1 < self.lines.len() { + self.line += 1; + self.col = 0; + } + } + + /// `Up`: previous line, column clamped to its length so the cursor never lands mid a + /// shorter line's missing tail. Readline has no analog for this — multi-line-specific. + pub fn move_up(&mut self) { + if self.line == 0 { + return; + } + self.line -= 1; + self.col = self.col.min(self.lines[self.line].len()); + } + + /// `Down`: mirror of [`Self::move_up`]. + pub fn move_down(&mut self) { + if self.line + 1 >= self.lines.len() { + return; + } + self.line += 1; + self.col = self.col.min(self.lines[self.line].len()); + } + + /// `Ctrl-a`/`Home`: start of the CURRENT line (not the whole buffer — multi-line + /// [`crate::prompt::PromptState::move_home`] has no "whole buffer" to distinguish from). + pub fn move_home(&mut self) { + self.col = 0; + } + + /// `Ctrl-e`/`End`: end of the current line. + pub fn move_end(&mut self) { + self.col = self.lines[self.line].len(); + } + + /// `Ctrl-u`: delete from the start of the current line up to the cursor — same rule as + /// [`crate::prompt::PromptState::clear_to_start`], scoped to one line. + pub fn clear_to_start(&mut self) { + self.lines[self.line].drain(..self.col); + self.col = 0; + } + + /// `Ctrl-w`: delete the "word" immediately before the cursor on the current line — same + /// skip-trailing-whitespace-then-delete-the-word algorithm as + /// [`crate::prompt::PromptState::delete_word_back`]. + pub fn delete_word_back(&mut self) { + if self.col == 0 { + return; + } + let before = &self.lines[self.line][..self.col]; + let mut end = self.col; + let mut chars = before.char_indices().rev().peekable(); + while let Some(&(i, c)) = chars.peek() { + if c.is_whitespace() { + end = i; + chars.next(); + } else { + break; + } + } + let mut start = end; + while let Some(&(i, c)) = chars.peek() { + if c.is_whitespace() { + break; + } + start = i; + chars.next(); + } + self.lines[self.line].drain(start..self.col); + self.col = start; + } + + /// The buffer wrapped to `width` display columns ([`wrap_text`]), one wrap call per SOURCE + /// line — a wrap must never merge two authored lines into one; the editor's own `\n`s are + /// paragraph breaks, not fill text `wrap_text` is free to re-flow across. + pub fn wrapped_lines(&self, width: usize) -> Vec { + self.lines + .iter() + .flat_map(|l| wrap_text(l, width)) + .collect() + } + + /// Where the cursor lands in [`Self::wrapped_lines`]' output space: `(row, col)`, both + /// 0-based, `col` a DISPLAY column (unicode-width aware, matching + /// `render::hscroll_cut`/[`crate::prompt::PromptState::cursor_col`]) rather than the byte + /// offset [`Self::col`] holds. + /// + /// Counts whole wrapped rows for every source line before [`Self::line`], then re-wraps just + /// the PREFIX of the current line up to the cursor to find which of ITS wrapped rows the + /// cursor sits on and how wide that row's prefix is — cheap (editor buffers are short) and + /// exact for the common case; a cursor sitting mid a run of collapsed whitespace (see + /// [`wrap_text`]'s doc comment on whitespace collapsing) can land a column or two off, which + /// is the same imprecision `wrap_text` itself accepts for prose reflow. + pub fn cursor_screen_pos(&self, width: usize) -> (usize, usize) { + let mut row = 0usize; + for line in &self.lines[..self.line] { + row += wrap_text(line, width).len(); + } + let prefix = &self.lines[self.line][..self.col]; + let prefix_wrapped = wrap_text(prefix, width); + // `wrap_text` never returns an empty `Vec` (an empty prefix still yields one empty + // line — see its doc comment), so `last()` always has something to measure. + let last_row = prefix_wrapped.last().map(String::as_str).unwrap_or(""); + let col = last_row + .chars() + .map(|c| UnicodeWidthChar::width(c).unwrap_or(0)) + .sum(); + row += prefix_wrapped.len() - 1; + (row, col) + } +} + +impl Default for EditorState { + fn default() -> Self { + Self::new() + } +} + +#[cfg(test)] +mod tests { + use super::EditorState; + + #[test] + fn insert_appends_and_advances_cursor() { + let mut e = EditorState::new(); + e.insert_char('a'); + e.insert_char('b'); + assert_eq!(e.lines(), &["ab"]); + assert_eq!((e.cursor_line(), e.cursor_col()), (0, 2)); + } + + #[test] + fn newline_splits_the_line_at_the_cursor() { + let mut e = EditorState::new(); + for c in "abcd".chars() { + e.insert_char(c); + } + e.move_left(); + e.move_left(); + e.newline(); + assert_eq!(e.lines(), &["ab", "cd"]); + assert_eq!((e.cursor_line(), e.cursor_col()), (1, 0)); + } + + #[test] + fn backspace_at_column_zero_joins_the_previous_line() { + let mut e = EditorState::from_text("ab\ncd"); + // `from_text` leaves the cursor at the end of the seeded text; walk it back to the + // start of the second line (column 0) — same field access `mod tests` gets on any + // private field of its parent module's types. + e.col = 0; + e.backspace(); + assert_eq!(e.lines(), &["abcd"]); + assert_eq!((e.cursor_line(), e.cursor_col()), (0, 2)); + } + + #[test] + fn backspace_on_the_first_line_at_column_zero_is_a_noop() { + let mut e = EditorState::new(); + e.backspace(); + assert_eq!(e.lines(), &[""]); + } + + #[test] + fn delete_at_end_of_line_joins_the_next_line() { + let mut e = EditorState::from_text("ab\ncd"); + e.move_home(); + e.move_up(); + e.move_end(); + e.delete(); + assert_eq!(e.lines(), &["abcd"]); + assert_eq!((e.cursor_line(), e.cursor_col()), (0, 2)); + } + + #[test] + fn move_up_down_clamp_column_to_the_shorter_line() { + let mut e = EditorState::from_text("abcdef\nxy"); + // Cursor starts at the end of "xy" (col 2); moving up should clamp to "abcdef"'s + // column 2, not carry a column past its own length. + e.move_up(); + assert_eq!((e.cursor_line(), e.cursor_col()), (0, 2)); + e.move_end(); + e.move_down(); + assert_eq!((e.cursor_line(), e.cursor_col()), (1, 2)); + } + + #[test] + fn is_dirty_is_false_only_for_a_fresh_empty_buffer() { + let mut e = EditorState::new(); + assert!(!e.is_dirty()); + e.insert_char('x'); + assert!(e.is_dirty()); + } + + #[test] + fn text_round_trips_through_newline_and_submit() { + let mut e = EditorState::new(); + for c in "line one".chars() { + e.insert_char(c); + } + e.newline(); + for c in "line two".chars() { + e.insert_char(c); + } + assert_eq!(e.text(), "line one\nline two"); + } + + #[test] + fn wrapped_lines_never_merges_across_a_source_newline() { + let mut e = EditorState::new(); + for c in "a b".chars() { + e.insert_char(c); + } + e.newline(); + for c in "c d".chars() { + e.insert_char(c); + } + // Width 3 alone would fit "a b" and "c d" onto one combined greedy-wrapped line if the + // wrap ran over the joined text — asserting it doesn't confirms the per-source-line call. + assert_eq!(e.wrapped_lines(3), vec!["a b", "c d"]); + } + + #[test] + fn cursor_screen_pos_tracks_a_wrapped_row_and_column() { + let mut e = EditorState::new(); + for c in "hello world".chars() { + e.insert_char(c); + } + // Width 5 wraps "hello world" to ["hello", "world"]; the cursor (at the end, after + // "world") should land on row 1, column 5. + assert_eq!(e.cursor_screen_pos(5), (1, 5)); + } + + #[test] + fn delete_word_back_skips_trailing_whitespace_then_deletes_the_word() { + let mut e = EditorState::new(); + for c in "foo bar ".chars() { + e.insert_char(c); + } + e.delete_word_back(); + assert_eq!(e.lines(), &["foo "]); + } +} diff --git a/git-workon-review/src/lib.rs b/git-workon-review/src/lib.rs index 5f16f1f8..cfdb0bc9 100644 --- a/git-workon-review/src/lib.rs +++ b/git-workon-review/src/lib.rs @@ -18,6 +18,7 @@ pub mod app; pub mod apply; pub mod clipboard; pub mod config; +pub mod editor; pub mod error; pub mod file_ops; pub mod highlight; @@ -40,3 +41,4 @@ pub mod synthesis; pub mod terminal_query; pub mod theme; pub mod wordiff; +pub mod wrap; diff --git a/git-workon-review/src/wrap.rs b/git-workon-review/src/wrap.rs new file mode 100644 index 00000000..028f56ae --- /dev/null +++ b/git-workon-review/src/wrap.rs @@ -0,0 +1,148 @@ +//! Pure greedy word-wrap, pulled forward from slice 5's chapter-prose plan into slice 3 +//! (ADR-039) because the multi-line annotation editor ([`crate::editor`]) needs it first — see +//! that module's `wrapped_lines` doc comment. Nothing in this crate wraps text today; do not use +//! `Paragraph::wrap` (no addressable output-line count, which both the editor's cursor placement +//! and slice 5's chapter height budget need). +//! +//! Rules: a `\n` in the source is always a hard break, never merged with the surrounding text; +//! within a paragraph, words fill greedily up to `width` display columns +//! ([`unicode_width::UnicodeWidthChar`], the same column math `render::hscroll_cut` uses for the +//! same reason — a byte and a display column disagree the instant a line has a multibyte or wide +//! char); a single word wider than `width` breaks AT the column. Unlike `hscroll_cut` (which +//! renders a fixed viewport and can't backtrack, so a straddling wide char is dropped and padded) +//! wrapping just starts a new output line, so a straddling char moves whole onto it instead of +//! being dropped. + +use unicode_width::UnicodeWidthChar; + +/// Wrap `text` to `width` display columns, one [`String`] per output line. Empty input yields +/// one empty line (a blank buffer/chapter still occupies a row); `width == 0` degrades to one +/// char per line rather than looping forever on a word that can never fit. +pub fn wrap_text(text: &str, width: usize) -> Vec { + let width = width.max(1); + text.split('\n') + .flat_map(|paragraph| wrap_paragraph(paragraph, width)) + .collect() +} + +/// Greedy-fill one `\n`-free paragraph. Whitespace runs collapse to a single joining space +/// between words (prose/comment wrapping, not a fixed-width preformatted block) — the exact +/// column an original run of spaces landed on isn't meaningful once the line has been re-flowed +/// anyway. +fn wrap_paragraph(paragraph: &str, width: usize) -> Vec { + let words: Vec<&str> = paragraph.split_whitespace().collect(); + if words.is_empty() { + return vec![String::new()]; + } + + let mut lines = Vec::new(); + let mut current = String::new(); + let mut current_width = 0usize; + + for word in words { + for piece in break_overlong(word, width) { + let piece_width = display_width(&piece); + let sep_width = if current.is_empty() { 0 } else { 1 }; + if !current.is_empty() && current_width + sep_width + piece_width > width { + lines.push(std::mem::take(&mut current)); + current_width = 0; + } + if !current.is_empty() { + current.push(' '); + current_width += 1; + } + current.push_str(&piece); + current_width += piece_width; + } + } + if !current.is_empty() { + lines.push(current); + } + lines +} + +/// Split `word` into `width`-or-narrower chunks when it alone is too wide to fit a line; +/// returns it unchanged as the sole element otherwise. Chunk boundaries never split a char (a +/// wide char that would push a chunk over `width` starts the NEXT chunk instead). +fn break_overlong(word: &str, width: usize) -> Vec { + if display_width(word) <= width { + return vec![word.to_string()]; + } + let mut chunks = Vec::new(); + let mut current = String::new(); + let mut current_width = 0usize; + for c in word.chars() { + let w = UnicodeWidthChar::width(c).unwrap_or(0); + if !current.is_empty() && current_width + w > width { + chunks.push(std::mem::take(&mut current)); + current_width = 0; + } + current.push(c); + current_width += w; + } + if !current.is_empty() { + chunks.push(current); + } + chunks +} + +fn display_width(s: &str) -> usize { + s.chars() + .map(|c| UnicodeWidthChar::width(c).unwrap_or(0)) + .sum() +} + +#[cfg(test)] +mod tests { + use unicode_width::UnicodeWidthChar; + + use super::wrap_text; + + #[test] + fn empty_input_yields_one_empty_line() { + assert_eq!(wrap_text("", 10), vec![String::new()]); + } + + #[test] + fn short_text_stays_on_one_line() { + assert_eq!(wrap_text("hello world", 20), vec!["hello world"]); + } + + #[test] + fn greedy_fill_breaks_at_the_word_boundary() { + // "hello world" is 11 columns; width 8 fits "hello" (5) but not "hello world" (11), and + // "hello" + " " + "world" (11) doesn't fit either, so "world" starts a new line. + assert_eq!(wrap_text("hello world", 8), vec!["hello", "world"]); + } + + #[test] + fn hard_break_on_newline_is_never_merged_with_fill() { + assert_eq!(wrap_text("hello\nworld", 20), vec!["hello", "world"]); + } + + #[test] + fn blank_paragraph_between_hard_breaks_survives_as_an_empty_line() { + assert_eq!(wrap_text("a\n\nb", 20), vec!["a", "", "b"]); + } + + #[test] + fn overlong_word_breaks_at_the_column() { + assert_eq!(wrap_text("abcdefghij", 4), vec!["abcd", "efgh", "ij"]); + } + + #[test] + fn wide_chars_count_by_display_width_not_char_count() { + // Each CJK char below is 2 display columns wide; width 4 fits exactly two per line. + let text = "\u{6f22}\u{5b57}\u{6f22}\u{5b57}"; + assert_eq!(UnicodeWidthChar::width('\u{6f22}'), Some(2)); + assert_eq!( + wrap_text(text, 4), + vec!["\u{6f22}\u{5b57}", "\u{6f22}\u{5b57}"] + ); + } + + #[test] + fn width_zero_degrades_to_one_char_per_line_instead_of_looping() { + assert_eq!(wrap_text("ab", 0), vec!["a", "b"]); + } +} From 0ccca9080a9db6e3daca750d40ff31a594efe837 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Wed, 2 Sep 2026 13:22:28 -0400 Subject: [PATCH 2/4] feat(review): create, reply, and resolve annotations --- git-workon-review/src/app.rs | 619 ++++++++++++++++++++++++++++++-- git-workon-review/src/keymap.rs | 19 + git-workon-review/src/render.rs | 32 ++ git-workon-review/src/tui.rs | 224 ++++++++++-- 4 files changed, 819 insertions(+), 75 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 677da2f0..1bc16353 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -18,7 +18,8 @@ use unicode_width::UnicodeWidthStr; use workon::{Changeset, ChangesetSpan}; use workon_annotations::store::AnnotationStore; use workon_annotations::{ - anchor as annot_anchor, Annotation, AnnotationKind, ChangesetKey, Fingerprint, + anchor as annot_anchor, Anchor, Annotation, AnnotationKind, ChangesetKey, Fingerprint, + NewAnnotation, Status, }; use crate::acquire::{ChangesetDiff, WorktreeDiffs}; @@ -28,6 +29,7 @@ use crate::align::{ }; use crate::apply::{Git2Applier, StageVerb}; use crate::config::RawViewConfig; +use crate::editor::EditorState; use crate::highlight::{lang_key_for_ext, FgSpan, TsHighlighter}; use crate::icons::IconMode; use crate::model::{DiffModel, FileChange, FileStatus, Hunk, LineKind}; @@ -1576,6 +1578,32 @@ pub struct App { /// showing — the same modal-capture shape as [`Self::help_visible`] (see that field's doc /// comment); `tui::update` intercepts every key while this is `true`. pub annotation_overlay_visible: bool, + /// The open annotation-authoring modal (`A` to create, or a reply key inside the annotation + /// overlay), or `None` when it's closed — combining presence, keyboard capture, AND the + /// pending write target in one field is the same doubled-up shape [`Self::pending_confirm`] + /// already uses (an `Option` that's both "is a modal showing" and "what does answering it + /// do"). `tui::update`'s Esc-ladder ranks this modal between the pending-confirm case and + /// the help overlay — see that function's doc comment. + editor: Option, +} + +/// The open annotation editor's buffer plus what accepting it (`Ctrl-s`, +/// [`App::submit_editor`]) writes through the store. +struct EditorSession { + state: EditorState, + target: EditorTarget, +} + +/// What [`App::submit_editor`] does with the editor's text once accepted. +#[derive(Debug, Clone, PartialEq, Eq)] +enum EditorTarget { + /// A brand-new top-level annotation, anchored to `anchor` at OPEN time (see + /// [`App::capture_annotation_anchor`]) — the anchor never moves while the editor is open, + /// even if the reviewer scrolls or re-selects before submitting. + Create { anchor: Anchor }, + /// A reply to the root annotation `parent_uid` — a reply carries no anchor of its own (see + /// [`workon_annotations::Annotation::anchor`]'s doc comment). + Reply { parent_uid: String }, } /// A destructive staging op deferred behind a [`Confirm`], identified by index into [`App::files`] @@ -1616,6 +1644,10 @@ pub enum PendingOp { files: Vec<(ChangesetIdentity, String)>, identity: OutlineRowIdentity, }, + /// Discard the open annotation editor's in-progress draft (`Esc` on a dirty buffer, see + /// [`App::editor_is_dirty`]) — the annotation-authoring analog of the worktree discards + /// above: this variant still discards SOMETHING, just a draft rather than a change. + DiscardEditorDraft, } /// A pending destructive op plus the scope-stating prompt shown on the footer until answered. @@ -1785,6 +1817,7 @@ impl App { tour_stops: Vec::new(), tour_idx: None, annotation_overlay_visible: false, + editor: None, }; // Position the outline cursor on the changeset/file the lib marked `current` (the same // row `sync_outline_to_current` would reposition to after any diff-initiated nav) rather @@ -1982,6 +2015,312 @@ impl App { self.annotation_overlay_visible = !self.annotation_overlay_visible; } + /// Capture an [`Anchor`] for the row under the cursor (or the active selection's range) in + /// the focused pane's current file/role — the annotation-authoring analog of + /// [`Self::resolve_yank_rows`], sharing [`resolve_row_side`]'s per-row side rule so anchor + /// capture and yank/copy can never pick a different side for the same row. The FIRST + /// resolved row picks the anchor's own side/target/context; a multi-line selection's LAST + /// resolved row only contributes `end_lineno` (mirroring [`Self::resolve_copy_location`]'s + /// lo/hi handling, which likewise doesn't require every row in a range to agree on a side). + /// `None` when there's no file/view loaded or the whole range is gap rows — same failure + /// shape as [`Self::resolve_yank_rows`]. + fn capture_annotation_anchor(&self) -> Option { + let view = self.current_view_ref()?; + let file = self.files().get(self.current)?; + let (lo, hi) = self.selection_range().unwrap_or((self.cursor, self.cursor)); + let rows: Vec<(bool, usize)> = (lo..=hi) + .filter_map(|r| resolve_row_side(view, self.layout, r)) + .collect(); + let (new_side, lineno) = *rows.first()?; + let (_, end_lineno) = *rows.last()?; + let lines: &[String] = if new_side { + &view.new_lines + } else { + &view.old_lines + }; + let idx = lineno.checked_sub(1)?; + let target = lines.get(idx)?.clone(); + let before = lines[idx.saturating_sub(3)..idx].to_vec(); + let after_end = (idx + 1 + 3).min(lines.len()); + let after = lines[idx + 1..after_end].to_vec(); + Some(Anchor { + path: file.path.clone(), + new_side, + lineno: lineno as u32, + end_lineno: end_lineno as u32, + target, + before, + after, + }) + } + + /// `user.name` from this repo's git config (the same identity `git commit` would use), or + /// `"reviewer"` when it's unset or the read fails — a repo with no configured identity, or a + /// permissions error, shouldn't block authoring on a config problem this crate has no UI to + /// fix. + fn annotation_author(&self) -> String { + self.repo + .config() + .and_then(|c| c.get_string("user.name")) + .unwrap_or_else(|_| "reviewer".to_string()) + } + + /// `AnnotationCreate` (`A`): open the editor to compose a brand-new top-level comment, + /// anchored via [`Self::capture_annotation_anchor`] at OPEN time. A footer notice (never a + /// panic) when the store is unavailable or there's no line to anchor to. + pub fn open_annotation_editor_for_create(&mut self) { + if self.annotations.is_none() { + self.notify( + "annotations unavailable — comments and tours are disabled this session", + Severity::Info, + ); + return; + } + let Some(anchor) = self.capture_annotation_anchor() else { + self.notify("no line here to annotate", Severity::Info); + return; + }; + self.editor = Some(EditorSession { + state: EditorState::new(), + target: EditorTarget::Create { anchor }, + }); + } + + /// Reply from inside the annotation overlay: open the editor targeting the FIRST root + /// annotation [`Self::annotations_at_cursor`] returns (a reply carries no anchor of its + /// own — it inherits the root's implicitly, see [`workon_annotations::Annotation::anchor`]'s + /// doc comment). A footer notice when there's nothing anchored to this row to reply to. + pub fn open_annotation_editor_for_reply(&mut self) { + if self.annotations.is_none() { + return; + } + let Some(root) = self + .annotations_at_cursor() + .into_iter() + .find(|a| a.parent_uid.is_none()) + else { + self.notify("nothing here to reply to", Severity::Info); + return; + }; + self.editor = Some(EditorSession { + state: EditorState::new(), + target: EditorTarget::Reply { + parent_uid: root.uid, + }, + }); + } + + /// Whether the annotation editor modal is showing — `tui::update`'s Esc-ladder case-2 modal + /// arm test, ranked between the pending-confirm case and the help overlay (see that + /// function's doc comment). + pub fn editor_is_open(&self) -> bool { + self.editor.is_some() + } + + /// Whether the open editor's buffer has anything typed — see + /// [`crate::editor::EditorState::is_dirty`]. `false` (never a panic) when the editor isn't + /// open at all. + pub fn editor_is_dirty(&self) -> bool { + self.editor.as_ref().is_some_and(|s| s.state.is_dirty()) + } + + /// The open editor's lines, for the render side — an empty slice when it isn't open. + pub fn editor_lines(&self) -> &[String] { + self.editor.as_ref().map(|s| s.state.lines()).unwrap_or(&[]) + } + + /// The open editor's buffer wrapped to `width` display columns — see + /// [`crate::editor::EditorState::wrapped_lines`]. Empty when the editor isn't open. + pub fn editor_wrapped_lines(&self, width: usize) -> Vec { + self.editor + .as_ref() + .map(|s| s.state.wrapped_lines(width)) + .unwrap_or_default() + } + + /// Where the open editor's cursor lands in its own wrapped output space — see + /// [`crate::editor::EditorState::cursor_screen_pos`]. `(0, 0)` when the editor isn't open + /// (the caller only reads this while it is). + pub fn editor_cursor_screen_pos(&self, width: usize) -> (usize, usize) { + self.editor + .as_ref() + .map(|s| s.state.cursor_screen_pos(width)) + .unwrap_or((0, 0)) + } + + /// `Esc` on a CLEAN editor buffer: close it outright, nothing to lose. + pub fn cancel_editor(&mut self) { + self.editor = None; + } + + /// `Esc` on a DIRTY editor buffer ([`Self::editor_is_dirty`]): raise a `pending_confirm` + /// instead of discarding outright — the same "a destructive op gets a confirm" rule the + /// staging discards follow, applied to a draft instead of a worktree change. + pub fn request_editor_discard_confirm(&mut self) { + self.pending_confirm = Some(Confirm { + prompt: "Discard this draft? (y/n)".to_string(), + op: PendingOp::DiscardEditorDraft, + }); + } + + /// `Ctrl-s`: accept the open editor. Writes the buffer's text through the store — + /// [`AnnotationStore::insert`] for a [`EditorTarget::Create`], + /// [`AnnotationStore::reply`] for a [`EditorTarget::Reply`] — then closes the modal. A blank + /// buffer (nothing typed) is silently dropped rather than writing an empty annotation. + /// + /// Deliberately does NOT bump [`Self::generation`] or call [`Self::coordinated_refresh`] + /// (ADR-039's gotcha, same as [`Self::poll_annotations`]): an annotation write changes what a + /// gutter marker overlays, not the diff content itself, and [`Self::annotation_markers`]/ + /// [`Self::annotations_at_cursor`] are computed fresh from the store on every call rather + /// than cached on `App` (see that method's doc comment) — the very next render already sees + /// this write with nothing further to rebuild. + pub fn submit_editor(&mut self) { + let Some(session) = self.editor.take() else { + return; + }; + let Some(store) = self.annotations.as_ref() else { + return; + }; + let text = session.state.text(); + if text.trim().is_empty() { + return; + } + let author = self.annotation_author(); + let result = match session.target { + EditorTarget::Create { anchor } => { + let changeset = self.current_changeset_key(); + store + .insert(NewAnnotation { + kind: AnnotationKind::Comment, + changeset, + anchor: Some(anchor), + body: text, + author, + tour: None, + seq: None, + }) + .map(|_| ()) + } + EditorTarget::Reply { parent_uid } => { + store.reply(&parent_uid, &text, &author).map(|_| ()) + } + }; + if result.is_err() { + self.notify("failed to save the annotation", Severity::Error); + } + } + + /// `AnnotationResolve`: toggle the FIRST root annotation [`Self::annotations_at_cursor`] + /// returns between [`Status::Open`]/[`Status::Resolved`] — bound both directly (from the + /// diff, without opening the overlay first) and, per ADR-039's slice-3 plan, as the key the + /// annotation overlay itself checks while it's showing. A footer notice when there's + /// nothing anchored to this row. Same generation/refresh gotcha as [`Self::submit_editor`]. + pub fn resolve_annotation_at_cursor(&mut self) { + let Some(store) = self.annotations.as_ref() else { + return; + }; + let Some(root) = self + .annotations_at_cursor() + .into_iter() + .find(|a| a.parent_uid.is_none()) + else { + self.notify("nothing here to resolve", Severity::Info); + return; + }; + let next = match root.status { + Status::Open => Status::Resolved, + Status::Resolved => Status::Open, + }; + if store.set_status(&root.uid, next).is_err() { + self.notify("failed to update the annotation status", Severity::Error); + } + } + + /// One typed char while the editor is focused — every non-control key `apply_editor_input_key` + /// decodes through `prompt_edit_for_key` routes here. + pub fn editor_insert_char(&mut self, c: char) { + if let Some(s) = self.editor.as_mut() { + s.state.insert_char(c); + } + } + + /// `Enter` while the editor is focused: split the line at the cursor. + pub fn editor_newline(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.newline(); + } + } + + /// `Backspace` while the editor is focused. + pub fn editor_backspace(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.backspace(); + } + } + + /// `Delete` while the editor is focused. + pub fn editor_delete(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.delete(); + } + } + + /// `Left` while the editor is focused. + pub fn editor_move_left(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.move_left(); + } + } + + /// `Right` while the editor is focused. + pub fn editor_move_right(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.move_right(); + } + } + + /// `Up` while the editor is focused. + pub fn editor_move_up(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.move_up(); + } + } + + /// `Down` while the editor is focused. + pub fn editor_move_down(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.move_down(); + } + } + + /// `Ctrl-a`/`Home` while the editor is focused. + pub fn editor_move_home(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.move_home(); + } + } + + /// `Ctrl-e`/`End` while the editor is focused. + pub fn editor_move_end(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.move_end(); + } + } + + /// `Ctrl-u` while the editor is focused. + pub fn editor_clear_to_start(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.clear_to_start(); + } + } + + /// `Ctrl-w` while the editor is focused. + pub fn editor_delete_word_back(&mut self) { + if let Some(s) = self.editor.as_mut() { + s.state.delete_word_back(); + } + } + /// Set the active walkthrough by name and reload its stops (`main.rs`'s future `--tour` /// flag, and tests). Nothing infers a tour automatically today — the store has no "list /// tours" query (a tour's identity is just whatever name its stops share), so a tour must @@ -4309,13 +4648,10 @@ impl App { /// active) in the FOCUSED pane's ACTIVE layout coordinate space — the same space /// [`Self::selection_range`] itself is already in, so no translation happens here. /// - /// - **SBS**: the NEW side's lineno, falling back to the OLD side on a pure-deletion row that - /// carries no new side (the same rule the old single-row `copy-path-line` resolver used). - /// `DisplayRow::Gap` rows are skipped, never emitted. - /// - **Inline**: `Del` -> old lineno, `Add` -> new lineno, `Context` -> new lineno — mirroring - /// [`Self::selection_line_ops`]'s per-side-precise handling (locked decision: which side - /// a row contributes — new side, old on pure deletions). - /// `InlineRow::Gap` rows are skipped. + /// The per-row side rule itself lives in [`resolve_row_side`] (its own doc comment has the + /// SBS/inline table) — factored out so ADR-039's annotation anchor capture + /// ([`Self::capture_annotation_anchor`]) shares it too, per that rule's own demand that + /// nothing else re-derive "which side does this row contribute." /// /// Each entry is `(is_new_side, lineno)`, one per non-gap row in range order — the order the /// caller needs both to pick text (per side) and to collapse a range to its first/last @@ -4326,32 +4662,9 @@ impl App { fn resolve_yank_rows(&self) -> Result, &'static str> { let view = self.current_view_ref().ok_or("no line to copy")?; let (lo, hi) = self.selection_range().unwrap_or((self.cursor, self.cursor)); - let mut rows = Vec::new(); - match self.layout { - Layout::Sbs => { - for r in lo..=hi { - let Some(row) = view.display.get(r) else { - continue; - }; - let (old, new) = display_row_linenos(row); - if let Some(n) = new { - rows.push((true, n)); - } else if let Some(n) = old { - rows.push((false, n)); - } - } - } - Layout::Inline => { - for r in lo..=hi { - match view.inline.get(r) { - Some(InlineRow::Del { old, .. }) => rows.push((false, *old)), - Some(InlineRow::Add { new, .. }) => rows.push((true, *new)), - Some(InlineRow::Context { new, .. }) => rows.push((true, *new)), - Some(InlineRow::Gap { .. }) | None => {} - } - } - } - } + let rows: Vec<(bool, usize)> = (lo..=hi) + .filter_map(|r| resolve_row_side(view, self.layout, r)) + .collect(); if rows.is_empty() { Err("no line to copy") } else { @@ -5657,6 +5970,7 @@ impl App { .collect(); self.outline_run_ops(ops, identity); } + PendingOp::DiscardEditorDraft => self.editor = None, } } @@ -6387,6 +6701,31 @@ pub(crate) fn display_row_linenos(row: &DisplayRow) -> (Option, Option Option<(bool, usize)> { + match layout { + Layout::Sbs => { + let row = view.display.get(row_idx)?; + let (old, new) = display_row_linenos(row); + match new { + Some(n) => Some((true, n)), + None => old.map(|n| (false, n)), + } + } + Layout::Inline => match view.inline.get(row_idx)? { + InlineRow::Del { old, .. } => Some((false, *old)), + InlineRow::Add { new, .. } => Some((true, *new)), + InlineRow::Context { new, .. } => Some((true, *new)), + InlineRow::Gap { .. } => None, + }, + } +} + /// The tree-sitter scope reveal's inputs for the gap at `gap_cursor`: the anchor line and which /// side it's in (`true` = new, `false` = old), resolved from the row immediately FOLLOWING the /// gap in `layout`'s row vector — the plan's rationale: the next hunk is what you're reading @@ -6607,10 +6946,10 @@ mod tests { use super::test_support::app_from_fixture; use super::{ - build_file_views, find_next_hunk_row, find_prev_hunk_row, App, ChangesetView, DiffState, - DiffTextMode, EffectiveZoom, HitRegions, Layout, LoadedViews, MarkerKind, Region, Role, - Severity, Summary, SummaryTarget, DEFAULT_OUTLINE_WIDTH, HSCROLL_STEP, MAX_OUTLINE_WIDTH, - MIN_OUTLINE_WIDTH, SCROLLOFF, + build_file_views, find_next_hunk_row, find_prev_hunk_row, resolve_row_side, App, + ChangesetView, DiffState, DiffTextMode, EffectiveZoom, HitRegions, Layout, LoadedViews, + MarkerKind, Region, Role, Severity, Summary, SummaryTarget, DEFAULT_OUTLINE_WIDTH, + HSCROLL_STEP, MAX_OUTLINE_WIDTH, MIN_OUTLINE_WIDTH, SCROLLOFF, }; use crate::align::{AlignedRow, CellKind, DisplayRow, InlineRow, Row}; use crate::config::{RawViewConfig, ReviewConfig}; @@ -6618,7 +6957,7 @@ mod tests { use crate::model::FileStatus; use crate::outline::{OutlineItem, OutlineMode, OutlineOrder, StagedStatus}; use workon_annotations::store::{AnnotationStore, TourStop, Walkthrough}; - use workon_annotations::{Anchor, AnnotationKind, ChangesetKey, NewAnnotation}; + use workon_annotations::{Anchor, AnnotationKind, ChangesetKey, NewAnnotation, Status}; /// Open the annotations store at `fixture`'s commondir — [`AnnotationStore::open`] the same /// way [`App::from_changesets`] does, so a test can seed rows [`app_from_fixture`]'s own @@ -14906,6 +15245,208 @@ mod tests { ); } + /// `view.display`'s row index whose NEW side resolves to `lineno` — a test-only helper over + /// [`resolve_row_side`] (the same per-row side rule [`App::capture_annotation_anchor`] + /// shares with [`App::resolve_yank_rows`]), since these tests need to park the cursor on a + /// specific content row before opening the editor rather than discovering the row from an + /// already-stored annotation the way the slice-2 tests above do. + fn row_for_new_lineno(view: &super::FileView, layout: Layout, lineno: usize) -> usize { + (0..view.display.len()) + .find(|&r| resolve_row_side(view, layout, r) == Some((true, lineno))) + .expect("no row resolves to that new-side lineno") + } + + #[test] + fn annotation_create_persists_and_a_marker_appears() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "tracked.txt", + "line1\nline2\nline3\n", + "line1\nCHANGED\nline3\n", + ) + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.ensure_loaded(0); + let role = app.focused_role_for(0); + let row_idx = { + let view = app.role_view_ref(0, role).unwrap(); + row_for_new_lineno(view, app.layout, 2) + }; + app.cursor = row_idx; + + app.open_annotation_editor_for_create(); + assert!( + app.editor_is_open(), + "a valid cursor row must capture an anchor and open the editor" + ); + for c in "why?".chars() { + app.editor_insert_char(c); + } + app.submit_editor(); + assert!(!app.editor_is_open(), "submit closes the modal"); + + let markers = app.annotation_markers(0, role); + assert_eq!(markers.len(), 1, "the new annotation resolves to one row"); + assert_eq!(*markers.values().next().unwrap(), MarkerKind::Comment); + } + + #[test] + fn annotation_create_with_a_blank_buffer_writes_nothing() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "tracked.txt", + "line1\nline2\nline3\n", + "line1\nCHANGED\nline3\n", + ) + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.ensure_loaded(0); + let role = app.focused_role_for(0); + let row_idx = { + let view = app.role_view_ref(0, role).unwrap(); + row_for_new_lineno(view, app.layout, 2) + }; + app.cursor = row_idx; + + app.open_annotation_editor_for_create(); + app.submit_editor(); + + assert!(!app.editor_is_open(), "submit always closes the modal"); + assert!( + app.annotation_markers(0, role).is_empty(), + "an empty buffer must not write an annotation" + ); + } + + #[test] + fn annotation_reply_from_overlay_writes_through_the_store() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "tracked.txt", + "line1\nline2\nline3\n", + "line1\nCHANGED\nline3\n", + ) + .build() + .unwrap(); + let store = seed_store(&fixture); + store + .insert(NewAnnotation { + kind: AnnotationKind::Comment, + changeset: ChangesetKey::new("main", true), + anchor: Some(single_line_anchor("tracked.txt", true, 2, "CHANGED")), + body: "why?".into(), + author: "reviewer".into(), + tour: None, + seq: None, + }) + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.ensure_loaded(0); + let role = app.focused_role_for(0); + let (&row_idx, _) = app.annotation_markers(0, role).iter().next().unwrap(); + app.cursor = row_idx; + + app.open_annotation_editor_for_reply(); + assert!( + app.editor_is_open(), + "a root at this row must open the editor" + ); + for c in "because".chars() { + app.editor_insert_char(c); + } + app.submit_editor(); + + let thread = app.annotations_at_cursor(); + assert_eq!(thread.len(), 2, "the root plus its new reply"); + assert_eq!(thread[1].body, "because"); + assert!( + thread[1].anchor.is_none(), + "a reply carries no anchor of its own" + ); + } + + #[test] + fn annotation_resolve_toggles_the_root_status() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "tracked.txt", + "line1\nline2\nline3\n", + "line1\nCHANGED\nline3\n", + ) + .build() + .unwrap(); + let store = seed_store(&fixture); + store + .insert(NewAnnotation { + kind: AnnotationKind::Comment, + changeset: ChangesetKey::new("main", true), + anchor: Some(single_line_anchor("tracked.txt", true, 2, "CHANGED")), + body: "why?".into(), + author: "reviewer".into(), + tour: None, + seq: None, + }) + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.ensure_loaded(0); + let role = app.focused_role_for(0); + let (&row_idx, _) = app.annotation_markers(0, role).iter().next().unwrap(); + app.cursor = row_idx; + + app.resolve_annotation_at_cursor(); + assert_eq!(app.annotations_at_cursor()[0].status, Status::Resolved); + + app.resolve_annotation_at_cursor(); + assert_eq!( + app.annotations_at_cursor()[0].status, + Status::Open, + "resolve toggles, it doesn't just set" + ); + } + + #[test] + fn annotation_submit_never_bumps_generation() { + // Mirrors `annotation_poll_never_bumps_generation`'s pin, over the write side: a local + // `submit_editor` write must never bump `App::generation` either — ADR-039's gotcha + // applies to every annotation write, not just the poll. + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "tracked.txt", + "line1\nline2\nline3\n", + "line1\nCHANGED\nline3\n", + ) + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.ensure_loaded(0); + let role = app.focused_role_for(0); + let row_idx = { + let view = app.role_view_ref(0, role).unwrap(); + row_for_new_lineno(view, app.layout, 2) + }; + app.cursor = row_idx; + let generation_before = app.generation(); + + app.open_annotation_editor_for_create(); + app.editor_insert_char('x'); + app.submit_editor(); + + assert_eq!( + app.generation(), + generation_before, + "submitting an annotation must never bump the ADR-037 generation counter" + ); + } + #[test] fn tour_next_and_prev_step_through_stops_and_switch_files() { let fixture = FixtureBuilder::new() diff --git a/git-workon-review/src/keymap.rs b/git-workon-review/src/keymap.rs index 6167090f..2be32c23 100644 --- a/git-workon-review/src/keymap.rs +++ b/git-workon-review/src/keymap.rs @@ -78,6 +78,8 @@ pub enum Command { CopyLines, CopyLocation, AnnotationView, + AnnotationCreate, + AnnotationResolve, TourNext, TourPrev, // Diff view. @@ -401,6 +403,23 @@ pub static REGISTRY: &[Registered] = &[ default_keys: "c", description: "View the comment thread/tour stop anchored to the row under the cursor", }, + Registered { + command: Command::AnnotationCreate, + view: View::Diff, + name: "annotation-create", + // `A` and `C` were both free at last audit (checked against the whole registry, not + // just `View::Diff` — `a`/`A`/`c`(taken by `annotation-view`)/`C` weren't bound + // anywhere). + default_keys: "A", + description: "Add a comment anchored to the cursor (or selection)", + }, + Registered { + command: Command::AnnotationResolve, + view: View::Diff, + name: "annotation-resolve", + default_keys: "C", + description: "Toggle open/resolved for the annotation thread under the cursor", + }, Registered { command: Command::TourNext, view: View::Diff, diff --git a/git-workon-review/src/render.rs b/git-workon-review/src/render.rs index 7ef96e23..8d2880c8 100644 --- a/git-workon-review/src/render.rs +++ b/git-workon-review/src/render.rs @@ -969,6 +969,13 @@ pub fn render(frame: &mut Frame, app: &mut App, keymap: &Keymap, theme: &Palette if app.annotation_overlay_visible { render_annotation_overlay(frame, app, area); } + // Drawn last (topmost) — the editor modal outranks every other modal in `tui::update`'s + // Esc-ladder (case 2, just below the pending confirm), so it must never render underneath + // one of these when both happen to be true (not reachable via normal input today, but the + // z-order should still match the precedence ordering). + if app.editor_is_open() { + render_editor_overlay(frame, app, area); + } } /// Convert a ratatui [`Rect`] into the [`Region`] shape [`App::hit_regions`] stores (mouse support) @@ -1083,6 +1090,31 @@ fn render_annotation_overlay(frame: &mut Frame, app: &App, area: Rect) { frame.render_widget(Paragraph::new(lines).block(block), popup_area); } +/// The annotation-authoring editor modal ([`App::editor_is_open`], ADR-039 slice 3) — same +/// centered-modal shape [`render_help_overlay`]/[`render_annotation_overlay`] use +/// (`centered_rect` + [`Clear`]). Content is [`App::editor_wrapped_lines`], wrapped to the +/// popup's own inner width (border columns subtracted first) so the cursor's wrapped position +/// ([`App::editor_cursor_screen_pos`], the same width) lines up with what's actually on screen. +/// No visible block cursor glyph yet (unlike [`crate::prompt::PromptState::render_line`]'s +/// reversed cell) — the wrapped row/col this method already computes is exactly what a future +/// cursor-placement pass would need; deferred since ratatui's own terminal cursor (not a styled +/// cell) is the more natural fit and needs a `Frame::set_cursor_position` call this render pass +/// doesn't otherwise make. +fn render_editor_overlay(frame: &mut Frame, app: &App, area: Rect) { + let popup_area = centered_rect(60, 60, area); + frame.render_widget(Clear, popup_area); + let inner_width = popup_area.width.saturating_sub(2) as usize; + let lines: Vec = app + .editor_wrapped_lines(inner_width.max(1)) + .into_iter() + .map(Line::from) + .collect(); + let block = Block::default() + .borders(Borders::ALL) + .title(" Annotate (Ctrl-s to submit, Esc to cancel) "); + frame.render_widget(Paragraph::new(lines).block(block), popup_area); +} + /// The style for a pane header/caption LABEL word (`focused-pane-header`), and — since /// `header-chrome-follows-focus` — the structural "identity" chrome that travels with it: the /// outline header's `[i/n]` counter, the diff header's `[fidx/nfiles]` counter, and the diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index 6b329d88..9ca97ef5 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -481,6 +481,8 @@ enum Action { CopyLines, CopyLocation, AnnotationView, + AnnotationCreate, + AnnotationResolve, TourNext, TourPrev, None, @@ -524,6 +526,8 @@ fn command_to_action(command: Command, pane_height: usize) -> Action { Command::CopyLines => Action::CopyLines, Command::CopyLocation => Action::CopyLocation, Command::AnnotationView => Action::AnnotationView, + Command::AnnotationCreate => Action::AnnotationCreate, + Command::AnnotationResolve => Action::AnnotationResolve, Command::TourNext => Action::TourNext, Command::TourPrev => Action::TourPrev, Command::NextFile => Action::NextFile, @@ -608,6 +612,11 @@ fn map_key( /// stop lands elsewhere in the stack), since those simply set a NEW pending open rather than /// needing the current one force-completed; plus pure UI toggles/no-ops (`Refresh` rebuilds all /// views itself; `ToggleHelp`/`AnnotationView`/`Quit`/`None` touch no view state at all). +/// +/// `AnnotationCreate`/`AnnotationResolve` join the first group too: both resolve +/// [`App::current_view_ref`] at ACTION time (anchor capture, and locating the root under the +/// cursor) rather than at render time the way `AnnotationView`'s overlay does, so they need the +/// same eager-load guarantee `j` + `s` does. fn action_needs_loaded_view(action: Action) -> bool { matches!( action, @@ -628,6 +637,8 @@ fn action_needs_loaded_view(action: Action) -> bool { | Action::ExpandAllGaps | Action::SearchNext | Action::SearchPrev + | Action::AnnotationCreate + | Action::AnnotationResolve ) } @@ -707,6 +718,8 @@ fn apply_action(app: &mut App, action: Action) -> bool { Action::CopyLines => app.copy_lines(), Action::CopyLocation => app.copy_location(), Action::AnnotationView => app.toggle_annotation_overlay(), + Action::AnnotationCreate => app.open_annotation_editor_for_create(), + Action::AnnotationResolve => app.resolve_annotation_at_cursor(), Action::TourNext => app.tour_next(), Action::TourPrev => app.tour_prev(), Action::None => {} @@ -887,6 +900,47 @@ fn apply_search_input_key(app: &mut App, key: KeyEvent) { } } +/// `update`'s case-2 modal arm (ADR-039 slice 3): apply one key press while the annotation +/// editor ([`App::editor_is_open`]) has keyboard capture. Handles its own `Ctrl-s`/`Enter`/ +/// `Up`/`Down`/`Esc` first, then delegates every other key to [`prompt_edit_for_key`] — same +/// shape as [`apply_filter_input_key`]/[`apply_search_input_key`], but the multi-line extras +/// ([`crate::editor::EditorState::newline`]/`move_up`/`move_down`) [`prompt_edit_for_key`] has +/// no token for, since [`crate::prompt::PromptState`] is single-line. +/// +/// `Esc` on a DIRTY buffer ([`App::editor_is_dirty`]) raises a `pending_confirm` +/// (`PendingOp::DiscardEditorDraft`) instead of discarding outright — same "a destructive op +/// gets a confirm" rule the staging discards follow, applied to a draft instead of a worktree +/// change; a clean buffer's `Esc` just closes, nothing to lose. +fn apply_editor_input_key(app: &mut App, key: KeyEvent) { + let ctrl = key.modifiers.contains(KeyModifiers::CONTROL); + match key.code { + KeyCode::Char('s') if ctrl => return app.submit_editor(), + KeyCode::Enter => return app.editor_newline(), + KeyCode::Up => return app.editor_move_up(), + KeyCode::Down => return app.editor_move_down(), + KeyCode::Esc => { + return if app.editor_is_dirty() { + app.request_editor_discard_confirm() + } else { + app.cancel_editor() + }; + } + _ => {} + } + match prompt_edit_for_key(key) { + Some(PromptEdit::InsertChar(c)) => app.editor_insert_char(c), + Some(PromptEdit::Backspace) => app.editor_backspace(), + Some(PromptEdit::Delete) => app.editor_delete(), + Some(PromptEdit::MoveLeft) => app.editor_move_left(), + Some(PromptEdit::MoveRight) => app.editor_move_right(), + Some(PromptEdit::MoveHome) => app.editor_move_home(), + Some(PromptEdit::MoveEnd) => app.editor_move_end(), + Some(PromptEdit::ClearToStart) => app.editor_clear_to_start(), + Some(PromptEdit::DeleteWordBack) => app.editor_delete_word_back(), + None => {} + } +} + /// Apply one [`AppEvent`] to `app`. Returns `true` when the loop should exit (q/Esc). Resize is a /// no-op — ratatui re-measures `body_area` every frame regardless. Tick drives /// [`App::on_tick`], the staging-verbs index watcher's poll (see the module doc). @@ -896,27 +950,38 @@ fn apply_search_input_key(app: &mut App, key: KeyEvent) { /// message and performs its normal action. `Resize`/`Tick` do NOT clear it: a redraw or timer /// tick isn't the user acting on the message. /// -/// Esc precedence (highest first): a pending discard confirm > the help overlay being open > -/// the annotation overlay being open > the outline fuzzy filter input having capture > the -/// in-diff search prompt having capture > an active line selection OR an active search -/// (diff-focused) > an active outline-filter query (outline-focused) > the outline having focus > -/// the diff having focus with the outline open > the normal key map (where Esc quits). -/// Concretely — the home-base model: the outline is where Esc always eventually lands you before -/// it quits, unwinding any inner mode (selection, search, filter) along the way. +/// Esc precedence (highest first): a pending discard confirm > the annotation editor modal being +/// open > the help overlay being open > the annotation overlay being open > the outline fuzzy +/// filter input having capture > the in-diff search prompt having capture > an active line +/// selection OR an active search (diff-focused) > an active outline-filter query +/// (outline-focused) > the outline having focus > the diff having focus with the outline open > +/// the normal key map (where Esc quits). Concretely — the home-base model: the outline is where +/// Esc always eventually lands you before it quits, unwinding any inner mode (selection, search, +/// filter) along the way. /// /// 1. A pending discard confirm captures the keyboard FIRST (before the notice clear and the /// normal key map): `y` accepts, `n`/`Esc` cancels, and every other key is swallowed — a modal /// that neither clears the notice nor runs a normal action while it's up. -/// 2. Otherwise, the help overlay (`?`) captures the keyboard next, mirroring the confirm modal's +/// 2. Otherwise, the annotation editor modal ([`App::editor_is_open`], ADR-039 slice 3) captures +/// the keyboard next: every key routes to [`apply_editor_input_key`]. Ranked just below the +/// confirm modal — a DIRTY buffer's `Esc` raises its OWN pending confirm +/// (`PendingOp::DiscardEditorDraft`, see [`App::request_editor_discard_confirm`]) rather than +/// discarding outright, so the confirm modal winning here keeps that draft-discard prompt from +/// ever being silently dismissed by a stray editor key, same rationale as help winning over +/// case 3 below. +/// 3. Otherwise, the help overlay (`?`) captures the keyboard next, mirroring the confirm modal's /// swallow: `?`/`q`/`Esc` close it, every other key is a no-op (nothing on the diff behind it -/// reacts). Ranked just below the confirm modal — in practice the two are never up -/// together, since opening help doesn't run through a confirm, but the confirm winning keeps +/// reacts). Ranked just below the editor modal — in practice the two are never up +/// together, since opening help doesn't run through the editor, but the editor winning keeps /// a destructive prompt from ever being silently dismissed by a stray overlay key. -/// 3. Otherwise, the annotation overlay (`c`, [`App::annotation_overlay_visible`]) captures the -/// keyboard next, mirroring help's own swallow: `c`/`q`/`Esc` close it, every other key is a -/// no-op. Ranked just below help — the two can't be up together (opening one never routes -/// through the other) — and above every case below, for the same reason help is. -/// 4. Otherwise, the outline fuzzy filter INPUT (`/`, while it has capture — see +/// 4. Otherwise, the annotation overlay (`c`, [`App::annotation_overlay_visible`]) captures the +/// keyboard next, mirroring help's own swallow: `c`/`q`/`Esc` close it; a reply key opens the +/// editor modal (case 2 above then wins on the NEXT key press); `C` +/// ([`App::resolve_annotation_at_cursor`]) toggles the row's root annotation's status without +/// closing the overlay; every other key is a no-op. Ranked just below help — the two can't be +/// up together (opening one never routes through the other) — and above every case below, for +/// the same reason help is. +/// 5. Otherwise, the outline fuzzy filter INPUT (`/`, while it has capture — see /// [`App::outline_filter_focused`]) captures next, mirroring the same swallow: typing/editing /// keys reach [`crate::prompt::PromptState`], `Enter`/`Esc` return capture to the outline row /// list KEEPING the query, `Ctrl-c` clears it and returns capture too, and `Down`/`Up`/ @@ -925,31 +990,31 @@ fn apply_search_input_key(app: &mut App, key: KeyEvent) { /// `?` nor `c` is part of the input's own key set — but the ordering still says which would /// win if that ever changed) and above every other case, since none of them should observe a /// key the filter input itself consumes. -/// 5. Otherwise, the in-diff search prompt (`/` in the diff view, while it has capture — see +/// 6. Otherwise, the in-diff search prompt (`/` in the diff view, while it has capture — see /// [`App::search_focused`]) captures next, mirroring the outline-filter input's swallow: /// typing/editing keys reach [`crate::prompt::PromptState`] (live-previewing highlights, never /// moving the cursor), `Enter` accepts and jumps, `Esc` aborts back to whatever search (or /// none) was active before `/` was pressed. Ranked below the outline-filter input for the same /// "can't actually collide today, but the ordering says who'd win" reason — the two prompts /// can never both have capture (one requires outline focus, the other diff focus). -/// 6. Otherwise, with the diff focused, Esc CANCELS an active line selection OR clears an active +/// 7. Otherwise, with the diff focused, Esc CANCELS an active line selection OR clears an active /// search (selection wins if, somehow, both are active) instead of moving focus or quitting -/// (`q` still quits). This arm is guarded to defer to case 8 when the outline has focus. Other +/// (`q` still quits). This arm is guarded to defer to case 9 when the outline has focus. Other /// keys fall through to the normal map — `j`/`k` extend a selection, `n`/`N` step a search. -/// 7. Otherwise, with the outline focused and a NON-EMPTY filter query (capture on the row list, -/// not the input — that's case 4), Esc CLEARS the filter ([`App::outline_filter_clear`]) -/// instead of quitting — the outline-side mirror of case 6's unwind-the-innermost-mode rule; -/// only the next Esc reaches case 8's quit leaf. -/// 8. Otherwise, while the outline pane has focus, Esc QUITS — same terminal leaf as `q`. The +/// 8. Otherwise, with the outline focused and a NON-EMPTY filter query (capture on the row list, +/// not the input — that's case 5), Esc CLEARS the filter ([`App::outline_filter_clear`]) +/// instead of quitting — the outline-side mirror of case 7's unwind-the-innermost-mode rule; +/// only the next Esc reaches case 9's quit leaf. +/// 9. Otherwise, while the outline pane has focus, Esc QUITS — same terminal leaf as `q`. The /// outline is home base; there's nowhere further out to walk to. -/// 9. Otherwise, with the diff focused and the outline OPEN, Esc walks outward one step: it -/// focuses the outline (same effect as `h`/[`App::focus_outline`]) rather than quitting. -/// 10. Otherwise (diff focused, outline closed) the normal map applies, where Esc (like `q`) +/// 10. Otherwise, with the diff focused and the outline OPEN, Esc walks outward one step: it +/// focuses the outline (same effect as `h`/[`App::focus_outline`]) rather than quitting. +/// 11. Otherwise (diff focused, outline closed) the normal map applies, where Esc (like `q`) /// quits — there's no outline to walk out to. /// -/// A `Key` event clears any showing footer notice before applying its own action (cases 6-10); -/// the confirm, help, annotation overlay, and the two prompt modals (cases 1-5) deliberately do -/// not. Cases 6-10 are delegated to [`resolve_key`], shared with [`update_batch`]. +/// A `Key` event clears any showing footer notice before applying its own action (cases 7-11); +/// the confirm, editor, help, annotation overlay, and the two prompt modals (cases 1-6) +/// deliberately do not. Cases 7-11 are delegated to [`resolve_key`], shared with [`update_batch`]. fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: AppEvent) -> bool { match event { AppEvent::Key(key) if app.pending_confirm.is_some() => { @@ -962,6 +1027,10 @@ fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: Ap } false } + AppEvent::Key(key) if app.editor_is_open() => { + apply_editor_input_key(app, key); + false + } AppEvent::Key(key) if app.help_visible => { match key.code { KeyCode::Char('?') | KeyCode::Char('q') | KeyCode::Esc => app.toggle_help(), @@ -970,14 +1039,19 @@ fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: Ap false } // The annotation overlay (`c`, `App::toggle_annotation_overlay`) captures the keyboard - // exactly like the help overlay above — `c`/`q`/`Esc` close it, every other key is - // swallowed. Ranked just below help (the two can't be up together; opening one never - // routes through the other), same as help is ranked just below the confirm modal. + // exactly like the help overlay above — `c`/`q`/`Esc` close it; `A` opens the editor + // modal in REPLY mode (case 2 above then wins on the next key press, since `editor_is_open` + // becomes true); `C` toggles the row's root annotation's status without closing the + // overlay. Every other key is swallowed. Ranked just below help (the two can't be up + // together; opening one never routes through the other), same as help is ranked just + // below the editor modal. AppEvent::Key(key) if app.annotation_overlay_visible => { match key.code { KeyCode::Char('c') | KeyCode::Char('q') | KeyCode::Esc => { app.toggle_annotation_overlay() } + KeyCode::Char('A') => app.open_annotation_editor_for_reply(), + KeyCode::Char('C') => app.resolve_annotation_at_cursor(), _ => {} } false @@ -994,12 +1068,13 @@ fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: Ap KeyOutcome::Handled => false, KeyOutcome::Action(action) => apply_action(app, action), }, - // Mouse support: all five modals swallow mouse input exactly like they swallow keys - // (cases 1-5 above) — a click/wheel while a discard confirm, the help overlay, the - // annotation overlay, the outline fuzzy filter input, or the in-diff search prompt is up - // does nothing. + // Mouse support: all six modals swallow mouse input exactly like they swallow keys + // (cases 1-6 above) — a click/wheel while a discard confirm, the editor modal, the help + // overlay, the annotation overlay, the outline fuzzy filter input, or the in-diff search + // prompt is up does nothing. AppEvent::Mouse(_) if app.pending_confirm.is_some() + || app.editor_is_open() || app.help_visible || app.annotation_overlay_visible || app.outline_filter_focused() @@ -2277,6 +2352,83 @@ mod tests { ); } + /// ADR-039 slice 3: the annotation editor modal's Esc-ladder case (case 2, between the + /// pending-confirm case and help) — a DIRTY buffer raises a `PendingOp::DiscardEditorDraft` + /// confirm rather than discarding outright. + #[test] + fn esc_on_a_dirty_editor_raises_a_discard_confirm() { + use git_workon_fixture::prelude::*; + use workon_review::app::PendingOp; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\ntwo\n", "one\nCHANGED\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + app.open_annotation_editor_for_create(); + assert!( + app.editor_is_open(), + "a loaded file's cursor row must capture an anchor and open the editor" + ); + app.editor_insert_char('x'); + + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + let quit = update( + &mut app, + &km, + &mut pending, + AppEvent::Key(key(KeyCode::Esc)), + ); + assert!(!quit, "the editor modal swallows Esc, it never quits"); + assert!( + app.editor_is_open(), + "a dirty buffer's Esc must not close the editor outright" + ); + assert!(matches!( + app.pending_confirm.as_ref().map(|c| &c.op), + Some(PendingOp::DiscardEditorDraft) + )); + + // Answering `y` (the confirm modal, which outranks the editor modal — case 1 over case 2) + // actually discards the draft. + app.resolve_confirm(true); + assert!(!app.editor_is_open()); + } + + /// The mirror of the test above: a CLEAN buffer's Esc closes the editor immediately, no + /// confirm — nothing to lose. + #[test] + fn esc_on_a_clean_editor_closes_it_immediately() { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\ntwo\n", "one\nCHANGED\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + app.open_annotation_editor_for_create(); + assert!(app.editor_is_open()); + + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + update( + &mut app, + &km, + &mut pending, + AppEvent::Key(key(KeyCode::Esc)), + ); + assert!( + !app.editor_is_open(), + "a clean buffer's Esc closes the editor with no confirm" + ); + assert!(app.pending_confirm.is_none()); + } + #[test] fn esc_cancels_a_selection_before_focusing_the_outline() { // Even with the outline open (so a bare Esc would otherwise walk out to it), an active From 4d7b037b02c5aec3fdb9f805044775b0e2e5d81b Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Sun, 6 Sep 2026 10:36:25 -0400 Subject: [PATCH 3/4] fix(review): route editor keys through the batched event loop --- git-workon-review/src/app.rs | 6 ++++ git-workon-review/src/tui.rs | 57 +++++++++++++++++++++++++++++++----- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 1bc16353..27da080e 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -2129,6 +2129,12 @@ impl App { self.editor.as_ref().map(|s| s.state.lines()).unwrap_or(&[]) } + /// The open editor's buffer, newline-joined — `None` when the editor isn't open, as opposed + /// to [`Self::editor_lines`]'s empty slice, since an open-but-empty buffer is one line. + pub fn editor_text(&self) -> Option { + self.editor.as_ref().map(|s| s.state.text()) + } + /// The open editor's buffer wrapped to `width` display columns — see /// [`crate::editor::EditorState::wrapped_lines`]. Empty when the editor isn't open. pub fn editor_wrapped_lines(&self, width: usize) -> Vec { diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index 9ca97ef5..34c0aa4e 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -1193,16 +1193,17 @@ fn update_batch( for event in events { match event { - // The coalescable path: no modal is up (the outline fuzzy filter input and the - // in-diff search - // search prompt both included — a key while either has capture must reach - // `apply_filter_input_key`/`apply_search_input_key` via the catch-all arm's `update` - // delegation below, never `resolve_key`/the coalescing path), and this isn't the - // selection-cancel/search-clear Esc guard (a context change — an "Esc cascade" — so it - // falls to the catch-all arm below, which flushes first and delegates the whole event - // to `update`). Notice-clearing still happens per key via `resolve_key`. + // The coalescable path: no modal is up (the outline fuzzy filter input, the in-diff + // search prompt, and the annotation editor all included — a key while any of them + // has capture must reach `apply_filter_input_key`/`apply_search_input_key`/ + // `apply_editor_input_key` via the catch-all arm's `update` delegation below, never + // `resolve_key`/the coalescing path), and this isn't the selection-cancel/search-clear + // Esc guard (a context change — an "Esc cascade" — so it falls to the catch-all arm + // below, which flushes first and delegates the whole event to `update`). Notice-clearing + // still happens per key via `resolve_key`. AppEvent::Key(key) if app.pending_confirm.is_none() + && !app.editor_is_open() && !app.help_visible && !app.annotation_overlay_visible && !app.outline_filter_focused() @@ -2398,6 +2399,46 @@ mod tests { assert!(!app.editor_is_open()); } + /// `update_batch`'s coalescing guard must bypass `resolve_key` for the annotation editor the + /// same as it does for every other modal — otherwise a real keystroke like `j`/`x` resolves + /// through the keymap (moving the cursor) instead of reaching `apply_editor_input_key`. + #[test] + fn editor_keys_reach_the_editor_through_update_batch() { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\ntwo\n", "one\nCHANGED\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + app.open_annotation_editor_for_create(); + assert!(app.editor_is_open()); + let cursor_before = app.cursor; + + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + let events = vec![ + AppEvent::Key(key(KeyCode::Char('x'))), + AppEvent::Key(key(KeyCode::Char('j'))), + AppEvent::Key(key(KeyCode::Enter)), + AppEvent::Key(key(KeyCode::Char('y'))), + ]; + let quit = update_batch(&mut app, &km, &mut pending, events); + + assert!(!quit); + assert_eq!( + app.cursor, cursor_before, + "with the editor open, `j` must not move the diff cursor" + ); + assert!( + app.editor_is_open(), + "typing into the editor must not close it" + ); + assert_eq!(app.editor_text().as_deref(), Some("xj\ny")); + } + /// The mirror of the test above: a CLEAN buffer's Esc closes the editor immediately, no /// confirm — nothing to lose. #[test] From 95ab107514b785940b6a6fd1c8751c13af3b4feb Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Sun, 6 Sep 2026 10:36:43 -0400 Subject: [PATCH 4/4] fix(review): allow line selection on committed changesets --- git-workon-review/src/app.rs | 128 +++++++++++++++++++++++++++-------- 1 file changed, 98 insertions(+), 30 deletions(-) diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 27da080e..490c9fa6 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -2135,6 +2135,17 @@ impl App { self.editor.as_ref().map(|s| s.state.text()) } + /// The open editor's [`EditorTarget::Create`] anchor, or `None` when the editor isn't open + /// or is targeting a reply instead — a test-only window into what + /// [`Self::capture_annotation_anchor`] resolved at open time. + #[cfg(test)] + pub fn editor_create_anchor(&self) -> Option<&Anchor> { + match self.editor.as_ref()?.target { + EditorTarget::Create { ref anchor } => Some(anchor), + EditorTarget::Reply { .. } => None, + } + } + /// The open editor's buffer wrapped to `width` display columns — see /// [`crate::editor::EditorState::wrapped_lines`]. Empty when the editor isn't open. pub fn editor_wrapped_lines(&self, width: usize) -> Vec { @@ -5744,8 +5755,8 @@ impl App { } /// Whether a staging verb would act on the current file rather than refuse — the same - /// [`Self::staging_role`] gate `stage_file`/`stage_hunk`/`start_selection` check before they - /// call [`Self::notify_unstageable_refusal`], read here by the renderer so the footer stops + /// [`Self::staging_role`] gate `stage_file`/`stage_hunk` check before they call + /// [`Self::notify_unstageable_refusal`], read here by the renderer so the footer stops /// advertising `stage`/`discard` where they can only refuse (`render::render_footer`). /// /// Deliberately the SAME predicate rather than a second one that reconstructs the conditions @@ -5778,16 +5789,15 @@ impl App { } } - /// Mode-aware refusal notice for a staging verb / line-selection start that only makes sense - /// outside the whole role — i.e. every call site below whose `staging_role()`/ - /// `staging_role().is_none()` guard failed (the locked decision that committed mode is - /// derived, not stored, with targeted guards). A - /// committed changeset is ALWAYS whole-only (no staged/unstaged split exists — see - /// [`Self::is_committed`]), so it gets its own wording. The non-committed branch's only + /// Mode-aware refusal notice for a staging verb that only makes sense outside the whole + /// role — i.e. every call site below whose `staging_role()`/`staging_role().is_none()` guard + /// failed (the locked decision that committed mode is derived, not stored, with targeted + /// guards). A committed changeset is ALWAYS whole-only (no staged/unstaged split exists — + /// see [`Self::is_committed`]), so it gets its own wording. The non-committed branch's only /// remaining caller is a binary file (ADR-038 decision 10): `effective_zoom` short-circuits /// on `!can_stage` before it looks at anything else, so no key press moves it out of /// `Role::Whole` — advising a key would be wrong, so this states non-stageability instead. - /// `verb` ("stage"/"select") keeps each call site's original non-committed wording. + /// `verb` ("stage") keeps each call site's original non-committed wording. fn notify_unstageable_refusal(&mut self, verb: &str) { if self.is_committed() { self.notify( @@ -6133,15 +6143,15 @@ impl App { self.derive_scroll(); } - /// Start a line selection anchored at the current cursor (`v`). Refuses (a notice, no anchor - /// set) on the whole role or any non-staging role — you can only select lines where you can - /// stage them (same gate as the verbs). A no-op on an empty file list. + /// Start a line selection anchored at the current cursor (`v`). A selection isn't + /// staging-shaped — `copy_lines`, `copy_location`, and `capture_annotation_anchor` all read + /// it in the whole role too — so the only real gate is having a loaded view to select in + /// (`stage_selection`/`discard_selection` already re-check `staging_role` themselves before + /// acting on it). Refuses (a notice, no anchor set) when nothing is loaded — a binary file, + /// or an empty file list. pub fn start_selection(&mut self) { - if self.cur().diff.files.is_empty() { - return; - } - if self.staging_role().is_none() { - self.notify_unstageable_refusal("select"); + if self.current_view_ref().is_none() { + self.notify("nothing here to select", Severity::Error); return; } self.selection_anchor = Some(self.cursor); @@ -10208,8 +10218,8 @@ mod tests { #[test] fn start_selection_on_a_binary_file_refuses() { - // Same re-point as `stage_hunk_on_a_binary_file_refuses_without_touching_the_index`: a - // binary file is the only non-committed case left that lands in `Role::Whole`. + // A binary file has no loaded view — `current_view_ref` is `start_selection`'s only + // gate now, so this is the one non-committed case left that still refuses. use super::Severity; let fixture = FixtureBuilder::new() @@ -10226,15 +10236,15 @@ mod tests { assert!( app.selection_anchor.is_none(), - "the whole role has no staging direction, so selection is refused" + "a binary file has nothing loaded to select in" ); let notice = app .notice .as_ref() - .expect("whole-role selection must refuse"); + .expect("a binary file's selection must refuse"); assert_eq!(notice.severity, Severity::Error); assert!( - notice.text.contains("not stageable"), + notice.text.contains("nothing here to select"), "got: {:?}", notice.text ); @@ -10788,11 +10798,17 @@ mod tests { app.clear_notice(); app.start_selection(); - assert!(app.selection_anchor.is_none()); + assert!( + app.selection_anchor.is_some(), + "start_selection no longer gates on staging_role — a committed changeset has a \ + loaded view, so selection starts" + ); + + app.stage_hunk(); let notice = app .notice .as_ref() - .expect("start_selection must refuse on a committed changeset"); + .expect("stage_hunk must still refuse on a committed changeset, selection or not"); assert!( notice.text.contains("already committed"), "got: {:?}", @@ -10800,6 +10816,57 @@ mod tests { ); } + /// A committed changeset's whole role now supports `v` end to end: `start_selection` sets an + /// anchor, extending the cursor extends the range, and `capture_annotation_anchor` (driven + /// through `open_annotation_editor_for_create`) resolves the FULL selected range into the + /// anchor rather than just the cursor row — the regression this bug fix targets. + #[test] + fn selection_on_a_committed_changeset_feeds_the_annotation_anchor() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let base = fixture + .commit("main") + .file("f.txt", "a\nb\nc\nd\ne\n") + .create("base") + .unwrap(); + let head = fixture + .commit("main") + .file("f.txt", "a\nB\nc\nD\ne\n") + .create("head") + .unwrap(); + let repo = fixture.repo().unwrap(); + let cs = Changeset { + name: "main".to_string(), + span: ChangesetSpan::Committed { base, head }, + title: None, + current: true, + needs_restack: false, + }; + let diff = crate::acquire::diff_changeset(repo, &cs).unwrap(); + let view = ChangesetView::from_changeset_diff(cs, diff); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view]); + app.open_current(); + let lineno = app.cursor as u32 + 1; + + app.start_selection(); + assert!(app.selection_anchor.is_some()); + app.move_cursor_by(1); + app.open_annotation_editor_for_create(); + + assert!(app.editor_is_open()); + let anchor = app + .editor_create_anchor() + .expect("open_annotation_editor_for_create must target a Create anchor"); + assert_eq!( + anchor.end_lineno, + lineno + 1, + "the anchor must span the whole selection, not just the cursor row" + ); + } + // ── The outline side pane (flat and stack modes) ────────────────────────────── /// A committed changeset (`base..head`, one file, not current) beneath an uncommitted @@ -15098,11 +15165,11 @@ mod tests { /// Content yank in `Role::Whole` succeeds — the locked decision that there is no /// whole-role exemption for yank pins this against a future "helpful" refusal: the /// side-selection rule (which side a row contributes) is total (it always yields a side), so - /// unlike the staging verbs there is nothing to refuse. `start_selection` itself still gates - /// whole role (it's a staging-shaped verb), so the selection is set directly here rather than - /// through `v`. ADR-038: `Role::Whole` for a file with real content is now only reachable - /// on a committed changeset (a binary file has no loaded view to copy from), so this exercises - /// it there instead of via a forced `Zoom::Combined`. + /// unlike the staging verbs there is nothing to refuse. `start_selection` no longer gates on + /// `staging_role` (it only needs a loaded view), so the selection is driven through it + /// directly here rather than being set by hand. ADR-038: `Role::Whole` for a file with real + /// content is now only reachable on a committed changeset (a binary file has no loaded view + /// to copy from), so this exercises it there instead of via a forced `Zoom::Combined`. #[test] fn content_yank_succeeds_in_whole_role() { let fixture = FixtureBuilder::new() @@ -15138,7 +15205,8 @@ mod tests { "a committed changeset always resolves to Role::Whole (effective_zoom)" ); app.cursor = 1; - app.selection_anchor = Some(1); + app.start_selection(); + assert!(app.selection_anchor.is_some()); app.cursor = 3; assert_eq!(app.resolve_copy_lines(), Ok("B\nc\nD".to_string()));