From c3beb202eb06b6f2b2a81fcdecd8bae5c4e34cab Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sat, 18 Jul 2026 23:20:36 -0700 Subject: [PATCH] fix(ui): apply round-2 review + fix-sanity findings (composer caches) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unprimed convergence re-review found a real login-recovery seam gap plus cleanups (round 1's fix round manufactured one of them); fix-sanity vetted the plan. - console onLoginSuccess recovery seam [0]+[2]: it re-warmed only skills+models after an in-place login; projects+personas (same pre-auth-401 gap) stayed empty (rail group-by-project flat, saved-coordinator raw slugs). Now force-refreshes ALL FOUR caches on login — force so a still-in-flight failing pre-auth fetch yields a trailing AUTHENTICATED refetch rather than coalescing onto the 401 (skills/personas have no *_changed event to recover). Threads an optional callOpts through the four cache modules + console wrappers (backward-compatible; every non-console caller passes nothing). - fork skill paint [4]: the round-1 wrapper extraction left the modal skill paint unconditional on a fork (wasted GET /v1/api/skills + hidden-select rebuild); fork-gate it like model/persona/project. - persona wrapper [5]: extract _paintPersonaSelect so all four composer pickers share the sync-then-refresh wrapper instead of persona being inline-duplicated. - dead machinery [6]: remove the zero-subscriber onModelsChange/onSkillsChange and the models fpExtra fingerprint fold (and the now-orphaned core fpExtra branch). The console repaints models via its direct models_changed handler, not a subscription; the fold only fed the subscriber-only fingerprint. - O(1) modelLabel [7]: index the models cache by alias (keyField) so modelLabel is a getByKey, not a per-paint scan. Declines documented in-code: forks-inherit-model [1] (deliberate) and the fail-open cache [3] (intended, same policy as projects/personas). Deferral comment at the ui onLoginSuccess twin (recovers on dashboard re-focus; follow-up). Tests: rewrote the 7 guards the code changes moved (persona relocation, callOpts threading, force, fpExtra removal) preserving their ordering intent, and added fork-skill-gate, persona-wrapper, all-four-force, keyField, and onModelsChange-removed coverage. 106 pass; ruff + mypy green. --- tests/test_app_js.py | 84 ++++++++++++++++++--------- turnstone/console/static/app.js | 40 ++++++++----- turnstone/shared_static/list_cache.js | 12 +--- turnstone/shared_static/models.js | 35 ++++------- turnstone/shared_static/personas.js | 4 +- turnstone/shared_static/projects.js | 12 ++-- turnstone/shared_static/skills.js | 18 +++--- turnstone/ui/static/app.js | 66 +++++++++++++-------- 8 files changed, 155 insertions(+), 116 deletions(-) diff --git a/tests/test_app_js.py b/tests/test_app_js.py index df0bcab4..7c6635e7 100644 --- a/tests/test_app_js.py +++ b/tests/test_app_js.py @@ -2412,12 +2412,10 @@ def test_new_ws_modal_paints_project_and_persona_from_cache_synchronously() -> N assert "_paintProjectPicker(projSelect" in fn, ( "the modal must paint the project picker via the shared _paintProjectPicker helper" ) - sync_persona = fn.find("_populatePersonaSelect(personaSelect") - async_persona = fn.find("refreshPersonas().then") - assert sync_persona >= 0, "the modal must paint the persona picker from cache synchronously" - assert async_persona >= 0, "the modal must keep refreshPersonas().then" - assert sync_persona < async_persona, ( - "the synchronous persona paint must precede the async refreshPersonas().then repaint" + # Persona is painted via the shared _paintPersonaSelect wrapper (fork-gated); + # its sync-before-async ordering is pinned in the wrapper-internals test. + assert "_paintPersonaSelect(personaSelect" in fn, ( + "the modal must paint the persona picker via the shared _paintPersonaSelect helper" ) @@ -2433,12 +2431,9 @@ def test_dashboard_paints_project_and_persona_from_cache_synchronously() -> None assert "_paintProjectPicker(projSel" in fn, ( "the dashboard must paint the project picker via the shared _paintProjectPicker helper" ) - sync_persona = fn.find("_populatePersonaSelect(personaSel") - async_persona = fn.find("refreshPersonas().then") - assert sync_persona >= 0, "the dashboard must paint the persona picker from cache synchronously" - assert async_persona >= 0, "the dashboard must keep refreshPersonas().then" - assert sync_persona < async_persona, ( - "the synchronous dashboard persona paint must precede the async refresh repaint" + # Persona is painted via the shared _paintPersonaSelect wrapper (freshOnOpen:false). + assert "_paintPersonaSelect(personaSel, { freshOnOpen: false })" in fn, ( + "the dashboard must paint the persona picker via _paintPersonaSelect (preserving)" ) # require_project label-hint parity: the dashboard Project label gained a # .label-hint span, seeded synchronously from requireProject() like the modal. @@ -2460,17 +2455,17 @@ def test_console_launcher_paints_project_and_persona_from_cache_synchronously() # The bare _populateHomeProjectDropdown() call is the sync paint; the refresh's # .then argument has no parens, so this matches only the standalone sync call. sync_proj = proj_fn.find("_populateHomeProjectDropdown()") - async_proj = proj_fn.find("refreshProjects().then") + async_proj = proj_fn.find("refreshProjects(callOpts).then") assert sync_proj >= 0, "the launcher must paint the project picker from cache synchronously" - assert async_proj >= 0, "the launcher must keep refreshProjects().then" + assert async_proj >= 0, "the launcher must keep refreshProjects(callOpts).then" assert sync_proj < async_proj, ( "the synchronous launcher project paint must precede the async refresh repaint" ) persona_fn = _slice_top_level_fn(body, "function _refreshAndPopulatePersonas(") sync_persona = persona_fn.find("_populateHomePersonaDropdown()") - async_persona = persona_fn.find("refreshPersonas().then") + async_persona = persona_fn.find("refreshPersonas(callOpts).then") assert sync_persona >= 0, "the launcher must paint the persona picker from cache synchronously" - assert async_persona >= 0, "the launcher must keep refreshPersonas().then" + assert async_persona >= 0, "the launcher must keep refreshPersonas(callOpts).then" assert sync_persona < async_persona, ( "the synchronous launcher persona paint must precede the async refresh repaint" ) @@ -2532,6 +2527,11 @@ def test_paint_model_and_skill_wrappers_sync_before_refresh() -> None: for wrapper, populate, refresh in ( ("function _paintModelSelects(", "_populateModelSelect(", "refreshModels().then"), ("function _paintSkillSelect(", "_populateSkillSelect(", "refreshSkills().then"), + ( + "function _paintPersonaSelect(", + "_populatePersonaSelect(", + "refreshPersonas().then", + ), ): fn = _slice_top_level_fn(body, wrapper) sync = fn.find(populate) @@ -2557,8 +2557,8 @@ def test_new_ws_modal_renders_all_selects_fresh_on_open() -> None: assert "_paintSkillSelect(tplSelect, { freshOnOpen: true })" in fn, ( "the modal must paint skill fresh-on-open" ) - assert "_populatePersonaSelect(personaSelect, { fresh: true })" in fn, ( - "the modal must paint persona fresh-on-open (kind default, no stale carryover)" + assert "_paintPersonaSelect(personaSelect, { freshOnOpen: true })" in fn, ( + "the modal must paint persona fresh-on-open via the shared wrapper" ) proj = fn[fn.find("_paintProjectPicker(projSelect") :] assert "freshOnOpen: true" in proj[:120], ( @@ -2601,6 +2601,17 @@ def test_new_ws_modal_fork_inherits_model_and_judge() -> None: assert "judgeSelect.hidden = !!_forkFromWsId" in modal, ( "modal must hide the judge select for a fork" ) + # [4] fix: the skill paint is fork-gated too (skill is hidden for a fork). + # Tie the gate to the skill paint SPECIFICALLY — a first-gate check would be + # satisfied by the model gate at line ~352 even if the skill paint were left + # unconditional, so require the nearest preceding gate to be right above it. + skill_paint = modal.find("_paintSkillSelect(tplSelect") + assert skill_paint >= 0, "the modal must paint the skill picker" + gate = modal.rfind("if (!_forkFromWsId) {", 0, skill_paint) + assert gate >= 0 and (skill_paint - gate) < 50, ( + "the modal skill paint must be directly wrapped in `if (!_forkFromWsId)` " + "(skip the wasted fetch + hidden-select rebuild on a fork)" + ) submit = _slice_top_level_fn(body, "function submitNewWs(") assert "if (model && !_forkFromWsId) body.model = model;" in submit, ( "submitNewWs must fork-gate body.model (distinct from the judge line)" @@ -2624,21 +2635,26 @@ def test_console_launcher_paints_model_and_skill_from_cache_synchronously() -> N ) skill_fn = _slice_top_level_fn(body, "function _refreshAndPopulateSkills(") sync_skill = skill_fn.find("_populateHomeSkillDropdown()") - async_skill = skill_fn.find("refreshSkills().then") + async_skill = skill_fn.find("refreshSkills(callOpts).then") assert 0 <= sync_skill < async_skill, ( "the synchronous launcher skill paint must precede the async refresh repaint" ) -def test_console_relogin_rewarms_model_and_skill() -> None: - """onLoginSuccess re-warms BOTH the skill and model caches after auth lands — - the boot-time pass runs pre-login (401), so without the model re-warm the - console model dropdown would stay at its placeholder until a reload.""" +def test_console_relogin_rewarms_all_four_caches_with_force() -> None: + """onLoginSuccess re-warms ALL FOUR composer caches after auth lands (the boot + pass runs pre-login -> 401 -> fail-open empty), EACH with {force:true} so a + still-in-flight failing pre-auth fetch yields a trailing authenticated refetch + (skills/personas have no *_changed event to recover otherwise). Fixes [0]+[2].""" body = _CONSOLE_APP_JS.read_text(encoding="utf-8") start = body.index("window.onLoginSuccess = function ()") + # The four re-warm calls live before the // Active-coordinators marker; an + # assert falling outside this slice fails loudly rather than silently passing. login = body[start : body.index("// Active-coordinators", start)] - assert "_refreshAndPopulateSkills()" in login, "onLoginSuccess must re-warm skills" - assert "_refreshAndPopulateModels()" in login, "onLoginSuccess must re-warm models" + for name in ("Skills", "Models", "Projects", "Personas"): + assert f"_refreshAndPopulate{name}({{ force: true }})" in login, ( + f"onLoginSuccess must force-re-warm {name.lower()} after login" + ) def test_models_changed_forces_trailing_single_repaint_path() -> None: @@ -2668,6 +2684,13 @@ def test_models_label_centralized_on_bridge() -> None: "models.js must register modelLabel on the window.TurnstoneModels bridge " "(classic bundles call it via the bridge)" ) + # [7] fix: modelLabel resolves via the core's O(1) keyField index, not a scan. + assert 'keyField: "alias"' in models_src, ( + "models.js must index by alias (keyField) so modelLabel is an O(1) getByKey" + ) + assert "getByKey(alias)" in models_src, ( + "modelLabel must resolve via the core's getByKey index (not a per-paint scan)" + ) assert "_resolveModelLabel" not in _APP_JS.read_text(encoding="utf-8"), ( "the ui app must not keep a local _resolveModelLabel (use TurnstoneModels.modelLabel)" ) @@ -2730,14 +2753,19 @@ def test_reset_extra_policy_per_cache() -> None: def test_models_cache_exposes_both_server_schemas() -> None: """models.js must carry ALL default-alias fields — the node server sends default_alias, the console sends coordinator_default_alias, both send - judge_default_alias — so each app reads its own, and fold them into the - fingerprint so a default-only change still fires onChange (R6).""" + judge_default_alias — so each app reads its own (via modelDefaults/captureExtra, + which drive the "Default — " placeholder). The dead onChange + subscription (+ its fingerprint fold) was removed: models has no live-render + subscriber (the console repaints via its direct models_changed handler), so it + exposes no onModelsChange.""" src = _MODELS_JS.read_text(encoding="utf-8") for field in ("default_alias", "judge_default_alias", "coordinator_default_alias"): assert field in src, f"models.js must carry {field} for the two server schemas" assert "window.TurnstoneModels" in src, "models.js must install the classic bridge" assert "makeListCache" in src, "models.js must build on the shared list_cache core" - assert "fpExtra" in src, "models.js must fold the default aliases into the fingerprint (R6)" + assert "onModelsChange" not in src, ( + "models.js must not expose the dead onModelsChange subscription (no subscribers)" + ) def test_skills_cache_returns_raw_rows() -> None: diff --git a/turnstone/console/static/app.js b/turnstone/console/static/app.js index e42c6642..0946e610 100644 --- a/turnstone/console/static/app.js +++ b/turnstone/console/static/app.js @@ -5,16 +5,25 @@ window.onLoginSuccess = function () { if (typeof _refreshHomeComposerVisibility === "function") { _refreshHomeComposerVisibility(); } - // Re-warm the home-composer skill AND model pickers now that auth has - // landed. The initial page-load pass runs before login completes, so - // /v1/api/skills and /v1/api/models 401 (fail-open: the caches keep their - // empty state); without this re-run the dropdowns stay at their placeholders - // until a reload (models used to only recover on a chance models_changed). + // Re-warm ALL FOUR home-composer caches now that auth has landed. The initial + // page-load pass runs before login completes, so /v1/api/{skills,models, + // projects,personas} all 401 (fail-open: the caches keep their empty state); + // without this re-run the launcher dropdowns — and the rail's group-by-project + + // the saved-coordinator project/persona columns — stay empty until a reload. + // {force:true} so a still-in-flight failing pre-auth fetch yields a trailing + // AUTHENTICATED refetch rather than coalescing onto the 401 (skills/personas + // have no *_changed event to recover otherwise). if (typeof _refreshAndPopulateSkills === "function") { - _refreshAndPopulateSkills(); + _refreshAndPopulateSkills({ force: true }); } if (typeof _refreshAndPopulateModels === "function") { - _refreshAndPopulateModels(); + _refreshAndPopulateModels({ force: true }); + } + if (typeof _refreshAndPopulateProjects === "function") { + _refreshAndPopulateProjects({ force: true }); + } + if (typeof _refreshAndPopulatePersonas === "function") { + _refreshAndPopulatePersonas({ force: true }); } // Active-coordinators list is SSE-driven via the console pseudo-node // (#9) — no poller to restart after login. The home-view renderer @@ -1492,7 +1501,7 @@ function _ensureHomeComposerInit() { // the saved-list / rail labels) then repaint the launcher's Persona picker. // Safe when the bridge is absent (module still loading): the picker keeps // its "Default" placeholder, which the server resolves to the kind default. -function _refreshAndPopulatePersonas() { +function _refreshAndPopulatePersonas(callOpts) { const TP = window.TurnstonePersonas; if (!TP) return; // Paint from the warm cache SYNCHRONOUSLY first so the Persona picker isn't @@ -1501,7 +1510,7 @@ function _refreshAndPopulatePersonas() { // PersonaDropdown preserves a mid-window pick and only applies the kind default // when nothing valid is selected, so the second paint can't clobber a choice. _populateHomePersonaDropdown(); - TP.refreshPersonas().then(_populateHomePersonaDropdown); + TP.refreshPersonas(callOpts).then(_populateHomePersonaDropdown); } // Populate the launcher's Persona picker for the ACTIVE kind, preselecting @@ -1532,7 +1541,7 @@ function _populateHomePersonaDropdown() { // rail's group-by-project) then repaint the launcher's Project picker. Safe // when the bridge is absent (project.read denied / module still loading): the // picker simply keeps its "No project" placeholder. -function _refreshAndPopulateProjects() { +function _refreshAndPopulateProjects(callOpts) { const TP = window.TurnstoneProjects; if (!TP) return; // Sync paint from the warm cache first (the launcher keeps its "No project" @@ -1541,7 +1550,7 @@ function _refreshAndPopulateProjects() { // console launcher intentionally does NOT gate on requireProject() (§8) — the // node create endpoint is the authoritative gate. _populateHomeProjectDropdown(); - TP.refreshProjects().then(_populateHomeProjectDropdown); + TP.refreshProjects(callOpts).then(_populateHomeProjectDropdown); } // Populate the launcher's Project picker from the shared cache, preserving the @@ -1731,11 +1740,11 @@ function _mountHomeCoordComposer() { // then refresh-and-repaint to catch a skill created elsewhere. Safe when the // bridge is absent (module still loading / 401 pre-auth — onLoginSuccess re-runs // this once auth lands). -function _refreshAndPopulateSkills() { +function _refreshAndPopulateSkills(callOpts) { const TS = window.TurnstoneSkills; if (!TS) return; _populateHomeSkillDropdown(); - TS.refreshSkills().then(_populateHomeSkillDropdown); + TS.refreshSkills(callOpts).then(_populateHomeSkillDropdown); } // Populate the launcher's Skill picker from the shared cache, preserving the @@ -1746,6 +1755,11 @@ function _populateHomeSkillDropdown() { if (!_homeCoordComposer) return; const TS = window.TurnstoneSkills; if (!TS) return; + // Reads the shared skills cache, which is FAIL-OPEN by design: a transient + // non-OK refresh keeps the last-known rows rather than blanking to the + // placeholder (main blanked via `r.ok ? json : {skills:[]}`; this matches the + // projects/personas policy). A since-removed skill is rejected server-side on + // launch — a narrow stale-selection window traded for not blanking on a blip. const previous = _homeCoordComposer.getOptionValue("skill"); const choices = TS.getSkills().map(function (t) { return { diff --git a/turnstone/shared_static/list_cache.js b/turnstone/shared_static/list_cache.js index 9a95f70a..8f71ef41 100644 --- a/turnstone/shared_static/list_cache.js +++ b/turnstone/shared_static/list_cache.js @@ -30,12 +30,6 @@ import { authFetch } from "./auth.js"; * @param {(row:object)=>any} opts.fpRow per-row fingerprint tuple — the fields * subscribers render, so a refresh that * returns identical data skips the fan-out - * @param {(extra:object)=>any} [opts.fpExtra] extra top-level fields to fold - * into the fingerprint (e.g. models' default - * aliases, so a default-only change still - * fires onChange). Omit to fingerprint rows - * only (a `captureExtra` value that changes - * without a row change then does NOT fire). * @param {(data:object)=>object} [opts.captureExtra] pull extra top-level * response fields into cache state on a * successful refresh (returns the new @@ -64,7 +58,6 @@ export function makeListCache(opts) { const name = opts.name; const keyField = opts.keyField || null; const fpRow = opts.fpRow; - const fpExtra = opts.fpExtra || null; const captureExtra = opts.captureExtra || null; const extraDefaults = opts.extraDefaults || null; // Default true: an advisory that gates UI (projects' require_project) must fail @@ -88,10 +81,7 @@ export function makeListCache(opts) { // (it escapes anything inside a string field) and needs no separator // chars; an earlier per-module version joined on raw control bytes, // which made git see the whole file as binary. - return JSON.stringify([ - _cache.map(fpRow), - fpExtra ? fpExtra(_extra) : null, - ]); + return JSON.stringify(_cache.map(fpRow)); } function _setCache(rows) { diff --git a/turnstone/shared_static/models.js b/turnstone/shared_static/models.js index 49222be8..4f807a20 100644 --- a/turnstone/shared_static/models.js +++ b/turnstone/shared_static/models.js @@ -32,19 +32,12 @@ const _core = makeListCache({ url: "/v1/api/models", dataKey: "models", name: "models", + // alias is the unique s from the shared models cache. One list // feeds BOTH selects with the same "alias (model)" labels; each placeholder is // annotated with the server-resolved default alias ("Default — gpt-5"), resolved @@ -728,8 +749,13 @@ function submitNewWs() { const initEl = document.getElementById("new-ws-initial-message"); const initial_message = initEl ? initEl.value.trim() : ""; if (name) body.name = name; - // Forks inherit their source's model + judge (the selects are hidden for a - // fork), so never override them — matches the skill/persona/project fork guards. + // Forks DELIBERATELY inherit their source's model + judge: the selects are + // hidden for a fork (showNewWsModal) and never sent here — matching the + // skill/persona/project fork guards. A fork resumes the source session + // (resume_ws), which already carries its model, so there is intentionally no + // fork model/judge override. Do NOT drop these !_forkFromWsId guards without + // also un-hiding the selects (a bare gate-removal ships a hidden-select's stale + // value). if (model && !_forkFromWsId) body.model = model; if (judge_model && !_forkFromWsId) body.judge_model = judge_model; if (skill && !_forkFromWsId) body.skill = skill; @@ -1514,17 +1540,11 @@ function _loadDashboardOptionsLists() { const projHint = projLabel ? projLabel.querySelector(".label-hint") : null; _paintProjectPicker(projSel, projHint, { fork: false }); - // Persona picker — same paint-from-cache-then-refresh policy; kind default - // preselected so a zero-touch launch behaves exactly like today. + // Persona picker — via the shared wrapper; the dashboard is a persistent panel + // so freshOnOpen:false (preserve a pick across a repaint), kind default when + // nothing valid is selected. const personaSel = document.getElementById("dashboard-persona"); - if (personaSel && window.TurnstonePersonas) { - // Sync paint first, then refresh-and-repaint; the helper preserves a mid- - // window pick and only applies the kind default when nothing valid is chosen. - _populatePersonaSelect(personaSel); - window.TurnstonePersonas.refreshPersonas().then(function () { - _populatePersonaSelect(personaSel); - }); - } + _paintPersonaSelect(personaSel, { freshOnOpen: false }); } // localStorage key for the dashboard composer's Options-panel disclosure