Skip to content

Keep tour steps and code peeks clipped to their pinned lines - #627

Merged
ketan0 merged 2 commits into
devdotfast:mainfrom
jykim256:fix/exact-document-lens
Oct 9, 2026
Merged

ketan0 merged 2 commits into
devdotfast:mainfrom
jykim256:fix/exact-document-lens

Conversation

@jykim256

@jykim256 jykim256 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Problem

When you step through a sequence diagram's tour, or open a code peek, the code card is supposed to show just that step's line range, like workflow_evaluation.py:199-204. Instead it shows the whole file for a newly added file, and the whole diffr hunk region for a modified file.

This broke in #502, "Preserve diffr context when applying review lenses." That PR made lensContextGaps() honor diffr's structural scope info (diff.contextScopes). That's the right call for the Diff tab, where you want a lens to keep the syntactic context around the code it highlights. But tour steps and code peeks use a different kind of lens. They build a "document" lens with one exact chunk in mind, and that lens goes through the same lensContextGaps() function.

The bug comes from the run-fill rule. When contextScopes is set, lensContextGaps() shows every unfolded run of lines that touches the lens range. For an added file, diffr folds nothing, so the whole file is one run. One small lens range anywhere in that file makes the whole file visible.

Fix

Added a new exact flag to ReviewDiffLens. The document lens (used by tour steps and code peeks) sets exact: true. When withLens() sees a lens with exact: true, it strips contextScopes off the diff before calling lensContextGaps(). Without contextScopes, only the old +/-3 line window around the pinned range applies, same as before #502.

Lenses used by the Diff tab never set exact, so nothing changes there. #502's scope-preserving behavior is still in effect for normal diff review.

Authoring guidance

  • instructions/authoring.md now tells authors to pin the smallest range that shows the claim (usually 3-15 lines), since tour steps and peeks show only that range.

Reviewer notes

This touches the vendored Code - OSS fork (apps/review-desktop/code-oss/src/vs/review/**) and adds an optional field to ReviewDiffLens in review-protocol. No stored-data or telemetry changes.

Testing

image

Before left, After right

Added a test in reviewLens.test.ts that builds an added-file diff (300 lines, all inserted) with a scope covering the whole file, and a lens range of lines 80-87. It checks two things:

  • Without stripping contextScopes (today's Preserve diffr context when applying review lenses #502 behavior), the whole file is visible — no gaps at all. This reproduces the bug.
  • With contextScopes stripped (what exact: true triggers), only the +/-3 window around lines 80-87 is visible. This is the fix.

Ran the full test suite for apps/review-desktop:

  • code-oss/src/vs/review/**/*.test.ts (204 tests via node --test) — all pass.
  • scripts/**/*.test.mjs (120 tests) — 1 pre-existing failure (curated-extensions.test.mjs, "extracts nested Windows executables from a VSIX archive"), confirmed failing the same way on main before this change, unrelated to this fix (missing native module for VSIX extraction).

I did not run typecheck-client (tsgo over the whole code-oss client), because it needs the separate Electron-ABI npm install in code-oss/. The touched files compile under tsx in the test runs above. CI should cover the full typecheck.

🤖 Generated with Claude Code

@thesiti92
thesiti92 requested a review from sidkmenon September 28, 2026 17:37
@milanb17
milanb17 self-requested a review September 30, 2026 17:47
@milanb17 milanb17 self-assigned this Sep 30, 2026
@jykim256
jykim256 force-pushed the fix/exact-document-lens branch from 4a84ea1 to fcc7ccc Compare October 6, 2026 15:32
jykim256 and others added 2 commits October 8, 2026 23:25
Tour steps and code peeks build a "document:" lens that goes through
lensContextGaps(). Since devdotfast#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 devdotfast#502's behavior there is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2
Since devdotfast#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 <noreply@anthropic.com>
Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2
@ketan0
ketan0 force-pushed the fix/exact-document-lens branch from fcc7ccc to 9242418 Compare October 9, 2026 07:03
@ketan0
ketan0 merged commit 823c9bd into devdotfast:main Oct 9, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants