Page MenuHomePhabricator

[native] modify SecondaryDeviceQRCodeScanner to support emulator
ClosedPublic

Authored by varun on Jun 28 2024, 12:58 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 24, 1:04 AM
Unknown Object (File)
Mon, Oct 21, 1:04 PM
Unknown Object (File)
Sat, Oct 19, 4:11 AM
Unknown Object (File)
Fri, Oct 11, 7:22 PM
Unknown Object (File)
Fri, Oct 11, 7:05 PM
Unknown Object (File)
Fri, Oct 11, 7:02 PM
Unknown Object (File)
Oct 1 2024, 7:55 AM
Unknown Object (File)
Oct 1 2024, 5:37 AM
Subscribers

Details

Summary

if we're trying to add a device from an iOS simulator or android emulator, we should just display a TextInput where the user can paste the QR Code URL

Depends on D12619

Test Plan

if device is added successfully, we are navigated back upon acknowledging the alert

if device can't be added successfully, we are navigated back when the alert fires

adding keyserver as secondary device works -- appears in DDB correctly

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Jun 28 2024, 1:13 PM
native/profile/secondary-device-qr-code-scanner.react.js
169 ↗(On Diff #41821)

You did use the alert constants above, but not here

ashoat requested changes to this revision.Jul 1 2024, 5:25 PM

Love some of the improvements you made to the existing code, such as passing more specific data into the hooks

Requesting chnages for some questions, and to remove the button for non-emulators

native/profile/secondary-device-qr-code-scanner.react.js
61 ↗(On Diff #41821)

Nit

169 ↗(On Diff #41821)

Personally, I'm not sure factoring it out is necessary if the string isn't duplicated elsewhere. I'd probably inline secondaryDeviceRegisteredAlertDetails

248 ↗(On Diff #41821)

Is this logic copied from somewhere? Wondering how the QR code parsing happens when we get the URL via a deep link, versus this new method

If it's duplicated, would be good to factor out

251 ↗(On Diff #41821)

Did you forget to remove this console.log?

263–273 ↗(On Diff #41821)

Don't we want to only show this button when deviceIsEmulator?

This revision now requires changes to proceed.Jul 1 2024, 5:25 PM
native/profile/secondary-device-qr-code-scanner.react.js
248 ↗(On Diff #41821)

i didn't realize parseDataFromDeepLink does what we need here

ashoat added inline comments.
native/profile/secondary-device-qr-code-scanner.react.js
258 ↗(On Diff #42243)

Can we use parsedData for this too?

This revision is now accepted and ready to land.Jul 11 2024, 9:19 PM
native/profile/secondary-device-qr-code-scanner.react.js
258 ↗(On Diff #42243)

i'm not sure i understand your feedback here

native/profile/secondary-device-qr-code-scanner.react.js
258 ↗(On Diff #42243)

In the last revision, you realized we could use parseDataFromDeepLink to parse the urlInput. Is it possible to use its result (parsedData) for this parsing step as well?

native/profile/secondary-device-qr-code-scanner.react.js
258 ↗(On Diff #42243)

we are doing that, right? keysMatch = parsedData?.data?.keys gives us the encoded URI component, so we decode it and then parse it to a JSON object here

parseDataFromDeepLink() returns a ParsedDeepLinkData, which is a union of types including

type ParsedQRCodeData = {
  +type: 'qr-code',
  +data: { +keysAndDeviceType: string },
};
native/profile/secondary-device-qr-code-scanner.react.js
258 ↗(On Diff #42243)

Thanks for explaining