Uh oh!
There was an error while loading. Please reload this page.
feat(ui): re-home onboarding onto app spacing/type tokens (#289) - #497
Conversation
Onboarding was color-correct but structurally orphaned: 12 hardcoded px values, a bespoke type ramp (32 on welcome, 26 everywhere else — matching neither TopBar's 30 nor Dashboard's 42), and a centred card floating in its own radial void. The flow and steps are untouched; only the frame changed. Introduces the --fs-* type scale (option (b)). It is DERIVED, not invented: every step is a size the app already uses, counted across screens/ and components/ — 13px is the true body size at 107 call sites, 12/11/10 carry chrome and micro-labels, 14-16 body copy, 18-26 headings, 30 is TopBar's screen title, 32 the Dashboard display numeral. Deliberately NOT density-aware, unlike --pad-*. Retuning the whole type ramp with the density preference changes how much text fits on every screen — a real product decision, not a refactor. Additive to do later; shipping it now would hide a behaviour change inside a token introduction. Onboarding is the scale's first and only consumer. The rest of the app keeps its inline sizes until each screen is converted deliberately — a token set with zero consumers is exactly the state #288 just finished cleaning up. Also: every hardcoded padding becomes --pad-* (so onboarding finally responds to the density preference), the radial void becomes the app's own --bg, and minHeight moves 100vh -> 100dvh — the same iOS Safari lesson ShellFrame learned in #331, which onboarding never got because it renders outside (shell). One visible change worth calling out: step headings move 26 -> 30 to match TopBar. That is the alignment the issue asks for, not a side effect. Adds the first e2e journey over /onboarding. There was none — notable, since it is the first screen a newly-approved student sees and it renders bare, outside (shell), with no ShellFrame, nav or <main> padding to inherit; nothing in the suite would have noticed it breaking. It signs in as USER_NEW (the only seeded user with onboarding_completed=False) and asserts the card actually occupies the screen with resolved padding — a mistyped var() computes to 0px and silently collapses the box, which no unit test would catch. That made onboarding a driven E2E surface, so it joins the testid enforcement list, and its ten controls are tagged and registered in docs/frontend-testids.md — both halves. part of #289 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 25ab839 | Commit Preview URL Branch Preview URL | Jul 31 2026, 08:12 AM |
Warning Review limit reached
Next review available in:45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
Unique testids for repeated elements. Six of the ten testids sat on elements
that render simultaneously — two TextInputs side by side in StepName, two
TagInputs (majors + minors) each with their own add button and chips, five
learning-style radios, N course results — so getByTestId could not
disambiguate them. docs/frontend-testids.md's "Repeated / list items" rule
already required a stable suffix and the codebase already had the precedent
(upload-modal-course-result-${c.id}); I missed both. TagInput now takes a
`field` prop for the same reason. eslint only checks that a data-testid is
PRESENT, not that it is unique, which is why lint stayed green.
Restored two-value paddings. Collapsing "40px 36px" to a single --pad-xl and
"14px 12px" to a single --pad-md threw away deliberate vertical/horizontal
asymmetry on the card every step renders inside. Both are two-value again.
The remaining value shifts are inherent to adopting a quantised scale and are
listed in the PR body rather than left to be discovered.
Corrected a comment that asserted something I had not verified. The USER_NEW
docstring claimed "approved so the middleware lets them past the pending
gate" — but /onboarding is not in middleware.ts's PROTECTED list or its
matcher, and Onboarding.tsx only redirects when unauthenticated, so the route
renders for any signed-in user regardless of approval or onboarding state.
USER_NEW is the right choice because it is the realistic first-run state, not
because the route gates on it. Both the stack.ts comment and the spec
docstring now say that.
part of #289
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>AndresL230
commented
Jul 31, 2026
Review found four things — all fixed in |
| was | now | delta |
|---|---|---|
card 40px 36px | --pad-xl --pad-lg = 32px 22px | −8 / −14 |
page 40px 20px | --pad-xl --pad-lg = 32px 22px | −8 / +2 |
inputs 10px 12px | --pad-sm --pad-md = 10px 16px | 0 / +4 |
loading rows 14px 12px | --pad-md --pad-sm = 16px 10px | +2 / −2 |
Type is exact except the intentional 26 → 30 on step headings.
E2E
36 passed (1.3m) # 35 existing + the new onboarding journey
0 finding(s)
Gates: tsc clean · lint 0 errors · vitest 58 files / 415 tests.
Onboarding was color-correct but structurally orphaned: 12 hardcoded px values, a bespoke type ramp (32 on welcome, 26 everywhere else — matching neither TopBar's 30 nor Dashboard's 42), and a centred card floating in its own radial void. The flow and steps are untouched; only the frame changed.
The
--fs-*scale — option (b), as you choseDerived, not invented. Every step is a size the app already uses, counted across
screens/andcomponents/:--fs-md: 13px--fs-sm/xs/2xs--fs-base/lg--fs-xl/2xl/3xl--fs-4xl: 30px--fs-5xl: 32pxDeliberately not density-aware, unlike
--pad-*. Retuning the whole type ramp with the density preference changes how much text fits on every screen — that's a product decision, not a refactor. Additive to add later; doing it now would hide a behaviour change inside a token introduction.Onboarding is the scale's first and only consumer. The rest of the app keeps its inline sizes until each screen is converted deliberately — a token set with zero consumers is exactly the state #288 just finished cleaning up, and I'd rather not recreate it.
The rest of the frame
--pad-*, so onboarding finally responds to the density preference at all.--bg.minHeight: 100vh→100dvh— the same iOS Safari lesson ShellFrame learned in Chat/Tutor: top nav bar scrolls out of view in horizontal-nav mode #331, which onboarding never got because it renders outside(shell).One visible change worth calling out: step headings move 26 → 30 to match TopBar. That's the alignment the issue asks for, not a side effect.
The journey that didn't exist
There was no e2e journey over
/onboardingat all — notable, since it's the first screen a newly-approved student sees and it renders bare, with no ShellFrame, nav or<main>padding to inherit. Nothing in the suite would have noticed it breaking.Added one. It signs in as
USER_NEW(the only seeded user withonboarding_completed=False, so the only one the funnel renders for) and asserts the card actually occupies the screen with resolved padding — a mistypedvar()computes to0pxand silently collapses the box, which no unit test would catch and which is the specific risk this PR's conversion introduces.It's a smoke-plus-layout journey rather than a five-step walkthrough on purpose: the field validation is unit-tested, and a long scripted click-path over a funnel that's still being redesigned would break on every copy tweak.
Testids — both halves
Driving onboarding made it an E2E surface, so
Onboarding.tsxjoins the enforcement list ineslint.config.mjs. That immediately flagged 7 untagged controls (the rule doing its job), so all ten are now tagged and registered indocs/frontend-testids.md— surface table and inventory.Not done
The proposal also suggested rendering onboarding in the app's left-aligned frame with shell-like chrome. I've left the centred card: moving the first-run funnel from centred-card to left-aligned-shell changes its whole character, and that's a taste call rather than debt cleanup. Happy to do it if you want it.
Gates
tsc --noEmitclean ·npm run lint0 errors ·npx vitest run58 files / 415 testspart of #289