Refactor sidebar hooks [SW-647] #4641
Conversation
usame-algan
changed the title
Branch preview✅ Deploy successful! Storybook: |
📦 Next.js Bundle Analysis for safe-wallet-webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 🎉 Global Bundle Size Decreased
DetailsThe 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 If you want further insight into what is behind the changes, give @next/bundle-analyzer a try! One Page Changed SizeThe following page changed size from the code in this PR compared to its base branch:
DetailsOnly 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 Any third party scripts you have added directly to your app using the 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. |
Coverage report
Test suite run success1696 tests passing in 229 suites. Report generated by 🧪jest coverage report action from 03c8136 |
usame-algan
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| })) | ||
| }) | ||
|
|
||
| it('returns an empty array if there is no wallet and allOwned is undefined', () => { |
There was a problem hiding this comment.
Consider implementing the buildSafeItem and prepareAddresses test cases soon to ensure the new functionality is correctly tested. Additionally, review current integration coverage to verify any untested edge cases.
| const addedOnChain = Object.keys(allAdded[chainId] || {}) | ||
| const ownedOnChain = allOwned[chainId] || [] | ||
| const undeployedOnChain = Object.keys(allUndeployed[chainId] || {}) | ||
|
|
There was a problem hiding this comment.
- Use consistent naming conventions for exported functions. Prefixing with an underscore is unconventional and might confuse maintainers.
- Consider memoization for
_prepareAddressesand_buildSafeItemto avoid recomputation on rerender. - Inline logic inside
useAllSafescan be encapsulated into a separate utility function to enhance readability and maintainability.
| const isPinned = Boolean(addedSafe) // Pinning a safe means adding it to the added safes storage | ||
|
|
||
| // Determine if the user is an owner | ||
| const isOwnerFromAdded = addedSafeOwners.some(({ value }) => sameAddress(walletAddress, value)) |
There was a problem hiding this comment.
This is just for CF safes, right?
There was a problem hiding this comment.
Good question. From what I can find is that we are using the isReadOnly flag for two things:
- To show the Add network button
- To show the queue actions
With that in mind I would expect the following behaviour:
CF Safes:
- Show the Add network button if the connected wallet is an owner regardless of pinned state 🔴
- No queue actions required
Owned Safes:
- Show the Add network button regardless of pinned state ✅
- Show the queue actions regardless of pinned state ✅
Added but not owned safes:
- Show neither the Add network button nor the queue actions ✅
Anything I missed? I will implement the CF Safes case
There was a problem hiding this comment.
That behaviour makes sense, thanks for clarifying!
There was a problem hiding this comment.
Good question. From what I can find is that we are using the isReadOnly flag for two things:
It's also used to display the read-only chip for the item in the safe list
|
|
||
| const { result } = renderHook(() => useAllSafes(), { | ||
| initialReduxState: { | ||
| addedSafes: { |
There was a problem hiding this comment.
Consider factoring out repetitive mock object creation in your tests into reusable shared functions to adhere to DRY principles. This will streamline your test setup and improve maintainability, especially if more tests are added in the future.
| '1': ['0x456', '0x789'], | ||
| } | ||
| jest.spyOn(allOwnedSafes, 'default').mockReturnValue([mockOwnedSafes, undefined, false]) | ||
|
|
There was a problem hiding this comment.
Add mockAllUndeployed to the declarations at the start of the test suite if it's used across multiple tests for consistency. Extract repeated configuration and props setups into a helper function to adhere to DRY principles. Check if casting to UndeployedSafe['props'] is necessary.
| export const useHasSafes = () => { | ||
| const { address = '' } = useWallet() || {} | ||
| const allAdded = useAddedSafes() | ||
| const hasAdded = !isEmpty(allAdded) |
There was a problem hiding this comment.
-
Combining
isOwnerFromAddedandisOwnerFromCF: Merge the logic of these two conditions if possible since both check ownership; this reduces redundancy. -
Consistent Naming: Ensure the term "CF" is well-documented or renamed for clarity if it's not a commonly understood term in this context.
-
Function Complexity:
_buildSafeItemcontains several logic checks; consider breaking it into smaller functions for better readability and testability.
| const ownedOnChain = allOwned[chainId] || [] | ||
| const undeployedOnChain = Object.keys(allUndeployed[chainId] || {}) | ||
|
|
||
| const combined = [...addedOnChain, ...ownedOnChain, ...undeployedOnChain] |
There was a problem hiding this comment.
- The initial setting of
allOwned = {}implies it should never beundefined; verify thatuseAllOwnedSafeshandles all cases to ensure reliability. - Consider defining
_prepareAddressesand_buildSafeItemas standalone functions outside the return statement to align with DRY and improve readability.
| const isOwnedSafe = (allOwned[chainId] || []).includes(address) | ||
| const isOwned = isOwnedSafe || isOwnerFromAdded | ||
| const isOwned = isOwnedSafe || isOwnerFromAdded || isOwnerFromCF |
There was a problem hiding this comment.
I think isOwnerFromAdded is not needed anymore. If it's deployed we check ownership from allOwned, if it's CF we check ownership from allUndeployed, so isOwnerFromAdded is redundant
There was a problem hiding this comment.
Very true, nice catch!
# Conflicts: # src/features/myAccounts/hooks/useAllOwnedSafes.ts
What it solves
Part of SW-647
How this PR fixes it
useAllSafesuseAllOwnedSafesas it is not needed anymoreuseAllSafessince it is already done inMyAccountsuseHasSafesinto its own fileToDos
useAllSafesbuildSafeItemprepareAddressesuseAllSafesand split up the logic