Skip to content

fix(ui,git,repository): offer backup stash cleanup after conflicts - #558

Merged
theBGuy merged 4 commits into
masterfrom
fix/conflict-banner-stash-reapply
Oct 16, 2026
Merged

theBGuy merged 4 commits into
masterfrom
fix/conflict-banner-stash-reapply

Conversation

@theBGuy

@theBGuy theBGuy commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

When reapplying stashed changes leaves conflicts, the Changes banner now explains which operation completed and keeps the backup stash available until resolution. Once conflicts are resolved, users can safely drop the backup or keep it, with SHA verification protecting against changes to the stash list.

Conflict recovery UI

  • Adds stash-reapply state derivation in src/features/repository/conflict-banner-state.ts, including operation-specific messaging, freshness checks, conflict counts, and resolved-state handling.
  • Updates src/features/repository/ConflictBanner.tsx to identify stash-reapply conflicts and provide Drop backup stash… and Keep it actions after resolution.
  • Adds confirmation, stale/moved stash handling, recovery toasts, command-palette actions, and focus rescue behavior in src/features/repository/ConflictBanner.tsx and src/features/repository/ChangesPanel.tsx.
  • Records the affected repository, operation context, and backup stash SHA in src/lib/stores/stash-reapply-marker.ts.
  • Passes operation details from src/features/repository/BranchSwitcher.tsx and src/features/repository/useStashReapplyRecovery.tsx so the banner can name the completed operation.
  • Registers drop-backup-stash and keep-backup-stash in src/lib/hotkeys/registry.ts.

Git stash safety

  • Captures the retained backup stash object ID in src-tauri/src/git/autostash.rs when a reapply conflict occurs and propagates it through src-tauri/src/git/pull_guard.rs.
  • Adds the git_drop_stash_by_sha command in src-tauri/src/git/autostash.rs and registers it in src-tauri/src/lib.rs.
  • Verifies that the recorded SHA is still the stash tip before dropping it, reports moved or missing entries, and attempts to restore a different entry if an external race is detected.
  • Exposes StashDropOutcome and the SHA-bearing AutostashOutcome through src/lib/git/api/stashes.ts and src/lib/git/api/sync.ts.
  • Adds the local mutation query in src/lib/git/queries/stashes.ts and includes it in scripts/networkmode-local-mutations.test.mjs.

Documentation and tests

  • Documents the conflict banner and backup-stash choices in README.md and src/features/help/content.ts.
  • Adds the release fragment changelog.d/changed-conflict-banner-stash-reapply.md.
  • Adds state-machine coverage for freshness gates, operation precedence, conflict messaging, and marker cleanup in scripts/conflict-banner-state.test.mjs.
  • Adds Rust coverage in src-tauri/src/git/autostash.rs for SHA capture, exact-entry drops, moved or missing backups, malformed IDs, output parsing, and serialized outcomes.
  • Extends src-tauri/src/git/pull_guard.rs tests to ensure the recorded backup SHA survives submodule failure handling.

Summary by CodeRabbit

  • New Features
    • When stash reapplication causes conflicts, the Changes banner identifies the operation and lists affected files.
    • After resolving conflicts, choose to keep the backup stash or drop it after confirmation. The banner and command palette offer these actions.
    • The backup stash is checked before removal; if it has changed or is missing, the app reports the outcome.
  • Documentation
    • Updated guidance explains conflict handling, backup stash choices, and when the banner is available.

- Add conflict-banner state tracking that identifies stash reapply conflicts, names the completed operation, and offers accessible drop or keep actions with focus recovery and hotkeys.
- Add SHA-verified stash backup removal with race detection, restoration safeguards, Tauri wiring, and query/API support.
- Preserve stash metadata across autostash outcomes and surface recovery behavior in the README and in-app guide.
- Add state, parser, serialization, network-mode, and Git integration tests with a changelog fragment.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Deploying gitdesktop with  Cloudflare Pages  Cloudflare Pages

Latest commit: df882e4
Status: ✅  Deploy successful!
Preview URL: https://83cf6342.gitdesktop.pages.dev
Branch Preview URL: https://fix-conflict-banner-stash-re.gitdesktop.pages.dev

View logs

