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.53k stars 2.88k forks source link

[HOLD for payment 2023-11-01] [HOLD for payment 2023-10-13] [$500] Chat - App displays mention suggestion for sometime when we remove space from before @ #28170

Closed kbecciv closed 1 year ago

kbecciv commented 1 year 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!

Action Performed:

  1. Open the app
  2. Open any report
  3. Type @, it will trigger mention suggestions
  4. Add space before @
  5. Remove the space and observe that suggestions are displayed for sometime and then dismissed

Expected Result:

App should not display suggestions on removing space before @

Actual Result:

App displays suggestions on removing space before @ for sometime

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.74.0 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

https://github.com/Expensify/App/assets/93399543/48d46156-7678-4827-a5dc-acb1af90ffe6

https://github.com/Expensify/App/assets/93399543/964e79e3-c452-4a51-b98b-05f42af1c718

https://github.com/Expensify/App/assets/93399543/1d46c808-c5db-4984-b8c4-1968426e3510

https://github.com/Expensify/App/assets/93399543/92c8c795-b9a4-4cdc-85e2-bcc05b9b4df9

https://github.com/Expensify/App/assets/93399543/969a841a-f864-4c6f-994e-fb09deb6df17

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0193240eea08f27c8d
  • Upwork Job ID: 1706350665337753600
  • Last Price Increase: 2023-09-25
  • Automatic offers:
    • cubuspl42 | Reviewer | 26974438
    • paultsimura | Contributor | 26974441
    • dhanashree-sawant | Reporter | 26974442
melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0193240eea08f27c8d

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

paultsimura commented 1 year ago

Proposal

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

The mentions suggestion appears for a moment when the symbol before "@" is removed.

What is the root cause of that problem?

The root cause is in this hook:

https://github.com/Expensify/App/blob/26cd27fb8e6982ce8bedc393c585f3679e4935ab/src/pages/home/report/ReportActionCompose/SuggestionMention.js#L235-L237

It is supposed to run only when the selection changes. However, calculateMentionSuggestion is also in the dependencies list of this hook. And this function has the value in its dependencies list:

https://github.com/Expensify/App/blob/26cd27fb8e6982ce8bedc393c585f3679e4935ab/src/pages/home/report/ReportActionCompose/SuggestionMention.js#L232

This leads to the calculateMentionSuggestion function being changed every time a value is changed. And therefore the initial hook is called on every selection & value change.

The hook is called 2 times on every change:

  1. With changed value but previous selection
  2. With changed value and new selection

As a result, on step 1, the cursor is calculated as standing after the "@" sign:

image

And the menu becomes visible in this chunk of code until the step 2 takes place:

https://github.com/Expensify/App/blob/26cd27fb8e6982ce8bedc393c585f3679e4935ab/src/pages/home/report/ReportActionCompose/SuggestionMention.js#L220-L224

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

We should execute this hook only if the selection has changed:

Option 1 (preferred, although not lint-friendly):

    useEffect(() => {
        calculateMentionSuggestion(selection.end);

+       // we want this hook to run only on selection change
+       // eslint-disable-next-line react-hooks/exhaustive-deps
+   }, [selection]);
-   }, [selection, calculateMentionSuggestion]);

What alternative solutions did you explore? (Optional)

Option 2 (lint-friendly, but with more complex checks):

    const prevSelection = usePrevious(selection);
    const didSelectionChange = !_.isEqual(prevSelection, selection);

    useEffect(() => {
        if (!didSelectionChange) {
            return;
        }

        calculateMentionSuggestion(selection.end);
    }, [didSelectionChange, selection, calculateMentionSuggestion]);
zanyrenney commented 1 year ago

dupe assignment, i got this @stephanieelliott - unassigning you.

cubuspl42 commented 1 year ago

I approve the proposal by @paultsimura. The RCA looks fine, and the fix is simple.

C+ reviewed 🎀 👀 🎀

melvin-bot[bot] commented 1 year ago

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

lakchote commented 1 year ago

Solution looks good to me. Assigning now.

melvin-bot[bot] commented 1 year ago

📣 @cubuspl42 🎉 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[bot] commented 1 year ago

📣 @paultsimura 🎉 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[bot] commented 1 year ago

📣 @dhanashree-sawant 🎉 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

paultsimura commented 1 year ago

Thanks, the PR is ready for review.

melvin-bot[bot] commented 1 year ago

🎯 ⚡️ Woah @cubuspl42 / @paultsimura, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

On to the next one 🚀

melvin-bot[bot] commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.76-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-10. :confetti_ball:

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.

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

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:

amyevans commented 1 year ago

Crossposting that a regression has been identified: https://github.com/Expensify/App/issues/28657

stephanieelliott commented 1 year ago

Updated Hold period to match the payout date of the regression.

cubuspl42 commented 1 year ago

@stephanieelliott I don't think we're eligible for payment here, at least for now. We had to revert the solution, and we're still investigating other options that will both solve this issue and don't have undesired side effects.

The fact that this is related to a bug in React Native itself doesn't help.

@paultsimura What's the latest status here? I know you've been discussing this on Slack

zanyrenney commented 1 year ago

