[HOLD for payment 2023-08-17] [$1000] Web - Emoji count and details are not dynamically updating when reactions are added

[kbecciv]

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. Send a message from account A to account B
2. On account A, react with 👍
3. Right click on the reacted emoji to open the details modal
4. On account B click on the 👍 to add to the reaction
5. Go to account A and check the details modal

Expected Result:

Details modal should update showing the added reaction count and the account which reacted

Actual Result:

Details modal doesn't update dynamically

Workaround:

Unknown

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: v1.3.46-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: Any additional supporting documentation

2023-07-26.21.28.26.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Nathan-Mulugeta
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690396271150109

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0103b055021bf558bd
• Upwork Job ID: 1684996447830929408
• Last Price Increase: 2023-07-28
• Automatic offers:
• s77rt | Reviewer | 25876690
• tienifr | Contributor | 25876692
• Nathan-Mulugeta | Reporter | 25876693

[melvin-bot]

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

[melvin-bot]

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

[samh-nl]

Proposal

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

Emoji count and details are not dynamically updating when reactions are added

What is the root cause of that problem?

When we open the popover, we explicitly pass the reactions:

App/src/components/Reactions/ReportActionItemEmojiReactions.js

Line 99 in f3196c1

reactionListRef.current.showReactionList(event, popoverReactionListAnchor.current, reaction);

However, if this information changes, the popover will be unaware of it.

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

1. Move PopoverReactionList to ReportActionItem and pass all reactions (props.emojiReactions) as a prop.
   We should define const reactionListRef = useRef(null); here instead of using context and pass the ref to ReportActionItemEmojiReactions.
2. reactionListRef.current.showReactionList should pass reaction.emojiName instead of reaction.

Subsequently, we can filter the passed emojiReactions for the relevant emoji.

What alternative solutions did you explore? (Optional)

N/A

[samh-nl]

Duplicate of #23746

[Nathan-Mulugeta]

I think the issue you have linked is the dupe, this issue was reported on slack yesterday.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[s77rt]

@samh-nl Thanks for the proposal. Your RCA makes sense. The solution also makes sense but can you please explain how we will execute that? I think we used to use onyx but we changed that in this PR #23371 to fix #23307.

[jfquevedol2198]

Proposal

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

Web - Emoji count and details are not dynamically updating when reactions are added

What is the root cause of that problem?

EmojiReaction is not being passed in PopoverReactionList dynamically component when its visible.

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

We need to pass reportActionID as a prop in PopoverReactionList and use withOnyx to get EmojiReactions from the reportActionID. EmojiReactions doesn't come from REPORT_ACTIONS anymore, it comes from REPORT_ACTIONS_REACTIONS

