Uh oh!
There was an error while loading. Please reload this page.
refactor(ui): extract the shared hero-card surface (#288) - #489
Conversation
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:23 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 (7)
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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 8e6d2b4 | Commit Preview URL Branch Preview URL | Jul 31 2026, 03:17 AM |
f8eb2ac to
41d748dCompareThe warm gradient behind the sign-in and beta modals was inlined as a literal at five sites, and had already drifted at four of them: three different shadow alphas (0.12 / 0.15 / 0.18), two radii (20 / 24), and one card missing the inset highlight entirely. Meanwhile --surface-hero and --surface-hero-shadow sat in globals.css with zero consumers. Adds `.card--hero` (a named variant of .card, used as `card card--hero`) and `.hero-surface` (the gradient alone, for panels nested inside a hero card that must not restate its border, radius or shadow), plus a thin <HeroCard> wrapper that owns those classes and forwards everything else. Both classes are now the tokens' only consumers. This is a VISIBLE change, not a pure refactor, and an intended one: adopting the token moves the shadow hue from cool slate rgba(15,23,42) to the warm rgba(19,38,16) the rest of the app uses, unifies the alphas at 0.12, and gives the beta success modal the 24px radius and inset highlight its two siblings already had. The literal fallbacks in both CSS rules are deliberate. The tokens are scoped to `.public-surface, .landing-page`; every consumer sits inside that subtree today (verified — SignInModal is mounted only from the landing page, and nothing here portals to document.body), so the fallback is defensive rather than load-bearing. For a shared component it is the cheap guard against the first mount that isn't. data-testid="signin-modal" and "signin-close" are unchanged. Deliberately NOT folded in: the close button and the logo/wordmark row. The close buttons are not actually duplicated verbatim — they differ in offset (18 vs 14) and only one carries a testid — and the wordmark appears at 20 sites repo-wide with no shared component, so extracting it for 2 of them would leave a half-migration. #111 touches every icon site anyway; that is the coherent place for it. part of #288 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Tick [P1-E] in the token-unification follow-ups. This PR is what completes it, and the repo's convention is to fold that tick into the same PR — #486 existed purely to backfill a tick missed the same way. Name the shadow hue change for what it is. The comments described the adoption as de-drifting "three shadow alphas, two radii, one missing inset", which is accurate but incomplete in a misleading direction: the shadow's base COLOUR was the one value that had not drifted — all five sites agreed on slate rgba(15,23,42). The token is rgba(19,38,16) = --sap-900, the base the app's other shadows use, so adopting it re-tints these shadows rather than reconciling them. Deliberate, and called out in the PR body, but a reader of the code would have assumed the colour was untouched. Now stated in globals.css and HeroCard.tsx. part of #288 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
41d748d to
72697d6Compare…cturally (#288) The review-fix commit closed the hero-surface block comment early: the `*/` that used to end the whole comment stayed put while a new paragraph was inserted above it, so the rest of the original comment became raw stylesheet text. postcss reported it as "Unclosed string" at line 1007 — 780 lines below the actual mistake, at the first quote it happened to reach. It got that far because nothing in the fast lane reads this file as CSS. eslint lints JS/TS, tsc checks types, vitest never imports the stylesheet; CI's `lint + tsc + vitest` job went green on the broken file. The first thing that actually parses globals.css is the Next production build, which is why the local e2e cycle caught it and everything cheaper did not. Adds src/app/globals.test.ts: a dependency-free structural scan (comments, strings, brace depth) that fails on exactly this class of breakage, in the fast lane, pointing at the right line. Hand-rolled rather than importing postcss on purpose — postcss is only a transitive dependency here and the installed copy already drifts from CI's. It also tests itself: a second case feeds it each broken shape to prove it does not silently return "fine". part of #288 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230
commented
Jul 31, 2026
Local e2e cycle — green (and it caught a real break)Final cycle at Gates: Worth flagging: the first cycle failed, and no cheaper gate could have caught itThe review-fix commit closed the hero-surface block comment early — the original 780 lines below the actual mistake, at the first quote postcss happened to reach. CI's So this PR also adds Rebase noteRebased onto |
Uh oh!
There was an error while loading. Please reload this page.
Jose-Gael-Cruz-Lopez
commented
Jul 31, 2026
Automated review pass (Claude Code), originally run against Still outstanding
Minor
Note for future edits: Verdict: ready to merge once the screenshots are posted (or the body reference removed). |
The warm gradient behind the sign-in and beta modals was inlined as a literal at five sites, and had already drifted at four of them.
--surface-hero/--surface-hero-shadowsat inglobals.csswith zero consumers.page.tsx:756rgba(15,23,42,0.15), no insetpage.tsx:785rgba(15,23,42,0.12)+ insetpage.tsx:819page.tsx:871SignInModal.tsx:226rgba(15,23,42,0.18)+ insetWhat replaced it
.card--hero— a named variant of.card, used asclass="card card--hero"(declared after.cardso it wins)..hero-surface— the gradient alone, for the two inner panels, which must not restate the parent's border/radius/shadow.<HeroCard>— a thin wrapper that owns those two classes and forwards ref, className and every other prop. Callers keep their own layout and dialog semantics.Both classes are now the tokens' only consumers.
This is a visible change, and an intended one
Adopting the token moves the shadow hue from cool slate
rgba(15,23,42)to the warmrgba(19,38,16)the rest of the app uses, unifies the alphas at0.12, and gives the beta success modal the 24px radius and inset highlight its two siblings already had. Before/after screenshots in a comment below.On the literal fallbacks
var(--surface-hero, <literal>)in both rules is deliberate. The tokens are scoped to.public-surface, .landing-page. I verified every consumer sits inside that subtree today —SignInModalis mounted only from the landing page, and nothing here portals todocument.body— so the fallback is defensive, not load-bearing. For a shared component it is the cheap guard against the first mount that isn't.Deliberately not folded in
The close button and the logo/wordmark row. The close buttons are not duplicated verbatim after all — they differ in offset (18 vs 14) and only one carries a testid. The wordmark appears at 20 sites repo-wide with no shared component, so extracting it for 2 of them would leave a half-migration; #111 touches every icon site anyway, which is the coherent place for it.
Tests
HeroCard.test.tsxpins the class contract and prop/ref forwarding, and adds two anti-drift guards: no inlined copy of the gradient may exist anywhere insrc/, and both CSS rules must keep a literal fallback (including the inset half, or token and fallback would render as two different surfaces).data-testid="signin-modal"and"signin-close"are unchanged; no testid surface was added, so neither half of the registry needed updating.Gates
tsc --noEmit— cleannpm run lint— 0 errors (36 pre-existing warnings)npx vitest run— 56 files, 403 tests passedpart of #288