Skip to content

feat(admin): migrate admin surfaces onto role tokens - #373

Open
chiptus wants to merge 9 commits into
mainfrom
claude/implement-367-zrn4kp
Open

feat(admin): migrate admin surfaces onto role tokens#373
chiptus wants to merge 9 commits into
mainfrom
claude/implement-367-zrn4kp

Conversation

@chiptus

@chiptus chiptus commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Moves role-token CSS variables from .edition-view scope onto global :root so the whole app shares one token set, instead of admin/portal surfaces silently keeping old shadcn light-theme defaults for the same variable names.
Introduces surface-foreground/surface-muted-foreground/surface-border tokens for white-background surfaces (dialogs, toasts, admin tables, inputs, OTP) so they no longer inherit the dark-identity foreground/muted-foreground/ring values.

Verification

  • Open the login dialog: title/description text is dark, email input border/focus ring are neutral (not purple).
  • Request an OTP and enter it: the 6-digit slots are legible, active slot shows the purple focus ring, caret is visible.
  • Trigger a toast notification: title/description text is legible on the white toast background.
  • Open the admin Artists table: header text, "Click to add…"/"add image…" placeholders, and cell text are all legible.
  • Confirm voter-facing EditionView pages still render the dark purple identity unchanged.

claude added 2 commits August 23, 2026 06:45
Scopes the admin layout root with EditionViewRoot and migrates the
non-portal hardcoded color sites (text-white, bg-white/10,
border-white/20, bg-purple-600) onto the role tokens from #363, with
no visual change since each token carries today's exact value.
Colors inside Radix Dialog/AlertDialog portals are left as-is since
those portals render outside the edition-view CSS variable scope.
…active

Code review flagged reusing the surface-active fill token as a border
color as crossing the vocabulary doc's role taxonomy, even though the
value matched. No border token covers this exact value yet, so leave
it literal, matching the untouched border-white/10 in the same file.
Copilot AI lite review requested due to automatic review settings August 23, 2026 06:49
@vercel

vercel Bot commented Aug 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
upline Ready Ready Preview Aug 23, 2026 9:55am

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Moderate token-migration gaps remain in hover and border colors, accent foregrounds, and an in-scope LinkWizard color.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Migrates admin surfaces to edition role tokens under EditionViewRoot while preserving current styling.

Changes:

  • Scopes admin content with EditionViewRoot.
  • Replaces selected hardcoded colors with semantic tokens.
  • Updates analytics, navigation, CTA, and archive styling.
File summaries
File Summary
src/routes/admin/festivals/$festivalSlug/editions/$editionSlug/sets.tsx Tokenizes the Add Set CTA; hardcoded hover color remains.
src/routes/admin/festivals/$festivalSlug/editions/$editionSlug.tsx Tokenizes the edition sub-navigation surface.
src/routes/admin/analytics.tsx Tokenizes analytics surfaces and text; hardcoded borders remain.
src/routes/admin.tsx Adds edition scope and tokenized tabs; active foreground and LinkWizard colors need updates.
src/pages/admin/festivals/StageManagement/CreateStageDialog.tsx Tokenizes the CTA; hardcoded hover color remains.
src/pages/admin/festivals/FestivalManagementSection.tsx Tokenizes the festival CTA; hardcoded hover color remains.
src/pages/admin/festivals/FestivalEditionManagement.tsx Tokenizes the edition CTA; hardcoded hover color remains.
src/pages/admin/festivals/EditionNavLink.tsx Tokenizes navigation states; active accent foreground needs correction.
src/pages/admin/ArtistsManagement/components/BulkEditorHeader.tsx Tokenizes the artist CTA; hardcoded hover color remains.
src/pages/admin/ArtistsManagement/ArchiveArtistDialog.tsx Updates archive hover text styling.
Review details

Suppressed comments (6)

src/pages/admin/ArtistsManagement/ArchiveArtistDialog.tsx:52

  • This replacement uses the page foreground role for text rendered over the destructive red hover fill. That is only correct while both happen to be white; when the edition theme changes foreground, the destructive action can get the wrong contrast. Use the destructive foreground role for text on this fill (and keep the fill on its semantic destructive token).
      className="border-red-400 text-red-400 hover:bg-red-400 hover:text-foreground"

src/pages/admin/festivals/FestivalManagementSection.tsx:38

  • This CTA now gets bg-accent, but Button's default variant still supplies text-primary-foreground; no text-accent-foreground is applied. That is near-white today, but the identity flip changes the dark-theme accent to lime with dark accent-foreground while the global primary foreground remains light, making the label fail the intended contrast. Add the paired accent foreground role to this CTA.
              className="bg-accent hover:bg-purple-700"

src/pages/admin/festivals/StageManagement/CreateStageDialog.tsx:42

  • Only the resting background is tokenized here. This Button still inherits text-primary-foreground, and hover:bg-purple-700 remains a literal; the palette flip changes the scoped accent/accent-foreground but not primary-foreground, so this CTA can have the wrong text contrast and revert to purple on hover. Use text-accent-foreground and a role token for the hover state, consistently with the other converted CTAs.
        <Button className="bg-accent hover:bg-purple-700">

