Avoid repeated theme DOM sync during startup#2779
Conversation
Skip DOM theme work when the requested theme and system-dark state are already applied. Several components subscribe with useTheme on startup; without this guard they each re-read computed styles and update chrome color state during first render. 10k Chrome first-load click profile, fresh copied fixture, 5 runs, median: | metric | previous | after | delta | |---|---:|---:|---:| | initial render | 3630.273 ms | 3269.955 ms | -360.318 ms (-9.9%) | | click settle | 9687.877 ms | 9654.482 ms | -33.395 ms (-0.3%) | | total | 13357.292 ms | 12924.437 ms | -432.855 ms (-3.2%) | | FunctionCall | 1195.416 ms | 1050.971 ms | -144.445 ms (-12.1%) | | RunTask | 2682.802 ms | 2298.999 ms | -383.803 ms (-14.3%) | | UpdateLayoutTree | 685.519 ms | 564.078 ms | -121.441 ms (-17.7%) | Verification: bun fmt; bun lint; bun typecheck; bun run test.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR optimizes theme application in the web app by caching the last applied theme state and short-circuiting redundant DOM updates.
Changes:
- Added
lastAppliedThemecaching to avoid reapplying the same theme/system-dark combination. - Reused a single
systemDarklookup perapplyThemecall instead of callinggetSystemDark()multiple times.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ApprovabilityVerdict: Approved Performance optimization that adds caching to skip redundant theme DOM updates, plus defensive checks for matchMedia availability. Changes are self-contained with unit tests, and the author has prior contributions to this file. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf9de2903d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Dismissing prior approval to re-evaluate cc88013
What Changed
Made theme application idempotent when
{theme, systemDark}has not changed. RepeateduseTheme()subscribers no longer re-apply the same DOM theme work during startup. Desktop theme sync is still preserved.Why
Several components subscribe through
useTheme()on startup. Before this change, each subscriber could re-read computed styles and update browser chrome color state during first render even when the effective theme had not changed.Chrome first-load click profile against a synthetic 10k-thread home (100 projects x 100 threads, 0 activities/thread, projected seed). Each of the 5 measured runs used a fresh copy of that fixture; table shows medians:
UI Changes
No visual UI changes.
Checklist
Verification:
bun fmtbun lintbun typecheckbun run testNote
Medium Risk
Moderate risk because it changes client-side theme application and event subscription behavior, which could cause subtle regressions in theme switching or system-theme updates across browsers/environments.
Overview
Makes
useThemetheme application idempotent by caching the last applied{theme, systemDark}and skipping repeated DOM class/theme-color updates when nothing effectively changed, while still syncing the desktop bridge theme.Hardens system-theme handling by guarding
matchMediausage (including in the preference-change subscription) so environments withoutwindow.matchMediadon’t throw, and adds Vitest coverage to ensure explicit stored themes don’t require or callmatchMediaon module initialization.Updates formatting config to ignore the
.cachedirectory.Reviewed by Cursor Bugbot for commit cc88013. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Avoid redundant DOM updates in
applyThemeduring startuplastAppliedThemecache in useTheme.ts so repeated calls toapplyThemewith the same effective theme skip class toggling and browser chrome sync work.window.matchMediaaccess with a function check ingetSystemDarkandsubscribe, preventing throws in environments (e.g. SSR or test) where it is undefined.syncBrowserChromeThemeand transition logic are now skipped on repeated identical calls, butsyncDesktopThemestill runs every time.Macroscope summarized cc88013.