Repository navigation
Remove notifications when they no longer apply - #98
Merged
Merged
Conversation
Notifications module: - Add start: Date to .meetingStarting and write eventStart to userInfo - Add DeliveredNotification value type and deliveredNotifications() seam on NotificationCenterProviding - Add cancelMeetingStarting(eventKey:), cancelAllMeetingStarting(), cancelAdHocDetected(bundleID:), deliveredOfferNotifications() to NotificationService - Both test fakes now model delivery (track delivered list, replace on same identifier, filter on remove) AppCore wiring: - C1: handleCalendarTimerFired guards with shouldPostCalendarNotification (skips post when now >= start + 300s) and schedules an expiry timer that removes the notification after 5 minutes - C2: startRecording cancels all expiry tasks and calls cancelAllMeetingStarting to clear calendar notifications - D1: handleDetectionStopped (now async) calls cancelAdHocDetected(bundleID:) for the stopped app - L1: cleanUpStaleNotificationsOnLaunch queries delivered notifications, applies launchNotificationCleanupPlan (pure), removes stale ones immediately, and schedules expiry for fresh ones Deviation from spec: calendarNotificationLifetime constant lives in an extension (calendar-start timers) rather than the class body, because the class body was at the 250-line SwiftLint type_body_length limit. The constant is nonisolated static let, so this is semantically identical. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (20)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe change adds APIs to query and cancel delivered notifications. AppCore uses them to expire calendar meeting notifications and remove ad-hoc notifications at defined lifecycle events. Tests and specifications cover the notification service and AppCore cleanup behavior. ChangesNotification cleanup
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant AppCore
participant NotificationService
participant NotificationCenterProviding
AppCore->>NotificationService: Query delivered offer notifications
NotificationService->>NotificationCenterProviding: Query delivered notifications
NotificationCenterProviding-->>NotificationService: Return delivered notification snapshots
NotificationService-->>AppCore: Return parsed meeting and ad-hoc offers
AppCore->>NotificationService: Cancel expired meeting or ad-hoc notifications
AppCore->>AppCore: Schedule expiry for fresh meeting notifications
Merge Risk: ⚪ Minimal · up to The notification cleanup changes are mergeable after normal checks. Expiry scheduling supports sleep/wake behavior; manual hardware testing remains useful but does not establish a blocking defect. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change reduces stale notification exposure and keeps cleanup within this app’s notifications. No newly expanded access or verified security regression was established. Concurrent delivery and cancellation remain an uncertainty in the cleanup guarantee. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 22.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 71 functions across 15 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Biscotti posted calendar and "Meeting detected" notifications at
.timeSensitivepriority and never removed calendar ones. This PR removes each notification (on screen and in Notification Center) when it no longer applies.start + 5 min. The time is fixed at post time; calendar refreshes do not cancel it. If the Mac wakes afterstart + 5 min, the late notification is not posted..stoppedsignal: mic released for 8 s), in all run states.Notification priority is unchanged.
Spec:
specs/projects/notification_cleanup/Implementation notes
Notifications:.meetingStartingcarriesstart: Date(written touserInfo); newdeliveredNotifications()seam +DeliveredNotification; newcancelMeetingStarting(eventKey:),cancelAllMeetingStarting(),cancelAdHocDetected(bundleID:),deliveredOfferNotifications().AppCore: separate expiry-task map (not tied to calendar timer refresh), pureshouldPostCalendarNotificationandlaunchNotificationCleanupPlanhelpers.Test plan
make precommit-checks(format, lint, 2804 tests) greenNotificationsTests/NotificationCleanupTests.swift,AppCoreTests/NotificationCleanupTests.swift)🤖 Generated with Claude Code
Summary by CodeRabbit