Implement themes with <ThemeProvider/>#18
Merged
ianpaschal merged 12 commits intomasterfrom Mar 28, 2026
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request refactors the styling and theming system across the codebase. It introduces a new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/PdfViewer/PdfViewer.module.scss (1)
61-61: 🧹 Nitpick | 🔵 TrivialInconsistent token usage: consider migrating
--gray-8to a theme variable.The background was updated to use
--color-page-bg, but the loading/error text color still uses the old--gray-8token. For consistency with the theme migration, consider using a semantic color token such as--color-ui-text-mutedor similar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/PdfViewer/PdfViewer.module.scss` at line 61, The CSS in PdfViewer.module.scss uses the legacy token var(--gray-8) for the loading/error text; replace that token with the semantic theme token (e.g., var(--color-ui-text-muted) or the project’s equivalent) so it matches the migrated background token var(--color-page-bg); update the color declaration that currently reads color: var(--gray-8); to use the chosen semantic token and ensure any related comments or usages in the PdfViewer component reflect the new token name.src/components/Button/Button.stories.tsx (1)
36-41:⚠️ Potential issue | 🟡 MinorVariant options are inconsistent with the new variant system.
The
variantargTypes options still list the old values ('primary','secondary','outlined','ghost'), but the story args default to'solid'which is part of the new variant system (solid,shaded,ghost,surface). This mismatch will cause Storybook's variant dropdown to show outdated options.🔧 Proposed fix to update variant options
variant: { control: 'select', - options: ['primary', 'secondary', 'outlined', 'ghost'], + options: ['solid', 'shaded', 'ghost', 'surface'], description: 'The variant of the button.', table: { category: 'Appearance' }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Button/Button.stories.tsx` around lines 36 - 41, The variant argTypes in Button.stories.tsx are listing old options ('primary','secondary','outlined','ghost') while the story default uses the new system ('solid'); update the variant control options to match the new variant system (solid, shaded, ghost, surface) so the Storybook dropdown reflects the actual possible values for the Button story; locate the variant argType definition (the object with control: 'select' and options) and replace the options array to ['solid','shaded','ghost','surface'], and verify the story args default for variant remains 'solid' (or adjust to a valid new variant) to keep them consistent.src/utils/getStyleClassNames.tsx (1)
50-63:⚠️ Potential issue | 🟡 MinorRestore a base variant for the border-only path.
Line 62 adds only the intent class. In
src/style/variants.module.scss,.neutral/.danger/etc. are only defined under.solid,.shaded, or.ghost, sosx({ border: true })now emits no base variant class at all. That leaves this path falling back to browser/currentColor border styling instead of the themed neutral border.Suggested fix
- if (config.border && !(config.variant === 'solid')) { + if (config.border && config.variant !== 'solid') { + if (!config.variant) { + classNames.add(variants.ghost); + } + if (Array.isArray(config.border)) { for (const side of config.border) { const key = camelKey(['border', side]) as keyof typeof borders; classNames.add(borders[key]); } @@ } else { classNames.add(borders.border); } - classNames.add(variants[config.intent ?? 'neutral']); + if (config.variant !== 'surface') { + classNames.add(variants[config.intent ?? 'neutral']); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/getStyleClassNames.tsx` around lines 50 - 63, In the border-only branch inside getStyleClassNames.tsx (the block that checks config.border), add the base variant class before adding the intent class so themed border styles are applied; specifically, call classNames.add(variants[config.variant ?? 'solid']) (using the existing variants lookup) prior to classNames.add(variants[config.intent ?? 'neutral']) and keep the existing borders/* additions and camelKey usage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.storybook/preview.tsx:
- Line 2: The import currently pulls Theme as a value; change it to a type-only
import to match its export: keep the value imports ThemeProvider, light, dark,
midnight in the regular import and import Theme using a type-only import (e.g.,
import type { Theme } from ...); update the import statement that references
ThemeProvider, light, dark, midnight, Theme so Theme is imported with the
type-only modifier while the others remain normal imports.
In `@package.json`:
- Around line 30-38: Add and pin the "tsx" package as a devDependency and update
the npm scripts to call it directly instead of relying on npx; specifically, add
a versioned entry for "tsx" in devDependencies and change the "gen:theme" and
"gen:scss" scripts (referenced by "gen", "gen:theme", "gen:scss") to invoke tsx
directly (e.g., "tsx scripts/generateThemeVariables.ts" and "tsx
scripts/generateScssTypes.ts") so the build is reproducible in CI without
relying on PostCSS peer installation.
In `@scripts/generateScssTypes.ts`:
- Around line 36-41: Wrap the per-file compilation in a try/catch so a failing
compile(file) doesn’t crash the whole run; around the loop body that calls
compile(file), extractClassNames(result.css), and writeFileSync(file + '.d.ts',
...), catch any error, log or throw a new Error that includes the file name and
the original error message (e.g., "Failed to compile SCSS for <file>: <err>")
and decide whether to continue to the next file or rethrow based on desired
behavior.
- Around line 23-32: The extractClassNames function is currently matching
dot-prefixed tokens inside URLs/comments (producing spurious names like
"org"/"w3"); before running the regex variable, further sanitize the stripped
string by removing CSS comments, url(...) contents and protocol patterns (e.g.,
"http://") so those dot-prefixed tokens cannot be matched: update the creation
of stripped in extractClassNames to first strip /\/\*[\s\S]*?\*\//g, remove
/url\(\s*[^)]+\)/g matches and remove protocol patterns like /\b[a-z]+:\/\//i
(or otherwise blank out characters between "://" and the next delimiter), then
run the existing regex and continue adding to the names Set and calling
toCamelCase as before.
In `@scripts/generateThemeVariables.ts`:
- Around line 18-23: Add a check that validates marker order after locating
START_MARKER and END_MARKER: after computing startIdx and endIdx, verify that
endIdx > startIdx (for example throw a descriptive Error if endIdx <= startIdx)
so a swapped or overlapping marker order is detected before rewriting in
generateThemeVariables.ts; reference startIdx, endIdx, START_MARKER and
END_MARKER when adding the validation and error message.
In
`@src/components/AppNavigation/components/DesktopRoutes/DesktopRoutes.module.scss`:
- Line 147: Remove the dead commented mixin directive "// `@include`
shadows.elevated;" from DesktopRoutes.module.scss to eliminate noise; locate the
commented include for the shadows.elevated mixin and delete that single line so
the stylesheet contains only active rules.
In `@src/components/DialogManager/Dialog.module.scss`:
- Line 55: The min-width calc in Dialog.module.scss uses 100vw which can
mismatch the max-width that uses 100dvw and cause overflow on mobile; update the
min-width declaration (the rule containing "min-width: min(calc(100vw - (2 *
var(--page-padding))), 24rem);") to use 100dvw instead of 100vw so it matches
the max-width calculation and prevents viewport-unit mismatch issues.
In `@src/components/Drawer/Drawer.tsx`:
- Around line 38-40: DrawerProps currently makes title required which breaks
title-less drawers; change the type so title is optional again (make title?:
string on DrawerProps) and update the Drawer component where it renders
BaseDrawer.Title to render it only when title is present (conditionally render
<BaseDrawer.Title> inside the Drawer render path). Ensure any usages of the
title prop within the Drawer component (e.g., in function Drawer or component
DrawerContent) guard against undefined to preserve backward compatibility.
In `@src/components/PdfViewer/PdfViewer.tsx`:
- Line 12: The import in PdfViewer.tsx currently references the component file
directly (import { PdfViewerControls } from
'./components/PdfViewerControls/PdfViewerControls'); update it to import from
the barrel export by changing the import source to the directory (e.g.
'./components/PdfViewerControls') so it uses the index.ts re-export for
PdfViewerControls; this keeps the PdfViewerControls symbol decoupled from the
internal file structure and consistent with other barrel imports.
In `@src/components/ScrollArea/ScrollArea.module.scss.d.ts`:
- Around line 1-2: The generated ScrollArea.module.scss.d.ts contains spurious
exports (html, org, w3) produced by a URL-pattern false positive in the SCSS
typing generator; fix the generator (generateScssTypes.ts) to ignore URL-derived
tokens (e.g., skip matches coming from url(...) or sanitize/validate class-name
candidates) and then regenerate the typings so ScrollArea.module.scss.d.ts no
longer exports html, org, or w3; locate logic that extracts class names in
generateScssTypes.ts and add a guard to filter out URL patterns before writing
declarations.
In `@src/components/Select/Select.module.scss`:
- Around line 56-62: The new popup animation should respect users' motion
preferences: wrap or override the transition and transform/opacity rules for the
selectors (the transition: transform 150ms, opacity 150ms; and the
&[data-starting-style], &[data-ending-style] { transform: scale(0.9); opacity:
0; }) inside a prefers-reduced-motion: no-preference context and add a
prefers-reduced-motion: reduce media query that removes transitions and
transforms (i.e., set transition: none and reset transform/opacity to their
final states) so users with motion sensitivity get an instantaneous,
non-animated popup.
In `@src/components/ThemeProvider/ThemeProvider.hooks.ts`:
- Around line 10-16: Replace the direct useLayoutEffect block with an isomorphic
effect and add cleanup that restores previous CSS variable values: use a
useIsomorphicLayoutEffect (or a conditional that aliases to useLayoutEffect on
the client and useEffect on the server) around the logic that calls
buildThemeVars(theme), applies each entry to document.documentElement via
root.style.setProperty, and before applying save the prior values for each key
so the effect's cleanup iterates those keys and restores their original values
(or removes the variable if it was undefined) to avoid leaking theme state for
nested providers; keep references to useLayoutEffect, buildThemeVars, theme,
document.documentElement and root.style.setProperty when locating the code to
change.
In `@src/components/ThemeProvider/ThemeProvider.tsx`:
- Line 10: The theme prop currently typed as Partial<Theme> is too shallow for
the deepmerge used in ThemeProvider; define a recursive DeepPartial<T> utility
type and change the ThemeProvider prop type from theme?: Partial<Theme> to
theme?: DeepPartial<Theme> so callers can override nested keys (e.g.,
text.header or surface.page.bg). Update the import/exports as needed and, where
you call deepmerge inside ThemeProvider (the merge invocation that combines
props.theme with defaults), cast the passed value to Partial<Theme> (or the type
the deepmerge overload expects) to satisfy the library types while preserving
the deep-partial typing for consumers.
In `@src/components/ThemeProvider/ThemeProvider.utils.ts`:
- Around line 53-75: Summary: Non-solid variants reuse `bg` for text tokens
causing insufficient contrast; derive distinct accessible foregrounds for
shaded/ghost. Fix: In ThemeProvider.utils.ts compute separate accessible
foreground values (e.g., shadedForeground and ghostForeground) instead of
reusing `bg` for [`--shaded-${intent}-text`], [`--shaded-${intent}-text-hover`],
[`--shaded-${intent}-text-active`], [`--ghost-${intent}-text`],
[`--ghost-${intent}-text-hover`], and [`--ghost-${intent}-text-active`]; derive
these using a contrast-aware helper (use or add a function like
getAccessibleForeground(bg, surface) or call your existing contrast/accessible
color util) so the non-solid foreground is selected against the surface color
and meets AA contrast, then substitute those symbols where `bg` is currently
used.
In `@src/components/ThemeProvider/themes/dark.ts`:
- Around line 33-52: Summary: The dark theme entries danger, warning, success,
and info currently set text: '#FFFFFF' which likely fails WCAG AA contrast
against their bg colors. Fix: update the text color for the intent objects named
danger, warning, success, and info in themes/dark.ts to a higher-contrast value
(e.g., use a dark text like '#000000' or an approved accessible token) so that
each bg/text pair meets WCAG AA for normal-size text; after changing, run a
contrast check for each pair and adjust to the minimum accessible color if
needed.
In `@src/components/ThemeProvider/themes/light.ts`:
- Around line 33-52: In themes/light.ts the intent entries danger, warning,
success and info currently use text: '#FFFFFF' which fails WCAG AA contrast
against their bg colors; update each of those objects (danger, warning, success,
info) to use a foreground color that meets at least 4.5:1 contrast against the
configured bg (either pick verified darker hex values or compute/select
black/white via a contrast utility), then validate all four pairs with a
contrast checker and commit the adjusted hex values into those objects.
---
Outside diff comments:
In `@src/components/Button/Button.stories.tsx`:
- Around line 36-41: The variant argTypes in Button.stories.tsx are listing old
options ('primary','secondary','outlined','ghost') while the story default uses
the new system ('solid'); update the variant control options to match the new
variant system (solid, shaded, ghost, surface) so the Storybook dropdown
reflects the actual possible values for the Button story; locate the variant
argType definition (the object with control: 'select' and options) and replace
the options array to ['solid','shaded','ghost','surface'], and verify the story
args default for variant remains 'solid' (or adjust to a valid new variant) to
keep them consistent.
In `@src/components/PdfViewer/PdfViewer.module.scss`:
- Line 61: The CSS in PdfViewer.module.scss uses the legacy token var(--gray-8)
for the loading/error text; replace that token with the semantic theme token
(e.g., var(--color-ui-text-muted) or the project’s equivalent) so it matches the
migrated background token var(--color-page-bg); update the color declaration
that currently reads color: var(--gray-8); to use the chosen semantic token and
ensure any related comments or usages in the PdfViewer component reflect the new
token name.
In `@src/utils/getStyleClassNames.tsx`:
- Around line 50-63: In the border-only branch inside getStyleClassNames.tsx
(the block that checks config.border), add the base variant class before adding
the intent class so themed border styles are applied; specifically, call
classNames.add(variants[config.variant ?? 'solid']) (using the existing variants
lookup) prior to classNames.add(variants[config.intent ?? 'neutral']) and keep
the existing borders/* additions and camelKey usage intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d7530a4-ecb4-4bc5-b847-80459e1dc001
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (94)
.storybook/preview.css.storybook/preview.tsx.stylelintrc.json.vscode/settings.jsonpackage.jsonscripts/generateScssTypes.tsscripts/generateThemeVariables.tssrc/components/AppNavigation/AppNavigation.module.scsssrc/components/AppNavigation/AppNavigation.stories.tsxsrc/components/AppNavigation/AppNavigation.tsxsrc/components/AppNavigation/components/AppLogo/AppLogo.module.scsssrc/components/AppNavigation/components/DesktopNavigation/DesktopNavigation.module.scsssrc/components/AppNavigation/components/DesktopNavigation/DesktopNavigation.module.scss.d.tssrc/components/AppNavigation/components/DesktopRoutes/DesktopRoutes.module.scsssrc/components/AppNavigation/components/DesktopRoutes/DesktopRoutes.module.scss.d.tssrc/components/AppNavigation/components/DesktopRoutes/DesktopRoutes.tsxsrc/components/AppNavigation/components/MobileNavigation/MobileNavigation.module.scsssrc/components/AppNavigation/components/MobileNavigation/MobileNavigation.module.scss.d.tssrc/components/AppNavigation/components/MobileNavigation/MobileNavigation.tsxsrc/components/AppNavigation/components/RouteItem/RouteItem.module.scsssrc/components/AppNavigation/components/RouteItem/RouteItem.tsxsrc/components/Badge/Badge.module.scss.d.tssrc/components/Badge/Badge.stories.tsxsrc/components/Button/Button.stories.tsxsrc/components/Button/Button.tsxsrc/components/Button/stories/ButtonStoryWrapper.module.scsssrc/components/Button/stories/ButtonStoryWrapper.tsxsrc/components/Card/Card.module.scsssrc/components/Card/Card.stories.tsxsrc/components/Card/Card.tsxsrc/components/DialogManager/Dialog.module.scsssrc/components/DialogManager/Dialog.module.scss.d.tssrc/components/DialogManager/Dialog.tsxsrc/components/Drawer/Drawer.module.scsssrc/components/Drawer/Drawer.module.scss.d.tssrc/components/Drawer/Drawer.tsxsrc/components/Hero/Hero.module.scsssrc/components/Hero/Hero.module.scss.d.tssrc/components/InputText/InputText.module.scsssrc/components/InputText/InputText.module.scss.d.tssrc/components/InputText/InputText.tsxsrc/components/PdfViewer/PdfViewer.module.scsssrc/components/PdfViewer/PdfViewer.module.scss.d.tssrc/components/PdfViewer/PdfViewer.stories.tsxsrc/components/PdfViewer/PdfViewer.tsxsrc/components/PdfViewer/components/PdfViewerControls/PdfViewerControls.module.scsssrc/components/PdfViewer/components/PdfViewerControls/PdfViewerControls.module.scss.d.tssrc/components/PdfViewer/components/PdfViewerControls/PdfViewerControls.tsxsrc/components/PdfViewer/components/PdfViewerControls/index.tssrc/components/Radio/Radio.module.scsssrc/components/Radio/Radio.module.scss.d.tssrc/components/Radio/Radio.tsxsrc/components/RatioBar/RatioBar.module.scss.d.tssrc/components/ScrollArea/ScrollArea.module.scsssrc/components/ScrollArea/ScrollArea.module.scss.d.tssrc/components/ScrollArea/ScrollArea.tsxsrc/components/Select/Select.module.scsssrc/components/Select/Select.module.scss.d.tssrc/components/Select/Select.tsxsrc/components/Select/stories/ControlledValueStory.tsxsrc/components/Spinner/Spinner.module.scsssrc/components/Spinner/Spinner.module.scss.d.tssrc/components/Table/Table.module.scsssrc/components/Table/Table.module.scss.d.tssrc/components/Table/TableCell.tsxsrc/components/ThemeProvider/ThemeProvider.context.tssrc/components/ThemeProvider/ThemeProvider.hooks.tssrc/components/ThemeProvider/ThemeProvider.tsxsrc/components/ThemeProvider/ThemeProvider.types.tssrc/components/ThemeProvider/ThemeProvider.utils.tssrc/components/ThemeProvider/index.tssrc/components/ThemeProvider/themes/dark.tssrc/components/ThemeProvider/themes/light.tssrc/components/ThemeProvider/themes/midnight.tssrc/index.tssrc/style/_animate.scsssrc/style/_animate.scss.d.tssrc/style/_backgrounds.scsssrc/style/_layers.scsssrc/style/_shadows.scsssrc/style/_text.scss.d.tssrc/style/borders.module.scsssrc/style/borders.module.scss.d.tssrc/style/index.scsssrc/style/index.scss.d.tssrc/style/shadows.module.scsssrc/style/shadows.module.scss.d.tssrc/style/variables.scsssrc/style/variables.scss.d.tssrc/style/variants.module.scsssrc/style/variants.module.scss.d.tssrc/types.tssrc/utils/getStyleClassNames.tsxsrc/utils/index.ts
💤 Files with no reviewable changes (23)
- src/style/_text.scss.d.ts
- src/components/AppNavigation/components/MobileNavigation/MobileNavigation.module.scss.d.ts
- src/components/Card/Card.module.scss
- src/components/DialogManager/Dialog.module.scss.d.ts
- src/components/Hero/Hero.module.scss.d.ts
- src/components/Drawer/Drawer.module.scss.d.ts
- src/components/Radio/Radio.module.scss.d.ts
- src/components/PdfViewer/components/PdfViewerControls/PdfViewerControls.module.scss.d.ts
- src/components/ScrollArea/ScrollArea.module.scss
- src/components/RatioBar/RatioBar.module.scss.d.ts
- src/components/Select/Select.module.scss.d.ts
- src/components/Spinner/Spinner.module.scss.d.ts
- src/components/Table/Table.module.scss.d.ts
- src/components/AppNavigation/components/DesktopNavigation/DesktopNavigation.module.scss
- src/components/AppNavigation/components/DesktopNavigation/DesktopNavigation.module.scss.d.ts
- src/style/_animate.scss.d.ts
- src/style/borders.module.scss.d.ts
- src/components/InputText/InputText.module.scss.d.ts
- src/style/variables.scss.d.ts
- src/style/index.scss.d.ts
- src/components/AppNavigation/components/DesktopRoutes/DesktopRoutes.module.scss.d.ts
- src/style/_backgrounds.scss
- src/style/_animate.scss
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…outes.module.scss Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Chores