From 2ffab3e77d607cf0701bd1758c2a4004d48751ba Mon Sep 17 00:00:00 2001 From: Jaeyoon Kim Date: Sat, 26 Sep 2026 19:38:39 -0400 Subject: [PATCH 1/4] Keep tour steps and code peeks clipped to their pinned lines Tour steps and code peeks build a "document:" lens that goes through lensContextGaps(). Since #502, when diffr supplies contextScopes, any unfolded run of lines that touches the lens range is shown whole. An added file is one unfolded run, so the card shows the entire file instead of the pinned lines. Add ReviewDiffLens.exact, set it on the document lens, and have withLens() drop contextScopes before computing gaps for an exact lens. Diff tab lenses never set exact, so #502's behavior there is unchanged. Co-Authored-By: Claude Opus 5.5 --- .../src/vs/review/common/reviewLens.test.ts | 14 ++++++++++++++ .../vs/review/services/reviewApiSourceService.ts | 2 +- .../code-oss/src/vs/review/services/reviewLens.ts | 5 ++++- packages/review-protocol/src/contracts.ts | 2 ++ 4 files changed, 21 insertions(+), 2 deletions(-) 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 4470324ed..2c6efa5c4 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 @@ -24,6 +24,20 @@ test('structural folds within the lens survive, overlapping folds do not', () => assert.ok(!gaps.includes(outside)); }); +test('an exact document lens (tour step, code peek) clips an added file to its +/-3 window; without it, #502 fills the whole open run', () => { + const diff = { + ...plain, + sourceLineAlignment: Array.from({ length: 300 }, (_, i) => [null, i] as const), + contextScopes: { original: [], modified: [[0, 300] as const] }, + }; + const ranges = [{ side: 'head' as const, file: 'new.ts', fromLine: 80, toLine: 87 }]; + // #502 behavior (Diff tab): the added file is one open run, so contextScopes fills it whole. + assert.deepEqual(lensContextGaps(diff, 0, 300, ranges), []); + // exact document lens: withLens strips contextScopes before calling lensContextGaps. + const clipped = lensContextGaps({ ...diff, contextScopes: undefined }, 0, 300, ranges); + assert.deepEqual(clipped.map(gap => [gap.modifiedStart, gap.modifiedCount]), [[1, 76], [91, 210]]); +}); + test('viewed folds never hide an unread counterpart, and do not overlap structural folds', async () => { const { viewedContextGaps } = await import('./reviewLens.js'); const range = (side: 'base' | 'head', fromLine: number, toLine: number) => ({ side, file: 'a.ts', fromLine, toLine }); 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 c71b01890..963eb375d 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 @@ -281,7 +281,7 @@ export class ReviewApiSourceService extends Disposable implements IReviewApiSour const current = spec.pins ? reviewSourceAnchor(view(), spec.pins) : reviewSourceComparison(view()); const lens: ReviewDiffLens = { id: "document:" + JSON.stringify([spec.path, spec.ranges, spec.pins]), title: spec.path, - reviewId: current.reviewId, version: current.version, + reviewId: current.reviewId, version: current.version, exact: true, ranges: spec.ranges.map(range => ({ file: spec.path, side: range.side ?? spec.side, fromLine: range.startLine, toLine: range.endLine })), }; return { lens, source: makeSource(() => current) }; 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 8cc8a33d2..1b73282bc 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 @@ -59,7 +59,10 @@ export function withLens(instantiation: IInstantiationService, entries: readonly }) ? { ...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; + // An exact lens (a tour step, a code peek) is authored to show one chunk; + // diffr's structural scope expansion is right for the Diff tab, not here. + const scopedDiff = lens?.exact ? { ...diff, contextScopes: undefined } : diff; + return lens && !lens.wholeFiles ? { ...diff, contextGaps: lensContextGaps(scopedDiff, original.getLineCount(), modified.getLineCount(), lensRanges(section ? { ...lens, ranges: section.sources } : lens, entry)) } : diff; }, }; }, diff --git a/packages/review-protocol/src/contracts.ts b/packages/review-protocol/src/contracts.ts index b9335aa6d..91021b612 100644 --- a/packages/review-protocol/src/contracts.ts +++ b/packages/review-protocol/src/contracts.ts @@ -192,6 +192,8 @@ export interface ReviewInlineEditorFactory { export interface ReviewDiffLens { /** Filter files while retaining ordinary diff context/folding within them. */ wholeFiles?: boolean; + /** Show only the pinned range, ignoring diffr's structural scope expansion. */ + exact?: boolean; id: string; title: string; reviewId: string; From 4a84ea15ffcbd6813370f0e91b6b6d6d6a7317e8 Mon Sep 17 00:00:00 2001 From: Jaeyoon Kim Date: Sat, 26 Sep 2026 20:30:10 -0400 Subject: [PATCH 2/4] Tell authors to pin tight ranges for tour steps and peeks Co-Authored-By: Claude Opus 5.5 --- packages/review/instructions/authoring.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/review/instructions/authoring.md b/packages/review/instructions/authoring.md index 78e7ded6f..b1bee202f 100644 --- a/packages/review/instructions/authoring.md +++ b/packages/review/instructions/authoring.md @@ -35,3 +35,4 @@ follow these first six steps exactly, without any extraneous tool calls. - keep whiteboards short and sweet when possible (esp. for small changes.) feel free to omit sections. - when something (a phrase in the prose, diagram node, etc.) describes actual code in the codebase, always default to attaching/hyperlink code. - Link repository code as `[label](review-source:head/src/file.ts#L10-L24)`; use `base` for old code. Use repository-relative paths and verified line numbers. +- point each code reference (diagram step, call-stack frame, `code_peek`) at the smallest range that shows the claim, usually 3-15 lines: the call, the branch, the assignment. not the whole function. tour steps and peeks show only that range. From 7e4467f24dc3ff3e05eadf63e6095e450b40029e Mon Sep 17 00:00:00 2001 From: Jaeyoon Kim Date: Sat, 26 Sep 2026 20:14:35 -0400 Subject: [PATCH 3/4] Let a sequence step show several code chunks A sequence step could carry only one source range, so a step that touches two places in the code had to pick one. Add an optional `sources` array (1 to 10 ranges) as a fourth alternative to `source`, `explanation` and `code`. A step still needs exactly one of them, and stored steps that use `source` read and render as before. The guided tour shows every chunk when it lands on the step. Chunks that share a file, side and pins go into one code card with several ranges, so the exact document lens folds the code between them. Chunks in other files get their own stacked cards. The first chunk anchors the step for jump-to-source. Each chunk is its own document reference (`:`), so it gets the same whitespace, pin and staleness checks as a single source. The MCP edit guidance and scratchpad instructions mention `sources`. Co-Authored-By: Claude Opus 5.5 --- .../review/app/src/CodePeek.browser.test.tsx | 53 ++++++++- packages/review/app/src/CodePeek.tsx | 108 ++++++++++++++---- .../review/app/src/code-peek-groups.test.ts | 48 ++++++++ packages/review/app/src/diagrams.tsx | 33 ++++-- packages/review/app/src/review-components.tsx | 16 ++- packages/review/app/src/review-panel-model.ts | 1 + packages/review/app/src/sequence-tour.test.ts | 47 ++++++++ packages/review/app/src/styles.css | 7 ++ packages/review/instructions/scratchpad.md | 2 +- .../src/review-api/authoring-pitfalls.test.ts | 58 +++++++++- .../review/src/review-api/authoring-tools.ts | 2 +- .../review/src/review-api/blocks/sequence.ts | 17 ++- .../review/src/review-api/document-text.ts | 2 + packages/review/src/review-api/document.ts | 10 ++ 14 files changed, 364 insertions(+), 40 deletions(-) create mode 100644 packages/review/app/src/code-peek-groups.test.ts diff --git a/packages/review/app/src/CodePeek.browser.test.tsx b/packages/review/app/src/CodePeek.browser.test.tsx index 2000a3fd6..2cd0ee3f0 100644 --- a/packages/review/app/src/CodePeek.browser.test.tsx +++ b/packages/review/app/src/CodePeek.browser.test.tsx @@ -7,7 +7,12 @@ import { createRoot } from "react-dom/client"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { selectSource, sourceAnchors } from "../../src/lens-selection"; -import { CodePeek, CodePeekCard, CodePeekGroup } from "./CodePeek"; +import { + CodePeek, + CodePeekCard, + CodePeekGroup, + CodePeekStack, +} from "./CodePeek"; import { type ReviewSession, ReviewSessionProvider, @@ -216,6 +221,52 @@ describe("CodePeek native editor", () => { }); }); + it("shows same-file chunks in one editor and other files in their own", async () => { + const chunk = (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/a.ts", + title: "src/a.ts:1-3, 20-22", + ranges: [ + { startLine: 1, endLine: 3 }, + { startLine: 20, endLine: 22 }, + ], + }, + { path: "src/b.ts", title: "src/b.ts:5" }, + ]); + }); + + await act(async () => { + created[0]?.onDidOpen?.(); + }); + + expect(posted.at(-1)).toMatchObject({ + name: "reveal", + args: { path: "src/a.ts", startLine: 1, endLine: 3 }, + }); + }); + it("gives range side peeks a source title and content height policy", async () => { const input = { side: "head", diff --git a/packages/review/app/src/CodePeek.tsx b/packages/review/app/src/CodePeek.tsx index 193df3e3f..9569cabb8 100644 --- a/packages/review/app/src/CodePeek.tsx +++ b/packages/review/app/src/CodePeek.tsx @@ -9,6 +9,7 @@ import { type DiffSelection, selectionKey, sourceAnchor, + sourcePinsKey, } from "../../src/lens-selection"; import type { ReviewComponentProps } from "../../src/review-document-data"; import { type FileLineRange, codePeekSource } from "../../src/source"; @@ -116,28 +117,82 @@ export function ReviewCodePeek({ anchor }: ReviewComponentProps<"CodePeek">) { return ; } +interface CodePeekCardOptions { + active?: boolean; + heightMode?: ReviewInlineEditorHeightMode; + onNativeFocus?: () => void; + lenses?: ReviewLensView; + /** Report resolution telemetry. Only the panel the user opened sets this, + * so an authored document with many inline peeks sends one event, not N. */ + reportOutcome?: boolean; +} + /** A document peek is an interval of the same alignment used by diff lenses. */ export function CodePeekCard({ source, + ...options +}: CodePeekCardOptions & { source: DiffSelection }) { + const sources = useMemo(() => [source], [source]); + + return ; +} + +/** Several chunks, one card per file: chunks in the same file share a card so + * its lens folds the code between them. */ +export function CodePeekStack({ + sources, + ...options +}: CodePeekCardOptions & { sources: readonly DiffSelection[] }) { + const groups = useMemo(() => codePeekFileGroups(sources), [sources]); + + return ( +
+ {groups.map((group) => ( + + ))} +
+ ); +} + +/** Chunks group by file, diff side and pins, in the order each group first + * appears. */ +export function codePeekFileGroups( + sources: readonly DiffSelection[], +): DiffSelection[][] { + const groups = new Map(); + + for (const source of sources) { + const key = JSON.stringify([ + source.file, + sourceAnchor(source).side, + source.pins ? sourcePinsKey(source.pins) : null, + ]); + + groups.set(key, [...(groups.get(key) ?? []), source]); + } + + return [...groups.values()]; +} + +/** Every source names the same file, side and pins; the first one anchors + * the card's jump-to-source. */ +function CodePeekFileCard({ + sources, active = false, heightMode = "capped", onNativeFocus, lenses: lensesOverride, reportOutcome = false, -}: { - source: DiffSelection; - active?: boolean; - heightMode?: ReviewInlineEditorHeightMode; - onNativeFocus?: () => void; - lenses?: ReviewLensView; - /** Report resolution telemetry. Only the panel the user opened sets this, - * so an authored document with many inline peeks sends one event, not N. */ - reportOutcome?: boolean; -}) { +}: CodePeekCardOptions & { sources: readonly DiffSelection[] }) { const session = useReviewSession(); const contextLenses = useReviewLenses(); const lenses = lensesOverride ?? contextLenses; - const resolved = lenses?.resolve([source]) ?? []; + const resolved = lenses?.resolve(sources) ?? []; + const source = sources[0]!; const anchor = sourceAnchor(source); const ranges = resolved.map((range) => ({ @@ -146,12 +201,14 @@ export function CodePeekCard({ endLine: range.toLine, })); - const key = selectionKey(source); + const key = sources.map(selectionKey).join("\n"); const outcome = peekResolutionOutcome({ resolvedCount: ranges.length, complete: Boolean(lenses?.progress?.complete), - unavailable: Boolean(lenses?.progress?.unavailableSelections?.[key]), + unavailable: sources.some( + (item) => lenses?.progress?.unavailableSelections?.[selectionKey(item)], + ), error: Boolean(lenses?.error), }); @@ -181,15 +238,7 @@ export function CodePeekCard({
source.start.side !== source.end.side)) + return file; + + const lines = sources.map(({ start, end }) => + start.line === end.line ? `${start.line}` : `${start.line}-${end.line}`, + ); + + return `${file}:${lines.join(", ")}`; +} + function FileSnippetCard({ source, active = false, diff --git a/packages/review/app/src/code-peek-groups.test.ts b/packages/review/app/src/code-peek-groups.test.ts new file mode 100644 index 000000000..d2030650d --- /dev/null +++ b/packages/review/app/src/code-peek-groups.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; + +import type { DiffSelection } from "../../src/lens-selection"; +import { codePeekFileGroups } from "./CodePeek"; + +const chunk = ( + file: string, + line: number, + extra: Partial = {}, +): DiffSelection => ({ + file, + start: { side: "head", line }, + end: { side: "head", line }, + ...extra, +}); + +describe("code peek file groups", () => { + it("puts chunks from one file in one card and other files in their own", () => { + const first = chunk("src/a.ts", 10); + const other = chunk("src/b.ts", 3); + const second = chunk("src/a.ts", 40); + + expect(codePeekFileGroups([first, other, second])).toEqual([ + [first, second], + [other], + ]); + }); + + it("keeps chunks apart when their side or pins differ", () => { + const head = chunk("src/a.ts", 10); + + const base = chunk("src/a.ts", 10, { + start: { side: "base", line: 10 }, + end: { side: "base", line: 10 }, + pins: { repositoryId: "repo", base: "b1", head: "h1" }, + }); + + const pinned = chunk("src/a.ts", 20, { + pins: { repositoryId: "repo", head: "h2" }, + }); + + expect(codePeekFileGroups([head, base, pinned])).toEqual([ + [head], + [base], + [pinned], + ]); + }); +}); diff --git a/packages/review/app/src/diagrams.tsx b/packages/review/app/src/diagrams.tsx index 308bdd1d4..12db820fb 100644 --- a/packages/review/app/src/diagrams.tsx +++ b/packages/review/app/src/diagrams.tsx @@ -30,7 +30,11 @@ import { DiagramTourOverlay, useDiagramTourShell } from "./diagram-tour"; import { useMotionPhase } from "./draw-queue-provider"; import { useReviewSession } from "./host/review-session"; import { useReviewPanel } from "./review-panel"; -import type { GuidedTour, PeekAnchor } from "./review-panel-model"; +import type { + GuidedTour, + PeekAnchor, + ReviewPeekContent, +} from "./review-panel-model"; import { useTourPersist, useTourRestore } from "./review-view-state"; import { captureUiEvent } from "./ui-telemetry"; @@ -90,7 +94,10 @@ export interface SequenceMessage { to: SequenceParticipant; label: string; style: Step["style"]; + /** The first chunk, where a single anchor is needed. */ source?: Step["source"]; + /** Every chunk the step shows, in authored order. */ + sources?: NonNullable[]; code?: Step["code"]; explanation?: string; } @@ -120,7 +127,12 @@ export function sequenceView(block: SequenceDiagramProps): SequenceView { style: step.style, }; - if (step.source) message.source = step.source; + const sources = step.sources ?? (step.source ? [step.source] : []); + + if (sources.length) { + message.source = sources[0]; + message.sources = sources; + } if (step.code) message.code = step.code; @@ -159,15 +171,22 @@ export function createSequenceTourEntry(sequence: SequenceView): GuidedTour { anchor: panelAnchor(message), label: message.label, 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 } - : { kind: "explanation" as const, text: message.explanation }, + content: sequenceStopContent(message), })), }; } +function sequenceStopContent(message: SequenceMessage): ReviewPeekContent { + if (message.code) return { kind: "inline-code", ...message.code }; + + if (message.sources && message.sources.length > 1) + return { kind: "sources", sources: message.sources }; + + if (message.source) return { kind: "source", source: message.source }; + + return { kind: "explanation", text: message.explanation }; +} + function participantsForMessages( messages: SequenceMessage[], ): SequenceParticipant[] { diff --git a/packages/review/app/src/review-components.tsx b/packages/review/app/src/review-components.tsx index 2c092e66c..e981e4591 100644 --- a/packages/review/app/src/review-components.tsx +++ b/packages/review/app/src/review-components.tsx @@ -9,7 +9,7 @@ import { useEffect, useEffectEvent, useMemo, useRef, useState } from "react"; import type { ReviewComponentProps } from "../../src/review-document-data"; import { AuthoredCodeSurface } from "./authored-code-surface"; -import { CodePeekCard } from "./CodePeek"; +import { CodePeekCard, CodePeekStack } from "./CodePeek"; import { findWhitespaceNormalizedSpan } from "./highlighted-text"; import { useOptionalReviewSession, @@ -613,7 +613,7 @@ function CodeReviewPeekPanel({ anchor: PeekAnchor; content: Extract< ReviewPeekContent, - { kind: "source" | "inline-code" | "explanation" } + { kind: "source" | "sources" | "inline-code" | "explanation" } >; onClose: () => void; }) { @@ -1038,6 +1038,18 @@ function ReviewPeekContentView({ ); } + if (content.kind === "sources") { + return ( + + ); + } + if (content.kind === "inline-code") { return ( { ).toEqual(["request", "request--sequence-use-2"]); }); + it("shows every chunk of a multi-chunk step in one tour stop", () => { + const chunk = (file: string, line: number) => ({ + file, + start: { side: "head" as const, line }, + end: { side: "head" as const, line: line + 2 }, + }); + + const chunks = [chunk("src/a.ts", 10), chunk("src/a.ts", 40)]; + + const sequence = sequenceView({ + id: "diagram-1", + title: "Save", + actors: { app: "App", db: "Database" }, + steps: [ + { + id: "step-1", + type: "step", + from: "app", + to: "db", + label: "write", + style: "call", + sources: chunks, + }, + { + id: "step-2", + type: "step", + from: "db", + to: "app", + label: "ack", + style: "return", + sources: [chunk("src/b.ts", 5)], + }, + ], + }); + + const [write, ack] = createSequenceTourEntry(sequence).stops; + + expect(write).toMatchObject({ + anchor: { id: "step-1", peek: chunks[0] }, + content: { kind: "sources", sources: chunks }, + }); + expect(ack).toMatchObject({ + anchor: { id: "step-2", peek: chunk("src/b.ts", 5) }, + content: { kind: "source", source: chunk("src/b.ts", 5) }, + }); + }); + it("calculates scroll targets that reveal the active message participants", () => { const actors = defineActors({ reviewer: { label: "Reviewer" }, diff --git a/packages/review/app/src/styles.css b/packages/review/app/src/styles.css index 4539c2fb0..eb869e077 100644 --- a/packages/review/app/src/styles.css +++ b/packages/review/app/src/styles.css @@ -3299,6 +3299,13 @@ button { min-width: 0; } +.code-peek-stack { + display: flex; + flex-direction: column; + gap: 14px; + min-width: 0; +} + .code-peek { min-width: 0; max-width: 100%; diff --git a/packages/review/instructions/scratchpad.md b/packages/review/instructions/scratchpad.md index ffcdf62f6..4ff415d51 100644 --- a/packages/review/instructions/scratchpad.md +++ b/packages/review/instructions/scratchpad.md @@ -14,7 +14,7 @@ Answer in chat when one sentence or one code line does it. Do not use the scratc ## How 1. `session_capabilities({})`. Draw only when `desktopAvailable` and `scratchpadEnabled` are true. Otherwise answer in chat; if the pad is off, say once that it can be turned on in Whiteboard Settings. -2. Every source reference names its own pins. For each repository you will quote, `session_register_repository({path})`, then `session_resolve_pins({repositoryId, base: "HEAD", head: "HEAD"})` (or the commit the user is looking at) to get commit ids. Put `pins: {repositoryId, head}` on each `code_peek` source, sequence step source, flow attachment source, call-stack frame source and database operation source; add `base` only when the block compares two commits. Put the same `pins` on a `markdown` block so its `review-source:` links resolve there. A reference without pins is rejected, since the scratchpad has none to lend. +2. Every source reference names its own pins. For each repository you will quote, `session_register_repository({path})`, then `session_resolve_pins({repositoryId, base: "HEAD", head: "HEAD"})` (or the commit the user is looking at) to get commit ids. Put `pins: {repositoryId, head}` on each `code_peek` source, sequence step source (or each of its `sources`), flow attachment source, call-stack frame source and database operation source; add `base` only when the block compares two commits. Put the same `pins` on a `markdown` block so its `review-source:` links resolve there. A reference without pins is rejected, since the scratchpad has none to lend. 3. Read what you cite with `session_file`, `session_tree` or `session_source` at those pins: pass `repositoryId` and `head` (and `base`) to `session_file` and `session_tree`, or `source.pins` to `session_source`. 4. Begin `session_activity({sessionId: "scratchpad", action: "begin", leaseId})` with a fresh UUID, then append blocks with `session_edit({sessionId: "scratchpad", edit: {type: "insert", content}})`. Omitted placement puts the block at the top of the pad, and the response's `targetId` names it. A thought that spans several blocks reads top-down only if you insert it bottom-up (last block first) or give each following block `afterId` of the block you just inserted. Draw a diagram whole in one insert; fix one you drew earlier by patching its nodes, edges or steps by ID, and `replace` a block only when the whole picture was wrong. The block shapes are the same as a session's; follow the edit tool's description for them, and read `session_get_instructions({topic:"authoring"})` for choosing components. End the lease when finished. 5. After your first insert in a session, `session_open({sessionId: "scratchpad"})` once so the pad is showing. Do not call it again for later edits; they appear live. diff --git a/packages/review/src/review-api/authoring-pitfalls.test.ts b/packages/review/src/review-api/authoring-pitfalls.test.ts index 029295f5b..070e942e2 100644 --- a/packages/review/src/review-api/authoring-pitfalls.test.ts +++ b/packages/review/src/review-api/authoring-pitfalls.test.ts @@ -254,10 +254,10 @@ describe("diagram rules", () => { "Unknown component name: cache", )); - it("rejects a step with none or two of source, explanation and code", async () => { + it("rejects a step with none or two of source, sources, explanation and code", async () => { await expectRejected( () => insert(sequence([{ from: "app", to: "db", label: "Write" }])), - "A step needs exactly one of source, explanation, or code.", + "A step needs exactly one of source, sources, explanation, or code.", ); await expectRejected( () => @@ -272,7 +272,39 @@ describe("diagram rules", () => { }, ]), ), - "A step needs exactly one of source, explanation, or code.", + "A step needs exactly one of source, sources, explanation, or code.", + ); + await expectRejected( + () => + insert( + sequence([ + { + from: "app", + to: "db", + label: "Write", + source: selectSource(head("src/store.ts", 1)), + sources: [selectSource(head("src/store.ts", 2))], + }, + ]), + ), + "A step needs exactly one of source, sources, explanation, or code.", + ); + }); + + it("stores a step that carries several code chunks", async () => { + const sources = [ + selectSource(head("src/store.ts", 1)), + selectSource(head("src/store.ts", 3)), + selectSource(head("order.ts", 1)), + ]; + + const result = await insert( + sequence([{ from: "app", to: "db", label: "Write", sources }]), + ); + + expect(result.status).toBe(200); + expect(elements(local.store.read(reviewId).document)).toContainEqual( + expect.objectContaining({ type: "step", sources }), ); }); @@ -343,6 +375,26 @@ describe("source rules in every peek position", () => { }), message, ); + await expectRejected( + () => + insert({ + type: "sequence", + title: "Save", + actors: { app: "App" }, + steps: [ + { + from: "app", + to: "app", + label: "Write", + sources: [ + selectSource(head("src/store.ts", 1)), + selectSource(blank), + ], + }, + ], + }), + message, + ); await expectRejected( () => insert({ diff --git a/packages/review/src/review-api/authoring-tools.ts b/packages/review/src/review-api/authoring-tools.ts index 50880d34f..b0c1132d7 100644 --- a/packages/review/src/review-api/authoring-tools.ts +++ b/packages/review/src/review-api/authoring-tools.ts @@ -25,7 +25,7 @@ export function authoringTools( 'Create a review of saved working files, immutable commits or a GitHub PR. Revisions are resolved on acceptance. Omitted commits base means source at head with no diff; supply the parent to review introduced changes. For a GitHub PR, pullRequestUrl alone is enough: target and title become optional, and the host fetches the PR into a registered checkout of its repository and pins the current PR head and GitHub diff base, titled from the PR. When a review for that PR exists, it is returned instead, reporting whether its head moved and whether another session owns it; update it in place, move its target with review_set_target, or create a separate review with reuseExisting. kind:"scratchpad" names the one scratchpad, which the host creates itself. The result carries review, the review as review_list shows it: its target with resolved commits, origin (its PR), repositoryName and repositoryPath, so no follow-up read is needed before diffing. When Desktop is available the review opens there and the result reports opened, softwareMapEnabled and environmentIssues, as review_open does; set open:false to author in the background without taking over Desktop.', set_target: "Change the review target, preserving document and component IDs. Returns warnings for source references needing repair. Earlier versions keep their retained source.", - edit: 'Edit a document component. Common content shapes: section `{"type":"section","title":"…","children":[]}`; prose `{"type":"markdown","markdown":"…"}`. Sections require `children` (not `blocks`); Markdown uses `markdown` (not `text`). To populate a section later, insert content with `edit.parentId` set to its returned ID. Flow diagram: `{"type":"flow_diagram","title":"Flow","nodes":[{"key":"a","label":"Start"},{"key":"b","label":"Finish"}],"edges":[{"from":"a","to":"b"}]}`. Use nodes and edges arrays (not children); at least one node is required, edges may be empty, and edge endpoints name unique node keys. Code peek: `{"type":"code_peek","source":{"file":"src/app.ts","start":{"side":"head","line":10},"end":{"side":"head","line":20}}}`. Call stack diff: `{"type":"call_stack_diff","title":"Request flow","base":[{"label":"Entry","source":{"file":"src/app.ts","start":{"side":"base","line":10},"end":{"side":"base","line":20}}}],"head":[{"label":"Entry","source":{"file":"src/app.ts","start":{"side":"head","line":10},"end":{"side":"head","line":20}}}]}`. Both base and head arrays are required; either may be empty. Each frame requires source; optional key/parentKey express nesting, and parentKey must name an earlier frame on that side. Sequence: `{"type":"sequence","title":"Request","actors":{"a":"Client","b":"Server"},"steps":[{"from":"a","to":"b","label":"Send","explanation":"Client sends a request."}]}`. actors is a key-to-label object; steps reference actor keys and each needs exactly one of source, explanation, or code. Database lens: `{"type":"database_lens","title":"Read items","actors":{"app":"App"},"stores":{"db":{"label":"DB","storage":"relational","collections":{"items":{"label":"Items","fields":{"id":{"label":"ID","dataType":"integer"}}}}}},"useCases":[{"label":"Load","operations":[{"kind":"read","actor":"app","store":"db","collection":"items","label":"Fetch items","source":{"file":"src/app.ts","start":{"side":"head","line":10},"end":{"side":"head","line":20}}}]}]}`. actors, stores, collections, and fields are keyed objects; useCases and operations are arrays. Include at least one store and use case, with at least one operation per use case; each operation requires source and must reference declared keys. Source paths are repository-relative; line numbers are 1-based and inclusive. Replace example paths and lines with verified source ranges. The host assigns short durable IDs; use returned IDs to edit components in place. The result identifies the edited component and, for an insert or replace, its first-level children, so they can be edited without a follow-up read. Accepted edits are saved immediately. Omitted placement appends; on the scratchpad it lands at the top, so insert a multi-block thought bottom-up or chain each block with afterId. null removes an optional field in a patch. While a reader may be watching, write small and often: one paragraph per edit, so the document draws itself as you go. Insert a new diagram whole, with all its nodes and edges or steps; the board traces it in one quick pass. Change a diagram already on the board one unit at a time: insert, update or remove a flow_node, flow_edge or step by ID (parentId names the diagram). Link each added flow_node to a node already drawn, so it arrives attached; a separate flow_edge is only for two nodes that already exist. Removing a flow_node removes its edges.', + edit: 'Edit a document component. Common content shapes: section `{"type":"section","title":"…","children":[]}`; prose `{"type":"markdown","markdown":"…"}`. Sections require `children` (not `blocks`); Markdown uses `markdown` (not `text`). To populate a section later, insert content with `edit.parentId` set to its returned ID. Flow diagram: `{"type":"flow_diagram","title":"Flow","nodes":[{"key":"a","label":"Start"},{"key":"b","label":"Finish"}],"edges":[{"from":"a","to":"b"}]}`. Use nodes and edges arrays (not children); at least one node is required, edges may be empty, and edge endpoints name unique node keys. Code peek: `{"type":"code_peek","source":{"file":"src/app.ts","start":{"side":"head","line":10},"end":{"side":"head","line":20}}}`. Call stack diff: `{"type":"call_stack_diff","title":"Request flow","base":[{"label":"Entry","source":{"file":"src/app.ts","start":{"side":"base","line":10},"end":{"side":"base","line":20}}}],"head":[{"label":"Entry","source":{"file":"src/app.ts","start":{"side":"head","line":10},"end":{"side":"head","line":20}}}]}`. Both base and head arrays are required; either may be empty. Each frame requires source; optional key/parentKey express nesting, and parentKey must name an earlier frame on that side. Sequence: `{"type":"sequence","title":"Request","actors":{"a":"Client","b":"Server"},"steps":[{"from":"a","to":"b","label":"Send","explanation":"Client sends a request."}]}`. actors is a key-to-label object; steps reference actor keys and each needs exactly one of source, sources, explanation, or code. Use `sources` (an array of up to 10 ranges) when one step spans several code chunks; its tour stop shows them all, with chunks from one file in one card. Database lens: `{"type":"database_lens","title":"Read items","actors":{"app":"App"},"stores":{"db":{"label":"DB","storage":"relational","collections":{"items":{"label":"Items","fields":{"id":{"label":"ID","dataType":"integer"}}}}}},"useCases":[{"label":"Load","operations":[{"kind":"read","actor":"app","store":"db","collection":"items","label":"Fetch items","source":{"file":"src/app.ts","start":{"side":"head","line":10},"end":{"side":"head","line":20}}}]}]}`. actors, stores, collections, and fields are keyed objects; useCases and operations are arrays. Include at least one store and use case, with at least one operation per use case; each operation requires source and must reference declared keys. Source paths are repository-relative; line numbers are 1-based and inclusive. Replace example paths and lines with verified source ranges. The host assigns short durable IDs; use returned IDs to edit components in place. The result identifies the edited component and, for an insert or replace, its first-level children, so they can be edited without a follow-up read. Accepted edits are saved immediately. Omitted placement appends; on the scratchpad it lands at the top, so insert a multi-block thought bottom-up or chain each block with afterId. null removes an optional field in a patch. While a reader may be watching, write small and often: one paragraph per edit, so the document draws itself as you go. Insert a new diagram whole, with all its nodes and edges or steps; the board traces it in one quick pass. Change a diagram already on the board one unit at a time: insert, update or remove a flow_node, flow_edge or step by ID (parentId names the diagram). Link each added flow_node to a node already drawn, so it arrives attached; a separate flow_edge is only for two nodes that already exist. Removing a flow_node removes its edges.', lens: 'Edit one Diff-view lens. Lenses partition the review\'s change for the Diff view; they sit beside the document (never in it) and version with it. The host assigns durable lens IDs; updates replace only the fields supplied. Write one lens per call while a reader may be watching; each draws in on the Diffs page. Requires the lenses lease: review_activity with scope:"lenses", which another agent can hold while the document lease is held elsewhere. The result identifies the lens and reports uncategorized: changed lines no lens selects yet, grouped by file. Keep adding lenses until it is empty or what remains is deliberate. review_lens_get reads the current lenses and gaps.', rename: "Change the review title.", repin: diff --git a/packages/review/src/review-api/blocks/sequence.ts b/packages/review/src/review-api/blocks/sequence.ts index 4c3061c4a..524988e28 100644 --- a/packages/review/src/review-api/blocks/sequence.ts +++ b/packages/review/src/review-api/blocks/sequence.ts @@ -11,6 +11,8 @@ import { text, } from "./definition.js"; +const maxStepSources = 10; + export const stepSchema = z .strictObject({ ...identity, @@ -20,14 +22,23 @@ export const stepSchema = z label, style: z.enum(["call", "return", "async"]).default("call"), source: lensSourceSchema.optional(), + sources: z + .array(lensSourceSchema) + .min(1) + .max(maxStepSources) + .optional() + .describe( + "Several code chunks for one step, shown together in its tour stop. Chunks in the same file share one card.", + ), explanation: label.optional(), code: z.strictObject(codeFields).optional(), }) .refine( (s) => - [s.source, s.explanation, s.code].filter((v) => v !== undefined) - .length === 1, - "A step needs exactly one of source, explanation, or code.", + [s.source, s.sources, s.explanation, s.code].filter( + (v) => v !== undefined, + ).length === 1, + "A step needs exactly one of source, sources, explanation, or code.", ); export type Step = z.infer; diff --git a/packages/review/src/review-api/document-text.ts b/packages/review/src/review-api/document-text.ts index 0912932f3..2d0561429 100644 --- a/packages/review/src/review-api/document-text.ts +++ b/packages/review/src/review-api/document-text.ts @@ -87,6 +87,8 @@ export function documentText( if (element.source) detail(sourceText(element.source)); + if (element.sources) detail(element.sources.map(sourceText).join(", ")); + if (detailed && element.explanation) detail(element.explanation); if (detailed && element.code) code(depth + 1, element.code); diff --git a/packages/review/src/review-api/document.ts b/packages/review/src/review-api/document.ts index 71d9913b3..1c6346d54 100644 --- a/packages/review/src/review-api/document.ts +++ b/packages/review/src/review-api/document.ts @@ -286,6 +286,16 @@ function documentReferences( ), ); + // Each chunk of a multi-chunk step is its own reference, so one stale + // chunk marks only itself. + if (element.type === "step" && element.sources) + return element.sources.map((source, index) => ({ + id: `${element.id}:${index}`, + source, + label: element.label, + peek: true, + })); + if (element.type === "call_stack_diff") return [...element.base, ...element.head].flatMap((frame) => [ { ...frame, id: frame.id!, peek: true }, From b192a695a46135b8465f79f032d59043eabe03ea Mon Sep 17 00:00:00 2001 From: Jaeyoon Kim Date: Sat, 26 Sep 2026 20:30:34 -0400 Subject: [PATCH 4/4] Tell authors to use several sources when a step spans places Co-Authored-By: Claude Opus 5.5 --- packages/review/instructions/authoring.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/review/instructions/authoring.md b/packages/review/instructions/authoring.md index b1bee202f..f431affe0 100644 --- a/packages/review/instructions/authoring.md +++ b/packages/review/instructions/authoring.md @@ -36,3 +36,4 @@ follow these first six steps exactly, without any extraneous tool calls. - when something (a phrase in the prose, diagram node, etc.) describes actual code in the codebase, always default to attaching/hyperlink code. - Link repository code as `[label](review-source:head/src/file.ts#L10-L24)`; use `base` for old code. Use repository-relative paths and verified line numbers. - point each code reference (diagram step, call-stack frame, `code_peek`) at the smallest range that shows the claim, usually 3-15 lines: the call, the branch, the assignment. not the whole function. tour steps and peeks show only that range. +- when a sequence step's story spans several places (a setting, its gate, its effect), give it `sources` instead of one wide range. list them in reading order. chunks in one file share a card with the gap folded; chunks in other files stack.