[HOLD for payment 2024-12-20] [$250] iOS - Report - Conversation does not scroll down after deleting an attachment

[lanitochka17]

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: 9.0.64-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: 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/5241542
Issue reported by: Applause - Internal Team

Action Performed:

1. Open New Expensify app Hybrid
2. Go to the conversation with the history of the message
3. Send an image to the conversation
4. Delete the sent image

Expected Result:

The conversation should scroll down after deleting the attachment

Actual Result:

Conversation does not scroll down after deleting an attachment

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6671321_1732146742667.ScreenRecording_11-21-2024_04-47-49_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021859628438339074230
• Upwork Job ID: 1859628438339074230
• Last Price Increase: 2024-12-05

Issue Owner

Current Issue Owner: @Pujan92

[melvin-bot]

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

[bernhardoj]

Proposal

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

The chat doesn't scroll after deleting an attachment.

What is the root cause of that problem?

This happens because hasNewestReportAction is false.

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

Line 471 in c18812a

const shouldEnableAutoScroll = (hasNewestReportAction && (!reportActionID || !isNavigatingToLinkedMessage)) || (transactionThreadReport && !prevTransactionThreadReport);

hasNewestReportAction compares the last action created with the report lastVisibleActionCreated.

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

Line 288 in c18812a

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

When we delete the attachment, lastVisibleActionCreated is updated to the previous action created. However, the last action is still the deleted attachment.

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

Currently, we have 2 places of hasNewestReportAction logic. If we look at the one in ReportActionsList, we can see that the report actions are filtered, so only visible actions will be considered the last action.

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

Lines 185 to 197 in c18812a

	const sortedVisibleReportActions = useMemo(
	() =>
	sortedReportActions.filter(
	(reportAction) =>
	(isOffline ||
	ReportActionsUtils.isDeletedParentAction(reportAction) ||
	reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ||
	reportAction.errors) &&
	ReportActionsUtils.shouldReportActionBeVisible(reportAction, reportAction.reportActionID, ReportUtils.canUserPerformWriteAction(report)),
	),
	[sortedReportActions, isOffline, report],
	);
	const lastAction = sortedVisibleReportActions.at(0);

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

Line 360 in c18812a

const hasNewestReportAction = lastAction?.created === lastVisibleActionCreated;

So, the hasNewestReportAction in ReportActionList works correctly, but not in ReportActionsView. To fix this, we need to filter out the actions in ReportActionsView too. We can move the filtered actions from ReportActionsList

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

Lines 185 to 197 in c18812a

	const sortedVisibleReportActions = useMemo(
	() =>
	sortedReportActions.filter(
	(reportAction) =>
	(isOffline ||
	ReportActionsUtils.isDeletedParentAction(reportAction) ||
	reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ||
	reportAction.errors) &&
	ReportActionsUtils.shouldReportActionBeVisible(reportAction, reportAction.reportActionID, ReportUtils.canUserPerformWriteAction(report)),
	),
	[sortedReportActions, isOffline, report],
	);
	const lastAction = sortedVisibleReportActions.at(0);

to ReportActionsView. Then, use it for hasNewestReportAction.

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

Line 288 in c18812a

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

const visibleReportActions = useMemo(
    () => reportActions.filter(...),
    [reportActions, isOffline],
);

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

Then, pass the filtered actions to ReportActionsList.

<ReportActionsList
    sortedVisibleReportActions={visibleReportActions}

we can consider moving the hasNewestReportAction from ReportActionsList to ReportActionsView too, so we only have 1 logic for it

[melvin-bot]

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

[melvin-bot]

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

[Pujan92]

@puneetlath we can close this issue due to a similar issue has been created earlier and @bernhardoj can transfer their proposal there.

[bernhardoj]

Based on the proposal there, looks like the root cause and solution is different.

[Pujan92]

RCA of both issues looks similar to me where the issue remains in deriving the hasNewestReportAction due to the different values of last report action created and report lastVisibleActionCreated.

[bernhardoj]

Yes, both issues are because of the wrong value of hasNewestReportAction, but the root cause and solution are still different. I tested my solution and the other issue still persists, so I can't transfer my proposal there because it won't fix that issue.

[puneetlath]

Do you agree @Pujan92?

[melvin-bot]

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

[Pujan92]

@bernhardoj Why the issue appears only for ios and not for android? Bcoz if it is caused by hasNewestReportAction then it should have the same effect on other platforms too.

[bernhardoj]

I think the first issue of this blank space started to happen in #30726 which they fixed in #31670.

This is how the logic looks now which requires hasNewestReportAction.

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

Lines 372 to 381 in c4ff235

	useEffect(() => {
	if (
	scrollingVerticalOffset.current < AUTOSCROLL_TO_TOP_THRESHOLD &&
	previousLastIndex.current !== lastActionIndex &&
	reportActionSize.current > sortedVisibleReportActions.length &&
	hasNewestReportAction
	) {
	reportScrollManager.scrollToBottom();
	}
	previousLastIndex.current = lastActionIndex;

[melvin-bot]

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

[melvin-bot]

@puneetlath, @Pujan92 Eep! 4 days overdue now. Issues have feelings too...

[Pujan92]

We can proceed with @bernhardoj's proposal to filter actions to consider visible actions only to derive hasNewestReportAction.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

@puneetlath @Pujan92 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!

[bernhardoj]

PR is ready

cc: @Pujan92

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

• Fix chat isn't scrolled down when deleting attachment #53743

If no regressions arise, payment will be issued on 2024-12-20. 🎊

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

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

[melvin-bot]

@Pujan92 @Pujan92 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]

Issue is ready for payment but no BZ is assigned. @joekaufmanexpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[melvin-bot]

Payment Summary

Upwork Job

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

BugZero Checklist (@joekaufmanexpensify)

• 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/1859628438339074230/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[joekaufmanexpensify]

Asked in slack so we can get checklist done today.

[Pujan92]

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 both staging and production
• 2d. 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: https://github.com/Expensify/App/pull/30269/files#r1894124542

• [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: N/A

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

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue: https://github.com/Expensify/Expensify/issues/455241

Regression Test Proposal

1. Send an attachment to any chat
2. Delete the attachment
3. Verify the chat is scrolled to the bottom

Do we agree 👍 or 👎

[joekaufmanexpensify]

TY! I shared the regression test issue above, so checklist is all set.

[joekaufmanexpensify]

Good to issue payment. We need to pay:

• @bernhardoj - $250 for PR via NewDot.
• @Pujan92 - $250 for C+ via NewDot.

[joekaufmanexpensify]

@Pujan92 / @bernhardoj please request payment at your earliest convenience. Closing as this is otherwise all set!

[bernhardoj]

Requested in ND.

[garrettmknight]

$250 approved for @Pujan92

[JmillsExpensify]

$250 approved for @bernhardoj