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

636 check for existing wallet #719

Closed

Conversation

zmjohnso
Copy link
Contributor

@zmjohnso zmjohnso commented Jan 29, 2024

This PR is a fix for #636. When creating a new wallet, the wallet name will now be validated immediately for duplicates, instead of on form submission when the user clicks "Create".

636.mov

Additionally, this PR refactors the getWalletAll api calls to use react router loaders. This should facilitate async data loading as well as reduce extraneous useEffect and useState calls. Using react router loaders/actions while making more use of errorElement's instead of extra useEffect/useState calls feels like a good pattern that could be explored more in the future...

I did do manual testing, but I am not 100% confident I was able to capture all existing functionality, especially in src/components/Wallets.tsx. This could use some extra eyes.

Many thanks to @theborakompanioni for drawing my attention to using react router loaders to fix this bug 🙌

@theborakompanioni theborakompanioni added the bug Something isn't working label Jan 29, 2024
@zmjohnso zmjohnso marked this pull request as ready for review January 30, 2024 03:03
@theborakompanioni
Copy link
Collaborator

Hey @zmjohnso! Thanks so much!

Seems the "import wallet" flow is currently not working (react_router_dom__WEBPACK_IMPORTED_MODULE_6__.useLoaderData() is undefined).

@zmjohnso
Copy link
Contributor Author

Hey @zmjohnso! Thanks so much!

Seems the "import wallet" flow is currently not working (react_router_dom__WEBPACK_IMPORTED_MODULE_6__.useLoaderData() is undefined).

My bad, missed that file when changing the return type of useLoaderData(). Should be good now!

@theborakompanioni
Copy link
Collaborator

So, I have looked over the changes and think it is really nice. If this works well, we should use it more extensively and finally get rid of custom loading indicators per page component.

However, what I noticed is that the requests are often repeatedly invoked. e.g. if you just use the theme toggle (top right corner), you'll see that everytime the button is pressed, multiple "get all wallets" requests are made. I did not investigate too much but maybe you have an idea why that is?

Additionally, if we can rectify this behaviour, "import wallet" should also check for already existing wallet names. 🙌
Hope we can somehow find the root cause of this. 🙌
Do you have any ideas?

@zmjohnso zmjohnso closed this Feb 8, 2024
@zmjohnso zmjohnso deleted the 636-check-for-existing-wallet branch April 30, 2024 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants