Skip to content

[W4Checklist] [Due for payment 2025-05-20] [$250] Cmd+K Navigation Fails to Open Chat After Desktop Update #60709

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
3 of 18 tasks
m-natarajan opened this issue Apr 23, 2025 · 34 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 Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause

Comments

@m-natarajan
Copy link

m-natarajan commented Apr 23, 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:
Reproducible in staging?: Needs Reproduction
Reproducible in production?: Needs Reproduction
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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
Expensify/Expensify Issue URL:
Issue reported by: @mallenexpensify
Slack conversation (hyperlinked to channel name): #Expensify Bugs

Action Performed:

  1. Update Desktop (possibly just close then reopen too)
  2. Type cmd+k
  3. Start typing a name of a contact
  4. Press return key once the contact is highlighted

Expected Result:

Opens highlighted/selected chat with person

Actual Result:

Nothing happens, have to mouse to name and click on trackpad to open

Workaround:

Unknown

Platforms:

Select the officially supported platforms where the issue was reproduced:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • Windows: Chrome
  • MacOS: Chrome / Safari
  • MacOS: Desktop
Platforms Tested: On which of our officially supported platforms was this issue tested:
  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • Windows: Chrome
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
2025-04-23_10-02-15.mp4

logs-2025-04-23 08_02_40.009.txt

Profile_trace_for_9.1.31-2.cpuprofile

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021915316750024963357
  • Upwork Job ID: 1915316750024963357
  • Last Price Increase: 2025-04-24
  • Automatic offers:
    • thelullabyy | Contributor | 107113235
Issue OwnerCurrent Issue Owner: @strepanier03
@m-natarajan m-natarajan added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause labels Apr 23, 2025
Copy link

melvin-bot bot commented Apr 23, 2025

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

@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@shubham1206agra
Copy link
Contributor

shubham1206agra commented Apr 23, 2025

Screen.Recording.2025-04-23.at.4.04.44.PM.mov

Able to repro this. This happens only once when I open the App.

@linhvovan29546
Copy link
Contributor

linhvovan29546 commented Apr 23, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-04-23 12:25:46 UTC.

Proposal

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

Cmd+K Navigation Fails to Open Chat After Desktop Update

What is the root cause of that problem?

In the #56948 We updated SearchAutocompleteList, we have disableKeyboardShortcuts={!shouldSubscribeToArrowKeyEvents}. This disables the Enter shortcut when shouldSubscribeToArrowKeyEvents is either false or undefined. That flag default is undefined. And, in SearchRouter, we render the SearchAutocompleteList without that flag, which leading to the disableKeyboardShortcuts is true, then nothing happen when press enter

shouldSubscribeToArrowKeyEvents={shouldSubscribeToArrowKeyEvents}
disableKeyboardShortcuts={!shouldSubscribeToArrowKeyEvents}

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {
captureOnInputs: true,
shouldBubble: !flattenedSections.allOptions.at(focusedIndex) || focusedIndex === -1,
shouldStopPropagation,
shouldPreventDefault,
isActive: !disableKeyboardShortcuts && !disableEnterShortcut && isFocused && focusedIndex >= 0,
});

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

We have two options:

Option 1:
Set shouldSubscribeToArrowKeyEvents = true by default in SearchAutocompleteList, as the issue in that PR seems to an edge case.

