diff --git a/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx b/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx index bed2d6af..4aa36e7a 100644 --- a/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx +++ b/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx @@ -1120,11 +1120,9 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap const [activeNodeCount, setActiveNodeCount] = useState(null); const [temporalBounds, setTemporalBounds] = useState(null); const [scrubberTime, setScrubberTime] = useState(null); - // Tracks the millisecond value of the last time setScrubberTime was called, - // so the TimelinePanel onTimeChange callback can skip redundant updates when - // React 18 concurrent mode re-runs the effect with a new Date object for the - // same timestamp (issue #830: redundant setScrubberTime calls → temporalState - // churn → diagnostics effect loop in dev mode). + // Deduplicates setScrubberTime calls by millisecond value so that React 18 + // concurrent-mode re-renders with a new Date object for the same timestamp + // do not churn temporalState and retrigger the diagnostics effect (issue #830). const lastScrubberMsRef = useRef(null); const onTimeChange = useCallback((time: Date) => { const ms = time.getTime(); @@ -2293,12 +2291,11 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap return; } - // Compare against the last accepted snapshot synchronously via a ref — - // this prevents React from entering the loop at all, rather than bailing - // out inside the functional updater after a render has already been - // scheduled (issue #830: calling setGraphDiagnosticsState with a new - // object on every effect run caused a render→effect→setState→render cycle - // that exceeded React's max update depth in dev mode). + // Compare against the last accepted snapshot synchronously before calling + // setState. buildEffectAvailability always returns a new object, so an + // unconditional setGraphDiagnosticsState on every call created a + // render → diagnostics effect → setState → render cycle that exceeded + // React's max update depth in dev mode (issue #830). const prev = lastDiagnosticsRef.current; if (prev !== null) { const EFFECT_KEYS = [ @@ -2328,8 +2325,8 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap prev.structureLayer?.lastDrawAt !== diagnostics.structureLayer?.lastDrawAt || prev.structureLayer?.enabled !== diagnostics.structureLayer?.enabled; - // distanceVisual comes from distanceVisualStateRef.current in GraphCanvas — - // same object reference when distances haven't changed. + // distanceVisual is compared by reference: GraphCanvas passes the same + // object when distances haven't changed. const distanceVisualChanged = prev.distanceVisual !== diagnostics.distanceVisual; if (!availabilityChanged && !edgeClassesChanged && !structureLayerChanged && !distanceVisualChanged) { diff --git a/explorer/src/workspaces/GraphWorkspace/pluginRegistryPredicates.ts b/explorer/src/workspaces/GraphWorkspace/pluginRegistryPredicates.ts index 3b059f97..e5a5331a 100644 --- a/explorer/src/workspaces/GraphWorkspace/pluginRegistryPredicates.ts +++ b/explorer/src/workspaces/GraphWorkspace/pluginRegistryPredicates.ts @@ -2,14 +2,9 @@ * 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). + * importing the full GraphWorkspace React component. Each predicate gates + * whether a plugin's module is lazily imported; none reference temporalState + * so temporal scrubber updates never retrigger plugin loading (issue #830). */ export type PluginShouldLoadContext = { diff --git a/explorer/tests/pluginRegistry.temporal.test.mjs b/explorer/tests/pluginRegistry.temporal.test.mjs index 5bf5caeb..8d5e156d 100644 --- a/explorer/tests/pluginRegistry.temporal.test.mjs +++ b/explorer/tests/pluginRegistry.temporal.test.mjs @@ -1,33 +1,19 @@ /** - * Regression tests for Issue #830: plugin registry shouldLoad predicates. + * Regression tests for issue #830: plugin registry shouldLoad predicates. * - * The original temporal-overlay shouldLoad was: - * ({ panelState, temporalState }) => - * Boolean(panelState["temporal-panel"] || temporalState?.currentTime) - * - * 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: 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. + * Imports the production predicates from pluginRegistryPredicates.ts so that + * a regression in GraphWorkspace.tsx is detected here. The key invariant: no + * predicate may read temporalState — doing so caused a render loop because + * temporalState.currentTime is non-null from startup, which triggered eager + * plugin loads on every scrubber update and continuously cancelled in-flight + * load() calls before they could register the plugin. */ import test from "node:test"; import assert from "node:assert/strict"; import { createRequire } from "node:module"; -// 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, @@ -44,9 +30,7 @@ test("temporal-overlay shouldLoad: false when panel is closed and no scrubber ti }); test("temporal-overlay shouldLoad: false when panel is closed even if scrubber time is set", () => { - // Before the fix this returned true — TimelinePanel sets currentTime on mount, - // causing eager loads that continuously reset the cancelled flag and prevented - // plugin registration. + // Before the fix, a non-null currentTime caused an eager load on every scrubber update. assert.equal( temporalOverlayShouldLoad({ panelState: { "temporal-panel": false }, @@ -86,8 +70,8 @@ test("neighborhood-panel shouldLoad: gates only on neighborhood-panel state", () }); test("all three shouldLoad conditions are consistent: none reference temporalState", () => { - // A predicate that regressed to reading temporalState?.currentTime would - // return true here even though every panel is closed — detecting the loop bug. + // A regressed predicate reading temporalState?.currentTime would return true + // for a closed panel when currentTime is set — detecting the loop bug. const nonNullTemporalState = { currentTime: new Date(), activeNodeCount: 6 }; assert.equal(