From 944c5d2c9790ae8a30a57ea6baf2ceb13baf268f Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Mon, 6 Jul 2026 18:13:47 -0400 Subject: [PATCH 1/7] feat(review): add combined diff and rename detection to acquire --- git-workon-review/src/acquire.rs | 55 +++++++++++++---- git-workon-review/tests/diff_model.rs | 86 +++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 12 deletions(-) diff --git a/git-workon-review/src/acquire.rs b/git-workon-review/src/acquire.rs index 30512e97..08f98369 100644 --- a/git-workon-review/src/acquire.rs +++ b/git-workon-review/src/acquire.rs @@ -5,44 +5,75 @@ //! (a committed rev pair, or "uncommitted"); this module only knows *how* to turn that into //! git2 diffs and then a [`DiffModel`]. -use git2::{DiffOptions, Oid, Repository}; +use git2::{DiffFindOptions, DiffOptions, Oid, Repository}; use workon::{Changeset, ChangesetSource}; use crate::error::DiffError; use crate::model::DiffModel; -/// The two working-tree diffs a review session needs: the index against `HEAD` (staged), and -/// the working tree against the index (unstaged, including untracked content). +/// The working-tree diffs a review session needs: the index against `HEAD` (staged), the +/// working tree against the index (unstaged, including untracked content), and the fused +/// `HEAD` ↔ worktree view (combined) the M3 renderer reviews by default. #[derive(Debug, Clone, PartialEq, Eq)] pub struct WorktreeDiffs { pub staged: DiffModel, pub unstaged: DiffModel, + /// `HEAD`'s tree diffed straight against the working tree (index consulted only for + /// untracked/ignore filtering), fusing staged and unstaged hunks on the same file into one + /// diff — the combined-zoom view the M3 renderer reviews (locked design decision #2). + pub combined: DiffModel, } -/// Diff `HEAD`'s tree against the index (staged) and the index against the working tree -/// (unstaged), for a [`ChangesetSource::Uncommitted`] changeset. +/// Diff `HEAD`'s tree against the index (staged), the index against the working tree +/// (unstaged), and `HEAD`'s tree against the working tree directly (combined), for a +/// [`ChangesetSource::Uncommitted`] changeset. /// -/// The unstaged side sets `include_untracked`/`recurse_untracked_dirs`/ +/// The unstaged and combined sides both set `include_untracked`/`recurse_untracked_dirs`/ /// `show_untracked_content` so untracked files carry real content in the model (git2 gives -/// `Delta::Untracked` natively here — no `/dev/null` header synthesis needed). +/// `Delta::Untracked` natively here — no `/dev/null` header synthesis needed). `find_similar` +/// runs on all three diffs before materialization so worktree renames (e.g. an untracked file +/// that replaces a tracked one under a new name) surface as [`crate::model::FileStatus::Renamed`] +/// rather than a delete+add pair — the read side already handles that status (corpus-proven). +/// +/// The two untracked-including diffs (unstaged, combined) pass explicit +/// [`DiffFindOptions::for_untracked`] — plain `find_similar(None)`'s default flags (just +/// `GIT_DIFF_FIND_RENAMES`) do NOT pair an untracked file with a workdir deletion; libgit2 +/// requires `for_untracked` opted in separately for that side of the match. The staged diff +/// never sees untracked deltas, so `None` (matching [`diff_committed`]'s convention) is enough +/// there. pub fn diff_uncommitted(repo: &Repository) -> Result { let head_tree = repo.head()?.peel_to_tree()?; let mut staged_opts = DiffOptions::new(); staged_opts.context_lines(3); - let staged_diff = repo.diff_tree_to_index(Some(&head_tree), None, Some(&mut staged_opts))?; + let mut staged_diff = + repo.diff_tree_to_index(Some(&head_tree), None, Some(&mut staged_opts))?; + staged_diff.find_similar(None)?; let staged = DiffModel::from_git2(&staged_diff)?; - let mut unstaged_opts = DiffOptions::new(); - unstaged_opts + let mut worktree_opts = DiffOptions::new(); + worktree_opts .include_untracked(true) .recurse_untracked_dirs(true) .show_untracked_content(true) .context_lines(3); - let unstaged_diff = repo.diff_index_to_workdir(None, Some(&mut unstaged_opts))?; + let mut untracked_find = DiffFindOptions::new(); + untracked_find.renames(true).for_untracked(true); + + let mut unstaged_diff = repo.diff_index_to_workdir(None, Some(&mut worktree_opts))?; + unstaged_diff.find_similar(Some(&mut untracked_find))?; let unstaged = DiffModel::from_git2(&unstaged_diff)?; - Ok(WorktreeDiffs { staged, unstaged }) + let mut combined_diff = + repo.diff_tree_to_workdir_with_index(Some(&head_tree), Some(&mut worktree_opts))?; + combined_diff.find_similar(Some(&mut untracked_find))?; + let combined = DiffModel::from_git2(&combined_diff)?; + + Ok(WorktreeDiffs { + staged, + unstaged, + combined, + }) } /// Diff `base`'s tree against `head`'s tree, for a [`ChangesetSource::Committed`] changeset — diff --git a/git-workon-review/tests/diff_model.rs b/git-workon-review/tests/diff_model.rs index fa77fa24..ac081672 100644 --- a/git-workon-review/tests/diff_model.rs +++ b/git-workon-review/tests/diff_model.rs @@ -343,6 +343,92 @@ fn hunk_to_diff_bytes_matches_diff_print() -> Result<(), Box Result<(), Box> { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .partially_staged_file( + "f.txt", + "line1\nline2\nline3\n", + "line1\nSTAGED\nline3\n", + "line1\nSTAGED\nWORKDIR\n", + ) + .build()?; + let repo = fixture.repo()?; + + let diffs = diff_uncommitted(repo)?; + // Split views each see only their own half of the change. + assert_eq!(diffs.staged.files.len(), 1); + assert_eq!(diffs.unstaged.files.len(), 1); + + // Combined fuses both onto one file, diffing straight from HEAD to the workdir. + assert_eq!(diffs.combined.files.len(), 1); + let file = &diffs.combined.files[0]; + assert_eq!(file.path, "f.txt"); + assert_eq!(file.status, FileStatus::Modified); + assert_eq!(file.hunks.len(), 1); + let added: Vec<&[u8]> = file.hunks[0] + .lines + .iter() + .filter(|l| l.kind == LineKind::Addition) + .map(|l| l.content.as_slice()) + .collect(); + // Both the staged AND the unstaged edit show up as additions in the one fused hunk. + assert!(added.contains(&b"STAGED\n".as_slice())); + assert!(added.contains(&b"WORKDIR\n".as_slice())); + + Ok(()) +} + +#[test] +fn untracked_file_appears_as_added_in_combined() -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .untracked_file("new.txt", "hello\nworld\n") + .build()?; + let repo = fixture.repo()?; + + let diffs = diff_uncommitted(repo)?; + assert_eq!(diffs.combined.files.len(), 1); + let file = &diffs.combined.files[0]; + assert_eq!(file.path, "new.txt"); + // Matches the unstaged side's convention (see `untracked_file_has_full_content_as_addition`): + // git2 reports untracked deltas as `Delta::Untracked`, not `Delta::Added` — all lines are + // still additions since there is no pre-image. + assert_eq!(file.status, FileStatus::Untracked); + assert!(file.hunks[0] + .lines + .iter() + .all(|l| l.kind == LineKind::Addition)); + + Ok(()) +} + +#[test] +fn renamed_in_worktree_file_surfaces_as_renamed_in_combined( +) -> Result<(), Box> { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + // Deleted from the working tree, still in HEAD/index... + .deleted_file("old.txt", "line1\nline2\nline3\nline4\nline5\n") + // ...and a same-content untracked file lands under a new name — a worktree rename + // `find_similar` must pair up. + .untracked_file("new.txt", "line1\nline2\nline3\nline4\nline5\n") + .build()?; + let repo = fixture.repo()?; + + let diffs = diff_uncommitted(repo)?; + assert_eq!(diffs.combined.files.len(), 1); + let file = &diffs.combined.files[0]; + assert_eq!(file.status, FileStatus::Renamed); + assert_eq!(file.path, "new.txt"); + assert_eq!(file.old_path.as_deref(), Some("old.txt")); + + Ok(()) +} + // ── diff_changeset over a real assemble_changesets result ───────────────────── #[test] From a7f555ccacfd78a8f317101c86dd776f1e993ff3 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Mon, 6 Jul 2026 18:27:32 -0400 Subject: [PATCH 2/7] feat(review): port SBS row alignment with collapsed context gaps --- git-workon-review/src/align.rs | 532 +++++++++++++++++++++++++++++++++ git-workon-review/src/lib.rs | 1 + 2 files changed, 533 insertions(+) create mode 100644 git-workon-review/src/align.rs diff --git a/git-workon-review/src/align.rs b/git-workon-review/src/align.rs new file mode 100644 index 00000000..f6421812 --- /dev/null +++ b/git-workon-review/src/align.rs @@ -0,0 +1,532 @@ +//! Side-by-side row alignment, ported from the `review-tui-spike` prototype's `align.rs`. +//! +//! Walks a file's hunks against its full old/new text and produces one row vector pairing +//! old-side and new-side positions so the UI can render a row-aligned side-by-side view. +//! Outside hunks, lines pair 1:1. Inside a hunk, git emits deletions before additions within +//! each change block; we pair del[i] with add[i] and give the shorter side filler rows for the +//! excess. +//! +//! This module reads only hunk counters (`old_start`/`old_count`/`new_start`/`new_count`), +//! [`crate::model::Hunk::lines`], and each line's kind + `old_lnum`/`new_lnum`. Content is NOT +//! read from hunk lines here — rendering reads full file text by line number so numbers and +//! content stay in sync (M4 concern; out of scope for this module). +//! +//! ## Lineno invariant +//! +//! [`crate::model::HunkLine::old_lnum`]/`new_lnum` are `None` for the wrong side of an +//! addition/deletion (see the doc comment on [`crate::synthesis::LineSelection`], which relies +//! on the same guarantee). Concretely: a [`LineKind::Context`] line always has both linenos +//! populated; a [`LineKind::Deletion`] line always has `old_lnum` populated; a +//! [`LineKind::Addition`] line always has `new_lnum` populated. This is git2's own guarantee +//! (`Patch::line_in_hunk`'s `old_lineno`/`new_lineno`), not something this module can violate, +//! so the pairing code below `expect()`s the lineno for the side each kind is documented to +//! carry. + +use crate::model::{Hunk, HunkLine, LineKind}; + +/// A row position on one side of the aligned view. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Row { + /// 1-based line number into the full file text for this side. + Line(usize), + Filler, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CellKind { + Context, + Del, + Add, + Filler, +} + +#[derive(Debug, Clone, Copy)] +pub struct AlignedRow { + pub old: Row, + pub new: Row, + pub old_kind: CellKind, + pub new_kind: CellKind, +} + +impl AlignedRow { + /// True when this row is a paired change line (Del on old, Add on new) eligible for + /// word-level diffing. Unpaired excess lines get whole-line emphasis instead. + pub fn is_word_diff_pair(&self) -> bool { + matches!( + (self.old_kind, self.new_kind), + (CellKind::Del, CellKind::Add) + ) + } +} + +pub struct Aligned { + pub rows: Vec, +} + +fn gap_end(start: usize, count: usize) -> usize { + if count == 0 { + start + } else { + start - 1 + } +} + +/// Flush a pending del/add block, pairing by index and emitting filler rows for the excess on +/// the shorter side. +fn flush_block(dels: &[&HunkLine], adds: &[&HunkLine], rows: &mut Vec) { + let max_len = dels.len().max(adds.len()); + for i in 0..max_len { + let (old, old_kind) = match dels.get(i) { + Some(d) => ( + Row::Line(d.old_lnum.expect("deletion line has old_lnum") as usize), + CellKind::Del, + ), + None => (Row::Filler, CellKind::Filler), + }; + let (new, new_kind) = match adds.get(i) { + Some(a) => ( + Row::Line(a.new_lnum.expect("addition line has new_lnum") as usize), + CellKind::Add, + ), + None => (Row::Filler, CellKind::Filler), + }; + rows.push(AlignedRow { + old, + new, + old_kind, + new_kind, + }); + } +} + +/// Align a file's rows given its hunks. `old_line_count` / `new_line_count` are the total line +/// counts of the full old/new text, used to fill the tail gap after the last hunk. +pub fn align_file(hunks: &[Hunk], old_line_count: usize, new_line_count: usize) -> Aligned { + let mut rows = Vec::new(); + let mut old_pos = 0usize; // count of old lines already emitted + let mut new_pos = 0usize; + + for hunk in hunks { + let old_start = hunk.old_start as usize; + let old_count = hunk.old_count as usize; + let new_start = hunk.new_start as usize; + let new_count = hunk.new_count as usize; + + let old_ge = gap_end(old_start, old_count); + let new_ge = gap_end(new_start, new_count); + let old_gap = old_ge.saturating_sub(old_pos); + let new_gap = new_ge.saturating_sub(new_pos); + debug_assert_eq!( + old_gap, new_gap, + "context gap between hunks must be equal length on both sides" + ); + let gap = old_gap.min(new_gap); + for i in 0..gap { + rows.push(AlignedRow { + old: Row::Line(old_pos + i + 1), + new: Row::Line(new_pos + i + 1), + old_kind: CellKind::Context, + new_kind: CellKind::Context, + }); + } + + let mut pending_dels: Vec<&HunkLine> = Vec::new(); + let mut pending_adds: Vec<&HunkLine> = Vec::new(); + for line in &hunk.lines { + match line.kind { + LineKind::Deletion => pending_dels.push(line), + LineKind::Addition => pending_adds.push(line), + LineKind::Context => { + if !pending_dels.is_empty() || !pending_adds.is_empty() { + flush_block(&pending_dels, &pending_adds, &mut rows); + pending_dels.clear(); + pending_adds.clear(); + } + rows.push(AlignedRow { + old: Row::Line(line.old_lnum.expect("context line has old_lnum") as usize), + new: Row::Line(line.new_lnum.expect("context line has new_lnum") as usize), + old_kind: CellKind::Context, + new_kind: CellKind::Context, + }); + } + } + } + if !pending_dels.is_empty() || !pending_adds.is_empty() { + flush_block(&pending_dels, &pending_adds, &mut rows); + } + + old_pos = old_start + old_count.saturating_sub(1); + new_pos = new_start + new_count.saturating_sub(1); + } + + // Tail gap after the last hunk (or the whole file, if there are no hunks). + let old_tail = old_line_count.saturating_sub(old_pos); + let new_tail = new_line_count.saturating_sub(new_pos); + debug_assert_eq!( + old_tail, new_tail, + "trailing context after the last hunk must be equal length on both sides" + ); + let tail = old_tail.min(new_tail); + for i in 0..tail { + rows.push(AlignedRow { + old: Row::Line(old_pos + i + 1), + new: Row::Line(new_pos + i + 1), + old_kind: CellKind::Context, + new_kind: CellKind::Context, + }); + } + + Aligned { rows } +} + +/// A row of the gap-collapsed display, layered over [`AlignedRow`]s. +/// +/// Unchanged stretches longer than `2 * CONTEXT_LINES` collapse to a single [`DisplayRow::Gap`] +/// so the view doesn't scroll through pages of untouched code. Gap rows are layout-agnostic — +/// they span both panes in SBS. +#[derive(Debug, Clone, Copy)] +pub enum DisplayRow { + Row(AlignedRow), + Gap { skipped: usize }, +} + +/// Number of context lines kept around hunk content on each side of a gap. +pub const CONTEXT_LINES: usize = 3; + +/// Collapse long unchanged stretches in `rows` into [`DisplayRow::Gap`] markers, keeping +/// [`CONTEXT_LINES`] rows of context immediately around hunk content (Del/Add/Filler rows). +/// +/// A stretch of context rows collapses only when it is strictly longer than `2 * CONTEXT_LINES` +/// (enough to keep `CONTEXT_LINES` on both sides of the gap); shorter stretches, including ones +/// between two hunks that are close together, are left as-is (no gap row — the hunks +/// effectively merge under one continuous context run). +pub fn collapse_gaps(rows: &[AlignedRow]) -> Vec { + collapse_gaps_with(rows, CONTEXT_LINES) +} + +/// Same as [`collapse_gaps`] but with an explicit context-line count, for testing. +fn collapse_gaps_with(rows: &[AlignedRow], context: usize) -> Vec { + let is_context = |row: &AlignedRow| { + matches!( + (row.old_kind, row.new_kind), + (CellKind::Context, CellKind::Context) + ) + }; + + let mut out = Vec::with_capacity(rows.len()); + let mut i = 0; + while i < rows.len() { + if !is_context(&rows[i]) { + out.push(DisplayRow::Row(rows[i])); + i += 1; + continue; + } + + // Measure the full run of context rows starting at i. + let run_start = i; + let mut run_end = i; + while run_end < rows.len() && is_context(&rows[run_end]) { + run_end += 1; + } + let run_len = run_end - run_start; + + // Keep `context` lines of lead-in unless this run touches the start of the file (no + // hunk before it to lead away from) or the end of the file (no hunk after it to lead + // into) — those edges get no filler on the missing side. + let keep_before = if run_start == 0 { 0 } else { context }; + let keep_after = if run_end == rows.len() { 0 } else { context }; + + if (keep_before == 0 && keep_after == 0) || run_len <= keep_before + keep_after { + // Either too short to collapse, or (keep_before == keep_after == 0) this run is + // the entire row list — a wholly unchanged file with no hunk on either side to + // contextualize. Emit every row, no gap. + for row in &rows[run_start..run_end] { + out.push(DisplayRow::Row(*row)); + } + } else { + for row in &rows[run_start..run_start + keep_before] { + out.push(DisplayRow::Row(*row)); + } + let skipped = run_len - keep_before - keep_after; + out.push(DisplayRow::Gap { skipped }); + for row in &rows[run_end - keep_after..run_end] { + out.push(DisplayRow::Row(*row)); + } + } + + i = run_end; + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::model::{Hunk, HunkLine, LineKind}; + + fn hl(kind: LineKind, old: Option, new: Option) -> HunkLine { + HunkLine { + kind, + content: Vec::new(), + old_lnum: old, + new_lnum: new, + missing_newline: false, + } + } + + fn hunk( + old_start: u32, + old_count: u32, + new_start: u32, + new_count: u32, + lines: Vec, + ) -> Hunk { + Hunk { + old_start, + old_count, + new_start, + new_count, + header: Vec::new(), + lines, + } + } + + #[test] + fn parity_invariant_holds() { + // 3 dels / 1 add block inside a hunk with context on both sides. + let h = hunk( + 1, + 5, + 1, + 3, + vec![ + hl(LineKind::Context, Some(1), Some(1)), + hl(LineKind::Deletion, Some(2), None), + hl(LineKind::Deletion, Some(3), None), + hl(LineKind::Deletion, Some(4), None), + hl(LineKind::Addition, None, Some(2)), + hl(LineKind::Context, Some(5), Some(3)), + ], + ); + let aligned = align_file(&[h], 5, 3); + for row in &aligned.rows { + assert_eq!( + matches!(row.old, Row::Filler), + row.old_kind == CellKind::Filler + ); + assert_eq!( + matches!(row.new, Row::Filler), + row.new_kind == CellKind::Filler + ); + } + + // ctx1, then 3 paired-or-filler rows for the del/add block, then ctx2. + assert_eq!(aligned.rows.len(), 5); + assert_eq!(aligned.rows[0].old_kind, CellKind::Context); + assert_eq!(aligned.rows[0].new_kind, CellKind::Context); + + // del1/add1 paired. + assert_eq!(aligned.rows[1].old, Row::Line(2)); + assert_eq!(aligned.rows[1].new, Row::Line(2)); + assert_eq!(aligned.rows[1].old_kind, CellKind::Del); + assert_eq!(aligned.rows[1].new_kind, CellKind::Add); + assert!(aligned.rows[1].is_word_diff_pair()); + + // del2/del3 have no add counterpart -> filler on new side. + assert_eq!(aligned.rows[2].old, Row::Line(3)); + assert_eq!(aligned.rows[2].new, Row::Filler); + assert_eq!(aligned.rows[2].new_kind, CellKind::Filler); + assert!(!aligned.rows[2].is_word_diff_pair()); + + assert_eq!(aligned.rows[3].old, Row::Line(4)); + assert_eq!(aligned.rows[3].new, Row::Filler); + + assert_eq!(aligned.rows[4].old_kind, CellKind::Context); + assert_eq!(aligned.rows[4].old, Row::Line(5)); + assert_eq!(aligned.rows[4].new, Row::Line(3)); + } + + #[test] + fn pure_addition_at_start_of_file() { + let h = hunk( + 0, + 0, + 1, + 2, + vec![ + hl(LineKind::Addition, None, Some(1)), + hl(LineKind::Addition, None, Some(2)), + ], + ); + let aligned = align_file(&[h], 0, 2); + assert_eq!(aligned.rows.len(), 2); + assert_eq!(aligned.rows[0].old, Row::Filler); + assert_eq!(aligned.rows[0].new, Row::Line(1)); + assert_eq!(aligned.rows[1].old, Row::Filler); + assert_eq!(aligned.rows[1].new, Row::Line(2)); + } + + #[test] + fn no_hunks_pairs_whole_file_1to1() { + let aligned = align_file(&[], 4, 4); + assert_eq!(aligned.rows.len(), 4); + for (i, row) in aligned.rows.iter().enumerate() { + assert_eq!(row.old, Row::Line(i + 1)); + assert_eq!(row.new, Row::Line(i + 1)); + assert_eq!(row.old_kind, CellKind::Context); + } + } + + fn context_row(n: usize) -> AlignedRow { + AlignedRow { + old: Row::Line(n), + new: Row::Line(n), + old_kind: CellKind::Context, + new_kind: CellKind::Context, + } + } + + fn change_row(old: Row, new: Row, old_kind: CellKind, new_kind: CellKind) -> AlignedRow { + AlignedRow { + old, + new, + old_kind, + new_kind, + } + } + + #[test] + fn tiny_file_produces_no_gaps() { + // Whole file is context, shorter than 2 * context: no gap. + let rows: Vec = (1..=4).map(context_row).collect(); + let display = collapse_gaps_with(&rows, 3); + assert_eq!(display.len(), 4); + assert!(display.iter().all(|r| matches!(r, DisplayRow::Row(_)))); + } + + #[test] + fn gap_between_hunks_collapses_middle() { + // hunk1 change, 10 lines context, hunk2 change: with context=3, the middle 4 lines + // (10 - 3 - 3) collapse into one gap row. + let mut rows = vec![change_row( + Row::Line(1), + Row::Line(1), + CellKind::Del, + CellKind::Add, + )]; + rows.extend((2..=11).map(context_row)); + rows.push(change_row( + Row::Line(12), + Row::Line(12), + CellKind::Del, + CellKind::Add, + )); + + let display = collapse_gaps_with(&rows, 3); + // change, 3 ctx, gap, 3 ctx, change + assert_eq!(display.len(), 9); + assert!(matches!(display[0], DisplayRow::Row(_))); + for row in &display[1..4] { + assert!(matches!(row, DisplayRow::Row(r) if r.old_kind == CellKind::Context)); + } + match display[4] { + DisplayRow::Gap { skipped } => assert_eq!(skipped, 4), + other => panic!("expected gap row, got {other:?}"), + } + for row in &display[5..8] { + assert!(matches!(row, DisplayRow::Row(r) if r.old_kind == CellKind::Context)); + } + assert!(matches!(display[8], DisplayRow::Row(_))); + + // The gap hides the same count on both sides by construction (rows are already + // parity-paired context lines), but assert explicitly on the surviving rows' + // continuity: line just before the gap and line just after are the expected distance + // apart on both old and new sides. + if let (DisplayRow::Row(before), DisplayRow::Row(after)) = (display[3], display[5]) { + let (Row::Line(before_old), Row::Line(before_new)) = (before.old, before.new) else { + panic!("expected line rows around the gap"); + }; + // after is the next change row (old=12,new=12); the gap plus kept context must + // account for all lines strictly between. + let (Row::Line(after_old), Row::Line(after_new)) = (after.old, after.new) else { + panic!("expected line rows around the gap"); + }; + assert_eq!( + after_old - before_old, + after_new - before_new, + "gap hides equal spans" + ); + } + } + + #[test] + fn adjacent_hunks_with_too_little_context_merge_without_gap() { + // Only 4 lines of context between two change blocks with context=3: 4 <= 3+3, no gap. + let mut rows = vec![change_row( + Row::Line(1), + Row::Line(1), + CellKind::Del, + CellKind::Add, + )]; + rows.extend((2..=5).map(context_row)); + rows.push(change_row( + Row::Line(6), + Row::Line(6), + CellKind::Del, + CellKind::Add, + )); + + let display = collapse_gaps_with(&rows, 3); + assert_eq!(display.len(), rows.len()); + assert!(display.iter().all(|r| matches!(r, DisplayRow::Row(_)))); + } + + #[test] + fn gap_at_file_start_has_no_lead_in() { + // Leading context run (file starts unchanged) before the first hunk: no context to + // "lead away from" on the left edge, so the whole run before the trailing keep-window + // can collapse. + let mut rows: Vec = (1..=10).map(context_row).collect(); + rows.push(change_row( + Row::Line(11), + Row::Line(11), + CellKind::Del, + CellKind::Add, + )); + + let display = collapse_gaps_with(&rows, 3); + // gap, 3 ctx, change + assert_eq!(display.len(), 5); + match display[0] { + DisplayRow::Gap { skipped } => assert_eq!(skipped, 7), + other => panic!("expected gap row, got {other:?}"), + } + for row in &display[1..4] { + assert!(matches!(row, DisplayRow::Row(_))); + } + assert!(matches!(display[4], DisplayRow::Row(_))); + } + + #[test] + fn gap_at_file_end_has_no_trail_out() { + let mut rows = vec![change_row( + Row::Line(1), + Row::Line(1), + CellKind::Del, + CellKind::Add, + )]; + rows.extend((2..=11).map(context_row)); + + let display = collapse_gaps_with(&rows, 3); + // change, 3 ctx, gap + assert_eq!(display.len(), 5); + assert!(matches!(display[0], DisplayRow::Row(_))); + for row in &display[1..4] { + assert!(matches!(row, DisplayRow::Row(_))); + } + match display[4] { + DisplayRow::Gap { skipped } => assert_eq!(skipped, 7), + other => panic!("expected gap row, got {other:?}"), + } + } +} diff --git a/git-workon-review/src/lib.rs b/git-workon-review/src/lib.rs index f3aa6f67..5ac59e54 100644 --- a/git-workon-review/src/lib.rs +++ b/git-workon-review/src/lib.rs @@ -13,6 +13,7 @@ //! verdict corpus lands in the next M2 changeset. pub mod acquire; +pub mod align; pub mod apply; pub mod error; pub mod file_ops; From f8a50bb30e20de9c87ba44f49b50ce148fdf9e4d Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Mon, 6 Jul 2026 18:39:30 -0400 Subject: [PATCH 3/7] feat(review): port word-diff spans and tree-sitter highlighting --- Cargo.lock | 77 ++++++ Cargo.toml | 7 + git-workon-review/Cargo.toml | 13 +- git-workon-review/src/highlight.rs | 397 +++++++++++++++++++++++++++++ git-workon-review/src/lib.rs | 2 + git-workon-review/src/wordiff.rs | 91 +++++++ 6 files changed, 584 insertions(+), 3 deletions(-) create mode 100644 git-workon-review/src/highlight.rs create mode 100644 git-workon-review/src/wordiff.rs diff --git a/Cargo.lock b/Cargo.lock index be644ab1..34027481 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -968,10 +968,17 @@ dependencies = [ "miette", "predicates", "ratatui", + "similar", "thiserror 2.0.19", "tree-sitter", "tree-sitter-highlight", + "tree-sitter-javascript", + "tree-sitter-json", + "tree-sitter-lua", + "tree-sitter-md", "tree-sitter-rust", + "tree-sitter-toml-ng", + "tree-sitter-typescript", ] [[package]] @@ -2356,6 +2363,15 @@ dependencies = [ "libc", ] +[[package]] +name = "similar" +version = "3.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6505efef05804732ed8a3f2d4f279429eb485bd69d5b0cc6b19cc02005cda16" +dependencies = [ + "bstr", +] + [[package]] name = "siphasher" version = "1.0.3" @@ -2728,12 +2744,53 @@ dependencies = [ "tree-sitter", ] +[[package]] +name = "tree-sitter-javascript" +version = "0.25.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "68204f2abc0627a90bdf06e605f5c470aa26fdcb2081ea553a04bdad756693f5" +dependencies = [ + "cc", + "tree-sitter-language", +] + +[[package]] +name = "tree-sitter-json" +version = "0.24.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4d727acca406c0020cffc6cf35516764f36c8e3dc4408e5ebe2cb35a947ec471" +dependencies = [ + "cc", + "tree-sitter-language", +] + [[package]] name = "tree-sitter-language" version = "0.1.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "009994f150cc0cd50ff54917d5bc8bffe8cad10ca10d81c34da2ec421ae61782" +[[package]] +name = "tree-sitter-lua" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8daaf5f4235188a58603c39760d5fa5d4b920d36a299c934adddae757f32a10c" +dependencies = [ + "cc", + "tree-sitter-language", +] + +[[package]] +name = "tree-sitter-md" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2efd398be546456c814598ee56c0f51769a77241511b4a58077815d120afa882" +dependencies = [ + "cc", + "tree-sitter", + "tree-sitter-language", +] + [[package]] name = "tree-sitter-rust" version = "0.24.2" @@ -2744,6 +2801,26 @@ dependencies = [ "tree-sitter-language", ] +[[package]] +name = "tree-sitter-toml-ng" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e9adc2c898ae49730e857d75be403da3f92bb81d8e37a2f918a08dd10de5ebb1" +dependencies = [ + "cc", + "tree-sitter-language", +] + +[[package]] +name = "tree-sitter-typescript" +version = "0.23.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6c5f76ed8d947a75cc446d5fccd8b602ebf0cde64ccf2ffa434d873d7a575eff" +dependencies = [ + "cc", + "tree-sitter-language", +] + [[package]] name = "typenum" version = "1.20.1" diff --git a/Cargo.toml b/Cargo.toml index 77756137..f0623ed3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,10 +49,17 @@ ratatui = "0.30" rusqlite = { version = "0.40", features = ["bundled"] } serde_json = "1.0" serial_test = "3" +similar = "3.1.1" thiserror = "2.0.18" tree-sitter = "0.26" tree-sitter-highlight = "0.26" +tree-sitter-javascript = "0.25.0" +tree-sitter-json = "0.24.8" +tree-sitter-lua = "0.5.0" +tree-sitter-md = { version = "0.5.3", features = ["parser"] } tree-sitter-rust = "0.24" +tree-sitter-toml-ng = "0.7.0" +tree-sitter-typescript = "0.23.2" unicode-width = "0.2.2" # The profile that 'dist' will build with diff --git a/git-workon-review/Cargo.toml b/git-workon-review/Cargo.toml index 2f9c731d..5de95e6b 100644 --- a/git-workon-review/Cargo.toml +++ b/git-workon-review/Cargo.toml @@ -37,7 +37,17 @@ git-workon-lib.workspace = true git2.workspace = true miette.workspace = true ratatui.workspace = true +similar.workspace = true thiserror.workspace = true +tree-sitter.workspace = true +tree-sitter-highlight.workspace = true +tree-sitter-javascript.workspace = true +tree-sitter-json.workspace = true +tree-sitter-lua.workspace = true +tree-sitter-md.workspace = true +tree-sitter-rust.workspace = true +tree-sitter-toml-ng.workspace = true +tree-sitter-typescript.workspace = true [package.metadata.dist] # Redundant with publish = false today; load-bearing at the M3 flip so @@ -48,6 +58,3 @@ dist = false assert_cmd.workspace = true git-workon-fixture.workspace = true predicates.workspace = true -tree-sitter.workspace = true -tree-sitter-highlight.workspace = true -tree-sitter-rust.workspace = true diff --git a/git-workon-review/src/highlight.rs b/git-workon-review/src/highlight.rs new file mode 100644 index 00000000..403d86e4 --- /dev/null +++ b/git-workon-review/src/highlight.rs @@ -0,0 +1,397 @@ +//! Syntax highlighting via tree-sitter. +//! +//! One `HighlightConfiguration` is built lazily per language and cached. +//! Highlight events give byte offsets over the whole source; we split them +//! into per-line spans here so the renderer can compose them against +//! word-diff spans without re-deriving line boundaries. + +use std::collections::HashMap; + +use ratatui::style::Color; +use tree_sitter_highlight::{HighlightConfiguration, HighlightEvent, Highlighter}; + +/// Files with more lines than this are skipped (plain fg) to keep +/// highlighting fast. +pub const MAX_HIGHLIGHT_LINES: usize = 20_000; + +/// Foreground color spans for a single line: byte range + color. +#[derive(Debug, Clone)] +pub struct FgSpan { + pub start: usize, + pub end: usize, + pub color: Color, +} + +/// The standard highlight-capture names we recognize. `configure()` matches +/// dotted capture names by longest prefix, so e.g. `keyword.control` maps to +/// `keyword`. Parallel with `HIGHLIGHT_COLORS`. +const HIGHLIGHT_NAMES: &[&str] = &[ + "attribute", + "comment", + "constant", + "constant.builtin", + "constructor", + "embedded", + "escape", + "function", + "function.builtin", + "function.macro", + "function.method", + "keyword", + "label", + "number", + "operator", + "property", + "punctuation", + "punctuation.bracket", + "punctuation.delimiter", + "punctuation.special", + "string", + "string.special", + "tag", + "type", + "type.builtin", + "variable", + "variable.builtin", + "variable.parameter", +]; + +// A small dark theme in the same family as syntect's base16-eighties.dark so +// the two engines look comparable side by side. +const C_RED: Color = Color::Rgb(0xf2, 0x77, 0x7a); +const C_ORANGE: Color = Color::Rgb(0xf9, 0x91, 0x57); +const C_YELLOW: Color = Color::Rgb(0xff, 0xcc, 0x66); +const C_GREEN: Color = Color::Rgb(0x99, 0xcc, 0x99); +const C_CYAN: Color = Color::Rgb(0x66, 0xcc, 0xcc); +const C_BLUE: Color = Color::Rgb(0x66, 0x99, 0xcc); +const C_PURPLE: Color = Color::Rgb(0xcc, 0x99, 0xcc); +const C_FG: Color = Color::Rgb(0xd3, 0xd0, 0xc8); +const C_COMMENT: Color = Color::Rgb(0x74, 0x73, 0x69); + +const HIGHLIGHT_COLORS: &[Color] = &[ + C_ORANGE, // attribute + C_COMMENT, // comment + C_ORANGE, // constant + C_ORANGE, // constant.builtin + C_YELLOW, // constructor + C_FG, // embedded + C_CYAN, // escape + C_BLUE, // function + C_BLUE, // function.builtin + C_BLUE, // function.macro + C_BLUE, // function.method + C_PURPLE, // keyword + C_RED, // label + C_ORANGE, // number + C_FG, // operator + C_CYAN, // property + C_FG, // punctuation + C_FG, // punctuation.bracket + C_FG, // punctuation.delimiter + C_CYAN, // punctuation.special + C_GREEN, // string + C_CYAN, // string.special + C_RED, // tag + C_YELLOW, // type + C_YELLOW, // type.builtin + C_FG, // variable + C_RED, // variable.builtin + C_FG, // variable.parameter +]; + +/// Color for a highlight-capture name, for tests and debugging. +#[cfg(test)] +pub fn color_of(name: &str) -> Option { + HIGHLIGHT_NAMES + .iter() + .position(|n| *n == name) + .map(|i| HIGHLIGHT_COLORS[i]) +} + +fn lang_key_for_ext(ext: &str) -> Option<&'static str> { + match ext { + "rs" => Some("rust"), + "lua" => Some("lua"), + "json" => Some("json"), + "toml" => Some("toml"), + "js" | "mjs" | "cjs" | "jsx" => Some("javascript"), + "ts" | "mts" | "cts" => Some("typescript"), + "tsx" => Some("tsx"), + "md" | "markdown" => Some("markdown"), + _ => None, + } +} + +fn build_config(key: &'static str) -> Option { + let result = match key { + "rust" => HighlightConfiguration::new( + tree_sitter_rust::LANGUAGE.into(), + "rust", + tree_sitter_rust::HIGHLIGHTS_QUERY, + tree_sitter_rust::INJECTIONS_QUERY, + "", + ), + "lua" => HighlightConfiguration::new( + tree_sitter_lua::LANGUAGE.into(), + "lua", + tree_sitter_lua::HIGHLIGHTS_QUERY, + tree_sitter_lua::INJECTIONS_QUERY, + tree_sitter_lua::LOCALS_QUERY, + ), + "json" => HighlightConfiguration::new( + tree_sitter_json::LANGUAGE.into(), + "json", + tree_sitter_json::HIGHLIGHTS_QUERY, + "", + "", + ), + "toml" => HighlightConfiguration::new( + tree_sitter_toml_ng::LANGUAGE.into(), + "toml", + tree_sitter_toml_ng::HIGHLIGHTS_QUERY, + "", + "", + ), + "javascript" => { + // The JS grammar includes JSX nodes, so the JSX query is safe to + // append for plain .js too. + let highlights = format!( + "{}{}", + tree_sitter_javascript::HIGHLIGHT_QUERY, + tree_sitter_javascript::JSX_HIGHLIGHT_QUERY + ); + HighlightConfiguration::new( + tree_sitter_javascript::LANGUAGE.into(), + "javascript", + &highlights, + tree_sitter_javascript::INJECTIONS_QUERY, + tree_sitter_javascript::LOCALS_QUERY, + ) + } + "typescript" => { + // tree-sitter-highlight gives precedence to the LAST matching pattern, so the + // inherited javascript query goes first and the language-specific query is + // appended (wins on conflicts). + let highlights = format!( + "{}{}", + tree_sitter_javascript::HIGHLIGHT_QUERY, + tree_sitter_typescript::HIGHLIGHTS_QUERY + ); + HighlightConfiguration::new( + tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into(), + "typescript", + &highlights, + "", + tree_sitter_typescript::LOCALS_QUERY, + ) + } + "tsx" => { + // Same last-wins precedence as the typescript arm above. + let highlights = format!( + "{}{}{}", + tree_sitter_javascript::HIGHLIGHT_QUERY, + tree_sitter_javascript::JSX_HIGHLIGHT_QUERY, + tree_sitter_typescript::HIGHLIGHTS_QUERY + ); + HighlightConfiguration::new( + tree_sitter_typescript::LANGUAGE_TSX.into(), + "tsx", + &highlights, + "", + tree_sitter_typescript::LOCALS_QUERY, + ) + } + "markdown" => HighlightConfiguration::new( + tree_sitter_md::LANGUAGE.into(), + "markdown", + tree_sitter_md::HIGHLIGHT_QUERY_BLOCK, + "", + "", + ), + _ => return None, + }; + + match result { + Ok(mut config) => { + config.configure(HIGHLIGHT_NAMES); + Some(config) + } + Err(_) => None, + } +} + +pub struct TsHighlighter { + core: Highlighter, + /// Lazily built configs; `None` records a failed build so we don't retry. + configs: HashMap<&'static str, Option>, +} + +impl TsHighlighter { + pub fn new() -> Self { + Self { + core: Highlighter::new(), + configs: HashMap::new(), + } + } + + /// Highlight the full text of a file, returning one Vec per line. + /// `None` means: no grammar for this extension, file too large, or a + /// highlight error — caller should fall back to unhighlighted text. + pub fn highlight_file(&mut self, path: &str, text: &str) -> Option>> { + let ext = std::path::Path::new(path) + .extension() + .and_then(|e| e.to_str()) + .unwrap_or(""); + let key = lang_key_for_ext(ext)?; + + let line_count = text.lines().count(); + if line_count > MAX_HIGHLIGHT_LINES { + return None; + } + + let config = self + .configs + .entry(key) + .or_insert_with(|| build_config(key)) + .as_ref()?; + + // Byte offset of each line start; used to split whole-source spans + // into per-line spans. + let mut line_starts = vec![0usize]; + for (i, b) in text.bytes().enumerate() { + if b == b'\n' { + line_starts.push(i + 1); + } + } + + let mut out: Vec> = vec![Vec::new(); line_count]; + let mut stack: Vec = Vec::new(); + + let events = self + .core + .highlight(config, text.as_bytes(), None, |_| None) + .ok()?; + + for event in events { + match event.ok()? { + HighlightEvent::HighlightStart(h) => stack.push(h.0), + HighlightEvent::HighlightEnd => { + stack.pop(); + } + HighlightEvent::Source { start, end } => { + let Some(&idx) = stack.last() else { continue }; + let color = HIGHLIGHT_COLORS[idx]; + let mut pos = start; + while pos < end { + let line_idx = line_starts.partition_point(|&s| s <= pos) - 1; + if line_idx >= line_count { + break; + } + let line_start = line_starts[line_idx]; + // End of line content, excluding the trailing '\n'. + let line_end = line_starts + .get(line_idx + 1) + .map(|s| s - 1) + .unwrap_or(text.len()); + let seg_end = end.min(line_end); + if pos < seg_end { + out[line_idx].push(FgSpan { + start: pos - line_start, + end: seg_end - line_start, + color, + }); + } + pos = match line_starts.get(line_idx + 1) { + Some(&next) => next, + None => end, + }; + } + } + } + } + + Some(out) + } +} + +impl Default for TsHighlighter { + fn default() -> Self { + Self::new() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn names_and_colors_are_parallel() { + assert_eq!(HIGHLIGHT_NAMES.len(), HIGHLIGHT_COLORS.len()); + } + + #[test] + fn rust_snippet_yields_expected_span_kinds_on_right_lines() { + let src = "fn main() {\n let s = \"hi\";\n}\n"; + let mut ts = TsHighlighter::new(); + let hl = ts + .highlight_file("test.rs", src) + .expect("rust grammar available"); + assert_eq!(hl.len(), 3); + + // Line 0: `fn` at bytes 0..2 should be keyword-colored. + let kw = color_of("keyword").unwrap(); + assert!( + hl[0] + .iter() + .any(|s| s.start == 0 && s.end >= 2 && s.color == kw), + "expected keyword span over `fn` on line 0, got {:?}", + hl[0] + ); + + // Line 0: `main` should be function-colored. + let func = color_of("function").unwrap(); + assert!( + hl[0] + .iter() + .any(|s| { s.color == func && &src[..11][s.start..s.end.min(11)] == "main" }), + "expected function span over `main` on line 0, got {:?}", + hl[0] + ); + + // Line 1: string literal should be string-colored. + let string = color_of("string").unwrap(); + assert!( + hl[1].iter().any(|s| s.color == string), + "expected string span on line 1, got {:?}", + hl[1] + ); + } + + #[test] + fn unknown_extension_returns_none() { + let mut ts = TsHighlighter::new(); + assert!(ts.highlight_file("mystery.zzz", "hello world\n").is_none()); + assert!(ts.highlight_file("no_extension", "hello world\n").is_none()); + } + + #[test] + fn spans_never_cross_line_boundaries() { + let src = "/* a\nmultiline\ncomment */\n"; + let mut ts = TsHighlighter::new(); + let hl = ts.highlight_file("c.rs", src).unwrap(); + let line_lens: Vec = src.lines().map(|l| l.len()).collect(); + for (i, spans) in hl.iter().enumerate() { + for s in spans { + assert!(s.end <= line_lens[i], "span {s:?} exceeds line {i} length"); + } + } + // The multiline comment should produce comment spans on all 3 lines. + let comment = color_of("comment").unwrap(); + for (i, spans) in hl.iter().enumerate() { + assert!( + spans.iter().any(|s| s.color == comment), + "expected comment span on line {i}" + ); + } + } +} diff --git a/git-workon-review/src/lib.rs b/git-workon-review/src/lib.rs index 5ac59e54..9ef8365a 100644 --- a/git-workon-review/src/lib.rs +++ b/git-workon-review/src/lib.rs @@ -17,8 +17,10 @@ pub mod align; pub mod apply; pub mod error; pub mod file_ops; +pub mod highlight; pub mod model; pub mod ops; pub mod queue; pub mod refresh; pub mod synthesis; +pub mod wordiff; diff --git a/git-workon-review/src/wordiff.rs b/git-workon-review/src/wordiff.rs new file mode 100644 index 00000000..5eb0d478 --- /dev/null +++ b/git-workon-review/src/wordiff.rs @@ -0,0 +1,91 @@ +//! Word-level diff spans for a paired del/add line. + +use similar::{ChangeTag, TextDiff}; + +/// A byte range `[start, end)` into a line's text that should be rendered +/// with emphasized ("strong") diff background. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Span { + pub start: usize, + pub end: usize, +} + +/// Compute word-granularity change spans for a paired old/new line. Returns +/// (old_spans, new_spans): byte ranges into `old_text` / `new_text` that +/// differ at word granularity. +pub fn word_diff_spans(old_text: &str, new_text: &str) -> (Vec, Vec) { + let diff = TextDiff::configure().diff_words(old_text, new_text); + + let mut old_spans = Vec::new(); + let mut new_spans = Vec::new(); + let mut old_pos = 0usize; + let mut new_pos = 0usize; + + for change in diff.iter_all_changes() { + let len = change.value().len(); + match change.tag() { + ChangeTag::Equal => { + old_pos += len; + new_pos += len; + } + ChangeTag::Delete => { + old_spans.push(Span { + start: old_pos, + end: old_pos + len, + }); + old_pos += len; + } + ChangeTag::Insert => { + new_spans.push(Span { + start: new_pos, + end: new_pos + len, + }); + new_pos += len; + } + } + } + + (merge_adjacent(old_spans), merge_adjacent(new_spans)) +} + +/// Merge spans that are directly adjacent (no gap) to reduce fragmentation +/// from word-boundary splitting. +fn merge_adjacent(mut spans: Vec) -> Vec { + spans.sort_by_key(|s| s.start); + let mut out: Vec = Vec::with_capacity(spans.len()); + for span in spans { + if let Some(last) = out.last_mut() { + if last.end == span.start { + last.end = span.end; + continue; + } + } + out.push(span); + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn detects_changed_word() { + let (old_spans, new_spans) = word_diff_spans("let x = 1;", "let x = 10;"); + assert!(!old_spans.is_empty()); + assert!(!new_spans.is_empty()); + let old_changed = &old_spans[0]; + let old_text = "let x = 1;"; + assert!(old_text[old_changed.start..old_changed.end].contains('1')); + let new_changed = &new_spans[0]; + let new_text = "let x = 10;"; + assert!(new_text[new_changed.start..new_changed.end].contains("10")); + } + + #[test] + fn identical_lines_have_no_spans() { + let (old_spans, new_spans) = word_diff_spans("same line", "same line"); + assert!(old_spans.is_empty()); + assert!(new_spans.is_empty()); + } +} From 13c18a7616f8b10616fb664f451846d4ff1fed1b Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Mon, 6 Jul 2026 18:53:10 -0400 Subject: [PATCH 4/7] feat(review): render side-by-side diff frames with highlights --- git-workon-review/src/app.rs | 447 +++++++++++++++++++++++++++ git-workon-review/src/lib.rs | 2 + git-workon-review/src/render.rs | 522 ++++++++++++++++++++++++++++++++ 3 files changed, 971 insertions(+) create mode 100644 git-workon-review/src/app.rs create mode 100644 git-workon-review/src/render.rs diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs new file mode 100644 index 00000000..510daac2 --- /dev/null +++ b/git-workon-review/src/app.rs @@ -0,0 +1,447 @@ +//! App state: the file list being reviewed, per-file view data (full text + alignment + +//! highlight cache + word-diff cache), and navigation/scroll state. +//! +//! Ported from the `review-tui-spike` prototype's `model.rs` — renamed here because `model` +//! already means the diff model in this crate (see the M3 plan's naming rule). +//! +//! Renders the **combined** (`HEAD` ↔ worktree) diff only (locked design decision #2 in the M3 +//! plan) — the staged/unstaged split zoom is M4. [`App`] owns its own [`git2::Repository`] +//! handle so it can lazily read blob/worktree content per file as the user navigates to it, +//! independent of whatever handle acquired the [`DiffModel`] it was built from. + +use std::collections::HashMap; +use std::path::Path; + +use git2::Repository; + +use crate::align::{align_file, collapse_gaps, CellKind, DisplayRow, Row}; +use crate::highlight::{FgSpan, TsHighlighter}; +use crate::model::{DiffModel, FileChange, FileStatus}; +use crate::wordiff::{word_diff_spans, Span}; + +/// Loaded, aligned, highlighted view of one file's combined diff. +/// +/// Full text is read once per side, from whichever source the file's status says still exists: +/// +/// | status | old-side source | new-side source | +/// |-----------------------|-------------------------------------|-----------------------------| +/// | Added / Untracked | none (empty) | worktree file on disk | +/// | Deleted | `HEAD` blob at `path` | none (empty) | +/// | Renamed / Copied | `HEAD` blob at `old_path` | worktree file at `path` | +/// | Modified / Unmerged | `HEAD` blob at `path` | worktree file at `path` | +/// +/// The new side reads from the **worktree file on disk**, not the index blob — unstaged +/// content isn't in the object database; reading the staged (index) blob is an M4 concern (the +/// staged/unstaged split zoom). +pub struct FileView { + old_text: String, + new_text: String, + old_lines: Vec, + new_lines: Vec, + /// The gap-collapsed row list the renderer walks. Word-diff spans and scroll coordinates + /// are indexed against THIS vector, not the pre-collapse `AlignedRow` vector — collapsing + /// only removes uninteresting context, so the underlying [`Row`]/[`CellKind`] pairing for + /// any surviving row is unchanged. + pub display: Vec, + /// Index into [`Self::display`] of the first hunk's first row (or 0 for a file with no + /// hunks), for the initial scroll jump. + pub first_hunk_row: usize, + pub old_hl: Option>>, + pub new_hl: Option>>, + /// Lazily computed word-diff spans, keyed by DISPLAY row index — the only coordinate the + /// renderer's viewport walks once gaps are collapsed. + word_spans: HashMap, Vec)>, +} + +impl FileView { + fn load( + repo: &Repository, + head_tree: &git2::Tree<'_>, + file: &FileChange, + ts: &mut TsHighlighter, + ) -> Self { + let old_source_path = file.old_path.as_deref().unwrap_or(file.path.as_str()); + let old_text = match file.status { + FileStatus::Added | FileStatus::Untracked => String::new(), + _ => read_head_blob(repo, head_tree, old_source_path), + }; + + let new_text = match file.status { + FileStatus::Deleted => String::new(), + _ => read_workdir_file(repo, &file.path), + }; + + let old_lines: Vec = old_text.lines().map(str::to_string).collect(); + let new_lines: Vec = new_text.lines().map(str::to_string).collect(); + + let aligned = align_file(&file.hunks, old_lines.len(), new_lines.len()); + let display = collapse_gaps(&aligned.rows); + let first_hunk_row = display + .iter() + .position(|row| { + matches!( + row, + DisplayRow::Row(r) if !(r.old_kind == CellKind::Context && r.new_kind == CellKind::Context) + ) + }) + .unwrap_or(0); + + let old_hl = ts.highlight_file(old_source_path, &old_text); + let new_hl = ts.highlight_file(&file.path, &new_text); + + Self { + old_text, + new_text, + old_lines, + new_lines, + display, + first_hunk_row, + old_hl, + new_hl, + word_spans: HashMap::new(), + } + } + + pub fn old_line(&self, n: usize) -> &str { + self.old_lines + .get(n.saturating_sub(1)) + .map(String::as_str) + .unwrap_or("") + } + + pub fn new_line(&self, n: usize) -> &str { + self.new_lines + .get(n.saturating_sub(1)) + .map(String::as_str) + .unwrap_or("") + } + + pub fn old_line_count(&self) -> usize { + self.old_lines.len() + } + + pub fn new_line_count(&self) -> usize { + self.new_lines.len() + } + + /// Full text loaded for the old/new side, for callers that need the whole blob rather than + /// line-by-line access (e.g. re-running highlighting at a different width is NOT needed + /// today, but tests assert against this directly). + pub fn old_text(&self) -> &str { + &self.old_text + } + + pub fn new_text(&self) -> &str { + &self.new_text + } + + /// Lazily compute (and cache) word-diff spans for a paired display row. Returns empty spans + /// (and does not populate the cache) for a row that isn't a `(Del, Add)` pair — callers + /// check [`crate::align::AlignedRow::is_word_diff_pair`] first in the common case, but this + /// stays total so it's safe to call unconditionally. + pub fn word_spans_for_row(&mut self, display_idx: usize) -> (Vec, Vec) { + if let Some(cached) = self.word_spans.get(&display_idx) { + return cached.clone(); + } + let pair = match self.display.get(display_idx) { + Some(DisplayRow::Row(row)) if row.is_word_diff_pair() => Some((row.old, row.new)), + _ => None, + }; + match pair { + Some((Row::Line(o), Row::Line(n))) => { + let spans = word_diff_spans(self.old_line(o), self.new_line(n)); + self.word_spans.insert(display_idx, spans.clone()); + spans + } + _ => (Vec::new(), Vec::new()), + } + } + + /// Read-only peek at an already-cached word-diff span pair (empty if uncached). Used by the + /// renderer's second (immutable) pass after a first mutable pass has populated the cache + /// for the visible viewport via [`Self::word_spans_for_row`]. + pub fn peek_word_spans(&self, display_idx: usize) -> (Vec, Vec) { + self.word_spans + .get(&display_idx) + .cloned() + .unwrap_or_default() + } +} + +fn read_head_blob(repo: &Repository, tree: &git2::Tree<'_>, path: &str) -> String { + tree.get_path(Path::new(path)) + .and_then(|entry| entry.to_object(repo)) + .ok() + .and_then(|obj| obj.into_blob().ok()) + .map(|blob| String::from_utf8_lossy(blob.content()).into_owned()) + .unwrap_or_default() +} + +fn read_workdir_file(repo: &Repository, path: &str) -> String { + repo.workdir() + .map(|wd| wd.join(path)) + .and_then(|p| std::fs::read(p).ok()) + .map(|bytes| String::from_utf8_lossy(&bytes).into_owned()) + .unwrap_or_default() +} + +/// Review session state: the combined diff's file list, per-file lazily loaded views, and +/// navigation/scroll state. One long-lived [`TsHighlighter`] lives here (not per file) — its +/// language-config cache is keyed per-instance, so a fresh highlighter per file would rebuild +/// every grammar config on every navigation. +pub struct App { + repo: Repository, + /// The combined diff's files. git2 enumerates these in path order (verified in + /// `tests`), so "current file index" is a stable alphabetical position, not an + /// arrival/discovery order that could reshuffle under the user. + pub files: Vec, + views: Vec>, + pub current: usize, + pub scroll: usize, + pub pane_height: usize, + /// Label for the old side of the diff, shown next to a rename's `old_path` in the header. + /// M3 only reviews the combined (`HEAD` ↔ worktree) diff, so this is always `"HEAD"` today; + /// M4's committed-changeset zoom will want to set this to the changeset's actual base rev. + pub base_label: String, + highlighter: TsHighlighter, +} + +impl App { + pub fn new(repo: Repository, combined: DiffModel) -> Self { + let n = combined.files.len(); + Self { + repo, + files: combined.files, + views: (0..n).map(|_| None).collect(), + current: 0, + scroll: 0, + pane_height: 20, + base_label: "HEAD".to_string(), + highlighter: TsHighlighter::new(), + } + } + + /// Load (and cache) the [`FileView`] for `idx`, unless the file is binary — binary files + /// skip content loading entirely (no blob read, no worktree read, no highlighting): there + /// is nothing for the SBS renderer to align, so [`crate::render`] shows a placeholder + /// without ever calling this. + pub fn ensure_loaded(&mut self, idx: usize) { + let Some(file) = self.files.get(idx) else { + return; + }; + if file.is_binary { + return; + } + if self.views[idx].is_none() { + // Re-peeled per call rather than cached on `App`: HEAD can move between file loads + // (a fine risk in M3's read-only TUI) and the tree is cheap to re-peel. + let Ok(head_tree) = self.repo.head().and_then(|h| h.peel_to_tree()) else { + return; + }; + let view = FileView::load( + &self.repo, + &head_tree, + &self.files[idx], + &mut self.highlighter, + ); + self.views[idx] = Some(view); + } + } + + pub fn current_view(&mut self) -> Option<&mut FileView> { + self.ensure_loaded(self.current); + self.views.get_mut(self.current).and_then(|v| v.as_mut()) + } + + pub fn current_view_ref(&self) -> Option<&FileView> { + self.views.get(self.current).and_then(|v| v.as_ref()) + } + + /// Jump the scroll position to the current file's first hunk (or the top, for a file with + /// no hunks or that isn't loaded yet). + pub fn jump_to_first_hunk(&mut self) { + self.scroll = self + .views + .get(self.current) + .and_then(|v| v.as_ref()) + .map(|v| v.first_hunk_row) + .unwrap_or(0); + } + + /// Load the current file (if not binary) and jump to its first hunk. + pub fn open_current(&mut self) { + self.ensure_loaded(self.current); + self.jump_to_first_hunk(); + } + + pub fn next_file(&mut self) { + if self.files.is_empty() { + return; + } + self.current = (self.current + 1) % self.files.len(); + self.open_current(); + } + + pub fn prev_file(&mut self) { + if self.files.is_empty() { + return; + } + self.current = (self.current + self.files.len() - 1) % self.files.len(); + self.open_current(); + } + + fn row_count(&self) -> usize { + self.current_view_ref() + .map(|v| v.display.len()) + .unwrap_or(0) + } + + fn max_scroll(&self) -> usize { + self.row_count().saturating_sub(self.pane_height.max(1)) + } + + pub fn scroll_by(&mut self, delta: i64) { + let max = self.max_scroll(); + let cur = self.scroll as i64; + let next = (cur + delta).clamp(0, max as i64); + self.scroll = next as usize; + } + + pub fn scroll_top(&mut self) { + self.scroll = 0; + } + + pub fn scroll_bottom(&mut self) { + self.scroll = self.max_scroll(); + } +} + +/// Test-only helper for building an [`App`] straight from a fixture, shared by `app.rs`'s own +/// tests and `render.rs`'s frame tests. `App` owns its `Repository` handle, but +/// [`git_workon_fixture::fixture::Fixture::repo`] only lends a borrowed one — so this opens a +/// second, independent handle on the same workdir. +#[cfg(test)] +pub(crate) mod test_support { + use git2::Repository; + use git_workon_fixture::fixture::Fixture; + + use super::App; + use crate::acquire::diff_uncommitted; + + pub(crate) fn app_from_fixture(fixture: &Fixture) -> App { + let repo = fixture.repo().expect("fixture repo"); + let combined = diff_uncommitted(repo).expect("diff_uncommitted").combined; + let owned = Repository::open(repo.workdir().expect("fixture has a workdir")) + .expect("reopen fixture repo"); + App::new(owned, combined) + } +} + +#[cfg(test)] +mod tests { + use git_workon_fixture::prelude::*; + + use super::test_support::app_from_fixture; + use crate::model::FileStatus; + + #[test] + fn combined_files_arrive_path_sorted() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .untracked_file("z_new.txt", "hello\n") + .unstaged_file("a_tracked.txt", "one\n", "one\nCHANGED\n") + .untracked_file("m_mid.txt", "middle\n") + .build() + .unwrap(); + + let app = app_from_fixture(&fixture); + let paths: Vec<&str> = app.files.iter().map(|f| f.path.as_str()).collect(); + assert_eq!(paths, vec!["a_tracked.txt", "m_mid.txt", "z_new.txt"]); + } + + #[test] + fn ensure_loaded_reads_head_and_worktree_sources_for_modified_file() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("tracked.txt", "line1\nline2\n", "line1\nCHANGED\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.ensure_loaded(0); + let view = app.current_view_ref().unwrap(); + assert_eq!(view.old_text(), "line1\nline2\n"); + assert_eq!(view.new_text(), "line1\nCHANGED\n"); + } + + #[test] + fn ensure_loaded_leaves_added_file_old_side_empty() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .staged_file("new.txt", "hello\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + assert_eq!(app.files[0].status, FileStatus::Added); + app.ensure_loaded(0); + let view = app.current_view_ref().unwrap(); + assert_eq!(view.old_text(), ""); + assert_eq!(view.new_text(), "hello\n"); + } + + #[test] + fn ensure_loaded_leaves_deleted_file_new_side_empty() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .deleted_file("gone.txt", "bye\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + assert_eq!(app.files[0].status, FileStatus::Deleted); + app.ensure_loaded(0); + let view = app.current_view_ref().unwrap(); + assert_eq!(view.old_text(), "bye\n"); + assert_eq!(view.new_text(), ""); + } + + #[test] + fn ensure_loaded_reads_old_path_for_renamed_file() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("old_name.txt", "same content\n", "same content\n") + .build() + .unwrap(); + let repo = fixture.repo().unwrap(); + let workdir = repo.workdir().unwrap(); + std::fs::rename(workdir.join("old_name.txt"), workdir.join("new_name.txt")).unwrap(); + + let mut app = app_from_fixture(&fixture); + assert_eq!(app.files.len(), 1); + assert_eq!(app.files[0].status, FileStatus::Renamed); + assert_eq!(app.files[0].old_path.as_deref(), Some("old_name.txt")); + app.ensure_loaded(0); + let view = app.current_view_ref().unwrap(); + assert_eq!(view.old_text(), "same content\n"); + assert_eq!(view.new_text(), "same content\n"); + } + + #[test] + fn ensure_loaded_skips_binary_files() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .staged_file("bin.dat", "hello\n") + .build() + .unwrap(); + // Overwrite the worktree copy with binary content post-build (the fixture staged plain + // text) so the combined diff's content-sniffing sees NUL bytes and flags it binary. + let repo = fixture.repo().unwrap(); + std::fs::write(repo.workdir().unwrap().join("bin.dat"), [0u8, 1, 2, 0, 3]).unwrap(); + + let mut app = app_from_fixture(&fixture); + assert!(app.files[0].is_binary); + app.ensure_loaded(0); + assert!(app.current_view_ref().is_none()); + } +} diff --git a/git-workon-review/src/lib.rs b/git-workon-review/src/lib.rs index 9ef8365a..2ceba1de 100644 --- a/git-workon-review/src/lib.rs +++ b/git-workon-review/src/lib.rs @@ -14,6 +14,7 @@ pub mod acquire; pub mod align; +pub mod app; pub mod apply; pub mod error; pub mod file_ops; @@ -22,5 +23,6 @@ pub mod model; pub mod ops; pub mod queue; pub mod refresh; +pub mod render; pub mod synthesis; pub mod wordiff; diff --git a/git-workon-review/src/render.rs b/git-workon-review/src/render.rs new file mode 100644 index 00000000..027272ee --- /dev/null +++ b/git-workon-review/src/render.rs @@ -0,0 +1,522 @@ +//! Frame rendering: header, side-by-side diff body, footer. +//! +//! Ported from the `review-tui-spike` prototype's `ui.rs`, adapted to render [`App`]'s +//! gap-collapsed [`crate::align::DisplayRow`]s instead of a flat aligned-row list, and extended +//! with a full-width `Gap` row (the collapsed-context marker is the same on both sides, so it +//! spans the whole body rather than living in one pane). + +use ratatui::buffer::Buffer; +use ratatui::layout::{Constraint, Direction, Layout, Rect}; +use ratatui::style::{Color, Modifier, Style}; +use ratatui::text::{Line, Span as TSpan}; +use ratatui::widgets::Paragraph; +use ratatui::Frame; + +use crate::align::{CellKind, DisplayRow, Row}; +use crate::app::{App, FileView}; +use crate::highlight::FgSpan; +use crate::model::FileStatus; +use crate::wordiff::Span as WordSpan; + +const BG_DEL_SUBTLE: Color = Color::Rgb(60, 24, 24); +const BG_DEL_STRONG: Color = Color::Rgb(120, 40, 40); +const BG_ADD_SUBTLE: Color = Color::Rgb(20, 48, 24); +const BG_ADD_STRONG: Color = Color::Rgb(32, 100, 48); +const FG_DEFAULT: Color = Color::Gray; +const FG_DIM: Color = Color::DarkGray; +const FG_GUTTER: Color = Color::DarkGray; + +/// One resolved (bg, fg) pair for a byte range of a line. +struct Segment { + start: usize, + end: usize, + bg: Option, + fg: Color, +} + +/// Merge background-role spans and syntax fg spans into a flat list of non-overlapping +/// segments covering `[0, len)`. +fn compose_segments( + len: usize, + bg_spans: &[(usize, usize, Color)], + fg_spans: Option<&Vec>, +) -> Vec { + let mut boundaries: Vec = vec![0, len]; + for (s, e, _) in bg_spans { + boundaries.push((*s).min(len)); + boundaries.push((*e).min(len)); + } + if let Some(fgs) = fg_spans { + for span in fgs { + boundaries.push(span.start.min(len)); + boundaries.push(span.end.min(len)); + } + } + boundaries.sort_unstable(); + boundaries.dedup(); + + let mut segments = Vec::with_capacity(boundaries.len()); + for w in boundaries.windows(2) { + let (start, end) = (w[0], w[1]); + if start >= end { + continue; + } + let mid = start; + // Later-pushed bg spans are more specific (word-level strong emphasis is pushed after + // the whole-line subtle span in `build_pane_line`) and must win, so the lookup scans in + // REVERSE push order. The spike's forward `find` silently dropped word-level emphasis: + // the whole-line subtle span contains every offset, so it always matched first. + let bg = bg_spans + .iter() + .rev() + .find(|(s, e, _)| mid >= *s && mid < *e) + .map(|(_, _, c)| *c); + let fg = fg_spans + .and_then(|fgs| fgs.iter().find(|s| mid >= s.start && mid < s.end)) + .map(|s| s.color) + .unwrap_or(FG_DEFAULT); + segments.push(Segment { start, end, bg, fg }); + } + segments +} + +fn gutter_width(max_lineno: usize) -> usize { + max_lineno.to_string().len().max(3) +} + +/// Which side of the aligned pair a pane line is being built for — determines which of +/// [`FileView`]'s two parallel (text, highlight) sources to read. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Side { + Old, + New, +} + +/// Build a single rendered line for one pane at a display row's resolved [`Row`]/[`CellKind`]. +#[allow(clippy::too_many_arguments)] +fn build_pane_line( + view: &FileView, + side: Side, + row: Row, + kind: CellKind, + word_spans: &[WordSpan], + is_word_pair: bool, + subtle_bg: Color, + strong_bg: Color, + gutter_w: usize, + content_w: usize, +) -> Line<'static> { + match row { + Row::Filler => { + let pattern: String = "╱".repeat(content_w + gutter_w + 1); + Line::from(TSpan::styled(pattern, Style::default().fg(FG_DIM))) + } + Row::Line(n) => { + let text = match side { + Side::Old => view.old_line(n), + Side::New => view.new_line(n), + }; + let hl = match side { + Side::Old => view.old_hl.as_ref(), + Side::New => view.new_hl.as_ref(), + } + .and_then(|v| v.get(n - 1)); + + let gutter = format!("{n:>gutter_w$} "); + let mut spans = vec![TSpan::styled(gutter, Style::default().fg(FG_GUTTER))]; + + let mut bg_spans: Vec<(usize, usize, Color)> = Vec::new(); + match kind { + CellKind::Del | CellKind::Add => { + if is_word_pair { + bg_spans.push((0, text.len(), subtle_bg)); + for s in word_spans { + bg_spans.push((s.start, s.end, strong_bg)); + } + } else { + // Unpaired excess line: whole-line strong emphasis. + bg_spans.push((0, text.len(), strong_bg)); + } + } + CellKind::Context | CellKind::Filler => {} + } + + let segments = compose_segments(text.len(), &bg_spans, hl); + if segments.is_empty() && !text.is_empty() { + spans.push(TSpan::styled( + text.to_string(), + Style::default().fg(FG_DEFAULT), + )); + } + for seg in segments { + let mut style = Style::default().fg(seg.fg); + if let Some(bg) = seg.bg { + style = style.bg(bg); + } + spans.push(TSpan::styled(text[seg.start..seg.end].to_string(), style)); + } + Line::from(spans) + } + } +} + +/// Render one frame: header, SBS body, footer. +pub fn render(frame: &mut Frame, app: &mut App) { + let area = frame.area(); + let vlayout = Layout::default() + .direction(Direction::Vertical) + .constraints([ + Constraint::Length(1), + Constraint::Min(1), + Constraint::Length(1), + ]) + .split(area); + + let header_area = vlayout[0]; + let body_area = vlayout[1]; + let footer_area = vlayout[2]; + + render_header(frame, app, header_area); + render_footer(frame, footer_area); + render_body(frame, app, body_area); +} + +fn render_header(frame: &mut Frame, app: &App, area: Rect) { + let idx = app.current + 1; + let n = app.files.len(); + let label = match app.files.get(app.current) { + Some(f) if f.status == FileStatus::Renamed || f.status == FileStatus::Copied => { + format!( + "{} @ {} -> {}", + f.old_path.as_deref().unwrap_or(""), + app.base_label, + f.path + ) + } + Some(f) => f.path.clone(), + None => String::new(), + }; + let text = format!("[{idx}/{n}] {label}"); + frame.render_widget( + Paragraph::new(text).style(Style::default().add_modifier(Modifier::BOLD)), + area, + ); +} + +fn render_footer(frame: &mut Frame, area: Rect) { + let text = "j/k scroll Ctrl-d/u half-page g/G top/bottom ]f/[f file ]h/[h hunk q quit"; + frame.render_widget( + Paragraph::new(text).style(Style::default().fg(FG_DIM)), + area, + ); +} + +/// Write a gap row's `··· N unchanged lines ···` marker across the FULL body width (both panes +/// and the divider column) — unlike a per-pane content row, a gap hides the same span on both +/// sides, so it isn't "about" one side or the other. +fn render_gap_row(buf: &mut Buffer, area: Rect, y: u16, skipped: usize) { + let msg = format!("··· {skipped} unchanged lines ···"); + let line = Line::from(TSpan::styled(msg, Style::default().fg(FG_DIM))); + buf.set_line(area.x, y, &line, area.width); +} + +fn render_body(frame: &mut Frame, app: &mut App, area: Rect) { + if app.files.is_empty() { + frame.render_widget(Paragraph::new("(no changes)"), area); + return; + } + + let idx = app.current; + if app.files[idx].is_binary { + let msg = format!("[Binary file: {}]", app.files[idx].path); + frame.render_widget(Paragraph::new(msg).style(Style::default().fg(FG_DIM)), area); + return; + } + + app.ensure_loaded(idx); + app.pane_height = area.height as usize; + + let left_w = area.width.saturating_sub(1) / 2; + let right_w = area.width.saturating_sub(1).saturating_sub(left_w); + let hlayout = Layout::default() + .direction(Direction::Horizontal) + .constraints([ + Constraint::Length(left_w), + Constraint::Length(1), + Constraint::Length(right_w), + ]) + .split(area); + let old_area = hlayout[0]; + let div_area = hlayout[1]; + let new_area = hlayout[2]; + + let Some(view) = app.current_view_ref() else { + frame.render_widget(Paragraph::new("(failed to load file)"), old_area); + return; + }; + let old_gutter_w = gutter_width(view.old_line_count()); + let new_gutter_w = gutter_width(view.new_line_count()); + let scroll = app.scroll; + let pane_height = app.pane_height; + let end = (scroll + pane_height).min(view.display.len()); + + // Phase 1 (mutable): populate the word-span cache for visible paired rows. Phase 2 below + // re-borrows `app`/`view` immutably to build lines — kept as the same two-phase dance the + // spike used (see app.rs's `word_spans_for_row`/`peek_word_spans` split) rather than + // restructured, since `FileView` lives behind `App`'s `Vec>` and the + // borrow checker requires the cache-populating borrow to end before the line-building + // borrow begins; there's no runtime benefit to trading that compile-time proof for + // `RefCell` interior mutability here. + if let Some(view) = app.current_view() { + for row_idx in scroll..end { + if matches!(view.display.get(row_idx), Some(DisplayRow::Row(r)) if r.is_word_diff_pair()) + { + view.word_spans_for_row(row_idx); + } + } + } + + let Some(view) = app.current_view_ref() else { + return; + }; + + for y in area.y..area.y + area.height { + frame + .buffer_mut() + .set_string(div_area.x, y, "│", Style::default().fg(FG_DIM)); + } + + for (i, row_idx) in (scroll..end).enumerate() { + let y = area.y + i as u16; + match &view.display[row_idx] { + DisplayRow::Gap { skipped } => { + render_gap_row(frame.buffer_mut(), area, y, *skipped); + } + DisplayRow::Row(row) => { + let is_pair = row.is_word_diff_pair(); + let (old_words, new_words) = if is_pair { + view.peek_word_spans(row_idx) + } else { + (Vec::new(), Vec::new()) + }; + + let old_line = build_pane_line( + view, + Side::Old, + row.old, + row.old_kind, + &old_words, + is_pair, + BG_DEL_SUBTLE, + BG_DEL_STRONG, + old_gutter_w, + old_area.width as usize, + ); + let new_line = build_pane_line( + view, + Side::New, + row.new, + row.new_kind, + &new_words, + is_pair, + BG_ADD_SUBTLE, + BG_ADD_STRONG, + new_gutter_w, + new_area.width as usize, + ); + frame + .buffer_mut() + .set_line(old_area.x, y, &old_line, old_area.width); + frame + .buffer_mut() + .set_line(new_area.x, y, &new_line, new_area.width); + } + } + } +} + +#[cfg(test)] +mod tests { + use ratatui::backend::TestBackend; + use ratatui::buffer::Buffer; + use ratatui::Terminal; + + use git_workon_fixture::prelude::*; + + use super::render; + use crate::app::test_support::app_from_fixture; + use crate::app::App; + + fn render_once(app: &mut App, width: u16, height: u16) -> Buffer { + let backend = TestBackend::new(width, height); + let mut terminal = Terminal::new(backend).unwrap(); + terminal.draw(|f| render(f, app)).unwrap(); + terminal.backend().buffer().clone() + } + + fn cell_text(buf: &Buffer, x: u16, y: u16) -> &str { + buf.cell((x, y)).unwrap().symbol() + } + + fn buf_lines(buf: &Buffer) -> Vec { + (0..buf.area.height) + .map(|y| (0..buf.area.width).map(|x| cell_text(buf, x, y)).collect()) + .collect() + } + + #[test] + fn small_modified_file_shows_gap_hunk_and_word_diff() { + // 12 lines of context around a single changed word, with more than 2*CONTEXT_LINES of + // untouched lines both before and after so a gap collapses on both edges. + let old = "l1\nl2\nl3\nl4\nl5\nl6\nl7\nl8\nold word here\nl10\nl11\nl12\nl13\nl14\n"; + let new = "l1\nl2\nl3\nl4\nl5\nl6\nl7\nl8\nnew word here\nl10\nl11\nl12\nl13\nl14\n"; + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("small.txt", old, new) + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + // `open_current` jumps the viewport straight to the first hunk row (the initial scroll + // behavior CS4 requires), so the leading gap before the hunk scrolls out of view — only + // the trailing gap (after the hunk, before EOF) stays visible at the top of a + // full-height render. + app.open_current(); + let buf = render_once(&mut app, 60, 20); + + let content = buf_lines(&buf); + + assert!( + content.iter().any(|line| line.contains("unchanged lines")), + "expected a collapsed gap row, got:\n{}", + content.join("\n") + ); + assert!( + content.iter().any(|line| line.contains("old word here")), + "expected the old-side changed line, got:\n{}", + content.join("\n") + ); + assert!( + content.iter().any(|line| line.contains("new word here")), + "expected the new-side changed line, got:\n{}", + content.join("\n") + ); + + // Word-diff emphasis: the changed word ("old"/"new") on the paired row should carry a + // strong background distinct from the rest of the line's subtle background. + let changed_row_y = content + .iter() + .position(|line| line.contains("old word here")) + .expect("changed row present") as u16; + // Gutter width 3 + 1 space = column 4 is where "old" starts. + let word_cell = buf.cell((4, changed_row_y)).unwrap(); + // l10/l11/l12 are the kept-context lines immediately after the hunk (before the + // trailing gap collapses l13/l14). + let ctx_row_y = content + .iter() + .position(|line| line.contains("l10 ")) + .expect("context row present") as u16; + let ctx_cell = buf.cell((4, ctx_row_y)).unwrap(); + assert_ne!( + word_cell.style().bg, + ctx_cell.style().bg, + "expected the word-diff row to carry a background style distinct from plain context" + ); + + // The changed word ("old", bytes 0..3 → columns 4..7) must carry the STRONG emphasis + // while the unchanged remainder of the same paired line ("word here", from column 8) + // stays subtle — three distinct backgrounds: strong word, subtle line, unstyled + // context. This pins the compositor's span precedence (specific-over-whole-line); a + // first-match lookup renders the whole line subtle and only the ctx comparison above + // would still pass. + let rest_cell = buf.cell((8, changed_row_y)).unwrap(); + assert_ne!( + word_cell.style().bg, + rest_cell.style().bg, + "expected the changed word's strong bg to differ from the line's subtle bg" + ); + assert_ne!( + rest_cell.style().bg, + ctx_cell.style().bg, + "expected the paired line's subtle bg to differ from plain context" + ); + } + + #[test] + fn binary_file_shows_placeholder() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .staged_file("bin.dat", "hello\n") + .build() + .unwrap(); + let repo = fixture.repo().unwrap(); + std::fs::write(repo.workdir().unwrap().join("bin.dat"), [0u8, 1, 2, 0, 3]).unwrap(); + + let mut app = app_from_fixture(&fixture); + let buf = render_once(&mut app, 60, 10); + + let content = buf_lines(&buf); + assert!( + content + .iter() + .any(|line| line.contains("[Binary file: bin.dat]")), + "expected binary placeholder, got:\n{}", + content.join("\n") + ); + } + + #[test] + fn deleted_file_renders_one_sided() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .deleted_file("gone.txt", "line one\nline two\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.open_current(); + let buf = render_once(&mut app, 60, 10); + + let content = buf_lines(&buf); + assert!( + content.iter().any(|line| line.contains("line one")), + "expected old-side deleted content, got:\n{}", + content.join("\n") + ); + // New (right) pane has nothing to show for a wholly deleted file: every visible row is + // filler on that side. Filler renders as a repeated '╱' run — assert the right half of + // at least one changed row is filler, not "line one"/"line two" text. + let left_w = (buf.area.width.saturating_sub(1)) / 2; + let right_x = left_w + 1; + let row_with_content = content + .iter() + .position(|line| line.contains("line one")) + .expect("row with old content present"); + let right_cell = cell_text(&buf, right_x, row_with_content as u16); + assert_eq!( + right_cell, "╱", + "expected filler on the new-side pane for a deleted file" + ); + } + + #[test] + fn renamed_file_header_shows_old_path_and_base() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("old_name.txt", "same content\n", "same content\n") + .build() + .unwrap(); + let repo = fixture.repo().unwrap(); + let workdir = repo.workdir().unwrap(); + std::fs::rename(workdir.join("old_name.txt"), workdir.join("new_name.txt")).unwrap(); + + let mut app = app_from_fixture(&fixture); + let buf = render_once(&mut app, 80, 10); + + let header: String = (0..buf.area.width).map(|x| cell_text(&buf, x, 0)).collect(); + assert!( + header.contains("old_name.txt @ HEAD -> new_name.txt"), + "expected renamed header with old_path @ base -> new_path, got: {header:?}" + ); + } +} From f9dfaf6add33be63c25fa4b6c1ac0fa9dfcb4006 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Mon, 6 Jul 2026 19:16:02 -0400 Subject: [PATCH 5/7] feat(review): wire uncommitted-source TUI with event loop and nav --- Cargo.lock | 1 + Cargo.toml | 1 + git-workon-review/Cargo.toml | 1 + git-workon-review/src/app.rs | 193 +++++++++++++++++++- git-workon-review/src/main.rs | 31 +++- git-workon-review/src/tui.rs | 315 +++++++++++++++++++++++++++++++++ git-workon-review/tests/cli.rs | 20 ++- 7 files changed, 546 insertions(+), 16 deletions(-) create mode 100644 git-workon-review/src/tui.rs diff --git a/Cargo.lock b/Cargo.lock index 34027481..f1af7bba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -962,6 +962,7 @@ version = "0.1.0" dependencies = [ "assert_cmd", "clap", + "crossterm", "git-workon-fixture", "git-workon-lib", "git2", diff --git a/Cargo.toml b/Cargo.toml index f0623ed3..8f2147b3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,6 +24,7 @@ clap = { version = "4.6.1", features = [ clap-verbosity-flag = "3.0.4" clap_complete = { version = "4.6.5", features = ["unstable-dynamic"] } clap_mangen = "0.3.0" +crossterm = "0.29.0" dialoguer = { version = "0.12.0", features = ["fuzzy-select"] } env_logger = "0.11.10" git-workon-lib = { version = "0.13.2", path = "./git-workon-lib" } diff --git a/git-workon-review/Cargo.toml b/git-workon-review/Cargo.toml index 5de95e6b..3ee08fc5 100644 --- a/git-workon-review/Cargo.toml +++ b/git-workon-review/Cargo.toml @@ -33,6 +33,7 @@ vendored = ["git-workon-lib/vendored", "git2/vendored-libgit2", "git2/vendored-o [dependencies] clap.workspace = true +crossterm.workspace = true git-workon-lib.workspace = true git2.workspace = true miette.workspace = true diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 510daac2..19c8f46f 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -300,11 +300,16 @@ impl App { self.row_count().saturating_sub(self.pane_height.max(1)) } + /// Relative scroll, clamped to `[0, max_scroll()]` — except it never snaps backward past + /// the current position when that position is itself beyond `max_scroll()` (e.g. right + /// after [`Self::next_hunk_row`]/[`Self::prev_hunk_row`] jumped the hunk to the top of a + /// pane taller than the remaining display). A relative scroll past a jump-placed position + /// is a no-op in the over-scrolled direction rather than a backward leap; scrolling back the + /// other way still works normally. pub fn scroll_by(&mut self, delta: i64) { - let max = self.max_scroll(); + let max = self.max_scroll() as i64; let cur = self.scroll as i64; - let next = (cur + delta).clamp(0, max as i64); - self.scroll = next as usize; + self.scroll = (cur + delta).clamp(0, max.max(cur)) as usize; } pub fn scroll_top(&mut self) { @@ -314,6 +319,53 @@ impl App { pub fn scroll_bottom(&mut self) { self.scroll = self.max_scroll(); } + + /// Scroll to the next hunk-start row after the current scroll position (`]h`). A no-op if + /// there is no later hunk, or the current file has no loaded view. + pub fn next_hunk_row(&mut self) { + let Some(view) = self.current_view_ref() else { + return; + }; + if let Some(row) = find_next_hunk_row(&view.display, self.scroll) { + self.scroll = row; + } + } + + /// Scroll to the previous hunk-start row before the current scroll position (`[h`). A no-op + /// if there is no earlier hunk, or the current file has no loaded view. + pub fn prev_hunk_row(&mut self) { + let Some(view) = self.current_view_ref() else { + return; + }; + if let Some(row) = find_prev_hunk_row(&view.display, self.scroll) { + self.scroll = row; + } + } +} + +/// True for a display row that carries change content (Del/Add/Filler on either side) rather +/// than pure context — the unit hunk navigation jumps between. +fn is_hunk_content_row(row: &DisplayRow) -> bool { + matches!( + row, + DisplayRow::Row(r) if !(r.old_kind == CellKind::Context && r.new_kind == CellKind::Context) + ) +} + +/// Row index of the next "hunk start" strictly after `after` — a hunk start is a content row +/// whose preceding row is context/gap/absent (i.e. a transition INTO a hunk, not every changed +/// row). Returns `None` if there is no such row. +fn find_next_hunk_row(display: &[DisplayRow], after: usize) -> Option { + (after + 1..display.len()).find(|&i| { + is_hunk_content_row(&display[i]) && (i == 0 || !is_hunk_content_row(&display[i - 1])) + }) +} + +/// Row index of the previous "hunk start" strictly before `before`. See [`find_next_hunk_row`]. +fn find_prev_hunk_row(display: &[DisplayRow], before: usize) -> Option { + (0..before.min(display.len())).rev().find(|&i| { + is_hunk_content_row(&display[i]) && (i == 0 || !is_hunk_content_row(&display[i - 1])) + }) } /// Test-only helper for building an [`App`] straight from a fixture, shared by `app.rs`'s own @@ -342,6 +394,8 @@ mod tests { use git_workon_fixture::prelude::*; use super::test_support::app_from_fixture; + use super::{find_next_hunk_row, find_prev_hunk_row}; + use crate::align::{AlignedRow, CellKind, DisplayRow, Row}; use crate::model::FileStatus; #[test] @@ -444,4 +498,137 @@ mod tests { app.ensure_loaded(0); assert!(app.current_view_ref().is_none()); } + + // Hunk-nav helpers below operate purely over `DisplayRow` vectors — no fixture repo needed. + + fn ctx_row(n: usize) -> DisplayRow { + DisplayRow::Row(AlignedRow { + old: Row::Line(n), + new: Row::Line(n), + old_kind: CellKind::Context, + new_kind: CellKind::Context, + }) + } + + fn change_row(n: usize) -> DisplayRow { + DisplayRow::Row(AlignedRow { + old: Row::Line(n), + new: Row::Line(n), + old_kind: CellKind::Del, + new_kind: CellKind::Add, + }) + } + + fn gap_row(skipped: usize) -> DisplayRow { + DisplayRow::Gap { skipped } + } + + #[test] + fn find_next_hunk_row_skips_within_a_hunk_and_stops_at_the_next_start() { + // ctx, change, change (same hunk — not a new "start"), ctx, ctx, change (next hunk). + let display = vec![ + ctx_row(1), + change_row(2), + change_row(3), + ctx_row(4), + ctx_row(5), + change_row(6), + ]; + assert_eq!(find_next_hunk_row(&display, 0), Some(1)); + // From inside the first hunk, the next START is the second hunk, not row 2 itself. + assert_eq!(find_next_hunk_row(&display, 1), Some(5)); + assert_eq!(find_next_hunk_row(&display, 5), None); + } + + #[test] + fn find_prev_hunk_row_mirrors_next() { + let display = vec![ + ctx_row(1), + change_row(2), + change_row(3), + ctx_row(4), + ctx_row(5), + change_row(6), + ]; + assert_eq!(find_prev_hunk_row(&display, 6), Some(5)); + assert_eq!(find_prev_hunk_row(&display, 5), Some(1)); + assert_eq!(find_prev_hunk_row(&display, 1), None); + } + + #[test] + fn hunk_row_helpers_treat_gap_rows_as_context() { + let display = vec![change_row(1), gap_row(10), change_row(12)]; + assert_eq!(find_next_hunk_row(&display, 0), Some(2)); + assert_eq!(find_prev_hunk_row(&display, 2), Some(0)); + } + + #[test] + fn app_next_and_prev_hunk_row_scroll_between_hunks() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "many.txt", + "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\n17\n18\n19\n20\n", + "1\nCHANGED\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\n17\n18\n19\nCHANGED_TOO\n", + ) + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.open_current(); + let first_hunk_row = app.scroll; + + app.next_hunk_row(); + assert!( + app.scroll > first_hunk_row, + "should scroll to the later hunk" + ); + let second_hunk_row = app.scroll; + + // No hunk after the last one: no-op. + app.next_hunk_row(); + assert_eq!(app.scroll, second_hunk_row); + + app.prev_hunk_row(); + assert_eq!(app.scroll, first_hunk_row); + + // No hunk before the first one: no-op. + app.prev_hunk_row(); + assert_eq!(app.scroll, first_hunk_row); + } + + #[test] + fn scroll_by_does_not_snap_backward_past_a_hunk_jump() { + // A small file (fits in the default pane height) with two hunks: `max_scroll() == 0`, + // but `next_hunk_row` still jumps `scroll` to the second hunk's row unclamped, leaving + // the view over-scrolled relative to `max_scroll()`. + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "small.txt", + "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n", + "1\nCHANGED\n3\n4\n5\n6\n7\n8\n9\nCHANGED_TOO\n", + ) + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.open_current(); + assert_eq!(app.max_scroll(), 0, "whole file must fit in one pane"); + + app.next_hunk_row(); + let over_scrolled = app.scroll; + assert!( + over_scrolled > 0, + "hunk jump should place scroll past max_scroll" + ); + + // `j` (scroll_by(1)) at an over-scrolled position is a no-op, not a backward snap. + app.scroll_by(1); + assert_eq!(app.scroll, over_scrolled); + + // `k` (scroll_by(-1)) still scrolls up normally. + app.scroll_by(-1); + assert_eq!(app.scroll, over_scrolled - 1); + } } diff --git a/git-workon-review/src/main.rs b/git-workon-review/src/main.rs index d7a38318..5dd2852c 100644 --- a/git-workon-review/src/main.rs +++ b/git-workon-review/src/main.rs @@ -1,18 +1,33 @@ +mod tui; + use clap::Parser; +use git2::Repository; +use miette::{IntoDiagnostic, Result}; +use workon_review::acquire::diff_uncommitted; +use workon_review::app::App; /// A TUI for reviewing changesets #[derive(Debug, Parser)] -#[clap( - about, - author, - bin_name = env!("CARGO_PKG_NAME"), - version, - arg_required_else_help = true -)] +#[clap(about, author, bin_name = env!("CARGO_PKG_NAME"), version)] struct Cli {} -fn main() -> miette::Result<()> { +fn main() -> Result<()> { Cli::parse(); + let repo = Repository::discover(".").into_diagnostic()?; + let combined = diff_uncommitted(&repo).into_diagnostic()?.combined; + + if combined.files.is_empty() { + eprintln!("nothing to review"); + return Ok(()); + } + + // `App` owns its own `Repository` handle (see `app.rs`'s doc comment) — moved in here after + // `diff_uncommitted` is done borrowing it. + let mut app = App::new(repo, combined); + app.open_current(); + + tui::run(&mut app).into_diagnostic()?; + Ok(()) } diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs new file mode 100644 index 00000000..3619b1d8 --- /dev/null +++ b/git-workon-review/src/tui.rs @@ -0,0 +1,315 @@ +//! Terminal lifecycle, event seam, and the main input loop for the review TUI. +//! +//! Ported loop shape from the `review-tui-spike` prototype's `main.rs` (`install_panic_hook`, +//! raw-mode + alternate-screen setup, `draw -> quit-check -> next_event -> update`), adapted to +//! read events through [`next_event`] rather than calling crossterm directly from the loop: M4 +//! swaps `next_event`'s internals for an mpsc channel fed by watcher threads without changing +//! the loop shape or [`AppEvent`]'s shape. + +use std::io::{self, Stdout}; +use std::time::Duration; + +use crossterm::event::{self, Event, KeyCode, KeyEvent, KeyEventKind, KeyModifiers}; +use crossterm::execute; +use crossterm::terminal::{ + disable_raw_mode, enable_raw_mode, EnterAlternateScreen, LeaveAlternateScreen, +}; +use ratatui::backend::CrosstermBackend; +use ratatui::Terminal; +use workon_review::app::App; +use workon_review::render; + +/// One event the review loop reacts to. `next_event`'s crossterm-specific mapping is the only +/// piece M4 will replace (for an mpsc channel fed by a file-watcher thread) — the loop and this +/// enum stay the same shape. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AppEvent { + Key(KeyEvent), + Resize(u16, u16), + Tick, +} + +/// Poll for the next terminal event, up to `timeout`. +/// +/// `Ok(Some(AppEvent::Tick))` on a plain timeout (the loop's regular redraw beat); `Ok(None)` for +/// a terminal event we don't map to an [`AppEvent`] (key release/repeat, mouse, paste, focus) — +/// the loop redraws and keeps going without calling `update`. +pub fn next_event(timeout: Duration) -> io::Result> { + if !event::poll(timeout)? { + return Ok(Some(AppEvent::Tick)); + } + Ok(match event::read()? { + Event::Key(key) if key.kind == KeyEventKind::Press => Some(AppEvent::Key(key)), + Event::Resize(w, h) => Some(AppEvent::Resize(w, h)), + _ => None, + }) +} + +/// The action a mapped key requests, independent of any [`App`] — kept separate from +/// [`map_key`]'s dispatch so the mapping itself is unit-testable without building an `App`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Action { + Quit, + ScrollBy(i64), + ScrollTop, + ScrollBottom, + NextFile, + PrevFile, + NextHunk, + PrevHunk, + None, +} + +/// Map one key press to an [`Action`], given `pending` (a `]` or `[` seen on the previous call, +/// awaiting its `f`/`h` suffix) and the current pane height (for `Ctrl-d`/`Ctrl-u` half-page +/// deltas). Unrecognized suffixes drop the pending bracket rather than re-processing the key. +fn map_key(pending: &mut Option, key: KeyEvent, pane_height: usize) -> Action { + if let Some(bracket) = pending.take() { + return match (bracket, key.code) { + (']', KeyCode::Char('f')) => Action::NextFile, + ('[', KeyCode::Char('f')) => Action::PrevFile, + (']', KeyCode::Char('h')) => Action::NextHunk, + ('[', KeyCode::Char('h')) => Action::PrevHunk, + _ => Action::None, + }; + } + + match key.code { + KeyCode::Char('q') | KeyCode::Esc => Action::Quit, + KeyCode::Char('j') | KeyCode::Down => Action::ScrollBy(1), + KeyCode::Char('k') | KeyCode::Up => Action::ScrollBy(-1), + KeyCode::Char('d') if key.modifiers.contains(KeyModifiers::CONTROL) => { + Action::ScrollBy((pane_height / 2).max(1) as i64) + } + KeyCode::Char('u') if key.modifiers.contains(KeyModifiers::CONTROL) => { + Action::ScrollBy(-((pane_height / 2).max(1) as i64)) + } + KeyCode::Char('g') => Action::ScrollTop, + KeyCode::Char('G') => Action::ScrollBottom, + KeyCode::Tab => Action::NextFile, + KeyCode::BackTab => Action::PrevFile, + KeyCode::Char(']') => { + *pending = Some(']'); + Action::None + } + KeyCode::Char('[') => { + *pending = Some('['); + Action::None + } + _ => Action::None, + } +} + +/// Apply an [`Action`] to `app`. Returns `true` when the loop should exit. +fn apply_action(app: &mut App, action: Action) -> bool { + match action { + Action::Quit => return true, + Action::ScrollBy(delta) => app.scroll_by(delta), + Action::ScrollTop => app.scroll_top(), + Action::ScrollBottom => app.scroll_bottom(), + Action::NextFile => app.next_file(), + Action::PrevFile => app.prev_file(), + Action::NextHunk => app.next_hunk_row(), + Action::PrevHunk => app.prev_hunk_row(), + Action::None => {} + } + false +} + +/// Apply one [`AppEvent`] to `app`. Returns `true` when the loop should exit (q/Esc). Resize and +/// Tick are no-ops today — ratatui re-measures `body_area` every frame regardless, and Tick +/// exists for M4's periodic-refresh consumers, not M3's read-only loop. +fn update(app: &mut App, pending: &mut Option, event: AppEvent) -> bool { + match event { + AppEvent::Key(key) => apply_action(app, map_key(pending, key, app.pane_height)), + AppEvent::Resize(_, _) | AppEvent::Tick => false, + } +} + +/// Install a panic hook that restores the terminal (raw mode off, leave alternate screen) before +/// the default hook prints the panic — without this, a panic mid-review leaves the user's shell +/// in alternate-screen raw mode with no visible message. Ported from the spike's +/// `install_panic_hook`. +fn install_panic_hook() { + let default_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + let _ = disable_raw_mode(); + let _ = execute!(io::stdout(), LeaveAlternateScreen); + default_hook(info); + })); +} + +/// Run the review TUI's terminal lifecycle and main loop against `app`. Callers must have +/// already loaded the initial file (`app.open_current()`) before calling this. +pub fn run(app: &mut App) -> io::Result<()> { + install_panic_hook(); + enable_raw_mode()?; + let mut stdout = io::stdout(); + execute!(stdout, EnterAlternateScreen)?; + let backend = CrosstermBackend::new(stdout); + let mut terminal = Terminal::new(backend)?; + + let result = event_loop(&mut terminal, app); + + disable_raw_mode()?; + execute!(terminal.backend_mut(), LeaveAlternateScreen)?; + terminal.show_cursor()?; + + result +} + +fn event_loop(terminal: &mut Terminal>, app: &mut App) -> io::Result<()> { + let mut pending: Option = None; + let mut quit = false; + + loop { + terminal.draw(|f| render::render(f, app))?; + + if quit { + return Ok(()); + } + + if let Some(event) = next_event(Duration::from_millis(200))? { + quit = update(app, &mut pending, event); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn key(code: KeyCode) -> KeyEvent { + KeyEvent::new(code, KeyModifiers::NONE) + } + + fn ctrl_key(c: char) -> KeyEvent { + KeyEvent::new(KeyCode::Char(c), KeyModifiers::CONTROL) + } + + #[test] + fn quit_keys_map_to_quit() { + let mut pending = None; + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('q')), 20), + Action::Quit + ); + assert_eq!(map_key(&mut pending, key(KeyCode::Esc), 20), Action::Quit); + } + + #[test] + fn scroll_keys_map_by_one_line() { + let mut pending = None; + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('j')), 20), + Action::ScrollBy(1) + ); + assert_eq!( + map_key(&mut pending, key(KeyCode::Down), 20), + Action::ScrollBy(1) + ); + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('k')), 20), + Action::ScrollBy(-1) + ); + assert_eq!( + map_key(&mut pending, key(KeyCode::Up), 20), + Action::ScrollBy(-1) + ); + } + + #[test] + fn ctrl_d_u_scroll_by_half_the_pane_height() { + let mut pending = None; + assert_eq!( + map_key(&mut pending, ctrl_key('d'), 21), + Action::ScrollBy(10) + ); + assert_eq!( + map_key(&mut pending, ctrl_key('u'), 21), + Action::ScrollBy(-10) + ); + // A pane height of 1 still scrolls by at least one line. + assert_eq!(map_key(&mut pending, ctrl_key('d'), 1), Action::ScrollBy(1)); + } + + #[test] + fn g_and_shift_g_map_to_top_and_bottom() { + let mut pending = None; + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('g')), 20), + Action::ScrollTop + ); + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('G')), 20), + Action::ScrollBottom + ); + } + + #[test] + fn tab_and_backtab_map_to_file_nav() { + let mut pending = None; + assert_eq!( + map_key(&mut pending, key(KeyCode::Tab), 20), + Action::NextFile + ); + assert_eq!( + map_key(&mut pending, key(KeyCode::BackTab), 20), + Action::PrevFile + ); + } + + #[test] + fn bracket_f_maps_to_file_nav() { + let mut pending = None; + assert_eq!( + map_key(&mut pending, key(KeyCode::Char(']')), 20), + Action::None + ); + assert_eq!(pending, Some(']')); + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('f')), 20), + Action::NextFile + ); + assert_eq!(pending, None); + + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('[')), 20), + Action::None + ); + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('f')), 20), + Action::PrevFile + ); + } + + #[test] + fn bracket_h_maps_to_hunk_nav() { + let mut pending = None; + map_key(&mut pending, key(KeyCode::Char(']')), 20); + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('h')), 20), + Action::NextHunk + ); + + map_key(&mut pending, key(KeyCode::Char('[')), 20); + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('h')), 20), + Action::PrevHunk + ); + } + + #[test] + fn unrecognized_bracket_suffix_drops_pending_without_side_effect() { + let mut pending = None; + map_key(&mut pending, key(KeyCode::Char(']')), 20); + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('x')), 20), + Action::None + ); + assert_eq!( + pending, None, + "pending bracket must be cleared, not left dangling" + ); + } +} diff --git a/git-workon-review/tests/cli.rs b/git-workon-review/tests/cli.rs index c621cf2d..28b9c881 100644 --- a/git-workon-review/tests/cli.rs +++ b/git-workon-review/tests/cli.rs @@ -1,12 +1,22 @@ use assert_cmd::cargo_bin_cmd; -use predicates::prelude::*; +use git_workon_fixture::prelude::*; +/// Locked design decision #7 (M3 plan): a clean worktree prints "nothing to review" to stderr +/// and exits 0 without ever entering the TUI — no raw-mode/alternate-screen setup, so this stays +/// a plain `assert_cmd` invocation (no PTY needed). #[test] -fn no_args_shows_usage_and_fails() { +fn clean_worktree_prints_nothing_to_review_and_exits_success() { + let fixture = FixtureBuilder::new().build().unwrap(); + let repo = fixture.repo().unwrap(); + let workdir = repo.workdir().unwrap(); + let mut cmd = cargo_bin_cmd!("git-workon-review"); - cmd.assert() - .failure() - .stderr(predicate::str::contains("Usage")); + cmd.current_dir(workdir) + .env("NO_COLOR", "1") + .assert() + .success() + .stdout(predicate::str::is_empty()) + .stderr(predicate::str::contains("nothing to review")); } #[test] From d8c7b9a0d2e19cf3d276ebdee80faf16c10e9ff5 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Mon, 6 Jul 2026 19:41:53 -0400 Subject: [PATCH 6/7] feat(review): add inline layout with runtime toggle --- git-workon-review/src/align.rs | 214 +++++++++++++++++++++++ git-workon-review/src/app.rs | 300 +++++++++++++++++++++++++++++++- git-workon-review/src/render.rs | 287 ++++++++++++++++++++++++++---- git-workon-review/src/tui.rs | 12 ++ 4 files changed, 772 insertions(+), 41 deletions(-) diff --git a/git-workon-review/src/align.rs b/git-workon-review/src/align.rs index f6421812..931bd105 100644 --- a/git-workon-review/src/align.rs +++ b/git-workon-review/src/align.rs @@ -259,6 +259,116 @@ fn collapse_gaps_with(rows: &[AlignedRow], context: usize) -> Vec { out } +/// One row of the inline (unified, single-column) display. +/// +/// Built by [`inline_rows`] from the SAME gap-collapsed [`DisplayRow`] vector [`collapse_gaps`] +/// already produces for the side-by-side layout — inline reuses that pass unchanged rather than +/// re-running gap collapse over its own row type (context-gap detection is layout-agnostic; only +/// how the surviving rows spread onto the screen differs). Because a del/add change block +/// becomes MULTIPLE `InlineRow` entries (deletions first, then additions — there's no second +/// column to pad against, so unlike [`AlignedRow`] there is no `Filler` variant here), this +/// vector's indices are a DIFFERENT coordinate space than `display`'s: [`crate::app::FileView`] +/// keeps a separate word-span cache keyed by THIS vector's row index. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum InlineRow { + /// An unchanged line; carries both linenos since old and new agree on its content. + Context { + old: usize, + new: usize, + }, + /// A deleted line. `paired_new` is the addition it was aligned with in the SAME + /// [`AlignedRow`] (index-paired within the change block), if any — kept only so the renderer + /// can still run word-level diffing on the pair even though the two lines are no longer + /// visually adjacent. + Del { + old: usize, + paired_new: Option, + }, + /// An added line. `paired_old` mirrors [`InlineRow::Del::paired_new`]. + Add { + new: usize, + paired_old: Option, + }, + Gap { + skipped: usize, + }, +} + +impl InlineRow { + /// True when this row has an index-paired counterpart on the other side, eligible for + /// word-level diffing — the inline analog of [`AlignedRow::is_word_diff_pair`]. + pub fn is_word_diff_pair(&self) -> bool { + matches!( + self, + InlineRow::Del { + paired_new: Some(_), + .. + } | InlineRow::Add { + paired_old: Some(_), + .. + } + ) + } +} + +/// Convert a gap-collapsed side-by-side display vector into the inline layout's row vector. +/// +/// Walks maximal runs of non-context rows (a "change block": consecutive `AlignedRow`s where +/// `old_kind`/`new_kind` isn't `(Context, Context)`) and, within each run, emits every deletion +/// line first, then every addition line — matching git's own convention of listing removed lines +/// before added ones — dropping `Filler` entries entirely (inline has no second column to pad +/// against). +pub fn inline_rows(display: &[DisplayRow]) -> Vec { + let mut out = Vec::with_capacity(display.len()); + let mut run: Vec = Vec::new(); + + fn flush(run: &mut Vec, out: &mut Vec) { + for r in run.iter().filter(|r| r.old_kind == CellKind::Del) { + let Row::Line(old) = r.old else { + unreachable!("a Del row always carries a Line on its old side") + }; + let paired_new = match r.new { + Row::Line(n) if r.new_kind == CellKind::Add => Some(n), + _ => None, + }; + out.push(InlineRow::Del { old, paired_new }); + } + for r in run.iter().filter(|r| r.new_kind == CellKind::Add) { + let Row::Line(new) = r.new else { + unreachable!("an Add row always carries a Line on its new side") + }; + let paired_old = match r.old { + Row::Line(o) if r.old_kind == CellKind::Del => Some(o), + _ => None, + }; + out.push(InlineRow::Add { new, paired_old }); + } + run.clear(); + } + + for row in display { + match row { + DisplayRow::Gap { skipped } => { + flush(&mut run, &mut out); + out.push(InlineRow::Gap { skipped: *skipped }); + } + DisplayRow::Row(r) + if r.old_kind == CellKind::Context && r.new_kind == CellKind::Context => + { + flush(&mut run, &mut out); + let (Row::Line(old), Row::Line(new)) = (r.old, r.new) else { + unreachable!("a Context row always carries a Line on both sides") + }; + out.push(InlineRow::Context { old, new }); + } + DisplayRow::Row(r) => run.push(*r), + } + } + flush(&mut run, &mut out); + + out +} + #[cfg(test)] mod tests { use super::*; @@ -529,4 +639,108 @@ mod tests { other => panic!("expected gap row, got {other:?}"), } } + + #[test] + fn inline_del_run_precedes_add_run_within_a_block() { + // 3 dels / 1 add block: SBS index-pairs del[0]/add[0] and fillers the rest; inline must + // emit all 3 dels first, then the 1 add — not interleaved by pairing index. + let h = hunk( + 1, + 5, + 1, + 3, + vec![ + hl(LineKind::Context, Some(1), Some(1)), + hl(LineKind::Deletion, Some(2), None), + hl(LineKind::Deletion, Some(3), None), + hl(LineKind::Deletion, Some(4), None), + hl(LineKind::Addition, None, Some(2)), + hl(LineKind::Context, Some(5), Some(3)), + ], + ); + let aligned = align_file(&[h], 5, 3); + let display = collapse_gaps(&aligned.rows); + let inline = inline_rows(&display); + + assert_eq!( + inline, + vec![ + InlineRow::Context { old: 1, new: 1 }, + InlineRow::Del { + old: 2, + paired_new: Some(2) + }, + InlineRow::Del { + old: 3, + paired_new: None + }, + InlineRow::Del { + old: 4, + paired_new: None + }, + InlineRow::Add { + new: 2, + paired_old: Some(2) + }, + InlineRow::Context { old: 5, new: 3 }, + ] + ); + } + + #[test] + fn inline_has_no_filler_rows() { + let h = hunk( + 0, + 0, + 1, + 2, + vec![ + hl(LineKind::Addition, None, Some(1)), + hl(LineKind::Addition, None, Some(2)), + ], + ); + let aligned = align_file(&[h], 0, 2); + let display = collapse_gaps(&aligned.rows); + let inline = inline_rows(&display); + + assert_eq!( + inline, + vec![ + InlineRow::Add { + new: 1, + paired_old: None + }, + InlineRow::Add { + new: 2, + paired_old: None + }, + ] + ); + } + + #[test] + fn inline_passes_gap_rows_through_unchanged() { + let mut rows = vec![change_row( + Row::Line(1), + Row::Line(1), + CellKind::Del, + CellKind::Add, + )]; + rows.extend((2..=11).map(context_row)); + rows.push(change_row( + Row::Line(12), + Row::Line(12), + CellKind::Del, + CellKind::Add, + )); + + let display = collapse_gaps_with(&rows, 3); + let inline = inline_rows(&display); + assert!( + inline + .iter() + .any(|r| matches!(r, InlineRow::Gap { skipped: 4 })), + "expected the gap row to survive the inline conversion unchanged: {inline:?}" + ); + } } diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 19c8f46f..7d84d532 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -14,7 +14,7 @@ use std::path::Path; use git2::Repository; -use crate::align::{align_file, collapse_gaps, CellKind, DisplayRow, Row}; +use crate::align::{align_file, collapse_gaps, inline_rows, CellKind, DisplayRow, InlineRow, Row}; use crate::highlight::{FgSpan, TsHighlighter}; use crate::model::{DiffModel, FileChange, FileStatus}; use crate::wordiff::{word_diff_spans, Span}; @@ -51,6 +51,15 @@ pub struct FileView { /// Lazily computed word-diff spans, keyed by DISPLAY row index — the only coordinate the /// renderer's viewport walks once gaps are collapsed. word_spans: HashMap, Vec)>, + /// The inline layout's row list, derived from [`Self::display`] via + /// [`crate::align::inline_rows`] — see that function's doc comment for why this is a + /// separate vector rather than a re-collapse over its own row type. + pub inline: Vec, + /// Word-diff span cache for the inline layout, keyed by [`Self::inline`]'s row index — a + /// SEPARATE coordinate space from [`Self::word_spans`] (a paired del/add block becomes two + /// `InlineRow` entries at different indices instead of one `AlignedRow`), so the two caches + /// cannot share keys. + inline_word_spans: HashMap, Vec)>, } impl FileView { @@ -88,6 +97,7 @@ impl FileView { let old_hl = ts.highlight_file(old_source_path, &old_text); let new_hl = ts.highlight_file(&file.path, &new_text); + let inline = inline_rows(&display); Self { old_text, @@ -99,6 +109,8 @@ impl FileView { old_hl, new_hl, word_spans: HashMap::new(), + inline, + inline_word_spans: HashMap::new(), } } @@ -166,6 +178,44 @@ impl FileView { .cloned() .unwrap_or_default() } + + /// Inline-layout analog of [`Self::word_spans_for_row`], keyed by [`Self::inline`]'s row + /// index instead of [`Self::display`]'s. A `Del`/`Add` row with no paired counterpart (an + /// unpaired excess line) returns empty spans without populating the cache, same as the SBS + /// version. + pub fn inline_word_spans_for_row(&mut self, inline_idx: usize) -> (Vec, Vec) { + if let Some(cached) = self.inline_word_spans.get(&inline_idx) { + return cached.clone(); + } + let pair = match self.inline.get(inline_idx) { + Some(InlineRow::Del { + old, + paired_new: Some(new), + }) => Some((*old, *new)), + Some(InlineRow::Add { + new, + paired_old: Some(old), + }) => Some((*old, *new)), + _ => None, + }; + match pair { + Some((old, new)) => { + let spans = word_diff_spans(self.old_line(old), self.new_line(new)); + self.inline_word_spans.insert(inline_idx, spans.clone()); + spans + } + None => (Vec::new(), Vec::new()), + } + } + + /// Read-only peek at an already-cached inline word-diff span pair (empty if uncached). See + /// [`Self::peek_word_spans`]. + pub fn peek_inline_word_spans(&self, inline_idx: usize) -> (Vec, Vec) { + self.inline_word_spans + .get(&inline_idx) + .cloned() + .unwrap_or_default() + } } fn read_head_blob(repo: &Repository, tree: &git2::Tree<'_>, path: &str) -> String { @@ -185,6 +235,16 @@ fn read_workdir_file(repo: &Repository, path: &str) -> String { .unwrap_or_default() } +/// Which layout the renderer draws the current file's rows in — runtime-toggled via `L` +/// (prototype analog: `rl`), and persists across file navigation (neither +/// [`App::next_file`]/[`App::prev_file`] nor [`App::open_current`] touch it). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum Layout { + #[default] + Sbs, + Inline, +} + /// Review session state: the combined diff's file list, per-file lazily loaded views, and /// navigation/scroll state. One long-lived [`TsHighlighter`] lives here (not per file) — its /// language-config cache is keyed per-instance, so a fresh highlighter per file would rebuild @@ -204,6 +264,8 @@ pub struct App { /// M4's committed-changeset zoom will want to set this to the changeset's actual base rev. pub base_label: String, highlighter: TsHighlighter, + /// Current render layout; see [`Layout`]'s doc comment for the persistence contract. + pub layout: Layout, } impl App { @@ -218,6 +280,7 @@ impl App { pane_height: 20, base_label: "HEAD".to_string(), highlighter: TsHighlighter::new(), + layout: Layout::default(), } } @@ -292,7 +355,10 @@ impl App { fn row_count(&self) -> usize { self.current_view_ref() - .map(|v| v.display.len()) + .map(|v| match self.layout { + Layout::Sbs => v.display.len(), + Layout::Inline => v.inline.len(), + }) .unwrap_or(0) } @@ -321,26 +387,54 @@ impl App { } /// Scroll to the next hunk-start row after the current scroll position (`]h`). A no-op if - /// there is no later hunk, or the current file has no loaded view. + /// there is no later hunk, or the current file has no loaded view. Searches [`Self::layout`]'s + /// own row vector and coordinate space — `scroll` is an index into `display` under + /// [`Layout::Sbs`] but into `inline` under [`Layout::Inline`], and the two disagree on row + /// count/position whenever a change block has unequal del/add counts. pub fn next_hunk_row(&mut self) { let Some(view) = self.current_view_ref() else { return; }; - if let Some(row) = find_next_hunk_row(&view.display, self.scroll) { + let next = match self.layout { + Layout::Sbs => find_next_hunk_row(&view.display, self.scroll), + Layout::Inline => find_next_inline_hunk_row(&view.inline, self.scroll), + }; + if let Some(row) = next { self.scroll = row; } } /// Scroll to the previous hunk-start row before the current scroll position (`[h`). A no-op - /// if there is no earlier hunk, or the current file has no loaded view. + /// if there is no earlier hunk, or the current file has no loaded view. See + /// [`Self::next_hunk_row`] for why the search dispatches on [`Self::layout`]. pub fn prev_hunk_row(&mut self) { let Some(view) = self.current_view_ref() else { return; }; - if let Some(row) = find_prev_hunk_row(&view.display, self.scroll) { + let prev = match self.layout { + Layout::Sbs => find_prev_hunk_row(&view.display, self.scroll), + Layout::Inline => find_prev_inline_hunk_row(&view.inline, self.scroll), + }; + if let Some(row) = prev { self.scroll = row; } } + + /// Toggle between side-by-side and inline layouts (`L`). Deliberately does not try to + /// re-derive an equivalent `scroll` position for the new layout — the two layouts' row + /// vectors track the same underlying content in a different shape, and translating exactly + /// isn't worth the complexity for M3; the user re-orients same as they would after a resize. + /// It DOES clamp `scroll` to the new layout's `max_scroll()`, though: inline is strictly + /// taller than SBS whenever paired del/add blocks exist, so a scroll position picked up + /// there (including an over-scrolled hunk-jump position, see [`Self::scroll_by`]) can exceed + /// SBS's shorter range. + pub fn toggle_layout(&mut self) { + self.layout = match self.layout { + Layout::Sbs => Layout::Inline, + Layout::Inline => Layout::Sbs, + }; + self.scroll = self.scroll.min(self.max_scroll()); + } } /// True for a display row that carries change content (Del/Add/Filler on either side) rather @@ -368,6 +462,30 @@ fn find_prev_hunk_row(display: &[DisplayRow], before: usize) -> Option { }) } +/// Inline-layout analog of [`is_hunk_content_row`]: true for a `Del`/`Add` row (inline has no +/// `Filler` variant — see [`InlineRow`]'s doc comment), false for `Context`/`Gap`. +fn is_inline_hunk_content_row(row: &InlineRow) -> bool { + matches!(row, InlineRow::Del { .. } | InlineRow::Add { .. }) +} + +/// Inline-layout analog of [`find_next_hunk_row`]: row index of the next "hunk start" (a +/// `Del`/`Add` row whose predecessor is `Context`/`Gap`/absent) strictly after `after`, searching +/// [`crate::app::FileView::inline`] instead of `display`. +fn find_next_inline_hunk_row(inline: &[InlineRow], after: usize) -> Option { + (after + 1..inline.len()).find(|&i| { + is_inline_hunk_content_row(&inline[i]) + && (i == 0 || !is_inline_hunk_content_row(&inline[i - 1])) + }) +} + +/// Inline-layout analog of [`find_prev_hunk_row`]. See [`find_next_inline_hunk_row`]. +fn find_prev_inline_hunk_row(inline: &[InlineRow], before: usize) -> Option { + (0..before.min(inline.len())).rev().find(|&i| { + is_inline_hunk_content_row(&inline[i]) + && (i == 0 || !is_inline_hunk_content_row(&inline[i - 1])) + }) +} + /// Test-only helper for building an [`App`] straight from a fixture, shared by `app.rs`'s own /// tests and `render.rs`'s frame tests. `App` owns its `Repository` handle, but /// [`git_workon_fixture::fixture::Fixture::repo`] only lends a borrowed one — so this opens a @@ -395,7 +513,7 @@ mod tests { use super::test_support::app_from_fixture; use super::{find_next_hunk_row, find_prev_hunk_row}; - use crate::align::{AlignedRow, CellKind, DisplayRow, Row}; + use crate::align::{AlignedRow, CellKind, DisplayRow, InlineRow, Row}; use crate::model::FileStatus; #[test] @@ -631,4 +749,172 @@ mod tests { app.scroll_by(-1); assert_eq!(app.scroll, over_scrolled - 1); } + + #[test] + fn toggle_layout_flips_and_persists_across_file_nav() { + use super::Layout; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .untracked_file("b.txt", "hello\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + assert_eq!(app.layout, Layout::Sbs, "default layout is side-by-side"); + + app.toggle_layout(); + assert_eq!(app.layout, Layout::Inline); + + // Navigating files must not reset the layout choice. + app.next_file(); + assert_eq!( + app.layout, + Layout::Inline, + "layout must persist across next_file" + ); + app.prev_file(); + assert_eq!( + app.layout, + Layout::Inline, + "layout must persist across prev_file" + ); + + app.toggle_layout(); + assert_eq!(app.layout, Layout::Sbs, "toggling back returns to Sbs"); + } + + #[test] + fn inline_word_spans_cache_and_peek_round_trip() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("small.txt", "old word here\n", "new word here\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.ensure_loaded(0); + let view = app.current_view().unwrap(); + + // The single change block here is a 1-del/1-add pair: inline row 0 is the Del, row 1 is + // the paired Add. + assert!(view.inline[0].is_word_diff_pair()); + assert!(view.inline[1].is_word_diff_pair()); + + // Uncached before the populating call. + assert_eq!(view.peek_inline_word_spans(0), (Vec::new(), Vec::new())); + + let (old_spans, new_spans) = view.inline_word_spans_for_row(0); + assert!(!old_spans.is_empty(), "expected the changed word's span"); + + // Now cached: peek returns the same spans without recomputing. + assert_eq!(view.peek_inline_word_spans(0), (old_spans, new_spans)); + } + + #[test] + fn inline_layout_scroll_bottom_reaches_full_tail() { + use super::Layout; + + // Every line paired-changed: each display row (one Del/Add pair) expands to TWO inline + // rows (Del then Add), so `inline.len() > display.len()` — under the F1 bug, scroll + // bounds were still clamped against the shorter `display` length, leaving the inline + // tail unreachable. + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("small.txt", "1\n2\n3\n4\n5\n", "a\nb\nc\nd\ne\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.open_current(); + app.layout = Layout::Inline; + app.pane_height = 2; + + let inline_len = app.current_view_ref().unwrap().inline.len(); + let display_len = app.current_view_ref().unwrap().display.len(); + assert!( + inline_len > display_len, + "paired changed lines must expand under inline layout" + ); + + app.scroll_bottom(); + assert_eq!( + app.scroll, + inline_len - app.pane_height, + "scroll_bottom must reach the inline tail, not the shorter SBS tail" + ); + } + + #[test] + fn inline_layout_next_and_prev_hunk_row_jump_between_change_blocks() { + use super::Layout; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file( + "many.txt", + "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\n17\n18\n19\n20\n", + "1\nCHANGED\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\n17\n18\n19\nCHANGED_TOO\n", + ) + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.ensure_loaded(0); + app.layout = Layout::Inline; + app.scroll = 0; + + app.next_hunk_row(); + let first_block_row = app.scroll; + assert!( + matches!( + app.current_view_ref().unwrap().inline[first_block_row], + InlineRow::Del { .. } | InlineRow::Add { .. } + ), + "next_hunk_row must land on a Del/Add inline row" + ); + + app.next_hunk_row(); + let second_block_row = app.scroll; + assert!( + second_block_row > first_block_row, + "should jump to the later change block" + ); + assert!(matches!( + app.current_view_ref().unwrap().inline[second_block_row], + InlineRow::Del { .. } | InlineRow::Add { .. } + )); + + app.prev_hunk_row(); + assert_eq!( + app.scroll, first_block_row, + "prev_hunk_row should return to the earlier block" + ); + } + + #[test] + fn toggle_layout_clamps_out_of_range_scroll() { + use super::Layout; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("small.txt", "1\n2\n3\n4\n5\n", "a\nb\nc\nd\ne\n") + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.open_current(); + app.layout = Layout::Inline; + app.pane_height = 2; + app.scroll_bottom(); + assert!(app.scroll > 0, "inline scroll should be over the SBS max"); + + app.toggle_layout(); + assert_eq!(app.layout, Layout::Sbs, "toggling back returns to Sbs"); + assert!( + app.scroll <= app.max_scroll(), + "scroll must be clamped to the new layout's max_scroll" + ); + } } diff --git a/git-workon-review/src/render.rs b/git-workon-review/src/render.rs index 027272ee..266c46f8 100644 --- a/git-workon-review/src/render.rs +++ b/git-workon-review/src/render.rs @@ -12,8 +12,8 @@ use ratatui::text::{Line, Span as TSpan}; use ratatui::widgets::Paragraph; use ratatui::Frame; -use crate::align::{CellKind, DisplayRow, Row}; -use crate::app::{App, FileView}; +use crate::align::{CellKind, DisplayRow, InlineRow, Row}; +use crate::app::{App, FileView, Layout as AppLayout}; use crate::highlight::FgSpan; use crate::model::FileStatus; use crate::wordiff::Span as WordSpan; @@ -63,7 +63,7 @@ fn compose_segments( } let mid = start; // Later-pushed bg spans are more specific (word-level strong emphasis is pushed after - // the whole-line subtle span in `build_pane_line`) and must win, so the lookup scans in + // the whole-line subtle span in `content_spans`) and must win, so the lookup scans in // REVERSE push order. The spike's forward `find` silently dropped word-level emphasis: // the whole-line subtle span contains every offset, so it always matched first. let bg = bg_spans @@ -92,6 +92,52 @@ enum Side { New, } +/// Build the styled content spans (everything after the gutter) for one line of text, shared by +/// [`build_pane_line`] (SBS) and [`build_inline_line`] (inline) — the two differ only in how they +/// resolve `text`/`hl`/`emphasis` from a [`Row`] vs an [`InlineRow`] and in their gutter, not in +/// how a resolved line gets colored. +/// +/// `emphasis` is `Some((subtle, strong))` for a `Del`/`Add` line (whole-line subtle background, +/// plus per-`word_spans` strong background when `is_word_pair`; whole-line strong when not paired +/// — an unpaired excess line) and `None` for `Context`/`Filler` (no background emphasis at all). +fn content_spans( + text: &str, + hl: Option<&Vec>, + emphasis: Option<(Color, Color)>, + word_spans: &[WordSpan], + is_word_pair: bool, +) -> Vec> { + let mut bg_spans: Vec<(usize, usize, Color)> = Vec::new(); + if let Some((subtle_bg, strong_bg)) = emphasis { + if is_word_pair { + bg_spans.push((0, text.len(), subtle_bg)); + for s in word_spans { + bg_spans.push((s.start, s.end, strong_bg)); + } + } else { + // Unpaired excess line: whole-line strong emphasis. + bg_spans.push((0, text.len(), strong_bg)); + } + } + + let segments = compose_segments(text.len(), &bg_spans, hl); + let mut spans = Vec::with_capacity(segments.len().max(1)); + if segments.is_empty() && !text.is_empty() { + spans.push(TSpan::styled( + text.to_string(), + Style::default().fg(FG_DEFAULT), + )); + } + for seg in segments { + let mut style = Style::default().fg(seg.fg); + if let Some(bg) = seg.bg { + style = style.bg(bg); + } + spans.push(TSpan::styled(text[seg.start..seg.end].to_string(), style)); + } + spans +} + /// Build a single rendered line for one pane at a display row's resolved [`Row`]/[`CellKind`]. #[allow(clippy::too_many_arguments)] fn build_pane_line( @@ -125,36 +171,11 @@ fn build_pane_line( let gutter = format!("{n:>gutter_w$} "); let mut spans = vec![TSpan::styled(gutter, Style::default().fg(FG_GUTTER))]; - let mut bg_spans: Vec<(usize, usize, Color)> = Vec::new(); - match kind { - CellKind::Del | CellKind::Add => { - if is_word_pair { - bg_spans.push((0, text.len(), subtle_bg)); - for s in word_spans { - bg_spans.push((s.start, s.end, strong_bg)); - } - } else { - // Unpaired excess line: whole-line strong emphasis. - bg_spans.push((0, text.len(), strong_bg)); - } - } - CellKind::Context | CellKind::Filler => {} - } - - let segments = compose_segments(text.len(), &bg_spans, hl); - if segments.is_empty() && !text.is_empty() { - spans.push(TSpan::styled( - text.to_string(), - Style::default().fg(FG_DEFAULT), - )); - } - for seg in segments { - let mut style = Style::default().fg(seg.fg); - if let Some(bg) = seg.bg { - style = style.bg(bg); - } - spans.push(TSpan::styled(text[seg.start..seg.end].to_string(), style)); - } + let emphasis = match kind { + CellKind::Del | CellKind::Add => Some((subtle_bg, strong_bg)), + CellKind::Context | CellKind::Filler => None, + }; + spans.extend(content_spans(text, hl, emphasis, word_spans, is_word_pair)); Line::from(spans) } } @@ -204,7 +225,8 @@ fn render_header(frame: &mut Frame, app: &App, area: Rect) { } fn render_footer(frame: &mut Frame, area: Rect) { - let text = "j/k scroll Ctrl-d/u half-page g/G top/bottom ]f/[f file ]h/[h hunk q quit"; + let text = + "j/k scroll Ctrl-d/u half-page g/G top/bottom ]f/[f file ]h/[h hunk L layout q quit"; frame.render_widget( Paragraph::new(text).style(Style::default().fg(FG_DIM)), area, @@ -236,6 +258,13 @@ fn render_body(frame: &mut Frame, app: &mut App, area: Rect) { app.ensure_loaded(idx); app.pane_height = area.height as usize; + match app.layout { + AppLayout::Sbs => render_body_sbs(frame, app, area), + AppLayout::Inline => render_body_inline(frame, app, area), + } +} + +fn render_body_sbs(frame: &mut Frame, app: &mut App, area: Rect) { let left_w = area.width.saturating_sub(1) / 2; let right_w = area.width.saturating_sub(1).saturating_sub(left_w); let hlayout = Layout::default() @@ -335,6 +364,123 @@ fn render_body(frame: &mut Frame, app: &mut App, area: Rect) { } } +/// Right-align `n` in a field of width `w`, or blank it out (`w` spaces) when there's no lineno +/// for this side — used by the inline gutter, which always reserves both the old and new lineno +/// columns even though a `Del`/`Add` row only fills one of them. +fn gutter_field(n: Option, w: usize) -> String { + match n { + Some(n) => format!("{n:>w$}"), + None => " ".repeat(w), + } +} + +/// Build a single rendered line for the inline layout's one full-width pane at a given +/// [`InlineRow`]. Context rows show BOTH the old and new lineno (there's a real line on each +/// side to number, and showing both matches the familiar unified-diff gutter convention); `Del` +/// rows show only the old-side column, `Add` rows only the new-side column — the other column is +/// blank rather than reused for anything, so a scan down the gutter reads as two honest, +/// independent line-number tracks. +fn build_inline_line( + view: &FileView, + row: &InlineRow, + word_spans: &[WordSpan], + old_gutter_w: usize, + new_gutter_w: usize, +) -> Line<'static> { + let (old_opt, new_opt, text, hl, kind) = match *row { + InlineRow::Context { old, new } => ( + Some(old), + Some(new), + view.new_line(new), + view.new_hl.as_ref().and_then(|v| v.get(new - 1)), + CellKind::Context, + ), + InlineRow::Del { old, .. } => ( + Some(old), + None, + view.old_line(old), + view.old_hl.as_ref().and_then(|v| v.get(old - 1)), + CellKind::Del, + ), + InlineRow::Add { new, .. } => ( + None, + Some(new), + view.new_line(new), + view.new_hl.as_ref().and_then(|v| v.get(new - 1)), + CellKind::Add, + ), + InlineRow::Gap { .. } => { + unreachable!("gap rows render via render_gap_row, not build_inline_line") + } + }; + + let gutter = format!( + "{} {} ", + gutter_field(old_opt, old_gutter_w), + gutter_field(new_opt, new_gutter_w) + ); + let mut spans = vec![TSpan::styled(gutter, Style::default().fg(FG_GUTTER))]; + + let is_word_pair = row.is_word_diff_pair(); + // `kind` is always Del/Add/Context here — inline has no Filler rows. + let emphasis = match kind { + CellKind::Del => Some((BG_DEL_SUBTLE, BG_DEL_STRONG)), + CellKind::Add => Some((BG_ADD_SUBTLE, BG_ADD_STRONG)), + CellKind::Context | CellKind::Filler => None, + }; + spans.extend(content_spans(text, hl, emphasis, word_spans, is_word_pair)); + Line::from(spans) +} + +fn render_body_inline(frame: &mut Frame, app: &mut App, area: Rect) { + let Some(view) = app.current_view_ref() else { + frame.render_widget(Paragraph::new("(failed to load file)"), area); + return; + }; + let old_gutter_w = gutter_width(view.old_line_count()); + let new_gutter_w = gutter_width(view.new_line_count()); + let scroll = app.scroll; + let pane_height = app.pane_height; + let end = (scroll + pane_height).min(view.inline.len()); + + // Same two-phase mutable/immutable dance as `render_body_sbs`, over the inline coordinate + // space instead. + if let Some(view) = app.current_view() { + for row_idx in scroll..end { + if matches!(view.inline.get(row_idx), Some(r) if r.is_word_diff_pair()) { + view.inline_word_spans_for_row(row_idx); + } + } + } + + let Some(view) = app.current_view_ref() else { + return; + }; + + for (i, row_idx) in (scroll..end).enumerate() { + let y = area.y + i as u16; + match &view.inline[row_idx] { + InlineRow::Gap { skipped } => { + render_gap_row(frame.buffer_mut(), area, y, *skipped); + } + row => { + let (old_spans, new_spans) = if row.is_word_diff_pair() { + view.peek_inline_word_spans(row_idx) + } else { + (Vec::new(), Vec::new()) + }; + let word_spans: &[WordSpan] = match row { + InlineRow::Del { .. } => &old_spans, + InlineRow::Add { .. } => &new_spans, + _ => &[], + }; + let line = build_inline_line(view, row, word_spans, old_gutter_w, new_gutter_w); + frame.buffer_mut().set_line(area.x, y, &line, area.width); + } + } + } +} + #[cfg(test)] mod tests { use ratatui::backend::TestBackend; @@ -519,4 +665,77 @@ mod tests { "expected renamed header with old_path @ base -> new_path, got: {header:?}" ); } + + #[test] + fn toggling_layout_reflows_the_same_fixture_and_toggling_back_restores_sbs() { + use crate::app::Layout; + + let old = "l1\nl2\nl3\nl4\nl5\nold word here\nl7\nl8\nl9\nl10\n"; + let new = "l1\nl2\nl3\nl4\nl5\nnew word here\nl7\nl8\nl9\nl10\n"; + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("small.txt", old, new) + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.open_current(); + + // SBS: old and new side by side on the SAME row. + let sbs_buf = render_once(&mut app, 60, 20); + let sbs_content = buf_lines(&sbs_buf); + let sbs_row = sbs_content + .iter() + .position(|line| line.contains("old word here")) + .expect("SBS row pairs old and new on one row"); + assert!( + sbs_content[sbs_row].contains("new word here"), + "expected SBS to show del and add on the same row, got:\n{}", + sbs_content.join("\n") + ); + + app.toggle_layout(); + assert_eq!(app.layout, Layout::Inline); + + let inline_buf = render_once(&mut app, 60, 20); + let inline_content = buf_lines(&inline_buf); + let del_row = inline_content + .iter() + .position(|line| line.contains("old word here")) + .expect("inline shows the deleted line"); + let add_row = inline_content + .iter() + .position(|line| line.contains("new word here")) + .expect("inline shows the added line"); + assert!( + del_row < add_row, + "expected the inline del line above its paired add line, got:\n{}", + inline_content.join("\n") + ); + assert_ne!( + del_row, add_row, + "del and add must be on separate rows in inline layout" + ); + + // Toggling back re-renders SBS (single row again) rather than staying stuck in inline. + app.toggle_layout(); + assert_eq!(app.layout, Layout::Sbs); + let sbs_again = render_once(&mut app, 60, 20); + let sbs_again_content: Vec = (0..sbs_again.area.height) + .map(|y| { + (0..sbs_again.area.width) + .map(|x| cell_text(&sbs_again, x, y)) + .collect::() + }) + .collect(); + let row = sbs_again_content + .iter() + .position(|line| line.contains("old word here")) + .expect("SBS (again) row pairs old and new on one row"); + assert!( + sbs_again_content[row].contains("new word here"), + "expected toggling back to re-render SBS with del/add on one row, got:\n{}", + sbs_again_content.join("\n") + ); + } } diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index 3619b1d8..dbb3efd3 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -57,6 +57,7 @@ enum Action { PrevFile, NextHunk, PrevHunk, + ToggleLayout, None, } @@ -86,6 +87,7 @@ fn map_key(pending: &mut Option, key: KeyEvent, pane_height: usize) -> Act } KeyCode::Char('g') => Action::ScrollTop, KeyCode::Char('G') => Action::ScrollBottom, + KeyCode::Char('L') => Action::ToggleLayout, KeyCode::Tab => Action::NextFile, KeyCode::BackTab => Action::PrevFile, KeyCode::Char(']') => { @@ -111,6 +113,7 @@ fn apply_action(app: &mut App, action: Action) -> bool { Action::PrevFile => app.prev_file(), Action::NextHunk => app.next_hunk_row(), Action::PrevHunk => app.prev_hunk_row(), + Action::ToggleLayout => app.toggle_layout(), Action::None => {} } false @@ -246,6 +249,15 @@ mod tests { ); } + #[test] + fn shift_l_maps_to_toggle_layout() { + let mut pending = None; + assert_eq!( + map_key(&mut pending, key(KeyCode::Char('L')), 20), + Action::ToggleLayout + ); + } + #[test] fn tab_and_backtab_map_to_file_nav() { let mut pending = None; From df11b9134c2fdbb8097e165d34756ca8e72075bc Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Mon, 6 Jul 2026 19:49:28 -0400 Subject: [PATCH 7/7] docs(rfc): mark M3 renderer milestone done --- docs/rfc/workon-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfc/workon-review.md b/docs/rfc/workon-review.md index e4799a0b..36676e8c 100644 --- a/docs/rfc/workon-review.md +++ b/docs/rfc/workon-review.md @@ -129,7 +129,7 @@ evidence, not to the conclusion. - **M0 — workspace plumbing.** New member crate `git-workon-review` (lib+bin, clap, error model matching workspace: thiserror+miette). Toolchain bump (ratatui/tree-sitter won't meet 1.68.2; resolved: workspace-wide `rust-version = 1.88` — no crate had ever inherited the old value, so there was no lib MSRV to preserve). Lib hygiene (drop unused dialoguer/env_logger). CI: tree-sitter C builds. Release posture per [ADR-033](../adr/033-review-crate-workspace-placement.md): `publish = false` keeps the crate out of release-plz and cargo-dist entirely; release-plz wiring is deliberately deferred to the M3 flip — do NOT add a release-plz.toml entry in M0. Acceptance: `cargo build --workspace` green, empty `git-workon-review` binary runs and prints help. - **M1 — fixture extensions + lib stack capabilities (test-first).** Fixture: sqlite metadata mode (also finally exercises the lib's primary read path), index-state builders. Lib: `parentBranchRevision` read (both formats) + needs-restack; git-inference StackModel; changeset assembly API (`Vec {branch, base_ref, head_ref, title, current, needs_restack}` + uncommitted layer). Acceptance: existing lib tests green + new capabilities spec'd against fixtures in both metadata formats. - **M2 — trap corpus port.** Diff parser + patch synthesis in the review lib, the six trap items as tests, git2-vs-CLI verdict rendered (and the write-path decision recorded here). Acceptance: round-trip corpus green against real repos. — DONE (2026-07-06): corpus green on both backends; verdict recorded above. -- **M3 — renderer + uncommitted source.** Port spike modules; wire changeset → parsed diff → SBS/inline render; file nav; the uncommitted source end-to-end. Acceptance: dogfood-able read-only review of a dirty worktree. +- **M3 — renderer + uncommitted source.** Port spike modules; wire changeset → parsed diff → SBS/inline render; file nav; the uncommitted source end-to-end. Acceptance: dogfood-able read-only review of a dirty worktree. — DONE (2026-07-06): combined-zoom read-only review with SBS + inline layouts, collapsed context gaps, word-diff emphasis, tree-sitter highlighting (spike's 8 grammars; syntect deferred), file/hunk nav; dogfooded against a dirty worktree. Port note: the spike's `compose_segments` had a latent first-match span-precedence bug that silently dropped word-level emphasis — fixed here (reverse-order lookup), pinned by a three-way bg test in `render.rs`. - **M4 — staging verbs + zoom states.** Queue, hunk/file/line ops (visual-style line selection), the `_gate` zoom matrix, attributed rendering. Acceptance: prototype staging parity, index watcher stable under external writes. - **M5 — stack + ref sources, outline.** Changeset navigation, outline panel, needs-restack markers, focus semantics (open at current branch; uncommitted adjacent-after, focused when present). - **M6 — comments + integration.** Comment store + `mcp` subcommand; `$NVIM`/`$EDITOR` edit jump; git-workon external dispatch + completion delegation. Acceptance: full agent loop — review, comment, agent addresses via MCP, re-review.