Provide autocomplete suggestions in SearchRouterList #51237

SzymczakJ · 2024-10-22T11:14:26Z

Details

This PR adds autocomplete in SearchRouter for simple keys like categories, tags, etc.. Searching with 'in:' keyword is not yet fully supported(reportIDs instead of full names will be visible in text input)

Fixed Issues

$ #50942
$ #50947
$ #50957
$ #50952
$ #50953
$ #50954
$ #50955
$ #50956
$ #50948
$ #50945
PROPOSAL:

Tests

Open SearchRouter and test these autocomplete keys: category, tag, taxRate, type, expenseType, card, from, to, status, and currency.
Verify that when category, tag or currency keys are working the way this comment specified it.
Verify that after clearing input or single autocomplete key the autocomplete suggestions are cleared.
Verify that after clicking on autocomplete suggestion it is added correctly to text input.

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android.mov

Android: mWeb Chrome

androidweb.mov

iOS: Native

ios.mov

iOS: mWeb Safari

iosweb.mov

MacOS: Chrome / Safari

web.mov

MacOS: Desktop

desktop.mov

SzymczakJ · 2024-10-22T15:09:25Z

Should we somehow limit the list of autosuggestions @luacmartins? This is POC implementation of autosuggestions for some of filters(the ones linked in description)

poc.mov

I don't see much designs connected to autocomplete.

luacmartins · 2024-10-22T15:30:00Z

@SzymczakJ I think we should only offer suggestions once the user typed at least one character after the filter key, e.g. currency:A. tagging @trjExpensify @JmillsExpensify for your thoughts as well.

JmillsExpensify · 2024-10-22T18:32:52Z

Agree with Carlos that we'd suggest on the first character after the filter key.

luacmartins · 2024-10-22T20:18:56Z

@SzymczakJ we discussed internally and the behavior we want for this is:

category: would show a list of 10 "recently used" across all policies, from the RecentlyUsedTags, etc Onyx key. This can be an empty list.
category:a would filter results from all policyCategories for categories starting with a and show those suggestions.

Does that make sense?

Kicu

On a quick glance the logic and data flow looks good 👌

I hope that in future we can somehow split the logic so that SearchRouter does not become a "god component"

src/libs/SearchAutocompleteUtils.ts

Kicu · 2024-10-23T12:01:47Z

src/libs/SearchAutocompleteUtils.ts

+    const singlePolicyTagsList: PolicyTagLists | undefined = allPoliciesTagsLists?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`];
+    if (!singlePolicyTagsList) {
+        const uniqueTagNames = new Set<string>();
+        const tagListsUnpacked = Object.values(allPoliciesTagsLists ?? {}).filter((item) => !!item) as PolicyTagLists[];


is possible to use default argument instead, and do

getAutoCompleteTagsList(allPoliciesTagsLists: OnyxCollection<PolicyTagLists> = {}, policyID?: string)

?

I believe our hands are tied by OnyxCollection which can be undefined.

src/libs/SearchAutocompleteUtils.ts

Kicu · 2024-10-23T12:07:01Z

src/components/Search/SearchRouter/SearchRouter.tsx

+    const [cardList = {}] = useOnyx(ONYXKEYS.CARD_LIST);
+    const cardsAutocompleteList = Object.values(cardList ?? {}).map((card) => card.bank);
+    const personalDetails = usePersonalDetails();
+    const participantsAutocompleteList = Object.values(personalDetails)


Good job, but still:

😆 but seriously NAB and if we need all this data then we need it

Yea, I'm not sure there's a way around it. Maybe we can create a custom hook, useAutocomplete so the logic lives outside the component?

luacmartins

I know this is still WIP so maybe you might be aware of these issues already, but I wanted to point them out anyways so we can get ahead and make sure they don't make to the final version of the PR:

I think we should limit the number of suggestions to maybe 10 sorted alphabetically. You can see on the list that if I type from, a full list of all personalDetails comes up, which seems a bit useless.
Cycling through the options should update the query string, e.g. when I press the arrow keys to select a different option, the query in the router should update to include the currently active suggestion.
Selecting an autocomplete suggestion runs the query, but it drops the filter key. Note in the video how I selected from:t3@t.com but the final search dropped the from: part

Screen.Recording.2024-10-23.at.9.49.14.AM.mov

Kicu

very small stuff, feel free to open PR when you're ready.

src/components/Search/SearchRouter/SearchRouterList.tsx

src/components/Search/SearchRouter/SearchRouter.tsx

luacmartins

Nice, this is looking really good. Just gonna report a few bugs that I found

luacmartins · 2024-10-25T19:02:47Z

BUG?: Pressing tab cycles through the options as it should, but it also removes the focus from the text input so the user has to click back into the input. This doesn't happen with the arrow navigation so users can keep typing into the input. We should make this behavior consistent.

Tab:

Screen.Recording.2024-10-25.at.12.52.50.PM.mov

Arrows

Screen.Recording.2024-10-25.at.12.54.18.PM.mov

luacmartins · 2024-10-25T19:04:25Z

BUG: We should make suggestions case insensitive. For example, when searching for a currency, it correctly returns the results if I search in upper case, but nothing when searching in lower case

Screen.Recording.2024-10-25.at.12.56.50.PM.mov

luacmartins · 2024-10-25T19:04:59Z

BUG: Searching for categories with space results in no results and incorrect final syntax.

Screen.Recording.2024-10-25.at.1.00.21.PM.mov

SzymczakJ · 2024-10-28T11:23:02Z

BUG?: Pressing tab cycles through the options as it should, but it also removes the focus from the text input

This was also the tab behaviour of ChatFinderPage. I tried to make it not defocus TextInput but after investigating it for some time, I think we would need to modify https://github.com/Expensify/react-native-key-command library, which makes this "bug" harder to fix.
I think this is a good candidate for Search 2.5 polish, or maybe, let's decide that's not a bug 😄 ? As I said before ChatFinderPage had identical behaviour. WDYT @luacmartins?

SzymczakJ · 2024-10-28T11:23:31Z

@ikevin127 kind bump 🙇

luacmartins · 2024-10-28T15:54:58Z

This was also the tab behaviour of ChatFinderPage. I tried to make it not defocus TextInput but after investigating it for some time, I think we would need to modify https://github.com/Expensify/react-native-key-command library, which makes this "bug" harder to fix.

Thanks for looking into it. I think it's ok to do this as a polish or wait until we get feedback that we should implement this.

ikevin127 · 2024-10-28T18:19:12Z

I'll start reviewing in ~ 1 hour.

ikevin127 · 2024-10-28T19:29:22Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

android.webm

Android: mWeb Chrome

android-mweb.webm

iOS: Native

ios.mp4

iOS: mWeb Safari

ios-mweb.mp4

MacOS: Chrome / Safari

web.mov

MacOS: Desktop

desktop.mov

luacmartins

LGTM. Left a few small comments. We also have conflicts now and product is still testing this branch to make sure things are what we expect.

luacmartins · 2024-10-28T21:38:09Z

src/components/Search/SearchRouter/SearchRouter.tsx

+                    );
+                    return;
+                }
+                default: {


NAB we can DRY the code above since they all run the same logic, just on different lists

I know this code looks easily "DRYable", but with Mateusz PR these switch cases will become much more distinct. Let's not add conflicts to his branch, as he said in #51237 (comment) 😄. After merging these two PRs we could put up a small clean-up PR.

src/languages/en.ts

src/libs/SearchAutocompleteUtils.ts

luacmartins · 2024-10-28T22:03:32Z

src/libs/SearchAutocompleteUtils.ts

+        return Array.from(uniqueCategoryNames);
+    }
+    return Object.values(singlePolicyRecentCategories ?? {}).map((category) => category);
+}


NAB maybe we could DRY the logic in some of these functions too since the logic is 95% identical

Same as in #51237 (comment)

luacmartins

Let's address the variable reaming/typos above

ikevin127 · 2024-10-28T22:06:24Z

⚠️ Found two issues while reviewing:

Multiple options for 1 autocomplete key results in infinite loading

Note

Explanation

When selecting 2 or more options and performing search we're stuck with infinite loading for the following keys: type, taxRate and status.

Note: I want to mention that since I don't have cards for testing I wasn't able to test the card key autocomplete but I'm assuming it works as expected 🤞

Below video demonstrates the behaviour for the type key.

multipleTypetaxRate-infinite-loading.mov

[UX] Selecting 1 autocomplete option adds [space] then when comma is added after space, no autocomplete options are shown

Note

Explanation

The issue here is that when selecting 1 autocomplete option, [space] is added in the input after the option which if we type , (comma) after the space, other autocomplete options don't show up. The only way options continue to show up is if we delete the space that is added automatically, then type , (comma) right after the first selected option.

Important: Same thing happens when trying to chain multiple keys, not only key options - if the space is not removed first, the options for the second key won't show up:

Below video demonstrates the behaviour.

all-spacing-comaSeparator.mov

cc @SzymczakJ @luacmartins

I'll move forward with the reviewer checklist in the meantime, and will approve once the above issues are addressed and conflicts resolved.

Kicu · 2024-10-29T08:31:37Z

@ikevin127 @luacmartins @SzymczakJ
In order to speed implementing things up, I'm already branched from this PR and working on the next steps of autocomplete. Mainly the accountID/reportID <---> user login/chatname substitution, so that the query the user sees is always correct.

Because of that I would like to suggest any NAB's and other improvements are not done here, but I will instead do them in my PR 😁 it will greatly help me.
Let's fix only critical and problematic bugs here.

SzymczakJ · 2024-10-29T12:42:26Z

Multiple options for 1 autocomplete key results in infinite loading

It results in infinite loading, because it doesn't make sense from UI point of view(we cannot be on an Expense tab and on Chat tab on the same moment). On the other hand I think that we shouldn't limit the user, even though his query may produce something that doesn't make sense. Maybe we could provide some modal on SearchResultsPage that tells user that 'This query doesn't make sense'. WDYT @luacmartins

[UX] Selecting 1 autocomplete option adds [space] then when comma is added after space, no autocomplete options are shown

No autocomplete was added for input "category:X ," because of the limitations of our parser. Here is my solution for this: when user adds "," to add another autocomplete then the spaces before "," are trimmed so our parser can parse it properly. Please check it out @luacmartins @ikevin127 and tell me WDYT

proposal.mov

luacmartins · 2024-10-29T17:55:09Z

It results in infinite loading, because it doesn't make sense from UI point of view(we cannot be on an Expense tab and on Chat tab on the same moment). On the other hand I think that we shouldn't limit the user, even though his query may produce something that doesn't make sense. Maybe we could provide some modal on SearchResultsPage that tells user that 'This query doesn't make sense'. WDYT @luacmartins

Yea, I agree that this is an issue, but it also happens on main today because a user can type that. So NAB for this PR. I think ultimately, we need to improve the parser to validate some of these keys, like type, status, sortBy, sortOrder, etc so we handle this more gracefully. NAB for now though

No autocomplete was added for input "category:X ," because of the limitations of our parser. Here is my solution for this: when user adds "," to add another autocomplete then the spaces before "," are trimmed so our parser can parse it properly. Please check it out @luacmartins @ikevin127 and tell me WDYT

I like it! Thanks for fixing that!

ikevin127

Looks great, thanks to everybody for addressing and the issues!

luacmartins · 2024-10-29T18:22:25Z

NAB I think we can get smarter with the suggested statuses and only suggest statuses that apply to the current type

luacmartins

LGTM and tests well. Just waiting on final approval from the product team!

github-actions · 2024-10-29T18:28:58Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
❌ FAILED ❌	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/51237/index.html
The QR code can't be generated, because the android build failed
Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/51237/NewExpensify.dmg	https://51237.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

OSBotify · 2024-10-29T18:58:31Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2024-10-31T11:41:03Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.56-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅
🤖🔄 android HybridApp 🤖🔄	success ✅
🍎🔄 iOS HybridApp 🍎🔄	success ✅

SzymczakJ added 6 commits October 14, 2024 11:12

autocomplete POC

433ec49

Merge branch 'main' into @szymczak/autocomplete-search-router

6578dde

Merge branch 'main' into @szymczak/autocomplete-search-router

9fd9ead

remove previously added parser

2b63c0e

remove autocomplete list behaviour

3f748b5

remove unused CONST values

c58aa27

SzymczakJ changed the title ~~Autocomplete search router draft~~ Provide autocomplete suggestions in SearchRouterList Oct 22, 2024

luacmartins assigned SzymczakJ Oct 22, 2024

luacmartins self-requested a review October 22, 2024 16:00

SzymczakJ added 2 commits October 23, 2024 13:16

Merge branch 'main' into @szymczak/autocomplete-search-router

a354bdc

move autocomplete functions to autocompleteUtils

157c3e1

Kicu reviewed Oct 23, 2024

View reviewed changes

SzymczakJ added 3 commits October 23, 2024 15:16

autosuggest recent categories and currencies when textinput is empty

ffd2995

fix pr comments

fb4359b

autosuggest tags

00d61c6

luacmartins reviewed Oct 23, 2024

View reviewed changes

SzymczakJ mentioned this pull request Oct 24, 2024

[$250] iOS - Search - Search page cannot be dismissed by swiping to the right #51191

Open

8 tasks

SzymczakJ added 4 commits October 24, 2024 13:47

put autocomplete suggestions into input

dc8f993

ignore already autocompleted values

8cf4690

autocomplete "in:" key

3354af2

fix SearchRouterInput bug

3ec1802

Kicu approved these changes Oct 24, 2024

View reviewed changes

src/components/Search/SearchRouter/SearchRouterList.tsx Outdated Show resolved Hide resolved

src/components/Search/SearchRouter/SearchRouter.tsx Show resolved Hide resolved

src/components/Search/SearchRouter/SearchRouter.tsx Outdated Show resolved Hide resolved

SzymczakJ added 4 commits October 24, 2024 16:51

fix PR comments

94c259f

add arrowSelectionAutocomplete logic

4002093

sort autocomplete values

0dc345c

clean up code

11ee9da

luacmartins requested changes Oct 25, 2024

View reviewed changes

SzymczakJ added 3 commits October 28, 2024 09:59

Merge branch 'main' into @szymczak/autocomplete-search-router

d834b70

make autocomplete suggestions insensitive

c1e1bc5

sanitize search query in search router

d7b51bb

luacmartins approved these changes Oct 28, 2024

View reviewed changes

luacmartins requested changes Oct 28, 2024

View reviewed changes

fix PR comments

cf4eb14

SzymczakJ added 2 commits October 29, 2024 09:32

Merge branch 'main' into @szymczak/autocomplete-search-router

73ecc8f

trim space to allow for autocomplete sugggestion after comma

f66dfee

ikevin127 approved these changes Oct 29, 2024

View reviewed changes

melvin-bot bot requested a review from luacmartins October 29, 2024 18:01

luacmartins approved these changes Oct 29, 2024

View reviewed changes

luacmartins merged commit 94fd9b5 into Expensify:main Oct 29, 2024
17 checks passed

github-actions bot mentioned this pull request Oct 31, 2024

Deploy Checklist: New Expensify 2024-10-31 #51778

Open

Provide autocomplete suggestions in SearchRouterList #51237

Provide autocomplete suggestions in SearchRouterList #51237

Conversation

SzymczakJ commented Oct 22, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

SzymczakJ commented Oct 22, 2024

luacmartins commented Oct 22, 2024

JmillsExpensify commented Oct 22, 2024

luacmartins commented Oct 22, 2024

Kicu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

Kicu left a comment

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

luacmartins commented Oct 25, 2024 • edited Loading

luacmartins commented Oct 25, 2024

luacmartins commented Oct 25, 2024

SzymczakJ commented Oct 28, 2024

SzymczakJ commented Oct 28, 2024

luacmartins commented Oct 28, 2024 • edited Loading

ikevin127 commented Oct 28, 2024

ikevin127 commented Oct 28, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

luacmartins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins left a comment • edited Loading

Choose a reason for hiding this comment

ikevin127 commented Oct 28, 2024 • edited Loading

⚠️ Found two issues while reviewing:

Explanation

Explanation

Kicu commented Oct 29, 2024

SzymczakJ commented Oct 29, 2024 • edited Loading

luacmartins commented Oct 29, 2024

ikevin127 left a comment

Choose a reason for hiding this comment

luacmartins commented Oct 29, 2024

luacmartins left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 29, 2024

OSBotify commented Oct 29, 2024

github-actions bot commented Oct 31, 2024

SzymczakJ commented Oct 22, 2024 •

edited

Loading

luacmartins commented Oct 25, 2024 •

edited

Loading

luacmartins commented Oct 28, 2024 •

edited

Loading

ikevin127 commented Oct 28, 2024 •

edited

Loading

luacmartins left a comment •

edited

Loading

ikevin127 commented Oct 28, 2024 •

edited

Loading

SzymczakJ commented Oct 29, 2024 •

edited

Loading