From 778652bc38ba421523cd24587f33a363cbff9c15 Mon Sep 17 00:00:00 2001 From: redquill test Date: Thu, 30 Jul 2026 00:08:25 -0500 Subject: [PATCH 1/6] feat(ui): make the submit modal scrollable with overflow indicators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The submit-review modal rendered one unscrollable Paragraph in a 72%x72% box, so a review batch taller than the modal was clipped with no scroll and no indicator — in the one surface whose job is confirming exactly what will be sent. The body now scrolls: Up/Down by a line, PageUp/PageDown by a real viewport, added to SUBMIT_FORGE_KEYS so the help overlay and footer stay in sync. Printable characters still fall through to the summary field, which is why the scroll keys are the arrow/page keys and not j/k. The offset is a Cell clamped to the content at render time (the help overlay's model) and reset on every fresh open, and when content is clipped the modal spends one row top and bottom on a marker naming how many lines are hidden in that direction. A blocked request-changes confirm jumps to the bottom so its hint can't land off-screen. Lines are pre-wrapped rather than handed to Paragraph's Wrap, so the count the scroll math clamps against is the row count the terminal really shows — otherwise a long batch's last rows stay unreachable and the hidden-line count understates. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AWX3hBxudUxZooeph9ngEB --- docs/forge-setup.md | 6 + src/ui/forge_submit.rs | 259 +++++++++++++++++++++++++++++++---- src/ui/forge_submit_tests.rs | 232 +++++++++++++++++++++++++++++++ src/ui/modal_keys.rs | 47 ++++++- src/ui/modes.rs | 7 +- 5 files changed, 519 insertions(+), 32 deletions(-) diff --git a/docs/forge-setup.md b/docs/forge-setup.md index 90fa16f..8608ba3 100644 --- a/docs/forge-setup.md +++ b/docs/forge-setup.md @@ -152,6 +152,12 @@ replies, a verdict, an optional summary — is local until you confirm it from the submit modal. Nothing is ever sent on quit, and nothing is sent until that confirm. +A batch taller than the modal scrolls — `↑`/`↓` by a line, `PageUp`/ +`PageDown` by a page — and a marker line names how many lines are hidden +above and below, so nothing you're about to send is clipped out of sight. +Printable keys still type the summary, which is why the scroll keys are the +arrow/page keys rather than `j`/`k`. + - **GitHub** posts one review (the reviews endpoint carries every positioned comment plus the verdict and summary at once), then any file-level comments and thread replies follow one at a time. Verdicts: diff --git a/src/ui/forge_submit.rs b/src/ui/forge_submit.rs index 72bb56e..9457e98 100644 --- a/src/ui/forge_submit.rs +++ b/src/ui/forge_submit.rs @@ -16,11 +16,13 @@ //! submitter, so a fake-provider test exercises the marking/persist/split logic //! via [`App::apply_submit_outcome`] directly without spawning anything. +use std::cell::Cell; + use ratatui::Frame; use ratatui::layout::{Constraint, Flex, Layout, Rect}; use ratatui::style::{Modifier, Style}; use ratatui::text::{Line, Span}; -use ratatui::widgets::{Block, Borders, Clear, Paragraph, Wrap}; +use ratatui::widgets::{Block, Borders, Clear, Paragraph}; use crate::annotate::{Annotation, Classification, Target}; use crate::forge::{Capabilities, SubmitBatch, SubmitReplyItem, SubmitReport, Verdict}; @@ -28,6 +30,7 @@ use crate::review::store::ForgeProviderKind; use super::app::{App, Mode}; use super::background::TaskId; +use super::theme::Theme; /// A background submit run awaiting completion: its [`TaskId`] and the /// generation captured at spawn (a straggler from a superseded run is @@ -58,6 +61,14 @@ pub(super) struct SubmitForgeState { /// blocked (e.g. request-changes with no summary). Cleared the moment the /// reviewer edits the verdict or summary. pub(super) hint: Option, + /// The preview's vertical scroll offset, advanced by the scroll keys and + /// clamped to the real rendered line count by [`render`], which writes the + /// clamped value back — the batch's height isn't known until the frame is + /// laid out (the help overlay's `Cell` model). + pub(super) scroll: Cell, + /// The scrollable body's height, recorded each frame so PageUp/PageDown + /// page by a real viewport. + pub(super) viewport: Cell, } impl SubmitForgeState { @@ -70,6 +81,99 @@ impl SubmitForgeState { } } +/// One frame's scroll geometry for the modal body: the clamped offset, the +/// body's height, and how many rendered lines are hidden above and below it. +/// Pure, so the clamp and the overflow decision are unit-tested without +/// building a frame. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) struct SubmitScroll { + pub(super) offset: u16, + pub(super) body_height: u16, + pub(super) hidden_above: u16, + pub(super) hidden_below: u16, +} + +/// Resolves the body geometry for `total` rendered lines inside an +/// `inner_height`-row modal at the requested offset. Content that fits gets +/// the whole box and no offset; content that overflows gives up one row at the +/// top and one at the bottom for the overflow markers — both for the whole +/// scroll, so the body never changes height (and the text never shifts) as the +/// markers come and go. `requested` is clamped, so `u16::MAX` means "bottom". +pub(super) fn resolve_scroll(total: u16, inner_height: u16, requested: u16) -> SubmitScroll { + if total <= inner_height { + return SubmitScroll { + offset: 0, + body_height: inner_height, + hidden_above: 0, + hidden_below: 0, + }; + } + // Below three rows there is no room to spend on markers; the body keeps + // every row it has and scrolls unmarked. + let body_height = if inner_height >= 3 { + inner_height - 2 + } else { + inner_height + }; + let offset = requested.min(total.saturating_sub(body_height)); + SubmitScroll { + offset, + body_height, + hidden_above: offset, + hidden_below: total.saturating_sub(offset.saturating_add(body_height)), + } +} + +/// `s` when `n` isn't one, for the marker lines' line count. +fn plural(n: u16) -> &'static str { + if n == 1 { "" } else { "s" } +} + +/// The clipped-below marker, or `None` when the last line is on screen. +fn below_marker(hidden: u16) -> Option { + (hidden > 0).then(|| { + format!( + "\u{25be} {hidden} more line{} \u{2014} \u{2193} to scroll", + plural(hidden) + ) + }) +} + +/// The clipped-above marker, or `None` when the first line is on screen. +fn above_marker(hidden: u16) -> Option { + (hidden > 0).then(|| format!("\u{25b4} {hidden} more line{} above", plural(hidden))) +} + +/// Splits `line` into rows no wider than `width`, preserving each character's +/// style. The modal pre-wraps rather than handing `Paragraph` a `Wrap`, so the +/// line count the scroll math clamps against is the row count the terminal +/// really shows — a re-flow behind the offset would put the bottom of a long +/// batch out of reach and understate the "N more lines" count. +fn wrap_line(line: &Line<'_>, width: usize) -> Vec> { + let chars: Vec<(char, Style)> = line + .spans + .iter() + .flat_map(|span| span.content.chars().map(move |c| (c, span.style))) + .collect(); + if chars.is_empty() { + return vec![Line::from(String::new())]; + } + let text: String = chars.iter().map(|(c, _)| *c).collect(); + super::textwrap::wrap_ranges(&text, width) + .into_iter() + .map(|(start, end)| { + let mut spans: Vec> = Vec::new(); + for (c, style) in &chars[start..end] { + match spans.last_mut() { + Some(prev) if prev.style == *style => prev.content.to_mut().push(*c), + _ => spans.push(Span::styled(c.to_string(), *style)), + } + } + Line::from(spans) + }) + .collect() +} + /// Which verdicts a provider supports, from its [`Capabilities`] — Comment is /// always offered; Approve and Request changes only when the capability flag /// is set. Drives the modal's verdict picker so it renders exactly the @@ -286,6 +390,9 @@ impl App { target_line, disclosure, hint: None, + // A fresh open starts at the top of the batch. + scroll: Cell::new(0), + viewport: Cell::new(0), }); self.mode = Mode::SubmitForge; } @@ -318,6 +425,38 @@ impl App { } } + /// Scrolls the preview down one line. The offset is clamped to the content + /// by [`render`], so an overshoot here can't run off the end. + pub(super) fn submit_forge_scroll_down(&mut self) { + if let Some(state) = self.submit_forge.as_ref() { + state.scroll.set(state.scroll.get().saturating_add(1)); + } + } + + /// Scrolls the preview up one line. + pub(super) fn submit_forge_scroll_up(&mut self) { + if let Some(state) = self.submit_forge.as_ref() { + state.scroll.set(state.scroll.get().saturating_sub(1)); + } + } + + /// Scrolls the preview down a full viewport (the height the last frame + /// recorded). + pub(super) fn submit_forge_page_down(&mut self) { + if let Some(state) = self.submit_forge.as_ref() { + let page = state.viewport.get().max(1); + state.scroll.set(state.scroll.get().saturating_add(page)); + } + } + + /// Scrolls the preview up a full viewport. + pub(super) fn submit_forge_page_up(&mut self) { + if let Some(state) = self.submit_forge.as_ref() { + let page = state.viewport.get().max(1); + state.scroll.set(state.scroll.get().saturating_sub(page)); + } + } + /// Appends a typed character to the summary (the modal's free-text field). pub(super) fn submit_forge_insert_char(&mut self, c: char) { if let Some(state) = self.submit_forge.as_mut() { @@ -349,6 +488,10 @@ impl App { if verdict == Verdict::RequestChanges && summary.is_empty() { state.hint = Some("Request changes needs a summary explaining what to change.".to_string()); + // The hint is the last line of the content, so jumping to the + // bottom (the render clamps it) puts it on screen for a batch too + // tall to fit. + state.scroll.set(u16::MAX); return; } self.submit_forge = None; @@ -564,26 +707,17 @@ fn centered(area: Rect, width_pct: u16, height_pct: u16) -> Rect { area } -/// Renders the submit-review modal, centered over `area`. A no-op when the -/// modal isn't open. Shows the target line, the verdict picker (selected -/// verdict emphasized), the grouped-by-file batch preview with local-only and -/// file-comment labels, the summary being typed, and an unmistakable -/// nothing-sent-until-confirm footer. -pub fn render(frame: &mut Frame, area: Rect, app: &App) { - let Some(state) = &app.submit_forge else { - return; - }; - let theme = &app.theme; - let popup = centered(area, 72, 72); - frame.render_widget(Clear, popup); - - let preview = build_preview( - app.annotations.unpublished(), - app.replies - .unpublished() - .map(|r| (r.thread_id, r.body.as_str())), - ); - +/// Builds the modal's body, top to bottom: the target line and submit-shape +/// disclosure, the verdict picker (selected verdict emphasized), the +/// grouped-by-file batch preview with local-only and file-comment labels, the +/// summary being typed, and a blocked-confirm validation hint. Split out from +/// [`render`] so the rendered line count — what the scroll math clamps against +/// — is a value the caller holds rather than a side effect of drawing. +fn build_lines( + state: &SubmitForgeState, + preview: &SubmitPreview, + theme: &Theme, +) -> Vec> { let mut lines: Vec = Vec::new(); // Target line — which PR this lands on. @@ -713,16 +847,89 @@ pub fn render(frame: &mut Frame, area: Rect, app: &App) { ))); } + lines +} + +/// Renders the submit-review modal, centered over `area`. A no-op when the +/// modal isn't open. Shows the batch (see [`build_lines`]) and an unmistakable +/// nothing-sent-until-confirm footer. A batch taller than the modal scrolls +/// (`Up`/`Down`, `PageUp`/`PageDown`) with a marker row naming how many lines +/// are hidden in each direction, so nothing about what will be sent is clipped +/// silently. +pub fn render(frame: &mut Frame, area: Rect, app: &App) { + let Some(state) = &app.submit_forge else { + return; + }; + let theme = &app.theme; + let popup = centered(area, 72, 72); + frame.render_widget(Clear, popup); + + let preview = build_preview( + app.annotations.unpublished(), + app.replies + .unpublished() + .map(|r| (r.thread_id, r.body.as_str())), + ); + let block = Block::default() .borders(Borders::ALL) .title("Submit review \u{2014} nothing is sent until you confirm") + // Kept under the narrow modal's title width: the "type summary" + // affordance the scroll hint displaces is already spelled out by the + // summary field's own placeholder, and the page keys by the `?` help. .title_bottom(Line::from( - " Enter submit Esc cancel Tab/Shift-Tab verdict type summary ", + " Enter submit Esc cancel Tab/Shift-Tab verdict \u{2191}\u{2193} scroll ", )); - let paragraph = Paragraph::new(lines) - .block(block) - .wrap(Wrap { trim: false }); - frame.render_widget(paragraph, popup); + let inner = block.inner(popup); + frame.render_widget(block, popup); + + let lines: Vec = build_lines(state, &preview, theme) + .into_iter() + .flat_map(|line| wrap_line(&line, inner.width as usize)) + .collect(); + + let total = u16::try_from(lines.len()).unwrap_or(u16::MAX); + let view = resolve_scroll(total, inner.height, state.scroll.get()); + state.scroll.set(view.offset); + state.viewport.set(view.body_height.max(1)); + + // Two rows means `resolve_scroll` reserved the marker rows; otherwise the + // whole box is body. + let body = if inner.height.saturating_sub(view.body_height) == 2 { + let [top, body, bottom] = Layout::vertical([ + Constraint::Length(1), + Constraint::Min(0), + Constraint::Length(1), + ]) + .areas(inner); + if let Some(text) = above_marker(view.hidden_above) { + frame.render_widget( + Paragraph::new(Line::from(Span::styled( + text, + Style::default() + .fg(theme.gutter) + .add_modifier(Modifier::DIM), + ))), + top, + ); + } + if let Some(text) = below_marker(view.hidden_below) { + frame.render_widget( + Paragraph::new(Line::from(Span::styled( + text, + Style::default() + .fg(theme.hunk_header) + .add_modifier(Modifier::BOLD), + ))), + bottom, + ); + } + body + } else { + inner + }; + + frame.render_widget(Paragraph::new(lines).scroll((view.offset, 0)), body); } #[cfg(test)] diff --git a/src/ui/forge_submit_tests.rs b/src/ui/forge_submit_tests.rs index 0d384b6..a34420e 100644 --- a/src/ui/forge_submit_tests.rs +++ b/src/ui/forge_submit_tests.rs @@ -1,3 +1,7 @@ +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; +use ratatui::Terminal; +use ratatui::backend::TestBackend; + use crate::annotate::{Classification, Side, Target}; use crate::diff::FileDiff; use crate::forge::{SubmitReport, Verdict}; @@ -5,6 +9,7 @@ use crate::git::{DiffTarget, RawFilePatch}; use crate::review::store::{ForgeMetadata, ForgeProviderKind}; use super::super::app::{App, Mode}; +use super::super::modes::handle_submit_forge_key; use super::*; // -- fixtures ---------------------------------------------------------------- @@ -537,6 +542,233 @@ fn typing_a_summary_clears_the_hint_and_lets_request_changes_confirm() { // -- confirm on the fake path sends nothing (no live backend) ---------------- +// -- scrollable preview + overflow markers ----------------------------------- + +/// Renders the modal over a `width` x `height` terminal and returns its cell +/// symbols as one string (the `cleanup_reviews_modal` test idiom). +fn render_modal(app: &App, width: u16, height: u16) -> String { + let backend = TestBackend::new(width, height); + let mut terminal = Terminal::new(backend).unwrap(); + let area = Rect::new(0, 0, width, height); + terminal.draw(|frame| render(frame, area, app)).unwrap(); + terminal + .backend() + .buffer() + .content() + .iter() + .map(|c| c.symbol()) + .collect() +} + +/// A review with `n` annotations spread over three files — enough rows to +/// overflow any sensibly sized modal. +fn app_with_many_annotations(n: usize) -> App { + let paths = ["src/a.rs", "src/b.rs", "src/c.rs"]; + let mut app = github_review_app(&paths); + for i in 0..n { + app.annotations + .add( + Target::line(paths[i % paths.len()], 2, Side::New), + Classification::Issue, + format!("comment number {i}"), + ) + .unwrap(); + } + app +} + +#[test] +fn scroll_geometry_clamps_to_the_content_and_counts_what_is_hidden() { + // Content that fits keeps the whole box, never offsets, and hides nothing + // — even when a stale offset asks for the bottom. + let fits = resolve_scroll(6, 20, u16::MAX); + assert_eq!(fits.offset, 0); + assert_eq!(fits.body_height, 20); + assert_eq!((fits.hidden_above, fits.hidden_below), (0, 0)); + + // 40 lines in a 10-row box: two rows go to the markers, so 8 show at once. + let top = resolve_scroll(40, 10, 0); + assert_eq!(top.body_height, 8); + assert_eq!((top.offset, top.hidden_above, top.hidden_below), (0, 0, 32)); + + // A mid-scroll offset splits the hidden lines between the two directions. + let mid = resolve_scroll(40, 10, 12); + assert_eq!( + (mid.offset, mid.hidden_above, mid.hidden_below), + (12, 12, 20) + ); + + // An overshoot lands on the last page, with nothing left below. + let bottom = resolve_scroll(40, 10, u16::MAX); + assert_eq!( + (bottom.offset, bottom.hidden_above, bottom.hidden_below), + (32, 32, 0) + ); + + // A box too short to spend rows on markers still scrolls, using them all. + let tiny = resolve_scroll(40, 2, u16::MAX); + assert_eq!(tiny.body_height, 2); + assert_eq!(tiny.offset, 38); +} + +#[test] +fn overflow_markers_are_shown_only_for_the_clipped_direction() { + assert_eq!(below_marker(0), None, "nothing below, no marker"); + assert_eq!(above_marker(0), None, "nothing above, no marker"); + let below = below_marker(7).expect("clipped below is marked"); + assert!( + below.contains('7') && below.contains("more lines"), + "{below}" + ); + assert!( + below.contains('\u{2193}'), + "the marker must name the scroll key: {below}" + ); + let above = above_marker(1).expect("clipped above is marked"); + assert!( + above.contains("1 more line above"), + "a single hidden line reads singular: {above}" + ); +} + +#[test] +fn a_tall_batch_renders_the_overflow_marker_and_scrolls() { + let mut app = app_with_many_annotations(40); + app.open_submit_forge(); + + // Fresh open: clipped below, nothing above. + let first = render_modal(&app, 90, 24); + assert!( + first.contains("more lines"), + "a clipped batch must say how much is hidden: {first}" + ); + assert!( + !first.contains("above"), + "nothing is hidden above at the top of the batch: {first}" + ); + + // Scrolling down moves the window: the clamped offset advances and the + // top marker appears. + for _ in 0..5 { + app.submit_forge_scroll_down(); + } + let scrolled = render_modal(&app, 90, 24); + assert_eq!(app.submit_forge.as_ref().unwrap().scroll.get(), 5); + assert!( + scrolled.contains("more lines above"), + "scrolled down, the lines above must be marked: {scrolled}" + ); + + // An overshoot is clamped to the last page by the render, and the bottom + // marker goes away because nothing is left below. + app.submit_forge.as_ref().unwrap().scroll.set(u16::MAX); + let bottom = render_modal(&app, 90, 24); + let offset = app.submit_forge.as_ref().unwrap().scroll.get(); + assert!(offset > 0 && offset < u16::MAX, "clamped offset: {offset}"); + assert!( + !bottom.contains("to scroll"), + "at the bottom there is nothing more to scroll to: {bottom}" + ); + // Re-rendering at the clamped offset is stable (the clamp is idempotent). + render_modal(&app, 90, 24); + assert_eq!(app.submit_forge.as_ref().unwrap().scroll.get(), offset); +} + +#[test] +fn a_short_batch_neither_scrolls_nor_marks_overflow() { + let mut app = app_with_many_annotations(1); + app.open_submit_forge(); + // A stale/overshooting offset must not scroll content that already fits. + app.submit_forge.as_ref().unwrap().scroll.set(u16::MAX); + let content = render_modal(&app, 90, 24); + assert_eq!(app.submit_forge.as_ref().unwrap().scroll.get(), 0); + assert!( + !content.contains("more line"), + "everything fits, so no overflow marker: {content}" + ); + assert!(content.contains("comment number 0")); +} + +#[test] +fn reopening_the_modal_starts_at_the_top() { + let mut app = app_with_many_annotations(40); + app.open_submit_forge(); + app.submit_forge_page_down(); + render_modal(&app, 90, 24); + assert!(app.submit_forge.as_ref().unwrap().scroll.get() > 0); + app.close_submit_forge(); + app.open_submit_forge(); + assert_eq!( + app.submit_forge.as_ref().unwrap().scroll.get(), + 0, + "a fresh open starts at the top of the batch" + ); +} + +#[test] +fn scroll_keys_move_the_preview_while_printable_keys_still_type_the_summary() { + let mut app = app_with_many_annotations(40); + app.open_submit_forge(); + // Record a viewport so PageDown has a real page to move by. + render_modal(&app, 90, 24); + + // Down scrolls and leaves the summary alone. + handle_submit_forge_key(&mut app, KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + let state = app.submit_forge.as_ref().unwrap(); + assert_eq!(state.scroll.get(), 1); + assert_eq!( + state.summary, "", + "a scroll key must not type into the summary" + ); + + // `j`/`k` belong to the summary, not to the scroll — they must type. + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::Char('j'), KeyModifiers::NONE), + ); + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::Char('k'), KeyModifiers::NONE), + ); + let state = app.submit_forge.as_ref().unwrap(); + assert_eq!(state.summary, "jk"); + assert_eq!(state.scroll.get(), 1, "typing must not move the preview"); + + // PageDown pages by the recorded viewport, Up steps back. + let page = app.submit_forge.as_ref().unwrap().viewport.get(); + assert!(page > 1, "the render must record a real viewport"); + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::PageDown, KeyModifiers::NONE), + ); + assert_eq!(app.submit_forge.as_ref().unwrap().scroll.get(), 1 + page); + handle_submit_forge_key(&mut app, KeyEvent::new(KeyCode::PageUp, KeyModifiers::NONE)); + assert_eq!(app.submit_forge.as_ref().unwrap().scroll.get(), 1); + handle_submit_forge_key(&mut app, KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + assert_eq!(app.submit_forge.as_ref().unwrap().scroll.get(), 0); +} + +#[test] +fn a_blocked_confirm_scrolls_its_hint_into_view_on_a_tall_batch() { + let mut app = app_with_many_annotations(40); + app.open_submit_forge(); + render_modal(&app, 90, 24); + // Select request-changes and confirm with no summary: blocked, hinted. + app.submit_forge_verdict_next(); + app.submit_forge_verdict_next(); + app.submit_forge_confirm(); + assert_eq!(app.mode, Mode::SubmitForge); + + // The hint is the last line of the content, so the modal must be showing + // its bottom for the reviewer to read why the confirm did nothing. + let content = render_modal(&app, 90, 24); + assert!( + app.submit_forge.as_ref().unwrap().scroll.get() > 0, + "the hint's own line was off-screen and must be scrolled to" + ); + assert!(content.contains("needs a summary"), "hint not visible"); +} + #[test] fn confirm_without_a_live_submitter_backend_sends_nothing() { let mut app = github_review_app(&["src/a.rs"]); diff --git a/src/ui/modal_keys.rs b/src/ui/modal_keys.rs index cfc8aa4..c16d510 100644 --- a/src/ui/modal_keys.rs +++ b/src/ui/modal_keys.rs @@ -1244,11 +1244,13 @@ pub(super) static THREAD_VIEW_KEYS: LazyLock> /// What a control key does in the submit-review modal /// ([`super::app::Mode::SubmitForge`]): confirm the publish, cancel it, cycle -/// the verdict picker, or delete a summary character. Free-text like -/// Compose/Search — every printable char extends the summary (a hand-written -/// fallback in [`super::modes::handle_submit_forge_key`], never remappable) — -/// so this table documents only the control keys. Not config-remappable yet; -/// see the module doc. +/// the verdict picker, scroll the batch preview, or delete a summary +/// character. Free-text like Compose/Search — every printable char extends the +/// summary (a hand-written fallback in +/// [`super::modes::handle_submit_forge_key`], never remappable) — so this +/// table documents only the control keys, and the scroll keys are deliberately +/// the arrow/page keys rather than `j`/`k`, which belong to the summary. Not +/// config-remappable yet; see the module doc. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum SubmitForgeAction { /// Publishes the previewed batch (see @@ -1261,6 +1263,14 @@ pub(super) enum SubmitForgeAction { VerdictNext, /// Cycles the verdict picker backward. VerdictPrev, + /// Scrolls the batch preview down one line. + ScrollDown, + /// Scrolls the batch preview up one line. + ScrollUp, + /// Scrolls the batch preview down a full viewport. + PageDown, + /// Scrolls the batch preview up a full viewport. + PageUp, /// Deletes the last summary character. DeleteChar, } @@ -1303,6 +1313,33 @@ pub(super) static SUBMIT_FORGE_KEYS: LazyLock app.close_submit_forge(), SubmitForgeAction::VerdictNext => app.submit_forge_verdict_next(), SubmitForgeAction::VerdictPrev => app.submit_forge_verdict_prev(), + SubmitForgeAction::ScrollDown => app.submit_forge_scroll_down(), + SubmitForgeAction::ScrollUp => app.submit_forge_scroll_up(), + SubmitForgeAction::PageDown => app.submit_forge_page_down(), + SubmitForgeAction::PageUp => app.submit_forge_page_up(), SubmitForgeAction::DeleteChar => app.submit_forge_delete_char(), } } From e6849242f4468f6050a74eefbe87a82429f1c6a9 Mon Sep 17 00:00:00 2001 From: redquill test Date: Thu, 30 Jul 2026 00:17:05 -0500 Subject: [PATCH 2/6] feat(ui): name reply targets in the submit preview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replies in the submit-review modal previewed as `thread : ` — a raw numeric id is meaningless at confirm time. Resolve each reply's target from the fetched thread overlay instead: root author and anchor (`path:line`, or `path (file-level)` once outdated), same conventions the thread overlay itself uses. Falls back to the id form only when the thread has dropped out of the overlay (e.g. a failed refresh). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AWX3hBxudUxZooeph9ngEB --- src/ui/forge_submit.rs | 60 ++++++++++++++++--- src/ui/forge_submit_tests.rs | 109 ++++++++++++++++++++++++++++++++++- 2 files changed, 160 insertions(+), 9 deletions(-) diff --git a/src/ui/forge_submit.rs b/src/ui/forge_submit.rs index 9457e98..7bd6236 100644 --- a/src/ui/forge_submit.rs +++ b/src/ui/forge_submit.rs @@ -25,7 +25,10 @@ use ratatui::text::{Line, Span}; use ratatui::widgets::{Block, Borders, Clear, Paragraph}; use crate::annotate::{Annotation, Classification, Target}; -use crate::forge::{Capabilities, SubmitBatch, SubmitReplyItem, SubmitReport, Verdict}; +use crate::forge::{ + Capabilities, SubmitBatch, SubmitReplyItem, SubmitReport, ThreadAnchor, ThreadOverlayStore, + Verdict, +}; use crate::review::store::ForgeProviderKind; use super::app::{App, Mode}; @@ -286,14 +289,35 @@ pub(super) struct FileGroup { pub(super) items: Vec, } -/// One drafted reply's preview row: which thread it answers and its body -/// summary. +/// Who and where a reply's thread is, resolved from the fetched thread +/// overlay — `None` when the thread is no longer present there (e.g. a +/// failed refresh dropped it), in which case the preview falls back to a +/// bare thread id. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) struct ReplyTarget { + pub(super) author: String, + pub(super) anchor: String, +} + +/// One drafted reply's preview row: which thread it answers, its resolved +/// target (when still known), and its body summary. #[derive(Debug, Clone, PartialEq, Eq)] pub(super) struct ReplyPreview { pub(super) thread_id: u64, + pub(super) target: Option, pub(super) summary: String, } +/// A thread's anchor as a display label — same conventions as +/// [`super::forge_threads`]'s thread-overlay render: `path:line` for a still- +/// live position, `path (file-level)` once the position is outdated. +fn thread_anchor_label(anchor: &ThreadAnchor) -> String { + match anchor { + ThreadAnchor::Position { path, line, .. } => format!("{path}:{line}"), + ThreadAnchor::File { path } => format!("{path} (file-level)"), + } +} + /// The whole batch preview, grouped by file with replies listed separately /// (a reply answers a thread, not a diff line). Pure — built from the /// unpublished annotation/reply sets — so grouping and labels are unit-tested @@ -328,10 +352,13 @@ fn first_line(body: &str) -> String { /// Builds the grouped preview from the unpublished annotations and replies — /// annotations grouped by file in first-seen order, replies in insertion -/// order. Pure; the caller passes the already-filtered unpublished sets. +/// order. Pure; the caller passes the already-filtered unpublished sets and a +/// thread overlay reference to resolve each reply's target (a read-only +/// lookup, so the function stays pure and unit-testable without a modal). pub(super) fn build_preview<'a>( annotations: impl Iterator, replies: impl Iterator, + threads: &ThreadOverlayStore, ) -> SubmitPreview { let mut groups: Vec = Vec::new(); for annotation in annotations { @@ -351,9 +378,16 @@ pub(super) fn build_preview<'a>( } } let replies = replies - .map(|(thread_id, body)| ReplyPreview { - thread_id, - summary: first_line(body), + .map(|(thread_id, body)| { + let target = threads.find(thread_id).map(|thread| ReplyTarget { + author: thread.root.author.clone(), + anchor: thread_anchor_label(&thread.anchor), + }); + ReplyPreview { + thread_id, + target, + summary: first_line(body), + } }) .collect(); SubmitPreview { groups, replies } @@ -809,8 +843,17 @@ fn build_lines( .add_modifier(Modifier::BOLD), ))); for reply in &preview.replies { + let label = match &reply.target { + Some(target) => format!( + "to {} @ {} \u{2014} {}", + target.author, target.anchor, reply.summary + ), + // The thread dropped out of the overlay (e.g. a failed + // refresh) — fall back to the id, the only thing still known. + None => format!("thread {}: {}", reply.thread_id, reply.summary), + }; lines.push(Line::from(Span::styled( - format!(" \u{21b3} thread {}: {}", reply.thread_id, reply.summary), + format!(" \u{21b3} {label}"), Style::default().fg(theme.annotation_text), ))); } @@ -869,6 +912,7 @@ pub fn render(frame: &mut Frame, area: Rect, app: &App) { app.replies .unpublished() .map(|r| (r.thread_id, r.body.as_str())), + &app.thread_overlay, ); let block = Block::default() diff --git a/src/ui/forge_submit_tests.rs b/src/ui/forge_submit_tests.rs index a34420e..0aedd30 100644 --- a/src/ui/forge_submit_tests.rs +++ b/src/ui/forge_submit_tests.rs @@ -4,7 +4,9 @@ use ratatui::backend::TestBackend; use crate::annotate::{Classification, Side, Target}; use crate::diff::FileDiff; -use crate::forge::{SubmitReport, Verdict}; +use crate::forge::{ + SubmitReport, Thread, ThreadAnchor, ThreadComment, ThreadOverlayStore, Verdict, +}; use crate::git::{DiffTarget, RawFilePatch}; use crate::review::store::{ForgeMetadata, ForgeProviderKind}; @@ -136,6 +138,7 @@ fn preview_groups_annotations_by_file_and_lists_replies_separately() { app.replies .unpublished() .map(|r| (r.thread_id, r.body.as_str())), + &ThreadOverlayStore::new(), ); // a.rs group has the line comment then the file comment; b.rs has the @@ -165,6 +168,74 @@ fn preview_groups_annotations_by_file_and_lists_replies_separately() { assert_eq!(preview.replies[0].summary, "agreed"); } +/// A positioned thread rooted by `author`, for reply-preview target +/// resolution (mirrors `forge_threads_tests::positioned_thread`). +fn positioned_thread(id: u64, author: &str, path: &str, line: u32) -> Thread { + Thread { + id, + anchor: ThreadAnchor::Position { + path: path.to_string(), + side: Side::New, + line, + }, + root: ThreadComment { + id, + author: author.to_string(), + created_at: "2026-07-01T10:00:00Z".to_string(), + body: "root comment".to_string(), + }, + replies: Vec::new(), + resolved: false, + outdated: false, + discussion_id: None, + } +} + +#[test] +fn reply_preview_names_the_root_author_and_anchor_when_the_thread_is_known() { + let mut app = github_review_app(&["src/a.rs"]); + app.replies.add(100, "agreed").unwrap(); + let mut threads = ThreadOverlayStore::new(); + threads.replace(vec![positioned_thread(100, "alice", "src/a.rs", 12)]); + + let preview = build_preview( + app.annotations.unpublished(), + app.replies + .unpublished() + .map(|r| (r.thread_id, r.body.as_str())), + &threads, + ); + + let target = preview.replies[0] + .target + .as_ref() + .expect("the thread is present in the overlay"); + assert_eq!(target.author, "alice"); + assert_eq!(target.anchor, "src/a.rs:12"); +} + +#[test] +fn reply_preview_falls_back_to_the_thread_id_when_the_thread_is_missing() { + // Simulates a failed refresh dropping the thread from the overlay: the + // reply is still drafted, but its author/anchor are no longer known. + let mut app = github_review_app(&["src/a.rs"]); + app.replies.add(100, "agreed").unwrap(); + let threads = ThreadOverlayStore::new(); + + let preview = build_preview( + app.annotations.unpublished(), + app.replies + .unpublished() + .map(|r| (r.thread_id, r.body.as_str())), + &threads, + ); + + assert!( + preview.replies[0].target.is_none(), + "no target once the thread is gone from the overlay" + ); +} + // -- open / not-a-forge-session no-op ---------------------------------------- #[test] @@ -560,6 +631,42 @@ fn render_modal(app: &App, width: u16, height: u16) -> String { .collect() } +// -- reply preview target labels (ENG-174) ----------------------------------- + +#[test] +fn a_reply_preview_renders_the_root_author_and_anchor_not_the_raw_id() { + let mut app = github_review_app(&["src/a.rs"]); + app.replies.add(100, "agreed").unwrap(); + app.thread_overlay + .replace(vec![positioned_thread(100, "alice", "src/a.rs", 12)]); + app.open_submit_forge(); + + let rendered = render_modal(&app, 90, 24); + assert!( + rendered.contains("to alice @ src/a.rs:12"), + "the reply must name the root author and anchor: {rendered}" + ); + assert!( + !rendered.contains("thread 100:"), + "a resolved thread must not fall back to the raw id: {rendered}" + ); +} + +#[test] +fn a_reply_preview_falls_back_to_the_thread_id_once_the_thread_drops_out_of_the_overlay() { + // The overlay is empty — e.g. a failed refresh dropped the thread — so + // there is no author/anchor left to show. + let mut app = github_review_app(&["src/a.rs"]); + app.replies.add(100, "agreed").unwrap(); + app.open_submit_forge(); + + let rendered = render_modal(&app, 90, 24); + assert!( + rendered.contains("thread 100: agreed"), + "with no thread in the overlay, the id form is the only honest label: {rendered}" + ); +} + /// A review with `n` annotations spread over three files — enough rows to /// overflow any sensibly sized modal. fn app_with_many_annotations(n: usize) -> App { From b1be2d78a21a3b41d9ea85d9bed9b0010c250d60 Mon Sep 17 00:00:00 2001 From: redquill test Date: Thu, 30 Jul 2026 00:36:38 -0500 Subject: [PATCH 3/6] feat(ui): compose multi-line review summaries The submit modal's summary was a single-line push/pop field, so the substantive body of a forge review was capped at one line with no cursor motion or word-delete. `Ctrl-e` now hands it to the Compose editor, seeded with the text so far; saving returns to the modal with the whole body, cancelling leaves it as it was. The field shows the summary's first line plus a dim count of the lines it keeps off screen, and once the summary is multi-line the in-modal push/pop gestures step aside for `Ctrl-e` rather than silently mutate a line the reviewer can't see. Direct typing of a one-line summary is unchanged, as is Enter to confirm. Compose's two-mode `thread_id: Option` discriminant becomes a three-variant `ComposeKind`, so no combination of flags can describe a compose that is an annotation and a summary at once. The change is folded in here rather than split out because the third variant is the feature. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AWX3hBxudUxZooeph9ngEB --- src/ui/annotation_list.rs | 2 +- src/ui/app.rs | 23 +++- src/ui/compose.rs | 71 +++++++--- src/ui/compose_modal.rs | 16 ++- src/ui/forge_submit.rs | 126 ++++++++++++++--- src/ui/forge_submit_tests.rs | 251 +++++++++++++++++++++++++++++++++- src/ui/forge_threads_tests.rs | 2 +- src/ui/modal_keys.rs | 19 ++- src/ui/modes.rs | 4 +- 9 files changed, 462 insertions(+), 52 deletions(-) diff --git a/src/ui/annotation_list.rs b/src/ui/annotation_list.rs index 93e39c5..45f9796 100644 --- a/src/ui/annotation_list.rs +++ b/src/ui/annotation_list.rs @@ -703,7 +703,7 @@ index 111..222 100644 assert_eq!(app.mode, Mode::Compose); let compose = app.compose.as_ref().unwrap(); - assert_eq!(compose.thread_id, Some(77)); + assert_eq!(compose.thread_id(), Some(77)); assert_eq!(compose.buffer.text(), "original reply"); } diff --git a/src/ui/app.rs b/src/ui/app.rs index 1ad36f6..e67eb77 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -24,7 +24,7 @@ use crate::review::ReviewStatus; use super::background::{BackgroundTasks, CommandOutcome, TaskId, run_command}; use super::command_log::{CommandLog, CommandLogEntry}; use super::commit_message::CommitMessageState; -use super::compose::ComposeState; +use super::compose::{ComposeKind, ComposeState}; use super::diff_view_state::DiffViewState; use super::editor::EditorLaunch; use super::file_finder::{FinderState, InFlightFinderLoad}; @@ -1557,10 +1557,15 @@ impl App { } } - /// Cancels Compose without saving, discarding the draft. + /// Cancels Compose without saving, discarding the draft. A summary compose + /// returns to the submit modal it was opened from, leaving the summary as + /// it was. pub fn cancel_compose(&mut self) { - self.compose = None; - self.mode = Mode::Normal; + let kind = self.compose.take().map(|c| c.kind); + self.mode = match kind { + Some(ComposeKind::ReviewSummary) if self.submit_forge.is_some() => Mode::SubmitForge, + _ => Mode::Normal, + }; } /// Submits the Compose draft: adds a new annotation, or (when editing) @@ -1568,18 +1573,26 @@ impl App { /// whitespace-only body cancels instead — the store rejects empty /// bodies, and surfacing that as a hard error over "just cancel" would /// be needless friction for a body the reviewer clearly abandoned. + /// + /// A summary compose is the exception to that rule: it writes back to the + /// submit modal's own state (no store involved), where an emptied buffer is + /// a deliberate "clear the summary" rather than an abandoned draft. pub fn submit_compose(&mut self) { let Some(compose) = self.compose.take() else { self.mode = Mode::Normal; return; }; let body = compose.buffer.text(); + if compose.kind == ComposeKind::ReviewSummary { + self.save_review_summary(&body); + return; + } if body.trim().is_empty() { self.mode = Mode::Normal; return; } - if let Some(thread_id) = compose.thread_id { + if let Some(thread_id) = compose.thread_id() { // Reply mode: add or edit a draft reply rather than an annotation. // Replies never carry a classification and never reach stdout. match compose.editing_id { diff --git a/src/ui/compose.rs b/src/ui/compose.rs index 95c36b3..66a9334 100644 --- a/src/ui/compose.rs +++ b/src/ui/compose.rs @@ -5,12 +5,32 @@ use crate::annotate::{Classification, Target}; -/// The Compose modal's state while open: the target being annotated, the -/// currently selected classification (`Ctrl-t` cycles it), the text being -/// edited, and — when editing an existing annotation rather than creating a -/// new one — the id to write back to on submit. +/// What a Compose session is editing, and therefore where its text goes on +/// submit. One discriminant for all three so no combination of flags can +/// describe a compose that is two things at once. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ComposeKind { + /// An annotation on the state's target — the vast majority of opens. + /// Submit adds to (or edits in) the annotation store. + Annotation, + /// A reply to the imported PR thread whose root comment id is given. + /// Submit adds to (or edits in) the draft-reply store; the modal renders a + /// reply header rather than a target/classification title. + Reply { thread_id: u64 }, + /// The submit modal's review summary. Submit writes the text straight back + /// into the open submit state and returns to that modal — nothing reaches + /// any store, so a summary is never an annotation and never a reply. + ReviewSummary, +} + +/// The Compose modal's state while open: what it's editing ([`ComposeKind`]), +/// the target being annotated, the currently selected classification (`Ctrl-t` +/// cycles it), the text being edited, and — when editing an existing item +/// rather than creating a new one — the id to write back to on submit. #[derive(Debug, Clone, PartialEq)] pub struct ComposeState { + /// What this compose is editing, and where submit routes its text. + pub kind: ComposeKind, /// What the annotation being composed is attached to. pub target: Target, /// The annotation's current classification. @@ -18,16 +38,11 @@ pub struct ComposeState { /// The body text being edited. pub buffer: TextBuffer, /// `Some(id)` when editing an existing item (submit updates it in place); - /// `None` when composing a new one (submit adds). In reply mode - /// ([`thread_id`](Self::thread_id) is `Some`) the id is the draft reply's - /// id; otherwise it is the annotation's id. + /// `None` when composing a new one (submit adds). In + /// [`ComposeKind::Reply`] the id is the draft reply's id; otherwise it is + /// the annotation's id. Always `None` for + /// [`ComposeKind::ReviewSummary`], which has no store item to update. pub editing_id: Option, - /// `Some(thread_root_id)` when this compose is drafting a reply to an - /// imported PR thread rather than an annotation — `submit_compose` - /// branches on it, and the modal renders a reply header instead of a - /// target/classification title. `None` for the ordinary annotation - /// compose (the vast majority of opens). - pub thread_id: Option, } impl ComposeState { @@ -35,11 +50,11 @@ impl ComposeState { /// buffer and `Classification::Issue` as the default. pub fn new(target: Target) -> ComposeState { ComposeState { + kind: ComposeKind::Annotation, target, classification: Classification::Issue, buffer: TextBuffer::new(), editing_id: None, - thread_id: None, } } @@ -52,11 +67,11 @@ impl ComposeState { body: &str, ) -> ComposeState { ComposeState { + kind: ComposeKind::Annotation, target, classification, buffer: TextBuffer::from_str(body), editing_id: Some(id), - thread_id: None, } } @@ -66,11 +81,11 @@ impl ComposeState { /// carry harmless defaults the modal never renders. pub fn reply(thread_id: u64) -> ComposeState { ComposeState { + kind: ComposeKind::Reply { thread_id }, target: Target::file(String::new()), classification: Classification::Issue, buffer: TextBuffer::new(), editing_id: None, - thread_id: Some(thread_id), } } @@ -78,11 +93,33 @@ impl ComposeState { /// `thread_id`, pre-filled with its body. pub fn editing_reply(reply_id: usize, thread_id: u64, body: &str) -> ComposeState { ComposeState { + kind: ComposeKind::Reply { thread_id }, target: Target::file(String::new()), classification: Classification::Issue, buffer: TextBuffer::from_str(body), editing_id: Some(reply_id), - thread_id: Some(thread_id), + } + } + + /// Starts editing the submit modal's review summary, seeded with `body` + /// (the summary as it stands). The `target`/`classification` are inert + /// placeholders, as in reply mode — a review summary has no diff anchor + /// and no classification. + pub fn review_summary(body: &str) -> ComposeState { + ComposeState { + kind: ComposeKind::ReviewSummary, + target: Target::file(String::new()), + classification: Classification::Issue, + buffer: TextBuffer::from_str(body), + editing_id: None, + } + } + + /// The thread this compose replies to, or `None` for every other kind. + pub fn thread_id(&self) -> Option { + match self.kind { + ComposeKind::Reply { thread_id } => Some(thread_id), + ComposeKind::Annotation | ComposeKind::ReviewSummary => None, } } } diff --git a/src/ui/compose_modal.rs b/src/ui/compose_modal.rs index bc617e9..e696eaf 100644 --- a/src/ui/compose_modal.rs +++ b/src/ui/compose_modal.rs @@ -11,6 +11,7 @@ use ratatui::widgets::{Block, Borders, Clear, Paragraph}; use crate::annotate::{Side, Target}; use super::app::App; +use super::compose::ComposeKind; use super::textwrap; /// The horizontal slice a 60%-wide modal occupies within `area` (full height, @@ -87,10 +88,11 @@ pub fn render(frame: &mut Frame, area: Rect, app: &App) { }; // Reply mode renders a thread-anchored header (no classification, since a - // reply answers a thread rather than classifying a diff line); the - // ordinary annotation compose keeps its target/classification title. - let (title, footer) = match compose.thread_id { - Some(thread_id) => { + // reply answers a thread rather than classifying a diff line), and summary + // mode names the review it belongs to; the ordinary annotation compose + // keeps its target/classification title. + let (title, footer) = match compose.kind { + ComposeKind::Reply { thread_id } => { let where_ = app .thread_overlay .find(thread_id) @@ -106,7 +108,11 @@ pub fn render(frame: &mut Frame, area: Rect, app: &App) { " Enter submit Shift-Enter/Ctrl-j newline Esc cancel ", ) } - None => ( + ComposeKind::ReviewSummary => ( + "review summary".to_string(), + " Enter save Shift-Enter/Ctrl-j newline Esc cancel ", + ), + ComposeKind::Annotation => ( format!( "{} — {}", target_label(&compose.target), diff --git a/src/ui/forge_submit.rs b/src/ui/forge_submit.rs index 7bd6236..7970bac 100644 --- a/src/ui/forge_submit.rs +++ b/src/ui/forge_submit.rs @@ -3,7 +3,10 @@ //! forge PR only) opens [`Mode::SubmitForge`] — a grouped-by-file preview of //! every unpublished annotation and drafted reply, a capability-driven verdict //! picker, and an optional summary — and **nothing is sent until the reviewer -//! confirms from here** (the spec's safety boundary). On confirm the batch is +//! confirms from here** (the spec's safety boundary). The summary takes a line +//! typed straight into the modal, or a whole body written in the Compose editor +//! (`Ctrl-e`, [`super::compose::ComposeKind::ReviewSummary`]), of which the +//! field shows the first line and a count of the rest. On confirm the batch is //! resolved on the render thread and handed to a background submit sequence //! (see [`crate::forge::run_submit_sequence`]) through the fake-able //! [`super::stage_ops::AsyncForgeSubmitter`] seam, single-flight so a second @@ -53,6 +56,11 @@ pub(super) struct InFlightSubmit { pub(super) struct SubmitForgeState { pub(super) verdicts: Vec, pub(super) verdict_index: usize, + /// The review body, possibly multi-line: short ones are typed straight + /// into the modal, longer ones edited in Compose via `Ctrl-e`. Lives here + /// and nowhere else, so it lasts exactly as long as the modal does — a + /// `Ctrl-e` round-trip keeps it (the state stays open behind the editor), + /// an `Esc` close discards it, and a fresh open starts empty. pub(super) summary: String, pub(super) target_line: String, /// An honest one-line disclosure of the provider's submit shape, shown @@ -82,8 +90,19 @@ impl SubmitForgeState { .copied() .unwrap_or(Verdict::Comment) } + + /// Whether the summary spans more than one line — the modal shows only its + /// first, so the in-modal push/pop editing gestures step aside for + /// `Ctrl-e` rather than mutate a line the reviewer can't see. + fn summary_is_multi_line(&self) -> bool { + self.summary.contains('\n') + } } +/// The hint shown when a direct edit gesture lands on a multi-line summary, +/// which only the editor can edit safely. +const MULTI_LINE_SUMMARY_HINT: &str = "Ctrl-e to edit multi-line summary"; + /// One frame's scroll geometry for the modal body: the clamped offset, the /// body's height, and how many rendered lines are hidden above and below it. /// Pure, so the clamp and the overflow decision are unit-tested without @@ -128,7 +147,7 @@ pub(super) fn resolve_scroll(total: u16, inner_height: u16, requested: u16) -> S } /// `s` when `n` isn't one, for the marker lines' line count. -fn plural(n: u16) -> &'static str { +fn plural(n: usize) -> &'static str { if n == 1 { "" } else { "s" } } @@ -137,14 +156,38 @@ fn below_marker(hidden: u16) -> Option { (hidden > 0).then(|| { format!( "\u{25be} {hidden} more line{} \u{2014} \u{2193} to scroll", - plural(hidden) + plural(usize::from(hidden)) ) }) } /// The clipped-above marker, or `None` when the first line is on screen. fn above_marker(hidden: u16) -> Option { - (hidden > 0).then(|| format!("\u{25b4} {hidden} more line{} above", plural(hidden))) + (hidden > 0).then(|| { + format!( + "\u{25b4} {hidden} more line{} above", + plural(usize::from(hidden)) + ) + }) +} + +/// The summary's first line — the only one the modal shows, since the field is +/// one line tall and the batch preview owns the vertical space. +fn summary_first_line(summary: &str) -> &str { + summary.lines().next().unwrap_or("") +} + +/// The suffix disclosing the summary lines the field doesn't show, or `None` +/// for a summary that fits on its one line. The count is what stays hidden, so +/// the whole body is never silently understated. +fn summary_overflow_note(summary: &str) -> Option { + let hidden = summary.lines().count().saturating_sub(1); + (hidden > 0).then(|| { + format!( + "({hidden} more line{} \u{2014} Ctrl-e to edit)", + plural(hidden) + ) + }) } /// Splits `line` into rows no wider than `width`, preserving each character's @@ -492,21 +535,61 @@ impl App { } /// Appends a typed character to the summary (the modal's free-text field). + /// Once the summary is multi-line the field is read-only — see + /// [`SubmitForgeState::summary_is_multi_line`] — and the keystroke leaves + /// the `Ctrl-e` hint instead. pub(super) fn submit_forge_insert_char(&mut self, c: char) { if let Some(state) = self.submit_forge.as_mut() { + if state.summary_is_multi_line() { + state.hint = Some(MULTI_LINE_SUMMARY_HINT.to_string()); + return; + } state.summary.push(c); state.hint = None; } } - /// Deletes the last summary character. + /// Deletes the last summary character, or hints toward `Ctrl-e` on a + /// multi-line summary (a pop from an off-screen line is invisible). pub(super) fn submit_forge_delete_char(&mut self) { if let Some(state) = self.submit_forge.as_mut() { + if state.summary_is_multi_line() { + state.hint = Some(MULTI_LINE_SUMMARY_HINT.to_string()); + return; + } state.summary.pop(); state.hint = None; } } + /// `Ctrl-e`: hands the summary to the Compose editor, seeded with the text + /// as it stands, for full multi-line editing. The submit state is left + /// open behind the editor, so saving or cancelling comes straight back to + /// this modal with the batch and verdict untouched. + pub(super) fn open_summary_compose(&mut self) { + let Some(state) = self.submit_forge.as_ref() else { + return; + }; + self.compose = Some(super::compose::ComposeState::review_summary(&state.summary)); + self.mode = Mode::Compose; + } + + /// Writes an edited summary back from Compose and returns to the submit + /// modal. An emptied buffer clears the summary — in the editor that is a + /// deliberate erase, not an abandoned draft. Defensive fallback to Normal + /// if the modal is somehow no longer open, since there'd be nowhere to + /// return to. + pub(super) fn save_review_summary(&mut self, body: &str) { + match self.submit_forge.as_mut() { + Some(state) => { + state.summary = body.trim().to_string(); + state.hint = None; + self.mode = Mode::SubmitForge; + } + None => self.mode = Mode::Normal, + } + } + /// The confirm gesture: reads the chosen verdict + summary, closes the /// modal, and spawns the background submit. The only path that ever begins /// a forge write. @@ -859,25 +942,32 @@ fn build_lines( } } - // Summary field. + // Summary field: its first line, plus a dim count of the lines a + // multi-line summary keeps off screen. lines.push(Line::from(String::new())); - let summary_display = if state.summary.is_empty() { - Span::styled( - "(optional summary \u{2014} type to add)", + let mut summary_spans = vec![Span::styled("Summary: ", Style::default().fg(theme.gutter))]; + if state.summary.is_empty() { + summary_spans.push(Span::styled( + "(optional \u{2014} type, or Ctrl-e for multi-line)", Style::default() .fg(theme.gutter) .add_modifier(Modifier::DIM), - ) + )); } else { - Span::styled( - state.summary.clone(), + summary_spans.push(Span::styled( + summary_first_line(&state.summary).to_string(), Style::default().fg(theme.annotation_text), - ) - }; - lines.push(Line::from(vec![ - Span::styled("Summary: ", Style::default().fg(theme.gutter)), - summary_display, - ])); + )); + if let Some(note) = summary_overflow_note(&state.summary) { + summary_spans.push(Span::styled( + format!(" {note}"), + Style::default() + .fg(theme.gutter) + .add_modifier(Modifier::DIM), + )); + } + } + lines.push(Line::from(summary_spans)); // A blocked-confirm validation hint (e.g. request-changes with no summary). if let Some(hint) = &state.hint { diff --git a/src/ui/forge_submit_tests.rs b/src/ui/forge_submit_tests.rs index 0aedd30..64657e5 100644 --- a/src/ui/forge_submit_tests.rs +++ b/src/ui/forge_submit_tests.rs @@ -11,7 +11,8 @@ use crate::git::{DiffTarget, RawFilePatch}; use crate::review::store::{ForgeMetadata, ForgeProviderKind}; use super::super::app::{App, Mode}; -use super::super::modes::handle_submit_forge_key; +use super::super::compose::ComposeKind; +use super::super::modes::{handle_compose_key, handle_submit_forge_key}; use super::*; // -- fixtures ---------------------------------------------------------------- @@ -876,6 +877,254 @@ fn a_blocked_confirm_scrolls_its_hint_into_view_on_a_tall_batch() { assert!(content.contains("needs a summary"), "hint not visible"); } +// -- multi-line review summary via the composer (ENG-175) -------------------- + +/// Types `text` into the open composer through its real keymap: `Ctrl-j` for +/// each newline, a plain char for everything else. +fn type_into_compose(app: &mut App, text: &str) { + for c in text.chars() { + let key = if c == '\n' { + KeyEvent::new(KeyCode::Char('j'), KeyModifiers::CONTROL) + } else { + KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE) + }; + handle_compose_key(app, key); + } +} + +/// Writes `text` as the open modal's summary the way a reviewer does: `Ctrl-e` +/// into the composer, type, `Enter` to save. Assumes the summary starts empty. +fn compose_summary(app: &mut App, text: &str) { + handle_submit_forge_key( + app, + KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL), + ); + type_into_compose(app, text); + handle_compose_key(app, KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); +} + +/// A GitHub review with its submit modal open on `text` as the summary. +fn app_with_summary(text: &str) -> App { + let mut app = github_review_app(&["src/a.rs"]); + app.open_submit_forge(); + compose_summary(&mut app, text); + app +} + +#[test] +fn ctrl_e_composes_a_multi_line_summary_and_hands_it_back_to_the_modal() { + let mut app = github_review_app(&["src/a.rs"]); + app.open_submit_forge(); + // A line typed straight into the modal seeds the editor. + for c in "one".chars() { + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE), + ); + } + + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL), + ); + assert_eq!(app.mode, Mode::Compose); + let compose = app.compose.as_ref().expect("the composer opens"); + assert_eq!(compose.kind, ComposeKind::ReviewSummary); + assert_eq!( + compose.buffer.text(), + "one", + "the editor is seeded with the summary so far" + ); + + type_into_compose(&mut app, "\ntwo\nthree"); + handle_compose_key(&mut app, KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert_eq!( + app.mode, + Mode::SubmitForge, + "saving returns to the submit modal, not to Normal" + ); + assert!(app.compose.is_none()); + assert_eq!( + app.submit_forge.as_ref().unwrap().summary, + "one\ntwo\nthree", + "every line survives the round trip" + ); + // A summary is neither an annotation nor a reply. + assert_eq!(app.annotations.iter().count(), 0); + assert!(app.replies.is_empty()); +} + +#[test] +fn cancelling_the_summary_composer_leaves_the_summary_untouched() { + let mut app = app_with_summary("keep\nme"); + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL), + ); + type_into_compose(&mut app, "\nthrown away"); + handle_compose_key(&mut app, KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); + + assert_eq!( + app.mode, + Mode::SubmitForge, + "cancelling comes back to the submit modal" + ); + assert!(app.compose.is_none()); + assert_eq!(app.submit_forge.as_ref().unwrap().summary, "keep\nme"); +} + +#[test] +fn the_summary_field_shows_its_first_line_and_counts_the_rest() { + let single = render_modal(&app_with_summary("only line"), 90, 24); + assert!( + single.contains("Summary: only line"), + "a one-line summary shows whole: {single}" + ); + assert!( + !single.contains("Ctrl-e to edit"), + "nothing is hidden, so nothing is counted: {single}" + ); + + let two = render_modal(&app_with_summary("first\nsecond"), 90, 24); + assert!( + two.contains("first") && !two.contains("second"), + "only the first line is shown: {two}" + ); + assert!( + two.contains("(1 more line \u{2014} Ctrl-e to edit)"), + "one hidden line reads singular: {two}" + ); + + let three = render_modal(&app_with_summary("first\nsecond\nthird"), 90, 24); + assert!( + three.contains("(2 more lines \u{2014} Ctrl-e to edit)"), + "the hidden lines are counted, not the total: {three}" + ); +} + +#[test] +fn a_multi_line_summary_is_read_only_in_the_modal_and_points_at_ctrl_e() { + let mut app = app_with_summary("first\nsecond"); + + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::NONE), + ); + let state = app.submit_forge.as_ref().unwrap(); + assert_eq!( + state.summary, "first\nsecond", + "typing must not extend a line the field doesn't show" + ); + assert!( + state.hint.as_deref().is_some_and(|h| h.contains("Ctrl-e")), + "the refusal must name the way in: {:?}", + state.hint + ); + + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE), + ); + let state = app.submit_forge.as_ref().unwrap(); + assert_eq!( + state.summary, "first\nsecond", + "backspace must not eat an off-screen character either" + ); + assert!(state.hint.is_some()); + + // And Ctrl-e still opens the editor from the refused state. + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL), + ); + assert_eq!(app.mode, Mode::Compose); +} + +#[test] +fn request_changes_accepts_a_multi_line_summary() { + let mut app = app_with_summary("needs work\n\n- fix the parser\n- add a test"); + app.submit_forge_verdict_next(); + app.submit_forge_verdict_next(); + assert_eq!( + app.submit_forge.as_ref().unwrap().verdict(), + Verdict::RequestChanges + ); + + app.submit_forge_confirm(); + assert_eq!( + app.mode, + Mode::Normal, + "a multi-line body satisfies the needs-a-summary rule" + ); + assert!(app.submit_forge.is_none(), "the confirm was not blocked"); +} + +/// A [`crate::forge::ForgeSubmitExecutor`] that records the review bodies it is +/// handed. A fake — no `gh`/`glab` is ever run. +#[derive(Default)] +struct RecordingSubmitter { + bodies: std::cell::RefCell>, +} + +impl crate::forge::ForgeSubmitExecutor for RecordingSubmitter { + fn submit_review( + &self, + payload: &crate::forge::ReviewPayload, + ) -> Result<(), crate::forge::ForgeError> { + self.bodies.borrow_mut().push(payload.body.clone()); + Ok(()) + } + + fn post_file_comment(&self, _path: &str, _body: &str) -> Result<(), crate::forge::ForgeError> { + Ok(()) + } + + fn post_reply(&self, _thread_id: u64, _body: &str) -> Result<(), crate::forge::ForgeError> { + Ok(()) + } +} + +#[test] +fn the_outgoing_review_payload_carries_every_summary_line() { + let summary = "needs work\n\n- fix the parser\n- add a test"; + let mut app = github_review_app(&["src/a.rs"]); + app.annotations + .add( + Target::line("src/a.rs", 2, Side::New), + Classification::Issue, + "fix", + ) + .unwrap(); + + let batch = app.build_submit_batch(Verdict::RequestChanges, Some(summary)); + assert_eq!( + batch.plan.payload.body, summary, + "the summary crosses into the payload whole" + ); + + let fake = RecordingSubmitter::default(); + let report = crate::forge::run_submit_sequence(&batch, &fake); + assert!(report.failure.is_none(), "{:?}", report.failure); + assert_eq!( + fake.bodies.borrow().as_slice(), + &[summary.to_string()], + "the submit sequence delivers every line, unsplit" + ); +} + +#[test] +fn the_summary_lives_with_the_modal_and_a_fresh_open_starts_empty() { + let mut app = app_with_summary("first\nsecond"); + app.close_submit_forge(); + app.open_submit_forge(); + assert_eq!( + app.submit_forge.as_ref().unwrap().summary, + "", + "the summary belongs to the modal that was cancelled, not to the session" + ); +} + #[test] fn confirm_without_a_live_submitter_backend_sends_nothing() { let mut app = github_review_app(&["src/a.rs"]); diff --git a/src/ui/forge_threads_tests.rs b/src/ui/forge_threads_tests.rs index 4386698..c4e1c50 100644 --- a/src/ui/forge_threads_tests.rs +++ b/src/ui/forge_threads_tests.rs @@ -814,7 +814,7 @@ fn thread_conversation_and_reply_journey_transcript() { // Draft a reply to the thread (r), type it, submit. app.open_reply_compose(); assert_eq!(app.mode, Mode::Compose); - assert_eq!(app.compose.as_ref().and_then(|c| c.thread_id), Some(1)); + assert_eq!(app.compose.as_ref().and_then(|c| c.thread_id()), Some(1)); if let Some(compose) = app.compose.as_mut() { for ch in "I'll take the empty-input guard.".chars() { compose.buffer.insert_char(ch); diff --git a/src/ui/modal_keys.rs b/src/ui/modal_keys.rs index c16d510..2789e32 100644 --- a/src/ui/modal_keys.rs +++ b/src/ui/modal_keys.rs @@ -1244,8 +1244,9 @@ pub(super) static THREAD_VIEW_KEYS: LazyLock> /// What a control key does in the submit-review modal /// ([`super::app::Mode::SubmitForge`]): confirm the publish, cancel it, cycle -/// the verdict picker, scroll the batch preview, or delete a summary -/// character. Free-text like Compose/Search — every printable char extends the +/// the verdict picker, scroll the batch preview, delete a summary character, or +/// hand the summary to the Compose editor for multi-line editing. +/// Free-text like Compose/Search — every printable char extends the /// summary (a hand-written fallback in /// [`super::modes::handle_submit_forge_key`], never remappable) — so this /// table documents only the control keys, and the scroll keys are deliberately @@ -1273,6 +1274,9 @@ pub(super) enum SubmitForgeAction { PageUp, /// Deletes the last summary character. DeleteChar, + /// Opens the Compose editor on the summary, for multi-line editing (see + /// [`super::app::App::open_summary_compose`]). + ComposeSummary, } /// The submit-review modal's control-key table, for the help overlay, footer @@ -1346,6 +1350,15 @@ pub(super) static SUBMIT_FORGE_KEYS: LazyLock app.submit_forge_page_down(), SubmitForgeAction::PageUp => app.submit_forge_page_up(), SubmitForgeAction::DeleteChar => app.submit_forge_delete_char(), + SubmitForgeAction::ComposeSummary => app.open_summary_compose(), } } From 3ad9fb50c8e5b96c2c3e89fe4d164ccfe4ca847d Mon Sep 17 00:00:00 2001 From: redquill test Date: Thu, 30 Jul 2026 02:12:08 -0500 Subject: [PATCH 4/6] feat(ui): show a per-item result view when a submit partially fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stopped submit left everything in one transient status line: honest about the counts, silent about which comments actually landed, so the only way to find out was to open the PR in a browser. The sequence now records what it set out to send (`SubmitAttempt`) into its report before it starts writing. Joined against the published and draft lists, that makes each item's fate knowable — published, pending draft, or never reached — rather than inferred from a diff of local state. A run that stops opens a read-only modal grouping every item under its outcome, named exactly as the submit preview named it (including the humanized "to @ :" reply labels), with the review itself leading the not-sent group when the verdict never landed and the diagnostic underneath. The one-line status is unchanged and still set in both cases; a submit that publishes everything opens no modal. Keys come from a new SUBMIT_RESULT_KEYS table: Enter/Esc/q dismiss, U reopens the submit modal to retry the remainder, and j/k/arrows plus the page keys scroll (no summary field here, so the letter keys are free, unlike the submit modal). Long lists scroll with the render-time clamp the help overlay uses. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AWX3hBxudUxZooeph9ngEB --- docs/forge-setup.md | 10 + src/forge/gitlab.rs | 20 +- src/forge/mod.rs | 3 +- src/forge/submit.rs | 110 ++++- src/ui/annotation_list.rs | 1 + src/ui/app.rs | 15 + src/ui/footer.rs | 1 + src/ui/forge_submit.rs | 92 +++-- src/ui/forge_submit_result.rs | 368 +++++++++++++++++ src/ui/forge_submit_result_tests.rs | 619 ++++++++++++++++++++++++++++ src/ui/forge_submit_tests.rs | 7 +- src/ui/git_panel.rs | 1 + src/ui/help.rs | 35 +- src/ui/mod.rs | 5 + src/ui/modal_keys.rs | 93 +++++ src/ui/modal_keys_config.rs | 1 + src/ui/modes.rs | 30 +- src/ui/stage_ops.rs | 4 + src/ui/staging.rs | 1 + 19 files changed, 1376 insertions(+), 40 deletions(-) create mode 100644 src/ui/forge_submit_result.rs create mode 100644 src/ui/forge_submit_result_tests.rs diff --git a/docs/forge-setup.md b/docs/forge-setup.md index 8608ba3..8f32235 100644 --- a/docs/forge-setup.md +++ b/docs/forge-setup.md @@ -174,6 +174,16 @@ arrow/page keys rather than `j`/`k`. A re-submit only ever sends what hasn't already published, so a partial failure never double-posts. +When a submit stops partway, the status line's counts are joined by a +read-only result view naming each item's fate: `✓ published`, `◌ pending +draft` (staged on GitLab, awaiting a publish), and `✗ not sent` — the last +group led by the review itself when the verdict never landed — with the +error underneath. Comments are named exactly as the submit modal named +them. `Enter`/`Esc`/`q` dismiss it, `U` goes straight back to the submit +modal to retry the remainder, and `j`/`k`/arrows and `PageUp`/`PageDown` +scroll a long list (no summary field here, so the letter keys are free). A +submit that publishes everything shows only the one-line status. + ## Security notes redquill never reads, stores, logs, or displays a forge token. The two diff --git a/src/forge/gitlab.rs b/src/forge/gitlab.rs index 6a87a91..50d1de0 100644 --- a/src/forge/gitlab.rs +++ b/src/forge/gitlab.rs @@ -61,7 +61,7 @@ use crate::annotate::Side; use super::diagnose::submit_error_headline; use super::process::{harden_glab, run_captured_with_timeout, run_with_input_and_timeout}; -use super::submit::SubmitReport; +use super::submit::{SubmitAttempt, SubmitReport}; use super::threads::{Thread, ThreadAnchor, ThreadComment}; use super::{ForgeError, PullRequest}; @@ -723,9 +723,25 @@ pub fn run_gitlab_submit_sequence( batch: &GitlabSubmitBatch, exec: &dyn GitlabSubmitExecutor, ) -> SubmitReport { - match try_draft_submit(batch, exec) { + let mut report = match try_draft_submit(batch, exec) { DraftAttempt::Completed(report) => report, DraftAttempt::Unavailable => run_visible_fallback(batch, exec), + }; + // Recorded once here rather than at each early return inside the two + // paths: the set a run sets out to send is a property of the batch, so one + // assignment can't drift out of step with a new stop point. + report.attempt = gitlab_attempt(batch); + report +} + +/// What a GitLab run sets out to send: every note (positioned annotations and +/// file comments alike), every reply whose discussion resolved, and the review +/// itself when the batch carries a summary or an approval. +fn gitlab_attempt(batch: &GitlabSubmitBatch) -> SubmitAttempt { + SubmitAttempt { + annotation_ids: batch.notes.iter().map(|n| n.annotation_id).collect(), + reply_ids: batch.replies.iter().map(|r| r.reply_id).collect(), + review_post: batch.summary.is_some() || batch.approve, } } diff --git a/src/forge/mod.rs b/src/forge/mod.rs index 9230a61..674466b 100644 --- a/src/forge/mod.rs +++ b/src/forge/mod.rs @@ -57,7 +57,8 @@ pub use gitlab::{ }; pub use remote_url::{Hostname, RemoteUrlError, parse_origin_hostname, parse_origin_repo_slug}; pub use submit::{ - ForgeSubmitExecutor, SubmitBatch, SubmitReplyItem, SubmitReport, run_submit_sequence, + ForgeSubmitExecutor, ItemOutcome, SubmitAttempt, SubmitBatch, SubmitReplyItem, SubmitReport, + run_submit_sequence, }; pub use threads::{ Thread, ThreadAnchor, ThreadComment, ThreadOverlayStore, apply_resolved_states, diff --git a/src/forge/submit.rs b/src/forge/submit.rs index 2c20fbd..e5c13b1 100644 --- a/src/forge/submit.rs +++ b/src/forge/submit.rs @@ -55,10 +55,62 @@ pub struct SubmitBatch { pub summary_draft_created: bool, } +impl SubmitBatch { + /// What a run over this batch sets out to send, in send order: the atomic + /// review's comments, then the file-comment follow-ups, then the replies. + /// Recorded into the report so a stopped run can name the items it never + /// reached. + pub fn attempt(&self) -> SubmitAttempt { + let mut annotation_ids = self.plan.comment_annotation_ids.clone(); + annotation_ids.extend( + self.plan + .file_comment_follow_ups + .iter() + .map(|f| f.annotation_id), + ); + SubmitAttempt { + annotation_ids, + reply_ids: self.replies.iter().map(|r| r.reply_id).collect(), + review_post: self.include_review_post && self.plan.payload.carries_content(), + } + } +} + +/// Every item one run set out to send, recorded by the sequence itself before +/// it starts writing. Joined against the published/draft lists it makes each +/// item's fate knowable — an id here and in neither list was never attempted — +/// which is what lets a stopped run be reported item by item instead of as a +/// bare count. Order is send order. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct SubmitAttempt { + /// Annotation store ids this run meant to publish. + pub annotation_ids: Vec, + /// Reply store ids this run meant to publish. + pub reply_ids: Vec, + /// Whether the run meant to deliver the review itself (the verdict and + /// summary): GitHub's reviews POST, GitLab's summary note plus approve. + /// `false` for a batch that carries neither — a reply-only resume — so + /// nothing is reported unsent that was never owed. + pub review_post: bool, +} + +/// Where one attempted item ended up, resolved by +/// [`SubmitReport::annotation_outcomes`]/[`SubmitReport::reply_outcomes`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ItemOutcome { + /// Visible on the forge. + Published, + /// Staged server-side as a private GitLab draft, awaiting a publish. + PendingDraft, + /// Never attempted — the run stopped before reaching it. + NotSent, +} + /// What one submit run accomplished: which annotations and replies are now /// published (to mark locally and persist), whether the reviews POST is now -/// done (so a resume skips it), and the one-line diagnostic when the run -/// stopped early. `failure: None` means every item in the batch published. +/// done (so a resume skips it), what the run set out to send in the first +/// place, and the one-line diagnostic when the run stopped early. +/// `failure: None` means every item in the batch published. #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct SubmitReport { /// Store ids of annotations published this run — the line/range/hunk @@ -86,6 +138,59 @@ pub struct SubmitReport { /// Whether the summary/verdict body now exists as an unpublished GitLab /// draft — the id-less counterpart to the two draft lists. pub summary_draft_created: bool, + /// What this run set out to send, recorded by the sequence itself — the + /// other half of the per-item outcome (see [`SubmitAttempt`]). + pub attempt: SubmitAttempt, +} + +impl SubmitReport { + /// Each attempted annotation's fate, in send order. `Published` wins over + /// `PendingDraft` for the same id (a published item's draft is consumed), + /// and an attempted id in neither list was never sent. + pub fn annotation_outcomes(&self) -> Vec<(usize, ItemOutcome)> { + outcomes( + &self.attempt.annotation_ids, + &self.published_annotation_ids, + &self.draft_annotation_ids, + ) + } + + /// Each attempted reply's fate, in send order — the reply counterpart to + /// [`SubmitReport::annotation_outcomes`]. + pub fn reply_outcomes(&self) -> Vec<(usize, ItemOutcome)> { + outcomes( + &self.attempt.reply_ids, + &self.published_reply_ids, + &self.draft_reply_ids, + ) + } + + /// Whether the review itself (verdict + summary) was owed and did not + /// land. A run that never owed one reports `false`. + pub fn review_post_not_sent(&self) -> bool { + self.attempt.review_post && !self.review_submitted + } +} + +/// Resolves each attempted id against the published and drafted lists. +fn outcomes( + attempted: &[usize], + published: &[usize], + drafted: &[usize], +) -> Vec<(usize, ItemOutcome)> { + attempted + .iter() + .map(|id| { + let outcome = if published.contains(id) { + ItemOutcome::Published + } else if drafted.contains(id) { + ItemOutcome::PendingDraft + } else { + ItemOutcome::NotSent + }; + (*id, outcome) + }) + .collect() } /// The three positioned GitHub write operations the driver sequences, behind @@ -120,6 +225,7 @@ pub trait ForgeSubmitExecutor { pub fn run_submit_sequence(batch: &SubmitBatch, exec: &dyn ForgeSubmitExecutor) -> SubmitReport { let mut report = SubmitReport { review_submitted: !batch.include_review_post, + attempt: batch.attempt(), ..SubmitReport::default() }; diff --git a/src/ui/annotation_list.rs b/src/ui/annotation_list.rs index 45f9796..5d18983 100644 --- a/src/ui/annotation_list.rs +++ b/src/ui/annotation_list.rs @@ -43,6 +43,7 @@ impl App { | Mode::ConfirmRemoteOp { .. } | Mode::ThreadView | Mode::SubmitForge + | Mode::SubmitResult { .. } | Mode::CleanupReviews { .. } | Mode::ConfirmRestore { .. } => {} Mode::Normal | Mode::Visual { .. } => { diff --git a/src/ui/app.rs b/src/ui/app.rs index e67eb77..6ff7fbd 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -164,6 +164,14 @@ pub enum Mode { /// confirms from here. Its selection/edit state lives in /// [`App::submit_forge`] (see [`super::forge_submit`]). SubmitForge, + /// The post-submit result modal is open (see + /// [`super::forge_submit_result`]): a read-only, per-item account of what a + /// stopped submit run published, left as a pending draft, or never sent. + /// Opened only when a run reports a failure, and asynchronously — the + /// report arrives on a background poll — so `origin` is whatever mode the + /// reviewer was in when it landed, restored exactly on dismiss. The outcome + /// snapshot lives in [`App::submit_result`]. + SubmitResult { origin: ModeOrigin }, /// The finished-review cleanup confirm modal (`cleanup-finished-reviews`, /// Pull Requests tab, opened by [`App::open_cleanup_reviews`]): enumerates /// every managed review whose PR is no longer open — number, title, @@ -607,6 +615,12 @@ pub struct App { /// The submit-review modal's state, `Some` only while /// [`Mode::SubmitForge`] is active (see [`super::forge_submit`]). pub(super) submit_forge: Option, + /// The post-submit result modal's outcome snapshot, `Some` only while + /// [`Mode::SubmitResult`] is active (see [`super::forge_submit_result`]). + /// Built once from the arriving report rather than per frame: the stores it + /// labels items from keep changing, and the account must stay the one the + /// run reported. + pub(super) submit_result: Option, /// The background poller the forge submit sequence runs through, separate /// from the other pollers so its result drains independently (see /// [`App::poll_forge_submit`]). @@ -893,6 +907,7 @@ impl App { threads_unavailable: false, thread_view: None, submit_forge: None, + submit_result: None, forge_submit_tasks: BackgroundTasks::new(), forge_submit_in_flight: None, forge_submit_generation: 0, diff --git a/src/ui/footer.rs b/src/ui/footer.rs index 1bf4c0c..dc0549a 100644 --- a/src/ui/footer.rs +++ b/src/ui/footer.rs @@ -513,6 +513,7 @@ pub(super) fn build_hints( Mode::ConfirmRemoteOp { .. } => modal_hints(&modal_keys.confirm_remote_op), Mode::ThreadView => modal_hints(&modal_keys.thread_view), Mode::SubmitForge => modal_hints(&modal_keys.submit_forge), + Mode::SubmitResult { .. } => modal_hints(&modal_keys.submit_result), Mode::CleanupReviews { .. } => modal_hints(&modal_keys.cleanup_reviews), Mode::ConfirmRestore { .. } => modal_hints(&modal_keys.restore), Mode::ReviewLauncher { .. } => modal_hints(&modal_keys.review_launcher), diff --git a/src/ui/forge_submit.rs b/src/ui/forge_submit.rs index 7970bac..06a7f09 100644 --- a/src/ui/forge_submit.rs +++ b/src/ui/forge_submit.rs @@ -152,7 +152,7 @@ fn plural(n: usize) -> &'static str { } /// The clipped-below marker, or `None` when the last line is on screen. -fn below_marker(hidden: u16) -> Option { +pub(super) fn below_marker(hidden: u16) -> Option { (hidden > 0).then(|| { format!( "\u{25be} {hidden} more line{} \u{2014} \u{2193} to scroll", @@ -162,7 +162,7 @@ fn below_marker(hidden: u16) -> Option { } /// The clipped-above marker, or `None` when the first line is on screen. -fn above_marker(hidden: u16) -> Option { +pub(super) fn above_marker(hidden: u16) -> Option { (hidden > 0).then(|| { format!( "\u{25b4} {hidden} more line{} above", @@ -195,7 +195,7 @@ fn summary_overflow_note(summary: &str) -> Option { /// line count the scroll math clamps against is the row count the terminal /// really shows — a re-flow behind the offset would put the bottom of a long /// batch out of reach and understate the "N more lines" count. -fn wrap_line(line: &Line<'_>, width: usize) -> Vec> { +pub(super) fn wrap_line(line: &Line<'_>, width: usize) -> Vec> { let chars: Vec<(char, Style)> = line .spans .iter() @@ -372,8 +372,10 @@ pub(super) struct SubmitPreview { } /// The in-file anchor label for an annotation (`path:line`, `path:start-end`, -/// or `path` for a whole-file target), for the preview. -fn anchor_label(target: &Target) -> String { +/// or `path` for a whole-file target), for the preview and the post-submit +/// result view — one labeling, so an item reads the same before and after it +/// is sent. +pub(super) fn anchor_label(target: &Target) -> String { match target { Target::Line { path, line, .. } => format!("{path}:{line}"), Target::Range { @@ -393,6 +395,48 @@ fn first_line(body: &str) -> String { body.lines().next().unwrap_or("").to_string() } +/// One annotation's preview row — anchor, classification, one-line body, and +/// publish path. Shared with the post-submit result view. +pub(super) fn annotation_preview(annotation: &Annotation) -> AnnotationPreview { + AnnotationPreview { + anchor: anchor_label(&annotation.target), + classification: annotation.classification, + summary: first_line(&annotation.body), + note: PreviewNote::of(&annotation.target), + } +} + +/// One reply's preview row, resolving its thread against the fetched overlay. +/// Shared with the post-submit result view, so a reply is named the same way +/// before and after it is sent. +pub(super) fn reply_preview( + thread_id: u64, + body: &str, + threads: &ThreadOverlayStore, +) -> ReplyPreview { + ReplyPreview { + thread_id, + target: threads.find(thread_id).map(|thread| ReplyTarget { + author: thread.root.author.clone(), + anchor: thread_anchor_label(&thread.anchor), + }), + summary: first_line(body), + } +} + +/// A reply row's human label: who and where the thread is, or a bare thread id +/// when the thread has dropped out of the overlay (e.g. a failed refresh) — +/// the id is then the only thing still known. +pub(super) fn reply_preview_label(reply: &ReplyPreview) -> String { + match &reply.target { + Some(target) => format!( + "to {} @ {} \u{2014} {}", + target.author, target.anchor, reply.summary + ), + None => format!("thread {}: {}", reply.thread_id, reply.summary), + } +} + /// Builds the grouped preview from the unpublished annotations and replies — /// annotations grouped by file in first-seen order, replies in insertion /// order. Pure; the caller passes the already-filtered unpublished sets and a @@ -406,12 +450,7 @@ pub(super) fn build_preview<'a>( let mut groups: Vec = Vec::new(); for annotation in annotations { let path = annotation.target.path().to_string(); - let item = AnnotationPreview { - anchor: anchor_label(&annotation.target), - classification: annotation.classification, - summary: first_line(&annotation.body), - note: PreviewNote::of(&annotation.target), - }; + let item = annotation_preview(annotation); match groups.iter_mut().find(|g| g.path == path) { Some(group) => group.items.push(item), None => groups.push(FileGroup { @@ -421,17 +460,7 @@ pub(super) fn build_preview<'a>( } } let replies = replies - .map(|(thread_id, body)| { - let target = threads.find(thread_id).map(|thread| ReplyTarget { - author: thread.root.author.clone(), - anchor: thread_anchor_label(&thread.anchor), - }); - ReplyPreview { - thread_id, - target, - summary: first_line(body), - } - }) + .map(|(thread_id, body)| reply_preview(thread_id, body, threads)) .collect(); SubmitPreview { groups, replies } } @@ -809,12 +838,19 @@ impl App { }; self.set_status_message(message); self.rebuild_rows(); + // A stopped run leaves the reviewer needing to know *which* comments + // landed, which no one-line count can say — so the per-item result + // view opens on top of the status line (additive: the status stays for + // after it is dismissed). A clean submit needs no such accounting. + if report.failure.is_some() { + self.open_submit_result(&report); + } } } /// Centers a `width_pct`% x `height_pct`% rect inside `area` (same helper /// shape as [`super::forge_threads`]'s `centered`). -fn centered(area: Rect, width_pct: u16, height_pct: u16) -> Rect { +pub(super) fn centered(area: Rect, width_pct: u16, height_pct: u16) -> Rect { let [area] = Layout::horizontal([Constraint::Percentage(width_pct)]) .flex(Flex::Center) .areas(area); @@ -926,15 +962,7 @@ fn build_lines( .add_modifier(Modifier::BOLD), ))); for reply in &preview.replies { - let label = match &reply.target { - Some(target) => format!( - "to {} @ {} \u{2014} {}", - target.author, target.anchor, reply.summary - ), - // The thread dropped out of the overlay (e.g. a failed - // refresh) — fall back to the id, the only thing still known. - None => format!("thread {}: {}", reply.thread_id, reply.summary), - }; + let label = reply_preview_label(reply); lines.push(Line::from(Span::styled( format!(" \u{21b3} {label}"), Style::default().fg(theme.annotation_text), diff --git a/src/ui/forge_submit_result.rs b/src/ui/forge_submit_result.rs new file mode 100644 index 0000000..25e3415 --- /dev/null +++ b/src/ui/forge_submit_result.rs @@ -0,0 +1,368 @@ +//! The post-submit result view: a read-only modal that names, item by item, +//! what a stopped submit run actually landed. The one-line status a partial +//! failure leaves ("submit stopped: 3 published, 2 not sent — …") is honest +//! about the counts but says nothing about *which* comments made it, which +//! left opening the PR in a browser as the only way to find out. This modal +//! answers that question from the run's own report. +//! +//! Opened by [`super::forge_submit::App::apply_submit_outcome`] only when the +//! run reported a failure; a clean submit keeps the one-line status and no +//! modal. The status line is set either way, so dismissing the modal leaves +//! the outcome recorded where it always was. +//! +//! Every row comes from the [`SubmitReport`] the sequence produced — its +//! attempted set joined against its published and drafted lists (see +//! [`crate::forge::SubmitAttempt`]) — never from a diff of local state, so an +//! item is reported unsent only when the run really never reached it. The +//! labels are [`super::forge_submit`]'s own preview labels, so a comment reads +//! the same before and after it is sent. + +use std::cell::Cell; + +use ratatui::Frame; +use ratatui::layout::{Constraint, Layout, Rect}; +use ratatui::style::{Modifier, Style}; +use ratatui::text::{Line, Span}; +use ratatui::widgets::{Block, Borders, Clear, Paragraph}; + +use crate::annotate::AnnotationStore; +use crate::forge::{ItemOutcome, SubmitReport, ThreadOverlayStore}; + +use super::app::{App, Mode, ModeOrigin}; +use super::draft_reply::DraftReplyStore; +use super::forge_submit::{ + above_marker, annotation_preview, below_marker, reply_preview, reply_preview_label, + resolve_scroll, wrap_line, +}; +use super::theme::Theme; + +/// The grouped per-item outcome of one stopped submit run: three lists of +/// display labels plus the diagnostic that stopped it. Pure data built by +/// [`build_result`], so the grouping is unit-tested without a frame. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub(super) struct SubmitResultView { + /// Items visible on the forge now. + pub(super) published: Vec, + /// Items staged as private GitLab drafts, awaiting a publish. + pub(super) pending_drafts: Vec, + /// Items the run never reached — plus, first, the review itself when the + /// verdict/summary POST is what failed (everything after it is unsent by + /// construction, so it belongs at the top of this group). + pub(super) not_sent: Vec, + /// The one-line diagnostic from the run (already carrying + /// `submit_error_headline`'s token-scope hint where one applies). + pub(super) diagnostic: String, +} + +impl SubmitResultView { + /// Whether the run reported no per-item outcome at all — only reachable + /// when it failed before recording what it meant to send (a panicked + /// submit task). The modal says so rather than showing three empty groups. + fn is_itemless(&self) -> bool { + self.published.is_empty() && self.pending_drafts.is_empty() && self.not_sent.is_empty() + } +} + +/// The modal's live state: the outcome it shows (built once when the report +/// arrives — the stores it was labeled from go on changing) and the scroll +/// offset, clamped by [`render`] against the real rendered line count. +#[derive(Debug)] +pub(super) struct SubmitResultState { + pub(super) view: SubmitResultView, + pub(super) scroll: Cell, + /// The scrollable body's height, recorded each frame so the page keys page + /// by a real viewport. + pub(super) viewport: Cell, +} + +/// The label a row shows for annotation `id`: its preview anchor, +/// classification, and one-line body. Falls back to the bare id if the +/// annotation is somehow gone from the store, so a row is never dropped. +fn annotation_label(id: usize, annotations: &AnnotationStore) -> String { + match annotations.iter().find(|a| a.id == id) { + Some(annotation) => { + let preview = annotation_preview(annotation); + format!( + "{} [{}] {}", + preview.anchor, + preview.classification.label(), + preview.summary + ) + } + None => format!("annotation #{id}"), + } +} + +/// The label a row shows for reply `id`: the same "to @ " +/// naming the submit preview uses, or the bare thread id when the thread has +/// dropped out of the overlay. +fn reply_label(id: usize, replies: &DraftReplyStore, threads: &ThreadOverlayStore) -> String { + match replies.get(id) { + Some(reply) => format!( + "\u{21b3} {}", + reply_preview_label(&reply_preview(reply.thread_id, &reply.body, threads)) + ), + None => format!("\u{21b3} reply #{id}"), + } +} + +/// The row naming the review itself in the not-sent group. +const REVIEW_NOT_POSTED: &str = "the review itself (verdict + summary) was not posted"; + +/// Groups one run's report into the three outcome lists, labeling each item +/// through the submit preview's own naming. Annotations come before replies +/// within a group, each in the run's send order. +pub(super) fn build_result( + report: &SubmitReport, + annotations: &AnnotationStore, + replies: &DraftReplyStore, + threads: &ThreadOverlayStore, +) -> SubmitResultView { + let mut view = SubmitResultView { + diagnostic: report.failure.clone().unwrap_or_default(), + ..SubmitResultView::default() + }; + if report.review_post_not_sent() { + view.not_sent.push(REVIEW_NOT_POSTED.to_string()); + } + let rows = report + .annotation_outcomes() + .into_iter() + .map(|(id, outcome)| (annotation_label(id, annotations), outcome)) + .chain( + report + .reply_outcomes() + .into_iter() + .map(|(id, outcome)| (reply_label(id, replies, threads), outcome)), + ); + for (label, outcome) in rows { + match outcome { + ItemOutcome::Published => view.published.push(label), + ItemOutcome::PendingDraft => view.pending_drafts.push(label), + ItemOutcome::NotSent => view.not_sent.push(label), + } + } + view +} + +impl App { + /// Opens the result modal on a stopped run's report, capturing whatever + /// mode the reviewer was in so dismissing restores it exactly (the + /// [`ModeOrigin`] contract every other modal uses). The report arrives + /// asynchronously, so this can interrupt any mode; the interrupted mode's + /// own state lives on `App` and survives the round trip untouched. + pub(super) fn open_submit_result(&mut self, report: &SubmitReport) { + let view = build_result( + report, + &self.annotations, + &self.replies, + &self.thread_overlay, + ); + self.submit_result = Some(SubmitResultState { + view, + scroll: Cell::new(0), + viewport: Cell::new(0), + }); + self.mode = Mode::SubmitResult { + origin: ModeOrigin::capture(self.mode), + }; + } + + /// Dismisses the result modal, restoring the mode it interrupted. + pub(super) fn close_submit_result(&mut self) { + let origin = match self.mode { + Mode::SubmitResult { origin } => origin, + _ => ModeOrigin::Normal, + }; + self.submit_result = None; + self.mode = origin.restore(); + } + + /// Dismisses the modal and reopens the submit modal for another pass. The + /// batch is rebuilt there from the still-unpublished items, so a retry + /// re-sends nothing that already landed. + pub(super) fn submit_result_retry(&mut self) { + self.close_submit_result(); + self.open_submit_forge(); + } + + /// Scrolls the result list down one line. [`render`] clamps the offset to + /// the content, so an overshoot here can't run off the end. + pub(super) fn submit_result_scroll_down(&mut self) { + if let Some(state) = self.submit_result.as_ref() { + state.scroll.set(state.scroll.get().saturating_add(1)); + } + } + + /// Scrolls the result list up one line. + pub(super) fn submit_result_scroll_up(&mut self) { + if let Some(state) = self.submit_result.as_ref() { + state.scroll.set(state.scroll.get().saturating_sub(1)); + } + } + + /// Scrolls down a full viewport (the height the last frame recorded). + pub(super) fn submit_result_page_down(&mut self) { + if let Some(state) = self.submit_result.as_ref() { + let page = state.viewport.get().max(1); + state.scroll.set(state.scroll.get().saturating_add(page)); + } + } + + /// Scrolls up a full viewport. + pub(super) fn submit_result_page_up(&mut self) { + if let Some(state) = self.submit_result.as_ref() { + let page = state.viewport.get().max(1); + state.scroll.set(state.scroll.get().saturating_sub(page)); + } + } +} + +/// One outcome group's block of lines: a counted header in the group's own +/// style, then its rows. Nothing is emitted for an empty group — a +/// "published (0)" header would be noise on a run that published nothing. +fn group_lines( + header: String, + rows: &[String], + header_style: Style, + row_style: Style, +) -> Vec> { + if rows.is_empty() { + return Vec::new(); + } + let mut lines = vec![Line::from(Span::styled(header, header_style))]; + lines.extend( + rows.iter() + .map(|row| Line::from(Span::styled(format!(" {row}"), row_style))), + ); + lines.push(Line::from(String::new())); + lines +} + +/// Builds the modal's body: the three outcome groups top to bottom (what +/// landed first, so the reassuring half is read before the bad news), then the +/// diagnostic headline. Split out from [`render`] so the rendered line count — +/// what the scroll math clamps against — is a value the caller holds. +fn build_lines(view: &SubmitResultView, theme: &Theme) -> Vec> { + let dim = Style::default() + .fg(theme.gutter) + .add_modifier(Modifier::DIM); + let mut lines: Vec = Vec::new(); + lines.extend(group_lines( + format!("\u{2713} published ({})", view.published.len()), + &view.published, + Style::default() + .fg(theme.added_fg) + .add_modifier(Modifier::BOLD), + Style::default().fg(theme.annotation_text), + )); + lines.extend(group_lines( + format!( + "\u{25cc} pending draft ({}) \u{2014} submit again to publish", + view.pending_drafts.len() + ), + &view.pending_drafts, + Style::default() + .fg(theme.hunk_header) + .add_modifier(Modifier::BOLD), + Style::default().fg(theme.annotation_text), + )); + lines.extend(group_lines( + format!("\u{2717} not sent ({})", view.not_sent.len()), + &view.not_sent, + Style::default() + .fg(theme.removed_fg) + .add_modifier(Modifier::BOLD), + Style::default().fg(theme.annotation_text), + )); + if view.is_itemless() { + lines.push(Line::from(Span::styled( + "The run reported no per-item outcome \u{2014} check the PR before resubmitting.", + dim, + ))); + lines.push(Line::from(String::new())); + } + if !view.diagnostic.is_empty() { + lines.push(Line::from(Span::styled( + view.diagnostic.clone(), + Style::default() + .fg(theme.status_message) + .add_modifier(Modifier::BOLD), + ))); + } + lines +} + +/// Renders the result modal, centered over `area`. A no-op when it isn't open. +/// A list taller than the modal scrolls (`j`/`k`, the arrows, and the page +/// keys) with a marker row naming how many lines are hidden in each direction, +/// so no outcome is clipped silently. +pub fn render(frame: &mut Frame, area: Rect, app: &App) { + let Some(state) = &app.submit_result else { + return; + }; + let theme = &app.theme; + let popup = super::forge_submit::centered(area, 72, 72); + frame.render_widget(Clear, popup); + + let block = Block::default() + .borders(Borders::ALL) + .title("Submit stopped \u{2014} what landed and what didn't") + .title_bottom(Line::from( + " Enter/Esc dismiss U submit again j/k scroll ", + )); + let inner = block.inner(popup); + frame.render_widget(block, popup); + + let lines: Vec = build_lines(&state.view, theme) + .into_iter() + .flat_map(|line| wrap_line(&line, inner.width as usize)) + .collect(); + + let total = u16::try_from(lines.len()).unwrap_or(u16::MAX); + let view = resolve_scroll(total, inner.height, state.scroll.get()); + state.scroll.set(view.offset); + state.viewport.set(view.body_height.max(1)); + + // Two rows means `resolve_scroll` reserved the marker rows; otherwise the + // whole box is body. + let body = if inner.height.saturating_sub(view.body_height) == 2 { + let [top, body, bottom] = Layout::vertical([ + Constraint::Length(1), + Constraint::Min(0), + Constraint::Length(1), + ]) + .areas(inner); + if let Some(text) = above_marker(view.hidden_above) { + frame.render_widget( + Paragraph::new(Line::from(Span::styled( + text, + Style::default() + .fg(theme.gutter) + .add_modifier(Modifier::DIM), + ))), + top, + ); + } + if let Some(text) = below_marker(view.hidden_below) { + frame.render_widget( + Paragraph::new(Line::from(Span::styled( + text, + Style::default() + .fg(theme.hunk_header) + .add_modifier(Modifier::BOLD), + ))), + bottom, + ); + } + body + } else { + inner + }; + + frame.render_widget(Paragraph::new(lines).scroll((view.offset, 0)), body); +} + +#[cfg(test)] +#[path = "forge_submit_result_tests.rs"] +mod tests; diff --git a/src/ui/forge_submit_result_tests.rs b/src/ui/forge_submit_result_tests.rs new file mode 100644 index 0000000..22c0e41 --- /dev/null +++ b/src/ui/forge_submit_result_tests.rs @@ -0,0 +1,619 @@ +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; +use ratatui::Terminal; +use ratatui::backend::TestBackend; + +use crate::annotate::{Classification, Side, Target}; +use crate::diff::FileDiff; +use crate::forge::{ + ForgeError, ForgeSubmitExecutor, ReviewPayload, SubmitAttempt, SubmitReport, Thread, + ThreadAnchor, ThreadComment, Verdict, +}; +use crate::git::{DiffTarget, RawFilePatch}; +use crate::review::store::{ForgeMetadata, ForgeProviderKind}; + +use super::super::app::{App, Mode, ModeOrigin, PanelTab}; +use super::super::modes::handle_submit_result_key; +use super::*; + +// -- fixtures ---------------------------------------------------------------- + +fn file(path: &str) -> FileDiff { + let raw = format!( + "diff --git a/{path} b/{path}\nindex 1..2 100644\n--- a/{path}\n+++ b/{path}\n@@ -1,3 +1,3 @@\n fn f() {{\n- old();\n+ new();\n" + ); + FileDiff::from_patch(&RawFilePatch { + path: path.to_string(), + old_path: None, + raw, + is_binary: false, + }) + .unwrap() +} + +/// A GitHub PR review session, so `submit-forge-review` is live. +fn review_app(paths: &[&str]) -> App { + let mut app = App::new(paths.iter().map(|p| file(p)).collect()); + app.target = DiffTarget::Review { + base: "main".to_string(), + branch: "redquill/pr/34".to_string(), + }; + app.review_forge = Some(ForgeMetadata { + provider: ForgeProviderKind::GitHub, + host: "github.com".to_string(), + number: 34, + title: String::new(), + last_head_sha: "deadbeef".to_string(), + diff_refs: None, + }); + app +} + +fn thread(id: u64, author: &str, path: &str, line: u32) -> Thread { + Thread { + id, + anchor: ThreadAnchor::Position { + path: path.to_string(), + side: Side::New, + line, + }, + root: ThreadComment { + id, + author: author.to_string(), + created_at: "2026-07-01T10:00:00Z".to_string(), + body: "root".to_string(), + }, + replies: Vec::new(), + resolved: false, + outdated: false, + discussion_id: None, + } +} + +fn boom() -> ForgeError { + ForgeError::Command { + cli: "gh", + command: "api".to_string(), + code: "403".to_string(), + stderr: "HTTP 403: Resource not accessible".to_string(), + } +} + +/// A fake [`ForgeSubmitExecutor`] (no `gh`/`glab` is ever run) that fails a +/// chosen phase of the sequence, so a real [`SubmitReport`] can be produced for +/// a genuinely partial run. +struct PartialSubmitter { + review_ok: bool, + file_comments_ok: bool, + replies_ok: bool, +} + +impl ForgeSubmitExecutor for PartialSubmitter { + fn submit_review(&self, _payload: &ReviewPayload) -> Result<(), ForgeError> { + if self.review_ok { Ok(()) } else { Err(boom()) } + } + + fn post_file_comment(&self, _path: &str, _body: &str) -> Result<(), ForgeError> { + if self.file_comments_ok { + Ok(()) + } else { + Err(boom()) + } + } + + fn post_reply(&self, _thread_id: u64, _body: &str) -> Result<(), ForgeError> { + if self.replies_ok { Ok(()) } else { Err(boom()) } + } +} + +/// A review with a line comment (rides the atomic review POST), a file comment +/// (a follow-up), and a drafted reply — one item per submit phase, so a failure +/// in any phase leaves a genuinely mixed outcome. +fn app_with_one_item_per_phase() -> App { + let mut app = review_app(&["src/a.rs"]); + app.annotations + .add( + Target::line("src/a.rs", 2, Side::New), + Classification::Issue, + "fix the line", + ) + .unwrap(); + app.annotations + .add( + Target::file("src/a.rs"), + Classification::Praise, + "nice file", + ) + .unwrap(); + app.replies.add(100, "agreed").unwrap(); + app.thread_overlay + .replace(vec![thread(100, "alice", "src/a.rs", 12)]); + app +} + +/// A stopped run over `n` line comments, none of which published — enough rows +/// to overflow any sensibly sized modal. +fn app_with_a_long_result(n: usize) -> App { + let mut app = review_app(&["src/a.rs"]); + let mut ids = Vec::new(); + for i in 0..n { + ids.push( + app.annotations + .add( + Target::line("src/a.rs", 2, Side::New), + Classification::Issue, + format!("comment number {i}"), + ) + .unwrap(), + ); + } + app.apply_submit_outcome(SubmitReport { + failure: Some("HTTP 403".to_string()), + attempt: SubmitAttempt { + annotation_ids: ids, + reply_ids: Vec::new(), + review_post: true, + }, + ..SubmitReport::default() + }); + app +} + +/// Renders the modal over a `width` x `height` terminal and returns its cell +/// symbols as one string. +fn render_modal(app: &App, width: u16, height: u16) -> String { + let backend = TestBackend::new(width, height); + let mut terminal = Terminal::new(backend).unwrap(); + let area = Rect::new(0, 0, width, height); + terminal.draw(|frame| render(frame, area, app)).unwrap(); + terminal + .backend() + .buffer() + .content() + .iter() + .map(|c| c.symbol()) + .collect() +} + +/// Runs the real submit sequence over the app's batch against a fake that +/// fails the named phase, then applies the report exactly as the poller does. +fn submit_with(app: &mut App, exec: &PartialSubmitter) { + let batch = app.build_submit_batch(Verdict::Comment, Some("overall")); + let report = crate::forge::run_submit_sequence(&batch, exec); + app.apply_submit_outcome(report); +} + +// -- when the result view opens at all --------------------------------------- + +#[test] +fn a_clean_submit_reports_in_the_status_line_and_opens_no_modal() { + let mut app = app_with_one_item_per_phase(); + submit_with( + &mut app, + &PartialSubmitter { + review_ok: true, + file_comments_ok: true, + replies_ok: true, + }, + ); + + assert!( + app.submit_result.is_none(), + "a fully published submit needs no per-item accounting" + ); + assert_eq!(app.mode, Mode::Normal); + assert!( + app.status_message + .as_deref() + .is_some_and(|m| m.contains("review submitted")), + "status: {:?}", + app.status_message + ); +} + +#[test] +fn a_partial_failure_opens_the_result_view_and_still_sets_the_status_line() { + let mut app = app_with_one_item_per_phase(); + // The review POST lands (the line comment publishes), the file comment + // follow-up fails, so the reply is never attempted. + submit_with( + &mut app, + &PartialSubmitter { + review_ok: true, + file_comments_ok: false, + replies_ok: true, + }, + ); + + let view = &app + .submit_result + .as_ref() + .expect("a stopped run opens the result view") + .view; + assert_eq!(view.published.len(), 1, "{view:?}"); + assert!(view.published[0].contains("src/a.rs:2"), "{view:?}"); + assert!(view.pending_drafts.is_empty(), "{view:?}"); + // The unsent file comment and the never-attempted reply. + assert_eq!(view.not_sent.len(), 2, "{view:?}"); + assert!( + view.not_sent.iter().any(|r| r.contains("src/a.rs")), + "{view:?}" + ); + assert!( + view.not_sent + .iter() + .any(|r| r.contains("to alice @ src/a.rs:12")), + "the reply keeps the humanized label the preview gave it: {view:?}" + ); + assert!( + !view.not_sent.iter().any(|r| r.contains("review itself")), + "the review POST did land, so it must not be listed unsent: {view:?}" + ); + assert!(view.diagnostic.contains("403"), "{view:?}"); + assert!( + app.status_message + .as_deref() + .is_some_and(|m| m.contains("submit stopped")), + "the one-line status stays for after the modal is dismissed: {:?}", + app.status_message + ); +} + +#[test] +fn a_total_failure_lists_every_item_and_the_review_post_as_not_sent() { + let mut app = app_with_one_item_per_phase(); + submit_with( + &mut app, + &PartialSubmitter { + review_ok: false, + file_comments_ok: true, + replies_ok: true, + }, + ); + + let view = &app + .submit_result + .as_ref() + .expect("a total failure opens the result view") + .view; + assert!(view.published.is_empty(), "{view:?}"); + assert!(view.pending_drafts.is_empty(), "{view:?}"); + // The review itself, then the two annotations, then the reply. + assert_eq!(view.not_sent.len(), 4, "{view:?}"); + assert!( + view.not_sent[0].contains("review itself"), + "the failed review POST leads the group: {view:?}" + ); +} + +// -- grouping across all three outcomes -------------------------------------- + +#[test] +fn published_pending_and_unsent_items_land_in_their_own_groups() { + // A GitLab-shaped stop: one comment published, one left as a private + // draft, one never attempted, one reply never attempted, and the review + // itself not posted. + let mut app = review_app(&["src/a.rs"]); + let published = app + .annotations + .add( + Target::line("src/a.rs", 2, Side::New), + Classification::Issue, + "published one", + ) + .unwrap(); + let drafted = app + .annotations + .add( + Target::range("src/a.rs", 3, 5, Side::New).unwrap(), + Classification::Nit, + "drafted one", + ) + .unwrap(); + let unsent = app + .annotations + .add( + Target::file("src/a.rs"), + Classification::Question, + "unsent one", + ) + .unwrap(); + let reply = app.replies.add(100, "agreed").unwrap(); + + let report = SubmitReport { + published_annotation_ids: vec![published], + draft_annotation_ids: vec![drafted], + failure: Some("HTTP 403: forbidden (write blocked)".to_string()), + attempt: SubmitAttempt { + annotation_ids: vec![published, drafted, unsent], + reply_ids: vec![reply], + review_post: true, + }, + ..SubmitReport::default() + }; + let view = build_result(&report, &app.annotations, &app.replies, &app.thread_overlay); + + assert_eq!(view.published.len(), 1); + assert!(view.published[0].contains("src/a.rs:2"), "{view:?}"); + assert!(view.published[0].contains("issue"), "{view:?}"); + assert_eq!(view.pending_drafts.len(), 1); + assert!( + view.pending_drafts[0].contains("src/a.rs:3-5"), + "a range anchor keeps its span: {view:?}" + ); + assert_eq!(view.not_sent.len(), 3, "{view:?}"); + assert!(view.not_sent[0].contains("review itself"), "{view:?}"); + assert!(view.not_sent[1].contains("unsent one"), "{view:?}"); + assert!( + view.not_sent[2].contains("thread 100"), + "with no thread in the overlay the id is the only honest label: {view:?}" + ); + assert!(view.diagnostic.contains("write blocked"), "{view:?}"); +} + +#[test] +fn a_published_item_is_never_also_reported_as_a_pending_draft() { + // The GitLab bulk publish flips a pre-existing draft: the same id appears + // in both lists' inputs, and only "published" is true of it. + let mut app = review_app(&["src/a.rs"]); + let id = app + .annotations + .add( + Target::line("src/a.rs", 2, Side::New), + Classification::Issue, + "fix", + ) + .unwrap(); + let report = SubmitReport { + published_annotation_ids: vec![id], + draft_annotation_ids: vec![id], + failure: Some("approve failed".to_string()), + attempt: SubmitAttempt { + annotation_ids: vec![id], + ..SubmitAttempt::default() + }, + ..SubmitReport::default() + }; + let view = build_result(&report, &app.annotations, &app.replies, &app.thread_overlay); + assert_eq!(view.published.len(), 1); + assert!(view.pending_drafts.is_empty(), "{view:?}"); +} + +#[test] +fn a_reply_only_resume_never_reports_a_review_post_it_did_not_owe() { + // `include_review_post: false` (a resume): the sequence owed no review + // POST, so a later failure must not accuse it of skipping one. + let mut app = review_app(&["src/a.rs"]); + let reply = app.replies.add(100, "agreed").unwrap(); + app.forge_review_submitted = true; + let batch = app.build_submit_batch(Verdict::Comment, None); + assert!(!batch.include_review_post); + + let report = crate::forge::run_submit_sequence( + &batch, + &PartialSubmitter { + review_ok: true, + file_comments_ok: true, + replies_ok: false, + }, + ); + let view = build_result(&report, &app.annotations, &app.replies, &app.thread_overlay); + assert!( + !view.not_sent.iter().any(|r| r.contains("review itself")), + "{view:?}" + ); + assert_eq!(view.not_sent.len(), 1, "just the reply: {view:?}"); + let _ = reply; +} + +// -- keys -------------------------------------------------------------------- + +fn press(app: &mut App, code: KeyCode) { + handle_submit_result_key(app, KeyEvent::new(code, KeyModifiers::NONE)); +} + +#[test] +fn every_dismiss_key_restores_the_mode_the_report_interrupted() { + for code in [KeyCode::Enter, KeyCode::Esc, KeyCode::Char('q')] { + let mut app = app_with_one_item_per_phase(); + app.mode = Mode::Panel { + cursor: 3, + tab: PanelTab::Changes, + }; + submit_with( + &mut app, + &PartialSubmitter { + review_ok: false, + file_comments_ok: true, + replies_ok: true, + }, + ); + assert_eq!( + app.mode, + Mode::SubmitResult { + origin: ModeOrigin::Panel { + cursor: 3, + tab: PanelTab::Changes, + }, + }, + "{code:?}" + ); + + press(&mut app, code); + assert_eq!( + app.mode, + Mode::Panel { + cursor: 3, + tab: PanelTab::Changes + }, + "{code:?} must restore the interrupted mode" + ); + assert!(app.submit_result.is_none(), "{code:?}"); + } +} + +#[test] +fn retry_reopens_the_submit_modal_with_only_what_did_not_land() { + let mut app = app_with_one_item_per_phase(); + // The review POST lands (the line comment publishes); the file comment + // fails, so it and the reply remain. + submit_with( + &mut app, + &PartialSubmitter { + review_ok: true, + file_comments_ok: false, + replies_ok: true, + }, + ); + assert!(app.submit_result.is_some()); + + press(&mut app, KeyCode::Char('U')); + assert_eq!(app.mode, Mode::SubmitForge); + assert!(app.submit_result.is_none(), "the result view is dismissed"); + + // The rebuilt batch carries the remainder only, and skips the review POST + // that already landed. + let batch = app.build_submit_batch(Verdict::Comment, Some("overall")); + assert!( + !batch.include_review_post, + "the verdict already landed; a retry must not re-deliver it" + ); + assert!( + batch.plan.comment_annotation_ids.is_empty(), + "the published line comment is not re-sent" + ); + assert_eq!(batch.plan.file_comment_follow_ups.len(), 1); + assert_eq!(batch.replies.len(), 1); +} + +#[test] +fn every_result_table_entry_drives_its_documented_action() { + use super::super::modal_keys::{SUBMIT_RESULT_KEYS, SubmitResultAction}; + + for binding in SUBMIT_RESULT_KEYS.iter() { + for key in &binding.keys { + let label = binding.key_label(); + let mut app = app_with_a_long_result(40); + // A render fixes the viewport the page keys move by, and the + // clamp the scroll assertions below are read against. + let _ = render_modal(&app, 80, 16); + let viewport = app.submit_result.as_ref().unwrap().viewport.get(); + + match binding.action { + SubmitResultAction::ScrollDown => { + handle_submit_result_key(&mut app, key.event()); + assert_eq!( + app.submit_result.as_ref().unwrap().scroll.get(), + 1, + "Submit result {label}: scroll-down advances one line" + ); + } + SubmitResultAction::ScrollUp => { + app.submit_result.as_ref().unwrap().scroll.set(3); + handle_submit_result_key(&mut app, key.event()); + assert_eq!( + app.submit_result.as_ref().unwrap().scroll.get(), + 2, + "Submit result {label}: scroll-up retreats one line" + ); + } + SubmitResultAction::PageDown => { + handle_submit_result_key(&mut app, key.event()); + assert_eq!( + app.submit_result.as_ref().unwrap().scroll.get(), + viewport, + "Submit result {label}: page-down moves a full viewport" + ); + } + SubmitResultAction::PageUp => { + app.submit_result.as_ref().unwrap().scroll.set(viewport + 1); + handle_submit_result_key(&mut app, key.event()); + assert_eq!( + app.submit_result.as_ref().unwrap().scroll.get(), + 1, + "Submit result {label}: page-up moves a full viewport" + ); + } + SubmitResultAction::Dismiss => { + handle_submit_result_key(&mut app, key.event()); + assert!( + app.submit_result.is_none(), + "Submit result {label}: dismiss closes the modal" + ); + assert_eq!(app.mode, Mode::Normal, "Submit result {label}"); + } + SubmitResultAction::Retry => { + handle_submit_result_key(&mut app, key.event()); + assert_eq!( + app.mode, + Mode::SubmitForge, + "Submit result {label}: retry reopens the submit modal" + ); + assert!(app.submit_result.is_none(), "Submit result {label}"); + } + } + } + } +} + +#[test] +fn a_report_with_no_recorded_attempt_says_so_instead_of_showing_nothing() { + // The panicked-submit-task path: a failure with no attempt recorded. Three + // empty groups would render as a blank box that reads like "nothing + // happened", which is the one thing the run cannot promise. + let mut app = review_app(&["src/a.rs"]); + app.apply_submit_outcome(SubmitReport { + failure: Some("submit task panicked".to_string()), + ..SubmitReport::default() + }); + + assert!(app.submit_result.is_some()); + let rendered = render_modal(&app, 80, 16); + assert!( + rendered.contains("no per-item outcome"), + "an itemless report must say so: {rendered}" + ); + assert!(rendered.contains("panicked"), "{rendered}"); +} + +// -- scrolling --------------------------------------------------------------- + +#[test] +fn the_result_list_scrolls_and_the_offset_clamps_to_the_content() { + let mut app = app_with_a_long_result(40); + // A render establishes the viewport and clamps whatever the keys asked for. + let top = render_modal(&app, 80, 16); + assert!(top.contains("more line"), "the overflow is marked: {top}"); + assert_eq!(app.submit_result.as_ref().unwrap().scroll.get(), 0); + + press(&mut app, KeyCode::Char('j')); + press(&mut app, KeyCode::Char('j')); + let _ = render_modal(&app, 80, 16); + assert_eq!(app.submit_result.as_ref().unwrap().scroll.get(), 2); + + press(&mut app, KeyCode::Char('k')); + let _ = render_modal(&app, 80, 16); + assert_eq!(app.submit_result.as_ref().unwrap().scroll.get(), 1); + + // Paging far past the end lands on the last page rather than off it. + for _ in 0..20 { + press(&mut app, KeyCode::PageDown); + } + let bottom = render_modal(&app, 80, 16); + let offset = app.submit_result.as_ref().unwrap().scroll.get(); + assert!(offset > 0, "a long list scrolls"); + assert!( + !bottom.contains("to scroll"), + "the bottom of the list is reached, so nothing is marked below: {bottom}" + ); + // A second render at the clamped offset must not move it again. + let _ = render_modal(&app, 80, 16); + assert_eq!(app.submit_result.as_ref().unwrap().scroll.get(), offset); +} + +#[test] +fn the_diagnostic_headline_is_rendered_below_the_groups() { + let app = app_with_a_long_result(1); + let rendered = render_modal(&app, 80, 16); + assert!(rendered.contains("not sent"), "{rendered}"); + assert!(rendered.contains("HTTP 403"), "{rendered}"); +} diff --git a/src/ui/forge_submit_tests.rs b/src/ui/forge_submit_tests.rs index 64657e5..d6e722d 100644 --- a/src/ui/forge_submit_tests.rs +++ b/src/ui/forge_submit_tests.rs @@ -5,7 +5,7 @@ use ratatui::backend::TestBackend; use crate::annotate::{Classification, Side, Target}; use crate::diff::FileDiff; use crate::forge::{ - SubmitReport, Thread, ThreadAnchor, ThreadComment, ThreadOverlayStore, Verdict, + SubmitAttempt, SubmitReport, Thread, ThreadAnchor, ThreadComment, ThreadOverlayStore, Verdict, }; use crate::git::{DiffTarget, RawFilePatch}; use crate::review::store::{ForgeMetadata, ForgeProviderKind}; @@ -389,6 +389,7 @@ fn apply_outcome_marks_published_items_and_reports_a_clean_success() { failure: None, draft_annotation_ids: vec![], draft_reply_ids: vec![], + attempt: SubmitAttempt::default(), summary_draft_created: false, }); @@ -425,6 +426,7 @@ fn apply_outcome_on_mid_failure_reports_the_published_unpublished_split() { failure: Some("file boom".to_string()), draft_annotation_ids: vec![], draft_reply_ids: vec![], + attempt: SubmitAttempt::default(), summary_draft_created: false, }); @@ -463,6 +465,7 @@ fn apply_outcome_with_pending_drafts_reports_them_instead_of_calling_them_failed failure: Some("boom".to_string()), draft_annotation_ids: vec![a0], draft_reply_ids: vec![], + attempt: SubmitAttempt::default(), summary_draft_created: false, }); @@ -500,6 +503,7 @@ fn apply_outcome_records_pending_drafts_and_the_resubmit_batch_skips_them() { failure: Some("boom".to_string()), draft_annotation_ids: vec![a0], draft_reply_ids: vec![r0], + attempt: SubmitAttempt::default(), summary_draft_created: true, }); @@ -554,6 +558,7 @@ fn apply_outcome_publishing_clears_draft_state() { failure: None, draft_annotation_ids: vec![], draft_reply_ids: vec![], + attempt: SubmitAttempt::default(), summary_draft_created: false, }); diff --git a/src/ui/git_panel.rs b/src/ui/git_panel.rs index 8403244..e093b59 100644 --- a/src/ui/git_panel.rs +++ b/src/ui/git_panel.rs @@ -757,6 +757,7 @@ impl App { | Mode::ConfirmRemoteOp { .. } | Mode::ThreadView | Mode::SubmitForge + | Mode::SubmitResult { .. } | Mode::CleanupReviews { .. } | Mode::ConfirmRestore { .. } => {} Mode::Normal | Mode::Visual { .. } => { diff --git a/src/ui/help.rs b/src/ui/help.rs index b8ae5e3..932e334 100644 --- a/src/ui/help.rs +++ b/src/ui/help.rs @@ -242,7 +242,7 @@ fn modal_hints(table: &[ModalBinding]) -> Vec<(String, &'static str /// applies), so only one of the two ever documents itself here at a time, /// exactly like `Action::ToggleStage`/`Action::ToggleAccept`'s mutual /// exclusion in [`binding_hidden`]. -fn modal_sections(modal_keys: &ModalKeymaps, review_session: bool) -> [Section; 18] { +fn modal_sections(modal_keys: &ModalKeymaps, review_session: bool) -> [Section; 19] { let staging_section = if review_session { ( "Accepted files panel (s, review sessions)", @@ -295,6 +295,10 @@ fn modal_sections(modal_keys: &ModalKeymaps, review_session: bool) -> [Section; "Submit review (U, PR review session)", modal_hints(&modal_keys.submit_forge), ), + ( + "Submit result (after a stopped submit)", + modal_hints(&modal_keys.submit_result), + ), ( "Cleanup finished reviews (X, Pull Requests tab)", modal_hints(&modal_keys.cleanup_reviews), @@ -1444,4 +1448,33 @@ mod tests { } } } + + /// The post-submit result modal's keys must be documented on "All keys". + /// [`modal_sections`] is a hand-written list, so dropping (or never + /// adding) its row would leave the modal's keys reachable but invisible in + /// `?` — the exact hidden-feature drift the CLAUDE.md rule forbids, and + /// one nothing else catches: the array length only pins the count, not + /// which tables are in it. + #[test] + fn the_submit_result_keys_are_documented_in_the_help_overlay() { + let keymap = Keymap::default_map(); + let modal_keys = ModalKeymaps::default(); + let rows = all_rows(&all_keys_sections( + keymap.bindings(), + &modal_keys, + true, + true, + true, + None, + )); + for binding in &modal_keys.submit_result { + let key = binding.key_label(); + assert!( + rows.iter() + .any(|(k, d)| *k == key && *d == binding.description), + "submit-result key {key:?} ({:?}) is missing from the help overlay", + binding.description + ); + } + } } diff --git a/src/ui/mod.rs b/src/ui/mod.rs index 64efea7..16767a2 100644 --- a/src/ui/mod.rs +++ b/src/ui/mod.rs @@ -44,6 +44,7 @@ mod file_view; mod footer; pub(crate) mod forge_open; mod forge_submit; +mod forge_submit_result; mod forge_threads; mod git_panel; mod help; @@ -396,6 +397,7 @@ fn dispatch_key( Mode::ConfirmRemoteOp { .. } => modes::handle_confirm_remote_op_key(app, key), Mode::ThreadView => modes::handle_thread_view_key(app, key), Mode::SubmitForge => modes::handle_submit_forge_key(app, key), + Mode::SubmitResult { .. } => modes::handle_submit_result_key(app, key), Mode::CleanupReviews { .. } => modes::handle_cleanup_reviews_key(app, key), Mode::ConfirmRestore { .. } => modes::handle_restore_key(app, key), Mode::Normal | Mode::Visual { .. } => { @@ -893,6 +895,9 @@ fn draw(frame: &mut ratatui::Frame, app: &App, keymap: &Keymap, pending: Option< if matches!(app.mode, Mode::SubmitForge) { forge_submit::render(frame, area, app); } + if matches!(app.mode, Mode::SubmitResult { .. }) { + forge_submit_result::render(frame, area, app); + } if matches!(app.mode, Mode::ConfirmRemoteOp { .. }) { confirm_remote_op_modal::render(frame, area, app); } diff --git a/src/ui/modal_keys.rs b/src/ui/modal_keys.rs index 2789e32..95021db 100644 --- a/src/ui/modal_keys.rs +++ b/src/ui/modal_keys.rs @@ -1362,6 +1362,95 @@ pub(super) static SUBMIT_FORGE_KEYS: LazyLock>> = + LazyLock::new(|| { + vec![ + ModalBinding { + description: "Submit again — retry everything that didn't land", + keys: vec![ModalKey::plain(KeyCode::Char('U'))], + action: SubmitResultAction::Retry, + footer: Some(FooterHint { + rank: 1, + label: "submit again", + }), + }, + ModalBinding { + description: "Dismiss the result view", + keys: vec![ + ModalKey::plain(KeyCode::Enter), + ModalKey::plain(KeyCode::Esc), + ModalKey::plain(KeyCode::Char('q')), + ], + action: SubmitResultAction::Dismiss, + footer: Some(FooterHint { + rank: 2, + label: "dismiss", + }), + }, + ModalBinding { + description: "Scroll the outcome list down", + keys: vec![ + ModalKey::plain(KeyCode::Char('j')), + ModalKey::plain(KeyCode::Down), + ], + action: SubmitResultAction::ScrollDown, + footer: Some(FooterHint { + rank: 3, + label: "scroll", + }), + }, + ModalBinding { + description: "Scroll the outcome list up", + keys: vec![ + ModalKey::plain(KeyCode::Char('k')), + ModalKey::plain(KeyCode::Up), + ], + action: SubmitResultAction::ScrollUp, + footer: None, + }, + ModalBinding { + description: "Scroll the outcome list down a page", + keys: vec![ModalKey::plain(KeyCode::PageDown)], + action: SubmitResultAction::PageDown, + footer: None, + }, + ModalBinding { + description: "Scroll the outcome list up a page", + keys: vec![ModalKey::plain(KeyCode::PageUp)], + action: SubmitResultAction::PageUp, + footer: None, + }, + ] + }); + // -- Pull/push confirm modal -------------------------------------------------- /// What a key does in the pull/push confirm modal (`p`/`P` in a review @@ -2924,6 +3013,9 @@ pub struct ModalKeymaps { /// The submit-review modal. Not config-remappable yet — see /// [`SUBMIT_FORGE_KEYS`]. pub(super) submit_forge: Vec>, + /// The post-submit result modal. Not config-remappable yet — see + /// [`SUBMIT_RESULT_KEYS`]. + pub(super) submit_result: Vec>, /// The finished-review cleanup confirm modal. Not config-remappable yet — /// see [`CLEANUP_REVIEWS_KEYS`]. pub(super) cleanup_reviews: Vec>, @@ -2958,6 +3050,7 @@ impl Default for ModalKeymaps { restore: RESTORE_KEYS.clone(), thread_view: THREAD_VIEW_KEYS.clone(), submit_forge: SUBMIT_FORGE_KEYS.clone(), + submit_result: SUBMIT_RESULT_KEYS.clone(), cleanup_reviews: CLEANUP_REVIEWS_KEYS.clone(), filter_edit: FILTER_EDIT_KEYS.clone(), } diff --git a/src/ui/modal_keys_config.rs b/src/ui/modal_keys_config.rs index d57d323..8171887 100644 --- a/src/ui/modal_keys_config.rs +++ b/src/ui/modal_keys_config.rs @@ -162,6 +162,7 @@ pub(super) fn effective_modal_keys( confirm_remote_op: modal_keys::CONFIRM_REMOTE_OP_KEYS.clone(), thread_view: modal_keys::THREAD_VIEW_KEYS.clone(), submit_forge: modal_keys::SUBMIT_FORGE_KEYS.clone(), + submit_result: modal_keys::SUBMIT_RESULT_KEYS.clone(), cleanup_reviews: modal_keys::CLEANUP_REVIEWS_KEYS.clone(), restore: modal_keys::RESTORE_KEYS.clone(), }; diff --git a/src/ui/modes.rs b/src/ui/modes.rs index 704860c..9332002 100644 --- a/src/ui/modes.rs +++ b/src/ui/modes.rs @@ -19,7 +19,8 @@ use super::modal_keys::{ self, AcceptedPanelAction, CleanupReviewsAction, CommitMessageAction, ComposeAction, ConfirmRemoteOpAction, EndReviewAction, FilterEditAction, FinderAction, LauncherAction, ListAction, PeekAction, ProjectSearchInputAction, ProjectSearchResultsAction, RestoreAction, - SearchAction, StagingAction, SubmitForgeAction, SwitcherAction, ThreadViewAction, + SearchAction, StagingAction, SubmitForgeAction, SubmitResultAction, SwitcherAction, + ThreadViewAction, }; use super::motion; @@ -617,6 +618,33 @@ pub(super) fn handle_submit_forge_key(app: &mut App, key: KeyEvent) { } } +/// Handles one key event while [`super::Mode::SubmitResult`] is active (the +/// post-submit per-item result view): a read-only account, so only scroll, +/// dismiss, and "submit again" — resolved against `app.modal_keys.submit_result`. +/// See [`modal_keys::SUBMIT_RESULT_KEYS`]. Nothing here writes to the forge; +/// Retry only reopens the submit modal, which keeps its own confirm gate. +pub(super) fn handle_submit_result_key(app: &mut App, key: KeyEvent) { + let count = match intercept_motion_count(app, key) { + MotionIntercept::Handled => return, + MotionIntercept::Resolve(count) => count, + }; + let Some(action) = modal_keys::resolve(&app.modal_keys.submit_result, key) else { + return; + }; + match action { + SubmitResultAction::ScrollDown => { + apply_motion_n_times(count, || app.submit_result_scroll_down()) + } + SubmitResultAction::ScrollUp => { + apply_motion_n_times(count, || app.submit_result_scroll_up()) + } + SubmitResultAction::PageDown => app.submit_result_page_down(), + SubmitResultAction::PageUp => app.submit_result_page_up(), + SubmitResultAction::Dismiss => app.close_submit_result(), + SubmitResultAction::Retry => app.submit_result_retry(), + } +} + /// Handles one key event while [`super::Mode::Finder`] is active (the fuzzy /// file finder overlay): printable chars extend the query (re-ranking on /// every keystroke, never remappable), and the control keys — Backspace, diff --git a/src/ui/stage_ops.rs b/src/ui/stage_ops.rs index ae4e802..b853b91 100644 --- a/src/ui/stage_ops.rs +++ b/src/ui/stage_ops.rs @@ -861,8 +861,12 @@ impl StageOps for GitRunner { None => match forge::mr_detail(number) { Ok(detail) => detail.diff_refs, Err(e) => { + // Nothing was written, so every item in the batch + // is unsent — recorded so the result view names + // them instead of showing an empty list. return forge::SubmitReport { failure: Some(forge::diagnose::submit_error_headline(&e)), + attempt: batch.attempt(), ..forge::SubmitReport::default() }; } diff --git a/src/ui/staging.rs b/src/ui/staging.rs index 52813ea..5a63402 100644 --- a/src/ui/staging.rs +++ b/src/ui/staging.rs @@ -268,6 +268,7 @@ impl App { | Mode::ConfirmRemoteOp { .. } | Mode::ThreadView | Mode::SubmitForge + | Mode::SubmitResult { .. } | Mode::CleanupReviews { .. } | Mode::ConfirmRestore { .. } => {} Mode::Normal | Mode::Visual { .. } => { From 775471d6e2341685c3cef674852eddb57dfad4df Mon Sep 17 00:00:00 2001 From: redquill test Date: Thu, 30 Jul 2026 02:28:10 -0500 Subject: [PATCH 5/6] feat(ui): per-entry selection in the finished-review cleanup Adds a cursor and per-entry checkbox to the finished-review cleanup modal (X on the Pull Requests tab): j/k/arrows move the highlight, Space toggles the highlighted entry (all checked by default), and confirm deletes only the selected subset. Zero-selected Enter is a no-op; deselected entries keep their unpublished-work warnings visible since the warning belongs to the entry, not to the deletion. New CleanupReviewsAction rows (MoveDown/MoveUp/Toggle) go through the shared CLEANUP_REVIEWS_KEYS table so the footer strip and ? help stay in sync automatically. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AWX3hBxudUxZooeph9ngEB --- src/ui/app.rs | 15 ++- src/ui/cleanup_integration_tests.rs | 80 +++++++++++++ src/ui/cleanup_reviews.rs | 92 ++++++++++++--- src/ui/cleanup_reviews_modal.rs | 134 +++++++++++++++++----- src/ui/cleanup_reviews_tests.rs | 120 +++++++++++++++++++- src/ui/modal_keys.rs | 167 ++++++++++++++++++++++++++-- src/ui/modes.rs | 15 ++- 7 files changed, 560 insertions(+), 63 deletions(-) diff --git a/src/ui/app.rs b/src/ui/app.rs index 6ff7fbd..e3a8967 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -180,8 +180,13 @@ pub enum Mode { /// Nothing is deleted until the reviewer confirms from here; `origin` is /// the launcher's own origin, threaded back through the reopened launcher /// on cancel/confirm. The enumerated snapshot lives in - /// [`App::cleanup_reviews`] (see [`super::cleanup_reviews`]). - CleanupReviews { origin: ModeOrigin }, + /// [`App::cleanup_reviews`], per-entry selection (all checked by default) + /// in [`App::cleanup_reviews_selected`] (see [`super::cleanup_reviews`]). + /// `cursor` is the highlighted row `Space` toggles and `j`/`k`/arrows + /// move — kept inline rather than on `App` since [`Mode`] is `Copy` and a + /// `usize` costs nothing, unlike the snapshot `Vec`s (see + /// [`Mode::ConfirmRestore`]'s doc on that split). + CleanupReviews { origin: ModeOrigin, cursor: usize }, /// The restore confirm modal (`restore-file`, `d`) is open: a binary gate /// naming the one file whose uncommitted changes are about to be thrown /// away. Nothing touches the repo until the reviewer confirms from here — @@ -557,6 +562,11 @@ pub struct App { /// at open time so a background list refresh can't shift the rows out from /// under the confirmation. Empty while the modal is closed. pub(super) cleanup_reviews: Vec, + /// Per-entry selection for [`App::cleanup_reviews`], same index, all + /// `true` at open (opt out rather than opt in). `Space` flips the entry + /// under `Mode::CleanupReviews`'s `cursor`; confirm deletes only the + /// `true` entries. Empty while the modal is closed. + pub(super) cleanup_reviews_selected: Vec, /// The file the restore confirm modal ([`Mode::ConfirmRestore`]) is /// asking about, frozen at open time so a background refresh can't shift /// which path a confirm lands on. `None` while the modal is closed — see @@ -892,6 +902,7 @@ impl App { launcher_prs_tasks: BackgroundTasks::new(), launcher_finished_reviews: Vec::new(), cleanup_reviews: Vec::new(), + cleanup_reviews_selected: Vec::new(), restore_request: None, pr_checkout_in_flight: None, pr_checkout_tasks: BackgroundTasks::new(), diff --git a/src/ui/cleanup_integration_tests.rs b/src/ui/cleanup_integration_tests.rs index 7579a03..f9540c4 100644 --- a/src/ui/cleanup_integration_tests.rs +++ b/src/ui/cleanup_integration_tests.rs @@ -377,6 +377,86 @@ fn a_dirty_worktree_fails_that_entry_and_the_run_continues() { drop(bare); } +// -- Partial selection (Space-deselected entries survive a confirm) --------- + +#[test] +fn confirm_with_a_deselected_entry_deletes_only_the_selected_subset() { + let bare = setup_bare_origin(); + let contributor = clone_of(bare.path()); + push_pr_special_ref(contributor.path(), "feat1", 1, "one"); + push_pr_special_ref(contributor.path(), "feat2", 2, "two"); + + let reviewer = clone_of(bare.path()); + create_pr_checkout(reviewer.path(), 1); + create_pr_checkout(reviewer.path(), 2); + + let mut app = origin_app_with_open_listing(reviewer.path(), &[]); + assert_eq!(app.launcher_finished_reviews.len(), 2); + app.open_cleanup_reviews(); + assert_eq!( + app.cleanup_reviews_selected, + vec![true, true], + "both entries start selected" + ); + + // Deselect whichever row is #1 (the snapshot's entry order isn't a + // contract this test should pin to) and confirm. + let deselect_index = app + .cleanup_reviews + .iter() + .position(|e| e.number == 1) + .expect("PR #1 must be in the snapshot"); + app.cleanup_reviews_selected[deselect_index] = false; + app.confirm_cleanup_reviews(); + + // #1 was deselected: its worktree, branch, and state entry all survive. + assert!( + branch_exists(reviewer.path(), "redquill/pr/1"), + "the deselected entry's branch must survive" + ); + assert!( + worktree_list(reviewer.path()).contains("redquill/pr/1"), + "the deselected entry's worktree must survive" + ); + assert!( + store::load(&state_path_of(reviewer.path())) + .reviews + .contains_key("redquill/pr/1"), + "the deselected entry's state entry must survive" + ); + // #2 was selected: fully cleaned up. + assert!( + !branch_exists(reviewer.path(), "redquill/pr/2"), + "the selected entry must be cleaned up" + ); + assert!( + !worktree_list(reviewer.path()).contains("redquill/pr/2"), + "the selected entry's worktree must be removed" + ); + assert!( + !store::load(&state_path_of(reviewer.path())) + .reviews + .contains_key("redquill/pr/2"), + "the selected entry's state entry must be deleted" + ); + assert!( + app.status_message + .as_deref() + .is_some_and(|m| m.contains("cleaned up 1")), + "summary must report only the one deletion actually run: {:?}", + app.status_message + ); + // The deselected review is still finished (still unmerged/not open), so + // it reappears in the recomputed set rather than vanishing silently. + assert_eq!( + app.launcher_finished_reviews.len(), + 1, + "the deselected review must reappear in the finished set" + ); + assert_eq!(app.launcher_finished_reviews[0].number, 1); + drop(bare); +} + // -- Journey transcript (spec 13 task 5.0 proof) ----------------------------- /// Journey generator for spec 13 task 5.0: on a real scratch repo, checks out diff --git a/src/ui/cleanup_reviews.rs b/src/ui/cleanup_reviews.rs index a4d8ab0..f08ca4d 100644 --- a/src/ui/cleanup_reviews.rs +++ b/src/ui/cleanup_reviews.rs @@ -1,7 +1,8 @@ //! The finished-review cleanup modal's state transitions //! ([`super::app::Mode::CleanupReviews`]): opening it from the Pull Requests -//! tab, cancelling back into the launcher, and — on confirm — deleting each -//! finished review's managed worktree, branch, and persisted state entry. +//! tab, moving/toggling its per-entry selection, cancelling back into the +//! launcher, and — on confirm — deleting each *selected* finished review's +//! managed worktree, branch, and persisted state entry. //! //! Modeled on [`super::end_review`]'s finish path, which this mirrors for the //! single-review case: the deletion runs synchronously on the render thread @@ -11,7 +12,9 @@ //! worktree), prunes, deletes the `redquill/pr/` branch through the //! prefix-confined helper, and deletes the review's state entry — all //! per-entry, so one dirty or locked worktree fails just that entry and the -//! run continues to the next, ending in a one-line outcome summary. +//! run continues to the next, ending in a one-line outcome summary. Entries +//! left unchecked are never touched — they simply reappear the next time the +//! Pull Requests tab's finished set is recomputed. use crate::review::FinishedReview; @@ -27,9 +30,10 @@ impl App { /// checkout is in flight (deletion mutates the same worktrees/branches /// those touch). On success it snapshots the finished set into /// [`App::cleanup_reviews`] (frozen so a background list refresh can't - /// shift the rows mid-confirmation) and switches to - /// [`Mode::CleanupReviews`], carrying the launcher's own origin so - /// cancel/confirm can reopen it exactly. + /// shift the rows mid-confirmation), marks every entry selected in + /// [`App::cleanup_reviews_selected`], and switches to + /// [`Mode::CleanupReviews`] with the cursor on the first row, carrying the + /// launcher's own origin so cancel/confirm can reopen it exactly. pub(super) fn open_cleanup_reviews(&mut self) { let Mode::ReviewLauncher { tab: LauncherTab::PullRequests, @@ -54,7 +58,8 @@ impl App { return; } self.cleanup_reviews = self.launcher_finished_reviews.clone(); - self.mode = Mode::CleanupReviews { origin }; + self.cleanup_reviews_selected = vec![true; self.cleanup_reviews.len()]; + self.mode = Mode::CleanupReviews { origin, cursor: 0 }; } /// Closes the cleanup modal without deleting anything, reopening the @@ -62,25 +67,80 @@ impl App { /// from. Declining mutates nothing on disk. A no-op outside /// [`Mode::CleanupReviews`]. pub(super) fn cancel_cleanup_reviews(&mut self) { - let Mode::CleanupReviews { origin } = self.mode else { + let Mode::CleanupReviews { origin, .. } = self.mode else { return; }; self.cleanup_reviews.clear(); + self.cleanup_reviews_selected.clear(); self.reopen_launcher_after_cleanup(origin); } - /// Confirms the cleanup: deletes every enumerated finished review's - /// worktree, branch, and state entry (see [`App::run_cleanup_deletions`]), - /// recomputes the finished set from the still-current listing (a cleanup - /// never changes which PRs are open, so no re-fetch is needed), reopens - /// the launcher, and surfaces the per-entry outcome summary. A no-op - /// outside [`Mode::CleanupReviews`]. + /// Moves the cleanup modal's highlighted row down one entry, clamped at + /// the last. A no-op outside [`Mode::CleanupReviews`] or on an empty list. + pub(super) fn cleanup_reviews_move_down(&mut self) { + self.cleanup_reviews_step(1, true); + } + + /// Moves the cleanup modal's highlighted row up one entry, clamped at the + /// first. A no-op outside [`Mode::CleanupReviews`] or on an empty list. + pub(super) fn cleanup_reviews_move_up(&mut self) { + self.cleanup_reviews_step(1, false); + } + + fn cleanup_reviews_step(&mut self, step: usize, down: bool) { + let len = self.cleanup_reviews.len(); + if let Mode::CleanupReviews { cursor, .. } = &mut self.mode { + *cursor = super::motion::step(*cursor, len, step, down); + } + } + + /// Toggles the highlighted entry's selection (`Space`). Every entry + /// starts selected at open, so this is how a reviewer opts one out of a + /// batch delete rather than opting individual ones in. A no-op outside + /// [`Mode::CleanupReviews`] or on an empty list. + pub(super) fn toggle_cleanup_review_selection(&mut self) { + let Mode::CleanupReviews { cursor, .. } = self.mode else { + return; + }; + if let Some(slot) = self.cleanup_reviews_selected.get_mut(cursor) { + *slot = !*slot; + } + } + + /// The number of entries currently checked — drives the modal's "N of M + /// selected" title and confirm hint. + pub(super) fn cleanup_reviews_selected_count(&self) -> usize { + self.cleanup_reviews_selected + .iter() + .filter(|&&selected| selected) + .count() + } + + /// Confirms the cleanup: deletes every *selected* enumerated finished + /// review's worktree, branch, and state entry (see + /// [`App::run_cleanup_deletions`]), recomputes the finished set from the + /// still-current listing (a cleanup never changes which PRs are open, so + /// no re-fetch is needed), reopens the launcher, and surfaces the + /// per-entry outcome summary. With nothing selected this is a no-op — the + /// modal stays open exactly as it was, deleting nothing. Deselected + /// entries are simply dropped from the snapshot on confirm: they were + /// never touched, so they reappear the next time the finished set is + /// recomputed. A no-op outside [`Mode::CleanupReviews`]. pub(super) fn confirm_cleanup_reviews(&mut self) { - let Mode::CleanupReviews { origin } = self.mode else { + let Mode::CleanupReviews { origin, .. } = self.mode else { return; }; + if self.cleanup_reviews_selected_count() == 0 { + return; + } let entries = std::mem::take(&mut self.cleanup_reviews); - let summary = self.run_cleanup_deletions(&entries); + let selected = std::mem::take(&mut self.cleanup_reviews_selected); + let chosen: Vec = entries + .into_iter() + .zip(selected) + .filter_map(|(entry, keep)| keep.then_some(entry)) + .collect(); + let summary = self.run_cleanup_deletions(&chosen); // The open-PR set is unchanged by a cleanup, so recomputing against // the already-loaded listing (now with the deleted branches/state // entries gone) is enough — no network round-trip. diff --git a/src/ui/cleanup_reviews_modal.rs b/src/ui/cleanup_reviews_modal.rs index 6f19e27..e16cdb3 100644 --- a/src/ui/cleanup_reviews_modal.rs +++ b/src/ui/cleanup_reviews_modal.rs @@ -1,17 +1,22 @@ //! The finished-review cleanup confirm modal //! ([`super::app::Mode::CleanupReviews`]): a centered, bordered overlay that -//! enumerates every finished review about to be deleted — PR number/title, -//! worktree path, and an explicit unpublished-work warning when nonzero — with -//! a confirm/cancel hint line below. Nothing is deleted until the reviewer -//! confirms; the modal is the safety boundary, so it names exactly what a -//! confirm removes. Reads [`App::cleanup_reviews`]; renders nothing outside +//! enumerates every finished review — PR number/title, worktree path, an +//! explicit unpublished-work warning when nonzero, and a `[x]`/`[ ]` checkbox +//! — with the highlighted row reverse-styled and a confirm/cancel hint line +//! below. Every entry starts checked; `Space` toggles the highlighted one and +//! confirm deletes only the checked entries. Nothing is deleted until the +//! reviewer confirms; the modal is the safety boundary, so it names exactly +//! what a confirm removes, and a deselected entry keeps its unpublished-work +//! warning visible — the warning belongs to the entry, not to whether it's +//! slated for deletion. Reads [`App::cleanup_reviews`] and +//! [`App::cleanup_reviews_selected`]; renders nothing outside //! [`Mode::CleanupReviews`]. use ratatui::Frame; use ratatui::layout::{Constraint, Layout, Rect}; use ratatui::style::{Modifier, Style}; use ratatui::text::{Line, Span}; -use ratatui::widgets::{Block, Borders, Clear, List, ListItem, Padding, Paragraph}; +use ratatui::widgets::{Block, Borders, Clear, List, ListItem, ListState, Padding, Paragraph}; use crate::review::FinishedReview; @@ -20,14 +25,18 @@ use super::help::centered; use super::modal_keys::CleanupReviewsAction; use super::theme::Theme; -/// One finished-review entry's rows: a primary `#N title` line, a secondary -/// worktree-path line, and — only when nonzero — a warning line naming the -/// unpublished annotation/reply count that a delete would discard. -fn entry_items(entry: &FinishedReview, theme: &Theme) -> Vec> { +/// One finished-review entry's rows: a `[x]`/`[ ]` checkbox on the primary +/// `#N title` line, a secondary worktree-path line, and — only when +/// nonzero — a warning line naming the unpublished annotation/reply count a +/// delete would discard. The warning is unconditional on `entry`, never on +/// `selected`: an unchecked entry keeps its warning visible, since it names +/// what the entry is carrying, not what's about to happen to it. +fn entry_items(entry: &FinishedReview, selected: bool, theme: &Theme) -> Vec> { + let checkbox = if selected { "[x] " } else { "[ ] " }; let title = if entry.title.is_empty() { - format!("#{}", entry.number) + format!("{checkbox}#{}", entry.number) } else { - format!("#{} {}", entry.number, entry.title) + format!("{checkbox}#{} {}", entry.number, entry.title) }; let mut lines = vec![ Line::from(Span::styled( @@ -35,14 +44,14 @@ fn entry_items(entry: &FinishedReview, theme: &Theme) -> Vec> Style::default().add_modifier(Modifier::BOLD), )), Line::from(Span::styled( - format!(" {}", entry.worktree_path.display()), + format!(" {}", entry.worktree_path.display()), Style::default().fg(theme.footer_text), )), ]; if entry.unpublished_count > 0 { lines.push(Line::from(Span::styled( format!( - " \u{26a0} {} unpublished comment(s)/reply(ies) will be discarded", + " \u{26a0} {} unpublished comment(s)/reply(ies) will be discarded", entry.unpublished_count ), Style::default().fg(theme.status_message), @@ -51,8 +60,10 @@ fn entry_items(entry: &FinishedReview, theme: &Theme) -> Vec> vec![ListItem::new(lines)] } -/// The confirm/cancel hint line, keys read from the effective table so a remap -/// shows up here with no extra wiring. +/// The confirm/cancel hint line, keys read from the effective table so a +/// remap shows up here with no extra wiring. The confirm hint names the +/// selected count (`delete selected (N)`) and dims to `footer_text` with +/// nothing selected, since `Enter` is a no-op there. fn hint_line(app: &App) -> Line<'static> { let key = |action: CleanupReviewsAction| { app.modal_keys @@ -62,14 +73,17 @@ fn hint_line(app: &App) -> Line<'static> { .map(|b| b.key_label()) .unwrap_or_default() }; + let selected = app.cleanup_reviews_selected_count(); + let confirm_style = if selected == 0 { + Style::default().fg(app.theme.footer_text) + } else { + Style::default() + .fg(app.theme.help_key) + .add_modifier(Modifier::BOLD) + }; Line::from(vec![ - Span::styled( - key(CleanupReviewsAction::Confirm), - Style::default() - .fg(app.theme.help_key) - .add_modifier(Modifier::BOLD), - ), - Span::raw(" delete "), + Span::styled(key(CleanupReviewsAction::Confirm), confirm_style), + Span::styled(format!(" delete selected ({selected}) "), confirm_style), Span::styled( key(CleanupReviewsAction::Cancel), Style::default() @@ -100,12 +114,15 @@ pub fn render(frame: &mut Frame, area: Rect, app: &App) { frame.render_widget(Clear, popup); let count = app.cleanup_reviews.len(); + let selected = app.cleanup_reviews_selected_count(); let block = Block::default() .borders(Borders::ALL) .padding(Padding::horizontal(1)) - .title(format!(" Clean up {count} finished review(s) ")) + .title(format!( + " Clean up finished review(s): {selected} of {count} selected " + )) .title_bottom(Line::from( - " delete removes worktree, branch, and saved state ", + " Space toggles \u{2014} delete removes worktree, branch, and saved state ", )); let inner = block.inner(popup); frame.render_widget(block, popup); @@ -117,12 +134,27 @@ pub fn render(frame: &mut Frame, area: Rect, app: &App) { ]) .split(inner); + let cursor = match app.mode { + Mode::CleanupReviews { cursor, .. } => cursor, + _ => 0, + }; let items: Vec> = app .cleanup_reviews .iter() - .flat_map(|entry| entry_items(entry, &app.theme)) + .enumerate() + .flat_map(|(i, entry)| { + let selected = app + .cleanup_reviews_selected + .get(i) + .copied() + .unwrap_or(false); + entry_items(entry, selected, &app.theme) + }) .collect(); - frame.render_widget(List::new(items), rows[0]); + let list = List::new(items).highlight_style(Style::default().add_modifier(Modifier::REVERSED)); + let mut list_state = ListState::default(); + list_state.select(Some(cursor)); + frame.render_stateful_widget(list, rows[0], &mut list_state); frame.render_widget(Paragraph::new(hint_line(app)), rows[2]); } @@ -172,9 +204,11 @@ index 111..222 100644 fn cleanup_app(entries: Vec) -> App { let mut app = App::new(vec![sample_file()]); + app.cleanup_reviews_selected = vec![true; entries.len()]; app.cleanup_reviews = entries; app.mode = Mode::CleanupReviews { origin: ModeOrigin::Normal, + cursor: 0, }; app } @@ -224,4 +258,50 @@ index 111..222 100644 let content = render_modal(&app); assert!(!content.contains("unpublished")); } + + #[test] + fn every_entry_is_checked_by_default_and_the_title_names_the_full_count() { + let app = cleanup_app(vec![finished(1, "a", 0), finished(2, "b", 0)]); + let content = render_modal(&app); + assert_eq!( + content.matches("[x]").count(), + 2, + "both entries start checked: {content}" + ); + assert!( + content.contains("2 of 2 selected"), + "title must show the full count selected by default: {content}" + ); + } + + #[test] + fn a_deselected_entry_renders_unchecked_and_keeps_its_warning() { + let mut app = cleanup_app(vec![finished(1, "a", 3), finished(2, "b", 0)]); + app.cleanup_reviews_selected[0] = false; + let content = render_modal(&app); + assert!( + content.contains("[ ]"), + "entry 0 must render unchecked: {content}" + ); + assert!(content.contains("[x]"), "entry 1 stays checked: {content}"); + assert!( + content.contains("3 unpublished"), + "a deselected entry still names its unpublished-work warning: {content}" + ); + assert!( + content.contains("1 of 2 selected"), + "the title must reflect the deselected entry: {content}" + ); + } + + #[test] + fn zero_selected_confirm_hint_still_names_the_zero_count() { + let mut app = cleanup_app(vec![finished(1, "a", 0)]); + app.cleanup_reviews_selected[0] = false; + let content = render_modal(&app); + assert!( + content.contains("delete selected (0)"), + "the confirm hint must show a zero count rather than pretend nothing changed: {content}" + ); + } } diff --git a/src/ui/cleanup_reviews_tests.rs b/src/ui/cleanup_reviews_tests.rs index d6165f2..e6ffea4 100644 --- a/src/ui/cleanup_reviews_tests.rs +++ b/src/ui/cleanup_reviews_tests.rs @@ -1,6 +1,7 @@ //! Tests for the finished-review cleanup modal's open/cancel state -//! transitions; the confirmed deletion sequence is covered by the real-git -//! tempdir integration tests. +//! transitions and per-entry selection (cursor move, toggle, zero-selected +//! confirm); the confirmed deletion sequence — including a partial-selection +//! subset — is covered by the real-git tempdir integration tests. use std::path::PathBuf; @@ -66,7 +67,8 @@ fn open_from_prs_tab_with_finished_reviews_enters_cleanup_mode() { assert_eq!( app.mode, Mode::CleanupReviews { - origin: ModeOrigin::Normal + origin: ModeOrigin::Normal, + cursor: 0, } ); assert_eq!( @@ -74,6 +76,11 @@ fn open_from_prs_tab_with_finished_reviews_enters_cleanup_mode() { 1, "the snapshot is frozen at open" ); + assert_eq!( + app.cleanup_reviews_selected, + vec![true], + "every entry starts selected" + ); } #[test] @@ -119,4 +126,111 @@ fn cancel_returns_to_the_prs_tab_and_deletes_nothing() { } )); assert!(app.cleanup_reviews.is_empty()); + assert!(app.cleanup_reviews_selected.is_empty()); +} + +// -- Per-entry selection: cursor + toggle ------------------------------------ + +fn opened_cleanup_app(count: u64) -> App { + let mut app = prs_launcher_app(); + app.launcher_finished_reviews = (1..=count) + .map(|n| finished(n, &format!("/tmp/wt{n}"), 0)) + .collect(); + app.open_cleanup_reviews(); + app +} + +#[test] +fn space_toggles_the_highlighted_entry_and_the_count_reflects_it() { + let mut app = opened_cleanup_app(3); + assert_eq!( + app.cleanup_reviews_selected_count(), + 3, + "all selected at open" + ); + + app.toggle_cleanup_review_selection(); + + assert_eq!(app.cleanup_reviews_selected, vec![false, true, true]); + assert_eq!(app.cleanup_reviews_selected_count(), 2); + + // Toggling again flips it back. + app.toggle_cleanup_review_selection(); + assert_eq!(app.cleanup_reviews_selected, vec![true, true, true]); +} + +#[test] +fn move_down_and_up_walk_the_cursor_and_toggle_follows_it() { + let mut app = opened_cleanup_app(3); + + app.cleanup_reviews_move_down(); + app.toggle_cleanup_review_selection(); + + assert_eq!( + app.cleanup_reviews_selected, + vec![true, false, true], + "toggle must act on entry 1 (where the cursor moved to), not entry 0" + ); + + app.cleanup_reviews_move_up(); + app.toggle_cleanup_review_selection(); + assert_eq!( + app.cleanup_reviews_selected, + vec![false, false, true], + "moving back up and toggling must act on entry 0 again" + ); +} + +#[test] +fn cursor_movement_is_clamped_at_both_ends() { + let mut app = opened_cleanup_app(2); + assert_eq!( + app.mode, + Mode::CleanupReviews { + origin: ModeOrigin::Normal, + cursor: 0, + } + ); + + app.cleanup_reviews_move_up(); + assert_eq!( + app.mode, + Mode::CleanupReviews { + origin: ModeOrigin::Normal, + cursor: 0, + }, + "moving up from the first row stays pinned at 0" + ); + + app.cleanup_reviews_move_down(); + app.cleanup_reviews_move_down(); + app.cleanup_reviews_move_down(); + assert_eq!( + app.mode, + Mode::CleanupReviews { + origin: ModeOrigin::Normal, + cursor: 1, + }, + "moving past the last row stays pinned at the last index" + ); +} + +#[test] +fn zero_selected_confirm_is_a_no_op() { + let mut app = opened_cleanup_app(1); + app.toggle_cleanup_review_selection(); + assert_eq!(app.cleanup_reviews_selected_count(), 0); + + app.confirm_cleanup_reviews(); + + assert!( + matches!(app.mode, Mode::CleanupReviews { .. }), + "the modal must stay open with nothing selected" + ); + assert_eq!( + app.cleanup_reviews.len(), + 1, + "the snapshot must be untouched by a no-op confirm" + ); + assert!(app.status_message.is_none(), "no summary for a no-op"); } diff --git a/src/ui/modal_keys.rs b/src/ui/modal_keys.rs index 95021db..ab8dbdb 100644 --- a/src/ui/modal_keys.rs +++ b/src/ui/modal_keys.rs @@ -1553,14 +1553,26 @@ pub(super) static RESTORE_KEYS: LazyLock>> = Laz /// What a key does in the finished-review cleanup confirm modal /// ([`super::app::Mode::CleanupReviews`], opened by `cleanup-finished-reviews` -/// on the Pull Requests tab): a plain confirm/cancel gate over the enumerated -/// finished reviews. Confirm deletes every listed review's worktree, branch, -/// and state entry; cancel mutates nothing. Not config-remappable yet — see -/// module doc. +/// on the Pull Requests tab): `j`/`k`/arrows move the highlight, `Space` +/// toggles the highlighted entry's selection (all checked by default), and +/// confirm/cancel gate the batch. Confirm deletes only the *selected* +/// reviews' worktree, branch, and state entry (a no-op with nothing +/// selected); cancel mutates nothing. Not config-remappable yet — see module +/// doc. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum CleanupReviewsAction { - /// Deletes the enumerated finished reviews (see - /// [`super::app::App::confirm_cleanup_reviews`]). + /// Moves the highlight down one entry (see + /// [`super::app::App::cleanup_reviews_move_down`]). + MoveDown, + /// Moves the highlight up one entry (see + /// [`super::app::App::cleanup_reviews_move_up`]). + MoveUp, + /// Toggles the highlighted entry's selection (see + /// [`super::app::App::toggle_cleanup_review_selection`]). + Toggle, + /// Deletes every selected finished review (see + /// [`super::app::App::confirm_cleanup_reviews`]); a no-op with nothing + /// selected. Confirm, /// Closes the modal, deleting nothing (see /// [`super::app::App::cancel_cleanup_reviews`]). @@ -1568,13 +1580,46 @@ pub(super) enum CleanupReviewsAction { } /// The cleanup confirm modal's key table, for the help overlay, footer strip, -/// and [`super::modes::handle_cleanup_reviews_key`]'s dispatch. Mirrors -/// [`CONFIRM_REMOTE_OP_KEYS`]' binary-gate shape. +/// and [`super::modes::handle_cleanup_reviews_key`]'s dispatch. The +/// confirm/cancel rows mirror [`CONFIRM_REMOTE_OP_KEYS`]' binary-gate shape; +/// the move pair mirrors [`SWITCHER_KEYS`]' — only `MoveDown` carries the +/// footer hint, since its label ("j / Down") already reads as a compound key +/// display and merging `MoveUp`'s in would double the " / " separators. pub(super) static CLEANUP_REVIEWS_KEYS: LazyLock>> = LazyLock::new(|| { vec![ ModalBinding { - description: "Delete the finished reviews (worktree, branch, saved state)", + description: "Move the highlight down", + keys: vec![ + ModalKey::plain(KeyCode::Char('j')), + ModalKey::plain(KeyCode::Down), + ], + action: CleanupReviewsAction::MoveDown, + footer: Some(FooterHint { + rank: 3, + label: "move", + }), + }, + ModalBinding { + description: "Move the highlight up", + keys: vec![ + ModalKey::plain(KeyCode::Char('k')), + ModalKey::plain(KeyCode::Up), + ], + action: CleanupReviewsAction::MoveUp, + footer: None, + }, + ModalBinding { + description: "Toggle the highlighted entry's selection", + keys: vec![ModalKey::plain(KeyCode::Char(' '))], + action: CleanupReviewsAction::Toggle, + footer: Some(FooterHint { + rank: 2, + label: "toggle", + }), + }, + ModalBinding { + description: "Delete the selected finished reviews (worktree, branch, saved state)", keys: vec![ ModalKey::plain(KeyCode::Enter), ModalKey::plain(KeyCode::Char('y')), @@ -1593,7 +1638,7 @@ pub(super) static CLEANUP_REVIEWS_KEYS: LazyLock App { + let mut app = app(); + app.mode = Mode::ReviewLauncher { + tab: crate::ui::review_launcher::LauncherTab::PullRequests, + cursor: 0, + origin: crate::ui::app::ModeOrigin::Normal, + }; + app.launcher_finished_reviews = vec![ + crate::review::FinishedReview { + branch: "redquill/pr/1".to_string(), + number: 1, + title: "one".to_string(), + provider: crate::review::store::ForgeProviderKind::GitHub, + host: "github.com".to_string(), + worktree_path: PathBuf::from("/tmp/wt1"), + unpublished_count: 0, + }, + crate::review::FinishedReview { + branch: "redquill/pr/2".to_string(), + number: 2, + title: "two".to_string(), + provider: crate::review::store::ForgeProviderKind::GitHub, + host: "github.com".to_string(), + worktree_path: PathBuf::from("/tmp/wt2"), + unpublished_count: 0, + }, + ]; + app.open_cleanup_reviews(); + app + } + + #[test] + fn every_cleanup_reviews_table_entry_drives_its_documented_action() { + use crate::ui::modes::handle_cleanup_reviews_key; + + for binding in CLEANUP_REVIEWS_KEYS.iter() { + for key in &binding.keys { + let mut app = cleanup_reviews_app(); + let label = binding.key_label(); + match binding.action { + CleanupReviewsAction::MoveDown => { + handle_cleanup_reviews_key(&mut app, key.event()); + assert_eq!( + app.mode, + Mode::CleanupReviews { + origin: crate::ui::app::ModeOrigin::Normal, + cursor: 1, + }, + "Cleanup reviews {label}: must move the highlight down" + ); + } + CleanupReviewsAction::MoveUp => { + app.cleanup_reviews_move_down(); + handle_cleanup_reviews_key(&mut app, key.event()); + assert_eq!( + app.mode, + Mode::CleanupReviews { + origin: crate::ui::app::ModeOrigin::Normal, + cursor: 0, + }, + "Cleanup reviews {label}: must move the highlight back up" + ); + } + CleanupReviewsAction::Toggle => { + handle_cleanup_reviews_key(&mut app, key.event()); + assert_eq!( + app.cleanup_reviews_selected, + vec![false, true], + "Cleanup reviews {label}: must toggle the highlighted (first) entry" + ); + } + CleanupReviewsAction::Confirm => { + handle_cleanup_reviews_key(&mut app, key.event()); + assert!( + matches!(app.mode, Mode::ReviewLauncher { .. }), + "Cleanup reviews {label}: confirm must close back to the launcher" + ); + assert!( + app.cleanup_reviews.is_empty(), + "Cleanup reviews {label}: confirm must clear the snapshot" + ); + } + CleanupReviewsAction::Cancel => { + handle_cleanup_reviews_key(&mut app, key.event()); + assert!( + matches!(app.mode, Mode::ReviewLauncher { .. }), + "Cleanup reviews {label}: cancel must close back to the launcher" + ); + assert!( + app.cleanup_reviews.is_empty(), + "Cleanup reviews {label}: cancel must clear the snapshot without deleting" + ); + } + } + } + } + } + #[test] fn every_help_table_entry_drives_its_documented_action() { use super::super::help::HelpTab; diff --git a/src/ui/modes.rs b/src/ui/modes.rs index 9332002..8d06a8c 100644 --- a/src/ui/modes.rs +++ b/src/ui/modes.rs @@ -509,16 +509,21 @@ pub(super) fn handle_restore_key(app: &mut App, key: KeyEvent) { } } -/// Handles one key event while [`super::Mode::CleanupReviews`] is active: a -/// binary confirm/cancel gate over the enumerated finished reviews. Confirm -/// deletes each review's worktree, branch, and state entry; cancel closes back -/// into the launcher, deleting nothing. Resolved against -/// `app.modal_keys.cleanup_reviews` (see [`modal_keys::CLEANUP_REVIEWS_KEYS`]). +/// Handles one key event while [`super::Mode::CleanupReviews`] is active: +/// `j`/`k`/arrows move the highlight, `Space` toggles the highlighted entry's +/// selection, and confirm/cancel gate the batch. Confirm deletes each +/// selected review's worktree, branch, and state entry (a no-op with nothing +/// selected); cancel closes back into the launcher, deleting nothing. +/// Resolved against `app.modal_keys.cleanup_reviews` (see +/// [`modal_keys::CLEANUP_REVIEWS_KEYS`]). pub(super) fn handle_cleanup_reviews_key(app: &mut App, key: KeyEvent) { let Some(action) = modal_keys::resolve(&app.modal_keys.cleanup_reviews, key) else { return; }; match action { + CleanupReviewsAction::MoveDown => app.cleanup_reviews_move_down(), + CleanupReviewsAction::MoveUp => app.cleanup_reviews_move_up(), + CleanupReviewsAction::Toggle => app.toggle_cleanup_review_selection(), CleanupReviewsAction::Confirm => app.confirm_cleanup_reviews(), CleanupReviewsAction::Cancel => app.cancel_cleanup_reviews(), } From bbd6b0a8f17398e8ea2fbd27e23a056b13d093b3 Mon Sep 17 00:00:00 2001 From: redquill test Date: Thu, 30 Jul 2026 02:45:57 -0500 Subject: [PATCH 6/6] feat(config): remappable thread, submit, result, and cleanup modal keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Review launcher already honored `[keys.review-launcher]`, but the four PR-flow modals were pinned to their compiled-in tables, against the repo's "keymap is data" convention. Each gains a bijective action-name pair and routes through the same `apply_modal_overrides` merge, so `[keys.thread-view]`, `[keys.submit-forge]`, `[keys.submit-result]`, and `[keys.cleanup-reviews]` get identical grammar, replace/unbind/collision semantics, and warning surface. The `?` overlay and footer strip already read the effective tables, so remaps reach both with no extra wiring. Submit-forge is free-text: its table is consulted before the char-insert fallback, so binding a control action to a bare printable key takes that character away from summary typing. Allowed rather than rejected — the same trade every other free-text mode here already accepts — and documented in the example config instead of encoded as a special-case validation rule. The example config's doc-drift test now reads each mode's expected action set off its default table rather than a hand-kept parallel list; that list had gone stale in step with the doc it polices, which is how the launcher's `toggle-all-commits` and `cleanup-finished-reviews` went undocumented. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AWX3hBxudUxZooeph9ngEB --- docs/example-config.toml | 58 +++- src/config/keys.rs | 6 +- src/ui/footer_tests.rs | 46 ++++ src/ui/forge_submit_tests.rs | 33 +++ src/ui/modal_keys.rs | 172 ++++++++++-- src/ui/modal_keys_config.rs | 54 +++- src/ui/modal_keys_config_tests.rs | 422 ++++++++++++++++++------------ 7 files changed, 583 insertions(+), 208 deletions(-) diff --git a/docs/example-config.toml b/docs/example-config.toml index 8ea7c1c..7fbfc00 100644 --- a/docs/example-config.toml +++ b/docs/example-config.toml @@ -321,7 +321,9 @@ literal = false # the help overlay's scroll keys and its own `/` filter, the shared `/` # list-filter editing sub-state (annotation list / staging / accepted panel # / switcher, spec 12), Compose, the commit-message modal, the diff-view `/` -# search input, the fuzzy file finder, and the two Project Search focuses) — +# search input, the fuzzy file finder, the two Project Search focuses, the +# imported-thread overlay, the submit-review modal, the post-submit result +# view, and the finished-review cleanup modal) — # has its own `[keys.]` table, with the # identical grammar and merge semantics as `[keys.diff]`/`[keys.panel]` # above: `action-name = ""` or `= ["", ...]`, an override @@ -329,13 +331,18 @@ literal = false # its defaults, `= []` unbinds it, and a same-table collision is won by # the override (with a warning). # -# Free-text modes (Compose, the commit-message modal, Search, Finder, and -# both Project Search focuses) type printable characters into a buffer or -# query — that character insertion is never an action and can't be bound; -# only the *control* keys listed below (Enter/Esc/arrows/Backspace/...) -# are actions. -# -# The fourteen mode names and their complete action lists, each showing its +# Free-text modes (Compose, the commit-message modal, Search, Finder, both +# Project Search focuses, and the submit-review modal's summary field) type +# printable characters into a buffer or query — that character insertion is +# never an action and can't be bound; only the *control* keys listed below +# (Enter/Esc/arrows/Backspace/...) are actions. The table is consulted +# before the character-insert fallback, so binding a control action of one +# of those modes to a bare printable key is accepted but takes that +# character away from typing — e.g. `[keys.submit-forge] cancel = "q"` means +# you can no longer type `q` into the review summary. Prefer a modified key +# (`ctrl-`/`alt-`) or a named key in a free-text mode. +# +# The eighteen mode names and their complete action lists, each showing its # default key(s): # # [keys.list] # Annotation list panel (`a`) @@ -404,6 +411,8 @@ literal = false # jump-to-bottom = ["G", "end"] # Jump to bottom # confirm = "enter" # Confirm the highlighted row # enter-filter = "/" # Filter (fuzzy, narrows the active tab) +# toggle-all-commits = "a" # Commits tab: ahead-of-base <-> full recent-HEAD log +# cleanup-finished-reviews = "X" # Pull Requests tab: clean up finished reviews # close = "esc" # Close # # [keys.help] # Help overlay scroll/close/filter-open keys @@ -501,6 +510,39 @@ literal = false # toggle-whole-word = "alt-w" # Toggle whole-word matching # toggle-literal = "alt-r" # Toggle regex / literal matching # +# [keys.thread-view] # Imported PR comment thread overlay (spec 13) +# scroll-down = ["j", "down"] # Scroll conversation down +# scroll-up = ["k", "up"] # Scroll conversation up +# reply = "r" # Reply to this thread +# close = ["q", "esc"] # Close the thread overlay +# +# [keys.submit-forge] # Submit-review modal (`U` in a PR review, spec 13) +# confirm = "enter" # Submit the review (publishes to the forge) +# cancel = "esc" # Cancel — close this modal, send nothing +# verdict-next = "tab" # Next verdict (comment / approve / request changes) +# verdict-prev = "shift-tab" # Previous verdict +# scroll-down = "down" # Scroll the batch preview down +# scroll-up = "up" # Scroll the batch preview up +# page-down = "pagedown" # Scroll the batch preview down a page +# page-up = "pageup" # Scroll the batch preview up a page +# delete-char = "backspace" # Delete summary character +# compose-summary = "ctrl-e" # Edit the summary in the composer (multi-line) +# +# [keys.submit-result] # Post-submit result view (shown when a submit stops early) +# retry = "U" # Submit again — retry everything that didn't land +# dismiss = ["enter", "esc", "q"] # Dismiss the result view +# scroll-down = ["j", "down"] # Scroll the outcome list down +# scroll-up = ["k", "up"] # Scroll the outcome list up +# page-down = "pagedown" # Scroll the outcome list down a page +# page-up = "pageup" # Scroll the outcome list up a page +# +# [keys.cleanup-reviews] # Finished-review cleanup confirm modal (launcher `X`) +# move-down = ["j", "down"] # Move the highlight down +# move-up = ["k", "up"] # Move the highlight up +# toggle = "space" # Toggle the highlighted entry's selection +# confirm = ["enter", "y"] # Delete the selected finished reviews (worktree, branch, saved state) +# cancel = ["esc", "n"] # Cancel — close this modal, delete nothing +# # Example: remap the staging panel's unstage key to `x` and the switcher's # confirm key to `l` (also still one of ToggleTab's defaults, so this would # collide in a real config — shown separately here for two independent, diff --git a/src/config/keys.rs b/src/config/keys.rs index 768c894..195ba97 100644 --- a/src/config/keys.rs +++ b/src/config/keys.rs @@ -180,6 +180,10 @@ const MODAL_MODE_NAMES: &[&str] = &[ "finder", "project-search-input", "project-search-results", + "thread-view", + "submit-forge", + "submit-result", + "cleanup-reviews", "filter-edit", ]; @@ -195,7 +199,7 @@ const MODAL_MODE_NAMES: &[&str] = &[ /// `diff`/`panel`/`global`, keyed by mode name (one of [`MODAL_MODE_NAMES`]) /// — a single map rather than one field per mode, since /// `crate::ui::modal_keys_config` (the edge module resolving these) already -/// needs one generic merge function reusable across all thirteen modes. +/// needs one generic merge function reusable across every mode. #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct KeysConfig { pub diff: BTreeMap>, diff --git a/src/ui/footer_tests.rs b/src/ui/footer_tests.rs index 142b1c2..6dcd8d9 100644 --- a/src/ui/footer_tests.rs +++ b/src/ui/footer_tests.rs @@ -362,6 +362,52 @@ fn switcher_mode_hints() { ); } +/// End-to-end for the `[keys.]` -> footer path: the strip is built +/// from the *effective* tables, so a remapped modal key reaches the footer +/// with no per-mode wiring. Uses `Mode::ThreadView` as the representative +/// PR-flow modal — a strip built from the compiled-in default table instead +/// would still print `r`. +#[test] +fn a_remapped_modal_key_shows_up_in_that_modes_hint_strip() { + let mut keys = crate::config::KeysConfig::default(); + let mut table = std::collections::BTreeMap::new(); + table.insert( + "reply".to_string(), + vec![crate::config::keys::KeySeqSpec::One( + crate::config::keys::ChordSpec { + code: KeyCode::Char('a'), + mods: KeyModifiers::NONE, + }, + )], + ); + keys.modal.insert("thread-view".to_string(), table); + let (modal_keys, warnings) = crate::ui::modal_keys_config::effective_modal_keys(&keys); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + + let km = Keymap::default_map(); + let entries = build_hints( + Mode::ThreadView, + FooterFlags { + staging_allowed: true, + code_intel_allowed: true, + push_publishes: false, + viewing_commit: false, + help_open: false, + project_search_focus: SearchFocus::Input, + review_session: true, + web_target: None, + }, + None, + &km, + &modal_keys, + ); + let reply = entries + .iter() + .find(|e| e.label == "reply") + .expect("the thread overlay's reply hint"); + assert_eq!(reply.key, "a"); +} + #[test] fn search_mode_has_no_hint_strip() { let km = Keymap::default_map(); diff --git a/src/ui/forge_submit_tests.rs b/src/ui/forge_submit_tests.rs index d6e722d..c62d746 100644 --- a/src/ui/forge_submit_tests.rs +++ b/src/ui/forge_submit_tests.rs @@ -617,6 +617,39 @@ fn typing_a_summary_clears_the_hint_and_lets_request_changes_confirm() { assert!(app.submit_forge.is_none()); } +/// The accepted trade for making `[keys.submit-forge]` remappable: the table +/// is consulted before the char-insert fallback, so a control action bound to +/// a bare printable key takes that character away from summary typing. Pins +/// the ordering — flipping it would silently un-remap every letter-keyed +/// submit-forge override. +#[test] +fn a_submit_forge_action_remapped_onto_a_letter_shadows_summary_typing() { + let mut keys = crate::config::KeysConfig::default(); + let mut table = std::collections::BTreeMap::new(); + table.insert( + "cancel".to_string(), + vec![crate::config::keys::KeySeqSpec::One( + crate::config::keys::ChordSpec { + code: KeyCode::Char('q'), + mods: KeyModifiers::NONE, + }, + )], + ); + keys.modal.insert("submit-forge".to_string(), table); + let (modal_keys, warnings) = crate::ui::modal_keys_config::effective_modal_keys(&keys); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + + let mut app = github_review_app(&["src/a.rs"]); + app.modal_keys = modal_keys; + app.open_submit_forge(); + handle_submit_forge_key( + &mut app, + KeyEvent::new(KeyCode::Char('q'), KeyModifiers::NONE), + ); + assert_eq!(app.mode, Mode::Normal, "`q` must cancel, not type"); + assert!(app.submit_forge.is_none()); +} + // -- confirm on the fake path sends nothing (no live backend) ---------------- // -- scrollable preview + overflow markers ----------------------------------- diff --git a/src/ui/modal_keys.rs b/src/ui/modal_keys.rs index ab8dbdb..b945b56 100644 --- a/src/ui/modal_keys.rs +++ b/src/ui/modal_keys.rs @@ -1173,8 +1173,7 @@ pub(super) static END_REVIEW_KEYS: LazyLock>> /// What a key does in the imported-thread overlay /// ([`super::app::Mode::ThreadView`]): scroll the read-only conversation, -/// draft a reply to it, or close. Not config-remappable yet — see -/// [`THREAD_VIEW_KEYS`] and the module doc. +/// draft a reply to it, or close. Remappable through `[keys.thread-view]`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum ThreadViewAction { /// Scroll the conversation down one line (see @@ -1189,9 +1188,30 @@ pub(super) enum ThreadViewAction { Close, } +pub(super) fn thread_view_action_name(action: ThreadViewAction) -> &'static str { + match action { + ThreadViewAction::ScrollDown => "scroll-down", + ThreadViewAction::ScrollUp => "scroll-up", + ThreadViewAction::Reply => "reply", + ThreadViewAction::Close => "close", + } +} + +pub(super) fn thread_view_action_from_name(name: &str) -> Option { + Some(match name { + "scroll-down" => ThreadViewAction::ScrollDown, + "scroll-up" => ThreadViewAction::ScrollUp, + "reply" => ThreadViewAction::Reply, + "close" => ThreadViewAction::Close, + _ => return None, + }) +} + /// The thread-overlay control keys (`j`/`k`/arrow scroll plus `Esc`/`q` /// close), for the help overlay, footer strip, and -/// [`super::modes::handle_thread_view_key`]'s dispatch. +/// [`super::modes::handle_thread_view_key`]'s dispatch. Defaults only — the +/// effective table is this plus any `[keys.thread-view]` config override (see +/// `super::modal_keys_config`). pub(super) static THREAD_VIEW_KEYS: LazyLock>> = LazyLock::new(|| { vec![ @@ -1250,8 +1270,11 @@ pub(super) static THREAD_VIEW_KEYS: LazyLock> /// summary (a hand-written fallback in /// [`super::modes::handle_submit_forge_key`], never remappable) — so this /// table documents only the control keys, and the scroll keys are deliberately -/// the arrow/page keys rather than `j`/`k`, which belong to the summary. Not -/// config-remappable yet; see the module doc. +/// the arrow/page keys rather than `j`/`k`, which belong to the summary. +/// Remappable through `[keys.submit-forge]`; because the table is consulted +/// before the char-insert fallback, binding a bare printable key here takes +/// that character away from summary typing (documented in +/// `docs/example-config.toml`). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum SubmitForgeAction { /// Publishes the previewed batch (see @@ -1279,8 +1302,41 @@ pub(super) enum SubmitForgeAction { ComposeSummary, } +pub(super) fn submit_forge_action_name(action: SubmitForgeAction) -> &'static str { + match action { + SubmitForgeAction::Confirm => "confirm", + SubmitForgeAction::Cancel => "cancel", + SubmitForgeAction::VerdictNext => "verdict-next", + SubmitForgeAction::VerdictPrev => "verdict-prev", + SubmitForgeAction::ScrollDown => "scroll-down", + SubmitForgeAction::ScrollUp => "scroll-up", + SubmitForgeAction::PageDown => "page-down", + SubmitForgeAction::PageUp => "page-up", + SubmitForgeAction::DeleteChar => "delete-char", + SubmitForgeAction::ComposeSummary => "compose-summary", + } +} + +pub(super) fn submit_forge_action_from_name(name: &str) -> Option { + Some(match name { + "confirm" => SubmitForgeAction::Confirm, + "cancel" => SubmitForgeAction::Cancel, + "verdict-next" => SubmitForgeAction::VerdictNext, + "verdict-prev" => SubmitForgeAction::VerdictPrev, + "scroll-down" => SubmitForgeAction::ScrollDown, + "scroll-up" => SubmitForgeAction::ScrollUp, + "page-down" => SubmitForgeAction::PageDown, + "page-up" => SubmitForgeAction::PageUp, + "delete-char" => SubmitForgeAction::DeleteChar, + "compose-summary" => SubmitForgeAction::ComposeSummary, + _ => return None, + }) +} + /// The submit-review modal's control-key table, for the help overlay, footer -/// strip, and [`super::modes::handle_submit_forge_key`]'s dispatch. +/// strip, and [`super::modes::handle_submit_forge_key`]'s dispatch. Defaults +/// only — the effective table is this plus any `[keys.submit-forge]` config +/// override (see `super::modal_keys_config`). pub(super) static SUBMIT_FORGE_KEYS: LazyLock>> = LazyLock::new(|| { vec![ @@ -1369,8 +1425,8 @@ pub(super) static SUBMIT_FORGE_KEYS: LazyLock &'static str { + match action { + SubmitResultAction::ScrollDown => "scroll-down", + SubmitResultAction::ScrollUp => "scroll-up", + SubmitResultAction::PageDown => "page-down", + SubmitResultAction::PageUp => "page-up", + SubmitResultAction::Dismiss => "dismiss", + SubmitResultAction::Retry => "retry", + } +} + +pub(super) fn submit_result_action_from_name(name: &str) -> Option { + Some(match name { + "scroll-down" => SubmitResultAction::ScrollDown, + "scroll-up" => SubmitResultAction::ScrollUp, + "page-down" => SubmitResultAction::PageDown, + "page-up" => SubmitResultAction::PageUp, + "dismiss" => SubmitResultAction::Dismiss, + "retry" => SubmitResultAction::Retry, + _ => return None, + }) +} + /// The result-modal control keys, for the help overlay, footer strip, and -/// [`super::modes::handle_submit_result_key`]'s dispatch. +/// [`super::modes::handle_submit_result_key`]'s dispatch. Defaults only — the +/// effective table is this plus any `[keys.submit-result]` config override +/// (see `super::modal_keys_config`). pub(super) static SUBMIT_RESULT_KEYS: LazyLock>> = LazyLock::new(|| { vec![ @@ -1557,8 +1638,8 @@ pub(super) static RESTORE_KEYS: LazyLock>> = Laz /// toggles the highlighted entry's selection (all checked by default), and /// confirm/cancel gate the batch. Confirm deletes only the *selected* /// reviews' worktree, branch, and state entry (a no-op with nothing -/// selected); cancel mutates nothing. Not config-remappable yet — see module -/// doc. +/// selected); cancel mutates nothing. Remappable through +/// `[keys.cleanup-reviews]`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum CleanupReviewsAction { /// Moves the highlight down one entry (see @@ -1579,12 +1660,35 @@ pub(super) enum CleanupReviewsAction { Cancel, } +pub(super) fn cleanup_reviews_action_name(action: CleanupReviewsAction) -> &'static str { + match action { + CleanupReviewsAction::MoveDown => "move-down", + CleanupReviewsAction::MoveUp => "move-up", + CleanupReviewsAction::Toggle => "toggle", + CleanupReviewsAction::Confirm => "confirm", + CleanupReviewsAction::Cancel => "cancel", + } +} + +pub(super) fn cleanup_reviews_action_from_name(name: &str) -> Option { + Some(match name { + "move-down" => CleanupReviewsAction::MoveDown, + "move-up" => CleanupReviewsAction::MoveUp, + "toggle" => CleanupReviewsAction::Toggle, + "confirm" => CleanupReviewsAction::Confirm, + "cancel" => CleanupReviewsAction::Cancel, + _ => return None, + }) +} + /// The cleanup confirm modal's key table, for the help overlay, footer strip, /// and [`super::modes::handle_cleanup_reviews_key`]'s dispatch. The /// confirm/cancel rows mirror [`CONFIRM_REMOTE_OP_KEYS`]' binary-gate shape; /// the move pair mirrors [`SWITCHER_KEYS`]' — only `MoveDown` carries the /// footer hint, since its label ("j / Down") already reads as a compound key /// display and merging `MoveUp`'s in would double the " / " separators. +/// Defaults only — the effective table is this plus any +/// `[keys.cleanup-reviews]` config override (see `super::modal_keys_config`). pub(super) static CLEANUP_REVIEWS_KEYS: LazyLock>> = LazyLock::new(|| { vec![ @@ -2992,9 +3096,9 @@ pub(super) static SEARCH_HINTS: LazyLock>> = Lazy // -- Effective (post-config-override) modal tables -------------------------- /// The canonical `[keys.]` table names, in -/// the same order [`ModalKeymaps`]'s fields are declared. One table per modal -/// mode currently defined in this module; adding a fourteenth mode means -/// adding both a field here and a name here, which +/// the same order [`ModalKeymaps`]'s fields are declared. One name per +/// config-remappable modal mode; adding another means adding both a field +/// here and a name here, which /// `crate::config::keys::KeysConfig::from_value`'s parallel hardcoded list /// must also gain (that module can't import this one — see its layering /// note — so `crate::ui::modal_keys_config`'s tests cross-check the two @@ -3016,6 +3120,10 @@ pub(super) const MODAL_MODE_NAMES: &[&str] = &[ "finder", "project-search-input", "project-search-results", + "thread-view", + "submit-forge", + "submit-result", + "cleanup-reviews", "filter-edit", ]; @@ -3052,17 +3160,13 @@ pub struct ModalKeymaps { /// The pull/push confirm modal. Not config-remappable yet — see /// [`CONFIRM_REMOTE_OP_KEYS`]. pub(super) confirm_remote_op: Vec>, - /// The imported-thread overlay. Not config-remappable yet — see - /// [`THREAD_VIEW_KEYS`]. + /// The imported-thread overlay (`[keys.thread-view]`). pub(super) thread_view: Vec>, - /// The submit-review modal. Not config-remappable yet — see - /// [`SUBMIT_FORGE_KEYS`]. + /// The submit-review modal (`[keys.submit-forge]`). pub(super) submit_forge: Vec>, - /// The post-submit result modal. Not config-remappable yet — see - /// [`SUBMIT_RESULT_KEYS`]. + /// The post-submit result modal (`[keys.submit-result]`). pub(super) submit_result: Vec>, - /// The finished-review cleanup confirm modal. Not config-remappable yet — - /// see [`CLEANUP_REVIEWS_KEYS`]. + /// The finished-review cleanup confirm modal (`[keys.cleanup-reviews]`). pub(super) cleanup_reviews: Vec>, /// The restore confirm modal. Not config-remappable yet — see /// [`RESTORE_KEYS`]. @@ -3157,8 +3261,8 @@ mod tests { } } - /// One test over all fourteen mode tables: each mode's action names must - /// be unique and round-trip back to the same action. + /// One test over every config-remappable mode table: each mode's action + /// names must be unique and round-trip back to the same action. #[test] fn every_modal_action_name_mapping_is_total_and_bijective() { assert_action_names_are_total_and_bijective( @@ -3231,6 +3335,26 @@ mod tests { project_search_results_action_name, project_search_results_action_from_name, ); + assert_action_names_are_total_and_bijective( + &THREAD_VIEW_KEYS, + thread_view_action_name, + thread_view_action_from_name, + ); + assert_action_names_are_total_and_bijective( + &SUBMIT_FORGE_KEYS, + submit_forge_action_name, + submit_forge_action_from_name, + ); + assert_action_names_are_total_and_bijective( + &SUBMIT_RESULT_KEYS, + submit_result_action_name, + submit_result_action_from_name, + ); + assert_action_names_are_total_and_bijective( + &CLEANUP_REVIEWS_KEYS, + cleanup_reviews_action_name, + cleanup_reviews_action_from_name, + ); } fn sample_file() -> FileDiff { diff --git a/src/ui/modal_keys_config.rs b/src/ui/modal_keys_config.rs index 8171887..636470b 100644 --- a/src/ui/modal_keys_config.rs +++ b/src/ui/modal_keys_config.rs @@ -21,6 +21,15 @@ //! is resolved user-wins, with one [`ConfigWarning`] recorded. An unknown //! action name, or a two-chord key sequence (modal tables never supported //! `gd`-style sequences), is itself an invalid value. +//! +//! **Free-text modes** (Compose, the commit-message modal, Search, Finder, +//! Project Search, and the submit-review modal's summary field) consult their +//! table before the printable-char fallback, so binding a bare printable key +//! to one of their control actions takes that character away from typing. +//! That's allowed rather than rejected — it's the same trade every free-text +//! mode here has always accepted, and the consequence is documented in +//! `docs/example-config.toml` rather than encoded as a special-case +//! validation rule. use std::collections::BTreeMap; @@ -146,6 +155,38 @@ pub(super) fn effective_modal_keys( modal_keys::project_search_results_action_from_name, &mut warnings, ), + thread_view: apply_modal_overrides( + modal_keys::THREAD_VIEW_KEYS.clone(), + overrides_for("thread-view"), + "keys.thread-view", + modal_keys::thread_view_action_name, + modal_keys::thread_view_action_from_name, + &mut warnings, + ), + submit_forge: apply_modal_overrides( + modal_keys::SUBMIT_FORGE_KEYS.clone(), + overrides_for("submit-forge"), + "keys.submit-forge", + modal_keys::submit_forge_action_name, + modal_keys::submit_forge_action_from_name, + &mut warnings, + ), + submit_result: apply_modal_overrides( + modal_keys::SUBMIT_RESULT_KEYS.clone(), + overrides_for("submit-result"), + "keys.submit-result", + modal_keys::submit_result_action_name, + modal_keys::submit_result_action_from_name, + &mut warnings, + ), + cleanup_reviews: apply_modal_overrides( + modal_keys::CLEANUP_REVIEWS_KEYS.clone(), + overrides_for("cleanup-reviews"), + "keys.cleanup-reviews", + modal_keys::cleanup_reviews_action_name, + modal_keys::cleanup_reviews_action_from_name, + &mut warnings, + ), filter_edit: apply_modal_overrides( modal_keys::FILTER_EDIT_KEYS.clone(), overrides_for("filter-edit"), @@ -160,19 +201,14 @@ pub(super) fn effective_modal_keys( end_review: modal_keys::END_REVIEW_KEYS.clone(), accepted_panel: modal_keys::ACCEPTED_PANEL_KEYS.clone(), confirm_remote_op: modal_keys::CONFIRM_REMOTE_OP_KEYS.clone(), - thread_view: modal_keys::THREAD_VIEW_KEYS.clone(), - submit_forge: modal_keys::SUBMIT_FORGE_KEYS.clone(), - submit_result: modal_keys::SUBMIT_RESULT_KEYS.clone(), - cleanup_reviews: modal_keys::CLEANUP_REVIEWS_KEYS.clone(), restore: modal_keys::RESTORE_KEYS.clone(), }; // Every mode name the config actually provided a table for that isn't - // one of the thirteen known modes was already flagged (unknown key) at - // parse time in `crate::config::keys::KeysConfig::from_value`, which - // hardcodes the same thirteen names — see that module's `MODAL_MODE_NAMES` - // doc and this module's tests for the cross-check that the two lists - // agree. + // one of the known modes was already flagged (unknown key) at parse time + // in `crate::config::keys::KeysConfig::from_value`, which hardcodes the + // same list — see that module's `MODAL_MODE_NAMES` doc and this module's + // tests for the cross-check that the two lists agree. (keymaps, warnings) } diff --git a/src/ui/modal_keys_config_tests.rs b/src/ui/modal_keys_config_tests.rs index 116af2b..5e01ff4 100644 --- a/src/ui/modal_keys_config_tests.rs +++ b/src/ui/modal_keys_config_tests.rs @@ -35,7 +35,7 @@ fn modal_mode_names_match_config_keys_hardcoded_list() { // and the count must match exactly (a name accepted by config that the // ui list doesn't know about would still slip past this check only if // it also appeared in `effective_modal_keys`'s match below, which is - // exhaustive over the thirteen `ModalKeymaps` fields — so a name drifting + // exhaustive over the `ModalKeymaps` fields — so a name drifting // out of sync in either direction fails this test or fails to compile). let toml = modal_keys::MODAL_MODE_NAMES .iter() @@ -88,6 +88,13 @@ fn no_overrides_yields_every_default_table_unchanged() { &modal_keys::PROJECT_SEARCH_RESULTS_HINTS, ); same(&effective.filter_edit, &modal_keys::FILTER_EDIT_KEYS); + same(&effective.thread_view, &modal_keys::THREAD_VIEW_KEYS); + same(&effective.submit_forge, &modal_keys::SUBMIT_FORGE_KEYS); + same(&effective.submit_result, &modal_keys::SUBMIT_RESULT_KEYS); + same( + &effective.cleanup_reviews, + &modal_keys::CLEANUP_REVIEWS_KEYS, + ); } // -- Replace: an action named in config gets exactly the listed keys -------- @@ -337,6 +344,197 @@ fn overriding_a_compose_control_action_leaves_the_rest_of_the_table_intact() { assert_eq!(effective.compose.len(), modal_keys::COMPOSE_HINTS.len()); } +// -- PR-flow modals: each section is wired to its own table ------------------ +// +// One test per newly wired section, because the defect each catches is +// distinct: that *this* mode's `[keys.]` table never reached +// `effective_modal_keys` (the remap silently does nothing). Each asserts the +// three observable consequences of a wired section — the new key drives the +// action, the displaced default no longer does, and the row's `key_label` +// (the exact string `super::help`'s overlay and `super::footer`'s strip +// print) shows the new key. + +#[test] +fn remapping_a_thread_view_action_moves_the_key_and_its_displayed_label() { + let keys = keys_with( + "thread-view", + "reply", + one(KeyCode::Char('a'), KeyModifiers::NONE), + ); + let (effective, warnings) = effective_modal_keys(&keys); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + assert_eq!( + modal_keys::resolve( + &effective.thread_view, + KeyEvent::new(KeyCode::Char('a'), KeyModifiers::NONE) + ), + Some(modal_keys::ThreadViewAction::Reply) + ); + assert_eq!( + modal_keys::resolve( + &effective.thread_view, + KeyEvent::new(KeyCode::Char('r'), KeyModifiers::NONE) + ), + None, + "the displaced default must no longer reply" + ); + let row = effective + .thread_view + .iter() + .find(|b| b.action == modal_keys::ThreadViewAction::Reply) + .expect("reply row"); + assert_eq!(row.key_label(), "a"); +} + +#[test] +fn remapping_a_submit_forge_action_moves_the_key_and_its_displayed_label() { + let keys = keys_with( + "submit-forge", + "compose-summary", + one(KeyCode::Char('y'), KeyModifiers::CONTROL), + ); + let (effective, warnings) = effective_modal_keys(&keys); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + assert_eq!( + modal_keys::resolve( + &effective.submit_forge, + KeyEvent::new(KeyCode::Char('y'), KeyModifiers::CONTROL) + ), + Some(modal_keys::SubmitForgeAction::ComposeSummary) + ); + assert_eq!( + modal_keys::resolve( + &effective.submit_forge, + KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL) + ), + None, + "the displaced default must no longer open the composer" + ); + let row = effective + .submit_forge + .iter() + .find(|b| b.action == modal_keys::SubmitForgeAction::ComposeSummary) + .expect("compose-summary row"); + assert_eq!(row.key_label(), "Ctrl-y"); +} + +#[test] +fn remapping_a_submit_result_action_moves_the_key_and_its_displayed_label() { + let keys = keys_with( + "submit-result", + "retry", + one(KeyCode::Char('R'), KeyModifiers::NONE), + ); + let (effective, warnings) = effective_modal_keys(&keys); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + assert_eq!( + modal_keys::resolve( + &effective.submit_result, + KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE) + ), + Some(modal_keys::SubmitResultAction::Retry) + ); + assert_eq!( + modal_keys::resolve( + &effective.submit_result, + KeyEvent::new(KeyCode::Char('U'), KeyModifiers::NONE) + ), + None, + "the displaced default must no longer retry" + ); + let row = effective + .submit_result + .iter() + .find(|b| b.action == modal_keys::SubmitResultAction::Retry) + .expect("retry row"); + assert_eq!(row.key_label(), "R"); +} + +#[test] +fn remapping_a_cleanup_reviews_action_moves_the_key_and_its_displayed_label() { + let keys = keys_with( + "cleanup-reviews", + "toggle", + one(KeyCode::Char('t'), KeyModifiers::NONE), + ); + let (effective, warnings) = effective_modal_keys(&keys); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + assert_eq!( + modal_keys::resolve( + &effective.cleanup_reviews, + KeyEvent::new(KeyCode::Char('t'), KeyModifiers::NONE) + ), + Some(modal_keys::CleanupReviewsAction::Toggle) + ); + assert_eq!( + modal_keys::resolve( + &effective.cleanup_reviews, + KeyEvent::new(KeyCode::Char(' '), KeyModifiers::NONE) + ), + None, + "the displaced default must no longer toggle" + ); + let row = effective + .cleanup_reviews + .iter() + .find(|b| b.action == modal_keys::CleanupReviewsAction::Toggle) + .expect("toggle row"); + assert_eq!(row.key_label(), "t"); +} + +// -- Invalid values in the PR-flow sections surface the launcher's warnings -- + +/// A section that never reached [`effective_modal_keys`] fails differently: +/// `KeysConfig::from_value` would reject the whole `[keys.]` header as +/// an unknown key, so neither of the per-action warnings below could appear. +/// Table-driven over the four sections since the assertion is on the warning +/// shape, not on any mode's action types. +#[test] +fn each_pr_flow_section_reports_bad_values_like_the_launcher_section_does() { + for (mode, real_action) in [ + ("thread-view", "reply"), + ("submit-forge", "confirm"), + ("submit-result", "retry"), + ("cleanup-reviews", "toggle"), + ] { + // An unparseable key string is rejected at config-parse time. + let raw: toml::Table = format!("[{mode}]\n{real_action} = \"not-a-key\"\n") + .parse() + .expect("valid TOML"); + let mut warnings = Vec::new(); + let cfg = KeysConfig::from_value(toml::Value::Table(raw), &mut warnings); + assert_eq!(warnings.len(), 1, "[keys.{mode}]: {warnings:?}"); + match &warnings[0] { + crate::config::ConfigWarning::InvalidValue { section, key, .. } => { + assert_eq!(section, &format!("keys.{mode}")); + assert_eq!(key, real_action); + } + other => panic!("[keys.{mode}]: expected InvalidValue, got {other:?}"), + } + assert!( + cfg.modal.contains_key(mode), + "[keys.{mode}] must be a recognized section, not an unknown key" + ); + + // An unknown action name is rejected at merge time, naming the same + // section. + let keys = keys_with( + mode, + "not-a-real-action", + one(KeyCode::Esc, KeyModifiers::NONE), + ); + let (_effective, warnings) = effective_modal_keys(&keys); + assert_eq!(warnings.len(), 1, "[keys.{mode}]: {warnings:?}"); + match &warnings[0] { + crate::config::ConfigWarning::InvalidValue { section, key, .. } => { + assert_eq!(section, &format!("keys.{mode}")); + assert_eq!(key, "not-a-real-action"); + } + other => panic!("[keys.{mode}]: expected InvalidValue, got {other:?}"), + } + } +} + // -- docs/example-config.toml completeness ----------------------------------- // // The `[keys.]` sections are entirely commented out (like @@ -417,14 +615,27 @@ fn parse_doc_modal_blocks(text: &str) -> DocModalBlocks { /// Asserts `mode`'s doc block (from `docs/example-config.toml`) names /// exactly the modal action space `from_name` resolves, and that every key -/// string it lists parses under the grammar. +/// string it lists parses under the grammar. The expected action set is read +/// off `table` (the mode's default table) rather than hand-listed at the call +/// site: every variant appears in its table by construction — the same +/// argument `modal_keys`'s bijectivity test relies on — and a hand-kept +/// parallel list can go stale in step with the doc it's meant to police, +/// which is exactly how `[keys.review-launcher]`'s two newest actions went +/// undocumented. fn assert_doc_block_matches( blocks: &DocModalBlocks, mode: &str, - all_actions: &[A], + table: &[ModalBinding], name_of: fn(A) -> &'static str, from_name: fn(&str) -> Option, ) { + let mut all_actions: Vec = Vec::new(); + for b in table { + if !all_actions.contains(&b.action) { + all_actions.push(b.action); + } + } + let all_actions = &all_actions[..]; // The first block named `mode` is the canonical documentation block; // the doc's trailing "Example:" section deliberately reuses // `staging`/`switcher` for a live one-line demo and isn't meant to be @@ -478,248 +689,127 @@ fn example_config_documents_every_modal_action_exactly_once() { assert_doc_block_matches( &blocks, "list", - &[ - ListAction::MoveDown, - ListAction::MoveUp, - ListAction::HalfPageDown, - ListAction::HalfPageUp, - ListAction::FullPageDown, - ListAction::FullPageUp, - ListAction::JumpToTop, - ListAction::JumpToBottom, - ListAction::Jump, - ListAction::Edit, - ListAction::Delete, - ListAction::EnterFilter, - ListAction::Close, - ], + &LIST_KEYS, list_action_name, list_action_from_name, ); assert_doc_block_matches( &blocks, "staging", - &[ - StagingAction::MoveDown, - StagingAction::MoveUp, - StagingAction::HalfPageDown, - StagingAction::HalfPageUp, - StagingAction::FullPageDown, - StagingAction::FullPageUp, - StagingAction::JumpToTop, - StagingAction::JumpToBottom, - StagingAction::Unstage, - StagingAction::EnterFilter, - StagingAction::Close, - ], + &STAGING_KEYS, staging_action_name, staging_action_from_name, ); assert_doc_block_matches( &blocks, "peek", - &[ - PeekAction::MoveDown, - PeekAction::MoveUp, - PeekAction::HalfPageDown, - PeekAction::HalfPageUp, - PeekAction::FullPageDown, - PeekAction::FullPageUp, - PeekAction::JumpToTop, - PeekAction::JumpToBottom, - PeekAction::Enter, - PeekAction::Close, - ], + &PEEK_KEYS, peek_action_name, peek_action_from_name, ); assert_doc_block_matches( &blocks, "switcher", - &[ - SwitcherAction::ToggleTab, - SwitcherAction::MoveDown, - SwitcherAction::MoveUp, - SwitcherAction::HalfPageDown, - SwitcherAction::HalfPageUp, - SwitcherAction::FullPageDown, - SwitcherAction::FullPageUp, - SwitcherAction::JumpToTop, - SwitcherAction::JumpToBottom, - SwitcherAction::Confirm, - SwitcherAction::EnterFilter, - SwitcherAction::Close, - ], + &SWITCHER_KEYS, switcher_action_name, switcher_action_from_name, ); assert_doc_block_matches( &blocks, "review-launcher", - &[ - LauncherAction::ToggleTab, - LauncherAction::MoveDown, - LauncherAction::MoveUp, - LauncherAction::HalfPageDown, - LauncherAction::HalfPageUp, - LauncherAction::FullPageDown, - LauncherAction::FullPageUp, - LauncherAction::JumpToTop, - LauncherAction::JumpToBottom, - LauncherAction::Confirm, - LauncherAction::EnterFilter, - LauncherAction::Close, - ], + &REVIEW_LAUNCHER_KEYS, launcher_action_name, launcher_action_from_name, ); assert_doc_block_matches( &blocks, "help", - &[ - HelpAction::Close, - HelpAction::ScrollDown, - HelpAction::ScrollUp, - HelpAction::PageDown, - HelpAction::PageUp, - HelpAction::Top, - HelpAction::Bottom, - HelpAction::Search, - HelpAction::NextTab, - HelpAction::PrevTab, - ], + &HELP_KEYS, help_action_name, help_action_from_name, ); assert_doc_block_matches( &blocks, "help-search", - &[ - HelpSearchAction::Lock, - HelpSearchAction::Clear, - HelpSearchAction::DeleteChar, - ], + &HELP_SEARCH_HINTS, help_search_action_name, help_search_action_from_name, ); assert_doc_block_matches( &blocks, "filter-edit", - &[ - FilterEditAction::Lock, - FilterEditAction::Clear, - FilterEditAction::DeleteChar, - ], + &FILTER_EDIT_KEYS, filter_edit_action_name, filter_edit_action_from_name, ); assert_doc_block_matches( &blocks, "compose", - &[ - ComposeAction::Cancel, - ComposeAction::Submit, - ComposeAction::CycleClassification, - ComposeAction::Edit(BufferEditAction::Newline), - ComposeAction::Edit(BufferEditAction::MoveLeft), - ComposeAction::Edit(BufferEditAction::MoveRight), - ComposeAction::Edit(BufferEditAction::MoveUp), - ComposeAction::Edit(BufferEditAction::MoveDown), - ComposeAction::Edit(BufferEditAction::WordLeft), - ComposeAction::Edit(BufferEditAction::WordRight), - ComposeAction::Edit(BufferEditAction::LineStart), - ComposeAction::Edit(BufferEditAction::LineEnd), - ComposeAction::Edit(BufferEditAction::DocStart), - ComposeAction::Edit(BufferEditAction::DocEnd), - ComposeAction::Edit(BufferEditAction::DeleteBack), - ComposeAction::Edit(BufferEditAction::DeleteForward), - ComposeAction::Edit(BufferEditAction::DeleteWordBack), - ComposeAction::Edit(BufferEditAction::DeleteWordForward), - ], + &COMPOSE_HINTS, compose_action_name, compose_action_from_name, ); assert_doc_block_matches( &blocks, "commit-message", - &[ - CommitMessageAction::Cancel, - CommitMessageAction::Submit, - CommitMessageAction::Edit(BufferEditAction::Newline), - CommitMessageAction::Edit(BufferEditAction::MoveLeft), - CommitMessageAction::Edit(BufferEditAction::MoveRight), - CommitMessageAction::Edit(BufferEditAction::MoveUp), - CommitMessageAction::Edit(BufferEditAction::MoveDown), - CommitMessageAction::Edit(BufferEditAction::WordLeft), - CommitMessageAction::Edit(BufferEditAction::WordRight), - CommitMessageAction::Edit(BufferEditAction::LineStart), - CommitMessageAction::Edit(BufferEditAction::LineEnd), - CommitMessageAction::Edit(BufferEditAction::DocStart), - CommitMessageAction::Edit(BufferEditAction::DocEnd), - CommitMessageAction::Edit(BufferEditAction::DeleteBack), - CommitMessageAction::Edit(BufferEditAction::DeleteForward), - CommitMessageAction::Edit(BufferEditAction::DeleteWordBack), - CommitMessageAction::Edit(BufferEditAction::DeleteWordForward), - ], + &COMMIT_MESSAGE_HINTS, commit_message_action_name, commit_message_action_from_name, ); assert_doc_block_matches( &blocks, "search", - &[ - SearchAction::Confirm, - SearchAction::Cancel, - SearchAction::DeleteChar, - ], + &SEARCH_HINTS, search_action_name, search_action_from_name, ); assert_doc_block_matches( &blocks, "finder", - &[ - FinderAction::MoveUp, - FinderAction::MoveDown, - FinderAction::Open, - FinderAction::Close, - FinderAction::DeleteChar, - ], + &FINDER_HINTS, finder_action_name, finder_action_from_name, ); assert_doc_block_matches( &blocks, "project-search-input", - &[ - ProjectSearchInputAction::MoveUp, - ProjectSearchInputAction::MoveDown, - ProjectSearchInputAction::Open, - ProjectSearchInputAction::FocusResults, - ProjectSearchInputAction::ToggleFocus, - ProjectSearchInputAction::DeleteChar, - ProjectSearchInputAction::ToggleCase, - ProjectSearchInputAction::ToggleWholeWord, - ProjectSearchInputAction::ToggleLiteral, - ], + &PROJECT_SEARCH_INPUT_HINTS, project_search_input_action_name, project_search_input_action_from_name, ); assert_doc_block_matches( &blocks, "project-search-results", - &[ - ProjectSearchResultsAction::EditQuery, - ProjectSearchResultsAction::Close, - ProjectSearchResultsAction::MoveUp, - ProjectSearchResultsAction::MoveDown, - ProjectSearchResultsAction::Open, - ProjectSearchResultsAction::ToggleFocus, - ProjectSearchResultsAction::ToggleCase, - ProjectSearchResultsAction::ToggleWholeWord, - ProjectSearchResultsAction::ToggleLiteral, - ], + &PROJECT_SEARCH_RESULTS_HINTS, project_search_results_action_name, project_search_results_action_from_name, ); + assert_doc_block_matches( + &blocks, + "thread-view", + &THREAD_VIEW_KEYS, + thread_view_action_name, + thread_view_action_from_name, + ); + assert_doc_block_matches( + &blocks, + "submit-forge", + &SUBMIT_FORGE_KEYS, + submit_forge_action_name, + submit_forge_action_from_name, + ); + assert_doc_block_matches( + &blocks, + "submit-result", + &SUBMIT_RESULT_KEYS, + submit_result_action_name, + submit_result_action_from_name, + ); + assert_doc_block_matches( + &blocks, + "cleanup-reviews", + &CLEANUP_REVIEWS_KEYS, + cleanup_reviews_action_name, + cleanup_reviews_action_from_name, + ); }