@theBGuy theBGuy added the enhancement New feature or request label Oct 16, 2026
@theBGuy theBGuy self-assigned this Oct 16, 2026
Distinguish missing stash refs from failed reads so stash drops do not
report Gone when Git cannot inspect the ref. Preserve a conflicted-pop
backup only when the stash tip remains unchanged across the pop, avoiding
reapplication of an unrelated stash entry. Add coverage for ref-read
classification and unchanged-tip validation.
@theBGuy

theBGuy commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

Context for reviewers

  1. What this does. Update from master, pull, merge, rebase and branch switch with reapply can stash your changes, run the operation and pop the stash back. When that pop conflicts, git leaves unmerged files with no operation in progress, so the Changes banner used to show only "1 conflict — resolve it in the changes list.". It never said the conflict came from your own stashed changes, and the kept backup stash surfaced only in an 8-second toast.

    • The banner now names the cause: "Reapplying your stashed changes · 1 conflict", plus which operation finished.
    • Once every conflict is resolved, it offers Drop backup stash… (confirm first) or Keep it.
  2. The drop is SHA-verified.

    • The compound reads refs/stash right before the pop and again after a conflicted one, and records the backup's object id (AutostashOutcome::ReapplyConflicted.stashSha) only when both reads agree. Git can't pop by object id, so a stash pushed into that gap by another worktree or process would be the entry the pop reapplied and kept. A failed read records nothing, and the banner falls back to today's copy.
    • The probe tells a confirmed-absent ref (rev-parse --verify --quiet exit 1, no output) from a failed read. A failed read surfaces as an error and keeps the offer for a retry; it never reads as "already gone".
    • The new git_drop_stash_by_sha holds the repo lock, refuses unless refs/stash still equals that id ("moved" or "gone"), then drops stash@{0} and parses git's Dropped stash@{0} (<id>) confirmation.
    • Git can only drop by slot. If another writer slipped a stash in between, the dropped id won't match: the entry is put back with git stash store and the result is "moved". If that restore fails, the result names the dropped id (restoreFailed) and points to the Recoverable view. "Nothing was dropped" is never reported then.
    • The lock covers this process's writers on this checkout. The post-drop check covers the MCP server, sibling worktrees and external git.
  3. Every marker decision waits for fresh data. The cached status and stash-count reads are from before the compound when the outcome arrives, so a stale 0 would flash a Drop offer or clear the marker instantly. The conflicted copy, the resolved row and the clear-when-no-stashes rule act only on reads whose dataUpdatedAt is after the marker was recorded.

  4. pull_guard.rs carries the id through its fold. fold_submodule_failure destructures and rebuilds ReapplyConflicted exhaustively. It now binds and carries stash_sha; dropping it would silently remove the marker on the guarded rebase-pull paths. A test pins that the fold preserves it.

  5. The marker is in-memory, on purpose. It's keyed to the open repository and clears on a repo switch or restart. After that, the banner shows today's generic copy and the stash stays in View stashes. The help says so.

  6. Measured live (a throwaway fixture repo; the dirty-tree fast-forward whose pop conflicts):

    • All 10 scenarios pass: the prompt path, autoStashOnPull, a repo switch clearing the marker, the branch-switch path, Drop + Cancel, Drop + Confirm, Keep it on a clean tree, and the moved and gone refusals.
    • With an older unrelated stash underneath, only the recorded backup was dropped and the older one survived.
    • Copy is exact: 32 of 32 strings, character by character.
    • Focus: after Cancel it returns to Drop. After a drop, a refusal or Keep it, it lands on the tree-view toggle, or on a silent landing spot when the tree is clean, never on <body>.
  7. Keyboard and palette. Drop and Keep are two plain tab stops. drop-backup-stash and keep-backup-stash are palette-only actions, enabled only in the resolved state. A pre-mounted role="status" region announces the switch to the resolved state.

  8. Disclosures.

    • The restoreFailed arm can't be staged live, because it needs a race. Its parser is unit-tested against both spellings git prints, and a real-repo test pins the confirmation's stream and format. Its toast copy goes beyond the original design: "The stash list changed — a different stash () was dropped and couldn't be restored. Recover it under View stashes → Recoverable.". It stays up until dismissed.
    • View stashes' own Drop and the MCP drop_stash tool still target a stash@{n} slot, checked by message and date. Moving them onto the SHA-verified drop is tracked as a follow-up.
    • scripts/networkmode-local-mutations.test.mjs gains the new mutation in its LOCAL vocabulary; the guard requires every mutation to be classified.
    • Locally, cargo clippy --all-targets flags 6 lints in files this PR doesn't touch, a toolchain skew. Plain clippy -D warnings is clean, and the full suite passes (2825 tests).
  9. Docs.

    • The in-app guide is updated in three places: Stash and reapply, the branch-switch bullet, and Resolving conflicts. The last is scoped to Stash and reapply, since a manual Pop or Apply records no marker.
    • The README's stash-and-reapply paragraph.
    • One changed- changelog fragment.

