[$2000] Attachment is being displayed as the last message on LHN instead of the actual last message

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to any chat
2. Click on +
3. Click on Add attachment
4. Choose any attachment
5. Click Send
6. Before the attachment gets uploaded, send any text

Expected Result:

The text sent after attachment should be displayed on LHN

Actual Result:

Attachment is being displayed as the last message on LHN

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.89-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Expensify/Expensify Issue URL:
Issue reported by: @adeel0202
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1679851671259879

View all open jobs on GitHub

Recording.65.mp4

Screen.Recording.2023-03-26.at.10.16.11.PM.mov

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016f332cd8af2af20b
• Upwork Job ID: 1640316106037559296
• Last Price Increase: 2023-06-15

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

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

[MelvinBot]

Current assignee @dylanexpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

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

[tienifr]

Proposal

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

If we send an attachment, then immediately send a text message, LHN will preview the attachment instead of the newest text.

What is the root cause of that problem?

Let's look at the current flow of the situation

Send attachment -> Submit to API -> Send text -> Receive text pusher notification -> Receive attachment pusher notification.

Notice that the pusher notification from the latter text is received first. This is reasonable since uploading and processing an attachment should take much longer than sending a simple text. Since the pusher notification contains lastMessageText information, this cause Onyx to use the latter [Attachment] value for lastMessageText. Therefore, because OptionRowLHN use this value to render the preview message, it will render the [Attachment] text instead.

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

We should have a logic to check if the information coming from Onyx is the latest message. We will not update the report data rendered by OptionRowLHN if there has been a previous message that is more recent.

In SidebarUtils.js, we can have some check like this

+            // only update if the updated onyx data is the most recent
+            if (!chatReports[key] || chatReports[key].lastVisibleActionCreated < report.lastVisibleActionCreated)
                chatReports[key] = report;

What alternative solutions did you explore? (Optional)

We can apply the same check, but with some other indicative fields such as lastReadTimestamp, lastReadTime, lastReadTime, maxSequenceNumber.

Alternatively, we could apply the check in the Onyx update function. If the new data is not more recent, we will not update it in Onyx in the first place. However, I don't think this will be as clean as the change mentioned above.

Result

Working well after the fix

Screen.Recording.2023-03-27.at.11.42.47.mov

[bernhardoj]

I think the issue will be fixed once we stop send Pusher event back to the client from which the event originated. https://expensify.slack.com/archives/C01GTK53T8Q/p1678131839265059?thread_ts=1678029220.184279&cid=C01GTK53T8Q

cc @roryabraham

[s77rt]

@pecanoro Let's wait for @roryabraham input here.

[hellohublot]

Proposal

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

Attachment is being displayed as the last message on LHN instead of the actual last message

What is the root cause of that problem?

The reason is the same as this issue #15998, it is the order problem of updating onyx after the socket receives the messages

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

so I would suggest using the same solution
#15998 (comment)

What alternative solutions did you explore? (Optional)

Not Yet

[roryabraham]

Yes, I can confirm this is an instance of the "replay effect", which @neil-marcellini has been implementing here. Putting this on HOLD

[pecanoro]

Still on HOLD!

[rojiphil]

@s77rt Thanks for your feedback.

The root cause of the problem lies in this line

When an array of objects are passed into Onyx.update(), the updates are not guaranteed to happen in the same order of array items as Onyx.update implementation does not guarantee that.

The solution would be to iterate through QueuedOnyxUpdates and ensure that each Onyx.update is successfully done before proceeding with the next one.

Please let me know if the RCA sounds good to you and I can update the proposal accordingly.

[s77rt]

@rojiphil Thanks for the quick follow up. The onyx update part in our case seems to be handled by

App/src/libs/actions/User.js

