[HOLD for payment 2024-07-05] [$500] [CRITICAL] [UX Reliability] Long pressing the comment reaction does not open the reactions modal the first time you do it

[mountiny]

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

---

Version Number: 1.4.80-1
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
Expensify/Expensify Issue URL:
Issue reported by: @mountiny
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1717678892330039?thread_ts=1717676956.216859&cid=C049HHMV9SM

Action Performed:

Break down in numbered steps

1. Using iOS native
2. Open a chat with comments that have reactions on it
3. Long press the emoji reaction
4. The haptic feadback signals the click
5. Nothing happens
6. Long press once more
7. The modal with who posted those reactions opened

Expected Result:

Describe what you think should've happened

When you long press on the reaction, the modal with who posted those reactions should open

Actual Result:

Describe what actually happened

The first time it does not work and you only feel the haptic feedback but the click does not happen

Workaround:

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

You have to do it twice

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

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f70177dd3a98fd24
• Upwork Job ID: 1798702401875246255
• Last Price Increase: 2024-06-10
• Automatic offers:
• getusha | Reviewer | 102685488
• bernhardoj | Contributor | 102685489

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[mountiny]

RPReplay_Final1717682080.MP4

its not really that visible in the video, but I do long press on the heart once and nothing happens

[bernhardoj]

Proposal

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

We need to long press the emoji reaction twice to open it.

What is the root cause of that problem?

When we long press, it will call this function.

App/src/pages/home/report/ReactionList/PopoverReactionList/index.tsx

Lines 12 to 16 in bb14c99

	const showReactionList: ReactionListRef['showReactionList'] = (event, reactionListAnchor, emojiName, reportActionID) => {
	setReactionListReportActionID(reportActionID);
	setReactionListEmojiName(emojiName);
	innerReactionListRef.current?.showReactionList(event, reactionListAnchor);
	};

The issue here is that the innerReactionListRef is null the first time we do a long press. That's because we render nothing when reactionListReportActionID or reactionListEmojiName is empty.

App/src/pages/home/report/ReactionList/PopoverReactionList/index.tsx

Lines 26 to 36 in bb14c99

	if (reactionListReportActionID === '' || reactionListEmojiName === '') {
	return null;
	}
	return (
	<BasePopoverReactionList
	ref={innerReactionListRef}
	reportActionID={reactionListReportActionID}
	emojiName={reactionListEmojiName}
	/>
	);

Both of those states will be set when we do the long press. That's why the reaction list shows when we long press it the 2nd time because both of those states is already set on the first long press.

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

Don't render null and let the popover component be rendered. So, let's remove this code.

App/src/pages/home/report/ReactionList/PopoverReactionList/index.tsx

Lines 26 to 28 in bb14c99

	if (reactionListReportActionID === '' || reactionListEmojiName === '') {
	return null;
	}

That code was added in this TS PR migration.

And to be safe, we can make sure the popover isn't visible when the emoji name or report action ID is empty.

App/src/pages/home/report/ReactionList/PopoverReactionList/BasePopoverReactionList.tsx

