Refresh the cells below a run, and notice runs the VS Code webview never renders

docs/architecture/front-ends.md

@@ -41,10 +41,10 @@ The refresh policy is set by the expensive kernels, where a request is a full co
 
 - **Edit**: debounced, 500 ms after the last change the user makes. Text set from .NET (a reload, a tool editing cells) does not ask unless that editor has focus; the next focus asks instead.
 - **Focus**: when a cell's editor gains focus, since variable-dependent kernels (SQL, HTTP, HTML) change their answer when another cell runs, not when this text changes. Skipped when the text is the version last answered and nothing has happened since that could change the answer (a run ending, a restart, a language change).
-- **Run**: after the cell's own run finishes.
+- **Run**: after a cell's run finishes, for that cell and for every cell below it that is showing kernel markers, since the run may have changed what their code refers to and those are the markers it can visibly fix. They are asked top to bottom and one at a time, so the cells nearest the run update first and a run started in the meantime waits behind at most one request; a cell whose answer still holds is skipped. The other cells below, and the cells above, are asked on their next focus. A cell counts as run when it stops executing or when its execution count moves; in the VS Code webview a quick cell is never rendered as executing, because its executing and executed notifications are both handled before the next render.
 - **Restart**: markers are cleared in every cell, whichever kernel restarted, not recomputed.
 
-While any cell is running no request is issued (`NotebookCellContext.IsAnyExecuting`, passed to the editor as `DiagnosticsSuspended`). `Verso.Host` answers requests one at a time, so a request during a run would wait for the run, and some kernels answer empty while executing. A refresh asked for in that time runs once the run is over. One request is in flight per editor; a reply for text that has since changed is dropped; a failed request (null) keeps the markers that are showing, while an empty list clears them. Cells over 2,000 lines are not analyzed.
+While any cell is running no request is issued (`NotebookCellContext.IsAnyExecuting`, passed to the editor as `DiagnosticsSuspended`). `Verso.Host` answers requests one at a time, so a request during a run would wait for the run, and some kernels answer empty while executing. A refresh asked for in that time runs once the run is over, in the same top-to-bottom order. Run All is one `execution/runAll` request, so in VS Code the cells are refreshed once it has finished, not one by one as it goes. One request is in flight per editor; a reply for text that has since changed is dropped; a failed request (null) keeps the markers that are showing, while an empty list clears them. Cells over 2,000 lines are not analyzed.
 
 Monaco's bundled TypeScript worker would otherwise mark JavaScript and TypeScript cells as if each were a standalone file, so its validation is turned off when Monaco loads; its completion and hover stay on.
 

docs/extensions/extension-interfaces.md

@@ -61,7 +61,7 @@ Executes code, provides completions, diagnostics, and hover information for a sp
 
 ### Diagnostics in the Editor
 
-What `GetDiagnosticsAsync` returns is drawn in the cell editor as squiggles. The editor asks after the user pauses typing (about half a second), when the cell's editor gains focus, and after the cell runs. It does not ask while any cell in the notebook is running, and it clears every marker in the notebook when any kernel restarts. Diagnostics are advisory: if the kernel fails to initialize or `GetDiagnosticsAsync` throws, the request goes unanswered and the editor keeps the markers it already shows (none, for a kernel that never started); the failure is reported where it matters, when the cell runs. Return an empty list, not an exception, when there is nothing to report, so fixed problems are cleared.
+What `GetDiagnosticsAsync` returns is drawn in the cell editor as squiggles. The editor asks after the user pauses typing (about half a second), when the cell's editor gains focus, and after a run, for the cell that ran and every cell below it that is showing markers, nearest first. It does not ask while any cell in the notebook is running, and it clears every marker in the notebook when any kernel restarts. Diagnostics are advisory: if the kernel fails to initialize or `GetDiagnosticsAsync` throws, the request goes unanswered and the editor keeps the markers it already shows (none, for a kernel that never started); the failure is reported where it matters, when the cell runs. Return an empty list, not an exception, when there is nothing to report, so fixed problems are cleared.
 
 Leading `#!` magic command lines are blanked before the code reaches the kernel, keeping the line count, so a kernel never has to recognize them and its positions still match the cell. At run time the pipeline removes those lines instead, so a kernel that reads a directive from the cell's first line (as SQL reads `--connection`) should skip leading blank lines when it looks for that line during diagnostics.
 

src/Verso.Blazor.Shared/Components/Editor/MonacoEditor.razor

@@ -91,6 +91,21 @@
         catch (JSDisconnectedException) { }
     }
 
