opt(cardinal): Resolve flicker when search results updated#175
opt(cardinal): Resolve flicker when search results updated#175
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate UI flicker when file search results update by preventing stale, in-flight node info responses from mutating the current cache, and by reusing already-hydrated row data across backend result-set updates.
Changes:
- Introduces a monotonic “request epoch” in
useDataLoaderto drop lateget_nodes_inforesponses after a result-set change. - Stops clearing the row cache on
dataResultsVersionchanges to reuse previously hydrated rows and reduce visible blank/loading placeholders. - Updates and expands
useDataLoadertests to cover cache reuse and stale-response suppression.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cardinal/src/hooks/useDataLoader.ts | Adds request-epoch guarding and changes cache lifecycle behavior across result-set updates. |
| cardinal/src/hooks/tests/useDataLoader.test.ts | Updates tests to validate cache reuse and ignoring stale responses. |
| cardinal/src/components/VirtualList.tsx | Updates prop comment to reflect new epoch behavior. |
| cardinal/src/components/FilesTabContent.tsx | Updates prop comment to reflect stale-fetch rejection behavior. |
| cardinal/src/App.tsx | Updates inline documentation for dataResultsVersion semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // cannot write into the cache for a newer query. Cached rows stay reusable by slab index. | ||
| useEffect(() => { | ||
| versionRef.current += 1; | ||
| // Clear "currently loading" bookkeeping so the next visible range can request any slab | ||
| // indices that were pending for the previous result-set but never resolved into this one. | ||
| loadingRef.current.clear(); |
There was a problem hiding this comment.
On dataResultsVersion change you no longer reset/prune cacheRef/iconOverridesRef, which means entries can accumulate indefinitely (memory growth across searches). More importantly, this assumes a SlabIndex always refers to the same node across result-set versions; however the backend uses a slab with a freelist and removes nodes, so indices can be recycled. If a SlabIndex is reused for a different path after a rescan/deletion, ensureRangeLoaded will treat it as a cache hit and never refetch, potentially rendering the wrong row data. Consider either (a) keeping the epoch-based stale-response guard but pruning/clearing cache and icon overrides on dataResultsVersion, or (b) tracking per-entry generation/epoch so cached items from prior result-sets are not reused without a refresh.
| // cannot write into the cache for a newer query. Cached rows stay reusable by slab index. | |
| useEffect(() => { | |
| versionRef.current += 1; | |
| // Clear "currently loading" bookkeeping so the next visible range can request any slab | |
| // indices that were pending for the previous result-set but never resolved into this one. | |
| loadingRef.current.clear(); | |
| // cannot write into the cache for a newer query. Because slab indices can be recycled | |
| // by the backend across result-set versions, clear all slab-index keyed caches here. | |
| useEffect(() => { | |
| versionRef.current += 1; | |
| // Clear "currently loading" bookkeeping so the next visible range can request any slab | |
| // indices that were pending for the previous result-set but never resolved into this one. | |
| loadingRef.current.clear(); | |
| const nextCache = new Map<SlabIndex, SearchResultItem>(); | |
| cacheRef.current = nextCache; | |
| iconOverridesRef.current = new Map<SlabIndex, IconOverrideValue>(); | |
| setCache(nextCache); |
| if (needLoading.length === 0) return; | ||
| const versionAtRequest = versionRef.current; | ||
| const fetched = await invoke<NodeInfoResponse[]>('get_nodes_info', { results: needLoading }); | ||
| if (versionRef.current !== versionAtRequest) { | ||
| // The result-set changed while this request was in flight. Drop the payload instead of | ||
| // merging stale rows into the cache for the new query. | ||
| releaseLoadingBatch(needLoading); | ||
| return; |
There was a problem hiding this comment.
invoke('get_nodes_info', ...) is awaited without a try/catch. If the invoke rejects (backend error, IPC failure, etc.), the loadingRef entries for needLoading will never be released, so those rows can get stuck permanently as "loading" (and never be requested again until dataResultsVersion changes and clears the set). Wrap the invoke in a try/catch/finally and ensure releaseLoadingBatch(needLoading) runs on the error path.
No description provided.