From 80de3652cf0f6ab4f277352818704eefb4926e8a Mon Sep 17 00:00:00 2001 From: Sameer6305 Date: Wed, 5 Aug 2026 17:37:33 +0530 Subject: [PATCH] fixed qodo findings Two issues addressed: 1. Plugin-loading useEffect unnecessarily depended on temporalState. After the #830 fix, no shouldLoad predicate reads temporalState, but the effect's dep array still included it, causing extra re-runs on every scrubber update. Removed temporalState from the dep array and the shouldLoad call site. Made temporalState optional in the LazyPluginRegistryEntry shouldLoad context type to match. 2. Regression test imported a local copy of shouldLoad instead of the production predicate. Extracted all three shouldLoad predicates into pluginRegistryPredicates.ts (pure module, no React/DOM dependencies), wired GraphWorkspace.tsx to use the imported functions, and updated the test to import and exercise the real production code via tsx. Verified: introducing the old broken condition causes the test to fail; the correct implementation passes all 7 assertions. --- explorer/package.json | 2 +- .../GraphWorkspace/GraphWorkspace.tsx | 13 +-- .../pluginRegistryPredicates.ts | 29 ++++++ .../tests/pluginRegistry.temporal.test.mjs | 94 +++++++++---------- 4 files changed, 81 insertions(+), 57 deletions(-) create mode 100644 explorer/src/workspaces/GraphWorkspace/pluginRegistryPredicates.ts 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, ); });