[HOLD for payment 2023-09-07] [$1000] Web - Composer out focus when the thread has many messages

[kbecciv]

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

---

Action Performed:

1. Open any report
2. Chat a message
3. Go reply in thread
4. Click edit parent message
5. Click composer and send many messages (about > 22 messages), Observe that composer is out of focus and switches to focus edit parent message

Expected Result:

Composer doesn't lose focus

Actual Result:

Observe that composer is out of focus and switches to focus edit parent message after send many message

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

Screen.Recording.2023-07-29.at.13.30.43.mov

Recording.4004.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019d68a31c68ab4865
• Upwork Job ID: 1685790256290459648
• Last Price Increase: 2023-08-06
• Automatic offers:
• mollfpr | Reviewer | 26033382
• tienifr | Contributor | 26033383
• namhihi237 | Reporter | 26033386

[melvin-bot]

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

[jliexpensify]

I can somewhat repro this, but not reliably.

I did discover an additional issue related to the composer and the Edit function:

1. Start chat, type something and click on "Thread"
2. Click on "Edit" on original word/message
3. Start typing multiple letters in thread
4. After ~ 12 (?) letters in-thread, notice that the compose has been edited with the most recent letter.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019d68a31c68ab4865

[melvin-bot]

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

[melvin-bot]

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

[jliexpensify]

