diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewLens.test.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewLens.test.ts
index e4c17bdfd2..8f93046032 100644
--- a/apps/review-desktop/code-oss/src/vs/review/common/reviewLens.test.ts
+++ b/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]]);
+});
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 b9061a2765..38e7571dff 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
@@ -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;
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 add71e94e6..cb8c115cab 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
@@ -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) };
diff --git a/packages/review/app/src/CodePeek.browser.test.tsx b/packages/review/app/src/CodePeek.browser.test.tsx
index 676587220d..c7511c95d2 100644
--- a/packages/review/app/src/CodePeek.browser.test.tsx
+++ b/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(
+ ,
+ ),
+ );
+
+ 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(
+ <>
+
+
+
+
+
+
+
+
+
+ ,
+ ),
+ );
+
+ const cards = (host: string) => [
+ ...container.querySelectorAll(
+ `[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);
diff --git a/packages/review/app/src/CodePeek.tsx b/packages/review/app/src/CodePeek.tsx
index 8830730ba7..ebe405cf0a 100644
--- a/packages/review/app/src/CodePeek.tsx
+++ b/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) => (
+
+ {index > 0 && (
+
+ )}
+
+
+ ));
+}
+
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: "''",
+ },
+ },
});
diff --git a/packages/review/app/src/diagrams.tsx b/packages/review/app/src/diagrams.tsx
index 509f5c029c..30831d1761 100644
--- a/packages/review/app/src/diagrams.tsx
+++ b/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 },
})),
};
diff --git a/packages/review/app/src/review-components.tsx b/packages/review/app/src/review-components.tsx
index da23b002b6..1bda332a7a 100644
--- a/packages/review/app/src/review-components.tsx
+++ b/packages/review/app/src/review-components.tsx
@@ -34,7 +34,7 @@ import {
} from "./ask-thread-stream";
import { AskPills, AskSlot, AskWindow } from "./ask-window";
import { AuthoredCodeSurface } from "./authored-code-surface";
-import { CodePeekCard } from "./CodePeek";
+import { SourceCodePeek } from "./CodePeek";
import { controlStyles } from "./controls-styles";
import { documentStyles } from "./document-styles";
import { drawStyles } from "./draw-styles";
@@ -1325,11 +1325,9 @@ function ReviewPeekContentView({
if (content.kind === "source") {
return (
-
diff --git a/packages/review/app/src/review-panel-model.ts b/packages/review/app/src/review-panel-model.ts
index 2533cc44e0..0880159a69 100644
--- a/packages/review/app/src/review-panel-model.ts
+++ b/packages/review/app/src/review-panel-model.ts
@@ -3,7 +3,8 @@ import type { AskAgentId } from "@review/ask/thread-state";
import { type DiffSelection } from "@review/lens-selection";
export type ReviewPeekContent =
- | { kind: "source"; source: DiffSelection }
+ /** `more` follows `source` for a step with several ranges. */
+ | { kind: "source"; source: DiffSelection; more?: DiffSelection[] }
| { kind: "inline-code"; language?: string; text: string }
| { kind: "explanation"; text?: string }
| {
diff --git a/packages/review/app/src/sequence-tour.test.ts b/packages/review/app/src/sequence-tour.test.ts
index b2d807a9cc..00c2e05f9e 100644
--- a/packages/review/app/src/sequence-tour.test.ts
+++ b/packages/review/app/src/sequence-tour.test.ts
@@ -143,7 +143,9 @@ describe("sequence diagram guided tour", () => {
expect(
createSequenceTourEntry(sequence).stops.map((stop) => stop.anchor.id),
).toEqual(["request", "reply"]);
- expect(sequence.messages[0]?.source).toEqual(sequence.messages[1]?.source);
+ expect(sequence.messages[0]?.sources).toEqual(
+ sequence.messages[1]?.sources,
+ );
});
it("calculates scroll targets that reveal the active message participants", () => {
diff --git a/packages/review/app/src/side-panel-styles.ts b/packages/review/app/src/side-panel-styles.ts
index 330cba9fc6..8c80419b15 100644
--- a/packages/review/app/src/side-panel-styles.ts
+++ b/packages/review/app/src/side-panel-styles.ts
@@ -284,6 +284,9 @@ export const tourStyles = stylex.create({
[narrow]: "38px minmax(0, 1fr)",
},
paddingRight: { default: "16px", [narrow]: "8px" },
+ // The rail's x in the stop, for the rail and its faint runs between a
+ // step's code cards.
+ "--review-tour-rail-x": { default: "29px", [narrow]: "19px" },
},
rail: {
position: "relative",
@@ -293,7 +296,7 @@ export const tourStyles = stylex.create({
position: "absolute",
top: 0,
bottom: 0,
- left: { default: "29px", [narrow]: "19px" },
+ left: "var(--review-tour-rail-x)",
width: "2px",
backgroundColor: tokens.ruleSoft,
content: "''",
@@ -339,6 +342,8 @@ export const tourStyles = stylex.create({
width: { default: "calc(100% + 76px)", [narrow]: "calc(100% + 46px)" },
marginLeft: { default: "-60px", [narrow]: "-38px" },
backgroundColor: tokens.bg,
+ // A step's code cards stand apart; a lone peek's stay flush.
+ "--review-code-card-gap": "12px",
},
header: {
display: "flex",
diff --git a/packages/review/src/review-api/authoring-pitfalls.test.ts b/packages/review/src/review-api/authoring-pitfalls.test.ts
index 4888e80861..b7143f4c5d 100644
--- a/packages/review/src/review-api/authoring-pitfalls.test.ts
+++ b/packages/review/src/review-api/authoring-pitfalls.test.ts
@@ -104,6 +104,12 @@ beforeEach(async () => {
path.join(repository, "src/blank.ts"),
"export const a = 1;\n\n\n// end\n",
);
+ writeFileSync(
+ path.join(repository, "src/long.ts"),
+ Array.from({ length: 20 }, (_, i) => `export const n${i} = ${i};\n`).join(
+ "",
+ ),
+ );
writeFileSync(
path.join(repository, "assets/logo.png"),
Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]),
@@ -278,6 +284,64 @@ describe("diagram rules", () => {
);
});
+ const call = rangeAnchor(head("src/store.ts", 2));
+ const callee = rangeAnchor(head("order.ts", 1));
+
+ const step = (source: JsonValue) => ({
+ from: "app",
+ to: "db",
+ label: "Write",
+ source,
+ });
+
+ it("stores a step's several ranges in order and a single one as before", async () => {
+ const source = [call, rangeAnchor(head("src/blank.ts", 1))];
+
+ expect((await insert(sequence([step(source), step(call)]))).status).toBe(
+ 200,
+ );
+ expect(
+ elements(local.store.read(reviewId).document).filter(
+ (element) => element.type === "step",
+ ),
+ ).toMatchObject([{ source }, { source: call }]);
+ });
+
+ it("rejects more than four ranges and a range listed twice", async () => {
+ const lines = [1, 2, 3, 4].map((line) =>
+ rangeAnchor(head("src/long.ts", line)),
+ );
+
+ await expectRejected(
+ () => insert(sequence([step([...lines, call])])),
+ "expected array to have <=4 items",
+ );
+ await expectRejected(
+ () => insert(sequence([step([call, call])])),
+ `Range ${call} repeats another range of its step.`,
+ );
+ });
+
+ it("rejects a range that is another step's code", () =>
+ // The callee is the next step, not more of this one.
+ expectRejected(
+ () => insert(sequence([step([call, callee]), step(callee)])),
+ `Range ${callee} is step 2's code; link the step instead.`,
+ ));
+
+ it("warns once about each range longer than a call", async () => {
+ const long = rangeAnchor(head("src/long.ts", 1, 20));
+
+ const warnings = async (content: JsonValue) =>
+ ((await insert(content)).body as { warnings?: string[] }).warnings ?? [];
+
+ expect(await warnings(sequence([step([call, long])]))).toEqual([
+ `Step 1 of "Save" pins 20 lines at ${long}. A step's source is its call site; pin the call, not the function body.`,
+ ]);
+ // The step already written is not warned about again.
+ expect(await warnings({ type: "markdown", markdown: "Next" })).toEqual([]);
+ });
+
it("rejects duplicate frame keys within a column", async () => {
const baseSource = { ...head("src/store.ts", 1), side: "base" as const };
diff --git a/packages/review/src/review-api/blocks/definition.ts b/packages/review/src/review-api/blocks/definition.ts
index f5bda9311c..f1ded004de 100644
--- a/packages/review/src/review-api/blocks/definition.ts
+++ b/packages/review/src/review-api/blocks/definition.ts
@@ -17,12 +17,14 @@ export const elementPins = sourcePinsSchema
/**
* One block kind: the strict schema the store parses with, and the rules a
- * field schema cannot express. `check` throws ReviewInputError.
+ * field schema cannot express. `check` throws ReviewInputError; `advise`
+ * returns warnings for content that is allowed but usually a mistake.
*/
export interface BlockDefinition {
type: Content["type"];
schema: z.ZodType;
check?(block: Content): void;
+ advise?(block: Content): string[];
}
/** The strict schema for a block's own fields; id and type are added here. */
diff --git a/packages/review/src/review-api/blocks/index.ts b/packages/review/src/review-api/blocks/index.ts
index 2367db197d..1d24dabda3 100644
--- a/packages/review/src/review-api/blocks/index.ts
+++ b/packages/review/src/review-api/blocks/index.ts
@@ -86,3 +86,22 @@ export function checkReferences(document: Block[]): void {
document.forEach(visit);
}
+
+function adviseBlock(
+ type: K,
+ block: Extract,
+): string[] {
+ const definition: Definitions[K] = blocks[type];
+
+ return definition.advise?.(block) ?? [];
+}
+
+/** Every block's warnings about allowed content that is usually a mistake. */
+export function documentAdvice(document: Block[]): string[] {
+ const visit = (block: Block): string[] => [
+ ...adviseBlock(block.type, block),
+ ...("children" in block ? block.children.flatMap(visit) : []),
+ ];
+
+ return document.flatMap(visit);
+}
diff --git a/packages/review/src/review-api/blocks/sequence.ts b/packages/review/src/review-api/blocks/sequence.ts
index 6a2a2aa8c1..8f322d26bb 100644
--- a/packages/review/src/review-api/blocks/sequence.ts
+++ b/packages/review/src/review-api/blocks/sequence.ts
@@ -1,4 +1,10 @@
-import { anchorSchema } from "@review/lens-selection.js";
+import {
+ type Anchor,
+ type DiffSelection,
+ anchorSchema,
+ parseAnchor,
+} from "@review/lens-selection.js";
+import { ReviewInputError } from "@review/review-api/input-error.js";
import { z } from "zod";
import { codeFields } from "./code.js";
@@ -12,6 +18,9 @@ import {
text,
} from "./definition.js";
+/** A call that needs more lines than this is rare; a function body is not. */
+const callSiteLines = 12;
+
export const stepSchema = z
.strictObject({
...identity,
@@ -20,7 +29,11 @@ export const stepSchema = z
to: label,
label,
style: z.enum(["call", "return", "async"]).default("call"),
- source: anchorSchema.optional(),
+ /** The call site this arrow stands for, then any ranges directly about
+ * that call, each shown as its own card. */
+ source: z
+ .union([anchorSchema, z.array(anchorSchema).min(1).max(4)])
+ .optional(),
pins: elementPins,
explanation: label.optional(),
code: z.strictObject(codeFields).optional(),
@@ -34,6 +47,11 @@ export const stepSchema = z
export type Step = z.infer;
+/** A step's ranges in order; a single anchor is a list of one. */
+export function stepSources(step: Pick): Anchor[] {
+ return step.source === undefined ? [] : [step.source].flat();
+}
+
/** Kept for callers that parsed sequences directly. */
export const sequenceSchema = defineBlock("sequence", {
title: label,
@@ -48,9 +66,69 @@ export const sequence = {
type: "sequence",
schema: sequenceSchema,
check(block: SequenceBlock) {
+ const pins = (step: Step) => JSON.stringify(step.pins ?? block.pins);
+
for (const step of block.steps) {
requireKey(block.actors, step.from);
requireKey(block.actors, step.to);
+
+ const own = stepSources(step);
+
+ for (const [at, range] of own.entries()) {
+ if (own.some((other, index) => index !== at && covers(range, other)))
+ throw new ReviewInputError(
+ `Range ${range} repeats another range of its step.`,
+ );
+
+ // A range after the call site that holds another step's range is
+ // that step's code: the callee is the next step, not more of this
+ // one. The call site itself may repeat, as a call and its return do.
+ for (const [index, other] of block.steps.entries())
+ if (
+ at > 0 &&
+ other !== step &&
+ pins(other) === pins(step) &&
+ stepSources(other).some((theirs) => covers(range, theirs))
+ )
+ throw new ReviewInputError(
+ `Range ${range} is step ${index + 1}'s code; link the step instead.`,
+ );
+ }
}
},
+ advise(block: SequenceBlock) {
+ return block.steps.flatMap((step, index) =>
+ stepSources(step).flatMap((range) => {
+ const lines = oneSideLines(parseAnchor(range)!);
+
+ return lines && lines > callSiteLines
+ ? [
+ `Step ${index + 1} of "${block.title}" pins ${lines} lines at ${range}. A step's source is its call site; pin the call, not the function body.`,
+ ]
+ : [];
+ }),
+ );
+ },
} satisfies BlockDefinition;
+
+/** Lines in a one-sided selection; a selection across sides has no count. */
+function oneSideLines({ start, end }: DiffSelection): number | undefined {
+ return start.side === end.side ? end.line - start.line + 1 : undefined;
+}
+
+/** Whether `outer` equals or contains `inner`, in the same file. Across
+ * sides, only the same anchor counts. */
+function covers(outer: Anchor, inner: Anchor): boolean {
+ if (outer === inner) return true;
+ const a = parseAnchor(outer)!;
+ const b = parseAnchor(inner)!;
+
+ return (
+ a.file === b.file &&
+ oneSideLines(a) !== undefined &&
+ oneSideLines(b) !== undefined &&
+ a.start.side === b.start.side &&
+ a.start.line <= b.start.line &&
+ a.end.line >= b.end.line
+ );
+}
diff --git a/packages/review/src/review-api/component-reference.ts b/packages/review/src/review-api/component-reference.ts
index 3f2d65971f..1022dae238 100644
--- a/packages/review/src/review-api/component-reference.ts
+++ b/packages/review/src/review-api/component-reference.ts
@@ -15,7 +15,8 @@ export const COMPONENT_REFERENCE = [
"- trace_quote {traceId, eventId, text}",
"- software_map {mapVersionId, focusElementId?}",
"- sequence {title, actors: {key: label}, steps: step[], pins?}",
- "- step {from, to (actor keys), label, style?: call|return|async, and exactly one of source: anchor, explanation, or code: {text, language?}; pins?}",
+ "- step {from, to (actor keys), label, style?: call|return|async, and exactly one of source: anchor or anchor[] (at most 4), explanation, or code: {text, language?}; pins?}",
+ "A step's source is the line where its arrow leaves `from` (the call, return or emit). Add an additional range only when that line can't be read without it: the type or schema of what it passes, or the code that builds its argument. Never the callee's body; that is the next step.",
"- flow_diagram {title, nodes: flow_node[] (1 to 100), edges: flow_edge[], description?, direction?: right|down, pins?}",
"- flow_node {key, label, description?, kind?: process|decision|terminal, attachments?: {label, sources: anchor[], pins?}[], link?: {from or to, label?, style?}}",
"- flow_edge {from, to (node keys), label?, style?: solid|dashed}",
diff --git a/packages/review/src/review-api/document-text.ts b/packages/review/src/review-api/document-text.ts
index 38b0b88266..cbacdbd250 100644
--- a/packages/review/src/review-api/document-text.ts
+++ b/packages/review/src/review-api/document-text.ts
@@ -1,4 +1,9 @@
-import { type Element, ReviewInputError, elements } from "./document.js";
+import {
+ type Element,
+ ReviewInputError,
+ elements,
+ stepSources,
+} from "./document.js";
import type { Snapshot } from "./store.js";
/** A reading view of saved content, not another document format to maintain. */
@@ -79,7 +84,7 @@ export function documentText(
`${element.from} → ${element.to}: ${element.label} (${element.style})`,
);
- if (element.source) detail(element.source);
+ for (const source of stepSources(element)) detail(source);
if (detailed && element.explanation) detail(element.explanation);
diff --git a/packages/review/src/review-api/document.ts b/packages/review/src/review-api/document.ts
index c732c6528c..c2ad8c76bd 100644
--- a/packages/review/src/review-api/document.ts
+++ b/packages/review/src/review-api/document.ts
@@ -31,7 +31,7 @@ import {
blockSchema,
blocks,
} from "./blocks/index.js";
-import { type Step, stepSchema } from "./blocks/sequence.js";
+import { type Step, stepSchema, stepSources } from "./blocks/sequence.js";
import type { Lens } from "./diff-lenses.js";
import { ReviewInputError } from "./input-error.js";
@@ -45,6 +45,7 @@ export {
blockSchema,
blocks,
checkReferences,
+ documentAdvice,
} from "./blocks/index.js";
export { type Frame, frameSchema } from "./blocks/call_stack_diff.js";
@@ -67,6 +68,7 @@ export {
type Step,
sequenceSchema,
stepSchema,
+ stepSources,
} from "./blocks/sequence.js";
const text = z.string();
@@ -321,16 +323,18 @@ function documentReferences(
// A step, frame and operation render the range as a peek, so a
// whitespace-only range is an authoring mistake for each of them; prose
// links and context sources only need the range to exist.
+ // Each of a step's ranges is its own reference, so a stale one marks only
+ // itself. The first keeps the step's id, as a single source always had.
if (element.type === "sequence")
return element.steps.flatMap((step) =>
- step.source
- ? select(step.source, step.pins ?? element.pins).map((source) => ({
- id: step.id!,
- source,
- label: step.label,
- peek: true,
- }))
- : [],
+ stepSources(step).flatMap((anchor, index) =>
+ select(anchor, step.pins ?? element.pins).map((source) => ({
+ id: index ? `${step.id}:source:${index}` : step.id!,
+ source,
+ label: step.label,
+ peek: true,
+ })),
+ ),
);
if (element.type === "flow_diagram")
diff --git a/packages/review/src/review-api/local-data.test.ts b/packages/review/src/review-api/local-data.test.ts
index 358fe9f533..35c5bb099b 100644
--- a/packages/review/src/review-api/local-data.test.ts
+++ b/packages/review/src/review-api/local-data.test.ts
@@ -37,7 +37,7 @@ import {
callAuthoringTool,
} from "./agent-client.js";
import { ReviewApiClient } from "./client.js";
-import type { Pins } from "./document.js";
+import { type Pins, elements } from "./document.js";
import { createReviewApi } from "./http.js";
import { openLocalReviewStore } from "./local-data.js";
@@ -3082,6 +3082,58 @@ it("keeps authored coordinates fixed as live source changes and warns only on un
});
});
+it("marks only the range of a step that no longer exists", async () => {
+ writeFileSync(
+ path.join(repository, "range.ts"),
+ "const first = 1;\nconst second = 2;\nconst third = 3;\n",
+ );
+ git("add", "-N", "range.ts");
+
+ const result = await local.store.execute(
+ command({
+ type: "create",
+ title: "Ranges",
+ target: {
+ kind: "worktree",
+ repositoryId: pins.repositoryId,
+ base: pins.base,
+ },
+ }),
+ );
+
+ const line = (fromLine: number) =>
+ rangeAnchor({ side: "head", file: "range.ts", fromLine, toLine: fromLine });
+
+ await insert(result.reviewId, {
+ type: "sequence",
+ title: "Read",
+ actors: { app: "App" },
+ steps: [
+ {
+ from: "app",
+ to: "app",
+ label: "Read",
+ source: [line(1), line(2), line(3)],
+ },
+ ],
+ });
+
+ const step = elements(local.store.read(result.reviewId).document).find(
+ (element) => element.type === "step",
+ )!;
+
+ writeFileSync(
+ path.join(repository, "range.ts"),
+ "const first = 99;\nconst second = 2;\n",
+ );
+ await vi.waitFor(async () => {
+ await local.store.refreshWorktrees();
+ expect(local.store.read(result.reviewId).staleSources).toEqual([
+ `${step.id}:source:2`,
+ ]);
+ });
+});
+
it("reads current checkout even with an older authored version", async () => {
const api = createReviewApi(local.store, local.data);
writeFileSync(path.join(repository, "example.ts"), "const generation = 1;\n");
diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts
index d623c2f150..0797ab1fd7 100644
--- a/packages/review/src/review-api/store.ts
+++ b/packages/review/src/review-api/store.ts
@@ -44,6 +44,7 @@ import {
applyEdit,
assignFreshIds,
checkReferences,
+ documentAdvice,
documentSchema,
editSchema,
elements,
@@ -1452,11 +1453,19 @@ export class ReviewStore {
)
await this.providers.validatePins(snapshot.pins);
- const warnings = await this.validateExternal(
- snapshot,
- previous,
- op.type === "set_target",
- );
+ // Advice is about what this command wrote; earlier advice stays said.
+ const advised = new Set(documentAdvice(previous?.document ?? []));
+
+ const warnings = [
+ ...documentAdvice(snapshot.document).filter(
+ (advice) => !advised.has(advice),
+ ),
+ ...(await this.validateExternal(
+ snapshot,
+ previous,
+ op.type === "set_target",
+ )),
+ ];
snapshot.version = previous ? previous.version + 1 : 0;
snapshot.createdAt = new Date().toISOString();