diff --git a/AGENTS.md b/AGENTS.md index 97cf0700..4100dbbb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -65,7 +65,7 @@ src/ │ ├── mod.rs │ ├── comment.rs # Comment, CommentType (Note/Suggestion/Issue/Praise) │ ├── diff_types.rs # DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin -│ └── review.rs # ReviewSession, FileReview (the persisted review state) +│ └── review.rs # ReviewSession, CommitReview, FileReview (persisted review state) │ ├── input/ │ ├── mod.rs @@ -120,6 +120,7 @@ Repository-managed agent integrations: **ReviewSession** (`src/model/review.rs`): - Persisted review state with `files: HashMap` - Each `FileReview` has: `reviewed: bool`, `reviewed_hunks: BTreeSet`, `file_comments: Vec`, `line_comments: HashMap>` +- Per-commit mode stores one `CommitReview` per commit in `per_commit_reviews`; the active commit is mirrored into the flat `review_comments`/`files` fields so existing render, comment, and navigation code can operate on a single commit at a time. **ReviewStore** (`src/review_store.rs`): - Public library facade for persisted review sessions @@ -132,14 +133,14 @@ Repository-managed agent integrations: ### Data Flow -1. **Startup**: Parse CLI args (invalid `--theme` exits non-zero). With no subcommand, or with explicit `tuicr tui`, load config from `$XDG_CONFIG_HOME/tuicr/config.toml` (default `~/.config/tuicr/config.toml`, or `%APPDATA%\tuicr\config.toml` on Windows), ignore unknown config keys with startup warnings, resolve theme precedence (`--theme` > config > dark), then call `App::new()`. Theme selection first checks bundled names, then local theme files from `$XDG_CONFIG_HOME/tuicr/themes/` (default `~/.config/tuicr/themes/`, or `%APPDATA%\tuicr\themes\` on Windows). Local theme files may reference a local `.tmTheme` syntax theme. Some bat-compatible Base16 `.tmTheme` files encode ANSI palette slots as placeholders, and `src/syntax/mod.rs` translates those at render time. `App::new()` calls `detect_vcs()` (Jujutsu first, then Git, then Mercurial), using config `backend = "libgit2"` or `backend = "cli"` for Git. Normal Git repos default to libgit2; sparse checkout repos automatically use the Git CLI backend and show a startup warning when that overrides the default. It filters diff files via repo-root `.tuicrignore`, then enters commit selection mode by default. If staged/unstaged changes exist, the first selection rows are "Staged changes" and/or "Unstaged changes". The Pull Requests tab can toggle between all open PRs and forge PRs/MRs requesting the current user's review with `r`, which refetches page 1 using `gh pr list --search "review-requested:@me"` on GitHub or `glab mr list --reviewer=@me` on GitLab. With `-r/--revisions`, it opens the requested commit range directly. Config `show_file_list = false` hides the file list panel on startup (toggleable with `e`, where `leader` defaults to `;`). Config `diff_view = "side-by-side"` sets the default diff layout (toggleable with `:diff`). Config `wrap = true` enables line wrapping (toggleable with `:set wrap!`). Config `review_watch_interval_ms = 1000` controls persisted-session polling; set it to `0` to disable. +1. **Startup**: Parse CLI args (invalid `--theme` exits non-zero). With no subcommand, or with explicit `tuicr tui`, load config from `$XDG_CONFIG_HOME/tuicr/config.toml` (default `~/.config/tuicr/config.toml`, or `%APPDATA%\tuicr\config.toml` on Windows), ignore unknown config keys with startup warnings, resolve theme precedence (`--theme` > config > dark), then call `App::new()`. Theme selection first checks bundled names, then local theme files from `$XDG_CONFIG_HOME/tuicr/themes/` (default `~/.config/tuicr/themes/`, or `%APPDATA%\tuicr\themes\` on Windows). Local theme files may reference a local `.tmTheme` syntax theme. Some bat-compatible Base16 `.tmTheme` files encode ANSI palette slots as placeholders, and `src/syntax/mod.rs` translates those at render time. `App::new()` calls `detect_vcs()` (Jujutsu first, then Git, then Mercurial), using config `backend = "libgit2"` or `backend = "cli"` for Git. Normal Git repos default to libgit2; sparse checkout repos automatically use the Git CLI backend and show a startup warning when that overrides the default. It filters diff files via repo-root `.tuicrignore`, then enters commit selection mode by default. If staged/unstaged changes exist, the first selection rows are "Staged changes" and/or "Unstaged changes". The Pull Requests tab can toggle between all open PRs and forge PRs/MRs requesting the current user's review with `r`, which refetches page 1 using `gh pr list --search "review-requested:@me"` on GitHub or `glab mr list --reviewer=@me` on GitLab. With `-r/--revisions`, it opens the requested aggregate commit range directly; with `--commits -r `, it opens per-commit mode and displays one commit diff at a time, oldest first. Config `show_file_list = false` hides the file list panel on startup (toggleable with `e`, where `leader` defaults to `;`). Config `diff_view = "side-by-side"` sets the default diff layout (toggleable with `:diff`). Config `wrap = true` enables line wrapping (toggleable with `:set wrap!`). Config `review_watch_interval_ms = 1000` controls persisted-session polling; set it to `0` to disable. 2. **Render**: `ui::render()` draws the TUI based on `App` state. When rendered comments exist, the left sidebar splits vertically into file tree and comment navigator; the navigator is hidden when there are no rendered comment rows. 3. **Input**: `crossterm` events → `map_key_to_action` → match on Action in main loop -4. **Comments**: `App::save_comment()` builds an `AddCommentRequest` and calls `add_comment_to_session()` so TUI and library callers share insertion behavior. The TUI creates a persisted session file as soon as a review session becomes active, so `tuicr review add` can target it immediately. Successful comment submits autosave the session using a locked, atomic write that merges externally added comments first. -5. **Review CLI**: `tuicr review list|add|comments` exits before TUI startup, uses `ReviewStore`, and always emits JSON; `review list` includes `active: true` for currently open TUI sessions and a `kind` (`local`/`pr`) per session, and `review add --input` accepts JSON literal, `@file`, or stdin payloads. `--repo` is a *selector*: a checkout path (matches its local sessions + PR sessions for its `origin` repo) or a forge coordinate like `owner/repo` / a repo URL (matches local + PR sessions by owner/repo, parsed from each session's slug). PR sessions thus surface by naming the repo; `review list --all` dumps everything. Resolve a PR session with its emitted slug (`gh:owner/repo/pr/N`), which is self-contained and needs no `--repo`. +4. **Comments**: `App::save_comment()` builds an `AddCommentRequest` and calls `add_comment_to_session()` so TUI and library callers share insertion behavior. The TUI creates a persisted session file as soon as a review session becomes active, so `tuicr review add` can target it immediately. Successful comment submits autosave the session using a locked, atomic write that merges externally added comments first. In per-commit mode, the active flat review is synced into `ReviewSession::per_commit_reviews` before commit switches, saves, reloads, and exports. +5. **Review CLI**: `tuicr review list|add|comments` exits before TUI startup, uses `ReviewStore`, and always emits JSON; `review list` includes `active: true` for currently open TUI sessions and a `kind` (`local`/`pr`) per session, and `review add --input` accepts JSON literal, `@file`, or stdin payloads. For per-commit sessions, `review add` writes to the persisted active commit and `review comments` includes commit metadata on each emitted comment. `--repo` is a *selector*: a checkout path (matches its local sessions + PR sessions for its `origin` repo) or a forge coordinate like `owner/repo` / a repo URL (matches local + PR sessions by owner/repo, parsed from each session's slug). PR sessions thus surface by naming the repo; `review list --all` dumps everything. Resolve a PR session with its emitted slug (`gh:owner/repo/pr/N`), which is self-contained and needs no `--repo`. 6. **Persistence**: active TUI sessions, comment submit, and `:w` save the session JSON to `~/.local/share/tuicr/reviews/`; library callers use `ReviewStore`. Open TUI sessions are also recorded in `active_sessions.json` beside `index.json` with pid, slug, path, and last-seen timestamp. TUI-created empty session files are deleted on normal exit if they still contain no comments and no reviewed files. 7. **Reload diff/session**: `:e` reloads persisted comments/review state, then re-runs VCS diff loading and reapplies `.tuicrignore` filtering to refresh displayed files -8. **Export**: `:clip` (alias `:export`) calls `export_to_clipboard()`, generating markdown and copying it to the clipboard (or stdout with `--stdout` flag) +8. **Export**: `:clip` (alias `:export`) calls `export_to_clipboard()`, generating markdown and copying it to the clipboard (or stdout with `--stdout` flag). Per-commit sessions export comments grouped by commit and include commit-message context. ### Important Implementation Details @@ -270,6 +271,23 @@ Summary: {optional overall notes} 3. **[NOTE]** `src/utils.rs:15` - Why was this approach chosen? ``` +Per-commit reviews add one section per reviewed commit: + +`````markdown +## Local tuicr Comments + +### Commit abc1234 - Add token validation + +Commit message: + +````text +Add token validation +```` + +1. **[ISSUE]** `commit message:1` - Subject should be imperative +2. **[SUGGESTION]** `src/auth.rs:42` - Extract this check +````` + ### Comment Types | Type | Meaning | Action Required | @@ -284,6 +302,8 @@ Summary: {optional overall notes} Each comment is numbered and self-contained: - `{n}. **[TYPE]** \`{file}:{line}\` - {content}` (line comment) - `{n}. **[TYPE]** \`{file}\` - {content}` (file comment) +- `{n}. **[TYPE]** \`commit message:{line}\` - {content}` (per-commit commit-message comment) +- `{n}. **[TYPE]** \`commit\` - {content}` (per-commit commit-level comment) You can reference comments by number (e.g., "Regarding comment #2..."). diff --git a/README.md b/README.md index 9079aa0b..55e9f200 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,7 @@ tuicr # Pick from a commit selector tuicr tui # Same TUI, explicit subcommand tuicr -w # Uncommitted changes (skip selector) tuicr -r main..HEAD # Commit range +tuicr --commits -r main..HEAD # Review each commit in the range tuicr pr 125 # GitHub PR tuicr mr 125 # GitLab MR tuicr tui pr 125 # GitHub PR via explicit TUI subcommand @@ -129,6 +130,9 @@ Comment types: ISSUE (problems to fix), SUGGESTION (improvements), NOTE (observa 3. [NOTE] `src/auth.rs:50-55` - This block could be refactored ``` +For per-commit reviews (`--commits -r main..HEAD`), export groups local comments under +each commit and includes the commit message for message-level comments. + Paste it back to any coding agent (Claude, Codex, Cursor, etc). For an agent-driven workflow where your agent opens tuicr in a tmux split pane, see @@ -140,6 +144,7 @@ Run with `--stdout` to pipe the markdown to another process: ```bash tuicr --stdout > review.md +tuicr --commits -r main..HEAD --stdout > review.md tuicr --stdout | pbcopy ``` @@ -229,6 +234,7 @@ A first-session cheatsheet. Press `?` inside tuicr for the full reference. | `g` / `G` | Top / bottom | | `{` / `}` | Previous / next file | | `[` / `]` | Previous / next hunk | +| `(` / `)` | Previous / next active commit when the inline commit selector is visible | | `/` | Search | | `c` / `C` | Add line / file comment | | `v` / `V` | Visual mode (range comment) | diff --git a/docs/KEYBINDINGS.md b/docs/KEYBINDINGS.md index a33329d6..d56e3072 100644 --- a/docs/KEYBINDINGS.md +++ b/docs/KEYBINDINGS.md @@ -19,6 +19,7 @@ Full reference. Press `?` inside tuicr for an in-app version of this list. | `{N}{motion}` | Vim-style count prefix — repeats `j` / `k` / `h` / `l` / `{` / `}` / `[` / `]` `N` times | | `{` / `}` | Jump to previous / next file | | `[` / `]` | Jump to previous / next hunk | +| `(` / `)` | Jump to previous / next active commit when the inline commit selector is visible | | `/` | Search within diff | | `n` / `N` | Next / previous search match | | `Enter` | Expand or collapse hidden context between hunks | @@ -153,6 +154,7 @@ GitLab MRs. | `j` / `k` | Move selection | | `Space` | Toggle local commit selection | | `Enter` | Confirm local commit range, open PR, or load more PRs | +| `(` / `)` | Cycle the active commit in per-commit review mode or cycle the selected aggregate range | | `/` | Filter currently loaded PR rows locally | | `r` | In Pull Requests tab, toggle all open PRs / PRs requesting your review | | `q` / `Esc` | Quit / return | diff --git a/src/app.rs b/src/app.rs index c21cee0d..7724a83a 100644 --- a/src/app.rs +++ b/src/app.rs @@ -13,8 +13,8 @@ use crate::forge::context::{ContextProvider, ForgeContextProvider, VcsContextPro use crate::forge::selector::PullRequestsTab; use crate::forge::traits::{ForgeBackend, ForgeRepository}; use crate::model::{ - ClearScope, Comment, CommentType, DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin, - LineRange, LineSide, ReviewSession, SessionDiffSource, + ClearScope, Comment, CommentType, CommitReview, DiffFile, DiffHunk, DiffLine, FileStatus, + LineOrigin, LineRange, LineSide, ReviewSession, SessionDiffSource, }; use crate::persistence::load_latest_session_for_context; use crate::review_store::{AddCommentRequest, CommentTarget, add_comment_to_session}; @@ -577,6 +577,7 @@ pub enum DiffSource { Unstaged, StagedAndUnstaged, CommitRange(Vec), + PerCommitRange(Vec), StagedUnstagedAndCommits(Vec), /// Remote PR review. Carries identity + base/head SHAs needed for /// context expansion and status bar labels. @@ -981,9 +982,18 @@ struct StoredComment { #[derive(Debug, Clone, PartialEq, Eq)] enum StoredCommentLocation { - Review, - File { path: PathBuf }, - Line { path: PathBuf, line: u32 }, + Review { + commit_id: Option, + }, + File { + commit_id: Option, + path: PathBuf, + }, + Line { + commit_id: Option, + path: PathBuf, + line: u32, + }, } /// Pending "press again to confirm" state for the vim comment box. A first @@ -1160,6 +1170,11 @@ pub struct App { pub pending_confirm: Option, pub supports_keyboard_enhancement: bool, pub show_file_list: bool, + /// `true` when `--commits -r ` is active. The existing flat + /// session fields mirror one active commit; the full review lives in + /// `ReviewSession::per_commit_reviews`. + pub is_per_commit_mode: bool, + pub active_per_commit_id: Option, /// `true` when the session was opened via `--all-files`. Drives the /// `PRISTINE · N files` chip in the status bar and prevents that chip /// from showing in the regular `--file ` directory mode. @@ -1450,6 +1465,7 @@ enum CommentLocation { pub struct AppStartupOptions<'a> { pub revisions: Option<&'a str>, + pub per_commit: bool, pub working_tree: bool, pub path_filter: Option<&'a str>, pub file_path: Option<&'a str>, @@ -1611,6 +1627,83 @@ impl App { )?; let commit_ids = revision_range.commit_ids.to_vec(); + if options.per_commit { + let review_commits = crate::profile::time_with( + "startup.selected_commit_info", + || vcs.get_commits_info(&commit_ids), + profile_commit_result, + )?; + if review_commits.is_empty() { + return Err(TuicrError::NoChanges); + } + + let mut session = + Self::load_or_create_per_commit_range_session(&vcs_info, &commit_ids); + Self::ensure_per_commit_reviews(&mut session, &review_commits); + let active_index = Self::single_commit_selection_index( + session.commit_selection_range, + review_commits.len(), + ) + .unwrap_or(0); + let active_commit_id = review_commits[active_index].id.clone(); + let active_ids = vec![active_commit_id.clone()]; + let active_range = ResolvedRevisionRange::from_commit_ids( + &active_ids, + RevisionDiffTarget::CommitList, + ); + let diff_files = match Self::get_commit_range_diff_with_ignore( + vcs.as_ref(), + &vcs_info.root_path, + &active_range, + highlighter, + options.path_filter, + ) { + Ok(files) => files, + Err(TuicrError::NoChanges) => Vec::new(), + Err(e) => return Err(e), + }; + + Self::load_commit_review_into_flat_session(&mut session, &active_commit_id); + session.commit_selection_range = Some((active_index, active_index)); + + let mut app = Self::build( + vcs, + vcs_info, + theme, + comment_type_configs.clone(), + output_to_stdout, + diff_files, + session, + DiffSource::PerCommitRange(commit_ids), + InputMode::Normal, + Vec::new(), + options.path_filter, + options.repo_url_override.clone(), + )?; + + app.is_per_commit_mode = true; + app.active_per_commit_id = Some(active_commit_id); + app.range_diff_files = None; + app.commit_list = review_commits.clone(); + app.commit_list_cursor = active_index; + app.commit_selection_range = Some((active_index, active_index)); + app.commit_list_scroll_offset = 0; + app.visible_commit_count = review_commits.len(); + app.has_more_commit = false; + app.show_commit_selector = review_commits.len() > 1; + app.commit_diff_cache.clear(); + app.review_commits = review_commits; + app.insert_commit_message_if_single(); + app.sort_files_by_directory(true); + app.expand_all_dirs(); + app.populate_file_line_count_cache(); + app.rebuild_annotations(); + app.sync_active_per_commit_review(); + app.persisted_session_snapshot = app.session.clone(); + + return Ok(app); + } + if options.working_tree { // Combined: commit range + staged/unstaged changes let diff_files = Self::get_working_tree_with_commits_diff_with_ignore( @@ -1964,6 +2057,8 @@ impl App { pending_confirm: None, supports_keyboard_enhancement: false, show_file_list: true, + is_per_commit_mode: false, + active_per_commit_id: None, is_pristine_mode: false, is_single_file_view: false, primed_walk_next: false, @@ -2056,6 +2151,10 @@ impl App { self.session_path = Some(path.clone()); self.session_file_state = SessionFileState::from_path(&path).ok(); self.dirty = false; + self.load_active_per_commit_review(); + if self.is_per_commit_mode { + self.persisted_session_snapshot = self.session.clone(); + } } pub fn ensure_ephemeral_session_file(&mut self) -> Result> { @@ -2124,6 +2223,7 @@ impl App { } pub fn save_current_session_merging_external(&mut self) -> Result { + self.sync_active_per_commit_review(); let identity = self.session.clone(); let current = self.session.clone(); let base = self.persisted_session_snapshot.clone(); @@ -2190,6 +2290,7 @@ impl App { } pub fn reload_persisted_session_if_changed(&mut self, force: bool) -> Result { + self.sync_active_per_commit_review(); let path = match self.session_path.clone() { Some(path) => path, None => match crate::persistence::storage::session_path(&self.session) { @@ -2220,6 +2321,7 @@ impl App { ); self.persisted_session_snapshot = latest; self.session_file_state = Some(state); + self.load_active_per_commit_review(); if changed > 0 { self.rebuild_annotations(); } @@ -2234,20 +2336,57 @@ impl App { ) -> usize { let mut changed = 0; - for (path, latest_review) in &latest.files { - if !current.files.contains_key(path) { - current.files.insert(path.clone(), latest_review.clone()); - changed += latest_review.comment_count(); - continue; + if current.diff_source == SessionDiffSource::PerCommitRange { + for (commit_id, latest_review) in &latest.per_commit_reviews { + if !current.per_commit_reviews.contains_key(commit_id) { + current + .per_commit_reviews + .insert(commit_id.clone(), latest_review.clone()); + changed += latest_review.comment_count(); + continue; + } + + let Some(current_commit) = current.per_commit_reviews.get_mut(commit_id) else { + continue; + }; + let base_commit = base.per_commit_reviews.get(commit_id); + for (path, latest_file) in &latest_review.files { + if !current_commit.files.contains_key(path) { + current_commit + .files + .insert(path.clone(), latest_file.clone()); + changed += latest_file.comment_count(); + continue; + } + + let base_reviewed = base_commit + .and_then(|commit| commit.files.get(path)) + .map(|review| review.reviewed); + if let Some(current_file) = current_commit.files.get_mut(path) + && Some(current_file.reviewed) == base_reviewed + && current_file.reviewed != latest_file.reviewed + { + current_file.reviewed = latest_file.reviewed; + changed += 1; + } + } } + } else { + for (path, latest_review) in &latest.files { + if !current.files.contains_key(path) { + current.files.insert(path.clone(), latest_review.clone()); + changed += latest_review.comment_count(); + continue; + } - let base_reviewed = base.files.get(path).map(|review| review.reviewed); - if let Some(current_review) = current.files.get_mut(path) - && Some(current_review.reviewed) == base_reviewed - && current_review.reviewed != latest_review.reviewed - { - current_review.reviewed = latest_review.reviewed; - changed += 1; + let base_reviewed = base.files.get(path).map(|review| review.reviewed); + if let Some(current_review) = current.files.get_mut(path) + && Some(current_review.reviewed) == base_reviewed + && current_review.reviewed != latest_review.reviewed + { + current_review.reviewed = latest_review.reviewed; + changed += 1; + } } } @@ -2296,6 +2435,13 @@ impl App { } fn comment_count(session: &ReviewSession) -> usize { + if session.diff_source == SessionDiffSource::PerCommitRange { + return session + .per_commit_reviews + .values() + .map(|review| review.comment_count()) + .sum(); + } session.review_comments.len() + session .files @@ -2306,11 +2452,59 @@ impl App { fn collect_stored_comments(session: &ReviewSession) -> HashMap { let mut comments = HashMap::new(); + if session.diff_source == SessionDiffSource::PerCommitRange { + for (commit_id, commit_review) in &session.per_commit_reviews { + for comment in &commit_review.review_comments { + comments.insert( + comment.id.clone(), + StoredComment { + location: StoredCommentLocation::Review { + commit_id: Some(commit_id.clone()), + }, + comment: comment.clone(), + }, + ); + } + + for (path, review) in &commit_review.files { + for comment in &review.file_comments { + comments.insert( + comment.id.clone(), + StoredComment { + location: StoredCommentLocation::File { + commit_id: Some(commit_id.clone()), + path: path.clone(), + }, + comment: comment.clone(), + }, + ); + } + + for (line, line_comments) in &review.line_comments { + for comment in line_comments { + comments.insert( + comment.id.clone(), + StoredComment { + location: StoredCommentLocation::Line { + commit_id: Some(commit_id.clone()), + path: path.clone(), + line: *line, + }, + comment: comment.clone(), + }, + ); + } + } + } + } + return comments; + } + for comment in &session.review_comments { comments.insert( comment.id.clone(), StoredComment { - location: StoredCommentLocation::Review, + location: StoredCommentLocation::Review { commit_id: None }, comment: comment.clone(), }, ); @@ -2321,7 +2515,10 @@ impl App { comments.insert( comment.id.clone(), StoredComment { - location: StoredCommentLocation::File { path: path.clone() }, + location: StoredCommentLocation::File { + commit_id: None, + path: path.clone(), + }, comment: comment.clone(), }, ); @@ -2333,6 +2530,7 @@ impl App { comment.id.clone(), StoredComment { location: StoredCommentLocation::Line { + commit_id: None, path: path.clone(), line: *line, }, @@ -2349,16 +2547,42 @@ impl App { fn upsert_stored_comment(session: &mut ReviewSession, stored: StoredComment) { Self::remove_stored_comment(session, &stored.comment.id); match stored.location { - StoredCommentLocation::Review => { - session.review_comments.push(stored.comment); + StoredCommentLocation::Review { commit_id } => { + if let Some(commit_id) = commit_id { + if let Some(review) = session.per_commit_reviews.get_mut(&commit_id) { + review.review_comments.push(stored.comment); + } + } else { + session.review_comments.push(stored.comment); + } } - StoredCommentLocation::File { path } => { - if let Some(review) = session.files.get_mut(&path) { + StoredCommentLocation::File { commit_id, path } => { + if let Some(commit_id) = commit_id { + if let Some(commit) = session.per_commit_reviews.get_mut(&commit_id) + && let Some(review) = commit.files.get_mut(&path) + { + review.file_comments.push(stored.comment); + } + } else if let Some(review) = session.files.get_mut(&path) { review.file_comments.push(stored.comment); } } - StoredCommentLocation::Line { path, line } => { - if let Some(review) = session.files.get_mut(&path) { + StoredCommentLocation::Line { + commit_id, + path, + line, + } => { + if let Some(commit_id) = commit_id { + if let Some(commit) = session.per_commit_reviews.get_mut(&commit_id) + && let Some(review) = commit.files.get_mut(&path) + { + review + .line_comments + .entry(line) + .or_default() + .push(stored.comment); + } + } else if let Some(review) = session.files.get_mut(&path) { review .line_comments .entry(line) @@ -2379,6 +2603,45 @@ impl App { return true; } + for commit in session.per_commit_reviews.values_mut() { + if let Some(index) = commit + .review_comments + .iter() + .position(|comment| comment.id == id) + { + commit.review_comments.remove(index); + return true; + } + + for review in commit.files.values_mut() { + if let Some(index) = review + .file_comments + .iter() + .position(|comment| comment.id == id) + { + review.file_comments.remove(index); + return true; + } + + let mut empty_lines = Vec::new(); + for (line, comments) in &mut review.line_comments { + if let Some(index) = comments.iter().position(|comment| comment.id == id) { + comments.remove(index); + if comments.is_empty() { + empty_lines.push(*line); + } + for line in empty_lines { + review.line_comments.remove(&line); + } + return true; + } + } + for line in empty_lines { + review.line_comments.remove(&line); + } + } + } + for review in session.files.values_mut() { if let Some(index) = review .file_comments @@ -2628,6 +2891,121 @@ impl App { session } + fn load_or_create_per_commit_range_session( + vcs_info: &VcsInfo, + commit_ids: &[String], + ) -> ReviewSession { + let newest_commit_id = commit_ids.last().unwrap().clone(); + let loaded = load_latest_session_for_context( + &vcs_info.root_path, + vcs_info.branch_name.as_deref(), + &newest_commit_id, + SessionDiffSource::PerCommitRange, + Some(commit_ids), + ) + .ok() + .and_then(|found| found.map(|(_path, session)| session)); + + let mut session = loaded.unwrap_or_else(|| { + let mut s = ReviewSession::new( + vcs_info.root_path.clone(), + newest_commit_id, + vcs_info.branch_name.clone(), + SessionDiffSource::PerCommitRange, + ); + s.commit_range = Some(commit_ids.to_vec()); + s + }); + + session.diff_source = SessionDiffSource::PerCommitRange; + if session.commit_range.is_none() { + session.commit_range = Some(commit_ids.to_vec()); + session.updated_at = chrono::Utc::now(); + } + session + } + + fn commit_review_from_info(commit: &CommitInfo) -> CommitReview { + CommitReview::new( + commit.id.clone(), + commit.short_id.clone(), + commit.summary.clone(), + commit.body.clone(), + commit.author.clone(), + commit.time, + ) + } + + fn ensure_per_commit_reviews(session: &mut ReviewSession, commits: &[CommitInfo]) { + for commit in commits { + session + .per_commit_reviews + .entry(commit.id.clone()) + .and_modify(|review| { + review.short_id = commit.short_id.clone(); + review.summary = commit.summary.clone(); + review.body = commit.body.clone(); + review.author = commit.author.clone(); + review.time = commit.time; + }) + .or_insert_with(|| Self::commit_review_from_info(commit)); + } + } + + fn load_commit_review_into_flat_session(session: &mut ReviewSession, commit_id: &str) { + if let Some(review) = session.per_commit_reviews.get(commit_id).cloned() { + session.review_comments = review.review_comments; + session.files = review.files; + } else { + session.review_comments.clear(); + session.files.clear(); + } + } + + pub fn sync_active_per_commit_review(&mut self) { + if !self.is_per_commit_mode { + return; + } + let Some(commit_id) = self.active_per_commit_id.clone() else { + return; + }; + self.session.commit_selection_range = self.per_commit_active_index().map(|idx| (idx, idx)); + let fallback = self + .review_commits + .iter() + .find(|commit| commit.id == commit_id) + .map(Self::commit_review_from_info); + let entry_id = commit_id.clone(); + let review = self + .session + .per_commit_reviews + .entry(commit_id) + .or_insert_with(|| { + fallback.unwrap_or_else(|| { + CommitReview::new( + entry_id.clone(), + entry_id[..7.min(entry_id.len())].to_string(), + "(unknown commit)".to_string(), + None, + String::new(), + Utc::now(), + ) + }) + }); + review.review_comments = self.session.review_comments.clone(); + review.files = self.session.files.clone(); + } + + fn load_active_per_commit_review(&mut self) { + if !self.is_per_commit_mode { + return; + } + let Some(commit_id) = self.active_per_commit_id.clone() else { + return; + }; + Self::load_commit_review_into_flat_session(&mut self.session, &commit_id); + } + fn load_or_create_session(vcs_info: &VcsInfo, diff_source: SessionDiffSource) -> ReviewSession { let new_session = || { ReviewSession::new( @@ -2762,6 +3140,15 @@ impl App { None } + fn single_commit_selection_index(range: Option<(usize, usize)>, total: usize) -> Option { + let (start, end) = range?; + if total > 0 && start == end && end < total { + Some(start) + } else { + None + } + } + fn register_diff_files( session: &mut ReviewSession, diff_files: &[DiffFile], @@ -4079,6 +4466,13 @@ impl App { /// Reloads diff files from disk. Returns `(file_count, invalidated_count)` where /// `invalidated_count` is the number of previously reviewed files whose content changed. pub fn reload_diff_files(&mut self) -> Result<(usize, usize)> { + if self.is_per_commit_mode { + let idx = self.per_commit_active_index().unwrap_or(0); + self.commit_diff_cache.remove(&(idx, idx)); + self.select_per_commit_index(idx)?; + return Ok((self.diff_files.len(), 0)); + } + let current_path = self.current_file_path().cloned(); let prev_file_idx = self.diff_state.current_file_idx; let prev_cursor_line = self.diff_state.cursor_line; @@ -4102,6 +4496,7 @@ impl App { highlighter, self.path_filter.as_deref(), )?, + DiffSource::PerCommitRange(_) => unreachable!("handled by per-commit reload branch"), DiffSource::StagedUnstagedAndCommits(commit_ids) => { let ids = commit_ids.clone(); Self::get_working_tree_with_commits_diff_with_ignore( @@ -4362,6 +4757,7 @@ impl App { self.diff_state.current_file_idx = file_idx; } self.rebuild_annotations(); + self.sync_active_per_commit_review(); if adjust_cursor { let header_line = self.calculate_file_scroll_offset(file_idx); @@ -4444,6 +4840,7 @@ impl App { let reviewed = review.toggle_hunk_reviewed(key); self.dirty = true; self.rebuild_annotations(); + self.sync_active_per_commit_review(); self.diff_state.current_file_idx = file_idx; if let Some(tree_idx) = self.file_idx_to_tree_idx(file_idx) { self.file_list_state.select(tree_idx); @@ -6626,6 +7023,7 @@ impl App { self.dirty = true; self.set_message("Review comment deleted"); self.rebuild_annotations(); + self.sync_active_per_commit_review(); return true; } Some(CommentLocation::File { path, index }) => { @@ -6634,6 +7032,7 @@ impl App { self.dirty = true; self.set_message("Comment deleted"); self.rebuild_annotations(); + self.sync_active_per_commit_review(); return true; } } @@ -6667,6 +7066,7 @@ impl App { self.dirty = true; self.set_message(format!("Comment on line {line} deleted")); self.rebuild_annotations(); + self.sync_active_per_commit_review(); return true; } } @@ -6686,6 +7086,7 @@ impl App { self.dirty = true; self.rebuild_annotations(); + self.sync_active_per_commit_review(); let msg = match (cleared, unreviewed) { (0, n) => format!("Unreviewed {n} files"), (c, 0) => format!("Cleared {c} comments"), @@ -8049,15 +8450,26 @@ impl App { // If we have review commits, restore the inline selector state if !self.review_commits.is_empty() { self.commit_list = self.review_commits.clone(); - self.commit_selection_range = self.saved_inline_selection; - self.commit_list_cursor = 0; + self.commit_selection_range = if self.is_per_commit_mode { + let idx = self.per_commit_active_index().unwrap_or(0); + Some((idx, idx)) + } else { + self.saved_inline_selection + }; + self.commit_list_cursor = if self.is_per_commit_mode { + self.per_commit_active_index().unwrap_or(0) + } else { + 0 + }; self.commit_list_scroll_offset = 0; self.visible_commit_count = self.review_commits.len(); self.has_more_commit = false; self.saved_inline_selection = None; // Reload diff for the restored selection - if self.commit_selection_range.is_some() { + if self.is_per_commit_mode { + self.select_per_commit_index(self.commit_list_cursor)?; + } else if self.commit_selection_range.is_some() { self.reload_inline_selection()?; } return Ok(()); @@ -8991,6 +9403,106 @@ impl App { } } + pub fn cycle_per_commit_next(&mut self) -> Result<()> { + if self.review_commits.is_empty() { + return Ok(()); + } + let current = self.per_commit_active_index().unwrap_or(0); + let next = (current + 1) % self.review_commits.len(); + self.select_per_commit_index(next) + } + + pub fn cycle_per_commit_prev(&mut self) -> Result<()> { + if self.review_commits.is_empty() { + return Ok(()); + } + let current = self.per_commit_active_index().unwrap_or(0); + let prev = if current == 0 { + self.review_commits.len() - 1 + } else { + current - 1 + }; + self.select_per_commit_index(prev) + } + + fn per_commit_active_index(&self) -> Option { + let id = self.active_per_commit_id.as_ref()?; + self.review_commits + .iter() + .position(|commit| &commit.id == id) + } + + pub fn select_per_commit_index(&mut self, index: usize) -> Result<()> { + if !self.is_per_commit_mode { + return Ok(()); + } + let Some(commit) = self.review_commits.get(index).cloned() else { + return Ok(()); + }; + + self.sync_active_per_commit_review(); + self.active_per_commit_id = Some(commit.id.clone()); + self.commit_selection_range = Some((index, index)); + self.session.commit_selection_range = Some((index, index)); + self.commit_list_cursor = index; + if self.commit_list_viewport_height > 0 { + if index < self.commit_list_scroll_offset { + self.commit_list_scroll_offset = index; + } else if index >= self.commit_list_scroll_offset + self.commit_list_viewport_height { + self.commit_list_scroll_offset = + index.saturating_sub(self.commit_list_viewport_height - 1); + } + } + + self.session + .per_commit_reviews + .entry(commit.id.clone()) + .or_insert_with(|| Self::commit_review_from_info(&commit)); + Self::load_commit_review_into_flat_session(&mut self.session, &commit.id); + + let diff_files = if let Some(cached) = self.commit_diff_cache.get(&(index, index)) { + cached.clone() + } else { + let ids = vec![commit.id.clone()]; + let range = + ResolvedRevisionRange::from_commit_ids(&ids, RevisionDiffTarget::CommitList); + let highlighter = self.theme.syntax_highlighter(); + let files = match Self::get_commit_range_diff_with_ignore( + self.vcs.as_ref(), + &self.vcs_info.root_path, + &range, + highlighter, + self.path_filter.as_deref(), + ) { + Ok(files) => files, + Err(TuicrError::NoChanges) => Vec::new(), + Err(e) => return Err(e), + }; + self.commit_diff_cache.insert((index, index), files.clone()); + files + }; + + self.diff_files = diff_files; + for file in &self.diff_files { + self.session.add_diff_file(file); + } + + let wrap = self.diff_state.wrap_lines; + self.diff_state = DiffState::default(); + self.diff_state.wrap_lines = wrap; + self.file_list_state = FileListState::default(); + self.expanded_top.clear(); + self.expanded_bottom.clear(); + self.insert_commit_message_if_single(); + self.sort_files_by_directory(true); + self.expand_all_dirs(); + self.populate_file_line_count_cache(); + self.rebuild_annotations(); + self.sync_active_per_commit_review(); + self.set_message(format!("Commit {}: {}", commit.short_id, commit.summary)); + Ok(()) + } + pub fn confirm_commit_selection(&mut self) -> Result<()> { let selection = match self.commit_selection_range { Some((start, end)) => format!( @@ -9588,6 +10100,11 @@ impl App { commits.last().map(|s| s.as_str()) } } + DiffSource::PerCommitRange(_) => self.active_per_commit_id.as_deref().or_else(|| { + self.per_commit_active_index() + .and_then(|idx| self.review_commits.get(idx)) + .map(|commit| commit.id.as_str()) + }), _ => None, } } @@ -9633,6 +10150,7 @@ impl App { | DiffSource::StagedUnstagedAndCommits(_) | DiffSource::CommitRange(_) | DiffSource::PullRequest(_) + | DiffSource::PerCommitRange(_) ) } @@ -15991,6 +16509,99 @@ mod single_file_view_tests { .expect("build app") } + fn commit_info(id: &str, short_id: &str, summary: &str) -> CommitInfo { + CommitInfo { + id: id.to_string(), + short_id: short_id.to_string(), + branch_name: None, + summary: summary.to_string(), + body: None, + author: "reviewer@example.com".to_string(), + time: Utc::now(), + } + } + + #[test] + fn per_commit_switching_preserves_comments_per_commit() { + let file_a = file("a.rs", vec![hunk(1, 1)]); + let file_b = file("b.rs", vec![hunk(10, 1)]); + let id_a = "aaa111122223333444455556666777788889999".to_string(); + let id_b = "bbb111122223333444455556666777788889999".to_string(); + let commit_a = commit_info(&id_a, "aaa1111", "Prepare API"); + let commit_b = commit_info(&id_b, "bbb1111", "Wire API"); + + let mut app = app_with(vec![file_a.clone()]); + app.is_per_commit_mode = true; + app.active_per_commit_id = Some(id_a.clone()); + app.diff_source = DiffSource::PerCommitRange(vec![id_a.clone(), id_b.clone()]); + app.review_commits = vec![commit_a, commit_b]; + app.commit_list = app.review_commits.clone(); + app.visible_commit_count = app.review_commits.len(); + app.show_commit_selector = true; + app.commit_selection_range = Some((0, 0)); + app.session.diff_source = SessionDiffSource::PerCommitRange; + app.session.commit_range = Some(vec![id_a.clone(), id_b.clone()]); + app.commit_diff_cache.insert((0, 0), vec![file_a]); + app.commit_diff_cache.insert((1, 1), vec![file_b]); + + app.session.review_comments.push(Comment::new( + "Keep this with the first commit".to_string(), + CommentType::Issue, + None, + )); + + app.select_per_commit_index(1) + .expect("select second commit"); + assert_eq!(app.active_per_commit_id.as_deref(), Some(id_b.as_str())); + assert_eq!(app.session.commit_selection_range, Some((1, 1))); + assert_eq!( + app.session + .per_commit_reviews + .get(&id_a) + .expect("first commit review") + .review_comments[0] + .content, + "Keep this with the first commit" + ); + assert!(app.session.review_comments.is_empty()); + assert!( + app.diff_files + .first() + .is_some_and(|file| file.is_commit_message) + ); + assert_eq!(app.diff_files[0].hunks[0].lines[0].content, "Wire API"); + + app.session.review_comments.push(Comment::new( + "Keep this with the second commit".to_string(), + CommentType::Suggestion, + None, + )); + + app.select_per_commit_index(0).expect("select first commit"); + assert_eq!(app.active_per_commit_id.as_deref(), Some(id_a.as_str())); + assert_eq!(app.session.commit_selection_range, Some((0, 0))); + assert_eq!(app.session.review_comments.len(), 1); + assert_eq!( + app.session.review_comments[0].content, + "Keep this with the first commit" + ); + assert_eq!( + app.session + .per_commit_reviews + .get(&id_b) + .expect("second commit review") + .review_comments[0] + .content, + "Keep this with the second commit" + ); + assert!( + app.diff_files + .first() + .is_some_and(|file| file.is_commit_message) + ); + assert_eq!(app.diff_files[0].hunks[0].lines[0].content, "Prepare API"); + } + #[test] fn editor_target_uses_selected_file_list_row() { let dir = tempfile::tempdir().expect("tempdir"); diff --git a/src/cli.rs b/src/cli.rs index 82b95c45..62daaf02 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -20,6 +20,8 @@ pub struct CliArgs { pub no_update_check: bool, /// Commit/revision range to review. pub revisions: Option, + /// Review the selected revision range one commit at a time. + pub per_commit: bool, /// Skip commit selector and review uncommitted changes directly. pub working_tree: bool, /// Filter diff to a specific file or directory path. @@ -64,6 +66,15 @@ struct TuiOptions { )] revisions: Option, + /// Review the selected revision range one commit at a time. + #[arg( + long = "commits", + action = ArgAction::SetTrue, + requires = "revisions", + conflicts_with = "working_tree", + )] + per_commit: bool, + /// Color theme to use. Bundled themes resolve first; local themes are /// loaded from the config `themes/` directory. #[arg(long, value_name = "THEME", value_parser = non_empty_theme_name)] @@ -283,6 +294,7 @@ impl From for CliArgs { output_to_stdout: options.stdout, no_update_check: options.no_update_check, revisions: options.revisions, + per_commit: options.per_commit, working_tree: options.working_tree, path_filter: options.path_filter, file_path: options.file_path, @@ -301,6 +313,7 @@ impl TuiOptions { || self.stdout || self.no_update_check || self.revisions.is_some() + || self.per_commit || self.working_tree || self.path_filter.is_some() || self.file_path.is_some() @@ -315,6 +328,7 @@ impl TuiOptions { stdout: self.stdout || later.stdout, no_update_check: self.no_update_check || later.no_update_check, revisions: later.revisions.or(self.revisions), + per_commit: self.per_commit || later.per_commit, working_tree: self.working_tree || later.working_tree, path_filter: later.path_filter.or(self.path_filter), file_path: later.file_path.or(self.file_path), @@ -334,7 +348,21 @@ impl Cli { "TUI options cannot be used with `tuicr review`; run `tuicr review --help` for review CLI options", )); } - Ok(self.into()) + + let args: CliArgs = self.into(); + if args.per_commit && args.pr_target.is_some() { + return Err(clap::Error::raw( + clap::error::ErrorKind::ArgumentConflict, + "`--commits` is only supported for local `-r/--revisions` review, not PR mode", + )); + } + if args.per_commit && args.working_tree { + return Err(clap::Error::raw( + clap::error::ErrorKind::ArgumentConflict, + "`--commits` cannot be combined with `--working-tree`", + )); + } + Ok(args) } } @@ -586,6 +614,51 @@ mod tests { assert_eq!(parsed.revisions, Some("HEAD~3..".to_string())); } + #[test] + fn should_parse_per_commit_review_mode_with_revisions() { + let parsed = parse_for_test(&["tuicr", "--commits", "-r", "main..HEAD"]) + .expect("parse should succeed"); + assert!(parsed.per_commit); + assert_eq!(parsed.revisions, Some("main..HEAD".to_string())); + } + + #[test] + fn should_parse_explicit_tui_per_commit_review_mode_with_revisions() { + let parsed = parse_for_test(&["tuicr", "tui", "--commits", "-r", "main..HEAD"]) + .expect("parse should succeed"); + assert!(parsed.per_commit); + assert_eq!(parsed.revisions, Some("main..HEAD".to_string())); + } + + #[test] + fn should_require_revisions_for_per_commit_review_mode() { + let err = parse_for_test(&["tuicr", "--commits"]).expect_err("parse should fail"); + assert_eq!(err.kind(), ErrorKind::MissingRequiredArgument); + } + + #[test] + fn should_reject_per_commit_review_mode_with_working_tree() { + for args in [ + ["tuicr", "--commits", "-w", "-r", "main..HEAD"].as_slice(), + ["tuicr", "--commits", "-r", "main..HEAD", "tui", "-w"].as_slice(), + ] { + let err = parse_for_test(args).expect_err("parse should fail"); + assert_eq!(err.kind(), ErrorKind::ArgumentConflict); + } + } + + #[test] + fn should_reject_per_commit_review_mode_with_pr_mode() { + for args in [ + ["tuicr", "--commits", "-r", "main..HEAD", "pr", "12"].as_slice(), + ["tuicr", "pr", "12", "--commits", "-r", "main..HEAD"].as_slice(), + ["tuicr", "tui", "pr", "12", "--commits", "-r", "main..HEAD"].as_slice(), + ] { + let err = parse_for_test(args).expect_err("parse should fail"); + assert_eq!(err.kind(), ErrorKind::ArgumentConflict); + } + } + #[test] fn should_reject_file_combined_with_path() { let err = parse_for_test(&["tuicr", "--file", "f.md", "--path", "src/"]) diff --git a/src/handler.rs b/src/handler.rs index ed448804..42ea0c5c 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -337,6 +337,7 @@ fn handle_left_click(app: &mut App, pos: Position) { /// Export review: either to clipboard or set pending stdout output based on app.output_to_stdout. /// When output_to_stdout is true, stores the content and sets should_quit. fn handle_export(app: &mut App) { + app.sync_active_per_commit_review(); let slug = app.session_slug(); if app.output_to_stdout { match generate_export_content( @@ -1043,6 +1044,7 @@ pub fn handle_confirm_action(app: &mut App, action: Action) { match action { Action::ConfirmYes => { if let Some(app::ConfirmAction::CopyAndQuit) = app.pending_confirm { + app.sync_active_per_commit_review(); let slug = app.session_slug(); if app.output_to_stdout { match generate_export_content( @@ -1199,6 +1201,12 @@ pub fn handle_commit_selector_action(app: &mut App, action: Action) { Action::CursorUp(_) => app.commit_select_up(), // Toggle + auto-advance so repeated presses sweep a contiguous run. Action::ToggleExpand | Action::ToggleCommitSelect | Action::SelectFile => { + if app.is_per_commit_mode { + if let Err(e) = app.select_per_commit_index(app.commit_list_cursor) { + app.set_error(format!("Failed to load diff: {e}")); + } + return; + } app.toggle_commit_selection_and_advance(); if matches!(app.diff_source, crate::app::DiffSource::PullRequest(_)) { // PR mode reloads via the forge `compare` API on a @@ -1500,14 +1508,24 @@ fn handle_shared_normal_action(app: &mut App, action: Action) { } } Action::CycleCommitNext if app.has_inline_commit_selector() => { - app.cycle_commit_next(); - if let Err(e) = app.reload_inline_selection() { + let result = if app.is_per_commit_mode { + app.cycle_per_commit_next() + } else { + app.cycle_commit_next(); + app.reload_inline_selection() + }; + if let Err(e) = result { app.set_error(format!("Failed to load diff: {e}")); } } Action::CycleCommitPrev if app.has_inline_commit_selector() => { - app.cycle_commit_prev(); - if let Err(e) = app.reload_inline_selection() { + let result = if app.is_per_commit_mode { + app.cycle_per_commit_prev() + } else { + app.cycle_commit_prev(); + app.reload_inline_selection() + }; + if let Err(e) = result { app.set_error(format!("Failed to load diff: {e}")); } } diff --git a/src/main.rs b/src/main.rs index 803a9a67..34a4c3fe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -157,6 +157,7 @@ fn main() -> anyhow::Result<()> { cli_args.output_to_stdout, AppStartupOptions { revisions: cli_args.revisions.as_deref(), + per_commit: cli_args.per_commit, working_tree: cli_args.working_tree, path_filter: cli_args.path_filter.as_deref(), file_path: cli_args.file_path.as_deref(), diff --git a/src/model/mod.rs b/src/model/mod.rs index 3f7321b9..a537acb7 100644 --- a/src/model/mod.rs +++ b/src/model/mod.rs @@ -4,4 +4,4 @@ pub mod review; pub use comment::{Comment, CommentType, LineRange, LineSide}; pub use diff_types::{DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin}; -pub use review::{ClearScope, ReviewSession, SessionDiffSource}; +pub use review::{ClearScope, CommitReview, ReviewSession, SessionDiffSource}; diff --git a/src/model/review.rs b/src/model/review.rs index 7f84ce06..304dc2f5 100644 --- a/src/model/review.rs +++ b/src/model/review.rs @@ -63,6 +63,60 @@ impl FileReview { } } +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CommitReview { + pub commit_id: String, + pub short_id: String, + pub summary: String, + #[serde(default)] + pub body: Option, + pub author: String, + pub time: DateTime, + #[serde(default)] + pub review_comments: Vec, + #[serde(default)] + pub files: HashMap, +} + +impl CommitReview { + pub fn new( + commit_id: String, + short_id: String, + summary: String, + body: Option, + author: String, + time: DateTime, + ) -> Self { + Self { + commit_id, + short_id, + summary, + body, + author, + time, + review_comments: Vec::new(), + files: HashMap::new(), + } + } + + pub fn comment_count(&self) -> usize { + self.review_comments.len() + + self + .files + .values() + .map(|review| review.comment_count()) + .sum::() + } + + pub fn reviewed_count(&self) -> usize { + self.files.values().filter(|f| f.reviewed).count() + } + + pub fn has_comments(&self) -> bool { + !self.review_comments.is_empty() || self.files.values().any(|f| f.comment_count() > 0) + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] #[derive(Default)] @@ -73,6 +127,7 @@ pub enum SessionDiffSource { Unstaged, StagedAndUnstaged, CommitRange, + PerCommitRange, WorkingTreeAndCommits, StagedUnstagedAndCommits, /// Remote pull request review. Per-PR identity lives in @@ -108,10 +163,11 @@ pub struct ReviewSession { /// unresolved threads. #[serde(default)] pub remote_comments_visibility: PrCommentsVisibility, - /// Persisted inline commit selector range for PR sessions. Indices - /// reference the per-head-SHA `pr_commits` list captured at open - /// time. `None` means "all commits" (or no selector). Older sessions - /// without this field deserialize as `None`. + /// Persisted inline commit selector range. PR sessions use indices into + /// the per-head-SHA `pr_commits` list captured at open time. Per-commit + /// local sessions store the single active commit as `(idx, idx)`. + /// `None` means "all commits" (or no selector). Older sessions without + /// this field deserialize as `None`. #[serde(default)] pub commit_selection_range: Option<(usize, usize)>, pub created_at: DateTime, @@ -119,6 +175,8 @@ pub struct ReviewSession { #[serde(default)] pub review_comments: Vec, pub files: HashMap, + #[serde(default)] + pub per_commit_reviews: HashMap, pub session_notes: Option, } @@ -145,11 +203,19 @@ impl ReviewSession { updated_at: now, review_comments: Vec::new(), files: HashMap::new(), + per_commit_reviews: HashMap::new(), session_notes: None, } } pub fn reviewed_count(&self) -> usize { + if self.diff_source == SessionDiffSource::PerCommitRange { + return self + .per_commit_reviews + .values() + .map(CommitReview::reviewed_count) + .sum(); + } self.files.values().filter(|f| f.reviewed).count() } @@ -193,6 +259,12 @@ impl ReviewSession { } pub fn has_comments(&self) -> bool { + if self.diff_source == SessionDiffSource::PerCommitRange { + return self + .per_commit_reviews + .values() + .any(CommitReview::has_comments); + } !self.review_comments.is_empty() || self.files.values().any(|f| f.comment_count() > 0) } diff --git a/src/output/markdown.rs b/src/output/markdown.rs index 19506210..5a1b90a0 100644 --- a/src/output/markdown.rs +++ b/src/output/markdown.rs @@ -10,7 +10,7 @@ use crate::error::{Result, TuicrError}; use crate::forge::remote_comments::{ PrCommentsVisibility, RemoteReviewThread, filter_threads, group_threads_by_path, }; -use crate::model::{CommentType, LineRange, LineSide, ReviewSession}; +use crate::model::{Comment, CommentType, CommitReview, LineRange, LineSide, ReviewSession}; /// (file_path, line_range, side, comment_type, content) type CommentEntry<'a> = (String, Option, Option, String, &'a str); @@ -204,6 +204,7 @@ fn review_scope_label(diff_source: &DiffSource) -> String { DiffSource::Staged => "staged changes".to_string(), DiffSource::Unstaged => "unstaged changes".to_string(), DiffSource::CommitRange(_) => "selected commit range".to_string(), + DiffSource::PerCommitRange(_) => "current commit in per-commit review".to_string(), DiffSource::StagedUnstagedAndCommits(_) => { "selected commit range + staged/unstaged changes".to_string() } @@ -267,6 +268,15 @@ fn generate_markdown( } let _ = writeln!(md); } + DiffSource::PerCommitRange(commits) => { + let short_ids: Vec<&str> = commits.iter().map(|c| &c[..7.min(c.len())]).collect(); + let _ = writeln!( + md, + "Reviewing commits individually: {}", + short_ids.join(", ") + ); + let _ = writeln!(md); + } DiffSource::StagedUnstagedAndCommits(commits) => { let short_ids: Vec<&str> = commits.iter().map(|c| &c[..7.min(c.len())]).collect(); let _ = writeln!( @@ -335,6 +345,11 @@ fn generate_markdown( let _ = writeln!(md); } + if matches!(diff_source, DiffSource::PerCommitRange(_)) { + write_per_commit_comments(&mut md, session, diff_source, comment_types); + return md; + } + // Collect all comments into a flat list let mut all_comments: Vec = Vec::new(); let review_comment_location = review_scope_label(diff_source); @@ -475,8 +490,212 @@ fn generate_markdown( md } +fn write_per_commit_comments( + md: &mut String, + session: &ReviewSession, + diff_source: &DiffSource, + comment_types: &[CommentTypeDefinition], +) { + let mut ordered_ids = match &session.commit_range { + Some(ids) => ids.clone(), + None => match diff_source { + DiffSource::PerCommitRange(ids) => ids.clone(), + _ => Vec::new(), + }, + }; + if ordered_ids.is_empty() { + ordered_ids = session.per_commit_reviews.keys().cloned().collect(); + ordered_ids.sort(); + } + + let commented: Vec<(&String, &CommitReview)> = ordered_ids + .iter() + .filter_map(|id| { + session + .per_commit_reviews + .get(id) + .map(|review| (id, review)) + }) + .filter(|(_, review)| review.has_comments()) + .collect(); + + if commented.is_empty() { + return; + } + + let _ = writeln!(md, "## Local tuicr Comments"); + let _ = writeln!(md); + + let mut next_number = 1usize; + for (_commit_id, review) in commented { + let _ = writeln!(md, "### Commit {} - {}", review.short_id, review.summary); + let _ = writeln!(md); + if !review.author.is_empty() { + let _ = writeln!(md, "Author: {}", review.author); + } + let message = commit_message_text(review); + let fence = markdown_fence_for(&message); + let _ = writeln!(md, "Commit message:"); + let _ = writeln!(md); + let _ = writeln!(md, "{fence}text"); + let _ = writeln!(md, "{message}"); + let _ = writeln!(md, "{fence}"); + let _ = writeln!(md); + + for (location, line_range, side, comment_type, content) in + collect_per_commit_entries(review, comment_types) + { + let formatted_location = format_comment_location(&location, line_range, side); + let _ = writeln!( + md, + "{}. **[{}]** {} - {}", + next_number, comment_type, formatted_location, content + ); + next_number += 1; + } + let _ = writeln!(md); + } +} + +fn collect_per_commit_entries<'a>( + review: &'a CommitReview, + comment_types: &[CommentTypeDefinition], +) -> Vec> { + let mut entries = Vec::new(); + for comment in &review.review_comments { + entries.push(( + "commit".to_string(), + None, + None, + export_comment_type_label(&comment.comment_type, comment_types), + comment.content.as_str(), + )); + } + + let mut files: Vec<_> = review.files.iter().collect(); + files.sort_by_key(|(path, _)| { + if path.as_path() == std::path::Path::new("Commit Message") { + String::new() + } else { + path.to_string_lossy().to_string() + } + }); + + for (path, file_review) in files { + let path_str = if path.as_path() == std::path::Path::new("Commit Message") { + "commit message".to_string() + } else { + path.display().to_string() + }; + + for comment in &file_review.file_comments { + entries.push(( + path_str.clone(), + None, + None, + export_comment_type_label(&comment.comment_type, comment_types), + comment.content.as_str(), + )); + } + + let mut line_comments: Vec<_> = file_review.line_comments.iter().collect(); + line_comments.sort_by_key(|(line, _)| *line); + for (line, comments) in line_comments { + for comment in comments { + entries.push(per_commit_line_entry( + &path_str, + *line, + comment, + comment_types, + )); + } + } + } + + entries +} + +fn per_commit_line_entry<'a>( + path: &str, + line: u32, + comment: &'a Comment, + comment_types: &[CommentTypeDefinition], +) -> CommentEntry<'a> { + let line_range = comment.line_range.or_else(|| Some(LineRange::single(line))); + ( + path.to_string(), + line_range, + comment.side, + export_comment_type_label(&comment.comment_type, comment_types), + comment.content.as_str(), + ) +} + +fn format_comment_location( + file: &str, + line_range: Option, + side: Option, +) -> String { + match (line_range, side) { + (Some(range), Some(LineSide::Old)) if range.is_single() => { + format!("`{}:~{}`", file, range.start) + } + (Some(range), Some(LineSide::Old)) => { + format!("`{}:~{}-~{}`", file, range.start, range.end) + } + (Some(range), _) if range.is_single() => { + format!("`{}:{}`", file, range.start) + } + (Some(range), _) => { + format!("`{}:{}-{}`", file, range.start, range.end) + } + (None, _) => format!("`{file}`"), + } +} + +fn commit_message_text(review: &CommitReview) -> String { + match review.body.as_deref() { + Some(body) if !body.trim().is_empty() => { + format!("{}\n\n{}", review.summary, body) + } + _ => review.summary.clone(), + } +} + +fn markdown_fence_for(text: &str) -> String { + let longest = text + .split('`') + .fold((0usize, 0usize), |(best, current), segment| { + if segment.is_empty() { + (best.max(current + 1), current + 1) + } else { + (best.max(current), 0) + } + }) + .0; + "`".repeat(longest.max(3) + 1) +} + fn collect_used_comment_type_ids(session: &ReviewSession) -> HashSet { let mut ids = HashSet::new(); + if session.diff_source == crate::model::SessionDiffSource::PerCommitRange { + for review in session.per_commit_reviews.values() { + for c in &review.review_comments { + ids.insert(c.comment_type.id().to_string()); + } + for file in review.files.values() { + for c in &file.file_comments { + ids.insert(c.comment_type.id().to_string()); + } + for comments in file.line_comments.values() { + for c in comments { + ids.insert(c.comment_type.id().to_string()); + } + } + } + } + return ids; + } for c in &session.review_comments { ids.insert(c.comment_type.id().to_string()); } @@ -511,7 +730,10 @@ fn export_comment_type_label( mod tests { use super::*; use crate::app::CommentTypeDefinition; - use crate::model::{Comment, CommentType, FileStatus, LineRange, LineSide, SessionDiffSource}; + use crate::model::review::FileReview; + use crate::model::{ + Comment, CommentType, CommitReview, FileStatus, LineRange, LineSide, SessionDiffSource, + }; use std::path::PathBuf; fn comment_types() -> Vec { @@ -940,6 +1162,216 @@ mod tests { assert!(markdown.contains("Reviewing commit: abc1234")); } + #[test] + fn should_export_per_commit_comments_grouped_by_commit() { + let mut session = ReviewSession::new( + PathBuf::from("/tmp/test-repo"), + "newest".to_string(), + Some("main".to_string()), + SessionDiffSource::PerCommitRange, + ); + let oldest = "aaa111122223333444455556666777788889999".to_string(); + let newest = "bbb111122223333444455556666777788889999".to_string(); + session.commit_range = Some(vec![oldest.clone(), newest.clone()]); + + let mut first = CommitReview::new( + oldest.clone(), + "aaa1111".to_string(), + "Prepare API".to_string(), + Some("Explain the new shape.".to_string()), + "Alice".to_string(), + chrono::Utc::now(), + ); + first.review_comments.push(Comment::new( + "Commit mixes refactor and behavior".to_string(), + CommentType::Issue, + None, + )); + let mut msg_review = FileReview::new(PathBuf::from("Commit Message"), FileStatus::Added, 0); + msg_review.add_line_comment( + 1, + Comment::new( + "Subject should be imperative".to_string(), + CommentType::Suggestion, + Some(LineSide::New), + ), + ); + first + .files + .insert(PathBuf::from("Commit Message"), msg_review); + session.per_commit_reviews.insert(oldest.clone(), first); + + let mut second = CommitReview::new( + newest.clone(), + "bbb1111".to_string(), + "Wire API".to_string(), + None, + "Bob".to_string(), + chrono::Utc::now(), + ); + let mut file_review = FileReview::new(PathBuf::from("src/lib.rs"), FileStatus::Modified, 0); + file_review.add_line_comment( + 42, + Comment::new( + "Handle the empty input case".to_string(), + CommentType::Issue, + Some(LineSide::New), + ), + ); + second + .files + .insert(PathBuf::from("src/lib.rs"), file_review); + session.per_commit_reviews.insert(newest.clone(), second); + + let markdown = generate_export_content( + &session, + &DiffSource::PerCommitRange(vec![oldest, newest]), + &comment_types(), + true, + &[], + None, + ) + .unwrap(); + + let first_pos = markdown.find("### Commit aaa1111").unwrap(); + let second_pos = markdown.find("### Commit bbb1111").unwrap(); + assert!(first_pos < second_pos, "{markdown}"); + assert!(markdown.contains("1. **[ISSUE]** `commit` - Commit mixes refactor")); + assert!( + markdown + .contains("2. **[SUGGESTION]** `commit message:1` - Subject should be imperative") + ); + assert!(markdown.contains("3. **[ISSUE]** `src/lib.rs:42` - Handle the empty input case")); + } + + #[test] + fn should_skip_uncommented_commits_in_per_commit_export() { + let mut session = ReviewSession::new( + PathBuf::from("/tmp/test-repo"), + "bbb111122223333444455556666777788889999".to_string(), + Some("main".to_string()), + SessionDiffSource::PerCommitRange, + ); + let commented = "aaa111122223333444455556666777788889999".to_string(); + let quiet = "bbb111122223333444455556666777788889999".to_string(); + session.commit_range = Some(vec![commented.clone(), quiet.clone()]); + + let mut review = CommitReview::new( + commented.clone(), + "aaa1111".to_string(), + "Reviewed commit".to_string(), + None, + "Alice".to_string(), + chrono::Utc::now(), + ); + review.review_comments.push(Comment::new( + "This belongs to the reviewed commit".to_string(), + CommentType::Issue, + None, + )); + session.per_commit_reviews.insert(commented.clone(), review); + session.per_commit_reviews.insert( + quiet.clone(), + CommitReview::new( + quiet.clone(), + "bbb1111".to_string(), + "Quiet commit".to_string(), + None, + "Bob".to_string(), + chrono::Utc::now(), + ), + ); + + let markdown = generate_export_content( + &session, + &DiffSource::PerCommitRange(vec![commented, quiet]), + &comment_types(), + true, + &[], + None, + ) + .unwrap(); + + assert!(markdown.contains("### Commit aaa1111 - Reviewed commit")); + assert!(!markdown.contains("### Commit bbb1111 - Quiet commit")); + } + + #[test] + fn should_use_safe_fence_for_per_commit_messages_with_backticks() { + let mut session = ReviewSession::new( + PathBuf::from("/tmp/test-repo"), + "aaa111122223333444455556666777788889999".to_string(), + Some("main".to_string()), + SessionDiffSource::PerCommitRange, + ); + let commit_id = "aaa111122223333444455556666777788889999".to_string(); + session.commit_range = Some(vec![commit_id.clone()]); + + let mut review = CommitReview::new( + commit_id.clone(), + "aaa1111".to_string(), + "Document fence handling".to_string(), + Some("Example:\n```\ncode\n```".to_string()), + "Alice".to_string(), + chrono::Utc::now(), + ); + review.review_comments.push(Comment::new( + "Fence should remain readable".to_string(), + CommentType::Note, + None, + )); + session.per_commit_reviews.insert(commit_id.clone(), review); + + let markdown = generate_export_content( + &session, + &DiffSource::PerCommitRange(vec![commit_id]), + &comment_types(), + true, + &[], + None, + ) + .unwrap(); + + assert!( + markdown + .contains("````text\nDocument fence handling\n\nExample:\n```\ncode\n```\n````") + ); + } + + #[test] + fn should_fail_per_commit_export_when_no_commit_has_comments() { + let mut session = ReviewSession::new( + PathBuf::from("/tmp/test-repo"), + "aaa111122223333444455556666777788889999".to_string(), + Some("main".to_string()), + SessionDiffSource::PerCommitRange, + ); + let commit_id = "aaa111122223333444455556666777788889999".to_string(); + session.commit_range = Some(vec![commit_id.clone()]); + session.per_commit_reviews.insert( + commit_id.clone(), + CommitReview::new( + commit_id.clone(), + "aaa1111".to_string(), + "No comments".to_string(), + None, + "Alice".to_string(), + chrono::Utc::now(), + ), + ); + + let result = generate_export_content( + &session, + &DiffSource::PerCommitRange(vec![commit_id]), + &comment_types(), + true, + &[], + None, + ); + + assert!(matches!(result, Err(TuicrError::NoComments))); + } + #[test] fn should_write_osc52_escape_sequence() { // given diff --git a/src/persistence/manifest.rs b/src/persistence/manifest.rs index 12c113dc..6072acc6 100644 --- a/src/persistence/manifest.rs +++ b/src/persistence/manifest.rs @@ -7,7 +7,7 @@ //! rebuilt by walking the reviews directory. #![allow(dead_code)] -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; @@ -191,14 +191,30 @@ pub fn entry_from_session( None }; - let comment_count = session.review_comments.len() - + session - .files + let comment_count = if session.diff_source == SessionDiffSource::PerCommitRange { + session + .per_commit_reviews .values() - .map(|f| f.comment_count()) - .sum::(); + .map(|review| review.comment_count()) + .sum::() + } else { + session.review_comments.len() + + session + .files + .values() + .map(|f| f.comment_count()) + .sum::() + }; let reviewed_count = session.reviewed_count(); - let file_count = session.files.len(); + let file_count = if session.diff_source == SessionDiffSource::PerCommitRange { + let mut paths = HashSet::new(); + for review in session.per_commit_reviews.values() { + paths.extend(review.files.keys().cloned()); + } + paths.len() + } else { + session.files.len() + }; ManifestEntry { path: relative_path, @@ -269,6 +285,7 @@ pub fn diff_source_label(diff_source: SessionDiffSource) -> &'static str { SessionDiffSource::Unstaged => "unstaged", SessionDiffSource::StagedAndUnstaged => "staged-and-unstaged", SessionDiffSource::CommitRange => "commits", + SessionDiffSource::PerCommitRange => "per-commit", SessionDiffSource::WorkingTreeAndCommits => "worktree-and-commits", SessionDiffSource::StagedUnstagedAndCommits => "staged-and-unstaged-and-commits", SessionDiffSource::PullRequest => "pr", @@ -279,7 +296,8 @@ pub fn diff_source_label(diff_source: SessionDiffSource) -> &'static str { #[cfg(test)] mod tests { use super::*; - use crate::model::FileStatus; + use crate::model::review::FileReview; + use crate::model::{Comment, CommentType, CommitReview, FileStatus}; use std::path::PathBuf; fn entry(updated_at: DateTime, anchor: &str) -> ManifestEntry { @@ -562,6 +580,82 @@ mod tests { assert_eq!(entry.display.anchor, "main"); } + #[test] + fn should_extract_metadata_from_per_commit_session() { + let mut session = ReviewSession::new( + PathBuf::from("/tmp/test-repo"), + "bbb111122223333444455556666777788889999".to_string(), + Some("main".to_string()), + SessionDiffSource::PerCommitRange, + ); + + let mut first = CommitReview::new( + "aaa111122223333444455556666777788889999".to_string(), + "aaa1111".to_string(), + "First".to_string(), + None, + "Alice".to_string(), + Utc::now(), + ); + first.review_comments.push(Comment::new( + "Review first".to_string(), + CommentType::Issue, + None, + )); + first.files.insert( + PathBuf::from("src/a.rs"), + FileReview::new(PathBuf::from("src/a.rs"), FileStatus::Modified, 0), + ); + + let mut second = CommitReview::new( + "bbb111122223333444455556666777788889999".to_string(), + "bbb1111".to_string(), + "Second".to_string(), + None, + "Bob".to_string(), + Utc::now(), + ); + second.files.insert( + PathBuf::from("src/a.rs"), + FileReview::new(PathBuf::from("src/a.rs"), FileStatus::Modified, 0), + ); + second.files.insert( + PathBuf::from("src/b.rs"), + FileReview::new(PathBuf::from("src/b.rs"), FileStatus::Modified, 0), + ); + second + .files + .get_mut(&PathBuf::from("src/b.rs")) + .unwrap() + .reviewed = true; + second + .files + .get_mut(&PathBuf::from("src/a.rs")) + .unwrap() + .add_file_comment(Comment::new( + "Review second".to_string(), + CommentType::Suggestion, + None, + )); + + session + .per_commit_reviews + .insert(first.commit_id.clone(), first); + session + .per_commit_reviews + .insert(second.commit_id.clone(), second); + + let entry = entry_from_session( + &session, + PathBuf::from("local/abcd/foo/bar/main/per-commit.json"), + "main".to_string(), + ); + + assert_eq!(entry.display.comment_count, 2); + assert_eq!(entry.display.reviewed_count, 1); + assert_eq!(entry.display.file_count, 2); + } + #[test] fn should_extract_metadata_from_pr_session() { use crate::forge::traits::{ForgeRepository, PrSessionKey}; diff --git a/src/review_cli.rs b/src/review_cli.rs index 49b37d14..0265a422 100644 --- a/src/review_cli.rs +++ b/src/review_cli.rs @@ -10,7 +10,9 @@ use crate::cli::{LineSideArg, ReviewCommand}; use crate::config; use crate::error::{Result, TuicrError}; use crate::model::comment::{self, CommentLifecycleState}; -use crate::model::{Comment, CommentType, LineRange, LineSide, ReviewSession}; +use crate::model::{ + Comment, CommentType, CommitReview, LineRange, LineSide, ReviewSession, SessionDiffSource, +}; use crate::review_store::{ AddCommentRequest, CommentTarget, ReviewStore, SessionRef, SessionSummary, }; @@ -419,6 +421,10 @@ fn validate_line(line: u32, name: &str) -> Result<()> { } fn collect_comments(session: &ReviewSession) -> Vec { + if session.diff_source == SessionDiffSource::PerCommitRange { + return collect_per_commit_comments(session); + } + let mut comments = Vec::new(); for comment in &session.review_comments { comments.push(CommentOutput::from_parts( @@ -470,6 +476,83 @@ fn collect_comments(session: &ReviewSession) -> Vec { comments } +fn collect_per_commit_comments(session: &ReviewSession) -> Vec { + let mut comments = Vec::new(); + let mut ordered_ids = session + .commit_range + .clone() + .unwrap_or_else(|| session.per_commit_reviews.keys().cloned().collect()); + if session.commit_range.is_none() { + ordered_ids.sort(); + } + + for commit_id in ordered_ids { + let Some(commit_review) = session.per_commit_reviews.get(&commit_id) else { + continue; + }; + for comment in &commit_review.review_comments { + comments.push( + CommentOutput::from_parts("commit".to_string(), None, None, None, None, comment) + .with_commit(commit_review), + ); + } + + let mut files: Vec<_> = commit_review.files.iter().collect(); + files.sort_by_key(|(path, _)| { + if path.as_path() == Path::new("Commit Message") { + String::new() + } else { + path.to_string_lossy().to_string() + } + }); + for (path, review) in files { + let path_display = if path.as_path() == Path::new("Commit Message") { + "commit message".to_string() + } else { + path.to_string_lossy().to_string() + }; + for comment in &review.file_comments { + comments.push( + CommentOutput::from_parts( + path_display.clone(), + Some(path_display.clone()), + None, + None, + None, + comment, + ) + .with_commit(commit_review), + ); + } + + let mut line_comments: Vec<_> = review.line_comments.iter().collect(); + line_comments.sort_by_key(|(line, _)| *line); + for (line, line_comments) in line_comments { + for comment in line_comments { + let (start_line, end_line) = comment + .line_range + .map(|range| (range.start, range.end)) + .unwrap_or((*line, *line)); + let location = line_location(&path_display, start_line, end_line, comment.side); + comments.push( + CommentOutput::from_parts( + location, + Some(path_display.clone()), + Some(start_line), + Some(end_line), + comment.side, + comment, + ) + .with_commit(commit_review), + ); + } + } + } + } + + comments +} + fn line_location(path: &str, start_line: u32, end_line: u32, side: Option) -> String { let line = if start_line == end_line { start_line.to_string() @@ -543,6 +626,12 @@ impl From for SessionSummaryOutput { #[derive(Debug, Serialize)] struct CommentOutput { id: String, + #[serde(skip_serializing_if = "Option::is_none")] + commit_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + commit_short_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + commit_summary: Option, location: String, path: Option, start_line: Option, @@ -592,6 +681,9 @@ impl CommentOutput { ) -> Self { Self { id: comment.id.clone(), + commit_id: None, + commit_short_id: None, + commit_summary: None, location, path, start_line, @@ -603,14 +695,23 @@ impl CommentOutput { content: comment.content.clone(), } } + + fn with_commit(mut self, review: &CommitReview) -> Self { + self.commit_id = Some(review.commit_id.clone()); + self.commit_short_id = Some(review.short_id.clone()); + self.commit_summary = Some(review.summary.clone()); + self + } } #[cfg(test)] mod tests { use super::*; + use chrono::Utc; use tempfile::tempdir; - use crate::model::{FileStatus, SessionDiffSource}; + use crate::model::review::FileReview; + use crate::model::{CommitReview, FileStatus, SessionDiffSource}; fn test_session(repo_path: PathBuf) -> ReviewSession { let mut session = ReviewSession::new( @@ -649,6 +750,65 @@ mod tests { )); } + #[test] + fn should_collect_per_commit_comments_with_commit_metadata() { + let first_id = "aaa111122223333444455556666777788889999".to_string(); + let second_id = "bbb111122223333444455556666777788889999".to_string(); + let mut session = ReviewSession::new( + PathBuf::from("/repo"), + second_id.clone(), + Some("main".to_string()), + SessionDiffSource::PerCommitRange, + ); + session.commit_range = Some(vec![first_id.clone(), second_id.clone()]); + + let mut first = CommitReview::new( + first_id.clone(), + "aaa1111".to_string(), + "First commit".to_string(), + None, + "Alice".to_string(), + Utc::now(), + ); + first.review_comments.push(Comment::new( + "Commit-level note".to_string(), + CommentType::Note, + None, + )); + session.per_commit_reviews.insert(first_id.clone(), first); + + let mut second = CommitReview::new( + second_id.clone(), + "bbb1111".to_string(), + "Second commit".to_string(), + None, + "Bob".to_string(), + Utc::now(), + ); + let mut msg_review = FileReview::new(PathBuf::from("Commit Message"), FileStatus::Added, 0); + msg_review.add_line_comment( + 1, + Comment::new( + "Commit message note".to_string(), + CommentType::Suggestion, + Some(LineSide::New), + ), + ); + second + .files + .insert(PathBuf::from("Commit Message"), msg_review); + session.per_commit_reviews.insert(second_id.clone(), second); + + let comments = collect_comments(&session); + + assert_eq!(comments.len(), 2); + assert_eq!(comments[0].commit_short_id.as_deref(), Some("aaa1111")); + assert_eq!(comments[0].location, "commit"); + assert_eq!(comments[1].commit_short_id.as_deref(), Some("bbb1111")); + assert_eq!(comments[1].commit_summary.as_deref(), Some("Second commit")); + assert_eq!(comments[1].location, "commit message:1"); + } + #[test] fn should_reject_zero_line() { let err = build_comment_target( diff --git a/src/review_store.rs b/src/review_store.rs index 1df1fbe6..20faa0f4 100644 --- a/src/review_store.rs +++ b/src/review_store.rs @@ -3,7 +3,9 @@ use std::path::{Path, PathBuf}; use chrono::{DateTime, Utc}; use crate::error::{Result, TuicrError}; -use crate::model::{Comment, CommentType, LineRange, LineSide, ReviewSession}; +use crate::model::{ + Comment, CommentType, CommitReview, LineRange, LineSide, ReviewSession, SessionDiffSource, +}; use crate::persistence::manifest::{ManifestEntry, ManifestKind}; use crate::persistence::storage; @@ -78,7 +80,11 @@ impl ReviewStore { let reviews_dir = self.reviews_dir()?; let (_session, comment) = storage::update_session_in_dir(session_ref.path(), &reviews_dir, |session| { - add_comment_to_session(session, request) + if session.diff_source == SessionDiffSource::PerCommitRange { + add_comment_to_active_per_commit_session(session, request) + } else { + add_comment_to_session(session, request) + } })?; Ok(comment) } @@ -248,6 +254,47 @@ pub fn add_comment_to_session( Ok(comment) } +fn add_comment_to_active_per_commit_session( + session: &mut ReviewSession, + request: AddCommentRequest, +) -> Result { + let commit_id = active_per_commit_id(session)?; + let comment = add_comment_to_session(session, request)?; + let short_id = commit_id[..7.min(commit_id.len())].to_string(); + let review = session + .per_commit_reviews + .entry(commit_id.clone()) + .or_insert_with(|| { + CommitReview::new( + commit_id.clone(), + short_id, + "(unknown commit)".to_string(), + None, + String::new(), + Utc::now(), + ) + }); + review.review_comments = session.review_comments.clone(); + review.files = session.files.clone(); + Ok(comment) +} + +fn active_per_commit_id(session: &ReviewSession) -> Result { + let ids = session.commit_range.as_ref().ok_or_else(|| { + TuicrError::InvalidInput("per-commit session does not contain a commit range".to_string()) + })?; + let index = match session.commit_selection_range { + Some((start, end)) if start == end && end < ids.len() => start, + None if ids.len() == 1 => 0, + _ => { + return Err(TuicrError::InvalidInput( + "per-commit session does not have a single active commit".to_string(), + )); + } + }; + Ok(ids[index].clone()) +} + fn file_review_mut<'a>( session: &'a mut ReviewSession, path: &Path, @@ -260,6 +307,7 @@ fn file_review_mut<'a>( #[cfg(test)] mod tests { use super::*; + use crate::model::review::FileReview; use crate::model::{FileStatus, SessionDiffSource}; fn test_session(repo_path: PathBuf) -> ReviewSession { @@ -405,4 +453,93 @@ mod tests { let listed = store.list_sessions_for_repo(&repo).unwrap(); assert_eq!(listed[0].comment_count, 1); } + + #[test] + fn should_add_comment_to_active_commit_in_per_commit_session() { + let temp = tempfile::tempdir().unwrap(); + let repo = temp.path().join("repo"); + std::fs::create_dir_all(&repo).unwrap(); + let reviews_dir = temp.path().join("reviews"); + let store = ReviewStore::with_reviews_dir(reviews_dir); + + let first_id = "aaa111122223333444455556666777788889999".to_string(); + let second_id = "bbb111122223333444455556666777788889999".to_string(); + let mut session = ReviewSession::new( + repo, + second_id.clone(), + Some("main".to_string()), + SessionDiffSource::PerCommitRange, + ); + session.commit_range = Some(vec![first_id.clone(), second_id.clone()]); + session.commit_selection_range = Some((1, 1)); + session.files.insert( + PathBuf::from("src/main.rs"), + FileReview::new(PathBuf::from("src/main.rs"), FileStatus::Modified, 0), + ); + session.per_commit_reviews.insert( + second_id.clone(), + CommitReview::new( + second_id.clone(), + "bbb1111".to_string(), + "Second commit".to_string(), + None, + "Bob".to_string(), + Utc::now(), + ), + ); + let session_ref = store.save_review(&session).unwrap(); + + store + .add_comment( + &session_ref, + AddCommentRequest { + target: CommentTarget::Line { + path: PathBuf::from("src/main.rs"), + line: 7, + side: LineSide::New, + }, + content: "line note".to_string(), + comment_type: CommentType::Note, + author: crate::model::comment::DEFAULT_AUTHOR.to_string(), + }, + ) + .unwrap(); + + let loaded = store.get_review(&session_ref).unwrap(); + let commit = loaded + .per_commit_reviews + .get(&second_id) + .expect("active commit review"); + let review = commit.files.get(&PathBuf::from("src/main.rs")).unwrap(); + assert_eq!(review.line_comments.get(&7).unwrap().len(), 1); + + let listed = store.list_sessions_for_repo(loaded.repo_path).unwrap(); + assert_eq!(listed[0].comment_count, 1); + } + + #[test] + fn should_reject_per_commit_comment_without_single_active_commit() { + let first_id = "aaa111122223333444455556666777788889999".to_string(); + let second_id = "bbb111122223333444455556666777788889999".to_string(); + let mut session = ReviewSession::new( + PathBuf::from("/repo"), + second_id.clone(), + Some("main".to_string()), + SessionDiffSource::PerCommitRange, + ); + session.commit_range = Some(vec![first_id, second_id]); + + let err = add_comment_to_active_per_commit_session( + &mut session, + AddCommentRequest { + target: CommentTarget::Review, + content: "note".to_string(), + comment_type: CommentType::Note, + author: crate::model::comment::DEFAULT_AUTHOR.to_string(), + }, + ) + .unwrap_err(); + + assert!(matches!(err, TuicrError::InvalidInput(_))); + } } diff --git a/src/slug.rs b/src/slug.rs index 2fbe4ebf..12419ff3 100644 --- a/src/slug.rs +++ b/src/slug.rs @@ -74,6 +74,7 @@ pub enum SlugSource { StagedAndUnstaged(String), Pristine, Commits(CommitRange), + PerCommitRange(CommitRange), WorktreeAndCommits(CommitRange), StagedUnstagedAndCommits(CommitRange), } @@ -137,6 +138,9 @@ impl fmt::Display for SlugSource { SlugSource::StagedAndUnstaged(head) => write!(f, "staged-and-unstaged/{head}"), SlugSource::Pristine => f.write_str("pristine"), SlugSource::Commits(r) => write!(f, "commits/{}..{}", r.base, r.head), + SlugSource::PerCommitRange(r) => { + write!(f, "per-commit/{}..{}", r.base, r.head) + } SlugSource::WorktreeAndCommits(r) => { write!(f, "worktree-and-commits/{}..{}", r.base, r.head) } @@ -253,6 +257,9 @@ fn parse_source(s: &str) -> Result { if let Some(range) = s.strip_prefix("commits/") { return Ok(SlugSource::Commits(parse_range(range)?)); } + if let Some(range) = s.strip_prefix("per-commit/") { + return Ok(SlugSource::PerCommitRange(parse_range(range)?)); + } if let Some(range) = s.strip_prefix("worktree-and-commits/") { return Ok(SlugSource::WorktreeAndCommits(parse_range(range)?)); } @@ -521,6 +528,10 @@ fn build_source( SessionDiffSource::CommitRange => { Ok(SlugSource::Commits(range_from(commit_range, diff_source)?)) } + SessionDiffSource::PerCommitRange => Ok(SlugSource::PerCommitRange(range_from( + commit_range, + diff_source, + )?)), SessionDiffSource::WorkingTreeAndCommits => Ok(SlugSource::WorktreeAndCommits(range_from( commit_range, diff_source, diff --git a/src/ui/help_popup.rs b/src/ui/help_popup.rs index cbfa7927..4fd31c6c 100644 --- a/src/ui/help_popup.rs +++ b/src/ui/help_popup.rs @@ -183,7 +183,7 @@ pub fn render_help(frame: &mut Frame, app: &mut App) { )), Line::from(""), Line::from(Span::styled( - "Commit Selector (multi-commit reviews)", + "Commit Selector (multi-commit / per-commit reviews)", Style::default().add_modifier(Modifier::BOLD | Modifier::UNDERLINED), )), Line::from(""), @@ -209,7 +209,7 @@ pub fn render_help(frame: &mut Frame, app: &mut App) { " (/) ", Style::default().add_modifier(Modifier::BOLD), ), - Span::raw("Cycle through individual commits"), + Span::raw("Cycle active commit or aggregate range"), ]), Line::from(vec![ Span::styled( diff --git a/src/ui/status_bar.rs b/src/ui/status_bar.rs index c6888dc1..497c3a21 100644 --- a/src/ui/status_bar.rs +++ b/src/ui/status_bar.rs @@ -178,6 +178,17 @@ fn header_source_chunk(app: &App) -> Option { } } } + DiffSource::PerCommitRange(commits) => { + let total = commits.len(); + match app.commit_selection_range { + Some((idx, idx2)) if idx == idx2 => app + .review_commits + .get(idx) + .map(|commit| format!("commit {}/{} {}", idx + 1, total, commit.short_id)) + .or_else(|| Some(format!("per-commit {total} commits"))), + _ => Some(format!("per-commit {total} commits")), + } + } DiffSource::StagedUnstagedAndCommits(commits) => { if commits.len() == 1 { Some(format!(