-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[Due for payment 2025-04-25] [$250] Web - Search - Search is not opened on report page with CMD(ctrl) + K #59643
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
Comments
Triggered auto assignment to @isabelastisser ( |
🚨 Edited by proposal-police: This proposal was edited at 2025-04-04 07:36:30 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Search is not opened. What is the root cause of that problem?When users go to Expense report page, we trigger the following logic to register the input ref
and if the inputRef has value we will prevent showing the search router
What changes do you think we should make in order to solve the problem?We should unregister the inputRef if the Expense report page is removed
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?None What alternative solutions did you explore? (Optional)Update the condition here
to
so isUserOnSearchPage is true only when users are on the search root page. Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
@isabelastisser Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Job added to Upwork: https://www.upwork.com/jobs/~021909318554554905924 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ahmedGaber93 ( |
🚨 Edited by proposal-police: This proposal was edited at 2025-04-09 03:33:29 UTC. ProposalPlease re-state the problem that we are trying to solve in this issueWhen users are viewing the new money request report page ( What is the root cause of that problem?The root cause is in the Currently, it has two main logic paths:
The issue is that the new
What changes do you think we should make in order to solve the problem?The solution is to enhance the
Note This ensures Additionally this addresses the 2nd issue mentioned in the reviewer's comment where the global search input modal opens while the RHP is already opened ✅ Here's a test branch: ikevin127-searchRouterShortcutFix.What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
These tests would verify the correct behavior of Result videoMacOS: Chromeweb.mov |
Note for C+: We shouldn't call
|
This comment has been minimized.
This comment has been minimized.
Thanks all for the proposals I think there is no significant change between the both proposals (main solution), put let us pass this point now because the both cause a regression with me. Regression: search page input not focused after jumping between LHN typesSteps to reproduce:
20250409021956081.mp4I think this because when jumping between types, the new type route will @nkdengineer's your alternative solution works good with me, but after view the expense on RHN and click CMD + K, search router is opened on the right (this also reproduced on main on LHN reports if you click on the header to open RHN then click CMD + K) 20250409023156599.mp4Note: this can fix on PR, but it will be better to be fixed here because I didn't confirm we will move with that solution as I will double-check it again tomorrow Thanks both, I will double-check @nkdengineer's alternative solution again tomorrow and compare it with main solutions if they addressed the "jump between types" regression, and move forward with the best one. |
@ahmedGaber93 If it already happened on main, and it's not related to this issue, I think we can move forward. What do you think? |
Updated proposal
Important Please note the accuracy / details of the RCA compared to other proposals, the order in which the proposals were updated, and which one mentioned the first fully working solution including the RHP issue (which IS within scope of this issue). |
Updated proposal to fix for all RHP cases (alternative solution); we can improve the condition in the PR. But currently, I think it's not a problem if we open the search page when the user is in RHP. |
Let's get @Expensify/design confirmation, is the below behavior expected? And if not, what is the expected behavior? CMD + K shortcut open the search route modal while user in RHN. Steps to reproduce:
Expected: the search route modal shouldn't open while RHN is opened 20250410012313638.mp4and below is the same case in search tab, but it is related to the current issue, and reproduced only with the proposals here 20250409023156599.mp4 |
I know it doesn't look the best, but I really don't like that fact that we don't open the search route modal when using CMD+K even if the RHP is open. To me it should be accessible at any point. My take is that I'd rather show it and try to figure out the visual part in a separate issue. The alternative could be closing the RHP and then showing the search route modal, but I don't love that either. Keen to hear what the other designers think. @Expensify/design |
We currently allow you to launch the search router on top of an open RHP in the Inbox, so I think we should do that here too for consistency's sake. |
We've talked about this one previously in Slack. Down to allow you to launch the router, but agree we need to clean this up. Basically, I agree with Jon's comment here:
|
Thanks for confirmation. |
@nkdengineer could you please roll back your alternative solution, we don't need the last change as we will keep the current behavior. |
@ahmedGaber93 Updatded |
@nkdengineer's alternative solution LGTM! 🎀 👀 🎀 C+ reviewed |
📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.29-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2025-04-25. 🎊 For reference, here are some details about the assignees on this issue:
|
@ahmedGaber93 @isabelastisser @ahmedGaber93 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button] |
BugZero Checklist:
Bug classificationSource of bug:
Where bug was reported:
Who reported the bug:
Regression Test Proposal Template
Regression Test ProposalPrecondition:Test:Do we agree 👍 or 👎 |
@ahmedGaber93 please complete the checklist above. Thanks! |
Payment summary: @ahmedGaber93 requires payment through NewDot Manual Requests $250 C+ review |
BugZero Checklist:
Bug classificationSource of bug:
Where bug was reported:
Who reported the bug:
Regression Test ProposalTest:
Do we agree 👍 or 👎 |
@isabelastisser Checklist is completed! |
All set! |
Uh oh!
There was an error while loading. Please reload this page.
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: v9.1.23-2
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: -
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Mac 14.5/ Chrome
App Component: Search
Action Performed:
Expected Result:
Search is opened and focused
Actual Result:
Search is not opened.
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6791774_1743726112826.Screen_Recording_2025-04-04_at_12.45.58_at_night.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @isabelastisserThe text was updated successfully, but these errors were encountered: