feat: v2 projects/workspaces/devices schema, cloud routers, and host-service workspace router#2373
Merged
Merged
Conversation
…-workspace-router
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces a V2 project and workspace system across the backend, API, and desktop layers. It adds new database tables with relationships, establishes host-service database infrastructure with SQLite migrations, implements TRPC routers for V2 operations, and integrates the system into the desktop UI and collections provider. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Update better-sqlite3 to 12.6.2, drizzle-orm to 0.45.1, and drizzle-kit to 0.31.8 to match the rest of the monorepo and pass sherif's multiple-dependency-versions check.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
9 issues found across 33 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/src/schema/v2.ts">
<violation number="1" location="packages/db/src/schema/v2.ts:68">
P1: Add tenant-consistency foreign key constraints so a workspace cannot reference a project/device from another organization.</violation>
</file>
<file name="packages/host-service/src/trpc/router/workspace/workspace.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace/workspace.ts:76">
P2: Handle cloud-create failures with rollback of the newly created worktree; otherwise failed cloud requests leave orphaned local worktrees.</violation>
<violation number="2" location="packages/host-service/src/trpc/router/workspace/workspace.ts:122">
P2: Don't swallow errors in the worktree-removal catch block; log a warning with context so cleanup failures are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
<violation number="3" location="packages/host-service/src/trpc/router/workspace/workspace.ts:129">
P2: Cloud deletion is ordered after local worktree removal. If cloud delete fails, local metadata can point to a removed worktree.</violation>
</file>
<file name="packages/host-service/src/trpc/router/project/project.ts">
<violation number="1" location="packages/host-service/src/trpc/router/project/project.ts:31">
P2: Don't silently ignore repository deletion failures; emit a warning so partial cleanup can be observed and investigated.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/host-service/src/serve.ts">
<violation number="1" location="packages/host-service/src/serve.ts:4">
P2: Normalize `HOST_DB_PATH` before passing it so empty/whitespace values fall back to the default DB path.</violation>
</file>
<file name="packages/trpc/src/router/v2-project/v2-project.ts">
<violation number="1" location="packages/trpc/src/router/v2-project/v2-project.ts:65">
P1: Validate `githubRepositoryId` belongs to the active organization before create/update. Without that check, projects can be linked to repositories from other organizations.</violation>
</file>
<file name="packages/trpc/src/router/v2-workspace/v2-workspace.ts">
<violation number="1" location="packages/trpc/src/router/v2-workspace/v2-workspace.ts:33">
P1: Validate `projectId` and `deviceId` ownership against the active organization before insert/update to prevent cross-org workspace links.</violation>
</file>
<file name="packages/host-service/src/db/db.ts">
<violation number="1" location="packages/host-service/src/db/db.ts:22">
P1: Migration folder resolution is gated by `ELECTRON_RUN_AS_NODE`, which can make packaged host-service instances use a non-existent dev migrations path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/trpc/router/workspace/workspace.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace/workspace.ts:116">
P2: Avoid using an empty catch block here; log a warning with context so failed worktree cleanup is observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/host-service/src/trpc/router/project/project.ts">
<violation number="1" location="packages/host-service/src/trpc/router/project/project.ts:29">
P2: Avoid empty catch blocks when removing worktrees; log the failure context so cleanup issues are observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
packages/host-service/src/db/schema.ts (1)
3-13: Consider adding a unique constraint onrepoPath.If a local repository should only be registered as one project, adding
.unique()torepoPathwould prevent accidental duplicate registrations and simplify lookup logic.- repoPath: text("repo_path").notNull(), + repoPath: text("repo_path").notNull().unique(),If duplicates are intentional (e.g., multiple cloud projects pointing to the same local repo), disregard this suggestion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/db/schema.ts` around lines 3 - 13, Add a uniqueness constraint to the repoPath column in the projects table by updating the sqliteTable definition for projects: modify the repoPath column definition (repoPath: text("repo_path").notNull()) to call .unique(), so repoPath becomes unique and the DB prevents duplicate local-repo registrations; keep the existing index on table.repoPath (index("projects_repo_path_idx")) or remove it if redundant after adding unique(), and ensure any code that inserts projects (calls interacting with projects table) handles uniqueness violations.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/HostServiceStatus/HostServiceStatus.tsx (1)
253-271: Consider guarding against undefinedserviceinside the async handler.While the button is disabled when
!service, the handler uses optional chaining (service?.client) which could result inresultbeingundefinedif the disabled state is somehow bypassed. Consider adding an early return guard.🛡️ Defensive guard suggestion
onClick={async () => { + if (!service) return; setV2Loading(true); setV2Error(null); setV2Result(null); try { const result = - await service?.client.workspace.create.mutate({ + await service.client.workspace.create.mutate({ projectId: v2ProjectId, name: `workspace-${v2Branch}`, branch: v2Branch, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/HostServiceStatus/HostServiceStatus.tsx` around lines 253 - 271, Add a defensive early-return at the top of the async onClick handler to guard against undefined service: check the service variable used in service.client.workspace.create.mutate and if absent, set V2 state appropriately (e.g., setV2Loading(false); setV2Error("Service unavailable");) and return before attempting the mutate; keep the existing state updates (setV2Loading, setV2Error, setV2Result, setV2WorkspaceId) unchanged for the success/error/finally paths and reference service.client.workspace.create.mutate, v2ProjectId and v2Branch when locating where to add the guard.packages/host-service/src/db/db.ts (1)
43-64: Consider re-throwing migration errors to fail fast.Currently, migration failures are logged but swallowed, allowing the app to continue with a potentially inconsistent database state. This could lead to subtle runtime errors that are harder to debug.
♻️ Proposed fix to re-throw migration errors
try { migrate(db, { migrationsFolder }); } catch (error) { console.error("[host-service:db] Migration failed:", error); + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/db/db.ts` around lines 43 - 64, The createDb function currently swallows migration errors in the try/catch around migrate(db, { migrationsFolder }); update it to surface failures so the process fails fast: either remove the try/catch so migrate can throw, or re-throw the caught error after logging (e.g., in the catch block log via console.error with context and then throw error). This change should be applied in createDb to ensure migration failures are not silently ignored.packages/trpc/src/router/v2-project/v2-project.ts (1)
96-107: Update mutation silently returns undefined for non-existent projects.If the project doesn't exist or belongs to a different organization, the update returns
undefinedwithout throwing an error. This makes it harder for clients to distinguish between "updated successfully" and "project not found".🛡️ Proposed fix to throw NOT_FOUND
const [updated] = await dbWs .update(v2Projects) .set(data) .where( and( eq(v2Projects.id, id), eq(v2Projects.organizationId, organizationId), ), ) .returning(); + if (!updated) { + throw new TRPCError({ + code: "NOT_FOUND", + message: "Project not found", + }); + } return updated;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/v2-project/v2-project.ts` around lines 96 - 107, The update mutation currently returns the first element from dbWs.update(...).returning() which can be undefined when no row matches (non-existent project or wrong organization); modify the resolver that calls dbWs.update on v2Projects (the block producing const [updated]) to check if updated is undefined and if so throw a TRPCError with code "NOT_FOUND" (include a clear message like "Project not found"); otherwise return updated as before. Ensure you reference the same dbWs.update(...).set(data).where(...).returning() flow and add the conditional check and throw before the final return.packages/host-service/src/trpc/router/project/project.ts (1)
26-34: Optimize git instance creation by reusing it across worktree removals.Creating a new git instance for each workspace is unnecessary since all worktrees belong to the same repository. The git instance can be created once outside the loop.
♻️ Proposed optimization
// Best-effort remove each worktree + const git = localWorkspaces.length > 0 + ? await ctx.git(localProject.repoPath) + : null; for (const ws of localWorkspaces) { try { - const git = await ctx.git(localProject.repoPath); - await git.raw(["worktree", "remove", ws.worktreePath]); + await git?.raw(["worktree", "remove", ws.worktreePath]); } catch { // Best-effort } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/project/project.ts` around lines 26 - 34, The loop creating a new git instance per workspace is wasteful; call ctx.git(localProject.repoPath) once before the for loop and reuse that git object when calling git.raw(["worktree", "remove", ws.worktreePath"]) inside the loop (the symbols to update are ctx.git, localProject.repoPath, localWorkspaces, and the worktree removal block). Ensure you still wrap individual removals in try/catch for "best-effort" behavior and do not change the existing error-handling semantics.packages/host-service/src/trpc/router/workspace/workspace.ts (1)
83-93: Consider handling null cloudRow as an error.If
ctx.api.v2Workspace.create.mutate()returns null/undefined (line 76), the local workspace is not created but the function still returnscloudRow(which would be null). This could lead to inconsistent state where the worktree exists locally but has no tracking record.🛡️ Proposed defensive check
// Create cloud workspace (orgId implicit from auth session) const cloudRow = await ctx.api.v2Workspace.create.mutate({ projectId: input.projectId, name: input.name, branch: input.branch, }); + if (!cloudRow) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Failed to create cloud workspace", + }); + } + // Track locally - if (cloudRow) { - ctx.db - .insert(workspaces) - .values({ - id: cloudRow.id, - projectId: input.projectId, - worktreePath, - branch: input.branch, - }) - .run(); - } + ctx.db + .insert(workspaces) + .values({ + id: cloudRow.id, + projectId: input.projectId, + worktreePath, + branch: input.branch, + }) + .run(); return cloudRow;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace/workspace.ts` around lines 83 - 93, The code calls ctx.api.v2Workspace.create.mutate() and proceeds even if cloudRow is null, risking inconsistent state; add a defensive check after the call to cloudRow and if it is null/undefined, throw a descriptive error (or perform rollback/cleanup of the created worktree) instead of continuing, ensuring you do not run the local insert into the workspaces table or return a null cloudRow; update the block around cloudRow (the result of ctx.api.v2Workspace.create.mutate) and the subsequent insert into workspaces to early-fail or clean up when cloudRow is falsy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/electron-builder.ts`:
- Around line 63-68: The packaged app still points HOST_MIGRATIONS_PATH at the
dev folder, so migrations copied by electron-builder to
resources/host-migrations are not found; update the code that defines
HOST_MIGRATIONS_PATH in apps/desktop/src/main/lib/host-service-manager.ts to
conditionally use the packaged resources path when running a built app (e.g. if
app.isPackaged or process.resourcesPath is present) by setting
HOST_MIGRATIONS_PATH = path.join(process.resourcesPath, "host-migrations") (or
path.join(__dirname, "resources/host-migrations") if you prefer) and fall back
to the existing development path
(__dirname/../../../packages/host-service/drizzle) otherwise; ensure the spawn
invocation that uses HOST_MIGRATIONS_PATH continues to reference this constant
so the child process finds the copied files in packaged builds.
In `@apps/desktop/src/main/lib/host-service-manager.ts`:
- Around line 74-77: HOST_MIGRATIONS_PATH currently points to the repo checkout,
causing packaged runs to miss the copied runtime migrations; change the env var
assignment so HOST_MIGRATIONS_PATH resolves to the runtime/copied directory (the
same location apps/desktop/vite/helpers.ts copies to, e.g.
path.join(process.resourcesPath, "host-migrations")) when running as packaged or
when ELECTRON_RUN_AS_NODE=1 — update the code that sets HOST_MIGRATIONS_PATH to
prefer path.join(process.resourcesPath, "host-migrations") (falling back to the
repo checkout only if that runtime path is missing) so
packages/host-service/src/db/db.ts will load the correct migrations at runtime.
In `@packages/db/src/schema/v2.ts`:
- Around line 12-99: The schema additions for v2Projects, v2Devices, and
v2Workspaces lack a generated Drizzle migration; run the Drizzle migration
generator after saving these schema changes (e.g. run bunx drizzle-kit generate
--name='v2_projects_workspaces_devices'), verify the generated migration
includes creation of tables for v2_projects, v2_devices, and v2_workspaces
(matching the v2Projects, v2Devices, v2Workspaces definitions), add and commit
the generated migration file to the PR, and ensure the migration is included
before merging.
In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts`:
- Around line 20-39: The mutation inserts workspace rows without ensuring
input.projectId and input.deviceId belong to the active organization, so load
and validate those resources under organizationId before persisting: in the
create mutation (and the similar update code around lines 52-72) query the
projects and devices by their IDs and organizationId (e.g., fetch project by id
and organizationId, fetch device by id and organizationId) and if either is
missing or the organizationId mismatches throw a TRPCError (BAD_REQUEST or
FORBIDDEN) instead of inserting; keep the existing verifyOrgMembership call but
add these explicit tenant-scoped checks prior to the dbWs.insert / update on
v2Workspaces.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/HostServiceStatus/HostServiceStatus.tsx`:
- Around line 253-271: Add a defensive early-return at the top of the async
onClick handler to guard against undefined service: check the service variable
used in service.client.workspace.create.mutate and if absent, set V2 state
appropriately (e.g., setV2Loading(false); setV2Error("Service unavailable");)
and return before attempting the mutate; keep the existing state updates
(setV2Loading, setV2Error, setV2Result, setV2WorkspaceId) unchanged for the
success/error/finally paths and reference
service.client.workspace.create.mutate, v2ProjectId and v2Branch when locating
where to add the guard.
In `@packages/host-service/src/db/db.ts`:
- Around line 43-64: The createDb function currently swallows migration errors
in the try/catch around migrate(db, { migrationsFolder }); update it to surface
failures so the process fails fast: either remove the try/catch so migrate can
throw, or re-throw the caught error after logging (e.g., in the catch block log
via console.error with context and then throw error). This change should be
applied in createDb to ensure migration failures are not silently ignored.
In `@packages/host-service/src/db/schema.ts`:
- Around line 3-13: Add a uniqueness constraint to the repoPath column in the
projects table by updating the sqliteTable definition for projects: modify the
repoPath column definition (repoPath: text("repo_path").notNull()) to call
.unique(), so repoPath becomes unique and the DB prevents duplicate local-repo
registrations; keep the existing index on table.repoPath
(index("projects_repo_path_idx")) or remove it if redundant after adding
unique(), and ensure any code that inserts projects (calls interacting with
projects table) handles uniqueness violations.
In `@packages/host-service/src/trpc/router/project/project.ts`:
- Around line 26-34: The loop creating a new git instance per workspace is
wasteful; call ctx.git(localProject.repoPath) once before the for loop and reuse
that git object when calling git.raw(["worktree", "remove", ws.worktreePath"])
inside the loop (the symbols to update are ctx.git, localProject.repoPath,
localWorkspaces, and the worktree removal block). Ensure you still wrap
individual removals in try/catch for "best-effort" behavior and do not change
the existing error-handling semantics.
In `@packages/host-service/src/trpc/router/workspace/workspace.ts`:
- Around line 83-93: The code calls ctx.api.v2Workspace.create.mutate() and
proceeds even if cloudRow is null, risking inconsistent state; add a defensive
check after the call to cloudRow and if it is null/undefined, throw a
descriptive error (or perform rollback/cleanup of the created worktree) instead
of continuing, ensuring you do not run the local insert into the workspaces
table or return a null cloudRow; update the block around cloudRow (the result of
ctx.api.v2Workspace.create.mutate) and the subsequent insert into workspaces to
early-fail or clean up when cloudRow is falsy.
In `@packages/trpc/src/router/v2-project/v2-project.ts`:
- Around line 96-107: The update mutation currently returns the first element
from dbWs.update(...).returning() which can be undefined when no row matches
(non-existent project or wrong organization); modify the resolver that calls
dbWs.update on v2Projects (the block producing const [updated]) to check if
updated is undefined and if so throw a TRPCError with code "NOT_FOUND" (include
a clear message like "Project not found"); otherwise return updated as before.
Ensure you reference the same dbWs.update(...).set(data).where(...).returning()
flow and add the conditional check and throw before the final return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1865abbf-7475-4e26-8535-c309d661c5b7
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
apps/desktop/electron-builder.tsapps/desktop/src/main/host-service/index.tsapps/desktop/src/main/lib/host-service-manager.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/HostServiceStatus/HostServiceStatus.tsxapps/desktop/vite/helpers.tspackages/db/src/schema/index.tspackages/db/src/schema/relations.tspackages/db/src/schema/v2.tspackages/host-service/drizzle.config.tspackages/host-service/drizzle/0000_initial_projects_workspaces.sqlpackages/host-service/drizzle/meta/0000_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/package.jsonpackages/host-service/src/app.tspackages/host-service/src/db/db.tspackages/host-service/src/db/index.tspackages/host-service/src/db/schema.tspackages/host-service/src/index.tspackages/host-service/src/serve.tspackages/host-service/src/trpc/context/context.tspackages/host-service/src/trpc/router/project/index.tspackages/host-service/src/trpc/router/project/project.tspackages/host-service/src/trpc/router/router.tspackages/host-service/src/trpc/router/workspace/index.tspackages/host-service/src/trpc/router/workspace/workspace.tspackages/host-service/src/types.tspackages/trpc/src/root.tspackages/trpc/src/router/v2-project/index.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/index.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
Comment on lines
+12
to
+99
| export const v2Projects = pgTable( | ||
| "v2_projects", | ||
| { | ||
| id: uuid().primaryKey().defaultRandom(), | ||
| organizationId: uuid("organization_id") | ||
| .notNull() | ||
| .references(() => organizations.id, { onDelete: "cascade" }), | ||
| name: text().notNull(), | ||
| slug: text().notNull(), | ||
| githubRepositoryId: uuid("github_repository_id").references( | ||
| () => githubRepositories.id, | ||
| { onDelete: "set null" }, | ||
| ), | ||
| createdAt: timestamp("created_at").notNull().defaultNow(), | ||
| updatedAt: timestamp("updated_at") | ||
| .notNull() | ||
| .defaultNow() | ||
| .$onUpdate(() => new Date()), | ||
| }, | ||
| (table) => [ | ||
| index("v2_projects_organization_id_idx").on(table.organizationId), | ||
| unique("v2_projects_org_slug_unique").on(table.organizationId, table.slug), | ||
| ], | ||
| ); | ||
|
|
||
| export type InsertV2Project = typeof v2Projects.$inferInsert; | ||
| export type SelectV2Project = typeof v2Projects.$inferSelect; | ||
|
|
||
| export const v2Devices = pgTable( | ||
| "v2_devices", | ||
| { | ||
| id: uuid().primaryKey().defaultRandom(), | ||
| organizationId: uuid("organization_id") | ||
| .notNull() | ||
| .references(() => organizations.id, { onDelete: "cascade" }), | ||
| name: text().notNull(), | ||
| type: text().notNull(), // "host" | "cloud" | "viewer" | ||
| hashedDeviceId: text("hashed_device_id").notNull(), | ||
| createdAt: timestamp("created_at").notNull().defaultNow(), | ||
| updatedAt: timestamp("updated_at") | ||
| .notNull() | ||
| .defaultNow() | ||
| .$onUpdate(() => new Date()), | ||
| }, | ||
| (table) => [ | ||
| index("v2_devices_organization_id_idx").on(table.organizationId), | ||
| unique("v2_devices_org_hashed_device_id_unique").on( | ||
| table.organizationId, | ||
| table.hashedDeviceId, | ||
| ), | ||
| ], | ||
| ); | ||
|
|
||
| export type InsertV2Device = typeof v2Devices.$inferInsert; | ||
| export type SelectV2Device = typeof v2Devices.$inferSelect; | ||
|
|
||
| export const v2Workspaces = pgTable( | ||
| "v2_workspaces", | ||
| { | ||
| id: uuid().primaryKey().defaultRandom(), | ||
| organizationId: uuid("organization_id") | ||
| .notNull() | ||
| .references(() => organizations.id, { onDelete: "cascade" }), | ||
| projectId: uuid("project_id") | ||
| .notNull() | ||
| .references(() => v2Projects.id, { onDelete: "cascade" }), | ||
| name: text().notNull(), | ||
| branch: text().notNull().default("main"), | ||
| deviceId: uuid("device_id").references(() => v2Devices.id, { | ||
| onDelete: "set null", | ||
| }), | ||
| createdByUserId: uuid("created_by_user_id").references(() => users.id, { | ||
| onDelete: "set null", | ||
| }), | ||
| createdAt: timestamp("created_at").notNull().defaultNow(), | ||
| updatedAt: timestamp("updated_at") | ||
| .notNull() | ||
| .defaultNow() | ||
| .$onUpdate(() => new Date()), | ||
| }, | ||
| (table) => [ | ||
| index("v2_workspaces_project_id_idx").on(table.projectId), | ||
| index("v2_workspaces_organization_id_idx").on(table.organizationId), | ||
| ], | ||
| ); | ||
|
|
||
| export type InsertV2Workspace = typeof v2Workspaces.$inferInsert; | ||
| export type SelectV2Workspace = typeof v2Workspaces.$inferSelect; |
Contributor
There was a problem hiding this comment.
Generate the cloud Drizzle migration before merge.
The PR objective still lists v2_projects_workspaces_devices as pending. Without the generated migration, deployed databases will not have v2_projects, v2_devices, or v2_workspaces, and the new routers will fail on first use. As per coding guidelines, "Create migrations by modifying schema files in packages/db/src/schema/ and running bunx drizzle-kit generate --name='<sample_name_snake_case>', not by manually creating migration files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/schema/v2.ts` around lines 12 - 99, The schema additions for
v2Projects, v2Devices, and v2Workspaces lack a generated Drizzle migration; run
the Drizzle migration generator after saving these schema changes (e.g. run bunx
drizzle-kit generate --name='v2_projects_workspaces_devices'), verify the
generated migration includes creation of tables for v2_projects, v2_devices, and
v2_workspaces (matching the v2Projects, v2Devices, v2Workspaces definitions),
add and commit the generated migration file to the PR, and ensure the migration
is included before merging.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/host-service/src/trpc/router/project/project.ts`:
- Around line 25-36: The current cleanup swallows all errors from removing
worktrees (loop using localWorkspaces and ctx.git) and rmSync then
unconditionally runs ctx.db.delete(projects).where(eq(projects.id,
input.projectId)).run(); instead, detect and surface failures from the worktree
removal loop (calls to ctx.git(...).raw(["worktree","remove", ...])) and the
rmSync call: collect any thrown errors or stop on first error and rethrow or
return an error to the caller so the DB delete is not executed if cleanup
failed; i.e., only call ctx.db.delete(...).run() when the filesystem cleanup
completed successfully, otherwise propagate the error.
- Line 1: The import of rmSync should be replaced with the non-blocking async
API and the deletion call sites updated to await the promise; replace the rmSync
import with the promise-based rm (e.g., import { rm } from "node:fs/promises" or
use fs.promises.rm), make the enclosing handler in project.ts async if it isn’t
already, call await rm(targetPath, { recursive: true, force: true }) where
rmSync was used, and add proper try/catch or error logging around the await to
surface failures without blocking the event loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a791e742-cab4-42c5-88b5-10de4bc76b13
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/desktop/electron-builder.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/HostServiceStatus/HostServiceStatus.tsxpackages/db/src/schema/relations.tspackages/host-service/package.jsonpackages/host-service/src/db/schema.tspackages/host-service/src/trpc/router/project/project.tspackages/host-service/src/trpc/router/workspace/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/host-service/package.json
- apps/desktop/electron-builder.ts
- packages/host-service/src/db/schema.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/HostServiceStatus/HostServiceStatus.tsx
- packages/host-service/src/trpc/router/workspace/workspace.ts
| @@ -0,0 +1,40 @@ | |||
| import { rmSync } from "node:fs"; | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the target file to confirm the issue
head -40 packages/host-service/src/trpc/router/project/project.tsRepository: superset-sh/superset
Length of output: 1140
🏁 Script executed:
# Check Node version requirements in package.json
cat package.json | head -50Repository: superset-sh/superset
Length of output: 2050
🏁 Script executed:
# Also check if there's a .tool-versions or similar file
fd -t f "\.nvmrc|\.tool-versions|\.node-version" -x cat {}Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the host-service package.json specifically for Node version constraints
cat packages/host-service/package.jsonRepository: superset-sh/superset
Length of output: 1235
🏁 Script executed:
# Search for any Node version constraints in the repository
rg "engines.*node|node.*version|minimum.*node" -i --type jsonRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check for CI/CD configuration that specifies Node version
fd -t f "\.yml|\.yaml|dockerfile" ".github|docker" -x head -20 {}Repository: superset-sh/superset
Length of output: 165
🏁 Script executed:
# Search for any Node version setup in workflow files
rg "node-version|node.version|setup-node" -i -A 2Repository: superset-sh/superset
Length of output: 866
🏁 Script executed:
# Check if there's a Dockerfile or build config
fd -t f "dockerfile|\.dockerignore|\.bun"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check for .nvmrc or .node-version files at root
cat .nvmrc 2>/dev/null || cat .node-version 2>/dev/null || echo "No version file found"Repository: superset-sh/superset
Length of output: 85
🏁 Script executed:
# Check GitHub workflows directory
ls -la .github/workflows/ 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 867
🏁 Script executed:
# Search for Node setup in workflow files
find .github -name "*.yml" -o -name "*.yaml" 2>/dev/null | xargs grep -l "node-version\|setup-node" 2>/dev/nullRepository: superset-sh/superset
Length of output: 48
🏁 Script executed:
# Check the ci.yml workflow for Node setup
cat .github/workflows/ci.ymlRepository: superset-sh/superset
Length of output: 3088
🏁 Script executed:
# Check if fs/promises.rm is available (it was added in Node 14.14.0)
# Let's verify the actual Node version expected
rg "node.*18|node.*20|node.*22" -iRepository: superset-sh/superset
Length of output: 50376
🏁 Script executed:
# Verify fs/promises.rm availability info - it was added in Node 14.14.0
# Let's confirm the code context one more time to ensure accuracy
sed -n '1p;32,35p' packages/host-service/src/trpc/router/project/project.tsRepository: superset-sh/superset
Length of output: 190
Avoid rmSync on the RPC path.
rmSync blocks the Node event loop for the full recursive delete. On a large repo, this mutation can stall the host-service process and freeze unrelated requests/UI work. Prefer the async fs API here.
💡 Suggested change
-import { rmSync } from "node:fs";
+import { rm } from "node:fs/promises";
@@
try {
- rmSync(localProject.repoPath, { recursive: true, force: true });
+ await rm(localProject.repoPath, { recursive: true, force: true });
} catch {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/trpc/router/project/project.ts` at line 1, The
import of rmSync should be replaced with the non-blocking async API and the
deletion call sites updated to await the promise; replace the rmSync import with
the promise-based rm (e.g., import { rm } from "node:fs/promises" or use
fs.promises.rm), make the enclosing handler in project.ts async if it isn’t
already, call await rm(targetPath, { recursive: true, force: true }) where
rmSync was used, and add proper try/catch or error logging around the await to
surface failures without blocking the event loop.
Comment on lines
+25
to
+36
| for (const ws of localWorkspaces) { | ||
| try { | ||
| const git = await ctx.git(localProject.repoPath); | ||
| await git.raw(["worktree", "remove", ws.worktreePath]); | ||
| } catch {} | ||
| } | ||
|
|
||
| try { | ||
| rmSync(localProject.repoPath, { recursive: true, force: true }); | ||
| } catch {} | ||
|
|
||
| ctx.db.delete(projects).where(eq(projects.id, input.projectId)).run(); |
Contributor
There was a problem hiding this comment.
Don't drop the local DB state after cleanup failures.
Lines 25-34 swallow every filesystem error, but Line 36 deletes the projects row anyway. Since workspaces.projectId cascades in packages/host-service/src/db/schema.ts:18-32, a failed worktree removal here leaves orphaned worktrees on disk while the local DB forgets they exist. Abort the DB delete when any cleanup step fails, or surface the failures to the caller instead of always returning success.
💡 Suggested change
+ const cleanupFailures: string[] = [];
+ const git = await ctx.git(localProject.repoPath);
+
for (const ws of localWorkspaces) {
try {
- const git = await ctx.git(localProject.repoPath);
await git.raw(["worktree", "remove", ws.worktreePath]);
- } catch {}
+ } catch (error) {
+ cleanupFailures.push(
+ `Failed to remove worktree ${ws.worktreePath}: ${String(error)}`,
+ );
+ }
}
try {
rmSync(localProject.repoPath, { recursive: true, force: true });
- } catch {}
+ } catch (error) {
+ cleanupFailures.push(
+ `Failed to remove repo ${localProject.repoPath}: ${String(error)}`,
+ );
+ }
+
+ if (cleanupFailures.length > 0) {
+ throw new Error(cleanupFailures.join("\n"));
+ }
ctx.db.delete(projects).where(eq(projects.id, input.projectId)).run();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/trpc/router/project/project.ts` around lines 25 -
36, The current cleanup swallows all errors from removing worktrees (loop using
localWorkspaces and ctx.git) and rmSync then unconditionally runs
ctx.db.delete(projects).where(eq(projects.id, input.projectId)).run(); instead,
detect and surface failures from the worktree removal loop (calls to
ctx.git(...).raw(["worktree","remove", ...])) and the rmSync call: collect any
thrown errors or stop on first error and rethrow or return an error to the
caller so the DB delete is not executed if cleanup failed; i.e., only call
ctx.db.delete(...).run() when the filesystem cleanup completed successfully,
otherwise propagate the error.
- Validate githubRepositoryId org ownership in v2-project create/update - Validate projectId and deviceId org ownership in v2-workspace create/update - Add console.warn to all best-effort catch blocks for observability - Reorder workspace.delete: cloud first, then local cleanup - Rollback worktree on cloud-create failure in workspace.create - Normalize HOST_DB_PATH with trim/fallback in serve.ts - Remove unnecessary comments
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/host-service/src/trpc/router/workspace/workspace.ts`:
- Around line 45-53: Wrap the entire sequence that performs
git.clone(cloudProject.repoCloneUrl, repoPath), any worktree operations (e.g.,
git.worktree.add(...)), and the local DB insert
(ctx.db.insert(projects).values({ id: input.projectId, repoPath
}).returning().get()) in a single try/catch so that if any step after the
initial filesystem side-effect fails you perform compensating cleanup: remove
the cloned repo directory at repoPath and remove any added worktree before
rethrowing the error; ensure you clear localProject state on failure so
subsequent runs won’t treat orphaned directories as valid.
- Around line 126-140: The current delete path removes the workspaces row
unconditionally after attempting git.raw(["worktree","remove", ...]) so if
ctx.git/localWorkspace removal fails the DB row is lost; change the flow so the
DB deletion only happens on successful worktree removal (move
ctx.db.delete(workspaces).where(eq(workspaces.id, input.id)).run() into the try
success path) or, on catch, instead mark the row for cleanup (e.g., update the
workspace with a cleanup_pending or pendingCleanup flag via
ctx.db.update(workspaces).set({...}).where(eq(workspaces.id, input.id)).run())
so project.removeFromDevice can still discover the orphaned worktree; ensure you
reference localProject, localWorkspace and the git removal try/catch block when
making the change.
- Around line 44-45: The current cold-start fails because ctx.git(repoPath)
expects an existing repo before cloning; change workspace.create to use a
clone-aware git client instead of binding to the not-yet-cloned path: call a
clone helper that accepts cloudProject.repoCloneUrl (or acquire a git client
bound to the parent directory of repoPath) and then run
git.clone(cloudProject.repoCloneUrl, repoPath); replace the ctx.git(repoPath)
usage that precedes git.clone with this clone-specific client so credentials are
derived correctly prior to cloning.
In `@packages/trpc/src/router/v2-project/v2-project.ts`:
- Around line 93-139: The update protectedProcedure currently allows calls with
only id and no mutable fields; modify the input z.object for update (the schema
passed to protectedProcedure.input) to require at least one of name, slug, or
githubRepositoryId (e.g., add a .refine(...) or a custom Zod check on the
object) so validation fails when no other fields are provided, and after
performing the dbWs.update(v2Projects)... .returning() check the result (the
const [updated] = ...) and if updated is falsy throw a TRPCError with code
"NOT_FOUND" (and a clear message like "Project not found") so the mutation does
not silently succeed when no row matches.
In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts`:
- Around line 72-118: The update protectedProcedure currently allows an id-only
payload and returns undefined when no row matches; modify the handler in
v2-workspace.update to (1) validate that the patch is non-empty by checking
Object.keys(data).length (or similar) after const { id, ...data } = input and
throw a TRPCError({ code: "BAD_REQUEST", message: "Empty update payload" }) if
there are no updatable fields, and (2) after performing
dbWs.update(...).returning(), check the returned updated row and throw new
TRPCError({ code: "NOT_FOUND", message: "Workspace not found" }) when no row was
updated; keep the existing verifyOrgMembership and device check logic intact.
- Around line 13-18: Update the branch field validation in the z.object input
schemas in v2-workspace.ts so empty strings are rejected: replace occurrences of
branch: z.string().optional() with branch: z.string().trim().min(1).optional()
(or a branch-specific validator) in both places where the z.object containing
projectId/name/branch/deviceId is defined so a provided branch cannot be an
empty string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27d72468-e701-4ee4-9aae-7cf156d2552c
📒 Files selected for processing (7)
packages/db/src/schema/v2.tspackages/host-service/src/db/db.tspackages/host-service/src/serve.tspackages/host-service/src/trpc/router/project/project.tspackages/host-service/src/trpc/router/workspace/workspace.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/host-service/src/db/db.ts
- packages/host-service/src/trpc/router/project/project.ts
- packages/host-service/src/serve.ts
- packages/db/src/schema/v2.ts
Comment on lines
+45
to
+53
| await git.clone(cloudProject.repoCloneUrl, repoPath); | ||
|
|
||
| const inserted = ctx.db | ||
| .insert(projects) | ||
| .values({ id: input.projectId, repoPath }) | ||
| .returning() | ||
| .get(); | ||
|
|
||
| localProject = inserted; |
Contributor
There was a problem hiding this comment.
Post-clone failures still leave orphaned local state.
Once git.clone() or worktree add succeeds, any later throw from the local DB writes leaves filesystem state behind with no matching row. The next create will think nothing is cloned but still hit existing directories. Wrap the clone/worktree/cloud/local-DB sequence in one compensating try/catch, and remove the repo/worktree on every failure after the first local side effect.
Also applies to: 69-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/trpc/router/workspace/workspace.ts` around lines 45
- 53, Wrap the entire sequence that performs
git.clone(cloudProject.repoCloneUrl, repoPath), any worktree operations (e.g.,
git.worktree.add(...)), and the local DB insert
(ctx.db.insert(projects).values({ id: input.projectId, repoPath
}).returning().get()) in a single try/catch so that if any step after the
initial filesystem side-effect fails you perform compensating cleanup: remove
the cloned repo directory at repoPath and remove any added worktree before
rethrowing the error; ensure you clear localProject state on failure so
subsequent runs won’t treat orphaned directories as valid.
Comment on lines
+126
to
+140
| if (localProject) { | ||
| try { | ||
| const git = await ctx.git(localProject.repoPath); | ||
| await git.raw(["worktree", "remove", localWorkspace.worktreePath]); | ||
| } catch (err) { | ||
| console.warn("[workspace.delete] failed to remove worktree", { | ||
| workspaceId: input.id, | ||
| worktreePath: localWorkspace.worktreePath, | ||
| err, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ctx.db.delete(workspaces).where(eq(workspaces.id, input.id)).run(); |
Contributor
There was a problem hiding this comment.
Don't drop the local row when worktree removal fails.
If git worktree remove throws, Line 140 still deletes the workspaces row. That loses the only handle to the orphaned worktree, and project.removeFromDevice won't be able to discover it later because it enumerates local workspace rows. Keep the row until local cleanup succeeds, or mark it pending cleanup instead of hard-deleting it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/trpc/router/workspace/workspace.ts` around lines
126 - 140, The current delete path removes the workspaces row unconditionally
after attempting git.raw(["worktree","remove", ...]) so if
ctx.git/localWorkspace removal fails the DB row is lost; change the flow so the
DB deletion only happens on successful worktree removal (move
ctx.db.delete(workspaces).where(eq(workspaces.id, input.id)).run() into the try
success path) or, on catch, instead mark the row for cleanup (e.g., update the
workspace with a cleanup_pending or pendingCleanup flag via
ctx.db.update(workspaces).set({...}).where(eq(workspaces.id, input.id)).run())
so project.removeFromDevice can still discover the orphaned worktree; ensure you
reference localProject, localWorkspace and the git removal try/catch block when
making the change.
Comment on lines
+13
to
+18
| z.object({ | ||
| projectId: z.string().uuid(), | ||
| name: z.string().min(1), | ||
| branch: z.string().optional(), | ||
| deviceId: z.string().uuid().optional(), | ||
| }), |
Contributor
There was a problem hiding this comment.
Reject empty branch values.
z.string().optional() accepts "", so both mutations can persist an invalid git ref. Tighten this to .trim().min(1) at minimum, or use a branch-specific validator when branch is provided.
Suggested validation change
Apply in both input schemas:
- branch: z.string().optional(),
+ branch: z.string().trim().min(1).optional(),Also applies to: 74-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts` around lines 13 - 18,
Update the branch field validation in the z.object input schemas in
v2-workspace.ts so empty strings are rejected: replace occurrences of branch:
z.string().optional() with branch: z.string().trim().min(1).optional() (or a
branch-specific validator) in both places where the z.object containing
projectId/name/branch/deviceId is defined so a provided branch cannot be an
empty string.
- Fix HOST_MIGRATIONS_PATH for prod builds (use process.resourcesPath) - Pass remoteUrl to GitFactory for cold-start clone (no existing repo) - Add branch min(1) validation in v2-workspace create/update - Require at least one mutable field in update, throw NOT_FOUND on miss
ctx.git(repoPath) already handles non-existent repos gracefully. Authenticated cloning for private repos is a separate concern.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/host-service/src/trpc/router/workspace/workspace.ts (3)
41-45:⚠️ Potential issue | 🟠 MajorBootstrap the clone from an existing directory.
This path is still rooted at
repoPathbefore anything creates~/.superset/repos/<projectId>. On a fresh machine, the cold-start clone path is brittle unless you create the parent directory first and bind the clone client to an existing directory instead of the not-yet-created repo path.In simple-git v3.x, is it valid to initialize the client with a `baseDir`/cwd that does not exist yet and then call `clone(remote, targetPath)`? Also, does `git clone <repo> <path>` create missing parent directories for `<path>`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace/workspace.ts` around lines 41 - 45, The code assumes repoPath exists before cloning; ensure the parent directory (~/.superset/repos) is created first and initialize the simple-git client bound to an existing directory (e.g., the parent dir) rather than the not-yet-created repoPath. Specifically, create the parent folder with a recursive mkdir (using fs.promises.mkdir or similar) for the path that contains repoPath, then call ctx.git(...) with that existing parentDir (not repoPath) and finally run git.clone(cloudProject.repoCloneUrl, repoPath). This guarantees the cwd passed to ctx.git is present and that the clone target has a valid parent directory.
44-99:⚠️ Potential issue | 🟠 MajorThe create flow still leaves orphaned state outside the cloud-create catch.
Only the
v2Workspace.createfailure rolls back the worktree. Ifgit.clone(), theprojectsinsert,worktree add, or the finalworkspacesinsert throws after the first local side effect, this mutation can leave a cloned repo/worktree on disk with no matching local row; if the failure happens aftercloudRowis created, it also leaves a cloud workspace behind. Wrap the full clone → local project insert → worktree add → cloud create → local workspace insert sequence in one compensatingtry/catchand undo every artifact created so far before rethrowing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace/workspace.ts` around lines 44 - 99, The sequence that performs ctx.git(...).clone, ctx.db.insert(projects).values(...), git.raw(["worktree","add",...]), ctx.api.v2Workspace.create.mutate(...), and the final ctx.db.insert(workspaces).values(...) must be wrapped in a single try/catch so every side-effect is undone on any failure: on error, if worktree was added remove it via git.raw(["worktree","remove", worktreePath]); if a project row was inserted delete that DB row (the insert into projects); if the repo was cloned remove the repo directory (repoPath); if the cloud workspace was created call the appropriate rollback/delete for ctx.api.v2Workspace (or delete the created cloudRow); rethrow the original error after cleanup. Ensure you check existence flags (e.g., cloudRow, localProject, worktreePath) to only undo what was actually created and perform cleanup in the reverse order of creation.
126-140:⚠️ Potential issue | 🟠 MajorKeep the local workspace row until local cleanup succeeds.
If
git.raw(["worktree", "remove", ...])fails here, Line 140 still deletes theworkspacesrow. That drops the only handle to the orphaned checkout, so later cleanup flows cannot discover it. Delete the row only after successful removal, or mark it pending cleanup on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace/workspace.ts` around lines 126 - 140, The current flow calls git.raw(["worktree","remove", ...]) and then always runs ctx.db.delete(workspaces).where(eq(workspaces.id, input.id)).run(), which can orphan a local worktree if the git removal fails; change the logic in the delete handler so that you only delete the DB row after the worktree removal succeeds (i.e., move or gate the ctx.db.delete(...) call behind the try block that calls ctx.git(...) / git.raw(...)), and if removal fails keep the row and update it to indicate pending cleanup (e.g., set a cleanup_pending flag or status column) instead of deleting it so later cleanup workers can discover and remediate the orphaned checkout; reference localProject, ctx.git, git.raw, localWorkspace.worktreePath and the ctx.db.delete(workspaces).where(eq(workspaces.id, input.id)).run() call when making the change.
🧹 Nitpick comments (3)
packages/trpc/src/router/v2-workspace/v2-workspace.ts (1)
136-156: Consider throwing NOT_FOUND for non-existent workspaces.The delete mutation returns
{ success: true }even when no workspace was deleted. While idempotent deletes are a valid REST pattern, this differs from the update mutation which throwsNOT_FOUNDfor missing resources.For consistency, you could check the delete result:
♻️ Optional: Add NOT_FOUND check for consistency
- await dbWs + const result = await dbWs .delete(v2Workspaces) .where( and( eq(v2Workspaces.id, input.id), eq(v2Workspaces.organizationId, organizationId), ), - ); + ) + .returning({ id: v2Workspaces.id }); + if (result.length === 0) { + throw new TRPCError({ + code: "NOT_FOUND", + message: "Workspace not found", + }); + } return { success: true };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts` around lines 136 - 156, The delete mutation currently always returns { success: true } even when no row is deleted; update the mutation handling in the delete protectedProcedure (the block using verifyOrgAdmin and dbWs.delete(v2Workspaces).where(...)) to capture the delete result/affected count from dbWs.delete(...).where(...), and if no rows were deleted throw a TRPCError with code "NOT_FOUND" and an appropriate message; otherwise return { success: true } as before to maintain existing response shape.packages/trpc/src/router/v2-project/v2-project.ts (2)
75-91: Consider handling duplicate slug constraint violations.The
v2_projects_org_slug_uniqueconstraint (from packages/db/src/schema/v2.ts lines 33) will throw a database error on duplicate slugs within the same organization. This will surface as an unhandled exception rather than a user-friendlyBAD_REQUEST.♻️ Wrap insert in try/catch to handle unique constraint violation
+ try { const [project] = await dbWs .insert(v2Projects) .values({ organizationId, name: input.name, slug: input.slug, githubRepositoryId: input.githubRepositoryId, }) .returning(); if (!project) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Failed to create project", }); } return project; + } catch (error) { + if ( + error instanceof Error && + error.message.includes("v2_projects_org_slug_unique") + ) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "A project with this slug already exists", + }); + } + throw error; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/v2-project/v2-project.ts` around lines 75 - 91, Wrap the insert call for creating a project in a try/catch around the dbWs.insert(v2Projects).values(...).returning() block in the handler that returns `project`; on catch inspect the error for the unique constraint name "v2_projects_org_slug_unique" (or database-specific code indicating a unique violation) and rethrow a TRPCError with code "BAD_REQUEST" and a clear message like "Project slug already exists in this organization"; for all other errors rethrow or convert to the existing INTERNAL_SERVER_ERROR path so existing consumers still get the same behavior.
157-177: Consider throwing NOT_FOUND for non-existent projects (same as workspace router).For consistency with the update procedure and the workspace router, consider checking if a row was actually deleted.
♻️ Optional: Add NOT_FOUND check for consistency
- await dbWs + const result = await dbWs .delete(v2Projects) .where( and( eq(v2Projects.id, input.id), eq(v2Projects.organizationId, organizationId), ), - ); + ) + .returning({ id: v2Projects.id }); + if (result.length === 0) { + throw new TRPCError({ + code: "NOT_FOUND", + message: "Project not found", + }); + } return { success: true };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/v2-project/v2-project.ts` around lines 157 - 177, The delete mutation in v2-project.ts should verify a row was actually deleted and throw a TRPCError with code "NOT_FOUND" if not; update the mutation to capture the result of dbWs.delete(v2Projects).where(...), check the returned affected row count/rowCount (or equivalent property on the delete result) after the where using the same input.id and organizationId, and if it indicates zero rows were removed throw new TRPCError({ code: "NOT_FOUND", message: "Project not found" }), otherwise return { success: true } (keep verifyOrgAdmin call as-is).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/host-service/src/trpc/router/workspace/workspace.ts`:
- Around line 41-45: The code assumes repoPath exists before cloning; ensure the
parent directory (~/.superset/repos) is created first and initialize the
simple-git client bound to an existing directory (e.g., the parent dir) rather
than the not-yet-created repoPath. Specifically, create the parent folder with a
recursive mkdir (using fs.promises.mkdir or similar) for the path that contains
repoPath, then call ctx.git(...) with that existing parentDir (not repoPath) and
finally run git.clone(cloudProject.repoCloneUrl, repoPath). This guarantees the
cwd passed to ctx.git is present and that the clone target has a valid parent
directory.
- Around line 44-99: The sequence that performs ctx.git(...).clone,
ctx.db.insert(projects).values(...), git.raw(["worktree","add",...]),
ctx.api.v2Workspace.create.mutate(...), and the final
ctx.db.insert(workspaces).values(...) must be wrapped in a single try/catch so
every side-effect is undone on any failure: on error, if worktree was added
remove it via git.raw(["worktree","remove", worktreePath]); if a project row was
inserted delete that DB row (the insert into projects); if the repo was cloned
remove the repo directory (repoPath); if the cloud workspace was created call
the appropriate rollback/delete for ctx.api.v2Workspace (or delete the created
cloudRow); rethrow the original error after cleanup. Ensure you check existence
flags (e.g., cloudRow, localProject, worktreePath) to only undo what was
actually created and perform cleanup in the reverse order of creation.
- Around line 126-140: The current flow calls git.raw(["worktree","remove",
...]) and then always runs ctx.db.delete(workspaces).where(eq(workspaces.id,
input.id)).run(), which can orphan a local worktree if the git removal fails;
change the logic in the delete handler so that you only delete the DB row after
the worktree removal succeeds (i.e., move or gate the ctx.db.delete(...) call
behind the try block that calls ctx.git(...) / git.raw(...)), and if removal
fails keep the row and update it to indicate pending cleanup (e.g., set a
cleanup_pending flag or status column) instead of deleting it so later cleanup
workers can discover and remediate the orphaned checkout; reference
localProject, ctx.git, git.raw, localWorkspace.worktreePath and the
ctx.db.delete(workspaces).where(eq(workspaces.id, input.id)).run() call when
making the change.
---
Nitpick comments:
In `@packages/trpc/src/router/v2-project/v2-project.ts`:
- Around line 75-91: Wrap the insert call for creating a project in a try/catch
around the dbWs.insert(v2Projects).values(...).returning() block in the handler
that returns `project`; on catch inspect the error for the unique constraint
name "v2_projects_org_slug_unique" (or database-specific code indicating a
unique violation) and rethrow a TRPCError with code "BAD_REQUEST" and a clear
message like "Project slug already exists in this organization"; for all other
errors rethrow or convert to the existing INTERNAL_SERVER_ERROR path so existing
consumers still get the same behavior.
- Around line 157-177: The delete mutation in v2-project.ts should verify a row
was actually deleted and throw a TRPCError with code "NOT_FOUND" if not; update
the mutation to capture the result of dbWs.delete(v2Projects).where(...), check
the returned affected row count/rowCount (or equivalent property on the delete
result) after the where using the same input.id and organizationId, and if it
indicates zero rows were removed throw new TRPCError({ code: "NOT_FOUND",
message: "Project not found" }), otherwise return { success: true } (keep
verifyOrgAdmin call as-is).
In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts`:
- Around line 136-156: The delete mutation currently always returns { success:
true } even when no row is deleted; update the mutation handling in the delete
protectedProcedure (the block using verifyOrgAdmin and
dbWs.delete(v2Workspaces).where(...)) to capture the delete result/affected
count from dbWs.delete(...).where(...), and if no rows were deleted throw a
TRPCError with code "NOT_FOUND" and an appropriate message; otherwise return {
success: true } as before to maintain existing response shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d8567c1-bb5a-4a0f-887c-bf6f9e8c3220
📒 Files selected for processing (6)
apps/desktop/src/main/lib/host-service-manager.tspackages/host-service/src/git/createGitFactory/createGitFactory.tspackages/host-service/src/git/types.tspackages/host-service/src/trpc/router/workspace/workspace.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/host-service-manager.ts
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/git/types.ts">
<violation number="1">
P2: Dropping the optional `remoteUrl` from `GitFactory` removes the only way to supply the clone URL when the repo doesn’t exist yet. In `workspace.create`, the factory is called before `git.clone`, so `getRemoteUrl` returns null and `CloudCredentialProvider` skips auth, which breaks private repo clones. Keep a `remoteUrl` parameter (and plumb it through) so initial clones can resolve credentials.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Remove duplicate packages/db/src/schema/v2.ts (Avi shipped v2 tables in schema.ts) - Remove Avi's unused duplicate cloud routers (projectsV2, workspacesV2) — zero callers - Keep our v2Project/v2Workspace routers (session-derived orgId, full CRUD, host-service consumer) - Fix v2Workspace.create branch input to required (matches NOT NULL schema) - Resolve collections.ts merge conflict (take Avi's v2 collection definitions with feature flag gating)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the v2 project/workspace/device infrastructure (SUPER-362):
better-sqlite3+drizzle-ormdatabase for tracking local filesystem state (cloned repo paths, worktree locations). Schema:projectsandworkspacestables with migration support across Electron/dev environments.packages/db) — three new tables:v2_projects,v2_devices,v2_workspaceswith proper FK relationships and indexes.packages/trpc) — CRUD routers forv2Projectandv2Workspacewith org membership verification, derivingorganizationIdfrom Better Auth session.workspace.create(lazily clones repo + creates git worktree + syncs to cloud),workspace.delete(removes worktree + cloud row),project.removeFromDevice(local-only cleanup, no cloud mutation).v2Projects,v2Workspaces,v2DevicestoOrgCollectionsfor real-time sync.HostServiceStatuscomponent for manual testing.Test plan
bun run typecheck— 22/22 passbun run lint:fix— cleanbun test— 1742 pass (4 pre-existing failures unrelated to this PR)bunx drizzle-kit generate --name="v2_projects_workspaces_devices"inpackages/dbto generate cloud migrationSummary by cubic
Implements the v2 projects/workspaces/devices stack across cloud and host-service to enable cloning repos, creating Git worktrees, and syncing records to the cloud. Extends SUPER-362 with session-scoped routers, org validations, safer workspace lifecycle, feature-flagged v2 collections, and production-ready migrations.
New Features
v2_projects,v2_devices,v2_workspacesin@superset/db; newv2Projectandv2Workspacerouters in@superset/trpcwith session-derived org checks andrepoCloneUrlonv2Project.get; removes legacyprojectsV2/workspacesV2routers; adoptsv2Projects,v2Workspaces,v2DevicesinOrgCollectionswith feature flag gating.better-sqlite3+drizzle-ormwith auto-migrations; routersworkspace.create(clone → worktree → cloud),workspace.delete,project.removeFromDevice; app acceptsHOST_DB_PATH; clone usesrepoCloneUrlwhen a local repo is missing; resolves migrations at runtime fromresources/host-migrationsorHOST_MIGRATIONS_PATH.resources/host-migrationsfor dev/prod, wiresHOST_DB_PATH/HOST_MIGRATIONS_PATH, and adds a V2 Operations section inHostServiceStatusfor manual create/delete tests.packages/host-servicewith workspace (better-sqlite3@12.6.2,drizzle-orm@0.45.1,drizzle-kit@0.31.8).Bug Fixes
githubRepositoryId,projectId,deviceId; requirebranchmin(1) in v2-workspace create/update; require at least one field in v2 project/workspace update and returnNOT_FOUNDwhen missing.HOST_MIGRATIONS_PATHfor prod builds (useprocess.resourcesPath); normalizeHOST_DB_PATHhandling; addconsole.warnto best-effort cleanups.Written for commit 8894ddc. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores