fix: drop a redundant usize cast in the git panel - #53
Merged
Conversation
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGu65bNJ18EqsyGDmd74ZX
…king tree 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGu65bNJ18EqsyGDmd74ZX
Correcting the previous commit, which built a `finished_annotations` buffer
so finish could still reach the annotation presentation after it stopped
quitting. That was based on a wrong reading of what finish did: the
presentation is `present_annotations`, which copies to the *clipboard* (plus
`-o <file>`) and only writes markdown to stdout when the clipboard is
unavailable — so "finish emits to stdout" was stale doc language, not
behavior, and deferring it to process exit would have concatenated two
finished reviews into one clipboard copy at exit rather than giving each its
own.
A review's annotations already have their destinations: `review-state.json`
while it is open, and the forge submit flow for a PR/MR. So none of the three
exits presents anything. `leave_review_session` simply clears the session's
annotations — which is also what keeps them out of the working tree's list
panel and out of the next review's persisted state — and `main`'s
presentation stays exactly as it was, reachable only from a non-review
session's `q`.
`AnnotationStore::drain_into` and `App::finished_annotations` go with it.
`QuitOutcome`'s doc now states the clipboard destination and that it is a
non-review-session contract. Finish's option row names the consequence
("remove worktree & comments") rather than the modal caption promising an
emission that never happens.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GGu65bNJ18EqsyGDmd74ZX
`U` (submit-forge-review) was inert outside a forge PR review — "submit unavailable — not a PR review" — which left every other target with no way to hand a review to an agent short of quitting redquill. It now means the same thing everywhere: hand off my review. On a PR that is still the submit modal, unchanged, verdict picker and confirm and all. On the working tree, `--staged`, a commit or range, or a local branch review with no PR behind it, it copies the annotations to the clipboard as the same public markdown `main` presents on quit, and reports it in the footer. No modal on the copy path: there is no verdict to choose and nothing irreversible to confirm, so a confirm step would be ceremony in front of a clipboard write. Copying consumes nothing — the reviewer can keep annotating and copy again, and the same annotations still reach the on-quit presentation. `U` is therefore no longer hidden by `binding_hidden`'s review-session gate, so the `?` overlay and the footer strip list it on every target, including the plain working tree where most sessions start. `main`'s inline `copy_to_clipboard` moves to a shared `clipboard` module. It caches its `arboard` handle for the process lifetime: on X11 the clipboard is served by the owning process, so the create-and-drop shape `main` could get away with on the way out would leave a mid-session copy pointing at nothing while the TUI is still on screen. The clipboard write is injected into the copy gesture as a closure, so the suite covers what actually reaches the clipboard without `cargo test` writing to the developer's own — the same host-isolation rule the tempdir convention exists for. The empty-set path returns before touching it at all. The `[keys]` config id stays `submit-forge-review` despite the broader meaning: renaming it would silently break existing config files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGu65bNJ18EqsyGDmd74ZX
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
unicode_width::UnicodeWidthStr::widthalready returnsusize, so thecast is a no-op that a newer clippy flags under
unnecessary_cast,failing the
cargo clippy -- -D warningsgate on an otherwise cleantree. Behavior is unchanged.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01GGu65bNJ18EqsyGDmd74ZX