perf: memoize MUI theme to prevent slow re-renders on mode switch #3106
Conversation
SohamBhattacharjee2003
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Theme & UI client/src/App.jsx |
Imported useMemo; created a memoized theme that selects lightTheme or darkTheme from mode; replaced inline theme prop in ThemeProvider with the memoized theme; added ToastContainer to render tree. |
Estimated code review effort
🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
🐰 I nibble code and tweak the theme,
Light or dark — no frantic re-dream,
A memo here, a toast to cheer,
Hops are swift and renders clear,
Happy rabbit, faster stream! 🥕✨
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the primary change: memoizing the MUI theme to fix slow re-renders on mode switch, matching the changeset's core objective. |
| Linked Issues check | ✅ Passed | The PR successfully addresses issue #3098 by memoizing the theme to prevent unnecessary re-renders when switching between light and dark modes, directly solving the reported performance issue. |
| Out of Scope Changes check | ✅ Passed | All changes are directly related to the performance issue: added useMemo import, memoized theme computation, updated ThemeProvider prop, and added ToastContainer (likely needed for the feature context). |
| Description check | ✅ Passed | The PR description addresses the template requirements with a clear explanation of the performance issue and fix; however, several checklist items remain unchecked and a code formatting check was not completed. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @client/src/App.jsx:
- Line 1: The PR incorrectly references issue #123 (unrelated to theme
memoization); update the PR description (and any other occurrences noted) to
reference the correct issue number or remove the "Fixes #123" line entirely;
check the commit messages and PR body where the import of useMemo/useEffect or
theme changes are discussed (symbols: useMemo, useEffect) and ensure the
corrected issue reference appears consistently in those places (also fix lines
called out at 17-19 and 29) so the PR links to the appropriate issue or has no
incorrect issue link.
🧹 Nitpick comments (1)
client/src/App.jsx (1)
39-39: Minor formatting improvement.The extra newline before the export statement follows common conventions for code organization.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/App.jsx
🔇 Additional comments (2)
client/src/App.jsx (2)
29-29: Clean implementation.The usage of the memoized theme in ThemeProvider is correct and maintains the same external behavior.
17-19: Remove the unnecessaryuseMemowrapper.Since
lightThemeanddarkThemeare static objects created at module load time (viacreateTheme()in their respective files), the ternary operator already returns the same object reference whenmodehasn't changed. TheuseMemoprovides no practical benefit and adds unnecessary complexity.
client/src/App.jsx
Outdated
| @@ -1,4 +1,4 @@ | |||
| import { useEffect } from "react"; | |||
| import { useEffect , useMemo } from "react"; | |||
There was a problem hiding this comment.
PR references incorrect issue number.
The PR description states "Fixes #123," but issue #123 is about implementing a forgot-password feature, which is completely unrelated to theme memoization. Please update the PR description to reference the correct issue number or remove the reference if no issue exists.
Also applies to: 17-19, 29-29
🤖 Prompt for AI Agents
In @client/src/App.jsx at line 1, The PR incorrectly references issue #123
(unrelated to theme memoization); update the PR description (and any other
occurrences noted) to reference the correct issue number or remove the "Fixes
#123" line entirely; check the commit messages and PR body where the import of
useMemo/useEffect or theme changes are discussed (symbols: useMemo, useEffect)
and ensure the corrected issue reference appears consistently in those places
(also fix lines called out at 17-19 and 29) so the PR links to the appropriate
issue or has no incorrect issue link.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions.
This PR memoizes the MUI theme to prevent slow re-renders on mode switch, improving performance, but introduces potential compatibility issues with theme providers and lacks input validation.
🌟 Strengths
- Performance optimization through memoization reduces unnecessary re-renders.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P2 | client/src/App.jsx | Performance | Theme memoization may cause MUI styling inconsistencies. | |
| P2 | client/src/App.jsx | Architecture | Mixing theme providers risks compatibility issues. | symbol:ThemeProvider |
| P2 | client/src/App.jsx | Maintainability | Lack of mode validation could lead to unexpected UI. | |
| P2 | PR Metadata | Documentation | Unfilled description obscures context and testing. |
🔍 Notable Themes
- Ensure consistent use of MUI theme providers to avoid cross-component styling issues.
- Add validation for state values to improve robustness and prevent silent failures.
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: N/A
Speculative: The PR description appears to be a template that wasn't properly filled out before submission. It references "Fixes #123" but the linked issue #123 is about "forgot password" functionality, which is completely unrelated to the theme memoization changes in this PR. This makes it difficult to understand the actual context, motivation, or any related performance issues being addressed. A proper description would help reviewers understand the performance problem being solved and any testing performed.
Related Code:
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| const theme = useMemo(() => { | ||
| return mode === "light" ? lightTheme : darkTheme; | ||
| }, [mode]); |
There was a problem hiding this comment.
P2 | Confidence: High
The use of useMemo here is a correct performance optimization that prevents unnecessary recreation of the theme object on every render, reducing re-render calculations in the component tree below. However, this optimization is incomplete because the ThemeProvider component from @emotion/react is being used, while MUI components expect the theme from @mui/material/styles. This creates a potential mismatch where the memoized theme object might not be consumed correctly by all MUI components, possibly causing subtle styling inconsistencies. The change fixes one performance issue but may introduce a functional regression.
Code Suggestion:
// If using MUI v5+, ensure you're using the correct ThemeProvider
import { ThemeProvider } from '@mui/material/styles';
// Rest of imports remain
// The useMemo implementation is correct
const theme = useMemo(() => {
return mode === "light" ? lightTheme : darkTheme;
}, [mode]);
// Use the MUI ThemeProvider
return (
<ThemeProvider theme={theme}>
{/* children */}
</ThemeProvider>
);
P2 | Confidence: Medium
The useMemo dependency array correctly includes only [mode], which is the minimal dependency set. However, the code assumes mode is always either "light" or "dark" without any validation or fallback. If the Redux state ever contains an unexpected value (null, undefined, or another string), the ternary operator will evaluate to darkTheme for any falsy/truthy value respectively, which may lead to unexpected UI behavior. While not a breaking change, this reinforces existing fragile logic.
Code Suggestion:
const theme = useMemo(() => {
// Add explicit validation with a sensible default
if (mode === "light") return lightTheme;
if (mode === "dark") return darkTheme;
// Fallback to light theme or handle error appropriately
console.warn(`Unexpected theme mode: "${mode}", defaulting to light`);
return lightTheme;
}, [mode]);
| import { useEffect , useMemo } from "react"; | ||
| import { useSelector } from "react-redux"; | ||
| import "react-toastify/dist/ReactToastify.css"; | ||
| import { ToastContainer } from "react-toastify"; |
There was a problem hiding this comment.
[Contextual Comment]
This comment refers to code near real line 5. Anchored to nearest_changed(4) line 4.
P2 | Confidence: High
The PR mixes theme providers from different libraries (@emotion/react vs @mui/material). While Emotion is the styling engine for MUI, MUI v5+ provides its own ThemeProvider wrapper that ensures proper theme propagation and compatibility with MUI components like CssBaseline. Using @emotion/react's ThemeProvider directly bypasses MUI's theme context layer, which may cause issues with:
- MUI component theming (especially nested theme scenarios)
- The
CssBaselinecomponent receiving the correct theme context - Future MUI theme-related features or customizations
Code Suggestion:
// Use MUI's ThemeProvider instead
import { ThemeProvider } from '@mui/material/styles';
// Or for better compatibility with both Emotion and MUI:
// import { ThemeProvider } from '@mui/material';
Evidence: symbol:ThemeProvider
|
@SohamBhattacharjee2003 can you come up with a before/after video? |
|
*Before*
When switching between Light and Dark mode, the MUI theme object was
recreated on every toggle. This caused a full re-render of the component
tree, leading to noticeable UI lag and delayed interactions, especially in
setups with a large number of configured monitors (≈500+). Chrome
Performance profiling showed long scripting and rendering tasks during the
mode switch.
*After*
The theme is now memoized and recalculated only when the mode value
actually changes. This significantly reduces unnecessary re-renders across
the application. As a result, theme switching is now instant and smooth,
with reduced scripting time and improved overall UI responsiveness, even
with a high number of monitors configured.
So, If you give me time till tomorrow so I can make a video
…On Sat, 10 Jan 2026 at 01:05, Gorkem Cetin (BWL) ***@***.***> wrote:
*gorkem-bwl* left a comment (bluewave-labs/Checkmate#3106)
<#3106 (comment)>
@SohamBhattacharjee2003 <https://github.com/SohamBhattacharjee2003> can
you come up with a before/after video?
—
Reply to this email directly, view it on GitHub
<#3106 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A536KHF6DC4EDNOWYX7YZ334F77GNAVCNFSM6AAAAACRGXRWV2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOMZQGMYDSMJRGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/App.jsx (1)
34-34: Addtheme={mode}to align ToastContainer styling with the app's light/dark theme.
Themodevariable is available from Redux state and already used for the main theme. Applying it to ToastContainer ensures consistent notification styling across light/dark modes.Proposed change
- <ToastContainer /> + <ToastContainer theme={mode} />
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/App.jsx
🔇 Additional comments (3)
client/src/App.jsx (3)
1-1: ReactuseMemoimport is fine; just ensure it’s actually needed.
IflightTheme/darkThemeare already singletons (module-levelcreateThemeresults), theuseMemohere is mostly redundant but harmless.
29-29: ThemeProvider now receives a stablethemereference permode— that’s the right direction for perf.
15-20: No issue found —lightThemeanddarkThemeare stable objects, not factory functions.Both exports are objects created once at module load via
createTheme()and exported directly. TheuseMemodependency onmodeis appropriate and will not cause silent breaks or unnecessary rerenders from theme object identity changes.Likely an incorrect or invalid review comment.
Yes, tomorrow is fine. |
|
Uploaded the videos |
Br0wnHammer
left a comment
Br0wnHammer
left a comment
There was a problem hiding this comment.
@SohamBhattacharjee2003 please see my review.
| const mode = useSelector((state) => state.ui.mode); | ||
|
|
||
| // Cleanup | ||
| const theme = useMemo(() => { |
There was a problem hiding this comment.
The profiles show mixed results: scripting actually increased (1,199ms → 1,863ms) while INP stayed at 129ms. The useMemo here is just adding overhead since lightTheme and darkTheme are pre-existing constants, not objects being recreated. If there's a real perf issue with 591 monitors, the fix lies elsewhere, likely in memoizing the monitor components themselves or virtualization.
There was a problem hiding this comment.
3 participants
(Please remove this line only before submitting your PR. Ensure that all relevant items are checked before submission.)
Describe your changes
This PR addresses a performance issue observed when switching between light and dark mode in the Settings page.
Previously, the MUI theme object was recreated on every theme toggle, which caused unnecessary and expensive re-renders across the application. This resulted in noticeable UI lag, especially when a large number of monitors were configured.
The theme is now memoized so it only recalculates when the theme mode actually changes, significantly improving UI responsiveness and overall performance.
Write your issue number after "Fixes "
Fixes #3098
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):-[x] I have not included any files that are not related to my pull request, including package-lock and package-json if dependencies have not changed
-[ ] I didn't use any hardcoded values (otherwise it will not scale, and will make it difficult to maintain consistency across the application).
-[ ] I made sure font sizes, color choices etc are all referenced from the theme. I don't have any hardcoded dimensions.
-[x] My PR is granular and targeted to one specific feature.
-[ ] I ran npm run format in server and client directories, which automatically formats your code.
-[ ] I took a screenshot or a video and attached to this PR if there is a UI change.
Summary by CodeRabbit
Release Notes
New Features
Performance
✏️ Tip: You can customize this high-level summary in your review settings.
📹 Before Performance Demo
This video shows the behavior before the optimization.
When switching between Light and Dark mode on the Settings page, the UI becomes noticeably slow and laggy, especially with many monitors configured.
before.mov
📹 After Performance Demo
After memoizing the MUI theme in
App.jsx, switching between Light and Dark mode is instant and smooth with no visible lag.After.mov