feat: add light theme token set, prefers-color-scheme support, and ex… - #21
Open
sharancreates wants to merge 20 commits into
Open
feat: add light theme token set, prefers-color-scheme support, and ex…#21sharancreates wants to merge 20 commits into
sharancreates wants to merge 20 commits into
Conversation
…plicit theme toggle
|
@sharancreates is attempting to deploy a commit to the stupnd's projects Team on Vercel. A member of the Team first needs to authorize it. |
Reviewer's GuideIntroduce a two-theme token system (light and dark) driven by theme-scoped CSS variables, wire it to prefers-color-scheme and a new ThemeToggle UI, and preload the chosen theme (including meta theme-color) to avoid FOUC. Sequence diagram for theme toggle interaction and applicationsequenceDiagram
actor User
participant ThemeToggle
participant theme_lib as theme_ts
participant Document
participant LocalStorage
participant MetaTag as meta_theme_color
User->>ThemeToggle: click
ThemeToggle->>ThemeToggle: cycleTheme()
ThemeToggle->>ThemeToggle: updateThemeMode(nextTheme)
ThemeToggle->>theme_lib: applyTheme(nextTheme)
theme_lib->>Document: getThemeAttribute(nextTheme)
alt attr is light/dark
theme_lib->>Document: setAttribute(data-theme, attr)
else system/unknown
theme_lib->>Document: removeAttribute(data-theme)
end
theme_lib->>LocalStorage: localStorage.setItem(THEME_STORAGE_KEY, nextTheme)
theme_lib->>theme_lib: updateMetaThemeColor(nextTheme)
theme_lib->>MetaTag: resolveMetaThemeColor(nextTheme, matchMedia("(prefers-color-scheme: light)").matches)
MetaTag->>MetaTag: setAttribute(content, themeColor)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The light theme token values are duplicated in both the
@media (prefers-color-scheme: light)block andhtml[data-theme="light"]; consider extracting them into a shared CSS custom properties block to avoid divergence over time. - Instead of injecting the inline theme-preload script with
dangerouslySetInnerHTMLin<head>, consider usingnext/scriptwithstrategy="beforeInteractive"to better align with Next.js’ script loading model. - In
ThemeToggle, you might want to listen forprefers-color-schemechanges when insystemmode so the metatheme-colorstays in sync if the OS theme toggles while the page is open.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The light theme token values are duplicated in both the `@media (prefers-color-scheme: light)` block and `html[data-theme="light"]`; consider extracting them into a shared CSS custom properties block to avoid divergence over time.
- Instead of injecting the inline theme-preload script with `dangerouslySetInnerHTML` in `<head>`, consider using `next/script` with `strategy="beforeInteractive"` to better align with Next.js’ script loading model.
- In `ThemeToggle`, you might want to listen for `prefers-color-scheme` changes when in `system` mode so the meta `theme-color` stays in sync if the OS theme toggles while the page is open.
## Individual Comments
### Comment 1
<location path="frontend/components/layout/ThemeToggle.tsx" line_range="48-55" />
<code_context>
+ else updateTheme("system");
+ };
+
+ if (!mounted) {
+ return (
+ <div
+ className={cn(
+ "inline-flex h-9 w-9 items-center justify-center rounded-pill border border-line bg-raised/50",
+ className,
+ )}
+ aria-hidden="true"
+ />
+ );
</code_context>
<issue_to_address>
**nitpick (performance):** The pre-mount placeholder has a fixed size that may differ from the actual button, causing subtle layout shift.
The skeleton uses `inline-flex h-9 w-9`, while the real button adds horizontal padding (`px-2.5`) and a text label at `sm` breakpoints, so its width changes on mount and reflows the navbar. Consider aligning the placeholder’s layout with the final button (same padding, gap, and possibly a dummy label at `sm`) to minimize layout shift.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The preloading script only applies
data-themefor"light"/"dark", butThemeTogglenow persists"system"as well; consider either omitting"system"from localStorage or updating the preload logic to handle it explicitly so the two code paths stay in sync. - The theme handling logic (localStorage key,
data-themeattribute, and metatheme-colorvalues) is duplicated between the inline script andThemeToggle; extracting shared constants/utilities would reduce the risk of these drifting apart over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The preloading script only applies `data-theme` for `"light"`/`"dark"`, but `ThemeToggle` now persists `"system"` as well; consider either omitting `"system"` from localStorage or updating the preload logic to handle it explicitly so the two code paths stay in sync.
- The theme handling logic (localStorage key, `data-theme` attribute, and meta `theme-color` values) is duplicated between the inline script and `ThemeToggle`; extracting shared constants/utilities would reduce the risk of these drifting apart over time.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="frontend/components/layout/ThemeToggle.tsx" line_range="49-58" />
<code_context>
+ onClick={cycleTheme}
+ aria-label={label}
+ title={label}
+ className={cn(
+ "inline-flex h-9 items-center gap-1.5 rounded-pill border border-line bg-raised/60 px-2.5 py-1",
+ "text-small font-medium text-text-soft transition-colors duration-[--duration-fast]",
+ "hover:bg-raised hover:text-text focus-visible:outline-2",
+ className,
</code_context>
<issue_to_address>
**issue (bug_risk):** The Tailwind `duration-[--duration-fast]` class likely compiles to invalid CSS.
Tailwind arbitrary values must be valid CSS, so this will likely emit `transition-duration: --duration-fast`, which is invalid. If you want to use the custom property, use `duration-[var(--duration-fast)]`; otherwise, use a concrete utility like `duration-150`.
</issue_to_address>
### Comment 2
<location path="frontend/components/layout/ThemeToggle.tsx" line_range="16-25" />
<code_context>
+ const [theme, setTheme] = useState<ThemeMode>("system");
+ const [mounted, setMounted] = useState(false);
+
+ useEffect(() => {
+ setMounted(true);
+ const stored = localStorage.getItem(THEME_STORAGE_KEY) as ThemeMode | null;
+ if (stored === "light" || stored === "dark" || stored === "system") {
+ setTheme(stored);
+ }
+ }, []);
+
+ // Listen for system preference changes when in 'system' mode
+ useEffect(() => {
+ if (theme !== "system") return;
+ const mediaQuery = window.matchMedia("(prefers-color-scheme: light)");
+ const handleChange = () => {
+ updateMetaThemeColor("system");
+ };
+ mediaQuery.addEventListener("change", handleChange);
+ return () => mediaQuery.removeEventListener("change", handleChange);
+ }, [theme]);
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `addEventListener('change', ...)` on `matchMedia` can cause compatibility issues in some browsers.
`MediaQueryList.addEventListener('change', ...)` isn’t universally supported (e.g. older Safari). To avoid runtime issues and keep system-mode working across browsers, feature-detect and fall back to `addListener`:
```ts
const mediaQuery = window.matchMedia('(prefers-color-scheme: light)');
const handleChange = () => updateMetaThemeColor('system');
if ('addEventListener' in mediaQuery) {
mediaQuery.addEventListener('change', handleChange);
return () => mediaQuery.removeEventListener('change', handleChange);
} else {
mediaQuery.addListener(handleChange);
return () => mediaQuery.removeListener(handleChange);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The dark theme surface and text token values are now defined both in the @theme block and in the :root/html[data-theme="dark"] rules; consider centralizing these in one place to avoid them drifting out of sync in future updates.
- In ThemeToggle, the system-mode matchMedia listener only updates the meta theme color and not the CSS data-theme attribute; if you intend the UI to live-switch between light/dark tokens when the OS preference changes, you may want to call applyTheme("system") in the handler instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The dark theme surface and text token values are now defined both in the @theme block and in the :root/html[data-theme="dark"] rules; consider centralizing these in one place to avoid them drifting out of sync in future updates.
- In ThemeToggle, the system-mode matchMedia listener only updates the meta theme color and not the CSS data-theme attribute; if you intend the UI to live-switch between light/dark tokens when the OS preference changes, you may want to call applyTheme("system") in the handler instead.
## Individual Comments
### Comment 1
<location path="frontend/components/layout/ThemeToggle.tsx" line_range="68-73" />
<code_context>
+ );
+ }
+
+ const label =
+ theme === "system"
+ ? "Theme: System (click to switch to Light)"
+ : theme === "light"
+ ? "Theme: Light (click to switch to Dark)"
+ : "Theme: Dark (click to switch to System)";
+
+ return (
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The aria-label describes the *next* action rather than the current state, which can be confusing for screen reader users.
Because the label mixes current state with the future action, it’s harder for assistive technologies to convey what is selected versus what will happen on click. It’s preferable for the accessible name to reflect only the current theme (e.g. "Theme: Light"), and to communicate the switching behaviour separately (e.g. via tooltip or nearby text) rather than in the aria-label/title itself.
Suggested implementation:
```typescript
const label =
theme === "system"
? "Theme: System"
: theme === "light"
? "Theme: Light"
: "Theme: Dark";
```
1. Ensure the `label` continues to be used as the `aria-label` (or accessible name) for the toggle control, without adding future-action wording elsewhere in the accessible name (e.g. `title`).
2. If you want to preserve the "click to switch" guidance, consider adding it as visible helper text near the toggle or as a tooltip that is *not* used as the accessible name (depending on your tooltip implementation).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…y and live OS switching
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The theme token assignments for dark/light are duplicated across :root, html[data-theme] and the prefers-color-scheme media query; consider extracting a shared helper or using CSS custom properties more compositionally to reduce repetition and ease future palette changes.
- The ThemeToggle’s aria-label only reflects the current mode while the control’s behavior is to cycle modes; consider using a more descriptive label (e.g. including the next state) or separate status text to better communicate the interaction to screen-reader users.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The theme token assignments for dark/light are duplicated across :root, html[data-theme] and the prefers-color-scheme media query; consider extracting a shared helper or using CSS custom properties more compositionally to reduce repetition and ease future palette changes.
- The ThemeToggle’s aria-label only reflects the current mode while the control’s behavior is to cycle modes; consider using a more descriptive label (e.g. including the next state) or separate status text to better communicate the interaction to screen-reader users.
## Individual Comments
### Comment 1
<location path="frontend/components/layout/Navbar.tsx" line_range="52" />
<code_context>
className={cn(
"inline-flex min-h-11 items-center whitespace-nowrap rounded-pill px-3",
- "text-small font-medium transition-colors duration-(--duration-fast)",
+ "text-small font-medium transition-colors duration-[--duration-fast]",
active
? "bg-accent-dim text-accent"
</code_context>
<issue_to_address>
**issue (bug_risk):** The Tailwind arbitrary duration value should wrap the CSS variable in `var(...)`.
`duration-[--duration-fast]` generates `animation-duration: --duration-fast`, which is not a valid time value. To correctly use the CSS variable, this should be `duration-[var(--duration-fast)]`, consistent with `ThemeToggle`.
</issue_to_address>
### Comment 2
<location path="frontend/app/globals.css" line_range="48-51" />
<code_context>
- /* accent = fills and small text (white on it passes AA).
- accent-vivid = display type and decoration only. */
- --color-accent: #b23f13;
+ --color-accent: var(--dark-color-accent);
+ --color-accent-bright: var(--dark-color-accent-bright);
--color-accent-vivid: #d9541f;
- /* Hover on a filled button darkens rather than brightens — white on this is
- 7.57, so the label stays readable through the transition. */
--color-accent-hover: #96330c;
- --color-accent-dim: rgba(217, 84, 31, 0.1);
+ --color-accent-dim: var(--dark-color-accent-dim);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Accent hover and vivid colors remain hard-coded and don’t follow the new dark/light token system.
These two variables remain hard-coded, so they won’t respond to theme changes or stay aligned with the WCAG-reviewed palette. Please add corresponding dark/light accent-vivid and hover tokens (e.g. `--dark-color-accent-vivid`, `--light-color-accent-vivid`) and use them here so accent states are tokenized consistently across themes.
Suggested implementation:
```
/* ── Accent ───────────────────────────────────────────────────────────── */
--color-accent: var(--dark-color-accent);
--color-accent-bright: var(--dark-color-accent-bright);
--color-accent-vivid: var(--dark-color-accent-vivid);
--color-accent-hover: var(--dark-color-accent-hover);
--color-accent-dim: var(--dark-color-accent-dim);
--color-rose: #a8395e;
```
To fully implement the dark/light token system for accent states, you should also:
1. Define `--dark-color-accent-vivid` and `--dark-color-accent-hover` alongside the other `--dark-color-*` tokens (using the WCAG-reviewed values that were previously hard-coded: `#d9541f` and `#96330c`).
2. Define corresponding `--light-color-accent-vivid` and `--light-color-accent-hover` tokens in the light theme section.
3. In the light theme block where `--color-accent` and related properties are set, mirror this pattern by wiring `--color-accent-vivid` and `--color-accent-hover` to the light tokens (`var(--light-color-accent-vivid)` / `var(--light-color-accent-hover)`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
ThemeToggleeffect that usesmatchMedia.addListenershould guard against missingwindowto avoid potential SSR/hydration issues and could be simplified by always usingaddEventListenerwith a feature check at module level. - The logic in
THEME_PRELOAD_SCRIPTduplicates parts ofapplyThemeandupdateMetaThemeColor; consider extracting a small shared helper or aligning the branches to keep theme handling consistent and easier to maintain. - In
updateMetaThemeColor, repeatedly queryingdocument.querySelector('meta[name="theme-color"]')on every theme change could be optimized by caching the element reference once, reducing DOM lookups during rapid toggling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ThemeToggle` effect that uses `matchMedia.addListener` should guard against missing `window` to avoid potential SSR/hydration issues and could be simplified by always using `addEventListener` with a feature check at module level.
- The logic in `THEME_PRELOAD_SCRIPT` duplicates parts of `applyTheme` and `updateMetaThemeColor`; consider extracting a small shared helper or aligning the branches to keep theme handling consistent and easier to maintain.
- In `updateMetaThemeColor`, repeatedly querying `document.querySelector('meta[name="theme-color"]')` on every theme change could be optimized by caching the element reference once, reducing DOM lookups during rapid toggling.
## Individual Comments
### Comment 1
<location path="frontend/components/layout/ThemeToggle.tsx" line_range="16-19" />
<code_context>
+ const [theme, setTheme] = useState<ThemeMode>("system");
+ const [mounted, setMounted] = useState(false);
+
+ useEffect(() => {
+ setMounted(true);
+ const stored = localStorage.getItem(THEME_STORAGE_KEY) as ThemeMode | null;
+ if (stored === "light" || stored === "dark" || stored === "system") {
+ setTheme(stored);
+ }
</code_context>
<issue_to_address>
**issue:** Guard localStorage access in effect to avoid runtime errors in restricted environments.
In some environments (e.g. Safari private mode or strict privacy settings), `localStorage.getItem` can throw instead of returning `null`. Because this effect runs on every mount, an exception here could break the toggle or page. Please wrap the `localStorage.getItem` call in a `try/catch`, similar to `THEME_PRELOAD_SCRIPT`, to make this hook resilient to those cases.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="frontend/lib/theme.ts" line_range="7" />
<code_context>
+export const THEME_COLOR_LIGHT = "#f7f8f9";
+export const THEME_COLOR_DARK = "#0b0b0c";
+
+let cachedMetaTag: HTMLMetaElement | null = null;
+
+/**
</code_context>
<issue_to_address>
**suggestion:** Align cachedMetaTag typing with querySelector return type to keep TypeScript happy.
`document.querySelector` returns `Element | null`, so assigning directly to `cachedMetaTag: HTMLMetaElement | null` will cause a TypeScript type error. Add an explicit assertion when caching the element to keep the strong `HTMLMetaElement` type:
```ts
let cachedMetaTag: HTMLMetaElement | null = null;
export function updateMetaThemeColor(theme: ThemeMode): void {
if (typeof window === "undefined") return;
if (!cachedMetaTag || !document.contains(cachedMetaTag)) {
cachedMetaTag = document.querySelector('meta[name="theme-color"]') as HTMLMetaElement | null;
}
if (!cachedMetaTag) return;
// ...
}
```
Suggested implementation:
```typescript
let cachedMetaTag: HTMLMetaElement | null = null;
```
```typescript
cachedMetaTag = document.querySelector('meta[name="theme-color"]') as HTMLMetaElement | null;
```
I only see the top of the file, so you’ll need to:
1. Locate the `updateMetaThemeColor` (or equivalent) function where `cachedMetaTag` is assigned from `document.querySelector('meta[name="theme-color"]')` and ensure the SEARCH block matches that exact line.
2. If the assignment is slightly different (e.g. using double quotes or different spacing), adjust the SEARCH string accordingly, keeping the REPLACE as the asserted type `as HTMLMetaElement | null`.
3. Confirm there are no other places where `cachedMetaTag` is assigned from `querySelector` and apply the same assertion there to maintain consistent typing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The light/dark theme token application blocks in globals.css are quite repetitive across :root, html[data-theme="light"/"dark"], and the prefers-color-scheme rule; consider consolidating these assignments (e.g. via a preprocessor or custom properties indirection) to reduce duplication and make future tuning easier.
- ThemeToggle’s system-mode media query listener currently only reacts to changes; if a user switches from an explicit light/dark mode back to system, you may want to immediately re-evaluate and apply the current OS preference (rather than waiting for the next change event) to avoid any transient mismatch.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The light/dark theme token application blocks in globals.css are quite repetitive across :root, html[data-theme="light"/"dark"], and the prefers-color-scheme rule; consider consolidating these assignments (e.g. via a preprocessor or custom properties indirection) to reduce duplication and make future tuning easier.
- ThemeToggle’s system-mode media query listener currently only reacts to changes; if a user switches from an explicit light/dark mode back to system, you may want to immediately re-evaluate and apply the current OS preference (rather than waiting for the next change event) to avoid any transient mismatch.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In globals.css, the default active theme variable assignments appear outside of any selector block (immediately after the shared token :root definition), which will result in invalid CSS; these should be moved inside a selector (e.g. :root) or the stray closing brace adjusted.
- The active-* CSS custom properties for dark and light themes are duplicated across html[data-theme] and the prefers-color-scheme media query; consider extracting a shared mixin or restructuring to reduce repetition and the risk of the two paths diverging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In globals.css, the default active theme variable assignments appear outside of any selector block (immediately after the shared token :root definition), which will result in invalid CSS; these should be moved inside a selector (e.g. :root) or the stray closing brace adjusted.
- The active-* CSS custom properties for dark and light themes are duplicated across html[data-theme] and the prefers-color-scheme media query; consider extracting a shared mixin or restructuring to reduce repetition and the risk of the two paths diverging.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The theme token mapping for dark/light (
--active-*assignments) is duplicated across:root,html[data-theme="dark"],html[data-theme="light"], and the media query; consider centralizing this logic (e.g., via shared utility classes or a smaller set of custom properties) to reduce maintenance overhead when tokens change. - The pre-hydration
THEME_PRELOAD_SCRIPTreimplements parts ofapplyTheme/updateMetaThemeColor; aligning these flows or deriving the inline script from the same helpers would help avoid drift if the theme application logic evolves.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The theme token mapping for dark/light (`--active-*` assignments) is duplicated across `:root`, `html[data-theme="dark"]`, `html[data-theme="light"]`, and the media query; consider centralizing this logic (e.g., via shared utility classes or a smaller set of custom properties) to reduce maintenance overhead when tokens change.
- The pre-hydration `THEME_PRELOAD_SCRIPT` reimplements parts of `applyTheme`/`updateMetaThemeColor`; aligning these flows or deriving the inline script from the same helpers would help avoid drift if the theme application logic evolves.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ration inline script
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The light theme variables are duplicated in both
html[data-theme="light"]and the@media (prefers-color-scheme: light)block; consider extracting a shared rule (e.g. a class or selector that both extend) to reduce maintenance overhead when adjusting tokens. - The
@themeblock still hardcodes the dark palette values that are then overridden by the theme rules; if Tailwind doesn’t rely on those base values directly, simplifying or aligning them with the active theme system could make the token source of truth clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The light theme variables are duplicated in both `html[data-theme="light"]` and the `@media (prefers-color-scheme: light)` block; consider extracting a shared rule (e.g. a class or selector that both extend) to reduce maintenance overhead when adjusting tokens.
- The `@theme` block still hardcodes the dark palette values that are then overridden by the theme rules; if Tailwind doesn’t rely on those base values directly, simplifying or aligning them with the active theme system could make the token source of truth clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The dark theme token definitions are duplicated in both :root and html[data-theme="dark"]; consider centralizing these values (e.g., via a shared class or CSS custom property aliasing) to reduce maintenance overhead when tweaking the palette.
- THEME_PRELOAD_SCRIPT reimplements parts of applyTheme/updateMetaThemeColor logic as an inline string; factoring the decision logic into a shared helper that can be reused for both the preload script and runtime functions would help avoid subtle drift between behaviours over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The dark theme token definitions are duplicated in both :root and html[data-theme="dark"]; consider centralizing these values (e.g., via a shared class or CSS custom property aliasing) to reduce maintenance overhead when tweaking the palette.
- THEME_PRELOAD_SCRIPT reimplements parts of applyTheme/updateMetaThemeColor logic as an inline string; factoring the decision logic into a shared helper that can be reused for both the preload script and runtime functions would help avoid subtle drift between behaviours over time.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
THEME_PRELOAD_SCRIPTcurrently relies ongetThemeAttribute.toString()andisLightTheme.toString(), which can be brittle under minification or future refactors; consider inlining a minimal, explicit version of that logic in the script string instead. - In
ThemeToggle, theuseEffectwatchingthemere-attaches thematchMedialistener whenever the mode changes tosystem; you could simplify by keeping a single listener and toggling behavior based on current mode to avoid repeated setup/teardown.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `THEME_PRELOAD_SCRIPT` currently relies on `getThemeAttribute.toString()` and `isLightTheme.toString()`, which can be brittle under minification or future refactors; consider inlining a minimal, explicit version of that logic in the script string instead.
- In `ThemeToggle`, the `useEffect` watching `theme` re-attaches the `matchMedia` listener whenever the mode changes to `system`; you could simplify by keeping a single listener and toggling behavior based on current mode to avoid repeated setup/teardown.
## Individual Comments
### Comment 1
<location path="frontend/components/layout/ThemeToggle.tsx" line_range="17-19" />
<code_context>
+
+ useEffect(() => {
+ setMounted(true);
+ const stored = getStoredTheme();
+ if (stored) {
+ setTheme(stored);
+ }
+ }, []);
</code_context>
<issue_to_address>
**issue (bug_risk):** Stored theme is loaded into state but never applied to the DOM if the preload script fails.
The mount effect only calls `setTheme(stored)`, but `applyTheme` is never run here, so `data-theme` and `meta[name="theme-color"]` won’t reflect the stored preference if the preload script doesn’t execute. To keep the DOM in sync with the resolved theme in all environments, call `applyTheme(stored ?? "system")` after `setTheme`, or replace `setTheme(stored)` with `updateThemeMode(stored)` in this effect.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The theme token rules in globals.css duplicate assignments across :root, .theme-dark, .theme-light, and the prefers-color-scheme block; consider extracting shared mappings into a single custom property set (e.g. via CSS custom properties or a utility class) to reduce repetition and make future palette changes less error-prone.
- The logic for resolving the effective theme and updating meta[name="theme-color"] is implemented both in THEME_PRELOAD_SCRIPT and in updateMetaThemeColor/applyTheme; centralizing this decision logic in one place and reusing it in the inline script would make behavior easier to reason about and keep in sync.
- The top-level suppressHydrationWarning on the element hides all hydration mismatches; if possible, narrow this to the specific theme-related markup or attributes that can diverge between server and client to avoid masking other potential hydration issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The theme token rules in globals.css duplicate assignments across :root, .theme-dark, .theme-light, and the prefers-color-scheme block; consider extracting shared mappings into a single custom property set (e.g. via CSS custom properties or a utility class) to reduce repetition and make future palette changes less error-prone.
- The logic for resolving the effective theme and updating meta[name="theme-color"] is implemented both in THEME_PRELOAD_SCRIPT and in updateMetaThemeColor/applyTheme; centralizing this decision logic in one place and reusing it in the inline script would make behavior easier to reason about and keep in sync.
- The top-level suppressHydrationWarning on the <html> element hides all hydration mismatches; if possible, narrow this to the specific theme-related markup or attributes that can diverge between server and client to avoid masking other potential hydration issues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…eme color resolution
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The light/dark token assignment blocks in globals.css are quite duplicated between :root/html[data-theme="dark"], .theme-light/html[data-theme="light"], and the prefers-color-scheme media query; consider consolidating shared property sets into a single rule or using CSS custom property indirection to reduce maintenance overhead when adjusting theme values.
- In ThemeToggle, the mounted placeholder and the interactive button have slightly different contents and widths (e.g., icon + label vs skeleton blocks), which may cause minor layout shift on hydration; you may want to align their structure and sizing more closely to avoid any visual jump when the component mounts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The light/dark token assignment blocks in globals.css are quite duplicated between :root/html[data-theme="dark"], .theme-light/html[data-theme="light"], and the prefers-color-scheme media query; consider consolidating shared property sets into a single rule or using CSS custom property indirection to reduce maintenance overhead when adjusting theme values.
- In ThemeToggle, the mounted placeholder and the interactive button have slightly different contents and widths (e.g., icon + label vs skeleton blocks), which may cause minor layout shift on hydration; you may want to align their structure and sizing more closely to avoid any visual jump when the component mounts.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The theme resolution logic is duplicated between THEME_PRELOAD_SCRIPT and the TypeScript helpers (isLightTheme/resolveMetaThemeColor); consider centralizing the conditions to reduce the risk of them drifting apart over time.
- ThemeToggle’s matchMedia listener logic could be moved into a small helper in lib/theme to keep side-effectful DOM interactions in one place and make the component itself more declarative.
- The pre-hydration theme script assumes a meta[name="theme-color"] tag exists; adding a guard or creating the tag if missing would make the behavior more robust across different HTML head configurations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The theme resolution logic is duplicated between THEME_PRELOAD_SCRIPT and the TypeScript helpers (isLightTheme/resolveMetaThemeColor); consider centralizing the conditions to reduce the risk of them drifting apart over time.
- ThemeToggle’s matchMedia listener logic could be moved into a small helper in lib/theme to keep side-effectful DOM interactions in one place and make the component itself more declarative.
- The pre-hydration theme script assumes a meta[name="theme-color"] tag exists; adding a guard or creating the tag if missing would make the behavior more robust across different HTML head configurations.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…istener, and add meta tag creation guard
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The THEME_PRELOAD_SCRIPT currently relies on getThemeAttribute.toString() and isLightTheme.toString(), which can be brittle under minification/transform; consider inlining minimal helper logic in the string literal instead to avoid dependency on function serialization.
- In layout.tsx you introduce a manual with a beforeInteractive Script; double-check that this doesn’t conflict with Next’s automatic head/metadata handling, and consider attaching the preload script via or using the metadata API if you run into head composition issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The THEME_PRELOAD_SCRIPT currently relies on getThemeAttribute.toString() and isLightTheme.toString(), which can be brittle under minification/transform; consider inlining minimal helper logic in the string literal instead to avoid dependency on function serialization.
- In layout.tsx you introduce a manual <head> with a beforeInteractive Script; double-check that this doesn’t conflict with Next’s automatic head/metadata handling, and consider attaching the preload script via <body> or using the metadata API if you run into head composition issues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…beforeInteractive script placement in layout
Contributor
Author
|
@sourcery-ai review |
Contributor
Author
|
@stupnd kindly check |
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.
feat: add light theme token set, prefers-color-scheme support, and explicit theme toggle
Summary
This PR adds a near-neutral light canvas token set to Tinted behind
@media (prefers-color-scheme: light)and explicitdata-theme="light"/data-theme="dark"attribute selectors, re-verifies all text/surface pairs at WCAG AA contrast, and introduces an explicit Theme Toggle component in the navigation bar.Like the dark canvas, the light canvas is kept near-neutral (
#f7f8f9bg,#ffffffsurface,#e8eaearaised) to prevent background tint from distorting perceived skin tone swatches.Changes Made
--color-bg: #f7f8f9,--color-surface: #ffffff,--color-raised: #e8eaea,--color-accent: #875018).@media (prefers-color-scheme: light)andhtml[data-theme="light"]/html[data-theme="dark"]rules..bg-swatch-check) to dynamically adapt using CSS variables.localStorageundertinted-theme.data-themeondocument.documentElementand<meta name="theme-color">.<ThemeToggle />in the header next to<AuthButton />.<head>to readlocalStoragetheme preference before render, preventing Flash of Unstyled Content (FOUC).WCAG AA Contrast Verification
text#141517--color-bg(#f7f8f9)text#141517--color-surface(#ffffff)text#141517--color-raised(#e8eaea)text-soft#484a4e--color-bg(#f7f8f9)text-soft#484a4e--color-surface(#ffffff)text-soft#484a4e--color-raised(#e8eaea)text-muted#616368--color-bg(#f7f8f9)text-muted#616368--color-surface(#ffffff)text-muted#616368--color-raised(#e8eaea)accent#875018--color-bg(#f7f8f9)accent#875018--color-surface(#ffffff)accent#875018--color-raised(#e8eaea)Summary by Sourcery
Introduce a theme system with light and dark token sets, automatic system preference support, and an explicit theme toggle in the navbar.
New Features:
Enhancements: