fix: save review comments only on explicit write

This commit is contained in:
2026-10-17 01:36:45 +02:00
parent e9a757adc5
commit 2cb3736b92
7 changed files with 246 additions and 40 deletions
+2 -1
View File
@@ -2,8 +2,9 @@
- honeylang treesitter project located at `/Users/valdemar/Developer/Personal/Languages/tree-sitter-honey`. - honeylang treesitter project located at `/Users/valdemar/Developer/Personal/Languages/tree-sitter-honey`.
- [x] bug: `x` cut action does not copy the hovered character if not in selection mode, though should always copy. - [x] bug: `x` cut action does not copy the hovered character if not in selection mode, though should always copy.
- [x] make diff comments word wrap / line wrap by default - [x] make diff comments word wrap / line wrap by default
- [ ] currently, a review comment seems to be attached even if the user doesn't "save" or write the comment buffer (e.g. using `:w`). just closing it with escape still seems to write it to the db. - [x] currently, a review comment seems to be attached even if the user doesn't "save" or write the comment buffer (e.g. using `:w`). just closing it with escape still seems to write it to the db.
- comments should be discardable - comments should be discardable
- [ ] cycle previous commands and search text in command and search input when pressing arrows up/down or ctrl+j/k.
- [ ] 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.
+3 -3
View File
@@ -43,7 +43,7 @@ Rust tests live under [`tests/`](../tests/). Private unit tests are in [`tests/u
- Editor checks cover Unicode editing, multi-target state, undo/redo, saving failures, and modal paste acceptance and rejection atomicity. - Editor checks cover Unicode editing, multi-target state, undo/redo, saving failures, and modal paste acceptance and rejection atomicity.
- Visual-command checks preserve directed characterwise/linewise ranges and multiple carets through command cancellation and plugin dispatch, exercise remapped command entry, and submit `:diff.comment` on either historical side. Live PTY smoke selects lines, cancels a prompt, then runs `:diff.comment` directly and verifies the saved original range without using the palette. - Visual-command checks preserve directed characterwise/linewise ranges and multiple carets through command cancellation and plugin dispatch, exercise remapped command entry, and submit `:diff.comment` on either historical side. Live PTY smoke selects lines, cancels a prompt, then runs `:diff.comment` directly and verifies the saved original range without using the palette.
- Presentation regressions render retained characterwise/linewise highlights during command entry in ordinary, split-diff, and unified views. Floating-review checks preserve source rectangles and the workspace layout, place the caret inside the overlay, retain dismissed drafts across tab switches, and restore source focus on Escape or pane close. Live PTY smoke edits, dismisses, reopens, and saves a floating comment and summary before returning to the unchanged diff. - Presentation regressions render retained characterwise/linewise highlights during command entry in ordinary, split-diff, and unified views. Floating-review checks preserve source rectangles and the workspace layout, place the caret inside the overlay, retain explicitly saved text across tab switches, and restore source focus on Escape or pane close.
- Review prose checks render long comments and summaries with wrapping enabled by default in split/unified views, preserve exact single-line text and source wrapping, and retain an explicit wrap toggle on reopening. Live PTY smoke displays a long comment across continuation rows, saves it, verifies exact text through the review CLI, and reopens it still wrapped. - Review prose checks render long comments and summaries with wrapping enabled by default in split/unified views, preserve exact single-line text and source wrapping, and retain an explicit wrap toggle on reopening. Live PTY smoke displays a long comment across continuation rows, saves it, verifies exact text through the review CLI, and reopens it still wrapped.
- Precise-range checks cover forward/reversed characterwise and partial-multiline selections on both sides, linewise and caret/empty-line fallbacks, source-column navigation, Unicode grapheme bounds, invalid schemas, and preserved save-conflict checks. Render checks cover exact text/newline cells, wrapping, horizontal clipping, shared unified context, stale suppression, and native Visual precedence. Live PTY smoke saves and reopens a selection containing a wide and combining grapheme. - Precise-range checks cover forward/reversed characterwise and partial-multiline selections on both sides, linewise and caret/empty-line fallbacks, source-column navigation, Unicode grapheme bounds, invalid schemas, and preserved save-conflict checks. Render checks cover exact text/newline cells, wrapping, horizontal clipping, shared unified context, stale suppression, and native Visual precedence. Live PTY smoke saves and reopens a selection containing a wide and combining grapheme.
- Markdown navigation checks cover inline/reference destinations, escaped/percent-encoded paths, heading markup and duplicates, same-document fragments, dirty target reuse, failed navigation atomicity, primary-only following, and `gf` prefix cancellation/configuration overrides. - Markdown navigation checks cover inline/reference destinations, escaped/percent-encoded paths, heading markup and duplicates, same-document fragments, dirty target reuse, failed navigation atomicity, primary-only following, and `gf` prefix cancellation/configuration overrides.
@@ -67,10 +67,10 @@ 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. 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 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.
Manual-resolution checks cover immediate persistence, reopen/export/import, independent stale status, conflict-retained drafts, and incompatible version preservation. CLI checks cover filename-safe dispatch, Unicode excerpts, unresolved filtering/counts, absent storage, read-only access and permissions, symlink rejection, and incompatible schema preservation. Manual-resolution checks cover immediate persistence, reopen/export/import, independent stale status, conflict-retained drafts, and incompatible version preservation. CLI checks cover filename-safe dispatch, Unicode excerpts, unresolved filtering/counts, absent storage, read-only access and permissions, symlink rejection, and incompatible schema preservation.
Live PTY/CLI smoke composes and autosaves a comment, resolves it in the editor, observes an empty default CLI comment list with explicit counts, retrieves the resolved comment and exact excerpt with `--all`, and unresolves it. Each transition advances the same stored review revision; listing absent storage creates no data directory. Live PTY/CLI smoke composes and explicitly saves a comment, resolves it in the editor, observes an empty default CLI comment list with explicit counts, retrieves the resolved comment and exact excerpt with `--all`, and unresolves it. Each transition advances the same stored review revision; listing absent storage creates no data directory.
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.
+3 -3
View File
@@ -456,7 +456,7 @@ Review marks belong only to the open comparison and survive filtering, file navi
**Local review comments** are independent of GitHub and the file-reviewed checkmarks above. 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** are independent of GitHub and the file-reviewed checkmarks above. 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.
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; drafts remain available for reopening and saving. Insert/Visual/Command Escape first retains its usual modal behavior. Source rows are never replaced with comment text. Magenta **`○`** gutter markers identify 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 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.
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.
@@ -475,11 +475,11 @@ Local review buffers (comments, summaries, overview, and stored-review selector)
| `: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.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 | | `:diff.review.new` | Create a separate stored review for the current repository |
Reviews save automatically to a local SQLite database when you dismiss or switch comment/summary editors. Verdicts, deletions, and manual resolution changes 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. 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. 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.
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. 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. Write errors, locked databases, incompatible schemas, and conflicting edits from another editor instance retain dirty drafts and report the failure. Failed text saves leave the committed review unchanged; unrelated metadata autosaves cannot commit those drafts. Normal destructive closes remain guarded until drafts are explicitly saved or discarded. `: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 3 JSON is the agent-readable sharing format: repository root, summary, optional verdict, and comments with stable IDs, bodies, required boolean `resolved` states, and anchors. New comments are unresolved. 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. Older formats, including version 2, are rejected and preserved without migration. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. Each stored or exported review is limited to 64 MiB. Version 3 JSON is the agent-readable sharing format: repository root, summary, optional verdict, and comments with stable IDs, bodies, required boolean `resolved` states, and anchors. New comments are unresolved. 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. Older formats, including version 2, are rejected and preserved without migration. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. Each stored or exported review is limited to 64 MiB.
+110 -21
View File
@@ -19,6 +19,7 @@ pub(super) struct ReviewBuffer {
owner: usize, owner: usize,
kind: ReviewText, kind: ReviewText,
title: String, title: String,
pending_comment: Option<Comment>,
} }
impl Editor { impl Editor {
@@ -108,6 +109,7 @@ impl Editor {
.as_ref() .as_ref()
.is_some_and(|binding| binding.owner == self.tab_id) .is_some_and(|binding| binding.owner == self.tab_id)
{ {
buffer.review_buffer.as_mut().unwrap().pending_comment = None;
buffer.saved_lines.clone_from(&buffer.lines); buffer.saved_lines.clone_from(&buffer.lines);
buffer.dirty_cache.set(Some(false)); buffer.dirty_cache.set(Some(false));
} }
@@ -115,11 +117,13 @@ impl Editor {
} }
pub(super) fn autosave_local_review(&mut self) -> Result<(), String> { pub(super) fn autosave_local_review(&mut self) -> Result<(), String> {
self.sync_review_text();
let Some(directory) = self.review_directory.as_deref() else { let Some(directory) = self.review_directory.as_deref() else {
return Ok(()); return Ok(());
}; };
if !self.current_review_dirty() { if !self
.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)?;
@@ -130,7 +134,7 @@ impl Editor {
.local_review .local_review
.document, .document,
)?; )?;
self.mark_review_saved(); 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> {
@@ -153,8 +157,18 @@ 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() { if let Some(mut binding) = self.active.review_buffer.take() {
self.set_message(format!("Review not saved: {error}")); let text = self.active.saved_lines.join("\n");
let wrap = self.active.view.word_wrap;
let mut buffer = Buffer::from_text(self.active.id, None, None, &text, None)
.expect("saved review text is valid");
buffer.view.word_wrap = wrap;
buffer.resolver_readonly = self.active.resolver_readonly;
if binding.pending_comment.take().is_some() {
self.review_diff_mut().unwrap().local_review.selected = None;
}
buffer.review_buffer = Some(binding);
self.active = buffer;
} }
self.select_pane(source); self.select_pane(source);
self.bind_diff_buffer(); self.bind_diff_buffer();
@@ -242,7 +256,7 @@ impl Editor {
), ),
ReviewSource::New => (Review::new(root), None, true), ReviewSource::New => (Review::new(root), None, true),
}; };
// Retire old identities before autosave can synchronize their native drafts. // 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.review_diff_mut().unwrap().local_review;
state.document = document; state.document = document;
@@ -276,7 +290,7 @@ impl Editor {
self.open_review_overview() self.open_review_overview()
} }
fn sync_review_text(&mut self) { fn sync_review_text(&mut self) -> Vec<(ReviewText, Option<String>)> {
let owner = self.tab_id; let owner = self.tab_id;
let updates: Vec<_> = std::iter::once(&self.active) let updates: Vec<_> = std::iter::once(&self.active)
.chain(self.parked.values()) .chain(self.parked.values())
@@ -284,13 +298,34 @@ impl Editor {
let binding = buffer.review_buffer.as_ref()?; let binding = buffer.review_buffer.as_ref()?;
(binding.owner == owner (binding.owner == owner
&& matches!(binding.kind, ReviewText::Comment(_) | ReviewText::Summary)) && matches!(binding.kind, ReviewText::Comment(_) | ReviewText::Summary))
.then(|| (binding.kind, buffer.lines.join("\n"))) .then(|| {
(
binding.kind,
buffer.lines.join("\n"),
binding.pending_comment.clone(),
)
})
}) })
.collect(); .collect();
let Some(diff) = self.review_diff_mut() else { let Some(diff) = self.review_diff_mut() else {
return; return Vec::new();
}; };
for (kind, text) in updates { let mut previous = Vec::new();
for (kind, text, pending) in updates {
if let Some(mut comment) = pending
&& !diff
.local_review
.document
.comments
.iter()
.any(|entry| entry.id == comment.id)
{
previous.push((kind, None));
comment.body = text;
diff.local_review.document.comments.push(comment);
diff.local_review.dirty = true;
continue;
}
let destination = match kind { let destination = match kind {
ReviewText::Comment(id) => diff ReviewText::Comment(id) => diff
.local_review .local_review
@@ -306,14 +341,48 @@ impl Editor {
if let Some(destination) = destination if let Some(destination) = destination
&& *destination != text && *destination != text
{ {
*destination = text; let old = std::mem::replace(destination, text);
previous.push((kind, Some(old)));
diff.local_review.dirty = true; diff.local_review.dirty = true;
} }
} }
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> {
self.sync_review_text(); let dirty = self
.review_diff_mut()
.ok_or("No comparison is open")?
.local_review
.dirty;
let previous = self.sync_review_text();
let result = self.write_local_review(path);
if result.is_err() {
let state = &mut self.review_diff_mut().unwrap().local_review;
for (kind, text) in previous {
match (kind, text) {
(ReviewText::Comment(id), None) => {
state.document.comments.retain(|comment| comment.id != id)
}
(ReviewText::Comment(id), Some(text)) => {
state
.document
.comments
.iter_mut()
.find(|comment| comment.id == id)
.unwrap()
.body = text;
}
(ReviewText::Summary, Some(text)) => state.document.summary = text,
_ => unreachable!("only editable review text is synchronized"),
}
}
state.dirty = dirty;
}
result
}
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.review_diff_mut()
@@ -324,6 +393,7 @@ impl Editor {
self.review_diff_mut().unwrap().local_review.path = Some(path.to_path_buf()); 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.set_message(if path.is_some() { self.set_message(if path.is_some() {
"Review exported as JSON; autosave remains in SQLite" "Review exported as JSON; autosave remains in SQLite"
} else { } else {
@@ -408,7 +478,6 @@ impl Editor {
}; };
return self.replace_local_review(root, source); return self.replace_local_review(root, source);
} }
self.sync_review_text();
match action { match action {
Action::DiffComment => self.add_review_comment()?, Action::DiffComment => self.add_review_comment()?,
Action::ReviewNew => self.replace_local_review(root, ReviewSource::New)?, Action::ReviewNew => self.replace_local_review(root, ReviewSource::New)?,
@@ -612,31 +681,46 @@ impl Editor {
.comments .comments
.iter() .iter()
.map(|comment| comment.id) .map(|comment| comment.id)
.chain(
std::iter::once(&self.active)
.chain(self.parked.values())
.filter_map(|buffer| {
let binding = buffer.review_buffer.as_ref()?;
(binding.owner == self.tab_id)
.then_some(binding.pending_comment.as_ref()?.id)
}),
)
.max() .max()
.unwrap_or(0) .unwrap_or(0)
.checked_add(1) .checked_add(1)
.ok_or("Review comment IDs exhausted")?; .ok_or("Review comment IDs exhausted")?;
self.prepare_command(); self.prepare_command();
self.finish_command_mode(); self.finish_command_mode();
let state = &mut self.diff.as_mut().unwrap().local_review; let comment = Comment {
state.document.comments.push(Comment {
id, id,
anchor, anchor,
body: String::new(), body: String::new(),
resolved: false, resolved: false,
}); };
state.dirty = true; self.open_review_comment(id, Some(&comment))?;
self.show_review_comment(id) self.active.review_buffer.as_mut().unwrap().pending_comment = Some(comment);
Ok(())
} }
fn show_review_comment(&mut self, id: u64) -> Result<(), String> { fn show_review_comment(&mut self, id: u64) -> Result<(), String> {
self.open_review_comment(id, None)
}
fn open_review_comment(&mut self, id: u64, draft: Option<&Comment>) -> Result<(), String> {
let diff = self.workspace_diff().unwrap(); let diff = self.workspace_diff().unwrap();
let comment = diff let comment = draft
.local_review .or_else(|| {
diff.local_review
.document .document
.comments .comments
.iter() .iter()
.find(|comment| comment.id == id) .find(|comment| comment.id == id)
})
.ok_or("Comment no longer exists")?; .ok_or("Comment no longer exists")?;
let text = comment.body.clone(); let text = comment.body.clone();
let anchor = &comment.anchor; let anchor = &comment.anchor;
@@ -668,6 +752,7 @@ impl Editor {
}) })
.flatten() .flatten()
}); });
let start = anchor.start;
let owner = self.diff_owner.unwrap(); let owner = self.diff_owner.unwrap();
self.select_pane(owner); self.select_pane(owner);
let diff = self.diff.as_mut().unwrap(); let diff = self.diff.as_mut().unwrap();
@@ -677,6 +762,9 @@ impl Editor {
diff.local_review.pending_navigation = Some(id); diff.local_review.pending_navigation = Some(id);
} }
self.finish_review_navigation(); self.finish_review_navigation();
if draft.is_some() {
self.place_navigation(start);
}
self.open_review_buffer(ReviewText::Comment(id), &text, title)?; self.open_review_buffer(ReviewText::Comment(id), &text, title)?;
if target.is_none() { if target.is_none() {
self.set_message("Comment is stale; original snapshot anchor preserved"); self.set_message("Comment is stale; original snapshot anchor preserved");
@@ -861,13 +949,14 @@ impl Editor {
owner: self.tab_id, owner: self.tab_id,
kind, kind,
title, title,
pending_comment: None,
}); });
buffer.resolver_readonly = matches!(kind, ReviewText::Overview | ReviewText::Reviews); buffer.resolver_readonly = matches!(kind, ReviewText::Overview | ReviewText::Reviews);
buffer.view.word_wrap = true; buffer.view.word_wrap = true;
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 · dismiss to commit · :w saves · :diff.review.save path exports JSON" "Local review · :w saves · Escape discards unsaved text · :diff.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 :diff.review.save <path>"
}); });
+2 -2
View File
@@ -94,9 +94,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
Local SQLite autosave on dismissal/switch; verdicts/deletions commit immediately; :w commits explicitly :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 :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 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/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
+107 -6
View File
@@ -2828,6 +2828,82 @@ fn historical_diff(editor: &mut Editor, root: &Path, before: Vec<String>, after:
assert!(editor.diff_interacting()); assert!(editor.diff_interacting());
} }
#[test]
fn unsaved_review_comments_are_discarded_on_escape() {
let directory = Directory::new();
let root = directory.0.canonicalize().unwrap();
let storage = root.join("data");
let mut editor = editor();
editor.set_review_directory(storage.clone());
historical_diff(&mut editor, &root, vec!["old".into()], vec!["new".into()]);
editor.execute("diff.comment");
type_text(&mut editor, "iDiscard me");
editor.handle(Input::Escape);
editor.handle(Input::Escape);
assert!(
editor
.workspace_diff()
.unwrap()
.local_review
.document
.comments
.is_empty()
);
assert!(
crate::review::Store::open(&storage)
.unwrap()
.list(&root)
.unwrap()
.is_empty()
);
editor.execute("diff.comment");
assert_eq!(editor.context().text, "");
type_text(&mut editor, "iKeep me");
editor.handle(Input::Escape);
editor.execute("w");
editor.handle(Input::Escape);
editor.execute("diff.review.edit 1");
type_text(&mut editor, "A discarded");
editor.handle(Input::Escape);
editor.handle(Input::Escape);
editor.execute("diff.review.edit 1");
assert_eq!(editor.context().text, "Keep me");
editor.handle(Input::Char('u'));
assert_eq!(editor.context().text, "Keep me");
let reviews = crate::review::Store::open(&storage)
.unwrap()
.list(&root)
.unwrap();
assert_eq!(reviews[0].comments[0].body, "Keep me");
}
#[test]
fn review_metadata_autosaves_do_not_commit_hidden_comment_or_summary_drafts() {
let directory = Directory::new();
let root = directory.0.canonicalize().unwrap();
let storage = root.join("data");
let mut editor = editor();
editor.set_review_directory(storage.clone());
historical_diff(&mut editor, &root, vec!["old".into()], vec!["new".into()]);
editor.execute("diff.comment");
type_text(&mut editor, "iHidden comment");
editor.handle(Input::Escape);
editor.execute("diff.review.summary");
type_text(&mut editor, "iHidden summary");
editor.handle(Input::Escape);
editor.execute("diff.review.verdict looks_good");
let store = crate::review::Store::open(&storage).unwrap();
let saved = store.list(&root).unwrap().pop().unwrap();
assert!(saved.comments.is_empty());
assert_eq!(saved.summary, "");
assert!(editor.local_reviews_dirty());
editor.execute("w");
let saved = store.load(&root, saved.storage_id.unwrap()).unwrap();
assert_eq!(saved.comments[0].body, "Hidden comment");
assert_eq!(saved.summary, "Hidden summary");
assert!(!editor.local_reviews_dirty());
}
#[test] #[test]
fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() { fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
use crate::review::{Review, Store, Verdict}; use crate::review::{Review, Store, Verdict};
@@ -2853,6 +2929,7 @@ fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
.unwrap() .unwrap()
.is_empty() .is_empty()
); );
first.execute("w");
first.handle(Input::Escape); first.handle(Input::Escape);
let id = first let id = first
.workspace_diff() .workspace_diff()
@@ -2875,6 +2952,7 @@ fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
first.execute("diff.review.summary"); first.execute("diff.review.summary");
type_text(&mut first, "iSummary"); type_text(&mut first, "iSummary");
first.handle(Input::Escape); first.handle(Input::Escape);
first.execute("w");
first.execute("diff.review.verdict changes_requested"); first.execute("diff.review.verdict changes_requested");
let persisted = store.load(&root, id).unwrap(); let persisted = store.load(&root, id).unwrap();
assert_eq!(persisted.summary, "Summary"); assert_eq!(persisted.summary, "Summary");
@@ -2884,6 +2962,7 @@ fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
first.execute("diff.review.edit 1"); first.execute("diff.review.edit 1");
type_text(&mut first, "A revised"); type_text(&mut first, "A revised");
first.handle(Input::Escape); first.handle(Input::Escape);
first.execute("w");
first.handle(Input::Escape); first.handle(Input::Escape);
assert_eq!( assert_eq!(
store.load(&root, id).unwrap().comments[0].body, store.load(&root, id).unwrap().comments[0].body,
@@ -2971,6 +3050,7 @@ fn review_resolution_persists_without_hiding_comments_or_changing_stale_anchors(
first.execute("diff.comment"); first.execute("diff.comment");
type_text(&mut first, "iFeedback"); type_text(&mut first, "iFeedback");
first.handle(Input::Escape); first.handle(Input::Escape);
first.execute("w");
assert!( assert!(
first first
.review_panel_title() .review_panel_title()
@@ -3077,6 +3157,7 @@ fn sqlite_review_conflicts_and_write_failures_keep_dirty_drafts() {
first.execute("diff.comment"); first.execute("diff.comment");
type_text(&mut first, "iStored feedback"); type_text(&mut first, "iStored feedback");
first.handle(Input::Escape); first.handle(Input::Escape);
first.execute("w");
first.handle(Input::Escape); first.handle(Input::Escape);
let id = first let id = first
.workspace_diff() .workspace_diff()
@@ -3120,8 +3201,11 @@ fn sqlite_review_conflicts_and_write_failures_keep_dirty_drafts() {
first.execute("diff.review.edit 1"); first.execute("diff.review.edit 1");
type_text(&mut first, "A unsaved"); type_text(&mut first, "A unsaved");
first.handle(Input::Escape); first.handle(Input::Escape);
first.handle(Input::Escape); first.execute("w");
assert!(first.current_review_dirty()); assert!(first.current_review_dirty());
assert_eq!(first.context().text, "Stored feedback unsaved");
first.handle(Input::Escape);
assert!(!first.current_review_dirty());
assert_eq!( assert_eq!(
first first
.workspace_diff() .workspace_diff()
@@ -3130,7 +3214,7 @@ fn sqlite_review_conflicts_and_write_failures_keep_dirty_drafts() {
.document .document
.comments[0] .comments[0]
.body, .body,
"Stored feedback unsaved" "Stored feedback"
); );
assert_eq!(fs::read_to_string(blocked).unwrap(), "preserve"); assert_eq!(fs::read_to_string(blocked).unwrap(), "preserve");
assert_eq!( assert_eq!(
@@ -3296,6 +3380,10 @@ fn precise_review_ranges_capture_graphemes_and_navigate_to_their_start() {
type_text(&mut editor, keys); type_text(&mut editor, keys);
type_text(&mut editor, ":diff.comment"); type_text(&mut editor, ":diff.comment");
editor.handle(Input::Enter); editor.handle(Input::Enter);
editor.execute(&format!(
"diff.review.save {}",
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 = &diff.local_review.document.comments[0].anchor;
let (start, end) = if keys == "llvj" { let (start, end) = if keys == "llvj" {
@@ -3331,6 +3419,10 @@ fn precise_review_ranges_capture_graphemes_and_navigate_to_their_start() {
editor.diff.as_mut().unwrap().focus = Focus::After; editor.diff.as_mut().unwrap().focus = Focus::After;
editor.bind_diff_buffer(); editor.bind_diff_buffer();
editor.execute("diff.comment"); editor.execute("diff.comment");
editor.execute(&format!(
"diff.review.save {}",
root.join("range.json").display()
));
let anchor = &editor let anchor = &editor
.workspace_diff() .workspace_diff()
.unwrap() .unwrap()
@@ -3420,6 +3512,10 @@ fn local_review_visual_commands_and_palette_preserve_both_side_ranges() {
type_text(&mut editor, ":diff.comment"); type_text(&mut editor, ":diff.comment");
} }
editor.handle(Input::Enter); editor.handle(Input::Enter);
editor.execute(&format!(
"diff.review.save {}",
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 = diff.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 });
@@ -3491,15 +3587,20 @@ 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_eq!( assert!(
editor editor
.workspace_diff() .workspace_diff()
.unwrap() .unwrap()
.local_review .local_review
.document .document
.comments[0] .comments
.body, .is_empty()
"Unsaved feedback" );
assert!(
editor
.parked
.values()
.any(|buffer| buffer.lines == ["Unsaved feedback"])
); );
let path = root.join("review.json"); let path = root.join("review.json");
editor.execute(&format!("diff.review.save {}", path.display())); editor.execute(&format!("diff.review.save {}", path.display()));
+15
View File
@@ -7936,6 +7936,11 @@ fn native_visual_selection_overrides_selected_review_range_background() {
} }
editor.handle(Input::Char('\t')); editor.handle(Input::Char('\t'));
editor.execute("diff.comment"); editor.execute("diff.comment");
let directory = Directory::new();
editor.execute(&format!(
"diff.review.save {}",
directory.0.join("review.json").display()
));
editor.handle(Input::Escape); editor.handle(Input::Escape);
let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(100, 20)).unwrap(); let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(100, 20)).unwrap();
let mut views = BufferViewports::default(); let mut views = BufferViewports::default();
@@ -8007,6 +8012,11 @@ fn review_editor_floats_over_the_diff_without_changing_workspace_layout() {
} }
assert_eq!(editor.context().text, "Retained draft"); assert_eq!(editor.context().text, "Retained draft");
assert_eq!(editor.review_panel(), Some(panel)); assert_eq!(editor.review_panel(), Some(panel));
let directory = Directory::new();
editor.execute(&format!(
"diff.review.save {}",
directory.0.join("review.json").display()
));
editor.handle(Input::Escape); editor.handle(Input::Escape);
assert!(editor.review_panel().is_none()); assert!(editor.review_panel().is_none());
assert_eq!(editor.pane_id(), owner); assert_eq!(editor.pane_id(), owner);
@@ -8061,6 +8071,11 @@ fn review_prose_wraps_by_default_without_changing_text_or_source_wrapping() {
.expect("comment tail must be visible without horizontal scrolling"); .expect("comment tail must be visible without horizontal scrolling");
assert!(tail > first, "{command}: {screen}"); assert!(tail > first, "{command}: {screen}");
assert_eq!(editor.context().text, text); assert_eq!(editor.context().text, text);
let directory = Directory::new();
editor.execute(&format!(
"diff.review.save {}",
directory.0.join("review.json").display()
));
editor.handle(Input::Escape); editor.handle(Input::Escape);
assert!(!editor.word_wrap()); assert!(!editor.word_wrap());
} }