Fix broken Clean→Analyze round-trip: persist cleaned epochs to host disk

docs/epoch-review-ui-plan.md

@@ -45,12 +45,18 @@ That environment has hard constraints:
   and (b) can't even cross the worker boundary — a `PyProxy`/Figure isn't
   `structuredClone`-able, so `postMessage` throws `DataCloneError`.
 
-**This is not an MNE-version problem.** We are already on **MNE 1.12.1** (current).
-The recent errors (`plot_psd` → `compute_psd`, `events=None` → `events=False`) are
-just the code catching up to the modern MNE API; bumping MNE won't remove them.
-The interactive-GUI problem is architectural: **native Matplotlib GUIs don't exist
-in WASM, and no MNE version changes that.** The only path to interactive epoch
-review is to build the UI ourselves — which is what this plan is for.
+**This is not an MNE-version problem — it's architectural.** We install the latest
+MNE from PyPI (`internals/scripts/InstallMNE.mjs` lists `mne` **unpinned** in
+`PYPI_PACKAGES`, so `postinstall` grabs whatever is current — a modern 1.x). The
+recent errors (`plot_psd` → `compute_psd`, `events=None` → `events=False`) were
+just the code catching up to the modern MNE API; no MNE version brings back native
+Matplotlib GUIs in WASM. The only path to interactive epoch review is to build the
+UI ourselves — which is what this plan is for.
+
+> **Note (grounding):** MNE is unpinned today. If exact-reproducibility of the
+> cleaned output matters (design goal 3), consider pinning the MNE version in
+> `InstallMNE.mjs` so a silent upstream bump can't change epoching/averaging
+> behavior out from under the tests.
 
 ---
 
@@ -133,14 +139,56 @@ Four concerns. Each has real design choices (deferred to the planning loop).
 We stop shipping a *GUI object* and instead ship the **raw numbers**, then render
 them ourselves. Needed from Python per "clean" request:
 - Epoch data array: `epochs.get_data()` → shape `(n_epochs, n_channels, n_times)`.
-- Metadata: `ch_names`, `sfreq`, `times`, per-epoch condition (from the marker
-  registry), and the **auto-reject flags + `drop_log`** for suggestions.
-
-**Transport is the crux** (and the lesson from this whole debugging session):
-don't JSON-serialize float arrays, and don't return a `PyProxy`. Get the numpy
-buffer as a **`Float32Array` / `ArrayBuffer`** and send it as a **transferable**
-over `postMessage` (zero-copy). Small metadata rides as JSON alongside. This
-sidesteps the serialization wall entirely — buffers cross cleanly; Figures don't.
+  (Returns float64; see the transport note below on downcasting to Float32.)
+- Metadata: `ch_names`, `sfreq`, `times`, **per-epoch condition codes**
+  (`epochs.events[:, -1]` — the numeric marker codes, *labeled* via the marker
+  registry's `codeToLabel`, not stored on the registry itself), and the
+  **auto-reject flags + `drop_log`** for suggestions.
+
+> **Grounding — exclude the Marker/stim channel from the arrays.** `get_data()`
+> returns *all* channels, including the trailing `Marker` channel that carries the
+> numeric event codes. The existing channel picker already drops it
+> (`requestChannelInfo` runs `[ch for ch in clean_epochs.ch_names if ch !=
+> 'Marker']`); `get_epochs_arrays` must do the same, or the reviewer renders the
+> raw code channel as a bogus "trace" and `ch_names` / the channel count are
+> off-by-one. (The per-epoch condition codes still come from `epochs.events[:,
+> -1]`, which is unaffected by dropping the channel.)
+
+> **Grounding correction — the registry gives labels, not colors.**
+> `buildMarkerRegistry` (`markerRegistry.ts`) returns only `{ codeToLabel,
+> eventId }`. There is **no color** on it. Condition colors today are hardcoded
+> RGB palettes inside `utils.py` (`plot_topo`, `plot_conditions`). So condition
+> coloring in the React UI needs a **new color source** — decide whether to (a)
+> define a palette UI-side keyed by condition label, or (b) promote the `utils.py`
+> palette into a single shared source both Python plots and the React reviewer
+> read from (preferred, avoids the ERP/topo legend and the reviewer disagreeing on
+> which color is which condition). Track this as a small design decision, not a
+> reuse of existing code.
+
+**Transport is the crux** (the lesson of this whole debugging session): don't
+JSON-serialize float arrays and don't return a `PyProxy`. Get the numpy buffer as
+an **`ArrayBuffer`** and post it as a **transferable** (zero-copy); the metadata
+above rides as JSON on the same message. Buffers cross cleanly; Figures don't.
+
+> **Downcasting to Float32 is safe here — and only here.** `get_data()` is
+> float64; halving it to Float32 loses precision, but this buffer is *display
+> only*. The apply path (§6d) is **index-based** (`epochs.drop(indices)`) — the
+> epoch samples never round-trip out of Python and back, so the cleaned `.fif`
+> stays bit-identical to what MNE would produce (design goal 3). Keep it that way:
+> never reconstruct epochs from the Float32 buffer on the Python side.
+
+**This means extending the worker message contract, not just adding a Python
+helper (grounded in `webworker.js` today).** The current handler is
+one-size-fits-all: it runs `data`, and if the result has `.toJs` it converts the
+`PyProxy` and posts `{ results, plotKey, dataKey }` **with no transfer list** — so
+it can't ship a zero-copy buffer. Phase 0 must:
+- Return the numpy buffer as an `ArrayBuffer` (e.g. `arr.tobytes()` off the
+  `PyProxy`, or via the emscripten heap) and `self.postMessage({ buffer, meta,
+  dataKey }, [buffer])` — the `[buffer]` transfer list is mandatory, or the buffer
+  is structured-cloned (a copy).
+- Route it on a **new `dataKey`** (e.g. `'epochArrays'`) so it bypasses the
+  existing PyProxy→`toJs`→SVG-string switch in `pyodideMessageEpic` (which assumes
+  results are plain JS or an SVG string).
 
 ### 6b. Rendering
 Muse is tiny (4 ch × ~256 samples × dozens of epochs), but we must not design for
@@ -162,10 +210,40 @@ Muse is tiny (4 ch × ~256 samples × dozens of epochs), but we must not design
 ### 6d. Apply path: renderer → Python
 - Collect rejected epoch indices + bad channels → dispatch an action → epic posts
   to the worker → Python `epochs.drop(indices)` + set `info['bads']` → save
-  `-cleaned-epo.fif` via the existing MEMFS/`saveEpochs` path.
+  `-cleaned-epo.fif`.
 - The result must be **bit-identical to what MNE's GUI would have produced** — the
   UI changes, the science does not.
 
+**Persistence is a real gap — the Clean→Analyze round-trip is broken today.**
+Traced end to end against `webworker/`, `src/main/index.ts`, `src/preload/index.ts`:
+- **Save side:** `saveEpochs` (`webworker/index.ts`) posts `raw_epochs.save("<host
+  OS path>")`, but Pyodide's worker FS is **MEMFS (in-memory)** — there is **no
+  NODEFS/`syncfs`/mount anywhere in `webworker/`**, so the write lands in the WASM
+  virtual FS, never on host disk.
+- **Load side:** the Analyze screen re-reads *host disk* both times — at
+  `componentDidMount` it calls `readWorkspaceCleanedEEGData` (main-process `fs`) to
+  populate the picker, then on selection `LoadCleanedEpochs` → `writeEpochsToMemfs`
+  → `fs:readFileAsBytes` stages host bytes into MEMFS. It never reuses in-session
+  MEMFS epochs.
+- **The gap:** nothing ever writes the cleaned `.fif` *to* host disk. Main has
+  write IPC (`fs:storePyodideImagePng`, `eeg:createWriteStream`,
+  `fs:storeAggregatedBehaviorData`) but **none writes an epoch `.fif` from the
+  worker**, so a fresh workspace has nothing for Analyze to load. The fix is a
+  **new MEMFS→host write-back bridge**, and because the worker can't reach
+  `ipcMain` it must cross every process boundary (see the `electron-ipc-channel`
+  skill) — don't collapse it to "postMessage → handler". The full chain: Python
+  `save()` to a MEMFS path → worker `pyodide.FS.readFile(path)` + `postMessage` the
+  bytes back on a **new `dataKey`** (e.g. `'savedEpochs'`, routed in
+  `pyodideMessageEpic` beside the existing `epochsInfo`/`channelInfo` cases — which
+  return no transfer list today, so shipping bytes zero-copy means extending that
+  reply path too) → a new epic → a new `window.electronAPI` method → the preload
+  bridge → a new main handler. **Reuse anchor, not new machinery:**
+  `fs:storePyodideImagePng` (`src/main/index.ts` line 315) is already this pattern —
+  `(title, imageTitle, rawData: ArrayBuffer)` → `Buffer.from(rawData)` →
+  `fs.writeFile` to disk; the write-back handler should mirror *it* (a binary
+  renderer→disk write), not `fs:readFileAsBytes`. This is not a "persist" nicety —
+  **it repairs a currently broken flow**.
+
 ### 6e. The onboarding layer (the differentiator)
 - Plain-language explanations of epochs and each artifact type.
 - **Guided mode** (default for newcomers): step through auto-flagged epochs with
@@ -180,21 +258,36 @@ Muse is tiny (4 ch × ~256 samples × dozens of epochs), but we must not design
 
 - **Worker protocol** (`pyodide-mne` skill): today's plots use fire-and-forget
   `postMessage` + `plotKey`/`dataKey` reply routing through `pyodideMessageEpic`.
-  A data-heavy request/response (fetch epoch arrays, get result back, then let the
-  user act) pushes toward finally doing the **`runPython` RPC** already tracked in
-  `TODOS.md` ("Optional Full Pyodide worker RPC"). This feature is the strongest
-  reason yet to build it — worth deciding early.
+  Note the `dataKey` pattern **already does request/response round-trips**
+  (`epochsInfo` → `SetEpochInfo`, `channelInfo` → `SetChannelInfo`), so fetching
+  epoch arrays and applying rejection can be built on it directly — a `runPython`
+  RPC is **not a hard prerequisite**. The `TODOS.md` item ("(Optional) Full
+  Pyodide worker RPC") itself says it's only worth doing "if the FIFO sequencing
+  ever actually bites." Where it *would* help this feature: an `await`-able RPC
+  lets an apply-then-refresh sequence (drop epochs → save → re-fetch stats/ERP)
+  read real results instead of relying on worker FIFO ordering across several
+  fire-and-forget messages. Treat it as an ergonomics/scaling call, not a blocker
+  (see Open Question 2).
 - **New Python helpers** (`webworker/utils.py`): `get_epochs_arrays(epochs)` →
   buffer + metadata; `apply_rejection(epochs, drop_indices, bad_channels)`. Keep
   them native-testable (the `tests/analysis/` pattern) so cleaning logic is
   verified against real MNE in CI.
 - **New epic(s)** in `pyodideEpics.ts` + **new actions** (fetch/set epoch data,
-  apply rejection). Reuse `buildMarkerRegistry` for condition labels/colors.
-- **New React component** (`EpochReviewer` or similar) replacing the plot area in
-  `CleanComponent`. Styling: shadcn/ui + Tailwind, brand teal, student tone.
+  apply rejection). Reuse `buildMarkerRegistry` for condition **labels** (code↔label
+  only, no colors — see §6a for where condition colors must come from).
+- **New React component** (`EpochReviewer` or similar) **added to**
+  `CleanComponent`. Correction: `CleanComponent` has **no plot area** to replace —
+  today it renders only subject/recording `<select>`s, Load/Clean buttons, and
+  epoch-count stats (`renderEpochLabels`). The static plots (`PyodidePlotWidget`,
+  which renders an SVG string via an `<img>`) live in `AnalyzeComponent` and
+  `HomeComponent`, not here. So the reviewer is a genuinely new interactive
+  surface on the Clean screen, wired to the `Load Dataset`/`Clean Data` flow that
+  already dispatches `LoadEpochs`/`CleanEpochs`. Styling: shadcn/ui + Tailwind,
+  brand teal, student tone.
 - **Constraints to respect:** headless `agg` in the worker; no large-array JSON;
-  transferables for buffers; keep main/renderer/worker separation clean; don't
-  leak Pyodide/MNE types into React.
+  transferables for buffers (which means extending the worker's `postMessage` to
+  pass a transfer list — see 6a); keep main/renderer/worker separation clean;
+  don't leak Pyodide/MNE types into React.
 
 ---
 
@@ -215,17 +308,36 @@ Muse is tiny (4 ch × ~256 samples × dozens of epochs), but we must not design
    fast-follow?
 7. **Static fallback** — keep a read-only SVG epochs view for environments where
    the interactive UI can't run, or all-in on the React UI?
+8. **Save persistence — resolved (see §6d), not really open.** The Clean→Analyze
+   round-trip is broken today; the MEMFS→host write-back bridge fixes it. Only
+   residual decision: write to the existing
+   `Data/<subject>/EEG/<subject>-cleaned-epo.fif` path that
+   `readWorkspaceCleanedEEGData` scans for (`epo.fif` suffix, confirmed in
+   `src/main/index.ts`) so Analyze's picker finds it unchanged.
 
 ---
 
 ## 9. Rough phases (to be firmed up in planning)
 
-- **Phase 0 — Transport & read-only render.** Ship epoch arrays + metadata across
-  the worker boundary (transferable buffers); render static traces in React.
-  Proves the data path and rendering choice.
+- **Phase 0 — Transport & read-only render.** Extend the worker message contract
+  to return an `ArrayBuffer` on a new `dataKey` with a real transfer list (see
+  6a); add the `get_epochs_arrays` Python helper; ship epoch arrays + metadata
+  across the worker boundary; render static traces in React. Proves the data path
+  and rendering choice. **Prerequisite input:** the rendering-tech decision (Open
+  Question 1, Canvas 2D vs WebGL) must be made *before* the "render static traces"
+  step — it determines the component's core. **Also (cheap, do it first):** the
+  one-line runtime confirmation of the broken Clean→Analyze round-trip (Open
+  Question 8) — now a sanity check, not a research task, since §6d resolves it
+  statically.
 - **Phase 1 — Core interaction.** Click-to-reject epochs, scroll/scale/zoom, apply
-  → `epochs.drop` → save `-cleaned-epo.fif`. Reaches functional parity with the
-  *essential* MNE workflow.
+  → `epochs.drop` → save `-cleaned-epo.fif` **via the new MEMFS→host write-back
+  bridge (see §6d)**. Reaches functional parity with the *essential* MNE workflow.
+  - **Sequencing note:** the write-back bridge is *not* intrinsically a Phase-1
+    dependency. It reuses the same worker-message-contract extension as Phase 0
+    (§6a) and independently fixes a live bug (§6d), so it can land early and
+    standalone — make the existing `Clean Data` button actually persist alongside
+    the Phase-0 transport work, de-risking the apply path before the reject flow
+    exists. Phase 1 then just points the apply action at a bridge that works.
 - **Phase 2 — Full parity.** Bad-channel flagging, condition coloring/legend,
   auto-flag suggestions from `drop_log`/peak-to-peak.
 - **Phase 3 — Onboarding layer.** Explanations, guided mode, artifact tutorials,
@@ -238,7 +350,9 @@ Muse is tiny (4 ch × ~256 samples × dozens of epochs), but we must not design
 ## 10. Context references
 
 - **Skills:** `pyodide-mne` (worker↔Python protocol, plot routing, `pyodide://`),
-  `redux-observable-epochs` (epic anatomy, numeric marker-code contract).
+  `redux-observable-epochs` (epic anatomy, numeric marker-code contract),
+  `electron-ipc-architecture` + `electron-ipc-channel` (the four files a new IPC
+  channel must keep in sync — needed for the §6d write-back bridge).
 - **Docs:** `docs/pyodide-in-electron-vite.md`, `docs/user-flow.md`.
 - **Learnings** (`.llms/learnings.md`): agg backend / WebAgg-in-worker limits;
   plot-result routing pattern; PyProxy serialization; marker registry / numeric
@@ -247,5 +361,5 @@ Muse is tiny (4 ch × ~256 samples × dozens of epochs), but we must not design
   (worker + Python), `src/renderer/epics/pyodideEpics.ts` (epics),
   `src/renderer/components/CleanComponent/`, `PyodidePlotWidget.tsx` (existing
   static-plot render path), `src/renderer/utils/eeg/markerRegistry.ts`.
-- **Related TODO:** `TODOS.md` → "Optional Full Pyodide worker RPC" (this feature
-  is the strongest motivation to build it).
+- **Related TODO:** `TODOS.md` → "(Optional) Full Pyodide worker RPC" — an
+  `await`-able apply-then-refresh ergonomics win, **not** a prerequisite (§7, OQ2).

src/main/index.ts

@@ -326,6 +326,28 @@ ipcMain.handle(
   }
 );
 
