Conversation
ticket: SDK-1127 epic: SDK-1207
ticket: SDK-1127 epic: SDK-1207
ticket: SDK-1127 epic: SDK-1207
ticket: SDK-1127 epic: SDK-1207
Control PanelHi, I'm an AI-powered Review Bot that helps you with summarizing and reviewing pull requests.
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
Posted review comments for the issues found: missing web fallback for reporting, non-serializable Url in serializable events, and an invalid JSON test constant. One additional iOS fallback issue remains worth addressing as well: the new required reporting field in iOS payload models should default to PushConstants.PUSH_DEFAULT_REPORTING to avoid deserialization failures for payloads that omit it.
PR Bot Information
Version: 1.31.43
- Event Trigger:
issue_comment.created - Correlation ID:
a633eea0-b656-11f1-80cb-be6edf774c63 - LLM:
gpt-5.5 - File Content Strategy: Full file content
|
|
||
| @Serializable | ||
| internal data class RemoteWebPushNotificationV2( | ||
| val reporting: String, |
There was a problem hiding this comment.
Bug: Missing fallback makes web push decoding fail when reporting is absent
The Android path tolerates missing reporting by using PUSH_DEFAULT_REPORTING, but the web remote model now requires the field. Any existing V2 payload without notification.reporting will fail to decode and the notification will not be processed.
Consider defaulting this property to PushConstants.PUSH_DEFAULT_REPORTING.
| val reporting: String, | |
| val reporting: String = PushConstants.PUSH_DEFAULT_REPORTING, |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| */ | ||
| sealed interface OperationalEvent : Internal, OnlineSdkEvent { | ||
| val applicationCode: String? | ||
| val targetUrl: Url? |
There was a problem hiding this comment.
Bug: Url is not serializable in these @Serializable events
OperationalEvent implementations are serializable event data classes, so adding a Url? property requires a kotlinx serializer for io.ktor.http.Url; otherwise this will fail compilation or event persistence serialization. Consider storing a serializable value such as the URL string instead, or mark this runtime-only field as transient if it must not be persisted.
| val targetUrl: Url? | |
| val targetUrl: String? |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const val COLLAPSE_ID = "testCollapseId" | ||
| const val TRACKING_INFO = """{"trackingInfoKey":"trackingInfoValue"}""" | ||
| const val REPORTING = """{"reportingKey":"reportingValue"}""" | ||
| const val DEFAULT_REPORTING = """{defaultReportingKey":"defaultReportingValue"}""" |
There was a problem hiding this comment.
Typo: Default reporting test payload is invalid JSON
The opening quote of defaultReportingKey is missing, so this test now exercises a malformed reporting payload while the rest of the suite uses valid JSON. Consider fixing the literal to keep assertions representative of the production payload.
| const val DEFAULT_REPORTING = """{defaultReportingKey":"defaultReportingValue"}""" | |
| const val DEFAULT_REPORTING = """{"defaultReportingKey":"defaultReportingValue"}""" |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
ticket: SDK-1127 epic: SDK-1207
This branch has not been deployed
1 participant
SDK-1127
In case there is no defaultAction in the push payload, the backend provides a top level reporting field under the notification property on each platform. That should be used to populate the reporting field of the reporting actions.