Details

Reviewers

atul
ginsu
varun
tomek

Commits

rCOMM34ef9be92e3f: [keyserver/lib/web/native] Support external cameras scanning the QR code

Summary

These are the changes to allow Comm to be opened from the external camera scanning one of the QR codes on either web or native. This flow only supports Comm being installed on the phone, and if it is, the QR code will open up the app and navigate to the barcode scanner where we can read the keys encoded in the QR code.

In this diff, I've not actually set any actual keys, but this is all hidden behind development so this won't get released to production. The idea is that down the line, we'll want to store a pair of two keys, so stringifying and encoding a JSON object into the URL that can later be parsed by the barcode scanner made the most sense.

Going to add @tomek as blocking since I tried to use the invite link work as inspiration for the best way to achieve this (especially in regards to link handling).

Addresses ENG-4648

Depends on D8912

Test Plan

Confirmed that the barcode scanner read the data encoded in the QR code correctly still
Had Comm logged in on my phone, then scanned the QR code with the iOS camera and saw Comm open and navigate to the barcode scanner
Had Comm logged out of my phone, then scanned the QR code with the iOS camera and saw Comm open. After logging in, it took me to the barcode scanner.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Aug 24 2023, 6:53 AM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 24 2023, 6:53 AM

rohan edited the summary of this revision. (Show Details)Aug 24 2023, 6:53 AM

Harbormaster completed remote builds in B22057: Diff 30279.Aug 24 2023, 7:10 AM

rohan requested review of this revision.Aug 24 2023, 7:10 AM

An additional thing worth testing is a case where Comm is closed when the code is scanned.

I think that a long-term solution might be to have one deep link handler/listener, that would decide which handler to call (either invite links or QR code). It might be a little challenging to do this because of InviteLinksContextProvider.

native/navigation/qr-code-link-handler.react.js
13–40 ↗	(On Diff #30279)	It doesn't seem that we're checking if this is a QR code link. What will happen when an invite link is opened? Won't that result in `SecondaryDeviceQRCodeScannerRouteName` being opened?
41 ↗	(On Diff #30279)	It is a good idea to explicitly return from a component
native/qr-code/qr-code-screen.react.js
21–24 ↗	(On Diff #30279)	This value will change in the near future, so it probably doesn't matter, but overall it is a good idea to compute things that don't depend on props or state outside the component.

This revision now requires changes to proceed.Aug 25 2023, 2:10 AM

In D8934#263624, @tomek wrote:

An additional thing worth testing is a case where Comm is closed when the code is scanned.

I thought it did, but it seems like when Comm is closed and a QR code is scanned, Comm opens but it's stuck on the splash screen. I'll update this diff to address feedback and try to investigate why

I think that a long-term solution might be to have one deep link handler/listener, that would decide which handler to call (either invite links or QR code). It might be a little challenging to do this because of InviteLinksContextProvider.

Yeah I agree, I was thinking something similar would be good, especially to make it easier down the line if we want more handlers/listeners. I'll create a follow up task so we can keep track of this: https://linear.app/comm/issue/ENG-4810/combine-deep-link-handlers-listeners-into-one-component

Use inviteLinkUrlPrefix and arCodeLinkUrlPrefix to handle respective deep links opening (so clicking an invite link doesn’t open the barcode scanner)
Return null explicit from the component
Compute qrCodeValue outside of the component

Need to figure out why Comm gets stuck on the splash screen if it was closed when a QR code was scanned

Harbormaster completed remote builds in B22090: Diff 30318.Aug 25 2023, 7:13 AM

After reading through this: https://reactnative.dev/docs/linking#getinitialurl, I made a prod release of Comm and scanned the QR code while the app was closed. Comm opened and successfully navigated to the barcode scanner

Sorry for requesting changes once again, but the logic in invite-links-context-provider seems to be a little brittle and I'd like to verify the changes before landing.

native/invite-links/invite-links-context-provider.react.js
83 ↗	(On Diff #30318)	This is unfortunately more complicated. D8693 changes iOS link URL, because the `https` one doesn't work on Chrome. So we have to also check if the link starts with `comm://invite/`.
88–98 ↗	(On Diff #30318)	We're actually already checking the condition by parsing a `secret` and checking if it's there. `parseSecret` would extract the secret from both `https` and `comm` schemes so we might be ok to leave it as it was, or refactor the function to make it more obvious what it does.

This revision now requires changes to proceed.Aug 28 2023, 1:31 AM

@tomek I think the best thing to do is to follow your suggestion of keeping invite links provider as-is, and actually follow a similar logic to parseSecret rather than checking for the URL existence in the QR code handler since it will be a little more resistant to changes

native/invite-links/invite-links-context-provider.react.js
83 ↗	(On Diff #30318)	Oh right thanks for pointing that out, I didn't consider the different URL there
88–98 ↗	(On Diff #30318)	That's a good point, I somehow skipped over `urlRegex` only matching links that contain the invite link pattern, and not all generic links.

Leave invite-links-context-provider.react.js as is
Revert the changes to links.js to export the urls
Follow a similar logic to parseSecret with a parseKeys function in qr-code-link-handler.react.js since it's more resistant to changing the url schema
Tested that clicking invite links doesn't open the scanenr and opening the qr-code schema in the app doesn't prompt the invite links modal