Skip to content

Commit 5655274

Browse files
edenoclaude
andcommitted
docs(plan): fix .trodesconf importer store-write path + DIO/type cleanups
Third/fourth-review fixes to phase-4 apply + phase-1 + shared-contracts: - re-import lands in BOTH the versioned snapshot AND the editable animal.devices mirror, via an extended addConfigurationSnapshotToAnimal that also carries the off-export siblings; the mirror sync is probe-fields-only (electrode_groups + ntrode map) so data_acq_device/device are preserved, not dropped. - first-config seeds v1 by importing before createAnimal (extended to set v1 siblings); the sibling-incapable updateAnimal fallback is removed. - sourceName added to TrodesconfImportPlan; dioInventory typed ParsedDioChannel[]; phase-1 DIO direction aligned to shared-contracts (input=0 => out, ECU_ ids). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 2835128 commit 5655274

3 files changed

Lines changed: 75 additions & 32 deletions

File tree

‎.claude/docs/plans/trodesconf-import/phase-1-parser.md‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,13 @@ is its sole producer.
2727
- **ntrodes:** select every `<SpikeNTrode>` (in document order); for each, read `id` (Number),
2828
`refNTrodeID`/`refChan` (Number, optional — `refNTrodeID <= 0` ⇒ omit), and its nested
2929
`<SpikeChannel>` elements → `channelCount = count`, `hwChans = [Number(hwChan)…]` in order.
30-
- **dio:** select `<Channel dataType="digital">`; resolve `direction` by the stricter rule in
31-
[shared-contracts §1](shared-contracts.md#1-parsedtrodesconfig) — explicit `input="1"`⇒`in` /
32-
`output="1"`⇒`out`; else id token (`…Din…`⇒`in`, `…Dout…`⇒`out`); else `'unknown'`. **Do NOT treat a
33-
missing `input` as output** — `BlankWorkspace.trodesconf` has `Controller_Din*` with no `input` attr.
34-
Preserve board-native ids **verbatim** (`Din1`, `MCU_Din3`, `Controller_Din1`, `Dout2`). De-duplicate
35-
by `id`.
30+
- **dio:** select `<Channel dataType="digital">`; resolve `direction` **exactly per**
31+
[shared-contracts §1](shared-contracts.md#1-parsedtrodesconfig): if an `input` attr is present,
32+
`input="1"`⇒`in`, **`input="0"` (or any non-`1`)⇒`out`** (matching `convert_dios.py`); **else** (no
33+
`input` attr — e.g. `BlankWorkspace.trodesconf`'s `Controller_Din*`) by id token **after stripping a
34+
leading `ECU_`/`MCU_`/`Controller_` prefix**: `…Din…`⇒`in`, `…Dout…`⇒`out`; else `'unknown'`. **Never
35+
treat a missing `input` as output.** Preserve board-native ids **verbatim** (`Din1`, `ECU_Din1`,
36+
`ECU_Dout1`, `MCU_Din3`, `Controller_Din1`, `Dout2`). De-duplicate by `id`.
3637
- **header metadata:** read `numChannels` + `samplingRate` from `<HardwareConfiguration>` (or
3738
`<GlobalOptions>` in older configs — check both) as Numbers; absent → omit. (`numChannels` powers the
3839
Phase 2 integrity check; `samplingRate` is provenance only.)
@@ -80,5 +81,6 @@ Dispatch `code-reviewer` against the diff. Confirm:
8081
- Parser is total (the malformed/empty/sparse tests prove no throw escapes).
8182
- Output matches [`ParsedTrodesConfig`](shared-contracts.md#1-parsedtrodesconfig) exactly; `hwChans`/refs
8283
are parsed but documented as reference-only.
83-
- Board variants (`Din*` vs `MCU_Din*`) both handled.
84+
- Direction matches §1 (`input="0"`⇒out; missing `input`⇒id-token); all id variants handled (`Din*`,
85+
`ECU_Din*`/`ECU_Dout*`, `MCU_Din*`, `Controller_Din*`), kept verbatim.
8486
- No app-type coupling; no plan/phase references in code or test names.

‎.claude/docs/plans/trodesconf-import/phase-4-apply.md‎

Lines changed: 65 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,20 @@ byte-identity gate.
1717
- [src/state/workspaceActions.ts:140-160](../../../../src/state/workspaceActions.ts) — the create-animal
1818
config-seed path for `isFirstConfig` (seed v1).
1919
- [src/state/workspaceTransitions.ts:74-80](../../../../src/state/workspaceTransitions.ts) —
20-
`ConfigSnapshotInput = {date, description, devices}`. **Extend the type AND the snapshot-building
21-
transition** so `dioInventory` reaches the persisted `ConfigurationSnapshot` (smaller-correction #2:
22-
type alone is insufficient).
20+
`ConfigSnapshotInput = {date, description, devices}` (extend with the two siblings).
21+
- [src/state/workspaceTransitions.ts:240-258](../../../../src/state/workspaceTransitions.ts) —
22+
`addConfigurationSnapshotToAnimal`: builds the snapshot from `{date,description,devices}` only and
23+
**does not set `updated.devices`** (the mirror). Extend it to carry the siblings + set the mirror
24+
(must-fix #1, see Tasks).
25+
- [src/state/workspaceTransitions.ts:155-168](../../../../src/state/workspaceTransitions.ts) —
26+
`applyAnimalUpdates`' devices-mirror (rewrites only the latest snapshot's `electrode_groups`/`ntrode…`,
27+
not the siblings) — why `updateAnimal` can't seed v1's siblings (must-fix #2) and why siblings survive
28+
later edits.
29+
- [src/pages/AnimalEditor/wiring/ElectrodeGroupsContainer.tsx:94](../../../../src/pages/AnimalEditor/wiring/ElectrodeGroupsContainer.tsx)
30+
— the editor reads `animal.devices` (the mirror); hence the mirror must match the new snapshot.
2331
- [src/state/workspaceTypes.ts:299-322](../../../../src/state/workspaceTypes.ts) —
24-
`ConfigurationSnapshot` (add optional `dioInventory` as a **sibling** of `devices` so `diffProbeConfigs`
25-
is untouched).
32+
`ConfigurationSnapshot` (add optional `dioInventory: ParsedDioChannel[]` + `trodesImport` as
33+
**siblings** of `devices` so `diffProbeConfigs` is untouched).
2634
- [src/state/persistence.ts:16-20,188](../../../../src/state/persistence.ts) +
2735
[src/state/workspaceMigrations.ts:70-103](../../../../src/state/workspaceMigrations.ts) — the
2836
persistence-invariant decision (overview Rollout).
@@ -35,25 +43,52 @@ byte-identity gate.
3543

3644
- **Off-export snapshot siblings + transition.** Add to `ConfigurationSnapshot` (both **siblings** of
3745
`devices`, so `diffProbeConfigs` and the export — which reads only `devices` — are untouched):
38-
`dioInventory?: DioChannel[]` and `trodesImport?: { ntrodeChannelCounts: Record<string, number>;
39-
sourceName?: string }` (the off-export counts the [§3](shared-contracts.md#3-invariant) completion
40-
needs while `device_type` is blank). Add both to `ConfigSnapshotInput` **and** thread them through the
41-
snapshot-building transition (`workspaceTransitions.ts`) — the type alone is insufficient
42-
(smaller-correction #2). Read via defensive selectors (`getConfigDioInventory`,
46+
`dioInventory?: ParsedDioChannel[]` (the shared §1 type — **not** an undefined `DioChannel`) and
47+
`trodesImport?: { ntrodeChannelCounts: Record<string, number>; sourceName?: string }` (the off-export
48+
counts the [§3](shared-contracts.md#3-invariant) completion needs while `device_type` is blank). Add
49+
both to `ConfigSnapshotInput`. Read via defensive selectors (`getConfigDioInventory`,
4350
`getConfigImportCounts`; absent → `[]`/`{}`).
44-
- **Apply — re-import vs first-config (must-fix #2).** `applyTrodesconfImport(actions, animalId, plan,
51+
- **Extend the snapshot transition `addConfigurationSnapshotToAnimal`
52+
([workspaceTransitions.ts:248-258](../../../../src/state/workspaceTransitions.ts)) — type alone is
53+
insufficient (must-fix #1/#2).** Today it builds `newVersion` from only `{date, description, devices}`
54+
and **does not touch `updated.devices`** (the editable mirror the editor reads at
55+
[ElectrodeGroupsContainer.tsx:94](../../../../src/pages/AnimalEditor/wiring/ElectrodeGroupsContainer.tsx)).
56+
Change it to (a) copy `config.dioInventory`/`config.trodesImport` onto `newVersion`, **and** (b) sync
57+
the new snapshot's **probe fields** into the mirror, **preserving the rest of `animal.devices`**.
58+
`animal.devices` is a `DeviceConfiguration` that also owns `data_acq_device` + `device`
59+
([workspaceTypes.ts:157-166](../../../../src/state/workspaceTypes.ts)) — the snapshot `devices` is a
60+
probe-only `ProbeConfiguration` ([:299-322](../../../../src/state/workspaceTypes.ts)), and export reads
61+
`data_acq_device`/`device` from the mirror ([workspaceUtils.ts:425,465](../../../../src/state/workspaceUtils.ts)),
62+
so a full `updated.devices = newVersion.devices` would **drop the recording-system/device catalog**. Use
63+
the same merge `applyAnimalUpdates` uses:
64+
```ts
65+
updated.devices = normalizeDevices({
66+
...getAnimalDevices(updated),
67+
electrode_groups: structuredClone(newVersion.devices.electrode_groups),
68+
ntrode_electrode_group_channel_map: structuredClone(newVersion.devices.ntrode_electrode_group_channel_map),
69+
});
70+
```
71+
**This is a no-op for the existing `ReconfigWizard` caller** (it passes a *clone of the current* probe
72+
devices, so the merged mirror is unchanged) and the fix for import (different probe devices) — add a
73+
`ReconfigWizard` regression test proving no behavior change there. Without (b), after a Trodes re-import the editor would still show the
74+
stale mirror and a subsequent edit would mirror it back over the imported snapshot (data loss). Note
75+
`applyAnimalUpdates`'s devices-mirror only rewrites the latest snapshot's `electrode_groups`/`ntrode…`,
76+
not the snapshot siblings ([workspaceTransitions.ts:161-167](../../../../src/state/workspaceTransitions.ts)),
77+
so the siblings survive later edits.
78+
- **Apply — re-import vs first-config (must-fix #1/#2).** `applyTrodesconfImport(actions, animalId, plan,
4579
dayIds)` (e.g. `src/state/trodesconfImportApply.ts`):
4680
- *Re-import* (`!isFirstConfig`): build `ConfigSnapshotInput = { date, description: "Imported from
47-
<plan.sourceName>", devices, dioInventory, trodesImport }` and call
48-
`createConfigurationSnapshotAndApplyForward` (a new version — correct here).
49-
- *First config* (`isFirstConfig`): **do NOT** call `createConfigurationSnapshotAndApplyForward` —
50-
`createAnimal` already creates `configurationHistory[0]` (v1)
51-
([workspaceActions.ts:154-161](../../../../src/state/workspaceActions.ts)), so that would make **v2**.
52-
Instead **seed v1 from the import**: either (preferred) build the parsed config *before* `createAnimal`
53-
and pass `devices` + the two siblings into the create path (extend `createAnimal`/its v1-snapshot
54-
construction to accept `dioInventory`/`trodesImport`), **or** replace v1 in place via `updateAnimal`
55-
immediately after create (overwrite `configurationHistory[0].devices` + set the siblings; no new
56-
version). Pick one in the PR; the result is a single v1 == the import.
81+
${plan.sourceName}", devices, dioInventory: plan.dioInventory, trodesImport: { ntrodeChannelCounts:
82+
plan.ntrodeChannelCounts, sourceName: plan.sourceName } }` and call
83+
`createConfigurationSnapshotAndApplyForward` (a new version — correct here). With the transition fix
84+
above, this both appends the snapshot **and** updates the mirror atomically.
85+
- *First config* (`isFirstConfig`): **import before `createAnimal`** and seed v1 from it.
86+
`createAnimal` already sets **both** `animal.devices` and `configurationHistory[0].devices` from the
87+
same `devices` input ([workspaceActions.ts:142,154-161](../../../../src/state/workspaceActions.ts)),
88+
so passing the imported `devices` makes mirror == v1 == import. **Extend `createAnimal` to also accept
89+
+ set v1's `dioInventory`/`trodesImport`.** Do **not** use the `updateAnimal` fallback: `updateAnimal`
90+
only mirrors `devices` into the latest snapshot and has **no path** to set the snapshot siblings
91+
([workspaceTransitions.ts:155-168](../../../../src/state/workspaceTransitions.ts)) — must-fix #2.
5792
- **The written `ntrode_id`s are the plan's preserved ids** ([§3](shared-contracts.md#3-invariant)) —
5893
never re-derived here.
5994
- **Persistence decision (from the code).** Determine if the load/shape-ensure path tolerates a snapshot
@@ -75,10 +110,12 @@ byte-identity gate.
75110
| --- | --- |
76111
| `applyTrodesconfImport` — first config seeds **v1** | after import-at-create, `configurationHistory.length === 1` and `version === 1` (NOT 2); the snapshot's `devices` = the import, with `dioInventory` + `trodesImport.ntrodeChannelCounts` set. |
77112
| `applyTrodesconfImport` — re-import | new version via the reconfig action; preserved groups keep `device_type`/`location` + grouping; days re-pin. |
113+
| re-import updates the **mirror** (must-fix #1) | after re-import, `animal.devices`'s **probe fields** (`electrode_groups`/`ntrode…`) equal the imported snapshot's, while `data_acq_device` + `device` are **preserved** (not dropped); a subsequent `updateAnimal` edit mirrors into the NEW snapshot, not over it. |
114+
| `addConfigurationSnapshotToAnimal` reconfig regression | the existing reconfig path (config.devices = clone of current) is unchanged — mirror stays equal, no observable behavior change. |
78115
| `ntrode_id` integrity | the persisted snapshot's `ntrode_id`s equal the plan's (== parsed config's); none renumbered on the way to disk. |
79116
| bad channels reset | bad channels on the old version don't carry onto the new (inherent to the bump). |
80117
| persistence | a blob without `dioInventory`/`trodesImport` loads with the defensive selectors → `[]`/`{}`; if bumped, the `vN` fixture hydrates to current. |
81-
| `baselines` | exported YAML byte-identical; **both `dioInventory` and `trodesImport` are off-export** (export reads only `devices`). |
118+
| `baselines` | exported YAML byte-identical; **both `dioInventory` and `trodesImport` are off-export** (they sit on the snapshot, outside any `devices`; the export merge never reads them). |
82119

83120
## Fixtures
84121

@@ -87,6 +124,9 @@ Phase 2/3 plan + completed-config fixtures; a persistence blob if a bump is take
87124
## Review
88125

89126
Dispatch `code-reviewer`. Confirm: reuses the reconfig/seed actions (no duplicated version logic);
90-
`dioInventory` threaded through the transition (not just typed) + defensively read; persisted
91-
`ntrode_id`s preserved; `baselines` byte-identical + `dioInventory` off-export; CHANGELOG updated; no
92-
plan/phase refs in code.
127+
re-import sets **both** the new snapshot **and** the editable `animal.devices` mirror (must-fix #1), with
128+
a passing `ReconfigWizard` no-op regression; first-config seeds **v1** via the extended `createAnimal`
129+
(not a v2, not the sibling-incapable `updateAnimal` fallback — must-fix #2); both siblings (`ParsedDioChannel[]`
130+
`dioInventory` + `trodesImport`) threaded through `addConfigurationSnapshotToAnimal` + defensively read;
131+
persisted `ntrode_id`s preserved; `baselines` byte-identical + both siblings off-export; CHANGELOG
132+
updated; no plan/phase refs in code.

‎.claude/docs/plans/trodesconf-import/shared-contracts.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ export interface TrodesconfImportPlan {
135135
message: string;
136136
}>;
137137
isFirstConfig: boolean;
138+
sourceName: string; // carried from ParsedTrodesConfig.sourceName — used for the snapshot description + trodesImport.sourceName
138139
}
139140
```
140141
- `ntrodeShells` carry **`ntrode_id` verbatim** ([§3](#3-invariant)) with `map: {}`. Expected channel

0 commit comments

Comments
 (0)