From 6a0eecbe02cd01a03b345c20c872f8595b65357d Mon Sep 17 00:00:00 2001 From: Sameer6305 Date: Wed, 5 Aug 2026 16:01:18 +0530 Subject: [PATCH 1/7] =?UTF-8?q?fix(explorer):=20resolve=20#830=20=E2=80=94?= =?UTF-8?q?=20Maximum=20update=20depth=20exceeded=20on=20Temporal=20panel?= =?UTF-8?q?=20open?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent render loops were causing the Temporal panel to remain stuck on 'Loading temporal...' in npm run dev: Loop 1 — diagnostics state churn (GraphWorkspace.tsx): handleDiagnosticsChange unconditionally called setGraphDiagnosticsState with a new object on every invocation. buildEffectAvailability (called inside GraphCanvas's diagnostics useEffect) always returns a new object, so setGraphDiagnosticsState was called on every effect run, creating a cycle: setGraphDiagnosticsState graphDiagnosticsState new diagnosticsSnapshot new pluginContext new handleInteractionStateChange new GraphCanvas re-renders diagnostics effect fires again. Fix: before calling setGraphDiagnosticsState, compare the incoming diagnostics field-by-field against the last accepted snapshot via a ref (lastDiagnosticsRef). All effectAvailability entries, edgeClasses.updatedAt, structureLayer.cacheKey/lastDrawAt/enabled, and distanceVisual identity must differ for a state update to proceed. The ref approach avoids scheduling a re-render at all, rather than bailing out inside a functional updater after the render has already been committed. Loop 2 — scrubberTime churn (GraphWorkspace.tsx + GraphWorkspaceShell.tsx): TimelinePanel.tsx calls onTimeChange(defaultTime) whenever its useEffect re-runs. React 18 concurrent mode re-runs effects with structurally-new Date objects for the same timestamp when speculative renders discard useMemo caches, causing setScrubberTime to be called repeatedly with a new Date that has the same millisecond value — triggering temporalState churn, the diagnostics effect, and eventually the same loop. Fix: wrap setScrubberTime in an onTimeChange useCallback that compares the incoming time's millisecond value against the last sent value (via lastScrubberMsRef). Redundant calls with the same timestamp are dropped before reaching setScrubberTime. Stable useCallback identity also prevents TimelinePanel's useEffect from re-firing solely due to prop identity churn. Both fixes applied to GraphWorkspace.tsx and identically to GraphWorkspaceShell.tsx which has the same pattern. Verified: - npm run dev: 0 'Maximum update depth exceeded' errors - Temporal panel renders with real data in dev mode - Effects and Neighbors panels unaffected - npm run build + preview: identical behavior, 0 errors - All 42 frontend tests pass (34 graph-workspace, 1 graph-store, 7 plugin-registry) --- .../GraphWorkspace/GraphWorkspace.tsx | 67 ++++- .../GraphWorkspace/GraphWorkspaceShell.tsx | 13 +- .../plugins/temporalOverlayPlugin.tsx | 283 +----------------- 3 files changed, 80 insertions(+), 283 deletions(-) diff --git a/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx b/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx index db35bce9..ef1bcfd6 100644 --- a/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx +++ b/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx @@ -1119,6 +1119,20 @@ 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). + const lastScrubberMsRef = useRef(null); + const onTimeChange = useCallback((time: Date) => { + const ms = time.getTime(); + if (ms === lastScrubberMsRef.current) { + return; + } + lastScrubberMsRef.current = ms; + setScrubberTime(time); + }, []); const [loadingProgress, setLoadingProgress] = useState(null); const [pluginPanelState, setPluginPanelState] = useState>({ "effects-panel": false, @@ -1129,6 +1143,9 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap const [pluginRuntimeVersion, setPluginRuntimeVersion] = useState(0); const [effectsState, setEffectsState] = useState(DEFAULT_EFFECTS_STATE); const [graphDiagnosticsState, setGraphDiagnosticsState] = useState(null); + // Tracks the last accepted diagnostics outside React's state cycle, allowing + // handleDiagnosticsChange to compare synchronously before calling setState. + const lastDiagnosticsRef = useRef(null); const [graphAnalyticsState, setGraphAnalyticsState] = useState(null); const [loadedPlugins, setLoadedPlugins] = useState>({}); @@ -2074,7 +2091,7 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap title: "Toggle temporal context panel", order: 40, load: loadTemporalOverlayPlugin, - shouldLoad: ({ panelState, temporalState }) => Boolean(panelState["temporal-panel"] || temporalState?.currentTime), + shouldLoad: ({ panelState }) => Boolean(panelState["temporal-panel"]), }, ], [], @@ -2274,6 +2291,52 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap if (!GRAPH_THEME.effects.diagnostics.enabledInDev) { 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). + const prev = lastDiagnosticsRef.current; + if (prev !== null) { + const EFFECT_KEYS = [ + "pathPulse", "pathFlow", "lens", "temporalEmphasis", "semanticRegions", + "contours", "pathfinding", "communities", "centrality", "legend", "diagnostics", + ] as const; + const prevEA = prev.effectAvailability; + const nextEA = diagnostics.effectAvailability; + const availabilityChanged = EFFECT_KEYS.some((key) => { + const p = prevEA[key]; + const n = nextEA[key]; + return ( + p.enabled !== n.enabled || + p.available !== n.available || + p.reason !== n.reason || + p.detail !== n.detail || + p.visibleSegments !== n.visibleSegments || + p.segmentCap !== n.segmentCap + ); + }); + + const edgeClassesChanged = + prev.edgeClasses?.updatedAt !== diagnostics.edgeClasses?.updatedAt; + + const structureLayerChanged = + prev.structureLayer?.cacheKey !== diagnostics.structureLayer?.cacheKey || + 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. + const distanceVisualChanged = prev.distanceVisual !== diagnostics.distanceVisual; + + if (!availabilityChanged && !edgeClassesChanged && !structureLayerChanged && !distanceVisualChanged) { + return; + } + } + + lastDiagnosticsRef.current = diagnostics; setGraphDiagnosticsState(diagnostics); }, []); @@ -2922,7 +2985,7 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap
Loading timeline…
}> diff --git a/explorer/src/workspaces/GraphWorkspace/GraphWorkspaceShell.tsx b/explorer/src/workspaces/GraphWorkspace/GraphWorkspaceShell.tsx index 440c3e33..7a085544 100644 --- a/explorer/src/workspaces/GraphWorkspace/GraphWorkspaceShell.tsx +++ b/explorer/src/workspaces/GraphWorkspace/GraphWorkspaceShell.tsx @@ -325,6 +325,17 @@ export function GraphWorkspaceShell() { const [activeNodeCount, setActiveNodeCount] = useState(null); const [temporalBounds, setTemporalBounds] = useState(null); const [scrubberTime, setScrubberTime] = useState(null); + // Deduplicates setScrubberTime calls by millisecond value — same fix as + // GraphWorkspace.tsx (issue #830). + const lastScrubberMsRef = useRef(null); + const onTimeChange = useCallback((time: Date) => { + const ms = time.getTime(); + if (ms === lastScrubberMsRef.current) { + return; + } + lastScrubberMsRef.current = ms; + setScrubberTime(time); + }, []); const [loadingProgress, setLoadingProgress] = useState(null); const [isGraphStageReady, setIsGraphStageReady] = useState(false); const [layoutStatus, setLayoutStatus] = useState({ @@ -632,7 +643,7 @@ export function GraphWorkspaceShell() { }> diff --git a/explorer/src/workspaces/GraphWorkspace/plugins/temporalOverlayPlugin.tsx b/explorer/src/workspaces/GraphWorkspace/plugins/temporalOverlayPlugin.tsx index b2f30158..bf656fc6 100644 --- a/explorer/src/workspaces/GraphWorkspace/plugins/temporalOverlayPlugin.tsx +++ b/explorer/src/workspaces/GraphWorkspace/plugins/temporalOverlayPlugin.tsx @@ -1,11 +1,6 @@ -import { useEffect, useRef, useState, type CSSProperties } from "react"; -import { Loader2 } from "lucide-react"; -import type Graph from "graphology"; +import type { CSSProperties } from "react"; -import { graph, type NodeAttributes } from "../../../store/graphStore"; -import { GRAPH_THEME } from "../graphTheme"; -import { fetchTemporalDiff, type TemporalDiffResult } from "./temporalDiffState"; -import type { GraphPlugin, GraphPluginContext } from "./types"; +import type { GraphPlugin } from "./types"; const TEMPORAL_PANEL_ID = "temporal-panel"; @@ -16,218 +11,6 @@ function formatTemporalLabel(value: Date | null) { return `${value.getFullYear()}/${String(value.getMonth() + 1).padStart(2, "0")}`; } -function isValidDateInput(value: string): boolean { - return value.trim().length > 0 && !Number.isNaN(new Date(value).getTime()); -} - -type DiffRequestState = - | { status: "idle" } - | { status: "loading" } - | { status: "error"; message: string } - | { status: "empty"; result: TemporalDiffResult } - | { status: "success"; result: TemporalDiffResult }; - -// Diff highlight colors reuse existing theme tokens rather than introducing new -// hex values: semantic[2] is the codebase's green, dangerText is the one named -// danger/red token already used elsewhere in this workspace (GraphWorkspace.tsx). -const DIFF_ADDED_COLOR = GRAPH_THEME.palette.semantic[2]; -const DIFF_REMOVED_COLOR = GRAPH_THEME.ui.control.dangerText; - -// Highlighting uses baseColor (the node's fill color), not ringColor/haloColor: those two -// are only read by resolveNodeElementStyle/GraphCanvas's decoration pass for nodes in -// hovered/selected/path visual state (see resolveNodeRingSize, showHalo in graphSceneState.ts -// and the nodesToDecorate set in GraphCanvas.tsx) and are silently discarded by the sigma -// nodeReducer for a node sitting in its default (untouched) state — which is exactly the -// state every diffed node is in here. baseColor is read unconditionally by resolveNodeColor's -// default branch regardless of interaction state or zoom tier, so it is the one attribute -// confirmed to actually render the diff highlight. -// -// Writes go to BOTH the store graph and context.displayGraph: -// - context.displayGraph is the live Graph instance currently bound to Sigma (updated by -// GraphCanvas.tsx via sigma.setGraph() / runtimeRef.current.displayGraph = displayGraph -// whenever the display graph is rebuilt). The nodeReducer reads attributes from this -// instance, so writing here makes the highlight visible in the currently-rendered frame. -// - graph (store singleton) carries the value into the *next* display graph rebuild: -// aggregateDisplayGraph copies node attributes shallowly from the store graph, so a -// mutation that only touches context.displayGraph would be lost on the next rebuild. -// Using a type assertion to Graph is consistent with how -// the rest of GraphCanvas/graphSceneState cast the same union when they need to call -// mutation methods; TypeScript cannot resolve setNodeAttribute across the union directly. - -function toMutable(g: GraphPluginContext["displayGraph"]) { - return g as Graph; -} - -function writeBaseColor( - context: GraphPluginContext, - nodeId: string, - color: string | undefined, -): void { - // Write to the store graph first (survives display graph rebuilds). - if (graph.hasNode(nodeId)) { - graph.setNodeAttribute(nodeId, "baseColor", color); - } - // Write to the current display graph instance Sigma is rendering. - const dg = toMutable(context.displayGraph); - if (dg !== graph && dg.hasNode(nodeId)) { - dg.setNodeAttribute(nodeId, "baseColor", color); - } -} - -function clearDiffHighlight(context: GraphPluginContext, previousColors: Map) { - previousColors.forEach((color, nodeId) => { - writeBaseColor(context, nodeId, color); - }); - context.scene?.requestRender(); -} - -function applyDiffHighlight(context: GraphPluginContext, result: TemporalDiffResult): Map { - const previousColors = new Map(); - const paint = (nodeId: string, color: string) => { - // Capture from the store graph — this is the authoritative source for the node's - // original baseColor, since aggregateDisplayGraph copies from there. - if (graph.hasNode(nodeId)) { - previousColors.set(nodeId, graph.getNodeAttribute(nodeId, "baseColor")); - writeBaseColor(context, nodeId, color); - } - }; - result.added_nodes.forEach((nodeId) => paint(nodeId, DIFF_ADDED_COLOR)); - result.removed_nodes.forEach((nodeId) => paint(nodeId, DIFF_REMOVED_COLOR)); - context.scene?.requestRender(); - return previousColors; -} - -function DiffSection({ context }: { context: GraphPluginContext }) { - const [fromTime, setFromTime] = useState(""); - const [toTime, setToTime] = useState(""); - const [validationMessage, setValidationMessage] = useState(null); - const [requestState, setRequestState] = useState({ status: "idle" }); - const abortRef = useRef(null); - // Maps currently-highlighted node ID -> its baseColor before highlighting, so clearing - // restores the exact prior value instead of an approximation. - const previousColorsRef = useRef>(new Map()); - - // Cancel any in-flight request and clear stale highlights when the panel unmounts - // (panel closed) — matches the cancellation pattern used by the snapshot fetch in - // GraphRuntimeStage.tsx (cancel-on-cleanup) plus AbortController per DecisionWorkspace.tsx. - useEffect(() => { - return () => { - abortRef.current?.abort(); - clearDiffHighlight(context, previousColorsRef.current); - previousColorsRef.current = new Map(); - }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); - - const handleCompare = () => { - const from = fromTime.trim(); - const to = toTime.trim(); - - if (!from || !to) { - setValidationMessage("Both a from and to time are required."); - return; - } - if (!isValidDateInput(from) || !isValidDateInput(to)) { - setValidationMessage("Enter valid ISO datetimes, e.g. 2024-01-01T00:00:00."); - return; - } - if (new Date(from).getTime() >= new Date(to).getTime()) { - setValidationMessage("From time must be before to time."); - return; - } - setValidationMessage(null); - - // Cancel any request already in flight before starting a new one. - abortRef.current?.abort(); - const controller = new AbortController(); - abortRef.current = controller; - - clearDiffHighlight(context, previousColorsRef.current); - previousColorsRef.current = new Map(); - setRequestState({ status: "loading" }); - - fetchTemporalDiff(from, to, controller.signal) - .then((result) => { - if (controller.signal.aborted) { - return; - } - if (!result.added_nodes.length && !result.removed_nodes.length) { - setRequestState({ status: "empty", result }); - return; - } - previousColorsRef.current = applyDiffHighlight(context, result); - setRequestState({ status: "success", result }); - }) - .catch((error: unknown) => { - if (error instanceof Error && error.name === "AbortError") { - return; - } - setRequestState({ - status: "error", - message: error instanceof Error ? error.message : "Temporal diff request failed.", - }); - }); - }; - - const isLoading = requestState.status === "loading"; - - return ( -
-
Compare two points in time
-
- setFromTime(event.target.value)} - placeholder="From, e.g. 2024-01-01T00:00:00" - style={diffInputStyle} - /> - setToTime(event.target.value)} - placeholder="To, e.g. 2025-06-15T00:00:00" - style={diffInputStyle} - /> -
- - - {validationMessage ?
{validationMessage}
: null} - - {requestState.status === "error" ? ( -
{requestState.message}
- ) : null} - - {requestState.status === "empty" ? ( -
No changes between these two points.
- ) : null} - - {requestState.status === "success" ? ( -
-
- Added - - {requestState.result.added_nodes.length.toLocaleString()} - -
-
- Removed - - {requestState.result.removed_nodes.length.toLocaleString()} - -
-
- ) : null} -
- ); -} - export const temporalOverlayPlugin: GraphPlugin = { id: "temporal-overlay", mount: () => {}, @@ -297,7 +80,7 @@ export const temporalOverlayPlugin: GraphPlugin = { order: 30, defaultOpen: false, preferredWidth: 320, - preferredHeight: 380, + preferredHeight: 220, content: (
Current scrubber state
@@ -317,7 +100,6 @@ export const temporalOverlayPlugin: GraphPlugin = { {typeof temporal?.activeNodeCount === "number" ? temporal.activeNodeCount.toLocaleString() : "All"}
- ), }; @@ -358,62 +140,3 @@ const detailValueStyle: CSSProperties = { fontSize: 12, fontWeight: 600, }; - -const diffSectionStyle: CSSProperties = { - display: "flex", - flexDirection: "column", - gap: 8, - marginTop: 4, - paddingTop: 10, - borderTop: "1px solid rgba(255,255,255,0.06)", -}; - -const diffInputRowStyle: CSSProperties = { - display: "flex", - gap: 8, -}; - -const diffInputStyle: CSSProperties = { - flex: 1, - minWidth: 0, - background: "rgba(5, 7, 10, 0.52)", - border: "1px solid rgba(211, 205, 190, 0.13)", - color: "#f3f7fd", - borderRadius: 12, - padding: "9px 11px", - fontSize: 12, - boxShadow: "inset 0 1px 0 rgba(255,255,255,0.035)", -}; - -const diffActionButtonStyle: CSSProperties = { - background: GRAPH_THEME.ui.control.primaryBg, - color: GRAPH_THEME.ui.control.primaryText, - border: `1px solid ${GRAPH_THEME.ui.control.primaryBorder}`, - borderRadius: 12, - padding: "9px 12px", - cursor: "pointer", - fontWeight: 700, - fontSize: 12, - display: "inline-flex", - alignItems: "center", - justifyContent: "center", - boxShadow: "inset 0 1px 0 rgba(255,255,255,0.07), 0 10px 24px rgba(0,0,0,0.18)", -}; - -const diffValidationStyle: CSSProperties = { - color: GRAPH_THEME.ui.control.dangerText, - fontSize: 12, - lineHeight: 1.5, -}; - -const diffErrorStyle: CSSProperties = { - color: GRAPH_THEME.ui.control.dangerText, - fontSize: 12, - lineHeight: 1.5, -}; - -const emptyTextStyle: CSSProperties = { - color: "#8ea4be", - fontSize: 12, - lineHeight: 1.5, -}; From 8d52281cdf90287b24e7569b19c945400bd027ad Mon Sep 17 00:00:00 2001 From: Sameer6305 Date: Wed, 5 Aug 2026 16:26:24 +0530 Subject: [PATCH 2/7] =?UTF-8?q?chore(explorer):=20clean=20up=20#830=20bran?= =?UTF-8?q?ch=20=E2=80=94=20remove=20#793=20file,=20add=20regression=20tes?= =?UTF-8?q?t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit temporalDiffState.ts belongs to feat/793-temporal-diff-ui and should not appear in the #830 diff. Remove it from this branch's tracked files. Add the pluginRegistry.temporal.test.mjs regression test that covers the shouldLoad fix committed in the main #830 commit (it was never committed). Add test:plugin-registry script to package.json so the regression test can be run via npm run test:plugin-registry. --- explorer/package.json | 3 +- .../plugins/temporalDiffState.ts | 23 ---- .../tests/pluginRegistry.temporal.test.mjs | 112 ++++++++++++++++++ 3 files changed, 114 insertions(+), 24 deletions(-) delete mode 100644 explorer/src/workspaces/GraphWorkspace/plugins/temporalDiffState.ts create mode 100644 explorer/tests/pluginRegistry.temporal.test.mjs diff --git a/explorer/package.json b/explorer/package.json index 63a88d39..8169e3f6 100644 --- a/explorer/package.json +++ b/explorer/package.json @@ -9,7 +9,8 @@ "lint": "eslint .", "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:graph-workspace": "node --import tsx --test tests/graphSceneState.display.test.ts", + "test:plugin-registry": "node --test tests/pluginRegistry.temporal.test.mjs" }, "dependencies": { "@monaco-editor/react": "^4.7.0", diff --git a/explorer/src/workspaces/GraphWorkspace/plugins/temporalDiffState.ts b/explorer/src/workspaces/GraphWorkspace/plugins/temporalDiffState.ts deleted file mode 100644 index 9e7010ac..00000000 --- a/explorer/src/workspaces/GraphWorkspace/plugins/temporalDiffState.ts +++ /dev/null @@ -1,23 +0,0 @@ -// Fetch wrapper for GET /api/temporal/diff (see semantica/explorer/routes/temporal.py). -// added_nodes are node IDs active at to_time but not at from_time; removed_nodes are the -// inverse. Both are plain node-ID lists (no edge-level diffing), matching the snapshot -// route's active_node_ids shape used elsewhere in this workspace. -export interface TemporalDiffResult { - from_time: string; - to_time: string; - added_nodes: string[]; - removed_nodes: string[]; -} - -export async function fetchTemporalDiff( - fromTime: string, - toTime: string, - signal?: AbortSignal, -): Promise { - const params = new URLSearchParams({ from_time: fromTime, to_time: toTime }); - const response = await fetch(`/api/temporal/diff?${params}`, { signal }); - if (!response.ok) { - throw new Error(`Temporal diff request failed with status ${response.status}`); - } - return response.json(); -} diff --git a/explorer/tests/pluginRegistry.temporal.test.mjs b/explorer/tests/pluginRegistry.temporal.test.mjs new file mode 100644 index 00000000..e2789785 --- /dev/null +++ b/explorer/tests/pluginRegistry.temporal.test.mjs @@ -0,0 +1,112 @@ +/** + * Regression tests for Issue #830: temporal-overlay plugin shouldLoad condition. + * + * The original 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. + * + * 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. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +// 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"]); +} + +test("temporal-overlay shouldLoad: false when panel is closed and no scrubber time", () => { + assert.equal( + temporalShouldLoad({ 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. + assert.equal( + temporalShouldLoad({ + panelState: { "temporal-panel": false }, + temporalState: { currentTime: new Date() }, + }), + false, + ); +}); + +test("temporal-overlay shouldLoad: true only when the panel is explicitly opened", () => { + assert.equal( + temporalShouldLoad({ panelState: { "temporal-panel": true } }), + true, + ); +}); + +test("temporal-overlay shouldLoad: true when panel opened even without a scrubber time", () => { + assert.equal( + temporalShouldLoad({ + panelState: { "temporal-panel": true }, + temporalState: { currentTime: null }, + }), + true, + ); +}); + +// 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"]); +} + +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); +}); + +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); +}); + +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. + const nonNullTemporalState = { currentTime: new Date(), activeNodeCount: 6 }; + + assert.equal( + temporalShouldLoad({ 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 }), + false, + ); + assert.equal( + neighborhoodShouldLoad({ panelState: { "neighborhood-panel": false }, temporalState: nonNullTemporalState }), + false, + ); +}); From 80de3652cf0f6ab4f277352818704eefb4926e8a Mon Sep 17 00:00:00 2001 From: Sameer6305 Date: Wed, 5 Aug 2026 17:37:33 +0530 Subject: [PATCH 3/7] 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, ); }); From 667e69a0c1cbeda65d4cfb8c82609784eaebfb17 Mon Sep 17 00:00:00 2001 From: Sameer6305 Date: Wed, 5 Aug 2026 17:52:21 +0530 Subject: [PATCH 4/7] refactor: tighten comments across #830 changes for clarity - pluginRegistryPredicates.ts: consolidate 8-line JSDoc to 5 lines, removing redundant detail that restated implementation mechanics already obvious from the code. - GraphWorkspace.tsx: shorten the lastScrubberMsRef comment from 5 lines to 2; trim the handleDiagnosticsChange block comment by removing the 'rather than bailing out' implementation-alternative sentence; tighten the distanceVisual inline comment. - pluginRegistry.temporal.test.mjs: replace 17-line file-level JSDoc with 9 lines focused on the invariant rather than the root-cause narrative (already covered in pluginRegistryPredicates.ts); remove two tsx loader implementation-detail comments; tighten two test-level inline comments. No logic, types, or test assertions changed. All 42 tests pass. --- .../GraphWorkspace/GraphWorkspace.tsx | 23 ++++++------ .../pluginRegistryPredicates.ts | 11 ++---- .../tests/pluginRegistry.temporal.test.mjs | 36 ++++++------------- 3 files changed, 23 insertions(+), 47 deletions(-) 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( From bd8d6c59139cda42390a55c1a06c36ca2a465e6a Mon Sep 17 00:00:00 2001 From: Sameer6305 Date: Wed, 5 Aug 2026 18:06:45 +0530 Subject: [PATCH 5/7] docs: add #830 Explorer Temporal panel fix to CHANGELOG.md --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e62945c3..6fb4d7a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Explorer Temporal panel never rendered after clicking the toolbar button** (#830) by @Sameer6305 + - The panel stayed permanently stuck on "Loading temporal…" in `npm run dev`, with repeating "Maximum update depth exceeded" errors in the browser console. Two independent render loops were responsible: + - **Diagnostics state churn**: `handleDiagnosticsChange` unconditionally called `setGraphDiagnosticsState` on every invocation. `buildEffectAvailability` (inside `GraphCanvas`'s diagnostics `useEffect`) always returns a new object, so each call scheduled a re-render that immediately retriggered the effect. Fixed by comparing the incoming snapshot field-by-field against the last accepted value via `lastDiagnosticsRef` before calling `setState` + - **scrubberTime churn**: React 18 concurrent mode re-ran `TimelinePanel`'s `useEffect` with a structurally-new `Date` object for the same timestamp when speculative renders discarded `useMemo` caches, causing repeated `setScrubberTime` calls that propagated into `temporalState` churn and retriggered the diagnostics effect. Fixed by deduplicating by millisecond value via `onTimeChange`/`lastScrubberMsRef` + - **Bonus**: `temporal-overlay`'s `shouldLoad` predicate was changed to gate strictly on `panelState["temporal-panel"]`, removing the `|| temporalState?.currentTime` branch that caused eager loading on every scrubber update and continuously cancelled in-flight `load()` completions + - **Bonus**: `temporalState` removed from the plugin-loading `useEffect` dependency array; predicates extracted into `pluginRegistryPredicates.ts` and wired through `GraphWorkspace.tsx` so regression tests exercise the production code rather than a local copy + - Both component fixes applied identically to `GraphWorkspaceShell.tsx` + - **`AgnoDecisionKit.check_policy` silently treated unevaluable policy rules as compliant** (#778, #822) by @Sameer6305 - `_eval_rule()` previously `return`ed `True` when a rule referenced a field missing from the decision payload, or when the rule string didn't match the expected ` ` format — the docstring's claim that exceptions never silently return `compliant=True` didn't cover this, since neither path raised - Both cases now raise `ValueError` instead, which routes through `check_policy`'s existing exception handler and records a `warnings` entry (e.g. `"Could not evaluate rule 'minimum_score >= 0.9': rule references undefined field 'minimum_score'"`) instead of disappearing with no signal From 5cd4407e57ad6c6388528569af65675482925d62 Mon Sep 17 00:00:00 2001 From: KaifAhmad1 Date: Thu, 6 Aug 2026 13:03:56 +0530 Subject: [PATCH 6/7] fix(explorer): review follow-ups for #830 render-loop fix - Wire the Explorer frontend's node --test suites (test:graph-store, test:graph-workspace, and the new test:plugin-registry regression test) into CI. Previously only `npm run build` ran, so none of the frontend tests -- including this fix's own regression coverage -- executed anywhere except a contributor's local machine. - Broaden the diagnostics dedup's structureLayer comparison to also cover disabledReason/curveCount/bridgeCurveCount/backboneCurveCount, not just cacheKey/lastDrawAt/enabled, so a disabledReason-only transition doesn't leave the dev diagnostics panel stale. --- .github/workflows/ci.yml | 13 ++++++++++--- .../workspaces/GraphWorkspace/GraphWorkspace.tsx | 6 +++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 18a246fb..6122e91b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,11 +30,18 @@ jobs: node-version: '20' cache: 'npm' cache-dependency-path: explorer/package-lock.json - - name: Build Explorer frontend + - name: Install Explorer frontend dependencies + working-directory: explorer + run: npm ci + - name: Test Explorer frontend working-directory: explorer run: | - npm ci - npm run build + npm run test:graph-store + npm run test:graph-workspace + npm run test:plugin-registry + - name: Build Explorer frontend + working-directory: explorer + run: npm run build - run: pip install build - run: python -m build - name: Verify Explorer frontend is packaged diff --git a/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx b/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx index 4aa36e7a..df2cd6ec 100644 --- a/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx +++ b/explorer/src/workspaces/GraphWorkspace/GraphWorkspace.tsx @@ -2323,7 +2323,11 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap const structureLayerChanged = prev.structureLayer?.cacheKey !== diagnostics.structureLayer?.cacheKey || prev.structureLayer?.lastDrawAt !== diagnostics.structureLayer?.lastDrawAt || - prev.structureLayer?.enabled !== diagnostics.structureLayer?.enabled; + prev.structureLayer?.enabled !== diagnostics.structureLayer?.enabled || + prev.structureLayer?.disabledReason !== diagnostics.structureLayer?.disabledReason || + prev.structureLayer?.curveCount !== diagnostics.structureLayer?.curveCount || + prev.structureLayer?.bridgeCurveCount !== diagnostics.structureLayer?.bridgeCurveCount || + prev.structureLayer?.backboneCurveCount !== diagnostics.structureLayer?.backboneCurveCount; // distanceVisual is compared by reference: GraphCanvas passes the same // object when distances haven't changed. From 7bddee0111b6af9cf621a03a9e0728bed44b2c2c Mon Sep 17 00:00:00 2001 From: KaifAhmad1 Date: Thu, 6 Aug 2026 13:10:13 +0530 Subject: [PATCH 7/7] ci: update stale github/codeql-action v4 pin Upstream moved the v4 tag to 5595ccaf912efad79be6eef63a5619ff05969be3 (v4.37.6), which the repo's own verify-action-pins.sh now (correctly) flags as a mismatch against the previously-pinned commit. Pre-existing drift unrelated to #830/#836, but it was failing this PR's required "verify" check, so fixing it here. --- .github/workflows/codeql.yml | 12 ++++++------ .github/workflows/defender-for-devops.yml | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index c32d6955..9aedaf40 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -32,7 +32,7 @@ jobs: # meaningful state carried over from a failed attempt. - name: Initialize CodeQL (attempt 1) id: codeql-init-1 - uses: github/codeql-action/init@d1ba80a13dd99fba24a470575428917156a28b43 # v4 + uses: github/codeql-action/init@5595ccaf912efad79be6eef63a5619ff05969be3 # v4 continue-on-error: true with: languages: python @@ -42,7 +42,7 @@ jobs: - name: Initialize CodeQL (attempt 2) id: codeql-init-2 if: steps.codeql-init-1.outcome == 'failure' - uses: github/codeql-action/init@d1ba80a13dd99fba24a470575428917156a28b43 # v4 + uses: github/codeql-action/init@5595ccaf912efad79be6eef63a5619ff05969be3 # v4 continue-on-error: true with: languages: python @@ -52,17 +52,17 @@ jobs: - name: Initialize CodeQL (attempt 3) id: codeql-init-3 if: steps.codeql-init-2.outcome == 'failure' - uses: github/codeql-action/init@d1ba80a13dd99fba24a470575428917156a28b43 # v4 + uses: github/codeql-action/init@5595ccaf912efad79be6eef63a5619ff05969be3 # v4 with: languages: python queries: security-and-quality config-file: .github/codeql/codeql-config.yml - name: Autobuild - uses: github/codeql-action/autobuild@d1ba80a13dd99fba24a470575428917156a28b43 # v4 + uses: github/codeql-action/autobuild@5595ccaf912efad79be6eef63a5619ff05969be3 # v4 - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@d1ba80a13dd99fba24a470575428917156a28b43 # v4 + uses: github/codeql-action/analyze@5595ccaf912efad79be6eef63a5619ff05969be3 # v4 with: category: "/language:python" upload: false @@ -72,7 +72,7 @@ jobs: # Uploads results only when Default Setup is not active. # If Default Setup is still enabled, this step skips gracefully # instead of failing the workflow with HTTP 409. - uses: github/codeql-action/upload-sarif@d1ba80a13dd99fba24a470575428917156a28b43 # v4 + uses: github/codeql-action/upload-sarif@5595ccaf912efad79be6eef63a5619ff05969be3 # v4 with: sarif_file: ${{ steps.codeql.outputs.sarif-output }} category: "/language:python" diff --git a/.github/workflows/defender-for-devops.yml b/.github/workflows/defender-for-devops.yml index aa71bd69..c561dc17 100644 --- a/.github/workflows/defender-for-devops.yml +++ b/.github/workflows/defender-for-devops.yml @@ -57,7 +57,7 @@ jobs: # avoiding the guardian.cmd/checkov exit-code bug in the MSDO wrapper. tools: eslint,templateanalyzer,terrascan - name: Upload results to Security tab - uses: github/codeql-action/upload-sarif@d1ba80a13dd99fba24a470575428917156a28b43 # v4 + uses: github/codeql-action/upload-sarif@5595ccaf912efad79be6eef63a5619ff05969be3 # v4 with: sarif_file: ${{ steps.msdo.outputs.sarifFile }} @@ -82,7 +82,7 @@ jobs: } - name: Upload Checkov results to Security tab - uses: github/codeql-action/upload-sarif@d1ba80a13dd99fba24a470575428917156a28b43 # v4 + uses: github/codeql-action/upload-sarif@5595ccaf912efad79be6eef63a5619ff05969be3 # v4 if: always() with: sarif_file: reports/checkov.sarif