From a05dd234ca89818328699429a42e43d147b79cb1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 17:47:26 +0000 Subject: [PATCH 1/4] fix: drop a redundant usize cast in the git panel `unicode_width::UnicodeWidthStr::width` already returns `usize`, so the cast is a no-op that a newer clippy flags under `unnecessary_cast`, failing the `cargo clippy -- -D warnings` gate on an otherwise clean tree. Behavior is unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GGu65bNJ18EqsyGDmd74ZX --- src/ui/git_panel.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/git_panel.rs b/src/ui/git_panel.rs index d9c1647..02f0030 100644 --- a/src/ui/git_panel.rs +++ b/src/ui/git_panel.rs @@ -454,7 +454,7 @@ pub(super) fn history_item( ]); // Right-align the short sha, leaving one trailing cell of margin. let sha_w = entry.short_sha.chars().count(); - let used = subject_line.width() as usize; + let used = subject_line.width(); let pad = content_width.saturating_sub(used + sha_w + 1).max(1); subject_line.spans.push(Span::raw(" ".repeat(pad))); subject_line.spans.push(Span::styled( From f3189d6d903b365dca2ca48f6039f91c2e6af5ba Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 17:47:44 +0000 Subject: [PATCH 2/4] feat(review): swap reviews without pausing first, and land on the working tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes to how a review session ends, both aimed at the same thing: the reviewer should never be forced out of redquill to get somewhere else. Confirming a second review while one is open used to refuse with "already reviewing X — press q to finish or pause". It now pauses the open review first and starts the requested one, on both launcher tabs. The pause is also what makes the swap correct: the incoming review's worktree, base-ref, and fetch machinery all read `stage_ops`/`repo_root`, which point *inside* the outgoing review's worktree until the re-root happens. Confirming the branch or PR already under review stays a no-op — there is nothing to swap to, and rebuilding the session would only lose the reviewer's place. Pause, finish, and (with nothing else to unwind) `Esc` no longer quit. All three re-root back onto the origin checkout's working tree — the view redquill opens in — through one shared `App::leave_review_session`, so what a review leaves behind can't drift between them. Pause and swap keep the worktree, the persisted progress, and every annotation; finish still removes the worktree and deletes the state entry first. `Q`/Ctrl-C is now the one keypress that leaves redquill from inside a review, and `q` takes two presses (end the review, then quit the working-tree view it lands on). Finish's stdout contract is unchanged from a consumer's side. Since it no longer quits, the session's annotations move to `App::finished_annotations` as it unwinds — out of the live store, so they can't bleed into the next target's list panel or the next review's persisted state — and `main` renders that buffer on exit regardless of the final `QuitOutcome`: an explicit finish must not be undone by a later `Q`. Each annotation still reaches a consumer exactly once, in the same markdown format. `review_origin_root` is now recorded by every session entry point (it was PR-only), since leaving needs somewhere to return to. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GGu65bNJ18EqsyGDmd74ZX --- src/annotate/store.rs | 21 ++ src/main.rs | 20 +- src/ui/app.rs | 31 ++- src/ui/end_review.rs | 242 +++++++++++++++-- src/ui/end_review_modal.rs | 11 +- src/ui/keymap.rs | 7 +- src/ui/mod.rs | 70 +++-- src/ui/modal_keys.rs | 82 ++++-- src/ui/modes.rs | 51 ++-- src/ui/pr_description_tests.rs | 18 +- src/ui/review_launcher.rs | 133 ++++++--- src/ui/review_launcher_integration_tests.rs | 252 ++++++++++++++++++ .../review_persistence_integration_tests.rs | 33 ++- 13 files changed, 807 insertions(+), 164 deletions(-) diff --git a/src/annotate/store.rs b/src/annotate/store.rs index 4a946fd..65e7d54 100644 --- a/src/annotate/store.rs +++ b/src/annotate/store.rs @@ -189,6 +189,27 @@ impl AnnotationStore { pub fn unpublished(&self) -> impl Iterator { self.annotations.iter().filter(|a| !a.published) } + + /// Moves every annotation out of `self` (leaving it empty) and appends + /// them to the end of `dest`, in insertion order. Ids are reassigned from + /// `dest`'s own counter so the merged store keeps the "stable ordinal, + /// never reused" guarantee `add` establishes — the two stores were + /// numbered independently, so carrying the original ids across would + /// collide. + /// + /// Used when a finished review session hands its annotations to the + /// process-lifetime emit buffer (`ui::App::finished_annotations`): the + /// session's own store is cleared as it returns to the working tree, + /// while the annotations it produced survive to be rendered to stdout + /// exactly once on exit. + pub fn drain_into(&mut self, dest: &mut AnnotationStore) { + for mut annotation in std::mem::take(&mut self.annotations) { + annotation.id = dest.next_id; + dest.next_id += 1; + dest.annotations.push(annotation); + } + self.next_id = 0; + } } #[cfg(test)] diff --git a/src/main.rs b/src/main.rs index 832cdb8..f0884c7 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,9 +328,19 @@ fn run_tui(config: &RunConfig) -> anyhow::Result<()> { )); let outcome = ui::run(&mut app)?; + // Reviews finished during the run already handed their annotations over + // (see `App::finished_annotations`); finish promised to emit them, so they + // go out regardless of how the process was eventually quit. The live + // session's own annotations join them only on `QuitOutcome::Emit`. One + // render, one presentation — a consumer still sees each annotation exactly + // once. + let mut emitted = std::mem::take(&mut app.finished_annotations); if let QuitOutcome::Emit = outcome { - let markdown = render_markdown(&app.annotations); - present_annotations(&markdown, app.annotations.len(), config.output.as_deref())?; + app.annotations.drain_into(&mut emitted); + } + if !emitted.is_empty() { + let markdown = render_markdown(&emitted); + present_annotations(&markdown, emitted.len(), config.output.as_deref())?; } Ok(()) diff --git a/src/ui/app.rs b/src/ui/app.rs index 288fde8..d019a94 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -274,6 +274,17 @@ pub struct App { pub(super) help: HelpOverlayState, /// Annotations accumulated this session. pub annotations: AnnotationStore, + /// Annotations carried over from review sessions *finished* during this + /// run. Finishing a review no longer quits — it returns to the working + /// tree (see [`super::end_review::LeaveReason`]) — so the session's + /// annotations are moved here as it unwinds rather than left in + /// `annotations`, where they would bleed into the next target's list + /// panel and into the next review's persisted state. `main` renders this + /// buffer to stdout on exit regardless of the final [`super::QuitOutcome`] + /// (a later `Q` must not silently drop what an explicit finish already + /// promised to emit), so each annotation still reaches a consumer exactly + /// once. Empty unless a review was finished this run. + pub finished_annotations: AnnotationStore, /// The current interaction mode. pub mode: Mode, /// The Compose modal's state, when `mode == Mode::Compose`. @@ -763,9 +774,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 @@ -882,6 +896,7 @@ impl App { view: DiffViewState::new(files), help: HelpOverlayState::new(), annotations, + finished_annotations: AnnotationStore::new(), mode: Mode::Normal, compose: None, commit_message: None, @@ -1148,6 +1163,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..5fbcb14 100644 --- a/src/ui/end_review.rs +++ b/src/ui/end_review.rs @@ -4,15 +4,74 @@ //! 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 in what they leave behind, not in where they land +//! (see [`LeaveReason`]): pause keeps the worktree, the persisted state, and +//! every annotation on disk; finish first removes the worktree and deletes the +//! persisted entry ([`App::finish_review`]) and carries the session's +//! annotations into [`App::finished_annotations`] for the one stdout emission +//! finish has always promised; a swap behaves exactly like pause, so the +//! review being left is resumable the moment the new one is done. -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 — the one thing that differs between +/// the three exits, all of which land on the origin working tree via +/// [`App::leave_review_session`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum LeaveReason { + /// `p` in the end-review modal, or `Esc` from the diff view. Everything + /// (worktree, persisted state, annotations) stays on disk; the session's + /// in-memory annotations are dropped rather than emitted, so a consumer + /// reading stdout still sees each one exactly once — on finish. + Pause, + /// `f` in the end-review modal, after [`App::finish_review`] has already + /// removed the worktree and deleted the persisted entry. The session's + /// annotations move to [`App::finished_annotations`] to be emitted on + /// exit. + 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