[HOLD for payment 2023-12-12] [$500] Start Chat - Invalid red dot briefly appears by closing 'room' tab when there's no prior action

[lanitochka17]

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

---

Version Number: 1.3.93-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Navigate to staging.new.expensify.com and log in to an Expensifail account
2. Click the green plus button.
3. Select "Start chat"
4. Choose ''Room'' tab
5. Click outside the panel, to close it
6. Repeat steps 2-5

Expected Result:

No red dots should be seen when closing the Room tab

Actual Result:

An invalid red dot appears briefly when closing the 'room' tab when there is no prior action

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Bug6257374_1698700833243.WEB_-_Red_dot_-_room_tab.mp4

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01cccde7c0776bc3a8
• Upwork Job ID: 1719141449851113472
• Last Price Increase: 2023-11-06
• Automatic offers:
• jjcoffee | Reviewer | 27647427
• dukenv0307 | Contributor | 27647429

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01cccde7c0776bc3a8

[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

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)

[dukenv0307]

Proposal

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

An invalid red dot appears briefly when closing the 'room' tab when there is no prior action

What is the root cause of that problem?

When the input is blurred, this will be triggered, causing the error to show. Normally we won't see it since the navigation happens very quickly after clicking, but if we long press outside the RHN, and after 1-2 seconds, release the press, we'll see the error very clearly.

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

We need to assign a nativeID to the background element that receives the "click outside", and in here, if the relatedTarget of the event has that ID, we'll early return and not trigger the setTouchedInput.

We also need to fix here so that it will pass through the event of the text input

onBlur={(event) => isFocused && onBlur(event)}

This will make sure the click on the background element to close the RHN, no matter long press or normal press, will never cause the error to show.

If there's any element, if pressed ,like the Back button, we don't want to trigger the touched input and show the error, we can add the native ID of that element to the list of native IDs above.

What alternative solutions did you explore? (Optional)

NA

Result

poc-resize.mov

[Krishna2323]

Proposal

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

Start Chat - Invalid red dot briefly appears by closing 'room' tab when there's no prior action

What is the root cause of that problem?

When the input gets changed or blurred the validation is called and sets the error, we have a smooth transition between pages that's why we can see the error for a second.

App/src/pages/workspace/WorkspaceSettingsPage.js

Lines 75 to 88 in 5b5d465

	const validate = useCallback((values) => {
	const errors = {};
	const name = values.name.trim();
	if (!name || !name.length) {
	errors.name = 'workspace.editor.nameIsRequiredError';
	} else if ([...name].length > CONST.WORKSPACE_NAME_CHARACTER_LIMIT) {
	// Uses the spread syntax to count the number of Unicode code points instead of the number of UTF-16
	// code units.
	errors.name = 'workspace.editor.nameIsTooLongError';
	}
	return errors;
	}, []);

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

We can return from validation when the page gets blurred, either we can apply this in WorkspaceNewRoomPage or we can directly apply it in Form so it gets applied for every form validations.

    const navigation = useNavigation();

    useEffect(() => {
        const removeBlurListener = navigation.addListener('blur', () => {
            isNavigating.current = true;
        });

        const removeFocusListener = navigation.addListener('focus', () => {
            isNavigating.current = false;
        });

        return () => {
            removeBlurListener();
            removeFocusListener();
        };
    }, [navigation]);
    
    if (isNavigating.current) {
        return {};
    }

Before fix

before_fix.mp4

After fix

after_fix.mp4

[s-alves10]

Proposal

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

Red dot briefly appears when closing create-room page by tapping outside the RHN even when there is no prior action

What is the root cause of that problem?

App/src/components/Form.js

Lines 356 to 359 in 5b5d465

	setTouchedInput(inputID);
	if (props.shouldValidateOnBlur) {
	onValidate(inputValues, !hasServerError);
	}

As you can see above, we set touched state to true and run validator when focused element gets blurred.
When the room tab gets active, the room name input gets focused. When closing the RHN, the name input gets blurred and so the validator runs by the above code.

This is the root cause

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

This issue is related to the current form validation logic. We need to change this.
I suggest not to validate the input entries with no changes. That is, only dirty inputs would be validated.
We are setting touched state below

App/src/components/Form.js

Line 356 in 5b5d465

setTouchedInput(inputID);

App/src/components/Form.js

Lines 367 to 369 in 5b5d465

	onTouched: () => {
	setTouchedInput(inputID);
	},

App/src/components/Form.js

Lines 373 to 375 in 5b5d465

	if (focusedInput.current && focusedInput.current !== inputKey) {
	setTouchedInput(focusedInput.current);
	}

1. Remove the first 2 code segments
2. We don't need to track the previous focused element. Update the third code to

    setTouchedInput(inputKey);

Result

30618.mp4

What alternative solutions did you explore? (Optional)

We can disable the validation in navigation blur event handler

1. Add the following code to the WorkspaceNewRoomPage here

    const shouldValidateRef = useRef(true);
    useFocusEffect(useCallback(() => () => {
        shouldValidateRef.current = false;
    }, []));

2. In validate function, add the following code here

            if (!shouldValidateRef.current) {
                return errors;
            }

We can apply this change to the form to generalize

[abekkala]

@jjcoffee whoo! already a couple proposals! Can you review when you have a moment?

[yh-0218]

Proposal

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

Red dot briefly appears when closing create-room page by tapping outside the RHN even when there is no prior action

What is the root cause of that problem?

App/src/components/Form.js

Line 358 in a31933a

onValidate(inputValues, !hasServerError);

When onBlur(), we do validate.
So when click tab, or during click outside of Modal, we will see error text.

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

We can set shouldValidateOnBlur=false for web and desktop.

What alternative solutions did you explore? (Optional)

We can catch event of click of outside of modal and then disable validate.

App/src/libs/Navigation/AppNavigator/Navigators/RightModalNavigator.js

Line 26 in a31933a

{!isSmallScreenWidth && <Overlay onPress={props.navigation.goBack} />}

[jjcoffee]

Sorry this one slipped through the cracks, will review on Monday!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[jjcoffee]

Apologies, will have to be tomorrow that I review this one!

[jjcoffee]

@yh-0218 We don't want to set shouldValidateOnBlur to false for web and desktop; it is deliberate to validate on blur, see here.

@s-alves10 Correct me if I'm wrong but I think your proposal would briefly show the error in the same situation, as long as some text was entered. I don't think this is what we want here.

@Krishna2323 I'm unsure why the event listeners get added in the cleanup of the useEffect in your proposal?

@dukenv0307 I like the general approach in your proposal, but I'm not sure how generic a solution it will be since it requires modifying specific inputs to pass through the event.

[s-alves10]

Proposal

Updated

@jjcoffee
I added an alternative solution

[dukenv0307]

@jjcoffee

but I'm not sure how generic a solution it will be since it requires modifying specific inputs to pass through the event.

Actually, we just need to find the inputs that are wrapped by a Form or FormProvider and don't explicitly pass event to
onBlur={() => onBlur()} // need to pass event
onBlur={onBlur} // this works already
and after checking, we only missed one place in the RoomNameInput component and it's the only "specific input" that we need to fix.

Passing through the event is best practice and it's not something that only becomes necessary due to my solution 😄

So I think of my proposal as "generic solution + fix 1 existing bug in RoomNameInput", not "non-generic solution that requires modifying certain inputs"

[Krishna2323]

@jjcoffee, we are always removing the listeners in cleanup function whenever we are using navigation.addListener.

The purpose of the cleanup function is to remove these event listeners when the component unmounts to prevent memory leaks. The cleanup function ensures that resources are properly released when they are no longer needed.

[jjcoffee]

Sorry wasn't able to get to this today, will review on Monday!

[dukenv0307]

@jjcoffee @Julesssss the updated PR is ready for review

[abekkala]

I'm heading out for the Thanksgiving holiday and will be back bright and early Monday morning.
If something pops up that needs urgent attention please reapply label or ask in slack.

[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.4.3-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Invalid red dot briefly appears by closing 'room' tab when there's no prior action #31669

If no regressions arise, payment will be issued on 2023-12-04. 🎊

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

For reference, here are some details about the assignees on this issue:

• @jjcoffee requires payment offer (Reviewer)
• @dukenv0307 requires payment offer (Contributor)

[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:

• [@jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
• [@jjcoffee] 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:
• [@jjcoffee] 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:
• [@jjcoffee] Determine if we should create a regression test for this bug.
• [@jjcoffee] 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.
• [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[abekkala]

PAYMENTS FOR DEC 04

• Reporter: Applause [no payment]
• Contributor who fixed: @dukenv0307 [$500] offer
• PR Review: @jjcoffee [$500] offer

@jjcoffee can you please complete the checklist above?

[parasharrajat]

There was a regression from the first PR @abekkala

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: N/A - this was always the behaviour as far as I can see
• 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: N/A
• 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: N/A
• Determine if we should create a regression test for this bug. Yes
• 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.

Regression Test Proposal

1. Open the app and click the green plus button.
2. Select "Start chat"
3. Choose ''Room'' tab
4. Click outside the panel or the back button and hold
5. Verify that no Red dot appears

Do we agree 👍 or 👎

[jjcoffee]

@abekkala Checklist complete! Also note that there was a regression.

[abekkala]

Contributor who fixed: @dukenv0307 [$250] offer
PR Review: @jjcoffee [$250] offer

50% penalty for regression. Offer Link remains but at payout will be edited to $250

[Julesssss]

We can pay this one out tomorrow

[abekkala]

payments will be sent Tue Dec 12

[abekkala]

@dukenv0307 & @jjcoffee - payments sent and contract ended - thank you! 🎉

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.