Page MenuHomePhabricator

Create large notifications in thick threads
ClosedPublic

Authored by marcin on Sep 25 2024, 11:14 AM.
Tags
None
Referenced Files
F3515518: D13469.id44635.diff
Sun, Dec 22, 9:06 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: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
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.