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 |
github-actions
Bot
added
size:S
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.
| const systemDark = getSystemDark(); | ||
| if (lastAppliedTheme?.theme === theme && lastAppliedTheme.systemDark === systemDark) { | ||
| syncDesktopTheme(theme); | ||
| return; | ||
| } |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved Simple performance optimization that caches the last applied theme to avoid redundant DOM synchronization during startup. The open review comment is a minor efficiency suggestion, not a correctness issue. You can customize Macroscope's approvability policy. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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".
|
|
||
| function applyTheme(theme: Theme, suppressTransitions = false) { | ||
| if (typeof document === "undefined" || typeof window === "undefined") return; | ||
| const systemDark = getSystemDark(); |
There was a problem hiding this comment.
applyTheme now calls getSystemDark() unconditionally, so environments where window.matchMedia is unavailable (older/embedded webviews or test runtimes without a polyfill) will throw before any theme is applied. Before this change, getSystemDark() was only evaluated for theme === "system", so explicit "light"/"dark" themes did not depend on matchMedia; this commit introduces a new runtime failure path for those themes.
Useful? React with 👍 / 👎.
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
Low Risk
Low risk performance optimization in theme application logic; main risk is stale theme state if the idempotency check misses a real change (e.g., system dark mode transitions).
Overview
Makes theme application idempotent by caching the last applied
{theme, systemDark}and short-circuiting repeatedapplyTheme()calls that would otherwise re-toggle DOM classes and recompute browser chrome colors.When the effective theme is unchanged, it now skips DOM/theme-color updates while still running desktop bridge sync to keep the native app theme in step.
Reviewed by Cursor Bugbot for commit bf9de29. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Skip redundant DOM theme sync on repeated
applyThemecalls during startupAdds a module-level
lastAppliedThemecache in useTheme.ts to memoize the last applied theme and system dark mode state. WhenapplyThemeis called with the same theme and unchanged system dark mode, it skips DOM class toggling and transition suppression, calling onlysyncDesktopTheme. Behavioral Change: repeated calls that previously modified DOM state now no-op on the class/transition side.Macroscope summarized bf9de29.