Conversation
Reviewer's GuideRefactors utility helpers for DOM/script/link management and updates getTheme to optionally bypass localStorage and always resolve concrete theme values, while performing minor style cleanups. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
getTheme, theuseLocalstorageparameter is already defaulted totrue, so the lineuseLocalstorage = useLocalstorage ?? true;is redundant and can be removed to avoid unnecessary reassignment of the parameter. - The updated
getThemenow maps a stored or DOMthemevalue of'auto'togetAutoThemeValue()instead of returning'auto'as before; if some callers still need the raw'auto'value, consider either documenting this behavioral change clearly or adding an option to return the original value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getTheme`, the `useLocalstorage` parameter is already defaulted to `true`, so the line `useLocalstorage = useLocalstorage ?? true;` is redundant and can be removed to avoid unnecessary reassignment of the parameter.
- The updated `getTheme` now maps a stored or DOM `theme` value of `'auto'` to `getAutoThemeValue()` instead of returning `'auto'` as before; if some callers still need the raw `'auto'` value, consider either documenting this behavioral change clearly or adding an option to return the original value.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7874 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34162 34162
Branches 4703 4703
=========================================
Hits 34162 34162
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates the JavaScript theme utility to change what getTheme() returns, intended to address theme initialization issues (linked to #7871) and bumps the package version.
Changes:
- Modify
getTheme()inutility.js(new parameter + new resolution behavior for'auto'). - Minor JS formatting/comment cleanup in
utility.js. - Bump
BootstrapBlazorpackage version to10.5.1-beta08.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/wwwroot/modules/utility.js | Changes getTheme() behavior/signature and applies minor JS formatting/comment removals. |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Version bump to 10.5.1-beta08. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function getTheme(useLocalstorage = true) { | ||
| useLocalstorage = useLocalstorage ?? true; | ||
| let theme = null; | ||
| if (useLocalstorage) { | ||
| theme = localStorage.getItem('theme'); | ||
| } | ||
| else { | ||
| theme = document.documentElement.getAttribute('data-bs-theme'); | ||
| } | ||
|
|
||
| if (theme === null || theme === 'auto') { | ||
| theme = getAutoThemeValue(); | ||
| } | ||
| return theme; |
There was a problem hiding this comment.
getTheme() now resolves a stored/attribute value of 'auto' into 'dark'/'light'. However ThemeProvider.razor.js calls getTheme() when ThemeValue === 'useLocalStorage' and then immediately calls setTheme(currentTheme, true) (which persists back to localStorage). With this change, a previously saved 'auto' preference will be overwritten as 'dark' or 'light', breaking future auto-switching on prefers-color-scheme changes.
Also, when useLocalstorage is true and localStorage has no value (or an empty string), the function no longer falls back to data-bs-theme like the previous implementation did.
Consider keeping getTheme() returning the persisted theme value ('auto'|'dark'|'light'|null) and adding an explicit option/new API to return the effective theme (resolving 'auto' via getAutoThemeValue()), then update only the consumers that need the effective theme.
| export function getTheme(useLocalstorage = true) { | ||
| useLocalstorage = useLocalstorage ?? true; | ||
| let theme = null; | ||
| if (useLocalstorage) { |
There was a problem hiding this comment.
Parameter name useLocalstorage is inconsistent with the established localStorage casing (and with the existing useLocalStorage string used by ThemeValue.UseLocalStorage). Renaming it to useLocalStorage would improve readability and reduce confusion when this exported API is used directly from JS.
| export function getTheme(useLocalstorage = true) { | |
| useLocalstorage = useLocalstorage ?? true; | |
| let theme = null; | |
| if (useLocalstorage) { | |
| export function getTheme(useLocalStorage = true) { | |
| useLocalStorage = useLocalStorage ?? true; | |
| let theme = null; | |
| if (useLocalStorage) { |
Link issues
fixes #7871
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Adjust utility helpers and theme retrieval behavior in the BootstrapBlazor front-end module.
New Features:
Enhancements: