Skip to content

Commit 4bd12d7

Browse files
committed
fix(graph): close plan-review gaps before the RC cut
Review of phases 1-9 against plans/graph-ux-motion-and-api.md surfaced six deviations; all fixed here. - warmup now defaults ON (100 ticks / 250ms budget) as the plan and migrating-v8 specify; omitted `layout.warmup` previously resolved to 0 ticks, so the default entrance revealed the explosive first frames and the fitBounds spread bypass never engaged - edgeStyle domains go through resolveCategoricalDomain, so `sort` and `scale.domain` order dash styles instead of first-seen data order - provider-form camera flights (zoomToNode) keep following the tracked node after the flight converges, until sim alpha settles below 0.05 - onCameraChange now fires for user pan/zoom and instant fits, not just programmatic flights - its stated purpose is camera persistence and zoom UIs without polling - React StrictMode dev remounts play the entrance again: suppression now keys on changed mount deps, not a survives-cleanup boolean ref - update() churn counts edges added between two survivors, and the visual-only gate compares edge multisets (duplicate edges) Claude-Session: https://claude.ai/code/session_016rS7hi2g4bSzRBfnD1p2ji
1 parent 1e105ff commit 4bd12d7

12 files changed

Lines changed: 349 additions & 41 deletions

File tree

packages/engine/src/graphs/__tests__/encoding.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,35 @@ describe('resolveEdgeVisuals', () => {
372372
expect(fourthEdge.style).toBe('solid');
373373
});
374374

