refactor(desktop): rewrite v1→v2 migration as pull-based importer#4122
Merged
Conversation
Replaces the auto-running batch migration with a manual click-to-import flow across three pages (Projects / Workspaces / Presets) inside the welcome modal. v1MigrationState stays as an audit log; per-row status is derived live from host-service findByPath and the audit log so a re-run is naturally idempotent and cross-device dedup falls out for free. Removes ~3.4k LOC of batch orchestration: the auto-run hook, the heuristic findByPath fallbacks, the singleton concurrency guard, the localStorage event bus, the bulk sidebar writeback, and the separate preset auto-migration.
…ates
Three real bugs the auto-batch v1→v2 migration silently mishandled,
surfaced concretely while triaging a coworker's mangled import:
- findByPath only ever read the local repo's `origin` (or first GitHub
remote). Cloned-fork-as-origin contributors got linked to the wrong
v2 project. Now reads every GitHub remote, accepts an
`expectedRemoteUrl` hint from v1's recorded `githubOwner`, and tags
matching candidates so the picker can recommend them.
- Audit success was trusted blind. If another device or user deleted
the v2 project, the row stayed flagged "Imported" forever pointing
at a ghost. Adds `project.cloudList` + `workspace.cloudList`
procedures; pages cross-check audit v2 ids against live cloud and
demote ghosts back to "available".
- Linking a v1 project whose folder differs from where v2 already had
the project set up on this device threw a confusing CONFLICT. Now
surfaces a confirm row ("Already set up at X. Link to Y instead?")
with explicit Use-this-folder vs Cancel.
Plus: surface findByPath cloud query failures (silent `console.warn`
before, misleadingly fell through to "Import"); refresh button per
page; stale-worktree NOT_FOUND retry-by-branch on adopt;
listProjectWorktrees procedure for filtering v1 workspaces against
the project's current `git worktree list` so guaranteed-to-fail rows
don't surface; invalidate the cloud-list query after every successful
import so the audit-ghost detector doesn't false-flag fresh imports.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces automatic V1→V2 migration with a user-driven import workflow: removes legacy migration modules and hooks; adds a multi-page V1 import modal (welcome → projects → workspaces → presets), a dismissible dashboard banner, TRPC cloud-aware endpoints and worktree listing, and a local DB table to track per-item import state. ChangesV1 Import Flow & Migration Removal
Sequence DiagramsequenceDiagram
participant User
participant Dashboard as Dashboard UI
participant Modal as V1ImportModal
participant Store as V1Modal Store
participant Backend as TRPC Backend
participant Cloud as Cloud API
participant DB as Local DB
User->>Dashboard: View dashboard (V2 enabled)
Dashboard->>Backend: readV1Projects()
Backend-->>Dashboard: v1 projects
Dashboard->>Backend: listState(orgId)
Backend-->>Dashboard: migration audit records
User->>Dashboard: Click "Import from v1"
Dashboard->>Store: openModal()
Store-->>Modal: isOpen=true,page="welcome"
User->>Modal: Next -> Projects page
Modal->>Backend: readV1Projects()
Backend-->>Modal: v1 projects
User->>Modal: Request project import
Modal->>Backend: findByPath(repoPath, expectedRemoteUrl?)
Backend->>Cloud: query candidates by remote URL(s)
Cloud-->>Backend: cloud project matches
Backend-->>Modal: candidates + cloudErrors
Modal->>Backend: create/setup v2 project
Backend->>DB: insert/update v2 records
DB-->>Backend: persisted
Modal->>Backend: upsertState(v1Id, kind, v2Id, status)
Backend->>DB: update v1_migration_state
DB-->>Backend: persisted
Modal->>Backend: listProjectWorktrees(projectId)
Backend-->>Modal: worktrees (branch/path)
User->>Modal: Import presets
Modal->>Backend: insert presets + upsertState
Backend->>DB: insert presets and state
DB-->>Backend: persisted
User->>Modal: Done
Modal->>Store: closeModal()
Store-->>Dashboard: modal closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 replaces the automatic v1→v2 batch migration with a click-to-import modal, deleting ~3.4k LOC of batch orchestration and replacing it with three per-item import pages (Projects, Workspaces, Presets), each with idempotent state stored in the audit log.
Confidence Score: 3/5The core import flow is well-structured but two defects in the project-linking path can silently create duplicate v2 projects instead of linking existing ones. The staleness check in
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/project/project.ts | Rewrites findByPath to query all GitHub remotes and an optional expectedRemoteUrl hint; adds cloudList procedure. Staleness check catches all exceptions (not just NOT_FOUND), risking false-ghost classification on transient network failures. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx | New per-row import UI for v1 projects; handles ghost-detection and relocate-confirm flow well, but runImport with a linkToProjectId can fall through to project creation when the candidate list is stale; path splitting uses Unix-only separator for remote-URL hint. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx | New per-row workspace importer; ghost-detection and stale-worktree retry logic look correct; NOT_FOUND retry path correctly strips worktreePath before the second attempt. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportPresetsPage/ImportPresetsPage.tsx | Straightforward preset import with local-only state; no cloud cross-check needed since presets are local. Logic is clean. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/V1ImportModal.tsx | Modal shell with page-order navigation; transition logic and page guards look correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsx | Dismiss-on-session banner with a redundant useEffect that re-reads sessionStorage after the synchronous initializer already did; causes an extra render on mount. |
| packages/host-service/src/trpc/router/workspace-creation/procedures/list-project-worktrees.ts | New listProjectWorktrees procedure that filters to non-bare, non-prunable, branched worktrees; clean and straightforward. |
| packages/host-service/src/trpc/router/workspace/workspace.ts | Adds cloudList procedure for ghost-detection; minimal and correct. |
| apps/desktop/src/renderer/stores/v1-import-modal.ts | Clean Zustand store for modal open state and page navigation. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportRow/ImportRow.tsx | Well-typed discriminated union for row actions; all variants handled with appropriate UI. Pick/confirm/error/blocked states all look correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Banner[V1ImportBanner\nshows remaining count] -->|click| Modal[V1ImportModal]
Modal --> Welcome[WelcomePage]
Welcome -->|Next| Projects[ImportProjectsPage]
Projects -->|Next| Workspaces[ImportWorkspacesPage]
Workspaces -->|Next| Presets[ImportPresetsPage]
Presets -->|Done| Close[Modal Closed]
Projects --> FindByPath[project.findByPath\nall remotes + expectedRemoteUrl]
FindByPath -->|1 candidate| Link[project.setup\nlinkToProjectId]
FindByPath -->|0 candidates| Create[project.create\nimportLocal]
FindByPath -->|N candidates| Pick[Pick dropdown\nmatchesExpected tagged]
Link -->|CONFLICT| Relocate[Confirm: Use this folder?\nallowRelocate=true retry]
Link -->|OK| AuditUpsert[upsertState linked]
Create --> AuditUpsert
Relocate -->|confirm| Link
AuditUpsert --> CloudListInvalidate[invalidate projectCloudList\nghost-detection refreshed]
Workspaces --> CloudWorkspaceList[workspace.cloudList\nghost cross-check]
Workspaces --> ListWorktrees[listProjectWorktrees\nfilter orphan branches]
Workspaces --> Adopt[workspaceCreation.adopt\n+worktreePath]
Adopt -->|NOT_FOUND| AdoptRetry[adopt retry\nwithout worktreePath]
AdoptRetry --> AuditWs[upsertState success]
Adopt -->|OK| AuditWs
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
packages/host-service/src/trpc/router/project/project.ts:197-210
**Transient errors in staleness check silently hide valid candidates**
`v2Project.get` is called to verify the local project is still alive in cloud, but the bare `catch` marks `staleLocalLink = true` for _any_ exception — including network timeouts, auth errors, or server 5xx responses — not only a project-not-found 404. When that happens the legitimate local candidate is filtered out of the results, and the next render surfaces zero candidates, pushing the user toward creating a new project instead of linking.
A second issue: the `source` field is never updated in the cloud-URL loop even when `findByGitHubRemote` already confirmed the project exists. Because `source === "local-path"` remains true, this extra `v2Project.get` call fires even when the previous loop already proved the project is reachable, doubling the round-trips and amplifying the window for a false-stale classification.
The fix is to (a) only set `staleLocalLink` on a `TRPCError` with code `NOT_FOUND`, and (b) add a `cloudConfirmed` flag to `Candidate` that the remote-URL loop sets to `true`; skip the staleness call when that flag is already set.
### Issue 2 of 4
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx:234-247
**`linkToProjectId` falls through to project creation when candidate goes stale**
When `runImport(pendingRelocate.v2ProjectId, { allowRelocate: true })` is called (the "Use this folder" confirm path), `targetCandidate` is resolved by scanning `findByPathQuery.data?.candidates`. If the query has re-fetched between when `pendingRelocate` was first set and when the user clicks "Use this folder" — and that re-fetch returns a different or empty candidates list — `candidates.find()` returns `undefined`. The code then falls through to `project.create.mutate`, silently creating a new v2 project instead of re-linking the existing one. The `pendingRelocate.v2ProjectId` is never used in the `else` branch.
### Issue 3 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsx:20-27
The `useEffect` re-reads `sessionStorage` after mount and calls `setDismissed` unconditionally, triggering an extra render on every mount. Since the `useState` initializer already reads from `sessionStorage` synchronously in the browser, the effect only adds a second paint with the same value.
```suggestion
const [dismissed, setDismissed] = useState(() => {
if (typeof window === "undefined") return false;
return sessionStorage.getItem(DISMISS_SESSION_KEY) === "1";
});
```
### Issue 4 of 4
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx:146
`expectedRemoteUrlFor` splits `mainRepoPath` on `"/"`, which silently produces `undefined` on Windows paths that use backslashes. If `repoName` is `undefined` the function returns `undefined` and the fork-matching hint is never sent, so the picker silently falls back to showing all candidates with none tagged "matches v1".
```suggestion
const repoName = project.mainRepoPath.split(/[\\/]/).filter(Boolean).pop();
```
Reviews (1): Last reviewed commit: "refactor(desktop): harden v1→v2 importer..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+197
to
+210
| err, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Detect stale local-DB row: returned by the path lookup but cloud | ||
| // never confirmed it via any remote URL. Two ways this happens: | ||
| // (a) the cloud project was deleted; (b) the local row's tracked | ||
| // remote URL no longer matches any current remote on the repo. | ||
| if (localProject) { | ||
| const candidate = byId.get(localProject.id); | ||
| if (candidate && candidate.source === "local-path") { | ||
| try { | ||
| await ctx.api.v2Project.get.query({ |
There was a problem hiding this comment.
v2Project.get is called to verify the local project is still alive in cloud, but the bare catch marks staleLocalLink = true for any exception — including network timeouts, auth errors, or server 5xx responses — not only a project-not-found 404. When that happens the legitimate local candidate is filtered out of the results, and the next render surfaces zero candidates, pushing the user toward creating a new project instead of linking.
A second issue: the source field is never updated in the cloud-URL loop even when findByGitHubRemote already confirmed the project exists. Because source === "local-path" remains true, this extra v2Project.get call fires even when the previous loop already proved the project is reachable, doubling the round-trips and amplifying the window for a false-stale classification.
The fix is to (a) only set staleLocalLink on a TRPCError with code NOT_FOUND, and (b) add a cloudConfirmed flag to Candidate that the remote-URL loop sets to true; skip the staleness call when that flag is already set.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/project/project.ts
Line: 197-210
Comment:
**Transient errors in staleness check silently hide valid candidates**
`v2Project.get` is called to verify the local project is still alive in cloud, but the bare `catch` marks `staleLocalLink = true` for _any_ exception — including network timeouts, auth errors, or server 5xx responses — not only a project-not-found 404. When that happens the legitimate local candidate is filtered out of the results, and the next render surfaces zero candidates, pushing the user toward creating a new project instead of linking.
A second issue: the `source` field is never updated in the cloud-URL loop even when `findByGitHubRemote` already confirmed the project exists. Because `source === "local-path"` remains true, this extra `v2Project.get` call fires even when the previous loop already proved the project is reachable, doubling the round-trips and amplifying the window for a false-stale classification.
The fix is to (a) only set `staleLocalLink` on a `TRPCError` with code `NOT_FOUND`, and (b) add a `cloudConfirmed` flag to `Candidate` that the remote-URL loop sets to `true`; skip the staleness call when that flag is already set.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+234
to
+247
| mainWorkspaceId = result.mainWorkspaceId; | ||
| repoPath = result.repoPath; | ||
| status = "linked"; | ||
| } catch (err) { | ||
| // Setup throws CONFLICT when the v2 project is already set | ||
| // up at a different folder on this device. Surface a | ||
| // confirm so the user can opt in to repointing instead of | ||
| // silently failing. | ||
| if (isAlreadySetUpElsewhereError(err) && !options.allowRelocate) { | ||
| setPendingRelocate({ | ||
| v2ProjectId: targetCandidate.id, | ||
| message: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| setRunning(false); |
There was a problem hiding this comment.
linkToProjectId falls through to project creation when candidate goes stale
When runImport(pendingRelocate.v2ProjectId, { allowRelocate: true }) is called (the "Use this folder" confirm path), targetCandidate is resolved by scanning findByPathQuery.data?.candidates. If the query has re-fetched between when pendingRelocate was first set and when the user clicks "Use this folder" — and that re-fetch returns a different or empty candidates list — candidates.find() returns undefined. The code then falls through to project.create.mutate, silently creating a new v2 project instead of re-linking the existing one. The pendingRelocate.v2ProjectId is never used in the else branch.
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: 234-247
Comment:
**`linkToProjectId` falls through to project creation when candidate goes stale**
When `runImport(pendingRelocate.v2ProjectId, { allowRelocate: true })` is called (the "Use this folder" confirm path), `targetCandidate` is resolved by scanning `findByPathQuery.data?.candidates`. If the query has re-fetched between when `pendingRelocate` was first set and when the user clicks "Use this folder" — and that re-fetch returns a different or empty candidates list — `candidates.find()` returns `undefined`. The code then falls through to `project.create.mutate`, silently creating a new v2 project instead of re-linking the existing one. The `pendingRelocate.v2ProjectId` is never used in the `else` branch.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+20
to
+27
| const [dismissed, setDismissed] = useState(() => { | ||
| if (typeof window === "undefined") return false; | ||
| return sessionStorage.getItem(DISMISS_SESSION_KEY) === "1"; | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| setDismissed(sessionStorage.getItem(DISMISS_SESSION_KEY) === "1"); | ||
| }, []); |
There was a problem hiding this comment.
useEffect re-reads sessionStorage after mount and calls setDismissed unconditionally, triggering an extra render on every mount. Since the useState initializer already reads from sessionStorage synchronously in the browser, the effect only adds a second paint with the same value.
Suggested change
| const [dismissed, setDismissed] = useState(() => { | |
| if (typeof window === "undefined") return false; | |
| return sessionStorage.getItem(DISMISS_SESSION_KEY) === "1"; | |
| }); | |
| useEffect(() => { | |
| setDismissed(sessionStorage.getItem(DISMISS_SESSION_KEY) === "1"); | |
| }, []); | |
| const [dismissed, setDismissed] = useState(() => { | |
| if (typeof window === "undefined") return false; | |
| return sessionStorage.getItem(DISMISS_SESSION_KEY) === "1"; | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsx
Line: 20-27
Comment:
The `useEffect` re-reads `sessionStorage` after mount and calls `setDismissed` unconditionally, triggering an extra render on every mount. Since the `useState` initializer already reads from `sessionStorage` synchronously in the browser, the effect only adds a second paint with the same value.
```suggestion
const [dismissed, setDismissed] = useState(() => {
if (typeof window === "undefined") return false;
return sessionStorage.getItem(DISMISS_SESSION_KEY) === "1";
});
```
How can I resolve this? If you propose a fix, please make it concise.| // repo path (which is `git rev-parse --show-toplevel`'s last segment | ||
| // for any clone) — more reliable than the project's display name, | ||
| // which users can rename. | ||
| const repoName = project.mainRepoPath.split("/").filter(Boolean).pop(); |
There was a problem hiding this comment.
expectedRemoteUrlFor splits mainRepoPath on "/", which silently produces undefined on Windows paths that use backslashes. If repoName is undefined the function returns undefined and the fork-matching hint is never sent, so the picker silently falls back to showing all candidates with none tagged "matches v1".
Suggested change
| const repoName = project.mainRepoPath.split("/").filter(Boolean).pop(); | |
| const repoName = project.mainRepoPath.split(/[\\/]/).filter(Boolean).pop(); |
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: 146
Comment:
`expectedRemoteUrlFor` splits `mainRepoPath` on `"/"`, which silently produces `undefined` on Windows paths that use backslashes. If `repoName` is `undefined` the function returns `undefined` and the fork-matching hint is never sent, so the picker silently falls back to showing all candidates with none tagged "matches v1".
```suggestion
const repoName = project.mainRepoPath.split(/[\\/]/).filter(Boolean).pop();
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/WelcomePage/WelcomePage.tsx (1)
10-15: 💤 Low value
GRADIENT_COLORSdeclares 4 colors but only the first is used.
DitheredBackgroundreceives a 4-tuplecolorsbut passes onlycolors[0]toDitheringascolorFront; the remaining three values (#fb923c,#f59e0b,#431407) are dead. Either drop the unused entries (and the 4-tuple type), or wire them into the shader (e.g., gradient stops) so the constant actually drives the visual. As-is, future contributors will assume the palette is in use and tweak it without effect.Also applies to: 38-63
🤖 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/components/WelcomePage/WelcomePage.tsx` around lines 10 - 15, GRADIENT_COLORS defines four colors but only colors[0] is used when DitheredBackground passes props into the Dithering shader, leaving the other three dead; either shrink GRADIENT_COLORS to a single color (remove the 4-tuple type) or modify DitheredBackground and the Dithering invocation to accept and use the full 4-tuple (e.g., pass colors[1], colors[2], colors[3] as additional shader inputs/gradient stops or named props like colorBack/colorAccent/colorHighlight) and update Dithering to consume those props in its shader/gradient logic so the full palette actually affects rendering.apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx (1)
57-189: 💤 Low valueConsider memoizing the derived maps and arrays computed on every render.
projectAuditByV1Id,workspaceAuditByV1Id,importedV2ProjectIds,validBranchesByV2ProjectId,projectsById,worktreesById,visibleWorkspaces, andgroupedare all rebuilt on every render. More importantly,importedV2ProjectIdsis a fresh array reference each render and is used to driveuseQueries(line 82) — react-query handles this via deep-equal queryKeys, but you're still allocating N new query option objects per render. Wrapping these derivations inuseMemokeyed on the source query data would also letvalidBranchesByV2ProjectId.entries()skip re-iteration during refresh interactions. Strictly optional given typical v1 dataset sizes.Also note line 79:
entry.v2Id as stringcasts away the optional type even though the preceding filter already narrows it. A type-predicate filter (.filter((e): e is AuditLogEntry & { v2Id: string } => …)) avoids the cast.🤖 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 57 - 189, The derived collections (projectAuditByV1Id, workspaceAuditByV1Id, importedV2ProjectIds, validBranchesByV2ProjectId, projectsById, worktreesById, visibleWorkspaces, grouped) should be memoized with useMemo and keyed on their source query data (e.g. auditQuery.data, projectsQuery.data, workspacesQuery.data, worktreesQuery.data, worktreeListQueries.map(q=>q.data), liveWorkspaceIds) so you stop rebuilding arrays/maps and recreating the useQueries option objects each render; wrap the logic that builds each Map/array in useMemo and keep the same variable names so the rest of the component (including the useQueries call that uses importedV2ProjectIds) continues to work. Also replace the cast entry.v2Id as string when building importedV2ProjectIds by using a type‑predicate filter such as .filter((e): e is AuditLogEntry & { v2Id: string } => !!e.v2Id && (e.status === "success" || e.status === "linked")) to avoid unsafe casting.packages/host-service/src/trpc/router/project/project.ts (3)
206-218: 💤 Low valueOptional: skip redundant
v2Project.getwhen cloud already confirmed the project.If any
findByGitHubRemotecall above returnedlocalProject.id, the project is provably reachable in cloud and this extraget.queryis a wasted round-trip. Tracking acloudConfirmedflag (set totruewhenever the cloud loop merges intobyId) lets you skip the probe in the common case.🤖 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/project/project.ts` around lines 206 - 218, The code currently always calls ctx.api.v2Project.get.query for localProject even when the cloud loop already confirmed the project; add a cloudConfirmed boolean on the byId entry (set to true whenever the cloud merge path inserts/updates that id) and skip the v2Project.get.query probe when byId.get(localProject.id).cloudConfirmed is true; update the cloud-merge logic that writes into byId to set cloudConfirmed=true and ensure the staleLocalLink assignment still happens when probe fails for non-cloudConfirmed entries (refer to symbols: localProject, byId, v2Project.get.query, staleLocalLink, and the cloud merge path that updates byId).
162-200: 💤 Low valueOptional: parallelize cloud lookups with
Promise.all.
findByGitHubRemoteis awaited sequentially per URL. For a fork with multiple remotes (the case this PR explicitly targets), this serializes round-trips that are independent.Promise.alloverurlsToQuery.values()would shorten total latency without changing semantics, since merge intobyIdis order-independent (theexisting.matchesExpected ||= …accumulation is commutative).♻️ Sketch
- const cloudErrors: { url: string; message: string }[] = []; - for (const parsed of urlsToQuery.values()) { - try { - const { candidates } = - await ctx.api.v2Project.findByGitHubRemote.query({ - organizationId: ctx.organizationId, - repoCloneUrl: parsed.url, - }); - for (const c of candidates) { /* … */ } - } catch (err) { /* … */ } - } + const cloudErrors: { url: string; message: string }[] = []; + const results = await Promise.all( + Array.from(urlsToQuery.values()).map(async (parsed) => { + try { + const { candidates } = + await ctx.api.v2Project.findByGitHubRemote.query({ + organizationId: ctx.organizationId, + repoCloneUrl: parsed.url, + }); + return { parsed, candidates, error: null as Error | null }; + } catch (err) { + return { parsed, candidates: [], error: err instanceof Error ? err : new Error(String(err)) }; + } + }), + ); + for (const { parsed, candidates, error } of results) { + if (error) { + cloudErrors.push({ url: parsed.url, message: error.message }); + continue; + } + for (const c of candidates) { /* merge into byId */ } + }🤖 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/project/project.ts` around lines 162 - 200, The cloud lookups are done sequentially in the for..of over urlsToQuery.values(), which slows forks with multiple remotes; refactor to run ctx.api.v2Project.findByGitHubRemote.query in parallel using Promise.all over urlsToQuery.values(), collecting per-URL results and errors, then iterate those results to perform the same merge logic into byId (preserving the existing.matchesExpected ||= matches(parsed.url) behavior and repoCloneUrl assignment) and push errors into cloudErrors (keeping the same message extraction and console.warn for failures); reference the urlsToQuery collection, ctx.api.v2Project.findByGitHubRemote.query call, cloudErrors array, byId Map, and matches(parsed.url) when making the change.
107-109: 💤 Low valueConsider using
await ctx.git(gitRoot)instead of constructingsimpleGitdirectly.The rest of this router (e.g., line 489) uses
ctx.git(...)to obtain a SimpleGit instance, which wrapssimpleGitwith credential provider setup and environment configuration. Using rawsimpleGit()here bypasses that wrapper and introduces inconsistency.🤖 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/project/project.ts` around lines 107 - 109, Replace the raw simpleGit(gitRoot) usage with the wrapped instance from ctx.git to preserve credential provider and env config: after resolving the repo with resolveLocalRepo(input.repoPath) and computing gitRoot, call await ctx.git(gitRoot) and pass that result into getGitHubRemotes instead of simpleGit(gitRoot); update any variable names if needed so getGitHubRemotes receives the SimpleGit instance from ctx.git while leaving resolveLocalRepo, gitRoot and getGitHubRemotes references intact.apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx (1)
53-61: 💤 Low valueRebuild
auditByV1Idinside auseMemo.The Map is reconstructed on every render even when neither
auditQuery.datanor the row identities change, and it's then read by everyProjectRowlookup. Wrapping inuseMemo([auditQuery.data])makes the dependency explicit and avoids repeated allocations on parent re-renders (e.g. whileisRefreshingtoggles).🤖 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 53 - 61, The Map auditByV1Id is rebuilt on every render; wrap its construction in React's useMemo so it only recalculates when auditQuery.data changes. Replace the top-level creation of auditByV1Id with a useMemo that iterates over auditQuery.data and returns the Map, and keep all existing keys/values (v1Id -> AuditLogEntry) intact so ProjectRow lookups continue to work; use [auditQuery.data] as the dependency array for the memo.apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/V1ImportModal.tsx (1)
42-49: 💤 Low value
window.setTimeout+ state update can fire after unmount.If the dialog closes (or the component unmounts) during the 160ms transition, the trailing
setPage/setIsTransitioningstill fires and React will warn about state updates on an unmounted component. Track the timer in a ref and clear it in cleanup, or guard with anisMountedRef.🤖 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/V1ImportModal.tsx` around lines 42 - 49, The transitionToPage function uses window.setTimeout to update state after TRANSITION_MS which can run after the component unmounts; track the timeout ID in a ref (e.g., timeoutRef) when you call window.setTimeout and clear it in a useEffect cleanup (or alternatively maintain an isMountedRef and check it before calling setPage/setIsTransitioning) so that setPage and setIsTransitioning in transitionToPage never run on an unmounted component.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsx (1)
20-27: 💤 Low valueRedundant
useEffectre-readingsessionStorage.
useState's lazy initializer at L20–23 already readssessionStorageon mount, and nothing writes the key from outside this component (thedismisshandler at L54–57 writes locally and updates state synchronously). TheuseEffectat L25–27 just re-sets the same value on mount, causing an extra render with no behavior change. Either drop the effect or, if you want cross-tab/cross-mount sync, listen to astorageevent instead.♻️ Proposed cleanup
const [dismissed, setDismissed] = useState(() => { if (typeof window === "undefined") return false; return sessionStorage.getItem(DISMISS_SESSION_KEY) === "1"; }); - - useEffect(() => { - setDismissed(sessionStorage.getItem(DISMISS_SESSION_KEY) === "1"); - }, []);(Remove the now-unused
useEffectimport.)🤖 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 20 - 27, The useEffect that calls setDismissed on mount is redundant because the lazy initializer for useState already reads sessionStorage; remove the useEffect import and the effect block entirely, leaving the lazy initializer as the sole source of initial dismissed state (referencing DISMISS_SESSION_KEY, dismissed, and setDismissed); if cross-tab sync is desired instead, replace the removed effect with a window "storage" event listener that updates setDismissed when DISMISS_SESSION_KEY changes.
🤖 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/ImportPresetsPage/ImportPresetsPage.tsx`:
- Around line 102-157: runImport can create duplicate v2 rows because you insert
into collections.v2TerminalPresets.insert with a fresh v2Id before recording the
audit via upsertState.mutateAsync; instead, make the operation idempotent by
obtaining or reserving the v2Id via the audit upsert first (call
upsertState.mutateAsync for preset.id with status:"pending" and use any
returned/audited v2Id or the existing audit.v2Id), then insert into
collections.v2TerminalPresets.insert only if a row with that v2Id does not
already exist (or when the audit indicates success), and finally update the
audit to status:"success" and invalidate via
trpcUtils.migration.listState.invalidate; reference runImport, preset.id, v2Id,
upsertState.mutateAsync, and collections.v2TerminalPresets.insert when applying
this change.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx`:
- Around line 33-51: cloudProjectsQuery failures are currently ignored by
isLoading causing liveProjectIds to remain null forever and ghost-detection to
be silently disabled; update the loading/error handling so cloud query failures
are surfaced or retried: either include cloudProjectsQuery.isPending (and/or
cloudProjectsQuery.isError) in the isLoading calculation used by the page (the
isLoading variable) or add a small retry/backoff on cloudProjectsQuery (remove
retry:false or set retry: 1) and ensure liveProjectIds reacts to
cloudProjectsQuery.status so rows can demote when cloudList
(project.cloudList.query) fails; touch cloudProjectsQuery, liveProjectIds, and
isLoading to implement this change.
- Around line 219-261: If linkToProjectId is provided but candidates.find(...)
returns undefined (targetCandidate is falsy), do not fall through to the create
branch; instead surface an error and abort the import. Update the logic around
targetCandidate (the result of candidates.find and linkToProjectId) so that when
linkToProjectId != null and targetCandidate is undefined you set an
error/pending state (e.g., call setRunning(false) and set an appropriate
user-facing error or throw a specific error) and return early, rather than
proceeding to client.project.create.mutate; keep the existing successful path
for when targetCandidate exists and leave client.project.setup.mutate handling
unchanged.
- Around line 129-134: The current brittle parsing in extractExistingPath and
isAlreadySetUpElsewhereError should be replaced by using a structured tRPC error
contract: update the server to return a CONFLICT tRPC error with data.cause = {
kind: "ALREADY_SET_UP_ELSEWHERE", existingPath: string } (matching
workspace-cleanup's pattern), then change isAlreadySetUpElsewhereError to detect
that discriminator (err.data?.code === "CONFLICT" && err.data?.cause?.kind ===
"ALREADY_SET_UP_ELSEWHERE") and change extractExistingPath to read the path from
err.data.cause.existingPath instead of regex-ing the message; finally update the
ImportProjectsPage code that sets pendingRelocate to pull the path from the
cause.existingPath when the discriminator matches.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx`:
- Around line 340-357: The silent .catch on upsertState.mutateAsync must be
replaced with explicit error handling: remove .catch(() => {}) and instead catch
the upsert error, log it (e.g., console.error or your app logger) including the
original adopt error and the upsert error, and attempt a fallback audit write
that marks the workspace with a degraded status (e.g., call
upsertState.mutateAsync with status: "audit_write_failed" and a reason combining
both messages); if the fallback also fails, log that failure and do not swallow
it silently. Make these changes around upsertState.mutateAsync in the try/catch
where adopt.mutate and setErrorMessage are used, and still call
trpcUtils.migration.listState.invalidate({ organizationId }) after handling.
- Around line 43-50: cloudWorkspacesQuery is currently silent on errors which
leaves liveWorkspaceIds null and the UI showing "No v1 workspaces left to
import" without surfacing the fetch failure; update the empty-state / candidate
computation to check cloudWorkspacesQuery.error (similar to ImportProjectsPage
using findByPathQuery.data.cloudErrors) and expose that error as cloudErrors in
the same shape the UI expects, display a user-facing error message and a retry
action that refetches cloudWorkspacesQuery, and ensure any logic that treats
liveWorkspaceIds === null as a "trust audit" fallback only runs when there is no
cloudWorkspacesQuery.error to avoid hiding failures.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/V1ImportModal.tsx`:
- Around line 84-104: The fallback "Host service is not ready" is rendered even
when page === "presets"; update the conditional that shows the fallback to
exclude the presets page so ImportPresetsPage can render without an
activeHostUrl. Concretely, modify the fallback check (uses page and
activeHostUrl in V1ImportModal) from page !== "welcome" && !activeHostUrl to
page !== "welcome" && page !== "presets" && !activeHostUrl so ImportPresetsPage
(the ImportPresetsPage component) is not overlapped by the fallback.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsx`:
- Around line 20-27: The useEffect that calls setDismissed on mount is redundant
because the lazy initializer for useState already reads sessionStorage; remove
the useEffect import and the effect block entirely, leaving the lazy initializer
as the sole source of initial dismissed state (referencing DISMISS_SESSION_KEY,
dismissed, and setDismissed); if cross-tab sync is desired instead, replace the
removed effect with a window "storage" event listener that updates setDismissed
when DISMISS_SESSION_KEY changes.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/WelcomePage/WelcomePage.tsx`:
- Around line 10-15: GRADIENT_COLORS defines four colors but only colors[0] is
used when DitheredBackground passes props into the Dithering shader, leaving the
other three dead; either shrink GRADIENT_COLORS to a single color (remove the
4-tuple type) or modify DitheredBackground and the Dithering invocation to
accept and use the full 4-tuple (e.g., pass colors[1], colors[2], colors[3] as
additional shader inputs/gradient stops or named props like
colorBack/colorAccent/colorHighlight) and update Dithering to consume those
props in its shader/gradient logic so the full palette actually affects
rendering.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx`:
- Around line 53-61: The Map auditByV1Id is rebuilt on every render; wrap its
construction in React's useMemo so it only recalculates when auditQuery.data
changes. Replace the top-level creation of auditByV1Id with a useMemo that
iterates over auditQuery.data and returns the Map, and keep all existing
keys/values (v1Id -> AuditLogEntry) intact so ProjectRow lookups continue to
work; use [auditQuery.data] as the dependency array for the memo.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx`:
- Around line 57-189: The derived collections (projectAuditByV1Id,
workspaceAuditByV1Id, importedV2ProjectIds, validBranchesByV2ProjectId,
projectsById, worktreesById, visibleWorkspaces, grouped) should be memoized with
useMemo and keyed on their source query data (e.g. auditQuery.data,
projectsQuery.data, workspacesQuery.data, worktreesQuery.data,
worktreeListQueries.map(q=>q.data), liveWorkspaceIds) so you stop rebuilding
arrays/maps and recreating the useQueries option objects each render; wrap the
logic that builds each Map/array in useMemo and keep the same variable names so
the rest of the component (including the useQueries call that uses
importedV2ProjectIds) continues to work. Also replace the cast entry.v2Id as
string when building importedV2ProjectIds by using a type‑predicate filter such
as .filter((e): e is AuditLogEntry & { v2Id: string } => !!e.v2Id && (e.status
=== "success" || e.status === "linked")) to avoid unsafe casting.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/V1ImportModal.tsx`:
- Around line 42-49: The transitionToPage function uses window.setTimeout to
update state after TRANSITION_MS which can run after the component unmounts;
track the timeout ID in a ref (e.g., timeoutRef) when you call window.setTimeout
and clear it in a useEffect cleanup (or alternatively maintain an isMountedRef
and check it before calling setPage/setIsTransitioning) so that setPage and
setIsTransitioning in transitionToPage never run on an unmounted component.
In `@packages/host-service/src/trpc/router/project/project.ts`:
- Around line 206-218: The code currently always calls
ctx.api.v2Project.get.query for localProject even when the cloud loop already
confirmed the project; add a cloudConfirmed boolean on the byId entry (set to
true whenever the cloud merge path inserts/updates that id) and skip the
v2Project.get.query probe when byId.get(localProject.id).cloudConfirmed is true;
update the cloud-merge logic that writes into byId to set cloudConfirmed=true
and ensure the staleLocalLink assignment still happens when probe fails for
non-cloudConfirmed entries (refer to symbols: localProject, byId,
v2Project.get.query, staleLocalLink, and the cloud merge path that updates
byId).
- Around line 162-200: The cloud lookups are done sequentially in the for..of
over urlsToQuery.values(), which slows forks with multiple remotes; refactor to
run ctx.api.v2Project.findByGitHubRemote.query in parallel using Promise.all
over urlsToQuery.values(), collecting per-URL results and errors, then iterate
those results to perform the same merge logic into byId (preserving the
existing.matchesExpected ||= matches(parsed.url) behavior and repoCloneUrl
assignment) and push errors into cloudErrors (keeping the same message
extraction and console.warn for failures); reference the urlsToQuery collection,
ctx.api.v2Project.findByGitHubRemote.query call, cloudErrors array, byId Map,
and matches(parsed.url) when making the change.
- Around line 107-109: Replace the raw simpleGit(gitRoot) usage with the wrapped
instance from ctx.git to preserve credential provider and env config: after
resolving the repo with resolveLocalRepo(input.repoPath) and computing gitRoot,
call await ctx.git(gitRoot) and pass that result into getGitHubRemotes instead
of simpleGit(gitRoot); update any variable names if needed so getGitHubRemotes
receives the SimpleGit instance from ctx.git while leaving resolveLocalRepo,
gitRoot and getGitHubRemotes references intact.
🪄 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: b351a706-047d-4d33-8ddc-d8ee5bb42cb4
📒 Files selected for processing (41)
apps/desktop/src/lib/trpc/routers/migration/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/V2PresetsBar/V2PresetsBar.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportPresetsPage/ImportPresetsPage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportPresetsPage/index.tsapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/index.tsapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/index.tsapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/V1ImportModal.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportPageShell/ImportPageShell.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportPageShell/index.tsapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportRow/ImportRow.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportRow/index.tsapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/WelcomePage/WelcomePage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/WelcomePage/index.tsapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/index.tsapps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/normalize.test.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/normalize.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/writeSidebarState.test.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/writeSidebarState.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1PresetsToV2/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1PresetsToV2/useMigrateV1PresetsToV2.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2PresetsSection/V2PresetsSection.tsxapps/desktop/src/renderer/stores/v1-import-modal.tspackages/host-service/src/trpc/router/project/project.tspackages/host-service/src/trpc/router/workspace-creation/procedures/index.tspackages/host-service/src/trpc/router/workspace-creation/procedures/list-project-worktrees.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.tspackages/host-service/src/trpc/router/workspace/workspace.tspackages/local-db/src/schema/schema.ts
💤 Files with no reviewable changes (14)
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1PresetsToV2/index.ts
- apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/index.ts
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/index.ts
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts
- apps/desktop/src/renderer/routes/_authenticated/components/V1MigrationSummaryModal/V1MigrationSummaryModal.tsx
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.ts
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1PresetsToV2/useMigrateV1PresetsToV2.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/V2PresetsBar/V2PresetsBar.tsx
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.test.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2PresetsSection/V2PresetsSection.tsx
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/writeSidebarState.ts
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/normalize.ts
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/writeSidebarState.test.ts
- apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/normalize.test.ts
Comment on lines
+33
to
+51
| 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]); | ||
|
|
||
| const isLoading = | ||
| projectsQuery.isPending || auditQuery.isPending || !projectsQuery.data; |
Contributor
There was a problem hiding this comment.
Cloud query failures aren't reflected in isLoading and ghost-detection becomes silently disabled.
cloudProjectsQuery has retry: false and isn't included in isLoading. If project.cloudList fails (offline, transient 500), liveProjectIds stays null indefinitely, which means audit-ghost demotion never runs and rows that were deleted in the cloud keep showing as Imported — the exact failure mode the cloudList procedure was added to fix. Consider either surfacing the cloud error at the page level (similar to per-row cloudErrors at L364) or adding a small retry, so users aren't misled by stale "Imported" labels when cloud is unreachable.
🤖 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 33 - 51, cloudProjectsQuery failures are currently ignored by
isLoading causing liveProjectIds to remain null forever and ghost-detection to
be silently disabled; update the loading/error handling so cloud query failures
are surfaced or retried: either include cloudProjectsQuery.isPending (and/or
cloudProjectsQuery.isError) in the isLoading calculation used by the page (the
isLoading variable) or add a small retry/backoff on cloudProjectsQuery (remove
retry:false or set retry: 1) and ensure liveProjectIds reacts to
cloudProjectsQuery.status so rows can demote when cloudList
(project.cloudList.query) fails; touch cloudProjectsQuery, liveProjectIds, and
isLoading to implement this change.
Comment on lines
+129
to
+134
| function extractExistingPath(message: string): string | null { | ||
| const match = message.match( | ||
| /already set up on this device at (.+?)\.\s+Remove/, | ||
| ); | ||
| return match?.[1] ?? null; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how project.setup signals the "already set up" CONFLICT and whether
# typed error data (TRPCError code/cause/data) is already exposed.
rg -nP -C5 'already set up on this device at' --type=ts
rg -nP -C5 'allowRelocate' --type=tsRepository: superset-sh/superset
Length of output: 20620
🏁 Script executed:
# Check how TRPCError is used elsewhere in the codebase with data fields
rg -n 'new TRPCError' -A 5 --type=ts packages/host-service | head -60
# Check if there are examples of error data structures
rg -n 'TRPCError.*data' --type=ts | head -20Repository: superset-sh/superset
Length of output: 5192
🏁 Script executed:
# Check if any TRPCError in the codebase uses a data field
rg -n 'TRPCError' -A 10 --type=ts packages/host-service | rg -A 10 'data:'
# Check the TRPCError import and type definition
rg -n 'from.*TRPCError\|import.*TRPCError' --type=ts packages/host-service
# Look at tRPC version and config
cat packages/host-service/package.json | grep -A 2 '@trpc'Repository: superset-sh/superset
Length of output: 3079
🏁 Script executed:
# Check the TRPCError type signature and its constructor options
rg -n 'TRPCError.*{' -A 5 --type=ts packages/host-service/src/trpc | grep -A 5 'code:' | head -40
# Check if the codebase ever passes a data parameter to TRPCError
rg -n 'new TRPCError' -A 3 --type=ts packages/host-service | rg 'data:' -B 3
# Look at the actual `@trpc/server` types for TRPCError
fd -e 'd.ts' node_modules/@trpc/server 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 152
🏁 Script executed:
# Check if any TRPCError uses a data field
rg -n 'new TRPCError' -A 5 --type=ts packages/host-service | grep -B 5 -A 1 'data'
# Search for TRPCError constructor calls to see what fields are supported
rg 'code:.*CONFLICT' -A 2 -B 2 --type=ts packages/host-service/src/trpc/router/project/project.ts
# Check if there's any documentation or type hints about TRPCError in the codebase
rg 'TRPCError' --type=ts packages/host-service/src/trpc | head -20Repository: superset-sh/superset
Length of output: 5307
🏁 Script executed:
# Check the tRPC version and search online documentation or installed types
cat packages/host-service/package.json | grep '@trpc'
# Try to find if there's a cause or data field supported in TRPCError
python3 << 'EOF'
import json
with open('packages/host-service/package.json', 'r') as f:
package = json.load(f)
trpc_version = package.get('dependencies', {}).get('@trpc/server', '')
print(f"tRPC server version: {trpc_version}")
EOF
# Check if there are any examples of structured error responses in tRPC routers
rg 'cause|data' -B 5 --type=ts packages/host-service/src/trpc/router | grep -A 5 -B 5 'TRPCError' | head -30Repository: superset-sh/superset
Length of output: 2927
🏁 Script executed:
# Check the actual cause field usage pattern in workspace-cleanup to understand the structure
rg -B 2 -A 5 'cause:.*satisfies' --type=ts packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts
# Verify the exact location where the "already set up" error is thrown for final context
sed -n '320,335p' packages/host-service/src/trpc/router/project/project.tsRepository: superset-sh/superset
Length of output: 877
Brittle string-matching couples the renderer to the host-service error text.
Both isAlreadySetUpElsewhereError and extractExistingPath parse a free-form server message ("Project is already set up on this device at <path>. Remove ..."). Any wording tweak on the server (punctuation, casing, wrapping the path in quotes) silently breaks the relocate-confirm flow — pendingRelocate won't be set and the user will see a generic error. The codebase already uses structured error data (see workspace-cleanup.ts with cause field), so refactor this to use a typed discriminator on the tRPC error (e.g. code: "CONFLICT" with cause: { kind: "ALREADY_SET_UP_ELSEWHERE", existingPath }) to make this contract explicit rather than parsing message text.
🤖 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 129 - 134, The current brittle parsing in extractExistingPath and
isAlreadySetUpElsewhereError should be replaced by using a structured tRPC error
contract: update the server to return a CONFLICT tRPC error with data.cause = {
kind: "ALREADY_SET_UP_ELSEWHERE", existingPath: string } (matching
workspace-cleanup's pattern), then change isAlreadySetUpElsewhereError to detect
that discriminator (err.data?.code === "CONFLICT" && err.data?.cause?.kind ===
"ALREADY_SET_UP_ELSEWHERE") and change extractExistingPath to read the path from
err.data.cause.existingPath instead of regex-ing the message; finally update the
ImportProjectsPage code that sets pendingRelocate to pull the path from the
cause.existingPath when the discriminator matches.
Comment on lines
+43
to
+50
| const cloudWorkspacesQuery = useQuery({ | ||
| queryKey: [...WORKSPACE_CLOUD_LIST_KEY, organizationId, activeHostUrl], | ||
| queryFn: async () => { | ||
| const client = getHostServiceClientByUrl(activeHostUrl); | ||
| return client.workspace.cloudList.query(); | ||
| }, | ||
| retry: false, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ImportProjectsPage / V1ImportModal surface cloud-list errors
# in a way that ImportWorkspacesPage relies on, vs. workspaces being a gap.
fd -t f 'ImportProjectsPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | xargs rg -nC3 -P 'cloudList|cloudListError|isError|error\b'
fd -t f 'V1ImportModal.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | xargs rg -nC3 -P 'isError|error\b|cloudList'
fd -t f 'ImportPageShell.tsx' | xargs rg -nC3 -P 'error|isError'Repository: superset-sh/superset
Length of output: 2140
🏁 Script executed:
fd -t f 'ImportWorkspacesPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs cat -n | sed -n '140,210p'Repository: superset-sh/superset
Length of output: 3058
🏁 Script executed:
fd -t f 'ImportWorkspacesPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs rg -nC5 'cloudErrors|cloudWorkspacesQuery\.error|cloudWorkspacesQuery\.isError'Repository: superset-sh/superset
Length of output: 48
🏁 Script executed:
fd -t f 'ImportWorkspacesPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs cat -n | sed -n '110,135p'Repository: superset-sh/superset
Length of output: 869
🏁 Script executed:
fd -t f 'ImportWorkspacesPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs rg -n 'cloudWorkspacesQuery\.(isError|error|refetch)'Repository: superset-sh/superset
Length of output: 104
🏁 Script executed:
fd -t f 'ImportPageShell.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs cat -n | sed -n '1,50p'Repository: superset-sh/superset
Length of output: 1678
🏁 Script executed:
fd -t f 'ImportPageShell.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs rg -n 'props\.|type.*Props|interface.*Props' | head -15Repository: superset-sh/superset
Length of output: 99
🏁 Script executed:
fd -t f 'ImportProjectsPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs rg -nC8 'return.*ImportPageShell'Repository: superset-sh/superset
Length of output: 48
🏁 Script executed:
fd -t f 'ImportProjectsPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModalRepository: superset-sh/superset
Length of output: 179
🏁 Script executed:
fd -t f 'ImportProjectsPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs wc -lRepository: superset-sh/superset
Length of output: 183
🏁 Script executed:
fd -t f 'ImportProjectsPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs cat -n | sed -n '360,385p'Repository: superset-sh/superset
Length of output: 1009
🏁 Script executed:
fd -t f 'ImportWorkspacesPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs cat -n | sed -n '43,55p'Repository: superset-sh/superset
Length of output: 613
🏁 Script executed:
fd -t f 'ImportProjectsPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs cat -n | sed -n '26,42p'Repository: superset-sh/superset
Length of output: 753
🏁 Script executed:
fd -t f 'ImportProjectsPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs rg -n 'const findByPathQuery|const cloudProjectsQuery'Repository: superset-sh/superset
Length of output: 146
🏁 Script executed:
fd -t f 'ImportProjectsPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs cat -n | sed -n '43,80p'Repository: superset-sh/superset
Length of output: 1351
🏁 Script executed:
fd -t f 'ImportProjectsPage.tsx' apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal | head -1 | xargs cat -n | sed -n '185,210p'Repository: superset-sh/superset
Length of output: 841
Surface cloud-list errors for workspaces, similar to projects.
cloudWorkspacesQuery fails silently: when it errors, liveWorkspaceIds stays null, causing the fallback to "trust audit until proven otherwise". The page then shows "No v1 workspaces left to import" with no indication that the cloud list fetch failed. In contrast, ImportProjectsPage surfaces cloud errors via findByPathQuery.data.cloudErrors when candidates are empty. If workspaces rely on the cloud list to detect stale v2 workspaces (lines 160–163), then silent failures could leave imported workspaces showing "Imported" even after deletion. The refresh button is the only recovery path. Consider adding explicit error handling for cloudWorkspacesQuery.error (similar to projects) in the empty state or action computation to surface this failure to the user and offer a retry path.
🤖 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 43 - 50, cloudWorkspacesQuery is currently silent on errors which
leaves liveWorkspaceIds null and the UI showing "No v1 workspaces left to
import" without surfacing the fetch failure; update the empty-state / candidate
computation to check cloudWorkspacesQuery.error (similar to ImportProjectsPage
using findByPathQuery.data.cloudErrors) and expose that error as cloudErrors in
the same shape the UI expects, display a user-facing error message and a retry
action that refetches cloudWorkspacesQuery, and ensure any logic that treats
liveWorkspaceIds === null as a "trust audit" fallback only runs when there is no
cloudWorkspacesQuery.error to avoid hiding failures.
Hardening commit removed the short-circuit and started always walking all remotes + merging cloud results. That changed folder-first import behavior: if your org happens to have multiple v2 projects pointing at the same remote URL, re-importing a folder you'd already linked locally now triggers the multi-project picker instead of silently linking to the local row. /settings/projects has no setup-at-path affordance, so the multi-project toast was a dead end. Restore the short-circuit, gated by a cloud existence check on the local v2 id. Non-stale local hit → return that candidate only. Stale local hit (cloud project deleted) → fall through to the multi-remote walk so callers see real alternatives. Migration importer is unaffected: v1 paths typically have no local-DB hit, so it still takes the cloud-walk path and gets the picker for forks etc.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/host-service/src/trpc/router/project/project.ts (1)
186-221: ⚡ Quick winFan out the remote lookups instead of awaiting them one by one.
These queries are independent, but the current
await-in-forloop adds one full cloud round trip per extra remote. In the fork/multi-remote case this PR is targeting, that directly stretches click latency.Promise.allSettledwould preserve the per-URL error collection while reducing the happy-path wait to the slowest lookup.🤖 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/project/project.ts` around lines 186 - 221, The loop over urlsToQuery.values() currently awaits each ctx.api.v2Project.findByGitHubRemote.query sequentially, causing serial cloud round-trips; change this to fire all lookups in parallel using Promise.allSettled over an array mapped from urlsToQuery.values(), then iterate the settled results: for fulfilled results apply the same logic that updates byId (use c.id, c.name, parsed.url, matches(parsed.url), staleLocalLink) and for rejected results push into cloudErrors with the same message extraction and console.warn. Ensure you preserve the existing behavior of updating existing.matchesExpected and existing.repoCloneUrl when an entry already exists, and reference the same symbols (urlsToQuery, ctx.api.v2Project.findByGitHubRemote.query, byId, matches, cloudErrors) so callers and error handling remain identical.
🤖 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 `@packages/host-service/src/trpc/router/project/project.ts`:
- Around line 155-177: The current catch around ctx.api.v2Project.get.query
treats any failure as a delete; change it so only a confirmed not-found (e.g.,
404 / "NOT_FOUND" from ctx.api.v2Project.get.query) sets stale = true, and for
any other error return the same candidate (using localProject.id,
localProject.repoName/basename(gitRoot), localProject.repoUrl,
matches(localProject.repoUrl ?? null)) but populate cloudErrors with the error
details (include error.message and a reference to the cloud endpoint or
error.name) so the importer sees a cloud error instead of marking the local link
stale; update the catch to inspect the thrown error and branch accordingly
rather than unconditionally setting stale.
---
Nitpick comments:
In `@packages/host-service/src/trpc/router/project/project.ts`:
- Around line 186-221: The loop over urlsToQuery.values() currently awaits each
ctx.api.v2Project.findByGitHubRemote.query sequentially, causing serial cloud
round-trips; change this to fire all lookups in parallel using
Promise.allSettled over an array mapped from urlsToQuery.values(), then iterate
the settled results: for fulfilled results apply the same logic that updates
byId (use c.id, c.name, parsed.url, matches(parsed.url), staleLocalLink) and for
rejected results push into cloudErrors with the same message extraction and
console.warn. Ensure you preserve the existing behavior of updating
existing.matchesExpected and existing.repoCloneUrl when an entry already exists,
and reference the same symbols (urlsToQuery,
ctx.api.v2Project.findByGitHubRemote.query, byId, matches, cloudErrors) so
callers and error handling remain identical.
🪄 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: 4dafc7c3-4794-4a3e-b27a-2aeded2bdf59
📒 Files selected for processing (1)
packages/host-service/src/trpc/router/project/project.ts
…in flag Previous fix unconditionally short-circuited on local-DB hit which preserved folder-first behavior but also limited what the migration importer could see. Cleaner separation: add a `walkAllRemotes` input flag that gates the new discovery semantics (multi-remote walk + expected-URL hint + stale-local-link surfacing). Default false → folder-first sees the long-standing behavior unchanged. Importer passes true and gets the picker-eligible candidate set. No new behavior for any pre-existing caller.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/host-service/src/trpc/router/project/project.ts (1)
256-266:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly mark stale local links on confirmed NOT_FOUND
Line 264 currently treats every
ctx.api.v2Project.get.queryfailure as deletion. Transient cloud/read errors will wrongly hide a valid linked project (staleLocalLink = true) and can surface as “Available” instead of an error. Restrict stale demotion to confirmed NOT_FOUND, and push other failures intocloudErrorswhile keeping the local candidate visible.Suggested fix
if (localProject) { const candidate = byId.get(localProject.id); if (candidate && candidate.source === "local-path") { try { await ctx.api.v2Project.get.query({ organizationId: ctx.organizationId, id: localProject.id, }); - } catch { - candidate.staleLocalLink = true; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + const isNotFound = + typeof err === "object" && + err !== null && + ( + // common TRPC/client error shapes + (err as { data?: { code?: string } }).data?.code === "NOT_FOUND" || + (err as { code?: string }).code === "NOT_FOUND" || + (err as { data?: { httpStatus?: number } }).data?.httpStatus === 404 + ); + + if (isNotFound) { + candidate.staleLocalLink = true; + } else { + cloudErrors.push({ + url: candidate.repoCloneUrl ?? "(project.get)", + message, + }); + } } } }#!/bin/bash # Verify how this repo currently discriminates NOT_FOUND for TRPC/cloud calls. rg -nP --type=ts -C3 'NOT_FOUND|httpStatus\s*===\s*404|data\?\.code' packages/host-service/src packages/host-service/test🤖 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/project/project.ts` around lines 256 - 266, The current catch treats any failure from ctx.api.v2Project.get.query as deletion; change it to inspect the thrown error and only set candidate.staleLocalLink = true when the error is a confirmed NOT_FOUND (e.g., error.data?.code === 'NOT_FOUND' or httpStatus === 404). For all other errors, do not demote the candidate: instead push the error/details into cloudErrors (or a similar aggregator) so the local candidate remains visible and cloud failures are reported. Update the try/catch around ctx.api.v2Project.get.query in the block referencing localProject, byId, and candidate to implement this conditional handling.
🤖 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 50-51: The isLoading flag currently uses "!projectsQuery.data"
which stays true when projectsQuery errors (data undefined) and causes a
perpetual loading state; update the isLoading computation in ImportProjectsPage
to remove the "!projectsQuery.data" check and instead compute loading from
projectsQuery.isPending || auditQuery.isPending (or explicitly check
projectsQuery.isError to render an error/retry path). Also add an error branch
(or surface) in ImportPageShell to display projectsQuery.error and/or
auditQuery.error with a Retry action (mirroring the per-row findByPathQuery
retry flow) so users can recover from v1 read failures.
- Around line 136-149: expectedRemoteUrlFor currently uses
project.mainRepoPath.split("/") to derive the repo name which fails on Windows
backslash paths; replace that logic to call the existing getBaseName utility
(imported from the renderer exports) to extract the repository base name so it
correctly handles POSIX, Windows and UNC/mixed separators, and keep the existing
guard (if no repoName return undefined) and the final URL construction using the
result.
---
Duplicate comments:
In `@packages/host-service/src/trpc/router/project/project.ts`:
- Around line 256-266: The current catch treats any failure from
ctx.api.v2Project.get.query as deletion; change it to inspect the thrown error
and only set candidate.staleLocalLink = true when the error is a confirmed
NOT_FOUND (e.g., error.data?.code === 'NOT_FOUND' or httpStatus === 404). For
all other errors, do not demote the candidate: instead push the error/details
into cloudErrors (or a similar aggregator) so the local candidate remains
visible and cloud failures are reported. Update the try/catch around
ctx.api.v2Project.get.query in the block referencing localProject, byId, and
candidate to implement this conditional handling.
🪄 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: 23c44734-6bd9-44ba-866e-d79bf901c2bd
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsxpackages/host-service/src/trpc/router/project/project.ts
Bug fixes from review:
- findByPath staleness: only mark `staleLocalLink` on a confirmed
NOT_FOUND. Transient cloud errors (network, auth, 5xx) used to drop
the legitimate local candidate; now they leave it intact and surface
via `cloudErrors` instead.
- findByPath redundant round-trip: when the cloud-URL loop already
saw the local id, skip the per-id `v2Project.get` staleness check
(new internal `cloudConfirmed` flag, stripped from wire response).
- ImportProjectsPage `linkToProjectId` fall-through: if the picker's
candidate has gone stale between render and click, throw a clear
error instead of silently calling `project.create` and duplicating
the v2 project.
- V1ImportModal: presets page used to render alongside the "host
service not ready" fallback when activeHostUrl was missing — gated
the fallback to projects/workspaces only since presets don't need
the host service.
Cheap wins:
- V1ImportBanner: drop the redundant `useEffect` that re-read
sessionStorage after mount (the `useState` initializer already does
it synchronously).
- ImportProjectsPage `expectedRemoteUrlFor`: split on `[\\/]` so a
Windows-style v1 path doesn't silently produce undefined.
- ImportPresetsPage: reuse `audit?.v2Id` on retry so a failed audit
upsert after a successful collection insert doesn't leave an
orphan v2 preset on next click.
- Replace silent `.catch(() => {})` on audit-error writes with a
warn that names the failing entity, so a real audit-write failure
is greppable instead of invisible.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportPresetsPage/ImportPresetsPage.tsx (1)
112-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the reserved
v2Idon the error path, or retries can still duplicate the preset.Line 116 reuses
audit?.v2Id, but the catch path clears it back tonull. If the local insert succeeds and the success audit write fails, the next retry generates a fresh UUID and inserts a second preset row. Preserve the samev2Idin the error audit entry, or reserve the id before inserting, so retries stay idempotent.🩹 Minimal fix
- await upsertState + await upsertState .mutateAsync({ v1Id: preset.id, kind: "preset", - v2Id: null, + v2Id, organizationId, status: "error", reason: message, })Also applies to: 132-155
🤖 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/ImportPresetsPage/ImportPresetsPage.tsx` around lines 112 - 117, The code currently generates v2Id with const v2Id = audit?.v2Id ?? crypto.randomUUID() but then clears it to null on the error/audit-failure path which allows retries to create a new UUID and duplicate the preset; modify the logic so the generated or existing v2Id is preserved for the error audit entry (or pre-reserve it before the local insert) — ensure the same v2Id variable used to construct the V2TerminalPresetRow is also written into the audit upsert on both success and in the catch path (reference v2Id, V2TerminalPresetRow, and the audit upsert/error handling code) so retries remain idempotent.apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx (2)
33-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently disable project ghost detection when
project.cloudListfails.If this query errors,
liveProjectIdsstaysnullforever, soauditGhostnever flips true and deleted v2 projects keep rendering asImported/Linked. This needs a retryable error path, or thenullfallback should apply only while the query is still pending.Also applies to: 174-181
🤖 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 33 - 48, The cloudProjectsQuery result is treated as unknown forever when it errors because liveProjectIds returns null whenever cloudProjectsQuery.data is falsy; change liveProjectIds to return null only while the query is still loading (e.g., cloudProjectsQuery.status === 'loading' or cloudProjectsQuery.isLoading) and return an empty Set when the query errored (cloudProjectsQuery.isError) so auditGhost can flip and deleted projects are detected; update the logic around the liveProjectIds use (and the similar block at 174-181) to rely on query status rather than data presence so errors become a retryable/handled path instead of silently disabling ghost detection (referencing cloudProjectsQuery, liveProjectIds, auditGhost, and project.cloudList).
50-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
!projectsQuery.datacan pin the page in a permanent loading state.On a
readV1Projectsfailure,isPendingbecomes false butdatastaysundefined, soisLoadingnever clears and the page has no recovery path. Compute loading from query status, then render a retryable error for the failed query instead of keying offdatapresence.🤖 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 50 - 51, The current isLoading computation in ImportProjectsPage uses "!projectsQuery.data" which can leave the UI stuck loading when readV1Projects fails; change isLoading to derive solely from query status flags (e.g., projectsQuery.isPending || auditQuery.isPending or projectsQuery.status !== 'success') and remove the "!projectsQuery.data" check, and add explicit error handling/UI for projectsQuery (render a retryable error state that calls projectsQuery.retry or re-invokes the fetch) so failures recover instead of permanently showing the loader; update references in ImportProjectsPage to use the new loading logic and display the error UI when projectsQuery.isError (or status === 'error').apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx (1)
43-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSurface
workspace.cloudListfailures instead of treating them like “still loading”.When this query errors,
liveWorkspaceIdsstaysnull, so the audit fallback continues indefinitely. That disables ghost demotion and can leave the page showing staleImportedrows — or even “No v1 workspaces left to import” — without any indication that the cloud cross-check failed. Only use thenullfallback while the query is pending, and surface a retryable error once it has failed.Also applies to: 156-163, 197-203
🤖 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 43 - 55, The liveWorkspaceIds memo currently treats missing data as "still loading" and returns null even when cloudWorkspacesQuery failed; update the useMemo (and the same logic at the other occurrences) to only return null while cloudWorkspacesQuery.isLoading (or status === 'loading'), return new Set(...) when cloudWorkspacesQuery.data is present, and if cloudWorkspacesQuery.isError (or status === 'error') surface the failure by rethrowing cloudWorkspacesQuery.error or returning a distinct error state so the UI can show a retryable error instead of silently falling back; adjust any consumers of liveWorkspaceIds accordingly (references: cloudWorkspacesQuery, liveWorkspaceIds, useQuery).
🤖 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 11-23: The dismissal flag is currently global; update
V1ImportBanner to scope it by organizationId by replacing DISMISS_SESSION_KEY
with a computed key (e.g., `${DISMISS_SESSION_KEY}:${organizationId}`) when
reading/writing sessionStorage in the useState initializer and wherever
setDismissed persists the value, and add an effect that re-reads the
organization-scoped key and calls setDismissed whenever organizationId changes
so the local dismissed state stays in sync across org switches (use the existing
DISMISS_SESSION_KEY, V1ImportBanner, dismissed, setDismissed, and organizationId
symbols to locate changes).
---
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportPresetsPage/ImportPresetsPage.tsx`:
- Around line 112-117: The code currently generates v2Id with const v2Id =
audit?.v2Id ?? crypto.randomUUID() but then clears it to null on the
error/audit-failure path which allows retries to create a new UUID and duplicate
the preset; modify the logic so the generated or existing v2Id is preserved for
the error audit entry (or pre-reserve it before the local insert) — ensure the
same v2Id variable used to construct the V2TerminalPresetRow is also written
into the audit upsert on both success and in the catch path (reference v2Id,
V2TerminalPresetRow, and the audit upsert/error handling code) so retries remain
idempotent.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx`:
- Around line 33-48: The cloudProjectsQuery result is treated as unknown forever
when it errors because liveProjectIds returns null whenever
cloudProjectsQuery.data is falsy; change liveProjectIds to return null only
while the query is still loading (e.g., cloudProjectsQuery.status === 'loading'
or cloudProjectsQuery.isLoading) and return an empty Set when the query errored
(cloudProjectsQuery.isError) so auditGhost can flip and deleted projects are
detected; update the logic around the liveProjectIds use (and the similar block
at 174-181) to rely on query status rather than data presence so errors become a
retryable/handled path instead of silently disabling ghost detection
(referencing cloudProjectsQuery, liveProjectIds, auditGhost, and
project.cloudList).
- Around line 50-51: The current isLoading computation in ImportProjectsPage
uses "!projectsQuery.data" which can leave the UI stuck loading when
readV1Projects fails; change isLoading to derive solely from query status flags
(e.g., projectsQuery.isPending || auditQuery.isPending or projectsQuery.status
!== 'success') and remove the "!projectsQuery.data" check, and add explicit
error handling/UI for projectsQuery (render a retryable error state that calls
projectsQuery.retry or re-invokes the fetch) so failures recover instead of
permanently showing the loader; update references in ImportProjectsPage to use
the new loading logic and display the error UI when projectsQuery.isError (or
status === 'error').
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx`:
- Around line 43-55: The liveWorkspaceIds memo currently treats missing data as
"still loading" and returns null even when cloudWorkspacesQuery failed; update
the useMemo (and the same logic at the other occurrences) to only return null
while cloudWorkspacesQuery.isLoading (or status === 'loading'), return new
Set(...) when cloudWorkspacesQuery.data is present, and if
cloudWorkspacesQuery.isError (or status === 'error') surface the failure by
rethrowing cloudWorkspacesQuery.error or returning a distinct error state so the
UI can show a retryable error instead of silently falling back; adjust any
consumers of liveWorkspaceIds accordingly (references: cloudWorkspacesQuery,
liveWorkspaceIds, useQuery).
🪄 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: fe184505-9ff7-4001-a895-00a6a4246132
📒 Files selected for processing (6)
apps/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.ts
- ImportProjectsPage: drop `!projectsQuery.data` from isLoading. If readV1Projects errors, isPending flips false but data stays undefined — without this fix the page got stuck in a permanent loading spinner with no path to recovery. Falling through to the empty-state message is a cleaner dead-end. - ImportProjectsPage: replace ad-hoc `[\\/]` regex with the existing `getBaseName` util from `renderer/lib/pathBasename` — same POSIX/Windows/UNC handling, less code, already tested. - V1ImportBanner: scope the session-storage dismiss flag to organizationId. Dismissing in one org no longer hides the banner in every other org for the rest of the session. Re-sync local state on org change so flipping orgs reveals the banner again if it hasn't been dismissed there yet.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx (2)
127-138:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBrittle error message parsing still couples renderer to server wording (unresolved from previous review).
isAlreadySetUpElsewhereError(line 129) andextractExistingPath(lines 134–137) both parse the server's free-form message string. Any edit to the error text on the host-service side (punctuation, path quoting, sentence restructuring) will silently break the relocate-confirm flow:pendingRelocatewon't be set and the user sees a generic error with no recovery path.tRPC's
TRPCClientErrorexposes a typeddatafield (inferErrorShape) that can carry a structured payload when the router's error formatter opts it in. The codebase already uses thecausepattern inworkspace-cleanup.ts. Switching the server'sCONFLICTthrow to include a discriminatedcause: { kind: "ALREADY_SET_UP_ELSEWHERE", existingPath: string }and reading that on the client side would make this contract explicit and refactor-safe.🤖 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 127 - 138, The renderer should stop brittle parsing of server message strings in isAlreadySetUpElsewhereError and extractExistingPath and instead read a structured discriminant from the tRPC error payload; update isAlreadySetUpElsewhereError to detect TRPCClientError-like errors (e.g., check err.data?.cause?.kind === "ALREADY_SET_UP_ELSEWHERE") and update extractExistingPath to return err.data.cause.existingPath (or null) when that structured payload exists; keep the string-fallback only as a last resort, but prefer the typed data.cause fields so the relocate-confirm flow reliably sets pendingRelocate based on err.data.cause.existingPath.
34-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGhost detection is silently disabled on cloud query failure (unresolved from previous review).
When
cloudProjectsQueryerrors,liveProjectIdspermanently returnsnull(line 47:if (!cloudProjectsQuery.data) return null), which permanently short-circuits theauditGhostcondition at line 181 (liveProjectIds !== null). Rows with a stale "success"/"linked" audit entry for a cloud-deleted project will display as Imported indefinitely — the exact failure mode thecloudListcross-check was introduced to fix.Consider surfacing
cloudProjectsQuery.isErrorat the page level (analogous to the per-rowcloudErrorsblock at lines 388–398), or allowliveProjectIdsto reflect the error state so callers can distinguish "still loading" from "failed to load".🤖 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 34 - 49, The cloud-projects failure path currently returns null from liveProjectIds which conflates "loading" and "errored" and permanently disables auditGhost; change the liveProjectIds useMemo (and any callers that rely on liveProjectIds !== null, e.g., auditGhost logic) so it distinguishes three states: loading, success, and error. Specifically, in the useMemo for liveProjectIds check cloudProjectsQuery.isLoading and return null only while loading; when cloudProjectsQuery.data exists return the Set as today; when cloudProjectsQuery.isError return a deterministic non-null sentinel (e.g., an empty Set or a distinct symbol) or surface cloudProjectsQuery.isError to the page and have auditGhost consult that flag so rows can still perform ghost-detection when the cloud query failed. Ensure references to cloudProjectsQuery, liveProjectIds, and auditGhost are updated accordingly.
🤖 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.
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx`:
- Around line 127-138: The renderer should stop brittle parsing of server
message strings in isAlreadySetUpElsewhereError and extractExistingPath and
instead read a structured discriminant from the tRPC error payload; update
isAlreadySetUpElsewhereError to detect TRPCClientError-like errors (e.g., check
err.data?.cause?.kind === "ALREADY_SET_UP_ELSEWHERE") and update
extractExistingPath to return err.data.cause.existingPath (or null) when that
structured payload exists; keep the string-fallback only as a last resort, but
prefer the typed data.cause fields so the relocate-confirm flow reliably sets
pendingRelocate based on err.data.cause.existingPath.
- Around line 34-49: The cloud-projects failure path currently returns null from
liveProjectIds which conflates "loading" and "errored" and permanently disables
auditGhost; change the liveProjectIds useMemo (and any callers that rely on
liveProjectIds !== null, e.g., auditGhost logic) so it distinguishes three
states: loading, success, and error. Specifically, in the useMemo for
liveProjectIds check cloudProjectsQuery.isLoading and return null only while
loading; when cloudProjectsQuery.data exists return the Set as today; when
cloudProjectsQuery.isError return a deterministic non-null sentinel (e.g., an
empty Set or a distinct symbol) or surface cloudProjectsQuery.isError to the
page and have auditGhost consult that flag so rows can still perform
ghost-detection when the cloud query failed. Ensure references to
cloudProjectsQuery, liveProjectIds, and auditGhost are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8236df29-aa1d-4e6a-b772-f0f38adfad05
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/V1ImportBanner/V1ImportBanner.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx
10 tasks
6 tasks
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 8, 2026
…perset-sh#4122) * refactor(desktop): rewrite v1→v2 migration as pull-based importer Replaces the auto-running batch migration with a manual click-to-import flow across three pages (Projects / Workspaces / Presets) inside the welcome modal. v1MigrationState stays as an audit log; per-row status is derived live from host-service findByPath and the audit log so a re-run is naturally idempotent and cross-device dedup falls out for free. Removes ~3.4k LOC of batch orchestration: the auto-run hook, the heuristic findByPath fallbacks, the singleton concurrency guard, the localStorage event bus, the bulk sidebar writeback, and the separate preset auto-migration. * refactor(desktop): harden v1→v2 importer for forks, ghosts, and relocates Three real bugs the auto-batch v1→v2 migration silently mishandled, surfaced concretely while triaging a coworker's mangled import: - findByPath only ever read the local repo's `origin` (or first GitHub remote). Cloned-fork-as-origin contributors got linked to the wrong v2 project. Now reads every GitHub remote, accepts an `expectedRemoteUrl` hint from v1's recorded `githubOwner`, and tags matching candidates so the picker can recommend them. - Audit success was trusted blind. If another device or user deleted the v2 project, the row stayed flagged "Imported" forever pointing at a ghost. Adds `project.cloudList` + `workspace.cloudList` procedures; pages cross-check audit v2 ids against live cloud and demote ghosts back to "available". - Linking a v1 project whose folder differs from where v2 already had the project set up on this device threw a confusing CONFLICT. Now surfaces a confirm row ("Already set up at X. Link to Y instead?") with explicit Use-this-folder vs Cancel. Plus: surface findByPath cloud query failures (silent `console.warn` before, misleadingly fell through to "Import"); refresh button per page; stale-worktree NOT_FOUND retry-by-branch on adopt; listProjectWorktrees procedure for filtering v1 workspaces against the project's current `git worktree list` so guaranteed-to-fail rows don't surface; invalidate the cloud-list query after every successful import so the audit-ghost detector doesn't false-flag fresh imports. * fix(host-service): restore local-DB short-circuit in findByPath Hardening commit removed the short-circuit and started always walking all remotes + merging cloud results. That changed folder-first import behavior: if your org happens to have multiple v2 projects pointing at the same remote URL, re-importing a folder you'd already linked locally now triggers the multi-project picker instead of silently linking to the local row. /settings/projects has no setup-at-path affordance, so the multi-project toast was a dead end. Restore the short-circuit, gated by a cloud existence check on the local v2 id. Non-stale local hit → return that candidate only. Stale local hit (cloud project deleted) → fall through to the multi-remote walk so callers see real alternatives. Migration importer is unaffected: v1 paths typically have no local-DB hit, so it still takes the cloud-walk path and gets the picker for forks etc. * refactor(host-service): gate findByPath multi-remote walk behind opt-in flag Previous fix unconditionally short-circuited on local-DB hit which preserved folder-first behavior but also limited what the migration importer could see. Cleaner separation: add a `walkAllRemotes` input flag that gates the new discovery semantics (multi-remote walk + expected-URL hint + stale-local-link surfacing). Default false → folder-first sees the long-standing behavior unchanged. Importer passes true and gets the picker-eligible candidate set. No new behavior for any pre-existing caller. * fix(desktop): address PR review nits on v1→v2 importer Bug fixes from review: - findByPath staleness: only mark `staleLocalLink` on a confirmed NOT_FOUND. Transient cloud errors (network, auth, 5xx) used to drop the legitimate local candidate; now they leave it intact and surface via `cloudErrors` instead. - findByPath redundant round-trip: when the cloud-URL loop already saw the local id, skip the per-id `v2Project.get` staleness check (new internal `cloudConfirmed` flag, stripped from wire response). - ImportProjectsPage `linkToProjectId` fall-through: if the picker's candidate has gone stale between render and click, throw a clear error instead of silently calling `project.create` and duplicating the v2 project. - V1ImportModal: presets page used to render alongside the "host service not ready" fallback when activeHostUrl was missing — gated the fallback to projects/workspaces only since presets don't need the host service. Cheap wins: - V1ImportBanner: drop the redundant `useEffect` that re-read sessionStorage after mount (the `useState` initializer already does it synchronously). - ImportProjectsPage `expectedRemoteUrlFor`: split on `[\\/]` so a Windows-style v1 path doesn't silently produce undefined. - ImportPresetsPage: reuse `audit?.v2Id` on retry so a failed audit upsert after a successful collection insert doesn't leave an orphan v2 preset on next click. - Replace silent `.catch(() => {})` on audit-error writes with a warn that names the failing entity, so a real audit-write failure is greppable instead of invisible. * fix(desktop): more PR review nits on v1→v2 importer - ImportProjectsPage: drop `!projectsQuery.data` from isLoading. If readV1Projects errors, isPending flips false but data stays undefined — without this fix the page got stuck in a permanent loading spinner with no path to recovery. Falling through to the empty-state message is a cleaner dead-end. - ImportProjectsPage: replace ad-hoc `[\\/]` regex with the existing `getBaseName` util from `renderer/lib/pathBasename` — same POSIX/Windows/UNC handling, less code, already tested. - V1ImportBanner: scope the session-storage dismiss flag to organizationId. Dismissing in one org no longer hides the banner in every other org for the rest of the session. Re-sync local state on org change so flipping orgs reveals the banner again if it hasn't been dismissed there yet.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 8, 2026
The v1→v2 importer cherry-picks (superset-sh#4122 et al.) introduced two missing dependencies that need backporting: - packages/host-service/.../shared/worktree-list.ts: required by the new list-project-worktrees procedure. Originally added in upstream superset-sh#4074 (not in this batch). Brought in verbatim from 8ae0b3e. - packages/trpc/.../v2-workspace.ts: workspace.cloudList calls ctx.api.v2Workspace.list which lands in upstream superset-sh#3889. Backported just the list procedure here so the cherry-picked importer has the shape it expects.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 8, 2026
Auto-merge of superset-sh#4122 brought useV2AgentConfigs + buildAgentLaunchCommand imports for the agent-aware preset launch flow, but the fork's V2PresetsSection doesn't use that flow yet (depends on agents/PR3). Lint flagged them as unused; remove until the agents PR lands.
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
Replaces the auto-running v1→v2 batch migration with a per-item, click-to-import flow inside the existing welcome modal. Drops ~3.4k LOC of brittle batch orchestration (singleton concurrency guards, sessionStorage gates, heuristic
findByPathfallbacks, bulk sidebar writeback, the separate preset auto-migration) and replaces it with three import pages: Projects, Workspaces, Presets. Each row carries its own state — Available / Linked / Imported / Error — and re-clicking is naturally idempotent because cloud is the source of truth.While testing against a teammate's mangled host.db (linked to the wrong v2 project, then cloud-deleted) the original design surfaced three concrete failure modes worth handling explicitly. The second commit hardens for those:
findByPathused to only readorigin(or the first GitHub remote). Contributors who cloned a fork as origin got silently linked to the wrong v2 project. Now reads every GitHub remote, optionally accepts anexpectedRemoteUrlhint constructed from v1's recordedgithubOwner, and tags matches so the picker can recommend the right one.success, but another device / user deleted the v2 project since. Newproject.cloudListandworkspace.cloudListprocedures cross-check audit v2 ids against live cloud — ghosts demote back to "available" instead of permanently displaying a green "Imported" badge that points at nothing.<existing>. Link to<new>instead?" with explicit Use this folder / Cancel. Confirming retriesproject.setupwithallowRelocate: true.Test plan
origin(fork) andupstream(real). Picker shows both candidates, the one matching v1'sgithubOwnerflagged "matches v1" and sorted first.findByPathreturns 0 candidates →Import→ creates v2 project withrepoCloneUrl: null.git worktree list→ adopt with the bad path returns NOT_FOUND, code retries withoutworktreePath→ server resolves by branch.bun run lintandbun run typecheckclean.Summary by cubic
Replaces the auto-running v1→v2 migration with a simple, pull-based importer inside a new modal. This is safer, easy to retry, and removes ~3.4k LOC of brittle batch code.
New Features
V1ImportModalwith Projects, Workspaces, and Presets pages; open from a dashboard banner or Settings.project.cloudListandworkspace.cloudListto avoid stale “Imported” ghosts.workspaceCreation.listProjectWorktreesso orphaned v1 workspaces don’t show up.Bug Fixes
findByPath: opt-inwalkAllRemotesfor importer, restored local-DB short-circuit for folder-first flows; only mark staleness on confirmed NOT_FOUND; surface transient cloud errors as row errors; skip redundant per-id checks when cloud already confirmed the id.allowRelocate: true.readV1Projectserrors; falls back to empty-state.getBaseName, reuseaudit.v2Idon retry, replace silent catches with warns.Written for commit d2f176a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Changes