Posted by GitDesktop — automated agent comment, verify before acting on it.

@theBGuy
theBGuy marked this pull request as ready for review October 16, 2026 15:10
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

📝 Walkthrough

Conflicted stash reapplications now retain a verified backup-stash identifier and appear in the Changes banner. After conflicts are resolved, the banner offers users the choice to keep the backup or confirm its SHA-checked removal.

Changes: Stash reapply recovery

Layer / File(s) Summary
Capture and record the backup stash identity
src-tauri/src/git/autostash.rs, src-tauri/src/git/pull_guard.rs, src/lib/git/api/sync.ts, src/features/repository/useStashReapplyRecovery.tsx, src/lib/stores/stash-reapply-marker.ts, src/features/repository/BranchSwitcher.tsx, src/features/repository/ChangesPanel.tsx, src/features/repository/StashesDialog.tsx, src/lib/stores/worktree-removal.ts
A conflicted reapply outcome carries the retained stash SHA when the stash tip remains unchanged across the pop. Recovery records that SHA with operation details for the matching repository. Other stash operations clear the marker.
Derive and display stash-reapply banner states
src/features/repository/conflict-banner-state.ts, scripts/conflict-banner-state.test.mjs, src/features/repository/ChangesPanel.tsx, src/features/repository/ConflictBanner.tsx, README.md, src/features/help/content.ts, changelog.d/changed-conflict-banner-stash-reapply.md
The banner derives conflicted or resolved states from repository, operation, conflict, status, and stash-count data. It displays state-specific copy and actions. Tests and documentation describe the state rules and available choices.
Drop a backup by verified SHA
src-tauri/src/git/autostash.rs, src-tauri/src/lib.rs, src/lib/git/api/stashes.ts, src/lib/git/queries/stashes.ts, src/lib/hotkeys/registry.ts, scripts/networkmode-local-mutations.test.mjs, src/features/repository/ConflictBanner.tsx
The drop command validates the object ID and checks it against the stash tip before dropping. The banner confirms removal and handles the returned outcomes. The API, mutation, and command-palette actions connect the banner to the command.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Recovery as useStashReapplyRecovery
  participant Marker as useStashReapplyMarker
  participant Banner as ConflictBanner
  participant Mutation as useDropStashBySha
  participant Command as git_drop_stash_by_sha
  Recovery->>Marker: record the conflicted reapply SHA and operation details
  Banner->>Mutation: submit the confirmed backup SHA
  Mutation->>Command: invoke the SHA-verified drop
  Command->>Command: compare the SHA with refs/stash and drop the matching tip
  Command-->>Mutation: return Dropped, Moved, Gone, or RestoreFailed
  Mutation-->>Banner: return the drop outcome
Loading
Merge Risk: 🔵 Low · up to df882

A narrow timing race can remove or hide the backup-stash cleanup choice. The stash remains accessible through View stashes, but the banner’s recovery choice should be made reliable before merging if practical.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 71.19% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 59 functions across 18 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: offering backup stash cleanup after stash-reapply conflicts.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@theBGuy

theBGuy commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

🤖 GitDesktop AI review · opus · automated


The PR names the cause of a conflicted autostash reapply in the Changes banner, and after resolution offers to drop or keep the backup stash. The drop is verified against the backup's SHA. The Rust side is solid: the freshness gate and the before/after-pop tip check are tight, and both are tested. Nothing blocks the merge. The main gap is that the marker can outlive the reapply it describes.

Correctness

  • should-fix: The marker outlives the reapply it describes. Files: src/features/repository/useStashReapplyRecovery.tsx reportAutostashOutcome, and src/lib/stores/stash-reapply-marker.ts, which clears only on a repo switch.
    • The marker stays in the resolved state until the user picks Drop or Keep, and ignoring it is easy. Nothing else in the same repo replaces or clears it.
    • Trigger (a): a later compound lands an outcome that records nothing. Examples: a switch with reapply off (stashedOnly), opFailedStashKept, or a reapplyConflicted whose stashSha came back null. The new entry now sits on stash@{0}, but the banner still says "The update from master finished." Its Drop targets the old SHA, now at stash@{1}, so every Drop answers moved ("The stash list changed — nothing was dropped").
    • Trigger (b): with the marker unanswered, the user runs Pop latest stash (BranchSwitcher.tsx doStashPop) or a View stashes pop/apply (StashesDialog.tsx via useStashApply), and it conflicts. The banner then shows "Reapplying your stashed changes · 1 conflict" plus "The update from master finished…" for a conflict that came from the manual pop.
    • Fix:
      • Add clearFor(repoPath) to the store; it clears only when marker?.repoPath === repoPath.
      • In reportAutostashOutcome, call it in the stashedOnly and opFailedStashKept arms, and in the reapplyConflicted arm whenever the record condition fails.
      • Call it before the mutation in doStashPop and in StashesDialog's apply/pop handler.
      • Update the guide sentence in Stash and reapply (content.ts), "The offer lasts until you switch repositories or restart", so it names the new endings (e.g. "…or until another stash or reapply replaces it").

Tests

  • should-fix: In src-tauri/src/git/autostash.rs git_drop_stash_by_sha_core, the Some(printed) if printed != sha branch has no test. That branch looks up the entry's message in listing and runs git stash store -m <msg> <printed>, and it's the only code that rewrites a stash the user didn't ask to touch.
    • Your note that the restoreFailed arm can't be staged live is recorded. It doesn't need a race at the unit level, though.
    • Fix: move the branch into a helper, e.g. async fn restore_dropped(repo: &str, listing: &str, printed: String) -> StashDropOutcome, and call it from the match. Then add two real-repo tests:
      • Restore succeeds: push_stash A, then X. Capture stash list --format=%H%x09%gs, drop stash@{0} with run_git, then call the helper with X's SHA. Assert Moved, assert stash_shas == [X, A], and assert stash@{0}'s %gs equals the captured subject.
      • Restore fails: pass a well-formed id with no object behind it, such as "f".repeat(40). Don't use the all-zero id: stash store treats it as a delete. Assert RestoreFailed { dropped_sha }.

Readability

  • nit: The docs on stashSha say it's null only "when that read failed". This affects the stash_sha field doc in autostash.rs and the stashSha doc in src/lib/git/api/sync.ts. It's also None/null whenever conflicted is false, when the tip moved across the pop, or when the post-pop read failed. Reword both to something like "null unless the pop left conflicts and refs/stash read the same id before and after it".
  • nit: ChangesPanel.tsx derives the banner state a second time (opState, reapplyMarker, markerRowsShown) only to feed useFocusOnControlsSwap.
    • ConflictBanner is always mounted once status has loaded, and its marker rows need fresh status, so it already has both markerRows and landingRef. Call useFocusOnControlsSwap(markerRows, landingRef) there instead.
    • In ChangesPanel.tsx, drop those three locals and the now-unused useOpState, useStashReapplyMarker, activeRepoOp/deriveStashBannerState and useFocusOnControlsSwap imports.
    • Reword the comments that point at ChangesPanel:
      • the doc on activeRepoOp ("shared so ChangesPanel's marker-row focus rescue…");
      • the test-file header ("the op precedence shared with ChangesPanel's focus rescue");
      • the trailing comment in doDrop ("which ChangesPanel's rescue catches").
  • nit: conflict-banner-state.ts opens with "Import-free on purpose" and then holds two imports. Say "Runtime-import-free on purpose", which is how the test file words it.

Posted by GitDesktop — AI output, verify before acting on it.

@theBGuy

theBGuy commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

🤖 GitDesktop AI security audit · opus · automated


No security issues found in this diff. The new git_drop_stash_by_sha command only lets a full lowercase hex object ID reach git. The restore message is passed as its own argument after -m, and git is spawned directly rather than through a shell. repo_path gets the same handling as the existing git_stash_drop command, and all UI strings render through escaped JSX or sonner text.


Posted by GitDesktop — AI output, verify before acting on it.

@theBGuy

theBGuy commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

