Skip to content

worktree dirigent backup - #2

Merged
choas merged 17 commits into
mainfrom
worktree-dirigent-backup
Mar 22, 2026
Merged

worktree dirigent backup#2
choas merged 17 commits into
mainfrom
worktree-dirigent-backup

Conversation

@choas

@choas choas commented Mar 22, 2026

Copy link
Copy Markdown
Owner

Changes

  • 58b892d Dirigent: take a look at the @analyze_worktree_dirigent.md file and impleme...
  • 8c72e7c Dirigent: take a look at the @analyze_worktree_dirigent.md file and impleme...
  • 9f92d88 Dirigent: file tabs need a right click to close all, and other things
  • 3cbd2bd Dirigent: 5 cues committed
  • 81aef15 Dirigent: Highlight or add a dot at the git commits which aren't pushed in ...
  • 952a08e Dirigent: Commit All should include in the additional git commit text all t...

Summary by CodeRabbit

  • New Features

    • Right-click context menu for code tabs: Close, Close Others, Close All, Close Tabs to the Right
    • Archived worktree DB management: view archived DBs, Delete and Reveal actions, and a Delete Archived DB confirmation dialog
    • Floating force-remove confirmation dialog for worktree removal; worktree removal can archive its DB before deleting
  • Improvements

    • Bulk cue commit message formatting refined (better subjects and detailed bodies)
    • Git log highlights unpushed commits
    • Minor UI tweaks (tab interaction and input row clamping)

choas added 6 commits March 22, 2026 01:57
… all t...

Commit All should include in the additional git commit text all the Cue texts ... maybe we need also a better "main" commit message than <number> Cues committed
…ed in ...

Highlight or add a dot at the git commits which aren't pushed in the Git Log view also load all commits which aren't pushed e.g. 16 aren't pushed but only (default) 10 are shown
- Fix: clamp-like pattern without using clamp function
- Fix: this `if` statement can be collapsed
- Fix: this `if` statement can be collapsed
- Fix: this `if` statement can be collapsed
- Fix: this `map_or` can be simplified
…mpleme...

take a look at the @analyze_worktree_dirigent.md file and implement it, therefore, analyze the file and the project strucktur - plan the implementation, adjust the document if necessary, and then take your time to do it step by step
…mpleme...

take a look at the @analyze_worktree_dirigent.md file and implement it, therefore, analyze the file and the project strucktur - plan the implementation, adjust the document if necessary, and then take your time to do it step by step
@coderabbitai

coderabbitai Bot commented Mar 22, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@choas has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 18 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4e572381-27cb-48c7-80ac-f4fd394a55db

📥 Commits

Reviewing files that changed from the base of the PR and between bce1b6c and 7063721.

📒 Files selected for processing (1)
  • src/app/dialog/repo.rs
📝 Walkthrough

Walkthrough

Added tab-level right-click context menu and tab-management helpers; introduced archived worktree DB listing, archive/delete/reveal flows, and force-remove dialogs; extended git utilities for main-worktree discovery, DB archiving, and archived-DB listing; minor UI tweaks in diff rendering and log styling.

Changes

Cohort / File(s) Summary
Tab UI & state
src/app/code_viewer.rs, src/app/mod.rs
Added per-tab right-click context menu and interaction handling; capture tab frame response; added close_all_tabs, close_other_tabs, close_tabs_to_right helpers and early control-flow to apply context-menu actions.
Worktree dialogs & UI
src/app/dialog/repo.rs, src/app/mod.rs
Added archived worktree DB panel with Delete/Reveal controls, deferred remove flow via do_remove_worktree, pending-force/remove state, and two floating dialogs: force-remove and delete-archive.
Git worktree & archive API
src/git.rs
Extended remove_worktree(..., force: bool); added main_worktree_path, archive_worktree_db, ArchivedDb struct, and list_archived_dbs to support archiving and listing archived DBs.
Commit message formatting
src/app/cue_pool/mod.rs
Reworked CommitAll handling for Review cues: new subject/body formatting that uses trimmed first lines for subject and full trimmed cue texts for body.
Git log UI
src/app/panels.rs
Mark and style unpushed commits by comparing index to ahead_of_remote; adjusted hover text for unpushed entries.
Small UI tweaks
src/app/dialog/running_log.rs, src/app/code_viewer.rs
Replaced manual min/max with clamp for desired_rows; condensed small-button conditional in code viewer (no behavioral change).

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant UI as Dirigent UI
  participant App as DirigentApp
  participant Git as git module
  participant FS as Filesystem/OS

  User->>UI: Right-click tab / Click "Close Others" / "Close All" / "Close to Right"
  UI->>App: send context-menu selection (close_* action)
  App->>App: update CodeViewerState (close_*_tabs)
  App->>UI: re-render tabs (active_tab updated)
  User->>UI: Open Repo dialog -> Click "Remove" on worktree
  UI->>App: request remove_worktree(path, force=false)
  App->>Git: call git::get_dirty_files(path) (preflight)
  alt dirty files found
    Git->>App: returns dirty list
    App->>UI: set pending_force_remove + pending_archive_msg -> show force dialog
    User->>UI: click "Force Remove"
    UI->>App: request do_remove_worktree(path, force=true)
  end
  App->>Git: call git::main_worktree_path(repo) 
  Git->>FS: read worktree list
  Git->>App: return main_worktree_path
  App->>Git: call git::archive_worktree_db(main_path, wt_path, name)
  Git->>FS: copy .Dirigent/Dirigent.db -> archives/, possibly add timestamp
  Git->>App: return archived path (Option)
  App->>Git: call git::remove_worktree(repo, wt_path, force)
  Git->>FS: run `git worktree remove` (with --force if requested)
  Git->>App: return result
  alt success
    App->>App: reload_worktrees(), update archived_dbs
    App->>UI: show status (with archive info if present)
  else failure
    App->>UI: set error status
  end
  User->>UI: Click "Reveal" archived DB
  UI->>App: spawn OS-specific reveal command via FS (open/explorer/xdg-open)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • code understanding #1 — Implements tab-level right-click context menu and tab-close operations closely related to the tab-management and UI changes in this PR.

Poem

🐇
I hopped through tabs and hid a clue,
Archived DBs I labeled new,
I nudged a right-click, gave tabs a choice,
Patted removed worktrees with a gentle voice,
Now code and carrots both feel true.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'worktree dirigent backup' is partially related to the changeset. While archived worktree database backup/archiving is a significant feature, the PR contains many other substantial changes including tab context menu improvements, commit message formatting, unpushed commit highlighting, and cue text handling—none of which are conveyed by the title.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-dirigent-backup

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/app/cue_pool/mod.rs (1)

593-612: Inconsistent trimming between single and multi-cue subject construction.

Line 595 uses .trim() (removes leading and trailing whitespace) for the single-cue case, but line 605 uses .trim_end() (removes only trailing whitespace) for the multi-cue case. This could result in unexpected leading whitespace in multi-cue commit subjects.

Additionally, consider the edge case where a cue's text begins with blank lines — lines().next() would return an empty string, leading to subjects like "Dirigent: " or malformed combined subjects with empty entries.

Suggested fix for consistent trimming
                                         let short_names: Vec<&str> = review_cues
                                             .iter()
