Fix findUsages/undo/node-lookup bugs, split Viewport and Editor, share command and inspector helpers

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

tests/core/bag-commands.test.ts

@@ -0,0 +1,60 @@
+/**
+ * The undo contract every "write one key into a property bag" command shares: a key that was
+ * not in the bag before the edit must be *absent* again afterwards, not present as undefined.
+ * Serialization and the "is this an override?" checks in the inspector go by key presence.
+ */
+import { describe, expect, it } from 'vitest';
+import { ResetAssetPropertyCommand, SetAssetPropertyCommand } from '../../src/core/commands/assetCommands';
+import { SetNodeParamCommand } from '../../src/core/commands/graphCommands';
+import { ClearOverrideCommand, SetOverrideCommand } from '../../src/core/commands/sceneObjectCommands';
+import { findAsset } from '../../src/core/model/Project';
+import { graphEditor, makeBehavior, makeSpriteScene, place } from '../helpers';
+
+describe('bag value commands', () => {
+  it('undoing an override leaves no key behind', () => {
+    const fixture = makeSpriteScene();
+    const box = place(fixture, fixture.spriteId, { x: 0, y: 0 });
+    fixture.stack.execute(new SetOverrideCommand(fixture.sceneId, box, 'width', 33));
+    expect(fixture.object(box).overrides).toEqual({ width: 33 });
+
+    fixture.stack.undo();
+    expect('width' in fixture.object(box).overrides).toBe(false);
+  });
+
+  it('undoing a cleared override puts the old value back', () => {
+    const fixture = makeSpriteScene();
+    const box = place(fixture, fixture.spriteId, { x: 0, y: 0 });
+    fixture.stack.execute(new SetOverrideCommand(fixture.sceneId, box, 'width', 33));
+    fixture.stack.execute(new ClearOverrideCommand(fixture.sceneId, box, 'width'));
+    expect('width' in fixture.object(box).overrides).toBe(false);
+
+    fixture.stack.undo();
+    expect(fixture.object(box).overrides.width).toBe(33);
+  });
+
+  it('coerces on set and restores the untouched default on undo of a reset', () => {
+    const fixture = makeSpriteScene();
+    const sprite = () => findAsset(fixture.project, fixture.spriteId)!;
+    // 'width' is a number property, so a numeric string is coerced.
+    fixture.stack.execute(new SetAssetPropertyCommand(fixture.spriteId, 'width', '250'));
+    expect(sprite().props.width).toBe(250);
+
+    fixture.stack.execute(new ResetAssetPropertyCommand(fixture.spriteId, 'width'));
+    expect(sprite().props.width).toBe(100);
+    fixture.stack.undo();
+    expect(sprite().props.width).toBe(250);
+  });
+
+  it('restores a node param that had no value before', () => {
+    const fixture = makeSpriteScene();
+    const behavior = makeBehavior(fixture, 'B');
+    // `Owner` declares no params, so 'value' is an inline input value: stored as given.
+    const node = graphEditor(fixture, behavior).addNode('Owner');
+
+    fixture.stack.execute(new SetNodeParamCommand(behavior.id, node.id, 'value', 7));
+    expect(node.params.value).toBe(7);
+
+    fixture.stack.undo();
+    expect('value' in node.params).toBe(false);
+  });
+});

tests/core/graph.test.ts

@@ -1,5 +1,6 @@
 import { describe, expect, it } from 'vitest';
-import { DisconnectCommand, RemoveNodeCommand, SetNodeParamCommand } from '../../src/core/commands/graphCommands';
+import { CommandError } from '../../src/core/commands/Command';
+import { AddNodeCommand, DisconnectCommand, RemoveNodeCommand, SetNodeParamCommand } from '../../src/core/commands/graphCommands';
 import { AddVariableCommand, RemoveVariableCommand, RenameVariableCommand } from '../../src/core/commands/variableCommands';
 import { GraphInterpreter, allNodeDefs, findNode, portsCompatible } from '../../src/core/graph';
 import type { BehaviorAsset } from '../../src/core/model/Asset';
@@ -296,3 +297,16 @@ describe('behavior serialization', () => {
     expect(cleaned.graph.edges.length).toBe(behavior.graph.edges.length);
   });
 });
+
+describe('unknown node types', () => {
+  it('are rejected as a CommandError by execute, not thrown from the constructor', () => {
+    const fixture = makeProject();
+    const behavior = makeBehavior(fixture, 'B');
+    // Building the command must not throw: callers construct it outside the editor's
+    // error handling (`state.execute(new AddNodeCommand(…))`), so a throw here would crash.
+    const command = new AddNodeCommand(behavior.id, 'DoesNotExist', 0, 0);
+    expect(command.label).toBe('Add DoesNotExist');
+    expect(() => command.execute(fixture.project)).toThrow(CommandError);
+    expect(behavior.graph.nodes).toHaveLength(0);
+  });
+});

tests/core/holders.test.ts

@@ -0,0 +1,113 @@
+/**
+ * Container assets hold placed objects just like scenes do. Anything that walks "all placed
+ * objects" has to cover both – the tests here pin the places that used to look at scenes only.
+ * The history tests pin that a rejected undo leaves the stacks usable.
+ */
+import { describe, expect, it } from 'vitest';
+import type { Command } from '../../src/core/commands/Command';
+import { CommandError } from '../../src/core/commands/Command';
+import { CommandStack } from '../../src/core/commands/CommandStack';
+import { CreateContainerCommand } from '../../src/core/commands/containerCommands';
+import { SetOverrideCommand } from '../../src/core/commands/sceneObjectCommands';
+import { RemoveVariableCommand, RenameVariableCommand } from '../../src/core/commands/variableCommands';
+import type { ContainerAsset } from '../../src/core/model/Asset';
+import { findAsset, findUsages, objectHolders } from '../../src/core/model/Project';
+import { makeBehavior, makeSpriteScene, place } from '../helpers';
+
+/** A behavior with a `speed` variable, placed and overridden to 42, then wrapped in a container. */
+function containerWithOverride() {
+  const f = makeSpriteScene();
+  const behavior = makeBehavior(f, 'B', ['speed']);
+  const objectId = place(f, behavior.id, { x: 0, y: 0 });
+  f.stack.execute(new SetOverrideCommand(f.sceneId, objectId, 'speed', 42));
+  const container = new CreateContainerCommand(f.sceneId, [objectId]);
+  f.stack.execute(container);
+  const template = () => (findAsset(f.project, container.assetId) as ContainerAsset).objects[0]!;
+  return { ...f, behaviorId: behavior.id, objectId, template };
+}
+
+describe('object holders', () => {
+  it('counts scenes and container assets alike', () => {
+    const { project } = containerWithOverride();
+    expect(objectHolders(project).map((h) => h.type).sort()).toEqual(['container', 'scene']);
+  });
+
+  it('finds usages inside container templates, not just in scenes', () => {
+    const { project, behaviorId, template } = containerWithOverride();
+    // The placed object and the template copy the container was built from.
+    expect(findUsages(project, behaviorId)).toHaveLength(2);
+    expect(template().overrides.speed).toBe(42);
+  });
+});
+
+describe('RemoveVariableCommand', () => {
+  it('clears the override on container templates too, and puts it back on undo', () => {
+    const { stack, behaviorId, objectId, sceneOf, template } = containerWithOverride();
+    const placed = () => sceneOf().objects.find((o) => o.id === objectId)!;
+
+    stack.execute(new RemoveVariableCommand(behaviorId, 'speed'));
+    expect(placed().overrides.speed).toBeUndefined();
+    expect(template().overrides.speed).toBeUndefined();
+
+    stack.undo();
+    expect(placed().overrides.speed).toBe(42);
+    expect(template().overrides.speed).toBe(42);
+  });
+});
+
+describe('RenameVariableCommand', () => {
+  it('renames the override on container templates too', () => {
+    const { stack, behaviorId, template } = containerWithOverride();
+    stack.execute(new RenameVariableCommand(behaviorId, 'speed', 'velocity'));
+    expect(template().overrides.speed).toBeUndefined();
+    expect(template().overrides.velocity).toBe(42);
+
+    stack.undo();
+    expect(template().overrides.speed).toBe(42);
+    expect(template().overrides.velocity).toBeUndefined();
+  });
+});
+
+/** Applies a marker to the project; its undo refuses to run. */
+class RefusingUndo implements Command {
+  readonly label = 'Refusing';
+  execute(project: { name: string }): void {
+    project.name = 'executed';
+  }
+  undo(): void {
+    throw new CommandError('target is gone');
+  }
+}
+
+describe('CommandStack', () => {
+  it('keeps a command on the undo stack when its undo is rejected', () => {
+    const { project, stack } = makeSpriteScene();
+    stack.execute(new RefusingUndo());
+    expect(stack.canUndo).toBe(true);
+
+    expect(() => stack.undo()).toThrow(CommandError);
+    // Not swallowed by the history: still undoable, and not queued for redo.
+    expect(stack.canUndo).toBe(true);
+    expect(stack.canRedo).toBe(false);
+    expect(project.name).toBe('executed');
+  });
+
+  it('keeps a command on the redo stack when its redo is rejected', () => {
+    const project = { formatVersion: 1, id: 'p', name: 'P', assets: [], activeSceneId: '' };
+    const stack = new CommandStack(project);
+    let failing = false;
+    stack.execute({
+      label: 'Toggle',
+      execute: () => {
+        if (failing) throw new CommandError('nope');
+      },
+      undo: () => {},
+    });
+    stack.undo();
+    failing = true;
+
+    expect(() => stack.redo()).toThrow(CommandError);
+    expect(stack.canRedo).toBe(true);
+    expect(stack.canUndo).toBe(false);
+  });
+});

