fix:blank page appears in validate code form page #55588

jacobkim9881 · 2025-01-22T03:22:58Z

Explanation of Change

This PR replaces <ValidateCodeActionModal> with <ValidateCodeActionForm> in ContactMethodDetailsPage as it creates <ValidateCodeActionForm> component. This component doesn't have HeaderWithBackButton or Modal so it can be used in any page that needs validate code form but it can uses props for <ValidateCodeActionModal>.

Fixed Issues

$ #53884
PROPOSAL:$ #53884 (comment)

Tests

Verify that no errors appear in the JS console

Preparation

Go to "Settings" bottom tab -> Profile -> ContactMethod.
Click "New contact method" at bottom in RHP.
Put Email address and click "Add".
Put numbers to validate the email.

Test

Click one of contact methods from Preparation 4.
Check there is blank modal briefly.

Offline tests

Preparation

Go to "Settings" bottom tab -> Profile -> ContactMethod.
Click "New contact method" at bottom in RHP.
Put Email address and click "Add".
Put numbers to validate the email.

Test

Click one of contact methods from Preparation 4.
Check there is blank modal briefly.

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."

Verify that no errors appear in the JS console

Preparation

Go to "Settings" bottom tab -> Profile -> ContactMethod.
Click "New contact method" at bottom in RHP.
Put Email address and click "Add".
Put numbers to validate the email.

Test

Click one of contact methods from Preparation 4.
Check there is blank modal briefly.

PR Author Checklist

Screenshots/Videos

Android: Native

expensify-test15-2025-01-29_05.23.39.mp4

Android: mWeb Chrome

expensify-test16-2025-01-29_05.28.07.mp4

iOS: Native

2025-01-29.5.18.03.mov

iOS: mWeb Safari

2025-01-29.5.19.48.mov

MacOS: Chrome / Safari

expensify-test14-2025-01-29_05.12.06.mp4

MacOS: Desktop

2025-01-29.5.30.16.mov

This reverts commit c4e944f.

This reverts commit 6a12518.

jacobkim9881 · 2025-01-28T06:22:33Z

@hoangzinh Another issue from other branch bother testing. Should we handle this bug too?(If we don't then we can't test on iOS)

2025-01-28.3.18.24.mov

Process:

Click default contact method or validated one.
Navigate back and click the one again.
Can not enter contact method.

jacobkim9881 · 2025-01-28T06:30:49Z

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx

Lines 171 to 173 in 433778f

    
           useEffect(() => { 
        
               resetContactMethodValidateCodeSentState(contactMethod); 
        
           }, [contactMethod]);

This line makes this issue

cc: @nkdengineer

melvin-bot · 2025-01-28T20:36:50Z

@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

hoangzinh · 2025-01-29T14:38:00Z

@jacobkim9881 is it reproducible if we click on "unverified" contact method in the 1st step?

jacobkim9881 · 2025-01-29T21:01:16Z

@hoangzinh It isn't reproducible if clicking 1st time so it isn't unrelated with the PR. Thanks for clarifying!

hoangzinh · 2025-01-30T01:11:31Z

src/components/ValidateCodeActionWithoutModal.tsx

