fix(theme): native chrome + menu + popups follow app pref at runtime#400
Merged
Conversation
Round-1 of review-driven refactor for the theme-switch bug. Closes the
gap where:
- The system theme is dark but the user picks Light in the app menu,
and the native title bar + menu bar + popup dropdowns stay dark.
- Switching theme via the View menu does not refresh the title bar.
- Opening a new window after a theme change shows a freshly-built
window with the wrong-themed menu.
## Root causes
1. The set_theme IPC only persisted to disk and updated Zustand /
<html data-theme>. The native side (DWM title bar, muda HMENU,
Win32 popup menus, NSApp.appearance) was never updated post-build.
2. WebviewWindowBuilder::theme() at builder time only sets Windows
DWM immersive-dark-mode. It does NOT route through muda (per-window
HMENU) and does NOT flip the process-wide popup-menu mode.
3. AppHandle::set_theme on Windows forwards to event_loop.set_theme
which posts a per-window message. tao's update_theme reads
window_state.preferred_theme.or(event_loop_preferred_theme),
short-circuiting on Some. Our builder sets it to Some(...), so
the event-loop value is silently ignored - the title bar never
repaints on runtime theme switch.
4. muda's set_theme_for_hwnd paints the menu BAR but not the dropdown
popups - those are USER32 HMENUs drawn with GetSysColor(COLOR_MENU)
and follow the OS theme unless the process calls the undocumented
uxtheme SetPreferredAppMode with ForceDark / ForceLight and
FlushMenuThemes.
## Fix
- New commands/theme.rs module owning the dispatcher chokepoint,
ThemeApplier trait (production: AppHandle; tests: mock that records
calls), apply_theme_to_window post-build helper, and a Windows-only
popup_theme submodule that calls uxtheme ordinals 135 + 136 to
override the process popup-menu mode.
- set_theme IPC now iterates app.webview_windows() calling per-window
WebviewWindow::set_theme (bypasses the broken event-loop
short-circuit; updates per-window DWM bit + muda HMENU + forces
WM_NCACTIVATE redraw), then flips the process popup mode.
Cross-window propagation: single IPC iterates all windows, so
menu-theme events are now Targeted (firing window) instead of
Broadcast (every renderer re-firing the IPC). This collapses O(N^2)
IPC x window calls to O(N).
- lib.rs build_main_window + create_app_window now use
resolve_persisted_theme(app) - a SINGLE disk read returning bg +
theme + raw_pref - instead of resolve_window_bg + persisted_theme_pref
separately (eliminates duplicate onboarding.json read on the
cold-start hot path).
- macOS ordering: apply_theme_to_window is called AFTER app.set_menu(...)
in setup() so NSApp.appearance flips with the global menu attached.
## Tests
- New commands::theme::tests: 4 theme_to_tauri mapping cases, 4
ThemeApplier dispatch cases (mock records each invocation -
regression guard against silent removal of the load-bearing
applier.apply_theme(...) line), 8 resolve_window_bg_with branch
cases, detect_os_theme smoke.
- Updated menu_test.rs routing pin to assert theme-* is Targeted (was
Broadcast). Renamed the bug-1 flip-side test to a Targeted regression
guard with explicit O(N^2) -> O(N) rationale.
- New source-text guards in main-window-menu-at-build-time.test.ts:
(a) apply_theme_to_window is called AFTER builder.build() in both
window factories, (b) on macOS the call happens AFTER
app.set_menu(...) in setup.
## File-size budgets (rule 23)
- config.rs: 495 -> 293 lines (under 400; net reduction).
- theme.rs: new, 522 total / ~350 production (under 400).
- lib.rs: 1218 -> 1229 (+11; already on watchlist, change is net
additive to the macOS post-set_menu block).
## Docs
docs/features/{settings,app-chrome}.md describe the new runtime
native-theme propagation and the Win32 IFileDialog OS-limitation (not
themable per-app - same constraint VS Code / Chrome / Office hit on
Windows; accepted under Non-Goals).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…window DOM test
Addresses all blocking findings from the 4 round-2 reviewers
(arch-pattern-r2 / security-perf-r2 / test-coverage-r2 / platform-quirks-r2)
plus a new native E2E spec.
## Blocking fixes
1. macOS duplicate disk read (arch-r2 Issue 2 HIGH + security-perf-r2
Finding 1 MEDIUM regression). build_main_window now returns
(WebviewWindow, PersistedTheme); setup() reuses the returned `pref`
on macOS instead of re-reading onboarding.json post-set_menu. This
closes the regression introduced by round-1's macOS ordering fix.
2. popup_theme FFI zero coverage (test-r2 NEW GAP 1 HIGH). Adds a
Windows-only smoke test that exercises every PreferredAppMode
variant of popup_theme::apply (Light/Dark/None + a second pass to
verify OnceLock caching). The test guards against crashes from a
future regression in the ordinal numbers, transmute signatures, or
OnceLock init.
3. apply_theme_to_window error-swallow source-text guard (test-r2
Finding 2 residual MEDIUM). Adds a structural assertion that the
function body uses `if let Err(e)` + `log::warn!` and never
`.unwrap()` / `.expect()`. Prevents a future maintainer from
"improving" the function to panic-on-error and breaking cold-start
for users hitting a transient construction race.
4. Cross-window DOM update test for theme (test-r2 NEW GAP 2 MEDIUM).
New useCrossWindowPrefsSync test composes both useCrossWindowPrefsSync
and useApplyTheme, fires a synthetic storage event, and asserts
`<html data-theme>` actually updates. Previously only the Zustand
side of the chain was tested.
## Should-fix
5. Native E2E spec for theme runtime toggle (test-r2 Finding 4
STILL-OUTSTANDING HIGH). New e2e/native/10-theme-runtime-toggle.spec.ts:
- 10.1 drives set_theme("dark"/"light"/"system"), verifies
onboarding.json persists each pref, and polls `<html data-theme>`
via the real binary.
- 10.2 asserts the IPC rejects garbage with a typed InvalidTheme
error (closed-enum validator contract).
Documents what the spec CANNOT cover (Win32 DWM/HMENU/popup colour
introspection requires Win32 APIs Playwright can't drive; round-1
source-text guards cover the structural call chain).
## Documentation improvements
- theme.rs module preamble: explicit ownership boundary vs config.rs
(validation + disk writes in config.rs; runtime application +
cold-start resolver in theme.rs). (arch-r2 Issue 5)
- set_theme IPC rustdoc: explicit "Side-effect IPC (Rust-First pattern)"
label so the renderer-invisible native mutation is documented; new
"Windows system-mode limitation" block describing the menu-bar
snapshot vs popup-menu auto-tracking asymmetry. (arch-r2 Issue 6 +
platform-r2 Finding 2 documentation gap)
- popup_theme module rustdoc: explicit Windows version requirement
(ordinals 135 + 136 stable on Win10 1903+) and DLL planting analysis
(uxtheme.dll is a Known DLL so LoadLibraryA is safe; at parity with
tao). (platform-r2 NIT findings)
- create_app_window post-build comment: clarifies why no macOS ordering
concern for secondary windows (NSApp.appearance already stable from
main-window setup). (arch-r2 Issue 3)
## File-size budgets (rule 23)
- config.rs: 293 lines (under 400) - no change from round-1.
- theme.rs: 636 total / ~400 production (the new tests + smoke +
source-text guard add ~50 test lines + ~25 doc lines). Production
still under 400.
- lib.rs: 1251 (was 1229 round-1, was 1218 baseline). +22 from round-1
for the macOS-dupread refactor (returning PersistedTheme, suppressing
unused-var on non-macOS). Already on watchlist.
## Verification
- cargo test: 773 lib + 19 commands::theme + all integration tests
passing.
- vitest: 2349 passing (was 2348 round-1; +1 for new cross-window
DOM update test).
- tsc --noEmit: exit 0.
## Outstanding review notes (accepted / deferred)
- platform-r2 Finding 2 (Windows "system" mode menu-bar snapshot):
documented as a known limitation in the IPC rustdoc. Fixing
would require a WM_SETTINGCHANGE listener in Rust that re-fires
set_theme when the OS theme changes and the user pref is "system".
Tracked as a follow-up. Asymmetry is small (only the menu bar
stays stale; chrome + popups + renderer DOM all track the OS).
- platform-r2 Finding 3 (useApplyTheme prefers-color-scheme): not a
bug, verified that the media query fires in both WebView2 and
WKWebView when the OS theme changes.
- security-perf-r2 macOS per-window iteration redundancy: optional
optimization, deferred. Per-window WebviewWindow::set_theme on macOS
IS idempotent (Objective-C setAppearance: on NSApp); the loop costs
N message sends which is negligible.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The native E2E spec was asserting that <html data-theme> updates after
invoking set_theme via __TAURI_INTERNALS__.invoke(...) directly. That
assertion is wrong by design: the Rust set_theme IPC writes disk and
mutates native chrome but does NOT update Zustand. The renderer's
useThemePref::setTheme hook is what writes to Zustand; the IPC alone
bypasses the renderer state machine, so data-theme never flips and
the poll times out.
The renderer-side propagation chain (storage event -> Zustand ->
useApplyTheme -> data-theme) is already covered by the vitest test
useCrossWindowPrefsSync.test.ts added in the round-2 commit.
This spec now covers what only an end-to-end native test can cover:
- 10.1 verifies set_theme IPC persists each valid pref (dark, light,
system) to onboarding.json - read back via onboarding_state IPC.
- 10.2 verifies set_theme rejects garbage with the typed
InvalidTheme error (closed-enum validator contract).
Documents the scope (CAN/CANNOT) inline so future maintainers don't
re-introduce the wrong-mental-model assertions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round-2 added the Side-effect IPC + O(N^2) -> O(N) + Windows system-mode limitation blocks to the set_theme rustdoc. specta picks these up as JSDoc on the TypeScript binding, so bindings.ts needed regeneration to stay in sync. The bindings-drift CI gate is asserting this lockstep. Generated via: cd src-tauri ; MDOWNREVIEW_GEN_BINDINGS_ONLY=1 cargo run --features codegen Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reported by user during dev-mode testing:
Root causes
set_themeIPC only persisted to disk. Renderer-side updated, but the Win32 DWM title bar, the muda HMENU, the popup menus, and macOS NSApp.appearance were never touched post-build.WebviewWindowBuilder::theme()at build time only flips Windows DWM immersive-dark-mode. Does NOT route through muda (HMENU theme) and does NOT set the process-wide popupSetPreferredAppMode. So newly-built windows always had wrong-themed menus when app pref disagreed with the OS.AppHandle::set_themeon Windows is silently no-op for our case. Forwards toevent_loop.set_theme(process-wide), which lands in tao'supdate_themereadingwindow_state.preferred_theme.or(event_loop_preferred_theme). Per-windowpreferred_theme(set via.theme(Some(_))at builder time) short-circuits the.or(...), so the event-loop value is ignored — title bar never repaints on runtime theme switch.muda only paints the menu BAR, not popup dropdowns. Dropdowns are USER32-drawn
HMENUs usingGetSysColor(COLOR_MENU). To override per-app we need the undocumenteduxtheme.dll!SetPreferredAppMode(ordinal 135) +FlushMenuThemes(ordinal 136) — same trick Windows Terminal, Notepad++, VS Code use.The fix
New
src-tauri/src/commands/theme.rs(split out ofconfig.rs)ThemeAppliertrait (production:AppHandleiterateswebview_windows()+ calls per-windowWebviewWindow::set_theme+ on Windows flips popup mode; tests: mock that records every invocation — regression guard mirroringmenu.rs::MenuEmitter).dispatch_set_theme(applier, pref)— pure adapter, testable without anAppHandle.set_themeIPC: persist viaconfig::set_theme_at, thendispatch_set_theme(&app, &theme). Side-effect IPC explicitly labeled in rustdoc.resolve_persisted_theme(app) -> PersistedTheme { bg, theme, raw_pref }— single disk read shared between the builder lookup and the post-build helper.apply_theme_to_window(window, native)— per-window post-build helper.popup_theme(Windows-only) — lazy-loadeduxtheme.dllFFI for ordinals 135 + 136 withOnceLockcaching and graceful no-op degradation on Win10 < 1903.lib.rsbuild_main_windowreturns(WebviewWindow, PersistedTheme)so macOSsetup()reusespref(no second disk read).create_app_windowapplies the theme post-build for the freshly-attached muda menu.setup():apply_theme_to_windowis called AFTERapp.set_menu()so NSApp.appearance flips with the global menu attached.menu.rsBroadcasttoTargeted(firing_label). The IPC chokepoint iterates every window applying native theme; cross-window propagation runs once via existinguseCrossWindowPrefsSync. O(N²) → O(N).Tests
commands::theme::tests): 19 tests — 4 mapper, 4ThemeApplierdispatch (regression guard), 8 resolver branch matrix, OS-detect smoke, Windows-onlypopup_themeFFI smoke, error-swallow source-text guard.menu_test.rs): routing table updated; theme-Targeted regression guard with O(N²)→O(N) rationale.main-window-menu-at-build-time.test.ts(post-build call order, macOS post-set_menu ordering); new cross-window DOM update test inuseCrossWindowPrefsSync.test.ts.e2e/native/10-theme-runtime-toggle.spec.ts): drivesset_themeIPC end-to-end on the real binary, verifies disk persistence +<html data-theme>, asserts garbage rejection with typedInvalidTheme.Review process
Driven by 4 parallel critical-posture reviewers (
code-reviewagent) per round across 2 iterations of findings + 1 final pass:What this PR does NOT change
"system"and the OS theme changes while the app is running, the muda-painted menu bar stays at the snapshotted theme until the nextset_themecall. Popups + chrome + renderer DOM all track the OS dynamically; only the menu bar lags. Documented inset_themeIPC rustdoc; tracked as a follow-up that would need aWM_SETTINGCHANGElistener in Rust.Verification
cargo test: 792+ tests passing (lib + integration + per-test-file).vitest: 2349 frontend tests passing.tsc --noEmit: exit 0.File-size budgets (architecture rule 23)
config.rs: 495 → 293 lines (net reduction).theme.rs: new, 636 total (~400 production + ~200 tests; production under 400).lib.rs: 1218 → 1251 (+33; already on the architecture watchlist).