Skip to content

Conversation

alexgav
Copy link
Contributor

@alexgav alexgav commented Jul 8, 2025

Summary of the changes

  • Don't add html snippets to completion inside of style, script, and HTML comment blocks
  • Ideally HTML snippets should be added by HTML LSP server to the returned completion items rather than Razor is doing it

Fixes:
https://developercommunity.visualstudio.com/t/Snippets-are-way-too-agressive-in-Razor-/10676182

@alexgav alexgav requested a review from a team as a code owner July 8, 2025 05:01
Comment on lines 233 to 235
if (RazorSyntaxFacts.IsInStyleBlock(node)
|| RazorSyntaxFacts.IsInScriptBlock(node)
|| RazorSyntaxFacts.IsInMarkupCommentBlock(node))
Copy link
Member

Choose a reason for hiding this comment

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

Having these helper methods is nice, but I think in this case we probably want to have more explicit code, as this is currently doing 3 complete tree walks all the way up the tree, every time. Combining these into one FirstAncestorOrSelf call, would be much better. Could still use helper methods to identify the actual elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Walks up the tree shouldn't be too bad, should they? Or is the razor syntax tree insanely deep?

Copy link
Member

Choose a reason for hiding this comment

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

It's definitely not the worst thing, and I would say that most of the time Razor trees aren't very deep, no, but just seems like three walks that could be trivially combined into one is pretty low hanging fruit, and being completion its sort of on the typing path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Todd, I didn't think it would be a big deal since we are really walking up just to the first containing element, which really shouldn't be too deep.

At the same time, happy to implement this, shouldn't be too bad.

@alexgav alexgav merged commit dc47633 into main Jul 9, 2025
11 checks passed
@alexgav alexgav deleted the dev/alexgav/NoHtmlSnippetsInDisallowedLocations branch July 9, 2025 08:00
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 9, 2025
@RikkiGibson RikkiGibson modified the milestones: Next, 18.0 P1 Aug 20, 2025
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