Notebook CodeActionKind Support (#192248)

* shift nb codeaction filter to editor layer

* readability

* revised API, clearer typings, filter out autosave triggers

* quick lil rename bc i kept confusing myself

* revert back to boolean setting. wait for editor enum

src/vs/editor/contrib/codeAction/browser/codeAction.ts

@@ -3,6 +3,7 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
+import * as nls from 'vs/nls';
 import { coalesce, equals, isNonEmptyArray } from 'vs/base/common/arrays';
 import { CancellationToken } from 'vs/base/common/cancellation';
 import { illegalArgument, isCancellationError, onUnexpectedExternalError } from 'vs/base/common/errors';
@@ -18,7 +19,6 @@ import { ITextModel } from 'vs/editor/common/model';
 import { ILanguageFeaturesService } from 'vs/editor/common/services/languageFeatures';
 import { IModelService } from 'vs/editor/common/services/model';
 import { TextModelCancellationTokenSource } from 'vs/editor/contrib/editorState/browser/editorState';
-import * as nls from 'vs/nls';
 import { CommandsRegistry, ICommandService } from 'vs/platform/commands/common/commands';
 import { ServicesAccessor } from 'vs/platform/instantiation/common/instantiation';
 import { INotificationService } from 'vs/platform/notification/common/notification';
@@ -89,14 +89,20 @@ export async function getCodeActions(
 	token: CancellationToken,
 ): Promise<CodeActionSet> {
 	const filter = trigger.filter || {};
+	const notebookFilter: CodeActionFilter = {
+		...filter,
+		excludes: [...(filter.excludes || []), CodeActionKind.Notebook],
+	};
 
 	const codeActionContext: languages.CodeActionContext = {
 		only: filter.include?.value,
 		trigger: trigger.type,
 	};
 
 	const cts = new TextModelCancellationTokenSource(model, token);
-	const providers = getCodeActionProviders(registry, model, filter);
+	// if the trigger is auto (autosave, lightbulb, etc), we should exclude notebook codeActions
+	const excludeNotebookCodeActions = (trigger.type === languages.CodeActionTriggerType.Auto);
+	const providers = getCodeActionProviders(registry, model, (excludeNotebookCodeActions) ? notebookFilter : filter);
 
 	const disposables = new DisposableStore();
 	const promises = providers.map(async provider => {

src/vs/editor/contrib/codeAction/common/types.ts

@@ -20,6 +20,7 @@ export class CodeActionKind {
 	public static readonly RefactorInline = CodeActionKind.Refactor.append('inline');
 	public static readonly RefactorMove = CodeActionKind.Refactor.append('move');
 	public static readonly RefactorRewrite = CodeActionKind.Refactor.append('rewrite');
+	public static readonly Notebook = new CodeActionKind('notebook');
 	public static readonly Source = new CodeActionKind('source');
 	public static readonly SourceOrganizeImports = CodeActionKind.Source.append('organizeImports');
 	public static readonly SourceFixAll = CodeActionKind.Source.append('fixAll');

src/vs/workbench/api/common/extHost.api.impl.ts

@@ -1585,7 +1585,6 @@ export function createApiFactoryAndRegisterActors(accessor: ServicesAccessor): I
 			InteractiveEditorResponseFeedbackKind: extHostTypes.InteractiveEditorResponseFeedbackKind,
 			StackFrameFocus: extHostTypes.StackFrameFocus,
 			ThreadFocus: extHostTypes.ThreadFocus,
-			NotebookCodeActionKind: extHostTypes.NotebookCodeActionKind,
 			RelatedInformationType: extHostTypes.RelatedInformationType
 		};
 	};

src/vs/workbench/api/common/extHostLanguageFeatures.ts

@@ -7,7 +7,7 @@ import { URI, UriComponents } from 'vs/base/common/uri';
 import { equals, mixin } from 'vs/base/common/objects';
 import type * as vscode from 'vscode';
 import * as typeConvert from 'vs/workbench/api/common/extHostTypeConverters';
-import { Range, Disposable, CompletionList, SnippetString, CodeActionKind, SymbolInformation, DocumentSymbol, SemanticTokensEdits, SemanticTokens, SemanticTokensEdit, Location, InlineCompletionTriggerKind, InternalDataTransferItem, CodeActionTriggerKind, SyntaxTokenType } from 'vs/workbench/api/common/extHostTypes';
+import { Range, Disposable, CompletionList, SnippetString, CodeActionKind, SymbolInformation, DocumentSymbol, SemanticTokensEdits, SemanticTokens, SemanticTokensEdit, Location, InlineCompletionTriggerKind, InternalDataTransferItem, SyntaxTokenType } from 'vs/workbench/api/common/extHostTypes';
 import { ISingleEditOperation } from 'vs/editor/common/core/editOperation';
 import * as languages from 'vs/editor/common/languages';
 import { ExtHostDocuments } from 'vs/workbench/api/common/extHostDocuments';
@@ -377,7 +377,6 @@ class CodeActionAdapter {
 
 	private readonly _cache = new Cache<vscode.CodeAction | vscode.Command>('CodeAction');
 	private readonly _disposables = new Map<number, DisposableStore>();
-	private readonly nbKind = new CodeActionKind('notebook');
 
 	constructor(
 		private readonly _documents: ExtHostDocuments,
@@ -437,10 +436,6 @@ class CodeActionAdapter {
 					command: this._commands.toInternal(candidate, disposables),
 				});
 			} else {
-				if (codeActionContext.triggerKind !== CodeActionTriggerKind.Invoke && candidate.kind && this.nbKind.contains(candidate.kind)) {
-					continue;
-				}
-
 				if (codeActionContext.only) {
 					if (!candidate.kind) {
 						this._logService.warn(`${this._extension.identifier.value} - Code actions of kind '${codeActionContext.only.value} 'requested but returned code action does not have a 'kind'. Code action will be dropped. Please set 'CodeAction.kind'.`);

src/vs/workbench/api/common/extHostTypes.ts

@@ -1397,16 +1397,6 @@ export class CodeActionKind {
 	}
 }
 
-export class NotebookCodeActionKind extends CodeActionKind {
-	public static override Notebook: CodeActionKind;
-
-	constructor(
-		public override readonly value: string
-	) {
-		super(value);
-	}
-}
-
 CodeActionKind.Empty = new CodeActionKind('');
 CodeActionKind.QuickFix = CodeActionKind.Empty.append('quickfix');
 CodeActionKind.Refactor = CodeActionKind.Empty.append('refactor');

src/vs/workbench/contrib/notebook/browser/contrib/saveParticipants/saveParticipants.ts

@@ -38,8 +38,6 @@ import { LifecyclePhase } from 'vs/workbench/services/lifecycle/common/lifecycle
 import { IStoredFileWorkingCopy, IStoredFileWorkingCopyModel } from 'vs/workbench/services/workingCopy/common/storedFileWorkingCopy';
 import { IStoredFileWorkingCopySaveParticipant, IWorkingCopyFileService } from 'vs/workbench/services/workingCopy/common/workingCopyFileService';
 
-const NotebookCodeAction = new CodeActionKind('notebook');
-
 class FormatOnSaveParticipant implements IStoredFileWorkingCopySaveParticipant {
 	constructor(
 		@IEditorWorkerService private readonly editorWorkerService: IEditorWorkerService,
@@ -306,6 +304,7 @@ class CodeActionOnSaveParticipant implements IStoredFileWorkingCopySaveParticipa
 	}
 
 	async participate(workingCopy: IStoredFileWorkingCopy<IStoredFileWorkingCopyModel>, context: { reason: SaveReason }, progress: IProgress<IProgressStep>, token: CancellationToken): Promise<void> {
+		const nbDisposable = new DisposableStore();
 		const isTrusted = this.workspaceTrustManagementService.isWorkspaceTrusted();
 		if (!isTrusted) {
 			return;
@@ -315,31 +314,46 @@ class CodeActionOnSaveParticipant implements IStoredFileWorkingCopySaveParticipa
 			return;
 		}
 
+		let saveTrigger = '';
 		if (context.reason === SaveReason.AUTO) {
+			// currently this won't happen, as vs/editor/contrib/codeAction/browser/codeAction.ts L#104 filters out codeactions on autosave. Just future-proofing
+			// ? notebook CodeActions on autosave seems dangerous (perf-wise)
+			saveTrigger = 'always';
+		} else if (context.reason === SaveReason.EXPLICIT) {
+			saveTrigger = 'explicit';
+		} else {
+			// 	SaveReason.FOCUS_CHANGE, WINDOW_CHANGE need to be addressed when autosaves are enabled
 			return undefined;
 		}
 
-		const setting = this.configurationService.getValue<{ [kind: string]: boolean } | string[]>(NotebookSetting.codeActionsOnSave);
+		const notebookModel = workingCopy.model.notebookModel;
+
+		const setting = this.configurationService.getValue<{ [kind: string]: string }>(NotebookSetting.codeActionsOnSave);
 		if (!setting) {
 			return undefined;
 		}
-
-		const notebookModel = workingCopy.model.notebookModel;
-
 		const settingItems: string[] = Array.isArray(setting)
 			? setting
 			: Object.keys(setting).filter(x => setting[x]);
-
 		if (!settingItems.length) {
 			return undefined;
 		}
 
-		const codeActionsOnSave = this.createCodeActionsOnSave(settingItems).filter(x => !NotebookCodeAction.contains(x));
-		const notebookCodeActionsOnSave = this.createCodeActionsOnSave(settingItems).filter(x => NotebookCodeAction.contains(x));
+		const allCodeActions = this.createCodeActionsOnSave(settingItems);
+		const excludedActions = allCodeActions
+			.filter(x => setting[x.value] === 'never');
+		const includedActions = allCodeActions
+			.filter(x => setting[x.value] === saveTrigger);
+
+		const editorCodeActionsOnSave = includedActions.filter(x => !CodeActionKind.Notebook.contains(x));
+		const notebookCodeActionsOnSave = includedActions.filter(x => CodeActionKind.Notebook.contains(x));
+		if (!editorCodeActionsOnSave.length && !notebookCodeActionsOnSave.length) {
+			return undefined;
+		}
 
 		// prioritize `source.fixAll` code actions
 		if (!Array.isArray(setting)) {
-			codeActionsOnSave.sort((a, b) => {
+			editorCodeActionsOnSave.sort((a, b) => {
 				if (CodeActionKind.SourceFixAll.contains(a)) {
 					if (CodeActionKind.SourceFixAll.contains(b)) {
 						return 0;
@@ -353,21 +367,6 @@ class CodeActionOnSaveParticipant implements IStoredFileWorkingCopySaveParticipa
 			});
 		}
 
-
-
-
-		if (!codeActionsOnSave.length && !notebookCodeActionsOnSave.length) {
-			return undefined;
-		}
-
-		const excludedActions = Array.isArray(setting)
-			? []
-			: Object.keys(setting)
-				.filter(x => setting[x] === false)
-				.map(x => new CodeActionKind(x));
-
-		const nbDisposable = new DisposableStore();
-
 		// run notebook code actions
 		progress.report({ message: localize('notebookSaveParticipants.notebookCodeActions', "Running 'Notebook' code actions") });
 		try {
@@ -387,15 +386,15 @@ class CodeActionOnSaveParticipant implements IStoredFileWorkingCopySaveParticipa
 
 		// run cell level code actions
 		const disposable = new DisposableStore();
-		progress.report({ message: localize('notebookSaveParticipants.cellCodeActions', "Running code actions") });
+		progress.report({ message: localize('notebookSaveParticipants.cellCodeActions', "Running 'Cell' code actions") });
 		try {
 			await Promise.all(notebookModel.cells.map(async cell => {
 				const ref = await this.textModelService.createModelReference(cell.uri);
 				disposable.add(ref);
 
 				const textEditorModel = ref.object.textEditorModel;
 
-				await this.applyOnSaveActions(textEditorModel, codeActionsOnSave, excludedActions, progress, token);
+				await this.applyOnSaveActions(textEditorModel, editorCodeActionsOnSave, excludedActions, progress, token);
 			}));
 		} catch {
 			this.logService.error('Failed to apply code action on save');
@@ -447,7 +446,7 @@ class CodeActionOnSaveParticipant implements IStoredFileWorkingCopySaveParticipa
 				for (const action of actionsToRun.validActions) {
 					const codeActionEdits = action.action.edit?.edits;
 					let breakFlag = false;
-					if (!action.action.kind?.includes('notebook')) {
+					if (!action.action.kind?.startsWith('notebook')) {
 						for (const edit of codeActionEdits ?? []) {
 							const workspaceTextEdit = edit as IWorkspaceTextEdit;
 							if (workspaceTextEdit.resource && isEqual(workspaceTextEdit.resource, model.uri)) {

src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts

@@ -964,10 +964,14 @@ configurationRegistry.registerConfiguration({
 			default: false
 		},
 		[NotebookSetting.codeActionsOnSave]: {
-			markdownDescription: nls.localize('notebook.codeActionsOnSave', "Experimental. Run a series of CodeActions for a notebook on save. CodeActions must be specified, the file must not be saved after delay, and the editor must not be shutting down. Example: `source.fixAll: true`"),
+			markdownDescription: nls.localize('notebook.codeActionsOnSave', "Run a series of CodeActions for a notebook on save. CodeActions must be specified, the file must not be saved after delay, and the editor must not be shutting down. Example: `source.fixAll: true`"),
 			type: 'object',
 			additionalProperties: {
-				type: 'boolean'
+				type: 'string',
+				enum: ['explicit', 'never'],
+				// enum: ['explicit', 'always', 'never'], -- autosave support needs to be built first
+				// nls.localize('always', 'Always triggers Code Actions on save, including autosave, focus, and window change events.'),
+				enumDescriptions: [nls.localize('never', 'Never triggers Code Actions on save.'), nls.localize('explicit', 'Triggers Code Actions only when explicitly saved.')],
 			},
 			default: {}
 		},