[codex] Harden v1 to v2 migration idempotency#3781
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMigration now opportunistically uses v1 worktree paths for adoption, records path-specific NOT_FOUND skips as Changes
Sequence DiagramsequenceDiagram
participant Migrator as Migration Process
participant WorktreeStore as v1 Worktrees (worktreesById)
participant AdoptionSvc as workspaceCreation.adopt
participant SkipStore as skippedWorkspaces store
Migrator->>WorktreeStore: lookup v1 worktree (id -> path, baseBranch)
alt worktree path found
Migrator->>AdoptionSvc: adopt(workspaceId, worktreePath, baseBranch)
alt AdoptionSvc -> Migrator: NOT_FOUND
Migrator->>SkipStore: upsert skipped as `worktree_not_registered`
Migrator->>AdoptionSvc: adopt(workspaceId, baseBranch) -- retry without path
AdoptionSvc-->>Migrator: success / error
Migrator->>SkipStore: record result (adopted or still skipped)
else AdoptionSvc -> Migrator: success
Migrator->>SkipStore: record adopted mapping
end
else no worktree path
Migrator->>AdoptionSvc: adopt(workspaceId, baseBranch) -- branch-based adoption
AdoptionSvc-->>Migrator: success / error
Migrator->>SkipStore: record result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Greptile SummaryThis PR hardens v1→v2 migration idempotency by: (1) passing Confidence Score: 5/5Safe to merge — all changes are well-scoped migration hardening with matching regression tests and no P0/P1 issues found. The logic is correct across all edge cases: the two-attempt adoption (path → branch) handles every NOT_FOUND permutation properly, the shouldRetryWorkspace expansion is consistent with the new retry semantics, and allowRelocate: true is appropriate for the migration context. Four targeted regression tests verify the new paths. No data-loss, auth, or correctness concerns were found. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts | Core migration logic hardened: adds allowRelocate, expands shouldRetryWorkspace to include orphan_worktree/worktree_not_registered, removes hard skip for missing worktree rows, and adds two-attempt adoption with worktree-path-then-branch fallback. |
| apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.ts | Test harness extended with adoptThrowsForPath map and allowRelocate tracking; adds four new regression tests covering missing worktree rows, stale paths, retrying old skipped rows, and relocation-enabled setup. Existing permanent-skip test flipped to verify recovery. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Workspace iteration] --> B{existing.status?}
B -- success + v2Id --> C{hasLocalWorkspace?}
C -- yes --> D[Emit 'synced', continue]
C -- no --> E[recoverCompletedWorkspace = true]
C -- throws --> F[Record error, continue]
B -- other --> G{shouldRetryWorkspace?}
G -- false --> H[Emit existing skipped reason, continue]
G -- true --> I{v2ProjectId resolved?}
E --> I
I -- no --> J[Store parent_project_unresolved, skip]
I -- yes --> K[Resolve v1WorktreePath from worktreesById]
K --> L[adoptWorkspace v1WorktreePath]
L -- success --> M[Store success, increment workspacesCreated]
L -- NOT_FOUND && v1WorktreePath set --> N[Retry adoptWorkspace undefined / branch-only]
N -- success --> M
N -- throws --> O[recordAdoptFailure]
L -- NOT_FOUND && no path --> O
L -- other error --> O
O -- NOT_FOUND --> P[Store worktree_not_registered, skip]
O -- other --> Q[Store error, increment workspacesErrored]
Reviews (1): Last reviewed commit: "Harden v1 to v2 migration idempotency" | Re-trigger Greptile
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
saddlepaddle
force-pushed
the
fix-migration-idempotency
branch
from
bd75760 to
1d6e7b8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts (1)
65-74:⚠️ Potential issue | 🟠 MajorPass
allowRelocate: truetosetupProjectImportcall to enable migration relocation.The PR description explicitly states the feature is "allowing migration project reconciliation to relocate an existing local v2 project back to the v1 repo path (allowRelocate: true)", yet
setupProjectImportat lines 65–74 does not pass this flag. The schema supports it withallowRelocate: z.boolean().default(false), and the test mock was widened to accept it (line 183), signaling intent.The
allowRelocateparameter is used elsewhere in the codebase:
ProjectLocationSection.tsx:72passes it for user-initiated relocationuseFolderFirstImport.ts:86does not (first-time import has no existing project to relocate)For migration reconciliation to work as described, update
setupProjectImportto accept and passallowRelocate: true:Suggested fix
async function setupProjectImport( hostService: HostServiceClient, projectId: string, repoPath: string, + allowRelocate: boolean = true, ): Promise<void> { await hostService.project.setup.mutate({ projectId, - mode: { kind: "import", repoPath }, + mode: { kind: "import", repoPath, allowRelocate }, }); }🤖 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/migrate.ts` around lines 65 - 74, The setupProjectImport function currently calls hostService.project.setup.mutate without the allowRelocate flag; update the function signature (setupProjectImport) to accept an allowRelocate boolean (or default true when used for migration) and pass it into hostService.project.setup.mutate as part of the mode payload (mode: { kind: "import", repoPath, allowRelocate: true }) so the mutation receives allowRelocate: true for migration reconciliation; ensure any callers (where migration is intended) pass true or rely on the default you add.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.ts (2)
183-184: Type widening to optionalrepoPathis broader than needed.The mock now accepts
mode: { repoPath?: string; allowRelocate?: boolean }. None of the production call sites inmigrate.tsactually omitrepoPath, so making it optional in the stub loosens the type contract beyond what the code under test exercises. If the goal was just to permitallowRelocate, you can keeprepoPath: stringrequired. Marginal — only relevant if a future caller dropsrepoPathand the test would no longer flag it.🤖 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/migrate.test.ts` around lines 183 - 184, The test stub relaxes the type for the `mode` parameter to `{ repoPath?: string; allowRelocate?: boolean }`; change it back so `repoPath` is required (i.e. `{ repoPath: string; allowRelocate?: boolean }`) to match production call sites in `migrate.ts` and keep `allowRelocate` optional, by updating the mock/signature in `migrate.test.ts` where the function parameter is declared so the test fails if a caller omits `repoPath`.
921-977: Idempotent re-skip coverage is the missing piece — nice addition.This locks in the
wasAlreadyMissingWorktreeSkipbranch behavior: a previously-skippedworktree_not_registeredworkspace whose retry also fails with NOT_FOUND is reported insummary.workspacesbut not counted inworkspacesSkipped. Without this test, a future refactor could regress and start double-counting reruns as new skips.Two small things that would make the assertion even tighter (optional):
- Assert
env.adoptCallslength to confirm the retry loop ran exactly once (no spurious branch-only re-call on top of an already-pathless first attempt — there's no path here sincev1Worktrees: [], so it should be 1).- Assert
summary.workspaceslength is 1, not just "contains" the expected entry, to guard against duplicate pushes.Proposed tightening
expect(summary.workspacesCreated).toBe(0); expect(summary.workspacesSkipped).toBe(0); + expect(env.adoptCalls).toHaveLength(1); + expect(summary.workspaces).toHaveLength(1); expect(summary.workspaces).toContainEqual({ name: "workspace-w-orphan", branch: "branch-w-orphan", status: "skipped", reason: "worktree no longer exists", });🤖 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/migrate.test.ts` around lines 921 - 977, The test "failed retry of previous missing-worktree skip does not count as new work" is missing two tighter assertions: after calling migrateV1DataToV2, assert env.adoptCalls.length === 1 to ensure the retry loop invoked adopt exactly once, and assert summary.workspaces.length === 1 (instead of only containsEqual) to ensure no duplicate entries were pushed; update the test to include these two assertions referencing migrateV1DataToV2, env.adoptCalls, and summary.workspaces.apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts (1)
413-501: Adoption refactor with NOT_FOUND retry is well-structured.A few things worth noting (non-blocking):
- Outer
trywraps both the path-based attempt and the branch-based retry;recordAdoptFailureis only invoked once with the terminal error — correct, no double-recording.- Retry guard
trpcCode(err) !== "NOT_FOUND" || !v1WorktreePathcorrectly avoids a redundant re-call when the first attempt already had no path.existingWorkspaceIdis preserved across the retry via the closure, so the host-service can still relink an orphaned cloud workspace on the branch-only attempt.- In
recordAdoptFailure, whenwasAlreadyMissingWorktreeSkip(existing)is true, the workspace is pushed tosummary.workspacesbutworkspacesSkippedis intentionally not incremented — this preserves "did nothing new" semantics on idempotent reruns and is covered by the new test at lines 921-977.One minor optional thought:
recordAdoptFailurere-writes the sameworktree_not_registeredrow even whenexistingwas already in that exact state. It's harmless (and arguably simpler than skipping the write), but ifupsertStateis non-trivial you could short-circuit whenexisting.status === "skipped" && existing.reason === "worktree_not_registered". Defer if you prefer the uniform write.🤖 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/migrate.ts` around lines 413 - 501, The current recordAdoptFailure always upserts a worktree_not_registered row even if existing already records that state; short-circuit this by checking inside recordAdoptFailure whether existing?.status === "skipped" && existing.reason === "worktree_not_registered" (or reuse wasAlreadyMissingWorktreeSkip(existing)) and if so skip the electronTrpc.migration.upsertState.mutate call and any summary mutation, returning early; update the recordAdoptFailure function to perform this guard before calling upsertState.mutate so you avoid redundant writes while preserving current behavior when the condition is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts`:
- Around line 65-74: The setupProjectImport function currently calls
hostService.project.setup.mutate without the allowRelocate flag; update the
function signature (setupProjectImport) to accept an allowRelocate boolean (or
default true when used for migration) and pass it into
hostService.project.setup.mutate as part of the mode payload (mode: { kind:
"import", repoPath, allowRelocate: true }) so the mutation receives
allowRelocate: true for migration reconciliation; ensure any callers (where
migration is intended) pass true or rely on the default you add.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.ts`:
- Around line 183-184: The test stub relaxes the type for the `mode` parameter
to `{ repoPath?: string; allowRelocate?: boolean }`; change it back so
`repoPath` is required (i.e. `{ repoPath: string; allowRelocate?: boolean }`) to
match production call sites in `migrate.ts` and keep `allowRelocate` optional,
by updating the mock/signature in `migrate.test.ts` where the function parameter
is declared so the test fails if a caller omits `repoPath`.
- Around line 921-977: The test "failed retry of previous missing-worktree skip
does not count as new work" is missing two tighter assertions: after calling
migrateV1DataToV2, assert env.adoptCalls.length === 1 to ensure the retry loop
invoked adopt exactly once, and assert summary.workspaces.length === 1 (instead
of only containsEqual) to ensure no duplicate entries were pushed; update the
test to include these two assertions referencing migrateV1DataToV2,
env.adoptCalls, and summary.workspaces.
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts`:
- Around line 413-501: The current recordAdoptFailure always upserts a
worktree_not_registered row even if existing already records that state;
short-circuit this by checking inside recordAdoptFailure whether
existing?.status === "skipped" && existing.reason === "worktree_not_registered"
(or reuse wasAlreadyMissingWorktreeSkip(existing)) and if so skip the
electronTrpc.migration.upsertState.mutate call and any summary mutation,
returning early; update the recordAdoptFailure function to perform this guard
before calling upsertState.mutate so you avoid redundant writes while preserving
current behavior when the condition is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bfed2fc-b495-4e03-928c-ce5b5adfd46a
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts
saddlepaddle
force-pushed
the
fix-migration-idempotency
branch
from
1d6e7b8 to
af65edc
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Hardens the desktop v1-to-v2 migration so reruns recover more partial-success cases without repeatedly surfacing stale skipped work as new migration activity.
Root Cause
The migration depended too strongly on v1 worktree path records and treated some skipped states as final. If those records were missing or stale, a rerun could skip a workspace even when host-service could still discover the worktree by branch. The first version of this PR also made project reconciliation relocate local project paths too broadly; that was corrected because relocation has user-visible consequences for existing worktrees.
Validation
bun test apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2bunx tsc -p apps/desktop/tsconfig.json --noEmitbunx @biomejs/biome@2.4.2 check apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.tsgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes