Skip to content

[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

Closed
2 of 8 tasks
IuliiaHerets opened this issue Apr 4, 2025 · 31 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

@IuliiaHerets
Copy link

IuliiaHerets commented Apr 4, 2025

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:

  1. Login to an account that has a report with multiple expenses
  2. Go to Reports > Expense reports
  3. Click on any report with multiple expenses
  4. Press CMD(ctrl) + K

Expected Result:

Search is opened and focused

Actual Result:

Search is not opened.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

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
  • Upwork Job URL: https://www.upwork.com/jobs/~021909318554554905924
  • Upwork Job ID: 1909318554554905924
  • Last Price Increase: 2025-04-07
  • Automatic offers:
    • nkdengineer | Contributor | 106880633
Issue OwnerCurrent Issue Owner: @isabelastisser
@IuliiaHerets IuliiaHerets added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Apr 4, 2025
Copy link

melvin-bot bot commented Apr 4, 2025

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

@nkdengineer
Copy link
Contributor

nkdengineer commented Apr 4, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-04-04 07:36:30 UTC.

Proposal

Please 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

registerSearchPageInput(textInputRef.current);

and if the inputRef has value we will prevent showing the search router

if (isUserOnSearchPage && searchPageInputRef.current) {

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

useEffect(() => {

          if(!isFocused && textInputRef.current) {
                unregisterSearchPageInput();
                 return;
            }
        if (displayNarrowHeader || !isFocused || !textInputRef.current) {
            return;
        }

        registerSearchPageInput(textInputRef.current);
    }, [isFocused, registerSearchPageInput, displayNarrowHeader]);

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

const isUserOnSearchPage = isSearchTopmostFullScreenRoute();

to

const searchFullScreenRoutes = navigationRef.getRootState()?.routes.findLast((route) => route.name === NAVIGATORS.SEARCH_FULLSCREEN_NAVIGATOR);
const lastRoute = searchFullScreenRoutes?.state?.routes?.at(-1);
const isUserOnSearchPage = isSearchTopmostFullScreenRoute() && lastRoute?.name === SCREENS.SEARCH.ROOT;

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.

@melvin-bot melvin-bot bot added the Overdue label Apr 7, 2025
Copy link

melvin-bot bot commented Apr 7, 2025

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

@isabelastisser isabelastisser added 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 Apr 7, 2025
@melvin-bot melvin-bot bot changed the title Web - Search - Search is not opened on report page with CMD(ctrl) + K [$250] Web - Search - Search is not opened on report page with CMD(ctrl) + K Apr 7, 2025
Copy link

melvin-bot bot commented Apr 7, 2025

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

Copy link

melvin-bot bot commented Apr 7, 2025

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

@ikevin127
Copy link
Contributor

ikevin127 commented Apr 8, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-04-09 03:33:29 UTC.

Proposal

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

When users are viewing the new money request report page (SEARCH_MONEY_REQUEST_REPORT route), pressing CMD+K doesn't open the search router modal. Normally, CMD+K should either focus / open the existing search input if on a search page with an input, or open the global search input modal if not.

What is the root cause of that problem?

The root cause is in the SearchRouterContext -> toggleSearch() function, which determines what happens when CMD+K is pressed.

Currently, it has two main logic paths:

  1. If the user is on a search page (isSearchTopmostFullScreenRoute() === true) AND there's a registered search input, focus that input.
  2. Otherwise, toggle the global search input modal.

The issue is that the new SEARCH_MONEY_REQUEST_REPORT page route:

  • Is rendered in the central panel as part of the Report (search) page (so isSearchTopmostFullScreenRoute() returns true)
  • Doesn't display a search input (shouldDisplaySearch={false} on TopBar)
  • Has no registered search input
  • Falls into a logic gap where none of the toggleSearch() conditions are satisfied

⚠️ Thus, pressing CMD+K does nothing on this page.

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

The solution is to enhance the toggleSearch() function in SearchRouterContext to specifically check for the new money request report page route, which is a special case of search pages:

  1. Keep checking if we're on a search page via isSearchTopmostFullScreenRoute().
  2. Add a new check to identify if we're specifically on the money request report page.
  3. Only try to focus an input if we're on a search page, we have a registered input AND WE'RE NOT on the money request report page.
  4. Otherwise, open the global search input modal ✅

Note

This ensures CMD+K works correctly on all pages, including the new money request report page where we want to open the global search input modal.

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?

  1. Test CMD+K on the main search page (SEARCH_ROOT) - should focus the search input
  2. Test CMD+K on a money request report page (SEARCH_MONEY_REQUEST_REPORT) - should open the global search input modal
  3. Test CMD+K on a non-search page - should open the global search input modal
  4. Test CMD+K when the global search input modal is already open - should close it
  5. Test CMD+K when on a search page with the input already focused - should blur the input

These tests would verify the correct behavior of toggleSearch() across all the application states, ensuring CMD+K always performs a sensible action regardless of the current page or state.

Result video

MacOS: Chrome
web.mov

@nkdengineer
Copy link
Contributor

nkdengineer commented Apr 8, 2025

Note for C+: We shouldn't call unregisterSearchPageInput in cleanup function for some following reasons:

  1. We just do it if we always need to register the new one on useEffect (for example event listener, ...), but in this case the inputRef doesn't change
  2. Currently we just assign input ref when textInputRef.current doesn't have value, if we trigger unregisterSearchPageInput in cleanup function, the check !textInputRef.current is useless and we have to reassign every time the dependencies change.

@ikevin127

This comment has been minimized.

@ahmedGaber93
Copy link
Contributor

ahmedGaber93 commented Apr 9, 2025

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 types

Steps to reproduce:

  1. Open "Reports" tab
  2. On LHN jump to another type
  3. Click CMD + K
  4. Verify the search page input is not focused, and search router is opened on the right.
20250409021956081.mp4

I think this because when jumping between types, the new type route will registerSearchPageInput first, then the previous type route will unregisterSearchPageInput, so the final result is unregistered input.

@nkdengineer @ikevin127


@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.mp4

Note: 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.

@nkdengineer
Copy link
Contributor

this also reproduced on main on LHN reports if you click on the header to open RHN then click CMD + K

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

@ikevin127
Copy link
Contributor

ikevin127 commented Apr 9, 2025

Updated proposal

  • addressed both issues mentioned in review (including the search router modal opening while RHP is already open) ✅

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

@nkdengineer
Copy link
Contributor

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.

@ahmedGaber93
Copy link
Contributor

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:

  1. open any chat
  2. click on the header to open RHN
  3. press CMD + K shortcut

Expected: the search route modal shouldn't open while RHN is opened
Current: the search route modal is opened above RHN

20250410012313638.mp4

and 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

@dubielzyk-expensify
Copy link
Contributor

Expected: the search route modal shouldn't open while RHN is opened
Current: the search route modal is opened above RHN

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

@shawnborton
Copy link
Contributor

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.

@dannymcclain
Copy link
Contributor

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:

My take is that I'd rather show it and try to figure out the visual part in a separate issue.

@ahmedGaber93
Copy link
Contributor

Thanks for confirmation.

@ahmedGaber93
Copy link
Contributor

@nkdengineer could you please roll back your alternative solution, we don't need the last change as we will keep the current behavior.

@nkdengineer
Copy link
Contributor

@ahmedGaber93 Updatded

@ahmedGaber93
Copy link
Contributor

@nkdengineer's alternative solution LGTM!

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 11, 2025

📣 @nkdengineer 🎉 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 melvin-bot bot added Overdue Reviewing Has a PR in review Weekly KSv2 and removed Overdue Daily KSv2 labels Apr 14, 2025
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 18, 2025
@melvin-bot melvin-bot bot changed the title [$250] Web - Search - Search is not opened on report page with CMD(ctrl) + K [Due for payment 2025-04-25] [$250] Web - Search - Search is not opened on report page with CMD(ctrl) + K Apr 18, 2025
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 18, 2025
Copy link

melvin-bot bot commented Apr 18, 2025

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

Copy link

melvin-bot bot commented Apr 18, 2025

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:

Copy link

melvin-bot bot commented Apr 18, 2025

@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]

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 25, 2025
@isabelastisser
Copy link
Contributor

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] 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:

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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:

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template
  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

Test:

Do we agree 👍 or 👎

@isabelastisser
Copy link
Contributor

@ahmedGaber93 please complete the checklist above. Thanks!

@isabelastisser
Copy link
Contributor

Payment summary:

@ahmedGaber93 requires payment through NewDot Manual Requests $250 C+ review PENDING
@nkdengineer requires payment automatic offer (Contributor) $250 proposal

@ahmedGaber93
Copy link
Contributor

ahmedGaber93 commented Apr 26, 2025

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other: Applause Internal Team
  • [Contributor] 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: https://github.com/Expensify/App/pull/53198/files#r2061270359

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Test:

  1. Login to an account that has a report with multiple expenses
  2. Go to Reports > Expense reports
  3. Press CMD(ctrl) + K
  4. Verify that search input on the header is opened and focused
  5. Go to Reports > Expense reports > report with multiple expenses
  6. Press CMD(ctrl) + K
  7. Verify that search modal is opened on the top right of the screen

Do we agree 👍 or 👎

@ahmedGaber93
Copy link
Contributor

@isabelastisser Checklist is completed!

@melvin-bot melvin-bot bot added the Overdue label Apr 28, 2025
@inimaga inimaga added the Reviewing Has a PR in review label Apr 28, 2025
@melvin-bot melvin-bot bot removed the Overdue label Apr 28, 2025
@isabelastisser
Copy link
Contributor

All set!

#59643 (comment)

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
Status: Done
Development

No branches or pull requests

9 participants