-                                            .map(|c| c.text.lines().next().unwrap_or(&c.text).trim_end())
+                                            .map(|c| c.text.lines().next().unwrap_or(&c.text).trim())
+                                            .filter(|s| !s.is_empty())
                                             .collect();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/app/cue_pool/mod.rs` around lines 593 - 612, The multi-cue subject
builder is using trim_end() while the single-cue path uses trim(), causing
inconsistent leading-whitespace handling and allowing empty-first-line entries;
update the multi-cue construction that builds short_names to mirror the
single-cue logic by selecting the first non-empty line (e.g., lines().find(|l|
!l.trim().is_empty()).unwrap_or_else(|| lines().next().unwrap_or(&c.text))) and
then applying .trim(), and skip any entries whose trimmed first line is empty so
combined (joined) subjects don't include blank tokens; keep the existing
fallback to the "Dirigent: {} cues" branch and reuse crate::app::truncate_str
where appropriate.
src/app/dialog/repo.rs (1)

254-262: Consider cross-platform support for the Reveal action.

The open -R command is macOS-specific. The #[cfg(target_os = "macos")] block handles this, but on other platforms the button will do nothing silently.

Consider either hiding the button on non-macOS or adding handlers for Windows (explorer /select,) and Linux (xdg-open on parent directory).

💡 Optional: Add Windows support
         if let Some(path) = reveal_path {
             #[cfg(target_os = "macos")]
             {
                 let _ = std::process::Command::new("open")
                     .arg("-R")
                     .arg(&path)
                     .spawn();
             }
+            #[cfg(target_os = "windows")]
+            {
+                let _ = std::process::Command::new("explorer")
+                    .arg("/select,")
+                    .arg(&path)
+                    .spawn();
+            }
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/app/dialog/repo.rs` around lines 254 - 262, The Reveal action currently
only runs macOS "open -R" inside the reveal_path block, so on non-macOS
platforms the button is a no-op; update the handler where reveal_path is used
(the std::process::Command spawn inside the reveal_path if-block in repo.rs) to
be cross-platform: on Windows run "explorer" with the "/select," argument and
the full path, on Linux detect the parent directory and run "xdg-open" (or the
distro-appropriate opener) against that directory, and keep the existing macOS
"open -R" branch; alternatively, if platform detection fails, hide/disable the
Reveal button on unsupported platforms. Ensure you use cfg(target_os = "...")
conditional branches around the respective Command invocations and preserve
error handling for spawn results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/app/dialog/repo.rs`:
- Around line 265-327: When force==true and this is a confirmation retry, avoid
re-archiving by reusing any previously stored archive message: in
do_remove_worktree check if force && self.git.pending_archive_msg.is_some() and
if so set archive_msg = self.git.pending_archive_msg.clone() instead of calling
git::archive_worktree_db again; otherwise perform the existing archival logic.
Keep the rest of the flow (storing pending_force_remove/pending_archive_msg on
failure and clearing them on success) unchanged so archive_msg is preserved
between the initial failure and the force-confirmation path.

In `@src/app/markdown_parser.rs`:
- Line 295: The code uses Option::is_none_or in the is_block_content computation
(see is_block_content and item_events), which requires Rust 1.82.0+; either
declare the MSRV by adding a rust-version entry in Cargo.toml (e.g.,
"rust-version = \"1.82\"") or replace the usage of Option::is_none_or with an
equivalent that works on older compilers (e.g., use match, map_or, or is_none()
|| item_events.first().map_or(false, |ev| ...)) so the crate builds for your
supported Rust version.

---

Nitpick comments:
In `@src/app/cue_pool/mod.rs`:
- Around line 593-612: The multi-cue subject builder is using trim_end() while
the single-cue path uses trim(), causing inconsistent leading-whitespace
handling and allowing empty-first-line entries; update the multi-cue
construction that builds short_names to mirror the single-cue logic by selecting
the first non-empty line (e.g., lines().find(|l|
!l.trim().is_empty()).unwrap_or_else(|| lines().next().unwrap_or(&c.text))) and
then applying .trim(), and skip any entries whose trimmed first line is empty so
combined (joined) subjects don't include blank tokens; keep the existing
fallback to the "Dirigent: {} cues" branch and reuse crate::app::truncate_str
where appropriate.

In `@src/app/dialog/repo.rs`:
- Around line 254-262: The Reveal action currently only runs macOS "open -R"
inside the reveal_path block, so on non-macOS platforms the button is a no-op;
update the handler where reveal_path is used (the std::process::Command spawn
inside the reveal_path if-block in repo.rs) to be cross-platform: on Windows run
"explorer" with the "/select," argument and the full path, on Linux detect the
parent directory and run "xdg-open" (or the distro-appropriate opener) against
that directory, and keep the existing macOS "open -R" branch; alternatively, if
platform detection fails, hide/disable the Reveal button on unsupported
platforms. Ensure you use cfg(target_os = "...") conditional branches around the
respective Command invocations and preserve error handling for spawn results.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fd353e83-3b35-4c86-8c70-8ca550901914

📥 Commits

Reviewing files that changed from the base of the PR and between 2b25daa and 58b892d.

📒 Files selected for processing (8)
  • src/app/code_viewer.rs
  • src/app/cue_pool/mod.rs
  • src/app/dialog/repo.rs
  • src/app/dialog/running_log.rs
  • src/app/markdown_parser.rs
  • src/app/mod.rs
  • src/app/panels.rs
  • src/git.rs

Comment thread src/app/dialog/repo.rs
Comment thread src/app/markdown_parser.rs Outdated
choas added 4 commits March 22, 2026 13:48
…veal a...

In `@src/app/dialog/repo.rs` around lines 254 - 262, The Reveal action currently
only runs macOS "open -R" inside the reveal_path block, so on non-macOS
platforms the button is a no-op; update the handler where reveal_path is used
(the std::process::Command spawn inside the reveal_path if-block in repo.rs) to
be cross-platform: on Windows run "explorer" with the "/select," argument and
the full path, on Linux detect the parent directory and run "xdg-open" (or the
distro-appropriate opener) against that directory, and keep the existing macOS
"open -R" branch; alternatively, if platform detection fails, hide/disable the
Reveal button on unsupported platforms. Ensure you use cfg(target_os = "...")
conditional branches around the respective Command invocations and preserve
error handling for spawn results.
…ulti-c...

In `@src/app/cue_pool/mod.rs` around lines 593 - 612, The multi-cue subject
builder is using trim_end() while the single-cue path uses trim(), causing
inconsistent leading-whitespace handling and allowing empty-first-line entries;
update the multi-cue construction that builds short_names to mirror the
single-cue logic by selecting the first non-empty line (e.g., lines().find(|l|
!l.trim().is_empty()).unwrap_or_else(|| lines().next().unwrap_or(&c.text))) and
then applying .trim(), and skip any entries whose trimmed first line is empty so
combined (joined) subjects don't include blank tokens; keep the existing
fallback to the "Dirigent: {} cues" branch and reuse crate::app::truncate_str
where appropriate.
… Optio...

In `@src/app/markdown_parser.rs` at line 295, The code uses Option::is_none_or in
the is_block_content computation (see is_block_content and item_events), which
requires Rust 1.82.0+; either declare the MSRV by adding a rust-version entry in
Cargo.toml (e.g., "rust-version = \"1.82\"") or replace the usage of
Option::is_none_or with an equivalent that works on older compilers (e.g., use
match, map_or, or is_none() || item_events.first().map_or(false, |ev| ...)) so
the crate builds for your supported Rust version.
…orce==...

In `@src/app/dialog/repo.rs` around lines 265 - 327, When force==true and this is
a confirmation retry, avoid re-archiving by reusing any previously stored
archive message: in do_remove_worktree check if force &&
self.git.pending_archive_msg.is_some() and if so set archive_msg =
self.git.pending_archive_msg.clone() instead of calling git::archive_worktree_db
again; otherwise perform the existing archival logic. Keep the rest of the flow
(storing pending_force_remove/pending_archive_msg on failure and clearing them
on success) unchanged so archive_msg is preserved between the initial failure
and the force-confirmation path.
@choas

choas commented Mar 22, 2026

Copy link
Copy Markdown
Owner Author

Fixed in commit 0e99af8.

Automated reply from Dirigent

@choas

choas commented Mar 22, 2026

Copy link
Copy Markdown
Owner Author

Fixed in commit 4ddf592.

Automated reply from Dirigent

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/app/cue_pool/mod.rs`:
- Around line 593-603: Normalize the single-cue subject branch: after you pick
first_line from review_cues[0] compute let trimmed = first_line.trim(); if
trimmed.is_empty() fall back to review_cues[0].text.trim() and if that is still
empty use a short neutral subject (e.g. "Dirigent" without a trailing colon)
instead of emitting "Dirigent: " with empty content; also change the length
logic so the 72-character target includes the "Dirigent: " prefix by doing let
prefix = "Dirigent: "; let allowed = 72 - prefix.len(); if trimmed.len() >
allowed then truncate trimmed to allowed - 3 (for the ellipsis) via
crate::app::truncate_str and append "..." before formatting with format!("{}{}",
prefix, truncated) otherwise format!("{}{}", prefix, trimmed).

In `@src/app/dialog/repo.rs`:
- Around line 291-327: The archive step currently swallows IO errors from
git::main_worktree_path and git::archive_worktree_db and proceeds to call
git::remove_worktree, which can delete a worktree before its DB is preserved;
update the logic in the surrounding method in repo.rs so that when
git::main_worktree_path or git::archive_worktree_db returns Err(_) you do not
continue to git::remove_worktree—either propagate the error or return early
(with a user-visible error/Result) so removal is aborted on archive failures;
keep the Ok(None) path (no DB to archive) unchanged, but ensure Err branches for
archive/main path stop execution before git::remove_worktree is invoked.
- Around line 338-345: Instead of parsing Git stderr for localized messages,
perform a preflight dirty-file check (using the existing get_dirty_files()
helper or running git status --porcelain) before attempting removal; if the
target path is present in the dirty list and force is false, set
self.git.pending_force_remove = Some((path, /* appropriate message */)) and
self.git.pending_archive_msg = archive_msg and skip the removal attempt,
otherwise proceed with normal removal; update the logic in the method containing
the current stderr-parsing block so it queries get_dirty_files() (or equivalent)
and uses that result to decide when to prompt for force-remove.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2101243d-e5dc-4403-ae07-b21ac19dd3db

📥 Commits

