Conversation
045b897 to
27e825f
Compare
cetiennec
changed the title
f3eec6a to
84c3b18
Compare
nicolas-rabault
left a comment
nicolas-rabault
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The recording transition checks passed. Please address the startup exit issue before merging; details inline.
| <p className="mt-4 text-sm text-red-400">{startupError}</p> | ||
| )} | ||
| <Button | ||
| onClick={() => navigate("/")} |
There was a problem hiding this comment.
[P2] Stop the session before leaving the startup screen. If /start-recording succeeds but /recording-status fails, this button navigates home without sending /stop-recording. Unmounting only cancels polling, so the recording worker keeps driving the arm and cycling through takes without its recording controls. The same race exists if the start request is still pending when the button is clicked. Make this exit cancel the session, including a late successful start response, and retain a way to stop it if cancellation fails.
There was a problem hiding this comment.
This should be fixed by the last commit, testing this afternoon
nicolas-rabault
left a comment
nicolas-rabault
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The recording transition checks passed. Please address the startup exit issue before merging; details inline.
fc82f52 to
b24e75d
Compare
|
@nicolas-rabault tested on a real robot and adressed the comment |
nicolas-rabault
left a comment
nicolas-rabault
left a comment
There was a problem hiding this comment.
Thanks for following up! Two failure paths still need attention; details inline.
| // This endpoint always answers 200; `success: false` in the body just | ||
| // means there was nothing left to stop (e.g. a retry after it already | ||
| // did) — either way it's now safe to leave. | ||
| await response.json().catch(() => null); |
There was a problem hiding this comment.
[P2] Keep the stop control available after an HTTP error. fetch resolves normally for HTTP 500/502 responses, so this path marks the session inactive and navigates home even when the stop request failed. handleBackToHome has the same issue at lines 548-549. Check response.ok in both paths and route unsuccessful responses through the existing error/retry behavior before navigating.
| navigate("/"); | ||
| } | ||
| } catch (error) { | ||
| activeSessionRef.current = false; |
There was a problem hiding this comment.
[P2] Preserve uncertainty when the start response is lost. A rejected fetch or unreadable response does not prove the server failed to start recording. If the user clicks Back to Home while start is pending and the response then drops after the server accepted it, setting this ref to false makes attemptLeave navigate home without ever sending stop. Keep this state uncertain and retain cancellation/retry until the backend confirms the session is stopped or absent.
b24e75d to
75baeef
Compare
…ing screen Recording jumped straight from *preparing* into writing frames, so the first seconds of episode 1 caught the arm still being moved into position. Every later episode already gets that grace period: the reset phase between episodes runs the control loop with no dataset, so the arm keeps following the leader while nothing is recorded. The first episode simply never had one. It does now — same phase, same reset_time_s, same orange in the UI, same exit-early control to skip it. No new setting: a second duration for what is mechanically the same phase would only mean two orange stretches back to back between episodes. The loop called record_loop in four places with the same eight arguments each time, differing only in `dataset` and `control_time_s`. `loop_kwargs` collects the shared part once; each call site keeps the real `record_loop(**loop_kwargs, ...)` call and states just what differs. (Deliberately not functools.partial: that would leave exactly one `record_loop(` in the file and make the four real call sites ungreppable by name.) The recording screen itself: - episode counter is a boxed chip instead of small gray text - phase limit reads at a glance next to the elapsed time (00:12 / 00:30), with a caption naming what is being timed, so it isn't confused with the session clock - re-record is a real button next to the primary action (DEL or left arrow) rather than a row in the overflow menu - the startup screen says which half of the handshake is stuck instead of spinning "Connecting..." with no way back Both "Back to Home" exits now stop the session instead of abandoning it: the startup screen's (if /start-recording already succeeded, or succeeds after the click while it was still in flight) and the main screen's (once a session is confirmed live). Either fires /stop-recording before navigating; a failed stop keeps the user on the screen with a retry rather than navigating home and losing the only control that could shut down a worker thread left driving the arm unattended. Two follow-up review fixes to that last piece: - /start-recording and /stop-recording both always answer HTTP 200, even on failure (they never raise; failure is only ever a `success: false` body) — so `response.ok` can't tell success from failure, and neither path was checking it before treating a stop as done. Fixed in both the startup and main screens: an HTTP error status now routes through the same catch-driven retry as a network failure, instead of being read as a successful stop. - A rejected /start-recording fetch (or an unparseable response) doesn't prove the server never created a session — the request may have reached it before the *response* was lost in transit. That case now stays "uncertain" rather than being marked safe: it's treated the same as a confirmed-active session, routing through the stop-then-leave path instead of navigating home on a guess. The only outcome trusted as "nothing to stop" is a clean, parsed, HTTP-ok body that says so. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
75baeef to
3db481f
Compare
|
Can you recheck @nicolas-rabault ? |
What
Recording jumped straight from preparing into writing frames, so the first seconds of episode 1 caught the arm still being moved into position.
Every later episode already gets that grace period: the reset phase between episodes runs the control loop with no dataset, so the arm keeps following the leader while nothing is recorded. The first episode simply never had one. It does now — same phase, same
reset_time_s, same orange in the UI, same exit-early control to skip it.No new setting on purpose: a second duration for what is mechanically the same phase would only mean two orange stretches back to back between episodes.
Cleanup
The loop called
record_loopin four places with the same eight arguments each time, differing only indatasetandcontrol_time_s.loop_kwargscollects the shared part once; each call site keeps the realrecord_loop(**loop_kwargs, ...)call and states just what differs. (Deliberately notfunctools.partial: that would leave exactly onerecord_loop(in the file and make the four real call sites ungreppable by name.)Recording screen
00:12 / 00:30), with a caption naming what is being timed, so it isn't confused with the session clockFix (review feedback)
Both "Back to Home" exits used to navigate away unconditionally, leaving an active session's worker thread driving the arm with no UI attached to it:
/start-recordinghad already succeeded — or succeeded after the click while still in flight — leaving abandoned it./stop-recordingfirst.Two follow-up fixes from a second review pass on that:
/start-recordingand/stop-recordingboth always answer HTTP 200, even on failure — soresponse.okcouldn't tell success from failure. Neither path was checking it before treating a stop as done; an HTTP error status now routes through the same catch-driven retry as a network failure./start-recordingfetch doesn't prove the server never created a session (the request may have reached it before the response was lost). That case now stays "uncertain" and is treated as if a session might exist — routed through the stop-then-leave path — rather than being read as safe to skip.Notes
frontend/dist/rebuilt with Node 22, as the Quality workflow requires.🤖 Generated with Claude Code