Skip to content

Some code actions ordering and cleanup #11659

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 26, 2025
Merged

Some code actions ordering and cleanup #11659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
54a350d
Always specify a name for code actions, and verify the same
davidwengier Mar 25, 2025
9162078
Add failing test to ensure add using is offered first
davidwengier Mar 25, 2025
0029139
Order Razor code actions by the Order property
davidwengier Mar 26, 2025
0b7c1a8
Offer extract to code behind at the start of an `@code` block
davidwengier Mar 26, 2025
5bea401
Update skipped tests
davidwengier Mar 26, 2025
8ac8129
Update old tests to use names instead of titles
davidwengier Mar 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/CodeActionsService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ private static async Task<ImmutableArray<RazorVSInternalCodeAction>> Consolidate
{
var results = await Task.WhenAll(tasks).ConfigureAwait(false);

using var codeActions = new PooledArrayBuilder<RazorVSInternalCodeAction>();
using var codeActions = new PooledArrayBuilder<RazorVSInternalCodeAction>(capacity: tasks.Length);

cancellationToken.ThrowIfCancellationRequested();

Expand All @@ -255,7 +255,7 @@ private static async Task<ImmutableArray<RazorVSInternalCodeAction>> Consolidate
codeActions.AddRange(result);
}

return codeActions.ToImmutable();
return codeActions.DrainToImmutableOrderedBy(static r => r.Order);
}

private static ImmutableHashSet<string> GetAllAvailableCodeActionNames()
Expand Down
6 changes: 6 additions & 0 deletions ...c/Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/Models/RazorVSInternalCodeAction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,10 @@ internal sealed class RazorVSInternalCodeAction : VSInternalCodeAction
[JsonPropertyName("name")]
[DataMember(Name = "name")]
public string? Name { get; set; }

/// <summary>
/// The order code actions should appear. This is not serialized as its just used in the code actions service
/// </summary>
[JsonIgnore]
public int Order { get; set; }
}
3 changes: 3 additions & 0 deletions ....CodeAnalysis.Razor.Workspaces/CodeActions/Razor/ExtractToCodeBehindCodeActionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeAct
// When the caret is '@$$code' or '@c$$ode' or '@co$$de' or '@cod$$e' then tree is:
// RazorDirective -> RazorDirectiveBody -> MetaCode
RazorDirectiveBodySyntax { Parent: RazorDirectiveSyntax d } => d,
// When the caret is '$$@code' then tree is:
// RazorDirective
RazorDirectiveSyntax d => d,
_ => null
};
if (directiveNode is null)
Expand Down
30 changes: 20 additions & 10 deletions ...r/src/Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/Razor/RazorCodeActionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ public static RazorVSInternalCodeAction CreateAddComponentUsing(string @namespac
Title = newTagName is null ? title : $"{newTagName} - {title}",
Data = data,
TelemetryId = s_addComponentUsingTelemetryId,
Priority = VSInternalPriorityLevel.High
Priority = VSInternalPriorityLevel.High,
Name = LanguageServerConstants.CodeActions.AddUsing,
// Adding a using for an existing component should be first
Order = -1000,
Copy link
Contributor

@ryzngard ryzngard Mar 26, 2025

Choose a reason for hiding this comment

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

💡 Should these be in some kind of constants file? That way it's easier to compare across actions which comes before another and what space is available if needed

Copy link
Member Author

@davidwengier davidwengier Mar 26, 2025

Choose a reason for hiding this comment

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

IMO no, but I'm happy to go with majority rules if someone wants to move them. My arguments are a) They're only specified in this file anyway and b) I expect setting Order to be the exception not the rule.

I thought about porting over Roslyns [Extension(After = ...)] but I thought about it for a bit and decided it would make things hard to navigate and reason about. IMO having the 3 things that set an order all be in this one file makes it easy to see.

Copy link
Contributor

@ryzngard ryzngard Mar 26, 2025

Choose a reason for hiding this comment

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

That's fine with me. It's just something that will wait until someone pushes it into being needed (and outside this file)

};
return codeAction;
}
Expand All @@ -60,46 +63,51 @@ public static RazorVSInternalCodeAction CreateFullyQualifyComponent(string fully
Title = fullyQualifiedName,
Edit = workspaceEdit,
TelemetryId = s_fullyQualifyComponentTelemetryId,
Priority = VSInternalPriorityLevel.High
Priority = VSInternalPriorityLevel.High,
Name = LanguageServerConstants.CodeActions.FullyQualify,
// Fully qualifying an existing component should be very high, but not quite as high as Add Using
Order = -900,
};
return codeAction;
}

public static RazorVSInternalCodeAction CreateComponentFromTag(RazorCodeActionResolutionParams resolutionParams)
{
var title = SR.Create_Component_FromTag_Title;
var data = JsonSerializer.SerializeToElement(resolutionParams);
var codeAction = new RazorVSInternalCodeAction()
{
Title = title,
Title = SR.Create_Component_FromTag_Title,
Data = data,
TelemetryId = s_createComponentFromTagTelemetryId,
Name = LanguageServerConstants.CodeActions.CreateComponentFromTag,
};
return codeAction;
}

public static RazorVSInternalCodeAction CreateExtractToCodeBehind(RazorCodeActionResolutionParams resolutionParams)
{
var title = SR.ExtractTo_CodeBehind_Title;
var data = JsonSerializer.SerializeToElement(resolutionParams);
var codeAction = new RazorVSInternalCodeAction()
{
Title = title,
Title = SR.ExtractTo_CodeBehind_Title,
Data = data,
TelemetryId = s_createExtractToCodeBehindTelemetryId,
Name = LanguageServerConstants.CodeActions.ExtractToCodeBehindAction,
};
return codeAction;
}

public static RazorVSInternalCodeAction CreateExtractToComponent(RazorCodeActionResolutionParams resolutionParams)
{
var title = SR.ExtractTo_Component_Title;
var data = JsonSerializer.SerializeToElement(resolutionParams);
var codeAction = new RazorVSInternalCodeAction()
{
Title = title,
Title = SR.ExtractTo_Component_Title,
Data = data,
TelemetryId = s_createExtractToComponentTelemetryId,
Name = LanguageServerConstants.CodeActions.ExtractToNewComponentAction,
// Since Extract to Component is offered basically everywhere, always offer it last
Copy link
Contributor

@ryzngard ryzngard Mar 26, 2025

Choose a reason for hiding this comment

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

I wonder if we should reduce where it's offered without a selection...

Copy link
Member Author

@davidwengier davidwengier Mar 26, 2025

Choose a reason for hiding this comment

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

I could see that argument, but I do also wonder if this is a transitional point in time. If you haven't seen it, the reddit poster mentions specifically that they see the light bulb in their peripheral vision, and assume it must be the code action they want. This would have been reliable in the past in Razor, but I don't think its something we should try to get back to, or rely on for usability. I would assume that people don't have that same behaviour in C# files, because there is usually something that is offered, and I'd love to think Razor will eventually get to that too.

Order = 9999
};
return codeAction;
}
Expand Down Expand Up @@ -127,7 +135,8 @@ public static RazorVSInternalCodeAction CreateGenerateMethod(VSTextDocumentIdent
{
Title = title,
Data = data,
TelemetryId = s_generateMethodTelemetryId
TelemetryId = s_generateMethodTelemetryId,
Name = LanguageServerConstants.CodeActions.GenerateEventHandler,
};
return codeAction;
}
Expand Down Expand Up @@ -155,7 +164,8 @@ public static RazorVSInternalCodeAction CreateAsyncGenerateMethod(VSTextDocument
{
Title = title,
Data = data,
TelemetryId = s_generateAsyncMethodTelemetryId
TelemetryId = s_generateAsyncMethodTelemetryId,
Name = LanguageServerConstants.CodeActions.GenerateAsyncEventHandler,
};
return codeAction;
}
Expand Down
4 changes: 4 additions & 0 deletions src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Protocol/LanguageServerConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public static class CodeActions
{
public const string GenerateEventHandler = "GenerateEventHandler";

public const string GenerateAsyncEventHandler = "GenerateAsyncEventHandler";

public const string EditBasedCodeActionCommand = "EditBasedCodeActionCommand";

public const string ExtractToCodeBehindAction = "ExtractToCodeBehind";
Expand All @@ -45,6 +47,8 @@ public static class CodeActions

public const string AddUsing = "AddUsing";

public const string FullyQualify = "FullyQualify";

public const string PromoteUsingDirective = "PromoteUsingDirective";

public const string CodeActionFromVSCode = "CodeActionFromVSCode";
Expand Down
Loading