[settings-update-workflow]: Settings update workflow and runtime freshness#71
[settings-update-workflow]: Settings update workflow and runtime freshness#71archae0pteryx wants to merge 2 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR fixes runtime settings freshness by introducing a ChangesRuntime Settings Freshness & Workflow Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/commands/src/lib.rs (1)
25-66: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftUse an explicit trait at the module boundary instead of a callable-based
SettingsProvideralias.
SettingsProvideris currentlyArc<dyn Fn() -> Settings + Send + Sync>and is stored/called inCommandsandCapEvaluator, then passed across thesrc-tauriboundary (and called assettings().capsinsrc-tauri/src/app/tray.rs::rebuild_handler). Introduce aSettingsReadertrait (fn get(&self) -> Settings) and depend onArc<dyn SettingsReader>instead.♻️ Suggested direction
+pub trait SettingsReader: Send + Sync { + fn get(&self) -> Settings; +} + +impl<F> SettingsReader for F +where + F: Fn() -> Settings + Send + Sync, +{ + fn get(&self) -> Settings { + self() + } +} - -pub type SettingsProvider = Arc<dyn Fn() -> Settings + Send + Sync>; +pub type SettingsProvider = Arc<dyn SettingsReader>; ... - (self.settings)() + self.settings.get()
- Update remaining call sites like
settings().capsinsrc-tauri/src/app/tray.rsto usesettings.get().caps.- Keep the existing closure-based wiring from
src-tauri/src/app/mod.rs—it will still coerce todyn SettingsReadervia the blanket impl.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/commands/src/lib.rs` around lines 25 - 66, Replace the callable alias SettingsProvider with a small trait: introduce trait SettingsReader { fn get(&self) -> Settings; } and change the stored type on Commands (and CapEvaluator) from SettingsProvider to Arc<dyn SettingsReader + Send + Sync>; update constructors new and new_with_settings_provider to accept Arc<dyn SettingsReader> (or rename new_with_settings_reader) and wire existing call sites to call settings.get() instead of invoking the closure (e.g. change settings().caps to settings.get().caps); add a blanket impl so existing closure wiring still works (impl<F: Fn() -> Settings + Send + Sync> SettingsReader for F { fn get(&self) -> Settings { (self)() } }) and update all references and imports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/app/mod.rs`:
- Around line 95-103: The current settings_provider closure silently falls back
to Settings::default() when the settings_state mutex is poisoned (due to
unwrap_or_default), which can flip runtime caps/notifications/tray state; change
the lock handling in the closure used to build settings_provider (type
adhd_ranch_commands::SettingsProvider, capturing settings_state) to explicitly
handle a poisoned lock: on Ok return the cloned settings, on Err recover the
inner value via the poisoning error's into_inner() and return that recovered
settings while also logging a warning (use the project logger/tracing or
log::warn) that the mutex was poisoned and a recovered value is being used
instead of defaulting. Ensure you remove the unwrap_or_default and replace it
with this match/unwrap_on_poison logic so defaults are not silently substituted.
---
Outside diff comments:
In `@crates/commands/src/lib.rs`:
- Around line 25-66: Replace the callable alias SettingsProvider with a small
trait: introduce trait SettingsReader { fn get(&self) -> Settings; } and change
the stored type on Commands (and CapEvaluator) from SettingsProvider to Arc<dyn
SettingsReader + Send + Sync>; update constructors new and
new_with_settings_provider to accept Arc<dyn SettingsReader> (or rename
new_with_settings_reader) and wire existing call sites to call settings.get()
instead of invoking the closure (e.g. change settings().caps to
settings.get().caps); add a blanket impl so existing closure wiring still works
(impl<F: Fn() -> Settings + Send + Sync> SettingsReader for F { fn get(&self) ->
Settings { (self)() } }) and update all references and imports accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30067aa5-36c5-4c9e-a193-d38fb746c611
📒 Files selected for processing (10)
crates/commands/src/caps.rscrates/commands/src/focus.rscrates/commands/src/lib.rsissues/031-notification-source-settings.mdissues/050-settings-update-workflow.mdissues/README.mdsrc-tauri/src/app/mod.rssrc-tauri/src/app/settings_workflow.rssrc-tauri/src/app/tray.rssrc-tauri/src/ui_bridge/mod.rs
1 participant
What changed
CommandsandCapEvaluatorto read Settings through a live provider instead of startup-copied values.Issue
issues/050-settings-update-workflow.mdCompletion promise
Validation
rtk task checkSummary by CodeRabbit