Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mWeb Android/Chrome - Compose box is hidden when we delete edited message #24016

Closed
1 of 6 tasks
mvtglobally opened this issue Aug 1, 2023 · 27 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering

Comments

@mvtglobally
Copy link

mvtglobally commented Aug 1, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Open the app in android chrome
  2. Open any report
  3. Send any text
  4. Write some text in compose box and long press message sent in step 3 and edit the message
  5. Remove all the text and click send, in delete popup, click on delete
  6. Observe that composer is now not visible

Expected Result:

App should display the composer after we delete edited message

Actual Result:

App does not display the composer after we delete edited message in android chrome

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.48-3
Reproducible in staging?: Y
Reproducible in production?: Needs repro
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

compose.box.invisible.on.delete.edit.msg.mp4
az_recorder_20230801_125819.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690903191529089

View all open jobs on GitHub

@mvtglobally mvtglobally added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

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

@OSBotify
Copy link
Contributor

OSBotify commented Aug 1, 2023

👋 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
Copy link

melvin-bot bot commented Aug 1, 2023

Triggered auto assignment to @marcochavezf (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@mvtglobally
Copy link
Author

@bfitzexpensify We are near closure of the checklist. Contributor is not able to repro on PROD, i need a bit more time to validate PROD. If you can check on your side as well to confirm if its a blocker or no, would be great

@marcochavezf
Copy link
Contributor

I'm also not able to reproduce it on prod, investigating what caused the issue

@kushu7
Copy link
Contributor

kushu7 commented Aug 1, 2023

@marcochavezf Issue is from this PR
tested with reverting this PR.

@s-alves10
Copy link
Contributor

Proposal

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

Compose box is hidden when we delete message by saving empty draft

What is the root cause of that problem?

When we save the empty draft, delete confirmation modal appears and the input gets blurred. On select delete, deleteDraft is called, but isFocusedRef.current is false and the main composer doesn't show

if (isFocusedRef.current) {
ComposerActions.setShouldShowComposeInput(true);
ReportActionComposeFocusManager.focus();
}

This is the root cause

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

We need to check if deletion is from confirm modal
Change the above code to

        if (isFocusedRef.current || ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
            ComposerActions.setShouldShowComposeInput(true);
            ReportActionComposeFocusManager.focus();
        }

Removes the isFocused state. We can use isFocusedRef instead.

This works fine

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Hourly KSv2 labels Aug 1, 2023
@s77rt
Copy link
Contributor

s77rt commented Aug 1, 2023

We will handle this as a part of #23618 since it's a regression

@s-alves10
Copy link
Contributor

@s77rt

Do you agree with the solution I proposed?

@marcochavezf
Copy link
Contributor

Hi @s-alves10, I tested the proposed change but the compose input is still not shown after following the reproduction steps:

Screen.Recording.2023-08-01.at.14.28.05.mov

@s77rt
Copy link
Contributor

s77rt commented Aug 1, 2023

@s-alves10 Yes.

We should show the Main Composer only if we are deleting the active report action.

A report action is considered active if it's focused, have the context menu open or have the emoji picker open - we need to be sure we are not missing any other cases.

@s-alves10
Copy link
Contributor

Yes. We've missed the deletion by saving empty draft

@marcochavezf
Copy link
Contributor

We will handle this as a part of #23618 since it's a regression

Ok, I'm going to revert the PR that introduced the regression to unblock the deployment

@s-alves10
Copy link
Contributor

Follow up PR will be created in an hour

@marcochavezf

@marcochavezf
Copy link
Contributor

Ok sounds good, let me know when it's ready, otherwise we'd need to revert

@s-alves10
Copy link
Contributor

@s77rt
Can you test with the branch https://github.com/s-alves10/App/tree/fix/issue-24016?

@s77rt
Copy link
Contributor

s77rt commented Aug 1, 2023

@s-alves10 Please raise a PR, I will test asap. You need to test that as well.

@s-alves10
Copy link
Contributor

@s77rt

PR is in draft.
Please test with it. I'm testing but it shows an error in android native

@s77rt
Copy link
Contributor

s77rt commented Aug 1, 2023

@s-alves10 Let's continue proposal discussion here.

@marcochavezf marcochavezf removed the DeployBlockerCash This issue or pull request should block deployment label Aug 2, 2023
@marcochavezf
Copy link
Contributor

The PR that caused the regression was reverted, so I'm going to close this issue since it's no longer happening on staging

@dhanashree-sawant
Copy link

Hi @marcochavezf, @s77rt, it seems like issue was useful to trigger the solution, will it be eligible for reporting bonus?

@marcochavezf
Copy link
Contributor

Oh sorry @dhanashree-sawant, I think yes since it was a deploy blocker and fixed with the revert. Reopening for payment cc @bfitzexpensify

@marcochavezf marcochavezf reopened this Aug 2, 2023
@bfitzexpensify
Copy link
Contributor

Cool, this has been paid out.

@s77rt
Copy link
Contributor

s77rt commented Aug 24, 2023

I think this is reproducible again

@s77rt
Copy link
Contributor

s77rt commented Aug 30, 2023

This actually got reported again https://expensify.slack.com/archives/C049HHMV9SM/p1692747837798019

@s-alves10
Copy link
Contributor

I think this should be fixed in #23908

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering
Projects
None yet
Development

No branches or pull requests

8 participants