[HOLD for payment 2024-03-11] [HOLD for payment 2024-03-07] IOU details page appears after delay and it is empty when request is created offline

[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: 1.4.45-0
Reproducible in staging?: y
Reproducible in production?: no
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. Go to staging.new.expensify.com
2. Request money from any user.
3. In 1:1 DM, click on the IOU preview twice to go to transaction thread.
4. Return to IOU report.
5. Click on the IOU preview to go transaction thread again.
6. Note that the details show up after some delay.
7. Go offline.
8. Create another IOU with the same user.
9. Click on the IOU preview twice to go to transaction thread.

Expected Result:

In Step 5, the IOU details page will appear without delay (production behavior).
In Step 9, the IOU details will show even when the request is created offline (production behavior).

Actual Result:

In Step 5, the IOU details page appears after some delay.
In Step 9, the IOU details page is blank when the request is created offline.

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

Bug6396511_1709174884370.20240229_104158.mp4

View all open jobs on GitHub

[melvin-bot]

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

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @techievivek (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[m-natarajan]

@techievivek FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[techievivek]

Seems like a regression from some recent change and indeed a bad one.

[youssef-lr]

This seems to be coming from here, reverting it fixed it for me. cc @tgolen. Really odd though, I can't see how those changes are causing this, but I double checked and triple checked, reverting the PR definitely fixed it.

Screen.Recording.2024-02-29.at.10.15.10.mov

[techievivek]

@youssef-lr Great catch, any idea why I am unable to find this PR changes in the diff between staging and production production...staging ?
I was trying to look at the recent deployed changes to figure out what could have caused this.

[youssef-lr]

I'm not sure, what I did was go through the PRs from the checklist. I tried to guess which PR could have caused this from the titles, but at the end gave up and started reverting PRs one by one lol.

[bernhardoj]

Proposal

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

The detail view appears with a delay while online, but not shown at all while offline.

What is the root cause of that problem?

This happens after this PR that replaces ReportActionsUtils.getParentReportAction usage with an onyx subscription.

App/src/pages/home/ReportScreen.js

Lines 648 to 658 in 48b24ec

	parentReportAction: {
	key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`,
	selector: (parentReportActions, props) => {
	const parentReportActionID = lodashGet(props, 'report.parentReportActionID');
	if (!parentReportActionID) {
	return {};
	}
	return lodashGet(parentReportActions, parentReportActionID, {});
	},
	canEvict: false,
	},

When we open the transaction thread for the first time, the onyx selector props is always undefined (onyx bug, known issue, explained at the bottom of root cause), so parentReportAction will return an empty object initially. After we get the report action data, the ReportActionsView won't re-render because the component is memoized and we didn't include parentReportAction.

If the parent action data is empty, shouldDisplayParentAction will be true even though it should be false

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

Lines 58 to 59 in 5cfcf69

	const shouldDisplayParentAction =
	reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED && ReportUtils.isChatThread(report) && !ReportActionsUtils.isTransactionThread(parentReportAction);

The shouldDisplayParentAction component uses ancestor logic as shown below.

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 76 in 5cfcf69

{allAncestors.map((ancestor) => (

App/src/libs/ReportUtils.ts

Lines 4991 to 4995 in 5cfcf69

	const parentReportAction = ReportActionsUtils.getReportAction(parentReportID, parentReportActionID ?? '0');
	if (!parentReportAction || ReportActionsUtils.isTransactionThread(parentReportAction) || !parentReport) {
	break;
	}

Notice that we take the parentReportAction in getAllAncestorReportActionIDs imperatively and it's possible that the data is available in that function scope.

While online, it's most likely other props is triggering the re-render

Onyx issue

Both parentReportAction key and selector depend on the component props, in this case, report and both have issues. First, the key.

key expects the function params to pass the component existing props. However, the component props never contains the other previous onyx data.
https://github.com/Expensify/react-native-onyx/blob/d59649ef69a563ad2cffd55995875e711a5182a3/lib/withOnyx.js#L60

Second, the selector. When the component is first rendered, we try to get the cached value and use the selector if available.
https://github.com/Expensify/react-native-onyx/blob/d59649ef69a563ad2cffd55995875e711a5182a3/lib/Onyx.js#L278-L284

However, the selector will only work properly if we pass the current state of the component. From the code linked above, notice that we take the state from mapping.withOnyxInstance.state, but it's never available because the mapping object inside the withOnyx constructor doesn't contain a withOnyxInstance with a state property.
https://github.com/Expensify/react-native-onyx/blob/d59649ef69a563ad2cffd55995875e711a5182a3/lib/withOnyx.js#L61

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

For the re-render issue, we should include parentReportAction equality check to ReportActionsView memo.

if (!_.isEqual(oldProps.parentReportAction, newProps.parentReportAction)) {
    return false;
}

For the onyx issue, first, we should get the current state of the onyx data.

const cachedState = underscore_1.default.reduce(mapOnyxToState, (resultObj, mapping, propertyName) => {
    // resultObj is the latest reduced object
    const currentState = getOnyxDataFromState(resultObj, mapOnyxToState);

Then, extend both key props and value mapping with the current state.

const key = Str.result(mapping.key, {...props, ...currentState});
let value = Onyx.tryGetCachedValue(key, {
    ...mapping,
    withOnyxInstance: {state: currentState},
});

Just like we did in here

[techievivek]

Ok, that is a nice way to figure things out as well. :hattip:
I just realized that changes are there but it was not being rendered unless we click on load diff.

production...staging#diff-dd4bfa50713397aca8cb40145317936ab0affe19abc2a9c24d6c8849ed75dc9bR564

[techievivek]

I tried to guess which PR could have caused this from the titles, but at the end gave up and started reverting PRs one by one lol.

Woww 😄 🙇

[techievivek]

@bernhardoj Your RCA looks good to me, but I will wait for @tgolen to have a look since he authored the original PR.

[tgolen]

Wow, great RCA @bernhardoj. I confirmed that updating the memoization fixes the problem. I missed that in the original PR. I should be able to create a PR for that in a few minutes.

For the Onyx issues, it would be great to open a new GH for that and finally get it fixed. I think the explanation is great and that could be a big source of the problems with using selectors.

[tgolen]

PR is created here: #37504

[parasharrajat]

Reviewing the PR.

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

• Make sure to compare the new parentReportAction when memoizing Report… #37504

If no regressions arise, payment will be issued on 2024-03-07. 🎊

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

• [@tgolen / @techievivek] The PR that introduced the bug has been identified. Link to the PR: Remove deprecated method and pass down parentReportAction #37449
• [@tgolen / @techievivek] 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: https://github.com/Expensify/App/pull/37449/files#r1511541682
• [@tgolen / @techievivek] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1709574493639929
• [@tgolen / @techievivek] Determine if we should create a regression test for this bug. I don't think this is necessary since it was caught by existing tests
• [@tgolen / @techievivek] 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.
• [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.46-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Make sure to compare the new parentReportAction when memoizing Report… #37504

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

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

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

[OfstadC]

Adding this here for tracking

Affected Primary Email Address
oyaboardtreasurer@gmail.com

Concierge Chat
https://www.expensify.com/concierge/#/chat/17591280

[youssef-lr]

I think this should be fixed now.

[bernhardoj]

Just FYI, the appearance delay is still happening because of the Onyx issue.

[parasharrajat]

@bernhardoj may be you can propose this on Slack with previous details and suggest a solution as well.

[sakluger]

I'm kinda confused what's going on here. I think this PR was intended to fix the issue, but according to @bernhardoj, there is still a delay.

@techievivek @youssef-lr do either of you know what's going on?

[tgolen]

That PR was really intended to fix the page not loading at all when offline (the worst part). I couldn't see any noticeable delay when loading, but I also don't think it's really that related to any of the recent changes. It's more just that it takes a little time to read the data from Onyx, and that's OK. We could maybe think about implementing a loading state for this, but I don't know if it's really worth it. If you think it's a problem, I think we should open a new GH issue and get proposals for solving the delay.

[sakluger]

Thanks for explaining @tgolen. I agree that we've solved the main issue and we can close this out.

Is @parasharrajat due payment for the PR review, or no because it's a regression?

[tgolen]

@parasharrajat Still needs paid for a C+ review. The regression was caused by my PR so it's only a mark against me :D

[sakluger]

Thanks @tgolen for clarifying. 🙇

Summarizing payment on this issue:

Contributor+: @parasharrajat $500, please request on Newdot

[parasharrajat]

Payment requested as per #37466 (comment)

[JmillsExpensify]

$500 approved for @parasharrajat

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