mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
6ec1a66c9b
* feat(review): large GitHub PR fallback + non-blocking PR checkout
Two PR-mode improvements:
1. Large GitHub PRs no longer fail to load. When `gh pr diff` is refused
(HTTP 406 for oversized diffs), fetchGhPR pages through the pulls files
API and stitches the per-file patches into a unified diff — mirroring
the existing GitLab raw_diffs fallback. Path quoting matches git's
exact rules (bare spaces unquoted) so downstream parsers round-trip;
truncation at the API's 3000-file cap is surfaced, never silent.
2. The --local worktree/clone no longer blocks startup. The review server
opens as soon as the platform diff arrives; the checkout warms in the
background as a seeded not-ready pool entry. Consumers that need real
files (agent jobs, full-stack diff, code-nav, semantic diff, AI
sessions) await pool.ensure(), with creations serialized so concurrent
fetches can't clobber the shared FETCH_HEAD. Cross-repo clone steps
converted from spawnSync to async spawns; warmup children are killed
on exit (plus `git worktree prune`) so aborted sessions can't leak
stale registrations; failed checkouts degrade honestly (no agent runs
in the wrong directory claiming local access) with a 30s retry
cooldown.
* 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).
* feat(review): local full-diff upgrade for PRs whose API diff is truncated
On oversized PRs the platform APIs withhold per-file patch content entirely
(bun#30412: 1,066 of 2,188 files came back with status added/modified, zeroed
counts, and no patch). Those files rendered as empty stubs with no diff.
- fetchGhPR/fetchGlMR flag the result `patchIncomplete` when patch-less
non-rename entries exist or the 3000-file cap truncates the listing.
- New runPRLayerLocalDiff (pr-stack.ts) recomputes the exact layer diff in
the local checkout: platform merge-base + head SHA two-dot diff (three-dot
vs baseSha fallback), fetch-by-SHA for objects missing from shallow clones,
-l0 so rename detection doesn't silently degrade on huge PRs.
- The review UI shows a "Partial diff · Load full diff" notice in layer
scope; clicking re-requests the layer scope and the server swaps in the
recomputed full diff (waiting out the background clone if needed).
- PR scope/switch state writes are epoch-guarded: a request parked on the
checkout warmup can no longer overwrite a newer scope select or pr-switch.
- draftKey follows the upgraded patch so annotation drafts survive pr-switch
round-trips; recompute failures surface in the response error field.
- Pi server mirrors all of it, including an agentCwd fallback so the upgrade
works for PRs switched-to under a cross-repo clone pool.
* fix(review): use GitLab's too_large/collapsed flags for withheld-diff detection
External review caught a false negative: a too-large ADDED file comes back
new_file:true with an empty diff — indistinguishable from a legitimately
empty new file under the old heuristic, so the partial-diff upgrade was
never offered for exactly the files that matter most on big MRs.
The REST /diffs endpoint marks withheld content explicitly per entry
(verified against gitlab.com): too_large/collapsed are now authoritative in
both directions — withheld adds/deletes are flagged, binaries and empty
files are never misflagged. Older GitLab without the fields keeps the
empty-diff-on-modification heuristic.
* feat(prompts): unify review-denied suffix — triage first, no coding off raw feedback
The per-runtime defaults map (#627) gave OpenCode and Pi a different
review-denied suffix than every other runtime; updating one meant the
others silently kept "you must address all of them" — an instruction to
start coding immediately. Claude Code, Amp, Droid, Codex, Copilot, Gemini,
and Kiro were all still on it.
One default for every runtime now: triage the feedback, verify it against
the code, discuss before changing anything. Per-runtime customization
remains available via config (prompts.review.runtimes.<rt>.denied), which
resolves above the built-in default as before.
* fix(prompts): generalize review-denied suffix — 'from review', not 'external AI reviewers'
Review feedback isn't always from AI reviewers or agent jobs; often it's
the human reviewer's own annotations. Neutral wording covers both.
* fix(review): non-blocking 'Load full diff' + flag-handling hardenings
Self-review findings:
- The partial-diff upgrade reused the scope-switch handler, so clicking
"Load full diff" raised the full-screen PRSwitchOverlay — blocking the
entire UI, potentially for minutes behind a cold clone, with no text and
no cancel. The upgrade now has its own loading state: the notice shows a
spinner ("Loading full diff…") and the reviewer keeps working with the
partial diff while the request parks. Server-side epoch guards already
handle scope/PR changes made during the wait.
- GitLab too_large/collapsed: treat explicit null like absent (flags
inconclusive → legacy heuristic decides) instead of silently exonerating.
- Rename-limit lift uses -l100000 instead of -l0 ("0 = unlimited" only
holds on git >= 2.29; on older git it could disable detection outright).
* fix(review): stop scroll-driven sem stampede when semantic diff is failing
The badge retry change (a2d19a4e) cleared the client-side sem cache on
failure so transient errors could recover. But file-header badges mount and
unmount on every scroll in the virtualized all-files view, and each mount
re-requests /api/semantic-diff — and the server only cached SUCCESSFUL runs.
With sem erroring, scrolling spawned a continuous stream of sem processes,
pegging the CPU and making scrolling severely choppy.
Bound retry rate by time, not by mount events:
- client: keep the failed result memoized and expire it after a 60s
cooldown instead of clearing immediately
- server (Bun + Pi): memoize failed sem runs for 30s in
SemanticDiffResponseCache — request rate can no longer drive execution
rate
* fix(review): eliminate all-files scroll jank (pre-existing on main, from #885)
The CodeView migration introduced severe scroll chop; scrolling UP could
freeze the viewport entirely ("scrolling but nothing changes"). Three
compounding causes, diagnosed against Pierre 1.2.8 source:
1. Lazy full-content augmentation landed updateItem() mid-scroll-gesture:
the full-content parse counts collapsed-context regions the raw-patch
parse doesn't, so the item GROWS — re-render + re-tokenize hitches both
directions, and when the grown item sat above CodeView's scroll anchor,
its corrective scrollTo() killed wheel momentum (the up-scroll freeze).
Fetches still start as items enter the window; the item mutation now
waits for 150ms of scroll quiet (staleness re-checked at apply time).
2. reportVisibleFile read container.scrollTop/clientHeight/scrollHeight on
EVERY scroll event — a forced synchronous layout right after each
frame's DOM writes. Replaced with CodeView's cached accessors and
coalesced the handler to once per animation frame.
3. Missing containment CSS: Pierre's own production wrapper uses
contain:strict + will-change:scroll-position so forced layouts stay
scoped to the scroller instead of the whole document. Adopted.
Also: __devOnlyValidateItemHeights now requires explicit opt-in
(VITE_PIERRE_VALIDATE_HEIGHTS=1) — it runs getBoundingClientRect() per
rendered item per frame and made dev-server scrolling choppy by itself.
* feat(review): change-type status in headers + tree, diffshub CSS parity
Adopts two diffshub practices identified in the architecture comparison:
- DiffFile now carries a derived status (added/deleted/renamed/modified)
from the chunk's git metadata lines. FileHeader shows a status icon and
renders renames as "old/path → new/path" (dimmed old, arrow — diffshub's
treatment, including its rename blue); the file tree shows A/D/R markers.
'modified' is deliberately undecorated so the others pop. Works in both
the all-files surface and the single-file panel, including header-only
pure renames from the large-PR reconstruction.
- CodeView container gains diffshub's remaining perf CSS: overflow-anchor:
none (native scroll anchoring fights CodeView's own anchor resolution
whenever item heights change — exactly our augmentation applies),
overflow-x-clip, and overflow-clip containment on item elements.
* feat(review): worker-pool syntax highlighting (diffshub parity)
A performance trace of scrolling a small local diff attributed 2.2s of
2.6s main-thread CPU to findNextMatchSync — shiki's TextMate regex
scanner tokenizing on the main thread. diffshub avoids this entirely by
running tokenization in Pierre's worker pool; we never opted in.
Wires WorkerPoolContextProvider around the review app (pool size
min(cores-1, 3), 100-entry AST LRU, common languages preloaded), gates
the all-files surface on pool readiness with a 5s escape hatch (a dead
pool degrades to plaintext-then-highlight, never a blank view), and
syncs the UI theme pair into the long-lived pool.
Single-file build constraint solved with Vite's ?worker&inline (base64
blob worker) + worker.format 'es' with inlineDynamicImports — the
worker's lazy import("shiki/wasm") branch collapses into the bundle and
is never taken (shiki-js engine: the win is moving work off the main
thread, with no .wasm asset to smuggle into one HTML file). Bundle
+850KB.
* fix(review): un-poison worker-pool theme dedup on failed setRenderOptions
A failed round-trip recorded the theme as synced and never retried,
pinning the pool to the wrong palette for the session.
* fix(review): report partial diffs without a checkout; fail fast on missing checkout
Dogfood review of this PR (via plannotator itself) caught two valid issues:
- prPatchIncomplete was gated on the worktree pool, so a --no-local session
showed a truncated diff with no indication at all. Partiality is
information; upgradability is a capability. The flag is now always
reported, with a separate prPatchUpgradeAvailable — the UI shows the
amber notice either way, with the "Load full diff" button only when a
checkout can exist (otherwise a "re-run with --local" hint).
- After a FAILED checkout warmup, Ask AI sessions and agent jobs fell back
to process.cwd() (or a wrong revision on Pi) — running in the wrong tree
instead of failing. Both launch points now refuse with a clear "Local
PR checkout unavailable — retry shortly" error (503); the job handlers
surface buildCommand refusals instead of mislabeling them "Invalid
JSON". Bun and Pi mirrored.
A third finding (sem availability stuck after warmup) was triaged invalid:
the availability probe detects the sem binary, which is cwd-independent.
* fix(review): runtime-neutral copy for the no-checkout partial-diff hint
--local is a CLI remedy; OpenCode sessions have no such flag. Visible
text states the fact, the tooltip carries the CLI guidance.
245 lines
8.5 KiB
TypeScript
245 lines
8.5 KiB
TypeScript
import { describe, it, expect } from "bun:test";
|
|
import { buildFileTree, getAncestorPaths, getAllFolderPaths } from "./buildFileTree";
|
|
import type { DiffFile } from "../types";
|
|
|
|
const diffFile = (path: string, overrides: Partial<DiffFile> = {}): DiffFile => ({
|
|
path,
|
|
patch: "",
|
|
additions: 0,
|
|
deletions: 0,
|
|
status: "modified",
|
|
...overrides,
|
|
});
|
|
|
|
describe("buildFileTree - workspace mode with repo-prefixed paths", () => {
|
|
it("builds separate trees for different repo prefixes", () => {
|
|
const files = [
|
|
diffFile("repo-a/src/index.ts"),
|
|
diffFile("repo-b/src/index.ts"),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
|
|
// With flat fallback: single root folder with only file children gets unwrapped
|
|
// But here we have two repos at root level, so they stay as folders
|
|
expect(tree.length).toBeGreaterThanOrEqual(2);
|
|
// After collapseSingleChild, paths like repo-a/src/index.ts become:
|
|
// folder: "repo-a/src" with file child "index.ts"
|
|
const names = tree.map(n => n.name);
|
|
expect(names).toContain("repo-a/src");
|
|
expect(names).toContain("repo-b/src");
|
|
});
|
|
|
|
it("handles same relative paths in different repos", () => {
|
|
const files = [
|
|
diffFile("repo-a/src/utils/helper.ts", { additions: 5, deletions: 2 }),
|
|
diffFile("repo-b/src/utils/helper.ts", { additions: 3, deletions: 1 }),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
|
|
// After collapseSingleChild: repo-a/src/utils becomes a single folder node
|
|
const repoA = tree.find(n => n.name === "repo-a/src/utils");
|
|
const repoB = tree.find(n => n.name === "repo-b/src/utils");
|
|
|
|
expect(repoA).toBeDefined();
|
|
expect(repoB).toBeDefined();
|
|
|
|
// Each should have the helper.ts file as a child
|
|
const repoAFile = repoA?.children?.find(n => n.name === "helper.ts");
|
|
const repoBFile = repoB?.children?.find(n => n.name === "helper.ts");
|
|
|
|
expect(repoAFile).toBeDefined();
|
|
expect(repoBFile).toBeDefined();
|
|
expect(repoAFile?.path).toBe("repo-a/src/utils/helper.ts");
|
|
expect(repoBFile?.path).toBe("repo-b/src/utils/helper.ts");
|
|
expect(repoAFile?.additions).toBe(5);
|
|
expect(repoBFile?.additions).toBe(3);
|
|
});
|
|
|
|
it("handles nested repo labels (longest prefix)", () => {
|
|
// Simulates repos like "apps", "apps/api", "apps/web"
|
|
const files = [
|
|
diffFile("apps/src/main.ts"),
|
|
diffFile("apps/api/src/server.ts"),
|
|
diffFile("apps/web/src/app.ts"),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
|
|
// All under single "apps" root, with children for each sub-repo
|
|
// After collapseSingleChild: api/src and web/src collapse
|
|
expect(tree).toHaveLength(1);
|
|
expect(tree[0].name).toBe("apps");
|
|
expect(tree[0].type).toBe("folder");
|
|
|
|
// Children: "api/src" (collapsed), "src" (from apps/src), "web/src" (collapsed)
|
|
const childNames = tree[0].children?.map(n => n.name).sort();
|
|
expect(childNames).toEqual(["api/src", "src", "web/src"]);
|
|
});
|
|
|
|
it("handles deeply nested repo labels", () => {
|
|
const files = [
|
|
diffFile("packages/shared/utils/helpers/string.ts"),
|
|
diffFile("packages/core/src/index.ts"),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
|
|
expect(tree).toHaveLength(1);
|
|
expect(tree[0].name).toBe("packages");
|
|
expect(tree[0].type).toBe("folder");
|
|
|
|
// After collapseSingleChild, packages/core/src collapses to "core/src"
|
|
// and packages/shared/utils/helpers collapses to "shared/utils/helpers"
|
|
const children = tree[0].children?.map(n => n.name).sort();
|
|
expect(children).toEqual(["core/src", "shared/utils/helpers"]);
|
|
});
|
|
|
|
it("collapses single-child folders correctly with repo prefixes", () => {
|
|
const files = [
|
|
diffFile("repo-a/src/components/Button.tsx"),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
|
|
// After collapseSingleChild: repo-a/src/components collapses to single path
|
|
// Then flat fallback kicks in: single folder with only file children gets unwrapped
|
|
// Result: just the file at root level
|
|
expect(tree).toHaveLength(1);
|
|
expect(tree[0].name).toBe("Button.tsx");
|
|
expect(tree[0].type).toBe("file");
|
|
expect(tree[0].path).toBe("repo-a/src/components/Button.tsx");
|
|
});
|
|
|
|
it("aggregates stats correctly across repo boundaries", () => {
|
|
const files = [
|
|
diffFile("repo-a/src/index.ts", { additions: 10, deletions: 5 }),
|
|
diffFile("repo-a/src/utils.ts", { additions: 5, deletions: 2 }),
|
|
diffFile("repo-b/src/index.ts", { additions: 8, deletions: 3 }),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
|
|
// After collapseSingleChild: repo-a/src contains both files
|
|
const repoA = tree.find(n => n.name === "repo-a/src");
|
|
const repoB = tree.find(n => n.name === "repo-b/src");
|
|
|
|
expect(repoA?.additions).toBe(15); // 10 + 5
|
|
expect(repoA?.deletions).toBe(7); // 5 + 2
|
|
expect(repoB?.additions).toBe(8);
|
|
expect(repoB?.deletions).toBe(3);
|
|
});
|
|
|
|
it("handles repo labels with special characters", () => {
|
|
const files = [
|
|
diffFile("my-repo_2.0/src/index.ts"),
|
|
diffFile("my-repo_2.0-beta/src/app.ts"),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
|
|
// Two separate root-level folders after collapse
|
|
expect(tree.length).toBeGreaterThanOrEqual(2);
|
|
const names = tree.map(n => n.name);
|
|
expect(names).toContain("my-repo_2.0/src");
|
|
expect(names).toContain("my-repo_2.0-beta/src");
|
|
});
|
|
|
|
it("preserves full prefixed path in node path property", () => {
|
|
const files = [
|
|
diffFile("owner/repo/src/index.ts"),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
|
|
// Collapses to "owner/repo/src" folder, then flat fallback unwraps
|
|
// Result: just the file with full path preserved
|
|
expect(tree[0].name).toBe("index.ts");
|
|
expect(tree[0].path).toBe("owner/repo/src/index.ts");
|
|
});
|
|
|
|
it("handles empty file list", () => {
|
|
const tree = buildFileTree([]);
|
|
expect(tree).toHaveLength(0);
|
|
});
|
|
|
|
it("handles single file in repo (flat fallback)", () => {
|
|
const files = [diffFile("repo-a/README.md")];
|
|
const tree = buildFileTree(files);
|
|
|
|
// Flat fallback: single root folder with only file children gets unwrapped
|
|
// But first collapseSingleChild collapses repo-a to contain README.md
|
|
// Then flat fallback sees single folder "repo-a" with file child, unwraps it
|
|
expect(tree).toHaveLength(1);
|
|
expect(tree[0].name).toBe("README.md");
|
|
expect(tree[0].type).toBe("file");
|
|
expect(tree[0].path).toBe("repo-a/README.md");
|
|
});
|
|
|
|
it("handles multiple files in same repo subdirectories", () => {
|
|
const files = [
|
|
diffFile("repo-a/src/index.ts"),
|
|
diffFile("repo-a/src/app.ts"),
|
|
diffFile("repo-a/lib/helpers.ts"),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
|
|
// repo-a has two children: src (with 2 files) and lib (with 1 file)
|
|
// So it doesn't get unwrapped by flat fallback
|
|
expect(tree).toHaveLength(1);
|
|
expect(tree[0].name).toBe("repo-a");
|
|
expect(tree[0].type).toBe("folder");
|
|
|
|
const children = tree[0].children?.map(n => n.name).sort();
|
|
expect(children).toEqual(["lib", "src"]);
|
|
});
|
|
});
|
|
|
|
describe("getAncestorPaths - workspace mode", () => {
|
|
it("returns ancestor paths for repo-prefixed file", () => {
|
|
const paths = getAncestorPaths("repo-a/src/utils/helper.ts");
|
|
expect(paths).toEqual([
|
|
"repo-a",
|
|
"repo-a/src",
|
|
"repo-a/src/utils",
|
|
]);
|
|
});
|
|
|
|
it("handles deeply nested repo labels", () => {
|
|
const paths = getAncestorPaths("packages/shared/utils/helpers/string.ts");
|
|
expect(paths).toEqual([
|
|
"packages",
|
|
"packages/shared",
|
|
"packages/shared/utils",
|
|
"packages/shared/utils/helpers",
|
|
]);
|
|
});
|
|
|
|
it("handles flat repo structure", () => {
|
|
const paths = getAncestorPaths("repo-a/file.ts");
|
|
expect(paths).toEqual(["repo-a"]);
|
|
});
|
|
});
|
|
|
|
describe("getAllFolderPaths - workspace mode", () => {
|
|
it("collects all folder paths from repo-prefixed tree", () => {
|
|
const files = [
|
|
diffFile("repo-a/src/index.ts"),
|
|
diffFile("repo-b/src/app.ts"),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
const folders = getAllFolderPaths(tree);
|
|
|
|
// After collapseSingleChild, we get "repo-a/src" and "repo-b/src"
|
|
expect(folders).toContain("repo-a/src");
|
|
expect(folders).toContain("repo-b/src");
|
|
});
|
|
|
|
it("collects nested repo label folders", () => {
|
|
const files = [
|
|
diffFile("apps/api/src/server.ts"),
|
|
diffFile("apps/web/src/app.ts"),
|
|
];
|
|
const tree = buildFileTree(files);
|
|
const folders = getAllFolderPaths(tree);
|
|
|
|
// After collapseSingleChild: apps stays, apps/api/src and apps/web/src
|
|
expect(folders).toContain("apps");
|
|
expect(folders).toContain("apps/api/src");
|
|
expect(folders).toContain("apps/web/src");
|
|
});
|
|
});
|