fix: prevent search results from being cleared on task cancellation#2253
fix: prevent search results from being cleared on task cancellation#2253tiran133 wants to merge 1 commit intoopencloud-eu:mainfrom
Conversation
Fix stale route name 'search-provider-list' to 'files-common-search' in Preview.available so preview search is correctly disabled on the search results page. Add cancellation handling in List.vue catch block to avoid resetting searchResult when vue-concurrency's restartable task throws 'cancel'. Update unit test to use correct route name.
There was a problem hiding this comment.
This is definitely an issue, thanks for bringing it up! I'm not quite happy with the solution though.
search-provider-list seems to be a stale route indeed, but I think the search preview should be available on the search result page. It doesn't only show file search results, it's also an extension point for other apps (e.g. https://github.com/opencloud-eu/web-extensions/tree/main/packages/web-app-calculator, which throws errors on the search result page with this change). Also, we want to extend its core functionality in the future to reveal more than just files.
You could try your fix after removing this condition, but it will probably break things again. IMO the real issue is that both the preview and the list use the same task instance for loading search results.
|
Agree it's not ideal. Not sure what to do about this one then. There is a test This would then be changed to this and change the the to web/packages/web-app-files/src/search/sdk/preview.ts Lines 25 to 29 in 50005f5 public get available(): boolean {
return true;
}Proposed solution: Let me know what you think, and I can push that so you can try it for yourself. packages/web-app-search/src/portals/SearchBar.vuediff --git a/packages/web-app-search/src/portals/SearchBar.vue b/packages/web-app-search/src/portals/SearchBar.vue
--- a/packages/web-app-search/src/portals/SearchBar.vue (revision 2ff99ae77ed67012f64cca3cf9bd987420f9233c)
+++ b/packages/web-app-search/src/portals/SearchBar.vue (date 1774909326974)
@@ -398,7 +398,8 @@
getSearchResultLocation,
showDrop,
isAppActive,
- getFocusableElements
+ getFocusableElements,
+ debouncedSearch,
}
},
@@ -503,6 +504,10 @@
this.currentFolderAvailable = currentFolderAvailable
}
+ if (isLocationCommonActive(this.$router, 'files-common-search')) {
+ this.debouncedSearch.cancel()
+ }
+
this.$nextTick(() => {
if (!this.availableProviders.length) {
returnpackages/web-app-files/src/search/sdk/preview.tsdiff --git a/packages/web-app-files/src/search/sdk/preview.ts b/packages/web-app-files/src/search/sdk/preview.ts
--- a/packages/web-app-files/src/search/sdk/preview.ts (revision 2ff99ae77ed67012f64cca3cf9bd987420f9233c)
+++ b/packages/web-app-files/src/search/sdk/preview.ts (date 1774907102967)
@@ -23,9 +23,6 @@
}
public get available(): boolean {
- return (
- unref(this.router.currentRoute).name !== 'files-common-search' &&
- !this.configStore.options?.embed?.enabled
- )
+ return true
}
}packages/web-app-files/tests/unit/search/sdk.spec.tsdiff --git a/packages/web-app-files/tests/unit/search/sdk.spec.ts b/packages/web-app-files/tests/unit/search/sdk.spec.ts
--- a/packages/web-app-files/tests/unit/search/sdk.spec.ts (revision 2ff99ae77ed67012f64cca3cf9bd987420f9233c)
+++ b/packages/web-app-files/tests/unit/search/sdk.spec.ts (date 1774906483526)
@@ -19,10 +19,10 @@
})
describe('SDKProvider previewSearch', () => {
- it('is not available on certain routes', () => {
+ it('is available on all routes', () => {
;[
{ route: 'foo', available: true },
- { route: 'files-common-search' },
+ { route: 'files-common-search', available: true },
{ route: 'bar', available: true }
].forEach((v) => {
const router = mock<Router>()
@@ -35,7 +35,7 @@
mock<ConfigStore>()
)
- expect(!!search.previewSearch.available).toBe(!!v.available)
+ expect(search.previewSearch.available).toBe(true)
})
})
}) |
|
Just realized that it might be even better to cancel the debounce in But neither is handling the case properly. When you do the following
That's when you mention that both searches share the same task. |
Description
Fix a race condition where search results display "No results found" even though the search request returns valid results. This happens when the user is already on the search results page and performs a new search.
Two bugs are fixed:
Stale route name in
Preview.available(web-app-files/src/search/sdk/preview.ts): ThePreview.availablegetter checks against the route name'search-provider-list', which does not exist in the codebase. The actual search results route is'files-common-search'. Because of this,Preview.availablealways returnstrue— even when the user is on the search results page — so the SearchBar's preview search is never disabled on that page.No cancellation handling in list search (
web-app-search/src/views/List.vue): Both the list search and the preview search share the samesearchTask(created viauseSearch()inextensions.ts), which uses vue-concurrency's.restartable()modifier. When the preview search fires (due to Bug 1 not disabling it on the search page), it cancels the in-flight list search. Thecatchblock inList.vuedoes not distinguish cancellation errors from real errors, so it resetssearchResultto empty and setsloadingtofalse— overwriting valid results or preventing the UI from showing them.Race condition timeline (when already on the search page):
List.vueemits'search'→searchTask.perform(term, 200)— Request A startsSearchBar.vue's$routewatcher firesparseRouteQuery(route, false)→ setsrestoreSearchFromRoute = false→termwatcher callsdebouncedSearch()(500ms debounce)debouncedSearchfires →previewSearch.search()→searchTask.perform(term, 8)— Request B starts, cancels Request A via.restartable()'cancel'→catchblock resetssearchResult = { values: [], totalResults: null },loading = false→ "No results found"searchResultrefRelated Issue
If you want me to create an Github Issue first, let me know.
Motivation and Context
When using OpenCloud and performing a search while already on the search results page causes the results to be cleared. The user sees "No results found" even though the network request eventually returns valid results. This is confusing and breaks the search experience, particularly for extensions that navigate to the built-in search page.
How Has This Been Tested?
Screenshots (if appropriate):
N/A
Types of changes
Checklist: