refactor(desktop): extract db helpers from workspaces router#681
refactor(desktop): extract db helpers from workspaces router#681
Conversation
Extract repeated database operations from workspaces.ts into a dedicated db-helpers.ts module to reduce code duplication and improve readability. Extracted helpers include: - getWorkspace, getProject, getWorktree - basic fetches - setLastActiveWorkspace - settings upsert pattern - touchWorkspace - timestamp updates with optional fields - activateProject - project activation with tab order management - hideProjectIfNoWorkspaces - cleanup when last workspace removed - updateActiveWorkspaceIfRemoved - cascading active workspace updates - getBranchWorkspace - branch workspace lookup - getMaxWorkspaceTabOrder - tab order calculation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors the workspaces tRPC router into modular procedure modules (create, delete, query, branch, status, git-status, init), adds workspace DB helpers, and implements background worktree initialization; exposes Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant tRPC_Server
participant DB
participant Git
participant InitManager
Client->>tRPC_Server: call createWorkspace / openWorktree
tRPC_Server->>DB: insert workspace/worktree records
tRPC_Server->>InitManager: start background initializeWorkspaceWorktree(params)
InitManager->>Git: refresh default branch / fetch refs
Git-->>InitManager: branch/ref existence, fetch results
InitManager->>DB: update worktree gitStatus & paths
InitManager->>InitManager: emit progress events
InitManager->>Client: (via subscription) progress updates
InitManager->>DB: mark initialized, track analytics
Client->>tRPC_Server: query getInitProgress / getWorktreeInfo
tRPC_Server->>DB: return persisted worktree + git/github status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Break up the large workspaces.ts file (~1560 lines) into smaller, focused modules organized by domain: utils/ - workspace-init.ts: Background worktree initialization logic procedures/ - create.ts: create, createBranchWorkspace, openWorktree - delete.ts: delete, close, canDelete - query.ts: get, getAll, getAllGrouped, getActive - branch.ts: getBranches, switchBranchWorkspace - git-status.ts: refreshGitStatus, getGitHubStatus, getWorktreeInfo, getWorktreesByProject - status.ts: setActive, reorder, update, setUnread - init.ts: onInitProgress, retryInit, getInitProgress, getSetupCommands The main workspaces.ts is now a thin orchestrator (~35 lines) that merges all sub-routers using tRPC's mergeRouters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![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
🤖 Fix all issues with AI agents
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.ts:
- Around line 17-56: The current getBranches implementation builds
worktreeBranchMap as Record<string,string> and assigns
worktreeBranchMap[ws.branch] = ws.id which silently overwrites if multiple
worktrees share a branch; change worktreeBranchMap to Record<string,string[]>
and when iterating projectWorkspaces push ws.id into
worktreeBranchMap[ws.branch] (initializing an array if needed), then update the
returned payload so inUse remains Object.keys(worktreeBranchMap) and
inUseWorkspaces maps branch -> string[] (or if API must stay string, pick a
deterministic winner like the first id and document that choice); update any
consumers accordingly (symbols: getBranches, worktreeBranchMap,
projectWorkspaces, inUseWorkspaces).
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts:
- Around line 186-199: The teardown is being fire-and-forget via
runTeardown(...).then(...) which can race with the subsequent call to
removeWorktree; change the flow so you await runTeardown(project.mainRepoPath,
worktree.path, workspace.name) (or await its Promise result) before calling
removeWorktree so teardown scripts run against the live worktree, and
handle/report errors from runTeardown (log or decide whether to abort removal)
based on the returned result.success/value from runTeardown.
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts:
- Around line 30-100: getAllGrouped currently calls getWorkspacePath per
workspace causing N+1 DB reads; instead preload related worktree rows once and
use project data already in groupsMap. Query all worktrees in one select (e.g.,
localDb.select().from(worktrees).all()) and build a map by worktree id, then in
the allWorkspaces loop avoid calling getWorkspacePath: for type === "worktree"
read the path from the preloaded worktreesMap, for type === "branch" use the
project's mainRepoPath from groupsMap.get(workspace.projectId).project (or
fallback ""), and remove any per-workspace localDb calls to getWorkspacePath or
projects to eliminate repeated DB access.
- Around line 134-173: The condition checking for "not yet attempted" uses
baseBranch === undefined which is impossible because
SelectWorktree["baseBranch"] is string | null; change the logic in the block
around worktree/baseBranch so that you treat null as "not yet attempted" (i.e.,
check baseBranch === null) or introduce a separate sentinel (e.g.,
baseBranchDetectionAttemptedAt) to distinguish states; update the branch where
detection runs (the code calling hasOriginRemote(project.mainRepoPath),
detectBaseBranch(worktree.path, worktree.branch, defaultBranch), and the
localDb.update(worktrees).set({ baseBranch: ... })) to use the new condition and
persist either the detected branch or null/appropriate sentinel consistently.
🧹 Nitpick comments (14)
apps/desktop/src/lib/trpc/index.ts (1)
18-45: Avoid fully silent failure in Sentry capture path.
Thecatch {}(Line 39-41) can hide regressions (e.g., Sentry import path changes). At least emit a debug/warn with context (and avoid logging PII).Proposed tweak
} catch { - // Sentry not available + console.debug("[trpc/sentry] Sentry capture failed or unavailable"); }apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts (2)
22-28: Push ordering into SQL instead of sorting in JS.
Sorting after.all()reads all rows and does extra work; use DrizzleorderBy(workspaces.tabOrder)where possible.Also applies to: 80-85
12-20: UseTRPCErrorwith appropriate error codes instead of genericError.The procedures at lines 12-20 and 102-119 throw generic
Errorwhich clients receive as internal errors. UseTRPCErrorwith specific codes for better error handling in UI:
- Line 17 (
getprocedure): use code"NOT_FOUND"for missing workspace by ID- Lines 116-118 (
getActiveprocedure): use code"INTERNAL_SERVER_ERROR"for data consistency issuesThis pattern is already established elsewhere in the codebase (e.g.,
usability.ts,external/index.ts).apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts (2)
195-207: Don’t fully swallow fetch errors; log + surface minimally.
The emptycatchforfetchDefaultBranch(Line 204-206) makes diagnosing “stale default branch” issues hard; at leastconsole.warnwith[workspace-init].
279-306: Log the error object (or stack), not onlyerror.message.
Current logging loses stack/context; preferconsole.error("[workspace-init] ...", error)and then derive a user-facing message separately.apps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts (3)
17-26: Extract the magic settings row id (1) into a constant.
Keeps the invariant explicit and reduces duplication across helpers/procedures.
32-56: Avoid.all()for “max/existence/first row” helpers.
getMaxWorkspaceTabOrder,getMaxProjectTabOrder,hideProjectIfNoWorkspaces,selectNextActiveWorkspacecan be done withorderBy(desc(...)).get()or existence checks to avoid loading full tables.Also applies to: 92-115
189-207: Guidelines: switch 2+ parameter helpers to a params object (e.g.,touchWorkspace,updateProjectDefaultBranch).
This improves call-site clarity and future extensibility.Based on coding guidelines, functions with 2+ parameters should accept a single params object.
Also applies to: 243-252
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.ts (1)
32-69:reorder: consider transaction + only updating changed rows.
Current loop does N writes even if few positions changed; wrapping in a transaction also prevents partial reorder if something fails mid-loop.apps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts (2)
55-95: Fire-and-forget init should be explicitly detached (and guarded against unhandled rejections).
Even ifinitializeWorkspaceWorktree“does not throw”, future changes could reintroduce rejections; considervoid initializeWorkspaceWorktree(...).catch(...).
58-72: Prefer typed tRPC errors for missing workspace/worktree/project.
Returning NOT_FOUND (or BAD_REQUEST) helps renderer decide whether to retry vs prompt user.apps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.ts (1)
67-79: UseTRPCErrorwithNOT_FOUNDfor missing project/workspace cases.
Keeps error codes stable for UI and aligns with tRPC best practices. This pattern is already established elsewhere in the codebase (e.g.,usability.ts).Also applies to: 95-97
apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts (1)
151-162: Consider optimizing N+1 query pattern.The current implementation queries each workspace individually inside the map loop, resulting in N+1 database queries. For small datasets this is acceptable, but if projects can have many worktrees, consider using a single query with a left join.
♻️ Suggested optimization using a join
getWorktreesByProject: publicProcedure .input(z.object({ projectId: z.string() })) .query(({ input }) => { - const projectWorktrees = localDb - .select() - .from(worktrees) - .where(eq(worktrees.projectId, input.projectId)) - .all(); - - return projectWorktrees.map((wt) => { - const workspace = localDb - .select() - .from(workspaces) - .where(eq(workspaces.worktreeId, wt.id)) - .get(); - return { - ...wt, - hasActiveWorkspace: workspace !== undefined, - workspace: workspace ?? null, - }; - }); + const results = localDb + .select({ + worktree: worktrees, + workspace: workspaces, + }) + .from(worktrees) + .leftJoin(workspaces, eq(workspaces.worktreeId, worktrees.id)) + .where(eq(worktrees.projectId, input.projectId)) + .all(); + + return results.map(({ worktree, workspace }) => ({ + ...worktree, + hasActiveWorkspace: workspace !== null, + workspace, + })); }),apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts (1)
221-227: Consider batching tabOrder updates.The loop performs individual UPDATE statements for each workspace. This could be optimized to a single batch update if workspace counts grow large, though the current approach is functionally correct.
♻️ Batch update alternative
- for (const ws of projectWorkspaces) { - localDb - .update(workspaces) - .set({ tabOrder: ws.tabOrder + 1 }) - .where(eq(workspaces.id, ws.id)) - .run(); - } + // Batch increment all existing workspace tabOrders + localDb + .update(workspaces) + .set({ tabOrder: sql`${workspaces.tabOrder} + 1` }) + .where( + and( + eq(workspaces.projectId, input.projectId), + not(eq(workspaces.id, newWorkspaceId)), + ), + ) + .run();Note: Requires importing
sqlfromdrizzle-orm.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/desktop/src/lib/trpc/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/init.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/status.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.tsapps/desktop/src/lib/trpc/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/init.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.tsapps/desktop/src/lib/trpc/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/init.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.tsapps/desktop/src/lib/trpc/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/init.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.tsapps/desktop/src/lib/trpc/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/init.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Separate by ownership and lifecycle - keep transport (routes, API handlers), orchestration (tRPC procedures), and domain rules in distinct layers
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : tRPC procedures and API route handlers should validate and delegate; keep orchestrators thin
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : tRPC procedures and API route handlers should validate and delegate; keep orchestrators thin
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/index.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : Use TRPCError with appropriate error codes: NOT_FOUND, UNAUTHORIZED, FORBIDDEN, BAD_REQUEST, INTERNAL_SERVER_ERROR, NOT_IMPLEMENTED
Applied to files:
apps/desktop/src/lib/trpc/index.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/*.ts : Never import Node.js modules in shared code like electron-router-dom.ts - it runs in both main and renderer processes
Applied to files:
apps/desktop/src/lib/trpc/index.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/lib/trpc/index.ts
🧬 Code graph analysis (7)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.ts (4)
apps/desktop/src/lib/trpc/index.ts (2)
router(47-47)publicProcedure(49-49)apps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts (3)
getWorkspace(134-140)touchWorkspace(189-207)setLastActiveWorkspace(17-26)apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(82-82)packages/local-db/src/schema/schema.ts (1)
workspaces(80-118)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.ts (5)
apps/desktop/src/lib/trpc/index.ts (2)
router(47-47)publicProcedure(49-49)apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(82-82)packages/local-db/src/schema/schema.ts (2)
projects(16-42)workspaces(80-118)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (2)
listBranches(649-675)safeCheckoutBranch(848-874)apps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts (4)
getBranchWorkspace(228-238)touchWorkspace(189-207)setLastActiveWorkspace(17-26)getWorkspace(134-140)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts (6)
apps/desktop/src/lib/trpc/index.ts (2)
router(47-47)publicProcedure(49-49)apps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts (1)
getWorkspace(134-140)apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(82-82)packages/local-db/src/schema/schema.ts (3)
workspaces(80-118)projects(16-42)worktrees(50-72)apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (1)
getWorkspacePath(22-43)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (2)
hasOriginRemote(261-269)detectBaseBranch(595-641)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts (6)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(82-82)apps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts (7)
getMaxWorkspaceTabOrder(32-41)setLastActiveWorkspace(17-26)activateProject(63-74)getBranchWorkspace(228-238)touchWorkspace(189-207)getWorktree(156-162)getProject(145-151)apps/desktop/src/main/lib/workspace-init-manager.ts (1)
workspaceInitManager(302-302)apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts (1)
initializeWorkspaceWorktree(36-312)apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)
loadSetupConfig(30-56)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (3)
getCurrentBranch(682-694)safeCheckoutBranch(848-874)worktreeExists(244-259)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (7)
apps/desktop/src/lib/trpc/index.ts (2)
router(47-47)publicProcedure(49-49)apps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts (7)
getWorkspace(134-140)getWorktree(156-162)getProject(145-151)deleteWorkspace(212-214)deleteWorktreeRecord(219-221)hideProjectIfNoWorkspaces(92-101)updateActiveWorkspaceIfRemoved(121-129)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (4)
worktreeExists(244-259)hasUncommittedChanges(401-407)hasUnpushedCommits(409-434)removeWorktree(213-232)apps/desktop/src/main/lib/workspace-init-manager.ts (1)
workspaceInitManager(302-302)packages/local-db/src/schema/schema.ts (1)
SelectWorktree(75-75)apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts (1)
runTeardown(41-80)apps/desktop/src/main/lib/analytics/index.ts (1)
track(27-46)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts (6)
apps/desktop/src/lib/trpc/index.ts (2)
router(47-47)publicProcedure(49-49)apps/desktop/src/shared/types/workspace-init.ts (1)
WorkspaceInitProgress(17-23)apps/desktop/src/main/lib/workspace-init-manager.ts (1)
workspaceInitManager(302-302)apps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts (2)
getWorkspaceWithRelations(168-184)getProject(145-151)apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts (1)
initializeWorkspaceWorktree(36-312)apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)
loadSetupConfig(30-56)
apps/desktop/src/lib/trpc/routers/workspaces/utils/db-helpers.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(82-82)packages/local-db/src/schema/schema.ts (7)
settings(123-139)workspaces(80-118)projects(16-42)SelectProject(45-45)SelectWorkspace(121-121)SelectWorktree(75-75)worktrees(50-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (12)
apps/desktop/src/lib/trpc/index.ts (1)
47-50:mergeRoutersexport is a clean public API improvement.
This makes the new modular router composition pattern more discoverable/consistent.apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.ts (2)
14-30:setActiveflow looks solid (clears unread + persists last active).
Nice use of helpers to keep the procedure short.
73-93: Verify timestamp semantics: renaming shouldn’t necessarily bumplastOpenedAt; unread toggle likely should bumpupdatedAt.
touchWorkspacealways updateslastOpenedAt(Line 88-90 usage), which may skew MRU behavior.setUnreaddoesn’t updateupdatedAtat all. Consider splitting helpers (e.g.,updateWorkspaceMetadatavsmarkWorkspaceOpened).Also applies to: 95-110
apps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts (1)
16-49: Subscription implementation matches trpc-electron observable requirements.
Good: sends current state, filters by ids, unsubscribes cleanly.Based on coding guidelines, subscriptions should use
@trpc/server/observable.apps/desktop/src/lib/trpc/routers/workspaces/procedures/branch.ts (1)
58-103:switchBranchWorkspaceflow is cohesive and well-factored.
Safe checkout + terminal refresh + DB updates are clear and sequenced correctly.apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts (1)
307-311: The lock semantics are safe and do not require changes.acquireProjectLockis properly awaited (line 50) and does not throw—it waits in a loop.releaseProjectLockis synchronous and includes a guard (if (resolve)) making it a safe no-op if the lock was never acquired. No issue with the finally block.Likely an incorrect or invalid review comment.
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
1-34: Clean modular composition pattern.The refactor successfully transforms a monolithic router into a thin orchestrator that merges domain-specific procedure modules. This aligns well with the project learnings to keep orchestrators thin and delegate to utility functions. The JSDoc documentation clearly maps procedures to their logical groups.
apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts (1)
22-84: LGTM - refreshGitStatus mutation is well-structured.The procedure correctly validates all required entities, handles default branch synchronization with remote, and persists the git status. The logic is at the ~60-line threshold but remains cohesive and readable as a single unit.
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts (2)
132-260: Race condition handling is well-implemented.The
createBranchWorkspaceprocedure correctly handles concurrent calls usingonConflictDoNothing()combined with the unique partial index. The "insert-first, then shift" pattern prevents double-shifts from race conditions. Based on learnings, this ~130-line procedure could eventually be extracted into a utility function for independent testing, but the current inline approach is acceptable for now.
108-119: Error handling is already implemented internally.The
initializeWorkspaceWorktreefunction has comprehensive error handling: the entire function body is wrapped in a try-catch-finally block that catches errors, logs them with context ([workspace-init]), performs cleanup, updates progress state, and finalizes the job. No additional.catch()handler is needed at the call site.apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (2)
174-220: Project lock coordination is correctly implemented.The lock acquisition in a try/finally block ensures proper release even if worktree removal fails. The cancellation coordination with
workspaceInitManagerproperly prevents racing with concurrent init operations.
251-282: Close procedure is clean and consistent.The
closeprocedure correctly handles the simpler case of closing a workspace while preserving the worktree. The cleanup sequence (terminals → workspace record → project visibility → active workspace) is appropriate.
| getAllGrouped: publicProcedure.query(() => { | ||
| const activeProjects = localDb | ||
| .select() | ||
| .from(projects) | ||
| .where(isNotNull(projects.tabOrder)) | ||
| .all(); | ||
|
|
||
| const groupsMap = new Map< | ||
| string, | ||
| { | ||
| project: { | ||
| id: string; | ||
| name: string; | ||
| color: string; | ||
| tabOrder: number; | ||
| githubOwner: string | null; | ||
| mainRepoPath: string; | ||
| }; | ||
| workspaces: Array<{ | ||
| id: string; | ||
| projectId: string; | ||
| worktreeId: string | null; | ||
| worktreePath: string; | ||
| type: "worktree" | "branch"; | ||
| branch: string; | ||
| name: string; | ||
| tabOrder: number; | ||
| createdAt: number; | ||
| updatedAt: number; | ||
| lastOpenedAt: number; | ||
| isUnread: boolean; | ||
| }>; | ||
| } | ||
| >(); | ||
|
|
||
| for (const project of activeProjects) { | ||
| groupsMap.set(project.id, { | ||
| project: { | ||
| id: project.id, | ||
| name: project.name, | ||
| color: project.color, | ||
| // biome-ignore lint/style/noNonNullAssertion: filter guarantees tabOrder is not null | ||
| tabOrder: project.tabOrder!, | ||
| githubOwner: project.githubOwner ?? null, | ||
| mainRepoPath: project.mainRepoPath, | ||
| }, | ||
| workspaces: [], | ||
| }); | ||
| } | ||
|
|
||
| const allWorkspaces = localDb | ||
| .select() | ||
| .from(workspaces) | ||
| .all() | ||
| .sort((a, b) => a.tabOrder - b.tabOrder); | ||
|
|
||
| for (const workspace of allWorkspaces) { | ||
| if (groupsMap.has(workspace.projectId)) { | ||
| groupsMap.get(workspace.projectId)?.workspaces.push({ | ||
| ...workspace, | ||
| type: workspace.type as "worktree" | "branch", | ||
| worktreePath: getWorkspacePath(workspace) ?? "", | ||
| isUnread: workspace.isUnread ?? false, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return Array.from(groupsMap.values()).sort( | ||
| (a, b) => a.project.tabOrder - b.project.tabOrder, | ||
| ); | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find definition of getWorkspacePath
rg -n "function getWorkspacePath|const getWorkspacePath|export.*getWorkspacePath" --type=ts -A 5Repository: superset-sh/superset
Length of output: 679
🏁 Script executed:
# Find all call sites of getWorkspacePath to understand usage patterns
rg -n "getWorkspacePath\(" --type=ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 2026
🏁 Script executed:
# Check the file structure to understand where getWorkspacePath is defined
fd "getWorkspacePath|workspace" --type=f --extension=ts --extension=tsx apps/desktop/src/libRepository: superset-sh/superset
Length of output: 191
🏁 Script executed:
# Read the full getWorkspacePath function implementation
cat -n apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsRepository: superset-sh/superset
Length of output: 1481
getAllGrouped queries DB inside loop—eliminate N+1 reads.
getWorkspacePath() executes DB lookups per workspace in the loop (lines 91, 178). For branch workspaces, this re-queries projects that are already loaded in groupsMap; for worktree workspaces, it queries worktrees without preloading. Preload all worktrees once and pass project data directly to avoid repeated DB calls.
🤖 Prompt for AI Agents
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts around
lines 30 - 100, getAllGrouped currently calls getWorkspacePath per workspace
causing N+1 DB reads; instead preload related worktree rows once and use project
data already in groupsMap. Query all worktrees in one select (e.g.,
localDb.select().from(worktrees).all()) and build a map by worktree id, then in
the allWorkspaces loop avoid calling getWorkspacePath: for type === "worktree"
read the path from the preloaded worktreesMap, for type === "branch" use the
project's mainRepoPath from groupsMap.get(workspace.projectId).project (or
fallback ""), and remove any per-workspace localDb calls to getWorkspacePath or
projects to eliminate repeated DB access.
| // Detect and persist base branch for existing worktrees that don't have it | ||
| // We use undefined to mean "not yet attempted" and null to mean "attempted but not found" | ||
| let baseBranch = worktree?.baseBranch; | ||
| if (worktree && baseBranch === undefined && project) { | ||
| // Only attempt detection if there's a remote origin | ||
| const hasRemote = await hasOriginRemote(project.mainRepoPath); | ||
| if (hasRemote) { | ||
| try { | ||
| const defaultBranch = project.defaultBranch || "main"; | ||
| const detected = await detectBaseBranch( | ||
| worktree.path, | ||
| worktree.branch, | ||
| defaultBranch, | ||
| ); | ||
| if (detected) { | ||
| baseBranch = detected; | ||
| } | ||
| // Persist the result (detected branch or null sentinel) | ||
| localDb | ||
| .update(worktrees) | ||
| .set({ baseBranch: detected ?? null }) | ||
| .where(eq(worktrees.id, worktree.id)) | ||
| .run(); | ||
| } catch { | ||
| // Detection failed, persist null to avoid retrying | ||
| localDb | ||
| .update(worktrees) | ||
| .set({ baseBranch: null }) | ||
| .where(eq(worktrees.id, worktree.id)) | ||
| .run(); | ||
| } | ||
| } else { | ||
| // No remote - persist null to avoid retrying | ||
| localDb | ||
| .update(worktrees) | ||
| .set({ baseBranch: null }) | ||
| .where(eq(worktrees.id, worktree.id)) | ||
| .run(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the local-db schema definition for worktrees
find . -type f -name "*schema*" -o -name "*worktree*" | grep -E "\.(ts|tsx)$" | head -20Repository: superset-sh/superset
Length of output: 339
🏁 Script executed:
# Search for worktrees table schema definition in the packages/local-db directory
fd -e ts -e tsx . packages/local-db/src | head -20Repository: superset-sh/superset
Length of output: 249
🏁 Script executed:
# Look for the worktrees schema definition
rg -n "worktrees.*=" packages/local-db/src --type=ts -A 10 | head -50Repository: superset-sh/superset
Length of output: 1657
🏁 Script executed:
# Read the full worktrees table schema definition
sed -n '50,100p' packages/local-db/src/schema/schema.tsRepository: superset-sh/superset
Length of output: 1699
🏁 Script executed:
# Read the context around the code in question to see where worktree comes from
sed -n '120,175p' apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsRepository: superset-sh/superset
Length of output: 1673
Condition baseBranch === undefined is logically impossible and will never trigger detection for legacy rows.
The worktree's baseBranch column is nullable (text("base_branch") without .notNull()), which means SelectWorktree["baseBranch"] is inferred as string | null. When you query from the database, it can only be null or a string—never undefined.
Given the optional chaining worktree?.baseBranch, if worktree exists (truthy), then baseBranch can only be null or a string value. The condition worktree && baseBranch === undefined requires both worktree to be defined AND baseBranch to be undefined simultaneously, which is impossible.
This breaks the intended logic: detection never runs for legacy rows with null baseBranch (existing worktrees), only for completely new worktrees (where worktree doesn't exist yet). To fix this, change the check to baseBranch === null instead of undefined, or add a dedicated sentinel column (like baseBranchDetectionAttemptedAt) to distinguish "not yet attempted" from "attempted but not found."
🤖 Prompt for AI Agents
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts around
lines 134 - 173, The condition checking for "not yet attempted" uses baseBranch
=== undefined which is impossible because SelectWorktree["baseBranch"] is string
| null; change the logic in the block around worktree/baseBranch so that you
treat null as "not yet attempted" (i.e., check baseBranch === null) or introduce
a separate sentinel (e.g., baseBranchDetectionAttemptedAt) to distinguish
states; update the branch where detection runs (the code calling
hasOriginRemote(project.mainRepoPath), detectBaseBranch(worktree.path,
worktree.branch, defaultBranch), and the localDb.update(worktrees).set({
baseBranch: ... })) to use the new condition and persist either the detected
branch or null/appropriate sentinel consistently.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts:
- Around line 230-233: The delete procedure currently calls
hideProjectIfNoWorkspaces(workspace.projectId) only when project is truthy,
causing inconsistency with close which calls it unconditionally; change the
delete flow (and the similar block around lines referenced 266-268) to call
hideProjectIfNoWorkspaces(workspace.projectId) unconditionally after workspace
deletion so the project is hidden regardless of a stale getProject result,
keeping the same call-site (hideProjectIfNoWorkspaces) and parameter.
- Around line 153-216: The log lines inside the delete procedure are
inconsistent: only the initial console.log has a “[workspace/delete]” prefix
while console.error and console.warn (e.g., the teardown failure log, "Failed to
remove worktree" log, and "Worktree ... not found" warn) lack it; also you
should handle terminalManager.killByWorkspaceId failures before proceeding.
Update all console.* calls in this flow to include the same "[workspace/delete]"
prefix (look for usages around workspaceInitManager,
terminalManager.killByWorkspaceId, runTeardown, removeWorktree and their related
error/warn logs) and add a check after terminalResult = await
terminalManager.killByWorkspaceId(input.id) to detect a failed kill (use the
terminalResult shape returned by killByWorkspaceId), log a prefixed error and
return { success: false, error: <message> } early so you don’t continue to
worktree teardown when terminals weren’t closed.
- Around line 141-248: The delete procedure returns { success: false, error: ...
} on failures while the close procedure throws tRPC errors, causing inconsistent
client handling; update the delete mutation (the publicProcedure named "delete"
in this router) to throw TRPCError instances instead of returning error objects
for cases like workspace not found, git/remove failures, and terminal-kill
failures (use TRPCError with appropriate codes such as NOT_FOUND or CONFLICT),
and ensure cleanup paths still call workspaceInitManager.clearJob and other
teardown logic inside finally blocks where needed so errors propagate as throws
rather than returned { success: false } objects.
- Around line 26-139: The canDelete procedure currently returns inconsistent
shapes (some branches omit warning) and computes activeTerminalCount but never
uses it; update the canDelete publicProcedure in delete.ts to always include the
warning field in every returned object (use null or a descriptive string) and
decide on terminal gating: if you want active terminals to block deletion, set
canDelete to false when terminalManager.getSessionCountByWorkspaceId(input.id) >
0 and return a reason like "Active terminals present" (ensure
activeTerminalCount is still returned); otherwise, explicitly document and
return warning:null and keep canDelete true — apply the chosen behavior across
all return paths including the workspace-not-found and catch(error) branches so
the response shape is uniform.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Co-locate code by lifecycle - feature-specific code lives together, not split by type (e.g., all task-related code in router/task/)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| canDelete: publicProcedure | ||
| .input( | ||
| z.object({ | ||
| id: z.string(), | ||
| // Skip expensive git checks (status, unpushed) during polling - only check terminal count | ||
| skipGitChecks: z.boolean().optional(), | ||
| }), | ||
| ) | ||
| .query(async ({ input }) => { | ||
| const workspace = getWorkspace(input.id); | ||
|
|
||
| if (!workspace) { | ||
| return { | ||
| canDelete: false, | ||
| reason: "Workspace not found", | ||
| workspace: null, | ||
| activeTerminalCount: 0, | ||
| hasChanges: false, | ||
| hasUnpushedCommits: false, | ||
| }; | ||
| } | ||
|
|
||
| const activeTerminalCount = | ||
| terminalManager.getSessionCountByWorkspaceId(input.id); | ||
|
|
||
| // Branch workspaces are non-destructive to close - no git checks needed | ||
| if (workspace.type === "branch") { | ||
| return { | ||
| canDelete: true, | ||
| reason: null, | ||
| workspace, | ||
| warning: null, | ||
| activeTerminalCount, | ||
| hasChanges: false, | ||
| hasUnpushedCommits: false, | ||
| }; | ||
| } | ||
|
|
||
| // If skipping git checks, return early with just terminal count | ||
| // This is used during polling to avoid expensive git operations | ||
| if (input.skipGitChecks) { | ||
| return { | ||
| canDelete: true, | ||
| reason: null, | ||
| workspace, | ||
| warning: null, | ||
| activeTerminalCount, | ||
| hasChanges: false, | ||
| hasUnpushedCommits: false, | ||
| }; | ||
| } | ||
|
|
||
| const worktree = workspace.worktreeId | ||
| ? getWorktree(workspace.worktreeId) | ||
| : null; | ||
| const project = getProject(workspace.projectId); | ||
|
|
||
| if (worktree && project) { | ||
| try { | ||
| const exists = await worktreeExists( | ||
| project.mainRepoPath, | ||
| worktree.path, | ||
| ); | ||
|
|
||
| if (!exists) { | ||
| return { | ||
| canDelete: true, | ||
| reason: null, | ||
| workspace, | ||
| warning: | ||
| "Worktree not found in git (may have been manually removed)", | ||
| activeTerminalCount, | ||
| hasChanges: false, | ||
| hasUnpushedCommits: false, | ||
| }; | ||
| } | ||
|
|
||
| // Check for uncommitted changes and unpushed commits in parallel | ||
| const [hasChanges, unpushedCommits] = await Promise.all([ | ||
| hasUncommittedChanges(worktree.path), | ||
| hasUnpushedCommits(worktree.path), | ||
| ]); | ||
|
|
||
| return { | ||
| canDelete: true, | ||
| reason: null, | ||
| workspace, | ||
| warning: null, | ||
| activeTerminalCount, | ||
| hasChanges, | ||
| hasUnpushedCommits: unpushedCommits, | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| canDelete: false, | ||
| reason: `Failed to check worktree status: ${error instanceof Error ? error.message : String(error)}`, | ||
| workspace, | ||
| activeTerminalCount, | ||
| hasChanges: false, | ||
| hasUnpushedCommits: false, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| canDelete: true, | ||
| reason: null, | ||
| workspace, | ||
| warning: "No associated worktree found", | ||
| activeTerminalCount, | ||
| hasChanges: false, | ||
| hasUnpushedCommits: false, | ||
| }; | ||
| }), |
There was a problem hiding this comment.
Make canDelete response shape consistent (always include warning) and decide whether active terminals should block deletion.
Right now some branches omit warning (e.g., workspace-not-found and catch), and activeTerminalCount is computed but never affects canDelete, which can confuse polling/UI logic.
Proposed diff (shape consistency + optional terminal gating)
.query(async ({ input }) => {
const workspace = getWorkspace(input.id);
if (!workspace) {
return {
canDelete: false,
reason: "Workspace not found",
workspace: null,
+ warning: null,
activeTerminalCount: 0,
hasChanges: false,
hasUnpushedCommits: false,
};
}
const activeTerminalCount =
terminalManager.getSessionCountByWorkspaceId(input.id);
+ // Optional: if the intended UX is "don't allow delete/close while terminals are active"
+ // gate here so polling can disable the action.
+ // If you prefer to allow delete and just warn, return canDelete: true with warning instead.
+ if (activeTerminalCount > 0) {
+ return {
+ canDelete: false,
+ reason: "Close running terminals before deleting this workspace",
+ workspace,
+ warning: null,
+ activeTerminalCount,
+ hasChanges: false,
+ hasUnpushedCommits: false,
+ };
+ }
// Branch workspaces are non-destructive to close - no git checks needed
if (workspace.type === "branch") {
return {
canDelete: true,
reason: null,
workspace,
warning: null,
activeTerminalCount,
hasChanges: false,
hasUnpushedCommits: false,
};
}
...
} catch (error) {
return {
canDelete: false,
reason: `Failed to check worktree status: ${error instanceof Error ? error.message : String(error)}`,
workspace,
+ warning: null,
activeTerminalCount,
hasChanges: false,
hasUnpushedCommits: false,
};
}🤖 Prompt for AI Agents
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts around
lines 26 - 139, The canDelete procedure currently returns inconsistent shapes
(some branches omit warning) and computes activeTerminalCount but never uses it;
update the canDelete publicProcedure in delete.ts to always include the warning
field in every returned object (use null or a descriptive string) and decide on
terminal gating: if you want active terminals to block deletion, set canDelete
to false when terminalManager.getSessionCountByWorkspaceId(input.id) > 0 and
return a reason like "Active terminals present" (ensure activeTerminalCount is
still returned); otherwise, explicitly document and return warning:null and keep
canDelete true — apply the chosen behavior across all return paths including the
workspace-not-found and catch(error) branches so the response shape is uniform.
| delete: publicProcedure | ||
| .input(z.object({ id: z.string() })) | ||
| .mutation(async ({ input }) => { | ||
| const workspace = getWorkspace(input.id); | ||
|
|
||
| if (!workspace) { | ||
| return { success: false, error: "Workspace not found" }; | ||
| } | ||
|
|
||
| // Cancel any ongoing initialization and wait for it to complete | ||
| // This ensures we don't race with init's git operations | ||
| if (workspaceInitManager.isInitializing(input.id)) { | ||
| console.log( | ||
| `[workspace/delete] Cancelling init for ${input.id}, waiting for completion...`, | ||
| ); | ||
| workspaceInitManager.cancel(input.id); | ||
| // Wait for init to finish (up to 30s) - it will see cancellation and exit | ||
| await workspaceInitManager.waitForInit(input.id, 30000); | ||
| } | ||
|
|
||
| // Kill all terminal processes in this workspace first | ||
| const terminalResult = await terminalManager.killByWorkspaceId( | ||
| input.id, | ||
| ); | ||
|
|
||
| const project = getProject(workspace.projectId); | ||
|
|
||
| let worktree: SelectWorktree | undefined; | ||
|
|
||
| // Branch workspaces don't have worktrees - skip worktree operations | ||
| if (workspace.type === "worktree" && workspace.worktreeId) { | ||
| worktree = getWorktree(workspace.worktreeId); | ||
|
|
||
| if (worktree && project) { | ||
| // Acquire project lock before any git operations | ||
| // This prevents racing with any concurrent init operations | ||
| await workspaceInitManager.acquireProjectLock(project.id); | ||
|
|
||
| try { | ||
| // Run teardown scripts before removing worktree | ||
| const exists = await worktreeExists( | ||
| project.mainRepoPath, | ||
| worktree.path, | ||
| ); | ||
|
|
||
| if (exists) { | ||
| const teardownResult = await runTeardown( | ||
| project.mainRepoPath, | ||
| worktree.path, | ||
| workspace.name, | ||
| ); | ||
| if (!teardownResult.success) { | ||
| console.error( | ||
| `Teardown failed for workspace ${workspace.name}:`, | ||
| teardownResult.error, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| if (exists) { | ||
| await removeWorktree(project.mainRepoPath, worktree.path); | ||
| } else { | ||
| console.warn( | ||
| `Worktree ${worktree.path} not found in git, skipping removal`, | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| console.error("Failed to remove worktree:", errorMessage); | ||
| return { | ||
| success: false, | ||
| error: `Failed to remove worktree: ${errorMessage}`, | ||
| }; | ||
| } | ||
| } finally { | ||
| workspaceInitManager.releaseProjectLock(project.id); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Proceed with DB cleanup | ||
| deleteWorkspace(input.id); | ||
|
|
||
| if (worktree) { | ||
| deleteWorktreeRecord(worktree.id); | ||
| } | ||
|
|
||
| if (project) { | ||
| hideProjectIfNoWorkspaces(workspace.projectId); | ||
| } | ||
|
|
||
| updateActiveWorkspaceIfRemoved(input.id); | ||
|
|
||
| const terminalWarning = | ||
| terminalResult.failed > 0 | ||
| ? `${terminalResult.failed} terminal process(es) may still be running` | ||
| : undefined; | ||
|
|
||
| track("workspace_deleted", { workspace_id: input.id }); | ||
|
|
||
| // Clear init job state only after all cleanup is complete | ||
| // This ensures cancellation signals remain visible during cleanup | ||
| workspaceInitManager.clearJob(input.id); | ||
|
|
||
| return { success: true, terminalWarning }; | ||
| }), |
There was a problem hiding this comment.
Unify error semantics between delete and close (throw tRPC errors vs { success: false }).
delete returns { success: false, error: ... } while close throws. Mixing these patterns within the same router tends to cause inconsistent client handling.
Proposed diff (minimal consistency: throw on not found)
delete: publicProcedure
.input(z.object({ id: z.string() }))
.mutation(async ({ input }) => {
const workspace = getWorkspace(input.id);
if (!workspace) {
- return { success: false, error: "Workspace not found" };
+ throw new Error("Workspace not found");
}If you already use TRPCError elsewhere, it may be better to throw that (e.g., NOT_FOUND, CONFLICT) for better client UX.
Also applies to: 250-280
🤖 Prompt for AI Agents
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts around
lines 141 - 248, The delete procedure returns { success: false, error: ... } on
failures while the close procedure throws tRPC errors, causing inconsistent
client handling; update the delete mutation (the publicProcedure named "delete"
in this router) to throw TRPCError instances instead of returning error objects
for cases like workspace not found, git/remove failures, and terminal-kill
failures (use TRPCError with appropriate codes such as NOT_FOUND or CONFLICT),
and ensure cleanup paths still call workspaceInitManager.clearJob and other
teardown logic inside finally blocks where needed so errors propagate as throws
rather than returned { success: false } objects.
| // Cancel any ongoing initialization and wait for it to complete | ||
| // This ensures we don't race with init's git operations | ||
| if (workspaceInitManager.isInitializing(input.id)) { | ||
| console.log( | ||
| `[workspace/delete] Cancelling init for ${input.id}, waiting for completion...`, | ||
| ); | ||
| workspaceInitManager.cancel(input.id); | ||
| // Wait for init to finish (up to 30s) - it will see cancellation and exit | ||
| await workspaceInitManager.waitForInit(input.id, 30000); | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle waitForInit timeout/failure explicitly so deletes don’t hang or fail unclearly.
If waitForInit(input.id, 30000) rejects (timeout/other), the mutation will throw without a clear “what to do next” error, and without returning terminalWarning etc.
Proposed diff (explicit error message)
if (workspaceInitManager.isInitializing(input.id)) {
console.log(
`[workspace/delete] Cancelling init for ${input.id}, waiting for completion...`,
);
workspaceInitManager.cancel(input.id);
- // Wait for init to finish (up to 30s) - it will see cancellation and exit
- await workspaceInitManager.waitForInit(input.id, 30000);
+ try {
+ // Wait for init to finish (up to 30s) - it will see cancellation and exit
+ await workspaceInitManager.waitForInit(input.id, 30000);
+ } catch (error) {
+ throw new Error(
+ `Timed out waiting for workspace init to stop: ${
+ error instanceof Error ? error.message : String(error)
+ }`,
+ );
+ }
}📝 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.
| // Cancel any ongoing initialization and wait for it to complete | |
| // This ensures we don't race with init's git operations | |
| if (workspaceInitManager.isInitializing(input.id)) { | |
| console.log( | |
| `[workspace/delete] Cancelling init for ${input.id}, waiting for completion...`, | |
| ); | |
| workspaceInitManager.cancel(input.id); | |
| // Wait for init to finish (up to 30s) - it will see cancellation and exit | |
| await workspaceInitManager.waitForInit(input.id, 30000); | |
| } | |
| // Cancel any ongoing initialization and wait for it to complete | |
| // This ensures we don't race with init's git operations | |
| if (workspaceInitManager.isInitializing(input.id)) { | |
| console.log( | |
| `[workspace/delete] Cancelling init for ${input.id}, waiting for completion...`, | |
| ); | |
| workspaceInitManager.cancel(input.id); | |
| try { | |
| // Wait for init to finish (up to 30s) - it will see cancellation and exit | |
| await workspaceInitManager.waitForInit(input.id, 30000); | |
| } catch (error) { | |
| throw new Error( | |
| `Timed out waiting for workspace init to stop: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }`, | |
| ); | |
| } | |
| } |
| console.log( | ||
| `[workspace/delete] Cancelling init for ${input.id}, waiting for completion...`, | ||
| ); | ||
| workspaceInitManager.cancel(input.id); | ||
| // Wait for init to finish (up to 30s) - it will see cancellation and exit | ||
| await workspaceInitManager.waitForInit(input.id, 30000); | ||
| } | ||
|
|
||
| // Kill all terminal processes in this workspace first | ||
| const terminalResult = await terminalManager.killByWorkspaceId( | ||
| input.id, | ||
| ); | ||
|
|
||
| const project = getProject(workspace.projectId); | ||
|
|
||
| let worktree: SelectWorktree | undefined; | ||
|
|
||
| // Branch workspaces don't have worktrees - skip worktree operations | ||
| if (workspace.type === "worktree" && workspace.worktreeId) { | ||
| worktree = getWorktree(workspace.worktreeId); | ||
|
|
||
| if (worktree && project) { | ||
| // Acquire project lock before any git operations | ||
| // This prevents racing with any concurrent init operations | ||
| await workspaceInitManager.acquireProjectLock(project.id); | ||
|
|
||
| try { | ||
| // Run teardown scripts before removing worktree | ||
| const exists = await worktreeExists( | ||
| project.mainRepoPath, | ||
| worktree.path, | ||
| ); | ||
|
|
||
| if (exists) { | ||
| const teardownResult = await runTeardown( | ||
| project.mainRepoPath, | ||
| worktree.path, | ||
| workspace.name, | ||
| ); | ||
| if (!teardownResult.success) { | ||
| console.error( | ||
| `Teardown failed for workspace ${workspace.name}:`, | ||
| teardownResult.error, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| if (exists) { | ||
| await removeWorktree(project.mainRepoPath, worktree.path); | ||
| } else { | ||
| console.warn( | ||
| `Worktree ${worktree.path} not found in git, skipping removal`, | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| console.error("Failed to remove worktree:", errorMessage); | ||
| return { | ||
| success: false, | ||
| error: `Failed to remove worktree: ${errorMessage}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Prefix all console logs with context (and consider an early return when terminal kill fails).
Only the console.log has a [workspace/delete] prefix; the other console.error / console.warn lines don’t. Also, if terminal kill failures are common, attempting worktree removal right after may be noisy (file locks) and could benefit from an early “close terminals first” return.
🤖 Prompt for AI Agents
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts around
lines 153 - 216, The log lines inside the delete procedure are inconsistent:
only the initial console.log has a “[workspace/delete]” prefix while
console.error and console.warn (e.g., the teardown failure log, "Failed to
remove worktree" log, and "Worktree ... not found" warn) lack it; also you
should handle terminalManager.killByWorkspaceId failures before proceeding.
Update all console.* calls in this flow to include the same "[workspace/delete]"
prefix (look for usages around workspaceInitManager,
terminalManager.killByWorkspaceId, runTeardown, removeWorktree and their related
error/warn logs) and add a check after terminalResult = await
terminalManager.killByWorkspaceId(input.id) to detect a failed kill (use the
terminalResult shape returned by killByWorkspaceId), log a prefixed error and
return { success: false, error: <message> } early so you don’t continue to
worktree teardown when terminals weren’t closed.
| if (project) { | ||
| hideProjectIfNoWorkspaces(workspace.projectId); | ||
| } | ||
|
|
There was a problem hiding this comment.
Call hideProjectIfNoWorkspaces consistently (likely no need to guard by project).
close calls hideProjectIfNoWorkspaces(workspace.projectId) unconditionally, but delete only calls it when project is truthy—despite passing only the ID. If getProject is briefly stale, delete may leave the project visible while close would hide it.
Proposed diff
- if (project) {
- hideProjectIfNoWorkspaces(workspace.projectId);
- }
+ hideProjectIfNoWorkspaces(workspace.projectId);Also applies to: 266-268
🤖 Prompt for AI Agents
In @apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts around
lines 230 - 233, The delete procedure currently calls
hideProjectIfNoWorkspaces(workspace.projectId) only when project is truthy,
causing inconsistency with close which calls it unconditionally; change the
delete flow (and the similar block around lines referenced 266-268) to call
hideProjectIfNoWorkspaces(workspace.projectId) unconditionally after workspace
deletion so the project is hidden regardless of a stale getProject result,
keeping the same call-site (hideProjectIfNoWorkspaces) and parameter.
**refactor(desktop): modularize workspaces router** Split the large workspaces.ts router (~1560 lines) into focused modules: - Extract db helpers (getWorkspace, touchWorkspace, activateProject, etc.) - Organize procedures by domain (create, delete, query, branch, git-status, status, init) - Fix potential race condition in teardown script
1 participant
Summary
workspaces.tsinto a dedicateddb-helpers.tsmodulegetWorkspace,getProject,getWorktree,setLastActiveWorkspace,touchWorkspace,activateProject,hideProjectIfNoWorkspaces,updateActiveWorkspaceIfRemoved,getBranchWorkspace,getMaxWorkspaceTabOrderTest plan
bun tsc --noEmitin desktop appbun run lint🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.