feat(core): reduce unifieid search reveal interval

Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>

Author: pringelmann

core/src/services/UnifiedSearchController.ts

@@ -23,7 +23,7 @@ export interface CategorySearchParams {
 	extraQueries?: object
 }
 
-export const REVEAL_INTERVAL_MS = 1500
+export const REVEAL_INTERVAL_MS = 1000
 
 /**
  * Results fetched per category per page. Sized for the detail view (which shows the
@@ -32,9 +32,10 @@ export const REVEAL_INTERVAL_MS = 1500
 export const PAGE_SIZE = 10
 
 /**
- * Whether a category has anything for the user to look at. Blocked is deliberately
- * withheld, failed carries no entries, and a loading category keeps its previous page up
- * (stale-while-revalidate) so it stays visible through a refetch.
+ * Whether a category has anything for the user to look at. Blocked is deliberately withheld
+ * and failed carries no entries. Loading counts because paging keeps the pages already
+ * fetched on screen while the next one is in flight; a new query has no entries to show, so
+ * it reads as not visible until results actually land.
  *
  * Exported so the one definition also serves the Vue-side test doubles; the controller is
  * the only place that decides category-level visibility.
@@ -74,32 +75,20 @@ export class UnifiedSearchController {
 	 */
 	async search(query: string, categories: string[], params?: Record<string, CategorySearchParams>): Promise<void> {
 		this.cancelPendingRequests()
-		// Stale-while-revalidate: keep the previous page on screen while the new search is in
-		// flight, so refining a query swaps results in place instead of flashing an empty panel.
-		// Each recurring category is reseeded with its prior entries below; dropped ones vanish.
-		const previous = this.searchStates
+		// A new query hides everything the last one produced. Carrying results over would only
+		// let them shift under the user once the real ones land, and the results are about to
+		// differ anyway. So each search is a clean slate: empty screen, then a fresh ordered
+		// reveal from priority order. Nothing is on screen, so nothing can be displaced.
 		this.searchStates = {}
-		// Prune rather than clear: survivors keep the slots they already hold, so refining a query
-		// never re-sorts rendered results back to priority order. A category the new search
-		// dropped is reseeded invisible if it ever returns, so it re-enters at the bottom. This
-		// cannot cover the window while the states below are still being reseeded one at a time;
-		// getRevealOrder() does that.
-		this.revealOrder = this.revealOrder.filter((category) => categories.includes(category))
+		this.revealOrder = []
 		this.searchGeneration++
 		const generation = this.searchGeneration
 		this.query = query
 		this.params = params || {}
 
 		this.startRevealTimer()
 
-		await Promise.allSettled(categories.map((category) => {
-			const prev = previous[category]
-			// Only entries that were actually on screen seed the stale view. A blocked or failed
-			// category's entries were fetched but never rendered, so they must not carry over
-			// (and must not let the category skip the ordered reveal).
-			const staleEntries = prev && isCategoryVisible(prev) ? prev.entries : []
-			return this.searchCategory(category, generation, categories, staleEntries)
-		}))
+		await Promise.allSettled(categories.map((category) => this.searchCategory(category, generation, categories)))
 	}
 
 	/**
@@ -165,17 +154,18 @@ export class UnifiedSearchController {
 	/**
 	 * The ids of the categories currently on screen, in display order.
 	 *
-	 * Append-only, so a category never moves up into a slot another one already occupies: a
-	 * result that arrives late renders below what the user is already reading, however high
-	 * its priority. Read this rather than the snapshot's key order, which is the priority
-	 * order and an input to blocking, not a rendering order.
+	 * Append-only within a search, so a category never moves up into a slot another one already
+	 * occupies: a result that arrives late renders below what the user is already reading,
+	 * however high its priority. A new query starts over from priority order, since it clears
+	 * the screen first and so has nothing to displace. Read this rather than the snapshot's key
+	 * order, which is the priority order and an input to blocking, not a rendering order.
 	 *
-	 * Every id is indexable in the same snapshot, so a caller can map without guarding.
+	 * Only ever names categories the current snapshot holds, so a caller can map without guarding.
 	 *
 	 * @return visible category ids, top to bottom
 	 */
 	getRevealOrder(): string[] {
-		return this.revealOrder.filter((category) => category in this.searchStates)
+		return [...this.revealOrder]
 	}
 
 	dispose(): void {
@@ -196,13 +186,10 @@ export class UnifiedSearchController {
 		category: string,
 		generation: number,
 		categories: string[],
-		staleEntries: unknown[] = [],
 	): Promise<void> {
-		// Seed with the prior page (stale-while-revalidate) so it stays visible under the
-		// spinner until the fresh page replaces it. Empty on a first search.
 		this.patchStates({ [category]: {
 			status: 'loading',
-			entries: staleEntries,
+			entries: [],
 			cursor: null,
 			hasMore: false,
 			loadMoreFailed: false,
@@ -227,12 +214,9 @@ export class UnifiedSearchController {
 
 			const { entries, cursor, isPaginated } = response.data.ocs.data
 			// Decide blocked vs loaded once, here at settle. Reconcile only promotes after this
-			// (never re-blocks), so this is the only place a category becomes blocked. A category
-			// that carried stale results skips blocking: it is already on screen, so blocking it
-			// would blink it off until its predecessors clear. Ordered reveal is only for the
-			// first paint, when nothing is shown yet.
+			// (never re-blocks), so this is the only place a category becomes blocked.
 			this.patchStates({ [category]: {
-				status: (staleEntries.length === 0 && this.shouldBlockCategory(category, categories)) ? 'blocked' : 'loaded',
+				status: this.shouldBlockCategory(category, categories) ? 'blocked' : 'loaded',
 				entries,
 				cursor,
 				hasMore: this.hasMorePages(isPaginated, cursor),

core/src/tests/services/UnifiedSearchController.spec.ts

@@ -315,30 +315,29 @@ describe('UnifiedSearchController', () => {
 			expect(searchController.getRevealOrder()).toEqual(['talk', 'deck'])
 		})
 
-		it('keeps reveal positions across a refined query', async () => {
+		it('restarts the reveal order from priority on a new query', async () => {
 			const first = mockProviders(['files', 'talk'])
 
 			const searchController = new UnifiedSearchController()
 			searchController.search('old', ['files', 'talk'])
 
-			// talk gets on screen first, so the session order is talk before files.
+			// talk got on screen first, so this query renders talk above files.
 			first.talk.resolve(['Old talk'])
 			await vi.advanceTimersByTimeAsync(REVEAL_INTERVAL_MS)
 			first.files.resolve(['Old files'])
 			await vi.advanceTimersByTimeAsync(0)
 			expect(searchController.getRevealOrder()).toEqual(['talk', 'files'])
 
-			// Refining must not re-sort what is already on screen back to priority order.
-			// Both categories stay rendered throughout (stale-while-revalidate), so moving
-			// them would be a displacement with identical content.
+			// A new query hides everything: the results are about to be different, so there is
+			// nothing on screen to protect and the next paint starts from priority order again.
 			const second = mockProviders(['files', 'talk'])
 			searchController.search('new', ['files', 'talk'])
-			expect(searchController.getRevealOrder()).toEqual(['talk', 'files'])
+			expect(searchController.getRevealOrder()).toEqual([])
 
 			second.files.resolve(['New files'])
 			second.talk.resolve(['New talk'])
 			await vi.advanceTimersByTimeAsync(0)
-			expect(searchController.getRevealOrder()).toEqual(['talk', 'files'])
+			expect(searchController.getRevealOrder()).toEqual(['files', 'talk'])
 		})
 
 		it('releases a slot when a category loses its results, and appends it again if it returns', async () => {
@@ -360,17 +359,17 @@ describe('UnifiedSearchController', () => {
 			await vi.advanceTimersByTimeAsync(0)
 			expect(searchController.getRevealOrder()).toEqual(['talk'])
 
-			// It has results again on the next query, so it comes back as a fresh reveal:
-			// at the end, not back at its old priority slot.
+			// The next query is a clean slate, so it comes back in preferred order rather than
+			// staying demoted for the rest of the session.
 			const third = mockProviders(['files', 'talk'])
 			searchController.search('c', ['files', 'talk'])
 			third.files.resolve(['Files c'])
 			third.talk.resolve(['Talk c'])
 			await vi.advanceTimersByTimeAsync(0)
-			expect(searchController.getRevealOrder()).toEqual(['talk', 'files'])
+			expect(searchController.getRevealOrder()).toEqual(['files', 'talk'])
 		})
 
-		it('re-appends a category that left the search entirely instead of reclaiming its old slot', async () => {
+		it('recovers preferred order after a provider filter round trip', async () => {
 			const first = mockProviders(['files', 'talk'])
 
 			const searchController = new UnifiedSearchController()
@@ -380,24 +379,22 @@ describe('UnifiedSearchController', () => {
 			await vi.advanceTimersByTimeAsync(0)
 			expect(searchController.getRevealOrder()).toEqual(['files', 'talk'])
 
-			// A provider filter narrows the search: files leaves the category list altogether,
-			// which is a different exit from losing its results (that one goes through
-			// syncRevealOrder; this one goes through the prune in search()).
+			// A provider filter narrows the search: files leaves the category list altogether.
 			const second = mockProviders(['talk'])
 			searchController.search('foo', ['talk'])
 			second.talk.resolve(['Talk result'])
 			await vi.advanceTimersByTimeAsync(0)
 			expect(searchController.getRevealOrder()).toEqual(['talk'])
 
-			// The filter comes off. talk never left the screen, so files has to come back below
-			// it: reclaiming slot 0 would shove a rendered group down.
+			// The filter comes off. Each search stands on its own, so files is back on top
+			// instead of being stuck below talk until the popover closes.
 			const third = mockProviders(['files', 'talk'])
 			searchController.search('foo', ['files', 'talk'])
 			third.files.resolve(['Files result'])
 			third.talk.resolve(['Talk result'])
 			await vi.advanceTimersByTimeAsync(0)
 
-			expect(searchController.getRevealOrder()).toEqual(['talk', 'files'])
+			expect(searchController.getRevealOrder()).toEqual(['files', 'talk'])
 		})
 
 		it('never hands out a category the snapshot cannot index, part-way through a search', async () => {
@@ -443,41 +440,39 @@ describe('UnifiedSearchController', () => {
 			first.talk.resolve(['Talk result'])
 			await vi.advanceTimersByTimeAsync(0)
 
-			// A narrower search replaces the first. The dropped categories must
-			// not linger in the snapshot, nor in the display order: the view maps the
-			// order straight onto the snapshot and would hit a missing category.
+			// A narrower search replaces the first. The dropped categories must not linger in
+			// the snapshot, and nothing from the previous query stays on screen.
 			mockProviders(['files'])
 			searchController.search('second', ['files'])
 
 			expect(searchController.getSnapshot()).toEqual({
-				files: { status: 'loading', entries: ['Files result'], cursor: null, hasMore: false, loadMoreFailed: false },
+				files: loading,
 			})
-			expect(searchController.getRevealOrder()).toEqual(['files'])
+			expect(searchController.getRevealOrder()).toEqual([])
 		})
 	})
 
-	describe('stale-while-revalidate', () => {
-		it('keeps the previous results visible while a refetch is in flight', async () => {
+	describe('changing the query', () => {
+		it('drops the previous results as soon as the query changes', async () => {
 			const first = mockProviders(['files'])
 
 			const searchController = new UnifiedSearchController()
 			searchController.search('old', ['files'])
 			first.files.resolve(['Old result'])
 			await vi.advanceTimersByTimeAsync(0)
 
-			// A refined query starts a new search. The prior entries must stay on screen
-			// (status loading, entries kept) so the panel does not flash empty mid-request.
+			// The new query is about to return different results, so keeping the old ones up
+			// would only let them shift under the user once the real ones land. Hide, then show.
 			const second = mockProviders(['files'])
 			searchController.search('new', ['files'])
 			expect(searchController.getSnapshot().files).toEqual({
 				status: 'loading',
-				entries: ['Old result'],
+				entries: [],
 				cursor: null,
 				hasMore: false,
 				loadMoreFailed: false,
 			})
 
-			// The fresh page replaces them once it lands.
 			second.files.resolve(['New result'])
 			await vi.advanceTimersByTimeAsync(0)
 			expect(searchController.getSnapshot().files).toEqual({
@@ -489,7 +484,7 @@ describe('UnifiedSearchController', () => {
 			})
 		})
 
-		it('settles a refetched category that carried results straight to loaded, never blocked', async () => {
+		it('puts every category back through the ordered reveal on a new query', async () => {
 			const first = mockProviders(['files', 'talk'])
 
 			const searchController = new UnifiedSearchController()
@@ -499,23 +494,15 @@ describe('UnifiedSearchController', () => {
 			first.talk.resolve(['Old talk'])
 			await vi.advanceTimersByTimeAsync(0)
 
-			// Refine. talk (lower priority) comes back before files this time. It already had
-			// results, so it must not drop into blocked (which excludes it from the rendered
-			// set and blinks it off screen); it stays visible by settling straight to loaded.
+			// Refine. talk comes back first this time. Nothing is on screen to protect any more,
+			// so it takes its turn in the queue again instead of skipping the reveal.
 			const second = mockProviders(['files', 'talk'])
 			searchController.search('new', ['files', 'talk'])
 			second.talk.resolve(['New talk'])
 			await vi.advanceTimersByTimeAsync(0)
 
-			expect(searchController.getSnapshot().talk.status).toBe('loaded')
-			// files is still fetching; its stale page stays up meanwhile.
-			expect(searchController.getSnapshot().files).toEqual({
-				status: 'loading',
-				entries: ['Old files'],
-				cursor: null,
-				hasMore: false,
-				loadMoreFailed: false,
-			})
+			expect(searchController.getSnapshot().talk.status).toBe('blocked')
+			expect(searchController.getRevealOrder()).toEqual([])
 		})
 	})
 
@@ -1070,10 +1057,8 @@ describe('UnifiedSearchController', () => {
 			const searchController = new UnifiedSearchController()
 			searchController.search('first', ['files', 'talk'])
 
-			// The first search spends its window on talk, then stands the timer down. talk
-			// comes back empty so it carries no stale results into the second search, which
-			// would otherwise settle it straight to loaded and never block it.
-			first.talk.resolve([])
+			// The first search spends its window on talk, then stands the timer down.
+			first.talk.resolve(['First talk'])
 			await vi.advanceTimersByTimeAsync(REVEAL_INTERVAL_MS)
 			expect(searchController.getSnapshot().talk.status).toBe('loaded')
 			expect(vi.getTimerCount()).toBe(0)