Round 1 dispositions. Reviews: the app review, the security audit (clean) and CodeRabbit (no actionable comments).

  1. The marker outlives the reapply (should-fix): fixed.
    • The marker store gains clearFor(repoPath). reportAutostashOutcome clears it on every outcome that records no marker: stashedOnly, opFailedStashKept, and a reapplyConflicted that can't record.
    • It also clears before every other stash operation in the repo: a manual Pop, View stashes Apply and Pop, the Recoverable view's Restore, Stash all from both menus, Stash selected, and the Stash & promote push.
    • Measured live: Stash all, Apply and Pop each remove the offer, and a manual pop that conflicts now gets the generic banner copy.
    • The help sentence now reads: "…until you switch repositories, restart, or start another stash or reapply; …".
  2. The restore branch is untested (should-fix): fixed.
    • The printed != sha arm is now restore_dropped(repo, listing, printed), with two real-repo tests.
    • One stores a raced entry back with its original subject: Moved, order [X, A].
    • The other passes an id with no object behind it: RestoreFailed with that id, and the list is untouched.
    • Measured first: git stash store -m keeps the subject verbatim, and a non-stash id exits 128.
  3. The stashSha null semantics (nit): fixed in the Rust and TS docs: null unless the pop left conflicts and refs/stash read the same id before and after it.
  4. The duplicate derivation (nit): fixed. The focus swap now lives in ConflictBanner, and ChangesPanel only renders the landing spot. Focus was re-verified live on Drop → Cancel, Drop → Confirm, Keep it, and a moved refusal.
  5. "Import-free" wording (nit): fixed to "Runtime-import-free".

All of it lands in the next push. The full suite passes: 2827 tests, plain clippy clean, and pnpm run checks 1834/0.


Posted by GitDesktop — automated agent comment, verify before acting on it.

Retire stash reapply markers when later stash or reapply operations make them invalid, and move focus recovery into the conflict banner so controls remain usable after the offer disappears.

- Restore raced stash entries after verified drops and report failures explicitly
- Clear stale markers across branch switching, stash dialogs, worktree promotion, and autostash outcomes
- Update conflict recovery guidance and tests for the revised marker lifecycle
@theBGuy

theBGuy commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

🤖 GitDesktop AI review · opus · automated


The fixes from the last round have all landed. The marker now retires on every stash push, apply, pop and restore, and restore_dropped is tested both ways. What's left is two nits on copy those fixes made inaccurate, plus a duplicated doc lead. Nothing blocks the merge.

