fix: handle duplicate workspace slugs in onboarding#4085
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughConverts the onboarding wizard to a controlled component, updates onboarding step hooks to accept and call an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant OC as OnboardingContent
participant OW as OnboardingWizard (controlled)
participant WS as useWorkspaceStep
participant TRPC as trpc.workspace.create
participant Auth as authProvider
participant DB as Database
U->>OW: Click "Next" (Workspace step)
OW->>WS: call currentStep.onStepNext(currentStepIndex)
WS->>TRPC: mutate({ name, slug, ... })
TRPC->>DB: begin transaction
TRPC->>DB: check existing workspace(s) & duplicate slug
alt duplicate slug
DB-->>TRPC: conflict
TRPC-->>WS: throw CONFLICT
WS-->>OW: return false (do not advance)
else ok
TRPC->>Auth: createTenant()
TRPC->>DB: insert workspace
TRPC-->>WS: { orgId }
WS->>TRPC: switchOrg(orgId)
WS->>OC: props.advance()
OC->>OW: setCurrentStepIndex(next)
WS-->>OW: return true
end
sequenceDiagram
autonumber
participant U as User
participant OW as OnboardingWizard (controlled)
participant KS as useKeyCreationStep
U->>OW: Click "Next" (Key creation)
OW->>KS: call currentStep.onStepNext(currentStepIndex)
alt not loading
KS->>KS: submit form
alt success
KS->>OW: props.advance() -> setCurrentStepIndex(next)
KS-->>OW: return true
else failure
KS-->>OW: return false
end
else loading
KS-->>OW: return false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chronark
requested review from
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/app/new/components/onboarding-wizard.tsx (1)
103-108: Back button incorrectly disabled on last stepUsers should be able to go back from the last step. Remove isLastStep from disabled condition.
Apply this diff:
<Button className="rounded-lg bg-grayA-3 hover:bg-grayA-4 h-[22px]" variant="outline" onClick={handleBack} - disabled={isFirstStep || isLoading || isLastStep} + disabled={isFirstStep || isLoading} >
🧹 Nitpick comments (8)
apps/dashboard/app/new/hooks/use-key-creation-step.tsx (2)
50-60: Centralize advance in onSuccess (minor)You can call props.advance() inside onSuccess and drop it from onSubmit to keep success-side effects in one place.
Apply this diff:
const createApiAndKey = trpc.workspace.onboarding.useMutation({ onSuccess: (data) => { setApiCreated(true); startTransition(() => { const params = new URLSearchParams(searchParams?.toString()); params.set(API_ID_PARAM, data.apiId); params.set(KEY_PARAM, data.key); router.push(`?${params.toString()}`); }); + // Move step progression here so it only fires on success + props.advance(); }, @@ - await createApiAndKey.mutateAsync(submitData); - props.advance(); + await createApiAndKey.mutateAsync(submitData);Also applies to: 99-101
235-241: Boolean returns from onStepNext are unusedOnboardingWizard doesn’t read the return value. Drop boolean returns to avoid confusion.
Apply this diff:
- onStepNext: apiCreated - ? () => true - : () => { - if (!isLoading) { - formRef.current?.requestSubmit(); - } - return false; - }, + onStepNext: apiCreated + ? () => {} + : () => { + if (!isLoading) { + formRef.current?.requestSubmit(); + } + },apps/dashboard/lib/trpc/routers/workspace/create.ts (1)
112-118: Move cache invalidation outside the transactioninvalidateWorkspaceCache is an external side-effect; doing it inside the transaction can cause inconsistencies if the tx aborts.
Apply this diff and add invalidation after the transaction:
- // Invalidate workspace cache for the new orgId and current user's orgId - // The new orgId needs invalidation for consistency (though it's new) - // The current user's orgId needs invalidation since they're switching away - await invalidateWorkspaceCache(orgId); - await invalidateWorkspaceCache(ctx.tenant.id); - return orgId; + return orgId; }) - .catch((err) => { + .catch((err) => { if (err instanceof TRPCError) { throw err; } console.error(err); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to create the workspace. Please try again or contact support@unkey.dev", }); }); - return { - orgId, - }; + // Post-commit side effects + await Promise.all([ + invalidateWorkspaceCache(orgId), + invalidateWorkspaceCache(ctx.tenant.id), + ]); + return { orgId };apps/dashboard/app/new/components/onboarding-content.tsx (2)
16-24: Remove stepCount/useEffect; compute clamp from steps.lengthYou can avoid extra state and a render by deriving clamp directly from steps.length.
Apply this diff:
- const [stepCount, setStepCount] = useState(0); - - const handleStepChange = (newStepIndex: number) => { + const handleStepChange = (newStepIndex: number) => { setCurrentStepIndex(newStepIndex); }; - const clamp = (i: number) => { - return Math.max(0, Math.min(stepCount - 1, i)); - }; + const clamp = (i: number, total: number) => Math.max(0, Math.min(total - 1, i)); @@ - // stupid hack to get around the circular dependency - // we need the length of steps to determine the number of steps for boundary checking - useEffect(() => { - setStepCount(steps.length); - }, [steps.length]); + // No extra state needed; clamp uses steps.length at call sitesAnd update callers:
- const workspaceStep = useWorkspaceStep({ - advance: () => { - handleStepChange(clamp(currentStepIndex + 1)); - }, - }); + const workspaceStep = useWorkspaceStep({ + advance: () => handleStepChange(clamp(currentStepIndex + 1, steps.length)), + }); @@ - const keyCreationStep = useKeyCreationStep({ - advance: () => handleStepChange(clamp(currentStepIndex + 1)), - }); + const keyCreationStep = useKeyCreationStep({ + advance: () => handleStepChange(clamp(currentStepIndex + 1, steps.length)), + });Also applies to: 55-60
45-48: Return value from onStepNext is unusedOnboardingWizard ignores boolean returns. Drop the return for clarity.
Apply this diff:
- onStepNext: () => { - setIsConfirmOpen(true); - return true; - }, + onStepNext: () => { + setIsConfirmOpen(true); + },apps/dashboard/app/new/components/onboarding-wizard.tsx (2)
43-45: Update prop docs to match controlled APIReplace the stale “Callback fired whenever the current step changes” comment with descriptions for currentStepIndex and setCurrentStepIndex.
Apply this diff:
- /** Callback fired whenever the current step changes */ - currentStepIndex: number; - setCurrentStepIndex: (index: number) => void; + /** Zero-based index of the current step (controlled) */ + currentStepIndex: number; + /** Setter to change the current step (controlled) */ + setCurrentStepIndex: (index: number) => void;
73-74: Optional: support boolean return from onStepNext for compatibilityIf onStepNext returns true, optionally advance to the next step to support mixed patterns during migration.
Apply this diff:
- currentStep.onStepNext?.(currentStepIndex); + const res = currentStep.onStepNext?.(currentStepIndex); + if (res === true && currentStepIndex < steps.length - 1) { + setCurrentStepIndex(currentStepIndex + 1); + }Also adjust the type if desired:
- onStepNext?: (currentStep: number) => void; + onStepNext?: (currentStep: number) => boolean | void;apps/dashboard/app/new/hooks/use-workspace-step.tsx (1)
75-79: Await router.refresh() before advancing or confirm non-blocking intent
The wizard advances immediately aftermutateAsync, butswitchOrgMutation.onSuccesscallsrouter.refresh()without awaiting it. Since the next step doesn’t fetch workspace data, this won’t break functionality, but you may see a layout flicker. Consider sequencing it explicitly—e.g.:await switchOrgMutation.mutateAsync(orgId); await router.refresh(); props.advance();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/app/new/components/onboarding-content.tsx(4 hunks)apps/dashboard/app/new/components/onboarding-fallback.tsx(1 hunks)apps/dashboard/app/new/components/onboarding-wizard.tsx(2 hunks)apps/dashboard/app/new/hooks/use-key-creation-step.tsx(3 hunks)apps/dashboard/app/new/hooks/use-workspace-step.tsx(5 hunks)apps/dashboard/lib/trpc/routers/workspace/create.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/dashboard/app/new/hooks/use-key-creation-step.tsx (1)
apps/dashboard/app/new/components/onboarding-wizard.tsx (1)
OnboardingStep(5-37)
apps/dashboard/app/new/hooks/use-workspace-step.tsx (1)
apps/dashboard/app/new/components/onboarding-wizard.tsx (1)
OnboardingStep(5-37)
apps/dashboard/app/new/components/onboarding-content.tsx (3)
apps/dashboard/app/new/hooks/use-workspace-step.tsx (1)
useWorkspaceStep(36-199)apps/dashboard/app/new/hooks/use-key-creation-step.tsx (1)
useKeyCreationStep(43-244)apps/dashboard/app/new/components/onboarding-wizard.tsx (1)
OnboardingWizard(47-184)
apps/dashboard/lib/trpc/routers/workspace/create.ts (4)
apps/dashboard/lib/db.ts (1)
db(5-26)apps/dashboard/lib/env.ts (1)
env(3-52)internal/db/src/schema/workspaces.ts (1)
workspaces(16-104)internal/id/src/generate.ts (1)
newId(35-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
🔇 Additional comments (7)
apps/dashboard/app/new/components/onboarding-fallback.tsx (1)
60-61: Controlled wiring looks goodPassing currentStepIndex and a no-op setter is appropriate for a static fallback.
apps/dashboard/app/new/hooks/use-key-creation-step.tsx (1)
39-44: Good API evolution: step-local advance callbackSwitching the hook to accept an advance(): void prop aligns with the controlled wizard. No issues.
apps/dashboard/app/new/components/onboarding-content.tsx (1)
90-94: Controlled wizard wiring LGTMWiring steps + currentStepIndex/setCurrentStepIndex matches the new controlled model.
apps/dashboard/lib/trpc/routers/workspace/create.ts (1)
50-59: Map unique‐constraint violations to TRPC CONFLICT
Extend the existing.catch((err) => …)inapps/dashboard/lib/trpc/routers/workspace/create.ts(and similarly inchangeWorkspaceNameat lines 119–129) to inspecterr.code(e.g."ER_DUP_ENTRY") or/duplicate/ionerr.messageand rethrow.catch((err: unknown & { code?: string; message?: string }) => { if (err instanceof TRPCError) throw err; const code = (err as any).code; const msg = String((err as any).message ?? ""); if (code === "ER_DUP_ENTRY" || /duplicate/i.test(msg)) { throw new TRPCError({ code: "CONFLICT", message: "A workspace with this slug already exists." }); } console.error(err); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to create the workspace. Please try again or contact support@unkey.dev" }); });Verify the Planetscale/Drizzle driver’s duplicate‐error signature to ensure correct matching.
apps/dashboard/app/new/hooks/use-workspace-step.tsx (3)
31-36: LGTM! Clean API for controlled step progression.The Props type and updated function signature clearly communicate the hook's dependency on a parent-controlled advance callback, aligning well with the PR's goal of preventing auto-advance.
102-104: LGTM! Excellent duplicate slug error handling.The CONFLICT error handling provides clear feedback through both toast notification and inline field error with auto-focus, ensuring users understand and can easily correct the duplicate slug issue.
165-171: LGTM! Essential for clearing manual CONFLICT errors.The
form.clearErrors("slug")ensures that manually set CONFLICT errors are cleared when the user modifies the slug. Note that validation errors are automatically re-evaluated by react-hook-form due tomode: "onChange", so this primarily benefits the CONFLICT error case.
|
Thank you for following the naming conventions for pull request titles! 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard/app/new/hooks/use-workspace-step.tsx (1)
193-193: Remove debug console.info.The debug console statement should be removed per the PR checklist. This was flagged in a previous review.
Apply this diff:
onStepBack: () => { - console.info("Going back from workspace step"); + // Navigation handled by parent component },
🧹 Nitpick comments (1)
apps/dashboard/app/new/hooks/use-workspace-step.tsx (1)
165-168: Consider simplifying the comment.The logic is correct—clearing errors when the user modifies the slug prevents stale validation messages. However, the comment could be more concise.
Apply this diff to simplify the comment:
onChange={(evt) => { - // If we don't clear the manually set error, it will persist even if the user clears - // or changes the input + // Clear stale validation errors when user edits the slug form.clearErrors("slug"); const v = evt.currentTarget.value; setSlugManuallyEdited(v.length > 0); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/new/hooks/use-workspace-step.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/dashboard/app/new/hooks/use-workspace-step.tsx (1)
apps/dashboard/app/new/components/onboarding-wizard.tsx (1)
OnboardingStep(5-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (3)
apps/dashboard/app/new/hooks/use-workspace-step.tsx (3)
31-36: LGTM! Props type and hook signature align with controlled navigation.The addition of the
Propstype with anadvancecallback and the updated hook signature properly support the refactored controlled navigation pattern. This aligns well with the PR objective to prevent auto-advancement and manage step progression explicitly.
75-79: LGTM! Controlled navigation via props.advance().The refactored
onSuccesshandler correctly uses the returnedorgId, switches the organization context, and then explicitly callsprops.advance()to proceed to the next step. This controlled approach prevents the auto-advancement issues mentioned in the PR objectives.
102-104: LGTM! Proper handling of duplicate workspace slugs.The
CONFLICTerror branch correctly addresses the primary bug fix in this PR by displaying a toast notification and attaching the error message to the slug field with proper focus. This provides clear user feedback when attempting to create a workspace with a duplicate slug.
There was a problem hiding this comment.
Screen.Recording.2025-10-08.at.13.33.10.mov
We need this in apps/dashboard/app/new/hooks/use-workspace-step.tsx otherwise when you go back you are stuck
onStepNext: () => {
if (workspaceCreated) {
props.advance(); // This one
}
if (!isLoading) {
formRef.current?.requestSubmit();
}
},
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/dashboard/app/new/components/onboarding-content.tsx (2)
21-28: Use functional state update for advance to avoid stale indexAvoid capturing currentStepIndex; use a functional updater and clamp. This prevents missed increments if multiple async advances occur and removes reliance on the steps closure.
Apply this diff:
- const workspaceStep = useWorkspaceStep({ - advance: () => { - handleStepChange(currentStepIndex + 1); - }, - }); + const workspaceStep = useWorkspaceStep({ + advance: () => { + setCurrentStepIndex((i) => Math.min(i + 1, stepInfos.length - 1)); + }, + }); - const keyCreationStep = useKeyCreationStep({ - advance: () => handleStepChange(currentStepIndex + 1), - }); + const keyCreationStep = useKeyCreationStep({ + advance: () => setCurrentStepIndex((i) => Math.min(i + 1, stepInfos.length - 1)), + });
44-44: Remove no-op returnOnboardingWizard no longer uses onStepNext’s return value. Drop the unused
return truefor clarity.- setIsConfirmOpen(true); - return true; + setIsConfirmOpen(true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/new/components/onboarding-content.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/dashboard/app/new/components/onboarding-content.tsx (3)
apps/dashboard/app/new/hooks/use-workspace-step.tsx (1)
useWorkspaceStep(36-197)apps/dashboard/app/new/hooks/use-key-creation-step.tsx (1)
useKeyCreationStep(43-244)apps/dashboard/app/new/components/onboarding-wizard.tsx (1)
OnboardingWizard(47-184)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/new/components/onboarding-content.tsx (2)
16-20: Bounded step setter looks goodClear, explicit bounds check without extra state. No concerns.
81-85: Controlled wizard wiring LGTM; verify Back behaviorProps are correct. Note: OnboardingWizard disables the Back button on the last step (isLastStep). Confirm this UX is intentional; otherwise, remove that condition so users can go back from the success step.
ogzhanolguncu
left a comment
ogzhanolguncu
left a comment
There was a problem hiding this comment.
I tested it with duplicated slug and it worked fine I wasn't stuck. LGTM
|
|
* fix: handle duplicate workspace slugs in onboarding * revert: previous attempt to block advancing * fix: allow advancing if you have gone back before * chore: use a different hack
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (10/10/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
perkinsjr
deleted the
10-08-fix_handle_duplicate_workspace_slugs_in_onboarding
branch
4 participants
What does this PR do?
Refactors the onboarding wizard to fix navigation issues and improve state management.
The main change is in the trpc route, we just check for duplicate slugs and throw a
CONFLICTerror.The remaining changes were necessary to retrofit the onboarding wizard to not autoadvance regardless of outcome
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin main