-
Notifications
You must be signed in to change notification settings - Fork 537
feat: granular notification controls #3898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Mic detection: configurable delay (0=instant, N secs=prolonged) - Event notifications: configurable notify-before-minutes and timeout - Both mic and event: configurable auto-dismiss timeout - Unify MicStarted/MicProlongedUsage into MicDetected with duration_secs - DnD symmetry: both immediate and delayed paths respect policy at emit - Cooldown for immediate path (1hr, same as delayed) - isFocused guard for both paths - Remove 2000ms debounce (Rust cooldown handles dedup) - Migrate in_meeting_reminder -> mic_detection_delay_secs (180 when true) Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for hyprnote-storybook canceled.
|
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| }; | ||
|
|
||
| if claimed { | ||
| on_timer_fired(&env, &app, duration_secs); | ||
| let is_dnd = env.is_do_not_disturb(); | ||
| let policy_result = { | ||
| let guard = state.lock().unwrap_or_else(|e| e.into_inner()); | ||
| let ctx = PolicyContext { | ||
| apps: &[app], | ||
| is_dnd, | ||
| event_type: MicEventType::Started, | ||
| }; | ||
| guard.policy.evaluate(&ctx) | ||
| }; | ||
|
|
||
| match policy_result { | ||
| Ok(result) => on_timer_fired(&env, &result, duration_secs), | ||
| Err(reason) => tracing::info!(?reason, "skip_delayed_notification"), | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Delayed mic detection sets cooldown before DnD check, suppressing future notifications for 1 hour
When the delayed mic detection timer fires, claim() is called first (which unconditionally sets a 1-hour cooldown), and only then is the DnD policy evaluated. If DnD is active at timer-fire time, the notification is silently dropped but the cooldown remains, preventing any re-notification for that app for a full hour — even after DnD is turned off.
Root Cause
In plugins/detect/src/mic_usage_tracker.rs:117-137, the spawn_timer function calls claim() at line 119 before evaluating the policy at line 124-131. The claim() method (plugins/detect/src/mic_usage_tracker.rs:77-87) unconditionally inserts a cooldown entry:
pub fn claim(&mut self, app_id: &str, generation: u64) -> bool {
match self.timers.get(app_id) {
Some(entry) if entry.generation == generation => {
self.timers.remove(app_id);
self.cooldowns
.insert(app_id.to_string(), tokio::time::Instant::now());
true
}
_ => false,
}
}Then if guard.policy.evaluate(&ctx) returns Err(SkipReason::DoNotDisturb), no notification is emitted but the cooldown is already set. Compare this with the zero-delay path in plugins/detect/src/handler.rs:64-99, which correctly evaluates policy first and only sets cooldown after a successful policy check (lines 85-86).
Impact: Users with DnD enabled temporarily will miss notifications AND be locked out of re-notification for that app for 1 hour after DnD is disabled. This defeats the purpose of the DnD-at-emit-time design described in the PR.
(Refers to lines 117-137)
Prompt for agents
In plugins/detect/src/mic_usage_tracker.rs, in the spawn_timer function (lines 117-137), the cooldown is set by claim() before the DnD policy check. The fix should evaluate the policy BEFORE calling claim(), or alternatively, only set the cooldown after a successful policy evaluation. One approach:
1. After the sleep completes, first check the policy (DnD, ignored apps, etc.) without claiming.
2. If the policy says to skip, remove the timer entry WITHOUT setting a cooldown (or just leave it to expire naturally).
3. Only if the policy passes, call claim() to set the cooldown and emit the notification.
This requires either splitting claim() into separate remove-timer and set-cooldown steps, or adding a new method that removes the timer entry without setting cooldown. The zero-delay path in handler.rs:64-99 already does this correctly and can serve as a reference pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
| onChange={(e) => | ||
| subField.handleChange(Number(e.target.value)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Input validation issue: When user clears the input field, Number(e.target.value) returns 0 for empty string, which violates the min={1} constraint and bypasses HTML5 validation since the onChange fires immediately. This will set event_notify_before_minutes to 0, causing notifications to only trigger for events starting at the exact current moment (0ms window in event-notification/index.ts line 76).
value={subField.state.value}
onChange={(e) => {
const val = Number(e.target.value);
if (!isNaN(val) && val >= 1) {
subField.handleChange(val);
}
}}| onChange={(e) => | |
| subField.handleChange(Number(e.target.value)) | |
| } | |
| onChange={(e) => { | |
| const val = Number(e.target.value); | |
| if (!isNaN(val) && val >= 1) { | |
| subField.handleChange(val); | |
| } | |
| }} |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Summary
Configurable notification behavior for both event-based and mic detection-based notifications.
Event notifications
Mic detection notifications
Behavioral harmonization
MicStarted/MicProlongedUsageintoMicDetectedwithduration_secsMigration
in_meeting_reminder: true→mic_detection_delay_secs: 180Made with Cursor