fix(sidebar): dispatch workspace delete intent after menu close - #11646
fix(sidebar): dispatch workspace delete intent after menu close#11646ax-dfcorp wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe worktree deletion flow validates captured workspace identities against current store state before confirmation and execution. Stale or missing non-folder targets show a translated toast and stop deletion. Folder misses remain silent. Context-menu deletion now uses deferred typed intents for worktrees, batches, and folders. Git-status hydration runs in a dedicated hook. Batch deletion validates, orders, and executes targets in parallel. Tests cover stale, delayed, lineage, folder, batch, and deferred deletion cases. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Greptile SummaryThis PR fixes a race condition where right-clicking Delete on a workspace could silently no-op if the sidebar refreshed during the context menu's intentional 50 ms close delay. The fix passes the clicked
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to the 50 ms context-menu close window and introduces no new risk to the batch-delete or primary-workspace paths. The live store always takes priority; the snapshot is used only when the workspace ID is completely absent from the map and the snapshot ID matches exactly. All existing defensive guards apply naturally to the snapshot path. The only finding is a test filed under the wrong describe block, which is consistent with the pre-existing file convention. Files Needing Attention: No files require special attention. The test-organization issue in delete-worktree-flow.test.ts is cosmetic and pre-dates this PR.
|
| Filename | Overview |
|---|---|
| src/renderer/src/components/sidebar/delete-worktree-flow.ts | Adds optional targetSnapshot fallback to runWorktreeDelete; live store is always preferred, snapshot only used on exact ID match — logic is correct and well-guarded. |
| src/renderer/src/components/sidebar/WorktreeContextMenu.tsx | Passes full worktree object to runWorktreeDelete and updates useCallback deps from worktree.id to worktree; dependency widening is intentional and necessary to capture the snapshot. |
| src/renderer/src/components/sidebar/delete-worktree-flow.test.ts | New snapshot-fallback test is correct but nested inside describe('runWorktreeBatchDelete') while testing runWorktreeDelete — follows existing pattern but adds to test-organization debt. |
Sequence Diagram
sequenceDiagram
participant User
participant ContextMenu as WorktreeContextMenu
participant Store as AppStore
participant Flow as runWorktreeDelete
User->>ContextMenu: Right-click Delete
ContextMenu->>ContextMenu: handleDelete schedules 50 ms timeout captures worktree snapshot in closure
Note over Store: Sidebar refresh occurs worktree transiently absent from map
ContextMenu->>Flow: setTimeout fires runWorktreeDelete worktree.id worktree
Flow->>Store: getWorktreeMapFromState get worktreeId
Store-->>Flow: undefined transiently missing
Flow->>Flow: targetSnapshot id equals worktreeId
alt snapshot ID matches new path
Flow->>Flow: target equals targetSnapshot
Flow->>Store: removeWorktree worktreeId false
else no snapshot or ID mismatch existing guard
Flow->>Flow: target null return fail closed
end
Reviews (1): Last reviewed commit: "fix(sidebar): keep delayed workspace del..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e no-op runWorktreeDelete fails closed when the clicked row is no longer in the store (concurrent delete, state reset, or a runtime re-pair that drops live rows). The guard is right, but it returned with no feedback, so Delete looked broken. Report the miss with the stale-list toast runWorktreeBatchDelete already uses, extracted to a shared module so both paths share one description string.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Thanks for this — the diagnosis is right. I've pushed a change to your branch that takes a different route:
New tests cover both the vanished-row case and the folder-workspace case, and both go red if the change is reverted. Separately, and not for this PR: the workspace context menu in the agent map resolves its row from a wider source than this flow does, so Delete there can still do nothing for a hidden imported workspace. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f76bbfad-0238-40bd-8d9b-b2c6c9d156e9
📒 Files selected for processing (4)
src/renderer/src/components/sidebar/delete-worktree-flow.test.tssrc/renderer/src/components/sidebar/delete-worktree-flow.tssrc/renderer/src/components/sidebar/stale-workspace-list-toast.tssrc/renderer/src/i18n/locales/en.json
brennanb2025
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 35e1972b-0e6d-4819-a860-09877f701b2d
📒 Files selected for processing (7)
src/renderer/src/components/sidebar/WorktreeContextMenu.tsxsrc/renderer/src/components/sidebar/delete-worktree-flow.test.tssrc/renderer/src/components/sidebar/delete-worktree-flow.tssrc/renderer/src/components/sidebar/stale-workspace-list-toast.tssrc/renderer/src/components/sidebar/worktree-context-menu-delete-intent.test.tssrc/renderer/src/components/sidebar/worktree-context-menu-delete-intent.tssrc/renderer/src/i18n/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/components/sidebar/delete-worktree-flow.test.ts
- src/renderer/src/components/sidebar/delete-worktree-flow.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: adc7c5a3-e6a2-4224-b9b7-979e53389890
📒 Files selected for processing (6)
src/renderer/src/components/sidebar/WorktreeContextMenu.tsxsrc/renderer/src/components/sidebar/delete-worktree-flow.test.tssrc/renderer/src/components/sidebar/delete-worktree-flow.tssrc/renderer/src/components/sidebar/worktree-context-menu-delete-intent.test.tssrc/renderer/src/components/sidebar/worktree-context-menu-delete-intent.tssrc/renderer/src/components/sidebar/worktree-delete-request.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/src/components/sidebar/worktree-context-menu-delete-intent.test.ts
- src/renderer/src/components/sidebar/worktree-context-menu-delete-intent.ts
- src/renderer/src/components/sidebar/WorktreeContextMenu.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx (1)
336-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the same coverage for the lineage confirmation path.
This test covers
handleDeleteonly.handleDeleteAllinDeleteWorktreeDialog.tsx(Lines 319-329) resolveslineageDeleteIdentitiesthrough the same guard, but no test asserts that a stale descendant aborts the lineage deletion. Add a case that renders the dialog withlineageDeleteIdentities, replaces one descendant instance, clicks the destructive button, and assertsshowWorkspaceListChangedToastruns andrunWorktreeDeletesInParalleldoes not.src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx (1)
63-64: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMemoize the parsed identities to keep the callbacks stable.
readWorktreeDeleteIdentitiesreturns a new array on every render. Both arrays are dependencies ofhandleDelete(Line 313) andhandleDeleteAll(Line 348), so thoseuseCallbackwrappers recreate on every render and provide no memoization.♻️ Proposed change
- const worktreeDeleteIdentities = readWorktreeDeleteIdentities(modalData.worktreeDeleteIdentities) - const lineageDeleteIdentities = readWorktreeDeleteIdentities(modalData.lineageDeleteIdentities) + const worktreeDeleteIdentities = useMemo( + () => readWorktreeDeleteIdentities(modalData.worktreeDeleteIdentities), + [modalData.worktreeDeleteIdentities] + ) + const lineageDeleteIdentities = useMemo( + () => readWorktreeDeleteIdentities(modalData.lineageDeleteIdentities), + [modalData.lineageDeleteIdentities] + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d61e8703-2277-48e3-a9fe-863aea83fadb
📒 Files selected for processing (7)
src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsxsrc/renderer/src/components/sidebar/DeleteWorktreeDialog.tsxsrc/renderer/src/components/sidebar/delete-worktree-flow.test.tssrc/renderer/src/components/sidebar/delete-worktree-flow.tssrc/renderer/src/components/sidebar/use-delete-worktree-status-hydration.tssrc/renderer/src/components/sidebar/worktree-delete-execution.tssrc/renderer/src/components/sidebar/worktree-delete-request.ts
|
@coderabbitai review |
|
3 participants
Summary
Right-clicking Delete could silently do nothing because the context menu waited 50 ms before resolving the workspace from live state. A sidebar refresh during that arbitrary delay could temporarily remove the row.
This change uses an explicit delete intent and preserves it through execution:
Immediate ID-only callers retain their existing behavior. Folder deletion retains its separate route, and the backend deletion protocol is unchanged.
Screenshots
No visual design change. Electron validation confirmed that Delete opens the confirmation dialog after the menu closes; canceling leaves the selected workspace intact.
Testing
Compatibility
Renderer-only change. No IPC, remote wire, Git command, filesystem path, keyboard shortcut, or platform-specific behavior changed. The same flow applies to local, folder-workspace, and SSH-backed sidebar state.
Security
No new command execution, filesystem access, authentication, dependency, or IPC surface. Revalidating every captured instance identity at dispatch, confirmation, and queued execution prevents a delayed action from targeting a newly recreated workspace at the same path.