Added User theme(Light/Dark) preference detection #1072
Conversation
-> Used window.matchMedia API to get the user's theme preference.
WalkthroughThe changes in this pull request update the initial state of the UI settings in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Client/src/App.jsx (2 hunks)
🧰 Additional context used
🪛 Biome
Client/src/App.jsx
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
Client/src/App.jsx (2)
42-42: Yo, this import's clean!The import statement for
setModeis properly organized with other Redux-related imports.
Line range hint
42-72: Yo, let's verify this theme implementation! 🎯The theme detection looks solid, but let's make sure it's tested properly:
- Test in different browsers
- Verify theme changes in real-time
- Check fallback behavior
✅ Verification successful
The previous command failed because of file type recognition. Let's try these alternative approaches to gather more context about the theme implementation. This will help us verify if the theme detection is properly integrated with the Redux store and if there are any tests covering this functionality.
Theme implementation looks solid, eh! 🍁
The theme implementation is well-structured and robust:
- Theme detection properly integrates with system preferences
- Theme state is managed through Redux with proper actions
- Theme changes are persisted and applied consistently
- Light/dark themes are properly defined with complete styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for other theme-related code that might need updates rg -l "theme|mode|dark|light" --type js --type jsxLength of output: 80
Script:
#!/bin/bash # Let's try a different approach to find theme-related code rg -t js "theme|mode|dark|light" .Length of output: 105711
Script:
#!/bin/bash # Look for UI slice implementation fd -e js -e jsx "uiSlice"Length of output: 59
Script:
#!/bin/bash # Check for any theme-related tests fd -e test.js -e test.jsx -e spec.js -e spec.jsx .Length of output: 817
Script:
#!/bin/bash # Look for Redux store configuration fd "store.js|store.jsx" .Length of output: 49
Script:
#!/bin/bash # Check for theme-related components ast-grep --pattern 'useSelector((state) => state.ui.$_)'Length of output: 1996
🧰 Tools
🪛 Biome
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Client/src/App.jsx
Outdated
| const getPreferredTheme = () => { | ||
| if(window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) { | ||
| return 'dark'; | ||
| } | ||
| return 'light'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo dawg, let's make this function bulletproof! 🔥
The theme detection logic needs some defensive programming:
Here's a more robust implementation:
-const getPreferredTheme = () => {
- if(window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) {
- return 'dark';
- }
- return 'light';
-}
+const getPreferredTheme = () => {
+ try {
+ return window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches
+ ? 'dark'
+ : 'light';
+ } catch {
+ return 'light'; // Fallback for older browsers
+ }
+};Also consider:
- Memoizing this function since it's used in a useEffect
- Adding a listener for theme changes using
addListeneroraddEventListener
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getPreferredTheme = () => { | |
| if(window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) { | |
| return 'dark'; | |
| } | |
| return 'light'; | |
| } | |
| const getPreferredTheme = () => { | |
| try { | |
| return window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches | |
| ? 'dark' | |
| : 'light'; | |
| } catch { | |
| return 'light'; // Fallback for older browsers | |
| } | |
| }; |
🧰 Tools
🪛 Biome
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Client/src/App.jsx
Outdated
| useEffect(() => { | ||
| const theme = getPreferredTheme(); | ||
| dispatch(setMode(theme)); | ||
| },[dispatch]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo, this effect needs some real-time magic! 🎵
The useEffect hook only sets the initial theme but doesn't react to system theme changes.
Here's how to make it responsive:
useEffect(() => {
const theme = getPreferredTheme();
dispatch(setMode(theme));
+
+ const mediaQuery = window?.matchMedia?.('(prefers-color-scheme: dark)');
+ const handleThemeChange = (e) => {
+ dispatch(setMode(e.matches ? 'dark' : 'light'));
+ };
+
+ mediaQuery?.addEventListener?.('change', handleThemeChange);
+
+ return () => {
+ mediaQuery?.removeEventListener?.('change', handleThemeChange);
+ };
},[dispatch]);This will keep your app's theme in sync with system preferences in real-time! 🚀
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const theme = getPreferredTheme(); | |
| dispatch(setMode(theme)); | |
| },[dispatch]); | |
| useEffect(() => { | |
| const theme = getPreferredTheme(); | |
| dispatch(setMode(theme)); | |
| const mediaQuery = window?.matchMedia?.('(prefers-color-scheme: dark)'); | |
| const handleThemeChange = (e) => { | |
| dispatch(setMode(e.matches ? 'dark' : 'light')); | |
| }; | |
| mediaQuery?.addEventListener?.('change', handleThemeChange); | |
| return () => { | |
| mediaQuery?.removeEventListener?.('change', handleThemeChange); | |
| }; | |
| },[dispatch]); |
|
@ajhollid can you take a look at this PR and check if any changes are necessary ? |
Yep, we will take a look at it as soon as we can, it's the weekend so it might be a little while until someone is available to review it. Appreciate your patience, you can also request reviews from @marcelluscaio and @jennifer-gan, they are the front end maintainers so this is their domain. |
Client/src/App.jsx
Outdated
| import { setMode } from "./Features/UI/uiSlice"; | ||
|
|
||
| const getPreferredTheme = () => { | ||
| try { | ||
| return window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches? 'dark': 'light'; | ||
| } | ||
| catch { | ||
| return 'light'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Thank you for your time working on this issue, @aarya-16 !
This is a good path, but doing this in the app directory can bring some performance issues. I suggest you do the check using a ternary inside of the UI slice. so instead of defining the inital theme as light, we will be getting the inital one dynamically at the store level. With that we won't need to call a state update with the use effect.
There was a problem hiding this comment.
I'll implement those changes, @marcelluscaio. I do have one question though:
Do you want me to add the event listeners to handle dynamic user system theme changes in the UI Slice as well ?
There was a problem hiding this comment.
Good point! I don't think we need the event listener. All we want is to assess user's initial preference. The rest is being dealt with through the settings menu. Do you see a scenario in which that would be necessary?
There was a problem hiding this comment.
The only scenario I see is if someone changes their browser or system theme settings while the website is active. But even in that case, a page reload would do the trick. So I guess I'll just make the changes without the event listener.
There was a problem hiding this comment.
Great! I agree with you, I don't think users expect a live reload according to system settings, and we would get loose of an event listener.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Client/src/Features/UI/uiSlice.js (1 hunks)
🔇 Additional comments (1)
Client/src/Features/UI/uiSlice.js (1)
18-18: Hold up! We need to verify browser support! 🤔Let's check if this feature is supported across our target browsers.
✅ Verification successful
Yo, this matchMedia usage is actually pretty solid! 🎤
The
window.matchMedia()andprefers-color-schememedia query have excellent browser support (IE11+). The optional chaining (?.) provides a clean fallback to "light" mode if the feature isn't supported, making this a robust implementation.
- The code gracefully handles cases where:
windowis undefined (SSR scenarios)matchMediaisn't supportedprefers-color-schemeisn't supported- Any of these operations fail
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for any browser compatibility checks or polyfills in the codebase rg -i "matchmedia|prefers-color-scheme"Length of output: 162
Script:
#!/bin/bash # Let's check for any browser compatibility handling or polyfills rg -i "polyfill|fallback|browser.?support|compatibility" -A 3 # Also check package.json for any polyfill dependencies fd package.json --exec cat {} | jq '.dependencies + .devDependencies'Length of output: 11258
Client/src/Features/UI/uiSlice.js
Outdated
| @@ -15,7 +15,7 @@ const initialState = { | |||
| sidebar: { | |||
| collapsed: false, | |||
| }, | |||
| mode: "light", | |||
| mode: window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches? "dark": "light", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
One more thing about that initialization! 💡
Consider extracting the theme detection logic into a separate utility function. This would make it easier to test and maintain.
Create a new file utils/themeDetection.js:
export const getPreferredTheme = () => {
try {
const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
return mediaQuery.matches ? 'dark' : 'light';
} catch {
return 'light';
}
};
export const initializeThemeListener = (callback) => {
try {
const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
mediaQuery.addEventListener('change', e => callback(e.matches ? 'dark' : 'light'));
} catch (err) {
console.warn('Theme detection not supported');
}
};Then in uiSlice.js:
+import { getPreferredTheme } from '../utils/themeDetection';
...
-mode: window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches? "dark": "light",
+mode: getPreferredTheme(),Yo! The theme detection needs some love! 🍝
While the implementation works, there are several improvements we could make to ensure robustness:
- We should add a listener for preference changes
- We need a proper fallback for browsers that don't support matchMedia
- The optional chaining is good but we should be more explicit about the fallback
Here's a more robust implementation:
-mode: window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches? "dark": "light",
+mode: (() => {
+ try {
+ const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
+ // Add listener for theme changes
+ mediaQuery.addEventListener('change', e => {
+ store.dispatch(setMode(e.matches ? 'dark' : 'light'));
+ });
+ return mediaQuery.matches ? 'dark' : 'light';
+ } catch (err) {
+ console.warn('Color scheme detection not supported');
+ return 'light';
+ }
+})(),Committable suggestion was skipped due to low confidence.
|
@marcelluscaio I have moved the theme preference detection to the UISlice file. Please review it. |
Client/src/Features/UI/uiSlice.js
Outdated
| @@ -15,7 +15,7 @@ const initialState = { | |||
| sidebar: { | |||
| collapsed: false, | |||
| }, | |||
| mode: "light", | |||
| mode: window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches? "dark": "light", | |||
There was a problem hiding this comment.
Not sure what happened, but the formatting is not that clear without the space between the first part of the ternary and the question mark. Would you mind extracting the ternary from the object to increase legibility? I suggest saving it to a variable with a name like initialMode or something like that
There was a problem hiding this comment.
@marcelluscaio I have made that change
-> Used window.matchMedia API to get the user's theme preference to solve issue #1026