[HOLD for payment 2023-07-05] [$1000] unlink button appears as disable but able to click for second time

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. go to Settings > Profile > Contact methods
2. click on New contact method button
3. add userB as New contact method
4. try to login with userB in other tab
5. click on unlink button
6. try to click on disable unlink button

Expected Result:

user should not be able to click the unlink button

Actual Result:

user able to click disable button

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.29-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-06-09.at.5.59.53.PM.mov

Recording.1006.mp4

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686314130397609
View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01343ca85e7226e7d6
• Upwork Job ID: 1671681149798367232
• Last Price Increase: 2023-06-22

[melvin-bot]

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[AmjedNazzal]

Proposal

Please re-state the problem that we are trying to solve in this issue.

unlink button appears as disable but able to click for second time

What is the root cause of that problem?

The problem is related to the timing of the evaluation and the values being used for the disabled prop. upon logging in the children components it seem like the disabled value keeps changing between true and false depending on the value of isLoading and even when flipping isDisabled and isLoading the behaviour is still inconsistent possibly due to the use of the OR || operator in this case.

App/src/components/Button/index.js

Line 276 in 862955c

disabled={this.props.isLoading || this.props.isDisabled}

What changes do you think we should make in order to solve the problem?

We can change the approach to using a function with a more custom and better performing behaviour thus elemenating the unexpected behaviour of the OR || operator

disabled={() => {
    if(this.props.isDisabled){
        return true;
    }
    if(this.props.isLoading){
        return true;
    } 
}}

Uppon logging and testing with this approach the isLoading seem to disable the button when there's actually loading and isDisabled is disabling the button whenever it's true.

Result

Screen.Recording.2023-06-14.at.2.48.16.PM.mov

[namhihi237]

Proposal

Please re-state the problem that we are trying to solve in this issue.

user should not be able to click unlink button or button should not be show as disable

What is the root cause of that problem?

The root cause of the problem happens https://github.com/Expensify/App/blob/main/src/components/Pressable/PressableWithFeedback.js#L45-L72.

The problem here InteractionManager.runAfterInteractions will update isDisable after we press the button. But In the code, when passing props.disabled to the runAfterInteractions function, the value of props.disabled at that point will be remembered and used in the runAfterInteractions callback.

However, if the value of props.disabled changes after the runAfterInteractions function has been called, the new value is not automatically updated in the runAfterInteractions callback. This is because JavaScript works on a closure mechanism, where variables stored in a closure are not automatically updated when their initial value changes.

So after the first click it still set false for setIsDisable

What changes do you think we should make in order to solve the problem?

We check the function runAfterInteractions can see done function.

runAfterInteractions(task?: (() =>  any) | SimpleTask | PromiseTask): {
	then: (onfulfilled?: () =>  any, onrejected?: () =>  any) =>  Promise<any>;
	done: (...args: any[]) =>  any;
	cancel: () =>  void;
};

The solution would be to make sure the InteractionManager.runAfterInteractions is done and then setDisabled in useEffect

const interactionTask  =  useRef();

interactionTask.current = InteractionManager.runAfterInteractions((...))

Update useEffect make sure order is run correctly.:

useEffect(() => {
	if (!interactionTask.current){
	        setDisabled(props.disabled));
		return;
	}
	interactionTask.current.done(()=> setDisabled(props.disabled));
}, [props.disabled]);

Result:

Screen.Recording.2023-06-18.at.21.05.16.mov

What alternative solutions did you explore? (Optional)

Solution 2: To use the new value of props.disabled in the runAfterInteractions callback, you can do it another way. Instead of storing the props.disabled value in a temporary variable, you can use a ref to keep track of the props.disabled value. When the props.disabled value changes, the ref is automatically updated and used in the runAfterInteractions callback.

const disabledRef = useRef(props.disabled);

useEffect(() => {
        console.log(‘useEffect’, props.disabled);
        setDisabled(props.disabled);
        disabledRef.current = props.disabled; // add here
    }, [props.disabled]);

And in runAfterInteractions(() we will update the param for setIsDisabled:

setDisabled(disabledRef.current);

Solution 3:
If we want to pass the latest value from props.disabled to the runAfterInteractions callback, a callback function (closure) can be used to ensure that the latest value is used.

 const runAfterInteractionsCallback = useCallback((e) => {
        const onPress = props.onPress(e);
        if (!(onPress instanceof Promise)) {
            setDisabled(props.disabled);
            return;
        }
        onPress.finally(() => {
            setDisabled(props.disabled);
        });
      }, [props.disabled]);

InteractionManager.runAfterInteractions(runAfterInteractionsCallback(e));

[hungvu193]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Unlink button appears as disable but able to click for second time

What is the root cause of that problem?

We're using GenericPressable to wrap the Button, and even the state of the button is disabled, it's still allowed to call onPress, that why you can see after button is disabled we still can press button.

What changes do you think we should make in order to solve the problem?

We should add an early return here inside onPress function of GenericPressable. If the Button is already disabled, we do nothing.

        <GenericPressable
            ref={ref}
            style={props.wrapperStyle}
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...propsWithoutStyling}
            disabled={disabled}
            onPress={(e) => {
           // early return here
               if (props.isDisabled) {
                 return;
             }
                setDisabled(true);
                const onPress = props.onPress(e);
                InteractionManager.runAfterInteractions(() => {
                    if (!(onPress instanceof Promise)) {
                        setDisabled(props.disabled);
                        return;
                    }
                    onPress.finally(() => {
                        setDisabled(props.disabled);
                    });
                });
            }}

What alternative solutions did you explore? (Optional)

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We can still press once more a "disabled" Unlink button

What is the root cause of that problem?

The button use a disabled style but not actually disabled.

The Unlink button uses a Button component that internally uses PressableWithFeedback. Unlink button will be disabled when we either offline or a message exists (in this case the Link sent message).

App/src/pages/signin/UnlinkLoginForm.js

Lines 87 to 93 in bee8fd3

	<Button
	medium
	success
	text={props.translate('unlinkLoginForm.unlink')}
	isLoading={props.account.isLoading && props.account.loadingForm === CONST.FORMS.UNLINK_LOGIN_FORM}
	onPress={() => Session.requestUnlinkValidationLink()}
	isDisabled={props.network.isOffline || !_.isEmpty(props.account.message)}

And in Button, it will be disabled if either isLoading or isDisabled is true.

App/src/components/Button/index.js

Line 276 in bee8fd3

disabled={this.props.isLoading || this.props.isDisabled}

Everything works fine here. The problem is in PressableWithFeedback. In PressableWithFeedback, we have a logic to disable the button until the onPress (if a Promise) is done. This is done to solve multiple press issue like this #14572

App/src/components/Pressable/PressableWithFeedback.js

Lines 56 to 68 in bee8fd3

	onPress={(e) => {
	setDisabled(true);
	const onPress = props.onPress(e);
	InteractionManager.runAfterInteractions(() => {
	if (!(onPress instanceof Promise)) {
	setDisabled(props.disabled);
	return;
	}
	onPress.finally(() => {
	setDisabled(props.disabled);
	});
	});
	}}

As soon as we press a button, it will set the disabled state to true. When it's done, it will set back the disabled state withprops.disabled. The issue is, every value reference in a function is captured and will never change. So, when either runAfterInteractions or the promise is done, it will call setDisabled with the value of props.disabled that is captured when onPress is called. This could potentially having an old value of props.disabled, which what happen in our case.

Also, we have a useEffect that will run every time props.disabled changes

App/src/components/Pressable/PressableWithFeedback.js

Lines 45 to 47 in bee8fd3

	useEffect(() => {
	setDisabled(props.disabled);
	}, [props.disabled]);

But this doesn't help.

This is the scenario in our case:
props.disabled initial value is false, so disabled is also false

1. User press Unlink. onPress captures props.disabled current value which is false.
2. setDisabled is called with true.
   props.disabled: false, disabled: true
3. The unlink request loading happen
   props.disabled: true, disabled: true
4. runAfterInteractions is done, setDisabled is called with the captured props.disabled which is false.
   props.disabled: true, disabled: false
5. Unlink request is done with the Link sent message.
   props.disabled: true, disabled: false

What changes do you think we should make in order to solve the problem?

I think the current code is kinda confusing, so I propose to modify it a little bit.

1. Rename disabled state to isPressed
2. Add a new variable called disabled that use useMemo. It will return props.disabled || isPressed.
   const disabled = useMemo(() => props.disabled || isPressed, [props.disabled, isPressed]);
3. When onPress is called, we will still call setPressed to true, but when it's done, we will simply call setPressed with false.

The new disabled is now a value that depends on both prop.disabled and isPressed. In the current implementation, we have a single state that handle both props and the press logic thing.

[trjExpensify]

This is reproducible. Though the expected result in the OP is a bit ambiguous as written:

user should not be able to click unlink button or button should not be show as disable

@gadhiyamanan I think it's better to confirm expected behaviour for your bug reports versus state them as "either or" to cover bases.

@mananjadhav @s77rt @roryabraham I feel like you guys have some context on this PressableWithFeedback stuff from #14572 as well. In this scenario, I presume the button should not be able to be clicked. Can you confirm, and then I'll update the OP and move it on.

[trjExpensify]

^^ bump on the above please guys so we can keep this moving :)

[s77rt]

@trjExpensify Oops, so sorry didn't see this until now. Feel free to DM or tag me in Slack next time. For the expected behaviour, yes you are right the button should not be clickable. For the RCA, I think both @namhihi237 and @bernhardoj provided the correct RCA. But not sure what solution to go with yet. Also I think this bug exists on OptionRow too since that logic is brought from there, so let's make sure we fix that as well.

[trjExpensify]

Okay, excellent. I'm going to update the OP's expected behaviour and apply the label. You want to be C+ on this one given you're here already 😂?

[s77rt]

That would be great 😁

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01343ca85e7226e7d6

[melvin-bot]

Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Current assignee @s77rt is eligible for the External assigner, not assigning anyone new.

[s77rt]

@AmjedNazzal Thanks for the proposal. I don't think your RCA is correct. I don't see how the OR operator is the issue here.

[s77rt]

@namhihi237 Thanks for the proposal. Your RCA is correct. I like the first alternative solution (use of disabledRef) since it's based on the current working useEffect that relies on props.disabled. Do you think we can optimise this more or make it more clear?

[s77rt]

@hungvu193 Thanks for the proposal. I don't think you RCA is correct. This kind of early return is already implemented in GenericPressable.

[namhihi237]

Thanks, @s77rt , Currently, I haven't figured out how to further optimize it. I think To clarify we can leave a comment.

[s77rt]

@bernhardoj Thanks for the proposal. Your RCA is correct. I like the proposed solution and the clean up. Can you please apply #20838 (comment) and your solution and confirm that #14572 is still fixed (especially on Android)

[s77rt]

BTW, the issue in not reproducible on main due to another unwanted change. More info #20838 (comment). Once this is resolved the bug would be reproducible again and we can go ahead with a proposal.

[s77rt]

@namhihi237 Thanks for the quick follow up. For now I think we need to wait on @bernhardoj confirmation on his proposed solution then we can see how to move forward.

[bernhardoj]

Yes, applying the changes from #20838 (comment) will make the issue reproducible again, and applying my fix still works. But I see we already have isPressed state, maybe we can rename it to isPressedIn?

[melvin-bot]

Triggered auto assignment to @johnmlee101, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[johnmlee101]

makes sense 👍

[melvin-bot]

📣 @bernhardoj You have been assigned to this job by @johnmlee101!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

PR is ready

cc: @s77rt

[melvin-bot]

🎯 ⚡️ Woah @s77rt / @bernhardoj, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @bernhardoj got assigned: 2023-06-22 19:51:13 Z
• when the PR got merged: 2023-06-26 15:34:02 UTC

On to the next one 🚀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.33-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix Unlink button can be clicked once again after get disabled #21373

If no regressions arise, payment will be issued on 2023-07-05. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: Disables the Button during onPress call in PressableWithFeedback #18122
• The offending PR has been commented on: Disables the Button during onPress call in PressableWithFeedback #18122 (comment)
• A discussion in #expensify-bugs has been started: https://expensify.slack.com/archives/C049HHMV9SM/p1688042057261969
• Determine if we should create a regression test for this bug: No

[trjExpensify]

Thanks, @s77rt!

Ignoring this timeline recap, it didn't account for the weekend.

Following offers have been sent:

• $1,500 to @s77rt for C+ review
• $1,500 to @bernhardoj for the fix
• $250 to @gadhiyamanan for the bug report.

[s77rt]

@trjExpensify Accepted! Thanks!

BTW, the timeline seems correct, isn't it?

[trjExpensify]

Ah, my eyes are playing tricks on me.. I read it as not being eligible. 😂 I'd applied the bonus anyhow.

Also, settled up with you after accepting - thanks!

[trjExpensify]

Paid @bernhardoj, waiting on @gadhiyamanan to accept the offer.

[trjExpensify]

Paid @gadhiyamanan, closing!