Repository navigation
fix(host-service,desktop): broken import flow, stop v1 importer from duplicating already-imported projects - #6309
Merged
Conversation
… for appearance, lifted wizard state
…, shared relocate state, safe E2E procedure
… v1 project importer
Contributor
📝 WalkthroughWalkthroughThe change prevents duplicate V1 project imports by prioritizing local repository matches, reusing existing projects, preserving reused project settings, and coordinating discovery, retries, relocation, and bulk-import status in the renderer. ChangesV1 import duplicate prevention
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ImportProjectsPage
participant project.findByPath
participant createFromImportLocal
participant ProjectDatabase
User->>ImportProjectsPage: Start project import
ImportProjectsPage->>project.findByPath: Find repository path
project.findByPath->>ProjectDatabase: Check local project
ProjectDatabase-->>project.findByPath: Existing project or no match
project.findByPath-->>ImportProjectsPage: Import decision
ImportProjectsPage->>createFromImportLocal: Import pending project
createFromImportLocal->>ProjectDatabase: Reuse or create project
ProjectDatabase-->>createFromImportLocal: Project and created flag
createFromImportLocal-->>ImportProjectsPage: Import result
ImportProjectsPage-->>User: Updated project status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/import-plan.ts (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the
statesparameter type.
ReadonlyMap<string, { kind: string }>discards theProjectImportStatusdiscriminant. A misspelled kind string in a future caller would compile and silently change the pending set. The call sites inImportProjectsPage.tsxalready passMap<string, ProjectImportStatus>, and the test literals useas constkinds, so the stricter type stays compatible.♻️ Proposed type change
export function selectPendingProjects<T extends { id: string }>( projects: readonly T[], decisions: ReadonlyMap<string, ProjectImportDecision | undefined>, - states: ReadonlyMap<string, { kind: string }>, + states: ReadonlyMap<string, ProjectImportStatus>, ): T[] {🤖 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/import-plan.ts` around lines 25 - 29, Update the states parameter of selectPendingProjects to use ProjectImportStatus instead of the broad { kind: string } shape, preserving the existing map and pending-selection behavior while enforcing the ProjectImportStatus discriminant for callers.apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx (1)
67-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid writing a ref during render; sync it in an effect.
Line 71 mutates
importStatesRef.currentin the render body. React can render a component without committing it, so the ref can hold state from an abandoned render. Sync the ref in an effect instead.Also note that
decisionsnever memoizes:findByPathQueriesis a new array on every render, andprojectsis a new[]whenprojectsQuery.datais undefined. The map build is cheap, so this is a clarity issue rather than a performance issue.♻️ Proposed ref sync
- const importStatesRef = useRef(importStates); - importStatesRef.current = importStates; + const importStatesRef = useRef(importStates); + useEffect(() => { + importStatesRef.current = importStates; + }, [importStates]);Add
useEffectto the React import.🤖 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 67 - 103, Move the importStatesRef.current assignment out of the render body and synchronize it in a useEffect that depends on importStates, adding useEffect to the React imports. Keep updateImportStatus and the ref’s existing usage unchanged; do not alter the decisions calculation.apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/import-plan.test.ts (1)
134-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the cloud-unreachable branch.
planProjectRowActionbuilds a distinct user-facing message whencandidatesis empty andcloudErrorsis non-empty (import-plan.tslines 95-104). No test exercises that branch, and no test exercisesfindByPathPending: true.💚 Proposed additional cases
it("query errors are retryable via refetch", () => { expect( planProjectRowAction({ ...base, findByPathErrorMessage: "host unreachable", }), ).toEqual({ kind: "error", message: "host unreachable", retry: "query" }); }); + + it("pending discovery renders the running state", () => { + expect( + planProjectRowAction({ ...base, findByPathPending: true }), + ).toEqual({ kind: "running" }); + }); + + it("cloud errors with no candidates surface a retryable query error", () => { + const cloudDown = { + candidates: [], + cloudErrors: [{ url: "https://cloud.example", message: "timeout" }], + } as unknown as Pick< + ProjectFindByPathResult, + "candidates" | "cloudErrors" + >; + expect( + planProjectRowAction({ ...base, findByPathData: cloudDown }), + ).toEqual({ + kind: "error", + message: "Couldn't reach cloud for https://cloud.example: timeout", + retry: "query", + }); + });🤖 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/import-plan.test.ts` around lines 134 - 141, Add tests for both uncovered planProjectRowAction states: when candidates is empty and cloudErrors is non-empty, assert the distinct cloud-unreachable user-facing error message; and when findByPathPending is true, assert the expected pending action. Extend the existing test coverage without changing the established query-error retry assertion.
🤖 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 `@plans/done/2026-08-10-v1-import-duplicate-projects-design.md`:
- Around line 3-5: Update
plans/done/2026-08-10-v1-import-duplicate-projects-design.md lines 3-5 to mark
the plan as shipped. In
plans/done/2026-08-10-v1-import-duplicate-projects-fix-plan.md line 7, change
the design-document reference to its plans/done/ path, and at line 1278 update
the staging command to include both files from plans/done/.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/import-plan.test.ts`:
- Around line 134-141: Add tests for both uncovered planProjectRowAction states:
when candidates is empty and cloudErrors is non-empty, assert the distinct
cloud-unreachable user-facing error message; and when findByPathPending is true,
assert the expected pending action. Extend the existing test coverage without
changing the established query-error retry assertion.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/import-plan.ts`:
- Around line 25-29: Update the states parameter of selectPendingProjects to use
ProjectImportStatus instead of the broad { kind: string } shape, preserving the
existing map and pending-selection behavior while enforcing the
ProjectImportStatus discriminant for callers.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx`:
- Around line 67-103: Move the importStatesRef.current assignment out of the
render body and synchronize it in a useEffect that depends on importStates,
adding useEffect to the React imports. Keep updateImportStatus and the ref’s
existing usage unchanged; do not alter the decisions calculation.
🪄 Autofix
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 Plus
Run ID: 77bd7a2b-afd1-418a-81f2-dba6aa5bc7b2
📒 Files selected for processing (12)
apps/desktop/src/renderer/lib/v1-migration/index.tsapps/desktop/src/renderer/lib/v1-migration/projects.tsapps/desktop/src/renderer/lib/v1-migration/v1-migration.test.tsapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/import-plan.test.tsapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/import-plan.tsapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportRow/ImportRow.tsxpackages/host-service/src/trpc/router/project/handlers.tspackages/host-service/src/trpc/router/project/project-import.test.tspackages/host-service/src/trpc/router/project/project.tsplans/done/2026-08-10-v1-import-duplicate-projects-design.mdplans/done/2026-08-10-v1-import-duplicate-projects-fix-plan.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Architecture diagram
sequenceDiagram
participant R as "Renderer (ImportProjectsPage)"
participant S as "Page-level importStates map"
participant H as "HostService (tRPC project router)"
participant D as "Local DB (projects table)"
participant C as "Cloud API"
Note over R,C: Local-is-reality: a local project row is authoritative
R->>H: findByPath(repoPath, walkAllRemotes:true)
alt Local DB row exists
H->>D: query projects by resolved git root
D-->>H: row present
H-->>R: single local-path candidate (no cloud call)
else No local row
H->>D: query projects by resolved git root
D-->>H: not found
H->>C: walk GitHub remotes (findByGitHubRemote)
C-->>H: remote candidates (or cloudErrors)
H-->>R: candidate list + errors
end
Note over R,S: NEW: Import All driven by shared status map
R->>S: selectPendingProjects (skip already-imported/running)
S-->>R: queue with pending count
R->>S: set running for first project
loop each pending project
R->>H: createFromImportLocal(name, repoPath)
H->>D: check existing project by repoPath
alt Existing project (idempotent reuse)
H->>D: ensureMainWorkspaceStrict(existing.id)
H-->>R: {projectId, created:false}
R->>R: skip v1 appearance carry (created != false)
else New project
H->>D: insert project + main workspace
H-->>R: {projectId, created:true}
R->>H: carryV1ProjectAppearance (setColor/icon)
end
R->>S: mark imported, invalidate findByPath query
end
Note over R,S: Failure paths preserved with retry semantics
alt Lookup (findByPath) failure
R->>S: set idle (back to refetch)
R-->>R: show inline error with Retry=query
else Import failure (createFromImportLocal throws)
R->>S: set error status (stays pending)
R-->>R: show Retry=import, during batch shows Queued
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…status types, extra plan coverage, shipped-plan metadata
Contributor
Author
|
hey @Kitenite I dug in and found the cause in findByPath. Projects imported from v1 only exist in the host's local db, the cloud never hears about them. The stale link check asks the cloud about the local row, gets NOT_FOUND, and drops it. So the importer always thinks nothing was imported and creates a new project on every pass. The fix treats the local row as the source of truth, which is the same rule the delete path already follows. It also makes importLocal create reuse an existing project for the same repo path, and gives the projects page the same Import all behavior the workspaces page already has, since that one worked well. I tested on a local dev stack before and after the change, including a run with a fresh profile. Test notes are in the description. If the stale link check was covering a case I missed, tell me and I will rework it. |
Member
|
thnka you so much @distroinfinity im testing this now |
distroinfinity
had a problem deploying
to
preview
August 10, 2026 13:59 — with 
GitHub Actions
Failure
GitHub Actions
Failure
This branch had an error being deployed
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
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.
Problem
Using the Settings → Experimental → "Import from v1" wizard, the "Bring over your projects" step had three compounding issues:
Root cause
Importing a v1 project creates the v2 project locally only (
project.create {kind:"importLocal"}— "fully local, the cloud is never involved", by design). But the importer decides row state viaproject.findByPathwithwalkAllRemotes: true, and that branch had a "stale local link" probe: it found the local row, asked cloudv2Project.get(id), gotNOT_FOUND(the cloud never knew this project), concluded the row was stale, and silently dropped it from the candidates. With nolocal-pathcandidate,decideProjectImportreturnedimport— so the wizard re-imported, andcreateFromImportLocalunconditionally inserted a fresh row.The probe was written for "cloud project deleted from another device", but that inference contradicts the delete saga's own principle in the same file ("Local is reality — the local deletes are the source of truth"): a surviving local row means the project exists on this device, regardless of what the cloud knows.
Confirmed empirically in a local dev stack: with 4 rows for a repo in host.db, live
findByPath {walkAllRemotes:true}returned{"candidates": [], "cloudErrors": []}, with a cloudNOT_FOUNDlogged per probe, and each Import-all press added exactly one more row per project.Fix
1.
findByPath: a local-DB row is authoritative (host-service). A row keyed by the repo's resolved git root short-circuits in both modes — exactly like the non-walkAllRemotesbranch always has. The staleness probe and thestaleLocalLinkfield are deleted (the field had zero consumers repo-wide). The cloud remote-walk still runs when no local row exists (candidate discovery for linking is unchanged).2.
createFromImportLocalis idempotent on repo path (host-service). Importing a repo that's already a project returns the existing project (with its ensured main workspace) and reports the new additivecreated: falseonCreateResult— duplicates become impossible from any caller. The reused row is left untouched: no rename, and the renderer now gates the v1 color/icon carry oncreated !== falseso a re-import can't overwrite v2 customizations (older hosts without the field keep today's behavior).3. Wizard adopts the workspaces-page pattern (desktop). Per-row import state lives in one page-level map (like
adoptStates); the header button readsImport all · N, showsImporting i/nwhile running, and disappears when nothing is pending; rows are disabled during the batch; failed rows show inline errors with Retry and stay pending;needs-relocatelives in shared state so it surfaces its confirm UI instead of being re-queued. Lookup failures are kept out of the import-retry path (retry refetches discovery; imports never run without discovery data).Testing
findByPathtests (local row survives with cloud unaware; cloud never called on a local hit; remote walk preserved when no local row),createFromImportLocalidempotency (same id, one row, customizations preserved), appearance gating, and the wizard's pending-selection + row-action matrix (batch disabling, retry semantics). Existing harness idioms only (bun:test, in-memory sqlite + real migrations, real temp git repos).bun run lint,bun run typecheck, full targeted test suite green.Behavior notes
staleLocalLinkis removed from thefindByPathwire shape (verified unused).createdonCreateResultis additive.plans/done/.Summary by cubic
Fixes the v1 “Bring over your projects” step so already-imported projects are detected and duplicates are no longer created. Makes import idempotent in
packages/host-serviceand updates theapps/desktopwizard to show clear Import All progress, guard retries during batch, and preserve user customizations.host-service:project.findByPathnow treats a local DB row as authoritative in both modes; removes the cloud staleness probe andstaleLocalLink. Still walks remotes only when no local row exists.host-service:createFromImportLocalis idempotent on repo path; reuses the existing project and returnscreated: false. Prevents duplicate rows and gates v1 appearance carry oncreated !== falseto preserve v2 customizations.desktop: Import Projects page uses a shared per-row state map. “Import all · N” shows counts and in-flight progress, disables rows and Retry during batch, surfaces inline errors with Retry, keeps “needs relocate” in shared state, and keeps lookup failures out of import retries.Written for commit c5333a8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes