[HOLD for payment 2024-07-24] [$250] Chat - Copying a message with a reply doesn't refocus on the composer

[lanitochka17]

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.86-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: N/A
Email or phone of affected tester (no customers): abebemiherat@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to any chat and write a message
2. Reply to the message and click on the header to navigate back
3. Hover over the message and click the 'copy to clipboard' button

Expected Result:

The app should focus on the composer after copying

Actual Result:

The app doesn't focus on the composer after copying

Workaround:

Unknown

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

Bug6519290_1718880662989.Screen_Recording_2024-06-20_at_3.47.23_AM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0132ecedb1bbac288c
• Upwork Job ID: 1803846769475546722
• Last Price Increase: 2024-06-20
• Automatic offers:
• nkdengineer | Contributor | 102861722

Issue Owner

Current Issue Owner: @stephanieelliott

[melvin-bot]

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

[lanitochka17]

@garrettmknight FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0132ecedb1bbac288c

[melvin-bot]

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

[gijoe0295]

Proposal

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

The app doesn't focus on the composer after copying

What is the root cause of that problem?

This only happens in case the URL points to a report action (i.e. r/:reportID/:reportActionID).

After pressing the context menu item, we refocus the composer. But we early return if the active route is not the current report route (i.e. r/:reportID):

App/src/libs/ReportActionComposeFocusManager.ts

Lines 34 to 37 in e9dc2ef

	/** Do not trigger the refocusing when the active route is not the report route, */
	if (!Navigation.isActiveRoute(ROUTES.REPORT_WITH_ID.getRoute(Navigation.getTopmostReportId() ?? ''))) {
	return;
	}

But we didn't cover the case of report action linking.

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

Include the case of report action linking as well:

if (!Navigation.isActiveRoute(ROUTES.REPORT_WITH_ID.getRoute(Navigation.getTopmostReportId() ?? '-1', Navigation.getTopmostReportActionId()))) {
    return;
}

[nkdengineer]

Proposal

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

The app doesn't focus on the composer after copying

What is the root cause of that problem?

When we copy a message, we have a callback to focus to composer here.

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Line 416 in 633e708

hideContextMenu(true, ReportActionComposeFocusManager.focus);

But in the case the URL of report screen has reportActionID param, this check doesn't pass and then the composer isn't focused

App/src/libs/ReportActionComposeFocusManager.ts

Lines 35 to 36 in 633e708

	if (!Navigation.isActiveRoute(ROUTES.REPORT_WITH_ID.getRoute(Navigation.getTopmostReportId() ?? '-1'))) {
	return;

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

This also doesn't work when we open report as a RHP from search screen. So we should check if the current route is the report screen or report screen RHP, we will re-focus on the composer. We can do like this (Here just an example implementation, we can optimize it or have other way with this idea in the PR phrase).

const navigationState = navigationRef?.current?.getState();
const routes = navigationState?.routes;
const currentRoute = routes?.[navigationState?.index ?? 0];
const params = currentRoute?.params;
const isReportScreen = params && 'screen' in params && typeof params.screen === 'string' ? (params?.screen === SCREENS.RIGHT_MODAL.SEARCH_REPORT || params?.screen === SCREENS.REPORT) : false

if (!isReportScreen) {
    return;
}

App/src/libs/ReportActionComposeFocusManager.ts

Lines 35 to 36 in 633e708

	if (!Navigation.isActiveRoute(ROUTES.REPORT_WITH_ID.getRoute(Navigation.getTopmostReportId() ?? '-1'))) {
	return;

What alternative solutions did you explore? (Optional)

[rushatgabhane]

@gijoe0295 your root cause makes sense. What would Navigation.getTopmostReportActionId() return?
Will this work if there is not reportActionID in the url?

[gijoe0295]

@rushatgabhane it would return undefined and getRoute would return /r/:reportID only.

[rushatgabhane]

@gijoe0295's proposal LGTM
🎀 👀 🎀

[melvin-bot]

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

[nkdengineer]

@rushatgabhane This proposal doesn't work when we do the same step in Search_Report screen. This report has another route pattern.

[nkdengineer]

Screen.Recording.2024-06-21.at.18.00.43.mov

[luacmartins]

@rushatgabhane can you check the latest comment above? Is the selected proposal still good?

[melvin-bot]

@garrettmknight, @luacmartins, @rushatgabhane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[garrettmknight]

@rushatgabhane bump on this one when you get a chance.

[rushatgabhane]

@nkdengineer yeah your proposal is iterating on the same root cause for another page. Which we wouldn't have found unless you had mentioned it

[rushatgabhane]

@gijoe0295's proposal fixes this issue, but @nkdengineer has identified another page that has this issue too.

@luacmartins who should we hire?

[luacmartins]

I think we can hire @gijoe0295 since they pointed out the correct RCA for the issue. We can split the bounty 50/50 between @gijoe0295 and @nkdengineer since they both contributed to the fix.

[melvin-bot]

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

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

[rushatgabhane]

PR on production

[gijoe0295]

@garrettmknight @nkdengineer @rushatgabhane Before summarizing payment, I want to raise a discussion on the split share between me and the other contributor (50:50 currently).

• @nkdengineer pointed out another place where the bug happens and proposed a solution for it
• However, that solution did not fully work as explained in fix: Copying a message with a reply doesn't refocus on the composer #44399 (comment). The final solution was investigated and implemented by me
• Additionally, I also spotted and solved another related issue as explained in fix: Copying a message with a reply doesn't refocus on the composer #44399 (comment)

IMO @nkdengineer contribution here is obviously valuable (I'm grateful for that) but comparing to my efforts to investigate the final working solution, the 50:50 split seems not convincing. Therefore I suggest a 75:25 share for me and @nkdengineer. What do you all think about it? I'm open for discussions!

[garrettmknight]

Hey @gijoe0295 I appreciate your work on this. Since you were the one who actually coded it, I'm open to giving you 75% payment AND giving @nkdengineer 50%.

[garrettmknight]

Payment Summary:

• Contributor: @gijoe0295 $187.50 paid via upwork
• Contributor: @nkdengineer $125 paid via upwork
• Reviewer: @rushatgabhane $250

@rushatgabhane can you complete the checklist and request payment when you have?

[melvin-bot]

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

[garrettmknight]

@stephanieelliott just handing this one off while I'm OOO next week - only awaiting the checklist. If it doesn't get done by the time I'm back I'll pick back up. Thanks!

[rushatgabhane]

1. The PR that introduced the bug has been identified. Link to the PR: Fix: App focuses back on composer even if RHN is open on copy link from app download link in about settings #29254

2. 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: https://github.com/Expensify/App/pull/29254/files#r1692362674

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

4. Determine if we should create a regression test for this bug. Yes

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

      1. Go to any chat and write a message
      2. Reply to the message and click on the header to navigate back
      3. Focus the composer
      4. Hover/Tap and hold the message and click the 'copy to clipboard' button
      5. Verify that the composer is focused after copying
   

[JmillsExpensify]

$250 approved for @rushatgabhane

[stephanieelliott]

Issue for new test case here! https://github.com/Expensify/Expensify/issues/new