[HOLD for payment 2025-01-30] [$250] Expense - After editing expense details, page not scrolled and focused to system message

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number:
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Open 1:1 chat
3. Submit an expense
4. Enter description and merchant

Expected Result:

After editing expense details, page must be scrolled and focused to system message.

Actual Result:

After editing expense details, page not scrolled and focused to system message.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6631952_1728660268703.screenrecorder-2024-10-11-18-34-05-385_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021845669426671518848
• Upwork Job ID: 1845669426671518848
• Last Price Increase: 2025-01-13

Issue Owner

Current Issue Owner: @VictoriaExpensify

[melvin-bot]

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

[IuliiaHerets]

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

[mkzie2]

Proposal

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

After editing expense details, page not scrolled and focused to system message.

What is the root cause of that problem?

We don't notifyNewAction when updating all fields of money request

App/src/libs/actions/IOU.ts

Line 5574 in 1220039

API.write(WRITE_COMMANDS.UPDATE_MONEY_REQUEST_AMOUNT_AND_CURRENCY, params, onyxData);

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

We should call notifyNewAction to scroll to the bottom after we edit the money request

Report.notifyNewAction(transactionThreadReportID, Report.getCurrentUserAccountID(), params.reportActionID);

App/src/libs/actions/IOU.ts

Line 5574 in 1220039

API.write(WRITE_COMMANDS.UPDATE_MONEY_REQUEST_AMOUNT_AND_CURRENCY, params, onyxData);

OPTIONAL: We can check if the transaction thread report is one transaction thread we will notifyNewAction to parentReport

We should do the same for other update money request functions.

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

Edited by proposal-police: This proposal was edited at 2025-01-10 04:26:21 UTC.

Proposal

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

The edit system message is not shown after editing the money request.

What is the root cause of that problem?

In the video, we can see that the user is at the bottom of the chat list, but the new edit system message doesn't show. Our FlatList custom component has a config to auto-scroll to the top (or bottom because it's inverted) if the user is within the scroll threshold but only if shouldEnableAutoScrollToTopThreshold is true.

App/src/components/InvertedFlatList/BaseInvertedFlatList/index.tsx

Lines 15 to 26 in 7814eed

	const maintainVisibleContentPosition = useMemo(() => {
	const config: ScrollViewProps['maintainVisibleContentPosition'] = {
	// This needs to be 1 to avoid using loading views as anchors.
	minIndexForVisible: 1,
	};
	if (shouldEnableAutoScrollToTopThreshold) {
	config.autoscrollToTopThreshold = AUTOSCROLL_TO_TOP_THRESHOLD;
	}
	return config;
	}, [shouldEnableAutoScrollToTopThreshold]);

We only enable shouldEnableAutoScrollToTopThreshold if we have the newest report action (hasNewestReportAction)

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

Lines 473 to 474 in 7814eed

	// AutoScroll is disabled when we do linking to a specific reportAction
	const shouldEnableAutoScroll = (hasNewestReportAction && (!reportActionID || !isNavigatingToLinkedMessage)) || (transactionThreadReport && !prevTransactionThreadReport);

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

Line 495 in 7814eed

shouldEnableAutoScrollToTopThreshold={shouldEnableAutoScroll}

hasNewestReportAction compares the last report action created with the report lastVisbileActionCreated.

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

Line 289 in 7814eed

const hasNewestReportAction = reportActions.at(0)?.created === report.lastVisibleActionCreated || reportActions.at(0)?.created === transactionThreadReport?.lastVisibleActionCreated;

In our case, when updating the description, for example, the new report action is added earlier before the report lastVisibleActionCreated is updated, so hasNewestReportAction is false.

Why the report lastVisibleActionCreated is late to be updated? If we see here, both report action and report are added to the optimistic data with MERGE operation.

App/src/libs/actions/IOU.ts

Lines 2560 to 2574 in 7814eed

	optimisticData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThread?.reportID}`,
	value: {
	[updatedReportAction.reportActionID]: updatedReportAction as OnyxTypes.ReportAction,
	},
	});
	optimisticData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThread?.reportID}`,
	value: {
	lastVisibleActionCreated: updatedReportAction.created,
	lastReadTime: updatedReportAction.created,
	},
	});

However, inside Onyx.update, if there are multiple collection member keys with the same collection key, they will be batched into using mergeCollection. In our case, we are updating the IOU report and transaction thread report and mergeCollection is slower than individual merge by ~150ms.

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

Update the hasNewestReportAction to return true if the lastVisibleActionCreated is bigger than the last action created time.

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

Lines 302 to 303 in fe6cfc2

	const hasNewestReportAction =
	visibleReportActions.at(0)?.created === report.lastVisibleActionCreated || visibleReportActions.at(0)?.created === transactionThreadReport?.lastVisibleActionCreated;

