From c4dec213dee6b61ea936bd5b1be68beb3b0c6af1 Mon Sep 17 00:00:00 2001
From: Patrick Buckley <eous@users.noreply.github.com>
Date: Sun, 7 Jun 2026 22:50:53 -0700
Subject: [PATCH] fix(ui): address /review of the workstream-lifecycle change
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Multi-stage review (find → verify → sanity) of b8914854 found one critical
bug plus four minor + one nit; all confirmed against source and fixed:

- CRITICAL — the interactive launcher's "Specific node" pick was unusable:
  selecting a node fired the composer `change` event → onChange →
  _applyLauncherFields → _populateLauncherNodes → setOptionChoices, which
  rebuilds the <select> and reset it to the placeholder, wiping the selection
  the instant it was made (submit then failed "Choose a node…").  Fix:
  _populateLauncherNodes snapshots the current pick before the rebuild and
  restores it after (setOptionValue does not dispatch `change`, so no loop).

- perf — every interactive open blocked first paint on a POST /open round-trip,
  even on the hot rail / active-row paths where the ws is already live.  The
  pane now connects DIRECTLY when the Tier-1 snapshot already names the owning
  node; only the dormant / reload case (snapshot empty) resolves + opens.  This
  resolves the uniform-vs-gated /open question left open last change; refresh
  safety is unchanged (a reload activates before the snapshot lands → nodeForWs
  null → resolve path).

- bug — an errored resolve (capacity / no node free) had no in-place retry
  (re-clicking the active tab is a no-op); the error status line is now
  click-to-retry.

- quality — resolveInteractiveNode surfaced each failure twice (toast + in-pane
  line) with drifted wording; dropped the toasts, the in-pane status line is the
  single source of truth.

- quality — corrected a setOptionFieldVisible comment that cited a nonexistent
  "flex/grid rule" (the row is `display: contents`).

- nit — buildController skips the redundant sessionStorage re-persist when the
  resolved node already matches the persisted hint.

Guard tests extended (bug-1 capture/restore, the live-direct path); the
behavioral harnesses were strengthened to fire a real selection rebuild and to
exercise the live-direct vs reload-resolve split that the first round missed.
---
 tests/test_shell_js.py              |  18 ++++-
 turnstone/console/static/app.js     |  39 ++++++-----
 turnstone/shared_static/composer.js |   5 +-
 turnstone/shared_static/shell.css   |   6 ++
 turnstone/shared_static/shell.js    | 103 +++++++++++++++++++---------
 5 files changed, 117 insertions(+), 54 deletions(-)

diff --git a/tests/test_shell_js.py b/tests/test_shell_js.py
index 373b0084..f7da8ae3 100644
--- a/tests/test_shell_js.py
+++ b/tests/test_shell_js.py
@@ -241,6 +241,16 @@ def test_console_launcher_node_strategy() -> None:
     assert "Composer.prototype.setOptionFieldVisible" in composer, (
         "composer must support conditionally revealing an option field"
     )
+    # Review bug-1 regression: picking a node fires the composer `change` event,
+    # which re-runs _populateLauncherNodes -> setOptionChoices (a rebuild that
+    # resets the <select>).  The selection MUST be captured + restored across the
+    # rebuild, else a Specific-node session can never be launched.
+    assert 'const previous = _homeCoordComposer.getOptionValue("node_id")' in app, (
+        "_populateLauncherNodes must snapshot the current node pick before rebuild"
+    )
+    assert 'if (previous) _homeCoordComposer.setOptionValue("node_id", previous)' in app, (
+        "_populateLauncherNodes must restore the node pick after rebuild (bug-1)"
+    )
 
 
 def test_pane_persists_meta_for_rehydrate() -> None:
@@ -364,7 +374,7 @@ def test_step5_interactive_pane_registered_and_wired() -> None:
         "interactive is ESM — the shell imports it (as it now does the coordinator)"
     )
     assert 'registerType("interactive"' in shell, "shell must register the interactive pane type"
