[$500] Attachment - 'New message' button appears when deleting attachment

[izarutskaya]

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: 1.4.59-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4468722
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Navigate to a report has multiple chat
2. Send attachment
3. Delete the attachment
4. Click on 'New messages' button

Expected Result:

'New messages' button shouldn't appear, and attachment should remain deleted

Actual Result:

'New messages' button appears, and attachment reappear when clicking 'New messages' button

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

Bug6435643_1712067883261.Screen_Recording_2024-04-02_at_5.16.43_in_the_afternoon.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010fcec62910c073e7
• Upwork Job ID: 1775234083431460864
• Last Price Increase: 2024-04-30
• Automatic offers:
• jjcoffee | Reviewer | 0
• c3024 | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[izarutskaya]

@garrettmknight 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.

[izarutskaya]

We think this issue might be related to the #vip-vsb.

[izarutskaya]

Production

bandicam.2024-04-02.19-07-13-703.mp4

[deetergp]

I was able to reproduce this locally. Also navigating away from the chat & back will cause the supposedly deleted attachment to reappear.

[garrettmknight]

Rerpo'd it as well. I think this could be External so I'll start there.

[melvin-bot]

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

[melvin-bot]

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

[jasperhuangg]

I just reproduced this on prod so it doesn't need to be a blocker

[kaushiktd]

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

Attachment - 'New message' button appears when deleting attachment

What is the root cause of that problem?

The fundamental issue stems from the inaccurate calculation of the canScrollToNewerComments variable, which governs the display of the "New Messages" button. Currently, certain conditions, including !isLoadingInitialReportActions, !hasNewestReportAction, and !isLastPendingActionIsDelete, are incorrectly inverted, resulting in the button being erroneously shown even when there are no unread messages.

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

to solve this problem change here

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

Line 614 in b0c4178

const canScrollToNewerComments = !isLoadingInitialReportActions && !hasNewestReportAction && sortedReportActions.length > 25 && !isLastPendingActionIsDelete;

const canScrollToNewerComments = isLoadingInitialReportActions && hasNewestReportAction && sortedReportActions.length > 25 && isLastPendingActionIsDelete;

canScrollToNewerComments: This variable determines whether the "scroll to newer comments" button should be displayed. It evaluates to true if the following conditions are met:

• isLoadingInitialReportActions: Indicates whether the report actions are still loading initially.
• hasNewestReportAction: Indicates whether there is a newest report action available.
• sortedReportActions.length > 25: Ensures that there are more than 25 sorted report actions in the list.
• isLastPendingActionIsDelete: Indicates whether the last pending action is a delete action.

The proposed fix adjusts the canScrollToNewerComments variable to ensure that it evaluates to false when it should not override the other conditions. By modifying the condition to include the correct logic without the use of the logical NOT operator, we ensure that the button is displayed only when necessary.
By accurately evaluating the conditions and addressing the root cause of the issue, we ensure that the button displays correctly in all relevant scenarios

video:-https://drive.google.com/file/d/1dBX9XXeHCps6-OU2qmPJvhpcDts-9p5j/view?usp=sharing

[jjcoffee]

Reviewing Monday!

[jjcoffee]

@kaushiktd Thanks, could you explain though what condition (or combination) is causing the button to display in this particular case? I think it would be reckless to flip the logic on all the conditions without understanding why 😄

I feel like your fix probably only works because your changes evaluate canScrollToNewerComments as false and so it no longer overrides the other conditions here:

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

Line 618 in b0c4178

isActive={(isFloatingMessageCounterVisible && !!currentUnreadMarker) || canScrollToNewerComments}

[kaushiktd]

@jjcoffee

canScrollToNewerComments: This variable determines whether the "scroll to newer comments" button should be displayed. It evaluates to true if the following conditions are met:

• isLoadingInitialReportActions: Indicates whether the report actions are still loading initially.
• hasNewestReportAction: Indicates whether there is a newest report action available.
• sortedReportActions.length > 25: Ensures that there are more than 25 sorted report actions in the list.
• isLastPendingActionIsDelete: Indicates whether the last pending action is a delete action.
  The proposed fix adjusts the canScrollToNewerComments variable to ensure that it evaluates to false when it should not override the other conditions. By modifying the condition to include the correct logic without the use of the logical NOT operator, we ensure that the button is displayed only when necessary.

By accurately evaluating the conditions and addressing the root cause of the issue, we ensure that the button displays correctly in all relevant scenarios

[jjcoffee]

@kaushiktd Flipping the conditions will likely break the expected behaviour from the comment above the code you're proposing to change:

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

Lines 612 to 614 in b0c4178

	// When performing comment linking, initially 25 items are added to the list. Subsequent fetches add 15 items from the cache or 50 items from the server.
	// This is to ensure that the user is able to see the 'scroll to newer comments' button when they do comment linking and have not reached the end of the list yet.
	const canScrollToNewerComments = !isLoadingInitialReportActions && !hasNewestReportAction && sortedReportActions.length > 25 && !isLastPendingActionIsDelete;

[jjcoffee]

Friendly bump @bernhardoj in case you're available and want to take a look at this, since you worked on a related PR recently.

[kaushiktd]

@kaushiktd Flipping the conditions will likely break the expected behaviour from the comment above the code you're proposing to change:

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

Lines 612 to 614 in b0c4178

	// When performing comment linking, initially 25 items are added to the list. Subsequent fetches add 15 items from the cache or 50 items from the server.
	// This is to ensure that the user is able to see the 'scroll to newer comments' button when they do comment linking and have not reached the end of the list yet.
	const canScrollToNewerComments = !isLoadingInitialReportActions && !hasNewestReportAction && sortedReportActions.length > 25 && !isLastPendingActionIsDelete;

@jjcoffee As I've mentioned in my last response, by evacuating the conditions the behaviour is expected and will not break.🙂 Still if there is a way for you to test it, I'd love to see the results because it is working as expected for me.

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

@deetergp @garrettmknight @jjcoffee this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[jjcoffee]

@kaushiktd My problem with your proposal as it stands is that it lacks a clear RCA and the explanatory link to the solution, i.e. why does it work. If you can explain how flipping the isLoadingInitialReportActions and hasNewestReportAction conditions won't break the comment linking functionality, that would help.

In the meantime open to new proposals!

[kaushiktd]

@jjcoffee proposal updated

[c3024]

Proposal

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

'New messages' floating button appears when the latest message is deleted after crossing a threshold.

What is the root cause of that problem?

We show the floating message counter when hasNewestReportAction is false among other things here

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

Lines 616 to 622 in 573d989

	// When performing comment linking, initially 25 items are added to the list. Subsequent fetches add 15 items from the cache or 50 items from the server.
	// This is to ensure that the user is able to see the 'scroll to newer comments' button when they do comment linking and have not reached the end of the list yet.
	const canScrollToNewerComments = !isLoadingInitialReportActions && !hasNewestReportAction && sortedReportActions.length > 25 && !isLastPendingActionIsDelete;
	return (
	<>
	<FloatingMessageCounter
	isActive={(isFloatingMessageCounterVisible && !!currentUnreadMarker) || canScrollToNewerComments}

but hasNewestReportAction here compares the latest sortedReportAction

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

Line 203 in 573d989

const hasNewestReportAction = sortedReportActions?.[0].created === report.lastVisibleActionCreated;

but latest sortedReportActions include deleted actions and the check fails even though we are already at the latest message because the latest sortedReportAction created time is of the deleted reportAction's whereas the report's lastVisibleActionCreated is of the present latest message creation time. This check fails and the floating button appears

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

We have sortedVisibleReportActions here

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

Lines 192 to 199 in 573d989

	const sortedVisibleReportActions = useMemo(
	() =>
	sortedReportActions.filter(
	(reportAction) =>
	(isOffline || reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || reportAction.errors) &&
	ReportActionsUtils.shouldReportActionBeVisible(reportAction, reportAction.reportActionID),
	),
	[sortedReportActions, isOffline],

and this omits the deleted action here

App/src/libs/ReportActionsUtils.ts

Line 539 in 573d989

return !isDeleted || isPending || isDeletedParentAction(reportAction) || isReversedTransaction(reportAction);

So, change the check for hasNewestReportAction above to use sortedVisibleReportActions?.[0].created

What alternative solutions did you explore? (Optional)

[dragnoir]

Proposal

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

'New message' button appears when deleting attachment

What is the root cause of that problem?

The logic behind canScrollToNewerComments is using a wrong conditions

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

Line 618 in 573d989

const canScrollToNewerComments = !isLoadingInitialReportActions && !hasNewestReportAction && sortedReportActions.length > 25 && !isLastPendingActionIsDelete;

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

I suggest implementing a new logic for the "New Message" floating button, similar to the functionality observed in Slack. This button should only appear under specific conditions:

1. If there are new unread messages.
2. If the user's scrolling position is above the first unread message.

This approach ensures that the button is both contextual and minimally intrusive, improving user experience by providing a quick way to navigate to the first unread message.

1. State Management:

   • Maintain a state variable hasUnreadMessages to track if there are unread messages.
   • Use another state variable firstUnreadMessageIndex to store the index of the first unread message in your messages array.

2. Scroll Position Detection:

   • Attach an onScroll event handler to determine the scroll position. Use the event.nativeEvent.contentOffset.y to get the vertical scroll position.

3. Display Logic for Button:

   • Determine the position of the first unread message relative to the scroll view.
   • Compare the current scroll position with this calculated position. If the scroll position is above this point and hasUnreadMessages is true, set another state variable showFloatingButton to true.

By following these steps, we can create a user-friendly navigation feature that closely mirrors the convenient functionality seen in Slack.

[jjcoffee]

The approach in @c3024's proposal LGTM! It also explains why the scroll button appears after a short delay; report.lastVisibleActionCreated is populated once the BE request completes. We have to make sure that any changes to hasNewestReportAction don't have knock-on effects, or maybe introduce a new hasNewestVisibleReportAction to be safe 😄

I don't think we want to rework the whole way this feature works, as @dragnoir suggests.

@kaushiktd's proposal lacks the detail for me to fully analyse it. I strongly doubt it's a workable fix.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[bernhardoj]

Looks like the selected solution is the same as in my PR here

[melvin-bot]

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

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

[jjcoffee]

Looks like the selected solution is the same as in my PR here

@bernhardoj Thanks for the heads up! Hmm I'm not sure what we'd normally do in this situation. This issue is older so it would normally take precedence, but the other issue is also not a straight dupe of this one...

[jjcoffee]

Just a heads up I'll be OOO 6-13th (I may have some time on the 6th), so this may need to be reassigned depending on what is decided to do here. cc @garrettmknight

[kaushiktd]

@garrettmknight I pulled latest code and this issue appears to be fixed!

[melvin-bot]

@deetergp, @garrettmknight, @jjcoffee, @c3024 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[deetergp]

I asked for a re-test here https://expensify.slack.com/archives/C9YU7BX5M/p1715052903386669

[kavimuru]

Issue is fixed.

Screen.Recording.2024-05-07.at.3.42.40.in.the.afternoon.mp4

[deetergp]

@garrettmknight I think we can close this one out.

[deetergp]

Is there anything we need to do before closing this one @garrettmknight?

[garrettmknight]

Just reviewing - doesn't look like we got a PR up or anything so I'm going to just close since this was fixed elsewhere.