Let a sequence step show several code ranges

apps/review-desktop/code-oss/src/vs/review/common/reviewLens.test.ts

@@ -4,6 +4,7 @@ import { lensContextGaps } from './reviewLens.js';
 import { LineRange } from '../../editor/common/core/ranges/lineRange.js';
 import { DetailedLineRangeMapping } from '../../editor/common/diff/rangeMapping.js';
 import type { IDocumentContextGap, IDocumentDiff } from '../../editor/common/diff/documentDiffProvider.js';
+import { structuralContextScopes, type StructuralLeaf } from './reviewStructuralDiff.js';
 
 const plain: IDocumentDiff = { changes: [], moves: [], identical: false, quitEarly: false };
 test('a lens keeps disjoint attachments and hides the intervening code', () => {
@@ -136,3 +137,76 @@ test('an identical file exposes the referenced slice with three surrounding line
 	]);
 	assert.deepEqual(gaps.map(gap => [gap.originalStart, gap.originalCount, gap.modifiedStart, gap.modifiedCount]), [[1, 6, 1, 6], [16, 15, 16, 15]]);
 });
+
+// A modified file shaped like a schema declaration, the same on both sides
+// except for lines 1-3 and 23-24: an import block on lines 1-11 that diffr
+// leaves open, `export const schema = z` / `.strictObject({` on lines 15-16
+// opening a scope that closes on line 40, and an unchanged run on lines
+// 41-50 that diffr also leaves open. Diffr collapses lines 17-19, 28-39 and
+// 51-60.
+const schemaDiff = (contextScopes: IDocumentDiff['contextScopes']): IDocumentDiff => ({
+	...plain,
+	changes: [
+		new DetailedLineRangeMapping(new LineRange(1, 4), new LineRange(1, 4), undefined),
+		new DetailedLineRangeMapping(new LineRange(23, 25), new LineRange(23, 25), undefined),
+	],
+	contextScopes,
+	contextGaps: [[17, 3], [28, 12], [51, 10]].map(([start, count]) =>
+		({ originalStart: start, originalCount: count, modifiedStart: start, modifiedCount: count, collapsed: true })),
+});
+const schemaScope: [number, number] = [14, 40];
+
+test('a code peek never treats the file as a scope around its pin', () => {
+	// A tagged root, as a plugin could leave it, around the schema's scope.
+	const lines = Array.from({ length: 60 }, (_, i) => `line ${i + 1}`);
+	const at = (line: number) => ({ line, column: 0 });
+	const leaf = (id: number, start: number, end: number): StructuralLeaf =>
+		({ id, alignment_id: id, fold_state_id: id, kind: 'leaf', start: at(start), end: at(end) });
+	const source = {
+		text: lines.join('\n') + '\n',
+		root: {
+			id: 1, fold_state_id: 1, kind: 'fold' as const, tags: ['context:scope'], start: at(0), end: at(60), indent: at(0), children: [
+				leaf(2, 0, 14),
+				{ id: 3, fold_state_id: 3, kind: 'fold' as const, tags: ['context:scope'], start: at(14), end: at(40), indent: at(14), children: [leaf(4, 14, 40)] },
+				leaf(5, 40, 60),
+			],
+		},
+	};
+	const scopes = structuralContextScopes({ type: 'text', stats: { textual: { added: 0, removed: 0 }, visible: { added: 0, removed: 0 } }, structural_changes: { base: [], head: [] }, lhs: source, rhs: source });
+	assert.deepEqual(scopes, { original: [schemaScope], modified: [schemaScope] });
+	const shown = shownLines(lensContextGaps(schemaDiff(scopes), 60, 60, head(23, 24)), 60);
+	assert.ok(shown.every(([from]) => from > 11), JSON.stringify(shown));
+});
+
+test('a code peek shows a scope boundary, not the unchanged code beyond it', () => {
+	const diff = schemaDiff({ original: [schemaScope], modified: [schemaScope] });
+	// The header's second line is kept, as diffr keeps it, up to the first fold.
+	assert.deepEqual(shownLines(lensContextGaps(diff, 60, 60, head(23, 24)), 60), [[15, 16], [20, 27], [40, 40]]);
+	// A base-side pin projects the same way.
+	const base = lensContextGaps(diff, 60, 60, [{ side: 'base', file: 'a.ts', fromLine: 23, toLine: 24 }]);
+	assert.deepEqual(shownLines(base, 60), [[15, 16], [20, 27], [40, 40]]);
+});
+
+test('a code peek inside nested scopes shows each header and closer, and leaves diffr\'s folds folded', () => {
+	// A class on lines 1-40, a method on 6-35, an if on 14-28; the pin is line 21.
+	const fold = (start: number, count: number) => ({ originalStart: start, originalCount: count, modifiedStart: start, modifiedCount: count, collapsed: true });
+	const diff: IDocumentDiff = {
+		...plain,
+		changes: [new DetailedLineRangeMapping(new LineRange(21, 22), new LineRange(21, 22), undefined)],
+		contextScopes: { original: [[0, 40], [5, 35], [13, 28]], modified: [[0, 40], [5, 35], [13, 28]] },
+		contextGaps: [fold(2, 4), fold(7, 7), fold(15, 3), fold(25, 3), fold(29, 6), fold(36, 4)],
+	};
+	assert.deepEqual(shownLines(lensContextGaps(diff, 40, 40, head(21, 21)), 40), [[1, 1], [6, 6], [14, 14], [18, 24], [28, 28], [35, 35], [40, 40]]);
+});
+
+test('a code peek spanning two functions shows both of their boundaries', () => {
+	// Functions on lines 3-20 and 22-40, each body collapsed away from the pin on lines 19-23.
+	const fold = (start: number, count: number) => ({ originalStart: start, originalCount: count, modifiedStart: start, modifiedCount: count, collapsed: true });
+	const diff: IDocumentDiff = {
+		...plain,
+		changes: [new DetailedLineRangeMapping(new LineRange(19, 24), new LineRange(19, 24), undefined)],
+		contextScopes: { original: [[2, 20], [21, 40]], modified: [[2, 20], [21, 40]] },
+		contextGaps: [fold(1, 2), fold(4, 12), fold(27, 13)],
+	};
+	assert.deepEqual(shownLines(lensContextGaps(diff, 40, 40, head(19, 23)), 40), [[3, 3], [16, 26], [40, 40]]);
+});

