diff --git a/CLAUDE.md b/CLAUDE.md index 7bcbd49..c2b483b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -41,6 +41,7 @@ Keep these boundaries clean; they're the seams for testing and for future work: - `git/` — runs git commands, parses porcelain/diff output into typed structs. No TUI types leak in here. - `diff/` — diff model: files, hunks, lines, intra-line word diff. Pure data + transforms; heavily unit-tested. - `annotate/` — annotation model, persistence, stdout serialization. +- `clipboard.rs` — the one seam around `arboard`, shared by `ui`'s in-app copy gesture (`U` off a PR) and `main`'s on-quit presentation. Caches its handle for the process lifetime: on X11 the clipboard is served by the owning process, so a handle created and dropped per copy would leave the reviewer with nothing while the TUI is still running. - `lsp/` — server lifecycle + the three requests. Must be fully async and never block the render loop; missing/slow servers degrade silently. - `ui/` — ratatui widgets, layout, event loop, keymap. Keymap is data (remappable), not hardcoded match arms scattered through widgets. - `review/` — per-file review-status model (spec 08 Unit 3, docs/specs/08-spec-branch-review-mode/08-spec-branch-review-mode.md): pure `ReviewStatus` tri-state and transition functions, no TUI types. Persistence (`review-state.json`, blob-SHA reconciliation) lands as a `review::store` submodule in spec 08 task 4.0. diff --git a/README.md b/README.md index d2aa67a..91e1ea7 100644 --- a/README.md +++ b/README.md @@ -25,8 +25,8 @@ brew install sdavisde/tap/redquill 2. Run `redquill` in the git repo you want to review 3. Press \` to open the git panel, and `?` for help — it opens on a "This context" view scoped to wherever you pressed it from (plus a curated list of common workflows), with the full reference one `Tab` away. Pause after `g` or `z` and the footer shows what keys can follow. -4. When viewing the diff, press `c` to leave a comment. When the session ends, your comments are copied to the clipboard so you can paste them straight into an agent. -5. Reviewing a teammate's pull request? Press `R` to open the Review launcher's Pull Requests tab — see [`docs/forge-setup.md`](docs/forge-setup.md) for supported providers and setup. +4. When viewing the diff, press `c` to leave a comment and `U` to hand the review off — your comments are copied to the clipboard as markdown, ready to paste straight into an agent, without leaving redquill. Quitting a plain session copies them too. +5. Reviewing a teammate's pull request? Press `R` to open the Review launcher's Pull Requests tab — see [`docs/forge-setup.md`](docs/forge-setup.md) for supported providers and setup. There `U` submits the review to the forge instead, behind a confirm modal where you pick the verdict. ## Documentation diff --git a/src/clipboard.rs b/src/clipboard.rs new file mode 100644 index 0000000..9eaaa24 --- /dev/null +++ b/src/clipboard.rs @@ -0,0 +1,70 @@ +//! The system clipboard: one thin seam around `arboard`, shared by the two +//! places redquill hands the reviewer their annotations — the in-app +//! `submit-forge-review` gesture on a non-PR target (see +//! `ui::forge_submit`'s copy path) and `main`'s on-quit presentation. +//! +//! # Why the handle is cached +//! +//! On X11 the clipboard is *served by the owning process*: the content lives +//! as long as some live `arboard::Clipboard` keeps announcing ownership, and +//! evaporates when the last one drops. `main`'s exit-time copy can get away +//! with a throwaway handle because the process is about to end either way +//! (the documented X11 caveat). A copy made from inside the running TUI +//! cannot — a handle created and dropped per keypress would leave the +//! reviewer with an empty clipboard while redquill is still on screen, which +//! is the exact case the in-app gesture exists to serve. +//! +//! So the handle is created lazily on first use and kept for the process +//! lifetime, in a `thread_local` rather than on `App`: `arboard::Clipboard` +//! is not `Sync`, both call sites run on the main thread, and keeping it out +//! of `App` avoids threading a non-`Sync` field through a struct whose other +//! members are all plain data. A failed creation is not cached — a clipboard +//! that was unavailable at one keypress (no display server yet, a +//! transiently busy selection owner) may be available at the next. +//! +//! # Error contract +//! +//! Failures surface as [`ClipboardError`] for the caller to report; nothing +//! here degrades silently, because a copy the reviewer asked for and did not +//! get is exactly the thing they must be told about. `main` falls back to +//! writing the markdown on stdout; the TUI reports the error in the footer +//! (it has no fallback — stdout belongs to the annotation format and writing +//! there mid-render would corrupt the screen). + +use std::cell::RefCell; + +/// A clipboard write that didn't land, carrying `arboard`'s own message — +/// the reason is always environmental (no display server, no clipboard +/// backend compiled in, a selection owner that refused), never something the +/// caller passed in, so there is nothing to distinguish beyond the text. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +#[error("{0}")] +pub struct ClipboardError(String); + +thread_local! { + /// The cached handle (see the module doc). `None` until the first + /// successful creation. + static HANDLE: RefCell> = const { RefCell::new(None) }; +} + +/// Copies `text` to the system clipboard, reusing this thread's cached +/// handle (creating it on first use) so the content survives for as long as +/// the process runs. +pub fn copy(text: &str) -> Result<(), ClipboardError> { + HANDLE.with(|cell| { + let mut slot = cell.borrow_mut(); + if slot.is_none() { + // Not cached on failure: see the module doc. + *slot = Some(arboard::Clipboard::new().map_err(|e| ClipboardError(e.to_string()))?); + } + let Some(clipboard) = slot.as_mut() else { + // Unreachable in practice — the block above either filled the + // slot or returned — but written as a fallback rather than an + // `expect`, per this repo's no-panic rule. + return Err(ClipboardError("clipboard unavailable".to_string())); + }; + clipboard + .set_text(text.to_owned()) + .map_err(|e| ClipboardError(e.to_string())) + }) +} diff --git a/src/lib.rs b/src/lib.rs index 346633e..e15ec3e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,6 +4,7 @@ //! integration tests exercise them directly. pub mod annotate; +pub mod clipboard; pub mod config; pub mod diff; pub mod forge; diff --git a/src/main.rs b/src/main.rs index 832cdb8..c3fbae7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -270,6 +270,12 @@ fn run_tui(config: &RunConfig) -> anyhow::Result<()> { let mut app = App::with_git(snapshot, target.clone(), Box::new(runner.clone())); if let DiffTarget::Review { branch, .. } = &target { + // Recorded before `discovered` moves into `set_review_origin_ops`: + // pausing or finishing the review re-roots the app back onto this + // root's working tree rather than exiting (see + // `ui::end_review::App::leave_review_session`), so a `--review` + // launch needs it just as much as a launcher-started session does. + app.set_review_origin_root(discovered.root().to_path_buf()); // Load + reconcile this branch's persisted progress before the // first render, so `Accepted`/`Deferred` files start collapsed and a // stale `Accepted` file starts marked `ChangedSinceAccepted` and @@ -322,6 +328,11 @@ fn run_tui(config: &RunConfig) -> anyhow::Result<()> { )); let outcome = ui::run(&mut app)?; + // Only a non-review session's `q` ever reaches here with `Emit` (see + // `QuitOutcome`): a review's own annotations belong to its persisted + // entry and, for a PR/MR, to the forge submit flow, and are cleared from + // `app.annotations` as the review is left — so nothing a review produced + // can arrive here to be presented. if let QuitOutcome::Emit = outcome { let markdown = render_markdown(&app.annotations); present_annotations(&markdown, app.annotations.len(), config.output.as_deref())?; @@ -354,7 +365,7 @@ fn present_annotations( if count == 0 { return Ok(()); } - match copy_to_clipboard(markdown) { + match redquill::clipboard::copy(markdown) { Ok(()) => { let noun = if count == 1 { "annotation" @@ -373,15 +384,6 @@ fn present_annotations( Ok(()) } -/// Copies `text` to the system clipboard, returning any backend error so the -/// caller can fall back. Kept as a thin seam around `arboard` so the fallback -/// policy lives in one place ([`present_annotations`]). -fn copy_to_clipboard(text: &str) -> anyhow::Result<()> { - let mut clipboard = arboard::Clipboard::new()?; - clipboard.set_text(text.to_owned())?; - Ok(()) -} - fn main() -> anyhow::Result<()> { let cli = Cli::parse(); let config = RunConfig::from(cli); diff --git a/src/ui/annotation_export.rs b/src/ui/annotation_export.rs new file mode 100644 index 0000000..6019fa1 --- /dev/null +++ b/src/ui/annotation_export.rs @@ -0,0 +1,211 @@ +//! Handing the reviewer their annotations without quitting: the clipboard +//! half of the `submit-forge-review` gesture (`U`). +//! +//! A forge PR review submits to the forge; every other target — the working +//! tree, `--staged`, a commit or range, a local branch review with no PR +//! behind it — has nowhere to submit, so the same key copies the annotations +//! to the clipboard in the public markdown format (see +//! [`crate::annotate::render_markdown`], the same bytes `main` presents on +//! quit). Nothing is consumed or cleared by a copy: the reviewer can keep +//! annotating and copy again, and the same annotations still reach `main`'s +//! on-quit presentation. See [`super::forge_submit::App::open_submit_forge`] +//! for the branch that picks between the two destinations. +//! +//! The copy is synchronous. It is a user-initiated one-shot on a cached +//! clipboard handle (see [`crate::clipboard`]) rather than a per-tick or +//! per-keystroke cost, so it doesn't belong on a background thread the way +//! git subprocesses and state saves do. + +use crate::annotate::render_markdown; + +use super::app::App; + +/// The footer line for a copy attempt: `count` annotations were offered and +/// `error` is the clipboard's own message if the write failed. +/// +/// Pure and separate from the gesture so the one thing that actually matters +/// here is testable without a system clipboard: a failed copy must never read +/// as a successful one. A reviewer who is told their review is on the +/// clipboard, and then pastes an empty buffer into an agent prompt, has lost +/// the review — so "clipboard unavailable" has to be as visible as the +/// success line. +pub(super) fn copy_status_message(count: usize, error: Option<&str>) -> String { + match (count, error) { + (0, _) => "no annotations to copy".to_string(), + (_, Some(e)) => format!("clipboard unavailable ({e}) \u{2014} nothing copied"), + (1, None) => "copied 1 annotation to the clipboard".to_string(), + (n, None) => format!("copied {n} annotations to the clipboard"), + } +} + +impl App { + /// Copies every annotation in this session to the clipboard as markdown + /// and reports the outcome in the footer. A no-op (beyond the footer + /// line) with nothing to copy. + /// + /// There is no stdout fallback here, unlike `main`'s on-quit + /// presentation: stdout carries the annotation format for other programs + /// to parse, and writing to it mid-render would corrupt the screen. A + /// clipboard failure is reported and nothing else happens — the reviewer + /// still has the on-quit path, and the annotations are untouched. + pub(super) fn copy_annotations_to_clipboard(&mut self) { + self.copy_annotations_with(|markdown| { + crate::clipboard::copy(markdown).map_err(|e| e.to_string()) + }); + } + + /// [`App::copy_annotations_to_clipboard`]'s body, with the clipboard + /// write injected — the seam this repo puts in front of every external + /// service, kept as a plain closure rather than a trait because there is + /// exactly one operation and no state behind it. + /// + /// It also means the tests never touch the developer's real clipboard: + /// `cargo test` runs on a machine whose clipboard belongs to a person, + /// not to the suite, so a test that exercised the live path would be a + /// host side effect of the kind this repo's tempdir rules exist to + /// prevent. `copy` is not called at all when there is nothing to copy. + fn copy_annotations_with(&mut self, copy: impl FnOnce(&str) -> Result<(), String>) { + let count = self.annotations.len(); + if count == 0 { + self.set_status_message(copy_status_message(0, None)); + return; + } + let markdown = render_markdown(&self.annotations); + let error = copy(&markdown).err(); + self.set_status_message(copy_status_message(count, error.as_deref())); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::annotate::{Classification, Target}; + use crate::diff::FileDiff; + use crate::git::RawFilePatch; + use std::cell::RefCell; + + /// "… to the clipboard" is the phrase that constitutes the promise; a + /// message without it is not claiming the annotations got there. + fn claims_success(message: &str) -> bool { + message.contains("to the clipboard") + } + + /// The guardrail: whichever way a copy goes, the footer must not tell the + /// reviewer their annotations are on the clipboard unless they are. + #[test] + fn a_failed_or_empty_copy_never_reads_as_a_successful_one() { + let ok = copy_status_message(3, None); + assert!(claims_success(&ok), "got {ok:?}"); + assert!(ok.contains('3'), "the count must be reported: {ok:?}"); + + let failed = copy_status_message(3, Some("no display server")); + assert!( + !claims_success(&failed), + "a failed copy must not read as a success: {failed:?}" + ); + assert!( + failed.contains("no display server"), + "the clipboard's own reason must surface: {failed:?}" + ); + + let empty = copy_status_message(0, None); + assert!( + !claims_success(&empty), + "an empty set must not claim a copy: {empty:?}" + ); + } + + fn app_with_annotations(bodies: &[&str]) -> App { + let raw = "\ +diff --git a/src/a.rs b/src/a.rs +index 111..222 100644 +--- a/src/a.rs ++++ b/src/a.rs +@@ -1,2 +1,2 @@ + fn main() { +- old(); ++ new(); +"; + let file = FileDiff::from_patch(&RawFilePatch { + path: "src/a.rs".to_string(), + old_path: None, + raw: raw.to_string(), + is_binary: false, + }) + .unwrap(); + let mut app = App::new(vec![file]); + for body in bodies { + app.annotations + .add(Target::file("src/a.rs"), Classification::Question, *body) + .unwrap(); + } + app + } + + /// What reaches the clipboard is the public markdown format over the + /// whole annotation set — the same bytes `main` presents on quit, since + /// the point of the gesture is to hand an agent a review without quitting + /// first. The defect this catches is the copy shipping a partial set (a + /// stray `unpublished()` filter, say) or some other rendering. + #[test] + fn the_copy_hands_over_the_public_markdown_for_every_annotation() { + let mut app = app_with_annotations(&["first note", "second note"]); + let seen = RefCell::new(None); + + app.copy_annotations_with(|markdown| { + *seen.borrow_mut() = Some(markdown.to_string()); + Ok(()) + }); + + let copied = seen.into_inner().expect("the clipboard write must run"); + assert_eq!(copied, crate::annotate::render_markdown(&app.annotations)); + assert!(copied.contains("first note"), "got {copied:?}"); + assert!(copied.contains("second note"), "got {copied:?}"); + assert!( + app.status_message.as_deref().is_some_and(claims_success), + "got {:?}", + app.status_message + ); + assert_eq!( + app.annotations.len(), + 2, + "a copy must not consume the annotations — the reviewer keeps working" + ); + } + + /// Nothing to copy means the clipboard is never touched at all, rather + /// than an empty string replacing whatever the reviewer had on it. + #[test] + fn an_empty_annotation_set_never_reaches_the_clipboard() { + let mut app = app_with_annotations(&[]); + let called = RefCell::new(false); + + app.copy_annotations_with(|_| { + *called.borrow_mut() = true; + Ok(()) + }); + + assert!(!called.into_inner(), "the clipboard must be left alone"); + assert!( + app.status_message + .as_deref() + .is_some_and(|m| !claims_success(m)), + "got {:?}", + app.status_message + ); + } + + /// A clipboard that refuses is reported, not swallowed — the reviewer + /// must not paste an empty buffer into an agent prompt believing the + /// review went with it. + #[test] + fn a_clipboard_failure_surfaces_instead_of_reading_as_success() { + let mut app = app_with_annotations(&["a note"]); + + app.copy_annotations_with(|_| Err("no display server".to_string())); + + let message = app.status_message.as_deref().unwrap_or_default(); + assert!(!claims_success(message), "got {message:?}"); + assert!(message.contains("no display server"), "got {message:?}"); + } +} diff --git a/src/ui/app.rs b/src/ui/app.rs index 288fde8..4734f36 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -272,7 +272,13 @@ pub struct App { /// mirroring lazygit), and the mode/scope `?` was pressed from. Reset by /// [`Action::ToggleHelp`] whenever the overlay opens or closes. pub(super) help: HelpOverlayState, - /// Annotations accumulated this session. + /// Annotations accumulated this session. Cleared when a review session is + /// left (see [`super::end_review::App::leave_review_session`]): a review's + /// annotations belong to that review — persisted in `review-state.json` + /// and, for a PR/MR, submitted to the forge — and must not follow the + /// reviewer back to the working tree, where they would show up in the + /// list panel against a diff they don't describe and be written into the + /// *next* review's persisted state. pub annotations: AnnotationStore, /// The current interaction mode. pub mode: Mode, @@ -763,9 +769,12 @@ pub struct App { /// git-less/test contexts that never call finish. pub(super) review_origin_ops: Option>, /// The origin repository root the PR-checkout fetch/worktree ops run - /// from (outside any managed worktree), captured when a PR review session - /// starts so a mid-session refresh can re-root a fresh origin runner for - /// the fetch. `None` outside a PR review session. + /// from (outside any managed worktree), captured when a review session + /// starts. A mid-session PR refresh re-roots a fresh origin runner from + /// it for the fetch, and leaving a review ([`App::leave_review_session`]) + /// re-roots the whole app back onto it. Set by every session entry point + /// — the CLI's `--review` bootstrap and both launcher tabs — so leaving + /// always has somewhere to return to; `None` outside a review session. pub(super) review_origin_root: Option, /// The path `/redquill/review-state.json` resolves to /// for this session, set once at startup by @@ -1148,6 +1157,16 @@ impl App { self.review_origin_ops = Some(ops); } + /// Records the origin checkout's root (outside any managed worktree) for + /// this review session — the directory [`App::leave_review_session`] + /// re-roots back onto when the review is paused, finished, or swapped for + /// another. Set alongside [`App::set_review_origin_ops`] by every session + /// entry point; without it, leaving a review has no working tree to + /// return to and degrades to a status message. + pub fn set_review_origin_root(&mut self, root: PathBuf) { + self.review_origin_root = Some(root); + } + /// Sets the path this session persists review progress to /// (`/redquill/review-state.json`), resolved once by /// `main`'s review-session bootstrap before the first render. Every diff --git a/src/ui/end_review.rs b/src/ui/end_review.rs index c15a52d..6f9bd73 100644 --- a/src/ui/end_review.rs +++ b/src/ui/end_review.rs @@ -4,15 +4,79 @@ //! and deleting the persisted state). Split out of `app.rs` alongside this //! state, mirroring [`super::switcher`]'s own state-plus-handlers split. //! -//! Pausing has no dedicated method here: it's exactly the pre-existing quit -//! path (`Flow::Quit(QuitOutcome::Discard)`, handled by -//! [`super::modes::handle_end_review_key`]'s `end_review_choice`). Pause -//! keeps the worktree, review state, and annotations on disk and emits -//! nothing; quit-and-emit prints annotations exactly once. +//! Ending a review does not end the process. Pause, finish, and the +//! swap-to-another-review path all converge on [`App::leave_review_session`], +//! which re-roots the app back onto the origin checkout's working tree — the +//! view redquill opens in by default. Only `Q`/Ctrl-C, and `q` from outside a +//! review, still quit. +//! +//! The three reasons differ only in what they leave *on disk* (see +//! [`LeaveReason`]) and in the status line's wording: pause and a swap keep +//! the worktree and the persisted entry, so the review resumes exactly where +//! it stopped; finish removes both first ([`App::finish_review`]). +//! +//! **Leaving a review never emits.** A review's annotations have their own +//! destinations — `review-state.json` while it is open, and for a PR/MR the +//! forge submit flow (`super::forge_submit`) — so none of the three exits +//! hands anything to the clipboard/stdout presentation `main` runs on quit. +//! That path belongs to non-review sessions, whose `q` still emits exactly as +//! it always has. The session's in-memory annotations are simply dropped as +//! it unwinds, which is also what keeps them out of the working tree's list +//! panel and out of the *next* review's persisted state. -use super::QuitOutcome; use super::app::{App, Mode, ModeOrigin}; use super::modal_keys::EndReviewAction; +use crate::git::{DiffTarget, GitRunner}; + +/// Why a review session is being left. All three land on the origin working +/// tree via [`App::leave_review_session`] and clear the same in-memory state; +/// what they leave on disk was already settled by the caller before it got +/// here, so this only picks the status line's wording. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum LeaveReason { + /// `p` in the end-review modal, or `Esc` from the diff view. The + /// worktree, the persisted entry, and every annotation in it stay on + /// disk, so reopening this review resumes mid-review. + Pause, + /// `f` in the end-review modal, after [`App::finish_review`] has already + /// removed the worktree and deleted the persisted entry — annotations + /// included, since one entry holds both. + Finish, + /// Confirming a different branch or PR in the review launcher while a + /// review is already open. Identical to [`LeaveReason::Pause`] in effect; + /// distinct only so the status line can say the old review was paused + /// rather than claim the user asked to pause it. + Switch, +} + +/// What [`App::pause_review_for_switch`] found and did — the three cases a +/// launcher confirm has to tell apart before it starts a new review. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum SwitchPause { + /// No review was open; start the new one directly. + Nothing, + /// The named review was paused and the app is back on the origin + /// checkout, so `stage_ops`/`repo_root` are the origin's again. The + /// payload is the paused review's banner label, for the caller's + /// "paused X — starting Y" status line. + Paused(String), + /// A review was open and could not be left — the footer already says why. + /// The new review must not start: it would run rooted inside the outgoing + /// review's worktree. + Blocked, +} + +impl SwitchPause { + /// The `"paused