Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

πŸ¦β€πŸ”₯ [FIX]: Refresh ledger sync QR code on expiration #7690

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

thesan
Copy link
Contributor

@thesan thesan commented Aug 27, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

πŸ“ Description

It retries the QR code processing flow when the WS closes unexpectedly. Since AFAIK there is no way to make sure the WS closed because of the QR code expiration: if the connection closes in less than 30 seconds the flow is not retried and the old error is shown. Here's a demo the behaviour with a min time 3 seconds:

Screen.Recording.2024-08-27.at.17.12.23.mov

I ended up using react query because I was getting some weird behaviour with the old logic. However I didn't manage to use the build in retry option because the delay option didn't work for me it kept on retrying after +30 seconds instead of immediatley 🀷.

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@thesan thesan requested a review from a team as a code owner August 27, 2024 15:31
Copy link

vercel bot commented Aug 27, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

5 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 1:12pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 1:12pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 1:12pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 1:12pm
web-tools ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 1:12pm

@live-github-bot live-github-bot bot added desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs labels Aug 27, 2024
@thesan thesan changed the title πŸ¦β€πŸ”₯ Refresh ledger sync QR code on expiration πŸ¦β€πŸ”₯ [FIX]: Refresh ledger sync QR code on expiration Aug 27, 2024
@thesan thesan force-pushed the fix/refresh-trustchain-qr-code branch from 927ebdb to ae6dd17 Compare August 27, 2024 16:16
@live-github-bot live-github-bot bot added the mobile Has changes in LLM label Aug 27, 2024
@mcayuelas-ledger
Copy link
Contributor

At the end we have an error message?

@thesan
Copy link
Contributor Author

thesan commented Aug 28, 2024

At the end we have an error message?

@mcayuelas-ledger the error message won't show even if you stayed several days on the QR code page. Every time the QR code expire a new WS connection will created and the QR code will be refreshed. But if the WS connection closes in less than 30 seconds something else is wrong so the "WS closed prematurely" error would be shown. I wanted to avoid to constantly refresh the WS connection when something actually is wrong with it (and also potentially IP ban users).

(In the screen recording I lowered the 30 seconds to 3).

}
if (e instanceof NoTrustchainInitialized) {
dispatch(setFlow({ flow: Flow.Synchronize, step: Step.UnbackedError }));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to add

setError(e); throw e; also no? Like before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setError is not needed because react-query manages the error (that's why I removed the useState).

For the throw I don't think it's needed either. From my understanding it was here to avoid the then that follows. Or am I missing something ?

if (e instanceof NoTrustchainInitialized) {
setCurrentStep(Steps.UnbackedError);
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here?

Copy link
Contributor

@mcayuelas-ledger mcayuelas-ledger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@thesan thesan merged commit a3fd728 into develop Aug 30, 2024
62 of 64 checks passed
@thesan thesan deleted the fix/refresh-trustchain-qr-code branch August 30, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants