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

[Payment now] [$1000] Compose box - Suggestion list not displays when open group chat and write '@' #24265

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 8, 2023 · 37 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 8, 2023

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:

Precondition: user should be signed in and have a group chat with several members

  1. Open app
  2. Navigate to group chat
  3. Tap on the compose box
  4. Write '@"

Expected Result:

Suggestion list should be displayed

Actual Result:

Nothing happens

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.51.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6157704_Suggestion_list_not_displayed_on_iphone_14_Pro_Max.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01597e4b673ae39722
  • Upwork Job ID: 1689186963484561408
  • Last Price Increase: 2023-08-09
  • Automatic offers:
    • WikusKriek | Contributor | 26156328
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 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

@jeet-dhandha
Copy link
Contributor

Proposal

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

  • On first character typed for iOS as '@' mention suggestions are not shown.

What is the root cause of that problem?

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

  • We should wait for the state to be updated before checking if the value is empty or not.

  • Changing below function

onSelectionChange(e) {
LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));
this.setState({selection: e.nativeEvent.selection});
if (!this.state.value || e.nativeEvent.selection.end < 1) {
this.resetSuggestions();
this.shouldBlockEmojiCalc = false;
this.shouldBlockMentionCalc = false;
return;
}
this.calculateEmojiSuggestion();
this.calculateMentionSuggestion();
}

Updated Function Code:
  • Here we are moving the this.state.value check code in the setTimeout function. This will make sure that the state is updated before we check the value. Also we are only setting the timeout if the value is empty and the OS is iOS. This will make sure that we don't have any performance issues.
onSelectionChange(e) {
    LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));
    const selection = e.nativeEvent ? e.nativeEvent.selection : {};
    this.setState({selection});
    const shouldWait = this.state.value === '' && getOperatingSystem() === CONST.OS.IOS;
    const ms = shouldWait ? 10 : 0;
    setTimeout(() => {
        if (!this.state.value || selection.end < 1) {
            this.resetSuggestions();
            this.shouldBlockEmojiCalc = false;
            this.shouldBlockMentionCalc = false;
            return;
        }
        this.calculateEmojiSuggestion();
        this.calculateMentionSuggestion();
    }, ms);
}

What alternative solutions did you explore? (Optional)

  • We should find why is this not happening on other platforms.

@dukenv0307
Copy link
Contributor

Proposal

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

Compose box - Suggestion list not displays when open group chat and write '@'

What is the root cause of that problem?

We calculate the mentioned suggestion on onSelectionChange. But onSelectionChange is triggered before onChangeText (native behavior on IOS)

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

We just have the same issue before. @s77rt @mollfpr Ping you here because you have context in #12854

More reference: facebook/react-native#28865

What alternative solutions did you explore? (Optional)

@conorpendergrast
Copy link
Contributor

Reproduced on iOS native app, and not on OSX desktop.

@conorpendergrast conorpendergrast added the External Added to denote the issue can be worked on by a contributor label Aug 9, 2023
@melvin-bot melvin-bot bot changed the title Compose box - Suggestion list not displays when open group chat and write '@' [$1000] Compose box - Suggestion list not displays when open group chat and write '@' Aug 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01597e4b673ae39722

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 2023

Current assignee @conorpendergrast is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 2023

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

@parasharrajat
Copy link
Member

The root cause is that the state is not updated yet when onSelectionChange is called.

This is due to the fact that the state is updated in a different function and onSelectionChange is called parallel to it. This happens only on iOS for the first character typed.

@jeet-dhandha These are confusing. can you update your proposal to clarify the root cause in detail?

The state is updated in a different function and onSelectionChange is called parallel to it

How does it create the issue?

@parasharrajat
Copy link
Member

I would love to hear more context @s77rt @mollfpr.

@WikusKriek
Copy link
Contributor

Proposal

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

On iOS when you launch a chat and type @ the suggestions do not open.

What is the root cause of that problem?

On iOS the this.state.value updates after the onSelectionChange event only for the first character typed. It seems like the component is not properly mounted and has this side effect that onSelectionChange runs before the value gets set. Resulting in an early return because !this.state.value is true and the calculateMentionSuggestion not running. As soon as you type @ remove it and type @ again it works fine.

onSelectionChange(e) {
LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));
this.setState({selection: e.nativeEvent.selection});
if (!this.state.value || e.nativeEvent.selection.end < 1) {
this.resetSuggestions();
this.shouldBlockEmojiCalc = false;
this.shouldBlockMentionCalc = false;
return;
}
this.calculateEmojiSuggestion();
this.calculateMentionSuggestion();
}

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

We should revert this commit changes https://github.com/Expensify/App/pull/19334/files. The issue that these changes aimed to fix are no longer a problem since new upstream changes.

Specifically the early return check for !this.state.value needs to be removed in onSelectionChange. It could be moved back to calculateEmojiSuggestion() where it was prior to the changes. And e.nativeEvent.selection.end < 1 does not seem to be making any difference and could also be either moved back or completely removed.

What alternative solutions did you explore? (Optional)

N/A

@mollfpr
Copy link
Contributor

mollfpr commented Aug 9, 2023

We calculate the mentioned suggestion on onSelectionChange. But onSelectionChange is triggered before onChangeText (native behavior on IOS)

@dukenv0307 Shouldn't the issue fixed by this PR facebook/react-native#35603?

@parasharrajat
Copy link
Member

Thanks for your proposal @WikusKriek Checking it...

@jeet-dhandha
Copy link
Contributor

@parasharrajat i will update the proposal with better description.

@jeet-dhandha
Copy link
Contributor