Correctness

  • nit: In src/features/repository/ConflictBanner.tsx doDrop, the post-confirm stale check (lines 326–328) can now say "already gone" while the backup is still in the stash list.
    • Why it's new: that branch reads a null marker as "gone". Before this push, a null marker in the same repo meant the fresh-empty-stash-list clear had run, so "gone" was right. Now clearFor also nulls it while the backup still exists.
    • Trigger: a Stash & promote is running (runPromote's removal leg "can run for minutes"), and the user opens Drop's confirm. The promote reaches clearFor(activeKey) and gitStashAll, then the user confirms. The toast says "The backup stash is already gone…" even though the backup is at stash@{1}.
    • The same happens when a compound already in flight settles to stashedOnly, opFailedStashKept, or a sha-less reapplyConflicted while the confirm is open.
    • Fix: replace the if (live?.repoPath === repoPath) toastStashMoved(); else toastStashGone(); pair with a plain toastStashMoved();. "The stash list changed — nothing was dropped." holds for both causes, and the repo-switch case has already returned above. toastStashGone stays in use by the gone outcome arm, so nothing else changes.

Documentation

  • nit: In src/features/help/content.ts, Stash and reapply (around line 958), the new sentence "The offer lasts until you switch repositories, restart, or start another stash or reapply" claims more than the code does.
    • A stash and reapply that ends in reapplied or opFailedRestored correctly keeps the offer (the backup is back on top), because those arms of reportAutostashOutcome don't call clearFor.
    • Fix: reword to match what actually ends it, e.g. "The offer lasts until you switch repositories or restart, until a newer stash is left on top of the backup, or until you apply a stash yourself; the stash itself stays in View stashes either way."

Readability

  • nit: In src/features/repository/conflict-banner-state.ts, the docs on OP_BY_FLAG and activeRepoOp both open with "Which op the banner names". Start the activeRepoOp doc at "Kept beside the deriver…", or drop the phrase from OP_BY_FLAG's doc.

Resolved since last review

  • Marker outlives the reapply: clearFor(repoPath) is added to the store and called in:

    • reportAutostashOutcome's stashedOnly and opFailedStashKept arms, and the reapplyConflicted arm when it records nothing;
    • BranchSwitcher stash-all and pop;
    • ChangesPanel.confirmStash;
    • StashesDialog runApply and restoreSha;
    • runPromote, where activeKey is info.root, the ui-store spelling the marker records.

    The guide sentence is updated (see the nit above).

  • Untested restore branch: it's extracted into restore_dropped, with real-repo tests for both the restore and its failure.

  • stashSha null semantics: the docs are corrected in both autostash.rs and sync.ts.

  • Duplicate derivation in ChangesPanel: the focus swap now lives in ConflictBanner. The unused imports are gone, and the comments on activeRepoOp, the test header and doDrop are reworded.

  • "Import-free" wording: now reads "Runtime-import-free".

Verdict: no blocking issues — remaining items are non-blocking; merge when ready


Posted by GitDesktop — AI output, verify before acting on it.

@theBGuy

theBGuy commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

Round 2 dispositions. The app review raised 3 nits; CodeRabbit was rate-limited on this head.

  1. A refusal after confirming could say "already gone" (nit): fixed. A cleared marker no longer means the backup is gone: a newer stash, or a compound settling while the confirm is open, retires it too. The post-confirm check now always shows "The stash list changed — nothing was dropped." The repo-switch exit above it is unchanged.
  2. The help sentence over-claimed (nit): fixed. A stash and reapply that ends with the backup back on top correctly keeps the offer, so the sentence now names exactly what ends it: "The offer lasts until you switch repositories or restart, until a newer stash is left on top of the backup, or until you apply a stash yourself; the stash itself stays in View stashes either way." Each clause was checked against reportAutostashOutcome's arms and the clearFor sites.
  3. Duplicated doc lead (nit): fixed. The activeRepoOp doc now starts at "Kept beside the deriver…".

These land in the next push, as copy plus one toast branch.


Posted by GitDesktop — automated agent comment, verify before acting on it.

Show the stash-moved notification whenever a recovery marker changes, since a
cleared marker may indicate a newer stash or settling operation rather than a
deleted backup. Update the in-app guide to describe the revised recovery offer
lifetime.
@theBGuy

theBGuy commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

🤖 GitDesktop AI review · opus · automated


Re-review: all three nits from the last round are fixed in this push, and the fixes didn't break anything nearby. Nothing blocks the merge.

Resolved since last review

  • doDrop post-confirm stale toast (ConflictBanner.tsx): the "already gone" branch is gone, so this check now always shows toastStashMoved(). The new comment gives the reason in one sentence. toastStashGone is still used by the gone outcome arm, so nothing is left unused. The repo-switch early return above it and the focus placement after dialogClosed() are unchanged.
  • Guide over-claimed when the offer ends (content.ts, Stash and reapply): I checked the new wording against every clearFor call and it holds:
    • "until a newer stash is left on top": stashedOnly, opFailedStashKept, a reapplyConflicted with no sha, Stash all in BranchSwitcher, ChangesPanel.confirmStash (Stash all and Stash selected), and the Stash & promote push. That push really does leave the stash on top; its toast says "Pop latest stash brings them back".
    • "until you apply a stash yourself": Pop latest stash in BranchSwitcher, plus StashesDialog runApply and restoreSha.
    • The reapplied and opFailedRestored arms correctly don't clear it.
    • I found no other stash push, apply or pop call site in src/ that skips the clear.
  • Duplicated doc lead (conflict-banner-state.ts): the activeRepoOp doc now opens with "Kept beside the deriver…", and only OP_BY_FLAG keeps "Which op the banner names".

I have nothing further to raise.

Verdict: no blocking issues — remaining items are non-blocking; merge when ready


Posted by GitDesktop — AI output, verify before acting on it.

@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.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟡 Minor · Cancel the pre-marker stash-count read before recording the… · conflict-banner-state.ts:70-99

src/features/repository/conflict-banner-state.ts:70-99
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Cancel the pre-marker stash-count read before recording the marker.

dataUpdatedAt reflects when a query result is published. A stash-count read that started before the marker can therefore publish a stale zero after the marker and satisfy countFresh, which clears the valid marker. If that read finishes before recording, its zero remains stale and the resolved row stays hidden because no post-marker count is fetched.

Cancel the count query immediately before recording the marker, then refetch it immediately after recording. A post-marker zero still clears the marker, while a post-marker nonzero count enables the resolved offer.

Suggested fix
diff --git a/src/features/repository/useStashReapplyRecovery.tsx b/src/features/repository/useStashReapplyRecovery.tsx
--- a/src/features/repository/useStashReapplyRecovery.tsx
+++ b/src/features/repository/useStashReapplyRecovery.tsx
@@
 import { useState } from "react";
+import { useQueryClient } from "@tanstack/react-query";
 import { toast } from "sonner";
@@
   useRebaseAutostash,
   useRebaseOntoAutostash,
+  stashCountOptions,
 } from "@/lib/git/queries";
