Simplify one pilot one robot - #6
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef94afe21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dataChannelProfile: this.controlConfig.splatBatchDataChannel, | ||
| }); | ||
| this.splatPeer.addEventListener("splatbatch", (event) => this.receiveSplatBatch(event.detail)); | ||
| const [pilotChannel] = await Promise.all([this.pilotPeer.negotiate(), this.splatPeer.negotiate()]); |
There was a problem hiding this comment.
When starting any control session that expects reconstructed splat batches, this now waits on SplatBatchPeer.negotiate(), but that peer only listens for a remote datachannel and never creates an offer-side data channel or transceiver before createOffer(). Such offers don't include an SCTP/application m-line, so the server-created ito.splatBatches channel cannot be negotiated; the promise may complete but no splatbatch events arrive, leaving the visual stream permanently stale and eventually pausing pilot input.
Useful? React with 👍 / 👎.
| if was_active: | ||
| await _maybe_await(self.adapter.stop_control()) |
There was a problem hiding this comment.
In the remote backend, if the driver stops responding or rejects driver.control.stop, RemoteRobotAdapter.stop_control() raises here after control_active is cleared but before reconstruction, live WebRTC paths, and the pilot notification are cleaned up. _handle_control_stop has already sent an OK result to the pilot, so this leaves the UI without control.stopped and can keep runtime resources open; adapter stop should be best-effort and cleanup should continue.
Useful? React with 👍 / 👎.
| if candidate.is_dir(): | ||
| candidate = candidate / "index.html" |
There was a problem hiding this comment.
For directory requests, the containment check runs before appending index.html; when ITO_CLIENT_DIR is configurable or writable, client_dir/sub/index.html can be a symlink outside the client tree and read_bytes() will follow it because the appended path is never resolved and checked again. This bypasses the traversal guard specifically for directory-index requests.
Useful? React with 👍 / 👎.
| if not self.control_active: | ||
| return |
There was a problem hiding this comment.
In the remote-driver backend, a driver can send driver.control.start.result and then immediately send its cameraMedia offer; because the pilot-side start handler only sets self.control_active = True after the awaited request resumes, the driver connection can process the already-buffered offer first and this guard silently discards it. That leaves the remote camera path without an answer and the pilot never receives reconstruction output, so start should either mark a pending/active state before awaiting the driver or queue valid offers during startup.
Useful? React with 👍 / 👎.
| """Latch a robot-local stop until an explicit control start.""" | ||
|
|
||
| was_active = self.control_active | ||
| self.control_active = False |
There was a problem hiding this comment.
When a local integration invokes LocalRobotAdapter.emergency_stop() during active control, this flips only the adapter's state; ItoApplication.control_active remains true, so a later pilot control.start takes the already-active fast path and never calls adapter.start_control() to clear the latch. In that emergency-stop recovery path the UI still appears active but pilot input is ignored until some separate stop lifecycle happens.
Useful? React with 👍 / 👎.
| session_id=session_id, | ||
| return | ||
| if not self.control_active: | ||
| self._start_reconstruction() |
There was a problem hiding this comment.
When a configured reconstruction processor raises during construction or start() (for example missing GPU/runtime resources), this call happens before the try that converts start failures into control.start.result errors. The exception escapes the WebSocket handler, leaving the pilot request unanswered and the connection likely closed instead of reporting a displayable start failure and cleaning up the partial runtime.
Useful? React with 👍 / 👎.
| const batch = this.splatScene.applySplatBatch(payload, metadata); | ||
| if (batch) this.freshness?.markFresh(); |
There was a problem hiding this comment.
After a temporary visual timeout, setVisualPaused(true) freezes the splat scene, and applySplatBatch() returns null while frozen. Because freshness is now only marked when a batch is applied, subsequent valid splat batches cannot trigger the fresh event, so pilot input remains paused forever after the first transient reconstruction gap.
Useful? React with 👍 / 👎.
| if (!location?.host) return "ws://localhost:8765/ws"; | ||
| const scheme = location.protocol === "https:" ? "wss:" : "ws:"; | ||
| return `${scheme}//${location.hostname}:8765`; | ||
| return `${scheme}//${location.host}/ws`; |
There was a problem hiding this comment.
Changing the default endpoint to /ws only helps fresh installs; the localStorage key is unchanged, so any pilot who previously saved settings keeps a ws://host:8765 URL without the path. The new server treats WebSocket upgrades to / as a static HTTP request and rejects the handshake, so upgraded clients can no longer connect until storage is manually cleared or edited.
Useful? React with 👍 / 👎.
1 participant
No description provided.