🐛 fix(dark-mode): defer media query initialization to afterNextRenderer for SSR#446
🐛 fix(dark-mode): defer media query initialization to afterNextRenderer for SSR#446Luizgomess merged 2 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant Page as DarkmodePage (Component)
end
rect rgba(200,255,200,0.5)
participant Service as ZardDarkMode (Service)
end
rect rgba(255,240,200,0.5)
participant Matcher as MediaMatcher / MQL (Browser)
end
Page->>Service: construct / first render -> call init() or rely on afterNextRender
Service-->>Service: afterNextRender -> ensureQueryInitialized()
Service->>Matcher: create MediaQueryList("prefers-color-scheme: dark")
Matcher-->>Service: initial matches -> set systemDark signal
Matcher->>Service: change events -> handleThemeChange() -> update systemDark / themeMode
Service->>Page: computed themeMode changes -> applyTheme() -> update DOM (.dark) / bindings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/zard/src/lib/shared/services/dark-mode.ts (2)
27-46: 🧹 Nitpick | 🔵 TrivialRedundant DOM update during initialization.
The
effect(lines 29–36) fires afterinitializeTheme()completes because bothinitializedandthemeSignalchange. ButinitializeTheme()already callsupdateThemeMode()directly at line 109. This means on first initialization the DOM toggle happens twice — once explicitly, then again via the effect.Consider either removing the direct
updateThemeModecall at line 109 (letting the effect handle it), or moving theinitialized.set(true)to the very end ofinitializeTheme()after the explicit update, and restructuring so the effect only reacts to subsequentthemeSignalchanges. The cleanest approach is to drop lines 108-109 entirely and let the effect be the single source of truth for DOM updates.♻️ Suggested simplification in initializeTheme()
private initializeTheme(): void { const storedTheme = this.getStoredTheme(); if (storedTheme) { this.themeSignal.set(storedTheme); } if (!storedTheme || storedTheme === EDarkModes.SYSTEM) { this.handleSystemChanges(); } this.initialized.set(true); - - const theme = this.themeSignal(); - this.updateThemeMode(this.isDarkModeActive(theme), theme); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/zard/src/lib/shared/services/dark-mode.ts` around lines 27 - 46, The initialization currently triggers updateThemeMode twice (once directly in initializeTheme() and again via the reactive effect), so remove the explicit DOM update inside initializeTheme(): delete the direct call to updateThemeMode(...) in initializeTheme() and keep setting initialized(true) there so the existing effect (effect(() => { if (!this.initialized()) return; const theme = this.themeSignal(); ... this.updateThemeMode(...) })) becomes the single source of truth for applying the theme; locate initializeTheme(), updateThemeMode(), initialized, themeSignal, and effect to make this change and ensure afterNextRender/handleSystemChanges logic remains unchanged.
49-55:⚠️ Potential issue | 🔴 CriticalGuard
isDarkModeActive()against uninitialized media query to prevent throws on earlytoggleTheme()calls.During the first render on browser,
themeMode()can throw if accessed beforeafterNextRendercompletes. SincethemeMode()is read at line 76 bytoggleTheme(), early user interactions or pre-initialization access will hit:themeMode()→isDarkModeActive(SYSTEM)→this.query.matches→ throws because_queryis undefined.Recommended fix
private isDarkModeActive(currentTheme: DarkModeOptions): boolean { if (!this.isBrowser) { return false; } - - return currentTheme === EDarkModes.DARK || (currentTheme === EDarkModes.SYSTEM && this.query.matches); + if (!this._query) { + return false; + } + return currentTheme === EDarkModes.DARK || (currentTheme === EDarkModes.SYSTEM && this._query.matches); }This prevents the throwing getter access and safely defaults to light mode (consistent with SSR) until initialization completes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/zard/src/lib/shared/services/dark-mode.ts` around lines 49 - 55, themeMode's computed getter can throw because isDarkModeActive reads the uninitialized media query; update the guard so isDarkModeActive returns false (i.e., treat as light) when the media-query object is not yet initialized instead of accessing its .matches. Specifically, in the isDarkModeActive method (and/or the internal _query accessor used by it), check for the presence of the media-query object (e.g., this._query or this.query) and return false if missing; this prevents themeMode() and toggleTheme() from throwing during early calls before afterNextRender initializes the query.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/zard/src/lib/shared/services/dark-mode.ts`:
- Around line 57-66: The duplicated setup of the media query and onDestroy
handler in init() and afterNextRender() should be extracted into a private
helper to avoid repetition: create a private method (e.g. ensureQueryInitialized
or setupQuery) that checks this._query, assigns this._query =
this.mediaMatcher.matchMedia('(prefers-color-scheme: dark)') and registers
this.destroyRef.onDestroy(() => this.handleSystemChanges(false)); then replace
the duplicated blocks in init() and afterNextRender() with a call to that new
method so _query and destroyRef wiring live in one place.
---
Outside diff comments:
In `@libs/zard/src/lib/shared/services/dark-mode.ts`:
- Around line 27-46: The initialization currently triggers updateThemeMode twice
(once directly in initializeTheme() and again via the reactive effect), so
remove the explicit DOM update inside initializeTheme(): delete the direct call
to updateThemeMode(...) in initializeTheme() and keep setting initialized(true)
there so the existing effect (effect(() => { if (!this.initialized()) return;
const theme = this.themeSignal(); ... this.updateThemeMode(...) })) becomes the
single source of truth for applying the theme; locate initializeTheme(),
updateThemeMode(), initialized, themeSignal, and effect to make this change and
ensure afterNextRender/handleSystemChanges logic remains unchanged.
- Around line 49-55: themeMode's computed getter can throw because
isDarkModeActive reads the uninitialized media query; update the guard so
isDarkModeActive returns false (i.e., treat as light) when the media-query
object is not yet initialized instead of accessing its .matches. Specifically,
in the isDarkModeActive method (and/or the internal _query accessor used by it),
check for the presence of the media-query object (e.g., this._query or
this.query) and return false if missing; this prevents themeMode() and
toggleTheme() from throwing during early calls before afterNextRender
initializes the query.
mikij
force-pushed
the
fix/dark-mode-ssr-friendly
branch
from
9c0ca39 to
3dfed2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/zard/src/lib/shared/services/dark-mode.ts (1)
91-95:⚠️ Potential issue | 🟡 MinorMisleading error message — "on server" but also throws in the browser before initialization.
The message
'Cannot access media query on server'conflates two distinct cases. After thethemeModebug is fixed (no longer accessingqueryfrom the computed), this getter should only be reachable post-init in the browser, but the message still misguides anyone who hits it during development.🔧 Suggested fix
- throw new Error('Cannot access media query on server'); + throw new Error('MediaQueryList is not initialized. Ensure ensureQueryInitialized() has been called before accessing the query.');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/zard/src/lib/shared/services/dark-mode.ts` around lines 91 - 95, The getter query currently throws a misleading message ('Cannot access media query on server') when either running on server or when the service hasn't been initialized; update the thrown Error in the query getter to a clear message such as "MediaQueryList not available: either running on server or not initialized" (or similar) and ensure the conditional still checks both this.isBrowser and this._query so callers of the query getter (referenced as the query getter and the private _query field) get an accurate error indicating either server context or missing initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/zard/src/lib/shared/services/dark-mode.ts`:
- Around line 26-32: themeMode's computed currently calls
isDarkModeActive(SYSTEM) which accesses this.query (backed by _query) before
ensureQueryInitialized() runs, and it doesn't react to OS changes because
query.matches isn't tracked; fix by adding a new reactive signal (e.g.,
systemDark) initialized/updated in ensureQueryInitialized() and
handleThemeChange(), have isDarkModeActive read systemDark (or consult
systemDark in the computed) instead of accessing this.query directly, ensure
ensureQueryInitialized() creates the MediaQueryList and sets systemDark to its
current matches, and update handleThemeChange() to set systemDark when the OS
preference changes and then call updateThemeMode so themeMode stays correct and
no throw occurs when _query is undefined.
---
Outside diff comments:
In `@libs/zard/src/lib/shared/services/dark-mode.ts`:
- Around line 91-95: The getter query currently throws a misleading message
('Cannot access media query on server') when either running on server or when
the service hasn't been initialized; update the thrown Error in the query getter
to a clear message such as "MediaQueryList not available: either running on
server or not initialized" (or similar) and ensure the conditional still checks
both this.isBrowser and this._query so callers of the query getter (referenced
as the query getter and the private _query field) get an accurate error
indicating either server context or missing initialization.
3dfed2f to
2d3cc72
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/zard/src/lib/shared/services/dark-mode.ts (1)
42-49: 🧹 Nitpick | 🔵 TrivialConsider
afterRenderEffectfor reactive DOM writes instead ofeffect.
effect()runs before Angular updates the DOM, so for situations requiring direct DOM inspection or manipulation it's more appropriate to useafterRenderEffect, which functions likeeffectbut runs after Angular has finished rendering and committed its changes.
updateThemeModewrites directly todocument.documentElement— a DOM write — makingafterRenderEffectthe idiomatic Angular v21 alternative:♻️ Proposed refactor using afterRenderEffect
- effect(() => { - if (!this.initialized()) { - return; - } - const theme = this.themeSignal(); - const isDarkMode = this.isDarkModeActive(theme); - this.updateThemeMode(isDarkMode, theme); - }); + afterRenderEffect({ + write: () => { + if (!this.initialized()) { + return; + } + const theme = this.themeSignal(); + const isDarkMode = this.isDarkModeActive(theme); + this.updateThemeMode(isDarkMode, theme); + }, + });Note:
afterRenderEffectalso runs browser-only, so the existingisBrowserguard on the enclosingifblock remains correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/zard/src/lib/shared/services/dark-mode.ts` around lines 42 - 49, The current reactive callback uses effect() but performs DOM writes in updateThemeMode (which mutates document.documentElement); replace effect with afterRenderEffect so the callback runs after Angular has rendered. Locate the effect block that checks this.initialized(), reads this.themeSignal(), calls this.isDarkModeActive(theme) and then this.updateThemeMode(isDarkMode, theme) and change it to use afterRenderEffect with the same body, preserving the existing isBrowser/initialized guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/zard/src/lib/shared/services/dark-mode.ts`:
- Around line 51-57: The afterNextRender call in dark-mode.ts currently uses the
default mixedReadWrite phase; change it to call afterNextRender with the
explicit 'write' phase to avoid layout thrashing—i.e., replace the bare
afterNextRender(() => { ... }) with afterNextRender('write', () => { ... })
around the block that checks this.initialized() and calls
this.ensureQueryInitialized() and this.initializeTheme(), leaving the existing
initialization logic and guards intact.
---
Outside diff comments:
In `@libs/zard/src/lib/shared/services/dark-mode.ts`:
- Around line 42-49: The current reactive callback uses effect() but performs
DOM writes in updateThemeMode (which mutates document.documentElement); replace
effect with afterRenderEffect so the callback runs after Angular has rendered.
Locate the effect block that checks this.initialized(), reads
this.themeSignal(), calls this.isDarkModeActive(theme) and then
this.updateThemeMode(isDarkMode, theme) and change it to use afterRenderEffect
with the same body, preserving the existing isBrowser/initialized guards.
2d3cc72 to
7ef1e72
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/zard/src/lib/shared/services/dark-mode.ts (1)
134-141:⚠️ Potential issue | 🟡 Minor
handleSystemChanges(false)without_queryguard logs a misleading warning.When
applyTheme(EDarkModes.LIGHT | EDarkModes.DARK)is called beforeafterNextRenderfires (e.g.,toggleThemetriggered programmatically before first render),_queryisundefined. Line 139 callshandleSystemChanges(false), which hits thequerygetter, throws'MediaQueryList not available…', and the catch at line 180 logs"Failed to manage media query event listener"— a false alarm since no listener was ever registered.Also, line 136's
this._query!.matchesis safe post-ensureQueryInitialized(), but unlike the first-init path whereensureQueryInitialized()already seedssystemDark, this line only does real work when_querywas already live. A brief comment explaining why both lines 135–136 coexist would aid clarity.🛡️ Proposed fix — guard `handleSystemChanges` with an early exit
private handleSystemChanges(addListener = true): void { + if (!this._query) { + return; // listener never registered; nothing to add or remove + } try { if (addListener) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/zard/src/lib/shared/services/dark-mode.ts` around lines 134 - 141, applyTheme calls handleSystemChanges(false) when switching away from SYSTEM even if _query is undefined, causing the query getter to throw and log a misleading warning; fix by adding a guard in handleSystemChanges (or before calling it) that returns early if this._query is not set (check this._query or the query getter safely), and add a short comment near applyTheme (lines around ensureQueryInitialized, this.systemDark.set(...)) explaining why ensureQueryInitialized() seeds systemDark on first-init while the subsequent this._query!.matches path is for the already-initialized case so future calls can skip initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/zard/src/lib/shared/services/dark-mode.ts`:
- Around line 21-25: handleThemeChange currently sets this.systemDark and then
calls updateThemeMode directly, causing duplicate DOM updates because the
existing effect() already reacts to systemDark changes via isDarkModeActive;
remove the direct call to this.updateThemeMode(...) from the handleThemeChange
method so it only does this.systemDark.set(event.matches) and lets the effect()
(which reads isDarkModeActive and uses systemDark) perform the DOM update once.
---
Outside diff comments:
In `@libs/zard/src/lib/shared/services/dark-mode.ts`:
- Around line 134-141: applyTheme calls handleSystemChanges(false) when
switching away from SYSTEM even if _query is undefined, causing the query getter
to throw and log a misleading warning; fix by adding a guard in
handleSystemChanges (or before calling it) that returns early if this._query is
not set (check this._query or the query getter safely), and add a short comment
near applyTheme (lines around ensureQueryInitialized, this.systemDark.set(...))
explaining why ensureQueryInitialized() seeds systemDark on first-init while the
subsequent this._query!.matches path is for the already-initialized case so
future calls can skip initialization.
7ef1e72 to
f7184be
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/zard/src/lib/shared/services/dark-mode.ts`:
- Around line 27-33: Add a one-line JSDoc above the readonly computed property
themeMode to clarify its contract: explain that themeMode derives from
themeSignal() and resolves SYSTEM via systemDark(), so it will only ever return
EDarkModes.LIGHT or EDarkModes.DARK (never EDarkModes.SYSTEM). Reference the
computed property name themeMode, the source signal themeSignal(), the enum
EDarkModes, and the resolver method systemDark() in the comment so callers
understand the transformation.
- Around line 129-131: Replace the redundant optional chaining when reading the
MediaQueryList after calling ensureQueryInitialized(): use this._query!.matches
instead of this._query?.matches ?? false to assert the invariant set by
ensureQueryInitialized() (refer to ensureQueryInitialized and _query); also add
a brief clarifying comment on the same line explaining that this assignment to
systemDark (systemDark.set(...)) is only necessary on re-entry when switching
back to EDarkModes.SYSTEM and may look like a duplicate when _query was just
created.
mikij
force-pushed
the
fix/dark-mode-ssr-friendly
branch
from
f7184be to
c351656
Compare
|
waiting for #447 to be merged first!!! |
2 participants
What was done? 📝
The dark mode service now uses
afterNextRenderto defer browser-only media query initialization, making it compatible with Angular SSR without requiringngSkipHydration.Screenshots or GIFs 📸
|-----Figma-----|
|-----Implementation-----|
Link to Issue 🔗
Type of change 🏗
Breaking change 🚨
Checklist 🧐
Summary by CodeRabbit