@@
 export function useStashReapplyRecovery(repoPath: string) {
+  const queryClient = useQueryClient();
   const settings = useSettings();
@@
     };
     try {
-      reportAutostashOutcome(await runCompound(req.run), copy, {
+      const outcome = await runCompound(req.run);
+      const recordsBackupMarker =
+        outcome.kind === "reapplyConflicted" &&
+        outcome.conflicted &&
+        outcome.stashSha !== null &&
+        useUiStore.getState().repoPath === repoPath;
+      if (recordsBackupMarker) {
+        await queryClient.cancelQueries({
+          queryKey: stashCountOptions(repoPath).queryKey,
+        });
+      }
+      reportAutostashOutcome(outcome, copy, {
         repoPath,
         operationLabel: req.operationLabel,
         detail: req.detail,
         detailPreposition: req.detailPreposition,
       });
+      if (recordsBackupMarker) {
+        await queryClient.refetchQueries({
+          queryKey: stashCountOptions(repoPath).queryKey,
+        });
+      }
     } catch (e) {

ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8fe1a0b7-65af-435b-9e03-85f10f41fdfa
📥 Commits

Reviewing files that changed from the base of the PR and between 51db99c and df882e4.

📒 Files selected for processing: 12
  • scripts/conflict-banner-state.test.mjs
  • src-tauri/src/git/autostash.rs
  • src/features/help/content.ts
  • src/features/repository/BranchSwitcher.tsx
  • src/features/repository/ChangesPanel.tsx
  • src/features/repository/ConflictBanner.tsx
  • src/features/repository/StashesDialog.tsx
  • src/features/repository/conflict-banner-state.ts
  • src/features/repository/useStashReapplyRecovery.tsx
  • src/lib/git/api/sync.ts
  • src/lib/stores/stash-reapply-marker.ts
  • src/lib/stores/worktree-removal.ts
🚧 Files skipped from review as they are similar to previous changes: 3
  • scripts/conflict-banner-state.test.mjs
  • src/lib/git/api/sync.ts
  • src/features/repository/conflict-banner-state.ts

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

📜 Review details
⏰ Context from checks skipped due to timeout: 2
  • GitHub Check: build
  • GitHub Check: Cloudflare Pages
🧰 Additional context used
📚 Code guidelines: 3
.claude/rules/rust-tauri.md — unknown
.claude/rules/frontend.md — unknown
CLAUDE.md — unknown
📓 Path-based instructions: 3
Source excerpt: Never repo-wide `cargo fmt`; new files only via `rustfmt `.

📄 CodeRabbit inference engine (.claude/rules/rust-tauri.md)

Files:

  • src-tauri/src/git/autostash.rs
Source excerpt: Every new selectable list gets arrow-key navigation in the same change (`listKeyboardNav`, `src/lib/list-keyboard-nav.ts`) — invariant, not polish.

📄 CodeRabbit inference engine (.claude/rules/frontend.md)

Files:

  • src/lib/stores/worktree-removal.ts
  • src/features/repository/StashesDialog.tsx
  • src/features/help/content.ts
  • src/features/repository/BranchSwitcher.tsx
  • src/lib/stores/stash-reapply-marker.ts
  • src/features/repository/ChangesPanel.tsx
  • src/features/repository/useStashReapplyRecovery.tsx
  • src/features/repository/ConflictBanner.tsx
Applicability reference: AGENTS.md: # AGENTS.md — GitDesktop GitDesktop is an AI-native, keyboard-first Git desktop client (Tauri 2 + React 19) with an Astro marketing site in `site/`.

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

  • src/features/help/content.ts
🔇 Additional comments: 9
src-tauri/src/git/autostash.rs: 1

587-587: LGTM!

Also applies to: 593-617

src/features/repository/useStashReapplyRecovery.tsx: 1

85-86: LGTM!

Also applies to: 91-92, 106-107, 119-119

src/lib/stores/stash-reapply-marker.ts: 1

33-35: LGTM!

Also applies to: 39-39, 43-45

src/features/repository/BranchSwitcher.tsx: 1

127-127: LGTM!

Also applies to: 1328-1329, 1343-1344

src/features/repository/ChangesPanel.tsx: 1

1295-1296: LGTM!

src/features/repository/StashesDialog.tsx: 1

35-35: LGTM!

Also applies to: 152-153, 376-377

src/lib/stores/worktree-removal.ts: 1

27-27: LGTM!

Also applies to: 547-548

src/features/repository/ConflictBanner.tsx: 1

20-20: LGTM!

Also applies to: 205-207, 327-329, 375-375

src/features/help/content.ts: 1

959-964: LGTM!

@theBGuy

theBGuy commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai Thanks. I'm declining the outside-diff Minor on src/features/repository/conflict-banner-state.ts:70-99 (cancel the pre-marker stash-count read). The reasons, for the record:

  • Which reads can land stale. The compounds run through useRepoMutation. Its onSettled invalidates the repo subtree (src/lib/git/queries/repo-mutation-options.ts:71-83) before mutateAsync resolves, which is before reportAutostashOutcome records the marker. invalidateQueries defaults to cancelRefetch: true, so when the stash-count query already has data, any read in flight is cancelled and replaced by a fetch that starts after the pop.
  • The one remaining case. A first-ever stash-count fetch still in flight during the compound has no data yet, so it dedupes instead of cancelling. That is the gap the marker's recordedAt soundness note in src/lib/stores/stash-reapply-marker.ts already names. The Changes panel's Stashed Changes section reads the count as soon as a repository opens, so the gap needs a compound to start before that first read lands.
  • The effect if it happens. The drop offer clears early. The backup stays in View stashes, and nothing is dropped.

This is recorded as a documented residual, with no change in this PR.


Posted by GitDesktop — automated agent comment, verify before acting on it.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
fd -t f '^(conflict-banner-state\.ts|repo-mutation-options\.ts|stash-reapply-marker\.ts|useStashReapplyRecovery\.tsx)$'
ast-grep outline src/features/repository/conflict-banner-state.ts
ast-grep outline src/lib/git/queries/repo-mutation-options.ts
ast-grep outline src/lib/stores/stash-reapply-marker.ts
ast-grep outline src/features/repository/useStashReapplyRecovery.tsx
sed -n '1,130p' src/features/repository/conflict-banner-state.ts
sed -n '1,125p' src/lib/git/queries/repo-mutation-options.ts
sed -n '1,120p' src/lib/stores/stash-reapply-marker.ts
rg -n -C 5 'reportAutostashOutcome|mutateAsync|onSettled' src/features/repository/useStashReapplyRecovery.tsx

Length of output: 14750


@theBGuy Thanks for the explanation. I checked the invalidation ordering and the recordedAt note.

I accept leaving this as a documented, non-blocking residual in this PR. The first-fetch race can clear the drop offer early. It does not drop the backup stash.

Would you like a follow-up issue for this residual?


✏️ Learnings added
Learnt from: theBGuy
URL: https://github.com/theBGuy/GitDesktop/pull/558

Timestamp: 2026-10-16T20:16:31.536Z
Learning: In theBGuy/GitDesktop, src/lib/stores/stash-reapply-marker.ts documents a residual first-fetch race for the stash-reapply banner. Compound mutations trigger repository invalidation before reportAutostashOutcome records the marker. With existing query data, default cancelRefetch behavior replaces an in-flight read. A first-ever stash-count fetch without cached data can instead deduplicate and return pre-pop data after recordedAt. A stale zero can then clear the offer in src/features/repository/conflict-banner-state.ts; the backup stash remains in View stashes and nothing is dropped. The maintainer explicitly accepts this documented residual without a cancellation change in PR #558.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

@theBGuy
theBGuy merged commit 51f3657 into master Oct 16, 2026
9 checks passed
@theBGuy
theBGuy deleted the fix/conflict-banner-stash-reapply branch October 16, 2026 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant