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, Nov 20, 9:59 AM
Unknown Object (File)
Sat, Nov 16, 2:44 AM
Unknown Object (File)
Fri, Nov 8, 3:02 AM
Unknown Object (File)
Fri, Nov 8, 3:02 AM
Unknown Object (File)
Fri, Nov 8, 3:00 AM
Unknown Object (File)
Fri, Nov 1, 4:22 PM
Unknown Object (File)
Sun, Oct 27, 3:43 PM
Unknown Object (File)
Thu, Oct 24, 2:01 AM
Subscribers

Details

Summary

This differential creates large notifications in thick threads

Test Plan

Tested in final diff

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

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.