fix: CheckWallet sdk only if safe is also loaded #4637
Conversation
Branch preview✅ Deploy successful! Storybook: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -54,7 +54,7 @@ const CheckWallet = ({ | |||
| if (!wallet) { | |||
| return Message.WalletNotConnected | |||
| } | |||
| if (!sdk) { | |||
| if (!sdk && safeLoaded) { | |||
| return Message.SDKNotInitialized | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The condition !sdk && safeLoaded in getErrorMessage could delay error messaging if safeLoaded is not immediately true when sdk is unavailable. Ensure safeLoaded correctly represents when readiness should be checked, or consider decoupling them for clarity.
📦 Next.js Bundle Analysis for safe-wallet-webThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
1 MB (🟡 +10 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!
Coverage report
Test suite run success1676 tests passing in 228 suites. Report generated by 🧪jest coverage report action from a211097 |
|
|
||
| expect(queryByText('Continue')).not.toBeDisabled() | ||
| }) | ||
|
|
||
| it('should allow nested Safe owners', () => { | ||
| ;(useIsSafeOwner as jest.MockedFunction<typeof useIsSafeOwner>).mockReturnValueOnce(false) | ||
| mockUseNestedSafeOwners.mockReturnValue([faker.finance.ethereumAddress()]) |
There was a problem hiding this comment.
The test descriptions are inconsistent in mentioning the conditions for enabling/disabling the button. Consider standardizing the naming pattern, e.g., "disables button when SDK uninitialized and safe loaded" vs. "doesn't disable button when SDK uninitialized and safe not loaded."
| @@ -44,7 +44,7 @@ const CheckWallet = ({ | |||
| const sdk = useSafeSDK() | |||
| const isProposer = useIsWalletProposer() | |||
|
|
|||
| const { safe } = useSafeInfo() | |||
| const { safe, safeLoaded } = useSafeInfo() | |||
|
|
|||
| const isNestedSafeOwner = useIsNestedSafeOwner() | |||
There was a problem hiding this comment.
Evaluate the need to include safeLoaded in the dependency array. If safeLoaded is a state that changes, it should be included if checkNetwork depends on it; otherwise, remove it to prevent unnecessary re-renders.
I wonder why we use Added here: #4655 |
|
LGTM You can disable notifcations from the global settings |
What it solves
Resolves #4636
How this PR fixes it
safeLoadedwhen evaluating if the sdk exists or not inCheckWalletsince the sdk won't exist if there is no safe openedHow to test it
This should not break existing behaviour for transaction related buttons
Screenshots
Checklist