refactor(engine): tween-to-galaxy helper dedup

src/services/engine/engine.ts

@@ -113,7 +113,8 @@ import { filamentFetcher } from '../loading/fetchers/filamentFetcher';
 import { famousMetaFetcher } from '../loading/fetchers/famousMetaFetcher';
 import { pgcAliasFetcher, type PgcAliasMap } from '../loading/fetchers/pgcAliasFetcher';
 import { TIER_TARGETS } from '../../data/tierTargets';
-import { FOCUS_TWEEN_MS, focusDistanceMpc } from './focusTween';
+import { FOCUS_TWEEN_MS } from './focusTween';
+import { tweenToGalaxy } from './tweenToGalaxy';
 
 // ── Galaxy thumbnail subsystem ────────────────────────────────────────────
 //
@@ -1891,8 +1892,13 @@ export function createEngine(canvas: HTMLCanvasElement, cb: EngineCallbacks): En
     },
 
     focusOn(info) {
-      // Camera may not be ready yet (cloud still loading); drop the call.
-      // Same defensive pattern as resetCamera() above.
+      // Camera may not be ready yet (cloud still loading); drop the
+      // call.  This guard is *separate* from `tweenToGalaxy`'s own
+      // cam-null guard — we need it here to gate the
+      // `onFocusChange` callback below.  Without the early return,
+      // a focus call against a still-bootstrapping engine would
+      // update `#focus=…` in the URL while the camera silently
+      // refused to move.
       const cam = state.cam;
       if (!cam) return;
 
@@ -1903,31 +1909,14 @@ export function createEngine(canvas: HTMLCanvasElement, cb: EngineCallbacks): En
       // for "we just decided to focus on this galaxy."
       cb.onFocusChange?.(info);
 
-      // Snapshot the CURRENT camera state — not the original startup state —
-      // so an in-progress tween hands off smoothly to the new one.  vec3.clone
-      // copies the target tuple so future mutation of cam.target doesn't
-      // corrupt the from-snapshot.
-      //
-      // The framing distance is 4× the galaxy's diameter (close-but-not-
-      // inside framing that scales naturally with size); when the
-      // PointInfo's diameter is the fallback 30 kpc, this lands on the
-      // pre-v4 placeholder framing exactly.
-      state.subsystems.tweens.start({
-        startMs: performance.now(),
-        durationMs: FOCUS_TWEEN_MS,
-        fromTarget: vec3.clone(cam.target as vec3),
-        toTarget: vec3.fromValues(info.x, info.y, info.z),
-        fromDistance: cam.distance,
-        toDistance: focusDistanceMpc(info.diameterKpc),
-        fromYaw: cam.yaw,
-        toYaw: cam.yaw, // preserve yaw — user keeps their orientation
-        fromPitch: cam.pitch,
-        toPitch: cam.pitch, // preserve pitch
-      });
-      // Kick the loop into motion — the tween's per-frame advance will
-      // keep it ticking via the still-animating predicate until the
-      // tween completes.
-      state.subsystems.scheduler.requestRender();
+      // The framing distance is derived from the galaxy's diameter
+      // (close-but-not-inside framing that scales naturally with size);
+      // when the PointInfo's diameter is the fallback 30 kpc, this
+      // lands on the pre-v4 placeholder framing exactly.  All the
+      // tween-construction details (yaw/pitch preservation, vec3.clone
+      // of cam.target, performance.now() startMs, scheduler kick-off)
+      // live in `tweenToGalaxy`.
+      tweenToGalaxy(state, info);
     },
 
     selectFamous(id) {
@@ -1961,25 +1950,14 @@ export function createEngine(canvas: HTMLCanvasElement, cb: EngineCallbacks): En
       cb.onFocusChange?.(info);
 
       // Tween the camera onto the galaxy — same tween as `focusOn`.
-      // We inline the tween-creation here rather than calling `handle.focusOn`
-      // because we're inside the object literal and `this` would be unreliable
-      // at call time (depending on how App.tsx invokes the handle method).
-      // Copying the tween-setup block keeps the behaviour identical.
-      const cam = state.cam;
-      if (!cam) return;
-      state.subsystems.tweens.start({
-        startMs: performance.now(),
-        durationMs: FOCUS_TWEEN_MS,
-        fromTarget: vec3.clone(cam.target as vec3),
-        toTarget: vec3.fromValues(info.x, info.y, info.z),
-        fromDistance: cam.distance,
-        toDistance: focusDistanceMpc(info.diameterKpc),
-        fromYaw: cam.yaw,
-        toYaw: cam.yaw,
-        fromPitch: cam.pitch,
-        toPitch: cam.pitch,
-      });
-      state.subsystems.scheduler.requestRender();
+      // We don't call `handle.focusOn` directly because `this` would be
+      // unreliable at call time (depending on how App.tsx invokes the
+      // handle method) and because focusOn fires `onFocusChange` again,
+      // which we already did above with the prebuilt PointInfo.
+      // `tweenToGalaxy` is the shared kernel both methods build on top
+      // of — same observable behaviour, no duplication.  Cam-null is
+      // handled inside the helper.
+      tweenToGalaxy(state, info);
     },
 
     getCloudObjIds(source) {
@@ -2047,22 +2025,11 @@ export function createEngine(canvas: HTMLCanvasElement, cb: EngineCallbacks): En
       // the selection.
       cb.onFocusChange?.(info);
 
-      // Camera focus tween — same setup as selectFamous / focusOn.
-      const cam = state.cam;
-      if (!cam) return;
-      state.subsystems.tweens.start({
-        startMs: performance.now(),
-        durationMs: FOCUS_TWEEN_MS,
-        fromTarget: vec3.clone(cam.target as vec3),
-        toTarget: vec3.fromValues(info.x, info.y, info.z),
-        fromDistance: cam.distance,
-        toDistance: focusDistanceMpc(info.diameterKpc),
-        fromYaw: cam.yaw,
-        toYaw: cam.yaw,
-        fromPitch: cam.pitch,
-        toPitch: cam.pitch,
-      });
-      state.subsystems.scheduler.requestRender();
+      // Camera focus tween — same setup as selectFamous / focusOn,
+      // routed through the shared `tweenToGalaxy` kernel.  The helper's
+      // own cam-null guard absorbs the post-destroy / pre-bootstrap
+      // race so this method doesn't need a local one.
+      tweenToGalaxy(state, info);
     },
 
     focusOnHome() {

src/services/engine/tweenToGalaxy.ts

@@ -0,0 +1,138 @@
+/**
+ * tweenToGalaxy — kick off a focus camera tween toward a galaxy.
+ *
+ * ### Why a helper
+ *
+ * Three public-handle methods on `EngineHandle` — `focusOn`, `selectFamous`,
+ * and `selectByAlias` — each carried the same five-line block:
+ *
+ * ```ts
+ * state.subsystems.tweens.start({
+ *   startMs: performance.now(),
+ *   durationMs: FOCUS_TWEEN_MS,
+ *   fromTarget: vec3.clone(cam.target as vec3),
+ *   toTarget: vec3.fromValues(info.x, info.y, info.z),
+ *   fromDistance: cam.distance,
+ *   toDistance: focusDistanceMpc(info.diameterKpc),
+ *   fromYaw: cam.yaw,
+ *   toYaw: cam.yaw,
+ *   fromPitch: cam.pitch,
+ *   toPitch: cam.pitch,
+ * });
+ * state.subsystems.scheduler.requestRender();
+ * ```
+ *
+ * Three near-identical bodies — exactly the kind of copy-paste that
+ * silently rots when one site adds a new field (e.g. an FOV transition)
+ * and the others lag behind.  One helper, three call sites collapse to
+ * a single line each.
+ *
+ * ### Why we DON'T extend the responsibility
+ *
+ * Each call site has its own pre-tween bookkeeping that doesn't belong
+ * in the helper:
+ *   - `selectFamous` / `selectByAlias` resolve a `PointInfo` via
+ *     `buildPointInfo`, then call `setSelected` and `cb.onFocusChange`
+ *     before tweening;
+ *   - `focusOn` calls `cb.onFocusChange` first so the URL hash updates
+ *     in lock-step with the user's commitment.
+ *
+ * Pulling that work into `tweenToGalaxy` would force the helper to know
+ * about callbacks, selection state, and source enums — turning a
+ * five-line dispatcher into a multi-purpose coordinator.  The single
+ * responsibility we DO want is "given a camera-able target, start the
+ * tween" — keep it tiny.
+ *
+ * ### Why the cam-null guard is here
+ *
+ * `state.cam` is typed `OrbitCamera | null` because two reachable
+ * windows leave it null:
+ *   - **Pre-bootstrap**: `createOrbitCamera` runs inside `wireInput`
+ *     during the bootstrap IIFE, after `initGpu` and the first cloud
+ *     arrival.  Code that fires before then (e.g. an unlikely
+ *     `selectByAlias` from a deep-link drain that races the very first
+ *     cloud upload) still finds `cam` null.
+ *   - **Post-destroy**: `handle.destroy()` detaches controls and clears
+ *     `state.cam = null`.  An in-flight focus promise that resolves
+ *     after destroy must not crash the engine on shutdown.
+ *
+ * The three call sites all check for null themselves today — the helper
+ * absorbs that check so future call sites get the safe behaviour for
+ * free.  It is genuinely needed; do not remove on the grounds of YAGNI.
+ *
+ * ### Why `TweenTarget` is a structural minimum, not `PointInfo`
+ *
+ * The helper only reads four fields off the target: `x`, `y`, `z`, and
+ * `diameterKpc`.  Declaring the parameter as `PointInfo` would imply
+ * the helper might reach for ra/dec/redshift/etc., which it never does.
+ * The minimum-surface type doubles as documentation: "this is exactly
+ * the data the tween needs."  Production callers pass a full
+ * `PointInfo` and TypeScript accepts it via structural compatibility.
+ */
+
+import { vec3 } from 'gl-matrix';
+
+import type { EngineState } from '../../@types';
+import { FOCUS_TWEEN_MS, focusDistanceMpc } from './focusTween';
+
+/**
+ * The minimum-surface descriptor the helper actually consumes.
+ *
+ * Every existing call site already has a `PointInfo` in scope (built
+ * via `buildPointInfo`), and `PointInfo` is structurally a superset of
+ * this type — so passing `info` directly works without any field
+ * extraction at the call site.
+ */
+export type TweenTarget = {
+  /** World-space X in Mpc. */
+  x: number;
+  /** World-space Y in Mpc. */
+  y: number;
+  /** World-space Z in Mpc. */
+  z: number;
+  /**
+   * Physical galaxy diameter in kpc — drives the focus distance via
+   * `focusDistanceMpc`.  Callers that genuinely lack a diameter (none
+   * today) should pass the project-wide fallback explicitly rather
+   * than letting NaN through.
+   */
+  diameterKpc: number;
+};
+
+/**
+ * Start a focus tween toward `target`, snapshotting the current camera
+ * pose so an in-flight tween hands off smoothly to the new one.
+ *
+ * Yaw and pitch are preserved — the user keeps their orientation; only
+ * the orbit target and distance change.  The duration is the
+ * project-wide `FOCUS_TWEEN_MS`; the destination distance is derived
+ * from `target.diameterKpc` via `focusDistanceMpc` (which clamps to a
+ * sensible minimum so dwarfs don't end up framing the camera inside
+ * the disk).
+ *
+ * No-op when `state.cam` is null — see the module header for the two
+ * windows where that happens.
+ */
+export function tweenToGalaxy(state: EngineState, target: TweenTarget): void {
+  const cam = state.cam;
+  if (!cam) return;
+
+  state.subsystems.tweens.start({
+    startMs: performance.now(),
+    durationMs: FOCUS_TWEEN_MS,
+    // vec3.clone copies the target tuple so later mutation of
+    // cam.target (the next frame's orbit-controls update, an
+    // interrupting tween, …) doesn't corrupt the from-snapshot.
+    fromTarget: vec3.clone(cam.target as vec3),
+    toTarget: vec3.fromValues(target.x, target.y, target.z),
+    fromDistance: cam.distance,
+    toDistance: focusDistanceMpc(target.diameterKpc),
+    fromYaw: cam.yaw,
+    toYaw: cam.yaw,
+    fromPitch: cam.pitch,
+    toPitch: cam.pitch,
+  });
+  // Wake the render loop — the tween's per-frame advance keeps it
+  // ticking via the still-animating predicate until completion.
+  state.subsystems.scheduler.requestRender();
+}

tests/services/engine/tweenToGalaxy.test.ts

@@ -0,0 +1,122 @@
+/**
+ * tweenToGalaxy — unit tests for the camera-tween-to-galaxy helper.
+ *
+ * Three public-handle methods (`focusOn`, `selectFamous`, `selectByAlias`)
+ * each open-coded the same five-line "build a CameraTween from a galaxy's
+ * world-space position + diameter, hand it to the tween manager, kick the
+ * scheduler" block.  `tweenToGalaxy` reifies that block as a single
+ * helper.  These tests exercise it without spinning up the full engine:
+ *
+ *   - happy path: the tween descriptor lands in `tweens.start` with
+ *     to-target = (info.x, info.y, info.z), to-distance derived from
+ *     diameterKpc, and from-* snapshots cloned off the live camera so
+ *     interrupting an in-flight tween hands off smoothly;
+ *   - cam-null guard: when the engine has been destroyed (or is still
+ *     bootstrapping pre-`startLoop`) `state.cam` is null — the helper
+ *     must short-circuit silently rather than dereference null.
+ *
+ * We intentionally do NOT re-test `focusDistanceMpc` here — its own test
+ * suite in `focusTween.test.ts` covers the diameter-to-distance math.
+ * This test only verifies the *plumbing* between the helper and the
+ * tween manager.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+
+import { tweenToGalaxy } from '../../../src/services/engine/tweenToGalaxy';
+import { focusDistanceMpc } from '../../../src/services/engine/focusTween';
+import type { EngineState } from '../../../src/@types';
+
+/**
+ * Build a minimal `EngineState`-shaped fixture that exposes only the
+ * fields `tweenToGalaxy` reads: `cam`, `subsystems.tweens.start`, and
+ * `subsystems.scheduler.requestRender`.  Casting through `unknown` keeps
+ * the test honest — if the helper ever reaches for a field outside this
+ * trio, the test will surface it as a runtime undefined rather than a
+ * silently-passing stub.
+ */
+function makeState(opts: {
+  cam: { target: [number, number, number]; distance: number; yaw: number; pitch: number } | null;
+  start: ReturnType<typeof vi.fn>;
+  requestRender: ReturnType<typeof vi.fn>;
+}): EngineState {
+  return {
+    cam: opts.cam,
+    subsystems: {
+      tweens: { start: opts.start },
+      scheduler: { requestRender: opts.requestRender },
+    },
+  } as unknown as EngineState;
+}
+
+describe('tweenToGalaxy', () => {
+  it('starts a CameraTween toward (info.x, info.y, info.z) with focusDistanceMpc(diameterKpc) and requests a render', () => {
+    const start = vi.fn();
+    const requestRender = vi.fn();
+    const cam = {
+      target: [1, 2, 3] as [number, number, number],
+      distance: 50,
+      yaw: 0.25,
+      pitch: -0.1,
+    };
+    const state = makeState({ cam, start, requestRender });
+
+    tweenToGalaxy(state, { x: 100, y: 200, z: 300, diameterKpc: 25 });
+
+    expect(start).toHaveBeenCalledOnce();
+    const tween = start.mock.calls[0]![0];
+    // toTarget is whatever vec3.fromValues produced — a 3-element array-like
+    // whose contents must equal (100, 200, 300).
+    expect(Array.from(tween.toTarget as ArrayLike<number>)).toEqual([100, 200, 300]);
+    expect(tween.toDistance).toBe(focusDistanceMpc(25));
+    // from-* snapshots come straight off the live camera.
+    expect(tween.fromDistance).toBe(50);
+    expect(tween.fromYaw).toBe(0.25);
+    expect(tween.fromPitch).toBe(-0.1);
+    // Yaw / pitch are preserved — the tween only moves target + distance.
+    expect(tween.toYaw).toBe(cam.yaw);
+    expect(tween.toPitch).toBe(cam.pitch);
+    // Duration is the project-wide focus tween length (sourced from
+    // focusTween.ts so this assertion stays honest if the constant moves).
+    expect(typeof tween.durationMs).toBe('number');
+    expect(tween.durationMs).toBeGreaterThan(0);
+    // startMs is `performance.now()`-shaped — finite, non-negative.
+    expect(Number.isFinite(tween.startMs)).toBe(true);
+    expect(tween.startMs).toBeGreaterThanOrEqual(0);
+
+    expect(requestRender).toHaveBeenCalledOnce();
+  });
+
+  it('clones cam.target so later mutation of cam.target does not corrupt the tween snapshot', () => {
+    const start = vi.fn();
+    const requestRender = vi.fn();
+    const cam = {
+      target: [1, 2, 3] as [number, number, number],
+      distance: 10,
+      yaw: 0,
+      pitch: 0,
+    };
+    const state = makeState({ cam, start, requestRender });
+
+    tweenToGalaxy(state, { x: 0, y: 0, z: 0, diameterKpc: 30 });
+
+    const captured = start.mock.calls[0]![0].fromTarget as ArrayLike<number>;
+    // Mutate the live camera target after the tween was started.
+    cam.target[0] = 999;
+    cam.target[1] = 999;
+    cam.target[2] = 999;
+    // The captured snapshot must be unchanged.
+    expect(Array.from(captured)).toEqual([1, 2, 3]);
+  });
+
+  it('is a no-op when state.cam is null (post-destroy / pre-startLoop race window)', () => {
+    const start = vi.fn();
+    const requestRender = vi.fn();
+    const state = makeState({ cam: null, start, requestRender });
+
+    tweenToGalaxy(state, { x: 100, y: 200, z: 300, diameterKpc: 25 });
+
+    expect(start).not.toHaveBeenCalled();
+    expect(requestRender).not.toHaveBeenCalled();
+  });
+});