-    assert "createInteractivePane(this.bodyEl, id, {" in shell, (
+    assert "createInteractivePane(pane.bodyEl, id, {" in shell, (
         "first activate must build the controller into the pane body"
     )
     # Rehydrate-safety: the node is RESOLVED + the session (re)opened before the
@@ -377,6 +387,12 @@ def test_step5_interactive_pane_registered_and_wired() -> None:
     assert "pm.setPaneMeta(pane.id" in shell, (
         "the resolved node must be persisted so a reload restores the same node"
     )
+    # Hot-path optimisation (review perf-1): a LIVE session (Tier-1 already names
+    # its node) connects directly — only the dormant/reload case pays the
+    # resolve+open round-trip.
+    assert "const liveNode = caps.cluster ? nodeForWs(id)" in shell, (
+        "a live ws (Tier-1 names its node) must connect directly, skipping POST /open"
+    )
     # Focus-tracking lifecycle: connect/deactivate/destroy + login re-arm.
     for hook in ("this._ctl.connect()", "this._ctl.deactivate()", "this._ctl.destroy()"):
         assert hook in shell, f"interactive pane missing lifecycle {hook!r}"
diff --git a/turnstone/console/static/app.js b/turnstone/console/static/app.js
index f647db03..e30f79a3 100644
--- a/turnstone/console/static/app.js
+++ b/turnstone/console/static/app.js
@@ -1064,6 +1064,13 @@ function _applyLauncherFields() {
 // nodes.  Re-read on each reveal so a node that just (dis)appeared is current.
 function _populateLauncherNodes() {
   if (!_homeCoordComposer) return;
+  // Preserve the operator's current pick across the rebuild: selecting a node
+  // fires the composer `change` event → onChange → _applyLauncherFields → here,
+  // and setOptionChoices() resets the <select> to its placeholder.  Without this
+  // the selection is wiped the instant it's made, so a Specific-node session can
+  // never be launched.  Restored only if the node still exists (else it falls
+  // back to the placeholder, which is correct — the picked node disappeared).
+  const previous = _homeCoordComposer.getOptionValue("node_id");
   const choices = [];
   if (clusterState && clusterState.nodes) {
     Object.keys(clusterState.nodes)
@@ -1079,6 +1086,7 @@ function _populateLauncherNodes() {
       });
   }
   _homeCoordComposer.setOptionChoices("node_id", choices);
+  if (previous) _homeCoordComposer.setOptionValue("node_id", previous);
 }
 
 function _wireLauncherToggle() {
@@ -1980,37 +1988,34 @@ window.TS_APP.resolveInteractiveNode = function (wsId, hintNodeId) {
       return r.ok ? { nodeId: nodeId } : { status: r.status };
     });
   };
+  // Single error surface: the failure is returned as {error} and the interactive
+  // pane writes it into its .pane-status line (the pane is always the active tab
+  // when it resolves).  No toast — one source of truth, no wording drift.
   const failResult = function (status) {
-    if (status === 429) {
-      showToast("Node at capacity; close a session and retry");
+    if (status === 429)
       return { error: "Node at capacity — close a session and retry." };
-    }
-    if (status === 403) {
-      showToast("Not permitted to open this session");
+    if (status === 403)
       return { error: "You don’t have permission to open this session." };
-    }
     return { error: "Could not open this session (" + status + ")." };
   };
   const routeFallback = function () {
     return authFetch("/v1/api/route?ws_id=" + encodeURIComponent(wsId))
       .then(function (r) {
         if (!r.ok) {
-          const msg =
-            r.status === 503
-              ? "No nodes available to open this session."
-              : "Could not locate the session node (" + r.status + ").";
-          showToast(msg);
-          return { error: msg };
+          return {
+            error:
+              r.status === 503
+                ? "No nodes available to open this session."
+                : "Could not locate the session node (" + r.status + ").",
+          };
         }
         return r.json();
       })
       .then(function (route) {
-        if (!route) return { error: "Could not locate the session node." };
-        if (route.error) return route; // route fetch already failed + toasted
-        if (!route.node_id) {
-          showToast("Could not locate the session node");
+        if (!route || route.error)
+          return route || { error: "Could not locate the session node." };
+        if (!route.node_id)
           return { error: "Could not locate the session node." };
-        }
         return openOn(route.node_id).then(function (res) {
           return res.nodeId ? res : failResult(res.status);
         });
diff --git a/turnstone/shared_static/composer.js b/turnstone/shared_static/composer.js
index 159ee746..a0b9fb6f 100644
--- a/turnstone/shared_static/composer.js
+++ b/turnstone/shared_static/composer.js
@@ -754,8 +754,9 @@
   // Show/hide a single options field (its label + control row).  Lets a caller
   // reveal a field conditionally — e.g. the launcher shows the node picker only
   // for the interactive persona, and the node list only under "Specific node".
-  // Toggles inline display (robust against the field row's own flex/grid rule,
-  // which [hidden] alone would lose a specificity battle with).
+  // Sets inline display because the row is `display: contents` (chat.css:537),
+  // which outweighs the [hidden] attribute's UA `display:none` on specificity —
+  // so toggling [hidden] alone would not actually hide the row.
   Composer.prototype.setOptionFieldVisible = function (id, visible) {
     var ctrl = this._optionFields && this._optionFields[id];
     if (!ctrl) return;
diff --git a/turnstone/shared_static/shell.css b/turnstone/shared_static/shell.css
index 774f6231..872a41cd 100644
--- a/turnstone/shared_static/shell.css
+++ b/turnstone/shared_static/shell.css
@@ -721,6 +721,12 @@
   color: var(--fg-dim);
   font-size: 13px;
 }
+/* An errored resolve is click-to-retry (see shell.js showResolveError). */
+.pane-status--retry {
+  cursor: pointer;
+  text-decoration: underline;
+  text-underline-offset: 2px;
+}
 
 /* ===== Dashboard session launcher — persona toggle (coordinator | interactive).
    A console-dashboard control; lives here because the L-shell loads shell.css. */
diff --git a/turnstone/shared_static/shell.js b/turnstone/shared_static/shell.js
index c816a5b8..5c0f2f54 100644
--- a/turnstone/shared_static/shell.js
+++ b/turnstone/shared_static/shell.js
@@ -391,14 +391,77 @@ async function mountShell() {
     // resolved node after ensureInteractiveNode settles, below.
     if (extra && extra.nodeId) pane.meta = { nodeId: extra.nodeId };
     pane.onMount = function () {
-      // The controller is built LAZILY on first activate — only after the owning
-      // node is resolved AND the session is (re)opened there (the node /events
-      // stream 404s on a ws not loaded on its node, so a rehydrated pane can't
-      // just connect blind).  onMount only reserves a status line so a
-      // not-yet-resolved pane isn't a blank box.
+      // The controller is built LAZILY on first activate (see beginConnect) —
+      // a node-proxied session may need its node resolved + (re)opened first.
+      // onMount only reserves a status line so a not-yet-connected pane isn't a
+      // blank box.
       this._statusEl = make("div", "pane-status", "Connecting…");
       this.bodyEl.append(this._statusEl);
     };
+    // Build the controller on a resolved node, persist that node for the next
+    // reload, and open the stream.  nodeId null = standalone-local (base="").
+    const buildController = (nodeId) => {
+      if (pane._closed) return; // resolved after the tab was closed
+      // Persist the node so a reload restores onto the SAME node — but skip the
+      // write when it already matches (no redundant sessionStorage round-trip).
+      if (nodeId && (!pane.meta || pane.meta.nodeId !== nodeId)) {
+        pane.meta = { nodeId };
+        pm.setPaneMeta(pane.id, pane.meta);
+      }
+      if (pane._statusEl && pane._statusEl.parentNode) pane._statusEl.remove();
+      pane._statusEl = null;
+      pane._ctl = createInteractivePane(pane.bodyEl, id, {
+        nodeId,
+        onClose: () => pm.close(pane.id),
+      });
+      pane._ctl.connect();
+      if (window.TS_LOGIN && pane._ctl.onLogin) {
+        pane._loginArmed = true;
+        window.TS_LOGIN.subscribe(pane._ctl.onLogin);
+      }
+    };
+    // Errored resolve (capacity / no node free): show it in the status line and
+    // offer a one-click retry — re-clicking the tab won't re-fire onActivate
+    // (PaneManager fires it only on a pane CHANGE), so without this a transient
+    // failure would strand the pane until the user closed + reopened it.
+    const showResolveError = (msg) => {
+      const el = pane._statusEl;
+      if (!el) return;
+      el.className = "pane-status pane-status--retry msg error";
+      el.textContent = msg || "Could not connect to this session.";
+      el.title = "Click to retry";
+      el.onclick = () => {
+        if (pane._ctl || pane._resolving) return;
+        el.className = "pane-status";
+        el.title = "";
+        el.onclick = null;
+        el.textContent = "Connecting…";
+        beginConnect();
+      };
+    };
+    // First-activate connect.  A LIVE session (Tier-1 already names its node, so
+    // it is loaded there) connects DIRECTLY — no /open round-trip; this is the
+    // hot rail / active-row / just-created path.  Standalone runs locally.  Only
+    // the dormant / reload case (the snapshot has no node for this ws) resolves
+    // the node + (re)opens the session, whose /events would otherwise 404.
+    const beginConnect = () => {
+      const liveNode = caps.cluster ? nodeForWs(id) : null;
+      if (liveNode || !caps.cluster) {
+        buildController(liveNode || null);
+        return;
+      }
+      pane._resolving = true;
+      const hint = (pane.meta && pane.meta.nodeId) || (extra && extra.nodeId);
+      ensureInteractiveNode(caps, id, hint).then((res) => {
+        pane._resolving = false;
+        if (pane._closed) return; // closed mid-resolve — don't build into a detached body
+        if (!res || res.error) {
+          showResolveError(res && res.error);
+          return;
+        }
+        buildController(res.nodeId);
+      });
+    };
     pane.onActivate = function () {
       pm.setTabGlyph(pane.id, glyph(stateForWs(id))); // live Tier-1 state glyph
       if (this._ctl) {
@@ -406,35 +469,7 @@ async function mountShell() {
         return;
       }
       if (this._resolving) return; // first-activate resolve already in flight
-      this._resolving = true;
-      const hint = (this.meta && this.meta.nodeId) || (extra && extra.nodeId);
-      ensureInteractiveNode(caps, id, hint).then((res) => {
-        this._resolving = false;
-        if (this._closed) return; // pane closed mid-resolve — don't build into a detached body
-        if (!res || res.error) {
-          if (this._statusEl) {
-            this._statusEl.className = "pane-status msg error";
-            this._statusEl.textContent =
-              (res && res.error) || "Could not connect to this session.";
-          }
-          return;
-        }
-        // Pin + persist the resolved node so the next reload reuses it.
-        this.meta = { nodeId: res.nodeId };
-        pm.setPaneMeta(pane.id, this.meta);
-        if (this._statusEl && this._statusEl.parentNode)
-          this._statusEl.remove();
-        this._statusEl = null;
-        this._ctl = createInteractivePane(this.bodyEl, id, {
-          nodeId: res.nodeId,
-          onClose: () => pm.close(pane.id),
-        });
-        this._ctl.connect();
-        if (window.TS_LOGIN && this._ctl.onLogin) {
-          this._loginArmed = true;
-          window.TS_LOGIN.subscribe(this._ctl.onLogin);
-        }
-      });
+      beginConnect();
     };
     pane.onDeactivate = function () {
       if (this._ctl && this._ctl.deactivate) this._ctl.deactivate();