function SearchAutocompleteList(
{
autocompleteQueryValue,
searchQueryItem,
getAdditionalSections,
onListItemPress,
setTextQuery,
updateAutocompleteSubstitutions,
shouldSubscribeToArrowKeyEvents,

shouldSubscribeToArrowKeyEvents = true

Option 2:
Explicitly pass shouldSubscribeToArrowKeyEvents={true} from SearchRouter.

<SearchAutocompleteList
autocompleteQueryValue={autocompleteQueryValue || textInputValue}
searchQueryItem={searchQueryItem}

                    <SearchAutocompleteList
                        shouldSubscribeToArrowKeyEvents

We can check other places, like this one, to see if a similar issue exists.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

I think automated tests are not necessary in this case since we're making shouldSubscribeToArrowKeyEvents default to true. If a value of false is passed explicitly, it would be intentional.

What alternative solutions did you explore? (Optional)

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.

@ishakakkad
Copy link
Contributor

ishakakkad commented Apr 23, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-04-23 15:58:56 UTC.

Proposal

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

Item does not select using keyboard navigation, when first time open the search dialog on chat page.

What is the root cause of that problem?

When the search dialog is opened for the first time, the search input loses focus when arrow keys are pressed. As a result, pressing the Enter key does not trigger item selection. However, this issue does not occur when the dialog is opened subsequently—the search input retains focus during arrow key navigation, and Enter works as expected. This issue is occurred because of this condition.

On the first render, areOptionsInitialized is false. It only becomes true after a component re-render triggered by options loading. During this initial phase, pressing arrow keys causes the search input to lose focus, leading to the unexpected behavior.

On subsequent openings of the search dialog, the options have already been initialized, so areOptionsInitialized remains true from the start. This allows arrow key navigation without affecting the input focus, and Enter correctly selects the highlighted item.

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

Solution 1

To fix this issue we need to add areOptionsInitialized check before loading search input as well. We can make following changes here

const {areOptionsInitialized} = useOptionsList(); 
 {isRecentSearchesDataLoaded && areOptionsInitialized && (
    <>
        <SearchInputSelectionWrapper

Note: Also, we need to check other places where we used SearchAutocompleteList and make the same changes if needed.

Solution 2

We can add following code inside SearchRouter

useLayoutEffect(() => {
if (textInputRef.current && textInputRef.current.isFocused()) {
        setTimeout(() => {
            textInputRef?.current?.blur();
            textInputRef?.current?.focus();
        }, 0);
    }
}, [textInputRef.current]);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

UI bug so NA

What alternative solutions did you explore? (Optional)

Initialize options when the provider is mounted here (not ideal solution)

useEffect(() => {
        if (!isLoadingApp && !areOptionsInitialized.current && personalDetails && reports) {
            const optionLists = createOptionList(personalDetails, reports);
            setOptions({
                reports: optionLists.reports,
                personalDetails: optionLists.personalDetails,
            });
            areOptionsInitialized.current = true;
        }
    }, [isLoadingApp, personalDetails, reports]);

@thelullabyy
Copy link
Contributor

Proposal

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

  • When a user opens the search route for the first time and types a query, pressing "Enter" does nothing. The expected behavior is that the top search result is selected, but instead, users must manually click with the mouse or trackpad to proceed.

What is the root cause of that problem?

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

  <SearchAutocompleteList ... textInputRef={textInputRef} />;
  onLayout={() => {
      ...
      ref.current?.updateExternalTextInputFocus?.(textInputRef.current.isFocused())
  }}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  • NA

What alternative solutions did you explore? (Optional)

  • Introduced a potential fallback skeleton component SearchInputSelectionWrapperSkeleton. Then conditionally render either the skeleton or the actual SearchInputSelectionWrapper based on areOptionsInitialized:
    <SearchInputSelectionWrapper
  areOptionsInitialized ? <SearchInputSelectionWrapper /> : <SearchInputSelectionWrapperSkeleton />;
  • SearchInputSelectionWrapperSkeleton can be discussed by design team.

@mallenexpensify mallenexpensify added the External Added to denote the issue can be worked on by a contributor label Apr 24, 2025
@melvin-bot melvin-bot bot changed the title Cmd+K Navigation Fails to Open Chat After Desktop Update [$250] Cmd+K Navigation Fails to Open Chat After Desktop Update Apr 24, 2025
Copy link

melvin-bot bot commented Apr 24, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 24, 2025
Copy link

melvin-bot bot commented Apr 24, 2025

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

@mallenexpensify
Copy link
Contributor

@jjcoffee 👀 on the above proposals plz.

@jjcoffee
Copy link
Contributor

jjcoffee commented Apr 24, 2025

@mallenexpensify Looks like @shubham1206agra was able to repro when the Needs Reproduction label was added, so he should be assigned I think!

@mohit6789
Copy link
Contributor

mohit6789 commented Apr 24, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-04-24 09:04:17 UTC.

Proposal

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

When users opens the search on chat page user can not able to open the item using keyboard navigation.

What is the root cause of that problem?

There are two issues.

Issue 1:

During first render areOptionsInitialized is false

areOptionsInitialized && (

Because of this during first render input focus list is not initialized, so input and selectionlist focus is not sync with each other.

onFocus={() => {
onFocus?.();
autocompleteListRef?.current?.updateExternalTextInputFocus(true);
focusedSharedValue.set(true);
}}

Issue 2:

When we use tab and press enter it can not open the selected item because we haven't passed shouldSubscribeToArrowKeyEvents in SearchAutocompleteList because of that item does not select on enter (This issue is present after first render as well)

shouldSubscribeToArrowKeyEvents={shouldSubscribeToArrowKeyEvents}
disableKeyboardShortcuts={!shouldSubscribeToArrowKeyEvents}

isActive: !disableKeyboardShortcuts && !disableEnterShortcut && isFocused && focusedIndex >= 0,

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

Issue 1: To fix first issue where textbox lost focus during first load we have two approaches.

Option 1 (Preffered)

We can load text input only after areOptionsInitialized we can add check here or here. We don't need to add SearchInputSelectionWrapperSkeleton since areOptionsInitialized becomes true on second render which happens very quickly (fraction of a second). Adding a skeleton would lead to undesirable flickering when opening the search dialog for the first time which may leads to regression.

Still, if we think we should show some loader then we can add condition here and show loader for entire dialog.

Options 2

  • Pass isInputFocused to the SearchAutocompleteList component rather than passing textInputRef. If we pass textInputRef then it makes application difficult to debug.

<SearchAutocompleteList
autocompleteQueryValue={autocompleteQueryValue || textInputValue}
searchQueryItem={searchQueryItem}
getAdditionalSections={getAdditionalSections}
onListItemPress={onListItemPress}
setTextQuery={setTextAndUpdateSelection}
updateAutocompleteSubstitutions={updateAutocompleteSubstitutions}
onHighlightFirstItem={() => listRef.current?.updateAndScrollToFocusedIndex(1)}
ref={listRef}
/>

 <SearchAutocompleteList
   isInputFocused={textInputRef?.current?.isFocused()}
    ...
/>
onLayout={() => {
    setPerformanceTimersEnd();
    setIsInitialRender(false);
     if (isInputFocused !== undefined && ref && 'current' in ref) {
        ref.current?.updateExternalTextInputFocus(isInputFocused);
    }
}}

Issue 1: To Fix second issue where item does not select when user use TAB we need to pass shouldSubscribeToArrowKeyEvents in SearchAutocompleteList

<SearchAutocompleteList
      autocompleteQueryValue={autocompleteQueryValue || textInputValue}
      searchQueryItem={searchQueryItem}
      getAdditionalSections={getAdditionalSections}
      onListItemPress={onListItemPress}
      setTextQuery={setTextAndUpdateSelection}
      updateAutocompleteSubstitutions={updateAutocompleteSubstitutions}
      onHighlightFirstItem={() => listRef.current?.updateAndScrollToFocusedIndex(1)}
      ref={listRef}
      shouldSubscribeToArrowKeyEvents={true}
  />

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None because of ui bug

What alternative solutions did you explore? (Optional)

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.

Copy link

melvin-bot bot commented Apr 29, 2025

@jjcoffee Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2025
@jjcoffee
Copy link
Contributor

jjcoffee commented Apr 29, 2025

@shubham1206agra Do you want to take this (based on you being able to repro)? Happy to review the proposals if not!

@melvin-bot melvin-bot bot removed the Overdue label Apr 29, 2025
@shubham1206agra
Copy link
Contributor

@jjcoffee I can take over this issue.

@shubham1206agra
Copy link
Contributor

Thanks everyone for their proposals.

@thelullabyy's proposal looks good to me.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 29, 2025

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

@linhvovan29546
Copy link
Contributor

linhvovan29546 commented Apr 29, 2025

@shubham1206agra Not sure if I understand correctly, but it seems we have two issues with the 'Enter' key here. The first one, as I mentioned, is that the 'Enter' key is disabled (the steps are the same as in the original post). The second issue occurs with other proposals as well — if we press 'Tab' to focus on another item, then pressing 'Enter' still doesn’t work

Screen.Recording.2025-04-29.at.20.15.55.mov

@mallenexpensify
Copy link
Contributor

@thelullabyy can you please try to prioritize the PR? (not above any HIGH or CRITICAL issues though, this one is just annoying and there's an easy workaround)

@thelullabyy
Copy link
Contributor

@mallenexpensify Sure, the PR will be ready today

@jjcoffee
Copy link
Contributor

jjcoffee commented May 2, 2025

Can @shubham1206agra be assigned here please @strepanier03. Thanks!

@mallenexpensify
Copy link
Contributor

@shubham1206agra are we waiting on you to review the PR?

Assigned you to this issue and removed @jjcoffee

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 13, 2025
@melvin-bot melvin-bot bot changed the title [$250] Cmd+K Navigation Fails to Open Chat After Desktop Update [Due for payment 2025-05-20] [$250] Cmd+K Navigation Fails to Open Chat After Desktop Update May 13, 2025
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 13, 2025
Copy link

melvin-bot bot commented May 13, 2025

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

Copy link

melvin-bot bot commented May 13, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.44-8 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-05-20. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented May 13, 2025

@shubham1206agra @strepanier03 @shubham1206agra 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 May 20, 2025
@strepanier03
Copy link
Contributor

strepanier03 commented May 20, 2025

Payment summary

@strepanier03 strepanier03 changed the title [Due for payment 2025-05-20] [$250] Cmd+K Navigation Fails to Open Chat After Desktop Update [W4Checklist] [Due for payment 2025-05-20] [$250] Cmd+K Navigation Fails to Open Chat After Desktop Update May 20, 2025
@shubham1206agra
Copy link
Contributor

shubham1206agra commented May 20, 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:
  • [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/59709/files#r2098471451

  • [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: Not Required

  • [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. Open app on desktop or web.
  2. Type cmd+k.
  3. Start typing a name of a contact.
  4. Press return key once the contact is highlighted.
  5. Verify that pressing opens highlighted chat with person.

Do we agree 👍 or 👎

@strepanier03
Copy link
Contributor

@shubham1206agra - I've made the reg test GH and removed the hold from the payment summary, feel free to request and I'll close this out.

Thanks all!

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 Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause
Projects
Status: Done
Development

No branches or pull requests