[HOLD for payment 2024-06-11] HELD ON ONYX BUMP 2.0.40 [$250] FAB - Nothing happens when Start chat is clicked after creating a room offline

[izarutskaya]

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.4.65-0
Reproducible in staging?: Y
Reproducible in production?: Y
Found when executing: https://github.com/Expensify/Expensify/issues/302054
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Log in with a new account
2. Create a workspace
3. Go offline
4. Create a new room
5. Navigate to the LHN
6. Navigate to FAB - Start chat

Expected Result:

Start chat modal should open.

Actual Result:

Nothing happens when Start chat is clicked after creating a room offline. On iOS and Android native, you can see it opening for a short time but it disappears after. On mWeb Safari, the page shakes after the tap.

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

Bug6459380_1713902012941.bandicam_2024-04-23_21-44-21-452.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0114d6fec2c1cc3911
• Upwork Job ID: 1785002053872132096
• Last Price Increase: 2024-04-29
• Automatic offers:
• bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @muttmuure / @muttmuure

[melvin-bot]

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #vip-vsb.

[bernhardoj]

Proposal

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

When we open start chat again after creating a room while offline, nothing happen.

What is the root cause of that problem?

When we create a new room, it will optimistically set the loading to the new room form

App/src/libs/actions/Report.ts

Lines 1955 to 1957 in 08b7ff4

	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.FORMS.NEW_ROOM_FORM,
	value: {isLoading: true},

Because we are offline, the loading state is always true. When we reopen the page again, it will immediately close because of the below effect.

App/src/pages/workspace/WorkspaceNewRoomPage.tsx

Lines 146 to 152 in 08b7ff4

	useEffect(() => {
	if (!(((wasLoading && !isLoading) || (isOffline && isLoading)) && isEmptyObject(errorFields))) {
	return;
	}
	Navigation.dismissModal(newRoomReportID);
	// eslint-disable-next-line react-hooks/exhaustive-deps -- we just want this to update on changing the form State
	}, [isLoading, errorFields]);

The (isOffline && isLoading) condition is true. When the page is mounted, we clear the loading state (set to false)

App/src/pages/workspace/WorkspaceNewRoomPage.tsx

Lines 128 to 130 in 08b7ff4

	useEffect(() => {
	Report.clearNewRoomFormError();
	}, []);

and this condition (wasLoading && !isLoading) is also becomes true.

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

The useEffect is added (in #31655) so it will only close the page when the request is successful, but we added back the code to close it immediately in Report.addPolicyReport, so it's really weird, I think it's a bad merge, so we can remove the dismiss code here.

App/src/libs/actions/Report.ts

Lines 2012 to 2013 in 08b7ff4

	API.write(WRITE_COMMANDS.ADD_WORKSPACE_ROOM, parameters, {optimisticData, successData, failureData});
	Navigation.dismissModalWithReport(policyReport);

To fix this issue, we can set initWithStoredValues: false to the new room form state. This way, the data will always be empty initially. We don't want the previous state to affect the new page.

formState: {
    key: ONYXKEYS.FORMS.NEW_ROOM_FORM,
    initWithStoredValues: false,
},

Then, we can remove this effect

App/src/pages/workspace/WorkspaceNewRoomPage.tsx

Lines 128 to 130 in 08b7ff4

	useEffect(() => {
	Report.clearNewRoomFormError();
	}, []);

Or trigger it when it's unmounted instead.

NOTE: there is currently a bug in Onyx where initWithStoredValues doesn't work. To fix it we should initialize the onyx key only if initWithStoredValues is true.

-if (
+if (mapping.initWithStoredValues &&
    ((value !== undefined
        && !Onyx.hasPendingMergeForKey(key))
    || mapping.allowStaleData)
) {

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0114d6fec2c1cc3911

[melvin-bot]

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

[muttmuure]

@mollfpr there's a proposal above for you to take a look at

[mollfpr]

@bernhardoj Is there any conversation regarding the bug of initWithStoredValues? It should be critical if the property doesn't work.

[bernhardoj]

@mollfpr I'm not aware of any conversation of that

[mollfpr]

The @bernhardoj proposal looks good to me! For the issue on initWithStoredValues, I think we can decide after the PR is created.

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

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

[puneetlath]

For the issue on initWithStoredValues, I think we can decide after the PR is created.

Sorry, I don't follow what this means. Mind explaining for me?

[mollfpr]

I notice that initWithStoredValues is only works for the first time the page rendered.

Screen.Recording.2024-05-07.at.11.39.23.mp4

Here's in the video, after the refresh and then opening the new room page, you can see in the console log it will show undefined which is expected because I set initWithStoredValues to false. But the second time I opened the page, it didn't show undefined but instead the value stored.

I'm not sure if this is expected and what causing the issue, but @bernhardoj may be another option is to clear the formState after it unmounts can be the option to dodge the issue?

[bernhardoj]

may be another option is to clear the formState after it unmounts can be the option to dodge the issue?

It won't work if the user closes the app/tab. We need to fix the onyx first before creating the App PR. We can create a local state of loading with an initial value of false, but I think it's better to fix the onyx instead of avoiding it.

[mollfpr]

@bernhardoj Could you create a discussion of the onyx issue? We can confirm if it is an issue before fixing it.

[bernhardoj]

I think the property name is self-explanatory and currently it doesn't work as the name and comment suggests,

but here is the discussion.

[puneetlath]

Ok understood. So my apologies, can you summarize for me the changes we'll be making in Onyx and in App?

[bernhardoj]

@mollfpr so, based on the discussion, it's confirmed the current behavior of initWithStoredValues is wrong. To further prove that it's a bug, we can run a unit test. In onyx repo itself, there is already a unit test for withOnyx + initWithStoredValues, but the test case is currently wrong.

You can see the key name that is set to Onyx ('test_key') and is connected to the component ('test') is different. That's why the test will always pass. You can update the 'test_key' to 'test' and see it will fails.

@puneetlath the plan will be,

1. Update the onyx unit test case
2. Apply the onyx initWithStoredValues bug fix in here

3. Update the onyx in the App and apply the App fix

the same issue actually happens in useOnyx hook too, but I'm not confident to fix the new hook. initWithStoredValues currently only works with Onyx.connect

[melvin-bot]

@puneetlath @mollfpr @muttmuure this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

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

• Fix start chat page closes immediately after creating a room while offline #42679

If no regressions arise, payment will be issued on 2024-06-11. 🎊

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

• @mollfpr requires payment through NewDot Manual Requests
• @bernhardoj requires payment automatic 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:

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

[puneetlath]

@mollfpr bump on the checklist so we can pay.

[bernhardoj]

@puneetlath I think we still need to wait a bit based on my comment here

[bernhardoj]

@puneetlath The Onyx PR is merged. I think we can have QA retest this.

[kavimuru]

Bug fixed and not reproducible

bandicam.2024-06-13.21-33-38-862.1.mp4

[puneetlath]

Ok, so we're ready for the checklist now yeah @bernhardoj @mollfpr? So that we can pay?

[bernhardoj]

Yes @mollfpr

[melvin-bot]

@puneetlath, @mollfpr, @muttmuure, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] 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:

No offending PR.

[@mollfpr] 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:
[@mollfpr] Determine if we should create a regression test for this bug.

The regression step should be enough.

[@mollfpr] 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.

1. Go offline
2. Create a new room from FAB
3. Open the new room page again from FAB
4. Verify the new room page is shown
5. 👍 or 👎

[muttmuure]

@mollfpr - $250 C+

[JmillsExpensify]

$250 approved for @mollfpr

[melvin-bot]

@puneetlath, @mollfpr, @muttmuure, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[puneetlath]

@muttmuure what is left to do here? Can we close?

[bernhardoj]

I think the payment for me is still pending.

[muttmuure]

@bernhardoj paid $250 for C