Lines 28 to 30 in fbd51e4

	return (
	<PopoverWithMeasuredContent
	isVisible={isPopoverVisible}

isVisible={isPopoverVisible && !!reportActionID && !!emojiName}

or we can just return null here too when they are empty

[WojtekBoman]

Hi! I'm going to work on that :)

[mountiny]

@WojtekBoman can you please review the proposal from @bernhardoj and if the RCA/ solution makes sense, they can implement the PR

[ShridharGoel]

Proposal

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

Long pressing the comment reaction does not open the reactions modal the first time you do it.

What is the root cause of that problem?

This happens because we are setting reactionListReportActionID and reactionListEmojiName with state updates with are async, because of which innerReactionListRef.current is accessed while it still has a null value.

App/src/pages/home/report/ReactionList/PopoverReactionList/index.tsx

Lines 12 to 16 in 478db52

	const showReactionList: ReactionListRef['showReactionList'] = (event, reactionListAnchor, emojiName, reportActionID) => {
	setReactionListReportActionID(reportActionID);
	setReactionListEmojiName(emojiName);
	innerReactionListRef.current?.showReactionList(event, reactionListAnchor);
	};

The below condition is true, which leads to nothing rendering:

App/src/pages/home/report/ReactionList/PopoverReactionList/index.tsx

Lines 26 to 28 in 478db52

	if (reactionListReportActionID === '' || reactionListEmojiName === '') {
	return null;
	}

This makes innerReactionListRef.current null and we don't see the reaction list opening.

On the second try, innerReactionListRef.current is no longer null, hence the list opens.

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

We can add some new states and update the showReactionList method as below:

const [isReady, setIsReady] = useState(false);
const [event, setEvent] = useState<ReactionListEvent | undefined>();
const [reactionListAnchor, setReactionListAnchor] = useState<ReactionListAnchor>();

const showReactionList: ReactionListRef['showReactionList'] = (event, reactionListAnchor, emojiName, reportActionID) => {
    setReactionListReportActionID(reportActionID);
    setReactionListEmojiName(emojiName);
    setEvent(event)
    setIsReady(true);
    setReactionListAnchor(reactionListAnchor)
};

Then, we can add a useEffect:

useEffect(() => {
    if (isReady) {
        innerReactionListRef.current?.showReactionList(event, reactionListAnchor);
    }
}, [isReady, event, reactionListAnchor]);

[ShridharGoel]

@mountiny @WojtekBoman I was also working on a proposal, which I have posted above.

[ShridharGoel]

if (reactionListReportActionID === '' || reactionListEmojiName === '') {
    return null;
}

This existing check ensures that the popover doesn't open if reactionListReportActionID or reactionListEmojiName is passed as empty. If we remove this check, then the popover will open with a kind of empty state if one of the above values is not available.

[WojtekBoman]

@bernhardoj Your proposal makes sense to me :)

This if statement was actually added here and wasn't present in the .js file.

if (reactionListReportActionID === '' || reactionListEmojiName === '') {
    return null;
}

This existing check ensures that the popover doesn't open if reactionListReportActionID or reactionListEmojiName is passed as empty. If we remove this check, then the popover will open with a kind of empty state if one of the above values is not available.

@ShridharGoel Is it possible to trigger opening the popover with empty values? If so, I believe that it should be secured at a different level that doesn't affect innerReactionListRef value.

[ShridharGoel]

Is it possible to trigger opening the popover with empty values? If so, I believe that it should be secured at a different level that doesn't affect innerReactionListRef value.

That can happen if the emoji name is not available for an emoji.

[WojtekBoman]

That can happen if the emoji name is not available for an emoji.

I'm not sure if the emoji name might be unavailable but if so we should move this checking logic elsewhere to fix innerReactionListRef.

[mountiny]

@getusha can you please review the proposals also given @WojtekBoman feedback? thanks

[bernhardoj]

I don't know if it's possible for an emoji to not have a name, but I have updated my proposal to not show the list when the name is empty.

[getusha]

I don't think the root cause is correct @bernhardoj, the TS migration PR was merged 5 months ago and i am pretty sure this issue wasn't present while testing.

[melvin-bot]

Upwork job price has been updated to $500

[melvin-bot]

This issue has not been updated in over 15 days. @kevinksullivan, @mountiny, @bernhardoj, @getusha eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[mountiny]

@kevinksullivan this is ready for payment

$500 to @getusha and to @bernhardoj

[bernhardoj]

@mountiny @kevinksullivan I recently set up a payment to get paid in ND, what should be the process? Should I submit the request in ND and comment here when I did?

[mountiny]

Yeah I believe that is the process, in the request you should link to the correct issue

[bernhardoj]

Thanks! Requested in ND.

[JmillsExpensify]

$500 approved for @bernhardoj

[melvin-bot]

@kevinksullivan, @mountiny, @bernhardoj, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@kevinksullivan, @mountiny, @bernhardoj, @getusha Eep! 4 days overdue now. Issues have feelings too...

[mountiny]

@getusha Have you been paid for this task?

[getusha]

Not yet @mountiny, working on the checklist.

[kevinksullivan]

Here's the summary:

• @bernhardoj $500 (already paid in newdot)
• @getusha $500 (newdot)

[melvin-bot]

@kevinksullivan, @mountiny, @bernhardoj, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mountiny]

Both to be paid in NewDot so I think we can close this. no regression steps required as its quite specific issue

[JmillsExpensify]

$500 approved for @getusha