fix: Disable Add to Batch button for safe apps [SW-363] #4623
Conversation
Branch preview✅ Deploy successful! Storybook: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -130,7 +132,7 @@ export const SignForm = ({ | |||
| spacing={{ xs: 2, md: 2 }} | |||
| > | |||
| {/* Batch button */} | |||
| {isCreation && !isBatch && ( | |||
| {isCreation && !isBatch && !isSafeAppTransaction && ( | |||
| <BatchButton | |||
| onClick={onBatchClick} | |||
| disabled={submitDisabled || !isBatchable} | |||
There was a problem hiding this comment.
Ensure the new isSafeAppTransaction variable is meaningful. Its usage is currently limited to a check condition in the render block. Consider consolidating this logic and removing it if it doesn't add clarity or functionality.
📦 Next.js Bundle Analysis for safe-wallet-webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
| const button = getByText('Add to batch') | ||
|
|
||
| expect(button).toBeInTheDocument() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
-
Consider refactoring the repeated
renderlogic into a helper function to adhere to the DRY principle. This reduces redundancy and enhances maintainability. -
The usage of
queryByTextandgetByTextcould lead to inconsistent behavior if the text content changes. Consider using a more robust method like test IDs for querying elements.
Coverage report
Test suite run success1664 tests passing in 226 suites. Report generated by 🧪jest coverage report action from fe87c47 |
| @@ -101,6 +101,8 @@ export const SignForm = ({ | |||
| const submitDisabled = | |||
| !safeTx || !isSubmittable || disableSubmit || cannotPropose || (needsRiskConfirmation && !isRiskConfirmed) | |||
|
|
|||
| const isSafeAppTransaction = !!origin | |||
There was a problem hiding this comment.
I think origin is also non-null in case of WalletConnect txs and probably swaps/staking/bridging as well.
There was a problem hiding this comment.
I think we should hide it for those transactions as well to be on the safe side. At least for swaps the button is already disabled since it is a delegate call:
usame-algan
changed the title
| @@ -133,7 +135,7 @@ export const SignForm = ({ | |||
| {isCreation && !isBatch && ( | |||
| <BatchButton | |||
| onClick={onBatchClick} | |||
There was a problem hiding this comment.
The condition for rendering BatchButton has been modified, potentially affecting its visibility logic. Review if excluding !isSafeAppTransaction in the surrounding condition and adding it to the disabled prop aligns with the desired behavior.
|
Slightly adjusted it so we always show the button but it is disabled to keep it consistent with previous behaviour. |
|
LGTM Checked that dapps don't have the add to batch button. Same for other types of tx like reject tx, swap, stake, bridge.... |
What it solves
Resolves SW-363
How this PR fixes it
BatchButtonif there is anoriginpassed (it is a safe app or walletconnect)How to test it
Checklist