Reviewing files that changed from the base of the PR and between 58b892d and 95bb4b4.

📒 Files selected for processing (2)
  • src/app/cue_pool/mod.rs
  • src/app/dialog/repo.rs

Comment thread src/app/cue_pool/mod.rs
Comment thread src/app/dialog/repo.rs Outdated
Comment thread src/app/dialog/repo.rs Outdated
choas added 3 commits March 22, 2026 14:11
… again...

In `src/app/dialog/repo.rs` (line 345): Verify each finding against the current code and only fix it if needed.

In `@src/app/dialog/repo.rs` around lines 338 - 345, Instead of parsing Git stderr
for localized messages, perform a preflight dirty-file check (using the existing
get_dirty_files() helper or running git status --porcelain) before attempting
removal; if the target path is present in the dirty list and force is false, set
self.git.pending_force_remove = Some((path, /* appropriate message */)) and
self.git.pending_archive_msg = archive_msg and skip the removal attempt,
otherwise proceed with normal removal; update the logic in the method containing
the current stderr-parsing block so it queries get_dirty_files() (or equivalent)
and uses that result to decide when to prompt for force-remove.
…g agai...

In `src/app/cue_pool/mod.rs` (line 603): Verify each finding against the current code and only fix it if needed.

In `@src/app/cue_pool/mod.rs` around lines 593 - 603, Normalize the single-cue
subject branch: after you pick first_line from review_cues[0] compute let
trimmed = first_line.trim(); if trimmed.is_empty() fall back to
review_cues[0].text.trim() and if that is still empty use a short neutral
subject (e.g. "Dirigent" without a trailing colon) instead of emitting
"Dirigent: " with empty content; also change the length logic so the
72-character target includes the "Dirigent: " prefix by doing let prefix =
"Dirigent: "; let allowed = 72 - prefix.len(); if trimmed.len() > allowed then
truncate trimmed to allowed - 3 (for the ellipsis) via crate::app::truncate_str
and append "..." before formatting with format!("{}{}", prefix, truncated)
otherwise format!("{}{}", prefix, trimmed).
… again...

In `src/app/dialog/repo.rs` (line 327): Verify each finding against the current code and only fix it if needed.

