From 1ce2fa711b81d9835c7c4f50b49bbd8cba8c66cb Mon Sep 17 00:00:00 2001 From: P S Kesavan Date: Sun, 6 Sep 2026 07:13:02 +0530 Subject: [PATCH] fix(operate): restore eight controls that were rendering and doing nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My regression, shipped in #157. Consolidating the two roster renderers into one panel, the roster's own click/keydown listeners had to go — the panel owns the markup, so it owns the handlers. I removed them by slicing text between two landmarks, and the end landmark matched too late. The slice took eight unrelated handlers with it: op-spim-toggle start/stop the light-sheet view op-calibrate run the calibration [data-gv] galvo nudges [data-pz] piezo nudges [data-backoff] the interlock banner's "Back off 100 µm" op-modes the run-mode chooser op-tl-stop the stop-condition select op-lib-list pick a saved tactic Every one still rendered and still looked enabled. `[data-backoff]` is the worst of them: a dead button on an interlock banner is pressed at exactly the moment something is already wrong. Nothing caught it. CI runs no JavaScript, and no test asserted that a control was connected to a function, so there was no signal — I found it only by noticing that clicking a run mode did not change the run mode. `tests/test_operate_controls_are_wired.py` closes that. It checks every control inside `wire()` specifically, not merely somewhere in the file: several of these ids are also read elsewhere (the SPIM toggle's label is updated in `applySpim`), so presence proves the control is mentioned, not that anything listens to it. Verified against the broken file — it flags all eight. The helper anchors on `_wired`, which only the top-level `wire()` uses. My first attempt matched "function wire() {" and found the gauge factory's inner one — landmark-matching text is the mistake this file is about, so it is not repeated in it. Verified live: the mode chooser switches panel and run verb, the stop-condition select reveals its value field, and the back-off, galvo and piezo controls are present and bound. Co-Authored-By: Claude Opus 5 (1M context) --- gently/ui/web/static/js/operate.js | 34 +++++++ tests/test_operate_controls_are_wired.py | 121 +++++++++++++++++++++++ 2 files changed, 155 insertions(+) create mode 100644 tests/test_operate_controls_are_wired.py diff --git a/gently/ui/web/static/js/operate.js b/gently/ui/web/static/js/operate.js index d46d3671..093d6606 100644 --- a/gently/ui/web/static/js/operate.js +++ b/gently/ui/web/static/js/operate.js @@ -1476,6 +1476,40 @@ const OperateManager = (function () { // 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. + // + // Everything below was collateral of that deletion: the slice that + // removed the roster's own listeners ran past them, and eight controls + // went dead — including the interlock banner's back-off button. + 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 stopSel = $('op-tl-stop'); + if (stopSel) { + stopSel.addEventListener('change', () => { + const w = $('op-tl-condwrap'); + if (w) w.hidden = stopSel.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 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); diff --git a/tests/test_operate_controls_are_wired.py b/tests/test_operate_controls_are_wired.py new file mode 100644 index 00000000..6103ea50 --- /dev/null +++ b/tests/test_operate_controls_are_wired.py @@ -0,0 +1,121 @@ +"""Every control the Operate surface offers must be wired to something. + +This test exists because of a specific failure. Consolidating the two roster +renderers into one panel, the roster's own click/keydown listeners had to go — +the panel owns the markup, so it owns the handlers. The deletion was done by +slicing text between two landmarks, and the end landmark matched too late. It +took **eight** unrelated handlers with it: + + op-spim-toggle start/stop the light-sheet view + op-calibrate run the calibration + [data-gv] galvo nudges + [data-pz] piezo nudges + [data-backoff] the interlock banner's "Back off 100 µm" + op-modes the run-mode chooser + op-tl-stop the stop-condition select + op-lib-list pick a saved tactic + +Every one of those controls still rendered, still looked enabled, and did +nothing. It shipped. CI runs no JavaScript, and nothing else asserted that a +button was connected to a function, so there was no signal at all — the +regression was found only by noticing that clicking a run mode did not change +the run mode. + +The lesson is narrow and worth pinning: a rendered control that is not wired is +indistinguishable from a working one until someone presses it. On a microscope, +one of these was a safety control. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +WEB = Path(__file__).resolve().parents[1] / "gently" / "ui" / "web" +OPERATE = WEB / "static" / "js" / "operate.js" +INDEX = WEB / "templates" / "index.html" + +# Each control, and the token that proves operate.js reaches for it. Ids are +# looked up with $('id'); attribute hooks are queried by selector. +CONTROLS = { + "op-spim-toggle": "$('op-spim-toggle')", + "op-calibrate": "$('op-calibrate')", + "op-modes": "$('op-modes')", + "op-tl-stop": "$('op-tl-stop')", + "op-lib-list": "$('op-lib-list')", + "op-run-start": "$('op-run-start')", + "op-run-pause": "$('op-run-pause')", + "op-run-stop": "$('op-run-stop')", + "op-detect": "$('op-detect')", + "op-confirm": "$('op-confirm')", + "op-clear": "$('op-clear')", + "op-cam-toggle": "$('op-cam-toggle')", + "[data-gv]": "[data-gv]", + "[data-pz]": "[data-pz]", + "[data-backoff]": "[data-backoff]", + "[data-mode]": "[data-mode]", + "[data-lib]": "[data-lib]", +} + + +def _wire_body() -> str: + """Just `wire()`. + + Scoped deliberately. Several of these ids are also read elsewhere — the + SPIM toggle's label is updated in `applySpim`, for instance — so their mere + presence in the file proves the control is *mentioned*, not that anything + listens to it. `wire()` is where listeners are attached, so that is where + the assertion belongs. + """ + src = OPERATE.read_text(encoding="utf-8") + # Anchored on `_wired`, which only the top-level wire() uses. Matching on + # "function wire() {" finds the gauge factory's inner one first — and + # landmark-matching text is exactly the mistake that caused the bug this + # file is about, so it is not repeated here. + start = src.index("if (_wired) return;") + end = src.index("\n async function ", start) + return src[start:end] + + +def test_every_control_is_wired_in_wire() -> None: + body = _wire_body() + missing = [name for name, token in CONTROLS.items() if token not in body] + assert not missing, ( + "these controls are not wired in wire(), so they render and do " + f"nothing when pressed: {missing}" + ) + + +def test_the_safety_control_is_wired() -> None: + """`[data-backoff]` retracts the objective from the sample. + + Called out separately because it was among the eight, and because a dead + button on an interlock banner is the worst case: it is pressed exactly when + something is already wrong. + """ + src = OPERATE.read_text(encoding="utf-8") + hook = re.search(r"\[data-backoff\][^;]*addEventListener\('click',\s*(\w+)", src, re.S) + assert hook, "[data-backoff] is not bound to a click handler" + handler = hook.group(1) + assert f"function {handler}" in src or f"async function {handler}" in src, ( + f"[data-backoff] is bound to {handler!r}, which is not defined" + ) + + +def test_the_controls_the_markup_offers_are_the_ones_js_knows() -> None: + """A control in the template with no counterpart in operate.js is dead.""" + html = INDEX.read_text(encoding="utf-8") + + # Only the Operate surface; other tabs own their own scripts. + operate = html[html.index('id="devices-view-operate"') : html.index('id="devices-view-map"')] + ids = set(re.findall(r'id="(op-[a-z0-9-]+)"', operate)) + + # Hosts, readouts and containers are written to, not listened on — they are + # legitimately absent from the handler list. + interactive = {i for i in ids if i in CONTROLS} + assert interactive, "no interactive Operate controls found — has the markup moved?" + body = _wire_body() + for i in sorted(interactive): + assert f"$('{i}')" in body, ( + f"{i} exists in the markup but wire() never binds it — it will render and do nothing" + )