Repository navigation
fix(desktop): improve teardown and delete error handling with diagnostic logging - #1217
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRemoved stderr suppression in the Neon branch-delete script, made branch-delete failures exit with code 1, and refactored workspace deletion to use a new removeWorktreeFromDisk helper plus structured teardown results and additional diagnostic logging; deletion flows now propagate teardown errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TRPC as "TRPC delete.ts"
participant Terminals as "Terminal Manager"
participant Teardown as "teardown.ts"
participant Git as "git / neonctl"
Client->>TRPC: request delete workspace
TRPC->>Terminals: kill terminals (parallel)
Terminals-->>TRPC: killed / warnings
TRPC->>Teardown: runTeardown(workspace) & removeWorktreeFromDisk
Teardown->>Git: run teardown commands / neonctl/removeWorktree
Git-->>Teardown: {success:true} or {success:false, error}
Teardown-->>TRPC: structured teardown result
TRPC-->>Client: return success or propagate error (on teardown failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Kitenite
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts`:
- Around line 47-50: The log message incorrectly reports "found, no teardown
field" even when config.teardown exists but is empty; update the block that
checks if (!config?.teardown || config.teardown.length === 0) to compute a clear
reason string (e.g., "config not found", "config found, teardown field missing",
or "config found, teardown field empty") and include that reason in the
console.log; reference the existing variables config, config.teardown,
workspaceName, mainRepoPath and the current console.log call in teardown.ts to
implement this clearer diagnostic.
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts (1)
146-170: PrefixremoveWorktreeFromDisklogs with a context tag.The warning/error logs here are missing a
[domain/operation]prefix.As per coding guidelines: Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions.🪵 Suggested logging prefix
- console.warn( - `Worktree ${worktreePath} not found in git, skipping removal`, - ); + console.warn( + `[teardown/removeWorktree] Worktree ${worktreePath} not found in git, skipping removal`, + ); return { success: true }; } - console.error("Failed to remove worktree:", msg); + console.error(`[teardown/removeWorktree] Failed to remove worktree: ${msg}`);apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (2)
200-240: Consider extracting teardown orchestration and switchingrunTeardownto an options object.This mutation is already large; pulling the teardown/cleanup orchestration into a helper would keep the tRPC procedure thinner. Also, switching
runTeardownto an options object reduces positional-parameter ordering risks.As per coding guidelines: Use object parameters for functions with 2 or more parameters instead of positional arguments. Based on learnings: Extract business logic from tRPC procedures when: the procedure exceeds ~50 lines of non-trivial logic, the same logic is needed by multiple procedures, complex error handling exists, or you need to mock the logic independently for testing.♻️ Suggested refactor (options object)
diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts @@ -export async function runTeardown( - mainRepoPath: string, - worktreePath: string, - workspaceName: string, -): Promise<TeardownResult> { +export async function runTeardown({ + mainRepoPath, + worktreePath, + workspaceName, +}: { + mainRepoPath: string; + worktreePath: string; + workspaceName: string; +}): Promise<TeardownResult> { const config = loadSetupConfig(mainRepoPath); diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts @@ - teardownPromise = runTeardown( - project.mainRepoPath, - worktree.path, - workspace.name, - ); + teardownPromise = runTeardown({ + mainRepoPath: project.mainRepoPath, + worktreePath: worktree.path, + workspaceName: workspace.name, + }); @@ - const teardownResult = await runTeardown( - project.mainRepoPath, - worktree.path, - worktree.branch, - ); + const teardownResult = await runTeardown({ + mainRepoPath: project.mainRepoPath, + worktreePath: worktree.path, + workspaceName: worktree.branch, + });
428-447: Add a prefixed context to the missing-worktree warning.The new warning should include the standard
[domain/operation]prefix for consistency with other logs in this file.As per coding guidelines: Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions.🪵 Suggested logging prefix
- console.warn( - `Worktree ${worktree.path} not found in git, skipping removal`, - ); + console.warn( + `[workspace/deleteWorktree] Worktree ${worktree.path} not found in git, skipping removal`, + );
Comment on lines
47
to
+50
| if (!config?.teardown || config.teardown.length === 0) { | ||
| console.log( | ||
| `[teardown] No teardown commands found for "${workspaceName}" (config: ${config ? "found, no teardown field" : "not found"}, mainRepoPath: ${mainRepoPath})`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Clarify the config state in the no-teardown log.
When config.teardown exists but is empty, the message still says “no teardown field,” which can mislead diagnostics.
✅ Suggested tweak
- console.log(
- `[teardown] No teardown commands found for "${workspaceName}" (config: ${config ? "found, no teardown field" : "not found"}, mainRepoPath: ${mainRepoPath})`,
- );
+ const configStatus = !config
+ ? "not found"
+ : config.teardown
+ ? "found, teardown empty"
+ : "found, no teardown field";
+ console.log(
+ `[teardown] No teardown commands found for "${workspaceName}" (config: ${configStatus}, mainRepoPath: ${mainRepoPath})`,
+ );📝 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.
Suggested change
| if (!config?.teardown || config.teardown.length === 0) { | |
| console.log( | |
| `[teardown] No teardown commands found for "${workspaceName}" (config: ${config ? "found, no teardown field" : "not found"}, mainRepoPath: ${mainRepoPath})`, | |
| ); | |
| if (!config?.teardown || config.teardown.length === 0) { | |
| const configStatus = !config | |
| ? "not found" | |
| : config.teardown | |
| ? "found, teardown empty" | |
| : "found, no teardown field"; | |
| console.log( | |
| `[teardown] No teardown commands found for "${workspaceName}" (config: ${configStatus}, mainRepoPath: ${mainRepoPath})`, | |
| ); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts` around lines
47 - 50, The log message incorrectly reports "found, no teardown field" even
when config.teardown exists but is empty; update the block that checks if
(!config?.teardown || config.teardown.length === 0) to compute a clear reason
string (e.g., "config not found", "config found, teardown field missing", or
"config found, teardown field empty") and include that reason in the
console.log; reference the existing variables config, config.teardown,
workspaceName, mainRepoPath and the current console.log call in teardown.ts to
implement this clearer diagnostic.
This was referenced Feb 5, 2026
This branch was previously deployed
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
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.
Fixes #1178
Summary
delete(active workspace) anddeleteWorktree(closed worktree) now abort and return an error if teardown scripts fail, instead of silently proceeding with worktree removalremoveWorktreeFromDiskhelper: Shared helper deduplicates the git worktree removal + error handling logic across both deletion paths2>/dev/nullremoved,warn→error+return 1)Type of Change
Testing
.superset/config.json→ succeeds (no teardown = success)Summary by CodeRabbit
Bug Fixes