Motion plan preview - #908
Conversation
🦋 Changeset detectedLatest commit: 0395163 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
21cd300 to
4379158
Compare
1ff2f11 to
42e0107
Compare
42e0107 to
a179f19
Compare
a179f19 to
0ebe64d
Compare
0ebe64d to
b7fc733
Compare
b7fc733 to
a879ef0
Compare
a879ef0 to
32a6575
Compare
32a6575 to
06e5668
Compare
cf5bb77 to
3dd9843
Compare
3dd9843 to
fd1d090
Compare
fd1d090 to
cafb4e5
Compare
cafb4e5 to
364e279
Compare
364e279 to
8a9cb28
Compare
|
Claude (@claude) review |
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 4m 53s —— View job Review
SummaryNo bugs found. The design is clean and the core execute paths are correct. Three test gaps worth closing before merge. Correctness
Test gaps (see inline comments)
Positives worth calling out
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| await change(user) | ||
|
|
||
| expect(invalidationKey()).not.toEqual(before) | ||
| }) |
There was a problem hiding this comment.
The it.each covers 4 of the 5 inputs listed in invalidateOn. The service name (service) is included in the array but has no corresponding test case here. The PR description explicitly calls this out as a reason for the design ("switching motion service changes who answers"), so it's worth covering.
A fifth entry would look like:
{
input: 'the motion service changes',
change: async (user: UserEvent) => {
// Re-mount with a second service in the list so the combobox has an alternative to select.
// Would require withBuiltinService() to expose two services: [service('builtin'), service('planner')]
// then simulate selecting 'planner'.
},
},The tricky part is that withBuiltinService() only returns one service, so selecting a different one requires a two-service setup. Still worth adding — this is the only input without test coverage.
|
|
||
| await waitFor(() => expect(preview.clear).toHaveBeenCalled()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Missing test: the PR description says "Re-plan & execute clears up front so that committing to a move also cancels any plan still in flight." But there is no case that:
- Sets
preview.status = 'ready' - Clicks the "Re-plan & execute" button (relabeled from "Execute move")
- Asserts
preview.clearwas called beforeclient.move
The existing executeMove tests in the base branch never have a ready preview, so this interaction is uncovered. Adding a describe('with executeMove while a preview is ready') block would close the gap and protect the preview.clear() call on line 288 of MoveControls.svelte.
| expect(screen.getByRole('slider', { name: 'arm preview step' })).toBeInTheDocument() | ||
| expect(screen.getByRole('button', { name: /re-plan preview/i })).toBeInTheDocument() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The planning status is untested. It renders the plan button with progress="indeterminate" (the spinner) while disabling it — a transient state but a user-visible one. A simple case would verify both:
it('shows planning progress while a plan is in flight', () => {
render(MovePreview, {
props: { preview: preview({ status: 'planning' }), frameName: 'arm' },
})
const button = screen.getByRole('button', { name: /preview move/i })
expect(button).toHaveAttribute('aria-disabled', 'true')
// The progress indicator: prime's Button renders aria-busy while progress is set.
// Adjust to whatever attribute prime-core exposes.
})Not blocking, but the current suite goes from idle straight to ready and error, leaving the in-between state undocumented.
8a9cb28 to
0395163
Compare
0395163 to
6bc8ada
Compare
1 participant
Move mode can now show you a move before it runs: stage a goal on the gizmo, ask the motion service to plan it, and scrub the resulting trajectory as ghost geometry to see the path the machine would actually take. When the path looks right,
Execute previewruns that exact trajectory instead of planning a fresh one. Stacks on #929, which supplies the preview lifecycle this panel mounts.Stack
$lib/motion(make motion utils reusable #917)MoveFramepluginFrontend
MovePreview.svelteis the panel's preview section: the plan button, the error and already-at-goal messages, the approximation banner, and the trajectory scrubber.MoveControls.sveltemounts it, wiresusePreviewMoveup with aninvalidateOnkey naming every input the plan was computed from, and gains anExecute previewbutton beside a relabeledRe-plan & execute.moveExecutionOwner.svelte.tsnames the single frame whose move is currently running, across every open panel. It is the twin ofmoveGizmoOwner.Why?
Why does the panel need its own execution lock?
Because
executeis notMove.builtIn.Moveopens withoperation.CancelOtherWithLabel(ctx, builtinOpLabel)inservices/motion/builtin/builtin.go, so twoclient.movecalls arbitrate themselves inside RDK.builtIn.DoCommanddoes neither: no operation label, and only a read lock. Move mode renders a panel per selected frame and the execute buttons are not gated on owning the gizmo, so selecting an arm and a gripper mounted on it and executing both would batchGoToInputsfor the same arm from two different trajectories. The server structurally cannot arbitrate this, so the UI has to. It holds a frame name rather than a boolean so a panel can tell "I am the one moving" from "someone else is": the first shows progress, the second disables.Why does re-planning trigger on more than the goal moving?
Because the plan depends on more than the goal. Editing the world state JSON adds an obstacle; editing constraints changes what counts as a valid path; switching motion service changes who answers; and a config revision replaces the kinematics the ghosts are drawn through. All of them make the displayed trajectory wrong in a way the user cannot see, and editing the world state to describe an obstacle the preview just revealed is the whole reason the field exists, so
invalidateOnnames all five.Why does
Execute previewnot replan?That is the point: the preview is what is being approved.
executeCommandarms RDK's own start-state guard, so a component that has drifted away from the configuration the plan begins at refuses rather than flying a path nothing validated. The world it was planned against is still a snapshot, and a dynamic obstacle that has moved since is invisible to both sides, which is whyRe-plan & executestays available alongside it.Why clear the preview when a move fails?
A failed
executeis not a move that never happened. RDK batches the waypoints to the component and can stop anywhere along them, so whatever configuration the machine is in afterwards is not the one the plan starts from. Leaving the drawing up, withExecute previewarmed over it, offers to re-run a path from a state that no longer exists. The same applies toRe-plan & execute, which clears up front so that committing to a move also cancels any plan still in flight.Why gate the buttons on the client rather than the service name?
They are not the same thing.
useResourceNamesserves names from cache withstaleTime: Infinity, whilecreateResourceClientyieldsundefinedfor as long as the connection is notCONNECTED. On a dropped socket the service is set and the client is not, so gating on the name alone leaves the button lit while clicking it does nothing at all: no spinner, no error, no state change.Why does the ready state call the preview an approximation?
Because a plan is a validated path and nothing more, and every decision about how to fly it is made later, by the component. Plans carry no timing, so the scrubber plays at a fixed rate that is not the speed the machine will move at. And what the planner guarantees is that the path is collision-free, not that the arm traces it exactly:
builtin.executehands the whole waypoint list to the component in one batch precisely so it can decide how to move between the waypoints. Both are true of every plan, always, and nothing has gone wrong when they are, so the banner is styled as information rather than as a warning.Why do an unsupported RDK and an unsupported api report differently?
Because they fail independently, and a robot can satisfy one and not the other. The preview needs RDK v0.101.1 or newer for the
planandexecuteDoCommands on the builtin motion service; older versions return noplankey, which surfaces as "this motion service does not support previewing". It separately needs api v0.1.485 or newer forFrameSystemConfig.kinematicsto carry the model, without which there are no kinematics to run and the preview reports an empty frame system. Collapsing the two into one message would send people looking at the wrong half.executeCheckStart(#927) is honored from RDK v0.101.1 onward and ignored rather than rejected before it, soExecute previewstill runs on an older server, just without the start-state guard.Testing
pnpm exec vitest --runpasses 1114 tests across 84 files, up 18 tests and one file fromfeat/preview-lifecycle.MovePreview.svelte.spec.tsis new. It drives the component off plainPreviewMoveobjects rather than mocking anything, and covers the idle state with neither live region on screen, each error and already-at-goal message throughrole="alert"androle="status", and the ready state's approximation banner and scrubber.MoveControls.svelte.spec.tscovers theExecute previewpath and what it sends, that the start-state check is armed, that a second panel cannot execute while the first move is still running, that a failed execute drops the drawn plan, that a missing client disables the buttons rather than leaving them armed and inert, and that the goal, the world state, the constraints and the frame system each re-key the preview.Each behavior was verified by reverting the line that implements it and confirming the matching test fails.
pnpm exec svelte-checkreports 0 errors and 0 warnings.