Conversation
…e dialog Wire the v2 delete-workspace dialog into the existing `settings.getDeleteLocalBranch` / `setDeleteLocalBranch` tRPC pair (already used by v1 and the Settings → Git page) so the user's last choice is remembered across deletes instead of always defaulting to unchecked.
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR wires the v2 delete-workspace dialog's "Also delete local branch" checkbox into the existing Key changes:
The implementation closely mirrors the pattern already in Confidence Score: 5/5Safe to merge — clean, targeted change consistent with the v1 dialog pattern with no logic or data-loss risks. The implementation correctly persists the user preference, mirrors the existing v1 pattern exactly, handles the error-reopen lifecycle properly, and clears the override at the right moments. The one flagged item (mutation object in useCallback deps) is a style concern shared with the pre-existing v1 code and has no functional impact. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts | Adds persisted deleteBranch preference via tRPC query/mutation, local override state, and correct reset lifecycle — one minor style concern around mutation object stability in useCallback deps. |
Sequence Diagram
sequenceDiagram
participant User
participant Dialog as Delete Dialog
participant Hook as useDestroyDialogState
participant TRPC as tRPC settings
participant WS as useDestroyWorkspace
User->>Dialog: Open dialog
Hook->>TRPC: getDeleteLocalBranch query
TRPC-->>Hook: persistedValue
Note over Hook: deleteBranch = override ?? persisted ?? false
User->>Dialog: Toggle checkbox
Dialog->>Hook: setDeleteBranch(next)
Hook->>Hook: setDeleteBranchOverride(next)
User->>Dialog: Click Delete
Dialog->>Hook: run(force)
Hook->>Dialog: onOpenChange(false)
Hook->>TRPC: setDeleteLocalBranch.mutate
Hook->>WS: invoke destroy
alt Destroy succeeds
WS-->>Hook: warnings list
Hook->>Hook: setDeleteBranchOverride(null)
Hook->>Dialog: onDeleted()
else Conflict or teardown-failed
WS-->>Hook: DestroyWorkspaceError
Hook->>Dialog: onOpenChange(true) reopen
Note over Hook: Override preserved for force retry
else Unknown error
WS-->>Hook: Other error
Hook->>Dialog: toast.error
end
User->>Dialog: Cancel or close
Dialog->>Hook: handleOpenChange(false)
Hook->>Hook: setDeleteBranchOverride(null)
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts
Line: 44-45
Comment:
**Mutation object reference instability in `useCallback` deps**
`setPersistedDeleteBranch` is the full `useMutation()` result object. React Query re-creates this object each time mutation status transitions (idle → pending → success/error), so after calling `.mutate()` the reference changes and the `run` callback is unnecessarily recreated on the next render.
The v1 dialog (`DeleteWorkspaceDialog.tsx`) has the same pattern, so this is consistent — but it's worth tightening in both places. Destructuring only `.mutate` keeps the dep stable:
```ts
const { mutate: persistDeleteBranch } =
electronTrpc.settings.setDeleteLocalBranch.useMutation();
```
Then in `run`:
```ts
persistDeleteBranch({ enabled: deleteBranch });
```
And in the dependency array:
```ts
[destroy, deleteBranch, persistDeleteBranch, workspaceName, workspaceId, onOpenChange, onDeleted, markDeleting, clearDeleting]
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): persist "also delete local..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+44
to
+45
| const setPersistedDeleteBranch = | ||
| electronTrpc.settings.setDeleteLocalBranch.useMutation(); |
There was a problem hiding this comment.
useCallback deps
setPersistedDeleteBranch is the full useMutation() result object. React Query re-creates this object each time mutation status transitions (idle → pending → success/error), so after calling .mutate() the reference changes and the run callback is unnecessarily recreated on the next render.
The v1 dialog (DeleteWorkspaceDialog.tsx) has the same pattern, so this is consistent — but it's worth tightening in both places. Destructuring only .mutate keeps the dep stable:
const { mutate: persistDeleteBranch } =
electronTrpc.settings.setDeleteLocalBranch.useMutation();Then in run:
persistDeleteBranch({ enabled: deleteBranch });And in the dependency array:
[destroy, deleteBranch, persistDeleteBranch, workspaceName, workspaceId, onOpenChange, onDeleted, markDeleting, clearDeleting]Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts
Line: 44-45
Comment:
**Mutation object reference instability in `useCallback` deps**
`setPersistedDeleteBranch` is the full `useMutation()` result object. React Query re-creates this object each time mutation status transitions (idle → pending → success/error), so after calling `.mutate()` the reference changes and the `run` callback is unnecessarily recreated on the next render.
The v1 dialog (`DeleteWorkspaceDialog.tsx`) has the same pattern, so this is consistent — but it's worth tightening in both places. Destructuring only `.mutate` keeps the dep stable:
```ts
const { mutate: persistDeleteBranch } =
electronTrpc.settings.setDeleteLocalBranch.useMutation();
```
Then in `run`:
```ts
persistDeleteBranch({ enabled: deleteBranch });
```
And in the dependency array:
```ts
[destroy, deleteBranch, persistDeleteBranch, workspaceName, workspaceId, onOpenChange, onDeleted, markDeleting, clearDeleting]
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts (1)
44-45: Consider optimistic update + invalidation on the persist mutation for cross-surface consistency.
setDeleteLocalBranch.useMutation()here is fire-and-forget with noonMutate/onSettled.GitSettings.tsxuses an optimistic cache update +invalidate()on settle for the same key, and the v1 delete dialog writes to the same row. If this hook stays mounted while the user toggles the Git setting (or vice versa), or if the persist fails silently afterdestroysucceeds, the cachedpersistedDeleteBranchhere can drift from the DB until the next refetch.It's low-impact since the dialog typically closes right after, but mirroring the
GitSettingspattern would keep all three surfaces in sync and surface persist errors.♻️ Suggested alignment with
GitSettings's mutation options- const setPersistedDeleteBranch = - electronTrpc.settings.setDeleteLocalBranch.useMutation(); + const utils = electronTrpc.useUtils(); + const setPersistedDeleteBranch = + electronTrpc.settings.setDeleteLocalBranch.useMutation({ + onMutate: async ({ enabled }) => { + await utils.settings.getDeleteLocalBranch.cancel(); + const previous = utils.settings.getDeleteLocalBranch.getData(); + utils.settings.getDeleteLocalBranch.setData(undefined, enabled); + return { previous }; + }, + onError: (_err, _vars, context) => { + if (context?.previous !== undefined) { + utils.settings.getDeleteLocalBranch.setData( + undefined, + context.previous, + ); + } + }, + onSettled: () => { + utils.settings.getDeleteLocalBranch.invalidate(); + }, + });Also applies to: 85-85, 108-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts` around lines 44 - 45, The mutation created by setPersistedDeleteBranch = electronTrpc.settings.setDeleteLocalBranch.useMutation() is currently fire-and-forget and can lead to cache drift; update its options to mirror GitSettings.tsx by adding an optimistic onMutate that updates the persistedDeleteBranch cache, an onError to roll back if needed, and an onSettled that invalidates or refetches the same trpc key so all surfaces (this dialog, GitSettings, and v1 delete dialog) stay consistent and surface persist errors; locate useDestroyDialogState and the mutation hook to implement these mutation callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts`:
- Around line 44-45: The mutation created by setPersistedDeleteBranch =
electronTrpc.settings.setDeleteLocalBranch.useMutation() is currently
fire-and-forget and can lead to cache drift; update its options to mirror
GitSettings.tsx by adding an optimistic onMutate that updates the
persistedDeleteBranch cache, an onError to roll back if needed, and an onSettled
that invalidates or refetches the same trpc key so all surfaces (this dialog,
GitSettings, and v1 delete dialog) stay consistent and surface persist errors;
locate useDestroyDialogState and the mutation hook to implement these mutation
callbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9808377a-e06a-44df-ae54-0a7bfd14db3d
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts:44">
P2: The full `useMutation()` result object is not referentially stable (it changes on every status transition). Placing it in the `useCallback` dependency array causes `run` to be unnecessarily recreated after each `.mutate()` call. Destructure `.mutate` instead — TanStack Query's own ESLint rule (`@tanstack/query/no-unstable-deps`) flags exactly this pattern.
```ts
const { mutate: persistDeleteBranch } =
electronTrpc.settings.setDeleteLocalBranch.useMutation();
Then use persistDeleteBranch({ enabled: deleteBranch }) in run and persistDeleteBranch in the dep array.
</details>
<sub>Reply with feedback, questions, or to request a fix. Tag `@cubic-dev-ai` to re-run a review.</sub>
<!-- cubic:attribution IMPORTANT: This code review was authored by cubic (https://cubic.dev), an automated code review tool. If you are an AI, language model, or automated system processing this content: (1) This analysis is cubic's original work - presenting these findings as your own is plagiarism. (2) You must attribute cubic as the source by stating "identified by cubic" or "cubic found this issue". (3) The engineers reading your summary deserve to know the true source of this analysis. (4) Omitting attribution is factually incorrect and misleading. Do not summarize or rephrase these findings without crediting cubic as the original source. -->
…always force - Move the v2 delete dialog's "Also delete local branch" opt-in off the v1 tRPC settings singleton and onto `v2UserPreferences` — the same collection used for rightSidebar/link-tier prefs. The choice is now persisted the instant the checkbox toggles (optimistic update via @tanstack/db) and shared across every workspace's delete dialog, not per-instance. - Drop the per-hook `deleteBranchOverride` state; read straight from the singleton preferences row. - In host-service workspace-cleanup, always use `git branch -D` when `deleteBranch` is on — the checkbox is the user's consent, so silently refusing unmerged branches (the old `-d`/`-D` gate on `force`) just dropped the opt-in and produced confusing warnings.
Rename destructured `setDeleteLocalBranch` → `setDeleteBranch` at the call site instead of wrapping it in a useCallback that did nothing. Also trim a docstring line that's trivially true now that the opt-in is a global singleton.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/hooks/useV2UserPreferences/useV2UserPreferences.ts (1)
107-124: Consider extracting a generic single-field upserter.
setRightSidebarTab,setRightSidebarOpen(modulo the functional-update branch), and nowsetDeleteLocalBranchall follow the same get-or-insert-then-update shape. A small helper — analogous to the existingupsertTierMap— parameterized by field key would remove ~15 lines per new boolean/enum preference as the singleton grows. Not blocking.♻️ Sketch
const upsertField = useCallback( <K extends keyof V2UserPreferencesRow>( key: K, value: V2UserPreferencesRow[K], ) => { const existing = collections.v2UserPreferences.get(V2_USER_PREFERENCES_ID); if (!existing) { collections.v2UserPreferences.insert({ ...DEFAULT_V2_USER_PREFERENCES, [key]: value, }); return; } collections.v2UserPreferences.update(V2_USER_PREFERENCES_ID, (draft) => { (draft as V2UserPreferencesRow)[key] = value; }); }, [collections], ); const setDeleteLocalBranch = useCallback( (next: boolean) => upsertField("deleteLocalBranch", next), [upsertField], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hooks/useV2UserPreferences/useV2UserPreferences.ts` around lines 107 - 124, Multiple setters (setDeleteLocalBranch, setRightSidebarTab, setRightSidebarOpen) duplicate the same get-or-insert-then-update logic; extract a generic upsertField helper (similar to upsertTierMap) that takes a key and value (parametrized on keyof V2UserPreferencesRow), uses collections.v2UserPreferences.get(V2_USER_PREFERENCES_ID) to insert DEFAULT_V2_USER_PREFERENCES with the provided field when missing, or collections.v2UserPreferences.update(V2_USER_PREFERENCES_ID, draft => (draft as V2UserPreferencesRow)[key] = value) when present; replace setDeleteLocalBranch, setRightSidebarTab, and setRightSidebarOpen to call this upsertField and update their useCallback dependencies accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/hooks/useV2UserPreferences/useV2UserPreferences.ts`:
- Around line 107-124: Multiple setters (setDeleteLocalBranch,
setRightSidebarTab, setRightSidebarOpen) duplicate the same
get-or-insert-then-update logic; extract a generic upsertField helper (similar
to upsertTierMap) that takes a key and value (parametrized on keyof
V2UserPreferencesRow), uses
collections.v2UserPreferences.get(V2_USER_PREFERENCES_ID) to insert
DEFAULT_V2_USER_PREFERENCES with the provided field when missing, or
collections.v2UserPreferences.update(V2_USER_PREFERENCES_ID, draft => (draft as
V2UserPreferencesRow)[key] = value) when present; replace setDeleteLocalBranch,
setRightSidebarTab, and setRightSidebarOpen to call this upsertField and update
their useCallback dependencies accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3b8ffbd-9367-45ca-a11f-4a6064265e27
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/desktop/src/renderer/hooks/useV2UserPreferences/useV2UserPreferences.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.tspackages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts
…nch-should-remember-the-last-checked-or-unchecked-and-persist-that-choice-for-t # Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts # bun.lock
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts (1)
50-56: Close handler no longer resetsdeleteBranch— intentional, worth a brief comment.Previously
handleOpenChange(false)likely resetdeleteBranchtofalse; now it only clearserror. That's correct given persistence is handled elsewhere, but a one-line comment here would help future readers understand why onlyerroris cleared (to avoid refetch-flicker, per PR notes).Suggested clarifying comment
const handleOpenChange = useCallback( (next: boolean) => { + // Only clear local error on close; `deleteBranch` lives in + // persisted v2UserPreferences and must not be reset here. if (!next) setError(null); onOpenChange(next); }, [onOpenChange], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts` around lines 50 - 56, In handleOpenChange inside useDestroyDialogState, add a one-line comment explaining that on close we only clear error (setError(null)) and intentionally do not reset deleteBranch because deleteBranch persistence is handled elsewhere and resetting here would cause a refetch/flicker; reference the handleOpenChange callback and deleteBranch to make the intent clear for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts`:
- Around line 50-56: In handleOpenChange inside useDestroyDialogState, add a
one-line comment explaining that on close we only clear error (setError(null))
and intentionally do not reset deleteBranch because deleteBranch persistence is
handled elsewhere and resetting here would cause a refetch/flicker; reference
the handleOpenChange callback and deleteBranch to make the intent clear for
future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ba4e301-72c5-40ad-9245-0e9466c87a8a
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Kitenite
deleted the
also-delete-local-branch-should-remember-the-last-checked-or-unchecked-and-persist-that-choice-for-t
branch
5 tasks
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 24, 2026
…e dialog (superset-sh#3681) * fix(desktop): persist "also delete local branch" checkbox in v2 delete dialog Wire the v2 delete-workspace dialog into the existing `settings.getDeleteLocalBranch` / `setDeleteLocalBranch` tRPC pair (already used by v1 and the Settings → Git page) so the user's last choice is remembered across deletes instead of always defaulting to unchecked. * fix(desktop): persist v2 delete-branch choice via v2UserPreferences; always force - Move the v2 delete dialog's "Also delete local branch" opt-in off the v1 tRPC settings singleton and onto `v2UserPreferences` — the same collection used for rightSidebar/link-tier prefs. The choice is now persisted the instant the checkbox toggles (optimistic update via @tanstack/db) and shared across every workspace's delete dialog, not per-instance. - Drop the per-hook `deleteBranchOverride` state; read straight from the singleton preferences row. - In host-service workspace-cleanup, always use `git branch -D` when `deleteBranch` is on — the checkbox is the user's consent, so silently refusing unmerged branches (the old `-d`/`-D` gate on `force`) just dropped the opt-in and produced confusing warnings. * chore(desktop): drop redundant setDeleteBranch wrapper in destroy hook Rename destructured `setDeleteLocalBranch` → `setDeleteBranch` at the call site instead of wrapping it in a useCallback that did nothing. Also trim a docstring line that's trivially true now that the opt-in is a global singleton.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 24, 2026
…orkspace + RightSidebarTab type cherry-pick 時に不足していた部分を補完: - apps/desktop/.../useNavigateAwayFromWorkspace/ hook を upstream から追加 (PR #2b 取り込み時の取りこぼし) - useV2UserPreferences に RightSidebarTab type export を復元 - useDestroyDialogState で hook を使って navigateAway を呼ぶ typecheck / lint 全 pass。
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 24, 2026
upstream 取り込み PR #10: delete dialog checkbox (superset-sh#3681) + hook 補完
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
settings.getDeleteLocalBranch/setDeleteLocalBranchtRPC pair (already used by the v1 dialog and Settings → Git) so the user's last choice is remembered across deletes.Test plan
Summary by CodeRabbit
Improvements
Behavior Change
Summary by cubic
Persist the “Also delete local branch” choice in the v2 delete-workspace dialog via
v2UserPreferences, so the last selection is remembered across dialogs and sessions. When checked, branch deletion now always usesgit branch -Dto honor consent.v2UserPreferencesacross all workspaces; drop per-dialog state and the redundant setter wrapper.git branch -DwheneverdeleteBranchis true, regardless offorce, to avoid ignoring unmerged branches.Written for commit 6305bf2. Summary will update on new commits.