feat: Improve safe creation status screen

usame-algan commented 1 month ago

What it solves

Resolves #3612

How this PR fixes it

Removes the timeout for safe creation transactions
Only navigates to the status screen after the user submits the safe creation in their wallet
Renames the Cancel button to Back to homepage
Renames the Retry button to Try again
Navigates the user to the safe creation review screen when they press Try again
Adds a new screen for when the transaction has failed
Removes the StatusStepper
Removes useSafeCreation
Removes useSafeCreationEffects
Shows a speed-up message if the transaction takes longer than 15 seconds
Removes CreationModal and instead shows the new Success screen with the address attached

ToDos

[x] Need to figure out how to select the correct pending safe creation as there can be two now with counterfactual
[x] Still pass safeViewRedirectURL on SUCCESS
[x] Navigate user to dashboard on SUCCESS
[x] Check that analytics events are still mapped correctly
[x] Add safe (+ owners) to address book on SUCCESS

How to test it

create a safe through a safe
the user should only proceed to the status screen once they submit the tx in their wallet

Screenshots

https://github.com/safe-global/safe-wallet-web/assets/5880855/19a1c895-2749-4a7e-b077-86abb1ea15af

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) 🧑‍💻

github-actions[bot] commented 1 month ago

Branch preview

✅ Deploy successful!

Website: https://create_safe_status--walletweb.review.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

Storybook: https://create_safe_status--walletweb.review.5afe.dev/storybook/

github-actions[bot] commented 1 month ago

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: :white_check_mark: success
- Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions[bot] commented 1 month ago

Coverage report

St.^{:grey_question:}	Category	Percentage	Covered / Total
🟡	Statements	79% (-0.16% 🔻)	11317/14325
🔴	Branches	58.26% (-0.18% 🔻)	2740/4703
🟡	Functions	66% (-0.07% 🔻)	1821/2759
🟢	Lines	80.37% (-0.12% 🔻)	10198/12689

Show new covered files 🐣

|

St.^{:grey_question:}

| File | Statements | Branches | Functions | Lines | | :-: | :- | :- | :- | :- | :- | | 🟢 |

`...` / useUndeployedSafe.ts

| 100% | 100% | 100% | 100% |

Show files with reduced coverage 🔻

|

St.^{:grey_question:}

| File | Statements | Branches | Functions | Lines | | :-: | :- | :- | :- | :- | :- | | 🟢 |

`...` / safeCoreSDK.ts

|

89.09% (-3.5% 🔻)

| 84.62% |

85.71% (-14.29% 🔻)

|

91.67% (-1.95% 🔻)

| | 🟡 |

`...` / notificationsSlice.ts

|

76.67% (-3.33% 🔻)

| 16.67% |

46.15% (-7.69% 🔻)

|

72.73% (-4.55% 🔻)

| | 🟡 |

`...` / gtm.ts

|

70.77% (-1.54% 🔻)

| 60% |

50% (-12.5% 🔻)

| 77.78% | | 🟢 |

`...` / txMonitor.ts

|

92% (-0.47% 🔻)

|

94.44% (+2.44% 🔼)

|

95.45% (-0.55% 🔻)

|

92.96% (-0.22% 🔻)

| | 🔴 |

`...` / utils.ts

|

36.52% (-0.65% 🔻)

|

34.38% (-1.11% 🔻)

| 21.43% |

37% (-0.76% 🔻)

| | 🟡 |

`...` / index.tsx

|

66.67% (-9.86% 🔻)

|

41.86% (-11.26% 🔻)

|

61.54% (-11.19% 🔻)

|

66.67% (-10.51% 🔻)

| | 🔴 |

`...` / index.ts

|

47.83% (-21.85% 🔻)

|

18.18% (-40.91% 🔻)

|

10% (-37.06% 🔻)

|

48.33% (-20.53% 🔻)

|

Test suite run success

1409 tests passing in 195 suites.

Report generated by 🧪jest coverage report action from ce37ab43ae4569a973980e6ddc5b16b169091dbe

github-actions[bot] commented 1 month ago

📦 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`	`999.52 KB` (🟢 -4.4 KB)

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!

Eighteen Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page	Size (compressed)	First Load
`/apps/open`	`49.79 KB` (🟡 +1.08 KB)	1.02 MB
`/balances`	`29.43 KB` (-1 B)	1 MB
`/home`	`55.07 KB` (🟢 -1.35 KB)	1.03 MB
`/new-safe/create`	`33.82 KB` (🔴 +7.24 KB)	1.01 MB
`/new-safe/load`	`16.45 KB` (🟡 +62 B)	1015.97 KB
`/settings/appearance`	`8.02 KB` (🟢 -432 B)	1007.54 KB
`/settings/data`	`7.54 KB` (🟢 -1 B)	1007.06 KB
`/settings/modules`	`9.62 KB` (🟡 +1 B)	1009.14 KB
`/settings/notifications`	`27.62 KB` (🟢 -463 B)	1 MB
`/settings/safe-apps`	`21.99 KB` (🟢 -415 B)	1021.51 KB
`/settings/security`	`8.06 KB` (🟢 -413 B)	1007.58 KB
`/settings/setup`	`35.95 KB` (-1 B)	1.01 MB
`/swap`	`26.49 KB` (🟡 +143 B)	1 MB
`/transactions`	`73.74 KB` (🟢 -910 B)	1.05 MB
`/transactions/history`	`73.71 KB` (🟢 -909 B)	1.05 MB
`/transactions/messages`	`37.83 KB` (🟡 +2.25 KB)	1.01 MB
`/transactions/queue`	`30.94 KB` (🟡 +1.08 KB)	1.01 MB
`/transactions/tx`	`20.71 KB` (🟡 +1.08 KB)	1020.23 KB

Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

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

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

github-actions[bot] commented 1 month ago

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: :white_check_mark: success
- Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions[bot] commented 1 month ago

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: :white_check_mark: success
- Annotations: 0 total

^{Report generated by eslint-plus-action}

francovenica commented 1 month ago

This Scroll bar shoudn't be there

francovenica commented 1 month ago

If the user reject the tx we show an error message saying that "Something went wrong and it should try again later". For tx executions in a safe we say that "the user rejected the tx". Can't we do the same here?

francovenica commented 1 month ago

When the tx fails it says that the tx "was rejected by the connected wallet". I think this makes belive that the user rejected the tx on purpose, and not that the tx failed for some reason outside of the user's control. I think it should just say that the tx has failed without the "rejected by the wallet" text.

In this snapshot I, on purpose, set the gas limit too low to make the tx fail:

francovenica commented 1 month ago

Beyond the comments I made, the rest looks fine.

Regarding this comment in the Notion doc: "We remove the status page and instead navigate the user to their safe even while its deploying. This is already how its done for Counterfactual safes and would unblock the UI for the user. We could show the deployment status as a sticky notification."

I'm not 100% in removing the "creation done" modal that used to show what the user can do with the safe and also not fully sold about dumping the user into the safe as it is being deployed. I don't think the user will start to navigate the UI as it sees that the safe is deploying with all those loaders.

My suggestion would be to show a modal saying "your safe is being deployed, it might take a few minutes, on the meanwhile you can still explore it". And if the safe is deployed then you show the "Your safe has been deployed: [ADDRESS]" modal.

usame-algan commented 4 weeks ago

@francovenica thanks for testing! I pushed a fix to address the issues you found. Could you try again?

I'm not 100% in removing the "creation done" modal that used to show what the user can do with the safe and also not fully sold about dumping the user into the safe as it is being deployed. I don't think the user will start to navigate the UI as it sees that the safe is deploying with all those loaders.

My suggestion would be to show a modal saying "your safe is being deployed, it might take a few minutes, on the meanwhile you can still explore it". And if the safe is deployed then you show the "Your safe has been deployed: [ADDRESS]" modal.

You are talking about this screen right? We should document why we want to remove it. cc @TanyaEfremova From my understanding it overloads the user with information about what they can do with their Safe while also blocking them from those actions and essentially forcing them to do an onboarding which can be frustrating.

Removing this dialog also affects one of our e2e tests (smoke/create_safe_cf) so we would have to remove dialogConfirmBtn and associated logic. cc @mike10ca

github-actions[bot] commented 4 weeks ago

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: :white_check_mark: success
- Annotations: 0 total

^{Report generated by eslint-plus-action}

TanyaEfremova commented 4 weeks ago

I think, we agreed to show the success screen with the account address and a welcoming text instead of this one. I don't think, the current screen provides a lot of error. We also have another success screen for CF accounts, which is completely different from this one, so they need to be aligned anyway, there is no reason in keeping both.

francovenica commented 3 weeks ago

The issues I reported were fixed

We also have another success screen for CF accounts, which is completely different from this one, so they need to be aligned anyway, there is no reason in keeping both.

So right now the only " success screen" is this one below. CF don't have a success screen for themselves, only the same one once it is deployed, so technically there is only the "Safe deployed success screen". Is there another one I'm not aware of? or it is planned for another ticket?

If that is the only success screen that should show up for the scope of this ticket then is ok to be closed and merged

TanyaEfremova commented 3 weeks ago

Yes, exactly! Before, we had a modal with the overview, when an account was created, then once the CF account is deployed a different modal was shown, and the overview wasn't. So we would like to merge them, and only show the one you posted on the screenshot :) So you are correct.

francovenica commented 3 weeks ago

Ok, Given Tanya's comment and a talk I had with Usame I see that this success screen is the only one and correct. I was afraid I was missing some success screen somewhere

LGTM, we can pass this ticket to done

github-actions[bot] commented 2 weeks ago

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: :white_check_mark: success
- Annotations: 0 total

^{Report generated by eslint-plus-action}

safe-global / safe-wallet-web