Fix forEach(async ...) causing fire-and-forget promises in notebook code

[pefia]

What I Found

I was auditing the notebook codebase for async anti-patterns and I found two places where Array.forEach(async ...) is used to drive async operations. This is a classic JavaScript footgun — forEach completely ignores the return value of its callback, so when you pass it an async function, the promises it creates just float away into the void. The calling code carries on as if everything is done, but nothing has actually happened yet.

---

Bug 1: Notebook multicursor undo/redo runs all operations concurrently and resolves instantly

I found this in notebookMulticursor.ts inside updateFinalUndoRedo(). The undo and redo handlers that get pushed onto the undo/redo stack looked like this:

undo: async () => {
    newElementsMap.forEach(async value => {
        value.reverse().forEach(async element => {
            await element.undo();
        });
    });
},

There are actually two nested forEach(async ...) calls here, which makes the problem even worse. What happens at runtime is:

1. The outer forEach fires off one un-awaited promise per resource in the map
2. Each of those immediately fires off more un-awaited promises per undo element
3. The undo() async function returns its promise right away — before a single element.undo() has completed
4. The undo/redo service thinks the operation is finished and moves on

I noticed that value.reverse() is called in the undo path, which tells me the author intended for undo operations to run in reverse order. But with forEach(async ...), all the elements fire concurrently — the reverse() is meaningless. Redo has the same problem, just without the reverse.

On top of all that, if any element.undo() or element.redo() throws, the error becomes an unhandled promise rejection — it never reaches the undo/redo service.

What I did: I replaced both nested forEach(async ...) with for...of loops. Now the outer loop iterates resources sequentially with for (const value of newElementsMap.values()), and the inner loop awaits each element in order with for (const element of value.reverse()). The async function only resolves when all the work is truly done, and errors propagate correctly.

---

Bug 2: Notebook "Toggle Line Comment" on multiple cells returns before doing anything

I found this in editActions.ts in the CommentSelectedCellsAction class. The runWithContext method is async and returns Promise<void>, but internally it does:

context.selectedCells.forEach(async cellViewModel => {
    const textModel = await cellViewModel.resolveTextModel();
    // ... build comment command, execute it, restore selections ...
});

So runWithContext returns immediately after kicking off the forEach — it doesn't wait for any cell to be processed. Each cell's resolveTextModel() is async, so the actual comment toggling and selection restoration happen at some indeterminate time after the command "completes."

In practice this probably works okay when there's only one cell selected (the async work happens quickly enough), but with multiple selected cells, they all race each other. If any cell's resolveTextModel() is slow or fails, that's silently swallowed.

What I did: I replaced forEach(async ...) with a for...of loop so each cell is processed sequentially: resolve the text model, toggle the comment, restore selections, then move to the next cell. The runWithContext promise now correctly represents when all the work is actually done.

---

How I validated

• TypeScript compilation passes cleanly (npm run compile-check-ts-native — zero errors)
• No IDE diagnostics on either changed file
• Pre-commit hygiene check passes
• The changes are purely mechanical — replacing forEach(async => ...) with for...of await — so the logic inside each iteration is completely untouched

Files changed

• src/vs/workbench/contrib/notebook/browser/contrib/multicursor/notebookMulticursor.ts — undo/redo handlers
• src/vs/workbench/contrib/notebook/browser/controller/editActions.ts — CommentSelectedCellsAction.runWithContext

[Copilot]

Pull request overview

Fixes async “fire-and-forget” behavior in notebook actions by replacing forEach(async ...) loops (which don’t await) with properly awaited for...of loops, ensuring operations complete (and errors propagate) before the command/undo-redo completes.

Changes:

• Make “Toggle Line Comment” across multiple selected cells await each cell’s async text model resolution and comment operation.
• Make notebook multicursor undo/redo await each undo/redo element in sequence instead of spawning untracked promises.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/vs/workbench/contrib/notebook/browser/controller/editActions.ts	Replaces selectedCells.forEach(async ...) with a for...of loop so the action’s promise resolves only after all cells are processed.
src/vs/workbench/contrib/notebook/browser/contrib/multicursor/notebookMulticursor.ts	Replaces nested forEach(async ...) in multicursor undo/redo with awaited for...of loops to enforce correct completion and error propagation.

[pefia]

@microsoft-github-policy-service agree