Dismiss tapped notifications from Notification Center#371
Conversation
📝 WalkthroughWalkthroughAdds delivered-notification removal to the push notification dependency and invokes it from ChangesDelivered notification removal
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant PushNotificationsClient
participant UNUserNotificationCenter
AppDelegate->>PushNotificationsClient: removeDeliveredNotification(identifier)
PushNotificationsClient->>UNUserNotificationCenter: removeDeliveredNotifications(withIdentifiers:)
AppDelegate->>AppDelegate: continue notification routing
AppDelegate->>AppDelegate: invoke completion handler
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Greptile SummaryThis PR dismisses tapped push notifications from Notification Center. The main changes are:
Confidence Score: 4/5The notification tap path needs a fix before merging.
PlayolaRadio/PlayolaRadioApp.swift Important Files Changed
Reviews (1): Last reviewed commit: "Dismiss delivered notification after tap" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| userInfo: [String: any Sendable], | ||
| completionHandler: @escaping () -> Void | ||
| ) { | ||
| pushNotifications.removeDeliveredNotification(identifier) |
There was a problem hiding this comment.
When a tapped notification has a malformed giveaway payload, a missing stationId, or a station that is not yet loaded in stationLists, this line removes it before routing can produce any in-app effect. Those handlers can then return silently, so the user sees the notification disappear with no winner sheet or station playback and no notification left to retry.
Knowledge Base Used: Push Notifications
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PlayolaRadio/Core/PushNotifications/PushNotificationsTests.swift`:
- Around line 120-142: Update
testHandleNotificationResponseRemovesDeliveredNotification to record calls made
through the injected pushNotifications.handleNotificationTap dependency,
including the provided userInfo payload. Assert that the tap handler is called
with the expected payload alongside the existing removal and completion
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc3a7ae5-69a4-4648-a65c-6bb4a8c47b5d
📒 Files selected for processing (3)
PlayolaRadio/Core/PushNotifications/PushNotifications.swiftPlayolaRadio/Core/PushNotifications/PushNotificationsTests.swiftPlayolaRadio/PlayolaRadioApp.swift
| @Test | ||
| func testHandleNotificationResponseRemovesDeliveredNotification() { | ||
| let removedIdentifiers = LockIsolated<[String]>([]) | ||
| let completionCalled = LockIsolated(false) | ||
|
|
||
| withDependencies { | ||
| $0.pushNotifications.removeDeliveredNotification = { identifier in | ||
| removedIdentifiers.withValue { $0.append(identifier) } | ||
| } | ||
| $0.pushNotifications.handleNotificationTap = { _ in } | ||
| } operation: { | ||
| let appDelegate = AppDelegate() | ||
| appDelegate.handleNotificationResponse( | ||
| identifier: "notification-123", | ||
| userInfo: [:] | ||
| ) { | ||
| completionCalled.setValue(true) | ||
| } | ||
| } | ||
|
|
||
| expectNoDifference(removedIdentifiers.value, ["notification-123"]) | ||
| #expect(completionCalled.value) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that notification responses are still routed.
handleNotificationTap is stubbed with a no-op, so this test passes even if handleNotificationResponse removes the notification but never routes the payload. Record and assert the injected tap call (including its payload) so this regression test covers the preserved routing behavior.
As per coding guidelines, add regression tests for bug fixes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PlayolaRadio/Core/PushNotifications/PushNotificationsTests.swift` around
lines 120 - 142, Update
testHandleNotificationResponseRemovesDeliveredNotification to record calls made
through the injected pushNotifications.handleNotificationTap dependency,
including the provided userInfo payload. Assert that the tap handler is called
with the expected payload alongside the existing removal and completion
assertions.
Source: Coding guidelines
1 participant
Summary
PushNotificationsClientand extract response handling into a testable helper.Testing
PushNotificationsTests: 21 tests passed; repository-wide Swift format check passed.Summary by CodeRabbit
New Features
Bug Fixes