From 662c9d53e41f93aadb8ac8784ded3537487009d4 Mon Sep 17 00:00:00 2001 From: Jaeyoon Kim Date: Sat, 26 Sep 2026 19:38:39 -0400 Subject: [PATCH 1/2] 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 b27f49d51..b50e5bcf3 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 @@ -287,7 +287,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 67e486393..b1a5478b2 100644 --- a/packages/review-protocol/src/contracts.ts +++ b/packages/review-protocol/src/contracts.ts @@ -199,6 +199,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 fcc7cccd9d028b59265bc0923bde09a949b5d0c0 Mon Sep 17 00:00:00 2001 From: Jaeyoon Kim Date: Sat, 26 Sep 2026 20:30:10 -0400 Subject: [PATCH 2/2] 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 438b9e7b5..593eb0094 100644 --- a/packages/review/instructions/authoring.md +++ b/packages/review/instructions/authoring.md @@ -34,3 +34,4 @@ follow these first five steps in order, with no tool calls beyond what they need - 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.