1. Add reportActionID and emojiReactions in PopoverReactionList component and add withOnyx

    withOnyx({
        emojiReactions: {
            key: ({reportActionID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS_REACTIONS}${reportActionID}`,
        },
    })

2. Move PopoverReactionList usage from ReportActionsView to ReportActionsList

<PopoverReactionList ref={context.reactionListRef} reportActionID={reportActionID} />

3. Call setReportActionID to set reportActionID of the updated EmojiReactions in ReportActionItemt

    useEffect(() => {
        props.setReportActionID(props.action.reportActionID);
    }, [props.emojiReactions]);

4. When reportActionID is updated and PopoverReactionList if its visible, update its content.
   We can get the updated EmojiReactions from Onyx and reportActionID.

What alternative solutions did you explore? (Optional)

N/A

[tienifr]

Proposal

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

Details modal doesn't update dynamically

What is the root cause of that problem?

In here, we show the reaction list with the static reaction data, so when that data changes, PopoverReactionList does not re-render again.

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

1. Make reportActionID a prop of PopoverReactionList, and connect to the ${ONYXKEYS.COLLECTION.REPORT_ACTIONS_REACTIONS}${reportActionID} key in PopoverReactionList.
2. From that data we can get the correct reaction based on the this.state.emojiName in the PopoverReactionList
3. In here, we can just pass the emojiName, no need for the full reaction
4. Create a reactionListReportActionID and setReactionListReportActionID in the ReportScreenContext, this is because we should only have 1 PopoverReactionList being rendered at a time (not rendered for every report action item), we need to know for which reportActionID we're opening the reaction list, so that we have the reportActionID to pass to it.
5. Pass the reactionListReportActionID from ReportScreenContext as reportActionID of PopoverReactionList here
6. In here, we need to call setReactionListReportActionID
7. We also have an issue where if User A open the PopoverReactionList from a single reaction of User B, then User B deletes that reaction, the popover will still show for User A. To fix this, in componentDidUpdate of PopoverReactionList we need to check if the popup is visible, but the emoji from state.emojiName has been deleted from emojiReactions, then we call hideReactionList.

What alternative solutions did you explore? (Optional)

As suggested by @s77rt, instead of using the ReportScreenContext to pass the reportActionID to PopoverReactionList, we can create a child component of PopoverReactionList to do the rendering of the list, and PopoverReactionList will provide the reportActionID to that child component for connection to Onyx. Then we can just add the reportActionID when we use showReactionList.

[samh-nl]

@s77rt From this discussion in the PR I can conclude that this problem was anticipated, but it was decided to treat this as a separate issue instead of fixing it there.
#23371 (comment)

looks like there were 2 issues, and we're going with this one #23752

Please see the updated proposal. I have simplified it with an approach that does not require withOnyx and uses the existing data in ReportActionItem.

[s77rt]

@jfquevedol2198 Thanks for the proposal. Your RCA is correct. As for the solution, I don't see the need to move PopoverReactionList and couple it with ReportActionsList state. Also it does not make sense to change report action whenever the reaction changes. If you are viewing reactions of reportAction A then changes to reportAction B should not effect you.

[s77rt]

@tienifr Thanks for the proposal. Your RCA is correct. The solution makes sense but I think it's better to avoid passing by ReportScreen as we will end up rendering that component whenever we open a new reaction list. I see that the main problem is on how we can pass reportActionID. I have an idea not sure it would work or if it's well optimised but worth the investigation, what if we split the PopoverReactionList into two components, one that handles the real logic and subscribes to emojiReactions and another which is just a wrapper that we use to render that component.

We call showReactionList with reportActionID on the wrapper -> set state to that new report action id -> render the underling component passing reportActionID={this.state.reportActionID}.

[s77rt]

@samh-nl Thanks for the update. I don't think we want to go with that solution, there should be only one PopoverReactionList.

[tienifr]

I have an idea not sure it would work or if it's well optimised but worth the investigation, what if we split the PopoverReactionList into two components, one that handles the real logic and subscribes to emojiReactions and another which is just a wrapper that we use to render that component.

@s77rt yeah there's a constraint where we need to pass reportActionID as a prop to PopoverReactionList in order to use withOnyx, your suggestion will work. Later once we have the useOnyx prop, we won't have to split the component like that.

Proposal updated to highlight that approach in alternate solution.

[s77rt]

@tienifr Thanks for the update. Indeed once we have the useOnyx hook things will get much easier but I don't think we should block on that. The alternative solution looks good to me. Let's go with that.

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

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

[flodnv]

Sounds good 👍

[melvin-bot]

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

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

[melvin-bot]

📣 @Nathan-Mulugeta 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[tienifr]

PR ready for review #24024

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @tienifr got assigned: 2023-08-01 09:27:30 Z
• when the PR got merged: 2023-08-08 23:01:02 UTC
• days elapsed: 5

On to the next one 🚀

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

• Fix: Popover reaction list does not update dynamically #24024

If no regressions arise, payment will be issued on 2023-08-17. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @s77rt requires payment offer (Reviewer)
• @tienifr requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] 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:
• [@s77rt] A discussion in #expensify-bugs 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:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@NicMendonca] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: fixed: dev: '0:undefined:' displayed in reaction emoji tooltip #23371
• The offending PR has been commented on: fixed: dev: '0:undefined:' displayed in reaction emoji tooltip #23371 (comment)
• A discussion in #expensify-bugs has been started: Not needed as there is not much to discuss, the regression could have been caught during PR review/test
• Determine if we should create a regression test for this bug: No

[NicMendonca]

• Reporter: @Nathan-Mulugeta - $250
• Contributor: @tienifr - $1,000
• Contributor+: @s77rt - $1,000

[NicMendonca]

Everyone has been paid 🎉