Lines 611 to 619 in 5296b4e

	function subscribeToUserDeprecatedEvents() {
	// Receive any relevant Onyx updates from the server
	PusherUtils.subscribeToPrivateUserChannelEvent(Pusher.TYPE.ONYX_API_UPDATE, currentUserAccountID, (pushJSON) => {
	SequentialQueue.getCurrentRequest().then(() => {
	Onyx.update(pushJSON);
	triggerNotifications(pushJSON);
	});
	});
	}

Thus your RCA does not seem correct, even if we replace QueuedOnyxUpdates.queueOnyxUpdates with Onyx.update the issue would still be reproducible.

[rojiphil]

Even if we replace QueuedOnyxUpdates.queueOnyxUpdates with Onyx.update the issue would still be reproducible.

@s77rt Yes. You are right. Even if we replace QueuedOnyxUpdates.queueOnyxUpdates with Onyx.update, the issue would still be reproducible. However, please note that the problem is when we pass an array of updates into the Onyx.update. Instead, if we call Onyx.update for individual updates and wait for it to return before calling the next one, the data would be stored correctly.

The onyx update part in our case seems to be handled by

App/src/libs/actions/User.js

Lines 611 to 619 in 5296b4e

I do not understand how, in our case, the onyx update part comes to this. Can you please share more details on this?

What I observed was that once QueuedOnyxUpdates.queueOnyxUpdates is called, the data stored in indexedDB was incorrect even though the API responses were in order. This resulted in lastMessageText data being incorrect when the contents of LHN row were fetched.

Meanwhile, I also tested this out and it seems to solve the issue. I have updated the proposal with a test video too. Kindly check.

[neil-marcellini]

What is the root cause of that problem?

The root cause of the problem lies in this line

When an array of objects are passed into Onyx.update(), the updates are not guaranteed to happen in the same order of array items as Onyx.update implementation does not guarantee that.

@rojiphil It's not clear to me if this is the actual root cause, or if there is some other more simple explanation. Let's wait to retest this until we deploy a backend fix to properly avoid sending Push events to the requesting client. I'll provide updates about that on the replay effect tracking issue. #12775

That being said, I agree that the updates are not guaranteed to happen in order. I did an investigation on that here. Please feel free to check that out and comment there, DM me on NewDot, or create an issue in react-native-onyx. If we decide it needs to be fixed than we should fix it in Onyx.

[s77rt]

@rojiphil I think something is wrong about subscribeToUserDeprecatedEvents. What you described is not the RCA, you can still notice the last message in LHN changes. I'm not sure if we are even supposed to get calls to subscribeToUserDeprecatedEvents. If we overwrite this function to a nop function, all would work well and the onyx data will be updated just from SaveResponseInOnyx middleware. Currently onyx data is updated from both subscribeToUserDeprecatedEvents and SaveResponseInOnyx which is very questionable and looks wrong.

[s77rt]

@pecanoro or @neil-marcellini Can we put this on hold again for #12775?

[pecanoro]

Back to on hold 😄 I have hope it will get resolved this time 🤞

[conorpendergrast]

Making this Weekly to match the cadence of #12775

[conorpendergrast]

No change this week; still held on #12775

[s77rt]

Still on hold? @neil-marcellini

[neil-marcellini]

Yes unfortunately, but it should be off hold soon. See the update here.

[neil-marcellini]

Off hold, you can retest on staging!

[conorpendergrast]

I will do so tomorrow!

[conorpendergrast]

Alright, so, this is no longer reproducible on staging! I vote we pay the bug reporter (@adeel0202) and close this out. Any disagreement? Unless I hear otherwise, I'll do that tomorrow

[melvin-bot]

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

[conorpendergrast]

Ok, I'm now paying @adeel0202 and then closing this out

[conorpendergrast]

Contract sent to @adeel0202 via Upwork

[adeel0202]

Accepted, thanks.

[conorpendergrast]

And paid. All done, closing out. Thanks!

[adeel0202]

Thanks @conorpendergrast. Please end the contract on Upwork too.

[conorpendergrast]

@adeel0202 Upwork wouldn't let me do it on Friday, and that's done now!