Fix notification dismissal for Leave/Hang Up notification actions#1732
Merged
rahul-lohra merged 17 commits intoJun 23, 2026
Conversation
…ndering normal notification instead of CallStyle notification
…l-disabled-regression' into bugfix/rahullohra/battery-restrictions
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
rahul-lohra
changed the title
rahul-lohra
changed the title
Contributor
WalkthroughDeprecates ChangesNotification ID migration away from intent extras
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandler.kt (1)
206-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnhance TODO comments with specific NotificationType guidance to prevent ID mismatches during migration.
The four TODO markers appropriately flag locations where
callId.hashCode()should be replaced withStreamCallId.getNotificationId(NotificationType). However, the placeholder "appropriateType" is vague and may lead to incorrect NotificationType choices during implementation, which would re-introduce ID mismatches between handler posting and downstream dismissal.Per the migration documented in Context Snippet 4 (
AbstractNotificationActivitydismisses withgetNotificationId(NotificationType.Incoming)), the NotificationType selection is critical. Different notification scenarios (live, incoming, ongoing, outgoing, livestream) may use different types, and misalignment will causeNotificationManager.cancel(wrongId)failures.Consider refining each TODO to specify the likely NotificationType:
- Line 206 (
onLiveCall): Determine whetherLiveCallor another type applies.- Line 257 (
onNotification): Clarify which type this generic path should use.- Line 750 (
getSimpleOngoingCallNotification): LikelyOngoing, but confirm whether separate types exist for outgoing vs. ongoing.- Line 1186 (
getMinimalMediaStyleNotification): LikelyOngoingorLiveCall, needs verification.Additionally, verify that all notification posting paths in this handler are marked. If additional unmarked paths exist (e.g., for missed calls, generic notifications), they should also be updated in the follow-up layer to maintain consistency.
📋 Suggested refined TODOs (example)
- // TODO: Replace StreamCallId.hashCode with StreamCallId.getNotificationId(appropriateType) + // TODO: Replace callId.hashCode() with callId.getNotificationId(NotificationType.LiveCall) — confirm type matches downstream dismissal in AbstractNotificationActivity/LeaveCallBroadcastReceiver - // TODO: Replace StreamCallId.hashCode with StreamCallId.getNotificationId(appropriateType) + // TODO: Replace callId.hashCode() with callId.getNotificationId(NotificationType.???) — determine correct type for generic notification path - // TODO: Replace StreamCallId.hashCode with StreamCallId.getNotificationId(appropriateType) + // TODO: Replace callId.hashCode() with callId.getNotificationId(NotificationType.Ongoing) — verify consistency with downstream isOutgoingCall branching - // TODO: Replace StreamCallId.hashCode with StreamCallId.getNotificationId(appropriateType) + // TODO: Replace callId.hashCode() with callId.getNotificationId(NotificationType.Ongoing) or NotificationType.LiveCall — confirm media-style livestream typeAlso applies to: 257-257, 750-750, 1186-1186
🤖 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 `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandler.kt` at line 206, The TODO comments at lines 206, 257, 750, and 1186 are too vague with "appropriateType" placeholders, which will lead to incorrect NotificationType choices during the migration from callId.hashCode() to StreamCallId.getNotificationId(NotificationType). For the onLiveCall method, the TODO should specify likely LiveCall type; for the onNotification method, clarify which type this generic path requires; for getSimpleOngoingCallNotification, confirm whether Ongoing or a separate outgoing type applies; and for getMinimalMediaStyleNotification, specify whether Ongoing or LiveCall is appropriate. Update each TODO comment to include the specific NotificationType that should be used when the method is refactored, ensuring that the notification IDs posted in these handlers match the IDs used when dismissing notifications downstream (such as in AbstractNotificationActivity).
🤖 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
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/receivers/LeaveCallBroadcastReceiver.kt`:
- Around line 49-59: The notification cancellation in
LeaveCallBroadcastReceiver's onReceive method is performed after call.leave(),
which means if call.leave() throws an exception, the notification cleanup code
will not execute. Wrap the call.leave() invocation in a try-finally block,
moving the notificationManager.cancel() calls (for both notificationId and
legacyNotificationId) into the finally block to guarantee notification cleanup
occurs regardless of whether call.leave() succeeds or fails.
---
Nitpick comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandler.kt`:
- Line 206: The TODO comments at lines 206, 257, 750, and 1186 are too vague
with "appropriateType" placeholders, which will lead to incorrect
NotificationType choices during the migration from callId.hashCode() to
StreamCallId.getNotificationId(NotificationType). For the onLiveCall method, the
TODO should specify likely LiveCall type; for the onNotification method, clarify
which type this generic path requires; for getSimpleOngoingCallNotification,
confirm whether Ongoing or a separate outgoing type applies; and for
getMinimalMediaStyleNotification, specify whether Ongoing or LiveCall is
appropriate. Update each TODO comment to include the specific NotificationType
that should be used when the method is refactored, ensuring that the
notification IDs posted in these handlers match the IDs used when dismissing
notifications downstream (such as in AbstractNotificationActivity).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 726d1491-db51-454c-905d-4b1030d74d0c
📒 Files selected for processing (5)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/DefaultStreamIntentResolver.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/NotificationHandler.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandler.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/receivers/LeaveCallBroadcastReceiver.ktstream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/notification/AbstractNotificationActivity.kt
💤 Files with no reviewable changes (1)
- stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/DefaultStreamIntentResolver.kt
Contributor
SDK Size Comparison 📏
|
aleksandar-apostolov
approved these changes
Jun 23, 2026
rahul-lohra
force-pushed
the
bugfix/rahullohra/battery-restrictions
branch
from
8c237bf to
a0f1e7c
Compare
Base automatically changed from
bugfix/rahullohra/battery-restrictions
to
develop
June 23, 2026 09:05
rahul-lohra
force-pushed
the
bugfix/rahullohra/leave-notification-dismissal
branch
2 times, most recently
from
6436d35 to
838d4b9
Compare
…cation-dismissal # Conflicts: # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandler.kt
|
Collaborator
|
🚀 Available in v1.28.0 |
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
3 participants
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.
Goal
Closes #AND-1252
Stacked on #1731
A customer reported that call notifications were not being dismissed after tapping the Leave/Hang Up action from the notification.
Notification removal can be triggered from two different components:
During the investigation, we found that
LeaveCallBroadcastReceiverwas unable to retrieve the correct notification ID fromINTENT_EXTRA_NOTIFICATION_ID.The SDK was incorrectly populating this extra with the call CID instead of the notification ID when creating notification action intents.
As a result,
LeaveCallBroadcastReceivercould not cancel the corresponding notification, leaving stale notifications visible after the call was ended from the notification action.This change ensures that the correct notification ID is attached to the intent so that notifications are properly dismissed when users leave or hang up a call from the notification.
Implementation
🎨 UI Changes
None
Testing
Test notification dismissal for
Summary by CodeRabbit
Bug Fixes
Chores