Skip to content

feat(review): explain agent-generated findings with Ask AI - #1181

Open
leoreisdias wants to merge 1 commit into
backnotprop:mainfrom
leoreisdias:feat/explore-ai-finding-explanations
Open

feat(review): explain agent-generated findings with Ask AI#1181
leoreisdias wants to merge 1 commit into
backnotprop:mainfrom
leoreisdias:feat/explore-ai-finding-explanations

Conversation

@leoreisdias

@leoreisdias leoreisdias commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Context

Review agents can add useful findings directly to the code review, but following up on one currently means copying its text and rebuilding the relevant code context by hand.

This adds an Explain finding action that hands the finding to Ask AI in place. The response is intentionally educational: it independently checks the finding before explaining the behavior and impact, then offers a brief recommendation when the finding holds.

What changed

  • Adds an Explain action to agent-generated line, file, and general findings across the diff, all-files, sidebar, guided review, and agent-job detail surfaces.
  • Opens the Ask AI sidebar and submits the finding immediately with its own file, range, side, selected code, finding text, and review-agent reasoning.
  • Keeps the request tied to the finding itself instead of reusing the user's pending line selection.
  • Hides the action when Ask AI is unavailable and disables it while Ask AI is creating or streaming a response.
  • Leaves user-authored annotations and GitHub/GitLab review comments unchanged.

Demo

Aug-03-2026 10-06-35

Review focus

  • buildExplainFindingRequest is the context boundary: general findings stay fileless, file findings carry only the path, and line findings include their exact range and extracted code.
  • Agent provenance is intentionally centralized in isAgentGeneratedFinding and currently recognizes review-job sources through the agent- prefix.
  • The all-files render callbacks change identity when Explain availability changes so Pierre's memoized annotation and header portals republish the action without changing Pierre's data contracts.
  • Guided Review uses the current virtualized GuideFileCard path rather than restoring the removed legacy guide viewer.

Test plan

  • bun test packages/review-editor — 166 passed, 29 skipped
  • DOM_TESTS=1 bun test packages/review-editor/components/CommentActions.test.tsx — 2 passed
  • bun run typecheck
  • bun run build:review
  • bun run build:hook
  • Add the interaction GIF above

@backnotprop

Copy link
Copy Markdown
Owner

Review (at 9b64cbe0)

Thanks for this. The core design is sound: all five surfaces route through one handler, the pending line-selection is genuinely untouched (cleanest part of the PR), the hidden/disabled gating holds against the real session state machine, and the explain request rides the normal Ask AI path so it gets the changes-under-review preamble correctly. Typecheck clean, suite green, and explainFinding.test.ts survived mutation testing on all four attempted mutations. Verdict: needs changes, all of it well-scoped.

Required

1. CommentActions.test.tsx never runs in CI. It is DOM-gated but not registered in the DOM_TESTS=1 step of .github/workflows/test.yml, so the only test guarding the disabled-while-streaming claim silently skips (verified: 2 pass with the flag, 2 skip without). The test itself has teeth (removing disabled fails it). Add the path to that workflow step; this repo has been bitten by exactly this before.

2. The all-files render callbacks change identity on every render, not on availability changes. useAIChat returns a bare object literal, so askAI, then handleExplainAnnotation, then the panel props, then the new useCallback wrappers in AllFilesCodeView.tsx:864 and :2119 all get new identities every ReviewApp render. Pierre's SlotPortals is memoized on exactly those props, and before this PR both renderers were useStableCallback, so it never re-rendered. After, every visible file header and inline annotation re-renders per Ask AI SSE chunk and per search keystroke. Verified perf-only (portal keys and element types are stable, so card state survives), but this is the large-diff lag class. The fix is one token per site: depend on Boolean(onExplainAnnotation) (plus isAILoading) instead of the callback identity, which restores the behavior the PR body describes.

3. Tighten provenance from prefix to registry membership, and fence the finding text. isAgentGeneratedFinding keys on source.startsWith('agent-'), but POST /api/external-annotations applies zero validation to source (the one free-form field in the validator), and PATCH bypasses the transform entirely, so any local caller can relabel an annotation to agent-* and get one-click submission into an Ask AI session that can hold tool permissions. The authoritative registry is already in scope: agentJobs.jobs has a required source field, so a Set membership check is a one-liner in App.tsx. Relatedly, the composed message currently ends with the untrusted finding text unfenced, directly after the instruction block. The codebase's own convention fences untrusted content (buildDefaultPrompt wraps scope text and selected code in code fences); do the same for the finding and reasoning blocks and label them as data.

Verified clean on the adjacent worries: user-authored annotations never carry source, and GitHub/GitLab review comments become CommentAnnotation, which has no source field and whose sidebar branch passes no onExplain, so neither can ever grow the button.

4. Commit-scoped findings produce confidently wrong code. buildExplainFindingRequest drops commitSha, so a finding made on a commit:<sha> diff, explained after switching views, extracts code from the currently active patch at the historical line numbers and presents it as the finding's code with no warning. The export path already solved this exact problem (exportFeedback.ts:105 emits an anchored-to-that-commit note); the explain path needs the same guard, or should skip selectedCode on a commit mismatch.

Worth fixing while in there

5. Inverted ranges are passed raw (lines 16-12, empty extraction). Both established helpers normalize with Math.min/Math.max; explainFinding.ts should too, especially since external callers control these fields.

6. Stale ranges after a diff refresh degrade to misleading rather than fileless: the agent gets a path plus line numbers that may no longer exist, with no selectedCode and no signal. Prefer degrading to a fileless request (or flagging the missing extraction in the prompt).

7. ReviewAgentJobDetailPanel is the only surface that skips the isAgentGeneratedFinding check, and a non-matching row there gets a button that silently no-ops (the App.tsx guard returns without feedback). Add the check for consistency, and consider a toast on the no-op path.

8. [@media(hover:none)]:opacity-100 on the shared action row makes edit/copy/delete permanently visible on touch for every comment card, including PR-comment cards, which contradicts the "GitHub/GitLab review comments unchanged" claim. Probably desirable, but it deserves a mention in the PR body. Also group/finding in the job panel is now vestigial after the CopyButton relocation.

9. Nit: explainFinding.ts:717 inlines annotation.scope ?? 'line' instead of using annotationScope() from annotationDisplay.ts, the documented single source of truth.

The two full-suite failures during review were the documented timing flakes (diff-fingerprint, file-browser-watch); both pass in isolation and are unrelated to this PR.

leoreisdias added a commit to leoreisdias/plannotator that referenced this pull request Aug 3, 2026
@leoreisdias

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I will work on them

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.

2 participants