diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.test.ts index ed0b51d64..aa5e93161 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.test.ts @@ -14,7 +14,7 @@ const view = (version: number) => resolveReviewSourceView({ reviewId: "review-a" function setup() { let provider: ITextModelContentProvider; let disposed = 0; - const models = new Map(); + const models = new Map(); const opened: Array<{ original: { resource: URI }; modified: { resource: URI } }> = []; const editor = { resource: URI.parse("review-api-source://review-a/file") }; const registered: string[] = []; @@ -34,13 +34,13 @@ function setup() { } as never, { getModel: (uri: URI) => models.get(uri.toString()), - createModel: (text: string, _: unknown, uri: URI) => { - const model = { uri, text, getLineCount: () => text.split("\n").length }; + createModel: (text: string, language: unknown, uri: URI) => { + const model = { uri, text, language, getLineCount: () => text.split("\n").length }; models.set(uri.toString(), model); return model; }, } as never, - { createByFilepathOrFirstLine: () => ({ languageId: "typescript" }) } as never, + { createById: (languageId: string) => ({ languageId }), createByFilepathOrFirstLine: () => ({ languageId: "typescript" }) } as never, { openEditor: async (input: (typeof opened)[number]) => { opened.push(input); @@ -239,3 +239,31 @@ test("a source at its own pins keeps them through its URI and reads them back fr const model = await readModel(uri); assert.equal((model as unknown as { text: string }).text, "at own pins"); }); + +for (const side of ["base", "head"] as const) { + test(`binary ${side} source resolves to a notice using plaintext instead of the file language`, async (t) => { + const { service, readModel, models } = setup(); + t.after(() => service.dispose()); + t.mock.method(globalThis, "fetch", async (value: string) => { + const url = new URL(value); + assert.equal(url.searchParams.get("binary"), "describe"); + assert.equal(url.searchParams.get("side"), side); + return Response.json({ binary: true, file: "image.png", side, commit: "pinned" }); + }); + const uri = apiSourceUri({ view: view(0), side, file: "image.png" }); + await readModel(uri); + assert.match(models.get(uri.toString())!.text, /Binary file.*cannot be displayed as text/); + assert.deepEqual(models.get(uri.toString())!.language, { languageId: "plaintext" }); + }); +} + +test("absent binary diff sides resolve as empty without reading the missing file", async (t) => { + const { service, readModel, models } = setup(); + t.after(() => service.dispose()); + t.mock.method(globalThis, "fetch", async () => { throw new Error("Absent sides must not request source"); }); + for (const side of ["base", "head"] as const) { + const uri = apiSourceUri({ view: view(0), side, file: "image.png" }, true); + await readModel(uri); + assert.equal(models.get(uri.toString())!.text, ""); + } +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.ts index c71b01890..6661cc89c 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.ts @@ -109,18 +109,21 @@ export class ReviewApiSourceService extends Disposable implements IReviewApiSour if (existing) return existing; const query = new URLSearchParams(resource.query); const target = sourceLocation(resource); - const body = query.has("empty") + const body: { text: string; localPath?: string; binary?: false } | { binary: true } = query.has("empty") ? { text: "" } - : await this.read<{ text: string; localPath?: string }>(target.view.reviewId, "/file", { ...reviewSourceQuery(target.view), side: target.side, file: target.file }); + : await this.read(target.view.reviewId, "/file", { ...reviewSourceQuery(target.view), side: target.side, file: target.file, binary: "describe" }); + // Keep the file visible in source/diff browsing without treating its + // bytes (or this notice) as source code. Authoring still validates it. + const text = body.binary ? "Binary file cannot be displayed as text." : body.text; const model = ( modelService.getModel(resource) ?? modelService.createModel( - body.text, - languages.createByFilepathOrFirstLine(resource, body.text.split("\n", 1)[0]), + text, + body.binary ? languages.createById("plaintext") : languages.createByFilepathOrFirstLine(resource, text.split("\n", 1)[0]), resource, ) ); - if (body.localPath) { + if (!body.binary && body.localPath) { this.followDisk(model, URI.file(body.localPath), async () => (await this.read<{ text: string }>(target.view.reviewId, "/file", { ...reviewSourceQuery(target.view), side: target.side, file: target.file })).text); } return model; diff --git a/packages/review/src/review-api/http.ts b/packages/review/src/review-api/http.ts index 46e4cb922..8a7dd6115 100644 --- a/packages/review/src/review-api/http.ts +++ b/packages/review/src/review-api/http.ts @@ -819,7 +819,11 @@ export function createReviewApi( ); }); app.get("/:id/file", async (context) => { - const input = readQuerySchemas.file.parse(context.req.query()); + // Browsing can describe binaries; authoring reads still require text. + const input = readQuerySchemas.file + .extend({ binary: z.literal("describe").optional() }) + .parse(context.req.query()); + const id = context.req.param("id"); const anchor = queryAnchor(input); @@ -830,7 +834,21 @@ export function createReviewApi( anchor, ); - const file = await data.file(pins, input.side, input.file); + const file = await data.file( + pins, + input.side, + input.file, + input.binary === "describe", + ); + + if (file.text.includes("\0")) { + return context.json({ + binary: true, + file: file.file, + side: file.side, + commit: file.commit, + }); + } const local = !input.commit && diff --git a/packages/review/src/review-api/local-data.test.ts b/packages/review/src/review-api/local-data.test.ts index 2b73f1cf6..f266e8d9c 100644 --- a/packages/review/src/review-api/local-data.test.ts +++ b/packages/review/src/review-api/local-data.test.ts @@ -1381,6 +1381,48 @@ it("refuses a committed binary file as a code reference", async () => { ).rejects.toThrow("Binary files cannot be used as code references."); }); +it("describes binary source for browsing without allowing it as code evidence", async () => { + writeFileSync(path.join(repository, "binary.bin"), "text\u0000more\n"); + git("add", "binary.bin"); + git("-c", "commit.gpgsign=false", "commit", "-qm", "Binary"); + + const binaryPins = await local.data.resolvePins( + pins.repositoryId, + pins.head, + "HEAD", + ); + + const review = await local.store.execute( + command({ type: "create", title: "Binary", pins: binaryPins }), + ); + + const app = createReviewApi(local.store, local.data); + const route = `/${review.reviewId}/file?side=head&file=binary.bin`; + const response = await app.request(`${route}&binary=describe`); + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ + binary: true, + file: "binary.bin", + side: "head", + commit: binaryPins.head, + }); + expect((await app.request(route)).status).toBe(400); + await expect( + insert(review.reviewId, { + type: "code_peek", + source: selectSource({ ...source, file: "binary.bin", toLine: 1 }), + }), + ).rejects.toThrow("Binary files cannot be used as code references."); + + const text = await app.request( + `/${review.reviewId}/file?side=head&file=example.ts&binary=describe`, + ); + + expect(await text.json()).toMatchObject({ + text: "export const value = 2;\nexport const saved = true;\n", + }); +}); + it("reads a committed empty file as empty text, not a missing file", async () => { writeFileSync(path.join(repository, "blank.ts"), ""); git("add", "blank.ts");