[HOLD for payment 2024-04-09] [$500] LOW - Android-Thread-Incorrect number of replies displayed for multilevel thread when going back

[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.4.49-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4411711
Issue reported by: Applause - Internal Team

Action Performed:

1. Open the app and log in
2. Navigate to any chat
3. Send a message
4. Start a thread
5. Create a multilevel tread until you get 5 levels
6. Tap the back icon

Expected Result:

The correct number of replies is displayed for every thread

Actual Result:

The number of replies does not change when navigating back to the parent thread. The count of replies updates after opening the multilevel thread

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

Bug6407285_1709922506831.video_2024-03-08_13-27-41.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017a1e94fc9637db19
• Upwork Job ID: 1767442147697999872
• Last Price Increase: 2024-03-12
• Automatic offers:
• fedirjh | Reviewer | 0
• bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @fedirjh

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp
CC @quinthar

[lanitochka17]

@bfitzexpensify 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

[bernhardoj]

Proposal

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

The thread replies number doesn't count the nested thread, except when we reopen the chat.

What is the root cause of that problem?

Currently, when we add a new comment to the thread, we only update the thread direct parent number of replies optimistically.

App/src/libs/actions/Report.ts

Lines 526 to 530 in 9302ade

	// Update optimistic data for parent report action if the report is a child report
	const optimisticParentReportData = ReportUtils.getOptimisticDataForParentReportAction(reportID, currentTime, CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);
	if (!isEmptyObject(optimisticParentReportData)) {
	optimisticData.push(optimisticParentReportData);
	}

App/src/libs/ReportUtils.ts

Lines 2912 to 2930 in 9302ade

	function getOptimisticDataForParentReportAction(reportID: string, lastVisibleActionCreated: string, type: string, parentReportID = '', parentReportActionID = ''): OnyxUpdate | EmptyObject {
	const report = getReport(reportID);
	if (!report || isEmptyObject(report)) {
	return {};
	}
	const parentReportAction = ReportActionsUtils.getParentReportAction(report);
	if (!parentReportAction || isEmptyObject(parentReportAction)) {
	return {};
	}
	const optimisticParentReportAction = updateOptimisticParentReportAction(parentReportAction, lastVisibleActionCreated, type);
	return {
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID || report?.parentReportID}`,
	value: {
	[parentReportActionID || (report?.parentReportActionID ?? '')]: optimisticParentReportAction,
	},
	};
	}

App/src/libs/ReportUtils.ts

Lines 2869 to 2902 in 9302ade

	function updateOptimisticParentReportAction(parentReportAction: OnyxEntry<ReportAction>, lastVisibleActionCreated: string, type: string): UpdateOptimisticParentReportAction {
	let childVisibleActionCount = parentReportAction?.childVisibleActionCount ?? 0;
	let childCommenterCount = parentReportAction?.childCommenterCount ?? 0;
	let childOldestFourAccountIDs = parentReportAction?.childOldestFourAccountIDs;
	if (type === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
	childVisibleActionCount += 1;
	const oldestFourAccountIDs = childOldestFourAccountIDs ? childOldestFourAccountIDs.split(',') : [];
	if (oldestFourAccountIDs.length < 4) {
	const index = oldestFourAccountIDs.findIndex((accountID) => accountID === currentUserAccountID?.toString());
	if (index === -1) {
	childCommenterCount += 1;
	oldestFourAccountIDs.push(currentUserAccountID?.toString() ?? '');
	}
	}
	childOldestFourAccountIDs = oldestFourAccountIDs.join(',');
	} else if (type === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
	if (childVisibleActionCount > 0) {
	childVisibleActionCount -= 1;
	}
	if (childVisibleActionCount === 0) {
	childCommenterCount = 0;
	childOldestFourAccountIDs = '';
	}
	}
	return {
	childVisibleActionCount,
	childCommenterCount,
	childLastVisibleActionCreated: lastVisibleActionCreated,
	childOldestFourAccountIDs,
	};
	}

After we reopen the app, the BE will give us the correct number of replies.

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

Update all the report parent and ancestors number of replies. To do that, we can either modify getOptimisticDataForParentReportAction or create a new function called getOptimisticDataForAncestorReportActions.

We will get the list of the ancestors from getAllAncestorReportActionIDs and update each ancestor thread data.

// getOptimisticDataForParentReportAction
...
const ancestors = getAllAncestorReportActionIDs(report);
const totalAncestor = ancestors.reportIDs.length;

return Array.from(Array(totalAncestor), (_, index) => {
    const ancestorReport = getReport(ancestors.reportIDs[index]);
    const ancestorReportAction = ReportActionsUtils.getReportAction(ancestorReport.reportID, ancestors.reportActionsIDs[index]);
    return {
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${ancestorReport.reportID}`,
        value: {
            [ancestorReportAction?.reportActionID ?? '']: updateOptimisticParentReportAction(ancestorReportAction, lastVisibleActionCreated, type),
        },
    };
})

And because it now returns an array of onyx update, we need to modify the add comment code to push the data one by one.

const optimisticParentReportData = ReportUtils.getOptimisticDataForParentReportAction(...)
optimisticParentReportData.forEach((parentReportData) => {
    if (!isEmptyObject(parentReportData)) {
        optimisticData.push(parentReportData);
    }
})

[melvin-bot]

@bfitzexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017a1e94fc9637db19

[melvin-bot]

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

[bfitzexpensify]

Added to the VSB project with a LOW priority

[bfitzexpensify]

Awaiting a volunteer

[bfitzexpensify]

Same update

[fedirjh]

@bernhardoj Your proposal makes sense to me. Did you test the solution on native? I'm particularly concerned about its potential impact on the performance of adding comments, especially as the thread depth increases.

[bernhardoj]

Before:

Screen.Recording.2024-03-16.at.16.03.00.mov

After:

Screen.Recording.2024-03-16.at.16.00.32.mov

@fedirjh I feel like there is no performance degradation noticeable after applying the solution. If I'm correct, the onyx will merge it asynchronously. Also, we use FreezeWrapper so only the current and previous screen will re-render when the onyx is updated.

App/src/libs/Navigation/FreezeWrapper.tsx

Lines 26 to 30 in 2979e3c

	const unsubscribe = navigation.addListener('state', () => {
	// if the screen is more than 1 screen away from the current screen, freeze it,
	// we don't want to freeze the screen if it's the previous screen because the freeze placeholder
	// would be visible at the beginning of the back animation then
	if ((navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0) > 1) {

[melvin-bot]

@fedirjh, @bfitzexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[fedirjh]

Thanks for confirming @bernhardoj; The result seems satisfactory. We can proceed with further testing in the PR.

Let's proceed with @bernhardoj proposal.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @fedirjh 🎉 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]

📣 @bernhardoj 🎉 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 📖

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[bernhardoj]

It's still reproducible.

PR is ready

cc: @fedirjh

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

• Correctly update thread ancestors reply count #38658

If no regressions arise, payment will be issued on 2024-04-09. 🎊

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

• @fedirjh requires payment automatic offer (Reviewer)
• @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:

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

[bfitzexpensify]

Payment summary:

Contributor: $500 to @bernhardoj - completed via Upwork ✅
C+: $500 to @fedirjh - @fedirjh, please complete the BZ checklist and I'll get this paid out!

[melvin-bot]

@puneetlath, @fedirjh, @bfitzexpensify, @bernhardoj Whoops! This issue is 2 days overdue. Let's get this updated quick!

[fedirjh]

BugZero Checklist:

• Link to the PR: Update optimistic data for parent report action when completing or reopening task #22990
• Link to comment: Update optimistic data for parent report action when completing or reopening task #22990 (comment)
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: No need for a regression test, it's a tiny bug that doesn’t affect the UX.

[bfitzexpensify]

Thanks @fedirjh - agree with your assessment.

Payment complete, we're all done here - thanks everyone!