apply comment, test and description review for #930

Author: DTCurrie

src/lib/plugins/MoveFrame/MovePreview.svelte

@@ -18,13 +18,6 @@
 	const planning = $derived(preview.status === 'planning')
 	const ready = $derived(preview.status === 'ready')
 
-	/**
-	 * Named for what a frame *is*, not for how processed it looks.
-	 *
-	 * "Raw / Smoothed" read as honest-versus-prettified, which is backwards: nothing is eased or
-	 * rounded, the same path is just sampled more finely. Waypoints leads because it is the plan
-	 * exactly as returned.
-	 */
 	const detailLabels: Record<PreviewDetail, string> = {
 		waypoints: 'Waypoints',
 		interpolated: 'Interpolated',
@@ -94,11 +87,6 @@
 				{/snippet}
 			</ToggleButtons>
 
-			<!--
-				The labels alone cannot carry this, so the counts do. Seeing "2 frames" against "182
-				frames" for the same move is the fastest way to understand what the setting changes,
-				and it makes a sparse plan's sparseness impossible to miss.
-			-->
 			<p class="text-subtle-2">
 				{#if preview.detail === 'waypoints'}
 					{frameCount} frames — one per configuration the planner returned, and nothing between.

src/lib/plugins/MoveFrame/__tests__/usePreviewMove.svelte.spec.ts

@@ -18,17 +18,11 @@ import {
 } from './__fixtures__/previewMoveHarness.svelte'
 
 const dump = parsePlan(planJson)
-/**
- * The rig `plan-gantry.json` (`interpolateTrajectory.spec.ts`) was lifted from: a 40 mm prismatic
- * slide, captured rather than hand-written so the joint is genuinely `type: "prismatic"` in the
- * model JSON and not just labelled that way by a test double.
- */
 const gantryDump = parsePlan(gantryPlanJson)
 
 /**
- * A part's `kinematics` carries the same `ModelConfigJSON` a dump nests under
- * `frames[partName].frame.model`, so a realistic frame system can be lifted out of a fixture instead
- * of hand-written — geometry, joint chain and all.
+ * A part's `kinematics` is the same `ModelConfigJSON` a dump nests under `frames[part].frame.model`,
+ * so a fixture yields a realistic frame system: geometry, joint chain and all.
  */
 const kinematicsFromDump = (source: ParsedPlan, partName: string): Struct => {
 	const entry = source.frames[partName]
@@ -65,23 +59,15 @@ const SHAPELESS = part(
 	} as never)
 )
 
-/**
- * One prismatic joint, real rather than hand-labelled: `gantry-plan.json`'s model has a `carriage`
- * link riding a `type: "prismatic"` joint, so a descriptor built from it is what `jointMotionsOf`
- * would actually see off a machine, not a stand-in built to say `'translational'`.
- */
+/** A captured `type: "prismatic"` joint, not a double hand-labelled `'translational'`. */
 const GANTRY = part('gantry-1', kinematicsFromDump(gantryDump, 'gantry-1'))
 
 /** Two distinct configurations, so it never reads as "already at the goal". */
 const PLAN_REPLY: JsonValue = {
 	plan: [{ 'left-arm': [0, 0, 0, 0, 0, 0] }, { 'left-arm': [1, 0, 0, 0, 0, 0] }],
 }
 
-/**
- * The same 40 mm slide `gantry-plan.json` captures: `gantry-1` moving from 50 to
- * 90.00000000000001, everything else held. Read as radians instead of millimetres, that stroke is
- * over 2291°, which is the gap `interpolatedFrames`'s `motions` argument exists to close.
- */
+/** The 40 mm slide `gantry-plan.json` captures. Read as radians rather than millimetres it is 2291°. */
 const GANTRY_SLIDE: JsonValue = {
 	plan: [{ 'gantry-1': [50] }, { 'gantry-1': [90.00000000000001] }],
 }
@@ -303,13 +289,8 @@ describe('where the ghosts stand before anything is scrubbed', () => {
 })
 
 /**
- * The hook keeps two arrays and they answer different questions. `trajectory` is what the planner
- * said and the only thing `execute` may be handed; `playbackFrames` is that same motion subdivided
- * for the scrubber, which the robot must never be asked to run.
- *
- * Nothing had ever told them apart: returning the playback frames from `get trajectory()` — handing
- * the robot the interpolated ones — passed the whole suite. Subdividing is what makes the two
- * observably different, so this is the first PR in which the distinction can be pinned at all.
+ * `trajectory` is the only thing `execute` may be handed; `playbackFrames` is that same motion
+ * subdivided for the scrubber, which the robot must never be asked to run.
  */
 describe('what the scrubber walks', () => {
 	it('plays one frame per configuration the planner returned', async () => {
@@ -388,10 +369,6 @@ describe('the request the panel sends', () => {
 	})
 })
 
-/**
- * `waypointIndices` is what the scrubber draws its tick marks from, and it is the only thing telling
- * a user which of 180 interpolated frames the planner actually chose.
- */
 describe('marking which played frames are planned waypoints', () => {
 	it('marks every frame when each one is a waypoint', async () => {
 		const h = setup()
@@ -417,11 +394,6 @@ describe('marking which played frames are planned waypoints', () => {
 	})
 })
 
-/**
- * The two detail settings are different framings of one motion, so a frame index does not carry
- * across. Leaving `currentStep` where it was pointed the scrubber past the end of the shorter
- * framing and left the ghosts showing a pose from the other one.
- */
 describe('switching what a frame represents', () => {
 	it('restarts playback rather than keeping an index that no longer means anything', async () => {
 		const h = setup()
@@ -440,12 +412,8 @@ describe('switching what a frame represents', () => {
 	})
 
 	/**
-	 * `interpolatedFrames` costs a joint's travel in degrees unless told otherwise, so a component the
-	 * hook does not label as prismatic has its millimetres read as radians — a 40 mm slide costs the
-	 * same as 40 radians of arm travel, over 200 turns. `jointMotionsOf(descriptors)` is what supplies
-	 * that label; `interpolateTrajectory.spec.ts` measures the unlabelled cost of this exact slide at
-	 * 1,529 frames against 10 labelled, so anything under 50 here is only reachable with the label
-	 * wired through.
+	 * Where the 50 comes from: without `jointMotionsOf`'s labels, `interpolateTrajectory.spec.ts`
+	 * measures this exact slide at 1,529 frames against 10 labelled.
 	 */
 	it('keeps a gantry slide within its millimetre budget instead of costing it in radians', async () => {
 		const h = setup([GANTRY])

src/lib/plugins/MoveFrame/usePreviewMove.svelte.ts

@@ -44,25 +44,14 @@ import {
 export type PreviewStatus = 'idle' | 'planning' | 'ready' | 'already-at-goal' | 'error'
 
 /**
- * What one frame of playback represents. Not a fidelity setting — both show the same motion, and
- * neither is faster or slower than the real move, since a trajectory carries no timing at all.
- *
- *   - `waypoints` gives each configuration the planner returned its own frame, and nothing between.
- *   - `interpolated` adds frames along the straight joint path between waypoints, in proportion to
- *     travel. That path is not invented — it is the one RDK collision-checks when it validates the
- *     segment — but it is what the planner *approved*, not a promise of what the arm traces: the
- *     component is handed the whole waypoint list so it can blend between them.
+ * What one frame of playback represents: `waypoints` gives each configuration the planner returned
+ * its own frame, `interpolated` fills in along the straight joint path RDK collision-checks.
  */
 export type PreviewDetail = 'waypoints' | 'interpolated'
 
 /**
- * How long a preview takes to play, whatever it is made of. Pacing to a duration rather than to a
- * frame rate is what makes the two detail settings comparable: `waypoints` and `interpolated`
- * describe the same motion with wildly different frame counts, so a fixed per-frame interval would
- * race through a two-waypoint plan and crawl through a two-hundred-waypoint one.
- *
- * It is also the honest unit here. A trajectory carries no timing, so no frame rate is more correct
- * than another — but "the whole move takes about this long" is at least a consistent claim.
+ * How long a preview takes to play, whatever it is made of. Pacing to a duration rather than a frame
+ * rate keeps a two-waypoint plan and a two-hundred-waypoint one comparable.
  */
 const PREVIEW_DURATION_MS = 4000
 
@@ -129,20 +118,10 @@ export const usePreviewMove = ({
 }: PreviewMoveOptions): PreviewMove => {
 	let status = $state<PreviewStatus>('idle')
 	let message = $state<string>()
-	// Raw: all three are replaced wholesale — a new array assigned outright, never mutated in place —
-	// so the fine-grained reactivity `$state`'s proxy buys elsewhere would go unused here. `trajectory`
-	// and `playbackFrames` are also large, and nothing in this file reads into either of them
-	// reactively; only `playbackFrames.length`, through the player. `waypointIndices` is smaller — one
-	// entry per waypoint, not per frame — and it is read into: `MovePreview.svelte` hands it to
-	// `TrajectoryScrubber` as `markers`, which maps, iterates and length-compares it directly. That
-	// reads elements of a wholesale-replaced array, which `$state.raw` still serves correctly; it is
-	// only element-level *mutation* that a plain array would miss.
-	//
-	// Three arrays rather than one because they answer different questions. `trajectory` is what the
-	// planner said and what `execute` must receive; `playbackFrames` is that same motion subdivided for
-	// playback, which the robot must never be asked to run; `waypointIndices` marks which entries of
-	// `playbackFrames` are planner waypoints rather than frames interpolated in between.
+	// Raw: all three are replaced wholesale rather than mutated, so the deep proxy would go unused,
+	// `waypointIndices` included — the scrubber reads it element-wise but never writes into it.
 	let trajectory = $state.raw<TrajectoryStep[]>([])
+	// What the scrubber walks. Only `trajectory` may ever be handed to `execute`.
 	let playbackFrames = $state.raw<TrajectoryStep[]>([])
 	let waypointIndices = $state.raw<number[]>([])
 	let detail = $state<PreviewDetail>('waypoints')
@@ -340,8 +319,8 @@ export const usePreviewMove = ({
 		get detail() {
 			return detail
 		},
-		// Switching detail re-frames the same plan, so playback restarts rather than trying to map
-		// the current frame across two different framings of the motion.
+		// A frame index does not carry across the two framings, so playback restarts rather than
+		// trying to map it.
 		set detail(next: PreviewDetail) {
 			if (next === detail) return
 			detail = next