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.
This commit is contained in:
Sameer6305
2026-08-05 17:52:21 +05:30
parent 80de3652cf
commit 667e69a0c1
3 changed files with 23 additions and 47 deletions
@@ -1120,11 +1120,9 @@ 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);
// 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<number | null>(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) {
@@ -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 = {
+10 -26
View File
@@ -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(