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.

Sign up for GitHub

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

Jump to bottom

fix: improve disable condition and tooltip tile on `TxSummary` buttons #1969

Merged

DiogoSoaress merged 8 commits into dev from fix-disable-execute-button

May 12, 2023

Member

DiogoSoaress commented May 9, 2023 •

edited

Loading

What it solves

Resolves #1932

How this PR fixes it

Includes the coreSDK initialization in the condition to disable the <TxSummary> buttons (ExecuteTxButton, RejectTxButton and SignTxbutton)
Improve tooltip description as per the disable condition

How to test it

Not next tx

Go to a Tx queue where a queued tx is fully signed but not the "Next" in the queue
Observe the execution icons is disabled and show a tooltip "Transaction {nonce} must be executed first"

Transaction is pending execution

1. Go to a Tx queue and have 2 transactions ready to be executed
2. Execute the first one
3. Observe the execution icons in the second one is disabled and show a tooltip "Pending transaction must first succeed"

Safe SDK not initialized

Go to a Tx queue where there is a tx queued in the "next" nonce
Refresh the page
Try to hover the Execute/Reject/Sign buttons immediately
Observe they are disabled and show a tooltip 'Waiting for the SDK to initialize'

Screenshots

Screenshot 2023-05-09 at 18 13 36

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

DiogoSoaress added 2 commits

May 9, 2023 17:52


          fix: improve disable condition and tooltip message on ExecuteTxButton

9bd716c


          improve disable condition and tooltip message for RejectTxButton and …

0751ab1

…SignTxButton

DiogoSoaress requested a review from usame-algan

May 9, 2023 16:20

github-actions bot commented May 9, 2023 •

edited

Loading

Branch preview

✅ Deploy successful!

https://fix_disable_execute_button--walletweb.review-wallet-web.5afe.dev

github-actions bot commented May 9, 2023 •

edited

Loading

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

^{Report generated by eslint-plus-action}

usame-algan reviewed

View reviewed changes

src/components/transactions/ExecuteTxButton/index.tsx Outdated Show resolved Hide resolved

src/components/transactions/ExecuteTxButton/index.tsx Outdated Show resolved Hide resolved

src/components/transactions/ExecuteTxButton/index.tsx Outdated Show resolved Hide resolved

iamacook reviewed

View reviewed changes

Member

iamacook left a comment

I've added some comments to the ExecuteTxButton but they stand for each. I'd suggest consulting with design about this.

A simpler solution would be disabling the submit button in the transaction SignOrExecute modal when the SDK is initialising.

src/components/transactions/ExecuteTxButton/index.tsx Outdated

