perf: detect system theme off UI thread#859
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThemeState now runs OS theme detection on background threads, tagging results with a wrapping generation. poll_and_apply drains queued generation-tagged replies, applies matching results, and schedules throttled spawn_blocking detection when preference is ThemeMode::System. ChangesAsync OS Theme Detection
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✅ Review complete (commit ed0d022) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Refactor moves recurring OS theme detection off the egui thread via spawn_blocking, with generation-tagged results to discard stale probes. Both reviewers independently flag two genuine concerns: (1) detection_in_flight can latch true forever if the worker panics or hangs, since the channel keeps a live sender inside ThemeState and the flag is only ever cleared by a matching-generation result or a preference change; (2) the new state machine has no test coverage. Remaining items are minor nits.
Reviewed commit: 4292e1f
🟡 2 suggestion(s) | 💬 2 nitpick(s)
3 additional findings
🟡 suggestion: New generation/throttle/in-flight state machine has no test coverage
src/app.rs (lines 69-177)
ThemeState now encodes several non-trivial invariants that are the entire point of this PR: the 2s throttle on probe dispatch, the detection_in_flight guard preventing concurrent probes, the generation-tagged discard semantics that protect against stale OS-detected values overwriting an explicit preference, and the None-detection fallback that preserves the previous resolved theme. None of this is covered by tests.
Most of it is testable without a tokio runtime or egui context by extracting drain_detection_results and the throttle/generation bookkeeping into pure functions taking (now: Instant, incoming: (u64, Option<ThemeMode>)). Two high-value cases to lock in: (a) a stale-generation result arriving after apply_new_preference must not overwrite resolved, and (b) a None detection result must leave the prior resolved value intact. Without coverage, a future refactor can silently reintroduce theme flashing or stale overwrites.
💬 nitpick: Explain why `apply_new_preference` keeps the synchronous probe
src/app.rs (lines 155-176)
The PR moves recurring detection off the UI thread, but apply_new_preference still calls try_detect_system_theme() synchronously when the user switches to System. This is intentional — it preserves the no-flash transition — but the asymmetry is exactly the kind of thing a future contributor will try to "fix" by making it async, reintroducing flicker. A one-line comment on this branch explaining the rationale would prevent that.
💬 nitpick: `apply_new_preference` updates `last_checked` even for non-System preferences
src/app.rs (lines 155-176)
After switching to Dark or Light, last_checked is still bumped to now. It's harmless today because request_system_detection is only called from poll_and_apply when preference == ThemeMode::System, so the throttle is never consulted while a fixed preference is active. But it means that on switching back to System, the throttle is measured from the time of the last preference change rather than the last actual detection — small surprise if the throttle is ever reused elsewhere. Either gate the update on new_theme == System or rename the field for clarity.
🤖 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] lines 113-136: `detection_in_flight` can latch true forever if the background probe panics or hangs
`request_system_detection` sets `self.detection_in_flight = true` before `spawn_blocking`, and the flag is only cleared in `drain_detection_results` when a matching-generation result arrives, or in `apply_new_preference` when the user changes preference. Two failure modes leave the flag stuck:
1. **Worker panic** — if `dark_light::detect()` (called inside `try_detect_system_theme`) panics on some platform/version, the cloned sender is dropped via stack unwinding, but the original `detection_sender` remains owned by `ThemeState` (line 76). The channel therefore never becomes `Disconnected`; `try_recv` keeps returning `Empty`. No result tuple is ever delivered for this generation, and `detection_in_flight` stays true for the lifetime of the process.
2. **Worker hang** — if the OS call never returns, the same outcome applies without any panic.
From that point on, every subsequent `request_system_detection` returns at the `if self.detection_in_flight { return; }` guard, so automatic OS-theme tracking is silently disabled until the user manually toggles preference. This is a real resiliency regression versus the prior synchronous path (which couldn't latch). Cheap mitigations: wrap the worker body in `catch_unwind` and always send a generation-tagged result; or treat `last_checked` as a watchdog and reset `detection_in_flight` if a probe outstays a reasonable bound (e.g. 10–30s) before falling through to the throttle check.
- [SUGGESTION] lines 69-177: New generation/throttle/in-flight state machine has no test coverage
`ThemeState` now encodes several non-trivial invariants that are the entire point of this PR: the 2s throttle on probe dispatch, the `detection_in_flight` guard preventing concurrent probes, the generation-tagged discard semantics that protect against stale OS-detected values overwriting an explicit preference, and the `None`-detection fallback that preserves the previous resolved theme. None of this is covered by tests.
Most of it is testable without a tokio runtime or egui context by extracting `drain_detection_results` and the throttle/generation bookkeeping into pure functions taking `(now: Instant, incoming: (u64, Option<ThemeMode>))`. Two high-value cases to lock in: (a) a stale-generation result arriving after `apply_new_preference` must not overwrite `resolved`, and (b) a `None` detection result must leave the prior `resolved` value intact. Without coverage, a future refactor can silently reintroduce theme flashing or stale overwrites.
| fn request_system_detection(&mut self, ctx: &egui::Context, now: Instant) { | ||
| if self.detection_in_flight { | ||
| return; | ||
| } | ||
| if self | ||
| .last_checked | ||
| .is_some_and(|last_checked| now.duration_since(last_checked) < Duration::from_secs(2)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| self.last_checked = Some(now); | ||
| self.detection_in_flight = true; | ||
| let sender = self.detection_sender.clone(); | ||
| let generation = self.detection_generation; | ||
| let ctx = ctx.clone(); | ||
| tokio::task::spawn_blocking(move || { | ||
| let detected = crate::ui::theme::try_detect_system_theme(); | ||
| if sender.send((generation, detected)).is_err() { | ||
| tracing::debug!("Dropping OS theme detection result because receiver closed"); | ||
| } | ||
| ctx.request_repaint(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: detection_in_flight can latch true forever if the background probe panics or hangs
request_system_detection sets self.detection_in_flight = true before spawn_blocking, and the flag is only cleared in drain_detection_results when a matching-generation result arrives, or in apply_new_preference when the user changes preference. Two failure modes leave the flag stuck:
- Worker panic — if
dark_light::detect()(called insidetry_detect_system_theme) panics on some platform/version, the cloned sender is dropped via stack unwinding, but the originaldetection_senderremains owned byThemeState(line 76). The channel therefore never becomesDisconnected;try_recvkeeps returningEmpty. No result tuple is ever delivered for this generation, anddetection_in_flightstays true for the lifetime of the process. - Worker hang — if the OS call never returns, the same outcome applies without any panic.
From that point on, every subsequent request_system_detection returns at the if self.detection_in_flight { return; } guard, so automatic OS-theme tracking is silently disabled until the user manually toggles preference. This is a real resiliency regression versus the prior synchronous path (which couldn't latch). Cheap mitigations: wrap the worker body in catch_unwind and always send a generation-tagged result; or treat last_checked as a watchdog and reset detection_in_flight if a probe outstays a reasonable bound (e.g. 10–30s) before falling through to the throttle check.
source: ['claude-general', 'codex-rust-quality']
🤖 Fix this 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] lines 113-136: `detection_in_flight` can latch true forever if the background probe panics or hangs
`request_system_detection` sets `self.detection_in_flight = true` before `spawn_blocking`, and the flag is only cleared in `drain_detection_results` when a matching-generation result arrives, or in `apply_new_preference` when the user changes preference. Two failure modes leave the flag stuck:
1. **Worker panic** — if `dark_light::detect()` (called inside `try_detect_system_theme`) panics on some platform/version, the cloned sender is dropped via stack unwinding, but the original `detection_sender` remains owned by `ThemeState` (line 76). The channel therefore never becomes `Disconnected`; `try_recv` keeps returning `Empty`. No result tuple is ever delivered for this generation, and `detection_in_flight` stays true for the lifetime of the process.
2. **Worker hang** — if the OS call never returns, the same outcome applies without any panic.
From that point on, every subsequent `request_system_detection` returns at the `if self.detection_in_flight { return; }` guard, so automatic OS-theme tracking is silently disabled until the user manually toggles preference. This is a real resiliency regression versus the prior synchronous path (which couldn't latch). Cheap mitigations: wrap the worker body in `catch_unwind` and always send a generation-tagged result; or treat `last_checked` as a watchdog and reset `detection_in_flight` if a probe outstays a reasonable bound (e.g. 10–30s) before falling through to the throttle check.
thepastaclaw
force-pushed
the
tracker-782-theme-detection-bg
branch
from
4292e1f to
ed0d022
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The refactor successfully addresses both prior blocking concerns: a 10s watchdog (line 142) plus catch_unwind (line 185) prevent the in-flight slot from latching, and a new test module (lines 239-336) covers stale-generation, None-result, in-flight guard, throttle, and watchdog behavior. Only two minor nits remain in apply_new_preference; this is a COMMENT-level review.
Reviewed commit: ed0d022
💬 2 nitpick(s)
| @@ -120,13 +229,169 @@ impl ThemeState { | |||
| } else { | |||
| new_theme | |||
| }; | |||
| self.last_checked = Instant::now(); | |||
| self.last_checked = Some(now); | |||
| crate::ui::theme::apply_theme(ctx, self.resolved); | |||
| self.last_applied = Some(self.resolved); | |||
| detection_failed | |||
| } | |||
There was a problem hiding this comment.
💬 Nitpick: Document why apply_new_preference keeps a synchronous probe
The PR's main goal is to move recurring OS theme detection off the egui thread, yet apply_new_preference still calls crate::ui::theme::try_detect_system_theme() synchronously at line 222 when the user switches to System. This is intentional — it preserves the no-flash transition by resolving the theme before the next frame paints — but the asymmetry with the async request_system_detection is exactly the kind of thing a future contributor will try to 'fix' by making it async, reintroducing flicker. Add a one-line comment on the if new_theme == ThemeMode::System branch noting that this synchronous detection is intentional, to avoid a Light→System theme flash.
source: ['claude-general', 'claude-rust-quality']
| @@ -120,13 +229,169 @@ impl ThemeState { | |||
| } else { | |||
| new_theme | |||
| }; | |||
| self.last_checked = Instant::now(); | |||
| self.last_checked = Some(now); | |||
There was a problem hiding this comment.
💬 Nitpick: apply_new_preference updates last_checked even for non-System preferences
After switching to Dark or Light, last_checked is unconditionally bumped to now at line 232. It's harmless today because request_system_detection is only called from poll_and_apply when preference == ThemeMode::System (line 202), so the throttle is never consulted while a fixed preference is active. But it means that on switching from (say) Dark back to System, the next probe's throttle window is measured from when Dark was selected — not from the last actual detection. Either gate the assignment on new_theme == ThemeMode::System, or rename last_checked to something like last_throttle_anchor to make the semantics explicit.
source: ['claude-general', 'claude-rust-quality']
1 participant
Pull Request
Summary
spawn_blockingworker.existing no-flash behavior.
overwrite newer preference state.
Fixes #782.
Validation
cargo fmt --checkcargo checkcode-review dashpay/dash-evo-tool upstream/v1.0-dev tracker-782-theme-detection-bg "Move recurring OS theme detection off the egui update path by running the throttled dark_light::detect poll in a spawn_blocking task, returning generation-tagged results over a channel, and applying cached theme state without blocking periodic UI frames while preserving startup and explicit preference-change behavior"Summary by CodeRabbit