src/pages/admin/festivals/StageManagement/CreateStageDialog.tsx:42

  • This CTA now gets bg-accent, but Button's default variant still supplies text-primary-foreground; no text-accent-foreground is applied. That is near-white today, but the identity flip changes the dark-theme accent to lime with dark accent-foreground while the global primary foreground remains light, making the label fail the intended contrast. Add the paired accent foreground role to this CTA.
        <Button className="bg-accent hover:bg-purple-700">

src/routes/admin.tsx:89

  • Applying the .edition-view scope to the entire admin tree also overrides shared variables such as --muted-foreground (purple-200), --border (purple-400/30), and --ring for existing shadcn components. Most admin cards still use the default white bg-card, while many labels and empty states still use text-muted-foreground (for example BulkEditorFooter), so they now become light purple on white and card borders/focus rings change. This contradicts the claimed zero visual change; scope only fully migrated surfaces or update the remaining admin components/variables before applying this wrapper.
    <EditionViewRoot>

src/routes/admin/festivals/$festivalSlug/editions/$editionSlug/sets.tsx:95

  • This CTA now gets bg-accent, but Button's default variant still supplies text-primary-foreground; no text-accent-foreground is applied. That is near-white today, but the identity flip changes the dark-theme accent to lime with dark accent-foreground while the global primary foreground remains light, making the label fail the intended contrast. Add the paired accent foreground role to this CTA.
              className="bg-accent hover:bg-purple-700"
  • Files reviewed: 10/10 changed files
  • Comments generated: 9
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/pages/admin/ArtistsManagement/components/BulkEditorHeader.tsx Outdated
Comment thread src/pages/admin/festivals/EditionNavLink.tsx Outdated
Comment thread src/pages/admin/festivals/FestivalEditionManagement.tsx Outdated
Comment thread src/pages/admin/festivals/FestivalManagementSection.tsx Outdated
Comment thread src/pages/admin/festivals/StageManagement/CreateStageDialog.tsx Outdated
Comment thread src/routes/admin.tsx Outdated
Comment thread src/routes/admin.tsx Outdated
Comment thread src/routes/admin/analytics.tsx Outdated
Comment thread src/routes/admin/festivals/$festivalSlug/editions/$editionSlug/sets.tsx Outdated
@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown

Playwright test results

passed  52 passed

Details

stats  52 tests across 15 suites
duration  53 seconds
commit  297985e

…t states

Per review feedback: moves the edition-view role tokens from the
.edition-view scope class onto :root so the whole app shares one set
of values instead of admin (or any other unmigrated surface) keeping
stale shadcn defaults for the same variable names — this also fixes
Radix portal content never seeing the override, since :root is the
ultimate ancestor of a portal too. Adds a real accent-hover token
(purple-700) so CTAs stop mixing a token fill with a literal hover
color, pairs every bg-accent use with text-accent-foreground, and
migrates the remaining non-portal literals flagged in review
(analytics.tsx borders via border-foreground/NN, LinkWizardTable via
bg-accent/20) instead of leaving them hardcoded.
| ------------------------------ | ------------------------------------------------ | -------------------------------------------------- |
| `accent` / `accent-foreground` | Primary interactive fill: selected toggles, CTAs | `bg-purple-600`, `bg-purple-700`, `bg-purple-400` |
| `accent` / `accent-foreground` | Primary interactive fill: selected toggles, CTAs | `bg-purple-600`, `bg-purple-400` |
| `accent-hover` **(new)** | Hover state for the accent fill | `hover:bg-purple-700` |

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `accent-hover` **(new)** | Hover state for the accent fill | `hover:bg-purple-700` |
| `accent-hover` | Hover state for the accent fill | `hover:bg-purple-700` |

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Three moderate findings remain regarding token scope, theme selector alignment, and archive hover color.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

docs/design/edition-color-vocabulary.md:12

  • This new global-token contract conflicts with the canonical identity spec: docs/design/edition-identity-spec.md:5 still limits the identity to voter-facing edition views and explicitly keeps admin, groups/settings, landing, and auth on the old skin, while this line makes the same roles app-global. Update the canonical spec and the zero-change scope together with the implementation, or retain the scoped model.
