[HOLD for Payment 2024-09-6][$250] Android - Chat - Open a new room/WS chat, entering @ does not display user suggestions

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.5 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?/tests/view/4704455 Issue reported by: Applause - Internal Team

Action Performed:

Launch app
Tap on a workspace or room with no activity
Enter @ in compose box
Note user suggestions are not displayed
Tap on header
Navigate back to conversation page
Keep cursor after @ and note now user suggestions are shown

Expected Result:

Open a new room/WS chat, entering @ must display user suggestions

Actual Result:

Open a new room/WS chat, entering @ does not display user suggestions. It is shown only after tapping header and navigate back to conversation

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/34338215-a0aa-47b2-aa13-2be358871e08

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0180b0b26a5f7f23c0
Upwork Job ID: 1811721981798318944
Last Price Increase: 2024-08-09
Automatic offers:

ikevin127 | Reviewer | 103480415
dominictb | Contributor | 103480416

Issue Owner

Current Issue Owner: @ikevin127

melvin-bot[bot] commented 1 month ago

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

@sonialiap 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 1 month ago

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

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~0180b0b26a5f7f23c0

melvin-bot[bot] commented 1 month ago

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

ikevin127 commented 1 month ago

📃 Issue is definitely reproducible, just reproduced on production as well. On local dev the behaviour might be even worse because of the recent introduction of React StrictMode which might interfere with the react-native-live-markdown implementation.

🔍 Looking for proposals!

melvin-bot[bot] commented 1 month ago

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

ikevin127 commented 1 month ago

Callback to https://github.com/Expensify/App/issues/45008#issuecomment-2227008347 -> 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? 💸

ikevin127 commented 1 month ago

Callback to https://github.com/Expensify/App/issues/45008#issuecomment-2227008347 -> Looking for proposals 🎲

melvin-bot[bot] commented 1 month ago

@sonialiap @ikevin127 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!

melvin-bot[bot] commented 1 month ago

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

ikevin127 commented 1 month ago

Callback to https://github.com/Expensify/App/issues/45008#issuecomment-2227008347 -> 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? 💸

melvin-bot[bot] commented 1 month ago

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

ikevin127 commented 1 month ago

Callback to https://github.com/Expensify/App/issues/45008#issuecomment-2227008347 -> Looking for proposals 🎲

melvin-bot[bot] commented 1 month ago

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

ikevin127 commented 1 month ago

Callback to https://github.com/Expensify/App/issues/45008#issuecomment-2227008347 -> Looking for proposals 🎲

dominictb commented 1 month ago

Proposal

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

Open a new room/WS chat, entering @ does not display user suggestions. It is shown only after tapping header and navigate back to conversation.

What is the root cause of that problem?

This is the problem of auto-focus input during screen animated transition/navigation in Android . Below is the explanation of what happening following each reproduction step

When we first enter a new blank chat, shouldAutoFocus is true. In Android emulator, we can see that the keyboard appear, and we can type in the composer. During typing, this function is triggered, but textInput.current.isFocused() return false. Hence, the selection state here doesn't change, and that means, SuggestionMention cannot calculate and display the mention box.
When we click on the chat header and come back, it will by-pass this condition, which means, the input is manually being focused here. And now, since textInput.current.isFocused() is true, the suggestion box display normally
Further note: Instead of clicking on the chat header and navigate back, we can try by pressing the composer box again to re-focus it correctly

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

This is a known issue and we have introduced useAutoFocusInput hook to overcome this. We'll just need to apply to the Composer native component instead of leaving the auto-focus management to native TextInput (which is used under the hood by react-native-live-markdown)

Below is the sample code change

// Composer/index.native.tsx

