-
Notifications
You must be signed in to change notification settings - Fork 427
RI-7754: improve system theme handling #5248
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.
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
Conversation
Code Coverage - Frontend unit tests
Test suite run success5410 tests passing in 701 suites. Report generated by 🧪jest coverage report action from 3d68098 |
- Add useRef pattern to ThemeComponent for stable context access - Add isValidTheme type guard with runtime string check - Use type guard in ThemeProvider for better type safety
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Jit has detected 1 important finding in this PR that you should review.
The finding is detailed below as a comment.
It’s highly recommended that you fix this security issue before merge.
Repository Risks:
- Critical Severity Findings: Indicates that the resource has critical severity security findings that need immediate action.
- Internally Accessible: Accessible only within the internal network, reducing exposure to external threats but still requiring proper controls.
- Database Integration: Connects to a database, often involving sensitive data that must be securely managed.
- Production: Critical as it operates in a live production environment, directly impacting users and business operations.
Repository Context:
graph LR
GitHub$Repository_U23_redis/RedisInsight["GitHub Repository<br/>redis/RedisInsight"]:::GitHub$Repository
DBIntegration_U23_redis["DBIntegration<br/>redis"]:::DBIntegration
GitHub$Actions_U23_pipeline_U2D_build_U2D_windows_U2E_yml["GitHub Actions<br/>pipeline-build-windows.yml"]:::GitHub$Actions
GitHub$Repository_U23_redis/RedisInsight -- "Is accessible to" --> DBIntegration_U23_redis
GitHub$Repository_U23_redis/RedisInsight -- "Has" --> GitHub$Actions_U23_pipeline_U2D_build_U2D_windows_U2E_yml
016ff09 to
c0716a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: System theme change listener completely removed instead of improved
The ThemeComponent that listened for OS theme changes via window.matchMedia(THEME_MATCH_MEDIA_DARK).addEventListener('change', handler) has been completely deleted rather than improved as described in the PR. This removes the ability for the app to automatically respond when users change their OS light/dark mode preference while having "System" theme selected. The PR description mentions adding checks for usingSystemTheme state, but instead the entire listener functionality was removed without being moved to ThemeProvider or elsewhere.
redisinsight/ui/src/App.tsx#L51-L52
RedisInsight/redisinsight/ui/src/App.tsx
Lines 51 to 52 in 3d68098
| return ( | |
| <div className="main-container"> |
What
In
ThemeComponentthe conditional to change theme if it is system is applied only ifthemeContext.usingSystemThemeis true.This is required, because this component - which is meant to load theme from localstorage on app load, is also listening for changes in the browser theme query. Which is trigerred when we change theme in settings. And we get a race condition, when we change from
SYSTEMtoLIGHT, we actually apply the theme BEFORE updating localStorage, and event hadler is trigerred and sees old value -systemand changes it right away todark(which is my system theme)Key changes:
useThemeContextcustom hook with error boundary for better developer experienceThemeComponentto use the new custom hook instead ofuseContextdirectlyusingSystemThemestateeventparameter from theme change handlerTesting
To verify the changes:
Note
Remove ThemeComponent and its test, introduce
useThemeContextandisValidTheme, and validate theme selection during initialization.isValidThemeguard for stored/query theme validation and use it during initialization.useThemeContexthook for consuming theme context.ThemeComponentimport/usage fromui/src/App.tsx.components/theme/ThemeComponent.spec.tsx.components/theme/ThemeComponent.tsx.Written by Cursor Bugbot for commit 3d68098. This will update automatically on new commits. Configure here.