Menu ▾ ▴

#22 Close open repository before moving to Recycle Bin and improve locked-folder error message

closed
nobody
enhancement (85)
2026-07-05
2026-07-05
Anonymous
No

Originally created by: theBGuy

This change ensures users can successfully remove a repository from the app and Recycle Bin, even if it is the currently open one. The dialog now closes the open repo before attempting to move its folder, preventing failures due to locked files (especially on Windows). If a lock still occurs, the error message clearly explains what to do instead of giving a raw error.

Repository Removal Workflow

  • Updates RemoveRepoDialog in src/features/repository/RepoDialogs.tsx to close the open repository with closeRepo() before calling deleteRepoFolder when the "Also move to Recycle Bin" option is selected.
  • Ensures the repo is only removed from the recent list if the trash operation succeeds, so failures leave it visible for easy retry.

Filesystem Operations

  • Refactors delete_repo_folder in src-tauri/src/fsops.rs to attempt trashing the folder up to three times, waiting briefly between tries to give in-flight git subprocesses time to exit.
  • Replaces the raw error string with an actionable message if folder deletion fails, specifically naming editors, terminals, or file-explorer windows as common sources of lock, and appending the original error as context.

Documentation

  • Adds changelog.d/fixed-remove-open-repo-to-recycle-bin.md summarizing the improvement and clearer user-facing error behavior.

Related

Tickets: #142

Discussion

  • Anonymous

    Anonymous - 2026-07-05

    Originally posted by: theBGuy

    AI review (sonnet) · automated

    This change fixes a real bug (moving the currently-open repo to the Recycle Bin failing because git-status polling keeps spawning subprocesses) and adds a Rust retry loop to handle subprocess teardown lag. The approach is sound for the happy path, but has a genuine UX regression on failure and a cross-platform string bug.

    Correctness

    should-fix — RepoDialogs.tsx, handleRemove (the closeRepo call)

    closeRepo() is now called unconditionally before deleteRepoFolder, even if the trash operation is about to fail. When deleteRepoFolder throws (all three retries exhausted), the catch block fires, but the repo is already closed and the user has been navigated away — from a repo that still exists on disk. The original code left the user in their repo on failure; this version silently ejects them. The comment says "the repo stays listed so you can close external programs and retry," which is true, but omits that the user also has to reopen the repo manually.

    Concrete case: user has an uncommitted change visible, checks "Also move to Recycle Bin," confirms; an IDE holds a file handle; after ~900ms of retries the toast error appears, but the app is now at the no-repo screen instead of back in the repo.

    Fix: guard closeRepo() inside the if (moveToTrash) block so the non-trash path is unaffected, and accept the current eject-on-failure behaviour only for the trash path — or restore the open repo in the catch block (if (moveToTrash && repo.path === repoPath) openRepo(repo.path)).

    should-fix — src-tauri/src/fsops.rs, the error string

    The message hardcodes "Recycle Bin", which is Windows-only terminology. The trash crate is cross-platform; on macOS the destination is Trash and on Linux it is the freedesktop trash. Mac users who hit this error will read "Recycle Bin" and be confused.

    Fix: use a platform-branched message (#[cfg(target_os = "windows")] / #[cfg(target_os = "macos")] / else), or use the vaguer but accurate "the system trash".

    Readability

    nit — src-tauri/src/fsops.rs, last_err.unwrap_or_else(|| "unknown error".to_string())

    last_err is always Some at the point it is consumed — the loop body is the only way to reach the code after the for, and it always sets last_err = Some(e) on every iteration. The unwrap_or_else fallback is dead code and obscures the invariant. Use .expect("last_err set in loop") or restructure to avoid the Option entirely (e.g. let last_err: trash::Error with a sentinel / unwrap()).

     

  • Anonymous

    Anonymous - 2026-07-05

    Originally posted by: theBGuy

    AI security audit (sonnet) · automated

    Let me examine the full context of the changed functions before concluding.No security issues introduced by these changes.

    The delete_repo_folder guard (dir.join(".git").exists()) predates this diff and is unchanged. The {cause} interpolated into the error message is sourced exclusively from the trash crate's internal error — not from any attacker-controlled input — and is returned only to the local user who initiated the operation via a toast. The reordering in RepoDialogs.tsx (closeRepo() before deleteRepoFolder) is a pure UX sequencing change with no security-relevant trust-boundary crossing.

     

  • Anonymous

    Anonymous - 2026-07-05

    Originally posted by: theBGuy

    AI review (sonnet) · automated

    This PR fixes three distinct bugs: the currently-open-repo trash failure on Windows (lock contention), the GitHub "Delete branch" flag also deleting the user's local branch, and the local-PR merge leaving the user on base instead of their original branch. The approach for all three is sound and the previous review's concerns have been addressed.

    Resolved since last review

    • Correctness – silent eject on trash failure (RepoDialogs.tsx): The openRepo({ root: repo.path, name: repo.name }) call in the catch block correctly restores the repo when wasOpen && !trashed, covering every failure point before the folder is actually gone.
    • Correctness – "Recycle Bin" cross-platform string (fsops.rs): Replaced with a compile-time #[cfg(windows)] / #[cfg(not(windows))] split; the frontend mirrors this via isWindows ? "Recycle Bin" : "Trash".
    • Nit – dead unwrap_or_else (fsops.rs): Replaced cleanly with let mut cause = String::new() set unconditionally in the loop body.

    Correctness

    nit — RepoDialogs.tsx, catch block

    If trash::delete succeeds but removeRecent.mutateAsync then throws (admittedly rare — it's a local settings write), trashed is true so openRepo is skipped, leaving a stale recent-repo entry pointing at a deleted folder. Not a blocker, but worth noting in a comment so future maintainers don't "fix" the condition.

    Previous findings from other reviewers

    Copilot: partial move of head in gh_delete_remote_head_branch — not an issue. head.head_repository_owner and head.head_repository are consumed by .map(...) before the let-else, but head.is_cross_repository is bool (Copy) so Rust allows accessing it on a partially-moved struct, and branch is a separately-owned String constructed earlier via .trim().to_string(). This compiles correctly.

    Copilot: closeRepo() called even when moveToTrash is false — the current code is a deliberate design choice: removing a repo from GitDesktop closes it regardless of whether the folder is trashed. The catch block restores the open repo on any failure, so the UX regression from the previous version is gone. This is correct.

    Copilot: 404 "Not found" being swallowed in gh_delete_remote_head_branch — fixed in the current diff; only "reference does not exist" (GitHub's 422 response) is now suppressed.

    Overall the change is solid. The logic for all three fixes is correct, the cross-platform string handling is done at both layers (Rust compile-time, TS runtime), and the rollback path in the frontend is right.

     

  • Anonymous

    Anonymous - 2026-07-05

    Ticket changed by: theBGuy

    • status: open --> closed
     

Log in to post a comment.

MongoDB Logo MongoDB