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

[$500] Web - Pencil icon appears for the last chat room when typing after popover is open from emoji reaction #27046

Closed
6 tasks
kbecciv opened this issue Sep 8, 2023 · 42 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@kbecciv
Copy link

kbecciv commented Sep 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:

  1. Open the website
  2. Open any chat from LHN
  3. Send a message and react to the message
  4. Open reaction detail by making right click
  5. Type something and send the message
  6. Open any other chat from LHN
  7. Repeat steps 3 to 4
  8. Type something
  9. Observe the LHN

Expected Result:

The pencil icon should only appear on the current chat

Actual Result:

The pencil icon appears on the current and last chat

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

Screenshare.-.2023-09-04.3_16_54.PM.mp4
Recording.4339.mp4

Expensify/Expensify Issue URL:
Issue reported by: @misgana96
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693829484228389

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012b0e708164831d25
  • Upwork Job ID: 1700197539850969088
  • Last Price Increase: 2023-09-29
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 8, 2023
@melvin-bot melvin-bot bot changed the title Web - Pencil icon appears for the last chat room when typing after popover is open from emoji reaction [$500] Web - Pencil icon appears for the last chat room when typing after popover is open from emoji reaction Sep 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

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

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

melvin-bot bot commented Sep 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

@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

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

@zukilover
Copy link
Contributor

Proposal

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

The pencil icon appears on the current and last chat

What is the root cause of that problem?

replaceSelectionWithText that is responsible for text insertion at selection point is excessively called within focusComposerOnKeyPress which is a handler for various event including navigation blur/focus. This is unnecessary because replaceSelectionWithText is already being executed in an useImperativeHandle hook.

focus();
replaceSelectionWithText(e.key, false);
},
[checkComposerVisibility, focus, replaceSelectionWithText],

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

Remove replaceSelectionWithText from focusComposerOnKeyPress event handler, on line 388:

                   focus();
            // Remove this: replaceSelectionWithText(e.key, false);
        },
        [
            checkComposerVisibility, 
            focus, 
            // Remove this: replaceSelectionWithText
        ],

What alternative solutions did you explore? (Optional)

N/A

@jeet-dhandha
Copy link
Contributor

jeet-dhandha commented Sep 9, 2023

Proposal

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

  • Editing the composer edits the previous route's composer too.

What is the root cause of that problem?

  • The issues is that when navigating to new report the previous report's composer is still mounted and thus editing the composer edits the previous route's composer too.

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

  • The solution is to check if the Navigation.getActiveRoute() has current reportID or not. If not then early return the updateComment function.
const updateComment = useCallback(
        (commentValue, shouldDebounceSaveComment) => {
            const isActiveReportID = Navigation.getActiveRoute().includes(reportID);
            if (!isActiveReportID) return; // early return if not active reportID
...rest

What alternative solutions did you explore? (Optional)

  • N/A

@sophiepintoraetz sophiepintoraetz removed their assignment Sep 11, 2023
@melvin-bot melvin-bot bot added the Overdue label Sep 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

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

@JmillsExpensify
Copy link

Huh, that's odd. I agree we should resolve, so I'm triaging.

@melvin-bot melvin-bot bot removed the Overdue label Sep 13, 2023
@JmillsExpensify
Copy link

We're open for proposals.

@jeet-dhandha
Copy link
Contributor

Any thoughts on my proposal #27046 (comment)

@melvin-bot melvin-bot bot added the Overdue label Sep 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

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

@tienifr
Copy link
Contributor

tienifr commented Sep 17, 2023

This is no longer reproducible on staging.

@misgana96
Copy link

misgana96 commented Sep 17, 2023

This is because the internal teams decided to disallow typing when popover is open. #27250

Am I eligible for the reporter bonus? The issue has fixed by disabling typing when the popover is open.

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

@JmillsExpensify, @allroundexperts Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2023

@JmillsExpensify, @allroundexperts Still overdue 6 days?! Let's take care of this!

@misgana96
Copy link

@JmillsExpensify @allroundexperts It is not reproducible anymore. This is because the internal teams decided to disallow typing when the popover is open. #27250

Am I eligible for the reporter bonus? The issue has been fixed by disabling typing when the popover is open.

@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2023

@JmillsExpensify @allroundexperts 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 melvin-bot bot added the Weekly KSv2 label Sep 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2023

This issue has not been updated in over 14 days. @JmillsExpensify, @allroundexperts eroding to Weekly issue.

@melvin-bot melvin-bot bot removed the Overdue label Sep 29, 2023
@allroundexperts
Copy link
Contributor

@JmillsExpensify can we please close this since this is not reproducible anymore.

@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2023

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

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2023

@JmillsExpensify @allroundexperts this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added Daily KSv2 Internal Requires API changes or must be handled by Expensify staff and removed Weekly KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Oct 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2023

Current assignee @allroundexperts is eligible for the Internal assigner, not assigning anyone new.

@misgana96
Copy link

@JmillsExpensify @allroundexperts It is not reproducible anymore. This is because the internal teams decided to disallow typing when the popover is open. #27250

Am I eligible for the reporter bonus? The issue has been fixed by disabling typing when the popover is open.

@melvin-bot melvin-bot bot added the Overdue label Oct 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

@JmillsExpensify
Copy link

Okay, so after some discussion, we've decided that it makes sense to disallow typing when any pop up is open, so we don't need to work on this issue - as this is now expected behavior. Thanks for your patience!

Given that we didn't award any reporting bonus for the other issue, I don't see why we're award it here either?

@melvin-bot melvin-bot bot removed the Overdue label Oct 10, 2023
@misgana96
Copy link

@JmillsExpensify The other issue is the expected behavior so they didn't award any reporting bonus, but here the issue is a valid issue and it was fixed by disallowing typing when any pop-up is open. I feel these two cases are different.

@melvin-bot melvin-bot bot added the Overdue label Oct 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

@JmillsExpensify, @allroundexperts Eep! 4 days overdue now. Issues have feelings too...

@JmillsExpensify
Copy link

JmillsExpensify commented Oct 16, 2023

That's fair. Payment summary: $50 payment to @misgana96 for issue reporting.

@melvin-bot melvin-bot bot removed the Overdue label Oct 16, 2023
@JmillsExpensify
Copy link

Upwork job is here: https://www.upwork.com/jobs/~015b38bc9aeb0ce5c0. Please apply and I'll issue payment.

@misgana96
Copy link

@JmillsExpensify I applied for the job.

@JmillsExpensify
Copy link

Offer sent

@misgana96
Copy link

Offer accepted.

@JmillsExpensify
Copy link

Thanks, all paid out!

@misgana96
Copy link

Accepted. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

9 participants