+// Writes the cleaned epochs .fif from the Pyodide worker's in-memory MEMFS to
+// host disk (the worker filesystem cannot reach host paths on its own). Analyze's
+// `readWorkspaceCleanedEEGData` scans for the `epo.fif` suffix this produces.
+ipcMain.handle(
+  'fs:writeCleanedEpochs',
+  (_event, title: string, subject: string, rawData: ArrayBuffer) => {
+    const dir = path.join(getWorkspaceDir(title), 'Data', subject, 'EEG');
+    mkdirPathSync(dir);
+    const buffer = Buffer.from(rawData);
+    return new Promise<void>((resolve, reject) => {
+      fs.writeFile(
+        path.join(dir, `${subject}-cleaned-epo.fif`),
+        buffer,
+        (err) => {
+          if (err) reject(err);
+          else resolve();
+        }
+      );
+    });
+  }
+);
+
 ipcMain.handle('fs:deleteWorkspaceDir', (_event, title) =>
   shell.trashItem(path.join(workspaces, title))
 );

src/preload/index.ts

@@ -117,6 +117,13 @@ contextBridge.exposeInMainWorld('electronAPI', {
   ): Promise<void> =>
     ipcRenderer.invoke('fs:storePyodideImagePng', title, imageTitle, rawData),
 
+  writeCleanedEpochs: (
+    title: string,
+    subject: string,
+    rawData: ArrayBuffer
+  ): Promise<void> =>
+    ipcRenderer.invoke('fs:writeCleanedEpochs', title, subject, rawData),
+
   deleteWorkspaceDir: (title: string): Promise<void> =>
     ipcRenderer.invoke('fs:deleteWorkspaceDir', title),
 

src/renderer/epics/pyodideEpics.ts

@@ -1,11 +1,10 @@
 import { combineEpics, Epic } from 'redux-observable';
-import { EMPTY, fromEvent, Observable, of } from 'rxjs';
-import { map, mergeMap, tap, pluck, filter } from 'rxjs/operators';
+import { EMPTY, from, fromEvent, Observable, of } from 'rxjs';
+import { map, mergeMap, tap, pluck, filter, catchError } from 'rxjs/operators';
 import { toast } from 'react-toastify';
 import { isActionOf } from '../utils/redux';
 import { PyodideActions, PyodideActionType } from '../actions';
 import { RootState } from '../reducers';
-import { getWorkspaceDir } from '../utils/filesystem/storage';
 import { buildMarkerRegistry } from '../utils/eeg/markerRegistry';
 import {
   loadCSV,
@@ -77,7 +76,7 @@ const pyodideMessageEpic: Epic<
   PyodideActionType,
   PyodideActionType,
   RootState
-> = (action$) =>
+> = (action$, state$) =>
   action$.pipe(
     filter(isActionOf(PyodideActions.SetPyodideWorker)),
     pluck('payload'),
@@ -107,6 +106,24 @@ const pyodideMessageEpic: Epic<
         // results is an array of channel-name strings
         return of(PyodideActions.SetChannelInfo(results as string[]));
       }
+      if (dataKey === 'savedEpochs') {
+        const savedEpochsBuffer = e.data.savedEpochsBuffer as ArrayBuffer;
+        const { title, subject } = state$.value.experiment;
+        return from(
+          window.electronAPI.writeCleanedEpochs(
+            title,
+            subject,
+            savedEpochsBuffer
+          )
+        ).pipe(
+          tap(() => toast.success('Cleaned data saved')),
+          mergeMap(() => EMPTY),
+          catchError((err) => {
+            toast.error(`Failed to save cleaned data: ${err?.message ?? err}`);
+            return EMPTY;
+          })
+        );
+      }
 
       // Route plot results to the appropriate Redux state slot.
       // results is an SVG string returned from Python.
@@ -189,10 +206,8 @@ const cleanEpochsEpic: Epic<PyodideActionType, PyodideActionType, RootState> = (
     filter(isActionOf(PyodideActions.CleanEpochs)),
     mergeMap(async () => {
       await cleanEpochsPlot(state$.value.pyodide.worker!);
-      const dir = await getWorkspaceDir(state$.value.experiment.title);
       return saveEpochs(
         state$.value.pyodide.worker!,
-        dir,
         state$.value.experiment.subject
       );
     }),

src/renderer/types/electron.d.ts

@@ -65,6 +65,11 @@ declare global {
       imageTitle: string,
       rawData: ArrayBuffer
     ) => Promise<void>;
+    writeCleanedEpochs: (
+      title: string,
+      subject: string,
+      rawData: ArrayBuffer
+    ) => Promise<void>;
     deleteWorkspaceDir: (title: string) => Promise<void>;
     readImages: (dir: string) => Promise<string[]>;
     getImages: (params: unknown) => Promise<string[]>;