Skip to content

Hookup semantic tokens range refresh notification to the viewport contribution#271419

Merged
alexdima merged 2 commits intomicrosoft:mainfrom
dibarbet:fix_semantic_tokens_range_refresh
Oct 24, 2025
Merged

Hookup semantic tokens range refresh notification to the viewport contribution#271419
alexdima merged 2 commits intomicrosoft:mainfrom
dibarbet:fix_semantic_tokens_range_refresh

Conversation

@dibarbet
Copy link
Member

@dibarbet dibarbet commented Oct 15, 2025

Resolves #268140

The semantic tokens range refresh event was introduced recently in #268148 however it looks like it was not hooked up to the viewportSemanticTokens part, and so didn't actually refresh. Took a stab at fixing it and adding a test for it. Used the document provider as inspiration here - https://github.com/microsoft/vscode/blob/main/src/vs/editor/contrib/semanticTokens/browser/documentSemanticTokens.ts#L153

Also tested with dibarbet/vscode-extension-samples@59cd751 to make sure it works

refresh_semantic_tokens.mov

@dibarbet dibarbet marked this pull request as ready for review October 15, 2025 07:18
Copilot AI review requested due to automatic review settings October 15, 2025 07:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where semantic tokens range refresh notifications were not properly hooked up to the viewport semantic tokens contribution, preventing actual refresh when providers fire their onDidChange events.

Key changes:

  • Adds event listener registration for provider onDidChange events in the viewport semantic tokens contribution
  • Includes comprehensive test coverage to verify the fix works correctly

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/vs/editor/contrib/semanticTokens/browser/viewportSemanticTokens.ts Implements the core fix by adding change listener management for range providers
src/vs/editor/contrib/semanticTokens/test/browser/viewportSemanticTokens.test.ts Adds test coverage to verify provider onDidChange events trigger viewport refresh

@aeschli aeschli requested a review from alexdima October 15, 2025 21:31
@dibarbet
Copy link
Member Author

@alexdima let me know if there is anything I can do to move this forward - would like to try and get this working in the oct update as there is a bit of a chain of dependencies that need to update

@aeschli
Copy link
Contributor

aeschli commented Oct 21, 2025

@alexdima Can you have a look?

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

🚀 Beautiful! Thank you!!

@alexdima alexdima enabled auto-merge (squash) October 24, 2025 10:47
@vs-code-engineering vs-code-engineering bot added this to the October 2025 milestone Oct 24, 2025
@alexdima alexdima merged commit edff076 into microsoft:main Oct 24, 2025
17 checks passed
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Dec 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DocumentRangeSemanticTokensProvider has no support for onDidChangeSemanticTokens

5 participants