Page MenuHomePhabricator

Update extractKeyserverIDFromID so that it returns null for DMs
ClosedPublic

Authored by tomek on Jul 3 2024, 6:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 1:10 PM
Unknown Object (File)
Fri, Nov 8, 11:05 PM
Unknown Object (File)
Fri, Nov 8, 12:25 PM
Unknown Object (File)
Tue, Nov 5, 9:59 AM
Unknown Object (File)
Thu, Oct 24, 10:24 AM
Unknown Object (File)
Fri, Oct 18, 8:32 AM
Unknown Object (File)
Oct 14 2024, 8:40 PM
Unknown Object (File)
Oct 14 2024, 8:40 PM
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.Jul 3 2024, 6:44 AM
bartek added 1 blocking reviewer(s): inka.

Okay, this makes sense ๐Ÿ˜‰

inka requested changes to this revision.Jul 4 2024, 4:15 AM
inka added inline comments.
lib/actions/activity-actions.js
32โ€“34 โ†—(On Diff #41950)

It really feels like we should extract this to a function

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

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 โ†—(On Diff #41950)

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

web/calendar/entry.react.js
479โ€“481 โ†—(On Diff #41950)

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.Jul 4 2024, 4:15 AM
lib/keyserver-conn/keyserver-call-utils.js
21 โ†—(On Diff #41950)

Great catch

web/calendar/entry.react.js
479โ€“481 โ†—(On Diff #41950)

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 โ†—(On Diff #41950)

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 โ†—(On Diff #41950)

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

web/calendar/entry.react.js
479โ€“481 โ†—(On Diff #41950)

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.Jul 18 2024, 8:23 AM