Add full-file context to experimental plugin diffs - #2106
Conversation
|
🚨 SLOP COP 🚨 · I am reviewing this pull request now. I will check security, code quality, performance, architecture, and the user path. |
| if (normalized === null || fullFileContents === undefined) { | ||
| return normalized?.file ?? null; | ||
| } | ||
| return enrichGitDiffFileForContext({ |
There was a problem hiding this comment.
🚨 slopcop/review — [P1] Reject full-file text that conflicts with the patch
Pierre sets isPartial to false whenever both file objects exist. It does not compare their lines with the patch.
I confirmed that empty or unrelated sides still produce isPartial: false. DiffHost then enables context expansion around a valid patch. Pierre can show unrelated lines.
The public contract says BB enables expansion only for consistent contents. Please validate paths and hunk lines before enrichment. Keep the patch partial when validation fails. Add tests for empty, mismatched, and swapped sides.
| }, | ||
| patchText: normalized.patch, | ||
| }); | ||
| }, [fullFileContents, normalized]); |
There was a problem hiding this comment.
🚨 slopcop/review — [P2] Avoid repeated full-file parsing
This memo uses the wrapper object as a key. A caller can create a new wrapper on each render. That repeats processFile across both complete files, although all paths and strings stay the same.
My 100,000-line test took 0.56 to 0.96 seconds for each call. The parse also runs before a replacement selects experimental_Original.
Move enrichment behind the original-renderer boundary. Key the cache with the patch, paths, and content strings. This change can also remove duplicate enrichment at GitDiffCardBody.tsx:453. Add a rerender test with a new wrapper and unchanged strings.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This pull request gives plugin diff views the complete old and new file text. They can then reveal unchanged lines around a patch, like BB’s built-in diff viewer.
I found two issues:
- P1 correctness: BB accepts unrelated full-file text as complete context. Pierre marks it complete without checking the patch. Context expansion can then show false file content.
- P2 performance: A new wrapper object repeats full-file parsing on each render. BB also parses before it knows whether a replacement needs the built-in renderer.
I posted both findings on the exact changed lines.
No security defect appeared. Frontend plugins already run as trusted same-origin code. This change adds no new file read, HTML sink, or wire field.
The architecture scan found two Pierre enrichment call sites. One lazy enrichment path behind the built-in renderer would remove this duplicate work. It would also avoid work for replacements that never delegate.
Checks completed:
- I verified head SHA
b67d08414e6c152e44a46507cc31865f1f4c991d. git diff --checkpassed.- The app and plugin SDK type checks passed.
- The 17 focused diff tests passed.
- The 107 plugin SDK tests passed in the quality review.
- A live dev app opened the real diff panel and loaded file cards through Doobie.
- Mismatched and empty file text reproduced the false
isPartial: falsestate. - A 100,000-line repeat parse took 0.56 to 0.96 seconds per call.
GitHub has one failed package check. The failure is an unrelated Tasks plugin persistence test. The app, server, integration, checks, and package smoke jobs passed.
This is a comment-only review. I recommend fixes before merge.
What was wrong
experimental_Diffaccepted only a patch, while the native diff panel could lazily load both complete file sides and enrich Pierre so users could expand unchanged context between hunks. The host diff boundary discarded that resolved semantic input, so plugin callers andexperimental_diffRendererreplacements could not offer equivalent context expansion.What changed
ExperimentalDiffFullFileContentscontract and optionalexperimental_fullFileContentsprop toexperimental_Diff.null, while keeping loading, retry, viewport, image-preview, and mobile policy with the caller.This is an app/plugin SDK contract change only. It does not change server-to-daemon messages, so
HOST_DAEMON_PROTOCOL_VERSIONis unchanged.How you verified
The new tests fail before this change because the full-file contract and renderer prop do not exist.
pnpm exec turbo run typecheck --filter=@bb/app --filter=@get-bb/plugin-sdkpnpm exec turbo run test --filter=@bb/app --force— 404 files, 3,124 tests passedpnpm exec turbo run test --filter=@get-bb/plugin-sdk --force— 107 tests passedpnpm exec turbo run lint --filter=@bb/app --force— no errorsgit diff --checkFixes: no linked issue.