[HOLD for payment 2023-09-27] [$2000] Chat - Main composer is displayed when editing two messages

[techievivek]

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:

Case 1:

1. Send two messages (message A and message B)
2. Switch to edit mode in message A
3. Switch to edit mode in message B
4. Hide keyboard (press back button)
5. Main Composer is shown but it shouldn't because focus is still on message B

Case 2:

1. Send two messages (message A and message B)
2. Switch to edit mode in message A
3. Switch to edit mode in message B
4. Click the cancel button of message A
5. Main Composer is shown but it shouldn't because focus is still on message B

Case 3:

1. Send two messages (message A and message B)
2. Switch to edit mode in message A
3. Open EmojiPicker
4. Close EmojiPicker
5. Switch to edit mode in message B
6. Click the cancel button of message A
7. Main Composer is shown but it shouldn't because focus is still on message B

Notes:

• For case 1 we may have a fix for that already (the fix didn't introduce a regression) (ref1, ref2)
• For cases 2 and 3 we need to make sure we are not re-introducing the regression caught in #24016

Additional testing (regression) steps:

1. Send a message (message A)
2. Switch to edit mode in message A
3. Clear the edit field
4. Press save
5. Confirm deletion prompt
6. Verify Main Composer is shown because there is no other focused message

Expected Result:

Main compose box should not be displayed

Actual Result:

Main compose box is displayed

Workaround:

Don't edit two messages at once.

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

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6018008_IMG_9466.mp4

Kooha-2023-07-28-17-55-44.mp4

Expensify/Expensify Issue URL:

Issue reported by: @s77rt

Slack conversation: https://expensify.slack.com/archives/C02NK2DQWUX/p1690561889854179

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01230125fc5ec18c7d
• Upwork Job ID: 1685886732158468096
• Last Price Increase: 2023-08-30
• Automatic offers:
• s77rt | Reviewer | 26608183
• s-alves10 | Contributor | 26608186
• s77rt | Reporter | 26608188

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01230125fc5ec18c7d

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @flaviadefaria (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[techievivek]

For context, we discovered and discussed about this issue here: https://expensify.slack.com/archives/C02NK2DQWUX/p1690561889854179

[techievivek]

@s77rt Can you confirm if this issue is reproducible on other platforms? or just on Android?

[s-alves10]

Proposal

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

Main composer is displayed when editing two messages

There are several cases we should handle in this issue

Case 1

1. Send two messages (message A and message B)
2. Switch to edit mode in message A
3. Switch to edit mode in message B
4. Hide keyboard (press back button)
5. Main Composer is shown but it shouldn't because focus is still on message B

Case 2

1. Send two messages (message A and message B)
2. Switch to edit mode in message A
3. Switch to edit mode in message B
4. Click the cancel button of message A
5. Main Composer is shown but it shouldn't because focus is still on message B

Case 3

1. Send two messages (message A and message B)
2. Switch to edit mode in message A
3. Open EmojiPicker
4. Close EmojiPicker
5. Switch to edit mode in message B
6. Click the cancel button of message A
7. Main Composer is shown but it shouldn't because focus is still on message B

After all, if there is no focused editing messages, main composer should be shown. If not, main composer should be hidden

What is the root cause of that problem?

We store the shouldShowComposeInput flag in Oynx store through the below function

App/src/libs/actions/Composer.js

Lines 7 to 9 in 93957f0

	function setShouldShowComposeInput(shouldShowComposeInput) {
	Onyx.set(ONYXKEYS.SHOULD_SHOW_COMPOSE_INPUT, shouldShowComposeInput);
	}

The main composer will be shown when this function is called with true.
This function is called with true on unmount, and delete, and with false on focus.

On blur event, we add event listener for keyboardDidHide and show the composer after keyboard is hidden. So when we switch message B to edit mode, message A gets blurred and keyboardDidHide event listener added. So if we force hide the keyboard, main composer is shown. This is the root cause for case 1

When there are multiple messages in edit mode, if blur event on any element(with keyborad hidden) or save/delete action happens, the shouldShowComposeInput function will be called with true and the main composer will be displayed. This is the reason for case 2

EmojiPicker's active reportAction is changed only when emoji picker is opened. So even when the emoji picker is closed and other input gets focused, EmojiPicker's active reportAction isn't changed. This is the root cause for case 3

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

Summary

case 1

When we need to show or hide the main composer, remove the event listener added before. Doing this prevents the unexpected hide of main composer

case 2

Show composer only when the active message is canceled/saved or unmounted.

1. We should show composer only when the message is focused. We have a state isFocused and check this value before showing composer
2. When deleting a message in edit mode, we show delete confirmation modal. We can check this by ReportActionContextMenu.isActiveReportAction but this won't work because we clear the active action early in modalDidHide. To solve this, don't clear the active action in modalDidHide and clear it in confirm/cancel callback. This way, we can control when to clear the active action of delete modal. We clear it when show confirm delete modal from context menu, and don't clear it when publishDraft is called

case 3

When a message in edit mode gets focused, clear the active action of the EmojiPicker. Doing this prevents the incorrect active reportAction of EmojiPicker and won't raise the race condition issue mentioned here

Detailed implementation

1. In order to prevent adding multiple keyboardDidHide event listeners, add parameter called shouldShow to the openReportActionComposeViewWhenClosingMessageEdit function.

index.js

import * as Composer from '../actions/Composer';

export default (shouldShow) => {
    Composer.setShouldShowComposeInput(shouldShow);
};

index.native.js

import {Keyboard} from 'react-native';
import * as Composer from '../actions/Composer';

let keyboardDidHideListener = null;
export default (shouldShow) => {
    if (keyboardDidHideListener) {
        keyboardDidHideListener.remove();
        keyboardDidHideListener = null;
    }

    if (!shouldShow) {
        Composer.setShouldShowComposeInput(false);
        return;
    }

    if (!Keyboard.isVisible()) {
        Composer.setShouldShowComposeInput(true);
        return;
    }

    keyboardDidHideListener = Keyboard.addListener('keyboardDidHide', () => {
        Composer.setShouldShowComposeInput(true);
        keyboardDidHideListener.remove();
    });
};

And replace all ComposerActions.setShouldShowComposeInput with openReportActionComposeViewWhenClosingMessageEdit in ReportActionItemMessageEdit

2. In web platforms, onBlur event is called before the onPress handler is called. To solve this, we can prevent blurring on discard/save/emoji button clicks. To do this, add the following props to save, cancel, and emoji buttons

    onMouseDown={(e) => e.preventDefault()}

We can now safely remove the code below

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

Lines 332 to 335 in e555022

	// Return to prevent re-render when save/cancel button is pressed which cancels the onPress event by re-rendering
	if (_.contains([saveButtonID, cancelButtonID, emojiButtonID], relatedTargetId)) {
	return;
	}

3. On unmount and deleteDraft, call openReportActionComposeViewWhenClosingMessageEdit function with true only when we should show the main composer.

• Add clearActiveReportAction to the EmojiPickerAction and ReportActionContextMenu

• Don't clear the active action in modalDidHide. Remove the following code
  

  App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

  Line 331 in 0196aa2

  this.setState({reportID: '0', reportAction: {}});

and update the context menu's showDeleteModal call here

            if (closePopover) {
                // Hide popover, then call showDeleteConfirmModal
                hideContextMenu(false, () => showDeleteModal(reportID, reportAction, true, clearActiveReportAction, clearActiveReportAction));
                return;
            }

            // No popover to hide, call showDeleteConfirmModal immediately
            showDeleteModal(reportID, reportAction, true, clearActiveReportAction, clearActiveReportAction);

• In the onFocus handler of Composer here, add the following code

        if (!EmojiPickerAction.isActiveReportAction(props.action.reportActionID)) {
            EmojiPickerAction.clearActiveReportAction();
        }
        if (!ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
            ReportActionContextMenu.clearActiveReportAction();
        }

We clear the active report action when another composer gets focused

I think there won't be race condition you mentioned here

• Add an parameter isFocused to the deleteDraft function.

• In deleteDraft and unmount event, clear the active report action if the current message is an active action and show the main composer if the current message is active

In deleteDraft

        if (isFocusedRef.current || EmojiPickerAction.isActiveReportAction(props.action.reportActionID) || ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
            openReportActionComposeViewWhenClosingMessageEdit(true);
            if (EmojiPickerAction.isActiveReportAction(props.action.reportActionID)) {
                EmojiPickerAction.clearActiveReportAction();
            }
            if (ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
                ReportActionContextMenu.clearActiveReportAction();
            }
        }

In unmount

            if (!isFocusedRef.current && !EmojiPickerAction.isActiveReportAction(props.action.reportActionID) && !ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
                return;
            }

            if (EmojiPickerAction.isActiveReportAction(props.action.reportActionID)) {
                EmojiPickerAction.clearActiveReportAction();
            }
            if (ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
                ReportActionContextMenu.clearActiveReportAction();
            }

• To solve the issue mentioned [HOLD for payment 2023-09-27] [$2000] Chat - Main composer is displayed when editing two messages #23908 (comment), we need to update isContextMenuActive state in ReportActionItem when ReportActionContextMenu.isActiveReportAction() changes. So replace the above code with

    const isActiveReportActionForMenu = ReportActionContextMenu.isActiveReportAction(props.action.reportActionID);

    useEffect(() => {
        setIsContextMenuActive(isActiveReportActionForMenu);
    }, [isActiveReportActionForMenu]);

• There is another issue mentioned here.
  The root cause of this issue: when deleting message, after the message gets focused and the unmount handler is called in a quick succession after deleteDraft is called. When get focused, setIsFocused(true) is called but isFocusedRef.current value doesn't change until this effect callback is called. The unmount handler is called just before this ref value is updated and so the handler returns here.

In order to solve this issue, we need to sync the isFocused and isFocusedRef.current. So remove the above useEffect and set isFocusedRef.current whenever setIsFocused is called

This works for all cases as expected

What alternative solutions did you explore? (Optional)

[s77rt]

@techievivek Yes, on all platforms (as long as the window width is small). Just a note the second video in OP does not reflect the current state. #23618 is not deployed to staging yet.

[s77rt]

@s-alves10 Thanks for the proposal. Your RCA is correct. The suggested solution although it looks like it would work it may cause a race condition ^{1 2}. The alternative solution looks more like a workaround (we are not clearing the report action in the right place).

It would better to fix the first solution race condition e.g. have ReportActionItemMessageEdit useEffect run before ReportActionItem or find another solution to #22214.

Footnotes

1. https://github.com/Expensify/App/pull/23618#issuecomment-1654058421 ↩

2. https://github.com/Expensify/App/pull/23618#issuecomment-1654776446 ↩

[s-alves10]

Proposal

Updated

[s77rt]

@s-alves10 Thanks for the update. I think the race condition fix may result in a regression

EmojiPicker is related to draft messages

It's related to the message itself as well e.g. you open the EmojiPicker to react to a message. If the message got deleted the picker should be hidden.

[dukenv0307]

Proposal

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

Chat - Main composer is displayed when editing two messages when the first one is cancelled/save

What is the root cause of that problem?

After canceling or saving the draft message, we will display the main composer again by this logic

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

Line 129 in 024d210

ComposerActions.setShouldShowComposeInput(true);

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

We only should display main composer if the number of the draft message of this report is 0

What alternative solutions did you explore? (Optional)

[s77rt]

@dukenv0307 Thanks for the proposal. I don't think your RCA is correct. This issue only occurs after opening the emoji picker. Can you please update your proposal taking this into account?

[s77rt]

@rojiphil Thanks for the proposal but isn't this proposed already by @s-alves10?

[rojiphil]

@rojiphil Thanks for the proposal but isn't this proposed already by @s-alves10?

But, he is also suggesting other things. If that is required, then, his proposal is a good choice. I do not think that is required. That way, my proposal is different. But, then, you can decide. I just provided my proposal.

[s77rt]

• The PR that introduced the bug has been identified:
  • Case 1: Fix/15086: Screen is flashing when changing focus between edit input #15201
  • Case 2: Edit Comments #2320
  • Case 3: Fix the emoji picker does not close when editing message is deleted #22984
• The offending PR has been commented on:
  • Case 1: Fix/15086: Screen is flashing when changing focus between edit input #15201 (comment)
  • Case 2: Edit Comments #2320 (comment)
  • Case 3: Fix the emoji picker does not close when editing message is deleted #22984 (comment)
• A discussion in #expensify-bugs has been started: I don't think this is needed. Those bugs could have been caught while testing.
• Determine if we should create a regression test for this bug: I think it may be an overkill to add a regression tests for such cases. So nothing to add here.

[thienlnam]

@kavimuru Do you have any additional details on being able to reproduce this one?

[techievivek]

Gentle bump @kavimuru ^

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[s-alves10]

@thienlnam @techievivek

Can I have a status update on this issue?

[techievivek]

We can move forward with the payment here. CC @garrettmknight can we pay the contributors here since the regression period is over, thanks.

[s77rt]

Should the payment here be $1000 because this issue is coming from #17531 where we had a regression there

[garrettmknight]

Good catch @s77rt - to recap, we didn't pay out the previous issue because we rolled the PR back, opened up this issue with a $2k start price and since this was created as part of a regression, we'll pay 50%.

Summmary of payments for this issue:

• urgency bonus: n/a
• regression penalty: yes
• Reporter: N/A - original issue was reported by Applause
• Contributor: @s-alves10 paid $1000 via Upwork
• Reviewer: @s77rt paid $1000 via Upwork

[s-alves10]

@garrettmknight

This solved new issues like https://expensify.slack.com/archives/C049HHMV9SM/p1692747837798019
I wasn't assigned at first and several contributors made their proposals like a normal issue
I'm not sure why this should be considered as a simple regression of #17531

I hope you explain why
Thanks

cc @s77rt @techievivek

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[garrettmknight]

I think you can consider this like an extension of #17531. If the automation had allowed, we would have reopened that issue, had you fix the regression and paid out at 50% for the regression. If that had happened and we'd separately focused this issue on the other cases (and no regressions were reported) we'd have paid out $1000 as standard at the time. With that in mind, I could see paying $1500 total out for this issue.

[garrettmknight]

Added $500 to both contracts.