Skip to content

RI-7754 detect system color change #5268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pd-redis merged 5 commits into from
Dec 1, 2025
Merged

RI-7754 detect system color change #5268

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d01faf
add useSystemThemeListener.ts, it should detect system change
pd-redis Nov 28, 2025
843336c
a little defensive coding to ensure that mediaQuery is indeed defined…
pd-redis Nov 28, 2025
bfefbf8
clean up only if mediaQuery exists
pd-redis Dec 1, 2025
324ddfd
refactor: improve system theme listener hook
pd-redis Dec 1, 2025
0c9a64c
refactor: improve system theme listener hook
pd-redis Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions redisinsight/ui/src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import MonacoLanguages from 'uiSrc/components/monaco-laguages'
import AppInit from 'uiSrc/components/init/AppInit'
import { Page, PageBody } from 'uiSrc/components/base/layout/page'
import { useSystemThemeListener } from 'uiSrc/services/hooks/useSystemThemeListener'
import { Pages, Theme } from './constants'
import { themeService } from './services'
import {
Expand Down Expand Up @@ -48,6 +49,7 @@
removePagePlaceholder()
}
}, [serverLoading])
useSystemThemeListener()

Check warning on line 52 in redisinsight/ui/src/App.tsx

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered 

Warning! Not covered statement
return (
<div className="main-container">
<MonacoEnvironmentInitializer />
Expand Down
36 changes: 36 additions & 0 deletions redisinsight/ui/src/services/hooks/useSystemThemeListener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { useCallback, useEffect } from 'react'
import { Theme, THEME_MATCH_MEDIA_DARK } from 'uiSrc/constants'
import { useThemeContext } from 'uiSrc/contexts/themeContext'

let mediaQuery = window.matchMedia?.(THEME_MATCH_MEDIA_DARK)

/**
* Hook that listens to OS system theme changes
* and updates the theme context when user has System theme selected.
*/
export const useSystemThemeListener = () => {
const { usingSystemTheme, changeTheme } = useThemeContext()

const handleSystemThemeChange = useCallback(() => {
usingSystemTheme && changeTheme(Theme.System)
}, [changeTheme, usingSystemTheme])

useEffect(() => {
if (!mediaQuery) {
// Initialize mediaQuery if not done already because window might not be defined when module is loaded
mediaQuery = window.matchMedia?.(THEME_MATCH_MEDIA_DARK)
}
// Only listen if using system theme
if (usingSystemTheme) {
if (!mediaQuery) {
return undefined
}
mediaQuery.addEventListener('change', handleSystemThemeChange)
}
return () => {
if (mediaQuery) {
mediaQuery.removeEventListener('change', handleSystemThemeChange)

Check warning on line 32 in redisinsight/ui/src/services/hooks/useSystemThemeListener.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered 

Warning! Not covered statement
}

Check warning on line 33 in redisinsight/ui/src/services/hooks/useSystemThemeListener.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered 

Warning! Not covered statement

Check warning on line 33 in redisinsight/ui/src/services/hooks/useSystemThemeListener.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🌿 Branch is not covered 

Warning! Not covered branch
}

Check warning on line 34 in redisinsight/ui/src/services/hooks/useSystemThemeListener.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered 

Warning! Not covered statement
}, [usingSystemTheme])

Check warning on line 35 in redisinsight/ui/src/services/hooks/useSystemThemeListener.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered 

Warning! Not covered statement
}
Comment on lines 9 to 25
Copy link
Contributor

@KrumTy KrumTy Dec 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm noticing some issues with this implementation.
The goal is to attach an event listener in case the theme is set to SYSTEM that would monitor the respective media match and change theme acordingly.

  1. handleSystemThemeChange is memoized callback, however its two dependencies are changeTheme (stable, does not change), usingSystemTheme (same as the useEffect one's). Also it is not listed as dependency of the useEffect - not clear that it's supposed to be redefined first or the effect is first, plus it's used as cleanup - hard to follow lifecycle. Finally, since It is not used outside the useEffect and has the same dependencies, it is preferable that it's defined inside the useEffect as well.
  2. The useEffect has the same check as the one in handleSystemThemeChange (usingSystemTheme) - it only adds the event listener if usingSystemTheme is true, meaning the check inside handleSystemThemeChange is redundant
  3. The useEffect returns a cleanup function regardless of weather or not an event was attached

I'm suggesting a much simplified solution that should perform the same task and addresses the point mentioned above:

Suggested change
export const useSystemThemeListener = () => {
const { usingSystemTheme, changeTheme } = useThemeContext()
const handleSystemThemeChange = useCallback(() => {
usingSystemTheme && changeTheme(Theme.System)
}, [changeTheme, usingSystemTheme])
useEffect(() => {
if (!mediaQuery) {
// Initialize mediaQuery if not done already because window might not be defined when module is loaded
mediaQuery = window.matchMedia?.(THEME_MATCH_MEDIA_DARK)
}
// Only listen if using system theme
if (usingSystemTheme) {
if (!mediaQuery) {
return undefined
}
mediaQuery.addEventListener('change', handleSystemThemeChange)
}
return () => {
if (mediaQuery) {
mediaQuery.removeEventListener('change', handleSystemThemeChange)
}
}
}, [usingSystemTheme])
}
export const useSystemThemeListener = () => {
const { usingSystemTheme, changeTheme } = useThemeContext()
useEffect(() => {
const mediaQuery = window.matchMedia?.(THEME_MATCH_MEDIA_DARK)
// Only listen if using system theme
if (usingSystemTheme && mediaQuery) {
const handleSystemThemeChange = () => changeTheme(Theme.System)
mediaQuery.addEventListener('change', handleSystemThemeChange)
return () => {
mediaQuery.removeEventListener('change', handleSystemThemeChange)
}
}
return undefined
}, [usingSystemTheme, changeTheme])
}

Copy link
Contributor Author

@pd-redis pd-redis Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will trigger non-consistent return points warning in tslint, that is why I prefer the more verbose and defensive style I used.

Better safe than sorry

Copy link
Contributor

@KrumTy KrumTy Dec 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, add return undefined in the end like you did, eslint is the not related to my concerns above

(edit: updated suggestion above)
(edit2: in js every void function returns undefined)

Loading