feat: persist local reviews automatically in SQLite

This commit is contained in:
Valdemar Lorenzen
2026-10-16 15:15:19 +02:00
parent ef1c377167
commit b558f82094
15 changed files with 634 additions and 48 deletions
+5 -3
View File
@@ -13,7 +13,7 @@
- preserve immutable snapshot identity and existing stale detection; never silently reanchor comments after source changes. - preserve immutable snapshot identity and existing stale detection; never silently reanchor comments after source changes.
- persist precise ranges in version 2 JSON import/export. SQLite storage follows in the next item. - persist precise ranges in version 2 JSON import/export. SQLite storage follows in the next item.
- future GitHub publishing may map precise local anchors to supported line ranges without reducing precision in the local review. - future GitHub publishing may map precise local anchors to supported line ranges without reducing precision in the local review.
- [ ] persist local reviews automatically in a local SQLite database using rusqlite; no server, hosted storage, or synchronization. - [x] persist local reviews automatically in a local SQLite database using rusqlite; no server, hosted storage, or synchronization.
- keep one database under $XDG_DATA_HOME/vamp/reviews/, falling back to ~/.local/share/vamp/reviews/; allow a configurable review_directory. - keep one database under $XDG_DATA_HOME/vamp/reviews/, falling back to ~/.local/share/vamp/reviews/; allow a configurable review_directory.
- key reviews by repository identity and persistent review ID, preserving reviews and stale comments across revision changes. - key reviews by repository identity and persistent review ID, preserving reviews and stale comments across revision changes.
- autosave committed comment edits and summaries when dismissing or switching review editors; save verdicts and deletions immediately, using transactions rather than per-keystroke writes. - autosave committed comment edits and summaries when dismissing or switching review editors; save verdicts and deletions immediately, using transactions rather than per-keystroke writes.
@@ -22,13 +22,15 @@
- version the database schema; use review-version checks to reject conflicting edits from multiple editor instances. - version the database schema; use review-version checks to reject conflicting edits from multiple editor instances.
- surface write failures and conflicts without discarding drafts or marking unsuccessful saves as persisted. - surface write failures and conflicts without discarding drafts or marking unsuccessful saves as persisted.
- persist the precise start/end grapheme anchors without reducing them to line ranges. - persist the precise start/end grapheme anchors without reducing them to line ranges.
- [ ] add an option for three pane lef/base/right view for conflict resolution. - [ ] diff view: consider an MCP or a something for a local review loop when using coding agents.
- alternative pane layout as left/right with base underneath (configurable)
- [ ] diff view: GitHub code review integration after the local review UX is usable. - [ ] 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.
- keep authentication, network requests, publishing, and remote thread synchronization out of the local UX work; treat timed-out writes as unknown outcomes rather than automatically retrying. - keep authentication, network requests, publishing, and remote thread synchronization out of the local UX work; treat timed-out writes as unknown outcomes rather than automatically retrying.
- define follow-on scope for reading/replying to remote threads, editing/deleting published comments, and resolving threads. - define follow-on scope for reading/replying to remote threads, editing/deleting published comments, and resolving threads.
- [ ] add an option for three pane lef/base/right view for conflict resolution.
- alternative pane layout as left/right with base underneath (configurable)
- [ ] take a look at the configuration options and their names, and consider whether renaming some of them is warranted, as well as maybe nesting some under some common namespace (like some already are).
- [ ] remove the diff palette polling fallback once a released Herdr fix delivers palette-ready DEC 2031 notifications, including palette-only changes with an unchanged light/dark preference. Related upstream issue: [herdrdev/herdr#4183](https://github.com/herdrdev/herdr/issues/4183). - [ ] remove the diff palette polling fallback once a released Herdr fix delivers palette-ready DEC 2031 notifications, including palette-only changes with an unchanged light/dark preference. Related upstream issue: [herdrdev/herdr#4183](https://github.com/herdrdev/herdr/issues/4183).
- verify that notification-triggered queries return fresh colors in Ghostty through Herdr before removal; retain startup, focus-regain, and notification-driven palette refresh. - verify that notification-triggered queries return fresh colors in Ghostty through Herdr before removal; retain startup, focus-regain, and notification-driven palette refresh.
- [ ] pull a pr description and display it in a buffer: - [ ] pull a pr description and display it in a buffer:
+2 -2
View File
@@ -16,8 +16,8 @@ One editor crate; the workspace also includes its patched Crossterm dependency.
Virtual diff leaves retain their original catalog input index for caller-owned annotations. Normal scanned leaves accept optional repository status and old-path metadata without filtering unchanged files; missing tracked deletions add virtual leaves and required directories. Metadata-only updates preserve query, selection, expansion, and view without rebuilding the catalog. Virtual diff leaves retain their original catalog input index for caller-owned annotations. Normal scanned leaves accept optional repository status and old-path metadata without filtering unchanged files; missing tracked deletions add virtual leaves and required directories. Metadata-only updates preserve query, selection, expansion, and view without rebuilding the catalog.
- `diff`: owns searchable Before/After selection, category, focus and explorer visibility, request freshness, read-only comparison DTOs, aligned rows with cached grapheme-level change ranges, and shared vertical/independent horizontal offsets. Changed paths form a virtual `files::FilePicker` catalog, reusing the normal tree projection, filtering, and navigation without scanning disk. It has no subprocess or terminal dependencies. - `diff`: owns searchable Before/After selection, category, focus and explorer visibility, request freshness, read-only comparison DTOs, aligned rows with cached grapheme-level change ranges, and shared vertical/independent horizontal offsets. Changed paths form a virtual `files::FilePicker` catalog, reusing the normal tree projection, filtering, and navigation without scanning disk. It has no subprocess or terminal dependencies.
- `repository`: discovers jj/Git repositories and local/advertised remote revisions, builds disk snapshots and changed-file hunks, and owns isolated temporary comparison repositories with bounded, cancellable subprocesses. It never saves editor buffers or modifies active refs/checkouts. - `repository`: discovers jj/Git repositories and local/advertised remote revisions, builds disk snapshots and changed-file hunks, and owns isolated temporary comparison repositories with bounded, cancellable subprocesses. It never saves editor buffers or modifies active refs/checkouts.
- `review`: owns versioned local JSON review documents, immutable source anchors, validation, bounded reads, and atomic writes with external-change protection. It has no terminal or network operations. Comparison state owns the active review; `editor_review` keeps native comment/summary buffers and the read-only overview attached to their owning tab, synchronizes drafts on review commands, and guards unsaved teardown. The floating review editor has a tab-owned native pane outside the workspace `PaneTree`; rendering overlays it on the diff owner's rectangle without changing the source layout. Dismissal retains drafts and restores source focus. Working snapshot IDs hash the private captured paths, modes, contents, and notices; anchors also retain exact file text to account for live edits without reanchoring. - `review`: owns the validated local review model, exact immutable anchors, SQLite storage through bundled rusqlite, and atomic JSON import/export with external-change protection. `Store` uses schema-versioned transactions and compare-and-swap review revisions; repository roots and persistent review IDs are independent of comparison revisions. One bounded JSON document is stored per row, retaining exact grapheme endpoints and stale snapshots. Incompatible schemas and concurrent edits fail without replacing data or advancing in-memory saved state.
Version 2 anchors use the existing `plugin::TextPosition` type for zero-based grapheme start/end positions with an exclusive end. JSON loading accepts only the current schema and retains original disk bytes for external-write checks. Selected-comment painting reuses source-side grapheme clipping and wrapping; native selections retain higher priority. `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.
- `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.
- `syntax`: owns Tree-sitter grammar detection, per-buffer incremental trees, bundled highlight/local/injection queries, and syntax byte spans; it has no terminal color/geometry or editing dependency. - `syntax`: owns Tree-sitter grammar detection, per-buffer incremental trees, bundled highlight/local/injection queries, and syntax byte spans; it has no terminal color/geometry or editing dependency.
- `markdown`: owns native Markdown link/reference resolution, local destination decoding, and heading fragment lookup through the installed block/inline grammars. It has no editor lifecycle, terminal, LSP, or external-process dependency. - `markdown`: owns native Markdown link/reference resolution, local destination decoding, and heading fragment lookup through the installed block/inline grammars. It has no editor lifecycle, terminal, LSP, or external-process dependency.
+3 -2
View File
@@ -30,6 +30,7 @@ Vamp.configure do |config|
config.diff_view_mode = "split" # or "unified" config.diff_view_mode = "split" # or "unified"
config.diff_split_orientation = "vertical" # or "horizontal" config.diff_split_orientation = "vertical" # or "horizontal"
config.diff_follow_working_copy = true config.diff_follow_working_copy = true
config.review_directory = nil # XDG data directory; override with an absolute or config-relative path
config.bind "ctrl+h", config.core.left, mode: config.modes.tree config.bind "ctrl+h", config.core.left, mode: config.modes.tree
config.bind "ctrl+l", config.core.right, mode: config.modes.tree config.bind "ctrl+l", config.core.right, mode: config.modes.tree
config.plugin "plugins/greeting.rb" config.plugin "plugins/greeting.rb"
@@ -229,7 +230,7 @@ Normal and Visual Ctrl+u/Ctrl+d invoke `config.core.scroll_step_up` / `config.co
| 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 | `diff.comment` |
| Normal | `diff.review.edit`, `diff.review.delete`, `diff.review.next`, `diff.review.previous`, `diff.review.overview`, `diff.review.summary`, `diff.review.verdict`, `diff.review.save`, `diff.review.open` | | Normal | `diff.review.edit`, `diff.review.delete`, `diff.review.next`, `diff.review.previous`, `diff.review.overview`, `diff.review.summary`, `diff.review.verdict`, `diff.review.save`, `diff.review.open`, `diff.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` |
@@ -324,7 +325,7 @@ config.bind "leader r o", config.core.diff.review.overview
config.bind "leader r n", config.core.diff.review.next config.bind "leader r n", config.core.diff.review.next
``` ```
Comment and summary composition uses native editing in a floating window over the diff workspace, without adding workspace splits. Escape in Normal mode or `:pane.close` dismisses it while retaining drafts. `:w` there writes the whole local JSON review, including hidden draft text; choose its destination with `:diff.review.save path`. These actions do not publish reviews or contact a forge. See [local review behavior](editing.md#repository-diff). Review editing uses a floating window without adding workspace splits. Dismissing or switching editors commits text to local SQLite; verdicts and deletions 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).
`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
@@ -61,6 +61,8 @@ Diff checks cover staged category/filter navigation, stale request cancellation,
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.
SQLite review checks cover exact anchors across restarts and revision changes, repository isolation, independent connections and optimistic conflicts, locked/unwritable stores, incompatible schema preservation, bounded/corrupt documents, private file permissions, and XDG/config path resolution. Editor checks commit text on dismissal and switching, 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 restarts three editor processes against one temporary XDG database and verifies stable IDs, Unicode anchors, automatic summary saves, JSON export isolation, and selector/verdict persistence.
Conflicted-revision regressions preserve unchanged symlinks, defer oversized unselected blobs until file selection, compare selected conflict text with jj's projection, and exclude unchanged conflicts when comparing later revisions. Live terminal smoke covers local bookmark selection and opening an actual conflicted file without changing the checkout. Conflicted-revision regressions preserve unchanged symlinks, defer oversized unselected blobs until file selection, compare selected conflict text with jj's projection, and exclude unchanged conflicts when comparing later revisions. Live terminal smoke covers local bookmark selection and opening an actual conflicted file without changing the checkout.
+9 -4
View File
@@ -466,12 +466,17 @@ Comments, summaries, and the review overview open in a focused floating editor o
| `:diff.review.summary` | Compose the review summary | | `:diff.review.summary` | Compose the review summary |
| `:diff.review.verdict looks_good` | Record **Looks good** | | `:diff.review.verdict looks_good` | Record **Looks good** |
| `:diff.review.verdict changes_requested` | Record **Changes requested**; `none` clears the verdict | | `:diff.review.verdict changes_requested` | Record **Changes requested**; `none` clears the verdict |
| `:diff.review.save [path]` | Save the entire review as local JSON; the first save requires a path | | `:diff.review.save [path]` | Commit to SQLite; with a path, also export JSON for sharing |
| `:diff.review.open path` | Reopen a saved review for the current repository | | `:diff.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 |
In a review text pane, **`:w` saves the entire review**, not a source file. Choose a destination with `:diff.review.save path` first, or use `:w path`. Saving includes edits in hidden comment/summary buffers. Normal close, quit, restart, comparison replacement, and source-buffer deletion protect unsaved reviews; forced close/quit retains its explicit discard semantics. An externally changed review file is never silently overwritten, including with `:w!`; use a new path or reopen it after preserving your edits. Reviews save automatically to a local SQLite database when you dismiss or switch comment/summary editors. Verdicts and deletions commit immediately; typing alone does not write per keystroke. **`:w`** explicitly commits the entire review, including hidden drafts. `:diff.review.save path` or `:w path` additionally exports JSON; later autosaves still go to SQLite and leave that export unchanged. Imports create a new stored review identity rather than overwriting another review.
Version 2 JSON is the canonical, agent-readable artifact: repository root, summary, optional verdict, and comments with stable IDs, bodies, and anchors. Each anchor retains its immutable comparison revision, side, relative path (old path for renamed Before files), and exact whole-file text including the final newline. `start` and `end` contain zero-based source `row` and extended-grapheme `column` coordinates; `end` is exclusive. Columns count complete Unicode graphemes, not bytes, code points, UTF-16 units, or screen cells. Overview/window labels display one-based rows and columns. Only the current format is supported; old review files are not migrated. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. The file limit remains 64 MiB. Prefer saving outside the checkout so the review itself does not become another changed file. Storage defaults to `$XDG_DATA_HOME/vamp/reviews/reviews.sqlite3`, or `~/.local/share/vamp/reviews/reviews.sqlite3` when XDG_DATA_HOME is unset or relative. Configure `review_directory` to override its directory; relative configuration paths resolve against the config file. Repository identity is the canonical checkout root. Each review has a persistent database ID independent of comparison revisions, so reopening changed revisions retains stale comments. Opening a comparison automatically restores the repository's sole review. Multiple reviews open a floating selector: move to an entry and press Enter, or use `:diff.review.open id`. Use `:diff.review.new` for another independent review.
Write errors, locked databases, incompatible schemas, and conflicting edits from another editor instance retain dirty drafts and report the failure. Normal destructive closes remain guarded until persistence succeeds. `:w!` does not bypass database revision conflicts; export your draft before reopening the current stored review. JSON exports retain external-file-change protection. Only the current database schema is accepted; existing incompatible databases are preserved rather than reset or migrated.
Version 2 JSON is the agent-readable sharing format: repository root, summary, optional verdict, and comments with stable IDs, bodies, and anchors. Database review IDs and optimistic storage revisions stay internal. Each anchor retains its immutable comparison revision, side, relative path (old path for renamed Before files), and exact whole-file text including the final newline. `start` and `end` contain zero-based source `row` and extended-grapheme `column` coordinates; `end` is exclusive. Columns count complete Unicode graphemes, not bytes, code points, UTF-16 units, or screen cells. Overview/window labels display one-based rows and columns. Only the current format is supported. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. Each stored or exported review is limited to 64 MiB.
Reopening requires the same repository root. Comments whose revision, path, or exact file text no longer matches are **stale**: their original anchors remain unchanged and their gutter markers disappear. Unloaded files are shown as **not checked** until visited; refresh the overview to update its statuses. There is no automatic reanchoring, server, authentication, or review-publishing operation. Reopening requires the same repository root. Comments whose revision, path, or exact file text no longer matches are **stale**: their original anchors remain unchanged and their gutter markers disappear. Unloaded files are shown as **not checked** until visited; refresh the overview to update its statuses. There is no automatic reanchoring, server, authentication, or review-publishing operation.
+253
View File
@@ -990,6 +990,12 @@ core_actions![
true, true,
true true
), ),
(
"core.diff.review.new",
Action::ReviewNew,
&[Mode::Normal],
true
),
( (
"core.word_wrap.toggle", "core.word_wrap.toggle",
Action::WordWrapToggle, Action::WordWrapToggle,
@@ -2072,6 +2078,7 @@ pub struct Editor {
render_pane_focused: bool, render_pane_focused: bool,
diff_owner: Option<usize>, diff_owner: Option<usize>,
parked_diff: Option<DiffState>, parked_diff: Option<DiffState>,
review_directory: Option<PathBuf>,
diff_pane_bounds: [(Focus, PaneRect); 3], diff_pane_bounds: [(Focus, PaneRect); 3],
// Closed destinations stay until bounded eviction so they cannot shift jump_index. // Closed destinations stay until bounded eviction so they cannot shift jump_index.
jumps: VecDeque<JumpLocation>, jumps: VecDeque<JumpLocation>,
@@ -2178,6 +2185,7 @@ impl Editor {
render_pane_focused: true, render_pane_focused: true,
diff_owner: None, diff_owner: None,
parked_diff: None, parked_diff: None,
review_directory: None,
diff_pane_bounds: [( diff_pane_bounds: [(
Focus::Files, Focus::Files,
PaneRect { PaneRect {
@@ -4502,6 +4510,9 @@ impl Editor {
} }
} }
self.finish_review_navigation(); self.finish_review_navigation();
if let Err(error) = self.initialize_local_review() {
self.set_message(format!("Local review: {error}"));
}
true true
} }
@@ -5250,6 +5261,7 @@ impl Editor {
| Action::ReviewVerdict | Action::ReviewVerdict
| Action::ReviewSave | Action::ReviewSave
| Action::ReviewOpen | Action::ReviewOpen
| Action::ReviewNew
| Action::WordWrapToggle | Action::WordWrapToggle
| Action::GitBlameToggle | Action::GitBlameToggle
| Action::Quit | Action::Quit
@@ -8448,6 +8460,7 @@ impl Editor {
| Action::ReviewVerdict | Action::ReviewVerdict
| Action::ReviewSave | Action::ReviewSave
| Action::ReviewOpen | Action::ReviewOpen
| Action::ReviewNew
| Action::WordWrapToggle | Action::WordWrapToggle
| Action::GitBlameToggle | Action::GitBlameToggle
| Action::LspDefinition | Action::LspDefinition
@@ -9361,6 +9374,7 @@ impl Editor {
| Action::ReviewVerdict | Action::ReviewVerdict
| Action::ReviewSave | Action::ReviewSave
| Action::ReviewOpen | Action::ReviewOpen
| Action::ReviewNew
) { ) {
self.finish_prompt(); self.finish_prompt();
if let Err(error) = self.local_review_command(entry.action, args) { if let Err(error) = self.local_review_command(entry.action, args) {
@@ -9368,6 +9382,21 @@ impl Editor {
} }
return None; return None;
} }
if self.review_directory.is_some()
&& matches!(
entry.action,
Action::TabPrevious
| Action::TabNext
| Action::TabNew
| Action::TabClose
| Action::Quit
| Action::Restart
)
&& let Err(error) = self.autosave_local_review()
{
self.set_message(format!("Review not saved: {error}"));
return None;
}
if self.review_panel().is_some() if self.review_panel().is_some()
&& matches!( && matches!(
entry.action, entry.action,
@@ -15911,11 +15940,235 @@ mod tests {
}], }],
})), })),
); );
if editor.review_panel().is_some() {
return;
}
editor.handle(Input::Char('\t')); editor.handle(Input::Char('\t'));
assert_eq!(editor.diff().unwrap().focus, Focus::Before); assert_eq!(editor.diff().unwrap().focus, Focus::Before);
assert!(editor.diff_interacting()); assert!(editor.diff_interacting());
} }
#[test]
fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
use crate::review::{Review, Store, Verdict};
let directory = Directory::new();
let root = directory.0.canonicalize().unwrap();
let storage = root.join("data");
let mut first = editor();
first.set_review_directory(storage.clone());
historical_diff(
&mut first,
&root,
vec!["p界e\u{301}z".into()],
vec!["changed".into()],
);
type_text(&mut first, "lvl:diff.comment");
first.handle(Input::Enter);
type_text(&mut first, "iFirst feedback");
first.handle(Input::Escape);
assert!(
Store::open(&storage)
.unwrap()
.list(&root)
.unwrap()
.is_empty()
);
first.handle(Input::Escape);
let id = first
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id
.unwrap();
let store = Store::open(&storage).unwrap();
let persisted = store.load(&root, id).unwrap();
assert_eq!(persisted.comments[0].body, "First feedback");
assert_eq!(
persisted.comments[0].anchor.start,
TextPosition { row: 0, column: 1 }
);
assert_eq!(
persisted.comments[0].anchor.end,
TextPosition { row: 0, column: 3 }
);
first.execute("diff.review.summary");
type_text(&mut first, "iSummary");
first.handle(Input::Escape);
first.execute("diff.review.verdict changes_requested");
let persisted = store.load(&root, id).unwrap();
assert_eq!(persisted.summary, "Summary");
assert_eq!(persisted.verdict, Some(Verdict::ChangesRequested));
let exported = root.join("shared.json");
first.execute(&format!("diff.review.save {}", exported.display()));
first.execute("diff.review.edit 1");
type_text(&mut first, "A revised");
first.handle(Input::Escape);
first.handle(Input::Escape);
assert_eq!(
store.load(&root, id).unwrap().comments[0].body,
"First feedback revised"
);
assert_eq!(
Review::load(&exported, &root).unwrap().comments[0].body,
"First feedback"
);
let mut reopened = editor();
reopened.set_review_directory(storage.clone());
historical_diff(
&mut reopened,
&root,
vec!["different revision".into()],
vec!["changed".into()],
);
let restored = &reopened.workspace_diff().unwrap().local_review.document;
assert_eq!(restored.storage_id, Some(id));
assert_eq!(restored.comments[0].body, "First feedback revised");
assert!(
!reopened
.workspace_diff()
.unwrap()
.review_anchor_current(&restored.comments[0].anchor)
);
reopened.execute("diff.review.new");
let second_id = reopened
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id
.unwrap();
assert_ne!(id, second_id);
let mut choosing = editor();
choosing.set_review_directory(storage);
historical_diff(&mut choosing, &root, vec!["old".into()], vec!["new".into()]);
assert_eq!(
choosing
.workspace_diff()
.unwrap()
.local_review
.choices
.iter()
.map(|(id, _)| *id)
.collect::<std::collections::BTreeSet<_>>(),
std::collections::BTreeSet::from([id, second_id])
);
choosing.handle(Input::Enter);
assert_eq!(
choosing
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id,
Some(second_id)
);
choosing.execute(&format!("diff.review.open {id}"));
assert_eq!(
choosing
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id,
Some(id)
);
choosing.execute("diff.review.delete 1");
assert!(store.load(&root, id).unwrap().comments.is_empty());
choosing.execute(&format!("diff.review.open {}", exported.display()));
let imported_id = choosing
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id
.unwrap();
assert_ne!(imported_id, id);
assert_eq!(
store.load(&root, imported_id).unwrap().comments[0].body,
"First feedback"
);
}
#[test]
fn sqlite_review_conflicts_and_write_failures_keep_dirty_drafts() {
use crate::review::Store;
let directory = Directory::new();
let root = directory.0.canonicalize().unwrap();
let storage = root.join("data");
let mut first = editor();
first.set_review_directory(storage.clone());
historical_diff(&mut first, &root, vec!["old".into()], vec!["new".into()]);
first.execute("diff.comment");
type_text(&mut first, "iStored feedback");
first.handle(Input::Escape);
first.handle(Input::Escape);
let id = first
.workspace_diff()
.unwrap()
.local_review
.document
.storage_id
.unwrap();
let mut stale = editor();
stale.set_review_directory(storage.clone());
historical_diff(&mut stale, &root, vec!["old".into()], vec!["new".into()]);
let version = stale
.workspace_diff()
.unwrap()
.local_review
.document
.storage_revision;
first.execute("diff.review.verdict looks_good");
stale.execute("diff.review.verdict changes_requested");
assert!(stale.current_review_dirty());
assert_eq!(
stale
.workspace_diff()
.unwrap()
.local_review
.document
.storage_revision,
version
);
assert_eq!(
Store::open(&storage)
.unwrap()
.load(&root, id)
.unwrap()
.verdict,
Some(crate::review::Verdict::LooksGood)
);
let blocked = root.join("not-a-directory");
fs::write(&blocked, "preserve").unwrap();
first.set_review_directory(blocked.clone());
first.execute("diff.review.edit 1");
type_text(&mut first, "A unsaved");
first.handle(Input::Escape);
first.handle(Input::Escape);
assert!(first.current_review_dirty());
assert_eq!(
first
.workspace_diff()
.unwrap()
.local_review
.document
.comments[0]
.body,
"Stored feedback unsaved"
);
assert_eq!(fs::read_to_string(blocked).unwrap(), "preserve");
assert_eq!(
Store::open(&storage)
.unwrap()
.load(&root, id)
.unwrap()
.comments[0]
.body,
"Stored feedback"
);
}
#[test] #[test]
fn local_review_native_composition_roundtrip_and_draft_lifecycle() { fn local_review_native_composition_roundtrip_and_draft_lifecycle() {
use crate::review::{Review, Side, Verdict}; use crate::review::{Review, Side, Verdict};
+205 -21
View File
@@ -6,6 +6,7 @@ enum ReviewText {
Comment(u64), Comment(u64),
Summary, Summary,
Overview, Overview,
Reviews,
} }
pub(super) struct ReviewBuffer { pub(super) struct ReviewBuffer {
@@ -15,6 +16,109 @@ pub(super) struct ReviewBuffer {
} }
impl Editor { impl Editor {
pub fn set_review_directory(&mut self, directory: PathBuf) {
self.review_directory = Some(directory);
}
pub(super) fn initialize_local_review(&mut self) -> Result<(), String> {
let Some(directory) = self.review_directory.as_ref() else {
return Ok(());
};
let Some(diff) = self
.workspace_diff()
.filter(|diff| diff.phase == Phase::View)
else {
return Ok(());
};
if diff.local_review.initialized {
return Ok(());
}
let root = diff
.comparison
.as_ref()
.ok_or("Comparison is loading")?
.root
.clone();
let directory = directory.clone();
self.review_diff_mut().unwrap().local_review.initialized = true;
self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone());
let store = crate::review::Store::open(&directory)?;
let choices = store.list(&root)?;
if choices.len() == 1 {
self.review_diff_mut().unwrap().local_review.document =
store.load(&root, choices[0].0)?;
} else if choices.len() > 1 {
self.review_diff_mut().unwrap().local_review.choices = choices;
self.open_review_selector()?;
}
Ok(())
}
fn open_review_selector(&mut self) -> Result<(), String> {
let diff = self.workspace_diff().ok_or("No comparison is open")?;
let directory = self
.review_directory
.as_deref()
.ok_or("Review storage is not configured")?;
let root = &diff
.comparison
.as_ref()
.ok_or("Comparison is loading")?
.root;
let choices = crate::review::Store::open(directory)?.list(root)?;
let mut text = String::from(
"# Local reviews\n\nEnter opens the review under the caret.\n:diff.review.new starts a separate review.\n",
);
for (id, label) in &choices {
use std::fmt::Write as _;
writeln!(text, "\n## Review #{id} · {label}").unwrap();
}
self.review_diff_mut().unwrap().local_review.choices = choices;
let row = text
.lines()
.position(|line| line.starts_with("## Review #"))
.unwrap_or(0);
self.open_review_buffer(
ReviewText::Reviews,
&text,
"Local reviews · Enter opens · :diff.review.new".into(),
)?;
self.place_navigation(TextPosition { row, column: 0 });
Ok(())
}
fn mark_review_saved(&mut self) {
self.review_diff_mut().unwrap().local_review.dirty = false;
for buffer in std::iter::once(&mut self.active).chain(self.parked.values_mut()) {
if buffer
.review_buffer
.as_ref()
.is_some_and(|binding| binding.owner == self.tab_id)
{
buffer.saved_lines.clone_from(&buffer.lines);
}
}
}
pub(super) fn autosave_local_review(&mut self) -> Result<(), String> {
self.sync_review_text();
let Some(directory) = self.review_directory.as_deref() else {
return Ok(());
};
if !self.current_review_dirty() {
return Ok(());
}
let mut store = crate::review::Store::open(directory)?;
store.save(
&mut self
.review_diff_mut()
.ok_or("No comparison is open")?
.local_review
.document,
)?;
self.mark_review_saved();
Ok(())
}
pub fn review_panel_title(&self) -> Option<&str> { pub fn review_panel_title(&self) -> Option<&str> {
self.active self.active
.review_buffer .review_buffer
@@ -35,6 +139,9 @@ impl Editor {
.origin .origin
.expect("review panel has a source pane"); .expect("review panel has a source pane");
self.finish_pane_interaction(); self.finish_pane_interaction();
if let Err(error) = self.autosave_local_review() {
self.set_message(format!("Review not saved: {error}"));
}
self.select_pane(source); self.select_pane(source);
self.bind_diff_buffer(); self.bind_diff_buffer();
true true
@@ -71,6 +178,12 @@ impl Editor {
} }
pub(super) fn local_review_close_blocked(&mut self) -> bool { pub(super) fn local_review_close_blocked(&mut self) -> bool {
if self.review_directory.is_some()
&& let Err(error) = self.autosave_local_review()
{
self.set_message(format!("Review not saved: {error}"));
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 :diff.review.save <path> before closing");
true true
@@ -97,8 +210,9 @@ impl Editor {
.chain(self.parked.values()) .chain(self.parked.values())
.filter_map(|buffer| { .filter_map(|buffer| {
let binding = buffer.review_buffer.as_ref()?; let binding = buffer.review_buffer.as_ref()?;
(binding.owner == owner && binding.kind != ReviewText::Overview) (binding.owner == owner
.then(|| (binding.kind, buffer.lines.join("\n"))) && matches!(binding.kind, ReviewText::Comment(_) | ReviewText::Summary))
.then(|| (binding.kind, buffer.lines.join("\n")))
}) })
.collect(); .collect();
let Some(diff) = self.review_diff_mut() else { let Some(diff) = self.review_diff_mut() else {
@@ -115,6 +229,7 @@ impl Editor {
.map(|comment| &mut comment.body), .map(|comment| &mut comment.body),
ReviewText::Summary => Some(&mut diff.local_review.document.summary), ReviewText::Summary => Some(&mut diff.local_review.document.summary),
ReviewText::Overview => None, ReviewText::Overview => None,
ReviewText::Reviews => None,
}; };
if let Some(destination) = destination if let Some(destination) = destination
&& *destination != text && *destination != text
@@ -127,6 +242,23 @@ impl Editor {
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> {
self.sync_review_text(); self.sync_review_text();
if self.review_directory.is_some() {
if let Some(path) = path {
self.review_diff_mut()
.ok_or("No comparison is open")?
.local_review
.document
.save(path)?;
self.review_diff_mut().unwrap().local_review.path = Some(path.to_path_buf());
}
self.autosave_local_review()?;
self.set_message(if path.is_some() {
"Review exported as JSON; autosave remains in SQLite"
} else {
"Local review saved to SQLite"
});
return Ok(());
}
let state = &mut self let state = &mut self
.review_diff_mut() .review_diff_mut()
.ok_or("No comparison is open")? .ok_or("No comparison is open")?
@@ -159,6 +291,7 @@ impl Editor {
if self.resolver().is_some() { if self.resolver().is_some() {
return Err("Local reviews require an ordinary diff comparison".into()); return Err("Local reviews require an ordinary diff comparison".into());
} }
self.initialize_local_review()?;
let diff = self let diff = self
.workspace_diff() .workspace_diff()
.filter(|diff| diff.phase == Phase::View) .filter(|diff| diff.phase == Phase::View)
@@ -191,26 +324,49 @@ impl Editor {
{ {
self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone()); self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone());
} }
if !matches!(action, Action::ReviewSave) {
self.autosave_local_review()?;
}
if action == Action::ReviewOpen { if action == Action::ReviewOpen {
if args.is_empty() {
return Err("Usage: diff.review.open <path>".into());
}
if self.current_review_dirty() { if self.current_review_dirty() {
return Err("Save the current local review before opening another".into()); return Err("Save the current local review before opening another".into());
} }
let document = Review::load(Path::new(args), &root)?; if args.is_empty() {
return self.open_review_selector();
}
let document = if let (Some(directory), Ok(id)) =
(self.review_directory.as_deref(), args.parse::<i64>())
{
crate::review::Store::open(directory)?.load(&root, id)?
} else {
Review::load(Path::new(args), &root)?
};
let imported = document.storage_id.is_none();
let dirty = imported && self.review_directory.is_some();
self.detach_review_buffers(); self.detach_review_buffers();
let state = &mut self.review_diff_mut().unwrap().local_review; let state = &mut self.review_diff_mut().unwrap().local_review;
state.document = document; state.document = document;
state.path = Some(PathBuf::from(args)); state.path = imported.then(|| PathBuf::from(args));
state.selected = None; state.selected = None;
state.pending_navigation = None; state.pending_navigation = None;
self.open_review_overview()?; state.dirty = dirty;
return Ok(()); self.autosave_local_review()?;
return self.open_review_overview();
} }
self.sync_review_text(); self.sync_review_text();
match action { match action {
Action::DiffComment => self.add_review_comment()?, Action::DiffComment => self.add_review_comment()?,
Action::ReviewNew => {
self.detach_review_buffers();
let state = &mut self.review_diff_mut().unwrap().local_review;
state.document = Review::new(root);
state.selected = None;
state.pending_navigation = None;
state.path = None;
state.dirty = true;
self.autosave_local_review()?;
self.open_review_overview()?;
}
Action::ReviewEdit => { Action::ReviewEdit => {
let id = self.review_comment_id(args)?; let id = self.review_comment_id(args)?;
self.show_review_comment(id)?; self.show_review_comment(id)?;
@@ -231,6 +387,7 @@ impl Editor {
buffer.saved_lines.clone_from(&buffer.lines); buffer.saved_lines.clone_from(&buffer.lines);
} }
} }
self.autosave_local_review()?;
self.open_review_overview()?; self.open_review_overview()?;
} }
Action::ReviewNext | Action::ReviewPrevious => { Action::ReviewNext | Action::ReviewPrevious => {
@@ -281,6 +438,7 @@ impl Editor {
state.document.verdict = verdict; state.document.verdict = verdict;
state.dirty = true; state.dirty = true;
} }
self.autosave_local_review()?;
self.open_review_overview()?; self.open_review_overview()?;
} }
Action::ReviewSave => { Action::ReviewSave => {
@@ -517,12 +675,24 @@ impl Editor {
Some(Verdict::ChangesRequested) => "Changes requested", Some(Verdict::ChangesRequested) => "Changes requested",
None => "Not set", None => "Not set",
}; };
let mut text = format!( let storage = if self.review_directory.is_some() {
"# Local review\n\nVerdict: {verdict}\nFile: {}\n\n## Summary\n{}\n\nEnter edits the comment under the caret.\n:diff.review.summary · :diff.review.verdict · :diff.review.save\n", state.document.storage_id.map_or_else(
|| "SQLite · uncommitted draft".into(),
|id| {
format!(
"SQLite · review #{id} · revision {}",
state.document.storage_revision
)
},
)
} else {
state state
.path .path
.as_ref() .as_ref()
.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!(
"# 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",
state.document.summary state.document.summary
); );
for comment in &state.document.comments { for comment in &state.document.comments {
@@ -604,7 +774,7 @@ impl Editor {
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);
if kind == ReviewText::Overview { if matches!(kind, ReviewText::Overview | ReviewText::Reviews) {
self.active.lines = split_lines(text); self.active.lines = split_lines(text);
self.active.saved_lines.clone_from(&self.active.lines); self.active.saved_lines.clone_from(&self.active.lines);
self.active.text_revision = self.active.text_revision.wrapping_add(1); self.active.text_revision = self.active.text_revision.wrapping_add(1);
@@ -618,10 +788,14 @@ impl Editor {
kind, kind,
title, title,
}); });
buffer.resolver_readonly = kind == ReviewText::Overview; buffer.resolver_readonly = matches!(kind, ReviewText::Overview | ReviewText::Reviews);
self.add_buffer(buffer); self.add_buffer(buffer);
} }
self.set_message("Local review · :w saves JSON; first choose :diff.review.save <path>"); self.set_message(if self.review_directory.is_some() {
"Local review · dismiss to commit · :w saves · :diff.review.save path exports JSON"
} else {
"Local review · :w saves JSON; first choose :diff.review.save <path>"
});
Ok(()) Ok(())
} }
@@ -638,13 +812,23 @@ impl Editor {
if *input == Input::Enter if *input == Input::Enter
&& self.active.view.mode == Mode::Normal && self.active.view.mode == Mode::Normal
&& self.palette.is_none() && self.palette.is_none()
&& self && self.active.review_buffer.as_ref().is_some_and(|binding| {
.active matches!(binding.kind, ReviewText::Overview | ReviewText::Reviews)
.review_buffer })
.as_ref()
.is_some_and(|binding| binding.kind == ReviewText::Overview)
{ {
if let Err(error) = self.local_review_command(Action::ReviewEdit, "") { let choice = if self.active.review_buffer.as_ref().unwrap().kind == ReviewText::Reviews
{
self.active.lines[..=self.cursor().row]
.iter()
.rev()
.find_map(|line| line.strip_prefix("## Review #")?.split_whitespace().next())
.map(str::to_owned)
.ok_or_else(|| "Place the caret on a review".to_string())
.and_then(|id| self.local_review_command(Action::ReviewOpen, &id))
} else {
self.local_review_command(Action::ReviewEdit, "")
};
if let Err(error) = choice {
self.set_message(error); self.set_message(error);
} }
true true
+3 -2
View File
@@ -87,8 +87,9 @@ Local reviews: :diff.comment anchors exact Visual text, Visual Line rows, or the
Selected comments highlight their precise text Selected comments highlight their precise text
:diff.review.edit/delete [ID], :diff.review.next/previous, :diff.review.overview :diff.review.edit/delete [ID], :diff.review.next/previous, :diff.review.overview
:diff.review.summary; :diff.review.verdict looks_good|changes_requested|none :diff.review.summary; :diff.review.verdict looks_good|changes_requested|none
:diff.review.save PATH / :diff.review.open PATH; :w in floating review editors saves local JSON Local SQLite autosave on dismissal/switch; verdicts/deletions commit immediately; :w commits explicitly
Normal Esc or :pane.close dismisses the floating editor and retains drafts :diff.review.save PATH exports JSON; :diff.review.open [ID|PATH] selects/imports; :diff.review.new
Normal Esc or :pane.close commits and dismisses the floating editor; failures retain drafts
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/All tabs, type to search Pull requests: :pull_requests [REMOTE]; GitHub/GitLab/Gitea Open/Closed/All tabs, type to search
Enter opens the immutable PR head against its merge base; API auth uses provider tokens or gh/glab Enter opens the immutable PR head against its merge base; API auth uses provider tokens or gh/glab
+2
View File
@@ -114,6 +114,8 @@ impl Default for Review {
#[derive(Clone, Debug, Default)] #[derive(Clone, Debug, Default)]
pub struct ReviewState { pub struct ReviewState {
pub document: Review, pub document: Review,
pub initialized: bool,
pub choices: Vec<(i64, String)>,
pub path: Option<PathBuf>, pub path: Option<PathBuf>,
pub selected: Option<u64>, pub selected: Option<u64>,
pub pending_navigation: Option<u64>, pub pending_navigation: Option<u64>,
+111 -4
View File
@@ -33,6 +33,7 @@ const INVOKE_TIMEOUT: Duration = Duration::from_millis(500);
pub struct Config { pub struct Config {
pub plugins: Vec<PluginConfig>, pub plugins: Vec<PluginConfig>,
pub lsp: Vec<crate::lsp::ServerConfig>, pub lsp: Vec<crate::lsp::ServerConfig>,
pub review_directory: Option<PathBuf>,
pub bindings: Vec<Binding>, pub bindings: Vec<Binding>,
pub leader: String, pub leader: String,
pub file_explorer_side: String, pub file_explorer_side: String,
@@ -85,6 +86,7 @@ impl Default for Config {
Self { Self {
plugins: Vec::new(), plugins: Vec::new(),
lsp: Vec::new(), lsp: Vec::new(),
review_directory: None,
bindings: Vec::new(), bindings: Vec::new(),
leader: "space".into(), leader: "space".into(),
file_explorer_side: "right".into(), file_explorer_side: "right".into(),
@@ -113,7 +115,7 @@ impl Default for Config {
} }
} }
/// Relative plugin paths are resolved against the configuration file's directory. /// Relative plugin and review paths are resolved against the configuration file's directory.
pub fn load_config(path: &Path) -> Result<Config, String> { pub fn load_config(path: &Path) -> Result<Config, String> {
let deadline = Instant::now() + STARTUP_TIMEOUT; let deadline = Instant::now() + STARTUP_TIMEOUT;
let path = absolute_file(path)?; let path = absolute_file(path)?;
@@ -124,6 +126,7 @@ pub fn load_config(path: &Path) -> Result<Config, String> {
let ConfigFrame { let ConfigFrame {
plugins, plugins,
lsp, lsp,
review_directory,
bindings, bindings,
leader, leader,
file_explorer_side, file_explorer_side,
@@ -152,6 +155,7 @@ pub fn load_config(path: &Path) -> Result<Config, String> {
} = *response; } = *response;
let mut config = Config::default(); let mut config = Config::default();
let directory = path.parent().expect("absolute file has a parent"); let directory = path.parent().expect("absolute file has a parent");
config.review_directory = review_directory.map(|path| directory.join(path));
for plugin in plugins { for plugin in plugins {
let plugin_path = plugin.path.to_string_lossy(); let plugin_path = plugin.path.to_string_lossy();
if plugin_path.is_empty() || plugin_path.contains('\0') { if plugin_path.is_empty() || plugin_path.contains('\0') {
@@ -490,6 +494,21 @@ fn scroll_distance<'de, D: serde::Deserializer<'de>>(
}) })
} }
fn review_directory<'de, D: serde::Deserializer<'de>>(
deserializer: D,
) -> Result<Option<PathBuf>, D::Error> {
let directory = Option::<String>::deserialize(deserializer)?;
if directory
.as_ref()
.is_some_and(|path| path.is_empty() || path.contains('\0'))
{
return Err(serde::de::Error::custom(
"review_directory must not be empty or contain NUL",
));
}
Ok(directory.map(PathBuf::from))
}
// Startup-only configuration does not inflate every queued plugin response. // Startup-only configuration does not inflate every queued plugin response.
#[derive(Deserialize)] #[derive(Deserialize)]
#[serde(deny_unknown_fields)] #[serde(deny_unknown_fields)]
@@ -499,6 +518,8 @@ struct ConfigFrame {
// Keep this wire field required; resolve its configuration after decoding. // Keep this wire field required; resolve its configuration after decoding.
#[serde(deserialize_with = "serde_json::Value::deserialize")] #[serde(deserialize_with = "serde_json::Value::deserialize")]
lsp: serde_json::Value, lsp: serde_json::Value,
#[serde(deserialize_with = "review_directory")]
review_directory: Option<PathBuf>,
bindings: Vec<Binding>, bindings: Vec<Binding>,
leader: String, leader: String,
file_explorer_side: String, file_explorer_side: String,
@@ -1003,6 +1024,87 @@ mod tests {
} }
} }
#[test]
fn review_directory_resolves_config_paths_and_keeps_session_startup_lazy() {
let fixture = Fixture::new();
let absolute = fixture.0.join("absolute-reviews");
for (value, expected) in [
(
r#""relative-reviews""#.to_owned(),
Some(fixture.0.join("relative-reviews")),
),
(serde_json::to_string(&absolute).unwrap(), Some(absolute)),
("nil".to_owned(), None),
] {
let path = fixture.file(
"config.rb",
&format!("Vamp.configure {{ |config| config.review_directory = {value} }}"),
);
assert_eq!(load_config(&path).unwrap().review_directory, expected);
let session = EditorSession::open(None, vec![], Some(&path)).unwrap();
assert!(session.editor().startup_diagnostic().is_none());
if let Some(directory) = expected {
assert!(
!directory.exists(),
"startup created {}",
directory.display()
);
}
}
let path = fixture.file("config.rb", "Vamp.configure { |_| }");
assert_eq!(load_config(&path).unwrap().review_directory, None);
for value in [
"7",
"false",
"[]",
"{}",
":reviews",
r#""""#,
r#""bad\0path""#,
] {
let path = fixture.file(
"config.rb",
&format!("Vamp.configure {{ |config| config.review_directory = {value} }}"),
);
let error = load_config(&path).unwrap_err();
assert!(error.contains("review_directory"), "{value}: {error}");
}
}
#[test]
fn review_directory_wire_field_is_required_nullable_and_validated() {
let mut frame = serde_json::json!({
"version": 1, "type": "config", "plugins": [], "lsp": {}, "bindings": [],
"review_directory": null, "leader": "space",
"file_explorer_side": "right", "file_picker_view": "tree", "file_explorer_view": "tree",
"clipboard_mode": "system", "git_blame": true, "git_gutter": "bar",
"top_bar": {}, "bottom_bar": {}, "popup": {"command": {}, "search": {}}, "labels": {},
"scroll_distance": 0, "scroll_step_lines": null, "highlight_trailing_whitespace": false,
"whitespace_characters": {}, "whitespace_scope": "all",
"tab_spaces": null, "language_tab_spaces": {},
"diff_context_lines": 10, "diff_expand_lines": 15,
"diff_view_mode": "split", "diff_split_orientation": "vertical",
"diff_follow_working_copy": true
});
for value in [serde_json::Value::Null, serde_json::json!("reviews")] {
frame["review_directory"] = value;
assert!(serde_json::from_value::<Response>(frame.clone()).is_ok());
}
for value in [
serde_json::json!(""),
serde_json::json!("bad\0path"),
serde_json::json!(7),
serde_json::json!(false),
serde_json::json!([]),
serde_json::json!({}),
] {
frame["review_directory"] = value;
assert!(serde_json::from_value::<Response>(frame.clone()).is_err());
}
frame.as_object_mut().unwrap().remove("review_directory");
assert!(serde_json::from_value::<Response>(frame).is_err());
}
#[test] #[test]
fn tab_spaces_follow_active_language_and_preserve_native_edit_semantics() { fn tab_spaces_follow_active_language_and_preserve_native_edit_semantics() {
let fixture = Fixture::new(); let fixture = Fixture::new();
@@ -1410,9 +1512,9 @@ end
assert!(load_config(&path).unwrap_err().contains(error), "{value}"); assert!(load_config(&path).unwrap_err().contains(error), "{value}");
} }
for frame in [ for frame in [
r#"{"type":"config","version":1,"plugins":[],"lsp":{},"bindings":[],"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true}"#, r#"{"type":"config","version":1,"plugins":[],"lsp":{},"review_directory":null,"bindings":[],"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true}"#,
r#"{"type":"config","version":1,"plugins":[],"lsp":{},"bindings":[],"leader":null,"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true}"#, r#"{"type":"config","version":1,"plugins":[],"lsp":{},"review_directory":null,"bindings":[],"leader":null,"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true}"#,
r#"{"type":"config","version":1,"plugins":[],"lsp":{},"bindings":[],"leader":7,"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true}"#, r#"{"type":"config","version":1,"plugins":[],"lsp":{},"review_directory":null,"bindings":[],"leader":7,"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true}"#,
] { ] {
assert!(serde_json::from_str::<Response>(frame).is_err(), "{frame}"); assert!(serde_json::from_str::<Response>(frame).is_err(), "{frame}");
} }
@@ -1485,6 +1587,7 @@ end
} }
let mut frame = serde_json::json!({ let mut frame = serde_json::json!({
"type": "config", "version": 1, "plugins": [], "lsp": {}, "bindings": [], "type": "config", "version": 1, "plugins": [], "lsp": {}, "bindings": [],
"review_directory": null,
"leader": "space", "file_explorer_side": "right", "file_picker_view": "tree", "leader": "space", "file_explorer_side": "right", "file_picker_view": "tree",
"file_explorer_view": "tree", "clipboard_mode": "system", "git_blame": true, "file_explorer_view": "tree", "clipboard_mode": "system", "git_blame": true,
"git_gutter": "bar", "git_gutter": "bar",
@@ -1627,6 +1730,7 @@ end"#,
} }
let mut frame = serde_json::json!({ let mut frame = serde_json::json!({
"type": "config", "version": 1, "plugins": [], "lsp": {}, "bindings": [], "type": "config", "version": 1, "plugins": [], "lsp": {}, "bindings": [],
"review_directory": null,
"leader": "space", "file_explorer_side": "right", "file_picker_view": "tree", "leader": "space", "file_explorer_side": "right", "file_picker_view": "tree",
"file_explorer_view": "tree", "clipboard_mode": "system", "git_blame": true, "file_explorer_view": "tree", "clipboard_mode": "system", "git_blame": true,
"git_gutter": "bar", "scroll_distance": 0, "highlight_trailing_whitespace": false, "git_gutter": "bar", "scroll_distance": 0, "highlight_trailing_whitespace": false,
@@ -1886,6 +1990,7 @@ end
} }
let frame = serde_json::json!({ let frame = serde_json::json!({
"version": 1, "type": "config", "plugins": [], "lsp": {}, "version": 1, "type": "config", "plugins": [], "lsp": {},
"review_directory": null,
"bindings": [], "leader": "space", "file_explorer_side": "right", "bindings": [], "leader": "space", "file_explorer_side": "right",
"file_picker_view": "tree", "file_explorer_view": "tree", "file_picker_view": "tree", "file_explorer_view": "tree",
"top_bar": {}, "bottom_bar": {}, "labels": {}, "top_bar": {}, "bottom_bar": {}, "labels": {},
@@ -1997,6 +2102,7 @@ end"
let fixture = Fixture::new(); let fixture = Fixture::new();
let frame = serde_json::json!({ let frame = serde_json::json!({
"version": 1, "type": "config", "plugins": [], "lsp": {}, "version": 1, "type": "config", "plugins": [], "lsp": {},
"review_directory": null,
"bindings": [], "leader": "space", "file_explorer_side": "right", "bindings": [], "leader": "space", "file_explorer_side": "right",
"file_picker_view": "tree", "file_explorer_view": "tree", "file_picker_view": "tree", "file_explorer_view": "tree",
"clipboard_mode": "system", "git_blame": true, "git_gutter": "bar", "clipboard_mode": "system", "git_blame": true, "git_gutter": "bar",
@@ -2427,6 +2533,7 @@ end"#,
fn lsp_wire_configuration_is_resolved_after_decoding() { fn lsp_wire_configuration_is_resolved_after_decoding() {
let mut frame = serde_json::json!({ let mut frame = serde_json::json!({
"version": 1, "type": "config", "plugins": [], "lsp": {}, "version": 1, "type": "config", "plugins": [], "lsp": {},
"review_directory": null,
"bindings": [], "leader": "space", "file_explorer_side": "right", "bindings": [], "leader": "space", "file_explorer_side": "right",
"file_picker_view": "tree", "file_explorer_view": "tree", "file_picker_view": "tree", "file_explorer_view": "tree",
"git_blame": true, "git_blame": true,
+11
View File
@@ -133,6 +133,7 @@ module Vamp
def initialize def initialize
@plugins = [] @plugins = []
@lsp = {} @lsp = {}
@review_directory = nil
@bindings = [] @bindings = []
@leader = "space" @leader = "space"
@file_explorer_side = "right" @file_explorer_side = "right"
@@ -170,6 +171,15 @@ module Vamp
nil nil
end end
# @param path [String, nil] relative to the configuration file; nil uses the XDG data directory
# @return [String, nil]
def review_directory=(path)
path = nonempty_string!(path, "review_directory") unless path.nil?
raise ArgumentError, "review_directory must not contain NUL" if path&.include?("\0")
@review_directory = path
end
# @param key [String] # @param key [String]
# @return [String] # @return [String]
def leader=(key) def leader=(key)
@@ -345,6 +355,7 @@ module Vamp
# @return [Hash{String => Object}] validated startup configuration frame # @return [Hash{String => Object}] validated startup configuration frame
def response def response
{ "version" => 1, "type" => "config", "plugins" => @plugins, "lsp" => lsp_options, "bindings" => @bindings, { "version" => 1, "type" => "config", "plugins" => @plugins, "lsp" => lsp_options, "bindings" => @bindings,
"review_directory" => @review_directory,
"leader" => @leader, "file_explorer_side" => @file_explorer_side, "file_picker_view" => @file_picker_view, "leader" => @leader, "file_explorer_side" => @file_explorer_side, "file_picker_view" => @file_picker_view,
"file_explorer_view" => @file_explorer_view, "clipboard_mode" => @clipboard_mode, "file_explorer_view" => @file_explorer_view, "clipboard_mode" => @clipboard_mode,
"git_blame" => @git_blame, "git_blame" => @git_blame,
+5
View File
@@ -482,6 +482,10 @@ impl EditorSession {
}), }),
None => Config::default(), None => Config::default(),
}; };
editor.set_review_directory(match config.review_directory {
Some(directory) => directory,
None => crate::review::default_directory().map_err(io::Error::other)?,
});
if let Err(error) = editor.set_leader(&config.leader) { if let Err(error) = editor.set_leader(&config.leader) {
notices.push(format!("leader {:?}: {error}", config.leader)); notices.push(format!("leader {:?}: {error}", config.leader));
} }
@@ -2664,6 +2668,7 @@ mod tests {
Some(&config), Some(&config),
) )
.unwrap(); .unwrap();
session.editor.set_review_directory(self.0.join("reviews"));
// Legacy editing scenarios exercise the internal register, never the user's desktop. // Legacy editing scenarios exercise the internal register, never the user's desktop.
session.editor.set_clipboard_mode("separate").unwrap(); session.editor.set_clipboard_mode("separate").unwrap();
session session
+19 -6
View File
@@ -109,7 +109,7 @@ fn typed_diff_edits_unsaved_working_after_and_preserves_undo_on_explicit_close()
let config = fixture.0.join("config.rb"); let config = fixture.0.join("config.rb");
fs::write( fs::write(
&config, &config,
"Vamp.configure do |config|\n config.clipboard_mode = 'separate'\n config.bind 'leader d', config.core.diff\nend\n", "Vamp.configure do |config|\n config.review_directory = 'reviews'\n config.clipboard_mode = 'separate'\n config.bind 'leader d', config.core.diff\nend\n",
) )
.unwrap(); .unwrap();
let mut session = let mut session =
@@ -228,7 +228,7 @@ fn tabs_keep_repository_workers_and_hidden_comparisons_independent() {
let config = first.0.join("config.rb"); let config = first.0.join("config.rb");
fs::write( fs::write(
&config, &config,
"Vamp.configure do |config|\n config.bind 'ctrl+option+n', config.core.tab.new\n config.bind 'ctrl+option+w', config.core.tab.close\nend\n", "Vamp.configure do |config|\n config.review_directory = 'reviews'\n config.bind 'ctrl+option+n', config.core.tab.new\n config.bind 'ctrl+option+w', config.core.tab.close\nend\n",
) )
.unwrap(); .unwrap();
let mut session = let mut session =
@@ -330,7 +330,7 @@ fn editable_diff_navigation_skips_omitted_lines_without_expanding_excerpts() {
let text = format!("{}\n", lines.join("\n")); let text = format!("{}\n", lines.join("\n"));
fs::write(&source, &text).unwrap(); fs::write(&source, &text).unwrap();
let config = fixture.0.join("config.rb"); let config = fixture.0.join("config.rb");
fs::write(&config, "Vamp.configure do |config|\n config.clipboard_mode = 'separate'\n config.diff_context_lines = 1\n config.diff_expand_lines = 2\n config.bind 'ctrl+f', config.core.down\n config.bind 'ctrl+b', config.core.up\nend\n").unwrap(); fs::write(&config, "Vamp.configure do |config|\n config.review_directory = 'reviews'\n config.clipboard_mode = 'separate'\n config.diff_context_lines = 1\n config.diff_expand_lines = 2\n config.bind 'ctrl+f', config.core.down\n config.bind 'ctrl+b', config.core.up\nend\n").unwrap();
let mut session = let mut session =
EditorSession::open(Some(source.clone()), vec!["text".into()], Some(&config)).unwrap(); EditorSession::open(Some(source.clone()), vec!["text".into()], Some(&config)).unwrap();
session.execute("diff"); session.execute("diff");
@@ -467,7 +467,14 @@ fn readonly_jump_history_restores_the_comparison_file_and_source_cursor() {
let text = fs::read_to_string(&path).unwrap(); let text = fs::read_to_string(&path).unwrap();
fs::write(path, text.replace("_before", "_after")).unwrap(); fs::write(path, text.replace("_before", "_after")).unwrap();
} }
let mut session = EditorSession::open(Some(root.join("first.rs")), vec![], None).unwrap(); let config = fixture.0.join("review-config.rb");
fs::write(
&config,
"Vamp.configure { |config| config.review_directory = 'reviews' }",
)
.unwrap();
let mut session =
EditorSession::open(Some(root.join("first.rs")), vec![], Some(&config)).unwrap();
session.execute("diff"); session.execute("diff");
wait_until(&mut session, |session| { wait_until(&mut session, |session| {
!session.editor().diff().unwrap().loading !session.editor().diff().unwrap().loading
@@ -530,7 +537,7 @@ fn branch_changes_default_and_mode_switches_compare_the_selected_divergent_tips(
fixture.git(&["commit", "-qm", "feature only"]); fixture.git(&["commit", "-qm", "feature only"]);
let after = fixture.git(&["rev-parse", "HEAD"]); let after = fixture.git(&["rev-parse", "HEAD"]);
let config = fixture.0.join("config.rb"); let config = fixture.0.join("config.rb");
fs::write(&config, "Vamp.configure do |config|\n config.clipboard_mode = 'separate'\n config.bind 'ctrl+b', config.core.diff.mode.toggle\nend\n").unwrap(); fs::write(&config, "Vamp.configure do |config|\n config.review_directory = 'reviews'\n config.clipboard_mode = 'separate'\n config.bind 'ctrl+b', config.core.diff.mode.toggle\nend\n").unwrap();
let mut session = let mut session =
EditorSession::open(Some(root.join("common")), vec![], Some(&config)).unwrap(); EditorSession::open(Some(root.join("common")), vec![], Some(&config)).unwrap();
session.execute("diff"); session.execute("diff");
@@ -643,7 +650,13 @@ fn jj_bookmark_picker_keeps_symbolic_targets_searchable_by_resolved_commit_id()
fixture.jj(&["bookmark", "create", "topic"]); fixture.jj(&["bookmark", "create", "topic"]);
let after = fixture.jj(&["log", "--no-graph", "-r", "@", "-T", "commit_id"]); let after = fixture.jj(&["log", "--no-graph", "-r", "@", "-T", "commit_id"]);
fixture.jj(&["new"]); fixture.jj(&["new"]);
let mut session = EditorSession::open(Some(source), vec![], None).unwrap(); let config = fixture.0.join("review-config.rb");
fs::write(
&config,
"Vamp.configure { |config| config.review_directory = 'reviews' }",
)
.unwrap();
let mut session = EditorSession::open(Some(source), vec![], Some(&config)).unwrap();
session.execute("diff"); session.execute("diff");
wait_until(&mut session, |session| { wait_until(&mut session, |session| {
!session.editor().diff().unwrap().loading !session.editor().diff().unwrap().loading
+2 -2
View File
@@ -374,7 +374,7 @@ impl Fixture {
"settings": {"rust-analyzer": {"cargo": {"allFeatures": true}}}, "settings": {"rust-analyzer": {"cargo": {"allFeatures": true}}},
}); });
let config = format!( let config = format!(
"Vamp.configure do |config|\n config.clipboard_mode = 'separate'\n config.diff_context_lines = 1\n config.diff_expand_lines = 2\n config.lsp['fixture-language'] = JSON.parse({})\n config.bind 'leader d', config.core.lsp.definition\n config.bind 'leader D', config.core.lsp.diagnostics\n config.bind 'K', config.core.lsp.hover\nend\n", "Vamp.configure do |config|\n config.review_directory = 'reviews'\n config.clipboard_mode = 'separate'\n config.diff_context_lines = 1\n config.diff_expand_lines = 2\n config.lsp['fixture-language'] = JSON.parse({})\n config.bind 'leader d', config.core.lsp.definition\n config.bind 'leader D', config.core.lsp.diagnostics\n config.bind 'K', config.core.lsp.hover\nend\n",
serde_json::to_string(&options.to_string()).unwrap(), serde_json::to_string(&options.to_string()).unwrap(),
); );
let config_path = self.root.join("config.rb"); let config_path = self.root.join("config.rb");
@@ -457,7 +457,7 @@ impl Fixture {
json!({"extensions": ["rs"], "command": command(2, "controlled")}); json!({"extensions": ["rs"], "command": command(2, "controlled")});
} }
let config = format!( let config = format!(
"Vamp.configure do |config|\n config.clipboard_mode = 'separate'\n JSON.parse({}).each {{ |language, options| config.lsp[language] = options }}\n config.bind 'K', config.core.lsp.hover\nend\n", "Vamp.configure do |config|\n config.review_directory = 'reviews'\n config.clipboard_mode = 'separate'\n JSON.parse({}).each {{ |language, options| config.lsp[language] = options }}\n config.bind 'K', config.core.lsp.hover\nend\n",
serde_json::to_string(&languages.to_string()).unwrap(), serde_json::to_string(&languages.to_string()).unwrap(),
); );
let config_path = self.root.join("config.rb"); let config_path = self.root.join("config.rb");
+2 -2
View File
@@ -46,7 +46,7 @@ impl Fixture {
let config = self.file( let config = self.file(
"config.rb", "config.rb",
&format!( &format!(
"Vamp.configure do |config|\n config.clipboard_mode = 'separate'\n{bindings}\nend\n" "Vamp.configure do |config|\n config.review_directory = 'reviews'\n config.clipboard_mode = 'separate'\n{bindings}\nend\n"
), ),
); );
let session = let session =
@@ -488,7 +488,7 @@ fn readonly_before_and_historical_after_seeks_preserve_snapshots_and_folded_exce
fs::write(&source, &working_text).unwrap(); fs::write(&source, &working_text).unwrap();
let config = fixture.file( let config = fixture.file(
"config.rb", "config.rb",
"Vamp.configure do |config|\n config.clipboard_mode = 'separate'\n config.diff_context_lines = 1\n config.diff_expand_lines = 2\nend\n", "Vamp.configure do |config|\n config.review_directory = 'reviews'\n config.clipboard_mode = 'separate'\n config.diff_context_lines = 1\n config.diff_expand_lines = 2\nend\n",
); );
let mut session = let mut session =
EditorSession::open(Some(source.clone()), vec!["text".into()], Some(&config)).unwrap(); EditorSession::open(Some(source.clone()), vec!["text".into()], Some(&config)).unwrap();