From 90fe7b686db81ee2607ca7879bdd898f05e7f1d0 Mon Sep 17 00:00:00 2001 From: Onur Solmaz <2453968+osolmaz@users.noreply.github.com> Date: Fri, 10 Jul 2026 21:07:10 +0800 Subject: [PATCH 1/2] feat: make comments keyboard-focusable --- README.md | 10 ++-- scripts/e2e-tmux.sh | 17 ++++++ src/app.rs | 132 +++++++++++++++++++++++++++++++++++++++++--- src/render.rs | 84 +++++++++++++++++++++++++++- src/runner.rs | 30 +++++++++- 5 files changed, 255 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 06f42cc..93115dc 100644 --- a/README.md +++ b/README.md @@ -12,8 +12,8 @@ second line" ``` Drag across source lines and release to open the inline comment editor. Enter saves -the comment. Existing comments remain visible below their ranges and can be clicked -to edit them. +the comment. Existing comments remain visible below their ranges; reach them with +Up/Down and press Enter, or click one, to edit it. ## Install @@ -73,13 +73,13 @@ Keyboard controls: | Key | Action | | --- | --- | -| `j` / `k`, arrows | Move between source lines | +| `j` / `k`, Up/Down | Move between source lines and inline comments | | `Shift-Up` / `Shift-Down` | Select a range; release Shift to write the comment | | `v` | Start or cancel range selection | -| `Enter` | Comment on the cursor/range, or save an active comment | +| `Enter` | Comment on a source range, edit a focused comment, or save the editor | | `Ctrl-O` | Insert a newline in a comment | | `Esc` | Cancel selection or editing | -| `e` / `d` | Edit or delete a comment on the cursor line | +| `e` / `d` | Edit or delete a focused comment or one on the cursor line | | `[` / `]` | Jump to the previous/next comment | | `h` / `l` | Scroll long source lines horizontally | | `q` | Write output and quit | diff --git a/scripts/e2e-tmux.sh b/scripts/e2e-tmux.sh index 1320642..c297ef9 100755 --- a/scripts/e2e-tmux.sh +++ b/scripts/e2e-tmux.sh @@ -101,6 +101,23 @@ assert review["comments"] == [ ] PY +start_tui "$tmpdir/keyboard-edit-output.md" "--comments '$tmpdir/review.json' --no-mouse" + +# Walk from source line 1 to source line 2 and then to its inline comment. +# Open, change, and save that comment without mouse input. +tmux send-keys -t "$session" Down Down +wait_for_pane "▶─ human comment here ..." +tmux send-keys -t "$session" Enter +wait_for_pane "Comment on lines 1" +tmux send-keys -t "$session" -l " edited" +tmux send-keys -t "$session" Enter +wait_for_pane "Comment saved" +wait_for_pane "▶─ human comment here ... edited" +finish_tui + +printf '> quoted part line 1\n> quoted part line 2\n\nhuman comment here ... edited\n' >"$tmpdir/expected-keyboard-edit.md" +diff -u "$tmpdir/expected-keyboard-edit.md" "$tmpdir/keyboard-edit-output.md" + start_tui "$tmpdir/shift-output.md" "--comments '$tmpdir/shift-review.json' --no-mouse" # Select all three source lines with Shift-Down, then inject a left-Shift release diff --git a/src/app.rs b/src/app.rs index 36652e7..c82c8ae 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1,3 +1,5 @@ +use std::collections::BTreeMap; + use ratatui_textarea::{CursorMove, TextArea}; use crate::{ @@ -28,6 +30,12 @@ impl Selection { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum BrowseTarget { + Source(usize), + Comment(u64), +} + #[derive(Debug)] pub struct CommentEditor { pub comment_id: Option, @@ -113,6 +121,37 @@ impl App { self.status = None; } + pub fn move_browse_focus(&mut self, delta: isize) { + if self.selection.is_some() { + self.move_cursor(delta); + return; + } + + let targets = self.browse_targets(); + let current = self + .active_comment_id + .filter(|id| self.review.comments.iter().any(|comment| comment.id == *id)) + .map_or( + BrowseTarget::Source(self.cursor_line), + BrowseTarget::Comment, + ); + let current_index = targets + .iter() + .position(|target| *target == current) + .unwrap_or_default(); + let target_index = current_index + .saturating_add_signed(delta) + .min(targets.len().saturating_sub(1)); + + match targets[target_index] { + BrowseTarget::Source(line) => self.move_to_line(line), + BrowseTarget::Comment(id) => { + self.focus_comment(id); + } + } + self.status = self.active_comment_status(); + } + pub fn move_to_line(&mut self, line: usize) { self.cursor_line = line.clamp(1, self.source.line_count()); if let Some(selection) = &mut self.selection { @@ -213,6 +252,12 @@ impl App { id.is_some_and(|id| self.begin_edit(id)) } + pub fn open_focused_editor(&mut self) { + if !self.active_comment_id.is_some_and(|id| self.begin_edit(id)) { + self.open_selected_editor(); + } + } + pub fn submit_editor(&mut self) -> bool { let Some(editor) = &self.editor else { return false; @@ -293,15 +338,59 @@ impl App { .rposition(|comment| comment.end_line < self.cursor_line) .unwrap_or(self.review.comments.len() - 1) }; - let comment = &self.review.comments[target_index]; - self.cursor_line = comment.start_line; - self.active_comment_id = Some(comment.id); + let id = self.review.comments[target_index].id; + self.focus_comment(id); + self.status = self.active_comment_status(); + } + + fn browse_targets(&self) -> Vec { + let mut comments_by_end = BTreeMap::>::new(); + for comment in &self.review.comments { + comments_by_end + .entry(comment.end_line) + .or_default() + .push(comment.id); + } + + let mut targets = Vec::with_capacity(self.source.line_count() + self.review.comments.len()); + for line in 1..=self.source.line_count() { + targets.push(BrowseTarget::Source(line)); + if let Some(comment_ids) = comments_by_end.get(&line) { + targets.extend(comment_ids.iter().copied().map(BrowseTarget::Comment)); + } + } + targets + } + + fn focus_comment(&mut self, id: u64) -> bool { + let Some(end_line) = self + .review + .comments + .iter() + .find(|comment| comment.id == id) + .map(|comment| comment.end_line) + else { + return false; + }; + self.cursor_line = end_line; + self.active_comment_id = Some(id); + self.selection = None; self.follow_cursor = true; - self.status = Some(format!( - "Comment {}/{} · e edit · d delete", - target_index + 1, + true + } + + fn active_comment_status(&self) -> Option { + let index = self.active_comment_id.and_then(|id| { + self.review + .comments + .iter() + .position(|comment| comment.id == id) + })?; + Some(format!( + "Comment {}/{} · Enter/e edit · d delete", + index + 1, self.review.comments.len() - )); + )) } fn comment_id_at_cursor(&self) -> Option { @@ -533,4 +622,33 @@ mod tests { assert!(app.edit_comment_at_cursor()); assert_eq!(app.editor.as_ref().unwrap().comment_id, Some(1)); } + + #[test] + fn vertical_focus_visits_inline_comments_in_document_order() { + let mut app = app(); + for id in [1, 2] { + app.review.upsert_comment(Comment { + id, + start_line: 1, + end_line: 2, + body: format!("comment {id}"), + }); + } + + app.move_browse_focus(1); + assert_eq!((app.cursor_line, app.active_comment_id), (2, None)); + app.move_browse_focus(1); + assert_eq!((app.cursor_line, app.active_comment_id), (2, Some(1))); + app.move_browse_focus(1); + assert_eq!(app.active_comment_id, Some(2)); + app.move_browse_focus(1); + assert_eq!((app.cursor_line, app.active_comment_id), (3, None)); + + app.move_browse_focus(-1); + assert_eq!(app.active_comment_id, Some(2)); + app.move_browse_focus(-1); + assert_eq!(app.active_comment_id, Some(1)); + app.move_browse_focus(-1); + assert_eq!((app.cursor_line, app.active_comment_id), (2, None)); + } } diff --git a/src/render.rs b/src/render.rs index 3ee93e0..e44044d 100644 --- a/src/render.rs +++ b/src/render.rs @@ -239,6 +239,28 @@ fn update_scroll(app: &mut App, rows: &[DocumentRow], viewport_height: usize) { ) }) .unwrap_or(0); + let focused_comment_row = app.active_comment_id.and_then(|id| { + rows.iter().position(|row| { + matches!( + row, + DocumentRow::Comment { + id: row_id, + first: true, + .. + } if *row_id == id + ) + }) + }); + if let Some(focus_row) = focused_comment_row { + if focus_row < app.scroll_row { + app.scroll_row = focus_row; + } else if focus_row >= app.scroll_row.saturating_add(viewport_height) { + app.scroll_row = focus_row.saturating_add(1).saturating_sub(viewport_height); + } + app.scroll_row = app.scroll_row.min(maximum); + app.follow_cursor = false; + return; + } let target_row = if app.editor.is_some() { rows.iter() .enumerate() @@ -278,7 +300,8 @@ fn render_source_row( let selected = app .selection .is_some_and(|selection| selection.contains(line_number)); - let cursor = app.cursor_line == line_number && app.editor.is_none(); + let cursor = + app.cursor_line == line_number && app.editor.is_none() && app.active_comment_id.is_none(); let marker = if cursor { "▶" } else { " " }; let number = format!("{marker}{line_number:>line_digits$} "); let text = horizontally_scrolled( @@ -326,7 +349,8 @@ fn render_comment_row( active: bool, ) { let prefix = if first { - format!("{}└─ ", " ".repeat(line_digits + 2)) + let marker = if active { "▶─ " } else { "└─ " }; + format!("{}{marker}", " ".repeat(line_digits + 2)) } else { " ".repeat(line_digits + 5) }; @@ -487,6 +511,15 @@ mod tests { assert!(rendered.contains("└─ first line")); assert!(hits.iter().any(|hit| hit.target == HitTarget::Comment(1))); + app.move_to_line(2); + app.move_browse_focus(1); + terminal + .draw(|frame| hits = render_app(frame, &mut app)) + .unwrap(); + let rendered = terminal.backend().to_string(); + assert!(rendered.contains("▶─ first line")); + assert!(!rendered.contains("▶2 ┃ two")); + app.begin_edit(1); terminal .draw(|frame| hits = render_app(frame, &mut app)) @@ -501,7 +534,7 @@ mod tests { terminal .draw(|frame| drop(render_app(frame, &mut app))) .unwrap(); - assert!(terminal.backend().to_string().contains("└─ code")); + assert!(terminal.backend().to_string().contains("▶─ code")); } #[test] @@ -526,6 +559,51 @@ mod tests { assert_eq!(horizontally_scrolled("e\u{301}abc", 1), "abc"); } + #[test] + fn keyboard_focused_comment_is_scrolled_into_view() { + let text = (1..=30) + .map(|line| format!("line {line}")) + .collect::>() + .join("\n"); + let source = SourceBuffer::from_bytes("long", text.as_bytes()).unwrap(); + let mut review = ReviewDocument::empty(source.source_ref()); + review.upsert_comment(crate::domain::Comment { + id: 1, + start_line: 20, + end_line: 20, + body: "focused comment".into(), + }); + let mut app = App::new(source, review); + app.jump_comment(true); + let backend = TestBackend::new(40, 8); + let mut terminal = Terminal::new(backend).unwrap(); + + terminal + .draw(|frame| drop(render_app(frame, &mut app))) + .unwrap(); + + let focused_row = document_rows(&app) + .iter() + .position(|row| { + matches!( + row, + DocumentRow::Comment { + id: 1, + first: true, + .. + } + ) + }) + .unwrap(); + let content_height = 6; + assert!(focused_row >= app.scroll_row); + assert!(focused_row < app.scroll_row + content_height); + assert!(terminal + .backend() + .to_string() + .contains("▶─ focused comment")); + } + #[test] fn tabs_expand_to_four_column_stops_before_scrolling() { assert_eq!(expand_tabs("\talpha"), " alpha"); diff --git a/src/runner.rs b/src/runner.rs index 8e79803..9d01c50 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -323,8 +323,8 @@ fn handle_browse_key(key: KeyEvent, app: &mut App) { KeyCode::Up if key.modifiers.contains(KeyModifiers::SHIFT) => { app.extend_shift_selection(-1); } - KeyCode::Down | KeyCode::Char('j') => app.move_cursor(1), - KeyCode::Up | KeyCode::Char('k') => app.move_cursor(-1), + KeyCode::Down | KeyCode::Char('j') => app.move_browse_focus(1), + KeyCode::Up | KeyCode::Char('k') => app.move_browse_focus(-1), KeyCode::PageDown => app.move_cursor(10), KeyCode::PageUp => app.move_cursor(-10), KeyCode::Char('g') => app.move_to_line(1), @@ -336,7 +336,7 @@ fn handle_browse_key(key: KeyEvent, app: &mut App) { app.begin_selection(app.cursor_line); } } - KeyCode::Enter => app.open_selected_editor(), + KeyCode::Enter => app.open_focused_editor(), KeyCode::Esc => app.cancel_selection(), KeyCode::Char('e') => { app.edit_comment_at_cursor(); @@ -549,6 +549,30 @@ mod tests { assert_eq!(app.review.comments.len(), 1); } + #[test] + fn arrows_and_enter_open_an_existing_comment_for_keyboard_editing() { + let mut app = app(); + app.review.upsert_comment(crate::domain::Comment { + id: 1, + start_line: 1, + end_line: 1, + body: "original".into(), + }); + + handle_key(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE), &mut app); + assert_eq!(app.active_comment_id, Some(1)); + handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE), &mut app); + assert_eq!(app.editor.as_ref().unwrap().comment_id, Some(1)); + handle_key( + KeyEvent::new(KeyCode::Char('!'), KeyModifiers::NONE), + &mut app, + ); + handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE), &mut app); + + assert_eq!(app.review.comments[0].body, "original!"); + assert_eq!(app.active_comment_id, Some(1)); + } + #[test] fn mouse_drag_release_opens_editor_for_the_range() { let mut app = app(); From 01a4c62801f5f789b1ccdd954fac53cd421444a5 Mon Sep 17 00:00:00 2001 From: Onur Solmaz <2453968+osolmaz@users.noreply.github.com> Date: Fri, 10 Jul 2026 21:09:05 +0800 Subject: [PATCH 2/2] test: cover keyboard focus state --- src/app.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/app.rs b/src/app.rs index c82c8ae..fb56f44 100644 --- a/src/app.rs +++ b/src/app.rs @@ -639,6 +639,10 @@ mod tests { assert_eq!((app.cursor_line, app.active_comment_id), (2, None)); app.move_browse_focus(1); assert_eq!((app.cursor_line, app.active_comment_id), (2, Some(1))); + assert_eq!( + app.status.as_deref(), + Some("Comment 1/2 · Enter/e edit · d delete") + ); app.move_browse_focus(1); assert_eq!(app.active_comment_id, Some(2)); app.move_browse_focus(1); @@ -651,4 +655,25 @@ mod tests { app.move_browse_focus(-1); assert_eq!((app.cursor_line, app.active_comment_id), (2, None)); } + + #[test] + fn moving_from_a_single_focused_comment_reaches_the_next_source_line() { + let mut app = app(); + app.review.upsert_comment(Comment { + id: 1, + start_line: 1, + end_line: 1, + body: "comment".into(), + }); + + app.move_browse_focus(1); + assert_eq!(app.active_comment_id, Some(1)); + assert_eq!( + app.status.as_deref(), + Some("Comment 1/1 · Enter/e edit · d delete") + ); + app.move_browse_focus(1); + assert_eq!((app.cursor_line, app.active_comment_id), (2, None)); + assert!(app.status.is_none()); + } }