[HOLD for payment 2024-04-09] [$500] Composer defocuses when editing parent message in thread

[kbecciv]

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.56-0
Reproducible in staging?: y
Reproducible in production?: n
Issue found when executing PR: #37566
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com.
2. Go to any chat.
3. Send a message containing inline image markdown. Example:
   An inline image and some text
4. Right click on the message > Edit comment.
5. Focus the cursor anywhere in the editor.
6. Note that the composer will not defocus.
7. Right click on the message > Reply in thread.
8. Right click on the parent message > Edit comment.
9. Focus the cursor anywhere in the editor.

Expected Result:

The composer will not defocus.

Actual Result:

The composer defocuses when editing message with inline image mark down in thread.

Workaround:

n/a

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

Bug6424058_1711173003900.20240323_134637.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012dd2062cbe841507
• Upwork Job ID: 1772340169085984768
• Last Price Increase: 2024-03-25
• Automatic offers:
• jjcoffee | Reviewer | 0
• bernhardoj | Contributor | 0

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

[melvin-bot]

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

[kbecciv]

We think that this bug might be related to #vip-bills

[amyevans]

This is related to a new feature and also pretty minor so not worth considering a blocker.

@kidroca do you have bandwidth to take this on and fix as polish? If not I can open it up to external proposals. cc @situchan @marcaaron

[amyevans]

Hmm actually I'm going to walk back my comment that it's related to the Image Markdown PR, I think it was just incidental that it was discovered during QA there. I can reproduce the bug on main with that PR reverted, and I can also reproduce on a comment that does not include image markdown.

Will continue to investigate...

[situchan]

Agree, I am not convinced why composer focus behavior is related to image MD.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012dd2062cbe841507

[melvin-bot]

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

[melvin-bot]

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

[shubham1206agra]

@amyevans I think this should block deployment here as it breaks crucial feature of editing messages.

[bernhardoj]

Proposal

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

The edit composer of a parent thread message lose focuses when clicking on it.

What is the root cause of that problem?

For the parent thread message, we set an on-press event that will navigate the user to the thread report.

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

Lines 85 to 86 in d8361ee

	<ReportActionItem
	onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(ancestor.report.reportID))}

In the video, the thread is 1 level deep, so pressing it will navigate to itself, so the composer loses the focus.

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

Don't trigger the on-press when the message is in draft mode.

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

Lines 798 to 800 in d8361ee

	<PressableWithSecondaryInteraction
	ref={popoverAnchorRef}
	onPress={onPress}

onPress={!draftMessage ? onPress : undefined}

also, maybe we can improve the thread parent press logic to ignore the press if the report destination is the same as the current report

[shubham1206agra]

@bernhardoj Please link offending PR here.

[bernhardoj]

It's related to the comment linking PR. The navigation triggers this memo to recalculate the ID which is passed to the chat list FlatList key, so the list re-renders every time a navigation happens.

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

Lines 135 to 141 in d8361ee

	const listID = useMemo(() => {
	isFirstLinkedActionRender.current = true;
	const newID = generateNewRandomInt(listOldID, 1, Number.MAX_SAFE_INTEGER);
	listOldID = newID;
	return newID;
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [route, isLoadingInitialReportActions]);

We still need to apply my fix, otherwise we can't edit a thread ancestor message.

Screen.Recording.2024-03-26.at.14.24.39.mov

[shahinyan11]

Proposal

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

Image MD - Composer defocuses when editing message with inline image mark down in thread

What is the root cause of that problem?

When we press anywhere in input, the event will bubble up the tree and call the onPress of the parent element here

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

Add onClick={(e)=>{e.stopPropagation()}} to this View

What alternative solutions did you explore? (Optional)

Replace this View with PressableWithoutFeedback component. This approach is also used in other places in the application to prevent events from bubbling up the tree (e.g here )

[jjcoffee]

Looks to be caused by the comment linking PR #30269. @bernhardoj's proposal LGTM to get this fixed quickly!

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[amyevans]

Proposal looks good @bernhardoj, thanks! Let us know when we can expect a PR

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

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

[amyevans]

Also cc @perunt @roryabraham (nothing y'all need to do here, just a tag for visibility since this came from comment linking)

[bernhardoj]

PR is ready

cc: @jjcoffee

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

• Fix composer focus is lost when clicking the edit composer of a thread parent message #39002

If no regressions arise, payment will be issued on 2024-04-09. 🎊

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

• @jjcoffee requires payment automatic offer (Reviewer)
• @bernhardoj requires payment automatic offer (Contributor)

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

• [@jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
• [@jjcoffee] 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:
• [@jjcoffee] 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:
• [@jjcoffee] Determine if we should create a regression test for this bug.
• [@jjcoffee] 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.
• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[isabelastisser]

@jjcoffee please complete the list above. Thanks!

[isabelastisser]

The payments were processed in Upwork.

Waiting for @jjcoffee to complete the BZ list before this can be closed.

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: HIGH: (Comment linking: Step 4) Main "glue" PR for Comment Linking #30269
• 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: HIGH: (Comment linking: Step 4) Main "glue" PR for Comment Linking #30269 (comment)
• 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: N/A - big PR so regressions were inevitable!
• Determine if we should create a regression test for this bug. Yes
• 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.

Regression Test Proposal

1. Send a message to any chat
2. Right-click and reply in thread to the message
3. Right-click and edit the thread parent message
4. Tap anywhere in the message
5. Verify that the focus on the composer isn't lost

Do we agree 👍 or 👎

[isabelastisser]

Hi @amyevans, do you agree that we should create the regression test with the steps above? Thanks!

[amyevans]

@isabelastisser I think since the bug was caught during normal regression testing by Applause, it means that adding an explicit test case for this would be unnecessary. However if you want to be safe you can always open a GH for it and let Isabella make that call!

[isabelastisser]

Thanks for the context, Amy! I agree and will close it then.