tests/core/list.test.ts

@@ -0,0 +1,45 @@
+import { describe, expect, it } from 'vitest';
+import { removeById, removeFirst, restoreAt } from '../../src/core/util/list';
+
+const items = () => [{ id: 'a' }, { id: 'b' }, { id: 'c' }];
+
+describe('removeFirst', () => {
+  it('takes the match out and reports the index it had', () => {
+    const list = items();
+    expect(removeFirst(list, (i) => i.id === 'b')).toEqual({ item: { id: 'b' }, index: 1 });
+    expect(list.map((i) => i.id)).toEqual(['a', 'c']);
+  });
+
+  it('stops at the first match', () => {
+    const list = [{ id: 'a' }, { id: 'a' }];
+    expect(removeFirst(list, (i) => i.id === 'a')?.index).toBe(0);
+    expect(list).toHaveLength(1);
+  });
+
+  it('leaves the list untouched when nothing matches', () => {
+    const list = items();
+    expect(removeFirst(list, (i) => i.id === 'zz')).toBeUndefined();
+    expect(list.map((i) => i.id)).toEqual(['a', 'b', 'c']);
+  });
+
+  it('round-trips with restoreAt', () => {
+    const list = items();
+    const removed = removeFirst(list, (i) => i.id === 'b');
+    restoreAt(list, removed ? [removed] : []);
+    expect(list.map((i) => i.id)).toEqual(['a', 'b', 'c']);
+  });
+});
+
+describe('removeById', () => {
+  it('returns a new list without that item and does not mutate the old one', () => {
+    const list = items();
+    const next = removeById(list, 'a');
+    expect(next.map((i) => i.id)).toEqual(['b', 'c']);
+    expect(list).toHaveLength(3);
+    expect(next).not.toBe(list);
+  });
+
+  it('keeps every item when the id is unknown', () => {
+    expect(removeById(items(), 'zz').map((i) => i.id)).toEqual(['a', 'b', 'c']);
+  });
+});

tests/core/walk.test.ts

@@ -0,0 +1,69 @@
+import { describe, expect, it } from 'vitest';
+import { AddColliderCommand } from '../../src/core/commands/colliderCommands';
+import { type EntityHandle, worldPositionOf } from '../../src/core/graph';
+import { hasParentCycle, parentChain } from '../../src/core/model/walk';
+import { makeSpriteScene, place, startWorld } from '../helpers';
+
+interface Node {
+  id: string;
+  parent: Node | null;
+}
+
+const parentOf = (node: Node) => node.parent;
+
+/** a → b → c, where c is parented back to a (only reachable through a broken document or a bug). */
+function cycle(): { a: Node; b: Node; c: Node } {
+  const a: Node = { id: 'a', parent: null };
+  const b: Node = { id: 'b', parent: a };
+  const c: Node = { id: 'c', parent: b };
+  a.parent = c;
+  return { a, b, c };
+}
+
+describe('parentChain', () => {
+  it('yields the item and its ancestors, nearest first', () => {
+    const a: Node = { id: 'a', parent: null };
+    const b: Node = { id: 'b', parent: a };
+    expect([...parentChain(b, parentOf)].map((n) => n.id)).toEqual(['b', 'a']);
+  });
+
+  it('terminates on a cyclic parent chain, visiting every item once', () => {
+    const { c } = cycle();
+    expect([...parentChain(c, parentOf)].map((n) => n.id)).toEqual(['c', 'b', 'a']);
+  });
+
+  it('reports a cycle only when the parents never reach a root', () => {
+    const { c } = cycle();
+    expect(hasParentCycle(c, parentOf)).toBe(true);
+    expect(hasParentCycle({ id: 'lone', parent: null }, parentOf)).toBe(false);
+  });
+});
+
+describe('cycle-safe walks in the runtime', () => {
+  it('world position of a cyclic chain terminates and counts each ancestor once', () => {
+    const a: EntityHandle = { id: 'a', assetId: 'x', name: 'a', x: 1, y: 10, z: 0, props: {}, parent: null };
+    const b: EntityHandle = { id: 'b', assetId: 'x', name: 'b', x: 2, y: 20, z: 0, props: {}, parent: a };
+    a.parent = b;
+    expect(worldPositionOf(b)).toEqual({ x: 3, y: 30, z: 0 });
+  });
+
+  // Guards `travelsWith`: before the shared iterator it walked parents without a seen-set and hung here.
+  it('moveAndSlide terminates when a collider sits in a cyclic parent chain', () => {
+    const f = makeSpriteScene();
+    const moverId = place(f, f.spriteId, { x: 0, y: 0 });
+    const wallId = place(f, f.spriteId, { x: 200, y: 0 });
+    const moverCollider = new AddColliderCommand(f.sceneId, moverId);
+    const wallCollider = new AddColliderCommand(f.sceneId, wallId);
+    f.stack.execute(moverCollider);
+    f.stack.execute(wallCollider);
+    const { world, collisions } = startWorld(f, { collisions: true });
+    const mover = world.findByObjectId(moverId)!;
+    const wall = world.findByObjectId(wallId)!;
+    const wallColliderEntity = world.findByObjectId(wallCollider.objectId)!;
+    // Close the loop: the wall's collider is its child and the wall is parented to that collider.
+    wall.parent = wallColliderEntity;
+
+    expect(collisions.moveAndSlide(world, mover, 10, 0)).toEqual({ blockedX: false, blockedY: false });
+    expect(mover.x).toBe(10);
+  }, 2000);
+});

tests/ui/math.test.ts