@@ @@ -26,9 +27,19 @@ const ExecuteTxButton = ({ @@
                 const txNonce = isMultisigExecutionInfo(txSummary.executionInfo) ? txSummary.executionInfo.nonce : undefined
                 const isPending = useIsPending(txSummary.id)
                 const { setSelectedTxId } = useContext(ReplaceTxHoverContext)
+                const safeSDK = getSafeSDK()

Member

iamacook May 10, 2023

getSafeSDK will not react to changes in ExternalStore state. We should use the hook when accessing the value.

src/components/transactions/ExecuteTxButton/index.tsx Outdated

+                const tooltipTitle =
+                  isDisabled && !isNext
+                    ? `Transaction ${safe.nonce} must be executed first`

Member

iamacook May 10, 2023

I don't think we need to show this, maybe even the button at all.

src/components/transactions/ExecuteTxButton/index.tsx Outdated

+                  isDisabled && !isNext
+                    ? `Transaction ${safe.nonce} must be executed first`
+                    : isPending
+                    ? `There's a pending transaction`

Member

iamacook May 10, 2023 •

edited

Loading

I would be more descriptive here, e.g.

Suggested change

      
                  ? `There's a pending transaction`
          
                  ? `You cannot execute while a transaction is pending.`

Member Author

DiogoSoaress May 10, 2023

I was trying to juggle between a more complete description and a more succinct one given it's a tooltip. Said that are you still in favor of the longer one?

Member

iamacook May 10, 2023

Compared to the title on line 37, I think it needs more information. An alternative: "Pending transaction must first succeed".

src/components/transactions/ExecuteTxButton/index.tsx Outdated

+                    : isPending
+                    ? `There's a pending transaction`
+                    : !safeSDK
+                    ? 'Waiting for the SDK to initialize'

Member

iamacook May 10, 2023

I'm not sure about this - it's somewhat technical for the "standard" user. It shouldn't happen to too long either.

Member Author

DiogoSoaress May 10, 2023

I've answered in https://github.com/safe-global/safe-wallet-web/pull/1969/files#r1189586069


          use hook instead of getter

a9da970

Member Author

DiogoSoaress commented May 10, 2023

A simpler solution would be disabling the submit button in the transaction SignOrExecute modal when the SDK is initialising.

That is already too late, the error appears when loading the values in that modal. That's why I think we should prevent opening it.

Member

iamacook commented May 10, 2023 •

edited

Loading

That is already too late, the error appears when loading the values in that modal. That's why I think we should prevent opening it.

Could you suppress the error until the SDK has initialised/failed to initialise?

Member Author

DiogoSoaress commented May 10, 2023

You could suppress the error until the SDK has initialised/failed to initialise.

The way we render ErrorMessage would delay any error until the SDK initialized/failed to initialize. But what I meant was that in order to display the tx data in the modal we rely on the SDK being initialized. See

safe-wallet-web/src/components/tx/modals/ConfirmTxModal/ConfirmProposedTx.tsx

Lines 32 to 46 in 3bc22a0

    
           const [safeTx, safeTxError] = useAsync<SafeTransaction>(() => { 
        
             return createExistingTx(chainId, safeAddress, txId) 
        
           }, [txId, safeAddress, chainId]) 
        
           const text = canSign ? (canExecute ? SIGN_EXECUTE_TEXT : SIGN_TEXT) : EXECUTE_TEXT 
        
           return ( 
        
             <SignOrExecuteForm 
        
               safeTx={safeTx} 
        
               txId={txId} 
        
               onSubmit={onSubmit} 
        
               isExecutable={canExecute} 
        
               onlyExecute={!canSign} 
        
               error={safeTxError} 
        
             >

DiogoSoaress added 2 commits

May 10, 2023 11:19


          improve tx pending message

ad5c0de


          extract get tootip title to an util

183e252

DiogoSoaress requested review from usame-algan and iamacook

May 10, 2023 14:34

iamacook approved these changes

View reviewed changes

src/components/transactions/__tests__/utils.test.ts Outdated

+                    const enabledTitle = 'Execute'
+                    expect(getTxButtonTooltip(enabledTitle, disabledPropsBase)).toBe('Execute')
+                    expect(true).toBe(true)

Member

iamacook May 10, 2023

Suggested change

expect(true).toBe(true)

src/components/transactions/__tests__/utils.test.ts Outdated

+                    const disabledProps = { ...disabledPropsBase, isPending: true }
+                    expect(getTxButtonTooltip(enabledTitle, disabledProps)).toBe('Pending transaction must first succeed')
+                    expect(true).toBe(true)

Member

iamacook May 10, 2023

Suggested change

expect(true).toBe(true)

src/components/transactions/__tests__/utils.test.ts Outdated

+                    const disabledProps = { ...disabledPropsBase, hasSafeSDK: false }
+                    expect(getTxButtonTooltip(enabledTitle, disabledProps)).toBe('Waiting for the SDK to initialize')
+                    expect(true).toBe(true)

Member

iamacook May 10, 2023

Suggested change

expect(true).toBe(true)

DiogoSoaress added 2 commits

May 10, 2023 17:12


          improve type and remove leftover code

850ab75


          improve SDK not initialized tooltip message

9f8f7fd

Contributor

francovenica commented May 11, 2023

Issue:
The tx, while is being executed, shows the "Pending tx must succeed first", giving the impression that the button will be available at some point, but actually that button should never be since the tx will go away after being executed.
Or we don't show nothing at all in the tx that is being currently executed
Or we show a "Tx in execution"

Member Author

DiogoSoaress commented May 12, 2023

@usame-algan @francovenica What do you think of changing that tooltip to "Transaction in execution" ? dropping the "must execute first" that can be misleading.
Or, should we simply not show a tooltip when we have a pending tx?

Member

katspaugh commented May 12, 2023

Can we please wrap this up? The amount of discussion and attention this fix receives starts to worry me.

Contributor

francovenica commented May 12, 2023 •

edited

Loading

I think not showing the tooltip in a executing tx is fine. The tx is supposed to disappear anyways after execution


          remove tx is pending tooltip

9985ca4

Contributor

francovenica commented May 12, 2023

I still see tooltips while the tx is executing, maybe because the status is not "pending" but "processing" or "relaying".

Still I don't think is that big of a deal. I think we can leave it this way until we get feedback about it.

Member Author

DiogoSoaress commented May 12, 2023

I still see tooltips while the tx is executing

These tooltips ("Execute", "Replace", "Confirm") were already in place and were not affected by this bug fix/improvement.

Contributor

francovenica commented May 12, 2023

Yes, I think we are done with this ticket. I'll pass it

DiogoSoaress merged commit 08077c2 into dev

DiogoSoaress deleted the fix-disable-execute-button branch

May 12, 2023 14:58

github-actions bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet