[029-timer-expiry-and-notification-interface]: Unify cap + timer-expiry notifications - #53
Merged
Merged
Conversation
Introduce one notification interface for the whole app. Every notifying subsystem (cap monitor, timer expiry) implements NotificationSource; NotificationSettings.sources is the single per-source toggle surface. Adds the timer-expiry detection loop on top of TimerTicker (044), so a running timer that reaches zero transitions to Expired exactly once, fires a Tauri event, and emits a system notification when enabled. Domain (crates/domain): - New crates/domain/src/notification.rs with NotificationSource trait, NotificationSettings, all_sources(), and TimerExpiredSource / FocusesOverCapSource / TasksOverCapSource concrete impls. - Settings.alerts removed; replaced by Settings.notifications: NotificationSettings. settings.yaml round-trip emits/parses a `notifications:` section; missing keys default to enabled. Commands (crates/commands): - CapEvaluator consults NotificationSettings::is_enabled for FocusesOverCapSource and TasksOverCapSource independently; the per-source toggle gates both the over and under (recovery) calls for that source. Storage (crates/storage): - FocusStore::update_timer added. MarkdownFocusStore writes timer.json atomically; returns NotFound when the focus dir is absent. Tauri composition root: - New src-tauri/src/app/timer_expiry.rs spawns a 1Hz tokio interval that calls adhd_ranch_domain::tick(now, &focuses) (no inline expiry arithmetic), persists TimerStatus::Expired via update_timer, emits the timer-expired event with focus_id + focus_title, and fires the system notification when TimerExpiredSource is enabled. Frontend: - ts-rs regenerates Alerts.ts → NotificationSettings.ts; Settings.ts reflects the new field. SettingsWindow renders one toggle per known source instead of a single system-notifications switch. Closes issue 029.
📝 WalkthroughWalkthroughPer-source NotificationSettings replace the old Alerts boolean; Settings serialization, CapEvaluator, UI, and tests are updated. FocusStore gains update_timer and a Tauri 1s background task detects expired timers, updates storage, emits events, and conditionally shows system notifications. ChangesPer-Source Notifications and Timer Expiry
Sequence Diagram(s)sequenceDiagram
participant Tokio as Tokio Interval
participant TimerTask as timer_expiry::run_once
participant FocusStore
participant SettingsState
participant Tauri as Tauri Events
participant Notification as Local Notification
loop Every 1 second
Tokio->>TimerTask: tick
TimerTask->>FocusStore: list focuses with timers
TimerTask->>TimerTask: compute expirations
TimerTask->>SettingsState: is TimerExpiredSource enabled?
alt Expired & notifications enabled
TimerTask->>FocusStore: update_timer(focus_id, Expired)
TimerTask->>Tauri: emit timer-expired event
TimerTask->>Notification: show "Timer expired"
else Expired & notifications disabled
TimerTask->>FocusStore: update_timer(focus_id, Expired)
TimerTask->>Tauri: emit timer-expired event
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/domain/src/settings.rs (1)
105-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegacy
alerts.system_notificationsconfigs are no longer migrated.Line 105-Line 111 and Line 131-Line 135 drop the old
alertssection entirely, so existing configs withalerts.system_notifications: falsenow resolve to default-enabled notifications after upgrade.Suggested compatibility patch
@@ - section = match name { + section = match name { "caps" => "caps", + "alerts" => "alerts", "notifications" => "notifications", "widget" => "widget", "displays" => "displays", _ => "", }; @@ + ("alerts", "system_notifications") => { + if let Some(enabled) = parse_bool(value) { + for source in crate::notification::all_sources() { + settings.notifications.set(source.as_ref(), enabled); + } + } + } ("notifications", k) => { if let Some(b) = parse_bool(value) { settings.notifications.sources.insert(k.to_string(), b); } }Also applies to: 131-135
🤖 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/domain/src/settings.rs` around lines 105 - 111, The migration drops the legacy "alerts" section so configs like alerts.system_notifications=false get lost; update the section-mapping logic (the match on name that assigns to variable section and the similar logic around lines referenced) to map "alerts" to "notifications" and ensure the nested key "system_notifications" is translated into the new notifications setting (preserve/translate false => disabled) during migration; adjust any code paths that read/merge keys so that when name == "alerts" you route its children into the "notifications" section and convert "system_notifications" into the corresponding notifications field.
🤖 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 `@crates/storage/src/focus_store.rs`:
- Around line 292-299: The update_timer method currently checks dir.is_dir()
then calls atomic_write which can fail with an Io(NotFound) if the directory is
removed in the race; change the error handling around the atomic_write call (the
call to atomic_write(&dir.join("timer.json"), &bytes)) to map an underlying
IoError with kind NotFound into FocusStoreError::NotFound(focus_id.to_string())
so the method always preserves its contract, e.g., by matching the Result from
atomic_write and converting Err(e) where e.kind() ==
std::io::ErrorKind::NotFound into the NotFound variant and returning other
errors unchanged.
In `@src-tauri/src/app/timer_expiry.rs`:
- Around line 30-84: The run_once function in src-tauri/src/app/timer_expiry.rs
contains domain workflow logic (calling tick, constructing FocusTimer,
persisting via store.update_timer, emitting TIMER_EXPIRED_EVENT and showing
notifications) that should be moved into a dedicated service so the app layer
only wires dependencies; refactor by extracting a new service (e.g.,
TimerExpiryService) that exposes a method like handle_transitions(now, &focuses,
store, handle, settings) which calls tick, builds
FocusTimer/TimerStatus::Expired, calls store.update_timer, emits
TIMER_EXPIRED_EVENT, and triggers notifications based on SettingsState, then
update run_once to only gather focuses, obtain now and settings, and delegate to
TimerExpiryService::handle_transitions (or similar) so run_once becomes thin
wiring that injects AppHandle, FocusStore, and SettingsState into the new
service.
- Around line 21-26: The timer task is performing blocking filesystem I/O on the
async runtime by calling run_once which calls synchronous store.list() and
store.update_timer() (MarkdownFocusStore) inside the tauri::async_runtime::spawn
loop; change the timer loop to offload blocking work by calling
tokio::task::spawn_blocking (or equivalent) for run_once’s storage calls (or
make MarkdownFocusStore async and await it), and update all call sites (the
interval loop and the other spots noted around
run_once/store.list()/store.update_timer()) to invoke the non-blocking variant
so the async runtime thread is not blocked.
---
Outside diff comments:
In `@crates/domain/src/settings.rs`:
- Around line 105-111: The migration drops the legacy "alerts" section so
configs like alerts.system_notifications=false get lost; update the
section-mapping logic (the match on name that assigns to variable section and
the similar logic around lines referenced) to map "alerts" to "notifications"
and ensure the nested key "system_notifications" is translated into the new
notifications setting (preserve/translate false => disabled) during migration;
adjust any code paths that read/merge keys so that when name == "alerts" you
route its children into the "notifications" section and convert
"system_notifications" into the corresponding notifications field.
🪄 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: 1c7b5ca7-1086-4e04-8210-dd7fe9eddf36
⛔ Files ignored due to path filters (2)
src/types/generated/NotificationSettings.tsis excluded by!**/generated/**src/types/generated/Settings.tsis excluded by!**/generated/**
📒 Files selected for processing (11)
crates/commands/src/caps.rscrates/domain/src/lib.rscrates/domain/src/notification.rscrates/domain/src/settings.rscrates/storage/src/focus_store.rscrates/storage/src/settings_writer.rssrc-tauri/Cargo.tomlsrc-tauri/src/app/mod.rssrc-tauri/src/app/timer_expiry.rssrc/components/SettingsWindow.tsxsrc/types/settings.ts
Comment on lines
+30
to
+84
| fn run_once(handle: &AppHandle, store: &dyn FocusStore) { | ||
| let focuses = match store.list() { | ||
| Ok(f) => f, | ||
| Err(e) => { | ||
| log::error!("timer_expiry: list failed: {e}"); | ||
| return; | ||
| } | ||
| }; | ||
| let now = current_unix_secs(); | ||
| let transitions = tick(now, &focuses); | ||
| if transitions.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| let notifications_enabled = handle | ||
| .try_state::<SettingsState>() | ||
| .map(|s| { | ||
| s.0.lock() | ||
| .map(|g| g.notifications.is_enabled(&TimerExpiredSource)) | ||
| .unwrap_or(true) | ||
| }) | ||
| .unwrap_or(true); | ||
|
|
||
| for t in transitions { | ||
| let focus = focuses.iter().find(|f| f.id.0 == t.focus_id); | ||
| let Some(focus) = focus else { continue }; | ||
| let Some(running_timer) = focus.timer.as_ref() else { | ||
| continue; | ||
| }; | ||
| let expired = FocusTimer { | ||
| duration_secs: running_timer.duration_secs, | ||
| started_at: running_timer.started_at, | ||
| status: TimerStatus::Expired, | ||
| }; | ||
| if let Err(e) = store.update_timer(&t.focus_id, &expired) { | ||
| log::error!("timer_expiry: update_timer for {} failed: {e}", t.focus_id); | ||
| continue; | ||
| } | ||
| let _ = handle.emit( | ||
| TIMER_EXPIRED_EVENT, | ||
| TimerExpiredPayload { | ||
| focus_id: &t.focus_id, | ||
| focus_title: &t.focus_title, | ||
| }, | ||
| ); | ||
| if notifications_enabled { | ||
| let _ = handle | ||
| .notification() | ||
| .builder() | ||
| .title("Timer expired") | ||
| .body(format!("{} reached its timer.", t.focus_title)) | ||
| .show(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move timer-expiry workflow logic out of src-tauri/src/app/ and keep this layer as wiring.
This module currently owns domain workflow (tick → persist → emit/notify), not just composition. Extract this into a dedicated service module/crate and let app/ only wire dependencies and start it.
As per coding guidelines, "In Rust (src-tauri/src/), app/ should be the composition root that wires everything in main.rs."
🤖 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 `@src-tauri/src/app/timer_expiry.rs` around lines 30 - 84, The run_once
function in src-tauri/src/app/timer_expiry.rs contains domain workflow logic
(calling tick, constructing FocusTimer, persisting via store.update_timer,
emitting TIMER_EXPIRED_EVENT and showing notifications) that should be moved
into a dedicated service so the app layer only wires dependencies; refactor by
extracting a new service (e.g., TimerExpiryService) that exposes a method like
handle_transitions(now, &focuses, store, handle, settings) which calls tick,
builds FocusTimer/TimerStatus::Expired, calls store.update_timer, emits
TIMER_EXPIRED_EVENT, and triggers notifications based on SettingsState, then
update run_once to only gather focuses, obtain now and settings, and delegate to
TimerExpiryService::handle_transitions (or similar) so run_once becomes thin
wiring that injects AppHandle, FocusStore, and SettingsState into the new
service.
Contributor
Author
There was a problem hiding this comment.
Valid per CLAUDE.md (app/ is composition only). Deferring to issue 046 to keep this PR scoped to 029; the extraction is a real refactor (new TimerExpiryService + TauriTimerExpiryNotifier trait split). Filed at issues/046-timer-expiry-service-extraction.md.
There was a problem hiding this comment.
@archae0pteryx, understood — deferring to keep this PR scoped to #029 is a reasonable call, and the planned TimerExpiryService + TauriTimerExpiryNotifier trait split is the right shape for the extraction.
You mentioned a local issues/046-timer-expiry-service-extraction.md file — would you like me to open a GitHub issue from that to track the work? If so, I can create it now.
5 tasks
- FocusStore::update_timer now maps io::ErrorKind::NotFound from the atomic_write call to FocusStoreError::NotFound, preserving the method contract under a directory-removal race between the is_dir() check and the write. - Move the timer-expiry tick off the async runtime thread via tokio::task::spawn_blocking. MarkdownFocusStore is synchronous file I/O, so a 1Hz tick was blocking the runtime for the duration of store.list() + each store.update_timer() call. Filed issue 046 to track the larger CR finding (extract domain workflow out of src-tauri/src/app/ into a dedicated service).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/timer_expiry.rs`:
- Around line 29-30: The current spawn_blocking call drops both the task join
error and any error returned by run_once; update the code around
tokio::task::spawn_blocking(move || run_once(&handle, store.as_ref())).await to
capture the JoinError from .await and the Result returned by run_once, then
handle each error instead of discarding it: if the .await returns Err, log/emit
that join/cancellation error using the existing handle or logger; if the inner
run_once(...) returns Err, log/emit that specific timer/notification error
(include the error details) so failures in run_once are visible. Use the
existing symbols run_once, handle, store and the spawn_blocking result to locate
and implement the checks and emission/logging.
🪄 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: 1d5aa28f-3762-4e72-843a-0c395ade30b5
📒 Files selected for processing (3)
crates/storage/src/focus_store.rsissues/046-timer-expiry-service-extraction.mdsrc-tauri/src/app/timer_expiry.rs
CodeRabbit flagged three silent-failure paths on the second review: the spawn_blocking JoinError, the Tauri event emit Result, and the system-notification show Result. Production diagnosis of missing timer expiries was impossible because every transport-layer failure was discarded. All three now log::error! with the focus_id (or join context) so failures surface in tauri-plugin-log output.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src-tauri/src/app/timer_expiry.rs (1)
38-100: 🛠️ Refactor suggestion | 🟠 MajorArchitecture extraction is still pending for composition-root compliance.
Line [38] through Line [100] still contains workflow orchestration (tick → persist → emit/notify) inside
src-tauri/src/app/. This remains the same previously-raised extraction item and should stay tracked as follow-up work.As per coding guidelines,
src-tauri/src/app/**/*.rs: "In Rust (src-tauri/src/),app/should be the composition root that wires everything inmain.rs."🤖 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 `@src-tauri/src/app/timer_expiry.rs` around lines 38 - 100, The run_once function in src-tauri/src/app/timer_expiry.rs is doing orchestration (calling tick, persisting updates via FocusStore, emitting TIMER_EXPIRED_EVENT and sending notifications) which violates the composition-root guideline; extract the workflow orchestration out of run_once so this module only delegates to injected collaborators: create a coordinator/service (e.g., TimerExpiryService) that encapsulates tick invocation, store.update_timer calls, handle.emit(TIMER_EXPIRED_EVENT, TimerExpiredPayload) and notification logic, and have run_once accept or resolve that collaborator (and SettingsState only for reading notification preference) so run_once becomes a thin adapter that calls the new service with FocusStore and AppHandle abstractions; update call sites to construct/inject the service in the application composition root.
🤖 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.
Duplicate comments:
In `@src-tauri/src/app/timer_expiry.rs`:
- Around line 38-100: The run_once function in src-tauri/src/app/timer_expiry.rs
is doing orchestration (calling tick, persisting updates via FocusStore,
emitting TIMER_EXPIRED_EVENT and sending notifications) which violates the
composition-root guideline; extract the workflow orchestration out of run_once
so this module only delegates to injected collaborators: create a
coordinator/service (e.g., TimerExpiryService) that encapsulates tick
invocation, store.update_timer calls, handle.emit(TIMER_EXPIRED_EVENT,
TimerExpiredPayload) and notification logic, and have run_once accept or resolve
that collaborator (and SettingsState only for reading notification preference)
so run_once becomes a thin adapter that calls the new service with FocusStore
and AppHandle abstractions; update call sites to construct/inject the service in
the application composition root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1828f9c5-0428-4751-9879-76500bef1420
📒 Files selected for processing (1)
src-tauri/src/app/timer_expiry.rs
5 tasks
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
Detects timer expiry, introduces a composable
NotificationSourceinterface, and migrates the existing cap notifications onto the same interface — one notification system for the whole app.NotificationSourcetrait + 3 concrete sources (TimerExpiredSource,FocusesOverCapSource,TasksOverCapSource) incrates/domain/src/notification.rsSettings.alertsdeleted; replaced bySettings.notifications: NotificationSettingswith per-source toggles.settings.yamlgains anotifications:section; missing keys default to enabled.CapEvaluatorconsultsNotificationSettings::is_enabledindependently for focuses-source and tasks-source; the over and under (recovery) calls for a source are gated by the same switch.FocusStore::update_timeradded;MarkdownFocusStorewritestimer.jsonatomically.src-tauri/src/app/timer_expiry.rsspawns a 1Hz tokio interval that consumesadhd_ranch_domain::tick(now, &focuses)(no inline expiry arithmetic), persistsTimerStatus::Expiredexactly once per timer, emits thetimer-expiredevent withfocus_id + focus_title, and fires the system notification whenTimerExpiredSourceis enabled.SettingsWindowrenders one toggle per known source instead of a single system-notifications switch.Closes issue 029.
Test plan
task checkgreen (lint + typecheck + cargo tests + vitest)CapEvaluatortests split into focuses-source-disabled vs tasks-source-disabled (each verifies the other source still fires)update_timertests (persists Expired status, NotFound for missing focus)Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation