[HOLD for payment 2023-11-30] [$500] Composer is very large on first render

[m-natarajan]

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: DEV
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
Expensify/Expensify Issue URL:
Issue reported by: @marcaaron
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1699488803622289

Action Performed:

1. build the app from main branch
2. open the local dev server
3. See giant composer

Expected Result:

Minimized composer

Actual Result:

Giant composer appears

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

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b525aa9327716a48
• Upwork Job ID: 1723113262718377984
• Last Price Increase: 2023-11-17
• Automatic offers:
• 0xmiroslav | Reviewer | 27759862
• erquhart | Contributor | 27759864

[melvin-bot]

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

[melvin-bot]

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

[strepanier03]

@srikarparsi - Could you please help me test this on DEV? I don't have access to a DEV environment.

[MonilBhavsar]

I was able to see this today.
Browser is firefox. I was already logged into an account. I opened browser and opened app. I saw large composer and related console error, that might help

[strepanier03]

Thank you @MonilBhavsar 🙌

[melvin-bot]

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

[melvin-bot]

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

[shubham1206agra]

@MonilBhavsar Unable to repro this. Can you check your reportIsComposerFullSize_ in Onyx? Maybe something is wrong there only.

[shubham1206agra]

Specifically, reportIsComposerFullSize_null if it all exists.

[erquhart]

Proposal

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

Note: this is now reproducible in production.

Given:

• loading the root domain with no path (Eg., new.expensify.com)
• the last active report has not been viewed in the current browser since last login

Observe: The composer is expanded by default, rather than collapsed.

Steps to repro consistently:

1. log out
2. log in
3. wait for app to load, including redirecting to a report
4. load app again from root domain (Eg., new.expensify.com)
5. wait for app to redirect to a report
6. observe expanded composer

To repeat without logging out and in again:
7. select a different report from the LHN, one that has not been loaded since last login
8. repeat steps 4-6

These steps work once per report. When the report initially loads and the bug is triggered, the bug won't reoccur in that report until the user logs out and back in (explanation below).

---

What is the root cause of that problem?

Summary (tl;dr)

Under specific conditions, we're using reportIsComposerFullSize_ as an onyx key to access composer full size state, without a report id appended, which results in an object, which is coerced to true. This is happening because Onyx is returning stale data, and not updating the value it returns when the key changes.

The bug is absent with react-native-onyx@1.0.107 and present in react-native-onyx@1.0.108, pointing to this PR as the likely cause: Expensify/react-native-onyx#401

Details

1. We're fetching composer expanded state before we have a report id, and report ID is an empty string.

   Click for details

   • On initial load to the root domain with no path, getRehydratedState has a partial state with a reportId set to an empty string:
     • partial state:

       App/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.js

       Line 76 in 2e8b89b

       addCentralPaneNavigatorRoute(partialState);

     • reportID default:

       App/src/components/withCurrentReportID.tsx

       Line 40 in 2e8b89b

       setCurrentReportID(Navigation.getTopmostReportId(state) ?? '');

   • When ReportScreenWrapper was converted to a functional component, getReportId() was updated to return null when no report id is available, to support showing a skeleton during loading:
     Refactor ReportScreenWrapper to functional component #22260 (comment)
   • Later in the same PR, the function was updated again to stringify null, the intent being to return a report id that should never exist, "null":
     Refactor ReportScreenWrapper to functional component #22260 (comment)
   • As mentioned earlier, reportID can be an empty string on load, and because lodash.get() only returns the provided default value in case of undefined (docs), the empty string is returned rather than "null"
   • Some other changes have happened to this line since, but they all have the same result on initial load because reportID is an empty string and not undefined

2. Report ID being an empty string shouldn't be a problem because we don't attempt to get composer full size state from onyx until we have a truthy report ID.

   Click for details

   • isReportReadyForDisplay only evaluates truthy if the report ID is truthy:

     App/src/pages/home/ReportScreen.js

     Lines 224 to 230 in 04b5fa4

     	const isReportReadyForDisplay = useMemo(() => {
     	const reportIDFromPath = getReportID(route);
     	// This is necessary so that when we are retrieving the next report data from Onyx the ReportActionsView will remount completely
     	const isTransitioning = report && report.reportID !== reportIDFromPath;
     	return reportIDFromPath !== '' && report.reportID && !isTransitioning;
     	}, [route, report]);

   • ReportFooter, which renders the composer, is not passed the onyx state value as a prop while isReportReadyForDisplay is still falsy:

     App/src/pages/home/ReportScreen.js

     Lines 434 to 447 in 04b5fa4

     	{isReportReadyForDisplay ? (
     	<ReportFooter
     	pendingAction={addWorkspaceRoomOrChatPendingAction}
     	reportActions={reportActions}
     	report={report}
     	isComposerFullSize={isComposerFullSize}
     	onSubmitComment={onSubmitComment}
     	policies={policies}
     	listHeight={listHeight}
     	personalDetails={personalDetails}
     	/>
     	) : (
     	<ReportFooter isReportReadyForDisplay={false} />
     	)}

   • the isComposerFullSize prop for the ReportActionCompose component - which provides the prop to the composer - defaults to false:

     App/src/pages/home/report/ReportActionCompose/ReportActionCompose.js

     Line 89 in 04b5fa4

     isComposerFullSize: false,

3. ReportScreenIDSetter successfully sets the initial report ID and triggers nav rehydration, making the composer visible and rendering it collapsed, because we have the correct Onyx key for the report composer full size state, which has a falsy return value (undefined in this case).

   Click for details

   • Initial report ID and redirect via navigation.setParams() here:

     App/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.js

     Lines 85 to 91 in 04b5fa4

     	// If there is no reportID in route, try to find last accessed and use it for setParams
     	const reportID = getLastAccessedReportID(reports, !canUseDefaultRooms, policies, isFirstTimeNewExpensifyUser, lodashGet(route, 'params.openOnAdminRoom', false));
     	// It's possible that reports aren't fully loaded yet
     	// in that case the reportID is undefined
     	if (reportID) {
     	navigation.setParams({reportID: String(reportID)});

4. Concurrently, deep link handling is being used when there is no path, triggering an unnecessary navigation state rehydration and updating the report ID in nav state back to an empty string.

   Click for details

   • openReportFromDeepLink() is pushing a new route after Session.waitForUserSignIn() completes on load, even if there is no route:

     App/src/libs/actions/Report.js

     Line 2016 in 04b5fa4

     Navigation.navigate(route, CONST.NAVIGATION.ACTION_TYPE.PUSH);

   • this triggers rehydration, marks the current nav state stale, and adds a new route to the nav state:

     App/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.js

     Lines 75 to 76 in 04b5fa4

     	partialState.stale = true;
     	addCentralPaneNavigatorRoute(partialState);

   • addCentralPaneNavigatorRoute() calls getTopMostReportIDFromRHP() to get a report ID, and since we just loaded the app, the report ID will be an empty string:

     App/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.js

     Lines 45 to 63 in 04b5fa4

     	const addCentralPaneNavigatorRoute = (state) => {
     	const reportID = getTopMostReportIDFromRHP(state);
     	const centralPaneNavigatorRoute = {
     	name: NAVIGATORS.CENTRAL_PANE_NAVIGATOR,
     	state: {
     	routes: [
     	{
     	name: SCREENS.REPORT,
     	params: {
     	reportID,
     	},
     	},
     	],
     	},
     	};
     	state.routes.splice(1, 0, centralPaneNavigatorRoute);
     	// eslint-disable-next-line no-param-reassign
     	state.index = state.routes.length - 1;
     	};

5. At least one render of ReportScreen occurs with an empty string report ID before isReportReadyForDisplay evaluates to false, causing key reportIsComposerFullSize_ to be passed to Onyx and resulting in an empty object value for composer full size state.

6. After the empty object is returned, even after the correct key is provided, Onyx continues providing an empty object and does not update even though the key quickly updates to include the report ID. This is a bug. The bug is absent with react-native-onyx@1.0.107 and present in react-native-onyx@1.0.108, pointing to this PR as the likely cause: Fix cache for missing keys and empty collections react-native-onyx#401

---

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

This needs to be fixed upstream, but the risk of the upstream change feels elevated, and reverting the offending commit may cause other issues. In the interest of getting a quick fix into production for this specific bug:

getReportID() is supposed to be returning the string "null" to help ensure against this kind of problem, but as explained under part 1 of the root cause, it isn't doing that.

App/src/pages/home/ReportScreen.js

Lines 128 to 131 in 04b5fa4

	function getReportID(route) {
	// // The reportID is used inside a collection key and should not be empty, as an empty reportID will result in the entire collection being returned.
	return String(lodashGet(route, 'params.reportID', null));
	}

We should update getReportID() to return "null" whenever the report ID is falsy, rather than just when it's undefined:

return String(lodashGet(route, 'params.reportID') || null);

This is simply ensuring that the current intended behavior - returning a string of value "null" - happens for any falsy report id value.

---

What alternative solutions did you explore?

Here are two other fixes that work just as well, and are legitimate fixes to unintended behavior in their own right. However it's possible that some parts of the app may be unintentionally relying on the current behavior. That said, both of these corrections should be made.

Alternative 1

isReportReadyForDisplay is memoized, but we're checking the report object in the dependencies array instead of just the report ID, which is what we actually access:

App/src/pages/home/ReportScreen.js

Lines 224 to 230 in 04b5fa4

	const isReportReadyForDisplay = useMemo(() => {
	const reportIDFromPath = getReportID(route);
	// This is necessary so that when we are retrieving the next report data from Onyx the ReportActionsView will remount completely
	const isTransitioning = report && report.reportID !== reportIDFromPath;
	return reportIDFromPath !== '' && report.reportID && !isTransitioning;
	}, [route, report]);

Limiting the dependency to the report ID keeps the memoized value from updating when the second rehydration occurs (part 4 in the root cause above), and stops the unintended collection key from being used when accessing composer full size state.

We should update report in the dependency array to report.reportID.

Alternative 2

Again looking at part 4 of the root cause above, our deep linking logic shouldn't push a route if there is no route, as it only serves to rerun the last active report redirect that ReportScreenIDSetter will have already executed. This could easily cause other bugs as well:

App/src/libs/actions/Report.js

Line 2016 in 04b5fa4

Navigation.navigate(route, CONST.NAVIGATION.ACTION_TYPE.PUSH);

We should return before we hit this line if route is falsy.

Alternative 3

We also have the option of reverting the PR in onyx that caused the regression, but that will likely cause other regressions - I'd recommend a forward looking solution rather than a revert there.

[0xmiros]

I believe this is a recent regression. I started experiencing this just about 2 weeks ago.
@erquhart are you able to find offending PR? Also, please clarify constant reproducible step.
Please explain in root cause why this is randomly reproducible

[erquhart]

Proposal

Updated

@0xmiroslav proposal rewritten. Note that this is now in production.

[dukenv0307]

Proposal

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

The composer is full-size on the first render

What is the root cause of that problem?

In this PR, we always return an object when the key is a collection key. In the past, we will return as undefined

https://github.com/Expensify/react-native-onyx/blob/14aed7bfba5668d1635bec5f5b99653c5a99916d/lib/Onyx.js#L256

On the first render, this key doesn't exist in Onyx so isComposerFullSize is an object, and then compose is displayed as full-size

Other keys like reportMetadata don't have this problem because it's merged in Onyx when we call openReport

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

In ReportScreen, we should create a useEffect to merge this key into Onyx if it doesn't present in Onyx.

useEffect(() => {
    if (isBoolean(isComposerFullSize) || !report.reportID) {
        return;
    }
    if (!_.isBoolean(isComposerFullSize)) {
        Report.setIsComposerFullSize(report.reportID, false);
    }
}, [report.reportID, isComposerFullSize])

And update this prop to isComposerFullSize={isBoolean(isComposerFullSize) && isComposerFullSize}

App/src/pages/home/ReportScreen.js

Line 442 in 77be80b

isComposerFullSize={isComposerFullSize}

What alternative solutions did you explore? (Optional)

In Report.js, we can connect this collection, and then in openReport function, we can add optimisticData for this key if it doesn't present if the collection

[melvin-bot]

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

[erquhart]

@tgolen makes sense, I can swap null for 0 in my PR 👍

[tgolen]

OK, thanks! I wanted to give a quick warning that I believe I tried that once and then it broke several of the other references to getReportID(), but at the time I didn't dig much deeper into it. Ideally, they would all default to 0 and work fine. I'm sure you'll get to the bottom of it.

[erquhart]

@cead22 I'm ready to get a PR up once assigned.

[melvin-bot]

📣 @0xmiroslav 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @erquhart 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

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

• fix: ensure composer not full size when onyx state missing #31602

If no regressions arise, payment will be issued on 2023-11-30. 🎊

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 - N/A - Internal report
• Contributor that fixed the issue - @erquhart - $500 via Upwork
• Contributor+ that helped on the issue and/or PR - @0xmiroslav - $500 via Upwork

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

• @erquhart requires payment offer (Contributor)
• @0xmiroslav requires payment offer (Reviewer)

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

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

[erquhart]

@strepanier03 bumping for payment

[strepanier03]

Yep, this just came off payment hold yesterday so I'm taking care of it today. Didn't have the time to handle it yesterday.

[strepanier03]

@erquhart - You're paid out and the contract closed, thank you again for your work and involvement with our community.

@0xmiroslav - Please post the checklist items as soon as you have time and I'll finish up then close.

[0xmiros]

The regression was from Onyx repo, but after discussion, it's expected, so needed to fix on app side.
I don't think we need regression test for this. Composer is main feature of the app and if bug happens again, it will be caught instantly either by QA team or by devs.

[strepanier03]

Gotcha, thanks @0xmiroslav - I've finished paying this out and I'm going to close now.