fix: display binary review sources without renderer exceptions

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<string, { uri: URI; text: string; getLineCount(): number }>();
+	const models = new Map<string, { uri: URI; text: string; language: unknown; getLineCount(): number }>();
 	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, "");
+	}
+});

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;

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 &&

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");