From 9a6e7a60d6a18c2af10d5d920c013b9cbc24b530 Mon Sep 17 00:00:00 2001 From: P S Kesavan Date: Sun, 6 Sep 2026 02:22:41 +0530 Subject: [PATCH] refactor(ui): one roster component, mounted twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There were three renderings of one embryo list: `renderEmbryoRail` and `renderRoster` in operate.js, plus a count badge in embryos.js. The two in Operate were ~80% the same code, each keeping its own count element — the root of #129, counts disagreeing across surfaces. Worse than the duplication, the action sets differed **arbitrarily**. On Bottom cam you could delete an embryo but not centre on it; on Acquisition you could centre and assign a role but not delete. Nothing about either pane justified the split; it is where each button happened to be added. `panels/roster.js` renders `SharedState.embryos` and dispatches to `OperateManager.roster.*`. The list, the selection and the endpoints stay in operate.js, next to the frame geometry they depend on — same split as the Marking panel, and the reason both can mount in the Atrium's EMBRYOS window where there is no canvas. Actions are declared per mount, so the difference is now a decision: rail remove beside the frame, where you spot a false positive roster role, centre, remove the pre-run review surface JUDGEMENT CALL: I gave Acquisition a delete it did not have. #113 complained that delete sat next to the select target in a mostly-empty list; the wide roster has room to separate them, and being unable to remove a false positive from the surface you review before a run was the odder half of the split. One row style replaces `.op-erow` and `.op-rrow` — near-identical grids differing only in border and column count, because each grew its own actions. 111 lines of CSS out, and the 12 rules that became unreachable are deleted rather than left to rot. `US-13`'s selectors moved with the markup; `.op-empty` stays because the tactic library and run spine still use it. ## A bug this introduced, then fixed `SharedState.set` emits only on a real change, and compares by value. `toggleRole` mutates `emb.role` in place, so publishing `_embryos.slice()` stored an array of the very objects being compared against — the change was invisible and the role button silently did nothing. Caught by driving the toggle in a browser, not by reading it. `structuredClone` now, and a test pins it. Same lesson as #126: never hand out a reference to mutable state. Verified live: both mounts render one list with their declared actions, counts agree, selecting in one reflects in the other, an action click does not also select, the reference embryo is marked, the toggle round-trips, and the empty state names the fix at both mounts with the CTA opt-in. Audit finding 1 in docs/devices-tab-audit.md. Co-Authored-By: Claude Opus 5 (1M context) --- gently/ui/web/static/css/operate.css | 195 ++++++++++-------- gently/ui/web/static/js/operate.js | 190 +++++------------ gently/ui/web/static/js/panels/roster.js | 174 ++++++++++++++++ gently/ui/web/static/js/status-store.js | 3 + gently/ui/web/templates/index.html | 1 + tests/test_roster_is_one_component.py | 85 ++++++++ .../stories/US-13-run-chooser-assign-roles.py | 6 +- 7 files changed, 430 insertions(+), 224 deletions(-) create mode 100644 gently/ui/web/static/js/panels/roster.js create mode 100644 tests/test_roster_is_one_component.py diff --git a/gently/ui/web/static/css/operate.css b/gently/ui/web/static/css/operate.css index ee9192aa..6c6db6fd 100644 --- a/gently/ui/web/static/css/operate.css +++ b/gently/ui/web/static/css/operate.css @@ -124,43 +124,6 @@ flex-direction: column; gap: var(--op-1); } -.op-erail-empty { - padding: var(--op-3); - font-size: 0.72rem; - line-height: 1.5; - color: var(--op-ink-dim); - text-align: center; -} -.op-erow { - display: grid; - grid-template-columns: 1fr auto; - align-items: center; - gap: var(--op-2); - padding: var(--op-2); - border: 1px solid transparent; - border-radius: 5px; - cursor: pointer; -} -.op-erow:hover { background: color-mix(in srgb, var(--op-ink) 6%, transparent); } -.op-erow.is-sel { - border-color: var(--accent); - background: color-mix(in srgb, var(--accent) 12%, transparent); -} -.op-erow:focus-visible { outline: 2px solid var(--accent); outline-offset: 1px; } -.op-erow-main { min-width: 0; display: flex; flex-direction: column; gap: 1px; } -.op-erow-label { font-size: 0.78rem; color: var(--op-ink); font-weight: 600; } -.op-erow-xy { font-family: var(--op-mono); font-size: 0.62rem; color: var(--op-ink-dim); } -.op-erow-del { - border: none; - background: none; - color: var(--op-ink-dim); - cursor: pointer; - font-size: 0.95rem; - line-height: 1; - padding: 2px 5px; - border-radius: 4px; -} -.op-erow-del:hover { color: var(--op-emit); background: color-mix(in srgb, var(--op-emit) 14%, transparent); } /* ── pane shell ─────────────────────────────────────────────────────────── */ .op-pane { @@ -607,8 +570,7 @@ gauge tracks. */ .op-btn:focus-visible, .op-nbtn:focus-visible, .op-segbtn:focus-visible, .op-num-in:focus-visible, .op-sel:focus-visible, .op-gauge-track:focus-visible, -.op-mark-canvas:focus-visible, .op-rrow:focus-visible, .op-libitem:focus-visible, -.op-rrole:focus-visible { +.op-mark-canvas:focus-visible, .op-libitem:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; } @@ -630,51 +592,6 @@ } .op-roster { display: flex; flex-direction: column; gap: var(--op-1); } -.op-rrow { - display: grid; - grid-template-columns: 1fr auto auto auto; - align-items: center; - gap: var(--op-2); - padding: var(--op-2); - border: 1px solid var(--op-rule); - border-radius: 6px; - background: var(--op-panel); - cursor: pointer; - text-align: left; - color: var(--op-ink); - transition: background-color 0.12s ease; -} -.op-rrow:hover { background: var(--bg-hover); } -.op-rrow.is-sel { border-color: var(--accent); } -.op-rlabel { font-size: 0.72rem; font-weight: 500; } -.op-rxy { font-family: var(--op-mono); font-size: 0.64rem; color: var(--op-ink-dim); } -.op-rrole { - font-family: var(--op-ui); - font-size: 0.56rem; - font-weight: 600; - letter-spacing: 0.08em; - text-transform: uppercase; - padding: 2px var(--op-2); - border: 1px solid var(--op-rule); - border-radius: 999px; - background: transparent; - color: var(--op-ink-dim); - cursor: pointer; -} -.op-rrole.is-reference { border-color: var(--accent-purple); color: var(--accent-purple); } -.op-rcenter { - font-family: var(--op-ui); - font-size: 0.62rem; - padding: 3px var(--op-2); - border: 1px solid var(--op-rule); - border-radius: 6px; - background: var(--bg-hover); - color: var(--op-ink); - cursor: pointer; - transition: background-color 0.12s ease; -} -.op-rcenter:hover { background: var(--border); } -.op-rcenter:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; } .op-lib-list { display: flex; flex-direction: column; gap: var(--op-1); } .op-libitem { @@ -1017,3 +934,113 @@ gap: 6px; justify-content: flex-end; } + +/* ── Roster panel ───────────────────────────────────────────────────────── + One row style for static/js/panels/roster.js, at both widths. There used to + be .op-erow and .op-rrow: near-identical grids differing only in border and + column count, because each grew its own action set. The actions are declared + per mount now, so the row only has to flex. */ +.rp-row { + display: grid; + grid-template-columns: 1fr auto; + align-items: center; + gap: var(--op-2); + padding: var(--op-2); + border: 1px solid transparent; + border-radius: 5px; + cursor: pointer; +} + +.rp-row + .rp-row { margin-top: 2px; } +.rp-row:hover { background: color-mix(in srgb, var(--op-ink) 6%, transparent); } + +.rp-row.is-sel { + border-color: var(--accent); + background: color-mix(in srgb, var(--accent) 12%, transparent); +} + +.rp-row:focus-visible { + outline: 2px solid var(--accent); + outline-offset: 1px; +} + +.rp-main { + display: flex; + flex-direction: column; + min-width: 0; + line-height: 1.25; +} + +.rp-label { + font-size: 0.78rem; + color: var(--op-ink); +} + +/* Coordinates are compared between rows, so they line up and stay monospace. */ +.rp-xy { + font-family: 'SF Mono', Monaco, 'Cascadia Code', monospace; + font-size: 0.68rem; + font-variant-numeric: tabular-nums; + color: var(--op-ink-dim); +} + +.rp-acts { + display: flex; + align-items: center; + gap: 4px; +} + +.rp-btn { + font-size: 0.7rem; + padding: 1px 6px; + border: 1px solid var(--op-rule); + border-radius: 4px; + background: var(--op-panel); + color: var(--op-ink); + cursor: pointer; +} + +.rp-btn:hover { border-color: var(--accent); color: var(--accent); } + +/* Destructive, and it sits next to the select target — #113 wanted it + distinguishable rather than merely present. */ +.rp-del { + border-color: transparent; + background: transparent; + color: var(--op-ink-dim); + font-size: 0.9rem; + line-height: 1; + padding: 0 4px; +} + +.rp-del:hover { + border-color: var(--accent-red, #ef4444); + color: var(--accent-red, #ef4444); +} + +/* A reference embryo is not imaged as a subject; it should not look like one. */ +.rp-role.is-reference { + border-color: var(--accent); + color: var(--accent); +} + +.rp-empty { + display: flex; + flex-direction: column; + align-items: flex-start; + gap: 8px; + padding: var(--op-3) var(--op-2); + font-size: 0.74rem; + line-height: 1.4; + color: var(--op-ink-dim); +} + +.rp-cta { + font-size: 0.72rem; + padding: 2px 8px; + border: 1px solid var(--op-rule); + border-radius: 4px; + background: var(--op-panel); + color: var(--op-ink); + cursor: pointer; +} diff --git a/gently/ui/web/static/js/operate.js b/gently/ui/web/static/js/operate.js index 9c8775ea..a6a1cfac 100644 --- a/gently/ui/web/static/js/operate.js +++ b/gently/ui/web/static/js/operate.js @@ -991,41 +991,28 @@ const OperateManager = (function () { // once, as before. The set() is what makes the other copies of this // cursor follow. SharedState.set('selectedEmbryoId', id); - renderRoster(); renderSpimTarget(); renderSingle(); renderEmbryoRail(); + publishRoster(); renderSpimTarget(); renderSingle(); publishRoster(); } // Shared embryo list, left of every instrument surface. Reads the canonical // _embryos (bootstrapped from /api/embryos/current, kept live by // EMBRYOS_UPDATE), so it is the same set on Bottom / SPIM / Acquire and it // survives a refresh. - function renderEmbryoRail() { - const host = $('op-erail-list'); - const count = $('op-erail-count'); - if (count) count.textContent = _embryos.length; - if (!host) return; - host.innerHTML = ''; - if (!_embryos.length) { - const box = document.createElement('div'); - box.className = 'op-erail-empty'; - box.textContent = 'No embryos yet — detect on the bottom camera, then register.'; - host.appendChild(box); - return; - } - _embryos.forEach(emb => { - const xy = resolveXY(emb); - const row = document.createElement('div'); - row.className = 'op-erow' + (emb.id === _selected ? ' is-sel' : ''); - row.tabIndex = 0; - row.dataset.embryo = emb.id; - row.innerHTML = - '' + - `Embryo ${escapeHtml(labelFor(emb))}` + - `${xy ? `${xy.x.toFixed(0)}, ${xy.y.toFixed(0)}` : '—'}` + - '' + - ``; - host.appendChild(row); - }); + // The roster is rendered by panels/roster.js, mounted at each host. There + // used to be publishRoster() and publishRoster() here — the same list, + // twice, each with its own count element and an arbitrarily different set + // of buttons. Publishing is all this file does now. + function publishRoster() { + // structuredClone, not slice(). SharedState only emits on a real change, + // and it compares by value — so publishing a shallow copy of an array + // whose ELEMENTS were mutated in place stores the same objects it is + // comparing against, and the change is invisible. toggleRole mutates + // `emb.role` in place, so the role button silently did nothing. + // + // Same lesson as #126: never hand out a reference to mutable state. + SharedState.set('embryos', structuredClone(_embryos)); + const rail = $('op-erail-count'); if (rail) rail.textContent = _embryos.length; + const ros = $('op-roster-count'); if (ros) ros.textContent = _embryos.length; } async function deleteEmbryo(id) { @@ -1043,44 +1030,12 @@ const OperateManager = (function () { _selected = _embryos.length ? _embryos[0].id : null; SharedState.set('selectedEmbryoId', _selected); } - renderEmbryoRail(); renderRoster(); renderSpimTarget(); renderSingle(); drawMarkers(); + publishRoster(); publishRoster(); renderSpimTarget(); renderSingle(); drawMarkers(); } catch (e) { toastFail(`Delete failed (${why(e)})`); } } - function renderRoster() { - const host = $('op-roster'); - const count = $('op-roster-count'); - if (count) count.textContent = _embryos.length; - if (!host) return; - host.innerHTML = ''; - if (!_embryos.length) { - const box = document.createElement('div'); - box.className = 'op-empty'; - box.innerHTML = 'No embryos marked yet.' + - ''; - host.appendChild(box); - return; - } - _embryos.forEach(emb => { - const xy = resolveXY(emb); - const role = (emb.role && emb.role !== 'unassigned') ? emb.role : 'test'; - const row = document.createElement('div'); - row.className = 'op-rrow' + (emb.id === _selected ? ' is-sel' : ''); - row.tabIndex = 0; - row.dataset.embryo = emb.id; - row.innerHTML = - `Embryo ${escapeHtml(labelFor(emb))}` + - `${xy ? `${xy.x.toFixed(0)}, ${xy.y.toFixed(0)}` : '—'}` + - `` + - ``; - host.appendChild(row); - }); - } - // Roles are read from the canonical embryo list and written through the // endpoint — deliberately NOT mirrored in a local map, which in the old // design drifted from _embryos and needed a reconciliation loop. @@ -1090,7 +1045,7 @@ const OperateManager = (function () { const cur = (emb.role && emb.role !== 'unassigned') ? emb.role : 'test'; const next = cur === 'calibration' ? 'test' : 'calibration'; emb.role = next; - renderRoster(); + publishRoster(); const roles = {}; _embryos.forEach(e => { roles[e.id] = (e.role && e.role !== 'unassigned') ? e.role : 'test'; }); try { await postJSON('/api/embryos/roles', { roles }); } @@ -1282,7 +1237,7 @@ const OperateManager = (function () { acquire: { onEnter() { renderRun(); }, onLeave() {}, - render() { renderRoster(); renderSingle(); }, + render() { publishRoster(); renderSingle(); }, }, }; function stopBottom() { @@ -1321,6 +1276,19 @@ const OperateManager = (function () { if (typeof MarkingPanel !== 'undefined' && $('op-marking-host')) { MarkingPanel.mount('op-marking-host'); } + if (typeof RosterPanel !== 'undefined') { + // Actions declared per mount. The narrow rail sits beside the frame, + // so removing a false positive belongs there; the Acquisition list + // is the pre-run review surface and gets everything. Previously the + // difference was an accident of where each button was added. + if ($('op-erail-list')) { + RosterPanel.mount('op-erail-list', { actions: ['remove'] }); + } + if ($('op-roster')) { + RosterPanel.mount('op-roster', + { actions: ['role', 'centre', 'remove'], emptyAction: 'bottom' }); + } + } if (typeof CameraPanel !== 'undefined') { if ($('op-cam-panel-bottom')) { CameraPanel.mount('op-cam-panel-bottom', { camera: 'bottom', titled: false }); @@ -1347,7 +1315,7 @@ const OperateManager = (function () { if (name === 'spim') mountLightPanel(); PANES[name].onEnter(); PANES[name].render(); - renderEmbryoRail(); + publishRoster(); renderLock(); } @@ -1390,10 +1358,10 @@ const OperateManager = (function () { // Render the shared rail even when the Operate view isn't the active tab, // so switching to it (or refreshing) shows the list immediately rather // than waiting for the next mutation event. - renderEmbryoRail(); + publishRoster(); publishMarking(); if (!_active) return; - renderRoster(); renderSpimTarget(); renderSingle(); + publishRoster(); renderSpimTarget(); renderSingle(); } function wire() { @@ -1423,76 +1391,10 @@ const OperateManager = (function () { if (cl) cl.addEventListener('click', clearMarks); const canvas = $('op-mark-canvas'); if (canvas) canvas.addEventListener('click', onCanvasClick); - // Shared embryo rail: click a row to select (delete button is guarded - // first so removing a false positive doesn't also select it). - const erail = $('op-erail-list'); - if (erail) { - erail.addEventListener('click', (e) => { - const del = e.target.closest('[data-del]'); - if (del) { e.stopPropagation(); deleteEmbryo(del.dataset.del); return; } - const row = e.target.closest('[data-embryo]'); - if (row) selectEmbryo(row.dataset.embryo); - }); - erail.addEventListener('keydown', (e) => { - if (e.key !== 'Enter' && e.key !== ' ') return; - const row = e.target.closest('[data-embryo]'); - if (row) { e.preventDefault(); selectEmbryo(row.dataset.embryo); } - }); - } - - const sp = $('op-spim-toggle'); if (sp) sp.addEventListener('click', toggleSpim); - const cal = $('op-calibrate'); if (cal) cal.addEventListener('click', calibrateSelected); - document.querySelectorAll('[data-gv]').forEach(b => - b.addEventListener('click', () => nudgeGalvo(Number(b.dataset.gv)))); - document.querySelectorAll('[data-pz]').forEach(b => - b.addEventListener('click', () => nudgePiezo(Number(b.dataset.pz)))); - document.querySelectorAll('[data-backoff]').forEach(b => - b.addEventListener('click', backOff)); - - const modes = $('op-modes'); - if (modes) { - modes.addEventListener('click', e => { - const b = e.target.closest('[data-mode]'); - if (b) setMode(b.dataset.mode); - }); - } - const stop = $('op-tl-stop'); - if (stop) { - stop.addEventListener('change', () => { - const w = $('op-tl-condwrap'); - if (w) w.hidden = stop.value === 'manual'; - }); - } - const lib = $('op-lib-list'); - if (lib) { - lib.addEventListener('click', e => { - const b = e.target.closest('[data-lib]'); - if (b) { _selectedLib = b.dataset.lib; loadLibrary(); } - }); - } - const roster = $('op-roster'); - if (roster) { - roster.addEventListener('click', e => { - const r = e.target.closest('[data-role-for]'); - if (r) { e.stopPropagation(); toggleRole(r.dataset.roleFor); return; } - const c = e.target.closest('[data-center]'); - if (c) { - e.stopPropagation(); - const emb = _embryos.find(x => x.id === c.dataset.center); - if (emb) centerOnEmbryo(emb); - return; - } - const g = e.target.closest('[data-goto]'); - if (g) { showPane(g.dataset.goto); return; } - const row = e.target.closest('[data-embryo]'); - if (row) selectEmbryo(row.dataset.embryo); - }); - roster.addEventListener('keydown', e => { - if (e.key !== 'Enter' && e.key !== ' ') return; - const row = e.target.closest('[data-embryo]'); - if (row) { e.preventDefault(); selectEmbryo(row.dataset.embryo); } - }); - } + // Row clicks and keys are the Roster panel's business now — it owns + // the markup, so it owns the handlers. Two listeners on one host, one + // of them looking for buttons the panel no longer emits, is how a + // select fires twice. const start = $('op-run-start'); if (start) start.addEventListener('click', startRun); const pause = $('op-run-pause'); if (pause) pause.addEventListener('click', pauseRun); const stopb = $('op-run-stop'); if (stopb) stopb.addEventListener('click', stopRun); @@ -1524,7 +1426,7 @@ const OperateManager = (function () { SharedState.on('selectedEmbryoId', id => { if (id === _selected) return; // our own publish, already applied _selected = id; - renderRoster(); renderSpimTarget(); renderSingle(); renderEmbryoRail(); + publishRoster(); renderSpimTarget(); renderSingle(); publishRoster(); }); if (typeof ClientEventBus !== 'undefined') { @@ -1596,6 +1498,18 @@ const OperateManager = (function () { // owns the readout and the verbs, not the marking itself. return { activate, deactivate, redraw: drawMarkers, + // panels/roster.js renders SharedState.embryos and calls these. The + // list, the selection and the endpoints stay here. + roster: { + select: id => selectEmbryo(id), + remove: id => deleteEmbryo(id), + centre: id => { + const emb = _embryos.find(e => e.id === id); + if (emb) centerOnEmbryo(emb); + }, + toggleRole: id => toggleRole(id), + goTo: pane => showPane(pane), + }, marking: { detect: () => runDetect(), register: () => confirmMarks(), diff --git a/gently/ui/web/static/js/panels/roster.js b/gently/ui/web/static/js/panels/roster.js new file mode 100644 index 00000000..70060294 --- /dev/null +++ b/gently/ui/web/static/js/panels/roster.js @@ -0,0 +1,174 @@ +/** + * Roster — the embryo list, rendered once and mounted wherever it is needed. + * + * RosterPanel.mount('op-erail-list', { actions: ['remove'] }); + * RosterPanel.mount('op-roster', { actions: ['role', 'centre', 'remove'], + * emptyAction: 'bottom' }); + * + * WHY + * + * There were three renderings of one roster: `renderEmbryoRail` and + * `renderRoster` in operate.js, plus a count badge in embryos.js. The two in + * Operate were ~80% the same code, each keeping its own count element — which + * is the root of #129, counts disagreeing across surfaces. + * + * Worse than the duplication, the action sets differed **arbitrarily**. On + * Bottom cam you could delete an embryo but not centre on it; on Acquisition + * you could centre and assign a role but not delete. Nothing about either pane + * justified the split — it is just where each button happened to be added. + * Actions are now declared per mount, so the difference is a decision someone + * made rather than an accident of history. + * + * WHAT IT DOES NOT OWN + * + * The roster itself. `_embryos`, the selection and every verb stay in + * operate.js, next to the frame geometry and the endpoints they depend on. + * This renders `SharedState.embryos` and calls `OperateManager.roster.*` — the + * same split as the Marking panel, and the reason both can mount in the + * Atrium's EMBRYOS window where there is no canvas. + */ +const RosterPanel = (() => { + 'use strict'; + + const mounts = new Map(); // hostId -> opts + + // label, then a cell per action. One row style at both widths — the narrow + // rail simply declares fewer actions than the Acquisition pane. + const ACTIONS = { + remove: { + cls: 'rp-del', + text: '×', + title: 'Remove this embryo (false positive)', + verb: 'remove', + }, + centre: { + cls: 'rp-centre', + text: 'Centre', + title: 'Centre the stage on this embryo', + verb: 'centre', + }, + // Label and styling depend on the row, so this one is built by render(). + role: { cls: 'rp-role', verb: 'toggleRole' }, + }; + + function mount(hostId, opts) { + mounts.set(hostId, { + actions: (opts && opts.actions) || [], + emptyAction: opts && opts.emptyAction, + }); + if (mounts.size === 1) { + SharedState.on('embryos', render); + SharedState.on('selectedEmbryoId', render); + } + render(); + } + + function unmount(hostId) { mounts.delete(hostId); } + + const verbs = () => + (typeof OperateManager !== 'undefined' && OperateManager.roster) || null; + + function esc(s) { + return String(s == null ? '' : s).replace(/[&<>"]/g, c => + ({ '&': '&', '<': '<', '>': '>', '"': '"' }[c])); + } + + /** Fine position beats coarse, same order operate.js resolves in. */ + function xyOf(emb) { + const f = emb && emb.position_fine; + if (f && Number.isFinite(f.x) && Number.isFinite(f.y)) return f; + const c = emb && emb.position_coarse; + if (c && Number.isFinite(c.x) && Number.isFinite(c.y)) return c; + return null; + } + + function labelOf(emb) { + const m = emb && emb.id && String(emb.id).match(/(\d+)/); + return m ? m[1] : '?'; + } + + function render() { + const embryos = SharedState.get('embryos') || []; + const selected = SharedState.get('selectedEmbryoId'); + + mounts.forEach((opts, hostId) => { + const host = document.getElementById(hostId); + if (!host) return; + host.innerHTML = embryos.length + ? embryos.map(e => row(e, selected, opts)).join('') + : empty(opts); + wire(host); + }); + } + + function empty(opts) { + // The actionable empty state used to live only on Acquisition, so the + // pane you were already on described the fix without offering it. + // Every mount can now offer the way forward. + const cta = opts.emptyAction === 'bottom' + ? '' + : ''; + return `
No embryos yet — detect on the bottom camera, + then register.${cta}
`; + } + + function row(emb, selected, opts) { + const xy = xyOf(emb); + const role = (emb.role && emb.role !== 'unassigned') ? emb.role : 'test'; + const isRef = role === 'calibration'; + + const buttons = opts.actions.map(name => { + const a = ACTIONS[name]; + if (!a) return ''; + if (name === 'role') { + return ``; + } + return ``; + }).join(''); + + return `
+ + Embryo ${esc(labelOf(emb))} + ${xy ? `${xy.x.toFixed(0)}, ${xy.y.toFixed(0)}` : '—'} + + ${buttons} +
`; + } + + function wire(host) { + host.onclick = e => { + const v = verbs(); + // An action button first: it sits inside the row, and a click on it + // must not also select. This was a live bug on the old rail. + const act = e.target.closest('[data-verb]'); + if (act) { + e.stopPropagation(); + const fn = v && v[act.dataset.verb]; + if (typeof fn === 'function') fn(act.dataset.id); + return; + } + const goto = e.target.closest('[data-goto]'); + if (goto) { if (v && v.goTo) v.goTo(goto.dataset.goto); return; } + const row = e.target.closest('[data-embryo]'); + if (row && v && v.select) v.select(row.dataset.embryo); + }; + host.onkeydown = e => { + if (e.key !== 'Enter' && e.key !== ' ') return; + const row = e.target.closest('[data-embryo]'); + if (!row) return; + e.preventDefault(); + const v = verbs(); + if (v && v.select) v.select(row.dataset.embryo); + }; + } + + return { mount, unmount, render, ACTIONS }; +})(); + +if (typeof module !== 'undefined' && module.exports) module.exports = RosterPanel; diff --git a/gently/ui/web/static/js/status-store.js b/gently/ui/web/static/js/status-store.js index c112e7d2..72f4adaa 100644 --- a/gently/ui/web/static/js/status-store.js +++ b/gently/ui/web/static/js/status-store.js @@ -98,6 +98,9 @@ const SharedState = (() => { // panels/marking.js — pending marks and a registered roster are two // different counts and were being shown as one. marking: null, + // The embryo roster itself, owned by operate.js and rendered by + // panels/roster.js. One value, so the copies cannot disagree (#129). + embryos: [], }; const subs = new Map(); // key -> Set diff --git a/gently/ui/web/templates/index.html b/gently/ui/web/templates/index.html index 96cf0693..03367cb9 100644 --- a/gently/ui/web/templates/index.html +++ b/gently/ui/web/templates/index.html @@ -1434,6 +1434,7 @@

Acquired + diff --git a/tests/test_roster_is_one_component.py b/tests/test_roster_is_one_component.py new file mode 100644 index 00000000..8d13cb34 --- /dev/null +++ b/tests/test_roster_is_one_component.py @@ -0,0 +1,85 @@ +"""The embryo roster is rendered once, and published as an independent copy. + +There were three renderings of one list: `renderEmbryoRail` and `renderRoster` +in operate.js — ~80% the same code, each with its own count element, which is +the root of #129 — plus a badge in embryos.js. Worse than the duplication, the +action sets differed arbitrarily: delete on Bottom cam, Centre and role on +Acquisition, for no reason either pane justified. + +Both are now `panels/roster.js`, mounted twice with actions declared per mount. + +The second assertion here is the more interesting one, and it is a bug this +change introduced and then fixed. `SharedState.set` only emits on a real change +and compares by value. `toggleRole` mutates `emb.role` **in place**, so +publishing `_embryos.slice()` stored an array of the very objects being +compared against — the change was invisible and the role button silently did +nothing. Same lesson as #126: never hand out a reference to mutable state. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +JS = Path(__file__).resolve().parents[1] / "gently" / "ui" / "web" / "static" / "js" + + +def test_operate_no_longer_renders_the_roster_itself() -> None: + src = (JS / "operate.js").read_text(encoding="utf-8") + for gone in ("function renderEmbryoRail", "function renderRoster"): + assert gone not in src, ( + f"{gone} is back — the roster is a panel now, and two renderers of " + "one list is what #129 is about" + ) + + +def test_the_published_snapshot_is_independent() -> None: + src = (JS / "operate.js").read_text(encoding="utf-8") + body = src[src.index("function publishRoster()") :] + body = body[: body.index("\n }")] + + assert "structuredClone" in body, ( + "publishRoster no longer deep-copies. SharedState compares by value and " + "emits only on change, so publishing a shallow copy of elements that are " + "mutated in place stores the objects it is comparing against — and the " + "change becomes invisible. toggleRole mutates emb.role in place." + ) + assert "_embryos.slice()" not in body, "a shallow copy is the bug, not the fix" + + +def test_both_mounts_declare_their_actions() -> None: + """The difference between the two lists must be a decision, not an accident.""" + src = (JS / "operate.js").read_text(encoding="utf-8") + mounts = re.findall(r"RosterPanel\.mount\('([^']+)',\s*\{([^}]*)\}", src, re.S) + hosts = {h for h, _ in mounts} + assert hosts == {"op-erail-list", "op-roster"}, f"unexpected mounts: {hosts}" + for host, opts in mounts: + assert "actions:" in opts, f"{host} mounts the roster without declaring actions" + + +def test_the_panel_dispatches_rather_than_reimplementing() -> None: + """The list, the selection and the endpoints stay in operate.js.""" + panel = (JS / "panels" / "roster.js").read_text(encoding="utf-8") + assert "OperateManager.roster" in panel + # A panel that fetches is a panel that has opinions about endpoints. + assert "fetch(" not in panel, "the roster panel should call verbs, not endpoints" + + src = (JS / "operate.js").read_text(encoding="utf-8") + exported = src[src.index(" roster: {") :] + exported = exported[: exported.index("\n }")] + for verb in ("select", "remove", "centre", "toggleRole", "goTo"): + assert f"{verb}:" in exported, f"operate.js stopped exporting the {verb} verb" + + +def test_every_mount_offers_a_way_out_of_the_empty_state() -> None: + """The actionable empty state used to be on the pane you reached second. + + Bottom cam described the fix without offering it; Acquisition carried the + button. The panel's empty state now names the fix everywhere, and the CTA + is opt-in per mount. + """ + panel = (JS / "panels" / "roster.js").read_text(encoding="utf-8") + empty = panel[panel.index("function empty(opts)") :] + empty = empty[: empty.index("\n }")] + assert "detect on the bottom camera" in empty + assert "emptyAction" in empty diff --git a/tools/ui_crawler/stories/US-13-run-chooser-assign-roles.py b/tools/ui_crawler/stories/US-13-run-chooser-assign-roles.py index dce1d03b..6dfdf995 100644 --- a/tools/ui_crawler/stories/US-13-run-chooser-assign-roles.py +++ b/tools/ui_crawler/stories/US-13-run-chooser-assign-roles.py @@ -24,8 +24,10 @@ async def flow(page, url, rec): modes = await dom_count(page, "#op-modes [data-mode]") start = await exists(page, "#op-run-start") # With nothing marked the roster is empty but the surface stays fully usable. - empty = await dom_count(page, "#op-roster .op-empty") - roles = await dom_count(page, ".op-rrole") + # The roster is one shared panel now (panels/roster.js), so both selectors + # moved with it: .op-empty/.op-rrole were the Acquisition-only markup. + empty = await dom_count(page, "#op-roster .rp-empty") + roles = await dom_count(page, ".rp-role") if not (on_pane and roster and modes and start): rec.gap("acquisition surface is missing the roster or the run-mode chooser")