Review changes

Review comments

Please check https://phab.comm.dev/D13951#389437 before landing

In D13951#389437, @ashoat wrote:

Just wondering, what do you think of this style?

const { userID, cookieID, sessionID } = viewer;
const data = { userID, cookieID, sessionID };
console.log(
  'Sending INITIAL_NOTIFICATIONS_ENCRYPTED_MESSAGE ' +
    JSON.stringify(data),
);

Could work both here and in D13952.

There is some discussion about user-facing language so probably @ashoat should do the final review

In D13939#389428, @ashoat wrote:

I guess a way to reframe what you're saying about useUsersSupportThickThreads is that userHasDeviceList doesn't match findUserIdentities in the case of an empty device list. We end up returning the correct answer anyways, since we check findUserIdentities as a fallback. But it would be better if we were able to get the right answer without having to query the identity service.

Just wondering, what do you think of this style?

I guess a way to reframe what you're saying about useUsersSupportThickThreads is that userHasDeviceList doesn't match findUserIdentities in the case of an empty device list. We end up returning the correct answer anyways, since we check findUserIdentities as a fallback. But it would be better if we were able to get the right answer without having to query the identity service.

use /* eslint-disable approach

Thanks @angelika. Some concerns:

Review comments

I'm also a bit confused as to why this is necessary or what it accomplishes. I re-read through the Linear task but couldn't find anything. Could you explain why we want to stop swallowing errors here?

With swallowing errors in case findUserIdentities fails because for example there is no internet connection, a thin thread will be created.
Without swallowing errors an error message will be shown to the user and no thread is created.

In D13939#389043, @ashoat wrote:

Hmmm... I know I initially proposed doing both D13938 and this, but after reading D13938 I wonder if we need this additional mechanism.

@kamil, do you know if it's possible for the user to see a thick thread in the UI if one of the members doesn't pass the userHasDeviceList check? Specifically in this case, I'm wondering about a PERSONAL thread, like in @tomek's original report in ENG-9509. It seems to me that the thread creation message would have had to come in from an established Olm session, so I would expect that userHasDeviceList would always be true, and the check in D13938 would suffice.

I went through each of these on my own and I think it's safe

Back to you with question

move react.automatic line

avoid flow errors with react.runtime option to automatic

On the web, we don't support the restoration flow, so I don't think it makes sense to introduce the link to the restore flow.

Update the copy

Squash

Don't love that we're artifically inserting a permission that doesn't exist. Is it truly necessary?

2. If the QR code screen is opened on mobile, then we should say "logged-in phone" or "primary device" instead of just "phone" in the QR code prompt.

I'll create a separate diff for it

https://phab.comm.dev/D13946

Looks great!

make param read only

new approach without using script viewer

Rebase

changing approach

decided not to rename. will instead call my new function fetchPrivilegedThreadInfos

Update the UI. Navigating from the QR screen to the restore flow will be added later in the stack.

To be honest, I think it looks okay without the underline. What do you think?

Agree, it is clear enough that this is a link

can you please make sure that Linear creates issues when these alarms fire?

LGTM but @bartek should review this too