In `@src/app/dialog/repo.rs` around lines 291 - 327, The archive step currently
swallows IO errors from git::main_worktree_path and git::archive_worktree_db and
proceeds to call git::remove_worktree, which can delete a worktree before its DB
is preserved; update the logic in the surrounding method in repo.rs so that when
git::main_worktree_path or git::archive_worktree_db returns Err(_) you do not
continue to git::remove_worktree—either propagate the error or return early
(with a user-visible error/Result) so removal is aborted on archive failures;
keep the Ok(None) path (no DB to archive) unchanged, but ensure Err branches for
archive/main path stop execution before git::remove_worktree is invoked.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/app/dialog/repo.rs`:
- Around line 205-206: The "Delete" button currently sets delete_archive =
Some(db.path.clone()) and then immediately calls std::fs::remove_file (also used
around the other block at lines 246-251); change this flow to present a
confirmation modal/dialog when ui.small_button("Delete").clicked() instead of
immediately deleting: set a pending deletion state (e.g., delete_archive =
Some(path)), render a modal that shows the path and asks the user to confirm or
cancel, and only call std::fs::remove_file when the user explicitly confirms;
canceling the modal should clear delete_archive and not touch the file.
Optionally implement a "Move to trash" or an undo buffer rather than hard-delete
if available.
- Around line 288-354: The preflight branch (the dirty-files path) is creating
an archive via git::archive_worktree_db before the user confirms, which leads to
duplicate timestamped archives when the dialog is reopened; move the call to
git::archive_worktree_db out of the preflight check and into the confirmed
remove path (the code that handles pending_force_remove confirmation), or
alternatively, if you must create an archive here, ensure you reload/surface the
created archive immediately and store its canonical path in
self.git.pending_archive_msg so Cancel/return does not leave a hidden archive;
update uses of git::archive_worktree_db, self.git.pending_force_remove,
self.git.pending_archive_msg, git::get_dirty_files and any set_status_message
early returns accordingly so no archive is created on the "dirty files"
preflight branch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 352b3a6e-da6a-4a9f-a629-8541b2202305

📥 Commits

Reviewing files that changed from the base of the PR and between 95bb4b4 and 5e98b40.

📒 Files selected for processing (2)
  • src/app/cue_pool/mod.rs
  • src/app/dialog/repo.rs
✅ Files skipped from review due to trivial changes (1)
  • src/app/cue_pool/mod.rs

Comment thread src/app/dialog/repo.rs Outdated
Comment thread src/app/dialog/repo.rs Outdated
choas added 2 commits March 22, 2026 14:38
… again...

In `src/app/dialog/repo.rs` (line 354): Verify each finding against the current code and only fix it if needed.

In `@src/app/dialog/repo.rs` around lines 288 - 354, The preflight branch (the
dirty-files path) is creating an archive via git::archive_worktree_db before the
user confirms, which leads to duplicate timestamped archives when the dialog is
reopened; move the call to git::archive_worktree_db out of the preflight check
and into the confirmed remove path (the code that handles pending_force_remove
confirmation), or alternatively, if you must create an archive here, ensure you
reload/surface the created archive immediately and store its canonical path in
self.git.pending_archive_msg so Cancel/return does not leave a hidden archive;
update uses of git::archive_worktree_db, self.git.pending_force_remove,
self.git.pending_archive_msg, git::get_dirty_files and any set_status_message
early returns accordingly so no archive is created on the "dirty files"
preflight branch.
… again...

In `src/app/dialog/repo.rs` (line 206): Verify each finding against the current code and only fix it if needed.

In `@src/app/dialog/repo.rs` around lines 205 - 206, The "Delete" button currently
sets delete_archive = Some(db.path.clone()) and then immediately calls
std::fs::remove_file (also used around the other block at lines 246-251); change
this flow to present a confirmation modal/dialog when
ui.small_button("Delete").clicked() instead of immediately deleting: set a
pending deletion state (e.g., delete_archive = Some(path)), render a modal that
shows the path and asks the user to confirm or cancel, and only call
std::fs::remove_file when the user explicitly confirms; canceling the modal
should clear delete_archive and not touch the file. Optionally implement a "Move
to trash" or an undo buffer rather than hard-delete if available.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/app/dialog/repo.rs (2)

419-426: Unnecessary clone of path at line 423.

path is already an owned PathBuf (cloned at line 375), so the second .clone() at line 423 is redundant.

Suggested fix
         if cancel {
             self.git.pending_force_remove = None;
             self.git.pending_archive_msg = None;
         } else if force {
-            let path = path.clone();
             self.git.pending_force_remove = None;
             self.do_remove_worktree(path, true);
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/app/dialog/repo.rs` around lines 419 - 426, The extra .clone() on path is
redundant because path is already an owned PathBuf (cloned earlier), so remove
the second clone and pass path directly to self.do_remove_worktree; update the
branch where force is true to call self.do_remove_worktree(path, true) instead
of cloning, keeping the existing assignments to self.git.pending_force_remove
and self.git.pending_archive_msg unchanged.

475-479: Include the error details in the failure message.

The actual I/O error is discarded, making debugging harder. Consider capturing and displaying the error.

