fix(web): deduplicate repository lookups in search results - #1684
dipeshbabu wants to merge 2 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. WalkthroughThe searcher now deduplicates repository IDs within each search result chunk before resolving repository metadata. Tests cover repeated IDs, legacy shards, missing repositories, and streaming chunks. The changelog records the fix. ChangesRepository lookup deduplication
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The lookup deduplication is ready to merge after normal checks; no actionable risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/web/src/features/search/zoektSearcher.test.ts">
<violation number="1" location="packages/web/src/features/search/zoektSearcher.test.ts:188">
P3: The streaming test verifies only file and repositoryInfo counts, never that each `response.files[i].repositoryId` corresponds to the emitted `repository_id`. The unary tests assert this mapping; the streaming chunk test should too, so a cache mis-association or reordering that keeps counts unchanged cannot slip through — it also directly covers the PR's "result order preserved" claim for the cross-chunk cache path.</violation>
</file>
<file name="packages/web/src/features/search/zoektSearcher.ts">
<violation number="1" location="packages/web/src/features/search/zoektSearcher.ts:344">
P3: Within a chunk the dedup is correct, but repositories that are not in the database never get cached (`if (repo) { reposMapCache.set(id, repo); }` only stores hits), so a missing repo referenced by a shard is still looked up once per streaming chunk instead of once per stream. Since this PR exists to collapse repeated lookups, track ids already queried in this stream (e.g., a `Set` of looked-up ids next to `_reposMapCache`) and skip re-querying them.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| grpcStream.emit('data', { response_chunk: { files: ids.map(id => createFile(id)) } }); | ||
| const chunk = await reader.read(); | ||
| const response = JSON.parse(new TextDecoder().decode(chunk.value).slice('data: '.length)); | ||
| expect(response.files).toHaveLength(ids.length); |
There was a problem hiding this comment.
P3: The streaming test verifies only file and repositoryInfo counts, never that each response.files[i].repositoryId corresponds to the emitted repository_id. The unary tests assert this mapping; the streaming chunk test should too, so a cache mis-association or reordering that keeps counts unchanged cannot slip through — it also directly covers the PR's "result order preserved" claim for the cross-chunk cache path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/web/src/features/search/zoektSearcher.test.ts, line 188:
<comment>The streaming test verifies only file and repositoryInfo counts, never that each `response.files[i].repositoryId` corresponds to the emitted `repository_id`. The unary tests assert this mapping; the streaming chunk test should too, so a cache mis-association or reordering that keeps counts unchanged cannot slip through — it also directly covers the PR's "result order preserved" claim for the cross-chunk cache path.</comment>
<file context>
@@ -104,4 +125,75 @@ describe('zoektSearch', () => {
+ grpcStream.emit('data', { response_chunk: { files: ids.map(id => createFile(id)) } });
+ const chunk = await reader.read();
+ const response = JSON.parse(new TextDecoder().decode(chunk.value).slice('data: '.length));
+ expect(response.files).toHaveLength(ids.length);
+ expect(response.repositoryInfo).toHaveLength(new Set(ids).size);
+ }
</file context>
| expect(response.files).toHaveLength(ids.length); | |
| expect(response.files).toHaveLength(ids.length); | |
| expect(response.files.map(file => file.repositoryId)).toEqual(ids); |
| await Promise.all(chunk.files.map(async (file) => { | ||
| const id = getRepoIdForFile(file); | ||
|
|
||
| const repoIds = [...new Set(chunk.files.map(getRepoIdForFile))]; |
There was a problem hiding this comment.
P3: Within a chunk the dedup is correct, but repositories that are not in the database never get cached (if (repo) { reposMapCache.set(id, repo); } only stores hits), so a missing repo referenced by a shard is still looked up once per streaming chunk instead of once per stream. Since this PR exists to collapse repeated lookups, track ids already queried in this stream (e.g., a Set of looked-up ids next to _reposMapCache) and skip re-querying them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/web/src/features/search/zoektSearcher.ts, line 344:
<comment>Within a chunk the dedup is correct, but repositories that are not in the database never get cached (`if (repo) { reposMapCache.set(id, repo); }` only stores hits), so a missing repo referenced by a shard is still looked up once per streaming chunk instead of once per stream. Since this PR exists to collapse repeated lookups, track ids already queried in this stream (e.g., a `Set` of looked-up ids next to `_reposMapCache`) and skip re-querying them.</comment>
<file context>
@@ -341,9 +341,8 @@ const encodeSSEREsponseChunk = (response: object | string) => {
- await Promise.all(chunk.files.map(async (file) => {
- const id = getRepoIdForFile(file);
-
+ const repoIds = [...new Set(chunk.files.map(getRepoIdForFile))];
+ await Promise.all(repoIds.map(async (id) => {
const repo = await (async () => {
</file context>
This branch has not been deployed
1 participant
Fixes #1681
Search results containing many files from one repository started a database lookup for every file before the metadata cache was populated. Deduplicate repository identifiers before starting those lookups, so 100 files from one repository make one lookup and 100 files across two repositories make two.
Preserves legacy name-based lookup, missing-repository handling, result order, and the existing cache across streamed chunks. Regression tests cover each of these cases.
Validation completed before opening this PR:
The verification workflow lives on a separate branch in the fork.
Note
Low Risk
Localized search-path optimization with existing behavior preserved and new regression tests; no auth or data-model changes.
Overview
Fixes redundant Prisma repository lookups when Zoekt returns many file matches from the same repo(s).
createReposMapForChunknow collects unique repository IDs (or legacy names) per chunk before querying, instead of starting a lookup per file.Unary and streaming search still use the same cache, name-based fallback for shards without IDs, and behavior when a repo is missing (files omitted). CHANGELOG notes the fix;
zoektSearcher.test.tsadds regression tests for 100-file deduplication, legacy name lookup, missing repos, and stream chunks reusing cached metadata.Reviewed by Cursor Bugbot for commit cde93da. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #1681 so search results with many files from the same repository no longer trigger a database lookup per file.
Deduplicates repository identifiers before resolving repository metadata in
zoektSearcher.ts, so 100 files from one repository make one lookup and 100 files across two repositories make two. Legacy name-based lookups, missing-repository handling, result order, and the existing metadata cache across streamed chunks are preserved. Adds a changelog entry.Tests
Written for commit cde93da. Summary will update on new commits.
Summary by CodeRabbit