apps/review-desktop/code-oss/src/vs/review/common/reviewLens.ts

@@ -16,10 +16,12 @@ export function lensContextGaps(diff: IDocumentDiff, originalCount: number, modi
 	const context = visible.map((_, index) => visible.slice(Math.max(0, index - 3), index + 4).some(Boolean));
 	if (diff.contextScopes) {
 		// A lens must not clip context that diffr kept around its selected code.
-		// Seed enclosing scope boundaries, then retain the unchanged lines of each
-		// visible context run touching a seed. Never expand through a collapsed
-		// provider band, and never into changed lines: diffr never folds those,
-		// so a run that holds them is not context diffr chose to keep.
+		// Retain the unchanged lines of each open run (no collapsed provider
+		// band) holding pinned lines. Show each enclosing scope's first and
+		// last line, plus the unchanged lines diffr kept between it and the
+		// pin, such as the rest of a multi-line header. Never fill away from
+		// the pin, and never into changed lines: diffr never folds those, so
+		// a run that holds them is not context diffr chose to keep.
 		const scopes = [diff.contextScopes.original, diff.contextScopes.modified];
 		const enclosing = scopes.map((sideScopes, side) => sideScopes.filter(([start, end]) =>
 			rows.some((row, index) => visible[index] && row[side] !== null && row[side]! >= start && row[side]! < end)));
@@ -31,8 +33,6 @@ export function lensContextGaps(diff: IDocumentDiff, originalCount: number, modi
 			const count = side === 0 ? gap.originalCount : gap.modifiedCount;
 			return line + 1 >= start && line + 1 < start + count;
 		})));
-		const seeds = rows.map((row, index) => visible[index] || row.some((line, side) =>
-			line !== null && enclosing[side].some(([start, end]) => line === start || line === end - 1)));
 		const edited = [new Uint8Array(originalCount), new Uint8Array(modifiedCount)];
 		for (const change of diff.changes) {
 			edited[0].fill(1, change.original.startLineNumber - 1, change.original.endLineNumberExclusive - 1);
@@ -43,9 +43,19 @@ export function lensContextGaps(diff: IDocumentDiff, originalCount: number, modi
 			if (!open[start]) { start++; continue; }
 			let end = start + 1;
 			while (end < rows.length && open[end]) end++;
-			if (seeds.slice(start, end).some(Boolean)) for (let index = start; index < end; index++) context[index] ||= seeds[index] || unchanged[index];
+			if (visible.slice(start, end).some(Boolean)) for (let index = start; index < end; index++) context[index] ||= unchanged[index];
 			start = end;
 		}
+		// A boundary inside a collapsed band stays folded with it.
+		const boundary = (index: number, edge: 'start' | 'end') => open[index] && rows[index].some((line, side) =>
+			line !== null && enclosing[side].some(([start, end]) => line === (edge === 'start' ? start : end - 1)));
+		for (let index = 0; index < rows.length; index++) {
+			for (const [edge, step] of [['start', 1], ['end', -1]] as const) {
+				if (!boundary(index, edge)) continue;
+				context[index] = true;
+				for (let next = index + step; next >= 0 && next < rows.length && !context[next] && open[next] && unchanged[next]; next += step) context[next] = true;
+			}
+		}
 	}
 	const gaps: IDocumentContextGap[] = [];
 	let left = 1, right = 1;

apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.ts

@@ -79,7 +79,8 @@ export function structuralContextScopes(diff: StructuralTextDiff) {
 			}
 			region.children.forEach(visit);
 		};
-		if (source) visit(source.root);
+		// The root is the file itself, not a construct around the pinned code.
+		if (source?.root.kind === "fold") source.root.children.forEach(visit);
 		return result;
 	};
 	return { original: scopes(diff.lhs), modified: scopes(diff.rhs) };

packages/review/app/src/CodePeek.browser.test.tsx

@@ -3,17 +3,24 @@ import type {
   ReviewVerbRequest,
 } from "@dev.fast/review-protocol";
 import { selectSource, sourceAnchors } from "@review/lens-selection";
+import * as stylex from "@stylexjs/stylex";
 import { type ReactNode, act } from "react";
 import { createRoot } from "react-dom/client";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 
-import { CodePeek, CodePeekCard, CodePeekGroup } from "./CodePeek";
+import {
+  CodePeek,
+  CodePeekCard,
+  CodePeekGroup,
+  SourceCodePeek,
+} from "./CodePeek";
 import {
   type ReviewSession,
   ReviewSessionProvider,
 } from "./host/review-session";
 import type { ReviewLensView } from "./review-lenses";
 import { testReviewSession } from "./review-session-test-utils";
+import { tourStyles } from "./side-panel-styles";
 
 const testLenses: ReviewLensView = {
   progress: null,
@@ -216,6 +223,86 @@ describe("CodePeek native editor", () => {
     });
   });
 
