In D9294#273227, @atul wrote:

2. Further, instead of "rolling our own" compressMessage, could we use the WebSocket per message-deflate extension? (https://datatracker.ietf.org/doc/html/rfc7692). Seems like it can seamlessly be integrated w/ ws: https://github.com/websockets/ws#websocket-compression. This leverages the Node/zlib library mentioned above out of the box.

Can we leverage Brotli compression/decompression functionality that's built into Node/Zlib instead of introducing a new dependency?

Example of using built-in Node functionality

Accepting to unblock, however, would be good to get thoughts on the following:

Is it valid to require policy acceptance only when a user is logged in? When a user isn't logged in, we don't allow them to do much, but I'm wondering, if from the legal perspective, we should require them to agree to something.

I just realized a problem with approach: we don't want to send all of the input to all of the keyservers. For example in D9240, in fetchSingleMostRecentMessagesFromThreads we want to send each keyserver only the ids that it "owns". Otherwise we could expose private date to other keyservers. We need some logic to split a single request into multiple request, one for each keyserver.

Requesting changes because I have a few smaller comments

I was testing compression while trying to reduce report sizes, but I used different packages ENG-3890 and failed to find something working well on reports, this one seems promising, especially on native we use only decompression which is less CPU consuming than compression, but you can you somehow test this with large payloads to make sure it will not kill the app performance for the time of decompressing?

Yeah, the electron clients are bit bigger problem... There is a task for the reload prompt: ENG-961 but it wasn't prioritized. For this task I guess we could just leave this diff on phab for some time to let clients migrate? For the desktop app we could even make a "dummy" release to force a reload but not sure if that's worth it.

This actually breaks threadInfoFromSourceMessageIDSelector... I'll need to make the change up the stack

nice!

rebase and land

how often are we rotating the token?

simplify logic

So I actually tried to include this in D9227: https://phab.comm.dev/D9227?vs=on&id=31269#toc, but there was some additional stuff that got included when I ran patch package (figured that was safer than just renaming file)

Update type imports

Simplify logic in chat-selectors.js and add parentThreadInfo to NotificationTextsParam

Thanks, this looks super close!!

Note: that means that web clients will need to reload to run the cookie migration.

1. Renamed parseSourceMessageIDFromPendingThreadID to parseSourceMessageIDFromPendingSidebarID
2. Update the check in change-role-message-spec.js (I’d rather not throw an invariant here since we cover the unexpected case of roleName just simply being undefined in constructChangeRoleEntityText
3. Pass in parentThreadInfo to robotextForMessageInfo from getMessageTitle
4. Pass in parentThreadInfo to robotextForMessageInfo from notifRobotextForMessageInfo. Drill it from sendPushNotif —> notifTextsForMessageInfo —> fullNotifTextsForMessageInfo —> messageSpec.notificationTexts —> notifRobotextForMessageInfo

That one is trivial to fix... here you go: D9284

Hey, I run yarn cleaninstall and got a warning:

Warning: patch-package detected a patch file version mismatch

One quick note: my original idea in ENG-4981 was to "extend RobotextParams to include a sidebar parent's ThreadInfo". In this case you appear to have extended it to include all threads' parents' ThreadInfo, which I think is probably better. I thought about whether we could save some work by reducing scope to my original proposal, but probably not. But if you think of something let me know.

It's generally an expectation that if you put a diff back on a reviewer's queue, you have addressed that reviewer's comments. If you haven't, you should at least attempt to explain why you're putting the diff back in that reviewer's queue (maybe you have a question?). Otherwise you should hit the "Plan Changes" button to keep it away from reviewers' queues until you've addressed all of the comments.

Don't forget to add a comment to ENG-2199 before landing!

Not a fan of keeping the solution with mutating input, but accepting this since fixing this is not a part of this work. Maybe we can at least create a task to improve this?