Suggested fix
-            if std::fs::remove_file(&path).is_ok() {
-                self.reload_worktrees();
-            } else {
-                self.set_status_message("Failed to delete archived DB".to_string());
+            match std::fs::remove_file(&path) {
+                Ok(()) => self.reload_worktrees(),
+                Err(e) => self.set_status_message(format!("Failed to delete archived DB: {}", e)),
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/app/dialog/repo.rs` around lines 475 - 479, The failure branch currently
discards the I/O error when remove_file fails; change the
std::fs::remove_file(&path) call to capture the Result error (e.g., via match or
if let Err(e) = ...) and include the error details in the status message instead
of the generic text; update the branch that now calls
self.set_status_message("Failed to delete archived DB".to_string()) to use
something like self.set_status_message(format!("Failed to delete archived DB:
{}", e)) and keep the existing success path that calls self.reload_worktrees().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/app/dialog/repo.rs`:
- Around line 419-426: The extra .clone() on path is redundant because path is
already an owned PathBuf (cloned earlier), so remove the second clone and pass
path directly to self.do_remove_worktree; update the branch where force is true
to call self.do_remove_worktree(path, true) instead of cloning, keeping the
existing assignments to self.git.pending_force_remove and
self.git.pending_archive_msg unchanged.
- Around line 475-479: The failure branch currently discards the I/O error when
remove_file fails; change the std::fs::remove_file(&path) call to capture the
Result error (e.g., via match or if let Err(e) = ...) and include the error
details in the status message instead of the generic text; update the branch
that now calls self.set_status_message("Failed to delete archived
DB".to_string()) to use something like self.set_status_message(format!("Failed
to delete archived DB: {}", e)) and keep the existing success path that calls
self.reload_worktrees().

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: de2d325a-e89f-4bae-a79c-4a15d3c0ffdb

📥 Commits

Reviewing files that changed from the base of the PR and between 5e98b40 and bce1b6c.

📒 Files selected for processing (2)
  • src/app/dialog/repo.rs
  • src/app/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/mod.rs

choas added 2 commits March 22, 2026 15:01
…t if n...

Verify each finding against the current code and only fix it if needed.

In `@src/app/dialog/repo.rs` around lines 475 - 479, The failure branch currently
discards the I/O error when remove_file fails; change the
std::fs::remove_file(&path) call to capture the Result error (e.g., via match or
if let Err(e) = ...) and include the error details in the status message instead
of the generic text; update the branch that now calls
self.set_status_message("Failed to delete archived DB".to_string()) to use
something like self.set_status_message(format!("Failed to delete archived DB:
{}", e)) and keep the existing success path that calls self.reload_worktrees().
…t if n...

Verify each finding against the current code and only fix it if needed.

In `@src/app/dialog/repo.rs` around lines 419 - 426, The extra .clone() on path is
redundant because path is already an owned PathBuf (cloned earlier), so remove
the second clone and pass path directly to self.do_remove_worktree; update the
branch where force is true to call self.do_remove_worktree(path, true) instead
of cloning, keeping the existing assignments to self.git.pending_force_remove
and self.git.pending_archive_msg unchanged.
@choas

choas commented Mar 22, 2026

Copy link
Copy Markdown
Owner Author

Fixed in commit 7063721.

Automated reply from Dirigent

@choas

choas commented Mar 22, 2026

Copy link
Copy Markdown
Owner Author

Fixed in commit latest commit.

Automated reply from Dirigent

@choas
choas merged commit deefb85 into main Mar 22, 2026
1 check passed
@choas
choas deleted the worktree-dirigent-backup branch March 22, 2026 14:47
@coderabbitai coderabbitai Bot mentioned this pull request Mar 24, 2026
choas added a commit that referenced this pull request Mar 30, 2026
[PR Import] Fetched 5 fin...

terminal logs these ... not necessary:

[PR Import] Fetched 5 findings from PR #2
[PR Filter] Import triggered (5 included)
[PR Filter] import_filtered_pr_findings: 5 pending, 0 excluded
[PR Filter] After filtering: 5 findings to import
[PR Import] handle_pr_findings called with 5 findings, pr_number='2'
[PR Import] upsert results: new=5, updated=0, errors=0
[PR Import] reload_cues done, total cues=5
[PR Import] Fetched 5 findings from PR #2
[PR Import] Fetched 5 findings from PR #2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant