Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

[$250] Android - Composer - Draft text reappears after clearing the compose box and navigating back #41499

Open lanitochka17 opened 2 weeks ago

lanitochka17 commented 2 weeks ago

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.69-2 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 Issue reported by: Applause - Internal Team

Action Performed:

  1. Open Expensify app
  2. Open a chat
  3. Send a message
  4. Type something inside the compose box but don't send it
  5. Hold on the sent message on step 3 and tap on reply in thread
  6. Tap on the three dot menu and tap on leave
  7. Clear the draft text inside the compose box
  8. Tap on the device back button

Expected Result:

The compose box doesn't contain any draft text

Actual Result:

The compose box contains the previously cleared draft text

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/80f26b98-b112-45e5-aa2b-bb8398ad5e0c

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01321f2f43059fc592
  • Upwork Job ID: 1787349096905330688
  • Last Price Increase: 2024-05-06
  • Automatic offers:
    • bernhardoj | Contributor | 0
Issue OwnerCurrent Issue Owner: @NikkiWines
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @kadiealexander (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 commented 2 weeks ago

@kadiealexander 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 commented 2 weeks ago

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

bernhardoj commented 2 weeks ago

Proposal

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

The draft text isn't cleared when we delete the message from another report screen.

What is the root cause of that problem?

When we leave a thread, it will navigate to the most recent report (except the current report). https://github.com/Expensify/App/blob/7483d10e686e240d07d9fa3e43e89ffdfcda9cbd/src/libs/actions/Report.ts#L2508-L2515

So, now there are 2 same reports in the navigation stack. If we clear the composer message from the 2nd report screen and go back, the 1st report screen will still show the composer message. It's because the onyx draft message and the composer message state aren't synchronized.

This doesn't happen on other platforms because we have an effect to sync it. https://github.com/Expensify/App/blob/7483d10e686e240d07d9fa3e43e89ffdfcda9cbd/src/pages/home/report/ReportActionCompose/SilentCommentUpdater/index.tsx#L25-L37

But we don't have it for Android (it was removed in https://github.com/Expensify/App/pull/32799) https://github.com/Expensify/App/blob/7483d10e686e240d07d9fa3e43e89ffdfcda9cbd/src/pages/home/report/ReportActionCompose/SilentCommentUpdater/index.android.tsx#L16-L23

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

We can update the navigate to the most recent report logic to just use goBack with the report screen as the fallback https://github.com/Expensify/App/blob/7483d10e686e240d07d9fa3e43e89ffdfcda9cbd/src/libs/actions/Report.ts#L2508-L2515

Navigation.goBack(lastAccessedReportRoute);

This will make sure we won't have 2 same report screens in the stack, so we don't need to sync the composer message state.

melvin-bot[bot] commented 1 week ago

Job added to Upwork: https://www.upwork.com/jobs/~01321f2f43059fc592

melvin-bot[bot] commented 1 week ago

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

kadiealexander commented 1 week ago

@mollfpr please review the proposal above, thanks!

mollfpr commented 1 week ago

@bernhardoj I don't understand the changes you suggest, the permalink you attach already uses Navigation.goBack(lastAccessedReportRoute);.

bernhardoj commented 1 week ago

Navigation.goBack(lastAccessedReportRoute); will only be called if it's not a thread, but in our case, we leave a thread.

melvin-bot[bot] commented 1 week ago

@mollfpr, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

mollfpr commented 5 days ago

The proposal from @bernhardoj makes sense to me and I have tested the solution it's fixed the issue.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed!

melvin-bot[bot] commented 5 days ago

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

NikkiWines commented 4 days ago

Yep, agreed - thanks for the proposal @bernhardoj πŸ™‡

melvin-bot[bot] commented 4 days ago

πŸ“£ @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 πŸ“–

bernhardoj commented 4 days ago

PR is ready

cc: @mollfpr