Repository navigation
feat: theme aware map styling - #419
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughReplaces static map initial state with a theme-aware, context-driven initialization flow: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant KeplerSlice
participant ThemeUtils
participant KeplerReducer
participant MapStore
Client->>KeplerSlice: createMap(request)
KeplerSlice->>ThemeUtils: getTheme() / getResolvedTheme(...)
ThemeUtils-->>KeplerSlice: ResolvedTheme
KeplerSlice->>KeplerSlice: createDefaultMapKeplerState(resolvedTheme)
KeplerSlice->>KeplerReducer: createKeplerReducer({reason: 'create-map', mapId, name})
KeplerReducer-->>KeplerSlice: reducer(instance)
KeplerSlice->>KeplerReducer: reducer(undefined, initAction)
KeplerReducer-->>MapStore: initial map state
MapStore-->>Client: map created with theme-aware initial state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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}
Contributor
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)
packages/kepler/src/KeplerSlice.ts (1)
261-347:⚠️ Potential issue | 🟠 MajorReintroduce a
kepler.destroy()lifecycle hook to preserve Room teardown behavior.With this slice no longer exposing
destroy,packages/room-store/src/BaseRoomStore.ts:48-82andpackages/room-store/src/BaseRoomStore.ts:167-177won’t invoke Kepler cleanup during room destroy/HMR. That can leave stale Kepler state/resources across hot reload cycles.♻️ Suggested fix
export type KeplerSliceState = { kepler: { @@ initialize: () => Promise<void>; + destroy: () => Promise<void>; @@ kepler: { @@ async initialize() { @@ }, + async destroy() { + set((state) => + produce(state, (draft) => { + for (const mapId of Object.keys(draft.kepler.map)) { + draft.kepler.map = keplerReducer( + draft.kepler.map, + deleteEntry(mapId), + ); + delete draft.kepler.forwardDispatch[mapId]; + } + draft.kepler.__reduxProviderStore = undefined; + }), + ); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kepler/src/KeplerSlice.ts` around lines 261 - 347, Add a kepler.destroy() lifecycle method to the KeplerSlice that performs the same cleanup BaseRoomStore expects on room destroy/HMR: implement a destroy method on the returned kepler object that (1) cancels or stops any ongoing sync/requests (eg. calls/flags used by syncKeplerDatasets/requestMapStyle), (2) clears forwardDispatch, (3) sets __reduxProviderStore to undefined, (4) resets kepler.map to {} (or keplerGlReducer.initialState if preferred) and (5) removes any subscriptions/listeners created in initialize; reference the KeplerSlice functions/fields (initialize, syncKeplerDatasets, requestMapStyle, forwardDispatch, __reduxProviderStore, kepler.map) and invoke those cleanup steps inside kepler.destroy so BaseRoomStore’s destroy/HMR paths will correctly teardown Kepler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kepler/src/KeplerSlice.ts`:
- Around line 60-66: In getBasemapForTheme, guard the call to window.matchMedia
before invoking it by checking that typeof window !== 'undefined' && typeof
window.matchMedia === 'function' (or window.matchMedia != null) so environments
without matchMedia won't throw; if matchMedia is unavailable, treat 'system' as
a default (e.g., 'light') or use a sensible fallback, then map the resolvedTheme
to the basemap string as before.
---
Outside diff comments:
In `@packages/kepler/src/KeplerSlice.ts`:
- Around line 261-347: Add a kepler.destroy() lifecycle method to the
KeplerSlice that performs the same cleanup BaseRoomStore expects on room
destroy/HMR: implement a destroy method on the returned kepler object that (1)
cancels or stops any ongoing sync/requests (eg. calls/flags used by
syncKeplerDatasets/requestMapStyle), (2) clears forwardDispatch, (3) sets
__reduxProviderStore to undefined, (4) resets kepler.map to {} (or
keplerGlReducer.initialState if preferred) and (5) removes any
subscriptions/listeners created in initialize; reference the KeplerSlice
functions/fields (initialize, syncKeplerDatasets, requestMapStyle,
forwardDispatch, __reduxProviderStore, kepler.map) and invoke those cleanup
steps inside kepler.destroy so BaseRoomStore’s destroy/HMR paths will correctly
teardown Kepler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e83a4c1-d9cc-40df-889f-ed8868476771
📒 Files selected for processing (2)
packages/kepler/src/KeplerSlice.tspackages/kepler/src/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/kepler/src/KeplerSlice.ts (1)
65-71:⚠️ Potential issue | 🟡 MinorGuard
window.matchMediabefore calling it.Line 70 still assumes
matchMediaexists wheneverwindowdoes. In tests, jsdom, and some embedded browser contexts that can throw duringcreateKeplerSlice()initialization instead of falling back to a default theme.Suggested fix
const resolvedTheme = theme === 'system' ? typeof window !== 'undefined' && + typeof window.matchMedia === 'function' && window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light' : theme;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kepler/src/KeplerSlice.ts` around lines 65 - 71, The getBasemapForTheme function calls window.matchMedia unguarded; update it to check that typeof window !== 'undefined' AND typeof window.matchMedia === 'function' (or window.matchMedia !== undefined) before invoking it, so when window exists but matchMedia is missing (e.g., jsdom/tests) it falls back to a safe default theme (e.g., 'light') and continues to return the correct basemap string; adjust the resolvedTheme logic inside getBasemapForTheme (and any callers like createKeplerSlice initialization) to use this guarded check and fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kepler/src/KeplerSlice.ts`:
- Around line 302-304: The code resolves the basemap once (using
getBasemapForTheme(getCurrentThemeFromDOM())) so existing maps'
kepler.map[*].mapStyle.styleType never update on theme changes; to fix,
subscribe to theme changes (or expose a theme-change handler) and, when theme
updates, iterate existing maps (kepler.map.forEach or equivalent) and set each
map.mapStyle.styleType = getBasemapForTheme(newTheme) and trigger the map
update/refresh path you already use in createMap so the new style is applied;
update KeplerSlice where mapStyle is initialized and/or augment createMap to
register that theme-change listener and clean it up on teardown.
---
Duplicate comments:
In `@packages/kepler/src/KeplerSlice.ts`:
- Around line 65-71: The getBasemapForTheme function calls window.matchMedia
unguarded; update it to check that typeof window !== 'undefined' AND typeof
window.matchMedia === 'function' (or window.matchMedia !== undefined) before
invoking it, so when window exists but matchMedia is missing (e.g., jsdom/tests)
it falls back to a safe default theme (e.g., 'light') and continues to return
the correct basemap string; adjust the resolvedTheme logic inside
getBasemapForTheme (and any callers like createKeplerSlice initialization) to
use this guarded check and fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b509e0a9-c3d1-47e2-822d-1995ee4a2a84
📒 Files selected for processing (1)
packages/kepler/src/KeplerSlice.ts
Comment on lines
+302
to
+304
| mapStyle: { | ||
| styleType: getBasemapForTheme(getCurrentThemeFromDOM()), | ||
| } as MapStyle, |
Contributor
There was a problem hiding this comment.
This only themes newly created state, not existing maps.
The basemap is resolved once for the initial state and once inside createMap(). After that, theme changes only update the HTML class in packages/ui/src/theme/theme-provider.tsx:91-108; nothing here propagates the new resolved theme to kepler.map[*].mapStyle.styleType, so already-open maps stay on the old basemap after a theme toggle or a system dark/light change.
Also applies to: 499-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kepler/src/KeplerSlice.ts` around lines 302 - 304, The code resolves
the basemap once (using getBasemapForTheme(getCurrentThemeFromDOM())) so
existing maps' kepler.map[*].mapStyle.styleType never update on theme changes;
to fix, subscribe to theme changes (or expose a theme-change handler) and, when
theme updates, iterate existing maps (kepler.map.forEach or equivalent) and set
each map.mapStyle.styleType = getBasemapForTheme(newTheme) and trigger the map
update/refresh path you already use in createMap so the new style is applied;
update KeplerSlice where mapStyle is initialized and/or augment createMap to
register that theme-change listener and clean it up on teardown.
Collaborator
Author
There was a problem hiding this comment.
this is ok, we don't want to change theme of other maps, just the newly created ones
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/theme/theme-provider.tsx (1)
80-87: Consider guardingwindow.matchMediabefore invoking it.While
windowis checked for SSR,matchMediamay not exist in certain test environments (e.g., Jest/jsdom without mocking). If tests don't mockmatchMedia, this will throw.🛡️ Optional defensive fix
export function getResolvedTheme(theme: Theme): ResolvedTheme { if (theme !== 'system') return theme; if (typeof window === 'undefined') return 'light'; - return window.matchMedia('(prefers-color-scheme: dark)').matches + return typeof window.matchMedia === 'function' && + window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/theme-provider.tsx` around lines 80 - 87, getResolvedTheme currently calls window.matchMedia without ensuring matchMedia exists, which can throw in some test/envs; update the getResolvedTheme function to guard access by checking typeof window.matchMedia === 'function' (or Boolean(window.matchMedia)) before invoking it and return a safe fallback ('light' or the non-dark branch) when matchMedia is unavailable so SSR/tests won't throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/theme/theme-provider.tsx`:
- Around line 80-87: getResolvedTheme currently calls window.matchMedia without
ensuring matchMedia exists, which can throw in some test/envs; update the
getResolvedTheme function to guard access by checking typeof window.matchMedia
=== 'function' (or Boolean(window.matchMedia)) before invoking it and return a
safe fallback ('light' or the non-dark branch) when matchMedia is unavailable so
SSR/tests won't throw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0f19456-b9e6-4f5f-ad84-4f0a1eabf90e
📒 Files selected for processing (5)
docs/upgrade-guide.mdpackages/kepler/src/KeplerSlice.tspackages/kepler/src/index.tspackages/ui/src/index.tspackages/ui/src/theme/theme-provider.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kepler/src/index.ts
Signed-off-by: Ilya Boyandin <[email protected]>
ilyabo
approved these changes
Mar 6, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/theme/theme-provider.tsx (1)
19-22: Consider exportingGetThemeOptionsfor consumers.The
GetThemeOptionstype is used in the public API (getThemePreferenceandgetThemefunction signatures) but is not exported. Consumers who want to type their options when calling these functions would benefit from having this type available.♻️ Suggested change
-type GetThemeOptions = { +export type GetThemeOptions = { defaultTheme?: Theme; storageKey?: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/theme-provider.tsx` around lines 19 - 22, The GetThemeOptions type is used in the public API (getThemePreference and getTheme) but isn't exported; make it part of the public types by exporting it (e.g., add `export` to GetThemeOptions) so consumers can import and use this type when calling getThemePreference/getTheme; ensure the exported name matches the existing references (GetThemeOptions) and update any barrel/index files if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/theme/theme-provider.tsx`:
- Around line 19-22: The GetThemeOptions type is used in the public API
(getThemePreference and getTheme) but isn't exported; make it part of the public
types by exporting it (e.g., add `export` to GetThemeOptions) so consumers can
import and use this type when calling getThemePreference/getTheme; ensure the
exported name matches the existing references (GetThemeOptions) and update any
barrel/index files if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6161d5e-20ce-4246-8d82-88250094c2d5
📒 Files selected for processing (1)
packages/ui/src/theme/theme-provider.tsx
dmitriy-kostianetskiy
pushed a commit
that referenced
this pull request
Mar 9, 2026
Signed-off-by: Ilya Boyandin <[email protected]> Co-authored-by: Ilya Boyandin <[email protected]>
bdjulbic
added a commit
that referenced
this pull request
Mar 16, 2026
Signed-off-by: Ilya Boyandin <[email protected]> Co-authored-by: Ilya Boyandin <[email protected]> Signed-off-by: bdjulbic <[email protected]>
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
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.
New Features
Breaking Changes
Summary by CodeRabbit
New Features
Documentation