search

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
3.3k stars 2.74k forks source link

[$250] Mention - Mention list takes 2-3 seconds to remove after sending a message #45485

Open lanitochka17 opened 1 month ago

lanitochka17 commented 1 month 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: 9.0.7-4 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/runs/view/22744&group_by=cases:section_id&group_order=asc&group_id=306201 Email or phone of affected tester (no customers): shussain+andmob2@applausemail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Open a chat
  2. Mention a user by entering their username/email
  3. Send the message without selecting the user from the list

Expected Result:

After sending the message without selecting the user from the list, the mention list should be immediately removed

Actual Result:

Mention list takes 2-3 seconds to remove after sending a message

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/541b09e1-df5b-4e40-9002-5e08c6f605c3

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012c64667ec4d95daf
  • Upwork Job ID: 1813327338752177204
  • Last Price Increase: 2024-07-30
Issue OwnerCurrent Issue Owner: @ishpaul777
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @sakluger (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 1 month ago

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

lanitochka17 commented 1 month ago

@sakluger 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

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~012c64667ec4d95daf

melvin-bot[bot] commented 1 month ago

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

ishpaul777 commented 1 month ago

Awaiting proposals

sakluger commented 1 month ago

Still no proposals.

ishpaul777 commented 1 month ago

we are looking for proposals

melvin-bot[bot] commented 1 month ago

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

fabOnReact commented 1 month ago

Proposal

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

The mention list is not removed immediately after sending a message. The mention list is removed after 2-3 seconds on Android.

What is the root cause of that problem?

https://github.com/Expensify/App/blob/1084706b35c2157656ca747374b766083c910061/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L369

In the video below you can see how the mention list disappears with 2-3 seconds of delay, while the RNMarkdownTextInput text disappears immediately.

https://gist.github.com/user-attachments/assets/361eb56e-91ed-40b0-80e1-4ca0eece806d

After commenting setNativeProps, the TextInput text/value and the suggestions disappear together after a delay of 2-3 seconds.

https://gist.github.com/user-attachments/assets/713e99b5-fa35-47b5-9671-4363d0f94712

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

The solution consists of the following steps:

1) Passing a forwardRef from ReportActionCompose.tsx to ComposeWithSuggestions. 2) Set the ref on the HostComponent responsible for displaying the Suggestions. From a quick investigation, I was able to hide the Suggestions by changing the style of the View HostComponent in AutoCompleteSuggestionsPortal. 2) hiding the Suggestions using ref.current.setNativePros({{ transform: [{ scale: 0 }] }) from the ReportActionCompose.tsx function handleSendMessage

You can find here my detailed explanation of why we use setNativeProps to clear the value of the TextInput.

What alternative solutions did you explore? (Optional)

An alternative approach is useAnimatedStyle or useAnimatedProp to hide the Suggestions.

1) Use a reanimated shared value to save the state change for hiding the suggestions. The message is sent with SendButton and the ReportActionCompose handleSendMessage function. 2) create an animatedStyle based on the shared value. 3) use animatedStyle in Suggestions.tsx to hide the suggestions.

I don't believe it will work, because the logic runs asynchronously on the JS thread, which causes this delay of 2-3 seconds. We need to update immediately the native UI and skip any update in JavaScript, which can be done using setNativeProps.

https://docs.swmansion.com/react-native-reanimated/docs/core/useSharedValue/#remarks

When you change the sv.value the update will happen synchronously on the UI thread. On the other hand, on the JavaScript thread the update is asynchronous. This means when you try to immediately log the value after the change it will log the previously stored value.

fabOnReact commented 1 month ago

Proposal

Updated

ishpaul777 commented 1 month ago

Thanks for your proposal @fabOnReact, it looks promising 👍, would you be able to provide a testing branch so i can test this quickly

melvin-bot[bot] commented 1 month ago

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

ishpaul777 commented 1 month ago

not overdue, just waiting for test branch to test proposal solution

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@sakluger @ishpaul777 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

fabOnReact commented 1 month ago

not-overdue @melvin-bot I already have a working solution https://github.com/fabriziobertoglio1987/App/commit/9c473efc9f531b572548bf217a71b16e3a72e2ae. I'm refactoring the solution to use React.forwardRef, useImperativeHandle and to fix the issue also for emojii.

I'll publish the final solution in 1-2 days. Thanks

UPDATE: I'm working on this branch and did not detect issues in the implementation, but It is still a work in progress.

fabOnReact commented 1 month ago

@ishpaul777 Thanks a lot

The branch is available here. There have been some minor changes with handleSendMessage in the last couple of days.

I will rebase the branch later and update the solution as required. I recorded the following tests on Android. I tested web, iOS and did not detect any issues:

Before After
Before After
sakluger commented 1 month ago

Not overdue - next step is for @ishpaul777 to test on the test branch.

ishpaul777 commented 1 month ago

I am not able to send message in any report at the moment on dev. it appears we have a recent bug only on dev enviornment

