Page MenuHomePhabricator

[lib] Track progress of message processing
ClosedPublic

Authored by tomek on Jul 17 2024, 10:54 AM.
Tags
None
Referenced Files
F3347908: D12786.diff
Fri, Nov 22, 1:33 PM
Unknown Object (File)
Sat, Nov 9, 1:45 PM
Unknown Object (File)
Fri, Nov 8, 5:13 PM
Unknown Object (File)
Tue, Nov 5, 12:03 AM
Unknown Object (File)
Mon, Nov 4, 1:03 AM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Subscribers

Details

Summary

Messages are scheduled using a function that returns a promise which is resolved after messages are sent.

https://linear.app/comm/issue/ENG-8674/track-progress-of-sending-tunnelbroker-messages

Depends on D12776

Test Plan

Tested on both native and web. Added some code

await this.props.p2p.sendDMOperation({
  op: {
    type: 'send_text_message',
    threadID: messageInfo.threadID,
    creatorID: messageInfo.creatorID,
    time: messageInfo.time,
    messageID: getUUID(),
    text: messageInfo.text,
  },
  supportsAutoRetry: false,
  recipients: 'self_devices',
});

to input state containers so that it is called after sending messages. Checked if the second device received a message after sending a text message.

Diff Detail

Repository
rCOMM Comm
Branch
dms (branched from palys-swm/dms)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

I wonder what the difference is between our terms "P2P messages" and "DM"s

I think "P2P messages" cover a superset of DMs – it's basically all client-to-client messages that are encrypted and passed via Tunnelbroker. Does that sound right? And is it consistent with our usage here?

I wonder what the difference is between our terms "P2P messages" and "DM"s

I think "P2P messages" cover a superset of DMs – it's basically all client-to-client messages that are encrypted and passed via Tunnelbroker. Does that sound right? And is it consistent with our usage here?

Yes, I agree - "P2P messages" cover a superset of DMs. What I'm describing here is sending P2P messages generated from a single DM operation. I'm going to review the naming and make sure it isn't confusing.

Allow processing DB ops with only p2p messages

I think the usage of if and dmOpID can be improved and more descriptive, this might look confusing as we have plenty IDs all over the codebase - but I don't have idea about better ones

lib/tunnelbroker/peer-to-peer-context.js
10–11 ↗(On Diff #42467)

can be merged

167 ↗(On Diff #42467)

Shouldn't we reject with error (alternatively use mixed here)?

lib/types/dm-ops.js
100 ↗(On Diff #42467)

I think is better for readability but up to you

web/shared-worker/utils/store.js
227 ↗(On Diff #42467)

Happy you fixed this one

This revision is now accepted and ready to land.Jul 18 2024, 7:18 AM

I think the usage of if and dmOpID can be improved and more descriptive, this might look confusing as we have plenty IDs all over the codebase - but I don't have idea about better ones

Makes sense. In fact id and dmOpID are the same ID.

Rebase and improve naming