[codex] make V1 migration org-idempotent#3783
Conversation
📝 WalkthroughWalkthroughRemoves the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
saddlepaddle
force-pushed
the
investigate-migration-blo
branch
from
aa7748b to
66312fc
Compare
Greptile SummaryThis PR removes the cross-organization preflight guard from the V1→V2 migration and its backing Confidence Score: 5/5Safe to merge — changes are minimal, well-reasoned, and fully covered by the updated tests. The guard removal is correct because No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/lib/trpc/routers/migration/index.ts | Removes the findMigrationByOtherOrg query endpoint; the remaining router endpoints are unchanged and correct. |
| apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts | Drops otherOrg from the parallel data-fetch and removes the guard that threw when another org had migration state; org isolation already enforced by listState's organizationId filter. |
| apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.ts | Replaces the "other-org guard rejects" test with "other-org state does not block active org" and adds a comprehensive completed-migration rerun regression test. |
Sequence Diagram
sequenceDiagram
participant C as migrate.ts
participant DB as migration router (tRPC)
participant HS as hostService
Note over C,DB: Before: guard would abort here if another org had success rows
C->>DB: listState({ organizationId: ORG }) — org-scoped
DB-->>C: rows for ORG only
loop For each V1 project
alt Row exists (success/linked) for ORG
C->>HS: setupProjectImport(v2Id, repoPath) [idempotent]
C-->>C: report "synced"
else No existing row
C->>HS: project.findByPath(repoPath)
alt Candidate found
C->>HS: setupProjectImport(candidate.id, repoPath)
C->>DB: upsertState(status=linked)
C-->>C: report "linked"
else No candidate
C->>HS: project.create(...)
C->>DB: upsertState(status=success)
C-->>C: report "created"
end
end
end
loop For each V1 workspace
alt success row + workspace exists in host
C-->>C: report "synced"
else retry conditions met
C->>HS: workspaceCreation.adopt(...)
C->>DB: upsertState(status=success)
end
end
Reviews (1): Last reviewed commit: "make v1 migration org-idempotent" | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.ts (1)
587-616: Optional: strengthen multi-tenant fidelity of the other-org test.The mock's
statemap is keyed by${kind}:${v1Id}, so when the active org upsertsproject:p1, it overwrites the seededsome-other-orgrow. In production the migration_state table is org-scoped (compound key by org), so both rows should coexist after the migration. The current assertions correctly verify the active org migrates without being blocked, but they don't pin down the "doesn't disturb other org's state" invariant. If you want to lock that in, consider keying the mock by${kind}:${v1Id}:${organizationId}(and updatinglistState/upsertStateaccordingly) and asserting thesome-other-orgrow is still present after the run.♻️ Sketch of a stricter assertion (requires composite-key mock)
expect(summary.projectsCreated).toBe(1); expect(summary.errors).toHaveLength(0); -expect(env.state.get("project:p1")?.organizationId).toBe(ORG); -expect(env.state.get("project:p1")?.status).toBe("success"); +// Active-org row was created. +expect(env.state.get(`project:p1:${ORG}`)?.status).toBe("success"); +// Other-org row is left untouched. +expect(env.state.get("project:p1:some-other-org")?.v2Id).toBe( + "v2-other-org-project", +);🤖 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 587 - 616, The test seeds a mocked migration state map keyed only by "${kind}:${v1Id}" so an upsert from the active org overwrites the seeded "some-other-org" row; update the mock to use a composite key "${kind}:${v1Id}:${organizationId}" in makeFakeEnv (and adjust the mock implementations of listState and upsertState to read/write using that composite key) so rows for different organizations can coexist, then in the test (migrate.test.ts) add an assertion after migrateV1DataToV2 that the original "some-other-org" entry still exists and retains its organizationId/status while the active org's entry is created/updated; reference the migrateV1DataToV2 call and the mock helpers makeFakeEnv, listState, and upsertState when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.ts`:
- Around line 587-616: The test seeds a mocked migration state map keyed only by
"${kind}:${v1Id}" so an upsert from the active org overwrites the seeded
"some-other-org" row; update the mock to use a composite key
"${kind}:${v1Id}:${organizationId}" in makeFakeEnv (and adjust the mock
implementations of listState and upsertState to read/write using that composite
key) so rows for different organizations can coexist, then in the test
(migrate.test.ts) add an assertion after migrateV1DataToV2 that the original
"some-other-org" entry still exists and retains its organizationId/status while
the active org's entry is created/updated; reference the migrateV1DataToV2 call
and the mock helpers makeFakeEnv, listState, and upsertState when making these
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 962df704-2c0c-484c-9d27-34fc755aa5a4
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/migration/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/lib/trpc/routers/migration/index.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Make the V1 to V2 migration rerunnable when stale migration state exists for another organization.
Root Cause
The migration state table is scoped by
organizationId, which is useful because V2 project and workspace IDs are organization-scoped. However, the migration also had a global preflight guard that queried for any successful project migration row belonging to a different org and aborted the run. That prevented the idempotent project reconciliation path from running, includingfindByPathlinking to an existing V2 project.Changes
findMigrationByOtherOrgmigration router endpoint.Rerun Safety
The rerun path now relies on the existing per-row idempotency model:
setupProjectImportand reported as synced instead of recreated.collection.get(id)checks, so repeated runs do not duplicate sidebar entries.Validation
bun test apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.ts apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/writeSidebarState.test.ts apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/normalize.test.tsbunx @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.ts apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/writeSidebarState.ts apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/writeSidebarState.test.ts apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/normalize.ts apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/normalize.test.ts apps/desktop/src/lib/trpc/routers/migration/index.tsbun run --cwd apps/desktop typecheckgit diff --checkSummary by CodeRabbit
Bug Fixes
Tests