fix(web): deduplicate repository lookups in search results - #1684
dipeshbabu wants to merge 3 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)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. WalkthroughThe searcher now deduplicates repository lookups within each result chunk. It batches uncached numeric IDs, resolves legacy names, and caches missing repositories. Tests cover unary and streaming searches. ChangesRepository lookup deduplication
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change reduces redundant repository lookups during search without altering the search request or response contracts. No merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
|
Great catch @dipeshbabu! Deduplicating repository IDs before resolving metadata eliminates redundant lookups when multiple file matches belong to the same repository. One additional optimization to consider: Currently, the PR deduplicates IDs but still uses Promise.all(repoIds.map(...)) with individual findUnique calls: While this collapses 100 files in 1 repo to 1 query, in broad or multi-repository searches where a chunk spans matches across 15–20 repositories, repoIds.map(findUnique) still fires 15–20 individual concurrent database queries. We could batch all uncached lookups into a single query using Prisma's findMany: This guarantees that regardless of how many repositories a chunk touches (1 or 20), exactly one single batch query is dispatched to PostgreSQL, completely preventing connection pool contention. |
|
Batched uncached IDs with findMany, keeping the scoped Prisma client. Missing rows are cached per stream too. Tests, lint and Docker builds passed. |
Fixes #1681
Batch uncached repository IDs into one scoped Prisma query per chunk and cache missing IDs/names for the request. Preserve legacy lookups and file order; tests cover mixed identifiers and streamed metadata mappings.
Validation: application tests, lint, Docker builds, Prisma migration gate, and regression checks passed.
Note
Low Risk
Search-result enrichment only; behavior is preserved with broad regression tests and no API or auth changes.
Overview
Fixes redundant Postgres work when Zoekt returns many file matches that share the same repository.
createReposMapForChunkinzoektSearcher.tsno longer runs afindUnique/findFirstper file. It collects unique repository keys from the chunk, skips keys already in the stream/unary cache, batches numeric IDs into a singlefindMany, and resolves each legacy name once. Missing numeric IDs are cached asnullso later streamed chunks do not repeat failed lookups; files for unknown repos are still dropped as before.Regression tests in
zoektSearcher.test.tscover batching, legacy name shards, missing repos, streaming cache reuse across chunks, and mixed numeric ID vs name identifiers. The unreleased changelog notes the fix.Reviewed by Cursor Bugbot for commit 5fd8298. 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