Skip to content

fix: clear stale R code chunk decorations - #1866

Open
sean0407 wants to merge 3 commits into
REditorSupport:mainfrom
sean0407:codex/clear-stale-chunk-decorations
Open

sean0407 wants to merge 3 commits into
REditorSupport:mainfrom
sean0407:codex/clear-stale-chunk-decorations

Conversation

@sean0407

@sean0407 sean0407 commented Oct 11, 2026 •

Copy link
Copy Markdown

Summary

when an R document has no remaining code chunks
clear both active chunk border decorations

This PR is scoped to stale borders after all chunk markers are removed.

Evidence

  • Before: Issue #1715 reports the current chunk borders remain after the chunk markers are removed.
  • After: highlightCurrentChunk() clears the top and bottom decorations when no current chunk exists. The extension-host regression test removes the chunk markers and verifies both decoration calls receive empty ranges.
  • Checks performed: pnpm run pretest, pnpm run lint, git diff --check, and the focused extension-host test with VS Code 1.141.0 passed.

Merge Danger

Door: two-way

Blast Radius: localized

The change affects chunk-border decorations in the active R document when no chunks remain.

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The production change clears both borders when all chunk markers are removed, including when the code remains in the document. I verified this in an isolated VS Code 1.141.0 extension host using the PR source, a supported cancellation token, and a mutable editor wrapper. The added regression test needs the two fixes below before it can compile and run.

Validation at cf66499: tsc --noEmit -p ./ reports TS2339 at test lines 19 and 30; running the emitted, otherwise unchanged regression test in the extension host fails at line 17 with Cannot redefine property: setDecorations (0 passing, 1 failing). Targeted lint also reports errors for the invalid cancellation-token expression.

One scope clarification: moving outside a chunk still leaves its borders visible. getCurrentChunk() returns the first/last/next chunk for positions outside chunk bounds, so the new else is not reached while any chunk exists. I reproduced this above and below a chunk. If that part of #1715 is intended to be covered here, the decoration path also needs an isWithinChunk() check and a regression case; otherwise, please narrow the description to clearing borders when no chunks remain and retain the cursor-outside behavior and disable-borders option as follow-ups.

const provider = new RMarkdownCodeLensProvider();
const setDecorations = sinon.spy(editor, 'setDecorations');
try {
provider.provideCodeLenses(document, vscode.CancellationToken.None);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Use a public cancellation token source

CancellationToken is an interface in the VS Code API, so vscode.CancellationToken.None is unavailable: TypeScript reports TS2339 here and at line 30, and the runtime export is undefined. This prevents the CI test-compilation step from completing. Create a new vscode.CancellationTokenSource(), pass its .token to both calls, and dispose the source during cleanup.

editor.selection = new vscode.Selection(1, 0, 1, 0);

const provider = new RMarkdownCodeLensProvider();
const setDecorations = sinon.spy(editor, 'setDecorations');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Spy through a mutable editor wrapper

The real TextEditor returned by showTextDocument() is frozen, and setDecorations is non-configurable. Consequently, sinon.spy(editor, 'setDecorations') throws TypeError: Cannot redefine property: setDecorations before the test reaches its assertions or cleanup. I reproduced this with the emitted PR test in VS Code 1.141.0. Fixing the token alone will therefore still leave the regression test failing. Supply a mutable editor wrapper through a stub of visibleTextEditors, put the spy on that wrapper, and delegate to the real editor's method, or introduce an equivalent test seam.

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the update at 454c8c8. Both earlier blockers are resolved: the test uses the public CancellationTokenSource API, and the injected editor wrapper avoids modifying VS Code's frozen TextEditor. The default editor getter preserves the production behavior.

Validation: TypeScript compilation (tsc -p ./) and lint (oxlint src) pass. The unchanged updated regression test passes in an isolated VS Code 1.141.0 extension host (1 passing). Temporarily removing the border-clearing branch from the compiled review copy makes the test fail at the expected empty-range assertion, confirming that it detects the original regression.

No remaining blocking findings for the stated scope of clearing stale borders when no chunks remain. The narrowed description addresses the earlier scope concern; clearing borders when moving outside a chunk and the option to disable borders remain follow-ups for #1715.

@sean0407
sean0407 force-pushed the codex/clear-stale-chunk-decorations branch from 454c8c8 to a71850e Compare October 21, 2026 08:29

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the rebased revision at a71850e. The implementation and regression test match the previously approved 454c8c8 revision after normalizing TypeScript to JavaScript; the differences are formatting and integration with the newer base. No new actionable findings, and the earlier test blockers remain fixed.

Validation on this revision: TypeScript compilation (tsc -p ./) and lint (oxlint src) pass. Formatting passes for both changed files. The focused, unchanged regression test passes in an isolated VS Code 1.141.0 extension host (1 passing).

The repository-wide formatting check reports src/test/suite/dataViewer.test.ts; that file is byte-for-byte identical to the current PR base, so this is an existing base issue rather than a finding introduced by this PR. Approval stands for the stated scope of clearing stale borders when no chunks remain.

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.

2 participants