feat: redesign new user onboarding flow#14
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| if (!session?.user?.email) { | ||
| return; | ||
| } | ||
| if (profile?.cubid_id || hasRequestedCubidId.current) { | ||
| return; | ||
| } | ||
|
|
||
| hasRequestedCubidId.current = true; | ||
| requestCubidId(session.user.email) | ||
| .then((cubid) => { | ||
| setForm((prev) => ({ ...prev, cubidId: cubid })); | ||
| setStatus("Cubid ID prepared"); |
There was a problem hiding this comment.
Avoid generating Cubid ID before profile loads
The effect that auto-generates a Cubid ID runs as soon as a session email is present, regardless of whether the user profile has finished loading. AuthProvider sets session before completing the profile fetch, so there is a render where session exists but profile is still null. In that window the effect calls requestCubidId and later overwrites the form state with the newly generated ID (setForm…cubidId: cubid). If the subsequently fetched profile already has a cubid_id, the promise resolution replaces it and the user may submit with a different ID than the one stored on the server, unintentionally changing their identifier. The effect should wait until the profile is known to be missing an ID or cancel pending updates once the profile arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the new user onboarding flow into a multi-step wizard (name → photo → wallet) with enhanced photo upload capabilities and automatic Cubid ID generation. The changes improve user experience by breaking down the onboarding process into digestible steps while adding support for both file uploads and URL-based photo imports.
Key Changes:
- Implemented a 3-step onboarding wizard with step indicators and navigation
- Added photo upload functionality supporting both file uploads and URL imports
- Automated Cubid ID generation on component mount
- Added test infrastructure for jsqr library mocking
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/src/app/new-user/page.tsx |
Major refactoring to implement multi-step onboarding flow with photo upload, URL cleanup, and automatic Cubid ID generation |
frontend/__tests__/new-user-page.test.tsx |
New test file covering the happy path through all three onboarding steps |
frontend/__mocks__/jsqr.ts |
Mock implementation for jsqr library to support testing |
frontend/vitest.setup.ts |
Added jsqr mock setup using vi.mock() |
frontend/vitest.config.ts |
Added alias configuration to redirect jsqr imports to mock file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/app/new-user/page.tsx
Outdated
| await uploadPhotoFromLinkOrFile(); | ||
| setStep(2); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : "We couldn't save your photo"; |
There was a problem hiding this comment.
Error message should not contain HTML entities like '. These entities are meant for JSX rendering and will be displayed literally in error messages shown to users. Use a regular apostrophe instead: "We couldn't save your photo"
| const message = err instanceof Error ? err.message : "We couldn't save your photo"; | |
| const message = err instanceof Error ? err.message : "We couldn't save your photo"; |
frontend/src/app/new-user/page.tsx
Outdated
| const extension = inferExtensionFromSource(photoLink, contentType); | ||
| const blob = await response.blob(); | ||
| fileToUpload = new File([blob], `linked.${extension}`, { type: contentType }); | ||
| updatePhotoPreview(URL.createObjectURL(blob), true); |
There was a problem hiding this comment.
Creating a preview object URL for the fetched blob is unnecessary here since the photo is being uploaded immediately afterward and won't be displayed before the upload completes. This creates an object URL that will be revoked shortly after, adding no value. Consider removing this line to simplify the code flow.
| updatePhotoPreview(URL.createObjectURL(blob), true); |
frontend/src/app/new-user/page.tsx
Outdated
| <div className="flex gap-2 text-xs uppercase tracking-widest text-muted-foreground"> | ||
| <span className={step === 0 ? "font-semibold text-blue-600" : ""}>Name</span> | ||
| <span>›</span> | ||
| <span className={step === 1 ? "font-semibold text-blue-600" : step > 1 ? "text-blue-600" : ""}>Photo</span> | ||
| <span>›</span> | ||
| <span className={step === 2 ? "font-semibold text-blue-600" : ""}>Wallet</span> | ||
| </div> |
There was a problem hiding this comment.
The step indicator (Name › Photo › Wallet) lacks semantic structure and ARIA attributes for screen readers. Users with assistive technology won't know which step is current or how many total steps exist. Consider wrapping this in a <nav> element with aria-label="Onboarding progress" and adding aria-current="step" to the current step indicator.
| <div className="flex gap-2 text-xs uppercase tracking-widest text-muted-foreground"> | |
| <span className={step === 0 ? "font-semibold text-blue-600" : ""}>Name</span> | |
| <span>›</span> | |
| <span className={step === 1 ? "font-semibold text-blue-600" : step > 1 ? "text-blue-600" : ""}>Photo</span> | |
| <span>›</span> | |
| <span className={step === 2 ? "font-semibold text-blue-600" : ""}>Wallet</span> | |
| </div> | |
| <nav aria-label="Onboarding progress"> | |
| <ol className="flex gap-2 text-xs uppercase tracking-widest text-muted-foreground list-none p-0 m-0"> | |
| <li> | |
| <span | |
| className={step === 0 ? "font-semibold text-blue-600" : ""} | |
| {...(step === 0 ? { "aria-current": "step" } : {})} | |
| > | |
| Name | |
| </span> | |
| </li> | |
| <li aria-hidden="true">›</li> | |
| <li> | |
| <span | |
| className={step === 1 ? "font-semibold text-blue-600" : step > 1 ? "text-blue-600" : ""} | |
| {...(step === 1 ? { "aria-current": "step" } : {})} | |
| > | |
| Photo | |
| </span> | |
| </li> | |
| <li aria-hidden="true">›</li> | |
| <li> | |
| <span | |
| className={step === 2 ? "font-semibold text-blue-600" : ""} | |
| {...(step === 2 ? { "aria-current": "step" } : {})} | |
| > | |
| Wallet | |
| </span> | |
| </li> | |
| </ol> | |
| </nav> |
| <button | ||
| className="rounded-full border border-neutral-300 px-8 py-3 text-lg font-semibold transition hover:bg-neutral-100 dark:border-neutral-700 dark:hover:bg-neutral-900" | ||
| onClick={() => { | ||
| setStatus(null); |
There was a problem hiding this comment.
When navigating back, only status is cleared but not error. This is inconsistent with the "Back" button on step 1 (line 332-334) which also only clears status. For better user experience, both error and status messages should be cleared when navigating between steps to avoid showing stale messages from previous steps. Consider adding setError(null) here and at line 333.
| setStatus(null); | |
| setStatus(null); | |
| setError(null); |
frontend/src/app/new-user/page.tsx
Outdated
| } else if (photoLink) { | ||
| const response = await fetch(photoLink); | ||
| if (!response.ok) { | ||
| throw new Error("We couldn't fetch that image link"); |
There was a problem hiding this comment.
Error messages should not contain HTML entities like '. These entities are meant for JSX rendering and will be displayed literally in error messages shown to users. Use regular apostrophes instead:
"We couldn't fetch that image link""We couldn't save your photo"
| throw new Error("We couldn't fetch that image link"); | |
| throw new Error("We couldn't fetch that image link"); |
frontend/vitest.setup.ts
Outdated
|
|
||
| vi.mock("jsqr", () => ({ | ||
| default: vi.fn(), | ||
| })); |
There was a problem hiding this comment.
The jsqr library is being mocked in two different ways: here in vitest.setup.ts using vi.mock() and in vitest.config.ts using an alias to __mocks__/jsqr.ts. This dual approach is redundant and could lead to confusion about which mock is being used. Consider removing one of these mocking strategies for clarity. Since the alias approach in vitest.config.ts points to an actual mock file, the vi.mock() call here may not be necessary.
| vi.mock("jsqr", () => ({ | |
| default: vi.fn(), | |
| })); |
| const globalUrl = globalThis.URL as unknown as Record<string, unknown>; | ||
| globalUrl.createObjectURL = createObjectURLMock; | ||
| globalUrl.revokeObjectURL = revokeObjectURLMock; |
There was a problem hiding this comment.
In the afterEach hook, URL.createObjectURL and URL.revokeObjectURL are being set back to the mock functions instead of restoring them to their original values. This is likely incorrect - they should be restored to their original implementations before the test started. Consider storing the original implementations in beforeEach and restoring them in afterEach, similar to how originalFetch is handled.
frontend/src/app/new-user/page.tsx
Outdated
| if (urlExtension && /^[a-z0-9]+$/i.test(urlExtension)) { | ||
| return urlExtension; | ||
| } | ||
| const mimeExtension = contentType.split("/")[1]; |
There was a problem hiding this comment.
The MIME type extension parsing is incomplete. For content types like image/jpeg, this will correctly extract jpeg, but for more complex MIME types with parameters (e.g., image/svg+xml; charset=utf-8), it will return svg+xml; charset=utf-8. Consider using contentType.split("/")[1]?.split(";")[0]?.trim() to properly extract the MIME subtype.
| const mimeExtension = contentType.split("/")[1]; | |
| const mimeExtension = contentType.split("/")[1]?.split(";")[0]?.trim(); |
frontend/src/app/new-user/page.tsx
Outdated
| setUser(updated); | ||
| setWalletAddress(address); | ||
| setStatus("Wallet linked"); | ||
| setStep(2); |
There was a problem hiding this comment.
This setStep(2) call is redundant because the user is already on step 2 when they click the "Connect wallet" button. The wallet connection button is only visible when step === 2, so setting the step to 2 again has no effect. This line can be safely removed.
| setStep(2); |
Summary
Testing
Codex Task