Improve code quality: add validation, documentation, and better error handling#16
Improve code quality: add validation, documentation, and better error handling#16
Conversation
WalkthroughComprehensive extension of monitoring and notification system. Adds configuration validation, implements full CategoryAlert logic with threshold tracking, introduces background threads for hourly summaries and new-day detection, and enhances notification delivery with platform-specific support and category formatting with emoji icons. Changes
Sequence DiagramsequenceDiagram
participant Main as main()
participant Config as Config<br/>Validation
participant Service as start_service()
participant Threads as Background<br/>Threads
participant Check as Threshold/<br/>Monitoring
participant Notify as Notification<br/>System
Main->>Config: load_config()
Config->>Config: validate()
Config-->>Main: NotificationConfig
Main->>Service: start_service()
Service->>Service: has_any_monitoring_enabled()?
alt Disabled
Service->>Service: log warning
end
Service->>Threads: spawn start_hourly()
Service->>Threads: spawn start_new_day()
Service->>Threads: spawn start_server_monitor()
Service->>Check: spawn threshold_alerts()
par Background Operations
Threads->>Notify: send_checkin() / send_detailed_checkin()
Notify->>Notify: get_top_categories()
Notify-->>Threads: Result<()>
Check->>Check: loop every THRESHOLD_CHECK_INTERVAL_SECS
Check->>Check: evaluate thresholds
alt Threshold Triggered
Check->>Notify: notify()
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
Co-authored-by: 0xbrayo <62670517+0xbrayo@users.noreply.github.com>
Co-authored-by: 0xbrayo <62670517+0xbrayo@users.noreply.github.com>
Co-authored-by: 0xbrayo <62670517+0xbrayo@users.noreply.github.com>
Co-authored-by: 0xbrayo <62670517+0xbrayo@users.noreply.github.com>
|
@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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main.rs (2)
522-546: DST-unsafe local time construction can panicUsing Local.from_local_datetime(...).single().unwrap() can panic on DST transitions (ambiguous/nonexistent local times). Use earliest()/latest() fallback to be robust.
- let now = Local::now(); - let day_end = now.date_naive().and_hms_opt(0, 0, 0).unwrap(); - let mut day_end = Local.from_local_datetime(&day_end).single().unwrap(); - if day_end < now { - day_end += Duration::days(1); - } - let time_to_next_day = day_end - now + TIME_OFFSET; + let now = Local::now(); + let next_midnight_naive = (now + Duration::days(1)) + .date_naive() + .and_hms_opt(0, 0, 0) + .expect("midnight must be constructible"); + let next_midnight = Local + .from_local_datetime(&next_midnight_naive) + .earliest() + .or_else(|| Local.from_local_datetime(&next_midnight_naive).latest()) + .expect("resolve ambiguous/nonexistent local time"); + let time_to_next_day = next_midnight - now + TIME_OFFSET;
932-939: DST-unsafe next-hour calculationSame .single().unwrap() issue as above; use earliest()/latest() resolution.
- let next_hour = now + Duration::hours(1); - let next_hour = next_hour - .date_naive() - .and_hms_opt(next_hour.hour(), 0, 0) - .unwrap(); - let next_hour = Local.from_local_datetime(&next_hour).single().unwrap(); + let nh = now + Duration::hours(1); + let nh_naive = nh.date_naive().and_hms_opt(nh.hour(), 0, 0).unwrap(); + let next_hour = Local + .from_local_datetime(&nh_naive) + .earliest() + .or_else(|| Local.from_local_datetime(&nh_naive).latest()) + .unwrap();
🧹 Nitpick comments (10)
src/main.rs (10)
38-43: Configurable intervals and type safetyConsider making these intervals/timeouts configurable (config/CLI/env) and representing them as chrono::Duration (or a single newtype) to avoid mixing u64/i32/chrono::Duration/std::time::Duration across call sites.
76-94: Threshold validation ergonomicsCurrent bool return loses context. Prefer Result<()> with the first offending index/pair to improve error messages, or return Option<(idx_left, idx_right)>. Also decide if equal thresholds should be rejected (strictly increasing) or deduped/sorted.
Confirm the intended rule: strictly increasing vs non-decreasing?
140-176: Doc/behavior mismatch in has_any_monitoring_enabledDocs say “valid thresholds,” but code checks only non-empty. Recommend checking both non-empty and valid:
- .any(|alert| !alert.thresholds_minutes.is_empty()) + .any(|a| !a.thresholds_minutes.is_empty() && a.validate_thresholds())Also consider including 1‑based index in the error message for consistency with the warning.
324-327: DRY: hostname init helperSame hostname fallback logic appears 3x. Extract a small helper (e.g., fn init_hostname() -> String) and reuse.
Also applies to: 352-355, 381-384
547-566: Threshold notifications may be delayed; cap refresh intervalupdate() defers querying until predicted threshold time. If the prediction is based on stale time_spent, alerts can be late. Cap the defer with the loop tick to bound latency.
- if now.with_timezone(&Utc) > (self.last_check + time_to_threshold) { + let min_poll = Duration::seconds(THRESHOLD_CHECK_INTERVAL_SECS as i64); + if now.with_timezone(&Utc) >= self.last_check + time_to_threshold.min(min_poll) {
660-664: Status is tracked but never loggedIf you intended to log on change, add a debug line.
- if Some(&status) != alert.last_status.as_ref() { - alert.last_status = Some(status); - } + if Some(&status) != alert.last_status.as_ref() { + log::debug!("Threshold status: {}", status); + alert.last_status = Some(status); + }
687-720: TIME_CACHE can grow unboundedKeys include date+mode and are never evicted. Consider an LRU/TTL eviction (e.g., moka) or manual pruning to recent N entries to prevent unbounded memory growth during long runs or varied dates.
1032-1032: Avoid hard-coded “5 minutes” in logTie the message to NEW_DAY_CHECK_INTERVAL_SECS to keep logs consistent with config.
- log::debug!("New day thread sleeping for 5 minutes until next check"); + log::debug!( + "New day thread sleeping for {} minutes until next check", + NEW_DAY_CHECK_INTERVAL_SECS / 60 + );
1209-1229: Avoid spawning “which”You can try spawning terminal-notifier directly and interpret NotFound, or use which::which crate. Reduces extra process and shell dependency.
1265-1272: Implement Unicode unescape (optional)If inputs can contain “\uXXXX” sequences, consider implementing actual unescaping (e.g., via serde_json round-trip or a small parser) to improve output fidelity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main.rs(35 hunks)
⏰ 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). (10)
- GitHub Check: Lint and Format Check
- GitHub Check: Build (Linux x64)
- GitHub Check: Build (Linux ARM64)
- GitHub Check: Build (Windows)
- GitHub Check: Build (macOS)
- GitHub Check: Build (macOS)
- GitHub Check: Build (Windows)
- GitHub Check: Lint and Format Check
- GitHub Check: Build (Linux ARM64)
- GitHub Check: Build (Linux x64)
🔇 Additional comments (8)
src/main.rs (8)
18-18: Good alias to avoid name clash with atomic OrderingAvoids confusion between cmp::Ordering and atomic::Ordering.
268-270: Config validated before useNice guardrail; fail-fast on invalid configs.
304-312: Better error context on client init/connectClear, actionable messages (host:port + hint).
Also applies to: 315-323
399-403: Good UX when idlingWarning when all monitoring is disabled is helpful.
1278-1284: Graceful fallback for classesNice resilience: default_classes used on missing/unparseable settings and when client not initialized.
Also applies to: 1294-1315
1451-1451: Robust float sortpartial_cmp with Equal fallback avoids panic on NaN; good call.
1198-1199: No changes requiredThe
Notification::timeoutmethod accepts a generic type parameterT: Into<Timeout>, which means it works with both i32 values and explicitTimeoutenum variants. Your current code passingNOTIFICATION_TIMEOUT_MSdirectly is already correct and requires no modification.
1129-1149: The review comment is based on an incorrect assumption about the API behaviorThe web search confirms that the aw-server API returns events in descending order (newest-first) when using
limit=1. Specifically, the datastore'sget(limit=1)returns the most recent event first, not the oldest. Therefore, callingclient.get_events(..., Some(1))and accessingevents[0]correctly retrieves the latest event—the code is already correct as written and does not need the suggested change to uselast().Likely an incorrect or invalid review comment.
Refactored codebase to improve maintainability, robustness, and user experience while preserving all existing functionality.
Changes
Code Quality
Orderingtype conflictTHRESHOLD_CHECK_INTERVAL_SECS,AFK_TIMEOUT_MINUTES, etc.)continuestatements in monitoring loopsmap_or_else()instead ofmap().unwrap_or_else()Documentation
Configuration Validation
Error Messages
Edge Cases
validate_thresholds()explicitly handles empty and single-element arrayshas_any_monitoring_enabled()checks for alerts with non-empty thresholds, not just presence of alertsget_server_classes()with fallback to defaultsStats: 214 insertions, 29 deletions across 1 file
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
New Features
Bug Fixes