Skip to content
Merged
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
16 changes: 11 additions & 5 deletions src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 === "<" ? "&lt;" : "&gt;"))}\``;
.replace(/[<>]/g, (char) => (char === "<" ? "&lt;" : "&gt;"));
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
Expand All @@ -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));
Expand All @@ -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<ReviewFileClass, { count: number; additions: number; deletions: number }>();
for (const file of files) {
Expand Down
39 changes: 38 additions & 1 deletion test/unit/changed-files-summary-collapsible.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,48 @@ describe("buildChangedFilesSummaryCollapsible per-file diff links (#2157)", () =
context,
);
expect(c?.body).toContain("&lt;1&gt;");
expect(c?.body).toContain("\\`");
expect(c?.body).toContain("``src/weird");
expect(c?.body).toContain("file&lt;1&gt;.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 | — |");
Expand Down
Loading