From 21a6a5100e01febaae9dcd3e18f5f7dfcaf7caab Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Wed, 2 Sep 2026 13:51:36 -0400 Subject: [PATCH] feat(review): render walkthrough chapters in the summary panel --- docs/rfc/workon-review.md | 2 +- git-workon-review/src/app.rs | 46 ++++++-- git-workon-review/src/main.rs | 28 +++++ git-workon-review/src/render.rs | 195 ++++++++++++++++++++++++++++++- git-workon-review/src/summary.rs | 25 ++++ 5 files changed, 284 insertions(+), 12 deletions(-) diff --git a/docs/rfc/workon-review.md b/docs/rfc/workon-review.md index fc101b28..777acad4 100644 --- a/docs/rfc/workon-review.md +++ b/docs/rfc/workon-review.md @@ -160,7 +160,7 @@ The remaining roadmap is resequenced around the tool being **the author's own ev - **Conflict resolution** *(stretch)*. Resolve merge/rebase conflicts in the SBS view. Large surface; may not make v1. -- **Agent loop** *(the eventual north star)*. **Design locked 2026-09-02 (ADR-039); the three open forks below are resolved, not just re-flagged.** One substrate serves both review comments and an integrated `/explain-diff`-style walkthrough: `AnnotationKind::{Comment, TourStop, Chapter}` in one sqlite table at `/workon-review/annotations.db`, anchored by content-hash context (target line + 3 lines each way, re-resolved per load: exact match, then a scored windowed scan, then whitespace-tolerant, else `Orphaned` — never silently wrong) rather than the originally-proposed `(changeset_id, path, side, lnum)` key. **Comment-store home:** its own crate, `git-workon-annotations` (`publish = false`, serde-free, no git2) — `git workon mcp` is the second consumer the "no separate core crate" rule was waiting on, so that condition is now met and the rule no longer applies to it. **MCP crate/transport:** `rmcp` 3.2 (minimal features, current-thread tokio), not hand-rolled JSON-RPC, served from its own `git-workon-mcp` crate (ADR-040) and reached from `git-workon` via the existing PATH-dispatch mechanism (not a built-in `Cmd::Mcp`, and not a `git-workon-review mcp` subcommand) — this is what lets the published `git-workon` binary depend on the feature without depending on the unpublished annotations crate. Landing as five stacked slices (crate scaffold → TUI read → TUI authoring → prose/walkthrough polish → MCP crate); each lands alone. Still deferred behind the daily-driver work, but no longer blocked on open design questions. +- **Agent loop** *(the eventual north star)*. **Design locked 2026-09-02 (ADR-039); the three open forks below are resolved, not just re-flagged.** One substrate serves both review comments and an integrated `/explain-diff`-style walkthrough: `AnnotationKind::{Comment, TourStop, Chapter}` in one sqlite table at `/workon-review/annotations.db`, anchored by content-hash context (target line + 3 lines each way, re-resolved per load: exact match, then a scored windowed scan, then whitespace-tolerant, else `Orphaned` — never silently wrong) rather than the originally-proposed `(changeset_id, path, side, lnum)` key. **Comment-store home:** its own crate, `git-workon-annotations` (`publish = false`, serde-free, no git2) — `git workon mcp` is the second consumer the "no separate core crate" rule was waiting on, so that condition is now met and the rule no longer applies to it. **MCP crate/transport:** `rmcp` 3.2 (minimal features, current-thread tokio), not hand-rolled JSON-RPC, served from its own `git-workon-mcp` crate (ADR-040) and reached from `git-workon` via the existing PATH-dispatch mechanism (not a built-in `Cmd::Mcp`, and not a `git-workon-review mcp` subcommand) — this is what lets the published `git-workon` binary depend on the feature without depending on the unpublished annotations crate. Landing as five stacked slices (crate scaffold → TUI read → TUI authoring → prose/walkthrough polish → MCP crate); each lands alone. — DONE (2026-09-02): four of five shipped — `annot-crate` (the store + resolver crate), `annot-read` (gutter markers, view/reply overlay, tour stepping), `annot-write` (the multi-line annotation editor, create/reply/resolve), `annot-prose` (walkthrough chapters wrapped into the summary panel, a `stop i/n` tour-progress indicator in the diff header, and `--tour ` to open straight into a walkthrough). `mcp-crate` lands next. ## Orchestration notes diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 490c9fa6..115503bd 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -970,6 +970,14 @@ impl ChangesetIdentity { } } +/// This session's [`ChangesetKey`] for an ARBITRARY changeset — the shared core +/// [`App::current_changeset_key`] (the active one) and [`App::summary_for`] (an outline +/// selection, which need not be the active changeset) both key their ADR-039 lookups off. See +/// [`ChangesetIdentity`]'s doc comment for why name alone is ambiguous. +fn changeset_key_for(cs: &Changeset) -> ChangesetKey { + ChangesetKey::new(cs.name.clone(), cs.span == ChangesetSpan::Uncommitted) +} + /// The outline side pane's own state (locked fork 3): whether it's showing, whether IT (rather /// than the diff) currently has keyboard focus, its own cursor (an index into /// [`App::outline_items`]'s row list — a wholly separate coordinate space from [`App::cursor`]), @@ -1921,8 +1929,7 @@ impl App { /// `ChangesetIdentity` itself, since that type's fields are private to this module's own /// nav-identity use and carry no annotations-crate conversion. fn current_changeset_key(&self) -> ChangesetKey { - let cs = &self.cur().cs; - ChangesetKey::new(cs.name.clone(), cs.span == ChangesetSpan::Uncommitted) + changeset_key_for(&self.cur().cs) } /// The active file `idx`'s `role` view's annotation markers, one [`MarkerKind`] per @@ -2338,11 +2345,13 @@ impl App { } } - /// Set the active walkthrough by name and reload its stops (`main.rs`'s future `--tour` - /// flag, and tests). Nothing infers a tour automatically today — the store has no "list - /// tours" query (a tour's identity is just whatever name its stops share), so a tour must - /// be named explicitly by the caller, matching how [`Self::set_review_source`] is a setter - /// rather than a constructor parameter. + /// Set the active walkthrough by name and reload its stops (`main.rs`'s `--tour` flag, and + /// tests). Nothing infers a tour automatically today — the store has no "list tours" query + /// (a tour's identity is just whatever name its stops share), so a tour must be named + /// explicitly by the caller, matching how [`Self::set_review_source`] is a setter rather + /// than a constructor parameter. Fail-soft on an unknown name: [`Self::reload_tour_stops`] + /// yields an empty stop list, so [`Self::tour_next`]/[`Self::tour_prev`] fall straight into + /// their existing "no active tour" notice rather than erroring. pub fn set_tour(&mut self, tour: impl Into) { self.tour_name = Some(tour.into()); self.tour_idx = None; @@ -2356,6 +2365,20 @@ impl App { }; } + /// The active tour's step position for the diff header's progress indicator + /// (`diff_header_line`'s "stop i/n") — `Some((1-based index, total))` once + /// [`Self::tour_next`]/[`Self::tour_prev`] has parked on a stop, `None` before the first + /// step or when no tour is active/has stops. Not derived from [`Self::tour_name`] alone + /// (a tour can be SET but not yet STEPPED into), matching the "stop" wording — showing + /// "stop -/n" for a not-yet-entered tour would claim a position that doesn't exist yet. + pub fn tour_progress(&self) -> Option<(usize, usize)> { + let idx = self.tour_idx?; + if self.tour_stops.is_empty() { + return None; + } + Some((idx + 1, self.tour_stops.len())) + } + /// `]t`: step to the next stop of the active tour (see [`Self::set_tour`]). Clamps at the /// last stop — does not wrap. A no-op with a footer notice when no tour is active or it has /// no stops. @@ -3676,6 +3699,14 @@ impl App { let view = &self.changesets[cs_idx]; let label = display_label(&view.cs); let failure_message = view.failure_message().map(|s| s.to_string()); + // ADR-039's per-changeset walkthrough prose: `None` on a store-open failure or + // when no `Chapter` annotation has been authored for this changeset — degrades + // silently, matching every other `annotations`-optional read in this module. + let chapter = self + .annotations + .as_ref() + .and_then(|store| store.chapter(&changeset_key_for(&view.cs)).ok().flatten()) + .map(|annotation| annotation.body); Summary::Changeset(summary::changeset_summary( label, view.cs.current, @@ -3683,6 +3714,7 @@ impl App { view.is_pending(), view.is_failed(), failure_message, + chapter, view.files(), )) } diff --git a/git-workon-review/src/main.rs b/git-workon-review/src/main.rs index d13e89b8..7659b14e 100644 --- a/git-workon-review/src/main.rs +++ b/git-workon-review/src/main.rs @@ -33,6 +33,13 @@ struct Cli { /// a PR reference #[arg(value_name = "SOURCE", add = ArgValueCompleter::new(complete_source))] source: Option, + + /// Open into an ADR-039 walkthrough by name — `]t` then steps to its first stop. Fail-soft + /// on an unknown name: `App::set_tour` degrades to an empty tour, which reads exactly like + /// no `--tour` flag at all plus the usual "no active tour" notice on `]t` — there's no + /// "list tours" query yet for this flag to validate against up front. + #[arg(long, value_name = "NAME")] + tour: Option, } fn main() -> Result<()> { @@ -222,6 +229,7 @@ fn main() -> Result<()> { repo, views, source, + cli.tour.as_deref(), &view_config, &keymap, &theme_override_warnings, @@ -248,6 +256,7 @@ fn main() -> Result<()> { repo, views, source, + cli.tour.as_deref(), &view_config, &keymap, &theme_override_warnings, @@ -272,6 +281,7 @@ fn seat_app( repo: Repository, views: Vec, source: Option, + tour: Option<&str>, view_config: &config::RawViewConfig, keymap: &Keymap, theme_override_warnings: &[String], @@ -280,6 +290,12 @@ fn seat_app( if let Some(source) = source { app.set_review_source(source); } + // `--tour`: fail-soft per `Cli::tour`'s doc comment — an unknown name just yields an empty + // stop list, so the reviewer's first `]t` gets the existing "no active tour" notice rather + // than a startup error. + if let Some(tour) = tour { + app.set_tour(tour.to_string()); + } // Idle-deferred file loads: defer file loads to the event loop's input-idle window rather than // blocking here (or on any later selection change) — `app.open_current()` below marks the // initial open pending @@ -325,6 +341,18 @@ fn surface_warnings(app: &mut App, keymap: &Keymap, extra_warnings: Vec) mod tests { use super::*; + #[test] + fn tour_flag_parses_into_cli() { + let cli = Cli::try_parse_from(["git-workon-review", "--tour", "explain-stack"]).unwrap(); + assert_eq!(cli.tour.as_deref(), Some("explain-stack")); + } + + #[test] + fn tour_flag_defaults_to_none() { + let cli = Cli::try_parse_from(["git-workon-review"]).unwrap(); + assert_eq!(cli.tour, None); + } + #[test] fn no_color_truth_table() { assert!(!no_color(None), "unset must not trigger mono"); diff --git a/git-workon-review/src/render.rs b/git-workon-review/src/render.rs index 8d2880c8..9efa7632 100644 --- a/git-workon-review/src/render.rs +++ b/git-workon-review/src/render.rs @@ -28,6 +28,7 @@ use crate::search::SearchSide; use crate::summary::{ChangesetSummary, DirSummary, SummaryFileRow}; use crate::theme::Palette; use crate::wordiff::Span as WordSpan; +use crate::wrap::wrap_text; use workon_annotations::AnnotationKind; // The on-tint colors (diff add/del gradient + staged variants, cursor/selection washes, and syntax @@ -1868,6 +1869,18 @@ fn diff_header_line( )); } spans.extend(file_segment_spans(app, theme, icons, focused, role_badge)); + if let Some((i, n)) = app.tour_progress() { + // ADR-039 walkthrough polish: the active tour's step position, appended to the diff + // header rather than the footer (the footer is a single overloaded row — confirm + // prompt, search prompt, or notice — that a persistent indicator would keep losing + // fights with). Only reachable once `]t`/`[t` has actually parked on a stop, so this + // never appears on the pending/failed/empty-files early return above (no file view to + // land a stop on there anyway). + spans.push(TSpan::styled( + format!(" stop {i}/{n}"), + Style::default().fg(theme.dim), + )); + } Line::from(spans) } @@ -2109,6 +2122,7 @@ fn push_summary_body( /// ([`render_body`]), and the body no longer duplicates it as its own first line. fn changeset_summary_lines( summary: &ChangesetSummary, + width: usize, height: usize, theme: &Palette, icons: IconMode, @@ -2143,6 +2157,19 @@ fn changeset_summary_lines( return (title, lines); } + // ADR-039's walkthrough chapter: wrapped prose plus a blank separator, prepended ahead of + // the file list — counted into `lines` BEFORE `push_summary_body` derives its own height + // budget below, so a long chapter elides file rows first rather than overflowing the pane. + if let Some(chapter) = &summary.chapter { + for wrapped in wrap_text(chapter, width) { + lines.push(Line::from(TSpan::styled( + wrapped, + Style::default().fg(theme.foreground), + ))); + } + lines.push(Line::from("")); + } + push_summary_body( &mut lines, &summary.files, @@ -2204,9 +2231,10 @@ fn render_summary( theme: &Palette, icons: IconMode, ) -> Line<'static> { + let width = area.width as usize; let height = area.height as usize; let (title, lines) = match summary { - Summary::Changeset(cs) => changeset_summary_lines(cs, height, theme, icons), + Summary::Changeset(cs) => changeset_summary_lines(cs, width, height, theme, icons), Summary::Dir(dir) => dir_summary_lines(dir, height, theme, icons), }; frame.render_widget(Paragraph::new(lines), area); @@ -2983,18 +3011,19 @@ mod tests { use unicode_width::UnicodeWidthChar; use super::{ - changeset_prefix_spans, compose_segments, content_spans, hscroll_cut, pan_spans, - pane_header_label_style, render, LineEmphasis, STATUS_PLACEHOLDER, + changeset_prefix_spans, changeset_summary_lines, compose_segments, content_spans, + hscroll_cut, pan_spans, pane_header_label_style, render, LineEmphasis, STATUS_PLACEHOLDER, }; use crate::align::{DisplayRow, Row}; use crate::app::test_support::app_from_fixture; use crate::app::{App, DiffTextMode, EffectiveZoom, Role}; use crate::highlight::FgSpan; + use crate::icons::IconMode; use crate::keymap::Keymap; use crate::outline::OutlineItem; use crate::theme::Palette; use crate::wordiff::Span as WordSpan; - use workon_annotations::store::AnnotationStore; + use workon_annotations::store::{AnnotationStore, TourStop, Walkthrough}; use workon_annotations::{Anchor, AnnotationKind, ChangesetKey, NewAnnotation}; /// Render one frame against the default (unrebound) keymap and the dark theme — the vast @@ -7588,4 +7617,162 @@ mod tests { "the overlay shows the comment's author" ); } + + // ── ADR-039 slice 5: walkthrough chapters in the summary panel, tour progress ────── + + /// Move `app`'s outline cursor onto `cs_idx`'s Header row and focus the outline — the + /// shared setup `summary_panel_title_has_no_counter_and_keeps_the_plain_foreground_look` + /// (above) already established for landing the diff pane on the summary panel. + fn focus_summary_for(app: &mut App, cs_idx: usize) { + let header_idx = app + .outline_items() + .iter() + .position(|it| matches!(it, OutlineItem::Header { cs_idx: c, .. } if *c == cs_idx)) + .expect("the changeset's header row is present") as i64; + let delta = header_idx - app.outline_cursor() as i64; + app.outline_move_by(delta); + app.focus_outline(); + } + + #[test] + fn chapter_renders_in_the_summary_panel() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = two_committed_changesets_app(&fixture); + let store = AnnotationStore::open(fixture.repo().unwrap().commondir()).unwrap(); + store + .insert(NewAnnotation { + kind: AnnotationKind::Chapter, + changeset: ChangesetKey::new("cs-b", false), + anchor: None, + body: "This stop walks through the restack fix.".into(), + author: "guide".into(), + tour: None, + seq: None, + }) + .unwrap(); + + focus_summary_for(&mut app, 1); + + let buf = render_once(&mut app, 80, 20); + let content = buf_lines(&buf).join("\n"); + assert!( + content.contains("This stop walks through the restack fix."), + "the chapter prose renders in the summary panel body: {content:?}" + ); + } + + #[test] + fn absent_chapter_renders_the_summary_body_as_before() { + // No `Chapter` annotation authored — [`ChangesetSummary::chapter`] is `None`, and the + // pure builder must fall straight back to `push_summary_body`'s own leading spacer + // blank rather than reserving a row for prose that isn't there. + let summary = crate::summary::changeset_summary( + "My Title".to_string(), + false, + false, + false, + false, + None, + None, + &[], + ); + let (_, lines) = + changeset_summary_lines(&summary, 40, 10, &Palette::dark(), IconMode::None); + let first: String = lines[0].spans.iter().map(|s| s.content.as_ref()).collect(); + assert_eq!( + first.trim(), + "", + "no chapter: first body row is still the pre-existing blank spacer, got {first:?}" + ); + } + + #[test] + fn wrapped_chapter_respects_the_height_budget_and_elides_file_rows() { + let mut builder = FixtureBuilder::new().config("core.autocrlf", "false"); + for i in 0..8 { + builder = + builder.unstaged_file(&format!("file{i}.txt"), "a\n", &format!("a\nchanged{i}\n")); + } + let fixture = builder.build().unwrap(); + let store = AnnotationStore::open(fixture.repo().unwrap().commondir()).unwrap(); + store + .insert(NewAnnotation { + kind: AnnotationKind::Chapter, + changeset: ChangesetKey::new("main", true), + anchor: None, + body: "This walkthrough chapter is long enough to wrap across several \ + display lines once it lands in the summary panel's narrow width." + .into(), + author: "guide".into(), + tour: None, + seq: None, + }) + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.focus_outline(); + let header_idx = app + .outline_items() + .iter() + .position(|it| matches!(it, OutlineItem::Header { .. })) + .expect("the single changeset's header row is present") as i64; + let delta = header_idx - app.outline_cursor() as i64; + app.outline_move_by(delta); + + // Wide enough that the summary pane (frame minus the 35-col outline) wraps the + // chapter at a readable width; 12 rows keeps the height budget tight. + let buf = render_once(&mut app, 100, 12); + let content = buf_lines(&buf).join("\n"); + assert!( + content.contains("This walkthrough chapter"), + "the wrapped chapter still renders within the budget: {content:?}" + ); + assert!( + content.contains("more"), + "8 files plus a wrapped chapter must overflow a 12-row pane and elide, got: \ + {content:?}" + ); + } + + #[test] + fn tour_progress_renders_in_the_diff_header() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "tracked.txt", + "line1\nline2\nline3\n", + "line1\nCHANGED\nline3\n", + ) + .build() + .unwrap(); + let store = AnnotationStore::open(fixture.repo().unwrap().commondir()).unwrap(); + store + .put_walkthrough(Walkthrough { + changeset: ChangesetKey::new("main", true), + tour: "demo".into(), + chapter: None, + chapter_author: None, + stops: vec![TourStop { + anchor: single_line_anchor("tracked.txt", true, 2, "CHANGED"), + body: "look here".into(), + author: "guide".into(), + seq: 0, + }], + }) + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.set_tour("demo"); + app.tour_next(); + + let buf = render_once(&mut app, 60, 20); + let content = buf_lines(&buf).join("\n"); + assert!( + content.contains("stop 1/1"), + "the diff header shows the tour's step position: {content:?}" + ); + } } diff --git a/git-workon-review/src/summary.rs b/git-workon-review/src/summary.rs index b7a8d5c6..1c2d9193 100644 --- a/git-workon-review/src/summary.rs +++ b/git-workon-review/src/summary.rs @@ -81,6 +81,10 @@ pub struct ChangesetSummary { pub failed: bool, /// The acquisition failure message (ADR-037), `Some` only when `failed`. pub failure_message: Option, + /// ADR-039's per-changeset walkthrough prose (`AnnotationKind::Chapter`), `None` when no + /// tour has authored one for this changeset — never populated for a [`DirSummary`], which + /// has no single changeset to anchor a chapter to. + pub chapter: Option, pub files: Vec, pub total_adds: usize, pub total_dels: usize, @@ -95,6 +99,7 @@ pub fn changeset_summary( loading: bool, failed: bool, failure_message: Option, + chapter: Option, files: &[FileChange], ) -> ChangesetSummary { let (files, total_adds, total_dels) = file_rows(files); @@ -105,6 +110,7 @@ pub fn changeset_summary( loading, failed, failure_message, + chapter, files, total_adds, total_dels, @@ -228,16 +234,33 @@ mod tests { false, false, None, + None, &files, ); assert_eq!(summary.label, "My Title"); assert!(summary.current); assert!(!summary.needs_restack); assert_eq!(summary.files.len(), 2); + assert_eq!(summary.chapter, None, "no chapter arg was passed"); assert_eq!(summary.total_adds, 2); assert_eq!(summary.total_dels, 4); } + #[test] + fn changeset_summary_carries_the_chapter_through_unchanged() { + let summary = changeset_summary( + "My Title".to_string(), + true, + false, + false, + false, + None, + Some("A walkthrough chapter.".to_string()), + &[], + ); + assert_eq!(summary.chapter.as_deref(), Some("A walkthrough chapter.")); + } + #[test] fn changeset_summary_loading_carries_no_files() { let summary = changeset_summary( @@ -247,6 +270,7 @@ mod tests { true, false, None, + None, &[], ); assert!(summary.loading); @@ -263,6 +287,7 @@ mod tests { false, true, Some("boom".to_string()), + None, &[], ); assert!(summary.failed);