[lib] Add keyserver to the store on set new session
AbandonedPublic
Actions

Authored by inka on Dec 29 2023, 5:28 AM.

Details

Reviewers

ginsu
michal
kamil

Summary

issue: https://linear.app/comm/issue/ENG-6299/save-new-keyserver-on-set-new-session
We want to save a keyserver if it was not present in the store, but it sent us a valid session

Test Plan

Checked that if a keyserver was not present in the store before, but we dispatch an action that sets a new session for it, it is correctly added with all its fields

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Dec 29 2023, 5:28 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 29 2023, 5:28 AM

inka added a parent revision: D10484: [lib][web][native] Create defaultKeyserverInfo.Dec 29 2023, 5:28 AM

inka added a child revision: D10486: [lib] Make possible to mark ks as temporarily connected.Dec 29 2023, 5:35 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 29 2023, 5:56 AM

Harbormaster failed remote builds in B25394: Diff 35084!

There is an issue on web, because sessionID is being set before this code adds the keyserver to the store. So here previousEntry is truthy, but contains only sessionID, which causes errors down the line

Harbormaster completed remote builds in B25394: Diff 35084.Dec 29 2023, 6:43 AM

inka requested review of this revision.Dec 29 2023, 6:43 AM

inka planned changes to this revision.Dec 29 2023, 6:56 AM

This feels pretty hacky. We should know the urlPrefix before we make the very first call to a keyserver... otherwise, we wouldn't know how to query the keyserver. Can we instead introduce an action that gets dispatched before we call eg. getVersion?

I'm also a bit confused why this is necessary even with the other changes in the stack around temporarily_connected. Couldn't we simply avoid mutating the KeyserverStore in this case, and wait until we log in to populate the KeyserverStore?

In D10485#303043, @ashoat wrote:

This feels pretty hacky. We should know the urlPrefix before we make the very first call to a keyserver... otherwise, we wouldn't know how to query the keyserver. Can we instead introduce an action that gets dispatched before we call eg. getVersion?

I'm also a bit confused why this is necessary even with the other changes in the stack around temporarily_connected. Couldn't we simply avoid mutating the KeyserverStore in this case, and wait until we log in to populate the KeyserverStore?

We do know the urlPrefix, we don't know the id. So we don't have an entry for this keyserver in the store, because the store is keyed with the id. Adding an action before getVersion won't help, because we have no way of finding the keyservers id before we call it.

Our logic was that it could be useful to use the cookie the keyserver returns us in response to getVersion/verifyInviteLink in the next request. So we wanted to store this sessionChange