Page MenuHomePhabricator

Update extractKeyserverIDFromID so that it returns null for DMs
ClosedPublic

Authored by tomek on Wed, Jul 3, 6:28 AM.
Tags
None
Referenced Files
F2322402: D12650.id42108.diff
Tue, Jul 23, 8:31 AM
Unknown Object (File)
Mon, Jul 22, 7:37 PM
Unknown Object (File)
Mon, Jul 22, 6:03 AM
Unknown Object (File)
Sun, Jul 21, 1:21 AM
Unknown Object (File)
Sat, Jul 20, 11:49 PM
Unknown Object (File)
Fri, Jul 19, 8:21 PM
Unknown Object (File)
Fri, Jul 19, 4:15 PM
Unknown Object (File)
Fri, Jul 19, 4:44 AM
Subscribers

Details

Summary

This requires changes in a lot of places because we were assuming that a thread ID is present.

https://linear.app/comm/issue/ENG-8427/make-sure-we-dont-run-keyserver-client-state-check-on-e2ee-dms

Test Plan

Mostly Flow. Tested some simple scenarios, e.g. sending a message.

Diff Detail

Repository
rCOMM Comm
Branch
master12
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Wed, Jul 3, 6:44 AM
bartek added 1 blocking reviewer(s): inka.

Okay, this makes sense ๐Ÿ˜‰

inka requested changes to this revision.Thu, Jul 4, 4:15 AM
inka added inline comments.
lib/actions/activity-actions.js
32โ€“34

It really feels like we should extract this to a function

lib/keyserver-conn/keyserver-call-utils.js
21

This will throw in keyserverAuth if threadWatcher.getWatchedIDs() includes thick threads. Are we sure this can't happen?

native/chat/settings/thread-settings-push-notifs.react.js
179โ€“184

I feel like this could be done inside of deviceTokenSelector, but not sure

web/calendar/entry.react.js
479โ€“481

Shouldn't this be checking some connection to tunnelbroker? Probably not in this diff, but it feels like we shouldn't just leave this like that. There probably is a reason for checking if we can send the save entry request

This revision now requires changes to proceed.Thu, Jul 4, 4:15 AM
lib/keyserver-conn/keyserver-call-utils.js
21

Great catch

web/calendar/entry.react.js
479โ€“481

Looks like the only place this is used is to trigger auto-retries of failed entry updates once we detect the user has come online

What @tomek is doing here is basically disabling that automatic retry. Checking Tunnelbroker connection would probably be appropriate, but it might not be necessary if we have a separate automatic retry mechanism, which I think @tomek is planning for this month (but tracked separately)

Unrelated: I might suggest collapsing this all into a single const online = useSelector

native/chat/settings/thread-settings-push-notifs.react.js
179โ€“184

That would make sense, but I'm not sure if in other places where this selector is used, we should present a tunnelbroker token.

Check the IDs in state sync specs

lib/keyserver-conn/keyserver-call-utils.js
76 โ†—(On Diff #42142)

I don't think !keyserverID can happen here? extractKeyserverIDFromID is the one that throws, right?

native/components/auto-join-community-handler.react.js
129 โ†—(On Diff #42142)

We could use extractKeyserverIDFromID here, because we don't expect thick threads to be tagged with farcaster, but up to you

web/calendar/entry.react.js
479โ€“481

I probably should do something about documenting it. Going to figure something out before landing the diff.

web/calendar/entry.react.js
479โ€“481

I think it's documented here

lib/keyserver-conn/keyserver-call-utils.js
76 โ†—(On Diff #42142)

Yes, it can't happen. We should use the optional one.

native/components/auto-join-community-handler.react.js
129 โ†—(On Diff #42142)

Makes sense!

This revision is now accepted and ready to land.Thu, Jul 18, 8:23 AM