Meeting Auto-Detection #1018
Meeting Auto-Detection #1018
Conversation
📝 WalkthroughWalkthroughThis update introduces a meeting detection and auto-recording system across the application and notification plugin. It adds configuration options for auto-start recording, integrates a multi-signal meeting detector, exposes new API commands for managing auto-recording settings, updates UI and schema, and extends notification logic to leverage meeting confidence scores. Changes
Sequence Diagram(s)Auto-Start Recording Configuration FlowsequenceDiagram
participant User
participant DesktopUI
participant NotificationPlugin
participant MeetingDetector
User->>DesktopUI: Toggle "Auto-start recording" switch
DesktopUI->>NotificationPlugin: set_auto_record_enabled(enabled)
NotificationPlugin->>MeetingDetector: set_auto_record_config(enabled, threshold)
MeetingDetector-->>NotificationPlugin: Ack
NotificationPlugin-->>DesktopUI: Success/Failure
Meeting Detection and Auto-Recording TriggersequenceDiagram
participant App
participant MeetingDetector
participant NotificationPlugin
App->>MeetingDetector: process_signal(MeetingSignal)
MeetingDetector->>MeetingDetector: Store signal, correlate, calculate confidence
alt Confidence >= threshold and auto-record enabled
MeetingDetector->>NotificationPlugin: Trigger auto-record notification
else Confidence < threshold or auto-record disabled
MeetingDetector->>NotificationPlugin: Show manual prompt or low confidence notification
end
Notification Worker Event CorrelationsequenceDiagram
participant Worker
participant MeetingDetector
Worker->>MeetingDetector: calculate_meeting_scores(events)
MeetingDetector-->>Worker: [MeetingScore]
loop For each high-confidence event
Worker->>MeetingDetector: process_signal(CalendarEvent)
MeetingDetector-->>Worker: Enhanced MeetingScore
Worker->>Worker: Log confidence, trigger notifications if needed
end
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)error: failed to load source for dependency Caused by: Caused by: Caused by: 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ 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). (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/docs/data/i18n.json (1)
4-4: Note: Korean translations missing for new entries.The total count correctly reflects the addition of 2 new translation entries. However, the Korean missing count indicates these new entries haven't been translated yet.
Would you like me to help identify which specific translation keys need Korean translations or create an issue to track this localization task?
Also applies to: 9-9
plugins/notification/src/meeting_detection.rs (2)
152-168: Consider handling potential timestamp collisions.Using timestamp seconds as the key could lead to signal loss if multiple signals arrive within the same second. The current implementation already uses a Vec for values, which handles this correctly, but it's worth documenting this behavior.
Add a comment explaining that multiple signals within the same second are grouped together:
let time_key = now.timestamp().to_string(); + // Multiple signals within the same second are grouped together
354-359: Clarify the time difference calculation.The time difference calculation
(now - event_start)produces negative values for future events, which might be counterintuitive. Consider adding a comment to clarify this.// Check if event is within reasonable time window (15 minutes before to 5 minutes after) let time_diff = (now - event_start).num_minutes(); + // time_diff is negative for future events, positive for past events if time_diff < -15 || time_diff > 5 { continue; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
apps/desktop/src/components/settings/views/general.tsx(5 hunks)apps/desktop/src/locales/en/messages.po(9 hunks)apps/desktop/src/locales/ko/messages.po(9 hunks)apps/docs/data/i18n.json(1 hunks)crates/db-user/src/config_types.rs(2 hunks)plugins/db/js/bindings.gen.ts(1 hunks)plugins/listener/src/lib.rs(2 hunks)plugins/notification/Cargo.toml(2 hunks)plugins/notification/src/commands.rs(1 hunks)plugins/notification/src/ext.rs(5 hunks)plugins/notification/src/lib.rs(6 hunks)plugins/notification/src/meeting_detection.rs(1 hunks)plugins/notification/src/store.rs(1 hunks)plugins/notification/src/worker.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,rs}`: 1. No error handling. 2. No unused imports, variables, or functions. 3. For comments, keep it minimal. It should be about "Why", not "What".
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
plugins/notification/src/store.rsapps/desktop/src/components/settings/views/general.tsxplugins/listener/src/lib.rscrates/db-user/src/config_types.rsplugins/db/js/bindings.gen.tsplugins/notification/src/commands.rsplugins/notification/src/worker.rsplugins/notification/src/ext.rsplugins/notification/src/lib.rsplugins/notification/src/meeting_detection.rs
🧬 Code Graph Analysis (2)
apps/desktop/src/components/settings/views/general.tsx (2)
packages/ui/src/components/ui/form.tsx (5)
FormField(158-158)FormItem(158-158)FormLabel(158-158)FormDescription(158-158)FormControl(158-158)packages/ui/src/components/ui/switch.tsx (1)
Switch(57-57)
plugins/notification/src/commands.rs (1)
plugins/notification/src/ext.rs (8)
get_auto_record_enabled(22-22)get_auto_record_enabled(197-203)set_auto_record_enabled(23-23)set_auto_record_enabled(206-211)get_auto_record_threshold(25-25)get_auto_record_threshold(214-220)set_auto_record_threshold(26-26)set_auto_record_threshold(223-228)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci
🔇 Additional comments (25)
plugins/notification/Cargo.toml (1)
23-23: LGTM! Dependencies align with meeting detection features.The addition of
tauri-plugin-listener,regex, anduuiddependencies supports the new meeting detection and auto-recording functionality described in the PR objectives.Also applies to: 40-41
plugins/notification/src/store.rs (1)
7-8: LGTM! Store keys properly defined for auto-recording configuration.The new
AutoRecordEnabledandAutoRecordThresholdenum variants follow the existing naming convention and will enable persistent storage of auto-recording settings.crates/db-user/src/config_types.rs (1)
47-47: LGTM! Auto-start recording configuration properly implemented.The optional boolean field with a default value of
Some(false)provides good backward compatibility and follows the established pattern for similar configuration options.Also applies to: 59-59
plugins/listener/src/lib.rs (1)
16-16: LGTM: Public API exposure for plugin integration.The visibility changes to expose
StateEventand thefsmfield enable external plugin integration for meeting detection functionality. While this breaks encapsulation by exposing internal state machine details, it appears to be a deliberate design choice for inter-plugin communication.Also applies to: 25-25
apps/desktop/src/locales/en/messages.po (1)
374-380: LGTM: Proper localization for new auto-recording feature.The new translation entries correctly support the auto-start recording UI with appropriate descriptive text.
apps/desktop/src/components/settings/views/general.tsx (3)
77-77: LGTM: Proper schema definition for new setting.The
autoStartRecordingfield is correctly added as an optional boolean, following the pattern of other optional settings in the schema.
102-102: LGTM: Consistent configuration handling.The implementation properly handles default values, config reset, and mutation mapping for the new
autoStartRecordingsetting, following the established patterns for other configuration fields.Also applies to: 114-114, 132-132
196-220: LGTM: Well-structured UI component for auto-recording toggle.The FormField implementation follows the existing pattern with proper form control binding, localized labels, and consistent styling. The Switch component usage matches other boolean settings in the form.
plugins/notification/src/commands.rs (1)
102-136: LGTM: Well-implemented Tauri commands for auto-recording configuration.The new commands follow the established pattern in the file with proper:
- Tauri command and specta annotations
- Error handling with string conversion
- Delegation to extension trait methods
- Appropriate parameter types (bool for enabled flag, f64 for threshold)
The implementation is consistent with existing commands and follows Tauri best practices.
plugins/db/js/bindings.gen.ts (2)
218-218: LGTM: Proper type definition for new configuration field.The
auto_start_recording: boolean | nullfield correctly reflects the optional boolean configuration added to the backendConfigGeneralstruct.
8-173: LGTM: Improved formatting for generated bindings.The formatting changes improve readability by adding consistent line breaks and indentation to the commands object and type declarations. Since this is a generated file, these changes likely reflect improvements in the code generator.
Also applies to: 181-362
plugins/notification/src/ext.rs (4)
1-1: LGTM!The
Arcimport is correctly added to support the new shared configuration usage.
22-27: LGTM!The new trait methods for auto-recording configuration follow the established pattern and provide a clean API.
87-97: LGTM!The integration of meeting detector and configuration into the worker state is properly implemented with appropriate use of
Arcfor shared ownership.
196-228: LGTM!The auto-record getter/setter implementations properly follow the established pattern with appropriate error handling and default values.
plugins/notification/src/worker.rs (2)
6-30: LGTM!The configuration struct with environment variable support provides good flexibility for deployment configurations while maintaining sensible defaults.
40-41: LGTM!The worker state correctly includes the shared configuration and meeting detector for the enhanced functionality.
plugins/notification/src/lib.rs (4)
7-7: LGTM!The meeting detection module and state field are properly integrated into the plugin structure.
Also applies to: 23-23
26-27: Verify the removal of generic parameters is intentional.The change from generic runtime parameters to hardcoded
tauri::Wryremoves flexibility and could be a breaking change if consumers were using different runtimes.Also applies to: 49-49
41-44: LGTM!The new commands properly expose the auto-record configuration functionality.
115-121: LGTM!The simplified test appropriately validates the basic meeting detector functionality.
plugins/notification/src/meeting_detection.rs (4)
9-46: LGTM!The data structures are well-designed with appropriate use of enums for type safety and Arc<Mutex<>> for thread-safe state management.
428-472: Tests will need updating if error handling is changed.The test coverage is comprehensive. Note that if the assertion-based validation is changed to Result-based error handling as suggested earlier, these panic tests will need to be updated to test for error returns instead.
319-340: start_session method verified on AppHandleThe
ListenerPluginExttrait inplugins/listener/src/ext.rsdeclaresfn start_session(&self, id: impl Into<String>) -> impl Future<Output = ()>, and it’s implemented by the async function inplugins/listener/src/commands.rs. The call toapp_handle_clone.start_session(session_id).awaitis valid.
94-126: Replace assertions with proper error handling.Using
assert!for input validation is problematic as assertions can be disabled in release builds and cause panics. Consider returning a Result instead.- pub fn set_auto_record_config(&self, enabled: bool, threshold: f64) { + pub fn set_auto_record_config(&self, enabled: bool, threshold: f64) -> Result<(), String> { // Validate threshold is within acceptable range - assert!( - threshold.is_finite(), - "Invalid threshold: {} (must be a finite number)", - threshold - ); - assert!( - (0.0..=1.0).contains(&threshold), - "Invalid threshold: {} (must be between 0.0 and 1.0)", - threshold - ); + if !threshold.is_finite() { + return Err(format!("Invalid threshold: {} (must be a finite number)", threshold)); + } + if !(0.0..=1.0).contains(&threshold) { + return Err(format!("Invalid threshold: {} (must be between 0.0 and 1.0)", threshold)); + } // Acquire both locks and update values let mut auto_enabled = self .auto_record_enabled .lock() - .expect("Failed to acquire lock for auto_record_enabled"); + .map_err(|_| "Failed to acquire lock for auto_record_enabled")?; let mut auto_threshold = self .auto_record_threshold .lock() - .expect("Failed to acquire lock for auto_record_threshold"); + .map_err(|_| "Failed to acquire lock for auto_record_threshold")?; // Update both values *auto_enabled = enabled; *auto_threshold = threshold; tracing::debug!( "auto_record_config_updated: enabled={}, threshold={:.2}", enabled, threshold ); + Ok(()) }Likely an incorrect or invalid review comment.
| // Defer initialization that depends on the plugin being fully set up | ||
| let app_handle = app.clone(); | ||
| tauri::async_runtime::spawn(async move { | ||
| // Small delay to ensure plugin is fully initialized | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; | ||
|
|
||
| // Configure the meeting detector with the app handle | ||
| if let Some(state) = app_handle.try_state::<SharedState>() { | ||
| let state_guard = state.lock().unwrap(); | ||
| state_guard | ||
| .meeting_detector | ||
| .set_app_handle(app_handle.clone()); | ||
|
|
||
| // Set initial auto-record configuration from stored settings | ||
| let auto_enabled = app_handle.get_auto_record_enabled().unwrap_or(false); | ||
| let auto_threshold = app_handle.get_auto_record_threshold().unwrap_or(0.7); | ||
| state_guard | ||
| .meeting_detector | ||
| .set_auto_record_config(auto_enabled, auto_threshold); | ||
| } | ||
|
|
||
| if app_handle.get_detect_notification().unwrap_or(false) { | ||
| if let Err(e) = app_handle.start_detect_notification() { | ||
| tracing::error!("start_detect_notification_failed: {:?}", e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if app.get_event_notification().unwrap_or(false) { | ||
| let app_handle = app.clone(); | ||
| tauri::async_runtime::spawn(async move { | ||
| if app_handle.get_event_notification().unwrap_or(false) { | ||
| if let Err(e) = app_handle.start_event_notification().await { | ||
| tracing::error!("start_event_notification_failed: {:?}", e); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more robust initialization approach.
The 100ms delay for initialization is fragile and could lead to race conditions. Consider using a proper initialization event or state check instead of a time-based delay.
Consider alternatives such as:
- Using Tauri's app lifecycle events
- Implementing a ready state check
- Using a proper initialization callback
The current approach might fail if the plugin takes longer than 100ms to initialize on slower systems.
🤖 Prompt for AI Agents
In plugins/notification/src/lib.rs between lines 58 and 90, replace the fixed
100ms sleep delay used for deferred initialization with a more reliable
mechanism such as listening to Tauri's app lifecycle events, implementing a
ready state check, or using an initialization callback to ensure the plugin is
fully set up before proceeding. This avoids race conditions and ensures
initialization only continues once the plugin is truly ready.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/notification/src/meeting_detection.rs (2)
95-119: Optimize the cleanup algorithm to avoid unnecessary iterations.The current cleanup implementation iterates through all signals to count valid ones, which is inefficient for large buffers.
- fn cleanup_old_signals(&mut self, max_age: Duration) { - let cutoff = Utc::now() - max_age; - let mut new_size = 0; - - // Count valid signals (more recent than cutoff) - for i in 0..self.size { - let idx = if self.size == self.capacity { - (self.head + i) % self.capacity - } else { - i - }; - - if let Some(ref entry) = self.buffer[idx] { - if entry.timestamp >= cutoff { - new_size += 1; - } else { - // Clear old entries - self.buffer[idx] = None; - } - } - } - - self.size = new_size; - } + fn cleanup_old_signals(&mut self, max_age: Duration) { + let cutoff = Utc::now() - max_age; + + // Use retain-like logic for better performance + let mut write_pos = 0; + for read_pos in 0..self.size { + let idx = if self.size == self.capacity { + (self.head + read_pos) % self.capacity + } else { + read_pos + }; + + if let Some(ref entry) = self.buffer[idx] { + if entry.timestamp >= cutoff { + if write_pos != read_pos { + let new_idx = if self.size == self.capacity { + (self.head + write_pos) % self.capacity + } else { + write_pos + }; + self.buffer[new_idx] = self.buffer[idx].take(); + } + write_pos += 1; + } else { + self.buffer[idx] = None; + } + } + } + self.size = write_pos; + }
262-347: Consider breaking down the complex scoring algorithm into smaller functions.The
calculate_enhanced_scoremethod is quite long and handles multiple concerns. Breaking it into smaller, focused functions would improve readability and testability.- fn calculate_enhanced_score(&self, current_signal: &MeetingSignal) -> MeetingScore { - let base_confidence = self.get_base_confidence(current_signal); - - // Get recent signals for correlation analysis - let recent_signals = self.get_recent_signals(Duration::minutes(5)); - - // Calculate correlation bonuses - let mut correlation_bonus = 0.0; - let mut total_signals = vec![current_signal.clone()]; - - // Check for signal correlation patterns - for signal in &recent_signals { - total_signals.push(signal.clone()); - - // Correlation bonuses for complementary signals - match (current_signal, signal) { - // ... pattern matching logic ... - } - } - - // Apply temporal proximity bonus for calendar events - let temporal_bonus = match current_signal { - // ... temporal bonus logic ... - }; - - // Calculate final confidence (capped at 1.0) - let final_confidence = (base_confidence + correlation_bonus + temporal_bonus).min(1.0); - - // Determine meeting type based on signal composition - let meeting_type = self.determine_meeting_type(&total_signals); - - MeetingScore { - confidence: final_confidence, - signals: total_signals, - event_id: self.extract_event_id(current_signal), - meeting_type, - } - } + fn calculate_enhanced_score(&self, current_signal: &MeetingSignal) -> MeetingScore { + let base_confidence = self.get_base_confidence(current_signal); + let recent_signals = self.get_recent_signals(Duration::minutes(5)); + let mut total_signals = vec![current_signal.clone()]; + total_signals.extend(recent_signals.clone()); + + let correlation_bonus = self.calculate_correlation_bonus(current_signal, &recent_signals); + let temporal_bonus = self.calculate_temporal_bonus(current_signal); + let final_confidence = (base_confidence + correlation_bonus + temporal_bonus).min(1.0); + let meeting_type = self.determine_meeting_type(&total_signals); + + MeetingScore { + confidence: final_confidence, + signals: total_signals, + event_id: self.extract_event_id(current_signal), + meeting_type, + } + } + + fn calculate_correlation_bonus(&self, current_signal: &MeetingSignal, recent_signals: &[MeetingSignal]) -> f64 { + // Move correlation logic here + } + + fn calculate_temporal_bonus(&self, current_signal: &MeetingSignal) -> f64 { + // Move temporal bonus logic here + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/notification/src/ext.rs(5 hunks)plugins/notification/src/meeting_detection.rs(1 hunks)plugins/notification/src/worker.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/notification/src/worker.rs
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,rs}`: 1. No error handling. 2. No unused imports, variables, or functions. 3. For comments, keep it minimal. It should be about "Why", not "What".
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
plugins/notification/src/ext.rsplugins/notification/src/meeting_detection.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci
🔇 Additional comments (6)
plugins/notification/src/ext.rs (3)
22-27: LGTM! Clean trait extension for auto-record configuration.The new trait methods follow the established pattern and provide the necessary interface for auto-record configuration management.
87-98: LGTM! Proper integration of meeting detector into worker state.The changes correctly clone the meeting detector and pass it to the worker with the notification config, enabling meeting detection integration.
203-236: LGTM! Consistent implementation of auto-record configuration methods.The implementation follows the established pattern for configuration management using the notification store with proper error mapping.
plugins/notification/src/meeting_detection.rs (3)
421-442: LGTM! Proper separation of concerns using plugin extension trait.The auto-recording trigger correctly uses the listener plugin's extension trait rather than accessing internal state directly, maintaining good architectural boundaries.
540-559: Panic-based tests remain accurateThe
set_auto_record_configmethod still usesassert!to validate the threshold (without returning aResult), so it will continue to panic on invalid values. The existing#[should_panic(expected = "Invalid threshold")]tests align with the current implementation and do not need updating.Likely an incorrect or invalid review comment.
191-223: Replace panicking assertions with proper error handling.Using
assert!in production code can cause crashes. The validation should returnResulttypes for better error handling.- pub fn set_auto_record_config(&self, enabled: bool, threshold: f64) { - // Validate threshold is within acceptable range - assert!( - threshold.is_finite(), - "Invalid threshold: {} (must be a finite number)", - threshold - ); - assert!( - (0.0..=1.0).contains(&threshold), - "Invalid threshold: {} (must be between 0.0 and 1.0)", - threshold - ); + pub fn set_auto_record_config(&self, enabled: bool, threshold: f64) -> Result<(), String> { + // Validate threshold is within acceptable range + if !threshold.is_finite() { + return Err(format!("Invalid threshold: {} (must be a finite number)", threshold)); + } + if !(0.0..=1.0).contains(&threshold) { + return Err(format!("Invalid threshold: {} (must be between 0.0 and 1.0)", threshold)); + } + + // Acquire both locks and update values + let mut auto_enabled = self + .auto_record_enabled + .lock() + .map_err(|_| "Failed to acquire lock for auto_record_enabled")?; + let mut auto_threshold = self + .auto_record_threshold + .lock() + .map_err(|_| "Failed to acquire lock for auto_record_threshold")?; + + // Update both values + *auto_enabled = enabled; + *auto_threshold = threshold; + + tracing::debug!( + "auto_record_config_updated: enabled={}, threshold={:.2}", + enabled, + threshold + ); + Ok(()) + }Likely an incorrect or invalid review comment.
No description provided.