[codex] Stabilize v1 to v2 migration#3775
Merged
saddlepaddle merged 10 commits intomainfrom Apr 27, 2026
Merged
Conversation
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 PR refactors the V1→V2 migration flow (UI and hooks), adds idempotent "synced" states and improved workspace/project reconciliation, expands repo resolution for local-only repos, enhances workspace adoption for reruns, and adds a support API to email migration reports via Resend. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Modal as V1MigrationSummaryModal
participant Hook as useMigrateV1DataToV2
participant Migrate as migrateV1DataToV2
participant Host as Host Service
participant Cloud as Cloud API
participant Support as Support Router
participant Resend as Resend
User->>Modal: Open modal / click "Get Started"
Modal->>Hook: rerun() / runMigration()
activate Hook
Hook->>Migrate: invoke migrateV1DataToV2()
activate Migrate
Migrate->>Host: project.findByPath / setup
Host->>Cloud: create/link cloud project (slug retry if needed)
Cloud-->>Host: created / error
Migrate->>Host: workspaceCreation.adopt (baseBranch, existingWorkspaceId)
Host-->>Migrate: workspace linked/created or error
Migrate-->>Hook: summary (projects/workspaces/errors/skipped)
deactivate Migrate
Hook-->>Modal: { completed, summary }
deactivate Hook
Modal->>User: show Results
User->>Modal: "Contact us" (on errors)
Modal->>Support: sendMigrationReport({ report })
activate Support
Support->>Support: rate-limit check (Upstash)
Support->>Resend: send email
Resend-->>Support: success / failure
Support-->>Modal: success / error (or fallback)
deactivate Support
Modal->>User: toast notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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! 🎉 |
Greptile SummaryThis PR stabilises the v1→v2 desktop migration so it is fully idempotent and recoverable: already-linked projects are reconciled via Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/quality issues that do not affect correctness of the migration or data integrity. The migration logic is thoroughly tested (420+ lines of new tests covering reruns, state-write failures, workspace reconciliation, and base-branch propagation). The three open findings are all P2: a useCallback/useEffect dependency pattern that is harmless due to existing guards, a duplicate-entry issue in the support email report, and fragile string-matching in isSlugConflict. useMigrateV1DataToV2.ts (isRunning dep pattern), V1MigrationSummaryModal.tsx (support report deduplication), handlers.ts (isSlugConflict reliability)
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts | Core migration logic: adds idempotent rerun paths for already-linked projects, synced/error/skipped workspace reconciliation, and helper functions for clean summary accumulation |
| apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts | Refactors auto-run effect into a callable callback; adds rerun/isRunning API; isRunning in useCallback deps causes the useEffect to re-fire on every state transition |
| apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx | Rebuilt modal with Welcome/Results pages, support-report sending via Resend, and expandable per-item sections; buildMigrationSupportReport duplicates entries from both summary.errors and filtered summary.projects/workspaces |
| packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts | Extensive hardening: existingWorkspaceId relink, cloud/local reconciliation, branch-name correction from actual worktree, base branch recording, and persistLocalWorkspace helper extraction |
| packages/host-service/src/trpc/router/project/handlers.ts | Adds slug-conflict retry (up to 10 attempts), local-only repo support via resolveLocalRepo, and cloud rollback on local persistence failure; isSlugConflict uses fragile string matching |
| packages/host-service/src/trpc/router/project/utils/resolve-repo.ts | Adds resolveLocalRepo to support repos without a GitHub remote; existing functions return the narrower ResolvedGitHubRepo type |
| apps/electric-proxy/src/index.ts | Wraps entire fetch handler in try/catch returning 500 with CORS headers on unhandled errors; indentation-only refactor otherwise |
| packages/trpc/src/router/support/support.ts | New router procedure sending migration issue reports via Resend; Resend client instantiated at module load from validated env |
| packages/trpc/src/router/v2-project/v2-project.ts | Adds deleteFromHost procedure for cloud rollback on local persistence failure; org-membership check guards the deletion |
| packages/trpc/src/router/v2-workspace/v2-workspace.ts | Adds getFromHost query returning the cloud workspace row (or null) for adopt-path reconciliation; org-membership guard present |
| packages/host-service/src/trpc/router/workspace-creation/shared/branch-search.ts | Extracts getWorktreeBranchAtPath from findWorktreeAtPath, adds realpathSync.native normalization for symlink safety |
| apps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsx | Adds conditional V1→V2 migration 'Run again' button backed by useMigrateV1DataToV2({ autoRun: false }) and a toast.promise feedback loop |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([migrateV1DataToV2]) --> B{For each v1 project}
B --> C{existing state\nsuccess/linked?}
C -- Yes --> D[setupProjectImport\nstatus=synced]
D -- error --> E[mark error\nremove from map]
C -- No\nor error --> F{findByPath\nfinds match?}
F -- Yes --> G[Link project\nstatus=linked]
F -- No --> H[Create project\nstatus=created]
G & H --> I[upsertState\nadd to projectV1ToV2]
I -- write fails --> J[delete from map\nmark error]
A --> K{For each v1 workspace}
K --> L{existing status\n= success + v2Id?}
L -- Yes --> M{hasLocalWorkspace?}
M -- Yes --> N[status=synced\ncontinue]
M -- No --> O[recoverCompleted=true\nre-adopt with existingWorkspaceId]
L -- shouldRetry? --> P{parent project\nin map?}
P -- No --> Q[skip: parent_unresolved]
P -- Yes --> R[adopt.mutate\nbaseBranch + existingWorkspaceId]
R -- worktree_not_registered --> S[skip]
R -- success --> T[upsertState success]
R -- error --> U[mark error]
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts, line 109-112 (link)isRunninginuseCallbackdeps re-fires the migration effectisRunningis listed as a dependency ofrunMigration, so every time the migration changes that state (false → true on start, true → false on finish) a newrunMigrationfunction is created and theuseEffectfires again. WithautoRun: truethis means:- Mount → migration starts ✓
isRunningbecomestrue→ newrunMigration→ effect re-fires → returns early ("already running") ✓isRunningbecomesfalse→ newrunMigration→ effect re-fires → returns early (attemptedRefguard) ✓
The guards prevent a second real execution, but the pattern is fragile: any change to those guards could let a duplicate run slip through. Because
isRunningis only needed to reject concurrent calls, a ref is a better fit here — it survives re-renders without changing the callback identity.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts Line: 109-112 Comment: **`isRunning` in `useCallback` deps re-fires the migration effect** `isRunning` is listed as a dependency of `runMigration`, so every time the migration changes that state (false → true on start, true → false on finish) a new `runMigration` function is created and the `useEffect` fires again. With `autoRun: true` this means: 1. Mount → migration starts ✓ 2. `isRunning` becomes `true` → new `runMigration` → effect re-fires → returns early ("already running") ✓ 3. `isRunning` becomes `false` → new `runMigration` → effect re-fires → returns early (`attemptedRef` guard) ✓ The guards prevent a second real execution, but the pattern is fragile: any change to those guards could let a duplicate run slip through. Because `isRunning` is only needed to reject concurrent calls, a ref is a better fit here — it survives re-renders without changing the callback identity. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts
Line: 109-112
Comment:
**`isRunning` in `useCallback` deps re-fires the migration effect**
`isRunning` is listed as a dependency of `runMigration`, so every time the migration changes that state (false → true on start, true → false on finish) a new `runMigration` function is created and the `useEffect` fires again. With `autoRun: true` this means:
1. Mount → migration starts ✓
2. `isRunning` becomes `true` → new `runMigration` → effect re-fires → returns early ("already running") ✓
3. `isRunning` becomes `false` → new `runMigration` → effect re-fires → returns early (`attemptedRef` guard) ✓
The guards prevent a second real execution, but the pattern is fragile: any change to those guards could let a duplicate run slip through. Because `isRunning` is only needed to reject concurrent calls, a ref is a better fit here — it survives re-renders without changing the callback identity.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx
Line: 490-517
Comment:
**Support report duplicates every error entry**
`buildMigrationSupportReport` collects entries from both `summary.errors` **and** the filtered `summary.projects` / `summary.workspaces` lists. Because `addProjectError` and `addWorkspaceError` push to *both* lists, every error is serialised twice — once as `"project: name - message"` from `summary.errors` and again as `"project: name - reason"` from the projects filter (and identically for workspaces). With the 20-entry `slice` cap this halves the effective information density of the report when there are many errors.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/project/handlers.ts
Line: 40-49
Comment:
**`isSlugConflict` uses fragile message-string matching**
The function matches on substrings like `"unique constraint"` and `"duplicate key"`, which are not stable across database drivers, ORM versions, or custom error messages. A constraint violation on a *different* unique column (e.g. `org_id + name`) would also match and trigger an incorrect retry loop. If the cloud API wraps the DB error in a structured TRPC response, checking `trpcCode(err)` for a canonical code (e.g. `"CONFLICT"`) would be more reliable.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Update migration modal dither color" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+490
to
+517
| (project) => | ||
| `project: ${project.name} - ${project.reason ?? "unknown error"}`, | ||
| ), | ||
| ...summary.workspaces | ||
| .filter( | ||
| (workspace) => | ||
| workspace.status === "error" || workspace.status === "skipped", | ||
| ) | ||
| .map( | ||
| (workspace) => | ||
| `workspace: ${workspace.name} (${workspace.branch}) - ${workspace.reason ?? workspace.status}`, | ||
| ), | ||
| ]; | ||
|
|
||
| if (relevantEntries.length > 0) { | ||
| lines.push( | ||
| "", | ||
| "Migration errors and skipped items:", | ||
| ...relevantEntries | ||
| .slice(0, 20) | ||
| .map((entry) => `- ${truncateSupportLine(entry)}`), | ||
| ); | ||
| if (relevantEntries.length > 20) { | ||
| lines.push(`- ${relevantEntries.length - 20} more item(s) not included`); | ||
| } | ||
| } | ||
|
|
||
| return lines.join("\n"); |
There was a problem hiding this comment.
buildMigrationSupportReport collects entries from both summary.errors and the filtered summary.projects / summary.workspaces lists. Because addProjectError and addWorkspaceError push to both lists, every error is serialised twice — once as "project: name - message" from summary.errors and again as "project: name - reason" from the projects filter (and identically for workspaces). With the 20-entry slice cap this halves the effective information density of the report when there are many errors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx
Line: 490-517
Comment:
**Support report duplicates every error entry**
`buildMigrationSupportReport` collects entries from both `summary.errors` **and** the filtered `summary.projects` / `summary.workspaces` lists. Because `addProjectError` and `addWorkspaceError` push to *both* lists, every error is serialised twice — once as `"project: name - message"` from `summary.errors` and again as `"project: name - reason"` from the projects filter (and identically for workspaces). With the 20-entry `slice` cap this halves the effective information density of the report when there are many errors.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+40
to
+49
| lower.includes("duplicate key") || | ||
| lower.includes("unique constraint") | ||
| ); | ||
| } | ||
|
|
||
| async function createCloudProjectWithSlugRetry( | ||
| ctx: HostServiceContext, | ||
| args: { name: string; repoCloneUrl?: string }, | ||
| ) { | ||
| const baseSlug = slugifyProjectName(args.name); |
There was a problem hiding this comment.
isSlugConflict uses fragile message-string matching
The function matches on substrings like "unique constraint" and "duplicate key", which are not stable across database drivers, ORM versions, or custom error messages. A constraint violation on a different unique column (e.g. org_id + name) would also match and trigger an incorrect retry loop. If the cloud API wraps the DB error in a structured TRPC response, checking trpcCode(err) for a canonical code (e.g. "CONFLICT") would be more reliable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/project/handlers.ts
Line: 40-49
Comment:
**`isSlugConflict` uses fragile message-string matching**
The function matches on substrings like `"unique constraint"` and `"duplicate key"`, which are not stable across database drivers, ORM versions, or custom error messages. A constraint violation on a *different* unique column (e.g. `org_id + name`) would also match and trigger an incorrect retry loop. If the cloud API wraps the DB error in a structured TRPC response, checking `trpcCode(err)` for a canonical code (e.g. `"CONFLICT"`) would be more reliable.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 29 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="apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts:139">
P1: Including `isRunning` in `runMigration` callback dependencies causes the autorun effect to retrigger after each failed run, leading to immediate retry loops instead of waiting for next launch.</violation>
</file>
<file name="apps/electric-proxy/src/index.ts">
<violation number="1" location="apps/electric-proxy/src/index.ts:97">
P2: Do not return raw internal error messages in the 500 response; this leaks implementation details to clients.</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:302">
P2: `deleteFromHost` leaks cross-organization project existence by querying by ID before org scoping. Scope the lookup to `organizationId` to avoid this enumeration signal.</violation>
</file>
<file name="apps/electric-proxy/src/electric.ts">
<violation number="1" location="apps/electric-proxy/src/electric.ts:37">
P2: Use a truthy fallback for `ELECTRIC_SHAPE_URL` so blank values still fall back to `ELECTRIC_URL`.</violation>
</file>
<file name="packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts:183">
P2: When `existingLocal` is found by branch but has a mismatched `worktreePath`, the by-path lookup is skipped entirely (forced to `null`). If a second stale local row exists for the same `worktreePath` with a different branch, it won't be cleaned up, leaving two local rows pointing at the same worktree path. Consider always running the by-path query (not short-circuiting on `existingLocal`) so path-based stale rows are also reconciled.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx:483">
P2: `buildMigrationSupportReport` duplicates every error entry in the report. `addProjectError`/`addWorkspaceError` push to both `summary.errors` and `summary.projects`/`summary.workspaces`, and this function collects from *all three* sources. Every errored item appears twice in `relevantEntries`, and with the `.slice(0, 20)` cap this halves effective report density. Remove the `summary.projects`/`summary.workspaces` error filters since `summary.errors` already covers them (or vice versa).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return addCorsHeaders(response); | ||
| return addCorsHeaders(response); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); |
Contributor
There was a problem hiding this comment.
P2: Do not return raw internal error messages in the 500 response; this leaks implementation details to clients.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/index.ts, line 97:
<comment>Do not return raw internal error messages in the 500 response; this leaks implementation details to clients.</comment>
<file context>
@@ -34,63 +34,68 @@ function addCorsHeaders(response: Response): Response {
- return addCorsHeaders(response);
+ return addCorsHeaders(response);
+ } catch (err) {
+ const message = err instanceof Error ? err.message : String(err);
+ return corsResponse(500, `Electric proxy error: ${message}`);
+ }
</file context>
| Boolean(env.ELECTRIC_SOURCE_ID) && Boolean(env.ELECTRIC_SOURCE_SECRET); | ||
|
|
||
| const upstream = new URL(env.ELECTRIC_SHAPE_URL ?? ""); | ||
| const shapeUrl = env.ELECTRIC_SHAPE_URL ?? env.ELECTRIC_URL; |
Contributor
There was a problem hiding this comment.
P2: Use a truthy fallback for ELECTRIC_SHAPE_URL so blank values still fall back to ELECTRIC_URL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/electric.ts, line 37:
<comment>Use a truthy fallback for `ELECTRIC_SHAPE_URL` so blank values still fall back to `ELECTRIC_URL`.</comment>
<file context>
@@ -34,7 +34,11 @@ export function buildUpstreamUrl(
Boolean(env.ELECTRIC_SOURCE_ID) && Boolean(env.ELECTRIC_SOURCE_SECRET);
- const upstream = new URL(env.ELECTRIC_SHAPE_URL ?? "");
+ const shapeUrl = env.ELECTRIC_SHAPE_URL ?? env.ELECTRIC_URL;
+ if (!shapeUrl) {
+ throw new Error("Missing ELECTRIC_SHAPE_URL or ELECTRIC_URL");
</file context>
Suggested change
| const shapeUrl = env.ELECTRIC_SHAPE_URL ?? env.ELECTRIC_URL; | |
| const shapeUrl = env.ELECTRIC_SHAPE_URL || env.ELECTRIC_URL; |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/trpc/router/workspace-creation/shared/branch-search.ts (1)
152-156:⚠️ Potential issue | 🟡 MinorStale function name in log. This
console.warnstill referencesfindWorktreeAtPath, but the failure path now lives ingetWorktreeBranchAtPath(the wrapper just compares its result). Update the tag to make grepping logs accurate.♻️ Proposed log tag fix
} catch (err) { console.warn( - "[workspace-creation] git worktree list failed in findWorktreeAtPath:", + "[workspace-creation] git worktree list failed in getWorktreeBranchAtPath:", err, ); }🤖 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-creation/shared/branch-search.ts` around lines 152 - 156, The console.warn message uses the stale function name "findWorktreeAtPath" — update the log tag to reference the current function "getWorktreeBranchAtPath" (and optionally include surrounding context like "git worktree list failed in getWorktreeBranchAtPath") so logs are accurate; locate the console.warn in branch-search.ts and replace the string tag while keeping the err passed through unchanged.
🧹 Nitpick comments (6)
packages/trpc/src/router/v2-workspace/v2-workspace.ts (1)
136-159: LGTM — consider mirroring the JWT rationale comment from sibling procedures.The org check + dual-key WHERE clause (
idANDorganizationId) is correct and consistent withcreateanddeletein this file; cross-org reads are not possible. Returning the full row (vs. the{id, organizationId}projection used by the scoped helpers) is appropriate since host-service needs the full workspace state for migration reconciliation.Optional nit:
delete(line 280-283) andupdateNameFromHost(line 220-225) both carry a short comment explaining whyjwtProcedureis used instead ofprotectedProcedure. Adding a similar one-liner here would keep the host-service-callable surface uniformly self-documenting.📝 Optional doc tweak
+ // JWT-authed so host-service can fetch workspace state during the + // v1→v2 migration reconciliation without an end-user session. getFromHost: jwtProcedure🤖 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 - 159, Add a one-line comment above the getFromHost jwtProcedure (mirroring the comments on delete and updateNameFromHost) explaining why jwtProcedure is used instead of protectedProcedure—e.g., that host-service calls this endpoint using a JWT and cross-org access is guarded by the explicit organizationId check—so the host-callable surface is self-documented; place the comment immediately above getFromHost for consistency with the other procedures.packages/trpc/src/router/v2-project/v2-project.ts (1)
288-315: Optional: scopeDELETEtoorganizationIdfor defense-in-depth.The auth check above is sufficient, but constraining the DELETE itself to both
idandorganizationIdwould harden against any future regressions in the scoping logic and matches whatlinkRepoCloneUrldoes.♻️ Proposed defensive tightening
- await dbWs.delete(v2Projects).where(eq(v2Projects.id, project.id)); + await dbWs + .delete(v2Projects) + .where( + and( + eq(v2Projects.id, project.id), + eq(v2Projects.organizationId, input.organizationId), + ), + );🤖 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 288 - 315, The deleteFromHost mutation currently deletes by id only which risks deleting a project outside the organization if scoping regresses; update the call that performs the deletion (the dbWs.delete(v2Projects).where(...)) to constrain the WHERE to both v2Projects.id and v2Projects.organizationId (use input.organizationId or project.organizationId) so the DELETE is scoped to the organization for defense-in-depth; keep the existing auth checks and error handling in deleteFromHost.packages/host-service/src/trpc/router/project/utils/resolve-repo.test.ts (1)
28-36: Optional: assert full normalized shape, not just the SSH→HTTPS happy path.Consider adding coverage for a non-GitHub remote (e.g.
https://gitlab.com/acme/example.git) to lock in thatparsedreturnsnullwhileremoteNameis still populated — that's a meaningful branch for the new local-only flow and prevents regressions inparseGitHubRemotefrom silently making non-GitHub remotes look like GitHub.🤖 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/utils/resolve-repo.test.ts` around lines 28 - 36, Add a new unit test alongside the existing "returns origin when a GitHub origin exists" test that uses resolveLocalRepo to verify behavior for a non-GitHub remote: create a repo remote like "https://gitlab.com/acme/example.git" (or similar), call resolveLocalRepo(repo), then assert that resolved.remoteName equals the remote name (e.g., "origin") and that resolved.parsed is null to lock in the non-GitHub branch handled by parseGitHubRemote and prevent regressions.packages/host-service/src/trpc/router/workspace-creation/schemas.ts (1)
103-104: LGTM — optional adoption fields wired through schema.Both fields are optional and backward-compatible. If workspace IDs are UUIDs in this codebase (as suggested by
z.string().uuid()usage infindByPath/get/setupabove), consider tighteningexistingWorkspaceIdtoz.string().uuid()so a malformed id fails fast at the boundary instead of insideadopt.ts.🤖 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-creation/schemas.ts` around lines 103 - 104, The schema currently defines existingWorkspaceId as z.string().optional(); tighten it to validate UUIDs by changing the schema entry for existingWorkspaceId to z.string().uuid().optional() so malformed IDs are rejected at the API boundary; update any call sites or tests that pass non-UUIDs if necessary and ensure adopt.ts (the adoption flow) continues to accept the validated UUID shape (no internal parsing changes needed other than relying on the schema-guaranteed UUID).apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx (1)
229-275: Multi-component file expanded further.
WelcomePage,DitheredBackground, andResultsPageare added to the same file asV1MigrationSummaryModal, alongsideExpandableSummaryRow/EntryList/Entry. Per the repo guideline, each should live in its own file (and per the*.tsxrule, in its own folder with a barrel export). Splitting them out also makes the lazyDitheringboundary cleaner.Suggested layout:
V1MigrationSummaryModal/ V1MigrationSummaryModal.tsx index.ts WelcomePage/ DitheredBackground/ ResultsPage/ ExpandableSummaryRow/ Entry/ EntryList/ utils/ // entryTone, countByStatus, buildMigrationSupportReport, etc.As per coding guidelines: "Do not create multi-component files; use one component per file." and "Use one folder per component with structure:
ComponentName/ComponentName.tsx+index.ts".Also applies to: 277-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx` around lines 229 - 275, The file has multiple top-level React components (WelcomePage, DitheredBackground, ResultsPage, ExpandableSummaryRow, EntryList, Entry) alongside V1MigrationSummaryModal which violates the one-component-per-file rule; refactor by moving each component into its own folder named for the component (e.g., WelcomePage/WelcomePage.tsx + index.ts), create a barrel export index.ts for V1MigrationSummaryModal/, and relocate shared utilities (entryTone, countByStatus, buildMigrationSupportReport, etc.) into a utils/ module; update imports in V1MigrationSummaryModal.tsx to lazy-load Dithering via the new DitheredBackground component to keep the Suspense boundary clean and ensure all exported symbols (WelcomePage, DitheredBackground, ResultsPage, ExpandableSummaryRow, EntryList, Entry) are imported from their new index barrel files.apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts (1)
63-145:isRunninginuseCallbackdeps churns the auto-run effect.Including
isRunninginrunMigration's dep array means everysetIsRunning(true|false)produces a new callback identity, which retriggers theuseEffect([autoRun, runMigration]). The effect short-circuits viaattemptedRef/sessionStorage so it's not a correctness bug, but it adds a redundantrunMigration({ manual: false })call on every run completion and makes the data-flow harder to reason about. A ref-backed running flag avoids this.♻️ Use a ref for the running gate
const [isRunning, setIsRunning] = useState(false); + const isRunningRef = useRef(false); ... - async ({ manual }: { manual: boolean }): Promise<MigrationRunResult> => { + async ({ manual }: { manual: boolean }): Promise<MigrationRunResult> => { ... - if (isRunning) { + if (isRunningRef.current) { return { completed: false, reason: "Migration is already running" }; } ... + isRunningRef.current = true; setIsRunning(true); try { ... } finally { + isRunningRef.current = false; setIsRunning(false); } }, - [activeHostUrl, collections, isRunning, isV2CloudEnabled, organizationId], + [activeHostUrl, collections, isV2CloudEnabled, organizationId], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts` around lines 63 - 145, The runMigration callback should stop depending on the isRunning state because setIsRunning changes its identity and churns the auto-run effect; introduce a ref (e.g., isRunningRef = useRef(false)) and change the running gate check from isRunning to isRunningRef.current inside runMigration, update isRunningRef.current = true/false in the same places you call setIsRunning(true/false) (try/finally) so UI state still updates, remove isRunning from the useCallback dependency array, and keep useEffect([autoRun, runMigration]) as-is so auto-run no longer retriggers on each setIsRunning.
🤖 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/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx`:
- Around line 314-336: The contactSupport function currently opens the mailto
fallback unconditionally after a failed send even if the clipboard copy also
failed; change the control flow so openUrl.mutate(buildMigrationSupportMailto())
is called only when there is actually a payload (i.e., after a successful copy
or when you deliberately want to open mailto on successful send), by moving the
openUrl call into the successful-copy branch (or into the send-success branch if
desired) and removing it from the outer catch; update contactSupport, which uses
buildMigrationSupportReport, apiTrpcClient.support.sendMigrationReport.mutate,
copyText.mutateAsync, and setIsSendingSupportReport, so that when both send and
copy fail you only show the toast.error and do not open the mailto window.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsx`:
- Around line 122-141: The current success message logic in ExperimentalSettings
(the code building the migration summary string using summary and the changed
variable) treats runs with only skipped items as changes because changed
includes summary.workspacesSkipped; update the logic so skipped-only runs are
handled distinctly: either remove summary.workspacesSkipped from the changed
calculation (compute changedWithoutSkipped = summary.projectsCreated +
summary.projectsLinked + summary.projectsErrored + summary.workspacesCreated +
summary.workspacesErrored) and then if changedWithoutSkipped === 0 &&
summary.workspacesSkipped > 0 return a clear "No changes — X workspaces skipped"
message, or keep changed as-is but add an explicit branch before the final
return that checks if projectsCreated+projectsLinked === 0 && workspacesCreated
=== 0 && summary.workspacesSkipped > 0 and returns a descriptive skipped-only
message; refer to the variables summary.workspacesSkipped,
summary.projectsCreated, summary.projectsLinked, summary.workspacesCreated and
the function that returns the migration string to locate where to apply the
change.
In `@apps/electric-proxy/src/index.ts`:
- Around line 96-99: The catch block in the request handler currently returns
the raw error message to the client; instead, log the full error server-side
(e.g., console.error(err) or an existing logger) so failures in verifyJWT,
buildWhereClause, buildUpstreamUrl, or upstream fetch are observable, and return
a generic 500 body like "Internal Server Error" via corsResponse(500, ...) to
avoid leaking config or URL details; update the catch around the handler in
index.ts to perform the logging first and then return the generic response.
In `@packages/host-service/src/trpc/router/project/handlers.ts`:
- Around line 35-43: The isSlugConflict helper currently treats any "duplicate
key"/"unique constraint" as a slug conflict and causes inappropriate slug-retry
loops; update isSlugConflict (used by the slug-retry logic) to only return true
for the explicit slug index name (v2_projects_org_slug_unique) or a specific
TRPC/DB error code that identifies a projects.slug unique violation, and return
false for other unique violations so they immediately rethrow; also ensure the
retry loop surfaces that retries occurred by including retry count / lastError
context when rethrowing so the real cause is not masked (refer to isSlugConflict
and the slug-generation retry loop where lastError is thrown).
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts`:
- Around line 138-164: The existingWorkspaceId early-return path persists a
local workspace without cleaning stale local rows, which can leave colliding
rows for the same (projectId, branch, worktreePath); before calling
persistLocalWorkspace in the block that handles input.existingWorkspaceId (after
getHostWorkspace returns a match and before
recordBaseBranch/persistLocalWorkspace), perform the same dedup/cleanup
performed in the non-existing path: delete any local rows that conflict by
(projectId, branch) and (projectId, worktreePath) so the upsert by workspaces.id
won’t create ambiguous duplicate local rows; keep error handling around
persistLocalWorkspace and then return adoptResult(existingCloud) as before.
In `@packages/trpc/src/router/support/support.ts`:
- Around line 19-36: user.name is interpolated into userLabel and used in the
email body, allowing CR/LF injection; sanitize the name before formatting by
removing control characters (e.g., CR and LF) and use the sanitized value when
building userLabel and the email text in the resend.emails.send call
(references: user, userLabel, organizationId, input.report, resend.emails.send,
SUPPORT_EMAIL); ensure replyTo still uses user.email and that only the sanitized
name is used in the message body.
- Around line 11-44: sendMigrationReport currently allows any authenticated user
to send unlimited reports; add a per-user rate limit (1 report per minute) using
the same `@upstash/ratelimit` sliding-window approach as in
packages/auth/src/lib/rate-limit.ts. In the supportRouter file, import or
instantiate the same rate limit helper and, inside the sendMigrationReport
mutation (before calling resend.emails.send), check the limit using the user's
unique key (e.g., user.id) and return/throw a TRPCError with code
"TOO_MANY_REQUESTS" if the check fails; only proceed to call resend.emails.send
when the rate check succeeds. Ensure you reference sendMigrationReport,
supportRouter, protectedProcedure, and use the same sliding window configuration
used in the existing rate-limit implementation.
---
Outside diff comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/shared/branch-search.ts`:
- Around line 152-156: The console.warn message uses the stale function name
"findWorktreeAtPath" — update the log tag to reference the current function
"getWorktreeBranchAtPath" (and optionally include surrounding context like "git
worktree list failed in getWorktreeBranchAtPath") so logs are accurate; locate
the console.warn in branch-search.ts and replace the string tag while keeping
the err passed through unchanged.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx`:
- Around line 229-275: The file has multiple top-level React components
(WelcomePage, DitheredBackground, ResultsPage, ExpandableSummaryRow, EntryList,
Entry) alongside V1MigrationSummaryModal which violates the
one-component-per-file rule; refactor by moving each component into its own
folder named for the component (e.g., WelcomePage/WelcomePage.tsx + index.ts),
create a barrel export index.ts for V1MigrationSummaryModal/, and relocate
shared utilities (entryTone, countByStatus, buildMigrationSupportReport, etc.)
into a utils/ module; update imports in V1MigrationSummaryModal.tsx to lazy-load
Dithering via the new DitheredBackground component to keep the Suspense boundary
clean and ensure all exported symbols (WelcomePage, DitheredBackground,
ResultsPage, ExpandableSummaryRow, EntryList, Entry) are imported from their new
index barrel files.
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts`:
- Around line 63-145: The runMigration callback should stop depending on the
isRunning state because setIsRunning changes its identity and churns the
auto-run effect; introduce a ref (e.g., isRunningRef = useRef(false)) and change
the running gate check from isRunning to isRunningRef.current inside
runMigration, update isRunningRef.current = true/false in the same places you
call setIsRunning(true/false) (try/finally) so UI state still updates, remove
isRunning from the useCallback dependency array, and keep useEffect([autoRun,
runMigration]) as-is so auto-run no longer retriggers on each setIsRunning.
In `@packages/host-service/src/trpc/router/project/utils/resolve-repo.test.ts`:
- Around line 28-36: Add a new unit test alongside the existing "returns origin
when a GitHub origin exists" test that uses resolveLocalRepo to verify behavior
for a non-GitHub remote: create a repo remote like
"https://gitlab.com/acme/example.git" (or similar), call resolveLocalRepo(repo),
then assert that resolved.remoteName equals the remote name (e.g., "origin") and
that resolved.parsed is null to lock in the non-GitHub branch handled by
parseGitHubRemote and prevent regressions.
In `@packages/host-service/src/trpc/router/workspace-creation/schemas.ts`:
- Around line 103-104: The schema currently defines existingWorkspaceId as
z.string().optional(); tighten it to validate UUIDs by changing the schema entry
for existingWorkspaceId to z.string().uuid().optional() so malformed IDs are
rejected at the API boundary; update any call sites or tests that pass non-UUIDs
if necessary and ensure adopt.ts (the adoption flow) continues to accept the
validated UUID shape (no internal parsing changes needed other than relying on
the schema-guaranteed UUID).
In `@packages/trpc/src/router/v2-project/v2-project.ts`:
- Around line 288-315: The deleteFromHost mutation currently deletes by id only
which risks deleting a project outside the organization if scoping regresses;
update the call that performs the deletion (the
dbWs.delete(v2Projects).where(...)) to constrain the WHERE to both v2Projects.id
and v2Projects.organizationId (use input.organizationId or
project.organizationId) so the DELETE is scoped to the organization for
defense-in-depth; keep the existing auth checks and error handling in
deleteFromHost.
In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts`:
- Around line 136-159: Add a one-line comment above the getFromHost jwtProcedure
(mirroring the comments on delete and updateNameFromHost) explaining why
jwtProcedure is used instead of protectedProcedure—e.g., that host-service calls
this endpoint using a JWT and cross-org access is guarded by the explicit
organizationId check—so the host-callable surface is self-documented; place the
comment immediately above getFromHost for consistency with the other procedures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbe14864-ac60-4db1-9228-2002da6f5e89
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
apps/desktop/package.jsonapps/desktop/src/lib/trpc/routers/migration/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsxapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/electric-proxy/src/electric.tsapps/electric-proxy/src/index.tsapps/electric-proxy/src/types.tspackages/host-service/src/trpc/router/project/handlers.tspackages/host-service/src/trpc/router/project/project.tspackages/host-service/src/trpc/router/project/utils/persist-project.tspackages/host-service/src/trpc/router/project/utils/resolve-repo.test.tspackages/host-service/src/trpc/router/project/utils/resolve-repo.tspackages/host-service/src/trpc/router/workspace-creation/procedures/adopt.tspackages/host-service/src/trpc/router/workspace-creation/schemas.tspackages/host-service/src/trpc/router/workspace-creation/shared/branch-search.test.tspackages/host-service/src/trpc/router/workspace-creation/shared/branch-search.tspackages/trpc/package.jsonpackages/trpc/src/env.tspackages/trpc/src/root.tspackages/trpc/src/router/support/support.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx
Comment on lines
+96
to
+99
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| return corsResponse(500, `Electric proxy error: ${message}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid echoing raw error messages to clients, and log them server-side.
The catch arm returns Electric proxy error: ${message} directly in the response body. Two concerns:
- Information disclosure / DX confusion.
messagemay contain configuration names (e.g.Missing ELECTRIC_SHAPE_URL or ELECTRIC_URLfrombuildUpstreamUrl), upstream URL fragments, orTypeErrortext fromnew URL(...). This is exposed to any caller hitting the public proxy. Consider returning a generic body (e.g."Internal Server Error") and only logging the detail. - No observability. Cloudflare Workers won't surface the failure anywhere unless you explicitly
console.errorit; right now an exception inverifyJWT,buildWhereClause,buildUpstreamUrl, or upstreamfetchwill silently turn into a 500 with no trail.
🛡️ Suggested change
} catch (err) {
- const message = err instanceof Error ? err.message : String(err);
- return corsResponse(500, `Electric proxy error: ${message}`);
+ console.error("Electric proxy error", err);
+ return corsResponse(500, "Internal Server Error");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| return corsResponse(500, `Electric proxy error: ${message}`); | |
| } | |
| } catch (err) { | |
| console.error("Electric proxy error", err); | |
| return corsResponse(500, "Internal Server Error"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electric-proxy/src/index.ts` around lines 96 - 99, The catch block in
the request handler currently returns the raw error message to the client;
instead, log the full error server-side (e.g., console.error(err) or an existing
logger) so failures in verifyJWT, buildWhereClause, buildUpstreamUrl, or
upstream fetch are observable, and return a generic 500 body like "Internal
Server Error" via corsResponse(500, ...) to avoid leaking config or URL details;
update the catch around the handler in index.ts to perform the logging first and
then return the generic response.
Contributor
There was a problem hiding this comment.
2 issues found across 7 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-creation/procedures/adopt.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts:223">
P2: Missing `deleteLocalWorkspaceConflicts` call in the `existingLocalByPath` + cloud-exists branch can leave two local rows sharing the same `(projectId, branch)`. The `existingWorkspaceId` path and the final create path both call `deleteLocalWorkspaceConflicts`, but this path updates the row's branch without first removing the stale row that already holds that branch, causing ambiguous `findFirst` results on rerun.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx:322">
P2: When report submission fails, the fallback only copies text and no longer opens an email draft, so the "Contact us" flow can dead-end for users who expected direct support handoff.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| console.warn("[v1-migration] Failed to send support report:", error); | ||
| try { | ||
| await copyText.mutateAsync(report); | ||
| toast.success("Migration details copied to clipboard"); |
Contributor
There was a problem hiding this comment.
P2: When report submission fails, the fallback only copies text and no longer opens an email draft, so the "Contact us" flow can dead-end for users who expected direct support handoff.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx, line 322:
<comment>When report submission fails, the fallback only copies text and no longer opens an email draft, so the "Contact us" flow can dead-end for users who expected direct support handoff.</comment>
<file context>
@@ -321,15 +319,14 @@ function ResultsPage({
try {
await copyText.mutateAsync(report);
- toast.success("Migration details copied for your email");
+ toast.success("Migration details copied to clipboard");
} catch (copyError) {
console.warn(
</file context>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/host-service/src/trpc/router/project/handlers.ts (1)
89-100: Consider adding error handling to prevent orphaned cloud projects.If
persistLocalProjectthrows after the cloud project is successfully created, you'll leave a project stranded with no local row. While this aligns with the PR's "cloud is durable" stance, aconsole.warn(matching the style elsewhere in the file) would improve debuggability:export async function createFromImportLocal( ctx: HostServiceContext, args: { name: string; repoPath: string }, ): Promise<CreateResult> { const resolved = await resolveLocalRepo(args.repoPath); const cloudProject = await createCloudProjectWithSlugRetry(ctx, { name: args.name, repoCloneUrl: resolved.parsed?.url, }); - persistLocalProject(ctx, cloudProject.id, resolved); - return { projectId: cloudProject.id, repoPath: resolved.repoPath }; + try { + persistLocalProject(ctx, cloudProject.id, resolved); + } catch (err) { + console.warn( + "[project.createFromImportLocal] cloud project created but local persistence failed; rerun will need to relink", + { projectId: cloudProject.id, repoPath: resolved.repoPath, err }, + ); + throw err; + } + return { projectId: cloudProject.id, repoPath: resolved.repoPath }; }🤖 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/handlers.ts` around lines 89 - 100, Wrap the call to persistLocalProject inside a try/catch in createFromImportLocal so failures don't silently disappear: after creating the cloud project via createCloudProjectWithSlugRetry (cloudProject.id), call persistLocalProject in a try block and in the catch log a console.warn (matching file style) that includes the cloudProject.id, repoPath and the caught error for debuggability, then rethrow the error so callers still see the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/host-service/src/trpc/router/project/handlers.ts`:
- Around line 89-100: Wrap the call to persistLocalProject inside a try/catch in
createFromImportLocal so failures don't silently disappear: after creating the
cloud project via createCloudProjectWithSlugRetry (cloudProject.id), call
persistLocalProject in a try block and in the catch log a console.warn (matching
file style) that includes the cloudProject.id, repoPath and the caught error for
debuggability, then rethrow the error so callers still see the failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d6bc407-785a-4d94-97e3-1fc309071e88
📒 Files selected for processing (1)
packages/host-service/src/trpc/router/project/handlers.ts
Contributor
There was a problem hiding this comment.
1 issue found across 2 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/project/handlers.ts">
<violation number="1" location="packages/host-service/src/trpc/router/project/handlers.ts:74">
P1: `persistLocalProject` is no longer rollback-protected here; a local persistence failure leaves an orphaned cloud project.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| slug: slugifyProjectName(args.name), | ||
| repoCloneUrl: args.url, | ||
| }); | ||
| persistLocalProject(ctx, cloudProject.id, resolved); |
Contributor
There was a problem hiding this comment.
P1: persistLocalProject is no longer rollback-protected here; a local persistence failure leaves an orphaned cloud project.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/project/handlers.ts, line 74:
<comment>`persistLocalProject` is no longer rollback-protected here; a local persistence failure leaves an orphaned cloud project.</comment>
<file context>
@@ -95,7 +71,7 @@ export async function createFromClone(
repoCloneUrl: args.url,
});
- await persistProjectOrRollbackCloud(ctx, cloudProject.id, resolved);
+ persistLocalProject(ctx, cloudProject.id, resolved);
return { projectId: cloudProject.id, repoPath: resolved.repoPath };
} catch (err) {
</file context>
Suggested change
| persistLocalProject(ctx, cloudProject.id, resolved); | |
| try { | |
| persistLocalProject(ctx, cloudProject.id, resolved); | |
| } catch (err) { | |
| await ctx.api.v2Project.deleteFromHost | |
| .mutate({ organizationId: ctx.organizationId, id: cloudProject.id }) | |
| .catch((cleanupErr) => { | |
| console.warn( | |
| "[project.create] failed to rollback cloud project after local persistence error", | |
| { projectId: cloudProject.id, cleanupErr }, | |
| ); | |
| }); | |
| throw err; | |
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/trpc/router/project/handlers.ts (1)
90-118:⚠️ Potential issue | 🟡 Minor
createFromClonedeletes the clone on persist failure but leaves the cloud project orphaned — and the warn text misrepresents this.When
persistLocalProjectOrWarnrethrows on Line 100, control falls into the catch (Lines 107-117) whichrmSyncsresolved.repoPath. The cloud project is preserved by design (per the file-level docstring), but:
- The warning logged inside
persistLocalProjectOrWarnsays "rerun will need to relink" — yet by the time the user reruns, the local clone has been deleted, so there is nothing local to relink to. That message is accurate forcreateFromImportLocal(user-owned path) but misleading here.- Nothing in this handler attempts to look up the just-created cloud project on rerun, so a second
createFromClonewith the same name will simply allocate<slug>-2via the retry loop, leaving the original cloud project orphaned (no local link, no local clone, no GC path visible from this file).Either the cleanup should be skipped when the failure originates from
persistLocalProject(so the local clone survives for relinking), or the warn message should be reworded to reflect that the caller must reconcile/garbage-collect the dangling cloud project. If higher-level migration reconciliation handles orphan cloud projects, a brief code comment pointing at it would prevent future confusion.♻️ One option — preserve the clone when only local persistence failed
export async function createFromClone( ctx: HostServiceContext, args: { name: string; parentDir: string; url: string }, ): Promise<CreateResult> { const resolved = await cloneRepoInto(args.url, args.parentDir); + let cloudCreated = false; try { const cloudProject = await createCloudProjectWithSlugRetry(ctx, { name: args.name, repoCloneUrl: args.url, }); + cloudCreated = true; persistLocalProjectOrWarn( ctx, cloudProject.id, resolved, "createFromClone", ); return { projectId: cloudProject.id, repoPath: resolved.repoPath }; } catch (err) { + // Only roll back the clone for pre-cloud failures; once the cloud + // project exists, keep the clone so reconciliation can relink. + if (cloudCreated) throw err; try { rmSync(resolved.repoPath, { recursive: true, force: true }); } catch (cleanupErr) { console.warn( "[project.createFromClone] failed to rollback clone after cloud error", { repoPath: resolved.repoPath, cleanupErr }, ); } throw err; } }🤖 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/handlers.ts` around lines 90 - 118, The current catch in createFromClone deletes the local clone regardless of whether the failure came from persistLocalProjectOrWarn, which leaves the cloud project orphaned while the warn text misleadingly suggests the user can "rerun to relink"; update createFromClone to only remove the cloned repo on non-persistence errors (or when the error is from createCloudProjectWithSlugRetry), and preserve the repo when persistLocalProjectOrWarn failed so the user can relink, or alternatively adjust the warn text inside persistLocalProjectOrWarn to explicitly say the cloud project will remain and must be reconciled/GC'd; add a short comment referencing createCloudProjectWithSlugRetry and persistLocalProjectOrWarn to document which path intentionally leaves a cloud-only project.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsx (1)
96-99: Nit: simplify redundantclassNameternary.Both branches share
h-4 w-4; onlyanimate-spinis conditional.♻️ Suggested simplification
- <LuRefreshCw - className={isRunning ? "h-4 w-4 animate-spin" : "h-4 w-4"} - strokeWidth={2} - /> + <LuRefreshCw + className={`h-4 w-4${isRunning ? " animate-spin" : ""}`} + strokeWidth={2} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsx` around lines 96 - 99, The LuRefreshCw icon uses a redundant ternary for className where "h-4 w-4" is duplicated; simplify by always including "h-4 w-4" and only conditionally add "animate-spin" (e.g., with a template literal or your project's classnames helper) so the className for LuRefreshCw only conditionally appends "animate-spin" when isRunning is true.packages/host-service/src/trpc/router/project/handlers.ts (1)
41-66: Add a breadcrumb when slug retries actually fire.Today the retry loop is silent on success-after-conflict: if attempts 0–8 all hit
v2_projects_org_slug_uniqueand attempt 9 succeeds, there is no signal that the user's project name was effectively renamed to<slug>-10. A singleconsole.warn(or whatever logger this package uses) on each conflict — or at least on the final returned attempt whenattempt > 0— would make name-collision storms diagnosable in production without changing behavior.🤖 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/handlers.ts` around lines 41 - 66, Add a breadcrumb when slug retries occur inside createCloudProjectWithSlugRetry: when isSlugConflict(err) is true (i.e., a retry will be attempted) log a warning that includes the original name (args.name), the attempted slug (slugWithSuffix(baseSlug, attempt)), the attempt number, and the organization id (ctx.organizationId); at minimum emit a log when attempt > 0 just before continuing, using the router/service logger (e.g., ctx.logger.warn) or console.warn if no logger is available so collisions are discoverable without changing retry behavior.
🤖 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/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsx`:
- Around line 147-151: The success summary currently builds a string from
summary.projectsCreated, summary.projectsLinked and summary.workspacesCreated
but omits summary.workspacesSkipped when there are also created/linked items;
update the string construction in ExperimentalSettings (the template that
returns `Migration run completed: ...`) to append the skipped workspaces count
when summary.workspacesSkipped > 0 (for example "1 workspace (+3 skipped)")
consistent with the existing skipped-only branch, using the existing variable
names summary.projectsCreated, summary.projectsLinked, summary.workspacesCreated
and summary.workspacesSkipped.
---
Outside diff comments:
In `@packages/host-service/src/trpc/router/project/handlers.ts`:
- Around line 90-118: The current catch in createFromClone deletes the local
clone regardless of whether the failure came from persistLocalProjectOrWarn,
which leaves the cloud project orphaned while the warn text misleadingly
suggests the user can "rerun to relink"; update createFromClone to only remove
the cloned repo on non-persistence errors (or when the error is from
createCloudProjectWithSlugRetry), and preserve the repo when
persistLocalProjectOrWarn failed so the user can relink, or alternatively adjust
the warn text inside persistLocalProjectOrWarn to explicitly say the cloud
project will remain and must be reconciled/GC'd; add a short comment referencing
createCloudProjectWithSlugRetry and persistLocalProjectOrWarn to document which
path intentionally leaves a cloud-only project.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsx`:
- Around line 96-99: The LuRefreshCw icon uses a redundant ternary for className
where "h-4 w-4" is duplicated; simplify by always including "h-4 w-4" and only
conditionally add "animate-spin" (e.g., with a template literal or your
project's classnames helper) so the className for LuRefreshCw only conditionally
appends "animate-spin" when isRunning is true.
In `@packages/host-service/src/trpc/router/project/handlers.ts`:
- Around line 41-66: Add a breadcrumb when slug retries occur inside
createCloudProjectWithSlugRetry: when isSlugConflict(err) is true (i.e., a retry
will be attempted) log a warning that includes the original name (args.name),
the attempted slug (slugWithSuffix(baseSlug, attempt)), the attempt number, and
the organization id (ctx.organizationId); at minimum emit a log when attempt > 0
just before continuing, using the router/service logger (e.g., ctx.logger.warn)
or console.warn if no logger is available so collisions are discoverable without
changing retry behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cadf3cde-c4a1-4f6b-a0d1-f75cfdcd5276
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsxpackages/host-service/src/trpc/router/project/handlers.tspackages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts
✅ Files skipped from review due to trivial changes (1)
- packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts
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
Context
This branch also merges
origin/mainafter Kiet's host/settings resiliency work landed, so the PR is tested against the current mainline shape.Validation
bun --cwd apps/desktop typecheckbun --cwd packages/trpc typecheckbun --cwd packages/host-service typecheckbun --cwd apps/electric-proxy typecheckbun test apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.tsbun test packages/host-service/src/trpc/router/workspace-creation/shared/branch-search.test.ts packages/host-service/src/trpc/router/project/utils/resolve-repo.test.tsgit diff --checkSummary by cubic
Stabilizes the v1 → v2 migration with safe reruns, clearer results, and support reporting. Hardens project/workspace adoption so reruns are safe and local/cloud state stays consistent.
Migration
existingWorkspaceId; passes worktreebaseBranch; reads the actual branch at a worktree path; clearer skip reasons.support.sendMigrationReportwith Upstash rate limiting (3/hour) and clipboard fallback.useMigrateV1DataToV2exposesrerun/isRunningand returns a session‑safe result; summary only shown on manual runs or if changes occurred.Host-Service Hardening
resolveLocalReposupports repos without GitHub;project.findByPathreturns local matches; cloud creation retries slug on conflict.baseBranchandexistingWorkspaceId, reads the branch at a worktree path, relinks/renames when needed, removes local conflicts, and normalizes worktree path detection; addsv2Workspace.getFromHost.support.sendMigrationReport; requireRESEND_API_KEYand Upstash KV; new deps@paper-design/shaders-react,resend,@upstash/ratelimit,@upstash/redis.Written for commit 40f317a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests