feat: support comments in ordinary source buffers

This commit is contained in:
2026-10-22 22:30:16 +02:00
parent ac5c64981d
commit e6d94dcb41
16 changed files with 1005 additions and 600 deletions
-2
View File
@@ -1,5 +1,3 @@
- [x] feature: syntax/lsp - add a default configuration for typst.
- for the lsp, i'm thinking tinymist is a good candidate.
- [ ] feature: diff view - GitHub code review integration after the local review UX is usable. - [ ] feature: diff view - GitHub code review integration after the local review UX is usable.
- publish local reviews explicitly: general PR comments, inline line/range comments, review summaries, approval, and requests for changes. - publish local reviews explicitly: general PR comments, inline line/range comments, review summaries, approval, and requests for changes.
- preserve explicit PR identity and reviewed commit IDs; validate anchors against GitHub's PR diff and recheck the PR head before submission. - preserve explicit PR identity and reviewed commit IDs; validate anchors against GitHub's PR diff and recheck the PR head before submission.
+1 -1
View File
@@ -20,7 +20,7 @@ One editor crate; the workspace also includes its patched Crossterm dependency.
- `review`: owns the validated local review model, exact immutable anchors, persisted file checkmarks, SQLite storage through bundled rusqlite, and atomic JSON import/export with external-change protection. Schema 3 stores review identity, repository, revision, summary, and verdict in `reviews`; `comments` stores ordered comment IDs, bodies, resolution states, and explicit immutable anchor fields. `reviewed_files` stores revision pairs, paths, optional old paths, and optional exact live After text. Foreign keys, unique keys, and checks enforce relationships and scalar invariants. Full `u64` comment IDs use checked decimal text because SQLite integers are signed. No JSON documents are stored in SQLite. - `review`: owns the validated local review model, exact immutable anchors, persisted file checkmarks, SQLite storage through bundled rusqlite, and atomic JSON import/export with external-change protection. Schema 3 stores review identity, repository, revision, summary, and verdict in `reviews`; `comments` stores ordered comment IDs, bodies, resolution states, and explicit immutable anchor fields. `reviewed_files` stores revision pairs, paths, optional old paths, and optional exact live After text. Foreign keys, unique keys, and checks enforce relationships and scalar invariants. Full `u64` comment IDs use checked decimal text because SQLite integers are signed. No JSON documents are stored in SQLite.
`Store` retains compare-and-swap review revisions and atomic whole-review saves: metadata and replacement comment/checkmark rows commit together. Read transactions reconstruct validated reviews from one consistent snapshot, including when listing in descending ID order. Stored text is bounded to 64 MiB per review, including a fixed per-comment/checkmark overhead. JSON version 4 is an independent import/export format. Incompatible databases, including schemas 1 and 2, are rejected and preserved without migration or reset. CLI metadata, sole-review restoration, and selector labels reuse loaded reviews; selector choices live only in the displayed buffer. `Store` retains compare-and-swap review revisions and atomic whole-review saves: metadata and replacement comment/checkmark rows commit together. Read transactions reconstruct validated reviews from one consistent snapshot, including when listing in descending ID order. Stored text is bounded to 64 MiB per review, including a fixed per-comment/checkmark overhead. JSON version 4 is an independent import/export format. Incompatible databases, including schemas 1 and 2, are rejected and preserved without migration or reset. CLI metadata, sole-review restoration, and selector labels reuse loaded reviews; selector choices live only in the displayed buffer.
The review CLI keeps `list`/`show` read-only. Comment deletion validates the explicit review/comment IDs through an existing read-only store before opening it for a compare-and-swap save; absent storage and invalid targets do not create reviews or a database. The review CLI keeps `list`/`show` read-only. Comment deletion validates the explicit review/comment IDs through an existing read-only store before opening it for a compare-and-swap save; absent storage and invalid targets do not create reviews or a database.
`editor_review` commits native comment/summary buffers on dismissal or switching, and verdicts/deletions immediately. It restores a sole stored review or opens the floating selector for multiple reviews. Exports retain their own file baseline and do not become automatic persistence targets; imports get fresh database identities. Review editors remain tab-owned native panes outside the workspace `PaneTree`. Storage opens lazily when a comparison is ready; the session supplies the configured or XDG default directory. Direct `Editor` callers inject a directory with `set_review_directory` to enable persistence. `editor_review` owns tab-local review state independently of comparisons. Ordinary repository source buffers and diff panes share the comment editor, immutable anchors, storage, and review commands. Comment/summary text saves explicitly; dismissing discards that editor's unsaved text, while switching may retain hidden drafts. Verdicts/deletions/resolution changes commit immediately without saving drafts. It restores a sole stored review or opens the floating selector for multiple reviews. Exports retain their own file baseline and do not become automatic persistence targets; imports get fresh database identities. Review editors remain tab-owned native panes outside the workspace `PaneTree`. The session supplies the configured or XDG default directory; direct `Editor` callers inject a directory with `set_review_directory` to enable persistence.
Successful SQLite autosaves and JSON-only saves share one acknowledgement operation: clear review dirtiness and advance the owning tab's native draft saved baselines only after persistence succeeds. Failed writes retain unsaved state; JSON exports do not acknowledge a pending SQLite save. Successful SQLite autosaves and JSON-only saves share one acknowledgement operation: clear review dirtiness and advance the owning tab's native draft saved baselines only after persistence succeeds. Failed writes retain unsaved state; JSON exports do not acknowledge a pending SQLite save.
Review replacement and comment deletion are private editor-owned operations. Replacement loads and validates its Stored/JSON/New source before retiring old bindings, installing the document, and resetting selection/navigation. Deletion retires and acknowledges only the matching comment drafts, preventing stale text from overwriting reused IDs. Both persist before opening the overview; a post-mutation save failure retains the applied, unsaved identity change. Native text/undo and general transition guards remain unchanged. Review replacement and comment deletion are private editor-owned operations. Replacement loads and validates its Stored/JSON/New source before retiring old bindings, installing the document, and resetting selection/navigation. Deletion retires and acknowledges only the matching comment drafts, preventing stale text from overwriting reused IDs. Both persist before opening the overview; a post-mutation save failure retains the applied, unsaved identity change. Native text/undo and general transition guards remain unchanged.
- `clipboard`: owns native plain-text provider selection and bounded subprocess I/O; it never computes selections or editing offsets. - `clipboard`: owns native plain-text provider selection and bounded subprocess I/O; it never computes selections or editing offsets.
+8 -8
View File
@@ -259,8 +259,8 @@ Normal and Visual Ctrl+u/Ctrl+d invoke `config.core.scroll_step_up` / `config.co
| Normal | `edit`, `buffers`, `buffer`, `buffer_next`, `buffer_previous`, `buffer_delete`, `buffer_force_delete` | | Normal | `edit`, `buffers`, `buffer`, `buffer_next`, `buffer_previous`, `buffer_delete`, `buffer_force_delete` |
| Normal | `follow_link` | | Normal | `follow_link` |
| Normal | `diff`, `diff.mode.toggle`, `diff.view.toggle`, `diff.orientation.toggle`, `diff.review.toggle` | | Normal | `diff`, `diff.mode.toggle`, `diff.view.toggle`, `diff.orientation.toggle`, `diff.review.toggle` |
| Normal, Visual | `diff.comment` | | Normal, Visual | `comment`, `diff.comment` |
| Normal | `diff.review.edit`, `diff.review.delete`, `diff.review.resolve`, `diff.review.unresolve`, `diff.review.next`, `diff.review.previous`, `diff.review.overview`, `diff.review.copy`, `diff.review.summary`, `diff.review.verdict`, `diff.review.save`, `diff.review.open`, `diff.review.new` | | Normal | `review.edit`, `review.delete`, `review.resolve`, `review.unresolve`, `review.next`, `review.previous`, `review.overview`, `review.copy`, `review.summary`, `review.verdict`, `review.save`, `review.open`, `review.new` |
| Normal, Insert, Visual, Command, Search | `diff.close` | | Normal, Insert, Visual, Command, Search | `diff.close` |
| Normal | `resolve`, `resolve.next`, `resolve.previous`, `resolve.use_source`, `resolve.source`, `resolve.base`, `resolve.next_file`, `resolve.previous_file`, `resolve.files`, `resolve.draft`, `resolve.history`, `resolve.view.toggle`, `resolve.close` | | Normal | `resolve`, `resolve.next`, `resolve.previous`, `resolve.use_source`, `resolve.source`, `resolve.base`, `resolve.next_file`, `resolve.previous_file`, `resolve.files`, `resolve.draft`, `resolve.history`, `resolve.view.toggle`, `resolve.close` |
| Normal, Insert, Visual, Command, Search | `word_wrap.toggle` | | Normal, Insert, Visual, Command, Search | `word_wrap.toggle` |
@@ -346,16 +346,16 @@ Ordinary `config.core.file_picker` and `config.core.file_explorer` bindings work
`config.core.diff.review.toggle` is a non-mutating Normal-mode action, also available as `:diff.review.toggle` and in the palette. It marks/unmarks the highlighted changed-file leaf when the comparison's explorer is focused, or the current comparison file from a text pane. Read-only snapshots permit review marks. The default `c` shortcut applies only while browsing the diff's changed-file tree; explicit bindings and disabled keys override it. For example, `config.bind "ctrl+g", config.core.diff.review.toggle`; use the palette or a control-key binding while filtering, including List view, so printable input stays in the query. `config.core.diff.review.toggle` is a non-mutating Normal-mode action, also available as `:diff.review.toggle` and in the palette. It marks/unmarks the highlighted changed-file leaf when the comparison's explorer is focused, or the current comparison file from a text pane. Read-only snapshots permit review marks. The default `c` shortcut applies only while browsing the diff's changed-file tree; explicit bindings and disabled keys override it. For example, `config.bind "ctrl+g", config.core.diff.review.toggle`; use the palette or a control-key binding while filtering, including List view, so printable input stays in the query.
Local comments use `config.core.diff.comment` in Normal or Visual mode; the remaining `config.core.diff.review` actions in the table above use Normal mode. They have no default bindings and are available through the palette and canonical Ex commands. `edit`, `delete`, `resolve`, `unresolve`, `verdict`, `save`, and `open` open argument prompts when selected as actions. Comment IDs are optional and default to the overview entry under the caret or selected comment. For example: Local comments use `config.core.comment` in Normal or Visual mode in ordinary repository source buffers and diff text panes; the remaining `config.core.review` actions in the table above use Normal mode. Update custom Ruby bindings from `config.core.diff.comment` and `config.core.diff.review.*` to these names; only the old Ex command spellings (`:diff.comment` and `:diff.review.*`) remain aliases. File checkmarks still use `config.core.diff.review.toggle`. These actions have no default bindings and are available through the palette and Ex commands. `edit`, `delete`, `resolve`, `unresolve`, `verdict`, `save`, and `open` open argument prompts when selected as actions. Comment IDs are optional and default to the overview entry under the caret or selected comment. For example:
```ruby ```ruby
config.bind "leader r c", config.core.diff.comment config.bind "leader r c", config.core.comment
config.bind "leader r c", config.core.diff.comment, mode: config.modes.visual config.bind "leader r c", config.core.comment, mode: config.modes.visual
config.bind "leader r o", config.core.diff.review.overview config.bind "leader r o", config.core.review.overview
config.bind "leader r n", config.core.diff.review.next config.bind "leader r n", config.core.review.next
``` ```
Review editing uses a floating window without adding workspace splits. Dismissing or switching editors commits text to local SQLite; verdicts, deletions, and resolution changes commit immediately. `:w` commits explicitly; `:diff.review.save path` exports JSON without redirecting autosaves. `config.review_directory` defaults to `nil`, selecting `$XDG_DATA_HOME/vamp/reviews/` (absolute XDG path only), otherwise `~/.local/share/vamp/reviews/`. String overrides are resolved relative to the configuration file; empty/NUL-containing strings and non-string/non-nil values are rejected. These actions do not contact a forge. See [local review behavior](editing.md#repository-diff). Review editing uses a floating window without adding workspace splits. `:w` or `:review.save` commits comment/summary text, including hidden drafts; dismissing an editor discards its unsaved text, and switching editors does not save it. Verdicts, deletions, and resolution changes commit immediately without saving drafts. `:review.save path` additionally exports JSON without redirecting autosaves. `config.review_directory` defaults to `nil`, selecting `$XDG_DATA_HOME/vamp/reviews/` (absolute XDG path only), otherwise `~/.local/share/vamp/reviews/`. String overrides are resolved relative to the configuration file; empty/NUL-containing strings and non-string/non-nil values are rejected. These actions do not contact a forge. See [local review behavior](editing.md#repository-diff).
`config.core.pane.close` and `config.core.pane.force_close` close a focused **docked** file list instead of its editor pane, without discarding buffers or exiting the comparison. A floating picker does not intercept these actions: normal close still protects underlying dirty buffers, while force-close retains its existing discard semantics. For example, bind `config.bind "ctrl+x", config.core.pane.close`; focused tree/selection overrides and printable filter input retain priority. Palette-originated close actions retain the originating dock target through force confirmation. `config.core.pane.close` and `config.core.pane.force_close` close a focused **docked** file list instead of its editor pane, without discarding buffers or exiting the comparison. A floating picker does not intercept these actions: normal close still protects underlying dirty buffers, while force-close retains its existing discard semantics. For example, bind `config.bind "ctrl+x", config.core.pane.close`; focused tree/selection overrides and printable filter input retain priority. Palette-originated close actions retain the originating dock target through force confirmation.
+2
View File
@@ -98,6 +98,8 @@ Diff-stat checks cover unopened frozen files, additions/deletions/renames, binar
Local review checks cover native comment/summary composition and undo, Visual ranges on either side, verdicts, JSON save/reopen, external-write refusal, hidden drafts and teardown guards, immutable Working identities in Git/jj, renamed paths, live EOF, and stale anchors. Render checks cover selected/range markers, split/unified context, wrapped continuations, and narrow gutters. Live PTY smoke creates and saves a comment/summary/verdict, closes and reopens the review, edits the reopened comment, changes the reviewed source, and observes stale status without altering the saved anchor. Fixtures are local repositories; no forge writes. Local review checks cover native comment/summary composition and undo, Visual ranges on either side, verdicts, JSON save/reopen, external-write refusal, hidden drafts and teardown guards, immutable Working identities in Git/jj, renamed paths, live EOF, and stale anchors. Render checks cover selected/range markers, split/unified context, wrapped continuations, and narrow gutters. Live PTY smoke creates and saves a comment/summary/verdict, closes and reopens the review, edits the reopened comment, changes the reviewed source, and observes stale status without altering the saved anchor. Fixtures are local repositories; no forge writes.
Ordinary-source review checks cover unsaved Unicode selections, line/empty-line anchors, no storage creation on initialization, explicit save/discard, optimistic conflicts, tab/pane guards, JSON round-trips, stale read-only snapshots, and rendered exact-range markers. Live PTY smoke selects a source line, creates a comment with `:comment`, saves with `:w`, verifies its exact excerpt through the review CLI, restarts with `:review.next`, and reopens after changing the selected source to observe the preserved original snapshot. Review state is tab-owned rather than comparison-owned; diff regression coverage remains required.
Review clipboard checks cover System/Separate routing with isolated providers, exact Unicode excerpts, resolved/stale comments across files, retained unsaved drafts, unchanged storage revisions, and empty/invalid/failed-copy preservation. Live PTY smoke imports a review, copies it, pastes into an empty buffer, and verifies the saved bytes exactly match CLI `show --all`; default CLI output still excludes resolved comments. Review clipboard checks cover System/Separate routing with isolated providers, exact Unicode excerpts, resolved/stale comments across files, retained unsaved drafts, unchanged storage revisions, and empty/invalid/failed-copy preservation. Live PTY smoke imports a review, copies it, pastes into an empty buffer, and verifies the saved bytes exactly match CLI `show --all`; default CLI output still excludes resolved comments.
SQLite review checks cover exact anchors across restarts and revision changes, repository isolation, independent connections and optimistic conflicts, locked/unwritable stores, incompatible schema preservation, relational constraints, full unsigned comment IDs, stable comment order, bounded/corrupt rows, rollback after partial comment insertion, private file permissions, and XDG/config path resolution. Schema 1 document storage is rejected without modification. Editor checks require explicit writes for comment/summary text, discard unsaved text on dismissal without retaining discarded undo history, prevent metadata autosaves from committing hidden drafts, persist verdicts/deletions, restore a sole review, choose among multiple reviews, and keep exports independent from autosaves. Test fixtures use isolated review directories. Live PTY smoke verifies that Escape leaves no new stored comment, `:w` persists exact text, and pane close discards a later edit; reopening and writing again preserves the saved body. SQLite review checks cover exact anchors across restarts and revision changes, repository isolation, independent connections and optimistic conflicts, locked/unwritable stores, incompatible schema preservation, relational constraints, full unsigned comment IDs, stable comment order, bounded/corrupt rows, rollback after partial comment insertion, private file permissions, and XDG/config path resolution. Schema 1 document storage is rejected without modification. Editor checks require explicit writes for comment/summary text, discard unsaved text on dismissal without retaining discarded undo history, prevent metadata autosaves from committing hidden drafts, persist verdicts/deletions, restore a sole review, choose among multiple reviews, and keep exports independent from autosaves. Test fixtures use isolated review directories. Live PTY smoke verifies that Escape leaves no new stored comment, `:w` persists exact text, and pane close discards a later edit; reopening and writing again preserves the saved body.
+19 -15
View File
@@ -482,27 +482,29 @@ Collapsed folders show a green **`✓`** when every changed file beneath them is
Review marks are saved with the selected local review and restored by immutable per-file Before/After identities and file paths. Changes to unrelated files or commit metadata do not clear them; changes to either side's bytes, final newline, file mode, or rename paths require re-review. They survive filtering, file navigation, lazy loading, tabs, split/unified/orientation changes, and pinned checkout-only transitions. Editing a writable After file clears only that file's mark and saves the removal; unchanged refreshes and saves preserve it. Marks on live text also retain its exact contents, including the final newline, so different unsaved text cannot inherit a checkmark. Marks do not change document text, undo history, or repository state. Without configured SQLite storage, use `:diff.review.save path` to save them as JSON. Older revision-pair-scoped marks remain stored but must be marked again to acquire per-file identities; comments, summaries, and verdicts are unaffected. Review marks are saved with the selected local review and restored by immutable per-file Before/After identities and file paths. Changes to unrelated files or commit metadata do not clear them; changes to either side's bytes, final newline, file mode, or rename paths require re-review. They survive filtering, file navigation, lazy loading, tabs, split/unified/orientation changes, and pinned checkout-only transitions. Editing a writable After file clears only that file's mark and saves the removal; unchanged refreshes and saves preserve it. Marks on live text also retain its exact contents, including the final newline, so different unsaved text cannot inherit a checkmark. Marks do not change document text, undo history, or repository state. Without configured SQLite storage, use `:diff.review.save path` to save them as JSON. Older revision-pair-scoped marks remain stored but must be marked again to acquire per-file identities; comments, summaries, and verdicts are unaffected.
**Local review comments** share local storage with the file-reviewed checkmarks above; neither feature publishes to GitHub. Focus either diff text pane and run **`:diff.comment`** to comment on the current line. A characterwise Visual selection (`v`) anchors its exact text, including partial lines and multiline spans; Visual Line (`V`) anchors whole selected lines. Type **`:diff.comment`** and press Enter, or use the palette/a configured binding. Multiple independent carets/ranges must be reviewed separately. **Local review comments** work in ordinary repository source buffers and diff text panes; they share storage with file-reviewed checkmarks and never publish to GitHub. Run **`:comment`** to comment on the current line. A characterwise Visual selection (`v`) anchors its exact text, including partial lines and multiline spans; Visual Line (`V`) anchors whole selected lines. Multiple independent carets/ranges must be reviewed separately. `:diff.comment` remains available for the same operation.
Comments, summaries, and the review overview open in a focused floating editor over the whole diff text workspace, not a source-side split. The source layout, file explorer, and viewports stay unchanged. Normal editing, selections, and undo/redo work in editable review buffers. Escape in Normal mode or `:pane.close` dismisses the window and returns to the diff, discarding that editor's unsaved text. An unsaved new comment is not attached to the review; an existing comment or summary reopens with its last saved text and no discarded undo history. Insert/Visual/Command Escape first retains its usual modal behavior. Source rows are never replaced with comment text. Magenta **`○`** gutter markers identify saved comment ranges; **`●`** identifies the selected comment. Its exact current text range has a magenta background, including wrapped continuations and selected newline cells; native Visual selections take precedence. Stale comments have neither markers nor text highlighting. Markers preserve addition/deletion signs, appear only on the first wrapped segment, and also work in unified view. Comments, summaries, and the review overview open in a focused floating editor over the source workspace, not a source-side split. The source layout, file explorer, and viewports stay unchanged. Normal editing, selections, and undo/redo work in editable review buffers. Escape in Normal mode or `:pane.close` dismisses the window and returns to the source, discarding that editor's unsaved text. An unsaved new comment is not attached to the review; an existing comment or summary reopens with its last saved text and no discarded undo history. Insert/Visual/Command Escape first retains its usual modal behavior. Source rows are never replaced with comment text. Magenta **`○`** gutter markers identify saved comment ranges; **`●`** identifies the selected comment. Its exact current text range has a magenta background, including wrapped continuations and selected newline cells; native Visual selections take precedence. Stale comments have neither markers nor text highlighting. Markers preserve addition/deletion signs, appear only on the first wrapped segment, and also work in unified view.
Local review buffers (comments, summaries, overview, and stored-review selector) soft-wrap by default without inserting newlines into saved text. Use `:word_wrap.toggle` to disable wrapping for the focused review buffer; reopening it retains that choice for the session. Source diff wrapping remains independent. Local review buffers (comments, summaries, overview, and stored-review selector) soft-wrap by default without inserting newlines into saved text. Use `:word_wrap.toggle` to disable wrapping for the focused review buffer; reopening it retains that choice for the session. Source diff wrapping remains independent.
| Command | Effect | | Command | Effect |
| --- | --- | | --- | --- |
| `:diff.comment` | Add a comment on the current line or exact selected text range, on the focused side | | `:comment` | Add a comment on the current source line or exact selected text range |
| `:diff.review.edit [id]` | Edit an existing comment; defaults to the overview entry under the caret or selected comment | | `:review.edit [id]` | Edit an existing comment; defaults to the overview entry under the caret or selected comment |
| `:diff.review.delete [id]` | Delete that draft comment | | `:review.delete [id]` | Delete that draft comment |
| `:diff.review.resolve [id]` / `:diff.review.unresolve [id]` | Mark a comment resolved/unresolved; uses the same default selection as edit/delete | | `:review.resolve [id]` / `:review.unresolve [id]` | Mark a comment resolved/unresolved; uses the same default selection as edit/delete |
| `:diff.review.next` / `:diff.review.previous` | Cycle comments, opening their text and revealing a matching source anchor | | `:review.next` / `:review.previous` | Cycle comments, opening their text and revealing a matching source anchor |
| `:diff.review.overview` | Refresh/open a read-only summary and comment list; Enter edits the entry under the caret | | `:review.overview` | Refresh/open a read-only summary and comment list; Enter edits the entry under the caret |
| `:diff.review.copy` | Copy all saved comments in the selected review to the configured clipboard, using the CLI text format | | `:review.copy` | Copy all saved comments in the selected review to the configured clipboard, using the CLI text format |
| `:diff.review.summary` | Compose the review summary | | `:review.summary` | Compose the review summary |
| `:diff.review.verdict looks_good` | Record **Looks good** | | `:review.verdict looks_good` | Record **Looks good** |
| `:diff.review.verdict changes_requested` | Record **Changes requested**; `none` clears the verdict | | `:review.verdict changes_requested` | Record **Changes requested**; `none` clears the verdict |
| `:diff.review.save [path]` | Commit to SQLite; with a path, also export JSON for sharing | | `:review.save [path]` | Commit to SQLite; with a path, also export JSON for sharing |
| `:diff.review.open [id or path]` | Show stored reviews, open a stored review ID, or import a JSON review as a new review | | `:review.open [id or path]` | Show stored reviews, open a stored review ID, or import a JSON review as a new review |
| `:diff.review.new` | Create a separate stored review for the current repository | | `:review.new` | Create a separate stored review for the current repository |
The `:diff.review.*` spellings remain available for these same operations. File checkmarks remain diff-specific (`:diff.review.toggle`).
Comment and summary text is committed only by **`:w`** or **`:diff.review.save`**, which saves the entire review, including hidden drafts. Dismissing an editor discards its unsaved text; switching between review editors can retain hidden drafts but does not save them. File checkmarks, verdicts, deletions, and manual resolution changes commit immediately to local SQLite without committing draft text. `:diff.review.save path` or `:w path` additionally exports JSON; later metadata autosaves still go to SQLite and leave that export unchanged. Imports create a new stored review identity rather than overwriting another review. Comment and summary text is committed only by **`:w`** or **`:diff.review.save`**, which saves the entire review, including hidden drafts. Dismissing an editor discards its unsaved text; switching between review editors can retain hidden drafts but does not save them. File checkmarks, verdicts, deletions, and manual resolution changes commit immediately to local SQLite without committing draft text. `:diff.review.save path` or `:w path` additionally exports JSON; later metadata autosaves still go to SQLite and leave that export unchanged. Imports create a new stored review identity rather than overwriting another review.
@@ -520,6 +522,8 @@ Version 4 JSON is the agent-readable sharing format: repository root, summary, o
Reopening requires the same repository root. Unchanged comment ranges follow edits elsewhere in the file, including inserted/deleted lines above and column shifts before a partial-line selection. Working-snapshot refreshes can retain these attachments; unrelated immutable revisions and paths do not match. Mapping requires an unchanged, unambiguous text region containing the selection. Modified/deleted selections or ambiguous repeated regions are **stale**: comments remain available through the overview and next/previous navigation, with original snapshot coordinates and no current-source marker or jump. Saved anchors and exported excerpts remain unchanged. Unloaded files are shown as **not checked** until visited; refresh the overview to update its statuses. There is no server, authentication, or review-publishing operation. Reopening requires the same repository root. Unchanged comment ranges follow edits elsewhere in the file, including inserted/deleted lines above and column shifts before a partial-line selection. Working-snapshot refreshes can retain these attachments; unrelated immutable revisions and paths do not match. Mapping requires an unchanged, unambiguous text region containing the selection. Modified/deleted selections or ambiguous repeated regions are **stale**: comments remain available through the overview and next/previous navigation, with original snapshot coordinates and no current-source marker or jump. Saved anchors and exported excerpts remain unchanged. Unloaded files are shown as **not checked** until visited; refresh the overview to update its statuses. There is no server, authentication, or review-publishing operation.
Ordinary source comments use an immutable content snapshot identity and capture the current buffer, including unsaved text; they do not require a commit or a diff. They retain the same SQLite/JSON formats. Opening a source comment navigates to its file and mapped selection; if that text is stale or the file is unavailable, it opens the preserved original text read-only instead of attaching feedback to different code.
Resolution is a manual human decision, independent of anchor freshness: a comment can be both resolved and stale. Overview entries and comment titles label resolved/unresolved status. Resolved comments remain in the overview and next/previous navigation, and can still be edited or reopened. Resolving never changes an anchor or deletes feedback. Resolution is a manual human decision, independent of anchor freshness: a comment can be both resolved and stale. Overview entries and comment titles label resolved/unresolved status. Resolved comments remain in the overview and next/previous navigation, and can still be edited or reopened. Resolving never changes an anchor or deletes feedback.
+24 -26
View File
@@ -641,7 +641,6 @@ pub struct DiffState {
pub loading: bool, pub loading: bool,
pub error: Option<String>, pub error: Option<String>,
pub comparison: Option<Comparison>, pub comparison: Option<Comparison>,
pub local_review: crate::review::ReviewState,
pub comparison_id: Option<u64>, pub comparison_id: Option<u64>,
pub reviewed_files: BTreeSet<usize>, pub reviewed_files: BTreeSet<usize>,
pub(crate) snapshot_roots: [Option<PathBuf>; 2], pub(crate) snapshot_roots: [Option<PathBuf>; 2],
@@ -708,7 +707,6 @@ impl DiffState {
loading: true, loading: true,
error: None, error: None,
comparison: None, comparison: None,
local_review: Default::default(),
comparison_id: None, comparison_id: None,
reviewed_files: BTreeSet::new(), reviewed_files: BTreeSet::new(),
files: None, files: None,
@@ -1988,7 +1986,10 @@ impl DiffState {
}) })
} }
pub(crate) fn toggle_reviewed(&mut self) -> Result<bool, &'static str> { pub(crate) fn toggle_reviewed(
&mut self,
review: &mut crate::review::ReviewState,
) -> Result<bool, &'static str> {
let comparison = self let comparison = self
.comparison .comparison
.as_ref() .as_ref()
@@ -2030,10 +2031,10 @@ impl DiffState {
.iter() .iter()
.all(|revision| !revision.is_empty()) .all(|revision| !revision.is_empty())
{ {
if self.local_review.document.root.as_os_str().is_empty() { if review.document.root.as_os_str().is_empty() {
self.local_review.document = crate::review::Review::new(comparison.root.clone()); review.document = crate::review::Review::new(comparison.root.clone());
} }
let records = &mut self.local_review.document.reviewed_files; let records = &mut review.document.reviewed_files;
records.retain(|record| { records.retain(|record| {
record.revisions record.revisions
!= *file != *file
@@ -2057,21 +2058,20 @@ impl DiffState {
.then(|| file.after.join("\n")), .then(|| file.after.join("\n")),
}); });
} }
self.local_review.dirty = true; review.dirty = true;
} }
Ok(reviewed) Ok(reviewed)
} }
pub(crate) fn restore_reviewed_files(&mut self) { pub(crate) fn restore_reviewed_files(&mut self, review: &crate::review::ReviewState) {
self.reviewed_files.clear(); self.reviewed_files.clear();
let Some(comparison) = self.comparison.as_ref() else { let Some(comparison) = self.comparison.as_ref() else {
return; return;
}; };
if self.local_review.document.root != comparison.root { if review.document.root != comparison.root {
return; return;
} }
let records: BTreeMap<_, _> = self let records: BTreeMap<_, _> = review
.local_review
.document .document
.reviewed_files .reviewed_files
.iter() .iter()
@@ -2369,12 +2369,13 @@ impl DiffState {
pub fn review_anchor_range( pub fn review_anchor_range(
&self, &self,
anchor: &crate::review::Anchor, anchor: &crate::review::Anchor,
review: &crate::review::ReviewState,
) -> Option<std::ops::Range<crate::plugin::TextPosition>> { ) -> Option<std::ops::Range<crate::plugin::TextPosition>> {
use crate::review::Side; use crate::review::Side;
let Some(comparison) = &self.comparison else { let Some(comparison) = &self.comparison else {
return None; return None;
}; };
if self.local_review.document.root != comparison.root { if review.document.root != comparison.root {
return None; return None;
} }
comparison comparison
@@ -2481,7 +2482,12 @@ impl DiffState {
} }
} }
pub fn refresh_after(&mut self, lines: &[String], revision: u64) { pub fn refresh_after(
&mut self,
lines: &[String],
revision: u64,
review: &mut crate::review::ReviewState,
) {
if !self.live_after_available() { if !self.live_after_available() {
return; return;
} }
@@ -2510,13 +2516,9 @@ impl DiffState {
self.modified_after.insert(self.file); self.modified_after.insert(self.file);
} }
let comparison = self.comparison.as_ref().unwrap(); let comparison = self.comparison.as_ref().unwrap();
let record = (self.local_review.document.root == comparison.root) let record = (review.document.root == comparison.root)
.then(|| { .then(|| {
self.local_review review.document.reviewed_files.iter().position(|record| {
.document
.reviewed_files
.iter()
.position(|record| {
record.revisions record.revisions
== *file == *file
.review_revisions .review_revisions
@@ -2528,11 +2530,7 @@ impl DiffState {
}) })
.flatten(); .flatten();
let matches = record let matches = record
.and_then(|position| { .and_then(|position| review.document.reviewed_files[position].after.as_ref())
self.local_review.document.reviewed_files[position]
.after
.as_ref()
})
.map(|text| text.split('\n').eq(lines.iter().map(String::as_str))); .map(|text| text.split('\n').eq(lines.iter().map(String::as_str)));
if matches == Some(true) { if matches == Some(true) {
self.reviewed_files.insert(self.file); self.reviewed_files.insert(self.file);
@@ -2540,8 +2538,8 @@ impl DiffState {
if self.reviewed_files.contains(&self.file) && !matches.unwrap_or(unchanged) { if self.reviewed_files.contains(&self.file) && !matches.unwrap_or(unchanged) {
self.reviewed_files.remove(&self.file); self.reviewed_files.remove(&self.file);
if let Some(position) = record { if let Some(position) = record {
self.local_review.document.reviewed_files.remove(position); review.document.reviewed_files.remove(position);
self.local_review.dirty = true; review.dirty = true;
} }
} }
if was_live && changed_lines.0 == changed_lines.1 && changed_lines.1 == changed_lines.2 { if was_live && changed_lines.0 == changed_lines.1 && changed_lines.1 == changed_lines.2 {
+89 -49
View File
@@ -990,96 +990,91 @@ core_actions![
true, true,
), ),
( (
"core.diff.comment", "core.comment",
Action::DiffComment, Action::Comment,
&[Mode::Normal, Mode::Visual], &[Mode::Normal, Mode::Visual],
true true
), ),
( (
"core.diff.review.edit", "core.review.edit",
Action::ReviewEdit, Action::ReviewEdit,
&[Mode::Normal], &[Mode::Normal],
true, true,
true true
), ),
( (
"core.diff.review.delete", "core.review.delete",
Action::ReviewDelete, Action::ReviewDelete,
&[Mode::Normal], &[Mode::Normal],
true, true,
true true
), ),
( (
"core.diff.review.resolve", "core.review.resolve",
Action::ReviewResolve, Action::ReviewResolve,
&[Mode::Normal], &[Mode::Normal],
true, true,
true true
), ),
( (
"core.diff.review.unresolve", "core.review.unresolve",
Action::ReviewUnresolve, Action::ReviewUnresolve,
&[Mode::Normal], &[Mode::Normal],
true, true,
true true
), ),
( (
"core.diff.review.next", "core.review.next",
Action::ReviewNext, Action::ReviewNext,
&[Mode::Normal], &[Mode::Normal],
true true
), ),
( (
"core.diff.review.previous", "core.review.previous",
Action::ReviewPrevious, Action::ReviewPrevious,
&[Mode::Normal], &[Mode::Normal],
true true
), ),
( (
"core.diff.review.overview", "core.review.overview",
Action::ReviewOverview, Action::ReviewOverview,
&[Mode::Normal], &[Mode::Normal],
true true
), ),
( (
"core.diff.review.copy", "core.review.copy",
Action::ReviewCopy, Action::ReviewCopy,
&[Mode::Normal], &[Mode::Normal],
true true
), ),
( (
"core.diff.review.summary", "core.review.summary",
Action::ReviewSummary, Action::ReviewSummary,
&[Mode::Normal], &[Mode::Normal],
true true
), ),
( (
"core.diff.review.verdict", "core.review.verdict",
Action::ReviewVerdict, Action::ReviewVerdict,
&[Mode::Normal], &[Mode::Normal],
true, true,
true true
), ),
( (
"core.diff.review.save", "core.review.save",
Action::ReviewSave, Action::ReviewSave,
&[Mode::Normal], &[Mode::Normal],
true, true,
true true
), ),
( (
"core.diff.review.open", "core.review.open",
Action::ReviewOpen, Action::ReviewOpen,
&[Mode::Normal], &[Mode::Normal],
true, true,
true true
), ),
( ("core.review.new", Action::ReviewNew, &[Mode::Normal], true),
"core.diff.review.new",
Action::ReviewNew,
&[Mode::Normal],
true
),
( (
"core.word_wrap.toggle", "core.word_wrap.toggle",
Action::WordWrapToggle, Action::WordWrapToggle,
@@ -1193,6 +1188,20 @@ const COMMAND_ALIASES: &[(&str, Action)] = &[
("bprev", Action::BufferPrevious), ("bprev", Action::BufferPrevious),
("bdelete", Action::BufferDelete), ("bdelete", Action::BufferDelete),
("bdelete!", Action::BufferForceDelete), ("bdelete!", Action::BufferForceDelete),
("diff.comment", Action::Comment),
("diff.review.edit", Action::ReviewEdit),
("diff.review.delete", Action::ReviewDelete),
("diff.review.resolve", Action::ReviewResolve),
("diff.review.unresolve", Action::ReviewUnresolve),
("diff.review.next", Action::ReviewNext),
("diff.review.previous", Action::ReviewPrevious),
("diff.review.overview", Action::ReviewOverview),
("diff.review.copy", Action::ReviewCopy),
("diff.review.summary", Action::ReviewSummary),
("diff.review.verdict", Action::ReviewVerdict),
("diff.review.save", Action::ReviewSave),
("diff.review.open", Action::ReviewOpen),
("diff.review.new", Action::ReviewNew),
]; ];
fn core_command(name: &str) -> Option<&'static CoreAction> { fn core_command(name: &str) -> Option<&'static CoreAction> {
@@ -1893,6 +1902,7 @@ struct Tab {
pane_id: usize, pane_id: usize,
pane_layout: PaneTree, pane_layout: PaneTree,
review_pane: Option<usize>, review_pane: Option<usize>,
local_review: crate::review::ReviewState,
file_picker: Option<FilePicker>, file_picker: Option<FilePicker>,
diff: Option<DiffState>, diff: Option<DiffState>,
parked_diff: Option<DiffState>, parked_diff: Option<DiffState>,
@@ -2200,6 +2210,7 @@ pub struct Editor {
panes: BTreeMap<usize, Pane>, panes: BTreeMap<usize, Pane>,
pane_layout: PaneTree, pane_layout: PaneTree,
review_pane: Option<usize>, review_pane: Option<usize>,
pub(crate) local_review: crate::review::ReviewState,
tab_id: usize, tab_id: usize,
next_tab_id: usize, next_tab_id: usize,
tab_order: Vec<usize>, tab_order: Vec<usize>,
@@ -2313,6 +2324,7 @@ impl Editor {
)]), )]),
pane_layout: PaneTree::new(1), pane_layout: PaneTree::new(1),
review_pane: None, review_pane: None,
local_review: Default::default(),
tab_id: 1, tab_id: 1,
next_tab_id: 2, next_tab_id: 2,
tab_order: vec![1], tab_order: vec![1],
@@ -3663,6 +3675,7 @@ impl Editor {
pane_id: std::mem::replace(&mut self.pane_id, next.pane_id), pane_id: std::mem::replace(&mut self.pane_id, next.pane_id),
pane_layout: std::mem::replace(&mut self.pane_layout, next.pane_layout), pane_layout: std::mem::replace(&mut self.pane_layout, next.pane_layout),
review_pane: std::mem::replace(&mut self.review_pane, next.review_pane), review_pane: std::mem::replace(&mut self.review_pane, next.review_pane),
local_review: std::mem::replace(&mut self.local_review, next.local_review),
file_picker: std::mem::replace(&mut self.file_picker, next.file_picker), file_picker: std::mem::replace(&mut self.file_picker, next.file_picker),
diff: std::mem::replace(&mut self.diff, next.diff), diff: std::mem::replace(&mut self.diff, next.diff),
parked_diff: std::mem::replace(&mut self.parked_diff, next.parked_diff), parked_diff: std::mem::replace(&mut self.parked_diff, next.parked_diff),
@@ -3712,6 +3725,7 @@ impl Editor {
pane_id, pane_id,
pane_layout: PaneTree::new(pane_id), pane_layout: PaneTree::new(pane_id),
review_pane: None, review_pane: None,
local_review: Default::default(),
file_picker: None, file_picker: None,
diff: None, diff: None,
parked_diff: None, parked_diff: None,
@@ -3814,11 +3828,6 @@ impl Editor {
// No input dispatch or document copies: source documents stay unique in active/parked. // No input dispatch or document copies: source documents stay unique in active/parked.
fn select_pane(&mut self, id: usize) { fn select_pane(&mut self, id: usize) {
let id = if self.review_pane == Some(id) && self.workspace_diff().is_none() {
self.panes[&id].origin.unwrap_or(id)
} else {
id
};
if id == self.pane_id { if id == self.pane_id {
return; return;
} }
@@ -4223,7 +4232,12 @@ impl Editor {
return; return;
} }
self.finish_pane_interaction(); self.finish_pane_interaction();
if self.diff_owner == Some(id) && !force && self.local_review_close_blocked() { let review_source =
self.review_pane.and_then(|panel| self.panes[&panel].origin) == Some(id);
if (self.diff_owner == Some(id) || review_source)
&& !force
&& self.local_review_close_blocked()
{
return; return;
} }
if self.diff_owner == Some(id) { if self.diff_owner == Some(id) {
@@ -4236,6 +4250,9 @@ impl Editor {
} }
let next = self.pane_layout.close(id).expect("another pane exists"); let next = self.pane_layout.close(id).expect("another pane exists");
self.select_pane(next); self.select_pane(next);
if review_source && let Some(panel) = self.review_pane {
self.panes.get_mut(&panel).unwrap().origin = Some(next);
}
self.panes.remove(&id); self.panes.remove(&id);
for buffer in buffers { for buffer in buffers {
self.forget_buffer(buffer); self.forget_buffer(buffer);
@@ -4627,11 +4644,17 @@ impl Editor {
view.clamp(&self.active.lines); view.clamp(&self.active.lines);
self.active.view = view; self.active.view = view;
} }
if let Err(error) = self.initialize_local_review() { if self.review_directory.is_some()
&& self.resolver().is_none()
&& self
.workspace_diff()
.is_some_and(|diff| diff.phase == Phase::View)
&& let Err(error) = self.initialize_local_review()
{
self.set_message(format!("Local review: {error}")); self.set_message(format!("Local review: {error}"));
} }
if restore_reviewed && let Some(diff) = &mut self.diff { if restore_reviewed && let Some(diff) = &mut self.diff {
diff.restore_reviewed_files(); diff.restore_reviewed_files(&self.local_review);
} }
let reviewed = self.diff.as_mut().and_then(|diff| { let reviewed = self.diff.as_mut().and_then(|diff| {
(refresh (refresh
@@ -5190,16 +5213,19 @@ impl Editor {
pub(crate) fn refresh_live_diff(&mut self) { pub(crate) fn refresh_live_diff(&mut self) {
self.refresh_resolver_comparison(); self.refresh_resolver_comparison();
for diff in self for (diff, parked, review) in std::iter::once((
.diff &mut self.diff,
.iter_mut() &mut self.parked_diff,
.chain(self.parked_diff.iter_mut()) &mut self.local_review,
))
.chain( .chain(
self.tabs self.tabs
.values_mut() .values_mut()
.flat_map(|tab| tab.diff.iter_mut().chain(tab.parked_diff.iter_mut())), .map(|tab| (&mut tab.diff, &mut tab.parked_diff, &mut tab.local_review)),
) ) {
{ let Some(diff) = diff.as_mut().or(parked.as_mut()) else {
continue;
};
let buffer = diff.live_buffer.and_then(|id| { let buffer = diff.live_buffer.and_then(|id| {
if id == self.active.id { if id == self.active.id {
Some(&self.active) Some(&self.active)
@@ -5211,7 +5237,7 @@ impl Editor {
&& diff.live_after_available() && diff.live_after_available()
&& diff.live_revision != Some(buffer.text_revision) && diff.live_revision != Some(buffer.text_revision)
{ {
diff.refresh_after(&buffer.lines, buffer.text_revision); diff.refresh_after(&buffer.lines, buffer.text_revision, review);
diff.follow_caret = true; diff.follow_caret = true;
} }
} }
@@ -5485,7 +5511,7 @@ impl Editor {
| Action::DiffViewToggle | Action::DiffViewToggle
| Action::DiffOrientationToggle | Action::DiffOrientationToggle
| Action::DiffReviewToggle | Action::DiffReviewToggle
| Action::DiffComment | Action::Comment
| Action::ReviewEdit | Action::ReviewEdit
| Action::ReviewDelete | Action::ReviewDelete
| Action::ReviewResolve | Action::ReviewResolve
@@ -8917,7 +8943,7 @@ impl Editor {
| Action::DiffViewToggle | Action::DiffViewToggle
| Action::DiffOrientationToggle | Action::DiffOrientationToggle
| Action::DiffReviewToggle | Action::DiffReviewToggle
| Action::DiffComment | Action::Comment
| Action::ReviewEdit | Action::ReviewEdit
| Action::ReviewDelete | Action::ReviewDelete
| Action::ReviewResolve | Action::ReviewResolve
@@ -9559,8 +9585,7 @@ impl Editor {
.map(|buffer| buffer.id); .map(|buffer| buffer.id);
if let Some(id) = loaded { if let Some(id) = loaded {
self.switch_buffer(id); self.switch_buffer(id);
return Ok(()); } else {
}
let buffer = Buffer::open( let buffer = Buffer::open(
self.next_buffer_id, self.next_buffer_id,
Some(path), Some(path),
@@ -9568,6 +9593,14 @@ impl Editor {
allow_missing, allow_missing,
)?; )?;
self.add_buffer(buffer); self.add_buffer(buffer);
}
if self.review_directory.is_some()
&& self.workspace_diff().is_none()
&& self.resolver().is_none()
&& let Err(error) = self.initialize_local_review()
{
self.set_message(format!("Local review: {error}"));
}
Ok(()) Ok(())
} }
@@ -9745,7 +9778,7 @@ impl Editor {
fn restart(&mut self, force: bool) { fn restart(&mut self, force: bool) {
if !force && self.local_reviews_dirty() { if !force && self.local_reviews_dirty() {
self.set_message("Unsaved local review; use :diff.review.save <path>"); self.set_message("Unsaved local review; use :review.save <path>");
return; return;
} }
if force || !self.any_dirty() { if force || !self.any_dirty() {
@@ -9876,7 +9909,7 @@ impl Editor {
} }
if matches!( if matches!(
entry.action, entry.action,
Action::DiffComment Action::Comment
| Action::ReviewEdit | Action::ReviewEdit
| Action::ReviewDelete | Action::ReviewDelete
| Action::ReviewResolve | Action::ReviewResolve
@@ -10102,15 +10135,15 @@ impl Editor {
self.diff_return_buffer = Some(self.active.id); self.diff_return_buffer = Some(self.active.id);
return None; return None;
} }
let Some(diff) = self if entry.action == Action::DiffReviewToggle {
let mut review = std::mem::take(&mut self.local_review);
let result = self
.picker_diff_mut() .picker_diff_mut()
.filter(|diff| diff.phase == Phase::View) .filter(|diff| diff.phase == Phase::View)
else { .ok_or("No comparison is open")
self.set_message("No comparison is open"); .and_then(|diff| diff.toggle_reviewed(&mut review));
return None; self.local_review = review;
}; match result {
if entry.action == Action::DiffReviewToggle {
match diff.toggle_reviewed() {
Ok(reviewed) => { Ok(reviewed) => {
if self.resolver().is_none() if self.resolver().is_none()
&& let Err(error) = self.autosave_local_review() && let Err(error) = self.autosave_local_review()
@@ -10128,6 +10161,13 @@ impl Editor {
} }
return None; return None;
} }
let Some(diff) = self
.picker_diff_mut()
.filter(|diff| diff.phase == Phase::View)
else {
self.set_message("No comparison is open");
return None;
};
if entry.action == Action::DiffViewToggle { if entry.action == Action::DiffViewToggle {
diff.set_view_mode(match diff.view_mode { diff.set_view_mode(match diff.view_mode {
ViewMode::Split => ViewMode::Unified, ViewMode::Split => ViewMode::Unified,
+268 -174
View File
@@ -25,58 +25,81 @@ pub(super) struct ReviewBuffer {
impl Editor { impl Editor {
pub fn set_review_directory(&mut self, directory: PathBuf) { pub fn set_review_directory(&mut self, directory: PathBuf) {
self.review_directory = Some(directory); self.review_directory = Some(directory);
if self.review_root().is_ok()
&& let Err(error) = self.initialize_local_review()
{
self.set_message(format!("Local review: {error}"));
}
} }
pub(super) fn initialize_local_review(&mut self) -> Result<(), String> { pub(super) fn initialize_local_review(&mut self) -> Result<(), String> {
let Some(directory) = self.review_directory.as_ref() else { let root = self.review_root()?;
return Ok(()); if self.local_review.document.root == root {
}; self.local_review.initialized = true;
let Some(diff) = self
.workspace_diff()
.filter(|diff| diff.phase == Phase::View)
else {
return Ok(());
};
if diff.local_review.initialized {
return Ok(()); return Ok(());
} }
let root = diff if self.current_review_dirty() {
.comparison return Err("Save the current review before changing repositories".into());
.as_ref() }
.ok_or("Comparison is loading")? let reviews = match self.review_directory.as_deref() {
.root Some(directory) => crate::review::Store::open_read_only(directory)?
.clone(); .map(|store| store.list(&root))
let directory = directory.clone(); .transpose()?
self.review_diff_mut().unwrap().local_review.initialized = true; .unwrap_or_default(),
self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone()); None => Vec::new(),
let store = crate::review::Store::open(&directory)?; };
let mut reviews = store.list(&root)?; self.detach_review_buffers();
self.local_review = crate::review::ReviewState {
document: Review::new(root),
initialized: true,
..Default::default()
};
let mut reviews = reviews;
if reviews.len() == 1 { if reviews.len() == 1 {
self.review_diff_mut().unwrap().local_review.document = reviews.pop().unwrap(); self.local_review.document = reviews.pop().unwrap();
} else if reviews.len() > 1 { } else if reviews.len() > 1 {
self.show_review_selector(reviews)?; self.show_review_selector(reviews)?;
} }
Ok(()) Ok(())
} }
fn review_root(&self) -> Result<PathBuf, String> {
if let Some(diff) = self.workspace_diff() {
return diff
.comparison
.as_ref()
.map(|comparison| {
fs::canonicalize(&comparison.root).unwrap_or_else(|_| comparison.root.clone())
})
.ok_or_else(|| "Comparison is still loading".into());
}
if self.active.review_buffer.is_some()
|| (self.active.path.is_none() && self.local_review.initialized)
{
return Ok(self.local_review.document.root.clone());
}
let path = self
.active
.identity
.as_deref()
.or(self.active.path.as_deref())
.ok_or("Save the source file before commenting")?;
crate::repository::discover_root(path.parent().ok_or("Source has no parent directory")?)
}
fn open_review_selector(&mut self) -> Result<(), String> { fn open_review_selector(&mut self) -> Result<(), String> {
let diff = self.workspace_diff().ok_or("No comparison is open")?; let root = self.review_root()?;
let directory = self let directory = self
.review_directory .review_directory
.as_deref() .as_deref()
.ok_or("Review storage is not configured")?; .ok_or("Review storage is not configured")?;
let root = &diff let reviews = crate::review::Store::open(directory)?.list(&root)?;
.comparison
.as_ref()
.ok_or("Comparison is loading")?
.root;
let reviews = crate::review::Store::open(directory)?.list(root)?;
self.show_review_selector(reviews) self.show_review_selector(reviews)
} }
fn show_review_selector(&mut self, reviews: Vec<Review>) -> Result<(), String> { fn show_review_selector(&mut self, reviews: Vec<Review>) -> Result<(), String> {
let mut text = String::from( let mut text = String::from(
"# Local reviews\n\nEnter opens the review under the caret.\n:diff.review.new starts a separate review.\n", "# Local reviews\n\nEnter opens the review under the caret.\n:review.new starts a separate review.\n",
); );
for review in reviews { for review in reviews {
let id = review.storage_id.unwrap(); let id = review.storage_id.unwrap();
@@ -95,14 +118,14 @@ impl Editor {
self.open_review_buffer( self.open_review_buffer(
ReviewText::Reviews, ReviewText::Reviews,
&text, &text,
"Local reviews · Enter opens · :diff.review.new".into(), "Local reviews · Enter opens · :review.new".into(),
)?; )?;
self.place_navigation(TextPosition { row, column: 0 }); self.place_navigation(TextPosition { row, column: 0 });
Ok(()) Ok(())
} }
fn mark_review_saved(&mut self) { fn mark_review_saved(&mut self) {
self.review_diff_mut().unwrap().local_review.dirty = false; self.local_review.dirty = false;
for buffer in std::iter::once(&mut self.active).chain(self.parked.values_mut()) { for buffer in std::iter::once(&mut self.active).chain(self.parked.values_mut()) {
if buffer if buffer
.review_buffer .review_buffer
@@ -120,21 +143,12 @@ impl Editor {
let Some(directory) = self.review_directory.as_deref() else { let Some(directory) = self.review_directory.as_deref() else {
return Ok(()); return Ok(());
}; };
if !self if !self.local_review.dirty {
.workspace_diff()
.is_some_and(|diff| diff.local_review.dirty)
{
return Ok(()); return Ok(());
} }
let mut store = crate::review::Store::open(directory)?; let mut store = crate::review::Store::open(directory)?;
store.save( store.save(&mut self.local_review.document)?;
&mut self self.local_review.dirty = false;
.review_diff_mut()
.ok_or("No comparison is open")?
.local_review
.document,
)?;
self.review_diff_mut().unwrap().local_review.dirty = false;
Ok(()) Ok(())
} }
pub fn review_panel_title(&self) -> Option<&str> { pub fn review_panel_title(&self) -> Option<&str> {
@@ -145,8 +159,7 @@ impl Editor {
} }
pub fn review_panel(&self) -> Option<usize> { pub fn review_panel(&self) -> Option<usize> {
self.review_pane self.review_pane.filter(|pane| *pane == self.pane_id)
.filter(|pane| *pane == self.pane_id && self.workspace_diff().is_some())
} }
pub(super) fn dismiss_review_panel(&mut self) -> bool { pub(super) fn dismiss_review_panel(&mut self) -> bool {
@@ -165,7 +178,7 @@ impl Editor {
buffer.view.word_wrap = wrap; buffer.view.word_wrap = wrap;
buffer.resolver_readonly = self.active.resolver_readonly; buffer.resolver_readonly = self.active.resolver_readonly;
if binding.pending_comment.take().is_some() { if binding.pending_comment.take().is_some() {
self.review_diff_mut().unwrap().local_review.selected = None; self.local_review.selected = None;
} }
buffer.review_buffer = Some(binding); buffer.review_buffer = Some(binding);
self.active = buffer; self.active = buffer;
@@ -175,25 +188,16 @@ impl Editor {
true true
} }
fn review_diff_mut(&mut self) -> Option<&mut DiffState> {
self.diff.as_mut().or(self.parked_diff.as_mut())
}
pub(super) fn local_reviews_dirty(&self) -> bool { pub(super) fn local_reviews_dirty(&self) -> bool {
self.workspace_diff() self.local_review.dirty
.is_some_and(|diff| diff.local_review.dirty) || self.tabs.values().any(|tab| tab.local_review.dirty)
|| self
.tabs
.values()
.any(|tab| tab.diff().is_some_and(|diff| diff.local_review.dirty))
|| std::iter::once(&self.active) || std::iter::once(&self.active)
.chain(self.parked.values()) .chain(self.parked.values())
.any(|buffer| buffer.review_buffer.is_some() && buffer.dirty()) .any(|buffer| buffer.review_buffer.is_some() && buffer.dirty())
} }
pub(super) fn current_review_dirty(&self) -> bool { pub(super) fn current_review_dirty(&self) -> bool {
self.workspace_diff() self.local_review.dirty
.is_some_and(|diff| diff.local_review.dirty)
|| std::iter::once(&self.active) || std::iter::once(&self.active)
.chain(self.parked.values()) .chain(self.parked.values())
.any(|buffer| { .any(|buffer| {
@@ -213,7 +217,7 @@ impl Editor {
return true; return true;
} }
if self.current_review_dirty() { if self.current_review_dirty() {
self.set_message("Unsaved local review; use :diff.review.save <path> before closing"); self.set_message("Unsaved local review; use :review.save <path> before closing");
true true
} else { } else {
false false
@@ -258,13 +262,15 @@ impl Editor {
}; };
// Retire old drafts before comment IDs can be reused. // Retire old drafts before comment IDs can be reused.
self.detach_review_buffers(); self.detach_review_buffers();
let state = &mut self.review_diff_mut().unwrap().local_review; let state = &mut self.local_review;
state.document = document; state.document = document;
state.path = path; state.path = path;
state.selected = None; state.selected = None;
state.pending_navigation = None; state.pending_navigation = None;
state.dirty = dirty; state.dirty = dirty;
self.review_diff_mut().unwrap().restore_reviewed_files(); if let Some(diff) = self.diff.as_mut().or(self.parked_diff.as_mut()) {
diff.restore_reviewed_files(&self.local_review);
}
// Persistence failure leaves the identity change applied and unsaved. // Persistence failure leaves the identity change applied and unsaved.
self.autosave_local_review()?; self.autosave_local_review()?;
self.open_review_overview() self.open_review_overview()
@@ -272,7 +278,7 @@ impl Editor {
fn delete_local_review_comment(&mut self, args: &str) -> Result<(), String> { fn delete_local_review_comment(&mut self, args: &str) -> Result<(), String> {
let id = self.review_comment_id(args)?; let id = self.review_comment_id(args)?;
let state = &mut self.review_diff_mut().unwrap().local_review; let state = &mut self.local_review;
state.document.comments.retain(|comment| comment.id != id); state.document.comments.retain(|comment| comment.id != id);
state.selected = None; state.selected = None;
state.pending_navigation = None; state.pending_navigation = None;
@@ -308,14 +314,11 @@ impl Editor {
}) })
}) })
.collect(); .collect();
let Some(diff) = self.review_diff_mut() else { let state = &mut self.local_review;
return Vec::new();
};
let mut previous = Vec::new(); let mut previous = Vec::new();
for (kind, text, pending) in updates { for (kind, text, pending) in updates {
if let Some(mut comment) = pending if let Some(mut comment) = pending
&& !diff && !state
.local_review
.document .document
.comments .comments
.iter() .iter()
@@ -323,19 +326,18 @@ impl Editor {
{ {
previous.push((kind, None)); previous.push((kind, None));
comment.body = text; comment.body = text;
diff.local_review.document.comments.push(comment); state.document.comments.push(comment);
diff.local_review.dirty = true; state.dirty = true;
continue; continue;
} }
let destination = match kind { let destination = match kind {
ReviewText::Comment(id) => diff ReviewText::Comment(id) => state
.local_review
.document .document
.comments .comments
.iter_mut() .iter_mut()
.find(|comment| comment.id == id) .find(|comment| comment.id == id)
.map(|comment| &mut comment.body), .map(|comment| &mut comment.body),
ReviewText::Summary => Some(&mut diff.local_review.document.summary), ReviewText::Summary => Some(&mut state.document.summary),
ReviewText::Overview => None, ReviewText::Overview => None,
ReviewText::Reviews => None, ReviewText::Reviews => None,
}; };
@@ -344,22 +346,18 @@ impl Editor {
{ {
let old = std::mem::replace(destination, text); let old = std::mem::replace(destination, text);
previous.push((kind, Some(old))); previous.push((kind, Some(old)));
diff.local_review.dirty = true; state.dirty = true;
} }
} }
previous previous
} }
pub(super) fn save_local_review(&mut self, path: Option<&Path>) -> Result<(), String> { pub(super) fn save_local_review(&mut self, path: Option<&Path>) -> Result<(), String> {
let dirty = self let dirty = self.local_review.dirty;
.review_diff_mut()
.ok_or("No comparison is open")?
.local_review
.dirty;
let previous = self.sync_review_text(); let previous = self.sync_review_text();
let result = self.write_local_review(path); let result = self.write_local_review(path);
if result.is_err() { if result.is_err() {
let state = &mut self.review_diff_mut().unwrap().local_review; let state = &mut self.local_review;
for (kind, text) in previous { for (kind, text) in previous {
match (kind, text) { match (kind, text) {
(ReviewText::Comment(id), None) => { (ReviewText::Comment(id), None) => {
@@ -386,12 +384,8 @@ impl Editor {
fn write_local_review(&mut self, path: Option<&Path>) -> Result<(), String> { fn write_local_review(&mut self, path: Option<&Path>) -> Result<(), String> {
if self.review_directory.is_some() { if self.review_directory.is_some() {
if let Some(path) = path { if let Some(path) = path {
self.review_diff_mut() self.local_review.document.save(path)?;
.ok_or("No comparison is open")? self.local_review.path = Some(path.to_path_buf());
.local_review
.document
.save(path)?;
self.review_diff_mut().unwrap().local_review.path = Some(path.to_path_buf());
} }
self.autosave_local_review()?; self.autosave_local_review()?;
self.mark_review_saved(); self.mark_review_saved();
@@ -402,14 +396,11 @@ impl Editor {
}); });
return Ok(()); return Ok(());
} }
let state = &mut self let state = &mut self.local_review;
.review_diff_mut()
.ok_or("No comparison is open")?
.local_review;
let path = path let path = path
.map(Path::to_path_buf) .map(Path::to_path_buf)
.or_else(|| state.path.clone()) .or_else(|| state.path.clone())
.ok_or("Use :diff.review.save <path> to choose a local JSON review file")?; .ok_or("Use :review.save <path> to choose a local JSON review file")?;
state.document.save(&path)?; state.document.save(&path)?;
state.path = Some(path); state.path = Some(path);
self.mark_review_saved(); self.mark_review_saved();
@@ -423,19 +414,10 @@ impl Editor {
args: &str, args: &str,
) -> Result<(), String> { ) -> Result<(), String> {
if self.resolver().is_some() { if self.resolver().is_some() {
return Err("Local reviews require an ordinary diff comparison".into()); return Err("Local reviews are unavailable in conflict resolution workspaces".into());
} }
self.initialize_local_review()?; self.initialize_local_review()?;
let diff = self let root = self.review_root()?;
.workspace_diff()
.filter(|diff| diff.phase == Phase::View)
.ok_or("No comparison is open")?;
let root = diff
.comparison
.as_ref()
.ok_or("Comparison is still loading")?
.root
.clone();
if !matches!( if !matches!(
action, action,
Action::ReviewEdit Action::ReviewEdit
@@ -449,16 +431,8 @@ impl Editor {
{ {
return Err("This review command takes no arguments".into()); return Err("This review command takes no arguments".into());
} }
if self if self.local_review.document.root.as_os_str().is_empty() {
.workspace_diff() self.local_review.document = Review::new(root.clone());
.unwrap()
.local_review
.document
.root
.as_os_str()
.is_empty()
{
self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone());
} }
if !matches!(action, Action::ReviewSave | Action::ReviewCopy) { if !matches!(action, Action::ReviewSave | Action::ReviewCopy) {
self.autosave_local_review()?; self.autosave_local_review()?;
@@ -480,7 +454,7 @@ impl Editor {
return self.replace_local_review(root, source); return self.replace_local_review(root, source);
} }
match action { match action {
Action::DiffComment => self.add_review_comment()?, Action::Comment => self.add_review_comment()?,
Action::ReviewNew => self.replace_local_review(root, ReviewSource::New)?, Action::ReviewNew => self.replace_local_review(root, ReviewSource::New)?,
Action::ReviewEdit => { Action::ReviewEdit => {
let id = self.review_comment_id(args)?; let id = self.review_comment_id(args)?;
@@ -489,7 +463,7 @@ impl Editor {
Action::ReviewResolve | Action::ReviewUnresolve => { Action::ReviewResolve | Action::ReviewUnresolve => {
let id = self.review_comment_id(args)?; let id = self.review_comment_id(args)?;
let resolved = action == Action::ReviewResolve; let resolved = action == Action::ReviewResolve;
let state = &mut self.review_diff_mut().unwrap().local_review; let state = &mut self.local_review;
let comment = state let comment = state
.document .document
.comments .comments
@@ -522,7 +496,7 @@ impl Editor {
} }
Action::ReviewDelete => self.delete_local_review_comment(args)?, Action::ReviewDelete => self.delete_local_review_comment(args)?,
Action::ReviewNext | Action::ReviewPrevious => { Action::ReviewNext | Action::ReviewPrevious => {
let state = &self.workspace_diff().unwrap().local_review; let state = &self.local_review;
let comments = &state.document.comments; let comments = &state.document.comments;
if comments.is_empty() { if comments.is_empty() {
return Err("No local review comments".into()); return Err("No local review comments".into());
@@ -540,7 +514,7 @@ impl Editor {
} }
Action::ReviewOverview => self.open_review_overview()?, Action::ReviewOverview => self.open_review_overview()?,
Action::ReviewCopy => { Action::ReviewCopy => {
let review = &self.workspace_diff().unwrap().local_review.document; let review = &self.local_review.document;
let count = review.comments.len(); let count = review.comments.len();
if count == 0 { if count == 0 {
return Err("No local review comments".into()); return Err("No local review comments".into());
@@ -554,13 +528,7 @@ impl Editor {
self.set_message(format!("Copied {count} review comments")); self.set_message(format!("Copied {count} review comments"));
} }
Action::ReviewSummary => { Action::ReviewSummary => {
let text = self let text = self.local_review.document.summary.clone();
.workspace_diff()
.unwrap()
.local_review
.document
.summary
.clone();
self.open_review_buffer( self.open_review_buffer(
ReviewText::Summary, ReviewText::Summary,
&text, &text,
@@ -574,11 +542,11 @@ impl Editor {
"none" => None, "none" => None,
_ => { _ => {
return Err( return Err(
"Usage: diff.review.verdict <looks_good|changes_requested|none>".into(), "Usage: review.verdict <looks_good|changes_requested|none>".into()
); );
} }
}; };
let state = &mut self.review_diff_mut().unwrap().local_review; let state = &mut self.local_review;
if state.document.verdict != verdict { if state.document.verdict != verdict {
state.document.verdict = verdict; state.document.verdict = verdict;
state.dirty = true; state.dirty = true;
@@ -595,10 +563,7 @@ impl Editor {
} }
fn review_comment_id(&self, args: &str) -> Result<u64, String> { fn review_comment_id(&self, args: &str) -> Result<u64, String> {
let state = &self let state = &self.local_review;
.workspace_diff()
.ok_or("No comparison is open")?
.local_review;
let under_cursor = self let under_cursor = self
.active .active
.review_buffer .review_buffer
@@ -633,16 +598,65 @@ impl Editor {
.ok_or_else(|| format!("No local comment #{id}")) .ok_or_else(|| format!("No local comment #{id}"))
} }
pub(crate) fn source_review_anchor_range(
&self,
anchor: &Anchor,
) -> Option<std::ops::Range<TextPosition>> {
if self.active.review_buffer.is_some() || !anchor.revision.starts_with("working:") {
return None;
}
let path = self
.active
.identity
.as_ref()
.or(self.active.path.as_ref())?;
let relative = path.strip_prefix(&self.local_review.document.root).ok()?;
anchor.mapped_range(
&anchor.revision,
relative,
Side::After,
&self.active.lines,
false,
)
}
fn review_anchor_range(&self, anchor: &Anchor) -> Option<std::ops::Range<TextPosition>> {
if let Some(diff) = self.workspace_diff() {
return diff.review_anchor_range(anchor, &self.local_review);
}
let path = self.local_review.document.root.join(&anchor.path);
let buffer = std::iter::once(&self.active)
.chain(self.parked.values())
.find(|buffer| {
buffer.identity.as_ref() == Some(&path) && buffer.review_buffer.is_none()
})?;
if !anchor.revision.starts_with("working:") {
return None;
}
anchor.mapped_range(
&anchor.revision,
&anchor.path,
Side::After,
&buffer.lines,
false,
)
}
fn add_review_comment(&mut self) -> Result<(), String> { fn add_review_comment(&mut self) -> Result<(), String> {
let owner = self.diff_owner.ok_or("No comparison is open")?; if let Some(owner) = self.diff_owner {
if self.pane_id != owner { if self.pane_id != owner {
self.select_pane(owner); self.select_pane(owner);
} }
self.bind_diff_buffer(); self.bind_diff_buffer();
let focus = self if self
.diff_buffer_focus() .diff_buffer_focus()
.filter(|_| self.diff_interacting()) .filter(|_| self.diff_interacting())
.ok_or("Focus a diff text pane to add a comment")?; .is_none()
{
return Err("Focus a diff text pane to add a comment".into());
}
} else if self.active.review_buffer.is_some() || self.active.resolver_readonly {
return Err("Focus a source text pane to add a comment".into());
}
if self.selection_count() != 1 { if self.selection_count() != 1 {
return Err("Use one caret or one contiguous selection for a review comment".into()); return Err("Use one caret or one contiguous selection for a review comment".into());
} }
@@ -663,23 +677,46 @@ impl Editor {
}, },
} }
}); });
let diff = self.diff.as_ref().unwrap(); let content = self.active.lines.join("\n");
let comparison = diff.comparison.as_ref().unwrap(); let (revision, path, side) = if let Some(diff) = self.diff.as_ref() {
let comparison = diff.comparison.as_ref().ok_or("Comparison is loading")?;
let file = &comparison.files[diff.file]; let file = &comparison.files[diff.file];
let side = if focus == Focus::Before { let side = if self.diff_buffer_focus() == Some(Focus::Before) {
Side::Before Side::Before
} else { } else {
Side::After Side::After
}; };
let anchor = Anchor { (
revision: comparison.revisions[side.index()].clone(), comparison.revisions[side.index()].clone(),
path: if side == Side::Before { if side == Side::Before {
file.old_path.as_ref().unwrap_or(&file.path) file.old_path.as_ref().unwrap_or(&file.path)
} else { } else {
&file.path &file.path
} }
.clone(), .clone(),
side, side,
)
} else {
let root = &self.local_review.document.root;
let path = self
.active
.identity
.as_ref()
.or(self.active.path.as_ref())
.ok_or("Save the source file before commenting")?
.strip_prefix(root)
.map_err(|_| "Source is outside the review repository")?
.to_path_buf();
(
crate::repository::source_snapshot_revision(root, &content)?,
path,
Side::After,
)
};
let anchor = Anchor {
revision,
path,
side,
start: TextPosition { start: TextPosition {
row: range.start.row, row: range.start.row,
column: range.start.column, column: range.start.column,
@@ -688,9 +725,9 @@ impl Editor {
row: range.end.row, row: range.end.row,
column: range.end.column, column: range.end.column,
}, },
content: self.active.lines.join("\n"), content,
}; };
let id = diff let id = self
.local_review .local_review
.document .document
.comments .comments
@@ -727,10 +764,10 @@ impl Editor {
} }
fn open_review_comment(&mut self, id: u64, draft: Option<&Comment>) -> Result<(), String> { fn open_review_comment(&mut self, id: u64, draft: Option<&Comment>) -> Result<(), String> {
let diff = self.workspace_diff().unwrap(); let state = &self.local_review;
let comment = draft let comment = draft
.or_else(|| { .or_else(|| {
diff.local_review state
.document .document
.comments .comments
.iter() .iter()
@@ -744,7 +781,7 @@ impl Editor {
} else { } else {
"unresolved" "unresolved"
}; };
let mapped = diff.review_anchor_range(anchor); let mapped = self.review_anchor_range(anchor);
let current = mapped.is_some(); let current = mapped.is_some();
let location = if current { let location = if current {
"current" "current"
@@ -761,31 +798,80 @@ impl Editor {
range.end.column + 1, range.end.column + 1,
anchor.side anchor.side
); );
let target = diff.comparison.as_ref().and_then(|comparison| { let anchor = anchor.clone();
let mut stale = !current;
self.local_review.selected = Some(id);
if let Some(owner) = self.diff_owner {
let target = self
.workspace_diff()
.and_then(|diff| diff.comparison.as_ref())
.and_then(|comparison| {
comparison.files.iter().position(|file| { comparison.files.iter().position(|file| {
let path = if anchor.side == Side::Before { if anchor.side == Side::Before {
file.old_path.as_ref().unwrap_or(&file.path) file.old_path.as_ref().unwrap_or(&file.path) == &anchor.path
} else { } else {
&file.path file.path == anchor.path
}; }
path == &anchor.path
}) })
}); });
let stale = !current stale = !current
&& target.is_none_or(|index| diff.comparison.as_ref().unwrap().files[index].loaded); && target.is_none_or(|index| {
let start = anchor.start; self.workspace_diff()
let owner = self.diff_owner.unwrap(); .unwrap()
.comparison
.as_ref()
.unwrap()
.files[index]
.loaded
});
self.select_pane(owner); self.select_pane(owner);
let diff = self.diff.as_mut().unwrap();
diff.local_review.selected = Some(id);
if let Some(index) = target { if let Some(index) = target {
diff.select_file_index(index); self.diff.as_mut().unwrap().select_file_index(index);
diff.local_review.pending_navigation = Some(id); self.local_review.pending_navigation = Some(id);
} }
self.finish_review_navigation(); self.finish_review_navigation();
if draft.is_some() { if draft.is_some() {
self.place_navigation(start); self.place_navigation(anchor.start);
} }
} else {
if let Some(panel) = self.review_panel() {
self.select_pane(self.panes[&panel].origin.unwrap());
}
let path = self.local_review.document.root.join(&anchor.path);
if draft.is_none() {
let opened = self.edit_buffer(path.clone(), false).is_ok();
let mapped = opened.then(|| self.review_anchor_range(&anchor)).flatten();
stale = mapped.is_none();
if let Some(range) = mapped {
self.place_navigation(range.start);
} else {
let mut snapshot = Buffer::from_text(
self.next_buffer_id,
Some(path),
None,
&anchor.content,
None,
)
.map_err(|error| error.to_string())?;
snapshot.resolver_readonly = true;
self.add_buffer(snapshot);
self.place_navigation(anchor.start);
}
}
}
let title = title.replacen(
if stale {
" · current · "
} else {
" · original snapshot · "
},
if stale {
" · original snapshot · "
} else {
" · current · "
},
1,
);
self.open_review_buffer(ReviewText::Comment(id), &text, title)?; self.open_review_buffer(ReviewText::Comment(id), &text, title)?;
if stale { if stale {
self.set_message("Comment is stale; original snapshot anchor preserved"); self.set_message("Comment is stale; original snapshot anchor preserved");
@@ -794,18 +880,18 @@ impl Editor {
} }
pub(super) fn finish_review_navigation(&mut self) { pub(super) fn finish_review_navigation(&mut self) {
let state = &mut self.local_review;
let Some(diff) = self.diff.as_mut() else { let Some(diff) = self.diff.as_mut() else {
return; return;
}; };
let Some(id) = diff.local_review.pending_navigation else { let Some(id) = state.pending_navigation else {
return; return;
}; };
if !diff.selected_file_loaded() { if !diff.selected_file_loaded() {
return; return;
} }
diff.local_review.pending_navigation = None; state.pending_navigation = None;
let Some(comment) = diff let Some(comment) = state
.local_review
.document .document
.comments .comments
.iter() .iter()
@@ -827,7 +913,7 @@ impl Editor {
if selected_path != Some(&comment.anchor.path) { if selected_path != Some(&comment.anchor.path) {
return; return;
} }
let Some(range) = diff.review_anchor_range(&comment.anchor) else { let Some(range) = diff.review_anchor_range(&comment.anchor, state) else {
self.set_message("Comment is stale; original snapshot anchor preserved"); self.set_message("Comment is stale; original snapshot anchor preserved");
return; return;
}; };
@@ -844,8 +930,7 @@ impl Editor {
} }
fn open_review_overview(&mut self) -> Result<(), String> { fn open_review_overview(&mut self) -> Result<(), String> {
let diff = self.workspace_diff().unwrap(); let state = &self.local_review;
let state = &diff.local_review;
let verdict = match state.document.verdict { let verdict = match state.document.verdict {
Some(Verdict::LooksGood) => "Looks good", Some(Verdict::LooksGood) => "Looks good",
Some(Verdict::ChangesRequested) => "Changes requested", Some(Verdict::ChangesRequested) => "Changes requested",
@@ -868,16 +953,19 @@ impl Editor {
.map_or_else(|| "not saved".into(), |path| path.display().to_string()) .map_or_else(|| "not saved".into(), |path| path.display().to_string())
}; };
let mut text = format!( let mut text = format!(
"# Local review\n\nVerdict: {verdict}\nStorage: {storage}\n\n## Summary\n{}\n\nEnter edits the comment under the caret.\n:diff.review.summary · :diff.review.verdict · :diff.review.save\n", "# Local review\n\nVerdict: {verdict}\nStorage: {storage}\n\n## Summary\n{}\n\nEnter edits the comment under the caret.\n:review.summary · :review.verdict · :review.save\n",
state.document.summary state.document.summary
); );
for comment in &state.document.comments { for comment in &state.document.comments {
let anchor = &comment.anchor; let anchor = &comment.anchor;
let mapped = diff.review_anchor_range(anchor); let mapped = self.review_anchor_range(anchor);
let range = mapped.clone().unwrap_or(anchor.start..anchor.end); let range = mapped.clone().unwrap_or(anchor.start..anchor.end);
let status = if mapped.is_some() { let status = if mapped.is_some() {
"current" "current"
} else if diff.comparison.as_ref().is_some_and(|comparison| { } else if self
.workspace_diff()
.and_then(|diff| diff.comparison.as_ref())
.is_some_and(|comparison| {
comparison.files.iter().any(|file| { comparison.files.iter().any(|file| {
!file.loaded !file.loaded
&& if anchor.side == Side::Before { && if anchor.side == Side::Before {
@@ -886,7 +974,8 @@ impl Editor {
file.path == anchor.path file.path == anchor.path
} }
}) })
}) { })
{
"not checked (file not loaded)" "not checked (file not loaded)"
} else { } else {
"stale" "stale"
@@ -915,7 +1004,7 @@ impl Editor {
self.open_review_buffer( self.open_review_buffer(
ReviewText::Overview, ReviewText::Overview,
&text, &text,
"Local review overview · Enter edits · :diff.review.next".into(), "Local review overview · Enter edits · :review.next".into(),
) )
} }
@@ -927,6 +1016,11 @@ impl Editor {
) -> Result<(), String> { ) -> Result<(), String> {
self.prepare_command(); self.prepare_command();
self.finish_command_mode(); self.finish_command_mode();
let source = if self.review_panel().is_some() {
self.panes[&self.pane_id].origin.unwrap()
} else {
self.pane_id
};
let existing = std::iter::once(&self.active) let existing = std::iter::once(&self.active)
.chain(self.parked.values()) .chain(self.parked.values())
.find(|buffer| { .find(|buffer| {
@@ -945,14 +1039,14 @@ impl Editor {
id, id,
Pane { Pane {
buffer_id: self.active.id, buffer_id: self.active.id,
origin: self.diff_owner, origin: Some(source),
views: BTreeMap::from([(self.active.id, self.active.view.inherit())]), views: BTreeMap::from([(self.active.id, self.active.view.inherit())]),
}, },
); );
self.review_pane = Some(id); self.review_pane = Some(id);
id id
}; };
self.panes.get_mut(&panel).unwrap().origin = self.diff_owner; self.panes.get_mut(&panel).unwrap().origin = Some(source);
self.select_pane(panel); self.select_pane(panel);
if let Some(id) = existing { if let Some(id) = existing {
self.switch_buffer(id); self.switch_buffer(id);
@@ -978,9 +1072,9 @@ impl Editor {
self.add_buffer(buffer); self.add_buffer(buffer);
} }
self.set_message(if self.review_directory.is_some() { self.set_message(if self.review_directory.is_some() {
"Local review · :w saves · Escape discards unsaved text · :diff.review.save path exports JSON" "Local review · :w saves · Escape discards unsaved text · :review.save path exports JSON"
} else { } else {
"Local review · :w saves JSON; first choose :diff.review.save <path>" "Local review · :w saves JSON; first choose :review.save <path>"
}); });
Ok(()) Ok(())
} }
+7 -5
View File
@@ -96,13 +96,15 @@ Diff: :diff (Normal); choose Before then After; Working checkout After edits liv
Esc closes Before/After selectors; :diff.close exits an opened comparison explicitly Esc closes Before/After selectors; :diff.close exits an opened comparison explicitly
Remote refs/history load into temporary comparison repos without changing active refs/checkout Remote refs/history load into temporary comparison repos without changing active refs/checkout
Changed files: normal picker/explorer bindings, Ctrl+w close; A/M/D/R status and rename paths Changed files: normal picker/explorer bindings, Ctrl+w close; A/M/D/R status and rename paths
Local reviews: :diff.comment anchors exact Visual text, Visual Line rows, or the current line Local reviews: :comment anchors exact Visual text, Visual Line rows, or the current line
Works in ordinary repository source buffers and diff text panes
Selected comments highlight their precise text Selected comments highlight their precise text
:diff.review.edit/delete [ID], :diff.review.next/previous, :diff.review.overview :review.edit/delete [ID], :review.next/previous, :review.overview
:diff.review.summary; :diff.review.verdict looks_good|changes_requested|none :review.summary; :review.verdict looks_good|changes_requested|none
:diff.review.copy copies all saved comments in CLI format using the configured clipboard :review.copy copies all saved comments in CLI format using the configured clipboard
:w saves comment/summary text, including hidden drafts; verdicts/deletions commit immediately :w saves comment/summary text, including hidden drafts; verdicts/deletions commit immediately
:diff.review.save PATH exports JSON; :diff.review.open [ID|PATH] selects/imports; :diff.review.new :review.save PATH exports JSON; :review.open [ID|PATH] selects/imports; :review.new
Existing :diff.comment and :diff.review.* spellings remain available
Normal Esc or :pane.close discards unsaved text and dismisses the floating editor Normal Esc or :pane.close discards unsaved text and dismisses the floating editor
Snapshot anchors stay immutable; stale comments lose gutter markers; no publishing Snapshot anchors stay immutable; stale comments lose gutter markers; no publishing
Pull requests: :pull_requests [REMOTE]; GitHub/GitLab/Gitea Open/Closed/Drafts/All tabs, type to search Pull requests: :pull_requests [REMOTE]; GitHub/GitLab/Gitea Open/Closed/Drafts/All tabs, type to search
+18
View File
@@ -1093,6 +1093,24 @@ pub fn discover_root(path: &Path) -> Result<PathBuf, String> {
discover_root_with_cancellation(path, &AtomicBool::new(false)) discover_root_with_cancellation(path, &AtomicBool::new(false))
} }
/// Identify the exact source bytes without writing objects or snapshotting the checkout.
pub(crate) fn source_snapshot_revision(root: &Path, content: &str) -> Result<String, String> {
let mut command = if root.join(".jj").exists() {
git_command(&store_for_jj(root)?)
} else {
let mut command = Command::new("git");
command.arg("-C").arg(root);
command
};
command.args(["hash-object", "--no-filters", "--stdin"]);
let id = text(checked(run(
command,
content.as_bytes().to_vec(),
&AtomicBool::new(false),
)?)?)?;
Ok(format!("working:{}", id.trim()))
}
fn discover_root_with_cancellation(path: &Path, cancelled: &AtomicBool) -> Result<PathBuf, String> { fn discover_root_with_cancellation(path: &Path, cancelled: &AtomicBool) -> Result<PathBuf, String> {
let path = fs::canonicalize(path).map_err(|error| error.to_string())?; let path = fs::canonicalize(path).map_err(|error| error.to_string())?;
let root = path let root = path
+1 -1
View File
@@ -43,7 +43,7 @@ pub enum Verdict {
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
#[serde(deny_unknown_fields)] #[serde(deny_unknown_fields)]
pub struct Anchor { pub struct Anchor {
/// Frozen comparison context; `content` additionally captures any live buffer edits. /// Frozen repository context; source comments use the exact buffer's Git blob identity.
pub revision: String, pub revision: String,
pub path: PathBuf, pub path: PathBuf,
pub side: Side, pub side: Side,
+80 -9
View File
@@ -807,6 +807,7 @@ impl BufferViewports {
editor.file_navigation_hint(), editor.file_navigation_hint(),
&mut self.panels, &mut self.panels,
Some(editor), Some(editor),
Some(&editor.local_review),
); );
} else { } else {
draw_overlays( draw_overlays(
@@ -1009,6 +1010,7 @@ impl BufferViewports {
view.whitespace_scope(), view.whitespace_scope(),
view.resolver(), view.resolver(),
true, true,
None,
); );
if view.render_pane_focused() if view.render_pane_focused()
&& !view.file_picker().is_some_and(|picker| picker.focused) && !view.file_picker().is_some_and(|picker| picker.focused)
@@ -1823,7 +1825,14 @@ fn chrome_items<'a>(
tab: None, tab: None,
accent: !matches!( accent: !matches!(
name.as_str(), name.as_str(),
"path" | "language" | "diagnostics" | "selections" | "count" | "pending" | "buffer" | "diff" "path"
| "language"
| "diagnostics"
| "selections"
| "count"
| "pending"
| "buffer"
| "diff"
), ),
bold: false, bold: false,
first: true, first: true,
@@ -2236,6 +2245,7 @@ fn draw_document(
editor.file_navigation_hint(), editor.file_navigation_hint(),
panels, panels,
Some(editor), Some(editor),
Some(&editor.local_review),
); );
return; return;
} }
@@ -2275,6 +2285,17 @@ fn draw_document(
let mut document_caret = None; let mut document_caret = None;
let carets = editor.carets(); let carets = editor.carets();
let selections = editor.selections(); let selections = editor.selections();
let review_comments: Vec<_> = editor
.local_review
.document
.comments
.iter()
.filter_map(|comment| {
editor
.source_review_anchor_range(&comment.anchor)
.map(|range| (comment, range))
})
.collect();
let wrapped = editor.word_wrap(); let wrapped = editor.word_wrap();
let presentation = (editor.buffer_id(), wrapped, width, height); let presentation = (editor.buffer_id(), wrapped, width, height);
let identity = (editor.buffer_id(), editor.text_revision(), cursor); let identity = (editor.buffer_id(), editor.text_revision(), cursor);
@@ -2576,6 +2597,25 @@ fn draw_document(
}) })
.collect(); .collect();
frame.render_widget(Paragraph::new(numbers), gutter); frame.render_widget(Paragraph::new(numbers), gutter);
for (screen, &(row, continuation, _)) in document.visible.iter().enumerate() {
if continuation != 0 {
continue;
}
if let Some((comment, _)) = review_comments
.iter()
.filter(|(_, range)| {
let end = range.end.row
- usize::from(range.end.column == 0 && range.end.row > range.start.row);
(range.start.row..=end).contains(&row)
})
.max_by_key(|(comment, _)| editor.local_review.selected == Some(comment.id))
{
let selected = editor.local_review.selected == Some(comment.id);
frame.buffer_mut()[(gutter.right() - 1, gutter.y + screen as u16)]
.set_symbol(if selected { "●" } else { "○" })
.set_fg(Color::Magenta);
}
}
} }
let lines: Vec<Line> = document let lines: Vec<Line> = document
.visible .visible
@@ -2627,6 +2667,33 @@ fn draw_document(
if editor.highlight_trailing_whitespace() { if editor.highlight_trailing_whitespace() {
paint_trailing_whitespace(frame, line, area, segment.start); paint_trailing_whitespace(frame, line, area, segment.start);
} }
if let Some((_, range)) = review_comments
.iter()
.find(|(comment, _)| editor.local_review.selected == Some(comment.id))
{
let end_row = range.end.row
- usize::from(range.end.column == 0 && range.end.row > range.start.row);
if (range.start.row..=end_row).contains(&row) {
let start = if row == range.start.row {
range.start.column
} else {
0
};
let end = if row == range.end.row {
range.end.column
} else {
usize::MAX
};
paint_grapheme_range(
frame,
area,
segment.start,
line,
start..end,
Style::default().bg(Color::Magenta),
);
}
}
paint_editor_line( paint_editor_line(
frame, frame,
editor, editor,
@@ -3337,6 +3404,7 @@ fn draw_diff_popups(
} }
} }
#[allow(clippy::too_many_arguments)]
fn draw_diff( fn draw_diff(
frame: &mut Frame, frame: &mut Frame,
diff: &DiffState, diff: &DiffState,
@@ -3345,6 +3413,7 @@ fn draw_diff(
file_navigation_hint: &str, file_navigation_hint: &str,
panels: &mut WheelPanels, panels: &mut WheelPanels,
editor: Option<&Editor>, editor: Option<&Editor>,
review: Option<&crate::review::ReviewState>,
) { ) {
if let Some(editor) = editor { if let Some(editor) = editor {
panels.picker_scroll.distance = editor.scroll_distance(); panels.picker_scroll.distance = editor.scroll_distance();
@@ -3814,7 +3883,7 @@ fn draw_diff(
comparison.map(|comparison| (comparison.files.as_slice(), &diff.reviewed_files)), comparison.map(|comparison| (comparison.files.as_slice(), &diff.reviewed_files)),
); );
} }
let document_caret = panels.draw_documents(frame, diff, panes, navigator, editor); let document_caret = panels.draw_documents(frame, diff, panes, navigator, editor, review);
if let Some(editor) = editor { if let Some(editor) = editor {
draw_diff_popups(frame, editor, area, document_caret, panels); draw_diff_popups(frame, editor, area, document_caret, panels);
} }
@@ -3839,6 +3908,7 @@ impl WheelPanels {
panes: Rect, panes: Rect,
navigator: Option<Rect>, navigator: Option<Rect>,
editor: Option<&Editor>, editor: Option<&Editor>,
review: Option<&crate::review::ReviewState>,
) -> Option<(Rect, Position)> { ) -> Option<(Rect, Position)> {
self.cached_diff_error(Some(diff)); self.cached_diff_error(Some(diff));
let selected = diff let selected = diff
@@ -4087,6 +4157,7 @@ impl WheelPanels {
editor.map_or(WhitespaceScope::All, Editor::whitespace_scope), editor.map_or(WhitespaceScope::All, Editor::whitespace_scope),
resolver, resolver,
false, false,
review,
); );
text_widths[0] = text_area.width; text_widths[0] = text_area.width;
if let Some(editor) = if let Some(editor) =
@@ -4127,6 +4198,7 @@ impl WheelPanels {
editor.map_or(WhitespaceScope::All, Editor::whitespace_scope), editor.map_or(WhitespaceScope::All, Editor::whitespace_scope),
resolver, resolver,
false, false,
review,
); );
text_widths[1] = text_area.width; text_widths[1] = text_area.width;
if let Some(editor) = if let Some(editor) =
@@ -4464,6 +4536,7 @@ fn draw_diff_pane(
whitespace_scope: WhitespaceScope, whitespace_scope: WhitespaceScope,
resolver: Option<&ResolverWorkspace>, resolver: Option<&ResolverWorkspace>,
content_only: bool, content_only: bool,
review: Option<&crate::review::ReviewState>,
) -> Rect { ) -> Rect {
if area.width == 0 || area.height == 0 { if area.width == 0 || area.height == 0 {
return area; return area;
@@ -4487,11 +4560,9 @@ fn draw_diff_pane(
*left = diff.bound_horizontal(focus, *left, width); *left = diff.bound_horizontal(focus, *left, width);
} }
let left = *left; let left = *left;
let review_comments = diff let review_comments = review
.local_review .into_iter()
.document .flat_map(|review| review.document.comments.iter())
.comments
.iter()
.filter_map(|comment| { .filter_map(|comment| {
let before = comment.anchor.side == crate::review::Side::Before; let before = comment.anchor.side == crate::review::Side::Before;
let path = if before { let path = if before {
@@ -4502,7 +4573,7 @@ fn draw_diff_pane(
if !(unified || before == (focus == Focus::Before)) || comment.anchor.path != *path { if !(unified || before == (focus == Focus::Before)) || comment.anchor.path != *path {
return None; return None;
} }
diff.review_anchor_range(&comment.anchor) diff.review_anchor_range(&comment.anchor, review.unwrap())
.map(|range| (comment, range)) .map(|range| (comment, range))
}); });
paint_diff_content( paint_diff_content(
@@ -4518,7 +4589,7 @@ fn draw_diff_pane(
whitespace_characters, whitespace_characters,
whitespace_scope, whitespace_scope,
review_comments, review_comments,
diff.local_review.selected, review.and_then(|review| review.selected),
) )
} }
+107 -78
View File
@@ -2460,6 +2460,7 @@ fn column_view_suppresses_whole_file_emphasis_without_losing_change_markers() {
#[test] #[test]
fn working_refresh_preserves_noops_errors_and_view_state_but_invalidates_old_identity() { fn working_refresh_preserves_noops_errors_and_view_state_but_invalidates_old_identity() {
let mut review = crate::review::ReviewState::default();
let mut state = live_state(vec!["old".into()], vec!["new".into()], false); let mut state = live_state(vec!["old".into()], vec!["new".into()], false);
state.loading = false; state.loading = false;
state.pending = None; state.pending = None;
@@ -2554,7 +2555,7 @@ fn working_refresh_preserves_noops_errors_and_view_state_but_invalidates_old_ide
.unwrap() .unwrap()
.editable_paths .editable_paths
.insert("file".into()); .insert("file".into());
state.refresh_after(&["dirty".into()], 1); state.refresh_after(&["dirty".into()], 1, &mut review);
assert_eq!(state.comparison.as_ref().unwrap().files[1].after, ["dirty"]); assert_eq!(state.comparison.as_ref().unwrap().files[1].after, ["dirty"]);
state.queue_checkout_refresh(false); state.queue_checkout_refresh(false);
state.take_request(); state.take_request();
@@ -2575,6 +2576,7 @@ fn working_refresh_preserves_noops_errors_and_view_state_but_invalidates_old_ide
#[test] #[test]
fn reviewed_files_use_highlighted_catalog_indices_not_sorted_or_filtered_rows() { fn reviewed_files_use_highlighted_catalog_indices_not_sorted_or_filtered_rows() {
let mut review = crate::review::ReviewState::default();
let mut state = DiffState::new(); let mut state = DiffState::new();
let mut changes = comparison(); let mut changes = comparison();
changes.files[0].path = "nested/z.txt".into(); changes.files[0].path = "nested/z.txt".into();
@@ -2591,9 +2593,9 @@ fn reviewed_files_use_highlighted_catalog_indices_not_sorted_or_filtered_rows()
} }
files.reveal_path(Path::new("nested/a.txt")); files.reveal_path(Path::new("nested/a.txt"));
assert_eq!(state.file, 0); assert_eq!(state.file, 0);
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
assert_eq!(state.reviewed_files, BTreeSet::from([1])); assert_eq!(state.reviewed_files, BTreeSet::from([1]));
assert_eq!(state.toggle_reviewed(), Ok(false)); assert_eq!(state.toggle_reviewed(&mut review), Ok(false));
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
} }
let files = state.files.as_mut().unwrap(); let files = state.files.as_mut().unwrap();
@@ -2602,7 +2604,7 @@ fn reviewed_files_use_highlighted_catalog_indices_not_sorted_or_filtered_rows()
let selected = files.entries().position(|entry| entry.is_dir).unwrap(); let selected = files.entries().position(|entry| entry.is_dir).unwrap();
files.selected = selected; files.selected = selected;
assert_eq!( assert_eq!(
state.toggle_reviewed(), state.toggle_reviewed(&mut review),
Err("Select a changed file, not a directory") Err("Select a changed file, not a directory")
); );
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
@@ -2611,32 +2613,42 @@ fn reviewed_files_use_highlighted_catalog_indices_not_sorted_or_filtered_rows()
for ch in "missing".chars() { for ch in "missing".chars() {
files.handle_filter_input(&Input::Char(ch)); files.handle_filter_input(&Input::Char(ch));
} }
assert_eq!(state.toggle_reviewed(), Err("No changed file is selected")); assert_eq!(
state.toggle_reviewed(&mut review),
Err("No changed file is selected")
);
state.files = Some(FilePicker::from_paths( state.files = Some(FilePicker::from_paths(
PathBuf::new(), PathBuf::new(),
[(0, PathBuf::from("ordinary.txt"))], [(0, PathBuf::from("ordinary.txt"))],
)); ));
assert_eq!(state.toggle_reviewed(), Err("No changed file is selected")); assert_eq!(
state.toggle_reviewed(&mut review),
Err("No changed file is selected")
);
state.focus = Focus::Before; state.focus = Focus::Before;
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
assert_eq!(state.reviewed_files, BTreeSet::from([0])); assert_eq!(state.reviewed_files, BTreeSet::from([0]));
assert!(state.select_file_index(2)); assert!(state.select_file_index(2));
state.focus = Focus::Before; state.focus = Focus::Before;
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
assert_eq!(state.reviewed_files, BTreeSet::from([0, 2])); assert_eq!(state.reviewed_files, BTreeSet::from([0, 2]));
} }
#[test] #[test]
fn reviewed_files_survive_lazy_details_and_views_but_reset_on_comparison() { fn reviewed_files_survive_lazy_details_and_views_but_reset_on_comparison() {
let mut review = crate::review::ReviewState::default();
let mut state = DiffState::new(); let mut state = DiffState::new();
assert_eq!(state.toggle_reviewed(), Err("No comparison is open")); assert_eq!(
state.toggle_reviewed(&mut review),
Err("No comparison is open")
);
let mut changes = comparison(); let mut changes = comparison();
changes.files[0].loaded = false; changes.files[0].loaded = false;
changes.files[0].before.clear(); changes.files[0].before.clear();
changes.files[0].after.clear(); changes.files[0].after.clear();
state.request_kind = Some(RequestKind::Comparison); state.request_kind = Some(RequestKind::Comparison);
state.apply_result(1, Ok(Outcome::Comparison(changes))); state.apply_result(1, Ok(Outcome::Comparison(changes)));
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
assert_eq!(state.reviewed_files, BTreeSet::from([0])); assert_eq!(state.reviewed_files, BTreeSet::from([0]));
let file = comparison().files.remove(0); let file = comparison().files.remove(0);
state.apply_result( state.apply_result(
@@ -2667,10 +2679,13 @@ fn reviewed_files_survive_lazy_details_and_views_but_reset_on_comparison() {
)); ));
state.toggle_comparison_mode().unwrap(); state.toggle_comparison_mode().unwrap();
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
assert_eq!(state.toggle_reviewed(), Err("No comparison is open")); assert_eq!(
state.toggle_reviewed(&mut review),
Err("No comparison is open")
);
state.apply_result(3, Ok(Outcome::Comparison(comparison()))); state.apply_result(3, Ok(Outcome::Comparison(comparison())));
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
state.request_kind = Some(RequestKind::Comparison); state.request_kind = Some(RequestKind::Comparison);
state.apply_result(4, Ok(Outcome::Comparison(comparison()))); state.apply_result(4, Ok(Outcome::Comparison(comparison())));
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
@@ -2678,17 +2693,18 @@ fn reviewed_files_survive_lazy_details_and_views_but_reset_on_comparison() {
#[test] #[test]
fn restored_review_marks_use_file_identities_and_rename_paths_not_catalog_order() { fn restored_review_marks_use_file_identities_and_rename_paths_not_catalog_order() {
let mut review = crate::review::ReviewState::default();
let mut state = DiffState::new(); let mut state = DiffState::new();
state.request_kind = Some(RequestKind::Comparison); state.request_kind = Some(RequestKind::Comparison);
state.apply_result(1, Ok(Outcome::Comparison(comparison()))); state.apply_result(1, Ok(Outcome::Comparison(comparison())));
state.comparison.as_mut().unwrap().files[0].review_revisions = state.comparison.as_mut().unwrap().files[0].review_revisions =
Some(["file:before".into(), "file:after".into()]); Some(["file:before".into(), "file:after".into()]);
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
state.comparison.as_mut().unwrap().files.swap(0, 1); state.comparison.as_mut().unwrap().files.swap(0, 1);
state.restore_reviewed_files(); state.restore_reviewed_files(&review);
assert_eq!(state.reviewed_files, BTreeSet::from([1])); assert_eq!(state.reviewed_files, BTreeSet::from([1]));
state.comparison.as_mut().unwrap().files[1].old_path = Some("different.txt".into()); state.comparison.as_mut().unwrap().files[1].old_path = Some("different.txt".into());
state.restore_reviewed_files(); state.restore_reviewed_files(&review);
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
state.comparison.as_mut().unwrap().files[1].old_path = Some("old.txt".into()); state.comparison.as_mut().unwrap().files[1].old_path = Some("old.txt".into());
for side in [0, 1] { for side in [0, 1] {
@@ -2698,7 +2714,7 @@ fn restored_review_marks_use_file_identities_and_rename_paths_not_catalog_order(
.unwrap(); .unwrap();
let original = identities[side].clone(); let original = identities[side].clone();
identities[side].push_str("-different"); identities[side].push_str("-different");
state.restore_reviewed_files(); state.restore_reviewed_files(&review);
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
state.comparison.as_mut().unwrap().files[1] state.comparison.as_mut().unwrap().files[1]
.review_revisions .review_revisions
@@ -2706,12 +2722,13 @@ fn restored_review_marks_use_file_identities_and_rename_paths_not_catalog_order(
.unwrap()[side] = original; .unwrap()[side] = original;
} }
state.comparison.as_mut().unwrap().files[1].loaded = false; state.comparison.as_mut().unwrap().files[1].loaded = false;
state.restore_reviewed_files(); state.restore_reviewed_files(&review);
assert_eq!(state.reviewed_files, BTreeSet::from([1])); assert_eq!(state.reviewed_files, BTreeSet::from([1]));
} }
#[test] #[test]
fn unchanged_file_review_survives_other_files_changing() { fn unchanged_file_review_survives_other_files_changing() {
let mut review = crate::review::ReviewState::default();
use crate::repository::Repository; use crate::repository::Repository;
use std::sync::{Arc, atomic::AtomicBool}; use std::sync::{Arc, atomic::AtomicBool};
let root = std::env::temp_dir().join(format!("vamp-review-files-{}", std::process::id())); let root = std::env::temp_dir().join(format!("vamp-review-files-{}", std::process::id()));
@@ -2759,13 +2776,13 @@ fn unchanged_file_review_survives_other_files_changing() {
let original_revisions = changes.revisions.clone(); let original_revisions = changes.revisions.clone();
state.request_kind = Some(RequestKind::Comparison); state.request_kind = Some(RequestKind::Comparison);
state.apply_result(1, Ok(Outcome::Comparison(changes))); state.apply_result(1, Ok(Outcome::Comparison(changes)));
state.toggle_reviewed().unwrap(); state.toggle_reviewed(&mut review).unwrap();
state.file = 1; state.file = 1;
state.focus = Focus::Before; state.focus = Focus::Before;
state.toggle_reviewed().unwrap(); state.toggle_reviewed(&mut review).unwrap();
let mut store = crate::review::Store::open(&root.join("reviews")).unwrap(); let mut store = crate::review::Store::open(&root.join("reviews")).unwrap();
store.save(&mut state.local_review.document).unwrap(); store.save(&mut review.document).unwrap();
let id = state.local_review.document.storage_id.unwrap(); let id = review.document.storage_id.unwrap();
std::fs::write(root.join("b.txt"), "edited again\n").unwrap(); std::fs::write(root.join("b.txt"), "edited again\n").unwrap();
let changes = repository let changes = repository
.compare(2, &before, &after, ComparisonMode::Snapshots, &[]) .compare(2, &before, &after, ComparisonMode::Snapshots, &[])
@@ -2773,8 +2790,8 @@ fn unchanged_file_review_survives_other_files_changing() {
assert_ne!(changes.revisions, original_revisions); assert_ne!(changes.revisions, original_revisions);
state.request_kind = Some(RequestKind::Comparison); state.request_kind = Some(RequestKind::Comparison);
state.apply_result(2, Ok(Outcome::Comparison(changes))); state.apply_result(2, Ok(Outcome::Comparison(changes)));
state.local_review.document = store.load(&root, id).unwrap(); review.document = store.load(&root, id).unwrap();
state.restore_reviewed_files(); state.restore_reviewed_files(&review);
assert_eq!(state.reviewed_files, BTreeSet::from([0])); assert_eq!(state.reviewed_files, BTreeSet::from([0]));
let detail = repository.load_file(2, 0).unwrap(); let detail = repository.load_file(2, 0).unwrap();
state.apply_result( state.apply_result(
@@ -2785,33 +2802,36 @@ fn unchanged_file_review_survives_other_files_changing() {
file: detail, file: detail,
}), }),
); );
state.restore_reviewed_files(); state.restore_reviewed_files(&review);
assert_eq!(state.reviewed_files, BTreeSet::from([0])); assert_eq!(state.reviewed_files, BTreeSet::from([0]));
std::fs::remove_dir_all(root).unwrap(); std::fs::remove_dir_all(root).unwrap();
} }
#[test] #[test]
fn restored_live_review_marks_require_exact_text_and_final_newline() { fn restored_live_review_marks_require_exact_text_and_final_newline() {
let mut review = crate::review::ReviewState::default();
let mut reopened_review = crate::review::ReviewState::default();
let mut state = live_state(vec!["base".into()], vec!["changed".into()], false); let mut state = live_state(vec!["base".into()], vec!["changed".into()], false);
state.refresh_after(&["unsaved".into(), String::new()], 1); state.refresh_after(&["unsaved".into(), String::new()], 1, &mut review);
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
let document = state.local_review.document.clone(); let document = review.document.clone();
let mut reopened = live_state(vec!["base".into()], vec!["changed".into()], false); let mut reopened = live_state(vec!["base".into()], vec!["changed".into()], false);
reopened.local_review.document = document.clone(); reopened_review.document = document.clone();
reopened.restore_reviewed_files(); reopened.restore_reviewed_files(&reopened_review);
assert!(reopened.reviewed_files.is_empty()); assert!(reopened.reviewed_files.is_empty());
reopened.refresh_after(&["unsaved".into(), String::new()], 1); reopened.refresh_after(&["unsaved".into(), String::new()], 1, &mut reopened_review);
assert_eq!(reopened.reviewed_files, BTreeSet::from([0])); assert_eq!(reopened.reviewed_files, BTreeSet::from([0]));
reopened.refresh_after(&["unsaved".into()], 2); reopened.refresh_after(&["unsaved".into()], 2, &mut reopened_review);
assert!(reopened.reviewed_files.is_empty()); assert!(reopened.reviewed_files.is_empty());
assert!(reopened.local_review.document.reviewed_files.is_empty()); assert!(reopened_review.document.reviewed_files.is_empty());
reopened.local_review.document = document; reopened_review.document = document;
reopened.restore_reviewed_files(); reopened.restore_reviewed_files(&reopened_review);
assert!(reopened.reviewed_files.is_empty()); assert!(reopened.reviewed_files.is_empty());
} }
#[test] #[test]
fn working_review_marks_clear_only_for_changed_bytes_or_eof() { fn working_review_marks_clear_only_for_changed_bytes_or_eof() {
let mut review = crate::review::ReviewState::default();
for (after, eof, lines) in [ for (after, eof, lines) in [
(vec!["changed".into()], false, vec!["changed".into()]), (vec!["changed".into()], false, vec!["changed".into()]),
( (
@@ -2822,24 +2842,24 @@ fn working_review_marks_clear_only_for_changed_bytes_or_eof() {
(Vec::new(), false, vec![String::new()]), (Vec::new(), false, vec![String::new()]),
] { ] {
let mut state = live_state(vec!["base".into()], after, eof); let mut state = live_state(vec!["base".into()], after, eof);
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
state.refresh_after(&lines, 1); state.refresh_after(&lines, 1, &mut review);
assert_eq!(state.reviewed_files, BTreeSet::from([0])); assert_eq!(state.reviewed_files, BTreeSet::from([0]));
state.refresh_after(&lines, 2); state.refresh_after(&lines, 2, &mut review);
assert_eq!(state.reviewed_files, BTreeSet::from([0])); assert_eq!(state.reviewed_files, BTreeSet::from([0]));
state.refresh_after(&["edited".into()], 3); state.refresh_after(&["edited".into()], 3, &mut review);
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
state.refresh_after(&["edited".into(), String::new()], 4); state.refresh_after(&["edited".into(), String::new()], 4, &mut review);
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
state.refresh_after(&["edited".into()], 5); state.refresh_after(&["edited".into()], 5, &mut review);
assert!(state.reviewed_files.is_empty()); assert!(state.reviewed_files.is_empty());
} }
let mut state = live_state(vec!["base".into()], vec!["changed".into()], false); let mut state = live_state(vec!["base".into()], vec!["changed".into()], false);
state.comparison.as_mut().unwrap().files[0].loaded = false; state.comparison.as_mut().unwrap().files[0].loaded = false;
assert_eq!(state.toggle_reviewed(), Ok(true)); assert_eq!(state.toggle_reviewed(&mut review), Ok(true));
state.refresh_after(&["edited".into()], 1); state.refresh_after(&["edited".into()], 1, &mut review);
assert_eq!(state.reviewed_files, BTreeSet::from([0])); assert_eq!(state.reviewed_files, BTreeSet::from([0]));
} }
@@ -2901,19 +2921,20 @@ fn unified_groups_changed_runs_and_roundtrips_side_specific_source_rows() {
#[test] #[test]
fn unified_live_reflow_preserves_removed_top_and_expanded_source_excerpts() { fn unified_live_reflow_preserves_removed_top_and_expanded_source_excerpts() {
let mut review = crate::review::ReviewState::default();
let base: Vec<_> = (0..40).map(|row| format!("line {row}")).collect(); let base: Vec<_> = (0..40).map(|row| format!("line {row}")).collect();
let mut lines = base.clone(); let mut lines = base.clone();
lines[20] = "new line".into(); lines[20] = "new line".into();
let mut state = live_state(base, lines.clone(), false); let mut state = live_state(base, lines.clone(), false);
state.set_context_options(1, 3); state.set_context_options(1, 3);
state.refresh_after(&lines, 1); state.refresh_after(&lines, 1, &mut review);
state.set_view_mode(ViewMode::Unified); state.set_view_mode(ViewMode::Unified);
state.top = state.source_display_row(Focus::Before, 20).unwrap(); state.top = state.source_display_row(Focus::Before, 20).unwrap();
state.expand_source_excerpt(Focus::After, false, 20); state.expand_source_excerpt(Focus::After, false, 20);
assert_eq!(state.display_source_row(Focus::Before, state.top), Some(20)); assert_eq!(state.display_source_row(Focus::Before, state.top), Some(20));
assert!(state.source_display_row(Focus::After, 16).is_some()); assert!(state.source_display_row(Focus::After, 16).is_some());
lines.insert(0, "inserted".into()); lines.insert(0, "inserted".into());
state.refresh_after(&lines, 2); state.refresh_after(&lines, 2, &mut review);
assert_eq!(state.display_focus(state.top), Some(Focus::Before)); assert_eq!(state.display_focus(state.top), Some(Focus::Before));
assert_eq!(state.display_source_row(Focus::Before, state.top), Some(20)); assert_eq!(state.display_source_row(Focus::Before, state.top), Some(20));
assert!(state.source_display_row(Focus::After, 17).is_some()); assert!(state.source_display_row(Focus::After, 17).is_some());
@@ -2932,6 +2953,7 @@ fn unified_live_reflow_preserves_removed_top_and_expanded_source_excerpts() {
#[test] #[test]
fn character_ranges_follow_insertions_deletions_unicode_and_live_edits() { fn character_ranges_follow_insertions_deletions_unicode_and_live_edits() {
let mut review = crate::review::ReviewState::default();
for (before, after, expected) in [ for (before, after, expected) in [
("ab", "a界b", vec![[1..1, 1..2]]), ("ab", "a界b", vec![[1..1, 1..2]]),
("a界b", "ab", vec![[1..2, 1..1]]), ("a界b", "ab", vec![[1..2, 1..1]]),
@@ -2941,17 +2963,18 @@ fn character_ranges_follow_insertions_deletions_unicode_and_live_edits() {
assert_eq!(state.rows[0].changes, expected); assert_eq!(state.rows[0].changes, expected);
} }
let mut state = live_state(vec!["abc".into()], vec!["aXbc".into()], false); let mut state = live_state(vec!["abc".into()], vec!["aXbc".into()], false);
state.refresh_after(&["aXYbc".into()], 1); state.refresh_after(&["aXYbc".into()], 1, &mut review);
assert_eq!(state.rows[0].changes, vec![[1..1, 1..3]]); assert_eq!(state.rows[0].changes, vec![[1..1, 1..3]]);
state.refresh_after(&["abYZc".into()], 2); state.refresh_after(&["abYZc".into()], 2, &mut review);
assert_eq!(state.rows[0].changes, vec![[2..2, 2..4]]); assert_eq!(state.rows[0].changes, vec![[2..2, 2..4]]);
state.refresh_after(&["abc".into()], 3); state.refresh_after(&["abc".into()], 3, &mut review);
assert_eq!(state.rows[0].kind, RowKind::Equal); assert_eq!(state.rows[0].kind, RowKind::Equal);
assert!(state.rows[0].changes.is_empty()); assert!(state.rows[0].changes.is_empty());
} }
#[test] #[test]
fn character_highlights_merge_incidental_matches_but_preserve_meaningful_gaps() { fn character_highlights_merge_incidental_matches_but_preserve_meaningful_gaps() {
let mut review = crate::review::ReviewState::default();
let mut cases = vec![ let mut cases = vec![
("a..c".into(), "b..d".into(), vec![[0..4, 0..4]]), ("a..c".into(), "b..d".into(), vec![[0..4, 0..4]]),
("base".into(), "edited".into(), vec![[0..4, 0..6]]), ("base".into(), "edited".into(), vec![[0..4, 0..6]]),
@@ -2995,14 +3018,15 @@ fn character_highlights_merge_incidental_matches_but_preserve_meaningful_gaps()
for (before, after, expected) in cases { for (before, after, expected) in cases {
let mut state = live_state(vec![before], vec![after.clone()], false); let mut state = live_state(vec![before], vec![after.clone()], false);
assert_eq!(state.rows[0].changes, expected, "{after:?}"); assert_eq!(state.rows[0].changes, expected, "{after:?}");
state.refresh_after(&[format!("{after}!")], 1); state.refresh_after(&[format!("{after}!")], 1, &mut review);
state.refresh_after(std::slice::from_ref(&after), 2); state.refresh_after(std::slice::from_ref(&after), 2, &mut review);
assert_eq!(state.rows[0].changes, expected, "live edit: {after:?}"); assert_eq!(state.rows[0].changes, expected, "live edit: {after:?}");
} }
} }
#[test] #[test]
fn reverting_loaded_file_keeps_unloaded_changes_selectable() { fn reverting_loaded_file_keeps_unloaded_changes_selectable() {
let mut review = crate::review::ReviewState::default();
let mut state = live_state(vec!["base".into()], vec!["changed".into()], false); let mut state = live_state(vec!["base".into()], vec!["changed".into()], false);
state.comparison_id = Some(17); state.comparison_id = Some(17);
state.pending = None; state.pending = None;
@@ -3020,7 +3044,7 @@ fn reverting_loaded_file_keeps_unloaded_changes_selectable() {
hunks: vec![], hunks: vec![],
notice: None, notice: None,
}); });
state.refresh_after(&["base".into()], 1); state.refresh_after(&["base".into()], 1, &mut review);
assert_eq!( assert_eq!(
state state
.files .files
@@ -3043,36 +3067,37 @@ fn reverting_loaded_file_keeps_unloaded_changes_selectable() {
#[test] #[test]
fn live_width_cache_scans_only_changed_lines_and_shrinks_after_deletion() { fn live_width_cache_scans_only_changed_lines_and_shrinks_after_deletion() {
let mut review = crate::review::ReviewState::default();
let base = vec!["original".into(), "medium".into(), "short".into()]; let base = vec!["original".into(), "medium".into(), "short".into()];
let mut lines = vec!["界\tz".into(), "medium".into(), "short".into()]; let mut lines = vec!["界\tz".into(), "medium".into(), "short".into()];
let mut state = live_state(base, lines.clone(), false); let mut state = live_state(base, lines.clone(), false);
state.refresh_after(&lines, 1); state.refresh_after(&lines, 1, &mut review);
assert_eq!(state.after_widths, [5, 6, 5]); assert_eq!(state.after_widths, [5, 6, 5]);
lines[0] = "界界\t🙂".into(); lines[0] = "界界\t🙂".into();
WIDTH_SCANS.set(0); WIDTH_SCANS.set(0);
state.refresh_after(&lines, 2); state.refresh_after(&lines, 2, &mut review);
assert_eq!(WIDTH_SCANS.get(), 1); assert_eq!(WIDTH_SCANS.get(), 1);
assert_eq!(state.max_after, 10); assert_eq!(state.max_after, 10);
state.left_after = 9; state.left_after = 9;
lines[0] = "界".into(); lines[0] = "界".into();
WIDTH_SCANS.set(0); WIDTH_SCANS.set(0);
state.refresh_after(&lines, 3); state.refresh_after(&lines, 3, &mut review);
assert_eq!(WIDTH_SCANS.get(), 1); assert_eq!(WIDTH_SCANS.get(), 1);
assert_eq!(state.max_after, 6); assert_eq!(state.max_after, 6);
assert_eq!(state.left_after, 6); assert_eq!(state.left_after, 6);
lines.remove(1); lines.remove(1);
WIDTH_SCANS.set(0); WIDTH_SCANS.set(0);
state.refresh_after(&lines, 4); state.refresh_after(&lines, 4, &mut review);
assert_eq!(WIDTH_SCANS.get(), 0); assert_eq!(WIDTH_SCANS.get(), 0);
assert_eq!(state.after_widths, [2, 5]); assert_eq!(state.after_widths, [2, 5]);
assert_eq!(state.max_after, 5); assert_eq!(state.max_after, 5);
lines.insert(1, "巨大\t🙂".into()); lines.insert(1, "巨大\t🙂".into());
WIDTH_SCANS.set(0); WIDTH_SCANS.set(0);
state.refresh_after(&lines, 5); state.refresh_after(&lines, 5, &mut review);
assert_eq!(WIDTH_SCANS.get(), 1); assert_eq!(WIDTH_SCANS.get(), 1);
assert_eq!(state.after_widths, [2, 10, 5]); assert_eq!(state.after_widths, [2, 10, 5]);
lines = vec!["".into()]; lines = vec!["".into()];
state.refresh_after(&lines, 6); state.refresh_after(&lines, 6, &mut review);
assert_eq!(state.after_widths, [0]); assert_eq!(state.after_widths, [0]);
assert_eq!(state.max_after, 0); assert_eq!(state.max_after, 0);
assert_eq!(state.left_after, 0); assert_eq!(state.left_after, 0);
@@ -3080,6 +3105,7 @@ fn live_width_cache_scans_only_changed_lines_and_shrinks_after_deletion() {
#[test] #[test]
fn stable_line_edits_keep_fresh_hunks_expansion_and_scroll_anchor_until_revert() { fn stable_line_edits_keep_fresh_hunks_expansion_and_scroll_anchor_until_revert() {
let mut review = crate::review::ReviewState::default();
let base = (0..100) let base = (0..100)
.map(|row| format!("line {row}")) .map(|row| format!("line {row}"))
.collect::<Vec<_>>(); .collect::<Vec<_>>();
@@ -3088,7 +3114,7 @@ fn stable_line_edits_keep_fresh_hunks_expansion_and_scroll_anchor_until_revert()
let mut state = live_state(base.clone(), lines.clone(), true); let mut state = live_state(base.clone(), lines.clone(), true);
state.set_context_options(2, 5); state.set_context_options(2, 5);
lines.push(String::new()); lines.push(String::new());
state.refresh_after(&lines, 1); state.refresh_after(&lines, 1, &mut review);
state.top = state.after_display_row(30).unwrap(); state.top = state.after_display_row(30).unwrap();
state.handle(Input::Char('[')); state.handle(Input::Char('['));
let display = state.display_rows.clone(); let display = state.display_rows.clone();
@@ -3102,7 +3128,7 @@ fn stable_line_edits_keep_fresh_hunks_expansion_and_scroll_anchor_until_revert()
changed_line_range(&file.after, &lines) changed_line_range(&file.after, &lines)
)); ));
WIDTH_SCANS.set(0); WIDTH_SCANS.set(0);
state.refresh_after(&lines, 2); state.refresh_after(&lines, 2, &mut review);
assert_eq!(WIDTH_SCANS.get(), 1); assert_eq!(WIDTH_SCANS.get(), 1);
assert_eq!(state.display_rows, display); assert_eq!(state.display_rows, display);
assert_eq!(state.excerpt_ranges, excerpts); assert_eq!(state.excerpt_ranges, excerpts);
@@ -3131,14 +3157,14 @@ fn stable_line_edits_keep_fresh_hunks_expansion_and_scroll_anchor_until_revert()
&lines, &lines,
changed_line_range(&file.after, &lines) changed_line_range(&file.after, &lines)
)); ));
state.refresh_after(&lines, 3); state.refresh_after(&lines, 3, &mut review);
assert!(state.comparison.as_ref().unwrap().files[0].hunks.is_empty()); assert!(state.comparison.as_ref().unwrap().files[0].hunks.is_empty());
assert!(state.rows.iter().all(|row| row.kind == RowKind::Equal)); assert!(state.rows.iter().all(|row| row.kind == RowKind::Equal));
assert!(state.after_display_row(23).is_some()); assert!(state.after_display_row(23).is_some());
assert_eq!(state.top, state.after_display_row(30).unwrap()); assert_eq!(state.top, state.after_display_row(30).unwrap());
assert_eq!(state.files.as_ref().unwrap().entries().len(), 0); assert_eq!(state.files.as_ref().unwrap().entries().len(), 0);
lines[50] = "changed again".into(); lines[50] = "changed again".into();
state.refresh_after(&lines, 4); state.refresh_after(&lines, 4, &mut review);
assert_eq!(state.files.as_ref().unwrap().entries().len(), 1); assert_eq!(state.files.as_ref().unwrap().entries().len(), 1);
assert!(state.after_display_row(23).is_some()); assert!(state.after_display_row(23).is_some());
assert!(state.after_display_row(50).is_some()); assert!(state.after_display_row(50).is_some());
@@ -3146,6 +3172,7 @@ fn stable_line_edits_keep_fresh_hunks_expansion_and_scroll_anchor_until_revert()
#[test] #[test]
fn live_reflow_keeps_separate_hunks_and_expanded_context_after_revert() { fn live_reflow_keeps_separate_hunks_and_expanded_context_after_revert() {
let mut review = crate::review::ReviewState::default();
let base = (0..100) let base = (0..100)
.map(|row| format!("line {row}")) .map(|row| format!("line {row}"))
.collect::<Vec<_>>(); .collect::<Vec<_>>();
@@ -3155,7 +3182,7 @@ fn live_reflow_keeps_separate_hunks_and_expanded_context_after_revert() {
let mut state = live_state(base.clone(), changed.clone(), true); let mut state = live_state(base.clone(), changed.clone(), true);
state.set_context_options(2, 5); state.set_context_options(2, 5);
changed.push(String::new()); changed.push(String::new());
state.refresh_after(&changed, 1); state.refresh_after(&changed, 1, &mut review);
assert_eq!(state.comparison.as_ref().unwrap().files[0].hunks.len(), 2); assert_eq!(state.comparison.as_ref().unwrap().files[0].hunks.len(), 2);
assert!( assert!(
state state
@@ -3167,7 +3194,7 @@ fn live_reflow_keeps_separate_hunks_and_expanded_context_after_revert() {
state.handle(Input::Char('[')); state.handle(Input::Char('['));
assert!(state.after_display_row(23).is_some()); assert!(state.after_display_row(23).is_some());
changed.insert(10, "inserted outside excerpts".into()); changed.insert(10, "inserted outside excerpts".into());
state.refresh_after(&changed, 2); state.refresh_after(&changed, 2, &mut review);
assert!(state.after_display_row(24).is_some()); assert!(state.after_display_row(24).is_some());
assert_eq!(state.comparison.as_ref().unwrap().files[0].hunks.len(), 3); assert_eq!(state.comparison.as_ref().unwrap().files[0].hunks.len(), 3);
assert!( assert!(
@@ -3178,7 +3205,7 @@ fn live_reflow_keeps_separate_hunks_and_expanded_context_after_revert() {
); );
let mut reverted = base; let mut reverted = base;
reverted.push(String::new()); reverted.push(String::new());
state.refresh_after(&reverted, 3); state.refresh_after(&reverted, 3, &mut review);
assert!(state.comparison.as_ref().unwrap().files[0].hunks.is_empty()); assert!(state.comparison.as_ref().unwrap().files[0].hunks.is_empty());
assert!(state.rows.iter().all(|row| row.kind == RowKind::Equal)); assert!(state.rows.iter().all(|row| row.kind == RowKind::Equal));
assert!(state.after_display_row(23).is_some()); assert!(state.after_display_row(23).is_some());
@@ -3186,21 +3213,22 @@ fn live_reflow_keeps_separate_hunks_and_expanded_context_after_revert() {
assert_eq!(state.files.as_ref().unwrap().entries().len(), 0); assert_eq!(state.files.as_ref().unwrap().entries().len(), 0);
assert_eq!(state.file, 0); assert_eq!(state.file, 0);
reverted[50] = "changed again".into(); reverted[50] = "changed again".into();
state.refresh_after(&reverted, 4); state.refresh_after(&reverted, 4, &mut review);
assert_eq!(state.files.as_ref().unwrap().entries().len(), 1); assert_eq!(state.files.as_ref().unwrap().entries().len(), 1);
assert!(state.after_display_row(50).is_some()); assert!(state.after_display_row(50).is_some());
} }
#[test] #[test]
fn live_empty_and_eof_rows_match_buffer_without_false_eof_highlights() { fn live_empty_and_eof_rows_match_buffer_without_false_eof_highlights() {
let mut review = crate::review::ReviewState::default();
let mut state = live_state(vec!["same".into()], vec!["changed".into()], true); let mut state = live_state(vec!["same".into()], vec!["changed".into()], true);
state.refresh_after(&["same".into(), "".into()], 1); state.refresh_after(&["same".into(), "".into()], 1, &mut review);
assert!(state.comparison.as_ref().unwrap().files[0].hunks.is_empty()); assert!(state.comparison.as_ref().unwrap().files[0].hunks.is_empty());
assert_eq!(state.rows.len(), 2); assert_eq!(state.rows.len(), 2);
assert!(state.rows.iter().all(|row| row.kind == RowKind::Equal)); assert!(state.rows.iter().all(|row| row.kind == RowKind::Equal));
assert!(state.after_display_row(0).is_some()); assert!(state.after_display_row(0).is_some());
assert!(state.after_display_row(1).is_none()); assert!(state.after_display_row(1).is_none());
state.refresh_after(&["same".into()], 2); state.refresh_after(&["same".into()], 2, &mut review);
assert_eq!(state.rows[0].kind, RowKind::Changed); assert_eq!(state.rows[0].kind, RowKind::Changed);
assert!( assert!(
state.comparison.as_ref().unwrap().files[0] state.comparison.as_ref().unwrap().files[0]
@@ -3209,18 +3237,18 @@ fn live_empty_and_eof_rows_match_buffer_without_false_eof_highlights() {
.unwrap() .unwrap()
.contains("After missing") .contains("After missing")
); );
state.refresh_after(&["".into()], 3); state.refresh_after(&["".into()], 3, &mut review);
assert!(state.after_display_row(0).is_some()); assert!(state.after_display_row(0).is_some());
assert_eq!(state.comparison.as_ref().unwrap().files[0].after, [""]); assert_eq!(state.comparison.as_ref().unwrap().files[0].after, [""]);
let mut empty = live_state(vec![], vec!["text".into()], false); let mut empty = live_state(vec![], vec!["text".into()], false);
empty.refresh_after(&["".into()], 1); empty.refresh_after(&["".into()], 1, &mut review);
assert!(empty.comparison.as_ref().unwrap().files[0].hunks.is_empty()); assert!(empty.comparison.as_ref().unwrap().files[0].hunks.is_empty());
assert_eq!(empty.rows.len(), 1); assert_eq!(empty.rows.len(), 1);
assert_eq!(empty.rows[0].kind, RowKind::Equal); assert_eq!(empty.rows[0].kind, RowKind::Equal);
assert_eq!(empty.after_display_row(0), Some(0)); assert_eq!(empty.after_display_row(0), Some(0));
empty.after.as_mut().unwrap().target = RevisionTarget::Commit("historic".into()); empty.after.as_mut().unwrap().target = RevisionTarget::Commit("historic".into());
empty.comparison.as_mut().unwrap().editable_paths.clear(); empty.comparison.as_mut().unwrap().editable_paths.clear();
empty.refresh_after(&["must not apply".into()], 2); empty.refresh_after(&["must not apply".into()], 2, &mut review);
assert_eq!(empty.comparison.as_ref().unwrap().files[0].after, [""]); assert_eq!(empty.comparison.as_ref().unwrap().files[0].after, [""]);
empty.handle(Input::Escape); empty.handle(Input::Escape);
assert_eq!(empty.phase, Phase::View); assert_eq!(empty.phase, Phase::View);
@@ -3372,6 +3400,7 @@ fn mode_action_keeps_filter_input_and_rejects_superseded_comparisons() {
#[test] #[test]
fn review_anchors_match_frozen_context_and_exact_live_eof_without_allocating() { fn review_anchors_match_frozen_context_and_exact_live_eof_without_allocating() {
let review = crate::review::ReviewState::default();
use crate::review::{Anchor, Side}; use crate::review::{Anchor, Side};
let mut state = live_state(vec!["old".into()], vec!["new".into()], true); let mut state = live_state(vec!["old".into()], vec!["new".into()], true);
let anchor = Anchor { let anchor = Anchor {
@@ -3382,15 +3411,15 @@ fn review_anchors_match_frozen_context_and_exact_live_eof_without_allocating() {
end: crate::plugin::TextPosition { row: 1, column: 0 }, end: crate::plugin::TextPosition { row: 1, column: 0 },
content: "new\n".into(), content: "new\n".into(),
}; };
assert!(state.review_anchor_range(&anchor).is_some()); assert!(state.review_anchor_range(&anchor, &review).is_some());
state.live_files.insert(0); state.live_files.insert(0);
assert!(state.review_anchor_range(&anchor).is_none()); assert!(state.review_anchor_range(&anchor, &review).is_none());
state.comparison.as_mut().unwrap().files[0] state.comparison.as_mut().unwrap().files[0]
.after .after
.push(String::new()); .push(String::new());
assert!(state.review_anchor_range(&anchor).is_some()); assert!(state.review_anchor_range(&anchor, &review).is_some());
state.comparison.as_mut().unwrap().files[0].after[0] = "changed".into(); state.comparison.as_mut().unwrap().files[0].after[0] = "changed".into();
assert!(state.review_anchor_range(&anchor).is_none()); assert!(state.review_anchor_range(&anchor, &review).is_none());
state.comparison.as_mut().unwrap().files[0].old_path = Some("original".into()); state.comparison.as_mut().unwrap().files[0].old_path = Some("original".into());
let before = Anchor { let before = Anchor {
revision: "base".into(), revision: "base".into(),
@@ -3399,12 +3428,12 @@ fn review_anchors_match_frozen_context_and_exact_live_eof_without_allocating() {
content: "old\n".into(), content: "old\n".into(),
..anchor ..anchor
}; };
assert!(state.review_anchor_range(&before).is_some()); assert!(state.review_anchor_range(&before, &review).is_some());
state.comparison.as_mut().unwrap().files[0].loaded = false; state.comparison.as_mut().unwrap().files[0].loaded = false;
assert!(state.review_anchor_range(&before).is_none()); assert!(state.review_anchor_range(&before, &review).is_none());
state.comparison.as_mut().unwrap().files[0].loaded = true; state.comparison.as_mut().unwrap().files[0].loaded = true;
state.comparison.as_mut().unwrap().revisions[0] = "other".into(); state.comparison.as_mut().unwrap().revisions[0] = "other".into();
assert!(state.review_anchor_range(&before).is_none()); assert!(state.review_anchor_range(&before, &review).is_none());
} }
#[test] #[test]
+188 -155
View File
@@ -3139,6 +3139,160 @@ fn historical_diff(editor: &mut Editor, root: &Path, before: Vec<String>, after:
assert!(editor.diff_interacting()); assert!(editor.diff_interacting());
} }
fn source_review_editor(directory: &Directory, text: &str) -> (Editor, PathBuf, PathBuf) {
let root = directory.0.canonicalize().unwrap();
assert!(
std::process::Command::new("git")
.args(["init", "-q", "--template="])
.current_dir(&root)
.status()
.unwrap()
.success()
);
let path = root.join("source.txt");
fs::write(&path, text).unwrap();
let mut editor = Editor::open(Some(path.clone()), vec!["text".into()]).unwrap();
editor.set_clipboard_mode("separate").unwrap();
editor.set_review_directory(root.join("reviews"));
(editor, root, path)
}
#[test]
fn source_review_drafts_guard_source_pane_close_and_saved_panels_retarget() {
let directory = Directory::new();
let (mut editor, _, _) = source_review_editor(&directory, "source\n");
let source = editor.pane_id;
editor.split_pane(SplitAxis::Vertical);
let other = editor.pane_id;
editor.select_pane(source);
editor.execute("comment");
type_text(&mut editor, "iDraft");
editor.handle(Input::Escape);
editor.select_pane(source);
editor.close_pane(false);
assert!(editor.panes.contains_key(&source));
editor.execute("review.save");
editor.close_pane(false);
assert!(!editor.panes.contains_key(&source));
assert_eq!(editor.pane_id, other);
editor.execute("review.edit 1");
assert_eq!(editor.context().text, "Draft");
editor.handle(Input::Escape);
assert_eq!(editor.pane_id, other);
assert_eq!(editor.context().text, "source\n");
}
#[test]
fn source_comments_capture_unsaved_exact_unicode_and_reopen_without_a_diff() {
let directory = Directory::new();
let (mut editor, root, path) = source_review_editor(&directory, "disk\n");
assert!(!root.join("reviews").exists());
let source = "first\nprefix e\u{301}界 suffix\nlast\n";
editor.apply(vec![Effect::Replace(source.into())]).unwrap();
set_carets(&mut editor, &[(1, 7)], 0);
type_text(&mut editor, "vl");
editor.execute("comment");
assert!(editor.review_panel().is_some());
assert!(editor.workspace_diff().is_none());
type_text(&mut editor, "iExact feedback");
editor.handle(Input::Escape);
editor.execute("w");
let review = editor.local_review.document.clone();
let anchor = review.comments[0].anchor.clone();
assert_eq!(anchor.excerpt(), "e\u{301}界");
assert_eq!(anchor.content, source);
assert!(anchor.revision.starts_with("working:"));
assert_eq!(fs::read_to_string(&path).unwrap(), "disk\n");
editor.handle(Input::Escape);
assert_eq!(editor.context().text, source);
editor.execute("w");
drop(editor);
let mut reopened = Editor::open(Some(path), vec!["text".into()]).unwrap();
reopened.set_review_directory(root.join("reviews"));
assert!(reopened.workspace_diff().is_none());
assert_eq!(reopened.local_review.document.storage_id, review.storage_id);
reopened.execute("review.next");
assert_eq!(reopened.context().text, "Exact feedback");
assert!(reopened.review_panel_title().unwrap().contains("current"));
reopened.handle(Input::Escape);
assert_eq!(reopened.cursor(), Cursor { row: 1, column: 7 });
assert_eq!(reopened.local_review.document.comments[0].anchor, anchor);
}
#[test]
fn source_comment_dismissal_conflicts_tabs_and_stale_snapshots_preserve_text() {
let directory = Directory::new();
let (mut editor, root, path) = source_review_editor(&directory, "original line\n");
editor.execute("comment");
type_text(&mut editor, "iDiscarded");
editor.handle(Input::Escape);
editor.handle(Input::Escape);
assert!(editor.local_review.document.comments.is_empty());
editor.execute("comment");
type_text(&mut editor, "iSaved feedback");
editor.handle(Input::Escape);
editor.execute("w");
editor.handle(Input::Escape);
let anchor = editor.local_review.document.comments[0].anchor.clone();
assert_eq!(anchor.excerpt(), "original line\n");
editor.execute("tab.new");
editor.execute("review.edit 1");
assert_eq!(editor.context().text, "Saved feedback");
editor.handle(Input::Escape);
editor.execute("tab.previous");
editor
.apply(vec![Effect::Replace("entirely changed\n".into())])
.unwrap();
editor.execute("review.edit 1");
assert!(editor.message().contains("stale"));
assert!(
editor
.review_panel_title()
.unwrap()
.contains("original snapshot")
);
editor.handle(Input::Escape);
assert_eq!(editor.context().text, "original line\n");
assert!(editor.active.resolver_readonly);
assert_eq!(editor.local_review.document.comments[0].anchor, anchor);
assert_eq!(fs::read_to_string(&path).unwrap(), "original line\n");
let mut concurrent = Editor::open(Some(path), vec!["text".into()]).unwrap();
concurrent.set_review_directory(root.join("reviews"));
concurrent.execute("review.verdict looks_good");
editor.execute("review.edit 1");
type_text(&mut editor, "A unsaved");
editor.handle(Input::Escape);
editor.execute("w");
assert!(editor.message().contains("changed in another session"));
assert_eq!(editor.context().text, "Saved feedback unsaved");
assert!(editor.current_review_dirty());
}
#[test]
fn source_comments_reject_multiple_carets_and_roundtrip_json_with_empty_lines() {
let directory = Directory::new();
let (mut editor, root, _) = source_review_editor(&directory, "one\n\n");
set_carets(&mut editor, &[(0, 0), (1, 0)], 0);
editor.execute("comment");
assert!(editor.message().contains("one caret"));
assert!(editor.review_panel().is_none());
set_carets(&mut editor, &[(1, 0)], 0);
editor.execute("comment");
type_text(&mut editor, "iEmpty line");
editor.handle(Input::Escape);
let export = root.join("review.json");
editor.execute(&format!("review.save {}", export.display()));
let exported = crate::review::Review::load(&export, &root).unwrap();
assert_eq!(exported.comments[0].anchor.excerpt(), "\n");
assert_eq!(exported.comments[0].body, "Empty line");
editor.handle(Input::Escape);
editor.execute(&format!("review.open {}", export.display()));
assert_eq!(
editor.local_review.document.comments[0].anchor,
exported.comments[0].anchor
);
}
#[test] #[test]
fn unsaved_review_comments_are_discarded_on_escape() { fn unsaved_review_comments_are_discarded_on_escape() {
let directory = Directory::new(); let directory = Directory::new();
@@ -3151,15 +3305,7 @@ fn unsaved_review_comments_are_discarded_on_escape() {
type_text(&mut editor, "iDiscard me"); type_text(&mut editor, "iDiscard me");
editor.handle(Input::Escape); editor.handle(Input::Escape);
editor.handle(Input::Escape); editor.handle(Input::Escape);
assert!( assert!(editor.local_review.document.comments.is_empty());
editor
.workspace_diff()
.unwrap()
.local_review
.document
.comments
.is_empty()
);
assert!( assert!(
crate::review::Store::open(&storage) crate::review::Store::open(&storage)
.unwrap() .unwrap()
@@ -3299,7 +3445,7 @@ fn review_navigation_maps_working_ranges_and_preserves_stale_snapshots() {
.as_mut() .as_mut()
.unwrap() .unwrap()
.revisions[1] = "working:old".into(); .revisions[1] = "working:old".into();
editor.diff.as_mut().unwrap().local_review.document = crate::review::Review::new(root); editor.local_review.document = crate::review::Review::new(root);
let anchor = Anchor { let anchor = Anchor {
revision: "working:old".into(), revision: "working:old".into(),
path: "file.txt".into(), path: "file.txt".into(),
@@ -3308,14 +3454,7 @@ fn review_navigation_maps_working_ranges_and_preserves_stale_snapshots() {
end: TextPosition { row: 1, column: 13 }, end: TextPosition { row: 1, column: 13 },
content: editor.lines().join("\n"), content: editor.lines().join("\n"),
}; };
editor editor.local_review.document.comments.push(Comment {
.diff
.as_mut()
.unwrap()
.local_review
.document
.comments
.push(Comment {
id: 1, id: 1,
anchor: anchor.clone(), anchor: anchor.clone(),
body: "Feedback".into(), body: "Feedback".into(),
@@ -3339,10 +3478,7 @@ fn review_navigation_maps_working_ranges_and_preserves_stale_snapshots() {
let owner = editor.diff_owner.unwrap(); let owner = editor.diff_owner.unwrap();
editor.select_pane(owner); editor.select_pane(owner);
assert_eq!(editor.cursor(), Cursor { row: 2, column: 14 }); assert_eq!(editor.cursor(), Cursor { row: 2, column: 14 });
assert_eq!( assert_eq!(editor.local_review.document.comments[0].anchor, anchor);
editor.diff.as_ref().unwrap().local_review.document.comments[0].anchor,
anchor
);
editor editor
.apply(vec![Effect::Replace( .apply(vec![Effect::Replace(
@@ -3355,10 +3491,7 @@ fn review_navigation_maps_working_ranges_and_preserves_stale_snapshots() {
assert!(editor.message().contains("stale")); assert!(editor.message().contains("stale"));
editor.select_pane(owner); editor.select_pane(owner);
assert_eq!(editor.cursor(), Cursor::default()); assert_eq!(editor.cursor(), Cursor::default());
assert_eq!( assert_eq!(editor.local_review.document.comments[0].anchor, anchor);
editor.diff.as_ref().unwrap().local_review.document.comments[0].anchor,
anchor
);
} }
#[test] #[test]
@@ -3388,13 +3521,7 @@ fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
); );
first.execute("w"); first.execute("w");
first.handle(Input::Escape); first.handle(Input::Escape);
let id = first let id = first.local_review.document.storage_id.unwrap();
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id
.unwrap();
let store = Store::open(&storage).unwrap(); let store = Store::open(&storage).unwrap();
let persisted = store.load(&root, id).unwrap(); let persisted = store.load(&root, id).unwrap();
assert_eq!(persisted.comments[0].body, "First feedback"); assert_eq!(persisted.comments[0].body, "First feedback");
@@ -3437,58 +3564,30 @@ fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
vec!["different revision".into()], vec!["different revision".into()],
vec!["changed".into()], vec!["changed".into()],
); );
let restored = &reopened.workspace_diff().unwrap().local_review.document; let restored = &reopened.local_review.document;
assert_eq!(restored.storage_id, Some(id)); assert_eq!(restored.storage_id, Some(id));
assert_eq!(restored.comments[0].body, "First feedback revised"); assert_eq!(restored.comments[0].body, "First feedback revised");
assert!( assert!(
reopened reopened
.workspace_diff() .workspace_diff()
.unwrap() .unwrap()
.review_anchor_range(&restored.comments[0].anchor) .review_anchor_range(&restored.comments[0].anchor, &reopened.local_review)
.is_none() .is_none()
); );
reopened.execute("diff.review.new"); reopened.execute("diff.review.new");
let second_id = reopened let second_id = reopened.local_review.document.storage_id.unwrap();
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id
.unwrap();
assert_ne!(id, second_id); assert_ne!(id, second_id);
let mut choosing = editor(); let mut choosing = editor();
choosing.set_review_directory(storage); choosing.set_review_directory(storage);
historical_diff(&mut choosing, &root, vec!["old".into()], vec!["new".into()]); historical_diff(&mut choosing, &root, vec!["old".into()], vec!["new".into()]);
choosing.handle(Input::Enter); choosing.handle(Input::Enter);
assert_eq!( assert_eq!(choosing.local_review.document.storage_id, Some(second_id));
choosing
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id,
Some(second_id)
);
choosing.execute(&format!("diff.review.open {id}")); choosing.execute(&format!("diff.review.open {id}"));
assert_eq!( assert_eq!(choosing.local_review.document.storage_id, Some(id));
choosing
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id,
Some(id)
);
choosing.execute("diff.review.delete 1"); choosing.execute("diff.review.delete 1");
assert!(store.load(&root, id).unwrap().comments.is_empty()); assert!(store.load(&root, id).unwrap().comments.is_empty());
choosing.execute(&format!("diff.review.open {}", exported.display())); choosing.execute(&format!("diff.review.open {}", exported.display()));
let imported_id = choosing let imported_id = choosing.local_review.document.storage_id.unwrap();
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id
.unwrap();
assert_ne!(imported_id, id); assert_ne!(imported_id, id);
assert_eq!( assert_eq!(
store.load(&root, imported_id).unwrap().comments[0].body, store.load(&root, imported_id).unwrap().comments[0].body,
@@ -3523,7 +3622,7 @@ fn review_resolution_persists_without_hiding_comments_or_changing_stale_anchors(
.contains("Comment #1 · resolved · current") .contains("Comment #1 · resolved · current")
); );
let store = Store::open(&storage).unwrap(); let store = Store::open(&storage).unwrap();
let document = &first.workspace_diff().unwrap().local_review.document; let document = &first.local_review.document;
let id = document.storage_id.unwrap(); let id = document.storage_id.unwrap();
let anchor = document.comments[0].anchor.clone(); let anchor = document.comments[0].anchor.clone();
assert!(store.load(&root, id).unwrap().comments[0].resolved); assert!(store.load(&root, id).unwrap().comments[0].resolved);
@@ -3557,25 +3656,8 @@ fn review_resolution_persists_without_hiding_comments_or_changing_stale_anchors(
assert!(!store.load(&root, id).unwrap().comments[0].resolved); assert!(!store.load(&root, id).unwrap().comments[0].resolved);
stale.execute("diff.review.unresolve 1"); stale.execute("diff.review.unresolve 1");
assert!(stale.current_review_dirty()); assert!(stale.current_review_dirty());
assert!( assert!(!stale.local_review.document.comments[0].resolved);
!stale assert_eq!(stale.local_review.document.comments[0].anchor, anchor);
.workspace_diff()
.unwrap()
.local_review
.document
.comments[0]
.resolved
);
assert_eq!(
stale
.workspace_diff()
.unwrap()
.local_review
.document
.comments[0]
.anchor,
anchor
);
assert!(stale.message().contains("changed in another session")); assert!(stale.message().contains("changed in another session"));
first.execute("diff.review.resolve"); first.execute("diff.review.resolve");
assert!( assert!(
@@ -3592,7 +3674,7 @@ fn review_resolution_persists_without_hiding_comments_or_changing_stale_anchors(
.contains("Comment #1 · unresolved · current") .contains("Comment #1 · unresolved · current")
); );
first.execute(&format!("diff.review.open {}", export.display())); first.execute(&format!("diff.review.open {}", export.display()));
let imported = &first.workspace_diff().unwrap().local_review.document; let imported = &first.local_review.document;
assert_ne!(imported.storage_id, Some(id)); assert_ne!(imported.storage_id, Some(id));
assert!( assert!(
store store
@@ -3617,34 +3699,15 @@ fn sqlite_review_conflicts_and_write_failures_keep_dirty_drafts() {
first.handle(Input::Escape); first.handle(Input::Escape);
first.execute("w"); first.execute("w");
first.handle(Input::Escape); first.handle(Input::Escape);
let id = first let id = first.local_review.document.storage_id.unwrap();
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id
.unwrap();
let mut stale = editor(); let mut stale = editor();
stale.set_review_directory(storage.clone()); stale.set_review_directory(storage.clone());
historical_diff(&mut stale, &root, vec!["old".into()], vec!["new".into()]); historical_diff(&mut stale, &root, vec!["old".into()], vec!["new".into()]);
let version = stale let version = stale.local_review.document.storage_revision;
.workspace_diff()
.unwrap()
.local_review
.document
.storage_revision;
first.execute("diff.review.verdict looks_good"); first.execute("diff.review.verdict looks_good");
stale.execute("diff.review.verdict changes_requested"); stale.execute("diff.review.verdict changes_requested");
assert!(stale.current_review_dirty()); assert!(stale.current_review_dirty());
assert_eq!( assert_eq!(stale.local_review.document.storage_revision, version);
stale
.workspace_diff()
.unwrap()
.local_review
.document
.storage_revision,
version
);
assert_eq!( assert_eq!(
Store::open(&storage) Store::open(&storage)
.unwrap() .unwrap()
@@ -3665,13 +3728,7 @@ fn sqlite_review_conflicts_and_write_failures_keep_dirty_drafts() {
first.handle(Input::Escape); first.handle(Input::Escape);
assert!(!first.current_review_dirty()); assert!(!first.current_review_dirty());
assert_eq!( assert_eq!(
first first.local_review.document.comments[0].body,
.workspace_diff()
.unwrap()
.local_review
.document
.comments[0]
.body,
"Stored feedback" "Stored feedback"
); );
assert_eq!(fs::read_to_string(blocked).unwrap(), "preserve"); assert_eq!(fs::read_to_string(blocked).unwrap(), "preserve");
@@ -3843,7 +3900,7 @@ fn precise_review_ranges_capture_graphemes_and_navigate_to_their_start() {
root.join("range.json").display() root.join("range.json").display()
)); ));
let diff = editor.workspace_diff().unwrap(); let diff = editor.workspace_diff().unwrap();
let anchor = &diff.local_review.document.comments[0].anchor; let anchor = &editor.local_review.document.comments[0].anchor;
let (start, end) = if keys == "llvj" { let (start, end) = if keys == "llvj" {
( (
TextPosition { row: 0, column: 2 }, TextPosition { row: 0, column: 2 },
@@ -3857,7 +3914,10 @@ fn precise_review_ranges_capture_graphemes_and_navigate_to_their_start() {
}; };
assert_eq!(anchor.start, start); assert_eq!(anchor.start, start);
assert_eq!(anchor.end, end); assert_eq!(anchor.end, end);
assert!(diff.review_anchor_range(anchor).is_some()); assert!(
diff.review_anchor_range(anchor, &editor.local_review)
.is_some()
);
editor.handle(Input::Escape); editor.handle(Input::Escape);
assert_eq!(editor.pane_id(), owner); assert_eq!(editor.pane_id(), owner);
assert_eq!( assert_eq!(
@@ -3881,13 +3941,7 @@ fn precise_review_ranges_capture_graphemes_and_navigate_to_their_start() {
"diff.review.save {}", "diff.review.save {}",
root.join("range.json").display() root.join("range.json").display()
)); ));
let anchor = &editor let anchor = &editor.local_review.document.comments[0].anchor;
.workspace_diff()
.unwrap()
.local_review
.document
.comments[0]
.anchor;
assert_eq!(anchor.start, TextPosition { row: 0, column: 0 }); assert_eq!(anchor.start, TextPosition { row: 0, column: 0 });
assert_eq!( assert_eq!(
anchor.end, anchor.end,
@@ -3975,7 +4029,7 @@ fn local_review_visual_commands_and_palette_preserve_both_side_ranges() {
root.join("range.json").display() root.join("range.json").display()
)); ));
let diff = editor.workspace_diff().unwrap(); let diff = editor.workspace_diff().unwrap();
let anchor = diff.local_review.document.comments[0].anchor.clone(); let anchor = editor.local_review.document.comments[0].anchor.clone();
assert_eq!(anchor.start, TextPosition { row: 0, column: 0 }); assert_eq!(anchor.start, TextPosition { row: 0, column: 0 });
assert_eq!(anchor.end, TextPosition { row: 2, column: 0 }); assert_eq!(anchor.end, TextPosition { row: 2, column: 0 });
assert_eq!( assert_eq!(
@@ -3986,7 +4040,10 @@ fn local_review_visual_commands_and_palette_preserve_both_side_ranges() {
Side::After Side::After
} }
); );
assert!(diff.review_anchor_range(&anchor).is_some()); assert!(
diff.review_anchor_range(&anchor, &editor.local_review)
.is_some()
);
let source_pane = editor.diff_owner.unwrap(); let source_pane = editor.diff_owner.unwrap();
editor.select_pane(source_pane); editor.select_pane(source_pane);
editor editor
@@ -4000,16 +4057,7 @@ fn local_review_visual_commands_and_palette_preserve_both_side_ranges() {
editor.execute("diff.review.overview"); editor.execute("diff.review.overview");
assert!(editor.context().text.contains("stale")); assert!(editor.context().text.contains("stale"));
editor.execute("diff.review.edit 1"); editor.execute("diff.review.edit 1");
assert_eq!( assert_eq!(editor.local_review.document.comments[0].anchor, anchor);
editor
.workspace_diff()
.unwrap()
.local_review
.document
.comments[0]
.anchor,
anchor
);
} }
} }
@@ -4045,15 +4093,7 @@ fn local_review_guards_metadata_and_hidden_native_drafts_against_loss() {
let invalid = root.join("missing/review.json"); let invalid = root.join("missing/review.json");
editor.execute(&format!("diff.review.save {}", invalid.display())); editor.execute(&format!("diff.review.save {}", invalid.display()));
assert!(editor.local_reviews_dirty()); assert!(editor.local_reviews_dirty());
assert!( assert!(editor.local_review.document.comments.is_empty());
editor
.workspace_diff()
.unwrap()
.local_review
.document
.comments
.is_empty()
);
assert!( assert!(
editor editor
.parked .parked
@@ -7633,14 +7673,7 @@ mod system_clipboard {
} }
editor.execute("diff.review.copy unexpected"); editor.execute("diff.review.copy unexpected");
editor.with_pane(editor.diff_owner.unwrap(), |editor| { editor.with_pane(editor.diff_owner.unwrap(), |editor| {
editor editor.local_review.document.comments.clear();
.diff
.as_mut()
.unwrap()
.local_review
.document
.comments
.clear();
}); });
editor.execute("diff.review.copy"); editor.execute("diff.review.copy");
if mode == "system" { if mode == "system" {
+27
View File
@@ -626,6 +626,33 @@ fn config_leader_defaults_custom_keys_validation_and_required_wire_field() {
} }
} }
#[test]
fn source_review_actions_and_diff_command_aliases_share_the_core_catalog() {
let fixture = Fixture::new();
let path = fixture.file(
"config.rb",
r#"
Vamp.configure do |config|
config.bind "C", config.core.comment
config.bind "N", config.core.review.next
config.bind "S", config.core.review.save
raise "missing diff comment alias" unless Vamp.builtin_command?("diff.comment")
raise "missing diff review alias" unless Vamp.builtin_command?("diff.review.edit")
raise "mutable review action" unless config.core.review.frozen? && config.core.comment.frozen?
end
"#,
);
let loaded = load_config(&path).unwrap();
let actions: Vec<_> = loaded
.bindings
.iter()
.filter_map(|binding| binding.command.as_deref())
.collect();
assert!(actions.contains(&"core.comment"));
assert!(actions.contains(&"core.review.next"));
assert!(actions.contains(&"core.review.save"));
}
#[test] #[test]
fn git_gutter_config_modes_default_and_rejection() { fn git_gutter_config_modes_default_and_rejection() {
use crate::gutter::GutterStyle; use crate::gutter::GutterStyle;
+115 -26
View File
@@ -2951,6 +2951,62 @@ fn diff_status_layouts_hide_absent_panes_and_leave_unchanged_renames_blank() {
assert!(text.contains("After · M")); assert!(text.contains("After · M"));
} }
#[test]
fn source_review_markers_highlight_exact_text_and_reject_stale_ranges() {
use crate::editor::Effect;
use crate::plugin::TextPosition;
use crate::review::{Anchor, Comment, Review, Side};
for wrapped in [false, true] {
let directory = Directory::new();
let text = format!("aé界{}\nsecond\n", " trailing".repeat(10));
let path = directory.file("source.txt", &text);
let mut editor = Editor::open(Some(path), vec!["text".into()]).unwrap();
editor.local_review.document = Review::new(directory.0.canonicalize().unwrap());
editor.local_review.document.comments.push(Comment {
id: 1,
anchor: Anchor {
revision: "working:source-snapshot".into(),
path: "source.txt".into(),
side: Side::After,
start: TextPosition { row: 0, column: 1 },
end: TextPosition { row: 0, column: 3 },
content: text,
},
body: "Feedback".into(),
resolved: false,
});
editor.local_review.selected = Some(1);
if wrapped {
editor.execute("word_wrap.toggle");
}
let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(40, 16)).unwrap();
let mut views = BufferViewports::default();
terminal
.draw(|frame| views.draw(frame, &mut editor))
.unwrap();
assert!(screen_text(&terminal).contains('●'));
assert!(
terminal
.backend()
.buffer()
.content
.iter()
.filter(|cell| cell.bg == Color::Magenta)
.count()
>= 2
);
let anchor = editor.local_review.document.comments[0].anchor.clone();
editor
.apply(vec![Effect::Replace("changed entirely\n".into())])
.unwrap();
terminal
.draw(|frame| views.draw(frame, &mut editor))
.unwrap();
assert!(!screen_text(&terminal).contains('●'));
assert_eq!(editor.local_review.document.comments[0].anchor, anchor);
}
}
fn render_diff( fn render_diff(
terminal: &mut Terminal<ratatui::backend::TestBackend>, terminal: &mut Terminal<ratatui::backend::TestBackend>,
diff: &DiffState, diff: &DiffState,
@@ -2959,12 +3015,44 @@ fn render_diff(
terminal terminal
.draw(|frame| { .draw(|frame| {
panels.begin_frame(); panels.begin_frame();
draw_diff(frame, diff, frame.area(), "↑/↓", "h/j/k/l", panels, None); draw_diff(
frame,
diff,
frame.area(),
"↑/↓",
"h/j/k/l",
panels,
None,
None,
);
}) })
.unwrap(); .unwrap();
} }
fn commented_diff() -> DiffState { fn render_review_diff(
terminal: &mut Terminal<ratatui::backend::TestBackend>,
diff: &DiffState,
review: &crate::review::ReviewState,
panels: &mut WheelPanels,
) {
terminal
.draw(|frame| {
panels.begin_frame();
draw_diff(
frame,
diff,
frame.area(),
"↑/↓",
"h/j/k/l",
panels,
None,
Some(review),
);
})
.unwrap();
}
fn commented_diff() -> (DiffState, crate::review::ReviewState) {
use crate::review::{Anchor, Comment, Side}; use crate::review::{Anchor, Comment, Side};
let before = format!( let before = format!(
"{}\nold e\u{301}界 second\nkeep", "{}\nold e\u{301}界 second\nkeep",
@@ -2980,8 +3068,9 @@ fn commented_diff() -> DiffState {
file.old_path = Some("old.txt".into()); file.old_path = Some("old.txt".into());
let mut diff = diff_comparison(vec![file]); let mut diff = diff_comparison(vec![file]);
diff.files_visible = false; diff.files_visible = false;
diff.local_review.document.root = diff.comparison.as_ref().unwrap().root.clone(); let mut review = crate::review::ReviewState::default();
diff.local_review.document.comments = vec![ review.document.root = diff.comparison.as_ref().unwrap().root.clone();
review.document.comments = vec![
Comment { Comment {
id: 1, id: 1,
anchor: Anchor { anchor: Anchor {
@@ -3009,8 +3098,8 @@ fn commented_diff() -> DiffState {
resolved: false, resolved: false,
}, },
]; ];
diff.local_review.selected = Some(1); review.selected = Some(1);
diff (diff, review)
} }
#[test] #[test]
@@ -3022,14 +3111,13 @@ fn local_review_highlights_exact_graphemes_newlines_clips_and_wrap_continuations
for side in [Side::Before, Side::After] { for side in [Side::Before, Side::After] {
for (wrapping, horizontal) in [(false, 0), (false, 6), (true, 0)] { for (wrapping, horizontal) in [(false, 0), (false, 6), (true, 0)] {
for (start, end) in [((0, 4), (0, 6)), ((0, 5), (1, 5)), ((0, 84), (1, 0))] { for (start, end) in [((0, 4), (0, 6)), ((0, 5), (1, 5)), ((0, 84), (1, 0))] {
let mut diff = commented_diff(); let (mut diff, mut review) = commented_diff();
diff.set_view_mode(mode); diff.set_view_mode(mode);
diff.top = 0; diff.top = 0;
diff.word_wrap = wrapping; diff.word_wrap = wrapping;
diff.left_before = horizontal; diff.left_before = horizontal;
diff.left_after = horizontal; diff.left_after = horizontal;
let comment = diff let comment = review
.local_review
.document .document
.comments .comments
.iter_mut() .iter_mut()
@@ -3044,11 +3132,11 @@ fn local_review_highlights_exact_graphemes_newlines_clips_and_wrap_continuations
column: end.1, column: end.1,
}; };
let anchor = comment.anchor.clone(); let anchor = comment.anchor.clone();
diff.local_review.selected = Some(comment.id); review.selected = Some(comment.id);
let mut terminal = let mut terminal =
Terminal::new(ratatui::backend::TestBackend::new(90, 40)).unwrap(); Terminal::new(ratatui::backend::TestBackend::new(90, 40)).unwrap();
let mut panels = WheelPanels::default(); let mut panels = WheelPanels::default();
render_diff(&mut terminal, &diff, &mut panels); render_review_diff(&mut terminal, &diff, &review, &mut panels);
let file = &diff.comparison.as_ref().unwrap().files[0]; let file = &diff.comparison.as_ref().unwrap().files[0];
let wrapped = panels let wrapped = panels
.diff_wrap .diff_wrap
@@ -3164,13 +3252,13 @@ fn local_review_highlights_exact_graphemes_newlines_clips_and_wrap_continuations
fn local_review_markers_preserve_diff_signs_ranges_and_wrapped_geometry() { fn local_review_markers_preserve_diff_signs_ranges_and_wrapped_geometry() {
for mode in [ViewMode::Split, ViewMode::Unified] { for mode in [ViewMode::Split, ViewMode::Unified] {
for wrapping in [false, true] { for wrapping in [false, true] {
let mut diff = commented_diff(); let (mut diff, review) = commented_diff();
diff.set_view_mode(mode); diff.set_view_mode(mode);
diff.top = 0; diff.top = 0;
diff.word_wrap = wrapping; diff.word_wrap = wrapping;
let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(90, 40)).unwrap(); let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(90, 40)).unwrap();
let mut panels = WheelPanels::default(); let mut panels = WheelPanels::default();
render_diff(&mut terminal, &diff, &mut panels); render_review_diff(&mut terminal, &diff, &review, &mut panels);
let buffer = terminal.backend().buffer(); let buffer = terminal.backend().buffer();
let markers: Vec<_> = buffer let markers: Vec<_> = buffer
.content .content
@@ -3219,20 +3307,20 @@ fn local_review_markers_preserve_diff_signs_ranges_and_wrapped_geometry() {
fn local_review_markers_reject_stale_identity_text_and_rename_paths() { fn local_review_markers_reject_stale_identity_text_and_rename_paths() {
for mode in [ViewMode::Split, ViewMode::Unified] { for mode in [ViewMode::Split, ViewMode::Unified] {
for mismatch in ["revision", "content", "path", "side", "root"] { for mismatch in ["revision", "content", "path", "side", "root"] {
let mut diff = commented_diff(); let (mut diff, mut review) = commented_diff();
diff.set_view_mode(mode); diff.set_view_mode(mode);
diff.local_review.document.comments.truncate(1); review.document.comments.truncate(1);
let anchor = &mut diff.local_review.document.comments[0].anchor; let anchor = &mut review.document.comments[0].anchor;
match mismatch { match mismatch {
"revision" => anchor.revision.push_str("-newer"), "revision" => anchor.revision.push_str("-newer"),
"content" => anchor.content.replace_range(..1, "X"), "content" => anchor.content.replace_range(..1, "X"),
"path" => anchor.path = "new.txt".into(), "path" => anchor.path = "new.txt".into(),
"side" => anchor.side = crate::review::Side::After, "side" => anchor.side = crate::review::Side::After,
"root" => diff.local_review.document.root = "another-repository".into(), "root" => review.document.root = "another-repository".into(),
_ => unreachable!(), _ => unreachable!(),
} }
let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(90, 20)).unwrap(); let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(90, 20)).unwrap();
render_diff(&mut terminal, &diff, &mut WheelPanels::default()); render_review_diff(&mut terminal, &diff, &review, &mut WheelPanels::default());
assert!( assert!(
terminal terminal
.backend() .backend()
@@ -3258,18 +3346,18 @@ fn local_review_markers_reject_stale_identity_text_and_rename_paths() {
#[test] #[test]
fn unified_context_shows_comments_from_either_snapshot_side() { fn unified_context_shows_comments_from_either_snapshot_side() {
for side in [crate::review::Side::Before, crate::review::Side::After] { for side in [crate::review::Side::Before, crate::review::Side::After] {
let mut diff = commented_diff(); let (mut diff, mut review) = commented_diff();
diff.set_view_mode(ViewMode::Unified); diff.set_view_mode(ViewMode::Unified);
diff.local_review review
.document .document
.comments .comments
.retain(|comment| comment.anchor.side == side); .retain(|comment| comment.anchor.side == side);
let comment = &mut diff.local_review.document.comments[0]; let comment = &mut review.document.comments[0];
comment.anchor.start = crate::plugin::TextPosition { row: 2, column: 1 }; comment.anchor.start = crate::plugin::TextPosition { row: 2, column: 1 };
comment.anchor.end = crate::plugin::TextPosition { row: 2, column: 3 }; comment.anchor.end = crate::plugin::TextPosition { row: 2, column: 3 };
diff.local_review.selected = Some(comment.id); review.selected = Some(comment.id);
let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(90, 20)).unwrap(); let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(90, 20)).unwrap();
render_diff(&mut terminal, &diff, &mut WheelPanels::default()); render_review_diff(&mut terminal, &diff, &review, &mut WheelPanels::default());
assert!( assert!(
screen_text(&terminal).contains("●keep"), screen_text(&terminal).contains("●keep"),
"{side:?}: {}", "{side:?}: {}",
@@ -3296,13 +3384,13 @@ fn unified_context_shows_comments_from_either_snapshot_side() {
fn local_review_markers_yield_to_text_on_narrow_surfaces() { fn local_review_markers_yield_to_text_on_narrow_surfaces() {
for mode in [ViewMode::Split, ViewMode::Unified] { for mode in [ViewMode::Split, ViewMode::Unified] {
for (width, height) in [(1, 1), (2, 2), (3, 3), (8, 4), (16, 8), (30, 12)] { for (width, height) in [(1, 1), (2, 2), (3, 3), (8, 4), (16, 8), (30, 12)] {
let mut diff = commented_diff(); let (mut diff, review) = commented_diff();
diff.set_view_mode(mode); diff.set_view_mode(mode);
diff.word_wrap = true; diff.word_wrap = true;
let mut terminal = let mut terminal =
Terminal::new(ratatui::backend::TestBackend::new(width, height)).unwrap(); Terminal::new(ratatui::backend::TestBackend::new(width, height)).unwrap();
let mut panels = WheelPanels::default(); let mut panels = WheelPanels::default();
render_diff(&mut terminal, &diff, &mut panels); render_review_diff(&mut terminal, &diff, &review, &mut panels);
for (area, focus) in panels for (area, focus) in panels
.diff .diff
.as_ref() .as_ref()
@@ -3702,6 +3790,7 @@ fn pull_request_review_symbols_use_terminal_cells_and_keep_the_selection_arrow_i
"h/j/k/l", "h/j/k/l",
&mut panels, &mut panels,
with_editor.then_some(&editor), with_editor.then_some(&editor),
None,
); );
}) })
.unwrap(); .unwrap();
@@ -4834,7 +4923,7 @@ fn unavailable_file_documents_show_the_actual_error_in_managed_workspaces() {
}; };
terminal terminal
.draw(|frame| { .draw(|frame| {
panels.draw_documents(frame, &diff, frame.area(), None, None); panels.draw_documents(frame, &diff, frame.area(), None, None, None);
}) })
.unwrap(); .unwrap();
let text = screen_text(&terminal); let text = screen_text(&terminal);