fix(core/xref-db): skip caching empty xref results#5260
Conversation
When the xref API returns no result for a query, `cacheXrefData()` was storing an empty array in IDB. On subsequent loads, the cache hit prevented re-fetching, permanently suppressing results for terms that were temporarily missing from the API. Skip caching queries with no results so they are re-fetched next time. Closes speced#5256
There was a problem hiding this comment.
Pull request overview
Fixes core/xref-db’s IndexedDB caching behavior so that “no results” responses from the xref API don’t get cached and block future refetches, addressing #5256.
Changes:
- Stop writing entries to IDB when a query’s xref result is empty (
[]). - Ensure missing/empty results will be re-fetched on subsequent runs instead of being treated as a cache hit.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot feedback: resolveXrefCache now uses a readwrite transaction that deletes empty-result entries it encounters, recovering from previously cached empty arrays.
Two tests using real IDB: 1. Queries with empty results are not cached (write path) 2. Pre-existing empty cache entries are deleted on read (migration) Address Copilot feedback requesting test coverage.
|
@copilot Can you confirm all your feedback has been addressed? Can you fix anything that was not addressed? |
1 similar comment
|
@copilot Can you confirm all your feedback has been addressed? Can you fix anything that was not addressed? |
| let cursor = await cache.transaction(STORE_NAME).store.openCursor(); | ||
| const tx = cache.transaction(STORE_NAME, "readwrite"); | ||
| let cursor = await tx.store.openCursor(); | ||
| while (cursor) { | ||
| if (requiredKeySet.has(cursor.key)) { | ||
| cachedData.set(cursor.key, cursor.value.result); | ||
| if (cursor.value.result?.length) { | ||
| cachedData.set(cursor.key, cursor.value.result); | ||
| } else { | ||
| cursor.delete(); | ||
| } | ||
| } | ||
| cursor = await cursor.continue(); | ||
| } | ||
| await tx.done; |
There was a problem hiding this comment.
Maybe these changes aren't really needed, if we skip storing these in the first place. For existing cached entries, we'll eventually bust the entire cache, iirc.
There was a problem hiding this comment.
Agreed. Simplified in f76c548: removed the read-side cleanup entirely. Now the only change vs main is the write-side skip (if (!result?.length) continue). Existing stale entries get cleared by the version-based cache bust.
Per sidvishnoi: "Maybe these changes aren't really needed, if we skip storing these in the first place." The write-side fix (skip storing empty results) prevents the problem. Existing stale entries will be cleared by the version-based cache bust. No need for a readwrite transaction on the read path.
Closes #5256
When the xref API returns no result for a query, `cacheXrefData()` stored an empty array in IDB. On subsequent loads, the cache hit prevented re-fetching, permanently suppressing results for terms that were temporarily missing from the API.
Skips caching queries with no results so they are re-fetched next time.