[lib] add device type to qr code URL
ClosedPublic
Actions

Authored by varun on Jul 12 2024, 12:17 PM.

Details

Reviewers

ashoat
tomek

Commits

rCOMMc2b7851036f3: [lib] add device type to qr code URL

Summary

we need to know whether the device being scanned is a keyserver so that we don't accidentally add multiple keyservers to a user's device list.

this change is backwards compatible. if a device presents a QR code without a device type, it will still be parsed successfully.

Test Plan

scanned QR code with old URL (sans device type) successfully
scanned QR code with new URL successfully
scanned QR code with new URL successfully from old device unaware of new URL format

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

varun requested review of this revision.Jul 12 2024, 12:34 PM

ashoat added inline comments.

lib/facts/links.js
6–28 ↗	(On Diff #42259)	The way you're representing it in memory when it's passed around (`DeviceType`) is novel. Wonder if we could have used `IdentityDeviceType` for that (the `number` that is used in the gRPC definitions) Additionally, I'm not clear of why it's lowercased here. Do we really need to interface a separate lowercase identifier when we already have an uppercase identifier and a number identifier?
web/account/qr-code-login.react.js
36–38 ↗	(On Diff #42259)	It's already `undefined` at declaration... not sure we need this

This revision is now accepted and ready to land.Jul 13 2024, 4:25 PM

lib/facts/links.js
6–28 ↗	(On Diff #42259)	I opted for a lowercase identifier because it's more descriptive and human-readable. I think the uppercase identifier looks really strange in a URL, but not opposed to the `number` identifier

lib/facts/links.js
48–70 ↗	(On Diff #42259)	I'm not able to convince flow that parsedDeviceType is of the type `IdentityDeviceType`, which is a union type of numbers

lib/facts/links.js
48–70 ↗	(On Diff #42259)	The way to do this is pretty manual, see `assertThinThreadType` for an example

ashoat requested changes to this revision.Jul 31 2024, 10:08 PM

ashoat added inline comments.

lib/facts/links.js
6 ↗	(On Diff #42259)	Let's get rid of this. Instead of using `'KEYSERVER'` at callsites you should can use `identityDeviceTypes.KEYSERVER`. That's the common behavior for this sort of thing
6–28 ↗	(On Diff #42259)	Instead of using this bespoke approach, we can use `identityDeviceTypeToPlatform` here to produce a string Note that this will be slightly different... I think previously you would have `'mac_os'` here, but afterwards you'll have `'macos'` It looks like this distinction matters in `web/grpc/interceptor.js`, so it might need to be handled here as well. If that's the case, please factor the logic out, so it can be shared with `web/grpc/interceptor.js`
native/qr-code/qr-code-screen.react.js
32 ↗	(On Diff #42259)	Once we make that change we should be able to use `platformToIdentityDeviceType` here, right?
web/account/qr-code-login.react.js
25–39 ↗	(On Diff #42259)	Once we make that change we should be able to use `platformToIdentityDeviceType` here, right?

This revision now requires changes to proceed.Jul 31 2024, 10:08 PM

lib/facts/links.js
6–28 ↗	(On Diff #42259)	`keyserver` isn't a platform so we actually can't use `identityDeviceTypeToPlatform`. the `number` identifier is fine i think if you're good with that

address feedback

ashoat added inline comments.

lib/facts/links.js
50 ↗	(On Diff #43273)	Let's replace `[^/]` with `[0-9]`
native/qr-code/qr-code-screen.react.js
33 ↗	(On Diff #43273)	Nit: it might be better to use `getConfig().platformDetails.platform` here too, if only for consistency
web/account/qr-code-login.react.js
15 ↗	(On Diff #43273)	This could be moved out of the `QRCodeLogin` function There's a shorthand you could use here