Proposal [UPDATED] => (#24265 (comment))

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

  • On first character typed for iOS as '@' mention suggestions are not shown.

What is the root cause of that problem?

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

  • We can add a 10ms timeout in onSelectionChange function.

  • Code change will be in below function in ReportActionCompose.js file.

    onSelectionChange(e) {
    LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));
    this.setState({selection: e.nativeEvent.selection});
    if (!this.state.value || e.nativeEvent.selection.end < 1) {
    this.resetSuggestions();
    this.shouldBlockEmojiCalc = false;
    this.shouldBlockMentionCalc = false;
    return;
    }
    this.calculateEmojiSuggestion();
    this.calculateMentionSuggestion();
    }

Updated Function Code:
onSelectionChange(e) {
    LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));
    const selection = e.nativeEvent ? e.nativeEvent.selection : {};
    this.setState({selection});
    const shouldWait = this.state.value === '' && getOperatingSystem() === CONST.OS.IOS;
    const ms = shouldWait ? 10 : 0;
    
    // We need to wait for the `this.state.value` state to be updated before we can calculate the suggestions
    // Added setTimeout due to https://github.com/facebook/react-native/issues/28865 (upstream issues)
    setTimeout(() => {
        // Still keeping this code as to reset the suggestions if the user removes all the text
        if (!this.state.value || selection.end < 1) {
            this.resetSuggestions();
            this.shouldBlockEmojiCalc = false;
            this.shouldBlockMentionCalc = false;
            return;
        }
        this.calculateEmojiSuggestion();
        this.calculateMentionSuggestion();
    }, ms);
}

What alternative solutions did you explore? (Optional)

  • N/A

@dukenv0307
Copy link
Contributor

@mollfpr It dupe this issue #12854 , right?

@parasharrajat
Copy link
Member

parasharrajat commented Aug 10, 2023

@dukenv0307 Does not seem so.

@WikusKriek I checked but the issue is still present after I reverted the changes. Can you show a video of the working solution?

@jeet-dhandha SetTimeout-based solutions are not preferred and you haven't clarified why Settimout is needed. Please add explanation with solution on how it fits on the system to solve the issue.

@WikusKriek
Copy link
Contributor

WikusKriek commented Aug 10, 2023

Screen.Recording.2023-08-10.at.15.09.59.mov

@parasharrajat this is what I did, main...WikusKriek:App:24265-Suggestion-list-not-display-when-open-group-chat

@parasharrajat
Copy link
Member

Try this with room chat or group chat instead of DM.

@WikusKriek
Copy link
Contributor

WikusKriek commented Aug 10, 2023

Try this with room chat or group chat instead of DM.

Screen.Recording.2023-08-10.at.16.07.30.mov

@parasharrajat is it not working on your side?

@parasharrajat
Copy link
Member

Going to test your diff...

@parasharrajat
Copy link
Member

@WikusKriek's proposal looks good to me. We just need to revert the extra changes.

🎀 👀 🎀 C+ reviewed

@parasharrajat
Copy link
Member

Bump @Beamanator

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 16, 2023

📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($1000)

@melvin-bot
Copy link

melvin-bot bot commented Aug 16, 2023

📣 @WikusKriek 🎉 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 📖

@Beamanator
Copy link
Contributor

@parasharrajat thanks for the bump, I like @WikusKriek 's proposal as well 👍

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Aug 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 18, 2023

🎯 ⚡️ Woah @parasharrajat / @WikusKriek, 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 🎉

  • when @WikusKriek got assigned: 2023-08-16 14:14:28 Z
  • when the PR got merged: 2023-08-18 11:17:21 UTC

On to the next one 🚀

@melvin-bot
Copy link

melvin-bot bot commented Aug 22, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@WikusKriek
Copy link
Contributor

This is in production for more than 7 days, but no title update. @Beamanator can you please provide some update here on status and payments. Thanks!

@parasharrajat
Copy link
Member

Yeah, it seems the automation failed on this.

@parasharrajat
Copy link
Member

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:

  • [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:

After we fixed the native selection issues in RN, the patch applied in #19334 became redundant and caused this issue.

  • [@parasharrajat] 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: It's not the problem on Fix mentions results does not close when we change focus to first character #19334.

  • [@parasharrajat] 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: I don't think we could have done anything better. We fixed the native and the workaround should have been removed. It was not known that the workaround would cause such an issue.

  • [@parasharrajat] Determine if we should create a regression test for this bug. Yes

  • [@parasharrajat] 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 Steps

  1. Open app
  2. Navigate to group chat
  3. Tap on the compose box
  4. Type "@"
  5. Verify that the suggestion list displays

Do you agree 👍 or 👎 ?

@conorpendergrast conorpendergrast added the Awaiting Payment Auto-added when associated PR is deployed to production label Sep 4, 2023
@conorpendergrast conorpendergrast changed the title [$1000] Compose box - Suggestion list not displays when open group chat and write '@' [Payment now] [$1000] Compose box - Suggestion list not displays when open group chat and write '@' Sep 4, 2023
@conorpendergrast conorpendergrast added Daily KSv2 and removed Weekly KSv2 labels Sep 4, 2023
@conorpendergrast
Copy link
Contributor

I missed this today, and will pay this tomorrow 👍

@conorpendergrast
Copy link
Contributor

Payouts due:

Issue Reporter: N/A - Applause

  • Contributor: $1000 + 50% urgency = $1500 @WikusKriek - via Upwork

Contributor+: $1000 + 50% urgency = $1500 @parasharrajat - via manual request please!

Eligible for 50% #urgency bonus? Yes

Upwork job is here.

@conorpendergrast
Copy link
Contributor

Oh wait, hah, this doesn't need a new regression test - it was caught via regression testing. Ok, that's all done now. Closing!

@parasharrajat
Copy link
Member

Payment requested as per #24265 (comment)

@JmillsExpensify
Copy link

$1,500 payment for @parasharrajat approved based on BZ summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants