[$250] Unread line not showing properly

[iwiznia]

Steps:

• Mark a message as unread. It adds the unread line and the bold to the LHN
• Switch to another chat
• Go back to the 1st cha that you marked as unread
• Check how the green unread line is not shown

This is only happening in main but not in staging, so has to be something we changed recently after the last deploy

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021902474591729150774
• Upwork Job ID: 1902474591729150774
• Last Price Increase: 2025-03-19

Issue Owner

Current Issue Owner: @jjcoffee

[melvin-bot]

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

[iwiznia]

Some conversations:
Public https://expensify.slack.com/archives/C01GTK53T8Q/p1742420025266599
Private https://expensify.slack.com/archives/C03TQ48KC/p1742417655726219

This is not a blocker right now, but will be one once we deploy whatever caused this to staging

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

🚨 Edited by proposal-police: This proposal was edited at 2025-03-20 04:48:15 UTC.

Proposal

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

Unread marker doesn't show when reopen the report.

What is the root cause of that problem?

This all happens after #58654. The PR is trying to fix an issue, where after we create a money request and then open the single expense report, both the money request fields view and its IOU preview are shown.

The expected result should look like this:

The root cause of the issue is explained here, basically, we lack the remote information, so the PR always shows a skeleton loader whenever the report is loading. However, I found it to be different.

The expected result can be achieved thanks to getCombinedReportActions.

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

Lines 175 to 180 in ca1755f

	// Get a sorted array of reportActions for both the current report and the transaction thread report associated with this report (if there is one)
	// so that we display transaction-level and report-level report actions in order in the one-transaction view
	const reportActions = useMemo(
	() => (reportActionsToDisplay ? getCombinedReportActions(reportActionsToDisplay, transactionThreadReportID ?? null, transactionThreadReportActions ?? []) : []),
	[reportActionsToDisplay, transactionThreadReportActions, transactionThreadReportID],
	);

getCombinedReportActions filters out any IOU action with a type of 'create'.

App/src/libs/ReportActionsUtils.ts

Lines 447 to 491 in ca1755f

	function getCombinedReportActions(
	reportActions: ReportAction[],
	transactionThreadReportID: string | null,
	transactionThreadReportActions: ReportAction[],
	reportID?: string,
	shouldFilterIOUAction = true,
	): ReportAction[] {
	const isSentMoneyReport = reportActions.some((action) => isSentMoneyReportAction(action));
	// We don't want to combine report actions of transaction thread in iou report of send money request because we display the transaction report of send money request as a normal thread
	if (isEmpty(transactionThreadReportID) || isSentMoneyReport) {
	return reportActions;
	}
	// Usually, we filter out the created action from the transaction thread report actions, since we already have the parent report's created action in `reportActions`
	// However, in the case of moving track expense, the transaction thread will be created first in a track expense, thus we should keep the CREATED of the transaction thread and filter out CREATED action of the IOU
	// This makes sense because in a combined report action list, whichever CREATED is first need to be retained.
	const transactionThreadCreatedAction = transactionThreadReportActions?.find((action) => action.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED);
	const parentReportCreatedAction = reportActions?.find((action) => action.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED);
	let filteredTransactionThreadReportActions = transactionThreadReportActions;
	let filteredParentReportActions = reportActions;
	if (transactionThreadCreatedAction && parentReportCreatedAction && transactionThreadCreatedAction.created > parentReportCreatedAction.created) {
	filteredTransactionThreadReportActions = transactionThreadReportActions?.filter((action) => action.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED);
	} else if (transactionThreadCreatedAction) {
	filteredParentReportActions = reportActions?.filter((action) => action.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED);
	}
	const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
	const isSelfDM = report?.chatType === CONST.REPORT.CHAT_TYPE.SELF_DM;
	// Filter out request and send money request actions because we don't want to show any preview actions for one transaction reports
	const filteredReportActions = [...filteredParentReportActions, ...filteredTransactionThreadReportActions].filter((action) => {
	if (!isMoneyRequestAction(action) || !shouldFilterIOUAction) {
	return true;
	}
	const actionType = getOriginalMessage(action)?.type ?? '';
	if (isSelfDM) {
	return actionType !== CONST.IOU.REPORT_ACTION_TYPE.CREATE;
	}
	return actionType !== CONST.IOU.REPORT_ACTION_TYPE.CREATE && actionType !== CONST.IOU.REPORT_ACTION_TYPE.TRACK;
	});
	return getSortedReportActions(filteredReportActions, true);
	}

However, in our case, isEmpty(transactionThreadReportID) returns true, so the report actions aren't combined.

App/src/libs/ReportActionsUtils.ts

Lines 456 to 459 in ca1755f

	// We don't want to combine report actions of transaction thread in iou report of send money request because we display the transaction report of send money request as a normal thread
	if (isEmpty(transactionThreadReportID) || isSentMoneyReport) {
	return reportActions;
	}

transactionThreadReportID contains a value, but in an integer. The lodash isEmpty returns false for an integer. If we see the type for the reportID, we can see it's a string, but BE returns it as an integer.

App/src/libs/ReportActionsUtils.ts

Line 449 in ca1755f

transactionThreadReportID: string | null,

When we create a money request, the optimistic transaction thread reportID is already in string, but the RequestMoney API, returns it in integer. Then, the OpenReport API returns it back to string. So, this is a BE bug after all.

(transactionThreadReportID is from the IOU action childReportID)

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

Revert #58654 and fix the BE bug. IF we can't fix the BE bug, we can update the FE code to.

App/src/libs/ReportActionsUtils.ts

Lines 457 to 459 in ca1755f

	if (isEmpty(transactionThreadReportID) || isSentMoneyReport) {
	return reportActions;
	}

// This will treat 0 (int) as empty too, which I believe is fine since we don't have a reportID of 0 anyway
if (!transactionThreadReportID || isSentMoneyReport) {

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

[adhorodyski]

Do you think this also solves the upstream issue? I tested this type myself but couldn't get it to work - might have missed something. I'd love to see the PR getting reverted tbh since it reverts offline-first improvements we made recently.

EDIT: I'll post recordings later today on all 3 scenarios (current state, revert only, revert + proposed changes)
EDIT2: We've tested all scenarios with @martasudol and here are the results:

Current
This is how we now force a skeleton for a slightly longer period of time, awaiting the API response.
https://github.com/user-attachments/assets/488d0b55-76f9-4fd9-88a5-5b4eaa02a941

Revert only
This represents the UI if we were to just revert the mentioned PR. As much as I'd love to do that, this leaves us with the intermediary step rendered from offline-available data.
https://github.com/user-attachments/assets/7beb3554-38b3-4eae-ba55-ecf254d089fb

Revert + Proposed changes
I was hoping for this to become true, but here is a reproduction of these 2 combined.
@bernhardoj, could you confirm this is true? Here, as much as I see the isEmpty() check being pointless (and this is clearly a js bug as well triggered by the wrong type coming from a BE response) - I also see that now we're still stuck with the intermediary step rendered (just a different one, still jumpy) which I don't think is what we initially aimed for.
https://github.com/user-attachments/assets/37b139c7-e5c8-4417-a4f7-7727a6fd1194

cc @shawnborton as you might be able to help us here. How do you like the 3rd option, proposed by @bernhardoj? In this scenario we'd go back to the eager rendering, but single expense reports will not load immediately in the full form.

[adhorodyski]

@bernhardoj for this issue we're currently in I honestly believe just showing/hiding a skeleton earlier/sooner does not have any impact on the green underline.

[bernhardoj]

for this issue we're currently in I honestly believe just showing/hiding a skeleton earlier/sooner does not have any impact on the green underline.

Hmm, but from my testing, reverting it fixes the issue.

Here, as much as I see the isEmpty() check being pointless

We don't want to combine the report actions if the transactionThreadReportID exists, so I don't think it's pointless in the current code (unless I misunderstood something).

I also see that now we're still stuck with the intermediary step rendered (just a different one, still jumpy) which I don't think is what we initially aimed for.

Hmm, I can't repro this one.

web.mp4

[adhorodyski]

Oh I can see you're opening it on the same account. In upstream, we focus on the received (external) expenses. Can you try on the other end?

[adhorodyski]

Can I ask you for before/after with the revert on your end? Let's see if we follow the same reproduction steps. It might be I've done things differently.

[iwiznia]

I am a bit confused, if I am reading this right, reverting #58654 will fix the green line issue and re-introduce the double rendering the PR was trying to fix, right?
If so, I don't see why we would not revert the PR and re-attempt the fix for the double rendering again.

but the RequestMoney API, returns it in integer. Then, the OpenReport API returns it back to string. So, this is a BE bug after all.

To clarify, you are saying that childReportID is int but should be string? Also are you saying that if we change that, the bug is gone?

[iwiznia]

BTW I just checked and I see childReportID is a string, so if you saw it being an int, then it must be in a specific case, can you share the list of exact steps to reproduce it?

[melvin-bot]

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

[melvin-bot]

💬 A slack conversation has been started in #expensify-open-source

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

⚠️ 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.

[iwiznia]

Just tested this and it's fixed