Skip to content

Add full-file context to experimental plugin diffs - #2106

Open
ymichael wants to merge 1 commit into
mainfrom
bb/review-experimental-diff-gaps-thr_xh6w4s8t3m
Open

Add full-file context to experimental plugin diffs#2106
ymichael wants to merge 1 commit into
mainfrom
bb/review-experimental-diff-gaps-thr_xh6w4s8t3m

Conversation

@ymichael

Copy link
Copy Markdown
Collaborator

What was wrong

experimental_Diff accepted 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 and experimental_diffRenderer replacements could not offer equivalent context expansion.

What changed

  • Add the experimental ExperimentalDiffFullFileContents contract and optional experimental_fullFileContents prop to experimental_Diff.
  • Resolve replacement props to the complete old/new text sides or null, while keeping loading, retry, viewport, image-preview, and mobile policy with the caller.
  • Preserve the native panel lazy-loading flow, enrich the built-in renderer only when complete contents are consistent with the patch, and forward those contents to replacement renderers.
  • Update the plugin testing harness, authoring skill, guide, and API audit entry.
  • Add regression coverage for public component enrichment and native panel lazy-loading through a replacement renderer.

This is an app/plugin SDK contract change only. It does not change server-to-daemon messages, so HOST_DAEMON_PROTOCOL_VERSION is 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-sdk
  • pnpm exec turbo run test --filter=@bb/app --force — 404 files, 3,124 tests passed
  • Focused diff context tests — 3 files, 21 tests passed after the final experimental API naming pass
  • pnpm exec turbo run test --filter=@get-bb/plugin-sdk --force — 107 tests passed
  • Plugin authoring documentation test — 14 tests passed
  • pnpm exec turbo run lint --filter=@bb/app --force — no errors
  • git diff --check

Fixes: no linked issue.

AGENT GENERATED: by GPT-5 Codex

@bb-slop-cop

bb-slop-cop Bot commented Aug 20, 2026

Copy link
Copy Markdown

🚨 SLOP COP 🚨 · review

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({

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

@bb-slop-cop bb-slop-cop Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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:

  1. 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.
  2. 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 --check passed.
  • 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: false state.
  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant