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: Add loading states to WC #3441

Merged
merged 2 commits into from
Mar 19, 2024
Merged

fix: Add loading states to WC #3441

merged 2 commits into from
Mar 19, 2024

Conversation

usame-algan
Copy link
Member

@usame-algan usame-algan commented Mar 14, 2024

What it solves

Resolves #3200

How this PR fixes it

  • Adds loading indicators for disconnect, reject and approve
  • Moves the onDisconnect, onReject and onApprove handler to the components where they are used instead of prop drilling them

Other changes:

πŸ’‘ These aim to reduce complexity and improve the UX. We should discuss if we want to keep them or not. πŸ’‘

  • Removes the handleBeforeUnload call as there is no error and no such handling in the example app. Reloading the page during an approval doesn't show any errors either, just an "Unknown" pairing but no active session. Using the same uri again establishes a session and updates the "Unknown" pairing which is expected.
  • Removes WcConnectionState and associated logic. Switching back and forth with the dialogs when approving/rejecting takes the user out of the current context which is confusing imo. We should rather keep the user in the current dialog and display a loading state there.

How to test it

  1. Open the app, open the WC widget and paste a uri
  2. Press Reject or Approve
  3. Observe a loading indicator
  4. Press Disconnect
  5. Observe a loading indicator

We should check that after pasting the URI into the input field and reloading the page before pressing reject and approve doesn't cause any errors.

Screenshots

Checklist

  • I've tested the branch on mobile πŸ“±
  • I've documented how it affects the analytics (if at all) πŸ“Š
  • I've written a unit/e2e test for it (if applicable) πŸ§‘β€πŸ’»

Copy link

github-actions bot commented Mar 14, 2024

Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: βœ… success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link

github-actions bot commented Mar 14, 2024

πŸ“¦ Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. πŸ€–

πŸŽ‰ Global Bundle Size Decreased

Page Size (compressed)
global 1016.32Β KB (-4Β B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

github-actions bot commented Mar 14, 2024

Coverage report

St.❔
Category Percentage Covered / Total
🟑 Statements
79.38% (-0.02% πŸ”»)
11124/14013
πŸ”΄ Branches
58.42% (-0.03% πŸ”»)
2612/4471
🟑 Functions
66.09% (-0.05% πŸ”»)
1795/2716
🟒 Lines
80.64% (-0.02% πŸ”»)
10020/12425
Show files with reduced coverage πŸ”»
St.❔
File Statements Branches Functions Lines
🟒
... / index.tsx
82.35% (-8.82% πŸ”»)
66.67% (-16.67% πŸ”»)
50% (-12.5% πŸ”»)
83.87% (-9.68% πŸ”»)
🟑 src/pages/404.tsx 68.75%
64.29% (-7.14% πŸ”»)
50% 71.43%

Test suite run success

1411 tests passing in 195 suites.

Report generated by πŸ§ͺjest coverage report action from 4b6d6fd

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

πŸ‘

@francovenica
Copy link
Contributor

francovenica commented Mar 19, 2024

The only "issue" I found by playing with the page reload is that, if you click "Disconnect" and reload the page at the same time, then after the reload you still look connected in the safe app, while you got disconnected in the 3rd party app.
Still I think this is a very nish case, and you can still click Disconnect again with no errors
WC connection

Beyond this the rest worked fine for me connecting and disconnecting from apps.
Even tried executing tx in sepolia with cowswap with no issues from the safe side (cowswap is taking ages to execute the tx tho...)

@usame-algan
Copy link
Member Author

While working on #3395 I realised it would be nicer to only have one loading state and keep it inside the context. When a user approves a session, the reject button should be disabled and vice versa. If there was one loading state e.g. with an indicator that its an "approval" or a "rejection" or a "disconnection" we could disable all other action buttons. I will implement it in this PR since it would otherwise revert most of the changes done here. Sorry for the back and forth!

@usame-algan
Copy link
Member Author

@francovenica could you test again? The only difference should be that now when pressing "Approve", the "Reject" button should be disabled and when pressing "Reject", the "Approve" button should be disabled.

Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: βœ… success
  • Annotations: 0 total

Report generated by eslint-plus-action

@francovenica
Copy link
Contributor

Yes, works fine for me.

@usame-algan usame-algan merged commit a8904f0 into dev Mar 19, 2024
14 checks passed
@usame-algan usame-algan deleted the wc-loading branch March 19, 2024 12:58
@github-actions github-actions bot locked and limited conversation to collaborators Mar 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[WalletConnect] Add loaders when connecting and signing
3 participants