[Search v2.4] - Same search query appears twice in Recent searches

[lanitochka17]

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: 9.0.50-3
Reproducible in staging?: Y
Reproducible in production?: N
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: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

• User has not searched using filters previously

1. Go to staging.new.expensify.com
2. Go to Search
3. Click Filters
4. Apply any filter and click View results
5. Go to Inbox
6. Click on the search icon on the report header
7. Click on the search query under Recent searches
8. Go back to Inbox
9. Click on the search icon on the report header

Expected Result:

The search query made in Step 4 will only appear once after clicking on the same search query again in Step 7

Actual Result:

The search query made in Step 4 appears twice after clicking on the same search query again in Step 7

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6637920_1729183854749.bandicam_2024-10-18_00-47-35-359.mp4

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @dangrous (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

💬 A slack conversation has been started in #expensify-open-source

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

We think that this bug might be related to #wave-control

[luacmartins]

Interesting, the Onyx data is different from what's being displayed

[jaydamani]

Proposal

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

Same search query appears twice in Recent searches

What is the root cause of that problem?

When we search again, new entry is added to nvp_RecentSearches

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

Add logic to remove duplicate queries and keep only most recent one in below line.

App/src/components/Search/SearchRouter/SearchRouter.tsx

Line 55 in 2aae716

return Object.values(recentSearches ?? {}).sort((a, b) => b.timestamp.localeCompare(a.timestamp));

[jaydamani]

Interesting, the Onyx data is different from what's being displayed

FYI, It happens because we don't use sort related properties when making the search header in below code. So, we essentially ignore sort by, sort order and policy id from the query.

App/src/libs/SearchUtils.ts

Lines 812 to 815 in 91c6e0c

	const {type, status} = queryJSON;
	const filters = queryJSON.flatFilters ?? {};
	let title = `type:${type} status:${status}`;

[dangrous]

@luacmartins do you want to take this one since you know the search stuff well?

[SzymczakJ]

I'd like to work on it, since I was implementing Search 2.4.

[luacmartins]

@SzymczakJ all yours. I think we can demote this to NAB since it doesn't really break any functionality.

[SzymczakJ]

This bug is on FE

Interesting, the Onyx data is different from what's being displayed

but after investigating this I think that it should be also fixed on BE. Recent Searches are generated on BE after searching for query. In case when we are searching for the same query twice, then it should be renewed in history and not duplicated. WDYT @luacmartins

[luacmartins]

@SzymczakJ that is the current logic in the backend. As you can see the Onyx data is accurate, not sure why the Onyx data and the list are out of sync

[luacmartins]

Oh I see that it shows the query because we have instances of the same query saved with and without sortBy:date sortOrder:desc. We must be adding that in the frontend when sending it back to the server so we end up saving it twice instead of updating the query.

[rayane-djouah]

@luacmartins I agree, I commented here: #50921 (comment). Adding that on the frontend when sending it back to the server will fix the bug for this case. However, it will not resolve the issue in cases of the same query with different sortBy or sortOrder. In this comment, I suggested filtering the recent searches based on a secondary hash.

[luacmartins]

Hmm in the case of different sortBy or sortOrder (that's not the default), wouldn't we want to display that to the user, which would make it a different recent search? After all, they'd have to explicitly set those to values other than the default.

[rayane-djouah]

@luacmartins @SzymczakJ What do you think about this plan:

1. Add sortBy and sortOrder (that's not the default) to buildFilterFormValuesFromQuery similar to type, status, and policyID:

App/src/libs/SearchUtils.ts

Lines 738 to 746 in 793dcaf

	const [typeKey = '', typeValue] = Object.entries(CONST.SEARCH.DATA_TYPES).find(([, value]) => value === queryJSON.type) ?? [];
	filtersForm[FILTER_KEYS.TYPE] = typeValue ? queryJSON.type : CONST.SEARCH.DATA_TYPES.EXPENSE;
	const [statusKey] = Object.entries(CONST.SEARCH.STATUS).find(([, value]) => Object.values(value).includes(queryJSON.status)) ?? [];
	filtersForm[FILTER_KEYS.STATUS] = typeKey === statusKey ? queryJSON.status : CONST.SEARCH.STATUS.EXPENSE.ALL;
	if (queryJSON.policyID) {
	filtersForm[FILTER_KEYS.POLICY_ID] = queryJSON.policyID;
	}

2. Change buildQueryStringFromFilterFormValues to use the existing sortBy and sortOrder (that's not the default) (if they exist) instead of including the default values like we're doing here.
3. Create a function that uses getSearchHeaderTitle and include sortBy and sortOrder (that's not the default) and the policyID (if it exists) and use this function instead of getSearchHeaderTitle when displaying recent searches and saved searches here:

App/src/pages/Search/SearchTypeMenu.tsx

Line 118 in 41ed362

title = SearchUtils.getSearchHeaderTitle(jsonQuery, personalDetails, cardList, reports, taxRates);

and here:

App/src/components/Search/SearchRouter/SearchRouterList.tsx

Line 135 in 41ed362

text: searchQueryJSON ? SearchUtils.getSearchHeaderTitle(searchQueryJSON, personalDetails, cardList, reports, taxRates) : query,

and keep using getSearchHeaderTitle only for the search input in the header of the results page (to encourage th users to use the contextual filters to change the values of sortBy, sortOrder, and policyID instead of manually typing them in)

4. Filter sortBy, sortOrder, and policyID (contextual filters) when determining shouldShowResetFilters (to avoid displaying the "Reset filters" button when the form is empty) :

App/src/pages/Search/SearchAdvancedFiltersPage.tsx

Lines 21 to 23 in 41ed362

	const shouldShowResetFilters = Object.entries(searchAdvancedFilters)
	.filter(([key]) => CONST.SEARCH.SYNTAX_ROOT_KEYS.TYPE !== key && CONST.SEARCH.SYNTAX_ROOT_KEYS.STATUS !== key)
	.some(([, value]) => (Array.isArray(value) ? value.length !== 0 : !!value));

[luacmartins]

That sounds good to me 👍

[SzymczakJ]

After talking about this with @Kicu and @289Adam289 I'd like to propose another solution:
Fix BE so same queries with changed sortOrder or sortBy should be treated as the same query and should be just renewed in history. Additionally don't show sortBy and sortOrder in SearchRouter and saved searches Here are arguments for that solution:

1. In development changing searchOrder is technically a new search, but for user it doesn't make much difference, take a look at this video as an example:

argument.1.2.mov

Now after playing around with sortOrder and sortBy for a couple of times user gets a list of the same query just with different sortOrder etc.. His meaningful previous searches are pushed out of recent list by garbage which is only differing in searchOrder. 🥲

2. Having SearchOrder in recent searches list can lead some UI/UX problems because Search router has limited space horizontally and we won't have enough space to view even medium length queries.

This query has only one filter currency:PLN and even this one filter isn't visible 😄
3. The same goes for Saved searches:

4. And everything that is shown on mobile screen:

After fixing this on BE we could think of some follow-ups like:

• change mobile design a little, so it allows user to sort search results on mobile screen(add some button for this) without writing it manually into SearchRouter. Now it's not possible.
• change designs a little bit, so recent searches and saved searches, can be displayed reasonably, because now only really short queries can be shown fully and showing any longer query is confusing.

[luacmartins]

I'm not sure that this is a backend fix. Currently, the logic in the backend is:

1. If canned search, return
2. Get currentRecentSearches
3. If currentRecentSearches has the request hash, update its timestamp
4. Else if currentRecentSearches > 5, purge oldest and add new search keyed by request hash
5. Else add the search to currentRecentSearches keyed by request hash

So the backend logic is already updating the timestamp. The issue is that we use the hash as the key and that's changing with sortBy/order. Is there any benefit in changing the hash when sortBy/sortOrder changes? We exclude offset from the hash already, so maybe we should be doing the same for sorting

[Kicu]

Is there any benefit in changing the hash when sortBy/sortOrder changes?

A few things we'd need to check.

1. main thing is this - does the hash take any part in caching some parts of query or checking if the query is unique? I have completely misremembered that the hash is used as key in onyx, but after checking that does not seem to be the case
2. I'm pretty sure hash takes some part in deciding uniqueness of saved search, and highlighting if saved search is active
3. it's used as key in onyx for optimisticData for action saveSearch and deleteSavedSearch (something related to RBR which I don't fully understand) here: https://github.com/Expensify/App/blob/main/src/libs/actions/Search.ts#L63

So while this is one possible course of action, we'd need to verify if above cases don't break. Especially about point 1 - please remind me, we have never used hash as onyx key in front right? I imagined that? 😅

---

Now about what @SzymczakJ was trying to convey I think is this:
In case we keep hash as is right now, backend can see that the hash is different - so query is different.
However backend could additionally check if the different part of query is only sort values, and in that case don't treat it as new query; don't purge; just update the timestamp.

So basically for the user let's not save the same exact query that differs only by sorting.
However technically for us the query IS different, that's why I'm kinda reluctant to change hash generation.

In general we would like to avoid Recent Searches to look like this:

 | "sameQuery sortBy:date asc"
 | "sameQuery sortBy:date desc"
 | "sameQuery sortBy:from asc"
 | "sameQuery sortBy:from desc"
 | "sameQuery sortBy:date asc"

IMO ☝️ looks not really helpful. I'd rather see my actual older searches than the same search with changed sorts.

[SzymczakJ]

I think we have to include sortOrder and sortBy in hash because:

1. it is used as Onyx key on every search action. In previous hash function we also included it in hash because when the text is parsed the default sortOrder and sortBy are added.
2. changing sortOrder and sortBy can give us two different results because of pagination(here I explained more Search router polish  #50921 (comment))

Also, fixing this on BE would be a problem, so I think the only solution we have is to have two hashes, one for computing recentSearches(which doesn't include sortOrder and sortBy) and the other one for everything else(this hash would include sort). @rayane-djouah proposed it in #51044 (comment).
WDYT @luacmartins @rayane-djouah

[luacmartins]

Yea, I think the simplest solution would be to have two hashes. We could add recentSearchHash to the top level of the jsonQuery object and in the backend we use recentSearchHash instead of hash to key recent searches. Otherwise the alternative solutions would be:

1. Remove sort params for hash. This would lead to us losing caching based on sorting and maybe lead to other unknown issues now.
2. Having to store the full jsonQuery in the database (instead of the query input string) and doing a deep object comparison with each search.

I like just adding the recentSearchHash and using it. Let's just leave a comment explaining what that hash does.

[Kicu]

Agreed that in the current situation storing two hashes seems the most reasonable 👍

Feels to me like:

• one hash is more "data oriented" - because sorting produces different datasets, we need different keys for them
• the second hash will be more "user oriented" - so that user doesn't see (what looks like) identical queries