Feat/centralized color#29
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change refactors the application's theme system by centralizing color management into CSS custom properties, migrating from hardcoded color values and a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.css (1)
883-884:⚠️ Potential issue | 🟡 MinorDark-mode regression on search button hovers.
.search-button:hover,.search-clear:hover, and.search-close:hoverall usebackground: rgba(0, 0, 0, 0.05). Against--color-surface(#2a2a2a) in dark mode this hover tint is essentially invisible — previously these were covered bybody.dark-mode-scoped rules that the refactor removed. Swapping to a theme-aware tint (e.g. a new--color-hover-tinttoken, orcurrentColorwith alpha viacolor-mix) keeps hover affordance in both themes.Also applies to: 983-985, 998-1001
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.css` around lines 883 - 884, The hover styles for .search-button:hover, .search-clear:hover and .search-close:hover (and the same rules around the other occurrences) use a hardcoded background rgba(0,0,0,0.05) which is invisible on dark surfaces; replace that hardcoded tint with a theme-aware token or computed color (for example reference a new --color-hover-tint CSS variable or use color-mix()/currentColor with alpha) so the hover is visible in both themes, add a default in :root (e.g. light-mode value) and override it in body.dark-mode (or compute via color-mix(currentColor 10% , transparent) if you prefer), and update the background declarations in the rules for .search-button:hover, .search-clear:hover, and .search-close:hover (and the other affected blocks) to use that token instead of rgba(0,0,0,0.05).
🧹 Nitpick comments (6)
src/App.tsx (3)
537-539: Toggle.darkondocument.documentElementonly.The coding guideline specifies a single root
.darkclass ondocument.documentElement; toggling the class on bothdocumentElementandbodyis redundant (either alone is sufficient to drive the_colors.cssoverrides) and leaves two sources of truth that can drift if one is missed during future updates. Thebody.darkoverride in_colors.cssexists specifically because this code toggles there — dropping the body toggle and thebody.dark/#root.darkselectors in_colors.cssaligns both files with the guideline.♻️ Proposed simplification
useEffect(() => { document.documentElement.classList.toggle("dark", darkMode); - document.body.classList.toggle("dark", darkMode); }, [darkMode]);As per coding guidelines: "Theme switching should use a root
.darkclass ondocument.documentElement, with CSS variables overridden there".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 537 - 539, Remove the redundant body toggle so only the root html element is toggled: update the effect that currently calls document.documentElement.classList.toggle("dark", darkMode) and document.body.classList.toggle("dark", darkMode) to only toggle on document.documentElement based on the darkMode state, and then remove or update any corresponding body.dark or `#root.dark` selectors in _colors.css so all theme overrides live under :root or documentElement.dark instead of body.dark/#root.dark.
1497-1499: Inline-style replacements with CSS variables look correct.All the muted-text / border / color-picker callsites now read from
var(--color-text-muted),var(--color-border), andDEFAULT_CLIENT_COLOR, and the values line up with the light/dark tokens in_colors.css. Nice consolidation.Optional follow-up: the same
{ color: "var(--color-text-muted)", fontSize: "14px", padding: "10px" }empty-state block appears in at least three sections (lines 1497, 1538, 1694, 2023). Extracting a smallEmptyState/HelperTextcomponent or CSS class would remove the remaining inline style duplication.Also applies to: 1538-1540, 1694-1696, 1742-1746, 1824-1830, 1844-1850, 1872-1878, 1900-1906, 1930-1936, 1956-1962, 1982-1988, 2023-2025
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 1497 - 1499, Several identical inline style blocks (e.g., { color: "var(--color-text-muted)", fontSize: "14px", padding: "10px" }) are duplicated across empty-state callsites in App.tsx; extract them into a single reusable UI piece (create an EmptyState or HelperText React component) or a CSS class and replace the repeated inline objects with that component/class. Implement a new EmptyState component that accepts children/props and applies the shared style, then update all usages (the divs showing muted helper text) to render <EmptyState>...</EmptyState> (or use the CSS class) so duplication is removed and styling remains driven by the same CSS variables.
204-204: Consider derivingDEFAULT_CLIENT_COLORfrom the token palette.
#2196F3duplicates the value of--color-infoin_colors.css. Since this constant ultimately feeds into<input type="color">andgetContrastColor(...), it must remain a concrete hex — but adding a short code comment noting the mirror relationship (or moving it next to other palette-adjacent constants) would help future maintainers keep both in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` at line 204, DEFAULT_CLIENT_COLOR is a hardcoded hex that duplicates the CSS token --color-info; update the declaration of DEFAULT_CLIENT_COLOR so it's either derived from the token palette at runtime or annotated to show it's intentionally mirroring --color-info (e.g., add a short code comment referencing --color-info and getContrastColor usage), and move the constant next to other palette-adjacent constants so maintainers see the relationship and keep the hex in sync with the token.src/index.css (2)
259-270: These.darkoverrides duplicate the base rules — safe to delete.Because
--color-surface,--color-border-strong,--color-text, and--color-text-mutedare already re-valued by the.darkblock in_colors.css, the base.app-toast*rules automatically pick up the dark values. These three.darkselectors set the samevar(--...)on the same properties, so they have no effect at runtime and can be removed.♻️ Proposed cleanup
-.dark .app-toast { - background: var(--color-surface); - border-color: var(--color-border-strong); -} - -.dark .app-toast-description { - color: var(--color-text); -} - -.dark .app-toast-close { - color: var(--color-text-muted); -} -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.css` around lines 259 - 270, The three `.dark` override rules (.dark .app-toast, .dark .app-toast-description, .dark .app-toast-close) are redundant because `_colors.css` already redefines the CSS custom properties (`--color-surface`, `--color-border-strong`, `--color-text`, `--color-text-muted`) under `.dark`, so the base `.app-toast*` selectors pick up dark values automatically; remove these three `.dark` blocks from src/index.css to avoid duplicate no-op rules and leave the base `.app-toast`, `.app-toast-description`, and `.app-toast-close` rules intact.
747-747: Route modal backdrops through the new overlay tokens.
_colors.cssintroduces--color-overlayand--color-overlay-strong, but the four modal overlays (.calendar-modal-overlayL747,.search-modal-overlayL892,.search-confirmation-overlayL1230,.reminder-modal-overlayL1276) still hardcodergba(0, 0, 0, 0.5|0.6|0.7). Without this plumbing, the tokens are effectively dead code and dark-mode tuning has to be done in two places..calendar-modal-overlay { - background: rgba(0, 0, 0, 0.5); + background: var(--color-overlay); ... .search-modal-overlay { - background: rgba(0, 0, 0, 0.5); + background: var(--color-overlay); ... .search-confirmation-overlay { - background: rgba(0, 0, 0, 0.7); + background: var(--color-overlay-strong); ... .reminder-modal-overlay { - background: rgba(0, 0, 0, 0.6); + background: var(--color-overlay-strong);Note: this change should be paired with fixing the dark-mode value of
--color-overlay(see_colors.csscomment) so dark-mode backdrops still darken rather than lighten.Also applies to: 892-892, 1230-1230, 1276-1276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.css` at line 747, The modal overlay classes are still using hardcoded rgba values; update each overlay selector (.calendar-modal-overlay, .search-modal-overlay, .search-confirmation-overlay, .reminder-modal-overlay) to use the new CSS tokens instead of literal rgba(...) — e.g. replace background: rgba(...) with background: var(--color-overlay) or where a stronger backdrop was intended use var(--color-overlay-strong); ensure the property name and token spelling (--color-overlay, --color-overlay-strong) match the new _colors.css tokens so dark-mode overrides flow through.src/_colors.css (1)
32-34: Collapse the dark selector to a single root hook.The coding guideline says theme switching should use a root
.darkclass ondocument.documentElement. Listingbody.darkand#root.darkhere commits to supporting three toggle points, which is what forcedApp.tsxto also toggledocument.body(see separate comment there). Narrowing tohtml.dark(or just.darkto allow any ancestor to hook in, including during SSR/testing) keeps a single source of truth.♻️ Proposed diff
-html.dark, -body.dark, -#root.dark { +html.dark {As per coding guidelines: "Theme switching should use a root
.darkclass ondocument.documentElement, with CSS variables overridden there".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_colors.css` around lines 32 - 34, Replace the three separate theme selectors (html.dark, body.dark, `#root.dark`) with a single root hook so theme switching is governed by one class on document.documentElement; update the selector in src/_colors.css to use either html.dark or simply .dark and remove body.dark and `#root.dark` occurrences, then ensure any CSS variable overrides and references that previously relied on body.dark or `#root.dark` now reference the single root selector (search for html.dark, body.dark, `#root.dark` to find all places to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/_colors.css`:
- Around line 23-24: The CSS variables --color-overlay and
--color-overlay-strong are defined but not used and their dark-mode values are
inverted (lightening instead of darkening), so update their dark theme
definitions to dark rgba values (e.g. rgba(0,0,0,0.6/0.7)) and replace the
hardcoded backdrop colors in the modal overlay rules (.calendar-modal-overlay,
.search-modal-overlay, .search-confirmation-overlay, .reminder-modal-overlay in
index.css) to use var(--color-overlay) and var(--color-overlay-strong); also
apply the same fix for the duplicate variables at the other occurrence (lines
noted as 56-57) so theming is centralized and dark-mode overlays remain
darkening.
In `@src/index.css`:
- Line 1282: The rule for .reminder-modal-overlay sets border-color but no
border, so it has no effect; either remove the border-color declaration or
replace it with a full border declaration (e.g., add a border-width and
border-style using the same variable) to make the border render. Locate the
.reminder-modal-overlay selector in src/index.css and either delete the
border-color: var(--color-page-background) line or change it to a proper border
(e.g., border: 1px solid var(--color-page-background)) depending on the intended
visual.
---
Outside diff comments:
In `@src/index.css`:
- Around line 883-884: The hover styles for .search-button:hover,
.search-clear:hover and .search-close:hover (and the same rules around the other
occurrences) use a hardcoded background rgba(0,0,0,0.05) which is invisible on
dark surfaces; replace that hardcoded tint with a theme-aware token or computed
color (for example reference a new --color-hover-tint CSS variable or use
color-mix()/currentColor with alpha) so the hover is visible in both themes, add
a default in :root (e.g. light-mode value) and override it in body.dark-mode (or
compute via color-mix(currentColor 10% , transparent) if you prefer), and update
the background declarations in the rules for .search-button:hover,
.search-clear:hover, and .search-close:hover (and the other affected blocks) to
use that token instead of rgba(0,0,0,0.05).
---
Nitpick comments:
In `@src/_colors.css`:
- Around line 32-34: Replace the three separate theme selectors (html.dark,
body.dark, `#root.dark`) with a single root hook so theme switching is governed by
one class on document.documentElement; update the selector in src/_colors.css to
use either html.dark or simply .dark and remove body.dark and `#root.dark`
occurrences, then ensure any CSS variable overrides and references that
previously relied on body.dark or `#root.dark` now reference the single root
selector (search for html.dark, body.dark, `#root.dark` to find all places to
change).
In `@src/App.tsx`:
- Around line 537-539: Remove the redundant body toggle so only the root html
element is toggled: update the effect that currently calls
document.documentElement.classList.toggle("dark", darkMode) and
document.body.classList.toggle("dark", darkMode) to only toggle on
document.documentElement based on the darkMode state, and then remove or update
any corresponding body.dark or `#root.dark` selectors in _colors.css so all theme
overrides live under :root or documentElement.dark instead of
body.dark/#root.dark.
- Around line 1497-1499: Several identical inline style blocks (e.g., { color:
"var(--color-text-muted)", fontSize: "14px", padding: "10px" }) are duplicated
across empty-state callsites in App.tsx; extract them into a single reusable UI
piece (create an EmptyState or HelperText React component) or a CSS class and
replace the repeated inline objects with that component/class. Implement a new
EmptyState component that accepts children/props and applies the shared style,
then update all usages (the divs showing muted helper text) to render
<EmptyState>...</EmptyState> (or use the CSS class) so duplication is removed
and styling remains driven by the same CSS variables.
- Line 204: DEFAULT_CLIENT_COLOR is a hardcoded hex that duplicates the CSS
token --color-info; update the declaration of DEFAULT_CLIENT_COLOR so it's
either derived from the token palette at runtime or annotated to show it's
intentionally mirroring --color-info (e.g., add a short code comment referencing
--color-info and getContrastColor usage), and move the constant next to other
palette-adjacent constants so maintainers see the relationship and keep the hex
in sync with the token.
In `@src/index.css`:
- Around line 259-270: The three `.dark` override rules (.dark .app-toast, .dark
.app-toast-description, .dark .app-toast-close) are redundant because
`_colors.css` already redefines the CSS custom properties (`--color-surface`,
`--color-border-strong`, `--color-text`, `--color-text-muted`) under `.dark`, so
the base `.app-toast*` selectors pick up dark values automatically; remove these
three `.dark` blocks from src/index.css to avoid duplicate no-op rules and leave
the base `.app-toast`, `.app-toast-description`, and `.app-toast-close` rules
intact.
- Line 747: The modal overlay classes are still using hardcoded rgba values;
update each overlay selector (.calendar-modal-overlay, .search-modal-overlay,
.search-confirmation-overlay, .reminder-modal-overlay) to use the new CSS tokens
instead of literal rgba(...) — e.g. replace background: rgba(...) with
background: var(--color-overlay) or where a stronger backdrop was intended use
var(--color-overlay-strong); ensure the property name and token spelling
(--color-overlay, --color-overlay-strong) match the new _colors.css tokens so
dark-mode overrides flow through.
🪄 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: CHILL
Plan: Pro
Run ID: 8089d7f9-85c5-40de-9761-177cea46ac80
📒 Files selected for processing (5)
AGENTS.mdsrc/App.tsxsrc/_colors.csssrc/index.csssrc/main.tsx
| --color-overlay: rgba(0, 0, 0, 0.5); | ||
| --color-overlay-strong: rgba(0, 0, 0, 0.7); |
There was a problem hiding this comment.
--color-overlay dark value is semantically inverted and the token is currently unused.
In dark mode --color-overlay becomes rgba(255, 255, 255, 0.1) — i.e. a light overlay — whereas modal backdrops in index.css (.calendar-modal-overlay, .search-modal-overlay, .search-confirmation-overlay, .reminder-modal-overlay) all still use hardcoded dark rgba(0, 0, 0, 0.5|0.6|0.7). So:
- The token isn't wired up anywhere (consider routing those overlays through
var(--color-overlay)/var(--color-overlay-strong)to keep theming centralized — see the parallel comment inindex.css). - Once wired up, the dark value should almost certainly remain a darkening color (e.g.
rgba(0, 0, 0, 0.6)) rather than a lightening one, otherwise dark-mode modals will have inverted focus behavior and insufficient contrast against surface content.
Also applies to: 56-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/_colors.css` around lines 23 - 24, The CSS variables --color-overlay and
--color-overlay-strong are defined but not used and their dark-mode values are
inverted (lightening instead of darkening), so update their dark theme
definitions to dark rgba values (e.g. rgba(0,0,0,0.6/0.7)) and replace the
hardcoded backdrop colors in the modal overlay rules (.calendar-modal-overlay,
.search-modal-overlay, .search-confirmation-overlay, .reminder-modal-overlay in
index.css) to use var(--color-overlay) and var(--color-overlay-strong); also
apply the same fix for the duplicate variables at the other occurrence (lines
noted as 56-57) so theming is centralized and dark-mode overlays remain
darkening.
1 participant
Summary by CodeRabbit