Repository navigation
fix(desktop): drop onboarding auto-skip, align SetupButton with design system - #4125
Conversation
… system The auto-advance logic in onboarding steps was rerouting users away from pages they explicitly navigated to (e.g. clicking "Configure Codex" dropped them on /setup/gh-cli because Claude was already connected). The gate intended to suppress this — `manualWalkthrough` — was never set to true anywhere, so the auto-skip fired unconditionally. Removes: - `shouldAutoAdvance` effects in providers, gh-cli, and project steps - The no-projects / no-worktrees auto-finish in adopt-worktrees - The dead `manualWalkthrough` flag and its action Per-step "Skip for now" buttons are unchanged. Also rewrites SetupButton as a thin wrapper around @superset/ui/button, mapping primary/secondary/link to the design system's variants and the xs size, so onboarding CTAs share theming, focus ring, and hover behavior with the rest of the app.
📝 WalkthroughWalkthroughRemoved the ChangesOnboarding flow: removal of manualWalkthrough and auto-advance
Adopt-worktrees & V2 import flow
SetupButton component refactor
V2 Available Banner
Sequence Diagram(s)sequenceDiagram
participant UI as AdoptWorktrees UI
participant Host as Local Host Service
participant API as V2 Project API
participant Sidebar as Dashboard Sidebar
UI->>Host: ensure active host (hostReady?) / get machineId
Note right of Host: host connection check
UI->>API: call ensureV2Project(repoPath,name)
API-->>UI: return { hostUrl, projectId, repoPath, mainWorkspaceId }
UI->>API: finalizeProjectSetup(projectId, importMode)
API-->>UI: returns import job / success
UI->>Host: submit worktree import to host
Host-->>UI: import progress events
UI->>Sidebar: refresh / add new project entry on success
UI-->>UI: update progress state / show completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryRemoves the broken auto-advance logic from all onboarding step pages —
Confidence Score: 5/5Safe to merge — the changes are focused deletions of broken logic with no new behaviors introduced. The auto-advance guard was confirmed never set to true, so removing the entire mechanism is a correct and complete fix. The adopt-worktrees empty-projects path is handled cleanly via projects ?? [] and vacuous Array.every. The SetupButton rewrite is backed by a compile-time satisfies check against the design system variants. The persisted store shape change requires no migration. No files require special attention; the adopt-worktrees page is the most changed and is covered by the test plan.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/setup/adopt-worktrees/page.tsx | Removes AutoAdvance component and the projects-empty early return; passes projects ?? [] so AdoptWorktreesContent handles the empty case via nothingToAdopt state. |
| apps/desktop/src/renderer/routes/_authenticated/setup/components/SetupButton/SetupButton.tsx | Clean wrapper around @superset/ui/button; VARIANT_MAP uses satisfies for compile-time type safety; size="xs" and conditional w-full preserve the existing layout contract. |
| apps/desktop/src/renderer/stores/onboarding/onboardingStore.ts | Drops manualWalkthrough field and setManualWalkthrough action; persisted store name is unchanged so existing serialized state is harmlessly ignored on hydration. |
| apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx | Removes shouldAutoAdvance logic and the wasConfiguredOnMount ref; useRef import correctly dropped. |
| apps/desktop/src/renderer/routes/_authenticated/setup/gh-cli/page.tsx | Removes shouldAutoAdvance and the gh-cli completed/skipped/manualWalkthrough selectors; loading guard simplified to isPending only. |
| apps/desktop/src/renderer/routes/_authenticated/setup/project/page.tsx | Removes shouldAutoAdvance and all manualWalkthrough references; setManualWalkthrough(false) call on project creation removed. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Navigate to setup/providers] --> B{isPending?}
B -- yes --> C[Spinner]
B -- no --> D[Render providers UI]
D --> E[User clicks Continue or Skip]
E --> F[setup/gh-cli]
F --> G{isPending?}
G -- yes --> H[Spinner]
G -- no --> I[Render gh-cli UI]
I --> J[User clicks Continue or Skip]
J --> K[setup/project]
K --> L{isPending?}
L -- yes --> M[Spinner]
L -- no --> N{hasProjects?}
N -- yes --> O[Show project list]
N -- no --> P[Show Add project UI]
O --> Q[setup/adopt-worktrees]
P --> Q
Q --> R{isPending?}
R -- yes --> S[Spinner]
R -- no --> T[AdoptWorktreesContent with projects or empty array]
T --> U{nothingToAdopt?}
U -- yes --> V[Continue or Skip for now]
U -- no --> W[Worktree checklist]
V --> X[navigateAfterFlow]
W --> X
Reviews (1): Last reviewed commit: "fix(desktop): drop onboarding auto-skip,..." | Re-trigger Greptile
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/setup/adopt-worktrees/page.tsx (1)
39-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
getRecentserrors explicitly instead of converting them to an empty project list.At Line 120,
(projects ?? [])masks query failure as “no projects,” which can incorrectly drive the “nothing to adopt” path and misleading copy.💡 Suggested fix
- const { data: projects, isPending } = + const { data: projects, isPending, isError, error, refetch } = electronTrpc.projects.getRecents.useQuery(); if (isPending) { return ( <div className="flex h-full w-full items-center justify-center bg-[`#151110`]"> <Spinner className="size-6 text-[`#a8a5a3`]" /> </div> ); } + + if (isError) { + return ( + <StepShell backTo={STEP_ROUTES.project} maxWidth="lg"> + <StepHeader + title="Adopt existing worktrees" + subtitle="We couldn't load your projects." + /> + <div className="rounded-lg border border-red-500/30 bg-red-500/5 p-3 text-[12px] text-red-400 select-text cursor-text"> + {error instanceof Error ? error.message : "Failed to load projects"} + </div> + <div className="flex w-[273px] self-center"> + <SetupButton onClick={() => void refetch()}>Retry</SetupButton> + </div> + </StepShell> + ); + }Also applies to: 110-124
🤖 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/setup/adopt-worktrees/page.tsx` around lines 39 - 40, The current use of (projects ?? []) after calling electronTrpc.projects.getRecents.useQuery masks query failures as an empty list; update the logic around electronTrpc.projects.getRecents.useQuery (the projects and isPending values) to explicitly handle error states from the query (e.g., check for an error or use the query's error field) and only treat projects as an empty array when there is no error and data is genuinely missing; adjust the rendering paths that currently branch on (projects ?? []) so they show an explicit error message/UI when the query errored instead of the “nothing to adopt” copy.
🤖 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.
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/setup/adopt-worktrees/page.tsx`:
- Around line 39-40: The current use of (projects ?? []) after calling
electronTrpc.projects.getRecents.useQuery masks query failures as an empty list;
update the logic around electronTrpc.projects.getRecents.useQuery (the projects
and isPending values) to explicitly handle error states from the query (e.g.,
check for an error or use the query's error field) and only treat projects as an
empty array when there is no error and data is genuinely missing; adjust the
rendering paths that currently branch on (projects ?? []) so they show an
explicit error message/UI when the query errored instead of the “nothing to
adopt” copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2e04b45-09af-47b4-9d9e-c24157838e10
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/setup/adopt-worktrees/page.tsxapps/desktop/src/renderer/routes/_authenticated/setup/components/SetupButton/SetupButton.tsxapps/desktop/src/renderer/routes/_authenticated/setup/gh-cli/page.tsxapps/desktop/src/renderer/routes/_authenticated/setup/project/page.tsxapps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsxapps/desktop/src/renderer/stores/onboarding/onboardingStore.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/stores/onboarding/onboardingStore.ts
The onboarding "Select new repo" button always called the v1 projects.openNew IPC mutation, which writes a v1 project row in the local Electron SQLite. When v2 is enabled, navigating to /project/$projectId then loads a v2 project page that has no matching cloud row, leaving the user with a stranded v1 project. Routes the v2 case through useFolderFirstImport — the same hook the v2 dashboard's "Import folder" uses — so it goes through client.project.create on the host service and ends up as a real v2 project + main workspace. Falls back to the existing v1 path otherwise. The /new-project clone flow is still v1-only and is reached from many places outside onboarding; left as a follow-up.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
v1 is deprecated; the onboarding flow shouldn't read from or write to v1 anymore. Drops the isV2CloudEnabled gate and all v1 paths from the project setup step: - Project list now reads collections.v2Projects directly via useLiveQuery, scoped to the active org. Replaces the v1 electronTrpc.projects.getRecents query. - "Select new repo" always goes through useFolderFirstImport (client.project.create on the host service). No more v1 fallback. - "Clone from GitHub" is removed because /new-project is still v1. Folder-first import is the only supported entry until a v2 clone surface lands. - Per-row delete is removed. v2 client.project.remove deletes from the org and is too heavy for an onboarding affordance; users can manage projects from settings/projects. The next adopt-worktrees step is still v1-only (no v2 equivalent for external worktree adoption exists). Left as a follow-up.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/setup/project/page.tsx`:
- Around line 90-109: Summary: The folder import handler can be re-triggered
while folderImport.start() is in-flight; add a local loading state to prevent
duplicate clicks. Add a local boolean state (e.g., isImporting) in the component
that contains handleSelectNewRepo; set isImporting = true before awaiting any
async branch (both the isV2CloudEnabled -> await folderImport.start() path and
the openNew() path) and reset it in a finally block to ensure it is cleared on
success/failure. Update handleSelectNewRepo (and any other callers) to
early-return if isImporting is true. Wire this state into the UI by disabling
the button(s) that invoke handleSelectNewRepo and showing a loading label (e.g.,
"Importing…" or spinner) while isImporting is true so users cannot re-click;
keep existing calls to markComplete, navigate, and openV1ProjectInWorkspace
unchanged.
🪄 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: fd91549c-bdc3-4ba6-b0b0-098f8e9eee6e
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/setup/project/page.tsx
handleSelectNewRepo navigated to /project/$projectId immediately after the folder-first import resolved, kicking the user out of the onboarding flow. The v2 project page also rendered before Electric had synced the new v2_projects row into collections.v2Projects, so it looked like nothing was added. Stay on /setup/project after import. The live query against collections.v2Projects updates once Electric streams the new row in, the project shows up in the on-page list, and the user clicks "Continue with current" to advance.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/setup/project/page.tsx (1)
60-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent duplicate folder-import starts.
handleSelectNewRepo()kicks off a folder picker plus asyncproject.setup/creatework, but neither button is disabled while that promise is in flight. A double click can reopen the picker or race two create/setup requests.🛡️ Proposed fix
-import { useEffect } from "react"; +import { useEffect, useState } from "react"; @@ function OnboardingProjectPage() { const navigate = useNavigate(); + const [isImporting, setIsImporting] = useState(false); const goTo = useOnboardingStore((s) => s.goTo); @@ const handleSelectNewRepo = async () => { - const result = await folderImport.start(); - if (result) { - markComplete("project"); + if (isImporting) return; + setIsImporting(true); + try { + const result = await folderImport.start(); + if (result) { + markComplete("project"); + } + } finally { + setIsImporting(false); } }; @@ - <SetupButton variant="secondary" onClick={handleSelectNewRepo}> - Select new repo + <SetupButton + variant="secondary" + onClick={handleSelectNewRepo} + disabled={isImporting} + > + {isImporting ? "Opening…" : "Select new repo"} </SetupButton> @@ - <SetupButton onClick={handleSelectNewRepo}>Select new repo</SetupButton> + <SetupButton onClick={handleSelectNewRepo} disabled={isImporting}> + {isImporting ? "Opening…" : "Select new repo"} + </SetupButton>Also applies to: 131-132, 151-151
🤖 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/setup/project/page.tsx` around lines 60 - 64, handleSelectNewRepo() can be invoked multiple times allowing duplicate folderImport.start() calls; guard against reentrancy by adding an "isImporting" (or similar) boolean state and early-return if true, set it true immediately before calling await folderImport.start(), and clear it in a finally block so markComplete("project") still runs when appropriate; also apply the same guard/state and disable the corresponding UI buttons that call the same handler (the other occurrences referenced at lines 131-132 and 151) to prevent double-clicks and racing create/setup requests.
🤖 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/setup/project/page.tsx`:
- Around line 60-64: handleSelectNewRepo() can be invoked multiple times
allowing duplicate folderImport.start() calls; guard against reentrancy by
adding an "isImporting" (or similar) boolean state and early-return if true, set
it true immediately before calling await folderImport.start(), and clear it in a
finally block so markComplete("project") still runs when appropriate; also apply
the same guard/state and disable the corresponding UI buttons that call the same
handler (the other occurrences referenced at lines 131-132 and 151) to prevent
double-clicks and racing create/setup requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3a4cfd3-93a1-4ee1-af70-7fe5edefc4c7
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/setup/project/page.tsx
- adopt-worktrees: drop v1 fallbacks (useIsV2CloudEnabled / projects
navigation / lastViewedWorkspaceId), drop the v1 importExternalWorktrees
path. Land users on /v2-workspaces and let the dashboard route handle
empty/last-workspace logic instead of duplicating it here.
- adopt-worktrees: per-project ensureV2Project + finalizeProjectSetup,
then submit each worktree as a v2 workspace via useWorkspaceCreates and
pin via ensureWorkspaceInSidebar. "Importing X of Y…" progress, stay on
page if every selected import fails, "Connecting…" gating until the
host service is up.
- setup/project: gate "Select new repo" / "Continue with current" buttons
on activeHostUrl with a "Connecting…" label so v2 actions can't fire
before the host returns a port.
- providers: focus one provider at a time during configure/reconfigure,
smart-default to the unconnected provider when one is connected, "Set
up X instead" switcher when neither is connected, hide Continue while
any step is active.
- V2AvailableBanner: only render for v1 users; action routes to
/settings/experimental ("Try new version of Superset") instead of
flipping setOptInV2 directly.
- New useEnsureV2Project hook: findByPath → setup, falling through to
project.create only on TRPC NOT_FOUND so transient errors don't mint
duplicate cloud projects.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx (1)
81-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSubtitle and Continue-button visibility disagree when only one provider is connected.
When exactly one provider is connected (e.g., Claude only), the subtitle reads "Add another provider or continue to the next step." (line 82), but
inAnyStepistrue(because!codexConnected), so the Continue button at line 222 is suppressed. The user has no way to "continue" without either also connecting Codex or using "Skip for now" (which marks the step as skipped, not completed).This may be intentional per the PR objectives ("hide Continue while any step is active"), but the user-visible copy implies otherwise. Either:
- relax the visibility to
atLeastOneConnected && !reconfiguringClaude && !reconfiguringCodex, or- update the subtitle so it matches the actual flow when one provider is connected.
Also applies to: 222-224
🤖 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/setup/providers/page.tsx` around lines 81 - 89, The subtitle and the Continue-button visibility disagree because subtitle suggests continuing when atLeastOneConnected is true while inAnyStep is true whenever the other provider is missing; fix by making the Continue-button visibility match the subtitle: change the condition that controls the Continue button (currently using inAnyStep) to require atLeastOneConnected && !reconfiguringClaude && !reconfiguringCodex (i.e., allow Continue when atLeastOneConnected and neither provider is in reconfiguration), or alternatively update the subtitle string to reflect the stricter behavior; locate the variables subtitle and inAnyStep and the Continue-button render that checks inAnyStep (lines near where Continue is rendered) and apply one of these two changes so the UI copy and button visibility are consistent.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/setup/adopt-worktrees/page.tsx (1)
209-228: 💤 Low valueProgress counter only advances on successful submits — partial-failure UX feels stuck.
setProgress({ current: totalImported, total: totalToImport })is updated usingtotalImported, which only increments onresult.ok. If submits fail, the "Importing X of N…" label never advances toward N even though work is happening. Consider tracking attempts separately so the progress meter reflects real progress while still surfacing failures via the existing toasts:♻️ Suggested change
- for (const path of paths) { + let attempted = totalImported; + for (const path of paths) { const wt = projectResult.worktrees.find((w) => w.path === path); if (!wt) continue; const result = await submit({ ... }); if (result.ok) { totalImported++; ensureWorkspaceInSidebar(result.workspaceId, v2ProjectId); } else { toast.error(`Failed to import ${wt.branch}: ${result.error}`); } - setProgress({ current: totalImported, total: totalToImport }); + attempted++; + setProgress({ current: attempted, total: totalToImport }); }🤖 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/setup/adopt-worktrees/page.tsx` around lines 209 - 228, The progress counter only increments on successful imports because setProgress uses totalImported; change the loop to track an attempts counter (e.g., attemptedCount initialized before the for loop) and increment it on every iteration (regardless of result.ok), then call setProgress({ current: attemptedCount, total: totalToImport }); keep totalImported incrementing only on success so toasts and ensureWorkspaceInSidebar(result.workspaceId, v2ProjectId) remain unchanged for success/failure handling.apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx (1)
222-224: 💤 Low valueRedundant guard:
atLeastOneConnected && !inAnyStep.
!inAnyStepalready implies bothclaudeConnectedandcodexConnected(sinceinAnyStepis true whenever either is unconnected), soatLeastOneConnectedis always true at this point. The check can be simplified to just!inAnyStepfor clarity.🤖 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/setup/providers/page.tsx` around lines 222 - 224, The JSX conditional currently uses a redundant guard "atLeastOneConnected && !inAnyStep" to render <SetupButton onClick={handleContinueToNextStep}>; simplify it by removing the unnecessary atLeastOneConnected check and render the button when "!inAnyStep" only (keep the same <SetupButton> and handleContinueToNextStep callback), since inAnyStep already captures the connected-state logic.apps/desktop/src/renderer/hooks/useEnsureV2Project/useEnsureV2Project.ts (1)
39-50: 💤 Low valueUse tRPC's
isTRPCClientErrortype guard for safer error-code detection.The cast
(err as { data?: { code?: string } } | null | undefined)?.data?.codeis vulnerable to false matches from unrelated errors with adata.codefield. Instead, useisTRPCClientError<AppRouter>(err)to properly narrow the error type before accessingerr.data?.code, ensuring unrelated errors (DOMException, fetch TypeErrors, etc.) won't accidentally trigger this safety valve that prevents duplicate cloud projects.Alternatively, the codebase already has a
trpcCode()helper inImportWorkspacesPage.tsxthat safely extracts error codes—consider extracting it to a shared utility.🤖 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/hooks/useEnsureV2Project/useEnsureV2Project.ts` around lines 39 - 50, Replace the ad-hoc cast when checking the error code in useEnsureV2Project with tRPC's type guard: call isTRPCClientError<AppRouter>(err) and only then read err.data?.code (or reuse the existing trpcCode helper extracted to a shared util); update the catch in useEnsureV2Project to guard with isTRPCClientError before comparing to "NOT_FOUND" so unrelated errors (DOMException, fetch errors, etc.) are not misclassified.
🤖 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/hooks/useEnsureV2Project/useEnsureV2Project.ts`:
- Around line 25-51: The hook useEnsureV2Project currently picks
found.candidates[0] from hostService.project.findByPath.query and proceeds,
which can silently link the wrong project; instead, mirror useFolderFirstImport:
after calling hostService.project.findByPath.query({ repoPath }) check if
found.candidates.length > 1 and abort by surfacing an error (throw a specific
Error or call the same onMultipleProjects/onError path used elsewhere) rather
than selecting candidates[0]; only when candidates.length === 1 proceed to call
hostService.project.setup.mutate with candidate.id, and preserve the existing
NOT_FOUND handling for setup errors.
In
`@apps/desktop/src/renderer/routes/_authenticated/setup/adopt-worktrees/page.tsx`:
- Line 123: The disabled predicate uses hostNotReady = !activeHostUrl but the
runtime guard in handleImportSelected checks for machineId from
useLocalHostService; update the predicate so the button is disabled when the
runtime guard would abort (e.g., set hostNotReady = !machineId or hostNotReady =
!activeHostUrl || !machineId) and similarly adjust any other checks referenced
(lines around handleImportSelected and the button render) so the UI disabled
state and the runtime guard (handleImportSelected) use the same condition;
ensure you reference useLocalHostService, activeHostUrl, machineId,
hostNotReady, and handleImportSelected when making the change.
In `@apps/desktop/src/renderer/routes/_authenticated/setup/project/page.tsx`:
- Around line 63-68: handleSelectNewRepo currently calls folderImport.start()
and markComplete("project") but gives no immediate feedback if the live query
that surfaces the new v2 project lags; update handleSelectNewRepo to surface
success to the user by either triggering a success toast (e.g., call the app's
toast/notification API with a clear message when folderImport.start() resolves)
or by using the success callback/signal from useFolderFirstImport to set local
UI state (e.g., set a “recentlyImported” flag) so the component immediately
renders the has-projects branch and the "Continue with current" CTA even before
the live query updates. Ensure you reference handleSelectNewRepo,
folderImport.start, markComplete, and useFolderFirstImport when making the
change.
- Around line 30-47: The query is silently returning no rows when
activeOrganizationId is null; update the logic around authClient.useSession and
useLiveQuery so you do not run the projects query with an empty string.
Specifically, detect when env.SKIP_ENV_VALIDATION is false and
session?.session?.activeOrganizationId is null (i.e., activeOrganizationId ===
null) and in that case skip or short-circuit useLiveQuery (don’t call q.where
with ""), and instead surface a loading/error state in the UI (e.g., keep the
spinner or show an auth setup error) until the session resolves; adjust the code
that builds the dependencies ([collections, activeOrganizationId]) and the
render branch that currently assumes an empty projects array so it reflects the
loading/error state when activeOrganizationId === null.
---
Outside diff comments:
In `@apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx`:
- Around line 81-89: The subtitle and the Continue-button visibility disagree
because subtitle suggests continuing when atLeastOneConnected is true while
inAnyStep is true whenever the other provider is missing; fix by making the
Continue-button visibility match the subtitle: change the condition that
controls the Continue button (currently using inAnyStep) to require
atLeastOneConnected && !reconfiguringClaude && !reconfiguringCodex (i.e., allow
Continue when atLeastOneConnected and neither provider is in reconfiguration),
or alternatively update the subtitle string to reflect the stricter behavior;
locate the variables subtitle and inAnyStep and the Continue-button render that
checks inAnyStep (lines near where Continue is rendered) and apply one of these
two changes so the UI copy and button visibility are consistent.
---
Nitpick comments:
In `@apps/desktop/src/renderer/hooks/useEnsureV2Project/useEnsureV2Project.ts`:
- Around line 39-50: Replace the ad-hoc cast when checking the error code in
useEnsureV2Project with tRPC's type guard: call
isTRPCClientError<AppRouter>(err) and only then read err.data?.code (or reuse
the existing trpcCode helper extracted to a shared util); update the catch in
useEnsureV2Project to guard with isTRPCClientError before comparing to
"NOT_FOUND" so unrelated errors (DOMException, fetch errors, etc.) are not
misclassified.
In
`@apps/desktop/src/renderer/routes/_authenticated/setup/adopt-worktrees/page.tsx`:
- Around line 209-228: The progress counter only increments on successful
imports because setProgress uses totalImported; change the loop to track an
attempts counter (e.g., attemptedCount initialized before the for loop) and
increment it on every iteration (regardless of result.ok), then call
setProgress({ current: attemptedCount, total: totalToImport }); keep
totalImported incrementing only on success so toasts and
ensureWorkspaceInSidebar(result.workspaceId, v2ProjectId) remain unchanged for
success/failure handling.
In `@apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx`:
- Around line 222-224: The JSX conditional currently uses a redundant guard
"atLeastOneConnected && !inAnyStep" to render <SetupButton
onClick={handleContinueToNextStep}>; simplify it by removing the unnecessary
atLeastOneConnected check and render the button when "!inAnyStep" only (keep the
same <SetupButton> and handleContinueToNextStep callback), since inAnyStep
already captures the connected-state logic.
🪄 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: 41e21196-4747-4ee1-bafe-8a081ac3e6c3
📒 Files selected for processing (6)
apps/desktop/src/renderer/components/V2AvailableBanner/V2AvailableBanner.tsxapps/desktop/src/renderer/hooks/useEnsureV2Project/index.tsapps/desktop/src/renderer/hooks/useEnsureV2Project/useEnsureV2Project.tsapps/desktop/src/renderer/routes/_authenticated/setup/adopt-worktrees/page.tsxapps/desktop/src/renderer/routes/_authenticated/setup/project/page.tsxapps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx
| const found = await hostService.project.findByPath.query({ repoPath }); | ||
| const candidate = found.candidates[0]; | ||
| if (candidate) { | ||
| try { | ||
| const setupResult = await hostService.project.setup.mutate({ | ||
| projectId: candidate.id, | ||
| mode: { kind: "import", repoPath }, | ||
| }); | ||
| return { | ||
| hostUrl: activeHostUrl, | ||
| projectId: candidate.id, | ||
| repoPath: setupResult.repoPath, | ||
| mainWorkspaceId: setupResult.mainWorkspaceId, | ||
| }; | ||
| } catch (err) { | ||
| // findByPath returns local sqlite rows even when no cloud v2 project | ||
| // exists for that id; setup → v2Project.get → NOT_FOUND. Only that | ||
| // case is safe to fall through to create — every other error | ||
| // (network, auth, 5xx) must propagate so retries don't silently mint | ||
| // duplicate cloud projects. | ||
| const code = (err as { data?: { code?: string } } | null | undefined) | ||
| ?.data?.code; | ||
| if (code !== "NOT_FOUND") { | ||
| throw err; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Multiple findByPath candidates are silently ignored — risks linking to the wrong project.
findByPath returns a candidates list, and the sibling useFolderFirstImport explicitly treats candidates.length > 1 as an error case (surfacing it to the caller via onMultipleProjects / onError) precisely because more than one local sqlite row can match the same repoPath. Here, the hook just picks candidates[0] and proceeds to setup.mutate against it.
When this hook is used during adopt-worktrees, this means a project whose mainRepoPath matches multiple historical entries will be linked to whichever one happens to come first — potentially the wrong cloud project — without any error or user prompt.
Consider mirroring useFolderFirstImport's behavior: if found.candidates.length > 1, throw (or surface to the caller) so the caller can decide rather than silently disambiguating.
🛡️ Suggested guard
const found = await hostService.project.findByPath.query({ repoPath });
-const candidate = found.candidates[0];
-if (candidate) {
+if (found.candidates.length > 1) {
+ throw new Error(
+ `Multiple projects use this repository (${found.candidates.length}). Open the intended one from settings before importing.`,
+ );
+}
+const candidate = found.candidates[0];
+if (candidate) {
try {🤖 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/hooks/useEnsureV2Project/useEnsureV2Project.ts`
around lines 25 - 51, The hook useEnsureV2Project currently picks
found.candidates[0] from hostService.project.findByPath.query and proceeds,
which can silently link the wrong project; instead, mirror useFolderFirstImport:
after calling hostService.project.findByPath.query({ repoPath }) check if
found.candidates.length > 1 and abort by surfacing an error (throw a specific
Error or call the same onMultipleProjects/onError path used elsewhere) rather
than selecting candidates[0]; only when candidates.length === 1 proceed to call
hostService.project.setup.mutate with candidate.id, and preserve the existing
NOT_FOUND handling for setup errors.
| current: number; | ||
| total: number; | ||
| } | null>(null); | ||
| const hostNotReady = !activeHostUrl; |
There was a problem hiding this comment.
Disabled-button predicate disagrees with the runtime guard on host availability.
The button is disabled based on hostNotReady = !activeHostUrl (line 297), but handleImportSelected aborts with toast.error("No active host") when !machineId (line 173). activeHostUrl and machineId are separate fields on useLocalHostService, and there is no guarantee that activeHostUrl being set implies machineId is set.
If activeHostUrl is populated before machineId, the button is clickable, the user clicks "Import N selected", and is greeted with a generic "No active host" toast even though the UI told them the host was ready. Make the disabled predicate the same as the runtime guard, e.g.:
🛡️ Align disabled and runtime checks
- const hostNotReady = !activeHostUrl;
+ const hostNotReady = !activeHostUrl || !machineId;Also applies to: 172-176, 294-298
🤖 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/setup/adopt-worktrees/page.tsx`
at line 123, The disabled predicate uses hostNotReady = !activeHostUrl but the
runtime guard in handleImportSelected checks for machineId from
useLocalHostService; update the predicate so the button is disabled when the
runtime guard would abort (e.g., set hostNotReady = !machineId or hostNotReady =
!activeHostUrl || !machineId) and similarly adjust any other checks referenced
(lines around handleImportSelected and the button render) so the UI disabled
state and the runtime guard (handleImportSelected) use the same condition;
ensure you reference useLocalHostService, activeHostUrl, machineId,
hostNotReady, and handleImportSelected when making the change.
| const { data: session } = authClient.useSession(); | ||
| const activeOrganizationId = env.SKIP_ENV_VALIDATION | ||
| ? MOCK_ORG_ID | ||
| : (session?.session?.activeOrganizationId ?? null); | ||
|
|
||
| const { data: projects = [], isLoading } = useLiveQuery( | ||
| (q) => | ||
| q | ||
| .from({ projects: collections.v2Projects }) | ||
| .where(({ projects }) => | ||
| eq(projects.organizationId, activeOrganizationId ?? ""), | ||
| ) | ||
| .select(({ projects }) => ({ | ||
| id: projects.id, | ||
| name: projects.name, | ||
| repoCloneUrl: projects.repoCloneUrl, | ||
| })), | ||
| [collections, activeOrganizationId], |
There was a problem hiding this comment.
Silent fallback when activeOrganizationId is null hides session/setup errors.
When SKIP_ENV_VALIDATION is false and there is no active session/org, activeOrganizationId is null, and the useLiveQuery filter becomes eq(projects.organizationId, "") — which matches nothing, so the page renders the empty "Select a repository" branch as if the user simply has no projects. If the user actually does have projects but the session/org isn't loaded yet or failed to load, this is indistinguishable from the empty-state, and they'll be nudged into creating duplicates.
Consider treating activeOrganizationId === null as a loading/error state (e.g., keep the spinner up, or surface the auth issue) rather than running a query that silently returns no rows.
🤖 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/setup/project/page.tsx`
around lines 30 - 47, The query is silently returning no rows when
activeOrganizationId is null; update the logic around authClient.useSession and
useLiveQuery so you do not run the projects query with an empty string.
Specifically, detect when env.SKIP_ENV_VALIDATION is false and
session?.session?.activeOrganizationId is null (i.e., activeOrganizationId ===
null) and in that case skip or short-circuit useLiveQuery (don’t call q.where
with ""), and instead surface a loading/error state in the UI (e.g., keep the
spinner or show an auth setup error) until the session resolves; adjust the code
that builds the dependencies ([collections, activeOrganizationId]) and the
render branch that currently assumes an empty projects array so it reflects the
loading/error state when activeOrganizationId === null.
| const handleSelectNewRepo = async () => { | ||
| const created = await openNew(); | ||
| const project = created[0]; | ||
| if (project) { | ||
| const result = await folderImport.start(); | ||
| if (result) { | ||
| markComplete("project"); | ||
| setManualWalkthrough(false); | ||
| await openProjectInWorkspace(project.id); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Confirm intentional: no navigation after successful import.
Unlike the previous version (which navigated to /project/$projectId), handleSelectNewRepo now just calls markComplete("project") and stays on the page. Per the PR description ("Keep users on /setup/project after importing… and require explicit 'Continue with current' to advance") this is the desired behavior, but it relies on the live query promptly reflecting the newly created v2 project so the user sees the hasProjects branch and a "Continue with current" CTA. If the live query lags, the user is left on the empty-state page after a successful import with no obvious feedback that anything happened.
A success toast (or relying on useFolderFirstImport's own success signaling) would make the no-navigation flow less confusing.
🤖 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/setup/project/page.tsx`
around lines 63 - 68, handleSelectNewRepo currently calls folderImport.start()
and markComplete("project") but gives no immediate feedback if the live query
that surfaces the new v2 project lags; update handleSelectNewRepo to surface
success to the user by either triggering a success toast (e.g., call the app's
toast/notification API with a clear message when folderImport.start() resolves)
or by using the success callback/signal from useFolderFirstImport to set local
UI state (e.g., set a “recentlyImported” flag) so the component immediately
renders the has-projects branch and the "Continue with current" CTA even before
the live query updates. Ensure you reference handleSelectNewRepo,
folderImport.start, markComplete, and useFolderFirstImport when making the
change.
Recorded as integrated via -s ours after batch PRs #455-#464. Taken via individual PRs: - PR 1 (#455): v2 polish 前半 safe set (9 commits) - PR 2 (#456): v2/host-service polish 中盤 (12 commits) - PR 3 (#457): sidebar polish + jwt API (5 commits) - PR 4 (#458): host-service tRPC retry/cache/timeout (3 commits) - PR 5 (#459): v2 diff pane / file pane polish (2 commits) - PR 7 (#462): host-service v2 canonical workspace.create + attachment store (PR1 ParadevAI#3893 + PR2 ParadevAI#3916) - PR 11 (#463): agents API + onboarding (7 commits + 1 cleanup) - PR 12 (#464): v1→v2 import flow rewrite (11 commits + 2 follow-ups) - PR 13 (#460): host-service shell env probe + typo (2 commits) - PR 16 (#461): marketplace 19 themes (1 commit) Skipped / deferred (recorded as integrated for behind=0): - PR 6: CLI v1 launch (ParadevAI#3898 + 30+ CLI/SDK followups) — defer to dedicated migration - PR 9: v2 PR3 (ParadevAI#3940) + revert (ParadevAI#4017) — net-zero pair - PR 10: pty-daemon (ParadevAI#3896, ParadevAI#3971, ParadevAI#4054) — fork keeps its terminal-host - PR 14: Slack MCP-v2 (ParadevAI#4197, ParadevAI#4208) — depends on mcp-v2/sdk divergence - PR 15: onboarding remaining (ParadevAI#4115, ParadevAI#4125, ParadevAI#4214, ParadevAI#4213, ParadevAI#4222, ParadevAI#4225) — depends on fork's deleted setup pages Behind: 0 after this merge.
…n system (ParadevAI#4125) * fix(desktop): drop onboarding auto-skip, switch SetupButton to design system The auto-advance logic in onboarding steps was rerouting users away from pages they explicitly navigated to (e.g. clicking "Configure Codex" dropped them on /setup/gh-cli because Claude was already connected). The gate intended to suppress this — `manualWalkthrough` — was never set to true anywhere, so the auto-skip fired unconditionally. Removes: - `shouldAutoAdvance` effects in providers, gh-cli, and project steps - The no-projects / no-worktrees auto-finish in adopt-worktrees - The dead `manualWalkthrough` flag and its action Per-step "Skip for now" buttons are unchanged. Also rewrites SetupButton as a thin wrapper around @superset/ui/button, mapping primary/secondary/link to the design system's variants and the xs size, so onboarding CTAs share theming, focus ring, and hover behavior with the rest of the app. * fix(desktop): create v2 projects from onboarding when v2 is enabled The onboarding "Select new repo" button always called the v1 projects.openNew IPC mutation, which writes a v1 project row in the local Electron SQLite. When v2 is enabled, navigating to /project/$projectId then loads a v2 project page that has no matching cloud row, leaving the user with a stranded v1 project. Routes the v2 case through useFolderFirstImport — the same hook the v2 dashboard's "Import folder" uses — so it goes through client.project.create on the host service and ends up as a real v2 project + main workspace. Falls back to the existing v1 path otherwise. The /new-project clone flow is still v1-only and is reached from many places outside onboarding; left as a follow-up. * refactor(desktop): make onboarding project step v2-only v1 is deprecated; the onboarding flow shouldn't read from or write to v1 anymore. Drops the isV2CloudEnabled gate and all v1 paths from the project setup step: - Project list now reads collections.v2Projects directly via useLiveQuery, scoped to the active org. Replaces the v1 electronTrpc.projects.getRecents query. - "Select new repo" always goes through useFolderFirstImport (client.project.create on the host service). No more v1 fallback. - "Clone from GitHub" is removed because /new-project is still v1. Folder-first import is the only supported entry until a v2 clone surface lands. - Per-row delete is removed. v2 client.project.remove deletes from the org and is too heavy for an onboarding affordance; users can manage projects from settings/projects. The next adopt-worktrees step is still v1-only (no v2 equivalent for external worktree adoption exists). Left as a follow-up. * fix(desktop): keep onboarding flow after import in project step handleSelectNewRepo navigated to /project/$projectId immediately after the folder-first import resolved, kicking the user out of the onboarding flow. The v2 project page also rendered before Electric had synced the new v2_projects row into collections.v2Projects, so it looked like nothing was added. Stay on /setup/project after import. The live query against collections.v2Projects updates once Electric streams the new row in, the project shows up in the on-page list, and the user clicks "Continue with current" to advance. * feat(desktop): make onboarding v2-compatible end to end - adopt-worktrees: drop v1 fallbacks (useIsV2CloudEnabled / projects navigation / lastViewedWorkspaceId), drop the v1 importExternalWorktrees path. Land users on /v2-workspaces and let the dashboard route handle empty/last-workspace logic instead of duplicating it here. - adopt-worktrees: per-project ensureV2Project + finalizeProjectSetup, then submit each worktree as a v2 workspace via useWorkspaceCreates and pin via ensureWorkspaceInSidebar. "Importing X of Y…" progress, stay on page if every selected import fails, "Connecting…" gating until the host service is up. - setup/project: gate "Select new repo" / "Continue with current" buttons on activeHostUrl with a "Connecting…" label so v2 actions can't fire before the host returns a port. - providers: focus one provider at a time during configure/reconfigure, smart-default to the unconnected provider when one is connected, "Set up X instead" switcher when neither is connected, hide Continue while any step is active. - V2AvailableBanner: only render for v1 users; action routes to /settings/experimental ("Try new version of Superset") instead of flipping setOptInV2 directly. - New useEnsureV2Project hook: findByPath → setup, falling through to project.create only on TRPC NOT_FOUND so transient errors don't mint duplicate cloud projects.
…o-v2 ParadevAI#4113 remove skip-all onboarding affordance: integrate fork's githubSyncService import alongside upstream's selectExternalWorktreesForImport in workspaces/procedures/git-status.ts. ParadevAI#4115 default new users to v2 + v2 banner in v1: - Adopt upstream's V2_CLOUD flag removal: collapse useIsV2CloudEnabled to a plain boolean (drop isRemoteV2Enabled) and update every callsite back to scalar destructure (TopBar, settings/{behavior,experimental, git,links,terminal}/page, SettingsSidebar/{GeneralSettings,SettingsSidebar}). - ExperimentalSettings.tsx: drop isRemoteV2Enabled-gated 'Early access not enabled' note and Switch disabled prop; simplify track call. - v2-local-override.ts: keep fork's IS_DEV strong default (dev installs land on v2) alongside upstream's hasPriorSupersetUsage probe (initialOptInV2 = IS_DEV ? true : !hasPriorSupersetUsage()). - TopBar.tsx: integrate upstream's useWorkspaceSidebarStore reads with fork's existing chrome layout. - useMigrateV1DataToV2/useMigrateV1DataToV2.ts: re-delete (DU; upstream attempted to revive via ParadevAI#4115 vocabulary cleanup; cycle 38 rationale still applies — fork trpc lacks the required procedures). ParadevAI#4125 onboarding auto-skip drop, SetupButton design alignment: clean cherry-pick. ParadevAI#4175 keep Skip-for-now visible during loading: clean cherry-pick. Replaces v2Projects live query with imperative v2Project.list trpc call; backfill list procedure (jwtProcedure variant) into fork's v2-project router (mirrors get pattern using ctx.organizationIds membership check). ParadevAI#4214 stable Adopt button + Adopt-all (skipped): touches v1 ImportWorkspacesPage/V1ImportModal which the fork has removed. The upstream improvements (idempotent adopt-all guard, smarter skip-when- already-running) have no shipping surface in this fork; pick up if and when v1 import modal is reintroduced. ParadevAI#4093 re-add Settings to org dropdown for v1: clean cherry-pick.
Summary
manualWalkthrough— is never set totrueanywhere, so the auto-skip always fires. Symptom: clicking Configure Codex while Claude is already connected reroutes you to/setup/gh-clibefore you can do anything.shouldAutoAdvancefromproviders,gh-cli, andprojectstep pages, plus the no-projects / no-worktrees auto-finish inadopt-worktrees. Drops the deadmanualWalkthroughfield andsetManualWalkthroughaction from the store. Per-step Skip for now buttons are unchanged.SetupButtonas a thin wrapper around@superset/ui/button(primary→default,secondary→secondary,link→link,size="xs",w-fullfor non-link). Onboarding CTAs now share theming, focus ring, and hover with the rest of the app instead of using the hardcoded orange + inset shadow.Test plan
/setup/providerswith one provider connected, click Reconfigure on the other — page stays put./setup/gh-cliwithghinstalled, the "GitHub CLI is installed" view renders instead of auto-advancing./setup/projectwith existing projects, the project list renders with Continue with current instead of auto-advancing./setup/adopt-worktreeswith no projects, the empty "nothing to adopt" state renders with a working Continue button.StartViewetc.Summary by cubic
Stops onboarding from auto-skipping, switches setup buttons to the design system, and makes the flow v2-first end to end. Project and worktree steps now create real cloud projects/workspaces and keep users in the flow.
Bug Fixes
collections.v2ProjectsviauseLiveQuery; list shows name andrepoCloneUrl. “Select new repo” usesuseFolderFirstImportand stays on/setup/projectuntil the new project appears. Dropped v1 paths and removed “Clone from GitHub” and per-row delete.New Features
/v2-workspaceson finish. UsesuseEnsureV2Projectto avoid duplicate cloud projects.Written for commit 85125db. Summary will update on new commits.
Summary by CodeRabbit
Refactor
New Features
Style