+
+type ValidateCodeActionProps = ValidateCodeActionModalProps & ValidateCodeActionWithoutModalProps;
+
+function ValidateCodeActionWithoutModal({


Do you have any other names for this component? It seems is not a good name.

I have updated it to ValidateCodeActionForm.

Since it shares ValidateCodeActionModalProps, I think ValidateCodeAction should be included. It has the form element and has props to run actions. I think ValidateCodeActionForm can be properly used. Please to tell me if you have any idea though.

hoangzinh · 2025-01-30T01:14:13Z

src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx

-            <ScrollView keyboardShouldPersistTaps="handled">
+            <ScrollView
+                keyboardShouldPersistTaps="handled"
+                contentContainerStyle={themeStyles.flex1}


can you elaborate why do we need this style?

This style makes put "Verify" button to bottom. W/o the style the button will be placed beneath validate code form.

ping @jacobkim9881 again ^

The elements can fit the necessary space. The explain, form and button elements are placed in its position with this style.

hoangzinh · 2025-01-30T01:18:46Z

src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx

+                {!loginData?.validatedDate && (
+                    <ValidateCodeActionWithoutModal
+                        hasMagicCodeBeenSent={hasMagicCodeBeenSent}
+                        isVisible={isValidateCodeActionModalVisible && !loginData.validatedDate && !!loginData}


I think we should control the visibility of this component in line 323 instead of a prop. What do you think?

Before it is controled with isVisible in <ValidateCodeActionModal>. I think it should be controlled in <ValidateCodeActionWithoutModal>.

Maybe like this?

Before it is controled with isVisible in

App/src/components/ValidateCodeActionModal/index.tsx

Line 61 in 83aae80

isVisible={isVisible}

hoangzinh · 2025-01-30T01:19:27Z

src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx


-                {!isValidateCodeActionModalVisible && getMenuItems()}
+                {!isValidateCodeActionModalVisible && !!loginData?.validatedDate && getMenuItems()}


Can you elaborate why do we need add an extra condition here?

W/o it, trash can will show when navigating out on Android mWeb.

expensify-test19-2025-02-02_12.45.08.mp4

hoangzinh · 2025-01-30T01:22:07Z

src/components/ValidateCodeActionWithoutModal.tsx

+    useEffect(
+        () => () => {
+            firstRenderRef.current = true;
+        },
+        [],
+    );
+
+    useEffect(() => {
+        if (!firstRenderRef.current || !isVisible || hasMagicCodeBeenSent) {
+            return () => {
+                clearError();
+            };
+        }
+        firstRenderRef.current = false;
+        sendValidateCode();
+    }, [isVisible, sendValidateCode, hasMagicCodeBeenSent, clearError]);


If it's not a modal. Do we need firstRenderRef or isVisible anymore?

W/o it, it will send validate code in validated contact method or default method. It only sends validate code in validate code page only.

hoangzinh · 2025-02-04T13:55:55Z

src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx

+                focusTrapOptions: isMobileSafari()
+                    ? undefined


@jacobkim9881 will it cause any issue if we apply it for mweb Safari too?

It bothers keyboard showing on mWeb Safari:

REC-20250205094156.mp4

On mWeb Safari, number pad shows at contact method page. It is because of onActive event fired by focus trap. Number pad gets ready for putting numbers on the validate form. On the screen, contact method page is seen still but focusing is on validate form. As a result before transition animation shows, number pad shows earlier.

hoangzinh · 2025-02-04T13:56:18Z

src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx

-            <ScrollView keyboardShouldPersistTaps="handled">
+            <ScrollView
+                keyboardShouldPersistTaps="handled"
+                contentContainerStyle={themeStyles.flex1}


ping @jacobkim9881 again ^

hoangzinh · 2025-02-04T14:00:28Z

src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx

-                <ValidateCodeActionModal
-                    title={formattedContactMethod}
-                    onModalHide={() => {}}
+                <ValidateCodeActionForm


Suggested change

<ValidateCodeActionForm

{isValidateCodeFormVisible && !loginData.validatedDate && !!loginData && <ValidateCodeActionForm

Why don't we display/hide ValidateCodeActionForm here? But we prefer to pass isVisible prop into ValidateCodeActionForm?

We have used isVisible for the modal though.

jacobkim9881 · 2025-02-05T01:47:05Z

Oops, pardon for my mistake!

hoangzinh · 2025-02-05T09:15:26Z

@jacobkim9881 please let me know when the PR is ready for next review

jacobkim9881 · 2025-02-05T10:31:25Z

@hoangzinh I have replied and the PR is ready for next review.

hoangzinh · 2025-02-05T10:57:17Z

@jacobkim9881 could you check review feedbacks again? I couldn't find answer of #55588 (comment) and #55588 (comment)

jacobkim9881 · 2025-02-05T14:12:19Z

I just added answers. Please to review.

hoangzinh · 2025-02-05T15:23:52Z

It's weird. I can't see your answers.

hoangzinh · 2025-02-05T15:24:44Z

src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx

+                {isValidateCodeFormVisible && !loginData.validatedDate && !!loginData && (
+                    <ValidateCodeActionForm
+                        hasMagicCodeBeenSent={hasMagicCodeBeenSent}
+                        isVisible={isValidateCodeFormVisible && !loginData.validatedDate && !!loginData}


@jacobkim9881 Do we need this prop anymore? 👀

We need isVisble prop for executing clear function inside. For example, when navigating out from the validating code page, it clears validating error or other errors with RBR.

App/src/components/ValidateCodeActionForm.tsx

Lines 47 to 51 in 0e4b20f

if (!firstRenderRef.current || !isVisible || hasMagicCodeBeenSent) {

return () => {

clearError();

};

}

I think it always works without isVisible there. What do you think? Can you try to remove isVisible condition check and test again?

I think it will works without isVisible too since clear function works when the component is closed.

Can you try to remove isVisible condition check and test again?

It works without isVisible but I found an issue. Whenever opening default contact method, the clear function is executed so to run clearError() too. Clear function and clearError should run in validate code page only. Let me update the PR.

There was an exception. I checked clear function runs when <ValidateCodeActionForm> unmounted by line 321. It was on mWeb Safari and desktop app. However I agree with you. isVisible already has at line 321 {isValidateCodeFormVisible && !loginData.validatedDate && !!loginData &&. It looks redundant if there are 2 of isVisibles.

Are you going to clean up isVisible? I feel we don't really need it. isVisible = false is the same as unmounting the component.

Let me update without isVisible since it is at line 321.

@hoangzinh Updated!

Thank you @jacobkim9881. I will try to get another review today

jacobkim9881 · 2025-02-05T21:06:40Z

It's weird. I can't see your answers.

Pardon. I thought it is commented with pending label. I have submitted my pending review.

hoangzinh · 2025-02-10T10:18:12Z

src/components/ValidateCodeActionForm/index.tsx

+
+    useEffect(
+        () => () => {
+            firstRenderRef.current = true;


I think we don't need this condition anymore. Because it's only set to true when unmount.

I see. Let me update this one.

We don't have to check if it's firstly rendered or not.

Yeah, should we remove this ref?

hoangzinh · 2025-02-10T10:19:33Z

src/components/ValidateCodeActionForm/index.tsx

+                // We need to run clearError in cleanup function to use as onClose function.
+                // As 'useEffect cleanup function' runs when even the component is unmounted, we need to put clearError() in the if condition.
+                // So clearError() will not run when the form is unmounted.
+                if (isClosedRef.current && !isValidated) {


Suggested change

if (isClosedRef.current && !isValidated) {

if (!isValidated) {

I'm confused if we need to check isClosedRef.current which only be set to true in previous unmount event. What if this unmount are ran before isClosedRef.current set to true?

What if this unmount are ran before isClosedRef.current set to true?

Without isClosedRef.current, clearError() will run whenever dependencies are called.

// So clearError() will not run when the form is unmounted.

Sorry, this comment confuses me. I thought it should be "// So clearError() will run when the form is unmounted.". If yes, then can we just

useEffect( () => () => { clearError(); }, [], );

I tried use clearError() with useEffect(() => () => {}, []) but when tapping the contact method to open validate form, the browser was frozen.

Have you tried this @jacobkim9881

useEffect(() => { sendValidateCode(); return () => { clearError(); }; // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps }, []);

This should work but at first time to clicking a not validated contact method, clearError() isn't fired. Let me find how to fix.

It seems cleanup function can't run Onyx without any dependency.

I have updated to delete unnecessary conditions and I haven't find any solution for this yet:

This should work but at first time to clicking a not validated contact method, clearError() isn't fired

It's because of this line

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx

Line 303 in 5ddef34

clearError={() => clearContactMethodErrors(contactMethod, !isEmptyObject(validateLoginError) ? 'validateLogin' : 'validateCodeSent')}

Hmm, then I think we should do following

We should useCallback for this callback, to avoid it always try to clearError

Update existing code in ValidateCodeActionForm to

const isUnmounted = useRef(false); useEffect(() => { sendValidateCode(); return () => { isUnmounted.current = true; }; }, []); useEffect(() => { return () => { if (!isUnmounted.current) { return; } clearError(); }; }, [clearError]);

hoangzinh · 2025-02-10T10:20:24Z

src/components/ValidateCodeActionForm/index.tsx

+                clearError={clearError}
+                buttonStyles={[themeStyles.justifyContentEnd, themeStyles.flex1]}
+                ref={forwardedRef}
+                hasMagicCodeBeenSent={canSendHasMagicCodeBeenSent}


Suggested change

hasMagicCodeBeenSent={canSendHasMagicCodeBeenSent}

hasMagicCodeBeenSent={hasMagicCodeBeenSent}

Should we use hasMagicCodeBeenSent from the component props instead of create a new state canSendHasMagicCodeBeenSent here?

Pardon, if I misunderstood. Do you want hasMagicCodeBeenSent defined in <ValidateCodeActionForm> not as a prop?

ah no. I mean should we use prop hasMagicCodeBeenSent here

Something like this hasMagicCodeBeenSent={hasMagicCodeBeenSent}

I understood. If we use hasMagicCodeBeenSent={hasMagicCodeBeenSent}, the validate form will get focuses as soon to stop showing number pad on mobile device:

App/src/pages/settings/Profile/Contacts/ValidateCodeForm/BaseValidateCodeForm.tsx

Lines 142 to 145 in 1466c02

if (!hasMagicCodeBeenSent) {

return;

}

inputValidateCodeRef.current?.clear();

It's because the form was focused before interactions(num pad showing) are finished. So after interactions are finished, hasMagicCodeBeenSent prop is sent.

App/src/components/ValidateCodeActionForm/index.tsx

Lines 54 to 58 in 3fc7ee4

if (hasMagicCodeBeenSent) {

InteractionManager.runAfterInteractions(() => {

setCanSendHasMagicCodeBeenSent(true);

});

}

I will add autofocus.

This bug occurs on staging too.

Hmm, if we can fix it in this PR, then it should be good.

Yeah, inputValidateCodeRef has ref's focus(). ref's focus() event can be bothered to cause transition animation error or keyboard not showing error.

… m53884

…roid app

jacobkim9881 added 15 commits January 22, 2025 12:22

fix:blank page appears in validate code form page

6373c37

lint: fixed ! assertion error

172c7b0

pulled original/main

c4e944f

lint assertion err fixed

6232476

lint disabled prop spreading

d216e60

Revert "pulled original/main"

217ac60

This reverts commit c4e944f.

comment type for ref

622df4f

should resolve() if container is undefined

9f1f207

comment about focus trap for animation

5493c6b

fix: clicking header back err occurs

dd436e9

clear error only after closing the modal

a404826

Merge branch 'main' of https://github.com/Expensify/App into m53884

6640264

lint fixed

6a12518

Revert "lint fixed"

3f12bb5

This reverts commit 6a12518.

lint fixed

433778f

Merge branch 'main' of https://github.com/Expensify/App into m53884

c001758

jacobkim9881 marked this pull request as ready for review January 28, 2025 20:36

jacobkim9881 requested a review from a team as a code owner January 28, 2025 20:36

melvin-bot bot requested review from hoangzinh and removed request for a team January 28, 2025 20:36

Merge branch 'main' of https://github.com/Expensify/App into m53884

72e88b7

hoangzinh reviewed Jan 30, 2025

View reviewed changes

jacobkim9881 added 3 commits January 30, 2025 11:55

moved isVisible in the validate code form

0afd14a

Merge branch 'main' of https://github.com/Expensify/App into m53884

9b7b20d

hide menu items while sliding out of validate page

c19c2af

hoangzinh reviewed Feb 4, 2025

View reviewed changes

jacobkim9881 closed this Feb 5, 2025

jacobkim9881 reopened this Feb 5, 2025

show/hide code form

0e4b20f

hoangzinh reviewed Feb 5, 2025

View reviewed changes

jacobkim9881 added 7 commits February 6, 2025 22:07

fix: clear error runs on default contact method too

bc83a7b

Merge branch 'main' of https://github.com/Expensify/App into m53884

1c0cced

fix: has 2 isVisible

91e940d

not use setState for isValidated

943ce63

created type for ValidateCodeActionForm

a2c3878

check isValidated not needed for sendValidateCode

600d0d5

commeted for cleanup function

3fc7ee4

hoangzinh reviewed Feb 10, 2025

View reviewed changes

jacobkim9881 added 10 commits February 11, 2025 11:34

not use firstRenderRef

c715494

Merge branch 'main' of https://github.com/Expensify/App into m53884

ef17df1

showing button on number pad poped up on iOS mWeb

9b73217

fix: keyboard not show on Android app, iOS app

08b8d76

deleted unneeded condition

aac99d3

Merge branch 'main' of https://github.com/Expensify/App into m53884

bc398e4

Merge branch 'd53884-5' of github.com:jacobkim9881/Expensify-App into…

3a7ebdf

… m53884

lint

35ab2c8

runs clearError() as call back when unmounted

f415985

fix: when clicking contact method 2nd time, numpad not showing on And…

09e4bc1

…roid app


		type ValidateCodeActionProps = ValidateCodeActionModalProps & ValidateCodeActionWithoutModalProps;

		function ValidateCodeActionWithoutModal({


		{!isValidateCodeActionModalVisible && getMenuItems()}
		{!isValidateCodeActionModalVisible && !!loginData?.validatedDate && getMenuItems()}

	<ValidateCodeActionForm
	{isValidateCodeFormVisible && !loginData.validatedDate && !!loginData && <ValidateCodeActionForm

	if (!firstRenderRef.current \|\| !isVisible \|\| hasMagicCodeBeenSent) {
	return () => {
	clearError();
	};
	}

	if (isClosedRef.current && !isValidated) {
	if (!isValidated) {

	hasMagicCodeBeenSent={canSendHasMagicCodeBeenSent}
	hasMagicCodeBeenSent={hasMagicCodeBeenSent}

	if (!hasMagicCodeBeenSent) {
	return;
	}
	inputValidateCodeRef.current?.clear();

	if (hasMagicCodeBeenSent) {
	InteractionManager.runAfterInteractions(() => {
	setCanSendHasMagicCodeBeenSent(true);
	});
	}

fix:blank page appears in validate code form page #55588

Are you sure you want to change the base?

fix:blank page appears in validate code form page #55588

Conversation

jacobkim9881 commented Jan 22, 2025 • edited Loading

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

jacobkim9881 commented Jan 28, 2025

jacobkim9881 commented Jan 28, 2025 • edited Loading

melvin-bot bot commented Jan 28, 2025

hoangzinh commented Jan 29, 2025

jacobkim9881 commented Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobkim9881 commented Feb 5, 2025

hoangzinh commented Feb 5, 2025

jacobkim9881 commented Feb 5, 2025

hoangzinh commented Feb 5, 2025

jacobkim9881 commented Feb 5, 2025

hoangzinh commented Feb 5, 2025

Choose a reason for hiding this comment

jacobkim9881 Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobkim9881 Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

jacobkim9881 Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobkim9881 commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobkim9881 Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobkim9881 commented Jan 22, 2025 •

edited

Loading

jacobkim9881 commented Jan 28, 2025 •

edited

Loading

jacobkim9881 commented Jan 29, 2025 •

edited

Loading

jacobkim9881 Feb 5, 2025 •

edited

Loading

jacobkim9881 Feb 6, 2025 •

edited

Loading

jacobkim9881 Feb 6, 2025 •

edited

Loading

jacobkim9881 Feb 10, 2025 •

edited

Loading

jacobkim9881 Feb 14, 2025 •

edited

Loading