From 306a0756434aa0b4483020b445ec2096f86b5f12 Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Sat, 19 Sep 2026 14:49:37 -0700 Subject: [PATCH 1/3] Pass authored evidence through existing Review app surfaces Update code peeks, inline editors, stack views, diagrams and diff sections to carry discriminated evidence content and targets through the existing protocol. AI assistance: reorganized with Codex. --- packages/review/app/src/CodePeek.tsx | 17 +++++--- packages/review/app/src/DiffView.tsx | 42 +++++++++++++------ packages/review/app/src/InlineCodeEditor.tsx | 18 +++++--- .../review/app/src/authored-code-surface.tsx | 3 +- packages/review/app/src/call-stack-diff.tsx | 8 +++- packages/review/app/src/call-tree.ts | 31 ++++++++++---- packages/review/app/src/database-lens.tsx | 2 +- packages/review/app/src/diff-sections.ts | 15 +++++-- .../review/app/src/fixture-review-bridge.ts | 6 ++- packages/review/app/src/lens-diagram.tsx | 20 ++++++--- .../review/app/src/review-lenses.test.tsx | 8 ++++ packages/review/app/src/review-lenses.tsx | 12 ++++-- packages/review/app/src/review-panel-model.ts | 2 +- 13 files changed, 137 insertions(+), 47 deletions(-) diff --git a/packages/review/app/src/CodePeek.tsx b/packages/review/app/src/CodePeek.tsx index e7605088b..fc46476cc 100644 --- a/packages/review/app/src/CodePeek.tsx +++ b/packages/review/app/src/CodePeek.tsx @@ -6,7 +6,12 @@ import type { import { useMemo, useRef } from "react"; import type { ReviewComponentProps } from "../../src/review-document-data"; -import { type Source, codePeekSource } from "../../src/source"; +import { + type Source, + type CodeEvidence, + evidenceLocation, + codePeekSource, +} from "../../src/source"; import { useReviewSession } from "./host/review-session"; import { InlineCodeEditor } from "./InlineCodeEditor"; @@ -104,7 +109,7 @@ export function CodePeekCard({ heightMode = "capped", onNativeFocus, }: { - source: Source; + source: CodeEvidence; active?: boolean; heightMode?: ReviewInlineEditorHeightMode; onNativeFocus?: () => void; @@ -121,7 +126,8 @@ export function CodePeekCard({ @@ -138,7 +144,8 @@ export function CodePeekCard({ ); } -export function codePeekSubject(source: Source): CodePeekSubject { +export function codePeekSubject(evidence: CodeEvidence): CodePeekSubject { + const source = evidenceLocation(evidence); return { title: codePeekRangeTitle(source.file, source.fromLine, source.toLine), file: source.file, diff --git a/packages/review/app/src/DiffView.tsx b/packages/review/app/src/DiffView.tsx index 01841c691..f4cc6a1ed 100644 --- a/packages/review/app/src/DiffView.tsx +++ b/packages/review/app/src/DiffView.tsx @@ -6,6 +6,7 @@ import type { } from "@dev.fast/review-protocol"; import { useLayoutEffect, useMemo, useRef, useState } from "react"; +import { evidenceSources } from "../../src/source"; import type { Source } from "../../src/source"; import { type CoverageProgress, @@ -85,7 +86,14 @@ export function ReviewDiffView({ scope }: { scope?: ReviewCommitScope }) { ? { files: lenses.progress.files.map((file) => ({ path: file.path, - ...coverageProgress([file], lens.ranges), + ...coverageProgress( + [file], + lens.targets.flatMap((target) => + target.kind === "ranges" + ? [...target.ranges] + : target.results.flatMap(evidenceSources), + ), + ), viewedRanges: coverageSources(file), changedRanges: coverageSources(file, file.changed), unfoldRanges: lenses.unfoldRanges.filter( @@ -126,7 +134,11 @@ export function ReviewDiffView({ scope }: { scope?: ReviewCommitScope }) { const sources = scoped ? (sectionId ? sections.find((section) => section.id === sectionId)?.sources - : lens?.ranges + : lens?.targets.flatMap((target) => + target.kind === "ranges" + ? [...target.ranges] + : target.results.flatMap(evidenceSources), + ) )?.filter( (source) => source.file === @@ -244,16 +256,22 @@ export function ReviewDiffView({ scope }: { scope?: ReviewCommitScope }) { {lenses.progress ? lens ? new Set( - lens.ranges.map( - (source) => - lenses.progress!.files.find( - (file) => - source.file === - (source.side === "base" - ? (file.previousPath ?? file.path) - : file.path), - )?.path ?? source.file, - ), + lens.targets + .flatMap((target) => + target.kind === "ranges" + ? [...target.ranges] + : target.results.flatMap(evidenceSources), + ) + .map( + (source) => + lenses.progress!.files.find( + (file) => + source.file === + (source.side === "base" + ? (file.previousPath ?? file.path) + : file.path), + )?.path ?? source.file, + ), ).size : lenses.progress.files.length : "…"} diff --git a/packages/review/app/src/InlineCodeEditor.tsx b/packages/review/app/src/InlineCodeEditor.tsx index 675e0af14..1322a4aa1 100644 --- a/packages/review/app/src/InlineCodeEditor.tsx +++ b/packages/review/app/src/InlineCodeEditor.tsx @@ -7,6 +7,7 @@ import type { } from "@dev.fast/review-protocol"; import { useCallback, useLayoutEffect, useRef, useState } from "react"; +import type { CodeEvidence } from "../../src/source"; import { useReviewSession } from "./host/review-session"; import { useReviewFindRegistration } from "./review-find"; import { emitReviewInteraction } from "./review-interaction-event"; @@ -19,6 +20,7 @@ const INLINE_HEADER_HEIGHT = 40; export function InlineCodeEditor({ path, + evidence, title, description, side, @@ -31,6 +33,7 @@ export function InlineCodeEditor({ collapsed = false, }: { path: string; + evidence?: CodeEvidence; title: string; description?: string; side: ReviewDiffSide; @@ -43,6 +46,12 @@ export function InlineCodeEditor({ collapsed?: boolean; }) { const session = useReviewSession(); + const content = + evidence && "display" in evidence + ? { kind: "diffr" as const, result: evidence } + : { kind: "source" as const, path, side, ranges }; + const evidenceKey = + evidence && "display" in evidence ? JSON.stringify(evidence) : ""; const [container, setContainer] = useState(null); const handleNavigation = useCallback( @@ -110,9 +119,9 @@ export function InlineCodeEditor({ if (handle) return handle.setFindQuery(query); - return inlineEditorFactory.find({ path, side, ranges }, query); + return inlineEditorFactory.find({ content }, query); }, - [inlineEditorFactory, path, rangesKey, side], + [inlineEditorFactory, path, rangesKey, side, evidenceKey], ); const revealFindMatch = useCallback( @@ -188,11 +197,9 @@ export function InlineCodeEditor({ try { handle = inlineEditorFactory.create({ container, - path, + content, title, description, - side, - ranges, heightMode, active, @@ -242,6 +249,7 @@ export function InlineCodeEditor({ inlineEditorSessionId, path, rangesKey, + evidenceKey, shouldMount, side, title, diff --git a/packages/review/app/src/authored-code-surface.tsx b/packages/review/app/src/authored-code-surface.tsx index 480b00e4e..e66d1721a 100644 --- a/packages/review/app/src/authored-code-surface.tsx +++ b/packages/review/app/src/authored-code-surface.tsx @@ -1,5 +1,6 @@ import type { ReactElement } from "react"; +import { evidenceLocation } from "../../src/source"; import type { PeekAnchor } from "./review-panel-model"; /** @@ -15,7 +16,7 @@ export function AuthoredCodeSurface({ code: string; language?: string; }): ReactElement { - const firstLine = anchor.peek?.fromLine ?? 1; + const firstLine = anchor.peek ? evidenceLocation(anchor.peek).fromLine : 1; return (
diff --git a/packages/review/app/src/call-stack-diff.tsx b/packages/review/app/src/call-stack-diff.tsx index b58c5fa24..69f549da2 100644 --- a/packages/review/app/src/call-stack-diff.tsx +++ b/packages/review/app/src/call-stack-diff.tsx @@ -4,6 +4,7 @@ import { } from "../../src/call-stack-diff"; import { frameIdentity, frameName } from "../../src/call-stack-frames"; import type { Frame } from "../../src/review-api/document"; +import { evidenceLocation } from "../../src/source"; import { useReviewSession } from "./host/review-session"; import { useReviewPanel } from "./review-panel"; import type { PeekAnchor } from "./review-panel-model"; @@ -59,7 +60,7 @@ export function CallStackDiff({ title, base, head }: CallStackDiffProps) { role="listitem" className={`call-stack-row call-stack-${row.change}`} data-review-anchor-id={frame.id ?? frameIdentity(frame)} - title={`${rowTooltip(frame, parent)} — ${frame.source.file}:${frame.source.fromLine}`} + title={`${rowTooltip(frame, parent)} — ${evidenceLocation(frame.source).file}:${evidenceLocation(frame.source).fromLine}`} onClick={() => { captureUiEvent(session, "peek_opened", { via: "call_stack_frame", @@ -83,7 +84,10 @@ export function CallStackDiff({ title, base, head }: CallStackDiffProps) { ) : null} - {locationLabel(frame.source.file, frame.source.fromLine)} + {locationLabel( + evidenceLocation(frame.source).file, + evidenceLocation(frame.source).fromLine, + )} ); diff --git a/packages/review/app/src/call-tree.ts b/packages/review/app/src/call-tree.ts index 457428ec3..19345b4fd 100644 --- a/packages/review/app/src/call-tree.ts +++ b/packages/review/app/src/call-tree.ts @@ -1,10 +1,18 @@ +import type { ReviewDiffLensTarget } from "@dev.fast/review-protocol"; + import type { CallStackDiffBlock } from "../../src/review-api/blocks/call_stack_diff"; +import { + evidenceLocation, + evidenceSources, + evidenceTargets, +} from "../../src/source"; import type { Source } from "../../src/source"; export interface CallTreeStop { id: string; label: string; sources: Source[]; + targets: ReviewDiffLensTarget[]; parentId?: string; callSite?: Source; via?: string; @@ -27,19 +35,28 @@ export function callTreeStops(block: CallStackDiffBlock): CallTreeStop[] { const id = `${block.id}:${frame.key ?? `${side}:${index}`}`; const existing = nodes.get(id); - if (existing) + const evidence = [ + frame.source, + ...(frame.contextSources ?? []), + ...(frame.callSite ? [frame.callSite] : []), + ]; + if (existing) { + existing.targets.push(...evidenceTargets(evidence)); existing.sources.push( - frame.source, - ...(frame.contextSources ?? []), + ...evidenceSources(frame.source), + ...(frame.contextSources ?? []).flatMap(evidenceSources), ...(frame.callSite ? [frame.callSite] : []), ); - else + } else nodes.set(id, { id, - label: frame.label ?? frame.source.file.split("/").pop()!, + label: + frame.label ?? + evidenceLocation(frame.source).file.split("/").pop()!, + targets: evidenceTargets(evidence), sources: [ - frame.source, - ...(frame.contextSources ?? []), + ...evidenceSources(frame.source), + ...(frame.contextSources ?? []).flatMap(evidenceSources), ...(frame.callSite ? [frame.callSite] : []), ], parentId: diff --git a/packages/review/app/src/database-lens.tsx b/packages/review/app/src/database-lens.tsx index 5d6e056fe..d7ef2b148 100644 --- a/packages/review/app/src/database-lens.tsx +++ b/packages/review/app/src/database-lens.tsx @@ -15,7 +15,7 @@ import type { DatabaseOperation, DatabaseStore, } from "../../src/review-api/document"; -import type { Source } from "../../src/source"; +import type { CodeEvidence as Source } from "../../src/source"; import { DiagramTourOverlay, useDiagramTourShell } from "./diagram-tour"; import { useReviewSession } from "./host/review-session"; import type { GuidedTour, PeekAnchor } from "./review-panel-model"; diff --git a/packages/review/app/src/diff-sections.ts b/packages/review/app/src/diff-sections.ts index 34f0339eb..e571c90a2 100644 --- a/packages/review/app/src/diff-sections.ts +++ b/packages/review/app/src/diff-sections.ts @@ -1,5 +1,11 @@ +import type { ReviewDiffLensTarget } from "@dev.fast/review-protocol"; + import type { Block } from "../../src/review-api/document"; -import type { Source } from "../../src/source"; +import { + evidenceSources, + evidenceTargets, + type Source, +} from "../../src/source"; import { callTreeStops } from "./call-tree"; /** The same scope drives diagram navigation, diff boundaries and viewed actions. */ @@ -7,6 +13,7 @@ export interface DiffSection { id: string; label: string; sources: Source[]; + targets: ReviewDiffLensTarget[]; } export function diffSections(block: Block | undefined): DiffSection[] { @@ -19,7 +26,8 @@ export function diffSections(block: Block | undefined): DiffSection[] { { id: step.id ?? `${block.id}:${index}`, label: step.label, - sources: [step.source], + sources: evidenceSources(step.source), + targets: evidenceTargets([step.source]), }, ] : [], @@ -32,7 +40,8 @@ export function diffSections(block: Block | undefined): DiffSection[] { useCase.operations.map((operation, index) => ({ id: operation.id ?? `${useCase.id}:${index}`, label: operation.label, - sources: [operation.source], + sources: evidenceSources(operation.source), + targets: evidenceTargets([operation.source]), })), ); diff --git a/packages/review/app/src/fixture-review-bridge.ts b/packages/review/app/src/fixture-review-bridge.ts index f9d5df883..6bdec5f7e 100644 --- a/packages/review/app/src/fixture-review-bridge.ts +++ b/packages/review/app/src/fixture-review-bridge.ts @@ -85,7 +85,11 @@ export function fixtureReviewBridge(api: FixtureReviewApi): ReviewCanvasBridge { create: (spec) => { const editor = document.createElement("div"); editor.className = "fixture-inline-editor"; - editor.dataset.path = spec.path; + editor.dataset.path = + spec.content.kind === "source" + ? spec.content.path + : (spec.content.result.file.rhs ?? spec.content.result.file.lhs!) + .path; spec.container.appendChild(editor); return { diff --git a/packages/review/app/src/lens-diagram.tsx b/packages/review/app/src/lens-diagram.tsx index d579978a8..e29611d6b 100644 --- a/packages/review/app/src/lens-diagram.tsx +++ b/packages/review/app/src/lens-diagram.tsx @@ -1,5 +1,10 @@ import type { Block } from "../../src/review-api/document"; -import type { Source } from "../../src/source"; +import { + evidenceSources, + evidenceLocation, + type CodeEvidence, + type Source, +} from "../../src/source"; import { LensCallTree } from "./lens-call-tree"; import { ElementCounts } from "./lens-counts"; import { useReviewLenses } from "./review-lenses"; @@ -7,15 +12,18 @@ import { useReviewLenses } from "./review-lenses"; /** Compact diagrams are navigation: clicking evidence scrolls, never changes scope. */ export function LensDiagram({ block, - onReveal, + onReveal: reveal, }: { block: Block; onReveal(source: Source, sectionId?: string): void; }) { const lenses = useReviewLenses()!; + const onReveal = (evidence: CodeEvidence, sectionId?: string) => + reveal(evidenceLocation(evidence), sectionId); + const stats = (sources: CodeEvidence[]) => + lenses.stats(sources.flatMap(evidenceSources)); - const viewed = (sources: Source[]) => - lenses.stats(sources).state === "viewed"; + const viewed = (sources: CodeEvidence[]) => stats(sources).state === "viewed"; if (block.type === "sequence") { const actors = Object.entries(block.actors); @@ -72,7 +80,7 @@ export function LensDiagram({ {step.label} {source - ? ` · Total +${lenses.stats([source]).total.additions} −${lenses.stats([source]).total.deletions}` + ? ` · Total +${stats([source]).total.additions} −${stats([source]).total.deletions}` : ""} - + )} Date: Sat, 19 Sep 2026 14:49:38 -0700 Subject: [PATCH 2/3] Render supplied trees with the existing structural Monaco diff view Honor display sides independently of complete retained comparisons. Use the existing structural renderer for folds and summaries, add blue search highlights, and retain one-sided full-width layout and navigation. No custom replacement diff component. AI assistance: reorganized with Codex. --- .../multiDiffEditor/diffEditorItemTemplate.ts | 1 + .../vs/editor/common/config/editorOptions.ts | 2 + .../src/vs/review/browser/media/review.css | 6 + .../src/vs/review/common/reviewLens.ts | 6 +- .../src/vs/review/common/reviewLensFiles.ts | 2 +- .../vs/review/common/reviewSearchEvidence.ts | 157 ++++++++++++++++++ .../vs/review/common/reviewStructuralDiff.ts | 51 ++++-- .../review/services/reviewApiSourceService.ts | 40 +++-- .../services/reviewCodeResourceService.ts | 25 +++ .../review/services/reviewDiffViewService.ts | 26 ++- .../vs/review/services/reviewFilesDiffView.ts | 10 +- .../services/reviewInlineEditorService.ts | 68 ++++++-- .../src/vs/review/services/reviewLens.ts | 31 +++- .../review/services/reviewStructuralDiff.ts | 98 +++++++---- 14 files changed, 418 insertions(+), 105 deletions(-) create mode 100644 apps/review-desktop/code-oss/src/vs/review/common/reviewSearchEvidence.ts diff --git a/apps/review-desktop/code-oss/src/vs/editor/browser/widget/multiDiffEditor/diffEditorItemTemplate.ts b/apps/review-desktop/code-oss/src/vs/editor/browser/widget/multiDiffEditor/diffEditorItemTemplate.ts index 523f3313c..afaf2b2c7 100644 --- a/apps/review-desktop/code-oss/src/vs/editor/browser/widget/multiDiffEditor/diffEditorItemTemplate.ts +++ b/apps/review-desktop/code-oss/src/vs/editor/browser/widget/multiDiffEditor/diffEditorItemTemplate.ts @@ -189,6 +189,7 @@ export class DiffEditorItemTemplate extends Disposable implements IPooledObject< return { ...options, ...optionsOverride?.get(), + ...(options.forceInline ? { renderSideBySide: false } : {}), scrollBeyondLastLine: false, hideUnchangedRegions: options.hideUnchangedRegions ?? { enabled: true, diff --git a/apps/review-desktop/code-oss/src/vs/editor/common/config/editorOptions.ts b/apps/review-desktop/code-oss/src/vs/editor/common/config/editorOptions.ts index 6a7973ad8..b30661ab2 100644 --- a/apps/review-desktop/code-oss/src/vs/editor/common/config/editorOptions.ts +++ b/apps/review-desktop/code-oss/src/vs/editor/common/config/editorOptions.ts @@ -1015,6 +1015,8 @@ export interface IDiffEditorBaseOptions { * Configuration options for the diff editor. */ export interface IDiffEditorOptions extends IEditorOptions, IDiffEditorBaseOptions { + /** Keep an explicitly one-sided item inline even when its multi-diff container uses split layout. @internal */ + forceInline?: boolean; } /** diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/media/review.css b/apps/review-desktop/code-oss/src/vs/review/browser/media/review.css index 03fcaee15..2e5403f59 100644 --- a/apps/review-desktop/code-oss/src/vs/review/browser/media/review.css +++ b/apps/review-desktop/code-oss/src/vs/review/browser/media/review.css @@ -1204,3 +1204,9 @@ body .review-diff-group-toggle { display:flex; align-items:center; gap:8px; min-width:0; border:0; padding:0; color:inherit; background:transparent; cursor:pointer; text-align:left; } .review-diff-group-toggle:focus-visible { outline:1px solid var(--vscode-focusBorder); } + +/* Search evidence is distinct from inserted/deleted-line coloring. */ +.review-search-highlight { + background-color: rgba(65, 135, 245, 0.30); + outline: 1px solid rgba(65, 135, 245, 0.65); +} diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewLens.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewLens.ts index c59c126a9..a2bd1d383 100644 --- a/apps/review-desktop/code-oss/src/vs/review/common/reviewLens.ts +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewLens.ts @@ -6,7 +6,7 @@ import type { IDocumentDiff, IDocumentContextGap } from '../../editor/common/dif import type { ReviewDiffLens } from './reviewProtocol.js'; /** Project pinned ranges onto the current diff's correspondence, never onto another revision. */ -export function lensContextGaps(diff: IDocumentDiff, originalCount: number, modifiedCount: number, ranges: ReviewDiffLens['ranges']): IDocumentContextGap[] { +export function lensContextGaps(diff: IDocumentDiff, originalCount: number, modifiedCount: number, ranges: Extract['ranges']): IDocumentContextGap[] { const rows = alignmentRows(diff, originalCount, modifiedCount); const visible = rows.map(row => ranges.some(range => { const line = row[range.side === 'base' ? 0 : 1]; @@ -48,9 +48,9 @@ function alignmentRows(diff: IDocumentDiff, originalCount: number, modifiedCount } /** A paired row folds only when none of its changed lines remain unread. */ -export function viewedContextGaps(diff: IDocumentDiff, originalCount: number, modifiedCount: number, viewed: ReviewDiffLens['ranges'], changed: ReviewDiffLens['ranges']): IDocumentContextGap[] { +export function viewedContextGaps(diff: IDocumentDiff, originalCount: number, modifiedCount: number, viewed: Extract['ranges'], changed: Extract['ranges']): IDocumentContextGap[] { const rows = alignmentRows(diff, originalCount, modifiedCount); - const contains = (ranges: ReviewDiffLens['ranges'], side: 0 | 1, line: number) => ranges.some(range => range.side === (side === 0 ? 'base' : 'head') && line + 1 >= range.fromLine && line + 1 <= range.toLine); + const contains = (ranges: Extract['ranges'], side: 0 | 1, line: number) => ranges.some(range => range.side === (side === 0 ? 'base' : 'head') && line + 1 >= range.fromLine && line + 1 <= range.toLine); const hidden = rows.map(row => row.some((line, side) => line !== null && contains(viewed, side as 0 | 1, line)) && row.every((line, side) => line === null || !contains(changed, side as 0 | 1, line) || contains(viewed, side as 0 | 1, line))); const gaps: IDocumentContextGap[] = []; let left = 1, right = 1; diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewLensFiles.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewLensFiles.ts index 8fd4a8578..c36124047 100644 --- a/apps/review-desktop/code-oss/src/vs/review/common/reviewLensFiles.ts +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewLensFiles.ts @@ -5,7 +5,7 @@ export function lensFiles(files: readonly ReviewDiffFileWire[], lens?: ReviewDif if (!lens || lens.wholeFiles) return files; const known = new Set(files.flatMap(file => [file.path, ...(file.previousPath ? [file.previousPath] : [])])); const context: ReviewDiffFileWire[] = []; - for (const range of lens.ranges) { + for (const range of lens.targets.flatMap(target => target.kind === "ranges" ? [...target.ranges] : [])) { if (known.has(range.file)) continue; known.add(range.file); context.push({ path: range.file, status: 'unchanged', additions: 0, deletions: 0 }); diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewSearchEvidence.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewSearchEvidence.ts new file mode 100644 index 000000000..8d48b89ea --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewSearchEvidence.ts @@ -0,0 +1,157 @@ +import type { + RegionData, + SearchResultData, + SourceData, + ReviewInlineEditorSpec, +} from "./reviewProtocol.js"; +import type { ReviewUnifiedDiffRow } from "./reviewUnifiedDiff.js"; + +/** Rendering selection never removes sources from retained evidence. */ +export function displayedSources(result: SearchResultData) { + return { + lhs: result.display !== "rhs" ? result.sources.same ?? result.sources.lhs : undefined, + rhs: result.display !== "lhs" ? result.sources.same ?? result.sources.rhs : undefined, + }; +} + +export interface EvidenceRow extends ReviewUnifiedDiffRow { + highlights: { startColumn: number; endColumn: number }[]; + fold?: { id: number; collapsed: boolean }; +} +interface Item { + key: string; + line: number; + text: string; + changed: boolean; + highlights: EvidenceRow["highlights"]; + fold?: EvidenceRow["fold"]; +} + +function items(source: SourceData): Item[] { + const lines = source.text.split("\n").map((line) => line.replace(/\r$/, "")); + const output: Item[] = []; + const column = (text: string, byte: number) => + new TextDecoder().decode(new TextEncoder().encode(text).slice(0, byte)) + .length + 1; + const walk = (regions: RegionData[]) => { + for (const region of regions) { + const first = output.length; + const end = region.end.line + Number(region.end.column > 0); + if (region.visibility?.collapsed) { + output.push({ + key: `fold:${region.fold_state_id}`, + line: region.start.line + 1, + text: `… ${region.visibility.label || `${region.start.line + 1}–${end}`} …`, + changed: false, + highlights: [], + fold: { id: region.fold_state_id, collapsed: true }, + }); + } else if (region.kind === "fold") walk(region.children); + else + for (let line = region.start.line; line < end; line++) { + const text = lines[line] ?? ""; + output.push({ + key: `line:${region.alignment_id}:${line - region.start.line}`, + line: line + 1, + text, + changed: (region.changed ?? []).some((span) => span.line === line), + highlights: (region.search_highlights ?? []) + .filter((span) => span.line === line) + .map((span) => ({ + startColumn: column(text, span.start_column), + endColumn: column(text, span.end_column), + })), + }); + } + if ( + output[first] && + !output[first].fold && + (region.kind === "fold" || region.visibility?.label) + ) + output[first].fold = { id: region.fold_state_id, collapsed: false }; + } + }; + walk(source.regions); + return output; +} + +/** Use supplied structural alignment, visibility and highlights; never compute a new diff. */ +export function evidenceRows(result: SearchResultData): EvidenceRow[] { + const sources = displayedSources(result); + const lhs = sources.lhs ? items(sources.lhs) : []; + const rhs = sources.rhs ? items(sources.rhs) : []; + const rows: EvidenceRow[] = []; + const append = (left?: Item, right?: Item) => { + if ( + left && + right && + (left.changed || right.changed || left.text !== right.text) && + !left.fold?.collapsed && + !right.fold?.collapsed + ) { + append(left); + append(undefined, right); + return; + } + const item = right ?? left!; + rows.push({ + lineNumber: rows.length + 1, + content: item.text, + kind: item.changed ? (right ? "added" : "deleted") : "unchanged", + baseLine: left?.line, + headLine: right?.line, + authorSide: right ? "head" : "base", + authorLine: item.line, + highlights: [...(left?.highlights ?? []), ...(right?.highlights ?? [])], + fold: item.fold ?? left?.fold, + }); + }; + let right = 0; + for (const left of lhs) { + const match = rhs.findIndex( + (item, index) => index >= right && item.key === left.key, + ); + if (match < 0) append(left); + else { + while (right < match) append(undefined, rhs[right++]); + append(left, rhs[right++]); + } + } + while (right < rhs.length) append(undefined, rhs[right++]); + return rows; +} + +export function evidenceCoordinates( + content: ReviewInlineEditorSpec["content"], +) { + if (content.kind === "source") return content; + const result = content.result; + const sources = displayedSources(result); + const side = sources.rhs ? ("head" as const) : ("base" as const); + const file = side === "head" ? result.file.rhs! : result.file.lhs!; + const source = sources.rhs ?? sources.lhs!; + const highlighted = evidenceRows(result) + .filter( + (row) => + row.highlights.length && + (side === "head" + ? row.headLine !== undefined + : row.baseLine !== undefined), + ) + .map((row) => ({ + startLine: (side === "head" ? row.headLine : row.baseLine)!, + endLine: (side === "head" ? row.headLine : row.baseLine)!, + })); + if (highlighted.length) return { path: file.path, side, ranges: highlighted }; + return { + path: file.path, + side, + ranges: source.regions.map((region) => ({ + startLine: region.start.line + 1, + endLine: Math.max( + region.start.line + 1, + region.end.line + Number(region.end.column > 0), + ), + })), + }; +} diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.ts index ce92a6445..c177b6f2c 100644 --- a/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.ts +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.ts @@ -1,3 +1,5 @@ +import { displayedSources } from "./reviewSearchEvidence.js"; +import type { RegionData } from "./reviewProtocol.js"; /*--------------------------------------------------------------------------------------------- * Copyright (c) dev.fast. All rights reserved. * Licensed under the MIT License. See LICENSE in the repository root for license information. @@ -41,24 +43,9 @@ export interface StructuralVisibility { * side sharing it. Leaves tile the file in order; a fold's range is the hull * of its children. Tags are `:`. */ -export type StructuralRegion = StructuralLeaf | StructuralFold; -interface StructuralRegionBase { - id: number; - fold_state_id: number; - start: StructuralPos; - end: StructuralPos; - tags?: string[]; - visibility?: StructuralVisibility; -} -export interface StructuralLeaf extends StructuralRegionBase { - kind: "leaf"; - alignment_id: number; - changed?: StructuralSpan[]; -} -export interface StructuralFold extends StructuralRegionBase { - kind: "fold"; - children: StructuralRegion[]; -} +export type StructuralRegion = RegionData; +export type StructuralLeaf = Extract; +export type StructuralFold = Extract; export interface StructuralSyntaxSpan extends StructuralSpan { capture: string; } @@ -397,3 +384,31 @@ export function structuralCountsTooltip(counts: StructuralFileCounts): string { if (counts.fallback) rows.push(`line diff: ${counts.fallback.code}`); return rows.join("\n"); } + +/** Retained search trees enter the same fold/alignment renderer as streamed diffs. */ +export function searchStructuralDiff(result: import("./reviewProtocol.js").SearchResultData): StructuralTextDiff { + const count = (source: StructuralSource | undefined, visible: boolean) => { + const lines = new Set(); + const visit = (region: StructuralRegion) => { + if (visible && region.visibility?.collapsed) return; + if (region.kind === "fold") region.children.forEach(visit); + else for (const span of region.changed ?? []) lines.add(span.line); + }; + source?.regions?.forEach(visit); + return lines.size; + }; + const sources = displayedSources(result); + return { type: "text", ...sources, stats: { + textual: { added: count(sources.rhs, false), removed: count(sources.lhs, false) }, + visible: { added: count(sources.rhs, true), removed: count(sources.lhs, true) }, + } }; +} + +export function structuralSearchHighlights(source: StructuralSource | undefined) { + const lines = source?.text.replace(/\r\n/g, "\n").split("\n") ?? []; + return structuralLeaves(source?.regions).flatMap(leaf => (leaf.search_highlights ?? []).map(span => ({ + startLineNumber: span.line + 1, endLineNumber: span.line + 1, + startColumn: utf16Column(lines[span.line], span.start_column), + endColumn: utf16Column(lines[span.line], span.end_column), + }))); +} diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.ts index e0056509b..56322989b 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.ts @@ -1,3 +1,4 @@ +import { displayedSources, evidenceCoordinates } from "../common/reviewSearchEvidence.js"; import { lensFiles } from "../common/reviewLensFiles.js"; /*--------------------------------------------------------------------------------------------- * Copyright (c) dev.fast. All rights reserved. @@ -22,8 +23,8 @@ import { reviewPeekLineMappings, } from "../common/reviewPeek.js"; import type { - ReviewDiffSide, ReviewInlineEditorRange, + ReviewInlineEditorSpec, ReviewDiffFileWire, ReviewInlineEditorFactory, ReviewDiffViewFactory, @@ -444,15 +445,22 @@ export class ReviewApiSourceService } return list; }; - const source = ( - file: string, - side: ReviewDiffSide, - ranges: readonly ReviewInlineEditorRange[], - ): ReviewInlineSource => { + const source = (content: ReviewInlineEditorSpec["content"]): ReviewInlineSource => { + const {path: file, side, ranges} = evidenceCoordinates(content); const target = { view: view(), file, side }; return { snippet: () => this.snippet(target, ranges), - diff: () => this.peekDiff(target, ranges, files(target.view)), + diff: async () => { + if (content.kind === "source") return content.ranges.some(range => range.side && range.side !== content.side) ? this.peekDiff(target, ranges, files(target.view)) : undefined; + const result = content.result; + const path = (result.file.rhs ?? result.file.lhs!).path; + return { + original: apiSourceUri({...target, side: "base", file: result.file.lhs?.path ?? path}), + modified: apiSourceUri({...target, side: "head", file: result.file.rhs?.path ?? path}), + diffFile: {path, previousPath: result.file.lhs?.path, status: result.sources.same ? "unchanged" : result.file.lhs ? result.file.rhs ? "modified" : "deleted" : "added", additions: 0, deletions: 0}, + mappings: [], windows: () => ({original: [], modified: []}), + }; + }, }; }; const diffSource: ReviewDiffViewSource = { @@ -480,7 +488,7 @@ export class ReviewApiSourceService ? `commit=${encodeURIComponent(current.commit)}` : undefined, }), - entries: await Promise.all( + entries: [...await Promise.all( entries.map(async (file) => { const original = file.status === "added" @@ -505,16 +513,26 @@ export class ReviewApiSourceService goToFileResource: (modified ?? original)!, }; }), - ), + ), ...(lens && !lens.wholeFiles ? lens.targets.flatMap(target => target.kind === "results" ? target.results : []) : []).map((result, index) => { + const path = (result.file.rhs ?? result.file.lhs!).path; + const resource = (side: "base" | "head", file: string) => apiSourceUri({ view: current, side, file }).with({ fragment: `evidence-${index}` }); + const original = displayedSources(result).lhs && resource("base", result.file.lhs!.path); + const modified = displayedSources(result).rhs && resource("head", result.file.rhs!.path); + return { + evidence: result, + file: { path, previousPath: result.file.lhs?.path, status: entries.find(file => file.path === path || file.previousPath === path)?.status ?? (result.sources.same ? "unchanged" as const : result.file.lhs ? result.file.rhs ? "modified" as const : "deleted" as const : "added" as const), additions: 0, deletions: 0 }, + original, modified, goToFileResource: (modified ?? original)!, + }; + })], }; }, }; return { inlineEditors: { create: (spec) => - inline.create(spec, source(spec.path, spec.side, spec.ranges)), + inline.create(spec, source(spec.content)), find: (spec, query) => - inline.find(spec, query, source(spec.path, spec.side, spec.ranges)), + inline.find(spec, query, source(spec.content)), } satisfies ReviewInlineEditorFactory, diffView: { create: (spec) => diff.create(spec, diffSource), diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewCodeResourceService.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewCodeResourceService.ts index 189dd4586..c606f46f8 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewCodeResourceService.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewCodeResourceService.ts @@ -1,3 +1,5 @@ +import { displayedSources, evidenceRows } from "../common/reviewSearchEvidence.js"; +import type { SearchResultData } from "../common/reviewProtocol.js"; /*--------------------------------------------------------------------------------------------- * Copyright (c) dev.fast. All rights reserved. * Licensed under the MIT License. See LICENSE in the repository root for license information. @@ -88,6 +90,7 @@ export const IReviewCodeResourceService = createDecorator; + unifiedResource(resource: URI): ReviewUnifiedResourceInfo | undefined; reset(): void; } @@ -190,6 +194,27 @@ export class ReviewCodeResourceService extends Disposable implements IReviewCode }; } + acquireEvidence(result: SearchResultData, target: ReviewCodeDiffTarget): ReviewUnifiedCodeModelReference { + const rows = evidenceRows(result); + const path = (result.file.rhs ?? result.file.lhs!).path; + const resource = URI.from({scheme: REVIEW_UNIFIED_SCHEME, path: `/${path}`, query: `evidence=${Date.now()}-${Math.random()}`}); + const model = this.modelService.createModel(rows.map(row => row.content).join('\n'), this.languageService.createByFilepathOrFirstLine(URI.file(path)), resource); + const entry: ReviewUnifiedResourceEntry = { + model, rows, originalLineCount: displayedSources(result).lhs?.text.split('\n').length ?? 0, + modifiedLineCount: displayedSources(result).rhs?.text.split('\n').length ?? 0, references: 1, + info: { original: target.original, modified: target.modified, path, diffFile: target.diffFile, rows, + targetForRange: (startLine, endLine) => { + if (rows.slice(startLine - 1, endLine).some(row => row.fold?.collapsed)) return null; + const selected = reviewUnifiedTargetForRange(path, rows, startLine, endLine); + return selected ? {...selected, path: result.file[selected.side === "base" ? "lhs" : "rhs"]!.path} : null; + } }, + dispose: () => model.dispose(), + }; + this.unifiedResources.set(resource.toString(), entry); + const windows = [{startLine: 1, endLine: rows.length, lineCount: rows.length, visibleLineCount: rows.length, height: rows.length * 20}]; + return {model, target, rows, windows, ranges: [], dispose: () => {this.unifiedResources.delete(resource.toString()); model.dispose();}}; + } + unifiedResource(resource: URI): ReviewUnifiedResourceInfo | undefined { return this.unifiedResources.get(resource.toString())?.info; } diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts index 6cb3c3321..c4d2944dd 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts @@ -1,5 +1,5 @@ import type { ReviewDiffProgress } from "../common/reviewProtocol.js"; -import { lensRanges, withLens } from "./reviewLens.js"; +import { selectLensEntries, withLens } from "./reviewLens.js"; /*--------------------------------------------------------------------------------------------- * Copyright (c) dev.fast. All rights reserved. * Licensed under the MIT License. See LICENSE in the repository root for license information. @@ -97,9 +97,9 @@ class DiffViewHandle extends Disposable implements ReviewDiffViewHandle { private progress: ReviewDiffProgress | undefined; private readonly progressChanged = this._register(new Emitter()); private pendingSectionId: string | undefined; - private pendingSource: ReviewDiffLens['ranges'][number] | undefined; + private pendingSource: Extract['ranges'][number] | undefined; setProgress(progress: ReviewDiffProgress): void { this.progress = progress; this.view?.setProgress(progress); this.progressChanged.fire(); } - revealSource(source: ReviewDiffLens['ranges'][number], sectionId?: string): void { this.pendingSource = source; this.pendingSectionId = sectionId; this.view?.revealSource(source, sectionId); } + revealSource(source: Extract['ranges'][number], sectionId?: string): void { this.pendingSource = source; this.pendingSectionId = sectionId; this.view?.revealSource(source, sectionId); } private viewStateKey: string | undefined; private adoptedEditors: readonly ICodeEditor[] = []; private disposed = false; @@ -142,7 +142,8 @@ class DiffViewHandle extends Disposable implements ReviewDiffViewHandle { try { const data = await this.source.load(this.spec.scope, this.spec.lens); const { sourceUri, entries } = data; - const structuralEnabled = this.instantiationService.invokeFunction( + const selected = selectLensEntries(entries, this.spec.lens, this.progress?.sections); + const structuralEnabled = selected.some(entry => entry.evidence) || this.instantiationService.invokeFunction( (a) => a .get(IConfigurationService) @@ -155,29 +156,24 @@ class DiffViewHandle extends Disposable implements ReviewDiffViewHandle { const structural = structuralEnabled ? await prepareStructuralReview( this.instantiationService, - entries, + selected, store, data.structuralDiff!, ) : { instantiation: this.instantiationService, - entries, + entries: selected, enabled: false, load: undefined, onDidChangeCounts: undefined, }; if (this.disposed) return; const lens = this.spec.lens; - const sections = this.progress?.sections; - const selected = lens && sections?.length ? sections.flatMap(section => { - const matches = structural.entries.filter(entry => lensRanges({ ...lens, ranges: section.sources }, entry).length > 0); - return matches.map((entry, index) => ({ ...entry, sectionId: section.id, sectionStart: index === 0, original: entry.original?.with({ fragment: section.id }), modified: entry.modified?.with({ fragment: section.id }) })); - }) : lens ? structural.entries.filter(entry => lensRanges(lens, entry).length > 0) : structural.entries; - const selectedPaths = new Set(selected.map(entry => entry.file.path)); - const instantiation = withLens(structural.instantiation, selected, lens, store, () => this.progress, this.progressChanged.event); + const selectedPaths = new Set(structural.entries.map(entry => entry.file.path)); + const instantiation = withLens(structural.instantiation, structural.entries, lens, store, () => this.progress, this.progressChanged.event); // The input owns the text-model references its view model resolves, so // this handle disposes it alongside the view. - const input = store.add(instantiation.createInstance(ReviewFilesEditorInput, sourceUri, selected, + const input = store.add(instantiation.createInstance(ReviewFilesEditorInput, sourceUri, structural.entries, structural.enabled, !!lens || !!this.spec.onToggleViewed)); const view = store.add( instantiation.createInstance( @@ -192,7 +188,7 @@ class DiffViewHandle extends Disposable implements ReviewDiffViewHandle { ); this.view = view; if (this.progress) view.setProgress(this.progress); - if (structural.enabled) view.startLoading(selected); + if (structural.enabled) view.startLoading(structural.entries); store.add(view.onDidChangeActiveControl(() => this.bindActiveControl(view))); // A saved whole-list offset cannot be restored into a partial streamed list. await view.setInput(input, structural.enabled ? undefined : this.viewStates.get(this.viewStateKey), diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewFilesDiffView.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewFilesDiffView.ts index 9d44aac75..1e180a99c 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewFilesDiffView.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewFilesDiffView.ts @@ -51,6 +51,7 @@ const REVIEW_FILES_DIFF_EDITOR_OPTIONS = { } satisfies IDiffEditorOptions; export interface ReviewFilesEditorEntry { + readonly evidence?: import("../common/reviewProtocol.js").SearchResultData; readonly sectionId?: string; readonly sectionStart?: boolean; readonly file: ReviewDiffFileWire; @@ -96,7 +97,8 @@ export class ReviewFilesEditorInput extends MultiDiffEditorInput { (structural || lens) ? { ...REVIEW_FILES_DIFF_EDITOR_OPTIONS, - hideOriginalLineNumbers: entry.file.status === "added", + hideOriginalLineNumbers: entry.original?.scheme === "review-structural-empty" || entry.file.status === "added", + ...((entry.evidence || lens) && (entry.original?.scheme === "review-structural-empty" || entry.modified?.scheme === "review-structural-empty") ? { forceInline: true } : {}), hideUnchangedRegions: { enabled: true, minimumLineCount: 1, @@ -184,7 +186,7 @@ export class ReviewFilesDiffView extends Disposable { private readonly hiddenApplied = new Set(); private pendingPath: string | undefined; private pendingSectionId: string | undefined; - private pendingSource: ReviewDiffLens["ranges"][number] | undefined; + private pendingSource: Extract["ranges"][number] | undefined; private progress: ReviewDiffProgress | undefined; private readonly viewedApplied = new Map(); private readonly streamStatus: HTMLElement; @@ -540,8 +542,8 @@ export class ReviewFilesDiffView extends Disposable { this.viewedApplied.set(key, file.state); } } - revealSource(source: ReviewDiffLens['ranges'][number], sectionId?: string): void { - const entry = this.input?.entries.find(entry => (!sectionId || entry.sectionId === sectionId) && (!entry.sectionId || this.progress?.sections?.find(section => section.id === entry.sectionId)?.sources.some(range => range.file === source.file && range.side === source.side && range.fromLine <= source.fromLine && range.toLine >= source.fromLine)) && source.file === (source.side === 'base' ? entry.file.previousPath ?? entry.file.path : entry.file.path)); + revealSource(source: Extract['ranges'][number], sectionId?: string): void { + const entry = this.input?.entries.find(entry => (!sectionId || entry.sectionId === sectionId) && (source.side === "base" ? entry.original?.scheme !== "review-structural-empty" : entry.modified?.scheme !== "review-structural-empty") && (!entry.sectionId || this.progress?.sections?.find(section => section.id === entry.sectionId)?.sources.some(range => range.file === source.file && range.side === source.side && range.fromLine <= source.fromLine && range.toLine >= source.fromLine)) && source.file === (source.side === 'base' ? entry.file.previousPath ?? entry.file.path : entry.file.path)); if (!entry) return; if (entry.sectionId && this.collapsedSections.delete(entry.sectionId)) this.headerFactory.refreshHeaders(); this.pendingSource = source; this.pendingSectionId = sectionId; diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewInlineEditorService.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewInlineEditorService.ts index d37ceeb25..6be403e52 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewInlineEditorService.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewInlineEditorService.ts @@ -1,3 +1,4 @@ +import { displayedSources, evidenceCoordinates, type EvidenceRow } from "../common/reviewSearchEvidence.js"; /*--------------------------------------------------------------------------------------------- * Copyright (c) dev.fast. All rights reserved. * Licensed under the MIT License. See LICENSE in the repository root for license information. @@ -8,7 +9,7 @@ import { Disposable, DisposableStore, type IDisposable } from "../../base/common import { autorun, observableValue } from "../../base/common/observable.js"; import { URI } from "../../base/common/uri.js"; import type { IEditorConstructionOptions } from "../../editor/browser/config/editorConfiguration.js"; -import type { ICodeEditor } from "../../editor/browser/editorBrowser.js"; +import { MouseTargetType, type ICodeEditor } from "../../editor/browser/editorBrowser.js"; import { EditorExtensionsRegistry } from "../../editor/browser/editorExtensions.js"; import { ICodeEditorService } from "../../editor/browser/services/codeEditorService.js"; import { CodeEditorWidget } from "../../editor/browser/widget/codeEditor/codeEditorWidget.js"; @@ -78,11 +79,13 @@ export interface ReviewInlineSource { /** Pinned API sources feed the unified diff builder. */ async function acquireUnifiedFromSource( resources: IReviewCodeResourceService, - spec: Pick, + spec: ReviewInlineFindSpec, source: ReviewInlineSource, ) { const target = await source.diff(); - return target && resources.acquireUnifiedDiffForTarget(spec.path, spec.side, spec.ranges, target); + if (!target) return undefined; + if (spec.content.kind === "diffr") return resources.acquireEvidence(spec.content.result, target); + return resources.acquireUnifiedDiffForTarget(spec.content.path, spec.content.side, spec.content.ranges, target); } export class ReviewInlineEditorService extends Disposable implements ICompositeCodeEditor { @@ -362,6 +365,8 @@ class InlineEditorHandle extends Disposable implements ReviewInlineEditorHandle return this._height; } + private get coordinates() { return evidenceCoordinates(this.spec.content); } + get hasWidget(): boolean { return this.editor !== undefined; } @@ -383,15 +388,15 @@ class InlineEditorHandle extends Disposable implements ReviewInlineEditorHandle private readonly source: ReviewInlineSource, ) { super(); - if (spec.ranges.length === 0) { + if (evidenceCoordinates(spec.content).ranges.length === 0) { throw new Error("Inline editor requires at least one range."); } this.active = spec.active; - this.expandedHeight = estimatedHeight(spec.ranges, spec.heightMode); + this.expandedHeight = estimatedHeight(evidenceCoordinates(spec.content).ranges, spec.heightMode); this._height = this.expandedHeight; spec.container.classList.add("review-inline-code-editor"); - spec.container.dataset["reviewInlineEditorPath"] = spec.path; - spec.container.dataset["reviewInlineEditorSide"] = spec.side; + spec.container.dataset["reviewInlineEditorPath"] = evidenceCoordinates(spec.content).path; + spec.container.dataset["reviewInlineEditorSide"] = evidenceCoordinates(spec.content).side; const document = spec.container.ownerDocument; const headerHost = document.createElement("div"); headerHost.className = "review-inline-editor-header-host monaco-component multiDiffEditor"; @@ -419,8 +424,8 @@ class InlineEditorHandle extends Disposable implements ReviewInlineEditorHandle this.body.className = "review-inline-editor-body"; spec.container.append(headerHost, this.body); this.setHeader( - URI.from({ scheme: "file", path: `/${this.spec.path}` }), - URI.from({ scheme: "file", path: `/${this.spec.path}` }), + URI.from({ scheme: "file", path: `/${this.coordinates.path}` }), + URI.from({ scheme: "file", path: `/${this.coordinates.path}` }), ); this._register( autorun((reader) => { @@ -556,14 +561,14 @@ class InlineEditorHandle extends Disposable implements ReviewInlineEditorHandle private initializeUnifiedEditor(reference: ReviewUnifiedCodeModelReference): void { const labelUris = reviewMultiDiffLabelUris(reference.target.diffFile); this.setHeader( - reference.target.original, - reference.target.modified, + this.spec.content.kind === "diffr" && !displayedSources(this.spec.content.result).lhs ? undefined : reference.target.original, + this.spec.content.kind === "diffr" && !displayedSources(this.spec.content.result).rhs ? undefined : reference.target.modified, labelUris.original, labelUris.modified, reviewCodePeekRangeCounts( reference.target.diffFile.patch, - this.spec.countRanges ?? this.spec.ranges, - this.spec.side, + this.spec.countRanges ?? this.coordinates.ranges, + this.coordinates.side, ), ); this.unifiedModelReference = reference; @@ -587,7 +592,36 @@ class InlineEditorHandle extends Disposable implements ReviewInlineEditorHandle this.editorStore.add(editor); editor.setModel(reference.model); this.diffDecoration = editor.createDecorationsCollection(); - this.diffDecoration.set(reviewUnifiedDiffDecorations(reference.rows)); + this.diffDecoration.set([ + ...reviewUnifiedDiffDecorations(reference.rows), + ...(this.spec.content.kind === "diffr" ? (reference.rows as EvidenceRow[]).flatMap(row => [ + ...row.highlights.map(span => ({range: new Range(row.lineNumber, span.startColumn, row.lineNumber, Math.max(span.startColumn + 1, span.endColumn)), options: { description: 'Review search highlight', inlineClassName: "review-search-highlight" }})), + ...(row.fold ? [{range: new Range(row.lineNumber, 1, row.lineNumber, 1), options: {description: "Review evidence fold", glyphMarginClassName: row.fold.collapsed ? "codicon codicon-chevron-right" : "codicon codicon-chevron-down", glyphMarginHoverMessage: {value: row.fold.collapsed ? "Expand region" : "Collapse region"}}}] : []), + ]) : []), + ]); + if (this.spec.content.kind === "diffr") { + editor.updateOptions({glyphMargin: true}); + this.editorStore.add(editor.onMouseDown(event => { + const line = event.target.position?.lineNumber; + const row = line ? (reference.rows as EvidenceRow[])[line - 1] : undefined; + if (!row?.fold || event.target.type !== MouseTargetType.GUTTER_GLYPH_MARGIN || this.spec.content.kind !== "diffr") return; + const result = structuredClone(this.spec.content.result); + const toggle = (regions: import("../common/reviewProtocol.js").RegionData[]) => {for (const region of regions) { + if (region.fold_state_id === row.fold!.id) region.visibility = {...region.visibility, collapsed: !row.fold!.collapsed}; + if (region.kind === "fold") toggle(region.children); + }}; + for (const source of Object.values(result.sources)) toggle(source.regions); + this.spec.content = {kind: "diffr", result}; + this.clearFind(); + this.editorStore.clear(); + this.decoration = undefined; + this.diffDecoration = undefined; + this.unifiedModelReference = undefined; + this.editor = undefined; + this.body.replaceChildren(); + void this.initialize(); + })); + } this.bindFocus(editor); this.trackScroll( () => editor.getScrollTop(), @@ -768,16 +802,18 @@ class InlineEditorHandle extends Disposable implements ReviewInlineEditorHandle } private ranges(): Range[] { + if (this.spec.content.kind === "diffr") return (this.unifiedModelReference?.rows as EvidenceRow[] ?? []).flatMap(row => row.highlights.map(span => new Range(row.lineNumber, span.startColumn, row.lineNumber, Math.max(span.startColumn + 1, span.endColumn)))); + if (this.unifiedModelReference) { return this.unifiedModelReference.ranges.map( (range) => new Range(range.startLine, 1, range.endLine, Number.MAX_SAFE_INTEGER), ); } - return this.spec.ranges.map((range) => new Range(range.startLine, 1, range.endLine, Number.MAX_SAFE_INTEGER)); + return this.coordinates.ranges.map((range) => new Range(range.startLine, 1, range.endLine, Number.MAX_SAFE_INTEGER)); } private primaryRange(): Range { - return this.ranges()[0]!; + return this.ranges()[0] ?? new Range(1, 1, 1, 1); } private setHeight(height: number): void { diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewLens.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewLens.ts index 681c211e7..fadd05a55 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewLens.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewLens.ts @@ -11,8 +11,31 @@ import { lensContextGaps, viewedContextGaps } from '../common/reviewLens.js'; import type { ReviewDiffLens, ReviewDiffProgress } from '../common/reviewProtocol.js'; import type { ReviewFilesEditorEntry } from './reviewFilesDiffView.js'; -export function lensRanges(lens: ReviewDiffLens, entry: ReviewFilesEditorEntry): ReviewDiffLens['ranges'] { - return lens.ranges.filter(range => range.file === (range.side === 'base' ? entry.file.previousPath ?? entry.file.path : entry.file.path)); +export function lensRanges(lens: ReviewDiffLens, entry: ReviewFilesEditorEntry): Extract['ranges'] { + return lens.targets.flatMap(target => target.kind === "ranges" ? [...target.ranges] : []).filter(range => range.file === (range.side === 'base' ? entry.file.previousPath ?? entry.file.path : entry.file.path)); +} + +/** Preserve each result's identity even when several sections show the same file. */ +export function selectLensEntries(entries: readonly ReviewFilesEditorEntry[], lens: ReviewDiffLens | undefined, sections: ReviewDiffProgress["sections"]): readonly ReviewFilesEditorEntry[] { + if (!lens) return entries; + const select = (targets: ReviewDiffLens["targets"]) => { + const results = new Set(targets.flatMap(target => target.kind === "results" ? target.results.map(result => JSON.stringify(result)) : [])); + return entries.flatMap(entry => { + if (entry.evidence) return results.has(JSON.stringify(entry.evidence)) ? [entry] : []; + const ranges = lensRanges({ ...lens, targets }, entry); + if (!ranges.length) return []; + if (lens.wholeFiles) return [entry]; + const original = ranges.some(range => range.side === "base") ? entry.original : undefined; + const modified = ranges.some(range => range.side === "head") ? entry.modified : undefined; + return [{ ...entry, original, modified, goToFileResource: (modified ?? original)! }]; + }); + }; + if (!sections?.length) return select(lens.targets); + return sections.flatMap(section => select(section.targets).map((entry, index) => ({ + ...entry, sectionId: section.id, sectionStart: index === 0, + original: entry.original?.with({ fragment: `${entry.original.fragment}/${section.id}` }), + modified: entry.modified?.with({ fragment: `${entry.modified.fragment}/${section.id}` }), + }))); } export function withLens(instantiation: IInstantiationService, entries: readonly ReviewFilesEditorEntry[], lens: ReviewDiffLens | undefined, lifetime: DisposableStore, progress: () => ReviewDiffProgress | undefined, onProgress: Event): IInstantiationService { @@ -27,7 +50,7 @@ export function withLens(instantiation: IInstantiationService, entries: readonly async computeDiff(original, modified, options, token) { let diff = await provider.computeDiff(original, modified, options, token); const entry = entries.find(entry => entry.original?.toString() === original.uri.toString() || entry.modified?.toString() === modified.uri.toString()); - if (!entry) return diff; + if (!entry || entry.evidence) return diff; const file = progress()?.files.find(file => file.path === entry.file.path); if (file && (!lens || lens.wholeFiles) && !diff.contextGaps) diff = { ...diff, contextGaps: lensContextGaps(diff, original.getLineCount(), modified.getLineCount(), file.changedRanges).map(gap => ({ ...gap, label: 'Unchanged' })) }; if (file) diff = { ...diff, contextGaps: viewedContextGaps(diff, original.getLineCount(), modified.getLineCount(), file.viewedRanges, file.changedRanges) }; @@ -37,7 +60,7 @@ export function withLens(instantiation: IInstantiationService, entries: readonly return count > 0 && range.fromLine < start + count && range.toLine >= start; }) ? { ...gap, collapsed: false } : gap) }; const section = progress()?.sections?.find(section => section.id === entry.sectionId); - return lens && !lens.wholeFiles ? { ...diff, contextGaps: lensContextGaps(diff, original.getLineCount(), modified.getLineCount(), lensRanges(section ? { ...lens, ranges: section.sources } : lens, entry)) } : diff; + return lens && !lens.wholeFiles ? { ...diff, contextGaps: lensContextGaps(diff, original.getLineCount(), modified.getLineCount(), lensRanges(section ? { ...lens, targets: [{kind: "ranges", ranges: section.sources}] } : lens, entry)) } : diff; }, }; }, diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts index b47894033..f1a63623c 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts @@ -20,6 +20,8 @@ import { DetailedLineRangeMapping } from "../../editor/common/diff/rangeMapping. import { autorun, type IObservable } from "../../base/common/observable.js"; import type { UnchangedRegion } from "../../editor/browser/widget/diffEditor/diffEditorViewModel.js"; import { + searchStructuralDiff, + structuralSearchHighlights, structuralContextGaps, structuralFilePath, structuralInitialCounts, @@ -59,16 +61,17 @@ export async function prepareStructuralReview( /** Fires when a file's visible counts change: on arrival and on every fold toggle. */ onDidChangeCounts: Event<{ path: string; counts: StructuralFileCounts }>; }> { - const unchanged = new Set(entries.filter(entry => entry.file.status === "unchanged").map(entry => entry.file.path)); + const unchanged = new Set(entries.filter(entry => !entry.evidence && entry.file.status === "unchanged").map(entry => entry.file.path)); const files = new Map(); const binary = new Set(); - /** Collapse state by `${path}:${fold_state_id}`, seeded from the wire's initial visibility. A fold-state id spans sides. */ + /** Collapse state by editor pair and fold_state_id, seeded from the supplied visibility. A fold-state id spans sides. */ const collapsed = new Map(); const collapseKey = (path: string, foldStateId: number) => `${path}:${foldStateId}`; const countsChanged = lifetime.add(new Emitter<{ path: string; counts: StructuralFileCounts }>()); // Counts are the wire's, read once when a file arrives; folding never changes them. const emitCounts = (path: string) => { - const diff = files.get(path); + const entry = resolvedEntries.find(entry => !entry.evidence && entry.file.path === path); + const diff = entry && files.get(keyOf(entry)); if (!diff) return; countsChanged.fire({ path, counts: structuralInitialCounts(diff) }); }; @@ -85,13 +88,18 @@ export async function prepareStructuralReview( original: entry.original ?? URI.from({ scheme: "review-structural-empty", path: "/base/" + entry.file.path, query: entry.modified!.toString() }), modified: entry.modified ?? URI.from({ scheme: "review-structural-empty", path: "/head/" + entry.file.path, query: entry.original!.toString() }), })); - const pairs = new Map(resolvedEntries.map(e => [e.original!.toString() + "\n" + e.modified!.toString(), e.file.path])); - async function accept(event: Extract): Promise<{ path: string; stats?: StructuralLineCounts }> { + const keyOf = (entry: ReviewFilesEditorEntry) => entry.original!.toString() + "\n" + entry.modified!.toString(); + async function accept(event: Extract, supplied?: (typeof resolvedEntries)[number]): Promise<{ path: string; stats?: StructuralLineCounts }> { const path = structuralFilePath(event.file); - const entry = resolvedEntries.find(e => e.file.path === path); + const entry = supplied ?? resolvedEntries.find(e => !e.evidence && e.file.path === path); if (!entry) throw new Error(`diffr returned an unexpected file: ${path}`); if (event.error) throw new Error(event.error.message); - const diff = event.diff!; + const key = keyOf(entry); + const payload = event.diff!; + const diff = payload.type === "text" ? { ...payload, + lhs: entry.original.scheme === "review-structural-empty" ? undefined : payload.lhs, + rhs: entry.modified.scheme === "review-structural-empty" ? undefined : payload.rhs, + } : payload; if (diff.type === "text") { const sides: [typeof entry.original, StructuralSource | undefined][] = [ [entry.original, diff.lhs], @@ -109,24 +117,46 @@ export async function prepareStructuralReview( if (lifetime.isDisposed) throw new CancellationError(); if (diff.type === "text") { structuralInitialCounts(diff); - files.set(path, diff); + files.set(key, diff); for (const source of [diff.lhs, diff.rhs]) { const seed = (region: StructuralRegion) => { - const key = collapseKey(path, region.fold_state_id); - if (!collapsed.has(key)) collapsed.set(key, region.visibility?.collapsed === true); + const foldKey = collapseKey(key, region.fold_state_id); + if (!collapsed.has(foldKey)) collapsed.set(foldKey, region.visibility?.collapsed === true); if (region.kind === "fold") region.children.forEach(seed); }; (source?.regions ?? []).forEach(seed); } - } else binary.add(path); - emitCounts(path); + } else binary.add(key); + if (!entry.evidence) emitCounts(path); return { path, stats: diff.type === "text" ? diff.stats.textual : undefined }; } async function load( onFile: (path: string, outcome: StructuralFileOutcome) => void, ): Promise { + const retained = new Map(); + for (const entry of resolvedEntries.filter(entry => entry.evidence)) { + try { + const accepted = await accept({ type: "file", file: entry.evidence!.file, diff: searchStructuralDiff(entry.evidence!) }, entry); + if (!retained.has(accepted.path)) retained.set(accepted.path, {}); + } catch (error) { + retained.set(entry.file.path, { error: error instanceof Error ? error.message : String(error) }); + } + } + for (const [path, outcome] of retained) if (!entries.some(entry => !entry.evidence && entry.file.path === path && !unchanged.has(path))) onFile(path, outcome); + for (const entry of resolvedEntries.filter(entry => !entry.evidence && unchanged.has(entry.file.path))) { + const read = async (uri: URI): Promise => { + if (uri.scheme === "review-structural-empty") return undefined; + const reference = lifetime.add(await resolver.createModelReference(uri)); + const text = reference.object.textEditorModel.getValue(); + const lines = text.split("\n"); + return { text, regions: [{ kind: "leaf", id: 0, fold_state_id: 0, alignment_id: 0, start: { line: 0, column: 0 }, end: { line: lines.length - 1, column: new TextEncoder().encode(lines.at(-1)!).length } }] }; + }; + const lhs = await read(entry.original), rhs = await read(entry.modified); + if (lhs && rhs && lhs.text !== rhs.text) throw new Error("Referenced context file changed; reload the review."); + files.set(keyOf(entry), { type: "text", lhs, rhs, stats: { textual: { added: 0, removed: 0 }, visible: { added: 0, removed: 0 } } }); + } for (const path of unchanged) onFile(path, {}); - if (unchanged.size === entries.length) return; + if (entries.every(entry => entry.evidence || unchanged.has(entry.file.path))) return; const abort = new AbortController(); lifetime.add(toDisposable(() => abort.abort())); const response = await request(abort.signal); @@ -146,12 +176,14 @@ export async function prepareStructuralReview( started = true; } else if (event.type === "file") { const path = structuralFilePath(event.file); + if (!entries.some(entry => !entry.evidence && entry.file.path === path && !unchanged.has(path))) return; seen.add(path); try { - const accepted = await accept(event); - onFile(accepted.path, { - stats: accepted.stats, - hidden: event.visibility?.collapsed ? event.visibility.label || "Hidden by default" : undefined, + const accepted = await Promise.all(resolvedEntries.filter(entry => !entry.evidence && entry.file.path === path).map(entry => accept(event, entry))); + onFile(path, { + ...retained.get(path), + stats: accepted[0]?.stats, + hidden: !retained.has(path) && event.visibility?.collapsed ? event.visibility.label || "Hidden by default" : undefined, }); } catch (error) { if (lifetime.isDisposed) throw error; @@ -175,7 +207,7 @@ export async function prepareStructuralReview( } await line(buffer); if (!complete) throw new Error("diffr stream ended before completion."); - for (const entry of entries) if (!seen.has(entry.file.path) && !unchanged.has(entry.file.path)) + for (const entry of entries) if (!entry.evidence && !seen.has(entry.file.path) && !unchanged.has(entry.file.path)) onFile(entry.file.path, { error: "diffr did not supply a result for this file." }); } finally { await reader.cancel().catch(() => {}); @@ -192,15 +224,11 @@ export async function prepareStructuralReview( onDidChange: providerChanged.event, async computeDiff(original, modified, _options, token) { if (token.isCancellationRequested) throw new CancellationError(); - const path = pairs.get(original.uri.with({ fragment: "" }).toString() + "\n" + modified.uri.with({ fragment: "" }).toString()); - if (path !== undefined && unchanged.has(path)) { - if (original.getValue() !== modified.getValue()) throw new Error("Referenced context file changed; reload the review."); - return { changes: [], moves: [], identical: true, quitEarly: false }; - } - if (path !== undefined && binary.has(path)) { + const path = original.uri.toString() + "\n" + modified.uri.toString(); + if (binary.has(path)) { return { changes: [], moves: [], identical: false, quitEarly: false, changeHighlights: { original: [], modified: [] } }; } - const diff = path === undefined ? undefined : files.get(path); + const diff = files.get(path); if (!diff) throw new Error("diffr did not supply a result for this file."); const left = (diff.lhs?.text ?? "").replace(/\r\n/g, "\n"); const right = (diff.rhs?.text ?? "").replace(/\r\n/g, "\n"); @@ -248,8 +276,8 @@ export async function prepareStructuralReview( // Every collapsed region is a hidden-region band, labelled by the wire. contextGaps: structuralContextGaps( diff, - (id) => collapsed.get(collapseKey(path!, id)) === true, - (id) => collapsed.get(collapseKey(path!, id)), + (id) => collapsed.get(collapseKey(path, id)) === true, + (id) => collapsed.get(collapseKey(path, id)), ), changeHighlights: highlights, }; @@ -260,7 +288,7 @@ export async function prepareStructuralReview( const child = lifetime.add( instantiation.createChild(new ServiceCollection([IDiffProviderFactoryService, factory])), ); - attachStructuralEditors(instantiation, resolvedEntries, files, lifetime, { + attachStructuralEditors(instantiation, files, lifetime, { get: (path, id) => collapsed.get(collapseKey(path, id)), set: (path, id, value) => { if (collapsed.get(collapseKey(path, id)) === value) return; @@ -278,25 +306,29 @@ export async function prepareStructuralReview( */ function attachStructuralEditors( instantiation: IInstantiationService, - entries: readonly ReviewFilesEditorEntry[], files: Map, lifetime: DisposableStore, collapsed: CollapseState, ): void { const editors = instantiation.invokeFunction((a) => a.get(ICodeEditorService)); - const pairs = new Map(entries.map((e) => [e.original!.toString() + "\n" + e.modified!.toString(), e.file.path])); function watch(editor: IDiffEditor) { const store = lifetime.add(new DisposableStore()); store.add(editor.onDidDispose(() => store.dispose())); const widget = editor as unknown as { unchangedRegions?: IObservable }; if (!widget.unchangedRegions) return; + const decorations = [editor.getOriginalEditor().createDecorationsCollection(), editor.getModifiedEditor().createDecorationsCollection()]; + store.add(toDisposable(() => decorations.forEach(decoration => decoration.clear()))); let revealed = new Set(); store.add( autorun((reader) => { const model = editor.getModel(); - const path = model && pairs.get(model.original.uri.with({ fragment: "" }).toString() + "\n" + model.modified.uri.with({ fragment: "" }).toString()); + const path = model && model.original.uri.toString() + "\n" + model.modified.uri.toString(); const regions = widget.unchangedRegions!.read(reader); - if (!path || !files.has(path)) return; + if (!path || !files.has(path)) { decorations.forEach(decoration => decoration.clear()); return; } + const diff = files.get(path)!; + [diff.lhs, diff.rhs].forEach((source, side) => decorations[side].set(structuralSearchHighlights(source).map(range => ({ + range, options: { description: "Review search highlight", inlineClassName: "review-search-highlight" }, + })))); const gaps = structuralContextGaps(files.get(path)!, (id) => collapsed.get(path, id) === true, (id) => collapsed.get(path, id)); const next = new Set(); const gapOf = (region: UnchangedRegion) => @@ -325,7 +357,7 @@ function attachStructuralEditors( for (const editor of editors.listDiffEditors()) watch(editor); } -/** Collapse state per file, keyed by fold-state id. */ +/** Collapse state per editor pair, keyed by fold-state id. */ interface CollapseState { get(path: string, foldStateId: number): boolean | undefined; set(path: string, foldStateId: number, value: boolean): void; From 1c94555451e0a0f0628ede031e2f80a4df58e157 Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Sat, 19 Sep 2026 14:49:38 -0700 Subject: [PATCH 3/3] Verify structural evidence rendering and record live E2E results Cover shared, paired and one-sided evidence, structural folds, navigation and lens propagation. Retain the prior live-dev-build verification record with its original tested commit identities; the history rewrite preserves all application code. AI assistance: reorganized with Codex. --- .../vs/review/common/reviewLensFiles.test.ts | 6 +- .../common/reviewSearchEvidence.test.ts | 107 ++++++++++++++++++ .../common/reviewStructuralDiff.test.ts | 18 +++ .../services/reviewApiSourceService.test.ts | 9 +- .../src/vs/review/services/reviewLens.test.ts | 28 +++++ docs/plans/diffr-evidence-verification.md | 75 ++++++++++++ 6 files changed, 236 insertions(+), 7 deletions(-) create mode 100644 apps/review-desktop/code-oss/src/vs/review/common/reviewSearchEvidence.test.ts create mode 100644 apps/review-desktop/code-oss/src/vs/review/services/reviewLens.test.ts create mode 100644 docs/plans/diffr-evidence-verification.md diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewLensFiles.test.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewLensFiles.test.ts index a7b786b00..8278b5923 100644 --- a/apps/review-desktop/code-oss/src/vs/review/common/reviewLensFiles.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewLensFiles.test.ts @@ -5,11 +5,11 @@ import { lensContextGaps } from './reviewLens.js'; import type { ReviewDiffFileWire, ReviewDiffLens } from './reviewProtocol.js'; const files: ReviewDiffFileWire[] = [{ path: 'renamed.ts', previousPath: 'old.ts', status: 'renamed', additions: 2, deletions: 1 }]; -const lens: ReviewDiffLens = { id: 'lens', title: 'Context', reviewId: 'review', version: 1, ranges: [ +const lens: ReviewDiffLens = { id: 'lens', title: 'Context', reviewId: 'review', version: 1, targets: [{ kind: 'ranges', ranges: [ { file: 'old.ts', side: 'base', fromLine: 2, toLine: 5 }, { file: 'context.ts', side: 'base', fromLine: 10, toLine: 12 }, { file: 'context.ts', side: 'head', fromLine: 10, toLine: 12 }, -] }; +] }] }; test('diagram references add each unchanged file once, preserving rename identity', () => { const result = lensFiles(files, lens); @@ -23,6 +23,6 @@ test('clearing the lens and whole-file glob lenses do not introduce context file }); test('an identical file exposes the referenced slice with three surrounding lines', () => { - const gaps = lensContextGaps({ changes: [], moves: [], identical: true, quitEarly: false }, 30, 30, lens.ranges.filter(range => range.file === 'context.ts')); + const gaps = lensContextGaps({ changes: [], moves: [], identical: true, quitEarly: false }, 30, 30, lens.targets.flatMap(target => target.kind === 'ranges' ? target.ranges : []).filter(range => range.file === 'context.ts')); assert.deepEqual(gaps.map(gap => [gap.originalStart, gap.originalCount, gap.modifiedStart, gap.modifiedCount]), [[1, 6, 1, 6], [16, 15, 16, 15]]); }); diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewSearchEvidence.test.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewSearchEvidence.test.ts new file mode 100644 index 000000000..d86de5785 --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewSearchEvidence.test.ts @@ -0,0 +1,107 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import type { SearchResultData, SourceData } from "./reviewProtocol.js"; +import { evidenceRows } from "./reviewSearchEvidence.js"; + +const source = (text: string): SourceData => ({ + text, + regions: [ + { + kind: "leaf", + id: 1, + fold_state_id: 1, + alignment_id: 1, + start: { line: 0, column: 0 }, + end: { line: 1, column: 0 }, + search_highlights: [{ line: 0, start_column: 2, end_column: 6 }], + }, + { + kind: "fold", + id: 2, + fold_state_id: 2, + start: { line: 1, column: 0 }, + end: { line: 2, column: 0 }, + visibility: { collapsed: true }, + children: [ + { + kind: "leaf", + id: 3, + fold_state_id: 3, + alignment_id: 3, + start: { line: 1, column: 0 }, + end: { line: 2, column: 0 }, + }, + ], + }, + ], +}); +function result(): SearchResultData { + return { + display: "rhs", + scope: { + repo: "/unused", + baseWorktree: { commitId: "base", path: "/unused/base" }, + headWorktree: { commitId: "head", path: "/unused/head" }, + }, + file: { rhs: { path: "example.ts", oid: "a".repeat(40), mode: "100644" } }, + sources: { rhs: source("é🔎 match\nhidden\n") }, + }; +} + +test("head-only evidence preserves its lines, UTF-8 highlights, and nested folded context", () => { + const evidence = result(); + const rows = evidenceRows(evidence); + assert.ok(rows.every((row) => row.baseLine === undefined)); + assert.equal(rows[0].headLine, 1); + assert.deepEqual(rows[0].highlights, [{ startColumn: 2, endColumn: 4 }]); + assert.ok(!rows.some((row) => row.content === "hidden")); + const fold = evidence.sources.rhs!.regions[1]; + fold.visibility = { collapsed: false }; + const expanded = evidenceRows(evidence); + assert.equal(expanded[1].content, "hidden"); + assert.equal(expanded[1].headLine, 2); + assert.equal(expanded[1].fold?.id, 2); +}); + +test("paired changed lines remain separate while unchanged aligned lines merge", () => { + const evidence = result(); + evidence.display = "both"; + evidence.file = { lhs: evidence.file.rhs!, rhs: evidence.file.rhs! }; + evidence.sources = { + lhs: source("é🔎 match\nhidden\n"), + rhs: evidence.sources.rhs!, + }; + assert.equal(evidenceRows(evidence)[0].baseLine, 1); + assert.equal(evidenceRows(evidence)[0].headLine, 1); + const leaf = evidence.sources.lhs!.regions[0]; + if (leaf.kind !== "leaf") throw new Error("Expected leaf"); + leaf.changed = [{ line: 0, start_column: 0, end_column: 2 }]; + const rows = evidenceRows(evidence); + assert.equal(rows[0].kind, "deleted"); + assert.equal(rows[0].headLine, undefined); + assert.equal(rows[1].baseLine, undefined); +}); + + +test("display hides the counterpart without losing it, and shared sources keep folds", () => { + const single = result(); + const shared: SearchResultData = { + scope: single.scope, display: "rhs", + file: {lhs: single.file.rhs!, rhs: single.file.rhs!}, + sources: {same: single.sources.rhs!}, + }; + assert.ok(evidenceRows(shared).every(row => row.baseLine === undefined)); + assert.equal(evidenceRows(shared)[1].fold?.collapsed, true); + shared.display = "both"; + assert.equal(evidenceRows(shared)[0].baseLine, 1); + assert.equal(evidenceRows(shared)[0].headLine, 1); + assert.equal(evidenceRows(shared)[1].fold?.collapsed, true); + const paired: SearchResultData = {...shared, display: "lhs", sources: { + lhs: source("before\nhidden\n"), rhs: source("after\nhidden\n"), + }}; + assert.ok(evidenceRows(paired).every(row => row.headLine === undefined)); + assert.equal(evidenceRows(paired)[0].content, "before"); + paired.display = "rhs"; + assert.equal(evidenceRows(paired)[0].content, "after"); +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts index 016d601c9..5eb5a9f1e 100644 --- a/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts @@ -10,6 +10,8 @@ import { collapsedRegions, hiddenLinesOf, structuralContextGaps, + searchStructuralDiff, + structuralSearchHighlights, bandDetail, structuralCountsTooltip, structuralInitialCounts, @@ -325,3 +327,19 @@ test("a fold pairs through fold state, and a docstring fold only with a docstrin const unpaired: StructuralTextDiff = { ...diff, rhs: text(lines(4), [other]) }; assert.deepEqual(structuralContextGaps(unpaired, (id) => id === 9 || id === 10).map((g) => g.kind).sort(), ["inserted", "removed", "removed"]); }); + + +test("head-only search evidence retains native pseudocode bands and UTF-16 highlight positions", () => { + const body = fold(4, [leaf(5, 1, 3, { changed: [{ line: 1, start_column: 0, end_column: 6 }] })]); + body.visibility = { collapsed: true, label: "// pseudocode\nreturn cached value" }; + const source = text(["😀match", "hidden", "return"], [leaf(1, 0, 1, { search_highlights: [{ line: 0, start_column: 4, end_column: 9 }] }), body]); + const diff = searchStructuralDiff({ display: "rhs", scope: { repo: "/unused", baseWorktree: { commitId: "base", path: "/unused" }, headWorktree: { commitId: "head", path: "/unused" } }, file: { rhs: { path: "x.ts", oid: "a".repeat(40), mode: "100644" } }, sources: { rhs: source } }); + assert.ok(structuralRows(diff).every(([left]) => left === null)); + assert.deepEqual(structuralSearchHighlights(diff.rhs), [{ startLineNumber: 1, endLineNumber: 1, startColumn: 3, endColumn: 8 }]); + const [band] = structuralContextGaps(diff, id => id === 4); + assert.equal(band.originalCount, 0); + assert.equal(band.modifiedCount, 2); + assert.equal(bandDetail(band.label), "return cached value"); + assert.equal(structuralInitialCounts(diff).visible.added, 0); + assert.equal(structuralInitialCounts(diff).textual.added, 1); +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.test.ts index d6e038200..0904ad121 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewApiSourceService.test.ts @@ -87,12 +87,13 @@ test("a native peek reads the pinned version through the authenticated API, not } as never, {} as never, ); - canvas.inlineEditors.create({ + canvas.inlineEditors.create({content: {kind: "source", path: "src/[route].ts", side: "base", ranges: [{ startLine: 2, endLine: 2 }], - } as never); + }} as never); version = 4; + assert.equal(await source.diff(), undefined); const snippet = await source.snippet(); assert.equal(models.get(snippet.target.resource.toString())?.text, "old first line\nold second line"); snippet.dispose(); @@ -171,11 +172,11 @@ test("unavailable pinned files report the API error instead of falling back to d } as never, {} as never, ); - canvas.inlineEditors.create({ + canvas.inlineEditors.create({content: {kind: "source", path: "missing.ts", side: "head", ranges: [{ startLine: 1, endLine: 1 }], - } as never); + }} as never); await assert.rejects(source.snippet(), /unavailable at the pinned commit/); assert.equal(disposed(), 0); }); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewLens.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewLens.test.ts new file mode 100644 index 000000000..daeacdaa0 --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewLens.test.ts @@ -0,0 +1,28 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { URI } from '../../base/common/uri.js'; +import { selectLensEntries } from './reviewLens.js'; +import type { ReviewDiffLens, ReviewDiffProgress, SearchResultData } from '../common/reviewProtocol.js'; +import type { ReviewFilesEditorEntry } from './reviewFilesDiffView.js'; + +test('sections select their retained result after JSON transport without sharing editor identity', () => { + const result: SearchResultData = { + display: 'rhs', scope: { repo: '/repo', baseWorktree: { path: '/base', commitId: 'base' }, headWorktree: { path: '/head', commitId: 'head' } }, + file: { rhs: { path: 'same.ts', oid: 'a'.repeat(40), mode: '100644' } }, + sources: { rhs: { text: 'selected', regions: [] } }, + }; + const original = URI.parse('review-api-source://review/same.ts?side=base'); + const modified = URI.parse('review-api-source://review/same.ts?side=head'); + const entries: ReviewFilesEditorEntry[] = [{ file: { path: 'same.ts', status: 'modified', additions: 1, deletions: 1 }, original: undefined, modified, goToFileResource: modified, evidence: result }]; + const lens: ReviewDiffLens = { id: 'lens', reviewId: 'review', title: 'Results', version: 1, targets: [{ kind: 'results', results: [result] }] }; + const sections: ReviewDiffProgress['sections'] = ['first', 'second'].map(id => ({ id, label: id, sources: [], state: 'unread', total: { additions: 1, deletions: 0 }, remaining: { additions: 1, deletions: 0 }, targets: JSON.parse(JSON.stringify(lens.targets)) })); + const selected = selectLensEntries(entries, lens, sections); + assert.equal(selected.length, 2); + assert.ok(selected.every(entry => entry.original === undefined)); + assert.notEqual(selected[0].modified!.toString(), selected[1].modified!.toString()); + assert.deepEqual(selected.map(entry => entry.sectionId), ['first', 'second']); + + const ranged = selectLensEntries([{ ...entries[0], evidence: undefined, original }], { ...lens, targets: [{ kind: 'ranges', ranges: [{ file: 'same.ts', side: 'base', fromLine: 1, toLine: 1 }] }] }, undefined); + assert.equal(ranged[0].modified, undefined); + assert.equal(ranged[0].goToFileResource.toString(), original.toString()); +}); diff --git a/docs/plans/diffr-evidence-verification.md b/docs/plans/diffr-evidence-verification.md new file mode 100644 index 000000000..1d481a0c9 --- /dev/null +++ b/docs/plans/diffr-evidence-verification.md @@ -0,0 +1,75 @@ +# Direct diffr evidence: verification + +Review's contract scaffold is `bb635df4`; implementation starts with `2a7b8dfb`. Diffr's contract scaffold is `a4f76a4ad`; implementation starts with `71d8db355`. + +## Automated checks + +- 101 focused Review tests pass, including persistence/reopen, invalid pins/text/blob rejection, stale evidence after repin, mixed glob/result targets, authoring API behavior, and lens UI lifecycle. +- 132 native tests pass, including UTF-8 highlight conversion, one-sided rows, nested fold expansion, paired changed lines, pinned source reads, and separate evidence/navigation/viewed actions for two sections of the same file. The desktop script tests also pass. +- All 9 diffr code-mode tests pass. Existing pretty-output snapshots are unchanged; the new schema test round-trips real hydrated and postprocessed results. +- Review, native, and diffr TypeScript checks pass. The dev build succeeds. + +## Real API-to-app run + +`diffr-search/tests/code-mode/review.live.ts` queried `Diffr evidence text differs` in this implementation's `local-data.ts`, comparing `e54665e3` with `90d7288c`. It used the real diffr JS binding, hydrated and postprocessed the query, and submitted structured results through Review's HTTP authoring API. It also selected a base-side result and a wholly unchanged `AGENTS.md` result. + +The saved review includes a head-only excerpt, head-only and paired lenses, a mixed file-glob/result lens, an unchanged-file excerpt, and two diagram steps with distinct side selections on the same file. The script removed the query worktrees before reading back and opening the saved document. + +The completed dev Review is `cc4f1cc1-1fbf-40d1-b42c-df4dafece6e6`, version 7. It remains open in the isolated `/tmp/review-evidence-dev` app instance. Local artifacts are in `/tmp/review-evidence-e2e-final/` (`evidence.json`, `pretty.txt`, `review.json`), with no server credentials. + +Computer-use verification confirmed: + +- Head-only code displays with search highlighting; Open File selects its matched line 739 in pinned source. +- The mixed lens retains both the glob-selected `source.ts` and the supplied search results, including unchanged `AGENTS.md`. +- The diagram has separate head-only and paired sections; selecting the paired step navigates to its section. +- Marking the head-only section viewed marks 76 added lines and leaves all five base deletions unread. The paired section changes to +0/-5. Viewed state was reset afterward. +- Earlier checks confirmed local nested fold toggling, find over visible evidence, and paired deleted/added rows. + +The larger two-result sequence initially hit the old 1 MiB authoring limit. The command endpoint now permits 8 MiB, matching resource uploads; the unchanged live test then passed. No evidence was truncated or replaced with ranges to pass the test. + +## Contracts and remaining environment limitation + +The approved `targets` ADT now flows through `DiagramLens` and native `ReviewDiffSection`. `sources` remains a coverage/navigation projection. No SQL schema, computed-diff cache schema, upload requirement, or evidence wire-field rename was introduced. The shared diffr contract is installed from a pinned Git revision. + +The dev app reports a language-environment preparation failure on both tested revisions: its setup runs the review-protocol build before the `@dev.fast/json` and trace-protocol declaration outputs exist. Pinned-source opening and line selection were verified despite this warning; LSP definition navigation is not claimed as verified. + +## Review order + +- diffr: #6 contracts/output → #7 plugins → #20 Rust storage → #8 search/bindings → #10 Jev → [#13 Review evidence](https://github.com/devdotfast/diffr/pull/13) +- Review: [#369 contracts](https://github.com/devdotfast/review/pull/369) → [#381 validation/storage](https://github.com/devdotfast/review/pull/381) → [#370 rendering](https://github.com/devdotfast/review/pull/370) + +Both are draft stacks. History now groups the final contracts, consumers, and implementations by system boundary; superseded approaches and follow-up corrections are folded into their owning changes. The rewritten diffr tip has the exact original Git tree; Review application code is unchanged, with its dependency repinned to that rewritten tip. The verification identities below record the original runs. + +## Structural Diff renderer correction + +The initial integration routed explicit lenses through an inline-editor container. That preserved evidence coordinates but bypassed the established structural Diff UI; the initial E2E did not establish UI parity. This path, its bespoke toolbar/file list, and its implementation-coupled DOM test have been removed. + +Both retained results and ordinary range lenses now use ReviewFilesDiffView and the existing structural provider. Retained trees seed the existing alignment, fold bands, labels/pseudocode, and collapse controls. Search decorations are blue. Model-pair identity separates two same-file results/sections. Explicit side selection is preserved, with a native internal forceInline option preventing an empty counterpart pane. No public evidence DTO or stored schema changed. + +Validation: 133 native tests and native typechecking pass; the dev build succeeds. The new checks cover head-only pseudocode bands and UTF-16 search spans, JSON-transported section targets, and one-sided range navigation. Visual checks against the saved real search results confirmed the native file header, fold expansion, blue highlight, full-width head-only layout, mixed targets (three files), and navigation between the head-only and paired diagram sections. Pseudocode label conversion is covered by tests; the saved query's labels are unchanged-line summaries. + + +## Complete comparisons and shared unchanged sources + +Diffr now retains complete comparison trees through hydration and postprocessing. +The public Comparison union couples file references to sources, stores unchanged +content once as `sources.same`, and uses `display` solely for presentation. Review +validates both retained sides and upgrades older saved `kind` payloads on read. +It keeps using the existing structural Diff component; no replacement view was added. + +Validated against diffr `9516f12f4` and Review `b1c608ee`: 262 Rust library tests, +11 JS integration tests (unchanged pretty-output snapshot), 68 focused Review tests, +20 native presentation tests, and TypeScript checks. The dev build succeeds. + +The live query selected evidence from Review's own `e54665e3..b1c608ee` diff, +submitted head-only, paired, shared-unchanged and base-only displays, removed the +query worktrees, and verified the saved evidence round trip. Review +`0aabeb9f-e593-4b98-bf8c-dad4dbad66f4`, version 7, remains in the isolated dev app. +Artifacts: `/tmp/review-evidence-comparison/{evidence.json,pretty.txt,review.json}`. +Computer-use checks confirmed the full-width head-only structural view, blue +matched-line highlight, native fold expansion revealing retained lines 9–24, +and shared unchanged AGENTS.md displayed in the mixed-results lens. + +The separate language-environment warning remains. A normal whole-file entry in +the mixed lens also reported a busy pinned checkout during preparation; retained +search evidence displayed successfully. This check does not establish LSP readiness.