const { inputCallbackRef } = useAutoFocusInput();

    useEffect(() => {
        if(!autoFocus) {
            inputCallbackRef(null);
            return;
        }
        if(textInput.current) {
            inputCallbackRef(textInput.current)
        }
    }, [autoFocus, inputCallbackRef])

    /**
     * Set the TextInput Ref
     * @param {Element} el
     */
    const setTextInputRef = useCallback((el: AnimatedMarkdownTextInputRef) => {
        // eslint-disable-next-line react-compiler/react-compiler
        textInput.current = el;
        if (typeof ref !== 'function' || textInput.current === null) {
            return;
        }

        if(autoFocus) {
            inputCallbackRef(el);
        }

        // This callback prop is used by the parent component using the constructor to
        // get a ref to the inner textInput element e.g. if we do
        // <constructor ref={el => this.textInput = el} /> this will not
        // return a ref to the component, but rather the HTML element by default
        ref(textInput.current);
        // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
    }, [autoFocus, ref, inputCallbackRef]);

What alternative solutions did you explore? (Optional)

NA

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? 💸

ikevin127 commented 1 month ago

Reviewing proposal.

melvin-bot[bot] commented 1 month ago

@sonialiap @ikevin127 this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

ikevin127 commented 1 month ago

♻️ Will finish reviewing the proposal today.

ikevin127 commented 1 month ago

@dominictb Sorry for the delay in reviewing! Thank you for taking the time to come up with a quality proposal for fixing this 1 month old issue 👴

Dominic's proposal looks good to me. The root cause is well documented and every single point described in the RCA checks-out. The proposed solution fixes the issue accordingly, using the already existing useAutoFocusInput hook, aligning the Android behaviour with the one on iOS -> fulfilling the Expected result of the issue.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 3 weeks 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 3 weeks ago

@youssef-lr, @sonialiap, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

ikevin127 commented 3 weeks ago

We're currently awaiting @youssef-lr's contributor assignment based on proposal review.

melvin-bot[bot] commented 3 weeks ago

📣 @ikevin127 🎉 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 3 weeks ago

📣 @dominictb 🎉 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 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.

sonialiap commented 2 weeks ago

@kadiealexander I'm OOO Aug 19-30, adding leave buddy. Satus: waiting on @youssef-lr to assign contributor

ikevin127 commented 2 weeks ago

Status update: PR is open, contributor is re-working solution because performance tests are failing with initial solution.

ikevin127 commented 2 weeks ago

Status update: PR is ready for merge, we're currently awaiting on CME for final 🟢 Approve and merge.

melvin-bot[bot] commented 1 week ago

@youssef-lr, @sonialiap, @ikevin127, @kadiealexander, @dominictb Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 5 days ago

@youssef-lr, @sonialiap, @ikevin127, @kadiealexander, @dominictb Huh... This is 4 days overdue. Who can take care of this?

ikevin127 commented 5 days ago

Status update: PR was merged to staging 🟢, did not get to production yet ⚠️.

@youssef-lr do you happen to have more context on why the PR was not deployed to production yet ? Additionally, I noticed you closed a revert PR recently, what happened - something wrong with the initial PR ?

Nevermind, figured out what happened - see coment below👇

ikevin127 commented 5 days ago

⚠️ We just got confirmation on Slack that the Deploy Checklist: New Expensify 2024-08-26 which includes the PR of this issue was only deployed to production today in Deploy Checklist: New Expensify 2024-08-28. More context on why this happened can be found in this Slack thread and this Slack comment.

Given the context above, this issue should be on [HOLD for Payment 2024-09-6] according to today’s production deploy from Deploy Checklist: New Expensify 2024-08-28.

cc @sonialiap @youssef-lr

sonialiap commented 2 days ago

Thanks Kevin, I've updated the title

ikevin127 commented 4 hours ago

[x] [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: N/A.
[x] [@ikevin127] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A.
[x] [@ikevin127] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A.
[x] [@ikevin127] Determine if we should create a regression test for this bug.
[x] [@ikevin127] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

Launch app.
Tap on a workspace or room with no activity.
Input @ in the composer box.
Verify that user suggestions popup is displayed.

Do we agree 👍 or 👎.

Expensify / App