Alert notebook: read-only render on #/triage (#4222)

Darling/Darling.Tests/AlertNotebookRenderClientTests.cs

@@ -0,0 +1,176 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System;
+using Xunit;
+using static Darling.Tests.RepoFile;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #4222 (client slice c): source-text pins over <c>wwwroot/js/pages/triage.js</c> and
+/// <c>wwwroot/js/pages/views.js</c>'s alert-notebook render path. There is no JS test runner in this
+/// repository, so these pin the SHAPE of the shipped source the way <see cref="ViewTemplatesTests"/> and
+/// <see cref="ServerPageTabsTests"/> already do for this same pair of files — a rewrite that drops one of
+/// these properties changes visible behaviour without failing the C# build otherwise.
+///
+/// Pinned properties (the brief's own list):
+///  - the triage alert path renders through the SAME <c>renderNotebookDoc</c> the saved-view page uses, with
+///    an in-memory (not saved) definition;
+///  - the alert render path never calls <c>/api/fleet</c> (no fleet scope picker \u2014 the server is fixed);
+///  - a max-3 in-flight limiter exists and gates each alert-mode read cell's load;
+///  - "Save as notebook" POSTs to <c>/api/views</c> carrying the provenance string.
+/// </summary>
+public sealed class AlertNotebookRenderClientTests
+{
+    private const string TriagePath = "Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/triage.js";
+    private const string ViewsPath = "Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/views.js";
+
+    [Fact]
+    public void Triage_RendersTheAlertNotebook_ThroughTheSharedRenderNotebookDocWithAnInMemoryDefinition()
+    {
+        var triage = ReadRepoFile(TriagePath);
+
+        Assert.Contains("import { renderNotebookDoc } from \"./views.js\";", triage, StringComparison.Ordinal);
+        Assert.Contains("mode: \"alert\"", triage, StringComparison.Ordinal);
+        Assert.Contains("definition: def", triage, StringComparison.Ordinal);
+        Assert.Contains("/api/alert-notebook", triage, StringComparison.Ordinal);
+
+        // #2710: /api/triage stays the fallback for one release, on a 404 from the new endpoint.
+        Assert.Contains("/api/triage", triage, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void Triage_AlertNotebookPath_NeverReadsApiFleet()
+    {
+        var triage = ReadRepoFile(TriagePath);
+
+        Assert.DoesNotContain("/api/fleet", triage, StringComparison.Ordinal);
+        Assert.DoesNotContain("apiGetFleet", triage, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void Views_AlertMode_SkipsTheFleetScopeBar()
+    {
+        var views = ReadRepoFile(ViewsPath);
+
+        // The alert branch of renderNotebookDoc must not call fleetOptions/apiGetFleet on its own path \u2014
+        // only the saved-view (`!isAlert`) branch may.
+        var alertBranchStart = views.IndexOf("if (!isAlert) {", StringComparison.Ordinal);
+        Assert.True(alertBranchStart > 0, "Expected renderNotebookDoc's saved-mode scope-bar branch guarded by `if (!isAlert)`.");
+    }
+
+    [Fact]
+    public void Views_HasAMaxThreeInFlightLimiter_GatingAlertReadCells()
+    {
+        var views = ReadRepoFile(ViewsPath);
+
+        Assert.Contains("class InFlightLimiter", views, StringComparison.Ordinal);
+        Assert.Contains("new InFlightLimiter(3)", views, StringComparison.Ordinal);
+        Assert.Contains("limiter.acquire()", views, StringComparison.Ordinal);
+        Assert.Contains("limiter.release()", views, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void Views_SaveAsNotebook_PostsToApiViews_WithTheProvenanceString()
+    {
+        var views = ReadRepoFile(ViewsPath);
+
+        Assert.Contains("function saveAsNotebookButton(", views, StringComparison.Ordinal);
+        Assert.Contains("api.createView({ name, description: provenance || null, definition });", views, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void Triage_ProvenanceString_NamesTemplateMetricServerAndAt()
+    {
+        var triage = ReadRepoFile(TriagePath);
+
+        Assert.Contains("\"from alert template \"", triage, StringComparison.Ordinal);
+        Assert.Contains("t.template && t.template.id", triage, StringComparison.Ordinal);
+        Assert.Contains("t.template && t.template.version", triage, StringComparison.Ordinal);
+    }
+
+    /// <summary>
+    /// "Open live" drops each read cell's <c>as_of</c> pin (today's only per-cell absolute-window field in the
+    /// #4366 shape \u2014 no composed cells, no per-cell `range`, yet) rather than a client-authored recompute.
+    /// </summary>
+    [Fact]
+    public void Triage_OpenLive_DropsTheReadCellsAsOfParam()
+    {
+        var triage = ReadRepoFile(TriagePath);
+
+        Assert.Contains("function stripAsOf(", triage, StringComparison.Ordinal);
+        Assert.Contains("as_of", triage, StringComparison.Ordinal);
+        Assert.Contains("onOpenLive: () => { def = stripAsOf(def); paint(); }", triage, StringComparison.Ordinal);
+    }
+
+    /// <summary>
+    /// #4368: the header cell must render current_value/threshold_value/detail_text — the endpoint's alert
+    /// node carries all three (AlertNotebookEndpoint.cs's AlertRowNode), and the header dropped them entirely
+    /// before this slice. Pinned through <c>el()</c>'s text path only (R4): a rewrite that switches either
+    /// field to <c>innerHTML</c> or a template string must fail this build, not just an XSS review.
+    /// </summary>
+    [Fact]
+    public void Views_AlertHeaderCell_RendersValueThresholdAndDetailText_ThroughElsTextPath()
+    {
+        var views = ReadRepoFile(ViewsPath);
+
+        Assert.Contains("a.current_value", views, StringComparison.Ordinal);
+        Assert.Contains("a.threshold_value", views, StringComparison.Ordinal);
+        Assert.Contains("a.detail_text", views, StringComparison.Ordinal);
+
+        // The detail_text line must go through el()'s { text: ... } prop (textContent), never innerHTML.
+        Assert.Contains("el(\"pre\", { class: \"code\", text: a.detail_text })", views, StringComparison.Ordinal);
+
+        // Scope the innerHTML ban to renderAlertHeaderCell's own body — the file's header comment (R4)
+        // says "never innerHTML" in prose, which the whole-file assertion this replaces was tripping on.
+        var lf = ReadRepoFileLf(ViewsPath);
+        var start = lf.IndexOf("function renderAlertHeaderCell(", StringComparison.Ordinal);
+        Assert.True(start >= 0, "renderAlertHeaderCell not found in views.js");
+        var nextFn = lf.IndexOf("\nfunction ", start + 1, StringComparison.Ordinal);
+        Assert.True(nextFn > start, "could not find the next top-level function after renderAlertHeaderCell");
+        var body = lf.Substring(start, nextFn - start);
+        var bodyWithoutComments = StripJsComments(body);
+        Assert.DoesNotContain(".innerHTML", bodyWithoutComments, StringComparison.Ordinal);
+        Assert.DoesNotContain("innerHTML =", bodyWithoutComments, StringComparison.Ordinal);
+        Assert.DoesNotContain("insertAdjacentHTML", bodyWithoutComments, StringComparison.Ordinal);
+    }
+
+    /// <summary>
+    /// Strips `//` line comments and `/* ... */` block comments so a pin can check for real code use of a
+    /// pattern without tripping on prose that happens to mention it (e.g. "never innerHTML" in a comment).
+    /// Not a full JS parser — good enough for this file, which has no such pattern inside a string literal.
+    /// </summary>
+    private static string StripJsComments(string source)
+    {
+        var withoutBlockComments = System.Text.RegularExpressions.Regex.Replace(
+            source, @"/\*.*?\*/", string.Empty, System.Text.RegularExpressions.RegexOptions.Singleline);
+        var withoutLineComments = System.Text.RegularExpressions.Regex.Replace(
+            withoutBlockComments, @"//[^\n]*", string.Empty);
+        return withoutLineComments;
+    }
+
+    /// <summary>
+    /// #4368: a route change (hashchange off #/triage, or navigating away entirely) while an alert-mode read
+    /// cell's slot is queued or its fetch is outstanding must not paint a stale result into a detached holder.
+    /// The fetch and the limiter's release still have to run either way — only the mount is guarded.
+    /// </summary>
+    [Fact]
+    public void Views_AlertModeReadCells_GuardAgainstRenderingAfterARouteChange()
+    {
+        var views = ReadRepoFile(ViewsPath);
+
+        Assert.Contains("opts.isLive", views, StringComparison.Ordinal);
+        Assert.Contains("const stillLive = !opts.isLive || opts.isLive();", views, StringComparison.Ordinal);
+        Assert.Contains("if (stillLive) mount(holder, rendered);", views, StringComparison.Ordinal);
+
+        var triage = ReadRepoFile(TriagePath);
+        Assert.Contains("const isLive = () => location.hash === ourHash;", triage, StringComparison.Ordinal);
+        Assert.Contains("isLive,", triage, StringComparison.Ordinal);
+    }
+}

Darling/Darling.Tests/RepoFileAdoptionTests.cs

@@ -114,6 +114,10 @@ public sealed class RepoFileAdoptionTests
            its four source lines as one literal, so the guard's return is told apart from just those words
            appearing somewhere else in the file. */
         "AlertHistoryLazyRenderTests.cs",
+        /* #4368: its innerHTML-ban pin anchors renderAlertHeaderCell's own function body across the lines
+           between its declaration and the next top-level function, so the ban is scoped to real code and
+           not tripped by the file's own prose comment saying "never innerHTML". */
+        "AlertNotebookRenderClientTests.cs",
         "BuiltinAlertPersistenceRungTests.cs",
         "ChartWindowDomainTests.cs",
         "DarlingPathFilterGateTests.cs",

Darling/Darling.Tests/WebFetchLayerTests.cs

@@ -165,8 +165,14 @@ public void PanelsJs_CapturesAndAppliesTheRenderSignal()
         var panels = PanelsJs;
 
         Assert.Contains("export function setPanelSignal(signal) {", panels, StringComparison.Ordinal);
-        Assert.Contains("async function loadPanel(desc, body, signal) {", panels, StringComparison.Ordinal);
-        Assert.Contains("loadPanel(desc, body, signal);", panels, StringComparison.Ordinal);
+
+        // #4368: loadPanel grew an `onSettled` param — the alert notebook's max-3 in-flight limiter
+        // (views.js's renderAlertCell) needs to know when a panel's load reaches a terminal state so it can
+        // free the slot. The render-signal capture/apply behaviour this pin protects is unchanged: renderPanel
+        // still captures panelSignal synchronously and threads it straight through to the fetch.
+        Assert.Contains("export function renderPanel(desc, onSettled) {", panels, StringComparison.Ordinal);
+        Assert.Contains("async function loadPanel(desc, body, signal, onSettled) {", panels, StringComparison.Ordinal);
+        Assert.Contains("loadPanel(desc, body, signal, onSettled);", panels, StringComparison.Ordinal);
         Assert.Contains("if (res.kind === \"aborted\" || res.kind === \"auth\") {", panels, StringComparison.Ordinal);
     }
 

Darling/PerformanceMonitor.Darling.Service/wwwroot/js/compose.js

@@ -40,13 +40,13 @@ const OTHER_SERIES_LABEL = "(other)";
  * shape (title + span-2 + a body that shows a loading strip, then the chart / a state). `scope` is the view-level
  * run context {server, hours, variables, values}; flipping it and re-rendering re-scopes every panel at once.
  */
-export function renderComposedPanelCard(panelSpec, scope) {
+export function renderComposedPanelCard(panelSpec, scope, onSettled) {
   const body = el("div", { class: "panel-body" }, [loadingStrip()]);
   const panel = el("div", { class: "panel card" + (panelSpec.span === 2 ? " span-2" : "") }, [
     el("h3", {}, [panelSpec.title || measureLabel(panelSpec), pinBadge(panelSpec)]),
     body,
   ]);
-  driveComposedPanel(body, panelSpec, scope);
+  driveComposedPanel(body, panelSpec, scope, onSettled);
   return panel;
 }
 
@@ -198,9 +198,11 @@ function pinHoursLabel(hours) {
  * never persisted (the stored definition renders verbatim), and a "clear" chip pops back to the base spec. Each new
  * selection REPLACES the drill (a simple "you are viewing X" model), so drilling never stacks into a dead end.
  */
-function driveComposedPanel(body, panelSpec, scope) {
+function driveComposedPanel(body, panelSpec, scope, onSettled) {
   let drill = null; // { keys: [{dimension, value}] } or null
   let zoom = null; // { startIso, endIso } or null (#1606 brush-zoom — view-state only, like the drill)
+  let firstRun = true; // onSettled (#4222) fires once, for the initial load only — a later drill/zoom re-run is
+                        // user-driven and already past whatever concurrency budget gated the first load.
   function onDrill(next) {
     drill = next && Array.isArray(next.keys) && next.keys.length ? next : null;
     run();
@@ -211,7 +213,11 @@ function driveComposedPanel(body, panelSpec, scope) {
   }
   function run() {
     const spec = drill ? withDrillFilters(panelSpec, drill) : panelSpec;
-    renderComposedInto(body, spec, scope, { drill, onDrill, zoom, onZoomChange });
+    const done = firstRun ? onSettled : null;
+    firstRun = false;
+    renderComposedInto(body, spec, scope, { drill, onDrill, zoom, onZoomChange }).finally(() => {
+      if (done) done();
+    });
   }
   run();
 }

Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/triage.js

@@ -24,6 +24,8 @@ import { el, mount, apiGet, buildQuery, loadingStrip, errorStrip, emptyStrip, no
          alertDeliveryState } from "../util.js";
 import { VIZ } from "../panels.js";
 import { suggestViz, deriveVizConfig } from "../derive.js";
+import { renderNotebookDoc } from "./views.js";
+import { getSession, getCatalog } from "../views-api.js";
 
 /* The get_alert_history wire shape's status collapse, matching the Alert History page's own cell — including
    its #2781 rule: the "tray" channel (the Lite/Dashboard system-tray toast) is meaningless on the headless web,
@@ -96,6 +98,76 @@ function sectionCard(section) {
   ]);
 }
 
+/* #4222: the alert notebook render. GET /api/alert-notebook returns {alert, status, notes, template, definition}
+   bound to this link's server/metric/at/dedup — a per-family template server-side, so the page can render the
+   forensic reads an investigation needs (v1 read cells) as a document, read-only, no fleet scope picker (the
+   server is fixed). /api/triage is NOT called first: /api/alert-notebook is tried, and only on a 404 (it isn't on
+   dev yet, or this build predates it) does the page fall back to the legacy /api/triage assembly below — so the
+   route works whichever endpoint is live. Status refresh on the 60s poll is OUT of scope here: it needs the poll
+   guard #4222(d) adds to app.js's refresh(), which is not in this slice. */
+async function renderAlertNotebook(main, box, server, metric, at, dedup) {
+  /* #4368: the same route-change guard renderView (views.js) already uses for its own await gap — captured
+     BEFORE the first await, so a hashchange between now and any later check is visible. Threaded into
+     renderNotebookDoc as `opts.isLive` so its alert-mode read cells can drop a result that lands after the
+     viewer has navigated off #/triage, without cancelling the fetch or starving the limiter's next slot. */
+  const ourHash = location.hash;
+  const isLive = () => location.hash === ourHash;
+  const res = await apiGet("/api/alert-notebook" + buildQuery({ server, metric, at, dedup }));
+  if (res.kind === "error" && res.status === 404) return false; // not on this build — caller falls back
+  if (res.kind === "error") {
+    mount(box, errorStrip(res.message));
+    return true;
+  }
+  if (!isLive()) return true; // navigated away during the fetch above — nothing left to mount into.
+
+  const t = res.data || {};
+  const notes = Array.isArray(t.notes) ? t.notes : [];
+  const [session, catalog] = await Promise.all([getSession(), getCatalog()]);
+  const canEdit = !!session.can_edit;
+  if (!isLive()) return true; // ditto, across the session/catalog await gap.
+
+  let def = t.definition || { kind: "notebook", cells: [] };
+  const provenance =
+    "from alert template " + (t.template && t.template.id) + " v" + (t.template && t.template.version) +
+    ", " + metric + " on " + server + " at " + at;
+
+  function stripAsOf(d) {
+    const cells = Array.isArray(d.cells) ? d.cells : [];
+    return {
+      ...d,
+      cells: cells.map((c) => {
+        if (c && c.type === "read" && c.params && c.params.as_of != null) {
+          const { as_of, ...rest } = c.params;
+          return { ...c, params: rest };
+        }
+        return c;
+      }),
+    };
+  }
+
+  const noteBox = el("div", {});
+  const docHolder = el("div", {});
+  mount(box, [noteBox, docHolder]);
+
+  function paint() {
+    mount(noteBox, notes.map((n) => noticeStrip(n)));
+    renderNotebookDoc(docHolder, {
+      mode: "alert",
+      definition: def,
+      alert: t.alert || null,
+      status: t.status,
+      notes,
+      canEdit,
+      catalog,
+      provenance,
+      isLive,
+      onOpenLive: () => { def = stripAsOf(def); paint(); },
+    });
+  }
+  paint();
+  return true;
+}
+
 export async function renderTriage(main, queryString) {
   const params = new URLSearchParams(queryString || "");
   const server = params.get("server") || "";
@@ -113,6 +185,10 @@ export async function renderTriage(main, queryString) {
   ]);
 
   mount(box, loadingStrip("Assembling triage context…"));
+
+  const handled = await renderAlertNotebook(main, box, server, metric, at, dedup);
+  if (handled) return;
+
   const res = await apiGet("/api/triage" + buildQuery({ server, metric, at, dedup }));
   if (res.kind === "error") return mount(box, errorStrip(res.message));