diff --git a/explorer/package.json b/explorer/package.json index 8169e3f6..72e36f73 100644 --- a/explorer/package.json +++ b/explorer/package.json @@ -10,7 +10,7 @@ "preview": "vite preview", "test:graph-store": "node --test tests/graphStore.multi-edge.test.mjs", "test:graph-workspace": "node --import tsx --test tests/graphSceneState.display.test.ts", - "test:plugin-registry": "node --test tests/pluginRegistry.temporal.test.mjs" + "test:plugin-registry": "node --import tsx --test tests/pluginRegistry.temporal.test.mjs" }, "dependencies": { "@monaco-editor/react": "^4.7.0", diff --git a/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx b/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx index ef1bcfd6..bed2d6af 100644 --- a/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx +++ b/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx @@ -38,6 +38,7 @@ import { type GraphPluginPanelDescriptor, type GraphPluginToolbarItem, } from "./plugins"; +import { explorationEffectsShouldLoad, neighborhoodPanelShouldLoad, temporalOverlayShouldLoad } from "./pluginRegistryPredicates"; import type { LinkPrediction, PathResponse } from "./GraphInspectorPanel"; import type { GraphSceneHandle, GraphSceneRuntime } from "./scene"; import type { @@ -126,7 +127,7 @@ type LazyPluginRegistryEntry = { load: () => Promise; shouldLoad: (context: { panelState: Record; - temporalState: GraphTemporalState | null; + temporalState?: GraphTemporalState | null; }) => boolean; }; @@ -2073,7 +2074,7 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap title: "Open exploration effects controls", order: 18, load: loadExplorationEffectsPlugin, - shouldLoad: ({ panelState }) => Boolean(panelState["effects-panel"]), + shouldLoad: explorationEffectsShouldLoad, }, { id: "neighborhood-panel", @@ -2082,7 +2083,7 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap title: "Toggle neighborhood panel", order: 30, load: loadNeighborhoodPanelPlugin, - shouldLoad: ({ panelState }) => Boolean(panelState["neighborhood-panel"]), + shouldLoad: neighborhoodPanelShouldLoad, }, { id: "temporal-overlay", @@ -2091,7 +2092,7 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap title: "Toggle temporal context panel", order: 40, load: loadTemporalOverlayPlugin, - shouldLoad: ({ panelState }) => Boolean(panelState["temporal-panel"]), + shouldLoad: temporalOverlayShouldLoad, }, ], [], @@ -2109,7 +2110,7 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap return; } - if (!entry.shouldLoad({ panelState: pluginPanelState, temporalState })) { + if (!entry.shouldLoad({ panelState: pluginPanelState })) { return; } @@ -2128,7 +2129,7 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap return () => { cancelled = true; }; - }, [loadedPlugins, pluginPanelState, pluginRegistry, temporalState]); + }, [loadedPlugins, pluginPanelState, pluginRegistry]); const setEffectToggle = useCallback((effect: GraphEffectToggle, enabled: boolean | ((current: boolean) => boolean)) => { setEffectsState((current) => { diff --git a/explorer/src/workspaces/GraphWorkspace/pluginRegistryPredicates.ts b/explorer/src/workspaces/GraphWorkspace/pluginRegistryPredicates.ts new file mode 100644 index 00000000..3b059f97 --- /dev/null +++ b/explorer/src/workspaces/GraphWorkspace/pluginRegistryPredicates.ts @@ -0,0 +1,29 @@ +/** + * shouldLoad predicates for the GraphWorkspace lazy plugin registry. + * + * Extracted into a pure module so the predicates can be unit-tested without + * importing the full GraphWorkspace React component (which depends on sigma, + * React hooks, and browser globals). The corresponding registry entries in + * GraphWorkspace.tsx must use these functions directly. + * + * These predicates gate WHEN each plugin's module is lazily imported. + * None of them reference temporalState — temporal scrubber updates must not + * retrigger plugin loading (see issue #830 for the render-loop that resulted + * from the temporal-overlay entry originally reading temporalState?.currentTime). + */ + +export type PluginShouldLoadContext = { + panelState: Record; +}; + +export function explorationEffectsShouldLoad({ panelState }: PluginShouldLoadContext): boolean { + return Boolean(panelState["effects-panel"]); +} + +export function neighborhoodPanelShouldLoad({ panelState }: PluginShouldLoadContext): boolean { + return Boolean(panelState["neighborhood-panel"]); +} + +export function temporalOverlayShouldLoad({ panelState }: PluginShouldLoadContext): boolean { + return Boolean(panelState["temporal-panel"]); +} diff --git a/explorer/tests/pluginRegistry.temporal.test.mjs b/explorer/tests/pluginRegistry.temporal.test.mjs index e2789785..5bf5caeb 100644 --- a/explorer/tests/pluginRegistry.temporal.test.mjs +++ b/explorer/tests/pluginRegistry.temporal.test.mjs @@ -1,51 +1,54 @@ /** - * Regression tests for Issue #830: temporal-overlay plugin shouldLoad condition. + * Regression tests for Issue #830: plugin registry shouldLoad predicates. * - * The original shouldLoad was: + * The original temporal-overlay shouldLoad was: * ({ panelState, temporalState }) => * Boolean(panelState["temporal-panel"] || temporalState?.currentTime) * - * This caused an infinite render loop because: - * 1. TimelinePanel calls onTimeChange(defaultTime) on mount, making - * temporalState.currentTime non-null from startup. - * 2. temporalState is a useMemo that produces a new object reference - * on every activeNodeCount / scrubberTime change. - * 3. The plugin-loading useEffect has temporalState in its dep array, - * so it re-runs on every temporal update. - * 4. With shouldLoad returning true from startup, entry.load() fired - * on every re-run while the previous async import was still in-flight, - * continuously setting cancelled = true on the prior run before - * setLoadedPlugins could be called, so loadedPlugins["temporal-overlay"] - * was never populated and the cycle never settled. + * This caused an infinite render loop because temporalState.currentTime is + * non-null from startup (TimelinePanel fires onTimeChange on mount), so the + * predicate returned true before the panel was ever opened, repeatedly + * triggering the plugin-loading useEffect during every scrubber update and + * cancelling in-flight load() calls before they could register the plugin. * - * The fix: use only panelState["temporal-panel"], matching the exact - * pattern of the other two registry entries (exploration-effects, - * neighborhood-panel) that have never exhibited this problem. + * The fix: each predicate reads only panelState so plugin loading is + * gated strictly on the user opening the corresponding panel. + * + * These tests import the PRODUCTION predicates from pluginRegistryPredicates.ts + * via tsx so that a future regression in GraphWorkspace.tsx is detected here. */ import test from "node:test"; import assert from "node:assert/strict"; +import { createRequire } from "node:module"; -// The fixed shouldLoad condition, extracted verbatim from GraphWorkspace.tsx. -// If this function is ever changed in GraphWorkspace.tsx, this test will catch -// a regression back to the temporalState-referencing form. -function temporalShouldLoad({ panelState }) { - return Boolean(panelState["temporal-panel"]); -} +// tsx is available as a Node loader — use createRequire to exercise the +// TypeScript module from this .mjs file. +const require = createRequire(import.meta.url); + +// tsx must be registered before requiring .ts files. The test:plugin-registry +// script calls this file via `node --import tsx --test`, so tsx is already +// active in the process when this module runs. +const { + explorationEffectsShouldLoad, + neighborhoodPanelShouldLoad, + temporalOverlayShouldLoad, +} = require("../src/workspaces/GraphWorkspace/pluginRegistryPredicates.ts"); + +// ── temporal-overlay ───────────────────────────────────────────────────────── test("temporal-overlay shouldLoad: false when panel is closed and no scrubber time", () => { assert.equal( - temporalShouldLoad({ panelState: { "temporal-panel": false } }), + temporalOverlayShouldLoad({ panelState: { "temporal-panel": false } }), false, ); }); test("temporal-overlay shouldLoad: false when panel is closed even if scrubber time is set", () => { - // Before the fix, this would return true because temporalState?.currentTime - // was included in the condition. TimelinePanel sets currentTime on mount, - // so this would have triggered an eager load before the user opened the panel, - // causing the render loop. + // Before the fix this returned true — TimelinePanel sets currentTime on mount, + // causing eager loads that continuously reset the cancelled flag and prevented + // plugin registration. assert.equal( - temporalShouldLoad({ + temporalOverlayShouldLoad({ panelState: { "temporal-panel": false }, temporalState: { currentTime: new Date() }, }), @@ -55,14 +58,14 @@ test("temporal-overlay shouldLoad: false when panel is closed even if scrubber t test("temporal-overlay shouldLoad: true only when the panel is explicitly opened", () => { assert.equal( - temporalShouldLoad({ panelState: { "temporal-panel": true } }), + temporalOverlayShouldLoad({ panelState: { "temporal-panel": true } }), true, ); }); test("temporal-overlay shouldLoad: true when panel opened even without a scrubber time", () => { assert.equal( - temporalShouldLoad({ + temporalOverlayShouldLoad({ panelState: { "temporal-panel": true }, temporalState: { currentTime: null }, }), @@ -70,43 +73,34 @@ test("temporal-overlay shouldLoad: true when panel opened even without a scrubbe ); }); -// Verify the other two registry entries' shouldLoad conditions are unchanged -// and still gate only on their respective panelState keys — establishing that -// they have never had and still don't have the temporalState cross-dependency. -function effectsShouldLoad({ panelState }) { - return Boolean(panelState["effects-panel"]); -} - -function neighborhoodShouldLoad({ panelState }) { - return Boolean(panelState["neighborhood-panel"]); -} +// ── other entries — confirm they also gate only on panelState ───────────────── test("exploration-effects shouldLoad: gates only on effects-panel state", () => { - assert.equal(effectsShouldLoad({ panelState: { "effects-panel": false } }), false); - assert.equal(effectsShouldLoad({ panelState: { "effects-panel": true } }), true); + assert.equal(explorationEffectsShouldLoad({ panelState: { "effects-panel": false } }), false); + assert.equal(explorationEffectsShouldLoad({ panelState: { "effects-panel": true } }), true); }); test("neighborhood-panel shouldLoad: gates only on neighborhood-panel state", () => { - assert.equal(neighborhoodShouldLoad({ panelState: { "neighborhood-panel": false } }), false); - assert.equal(neighborhoodShouldLoad({ panelState: { "neighborhood-panel": true } }), true); + assert.equal(neighborhoodPanelShouldLoad({ panelState: { "neighborhood-panel": false } }), false); + assert.equal(neighborhoodPanelShouldLoad({ panelState: { "neighborhood-panel": true } }), true); }); test("all three shouldLoad conditions are consistent: none reference temporalState", () => { - // A shouldLoad that references temporalState as a load trigger would return - // true even when the panel is closed, given a non-null currentTime. + // A predicate that regressed to reading temporalState?.currentTime would + // return true here even though every panel is closed — detecting the loop bug. const nonNullTemporalState = { currentTime: new Date(), activeNodeCount: 6 }; assert.equal( - temporalShouldLoad({ panelState: { "temporal-panel": false }, temporalState: nonNullTemporalState }), + temporalOverlayShouldLoad({ panelState: { "temporal-panel": false }, temporalState: nonNullTemporalState }), false, "temporal-overlay must not load when panel is closed, regardless of scrubber time", ); assert.equal( - effectsShouldLoad({ panelState: { "effects-panel": false }, temporalState: nonNullTemporalState }), + explorationEffectsShouldLoad({ panelState: { "effects-panel": false }, temporalState: nonNullTemporalState }), false, ); assert.equal( - neighborhoodShouldLoad({ panelState: { "neighborhood-panel": false }, temporalState: nonNullTemporalState }), + neighborhoodPanelShouldLoad({ panelState: { "neighborhood-panel": false }, temporalState: nonNullTemporalState }), false, ); });