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

ENG-194 Fixed the issue with fetching the DAO info #2717

Merged
merged 4 commits into from
Feb 7, 2025

Conversation

johnqh
Copy link
Contributor

@johnqh johnqh commented Feb 6, 2025

Fixed: When a DAO is deployed, 90% of the time we get error retrieving the DAO info because

  1. When we have a promise and it fails, it throws an exception, which means the cache is not reset.
  2. Since the promise is in the requestMap, subsequent request will reuse the same promise. The promise cannot be invoked without resetting, thus causing the same exception to be thrown again.
  3. This means if the first request fails, it will continue to fail no matter how many times we retry.
  4. With the fix, I can see the DAO info is successfully retrieved at 2nd or 3rd try.

Fixed: The toast is not dismissible

  1. It was using loading(...) which is not dismissible.
  2. Change to warning() with Infinite.
  3. With the fix in useSafeAPI, this should almost never happen

Copy link

linear bot commented Feb 6, 2025

@@ -37,7 +37,7 @@ export function SafeCreatePage() {
if (daoFound) {
navigate(DAO_ROUTES.dao.relative(addressPrefix, safeAddress));
} else {
toast.loading(t('failedIndexSafe'));
toast.warning(t('failedIndexSafe'), { duration: Infinity });
Copy link
Contributor

@DarksightKellar DarksightKellar Feb 7, 2025

Choose a reason for hiding this comment

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

My UX concern here is that this isn't a warning per se. It's meant to be more of a re-assuring notice for the user. Suggest you use toast.info instead.

Copy link
Member

@adamgall adamgall Feb 7, 2025

Choose a reason for hiding this comment

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

@johnqh you can do:

toast.loading(t('failedIndexSafe'), { dismissible: true });

edit: nevermind, can't dismissible a loading toast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

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

Agree with Kelvin that we shouldn't change the toast to an error. Otherwise looks good!

@@ -37,7 +37,7 @@ export function SafeCreatePage() {
if (daoFound) {
navigate(DAO_ROUTES.dao.relative(addressPrefix, safeAddress));
} else {
toast.loading(t('failedIndexSafe'));
toast.warning(t('failedIndexSafe'), { duration: Infinity });
Copy link
Member

@adamgall adamgall Feb 7, 2025

Choose a reason for hiding this comment

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

@johnqh you can do:

toast.loading(t('failedIndexSafe'), { dismissible: true });

edit: nevermind, can't dismissible a loading toast.

Copy link

cloudflare-workers-and-pages bot commented Feb 7, 2025

Deploying decent-interface with  Cloudflare Pages  Cloudflare Pages

Latest commit: 78abbb2
Status: ✅  Deploy successful!
Preview URL: https://405e1460.decent-interface.pages.dev
Branch Preview URL: https://feature-eng-194-deployed-mes.decent-interface.pages.dev

View logs

…sn't work. Changed to info(...). I did test that yesterday.
@johnqh johnqh merged commit f8d0ed2 into develop Feb 7, 2025
4 checks passed
@johnqh johnqh deleted the feature/ENG-194-deployed-message branch February 7, 2025 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants