feat: stagger approval email behind push by default (#119)

[d-mo]

Summary

• Push goes out at T0; email for dual-channel users with stagger_email (default true) waits 60s and only sends if the approval is still pending (fresh DB session + CRUD re-check at fire time).
• Email-only users still get email immediately; push-unavailable / raise / no_devices falls back to immediate email so stagger never degrades delivery.
• Per-user stagger_email on NotificationPreferences (conductor decision a) with preferences UI toggle when both channels are enabled; deletes dormant NotificationService and its tests (conductor decision b).

Fixes #119

Conductor decisions

1. Toggle is per-user (stagger_email on NotificationPreferences, default true), not account-wide.
2. Dormant NotificationService is deleted in this PR; its partition/delay pattern is ported into the live ApprovalService.send_notifications path.

Behavior

Case	Push	Email
Both channels, stagger on	T0	T+60s if still pending
Both channels, stagger off	T0	T0
Email only	-	T0
Push only	T0	never
Push unconfigured / raises / no devices	fail/empty	T0 for all email-enabled
Mute bypass	suppressed	suppressed (no delayed task)
Escalation	T0	T0 (unchanged)

Scheduling is in-process asyncio.create_task + sleep with a module-level task set (no new infra). Slack/mattermost/webhook and escalations are out of scope / unchanged.

Migration note

Alembic revision 20260801_stagger_email parents on current main head (20260730_webauthn_credentials). Open PRs #137, #141, and possibly #136 also add migrations off the same head; this PR may need a re-parent after those land.

Test plan

• Backend: TestNotificationStagger (17 cases covering the spec list)
• Backend neighbors: TestSendNotifications, mute path, push suite, notification-preferences endpoints
• Frontend: quiet-alerts checkbox visibility + PUT persistence
• ruff check/format on touched code
• CI green
• Manual: dual-channel user gets push now, email only if still pending after ~60s; toggle off restores both immediate

Made with Cursor

Note

Medium Risk
4 non-blocking MEDIUM/LOW issues persist from the last review (second pass: no code changes since initial review). The implementation remains well-structured with excellent test coverage.

Overview
Adds 60s email stagger behind push notifications for dual-channel users, controlled by a per-user stagger_email preference toggle. Deletes dormant NotificationService and ports its delay pattern into ApprovalService. Scheduling uses in-process asyncio.create_task with no new infrastructure.

^{Written by Preloop PR Reviewer for commit feat/notification-stagger-119. Updates automatically on new commits.}

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[preloop-staging]

💬 Preloop has suggestions for this PR.

The stagger implementation is well-structured overall — the partition logic is clean, the behavior spec is comprehensive, and the 17 test cases cover the decision matrix thoroughly. A few improvements are recommended below.

See the summary comment for details.

[preloop-staging]

🔍 Preloop Code Review

Last Updated: 2026-08-01
Reviewing Commit: feat/notification-stagger-119
Review Status: 💬 Changes Suggested

---

📝 Summary

The stagger implementation continues to be well-structured — the partition logic is clean and the behavior spec is comprehensive. The 17 TestNotificationStagger cases cover the decision matrix thoroughly. All 4 issues from the initial review persist unchanged; none are blocking but they represent genuine code quality and resource concerns worth addressing before merge.

✅ What Looks Good

• Clean partition logic separating users into push/email/stagger/immediate buckets
• Fresh DB session after the 60s delay ensures accurate re-check of approval state
• Comprehensive test coverage of all spec behavior cases
• Proper fallback to immediate email when push raises or has no devices
• Escalation path explicitly verified to be unaffected
• Alembic migration is clean and properly parented
• Deletion of dormant NotificationService simplifies the notification path

---

⚠️ Issues Found

🟡 Medium Priority

• [Code Quality]: Redundant status check in _send_delayed_approval_email — the != "pending" condition on line 274 is subsumed by the _TERMINAL_STATUSES membership check on line 273. Simplify to if not current_request or current_request.status != "pending". — approval_service.py:271-274

• [Resource]: _delayed_email_tasks set grows unbounded — under high approval throughput, tasks created faster than they complete (60s sleep) could accumulate without bound. Consider adding a size guard (e.g., max 500 pending tasks) with cleanup logging. — approval_service.py:227

• [Code Quality]: Heavy duplication in preference toggle methods — handleChannelChange, handleToggleEmail, handleToggleMobilePush, and handleToggleStaggerEmail share an identical try/catch/finally pattern (~100 lines). Extract a generic updatePreference(field, value, label) helper. — notification-preferences-view.ts:612-718

🟢 Low Priority

• [Resource]: onStateChange listener (line 405) is never unregistered in disconnectedCallback. Store the unsubscribe handle and clean up on disconnect to avoid stale callbacks accumulating on component recreation. — notification-preferences-view.ts:405

---

📄 Documentation Impact

No documentation updates are strictly required — the stagger behavior is documented in the PR description and the schema stagger_email field has a clear comment. Consider adding the behavior matrix to ARCHITECTURE.md if the notification flow section exists there.

---

Progress: 0 of 4 issues addressed

This summary updates automatically on each review. Inline comments provide detailed feedback on specific lines.

[preloop-staging]

💬 Preloop has suggestions for this PR.

Some improvements are recommended. See the summary comment below for details.

[preloop-staging]

Preloop has reviewed this PR.

See the summary comment below for the full review details and issue tracker.

This re-review confirms the previous MEDIUM finding (redundant status check) persists. One new MEDIUM finding on the unbounded task set under load. Otherwise the implementation remains solid with excellent test coverage.

[preloop-staging]

💬 Preloop has suggestions for this PR.

All 4 non-blocking MEDIUM/LOW issues from the initial review persist unchanged. The implementation remains well-structured with excellent test coverage — these are code quality and resource cleanup improvements worth addressing before merge.

See the summary comment below for the full issue tracker.