[Property Editor] Add APIs to support refactors from the Property Editor #9202

elliette · 2025-05-21T20:31:40Z

Work towards #8652

This PR:

Adds methods to editor_client and api_classes to call the APIs added in https://dart-review.googlesource.com/c/sdk/+/428784 to support refactors

This does not update the UI to show the available refactors. That will be follow-up.

Note: Fetching the available refactors is currently protected by the propertyEditorRefactors feature flag.

elliette · 2025-05-21T23:08:58Z

packages/devtools_app/test/test_infra/goldens/integration_inspector_errors_2_error_selected.png

Goldens change due to Flutter SDK update

elliette · 2025-05-22T16:48:03Z

packages/devtools_app/lib/src/shared/primitives/custom_pointer_scroll_view.dart

@@ -821,7 +821,7 @@ class _RenderScrollSemantics extends RenderProxyBox {
      if (child.isTagged(RenderViewport.excludeFromScrolling)) {
        excluded.add(child);
      } else {
-        if (!child.hasFlag(SemanticsFlag.isHidden)) {
+        if (!child.flagsCollection.isHidden) {


hasFlag is deprecated in latest Flutter version, see flutter/flutter#166101

kenzieschmoll · 2025-05-22T22:00:03Z

packages/devtools_app/lib/src/shared/editor/api_classes.dart

+    for (final method in LspMethod.values) {
+      if (method.methodName == methodName) return method;
+    }
+    return null;


can this be return LspMethod.values.firstWhereOrNull((method) => method.methodName == methodName)

For a minor perf increase I think you could also add something like:

static nameMap = LspMethod.values.asNameMap();

And then look it up as:

return nameMap[methodName];`

Done! Though we need the method name map instead of the name map, so I've added a static map for method name -> method to use for the look-up.

kenzieschmoll · 2025-05-22T22:02:55Z

packages/devtools_app/lib/src/shared/editor/api_classes.dart

@@ -501,6 +528,105 @@ class EditableArgumentsResult with Serializable {
  };
 }

+/// Constants for [CodeAction] prefixes used to filter the results returned by
+/// an [LspMethod.codeAction] request.
+abstract class CodeActionPrefixes {


can we add unit tests for all of the new logic in this file?

Sure, added editor_client_test (also added test cases for editArgument and editableArguments)

kenzieschmoll · 2025-05-22T22:08:28Z

packages/devtools_app/test/standalone_ui/ide_shared/property_editor/property_editor_test.dart

@@ -99,6 +96,7 @@ void main() {
          mockEditorClient.getEditableArguments(
            textDocument: location.document,
            position: location.position,
+            screenId: 'propertyEditorSidebar',


can we use the analytics constant here instead of the raw string?

DanTup

Added a few comments. Mostly minor stuff, but the deserializing of command arguments I do think we need to remove because that's not public API, it should just be round-tripped whatever it is.

DanTup · 2025-05-23T08:47:33Z

packages/devtools_app/lib/src/shared/editor/api_classes.dart

+    for (final method in LspMethod.values) {
+      if (method.methodName == methodName) return method;
+    }
+    return null;


For a minor perf increase I think you could also add something like:

static nameMap = LspMethod.values.asNameMap();

And then look it up as:

return nameMap[methodName];`

DanTup · 2025-05-23T08:50:21Z

packages/devtools_app/lib/src/shared/editor/api_classes.dart

  static const line = 'line';
+  static const loggedAction = 'loggedAction';


I actually don't think DevTools should have any knowledge of these fields that appear within the command arguments. The exact command/arguments are an implementation detail so you should just be sending whatever the values of command and arguments back to executeCommand without ever really looking at them.

Sounds good, I've removed CodeActionArgument

DanTup · 2025-05-23T08:51:22Z

packages/devtools_app/lib/src/shared/editor/api_classes.dart

+        args: (map[Field.arguments] as List<Object?>? ?? <Object?>[])
+            .cast<Map<String, Object?>>()
+            .map(CodeActionArgument.fromJson)
+            .toList(),


So here, I think we just want args: map[Field.arguments] and leave it as Object?, removing the CodeActionArgument class entirely.

Removed! Thanks

DanTup · 2025-05-23T08:51:58Z

packages/devtools_app/lib/src/shared/editor/api_classes.dart

-class EditArgumentResponse {
-  EditArgumentResponse({required this.success, this.errorMessage, errorCode})
-    : _errorCode = errorCode;
+/// Generic response representing whether a reqeust was a [success].


typo: reqeust

Oops, thanks for catching!

DanTup · 2025-05-23T08:54:10Z

packages/devtools_app/lib/src/shared/editor/editor_client.dart

+  /// Requests that the Analysis Server makes a code edit for an argument.
+  Future<GenericApiResponse> applyRefactor({


The dartdoc is stale here (this isn't editing an argument), but maybe these should also just be called executeCommand since it's more generic than refactors.

Updated dartdoc and renamed to executeCommand

DanTup · 2025-05-23T08:59:37Z

...evtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_controller.dart

+    // in the Property Editor by default.
+    if (FeatureFlags.propertyEditorRefactors) {
+      // Get any supported refactors for the current position.
+      refactorsResult = await editorClient.getRefactors(


I think this await here means we don't show any of the editable fields until we also have the list of actions. I wonder if we should separate these, so that the editable fields would show immediately (perhaps with any previous actions buttons disabled) and then update the actions after. This might improve performance a little for the most likely case (using the editor and not the refactors).

Good point, I've added a TODO for once I tackle displaying the refactor buttons, will decide whether to request the refactors separately then

DanTup · 2025-05-23T09:03:38Z

packages/devtools_app/lib/src/shared/editor/api_classes.dart

+/// via an [LspMethod.executeCommand] request.
+///
+/// For example, "Wrap with Center" or "Wrap with Container".
+class CodeAction with Serializable {


nit: I would call this CodeActionCommand just to avoid some confusion (at least for me), because CodeActions according to LSP can actually be one of two things:

typedef CodeAction = CodeActionLiteral | Command

A CodeActionLiteral is a newer type that supports inline edits, but DTD clients do not support them, and therefore we should always be returning Commands to you here.

In analysis server code, I'm trying to consistently use CodeAction to mean the union of CodeActionLiteral and Command, and then those names explicitly where only one is allowed.

Thank you for that context! I've renamed to CodeActionCommand

DanTup · 2025-05-23T09:05:24Z

...evtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_controller.dart

+
+  List<CodeAction> _extractRefactors(CodeActionResult? result) =>
+      (result?.actions ?? <CodeAction>[])
+          .where((action) => action.title != null && action.command != null)


Have you seen data where any of these were null? In theory, you should only ever be getting Command back from textDocument/codeAction, so both fields should be non-null (except for bugs). If you get a CodeActionLiteral back (you should not, but other editor LSP clients may) then title should still always be non-null (per the LSP spec), although command could be null (and if not, would be a Command rather than a String).

No I haven't! I added this check just to be safe, though I could also make title and command non-nullable if we expect them to always be returned. WDYT?

elliette added 3 commits May 21, 2025 12:27

Add APIs to support refactors from the Property Editor

0100376

Protect behind feature flag

2c88aa8

Update Flutter SDK to get refactors support

6d1667b

elliette requested a review from a team as a code owner May 21, 2025 20:31

elliette requested review from bkonyi and removed request for a team May 21, 2025 20:31

elliette added the release-notes-not-required label May 21, 2025

elliette added 3 commits May 21, 2025 15:27

Fix analyzer errors

8740d15

Fix property editor test

13d9754

Update goldens

36f6e60

elliette requested review from kenzieschmoll and removed request for bkonyi May 21, 2025 23:08

elliette commented May 21, 2025

View reviewed changes

Fix deprecated members use

ee0fe55

elliette commented May 22, 2025

View reviewed changes

kenzieschmoll requested a review from DanTup May 22, 2025 22:09

kenzieschmoll reviewed May 22, 2025

View reviewed changes

DanTup reviewed May 23, 2025

View reviewed changes

elliette added 4 commits May 23, 2025 11:18

Respond to first of PR comments

abe353c

Update test

a77c314

Add new test file

64e19bd

Format

d891ad8

		static const line = 'line';
		static const loggedAction = 'loggedAction';

		/// Requests that the Analysis Server makes a code edit for an argument.
		Future<GenericApiResponse> applyRefactor({

[Property Editor] Add APIs to support refactors from the Property Editor #9202

Are you sure you want to change the base?

[Property Editor] Add APIs to support refactors from the Property Editor #9202

Uh oh!

Conversation

elliette commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanTup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!