Reaction emoji disappears/reappears after quickly adding/removing reactions

[kavimuru]

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

---

Action Performed:

1. Go to any chat

2. Type a text

3. Long press and react to some emojis , Please do select 3 / 4 emojis ( using add emoji button)

4. Notice that you have now multiple emoji reactions on that typed text.

5. Now unselect those emojis

   Notice that after unselecting all emojis, they reappear

Expected Result:

After unselecting emojis, they should not reappear

Actual Result:

When all emojis are unselected, usually one or all emojis are restored. Further, if new text is typed below. Such restoration doesn’t last.

Workaround:

unknown

Platforms:

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

• [x] Android / native ( Device model : one plus nord)
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [ ] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.2.87-1 Reproducible in staging?: Needs reproduction Reproducible in production?: Needs reproduction 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:

https://user-images.githubusercontent.com/43996225/227727393-9e12dfb6-a7a9-403b-9feb-ffb7fc8e72a5.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016e24bec63a2e084f
• Upwork Job ID: 1640357797486551040
• Last Price Increase: 2023-03-27

[MelvinBot]

Triggered auto assignment to @laurenreidexpensify (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/~016e24bec63a2e084f

[MelvinBot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @eVoloshchak (Internal)

[stitesExpensify]

This is the same as number 3 from my slack post here https://expensify.slack.com/archives/C049HHMV9SM/p1678286500121729

I'll leave this open and assigned to me since we didn't have an issue for it, but it is known

[priyeshshah11]

@stitesExpensify Just FYI, this also happens when you add/remove reactions when offline & then go back online as it effectively is the same as reacting quickly as both requests will be sent in quick successions.

[laurenreidexpensify]

@stitesExpensify what is the latest on this one?

[stitesExpensify]

No update yet. This is a bug that is not specific to emojis and happens in other places like pins as well, so it will require a larger discussion. I've been focusing on the smaller emoji reaction polish, and plan on coming back to this next week when I have more time.

[MelvinBot]

@eVoloshchak @stitesExpensify @laurenreidexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[MelvinBot]

@eVoloshchak, @stitesExpensify, @laurenreidexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[laurenreidexpensify]

@stitesExpensify is this merged now?

[laurenreidexpensify]

Note for myself: Haven't hired anyone in Upwork yet, waiting for confirmation that we're fixing this.

[stitesExpensify]

No, this has not been dealt with. I'm going to try to create a plan today. This issue is pretty complicated due to the interaction between optimistic onyx data, the server response, and the pusher data, so I need to figure out the best way to prevent regressions

[laurenreidexpensify]

@stitesExpensify any update on the plan?

[stitesExpensify]

There are 2 options as far as I can tell.

1. Don't send pusher updates to the client that reacted, but do send failure data
   • Pros:
     • Directly solves the problem of your own reactions causing flickering
     • No front end trickery
   • Cons:
     • As we saw with inviting to large group chats, deciding who to send pusher data to can be slow
     • Goes against the pattern of always pushing data back to the front end for writes
2. Always prefer the local changes for the user instead of accepting the server push
   • Pros:
     • This will prevent flickering for both yourself, and multiple users reacting at once
   • Cons:
     • Involves manipulating server data on the front end which is against our development philosophy
     • Uses the front end as source of truth instead of the server
     • Could cause problems with adding/removing emojis on other clients (how do we know when it's legit or not)
3. Some combination of both of the above (which might be necessary)

I plan on starting with 1 and testing it out because it is the most straightforward, and then I'll come back for 2/3 if necessary

[MelvinBot]

@eVoloshchak @stitesExpensify @laurenreidexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[laurenreidexpensify]

@stitesExpensify listed a plan above that we'll work on this week

[stitesExpensify]

So I almost immediately realized a problem with 1 which is that we still need to use pusher for the user that reacted, because we need to push out the reaction to the user's other devices.

I'm thinking the better way to do it will be to control the merge somehow. Looking into it today, this is my top priority now btw

[MelvinBot]

@eVoloshchak, @stitesExpensify, @laurenreidexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MelvinBot]

@eVoloshchak @stitesExpensify @laurenreidexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

[laurenreidexpensify]

@stitesExpensify any update? thanks!

[stitesExpensify]

I'm at a bit of a loss on this one tbh, my attempts to fix it haven't been successful. I've started asking around to see how (or if) we fixed this same issue with pinning reports to see if the same solution could be applied here.

[stitesExpensify]

The pinning issue is not fixed FWIW.

Another potential solution would be to basically batch all of the requests into a single API call with multiple emojis so that it all gets sent at once.

Some potential issues with that are:

1. If you are the first to react and do a bunch of reactions when someone else does one, you will get a push from someone else and you will now be the second person to react (your order will switch)
2. We will still have the same problem where if you send the batch and then react one more time right after, the flicker will still be there

[laurenreidexpensify]

Discussing with @tgolen here

[tgolen]

In our discussion, I realized that the Onxy.merge() isn't working as it should because the reactions are stored as an array, and therefore, that's what's causing the results from one API request to overwrite the other API request. In a world where the reactions are stored as an object, this wouldn't happen. I'm going to be taking over this issue and working on it as top priority to figure out what we need to do here.

[tgolen]

Yesterday a built a POC in NewDot for moving reactions to their own Onyx collection and stored the data as objects instead of arrays. This works well on the front-end. Today, I'm going to attempt to build that into the PHP API, and then eventually the Auth commands.

[tgolen]

OK, I think I've got an idea of how this is going to work. To start with, here is the data format that I'm looking at using. Are we OK with this @stitesExpensify @marcaaron @hannojg ?

message = {
    "html": "test message with emoji \ud83d\ude1c",
    "emojiReactions": {
        "<emojiCode>": {
            "createdAt": 123456789,
            "users": {
               "<accountID>": {
                    "createdAt": 123456789,
                    "skinTone": 1,
                },
            },
        },
    },
}

Old data format

message = {
	"html": "test message with emoji \ud83d\ude1c",
	"reactions": [{
		"emoji": ":+1:",
		"users": [{
			"accountID": 2352342,
			"skinTone": 1
		}, {
			"accountID": 4532562,
			"skinTone": 2
		}]
	}]
}

A few notes for completeness:

• The new property is called emojiReactions. I am not in love with this, but I couldn't think of any other way to migrate to a new data format without using a new property name. I'm open to ideas and suggestions here!
• The emojiCode and accountID will now be the object keys
• The createdAt timestamp will come from the front-end and it will be used in the UI to ensure that the emojis and users are sorted in the order of creation
• I don't really think we need to consider a migration from the old format. This means we will lose all previous reactions, but I think that it is fine at this stage and I don't think trying to keep them is very high priority. At some point, we could maybe delete them to clean up the database, but it's a minimal amount of data IMO

[marcaaron]

Did we consider the tradeoffs of letting the format on the server continue to be an array and transform it into an object when handing off to the front end?

As for an alternative name I can offer... userReactions, participantReactions or emojiReactions

[stitesExpensify]

I think that the new data format makes sense!

[tgolen]

We could considering massaging the data in PHP, but IMO it's not worth it since this feature is so new. It seems like if we did that, then our 5 year-from-now selves would regret that and we should always try to keep things stored on the front-end roughly the same way they are on the backend.