Conversation
multi-channel wip, integrated arm wip
previously described as "modules" but there are so many meaning of modules and it has no self-explaining connotation
…zation - LiquidHandler: replace single `_resource_pickup` with `_resource_pickups` dict keyed by arm index, with backward-compatible property for arm 0 - LiquidHandler: only create head96 trackers when `core96_head_installed` is True - LiquidHandler: serialize head96_state and arm_state (resource name, type, direction, dimensions) for downstream consumers (e.g. visualizer) - LiquidHandler: register _state_updated callback on head/head96 trackers, and fire it on pick_up_resource / drop_resource - LiquidHandler: guard pick_up_resource with RuntimeError if no arm installed - Backends: add `num_arms` attribute to STARBackend, ChatterboxBackend, STARChatterboxBackend (configurable `iswap_installed` param) - TipTracker: propagate state-update callback to tip's volume tracker on commit() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Rick Wierenga <rick_wierenga@icloud.com>
…robot into pr/multi-arm-head96
Resource names flow from the Python backend into innerHTML, allowing vectors like <img src=x onerror=...> to execute arbitrary JS. Use textContent + DOM element creation instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
_handle_state_update_callback wrote to _pending_state_updates from arbitrary threads while _flush_state_updates read it on the event loop. This could lose updates when a thread wrote to the dict between the flush clearing it and resetting _flush_scheduled. Move all dict writes onto the event loop via call_soon_threadsafe so both enqueue and flush are single-threaded. Preserves batching behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
set_state was calling buildResourceTree() which destroys and recreates the entire sidebar DOM on every state change (e.g. a single well volume update). Use the existing updateSidepanelState() for each changed resource instead, which does a targeted DOM update via querySelector. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
setup() was globally enabling tip and volume tracking, silently overriding user settings. This is the caller's responsibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
inspect.signature was called for every method of every resource instance. For a 96-well plate that's 97 × ~50 calls. Take the class as argument instead and cache with lru_cache since all instances share the same methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The string-replace approach (': Infinity' -> ': "Infinity"') could
false-positive on resource names containing that substring. Use
allow_nan=False with a custom default handler instead.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The allow_nan=False + default handler approach doesn't work because
float('inf') is a native float type — json raises ValueError before
the default handler is ever called. Revert to the original string
replacement which handles this correctly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the PyLabRobot Visualizer UI and backend event/serialization to support a richer interface (toolbars/sidepanel, source name + favicon, actuator panels) and reduce UI churn by batching state updates.
Changes:
- Enrich resource serialization with public method signatures and recursively serialized children.
- Add configurable visualizer header name/favicon and an optional “show actuators at start” event.
- Update JS/HTML/CSS for a revamped layout (toolbars, sidepanel/tree, zoom controls) and more defensive state handling.
Reviewed changes
Copilot reviewed 5 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pylabrobot/visualizer/visualizer_tests.py | Updates tests to match new serialization helper and additional startup event. |
| pylabrobot/visualizer/visualizer.py | Adds method-enriched serialization, UI metadata injection (name/favicon), actuator startup event, and batched state updates. |
| pylabrobot/visualizer/vis.js | Updates root rendering workflow, sidepanel/tree updates, snapshots on unassign, and handles new show_actuators event. |
| pylabrobot/visualizer/main.css | Major UI styling additions for new navbar/toolbars/sidepanel and overlays. |
| pylabrobot/visualizer/index.html | Adds favicon link, cache-busting query params, new toolbar/sidepanel layout, and new UI panels/controls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if resource_state is not None: | ||
| state[resource.name] = resource_state | ||
| resource_state = resource.serialize_state() | ||
| if resource_state: |
There was a problem hiding this comment.
if resource_state: will drop valid “empty” states (e.g., {}) which were previously sent (the old code checked is not None). This can cause initial UI state to be missing for resources whose serialized state is intentionally empty. Prefer if resource_state is not None: (or an explicit sentinel check) to preserve empty-but-meaningful states.
| if resource_state: | |
| if resource_state is not None: |
| continue | ||
| basename = os.path.basename(fname) | ||
| if "ipykernel" in fname or fname.startswith("<"): | ||
| return "" |
There was a problem hiding this comment.
Returning "" as soon as an ipykernel/<...> frame is encountered can prematurely abort detection even when a later frame is a real .py caller. This should skip those frames (use continue) rather than returning immediately, so the loop can still find the actual script filename when present.
| return "" | |
| continue |
| <button class="tree-action-btn" id="expand-all-btn" title="Collapse All"> | ||
| <svg width="14" height="18" viewBox="0 0 14 18" fill="none" stroke="#555" stroke-width="1.8" stroke-linecap="round" stroke-linejoin="round"> | ||
| <polyline points="3,3 7,7 11,3"/> | ||
| <polyline points="3,15 7,11 11,15"/> | ||
| </svg> | ||
| </button> |
There was a problem hiding this comment.
The button id (expand-all-btn) conflicts with the tooltip (title="Collapse All"), making behavior/UI intent ambiguous. Align the id and title (and any corresponding handler names) to reflect the actual action.
| <div id="navbar-lh-actuators"></div> | ||
|
|
||
| <div class="status-box"> | ||
| <span onclick="openSocket();" id="status-indicator"></span> |
There was a problem hiding this comment.
A clickable <span> is not keyboard-accessible by default and lacks semantic button roles for assistive technologies. Use a <button type="button"> (preferred) or add role="button", tabindex="0", and key handlers (Enter/Space) plus an accessible name (e.g., aria-label) so reconnect is usable without a mouse.
| <span onclick="openSocket();" id="status-indicator"></span> | |
| <button type="button" onclick="openSocket();" id="status-indicator" aria-label="Connection status, click to reconnect"></button> |
| if self._show_actuators_at_start: | ||
| await self.send_command("show_actuators", {}, wait_for_response=False) |
There was a problem hiding this comment.
New conditional behavior is introduced but tests only cover the default path (startup includes show_actuators). Add a test case that constructs Visualizer(show_actuators_at_start=False) and asserts the event is not emitted, to prevent regressions in startup sequencing.
2 participants
No description provided.