From 079257967d0edf63ea56e07fabd14837377d3022 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sat, 18 Jul 2026 17:11:41 -0700 Subject: [PATCH] refactor(ui): extract _paintProjectPicker (dedupe modal/dashboard) Review of #868 flagged the sync-paint + refresh + required/optional hint block as copy-pasted between showNewWsModal and _loadDashboardOptionsLists, already diverging structurally, so a future tweak could drift and silently re-introduce the FOUC on the missed surface. Collapse both into a shared _paintProjectPicker(sel, hint, {fork}) -- the modal passes the fork flag, the dashboard never forks. Guards re-pointed at the helper + a new one pins its sync-before-async pattern. --- tests/test_app_js.py | 49 ++++++++++++++---------- turnstone/ui/static/app.js | 76 +++++++++++++------------------------- 2 files changed, 56 insertions(+), 69 deletions(-) diff --git a/tests/test_app_js.py b/tests/test_app_js.py index fe7cd266..5fb1fe1f 100644 --- a/tests/test_app_js.py +++ b/tests/test_app_js.py @@ -2407,15 +2407,10 @@ def test_new_ws_modal_paints_project_and_persona_from_cache_synchronously() -> N these guards pin only that a synchronous populate precedes it.""" body = _APP_JS.read_text(encoding="utf-8") fn = _slice_top_level_fn(body, "function showNewWsModal(") - # The FIRST _populateProjectSelect(projSelect, …) is the sync paint; it must - # precede the refreshProjects().then(…) async repaint. - sync_proj = fn.find("_populateProjectSelect(projSelect") - async_proj = fn.find("refreshProjects().then") - assert sync_proj >= 0, "the modal must paint the project picker from cache synchronously" - assert async_proj >= 0, "the modal must keep refreshProjects().then (catches remote changes)" - assert sync_proj < async_proj, ( - "the synchronous project paint must precede the async refreshProjects().then " - "repaint — otherwise the picker flashes empty on every open" + # Project is painted via the shared _paintProjectPicker helper (fork-gated); + # the sync-before-async pattern is pinned in test_paint_project_picker_syncs. + 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") @@ -2434,12 +2429,9 @@ def test_dashboard_paints_project_and_persona_from_cache_synchronously() -> None from requireProject() synchronously, matching the new-ws modal.""" body = _APP_JS.read_text(encoding="utf-8") fn = _slice_top_level_fn(body, "function _loadDashboardOptionsLists(") - sync_proj = fn.find("_populateProjectSelect(projSel") - async_proj = fn.find("refreshProjects().then") - assert sync_proj >= 0, "the dashboard must paint the project picker from cache synchronously" - assert async_proj >= 0, "the dashboard must keep refreshProjects().then" - assert sync_proj < async_proj, ( - "the synchronous dashboard project paint must precede the async refresh repaint" + # Project is painted via the shared _paintProjectPicker helper. + 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") @@ -2455,10 +2447,7 @@ def test_dashboard_paints_project_and_persona_from_cache_synchronously() -> None assert 'class="label-hint"' in html[lbl : lbl + 120], ( "the dashboard Project label must carry a .label-hint span (required/optional cue)" ) - sync_hint = fn.find("projHint.textContent") - assert 0 <= sync_hint < async_proj, ( - "the dashboard project label hint must be seeded synchronously (before the refresh)" - ) + # The hint is seeded synchronously inside _paintProjectPicker (asserted there). def test_console_launcher_paints_project_and_persona_from_cache_synchronously() -> None: @@ -2485,3 +2474,25 @@ def test_console_launcher_paints_project_and_persona_from_cache_synchronously() assert sync_persona < async_persona, ( "the synchronous launcher persona paint must precede the async refresh repaint" ) + + +def test_paint_project_picker_syncs_before_refresh() -> None: + """The shared _paintProjectPicker (used by BOTH the modal and dashboard, so + the two can't drift and silently re-introduce the FOUC) seeds the required/ + optional hint + paints the picker via _populateProjectSelect SYNCHRONOUSLY, + then refreshes-and-repaints. It skips a fork, and both paints route through + the same _populateProjectSelect (preserving the #867 strict-picker invariant).""" + body = _APP_JS.read_text(encoding="utf-8") + fn = _slice_top_level_fn(body, "function _paintProjectPicker(") + assert "_populateProjectSelect(" in fn, ( + "_paintProjectPicker must paint via _populateProjectSelect" + ) + assert "hint.textContent" in fn, "_paintProjectPicker must seed the required/optional hint" + assert "opts.fork" in fn, "_paintProjectPicker must skip for a fork" + sync_call = fn.find("paint();") + async_refresh = fn.find("refreshProjects().then") + assert async_refresh >= 0, "_paintProjectPicker must keep refreshProjects().then" + assert 0 <= sync_call < async_refresh, ( + "_paintProjectPicker must paint synchronously (paint()) BEFORE the async " + "refreshProjects().then repaint" + ) diff --git a/turnstone/ui/static/app.js b/turnstone/ui/static/app.js index 5e6cb7c1..7b0019b1 100644 --- a/turnstone/ui/static/app.js +++ b/turnstone/ui/static/app.js @@ -414,33 +414,11 @@ function showNewWsModal(forkFromWsId) { const projHint = projLabel ? projLabel.querySelector(".label-hint") : null; if (projLabel) projLabel.hidden = !!_forkFromWsId; if (projSelect) projSelect.hidden = !!_forkFromWsId; - // Seed the hint from the warm cache SYNCHRONOUSLY so a fresh create isn't - // mislabeled "optional" for the duration of the (redundant) refresh round-trip - // — requireProject() reads a cache the rail warms at startup. The refresh below - // re-affirms it for the rare cold-cache open. - if (projHint && !_forkFromWsId && window.TurnstoneProjects) { - projHint.textContent = window.TurnstoneProjects.requireProject() - ? "required" - : "optional"; - } - if (projSelect && !_forkFromWsId && window.TurnstoneProjects) { - // Paint from the warm cache SYNCHRONOUSLY first so the picker isn't empty for - // the (redundant) refresh round-trip — the rail warms projects at startup and - // projectChoices()/requireProject() are sync. On a cold cache projectChoices() - // is [] and this is a no-op the async fills, so it's never worse than before. - // BOTH paints route through the SAME _populateProjectSelect, so the #867 - // strict-picker invariant (never auto-select a real project) holds identically. - _populateProjectSelect(projSelect, { - requireProject: !!window.TurnstoneProjects.requireProject(), - }); - window.TurnstoneProjects.refreshProjects().then(function () { - const strict = !!window.TurnstoneProjects.requireProject(); - // Honest label: under require_project a fresh create must resolve to a real - // project, so the static "optional" hint must not claim otherwise. - if (projHint) projHint.textContent = strict ? "required" : "optional"; - _populateProjectSelect(projSelect, { requireProject: strict }); - }); - } + // Paint the picker + its required/optional hint from the warm cache, then + // refresh-and-repaint — shared with the dashboard via _paintProjectPicker so + // the two can't drift; skips for a fork (its picker is hidden above, + // inheritance is server-enforced). + _paintProjectPicker(projSelect, projHint, { fork: !!_forkFromWsId }); // Persona picker — hidden when forking (a fork resumes the source's // stamped persona; the create handler skips resolution on resume_ws). @@ -571,6 +549,24 @@ function _optionExists(sel, val) { return false; } +// Paint a FRESH-create project picker (+ its required/optional label hint) from +// the warm cache SYNCHRONOUSLY, then refresh-and-repaint. Shared by the new-ws +// modal and the dashboard composer so the two paths can't drift and silently +// re-introduce the empty-dropdown / mislabel FOUC this exists to prevent. A fork +// skips entirely (a fork inherits its source's project server-side; the modal +// hides the picker for forks). Both paints reuse _populateProjectSelect, so the +// #867 strict-picker invariant (never auto-select a real project) holds on each. +function _paintProjectPicker(sel, hint, opts) { + if ((opts && opts.fork) || !sel || !window.TurnstoneProjects) return; + const paint = function () { + const strict = !!window.TurnstoneProjects.requireProject(); + if (hint) hint.textContent = strict ? "required" : "optional"; + _populateProjectSelect(sel, { requireProject: strict }); + }; + paint(); // sync from the warm cache (no-op when cold; the async fills it) + window.TurnstoneProjects.refreshProjects().then(paint); +} + // Fill a project