We used to set high priority for every Android notification. Now we believe that this was violation of the OS rules and high priority should be reserved only for visual notifs. Silent updates such as badge update and rescinds should be normal priority.
Details
Details
- Flow
- Check that notification fuinctionality is unchanged.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-7986
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
I might be missing something, but doesn't the notif have to include android: { priority: 'high' } }? This is what Chat GPT seemed to indicate (transcript linked on Linear)
Or is this diff just decreasing priority with the FCM service, but not touching priority on the device? If that's the case, I'll like to understand the motivation for decreasing the priority with FCM.
we believe that this was violation of the OS rules
I don't think either myself or ChatGPT ever indicated this belief regarding the FCM priority. I don't think the OS even knows the FCM priority. Are you sure you read the ChatGPT transcript correctly?
| keyserver/src/push/types.js | ||
|---|---|---|
| 41 ↗ | (On Diff #39742) | It's strange that for encrypted badge-only notifs, we don't include the badgeOnly in the unencrypted payload. But for encrypted visual notifications, we seem to include it. Are the types accidentally wrong here? |
| 82 ↗ | (On Diff #39742) | Accidentally forgot a + sign here I think |
Comment Actions
Oh wait, there are updates on Linear I missed. Let me read them first, sorry for missing that
Comment Actions
It looks like @marcin's investigation has shown that ChatGPT is wrong, and he is pursuing a different theory that perhaps Android is punishing us for sending non-visual notifs (rescind and badge-only) with high priority. I think this is a good direction to explore.
My inline comments above still stand, but the main text of the comment can be ignored.
Comment Actions
I think ChatGPT was right about setting android: {priority: 'high'} - I just found that this is already handled by firebase-admin. I think that setting priority to high (which we are currently doing) for badge updates and rescinds might be a mistake based on this docs: https://firebase.google.com/docs/cloud-messaging/concept-options#setting-the-priority-of-a-message
Comment Actions
Saw this on the doc:
When sending data messages to Apple devices, the priority must be set to 5, or normal priority. Messages sent with high priority are rejected by the FCM backend with the error INVALID_ARGUMENT.
Do you know if any of our notifs are data messages?
Comment Actions
Data messages for Apple devices in FCM correspond to background notifications in plain APNs settings. We don't use background notifications anymore. We used to rescind with background notifs but it is not the case anymore.
| keyserver/src/push/types.js | ||
|---|---|---|
| 41 ↗ | (On Diff #39742) | That is definitely an accidental discrepancy. I think we should encrypt this field for all notifs. |
| keyserver/src/push/send.js | ||
|---|---|---|
| 1209 | Why do we define this variable here? Can we move it closer to its usages? | |
| keyserver/src/push/types.js | ||
| 64–72 | If we spread a type, we probably have to use an explicit $ReadOnly to make it read-only. | |
| 85–89 | ||
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
| 185–189 | Can we make this more generic and report every change, not only from high? | |
| keyserver/src/push/types.js | ||
|---|---|---|
| 64–72 | Note that the $ReadOnly needs to wrap the value associated with the +data key here, since the ... operator removes read-only-ness | |
| keyserver/src/push/crypto.js | ||
|---|---|---|
| 263–282 ↗ | (On Diff #39934) | This looks strange but apparently this is the only way to inform flow that we return the same notification type that we pass into this function. Otherwise flow would think that we might put rescind and return badge only and complained about this, |
| keyserver/src/push/types.js | ||
| 28–38 ↗ | (On Diff #39934) | This is somehow duplication of what is in next lines but I think flow doesn't handle nested spreads. If I use AndroidVisualNotificationPayload directly in next type then flow thinks that it will never have encryptedPayload field and show errors in crypto.js that aren't true. |