375+
it('assigns styles in ascending label order regardless of data order', () => {
376+
const styledEdges: GraphEdge[] = [
377+
{ source: 'a', target: 'b', kind: 'zeta' },
378+
{ source: 'b', target: 'c', kind: 'alpha' },
379+
{ source: 'a', target: 'c', kind: 'mid' },
380+
];
381+
const edges = resolveEdgeVisuals(styledEdges, { edgeStyle: { field: 'kind' } }, theme);
382+
383+
// Sorted domain [alpha, mid, zeta] → solid, dashed, dotted.
384+
expect(edges.find((e) => e.data.kind === 'alpha')!.style).toBe('solid');
385+
expect(edges.find((e) => e.data.kind === 'mid')!.style).toBe('dashed');
386+
expect(edges.find((e) => e.data.kind === 'zeta')!.style).toBe('dotted');
387+
});
388+
389+
it('sort: null keeps first-seen data order for style assignment', () => {
390+
const styledEdges: GraphEdge[] = [
391+
{ source: 'a', target: 'b', kind: 'zeta' },
392+
{ source: 'b', target: 'c', kind: 'alpha' },
393+
];
394+
const edges = resolveEdgeVisuals(
395+
styledEdges,
396+
{ edgeStyle: { field: 'kind', sort: null } },
397+
theme,
398+
);
399+
400+
expect(edges.find((e) => e.data.kind === 'zeta')!.style).toBe('solid');
401+
expect(edges.find((e) => e.data.kind === 'alpha')!.style).toBe('dashed');
402+
});
403+
375404
it('defaults to solid when no edgeStyle encoding', () => {
376405
const edges = resolveEdgeVisuals(basicEdges, {}, theme);
377406

packages/engine/src/graphs/__tests__/graph-resolvers.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,17 @@ describe('layout presets + seed + highlight', () => {
248248
expect(r.simulationConfig.warmupBudgetMs).toBe(250);
249249
});
250250

251+
it('warmup defaults ON when omitted (100 ticks)', () => {
252+
const r = compileGraph(base, compileOptions);
253+
expect(r.simulationConfig.warmupTicks).toBe(100);
254+
expect(r.simulationConfig.warmupBudgetMs).toBe(250);
255+
});
256+
257+
it('warmup: false disables warmup', () => {
258+
const r = compileGraph({ ...base, layout: { type: 'force', warmup: false } }, compileOptions);
259+
expect(r.simulationConfig.warmupTicks).toBe(0);
260+
});
261+
251262
it('captures nodeColor.highlight into initialHighlight against the resolved domain', () => {
252263
const r = compileGraph(
253264
{ ...base, encoding: { nodeColor: { field: 'g', type: 'nominal', highlight: ['X'] } } },

packages/engine/src/graphs/compile-graph.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -417,8 +417,7 @@ export function compileGraph(spec: unknown, options: CompileOptions): GraphCompi
417417
};
418418

419419
// 10. Build simulation config. Energy/settle presets provide defaults; raw
420-
// layout fields (chargeStrength, and the non-spec alphaDecay/velocityDecay)
421-
// always win over a preset.
420+
// layout fields (chargeStrength) always win over a preset.
422421
const collisionPadding = graphSpec.layout.collisionPadding ?? 2;
423422
const maxRadius =
424423
compiledNodes.length > 0
@@ -432,14 +431,16 @@ export function compileGraph(spec: unknown, options: CompileOptions): GraphCompi
432431
? SETTLE_PRESETS[graphSpec.layout.settle]
433432
: SETTLE_PRESETS.balanced;
434433

435-
// Warmup: true → defaults, number → explicit tick count, false/undefined → 0.
434+
// Warmup defaults ON: undefined/true → default ticks, number → explicit tick
435+
// count, false → 0. It lives in layout (not animation) so `animation: false`
436+
// still gets the off-screen settle instead of the explosive first frames.
436437
const warmupRaw = graphSpec.layout.warmup;
437438
const warmupTicks =
438-
warmupRaw === true
439-
? DEFAULT_WARMUP_TICKS
439+
warmupRaw === false
440+
? 0
440441
: typeof warmupRaw === 'number'
441442
? Math.max(0, Math.floor(warmupRaw))
442-
: 0;
443+
: DEFAULT_WARMUP_TICKS;
443444

444445
const simulationConfig: SimulationConfig = {
445446
chargeStrength: graphSpec.layout.chargeStrength ?? energyPreset.chargeStrength,

packages/engine/src/graphs/encoding.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -410,10 +410,14 @@ export function resolveEdgeVisuals(
410410
let styleFn: ((edge: GraphEdge) => 'solid' | 'dashed' | 'dotted') | undefined;
411411
if (encoding.edgeStyle?.field) {
412412
const field = encoding.edgeStyle.field;
413-
const uniqueValues = [...new Set(edges.map((e) => String(e[field] ?? '')))];
413+
const domain = resolveCategoricalDomain(
414+
edges.map((e) => String(e[field] ?? '')),
415+
encoding.edgeStyle.sort,
416+
encoding.edgeStyle.scale?.domain,
417+
);
414418
const styleMap = new Map<string, 'solid' | 'dashed' | 'dotted'>();
415-
for (let i = 0; i < uniqueValues.length; i++) {
416-
styleMap.set(uniqueValues[i], EDGE_STYLES[i % EDGE_STYLES.length]);
419+
for (let i = 0; i < domain.length; i++) {
420+
styleMap.set(domain[i], EDGE_STYLES[i % EDGE_STYLES.length]);
417421
}
418422
styleFn = (edge: GraphEdge) => styleMap.get(String(edge[field] ?? '')) ?? 'solid';
419423
}

packages/react/src/Graph.tsx

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -131,9 +131,13 @@ export const Graph = forwardRef<GraphHandle, GraphProps>(function Graph(
131131
const containerRef = useRef<HTMLDivElement>(null);
132132
const graphRef = useRef<GraphInstance | null>(null);
133133
const specRef = useRef<string>('');
134-
// First run of the mount effect plays the entrance; subsequent runs (theme/
135-
// darkMode/structural-tooltip/legend change — all spec-unchanged) suppress it.
136-
const mountedOnceRef = useRef(false);
134+
// Deps of the previous mount-effect run. A recreation with CHANGED deps
135+
// (theme/darkMode/structural-tooltip/legend — all spec-unchanged) suppresses
136+
// the entrance; identical deps mean a StrictMode dev replay of the same
137+
// mount, which must still play the entrance. A survives-cleanup boolean ref
138+
// can't tell those apart — it would suppress the entrance in every
139+
// StrictMode app.
140+
const prevMountDepsRef = useRef<unknown[] | null>(null);
137141

138142
// Store event handlers AND function-valued options in refs so they don't
139143
// trigger graph recreation. Inline functions create new references every
@@ -287,6 +291,14 @@ export const Graph = forwardRef<GraphHandle, GraphProps>(function Graph(
287291
? { formatter: stableTooltipFormatter }
288292
: false;
289293

294+
// Suppress the entrance only when a PREVIOUS mount existed and a dep
295+
// actually changed (theme/darkMode/structural option — spec unchanged).
296+
// Identical deps = StrictMode's dev remount of the same graph: play it.
297+
const mountDeps: unknown[] = [theme, resolvedDarkMode, tooltipOn, legendKey, fitOnLoad];
298+
const prevDeps = prevMountDepsRef.current;
299+
const suppressEntrance =
300+
prevDeps !== null && mountDeps.some((d, i) => !Object.is(d, prevDeps[i]));
301+
290302
const options: GraphMountOptions = {
291303
theme,
292304
darkMode: resolvedDarkMode,
@@ -303,14 +315,12 @@ export const Graph = forwardRef<GraphHandle, GraphProps>(function Graph(
303315
onHighlightChange: stableOnHighlightChange,
304316
onCameraChange: stableOnCameraChange,
305317
responsive: true,
306-
// First mount plays the entrance; theme/darkMode-only recreations suppress
307-
// it so the reveal doesn't replay on an unchanged spec.
308-
suppressEntrance: mountedOnceRef.current,
318+
suppressEntrance,
309319
};
310320

311321
graphRef.current = createGraph(container, spec, options);
312322
specRef.current = JSON.stringify(spec);
313-
mountedOnceRef.current = true;
323+
prevMountDepsRef.current = mountDeps;
314324

315325
return () => {
316326
graphRef.current?.destroy();

packages/react/src/__tests__/Graph.suppress-entrance.test.tsx

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
import type { GraphSpec } from '@opendata-ai/openchart-core';
1111
import { cleanup, render, waitFor } from '@testing-library/react';
12-
import { createRef } from 'react';
12+
import { createRef, StrictMode } from 'react';
1313
import { afterEach, describe, expect, it, vi } from 'vitest';
1414

1515
// A stub GraphInstance whose methods we can assert against.
@@ -75,6 +75,19 @@ describe('<Graph /> suppressEntrance', () => {
7575
expect(secondOpts.suppressEntrance).toBe(true);
7676
});
7777

78+
it('StrictMode dev remount still plays the entrance (identical deps, no suppression)', async () => {
79+
render(
80+
<StrictMode>
81+
<Graph spec={spec} />
82+
</StrictMode>,
83+
);
84+
// StrictMode runs mount → cleanup → mount; the REAL (second) mount must not
85+
// be mistaken for a theme-style recreation.
86+
await waitFor(() => expect(createGraphMock).toHaveBeenCalledTimes(2));
87+
const lastOpts = createGraphMock.mock.calls.at(-1)?.[2] as { suppressEntrance?: boolean };
88+
expect(lastOpts.suppressEntrance).toBe(false);
89+
});
90+
7891
it('tooltip formatter change does NOT recreate the graph (rides the trampoline)', async () => {
7992
const { rerender } = render(<Graph spec={spec} tooltip={{ formatter: () => 'one' }} />);
8093
await waitFor(() => expect(createGraphMock).toHaveBeenCalledTimes(1));

packages/vanilla/src/graph-mount.ts

Lines changed: 53 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,12 @@ import type {
2020
GraphCompilation,
2121
} from '@opendata-ai/openchart-engine';
2222
import { buildEdgeTooltip, compileGraph } from '@opendata-ai/openchart-engine';
23-
import { type CameraFlightOptions, clampK, createCameraFlight } from './graph/camera';
23+
import {
24+
type CameraFlightOptions,
25+
clampK,
26+
createCameraFlight,
27+
createCameraFollow,
28+
} from './graph/camera';
2429
import { GraphCanvasRenderer } from './graph/canvas-renderer';
2530
import { ENTRANCE_STAGGER_MAX_NODES } from './graph/entrance';
2631
import {
@@ -180,6 +185,9 @@ const CURSOR_FORCE_MAX_NODES = 2000;
180185
/** Cursor pointer-feed throttle (~30Hz) so we don't post on every mousemove. */
181186
const CURSOR_POINTER_THROTTLE_MS = 33;
182187

188+
/** Post-flight camera follow stops once the sim alpha settles below this. */
189+
const FOLLOW_SETTLE_ALPHA = 0.05;
190+
183191
// ---------------------------------------------------------------------------
184192
// Main API
185193
// ---------------------------------------------------------------------------
@@ -240,6 +248,10 @@ export function createGraph(
240248
let lastPointerFeedTime = 0;
241249
// Camera flight state.
242250
let activeFlight: GraphAnimation | null = null;
251+
// Post-flight follow for provider-form flights (tracks a still-settling node).
252+
let activeFollow: GraphAnimation | null = null;
253+
// Latest simulation alpha, fed by onTick; the follow stops below the threshold.
254+
let lastAlpha = 1;
243255
let cameraChangePending = false;
244256

245257
// Focus / highlight state (Phase 5). One highlight slot with two writers:
@@ -635,13 +647,18 @@ export function createGraph(
635647
cursorRepulsion: cursorForceEnabled() ? compilation.interaction.cursorRepulsion : null,
636648
});
637649

650+
// A fresh sim starts hot; don't let a settled previous sim's alpha linger
651+
// (it would end a post-flight camera follow before the first tick lands).
652+
lastAlpha = opts?.initialAlpha ?? 1;
653+
638654
let initialSettleDone = false;
639655
// Update sims keep the current camera and don't run the entrance reveal, so
640656
// pre-mark the fit as done — the first update tick just streams positions.
641657
let initialFitDone = opts?.skipEntrance ?? false;
642658

643-
simulation.onTick((positions, _alpha) => {
659+
simulation.onTick((positions, alpha) => {
644660
if (destroyed) return;
661+
lastAlpha = alpha;
645662

646663
// Build position lookup
647664
const posMap = new Map<string, { x: number; y: number }>();
@@ -728,6 +745,7 @@ export function createGraph(
728745

729746
if (suppressed || !enter || prefersReducedMotion()) {
730747
interactionManager.setTransform(fit);
748+
cameraChangePending = true;
731749
entranceActive = false;
732750
entranceProgress = 1;
733751
return;
@@ -737,6 +755,7 @@ export function createGraph(
737755
const { width: cw, height: ch } = getCanvasDimensions();
738756
const pulledBack = fit.zoomAt(fit.k * 0.92, cw / 2, ch / 2);
739757
interactionManager.setTransform(pulledBack);
758+
cameraChangePending = true;
740759

741760
entranceActive = true;
742761
entranceProgress = 0;
@@ -1072,11 +1091,8 @@ export function createGraph(
10721091
): void {
10731092
if (destroyed || !interactionManager) return;
10741093

1075-
// Cancel any in-flight camera animation.
1076-
if (activeFlight) {
1077-
scheduler.remove(activeFlight);
1078-
activeFlight = null;
1079-
}
1094+
// Cancel any in-flight camera animation (and a lingering post-flight follow).
1095+
cancelFlight();
10801096

10811097
const cameraCfg = compilation.animation?.camera ?? null;
10821098
const resolveTarget = () => (typeof to === 'function' ? to() : to);
@@ -1111,6 +1127,9 @@ export function createGraph(
11111127
activeFlight = null;
11121128
isGesturing = false;
11131129
needsRender = true;
1130+
// Provider-form flights converge at t=1 while the tracked node may
1131+
// still be settling — keep following it until the sim quiets down.
1132+
if (typeof to === 'function') startFollow(to);
11141133
scheduleRender();
11151134
onDone?.();
11161135
},
@@ -1120,13 +1139,32 @@ export function createGraph(
11201139
scheduler.add(flight);
11211140
}
11221141

1123-
/** Cancel any active camera flight (called by user-initiated pan/zoom). */
1142+
/** Snap the camera to a provider each frame until the sim settles. */
1143+
function startFollow(target: () => ZoomTransform): void {
1144+
const follow = createCameraFollow({
1145+
target,
1146+
apply: (t) => {
1147+
interactionManager!.setTransform(t);
1148+
cameraChangePending = true;
1149+
needsRender = true;
1150+
},
1151+
isActive: () => !destroyed && lastAlpha >= FOLLOW_SETTLE_ALPHA,
1152+
});
1153+
activeFollow = follow;
1154+
scheduler.add(follow);
1155+
}
1156+
1157+
/** Cancel any active camera flight/follow (called by user-initiated pan/zoom). */
11241158
function cancelFlight(): void {
11251159
if (activeFlight) {
11261160
scheduler.remove(activeFlight);
11271161
activeFlight = null;
11281162
isGesturing = false;
11291163
}
1164+
if (activeFollow) {
1165+
scheduler.remove(activeFollow);
1166+
activeFollow = null;
1167+
}
11301168
}
11311169

11321170
// ---------------------------------------------------------------------------
@@ -1147,6 +1185,9 @@ export function createGraph(
11471185
// node-drag correctly doesn't reach here either.
11481186
cancelFlight();
11491187
markGesture();
1188+
// User pan/zoom is a camera change too — zoom UIs and camera
1189+
// persistence rely on the coalesced onCameraChange, not polling.
1190+
cameraChangePending = true;
11501191
needsRender = true;
11511192
scheduleRender();
11521193
},
@@ -1461,6 +1502,7 @@ export function createGraph(
14611502
cancelFlight();
14621503
entranceFitInFlight = false;
14631504
interactionManager.setTransform(computeInitialFit());
1505+
cameraChangePending = true;
14641506
}
14651507

14661508
needsRender = true;
@@ -1594,10 +1636,10 @@ export function createGraph(
15941636
Math.max(prevNodeCount, nextNodeCount),
15951637
);
15961638
const prevEdgeCount =
1597-
compilation.edges.length - enteringEdgeCount(diff) + diff.exitingEdges.length;
1639+
compilation.edges.length - diff.enteringEdgeCount + diff.exitingEdges.length;
15981640
const nextEdgeCount = compilation.edges.length;
15991641
const edgeRatio = ratio(
1600-
enteringEdgeCount(diff) + diff.exitingEdges.length,
1642+
diff.enteringEdgeCount + diff.exitingEdges.length,
16011643
Math.max(prevEdgeCount, nextEdgeCount),
16021644
);
16031645
const changeRatio = Math.max(nodeRatio, edgeRatio);
@@ -1639,17 +1681,6 @@ export function createGraph(
16391681
scheduleRender();
16401682
}
16411683

1642-
/** Count of edges in the new compilation touching an entering node. */
1643-
function enteringEdgeCount(diff: ReturnType<typeof diffGraphUpdate>): number {
1644-
if (diff.enteringIds.length === 0) return 0;
1645-
const entering = new Set(diff.enteringIds);
1646-
let count = 0;
1647-
for (const e of compilation.edges) {
1648-
if (entering.has(e.source) || entering.has(e.target)) count++;
1649-
}
1650-
return count;
1651-
}
1652-
16531684
/** Safe ratio (0 when the denominator is 0). */
16541685
function ratio(numerator: number, denominator: number): number {
16551686
return denominator > 0 ? numerator / denominator : 0;
@@ -1776,6 +1807,7 @@ export function createGraph(
17761807
// writes to removed state.
17771808
scheduler.cancelAll();
17781809
activeFlight = null;
1810+
activeFollow = null;
17791811
// Reset entrance state so a remount (React StrictMode) starts clean.
17801812
entranceReveal = null;
17811813
entranceActive = false;

0 commit comments

Comments
 (0)