From 8b6b55fa89d8532f49b9557d56f076a0a1106c6a 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 Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- .../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 858a7a608..ff83fd758 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 @@ -34,6 +34,20 @@ test('a clipped outer fold leaves its nested fold usable in a code peek', () => assert.deepEqual(whole.filter(gap => gap.foldStateId).map(gap => gap.foldStateId), [1]); }); +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 687a271b1..8edb51a4c 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 @@ -65,7 +65,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 382f57021..f49879dc4 100644 --- a/packages/review-protocol/src/contracts.ts +++ b/packages/review-protocol/src/contracts.ts @@ -200,6 +200,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 92424183a9794e84f58213c42e26a210b3effeda Mon Sep 17 00:00:00 2001 From: ketan0 Date: Thu, 8 Oct 2026 00:26:56 -0700 Subject: [PATCH 2/2] Show a scope's edges, not the whole added file, around a peek Since #502, a lens with diffr's contextScopes shows every open run (rows outside a collapsed band) that holds a pinned line or the first or last line of a scope enclosing one. diffr collapses nothing in an added file, so its one open run is the whole file, and a peek pinned to a few lines showed all of it. Replace the `exact` lens flag with a narrower rule in lensContextGaps: an open run made only of added lines shows only the pinned lines, three lines around them, and the first and last lines of their enclosing scopes. diffr never collapses inside added code, so no band of its own bounds such a run; a scope the reader folded only splits it. Runs with any base-side line are unchanged, so modified files keep diffr's context, and peeks and tour steps keep diffr's scopes instead of dropping them. The rule also applies to Diff-tab section lenses on added files, which now get the same narrow view. Co-Authored-By: Claude Opus 5.5 Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- .../src/vs/review/common/reviewLens.test.ts | 51 +++++++++++++++---- .../src/vs/review/common/reviewLens.ts | 5 +- .../review/services/reviewApiSourceService.ts | 2 +- .../src/vs/review/services/reviewLens.ts | 5 +- packages/review-protocol/src/contracts.ts | 2 - 5 files changed, 46 insertions(+), 19 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 ff83fd758..7799b376f 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 @@ -1,7 +1,7 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { lensContextGaps } from './reviewLens.js'; -import type { IDocumentDiff } from '../../editor/common/diff/documentDiffProvider.js'; +import type { IDocumentContextGap, IDocumentDiff } from '../../editor/common/diff/documentDiffProvider.js'; const plain: IDocumentDiff = { changes: [], moves: [], identical: false, quitEarly: false }; test('a lens keeps disjoint attachments and hides the intervening code', () => { @@ -34,18 +34,47 @@ test('a clipped outer fold leaves its nested fold usable in a code peek', () => assert.deepEqual(whole.filter(gap => gap.foldStateId).map(gap => gap.foldStateId), [1]); }); -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 = { +// The head lines left on screen, as [from, to] runs. +const shownLines = (gaps: readonly IDocumentContextGap[], lineCount: number) => { + const runs: [number, number][] = []; + for (let line = 1; line <= lineCount; line++) { + if (gaps.some(gap => gap.collapsed !== false && line >= gap.modifiedStart && line < gap.modifiedStart + gap.modifiedCount)) continue; + const last = runs.at(-1); + if (last && last[1] === line - 1) last[1] = line; + else runs.push([line, line]); + } + return runs; +}; +const head = (fromLine: number, toLine: number) => [{ side: 'head' as const, file: 'a.ts', fromLine, toLine }]; + +test('in an added file a lens shows the first and last lines of its enclosing scopes, not the whole file', () => { + // A 30-line added file: a class on lines 3-25 with a method on lines 10-18. Nothing is collapsed. + const added: IDocumentDiff = { + ...plain, + sourceLineAlignment: Array.from({ length: 30 }, (_, i) => [null, i] as const), + contextScopes: { original: [], modified: [[2, 25], [9, 18]] }, + }; + assert.deepEqual(shownLines(lensContextGaps(added, 0, 30, head(13, 14)), 30), [[3, 3], [10, 18], [25, 25]]); + // The same, with a function on lines 27-29 folded. + const folded: IDocumentDiff = { + ...added, + contextScopes: { original: [], modified: [[2, 25], [9, 18], [26, 29]] }, + contextGaps: [{ originalStart: 1, originalCount: 0, modifiedStart: 27, modifiedCount: 3, collapsed: true }], + }; + assert.deepEqual(shownLines(lensContextGaps(folded, 0, 30, head(13, 14)), 30), [[3, 3], [10, 18], [25, 25]]); +}); + +test('in a modified file a lens shows the whole run between collapsed bands', () => { + // A function on lines 5-20, with the code before and after it collapsed. + const modified: IDocumentDiff = { ...plain, - sourceLineAlignment: Array.from({ length: 300 }, (_, i) => [null, i] as const), - contextScopes: { original: [], modified: [[0, 300] as const] }, + contextScopes: { original: [[4, 20]], modified: [[4, 20]] }, + contextGaps: [ + { originalStart: 1, originalCount: 4, modifiedStart: 1, modifiedCount: 4, collapsed: true }, + { originalStart: 21, originalCount: 10, modifiedStart: 21, modifiedCount: 10, collapsed: true }, + ], }; - 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]]); + assert.deepEqual(shownLines(lensContextGaps(modified, 30, 30, head(15, 15)), 30), [[5, 20]]); }); test('viewed folds never hide an unread counterpart, and do not overlap structural folds', async () => { 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 1289ec812..11592fdb1 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 @@ -35,7 +35,10 @@ 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)) context.fill(true, start, end); + // diffr never collapses inside added code, so no band bounds a run of + // only added lines (as in an added file). Keep only its seeds. + if (rows.slice(start, end).every(row => row[0] === null)) for (let index = start; index < end; index++) context[index] ||= seeds[index]; + else if (seeds.slice(start, end).some(Boolean)) context.fill(true, start, end); start = end; } } 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 b50e5bcf3..b27f49d51 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, exact: true, + reviewId: current.reviewId, version: current.version, 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 8edb51a4c..687a271b1 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 @@ -65,10 +65,7 @@ export function withLens(instantiation: IInstantiationService, entries: readonly }) ? { ...gap, collapsed: false } : gap) }; const section = progress()?.sections?.find(section => section.id === entry.sectionId); - // 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; + return lens && !lens.wholeFiles ? { ...diff, contextGaps: lensContextGaps(diff, 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 f49879dc4..382f57021 100644 --- a/packages/review-protocol/src/contracts.ts +++ b/packages/review-protocol/src/contracts.ts @@ -200,8 +200,6 @@ 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;