const lastAction = visibleReportActions.at(0);
const hasNewestReportAction =
    lastAction?.created && ((report.lastVisibleActionCreated && lastAction.created >= report.lastVisibleActionCreated) || (transactionThreadReport?.lastVisibleActionCreated && lastAction?.created >= transactionThreadReport.lastVisibleActionCreated));

What alternative solutions did you explore? (Optional)

Alternative 1
The simplest solution is to change from MERGE to SET.

App/src/libs/actions/IOU.ts

Lines 2567 to 2574 in 7814eed

	optimisticData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThread?.reportID}`,
	value: {
	lastVisibleActionCreated: updatedReportAction.created,
	lastReadTime: updatedReportAction.created,
	},
	});

optimisticData.push({
    onyxMethod: Onyx.METHOD.SET,
    key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThread?.reportID}`,
    value: {
        ...transactionThread,
        lastVisibleActionCreated: updatedReportAction.created,
        lastReadTime: updatedReportAction.created,
    },
});

The batch logic will still be executed, but because we use SET, a multiSet operation will be used and it's (subs update) faster.

Alternative 2
I think I don't like the idea of automatically changing the MERGE to MERGE_COLLECTION (or multi set) when there are multiple same collection keys we are going to update because now we can't explicitly tell Onyx.update to individually update a key which we needed to solve this issue.

Before this automatic MERGE to MERGE_COLLECTION change, we used MERGE to merge individual keys and MERGE_COLLECTION to merge the collection key, which is predictable, but now, MERGE could change to MERGE_COLLECTION internally even if we want to merge it individually.

I know it would be a lot of reactors to change all action files to use MERGE_COLLECTION instead of MERGE, so, to solve that, we need to have a new attribute to each Onyx update called allowBatchedCollectionUpdate.

allowBatchedCollectionUpdate marks an update to never merge it as a collection or using multi set.

optimisticData.push({
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThread?.reportID}`,
    allowBatchedCollectionUpdate: false,
    value: {
        lastVisibleActionCreated: updatedReportAction.created,
        lastReadTime: updatedReportAction.created,
    },
});

Then,

1. Add a new object here called nonBatchableUpdate.
2. When iterating over the updates array, add any update that is marked with allowBatchedCollectionUpdate to the new object above.

data.forEach(({ onyxMethod, key, value, allowBatchedCollectionUpdate = true }) => {
    ...
    if (!allowBatchedCollectionUpdate) {
        nonBatchableUpdate[key] = true;
    }
})

3. Filter out any key that shouldn't be batched update here

const collectionItemKeys = Object.keys(updateQueue).filter((key) => !nonBatchableUpdate[key] && OnyxUtils_1.default.isKeyMatch(collectionKey, key));

[melvin-bot]

@rojiphil, @VictoriaExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[VictoriaExpensify]

hey @rojiphil - can you please review @bernhardoj 's proposal when you have a chance?

[melvin-bot]

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

[rojiphil]

Sorry for the delay here. I will review this my day tomorrow and update.

[rojiphil]

Is anyone able to reproduce this? I am unable to reproduce as demonstrated below.

50659-nor-reproduced.mp4

[bernhardoj]

Still reproducible

web.mp4

[melvin-bot]

@rojiphil @VictoriaExpensify 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]

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

[rojiphil]

@stitesExpensify Gentle bump for your review on comment here. Thanks

[stitesExpensify]

Oops! Assigned!

[bernhardoj]

PR is ready

cc: @rojiphil

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

• Fix auto scroll doesn't work when editing money request #55340

If no regressions arise, payment will be issued on 2025-01-30. 🎊

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

• @rojiphil requires payment through NewDot Manual Requests
• @bernhardoj requires payment through NewDot Manual Requests

[melvin-bot]

@rojiphil @VictoriaExpensify @rojiphil The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @rojiphil owed $250 via NewDot
• Reviewer: @bernhardoj owed $250 via NewDot

BugZero Checklist (@VictoriaExpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1845669426671518848/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[rojiphil]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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:

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: Not Required. The existing checklist is good enough to capture such issues.

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal

Precondition:

Test:

1. Create a money expense
2. Open the transaction report
3. Make sure it’s scrollable but within the scrollable threshold limit
4. Edit the description
5. Verify that the system message is displayed in the report.
6. Repeat (4) and (5) until the scrollable threshold limit is reached and verify that the autoscroll stops after reaching the threshold limit

Do we agree 👍 or 👎

[VictoriaExpensify]

Test rail issue raised - https://github.com/Expensify/Expensify/issues/466788

[VictoriaExpensify]

Payment summary:
C: @bernhardoj owed $250 via NewDot
C+: @rojiphil owed $250 via NewDot

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj