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.

Sign up for GitHub

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

Jump to bottom

Bugfix/live 9301 infinite loader #6394

Merged
merged 5 commits into from
Mar 25, 2024
Merged

Bugfix/live 9301 infinite loader #6394

merged 5 commits into from
Mar 25, 2024

Conversation

aussedatlo
Copy link
Contributor

@aussedatlo aussedatlo commented Mar 8, 2024 •
edited
Loading

📝 Description

In a case where the device is waiting for a user prompt (for example open Ethereum app)
The device action can result in an infinite loader because the unresponsive state of the device is not handled correctly.

  • Fix the event implementation to don't filter out the unresponsive event
  • Improve the displayed error in case of unresponsive event

Before:

LIVE-99301-before.mov

After:

LIVE-9301-after.mov

❓ Context

✅ Checklist

Pull Requests must pass the CI and be code reviewed. Set as Draft if the PR is not ready.

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes: A new Error display is added in case of unresponsive event for LLD. Some changes are made in LLC, that impact LLD and LLM.
    • ...

🧐 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)

@aussedatlo aussedatlo requested a review from a team as a code owner March 8, 2024 08:31
@vercel Vercel
Copy link

vercel bot commented Mar 8, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-tools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 13, 2024 8:17am
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Mar 13, 2024 8:17am
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Mar 13, 2024 8:17am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Mar 13, 2024 8:17am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Mar 13, 2024 8:17am

@live-github-bot live-github-bot bot added desktop Has changes in LLD common Has changes in live-common translations Translation files have been touched labels Mar 8, 2024
smartine-ledger
smartine-ledger reviewed Mar 8, 2024
View reviewed changes
apps/ledger-live-desktop/src/renderer/components/DeviceAction/index.tsx Outdated
@@ -413,6 +415,15 @@ export const DeviceActionDefaultRendering = <R, H extends States, P>({
});
}

if (unresponsive || error instanceof TransportRaceCondition) {
Copy link

Choose a reason for hiding this comment

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

[Comment] Not relate to your PR but what a generic name for an error TransportRaceCondition 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thoughts 😅 we should maybe rename it like TransportPendingOperation. It would be more clear and less weird

smartine-ledger
smartine-ledger previously approved these changes Mar 8, 2024
View reviewed changes
valpinkman
valpinkman previously approved these changes Mar 8, 2024
View reviewed changes
@aussedatlo aussedatlo force-pushed the bugfix/LIVE-9301-infinite-loader branch from d2b248a to 0c5a47c Compare March 8, 2024 09:53
@aussedatlo aussedatlo requested a review from a team as a code owner March 8, 2024 10:22
@live-github-bot live-github-bot bot added mobile Has changes in LLM ledgerjs Has changes in the ledgerjs open source libs labels Mar 8, 2024
mcayuelas-ledger
mcayuelas-ledger approved these changes Mar 8, 2024
View reviewed changes
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.

Ok for Hub team scope :)

@aussedatlo aussedatlo force-pushed the bugfix/LIVE-9301-infinite-loader branch from e250902 to 931d511 Compare March 13, 2024 08:11
@aussedatlo aussedatlo merged commit fe951dd into develop Mar 25, 2024
58 checks passed
@aussedatlo aussedatlo deleted the bugfix/LIVE-9301-infinite-loader branch March 25, 2024 10:44
@devfive
Copy link

devfive commented Apr 23, 2024

hey @aussedatlo this is a breaking change, not in line with semver

https://github.com/LedgerHQ/ledger-live/pull/6394/files#diff-23143765da90843c3585306c99595def197f9c183e4b2365436918b1bf985e35L127

Can we bring TransportRaceCondition back with deprecation notice? It breaks for example Solana wallet adapter https://github.com/anza-xyz/wallet-adapter

@live-github-bot
Copy link
Contributor

@devfive you are not part of the organization, please contact a maintainer if you need to run this command.

3 similar comments
@live-github-bot
Copy link
Contributor

@devfive you are not part of the organization, please contact a maintainer if you need to run this command.

@live-github-bot
Copy link
Contributor

@devfive you are not part of the organization, please contact a maintainer if you need to run this command.

@live-github-bot
Copy link
Contributor

@devfive you are not part of the organization, please contact a maintainer if you need to run this command.

@devfive devfive mentioned this pull request Apr 23, 2024
Merged
@gre gre mentioned this pull request May 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiyuzhuang jiyuzhuang jiyuzhuang left review comments

@smartine-ledger smartine-ledger smartine-ledger approved these changes

@mcayuelas-ledger mcayuelas-ledger mcayuelas-ledger approved these changes

@valpinkman valpinkman valpinkman left review comments

Assignees

@aussedatlo aussedatlo

Labels
common Has changes in live-common desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM translations Translation files have been touched
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aussedatlo @devfive @valpinkman @jiyuzhuang @smartine-ledger @mcayuelas-ledger