reported here- https://expensify.slack.com/archives/C049HHMV9SM/p1722528633557129

i'll investigate what's wrong provide a update ASAP. 🙇

fabOnReact commented 1 month ago

The branch is available here. There have been some minor changes with handleSendMessage in the last couple of days.

* they started introducing changes to this functionality with commit [0c7af9a](https://github.com/Expensify/App/commit/0c7af9a7bb590560dc41963d54713c2f79bb8d86)

* later the changes were reverted (commit [0370382](https://github.com/Expensify/App/commit/0370382b611b3f292e7fa25d7d831550d038a044))

* the changes landed again with [60af307](https://github.com/Expensify/App/commit/60af307f768228fd9b95fc406cd5e43b804c0ffd)

Thanks @ishpaul777

The changes have been reverted again last night. So there are no problematic changes on the main branch. I rebased the branch based on the latest main branch, pushed and tested it again.

Before After
ishpaul777 commented 1 month ago

Thank you! I will provide a update in ~6-8 hours 🙇

ishpaul777 commented 1 month ago

Thank you for your patience @fabOnReact! Proposed solution works great on Android! 🎉

However I have spotted a bug (reproducable on main also) where i am not able send message when suggestion popover is open, which prevents testing for the branch on IOS. Can you try if this is reproducable for you?

Action Performed:

  1. Open any report and type @
  2. send the message

Expected Result: Message sent with @ Actual Result: Message not sent with @ and suggestion popover closes

I have reported it here https://expensify.slack.com/archives/C049HHMV9SM/p1722621638556609

https://github.com/user-attachments/assets/6410b2b5-0d8c-4330-a6f4-a06a4da016eb

ishpaul777 commented 1 month ago

It seems the above mentioned issue is not a bug but a expected behaviour. from https://expensify.slack.com/archives/C01GTK53T8Q/p1721047319091279

Now i really unsure how to proceed on this issue, it appears that android is the only platfor that sends message when popover is open, ios and web just close the popover don't send message which is a major inconsistency for the platform

i'll bring a internal engineer for decision on next steps, if we want to fix OP @fabOnReact proposal LGTM!

🎀 👀 🎀

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@tylerkaraszewski, @sakluger, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

ishpaul777 commented 1 month ago

@sakluger @tylerkaraszewski Any thoughts if we want to fix this issue, it seems like android is only platform on which we can send a message while suggestion menu is open, Do we want to align behaviour with other platform or fix the issue in OP ?

sakluger commented 1 month ago

Hey @ishpaul777 - based on the Slack thread and the PR it's based on, we changed the behavior so that clicking outside the mention list (including clicking on the Send button) will only close the mention list. I tested this in NewDot Web - opening the mention list, and then clicking on the Send button closes the mention list but does not send the message.

It seems like maybe Android wasn't tested for that PR - In the Screenshots/Videos section of the top comment, Android has no screenshots (mWeb Safari is also missing). I think we should definitely align the behavior to that PR. In this scenario, would we ask the original PR author to handle fixing for Android?

ishpaul777 commented 1 month ago

I think we should definitely align the behavior to that PR. In this scenario, would we ask the original PR author to handle fixing for Android?

I think we should if they have the bandwidth, otherwise we can adjust the issue description and keep this open for new proposals

melvin-bot[bot] commented 3 weeks ago

@tylerkaraszewski, @sakluger, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

ishpaul777 commented 3 weeks ago

I have asked the PR author in the slack thread https://expensify.slack.com/archives/C01GTK53T8Q/p1723486965918939?thread_ts=1721047319.091279&cid=C01GTK53T8Q

melvin-bot[bot] commented 3 weeks ago

@tylerkaraszewski @sakluger @ishpaul777 this issue is now 4 weeks old, please consider:

Thanks!

sakluger commented 3 weeks ago

The PR author (@perunt) is on vacation until Monday. Setting to weekly to keep Melvin happy.

perunt commented 2 weeks ago

Hey guys, I'm back. I'll add this to my list and start working on this soon. So, I'll take care of the suggestion box disappearing immediately

mvtglobally commented 2 weeks ago

Issue not reproducible during KI retests. (First week)

ishpaul777 commented 2 weeks ago

not overdue, @perunt Any updates?

perunt commented 1 week ago

@ishpaul777 the original issue was that the suggestion didn't hide immediately. However, we later agreed that this was expected behavior. Now, the actual issue is the inconsistent behavior between platforms. Is my understanding correct? If yes, then I have a fix for that.

ishpaul777 commented 1 week ago

Now, the actual issue is the inconsistent behavior between platforms.

thats correct @perunt

perunt commented 1 week ago

Nice! I made kind of a one-line fix for that. The problem was that the gesture handler was checking if the view was translucent, so to avoid it, I added a transparent color to that view

ishpaul777 commented 2 days ago

Deployed 3 days ago, automation failed Can we add Awaiting payment label