Skip to content

Commit 6e5aae2

Browse files
committed
Refactor and enhance import robustness, validation rules, and UX clarity across multiple phases
- Improved handling of legacy files with space-key normalization in import repair (Phase 8). - Updated validation rules to address false positives in orphaned file checks and added checks for associated files (Phases 8-12). - Enhanced user experience in the Day Editor with a new layout and read-only sections for inherited metadata (Phase 15). - Addressed first-run completeness issues in the Create-Animal wizard, ensuring required fields are populated (Phase 14). - Refined camera calibration processes to prefill standardized hardware information (Phase 10). - Implemented vocabulary nudges for device types and species validation (Phase 11). - Streamlined the Day Editor's interface and interaction model, ensuring clarity and ease of use (Phase 15).
1 parent 808db8f commit 6e5aae2

8 files changed

Lines changed: 109 additions & 92 deletions

‎.claude/docs/plans/v3-review-fixes/PLAN.md‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,19 +43,27 @@ For agent invocation, **load only the slice you need**:
4343
- [phase-6-type-safety.md](phase-6-type-safety.md) — derive the closed status unions (`DAY_STATUS`/`StepStatus`/`DAY_LIFECYCLE`) + a boundary guard.
4444
- [phase-7-test-ci-hardening.md](phase-7-test-ci-hardening.md) — validation/migration/divergence test gaps, e2e self-skip, CI schema-sync fail-open, guard fragility.
4545
- **Data-grounded app improvements (from the `yaml-corpus-2` study):**
46-
- [phase-8-import-robustness.md](phase-8-import-robustness.md) — fix the list-vs-scalar `orphaned_file`/`orphaned_video` bug (blocks ~63% of real-file imports) + `YYYYMMDD_<subject>` date parsing + repairable coercions. *(highest value)*
46+
- [phase-8-import-robustness.md](phase-8-import-robustness.md) — fix the list-vs-scalar `orphaned_file`/`orphaned_video` bug (blocks ~63% of real-file imports) + `YYYYMMDD_<subject>` date parsing + known legacy space-key normalization + repairable coercions. *(highest value)*
4747
- [phase-9-opto-power-guard.md](phase-9-opto-power-guard.md) — `power_in_W` range warn-to-confirm (the one unguarded silent NWB corruption) + fix the misleading placeholder.
4848
- [phase-10-camera-calibration.md](phase-10-camera-calibration.md) — `meters_per_pixel > 0`, placeholder camera names, cross-day calibration-aliasing on import (within-file aliasing already guarded).
4949
- [phase-11-vocab-nudges.md](phase-11-vocab-nudges.md) — experimenter name-shape (Spyglass), genotype-vs-strain, placeholder-id, `location` typo nudge (catalog + species already done).
5050
- [phase-12-statescript-integrity.md](phase-12-statescript-integrity.md) — `associated_files` duplicate name/path + statescript description-keyword + path-shape guards.
51-
- **UX efficiency & clarity (from the live UX walkthrough, [../../research/yaml-corpus-2/13-ux-live-walkthrough.md](../../research/yaml-corpus-2/13-ux-live-walkthrough.md)):**
51+
- **UX efficiency & clarity (from the live UX walkthrough + Day Editor mock review: [13-ux-live-walkthrough.md](../../research/yaml-corpus-2/13-ux-live-walkthrough.md), [14-day-screen-vs-mock.md](../../research/yaml-corpus-2/14-day-screen-vs-mock.md)):**
5252
- [phase-13-validation-presentation.md](phase-13-validation-presentation.md) — *(keystone; sequence BEFORE 8–12)* group/tier/collapse the Day Editor banner + reward-early/punish-late (use the existing `DRAFT` status for untouched days) + extend `humanizeValidationMessage` coverage + status-signal consistency. Makes the presentation absorb the new guards instead of becoming a ~20-row wall.
53-
- [phase-14-first-run-scope.md](phase-14-first-run-scope.md) — wizard collects `experiment_description` + pre-fills lab/institution (no first day born with errors). *(Its day-view read-only task moves into Phase 15 — trim Phase 14 to the wizard.)*
54-
- [phase-15-day-editor-ia-realign.md](phase-15-day-editor-ia-realign.md) — *(largest UX phase; the daily path)* realign the Day Editor to the finalized mock: grouped **vertical rail** (SESSION/RECORDING/FINISH), single-column sections, subject + technical params **inherited read-only**, DIO folded into RECORDING, day view scoped to the day-delta. Fixes the smushed/wrapping nav + the crammed "Day" tab at the root.
55-
56-
> **Sequencing note:** Phase 13 should land **before** the guard phases 8–12 — adding ~8–10 new issue
57-
> types onto today's flat, punish-early banner would degrade the UX even as the data gets safer. Phases
58-
> 1 is independent and can land any time; 14 is independent of the guard rules but pairs naturally with
59-
> 13. **Phase 15 is the highest-leverage UX work** (the daily screen) and depends on Phase 13's
60-
> per-section status model; it absorbs Phase 14's day-view read-only task. The original 1–7 keep their
61-
> low-risk-first order.
53+
- [phase-14-first-run-scope.md](phase-14-first-run-scope.md) — wizard collects `experiment_description` + pre-fills lab/institution (no first day born with errors).
54+
- [phase-15-day-editor-ia-realign.md](phase-15-day-editor-ia-realign.md) — *(largest UX phase; the daily path)* realign the Day Editor to the finalized mock: grouped **vertical rail** (SESSION/RECORDING/FINISH), single-column sections, subject + rig constants inherited read-only while day-owned technical fields stay editable, DIO folded into RECORDING, day view scoped to the day-delta. Fixes the smushed/wrapping nav + the crammed "Day" tab at the root.
55+
56+
## Execution waves
57+
58+
Use these waves as the source of truth for implementation order; phase numbers remain stable for review
59+
history and file references.
60+
61+
1. **Foundation and safety:** Phase 1 → Phase 6 → Phase 7 → Phase 2.
62+
2. **Core data integrity:** Phase 3 → Phase 4 → Phase 5.
63+
3. **UX readiness for heavier validation:** Phase 14 → Phase 13 → Phase 15.
64+
- Phase 13 must land before Phases 8–12 so the new guard issues do not create a flat warning wall.
65+
- Phase 15 depends on Phase 13's per-section status model and owns the Day Editor scope/IA work.
66+
4. **Corpus-driven guards:** Phase 8 → Phase 9 → Phase 10 → Phase 11 → Phase 12.
67+
- Phase 8 depends on or coordinates with Phase 4 for dual-key conflicts.
68+
- Phase 10 depends on or coordinates with Phase 4 for existing-animal catalog merge.
69+
- Phase 12 follows or coordinates with Phase 8 in `referenceRules`.