+    /// <summary>
+    /// The cell has finished running: ask again for this editor's diagnostics and for every
+    /// editor below it that is showing kernel markers, nearest first, since the run may have
+    /// changed what their code refers to.
+    /// </summary>
+    public async Task RefreshAfterRunAsync()
+    {
+        if (!_initialized) return;
+        try
+        {
+            await JS.InvokeVoidAsync("versoMonaco.refreshAfterRun", _elementId);
+        }
+        catch (JSDisconnectedException) { }
+    }
+
     /// <summary>Remove every kernel diagnostic marker from the editor.</summary>
     public async Task ClearDiagnosticsAsync()
     {

src/Verso.Blazor.Shared/Components/Notebook/Cell.razor

@@ -470,11 +470,16 @@
         // boundary is holding no error, which is the ordinary case. The first execution this
         // component sees is not treated as a change: there is nothing yet to recover.
         var execution = (CellData.Id, CellData.ExecutionCount);
-        if (_lastExecution is { } previous && previous != execution)
+        var ranAgain = _lastExecution is { } previous && previous != execution;
+        if (ranAgain)
             _outputErrorBoundary?.Recover();
         _lastExecution = execution;
 
-        if (_wasExecuting && !IsExecuting)
+        // The run has ended when the cell stops executing, or when its execution count moves
+        // without this component having seen it execute: in the VS Code webview the executing
+        // and executed notifications for a quick cell are handled before the next render, so
+        // IsExecuting is never seen true. A count cleared by a restart is not a run.
+        if ((_wasExecuting && !IsExecuting) || (ranAgain && execution.ExecutionCount is not null && !IsExecuting))
             _pendingDiagnosticsRefresh = true;
         _wasExecuting = IsExecuting;
 
@@ -599,7 +604,7 @@
         {
             _pendingDiagnosticsRefresh = false;
             if (_editor is not null)
-                await _editor.RefreshDiagnosticsAsync();
+                await _editor.RefreshAfterRunAsync();
         }
 
         await ReconcileMenuPopoverAsync();

src/Verso.Blazor.Shared/wwwroot/js/monaco-interop.js

@@ -240,16 +240,17 @@ window.versoMonaco = (function () {
 
     // onFocus is true for a refresh triggered by the editor gaining focus, which is skipped when
     // the last answer still holds. Edits, runs and explicit requests always go through.
+    // Returns a promise that settles when the request it sends does, or at once when it sends none.
     function refreshDiagnostics(elementId, onFocus) {
         const state = diagnosticsState[elementId];
         const editor = editors[elementId];
-        if (!state || !editor) return;
+        if (!state || !editor) return Promise.resolve();
 
         if (onFocus && !state.pending && !state.timer) {
             const current = editor.getModel();
             if (current && current.getVersionId() === state.answeredVersion
                 && state.answeredEpoch === state.staleEpoch) {
-                return;
+                return Promise.resolve();
             }
         }
 
@@ -262,28 +263,28 @@ window.versoMonaco = (function () {
         // against state it is changing. Ask once the run is over instead.
         if (state.suspended || state.inFlight) {
             state.pending = true;
-            return;
+            return Promise.resolve();
         }
         state.pending = false;
 
         const model = editor.getModel();
-        if (!model) return;
+        if (!model) return Promise.resolve();
         const ref = dotnetRefs[model.uri.toString()];
-        if (!ref) return;
+        if (!ref) return Promise.resolve();
 
         // A pathological pasted cell is not analyzed at all, and what it showed before no
         // longer applies.
         if (model.getLineCount() > DIAGNOSTICS_MAX_LINES) {
             setKernelMarkers(model, []);
-            return;
+            return Promise.resolve();
         }
 
         const version = model.getVersionId();
         const generation = state.generation;
         const epoch = state.staleEpoch;
         state.inFlight = true;
 
-        ref.invokeMethodAsync('GetDiagnostics', model.getValue())
+        return ref.invokeMethodAsync('GetDiagnostics', model.getValue())
             .then(function (result) {
                 if (diagnosticsState[elementId] !== state || state.generation !== generation) return;
                 if (model.isDisposed() || model.getVersionId() !== version) return;
@@ -302,6 +303,67 @@ window.versoMonaco = (function () {
             });
     }
 
+    // ── Refresh after a run ─────────────────────────────────────────────
+    // A run changes what the cells after it can refer to, so when a cell's run ends it is asked
+    // again, and so is every cell below it that is showing kernel markers, since those are the
+    // ones a run can visibly fix. For C# and F# each request is a full compilation, so the other
+    // cells below are left to their next focus, as are the cells above. The cells go top to
+    // bottom and one at a time: the cells nearest the run update first, and a run started in the
+    // meantime waits behind at most one request. Nothing goes while a cell runs; the walk resumes
+    // when the run is over. A cell whose answer still holds for its text and kernel state is
+    // skipped.
+    const refreshAfterRunQueue = new Set(); // elementIds still to ask
+    let refreshAfterRunActive = false;
+
+    function editorsInDocumentOrder() {
+        return Object.keys(diagnosticsState)
+            .filter(function (id) { return editors[id]; })
+            .sort(function (a, b) {
+                const position = editors[a].getContainerDomNode()
+                    .compareDocumentPosition(editors[b].getContainerDomNode());
+                return position & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1;
+            });
+    }
+
+    function showsKernelMarkers(elementId) {
+        const model = editors[elementId] && editors[elementId].getModel();
+        return !!model && !model.isDisposed()
+            && monaco.editor.getModelMarkers({ owner: DIAGNOSTICS_OWNER, resource: model.uri }).length > 0;
+    }
+
+    function queueRefreshAfterRun(elementId) {
+        refreshAfterRunQueue.add(elementId);
+        const ordered = editorsInDocumentOrder();
+        const start = ordered.indexOf(elementId);
+        if (start >= 0) {
+            ordered.slice(start + 1).filter(showsKernelMarkers).forEach(function (id) {
+                refreshAfterRunQueue.add(id);
+            });
+        }
+        runRefreshAfterRunQueue();
+    }
+
+    function runRefreshAfterRunQueue() {
+        if (refreshAfterRunActive || refreshAfterRunQueue.size === 0) return;
+        refreshAfterRunActive = true;
+
+        function step() {
+            const next = editorsInDocumentOrder().find(function (id) { return refreshAfterRunQueue.has(id); });
+            const state = next && diagnosticsState[next];
+            if (!state || state.suspended) {
+                // Done, or held until the run that is starting ends (setDiagnosticsSuspended).
+                refreshAfterRunActive = false;
+                if (!next) refreshAfterRunQueue.clear();
+                return;
+            }
+            refreshAfterRunQueue.delete(next);
+            refreshDiagnostics(next, true).then(step, step);
+        }
+
+        // On the next turn, so every editor a render lifts from suspension is lifted first.
+        setTimeout(step, 0);
+    }
+
     function scheduleDiagnostics(elementId) {
         const state = diagnosticsState[elementId];
         if (!state) return;
@@ -696,6 +758,7 @@ window.versoMonaco = (function () {
         dispose: function (elementId) {
             clearDiagnostics(elementId);
             delete diagnosticsState[elementId];
+            refreshAfterRunQueue.delete(elementId);
             const editor = editors[elementId];
             if (editor) {
                 const modelUri = editor.getModel()?.uri?.toString();
@@ -863,11 +926,17 @@ window.versoMonaco = (function () {
             }
         },
 
-        // Ask the kernel for this editor's diagnostics now (after its cell ran, for example).
+        // Ask the kernel for this editor's diagnostics now.
         refreshDiagnostics: function (elementId) {
             refreshDiagnostics(elementId);
         },
 
+        // This editor's cell has finished running: ask again for it and every cell below it
+        // that is showing kernel markers.
+        refreshAfterRun: function (elementId) {
+            queueRefreshAfterRun(elementId);
+        },
+
         clearDiagnostics: function (elementId) {
             clearDiagnostics(elementId);
         },
@@ -881,8 +950,10 @@ window.versoMonaco = (function () {
             if (state.suspended && !suspended) markDiagnosticsStale(state);
             state.suspended = !!suspended;
             if (!state.suspended && state.pending) {
-                refreshDiagnostics(elementId);
+                // Joins the walk after a run, so the cells a run held back are asked in order.
+                refreshAfterRunQueue.add(elementId);
             }
+            if (!state.suspended) runRefreshAfterRunQueue();
         },
 
         setTheme: function (theme) {

tests/Verso.Blazor.Shared.Tests/CellTests.cs

@@ -522,11 +522,39 @@ public void Diagnostics_SuspendedWhileNotebookExecutes()
     public void Diagnostics_RefreshedWhenOwnRunCompletes()
     {
         var cut = RenderCell(CreateCodeCell("var x = 1;"), isExecuting: true, editorCreated: true);
-        Assert.AreEqual(0, RefreshDiagnosticsCallCount());
+        Assert.AreEqual(0, RefreshAfterRunCallCount());
 
         cut.SetParametersAndRender(p => p.Add(c => c.IsExecuting, false));
 
-        Assert.AreEqual(1, RefreshDiagnosticsCallCount());
+        Assert.AreEqual(1, RefreshAfterRunCallCount());
+    }
+
+    [TestMethod]
+    public void Diagnostics_RefreshedWhenExecutionCountMovesUnseen()
+    {
+        // In the VS Code webview a quick cell's executing and executed notifications are both
+        // handled before the next render, so the cell is never rendered as executing.
+        var cell = CreateCodeCell("var x = 1;");
+        var cut = RenderCell(cell, editorCreated: true);
+        Assert.AreEqual(0, RefreshAfterRunCallCount());
+
+        cell.ExecutionCount = 1;
+        cut.SetParametersAndRender(p => p.Add(c => c.CellData, cell));
+
+        Assert.AreEqual(1, RefreshAfterRunCallCount());
+    }
+
+    [TestMethod]
+    public void Diagnostics_NotRefreshedWhenARestartClearsTheCount()
+    {
+        var cell = CreateCodeCell("var x = 1;");
+        cell.ExecutionCount = 3;
+        var cut = RenderCell(cell, editorCreated: true);
+
+        cell.ExecutionCount = null;
+        cut.SetParametersAndRender(p => p.Add(c => c.CellData, cell));
+
+        Assert.AreEqual(0, RefreshAfterRunCallCount());
     }
 
     [TestMethod]
@@ -596,8 +624,8 @@ public async Task Diagnostics_MarkdownCell_IsNotSent()
         StringAssert.Contains(System.Text.Json.JsonSerializer.Serialize(result), "\"items\":[]");
     }
 
-    private int RefreshDiagnosticsCallCount()
-        => TestContext!.JSInterop.Invocations.Count(i => i.Identifier == "versoMonaco.refreshDiagnostics");
+    private int RefreshAfterRunCallCount()
+        => TestContext!.JSInterop.Invocations.Count(i => i.Identifier == "versoMonaco.refreshAfterRun");
 
     private IRenderedComponent<Cell> RenderCell(
         CellModel cell,
@@ -616,6 +644,7 @@ private IRenderedComponent<Cell> RenderCell(
         TestContext.JSInterop.SetupVoid("versoMonaco.setLanguage", _ => true);
         TestContext.JSInterop.SetupVoid("versoMonaco.dispose", _ => true);
         TestContext.JSInterop.SetupVoid("versoMonaco.refreshDiagnostics", _ => true);
+        TestContext.JSInterop.SetupVoid("versoMonaco.refreshAfterRun", _ => true);
         TestContext.JSInterop.SetupVoid("versoMonaco.clearDiagnostics", _ => true);
         TestContext.JSInterop.SetupVoid("versoMonaco.setDiagnosticsSuspended", _ => true);
 

tests/Verso.Blazor.Shared.Tests/MonacoEditorTests.cs

@@ -104,9 +104,11 @@ public async Task RefreshAndClearDiagnostics_CallInterop()
         var cut = Render();
 
         await cut.InvokeAsync(() => cut.Instance.RefreshDiagnosticsAsync());
+        await cut.InvokeAsync(() => cut.Instance.RefreshAfterRunAsync());
         await cut.InvokeAsync(() => cut.Instance.ClearDiagnosticsAsync());
 
         Assert.AreEqual(1, TestContext!.JSInterop.Invocations.Count(i => i.Identifier == "versoMonaco.refreshDiagnostics"));
+        Assert.AreEqual(1, TestContext.JSInterop.Invocations.Count(i => i.Identifier == "versoMonaco.refreshAfterRun"));
         Assert.AreEqual(1, TestContext.JSInterop.Invocations.Count(i => i.Identifier == "versoMonaco.clearDiagnostics"));
     }
 }

tests/Verso.E2E.Tests/Serve/CellEditorHeightTests.cs

@@ -73,7 +73,7 @@ public async Task CellPastTheLimit_SaysSoAndHowToRaiseIt()
         var notebook = _scratch.Write("tall.verso", Notebooks.VersoWithCellsOf(20, 60));
         await OpenAsync(s_server, notebook);
         await MeasureEditorsAsync(".verso-cell", expected: 3);
-        await LineLimitNotes.First.WaitForAsync(ElementState.Visible, timeout: 10_000);
+        await LineLimitNotes.First.WaitForAsync(ElementState.Visible, timeout: 60_000);
 
         Assert.AreEqual(1, await LineLimitNotes.CountAsync(), "Only the cell past the limit should carry the note.");
         var note = await LineLimitNotes.First.InnerTextAsync();

tests/Verso.E2E.Tests/Serve/EditorDiagnosticsTests.cs

@@ -71,6 +71,21 @@ public async Task NameBoundInExecutedCell_IsNotMarked()
         StringAssert.Contains(markers[0].Message, "notDeclared");
     }
 
+    [TestMethod]
+    public async Task MarkerBelowARun_IsClearedWithoutFocus()
+    {
+        await OpenAsync();
+
+        await SetCellSourceAsync(1, "answer + 1");
+        await WaitForMarkersAsync(1, "markers.some(m => String(m.code) === 'CS0103')");
+
+        await SetCellSourceAsync(0, "var answer = 42;");
+        await RunCellAsync(0);
+
+        // The cell below is never focused, so only the run can have asked for it again.
+        await WaitForMarkersAsync(1, "markers.length === 0");
+    }
+
     [TestMethod]
     public async Task KernelRestart_ClearsMarkers()
     {