fix(ui): throttle system theme detection to prevent white flash#781
Conversation
…g sync When using the "System" color theme, dark_light::detect() was called every frame. During SPV sync (high repaint frequency), transient wrong values caused brief theme flips visible as white flashes. Changes: - Cache the resolved theme in AppState and only re-poll the OS every 2s - Guard font initialization with AtomicBool so set_fonts runs once - Immediately resolve and apply theme on preference change Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAppState now caches a resolved theme, tracks the last applied theme and a last-checked timestamp; OS theme resolution for Changes
Sequence Diagram(s)sequenceDiagram
participant AppUpdate as App::update
participant AppState as AppState
participant OS as OS Theme Resolver
participant Backend as Backend (events)
participant Apply as apply_theme
participant Egui as egui::Context
Note over AppUpdate,Apply: Theme preference = System
AppUpdate->>AppState: read theme_preference, last_applied_theme, theme_last_checked
alt theme_last_checked ≥ 2s ago
AppUpdate->>OS: resolve current OS theme
OS-->>AppUpdate: OS theme
AppUpdate->>AppState: update resolved_theme, theme_last_checked
end
Backend->>AppState: UpdatedThemePreference
Backend->>AppUpdate: notify (triggers immediate resolve)
AppUpdate->>AppState: resolve & cache resolved_theme, reset theme_last_checked
AppUpdate->>Apply: apply_theme(resolved_theme)
Apply->>Egui: ctx.set_style(style) (no font changes)
alt resolved_theme != last_applied_theme
Egui-->>AppState: theme visually applied
AppUpdate->>AppState: update last_applied_theme
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
This PR reduces UI flicker (“white flash”) caused by per-frame system theme detection by caching the resolved Light/Dark theme in AppState and throttling OS theme polling, and also avoids reconfiguring fonts every frame.
Changes:
- Add
resolved_theme+theme_last_checkedtoAppStateand throttle system theme polling to once per 2 seconds. - Apply theme using the cached resolved (non-System) theme.
- Guard
ctx.set_fonts()so fonts are configured only once.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/ui/theme.rs |
Adds a one-time font initialization guard inside apply_theme(). |
src/app.rs |
Caches/throttles system theme resolution and applies the cached theme in the main UI loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app.rs`:
- Around line 1096-1108: The shutdown repaint path is still calling
apply_theme(ctx, self.theme_preference) which triggers system detection each
repaint when theme_preference == ThemeMode::System; change the shutdown branch
to use the cached resolved theme (self.resolved_theme) instead of
self.theme_preference (so call apply_theme(ctx, self.resolved_theme)) and avoid
invoking crate::ui::theme::resolve_theme_mode during shutdown; this reuses the
already-computed resolved theme (resolved_theme) and prevents extra
dark_light::detect() calls on exit.
In `@src/ui/theme.rs`:
- Around line 1155-1158: Replace the process-global AtomicBool
(FONTS_INITIALIZED) with a context-scoped flag stored on the egui::Context:
remove the static and instead check/record a small marker in ctx.data_mut() (use
a unique key type like FontInitKey or a string key) before calling
ctx.set_fonts(configure_fonts()); if the key isn't present, call
configure_fonts() and insert the marker into ctx.data_mut() to avoid
reinitializing for the same egui::Context; alternatively, consolidate font
initialization so only one call path (e.g., the existing app-level initializer)
performs ctx.set_fonts to keep contexts consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec36e4a8-d3ac-47fb-b14d-72474bc679e0
📒 Files selected for processing (2)
src/app.rssrc/ui/theme.rs
- Use cached resolved_theme in shutdown path instead of theme_preference - Only call apply_theme() when resolved theme actually changes - Replace process-global FONTS_INITIALIZED with per-Context guard Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app.rs (1)
1101-1116: Finish the "once perContext" font-init cleanup.The throttled poll/apply split looks good, but
new_inner()still doesctx.set_fonts(...)at Line 271. The firstapply_theme()on this path will therefore configure fonts again on the sameegui::Context, so the optimization is only partial. Either remove the eager call or mark that context as already initialized when doing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1101 - 1116, The eager ctx.set_fonts(...) call in new_inner() causes fonts to be set twice for the same egui::Context; either remove that eager set or mark the Context as already initialized so apply_theme(ctx, ...) skips font re-initialization. Locate new_inner() and either delete the ctx.set_fonts(...) call there, or add a flag (e.g., on the struct or a Context-mark like last_applied_theme/initialized_contexts keyed by the egui::Context) and set it when performing the eager set so that apply_theme(ctx, ...) checks that flag and returns early for fonts if the Context was already initialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app.rs`:
- Around line 1101-1116: The eager ctx.set_fonts(...) call in new_inner() causes
fonts to be set twice for the same egui::Context; either remove that eager set
or mark the Context as already initialized so apply_theme(ctx, ...) skips font
re-initialization. Locate new_inner() and either delete the ctx.set_fonts(...)
call there, or add a flag (e.g., on the struct or a Context-mark like
last_applied_theme/initialized_contexts keyed by the egui::Context) and set it
when performing the eager set so that apply_theme(ctx, ...) checks that flag and
returns early for fonts if the Context was already initialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80c34f77-48f9-445d-92da-ca69deb6068f
📒 Files selected for processing (2)
src/app.rssrc/ui/theme.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/theme.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-targeted throttle fix. All callers correctly pass pre-resolved themes so the system works as intended. Findings are valid but none are blocking — the most actionable is the > vs >= off-by-one that stretches the effective throttle from 2s to ~3s at 1s repaint intervals.
Reviewed commit: 3e84e77
🟡 4 suggestion(s) | 💬 1 nitpick(s)
2 additional findings
🟡 suggestion: `apply_theme` internally re-resolves via `resolve_theme_mode` — redundant for current callers and latent throttle bypass
src/ui/theme.rs (lines 1035-1038)
All callers now pass a pre-resolved Light or Dark value (never System), so resolve_theme_mode at line 1037 is a no-op. However, apply_theme is pub and still accepts ThemeMode::System, meaning a future caller could bypass the throttle by calling apply_theme(ctx, ThemeMode::System) directly. Consider either:
- Adding a debug assertion that
theme_mode != ThemeMode::System, or - Changing the parameter type to only accept resolved themes.
Not urgent since all current callers are correct, but the throttling invariant is enforced by caller discipline rather than the type system.
🟡 suggestion: `resolve_theme_mode` silently swallows detection errors
src/ui/theme.rs (lines 25-29)
Line 27 uses .unwrap_or(ThemeMode::Light) which discards the error from detect_system_theme() with no logging. If theme detection is failing persistently (e.g., dbus unavailable on Linux), the user would be stuck on Light with no diagnostic trail. A tracing::warn! on the error path would help.
💡 Suggested change
pub fn resolve_theme_mode(preference: ThemeMode) -> ThemeMode {
match preference {
ThemeMode::System => detect_system_theme().unwrap_or_else(|e| {
tracing::warn!("System theme detection failed, defaulting to Light: {e}");
ThemeMode::Light
}),
other => other,
}
}
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/app.rs`:
- [SUGGESTION] line 1105: Strict `>` stretches the 2-second throttle to ~3 seconds at idle
With `request_repaint_after(Duration::from_secs(1))` at line 1233, the frame ticks are roughly 1s apart. Using strict `>` means:
- t=0: `theme_last_checked` set
- t=1: `1s > 2s` → false
- t=2: `2s > 2s` → false
- t=3: `3s > 2s` → true
So the effective interval is ~3s, not the documented 2s. Using `>=` aligns behavior with the comment.
- [SUGGESTION] lines 1101-1116: Throttle and cache logic has no automated tests
The throttle/cache state machine (three coordinated fields: `resolved_theme`, `last_applied_theme`, `theme_last_checked`) has no unit or integration test coverage. All four review agents flagged this independently. Given this is fixing a user-visible regression (white flash), a regression test would be valuable — though the tight coupling to egui's `Context` makes this harder to test in isolation.
In `src/ui/theme.rs`:
- [SUGGESTION] lines 1035-1038: `apply_theme` internally re-resolves via `resolve_theme_mode` — redundant for current callers and latent throttle bypass
All callers now pass a pre-resolved `Light` or `Dark` value (never `System`), so `resolve_theme_mode` at line 1037 is a no-op. However, `apply_theme` is `pub` and still accepts `ThemeMode::System`, meaning a future caller could bypass the throttle by calling `apply_theme(ctx, ThemeMode::System)` directly. Consider either:
1. Adding a debug assertion that `theme_mode != ThemeMode::System`, or
2. Changing the parameter type to only accept resolved themes.
Not urgent since all current callers are correct, but the throttling invariant is enforced by caller discipline rather than the type system.
- [SUGGESTION] lines 25-29: `resolve_theme_mode` silently swallows detection errors
Line 27 uses `.unwrap_or(ThemeMode::Light)` which discards the error from `detect_system_theme()` with no logging. If theme detection is failing persistently (e.g., dbus unavailable on Linux), the user would be stuck on Light with no diagnostic trail. A `tracing::warn!` on the error path would help.
… remove dead assignment Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app.rs (1)
726-728: Letapply_themeown font setup end-to-end.Because
last_applied_themeis initialized toNoneon Line 727, the first frame is guaranteed to enter the newapply_themepath at Line 1113. Withsrc/ui/theme.rsnow doing one-time font initialization peregui::Context, the eagerctx.set_fonts(...)innew_inner()at Line 271 is duplicate startup work and splits font configuration between two places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 726 - 728, Remove the eager font setup in new_inner(): delete the ctx.set_fonts(...) call so that apply_theme fully owns font initialization (including the initial startup path triggered by last_applied_theme being None). Keep last_applied_theme initialized to None so the first frame runs apply_theme, and ensure all font configuration now lives only in apply_theme (which uses src/ui/theme.rs one-time per egui::Context logic) to avoid duplicate/fragmented font setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app.rs`:
- Around line 726-728: Remove the eager font setup in new_inner(): delete the
ctx.set_fonts(...) call so that apply_theme fully owns font initialization
(including the initial startup path triggered by last_applied_theme being None).
Keep last_applied_theme initialized to None so the first frame runs apply_theme,
and ensure all font configuration now lives only in apply_theme (which uses
src/ui/theme.rs one-time per egui::Context logic) to avoid duplicate/fragmented
font setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3dcd3be-94ba-4963-b550-6d647e7143f7
📒 Files selected for processing (2)
src/app.rssrc/ui/theme.rs
✅ Files skipped from review due to trivial changes (1)
- src/ui/theme.rs
… persistence The font initialization guard in apply_theme() used get_persisted_mut_or_default, which serializes the flag to app.ron. On restart, configure_fonts() was skipped entirely, leaving users with default egui fonts instead of custom Dash fonts. Switch to get_temp_mut_or_default — temp data survives across frames within a session but is cleared on Context drop, so fonts are correctly re-initialized on each app launch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Font initialization was happening in two places: `new_inner()` (comprehensive i18n font set via `bundled::fonts()`) and `apply_theme()` (basic NotoSans variable font via `configure_fonts()`). The latter overwrote the i18n fonts on first frame, losing support for CJK, Thai, Arabic, Hebrew, and Devanagari scripts. Since fonts don't change with themes, remove the font init block and the now-unused `configure_fonts()` function from `apply_theme()`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/theme.rs (1)
1008-1013: 🛠️ Refactor suggestion | 🟠 MajorMake
apply_themeconsume the cached resolved theme.Resolving
ThemeMode::Systemhere reopens the rawdark_light::detect()path the PR is trying to centralize and throttle. If any caller passes the preference instead of the cachedresolved_theme, this bypasses the 2s guard and can bring the flash back. Keep resolution inAppStateand make this API accept only an already-resolved light/dark mode.Proposed change
-/// Configure fonts for the application -/// Apply the modern Dash theme to the egui context -pub fn apply_theme(ctx: &egui::Context, theme_mode: ThemeMode) { - // Resolve the actual theme to use - let resolved_theme = resolve_theme_mode(theme_mode); - let dark_mode = resolved_theme == ThemeMode::Dark; +/// Apply the modern Dash theme to the egui context using the cached resolved mode. +pub fn apply_theme(ctx: &egui::Context, resolved_theme: ThemeMode) { + let dark_mode = match resolved_theme { + ThemeMode::Dark => true, + ThemeMode::Light => false, + ThemeMode::System => unreachable!("apply_theme expects a resolved theme"), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/theme.rs` around lines 1008 - 1013, The apply_theme function currently calls resolve_theme_mode and can re-trigger ThemeMode::System detection; change apply_theme(&egui::Context, ThemeMode) to accept an already-resolved mode (e.g., a concrete ThemeMode or a bool like dark_mode) and remove the internal call to resolve_theme_mode/dark_light::detect(); update callers (AppState) to pass the cached resolved theme instead, and ensure apply_theme uses the provided resolved value (e.g., compare against ThemeMode::Dark or use the provided dark boolean) so no system detection happens inside apply_theme.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ui/theme.rs`:
- Around line 1008-1013: The apply_theme function currently calls
resolve_theme_mode and can re-trigger ThemeMode::System detection; change
apply_theme(&egui::Context, ThemeMode) to accept an already-resolved mode (e.g.,
a concrete ThemeMode or a bool like dark_mode) and remove the internal call to
resolve_theme_mode/dark_light::detect(); update callers (AppState) to pass the
cached resolved theme instead, and ensure apply_theme uses the provided resolved
value (e.g., compare against ThemeMode::Dark or use the provided dark boolean)
so no system detection happens inside apply_theme.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75d7529c-66f5-4685-89ad-df13ba86ca93
📒 Files selected for processing (1)
src/ui/theme.rs
The throttled theme polling called resolve_theme_mode which falls back to Light on both detection errors and Unspecified results from dark_light::detect(). This caused dark-mode users to flash to light when the OS returned a transient error or ambiguous result. Add try_detect_system_theme() that returns None on error/Unspecified, and use it in the polling path so the previous resolved theme is preserved when detection is inconclusive. The preference-change and initialization paths still use resolve_theme_mode with its Light fallback, which is correct for those contexts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.theme_preference = new_theme; | ||
| self.resolved_theme = crate::ui::theme::resolve_theme_mode(new_theme); |
There was a problem hiding this comment.
In the UpdatedThemePreference handler, resolve_theme_mode(new_theme) will call detect_system_theme().unwrap_or(ThemeMode::Light) when switching to System, which forces Light on any detection error/Unspecified result. That undermines the new “keep previous theme on ambiguous/error” behavior and can still cause a visible flip/flash when the user selects System during a transient dark_light::detect() glitch. Consider using try_detect_system_theme() here and falling back to the current resolved_theme when detection is None, while also ensuring the next OS poll happens promptly.
| self.theme_preference = new_theme; | |
| self.resolved_theme = crate::ui::theme::resolve_theme_mode(new_theme); | |
| // Preserve the previously resolved theme so that, when the user | |
| // selects the System theme and detection is ambiguous or fails, | |
| // we keep the current appearance instead of flashing to Light. | |
| let previous_resolved_theme = self.resolved_theme; | |
| self.theme_preference = new_theme; | |
| let resolved_theme = match new_theme { | |
| crate::ui::theme::ThemePreference::System => { | |
| match crate::ui::theme::try_detect_system_theme() { | |
| Some(system_mode) => system_mode, | |
| None => previous_resolved_theme, | |
| } | |
| } | |
| _ => crate::ui::theme::resolve_theme_mode(new_theme), | |
| }; | |
| self.resolved_theme = resolved_theme; |
| /// Configure fonts for the application | ||
| pub fn configure_fonts() -> FontDefinitions { | ||
| let mut fonts = FontDefinitions::default(); | ||
|
|
||
| // Load Noto Sans font for better international support | ||
| fonts.font_data.insert( | ||
| "NotoSans".to_owned(), | ||
| FontData::from_static(include_bytes!( | ||
| "../../assets/Fonts/Noto_Sans/NotoSans-VariableFont.ttf" | ||
| )) | ||
| .into(), | ||
| ); | ||
|
|
||
| // Add NotoSans to the proportional font family (used for UI text) | ||
| fonts | ||
| .families | ||
| .get_mut(&FontFamily::Proportional) | ||
| .unwrap() | ||
| .insert(0, "NotoSans".to_owned()); | ||
|
|
||
| fonts | ||
| } | ||
|
|
||
| /// Apply the modern Dash theme to the egui context |
There was a problem hiding this comment.
The /// Configure fonts for the application doc comment is now stale: configure_fonts() was removed and apply_theme() no longer sets fonts. Please remove or update this comment to reflect the current behavior (theme/styling only).
- Remove leftover "Configure fonts" doc comment from apply_theme - Use try_detect_system_theme in UpdatedThemePreference handler to avoid falling back to Light on transient detection errors when switching to System mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dark_light::detect() returns Unspecified on Linux when the desktop theme can't be determined. The previous commit treated Unspecified the same as errors (returning None), which made System theme mode non-functional on Linux. Only suppress actual detection errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the user switches to System theme mode and detection fails, show a warning banner instead of silently falling back. The app keeps the previous theme and will auto-correct when detection succeeds on the next poll. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
dark_light::detect()to once every 2 seconds when using System theme — prevents transient OS detection glitches from causing brief Light/Dark flips (visible as white flashes during high-frequency repaints like SPV sync)AppStateand only reapply when it actually changes (skip per-frameapply_themewhen theme is unchanged)resolved_themeconsistently — including the shutdown branch, which previously bypassed the throttleFONTS_INITIALIZEDstatic with a per-egui::Contextguard so fonts are correctly initialized for each Context instance (tests, multiple viewports)temp) storage for the font-init flag — persisted storage caused fonts to be skipped on app restart because the flag survived inapp.ronRoot Cause
apply_theme()was called on every frame. For System theme mode, each call queried the OS viadark_light::detect(). During rapid repaints (e.g. SPV sync progress), the detection could transiently return the wrong value → momentary theme flip → white flash.Test plan
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Refactor
Behavior Change