mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(ui): address /review of the workstream-lifecycle change
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.
This commit is contained in:
+17
-1
@@ -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}"
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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. */
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user