@@ -0,0 +1,35 @@
+import { describe, expect, it } from 'vitest';
+import { clamp, centreY, zoomFactor } from '../../src/ui/math';
+
+describe('clamp', () => {
+  it('keeps a value inside the range', () => {
+    expect(clamp(5, 0, 10)).toBe(5);
+    expect(clamp(-1, 0, 10)).toBe(0);
+    expect(clamp(11, 0, 10)).toBe(10);
+  });
+
+  it('lets the lower bound win when the range is inverted', () => {
+    // A popup wider than the screen: pinning it to the left edge beats pinning it to the right.
+    expect(clamp(5, 10, 0)).toBe(10);
+  });
+});
+
+describe('centreY', () => {
+  it('centres on whole pixels', () => {
+    expect(centreY(30, 20)).toBe(5);
+    expect(centreY(31, 20)).toBe(6);
+  });
+});
+
+describe('zoomFactor', () => {
+  it('zooms in for a negative delta and out for a positive one', () => {
+    expect(zoomFactor(-100)).toBeGreaterThan(1);
+    expect(zoomFactor(100)).toBeLessThan(1);
+    expect(zoomFactor(0)).toBe(1);
+  });
+
+  it('returns exactly to the starting zoom after scrolling back', () => {
+    // Why exponential: the scene view and the graph editor both rely on this cancelling out.
+    expect(1 * zoomFactor(-120) * zoomFactor(120)).toBeCloseTo(1, 12);
+  });
+});

tests/viewport/view.test.ts

@@ -0,0 +1,51 @@
+/**
+ * The viewport transforms used to be private methods of a Pixi widget and could only be
+ * checked through the browser. They are plain geometry, so they are pinned here.
+ */
+import { describe, expect, it } from 'vitest';
+import { cameraView, freeView, isInside, screenToWorld, worldBounds, worldToScreen } from '../../src/viewport/view';
+
+describe('freeView', () => {
+  it('puts the view centre in the middle of the widget', () => {
+    const view = freeView(50, -20, 2, 800, 600);
+    expect(worldToScreen(view, 50, -20)).toEqual({ x: 400, y: 300 });
+    expect(screenToWorld(view, 400, 300)).toEqual({ x: 50, y: -20 });
+  });
+
+  it('scales distances by the zoom', () => {
+    const view = freeView(0, 0, 2, 800, 600);
+    expect(worldToScreen(view, 100, 0)).toEqual({ x: 600, y: 300 });
+  });
+
+  it('round-trips any point', () => {
+    const view = freeView(13, -7, 0.35, 640, 480);
+    const back = screenToWorld(view, ...Object.values(worldToScreen(view, 123, -456)) as [number, number]);
+    expect(back.x).toBeCloseTo(123, 9);
+    expect(back.y).toBeCloseTo(-456, 9);
+  });
+
+  it('covers the widget in world units', () => {
+    const bounds = worldBounds(freeView(0, 0, 2, 800, 600));
+    expect(bounds).toEqual({ left: -200, top: -150, right: 200, bottom: 150 });
+  });
+});
+
+describe('cameraView', () => {
+  const camera = { x: 10, y: 20, props: { width: 640, height: 360 } };
+
+  it('fits the camera frame into the widget and centres it', () => {
+    // 640×360 into 800×600: width is the tighter fit (1.25 vs 1.67), so bars top and bottom.
+    const view = cameraView(camera, 800, 600);
+    expect(view.scale).toBeCloseTo(1.25, 9);
+    expect(view.rect).toEqual({ x: 0, y: 75, w: 800, h: 450 });
+    // The camera position sits at the centre of the frame, not of the widget.
+    expect(worldToScreen(view, 10, 20)).toEqual({ x: 400, y: 300 });
+  });
+
+  it('reports the letterbox as outside', () => {
+    const view = cameraView(camera, 800, 600);
+    expect(isInside(view, 400, 300)).toBe(true);
+    expect(isInside(view, 400, 20)).toBe(false); // top bar
+    expect(isInside(view, 400, 580)).toBe(false); // bottom bar
+  });
+});