Move Code Action Constraints

[justschen]

Acknowledgement

• I acknowledge that issues using this template may be closed without further explanation at the maintainer's discretion.

Comment

related #56416 and https://github.com/microsoft/TypeScript/pull/57080/files

We had some discussion within the VS Code team regarding the changes @aiday-mar made and how we wanted to make the constraints tighter.

the current sentiment after discussion:

• when moving or clicking through code (ie, not selecting a range of code), users likely will not want to see the Move to... code actions. however, it should be available to them if they manually select (via Refactor or CmdCtrl + .)
  • this fixes the annoyance of having the lightbulb widget shown virtually everywhere in the file when typing or moving trough code.
• however, if the cursor is at the top scoping level or on its identifier, we can show the Move to... actions.

Reasoning behind not having the lightbulb be so aggressive in suggesting code actions is that the lightbulb loses value (ie, blue lightbulb means there are fixes, but yellow lightbulb is super general). Even if users don't explicitly hit the lightbulb button, when they see certain changes in the lightbulb, they might hit CmdCtrl + . to trigger the code actions.

when actually selecting a range of code, I believe @aiday-mar had some proposed fixes for the constraints as well, similarly looking at scoping.

In our latest stable release (1.89.0), i added a good amount of telemetry tracking a few things that might help us make a more informed and concrete decision if we want to move forward with this:

• when the lightbulb is clicked -> which code actions and providers are shown in that code action menu
• when the code action menu is exited (and the source of the menu was the lightbulb) -> if a code action was selected or if it was just quit out.
• when code actions are applied -> we can see how often move to... is being used when the source is the lightbulb widget.
• in insiders only: how often the lightbulb widget shows up when there is only a single code action in that list (ie, only move to file)

currently have a draft PR for our built in extension, but as Matt mentioned, likely would be best to fix here so the behavior is consistent across all editors.

I'll likely take a look at making a PR here, so any help would be appreciated!

Let me know if there are any thoughts on this, if we think it's too drastic, if we want to see more telemetry, etc.

cc. @mjbvz @navya9singh @andrewbranch @aiday-mar

[DanielRosenwasser]

Conceptually some of this sounds right, but it'd help to have an idea of what is still annoying currently since #57080 was merged, and what this issue's proposing. For example:

however, if the cursor is at the top scoping level

This one I'd like to have a much better intuition around.

[justschen]

The main feedback is around the lightbulb widget (which could indicate that at it's core, it could be a VS Code issue that should be fixed on our side) showing up virtually everywhere because the Move to code actions contributed by TS are shown virtually everywhere!

As you know, VS Code automatically requests code actions at each cursor move, typing, etc, and it just happens that Move to shows up because it is a valid action in most code blocks.

Ideally, there are constraints around aggressively showing and having code actions everywhere, especially after telemetry suggests it's not being used in the automatically, and only when explicitly clicking refactor or CtrlCmd + .

[aiday-mar]

Hi @justschen, @DanielRosenwasser thank you for opening this issue. We currently believe the action is returned too often even after the changes, and after discussing with the team, we believe a good condition for showing the action would be the following.

Make the action appear when both ends of the primary selection are on a top-level scoping symbol such as interface, class, module, function or on its identifier, or completely enclosing a top-level scope.

[justschen]

Notes from TS editor sync:

• Telemetry shows a high rate of cancellation, not that many people are clicking on lightbulb -> move to....
• Move to New File will likely be removed entirely at this point. Will test this out and see if there is user comment on it.
• Move To File constraints are already good, uses "best case" to capture whether we show or don't show. works fine now for selections and non-selections, but want to be able to determine invoked vs implicit and how they actually work.

TLDR: do not show if automatically triggered (then lightbulb will also not show), only if requested from Refactor or Ctrl + . add that to current selection logic for move to file will not change, can be applied to move to new file until it is removed.

[DanielRosenwasser]

Looks like #58548 fixes this, but the GitHub "closes" pattern didn't kick in because of the ##.

Feel free to reopen, but otherwise thanks @justschen!