-
Notifications
You must be signed in to change notification settings - Fork 215
Add IntelliSense support for Blazor enhanced navigation and form handling data attributes #12324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: davidwengier <[email protected]>
Co-authored-by: davidwengier <[email protected]>
…h passing tests Co-authored-by: davidwengier <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Please create additional test(s) in CohostDocumentCompletionEndpointTest.cs
Also there seems to be test failures. Please make sure you run build.sh -test
before finishing work
} | ||
|
||
// Only show data-enhance-nav for anchor elements | ||
if (attributeName == "data-enhance-nav" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot data-enhance-nav
can go on any element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit. Removed the element-type filtering for data-enhance-nav
- it can now appear on any element.
else if (attribute.Kind == AspNetCore.Razor.Language.SyntaxKind.MarkupMinimizedAttributeBlock) | ||
{ | ||
var minAttrBlock = (MarkupMinimizedAttributeBlockSyntax)attribute; | ||
if (string.Equals(minAttrBlock.Name.GetContent(), attributeName, System.StringComparison.Ordinal)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot this logic is the same as the above condition. Would be better to use a switch over attribute.Kind
to get the attribute name, and then just have one check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored in commit to use a switch statement over attribute.Kind
to extract the attribute name, eliminating the duplicated logic.
…nd refactor duplicate check Co-authored-by: davidwengier <[email protected]>
Added three test cases to CohostDocumentCompletionEndpointTest.cs covering data-enhance on form, data-enhance-nav on div, and data-permanent on anchor elements. All existing BlazorDataAttributeCompletionItemProviderTest tests (9/9) pass. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@copilot There are test failures. To run the tests either run |
… and fix test trigger characters Co-authored-by: davidwengier <[email protected]>
Fixed in commit 48168f8. The issues were:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
// Adjust owner similar to TagHelperCompletionProvider | ||
owner = owner switch | ||
{ | ||
MarkupStartTagSyntax or MarkupEndTagSyntax or MarkupTagHelperStartTagSyntax or MarkupTagHelperEndTagSyntax or MarkupTagHelperAttributeSyntax => owner, | ||
RazorDocumentSyntax => owner, | ||
_ => owner.Parent | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Rather than copying code from TagHelperCompletionProvider, it would be better to extract the code from that class into a help, perhaps in an extension class, and then call it from TagHelperCompletionProvider and here. As it is, by coping the code you seem to have lost the comments in the original, which explain the context, and without it I don't understand why this block is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted shared logic into CompletionContextHelper.AdjustSyntaxNodeForCompletion() with full comments explaining the context. Updated both BlazorDataAttributeCompletionItemProvider and TagHelperCompletionProvider to use this shared helper.
} | ||
|
||
// Only provide completions when we're completing an attribute name | ||
// Similar to TagHelperCompletionProvider logic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Similar to above, don't comment "similar to" some other file, but rather share code if possible. If sharing isn't possible, include any comments explaining what this does. The if statement is hard to understand and it might be more readable to expand it into multiple checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted into CompletionContextHelper.IsAttributeNameCompletionContext() with detailed comments explaining the multi-line attribute parsing heuristic. Both providers now share this logic.
} | ||
|
||
var containingTagName = containingTagNameToken.Content; | ||
var completionItems = ImmutableArray.CreateBuilder<RazorCompletionItem>(s_blazorDataAttributes.Length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Use a PooledArrayBuilder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use PooledArrayBuilder.
continue; | ||
} | ||
|
||
// data-enhance-nav can go on any element, no filtering needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot This comment is unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the comment.
|
||
// data-enhance-nav can go on any element, no filtering needed | ||
|
||
// Check if the attribute already exists on the element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I think this logic is good, but there is no test coverage for it. Please add some to CohostDocumentCompletionEndpointTest.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test BlazorDataAttributeCompletion_DoesNotDuplicateExistingAttribute in CohostDocumentCompletionEndpointTest.cs that verifies data-enhance doesn't appear in completion when it already exists on the element.
foreach (var (attributeName, description) in s_blazorDataAttributes) | ||
{ | ||
// Only show data-enhance for form elements | ||
if (attributeName == "data-enhance" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Please make sure there is test coverage in CohostDocumentCompletionEndpointTest.cs to ensure data-enhance
does NOT appear in completion lists for other elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test coverage already exists - BlazorDataEnhanceNavAttributeCompletion_OnAnyElement and BlazorDataPermanentAttributeCompletion_OnAnchorElement both verify data-enhance does NOT appear on non-form elements via unexpectedItemLabels.
AspNetCore.Razor.Language.SyntaxKind.MarkupAttributeBlock => ((MarkupAttributeBlockSyntax)attribute).Name.GetContent(), | ||
AspNetCore.Razor.Language.SyntaxKind.MarkupMinimizedAttributeBlock => ((MarkupMinimizedAttributeBlockSyntax)attribute).Name.GetContent(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AspNetCore.Razor.Language.SyntaxKind.MarkupAttributeBlock => ((MarkupAttributeBlockSyntax)attribute).Name.GetContent(), | |
AspNetCore.Razor.Language.SyntaxKind.MarkupMinimizedAttributeBlock => ((MarkupMinimizedAttributeBlockSyntax)attribute).Name.GetContent(), | |
MarkupAttributeBlock attributeBlock => attributeBlock .Name.GetContent(), | |
MarkupMinimizedAttributeBlock minimizedAttributeBlock => minimizedAttributeBlock.Name.GetContent(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied the suggested pattern matching simplification.
PropertyName: attributeName, | ||
Documentation: description); | ||
|
||
var completionItem = RazorCompletionItem.CreateTagHelperAttribute( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot This isn't a tag helper attribute, so it doesn't make sense to create a fake BoundAttributeDescriptionInfo, and call the CreateTagHelperAttribute method. Create a new method on RazorCompletionItem called CreateAttribute, that uses a new RazorCompletionItemKind, and create a new AttributeDescriptionInfo for the description, and handle it in RazorCompletionItemResolver etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created AttributeDescriptionInfo record, added RazorCompletionItemKind.Attribute enum value, created RazorCompletionItem.CreateAttribute method, and updated RazorCompletionItemResolver and RazorCompletionListProvider to handle the new kind appropriately.
new DirectiveAttributeParameterCompletionItemProvider(), | ||
new DirectiveCompletionItemProvider() | ||
new DirectiveCompletionItemProvider(), | ||
new BlazorDataAttributeCompletionItemProvider() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Undo this change, we don't need to touch the legacy editor, and it doesn't support components anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted the Legacy Editor change.
…ilder, simplify switch, remove Legacy Editor Co-authored-by: davidwengier <[email protected]>
…dd test coverage Co-authored-by: davidwengier <[email protected]>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@copilot please merge main into your branch |
Merged origin/main successfully. The merge moved test files to a new location (Endpoints/Shared/) but all changes are preserved. All tests pass (9/9 unit tests, 4/4 cohost tests). Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
Successfully refactored IntelliSense support for Blazor data attributes:
Refactoring Changes
AdjustSyntaxNodeForCompletion
: Finds nearest start/end tag for completionIsAttributeNameCompletionContext
: Determines if cursor is in attribute name contextTests
All tests passing after merge:
Merge Details
Merged origin/main which moved test files from
Microsoft.VisualStudio.LanguageServices.Razor.Test/Cohost/Shared/
toMicrosoft.VisualStudioCode.RazorExtension.Test/Endpoints/Shared/
. All test changes preserved and working correctly.Original prompt
Fixes #9378
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.