Hi @mollfpr - given this is a composer issue, I also notice this GH was recently opened (#23898). Do you think the root causes are similar?

[tienifr]

Proposal

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

Observe that composer is out of focus and switches to focus edit parent message after send many message

What is the root cause of that problem?

It's here, we're focusing the "edit message" input in first render (and whenever isDraftEmpty change), regardless of the previous value of the draft message.

So when the page reloads, or when the message edit input goes out of view and is rerendered again, it will automatically gain focus (even though the isDraftEmpty status does not change)

This is a regression after the functional component refactor.

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

We can use isFirstRender similar to here and early return in this block if it's the first render:

if (isFirstRender.current) {
    isFirstRender.current = false;
    return;
}

aka we only focus the input if the isDraftEmpty status changes, not whenever that component remounts.

Since this first render logic is used quite commonly now, we can create a dedicated hook for it, for better reuse.

What alternative solutions did you explore? (Optional)

Another solution is to use usePrevious to get the value of the previous draft message and only focus the edit input if previous draft message is empty and current draft message is not empty, similar to the logic before the functional component refactor.

const prevDraftMessage = usePrevious(props.draftMessage);

useEffect(() => {
    if (prevDraftMessage || !props.draftMessage) {
        return;
    }

    focusTextInputAfterAnimation(textInputRef.current, 100);
}, [prevDraftMessage, props.draftMessage]);

[Update: the below was already fixed]
Also there's another bug in usePrevious where the initial value of it is always undefined (even though the observed value is never undefined.
We need to change this line https://github.com/Expensify/App/blob/09cf7dc362d78781d2efd734018780caa634a126/src/hooks/usePrevious.js#L10C26-L10C26 to const ref = useRef(value); to set the initial value of the ref correctly. Or we can add another initialValue as a 2nd param for usePrevious, and use it to set the initial value for the usePrevious in this case.

[mollfpr]

@jliexpensify I think it has different solution from this.

@tienifr Your solution is working, but the root cause still needs to be fixed. Why does the component need to re-render?

My observation led to this code causing it to re-render. If I remove the below code, the composer is not out of focus.

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

Lines 614 to 618 in dc32700

	// TaskReport's created actions render the TaskView, which updates depending on certain fields in the TaskReport
	ReportUtils.isTaskReport(prevProps.report) &&
	prevProps.action.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED &&
	ReportUtils.isTaskReport(nextProps.report) &&
	nextProps.action.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED &&

[Surojd]

Contributor details
Expensify account email: surojsmiling@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/surojd

[melvin-bot]

📣 @Surojd! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[Surojd]

Contributor details
Expensify account email: surojsmiling@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/surojd

[tienifr]

@tienifr Your solution is working, but the root cause still needs to be fixed. Why does the component need to re-render?

@mollfpr Sorry, to clarify, the issue does not happen when the component re-renders, it happens if the component is unmounted and re-mounted again. In this case, when there're many messages that cause the edited report action to go out of the screen, that report action is unmounted, then when we send another message, the list is re-rendered and the items are mounted again, causing the issue.

Also do note that the issue also occurs when reloading the page, so I believe my proposal is the correct fix.

[HLEDYA]

Proposal

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

Composer loses focus when an action is in edit mode and the thread has many messages.

What is the root cause of that problem?

@tienifr and @mollfpr considerations are correct. But IMO, the main problem is an action item gets into editing mode without hiding the composer.

For smallScreenWidths this is done automatically as you can see from the following screenshot.

Screen.Recording.2023-08-02.at.09.31.55.mov

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

I think, composer should be hidden if an action is in edit mode. To do this, the following line

App/src/pages/home/report/ReportFooter.js

Line 84 in 6a38341

{!hideComposer && (props.shouldShowComposeInput || !props.isSmallScreenWidth) && (

can be changed as:

{!hideComposer && props.shouldShowComposeInput && (  

What alternative solutions did you explore? (Optional)

@tienifr solution

[Surojd]

Proposal

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

Composer out focus when the thread has many messages

What is the root cause of that problem?

Here , we are focusing on the "edit message" input, regardless of the draft message's prior value, during the first render (and every time isDraftEmpty changes).

Therefore, even though the isDraftEmpty state remains same, the message edit input will automatically gain focus when the page reloads or when it moves out of view and is rendered again.

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

Lines 144 to 152 in 6a38341

	const isDraftEmpty = !props.draftMessage;
	useEffect(() => {
	if (isDraftEmpty) {
	return;
	}
	focusTextInputAfterAnimation(textInputRef.current, 100);
	}, [isDraftEmpty]);

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

In order to fix the issue we have to check whether the previous value of draftMessage is undefined or props.draftMessage

    const prevProps = usePrevious(props.draftMessage);
    const isDraftEmpty = !props.draftMessage || undefined === prevProps || prevProps === props.draftMessage;

Issue Fixed Video:

out.focus.when.the.thread.has.many.messages.mp4

[jliexpensify]

Not overdue, but bumping @mollfpr to review some proposals!

[mollfpr]

Also do note that the issue also occurs when reloading the page, so I believe my proposal is the correct fix.

@tienifr The focus is passed to the composer at the end, while this issue is the main composer loses the focus. Did I miss something?

Screen.Recording.2023-08-04.at.20.40.50.mov

that report action is unmounted, then when we send another message, the list is re-rendered, and the items are mounted again, causing the issue

If the ReportActionItem is unmounted, then the value of isFirstRender.current = false is no use, no?

I'm following the changes you suggest in your solution, but it's not working.

---

@HLEDYA I think that's intended.

I think, composer should be hidden if an action is in edit mode. To do this, the following line

You can open a discussion regarding this. This is not a fix but a suggestion to change our App behavior.

---

@Surojd it's already proposed in the alternative solution on @tienifr proposal.

[Surojd]

@mollfpr That solution is ineffective. I tested the code, investigated every possibility, and discovered the answer.

[mollfpr]

@Surojd Sorry, which solution do you mean?

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[Surojd]

@Surojd Sorry, which solution do you mean?

This solution does not work.

useEffect(() => {
        if (isFirstRender.current) {
            isFirstRender.current = false;
            return;
        }
    }, []);

[tienifr]

@mollfpr I've added the code snippet for that alternate solution of using usePrevious in case you want to try it out.

[Surojd]

@mollfpr ,

@Surojd What is the different between your solution with @tienifr's alternative solution?

@tienifr's solution

useEffect(() => {
    if (isFirstRender.current) {
        isFirstRender.current = false;
        return;
    }
}, []);

This solution is not working because it is re-rendering the react component when composer out of focus when the thread has many messages. this code runs on re-rendering.

In order to fix this issue. In my proposed solution. I have checked prevProps has draftMessage or props.draftMessage is same with prevDraftMessage.

Here is my implementation for fix

   const prevDraftMessage= usePrevious(props.draftMessage);
    const isDraftEmpty = !props.draftMessage || undefined === prevDraftMessage|| prevDraftMessage=== props.draftMessage;
 
    useEffect(() => {
        if (isDraftEmpty) {
            return;
        }
        focusTextInputAfterAnimation(textInputRef.current, 100);
    }, [isDraftEmpty]);

[mollfpr]

@Surojd In the @tienifr proposal, there are 2 solutions proposed. Using the usePrevious is their alternative solution, and they submitted the proposal on 31 Jul, while yours is on Aug 2. So your proposal is a duplicate.

Let's use the usePrevious for more convenient logic as before. The proposal from @tienifr looks good to me 👍

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

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

[melvin-bot]

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

📣 @namhihi237 🎉 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 #24369

[thienlnam]

For visibility, we didn't resolve this bug on mweb/safari because that would require us to add platform specific handling to the code. I've created this issue #26239 to track solving the root cause of the mweb/safari issue. When we solve that, this issue should be resolve for mweb/safari as well

[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-09 17:02:01 Z
• when the PR got merged: 2023-08-29 18:05:30 UTC
• days elapsed: 14

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

• fix: 23886 Composer out focus when the thread has many messages #24369

If no regressions arise, payment will be issued on 2023-09-07. 🎊

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:

• @mollfpr requires payment offer (Reviewer)
• @tienifr requires payment offer (Contributor)
• @namhihi237 requires payment offer (Reporter)

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:

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

[jliexpensify]

Payment summary:

• Bug Reporter: @namhihi237 $250
• Contributor: @tienifr $1000
• C+: @mollfpr $1000

Upwork job

@mollfpr could you complete the checklist? Thanks!

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:

#18809

[@mollfpr] 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/18809/files#r1316713622

[@mollfpr] 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:

I think this can be caught while QA, so adding the regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] 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. Open any report
2. Chat a message
3. Go reply in a thread
4. Click edit parent message
5. Click composer and send many messages (about > 22 messages)
6. Verify that the composer doesn't lose focus
7. 👍 or 👎

[jliexpensify]

Everyone paid, job removed!