+  it("shows each of a step's ranges as its own card, in order, clipped to its lines", async () => {
+    const range = (file: string, fromLine: number, toLine: number) =>
+      selectSource({ side: "head", file, fromLine, toLine });
+
+    const container = document.createElement("div");
+    document.body.append(container);
+    root = createRoot(container);
+
+    await act(async () =>
+      renderWithSession(
+        <SourceCodePeek
+          sources={[
+            range("src/store.ts", 412, 413),
+            range("src/store.ts", 380, 391),
+            range("src/config.ts", 41, 44),
+          ]}
+          lenses={testLenses}
+        />,
+      ),
+    );
+
+    await vi.waitFor(() => {
+      expect(created).toMatchObject([
+        { path: "src/store.ts", ranges: [{ startLine: 412, endLine: 413 }] },
+        { path: "src/store.ts", ranges: [{ startLine: 380, endLine: 391 }] },
+        { path: "src/config.ts", ranges: [{ startLine: 41, endLine: 44 }] },
+      ]);
+    });
+    // Plain cards: the editor draws each header, so no header row of ours.
+    expect(container.querySelector("button")).toBeNull();
+  });
+
+  it("spaces a tour step's cards apart and leaves a lone peek's cards flush", async () => {
+    const range = (file: string, fromLine: number, toLine: number) =>
+      selectSource({ side: "head", file, fromLine, toLine });
+
+    const sources = [
+      range("src/store.ts", 412, 413),
+      range("src/config.ts", 41, 44),
+    ];
+
+    const container = document.createElement("div");
+    document.body.append(container);
+    root = createRoot(container);
+
+    await act(async () =>
+      renderWithSession(
+        <>
+          <div data-host="tour" {...stylex.props(tourStyles.content)}>
+            <SourceCodePeek sources={sources} lenses={testLenses} />
+          </div>
+          <div data-host="tour-single" {...stylex.props(tourStyles.content)}>
+            <SourceCodePeek sources={sources.slice(0, 1)} lenses={testLenses} />
+          </div>
+          <div data-host="peek">
+            <SourceCodePeek sources={sources} lenses={testLenses} />
+          </div>
+        </>,
+      ),
+    );
+
+    const cards = (host: string) => [
+      ...container.querySelectorAll<HTMLElement>(
+        `[data-host="${host}"] .code-peek`,
+      ),
+    ];
+
+    const gapBetween = ([first, second]: HTMLElement[]) =>
+      second!.getBoundingClientRect().top -
+      first!.getBoundingClientRect().bottom;
+
+    expect(gapBetween(cards("tour"))).toBe(12);
+    expect(gapBetween(cards("peek"))).toBe(0);
+
+    const [single] = cards("tour-single");
+    expect(single!.getBoundingClientRect().bottom).toBe(
+      single!.parentElement!.getBoundingClientRect().bottom,
+    );
+  });
+
   it("disposes every prior editor while rapidly retargeting one inline surface", async () => {
     const container = document.createElement("div");
     document.body.append(container);

packages/review/app/src/CodePeek.tsx

@@ -10,7 +10,7 @@ import {
 } from "@review/lens-selection";
 import { type FileLineRange, codePeekSource } from "@review/source";
 import * as stylex from "@stylexjs/stylex";
-import { useEffect, useMemo, useRef } from "react";
+import { Fragment, useEffect, useMemo, useRef } from "react";
 
 import { DocumentCodeView } from "./DocumentCodeView";
 import { drawStyles } from "./draw-styles";
@@ -233,6 +233,42 @@ export function CodePeekCard({
   );
 }
 
+/** A peek's or tour stop's code: one ordinary card per range, in order. A
+ * step with several ranges, even in one file, gets a card for each. */
+export function SourceCodePeek({
+  sources,
+  active,
+  onNativeFocus,
+  lenses,
+  reportOutcome,
+}: {
+  sources: readonly DiffSelection[];
+  active?: boolean;
+  onNativeFocus?: () => void;
+  lenses?: ReviewLensView;
+  reportOutcome?: boolean;
+}) {
+  return sources.map((source, index) => (
+    <Fragment key={selectionKey(source)}>
+      {index > 0 && (
+        <div
+          {...withClass("code-peek-gap", styles.cardGap)}
+          aria-hidden="true"
+        />
+      )}
+      <CodePeekCard
+        source={source}
+        active={active}
+        heightMode="content"
+        edgeToEdge
+        onNativeFocus={onNativeFocus}
+        lenses={lenses}
+        reportOutcome={reportOutcome && index === 0}
+      />
+    </Fragment>
+  ));
+}
+
 function FileSnippetCard({
   source,
   active = false,
@@ -419,4 +455,19 @@ const styles = stylex.create({
   edgeToEdge: {
     marginBlock: 0,
   },
+  // Between a step's cards, sized by --review-code-card-gap (unset outside a
+  // tour step) with the tour rail running faintly through it.
+  cardGap: {
+    position: "relative",
+    height: "var(--review-code-card-gap, 0px)",
+    "::before": {
+      position: "absolute",
+      top: 0,
+      bottom: 0,
+      left: "var(--review-tour-rail-x)",
+      width: "2px",
+      backgroundColor: `color-mix(in srgb, ${tokens.ruleSoft} 70%, transparent)`,
+      content: "''",
+    },
+  },
 });

packages/review/app/src/diagrams.tsx

@@ -1,7 +1,11 @@
 import { flowLayer } from "@canvas/flow-layers.stylex";
 import { fontSize, fontWeight, radius } from "@canvas/scale.stylex";
 import { type DiffSelection, anchorSelection } from "@review/lens-selection";
-import type { SourcePins, Step } from "@review/review-api/document";
+import {
+  type SourcePins,
+  type Step,
+  stepSources,
+} from "@review/review-api/document";
 import * as stylex from "@stylexjs/stylex";
 import {
   BaseEdge,
@@ -97,7 +101,8 @@ export interface SequenceMessage {
   to: SequenceParticipant;
   label: string;
   style: Step["style"];
-  source?: DiffSelection;
+  /** Every range of the step, in order; the first is its call site. */
+  sources: DiffSelection[];
   code?: Step["code"];
   explanation?: string;
 }
@@ -125,11 +130,11 @@ export function sequenceView(block: SequenceDiagramProps): SequenceView {
       to: participant(step.to),
       label: step.label,
       style: step.style,
+      sources: stepSources(step).map((anchor) =>
+        anchorSelection(anchor, step.pins ?? block.pins),
+      ),
     };
 
-    if (step.source)
-      message.source = anchorSelection(step.source, step.pins ?? block.pins);
-
     if (step.code) message.code = step.code;
 
     if (step.explanation !== undefined) message.explanation = step.explanation;
@@ -153,7 +158,7 @@ function panelAnchor(message: SequenceMessage): PeekAnchor {
     title: message.label,
   };
 
-  if (message.source) anchor.peek = message.source;
+  if (message.sources[0]) anchor.peek = message.sources[0];
 
   return anchor;
 }
@@ -169,8 +174,14 @@ export function createSequenceTourEntry(sequence: SequenceView): GuidedTour {
       detail: `${message.from.label} -> ${message.to.label}`,
       content: message.code
         ? { kind: "inline-code" as const, ...message.code }
-        : message.source
-          ? { kind: "source" as const, source: message.source }
+        : message.sources[0]
+          ? {
+              kind: "source" as const,
+              source: message.sources[0],
+              ...(message.sources.length > 1 && {
+                more: message.sources.slice(1),
+              }),
+            }
           : { kind: "explanation" as const, text: message.explanation },
     })),
   };