feat: enrich renri remove and add --merged auto-cleanup#65
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe CLI ChangesRemove subcommand multi-mode expansion
Sequence Diagram(s)sequenceDiagram
participant User
participant CLIParser
participant cmd_remove
participant cmd_remove_merged
participant PRCache
participant Confirmation
participant remove_one
participant VCSBackend
User->>CLIParser: remove [name] --merged
alt merged mode
CLIParser->>cmd_remove_merged: dispatch
cmd_remove_merged->>PRCache: load_pr_cache_for_repo
PRCache-->>cmd_remove_merged: PR state map
cmd_remove_merged->>cmd_remove_merged: filter by MERGED/CLOSED
cmd_remove_merged->>cmd_remove_merged: apply skip rules
loop for each candidate
cmd_remove_merged->>cmd_remove_merged: print_worktree_details
end
cmd_remove_merged->>Confirmation: confirm_remove
Confirmation-->>cmd_remove_merged: approved
loop remove each
cmd_remove_merged->>remove_one: worktree, force
remove_one->>VCSBackend: dispatch by kind
VCSBackend-->>remove_one: removed
end
cmd_remove_merged-->>User: results
else single target
CLIParser->>cmd_remove: dispatch
cmd_remove->>PRCache: load_pr_cache_for_repo
PRCache-->>cmd_remove: PR state
cmd_remove->>cmd_remove: print_worktree_details
cmd_remove->>Confirmation: confirm_remove
Confirmation-->>cmd_remove: approved
cmd_remove->>remove_one: worktree, force
remove_one->>VCSBackend: dispatch by kind
VCSBackend-->>remove_one: removed
remove_one-->>User: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR introduces substantial logic expansion with two distinct workflows (single-target and bulk merged-PR removal), each with filtering, confirmation, and error handling. The additions span multiple behavioral checkpoints (CLI contract, PR-cache integration, details rendering, skip-rule evaluation, hook dispatch) distributed across a single file, with significant conditional branching and state tracking (candidate filtering, success/failure counts, skip reasons). Reviewing requires tracing both workflows end-to-end, validating skip rule correctness, and understanding the interaction between PR cache refresh modes, confirmation enforcement, and backend dispatch. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the remove command by introducing a --merged flag to automatically clean up worktrees linked to merged or closed GitHub PRs. It also adds detailed status panels, interactive confirmation prompts, and a --refresh flag for PR metadata. The review feedback focuses on refining the user experience: it suggests allowing the command to proceed even if the PR cache is empty to avoid misleading error messages and recommends enabling the --refresh flag for single-target removals to ensure the information displayed in the new details panel is current.
| if prs.is_empty() { | ||
| anyhow::bail!( | ||
| "no PR data available — install the `gh` CLI and authenticate, \ | ||
| then retry (optionally with --refresh)" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bailing when prs is empty can lead to a misleading error message. If a repository has zero PRs, load_pr_cache_for_repo returns an empty map, which triggers this bail and incorrectly suggests that the gh CLI is missing or unauthenticated. It is better to allow the command to proceed; if no candidates are found, the user will receive the "nothing to remove" message at line 1301, which is a more accurate description of the state. If you wish to warn about missing gh CLI, it should be done by checking for its presence explicitly rather than relying on an empty PR list.
There was a problem hiding this comment.
@gemini-code-assist Good catch — fixed in 48313e0. Dropped the empty-cache bail; the downstream "nothing to remove" path is now the single source of truth for the no-candidates state, and tool-missing vs zero-PRs are no longer conflated.
There was a problem hiding this comment.
Understood, thanks for the update. The approach of using the downstream "nothing to remove" path as the single source of truth for the no-candidates state is much cleaner and avoids the ambiguity of the previous bail. The changes look solid.
| /// Bypass the PR cache and re-fetch from GitHub. Only meaningful | ||
| /// with `--merged`; matches `renri list --refresh`. |
There was a problem hiding this comment.
The documentation for the --refresh flag should be updated to reflect that it is now also applicable to single-target removals. Since the removal process now includes a detailed panel showing PR information, allowing the user to refresh the PR cache ensures they are making decisions based on up-to-date data.
| /// Bypass the PR cache and re-fetch from GitHub. Only meaningful | |
| /// with `--merged`; matches `renri list --refresh`. | |
| /// Bypass the PR cache and re-fetch from GitHub. | |
| /// Matches `renri list --refresh`. |
There was a problem hiding this comment.
@gemini-code-assist Done in 48313e0 — help text now documents both single-target and --merged usage.
| if refresh { | ||
| anyhow::bail!("--refresh only makes sense with --merged"); | ||
| } |
There was a problem hiding this comment.
This restriction on --refresh should be removed. Allowing --refresh for single-target removals is consistent with renri list and provides value by ensuring the PR status shown in the pre-removal details panel is current.
There was a problem hiding this comment.
@gemini-code-assist Done in 48313e0 — removed the guard. --refresh is now valid for single-target removes too and forwards through to load_pr_cache_for_repo.
There was a problem hiding this comment.
The changes look great and correctly address the suggestion to allow --refresh for single-target removals. Thank you for the update.
| // Best-effort PR lookup: even when `[ui] show_pr = false` we still try | ||
| // the cache so a user who has list-with-PRs configured elsewhere gets | ||
| // the same signal here. No `gh` / no cache → silently no PR info. | ||
| let prs = load_pr_cache_for_repo(&opened, &loaded.config, &vcs_ctx, false); |
There was a problem hiding this comment.
Pass the refresh flag to load_pr_cache_for_repo so that single-target removals can benefit from up-to-date PR information in the details panel, assuming the bail at line 1148 is removed.
| let prs = load_pr_cache_for_repo(&opened, &loaded.config, &vcs_ctx, false); | |
| let prs = load_pr_cache_for_repo(&opened, &loaded.config, &vcs_ctx, refresh); |
There was a problem hiding this comment.
@gemini-code-assist Done in 48313e0 — refresh is now passed through. Paired with removing the guard above so the suggested call site behaves as you described.
There was a problem hiding this comment.
The changes look great, and passing the refresh flag correctly ensures consistency across both single-target and batch operations. Thank you for implementing this.
Two related improvements to make `renri remove` safer to call: - Print a detailed pre-remove panel — name, branch, path, vcs, head id + description, status flags (clean / main / stale / dirty / conflict / locked / bare), and PR info (number, state, URL) when the PR cache has a hit. Prompts for confirmation by default; -y/--yes skips it. --non-interactive without --yes is now an error so the guard cannot be silently bypassed. - Add --merged to sweep every worktree whose GitHub PR is MERGED or CLOSED in one go. dirty / conflict / locked / main rows are skipped with a warning (unless --force is also set). Each removal is per-row best-effort; failures are tallied and reported at the end. --refresh forces a re-fetch of the PR cache before deciding. - Add -f/--force which propagates to the backend's remove(force=true), so single-target removes can now drop uncommitted worktrees too (previously always force=false).
- Drop `if prs.is_empty()` bail in cmd_remove_merged. Empty cache is ambiguous (no `gh` / network failure / genuinely zero PRs); the downstream "nothing to remove" message is accurate either way and conflating tool-missing with no-PRs produces misleading errors in a fresh repo. - Remove the `--refresh only makes sense with --merged` guard in cmd_remove. Single-target removes now forward refresh through to the PR cache so the pre-remove details panel reflects current state, matching `renri list --refresh` semantics. - Update the `--refresh` help text to document both modes.
yukimemi
force-pushed
the
feat/remove-details-and-merged
branch
from
a4ceaa1 to
48313e0
Compare
1 participant
Summary
Two related improvements to make
renri removesafer and ergonomic:renri removenow prints a YAML-style block (name / branch / path / vcs / head id + description / status flags / PR info) before doing anything, then asks for confirmation.-y/--yesskips the prompt;--non-interactivewithout--yesis a hard error so the safety prompt cannot be silently bypassed.--mergedbatch mode — sweeps every worktree whose GitHub PR isMERGEDorCLOSED. Each candidate gets the same detail panel and the whole batch is confirmed once. Dirty / conflict / locked / main rows are skipped with a warning (override with--force). Per-row failures are tallied at the end rather than aborting the batch.--refreshre-fetches the PR cache before deciding.-f/--forcepropagates to the backend'sremove(force=true), so single-target removes can finally drop uncommitted worktrees (previously alwaysforce=false).Test plan
cargo make check(fmt + clippy + 78 tests + lock check) passes locallyrenri remove --helpsurfaces-y,-f,--merged,--refreshrenri remove <name>shows the panel and confirmsrenri remove --mergedfilters correctly, skips dirty/locked, removes the restrenri remove --merged --non-interactiveerrors clearly without--yesSummary by CodeRabbit
Release Notes
--yes,--force,--merged, and--refreshflags to the remove command for enhanced flexibility.--mergedmode to remove all worktrees with merged or closed GitHub PRs in a single operation.