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)
Sat, Nov 23, 9:31 AM
Unknown Object (File)
Sat, Nov 23, 7:40 AM
Unknown Object (File)
Tue, Nov 12, 9:32 AM
Unknown Object (File)
Tue, Nov 12, 8:52 AM
Unknown Object (File)
Tue, Nov 12, 4:35 AM
Unknown Object (File)
Fri, Nov 8, 10:39 PM
Unknown Object (File)
Fri, Nov 8, 7:14 AM
Unknown Object (File)
Mon, Nov 4, 10:48 PM
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
Branch
hackathon (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

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

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

i'm not sure i understand your feedback here

native/profile/secondary-device-qr-code-scanner.react.js
258

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

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

Thanks for explaining