agent: @U0AJM7X8FBR we started this PR, it I want to make it better. Current: #1580
Open
sweetmantech wants to merge 10 commits intotestfrom
Open
agent: @U0AJM7X8FBR we started this PR, it I want to make it better. Current: #1580sweetmantech wants to merge 10 commits intotestfrom
sweetmantech wants to merge 10 commits intotestfrom
Conversation
…etion The /api/account/update route was ignoring onboardingStatus and onboardingData from the request body, so onboarding_status was never persisted. On page reload the modal re-opened because the DB still had null. Also moved persist() to run in handleComplete (awaited before redirect) instead of on the tasks step, ensuring fresh data and guaranteed completion before the page navigates away. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a multi-step onboarding wizard (welcome → complete) with new UI steps, Spotify artist search, connector OAuth flows, confetti animation, persistence hook that saves artists/updates pulse and posts onboarding data to the account API, and mounts the modal on the HomePage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Home as HomePage
participant Modal as OnboardingModal
participant Steps as Step Components
participant Search as useSpotifyArtistSearch
participant OAuth as Connector OAuth
participant Persist as useOnboardingPersist
participant API as Backend/API
User->>Home: Open app
Home->>Modal: Render OnboardingModal
User->>Modal: Interact / Next
Modal->>Steps: Render current step
Steps->>Search: Query artist (debounced)
Search-->>Steps: Artist results
User->>Steps: Add artist / trigger connect
Steps->>OAuth: Open connector popup / authorize
OAuth-->>Steps: Connection completed (popup closed)
Modal->>Persist: persist(onboardingData)
Persist->>API: POST /api/account/update (onboardingData)
Persist->>API: saveArtist (parallel calls)
Persist->>API: updatePulse (if enabled)
Persist-->>Modal: done
Modal-->>User: Close modal / optional redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 508792c2c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| callbackUrl: window.location.href, | ||
| }); | ||
| if (url) { | ||
| onConnect(slug); |
There was a problem hiding this comment.
This marks a connector as connected as soon as an authorization URL is returned, but before the user completes OAuth in the new tab. If the user closes the flow or denies access, onboarding still counts the connector as connected and persists an inflated connectedCount, which produces incorrect onboarding data and misleading UI state.
Useful? React with 👍 / 👎.
| <OnboardingConnectionsStep | ||
| connected={data.connectedSlugs ?? []} | ||
| onConnect={slug => | ||
| updateData({ connectedSlugs: [...(data.connectedSlugs ?? []), slug] }) |
There was a problem hiding this comment.
This append is computed from render-time data.connectedSlugs, so concurrent connector flows can overwrite each other: if two onConnect callbacks resolve from the same render, each uses the same stale base array and the later write drops the earlier slug. This loses connection selections when users trigger multiple connects quickly.
Useful? React with 👍 / 👎.
![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 (11)
components/Onboarding/OnboardingNavButtons.tsx (1)
3-8: Consider exporting the props interface.Per coding guidelines, component prop types should be exported with explicit
ComponentNamePropsnaming convention for better reusability and documentation.♻️ Suggested change
-interface OnboardingNavButtonsProps { +export interface OnboardingNavButtonsProps { onBack: () => void; onNext: () => void; nextLabel?: string; nextDisabled?: boolean; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingNavButtons.tsx` around lines 3 - 8, The props interface OnboardingNavButtonsProps should be exported so other modules can import and reuse it; update the declaration of the interface OnboardingNavButtonsProps to be exported (e.g., add the export keyword before the interface) and ensure any local usages/imports remain valid after making the interface exportable.components/Onboarding/onboardingRoleConfig.ts (1)
70-80: Consider referencing the existing "other" role to avoid duplication.The fallback object in
getRoleConfigduplicates the "other" role definition. Reference the existing config to stay DRY.♻️ Suggested refactor
export function getRoleConfig(roleId: string | undefined): RoleConfig { - return ( - ROLE_CONFIG_MAP[roleId ?? ""] ?? { - id: "other", - label: "Other", - icon: "✨", - description: "", - companyLabel: "Company", - artistPlaceholder: "Search for an artist…", - } - ); + return ROLE_CONFIG_MAP[roleId ?? ""] ?? ROLE_CONFIG_MAP["other"]; }Note: This assumes "other" will always exist in
ONBOARDING_ROLES. If that's not guaranteed, add a runtime check or keep the inline fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/onboardingRoleConfig.ts` around lines 70 - 80, getRoleConfig currently returns an inline "other" object as a fallback which duplicates the existing role; change it to reference the canonical entry in ROLE_CONFIG_MAP (e.g., use ROLE_CONFIG_MAP["other"]) as the default, and if "other" may not always exist add a runtime guard that falls back to a minimal inline object only when ROLE_CONFIG_MAP["other"] is undefined; update getRoleConfig to use ROLE_CONFIG_MAP[roleId ?? ""] ?? ROLE_CONFIG_MAP["other"] (with the extra guard if needed).components/Onboarding/OnboardingStepDots.tsx (2)
4-11: Unusedlabelfield in STEPS array.Each step includes a
labelproperty that's never rendered. Either remove the unused field to keep the code lean, or render the labels to provide better user context.♻️ Option A: Remove unused labels
-const STEPS: { id: OnboardingStep; label: string }[] = [ - { id: "role", label: "Role" }, - { id: "context", label: "You" }, - { id: "artists", label: "Artists" }, - { id: "connections", label: "Connect" }, - { id: "pulse", label: "Pulse" }, - { id: "tasks", label: "Tasks" }, -]; +const STEPS: OnboardingStep[] = [ + "role", "context", "artists", "connections", "pulse", "tasks" +];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingStepDots.tsx` around lines 4 - 11, The STEPS constant contains an unused label field (const STEPS: { id: OnboardingStep; label: string }[]) in OnboardingStepDots.tsx; either remove the label property from the STEPS entries and their type to clean up dead data, or update the OnboardingStepDots component to render the label (e.g., next to each dot) so the label is used—locate the STEPS array and the rendering logic in OnboardingStepDots (and the OnboardingStep type) and apply one of these two changes consistently.
23-56: Consider adding ARIA attributes for accessibility.The step progress indicator lacks screen reader context. Adding semantic attributes would improve accessibility for users relying on assistive technologies.
♿ Suggested accessibility improvements
- <div className="flex items-center justify-center gap-0 w-full"> + <div + className="flex items-center justify-center gap-0 w-full" + role="group" + aria-label={`Onboarding progress: step ${currentIdx + 1} of ${STEPS.length}`} + > {STEPS.map((step, i) => { const isCompleted = i < currentIdx; const isCurrent = i === currentIdx; return ( - <div key={step.id} className="flex items-center"> + <div + key={step.id} + className="flex items-center" + aria-current={isCurrent ? "step" : undefined} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingStepDots.tsx` around lines 23 - 56, Update the OnboardingStepDots component to expose semantic ARIA attributes: add role="list" and an appropriate aria-label (e.g., "Onboarding progress") to the outer container, set each step wrapper (the div keyed by step.id) to role="listitem", and on the dot element include aria-current="step" when isCurrent and a clear accessible name (e.g., the step.title/step.label via a visually-hidden span or aria-label) so screen readers know which step is current and what each dot represents; also mark future steps as aria-disabled="true" or omit aria-current for non-current steps. Ensure you update the JSX near STEPS.map, currentIdx, and the dot/container divs, using cn only for visual classes.app/api/account/update/route.tsx (1)
9-9: Consider adding type safety for the request body.The destructured fields from
bodyare implicitly typed asany. While this works, adding a TypeScript interface for the expected request payload would improve type safety and make the API contract explicit.🔧 Suggested type definition
interface UpdateAccountRequest { accountId: string; instruction?: string; name?: string; organization?: string; image?: string; jobTitle?: string; roleType?: string; companyName?: string; knowledges?: unknown; onboardingStatus?: Record<string, unknown>; onboardingData?: Record<string, unknown>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/account/update/route.tsx` at line 9, Create a TypeScript interface (e.g. UpdateAccountRequest) that declares accountId: string and the optional fields instruction, name, organization, image, jobTitle, roleType, companyName, knowledges, onboardingStatus, and onboardingData with appropriate types (use unknown or Record<string, unknown> where structure is not fixed), then annotate the request body with that interface where you parse/destructure body (the variable named body in the route handler) so the destructured line "const { instruction, name, organization, accountId, image, jobTitle, roleType, companyName, knowledges, onboardingStatus, onboardingData } = body;" is type-checked against UpdateAccountRequest; adjust any downstream usages to satisfy the new types.components/Onboarding/OnboardingArtistsStep.tsx (1)
117-138: Consider using a stable key instead of array index for the artist list.Using
key={i}(line 120) can cause rendering issues if artists are reordered or removed from the middle. Since artists have uniquename(with case-insensitive dedup) orspotifyUrl, consider using a composite key.♻️ Use stable key for artist items
{artists.map((a, i) => ( - <li key={i} className="flex items-center gap-3 rounded-xl border bg-muted/20 px-3 py-2.5"> + <li key={a.spotifyUrl ?? a.name.toLowerCase()} className="flex items-center gap-3 rounded-xl border bg-muted/20 px-3 py-2.5">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingArtistsStep.tsx` around lines 117 - 138, The artist list uses the array index as the React key (key={i}) which is unstable; update the list rendering in the component that maps artists (the artists.map in OnboardingArtistsStep) to use a stable composite key such as `${a.spotifyUrl ?? a.name.toLowerCase()}` or similar so keys remain stable across reorders/removals; keep the remove(i) call the same but ensure each <li> uses that stable identifier instead of i and reference ArtistAvatar and the a object fields (name, spotifyUrl, imageUrl) when constructing the key.components/Onboarding/OnboardingConnectionsStep.tsx (1)
104-119: Good loading and disabled states on the Connect button.The button properly shows a spinner during connection and is disabled to prevent double-clicks. Consider adding
aria-labelto indicate the external link behavior for screen readers.♿ Accessibility enhancement
<Button size="sm" variant="outline" onClick={() => handleConnect(c.slug)} disabled={isConnecting} className="shrink-0" + aria-label={`Connect ${c.name} (opens in new tab)`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingConnectionsStep.tsx` around lines 104 - 119, The Connect button (Button) should include an accessible label indicating it opens an external destination for screen readers; update the JSX where onClick calls handleConnect(c.slug) and isConnecting controls state to pass an aria-label like `Connect to ${c.slug} (opens external link)` or similar descriptive text to the Button (or the ExternalLink icon if you prefer the label on the icon), ensuring the label dynamically uses c.slug and preserving existing isConnecting and disabled behavior.components/Onboarding/OnboardingWelcomeStep.tsx (1)
119-129: Consider adding accessible labels for status indicators.The emoji indicators (✅/⏳) are decorative but convey status information. Screen readers may not announce these meaningfully. Consider adding
aria-labeloraria-hiddenwith a visually hidden status text.♿ Proposed accessibility improvement
<motion.div key={i} initial={{ opacity: 0, x: -8 }} animate={{ opacity: 1, x: 0 }} className="flex items-center gap-2 text-sm" > - <span className="text-base"> + <span className="text-base" aria-hidden="true"> {i < lineIdx ? "✅" : "⏳"} </span> - <span className={i < lineIdx ? "text-foreground" : "text-foreground animate-pulse"}> + <span + className={i < lineIdx ? "text-foreground" : "text-foreground animate-pulse"} + aria-label={i < lineIdx ? "Completed:" : "In progress:"} + > {line} </span> </motion.div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingWelcomeStep.tsx` around lines 119 - 129, The emoji status indicators inside the OnboardingWelcomeStep (the motion.div that renders {i < lineIdx ? "✅" : "⏳"} alongside {line}) are decorative but convey state; update that rendering so the emoji itself is aria-hidden and add an accessible status label for screen readers—either by adding an aria-label on the motion.div (e.g., "completed" vs "in progress" based on i and lineIdx) or by inserting a visually-hidden <span> with that status text next to the emoji; ensure the logic uses the same i and lineIdx conditions so screen readers receive the current status for each line.components/Onboarding/OnboardingContextStep.tsx (1)
35-47: Consider the stale closure risk in useEffect dependencies.The effect intentionally skips
nameandonChangeNameto avoid re-running after manual edits, but this pattern can cause subtle bugs if the parent re-renders with different callback references. A safer approach is to use a ref to track whether pre-fill has occurred.♻️ Alternative using a ref to track pre-fill state
+import { useEffect, useRef } from "react"; export function OnboardingContextStep({ ... }: Props) { const { userData, email } = useUserProvider(); + const hasPrefilled = useRef(false); useEffect(() => { - if (!name && (userData?.name || email)) { + if (!hasPrefilled.current && !name && (userData?.name || email)) { + hasPrefilled.current = true; const inferred = userData?.name || (email ? email .split("@")[0] .replace(/[._]/g, " ") .replace(/\b\w/g, c => c.toUpperCase()) : ""); if (inferred) onChangeName(inferred); } - }, [userData?.name, email]); // eslint-disable-line react-hooks/exhaustive-deps + }, [userData?.name, email, name, onChangeName]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingContextStep.tsx` around lines 35 - 47, Replace the current useEffect with a version that uses a ref (e.g., hasPrefilledRef) to ensure pre-fill runs only once and avoid the stale-closure risk: create const hasPrefilledRef = useRef(false), then in the effect check if (!hasPrefilledRef.current && !name && (userData?.name || email)) { /* compute inferred same as before */ if (inferred) { onChangeName(inferred); hasPrefilledRef.current = true } }, and include onChangeName, userData?.name, email (and optionally name) in the dependency array and remove the eslint-disable comment so the effect sees updated callback references.components/Onboarding/OnboardingCompleteStep.tsx (2)
97-98: Hide decorative emoji from assistive technologies.Line 97 is decorative and duplicates Line 98 semantics; mark it
aria-hiddenfor cleaner screen-reader output.Suggested fix
- <span className="text-lg">{item.icon}</span> + <span className="text-lg" aria-hidden="true">{item.icon}</span> <span className="font-medium">{item.text}</span>As per coding guidelines,
**/*.{tsx,ts,jsx,js}: "Provide proper ARIA roles/states and test with screen readers".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingCompleteStep.tsx` around lines 97 - 98, The decorative emoji span in the OnboardingCompleteStep component (the <span> rendering item.icon alongside item.text) duplicates the textual semantics and should be hidden from assistive tech; update the icon span that renders item.icon to include aria-hidden="true" (or aria-hidden={true}) so screen readers ignore it while leaving the text span (item.text) unchanged.
8-25: Use an explicit exported props type name for the component API.Replace
PropswithOnboardingCompleteStepPropsand export it for clearer, consistent typing.Suggested fix
-interface Props { +export interface OnboardingCompleteStepProps { artistNames: string[]; name: string | undefined; connectedCount: number; pulseEnabled: boolean; onComplete: () => void; } @@ export function OnboardingCompleteStep({ artistNames, name, connectedCount, pulseEnabled, onComplete, -}: Props) { +}: OnboardingCompleteStepProps) {As per coding guidelines,
**/*.{tsx,ts}: "Export component prop types with explicitComponentNamePropsnaming convention".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingCompleteStep.tsx` around lines 8 - 25, Rename the inline Props type to an exported, explicitly named type OnboardingCompleteStepProps and update the component signature to use that type: export type OnboardingCompleteStepProps = { artistNames: string[]; name: string | undefined; connectedCount: number; pulseEnabled: boolean; onComplete: () => void; } and change the function parameter typing in OnboardingCompleteStep to accept OnboardingCompleteStepProps instead of Props; ensure the new type is exported so other modules can import it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/Onboarding/OnboardingCompleteStep.tsx`:
- Around line 13-14: The OnboardingCompleteStep component allows repeated clicks
to trigger duplicate completion actions; add a local boolean state (e.g.,
isCompleting) and use it as a guard in the completion handler and to disable the
completion button: in the handler (the function that currently calls
props.onComplete or performs persistence/redirect) return early if isCompleting
is true, set isCompleting = true at the start of work, await the async work,
then set isCompleting = false (or keep true if completed and component
unmounts); also set the button's disabled/aria-disabled prop based on
isCompleting to prevent rapid re-clicks.
In `@components/Onboarding/OnboardingConfetti.tsx`:
- Around line 54-62: The borderRadius is computed with Math.random() during
render which causes particle shapes to flip on re-renders; move the randomness
into the particle creation by adding a shape/borderRadius property on each
particle inside makeParticles() (e.g., p.borderRadius or p.isRound) and set it
once when particles are created, then replace the inline Math.random() in the
render style with that particle property (refer to makeParticles() and the
particle render that currently uses borderRadius: Math.random() > 0.5 ? "50%" :
"2px").
In `@components/Onboarding/OnboardingConnectionsStep.tsx`:
- Around line 52-69: The current handleConnect function calls onConnect(slug)
optimistically before OAuth completes, making the connector appear connected
even if the user cancels the flow; update handleConnect to NOT call onConnect
immediately — instead set a "pending" state for the slug (use setConnecting or a
new setPendingConnector) and open the OAuth URL via authorizeConnectorApi and
window.open; then wire the real completion path (the OAuth callback handling
code that receives the connector callback) to call onConnect(slug) only after
verifying the connector is actually authorized, or alternatively update the UI
to show "Pending…" or "Authorization opened in new tab" while waiting for the
callback rather than marking it connected in handleConnect.
In `@components/Onboarding/OnboardingModal.tsx`:
- Around line 96-98: The onConnect handler in OnboardingModal.tsx blindly
appends slug to data.connectedSlugs which can create duplicates and inflate
connectedCount; update the onConnect logic that calls updateData so it first
checks data.connectedSlugs (or uses a Set) and only adds the slug if it's not
already present (e.g., construct a new array from existing data.connectedSlugs
without duplicates, then call updateData with connectedSlugs set to that
deduplicated array).
- Around line 27-37: The handleComplete callback currently awaits persist(data)
but always calls complete() and triggers the redirect even if persist rejects;
wrap the await persist(...) in a try/catch inside handleComplete, call
complete() and run the setTimeout redirect only on successful persist, and in
the catch branch handle or surface the error (e.g., log or show validation) so
failures don't advance onboarding; reference the handleComplete function,
persist, complete, and the setTimeout redirect logic when making the change.
In `@components/Onboarding/OnboardingWelcomeStep.tsx`:
- Around line 43-57: The useEffect in OnboardingWelcomeStep sets a
setTimeout(onDone, 700) but never tracks or clears that timeout on unmount;
update the effect to store the timeout ID (e.g. const doneTimer =
setTimeout(...)) when scheduling onDone and clear it in the cleanup function
(clearTimeout(doneTimer)), and also clear any pending doneTimer right before
calling setTimeout inside the interval branch to avoid duplicates; reference the
useEffect, setInterval, setTimeout(onDone, 700), setLineIdx, setDone and onDone
symbols when making this change.
In `@components/Onboarding/useOnboardingPersist.ts`:
- Around line 43-63: The fetch call in useOnboardingPersist (inside the
persist/update block that POSTs to "/api/account/update") only catches network
errors and ignores non-OK HTTP responses; change it to await the fetch, check
response.ok, and handle non-OK by logging or throwing an error (include response
status/text or parsed JSON error) so callers know persistence failed; ensure the
promise rejection or error is propagated instead of swallowing the failure so
onboarding UI can react.
---
Nitpick comments:
In `@app/api/account/update/route.tsx`:
- Line 9: Create a TypeScript interface (e.g. UpdateAccountRequest) that
declares accountId: string and the optional fields instruction, name,
organization, image, jobTitle, roleType, companyName, knowledges,
onboardingStatus, and onboardingData with appropriate types (use unknown or
Record<string, unknown> where structure is not fixed), then annotate the request
body with that interface where you parse/destructure body (the variable named
body in the route handler) so the destructured line "const { instruction, name,
organization, accountId, image, jobTitle, roleType, companyName, knowledges,
onboardingStatus, onboardingData } = body;" is type-checked against
UpdateAccountRequest; adjust any downstream usages to satisfy the new types.
In `@components/Onboarding/OnboardingArtistsStep.tsx`:
- Around line 117-138: The artist list uses the array index as the React key
(key={i}) which is unstable; update the list rendering in the component that
maps artists (the artists.map in OnboardingArtistsStep) to use a stable
composite key such as `${a.spotifyUrl ?? a.name.toLowerCase()}` or similar so
keys remain stable across reorders/removals; keep the remove(i) call the same
but ensure each <li> uses that stable identifier instead of i and reference
ArtistAvatar and the a object fields (name, spotifyUrl, imageUrl) when
constructing the key.
In `@components/Onboarding/OnboardingCompleteStep.tsx`:
- Around line 97-98: The decorative emoji span in the OnboardingCompleteStep
component (the <span> rendering item.icon alongside item.text) duplicates the
textual semantics and should be hidden from assistive tech; update the icon span
that renders item.icon to include aria-hidden="true" (or aria-hidden={true}) so
screen readers ignore it while leaving the text span (item.text) unchanged.
- Around line 8-25: Rename the inline Props type to an exported, explicitly
named type OnboardingCompleteStepProps and update the component signature to use
that type: export type OnboardingCompleteStepProps = { artistNames: string[];
name: string | undefined; connectedCount: number; pulseEnabled: boolean;
onComplete: () => void; } and change the function parameter typing in
OnboardingCompleteStep to accept OnboardingCompleteStepProps instead of Props;
ensure the new type is exported so other modules can import it.
In `@components/Onboarding/OnboardingConnectionsStep.tsx`:
- Around line 104-119: The Connect button (Button) should include an accessible
label indicating it opens an external destination for screen readers; update the
JSX where onClick calls handleConnect(c.slug) and isConnecting controls state to
pass an aria-label like `Connect to ${c.slug} (opens external link)` or similar
descriptive text to the Button (or the ExternalLink icon if you prefer the label
on the icon), ensuring the label dynamically uses c.slug and preserving existing
isConnecting and disabled behavior.
In `@components/Onboarding/OnboardingContextStep.tsx`:
- Around line 35-47: Replace the current useEffect with a version that uses a
ref (e.g., hasPrefilledRef) to ensure pre-fill runs only once and avoid the
stale-closure risk: create const hasPrefilledRef = useRef(false), then in the
effect check if (!hasPrefilledRef.current && !name && (userData?.name || email))
{ /* compute inferred same as before */ if (inferred) { onChangeName(inferred);
hasPrefilledRef.current = true } }, and include onChangeName, userData?.name,
email (and optionally name) in the dependency array and remove the
eslint-disable comment so the effect sees updated callback references.
In `@components/Onboarding/OnboardingNavButtons.tsx`:
- Around line 3-8: The props interface OnboardingNavButtonsProps should be
exported so other modules can import and reuse it; update the declaration of the
interface OnboardingNavButtonsProps to be exported (e.g., add the export keyword
before the interface) and ensure any local usages/imports remain valid after
making the interface exportable.
In `@components/Onboarding/onboardingRoleConfig.ts`:
- Around line 70-80: getRoleConfig currently returns an inline "other" object as
a fallback which duplicates the existing role; change it to reference the
canonical entry in ROLE_CONFIG_MAP (e.g., use ROLE_CONFIG_MAP["other"]) as the
default, and if "other" may not always exist add a runtime guard that falls back
to a minimal inline object only when ROLE_CONFIG_MAP["other"] is undefined;
update getRoleConfig to use ROLE_CONFIG_MAP[roleId ?? ""] ??
ROLE_CONFIG_MAP["other"] (with the extra guard if needed).
In `@components/Onboarding/OnboardingStepDots.tsx`:
- Around line 4-11: The STEPS constant contains an unused label field (const
STEPS: { id: OnboardingStep; label: string }[]) in OnboardingStepDots.tsx;
either remove the label property from the STEPS entries and their type to clean
up dead data, or update the OnboardingStepDots component to render the label
(e.g., next to each dot) so the label is used—locate the STEPS array and the
rendering logic in OnboardingStepDots (and the OnboardingStep type) and apply
one of these two changes consistently.
- Around line 23-56: Update the OnboardingStepDots component to expose semantic
ARIA attributes: add role="list" and an appropriate aria-label (e.g.,
"Onboarding progress") to the outer container, set each step wrapper (the div
keyed by step.id) to role="listitem", and on the dot element include
aria-current="step" when isCurrent and a clear accessible name (e.g., the
step.title/step.label via a visually-hidden span or aria-label) so screen
readers know which step is current and what each dot represents; also mark
future steps as aria-disabled="true" or omit aria-current for non-current steps.
Ensure you update the JSX near STEPS.map, currentIdx, and the dot/container
divs, using cn only for visual classes.
In `@components/Onboarding/OnboardingWelcomeStep.tsx`:
- Around line 119-129: The emoji status indicators inside the
OnboardingWelcomeStep (the motion.div that renders {i < lineIdx ? "✅" : "⏳"}
alongside {line}) are decorative but convey state; update that rendering so the
emoji itself is aria-hidden and add an accessible status label for screen
readers—either by adding an aria-label on the motion.div (e.g., "completed" vs
"in progress" based on i and lineIdx) or by inserting a visually-hidden <span>
with that status text next to the emoji; ensure the logic uses the same i and
lineIdx conditions so screen readers receive the current status for each line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10231d98-d1ae-4aea-9d01-d7d2a427310c
📒 Files selected for processing (18)
app/api/account/update/route.tsxcomponents/Home/HomePage.tsxcomponents/Onboarding/OnboardingArtistsStep.tsxcomponents/Onboarding/OnboardingCompleteStep.tsxcomponents/Onboarding/OnboardingConfetti.tsxcomponents/Onboarding/OnboardingConnectionsStep.tsxcomponents/Onboarding/OnboardingContextStep.tsxcomponents/Onboarding/OnboardingModal.tsxcomponents/Onboarding/OnboardingNavButtons.tsxcomponents/Onboarding/OnboardingPulseStep.tsxcomponents/Onboarding/OnboardingRoleStep.tsxcomponents/Onboarding/OnboardingStepDots.tsxcomponents/Onboarding/OnboardingTasksStep.tsxcomponents/Onboarding/OnboardingWelcomeStep.tsxcomponents/Onboarding/onboardingRoleConfig.tscomponents/Onboarding/useOnboarding.tscomponents/Onboarding/useOnboardingPersist.tscomponents/Onboarding/useSpotifyArtistSearch.ts
Comment on lines
+43
to
+57
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| setLineIdx(prev => { | ||
| const next = prev + 1; | ||
| if (next >= RESEARCH_LINES.length) { | ||
| clearInterval(interval); | ||
| setDone(true); | ||
| setTimeout(onDone, 700); | ||
| return prev; | ||
| } | ||
| return next; | ||
| }); | ||
| }, 520); | ||
| return () => clearInterval(interval); | ||
| }, [onDone]); |
Contributor
There was a problem hiding this comment.
Potential memory leak: setTimeout may fire after unmount.
The setTimeout(onDone, 700) on line 50 isn't cleared on unmount. If the user closes the modal before the 700ms elapses, onDone will still fire, potentially causing state updates on an unmounted component.
🛡️ Proposed fix to track and clear the timeout
export function OnboardingWelcomeStep({ onDone }: Props) {
const { email, userData } = useUserProvider();
const [lineIdx, setLineIdx] = useState(0);
const [done, setDone] = useState(false);
+ const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
// ... displayName logic ...
useEffect(() => {
const interval = setInterval(() => {
setLineIdx(prev => {
const next = prev + 1;
if (next >= RESEARCH_LINES.length) {
clearInterval(interval);
setDone(true);
- setTimeout(onDone, 700);
+ timeoutRef.current = setTimeout(onDone, 700);
return prev;
}
return next;
});
}, 520);
- return () => clearInterval(interval);
+ return () => {
+ clearInterval(interval);
+ if (timeoutRef.current) clearTimeout(timeoutRef.current);
+ };
}, [onDone]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Onboarding/OnboardingWelcomeStep.tsx` around lines 43 - 57, The
useEffect in OnboardingWelcomeStep sets a setTimeout(onDone, 700) but never
tracks or clears that timeout on unmount; update the effect to store the timeout
ID (e.g. const doneTimer = setTimeout(...)) when scheduling onDone and clear it
in the cleanup function (clearTimeout(doneTimer)), and also clear any pending
doneTimer right before calling setTimeout inside the interval branch to avoid
duplicates; reference the useEffect, setInterval, setTimeout(onDone, 700),
setLineIdx, setDone and onDone symbols when making this change.
- handleComplete: wrap persist() in try/catch to gate complete() and redirect on success only - OnboardingModal: deduplicate connectedSlugs using Set to prevent inflated connectedCount from multiple connect events - OnboardingWelcomeStep: clear the auto-advance setTimeout on unmount via useRef to prevent state updates after component is gone - useOnboardingPersist: check response.ok from /api/account/update and throw on 4xx/5xx so persist failures surface to the caller - OnboardingCompleteStep: add isCompleting state to disable the button after first click, preventing duplicate persistence/redirect attempts - OnboardingConfetti: move borderRadius randomization into makeParticles() so particle shapes are stable across re-renders Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
components/Onboarding/OnboardingCompleteStep.tsx (2)
34-51: Consider extracting summary item generation for testability.The
summaryItemsconstruction logic is inline and uses a filter-then-cast pattern. While functional, extracting this to a pure function would improve testability and make the conditional logic more explicit.♻️ Optional: Extract summary builder
+type SummaryItem = { icon: string; text: string }; + +function buildSummaryItems( + artistNames: string[], + connectedCount: number, + pulseEnabled: boolean, +): SummaryItem[] { + const items: SummaryItem[] = []; + if (artistNames.length > 0) { + items.push({ + icon: "🎤", + text: `Deep research running on ${artistNames.slice(0, 2).join(" & ")}${ + artistNames.length > 2 ? ` +${artistNames.length - 2} more` : "" + }`, + }); + } + if (connectedCount > 0) { + items.push({ + icon: "🔗", + text: `${connectedCount} platform${connectedCount > 1 ? "s" : ""} connected`, + }); + } + if (pulseEnabled) { + items.push({ icon: "⚡", text: "Pulse active — briefing arrives tomorrow morning" }); + } + items.push( + { icon: "✅", text: "First week of tasks queued" }, + { icon: "🧠", text: "AI learning your artists and fans right now" }, + ); + return items; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingCompleteStep.tsx` around lines 34 - 51, Extract the inline summaryItems logic into a pure function (e.g., buildSummaryItems or getSummaryItems) that accepts artistNames: string[], connectedCount: number, and pulseEnabled: boolean and returns a typed array { icon: string; text: string }[]; move the conditional item creation out of the JSX and into that function, avoid the filter(Boolean) + cast pattern by only pushing valid items to the result array or using a compact map/filter with proper typing, then replace the inline summaryItems with a call to the new function so the logic is testable and isolated.
58-63: Minor indentation inconsistency in JSX.The
motion.divstarting at line 58 has inconsistent indentation compared to its parent structure. While this doesn't affect functionality, consistent formatting improves readability.🔧 Fix indentation
<> <OnboardingConfetti /> <motion.div - className="flex flex-col items-center gap-7 text-center" - initial={{ opacity: 0, scale: 0.95 }} - animate={{ opacity: 1, scale: 1 }} - transition={{ duration: 0.4, ease: "easeOut" }} - > + className="flex flex-col items-center gap-7 text-center" + initial={{ opacity: 0, scale: 0.95 }} + animate={{ opacity: 1, scale: 1 }} + transition={{ duration: 0.4, ease: "easeOut" }} + >Also applies to: 126-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingCompleteStep.tsx` around lines 58 - 63, The JSX indentation for the motion.div elements in OnboardingCompleteStep (the <motion.div ...> blocks) is inconsistent with the surrounding JSX; fix by aligning those motion.div opening and closing tags and their props to the same indentation level as their sibling JSX elements inside the component (ensure the className/initial/animate/transition props are indented one level in from the parent element), and make the same adjustment for the other occurrence of the motion.div to maintain consistent formatting throughout the component.components/Onboarding/OnboardingModal.tsx (1)
39-39: Extract magic number to a named constant.The
200millisecond delay is a magic number. Extracting it to a named constant improves readability and makes the intent clearer.♻️ Extract constant
+const REDIRECT_DELAY_MS = 200; + const handleComplete = useCallback(async () => { // ... - setTimeout(() => { window.location.href = `/?q=${q}`; }, 200); + setTimeout(() => { window.location.href = `/?q=${q}`; }, REDIRECT_DELAY_MS); }, [complete, persist, data]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingModal.tsx` at line 39, Extract the magic number 200ms used in the setTimeout inside the OnboardingModal component into a named constant (e.g., ONBOARDING_REDIRECT_DELAY_MS or REDIRECT_DELAY_MS) declared at the top of the module (or near the OnboardingModal definition) and replace the inline literal in setTimeout(() => { window.location.href = `/?q=${q}`; }, 200) with that constant so the delay is self-documenting and easy to adjust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/Onboarding/OnboardingModal.tsx`:
- Around line 27-41: The catch block in handleComplete currently swallows
persistence errors; update handleComplete to catch the error object from
persist(data) and show a user-facing toast via sonner (e.g., toast.error or
toast) with a clear message like "Failed to save onboarding — please try again"
and optionally include short error details, then return early (so complete() is
not called); keep the existing flow that calls complete() only after successful
await persist(data) and leaves the artist redirect logic (artistNames, encoded
q, setTimeout -> window.location.href) unchanged.
- Around line 35-40: In OnboardingModal, the redirect behavior is inconsistent:
when artistNames has entries the code builds q and does setTimeout(() => {
window.location.href = `/?q=${q}`; }, 200) but when artistNames is empty the
modal simply closes—either add a concise comment above this block (referencing
artistNames, q, setTimeout, window.location.href) explaining this intentional
difference, or unify behavior by providing a fallback/default query for the
empty-artist path and perform the same redirect pattern (use the same timeout +
window.location.href logic) instead of only closing the modal so both flows
consistently land on HomePage/Chat with a query param.
---
Nitpick comments:
In `@components/Onboarding/OnboardingCompleteStep.tsx`:
- Around line 34-51: Extract the inline summaryItems logic into a pure function
(e.g., buildSummaryItems or getSummaryItems) that accepts artistNames: string[],
connectedCount: number, and pulseEnabled: boolean and returns a typed array {
icon: string; text: string }[]; move the conditional item creation out of the
JSX and into that function, avoid the filter(Boolean) + cast pattern by only
pushing valid items to the result array or using a compact map/filter with
proper typing, then replace the inline summaryItems with a call to the new
function so the logic is testable and isolated.
- Around line 58-63: The JSX indentation for the motion.div elements in
OnboardingCompleteStep (the <motion.div ...> blocks) is inconsistent with the
surrounding JSX; fix by aligning those motion.div opening and closing tags and
their props to the same indentation level as their sibling JSX elements inside
the component (ensure the className/initial/animate/transition props are
indented one level in from the parent element), and make the same adjustment for
the other occurrence of the motion.div to maintain consistent formatting
throughout the component.
In `@components/Onboarding/OnboardingModal.tsx`:
- Line 39: Extract the magic number 200ms used in the setTimeout inside the
OnboardingModal component into a named constant (e.g.,
ONBOARDING_REDIRECT_DELAY_MS or REDIRECT_DELAY_MS) declared at the top of the
module (or near the OnboardingModal definition) and replace the inline literal
in setTimeout(() => { window.location.href = `/?q=${q}`; }, 200) with that
constant so the delay is self-documenting and easy to adjust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fc123ab-b481-40ea-8169-612fd1e5afce
📒 Files selected for processing (5)
components/Onboarding/OnboardingCompleteStep.tsxcomponents/Onboarding/OnboardingConfetti.tsxcomponents/Onboarding/OnboardingModal.tsxcomponents/Onboarding/OnboardingWelcomeStep.tsxcomponents/Onboarding/useOnboardingPersist.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- components/Onboarding/useOnboardingPersist.ts
- components/Onboarding/OnboardingConfetti.tsx
- components/Onboarding/OnboardingWelcomeStep.tsx
- Defer onConnect until OAuth popup closes to prevent false checkmarks on cancel - Use functional updater in updateData to avoid stale closure dropping concurrent slugs - Show sonner toast on persist error instead of silently swallowing it - Add comment explaining intentional redirect behavior difference (artists vs no artists) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
components/Onboarding/OnboardingConnectionsStep.tsx (1)
61-69:⚠️ Potential issue | 🟠 MajorClosing the auth window still marks the connector as connected.
Manual close or a canceled OAuth flow will still hit
onConnect(slug), so the checkmark is still optimistic. Please only mark the slug connected from a verified callback or a backend status check after the OAuth flow completes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Onboarding/OnboardingConnectionsStep.tsx` around lines 61 - 69, The current logic in the popup OAuth flow unconditionally calls onConnect(slug) when the popup is closed (see window.open, popup, checkClosed, onConnect, setConnecting), which marks connectors connected even on user-cancel or failed OAuth; change this to only mark a connector connected after a confirmed backend callback or verified status: remove the onConnect(slug) invocation from the popup.closed branch and instead implement a verification step (e.g., poll a backend endpoint or listen for a postMessage/webhook confirmation) that checks connection status for the given slug and only call onConnect(slug) and setConnecting(null) after that verification succeeds, while ensuring the popup closed case cancels the spinner and does not optimistically mark the connection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/Onboarding/OnboardingConnectionsStep.tsx`:
- Line 50: The current connecting state (connecting / setConnecting) holds a
single slug so a second OAuth flow overwrites the first and clears the wrong
spinner; fix by making connecting a collection or global flag and updating the
handlers: either change connecting from string|null to Set<string> (or
string[]), call setConnecting.add(slug) when a flow starts and remove the slug
on finish/error (ensure cleanup in the popup close/complete handlers), and
update the Connect button disabled logic to check collection.size>0 or
collection.includes(slug) as appropriate; alternatively, if you want to allow
only one flow at a time, convert connecting to a boolean and disable every
Connect button while connecting is true. Update all usages of connecting and
setConnecting (the click handler that initiates auth, the popup completion
handler, and any UI button disabled checks) to match the chosen approach.
- Around line 75-76: The empty catch in OnboardingConnectionsStep.tsx swallows
errors from authorizeConnectorApi() so users get no feedback; update the catch
to capture the thrown error and display a toast using sonner (e.g., toast.error)
with a clear message including the error.message (or a fallback) so the user
knows connector authorization failed, and then either return/stop further flow
or rethrow as appropriate; ensure you import toast from 'sonner' and reference
authorizeConnectorApi in the catch handling.
In `@components/Onboarding/OnboardingModal.tsx`:
- Around line 52-56: The Dialog currently has no accessible name; add a visible
or hidden heading and reference it from the dialog so screen readers can
announce the title: either include a DialogTitle element inside OnboardingModal
with the human-readable title text (e.g., <DialogTitle>Onboarding</DialogTitle>)
or render a heading element with a stable id (e.g., id="onboarding-dialog-title"
and visually-hidden styling) and pass aria-labelledby="onboarding-dialog-title"
to the DialogContent (or the dialog root) alongside the existing props
(onInteractOutside, onEscapeKeyDown) so assistive tech can identify the dialog.
In `@components/Onboarding/useOnboarding.ts`:
- Around line 46-61: The hook currently only toggles isOpen via complete(), so
stale userData with onboarding_status will reopen the wizard; add a local
per-account completion latch (e.g., a state map keyed by userData.id or
accountId) inside useOnboarding (lookup on mount and set when complete() is
called) or perform an optimistic update to userData via useOnboardingPersist()
so subsequent userData emissions don't reopen the wizard; update the useEffect
that reads userData/onboarding_status to consult this local latch before calling
setIsOpen and ensure complete() writes to the latch and/or updates userData
optimistically.
---
Duplicate comments:
In `@components/Onboarding/OnboardingConnectionsStep.tsx`:
- Around line 61-69: The current logic in the popup OAuth flow unconditionally
calls onConnect(slug) when the popup is closed (see window.open, popup,
checkClosed, onConnect, setConnecting), which marks connectors connected even on
user-cancel or failed OAuth; change this to only mark a connector connected
after a confirmed backend callback or verified status: remove the
onConnect(slug) invocation from the popup.closed branch and instead implement a
verification step (e.g., poll a backend endpoint or listen for a
postMessage/webhook confirmation) that checks connection status for the given
slug and only call onConnect(slug) and setConnecting(null) after that
verification succeeds, while ensuring the popup closed case cancels the spinner
and does not optimistically mark the connection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff6f4489-f2c4-4ff8-94ce-d09bc0c9e6e1
📒 Files selected for processing (3)
components/Onboarding/OnboardingConnectionsStep.tsxcomponents/Onboarding/OnboardingModal.tsxcomponents/Onboarding/useOnboarding.ts
| */ | ||
| export function OnboardingConnectionsStep({ connected, onConnect, onNext, onBack }: Props) { | ||
| const accessToken = useAccessToken(); | ||
| const [connecting, setConnecting] = useState<string | null>(null); |
Contributor
There was a problem hiding this comment.
A second connector flow overwrites the first loading state.
connecting only stores one slug, but only the matching button is disabled. Starting a second auth flow makes the first card look idle and lets popup A clear popup B's spinner when it closes. Either track pending slugs per connector or disable every Connect button while any flow is active.
💡 Minimal fix if only one OAuth flow should run at a time
@@
const accessToken = useAccessToken();
const [connecting, setConnecting] = useState<string | null>(null);
+ const isAuthorizing = connecting !== null;
@@
- disabled={isConnecting}
+ disabled={isAuthorizing}Also applies to: 96-97, 114-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Onboarding/OnboardingConnectionsStep.tsx` at line 50, The current
connecting state (connecting / setConnecting) holds a single slug so a second
OAuth flow overwrites the first and clears the wrong spinner; fix by making
connecting a collection or global flag and updating the handlers: either change
connecting from string|null to Set<string> (or string[]), call
setConnecting.add(slug) when a flow starts and remove the slug on finish/error
(ensure cleanup in the popup close/complete handlers), and update the Connect
button disabled logic to check collection.size>0 or collection.includes(slug) as
appropriate; alternatively, if you want to allow only one flow at a time,
convert connecting to a boolean and disable every Connect button while
connecting is true. Update all usages of connecting and setConnecting (the click
handler that initiates auth, the popup completion handler, and any UI button
disabled checks) to match the chosen approach.
Comment on lines
+75
to
+76
| } catch { | ||
| // silently continue |
Contributor
There was a problem hiding this comment.
Surface connector authorization failures.
authorizeConnectorApi() throws on non-2xx responses, and this empty catch drops the user back to idle with no explanation. Toast the failure so they know the auth flow never started.
💡 Proposed fix
@@
import { useState } from "react";
+import { toast } from "sonner";
@@
- } catch {
- // silently continue
+ } catch {
+ toast.error("Couldn't start the connector authorization. Please try again.");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Onboarding/OnboardingConnectionsStep.tsx` around lines 75 - 76,
The empty catch in OnboardingConnectionsStep.tsx swallows errors from
authorizeConnectorApi() so users get no feedback; update the catch to capture
the thrown error and display a toast using sonner (e.g., toast.error) with a
clear message including the error.message (or a fallback) so the user knows
connector authorization failed, and then either return/stop further flow or
rethrow as appropriate; ensure you import toast from 'sonner' and reference
authorizeConnectorApi in the catch handling.
Comment on lines
+52
to
+56
| <DialogContent | ||
| className="max-w-lg p-0 gap-0 overflow-hidden [&>button]:hidden" | ||
| onInteractOutside={e => e.preventDefault()} | ||
| onEscapeKeyDown={e => e.preventDefault()} | ||
| > |
Contributor
There was a problem hiding this comment.
Give the onboarding dialog an accessible name.
This modal opens without a DialogTitle or aria-labelledby, so assistive tech will announce an unnamed dialog. Add a visible title or wire the dialog to a hidden heading.
♿ Proposed fix
@@
<DialogContent
+ aria-labelledby="onboarding-dialog-title"
className="max-w-lg p-0 gap-0 overflow-hidden [&>button]:hidden"
onInteractOutside={e => e.preventDefault()}
onEscapeKeyDown={e => e.preventDefault()}
>
+ <h2 id="onboarding-dialog-title" className="sr-only">
+ Recoupable onboarding
+ </h2>
{isProgressStep && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Onboarding/OnboardingModal.tsx` around lines 52 - 56, The Dialog
currently has no accessible name; add a visible or hidden heading and reference
it from the dialog so screen readers can announce the title: either include a
DialogTitle element inside OnboardingModal with the human-readable title text
(e.g., <DialogTitle>Onboarding</DialogTitle>) or render a heading element with a
stable id (e.g., id="onboarding-dialog-title" and visually-hidden styling) and
pass aria-labelledby="onboarding-dialog-title" to the DialogContent (or the
dialog root) alongside the existing props (onInteractOutside, onEscapeKeyDown)
so assistive tech can identify the dialog.
Comment on lines
+46
to
+61
| const { userData } = useUserProvider(); | ||
| const [isOpen, setIsOpen] = useState(false); | ||
| const [step, setStep] = useState<OnboardingStep>("welcome"); | ||
| const [data, setData] = useState<Partial<OnboardingData>>({ | ||
| artists: [], | ||
| connectedSlugs: [], | ||
| pulseEnabled: false, | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!userData) return; | ||
| const status = userData.onboarding_status as Record<string, unknown> | null; | ||
| if (!status?.completed) { | ||
| setIsOpen(true); | ||
| } | ||
| }, [userData]); |
Contributor
There was a problem hiding this comment.
Closing locally is not durable if userData stays stale.
complete() only flips isOpen, so any later userData emission with the old onboarding_status will reopen the wizard. Since useOnboardingPersist() does not refresh userData, this hook needs a local completion latch keyed to the current account, or an optimistic provider update.
💡 One way to latch completion per account
@@
export function useOnboarding() {
const { userData } = useUserProvider();
+ const currentAccountKey = userData?.account_id ?? "__missing_account__";
const [isOpen, setIsOpen] = useState(false);
+ const [completedForAccountKey, setCompletedForAccountKey] = useState<string | null>(null);
const [step, setStep] = useState<OnboardingStep>("welcome");
const [data, setData] = useState<Partial<OnboardingData>>({
artists: [],
connectedSlugs: [],
pulseEnabled: false,
});
useEffect(() => {
- if (!userData) return;
+ if (!userData || completedForAccountKey === currentAccountKey) return;
const status = userData.onboarding_status as Record<string, unknown> | null;
- if (!status?.completed) {
- setIsOpen(true);
- }
- }, [userData]);
+ setIsOpen(!status?.completed);
+ }, [userData, currentAccountKey, completedForAccountKey]);
@@
const complete = useCallback(() => {
+ setCompletedForAccountKey(currentAccountKey);
setIsOpen(false);
- }, []);
+ }, [currentAccountKey]);Also applies to: 88-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Onboarding/useOnboarding.ts` around lines 46 - 61, The hook
currently only toggles isOpen via complete(), so stale userData with
onboarding_status will reopen the wizard; add a local per-account completion
latch (e.g., a state map keyed by userData.id or accountId) inside useOnboarding
(lookup on mount and set when complete() is called) or perform an optimistic
update to userData via useOnboardingPersist() so subsequent userData emissions
don't reopen the wizard; update the useEffect that reads
userData/onboarding_status to consult this local latch before calling setIsOpen
and ensure complete() writes to the latch and/or updates userData
optimistically.
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.
Automated PR from coding agent.
Summary by CodeRabbit
New Features
Chores