mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
fix(review): survive long PR checkout warmups + classify reconstructed renames
Stress-testing against oven-sh/bun#30412 (2,188 files) surfaced three bugs: - Bun.serve's default 10s idleTimeout killed /api/semantic-diff while it parked on the background checkout warmup (a clone that can take minutes). Disable the idle timeout on all servers — AI SSE streams can also stall >10s between bytes while a permission prompt waits. - The file-badge hook memoized that failed fetch in a module-level cache keyed by patch, pinning every badge to empty until a hard refresh. Never cache failures; retry with backoff (5s/15s/30s). - reconstructGhPatch/reconstructPatch omitted the `similarity index` line, which Pierre's parser keys rename classification off — pure renames rendered as blank plain changes with no old path. Emit 100% for patch-less renames/copies (exactly accurate) and a synthetic 99% for patched ones (consumers only branch on 100% vs not).
This commit is contained in:
@@ -9,32 +9,60 @@ import { isOrphanChange } from '../dock/panels/semanticDiffShared';
|
||||
/**
|
||||
* Single shared fetch of the semantic diff, cached by the active patch so every
|
||||
* file-header badge reuses one request (sem already caches per-patch server-side).
|
||||
*
|
||||
* Failures are retried with backoff and never memoized — in PR mode the server
|
||||
* parks this request while the checkout warms up (a clone that can take
|
||||
* minutes), and a connection killed mid-wait must not pin every badge to empty
|
||||
* for the rest of the session.
|
||||
*/
|
||||
let cacheKey: string | null = null;
|
||||
let cachePromise: Promise<SemanticDiffResponse> | null = null;
|
||||
|
||||
const RETRY_DELAYS_MS = [5_000, 15_000, 30_000];
|
||||
|
||||
async function fetchSemanticDiff(): Promise<SemanticDiffResponse> {
|
||||
const res = await fetch('/api/semantic-diff');
|
||||
if (!res.ok) throw new Error('Semantic diff failed');
|
||||
return res.json() as Promise<SemanticDiffResponse>;
|
||||
}
|
||||
|
||||
function loadSemanticDiff(rawPatch: string): Promise<SemanticDiffResponse> {
|
||||
if (cacheKey === rawPatch && cachePromise) return cachePromise;
|
||||
cacheKey = rawPatch;
|
||||
cachePromise = fetch('/api/semantic-diff')
|
||||
.then((res) => {
|
||||
if (!res.ok) throw new Error('Semantic diff failed');
|
||||
return res.json() as Promise<SemanticDiffResponse>;
|
||||
})
|
||||
.catch((error): SemanticDiffResponse => ({
|
||||
status: 'error',
|
||||
reason: 'fetch-failed',
|
||||
message: error instanceof Error ? error.message : String(error),
|
||||
}))
|
||||
.then((data) => {
|
||||
// Logged once per patch (the promise is cached) rather than per badge, so a
|
||||
// systemic failure leaves a trace instead of every badge vanishing silently.
|
||||
if (data.status !== 'ok') {
|
||||
console.error('Failed to load semantic diff for file badges:', data.message ?? data.reason ?? data.status);
|
||||
|
||||
const attempt = async (): Promise<SemanticDiffResponse> => {
|
||||
for (let i = 0; ; i++) {
|
||||
let result: SemanticDiffResponse;
|
||||
try {
|
||||
result = await fetchSemanticDiff();
|
||||
} catch (error) {
|
||||
result = {
|
||||
status: 'error',
|
||||
reason: 'fetch-failed',
|
||||
message: error instanceof Error ? error.message : String(error),
|
||||
};
|
||||
}
|
||||
return data;
|
||||
});
|
||||
return cachePromise;
|
||||
// 'unavailable' means sem isn't installed — retrying won't change that.
|
||||
if (result.status !== 'error' || i >= RETRY_DELAYS_MS.length) return result;
|
||||
await new Promise((resolve) => setTimeout(resolve, RETRY_DELAYS_MS[i]));
|
||||
if (cacheKey !== rawPatch) return result; // patch changed mid-retry; let the new fetch win
|
||||
}
|
||||
};
|
||||
|
||||
const promise = attempt().then((data) => {
|
||||
// Logged once per patch (the promise is cached) rather than per badge, so a
|
||||
// systemic failure leaves a trace instead of every badge vanishing silently.
|
||||
if (data.status !== 'ok') {
|
||||
console.error('Failed to load semantic diff for file badges:', data.message ?? data.reason ?? data.status);
|
||||
}
|
||||
if (data.status === 'error' && cacheKey === rawPatch && cachePromise === promise) {
|
||||
cacheKey = null;
|
||||
cachePromise = null;
|
||||
}
|
||||
return data;
|
||||
});
|
||||
cachePromise = promise;
|
||||
return promise;
|
||||
}
|
||||
|
||||
export interface FileSemanticChanges {
|
||||
|
||||
@@ -178,6 +178,9 @@ export async function startAnnotateServer(
|
||||
server = Bun.serve({
|
||||
hostname: getServerHostname(),
|
||||
port: configuredPort,
|
||||
// Bun's default 10s idleTimeout kills AI SSE streams that stall
|
||||
// between bytes (e.g. while a permission prompt waits on the user).
|
||||
idleTimeout: 0,
|
||||
|
||||
async fetch(req, server) {
|
||||
const url = new URL(req.url);
|
||||
|
||||
@@ -110,6 +110,8 @@ export async function startGoalSetupServer(
|
||||
server = Bun.serve({
|
||||
hostname: getServerHostname(),
|
||||
port: configuredPort,
|
||||
// Bun's default 10s idleTimeout kills long-running requests.
|
||||
idleTimeout: 0,
|
||||
|
||||
async fetch(req) {
|
||||
const url = new URL(req.url);
|
||||
|
||||
@@ -215,6 +215,9 @@ export async function startPlannotatorServer(
|
||||
server = Bun.serve({
|
||||
hostname: getServerHostname(),
|
||||
port: configuredPort,
|
||||
// Bun's default 10s idleTimeout kills AI SSE streams that stall
|
||||
// between bytes (e.g. while a permission prompt waits on the user).
|
||||
idleTimeout: 0,
|
||||
|
||||
async fetch(req, server) {
|
||||
const url = new URL(req.url);
|
||||
|
||||
@@ -657,6 +657,10 @@ export async function startReviewServer(
|
||||
server = Bun.serve({
|
||||
hostname: getServerHostname(),
|
||||
port: configuredPort,
|
||||
// Bun's default 10s idleTimeout kills requests that legitimately park:
|
||||
// PR-mode endpoints await the background checkout warmup (a clone that
|
||||
// can take minutes) and AI SSE streams can stall between bytes.
|
||||
idleTimeout: 0,
|
||||
|
||||
async fetch(req, server) {
|
||||
const url = new URL(req.url);
|
||||
|
||||
@@ -197,6 +197,9 @@ describe("reconstructGhPatch", () => {
|
||||
expect(lines[0]).toBe("diff --git a/before.ts b/after.ts");
|
||||
expect(parseDiffGitHeader(lines[0])).toEqual({ oldPath: "before.ts", newPath: "after.ts" });
|
||||
expect(parseDiffMetadataPathLines(lines)).toEqual({ oldPath: "before.ts", newPath: "after.ts" });
|
||||
// Pierre's parser classifies renames off the similarity line — a patched
|
||||
// rename must carry a sub-100% score or it renders as a plain change.
|
||||
expect(lines[1]).toBe("similarity index 99%");
|
||||
});
|
||||
|
||||
test("pure rename (no patch field) emits a header-only section", () => {
|
||||
@@ -204,7 +207,9 @@ describe("reconstructGhPatch", () => {
|
||||
{ filename: "after.ts", previous_filename: "before.ts", status: "renamed" },
|
||||
]);
|
||||
|
||||
expect(patch).toBe("diff --git a/before.ts b/after.ts\nrename from before.ts\nrename to after.ts\n");
|
||||
expect(patch).toBe(
|
||||
"diff --git a/before.ts b/after.ts\nsimilarity index 100%\nrename from before.ts\nrename to after.ts\n",
|
||||
);
|
||||
});
|
||||
|
||||
test("entry without patch (binary / per-file too large) doesn't corrupt the next file's section", () => {
|
||||
@@ -272,6 +277,7 @@ describe("reconstructGhPatch", () => {
|
||||
{ filename: "copy.ts", previous_filename: "orig.ts", status: "copied", patch: "@@ -1 +1 @@\n-a\n+b" },
|
||||
]);
|
||||
|
||||
expect(patch).toContain("similarity index 99%");
|
||||
expect(patch).toContain("copy from orig.ts");
|
||||
expect(patch).toContain("copy to copy.ts");
|
||||
expect(patch.split("\n")[0]).toBe("diff --git a/orig.ts b/copy.ts");
|
||||
|
||||
@@ -103,10 +103,17 @@ export function reconstructGhPatch(files: GitHubFileEntry[]): string {
|
||||
const isDeleted = f.status === "removed";
|
||||
|
||||
let header = `diff --git ${headerPathToken("a", oldPath)} ${headerPathToken("b", newPath)}`;
|
||||
if (f.status === "renamed") {
|
||||
header += `\nrename from ${metadataPathToken(oldPath)}\nrename to ${metadataPathToken(newPath)}`;
|
||||
} else if (f.status === "copied") {
|
||||
header += `\ncopy from ${metadataPathToken(oldPath)}\ncopy to ${metadataPathToken(newPath)}`;
|
||||
if (f.status === "renamed" || f.status === "copied") {
|
||||
// Git always prints a similarity score before rename/copy lines, and
|
||||
// diff parsers (e.g. Pierre's) key rename classification off it —
|
||||
// without the line a rename renders as a plain change with no old path.
|
||||
// The files API doesn't expose the score: a patch-less entry is by
|
||||
// definition a 100% match; for patched entries emit a synthetic <100%
|
||||
// value (consumers only branch on 100% vs not).
|
||||
header += f.patch ? "\nsimilarity index 99%" : "\nsimilarity index 100%";
|
||||
header += f.status === "renamed"
|
||||
? `\nrename from ${metadataPathToken(oldPath)}\nrename to ${metadataPathToken(newPath)}`
|
||||
: `\ncopy from ${metadataPathToken(oldPath)}\ncopy to ${metadataPathToken(newPath)}`;
|
||||
}
|
||||
if (isNew) {
|
||||
header += "\nnew file mode 100644";
|
||||
|
||||
@@ -152,6 +152,38 @@ describe("fetchGlMR raw_diffs fallback", () => {
|
||||
expect(calls.some((c) => c.includes("/diffs?per_page=100"))).toBe(true);
|
||||
});
|
||||
|
||||
test("reconstructed renames carry a similarity line so parsers classify them as renames", async () => {
|
||||
const entries = JSON.stringify([
|
||||
{
|
||||
old_path: "src/old.ts",
|
||||
new_path: "src/new.ts",
|
||||
new_file: false,
|
||||
deleted_file: false,
|
||||
renamed_file: true,
|
||||
diff: "", // pure rename — GitLab sends an empty diff
|
||||
},
|
||||
{
|
||||
old_path: "src/before.ts",
|
||||
new_path: "src/after.ts",
|
||||
new_file: false,
|
||||
deleted_file: false,
|
||||
renamed_file: true,
|
||||
diff: "@@ -1 +1 @@\n-a\n+b\n",
|
||||
},
|
||||
]);
|
||||
const { runtime } = gitlabRuntime({
|
||||
rawDiffs: { exitCode: 1, stderr: "404 Not Found" },
|
||||
diffs: { exitCode: 0, stdout: entries },
|
||||
});
|
||||
const result = await fetchGlMR(runtime, REF);
|
||||
expect(result.rawPatch).toContain(
|
||||
"diff --git a/src/old.ts b/src/new.ts\nsimilarity index 100%\nrename from src/old.ts\nrename to src/new.ts",
|
||||
);
|
||||
expect(result.rawPatch).toContain(
|
||||
"diff --git a/src/before.ts b/src/after.ts\nsimilarity index 99%\nrename from src/before.ts\nrename to src/after.ts",
|
||||
);
|
||||
});
|
||||
|
||||
test("falls back when raw_diffs returns empty (oversized MR)", async () => {
|
||||
const { runtime, calls } = gitlabRuntime({
|
||||
rawDiffs: { exitCode: 0, stdout: "" },
|
||||
|
||||
@@ -63,6 +63,10 @@ function reconstructPatch(diffs: GitLabDiffEntry[]): string {
|
||||
|
||||
let header = `diff --git a/${displayOld} b/${displayNew}`;
|
||||
if (d.renamed_file) {
|
||||
// Diff parsers (e.g. Pierre's) key rename classification off the
|
||||
// similarity line; the API doesn't expose the score, so emit 100% for
|
||||
// pure renames (empty diff) and a synthetic <100% otherwise.
|
||||
header += d.diff.trim() === "" ? "\nsimilarity index 100%" : "\nsimilarity index 99%";
|
||||
header += `\nrename from ${d.old_path}\nrename to ${d.new_path}`;
|
||||
}
|
||||
if (d.new_file) {
|
||||
|
||||
Reference in New Issue
Block a user