mirror of
https://github.com/semantica-agi/semantica.git
synced 2026-08-29 04:26:20 +00:00
Merge pull request #836 from Sameer6305/fix/830-temporal-panel-render-loop
fix(explorer): resolve infinite render loop preventing Temporal panel from rendering (#830)
This commit is contained in:
Mohd Kaif
@@ -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
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -60,6 +60,15 @@ 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, #836) 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
|
||||
- The `scrubberTime`-churn fix was also applied to the equivalent (but currently unused/unmounted) `GraphWorkspaceShell.tsx`, which shares the same `TimelinePanel` integration pattern but does not have the diagnostics-churn code path
|
||||
- **Follow-up review fix**: the diagnostics dedup's `structureLayer` comparison now also covers `disabledReason`, `curveCount`, `bridgeCurveCount`, and `backboneCurveCount` (previously only `cacheKey`/`lastDrawAt`/`enabled` were compared, so a pure `disabledReason` transition could leave the dev-only diagnostics panel stale)
|
||||
- **Follow-up review fix**: `test:graph-store`, `test:graph-workspace`, and the new `test:plugin-registry` regression test are now run in CI (`.github/workflows/ci.yml`) — previously none of the Explorer frontend's `node --test` suites executed anywhere in CI, only `npm run build`, so this fix's own regression coverage (and all prior frontend test coverage) provided no protection against silent regressions
|
||||
- **`HybridSearch.search()` crashed with `AttributeError` for any `VectorStore` backend other than `inmemory`** (#833, #837) by @KaifAhmad1
|
||||
- `HybridSearch.search()` read `self.vector_store.vectors` directly, an internal dict `VectorStore` only populates for `backend="inmemory"`; every other backend (faiss, weaviate, qdrant, milvus, pinecone, pgvector, sqlite) raised `AttributeError`, making `HybridSearch` unusable against any real store. It now delegates to `VectorStore.search_vectors()` (the backend-agnostic public API) for non-inmemory backends, applies `metadata_filter` as a post-filter over the returned candidates, and normalizes results to a consistent `{id, score, distance, metadata}` shape
|
||||
- **Fixed along the way**: `vector_ids` could stay `None` when callers passed explicit `vectors`/`metadata` without `vector_ids`, crashing downstream list indexing — now defaulted to generated positional IDs
|
||||
|
||||
@@ -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 --import tsx --test tests/pluginRegistry.temporal.test.mjs"
|
||||
},
|
||||
"dependencies": {
|
||||
"@monaco-editor/react": "^4.7.0",
|
||||
|
||||
@@ -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<GraphPlugin>;
|
||||
shouldLoad: (context: {
|
||||
panelState: Record<string, boolean>;
|
||||
temporalState: GraphTemporalState | null;
|
||||
temporalState?: GraphTemporalState | null;
|
||||
}) => boolean;
|
||||
};
|
||||
|
||||
@@ -1119,6 +1120,18 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap
|
||||
const [activeNodeCount, setActiveNodeCount] = useState<number | null>(null);
|
||||
const [temporalBounds, setTemporalBounds] = useState<TemporalBounds | null>(null);
|
||||
const [scrubberTime, setScrubberTime] = useState<Date | null>(null);
|
||||
// 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<number | null>(null);
|
||||
const onTimeChange = useCallback((time: Date) => {
|
||||
const ms = time.getTime();
|
||||
if (ms === lastScrubberMsRef.current) {
|
||||
return;
|
||||
}
|
||||
lastScrubberMsRef.current = ms;
|
||||
setScrubberTime(time);
|
||||
}, []);
|
||||
const [loadingProgress, setLoadingProgress] = useState<GraphLoadProgress | null>(null);
|
||||
const [pluginPanelState, setPluginPanelState] = useState<Record<string, boolean>>({
|
||||
"effects-panel": false,
|
||||
@@ -1129,6 +1142,9 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap
|
||||
const [pluginRuntimeVersion, setPluginRuntimeVersion] = useState(0);
|
||||
const [effectsState, setEffectsState] = useState<GraphEffectsState>(DEFAULT_EFFECTS_STATE);
|
||||
const [graphDiagnosticsState, setGraphDiagnosticsState] = useState<GraphRuntimeDiagnosticsSnapshot | null>(null);
|
||||
// Tracks the last accepted diagnostics outside React's state cycle, allowing
|
||||
// handleDiagnosticsChange to compare synchronously before calling setState.
|
||||
const lastDiagnosticsRef = useRef<GraphRuntimeDiagnosticsSnapshot | null>(null);
|
||||
const [graphAnalyticsState, setGraphAnalyticsState] = useState<GraphAnalyticsSnapshot | null>(null);
|
||||
const [loadedPlugins, setLoadedPlugins] = useState<Record<string, GraphPlugin>>({});
|
||||
|
||||
@@ -2056,7 +2072,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",
|
||||
@@ -2065,7 +2081,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",
|
||||
@@ -2074,7 +2090,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: temporalOverlayShouldLoad,
|
||||
},
|
||||
],
|
||||
[],
|
||||
@@ -2092,7 +2108,7 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap
|
||||
return;
|
||||
}
|
||||
|
||||
if (!entry.shouldLoad({ panelState: pluginPanelState, temporalState })) {
|
||||
if (!entry.shouldLoad({ panelState: pluginPanelState })) {
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -2111,7 +2127,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) => {
|
||||
@@ -2274,6 +2290,55 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap
|
||||
if (!GRAPH_THEME.effects.diagnostics.enabledInDev) {
|
||||
return;
|
||||
}
|
||||
|
||||
// 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 = [
|
||||
"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 ||
|
||||
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.
|
||||
const distanceVisualChanged = prev.distanceVisual !== diagnostics.distanceVisual;
|
||||
|
||||
if (!availabilityChanged && !edgeClassesChanged && !structureLayerChanged && !distanceVisualChanged) {
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
lastDiagnosticsRef.current = diagnostics;
|
||||
setGraphDiagnosticsState(diagnostics);
|
||||
}, []);
|
||||
|
||||
@@ -2922,7 +2987,7 @@ export function GraphWorkspace({ externalFocusNodeId, externalFocusToken }: Grap
|
||||
<div className="explore-scene-footer">
|
||||
<Suspense fallback={<div style={timelineFallbackStyle}>Loading timeline…</div>}>
|
||||
<LazyTimelinePanel
|
||||
onTimeChange={setScrubberTime}
|
||||
onTimeChange={onTimeChange}
|
||||
minDate={temporalBounds?.min ?? undefined}
|
||||
maxDate={temporalBounds?.max ?? undefined}
|
||||
/>
|
||||
|
||||
@@ -325,6 +325,17 @@ export function GraphWorkspaceShell() {
|
||||
const [activeNodeCount, setActiveNodeCount] = useState<number | null>(null);
|
||||
const [temporalBounds, setTemporalBounds] = useState<TemporalBounds | null>(null);
|
||||
const [scrubberTime, setScrubberTime] = useState<Date | null>(null);
|
||||
// Deduplicates setScrubberTime calls by millisecond value — same fix as
|
||||
// GraphWorkspace.tsx (issue #830).
|
||||
const lastScrubberMsRef = useRef<number | null>(null);
|
||||
const onTimeChange = useCallback((time: Date) => {
|
||||
const ms = time.getTime();
|
||||
if (ms === lastScrubberMsRef.current) {
|
||||
return;
|
||||
}
|
||||
lastScrubberMsRef.current = ms;
|
||||
setScrubberTime(time);
|
||||
}, []);
|
||||
const [loadingProgress, setLoadingProgress] = useState<GraphLoadProgress | null>(null);
|
||||
const [isGraphStageReady, setIsGraphStageReady] = useState(false);
|
||||
const [layoutStatus, setLayoutStatus] = useState<GraphLayoutStatus>({
|
||||
@@ -632,7 +643,7 @@ export function GraphWorkspaceShell() {
|
||||
|
||||
<Suspense fallback={<TimelineFallback min={temporalBounds?.min ?? null} max={temporalBounds?.max ?? null} />}>
|
||||
<TimelinePanel
|
||||
onTimeChange={setScrubberTime}
|
||||
onTimeChange={onTimeChange}
|
||||
minDate={temporalBounds?.min ?? undefined}
|
||||
maxDate={temporalBounds?.max ?? undefined}
|
||||
/>
|
||||
|
||||
@@ -0,0 +1,24 @@
|
||||
/**
|
||||
* 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. 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 = {
|
||||
panelState: Record<string, boolean>;
|
||||
};
|
||||
|
||||
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"]);
|
||||
}
|
||||
@@ -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<TemporalDiffResult> {
|
||||
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();
|
||||
}
|
||||
@@ -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<NodeAttributes, EdgeAttributes> 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<NodeAttributes>;
|
||||
}
|
||||
|
||||
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<string, string | undefined>) {
|
||||
previousColors.forEach((color, nodeId) => {
|
||||
writeBaseColor(context, nodeId, color);
|
||||
});
|
||||
context.scene?.requestRender();
|
||||
}
|
||||
|
||||
function applyDiffHighlight(context: GraphPluginContext, result: TemporalDiffResult): Map<string, string | undefined> {
|
||||
const previousColors = new Map<string, string | undefined>();
|
||||
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<string | null>(null);
|
||||
const [requestState, setRequestState] = useState<DiffRequestState>({ status: "idle" });
|
||||
const abortRef = useRef<AbortController | null>(null);
|
||||
// Maps currently-highlighted node ID -> its baseColor before highlighting, so clearing
|
||||
// restores the exact prior value instead of an approximation.
|
||||
const previousColorsRef = useRef<Map<string, string | undefined>>(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 (
|
||||
<div style={diffSectionStyle}>
|
||||
<div style={panelEyebrowStyle}>Compare two points in time</div>
|
||||
<div style={diffInputRowStyle}>
|
||||
<input
|
||||
value={fromTime}
|
||||
onChange={(event) => setFromTime(event.target.value)}
|
||||
placeholder="From, e.g. 2024-01-01T00:00:00"
|
||||
style={diffInputStyle}
|
||||
/>
|
||||
<input
|
||||
value={toTime}
|
||||
onChange={(event) => setToTime(event.target.value)}
|
||||
placeholder="To, e.g. 2025-06-15T00:00:00"
|
||||
style={diffInputStyle}
|
||||
/>
|
||||
</div>
|
||||
<button
|
||||
type="button"
|
||||
onClick={handleCompare}
|
||||
disabled={isLoading}
|
||||
style={{ ...diffActionButtonStyle, opacity: isLoading ? 0.7 : 1 }}
|
||||
>
|
||||
{isLoading ? <Loader2 size={13} className="animate-spin" style={{ marginRight: 6 }} /> : null}
|
||||
{isLoading ? "Comparing…" : "Compare"}
|
||||
</button>
|
||||
|
||||
{validationMessage ? <div style={diffValidationStyle}>{validationMessage}</div> : null}
|
||||
|
||||
{requestState.status === "error" ? (
|
||||
<div style={diffErrorStyle}>{requestState.message}</div>
|
||||
) : null}
|
||||
|
||||
{requestState.status === "empty" ? (
|
||||
<div style={emptyTextStyle}>No changes between these two points.</div>
|
||||
) : null}
|
||||
|
||||
{requestState.status === "success" ? (
|
||||
<div style={{ display: "flex", flexDirection: "column", gap: 8 }}>
|
||||
<div style={detailRowStyle}>
|
||||
<span style={detailLabelStyle}>Added</span>
|
||||
<span style={{ ...detailValueStyle, color: DIFF_ADDED_COLOR }}>
|
||||
{requestState.result.added_nodes.length.toLocaleString()}
|
||||
</span>
|
||||
</div>
|
||||
<div style={detailRowStyle}>
|
||||
<span style={detailLabelStyle}>Removed</span>
|
||||
<span style={{ ...detailValueStyle, color: DIFF_REMOVED_COLOR }}>
|
||||
{requestState.result.removed_nodes.length.toLocaleString()}
|
||||
</span>
|
||||
</div>
|
||||
</div>
|
||||
) : null}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
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: (
|
||||
<div style={panelBodyStyle}>
|
||||
<div style={panelEyebrowStyle}>Current scrubber state</div>
|
||||
@@ -317,7 +100,6 @@ export const temporalOverlayPlugin: GraphPlugin = {
|
||||
{typeof temporal?.activeNodeCount === "number" ? temporal.activeNodeCount.toLocaleString() : "All"}
|
||||
</span>
|
||||
</div>
|
||||
<DiffSection context={context} />
|
||||
</div>
|
||||
),
|
||||
};
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
/**
|
||||
* Regression tests for issue #830: plugin registry shouldLoad predicates.
|
||||
*
|
||||
* 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";
|
||||
|
||||
const require = createRequire(import.meta.url);
|
||||
|
||||
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(
|
||||
temporalOverlayShouldLoad({ panelState: { "temporal-panel": false } }),
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
test("temporal-overlay shouldLoad: false when panel is closed even if scrubber time is set", () => {
|
||||
// Before the fix, a non-null currentTime caused an eager load on every scrubber update.
|
||||
assert.equal(
|
||||
temporalOverlayShouldLoad({
|
||||
panelState: { "temporal-panel": false },
|
||||
temporalState: { currentTime: new Date() },
|
||||
}),
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
test("temporal-overlay shouldLoad: true only when the panel is explicitly opened", () => {
|
||||
assert.equal(
|
||||
temporalOverlayShouldLoad({ panelState: { "temporal-panel": true } }),
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
test("temporal-overlay shouldLoad: true when panel opened even without a scrubber time", () => {
|
||||
assert.equal(
|
||||
temporalOverlayShouldLoad({
|
||||
panelState: { "temporal-panel": true },
|
||||
temporalState: { currentTime: null },
|
||||
}),
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
// ── other entries — confirm they also gate only on panelState ─────────────────
|
||||
|
||||
test("exploration-effects shouldLoad: gates only on effects-panel state", () => {
|
||||
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(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 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(
|
||||
temporalOverlayShouldLoad({ panelState: { "temporal-panel": false }, temporalState: nonNullTemporalState }),
|
||||
false,
|
||||
"temporal-overlay must not load when panel is closed, regardless of scrubber time",
|
||||
);
|
||||
assert.equal(
|
||||
explorationEffectsShouldLoad({ panelState: { "effects-panel": false }, temporalState: nonNullTemporalState }),
|
||||
false,
|
||||
);
|
||||
assert.equal(
|
||||
neighborhoodPanelShouldLoad({ panelState: { "neighborhood-panel": false }, temporalState: nonNullTemporalState }),
|
||||
false,
|
||||
);
|
||||
});
|
||||
Reference in New Issue
Block a user