Page MenuHomePhabricator

Create large notifications in thick threads
ClosedPublic

Authored by marcin on Sep 25 2024, 11:14 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 18, 9:20 AM
Unknown Object (File)
Wed, Dec 18, 9:20 AM
Unknown Object (File)
Wed, Dec 18, 9:20 AM
Unknown Object (File)
Wed, Dec 18, 9:19 AM
Unknown Object (File)
Mon, Dec 16, 8:33 AM
Unknown Object (File)
Sat, Dec 14, 5:58 AM
Unknown Object (File)
Fri, Dec 13, 4:05 PM
Unknown Object (File)
Fri, Nov 29, 2:03 AM
Subscribers

Details

Summary

This differential creates large notifications in thick threads

Test Plan

Tested in final diff

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

It's quite a large diff adding support for large notifs - I think it would be a lot easier to review it if it was split into multiple smaller ones. We don't have the time to split it, but overall the diff makes sense to me.

lib/push/send-utils.js
505 ↗(On Diff #44563)

Can this be readonly?

793 ↗(On Diff #44563)
816 ↗(On Diff #44563)

Why is it inexact?

This revision is now accepted and ready to land.Sep 27 2024, 4:44 AM
lib/push/send-utils.js
505 ↗(On Diff #44563)

I must have forgotten to update this diff. This code is no longer there

793 ↗(On Diff #44563)

It is not going to work with eslint since largeNotifDataArray is already declared in the upper scope

816 ↗(On Diff #44563)

buildNotifsForPlatform expects a function that returns

{
      +targetedNotificationsWithPlatform: $ReadOnlyArray<TargetedNotificationWithPlatform>,
      +largeNotifDataArray?: ...
}

but function that create badge updates and rescinds return only targetedNotificationsWithPlatform. Making the type here inexact solves flow complaint.

This revision was landed with ongoing or failed builds.Sep 27 2024, 7:48 AM
This revision was automatically updated to reflect the committed changes.