[native][web] Retry QR code generation
ClosedPublic
Actions

Authored by tomek on Feb 18 2025, 4:25 PM.

Details

Reviewers

kamil
bartek

Commits

rCOMM8f7ba569bfd4: [native][web] Retry QR code generation

Summary

When QR generation fails, we should retry it. We're doing it with some timeout to avoid making the calls too frequent.

https://linear.app/comm/issue/ENG-10209/seeing-error-failed-to-generate-qr-code-message-task-cancelled

This diff also introduces a loading state for QR on native:

Test Plan

Modified the QR generation call to success infrequently and checked if it was retried.

Tested the following scenario both on native and web:

Open the QR screen on a logged out device
Log in
Log out
Log in again

Before this diff, this fails. After this diff, a loading state of the QR code is shown, and then the correct QR code is displayed.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Feb 18 2025, 4:25 PM

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 18 2025, 4:25 PM

tomek edited the summary of this revision. (Show Details)Feb 18 2025, 4:26 PM

Harbormaster completed remote builds in B33544: Diff 47150.Feb 18 2025, 4:44 PM

tomek requested review of this revision.Feb 18 2025, 4:44 PM

kamil requested changes to this revision.Feb 19 2025, 10:53 AM

kamil added inline comments.

lib/components/secondary-device-qr-auth-context-provider.react.js
276 ↗	(On Diff #47150)	Do we want to keep this condole log?
native/account/qr-code-screen.react.js
42–52 ↗	(On Diff #47150)	can't we wrap this logic in `openSecondaryQRAuth` to avoid code duplication?
42–55 ↗	(On Diff #47150)	I think this logic is broken now, as the cleanup function can close TB connection not on unmounting, but when `isGenerationScheduled` becomes `false`

This revision now requires changes to proceed.Feb 19 2025, 10:53 AM

Worth adding full QR auth flow to test plan, not only generating QR code

Change the approach to use attempt number instead of a flag. Also, fix a bug where the QR is invalid just after the logout - it is fixed by clearing the QR data after sending a message.

tomek added inline comments.Feb 19 2025, 1:14 PM

lib/components/secondary-device-qr-auth-context-provider.react.js
276 ↗	(On Diff #47150)	Yeah, I think it might be helpful in debugging
native/account/qr-code-screen.react.js
42–52 ↗	(On Diff #47150)	This code duplication is quite hard to avoid because we're using a focus effect in one of the places. I'm not sure if it is worth it, given how simple the logic is in the current revision.
42–55 ↗	(On Diff #47150)	Right, it was broken