Fix deleting worktree crash and improve UX#798
Fix deleting worktree crash and improve UX#798gsxdsm merged 4 commits intoAutoMaker-Org:v0.15.0rcfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughHarden server worktree deletion with prune-and-verify logic; add batch feature updates and sequencing in UI worktree/branch deletion flows; sanitize persisted worktree settings (keep only Changes
Sequence Diagram(s)sequenceDiagram
participant HTTP as HTTP Handler
participant Remove as git worktree remove
participant Prune as git worktree prune
participant List as git worktree list
participant FS as fs.access()
participant Branch as Branch Delete
HTTP->>Remove: attempt `git worktree remove <path>`
alt remove succeeds
Remove-->>HTTP: success
HTTP->>Branch: delete branch (if validated)
Branch-->>HTTP: branch deleted
else remove fails
Remove-->>HTTP: error
HTTP->>Prune: attempt `git worktree prune`
alt prune succeeds
Prune-->>HTTP: prune completed
HTTP->>List: `git worktree list --porcelain`
alt path not registered
List-->>HTTP: not present -> treat as removed
HTTP->>Branch: delete branch
else still registered
List-->>HTTP: still present -> treat as failure
HTTP->>FS: fs.access(path)
alt fs.access ENOENT
FS-->>HTTP: no dir -> treat as success
else exists
FS-->>HTTP: dir exists -> rethrow original error
end
end
else prune fails
Prune-->>HTTP: error
HTTP->>FS: fs.access(path)
alt fs.access ENOENT
FS-->>HTTP: no dir -> treat as success
HTTP->>Branch: delete branch
else exists
FS-->>HTTP: dir exists -> rethrow original error
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and user experience related to Git worktree management. It addresses critical crash scenarios during worktree deletion by making the server-side removal process more robust. On the UI side, it introduces several optimizations, including optimistic updates and batch processing of feature state, to ensure a smoother and more stable interaction when worktrees are removed. Furthermore, it prevents application crash loops on restart by sanitizing stored worktree selections, ensuring that only valid worktree states are restored. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the worktree deletion process, making it more robust and preventing crashes from various edge cases. The frontend state management has been refactored to use batch updates, which improves performance and avoids potential React errors. Additionally, a sanitization step has been added during settings hydration to prevent crash loops caused by stale worktree paths. The changes are well-thought-out and greatly enhance the stability and user experience. My main feedback is to address a small amount of code duplication for a utility function.
| function sanitizeWorktreeByProject( | ||
| raw: Record<string, { path: string | null; branch: string }> | undefined | ||
| ): Record<string, { path: string | null; branch: string }> { | ||
| if (!raw) return {}; | ||
| const sanitized: Record<string, { path: string | null; branch: string }> = {}; | ||
| for (const [projectPath, worktree] of Object.entries(raw)) { | ||
| if ( | ||
| typeof worktree === 'object' && | ||
| worktree !== null && | ||
| 'path' in worktree && | ||
| worktree.path === null | ||
| ) { | ||
| sanitized[projectPath] = worktree; | ||
| } | ||
| } | ||
| return sanitized; | ||
| } |
There was a problem hiding this comment.
This sanitizeWorktreeByProject function is duplicated in apps/ui/src/hooks/use-settings-migration.ts. To avoid code duplication and improve maintainability, consider extracting this function into a shared utility file (e.g., in apps/ui/src/lib/utils.ts or a new settings-specific utility file) and importing it in both hooks.
| function sanitizeWorktreeByProject( | ||
| raw: Record<string, { path: string | null; branch: string }> | undefined | ||
| ): Record<string, { path: string | null; branch: string }> { | ||
| if (!raw) return {}; | ||
| const sanitized: Record<string, { path: string | null; branch: string }> = {}; | ||
| for (const [projectPath, worktree] of Object.entries(raw)) { | ||
| if ( | ||
| typeof worktree === 'object' && | ||
| worktree !== null && | ||
| 'path' in worktree && | ||
| worktree.path === null | ||
| ) { | ||
| sanitized[projectPath] = worktree; | ||
| } | ||
| } | ||
| return sanitized; | ||
| } |
There was a problem hiding this comment.
This sanitizeWorktreeByProject function is also defined in apps/ui/src/hooks/use-settings-sync.ts. To follow the DRY (Don't Repeat Yourself) principle, this function should be extracted to a shared utility module and imported where needed. This will make the code easier to maintain.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/providers/codex-provider.ts (2)
876-890:⚠️ Potential issue | 🟠 Major
codexSettings.cliTimeoutMsis never applied to the spawn timeoutEven after the propagation fix above, the computed
timeoutstill ignorescliTimeoutMs:const baseTimeout = options.reasoningEffort === 'xhigh' ? CODEX_FEATURE_GENERATION_BASE_TIMEOUT_MS : CODEX_CLI_TIMEOUT_MS; // ← cliTimeoutMs from codexSettings is not consulted const timeout = calculateReasoningTimeout(options.reasoningEffort, baseTimeout);
spawnJSONLProcesswill always receive a value derived from the hardcoded constants, makingcliTimeoutMsinert end-to-end.🛠️ Proposed fix – honour `cliTimeoutMs` as the base timeout
- const baseTimeout = - options.reasoningEffort === 'xhigh' - ? CODEX_FEATURE_GENERATION_BASE_TIMEOUT_MS - : CODEX_CLI_TIMEOUT_MS; + const configuredBase = codexSettings.cliTimeoutMs ?? CODEX_CLI_TIMEOUT_MS; + const baseTimeout = + options.reasoningEffort === 'xhigh' + ? Math.max(configuredBase, CODEX_FEATURE_GENERATION_BASE_TIMEOUT_MS) + : configuredBase; const timeout = calculateReasoningTimeout(options.reasoningEffort, baseTimeout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex-provider.ts` around lines 876 - 890, The timeout passed to spawnJSONLProcess ignores codexSettings.cliTimeoutMs; update the base timeout calculation in the block using calculateReasoningTimeout so it first uses options.codexSettings?.cliTimeoutMs (when set and > 0) as the base, falling back to the existing constants (CODEX_FEATURE_GENERATION_BASE_TIMEOUT_MS or CODEX_CLI_TIMEOUT_MS) based on options.reasoningEffort; ensure calculateReasoningTimeout is called with that computed base and that spawnJSONLProcess receives the resulting timeout.
587-596:⚠️ Potential issue | 🟠 Major
cliTimeoutMsis dead code—declared but never used inloadCodexCliSettingsThe field is defined in
CodexCliSettings(line 595) and seeded intodefaults(line 614), but never propagates to the function's output:
resolved(lines 620–628) — nosettings.codexCliTimeoutMs ?? defaults.cliTimeoutMsline; the field is missing entirely.- Override-merge return (lines 634–642) —
cliTimeoutMsis omitted, so any caller passingoverrides.cliTimeoutMswill have it silently discarded.- Catch fallback (lines 644–652) — same omission.
Additionally, the timeout passed to
spawnJSONLProcess(line 888) is computed solely fromoptions.reasoningEffortand hardcoded constants (lines 877–880); it never referencescliTimeoutMs. No global settings keycodexCliTimeoutMsexists in the codebase.Remove
cliTimeoutMsfromCodexCliSettings(line 595) and from thedefaultsinitialization (line 614) since it cannot be set, retrieved, or used by the function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex-provider.ts` around lines 587 - 596, Remove the unused cliTimeoutMs property: delete cliTimeoutMs from the CodexCliSettings type and from the defaults object in loadCodexCliSettings, and remove any vestigial references in the resolved construction, the override-merge return, and the catch fallback so the function no longer declares or expects that field; also verify spawnJSONLProcess timeout logic remains unchanged (it should continue to derive timeout from options.reasoningEffort and constants) and no other code references codexCliTimeoutMs.
🧹 Nitpick comments (4)
apps/server/src/routes/worktree/routes/delete.ts (1)
55-88:removeSucceededis assigned but never consumed — remove dead variable or use it to gate branch deletion.
removeSucceededis written at lines 58, 67, and 87 but is never read by any downstream conditional. The branch-deletion block (line 93) and the finalres.json(line 108) are unconditional. In practice, the error-throwing path at line 84 handles the only "real failure" case, making the flag redundant. Either remove it entirely, or use it to explicitly guard branch deletion (only attempt branch cleanup after confirmed removal):♻️ Option A — remove the dead variable
- let removeSucceeded = false; try { await execGitCommand(['worktree', 'remove', worktreePath, '--force'], projectPath); - removeSucceeded = true; } catch (removeError) { ... try { await execGitCommand(['worktree', 'prune'], projectPath); - removeSucceeded = true; } catch (pruneError) { ... - removeSucceeded = true; } }♻️ Option B — gate branch deletion on removal outcome
if (deleteBranch && branchName && branchName !== 'main' && branchName !== 'master' + && removeSucceeded ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/delete.ts` around lines 55 - 88, The removeSucceeded flag is currently unused; instead of keeping dead state, gate the branch-cleanup and final response on the actual removal outcome: retain the removeSucceeded variable and its assignments in the try/catch around execGitCommand(['worktree', 'remove'...])/['worktree','prune'] (symbols: removeSucceeded, execGitCommand, worktreePath, projectPath, removeError, pruneError, fs.access), then wrap the subsequent branch deletion block (the code that deletes the branch) and the final res.json response so they only run when removeSucceeded === true; leave the existing throw(removeError) path unchanged so real failures still propagate.apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx (1)
75-86: Consider logging the swallowedonDeletederror.
Catching without any log makes failures hard to diagnose. A lightweightlogger.errororconsole.errorkeeps the crash protection while preserving visibility.Optional tweak
- } catch { - // Prevent errors in onDeleted from propagating to the error boundary - } + } catch (error) { + // Prevent errors in onDeleted from propagating to the error boundary + console.error('onDeleted failed:', error); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx` around lines 75 - 86, The catch block in the delete dialog swallows errors from onDeleted(worktree, deleteBranch); update the catch to capture the thrown error (e or err) and log it (e.g., logger.error or console.error) with context before swallowing to preserve crash protection; keep the existing calls to onOpenChange(false) and setDeleteBranch(false) and only add error logging in the catch for onDeleted referenced in this scope.apps/ui/src/hooks/use-settings-sync.ts (1)
42-63: DRY opportunity: shared sanitize helper.
sanitizeWorktreeByProjectis duplicated inapps/ui/src/hooks/use-settings-migration.ts. Consider extracting to a shared utility to keep behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-settings-sync.ts` around lines 42 - 63, The sanitizeWorktreeByProject implementation is duplicated between use-settings-sync.ts and use-settings-migration.ts; extract this function into a shared utility module (e.g., create a new export from a utils or settings helpers file), export the typed function signature, replace the local sanitizeWorktreeByProject in both files with an import of the shared sanitizeWorktreeByProject, and ensure you export/import the correct types so both use-settings-sync and use-settings-migration reference the exact same helper to keep behavior consistent.apps/ui/src/components/views/board-view.tsx (1)
418-436: Repeated batch-update-and-persist pattern — extract a shared helper.The following three-step pattern appears in
handleRemovedWorktrees(lines 429–433),onBranchDeletedDuringMerge(lines 1615–1619), andonDeleted(lines 2046–2050):
- Filter
hookFeaturesby branch name → collectaffectedIdsbatchUpdateFeatures(affectedIds, updates)- For-loop
persistFeatureUpdate(id, updates)per IDExtracting this to a single
useCallbackhelper also avoids recreating the same logic inline (theonBranchDeletedDuringMergecallback is not memoized, so it's recreated on every render).♻️ Proposed refactor
+ // Shared helper: batch-reset branch assignment and persist for each affected feature + const batchResetBranchFeatures = useCallback( + (branchName: string) => { + const affectedIds = hookFeatures + .filter((f) => f.branchName === branchName) + .map((f) => f.id); + if (affectedIds.length === 0) return; + const updates: Partial<Feature> = { branchName: null }; + batchUpdateFeatures(affectedIds, updates); + for (const id of affectedIds) { + persistFeatureUpdate(id, updates); + } + }, + [hookFeatures, batchUpdateFeatures, persistFeatureUpdate] + );Then replace all three occurrences — e.g. in
handleRemovedWorktrees:- const removedBranches = new Set(removedWorktrees.map((r) => r.branch)); - const affectedIds = hookFeatures - .filter((f) => f.branchName && removedBranches.has(f.branchName)) - .map((f) => f.id); - if (affectedIds.length === 0) return; - const updates = { branchName: null as unknown as string | undefined }; - batchUpdateFeatures(affectedIds, updates); - for (const id of affectedIds) { - persistFeatureUpdate(id, updates); - } + for (const { branch } of removedWorktrees) { + batchResetBranchFeatures(branch); + }And inline in
onBranchDeletedDuringMerge/onDeleted:- const affectedIds = hookFeatures - .filter((f) => f.branchName === branchName) - .map((f) => f.id); - if (affectedIds.length > 0) { - const updates = { branchName: null as unknown as string | undefined }; - batchUpdateFeatures(affectedIds, updates); - for (const id of affectedIds) { - persistFeatureUpdate(id, updates); - } - } + batchResetBranchFeatures(branchName);Also applies to: 1609-1622, 2042-2051
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view.tsx` around lines 418 - 436, Extract the repeated three-step logic in handleRemovedWorktrees/onBranchDeletedDuringMerge/onDeleted into a single memoized helper (e.g., a useCallback named applyUpdatesToBranchFeatures) that accepts a predicate or set of branch names plus an updates object; inside it compute affectedIds from hookFeatures (filtering by branchName), call batchUpdateFeatures(affectedIds, updates), then loop persistFeatureUpdate(id, updates) for each id. Replace the inline logic in handleRemovedWorktrees, onBranchDeletedDuringMerge, and onDeleted with calls to this helper so the code is deduplicated and the callback is memoized to avoid recreating the logic each render.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/delete.ts`:
- Around line 65-67: The current flow sets removeSucceeded = true after running
execGitCommand(['worktree', 'prune'], projectPath) which can produce false
positives; update the delete handler to explicitly verify the target worktree
(worktreePath) is no longer registered under projectPath either by (preferred)
calling execGitCommand(['worktree', 'list', '--porcelain'], projectPath) after
the prune and checking the output does not contain worktreePath, or by
performing that validation up front before attempting removal; ensure you
reference and update the removeSucceeded flag only based on that explicit
presence check (use the existing execGitCommand helper and the
worktreePath/projectPath variables).
- Line 8: The variable removeSucceeded is dead code: either delete its
declaration and all assignments to it, or actually use it to gate the branch
deletion and success response; specifically, locate removeSucceeded in the
delete route handler, and if you choose to keep it, set it based on the
fs.rm/fs.rmdir result (or the function that removes the worktree) and check it
before invoking the branch deletion logic and before sending the 200 success
response so the handler only returns success when removeSucceeded is true;
otherwise remove every reference to removeSucceeded to avoid unused state.
In `@apps/ui/src/components/views/board-view.tsx`:
- Line 2069: The call to void forceSyncSettingsToServer() swallows failures;
change it to await the promise and handle a false result by logging diagnostics
so failures are visible. Replace the void call with: const ok = await
forceSyncSettingsToServer(); if (!ok) log an error (e.g., console.error or the
component/logger used in this file) with context like "forceSyncSettingsToServer
failed — settings may be stale" so the failure is recorded for debugging; ensure
you reference the existing forceSyncSettingsToServer function name exactly when
making the change.
- Line 429: Update the Feature.branchName type to include null (i.e. change its
type from string | undefined to string | undefined | null) and then remove the
double-cast occurrences that force null into a string|undefined (the assignments
like const updates = { branchName: null as unknown as string | undefined } and
the two other similar casts); ensure all places that read or write
Feature.branchName treat null as a valid explicit "cleared" value and adjust any
checks accordingly (use strict === null or === undefined where needed).
---
Outside diff comments:
In `@apps/server/src/providers/codex-provider.ts`:
- Around line 876-890: The timeout passed to spawnJSONLProcess ignores
codexSettings.cliTimeoutMs; update the base timeout calculation in the block
using calculateReasoningTimeout so it first uses
options.codexSettings?.cliTimeoutMs (when set and > 0) as the base, falling back
to the existing constants (CODEX_FEATURE_GENERATION_BASE_TIMEOUT_MS or
CODEX_CLI_TIMEOUT_MS) based on options.reasoningEffort; ensure
calculateReasoningTimeout is called with that computed base and that
spawnJSONLProcess receives the resulting timeout.
- Around line 587-596: Remove the unused cliTimeoutMs property: delete
cliTimeoutMs from the CodexCliSettings type and from the defaults object in
loadCodexCliSettings, and remove any vestigial references in the resolved
construction, the override-merge return, and the catch fallback so the function
no longer declares or expects that field; also verify spawnJSONLProcess timeout
logic remains unchanged (it should continue to derive timeout from
options.reasoningEffort and constants) and no other code references
codexCliTimeoutMs.
---
Duplicate comments:
In `@apps/ui/src/hooks/use-settings-migration.ts`:
- Around line 626-648: The sanitizeWorktreeByProject function is duplicated;
extract this logic into a shared utility function (e.g., export a new
sanitizeWorktreeByProject helper from a common utils module) and replace the
local implementations in both sanitizeWorktreeByProject (in the current hook)
and the duplicate in use-settings-sync with imports from that shared module so
both hooks call the same exported function to avoid drift.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/delete.ts`:
- Around line 55-88: The removeSucceeded flag is currently unused; instead of
keeping dead state, gate the branch-cleanup and final response on the actual
removal outcome: retain the removeSucceeded variable and its assignments in the
try/catch around execGitCommand(['worktree', 'remove'...])/['worktree','prune']
(symbols: removeSucceeded, execGitCommand, worktreePath, projectPath,
removeError, pruneError, fs.access), then wrap the subsequent branch deletion
block (the code that deletes the branch) and the final res.json response so they
only run when removeSucceeded === true; leave the existing throw(removeError)
path unchanged so real failures still propagate.
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 418-436: Extract the repeated three-step logic in
handleRemovedWorktrees/onBranchDeletedDuringMerge/onDeleted into a single
memoized helper (e.g., a useCallback named applyUpdatesToBranchFeatures) that
accepts a predicate or set of branch names plus an updates object; inside it
compute affectedIds from hookFeatures (filtering by branchName), call
batchUpdateFeatures(affectedIds, updates), then loop persistFeatureUpdate(id,
updates) for each id. Replace the inline logic in handleRemovedWorktrees,
onBranchDeletedDuringMerge, and onDeleted with calls to this helper so the code
is deduplicated and the callback is memoized to avoid recreating the logic each
render.
In `@apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx`:
- Around line 75-86: The catch block in the delete dialog swallows errors from
onDeleted(worktree, deleteBranch); update the catch to capture the thrown error
(e or err) and log it (e.g., logger.error or console.error) with context before
swallowing to preserve crash protection; keep the existing calls to
onOpenChange(false) and setDeleteBranch(false) and only add error logging in the
catch for onDeleted referenced in this scope.
In `@apps/ui/src/hooks/use-settings-sync.ts`:
- Around line 42-63: The sanitizeWorktreeByProject implementation is duplicated
between use-settings-sync.ts and use-settings-migration.ts; extract this
function into a shared utility module (e.g., create a new export from a utils or
settings helpers file), export the typed function signature, replace the local
sanitizeWorktreeByProject in both files with an import of the shared
sanitizeWorktreeByProject, and ensure you export/import the correct types so
both use-settings-sync and use-settings-migration reference the exact same
helper to keep behavior consistent.
|
|
||
| if (affectedIds.length === 0) return; | ||
|
|
||
| const updates = { branchName: null as unknown as string | undefined }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Feature type and branchName definition in store
rg -n 'branchName' --type ts -B3 -A3 apps/ui/src/store/app-store.ts 2>/dev/null | head -100
echo "---"
echo "Searching in libs/types:"
echo "---"
# Also check shared type definitions
find . -name "*.ts" -path "*/libs/types/*" -o -path "*/types/*" | head -20 | xargs rg -l 'branchName' 2>/dev/null
echo "---"
echo "Searching for Feature interface/type definitions:"
echo "---"
# Look for Feature type definition more broadly
rg -n 'type Feature|interface Feature' --type ts -A10 2>/dev/null | head -80Repository: AutoMaker-Org/automaker
Length of output: 8481
🏁 Script executed:
#!/bin/bash
# Get the full Feature interface definition from libs/types/src/feature.ts
cat -n libs/types/src/feature.ts | sed -n '79,110p'Repository: AutoMaker-Org/automaker
Length of output: 1405
🏁 Script executed:
#!/bin/bash
# Verify the actual casts at lines 429, 1615, and 2046 in board-view.tsx
echo "=== Line 429 context ==="
sed -n '425,435p' apps/ui/src/components/views/board-view.tsx
echo ""
echo "=== Line 1615 context ==="
sed -n '1610,1620p' apps/ui/src/components/views/board-view.tsx
echo ""
echo "=== Line 2046 context ==="
sed -n '2040,2055p' apps/ui/src/components/views/board-view.tsx
echo ""
echo "=== Verify Feature.branchName is optional string, not string | null ==="
rg "branchName\?" libs/types/src/feature.ts -A1 -B1Repository: AutoMaker-Org/automaker
Length of output: 2104
Update Feature.branchName type to include null to eliminate the double-cast workaround.
The double cast (null as unknown as string | undefined) appears at lines 429, 1615, and 2046 because Feature.branchName is typed as string | undefined, which does not include null. This cast silently bypasses TypeScript's type checking, creating a subtle behavioral inconsistency: code checking branchName === undefined will not match the null values being written here, while truthiness checks will.
The correct fix is to update the Feature type definition to include null:
- branchName?: string;
+ branchName?: string | null;Then remove all three casts:
- const updates = { branchName: null as unknown as string | undefined };
+ const updates = { branchName: null };This makes the intent explicit: null = "explicitly cleared/unassigned", undefined = "not set".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/board-view.tsx` at line 429, Update the
Feature.branchName type to include null (i.e. change its type from string |
undefined to string | undefined | null) and then remove the double-cast
occurrences that force null into a string|undefined (the assignments like const
updates = { branchName: null as unknown as string | undefined } and the two
other similar casts); ensure all places that read or write Feature.branchName
treat null as a valid explicit "cleared" value and adjust any checks accordingly
(use strict === null or === undefined where needed).
| // in time and the stale worktree path survives in | ||
| // server settings, causing the deleted worktree to | ||
| // reappear on next load. | ||
| void forceSyncSettingsToServer(); |
There was a problem hiding this comment.
void forceSyncSettingsToServer() silently swallows failure — the very race it guards against can still occur.
The comment explains that without this call, the debounced 1 s sync may not complete in time, leaving the stale worktree path alive in server settings and causing it to reappear on next load. If forceSyncSettingsToServer itself fails (returns false), that reappearance scenario is not prevented, and there is no user feedback or log entry at the call site. The function's own internal logger handles errors, but the outcome is invisible here.
At minimum, log the failure so it shows up in diagnostics:
🛡️ Proposed fix
- void forceSyncSettingsToServer();
+ forceSyncSettingsToServer().then((ok) => {
+ if (!ok) {
+ logger.warn('[Board] forceSyncSettingsToServer failed after worktree deletion; stale path may reappear on reload');
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void forceSyncSettingsToServer(); | |
| const ok = forceSyncSettingsToServer(); | |
| if (!ok) { | |
| logger.warn('[Board] forceSyncSettingsToServer failed after worktree deletion; stale path may reappear on reload'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/board-view.tsx` at line 2069, The call to void
forceSyncSettingsToServer() swallows failures; change it to await the promise
and handle a false result by logging diagnostics so failures are visible.
Replace the void call with: const ok = await forceSyncSettingsToServer(); if
(!ok) log an error (e.g., console.error or the component/logger used in this
file) with context like "forceSyncSettingsToServer failed — settings may be
stale" so the failure is recorded for debugging; ensure you reference the
existing forceSyncSettingsToServer function name exactly when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/services/auto-mode/facade.ts (1)
1053-1058:⚠️ Potential issue | 🟡 Minor
analyzeProjectunconditionally throws — this method should not be called from routes.The facade method is intentionally incomplete during migration and will always fail. The route calls it via
AutoModeServiceCompat, which propagates the error. Although the route handler catches the exception and logs it, it still returns{ success: true }to the client, creating a silent failure where the client believes the operation succeeded.Either implement the method properly or refactor the route to use a different endpoint. The current flow risks misleading API behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/auto-mode/facade.ts` around lines 1053 - 1058, The analyzeProject method in AutoModeFacade currently always throws (see analyzeProject) and routes call it via AutoModeServiceCompat so the thrown error is caught but the route still returns { success: true }, causing silent success; fix by either implementing analyzeProject to delegate to the real AutoModeService.analyzeProject (wire up provider.execute or call AutoModeService.analyzeProject) or change the route to call the real AutoModeService directly (replace use of AutoModeServiceCompat in the route handler) and ensure the route returns success=false on exception; update the route handler to propagate real errors or return an accurate success flag instead of always returning true.
🧹 Nitpick comments (2)
apps/ui/src/components/views/board-view.tsx (1)
417-430:persistFeatureUpdatecalls are unawaited inside the loop — possible unhandled rejections.
persistFeatureUpdateis awaited in all other call-sites (e.g. line 1306). Calling it withoutawait(or.catch()) here means any rejection will be an unhandled promise rejection, which in Electron's renderer process can surface as a console error or, in strict configurations, terminate the renderer.♻️ Suggested fix — add `.catch` to suppress/log failures
for (const id of affectedIds) { - persistFeatureUpdate(id, updates); + persistFeatureUpdate(id, updates).catch((err) => + logger.error('[Board] Failed to persist feature branch reset:', err) + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view.tsx` around lines 417 - 430, batchResetBranchFeatures calls persistFeatureUpdate in a for-loop without awaiting or handling its promise, risking unhandled rejections; update the loop in batchResetBranchFeatures to either await persistFeatureUpdate(id, updates) for each id or append a .catch(...) to each call to handle/log failures (and keep batchUpdateFeatures and hookFeatures usage unchanged) so all persistFeatureUpdate promises are handled.apps/server/src/services/auto-mode/facade.ts (1)
1145-1147:saveExecutionStatealways saves withDEFAULT_MAX_CONCURRENCY, losing per-worktree config.The private helper hard-codes
nullbranch andDEFAULT_MAX_CONCURRENCY, so any worktrees running with a custom concurrency limit will have their state saved incorrectly after a success/failure cycle (via theExecutionServicecallback at line 461). ThegetAutoLoopConfigcall is readily available onthis.autoLoopCoordinatorto retrieve the actual limit.♻️ Proposed fix
private async saveExecutionState(): Promise<void> { - return this.saveExecutionStateForProject(null, DEFAULT_MAX_CONCURRENCY); + const config = this.autoLoopCoordinator.getAutoLoopConfigForProject(this.projectPath, null); + return this.saveExecutionStateForProject(null, config?.maxConcurrency ?? DEFAULT_MAX_CONCURRENCY); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/auto-mode/facade.ts` around lines 1145 - 1147, saveExecutionState is hard-coded to call saveExecutionStateForProject(null, DEFAULT_MAX_CONCURRENCY) which drops per-worktree concurrency and branch info; change it to fetch the current auto-loop config via this.autoLoopCoordinator.getAutoLoopConfig() and pass the real branch and maxConcurrency into saveExecutionStateForProject (use config.maxConcurrency with a fallback to DEFAULT_MAX_CONCURRENCY and config.branch or similar field instead of null). Update the call in saveExecutionState to use those values so saved state reflects the active worktree and its concurrency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/delete.ts`:
- Around line 65-103: The substring check listOut.includes(worktreePath) can
yield false positives; instead parse the porcelain output lines and perform an
exact path match against the worktree entry. Update the check in the prune
fallback block (around execGitCommand / listOut handling) to split listOut into
lines, find the lines that represent worktree paths (e.g. lines that start with
"worktree " in git worktree list --porcelain), and compare the path portion
exactly to worktreePath (trim whitespace and avoid prefix matches or
partial-string contains); use that exact-match result to decide whether prune
cleaned the entry and set/remove removeSucceeded accordingly.
---
Outside diff comments:
In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 1053-1058: The analyzeProject method in AutoModeFacade currently
always throws (see analyzeProject) and routes call it via AutoModeServiceCompat
so the thrown error is caught but the route still returns { success: true },
causing silent success; fix by either implementing analyzeProject to delegate to
the real AutoModeService.analyzeProject (wire up provider.execute or call
AutoModeService.analyzeProject) or change the route to call the real
AutoModeService directly (replace use of AutoModeServiceCompat in the route
handler) and ensure the route returns success=false on exception; update the
route handler to propagate real errors or return an accurate success flag
instead of always returning true.
---
Nitpick comments:
In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 1145-1147: saveExecutionState is hard-coded to call
saveExecutionStateForProject(null, DEFAULT_MAX_CONCURRENCY) which drops
per-worktree concurrency and branch info; change it to fetch the current
auto-loop config via this.autoLoopCoordinator.getAutoLoopConfig() and pass the
real branch and maxConcurrency into saveExecutionStateForProject (use
config.maxConcurrency with a fallback to DEFAULT_MAX_CONCURRENCY and
config.branch or similar field instead of null). Update the call in
saveExecutionState to use those values so saved state reflects the active
worktree and its concurrency.
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 417-430: batchResetBranchFeatures calls persistFeatureUpdate in a
for-loop without awaiting or handling its promise, risking unhandled rejections;
update the loop in batchResetBranchFeatures to either await
persistFeatureUpdate(id, updates) for each id or append a .catch(...) to each
call to handle/log failures (and keep batchUpdateFeatures and hookFeatures usage
unchanged) so all persistFeatureUpdate promises are handled.
… worktree services
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
120-124:setTimeouttimeout ID not tracked — consider cleanup for correctnessThe timeout ID returned by
setTimeoutis discarded. For a 0 ms delay this is practically safe —queryClientis a global singleton, so a post-unmount or stale-projectPathcall merely marks an unobserved cache entry as stale without triggering any network request. Still, if rapid worktree-switch clicks are possible (e.g., keyboard shortcut held down), multiple 0 ms timers are queued; each fires aninvalidateQuerieswhich is idempotent but redundant.If you want to be strict about cleanup, track the ID in a
useRefand cancel the previous timer before scheduling the next one:♻️ Optional: cancel previous pending timer on re-invocation
+ const featureInvalidationTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); + + // Clean up any pending timer on unmount + useEffect(() => { + return () => { + if (featureInvalidationTimerRef.current !== null) { + clearTimeout(featureInvalidationTimerRef.current); + } + }; + }, []); const handleSelectWorktree = useCallback( (worktree: WorktreeInfo) => { const isSameWorktree = worktree.isMain ? currentWorktreePath === null : pathsEqual(worktree.path, currentWorktreePath ?? ''); if (isSameWorktree) return; setCurrentWorktree(projectPath, worktree.isMain ? null : worktree.path, worktree.branch); + if (featureInvalidationTimerRef.current !== null) { + clearTimeout(featureInvalidationTimerRef.current); + } - setTimeout(() => { + featureInvalidationTimerRef.current = setTimeout(() => { + featureInvalidationTimerRef.current = null; queryClient.invalidateQueries({ queryKey: queryKeys.features.all(projectPath), }); }, 0); }, [projectPath, setCurrentWorktree, queryClient, currentWorktreePath] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts` around lines 120 - 124, The current setTimeout call that triggers queryClient.invalidateQueries(queryKeys.features.all(projectPath)) drops the timer ID; track it in a ref (e.g., const pendingTimeout = useRef<number | null>(null)) so you can clear previous timers before scheduling a new one (if (pendingTimeout.current) clearTimeout(pendingTimeout.current); pendingTimeout.current = window.setTimeout(...)), and clearTimeout on unmount or when scheduling anew (and null out pendingTimeout.current) to avoid queuing redundant 0ms timers.apps/ui/src/components/views/board-view.tsx (1)
417-435: Uselogger.errorinstead ofconsole.errorfor consistency.The rest of this file uses the structured
logger(Line 107:createLogger('Board')). Theconsole.errorin the.catch()handler on Line 427 should follow the same pattern.♻️ Proposed fix
persistFeatureUpdate(id, updates).catch((err: unknown) => { - console.error( + logger.error( `[batchResetBranchFeatures] Failed to persist update for feature ${id}:`, err );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view.tsx` around lines 417 - 435, The catch handler in batchResetBranchFeatures uses console.error instead of the module's structured logger; replace the console.error call with the Board logger (created via createLogger('Board')) and log a descriptive message plus the error object (e.g., logger.error("[batchResetBranchFeatures] Failed to persist update for feature %s", id, err) or equivalent) so that persistFeatureUpdate errors are recorded consistently; update the import/use-site where logger is available and keep the rest of the function (hookFeatures, batchUpdateFeatures, persistFeatureUpdate) unchanged.apps/server/src/routes/worktree/routes/delete.ts (2)
137-144: Response always returnssuccess: true— consider tying it toremoveSucceededdefensively.All failure paths throw before reaching line 137, so
removeSucceededis guaranteedtruehere. However, making the response explicitly depend on the flag would be more resilient to future refactors that might add non-throwing failure paths.♻️ Optional defensive tweak
- res.json({ - success: true, + res.json({ + success: removeSucceeded,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/delete.ts` around lines 137 - 144, The response currently always sets success: true even though a local flag removeSucceeded exists; change the res.json call in the delete handler to set success based on removeSucceeded (e.g., success: removeSucceeded) so the JSON reflects actual outcome, and keep the rest of the payload (worktreePath, branch: branchDeleted ? branchName : null, branchDeleted) unchanged; update references in the same function where removeSucceeded, branchDeleted, branchName and worktreePath are defined and used so future non-throwing failure paths are represented correctly in the response.
44-48: UseexecGitCommandconsistently instead of mixing withexecAsync.Lines 45 and 71 use
execAsync(which spawns a shell) for hardcoded git commands, while lines 57, 66, and 128 useexecGitCommandwith array arguments.execGitCommandalready returns stdout as a Promise, so the refactor is straightforward. For line 45 specifically, consider using the existinggetCurrentBranch()helper from the same module instead of duplicating the logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/delete.ts` around lines 44 - 48, Replace uses of execAsync for hardcoded git commands with the existing execGitCommand helper (which returns stdout as Promise<string>) to keep git invocation consistent; specifically, in the block that sets branchName (currently calling execAsync('git rev-parse --abbrev-ref HEAD', { cwd: worktreePath })), call the module's getCurrentBranch() helper instead or use execGitCommand(['rev-parse','--abbrev-ref','HEAD'], worktreePath) and assign its returned string to branchName; also change the other execAsync call around line 71 to use execGitCommand with array args and worktreePath so all git calls (execGitCommand, getCurrentBranch) are used consistently throughout the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/auto-mode/routes/analyze-project.ts`:
- Around line 22-28: Restore the original fire-and-forget behavior: remove the
await before autoModeService.analyzeProject(projectPath), but wrap the call in a
try/catch to catch any immediate thrown errors (synchronous exceptions) and
attach a .catch(...) to the returned promise to log/swallow async rejections so
they don't crash the process; keep res.json({ success: true, message: 'Project
analysis started' }) unchanged. If instead you intentionally want to wait for
completion, keep the await on autoModeService.analyzeProject and update the
response message to something like 'Project analysis completed' and handle
long-running timeouts accordingly.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/delete.ts`:
- Around line 137-144: The response currently always sets success: true even
though a local flag removeSucceeded exists; change the res.json call in the
delete handler to set success based on removeSucceeded (e.g., success:
removeSucceeded) so the JSON reflects actual outcome, and keep the rest of the
payload (worktreePath, branch: branchDeleted ? branchName : null, branchDeleted)
unchanged; update references in the same function where removeSucceeded,
branchDeleted, branchName and worktreePath are defined and used so future
non-throwing failure paths are represented correctly in the response.
- Around line 44-48: Replace uses of execAsync for hardcoded git commands with
the existing execGitCommand helper (which returns stdout as Promise<string>) to
keep git invocation consistent; specifically, in the block that sets branchName
(currently calling execAsync('git rev-parse --abbrev-ref HEAD', { cwd:
worktreePath })), call the module's getCurrentBranch() helper instead or use
execGitCommand(['rev-parse','--abbrev-ref','HEAD'], worktreePath) and assign its
returned string to branchName; also change the other execAsync call around line
71 to use execGitCommand with array args and worktreePath so all git calls
(execGitCommand, getCurrentBranch) are used consistently throughout the file.
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 417-435: The catch handler in batchResetBranchFeatures uses
console.error instead of the module's structured logger; replace the
console.error call with the Board logger (created via createLogger('Board')) and
log a descriptive message plus the error object (e.g.,
logger.error("[batchResetBranchFeatures] Failed to persist update for feature
%s", id, err) or equivalent) so that persistFeatureUpdate errors are recorded
consistently; update the import/use-site where logger is available and keep the
rest of the function (hookFeatures, batchUpdateFeatures, persistFeatureUpdate)
unchanged.
In
`@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts`:
- Around line 120-124: The current setTimeout call that triggers
queryClient.invalidateQueries(queryKeys.features.all(projectPath)) drops the
timer ID; track it in a ref (e.g., const pendingTimeout = useRef<number |
null>(null)) so you can clear previous timers before scheduling a new one (if
(pendingTimeout.current) clearTimeout(pendingTimeout.current);
pendingTimeout.current = window.setTimeout(...)), and clearTimeout on unmount or
when scheduling anew (and null out pendingTimeout.current) to avoid queuing
redundant 0ms timers.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Changes from fix/deleting-worktree * fix: Improve worktree deletion safety and branch cleanup logic * fix: Improve error handling and async operations across auto-mode and worktree services * Update apps/server/src/routes/auto-mode/routes/analyze-project.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Summary by CodeRabbit
New Features
Bug Fixes
Performance