diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 441c71a579..0cc0f219a6 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -460,12 +460,18 @@ export type ChangedFileSummaryInput = { path: string; additions: number; deletio /** Repo + PR coordinates for per-file "View diff" links on the changed-files table (#2157). */ export type ChangedFilesSummaryContext = { repoFullName: string; pullNumber: number }; +const MAX_CHANGED_FILE_DIFF_ROWS = 200; +const MAX_CHANGED_FILES_DIFF_BODY_LENGTH = 30_000; + function markdownChangedFilePath(value: string): string { - return `\`${value + const safeValue = value + .replace(/[\u0000-\u001f\u007f]/g, "�") .replace(/\\/g, "\\\\") - .replace(/`/g, "\\`") .replace(/\|/g, "\\|") - .replace(/[<>]/g, (char) => (char === "<" ? "<" : ">"))}\``; + .replace(/[<>]/g, (char) => (char === "<" ? "<" : ">")); + const longestBacktickRun = safeValue.match(/`+/g)?.reduce((longest, run) => Math.max(longest, run.length), 0) ?? 0; + const fence = "`".repeat(longestBacktickRun + 1); + return `${fence}${safeValue}${fence}`; } /** Display order for the "Changed files" table — SOURCE FIRST, mirroring the same source-first priority this @@ -492,7 +498,7 @@ export function buildChangedFilesSummaryCollapsible( context?: ChangedFilesSummaryContext | undefined, ): UnifiedCollapsible | null { if (files.length === 0) return null; - if (context) { + if (context && files.length <= MAX_CHANGED_FILE_DIFF_ROWS) { const sorted = [...files].sort((left, right) => { const leftCategory = CHANGED_FILE_CATEGORY_ORDER.indexOf(classifyChangedFile(left.path)); const rightCategory = CHANGED_FILE_CATEGORY_ORDER.indexOf(classifyChangedFile(right.path)); @@ -505,7 +511,7 @@ export function buildChangedFilesSummaryCollapsible( return `| ${markdownChangedFilePath(file.path)} | +${file.additions} | -${file.deletions} | ${diffCell} |`; }); const body = ["| File | Added | Removed | |", "| --- | --- | --- | --- |", ...rows].join("\n"); - return { title: "Changed files", body }; + if (body.length <= MAX_CHANGED_FILES_DIFF_BODY_LENGTH) return { title: "Changed files", body }; } const totals = new Map(); for (const file of files) { diff --git a/test/unit/changed-files-summary-collapsible.test.ts b/test/unit/changed-files-summary-collapsible.test.ts index aa0b2d6a07..1350251fe8 100644 --- a/test/unit/changed-files-summary-collapsible.test.ts +++ b/test/unit/changed-files-summary-collapsible.test.ts @@ -59,11 +59,48 @@ describe("buildChangedFilesSummaryCollapsible per-file diff links (#2157)", () = context, ); expect(c?.body).toContain("<1>"); - expect(c?.body).toContain("\\`"); + expect(c?.body).toContain("``src/weird"); + expect(c?.body).toContain("file<1>.ts``"); expect(c?.body).toContain("\\|"); expect(c?.body).toContain("\\\\"); }); + it("neutralizes line breaks in per-file paths before rendering public Markdown", () => { + const c = buildChangedFilesSummaryCollapsible( + [ + { + path: "src/safe.ts\n@octocat\r\n[approve](mailto:attacker@example.com)", + additions: 1, + deletions: 0, + }, + ], + context, + ); + expect(c?.body).toContain("src/safe.ts�@octocat��[approve](mailto:attacker@example.com)"); + expect(c?.body).not.toContain("\n@octocat"); + expect(c?.body).not.toContain("\n[approve]"); + }); + + it("falls back to grouped category totals when too many per-file rows would be rendered", () => { + const manyFiles = Array.from({ length: 201 }, (_, index) => ({ + path: `src/file-${index}.ts`, + additions: 1, + deletions: 0, + })); + const c = buildChangedFilesSummaryCollapsible(manyFiles, context); + expect(c?.body).toContain("| Source | 201 | +201 | -0 |"); + expect(c?.body).not.toContain("[View diff]"); + }); + + it("falls back to grouped category totals when per-file rows would exceed the body budget", () => { + const c = buildChangedFilesSummaryCollapsible( + [{ path: `src/${"a".repeat(31_000)}.ts`, additions: 1, deletions: 0 }], + context, + ); + expect(c?.body).toContain("| Source | 1 | +1 | -0 |"); + expect(c?.body).not.toContain("[View diff]"); + }); + it("omits the View diff link when the path or repo context cannot be anchored", () => { const unanchored = buildChangedFilesSummaryCollapsible([{ path: " ", additions: 1, deletions: 0 }], context); expect(unanchored?.body).toContain("| ` ` | +1 | -0 | — |");