Add light mode with CSS variables and prevent FOUC#118
Add light mode with CSS variables and prevent FOUC#118yash-pouranik merged 8 commits intogeturbackend:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a light theme variant and client-side initialization: an inline script in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 3
🧹 Nitpick comments (2)
apps/web-dashboard/index.html (1)
17-17: Multiple storage keys risks divergence — pick one canonical key.Reading from four different keys (
theme,color-theme,colorTheme,mode) without defining which the rest of the app writes to is a recipe for stale/conflicting state: the toggle component (yet to be added per the issue) writes to key A, but on reload this script might read a stale value from key B. Please commit to a single key (e.g.,theme) and document it; if the keys are intended for backward compatibility, add a short comment stating which ones are legacy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-dashboard/index.html` at line 17, The storageKeys array (storageKeys) currently contains four different keys ('theme', 'color-theme', 'colorTheme', 'mode') which can cause divergence; update the code to use a single canonical key (e.g., 'theme') everywhere the app reads/writes theme state and remove the other keys from storageKeys, or if those other keys are for backward compatibility, leave them only for an initial migration step and add a short comment marking them as legacy and which key is canonical; ensure any theme toggle component and any read/write helpers reference the canonical key (storageKeys variable or its consumers) so the app consistently reads/writes the same key.apps/web-dashboard/src/index.css (1)
42-42: Scope selector to:root.light-modeto match the issue spec and improve specificity intent.The linked issue calls out
:root.light-mode. Using the bare.light-modeclass works today (the inline script targetsdocument.documentElement), but:root.light-modemakes the intent explicit and guarantees the override only applies to the root token layer, preventing accidental matches if someone later adds a descendant element with that class name.-.light-mode { +:root.light-mode {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-dashboard/src/index.css` at line 42, Update the CSS selector from the generic .light-mode to :root.light-mode so the rule explicitly targets the document root and avoids accidental matches on descendant elements; locate the existing .light-mode selector in index.css and replace it with :root.light-mode to increase specificity and align with the issue spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-dashboard/index.html`:
- Around line 16-26: Wrap the synchronous localStorage reads in a try/catch to
prevent exceptions from aborting inline head scripts: around the loop that uses
storageKeys and calls localStorage.getItem (the storedValue assignment logic),
catch any errors (e.g., SecurityError) and treat them as a non-existent value
(leave storedValue null) so the rest of the script can continue; ensure you only
access localStorage if window and window.localStorage exist and on catch simply
break/continue without throwing.
- Around line 15-36: The script currently defaults to dark when no stored
preference is found; modify the logic that computes shouldUseLightMode (which
uses storageKeys, storedValue, and normalizedValue) to fall back to the OS
preference by checking window.matchMedia('(prefers-color-scheme:
light)').matches when storedValue is falsy/empty, then use that result alongside
the existing normalizedValue checks before calling
document.documentElement.classList.toggle('light-mode', shouldUseLightMode);
ensure the matchMedia branch only runs if no storedValue was discovered so
stored preferences still win.
In `@apps/web-dashboard/src/index.css`:
- Around line 42-65: The .light-mode block only overrides some CSS variables but
many rules still use hardcoded dark colors (notably .skeleton,
::-webkit-scrollbar-thumb, .btn-secondary, .btn-icon:hover, .nav-link:hover,
.glass-card, select/select option, and the entire .auth-shell) so light theme
looks broken; update the stylesheet to replace hardcoded color literals with
semantic custom properties (add vars such as --color-surface-hover,
--color-skeleton-from, --color-skeleton-to, --color-scrollbar-thumb,
--color-select-text, --color-select-bg, etc.), refactor rules that reference
.skeleton, ::-webkit-scrollbar-thumb, .btn-secondary, .btn-icon:hover,
.nav-link:hover, .glass-card, select, and .auth-shell to use those variables,
and then set appropriate light values inside .light-mode so enabling .light-mode
fully switches all surfaces to the light palette.
---
Nitpick comments:
In `@apps/web-dashboard/index.html`:
- Line 17: The storageKeys array (storageKeys) currently contains four different
keys ('theme', 'color-theme', 'colorTheme', 'mode') which can cause divergence;
update the code to use a single canonical key (e.g., 'theme') everywhere the app
reads/writes theme state and remove the other keys from storageKeys, or if those
other keys are for backward compatibility, leave them only for an initial
migration step and add a short comment marking them as legacy and which key is
canonical; ensure any theme toggle component and any read/write helpers
reference the canonical key (storageKeys variable or its consumers) so the app
consistently reads/writes the same key.
In `@apps/web-dashboard/src/index.css`:
- Line 42: Update the CSS selector from the generic .light-mode to
:root.light-mode so the rule explicitly targets the document root and avoids
accidental matches on descendant elements; locate the existing .light-mode
selector in index.css and replace it with :root.light-mode to increase
specificity and align with the issue spec.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3be99773-9dbf-4875-b512-4d98a196bb14
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/web-dashboard/index.htmlapps/web-dashboard/src/index.css
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
aliyasyeddd
changed the title
|
@coderabbitai is this PR ok to merge as a toggle feature that integrates with #114 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me examine both PRs and the code changes in detail. 🏁 Script executed: Length of output: 4686 Let me read the 🏁 Script executed: Length of output: 5292
|
| Issue | Location | Detail |
|---|---|---|
| Dead code | index.html |
normalizedValue === 'true' — ThemeToggle never stores 'true'; it stores 'light'/'dark' |
| Overly broad key scan | index.html |
Scanning 4 generic keys (theme, color-theme, colorTheme, mode) when only urbackend-theme is used |
| Generic key risk | index.html |
'mode' is too generic and could match unrelated localStorage entries from other parts of the app |
| PR checklist | PR description | Lint, responsiveness, and console error checks are unchecked |
✅ What PR #118 Gets Right
- IIFE pattern in
<head>is the correct approach for FOUC prevention — runs synchronously before any render. prefers-color-schemefallback is correct and good UX.- CSS variables are comprehensive and well-organized; all relevant color variables are overridden (dimensions like
--sidebar-widthdon't need to be, and correctly aren't).
✅ PR #114 Code Quality — Good
- Defensive
try/catcharound alllocalStorageandmatchMediacalls. - Proper accessibility:
aria-labelandaria-pressedon the toggle button. - Correct system preference listener with cleanup in
useEffect. getInitialThemeruns synchronously to minimize React-level flash.
Recommended Merge Order
Fix PR `#118` (add 'urbackend-theme' key + clean up dead code)
↓
Merge PR `#118` ← foundational CSS + FOUC prevention
↓
Merge PR `#114` ← the toggle UI that uses those CSS vars
Neither PR is complete in isolation — #118 without #114 has no UI to toggle; #114 without #118 has no light-mode CSS variables and no FOUC protection. They must land together, in order.
|
@coderabbitai I am going to merge #114 and @aliyasyeddd please fix this in next commit. |
|
Tip For best results, initiate chat on the files or code changes.
- var storageKeys = ['theme', 'color-theme', 'colorTheme', 'mode'];
+ var storageKeys = ['urbackend-theme', 'theme', 'color-theme', 'colorTheme', 'mode'];While you're at it, you can also clean up the two minor quality issues flagged earlier:
|
|
@aliyasyeddd please update this whenever u came on |
aliyasyeddd
force-pushed
the
feature/theme-css-fouc
branch
from
05faf3b to
45c5637
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/web-dashboard/src/index.css (1)
42-65:⚠️ Potential issue | 🟠 MajorDefine the new semantic variables before using them.
Many changed rules now reference custom properties that are not declared in either
:rootor.light-modein this file, so declarations likebackground: var(--color-auth-shell-bg)andcolor: var(--color-select-text)become invalid. This can break auth surfaces, skeletons, scrollbars, selects, and hover states in both dark and light themes.Please add dark defaults in
:rootand light overrides in.light-modefor every newly introduced token, or add safevar(..., fallback)values at each use. This is still in the same theme-tokenization area previously flagged.Suggested direction
:root { /* Glassmorphism - Darker & Smoother */ --glass-bg: rgba(10, 10, 10, 0.7); --glass-border: rgba(255, 255, 255, 0.05); --glass-backdrop: blur(12px); + + --color-glass-card-bg: rgba(255, 255, 255, 0.02); + --color-glass-card-border: rgba(255, 255, 255, 0.08); + --color-scrollbar-thumb: `#333333`; + --color-scrollbar-thumb-hover: `#444444`; + --color-surface-hover: rgba(255, 255, 255, 0.05); + --color-surface-hover-strong: rgba(255, 255, 255, 0.08); + --color-skeleton-from: `#1c1c1c`; + --color-skeleton-to: `#2a2a2a`; + --color-select-text: `#ffffff`; + --color-select-bg: `#0f0f0f`; + + /* Add matching dark defaults for every --color-auth-* / text/divider token used below. */ } .light-mode { --color-primary: `#3ECF8E`; --color-primary-hover: `#2fb67a`; @@ --glass-bg: rgba(255, 255, 255, 0.6); --glass-border: rgba(0, 0, 0, 0.08); --glass-backdrop: blur(10px); + + --color-glass-card-bg: rgba(0, 0, 0, 0.04); + --color-glass-card-border: rgba(0, 0, 0, 0.08); + --color-scrollbar-thumb: `#d1d5db`; + --color-scrollbar-thumb-hover: `#9ca3af`; + --color-surface-hover: rgba(0, 0, 0, 0.04); + --color-surface-hover-strong: rgba(0, 0, 0, 0.08); + --color-skeleton-from: `#e5e7eb`; + --color-skeleton-to: `#f3f4f6`; + --color-select-text: `#111111`; + --color-select-bg: `#ffffff`; + + /* Add matching light overrides for every --color-auth-* / text/divider token used below. */ }Also applies to: 117-119, 217-223, 259-267, 291-293, 340-341, 641-649, 653-955
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-dashboard/src/index.css` around lines 42 - 65, The new theme variables (e.g., --color-auth-shell-bg, --color-select-text and any other tokens introduced in the diff) are being referenced before they are declared; add dark-theme defaults in :root and corresponding light overrides in .light-mode for every newly introduced custom property, or alternatively change each usage to use var(--token, <fallback>) so there is always a safe fallback; locate references in the theme-tokenization area and update the :root block and the .light-mode block (and any other theme blocks) to declare the missing variables rather than leaving them undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web-dashboard/src/index.css`:
- Around line 42-65: The new theme variables (e.g., --color-auth-shell-bg,
--color-select-text and any other tokens introduced in the diff) are being
referenced before they are declared; add dark-theme defaults in :root and
corresponding light overrides in .light-mode for every newly introduced custom
property, or alternatively change each usage to use var(--token, <fallback>) so
there is always a safe fallback; locate references in the theme-tokenization
area and update the :root block and the .light-mode block (and any other theme
blocks) to declare the missing variables rather than leaving them undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61d4331c-6ab9-48e7-b115-948ca8fbfe45
📒 Files selected for processing (2)
apps/web-dashboard/index.htmlapps/web-dashboard/src/index.css
✅ Files skipped from review due to trivial changes (1)
- apps/web-dashboard/index.html
|
@yash-pouranik merged the changes |
|
@coderabbitai Please check if we can merge to main? its good to me |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the latest state of PR 🏁 Script executed: Length of output: 11913 Good progress — the critical fix and dead code cleanup are confirmed. Let me do one more check on the CSS variables before giving the green light. 🏁 Script executed: Length of output: 118 🏁 Script executed: Length of output: 3049
🔴 Blocking: ~30 new CSS variables used but never defined in
|
| Area | Undefined variables |
|---|---|
| Glass card | --color-glass-card-bg, --color-glass-card-border |
| Scrollbars | --color-scrollbar-thumb, --color-scrollbar-thumb-hover |
| Buttons / hover states | --color-surface-hover, --color-surface-hover-strong |
| Skeleton loader | --color-skeleton-from, --color-skeleton-to |
| Select dropdowns | --color-select-text, --color-select-bg |
| Auth shell | --color-auth-shell-bg, --color-auth-ambient-secondary |
| Auth brand | --color-auth-brand-mark-bg/text, --color-auth-brand-text |
| Auth mode pill | --color-auth-mode-bg, --color-auth-mode-text |
| Auth typography | --color-auth-heading, --color-text-soft, --color-text-faint, --color-text-placeholder, --color-auth-label |
| Auth inputs | --color-auth-input-border, --color-auth-input-bg, --color-auth-icon, --color-auth-input-toggle-text/hover |
| Auth links/dividers | --color-auth-link, --color-auth-link-hover, --color-auth-divider-text, --color-divider |
| Auth password check | --color-auth-password-text/passed/dot/shadow |
| Auth footer | --color-border-subtle, --color-auth-footer-text, --color-auth-footer-note |
@aliyasyeddd — all of these need dark-mode values added to :root in index.css. Example for a few:
:root {
/* ... existing vars ... */
/* Glass card */
--color-glass-card-bg: rgba(255, 255, 255, 0.02);
--color-glass-card-border: rgba(255, 255, 255, 0.08);
/* Scrollbar */
--color-scrollbar-thumb: `#333`;
--color-scrollbar-thumb-hover: `#444`;
/* Surfaces */
--color-surface-hover: rgba(255, 255, 255, 0.05);
--color-surface-hover-strong: rgba(255, 255, 255, 0.08);
/* Skeleton */
--color-skeleton-from: `#1c1c1c`;
--color-skeleton-to: `#2a2a2a`;
/* Select */
--color-select-text: `#ffffff`;
--color-select-bg: `#0f0f0f`;
/* Auth */
--color-auth-shell-bg: `#050505`;
--color-auth-ambient-secondary: rgba(255, 255, 255, 0.03);
/* ... and so on for the rest */
}The .light-mode block can then override whichever of these need light values.
Also noted: mergeStateStatus is UNSTABLE, suggesting CI checks are failing — worth verifying that too once the CSS is fixed.
|
@aliyasyeddd ?? |
|
@yash-pouranik i am currently making changes |
|
@coderabbitai Bro I think all done? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the latest state of PR 🏁 Script executed: Length of output: 9565
( ˶°ᗜ°˶)ᐝ |
|
@coderabbitai Level 1- 3pt Altough the changes were of level 1 but I think she cooperated so well during the PR so we should consider level2 ? what say |
|
Tip For best results, initiate chat on the files or code changes.
Technical scope (leans Level 1→2 boundary):
This isn't a Level 3 (no new feature architecture, no backend, no complex logic), but it's solidly above a trivial Level 1 change like fixing a typo or tweaking a colour value. Cooperation factor:
Recommendation: Level 2 (5pt) ✅ — justified both on scope and responsiveness. The cooperation tipped a borderline call into a clear one. |
|
Ok thanky @coderabbitai for the suggestions |
|
Tip For best results, initiate chat on the files or code changes.
|
yash-pouranik
added
enhancement
|
@yash-pouranik Thank you so much for the kind words! I really appreciate the support and guidance throughout the process. I’m excited to contribute more and take on bigger tasks going forward. |
2 participants
Fixes #98
🛠️ Type of Change
🧪 Testing & Validation(Checking)
Frontend Verification:
npm run lintin thefrontend/directory.✅ Checklist
Built with ❤️ for urBackend.
Summary by CodeRabbit