[$500] [HOLD for payment 2023-09-06] Dev - Unable to select a state

[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. Open App ->Settings -> Workspace -> bank Account
2. Enter the required details and proceed to step 2.
3. Attempt to select your state.

Expected Result:

The user is able to select a state.

Actual Result:

The user cannot select a state.

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:
Reproducible in staging?: dev
Reproducible in production?: dev
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-08-26.at.7.07.10.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @Nodebrute
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693058557198979

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017b84ce7e9fc17d03
• Upwork Job ID: 1701792992094031872
• Last Price Increase: 2023-09-13

[melvin-bot]

Triggered auto assignment to @kevinksullivan (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

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @tylerkaraszewski (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Pujan92]

Proposal

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

On typing any char for statepicker the modal gets hide

What is the root cause of that problem?

Seems regression from #25672 where we migrate the BaseModal class component to the function component. Currently, it is doing the cleanup on the dependency changes which should occur on unmounting.

App/src/components/Modal/BaseModal.js

Lines 84 to 94 in c169952

	return () => {
	// Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
	if (isVisible) {
	hideModal(true);
	Modal.willAlertModalBecomeVisible(false);
	}
	// To prevent closing any modal already unmounted when this modal still remains as visible state
	Modal.setCloseModal(null);
	};
	}, [hideModal, isVisible, onClose]);

https://github.com/Expensify/App/pull/25672/files#diff-a766f362da94923822313f0249c8cfaedf93151a08220f529255c40fc67bbf55L54-L63

Reason: Whenever user types in the state textinput, StatePicker will gets rerender and it creates new ref to the hidePickerModal as we haven't used useCallback for it. With this, useEffect dep change triggers and it will call the clean up function which will hide the modal.

Same can be reproducible for CountryPicker in Profile -> Home Address.

App/src/components/StatePicker/index.js

Line 52 in c169952

const hidePickerModal = () => {

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

I think we need to add a new useEffect without any dependency and move this clean up there which acts like a componentWillUnmount.

App/src/components/Modal/BaseModal.js

Line 95 in c169952

     useEffect(() => {
        return () => {
            // Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
            if (isVisible) {
                hideModal(true);
                Modal.willAlertModalBecomeVisible(false);
            }

            // To prevent closing any modal already unmounted when this modal still remains as visible state
            Modal.setCloseModal(null);
        };
    }, []);

• Considering the other issue https://expensify.slack.com/archives/C049HHMV9SM/p1693115903343099 or similar issues where subsequent modal isn't opening, we need to add dependency onModalHide here as the function needs to update the ref when onModalHide updates, otherwise it calls the old onModalHide function and create issues. Bcoz for ThreeDotsMenu and FloatingActionButtonAndPopover we don't use shouldSetModalVisibility which won't recreate the func hideModal in BaseModal component.

App/src/components/Modal/BaseModal.js

Lines 67 to 75 in c029dc5

	onModalHide();
	}
	Modal.onModalDidClose();
	if (!fullscreen) {
	ComposerFocusManager.setReadyToFocus();
	}
	},
	// eslint-disable-next-line react-hooks/exhaustive-deps -- adding onModalHide to the dependency array causes too many unnecessary rerenders
	[shouldSetModalVisibility],

[shouldSetModalVisibility, onModalHide]

Result

Screen.Recording.2023-08-27.at.01.27.50.mov

[hungvu193]

Proposal

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

Dev - Unable to select a state

What is the root cause of that problem?

Problem came from the cleanup function of this effect:

App/src/components/Modal/BaseModal.js

Lines 78 to 94 in c169952

	useEffect(() => {
	Modal.willAlertModalBecomeVisible(isVisible);
	// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
	Modal.setCloseModal(isVisible ? onClose : null);
	return () => {
	// Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
	if (isVisible) {
	hideModal(true);
	Modal.willAlertModalBecomeVisible(false);
	}
	// To prevent closing any modal already unmounted when this modal still remains as visible state
	Modal.setCloseModal(null);
	};
	}, [hideModal, isVisible, onClose]);

If we take a look at the log, we can see the cleanup function run everytime we type something, the problem is our onClose function is re-created everytime the component re-render. According the react docs:
React performs the cleanup when the component unmounts. However, as we learned earlier, effects run for every render and not just once. This is why React also cleans up effects from the previous render before running the effects next time.

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

Solution 1:
Add a new useEffect with only isVisible as dependency for cleanup:

    useEffect(() => {
        return () => {
            // Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
            if (isVisible) {
                hideModal(true);
                Modal.willAlertModalBecomeVisible(false);
            }
            // To prevent closing any modal already unmounted when this modal still remains as visible state
            Modal.setCloseModal(null);
        };
    }, [isVisible]);

Solution 2:
Every time we passed onClose to our modal, we should wrapped it with useCallBack.
For example in this case that hidePickerModal function.

What alternative solutions did you explore? (Optional)

N/A

[b4s36t4]

Proposal

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

Dev - Unable to select a state

What is the root cause of that problem?

This might be a regression from this PR #21049.

The component hierarchy is in this way AddressPage -> CountryPicker -> CountrySelectorModal

If we type anything in the CountrySelectorModal that update will be passed all the way to CountryPicker because of the state we have here

App/src/components/CountryPicker/index.js

Line 37 in c169952

const [searchValue, setSearchValue] = useState(lodashGet(allCountries, value, ''));

.

After the state update tree render would be like this CountryPicker -> CountrySelectorModal

which means all the state in CountrySelectorModal will be wiped off(obsolete and causes re-render as stated in the above proposals) and component Modal get's re-rendered.

Which triggers this

App/src/components/Modal/BaseModal.js

Lines 84 to 93 in c169952

	return () => {
	// Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
	if (isVisible) {
	hideModal(true);
	Modal.willAlertModalBecomeVisible(false);
	}
	// To prevent closing any modal already unmounted when this modal still remains as visible state
	Modal.setCloseModal(null);
	};

that calls the onModalHide.

Kapture.2023-08-27.at.01.03.32.mp4

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

This happens with both CountrySelectorModal and StateSelectorModal because both of them following same kind of principles.

Input is related to the Modal's component, handling the state outside of modal is certainly causing the
re-renders which is leading to close the modal

I think we need to move the state here

App/src/components/CountryPicker/index.js

Line 37 in c169952

const [searchValue, setSearchValue] = useState(lodashGet(allCountries, value, ''));

and here

App/src/components/StatePicker/index.js

Line 41 in c169952

const [searchValue, setSearchValue] = useState(lodashGet(allStates, `${value}.stateName`, ''));

should be moved to individual modal components so that component state is isolated and won't cause any re-renders.

Even this approach is also causing the whole component re-render(only state update) but it's only causing re-render for CountrySelectorModal which doesn't wipe the props which makes doesn't trigger this snippet here

App/src/components/Modal/BaseModal.js

Lines 84 to 93 in c169952

	return () => {
	// Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
	if (isVisible) {
	hideModal(true);
	Modal.willAlertModalBecomeVisible(false);
	}
	// To prevent closing any modal already unmounted when this modal still remains as visible state
	Modal.setCloseModal(null);
	};

What alternative solutions did you explore? (Optional)

NA

Result

Kapture.2023-08-27.at.01.09.21.mp4

[parasharrajat]

Yes, it seems like an regression from #25672. Unless we hear from the author, we can move ahead with solving this issue here. Due to the weekend, it is fine to wait till Monday.

[hayata-suenaga]

sorry people for a lot of assignment and unassignments, but because is a deploy blocker and it's causing issues with other PRs that involve pop over behavior, I gonna handle this 🙇

[Pujan92]

Ok, let me know in case my proposal makes sense and if I can do some help here.

[melvin-bot]

@Pujan92, @kevinksullivan, @allroundexperts, @hayata-suenaga Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@Pujan92, @kevinksullivan, @allroundexperts, @hayata-suenaga Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017b84ce7e9fc17d03

[melvin-bot]

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

[melvin-bot]

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

[kevinksullivan]

Adding label to get upwork job created, not sure why that was skipped before.

[kevinksullivan]

Offers sent to @Pujan92 @Nodebrute and @allroundexperts . Let me know when you accept!

[Pujan92]

Accepted @kevinksullivan, Just to confirm this needs to be paid using the earlier payment price, right?

[kevinksullivan]

Can you elaborate on that @Pujan92 ? what is the earlier payment price?

[Pujan92]

what is the earlier payment price?

@kevinksullivan I am referencing this as this issue seems created before 30th aug.

[kevinksullivan]

Payment summary:

• Reporter - @Nodebrute $250
• reviewer - @Pujan92 $1,500
• reviewer - @allroundexperts $1,500 (newdot)

[allroundexperts]

Is there any speed bonus here @kevinksullivan?

[allroundexperts]

Checklist

1. This is a regression from rewrite BaseModal to a functional component #25672. The authors know this already and we stepped in to fix this earlier than they could.
2. I don't think a Slack discussion is necessary here. Better testing would have caught this in PR review stage.
3. A regression test would be helpful here. The steps mentioned in the issue description look good enough to me.

[allroundexperts]

Bump of #25998 (comment) @kevinksullivan!

[kevinksullivan]

Sorry @allroundexperts I missed that. Updated payment amounts

[kevinksullivan]

Updated payment to @Pujan92 , just need to pay out @allroundexperts

[hayata-suenaga]

@allroundexperts could you accept the offer?

[kevinksullivan]

Asking @JmillsExpensify in DM about the newdot payment.

[JmillsExpensify]

$1,500 payment approved for @allroundexperts based on BZ summary.