refactor(desktop): drop v1_migration_state from importer UX#4128
Merged
Conversation
…rom real state The pull-based v1→v2 importer (d00b174) used `v1_migration_state` as the single source of truth for "is this row already imported?" and "did the last attempt fail?". That created a class of false-failure bugs where a stale audit row from a prior run survived a fix to the underlying state and kept the row showing "Retry — Project not found" forever, even when the new pull-based logic would have handled it cleanly. Removes the audit log as a UX driver for projects, workspaces, and presets. Each surface now derives state from real app state, self-healing across deletions and cross-device changes: - Projects: imported = host-service `findByPath` returns a `local-path` candidate (already cloud-staleness-filtered server-side). - Workspaces: v1→v2 project mapping via host-service `project.list` matched on repoPath; "imported workspace" via cloud workspace list keyed on (projectId, branch). - Presets: v1 preset.id is reused as the v2 collection row id, so "imported" is a single `v2TerminalPresets.has(preset.id)` check. - Banner: "remaining" count = v1 projects whose mainRepoPath isn't in host-service `project.list`. Wire-shape change: `workspace.cloudList` now returns `{id, projectId, branch, hostId}` instead of `{id}`. Only the importer consumes it. Removed: `migration.listState`, `migration.upsertState`, and `project.cloudList` (no callers). Errors are no longer persisted; they live in component state for the modal session and `console.error` for the log file. Also folds in a small V1ImportModal page-transition simplification (`key={page}` + Tailwind `animate-in` instead of an explicit isTransitioning fade) and a `findByPath` integration test fix that was missed in d00b174 — wire shape grew but the assertions didn't loosen. The `v1_migration_state` table and migration 0041 stay in place, inert. Drop in a follow-up after the rollout settles.
Pair of bash scripts that seed and tear down the full set of fixtures needed to walk the importer modal by hand: dev neon branch v2 projects across two orgs (covering same-URL and fork ambiguity), v1 local.db projects + worktrees + workspaces (clean adopt, stale-path-fallback, orphan-branch-hidden, ghost), on-disk repos with the right remote configurations, and a host.db row that triggers the relocate-confirm flow. setup.sh refuses to run if .env points at the prod neon branch. Useful for QA on every importer change; previously lived untracked in a separate worktree.
Contributor
📝 WalkthroughWalkthroughThis PR refactors the v1→v2 import flow to move from audit-based state tracking to query-driven state derived from host service data and cloud resources. The migration router drops the v1MigrationState API surface, host service endpoints are reorganized, and all import UI components are rewired to fetch and compute import eligibility from live queries instead of mutation-based audit logs. ChangesV1→V2 Import Flow Refactor
Sequence DiagramsequenceDiagram
participant User
participant UI as UI Components
participant HostSvc as Host Service
participant LocalDB as v1 Local DB
participant Cloud as Cloud (v2)
User->>UI: Open V1 Import Modal
rect rgba(100, 150, 200, 0.5)
note over UI, Cloud: Import Projects Flow
UI->>HostSvc: queryHostProjects()
HostSvc->>HostSvc: Fetch local v2 projects
HostSvc-->>UI: [v2 Projects]
UI->>LocalDB: readV1Projects()
LocalDB-->>UI: [v1 Projects]
UI->>UI: Compute remaining = v1Projects - imported
UI->>HostSvc: findByPath(v1Project.repoPath)
HostSvc->>Cloud: Query v2 by path
Cloud-->>HostSvc: [v2 Candidates]
HostSvc-->>UI: [Candidates or Error]
alt Multiple candidates
User->>UI: Select target candidate
end
alt Project already exists elsewhere
UI->>User: Prompt relocation
User->>UI: Confirm relocation
end
User->>UI: Import project
UI->>Cloud: Create v2 project
Cloud-->>UI: [New v2 Project]
UI->>HostSvc: Link v2 Project
HostSvc-->>UI: Success
end
rect rgba(150, 100, 200, 0.5)
note over UI, Cloud: Import Workspaces Flow
UI->>HostSvc: queryCloudWorkspaces()
HostSvc->>Cloud: Fetch workspaces
Cloud-->>HostSvc: [Cloud Workspaces]
HostSvc-->>UI: [Cloud Workspaces]
UI->>HostSvc: queryHostProjects()
HostSvc-->>UI: [v2 Projects with IDs]
UI->>UI: Build v2ProjectIdByV1Id mapping
UI->>UI: Derive importable workspaces
User->>UI: Adopt workspace
UI->>Cloud: Create v2 workspace
Cloud-->>UI: [Adopted Workspace]
UI->>UI: Update adoptedV2Id state
end
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 |
Greptile SummaryRemoves the audit log table as a UX driver for the v1-to-v2 importer, replacing stale-row bugs with state derived entirely from live data sources. Each tab now checks real app state rather than a potentially-stale audit row.
Confidence Score: 4/5Safe to merge with awareness of two minor cache-consistency gaps in the Projects to Workspaces tab transition. The core refactor is sound — per-row state now derives from live app data instead of a stale audit table, directly fixing the false-failure bug described in the PR. The main rough edge is that ImportProjectsPage invalidates only the findByPath cache after a successful import, leaving the hostProjectListQuery cache used by the Workspaces tab stale until its background refetch fires on mount. On a fast connection this is invisible; on a slow one the user sees an empty workspace list momentarily. ImportProjectsPage.tsx and ImportWorkspacesPage.tsx — the cache invalidation gap and the adopt-retry idempotency change.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx | Removes audit-log state from project rows; derives imported status from findByPath local-path candidates and ephemeral linkedV2Id. The hostProjectListQuery cache used by the Workspaces tab is not invalidated after a successful import. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx | Replaces audit-log based tracking with host-project-list and cloud-workspace-keys approach. Contains a redundant IIFE for alreadyImported prop with dead code; removal of existingWorkspaceId narrows idempotency on adopt retries. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportPresetsPage/ImportPresetsPage.tsx | Switches to useLiveQuery against v2TerminalPresets and uses preset.id directly as the v2 row id for idempotency. Clean removal of audit log dependency. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsx | Replaces audit-query with a host-service project.list call keyed on mainRepoPath; correctly gated to avoid spurious requests. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/V1ImportModal.tsx | Simplifies page transitions by replacing manual isTransitioning state with key-based remount and Tailwind animate-in. Cleaner and correct. |
| packages/host-service/src/trpc/router/workspace/workspace.ts | Expands cloudList response shape to include projectId, branch, and hostId to support the new composite key lookup in the importer. |
| packages/host-service/src/trpc/router/project/project.ts | Removes project.cloudList procedure that is no longer called by any consumer. |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx:209-216
**Dead code branch inside `alreadyImported` IIFE**
The outer `for` loop at line 142 already has `if (!v2ProjectId) continue`, so every `workspace` pushed into `visibleWorkspaces` is guaranteed to have a non-null `v2ProjectId`. The IIFE re-fetches the same Map key and includes an unreachable `if (!v2ProjectId) return false` branch. The simpler path is to capture `alreadyImported` from the loop body (already computed there as a `const`) or pass the outer-scope `v2ProjectId` directly.
### Issue 2 of 3
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx:284-296
**`existingWorkspaceId` removal narrows idempotency window for adopt retries**
The old code forwarded the persisted audit `v2Id` as `existingWorkspaceId` so that a partial failure (workspace created in cloud, but the success callback threw) would retry idempotently against the same workspace id. Without it, a retry calls `workspaceCreation.adopt` without any prior-id hint. If the first call created a workspace and then `ensureWorkspaceInSidebar` threw, the user sees an error button and the `cloudWorkspacesQuery` cache is still stale (the invalidation never ran). Clicking retry submits a second adopt with no `existingWorkspaceId`. Whether this produces a duplicate depends on whether `adopt` has server-side branch-uniqueness enforcement — worth confirming, since the `cloudWorkspaceKeys` guard only protects after the next successful cloud-list refetch.
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx:199-204
**Workspaces tab misses newly-imported projects until background refetch completes**
After `runImport` succeeds, only the `FIND_BY_PATH_KEY_PREFIX` cache is invalidated. The `hostProjectListQuery` cache used by `ImportWorkspacesPage` is not touched. When the user navigates to the Workspaces tab, that component mounts with the pre-import cached host-project list, `v2ProjectIdByV1Id` is empty for the just-imported project, and its workspace rows stay invisible until React Query's staleTime-0 background refetch finishes. In the old flow the audit-log upsert happened synchronously before the page switch, keeping both tabs consistent. Adding a `queryClient.invalidateQueries` call for the workspaces host-project query key after a successful import would restore that guarantee.
Reviews (1): Last reviewed commit: "test(desktop): manual test fixtures for ..." | Re-trigger Greptile
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+209
to
+216
| v2ProjectId={v2ProjectIdByV1Id.get(workspace.projectId) ?? null} | ||
| alreadyImported={(() => { | ||
| const v2ProjectId = v2ProjectIdByV1Id.get(workspace.projectId); | ||
| if (!v2ProjectId) return false; | ||
| return cloudWorkspaceKeys.has( | ||
| `${v2ProjectId}\0${workspace.branch}`, | ||
| ); | ||
| })()} |
There was a problem hiding this comment.
alreadyImported IIFE
The outer for loop at line 142 already has if (!v2ProjectId) continue, so every workspace pushed into visibleWorkspaces is guaranteed to have a non-null v2ProjectId. The IIFE re-fetches the same Map key and includes an unreachable if (!v2ProjectId) return false branch. The simpler path is to capture alreadyImported from the loop body (already computed there as a const) or pass the outer-scope v2ProjectId directly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx
Line: 209-216
Comment:
**Dead code branch inside `alreadyImported` IIFE**
The outer `for` loop at line 142 already has `if (!v2ProjectId) continue`, so every `workspace` pushed into `visibleWorkspaces` is guaranteed to have a non-null `v2ProjectId`. The IIFE re-fetches the same Map key and includes an unreachable `if (!v2ProjectId) return false` branch. The simpler path is to capture `alreadyImported` from the loop body (already computed there as a `const`) or pass the outer-scope `v2ProjectId` directly.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
284
to
+296
| } | ||
| } | ||
|
|
||
| await upsertState.mutateAsync({ | ||
| v1Id: workspace.id, | ||
| kind: "workspace", | ||
| v2Id: result.workspace.id, | ||
| organizationId, | ||
| status: "success", | ||
| reason: null, | ||
| }); | ||
|
|
||
| ensureWorkspaceInSidebar(result.workspace.id, v2ProjectId); | ||
| await trpcUtils.migration.listState.invalidate({ organizationId }); | ||
| // Without this, the freshly-adopted workspace isn't in the | ||
| // cached cloud-list snapshot, so the audit-ghost detector flips | ||
| // the row from "Imported" back to a fresh Adopt button. | ||
| setAdoptedV2Id(result.workspace.id); | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: WORKSPACE_CLOUD_LIST_KEY, | ||
| }); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| setErrorMessage(message); | ||
| await upsertState | ||
| .mutateAsync({ | ||
| v1Id: workspace.id, | ||
| kind: "workspace", | ||
| v2Id: null, | ||
| organizationId, | ||
| status: "error", | ||
| reason: message, | ||
| }) | ||
| .catch((auditErr) => { | ||
| console.warn( | ||
| "[v1-import] failed to record workspace adopt error in audit", | ||
| { workspaceId: workspace.id, auditErr }, | ||
| ); | ||
| }); | ||
| await trpcUtils.migration.listState.invalidate({ organizationId }); | ||
| console.error("[v1-import] workspace adopt failed", { | ||
| v1WorkspaceId: workspace.id, |
There was a problem hiding this comment.
existingWorkspaceId removal narrows idempotency window for adopt retries
The old code forwarded the persisted audit v2Id as existingWorkspaceId so that a partial failure (workspace created in cloud, but the success callback threw) would retry idempotently against the same workspace id. Without it, a retry calls workspaceCreation.adopt without any prior-id hint. If the first call created a workspace and then ensureWorkspaceInSidebar threw, the user sees an error button and the cloudWorkspacesQuery cache is still stale (the invalidation never ran). Clicking retry submits a second adopt with no existingWorkspaceId. Whether this produces a duplicate depends on whether adopt has server-side branch-uniqueness enforcement — worth confirming, since the cloudWorkspaceKeys guard only protects after the next successful cloud-list refetch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx
Line: 284-296
Comment:
**`existingWorkspaceId` removal narrows idempotency window for adopt retries**
The old code forwarded the persisted audit `v2Id` as `existingWorkspaceId` so that a partial failure (workspace created in cloud, but the success callback threw) would retry idempotently against the same workspace id. Without it, a retry calls `workspaceCreation.adopt` without any prior-id hint. If the first call created a workspace and then `ensureWorkspaceInSidebar` threw, the user sees an error button and the `cloudWorkspacesQuery` cache is still stale (the invalidation never ran). Clicking retry submits a second adopt with no `existingWorkspaceId`. Whether this produces a duplicate depends on whether `adopt` has server-side branch-uniqueness enforcement — worth confirming, since the `cloudWorkspaceKeys` guard only protects after the next successful cloud-list refetch.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
199
to
204
| repoPath, | ||
| mainWorkspaceId, | ||
| }); | ||
|
|
||
| await trpcUtils.migration.listState.invalidate({ organizationId }); | ||
| setLinkedV2Id(v2ProjectId); | ||
| await queryClient.invalidateQueries({ |
There was a problem hiding this comment.
After runImport succeeds, only the FIND_BY_PATH_KEY_PREFIX cache is invalidated. The hostProjectListQuery cache used by ImportWorkspacesPage is not touched. When the user navigates to the Workspaces tab, that component mounts with the pre-import cached host-project list, v2ProjectIdByV1Id is empty for the just-imported project, and its workspace rows stay invisible until React Query's staleTime-0 background refetch finishes. In the old flow the audit-log upsert happened synchronously before the page switch, keeping both tabs consistent. Adding a queryClient.invalidateQueries call for the workspaces host-project query key after a successful import would restore that guarantee.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx
Line: 199-204
Comment:
**Workspaces tab misses newly-imported projects until background refetch completes**
After `runImport` succeeds, only the `FIND_BY_PATH_KEY_PREFIX` cache is invalidated. The `hostProjectListQuery` cache used by `ImportWorkspacesPage` is not touched. When the user navigates to the Workspaces tab, that component mounts with the pre-import cached host-project list, `v2ProjectIdByV1Id` is empty for the just-imported project, and its workspace rows stay invisible until React Query's staleTime-0 background refetch finishes. In the old flow the audit-log upsert happened synchronously before the page switch, keeping both tabs consistent. Adding a `queryClient.invalidateQueries` call for the workspaces host-project query key after a successful import would restore that guarantee.
How can I resolve this? If you propose a fix, please make it concise.![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx (1)
87-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t derive the expected GitHub URL from the local folder name.
getBaseName(project.mainRepoPath)breaks for relocated clones like~/code/onbook-relocate-clone, producinghttps://github.com/.../onbook-relocate-cloneinstead of the real remote. In that casefindByPathcan miss the existing cloud project and fall back to creating a duplicate instead of linking it.A safer short-term fix is to stop passing
expectedRemoteUrluntil this value comes from the actual git remote (or another canonical repo identifier).Also applies to: 115-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx` around lines 87 - 95, The helper expectedRemoteUrlFor currently derives a GitHub URL from the local folder name (getBaseName(project.mainRepoPath)), which misidentifies relocated clones; stop deriving/passing expectedRemoteUrl until you have the real remote. Update expectedRemoteUrlFor (and any call sites that pass its result, e.g., where you call findByPath or construct the project lookup between lines around 87 and 128) to return/propagate undefined (or simply omit the expectedRemoteUrl argument) so no guessed URL is supplied; later wire it to the canonical git remote value once available.apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx (1)
106-111:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
cloudWorkspacesQuery.isPendinginisLoading.
alreadyImported(and therefore the per-row action) is derived fromcloudWorkspaceKeys, which is empty untilcloudWorkspacesQueryresolves. If the cloud query is slower than the others, the shell stops showing the loading state while every row still computesalreadyImported = false, so already-imported workspaces briefly render an actionable "Adopt" button and a click during that window will round-trip to the host and surface a confusing error.🛠️ Proposed fix
const isLoading = projectsQuery.isPending || workspacesQuery.isPending || worktreesQuery.isPending || hostProjectListQuery.isPending || + cloudWorkspacesQuery.isPending || worktreeListQueries.some((q) => q.isPending);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx` around lines 106 - 111, The isLoading flag currently omits cloudWorkspacesQuery, causing the UI to stop showing loading while cloudWorkspaceKeys is still empty; update the isLoading computation to include cloudWorkspacesQuery.isPending so rows that compute alreadyImported (derived from cloudWorkspaceKeys) remain in the loading state until the cloud workspaces query resolves, preventing actionable "Adopt" buttons from rendering prematurely.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx (1)
140-216: ⚡ Quick winPrecompute per-row metadata once and avoid the inline IIFE.
v2ProjectIdandalreadyImportedare computed in the visibility filter (lines 142–147) and then recomputed per row in the JSX via an IIFE (lines 210–216). Carry them through alongside eachvisibleWorkspaceso the render path is straight-line and the two computations cannot drift. As a side effect,v2ProjectId ?? nullon line 209 disappears since the filter already guarantees a non-null v2 project id.♻️ Proposed refactor
- const visibleWorkspaces: typeof allWorkspaces = []; + const visibleWorkspaces: Array<{ + workspace: (typeof allWorkspaces)[number]; + v2ProjectId: string; + alreadyImported: boolean; + }> = []; for (const workspace of allWorkspaces) { const v2ProjectId = v2ProjectIdByV1Id.get(workspace.projectId); if (!v2ProjectId) continue; const alreadyImported = cloudWorkspaceKeys.has( `${v2ProjectId}\0${workspace.branch}`, ); if (!alreadyImported) { const validBranches = validBranchesByV2ProjectId.get(v2ProjectId); if (validBranches !== undefined && !validBranches.has(workspace.branch)) { continue; } } - visibleWorkspaces.push(workspace); + visibleWorkspaces.push({ workspace, v2ProjectId, alreadyImported }); }…and update the grouping and render to consume the enriched entries:
- for (const workspace of visibleWorkspaces) { - const project = projectsById.get(workspace.projectId); + for (const entry of visibleWorkspaces) { + const project = projectsById.get(entry.workspace.projectId); if (!project) continue; - const bucket = grouped.get(workspace.projectId) ?? { + const bucket = grouped.get(entry.workspace.projectId) ?? { projectName: project.name, items: [], }; - bucket.items.push(workspace); - grouped.set(workspace.projectId, bucket); + bucket.items.push(entry); + grouped.set(entry.workspace.projectId, bucket); }- {group.items.map((workspace) => ( + {group.items.map(({ workspace, v2ProjectId, alreadyImported }) => ( <WorkspaceRow key={workspace.id} workspace={workspace} /* … */ - v2ProjectId={v2ProjectIdByV1Id.get(workspace.projectId) ?? null} - alreadyImported={(() => { - const v2ProjectId = v2ProjectIdByV1Id.get(workspace.projectId); - if (!v2ProjectId) return false; - return cloudWorkspaceKeys.has( - `${v2ProjectId}\0${workspace.branch}`, - ); - })()} + v2ProjectId={v2ProjectId} + alreadyImported={alreadyImported} organizationId={organizationId} activeHostUrl={activeHostUrl} /> ))}(
WorkspaceRowProps.v2ProjectIdcan be tightened tostringonce the IIFE branch is gone.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx` around lines 140 - 216, The code recomputes v2ProjectId and alreadyImported inside the JSX IIFE for each WorkspaceRow; instead, when building visibleWorkspaces (the first for-loop that uses v2ProjectIdByV1Id, cloudWorkspaceKeys, and validBranchesByV2ProjectId) create enriched entries like { workspace, v2ProjectId, alreadyImported } and push those into visibleWorkspaces, then update the grouping logic (grouped map) to use entry.workspace and entry.v2ProjectId, and update the render to map over group.items as enriched entries and pass workspace={entry.workspace}, v2ProjectId={entry.v2ProjectId} and alreadyImported={entry.alreadyImported} to WorkspaceRow (removing the inline IIFE and the v2ProjectId ?? null fallback).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsx`:
- Around line 41-66: The code currently treats hostProjectListQuery.data ?? []
as an empty list which makes remaining include all v1 projects when
activeHostUrl is unset or the host query hasn't succeeded; change the logic so
you only treat hostProjectListQuery.data as authoritative when the query
succeeded (use hostProjectListQuery.isSuccess or status === "success")—compute
importedRepoPaths from hostProjectListQuery.data only in that case, and if the
host query is not successful/loaded (data === undefined) avoid computing
remaining or return null/do not show the banner; update references in this block
(hostProjectListQuery, importedRepoPaths, remaining, projectsQuery,
activeHostUrl) accordingly so the banner only appears when hostProjectListQuery
has valid data.
In `@packages/host-service/src/trpc/router/workspace/workspace.ts`:
- Around line 26-35: The cloudList handler dereferences ctx.api unguarded;
update cloudList (in workspace.ts) to check that ctx.api and ctx.api.v2Workspace
exist before calling list and, if missing, throw a TRPCError with code
"PRECONDITION_FAILED" (matching how delete does it) with a clear message; ensure
you import/use the same TRPCError or error helper used elsewhere in this router
so the route returns the proper PRECONDITION_FAILED response instead of a
runtime exception.
In `@scripts/v1v2-import-test-cleanup.sh`:
- Around line 45-47: The DELETE runs directly against whatever DATABASE_URL is
in .env (PGURL) and lacks the non-prod guard used in v1v2-import-test-setup.sh;
add a safety check before the psql call that validates PGURL points to a
non-production branch (e.g., assert the URL/host contains the expected dev/neon
indicator or that the branch name is not "main"/"prod") and exit with an error
if the check fails so the psql "$PGURL" -c "DELETE FROM public.v2_projects ..."
command cannot run against a production DB.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx`:
- Around line 87-95: The helper expectedRemoteUrlFor currently derives a GitHub
URL from the local folder name (getBaseName(project.mainRepoPath)), which
misidentifies relocated clones; stop deriving/passing expectedRemoteUrl until
you have the real remote. Update expectedRemoteUrlFor (and any call sites that
pass its result, e.g., where you call findByPath or construct the project lookup
between lines around 87 and 128) to return/propagate undefined (or simply omit
the expectedRemoteUrl argument) so no guessed URL is supplied; later wire it to
the canonical git remote value once available.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx`:
- Around line 106-111: The isLoading flag currently omits cloudWorkspacesQuery,
causing the UI to stop showing loading while cloudWorkspaceKeys is still empty;
update the isLoading computation to include cloudWorkspacesQuery.isPending so
rows that compute alreadyImported (derived from cloudWorkspaceKeys) remain in
the loading state until the cloud workspaces query resolves, preventing
actionable "Adopt" buttons from rendering prematurely.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx`:
- Around line 140-216: The code recomputes v2ProjectId and alreadyImported
inside the JSX IIFE for each WorkspaceRow; instead, when building
visibleWorkspaces (the first for-loop that uses v2ProjectIdByV1Id,
cloudWorkspaceKeys, and validBranchesByV2ProjectId) create enriched entries like
{ workspace, v2ProjectId, alreadyImported } and push those into
visibleWorkspaces, then update the grouping logic (grouped map) to use
entry.workspace and entry.v2ProjectId, and update the render to map over
group.items as enriched entries and pass workspace={entry.workspace},
v2ProjectId={entry.v2ProjectId} and alreadyImported={entry.alreadyImported} to
WorkspaceRow (removing the inline IIFE and the v2ProjectId ?? null fallback).
🪄 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: d27f8da2-a827-406a-b824-ca6309b96da0
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
apps/desktop/src/lib/trpc/routers/migration/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportPresetsPage/ImportPresetsPage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/V1ImportModal.tsxpackages/host-service/src/trpc/router/project/project.tspackages/host-service/src/trpc/router/workspace/workspace.tspackages/host-service/test/integration/project.integration.test.tsscripts/v1v2-import-test-cleanup.shscripts/v1v2-import-test-setup.sh
💤 Files with no reviewable changes (1)
- packages/host-service/src/trpc/router/project/project.ts
Comment on lines
+41
to
+66
| const enabled = isV2CloudEnabled && !!organizationId && !dismissed; | ||
|
|
||
| const projectsQuery = electronTrpc.migration.readV1Projects.useQuery( | ||
| undefined, | ||
| { enabled: isV2CloudEnabled && !!organizationId && !dismissed }, | ||
| ); | ||
| const auditQuery = electronTrpc.migration.listState.useQuery( | ||
| { organizationId: organizationId ?? "" }, | ||
| { enabled: isV2CloudEnabled && !!organizationId && !dismissed }, | ||
| { enabled }, | ||
| ); | ||
| const hostProjectListQuery = useQuery({ | ||
| queryKey: ["v1-import-banner", "hostProjectList", activeHostUrl], | ||
| queryFn: async () => { | ||
| if (!activeHostUrl) return []; | ||
| const client = getHostServiceClientByUrl(activeHostUrl); | ||
| return client.project.list.query(); | ||
| }, | ||
| enabled: enabled && !!activeHostUrl, | ||
| retry: false, | ||
| }); | ||
|
|
||
| if (!isV2CloudEnabled || !organizationId || dismissed) return null; | ||
|
|
||
| const projects = projectsQuery.data ?? []; | ||
| const importedV1Ids = new Set( | ||
| (auditQuery.data ?? []) | ||
| .filter( | ||
| (row) => | ||
| row.kind === "project" && | ||
| (row.status === "success" || row.status === "linked"), | ||
| ) | ||
| .map((row) => row.v1Id), | ||
| const importedRepoPaths = new Set( | ||
| (hostProjectListQuery.data ?? []).map((p) => p.repoPath), | ||
| ); | ||
| const remaining = projects.filter((p) => !importedV1Ids.has(p.id)).length; | ||
| const remaining = projects.filter( | ||
| (p) => !importedRepoPaths.has(p.mainRepoPath), | ||
| ).length; |
Contributor
There was a problem hiding this comment.
Don’t treat a missing host-project list as “nothing is imported.”
hostProjectListQuery.data ?? [] makes remaining fall back to all v1 projects whenever activeHostUrl is still unset or project.list errors. Since this banner now uses host state as the source of truth, that shows a false import prompt for already-imported users.
Suggested fix
- const enabled = isV2CloudEnabled && !!organizationId && !dismissed;
+ const enabled =
+ isV2CloudEnabled && !!organizationId && !!activeHostUrl && !dismissed;
@@
- if (!isV2CloudEnabled || !organizationId || dismissed) return null;
+ if (!enabled) return null;
+ if (projectsQuery.isPending || hostProjectListQuery.isPending) return null;
+ if (projectsQuery.isError || hostProjectListQuery.isError) return null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsx`
around lines 41 - 66, The code currently treats hostProjectListQuery.data ?? []
as an empty list which makes remaining include all v1 projects when
activeHostUrl is unset or the host query hasn't succeeded; change the logic so
you only treat hostProjectListQuery.data as authoritative when the query
succeeded (use hostProjectListQuery.isSuccess or status === "success")—compute
importedRepoPaths from hostProjectListQuery.data only in that case, and if the
host query is not successful/loaded (data === undefined) avoid computing
remaining or return null/do not show the banner; update references in this block
(hostProjectListQuery, importedRepoPaths, remaining, projectsQuery,
activeHostUrl) accordingly so the banner only appears when hostProjectListQuery
has valid data.
Comment on lines
26
to
+35
| cloudList: protectedProcedure.query(async ({ ctx }) => { | ||
| const rows = await ctx.api.v2Workspace.list.query({ | ||
| organizationId: ctx.organizationId, | ||
| }); | ||
| return rows.map((row) => ({ id: row.id })); | ||
| return rows.map((row) => ({ | ||
| id: row.id, | ||
| projectId: row.projectId, | ||
| branch: row.branch, | ||
| hostId: row.hostId, | ||
| })); |
Contributor
There was a problem hiding this comment.
Guard cloudList when the cloud API is unavailable.
This route dereferences ctx.api unconditionally, so a host without cloud configured will throw a generic runtime error here instead of returning the PRECONDITION_FAILED response that delete already uses in this router.
Suggested fix
cloudList: protectedProcedure.query(async ({ ctx }) => {
+ if (!ctx.api) {
+ throw new TRPCError({
+ code: "PRECONDITION_FAILED",
+ message: "Cloud API not configured",
+ });
+ }
+
const rows = await ctx.api.v2Workspace.list.query({
organizationId: ctx.organizationId,
});
return rows.map((row) => ({
id: row.id,📝 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
| cloudList: protectedProcedure.query(async ({ ctx }) => { | |
| const rows = await ctx.api.v2Workspace.list.query({ | |
| organizationId: ctx.organizationId, | |
| }); | |
| return rows.map((row) => ({ id: row.id })); | |
| return rows.map((row) => ({ | |
| id: row.id, | |
| projectId: row.projectId, | |
| branch: row.branch, | |
| hostId: row.hostId, | |
| })); | |
| cloudList: protectedProcedure.query(async ({ ctx }) => { | |
| if (!ctx.api) { | |
| throw new TRPCError({ | |
| code: "PRECONDITION_FAILED", | |
| message: "Cloud API not configured", | |
| }); | |
| } | |
| const rows = await ctx.api.v2Workspace.list.query({ | |
| organizationId: ctx.organizationId, | |
| }); | |
| return rows.map((row) => ({ | |
| id: row.id, | |
| projectId: row.projectId, | |
| branch: row.branch, | |
| hostId: row.hostId, | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/trpc/router/workspace/workspace.ts` around lines 26
- 35, The cloudList handler dereferences ctx.api unguarded; update cloudList (in
workspace.ts) to check that ctx.api and ctx.api.v2Workspace exist before calling
list and, if missing, throw a TRPCError with code "PRECONDITION_FAILED"
(matching how delete does it) with a clear message; ensure you import/use the
same TRPCError or error helper used elsewhere in this router so the route
returns the proper PRECONDITION_FAILED response instead of a runtime exception.
Comment on lines
+45
to
+47
| echo "→ deleting fixture v2 projects from dev cloud" | ||
| PGURL=$(grep '^DATABASE_URL=' .env | sed 's/^DATABASE_URL=//;s/^"//;s/"$//') | ||
| psql "$PGURL" -c "DELETE FROM public.v2_projects WHERE id::text LIKE '11111111-aaaa-4aaa-8aaa-%' OR id::text LIKE '33333333-aaaa-4aaa-8aaa-%'" >/dev/null |
Contributor
There was a problem hiding this comment.
Add the same non-prod guard before deleting cloud fixtures.
This script runs a destructive DELETE against whatever DATABASE_URL is in .env, but it skips the branch safety check that v1v2-import-test-setup.sh already has. A misconfigured env will delete from the wrong Neon branch.
Suggested fix
echo "→ deleting fixture v2 projects from dev cloud"
PGURL=$(grep '^DATABASE_URL=' .env | sed 's/^DATABASE_URL=//;s/^"//;s/"$//')
+BRANCH_NAME=$(grep '^NEON_BRANCH_ID=' .env | sed 's/^NEON_BRANCH_ID=//;s/^"//;s/"$//')
+if [ -z "$BRANCH_NAME" ] || [ "$BRANCH_NAME" = "br-billowing-dream-af839yib" ]; then
+ echo "✗ refusing to clean up — .env points at the prod neon branch."
+ echo " Switch to a dev branch first."
+ exit 1
+fi
psql "$PGURL" -c "DELETE FROM public.v2_projects WHERE id::text LIKE '11111111-aaaa-4aaa-8aaa-%' OR id::text LIKE '33333333-aaaa-4aaa-8aaa-%'" >/dev/null📝 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
| echo "→ deleting fixture v2 projects from dev cloud" | |
| PGURL=$(grep '^DATABASE_URL=' .env | sed 's/^DATABASE_URL=//;s/^"//;s/"$//') | |
| psql "$PGURL" -c "DELETE FROM public.v2_projects WHERE id::text LIKE '11111111-aaaa-4aaa-8aaa-%' OR id::text LIKE '33333333-aaaa-4aaa-8aaa-%'" >/dev/null | |
| echo "→ deleting fixture v2 projects from dev cloud" | |
| PGURL=$(grep '^DATABASE_URL=' .env | sed 's/^DATABASE_URL=//;s/^"//;s/"$//') | |
| BRANCH_NAME=$(grep '^NEON_BRANCH_ID=' .env | sed 's/^NEON_BRANCH_ID=//;s/^"//;s/"$//') | |
| if [ -z "$BRANCH_NAME" ] || [ "$BRANCH_NAME" = "br-billowing-dream-af839yib" ]; then | |
| echo "✗ refusing to clean up — .env points at the prod neon branch." | |
| echo " Switch to a dev branch first." | |
| exit 1 | |
| fi | |
| psql "$PGURL" -c "DELETE FROM public.v2_projects WHERE id::text LIKE '11111111-aaaa-4aaa-8aaa-%' OR id::text LIKE '33333333-aaaa-4aaa-8aaa-%'" >/dev/null |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/v1v2-import-test-cleanup.sh` around lines 45 - 47, The DELETE runs
directly against whatever DATABASE_URL is in .env (PGURL) and lacks the non-prod
guard used in v1v2-import-test-setup.sh; add a safety check before the psql call
that validates PGURL points to a non-production branch (e.g., assert the
URL/host contains the expected dev/neon indicator or that the branch name is not
"main"/"prod") and exit with an error if the check fails so the psql "$PGURL" -c
"DELETE FROM public.v2_projects ..." command cannot run against a production DB.
- ImportWorkspacesPage: drop the dead-code IIFE for `alreadyImported` — the visibility loop already filters out workspaces without a v2 project, so the recompute and null guard at the WorkspaceRow call site were unreachable. Carry `(v2ProjectId, alreadyImported)` through the visible-workspace tuple instead. Drops a now-unreachable "blocked" branch in the action computation too. - ImportWorkspacesPage: pass `adoptedV2Id` back as `existingWorkspaceId` on adopt retry. If a previous adopt succeeded in cloud but the local follow-up threw, the retry now hints the workspace id so adopt's idempotency hits instead of provisioning a duplicate workspace. - ImportProjectsPage: invalidate the workspaces tab's host-project-list cache after a successful import. Without it, switching to Workspaces immediately after Import showed an empty list until the staleTime-0 background refetch finished.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx (1)
36-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat host/cloud lookup failures as an empty import state.
Both
v2ProjectIdByV1IdandcloudWorkspaceKeysfall back to empty collections, but neitherhostProjectListQuerynorcloudWorkspacesQueryis surfaced as an error state here. A failedproject.listturns into the misleading "Import a project..." empty state, and a failedworkspace.cloudListcan exposeAdoptfor rows that are already imported. This page should block or show an error when either source-of-truth query fails instead of rendering from empty defaults.Also applies to: 67-73, 106-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx` around lines 36 - 52, The page currently treats failed hostProjectListQuery or cloudWorkspacesQuery as empty lists (which lets v2ProjectIdByV1Id and cloudWorkspaceKeys fall back to empty arrays) — change the render logic to detect hostProjectListQuery.isError or cloudWorkspacesQuery.isError and short-circuit to an explicit error/blocking state (e.g., render an error message or loading/error UI) instead of using empty defaults; ensure any downstream logic that builds v2ProjectIdByV1Id or cloudWorkspaceKeys first checks the corresponding query succeeded (hostProjectListQuery.data / cloudWorkspacesQuery.data) and only computes fallbacks when the query is successful or still loading.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx`:
- Around line 24-29: The code currently hides read errors by defaulting
projectsQuery.data to [] and shows an empty-state; instead, detect and surface
query errors from electronTrpc.migration.readV1Projects.useQuery (inspect
projectsQuery.error and projectsQuery.isError) and render an error UI with the
error message and a retry action that calls projectsQuery.refetch; remove the
fall-back assignment const projects = projectsQuery.data ?? [] (or only use it
when !projectsQuery.isError) so failures don't present as "No v1 projects
found", and apply the same pattern to other similar queries referenced in the
file (lines ~43-52) so all read failures show an error + retry rather than an
empty state.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx`:
- Around line 54-63: Normalize repo paths before comparing them when building
v2ProjectIdByV1Id: canonicalize v2.repoPath into a normalized key when
populating v2ByPath and canonicalize v1.mainRepoPath before lookup, so
comparisons use the same casing/separator rules; update the logic around
v2ByPath.set(v2.repoPath, v2.id) and the lookup using
v2ByPath.get(v1.mainRepoPath) (referencing v2ProjectIdByV1Id,
hostProjectListQuery, projectsQuery, v2ByPath, v1.mainRepoPath, v2.repoPath) to
apply a shared normalizePath(...) helper or utility used elsewhere in the
codebase.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx`:
- Around line 36-52: The page currently treats failed hostProjectListQuery or
cloudWorkspacesQuery as empty lists (which lets v2ProjectIdByV1Id and
cloudWorkspaceKeys fall back to empty arrays) — change the render logic to
detect hostProjectListQuery.isError or cloudWorkspacesQuery.isError and
short-circuit to an explicit error/blocking state (e.g., render an error message
or loading/error UI) instead of using empty defaults; ensure any downstream
logic that builds v2ProjectIdByV1Id or cloudWorkspaceKeys first checks the
corresponding query succeeded (hostProjectListQuery.data /
cloudWorkspacesQuery.data) and only computes fallbacks when the query is
successful or still loading.
🪄 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: f16aaf56-7fa1-48b3-8ffa-2f2037a279f7
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx
Comment on lines
24
to
29
| const projectsQuery = electronTrpc.migration.readV1Projects.useQuery(); | ||
| const auditQuery = electronTrpc.migration.listState.useQuery({ | ||
| organizationId, | ||
| }); | ||
| const cloudProjectsQuery = useQuery({ | ||
| queryKey: [...PROJECT_CLOUD_LIST_KEY, organizationId, activeHostUrl], | ||
| queryFn: async () => { | ||
| const client = getHostServiceClientByUrl(activeHostUrl); | ||
| return client.project.cloudList.query(); | ||
| }, | ||
| retry: false, | ||
| }); | ||
| const [isRefreshing, setIsRefreshing] = useState(false); | ||
|
|
||
| const liveProjectIds = useMemo(() => { | ||
| // Returns null until the cloud query resolves so per-row code can | ||
| // distinguish "we don't know yet" from "it's gone". | ||
| if (!cloudProjectsQuery.data) return null; | ||
| return new Set(cloudProjectsQuery.data.map((p) => p.id)); | ||
| }, [cloudProjectsQuery.data]); | ||
|
|
||
| // Note: don't gate on `!projectsQuery.data`. If readV1Projects errors, | ||
| // `isPending` flips to false but `data` stays undefined, which would | ||
| // trap us in a permanent loading spinner. Falling through to itemCount=0 | ||
| // shows the empty-state message instead of a dead-end loader. | ||
| const isLoading = projectsQuery.isPending || auditQuery.isPending; | ||
|
|
||
| const auditByV1Id = new Map<string, AuditLogEntry>(); | ||
| for (const row of auditQuery.data ?? []) { | ||
| if (row.kind !== "project") continue; | ||
| auditByV1Id.set(row.v1Id, { | ||
| v2Id: row.v2Id, | ||
| status: row.status, | ||
| reason: row.reason, | ||
| }); | ||
| } | ||
| const isLoading = projectsQuery.isPending; | ||
|
|
||
| const projects = projectsQuery.data ?? []; |
Contributor
There was a problem hiding this comment.
Surface readV1Projects failures instead of the empty state.
If migration.readV1Projects errors, projects falls back to [] and the page renders "No v1 projects found on this device." That converts a read failure into a false empty state and leaves the user without a retry path at the page level.
Also applies to: 43-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx`
around lines 24 - 29, The code currently hides read errors by defaulting
projectsQuery.data to [] and shows an empty-state; instead, detect and surface
query errors from electronTrpc.migration.readV1Projects.useQuery (inspect
projectsQuery.error and projectsQuery.isError) and render an error UI with the
error message and a retry action that calls projectsQuery.refetch; remove the
fall-back assignment const projects = projectsQuery.data ?? [] (or only use it
when !projectsQuery.isError) so failures don't present as "No v1 projects
found", and apply the same pattern to other similar queries referenced in the
file (lines ~43-52) so all read failures show an error + retry rather than an
empty state.
Comment on lines
+54
to
+63
| const v2ProjectIdByV1Id = useMemo(() => { | ||
| const v2ByPath = new Map<string, string>(); | ||
| for (const v2 of hostProjectListQuery.data ?? []) { | ||
| v2ByPath.set(v2.repoPath, v2.id); | ||
| } | ||
| const map = new Map<string, string>(); | ||
| for (const v1 of projectsQuery.data ?? []) { | ||
| const v2Id = v2ByPath.get(v1.mainRepoPath); | ||
| if (v2Id) map.set(v1.id, v2Id); | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize repo paths before building the v1→v2 map.
Line 61 compares v1.mainRepoPath and v2.repoPath as raw strings. If host-service canonicalizes casing or separators differently, the same repo won't map here and all of that project's workspaces disappear from the importer. This looks like it can reintroduce the case-sensitivity/path-normalization bug on the workspace step.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx`
around lines 54 - 63, Normalize repo paths before comparing them when building
v2ProjectIdByV1Id: canonicalize v2.repoPath into a normalized key when
populating v2ByPath and canonicalize v1.mainRepoPath before lookup, so
comparisons use the same casing/separator rules; update the logic around
v2ByPath.set(v2.repoPath, v2.id) and the lookup using
v2ByPath.get(v1.mainRepoPath) (referencing v2ProjectIdByV1Id,
hostProjectListQuery, projectsQuery, v2ByPath, v1.mainRepoPath, v2.repoPath) to
apply a shared normalizePath(...) helper or utility used elsewhere in the
codebase.
6 tasks
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 8, 2026
…-sh#4128) * refactor(desktop): drop v1_migration_state from importer UX, derive from real state The pull-based v1→v2 importer (d00b174) used `v1_migration_state` as the single source of truth for "is this row already imported?" and "did the last attempt fail?". That created a class of false-failure bugs where a stale audit row from a prior run survived a fix to the underlying state and kept the row showing "Retry — Project not found" forever, even when the new pull-based logic would have handled it cleanly. Removes the audit log as a UX driver for projects, workspaces, and presets. Each surface now derives state from real app state, self-healing across deletions and cross-device changes: - Projects: imported = host-service `findByPath` returns a `local-path` candidate (already cloud-staleness-filtered server-side). - Workspaces: v1→v2 project mapping via host-service `project.list` matched on repoPath; "imported workspace" via cloud workspace list keyed on (projectId, branch). - Presets: v1 preset.id is reused as the v2 collection row id, so "imported" is a single `v2TerminalPresets.has(preset.id)` check. - Banner: "remaining" count = v1 projects whose mainRepoPath isn't in host-service `project.list`. Wire-shape change: `workspace.cloudList` now returns `{id, projectId, branch, hostId}` instead of `{id}`. Only the importer consumes it. Removed: `migration.listState`, `migration.upsertState`, and `project.cloudList` (no callers). Errors are no longer persisted; they live in component state for the modal session and `console.error` for the log file. Also folds in a small V1ImportModal page-transition simplification (`key={page}` + Tailwind `animate-in` instead of an explicit isTransitioning fade) and a `findByPath` integration test fix that was missed in d00b174 — wire shape grew but the assertions didn't loosen. The `v1_migration_state` table and migration 0041 stay in place, inert. Drop in a follow-up after the rollout settles. * test(desktop): manual test fixtures for v1→v2 importer Pair of bash scripts that seed and tear down the full set of fixtures needed to walk the importer modal by hand: dev neon branch v2 projects across two orgs (covering same-URL and fork ambiguity), v1 local.db projects + worktrees + workspaces (clean adopt, stale-path-fallback, orphan-branch-hidden, ghost), on-disk repos with the right remote configurations, and a host.db row that triggers the relocate-confirm flow. setup.sh refuses to run if .env points at the prod neon branch. Useful for QA on every importer change; previously lived untracked in a separate worktree. * fix(desktop): address v1→v2 importer review nits - ImportWorkspacesPage: drop the dead-code IIFE for `alreadyImported` — the visibility loop already filters out workspaces without a v2 project, so the recompute and null guard at the WorkspaceRow call site were unreachable. Carry `(v2ProjectId, alreadyImported)` through the visible-workspace tuple instead. Drops a now-unreachable "blocked" branch in the action computation too. - ImportWorkspacesPage: pass `adoptedV2Id` back as `existingWorkspaceId` on adopt retry. If a previous adopt succeeded in cloud but the local follow-up threw, the retry now hints the workspace id so adopt's idempotency hits instead of provisioning a duplicate workspace. - ImportProjectsPage: invalidate the workspaces tab's host-project-list cache after a successful import. Without it, switching to Workspaces immediately after Import showed an empty list until the staleTime-0 background refetch finished.
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
The pull-based v1→v2 importer (#4122) used
v1_migration_stateas the single source of truth for both "already imported" and "last attempt failed". That created a class of false-failure bugs where a stale audit row from a prior run kept a row showing "Retry — Project not found" forever, even when the live state had changed and the new pull-based logic would handle it cleanly. Original report: a teammate seeing "Retry — Project not found" for a project that exists on disk and in v1 (the path was correct, the v2 cloud row had been deleted under an older auto-batch migration).Removes the audit log as a UX driver. Every surface now derives from real app state:
findByPath→local-pathcandidate (cloud-staleness-filtered server-side)project.list(repoPath match) + cloud workspace list keyed on (projectId, branch)v2TerminalPresets.has(preset.id)mainRepoPathisn't in host-serviceproject.listErrors live in component state for the modal session +
console.errorfor the log file (no longer persisted to a table that drives UI).Wire shape change
workspace.cloudListnow returns{id, projectId, branch, hostId}instead of{id}. Verified the only caller is the importer.Removed
migration.listState,migration.upsertStatetrpc proceduresproject.cloudListhost-service procedure (no remaining callers)Kept inert
v1_migration_statetable + migration0041_v1_migration_state.sqlstay in place. Nothing reads or writes them. Drop in a follow-up after the rollout settles to keep the rollout reversible.Folded in
V1ImportModal.tsxpage-transition simplification (key={page}+ Tailwindanimate-inover manualisTransitioningfade)findByPathintegration test assertions loosened totoMatchObject— wire shape grew in refactor(desktop): rewrite v1→v2 migration as pull-based importer #4122 but assertions weren't updated; ports the fix from the migration-rewrite-manual branchManual test fixtures
Second commit lands two scripts in
scripts/:v1v2-import-test-setup.sh— seeds dev neon branch, v1 local.db, host.db, and on-disk repos to exercise every importer scenario (clean / fork ambiguity / relocate-confirm / stale-path fallback / orphan-branch hidden / local-only / unreachable remote)v1v2-import-test-cleanup.sh— reverses everything; idempotentsetup.sh refuses to run if
.envpoints at the prod neon branch.Out of scope (follow-ups)
host.db projects.repo_pathlookups (macOS APFS): same path in different case is treated as two projects today.host.db projects.repo_path: nothing prevents two rows pointing at the same path.Both pre-existing.
Test plan
bun run typecheckpassesbun run lintpassespackages/host-service/test/integration/project.integration.test.ts— 7/7 pass (was 5/7 on main)scripts/v1v2-import-test-setup.sh:Summary by cubic
Drops the
v1_migration_stateaudit log from the v1→v2 importer UI and derives import status from live app state. Fixes sticky “Retry — Project not found” errors and makes the flow resilient to cross‑device/cloud changes.Bug Fixes
existingWorkspaceId; removed unreachable “blocked” branch.project.listcache after import for immediate refresh.Refactors
project.findByPath; workspaces viaproject.list+ cloud list keyed by(projectId, branch); presets by reusing v1preset.idinv2TerminalPresets.migration.listState,migration.upsertState, and host-serviceproject.cloudList;workspace.cloudListnow returns{id, projectId, branch, hostId}; keptv1_migration_statetable inert.findByPathtest assertions totoMatchObject; addedscripts/v1v2-import-test-setup.shandscripts/v1v2-import-test-cleanup.sh.Written for commit c1727da. Summary will update on new commits.
Summary by CodeRabbit