refactor(ui): one roster component, mounted twice - #157
Merged
pskeshu merged 1 commit intoSep 5, 2026
Conversation
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 gently-project#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. gently-project#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 gently-project#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) <noreply@anthropic.com>
This was referenced Sep 5, 2026
pskeshu
added a commit
that referenced
this pull request
Sep 6, 2026
…thing 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Judgement call, flagged
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 old split. Reverse it by dropping
'remove'from that mount.Audit finding 1. Three renderings of one embryo list:
renderEmbryoRailandrenderRosterin operate.js (~80% the same code, each with its own count element — the root of #129), plus a badge in embryos.js.Worse than the duplication, the action sets differed arbitrarily:
removerole,centre,removeNothing about either pane justified the split — it is where each button happened to be added. Actions are declared per mount now, so the difference is a decision.
panels/roster.jsrendersSharedState.embryosand dispatches toOperateManager.roster.*. The list, selection and endpoints stay in operate.js next to the frame geometry — same split as the Marking panel, and the reason both can mount in the Atrium's EMBRYOS window where there is no canvas.One row style replaces
.op-erowand.op-rrow, which were near-identical grids differing only in border and column count. 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-emptystays because the tactic library and run spine still use it.A bug this introduced, then fixed
SharedState.setemits only on a real change, and compares by value.toggleRolemutatesemb.rolein 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.
structuredClonenow, with a test pinning 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 · the empty state names the fix at both mounts with the CTA opt-in.