test: add unit tests for attention timer, break scheduler and the whole state machine#5
test: add unit tests for attention timer, break scheduler and the whole state machine#5pilgrimlyieu merged 25 commits intomainfrom
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
你好 - 我已经审阅了你的更改，它们看起来很棒！
AI 代理提示
请解决此代码审查中的评论：
## 单独评论
### 评论 1
<location> `src-tauri/build.rs:10-11` </location>
<code_context>
+ .windows_attributes(tauri_build::WindowsAttributes::new_without_app_manifest());
+ add_manifest();
+ }
+ tauri_build::try_build(attributes).unwrap();
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Build errors are unconditionally unwrapped, which may panic.
Handle build errors explicitly to avoid panics and improve error reporting in CI and production.
```suggestion
if let Err(e) = tauri_build::try_build(attributes) {
eprintln!("Error during tauri build: {e}");
std::process::exit(1);
}
}
```
</issue_to_address>
### 评论 2
<location> `src-tauri/src/scheduler/shared_state.rs:15` </location>
<code_context>
//! │ SharedState │
//! │ │
//! │ pause_reasons │ ◄─── Manager (add/remove)
-//! │ in_break_.. │ ◄─── BreakScheduler (start/end)
+//! │ in_break_... │ ◄─── BreakScheduler (start/end)
//! │ in_attention_.. │ ◄─── AttentionTimer (start/end)
//! └────────┬────────┘
</code_context>
<issue_to_address>
**nitpick (typo):** Typo fix in documentation for in_break_... field.
```suggestion
//! │ in_break_session │ ◄─── BreakScheduler (start/end)
```
</issue_to_address>
### 评论 3
<location> `src-tauri/src/scheduler/break_scheduler.rs:51` </location>
<code_context>
/// Main break scheduler responsible for managing mini and long breaks
-pub struct BreakScheduler {
- app_handle: AppHandle,
+pub struct BreakScheduler<E, R = tauri::Wry>
+where
+ E: EventEmitter,
</code_context>
<issue_to_address>
**issue (complexity):** Consider boxing the event emitter and moving pure scheduling logic and its tests into a separate module to simplify the scheduler's generics and file structure.
```suggestion
Reduce the generic plumbing by boxing your emitter, and pull the pure scheduling logic (and its tests) into its own module. For example:
1) Change `BreakScheduler` to use a `Box<dyn EventEmitter>` instead of `<E, R>`:
// before
```rust
pub struct BreakScheduler<E, R = tauri::Wry>
where
E: EventEmitter,
R: Runtime,
{
app_handle: AppHandle<R>,
event_emitter: E,
// …
}
```
// after
```rust
pub struct BreakScheduler {
app_handle: AppHandle, // drop the R generic
event_emitter: Box<dyn EventEmitter + Send>, // one trait-object
shutdown_rx: watch::Receiver<()>,
// …
}
impl BreakScheduler {
pub fn new(
app_handle: AppHandle,
event_emitter: Box<dyn EventEmitter + Send>,
shutdown_rx: watch::Receiver<()>,
shared_state: SharedState,
) -> Self {
Self { app_handle, event_emitter, shutdown_rx, /* … */ }
}
// …
}
```
2) Extract the pure scheduling function (and its tests) into `core/scheduler/pure.rs`:
```rust
// core/scheduler/pure.rs
use chrono::{DateTime, Local, Utc};
use crate::{config::AppConfig, scheduler::BreakInfo, core::schedule::ScheduleSettings};
pub(crate) fn calculate_next_break_pure(
config: &AppConfig,
now: DateTime<Utc>,
mini_break_counter: u8,
last_break_time: Option<DateTime<Utc>>,
) -> Option<BreakInfo> {
// … original logic …
}
```
Then in `scheduler.rs`:
```rust
mod pure; // <— new module
impl BreakScheduler {
fn calculate_next_break(&self, config: &AppConfig) -> Option<BreakInfo> {
pure::calculate_next_break_pure(
config,
Utc::now(),
self.mini_break_counter,
self.last_break_time,
)
}
}
```
3) Move the big `#[cfg(test)] mod tests { … }` block from `scheduler.rs` into `pure.rs` (or into `tests/`) so the core file stays focused. Remove all the `#[cfg(test)]` getters in `BreakScheduler`—tests can drive the scheduler solely via the boxed emitter or the pure function. This will collapse ~600 lines of generics/tests/duplication into:
- A small `scheduler.rs` with runtime/emitter boxed
- A `pure.rs` with scheduling logic + its tests.
</issue_to_address>
### 评论 4
<location> `src-tauri/src/scheduler/event_emitter.rs:21` </location>
<code_context>
+/// making schedulers testable without requiring a full Tauri runtime.
+///
+/// The `emit` method signature matches Tauri's `Emitter::emit` for consistency.
+pub trait EventEmitter: Send + Sync {
+ /// Emit an event with the given name and payload
+ ///
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the trait and impls with a boxed function alias and helpers to simplify event emission.
You can collapse all of that boilerplate into a single boxed‐fn alias + two tiny helpers, then inject that everywhere instead of a 178-line trait and two impls:
```rust
use serde::Serialize;
use serde_json::Value;
use std::sync::Arc;
/// A single, shared emitter function that takes (event_name, JSON payload)
pub type EmitterFn = Arc<dyn Fn(&str, Value) -> Result<(), String> + Send + Sync>;
/// Helper to turn any `Serialize` into a `Value` and call the EmitterFn
pub fn emit<S: Serialize>(
emitter: &EmitterFn,
event: &str,
payload: &S,
) -> Result<(), String> {
let value = serde_json::to_value(payload)
.map_err(|e| format!("serialize failed: {}", e))?;
(emitter)(event, value)
}
```
– In production just wrap your `AppHandle` once:
```rust
let prod_emitter: EmitterFn = Arc::new(move |event, value| {
app_handle
.emit(event, value)
.map_err(|e| e.to_string())
});
```
– In tests use a tiny closure + shared Vec:
```rust
use parking_lot::Mutex;
let events = Arc::new(Mutex::new(Vec::<(String, Value)>::new()));
let test_emitter: EmitterFn = {
let events = events.clone();
Arc::new(move |event, value| {
events.lock().push((event.to_string(), value.clone()));
Ok(())
})
};
// later in your test:
emit(&test_emitter, "foo", &my_payload).unwrap();
assert_eq!(events.lock().len(), 1);
```
This removes the 178-line trait, the two impls, plus all the test scaffolding—you keep exactly the same behavior, but with ~20 lines of code.帮助我更有用！请点击每个评论上的 👍 或 👎，我将利用这些反馈来改进你的评论。
Original comment in English
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src-tauri/build.rs:10-11` </location>
<code_context>
+ .windows_attributes(tauri_build::WindowsAttributes::new_without_app_manifest());
+ add_manifest();
+ }
+ tauri_build::try_build(attributes).unwrap();
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Build errors are unconditionally unwrapped, which may panic.
Handle build errors explicitly to avoid panics and improve error reporting in CI and production.
```suggestion
if let Err(e) = tauri_build::try_build(attributes) {
eprintln!("Error during tauri build: {e}");
std::process::exit(1);
}
}
```
</issue_to_address>
### Comment 2
<location> `src-tauri/src/scheduler/shared_state.rs:15` </location>
<code_context>
//! │ SharedState │
//! │ │
//! │ pause_reasons │ ◄─── Manager (add/remove)
-//! │ in_break_.. │ ◄─── BreakScheduler (start/end)
+//! │ in_break_... │ ◄─── BreakScheduler (start/end)
//! │ in_attention_.. │ ◄─── AttentionTimer (start/end)
//! └────────┬────────┘
</code_context>
<issue_to_address>
**nitpick (typo):** Typo fix in documentation for in_break_... field.
```suggestion
//! │ in_break_session │ ◄─── BreakScheduler (start/end)
```
</issue_to_address>
### Comment 3
<location> `src-tauri/src/scheduler/break_scheduler.rs:51` </location>
<code_context>
/// Main break scheduler responsible for managing mini and long breaks
-pub struct BreakScheduler {
- app_handle: AppHandle,
+pub struct BreakScheduler<E, R = tauri::Wry>
+where
+ E: EventEmitter,
</code_context>
<issue_to_address>
**issue (complexity):** Consider boxing the event emitter and moving pure scheduling logic and its tests into a separate module to simplify the scheduler's generics and file structure.
```suggestion
Reduce the generic plumbing by boxing your emitter, and pull the pure scheduling logic (and its tests) into its own module. For example:
1) Change `BreakScheduler` to use a `Box<dyn EventEmitter>` instead of `<E, R>`:
// before
```rust
pub struct BreakScheduler<E, R = tauri::Wry>
where
E: EventEmitter,
R: Runtime,
{
app_handle: AppHandle<R>,
event_emitter: E,
// …
}
```
// after
```rust
pub struct BreakScheduler {
app_handle: AppHandle, // drop the R generic
event_emitter: Box<dyn EventEmitter + Send>, // one trait-object
shutdown_rx: watch::Receiver<()>,
// …
}
impl BreakScheduler {
pub fn new(
app_handle: AppHandle,
event_emitter: Box<dyn EventEmitter + Send>,
shutdown_rx: watch::Receiver<()>,
shared_state: SharedState,
) -> Self {
Self { app_handle, event_emitter, shutdown_rx, /* … */ }
}
// …
}
```
2) Extract the pure scheduling function (and its tests) into `core/scheduler/pure.rs`:
```rust
// core/scheduler/pure.rs
use chrono::{DateTime, Local, Utc};
use crate::{config::AppConfig, scheduler::BreakInfo, core::schedule::ScheduleSettings};
pub(crate) fn calculate_next_break_pure(
config: &AppConfig,
now: DateTime<Utc>,
mini_break_counter: u8,
last_break_time: Option<DateTime<Utc>>,
) -> Option<BreakInfo> {
// … original logic …
}
```
Then in `scheduler.rs`:
```rust
mod pure; // <— new module
impl BreakScheduler {
fn calculate_next_break(&self, config: &AppConfig) -> Option<BreakInfo> {
pure::calculate_next_break_pure(
config,
Utc::now(),
self.mini_break_counter,
self.last_break_time,
)
}
}
```
3) Move the big `#[cfg(test)] mod tests { … }` block from `scheduler.rs` into `pure.rs` (or into `tests/`) so the core file stays focused. Remove all the `#[cfg(test)]` getters in `BreakScheduler`—tests can drive the scheduler solely via the boxed emitter or the pure function. This will collapse ~600 lines of generics/tests/duplication into:
- A small `scheduler.rs` with runtime/emitter boxed
- A `pure.rs` with scheduling logic + its tests.
</issue_to_address>
### Comment 4
<location> `src-tauri/src/scheduler/event_emitter.rs:21` </location>
<code_context>
+/// making schedulers testable without requiring a full Tauri runtime.
+///
+/// The `emit` method signature matches Tauri's `Emitter::emit` for consistency.
+pub trait EventEmitter: Send + Sync {
+ /// Emit an event with the given name and payload
+ ///
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the trait and impls with a boxed function alias and helpers to simplify event emission.
You can collapse all of that boilerplate into a single boxed‐fn alias + two tiny helpers, then inject that everywhere instead of a 178-line trait and two impls:
```rust
use serde::Serialize;
use serde_json::Value;
use std::sync::Arc;
/// A single, shared emitter function that takes (event_name, JSON payload)
pub type EmitterFn = Arc<dyn Fn(&str, Value) -> Result<(), String> + Send + Sync>;
/// Helper to turn any `Serialize` into a `Value` and call the EmitterFn
pub fn emit<S: Serialize>(
emitter: &EmitterFn,
event: &str,
payload: &S,
) -> Result<(), String> {
let value = serde_json::to_value(payload)
.map_err(|e| format!("serialize failed: {}", e))?;
(emitter)(event, value)
}
```
– In production just wrap your `AppHandle` once:
```rust
let prod_emitter: EmitterFn = Arc::new(move |event, value| {
app_handle
.emit(event, value)
.map_err(|e| e.to_string())
});
```
– In tests use a tiny closure + shared Vec:
```rust
use parking_lot::Mutex;
let events = Arc::new(Mutex::new(Vec::<(String, Value)>::new()));
let test_emitter: EmitterFn = {
let events = events.clone();
Arc::new(move |event, value| {
events.lock().push((event.to_string(), value.clone()));
Ok(())
})
};
// later in your test:
emit(&test_emitter, "foo", &my_payload).unwrap();
assert_eq!(events.lock().len(), 1);
```
This removes the 178-line trait, the two impls, plus all the test scaffolding—you keep exactly the same behavior, but with ~20 lines of code.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the scheduler system to improve testability by introducing event emitter abstraction and extracting pure functions. The changes enable comprehensive unit testing of scheduler logic without requiring a full Tauri runtime.
Key changes:
- Introduced
EventEmittertrait with production (TauriEventEmitter) and test (TestEventEmitter) implementations to decouple event emission from Tauri's AppHandle - Extracted pure calculation functions (
calculate_next_break_pure,get_active_schedule,calculate_next_attention) from scheduler methods for easier testing - Added comprehensive test helpers module with builders, assertions, and time utilities
- Made generic types (
AppHandle<R>,Runtime) explicit to support both production (Wry) and test (MockRuntime) contexts - Added
mini_break_countertoSchedulerStatusfor frontend visibility - Configured test execution with
--test-threads=1due totokio::time::pause()global effects
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src-tauri/src/scheduler/event_emitter.rs |
New trait-based event emission abstraction with test and production implementations |
src-tauri/src/scheduler/test_helpers.rs |
Comprehensive test utilities including config builders, time helpers, and assertion functions |
src-tauri/src/scheduler/break_scheduler.rs |
Refactored to use EventEmitter trait and extracted pure functions; added extensive unit tests |
src-tauri/src/scheduler/attention_timer.rs |
Refactored to use EventEmitter trait and extracted pure attention calculation functions |
src-tauri/src/scheduler/models.rs |
Added mini_break_counter field to SchedulerStatus |
src-tauri/src/scheduler/manager.rs |
Updated to instantiate TauriEventEmitter and pass to schedulers |
src-tauri/src/scheduler/event.rs |
Refactored tests to use test helpers |
src-tauri/src/scheduler/shared_state.rs |
Added comprehensive edge case tests for state management |
src-tauri/src/platform/window.rs |
Made functions generic over Runtime type parameter |
src-tauri/src/platform/notifications.rs |
Made functions generic over Runtime type parameter |
src-tauri/build.rs |
Added Windows manifest handling to address common controls issue |
src-tauri/Cargo.toml |
Added test dependencies for Tauri and tokio test features |
justfile |
Added --test-threads=1 flag and new test-lib command |
.cargo/config.toml |
Updated linker configuration and added Tauri workspace workaround |
src-tauri/windows-app-manifest.xml |
New Windows application manifest for common controls |
src/types/generated/SchedulerStatus.ts |
Auto-generated TypeScript type with new miniBreakCounter field |
![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)
src-tauri/src/scheduler/attention_timer.rs (1)
147-160: Attention events no longer reach listeners
AttentionTimernow accepts anEventEmitter, buttrigger_attentionnever uses it. As a result, no"scheduler-event"is published when an attention fires, so the frontend/tests that listen for that topic stop seeing attention prompts altogether. Please emit the event (clone the enum so it can still be moved into the spawn) before creating the prompt window.self.shared_state.write().start_attention_session(); - let event = SchedulerEvent::Attention(attention_id); + let event = SchedulerEvent::Attention(attention_id); + + if let Err(e) = self + .event_emitter + .emit("scheduler-event", event.clone()) + { + tracing::warn!("Failed to emit scheduler-event: {e}"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/types/generated/SchedulerStatus.tsis excluded by!**/generated/**
📒 Files selected for processing (16)
.cargo/config.toml(1 hunks)justfile(3 hunks)src-tauri/Cargo.toml(1 hunks)src-tauri/build.rs(1 hunks)src-tauri/src/platform/notifications.rs(3 hunks)src-tauri/src/platform/window.rs(2 hunks)src-tauri/src/scheduler.rs(1 hunks)src-tauri/src/scheduler/attention_timer.rs(4 hunks)src-tauri/src/scheduler/break_scheduler.rs(12 hunks)src-tauri/src/scheduler/event.rs(5 hunks)src-tauri/src/scheduler/event_emitter.rs(1 hunks)src-tauri/src/scheduler/manager.rs(4 hunks)src-tauri/src/scheduler/models.rs(1 hunks)src-tauri/src/scheduler/shared_state.rs(2 hunks)src-tauri/src/scheduler/test_helpers.rs(1 hunks)src-tauri/windows-app-manifest.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src-tauri/src/scheduler/manager.rs (2)
src-tauri/src/scheduler/break_scheduler.rs (6)
new(74-89)app_handle(196-196)app_handle(240-240)app_handle(261-261)app_handle(331-331)app_handle(612-612)src-tauri/src/scheduler/attention_timer.rs (3)
new(35-48)app_handle(84-84)app_handle(178-178)
src-tauri/src/scheduler/event.rs (2)
src-tauri/src/scheduler/test_helpers.rs (5)
time_range(124-127)time_range(290-295)days_of_week(130-133)full_time_range(298-303)naive_time(285-287)src-tauri/src/scheduler/break_scheduler.rs (1)
get_active_schedule(688-696)
src-tauri/src/scheduler/attention_timer.rs (3)
src-tauri/src/scheduler/break_scheduler.rs (6)
new(74-89)app_handle(196-196)app_handle(240-240)app_handle(261-261)app_handle(331-331)app_handle(612-612)src-tauri/src/scheduler/test_helpers.rs (6)
new(52-91)naive_time(285-287)all_weekdays(185-195)default(175-177)test_local_datetime(250-262)weekend_days(209-211)src-tauri/src/core/schedule.rs (9)
new(26-28)new(190-192)default(38-40)default(68-80)default(97-102)default(119-127)default(152-170)default(202-204)default(233-253)
src-tauri/src/platform/notifications.rs (1)
src-tauri/src/scheduler/break_scheduler.rs (1)
send_notification(188-209)
src-tauri/src/scheduler/break_scheduler.rs (5)
src/types/generated/SchedulerStatus.ts (1)
SchedulerStatus(7-19)src-tauri/src/scheduler/attention_timer.rs (3)
new(35-48)app_handle(84-84)app_handle(178-178)src-tauri/src/scheduler/test_helpers.rs (11)
new(52-91)default(175-177)time_range(124-127)time_range(290-295)naive_time(285-287)all_weekdays(185-195)duration_s(270-272)assert_time_near(319-327)test_datetime(220-231)test_datetime_with_local(234-247)assert_duration_near(343-349)src-tauri/src/platform/window.rs (1)
create_prompt_windows(25-86)src-tauri/src/scheduler/event.rs (1)
get_active_schedule(20-28)
src-tauri/src/scheduler/shared_state.rs (2)
src-tauri/src/scheduler/break_scheduler.rs (1)
new(74-89)src-tauri/src/scheduler/attention_timer.rs (2)
new(35-48)is_paused(53-55)
src-tauri/src/platform/window.rs (1)
src-tauri/src/platform/notifications.rs (1)
app(41-41)
src-tauri/src/scheduler/test_helpers.rs (6)
src/types/generated/TimeRange.ts (1)
TimeRange(3-3)src-tauri/src/scheduler/break_scheduler.rs (6)
new(74-89)app_handle(196-196)app_handle(240-240)app_handle(261-261)app_handle(331-331)app_handle(612-612)src-tauri/src/scheduler/shared_state.rs (1)
create_shared_state(372-374)src-tauri/src/scheduler/manager.rs (1)
init(21-80)src-tauri/src/scheduler/event_emitter.rs (2)
get_events(76-78)has_event(101-106)src-tauri/src/core/time.rs (1)
contains(56-63)
🪛 GitHub Actions: PR Quality Check
src-tauri/src/scheduler/event.rs
[error] 1-1: cargo fmt --manifest-path src-tauri/Cargo.toml --all --check failed. Formatting differences detected (diff present in the file). Please run 'cargo fmt' to fix formatting.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (6)
src-tauri/src/platform/window.rs (1)
25-26: Generic runtime parameterization looks correct.
AppHandle<R>cleanly matches the tauri API expectations and keeps the existing window creation flow untouched.Also applies to: 167-168
src-tauri/src/platform/notifications.rs (1)
13-17: Runtime generics applied consistently.Propagating
AppHandle<R>keeps these helpers aligned with the runtime-agnostic scheduler work without altering notification behavior.Also applies to: 35-38
justfile (4)
37-37: LGTM!The alias follows the established naming convention and provides a convenient shortcut for the new
test-librecipe.
211-216: Excellent addition of test isolation.The
--test-threads=1flag correctly prevents race conditions when tests usetokio::time::pause(), which manipulates global time state. The documentation clearly explains the rationale.
241-246: LGTM!The enhanced documentation and
--test-threads=1flag addition properly isolate tests usingtokio::time::pause(). The comment clearly explains why concurrent execution would cause issues with global time manipulation.
219-224: No action required — library tests do not usetokio::time::pause(), so the concern about race conditions and the need for--test-threads=1does not apply here. The "pause" references found are related to scheduler pause logic (pause reasons, pause state), not time manipulation.The
test-librecipe is appropriately consistent withtest-back, which also allows parallel test execution for running specific tests.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src-tauri/src/scheduler/break_scheduler.rs (1)
693-700: Avoid duplicating schedule lookup logicWe already expose the same
get_active_schedulehelper inscheduler/event.rs. Keeping a second copy here invites divergence if we tweak the matching rules later. Please reuse the existing helper (or move it into a shared module) instead of cloning the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
src/types/generated/AppConfig.tsis excluded by!**/generated/**src/types/generated/ResolvedBackground.tsis excluded by!**/generated/**src/types/generated/SuggestionsSettings.tsis excluded by!**/generated/**src/types/generated/ThemeSettings.tsis excluded by!**/generated/**
📒 Files selected for processing (15)
src-tauri/capabilities/default.json(1 hunks)src-tauri/src/cmd.rs(1 hunks)src-tauri/src/cmd/window.rs(1 hunks)src-tauri/src/config/models.rs(1 hunks)src-tauri/src/core/suggestions.rs(1 hunks)src-tauri/src/core/theme.rs(2 hunks)src-tauri/src/lib.rs(2 hunks)src-tauri/src/monitors.rs(3 hunks)src-tauri/src/monitors/dnd.rs(1 hunks)src-tauri/src/monitors/orchestrator.rs(6 hunks)src-tauri/src/platform/window.rs(5 hunks)src-tauri/src/scheduler/break_scheduler.rs(14 hunks)src-tauri/src/scheduler/shared_state.rs(3 hunks)src/components/settings/AdvancedPanel.vue(1 hunks)src/views/PromptApp.vue(7 hunks)
✅ Files skipped from review due to trivial changes (3)
- src-tauri/src/core/suggestions.rs
- src-tauri/src/core/theme.rs
- src/components/settings/AdvancedPanel.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/platform/window.rs
🧰 Additional context used
🧬 Code graph analysis (6)
src-tauri/src/cmd.rs (1)
src-tauri/src/cmd/window.rs (2)
close_all_prompt_windows(14-34)open_settings_window(7-9)
src-tauri/src/cmd/window.rs (1)
src-tauri/src/platform/window.rs (3)
app(33-33)app(36-36)app(54-54)
src-tauri/src/lib.rs (2)
src-tauri/src/monitors/dnd.rs (1)
new(52-61)src-tauri/src/cmd/window.rs (1)
close_all_prompt_windows(14-34)
src-tauri/src/monitors/dnd.rs (4)
src-tauri/src/monitors/app_whitelist.rs (1)
new(29-38)src-tauri/src/monitors/idle.rs (1)
new(102-109)src-tauri/src/platform/dnd/windows.rs (2)
new(38-40)new(56-62)src-tauri/src/platform/dnd/linux.rs (1)
new(47-56)
src-tauri/src/scheduler/shared_state.rs (2)
src-tauri/src/scheduler/break_scheduler.rs (1)
new(74-89)src-tauri/src/scheduler/attention_timer.rs (2)
new(35-48)is_paused(53-55)
src-tauri/src/scheduler/break_scheduler.rs (5)
src/types/generated/SchedulerStatus.ts (1)
SchedulerStatus(7-19)src-tauri/src/scheduler/test_helpers.rs (12)
new(52-91)default(175-177)time_range(124-127)time_range(290-295)workdays(198-206)naive_time(285-287)all_weekdays(185-195)duration_s(270-272)assert_time_near(319-327)test_datetime(220-231)test_datetime_with_local(234-247)assert_duration_near(343-349)src-tauri/src/scheduler/attention_timer.rs (3)
new(35-48)app_handle(84-84)app_handle(178-178)src-tauri/src/platform/window.rs (1)
create_prompt_windows(25-93)src-tauri/src/scheduler/event.rs (1)
get_active_schedule(20-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
src-tauri/src/config/models.rs (1)
30-30: LGTM! Documentation updated to reflect terminology change.The update from "Break window" to "Prompt window" aligns with the broader terminology refactor across this PR. The functional behavior and field name remain unchanged.
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)
src-tauri/src/scheduler/break_scheduler.rs (1)
519-536: Restore notifications after postponingWhen we postpone while still in
WaitingForNotification, we blanknotification_timeand skip back toWaitingForBreak. That drops the pre-break reminder entirely—even if the schedule hasnotification_before_sconfigured—so the user never receives the follow-up ping they deferred for. Please recompute a freshnotification_time(and keep the state inWaitingForNotificationwhenever the schedule wants a lead-time notification) instead of forcing it toNone. citeturn0annotated529
🧹 Nitpick comments (1)
src-tauri/src/scheduler/break_scheduler.rs (1)
693-700: Reuse existing active-schedule helperWe now have two copies of
get_active_schedule(here and incrate::scheduler::event). Duplicating the logic invites drift; consider reusing the existing helper or moving the pure version into a shared module so we maintain one source of truth. citeturn0annotated693
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
justfile(2 hunks)src-tauri/src/scheduler.rs(1 hunks)src-tauri/src/scheduler/attention_timer_tests.rs(1 hunks)src-tauri/src/scheduler/break_scheduler.rs(13 hunks)src-tauri/src/scheduler/break_scheduler_tests.rs(1 hunks)src-tauri/src/scheduler/event.rs(5 hunks)src-tauri/src/scheduler/manager.rs(5 hunks)src-tauri/src/scheduler/manager_integration_tests.rs(1 hunks)src-tauri/src/scheduler/monitor_integration_tests.rs(1 hunks)src-tauri/src/scheduler/stress_tests.rs(1 hunks)src-tauri/src/scheduler/test_helpers.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src-tauri/src/scheduler/event.rs
- justfile
- src-tauri/src/scheduler.rs
🧰 Additional context used
🧬 Code graph analysis (8)
src-tauri/src/scheduler/manager_integration_tests.rs (2)
src-tauri/src/scheduler/test_helpers.rs (4)
advance_time_and_yield(525-534)create_manager_test_env(713-750)spawn_test_manager(756-809)duration_ms(265-267)src-tauri/src/scheduler/shared_state.rs (1)
pause_reasons(257-259)
src-tauri/src/scheduler/stress_tests.rs (2)
src-tauri/src/scheduler/test_helpers.rs (3)
create_manager_test_env(713-750)spawn_test_manager(756-809)advance_time_and_yield(525-534)src-tauri/src/scheduler/shared_state.rs (1)
pause_reasons(257-259)
src-tauri/src/scheduler/attention_timer_tests.rs (1)
src-tauri/src/scheduler/test_helpers.rs (14)
mpsc(761-761)mpsc(762-762)advance_time_and_yield(525-534)new(52-91)naive_time(285-287)days_of_week(130-133)duration_s(270-272)all_weekdays(185-195)create_manager_test_env(713-750)spawn_test_manager(756-809)duration_ms(265-267)default(175-177)workdays(198-206)weekend_days(209-211)
src-tauri/src/scheduler/break_scheduler_tests.rs (3)
src-tauri/src/scheduler/test_helpers.rs (8)
create_manager_test_env(713-750)spawn_test_manager(756-809)create_test_break_scheduler(478-517)advance_time_and_yield(525-534)duration_ms(265-267)get_latest_status(655-659)assert_duration_near(343-349)duration_s(270-272)src-tauri/src/scheduler/event_emitter.rs (1)
has_event(101-106)src-tauri/src/scheduler/shared_state.rs (1)
in_any_session(266-268)
src-tauri/src/scheduler/monitor_integration_tests.rs (2)
src-tauri/src/scheduler/test_helpers.rs (4)
advance_time_and_yield(525-534)create_manager_test_env(713-750)spawn_test_manager(756-809)duration_ms(265-267)src-tauri/src/scheduler/shared_state.rs (4)
pause_reasons(257-259)in_break_session(272-274)in_attention_session(278-280)in_any_session(266-268)
src-tauri/src/scheduler/break_scheduler.rs (4)
src-tauri/src/scheduler/test_helpers.rs (12)
new(52-91)default(175-177)time_range(124-127)time_range(290-295)workdays(198-206)naive_time(285-287)all_weekdays(185-195)duration_s(270-272)assert_time_near(319-327)test_datetime(220-231)test_datetime_with_local(234-247)assert_duration_near(343-349)src-tauri/src/scheduler/attention_timer.rs (3)
new(35-48)app_handle(84-84)app_handle(178-178)src-tauri/src/platform/window.rs (1)
create_prompt_windows(25-93)src-tauri/src/scheduler/event.rs (1)
get_active_schedule(20-28)
src-tauri/src/scheduler/test_helpers.rs (4)
src-tauri/src/scheduler/shared_state.rs (1)
create_shared_state(372-374)src-tauri/src/scheduler/manager.rs (2)
init(21-80)broadcast_commands(119-188)src-tauri/src/scheduler/event_emitter.rs (2)
get_events(76-78)has_event(101-106)src-tauri/src/core/time.rs (1)
contains(56-63)
src-tauri/src/scheduler/manager.rs (4)
src-tauri/src/scheduler/break_scheduler.rs (6)
new(74-89)app_handle(196-196)app_handle(244-244)app_handle(265-265)app_handle(335-335)app_handle(616-616)src-tauri/src/scheduler/shared_state.rs (1)
new(172-180)src-tauri/src/scheduler/attention_timer.rs (3)
new(35-48)app_handle(84-84)app_handle(178-178)src/types/generated/SchedulerStatus.ts (1)
SchedulerStatus(7-19)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src-tauri/build.rs (1)
23-26: Critical: Quote the manifest path to handle spaces.As already noted in previous review feedback, paths with spaces (e.g.,
C:\Users\Jane Doe\Focust) will break the MSVC linker since it splits arguments at spaces. The existing suggestion to wrap the path in quotes and usemanifest.display()is correct.src-tauri/src/scheduler/test_helpers.rs (1)
756-809: Fix: Reuse the shared TestEventEmitter from the environment.Lines 766 and 781 create fresh
TestEventEmitter::new()instances instead of cloningenv.event_emitter. This means emitted events are not recorded in the shared test emitter that callers inspect, causing tests to fail even though the schedulers do emit.Apply this diff:
// Spawn break scheduler (using TestEventEmitter for tests) let break_scheduler_handle = env.app_handle.clone(); - let break_event_emitter = TestEventEmitter::new(); // Use TestEventEmitter in tests + let break_event_emitter = env.event_emitter.clone(); let break_shutdown_rx = env.shutdown_tx.subscribe(); let break_shared_state = env.shared_state.clone(); tokio::spawn(async move {// Spawn attention timer (using TestEventEmitter for tests) let attention_timer_handle = env.app_handle.clone(); - let attention_event_emitter = TestEventEmitter::new(); // Use TestEventEmitter in tests + let attention_event_emitter = env.event_emitter.clone(); let attention_shutdown_rx = env.shutdown_tx.subscribe(); let attention_shared_state = env.shared_state.clone(); tokio::spawn(async move {
🧹 Nitpick comments (1)
src-tauri/src/scheduler/attention_timer.rs (1)
218-218: Minor: Unnecessary parentheses.The negation operator doesn't require parentheses.
Apply this diff:
- if (!attention.enabled) || attention.times.is_empty() || attention.days_of_week.is_empty() { + if !attention.enabled || attention.times.is_empty() || attention.days_of_week.is_empty() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.cargo/config.toml(1 hunks)src-tauri/build.rs(1 hunks)src-tauri/src/scheduler/attention_timer.rs(4 hunks)src-tauri/src/scheduler/break_scheduler.rs(12 hunks)src-tauri/src/scheduler/test_helpers.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .cargo/config.toml
🧰 Additional context used
🧬 Code graph analysis (3)
src-tauri/src/scheduler/attention_timer.rs (3)
src-tauri/src/scheduler/test_helpers.rs (6)
new(52-91)naive_time(285-287)all_weekdays(185-195)default(175-177)test_local_datetime(250-262)weekend_days(209-211)src-tauri/src/scheduler/break_scheduler.rs (6)
new(74-89)app_handle(196-196)app_handle(244-244)app_handle(265-265)app_handle(335-335)app_handle(616-616)src-tauri/src/core/schedule.rs (9)
new(26-28)new(190-192)default(38-40)default(68-80)default(97-102)default(119-127)default(152-170)default(202-204)default(233-253)
src-tauri/src/scheduler/break_scheduler.rs (6)
src-tauri/src/scheduler/test_helpers.rs (12)
new(52-91)default(175-177)time_range(124-127)time_range(290-295)workdays(198-206)naive_time(285-287)all_weekdays(185-195)duration_s(270-272)assert_time_near(319-327)test_datetime(220-231)test_datetime_with_local(234-247)assert_duration_near(343-349)src-tauri/src/platform/window.rs (1)
create_prompt_windows(25-93)src-tauri/src/platform/notifications.rs (1)
send_break_notification(35-70)src-tauri/src/scheduler/event.rs (1)
get_active_schedule(20-28)src-tauri/src/scheduler/attention_timer.rs (3)
new(35-48)app_handle(84-84)app_handle(178-178)src-tauri/src/scheduler/shared_state.rs (2)
new(172-180)default(362-364)
src-tauri/src/scheduler/test_helpers.rs (6)
src-tauri/src/scheduler/attention_timer.rs (3)
new(35-48)app_handle(84-84)app_handle(178-178)src-tauri/src/scheduler/break_scheduler.rs (6)
new(74-89)app_handle(196-196)app_handle(244-244)app_handle(265-265)app_handle(335-335)app_handle(616-616)src-tauri/src/scheduler/shared_state.rs (1)
create_shared_state(372-374)src-tauri/src/scheduler/manager.rs (2)
init(21-80)broadcast_commands(119-188)src-tauri/src/scheduler/event_emitter.rs (2)
get_events(76-78)has_event(101-106)src-tauri/src/core/time.rs (1)
contains(56-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Upload results
🔇 Additional comments (13)
src-tauri/build.rs (1)
4-11: Good Windows-specific build configuration structure.The conditional Windows manifest handling is well-structured with proper
#[cfg(windows)]guards. The overall approach of customizing Tauri build attributes for Windows is sound.However, note the existing feedback about error handling on line 11 - build scripts should handle errors gracefully rather than panicking.
src-tauri/src/scheduler/attention_timer.rs (3)
18-48: LGTM! Generic refactoring enables testability.The generic parameters
E: EventEmitterandR: Runtimeallow dependency injection of test doubles, and the constructor signature is consistent withBreakScheduler.
198-211: LGTM! Clean functional approach.The pure function correctly filters and finds the earliest attention time using
filter_mapandmin_by_key.
279-702: Excellent test coverage!The test suite is comprehensive, well-organized, and covers numerous edge cases including DST handling, weekend-only schedules, and week wrapping scenarios.
src-tauri/src/scheduler/break_scheduler.rs (5)
51-110: LGTM! Generic refactoring enables comprehensive testing.The struct is now generic over
E: EventEmitterandR: Runtime, allowing test doubles to be injected. Test-only accessors are properly gated with#[cfg(test)].
116-120: Good defensive logic.Properly guards the initial state transition to prevent scheduling when the scheduler starts in a paused state.
375-416: LGTM! Proper session state management and test mode handling.The method correctly:
- Marks the break session before creating windows (preventing DND monitor interference)
- Emits events for tests to verify
- Cleans up session state on window creation errors
- Skips window creation in test mode (MockRuntime doesn't support it)
630-686: Excellent pure function design!The extracted
calculate_next_break_purefunction takes all inputs as explicit parameters, has no side effects, and is easily testable in isolation. The logic for determining break type and notification time is correct.
688-1076: Comprehensive test suite with excellent edge case coverage!The tests thoroughly exercise both
get_active_scheduleandcalculate_next_break_purewith scenarios including notification timing, multiple schedules, disabled breaks, and known limitations (cross-midnight marked as ignored with explanation).src-tauri/src/scheduler/test_helpers.rs (4)
38-178: Excellent fluent API design!The
TestConfigBuilderprovides sensible defaults and comprehensive customization options with a clean, readable interface. The documentation and examples are helpful.
184-349: Comprehensive and well-documented time utilities!The collection of weekday, datetime, duration, and assertion helpers covers common testing needs with clear naming and helpful documentation.
356-454: Good practice: testing the test helpers!The
minimal_schedulefactory is useful for specific scenarios, and the unit tests for the helpers themselves ensure reliability.
460-669: Well-designed state machine testing utilities!The module provides comprehensive helpers for testing schedulers, including proper mock setup, time advancement with yielding, and event assertion utilities.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src-tauri/src/monitors/orchestrator.rs (1)
230-237: LGTM! Session-aware skipping correctly implemented.The implementation properly caches the session state once per iteration (line 230) before the monitor loop, which addresses the concern from the past review comment. The skipping logic correctly prevents monitors from interfering with active sessions when they opt in via
skip_during_session().The lock is acquired and released immediately when computing
in_session, so no lock is held during monitor checks—this is efficient and correct.src-tauri/build.rs (2)
11-11: Handletry_builderrors instead of unwrappingUnwrapping here will panic and hide the real root cause when
tauri_build::try_buildfails (e.g., linker/manifest mistakes), making CI and developer runs noisy instead of reporting the build error cleanly. Please surface the error and exit gracefully.- tauri_build::try_build(attributes).unwrap(); + if let Err(error) = tauri_build::try_build(attributes) { + eprintln!("tauri-build failed: {error}"); + std::process::exit(1); + }
22-28: Quote the manifest path and drop theto_str().unwrap()Passing
/MANIFESTINPUT:without quoting breaks whenever the project path contains spaces (very common on Windows), because the MSVC linker splits the argument at the first space. Additionally,to_str().unwrap()will panic on non-UTF-8 paths. Wrap the path in quotes and rely ondisplay()(orto_string_lossy()) to avoid both issues.- println!( - "cargo:rustc-link-arg=/MANIFESTINPUT:{}", - manifest.to_str().unwrap() - ); + println!( + "cargo:rustc-link-arg=/MANIFESTINPUT:\"{}\"", + manifest.display() + );
🧹 Nitpick comments (1)
src-tauri/src/monitors/orchestrator.rs (1)
174-174: Consider updating monitor responsibility documentation.The monitor responsibilities section states "Check [
SharedState] if needed to avoid interfering with sessions," but this is now handled automatically by the orchestrator for monitors withskip_during_session() == true. Consider clarifying that monitors can either implementskip_during_session()for automatic orchestrator-level protection or manually checkSharedStatefor custom session-aware logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src-tauri/build.rs(1 hunks)src-tauri/src/monitors/orchestrator.rs(6 hunks)src-tauri/src/scheduler/attention_timer.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src-tauri/src/scheduler/attention_timer.rs (3)
src-tauri/src/scheduler/test_helpers.rs (6)
new(52-91)naive_time(285-287)all_weekdays(185-195)default(175-177)test_local_datetime(250-262)weekend_days(209-211)src-tauri/src/scheduler/break_scheduler.rs (6)
new(74-89)app_handle(196-196)app_handle(244-244)app_handle(265-265)app_handle(335-335)app_handle(616-616)src-tauri/src/core/schedule.rs (9)
new(26-28)new(190-192)default(38-40)default(68-80)default(97-102)default(119-127)default(152-170)default(202-204)default(233-253)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Quality Check
🔇 Additional comments (5)
src-tauri/src/monitors/orchestrator.rs (3)
58-81: LGTM! Clean integration of shared state.The addition of the
shared_stateparameter is well-documented and correctly propagated torun_monitors. The documentation clearly explains the session-aware behavior.Also applies to: 89-89
120-124: Excellent documentation of session protection.The updated integration documentation clearly explains how the orchestrator prevents monitors from interfering with active sessions. The DND example is particularly helpful.
135-135: LGTM! Signature correctly updated.The
run_monitorssignature properly acceptsshared_stateand the parameter is well-documented.Also applies to: 203-207
src-tauri/src/scheduler/attention_timer.rs (2)
198-277: LGTM! Well-structured extraction with robust edge-case handling.The extraction of
calculate_next_attentionandget_next_attention_timeinto pure functions significantly improves testability. The implementation correctly handles DST transitions viaLocalResultvariants (lines 239-251) and appropriately returnsNonewith error logging (lines 272-276) instead of usingunreachable!(), addressing the past review comment.The algorithm is sound: it checks today's remaining times first, then iterates through the next 7 days to find the earliest matching occurrence.
279-702: Excellent comprehensive test coverage!The test suite is exceptionally well-structured with 20 test cases organized into three modules:
- Basic functionality tests for both free functions
- Multiple-attention scenarios
- Edge cases covering DST transitions, week wrapping, exact time matches, and boundary conditions
The tests leverage helper functions effectively and validate both success paths and
None-return cases. This level of coverage significantly increases confidence in the correctness of the scheduling logic.
Summary by CodeRabbit
Release Notes
Summary by CodeRabbit
New Features
Improvements
UI Updates
Tests