Split configuration/data store object - #141
Open
olivhoenen wants to merge 15 commits into
Open
olivhoenen wants to merge 15 commits into
olivhoenen wants to merge 15 commits into
Conversation
`getStateHandler` handed Electron's structured clone - and then the WebDriver JSON bridge - the whole store on every `getTestState()` poll. The store owns the fetched matrices, so on a production entry that is the psi matrix, per poll, in a `waitForValue` loop. BASELINE.md recorded the consequence and told the next author to drive the UI from the DOM instead; measured again here, on imas:hdf5?path=/work/imas/shared/imasdb/ITER/3/134173/106 with coordinate shapes [871,1,65] [871,1,129] [871,1] [871], WebDriver gives up with ScriptTimeoutError before the renderer finishes serializing. With the projection the same poll costs 52 ms once and 5-6 ms after that, for a 17 kB snapshot. utils/testState.ts drops four families on the way out - plot[].yData, plot[].error_bands[].yData, coordinates[].data and geometries[].x/.y - and nothing else. It is a denylist rather than an allowlist on purpose: a spec reading a field nobody thought to enumerate keeps working, and only those four can go missing. The derived vectors still cross, because they are a single row whatever the entry's size and because plot-ui.spec.ts asserts exact floats on plot[0].y. The inverse is not optional and ships in the same commit. Three sites (dataManipulation.ts, reactivity.perf.spec.ts twice) read the whole state and write it straight back to flip one field, and `setState` replaces configurations/active wholesale - so a projection without `mergeTestState` would have the first such round trip silently empty every plot in the store, and the spec that failed would not be the one that caused it. Grids carry a `__payloadsOmitted` marker so the merge knows which ones to rehydrate; the fixtures in tests/utils/state.ts have no marker and stay authoritative. Also: `active` and its entry in `configurations` are re-linked to one object after a merge, which is the invariant `updatedConfiguration` maintains and which `setState` could previously break; and App stops subscribing to the store. It is the root component, so that subscription re-rendered the whole tree on every write, for a bridge only the e2e suite uses - the handlers read at call time instead. No renderer behaviour changes and the benchmark counts are identical to stage 5 across all five scenarios. Full e2e suite green, 20 passing. Assisted-by: Claude/opus-5
`isEditing` was a boolean on every grid that only one grid could ever hold:
handleEditGrid cleared it on all the others on its way through, and stage 4 had
to add explicit identity preservation so that clearing it did not re-render
every panel. Expressing the singleton as a singleton - `editingGridId` in a new
`ui` slice - makes entering or leaving edit mode write one string and touch no
grid object at all. The panels read it through a boolean selector, so only the
two whose flag actually flips re-render, and the plot components take it as a
prop instead of reading it off the grid.
`static` went with it, and that one is the interesting half. It was a grid field
always equal to `isEditing`, and it is what made the stage-4 write cycle
possible: setting it changed the layout react-grid-layout derives from its
children, so RGL reported onLayoutChange, and handleUpdateLayout wrote the
report back. It is now derived where the `data-grid` prop is built and dropped
from what RGL reports, so the cycle cannot form rather than being unpicked after
the fact. Nothing was persisting it anyway - DataGridPlotToSave never had it.
Two consumers needed more than a rename:
- fetchErrorBandsInConfig located its grid with `find((d) => d.isEditing)`, so
it returned early whenever nothing was being edited. Giving each panel its own
id looks equivalent and is not: HoverButtons' effect runs on mount for every
panel with displayErrorBand defaulting to true, so every panel started
fetching error bands as it appeared. The benchmark caught it - metadata panel
revisit went 0 requests to 12 - and it now takes the editing grid's id,
whichever grid that is.
- SimplePlotly and Heatmap2D guarded their title write on the grid's flag, but
the customization and metadata panels render a detached copy of a grid whose
flag was copied with it, so a preview could write a title into the store.
`isEditing` is optional there and defaults to false, which is what the comment
in Heatmap2D ("to prevent from updating in customization") always said the
guard was for.
The e2e bridge grows an `editingGridId` field, typed as its own `TestState`
rather than the whole store, and the perf spec's setGridEditing stops rewriting
every grid to flip one flag.
All five benchmark scenarios are identical to stage 6 - the counts were already
at their floor here; what changed is what produces them. The remaining 1 redraw
on the toggle is the toggled panel itself, whose Plotly config genuinely
changes. Full e2e suite green, 20 passing.
Assisted-by: Claude/opus-5
`metadataGridLayout` and `customizedGridLayout` follow `isEditing` into the `ui` slice. Which panel is open is not part of the document - nothing saved it, and `ConfigurationToSave` never had either field - so opening or closing one now writes a single field instead of replacing the configuration and every grid reference in it. `TreeLibrariesAccordion` loses the two `useMemo(..., [JSON.stringify(x)])` stabilisations it needed, because both selectors now return plain ids. Moving them exposed an invariant the old model held by accident: while the ids lived on the configuration, clearing the configuration took them with it. As session state they outlive it, and a stale `customizing` is not harmless - TreeLibrary.handleDisableTree disables the whole node tree whenever a panel is open, so an id belonging to a configuration that no longer exists leaves the tree dead with no panel on screen to explain why. Eight e2e specs failed on exactly that, every one of them reporting that checking a node plotted nothing, and none of them pointing anywhere near the cause. So the invariant is now the store's, not the callers': `pruneUiState` drops any ui id that does not name a grid of the active configuration, and `setActive`, `removeConfiguration` and `setState` apply it. Both slices are typed against the whole store so the configuration slice can enforce it. A field added to `ui` that references a grid has to be added there too. Benchmark counts identical to stage 6 across all five scenarios. Full e2e suite green, 20 passing. Assisted-by: Claude/opus-5
First half of the data/configuration split: the bulk arrays get a name, a lifetime and somewhere to live that is not the configuration. Nothing reads through the registry yet - the refs sit alongside the arrays they name - so this commit cannot change behaviour, and the benchmark counts confirm it did not. stores/payloadRegistry.ts keys a payload by the request that produced it, in the same canonical form requestCacheKey computes, so a payload and its cached response body are one identity rather than two - which is what later lets the two caches merge. Registration happens inside fetchDataPlot and fetchFieldValue, where the request is known and the post-processing has finished, rather than at the twenty-odd call sites that store the result. requestCacheKey now canonicalises a relative endpoint to the same key as the absolute URL it becomes, so the fetch layer and the cache cannot disagree. It is deliberately a module-level Map and not a zustand slice. Nothing renders a payload directly, so one arriving must not notify a subscriber; and keeping it out of the store keeps it out of getState(), which the e2e bridge serialises, and out of every structuredClone of a configuration. Lifetime is mark-and-sweep from a store subscription, debounced onto an idle callback. Reference counting would need a matching release at all 24 configuration writers and would leak, or free too early, the first time one was missed; sweeping from what the store can reach means deleting a grid or a configuration needs no code at all. DataplotCustomization holds a detached copy of a grid the store cannot see, so pin/unpin exist for it and pins are roots. It sweeps for real rather than in audit mode, which is the opposite of the cautious choice and is the right one here: the registry holds a second reference to every payload, so a sweep that frees nothing is a memory leak, while an over-eager sweep has no observable effect as long as nothing reads through the registry. Audit mode stays for the stages that will. Where a transform still replaces a payload - transposeAxis, the applyRange family, tensorising a coordinate - the ref is cleared rather than re-derived. Those are genuinely different payloads and deriving keys for them is what the transposition and range stages do. Also adds `npm run test:unit`: mocha, ts-node and chai are already devDependencies, so this is a script and a CI workflow, not a toolchain. 16 tests over the key algebra and the sweep, no Electron, ~20 ms. Imports there must be relative - ts-node does not resolve the `src/*` alias without tsconfig-paths. The benchmark grows two cumulative columns, payloads and elements. On the benchmark canvas they read 8 / 25785, rising to 10 while the metadata panel fetches and falling back afterwards: that is the sweep, visible. Full e2e suite green, 20 passing. All five benchmark scenarios unchanged. Assisted-by: Claude/opus-5
Swapping two axes rebuilt every matrix in the grid with tfjs and threw the previous one away, so swapping back rebuilt it again. A transposed matrix is now a payload in its own right, named by composing the permutation onto the key of the array it came from. Composition is what makes it useful. A transposition permutes axes - out.dim[k] = in.dim[p[k]] - so applying q to an array already held as the base under p leaves the base under p[q[k]]. Keys never stack: every axis order of one payload names one entry, whichever route reached it, and a swap followed by the same swap back composes to the identity and names the untransposed array itself. The benchmark's new scenario pins that - swapping derives one payload, swapping back derives none and fetches nothing. The sweep now treats a payload and the views derived from it as one family, so a live view keeps its base and a live base keeps its views. Resident elements therefore go up, 25785 to 50985 on the benchmark canvas, and that is the trade: freeing the untransposed matrix as soon as the user looks at the transposed one would make going back cost a full transpose. A deleted grid still frees the whole family. Three deep copies on that path are gone with it, replaced by cloneGridStructure, which copies every object a transform assigns fields on and shares the arrays hanging off them: swapAxis deep-cloned every grid of the active configuration to modify one, getErrorYVectors deep-cloned every error payload on every slider tick, and transposeDataGrid cloned coordinates only to read axeIndex off them. Sharing arrays is sound because nothing in the renderer writes into a payload - transforms replace them wholesale. The stride scheme the plan called for is deferred to stage 11, where something finally reads through it; until the derived vectors leave the store every transform materialises at the boundary anyway. 20/20 e2e, 5/5 benchmark with every pre-existing count unchanged, 30 unit tests. Assisted-by: Claude/opus-5
A range used to be applied by cutting the array in the store down to size, which made it one-way: the wider data was gone, and the only way back was to ask the back end again. `restoreRange` therefore refetched once per trace plus once per error band, purely to recover what the trim had destroyed. Widening a range, or adding a trace to a grid that already had one, needed `oldRange` threaded through five async functions to turn an absolute range into an offset into an array that had already been cut, and `rangeAlreadyAppliedInPlot`, flipped by membership of a `newPlotsUri` list, to guess whether a particular trace had been narrowed yet. None of it had a test. A range is now a window on the payload: absolute bounds against the array the back end sent, named `...#value|range:3:119-177`, with the payload it was cut from still resident. Windows replace rather than stack, so applying [40,120] then [50,60] names the same entry as applying [50,60] to the untouched payload, and asking for no window names the payload itself. Re-applying a grid's ranges is therefore idempotent, which is what let `applyRangeInCoord`, `applyRangeInPlot`, `trimCoordData`, `trimPlotData` and `formatTrimmedCoordinate` collapse into one `applyRangesToGrid`, and deleted `oldRange`, `rangeAlreadyAppliedInPlot`, `shouldApplyRangeOriginInCoord` and the `newPlotsUri` plumbing outright. `applyRange` is pure now. Restoring lands on the full window rather than on the payload itself, which costs one slice the first time and nothing after: the payload is the response as parsed, in double precision, while every window is a tfjs slice and so single precision, and landing on the payload would move every value the moment a range was dropped. The same argument decides where a typed bound is resolved - against `Math.fround` of the coordinate, because a bound typed as 0.6 has to pick the point the axis labels 0.6. The old code got that by accident and inconsistently, resolving the first range of a grid against the raw response and every later one against a float32 array. Measured on the benchmark canvas: restoring a range goes from one plot_data per trace to none, and re-applying a range used before adds zero derivations. Resident elements rise from 50985 to 89100 and stay, which is the price of the restore no longer destroying anything; it is one family, so deleting the grid still frees all of it. plot-ui.spec.ts's range test passes unmodified, e2e 20/20, benchmark 6/6, unit 39. Assisted-by: Claude/opus-5
A trace's `y` is one row of a payload, taken with the integers the grid's coordinates carry, and its `x` is the vector of the axis on display. Both are determined by payload plus cursor, so storing them stored a copy that every writer touching either half had to rebuild. `handleUpdateCoordinate` was 130 lines doing exactly that on every slider tick, for the grid and for every grid synchronized with it: a new x, a new y, a new customdata and a new array per error band, each copied out of the payload. It is now one store action, `setCursor`, writing integers and labels and no array at all - so a tick costs the same whatever the payload weighs. What is drawn comes from a new `derive/vectors.ts`, called on the render path and memoised on the payload array itself and, within that, on the cursor. Moving one grid's cursor therefore cannot invalidate another grid's rows, and two synchronized grids at the same cursor derive once between them. The map is weak on the payload, so a family the sweep frees takes its derived rows with it. Heatmap2D held x, y, z and three axis descriptors in useState, filled by a chain of effects; Plotly compares `data` by reference, so each link was a separate draw. They are one useMemo now. `isSameAxisData` is deleted - it walked both coordinates element by element on every tick to decide whether a synchronized grid showed the same data, which `setCursor` now answers with a payload-key compare, and answers correctly: two grids can hold equal numbers from different nodes. `limitSlidersToMaxLength` survives as `clampCursors`, because a cursor is still an index that narrowing a range can leave out of bounds. `plot.x`, `plot.y`, `plot.customdata` and `error_bands[].array` are no longer written anywhere. `getErrorsAreaToPlot` became `buildTraces`, creating the objects Plotly is handed rather than copying the store's. `projectTestState` computes them on the way out and `mergeTestState` drops them on the way back in, so plot-ui.spec.ts's exact floats keep working unmodified - and now assert on what is drawn rather than on a stored copy. Measured: coordinate slider, 2 steps goes from 6 redraws / 28 renders to 2 / 16 - the row that had not moved since stage 2 - and swapping two axes from 3 / 10 to 1 / 4. The rows that open a customization panel show three more redraws and four fewer renders, because its preview no longer stays blank until its effects settle. e2e 20/20, benchmark 6/6, unit 55. Assisted-by: Claude/opus-5
Stage 11 decided which synchronized grids a slider moves by comparing the payload keys of their coordinates, on the grounds that two grids can hold equal numbers from different nodes. That is true, and it is exactly the case synchronization is used in: two panels plotting different nodes of one IDS each fetched the shared time coordinate inside their own response, so the arrays are equal under two different keys. Every one of them silently stopped following. The element-wise answer comes back, as `isSameAxisData` did it before. What the keys are used for now is remembering it: a key is stable while a cursor moves, so the walk runs once per pair of payloads and every later tick is a Map lookup, which is what stage 11 was after. An entry cannot go stale, because a key names the request that produced it. Guarded by a new e2e spec on a canvas of two panels over different nodes of one IDS - the shape the feature is used in, and one no existing spec built, which is why twenty green specs said nothing about this. Assisted-by: Claude/opus-5
`mergeTestState` kept `active` and its entry in `configurations` the same object only when the spec sent `configurations` too. A spec writing `active` alone - plot-sync.spec.ts links its panels that way - left the store's entry stale, and writers that start from `configurations` later put it back over `active`, silently undoing what the spec wrote. The re-link now falls back to the store's own list. Assisted-by: Claude/opus-5
…e awaiting `HoverButtons`' error-band effect deep-copied the configuration its render had closed over, awaited the band fetches, and wrote the whole copy back. Anything that happened during the await was undone when it landed: a slider moved, a panel linked. That is why plot-sync.spec.ts failed on this branch - the linked panel followed the slider, then the write reset both cursors and emptied both synchronisation lists. It also wrote on every mount and every toggle, whether or not a band arrived, replacing the configuration and every grid reference in it for nothing. The fetches now write into a structural copy of the store as it is when the effect runs - payload arrays shared, not cloned - and `mergeErrorBands` carries only what they are about onto the store as it is after the await: each trace's `error_bands` for the grids whose bands changed, and the band nodes checked or unchecked in the tree. Nothing changed, nothing is written. Benchmark: applying or restoring a data range goes from 12 redraws / 54 renders to 6 / 22, and a metadata revisit from 10 / 44 to 4 / 12 - those scenarios mount panels, and every mount used to cost a configuration write. Assisted-by: Claude/opus-5
… is now
Two writers of the same shape as the error-band one, masked until now because
that write happened to restore what they destroyed.
Tree updates - expanding a node, listing a data entry's IDSs, searching,
toggling error bars - awaited the backend and then wrote
`{ ...configurationBeforeAwait, customDataTree }`, dropping any plot made
meanwhile. They now go through `writeCustomDataTree`, which writes the tree
onto the latest configuration and nothing else.
`getNodesChecked` received the whole checked list as the tree saw it, and each
plot operation wrote back the configuration it had started from. Unchecking one
node while another was still loading therefore computed a list without the node
still loading, removed its grid, and the load then wrote its grid back over the
removal - every later click was one toggle out of step. A click is now reduced
to what it added and removed against the list the tree was showing, and the
operations run one at a time, each onto the configuration the previous one
left.
plot-ui.spec.ts caught the second one deterministically once the error-band
write stopped hiding it.
Assisted-by: Claude/opus-5
The request cache held response text and parsed it for every caller. Its own header said why: `fetchDataPlot` post-processed the parsed graph in place, and callers aliased its arrays into the store, where transforms then mutated them. Stages 9 to 11 made every transform assign a new array under a new registry key, so that reason is gone and the header with it. The cache now retains the response as the fetch layer finished it. The `plot_data` post-processing - renaming, forcing `[:]` targets, tensorising an irregular shape, the complex transform, nulls to NaN - moves into `normalizeDataPlotResponse` and runs once per request instead of once per caller, so it no longer has to be idempotent. A hit costs a copy of the objects around the arrays (`share`), never a `JSON.parse` of a multi-megabyte body. Metadata responses are small and are handed out as deep copies. The byte budgets still count the body length, and in-flight de-duplication and the expected 404/464 error-band suppression are unchanged - failures are never retained. The arrays a hit hands out are the ones the payload registry holds, so a payload is resident once rather than once parsed and once as text. Registration happens on every hand-out: the registry sweeps what no grid references while the cache may still hold it. A complex node is finished differently, so it is retained and registered under its own `#cpx` / `value:cpx` name. The "Data are incomplete" warning still fires on every hand-out. Sharing is sound only while nothing writes into a payload, so under E2E_TEST registration now deep-freezes each array and such a write fails the suites with a TypeError. Both ran green with it on. BASELINE.md records the stage, and prettier reformats the stage 10 and 11 tables, which `prettier --check .` in CI was rejecting. e2e 23/23 twice on fresh app instances, benchmark 6/6, unit 80. Assisted-by: Claude/opus-5
…forms Six deep clones were left in plot.ts, each copying every matrix of a grid to change fields around them: `updateInterpolatedPlots` and its caller in `handleExistingPlot`, `updateCoordsAfterInterpolation` (to set two integers per coordinate), the coordinate match check (to sort), `initPlotColors` (to set a line colour) and the saved-configuration loader (twice, once only to read axis indices). Every one of them reassigns fields and never writes into an array, so they become `cloneGridStructure`, a shallow copy, or no copy at all. `initPlotColors` was the one that wrote into a nested object, `plot.line`, which a structural copy shares with its source; it builds a new line instead. Assisted-by: Claude/opus-5
Opening visual customization or data manipulation on a 2-D node took over 13 s on the real 720-slice entry, against 1.6 s for a 1-D panel. The panel edits a detached copy of the grid, and that copy was a structuredClone - as were most edits made in it: a colour, a colourscale, a line shape, a smoothing or operation parameter, and the grid each data panel rebuilds after a fetch. Each duplicated every matrix the grid holds. The panel also deep-copied every trace on every render into a variable nothing read. They are structural copies now, and the handlers that wrote into nested objects - `line`, `customPreferences`, the geometry list - build new ones. Saving the panel's synchronisation links replaced the store's grids instead of assigning onto them in place. Measured on the real entry, opening and closing a menu on the psi heatmap: 13.5 s -> 1.5 s for visual customization, 13.4 s -> 1.4 s for data manipulation, 15.3 s -> 1.6 s opening the Heatmap section - the same as a 1-D panel now. Counts are unchanged, so the benchmark gains two rows that guard only that opening a panel fetches nothing; BASELINE.md records the real numbers. It also fixes what the copy exposed: nothing pinned it, so a payload fetched inside the panel was freed by the next sweep, and the next range applied there cut the already windowed array with absolute bounds. The panel pins every key its copy references, and a pin now keeps its payload's whole family alive, as a reachable key already did. e2e 23/23, benchmark 7/7, unit 81. Assisted-by: Claude/opus-5
The backend's `shape` is the node's shape before downsampling; the array in the response has `downsampled_shape`. The registry recorded the former, so restoring a data range on a downsampled trace (e.g. a DIII-D flux loop, 327680 samples sent as 1000) widened the window to the full node and tfjs threw "begin[1] + size[1] (327680) would overflow input.shape[1] (1000)". Assisted-by: Claude/opus-5
olivhoenen
requested review from
SimonPinches and
prasad-sawantdesai
as code owners
September 28, 2026 16:19
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a follow-up of #138:
This pull request focuses on improving frontend state management, simplifying grid editing logic, and adding a dedicated workflow for frontend unit tests.
These changes collectively improve the performance and reactivity of the frontend UI, avoiding unnecessary redraws and data saves, but also improving maintainability, and reliability of the frontend codebase.
Frontend state management and grid editing refactor:
GridLayoutPlot.tsxto use a singleeditingGridIdflag instead of per-gridisEditingfields, reducing unnecessary re-renders and simplifying state updates. Related actions like deleting or editing grids now update this flag and avoid rebuilding unrelated grid objects. (frontend/src/renderer/components/grid/GridLayoutPlot.tsx) [1] [2] [3]setMetadataGrid,setCustomizing) instead of rewriting the entire configuration, keeping transient UI state separate from saved configuration data. (frontend/src/renderer/components/grid/GridLayoutPlot.tsx) [1] [2]setCursor, ensuring only the relevant state is updated and improving performance. (frontend/src/renderer/components/grid/GridLayoutPlot.tsx)Testing and workflow improvements:
.github/workflows/frontend-unit-tests.ymlto run frontend unit tests automatically on push and pull request events, ensuring code quality.test:unitnpm script infrontend/package.jsonfor running unit tests on renderer components.Type and API consistency:
ConfigurationStatewithTestStatein Electron IPC handlers and preload scripts for consistency with the new state management approach. (frontend/src/main/ipc.ts,frontend/src/preload.ts) [1] [2] [3] [4] [5] [6]Code cleanup and dependency management:
GridLayoutPlot.tsxto streamline vector and coordinate handling. [1] [2]For reviewers, please focus on possible regressions you might spot w.r.t
develop.