replayer plan snapshot cleanup - #921
Conversation
🦋 Changeset detectedLatest commit: b75d018 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 |
|
0e9380b to
171d2b7
Compare
171d2b7 to
94ed389
Compare
02f2ddb to
c08ed82
Compare
c08ed82 to
5b189b6
Compare
|
Claude (@claude) review |
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 5m 17s —— View job ReviewThe fix is correct and complete. The root cause — snapshot store keyed by mutable array position rather than plan identity — is diagnosed precisely, and the fix is applied consistently at all four write/read sites ( CorrectnessCore fix (
TestsThe spec suite is thoughtfully constructed. A few specific observations:
Minor
Approve once the UUID nit and the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Only primitives here — proto objects (Snapshot[]) live outside $state to avoid Svelte 5 deep proxy | ||
| interface PlanState { |
There was a problem hiding this comment.
PlanState is not exported but it's the element type of MotionPlanReplayerContext.plans, so it's already part of the public API surface by inference. Callers who need to declare a typed variable for a plan entry have to reach for MotionPlanReplayerContext['plans'][number]. Since id is newly present and the UI already uses it as a key, exporting the interface would make consumption cleaner.
| interface PlanState { | |
| export interface PlanState { |
| const drawnFrames = (world: World): string[] => | ||
| world | ||
| .query(traits.Name) | ||
| .map((entity: Entity) => entity.get(traits.Name)) | ||
| .filter((name): name is string => typeof name === 'string' && name.endsWith('-frame')) | ||
| .toSorted() |
There was a problem hiding this comment.
drawnFrames is the right assertion primitive here — it catches "reading the wrong plan's array of the same length" where a simple step-count check would silently pass. One nit: the helper name is singular (frame) but it returns multiple names; drawnFrameNames or queryFrameNames would be self-documenting without needing the JSDoc above it.
| poseInObserverFrame: new PoseInFrame({ referenceFrame: 'world' }), | ||
| // Stable across a plan's steps, distinct across plans: reconcile keys on this, so | ||
| // repeating it is what makes a scrub update the entity rather than respawn it. | ||
| uuid: Uint8Array.from(UuidTool.toBytes(`${plan}-0000-4000-8000-000000000000`)), |
There was a problem hiding this comment.
UuidTool.toBytes expects a standard xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx format (8-4-4-4-12). plan-0-0000-4000-8000-000000000000 is 6-4-4-4-12, which is invalid. This works today because the tests pass (presumably UuidTool is lenient or produces usable bytes from the hex digits it finds), but if the library ever tightens its parser this silently produces wrong bytes rather than a test failure. Padding to 8 chars — e.g. 00plan-0-0000-4000-8000-000000000000 — would produce a well-formed UUID and keep the per-plan distinctness.
| const removePlan = (index: number) => { | ||
| const removed = plans[index] | ||
| if (!removed) return |
There was a problem hiding this comment.
Neat: plans[index] being undefined for negative, fractional, and out-of-range indices is the single guard that handles all three bad-input cases. The test confirms each one, and this is meaningfully simpler than explicit range/integer checks would be.
5b189b6 to
b75d018
Compare
b75d018 to
1a31725
Compare
1 participant
Fixes the plan replayer handing the active plan another plan's snapshots after a removal, so a scrub draws the plan you actually selected and the scrubber can reach the end of it. Stacks on #920.
snapshotStorewas keyed by a plan's index in theplansarray, whichremovePlanreindexes with afilter. Removing any plan ahead of the active one shifted every later plan down a slot, and each then read whichever snapshots had inherited its old position.activePlanIndexwas adjusted to follow the shift; the store was not. Nothing threw, becausesetStepclamps against the array it has just fetched, so the read was always in range. What you got instead was the wrong robot on screen and a scrubber that would not finish:currentSteppinned at the short array's last index whilelastStepIdxstill came from the plan's ownstepCount, soatEndwas never true, the play loop re-reconciled a single frame every 100 ms forever, and every forward control stayed enabled.Stack
$lib/motion(make motion utils reusable #917)MoveFrameplugin (Motion plan preview #908)Frontend
PlanStategains anid, assigned from anextPlanIdcounter wherever a plan is created: theinitialPlansmap andaddPlan. All foursnapshotStorecall sites key by it:addPlan's write,loadPlan's cached read,loadPlan's parse-path write, andsetStep's read.removePlanreadsplans[index]first and returns if there is nothing there. The id lookup needs the entry, and returning early also stops an index that names no plan from reaching theactivePlanIndexshift below it.activePlanIndexstill shifts on removal, because it genuinely is a position. It is now the only thing that is.MotionPlanReplayerUI.sveltekeys its{#each}onplan.idrather thanplan.name.setOrAddColorandsetOrAddOpacityare replaced with$lib/ecs's sharedsetOrAddTrait.Why?
Why does the panel's
{#each}key change too?Because this is the PR that gives it something stable to key on. Only the upload path in
handlePlanFilerejects a duplicate name; neitheraddPlan, which is public API through./plugins, nor theplansprop does. Svelte'seach_key_duplicateis thrown in production builds as well as in dev, so two plans sharing a name would take the panel down on mount. Keying by id retires name uniqueness as a load-bearing invariant.Why key
addPlan's write by id when the plan is being appended at the end?Because
index = plans.lengthis only unique in a list nothing has been removed from. Add A, B and C, remove A, then add D: D takesindex = 2, which is still C's key. Keyed by position, D's snapshots overwrite C's outright rather than merely being read in its place, so this one is corruption rather than a misread.loadPlan's parse-path write reaches the same store through the same arithmetic and has the same hole.Why replace the two local trait helpers with
setOrAddTrait?The pair was there for a real reason, but the reason recorded above them was wrong. They claimed koota's
seton an absent trait writes the store slot silently, leavinghasfalse so nothing ever sees the value. It does not fail silently, it throwsTypeError: Cannot read properties of undefined (reading 'store'). The guard is right either way, but a loud failure and a silent one are debugged very differently, and the shared helper in$lib/ecsalready documents the real behavior plusadd's mirror-image problem, where it returns early on a trait that is already present and drops the value it was passed. Both cases apply here: plan transforms carry no color metadata, soColoris always absent on spawn, whileOpacityonly happens to be present becausedrawTransformadds it unconditionally.Why a component fixture rather than
$effect.root?provideMotionPlanReplayerreads the world and the relationship registry off Svelte context, andsetContextonly works during component initialization. The provider cannot be reached from a plain$effect.root, soReplayerHarness.svelteis the smallest component that stands the three of them up together.Why does the harness hand back the world as well as the context?
Because almost everything this module does lands in the world rather than on the context. Without it a spec can assert step counts and indices and nothing else, which is exactly why the display-config and entity-teardown behavior had no coverage. It is one extra return value and it is what makes the identity assertions possible.
Why do the fixture's snapshots name their plan?
This is the difference between a test that pins the bug and one that looks like it does. Cycling a single fixture's snapshots across every plan makes each plan's geometry byte-identical, so a test can only ever notice that it read an array of the wrong length. Reading the wrong plan's array of the same length, which is the failure this module actually had, draws a completely different robot and would pass.
planSnapshotsnames the reference frame per plan, which makes the assertions about identity. The uuid is derived from the plan name too, stable across that plan's steps and distinct across plans, because reconcile keys on it: repeating it is what makes a scrub update the entity rather than respawn it, which the display-edit test depends on.Why does the UI spec mock three things?
They are the three boundaries a spec in this repo is allowed to stub, external I/O aside, and nothing else here is mocked.
FloatingPanelseeds its position fromuseThrelte().dom, which the global@threlte/coremock has no field for, so the real panel throws on mount outside a<Canvas>.DashboardPortalis imported from the$libbarrel, which re-exportsApp.svelteand drags the entire Threlte component tree in behind it; the component itself is only aPortal, already mocked globally to a passthrough, so a passthrough stand-in loses nothing.useToastneeds aprovideToastancestor and nothing here checks toast content.Testing
pnpm exec vitest --runpasses 768 tests across 72 files, up 16 tests and 2 files from the base branch.pnpm exec svelte-checkreports 0 errors and 0 warnings.Reverting the fix to position keying fails 3 tests, including
leaves the active plan reading its own snapshots, not its neighbour'son both its step count andexpected [ 'plan-1-frame' ] to deeply equal [ 'plan-2-frame' ], which is the shipped bug exactly: plans of 2, 2 and 6 steps, remove the first, and the active 6-step plan reads its neighbour's array.The assertion carrying that block is
drawnFrames(world), not the step counts around it.totalStepsreadsstepCountoffPlanState, which stays correct even while the store read is wrong, so the counts read stronger than they are: on their own they catch a length mismatch and nothing else. Only the drawn frame names catch reading the wrong plan's array of the same length.Three cases exist specifically because the naive version of them passes without the fix:
activePlanIndex > indexcomparison, so with the active plan held at index 1 both would pass with no guard at all.keys a plan it parsed itself the same way, without landing on a live planremoves a plan before adding the parsed one, so position and id genuinely differ. Added into an untouched list the two coincide and keying by either one passes.does not overwrite a surviving plan when a new one is added after a removalforces the same divergence foraddPlan.colors a freshly spawned plan entity even though Color is always absent on spawnbuilds its own snapshot with aphysicalObject, becauseapplyStepcolors only entities that got real geometry and the sharedplanSnapshotshelper spawns bareReferenceFramemarkers. It is the only case that gates theColorhalf of thesetOrAddTraitswap.keeps display edits made while scrubbingcovers the capture-and-restore block aroundreconcileSnapshotEntities, which had no coverage of any kind. Reconcile runsupdateMetadataon every step, which resetsOpacityand dropsInvisibleandShowAxesHelper; those forty-odd lines exist to put a user's Details-panel edits back, and the comments there record that scrubbing wiping them is a regression this module has already had once. Deleting the whole block, or just the restore loop, passed the entire suite.MotionPlanReplayerUI.svelte.spec.tsis a single test, and the render is the assertion rather than the setup for it: two plans sharing a name mount, and twoRemove planbuttons come back. Under the old(plan.name)key Svelte throwseach_key_duplicatebefore any query runs. The store spec's duplicate-name case pins the id generator but renders nothing, so this is the only thing constraining the{#each}key. Everything past the key still has no coverage, including the index threaded intoselectPlan(i)andremovePlan(i), which is this same class of bug one layer up.