- **The tokens live on `:root`, not a scope class (#367).** They used to sit under a `.edition-view` class so only voter-facing pages picked them up, but that meant every other surface (admin included) kept the old shadcn light-theme defaults for the same variable names — so a partial migration elsewhere (like admin's) recolored whatever hadn't been touched yet, and Radix portal content (which renders under `document.body`, outside any wrapper's scope) never picked up the override at all. The whole app renders on the same dark identity today, so there's one set of values for the whole app and no scope class to remember. `EditionViewRoot` still supplies the `bg-app-gradient` ground and is where the flip (#359) will attach `data-edition-theme="light"`.

src/index.css:32

  • Changing --accent at :root also recolors generic shadcn controls outside the migrated surfaces: Button outline/ghost variants and /terms's ghost back button use hover:bg-accent, which was neutral under the old global value but is now purple. The identity spec still says landing/auth/groups/settings keep the old skin, so this violates the zero-visual-change claim and mixes the edition role with the old global meaning. Keep a separate scope/token or update those consumers and the contract before globalizing it.
    --accent: 271.5 81.3% 55.9%; /* purple-600 */
  • Files reviewed: 15/15 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread src/index.css Outdated
Comment on lines +137 to +140
/* Light-theme stub per the CSS contract in the identity spec: light values
apply under an explicit data-edition-theme="light" on the document root.
Values land with the flip (#359). */
:root[data-edition-theme="light"] {
Comment thread src/index.css Outdated
Comment on lines +124 to +128
These used to live under a `.edition-view` scope class so only
voter-facing pages picked them up, but that meant every other surface
(admin included) still carried the old shadcn light-theme defaults for
the same variable names, which made partial migrations (like #367's
admin sweep) recolor whatever hadn't been touched yet. The whole app
variant="outline"
size="sm"
className="border-red-400 text-red-400 hover:bg-red-400 hover:text-white"
className="border-red-400 text-red-400 hover:bg-red-400 hover:text-destructive-foreground"
Comment thread docs/design/edition-color-vocabulary.md Outdated
- **No numeric ramps.** Each role is one value per theme; "lighter/darker" variants are expressed as alpha on the base token (Tailwind `/NN` over an HSL var) or as an explicit `-soft` companion, not as `-100…-900` scales.
- **`/NN` opacity modifiers work only on opaque roles** (`foreground`, `accent`, `live`, `vote-must`, …). The inherently translucent roles (`surface`/`surface-raised`/`surface-active`, `accent-soft`, `border`/`border-strong`, `*-soft`) carry their alpha inside the CSS variable, so `bg-surface/50` expands to a double-alpha `hsl(… / 0.05 / .5)` — silently invalid CSS. Use the next step in the role family (`surface` → `surface-raised` → `surface-active`) instead of a modifier.
- **Portals escape the scope.** The variables live on the edition root (`EditionViewRoot`), but Radix `Select`/`Popover`/`Sheet` content portals under `document.body` and resolves the global values there. Before tokenizing portaled content, give it the scope (portal `container` inside the edition root, or the scope class on the content element).
- **The tokens live on `:root`, not a scope class (#367).** They used to sit under a `.edition-view` class so only voter-facing pages picked them up, but that meant every other surface (admin included) kept the old shadcn light-theme defaults for the same variable names — so a partial migration elsewhere (like admin's) recolored whatever hadn't been touched yet, and Radix portal content (which renders under `document.body`, outside any wrapper's scope) never picked up the override at all. The whole app renders on the same dark identity today, so there's one set of values for the whole app and no scope class to remember. `EditionViewRoot` still supplies the `bg-app-gradient` ground and is where the flip (#359) will attach `data-edition-theme="light"`.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably not needed

EditionViewRoot's only remaining job was bg-app-gradient — the role
tokens live on :root now, so admin doesn't need the wrapper.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01546kmFCrpafJ2ykLQKjYvb

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two unresolved moderate issues in src/index.css affect dialog contrast and future theme selection.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (2) — in code that hasn't changed since the last review.

src/index.css:10

  • Moving --foreground to :root makes it white for every portal, but DialogContent and AlertDialogContent still render bg-background, whose root value remains white (src/components/ui/dialog.tsx:39). Their inherited titles/body text can therefore become white on white, so this is not a zero-visual-change migration for dialogs. Keep these overrides scoped or give the shared portal primitives an explicitly compatible foreground/background before globalizing the tokens.
    --foreground: 0 0% 100%; /* text-white */

src/index.css:118

  • EditionViewRoot renders the future data-edition-theme attribute on its wrapper <div>, but :root[...] matches only the document root (<html>). When #359 adds the attribute to EditionViewRoot as documented in EditionViewRoot.tsx:7, this block will never match and the light palette will not apply. Keep the selector on .edition-view or move the attribute to document.documentElement consistently.
  :root[data-edition-theme="light"] {

src/index.css:33

  • --accent-foreground is pure white, but these five migrated Add/Create buttons did not previously use text-white; Button's default variant supplied text-primary-foreground (210 40% 98%). Adding text-accent-foreground therefore changes their label color instead of preserving it, contrary to the zero-visual-change claim. Keep text-primary-foreground at these call sites or choose a token with the former value.
    --accent-foreground: 0 0% 100%;

src/index.css:16

  • These popover values now apply to every Radix popover/dropdown portal because those contents render under document.body and consume bg-popover text-popover-foreground (for example src/components/ui/popover.tsx:14-20). Before this change the global values were white/dark; they are now gray-800/purple-100, so portal content changes visually despite the PR's stated portal exception and zero-visual-change goal. Keep the edition values scoped or explicitly opt only the intended portals into them.
    --popover: 215 27.9% 16.9%; /* gray-800 */
    --popover-foreground: 268.7 100% 95.5%; /* purple-100 */

src/pages/admin/ArtistsManagement/ArchiveArtistDialog.tsx:52

  • text-destructive-foreground is not equivalent to the replaced text-white: --destructive-foreground is 210 40% 98% (#f1f5f9), while the old class is pure white. This changes the hover text color and breaks the stated zero-visual-change guarantee; use a token whose current value is white or deliberately update the destructive foreground token.
      className="border-red-400 text-red-400 hover:bg-red-400 hover:text-destructive-foreground"

src/routes/admin.tsx:80

  • The PR description says the admin layout is scoped with EditionViewRoot, but both the loading branch and the main branch in this file still return plain div roots (the main one is at lines 88-89). That means admin will not receive the edition-view root where the planned data-edition-theme marker is attached, so the future theme flip will skip this surface. Wrap both branches with EditionViewRoot or apply the same scope/data-theme contract directly.
        <div className="text-foreground text-xl">Loading...</div>
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread docs/design/edition-color-vocabulary.md Outdated
- **No numeric ramps.** Each role is one value per theme; "lighter/darker" variants are expressed as alpha on the base token (Tailwind `/NN` over an HSL var) or as an explicit `-soft` companion, not as `-100…-900` scales.
- **`/NN` opacity modifiers work only on opaque roles** (`foreground`, `accent`, `live`, `vote-must`, …). The inherently translucent roles (`surface`/`surface-raised`/`surface-active`, `accent-soft`, `border`/`border-strong`, `*-soft`) carry their alpha inside the CSS variable, so `bg-surface/50` expands to a double-alpha `hsl(… / 0.05 / .5)` — silently invalid CSS. Use the next step in the role family (`surface` → `surface-raised` → `surface-active`) instead of a modifier.
- **Portals escape the scope.** The variables live on the edition root (`EditionViewRoot`), but Radix `Select`/`Popover`/`Sheet` content portals under `document.body` and resolves the global values there. Before tokenizing portaled content, give it the scope (portal `container` inside the edition root, or the scope class on the content element).
- **The tokens live on `:root`, not a scope class (#367).** They used to sit under a `.edition-view` class so only voter-facing pages picked them up, but that meant every other surface (admin included) kept the old shadcn light-theme defaults for the same variable names — so a partial migration elsewhere (like admin's) recolored whatever hadn't been touched yet, and Radix portal content (which renders under `document.body`, outside any wrapper's scope) never picked up the override at all. The whole app renders on the same dark identity today, so there's one set of values for the whole app and no scope class to remember. `EditionViewRoot` still supplies the `bg-app-gradient` ground and is where the flip (#359) will attach `data-edition-theme="light"`.
…:root token migration

The AuthDialog, toast notifications, and admin Artists table (Bulk Editor
and generic Table primitives) render on white backgrounds but were
picking up the dark edition-view identity's now-global muted-foreground/
foreground values, washing out text, placeholders, and the input focus
ring. Add explicit gray/black overrides at these light-surface call
sites and shared table defaults, without reverting the :root migration.

Also removes the now-vestigial EditionViewRoot wrapper (it had shrunk to
a single div once its token-scoping job moved to :root) and the stale
CSS comment block in index.css.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Global token changes cause critical light-surface styling regressions, with additional tokenization and documentation issues unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (8)

Previously missed (1) — in code that hasn't changed since the last review.

src/index.css:32

  • The root token migration still leaves src/pages/admin/festivals/FestivalLogoDialog.tsx:178 on bg-purple-600 hover:bg-purple-700, so the admin Upload Logo CTA is not tokenized and will diverge when --accent/--accent-hover changes. Migrate this remaining admin CTA as well to satisfy the stated admin color-site scope.
    --accent: 271.5 81.3% 55.9%; /* purple-600 */

src/components/ui/table.tsx:76

  • These shared table primitives are being changed from the semantic text-muted-foreground (and inherited cell text) to fixed text-gray-* colors. That bypasses the role tokens introduced here, so table headers/captions/cells will ignore the planned theme flip; the admin-specific neutral override should be applied at the call site instead of changing the shared primitive.
      "h-12 px-4 text-left align-middle font-medium text-gray-500 [&:has([role=checkbox])]:pr-0",

src/components/ui/toaster.tsx:18

  • This only changes the toast root's inherited text color, but ToastClose still explicitly uses text-foreground/50 and hover:text-foreground (src/components/ui/toast.tsx:78). With the new global foreground set to white and the default toast background still bg-background (white), the close control remains white on white and is effectively invisible. Make the close control use the toast's default text color while preserving destructive-toast styling.
          <Toast key={id} className="text-gray-900" {...props}>

src/components/ui/toaster.tsx:24

  • These overrides are unconditional, so they replace the variant colors on destructive toasts. For example, useAddAdminMutation emits variant: "destructive"; text-gray-900 then overrides text-destructive-foreground, and the description's text-gray-500 overrides the inherited destructive color, producing dark text on the red toast. Apply the gray overrides only for the default variant and leave destructive toasts on their variant colors.
          <Toast key={id} className="text-gray-900" {...props}>
            <div className="grid gap-1">
              {title && <ToastTitle>{title}</ToastTitle>}
              {description && (
                <ToastDescription className="text-gray-500">
                  {description}
                </ToastDescription>

src/index.css:25

  • Making --muted-foreground purple-200 global also recolors unchanged admin helper and empty-state text on white cards. For example, FestivalManagementTable and StagesTable still use text-muted-foreground, so their messages become low-contrast purple on white instead of the previous neutral value. Keep the edition value scoped or provide a light-surface/admin override before globalizing it.
    --muted-foreground: 268.6 100% 91.8%; /* purple-200 */

src/index.css:40

  • Moving --border from the old light value to :root recolors every generic border-border consumer, not only the migrated admin literals. Card always renders a border, and admin cards such as FestivalManagementSection use <Card> without a border override, so their outlines change from the previous gray to purple-400/30, contradicting the claimed zero visual change. Keep the role override scoped or preserve the default border outside the intended surfaces.
    --border: 270 95.2% 75.3% / 0.3; /* border-purple-400/30 */

src/index.css:32

  • --accent is also moved to :root with purple-600, but the shared Button primitive's outline and ghost variants use hover:bg-accent/hover:text-accent-foreground (src/components/ui/button.tsx:16-19). Every unconverted admin action button therefore changes from the old neutral hover to purple—for example the table's ghost edit/delete buttons—so this is not zero visual change. Keep this token scoped or update those variants/surfaces as part of the migration.
    --accent: 271.5 81.3% 55.9%; /* purple-600 */

src/pages/admin/ArtistsManagement/ArchiveArtistDialog.tsx:52

  • This replacement is not value-equivalent to the old hover:text-white: --destructive-foreground still resolves to 210 40% 98% (slate-50, approximately #f8fafc), not 0 0% 100%. Either set the role's foreground value to white for this palette or retain the original class if zero visual change is required.
      className="border-red-400 text-red-400 hover:bg-red-400 hover:text-destructive-foreground"
  • Files reviewed: 24/24 changed files
  • Comments generated: 2
  • Review effort level: Lite

- **No numeric ramps.** Each role is one value per theme; "lighter/darker" variants are expressed as alpha on the base token (Tailwind `/NN` over an HSL var) or as an explicit `-soft` companion, not as `-100…-900` scales.
- **`/NN` opacity modifiers work only on opaque roles** (`foreground`, `accent`, `live`, `vote-must`, …). The inherently translucent roles (`surface`/`surface-raised`/`surface-active`, `accent-soft`, `border`/`border-strong`, `*-soft`) carry their alpha inside the CSS variable, so `bg-surface/50` expands to a double-alpha `hsl(… / 0.05 / .5)` — silently invalid CSS. Use the next step in the role family (`surface` → `surface-raised` → `surface-active`) instead of a modifier.
- **Portals escape the scope.** The variables live on the edition root (`EditionViewRoot`), but Radix `Select`/`Popover`/`Sheet` content portals under `document.body` and resolves the global values there. Before tokenizing portaled content, give it the scope (portal `container` inside the edition root, or the scope class on the content element).
- **The tokens live on `:root`, not a scope class (#367).** They used to sit under a `.edition-view` class so only voter-facing pages picked them up, but that meant every other surface (admin included) kept the old shadcn light-theme defaults for the same variable names — so a partial migration elsewhere (like admin's) recolored whatever hadn't been touched yet, and Radix portal content (which renders under `document.body`, outside any wrapper's scope) never picked up the override at all. The whole app renders on the same dark identity today, so there's one set of values for the whole app and no scope class to remember. Each root layout applies `bg-app-gradient` directly; the flip (#359) will attach `data-edition-theme="light"` to the document root.
Comment thread src/index.css
:root {
--background: 0 0% 100%;
--foreground: 222.2 84% 4.9%;
--foreground: 0 0% 100%; /* text-white */
…l grays

Introduces surface-foreground/surface-muted-foreground/surface-border
tokens for white-background surfaces (dialogs, toasts, admin tables),
replacing the hardcoded gray-900/gray-500/gray-300 classes.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Three moderate contrast regressions in toast and global muted-foreground styling remain unresolved.

Review details

Suppressed comments (7)

Previously missed (3) — in code that hasn't changed since the last review.

src/components/ui/toaster.tsx:22

  • text-gray-900 is applied unconditionally after toastVariants. For variant="destructive", the variant supplies text-destructive-foreground on the red background, but cn/twMerge keeps this later text color; the description’s unconditional text-gray-500 has the same problem. Destructive mutation errors (for example useRemoveAdminMutation) will therefore use dark gray text instead of the destructive foreground and can lose contrast. Apply the gray classes only to the default variant, or encode the colors in the toast variants.
          <Toast key={id} className="text-surface-foreground" {...props}>
            <div className="grid gap-1">
              {title && <ToastTitle>{title}</ToastTitle>}
              {description && (
                <ToastDescription className="text-surface-muted-foreground">

src/components/ui/toaster.tsx:18

  • The default toast's close control still uses text-foreground in src/components/ui/toast.tsx, while its background is bg-background; after the root token move these resolve to white on white. The new root text class does not affect that child, so the close button is invisible on hover/focus. Give the close control a foreground appropriate for the light toast or keep this token scoped.
          <Toast key={id} className="text-surface-foreground" {...props}>

src/index.css:25

  • --muted-foreground is now the light purple-200 value globally, but --card remains white and existing admin cards still use CardDescription/text-muted-foreground without a dark surface. For example, the Admin Roles card's description becomes light purple on white instead of the previous slate text, causing a severe contrast regression. Keep this role scoped or update all white-card consumers before globalizing it.
    --muted-foreground: 268.6 100% 91.8%; /* purple-200 */

docs/design/edition-color-vocabulary.md:12

  • This changes the contract to put the tokens and data-edition-theme="light" on :root, but docs/design/edition-identity-spec.md:15 still says the variables and theme attribute are scoped to the .edition-view root (and line 110 directs implementation to use that scope). Its scope statement also still says admin keeps the old skin. These contradictory source documents can cause the #359 flip to attach the attribute to the wrong node, where :root[data-edition-theme="light"] will not match; update the identity spec alongside this contract change.
- **The tokens live on `:root`, not a scope class (#367).** They used to sit under a `.edition-view` class so only voter-facing pages picked them up, but that meant every other surface (admin included) kept the old shadcn light-theme defaults for the same variable names — so a partial migration elsewhere (like admin's) recolored whatever hadn't been touched yet, and Radix portal content (which renders under `document.body`, outside any wrapper's scope) never picked up the override at all. The whole app renders on the same dark identity today, so there's one set of values for the whole app and no scope class to remember. Each root layout applies `bg-app-gradient` directly; the flip (#359) will attach `data-edition-theme="light"` to the document root.

src/index.css:10

  • Moving --foreground to :root while leaving --background at white changes every Radix portal/shared control that uses bg-background without its own text color. DialogContent and AlertDialogContent inherit the body’s new white text-foreground, so admin dialogs such as AddGenreDialog and EditStageDialog render titles, labels, and input values white on the unchanged white background; Sonner toasts have the same problem. This contradicts the claimed zero-visual-change migration. Keep the role overrides scoped and scope admin separately, or update the portal/shared surfaces and their controls together.
    --foreground: 0 0% 100%; /* text-white */

src/index.css:16

  • These root-level popover values are consumed by SelectContent as bg-popover text-popover-foreground, so Radix portal menus are recolored even though the PR says portal colors are out of scope: they change from the old white/dark root values to gray-800/purple-100. For example, the admin role selector will visibly change when opened. Keep these values scoped or explicitly preserve portal defaults.
    --popover: 215 27.9% 16.9%; /* gray-800 */
    --popover-foreground: 268.7 100% 95.5%; /* purple-100 */

src/pages/admin/ArtistsManagement/ArchiveArtistDialog.tsx:52

  • The replacement is not exact for hover:text-white: --destructive-foreground remains 210 40% 98% in index.css, whereas the removed utility was pure white. The hover icon therefore changes color in a PR that promises zero visual change; define the destructive foreground with the required value or use a token whose value is actually white.
      className="border-red-400 text-red-400 hover:bg-red-400 hover:text-destructive-foreground"
  • Files reviewed: 25/25 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings August 23, 2026 08:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Destructive toasts can render dark text on red backgrounds until the toast classes are made variant-aware.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

docs/design/edition-color-vocabulary.md:12

  • The new :root contract is not reflected in the identity spec: docs/design/edition-identity-spec.md:15 still says variables are scoped to the edition-view root and that data-edition-theme="light" is attached there. Since #359 is called out as the future theme flip, leaving these documents inconsistent can make the rollout target the wrong element; update the identity spec in this change or keep the implementation aligned with it.
- **The tokens live on `:root`, not a scope class (#367).** They used to sit under a `.edition-view` class so only voter-facing pages picked them up, but that meant every other surface (admin included) kept the old shadcn light-theme defaults for the same variable names — so a partial migration elsewhere (like admin's) recolored whatever hadn't been touched yet, and Radix portal content (which renders under `document.body`, outside any wrapper's scope) never picked up the override at all. The whole app renders on the same dark identity today, so there's one set of values for the whole app and no scope class to remember. Each root layout applies `bg-app-gradient` directly; the flip (#359) will attach `data-edition-theme="light"` to the document root.

src/components/AuthDialog/EmailStep.tsx:49

  • border-surface-border is not the value this input had before: it maps to gray-300 (216 12% 84%), while the shared Input's existing border-input is slate-200 (214.3 31.8% 91.4%). This makes the auth field's border visibly darker despite the claimed zero-visual-change migration. Leave the border on border-input and only override the focus ring here.
          className="border-surface-border focus-visible:ring-surface-foreground"

src/components/ui/toaster.tsx:18

  • The new class only colors the Toast root. ToastClose has its own text-foreground/50 in src/components/ui/toast.tsx:78; with the new root foreground and the default toast's bg-background both white, the close icon remains white on white. Pass text-surface-foreground/50 (and its hover color) to ToastClose while retaining the destructive-state overrides.
          <Toast key={id} className="text-surface-foreground" {...props}>

src/index.css:10

  • Moving --foreground to :root makes the edition white foreground apply to every light surface, not only the edition/admin gradient roots. body still applies text-foreground, while DialogContent and the legal routes use bg-background (white) without a local foreground, so unmodified dialog labels/headings on /terms, /privacy, and /cookies become white on white. Keep this token scoped or add a foreground token/class to all of those light surfaces before globalizing it.
    --foreground: 0 0% 100%; /* text-white */

src/index.css:16

  • Globalizing --popover also changes the Radix portal content that this PR says is intentionally out of scope: SelectContent/PopoverContent use bg-popover text-popover-foreground, so admin selects such as src/routes/admin/admins.tsx:221 and TimezonePicker change from the old white/dark popover to gray-800/purple-100. This is an observable visual change; either preserve the light global popover values or migrate all portal components together.
    --popover: 215 27.9% 16.9%; /* gray-800 */
    --popover-foreground: 268.7 100% 95.5%; /* purple-100 */

src/index.css:25

  • Changing --muted-foreground globally to purple-200 recolors untouched light-surface content. For example, the unmodified text-muted-foreground in FestivalEditionManagement's dialog (src/pages/admin/festivals/FestivalEditionManagement.tsx:303) now renders pale purple on a white DialogContent, whereas it previously used gray-500; use text-surface-muted-foreground for these light surfaces or keep the edition value scoped.
    --muted-foreground: 268.6 100% 91.8%; /* purple-200 */

src/pages/admin/ArtistsManagement/ArchiveArtistDialog.tsx:52

  • hover:text-destructive-foreground does not reproduce the old hover:text-white: --destructive-foreground is still 210 40% 98%, whereas the removed utility is pure white. This changes the hover color despite the PR's zero-visual-change claim; use the exact white role for this state or add a destructive foreground value matching white.
      className="border-red-400 text-red-400 hover:bg-red-400 hover:text-destructive-foreground"
  • Files reviewed: 25/25 changed files
  • Comments generated: 1
  • Review effort level: Lite

{toasts.map(function ({ id, title, description, action, ...props }) {
return (
<Toast key={id} {...props}>
<Toast key={id} className="text-surface-foreground" {...props}>
InputOTPSlot char/caret and Input's text/placeholder inherited the
white/purple dark-identity foreground tokens, making them invisible
or off-color on white surfaces (login dialog). Use the surface-*
tokens instead, since both primitives are only ever used on
white-background surfaces.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The default toast close-control styling remains unresolved and may leave its icon invisible.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

src/components/ui/toaster.tsx:18

  • The new root text override does not style the toast close control. ToastClose still hardcodes text-foreground/50 and hover:text-foreground (src/components/ui/toast.tsx:78), so on the unchanged white bg-background toast its close icon remains white and is effectively invisible. Apply the surface foreground role to the close control for default toasts while retaining the destructive foreground for destructive toasts.
          <Toast key={id} className="text-surface-foreground" {...props}>

docs/design/edition-color-vocabulary.md:12

  • Changing the contract to :root here leaves docs/design/edition-identity-spec.md contradictory: its scope section still says admin/groups/settings keep the old skin and line 15 says the light attribute is applied to .edition-view. Since this PR deletes EditionViewRoot and the CSS now only matches :root[data-edition-theme=...], an implementation of #359 following the identity spec will not activate the light values. Update the identity spec in this change, or preserve the old scope contract.
- **The tokens live on `:root`, not a scope class (#367).** They used to sit under a `.edition-view` class so only voter-facing pages picked them up, but that meant every other surface (admin included) kept the old shadcn light-theme defaults for the same variable names — so a partial migration elsewhere (like admin's) recolored whatever hadn't been touched yet, and Radix portal content (which renders under `document.body`, outside any wrapper's scope) never picked up the override at all. The whole app renders on the same dark identity today, so there's one set of values for the whole app and no scope class to remember. Each root layout applies `bg-app-gradient` directly; the flip (#359) will attach `data-edition-theme="light"` to the document root.

src/components/ui/toaster.tsx:24

  • This description style is applied to every toast, including variant="destructive". The destructive toast root supplies text-destructive-foreground, but this child override forces the gray-500 surface-muted-foreground onto the red destructive background, making error details low-contrast. Select the surface-muted class only for default toasts and retain text-destructive-foreground for destructive ones.
                <ToastDescription className="text-surface-muted-foreground">
                  {description}
                </ToastDescription>

src/index.css:10

  • Making the edition --foreground value global changes every route's inherited body color, but --background remains white and existing light surfaces are not all migrated. The shared DialogContent/AlertDialogContent portals still use bg-background without a foreground, so titles, labels, and outline-button text inherit white and become white-on-white; the legal routes likewise use a white from-background gradient with text-muted-foreground, now purple-200. Keep these overrides scoped, or update all light/portal surfaces (including Sonner) before promoting them to :root.
    --foreground: 0 0% 100%; /* text-white */

src/pages/admin/ArtistsManagement/ArchiveArtistDialog.tsx:52

  • destructive-foreground is still 210 40% 98% (src/index.css:43), but the replaced class was hover:text-white. This changes the hover text from pure white to slate-50 and therefore does not preserve the promised exact visual mapping. Keep the literal here or update the destructive foreground role to the intended current white value before using it.
      className="border-red-400 text-red-400 hover:bg-red-400 hover:text-destructive-foreground"

src/routes/admin.tsx:99

  • With the admin root now declared part of the role-token rollout, non-portal admin content is still left literal: /admin/.../settings renders ScheduleRevealControl with bg-purple-100 text-purple-900 (and the artists loading state still uses border-purple-600). These paths will ignore the next token palette change; the portal exception in the PR description does not apply. Migrate the remaining non-portal admin literals.
              className={`grid w-full ${isSuperAdmin ? "grid-cols-4" : "grid-cols-3"} bg-surface-raised backdrop-blur-md`}
  • Files reviewed: 27/27 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Documents that surface-foreground/muted-foreground/border are hardcoded
grays for shared primitives on white backgrounds, and should fold into
foreground/muted-foreground/border once the light/dark theme flip (#359)
lands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Global --muted-foreground has poor contrast on several white-surface descriptions and should use surface-muted-foreground or a dark surface.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

src/index.css:25

  • The new global purple-200 --muted-foreground is still consumed by CardDescription, DialogDescription, FormDescription, and the legal/footer text-muted-foreground content. Those components remain on white surfaces (bg-card/bg-background), so their secondary text has very poor contrast outside the few locally patched cases such as AuthDialog and analytics. Route light-surface descriptions through surface-muted-foreground (or provide a dark surface) before changing this role globally.
    --muted-foreground: 268.6 100% 91.8%; /* purple-200 */

docs/design/edition-color-vocabulary.md:12

  • The implementation and this vocabulary now define the role variables on :root, but docs/design/edition-identity-spec.md:5,15 still says the identity is voter-facing only and that variables are scoped under .edition-view. These are conflicting contracts for the upcoming light-theme flip and will mislead future token work; update or explicitly supersede the identity spec alongside this migration.
- **The tokens live on `:root`, not a scope class (#367).** They used to sit under a `.edition-view` class so only voter-facing pages picked them up, but that meant every other surface (admin included) kept the old shadcn light-theme defaults for the same variable names — so a partial migration elsewhere (like admin's) recolored whatever hadn't been touched yet, and Radix portal content (which renders under `document.body`, outside any wrapper's scope) never picked up the override at all. The whole app renders on the same dark identity today, so there's one set of values for the whole app and no scope class to remember. Each root layout applies `bg-app-gradient` directly; the flip (#359) will attach `data-edition-theme="light"` to the document root.

src/components/ui/input.tsx:11

  • This fixes only <Input>, but Textarea and SelectTrigger use the same white bg-background surfaces and still inherit the global white foreground and purple-200 placeholder color. Admin forms use both (for example AddArtistDialog's description and StageSelector), so entered values and placeholders remain unreadable on white. Apply the same surface roles to those shared primitives or explicitly style every light-surface use.
          "flex h-10 w-full rounded-md border border-input bg-background px-3 py-2 text-base text-surface-foreground ring-offset-background file:border-0 file:bg-transparent file:text-sm file:font-medium file:text-surface-foreground placeholder:text-surface-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50 md:text-sm",

src/components/ui/toaster.tsx:18

  • This updates the Radix useToast renderer, but the root also mounts the separate Sonner renderer (src/routes/__root__.tsx:72). Its classes in src/components/ui/sonner.tsx:16-17 still combine bg-background with global text-foreground/text-muted-foreground, so Sonner notifications such as the service-worker update prompt remain white/light-purple text on a white toast. Update the Sonner token mapping as well.
          <Toast key={id} className="text-surface-foreground" {...props}>

src/components/ui/toaster.tsx:22

  • The Toast destructive variant already supplies text-destructive-foreground, but this unconditional class is merged afterward and overrides it; the description class on line 22 does the same. Destructive notifications therefore use dark surface text on the red background. Apply the surface classes only for the default variant so destructive title/description/action content keeps its destructive foreground.
          <Toast key={id} className="text-surface-foreground" {...props}>
            <div className="grid gap-1">
              {title && <ToastTitle>{title}</ToastTitle>}
              {description && (
                <ToastDescription className="text-surface-muted-foreground">

src/index.css:10

  • Making --foreground global also changes the color inherited from body, but DialogContent/AlertDialogContent, SelectTrigger, and Textarea still render on bg-background (white) without a surface text color. As a result, most admin dialog titles/labels, select values, and textarea values are white on white; only the explicitly patched AuthDialog/Input paths avoid it. Apply the surface foreground at these shared light-surface primitives, or keep the dark foreground scoped.
    --foreground: 0 0% 100%; /* text-white */

src/index.css:10

  • Moving the edition text roles to :root also changes every non-edition light surface, but only a few consumers were migrated to the new surface-* roles. body applies text-foreground, so generic Dialog/AlertDialog contents and labels (including admin/onboarding dialogs), ToastClose, and the legal routes' bg-background content become white on white; CardDescription/DialogDescription and the Sonner renderer likewise use the light muted-foreground value on white. Keep these identity variables scoped to dark shells or migrate the shared/light consumers (including Dialog, AlertDialog, Card, Sonner, and legal surfaces) before making them global.
    --foreground: 0 0% 100%; /* text-white */
  • Files reviewed: 27/27 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants