From 7d7b5608b5af5914da5ffbb59f0d7e7e9dad7e38 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 1 Jul 2026 05:42:29 +0000 Subject: [PATCH 1/8] SLD: wrap long feeder names, navigate to far-end VL; mypy fix Frontend (SLD overlay): - Wrap relabelled branch-feeder names wider than ~15 chars onto up to three lines, vertically centred on the original baseline, so a long far-end VL name no longer occludes the neighbouring feeder's label at the top/bottom of the SLD. - Make a feeder name clickable: applyFeederRelabels tags each with data-feeder-nav (the far-end VL id) + the sld-feeder-navigable class, and the new useSldFeederNav hook opens that VL's SLD on click, keeping the current sub-tab so the same overload stays visible from the other extremity. Registered ahead of (and guarded against) the edit-mode switch/injection click handler so a name click is never a maneuver. Backend: - simulation_helpers.half_open_branch_reactive_from_obs: guard `nm` in the early return so mypy can prove it non-None before nm.set_working_variant (fixes union-attr at lines 526/533). Tests: wrapFeederLabel + nav-tagging cases in feederLabels.test.ts; wrapped-label + navigation cases in SldOverlay.test.tsx. Full frontend suite (1705) + backend simulation-helper suites green; ruff + mypy + code-quality gate pass. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_019mHWAwFyLaRJJUvcctZqDm --- docs/features/sld-diagram-feeder-labels.md | 29 +++++++- expert_backend/services/simulation_helpers.py | 2 +- frontend/CLAUDE.md | 9 ++- frontend/src/App.css | 12 +++ frontend/src/App.tsx | 9 +++ frontend/src/components/SldOverlay.test.tsx | 37 +++++++++- frontend/src/components/SldOverlay.tsx | 19 +++++ .../src/components/VisualizationPanel.tsx | 4 + frontend/src/hooks/useSldFeederNav.ts | 45 ++++++++++++ frontend/src/utils/svg/feederLabels.test.ts | 50 ++++++++++++- frontend/src/utils/svg/feederLabels.ts | 73 ++++++++++++++++++- 11 files changed, 276 insertions(+), 13 deletions(-) create mode 100644 frontend/src/hooks/useSldFeederNav.ts diff --git a/docs/features/sld-diagram-feeder-labels.md b/docs/features/sld-diagram-feeder-labels.md index f7c74a2..2f3f5c6 100644 --- a/docs/features/sld-diagram-feeder-labels.md +++ b/docs/features/sld-diagram-feeder-labels.md @@ -29,6 +29,27 @@ operator reads *where the line goes*, e.g. `MARSILLON 225kV`). (original stashed in `data-feeder-orig`, restored on tab/VL switch, highlight clones skipped) — the same render-every-time + self-gate pattern as the other SLD label passes. +- **Long labels wrap instead of occluding neighbours.** A relabelled name + wider than ~15 chars (`wrapFeederLabel`) is split on whitespace onto up to + three `` lines, vertically centred on the original baseline (first + line lifted by half the block height) so it spreads up AND down rather than + overprinting the adjacent feeder's label — the fix for the top/bottom label + pile-up on dense VLs like `VL_way_207479669-225`. + +## 1b. Navigate to a branch's other extremity by clicking its feeder name + +Each relabelled feeder is tagged with `data-feeder-nav` = the far-end VL id +(the `other_vl` field) and the `sld-feeder-navigable` class (dotted underline ++ pointer cursor). `hooks/useSldFeederNav.ts` installs one delegated +capture-phase click listener on the overlay body: a (non-pan) click on a +feeder name hands `other_vl` to `onNavigateToVl`, which re-opens the SLD for +that VL keeping the current sub-tab (`App.handleSldNavigateToVl` → +`handleVlDoubleClick(actionId, other_vl, tab)`). Because the contingency / +overload halo pass runs against whatever VL is displayed, the overload opened +at one end stays highlighted from the other end after the jump. The listener +uses `stopImmediatePropagation` and is registered before the edit-mode +switch/injection handler (which also early-bails on `[data-feeder-nav]`) so a +name click never doubles as a topology maneuver. ## 2. Overload halo visible on the extremity SLD @@ -85,6 +106,8 @@ the card annotates it with the live-end reactive power. - Backend: `test_feeder_labels.py`, `test_half_open_overload.py`, and the `build_half_open_reactive` cases in `test_simulation_helpers.py`. -- Frontend: `utils/svg/feederLabels.test.ts`, the "feeder relabelling" / - "overload halo via friendly name" suites in `components/SldOverlay.test.tsx`, - and the "half-open overload annotation" suite in `components/ActionCard.test.tsx`. +- Frontend: `utils/svg/feederLabels.test.ts` (relabel + `wrapFeederLabel` + + `data-feeder-nav` tagging), the "feeder relabelling" / "overload halo via + friendly name" / feeder-name navigation suites in + `components/SldOverlay.test.tsx`, and the "half-open overload annotation" + suite in `components/ActionCard.test.tsx`. diff --git a/expert_backend/services/simulation_helpers.py b/expert_backend/services/simulation_helpers.py index a8388b2..94d11d5 100644 --- a/expert_backend/services/simulation_helpers.py +++ b/expert_backend/services/simulation_helpers.py @@ -515,7 +515,7 @@ def half_open_branch_reactive_from_obs(obs: Any) -> dict[str, float]: nm = getattr(obs, "_network_manager", None) variant_id = getattr(obs, "_variant_id", None) network = getattr(nm, "network", None) if nm is not None else None - if network is None or variant_id is None: + if nm is None or network is None or variant_id is None: return {} try: original = network.get_working_variant_id() diff --git a/frontend/CLAUDE.md b/frontend/CLAUDE.md index 211b969..1432d3e 100644 --- a/frontend/CLAUDE.md +++ b/frontend/CLAUDE.md @@ -73,9 +73,14 @@ frontend/ │ ├── useTheme.ts # Light/dark theme toggle + persistence │ │ # (0.8.0; see docs/features/dark-mode.md) │ ├── useSldFeederRelabel.ts # Relabel SLD branch feeders with the far- - │ │ # end VL name (Issue 1; render-every-time - │ │ # self-gate, delegates the DOM swap to + │ │ # end VL name + wrap long labels (Issue 1; + │ │ # render-every-time self-gate, delegates the + │ │ # DOM swap to │ │ # utils/svg/feederLabels.applyFeederRelabels) + │ ├── useSldFeederNav.ts # Click a relabelled feeder name → open the + │ │ # far-end VL's SLD (reads data-feeder-nav; + │ │ # capture-phase, keeps the current sub-tab so + │ │ # the overload stays visible from both ends) │ └── useSldInjectionNameButtons.ts # Render editable-injection NAME │ # buttons on the SLD (extracted from │ # SldOverlay to keep it under the LoC ceiling) diff --git a/frontend/src/App.css b/frontend/src/App.css index 803f2d7..4fc7faf 100644 --- a/frontend/src/App.css +++ b/frontend/src/App.css @@ -426,6 +426,18 @@ circle.nad-highlight { cursor: pointer; } +/* Relabelled branch feeder names are clickable: a click navigates the SLD + overlay to that voltage level (the branch's other extremity). The dashed + underline + pointer cursor signal the affordance without recolouring. */ +.sld-feeder-navigable { + cursor: pointer; + text-decoration: underline; + text-decoration-style: dotted; +} +.sld-feeder-navigable:hover { + fill: var(--color-brand-strong) !important; +} + /* Injection name rendered as a button (same dark blue + white text as the former "Manual action" button): a rounded rect injected behind the name , with the label recoloured on top. */ diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 117ef8b..b199570 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -1609,6 +1609,14 @@ function App() { handleVlDoubleClick(selectedActionId || '', vlName); }, [handleVlDoubleClick, selectedActionId]); + // Clicking a (relabelled) feeder name on the SLD jumps to the far-end VL's + // SLD, keeping the current sub-tab so the same contingency / overload stays + // in view from the other extremity. + const handleSldNavigateToVl = useCallback((vlId: string) => { + if (!vlOverlay) return; + handleVlDoubleClick(vlOverlay.actionId || selectedActionId || '', vlId, vlOverlay.tab); + }, [vlOverlay, handleVlDoubleClick, selectedActionId]); + // Keep the VL-disk interaction callbacks in refs so the delegated // listeners below re-bind ONLY when a diagram / its metadata actually // changes — never on an unrelated App render, which would needlessly @@ -2024,6 +2032,7 @@ function App() { onSldSwitchFocus={sldTopologyEdit.setFocusedSwitch} onSldSwitchRemove={sldTopologyEdit.removeSwitch} onSldSwitchRemoveMany={sldTopologyEdit.removeSwitches} + onSldNavigateToVl={handleSldNavigateToVl} voltageLevels={voltageLevels} onVlOpen={handleVlOpen} onOverflowPinPreview={handlePinPreview} diff --git a/frontend/src/components/SldOverlay.test.tsx b/frontend/src/components/SldOverlay.test.tsx index bb985bb..2c332bd 100644 --- a/frontend/src/components/SldOverlay.test.tsx +++ b/frontend/src/components/SldOverlay.test.tsx @@ -1352,14 +1352,47 @@ describe('SldOverlay', () => { expect(lbl?.getAttribute('data-feeder-orig')).toBe('relation_8423569-225'); }); - it('preserves a parallel-circuit index in the label', () => { + it('preserves a parallel-circuit index in the label (wrapped over lines)', () => { const overlay = buildRelabelOverlay({ 'relation_8423569-225': { name: 'LANNEL61PRAGN', other_vl: 'VL_LANNE', label: 'LANNEMEZAN 225kV 2', }, }); const { container } = render(); - expect(container.querySelector('#lbl_marsil')?.textContent).toBe('LANNEMEZAN 225kV 2'); + const lbl = container.querySelector('#lbl_marsil')!; + // Long labels wrap onto lines so they don't occlude a + // neighbouring feeder; the full label (index included) is preserved. + const tspans = Array.from(lbl.querySelectorAll('tspan')); + expect(tspans.length).toBeGreaterThan(1); + expect(tspans.map(t => t.textContent).join(' ')).toBe('LANNEMEZAN 225kV 2'); + }); + + it('tags a relabelled feeder for navigation to the far-end VL', () => { + const overlay = buildRelabelOverlay({ + 'relation_8423569-225': { + name: 'MARSIL61PRAGN', other_vl: 'VL_MARSIL', label: 'MARSILLON 225kV', + }, + }); + const { container } = render(); + const lbl = container.querySelector('#lbl_marsil'); + expect(lbl?.getAttribute('data-feeder-nav')).toBe('VL_MARSIL'); + expect(lbl?.classList.contains('sld-feeder-navigable')).toBe(true); + }); + + it('navigates to the far-end VL when a feeder name is clicked', () => { + const overlay = buildRelabelOverlay({ + 'relation_8423569-225': { + name: 'MARSIL61PRAGN', other_vl: 'VL_MARSIL', label: 'MARSILLON 225kV', + }, + }); + const onNavigateToVl = vi.fn(); + const { container } = render( + , + ); + container.querySelector('#lbl_marsil')!.dispatchEvent( + new MouseEvent('click', { bubbles: true }), + ); + expect(onNavigateToVl).toHaveBeenCalledWith('VL_MARSIL'); }); it('leaves the id untouched when the label is null', () => { diff --git a/frontend/src/components/SldOverlay.tsx b/frontend/src/components/SldOverlay.tsx index cc33208..2e49916 100644 --- a/frontend/src/components/SldOverlay.tsx +++ b/frontend/src/components/SldOverlay.tsx @@ -10,6 +10,7 @@ import type { DiagramData, AnalysisResult, ActionDetail, VlOverlay, SldTab, SldF import { isCouplingAction } from '../utils/svgUtils'; import { buildFriendlyToEquip, overloadCandidates } from '../utils/svg/feederLabels'; import { useSldFeederRelabel } from '../hooks/useSldFeederRelabel'; +import { useSldFeederNav } from '../hooks/useSldFeederNav'; import { useSldInjectionNameButtons } from '../hooks/useSldInjectionNameButtons'; import { colors } from '../styles/tokens'; import SldEditPanel from './SldEditPanel'; @@ -93,6 +94,14 @@ export interface SldOverlayProps { onSwitchFocus?: (equipmentId: string | null) => void; onSwitchRemove?: (equipmentId: string) => void; onSwitchRemoveMany?: (equipmentIds: string[]) => void; + /** + * Navigate to the voltage level at a branch extremity. Fired when the + * operator clicks a (relabelled) feeder name — the far-end VL id is + * resolved from ``data-feeder-nav`` (set by ``applyFeederRelabels``). The + * host re-opens the SLD for that VL keeping the current tab, so the same + * overload stays visible from the other end. + */ + onNavigateToVl?: (vlId: string) => void; } const SldOverlay: React.FC = ({ @@ -121,6 +130,7 @@ const SldOverlay: React.FC = ({ onSwitchFocus, onSwitchRemove, onSwitchRemoveMany, + onNavigateToVl, }) => { const overlayBodyRef = useRef(null); // True once a press-drag has moved the pointer beyond a small slop @@ -1032,6 +1042,11 @@ const SldOverlay: React.FC = ({ // self-gate live in the hook (see utils/svg/feederLabels.ts for the swap). useSldFeederRelabel(overlayBodyRef, vlOverlay.feeder_labels, activeSvg, vlOverlay.tab, vlOverlay.actionId, previewActive); + // Click a relabelled feeder name → open the far-end VL's SLD. Registered + // BEFORE the edit-mode click effect below so its stop-immediate wins on a + // shared capture-phase click. + useSldFeederNav(overlayBodyRef, onNavigateToVl, panMovedRef); + // Delegate switch clicks on the SVG body when edit mode is on. // We attach to the body container (capture phase) so taps on the // breaker's nested children still fire. The ``equipmentId`` is @@ -1114,6 +1129,10 @@ const SldOverlay: React.FC = ({ // native listener, so we filter here. if (target.closest('[data-testid="sld-injection-popover"]')) return; + // A feeder-name click navigates to the far-end VL (handled by + // useSldFeederNav) — never treat it as a topology maneuver. + if (target.closest('[data-feeder-nav]')) return; + // Injection NAME button (rect / label tagged with // data-injection-equip) — opens the active-power editor. const nameHit = target.closest('[data-injection-equip]'); diff --git a/frontend/src/components/VisualizationPanel.tsx b/frontend/src/components/VisualizationPanel.tsx index ba83fc7..fe395d5 100644 --- a/frontend/src/components/VisualizationPanel.tsx +++ b/frontend/src/components/VisualizationPanel.tsx @@ -121,6 +121,8 @@ interface VisualizationPanelProps { onSldSwitchFocus?: (equipmentId: string | null) => void; onSldSwitchRemove?: (equipmentId: string) => void; onSldSwitchRemoveMany?: (equipmentIds: string[]) => void; + /** Navigate the SLD overlay to a branch extremity's voltage level. */ + onSldNavigateToVl?: (vlId: string) => void; voltageLevels: string[]; onVlOpen: (vlName: string) => void; /** @@ -283,6 +285,7 @@ const VisualizationPanel: React.FC = ({ onSldSwitchFocus, onSldSwitchRemove, onSldSwitchRemoveMany, + onSldNavigateToVl, voltageLevels, onVlOpen, onOverflowPinPreview, @@ -1404,6 +1407,7 @@ const VisualizationPanel: React.FC = ({ onSwitchFocus={onSldSwitchFocus} onSwitchRemove={onSldSwitchRemove} onSwitchRemoveMany={onSldSwitchRemoveMany} + onNavigateToVl={onSldNavigateToVl} /> )} diff --git a/frontend/src/hooks/useSldFeederNav.ts b/frontend/src/hooks/useSldFeederNav.ts new file mode 100644 index 0000000..4227e97 --- /dev/null +++ b/frontend/src/hooks/useSldFeederNav.ts @@ -0,0 +1,45 @@ +// Copyright (c) 2025-2026, RTE (https://www.rte-france.com) +// This Source Code Form is subject to the terms of the Mozilla Public License, version 2.0. +// If a copy of the Mozilla Public License, version 2.0 was not distributed with this file, +// you can obtain one at http://mozilla.org/MPL/2.0/. +// SPDX-License-Identifier: MPL-2.0 +// This file is part of Co-Study4Grid a Power Grid Study tool Assistant Interface to help solve contigencies for a grid state under study. + +import { useEffect, type RefObject } from 'react'; + +/** + * Navigate to a branch extremity's voltage level by clicking its (relabelled) + * feeder name on the SLD. ``applyFeederRelabels`` tags each navigable feeder + * label with ``data-feeder-nav`` = the far-end VL id; a delegated capture-phase + * listener on the overlay body resolves that id and hands it to + * ``onNavigateToVl`` (which re-opens the SLD for that VL, keeping the current + * tab so the same overload stays in view from the other end). + * + * Capture phase + a stop-immediate so the edit-mode switch/injection click + * handler on the same container never also fires for a label click. A press + * that moved (a pan, tracked in ``panMovedRef``) is ignored, matching the + * switch-click gesture discrimination. + */ +export function useSldFeederNav( + bodyRef: RefObject, + onNavigateToVl: ((vlId: string) => void) | undefined, + panMovedRef: RefObject, +): void { + useEffect(() => { + const container = bodyRef.current; + if (!container || !onNavigateToVl) return; + const onClick = (ev: MouseEvent) => { + if (panMovedRef.current) return; + const target = ev.target as Element | null; + const hit = target?.closest('[data-feeder-nav]'); + if (!hit) return; + const vlId = hit.getAttribute('data-feeder-nav'); + if (!vlId) return; + ev.stopImmediatePropagation(); + ev.preventDefault(); + onNavigateToVl(vlId); + }; + container.addEventListener('click', onClick, true); + return () => container.removeEventListener('click', onClick, true); + }, [bodyRef, onNavigateToVl, panMovedRef]); +} diff --git a/frontend/src/utils/svg/feederLabels.test.ts b/frontend/src/utils/svg/feederLabels.test.ts index 2e759aa..ba98834 100644 --- a/frontend/src/utils/svg/feederLabels.test.ts +++ b/frontend/src/utils/svg/feederLabels.test.ts @@ -5,7 +5,7 @@ // SPDX-License-Identifier: MPL-2.0 import { describe, it, expect } from 'vitest'; -import { buildFriendlyToEquip, overloadCandidates, applyFeederRelabels } from './feederLabels'; +import { buildFriendlyToEquip, overloadCandidates, applyFeederRelabels, wrapFeederLabel } from './feederLabels'; import type { FeederLabel } from '../../types'; const fl = (over: Partial> = {}): Record => ({ @@ -46,6 +46,29 @@ describe('overloadCandidates', () => { }); }); +describe('wrapFeederLabel', () => { + it('keeps a short label on a single line', () => { + expect(wrapFeederLabel('MARSILLON 225kV')).toEqual(['MARSILLON 225kV']); + }); + + it('never wraps a single word (no spaces)', () => { + expect(wrapFeederLabel('virtual_relation_8423568_a_0-225')) + .toEqual(['virtual_relation_8423568_a_0-225']); + }); + + it('wraps a long label on spaces and preserves every token', () => { + const lines = wrapFeederLabel('LANNEMEZAN 225kV 2'); + expect(lines.length).toBeGreaterThan(1); + expect(lines.join(' ')).toBe('LANNEMEZAN 225kV 2'); + }); + + it('caps at three lines, folding the overflow into the last one', () => { + const lines = wrapFeederLabel('ONE TWO THREE FOUR FIVE SIX SEVEN EIGHT'); + expect(lines.length).toBeLessThanOrEqual(3); + expect(lines.join(' ')).toBe('ONE TWO THREE FOUR FIVE SIX SEVEN EIGHT'); + }); +}); + describe('applyFeederRelabels', () => { const mount = (svg: string): HTMLElement => { const div = document.createElement('div'); @@ -87,4 +110,29 @@ describe('applyFeederRelabels', () => { applyFeederRelabels(container, fl(), null); expect(container.querySelector('#t')!.textContent).toBe('relation_8423569-225'); }); + + it('tags the relabelled feeder with the far-end VL id for navigation', () => { + const container = mount('relation_8423569-225'); + applyFeederRelabels(container, fl(), ''); + const t = container.querySelector('#t')!; + expect(t.getAttribute('data-feeder-nav')).toBe('VL_MARSIL'); + expect(t.classList.contains('sld-feeder-navigable')).toBe(true); + }); + + it('drops the navigation tag on restore', () => { + const container = mount('relation_8423569-225'); + applyFeederRelabels(container, fl(), ''); + applyFeederRelabels(container, {}, ''); + const t = container.querySelector('#t')!; + expect(t.getAttribute('data-feeder-nav')).toBeNull(); + expect(t.classList.contains('sld-feeder-navigable')).toBe(false); + }); + + it('does not tag navigation when the far-end VL is unknown', () => { + const container = mount('relation_8423569-225'); + applyFeederRelabels(container, { + 'relation_8423569-225': { name: 'X', other_vl: null, label: 'X' }, + }, ''); + expect(container.querySelector('#t')!.getAttribute('data-feeder-nav')).toBeNull(); + }); }); diff --git a/frontend/src/utils/svg/feederLabels.ts b/frontend/src/utils/svg/feederLabels.ts index a95efbe..6e3e099 100644 --- a/frontend/src/utils/svg/feederLabels.ts +++ b/frontend/src/utils/svg/feederLabels.ts @@ -41,13 +41,72 @@ export function overloadCandidates( return mapped && mapped.length > 0 ? [lineId, ...mapped] : [lineId]; } +const SVG_NS = 'http://www.w3.org/2000/svg'; +/** Wrap a relabelled feeder onto multiple lines past this width so a long + * far-end VL name (e.g. "LANNEMEZAN 225kV 1") stops overprinting the adjacent + * feeder's label at the top / bottom of the SLD. */ +const FEEDER_WRAP_CHARS = 15; +const FEEDER_MAX_LINES = 3; + +/** + * Break a feeder label into at most ``FEEDER_MAX_LINES`` lines no wider than + * ``FEEDER_WRAP_CHARS``, splitting on whitespace. A single word longer than the + * budget is left intact on its own line (hard-splitting an id / name reads + * worse than one slightly-long line). Overflow past the line cap is folded back + * into the last line. + */ +export function wrapFeederLabel(label: string): string[] { + if (label.length <= FEEDER_WRAP_CHARS || !label.includes(' ')) return [label]; + const words = label.split(/\s+/).filter(Boolean); + const lines: string[] = []; + let cur = ''; + for (const w of words) { + if (!cur) cur = w; + else if ((cur + ' ' + w).length <= FEEDER_WRAP_CHARS) cur += ' ' + w; + else { lines.push(cur); cur = w; } + } + if (cur) lines.push(cur); + if (lines.length > FEEDER_MAX_LINES) { + const head = lines.slice(0, FEEDER_MAX_LINES - 1); + head.push(lines.slice(FEEDER_MAX_LINES - 1).join(' ')); + return head; + } + return lines; +} + +/** + * Write ``label`` into ``textEl``, wrapping onto multiple ```` lines when + * it is long. The block is vertically centred on the original baseline (first + * line lifted by half the block height) so the wrapped label sits where the + * single-line id used to, spreading up AND down rather than only downward into + * the diagram. + */ +function setFeederText(textEl: SVGTextElement, label: string): void { + const lines = wrapFeederLabel(label); + if (lines.length <= 1) { + textEl.textContent = label; + return; + } + const x = textEl.getAttribute('x') ?? '0'; + textEl.textContent = ''; + lines.forEach((line, i) => { + const tspan = document.createElementNS(SVG_NS, 'tspan'); + tspan.setAttribute('x', x); + tspan.setAttribute('dy', i === 0 ? `${(-0.6 * (lines.length - 1)).toFixed(2)}em` : '1.2em'); + tspan.textContent = line; + textEl.appendChild(tspan); + }); +} + /** * Relabel branch feeders with the NAME of the voltage level at the OTHER end of * the branch (e.g. "MARSILLON 225kV") instead of pypowsybl's default raw IIDM * branch id (e.g. "relation_8423569-225"), which is hard to interpret on the * PyPSA grids (Issue 1). The backend (``build_feeder_labels``) resolves the - * far-end VL name + parallel-circuit index; here we just swap the matching - * ```` content. + * far-end VL name + parallel-circuit index; here we swap the matching + * ```` content, wrapping long labels so they don't occlude their + * neighbours, and tag each with ``data-feeder-nav`` (the far-end VL id) so a + * click can navigate to that VL's SLD. * * Idempotent: the original text is stashed in ``data-feeder-orig`` so a tab / * VL / preview switch restores cleanly, and highlight clones (which carry a @@ -61,9 +120,11 @@ export function applyFeederRelabels( // Restore any previous relabel first, then re-apply against the current SVG. container.querySelectorAll('[data-feeder-relabel]').forEach(el => { const orig = el.getAttribute('data-feeder-orig'); - if (orig !== null) el.textContent = orig; + if (orig !== null) el.textContent = orig; // replacing textContent drops any wrap s el.removeAttribute('data-feeder-relabel'); el.removeAttribute('data-feeder-orig'); + el.removeAttribute('data-feeder-nav'); + el.classList.remove('sld-feeder-navigable'); }); const entries = feederLabels ? Object.entries(feederLabels).filter(([, info]) => !!info && !!info.label) @@ -82,6 +143,10 @@ export function applyFeederRelabels( if (!textEl) continue; textEl.setAttribute('data-feeder-orig', textEl.textContent ?? ''); textEl.setAttribute('data-feeder-relabel', '1'); - textEl.textContent = label; + setFeederText(textEl, label); + if (info.other_vl) { + textEl.setAttribute('data-feeder-nav', info.other_vl); + textEl.classList.add('sld-feeder-navigable'); + } } } From c862d9d0cea65d869eb1b917b1f67724fafcd84c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 1 Jul 2026 08:07:00 +0000 Subject: [PATCH 2/8] NAD/SLD: full-area VL disk + name-box interactivity, wrap all feeder names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three UX fixes for the network diagram and SLD: - Feeder names at a substation's extremities no longer overlap. Beyond the branch relabelling (Issue 1), a new applyFeederLabelWrap pass wraps EVERY long feeder name (generators, loads, and branches whose far-end VL is unnamed) inside each feeder cell, and wrapFeederLabel now breaks long single-token ids on their _/-/. separators. - VL disks are interactive across their whole area, even where a branch is drawn on top: when the direct hit-test lands on an occluding edge, the handlers fall back to document.elementsFromPoint and pick the first VL disk/name box in the paint stack (discrete events only, no per-frame cost). - The VL name box on the NAD is interactive too — a single click opens its SLD. Resolved through metadata.textNodes (new textNodesBySvgId map on the metadata index) that maps each label box to its VL. Docs + unit tests updated (feederLabels, metadataIndex, vlInteractions). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014aLzZJp3MP3qYBr1WKGRC6 --- docs/features/sld-diagram-feeder-labels.md | 24 ++++- docs/features/vl-disk-interactions.md | 37 ++++++- frontend/CLAUDE.md | 10 +- frontend/src/App.css | 6 +- frontend/src/components/SldOverlay.tsx | 11 +- frontend/src/hooks/useSldFeederRelabel.ts | 30 ++++-- frontend/src/types.ts | 4 + frontend/src/utils/svg/feederLabels.test.ts | 102 +++++++++++++++++- frontend/src/utils/svg/feederLabels.ts | 98 +++++++++++++++-- frontend/src/utils/svg/metadataIndex.test.ts | 26 +++++ frontend/src/utils/svg/metadataIndex.ts | 26 ++++- frontend/src/utils/svg/vlInteractions.test.ts | 46 ++++++++ frontend/src/utils/svg/vlInteractions.ts | 91 ++++++++++++---- 13 files changed, 446 insertions(+), 65 deletions(-) diff --git a/docs/features/sld-diagram-feeder-labels.md b/docs/features/sld-diagram-feeder-labels.md index 2f3f5c6..5693fe1 100644 --- a/docs/features/sld-diagram-feeder-labels.md +++ b/docs/features/sld-diagram-feeder-labels.md @@ -30,11 +30,25 @@ operator reads *where the line goes*, e.g. `MARSILLON 225kV`). highlight clones skipped) — the same render-every-time + self-gate pattern as the other SLD label passes. - **Long labels wrap instead of occluding neighbours.** A relabelled name - wider than ~15 chars (`wrapFeederLabel`) is split on whitespace onto up to - three `` lines, vertically centred on the original baseline (first - line lifted by half the block height) so it spreads up AND down rather than - overprinting the adjacent feeder's label — the fix for the top/bottom label - pile-up on dense VLs like `VL_way_207479669-225`. + wider than ~15 chars (`wrapFeederLabel`) is split onto up to three `` + lines, vertically centred on the original baseline (first line lifted by half + the block height) so it spreads up AND down rather than overprinting the + adjacent feeder's label — the fix for the top/bottom label pile-up on dense + VLs like `VL_way_207479669-225`. Wrapping breaks on whitespace first and, for + a long single word with no spaces (a raw IIDM id like + `L_virtual_relation_8423568_a_0-225`), on its `_` / `-` / `.` separators. +- **Every long feeder NAME wraps, not just the relabelled branches.** + `applyFeederLabelWrap` (in `feederLabels.ts`, run right after + `applyFeederRelabels` from `useSldFeederRelabel`) wraps the remaining long + feeder names at a substation's extremities — generators, loads, and branches + whose far-end VL is unnamed (so they were never relabelled) — which is the + overlap that survived Issue 1's branch-only relabelling. It targets the + equipment-name `` inside each feeder cell (`.sld-extern-cell` / + `.sld-intern-cell` / `.sld-shunt-cell`), skipping the numeric P/Q flow labels, + the already-relabelled feeders (which wrap themselves), and highlight clones. + Idempotent via `data-feeder-wrap` / `data-feeder-wrap-orig`. It runs BEFORE + the injection-name-button pass so those buttons size their box to the wrapped + multi-line name. ## 1b. Navigate to a branch's other extremity by clicking its feeder name diff --git a/docs/features/vl-disk-interactions.md b/docs/features/vl-disk-interactions.md index 18ea496..6d31ace 100644 --- a/docs/features/vl-disk-interactions.md +++ b/docs/features/vl-disk-interactions.md @@ -8,12 +8,35 @@ each disk: | Gesture | Effect | |---------|--------| | **Hover** | Shows the VL name in a small floating tooltip — but only while the on-diagram VL labels are hidden (the `🏷 VL` toggle). When the labels are visible the name is already drawn, so the tooltip stays out of the way. | -| **Single-click** | Selects the VL: fills the bottom-left **Inspect** field with the VL id, auto-zooms / highlights it, and surfaces the `📄 SLD` shortcut — exactly as typing the name into the Inspect box would. | -| **Double-click** | Opens the VL's Single Line Diagram overlay (the same entry point as the `📄 SLD` button / `onVlOpen`). | +| **Single-click (disk)** | Selects the VL: fills the bottom-left **Inspect** field with the VL id, auto-zooms / highlights it, and surfaces the `📄 SLD` shortcut — exactly as typing the name into the Inspect box would. | +| **Single-click (name box)** | Opens the VL's Single Line Diagram overlay directly — the name label next to the disk is a second, larger click target for the SLD. | +| **Double-click (disk or name box)** | Opens the VL's Single Line Diagram overlay (the same entry point as the `📄 SLD` button / `onVlOpen`). | This is the disk-driven complement to the **Inspect** search field: the field finds an asset by name; the disk finds it by pointing at it. +### The whole disk is a target, even under a branch + +A branch is often drawn ON TOP of a VL disk, which would otherwise steal +the pointer hit-test from the disk beneath it. The handlers resolve this: +when the direct hit-test lands on something that is not a VL (an occluding +edge, or empty space), they fall back to `document.elementsFromPoint` and +take the **first VL disk / name box in the paint stack** under the cursor. +So the disk is interactive across its whole area regardless of what is +painted over it. The fallback only runs on discrete pointer events +(`mousedown` / hover-while-labels-hidden), never per frame, so the +performance contract below is unchanged. + +### The name box is resolved through the metadata + +pypowsybl renders each VL name as a `
` +inside a root-level ``. The box `id` +is the NAD metadata **text-node** svgId, which the metadata index +(`metaIndex.textNodesBySvgId`, built from `metadata.textNodes` via each +entry's `vlNode` link / `equipmentId`) maps to the same VL `NodeMeta` the +disk resolves to. A delegated handler climbs from the click target to the +first ancestor whose `id` is a known disk **or** text-node svgId. + ## Where it lives - **`frontend/src/utils/svg/vlInteractions.ts`** — `attachVlInteractions(container, metaIndex, handlers)`. @@ -21,15 +44,19 @@ field finds an asset by name; the disk finds it by pointing at it. - **`frontend/src/App.tsx`** — a single effect binds the three NAD containers, mapping each gesture to the existing handlers: - `onSelect(vlId)` → `handleInspectQueryChangeFor(tab, vlId)` (drives - the per-tab Inspect query + auto-zoom in `useDiagrams`). + the per-tab Inspect query + auto-zoom in `useDiagrams`). Fired by a + single-click on the **disk**. - `onOpenSld(vlId)` → `handleVlOpen(vlId)` → `useSldOverlay.handleVlDoubleClick`. + Fired by a **double-click** on the disk / name box **and** by a + single-click on the **name box**. The callbacks are held in refs (`vlSelectRef` / `vlOpenSldRef` / `vlDisplayNameRef`) so the listeners re-bind **only** when a diagram or its metadata index changes — never on an unrelated render, which would needlessly tear down the listeners and cancel an in-flight single-click timer. -- **`frontend/src/App.css`** — `.svg-container .nad-vl-nodes { cursor: pointer }` - is the (static) pointer-cursor affordance. +- **`frontend/src/App.css`** — `.svg-container .nad-vl-nodes, .svg-container + .nad-label-box { cursor: pointer }` is the (static) pointer-cursor + affordance on both the disk and the name box. `onSelect` / `onOpenSld` reuse pre-existing interaction events (`inspect_query_changed`, `sld_overlay_opened`), so the replay log stays diff --git a/frontend/CLAUDE.md b/frontend/CLAUDE.md index 1432d3e..d1775fd 100644 --- a/frontend/CLAUDE.md +++ b/frontend/CLAUDE.md @@ -73,10 +73,12 @@ frontend/ │ ├── useTheme.ts # Light/dark theme toggle + persistence │ │ # (0.8.0; see docs/features/dark-mode.md) │ ├── useSldFeederRelabel.ts # Relabel SLD branch feeders with the far- - │ │ # end VL name + wrap long labels (Issue 1; - │ │ # render-every-time self-gate, delegates the - │ │ # DOM swap to - │ │ # utils/svg/feederLabels.applyFeederRelabels) + │ │ # end VL name + wrap EVERY long feeder name + │ │ # (generators / loads / unmatched branches) + │ │ # so extremity names stop overlapping (Issue + │ │ # 1; render-every-time self-gate, delegates + │ │ # the DOM swap to feederLabels + │ │ # .applyFeederRelabels + .applyFeederLabelWrap) │ ├── useSldFeederNav.ts # Click a relabelled feeder name → open the │ │ # far-end VL's SLD (reads data-feeder-nav; │ │ # capture-phase, keeps the current sub-tab so diff --git a/frontend/src/App.css b/frontend/src/App.css index 4fc7faf..5feed86 100644 --- a/frontend/src/App.css +++ b/frontend/src/App.css @@ -178,11 +178,13 @@ This file is part of Co-Study4Grid a Power Grid Study tool Assistant Interface t /* Voltage-level disks are interactive: single-click selects (Inspect + auto-zoom), double-click opens the SLD, hover shows the name while the - static labels are hidden (see utils/svg/vlInteractions.ts). The pointer + static labels are hidden (see utils/svg/vlInteractions.ts). The VL name + box is interactive too — a single click opens the SLD. The pointer cursor is a static rule so the affordance costs nothing at render or gesture time; during a pan/zoom `.svg-interacting` disables SVG hit- testing, so the container's grab/grabbing cursor wins instead. */ -.svg-container .nad-vl-nodes { +.svg-container .nad-vl-nodes, +.svg-container .nad-label-box { cursor: pointer; } diff --git a/frontend/src/components/SldOverlay.tsx b/frontend/src/components/SldOverlay.tsx index 2e49916..5d67ba0 100644 --- a/frontend/src/components/SldOverlay.tsx +++ b/frontend/src/components/SldOverlay.tsx @@ -1034,14 +1034,17 @@ const SldOverlay: React.FC = ({ } }); + // Relabel branch feeders with the far-end VL name + wrap long feeder names + // (Issue 1) — pass + its self-gate live in the hook (see + // utils/svg/feederLabels.ts for the swap). Runs BEFORE the injection-name + // buttons below so those compute their bounding box on the wrapped, + // multi-line name rather than the original single line. + useSldFeederRelabel(overlayBodyRef, vlOverlay.feeder_labels, activeSvg, vlOverlay.tab, vlOverlay.actionId, previewActive); + // Render every editable injection's NAME as a clickable button (opens the // active-power editor) — pass + its self-gate live in the hook. useSldInjectionNameButtons(overlayBodyRef, vlOverlay.injections, editMode, activeSvg, vlOverlay.tab, vlOverlay.actionId, previewActive); - // Relabel branch feeders with the far-end VL name (Issue 1) — pass + its - // self-gate live in the hook (see utils/svg/feederLabels.ts for the swap). - useSldFeederRelabel(overlayBodyRef, vlOverlay.feeder_labels, activeSvg, vlOverlay.tab, vlOverlay.actionId, previewActive); - // Click a relabelled feeder name → open the far-end VL's SLD. Registered // BEFORE the edit-mode click effect below so its stop-immediate wins on a // shared capture-phase click. diff --git a/frontend/src/hooks/useSldFeederRelabel.ts b/frontend/src/hooks/useSldFeederRelabel.ts index f9cbb0a..42f4112 100644 --- a/frontend/src/hooks/useSldFeederRelabel.ts +++ b/frontend/src/hooks/useSldFeederRelabel.ts @@ -7,12 +7,14 @@ import { useLayoutEffect, useRef, type RefObject } from 'react'; import type { FeederLabel, SldTab } from '../types'; -import { applyFeederRelabels } from '../utils/svg/feederLabels'; +import { applyFeederRelabels, applyFeederLabelWrap } from '../utils/svg/feederLabels'; /** - * Relabel SLD branch feeders with the far-end VL name (Issue 1). Owns the - * render-every-time + signature self-gate so a pan reconciliation that drops - * the relabel re-applies it, mirroring the other SldOverlay label passes. + * Relabel SLD branch feeders with the far-end VL name (Issue 1) AND wrap every + * remaining long feeder NAME (generators / loads / unmatched branches) so the + * names stop overlapping at the substation extremities. Owns the + * render-every-time + self-gate so a pan reconciliation that drops either pass + * re-applies it, mirroring the other SldOverlay label passes. */ export function useSldFeederRelabel( bodyRef: RefObject, @@ -22,7 +24,7 @@ export function useSldFeederRelabel( actionId: string | null, preview: boolean, ): void { - const sigRef = useRef(''); + const stateRef = useRef({ sig: '', relabel: false, wrap: false }); useLayoutEffect(() => { const container = bodyRef.current; if (!container) return; @@ -36,10 +38,20 @@ export function useSldFeederRelabel( preview, labels: entries.map(([eid, info]) => `${eid}=${info.label}`).sort(), }); - const applied = container.querySelector('[data-feeder-relabel]') !== null; - const expect = entries.length > 0 && !!activeSvg; - if (sig === sigRef.current && (expect ? applied : !applied)) return; - sigRef.current = sig; + const relabelPresent = container.querySelector('[data-feeder-relabel]') !== null; + const wrapPresent = container.querySelector('[data-feeder-wrap]') !== null; + // Re-run when the inputs changed, or when a reconciliation dropped the + // marks we last produced (the DOM diverged from our recorded state). + const consistent = + stateRef.current.relabel === relabelPresent && + stateRef.current.wrap === wrapPresent; + if (sig === stateRef.current.sig && consistent) return; applyFeederRelabels(container, feederLabels, activeSvg); + applyFeederLabelWrap(container, activeSvg); + stateRef.current = { + sig, + relabel: container.querySelector('[data-feeder-relabel]') !== null, + wrap: container.querySelector('[data-feeder-wrap]') !== null, + }; }); } diff --git a/frontend/src/types.ts b/frontend/src/types.ts index f50f881..e978a9c 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -308,6 +308,10 @@ export interface MetadataIndex { nodesBySvgId: Map; edgesByEquipmentId: Map; edgesByNode: Map; + /** VL text-label (name box) svgId → its VL NodeMeta, so the label box can + * be interactive (open the SLD) exactly like the VL disk. Optional because + * the many hand-built MetadataIndex test fixtures omit it. */ + textNodesBySvgId?: Map; } export interface DiagramPatch { diff --git a/frontend/src/utils/svg/feederLabels.test.ts b/frontend/src/utils/svg/feederLabels.test.ts index ba98834..3011bc4 100644 --- a/frontend/src/utils/svg/feederLabels.test.ts +++ b/frontend/src/utils/svg/feederLabels.test.ts @@ -5,7 +5,7 @@ // SPDX-License-Identifier: MPL-2.0 import { describe, it, expect } from 'vitest'; -import { buildFriendlyToEquip, overloadCandidates, applyFeederRelabels, wrapFeederLabel } from './feederLabels'; +import { buildFriendlyToEquip, overloadCandidates, applyFeederRelabels, applyFeederLabelWrap, wrapFeederLabel } from './feederLabels'; import type { FeederLabel } from '../../types'; const fl = (over: Partial> = {}): Record => ({ @@ -51,9 +51,17 @@ describe('wrapFeederLabel', () => { expect(wrapFeederLabel('MARSILLON 225kV')).toEqual(['MARSILLON 225kV']); }); - it('never wraps a single word (no spaces)', () => { - expect(wrapFeederLabel('virtual_relation_8423568_a_0-225')) - .toEqual(['virtual_relation_8423568_a_0-225']); + it('keeps a short single word (no spaces) on one line', () => { + expect(wrapFeederLabel('relation_842')).toEqual(['relation_842']); + }); + + it('breaks a long single word on its separators and preserves every char', () => { + const lines = wrapFeederLabel('virtual_relation_8423568_a_0-225'); + expect(lines.length).toBeGreaterThan(1); + expect(lines.length).toBeLessThanOrEqual(3); + expect(lines.join('')).toBe('virtual_relation_8423568_a_0-225'); + // Each line stays within the wrap budget (last one may fold overflow). + expect(lines[0].length).toBeLessThanOrEqual(15); }); it('wraps a long label on spaces and preserves every token', () => { @@ -136,3 +144,89 @@ describe('applyFeederRelabels', () => { expect(container.querySelector('#t')!.getAttribute('data-feeder-nav')).toBeNull(); }); }); + +describe('applyFeederLabelWrap', () => { + const mount = (svg: string): HTMLElement => { + const div = document.createElement('div'); + div.innerHTML = svg; + return div; + }; + + it('wraps a long generator / load name inside a feeder cell', () => { + const container = mount( + '' + + 'G_virtual_relation_8423568_a_0-225' + + '', + ); + applyFeederLabelWrap(container, ''); + const t = container.querySelector('#g')!; + expect(t.getAttribute('data-feeder-wrap')).toBe('1'); + expect(t.querySelectorAll('tspan').length).toBeGreaterThan(1); + expect(t.getAttribute('data-feeder-wrap-orig')).toBe('G_virtual_relation_8423568_a_0-225'); + }); + + it('leaves a short name untouched', () => { + const container = mount( + 'IGNERES 225kV', + ); + applyFeederLabelWrap(container, ''); + const t = container.querySelector('#s')!; + expect(t.hasAttribute('data-feeder-wrap')).toBe(false); + expect(t.textContent).toBe('IGNERES 225kV'); + }); + + it('skips the numeric P/Q flow labels', () => { + const container = mount( + '' + + '-1234567.89' + + '-16.8 MVAr' + + '', + ); + applyFeederLabelWrap(container, ''); + expect(container.querySelector('#p')!.hasAttribute('data-feeder-wrap')).toBe(false); + expect(container.querySelector('#q')!.hasAttribute('data-feeder-wrap')).toBe(false); + }); + + it('leaves an already-relabelled feeder alone (relabel wraps its own)', () => { + const container = mount( + '' + + 'LANNEMEZAN 225kV 1' + + '', + ); + applyFeederLabelWrap(container, ''); + expect(container.querySelector('#r')!.hasAttribute('data-feeder-wrap')).toBe(false); + }); + + it('ignores long text outside a feeder cell (e.g. the busbar label)', () => { + const container = mount( + 'VL_virtual_relation_8423568_a_0-225_BBS1', + ); + applyFeederLabelWrap(container, ''); + expect(container.querySelector('#bb')!.hasAttribute('data-feeder-wrap')).toBe(false); + }); + + it('restores the original text on a subsequent call with no svg', () => { + const container = mount( + '' + + 'G_virtual_relation_8423568_a_0-225' + + '', + ); + applyFeederLabelWrap(container, ''); + applyFeederLabelWrap(container, null); + const t = container.querySelector('#g')!; + expect(t.hasAttribute('data-feeder-wrap')).toBe(false); + expect(t.textContent).toBe('G_virtual_relation_8423568_a_0-225'); + }); + + it('skips highlight clones', () => { + const container = mount( + '' + + 'G_virtual_relation_8423568_a_0-225' + + 'G_virtual_relation_8423568_a_0-225' + + '', + ); + applyFeederLabelWrap(container, ''); + expect(container.querySelector('#orig')!.getAttribute('data-feeder-wrap')).toBe('1'); + expect(container.querySelector('.sld-highlight-clone text')!.hasAttribute('data-feeder-wrap')).toBe(false); + }); +}); diff --git a/frontend/src/utils/svg/feederLabels.ts b/frontend/src/utils/svg/feederLabels.ts index 6e3e099..0b3786c 100644 --- a/frontend/src/utils/svg/feederLabels.ts +++ b/frontend/src/utils/svg/feederLabels.ts @@ -48,24 +48,57 @@ const SVG_NS = 'http://www.w3.org/2000/svg'; const FEEDER_WRAP_CHARS = 15; const FEEDER_MAX_LINES = 3; +/** + * Break a single word wider than ``budget`` into pieces no wider than it, + * preferring a natural separator (``_`` ``-`` ``.``) at or before the budget so + * an id like ``virtual_relation_8423568`` splits between segments rather than + * mid-token; falls back to a hard character cut when a chunk has no separator. + * The separator stays attached to the preceding piece so the pieces re-join + * exactly into the original word. + */ +function breakLongWord(word: string, budget: number): string[] { + if (word.length <= budget) return [word]; + const pieces: string[] = []; + let rest = word; + while (rest.length > budget) { + let cut = -1; + for (let i = Math.min(budget, rest.length - 1); i >= 1; i--) { + const ch = rest[i]; + if (ch === '_' || ch === '-' || ch === '.') { cut = i + 1; break; } + } + if (cut <= 0) cut = budget; // no separator in range → hard split + pieces.push(rest.slice(0, cut)); + rest = rest.slice(cut); + } + if (rest) pieces.push(rest); + return pieces; +} + /** * Break a feeder label into at most ``FEEDER_MAX_LINES`` lines no wider than - * ``FEEDER_WRAP_CHARS``, splitting on whitespace. A single word longer than the - * budget is left intact on its own line (hard-splitting an id / name reads - * worse than one slightly-long line). Overflow past the line cap is folded back - * into the last line. + * ``FEEDER_WRAP_CHARS``. Splits on whitespace first, and a single word still + * wider than the budget (a long raw IIDM id — ``L_virtual_relation_8423568…``) + * is further broken on its ``_`` / ``-`` / ``.`` separators so it stops running + * horizontally off its feeder and overprinting the neighbour. Overflow past the + * line cap is folded back into the last line. */ export function wrapFeederLabel(label: string): string[] { - if (label.length <= FEEDER_WRAP_CHARS || !label.includes(' ')) return [label]; - const words = label.split(/\s+/).filter(Boolean); + if (label.length <= FEEDER_WRAP_CHARS) return [label]; const lines: string[] = []; let cur = ''; - for (const w of words) { - if (!cur) cur = w; - else if ((cur + ' ' + w).length <= FEEDER_WRAP_CHARS) cur += ' ' + w; - else { lines.push(cur); cur = w; } + for (const word of label.split(/\s+/).filter(Boolean)) { + // A word that still fits on the current line joins it with a space. + if (cur && (cur + ' ' + word).length <= FEEDER_WRAP_CHARS) { + cur += ' ' + word; + continue; + } + if (cur) { lines.push(cur); cur = ''; } + const pieces = breakLongWord(word, FEEDER_WRAP_CHARS); + for (let i = 0; i < pieces.length - 1; i++) lines.push(pieces[i]); + cur = pieces[pieces.length - 1]; } if (cur) lines.push(cur); + if (lines.length === 0) return [label]; if (lines.length > FEEDER_MAX_LINES) { const head = lines.slice(0, FEEDER_MAX_LINES - 1); head.push(lines.slice(FEEDER_MAX_LINES - 1).join(' ')); @@ -150,3 +183,48 @@ export function applyFeederRelabels( } } } + +/** Feeder cells whose name label sits at the diagram extremity. */ +const FEEDER_CELL_SELECTOR = '.sld-extern-cell, .sld-intern-cell, .sld-shunt-cell'; +/** A flow value (``-14`` / ``16.8 MVAr``) rather than an equipment name. */ +const NUMERIC_LABEL_RE = /^[+-]?[\d.,\s]+(?:\s*[A-Za-z%°]+)?$/; + +/** + * Wrap every long feeder NAME label at a substation's extremities so the names + * stop overlapping — the generator / load / unmatched-branch names that + * :func:`applyFeederRelabels` does NOT rewrite (only far-end-named branches get + * relabelled). Targets the equipment-name ```` inside each feeder cell + * (excluding the numeric P/Q flow labels and the already-relabelled feeders, + * which wrap themselves) and rewrites it with the same centred multi-line + * ```` block used for relabels. + * + * Idempotent: the original text is stashed in ``data-feeder-wrap-orig`` and + * restored first, and highlight clones (removed on the next render) are skipped. + * Run AFTER :func:`applyFeederRelabels` so relabelled feeders are left alone. + */ +export function applyFeederLabelWrap( + container: HTMLElement, + activeSvg: string | null, +): void { + container.querySelectorAll('[data-feeder-wrap]').forEach(el => { + const orig = el.getAttribute('data-feeder-wrap-orig'); + if (orig !== null) el.textContent = orig; // replacing textContent drops any wrap s + el.removeAttribute('data-feeder-wrap'); + el.removeAttribute('data-feeder-wrap-orig'); + }); + if (!activeSvg) return; + + container.querySelectorAll(FEEDER_CELL_SELECTOR).forEach(cell => { + cell.querySelectorAll('text').forEach(textEl => { + if (textEl.closest('.sld-highlight-clone')) return; + if (textEl.hasAttribute('data-feeder-relabel')) return; // relabel wraps its own + if (textEl.closest('.sld-active-power, .sld-reactive-power')) return; // flow value + const raw = (textEl.textContent ?? '').trim(); + if (raw.length <= FEEDER_WRAP_CHARS || NUMERIC_LABEL_RE.test(raw)) return; + if (wrapFeederLabel(raw).length <= 1) return; + textEl.setAttribute('data-feeder-wrap-orig', textEl.textContent ?? ''); + textEl.setAttribute('data-feeder-wrap', '1'); + setFeederText(textEl, raw); + }); + }); +} diff --git a/frontend/src/utils/svg/metadataIndex.test.ts b/frontend/src/utils/svg/metadataIndex.test.ts index 577b61f..ddc882e 100644 --- a/frontend/src/utils/svg/metadataIndex.test.ts +++ b/frontend/src/utils/svg/metadataIndex.test.ts @@ -66,5 +66,31 @@ describe('buildMetadataIndex', () => { const idx = buildMetadataIndex({})!; expect(idx.nodesByEquipmentId.size).toBe(0); expect(idx.edgesByEquipmentId.size).toBe(0); + expect(idx.textNodesBySvgId?.size).toBe(0); + }); + + it('maps VL text-label nodes to their VL via the vlNode link', () => { + const idx = buildMetadataIndex({ + nodes: [{ equipmentId: 'VL_A', svgId: 'svg-a', x: 0, y: 0 }], + textNodes: [{ svgId: 'text-a', equipmentId: 'VL_A', vlNode: 'svg-a' }], + edges: [], + })!; + expect(idx.textNodesBySvgId?.get('text-a')?.equipmentId).toBe('VL_A'); + }); + + it('falls back to equipmentId when a text node has no vlNode link', () => { + const idx = buildMetadataIndex({ + nodes: [{ equipmentId: 'VL_A', svgId: 'svg-a', x: 0, y: 0 }], + textNodes: [{ svgId: 'text-a', equipmentId: 'VL_A' }], + })!; + expect(idx.textNodesBySvgId?.get('text-a')?.svgId).toBe('svg-a'); + }); + + it('skips text nodes that resolve to no known VL', () => { + const idx = buildMetadataIndex({ + nodes: [{ equipmentId: 'VL_A', svgId: 'svg-a', x: 0, y: 0 }], + textNodes: [{ svgId: 'text-x', equipmentId: 'VL_MISSING', vlNode: 'svg-missing' }], + })!; + expect(idx.textNodesBySvgId?.size).toBe(0); }); }); diff --git a/frontend/src/utils/svg/metadataIndex.ts b/frontend/src/utils/svg/metadataIndex.ts index cfaa08b..9c33043 100644 --- a/frontend/src/utils/svg/metadataIndex.ts +++ b/frontend/src/utils/svg/metadataIndex.ts @@ -6,6 +6,15 @@ import type { MetadataIndex, NodeMeta, EdgeMeta } from '../../types'; +/** Raw pypowsybl NAD `textNodes` entry — the VL name-box label metadata. */ +interface RawTextNode { + svgId?: string; + equipmentId?: string; + vlNode?: string; + node?: string; + connectedNode?: string; +} + /** * Build Map indices from pypowsybl metadata for O(1) lookups. * @@ -17,11 +26,13 @@ export const buildMetadataIndex = (metadata: unknown): MetadataIndex | null => { const meta = typeof metadata === 'string' ? JSON.parse(metadata) : metadata; const nodes: NodeMeta[] = (meta as { nodes?: NodeMeta[] }).nodes || []; const edges: EdgeMeta[] = (meta as { edges?: EdgeMeta[] }).edges || []; + const textNodes: RawTextNode[] = (meta as { textNodes?: RawTextNode[] }).textNodes || []; const nodesByEquipmentId = new Map(); const nodesBySvgId = new Map(); const edgesByEquipmentId = new Map(); const edgesByNode = new Map(); + const textNodesBySvgId = new Map(); nodes.forEach(n => { nodesByEquipmentId.set(n.equipmentId, n); @@ -36,5 +47,18 @@ export const buildMetadataIndex = (metadata: unknown): MetadataIndex | null => { edgesByNode.get(e.node2)!.push(e); }); - return { nodesByEquipmentId, nodesBySvgId, edgesByEquipmentId, edgesByNode }; + // pypowsybl NAD metadata carries a `textNodes` array for the VL name boxes, + // each linking back to its VL node (`vlNode` svgId) and equipment id. Map + // the label svgId → the VL's NodeMeta so a click on the name box resolves + // to the same VL the disk does. + textNodes.forEach(t => { + if (!t.svgId) return; + const vlNodeId = t.vlNode ?? t.node ?? t.connectedNode; + const vl = + (vlNodeId ? nodesBySvgId.get(vlNodeId) : undefined) ?? + (t.equipmentId ? nodesByEquipmentId.get(t.equipmentId) : undefined); + if (vl) textNodesBySvgId.set(t.svgId, vl); + }); + + return { nodesByEquipmentId, nodesBySvgId, edgesByEquipmentId, edgesByNode, textNodesBySvgId }; }; diff --git a/frontend/src/utils/svg/vlInteractions.test.ts b/frontend/src/utils/svg/vlInteractions.test.ts index d1cadc0..745e674 100644 --- a/frontend/src/utils/svg/vlInteractions.test.ts +++ b/frontend/src/utils/svg/vlInteractions.test.ts @@ -271,6 +271,52 @@ describe('attachVlInteractions', () => { expect(onSelect).toHaveBeenCalledWith('VL_225'); }); + it('resolves the disk under an occluding edge via the paint stack', () => { + const { container, disk } = makeContainer(); + // A branch drawn on top of the disk steals the direct hit-test. + const edge = document.createElementNS(SVG_NS, 'path'); + edge.id = 'edge-1'; + container.querySelector('svg')!.appendChild(edge); + + const onSelect = vi.fn(); + cleanups.push(attachVlInteractions(container, makeMetaIndex(), { onSelect })); + + // elementsFromPoint returns the occluding edge first, then the disk. + const orig = document.elementsFromPoint; + document.elementsFromPoint = vi.fn(() => [edge, disk]) as typeof document.elementsFromPoint; + + edge.dispatchEvent(mouse('mousedown')); + edge.dispatchEvent(mouse('click')); + vi.advanceTimersByTime(VL_SINGLE_CLICK_DELAY_MS); + expect(onSelect).toHaveBeenCalledWith('VL_400'); + document.elementsFromPoint = orig; + }); + + it('opens the SLD (not select) when the VL name box is clicked', () => { + const { container } = makeContainer(); + const fo = document.createElementNS(SVG_NS, 'foreignObject'); + fo.setAttribute('class', 'nad-text-nodes'); + const box = document.createElement('div'); + box.id = 'vl1-label'; + box.className = 'nad-label-box'; + box.textContent = 'PARIS 400kV'; + fo.appendChild(box); + container.querySelector('svg')!.appendChild(fo); + + const meta = makeMetaIndex(); + meta.textNodesBySvgId = new Map([['vl1-label', meta.nodesBySvgId.get('vl1-svg')!]]); + + const onSelect = vi.fn(); + const onOpenSld = vi.fn(); + cleanups.push(attachVlInteractions(container, meta, { onSelect, onOpenSld })); + + box.dispatchEvent(mouse('mousedown')); + box.dispatchEvent(mouse('click')); + vi.advanceTimersByTime(VL_SINGLE_CLICK_DELAY_MS); + expect(onOpenSld).toHaveBeenCalledWith('VL_400'); + expect(onSelect).not.toHaveBeenCalled(); + }); + it('does not fire a queued single-click after teardown', () => { const { container, disk } = makeContainer(); const onSelect = vi.fn(); diff --git a/frontend/src/utils/svg/vlInteractions.ts b/frontend/src/utils/svg/vlInteractions.ts index c9b4692..fabbc47 100644 --- a/frontend/src/utils/svg/vlInteractions.ts +++ b/frontend/src/utils/svg/vlInteractions.ts @@ -15,16 +15,27 @@ import { colors } from '../../styles/tokens'; * `nad-hide-vl-labels` class, toggled by the `🏷 VL` * button). When the labels are visible the name is already * drawn, so the tooltip stays out of the way. - * 2. Click — single-click selects the VL (drives the Inspect field / - * auto-zoom, exactly like typing it in the Inspect box). - * 3. Dbl-clk — double-click opens the VL's Single Line Diagram. + * 2. Click — single-click on the DISK selects the VL (drives the + * Inspect field / auto-zoom, exactly like typing it in the + * Inspect box); single-click on the VL NAME BOX opens its + * Single Line Diagram directly. + * 3. Dbl-clk — double-click on the disk (or name box) opens the VL's SLD. + * + * The disk is interactive across its WHOLE area, even where a branch is + * drawn on top of it: when the direct hit-test lands on an occluding edge + * (not the disk), we fall back to `document.elementsFromPoint` and pick the + * first VL disk / name box in the paint stack under the cursor. This runs + * only on discrete pointer events (never per frame), so the performance + * contract below is untouched. * * Performance contract (the whole point of the delegation design): * - A FIXED handful of listeners on the container, never one-per-node, * so a 5000-VL grid costs the same as a 5-VL grid to wire up. * - NO `mousemove` / per-frame work: the tooltip is positioned once on * `mouseover` and the cursor affordance is a static CSS rule - * (`.svg-container .nad-vl-nodes { cursor: pointer }` in App.css). + * (`.svg-container .nad-vl-nodes` / `.nad-label-box { cursor: pointer }` + * in App.css). The `elementsFromPoint` fallback fires only when the + * direct hit-test misses a VL, i.e. on an occluding edge or empty space. * - Pan/zoom gestures add `.svg-interacting`, which sets * `pointer-events: none` on every SVG child, so none of these * handlers resolve a node mid-gesture — fluidity is untouched. @@ -74,23 +85,50 @@ export const attachVlInteractions = ( if (!container || !metaIndex || metaIndex.nodesBySvgId.size === 0) return NOOP; const { nodesBySvgId } = metaIndex; + const textNodesBySvgId = metaIndex.textNodesBySvgId ?? new Map(); const { onSelect, onOpenSld, displayName } = handlers; - // Climb from the event target to the enclosing VL node group (the - // element whose id is a known node svgId), if any. - const resolveVl = (target: EventTarget | null): NodeMeta | null => { + // What a hit resolved to: the VL, and whether it landed on the name box + // (`viaText`) rather than the disk — the two get different click actions. + interface Resolved { node: NodeMeta; viaText: boolean; } + + // Climb from the event target to the enclosing VL node group (disk) or VL + // name box (`nad-label-box`, keyed by its text-node svgId), if any. + const resolveDirect = (target: EventTarget | null): Resolved | null => { let el = target as Element | null; while (el && el !== container) { const id = el.id; if (id) { const node = nodesBySvgId.get(id); - if (node) return node; + if (node) return { node, viaText: false }; + const textNode = textNodesBySvgId.get(id); + if (textNode) return { node: textNode, viaText: true }; } el = el.parentElement; } return null; }; + // Resolve a VL under the pointer even when a branch is drawn ON TOP of its + // disk: the direct hit-test then lands on the edge, so we walk the whole + // paint stack at (clientX, clientY) and take the first element that resolves + // to a VL. Only runs when the direct hit misses — never per frame. + const resolveDeep = ( + target: EventTarget | null, + clientX: number, + clientY: number, + ): Resolved | null => { + const direct = resolveDirect(target); + if (direct) return direct; + if (typeof document.elementsFromPoint !== 'function') return null; + for (const el of document.elementsFromPoint(clientX, clientY)) { + if (el === container || !container.contains(el)) continue; + const hit = resolveDirect(el); + if (hit) return hit; + } + return null; + }; + const tooltipText = (node: NodeMeta): string => { const id = node.equipmentId; const friendly = displayName ? displayName(id) : id; @@ -139,20 +177,22 @@ export const attachVlInteractions = ( }; const onOver = (evt: MouseEvent) => { - const node = resolveVl(evt.target); - if (!node) return; - hoveredSvgId = node.svgId; - // The name is already drawn when labels are visible — only - // surface the tooltip when the static labels are hidden. - if (container.classList.contains(HIDE_VL_LABELS_CLASS)) { - showTooltip(node, evt.clientX, evt.clientY); - } + // The name is already drawn when labels are visible — only surface the + // tooltip when the static labels are hidden. Gating the (slightly more + // expensive) `elementsFromPoint` fallback on that keeps hover cheap. + const labelsHidden = container.classList.contains(HIDE_VL_LABELS_CLASS); + const res = labelsHidden + ? resolveDeep(evt.target, evt.clientX, evt.clientY) + : resolveDirect(evt.target); + if (!res) return; + hoveredSvgId = res.node.svgId; + if (labelsHidden) showTooltip(res.node, evt.clientX, evt.clientY); }; const onOut = (evt: MouseEvent) => { // Ignore transitions that stay inside the same VL group. - const to = resolveVl(evt.relatedTarget); - if (to && to.svgId === hoveredSvgId) return; + const to = resolveDirect(evt.relatedTarget); + if (to && to.node.svgId === hoveredSvgId) return; hoveredSvgId = null; hideTooltip(); }; @@ -171,21 +211,30 @@ export const attachVlInteractions = ( let downX = 0; let downY = 0; let downVl: NodeMeta | null = null; + let downViaText = false; let clickTimer: ReturnType | null = null; const onDown = (evt: MouseEvent) => { downX = evt.clientX; downY = evt.clientY; - downVl = resolveVl(evt.target); + const res = resolveDeep(evt.target, evt.clientX, evt.clientY); + downVl = res ? res.node : null; + downViaText = res ? res.viaText : false; }; const onClick = (evt: MouseEvent) => { // A pan (pointer travelled) is not a selection. if (Math.hypot(evt.clientX - downX, evt.clientY - downY) > DRAG_THRESHOLD_PX) return; - // Second click of a double-click — let `dblclick` take over. - if (clickTimer !== null) return; const node = downVl; if (!node) return; + // Clicking the VL NAME BOX opens its SLD directly — no double-click + // window to guard (the box carries no competing single-click action). + if (downViaText) { + onOpenSld?.(node.equipmentId); + return; + } + // Second click of a double-click — let `dblclick` take over. + if (clickTimer !== null) return; const vlId = node.equipmentId; clickTimer = setTimeout(() => { clickTimer = null; From 738ffcd4945e67abaa7ad7fc0caf6faa41faf038 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 17:11:40 +0000 Subject: [PATCH 3/8] Add 2026-07 full-repository review to docs/architecture 12-dimension audit (backend/frontend architecture, API contract, UX, performance, documentation, maintainability, security, plus data-pipeline, Game Mode and deployment gap reviews) with adversarially verified findings (34 confirmed / 23 partially confirmed / 0 refuted), a sequenced deep-revision roadmap (D1-D9) and 25 quick wins. Indexed in docs/README.md. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014FmiTR5B6ud726XCKDyxVU --- docs/README.md | 1 + docs/architecture/2026-07-full-repo-review.md | 611 ++++++++++++++++++ 2 files changed, 612 insertions(+) create mode 100644 docs/architecture/2026-07-full-repo-review.md diff --git a/docs/README.md b/docs/README.md index 3dc27a9..ed75434 100644 --- a/docs/README.md +++ b/docs/README.md @@ -74,6 +74,7 @@ folder's index for the full list. | File | Topic | |------|-------| +| [`2026-07-full-repo-review.md`](architecture/2026-07-full-repo-review.md) | Full-repository review (2026-07): 12-dimension audit (architecture, API contract, UX, perf, docs, delivery, security + data pipeline / Game Mode / deployment gaps) with adversarially verified findings, a sequenced deep-revision roadmap (D1–D9), and 25 quick wins. | | [`app-refactoring-plan.md`](architecture/app-refactoring-plan.md) | Historical: Phase 1 + Phase 2 hook extraction from `App.tsx` (shipped). | | [`phase2-state-management-optimization.md`](architecture/phase2-state-management-optimization.md) | Memoize wrapper functions with `useCallback` (shipped 0.5.0). | | [`code-quality-analysis.md`](architecture/code-quality-analysis.md) | Continuous audit; latest deltas (§14–15) cover the 0.7.0 release + the function-LoC ceiling, postMessage envelope, FastAPI return-type follow-ups. | diff --git a/docs/architecture/2026-07-full-repo-review.md b/docs/architecture/2026-07-full-repo-review.md new file mode 100644 index 0000000..dc6e2b7 --- /dev/null +++ b/docs/architecture/2026-07-full-repo-review.md @@ -0,0 +1,611 @@ +# Co-Study4Grid — Full Repository Review (2026-07) + +**Scope**: code architecture (backend + frontend), interface & interaction design, +performance quality & bottlenecks, documentation, maintainability & delivery, +robustness/security, plus three gap areas (PyPSA-EUR data pipeline, Game Mode / +Codabench, deployment & release engineering). + +**Method**: 12 independent dimension reviews executed by parallel agents reading the +actual code (not the docs), followed by an adversarial verification pass in which +every high/medium-severity finding was attacked by a skeptic instructed to refute +it, and a completeness critic that identified uncovered subsystems. 22 agents, +~490 tool invocations, ~1.46 M tokens of analysis. + +**Verification outcome**: 57 high/medium findings verified — **34 confirmed, +23 partially confirmed (corrected below), 0 refuted**. Low-severity findings and +the three gap reviews were not adversarially verified and are flagged as such. + +--- + +## Part I — Principles and method behind this review + +A large review is only useful if its findings survive scrutiny and its proposals +can be sequenced. Seven principles structured this one: + +1. **Evidence over impression.** Every strength and weakness must cite + `file:line`, a measured count, or a commit hash. "App.tsx feels big" is not a + finding; "App.tsx is 2,076 lines against its own CI ceiling of 2,100 + (`scripts/check_code_quality.py:128`), while three docs claim ~1,400" is. + +2. **Documentation is a claim to test, not a source of truth.** Reviewers were + explicitly forbidden from trusting the CLAUDE.md files. Every doc-vs-code + disagreement is itself a finding (this surfaced the invisible + `expert_backend/recommenders/` subsystem). + +3. **Independent lenses, then convergence as signal.** Nine reviewers each saw + one dimension and none saw the others' output. When four independent lenses + (backend architecture, maintainability, docs, API) all land on the same root + cause — the monkey-patched recommender integration — that convergence is + strong evidence the issue is structural, not stylistic. + +4. **Adversarial verification.** Optimistic reviewers over-report. Every + high/medium finding was handed to a verifier whose instruction was to *refute* + it: re-open the cited lines, hunt for mitigations the reviewer missed (caches, + guards, CI steps, docs), and deflate severity where the deployment reality + doesn't support it. 40 % of findings came back corrected — several materially + (e.g. "four NDJSON parser copies" is actually **five**; the "300 s tkinter + hang" doesn't occur on the headless image because tkinter is absent). + +5. **Calibration to purpose.** This is a research/operator tool with two real + deployment modes (localhost single-user; one-player HuggingFace Space), not a + multi-tenant SaaS. Severity was judged against fitness-for-purpose: "no + authentication" is not automatically high; "any web page can read local files + through the default CORS wildcard" is. + +6. **Strengths carry the same evidentiary burden.** A review that only lists + deficits misleads: it invites "fixing" things that are load-bearing. The + strengths below are verified in code, and several proposals deliberately + *extend existing patterns* the repo already proved (helper extraction, + ratcheting gates, token discipline) rather than importing foreign ones. + +7. **Completeness check + honest limits.** A critic agent scanned for subsystems + no reviewer covered and spawned three gap reviews (data pipeline, Game Mode, + deployment). What remains uncovered is stated in Part VII rather than implied + to be fine. + +Process: scout (repo inventory, line counts, CI wiring) → nine parallel dimension +reviews → per-dimension adversarial verification → completeness critic → three gap +reviews → synthesis with cross-dimension deduplication. + +--- + +## Part II — Executive summary + +**Overall verdict**: Co-Study4Grid is a *far better engineered* codebase than its +category (2-person research tool) predicts — measurement-driven performance work +with committed benchmarks, dual test suites larger than the code they test, a +genuinely enforced quality ratchet, and unusually deliberate interaction design. +Its debts are equally real and cluster around five root causes, four of which are +different faces of the same phenomenon: **hand-maintained mirrors that have +drifted** (service composition vs. its docs, backend JSON vs. `types.ts`, five +copies of one stream parser, two copies of the step-2 generator, two copies of the +scoring function, annotated file trees vs. the tree). + +The five headline issues, all verified: + +1. **The pluggable-recommender subsystem is a ghost.** + `expert_backend/recommenders/_service_integration.py` rewrites + `RecommenderService` at import time (setattr-grafts a mixin, wraps + `update_config`/`reset`, wholesale-replaces `run_analysis_step2`), leaving a + ~190-line near-duplicate legacy generator that already caused one shipped bug + (commit `401045f`). The 8-file root `tests/` package (144 test functions) that + covers exactly this integration is collected by no pytest config and no CI + pipeline — green-by-omission. The subsystem is absent from both CLAUDE.md + architecture trees. *(confirmed by 4 independent reviewers + verifiers)* + +2. **Nothing machine-checks the API contract.** Zero `response_model` on 37 + routes; the 825-line `types.ts` is a hand-maintained mirror; ~26 blanket + `except Exception → HTTP 400` blocks collapse the error space into five + different client-visible error shapes; the NDJSON parser is hand-copied five + times with divergent robustness fixes (only the two App.tsx copies flush an + unterminated final event). + +3. **Concurrency is unguarded where it is now real.** Zero locks in the backend; + the shared pypowsybl `Network` is variant-switched from FastAPI threadpool + workers (`Promise.all` batches, detached tabs, second Space visitor), one + diagram path switches variants without try/finally + (`diagram_mixin.py:151-160`), and `/api/run-analysis-step1` is `async def` + running seconds of synchronous pypowsybl work — it blocks the entire event + loop, freezing every other request. The single-user assumption is documented + (`docs/performance/history/grid2op-shared-network.md`) but no longer holds on + the Space. + +4. **Failures are invisible to the operator.** The analysis hook's `error` state + is never rendered anywhere: a backend 500 or a dropped connection mid-stream + ends the "Analyzing…" spinner with zero feedback. Two re-simulation catch + blocks swallow errors to `console.error`. There is no cancel for any + long-running operation. + +5. **The trust model no longer matches deployment.** Desktop-era filesystem RPCs + (`config-file-path` reads any JSON, `save-session` writes to any path, + `list-sessions` enumerates any directory) ship unauthenticated behind a + default CORS wildcard — a drive-by local file read/write vector on localhost + installs, and open filesystem access on the public Space (Game Mode gates only + the UI, not the API). + +Counterweights, equally verified: variant-lifecycle discipline is rigorous +(clone-from-clean-N, finally-restore, drain-before-mutate); svgPatch DOM recycling +is load-bearing and measured (3.01 s → 0.49 s, 27.1 MB → 5.5 MB); the API endpoint +table is 100 % accurate (37/37); the quality gate has historically been ratcheted +*down*, not up; the parity harness documents its own blind spots; security basics +(path traversal, zip-slip, XSS in the injected overlay, shell injection) are all +handled correctly. + +--- + +## Part III — Cross-cutting themes + +### T1. The recommenders epicenter *(backend, maintainability, docs, API)* +One root cause, four symptoms: import-time monkey-patching makes the production +code path undiscoverable from the service sources; the shadowed legacy generator +invites divergence (and has already diverged once); the covering tests never run; +the docs don't admit the subsystem exists. Any fix that addresses only one symptom +leaves the trap armed. → Deep revision **D1**. + +### T2. Hand-maintained mirrors as the dominant failure mode +Count the copies: 2× `run_analysis_step2`, 5× NDJSON parser, 2× scoring function +(one of them outside the repo, at a hardcoded developer home path), 2× CI pipeline +(GH Actions + CircleCI, already drifted), `types.ts` mirroring undeclared response +shapes, annotated file trees mirroring the tree, 3× calibration logic in the data +pipeline, Docker install recipe claiming to "mirror CI" while it doesn't. The repo +is disciplined, so the mirrors were *initially* faithful — but every one of them +has drifted, and two drifts already shipped bugs. The systemic fix is to replace +mirrors with either a single source (extract the shared parser; delete the +shadowed generator; one CI) or a machine check (OpenAPI artifact diff; docs-tree +checker; scoring golden fixture). → **D1, D2, D8, D9** + quick wins. + +### T3. Concurrency debt meeting a new deployment reality +The single-user, single-flight assumption was a documented, reasonable 2025 +decision. The 0.8.0 Space deployment and the frontend's own `Promise.all` batches +invalidated it. The event-loop-blocking `async def` is a one-keyword fix; the +missing coarse lock is a day; the variant/observation lifecycle (unbounded growth +within a session) is the longer tail. → **D3** + quick wins QW2, QW5. + +### T4. Error paths were never designed as UX +Backend: everything → 400, details leak absolute paths. Contract: five error +shapes. Frontend: one error state rendered, one never rendered, catches swallowed, +no cancellation, no aria-live. Each layer made a locally-reasonable choice; the +composition means the operator sees either nothing or a raw exception string. +→ **D5** + quick wins QW4, QW6. + +### T5. Scale strain on the two coordination hubs +`App.tsx` (2,076 lines, 24 under its ceiling, hosting multi-hundred-line business +flows and two stream parsers) and `useDiagrams` (1,225 lines, six domains) are the +only two places where the otherwise-excellent decomposition discipline stopped. +State management is 100 % props (zero React context): an 88-prop +`VisualizationPanel`, a 44-prop `ActionFeed`, a 44-setter session-restore bag. +The repo's own deferred "Option 3" plan (`docs/architecture/app-refactoring-plan.md`) +already scopes the fix. → **D4**. + +### T6. The docs strategy outgrew its maintenance model +The doc *corpus* is a genuine strength (30+ indexed docs, accurate endpoint table, +traced quick start). The failure is concentrated in one genre: hand-maintained +annotated inventories (file trees, line counts, line-number anchors) — 20+ +verified mismatches, ghost files (`ActionTypeFilterChips.tsx` documented in two +trees, absent on disk), and an entire subsystem missing. Inventories should be +generated or machine-checked; prose should carry the judgment. → **D9** + QW12. + +### T7. The benchmark/data supply chain is unreproducible +Three independent gaps compose badly: `build_pipeline.py` cannot reproduce the +committed grid bundles (hardcoded forbidden 8,000-unit layout scale; two required +post-processing steps not in the pipeline); no provenance manifest links any +bundle to the code that made it; the Codabench scorer that `scoring.ts` must stay +"numerically identical" to lives outside the repo, and the exported session log is +neither replayable nor verifiable — the public ranking scores self-reported +numbers. → **D8**. + +--- + +## Part IV — Findings by dimension + +Verification legend: ✅ confirmed · 🟡 partially confirmed (statement corrected) · +▫ low severity, not adversarially verified. + +### 1. Backend architecture + +*A well-engineered single-operator analysis server whose central bet — module-level +singletons, mutable state in three layers, zero synchronization — is documented as +unsafe yet unguarded, and whose worst structural decision (import-time +monkey-patching) has already shipped a bug.* + +**Strengths** +- Rigorous pypowsybl **variant lifecycle discipline**: every switch paired with a + finally-restore; contingency variants clone from a clean N baseline; analysis + entry points drain the NAD-prefetch thread first + (`recommender_service.py:637-656`, `:802-878`). +- **Measured, layered caching** with documented invalidation rules and a + drain-first `reset()` ordering that anticipates a subtle poisoning bug + (`recommender_service.py:59-172`). +- The PR #104/#106 **helper extraction is real dependency injection** — stateless + helpers taking collaborators as arguments, call-time symbol resolution keeping + legacy `@patch` seams alive. +- A **textbook plugin registry** (`recommenders/registry.py`): decorator + registration, per-model degradation, server-side capability enforcement. +- **Error-type discrimination where it matters**: `ActionResultUnavailableError` + separates the expected post-reload condition from faults while preserving the + frontend's fallback contract. + +**Weaknesses** + +| Sev | Finding | Verdict | +|---|---|---| +| high | Production behavior of `run_analysis_step2` defined by import-time monkey-patching (`_service_integration.py:46-88, :323`), with a live ~190-line near-duplicate on `AnalysisMixin` — already caused the shipped cache-drift bug fixed in `401045f` | ✅ | +| high | Zero synchronization on globally-shared mutable state despite real request concurrency (threadpool + `Promise.all` + Space) | ✅ | +| high | Root `tests/` package (8 files) covering exactly this integration runs in no pytest config and no CI | ✅ | +| med | `reset()` misses `_n_state_currents` and `_last_action_path` | 🟡 — real, but `_last_action_path` is functionally harmless (the reload check also tests `_dict_action is None`); `_n_state_currents` staleness window is narrow. No test guards either. | +| med | `Overflow_Graph` dir resolved three different ways (module path, CWD, relative) | ✅ | +| med | All failures collapse to HTTP 400; broad exception swallowing; three error contracts | ✅ | +| med | `/api/config` route embeds domain logic, private-attribute reaches, a vestigial global | ✅ | +| med | `update_config` is a ~190-line god-method that silently rewrites the user's action file on disk | ✅ | +| med | Backend CLAUDE.md drifted (composition model wrong, line anchors hundreds off) | ✅ | +| low | Vestigial vertical slice: legacy `/api/run-analysis`, root `inspect_action.py`, hand-maintained type shim | ▫ | +| low | `overflow_overlay.py` embeds ~916 lines of CSS/JS in an f-string, tested via a hand-mirrored Python re-implementation | ▫ | + +### 2. Frontend architecture + +*Disciplined, test-heavy React 19 with genuinely strong leaf layers; the +coordination layer is under real strain.* + +**Strengths** +- **Leaf-layer decomposition**: `utils/svg/` is pure, per-module tested, no React + or axios imports — the imperative SVG work is quarantined. +- **Game Mode isolation verifiably holds**: three guarded touch points via the + bridge singleton, exactly as documented. +- **Design-token discipline machine-enforced** — which is what made dark mode a + token swap. +- Dense co-located tests; App-level integration tests split by domain. +- The **hybrid React/imperative SVG model is deliberate and correctly layered** + (React owns lifecycle; refs own the hot path). + +**Weaknesses** + +| Sev | Finding | Verdict | +|---|---|---| +| high | `App.tsx` (2,076 lines; 24 under its CI ceiling introduced under a "lower over time, don't raise" comment) hosts multi-hundred-line business flows incl. two stream parsers | ✅ | +| high | Props-only state at a scale where it stopped working: 88-prop `VisualizationPanel`, 44-prop `ActionFeed`, 44-setter restore bag, zero context | ✅ | +| med | Hook coupling via whole-state-object injection & sibling-setter threading; load-bearing hook instantiation order | 🟡 — worse than claimed in one respect (`DiagramsState` has 57 members, not 40) but hooks *are* individually testable | +| med | `useDiagrams`: 1,225 lines spanning six domains | ✅ | +| med | NDJSON parser hand-copied with divergent fixes | 🟡 — **five** copies, not four (missed `useDiagrams.ts:634-652`); only the two App.tsx copies flush the final unterminated event; failure currently latent because the backend happens to newline-terminate | +| med | CLAUDE.md contradicts the code (App size, "no API calls in components" vs. `ActionFeed`'s six call sites, ghost `ActionTypeFilterChips.tsx`) | 🟡 — 3 of 4 cited contradictions real | +| low | Fragile implicit contracts: comment-by-line-number, no console.log ceiling | ▫ | + +### 3. API & interface contract + +*A pragmatic RPC-over-HTTP surface of 37 routes, unusually well documented and +test-covered, with genuinely good large-payload engineering — undermined by the +absence of any machine-checked response contract.* + +**Strengths** +- **Endpoint table diff-matches `main.py` exactly (37/37)** — the single most + load-bearing reference is accurate. +- **Three-tier payload strategy** for 12–28 MB SVGs: gzip negotiation, bespoke + header+text framing, SVG-less patch endpoints with an explicit + `patchable:false` degrade path. +- Request side is **consistently Pydantic** (typed 422s). +- One simple, ordering-aware **NDJSON event grammar** across the three streams. +- **Real contract tests on both sides** (82 backend endpoint tests, 22 frontend). + +**Weaknesses** + +| Sev | Finding | Verdict | +|---|---|---| +| high | Error contract collapses to 400 and fragments into five client-visible shapes; backend relies on 400-as-please-resimulate | 🟡 — ~26 blanket handlers (not 32); one endpoint does split 400/500; core confirmed | +| high | No `response_model` anywhere; `types.ts` is an unenforced hand mirror; stream events landed via `as unknown as` casts | ✅ | +| med | Five duplicated NDJSON parsers (see above) | 🟡 | +| med | Desktop-era filesystem RPCs unauthenticated on the public Space | 🟡 — real, but the 300 s tkinter hang doesn't occur headless (tkinter absent, fails in ms); blast radius is one ephemeral single-player container | +| med | Dead surface: 4 of ~34 endpoints have zero callers | 🟡 — the focused-diagram pair backs a documented proposal and its internals are exercised by the live patch pipeline; truly dead ≈ `/api/run-analysis` + `element-voltage-levels` routes | +| med | HTTP layer reaches into service privates; domain logic in routes | ✅ | +| low | Wire types mixed with DOM state; unvalidated string enums | ▫ | +| low | `/api/models` contract tests never collected (root `tests/`) | ▫ | + +### 4. Interaction & UX design + +*Unusually deliberate for an operator tool — but error surfacing has a real hole, +and five click grammars coexist.* + +**Strengths** +- **Every destructive transition routes through one typed `ConfirmationDialog`** + (six loss-of-work gestures). +- **Two-step analysis with staged reveal** preserves operator investment across + re-runs; the overflow graph appears as soon as its stream event lands. +- **Notices tier system** deliberately fixes warning fatigue. +- **Detached-tab model survives the detach/reattach round-trip** (viewBox, refs, + listeners), with triple-redundant window pruning. +- **Transactional SLD edit idiom** (stage → preview → simulate → card) with + per-row and bulk revert. +- **Replay-ready interaction logging** woven through every gesture. + +**Weaknesses** + +| Sev | Finding | Verdict | +|---|---|---| +| high | Analysis failures invisible: `useAnalysis`'s `error` state set on three failure paths, rendered nowhere; `App.tsx` destructures a *different* local `error` | ✅ | +| med | Re-simulation failures swallowed (`console.error` only) | ✅ | +| med | Toast design: error banner has no dismiss & no timeout; info fixed 3 s; no `aria-live`; magic `'SUCCESS'` string protocol | ✅ | +| med | Five single-vs-double-click grammars across surfaces; one documented backwards | 🟡 — grammars enumerated & confirmed; "backwards" doc claim narrowed | +| med | No cancellation/abort for any long-running operation | ✅ | +| med | Dark theme doesn't reach detached popups; hardcoded `'white'` literals | ✅ | +| med | Keyboard/AT access near-absent; modals lack Escape & focus management | ✅ | +| med | Divergent NDJSON consumers with different error semantics (cross-ref T2) | ✅ | +| low | Debug console noise in interaction hot paths; `'+'`-joined contingency ids | ▫ | + +### 5. Backend performance + +*A mature, measurement-driven culture (committed benchmarks, perf-history docs +that match code, vectorized hot paths citing before/after timings). The three big +wall-clock costs — study load ~8.5 s, step-2 stream, cold N-1 view 18.1 s → 4.2 s — +are each substantially mitigated. Residual defects are specific and fixable.* + +**Strengths** +- Correct **sync-def threading model** for pypowsybl endpoints + disciplined + ≥10 KB gzip. +- **Caching mapped to profiled costs** with explicit reset discipline. +- **Committed benchmark suite with recorded numbers**; README tells contributors + when to re-run what. +- **Vectorized per-branch delta/overload pipelines**; narrow-attribute pypowsybl + queries (144 ms → 6.6 ms documented). +- **SVG-less patch endpoints** remove the dominant diagram cost from tab + switches. + +**Weaknesses** + +| Sev | Finding | Verdict | +|---|---|---| +| high | `/api/run-analysis-step1` is `async def` running seconds of sync pypowsybl/grid2op — blocks the entire event loop (all endpoints freeze) | ✅ — no `await`/`to_thread` anywhere in the backend | +| med | Shared Network variant-switched from threadpool workers, no lock; `diagram_mixin.py:151-160` lacks try/finally | 🟡 — accepted-and-documented single-user constraint, but invalidated by Space concurrency; one path genuinely unguarded | +| med | Pandas row-iteration reintroduced in newer helpers — worst case an 85,304-iteration `.loc` loop per action-SLD view (`_diff_switches`) | ✅ | +| med | Streaming endpoints ship the full 20–28 MB action NAD JSON-escaped, uncompressed | ✅ | +| med | Obs prewarm (`env.get_obs`) runs synchronously inside every contingency-diagram request | ✅ | +| low | Legacy analysis stream busy-polls with no worker timeout; per-study state only grows within a session; N-state reference flows recomputed per patch request; duplicated step-2 generator (T1) | ▫ | + +### 6. Frontend performance + +*Deep, honest performance engineering (svgPatch recycling measured at +3.01 s → 0.49 s; pan/zoom fully bypasses React; detached tabs move DOM instead of +copying). The remaining costs are concentrated in the full-NAD ingestion path and +render hygiene at the App.tsx level.* + +**Strengths** +- **svgPatch DOM recycling is load-bearing and guarded** (server-shipped + VL-subtree fragments with svgId rewriting). +- **Zero React renders during pan/zoom gestures**: direct viewBox writes, rAF + batching, cached CTM. +- **Bitmap snapshot mode** is sophisticated (taint handling, stylesheet + isolation) and honestly benchmarked. +- **O(1) lookup layers** (Map indices) replace repeated DOM scans. +- Hidden-tab **layout-tree cost was profiled and fixed with DevTools evidence**. + +**Weaknesses** + +| Sev | Finding | Verdict | +|---|---|---| +| high | Full-NAD loads pay an avoidable serialize + re-parse round-trip (`svgBoost.ts:621` serializes; every string path re-parses via innerHTML); `MemoizedSvgContainer`'s "zero extra parse" comment is false on all string paths | 🟡 — avoidable slice is several hundred ms per load, not the full 1.4 s | +| med | Whole-object dependency churn re-runs the entire highlight pipeline after every pan/zoom settle | ✅ | +| med | Default pan/zoom re-rasters the full vector layer per frame (~20 fps on the 5,247-VL grid) while the benchmarked 120 fps bitmap mode ships opt-in | 🟡 — opt-in is the *documented* shipped design pending real-hardware validation; a VL-count auto-enable is a reasonable follow-up, severity low-med | +| med | Memory retention: 3 always-mounted 200k-node DOMs + unbounded per-action processed-SVG string cache + overview clone | 🟡 — bounded in practice (~100–300 MB worst case within one contingency; cache clears on contingency change); LRU cap still warranted | +| med | App.tsx re-render blast radius defeats `React.memo` on the big children (unstable callback identities) | ✅ | +| low | Dead `react-zoom-pan-pinch` dependency; stale perf docs; unminified standalone rationale predates the legacy file's retirement; 25 `console.log` in hot paths | ▫ | + +### 7. Documentation + +*Extensive and mostly high quality (accurate 37/37 endpoint table, traced quick +start, layered docs/ index, 77 % docstring coverage, invariant-level comments in +the gnarliest modules, disciplined 83 KB CHANGELOG). The failure mode is one +genre: hand-maintained inventories.* + +**Weaknesses** + +| Sev | Finding | Verdict | +|---|---|---| +| high | Recommenders subsystem invisible/misdescribed in both CLAUDE.md trees; root `tests/` outside the documented test entry point | 🟡 — full map *does* exist at `docs/backend/recommender_models.md` and README's "Plug Your Own Model"; the *canonical onboarding layer* is what's wrong | +| high | Ghost files and materially wrong sizes in both CLAUDE.md trees — inventory layer no longer trustworthy | ✅ | +| med | `.env.example` documents vars the code never reads (`PYPOWSYBL_FAST_MODE`, `LOG_LEVEL`); nothing loads a `.env`; the three actually-read vars are missing | 🟡 — the real vars are documented elsewhere | +| med | `PARITY_AUDIT.md` frozen at 2026-04-20 while root CLAUDE.md calls it a living record | 🟡 — parity truth lives in CI; fix is a one-line reframe | +| med | Hard-coded line-number anchors systematically stale (5 of 6 sampled wrong) | ✅ | +| med | Root CLAUDE.md describes the pre-0.8.0 dependency world (requirements.txt vs. the pyproject reality) | 🟡 | + +### 8. Maintainability, testing & delivery + +*Testing discipline is exceptional for the team size: 892 backend test functions, +frontend test lines exceed source lines (32,776 vs 27,790, zero snapshots), a +prefer-real-fallback-to-mock conftest, coverage + mypy-at-zero gates, a 4-layer +parity harness with honest blind-spot docs. Delivery is where the debts sit.* + +**Weaknesses** + +| Sev | Finding | Verdict | +|---|---|---| +| high | Orphaned root `tests/` (1,708 lines, 144 functions) — and two docs present it as the canonical suite (`pytest tests/`); a running test explicitly defers coverage to an orphaned file | 🟡 — one filename collision with the running suite, not three | +| med | No machine-checked backend↔`types.ts` contract | ✅ | +| med | CI resolves the newest unpinned upstream recommender on every run — third-party releases can break unrelated PRs | ✅ | +| med | Full CI duplication GH Actions + CircleCI; CircleCI a strictly weaker drifted copy | 🟡 — documented belt-and-braces choice; real cost is doubled maintenance + one-way drift | +| med | Dependency metadata across four inconsistent files (pyproject the real source; dead stub requirements.txt; `requirements_py310.txt` pins *below* the CI floor; overrides.txt floors misdescribed as pins); no lockfile | 🟡 — README/CONTRIBUTING carry correct commands | +| med | Onboarding-doc drift incl. phantom `fileRegistry` utility | 🟡 | +| med | Mixin-level tests over-rely on patch chains (226 `patch(` / 249 `patch.object`) & private-attribute injection — each further decomposition round gets more expensive | 🟡 — real-stack numerical tests partially mitigate | +| low | Committed litter (`inspect_action.py`, tracked generated outputs); gate ceilings at near-zero headroom (by design); bus factor of one | ▫ | + +### 9. Robustness & security + +*The classic mistakes are absent — traversal, zip-slip, XSS, shell injection all +correctly handled. The problem is a trust model ("the client is the operator on +their own machine") violated in both real deployment modes.* + +**Strengths**: `/results/pdf` confined via `resolve()+relative_to()`; zip-slip-safe +extraction (`basename`); argv-only subprocess; `allow_credentials` correctly +disabled under the wildcard; injected overlay renders untrusted data via +`textContent`/`createElementNS`/`CSS.escape` only. + +**Weaknesses** + +| Sev | Finding | Verdict | +|---|---|---| +| high | CORS `*` + no auth + arbitrary-file endpoints = drive-by local file read/write from any web page against a localhost install | ✅ | +| high | Unauthenticated arbitrary filesystem access survives on the public Space (Game Mode gates only the UI); `../` traversal in `session_name` | ✅ | +| med | Singleton state shared across concurrent users, no locking (cross-ref T3) | ✅ | +| med | `detail=str(e)` leaks absolute server paths | ✅ | +| med | `/api/pick-path` spawns a subprocess per request | 🟡 — headless child fails in ms (tkinter absent); low-severity dead weight, not a DoS vector | +| low | `load-session` imports arbitrary HTML into the same-origin-served dir; `update_config` writes back to the caller-supplied action path | ▫ | + +### 10. *(gap)* PyPSA-EUR data pipeline & grid provenance — *not adversarially verified* + +Well-documented five-stage orchestration with an architecturally thoughtful +158-test suite — but: **`build_pipeline.py` cannot reproduce the committed +bundles** (hardcodes the forbidden 8,000-unit layout scale at +`convert_pypsa_to_xiidm.py:876`; omits the two layout post-processing scripts that +produced the committed layouts — a rebuild silently clobbers good layouts); +**the 158 tests run in no CI** and can't pass from a fresh clone; **no provenance +record** links any bundle to the pipeline version that made it (3 commits total +touch the pipeline; 22 MB of outputs arrived wholesale); three overlapping +calibration implementations plus a dead builder (~700 removable lines); six +scripts execute argparse/IO at import; Game Mode presets hardcode contingency ids +with no consistency test against the artifacts they reference. + +### 11. *(gap)* Game Mode & Codabench — *not adversarially verified* + +Internals better than a 1,690-LoC bolt-on has a right to be (verified isolation; +race bugs fixed with sophisticated CI-run regression tests; pure edge-guarded +scoring). But: **the declared dual-implementation scoring contract has no in-repo +Python twin** — `scoring.test.ts` claims to mirror a `score.py` living at a +hardcoded `~/Dev/codabench/...` path, no parity fixture, no CI guard, and the e2e +script carries a third partially-divergent copy; **the exported session log is +neither replayable nor verifiable**, so the public ranking scores self-reported +numbers; a mid-session backend failure destroys all completed results (the +promised retry doesn't exist); multi-action scoring is physically misleading +(actions are never combined — optimal play is always exactly one star); the e2e +harness covers 1 of 8 FR studies and zero of the shipped default EUR tier. + +### 12. *(gap)* Deployment & release engineering — *not adversarially verified* + +Mechanics are strong (inert same-origin SPA mount; triple-guarded pinGlyph +coupling; idempotent first-boot seeding; safe-by-default deploy automation). The +fundamentals are missing: **no reproducible Python closure anywhere** (all floors, +recommender floats at build time with `--no-deps`, floating base images, the only +lock-like file is stale and consumed by nothing — a zero-change HF factory rebuild +can produce a different broken image); the Dockerfile's "mirrors CI" claim is +false in the part that matters; **continuous ungated deploy + zero tags + +force-push = no rollback primitive**; the LFS zip-extraction step has two silent +failure modes; the image ships dead weight (jupyter-widget stack, `scripts/`, +tests); `PORT` is decorative; no `HEALTHCHECK`; config upgrades never merge. + +--- + +## Part V — Deep revisions (sequenced roadmap) + +Ordered by leverage; each unblocks or de-risks the ones after it. + +**D1. De-ghost the recommender subsystem** *(2–3 days — do first)* +Replace import-time monkey-patching with explicit composition: +`RecommenderService` inherits `ModelSelectionMixin` directly (it is already +written as a mixin); `update_config`/`reset` call `_apply_model_settings`/ +`_reset_model_settings` explicitly; **delete** the shadowed ~190-line legacy +generator in `analysis_mixin.py:565-756`. Rescue the root `tests/` into +`expert_backend/tests/` (resolving the one filename collision) so CI covers the +integration. Update both CLAUDE.md trees. This closes T1 entirely and removes the +mirror that already shipped a bug. + +**D2. Machine-check the API contract** *(4–6 days)* +(a) Pydantic response models on the ~12 highest-traffic endpoints (preserving +`sanitize_for_json` coercion); (b) commit an `app.openapi()` dump + CI diff so +response-shape changes become reviewable; (c) generate the TS types from OpenAPI +and retire the hand-mirror incrementally; (d) unify the error contract — one +middleware mapping domain errors → 400/404/409 with `{detail, code}`, everything +else → 500 + `logger.exception`, deleting the ~26 blanket handlers; one frontend +error extractor. Note the frontend's documented 400-triggers-resimulate dependency +(`main.py:706-715`) must be preserved via an explicit error code. + +**D3. Concurrency ownership for the shared Network** *(3–5 days)* +One service-level `RLock` (decorator on the ~12 variant-switching entry points), +a 409 "busy" contract for overlapping study mutations, try/finally on the +unguarded path (`diagram_mixin.py:151-160`), then LRU lifecycle for variants + +cached observations (keep N + last K, `remove_variant` on eviction). Watch +lock-ordering against the NAD-prefetch drain. + +**D4. Relieve the two frontend hubs** *(6–10 days, stageable)* +Execute the already-scoped "Option 3" extraction: move +`handleSimulateUnsimulatedAction` / `handleSimulateSldEdit` into a +`useManualSimulation` hook; split `useDiagrams` into per-domain hooks behind the +existing `DiagramsState` facade; replace exploded props with cohesive state-object +props on `VisualizationPanel`/`ActionFeed` (the `SettingsModal` pattern the repo +already uses). Then *lower* `APP_TSX_MAX` to lock in the win. + +**D5. One streaming + notification pipeline** *(3–4 days)* +`utils/ndjsonStream.ts` (buffer carry-over, trailing flush, uniform error +semantics, `AbortController`) replacing all five copies; a visible Cancel on +long operations; a typed notification store (severity, sticky, dismiss, +`aria-live`) replacing the dual error states and the `'SUCCESS'` string protocol. +Fixes T4 at the root; QW4 below is the 2-hour stopgap. + +**D6. Finish the SVG element-adoption pipeline** *(2–4 days)* +Make `processSvg` return the already-parsed `SVGSVGElement` (adoptNode) instead of +re-serializing; route all ingestion paths through the element path that already +exists for patches; then a VL-count heuristic to auto-select bitmap pan/zoom mode +(or a discoverability nudge) once validated on operator hardware. + +**D7. Deployment trust & reproducibility** *(3–4 days)* +Lockdown profile (env flag set in the Dockerfile) disabling/confining the +filesystem RPCs on non-local deployments; pin the Python closure (pip-compile +lockfile consumed by both CI and Docker so "mirrors CI" becomes true); version +tags + test-gated deploy + documented Space rollback. + +**D8. Reproducible data & benchmark supply chain** *(3–5 days)* +Fix the layout scale in `build_pipeline.py` and absorb the two missing +post-processing steps; write a provenance manifest into every bundle; wire the +hermetic slice of the pipeline suite into CI; bring the Codabench `score.py` +in-repo with a shared golden fixture locking cross-language parity; make the +session log replayable (the e2e harness already contains the replay machinery). + +**D9. Docs as checked artifact** *(1–2 days)* +`scripts/check_docs_tree.py` in the existing gate (file-exists / absent-from-tree +/ line-count claims), warn-only first week; replace line-number anchors with +symbol anchors; slim the CLAUDE.md trees to what the checker can verify. + +## Part VI — Quick wins + +Day-one (each ≤ ~2 h, near-zero risk): + +| # | Fix | Where | +|---|---|---| +| QW1 | Collect root `tests/` (add to `pytest.ini` testpaths or move files) — *the highest value-per-hour change in this review* | pytest.ini / CI | +| QW2 | `async def` → `def` on `run_analysis_step1` (one keyword, unblocks the event loop) | `main.py:635` | +| QW3 | CORS default `*` → loopback origins; wildcard becomes explicit opt-in | `main.py:141` | +| QW4 | Render `useAnalysis.error` in `StatusToasts`; add `setError` to the two swallowed catches | `App.tsx` | +| QW5 | try/finally on the unguarded variant switch | `diagram_mixin.py:151-160` | +| QW6 | Replace `detail=str(e)` with generic messages + server-side `logger.exception` | `main.py` | +| QW7 | Reject path separators/`..` in `session_name`; resolve+relative_to on session dirs (mirror the existing `/results/pdf` guard) | `main.py` | +| QW8 | Pin `expert_op4grid_recommender` per-PR; float it in a separate canary job | CI ×3 + Dockerfile | +| QW9 | Delete `inspect_action.py`, dead `requirements.txt` stub, `react-zoom-pan-pinch`; untrack generated outputs | repo root / frontend | + +Week-one (½–1 day each): + +| # | Fix | Where | +|---|---|---| +| QW10 | Extract `utils/ndjsonStream.ts`, delete the five copies (subset of D5, standalone-shippable) | frontend | +| QW11 | Vectorize the reintroduced pandas loops (85 k-iteration `_diff_switches` first) | `diagram_mixin.py:876-892` | +| QW12 | Memoize the per-study N-state flow/asset snapshot for patch + SLD delta endpoints | `diagram_mixin.py` | +| QW13 | Ship the patch payload in `simulate-and-variant-diagram` when patchable (machinery exists in `action_patch.py`) — removes the 20–28 MB uncompressed stream event | backend + fallback wiring | +| QW14 | Fix highlight-pipeline deps so it stops re-running on every pan/zoom settle | `useDiagramHighlights.ts` | +| QW15 | LRU-cap `actionDiagramCacheRef` (2–3 entries); stop duplicate string retention | `useDiagrams.ts` | +| QW16 | Batch-fix the 20+ verified doc mismatches; truthful `.env.example`; symbol anchors; reframe PARITY_AUDIT as closed | docs | +| QW17 | Single `Overflow_Graph` path constant; automate the `reset()` completeness invariant (fresh-instance `__dict__` comparison test) + fix the two leaks | backend | +| QW18 | Single-flight lock or 409 on `update_config`/analysis entry points (coarse version of D3) | backend | +| QW19 | Theme propagation into detached popups; purge `'white'` literals | `useDetachedTabs` | +| QW20 | `useModalKeyboard` hook: Escape, initial focus, focus restore, `aria-modal` | modals | +| QW21 | Frontend `console.log` ceiling in the gate (start at 25, ratchet down) | `check_code_quality.py` | +| QW22 | Watchdog deadline on the legacy analysis poll loop; or delete the legacy `/api/run-analysis` slice outright | `analysis_runner.py` | +| QW23 | Collapse to one CI system (keep GH Actions; reduce CircleCI to an explicit mirror or delete) | `.circleci/` | +| QW24 | Game Mode: retry + finish-with-partial-results on mid-session failure; preset↔artifact consistency test | `game/`, tests | +| QW25 | Dockerfile: real `PORT`, `HEALTHCHECK`, drop jupyter/`uv`/`scripts/` dead weight; loud LFS-pointer validation | Dockerfile | + +## Part VII — Limits of this review + +- **Static analysis only.** No live backend was run; performance findings rest on + the repo's own committed benchmarks/docs plus code reading, not fresh profiling. +- **Gap reviews (Parts IV.10–12) were not adversarially verified** — treat their + specifics as one-reviewer claims, though they follow the same evidence rules. +- **Low-severity findings were not verified** (marked ▫). +- **Not covered**: git-history churn analysis, frontend bundle-size deep dive + beyond the noted 704 KB chunk, the upstream `expert_op4grid_recommender` + library itself, accessibility beyond code-level ARIA/keyboard checks, and any + UX evaluation with real operators — the interaction findings are code-derived. +- Line numbers reference the reviewed commit (`c3f9c00`, 2026-07); they will + drift — which is, fittingly, one of this review's own findings about the docs. From 2dfeb1ae94ec4b8fbb5793589375d6b864ca5da1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 17:47:50 +0000 Subject: [PATCH 4/8] ci: run PR workflows for any base branch, not just main PR #3 targets the `claude/sld-voltage-navigation-mypy-1zm3om` working branch, but the Code Quality / Tests / Parity workflows only fired on `pull_request` events whose base was `main`, so no CI ran for it. Broaden each `pull_request` trigger to `branches: ['**']` so CI runs on PRs targeting any branch. `push` stays restricted to main, and the HuggingFace deploy workflow is left untouched. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014aLzZJp3MP3qYBr1WKGRC6 --- .github/workflows/code-quality.yml | 4 +++- .github/workflows/parity.yml | 4 +++- .github/workflows/test.yml | 4 +++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index 0699d20..6673874 100644 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -3,8 +3,10 @@ name: Code Quality on: push: branches: [main] + # Run on every PR regardless of its base branch (e.g. PRs stacked onto a + # `claude/*` working branch, not just those targeting main). pull_request: - branches: [main] + branches: ['**'] jobs: gate: diff --git a/.github/workflows/parity.yml b/.github/workflows/parity.yml index c6c17cd..9180b81 100644 --- a/.github/workflows/parity.yml +++ b/.github/workflows/parity.yml @@ -26,8 +26,10 @@ name: Parity on: push: branches: [main] + # Run on every PR regardless of its base branch (e.g. PRs stacked onto a + # `claude/*` working branch, not just those targeting main). pull_request: - branches: [main] + branches: ['**'] schedule: # Nightly at 02:30 UTC — catches drift introduced between PRs # (e.g. docs/features/interaction-logging.md updates without a code change). diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 88b923e..f19746f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -3,8 +3,10 @@ name: Tests on: push: branches: [ main ] + # Run on every PR regardless of its base branch (e.g. PRs stacked onto a + # `claude/*` working branch, not just those targeting main). pull_request: - branches: [ main ] + branches: [ '**' ] jobs: # ------------------------------------------------------------------ From 8375e3c2c0ba31f859aad8ab857e4f16d4a9877f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 19:20:10 +0000 Subject: [PATCH 5/8] ci: trigger workflows on PR #3 after enabling Actions Empty commit to fire a pull_request synchronize event now that repo-level GitHub Actions is enabled, so Code Quality / Tests / Parity run on PR #3. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014aLzZJp3MP3qYBr1WKGRC6 From 2cb209da23cb7e3a62bde009de6654ef69aaf0eb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 19:23:19 +0000 Subject: [PATCH 6/8] ci: also run workflows on push to claude/** working branches A pull_request whose base is not `main` is matched against the base branch's own trigger (still main-only), so broadening the head branch's pull_request filter alone did not fire CI for PR #3. Add the `claude/**` working branches to the push trigger so pushes to this branch get commit-level checks that surface on the PR. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014aLzZJp3MP3qYBr1WKGRC6 --- .github/workflows/code-quality.yml | 6 +++++- .github/workflows/parity.yml | 6 +++++- .github/workflows/test.yml | 6 +++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index 6673874..76dbf9e 100644 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -1,8 +1,12 @@ name: Code Quality on: + # Also run on pushes to the `claude/**` working branches so a PR stacked + # onto another working branch still gets commit-level checks (a `pull_request` + # whose base is not `main` is matched against the base branch's own trigger, + # which stays `main`-only). push: - branches: [main] + branches: [main, 'claude/**'] # Run on every PR regardless of its base branch (e.g. PRs stacked onto a # `claude/*` working branch, not just those targeting main). pull_request: diff --git a/.github/workflows/parity.yml b/.github/workflows/parity.yml index 9180b81..f52c7e0 100644 --- a/.github/workflows/parity.yml +++ b/.github/workflows/parity.yml @@ -24,8 +24,12 @@ name: Parity on: + # Also run on pushes to the `claude/**` working branches so a PR stacked + # onto another working branch still gets commit-level checks (a `pull_request` + # whose base is not `main` is matched against the base branch's own trigger, + # which stays `main`-only). push: - branches: [main] + branches: [main, 'claude/**'] # Run on every PR regardless of its base branch (e.g. PRs stacked onto a # `claude/*` working branch, not just those targeting main). pull_request: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f19746f..3954066 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,8 +1,12 @@ name: Tests on: + # Also run on pushes to the `claude/**` working branches so a PR stacked + # onto another working branch still gets commit-level checks (a `pull_request` + # whose base is not `main` is matched against the base branch's own trigger, + # which stays `main`-only). push: - branches: [ main ] + branches: [ main, 'claude/**' ] # Run on every PR regardless of its base branch (e.g. PRs stacked onto a # `claude/*` working branch, not just those targeting main). pull_request: From e41449566a06a8c9cbff7e81b625c5f7978b2226 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 20:35:55 +0000 Subject: [PATCH 7/8] ci: re-trigger workflows on PR #3 after enabling Actions Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014aLzZJp3MP3qYBr1WKGRC6 From 6993c34408a2544beab006a4a6e8a177d9fba4ea Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 20:39:01 +0000 Subject: [PATCH 8/8] ci: drop redundant push trigger now that pull_request fires The pull_request `branches: ['**']` change (read from the PR head branch's workflow) is confirmed to trigger CI for PR #3, so the `claude/**` push trigger is no longer needed and only produced duplicate runs. Restore push to `main`-only; keep the broadened pull_request filter. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014aLzZJp3MP3qYBr1WKGRC6 --- .github/workflows/code-quality.yml | 6 +----- .github/workflows/parity.yml | 6 +----- .github/workflows/test.yml | 6 +----- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index 76dbf9e..6673874 100644 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -1,12 +1,8 @@ name: Code Quality on: - # Also run on pushes to the `claude/**` working branches so a PR stacked - # onto another working branch still gets commit-level checks (a `pull_request` - # whose base is not `main` is matched against the base branch's own trigger, - # which stays `main`-only). push: - branches: [main, 'claude/**'] + branches: [main] # Run on every PR regardless of its base branch (e.g. PRs stacked onto a # `claude/*` working branch, not just those targeting main). pull_request: diff --git a/.github/workflows/parity.yml b/.github/workflows/parity.yml index f52c7e0..9180b81 100644 --- a/.github/workflows/parity.yml +++ b/.github/workflows/parity.yml @@ -24,12 +24,8 @@ name: Parity on: - # Also run on pushes to the `claude/**` working branches so a PR stacked - # onto another working branch still gets commit-level checks (a `pull_request` - # whose base is not `main` is matched against the base branch's own trigger, - # which stays `main`-only). push: - branches: [main, 'claude/**'] + branches: [main] # Run on every PR regardless of its base branch (e.g. PRs stacked onto a # `claude/*` working branch, not just those targeting main). pull_request: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3954066..f19746f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,12 +1,8 @@ name: Tests on: - # Also run on pushes to the `claude/**` working branches so a PR stacked - # onto another working branch still gets commit-level checks (a `pull_request` - # whose base is not `main` is matched against the base branch's own trigger, - # which stays `main`-only). push: - branches: [ main, 'claude/**' ] + branches: [ main ] # Run on every PR regardless of its base branch (e.g. PRs stacked onto a # `claude/*` working branch, not just those targeting main). pull_request: