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.

dangrous · 2025-02-03T22:15:26Z

hi all! @jacobkim9881 is this ready for another look? The failing jest test is unrelated, and should be fixed by a merge main. Thanks!

jacobkim9881 · 2025-02-03T23:15:58Z

@dangrous After updating a feature, the PR will be ready. Thanks for noticing.

dangrous · 2025-02-04T01:18:18Z

Great. Let's aim to get this merged on Tuesday if possible!

hoangzinh · 2025-02-04T11:19:04Z

Hi @jacobkim9881 is the PR ready for review?

hoangzinh · 2025-02-04T11:20:37Z

I also have some feedbacks above. Please take a look when you have time. Thank you.

jacobkim9881 · 2025-02-04T11:23:10Z

@hoangzinh Pardon, please to let me check 1.

jacobkim9881 · 2025-02-04T11:39:44Z

@hoangzinh PR is ready for review.

jacobkim9881 · 2025-02-04T12:04:21Z

When you have a time, please to take a look at this. Since focus processing is adjusted, this behavior is changed. I am not sure which is more easy for users. W/o Android and iOS mWeb, all has the behavior for main branch.
The PR:

expensify-test-20-2025-02-04_20.53.33.mp4

Dev main branch:

expensify-test-21-2025-02-04_20.55.41.mp4

hoangzinh · 2025-02-04T13:36:27Z

Since focus processing is adjusted, this behavior is changed. I am not sure which is more easy for users. W/o Android and iOS mWeb, all has the behavior for main branch.

@jacobkim9881, Sorry can you describe what was changed? I tried to watch 2 recordings and can't realize what are differents.

jacobkim9881 · 2025-02-04T13:44:44Z

@hoangzinh At 00:08 the keyboard showing. but I think I made a noise. I can't see something too while seeing again.

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.

I mean as you can see the value of prop isVisible

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

Line 324 in bc83a7b

isVisible={isValidateCodeFormVisible && !loginData.validatedDate && !!loginData}

is same as condition to render ValidateCodeActionForm in line 321

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

Lines 321 to 322 in bc83a7b

{isValidateCodeFormVisible && !loginData.validatedDate && !!loginData && (

<ValidateCodeActionForm

therefore I am concerned is there any way that isVisible=false inside ValidateCodeActionForm component? Because if it's false, it means component is unmounted

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.

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.

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

jacobkim9881 added 2 commits February 3, 2025 18:34

lint fixed

23498e9

focusTrap not needed in mSafari

0d1f9fe

jacobkim9881 added 3 commits February 4, 2025 11:32

fix: keyboard not show on Android app, iOS app

a317155

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

5b3af3c

fix: show remove contact method modal

e37d564

focusTrap not needed in mSafari

589f444

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 2 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


		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();
	};
	}

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

dangrous commented Feb 3, 2025

jacobkim9881 commented Feb 3, 2025

dangrous commented Feb 4, 2025

hoangzinh commented Feb 4, 2025

hoangzinh commented Feb 4, 2025

jacobkim9881 commented Feb 4, 2025 • edited Loading

jacobkim9881 commented Feb 4, 2025

jacobkim9881 commented Feb 4, 2025

hoangzinh commented Feb 4, 2025

jacobkim9881 commented Feb 4, 2025

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

Choose a reason for hiding this comment

hoangzinh 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

jacobkim9881 commented Feb 5, 2025

jacobkim9881 commented Jan 22, 2025 •

edited

Loading

jacobkim9881 commented Jan 28, 2025 •

edited

Loading

jacobkim9881 commented Jan 29, 2025 •

edited

Loading

jacobkim9881 commented Feb 4, 2025 •

edited

Loading

jacobkim9881 Feb 5, 2025 •

edited

Loading

jacobkim9881 Feb 6, 2025 •

edited

Loading

hoangzinh Feb 6, 2025 •

edited

Loading

jacobkim9881 Feb 6, 2025 •

edited

Loading