Create a worktree for each opened branch#113
Conversation
WalkthroughReplaces recents with a projects/worktrees model: adds Project and Worktree DB entities, new git worktree utilities, trpc endpoints (getRecents, openNew), workspace creation wired to worktrees, removes the old workspaces Zustand store and many recents-related UI components/hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as WorkspaceDropdown
participant TRPC_Projects as trpc.projects.openNew
participant TRPC_Workspaces as trpc.workspaces.create
participant Git as Git Utils
participant DB as Database
rect rgb(220,240,255)
Note over User,DB: Open New Project → Create Workspace flow
User->>UI: Click "Open New Project"
UI->>TRPC_Projects: openNew(folderPath)
TRPC_Projects->>Git: getGitRoot(folderPath)
alt is git repo
Git-->>TRPC_Projects: mainRepoPath
TRPC_Projects->>DB: create/update Project (nanoid, timestamps)
DB-->>TRPC_Projects: Project
TRPC_Projects-->>UI: { success: true, project }
UI->>TRPC_Workspaces: create({ projectId, name? })
TRPC_Workspaces->>Git: generateBranchName()
TRPC_Workspaces->>Git: createWorktree(mainRepoPath, branch, worktreePath)
Git-->>TRPC_Workspaces: success
TRPC_Workspaces->>DB: create Worktree, create Workspace (worktreeId, projectId)
DB-->>TRPC_Workspaces: Workspace
TRPC_Workspaces-->>UI: success
else not git repo
Git-->>TRPC_Projects: error
TRPC_Projects-->>UI: { success: false, error }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
153-155: Clarify whetherlastOpenedAtshould be updated on name change.Line 155 updates
lastOpenedAtwhenever the workspace name is changed. This conflates "last updated" with "last opened," which could confuse analytics or UI that relies on lastOpenedAt to show recently accessed workspaces.Consider whether
updateshould modifylastOpenedAt, or if it should only be updated bysetActive:// Update timestamps workspace.updatedAt = Date.now(); - workspace.lastOpenedAt = Date.now();If updating
lastOpenedAton name changes is intentional (e.g., to treat any interaction as "opening"), please add a comment explaining this semantic choice.
167-171: Inconsistent error handling: returns error object instead of throwing.The
deleteprocedure returns{ success: false, error: "..." }when the workspace is not found (Lines 169-170), whilegetandgetActivethrow errors in similar situations. This inconsistency makes error handling unpredictable for callers.Standardize error handling across all procedures:
const workspace = db.data.workspaces.find((w) => w.id === input.id); if (!workspace) { - return { success: false, error: "Workspace not found" }; + throw new Error(`Workspace ${input.id} not found`); }Then update the return type to just
{ success: true }(remove the error case from the union).
🧹 Nitpick comments (7)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
4-4: Swap toWorkspaceDropdownlooks good; verify width assumptionThe swap from the old add button to
<WorkspaceDropdown />is straightforward and integrates cleanly with the existing layout logic.One minor thing to verify:
ADD_BUTTON_WIDTHis still hard‑coded to48and used in the tab width calculation. If the new dropdown control’s rendered width differs from the old button, tab widths may be slightly off at smaller window sizes. Consider adjusting the constant or deriving the control width from the DOM if you notice layout drift.Also applies to: 122-123
apps/desktop/src/renderer/react-query/projects/useOpenNew.ts (1)
1-22: Solid mutation wrapper; consider guarding useronSuccessagainst invalidate failuresWrapping
trpc.projects.openNew.useMutationand auto‑invalidatingprojects.getRecentsis a good pattern. One small robustness improvement:If
utils.projects.getRecents.invalidate()ever throws/rejects, the user’soptions.onSuccesswon’t run. You can ensure it always runs by usingtry/finally:return trpc.projects.openNew.useMutation({ ...options, - onSuccess: async (...args) => { - // Auto-invalidate projects query - await utils.projects.getRecents.invalidate(); - - // Call user's onSuccess if provided - await options?.onSuccess?.(...args); - }, + onSuccess: async (...args) => { + try { + // Auto-invalidate projects query + await utils.projects.getRecents.invalidate(); + } finally { + // Call user's onSuccess if provided + await options?.onSuccess?.(...args); + } + }, });This keeps cache invalidation while avoiding surprising skips of caller callbacks.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx (1)
13-86: Workspace creation flow is solid; consider surfacingopenNewerrorsThe dropdown wiring for recent projects and “Open New Project...” looks good and lines up with the new router/hook APIs.
Right now, if
openNewreturns{ success: false, error }(e.g., non‑git repo selection), the user sees no feedback and the dropdown just stays open. It may be worth handling that case, e.g.:onSuccess: (result) => { if (result.success) { handleCreateWorkspace(result.project.id); } else if (result.error) { // show toast/notification with result.error } }Similarly, if you don’t have global error handling for
createWorkspace, you might want a local error surface there as well.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
4-8: New projects router aligns well with the updated modelThe
getRecentsandopenNewimplementations look consistent with the newProjectschema and the UI usage:
getRecentscorrectly returns a sorted copy, avoiding in‑place mutation.openNew:
- Cleanly handles cancel vs. selection.
- Validates the directory as a git repo via
isGitRepo.- Reuses existing projects by
mainRepoPathand updateslastOpenedAt, or creates a new one withnanoidand timestamps.Two minor considerations (not blockers):
- You traverse
projectstwice in the “existing project” path (once withdb.data.projects.find, once indb.update). With small lists this is fine; if project count grows, you might eventually want a helper that updates by ID in one pass.- If you expect pre‑existing databases from older schemas, ensure there’s a migration path that initializes
data.projectsand gracefully ignores any legacyrecentProjectsdata.Overall, the router surface matches the client hooks and looks good.
Also applies to: 12-16, 18-67
apps/desktop/src/main/lib/db/schemas.ts (2)
6-15: Schema changes line up with router usageThe new
Project,Worktree,Workspace, andDatabaseshapes match how the routers and hooks are using them (mainRepoPath,projectId,worktreeId, timestamps, etc.), and initializingprojects/worktreesindefaultDatabaseis appropriate.One thing to keep in mind is migration: existing user DB files that predate
projects/worktreeswill need a migration step to populate these arrays and possibly transform any legacyrecentProjectsdata, otherwise the new code will see empty recents. If that migration lives elsewhere, this definition looks good.Also applies to: 20-40, 55-70
42-49: ClarifyTabusage and avoid confusion with other Tab types
Tabis defined here but not included inDatabase, and there is already a differentTabshape inapps/desktop/src/shared/types.ts(fields likename,type: TabType, mosaic layout, etc.).To reduce confusion:
- Either add
tabs: Tab[](or similar) toDatabaseif these will be persisted soon, or- Move/rename this interface (e.g.,
DbTaborWorkspaceTab) or remove it until it’s actually used.This will make it clearer which Tab type is the source of truth in each layer.
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
109-126: Good error handling with clear distinction between valid and invalid states.The implementation correctly distinguishes between:
- No active workspace set (returns
null- valid state)- Active workspace ID exists but workspace missing (throws error - data inconsistency)
This approach helps detect database corruption early.
Consider enhancing the error message to suggest recovery:
if (!workspace) { throw new Error( - `Active workspace ${lastActiveWorkspaceId} not found in database`, + `Active workspace ${lastActiveWorkspaceId} not found in database. Database may be corrupted. Consider resetting lastActiveWorkspaceId in settings.`, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/desktop/src/lib/trpc/routers/projects/projects.ts(2 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(5 hunks)apps/desktop/src/main/lib/db/schemas.ts(1 hunks)apps/desktop/src/renderer/react-query/projects/index.ts(1 hunks)apps/desktop/src/renderer/react-query/projects/useOpenNew.ts(1 hunks)apps/desktop/src/renderer/react-query/projects/useOpenProject.ts(0 hunks)apps/desktop/src/renderer/react-query/projects/useOpenRecent.ts(0 hunks)apps/desktop/src/renderer/react-query/projects/useRemoveRecent.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/AddWorkspaceButton.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/RecentProjectItem.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/RecentSection.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/StartSection/StartSection.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/StartSection/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(0 hunks)
💤 Files with no reviewable changes (12)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/index.ts
- apps/desktop/src/renderer/react-query/projects/useRemoveRecent.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/StartSection/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/StartSection/StartSection.tsx
- apps/desktop/src/renderer/react-query/projects/useOpenRecent.ts
- apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/AddWorkspaceButton.tsx
- apps/desktop/src/renderer/react-query/projects/useOpenProject.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/RecentSection.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/RecentProjectItem.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx (1)
apps/desktop/src/renderer/react-query/projects/useOpenNew.ts (1)
useOpenNew(7-22)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx (1)
WorkspaceDropdown(13-86)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (4)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/main/lib/db/schemas.ts (1)
Project(9-15)apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
isGitRepo(111-118)
apps/desktop/src/main/lib/db/schemas.ts (1)
apps/desktop/src/shared/types.ts (3)
Worktree(58-68)Workspace(70-82)Tab(44-56)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (3)
generateBranchName(12-54)createWorktree(62-82)removeWorktree(89-104)
🔇 Additional comments (3)
apps/desktop/src/renderer/react-query/projects/index.ts (1)
1-1: Barrel export matches new projects API surfaceExporting only
useOpenNewaligns with the newprojectsrouter and hook set; no issues here.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
85-93: Thegetprocedure has no callers in the codebase.The review assumes that the breaking change from returning
nullto throwing an error would affect existing callers. However, verification shows thatworkspaces.getis never called anywhere in the desktop app—onlygetAllandgetActiveare used. The procedure appears to be an unused endpoint or future API without current integrations, so this breaking change has no practical impact on the codebase.Likely an incorrect or invalid review comment.
174-201: No cascading deletion risk — the 1:1 workspace-to-worktree relationship is by designThe codebase explicitly documents this relationship. The Workspace schema includes the comment: "Workspace represents a UI tab (1:1 with Worktree)" (line 29, schemas.ts). Worktree creation logic confirms this: each time a workspace is created, a fresh worktree is pushed to the database (line 67, workspaces.ts). No evidence of worktree sharing or reuse exists in the codebase. The deletion code correctly assumes 1:1 cardinality.
| /** | ||
| * Generate a random branch name using adjective-noun-number pattern | ||
| * Examples: "ws-crimson-forest-42", "ws-azure-cloud-17" | ||
| */ | ||
| export function generateBranchName(): string { | ||
| const adjectives = [ | ||
| "azure", | ||
| "crimson", | ||
| "emerald", | ||
| "golden", | ||
| "indigo", | ||
| "jade", | ||
| "lavender", | ||
| "magenta", | ||
| "navy", | ||
| "olive", | ||
| "pearl", | ||
| "rose", | ||
| "silver", | ||
| "teal", | ||
| "violet", | ||
| ]; | ||
|
|
||
| const nouns = [ | ||
| "cloud", | ||
| "forest", | ||
| "mountain", | ||
| "ocean", | ||
| "river", | ||
| "storm", | ||
| "sunset", | ||
| "thunder", | ||
| "wave", | ||
| "wind", | ||
| "meadow", | ||
| "canyon", | ||
| "glacier", | ||
| "valley", | ||
| "peak", | ||
| ]; | ||
|
|
||
| const adjective = adjectives[Math.floor(Math.random() * adjectives.length)]; | ||
| const noun = nouns[Math.floor(Math.random() * nouns.length)]; | ||
| const number = Math.floor(Math.random() * 100); | ||
|
|
||
| return `${adjective}-${noun}-${number}`; | ||
| } |
There was a problem hiding this comment.
Branch name examples don’t match implementation
JSDoc shows examples like "ws-crimson-forest-42", but the function currently returns "crimson-forest-42" (no ws- prefix).
Either add the prefix in the implementation:
- return `${adjective}-${noun}-${number}`;
+ return `ws-${adjective}-${noun}-${number}`;or update the doc examples to match actual output so downstream expectations (e.g., branch naming conventions or filters) stay consistent.
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts around lines 8 to
54, the JSDoc examples include a "ws-" prefix but the generateBranchName
function returns names without that prefix; update the implementation to prepend
"ws-" to the returned branch name (i.e., return
`ws-${adjective}-${noun}-${number}`) so runtime output matches docs and
downstream conventions, or alternatively update the JSDoc examples to remove
"ws-"—pick the option that matches project naming rules and ensure
tests/consumers are adjusted accordingly.
| export async function createWorktree( | ||
| mainRepoPath: string, | ||
| branch: string, | ||
| worktreePath: string, | ||
| ): Promise<void> { | ||
| try { | ||
| // Ensure the parent directory exists | ||
| const parentDir = join(worktreePath, ".."); | ||
| await mkdir(parentDir, { recursive: true }); | ||
|
|
||
| // Create the worktree with a new branch | ||
| await execAsync(`git worktree add "${worktreePath}" -b "${branch}"`, { | ||
| cwd: mainRepoPath, | ||
| }); | ||
|
|
||
| console.log(`Created worktree at ${worktreePath} with branch ${branch}`); | ||
| } catch (error) { | ||
| console.error(`Failed to create worktree: ${error}`); | ||
| throw new Error(`Failed to create worktree: ${error}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Remove a git worktree | ||
| * @param mainRepoPath - Path to the main git repository | ||
| * @param worktreePath - Path to the worktree to remove | ||
| */ | ||
| export async function removeWorktree( | ||
| mainRepoPath: string, | ||
| worktreePath: string, | ||
| ): Promise<void> { | ||
| try { | ||
| // Remove the worktree (--force removes even if there are uncommitted changes) | ||
| await execAsync(`git worktree remove "${worktreePath}" --force`, { | ||
| cwd: mainRepoPath, | ||
| }); | ||
|
|
||
| console.log(`Removed worktree at ${worktreePath}`); | ||
| } catch (error) { | ||
| console.error(`Failed to remove worktree: ${error}`); | ||
| throw new Error(`Failed to remove worktree: ${error}`); | ||
| } |
There was a problem hiding this comment.
Use safer path handling and avoid shell-constructed git commands
There are a few issues in createWorktree/removeWorktree:
- Parent directory computation
const parentDir = join(worktreePath, "..");
await mkdir(parentDir, { recursive: true });Using join(worktreePath, "..") yields a path with a trailing .. segment; behavior is OS‑dependent and brittle. Prefer dirname:
-import { join } from "node:path";
+import { dirname, join } from "node:path";
...
-const parentDir = join(worktreePath, "..");
+const parentDir = dirname(worktreePath);- Command construction / injection risk
Both helpers build shell commands via string interpolation:
await execAsync(`git worktree add "${worktreePath}" -b "${branch}"`, { cwd: mainRepoPath });
await execAsync(`git worktree remove "${worktreePath}" --force`, { cwd: mainRepoPath });Even though branch is currently generated internally, worktreePath and mainRepoPath come from the filesystem and can contain characters (", &, ;, etc.) that break quoting or enable command injection.
It’s safer to avoid the shell entirely and use execFile (or spawn) with an argument array:
-import { exec } from "node:child_process";
+import { execFile } from "node:child_process";
import { promisify } from "node:util";
...
-const execAsync = promisify(exec);
+const execFileAsync = promisify(execFile);
...
- await execAsync(`git worktree add "${worktreePath}" -b "${branch}"`, {
- cwd: mainRepoPath,
- });
+ await execFileAsync("git", ["worktree", "add", worktreePath, "-b", branch], {
+ cwd: mainRepoPath,
+ });
...
- await execAsync(`git worktree remove "${worktreePath}" --force`, {
- cwd: mainRepoPath,
- });
+ await execFileAsync("git", ["worktree", "remove", worktreePath, "--force"], {
+ cwd: mainRepoPath,
+ });- Error wrapping
Wrapping the caught error with new Error(...) loses the original stack:
catch (error) {
console.error(`Failed to create worktree: ${error}`);
throw new Error(`Failed to create worktree: ${error}`);
}If you want a custom message, consider using cause to preserve context:
catch (error) {
console.error("Failed to create worktree", error);
throw new Error("Failed to create worktree", {
cause: error instanceof Error ? error : undefined,
});
}Same applies to removeWorktree.
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts around lines
62-103, replace brittle join(worktreePath, "..") with path.dirname(worktreePath)
when creating the parent directory, switch from constructing shell strings and
execAsync to a shell-free API (e.g., child_process.execFile or spawn with an
args array) to run git with arguments like ["worktree","add", worktreePath,
"-b", branch] and ["worktree","remove", worktreePath, "--force"] (keeping cwd:
mainRepoPath), and change the catch blocks to log the original error object and
rethrow using Error with the cause option (or rethrow the original error) so the
original stack is preserved.
| // Find the project | ||
| const project = db.data.projects.find((p) => p.id === input.projectId); | ||
| if (!project) { | ||
| throw new Error(`Project ${input.projectId} not found`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that project.mainRepoPath is a valid Git repository.
The code assumes project.mainRepoPath is a valid Git repository but doesn't validate this before calling createWorktree. Invalid paths could lead to cryptic Git errors.
Check if Git repository validation exists:
🏁 Script executed:
#!/bin/bash
# Description: Check if mainRepoPath is validated as a Git repository
# Look for Git validation in the projects module or git utilities
rg -nP --type=ts -C3 'isGitRepo|git.*rev-parse|\.git.*HEAD|validate.*git|check.*repository' apps/desktop/src/lib/trpc/routers/Length of output: 2152
Now I need to check the actual workspaces.ts file to see if createWorktree is called after finding the project:
🏁 Script executed:
#!/bin/bash
# Description: Check the full context of workspaces.ts around createWorktree
# First, let's find where createWorktree is called and what happens
rg -nP --type=ts -A20 'project.mainRepoPath' apps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsLength of output: 98
🏁 Script executed:
#!/bin/bash
# Description: Find the correct location of workspaces.ts
fd -type f -name "workspaces.ts" apps/desktop/Length of output: 170
🏁 Script executed:
#!/bin/bash
# Description: Find the correct location of workspaces.ts
fd "workspaces\.ts$" apps/desktop/Length of output: 178
🏁 Script executed:
#!/bin/bash
# Description: Check the context around lines 22-26 in workspaces.ts and see if createWorktree is called
# Get the content around the specified lines and further to find createWorktree
cat apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts | head -100 | tail -80Length of output: 1948
Add Git repository validation before calling createWorktree at line 37.
The code passes project.mainRepoPath to createWorktree without validating it's a valid Git repository. The isGitRepo function exists in the codebase and is already used in projects.ts (line 31) for the same purpose—apply the same validation here to prevent cryptic Git errors.
// Add this check before createWorktree
if (!(await isGitRepo(project.mainRepoPath))) {
throw new Error(`Project repository at ${project.mainRepoPath} is not a valid git repository`);
}🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts around lines 22 to
26 (and before the createWorktree call at ~line 37), the code uses
project.mainRepoPath without validating it as a git repo; add an await
isGitRepo(project.mainRepoPath) check and if it returns false throw an
Error(`Project repository at ${project.mainRepoPath} is not a valid git
repository`) so createWorktree only runs against valid repositories; ensure
isGitRepo is imported/available in this module and use the same pattern as in
projects.ts line 31.
| throw new Error(`Project ${input.projectId} not found`); | ||
| } | ||
|
|
||
| const branch = generateBranchName(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify branch name uniqueness to prevent conflicts.
The generated branch name could collide with an existing branch in the repository, causing createWorktree to fail at Line 37. While the random generation makes collisions unlikely, a concurrent creation or an existing branch could cause issues.
Consider verifying branch uniqueness before creating the worktree:
🏁 Script executed:
#!/bin/bash
# Description: Check if git.ts validates branch uniqueness before creation
# Search for branch existence validation in git utilities
rg -nP --type=ts -C3 'branch.*exist|git.*branch.*list|git.*show-ref' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find git utilities file and createWorktree function
echo "=== Searching for createWorktree function ==="
rg -nP --type=ts 'createWorktree' apps/desktop/src/lib/trpc/routers/workspaces/ -A5
echo -e "\n=== Searching for git utilities file ==="
fd -e ts 'git' apps/desktop/src/lib/trpc/routers/workspaces/
echo -e "\n=== Searching for generateBranchName function ==="
rg -nP --type=ts 'generateBranchName' apps/desktop/src/lib/trpc/routers/workspaces/ -A8Length of output: 3899
🏁 Script executed:
#!/bin/bash
# Get full implementation of generateBranchName and createWorktree
echo "=== Full generateBranchName implementation ==="
rg -nP --type=ts 'export function generateBranchName' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts -A25
echo -e "\n=== Full createWorktree implementation ==="
rg -nP --type=ts 'export async function createWorktree' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts -A30
echo -e "\n=== Check for branch validation in git.ts ==="
rg -nP --type=ts 'branch' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts | grep -i 'exist\|check\|valid\|ref'Length of output: 1513
🏁 Script executed:
#!/bin/bash
# Check error handling in create mutation - what happens if createWorktree fails?
echo "=== Create mutation error handling (lines 14-80) ==="
rg -nP --type=ts 'create: publicProcedure' apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts -A70 | head -80
echo -e "\n=== Check if there's try-catch or cleanup logic ==="
rg -nP --type=ts 'catch|finally|rollback' apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts -B2 -A2Length of output: 2227
Add branch validation and error handling in create mutation to prevent orphaned worktrees.
The generateBranchName() function has no validation against existing branches, and createWorktree() is called without error handling. If a branch collision occurs or the worktree creation fails partway through, the system can be left in an inconsistent state with orphaned Git worktrees.
Specific issues:
- Line 28:
generateBranchName()generates names without checking if they already exist in the repository - Line 37:
createWorktree()is called without a try-catch wrapper; if it fails after partial creation, there's no cleanup - Lines 37-76: Atomicity problem—if
createWorktree()succeeds butdb.update()fails, the Git worktree is orphaned
Recommendations:
- Add a check in
createWorktree()or before calling it to validate the branch doesn't already exist - Wrap the
createWorktree()call in a try-catch with cleanup logic on failure - Consider transactional semantics or ensure consistent error recovery (similar to the try-catch in the delete mutation at line 185)
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts around line 28
(and the create flow spanning ~37–76, with delete example at 185),
generateBranchName() is used without verifying the branch doesn't already exist
and createWorktree() is invoked without error handling, risking orphaned
worktrees if later DB updates fail. Before calling createWorktree(), query the
repo for existing branch names and loop/regenerate until you get a unique name
(or fail after N attempts); wrap createWorktree() in a try-catch, and on any
failure perform cleanup (remove the created worktree and delete the branch if
created), then rethrow or return an error; if createWorktree() succeeds but
db.update() fails, perform the same rollback cleanup to ensure atomicity (or
persist a transactional marker and run compensating cleanup), mirroring the
try-catch/cleanup pattern used in the delete mutation.
| const worktreePath = join( | ||
| project.mainRepoPath, | ||
| ".superset", | ||
| branch, | ||
| ); | ||
|
|
||
| // Create git worktree | ||
| await createWorktree(project.mainRepoPath, branch, worktreePath); |
There was a problem hiding this comment.
Critical atomicity issue: Git worktree created before database transaction.
The Git worktree is created at Line 37 before the database transaction (Line 66). If the database update fails after the worktree is created, an orphaned worktree will remain on the filesystem without corresponding database records, leading to resource leaks and inconsistent state.
Consider one of these approaches:
Option 1: Wrap in try-catch with rollback
+try {
// Create git worktree
await createWorktree(project.mainRepoPath, branch, worktreePath);
// Create worktree record
const worktree = {
id: nanoid(),
projectId: input.projectId,
path: worktreePath,
branch,
createdAt: Date.now(),
};
// ... rest of creation logic ...
// Save to database
await db.update((data) => {
data.worktrees.push(worktree);
data.workspaces.push(workspace);
data.settings.lastActiveWorkspaceId = workspace.id;
// Update project lastOpenedAt
const p = data.projects.find((p) => p.id === input.projectId);
if (p) {
p.lastOpenedAt = Date.now();
}
});
return workspace;
+} catch (error) {
+ // Rollback: remove the worktree if it was created
+ try {
+ await removeWorktree(project.mainRepoPath, worktreePath);
+ } catch (cleanupError) {
+ console.error("Failed to cleanup worktree after error:", cleanupError);
+ }
+ throw error;
+}Option 2: Validate before creating resources
Add pre-flight validation (check disk space, verify repo state, validate branch name) before creating the worktree, though this doesn't fully eliminate the atomicity issue.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts around lines 30 to
37 and the DB transaction at ~66, the Git worktree is created before the
database transaction which can leave an orphaned worktree if the DB update
fails; change the flow so the worktree creation is atomic with the DB update:
either (preferred) perform the DB transaction first and only create the worktree
after the transaction commits, or (if worktree must be created first) wrap
creation and DB update in a try/catch where on any DB failure you remove the
created worktree (cleanup) and rethrow; additionally add pre-flight validation
(branch name, repo state, disk space) before creating resources to reduce
failure likelihood.
| if (worktree && project) { | ||
| try { | ||
| await removeWorktree(project.mainRepoPath, worktree.path); | ||
| } catch (error) { | ||
| console.error("Failed to remove worktree:", error); | ||
| // Continue with database cleanup even if git operation fails | ||
| } | ||
| } |
There was a problem hiding this comment.
Git worktree removed with --force flag, discarding uncommitted changes without warning.
Line 184 calls removeWorktree, which uses git worktree remove --force (per the relevant code snippet). This forcibly deletes the worktree even with uncommitted changes, potentially causing data loss for users who haven't committed their work.
Consider one of these approaches:
Option 1: Check for uncommitted changes first
// Remove git worktree if it exists
if (worktree && project) {
try {
+ // Check for uncommitted changes
+ const { stdout } = await execAsync(
+ `git -C "${worktree.path}" status --porcelain`,
+ { cwd: project.mainRepoPath }
+ );
+ if (stdout.trim()) {
+ throw new Error(
+ `Cannot delete workspace: worktree has uncommitted changes at ${worktree.path}`
+ );
+ }
await removeWorktree(project.mainRepoPath, worktree.path);
} catch (error) {
console.error("Failed to remove worktree:", error);
- // Continue with database cleanup even if git operation fails
+ throw error; // Don't delete from database if worktree can't be removed
}
}Option 2: Add a force flag to the delete input
Allow callers to explicitly opt-in to force deletion:
.input(z.object({
id: z.string(),
force: z.boolean().optional()
}))🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts around lines
182–189, the code unconditionally calls removeWorktree which uses `git worktree
remove --force`, risking data loss; modify the handler to accept an optional
force boolean on the input (update the zod schema to .input(z.object({ id:
z.string(), force: z.boolean().optional() })) ), then before calling
removeWorktree, run a git status check for uncommitted changes in that worktree
(or use an existing helper) and if uncommitted changes exist and force is not
true, abort with a clear error to the caller; only call removeWorktree with
force when either no uncommitted changes are found or the input.force is true,
and ensure errors are returned/logged rather than silently proceeding.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts (1)
73-73: Consider error handling for the setActiveWorkspace mutation.Same issue as line 66—the mutation lacks error handling. Please add an
onErrorcallback to handle failures gracefully.Apply the same fix suggested for line 66.
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
62-65: Preserve original errors instead of re‑wrapping with interpolated strings.In
createWorktree,removeWorktree, andgetGitRoot, the catch blocks both log and then thrownew Error(\Failed...: ${error}`)ornew Error("Not a git repository...")`, which discards the original stack trace and error type, making debugging harder.Prefer either:
- logging the original error object and rethrowing it, or
- wrapping with
cause:} catch (error) { console.error("Failed to create worktree", error); throw new Error("Failed to create worktree", { cause: error instanceof Error ? error : undefined, }); }and similarly for
removeWorktree/getGitRoot.#!/bin/bash # Quick check: locate all custom Error constructions in git.ts to ensure # they’re updated consistently if you adopt the `cause` pattern. rg -n "Failed to (create|remove) worktree|Not a git repository" apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts -n -C2Also applies to: 77-80, 88-90
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
22-23: Consider handling loading and error states from the TRPC query.The query result doesn't extract
isLoadingorisError, soactiveWorkspacewill beundefinedduring initial load or if the query fails. While the component has null checks in place (lines 31-33, 48), the user experience could be improved by explicitly handling these states—for example, disabling interactions or showing a loading indicator when the workspace data is unavailable.Consider destructuring the query result to handle these states:
-const { data: activeWorkspace } = trpc.workspaces.getActive.useQuery(); +const { data: activeWorkspace, isLoading, isError } = trpc.workspaces.getActive.useQuery();Then conditionally disable or adjust UI based on
isLoadingorisError.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (1)
11-12: Consider handling loading and error states from the TRPC query.Similar to other components in this PR, the query doesn't extract
isLoadingorisError. While the component gracefully handlesundefinedby returning an empty tabs array (line 22) and disabling the "New Tab" button (line 68), users won't have visibility into whether the workspace is still loading or failed to load.Consider extracting loading/error states for better UX:
-const { data: activeWorkspace } = trpc.workspaces.getActive.useQuery(); +const { data: activeWorkspace, isLoading } = trpc.workspaces.getActive.useQuery();You could then show a loading skeleton or message when
isLoadingis true.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
15-16: Loading state causes EmptyTabView to display during workspace data fetch.When the workspace data is loading,
activeWorkspaceisundefined, causingtabToRenderto benull(line 21) and the component to renderEmptyTabView(lines 38-43). This may confuse users who expect to see a loading indicator rather than an empty state during initial load or refetch.Extract the loading state and handle it explicitly:
-const { data: activeWorkspace } = trpc.workspaces.getActive.useQuery(); +const { data: activeWorkspace, isLoading } = trpc.workspaces.getActive.useQuery(); const activeWorkspaceId = activeWorkspace?.id;Then conditionally render a loading state:
+if (isLoading) { + return ( + <div ref={attachDrop} className="flex-1 h-full flex items-center justify-center"> + <p>Loading workspace...</p> + </div> + ); +} + if (!tabToRender) { return ( <div ref={attachDrop} className="flex-1 h-full">apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts (1)
66-66: Consider error handling for the setActiveWorkspace mutation.The mutation is called with
.mutate({ id }), which is fire-and-forget and doesn't handle errors. If the workspace switch fails (e.g., due to a network issue or backend error), the user won't receive feedback, and the UI state may become inconsistent.Consider adding error handling to the mutation:
-setActiveWorkspace.mutate({ id: workspaces[index - 1].id }); +setActiveWorkspace.mutate( + { id: workspaces[index - 1].id }, + { + onError: (error) => { + console.error('Failed to switch workspace:', error); + // Optionally show a toast notification + } + } +);Apply the same pattern to line 73 as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/desktop/package.json(2 hunks)apps/desktop/src/lib/trpc/routers/projects/projects.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(1 hunks)apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts(4 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/stores/index.ts(0 hunks)apps/desktop/src/renderer/stores/workspaces.ts(0 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/stores/index.ts
- apps/desktop/src/renderer/stores/workspaces.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (4)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/main/lib/db/schemas.ts (1)
Project(9-15)apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
getGitRoot(83-91)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx (1)
apps/desktop/src/renderer/react-query/projects/useOpenNew.ts (1)
useOpenNew(7-22)
🔇 Additional comments (4)
apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts (1)
35-37: Good use of default value for workspaces query.Line 35 provides a sensible default empty array for workspaces, preventing potential issues during loading. However, the
activeWorkspacequery on line 36 doesn't extractisLoadingorisError. While keyboard shortcuts gracefully no-op whenactiveWorkspaceIdis undefined (checked in handlers), consider whether you want to provide user feedback if the active workspace fails to load.apps/desktop/package.json (1)
48-48: Both dependencies are secure—no active vulnerabilities found.Security verification confirms:
execa@9.6.0: No known vulnerabilitiessimple-git@3.30.0: All historical CVEs (command injection, RCE) are patched in versions ≤3.16.0, which are superseded by 3.30.0The additions align well with the PR objective. Ensure your Node.js version is compatible with both packages before deploying.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx (1)
41-85: Workspace dropdown behavior looks consistent with the new projects/workspaces flow.The controlled
DropdownMenustate, recents rendering, and mutation disabling logic (usingcreateWorkspace.isPending/openNew.isPending) align with the new TRPC APIs and avoid duplicate submits; nothing blocking here.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
12-16: Projects router logic and API shape look solid; verification confirms simple-git pattern is valid.
getRecentscorrectly returns projects sorted bylastOpenedAt, andopenNewcleanly normalizes the selected folder to a Git root usingrevparse(['--show-toplevel'])(which is supported in current simple-git releases and still recommended). It then upserts aProjectrecord (updatinglastOpenedAtor creating withcreatedAt) and returns{ success, project }as expected by the renderer. No blocking issues.
| const handleOpenNewProject = () => { | ||
| openNew.mutate(undefined, { | ||
| onSuccess: (result) => { | ||
| if (result.success && result.project) { | ||
| handleCreateWorkspace(result.project.id); | ||
| } | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Surface openNew failure to the user (non‑git folder case is silent).
openNew can return { success: false, error: "Selected folder is not in a git repository" }, but the onSuccess handler ignores the failure path, so choosing a non‑git folder results in no visible feedback and no workspace being created.
Consider handling the !result.success branch here (e.g., toast/dialog) so the user understands why nothing happened, and optionally closing the dropdown on failure as well.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx
around lines 31–39, the onSuccess handler for openNew only handles the success
path and ignores result.success === false, leaving the user without feedback
when they pick a non-git folder; update the handler to detect !result.success
and surface result.error (or a fallback message) to the user via the app's
toast/dialog system, optionally close the dropdown on failure, and also ensure
the mutate onError path surfaces errors similarly so failures are never silent.
saddlepaddle
force-pushed
the
create-worktrees-for-workspaces
branch
from
01db3ba to
01b2171
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx (1)
20-43: SurfaceopenNew/createWorkspacefailures to the user (avoid silent no‑ops)Right now both flows can fail silently from the user’s perspective:
handleOpenNewProjectonly handles the success branch (result.success && result.project); ifopenNewreturns{ success: false, error }(e.g., non‑git folder), nothing happens and the user gets no feedback.handleCreateWorkspaceonly closes the dropdown on success and does not surface errors if the mutation fails.To avoid “nothing happened” UX, consider:
- Detecting
!result.successinhandleOpenNewProjectand showingresult.error(or a generic fallback) via the app’s toast/dialog mechanism, and optionally closing the dropdown on failure as well.- Wiring
onErrorfor bothopenNewandcreateWorkspacemutations so network or server errors are also communicated to the user.Also applies to: 60-85
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (4)
22-26: Verify that project.mainRepoPath is a valid Git repository.The code assumes
project.mainRepoPathis a valid Git repository but doesn't validate this before callingcreateWorktreeat line 37. If the project's mainRepoPath has been moved, deleted, or corrupted since the project was created, this will lead to cryptic Git errors.Based on past review feedback, add validation using
getGitRoot:// Find the project const project = db.data.projects.find((p) => p.id === input.projectId); if (!project) { throw new Error(`Project ${input.projectId} not found`); } + + // Verify the project's mainRepoPath is still a valid git repository + try { + await getGitRoot(project.mainRepoPath); + } catch (error) { + throw new Error(`Project repository at ${project.mainRepoPath} is not a valid git repository`); + }
28-37: Critical atomicity and branch validation issues.This code has two critical issues:
No branch uniqueness validation:
generateBranchName()at line 28 doesn't check if the branch already exists, risking collision andcreateWorktreefailure.Atomicity violation: The Git worktree is created at line 37 before the database transaction at line 66. If the database update fails, an orphaned worktree remains on the filesystem.
Apply this comprehensive fix:
const branch = generateBranchName(); + + // Validate branch uniqueness + const git = simpleGit(project.mainRepoPath); + const branches = await git.branchLocal(); + let attempts = 0; + let uniqueBranch = branch; + while (branches.all.includes(uniqueBranch) && attempts < 10) { + uniqueBranch = generateBranchName(); + attempts++; + } + if (branches.all.includes(uniqueBranch)) { + throw new Error("Failed to generate unique branch name after 10 attempts"); + } + const branch = uniqueBranch; const worktreePath = join( project.mainRepoPath, ".superset", branch, ); - // Create git worktree - await createWorktree(project.mainRepoPath, branch, worktreePath); - // Create worktree record const worktree = { id: nanoid(), projectId: input.projectId, path: worktreePath, branch, createdAt: Date.now(), }; // ... workspace creation ... - // Save to database - await db.update((data) => { - data.worktrees.push(worktree); - data.workspaces.push(workspace); - data.settings.lastActiveWorkspaceId = workspace.id; - - // Update project lastOpenedAt - const p = data.projects.find((p) => p.id === input.projectId); - if (p) { - p.lastOpenedAt = Date.now(); - } - }); + // Atomic operation: try to create worktree and update DB together + try { + // Save to database first + await db.update((data) => { + data.worktrees.push(worktree); + data.workspaces.push(workspace); + data.settings.lastActiveWorkspaceId = workspace.id; + + // Update project lastOpenedAt + const p = data.projects.find((p) => p.id === input.projectId); + if (p) { + p.lastOpenedAt = Date.now(); + } + }); + + // Create git worktree after DB commit + await createWorktree(project.mainRepoPath, branch, worktreePath); + } catch (error) { + // Rollback DB if worktree creation failed + await db.update((data) => { + data.worktrees = data.worktrees.filter((wt) => wt.id !== worktree.id); + data.workspaces = data.workspaces.filter((w) => w.id !== workspace.id); + }); + throw error; + } return workspace;
182-189: Git worktree removed with --force flag, risking data loss.Line 184 calls
removeWorktree, which usesgit worktree remove --force(per the relevant code snippet). This forcibly deletes the worktree even with uncommitted changes, potentially causing data loss for users who haven't committed their work.Based on past review feedback, add uncommitted changes validation:
// Remove git worktree if it exists if (worktree && project) { try { + // Check for uncommitted changes first + const git = simpleGit(worktree.path); + const status = await git.status(); + if (!status.isClean()) { + throw new Error( + `Cannot delete workspace: worktree has uncommitted changes at ${worktree.path}` + ); + } await removeWorktree(project.mainRepoPath, worktree.path); } catch (error) { console.error("Failed to remove worktree:", error); - // Continue with database cleanup even if git operation fails + throw error; // Don't delete from database if worktree can't be removed } }Alternatively, add a
force: z.boolean().optional()flag to the input schema to let users explicitly opt-in to force deletion.
197-201: Inconsistent error handling: Git operation can fail but DB cleanup continues.Lines 186-188 catch and log Git worktree removal errors but continue with database cleanup. This creates an inconsistency where the database shows the workspace as deleted, but the Git worktree still exists on the filesystem.
This contradicts the comment at line 187 which says "Continue with database cleanup even if git operation fails" but actually makes the problem worse—users will lose the database reference to the worktree, making manual cleanup harder.
Consider one of these approaches:
Option 1 (Recommended): Fail fast and preserve consistency
} catch (error) { console.error("Failed to remove worktree:", error); - // Continue with database cleanup even if git operation fails + throw error; // Preserve DB reference if git cleanup fails }Option 2: Add a "dangling worktree" flag in the database
If the Git operation fails, mark the worktree as dangling in the database rather than removing it, allowing users to retry cleanup later.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
33-45: Consider guarding the width calc for 0 workspaces and documentingADD_BUTTON_WIDTHThe new layout and
no-dragusage aroundWorkspaceItemandWorkspaceDropdownlook good. A couple of small robustness/readability tweaks you might consider:
- In
updateWorkspaceWidth, whenworkspaces.length === 0the expressionavailableWidth / workspaces.lengthbecomesInfinityand then gets clamped toMAX_WORKSPACE_WIDTH. It works today but is non-obvious; an early return likeif (!workspaces.length) { setWorkspaceWidth(MAX_WORKSPACE_WIDTH); return; }would make the intent clearer.
ADD_BUTTON_WIDTHis now effectively describing the width ofWorkspaceDropdown’s trigger. A brief comment (or deriving it from an actual measured ref if this ever drifts) would help keep the width logic in sync with future design tweaks.Also applies to: 70-71, 88-88, 120-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
apps/desktop/package.json(2 hunks)apps/desktop/src/lib/trpc/routers/projects/projects.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(5 hunks)apps/desktop/src/main/lib/db/schemas.ts(1 hunks)apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts(4 hunks)apps/desktop/src/renderer/react-query/projects/index.ts(1 hunks)apps/desktop/src/renderer/react-query/projects/useOpenNew.ts(1 hunks)apps/desktop/src/renderer/react-query/projects/useOpenProject.ts(0 hunks)apps/desktop/src/renderer/react-query/projects/useOpenRecent.ts(0 hunks)apps/desktop/src/renderer/react-query/projects/useRemoveRecent.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/AddWorkspaceButton.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/RecentProjectItem.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/RecentSection.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/StartSection/StartSection.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/StartSection/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(0 hunks)apps/desktop/src/renderer/stores/index.ts(0 hunks)apps/desktop/src/renderer/stores/workspaces.ts(0 hunks)
💤 Files with no reviewable changes (14)
- apps/desktop/src/renderer/stores/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/StartSection/StartSection.tsx
- apps/desktop/src/renderer/react-query/projects/useOpenProject.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/index.ts
- apps/desktop/src/renderer/react-query/projects/useOpenRecent.ts
- apps/desktop/src/renderer/react-query/projects/useRemoveRecent.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/index.ts
- apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/AddWorkspaceButton.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/RecentProjectItem.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/StartSection/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView/components/RecentSection/RecentSection.tsx
- apps/desktop/src/renderer/stores/workspaces.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/package.json
- apps/desktop/src/renderer/react-query/projects/useOpenNew.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
- apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
- apps/desktop/src/renderer/react-query/projects/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (3)
apps/desktop/src/main/lib/db/schemas.ts (1)
Project(9-15)apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
getGitRoot(83-91)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (3)
generateBranchName(5-47)createWorktree(49-66)removeWorktree(68-81)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx (1)
apps/desktop/src/renderer/react-query/projects/useOpenNew.ts (1)
useOpenNew(7-22)
apps/desktop/src/main/lib/db/schemas.ts (1)
apps/desktop/src/shared/types.ts (3)
Worktree(58-68)Workspace(70-82)Tab(44-56)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx (1)
WorkspaceDropdown(17-90)
🔇 Additional comments (14)
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (1)
20-24: Verify WindowControls remain non-draggable after removingno-dragfrom wrapperRemoving
no-dragfrom the right container makes that whole area part of the draggable region unless children explicitly applyno-drag. Please double-check that the root element insideWindowControlshasno-drag(or equivalent-webkit-app-region: no-drag) so the window buttons stay fully clickable on non‑Mac platforms.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
7-8: Switch to TRPC-sourcedactiveWorkspacelooks consistent with existing tab logicUsing
trpc.workspaces.getActive.useQuery()and derivingactiveWorkspaceIdfrom the result keeps thetabToRendermemoization and early-return behavior intact. The component still safely handles “no active workspace” and missing-tab cases, so this migration looks good.Also applies to: 15-17, 20-35
apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts (1)
18-20: TRPC-based workspace data + mutation wiring keeps shortcut behavior intactUsing
trpc.workspaces.getAll/getActiveplususeSetActiveWorkspace().mutate({ id })preserves the previous semantics for prev/next workspace switching and tab navigation: handlers are safely no‑ops without an active workspace, index helpers guard unknown IDs, and the keyboard listener is correctly registered/cleaned up inside the effect. This refactor looks solid.Also applies to: 35-38, 47-48, 60-75, 149-161
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
12-16: LGTM!The
getRecentsquery correctly returns projects sorted by most recent access. The use of.slice()prevents mutation of the original array.apps/desktop/src/main/lib/db/schemas.ts (5)
6-15: LGTM!The
Projectinterface is well-documented and uses appropriate types. The use of nanoid for IDs and timestamps for temporal tracking is consistent with the codebase patterns.
17-26: LGTM!The
Worktreeinterface correctly models a Git worktree with references to its parent project. The comment clarifying thatbranchis the "source of truth for git operations" is helpful for maintainers.
28-40: LGTM!The
Workspaceinterface correctly establishes a 1:1 relationship withWorktreewhile maintaining its own UI-specific properties likenameandorder. The inclusion of temporal tracking fields is appropriate.
55-70: LGTM!The
Databaseinterface anddefaultDatabaseinitialization correctly reflect the new data model withprojectsandworktreesarrays. The removal ofrecentProjectsaligns with the PR's objective to replace recents with a projects/worktrees model.
42-49: I need to verify whether the Tab interface in schemas.ts is actually exported or used elsewhere:Based on my verification of the script output, the review comment identifying a conflict between two
Tabinterface definitions is incorrect. Here's why:In TypeScript, two different modules will never contribute names to the same scope. The two
Tabinterfaces exist in completely separate ES6 modules:
apps/desktop/src/shared/types.ts- The canonical Tab interface used throughout the application (imported by renderer code, shared IPC channels, and stores)apps/desktop/src/main/lib/db/schemas.ts- A database schema Tab interface that is only defined locally and not imported anywhere in the codebaseThe search results show zero imports of
Tabfromschemas.ts. The database schema'sTabinterface is isolated to its own module and does not create type confusion or runtime errors because they exist in separate scopes with no cross-references.This is proper architectural design: database schemas are separated from runtime application types. No changes are needed.
Likely an incorrect or invalid review comment.
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (5)
85-93: LGTM!The
getquery correctly throws an error when a workspace is not found, providing clear error messaging. This is appropriate for cases where the caller expects the workspace to exist.
109-126: LGTM!The
getActivequery handles both valid states (no active workspace set → returns null) and invalid states (active ID exists but workspace missing → throws error). The distinction between these cases is well-documented and appropriate.
132-159: LGTM!The
updatemutation correctly implements partial updates, properly validates workspace existence, and maintains temporal tracking withupdatedAtandlastOpenedAttimestamps.
219-234: LGTM!The
setActivemutation correctly validates workspace existence before setting it as active, and properly updates both the active workspace ID and temporal tracking fields.
239-269: LGTM!The
reordermutation correctly implements drag-and-drop reordering by splicing the array and updating order fields. The use of.slice()prevents mutation of the source array during sorting.
| let project = db.data.projects.find( | ||
| (p) => p.mainRepoPath === mainRepoPath, | ||
| ); | ||
|
|
||
| if (project) { | ||
| await db.update((data) => { | ||
| const recent = data.recentProjects.find((p) => p.path === path); | ||
| if (recent) { | ||
| recent.lastOpenedAt = Date.now(); | ||
| const p = data.projects.find((p) => p.id === project?.id); | ||
| if (p) { | ||
| p.lastOpenedAt = Date.now(); | ||
| } | ||
| }); | ||
|
|
||
| return { | ||
| success: true as const, | ||
| path, | ||
| } else { | ||
| project = { | ||
| id: nanoid(), | ||
| mainRepoPath, | ||
| name, | ||
| lastOpenedAt: Date.now(), | ||
| createdAt: Date.now(), | ||
| }; | ||
| }), | ||
| getRecents: publicProcedure.query((): RecentProject[] => { | ||
| return db.data.recentProjects | ||
| .slice() | ||
| .sort((a, b) => b.lastOpenedAt - a.lastOpenedAt); | ||
| }), | ||
| removeRecent: publicProcedure | ||
| .input(z.object({ path: z.string() })) | ||
| .mutation(async ({ input }) => { | ||
|
|
||
| await db.update((data) => { | ||
| data.recentProjects = data.recentProjects.filter( | ||
| (p) => p.path !== input.path, | ||
| ); | ||
| data.projects.push(project!); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential race condition between project lookup and database update.
The code finds a project at lines 42-44, then later updates it inside db.update() at lines 47-52 by searching again by project?.id. Between the initial lookup and the database update, the project could theoretically be deleted by another operation, making the second lookup fail silently.
Additionally, at line 63, the non-null assertion project! assumes the if/else logic is exhaustive, but if there's a logic error, this could cause a runtime error.
Apply this diff to make the code more defensive:
if (project) {
await db.update((data) => {
const p = data.projects.find((p) => p.id === project?.id);
if (p) {
p.lastOpenedAt = Date.now();
}
});
+ // Refresh project reference after update
+ project = db.data.projects.find((p) => p.id === project?.id) ?? project;
} else {
project = {
id: nanoid(),
mainRepoPath,
name,
lastOpenedAt: Date.now(),
createdAt: Date.now(),
};
await db.update((data) => {
data.projects.push(project!);
});
}
return {
success: true as const,
- project,
+ project: project!,
};📝 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.
| let project = db.data.projects.find( | |
| (p) => p.mainRepoPath === mainRepoPath, | |
| ); | |
| if (project) { | |
| await db.update((data) => { | |
| const recent = data.recentProjects.find((p) => p.path === path); | |
| if (recent) { | |
| recent.lastOpenedAt = Date.now(); | |
| const p = data.projects.find((p) => p.id === project?.id); | |
| if (p) { | |
| p.lastOpenedAt = Date.now(); | |
| } | |
| }); | |
| return { | |
| success: true as const, | |
| path, | |
| } else { | |
| project = { | |
| id: nanoid(), | |
| mainRepoPath, | |
| name, | |
| lastOpenedAt: Date.now(), | |
| createdAt: Date.now(), | |
| }; | |
| }), | |
| getRecents: publicProcedure.query((): RecentProject[] => { | |
| return db.data.recentProjects | |
| .slice() | |
| .sort((a, b) => b.lastOpenedAt - a.lastOpenedAt); | |
| }), | |
| removeRecent: publicProcedure | |
| .input(z.object({ path: z.string() })) | |
| .mutation(async ({ input }) => { | |
| await db.update((data) => { | |
| data.recentProjects = data.recentProjects.filter( | |
| (p) => p.path !== input.path, | |
| ); | |
| data.projects.push(project!); | |
| }); | |
| } | |
| let project = db.data.projects.find( | |
| (p) => p.mainRepoPath === mainRepoPath, | |
| ); | |
| if (project) { | |
| await db.update((data) => { | |
| const p = data.projects.find((p) => p.id === project?.id); | |
| if (p) { | |
| p.lastOpenedAt = Date.now(); | |
| } | |
| }); | |
| // Refresh project reference after update | |
| project = db.data.projects.find((p) => p.id === project?.id) ?? project; | |
| } else { | |
| project = { | |
| id: nanoid(), | |
| mainRepoPath, | |
| name, | |
| lastOpenedAt: Date.now(), | |
| createdAt: Date.now(), | |
| }; | |
| await db.update((data) => { | |
| data.projects.push(project!); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/projects/projects.ts around lines 42 to 65,
avoid the race between the initial find and the later db.update and remove the
unsafe non-null assertion by making the create-or-update atomic inside
db.update: call db.update once and inside its callback search for the project by
mainRepoPath; if found set its lastOpenedAt = Date.now(); if not, construct a
new project object (with id = nanoid(), mainRepoPath, name, lastOpenedAt and
createdAt timestamps) and push it into data.projects; this ensures the lookup
and mutation happen under the same lock and eliminates the project! non-null
assertion.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Improvements
Removals
✏️ Tip: You can customize this high-level summary in your review settings.