mirror of
https://github.com/getpaseo/paseo.git
synced 2026-10-27 11:42:52 +00:00
fix(server): keep diff cancellation with its requester
Independent commit viewers must not cancel each other. Use request-source cancellation for commit highlighting and cache invalidation for checkout highlighting. Oversized source versions use the existing budgeted hunk reconstruction so large lockfiles retain useful syntax tokens.
This commit is contained in:
@@ -251,7 +251,10 @@ test("large source diffs stay renderable for reads, subscriptions, and commit fi
|
||||
expect(lines.filter((line) => line.type === "add").map((line) => line.content)).toEqual([
|
||||
`const value = Math.max(${value}, 7);`,
|
||||
]);
|
||||
expect(lines.every((line) => line.tokens === undefined)).toBe(true);
|
||||
expect(lines.find((line) => line.type === "add")?.tokens).toContainEqual({
|
||||
text: value,
|
||||
style: "number",
|
||||
});
|
||||
};
|
||||
check(initial.files[0], "43");
|
||||
const subscription = ctx.client.observeCheckoutDiff(cwd, { mode: "uncommitted" });
|
||||
|
||||
@@ -2964,7 +2964,7 @@ export class Session {
|
||||
case "checkout.commits.list.request":
|
||||
return this.checkoutSession.handleCommitsListRequest(msg);
|
||||
case "checkout.commits.file_diff.request":
|
||||
return this.checkoutSession.handleCommitFileDiffRequest(msg);
|
||||
return this.checkoutSession.handleCommitFileDiffRequest(msg, this.delivery.requestSignal);
|
||||
case "validate_branch_request":
|
||||
return this.checkoutSession.handleValidateBranchRequest(msg);
|
||||
case "branch_suggestions_request":
|
||||
|
||||
@@ -290,7 +290,10 @@ export class CheckoutSession {
|
||||
}
|
||||
}
|
||||
|
||||
async handleCommitFileDiffRequest(msg: CheckoutCommitFileDiffRequest): Promise<void> {
|
||||
async handleCommitFileDiffRequest(
|
||||
msg: CheckoutCommitFileDiffRequest,
|
||||
signal: AbortSignal,
|
||||
): Promise<void> {
|
||||
const { cwd, sha, path, requestId } = msg;
|
||||
|
||||
try {
|
||||
@@ -298,7 +301,7 @@ export class CheckoutSession {
|
||||
if (path.length === 0 || isAbsolute(path) || path.split(/[\\/]/).includes("..")) {
|
||||
throw new Error(`Invalid path: ${path}`);
|
||||
}
|
||||
const file = await getCommitFileDiff({ cwd: expandTilde(cwd), sha, path });
|
||||
const file = await getCommitFileDiff({ cwd: expandTilde(cwd), sha, path, signal });
|
||||
this.host.emit({
|
||||
type: "checkout.commits.file_diff.response",
|
||||
payload: { cwd, sha, path, file, error: null, requestId },
|
||||
|
||||
@@ -4,9 +4,9 @@ import { join } from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { setImmediate } from "node:timers/promises";
|
||||
import { performance } from "node:perf_hooks";
|
||||
import { highlightCode } from "@getpaseo/highlight";
|
||||
import { SessionDelivery } from "../session/owned-subscriptions/index.js";
|
||||
import { highlightCode, highlightCodeWithBudget } from "@getpaseo/highlight";
|
||||
import {
|
||||
DiffHighlightRequests,
|
||||
parseDiff,
|
||||
reconstructNewFile,
|
||||
reconstructOldFile,
|
||||
@@ -826,7 +826,7 @@ index 1111111..2222222 100644
|
||||
|
||||
describe("diff highlighting budget", () => {
|
||||
it.each(["supplied", "local"] as const)(
|
||||
"keeps a one-line diff on a 5 MiB %s source responsive and renders it without tokens",
|
||||
"keeps a one-line diff on a 5 MiB %s source responsive using hunk tokens",
|
||||
async (source) => {
|
||||
const cwd = mkdtempSync(join(tmpdir(), "diff-highlight-budget-"));
|
||||
const oldFileContent = "const value = Math.max(42, 7);\n".repeat(
|
||||
@@ -857,7 +857,7 @@ describe("diff highlighting budget", () => {
|
||||
await new Promise((resolve) => setTimeout(resolve, 5));
|
||||
console.info({ sourceBytes: Buffer.byteLength(oldFileContent), longestGap });
|
||||
expect(longestGap).toBeLessThan(50);
|
||||
expect(highlighted).toEqual(file);
|
||||
expect(highlighted).toEqual(highlightDiffFromHunks(file));
|
||||
} finally {
|
||||
clearInterval(timer);
|
||||
rmSync(cwd, { recursive: true, force: true });
|
||||
@@ -888,27 +888,29 @@ describe("diff highlighting budget", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("a newer request cancels the whole previous diff even before its sources arrive", async () => {
|
||||
const requests = new DiffHighlightRequests();
|
||||
const file = parseDiff(SIMPLE_DIFF)[0];
|
||||
const source = "const value = Math.max(42, 7);\n".repeat(3_000);
|
||||
const old = requests.run("checkout-target", async (signal) => {
|
||||
await setImmediate();
|
||||
return highlightDiffWithFileContent(file, ".", {
|
||||
oldFileContent: source,
|
||||
newFileContent: source,
|
||||
signal,
|
||||
});
|
||||
it("disconnecting the requesting source stops a parser between highlight steps", async () => {
|
||||
const delivery = new SessionDelivery(() => {});
|
||||
const source = {};
|
||||
delivery.attach(source, true);
|
||||
const yielded = Promise.withResolvers<void>();
|
||||
let result: Awaited<ReturnType<typeof highlightCodeWithBudget>> = [];
|
||||
const pending = delivery.request(source, { type: "ping", requestId: "highlight" }, async () => {
|
||||
result = await highlightCodeWithBudget(
|
||||
"const value = Math.max(42, 7);\n".repeat(3_000),
|
||||
"file.ts",
|
||||
{
|
||||
signal: delivery.requestSignal,
|
||||
yieldToEventLoop: () => {
|
||||
yielded.resolve();
|
||||
return setImmediate();
|
||||
},
|
||||
},
|
||||
);
|
||||
});
|
||||
const current = requests.run("checkout-target", (signal) =>
|
||||
highlightDiffWithFileContent(file, ".", {
|
||||
oldFileContent: "const bar = 2;",
|
||||
newFileContent: "const bar = 3;",
|
||||
signal,
|
||||
}),
|
||||
);
|
||||
expect(await old).toEqual(file);
|
||||
expect((await current).hunks[0].lines.some((line) => line.tokens)).toBe(true);
|
||||
await yielded.promise;
|
||||
await delivery.detach(source);
|
||||
await pending;
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it("keeps real session.ts diffs highlighted and byte-identical under the source budget", async () => {
|
||||
@@ -938,3 +940,35 @@ it("keeps real session.ts diffs highlighted and byte-identical under the source
|
||||
rmSync(cwd, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("highlights a one-line hunk from an over-budget JSON source without blocking the loop", async () => {
|
||||
const cwd = mkdtempSync(join(tmpdir(), "diff-highlight-large-json-"));
|
||||
const oldFileContent = `{\n${' "value": 42,\n'.repeat(100_000)} "last": true\n}`;
|
||||
const newFileContent = oldFileContent.replace("42", "43");
|
||||
writeFileSync(join(cwd, "package-lock.json"), newFileContent);
|
||||
const [file] = parseDiff(`diff --git a/package-lock.json b/package-lock.json
|
||||
--- a/package-lock.json
|
||||
+++ b/package-lock.json
|
||||
@@ -2 +2 @@
|
||||
- "value": 42,
|
||||
+ "value": 43,`);
|
||||
let last = performance.now(),
|
||||
longestGap = 0;
|
||||
const timer = setInterval(() => {
|
||||
const now = performance.now();
|
||||
longestGap = Math.max(longestGap, now - last);
|
||||
last = now;
|
||||
}, 1);
|
||||
try {
|
||||
const result = await highlightDiffWithFileContent(file, cwd, { oldFileContent });
|
||||
await new Promise((resolve) => setTimeout(resolve, 5));
|
||||
expect(longestGap).toBeLessThan(50);
|
||||
expect(result.hunks[0].lines.find((line) => line.type === "add")?.tokens).toContainEqual({
|
||||
text: "43",
|
||||
style: "number",
|
||||
});
|
||||
} finally {
|
||||
clearInterval(timer);
|
||||
rmSync(cwd, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
@@ -49,29 +49,6 @@ interface ParseAndHighlightDiffOptions {
|
||||
getNewFileContent?: (file: ParsedDiffFile) => Promise<string | null>;
|
||||
}
|
||||
|
||||
/** Owns supersession for a whole diff, including sources that arrive after cancellation. */
|
||||
export class DiffHighlightRequests {
|
||||
private readonly active = new Map<string, AbortController>();
|
||||
|
||||
async run<T>(
|
||||
target: string,
|
||||
operation: (signal: AbortSignal) => Promise<T>,
|
||||
parentSignal?: AbortSignal,
|
||||
): Promise<T> {
|
||||
this.active.get(target)?.abort();
|
||||
const controller = new AbortController();
|
||||
this.active.set(target, controller);
|
||||
const signal = parentSignal
|
||||
? AbortSignal.any([parentSignal, controller.signal])
|
||||
: controller.signal;
|
||||
try {
|
||||
return await operation(signal);
|
||||
} finally {
|
||||
if (this.active.get(target) === controller) this.active.delete(target);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Parse a unified diff into structured data
|
||||
*/
|
||||
@@ -315,23 +292,23 @@ async function buildFullFileTokenLookup(
|
||||
return new Map(highlighted.map((tokens, index) => [index + 1, tokens]));
|
||||
}
|
||||
|
||||
// null means over budget; undefined means unavailable and permits reconstruction.
|
||||
async function readHighlightSource(path: string): Promise<string | null | undefined> {
|
||||
async function readHighlightSource(path: string): Promise<string | null> {
|
||||
let handle;
|
||||
try {
|
||||
handle = await open(path, constants.O_RDONLY | constants.O_NONBLOCK);
|
||||
const stat = await handle.stat();
|
||||
if (!stat.isFile() || stat.size > HIGHLIGHT_MAX_SOURCE_BYTES) return null;
|
||||
const buffer = Buffer.alloc(HIGHLIGHT_MAX_SOURCE_BYTES + 1);
|
||||
const buffer = Buffer.alloc(Math.min(stat.size, HIGHLIGHT_MAX_SOURCE_BYTES) + 1);
|
||||
let offset = 0;
|
||||
while (offset < buffer.length) {
|
||||
const { bytesRead } = await handle.read(buffer, offset, buffer.length - offset, null);
|
||||
if (bytesRead === 0) break;
|
||||
offset += bytesRead;
|
||||
}
|
||||
return offset > HIGHLIGHT_MAX_SOURCE_BYTES ? null : buffer.toString("utf8", 0, offset);
|
||||
// A full buffer includes the extra byte: the source grew after stat.
|
||||
return offset === buffer.length ? null : buffer.toString("utf8", 0, offset);
|
||||
} catch {
|
||||
return undefined;
|
||||
return null;
|
||||
} finally {
|
||||
await handle?.close();
|
||||
}
|
||||
@@ -418,16 +395,16 @@ export async function highlightDiffWithFileContent(
|
||||
typeof options.newFileContent === "string"
|
||||
? options.newFileContent
|
||||
: await readHighlightSource(resolve(cwd, file.path));
|
||||
if (newContent === null || options.signal?.aborted) return file;
|
||||
if (options.signal?.aborted) return file;
|
||||
|
||||
const oldTokens = await highlightAvailableSource(
|
||||
options.oldFileContent,
|
||||
file.path,
|
||||
options.signal,
|
||||
);
|
||||
if (oldTokens === null || options.signal?.aborted) return file;
|
||||
if (options.signal?.aborted) return file;
|
||||
const newTokens = await highlightAvailableSource(newContent, file.path, options.signal);
|
||||
if (newTokens === null || options.signal?.aborted) return file;
|
||||
if (options.signal?.aborted) return file;
|
||||
if (oldTokens && newTokens) return applyTokensToHunks(file, newTokens, oldTokens);
|
||||
const reconstructed = await buildBudgetedReconstructedTokens(file, options.signal);
|
||||
if (!reconstructed || options.signal?.aborted) return file;
|
||||
@@ -442,23 +419,17 @@ function isDiffHighlightEligible(
|
||||
file: ParsedDiffFile,
|
||||
options: HighlightDiffWithFileContentOptions,
|
||||
): boolean {
|
||||
return (
|
||||
!options.signal?.aborted &&
|
||||
isLanguageSupported(file.path) &&
|
||||
!hasOversizedDiffLine(file) &&
|
||||
![options.oldFileContent, options.newFileContent].some(
|
||||
(content) => typeof content === "string" && exceedsSourceBudget(content),
|
||||
)
|
||||
);
|
||||
return !options.signal?.aborted && isLanguageSupported(file.path) && !hasOversizedDiffLine(file);
|
||||
}
|
||||
|
||||
async function highlightAvailableSource(
|
||||
content: string | null | undefined,
|
||||
path: string,
|
||||
signal?: AbortSignal,
|
||||
): Promise<Map<number, HighlightToken[]> | null | undefined> {
|
||||
): Promise<Map<number, HighlightToken[]> | null> {
|
||||
// Preserve the existing hunk-context fallback for oversized individual lines.
|
||||
if (typeof content !== "string" || hasOversizedLine(content)) return undefined;
|
||||
if (typeof content !== "string" || exceedsSourceBudget(content) || hasOversizedLine(content))
|
||||
return null;
|
||||
return buildFullFileTokenLookup(content, path, signal);
|
||||
}
|
||||
|
||||
|
||||
@@ -4,6 +4,7 @@ import { tmpdir } from "os";
|
||||
import { join } from "path";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { getCommitFileDiff, listCheckoutCommits } from "./checkout-git.js";
|
||||
import { SessionDelivery } from "../server/session/owned-subscriptions/index.js";
|
||||
import { startGitCommandMetrics, stopGitCommandMetrics } from "./run-git-command.js";
|
||||
|
||||
const tempDirs: string[] = [];
|
||||
@@ -302,3 +303,44 @@ describe("getCommitFileDiff", () => {
|
||||
expect(file?.deletions).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
it("identical commit-file requests from two requesters both retain syntax tokens", async () => {
|
||||
const repoDir = initRepo();
|
||||
const original = "const value = Math.max(42, 7);\n".repeat(3_000);
|
||||
commitFile(repoDir, "file.ts", original, "initial");
|
||||
commitFile(repoDir, "file.ts", original.replace("42", "43"), "edit");
|
||||
const input = { cwd: repoDir, sha: headSha(repoDir), path: "file.ts" };
|
||||
const results = await Promise.all([getCommitFileDiff(input), getCommitFileDiff(input)]);
|
||||
for (const file of results) {
|
||||
expect(
|
||||
file?.hunks[0].lines
|
||||
.find((line) => line.type === "add")
|
||||
?.tokens?.some((token) => token.style !== null),
|
||||
).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it("a disconnected requester cancels commit highlighting before its sources arrive", async () => {
|
||||
const repoDir = initRepo();
|
||||
commitFile(repoDir, "file.ts", "const value = 42;\n", "initial");
|
||||
commitFile(repoDir, "file.ts", "const value = 43;\n", "edit");
|
||||
const delivery = new SessionDelivery(() => {});
|
||||
const source = {};
|
||||
delivery.attach(source, true);
|
||||
const request = {
|
||||
type: "checkout.commits.file_diff.request" as const,
|
||||
cwd: repoDir,
|
||||
sha: headSha(repoDir),
|
||||
path: "file.ts",
|
||||
requestId: "disconnect",
|
||||
};
|
||||
const results: Awaited<ReturnType<typeof getCommitFileDiff>>[] = [];
|
||||
const pending = delivery.request(source, request, async () => {
|
||||
results.push(await getCommitFileDiff({ ...request, signal: delivery.requestSignal }));
|
||||
});
|
||||
await delivery.detach(source);
|
||||
await pending;
|
||||
expect(
|
||||
results[0]?.hunks.flatMap((hunk) => hunk.lines).every((line) => line.tokens === undefined),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
@@ -11,7 +11,6 @@ import { maxSecuredPlaintextByteLength } from "@getpaseo/relay";
|
||||
import type { Logger } from "pino";
|
||||
import type { ParsedDiffFile } from "../server/utils/diff-highlighter.js";
|
||||
import {
|
||||
DiffHighlightRequests,
|
||||
highlightDiffWithFileContent,
|
||||
parseAndHighlightDiff,
|
||||
parseDiff,
|
||||
@@ -2704,8 +2703,6 @@ async function readCommitRenames(cwd: string, sha: string): Promise<ReadonlyMap<
|
||||
return renames;
|
||||
}
|
||||
|
||||
const diffHighlightRequests = new DiffHighlightRequests();
|
||||
|
||||
/**
|
||||
* Fetches the unified diff of a single file as introduced by one commit and
|
||||
* parses it into the same {@link ParsedDiffFile} shape the diff subscription
|
||||
@@ -2718,29 +2715,17 @@ const diffHighlightRequests = new DiffHighlightRequests();
|
||||
* binary-only (no textual hunks). Throws on git failure (e.g. an unknown sha),
|
||||
* which the caller maps to a typed checkout error.
|
||||
*/
|
||||
export function getCommitFileDiff(input: {
|
||||
export async function getCommitFileDiff({
|
||||
cwd,
|
||||
sha,
|
||||
path,
|
||||
signal,
|
||||
}: {
|
||||
cwd: string;
|
||||
sha: string;
|
||||
path: string;
|
||||
signal?: AbortSignal;
|
||||
}): Promise<ParsedDiffFile | null> {
|
||||
return diffHighlightRequests.run(
|
||||
JSON.stringify(["commit", resolve(input.cwd), input.sha, input.path]),
|
||||
(signal) => readCommitFileDiff(input, signal),
|
||||
);
|
||||
}
|
||||
|
||||
async function readCommitFileDiff(
|
||||
{
|
||||
cwd,
|
||||
sha,
|
||||
path,
|
||||
}: {
|
||||
cwd: string;
|
||||
sha: string;
|
||||
path: string;
|
||||
},
|
||||
signal: AbortSignal,
|
||||
): Promise<ParsedDiffFile | null> {
|
||||
const worktreeRoot = await requireGitWorktreeRoot(cwd);
|
||||
const renames = await getCommitRenames(worktreeRoot, sha);
|
||||
const oldPath = renames.get(path);
|
||||
@@ -3150,7 +3135,7 @@ export function warmCheckoutShortstatInBackground(
|
||||
}
|
||||
|
||||
interface AppendStructuredTrackedDiffsInput {
|
||||
signal: AbortSignal;
|
||||
signal?: AbortSignal;
|
||||
cwd: string;
|
||||
trackedChanges: CheckoutFileChange[];
|
||||
trackedNumstatByPath: Map<string, FileStat>;
|
||||
@@ -3166,7 +3151,7 @@ interface AppendStructuredTrackedDiffsInput {
|
||||
}
|
||||
|
||||
async function buildHighlightedTrackedDiffFile(input: {
|
||||
signal: AbortSignal;
|
||||
signal?: AbortSignal;
|
||||
cwd: string;
|
||||
change: CheckoutFileChange;
|
||||
parsedFile: ParsedDiffFile;
|
||||
@@ -3317,7 +3302,7 @@ async function appendStructuredTrackedDiffs(
|
||||
}
|
||||
|
||||
interface ProcessUntrackedChangeInput {
|
||||
signal: AbortSignal;
|
||||
signal?: AbortSignal;
|
||||
cwd: string;
|
||||
change: CheckoutFileChange;
|
||||
ignoreWhitespace: boolean;
|
||||
@@ -3560,31 +3545,11 @@ async function prepareAllChangesIndex(cwd: string, baseRef: string) {
|
||||
}
|
||||
}
|
||||
|
||||
export function getCheckoutDiff(
|
||||
export async function getCheckoutDiff(
|
||||
cwd: string,
|
||||
compare: CheckoutDiffCompare,
|
||||
context?: CheckoutContext,
|
||||
signal?: AbortSignal,
|
||||
): Promise<CheckoutDiffResult> {
|
||||
return diffHighlightRequests.run(
|
||||
JSON.stringify([
|
||||
"checkout",
|
||||
resolve(cwd),
|
||||
compare.mode,
|
||||
compare.mode !== "uncommitted" ? (compare.baseRef ?? null) : null,
|
||||
compare.ignoreWhitespace === true,
|
||||
compare.includeStructured === true,
|
||||
]),
|
||||
(highlightSignal) => loadCheckoutDiff(cwd, compare, context, highlightSignal),
|
||||
signal,
|
||||
);
|
||||
}
|
||||
|
||||
async function loadCheckoutDiff(
|
||||
cwd: string,
|
||||
compare: CheckoutDiffCompare,
|
||||
context: CheckoutContext | undefined,
|
||||
signal: AbortSignal,
|
||||
): Promise<CheckoutDiffResult> {
|
||||
cwd = await requireGitWorktreeRoot(cwd);
|
||||
|
||||
@@ -3614,7 +3579,7 @@ async function readCheckoutDiff(
|
||||
cwd: string,
|
||||
compare: CheckoutDiffCompare,
|
||||
refsForDiff: CheckoutDiffRefs,
|
||||
signal: AbortSignal,
|
||||
signal?: AbortSignal,
|
||||
): Promise<CheckoutDiffResult> {
|
||||
const ignoreWhitespace = compare.ignoreWhitespace === true;
|
||||
let effectiveRefsForDiff = refsForDiff;
|
||||
|
||||
Reference in New Issue
Block a user