Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed
- Made the default home page configurable with `DEFAULT_HOME_VIEW_PAGE`, defaulting to Code Search and supporting Ask. [#1677](https://github.com/sourcebot-dev/sourcebot/pull/1677)
- Require authentication for the streaming and blocking Ask APIs in Public SaaS deployments. [#1679](https://github.com/sourcebot-dev/sourcebot/pull/1679)
- Fixed duplicate repository metadata lookups within search result chunks. [#1684](https://github.com/sourcebot-dev/sourcebot/pull/1684)

## [5.1.14] - 2026-09-17

Expand Down
94 changes: 93 additions & 1 deletion packages/web/src/features/search/zoektSearcher.test.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
import type { PrismaClient } from '@sourcebot/db';
import type { SearchRequest as ZoektGrpcSearchRequest } from '@/proto/zoekt/webserver/v1/SearchRequest';
import { beforeEach, describe, expect, test, vi } from 'vitest';
import { EventEmitter } from 'node:events';

const mocks = vi.hoisted(() => {
const close = vi.fn();
const search = vi.fn();
const streamSearch = vi.fn();

class WebserverService {
Search = search;
StreamSearch = streamSearch;
close = close;
}

return {
close,
loadSync: vi.fn(() => ({})),
search,
streamSearch,
WebserverService,
};
});
Expand Down Expand Up @@ -60,10 +64,27 @@ vi.mock('@/lib/posthog', () => ({
captureEvent: vi.fn(),
}));

import { zoektSearch } from './zoektSearcher';
import { zoektSearch, zoektStreamSearch } from './zoektSearcher';

const searchRequest = {} as ZoektGrpcSearchRequest;

const createFile = (id: number | undefined, repository = 'github.com/org/repo') => ({
repository_id: id,
repository,
file_name: Buffer.from('src/index.ts'),
chunk_matches: [],
branches: ['main'],
language: 'TypeScript',
});

const createRepo = (id: number, name = 'github.com/org/repo') => ({
id,
name,
displayName: name,
webUrl: null,
external_codeHostType: 'github',
});

describe('zoektSearch', () => {
beforeEach(() => {
vi.clearAllMocks();
Expand Down Expand Up @@ -104,4 +125,75 @@ describe('zoektSearch', () => {
await expect(zoektSearch(searchRequest, prisma)).rejects.toThrow('database unavailable');
expect(mocks.close).toHaveBeenCalledOnce();
});

test.each([1, 2])('looks up each of %i repositories only once for 100 files', async (repoCount) => {
const files = Array.from({ length: 100 }, (_, index) => createFile(index % repoCount + 1));
mocks.search.mockImplementation((_request, _metadata, callback) => {
callback(null, { files });
});
const findUnique = vi.fn(async ({ where: { id } }) => createRepo(id));
const prisma = { repo: { findUnique } } as unknown as PrismaClient;

const response = await zoektSearch(searchRequest, prisma);

expect(findUnique).toHaveBeenCalledTimes(repoCount);
expect(response.files).toHaveLength(100);
expect(response.files.map(file => file.repositoryId)).toEqual(files.map(file => file.repository_id));
expect(response.repositoryInfo).toHaveLength(repoCount);
});

test('deduplicates lookups by name for legacy shards without repository IDs', async () => {
mocks.search.mockImplementation((_request, _metadata, callback) => {
callback(null, { files: Array.from({ length: 100 }, () => createFile(undefined)) });
});
const findFirst = vi.fn().mockResolvedValue(createRepo(1));
const prisma = { repo: { findFirst } } as unknown as PrismaClient;

const response = await zoektSearch(searchRequest, prisma);

expect(findFirst).toHaveBeenCalledExactlyOnceWith({ where: { name: 'github.com/org/repo' } });
expect(response.files).toHaveLength(100);
});

test('looks up a missing repository once and omits its files', async () => {
mocks.search.mockImplementation((_request, _metadata, callback) => {
callback(null, { files: Array.from({ length: 100 }, () => createFile(1)) });
});
const findUnique = vi.fn().mockResolvedValue(null);
const prisma = { repo: { findUnique } } as unknown as PrismaClient;

const response = await zoektSearch(searchRequest, prisma);

expect(findUnique).toHaveBeenCalledOnce();
expect(response.files).toEqual([]);
expect(response.repositoryInfo).toEqual([]);
});

test('deduplicates streaming chunks and reuses repository metadata across chunks', async () => {
const grpcStream = Object.assign(new EventEmitter(), {
pause: vi.fn(),
resume: vi.fn(),
cancel: vi.fn(),
});
mocks.streamSearch.mockReturnValue(grpcStream);
const findUnique = vi.fn(async ({ where: { id } }) => createRepo(id));
const prisma = { repo: { findUnique } } as unknown as PrismaClient;
const stream = await zoektStreamSearch(searchRequest, prisma);
const reader = stream.getReader();

for (const ids of [[1, 1, 2, 2], [1, 2, 3, 3]]) {
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
expect(response.files).toHaveLength(ids.length);
expect(response.files).toHaveLength(ids.length);
expect(response.files.map(file => file.repositoryId)).toEqual(ids);

expect(response.repositoryInfo).toHaveLength(new Set(ids).size);
}

expect(findUnique).toHaveBeenCalledTimes(3);
grpcStream.emit('end');
while (!(await reader.read()).done) {
// Drain the final statistics and completion marker.
}
expect(mocks.close).toHaveBeenCalledOnce();
});
});
5 changes: 2 additions & 3 deletions packages/web/src/features/search/zoektSearcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -341,9 +341,8 @@ const encodeSSEREsponseChunk = (response: object | string) => {
// chunk. The mapping allows us to efficiently lookup repository metadata.
const createReposMapForChunk = async (chunk: ZoektGrpcSearchResponse, reposMapCache: Map<string | number, Repo>, prisma: PrismaClient): Promise<Map<string | number, Repo>> => {
const reposMap = new Map<string | number, Repo>();
await Promise.all(chunk.files.map(async (file) => {
const id = getRepoIdForFile(file);

const repoIds = [...new Set(chunk.files.map(getRepoIdForFile))];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

await Promise.all(repoIds.map(async (id) => {
const repo = await (async () => {
// If it's in the cache, return the cached value.
if (reposMapCache.has(id)) {
Expand Down