Yep, i think you're right @cubuspl42 this wouldn't qualify for payment. Let's link the slack conversation if there is one?

paultsimura commented 1 year ago

Yep, i think you're right @cubuspl42 this wouldn't qualify for payment. Let's link the slack conversation if there is one?

Here's the conversation, unfortunately it doesn't get the responses quite frequently: https://expensify.slack.com/archives/C01GTK53T8Q/p1696421170146759

cubuspl42 commented 1 year ago

@paultsimura Did we explore the "Option 2 (lint-friendly, but with more complex checks)"? Does it also has the undesired side-effects?

paultsimura commented 1 year ago

@cubuspl42 unfortunately yes, it had the same side-effect. However, I've come up with a new workaround approach that seem to do the job. Would you be able to test this approach anytime soon?

In SuggestionMention:

  1. Add a new state variable:
const [shouldRecalculateSuggestions, setShouldRecalculateSuggestions] = useState(false);
  1. Modify the hook I initially wanted to modify: 
    
useEffect(() => {
+        if (selection.end > 0 && !lodashGet(value, 'length', 0)) {
+            // This is a workaround for a known issue with iOS' first input.
+            // See: https://github.com/facebook/react-native/pull/36930#issuecomment-1593028467
+            setShouldRecalculateSuggestions(true);
+        }
    calculateMentionSuggestion(selection.end);

  1. Add a new hook:

    useEffect(() => {
    // This hook solves the issue with iOS' first input:
    // It enables showing the mention suggestions after the user enters '@' as a first char in the Composer.
    // See: https://github.com/facebook/react-native/pull/36930#issuecomment-1593028467
    if (!shouldRecalculateSuggestions) {
        return;
    }

    setShouldRecalculateSuggestions(false);
    calculateMentionSuggestion(selection.end);

    // We want this hook to run only on value change.
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, [value]);

This will allow handling the regression that was discovered, when the first input in RN iOS is handled in the wrong order:

https://github.com/Expensify/App/assets/12595293/802b436a-5477-4b66-b642-3164c77c9624

shubham1206agra commented 1 year ago

Please test https://github.com/Expensify/App/issues/28749 with the proposed solution

zanyrenney commented 1 year ago

who are you asking to test @shubham1206agra - please add the specific user you are directing your comments too so that we can understand better and keep pushing this forward with urgency?

cubuspl42 commented 1 year ago

@paultsimura Do you have a Git branch with these changes at hand?

paultsimura commented 1 year ago

@cubuspl42 here it is: https://github.com/paultsimura/Expensify/tree/fix/28170-mention-on-delete However, @shubham1206agra is right – this brings back the https://github.com/Expensify/App/issues/28749. Working on it...

paultsimura commented 1 year ago

Ok, seems like I have a workaround that covers all the cases I currently know. It's not elegant, but it's working:

+   const previousValue = usePrevious(value);

    useEffect(() => {
+       if (value.length < previousValue.length) {
+           return;
+       }

        calculateMentionSuggestion(selection.end);
    }, [selection, calculateMentionSuggestion]);

Since the hook is called 2 times: on value change and on selection change, it's safe to early return on deletion – in this case, only the correct update of value and selection should trigger the suggestions. On the input, the hook will still be called 2 times, so both normal (non-iOS) and buggy iOS flows should be covered.

@cubuspl42 please test when you have a spare minute. The changes are in the branch already (with some debugging logs for the moment). cc: @shubham1206agra

cubuspl42 commented 1 year ago

@paultsimura This might be working! But would you also test this branch and share your thoughts?

paultsimura commented 1 year ago

@paultsimura This might be working! But would you also test this branch and share your thoughts?

Unfortunately, the branch doesn't solve the current issue, does it work for you @cubuspl42 ?

https://github.com/Expensify/App/assets/12595293/8ece5d60-86ea-4658-9286-9688e8da785e

It's almost the same dependencies set as in current production – the calculateMentionSuggestion in our hook's dependencies changes with every value change (which is in calculateMentionSuggestion's dependencies list). So replacing calculateMentionSuggestion with value eventually doesn't change anything, as far as I can see 🤔

cubuspl42 commented 1 year ago

But would you also test this branch and share your thoughts?

@paultsimura Forget this. This doesn't work, as you observed. I tried really many work-around ideas (none worked) and I think I forgot what problem I'm workarounding in the end, however dumb that sounds.

paultsimura commented 1 year ago

I forgot what problem I'm workarounding in the end

I had the exact same situation at some point😅 It was because I was trying to find a workaround that solves all the issues at once. But after none of them worked, moved to just straightforward "don't show suggestions on removal" workaround.

cubuspl42 commented 1 year ago

You can file a PR with your best idea so far, we can move the discussion there

melvin-bot[bot] commented 1 year ago

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

On to the next one 🚀

melvin-bot[bot] commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.90-2 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-11-01. :confetti_ball:

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.

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

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:

zanyrenney commented 1 year ago

@cubuspl42 requires payment offer (Reviewer) - paid $500 @paultsimura requires payment offer (Contributor) paid $500 @dhanashree-sawant requires payment offer (Reporter) paid $50

no bonus as 15 days elapsed.

all payments complete. Closing!