From c8c2bddd5cd7fcfd4beea569c0cfba1ea0052d41 Mon Sep 17 00:00:00 2001 From: Jaeyoon Kim Date: Thu, 15 Oct 2026 16:41:42 -0700 Subject: [PATCH] Let a sequence step show several code ranges A step stands for one call, but one range often can't show it: the call reads wrong without the type it passes or the code that builds its argument. A step's source can now list up to four ranges: the call site, then ranges directly about that call. Each range is its own reference and its own code card, spaced 12px apart with the tour rail running faintly through the gaps. The sequence check refuses a range listed twice in a step, and an extra range that holds another step's code (that is the next step, not more of this one). A range longer than a call (over 12 lines) comes back as a warning. The component reference tells authors a step's source is the line its arrow leaves from, and to add a range only when that line can't be read without it. Code peek and tour cards now keep their context to the pinned lines plus each enclosing scope's first and last lines, instead of filling whole context runs that touch a scope boundary. Rewritten on current main from Jaeyoon Kim's original multi-chunk change. Co-authored-by: ketan0 Co-Authored-By: Claude Opus 5.5 Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- .../src/vs/review/common/reviewLens.test.ts | 74 +++++++++++++++ .../src/vs/review/common/reviewLens.ts | 24 +++-- .../vs/review/common/reviewStructuralDiff.ts | 3 +- .../review/app/src/CodePeek.browser.test.tsx | 89 ++++++++++++++++++- packages/review/app/src/CodePeek.tsx | 53 ++++++++++- packages/review/app/src/diagrams.tsx | 27 ++++-- packages/review/app/src/review-components.tsx | 8 +- packages/review/app/src/review-panel-model.ts | 3 +- packages/review/app/src/sequence-tour.test.ts | 4 +- packages/review/app/src/side-panel-styles.ts | 7 +- .../src/review-api/authoring-pitfalls.test.ts | 64 +++++++++++++ .../src/review-api/blocks/definition.ts | 4 +- .../review/src/review-api/blocks/index.ts | 19 ++++ .../review/src/review-api/blocks/sequence.ts | 82 ++++++++++++++++- .../src/review-api/component-reference.ts | 3 +- .../review/src/review-api/document-text.ts | 9 +- packages/review/src/review-api/document.ts | 22 +++-- .../review/src/review-api/local-data.test.ts | 54 ++++++++++- packages/review/src/review-api/store.ts | 19 ++-- 19 files changed, 521 insertions(+), 47 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 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 && ( +