‎.claude/docs/plans/v3-review-fixes/overview.md‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -31,21 +31,24 @@ Verified against the live tree at planning time; re-confirm before editing.
3131

3232
- `src/validation/rules/referenceRules.ts:200-228` (`orphaned_file`) + `:146-196` (`orphaned_video`) — `taskEpochSet.has(epoch)` false-positives when the referencing `task_epochs` is a **list** `[2]`; `src/nwb_schema.json:617-623` declares the field scalar `integer`. **Phase 8** (the list-vs-scalar bug; ~63% of real imports). Cover both `associated_files` and `associated_video_files`, including singular-only `task_epoch` legacy video rows and mixed singular/plural video rows, because the corpus shows video epoch linkage is the largest downstream drop-risk if normalized incorrectly.
3333
- `src/state/yamlImportPlan.ts:94-121` (`extractRecordingDate`) — parses only the template's `mmddYYYY_..._metadata.yml` + `session_id` ending `_YYYYMMDD`; real `YYYYMMDD_<subject>.yml` → `null` → `planImport` dead-end. **Phase 8**. (`src/state/importRepair.ts:348-420` is the normalization seam shared with **Phase 4**.)
34+
- `src/state/importRepair.ts` — legacy files can carry schema keys with spaces (`subject id`, `electrode groups`, `ntrode electrode group channel map`, `data acq device`). Normalize only these known aliases as listed benign import repairs before validation/decision/decompose; do not rewrite arbitrary user keys. **Phase 8**.
3435
- `src/validation/rules/optoRules.ts` — `optogeneticsRules` has **no `power_in_W` magnitude check**; `src/nwb_schema.json:36042` is the field; `src/pages/AnimalEditor/OptogeneticsStep.tsx:56` has the misleading `placeholder: 'e.g. 10'`. **Phase 9**.
3536
- `src/validation/rules/identityRules.ts:56-60` — the camera-name divergence signature **already includes `meters_per_pixel`** (within-file aliasing is done). Gaps: `meters_per_pixel > 0` (`src/nwb_schema.json:716`), placeholder names, and the cross-day case at the `src/state/yamlImportApply.ts:270-340` catalog-merge seam (shared with **Phase 4**). **Phase 10**.
3637
- `src/validation/rules/dandiSubjectRules.ts:24-36` — already rejects free-text `species` (`invalid_species`); add genotype-vs-strain + placeholder-`subject_id` nudges. `src/validation/rules/identityRules.ts` — add the experimenter name-shape rule (Spyglass `decompose_name`). `src/validation/rules/electrodeGroupRules.ts:105-160` — `empty_location` + the case warning; add a typo nudge. `src/ntrode/probeCatalog.ts:75-96` already has **all 12** probes (only `CLAUDE.md`'s "Current Supported Device Types" doc is stale at 8). **Phase 11**.
3738
- `src/validation/rules/referenceRules.ts` (sibling to `orphaned_file`) — no `associated_files` *internal* checks today (duplicate name → trodes `pynwb ValueError` hard-fail; duplicate path / missing statescript description-keyword / bad path-shape → silent drop). **Phase 12**.
3839

39-
### Phases 13–14 — UX efficiency & clarity (from the live UX walkthrough)
40+
### Phases 13–15 — UX efficiency & clarity (from the live UX walkthrough + Day Editor mock review)
4041

4142
Verified against the live tree; re-confirm line numbers before editing. Full findings:
42-
[../../research/yaml-corpus-2/13-ux-live-walkthrough.md](../../research/yaml-corpus-2/13-ux-live-walkthrough.md).
43+
[../../research/yaml-corpus-2/13-ux-live-walkthrough.md](../../research/yaml-corpus-2/13-ux-live-walkthrough.md),
44+
[../../research/yaml-corpus-2/14-day-screen-vs-mock.md](../../research/yaml-corpus-2/14-day-screen-vs-mock.md).
4345

4446
- `src/pages/DayEditor/DayEditorFrame.tsx:144-157,407` — `readinessIssues` → the `ReadinessBar` banner (the surface to group/tier/collapse). `src/pages/ValidationSummary/index.tsx:200-302` already has the grouped/collapsed + `warningsAcknowledged` pattern to reuse. **Phase 13.**
4547
- `src/domain/humanizeValidationMessage.ts` (+ applied at `src/viewModels/dayEditorViewModel.ts:545`) — `HUMANIZE_LABELS` misses `must have required property '<X>'` / `must NOT have fewer than 1 items`; extend **at display**, leaving raw `issue.message` intact (a downstream parser reads it — the file says so). **Phase 13.**
4648
- `src/domain/dayLifecycle.ts:28-34` — `DAY_LIFECYCLE.DRAFT` already exists; `src/viewModels/animalWorkspaceViewModel.ts:62-170` rolls a fresh day straight to the export-gate "Needs fixing". Gate the surfacing on "touched yet?" so untouched = DRAFT. `src/pages/AnimalEditor/CamerasSection.tsx:154-158` fires the calibration warning on an empty step (same punish-early class). **Phase 13.**
4749
- `src/pages/Home/CreateAnimalWizard.tsx:108-204` (`TeamStep`) — collects `lab`/`institution` from blank seeds; `experiment_description` collected nowhere. `src/domain/animalCreation.ts:134,149,157` has the defaults that don't reach the step. **Phase 14.**
48-
- `src/pages/DayEditor/DayTab.tsx:400-433` — the inherited-subject-metadata disclosure that writes the animal record for all days behind a grey "UPDATES ALL DAYS" label; `src/pages/AnimalView/index.tsx` is the correct edit path. **Phase 14.**
50+
- `src/pages/DayEditor/DayEditorFrame.tsx` + `DayEditorFrame.module.css` + `DayEditor.scss` — the day editor is mid-refactor: a horizontal tab bar is squeezed into the old 230px rail column, sections wrap/cram, and the mock's grouped vertical rail is not restored. **Phase 15.**
51+
- `src/pages/DayEditor/DayTab.tsx:400-433` — the inherited-subject-metadata disclosure writes animal-static fields from the day view; `src/pages/AnimalView/index.tsx` is the correct edit path. `src/pages/DayEditor/DayTechnicalSection.tsx` already distinguishes rig constants (read-only inherited values) from day-owned technical fields (`default_header_file_path`, `units`). **Phase 15.**
4952
- `src/pages/AnimalEditor/ElectrodeGroupModal.tsx` (+ CSS) modal scroll; `src/pages/Home/CreateAnimalWizard.tsx:245-268` stepper + Save-draft; `src/pages/AnimalEditor/wiring/ElectrodeGroupsContainer.tsx` device-type label. **Phase 1** (pure layout/label).
5053

5154
## Scope and dependency policy
@@ -79,6 +82,7 @@ branch order or same-PR coordination:
7982
- **Phase 2 before/with Phase 9:** both can touch opto validation helpers; Phase 9 should reuse the Phase-2 helper shape if Phase 2 lands first.
8083
- **Phase 13 before Phases 8-12:** Phase 13 bounds and tiers the Day Editor validation banner so the new data-quality guards do not create an attention/working-memory wall.
8184
- **Phase 14 before or with Phase 13:** Phase 14 removes wizard-created first-day errors; Phase 13 controls when/how any remaining validation results surface.
85+
- **Phase 15 after Phase 13:** Phase 15 renders Phase 13's per-section status model in the day-editor rail and owns the Day Editor scope/IA realignment.
8286
- **Phase 7 before any deliberate schema migration:** this plan avoids schema changes; if one becomes unavoidable, the schema-sync and AJV/jsonschema divergence gates must already be hard.
8387

8488
## Metrics
@@ -108,10 +112,14 @@ branch order or same-PR coordination:
108112

109113
All-at-once per phase (no feature flags). `modern` is local-only and unpushed; these land on `modern`
110114
the same way the epoch-editor phases did (branch off `modern` → full gate → independent review →
111-
`git merge --ff-only`; do not push unless asked). Suggested order: land the original review fixes
112-
low-risk-first (`1 → 7`), land the UX-presentation prep (`14` if convenient, then **13**), then land the
113-
data-grounded guard fixes (`8 → 12`) with the handoffs above. If a later phase is pulled forward,
114-
explicitly re-check its handoff partner before merging.
115+
`git merge --ff-only`; do not push unless asked). Suggested wave order:
116+
117+
1. **Foundation and safety:** `1 → 6 → 7 → 2`.
118+
2. **Core data integrity:** `3 → 4 → 5`.
119+
3. **UX readiness for heavier validation:** `14 → 13 → 15`.
120+
4. **Corpus-driven guards:** `8 → 9 → 10 → 11 → 12`.
121+
122+
If a later phase is pulled forward, explicitly re-check its handoff partner before merging.
115123

116124
## Open Questions
117125

@@ -149,10 +157,9 @@ explicitly re-check its handoff partner before merging.
149157
recommend **warning** (blocking risks rejecting an unusual-but-real name). (b) Active-scoping the
150158
`empty_location` error so it doesn't fire on deliberately-unused/all-bad tetrodes needs a first-class
151159
**disabled-group** concept — recommend **deferring** it (its own phase) rather than scoping it here.
152-
8. **`screw` / `single_electrode` (66 files / 17 animals).** A real non-probe electrode class trodes
153-
can't resolve (FileNotFoundError) and the app can't express. Genuine product decision: add a
154-
first-class non-probe electrode path (needs pipeline coordination) vs. document as unsupported.
155-
**Not** a Phase-11 catalog edit. Track here; decide before building.
160+
8. **`screw` / `single_electrode` (66 files / 17 animals).** Product decision for this plan: treat as
161+
out-of-scope / one-off for now. Do not add a catalog entry or non-probe electrode workflow in v3
162+
review fixes; revisit only if new users hit it again.
156163
9. **Associated-video rows with no epoch key.** The corpus has real `associated_video_files` rows without
157164
either `task_epoch` or `task_epochs`. Phase 8 must preserve/flag known singular/plural forms, but the
158165
no-key shape needs a product decision before gating: is it a valid videoless/Guidera workflow marker,
@@ -164,4 +171,5 @@ Small per phase. Rough diff sizing: Phase 1 ~150 LOC across 6 files (+ test upda
164171
Phase 3 ~120 LOC (the only non-trivial one — depends on Q1); Phase 4 ~100 LOC; Phase 5 ~60 LOC; Phase 6
165172
~60 LOC (mechanical, multi-file); Phase 7 ~250 LOC (mostly new tests). Data-grounded phases: Phase 8
166173
~160 LOC plus import fixtures; Phase 9 ~40 LOC; Phase 10 ~120 LOC; Phase 11 ~100 LOC; Phase 12 ~90 LOC.
167-
UX phases: Phase 13 ~180 LOC plus focused VM/component tests; Phase 14 ~120 LOC plus wizard/day-scope tests.
174+
UX phases: Phase 13 ~180 LOC plus focused VM/component tests; Phase 14 ~70 LOC plus wizard tests;
175+
Phase 15 ~250 LOC plus Day Editor component/e2e coverage.

‎.claude/docs/plans/v3-review-fixes/phase-1-hygiene.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,13 +78,13 @@ the *validation-timing* walkthrough findings (empty-Cameras warning, pre-greened
7878
- **Save-draft feedback (Med).** Show a brief inline "Draft saved" confirmation next to the wizard's
7979
"Save draft" button on click (a transient `role="status"` message), so the user knows work persisted.
8080
- **Duplicate "Add recording day" at zero days (Med).** The zero-days state renders the action twice — a
81-
header button ([RecordingDaysTab.tsx:409-415](../../../src/pages/AnimalWorkspace/RecordingDaysTab.tsx))
82-
**and** the EmptyState CTA ([DayList.tsx:78-92](../../../src/pages/AnimalWorkspace/DayList.tsx)), both
81+
header button ([RecordingDaysTab.tsx:409-415](../../../../src/pages/AnimalWorkspace/RecordingDaysTab.tsx))
82+
**and** the EmptyState CTA ([DayList.tsx:78-92](../../../../src/pages/AnimalWorkspace/DayList.tsx)), both
8383
opening the same calendar. Render only one in the empty state (keep the EmptyState CTA, hide/suppress the
8484
header button when there are zero days), so a new user sees a single clear call to action.
8585
- **Persistent "set up this animal" card (Med).** A fully-configured animal with zero recording days still
8686
shows the setup card because `showSetupCard = !(subjectPresent && dayCount > 0)`
87-
([animalWorkspaceViewModel.ts:546](../../../src/viewModels/animalWorkspaceViewModel.ts)) gates on
87+
([animalWorkspaceViewModel.ts:546](../../../../src/viewModels/animalWorkspaceViewModel.ts)) gates on
8888
*day-count*, not setup completeness. Gate it on actual section completeness (the same section-status the
8989
AnimalView SectionNav already computes) so a set-up animal with no days yet is invited to **add a day**,
9090
not to "set up the animal" again.

‎.claude/docs/plans/v3-review-fixes/phase-10-camera-calibration.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ camera to day-2's).
3939
silent overwrite of the existing calibration. (The within-file rule already covers the single-file case;
4040
this is the across-days extension.)
4141
- **Prefill the standardized camera hardware.** Today `CameraModal` seeds every field blank
42-
([src/pages/AnimalEditor/CameraModal.tsx:49-53](../../../src/pages/AnimalEditor/CameraModal.tsx)) — the
42+
([src/pages/AnimalEditor/CameraModal.tsx:49-53](../../../../src/pages/AnimalEditor/CameraModal.tsx)) — the
4343
cause of the corpus's `unknown`/`unknown2` make/model/lens pollution. The lab is overwhelmingly one rig:
4444
manufacturer **Allied Vision** (Manta), model **Manta G-158C**, lens **Theia SL183M** (corpus: G-158C
4545
448 / Theia SL183M 448, dominant when not left blank). Prefill those three on a **new** camera

‎.claude/docs/plans/v3-review-fixes/phase-11-vocab-nudges.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ Evidence: [../../research/yaml-corpus-2/06-spyglass-constraints.md](../../resear
4343

4444
- Widening / changing the **device_type catalog** — it already has all 12 probes and is validated; the
4545
only stale artifact is the CLAUDE.md doc (fixed above). The `screw`/`single_electrode` hardware class is
46-
a genuine product decision — see Open Question, not a catalog edit.
46+
intentionally ignored for now as a one-off/out-of-scope workflow, not a catalog edit.
4747
- The **species** guard — already implemented (`invalid_species`).
4848
- The task-catalog name collision — **Phase 3**.
4949
- **Active-scoping the `empty_location` error** (so it doesn't fire on deliberately-unused/all-bad

0 commit comments

Comments
 (0)