Skip to content

Commit b2b8d68

Browse files
edenoclaude
andcommitted
refactor(reconfig): apply Phase 9 review findings
code-reviewer / pr-test-analyzer / type-design-analyzer / ux-reviewer: Correctness: - applyConfigurationForward ignores day ids not in the workspace (no phantom entries leaking into appliedToDays); dedups and updates only real days. - resolveDayConfig guards a snapshot missing its `devices` (closes a latent crash). A11y / UX (ReconfigWizard): - role="alertdialog" + aria-describedby for this consequential multi-day action. - Clear the validation error as the user edits; aria-describedby links the error. - Relabel the action "Apply to N days" (shows the count); add a summary line and a day-selection hint explaining the default scope (this day onward). - Humanize changed electrode-group field names (e.g. location -> brain region). - Cancel button labeled "Cancel and close"; render the wizard as a sibling of the version bar (not nested in the flex layout); remount per day/version (fresh state). - Fix WCAG-AA contrast on the no-change italic text (#777 -> #666). - Guard empty snapshot description/date in the DevicesStep version bar. Types/contracts: - New ProbeConfigDiff typedef; document the appliedToDays partition invariant and mark it a denormalized cache (authoritative source is Day.configurationVersion). - updateDay notes that setting configurationVersion bypasses appliedToDays sync. - DevicesStep actions PropTypes tightened to a shape; both components use destructuring defaults instead of defaultProps. Tests: add ntrode-removed, combined map+bad-channels, empty/undefined-config diff, and phantom-day-id cases. Deferred items recorded in overview.md. Baselines 125/125; full suite 2961 passed / 1 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 71030b1 commit b2b8d68

11 files changed

Lines changed: 220 additions & 71 deletions

File tree

‎.claude/docs/plans/v3-workspace-cutover/overview.md‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,25 @@ not executed here.
210210
failures via `console.error` (the app has no Sentry/structured-logging infra today). Routing these
211211
through a real logging path with error IDs is a cross-app concern, out of scope for the summary.
212212

213+
- **Phase 9 review items deferred (out of scope for the wizard phase).** The Phase 9
214+
reviewers surfaced three follow-ups left for later, all low practical risk:
215+
1. **Cross-action atomicity of versioning.** The wizard calls `addConfigurationSnapshot`
216+
then `applyConfigurationForward` as two sequential store actions, predicting the new
217+
version as `configurationHistory.length + 1`. The calls are synchronous on the current
218+
animal, so the version is correct in practice; but if the second action ever threw, an
219+
orphan empty snapshot would remain. **Follow-up:** have `addConfigurationSnapshot`
220+
return the created version (or add a combined create-and-apply action) so the two can't
221+
desynchronize. Out of scope here (the plan deliberately kept creation and assignment
222+
as separate actions).
223+
2. **`appliedToDays` as a derived value.** It is currently a denormalized cache kept in
224+
sync by `applyConfigurationForward`, with `reconcileAppliedToDays` providing the
225+
trustworthy derived view (`updateDay({configurationVersion})` bypasses the stored
226+
lists). **Follow-up:** consider dropping the stored field entirely and always deriving
227+
it, removing the partition-maintenance burden. A data-model change, out of scope here.
228+
3. **Wizard UX niceties for long studies.** Select-all/deselect-all controls and
229+
relative/human-readable day labels for animals with 60–200+ days, and an explicit
230+
success confirmation. Deferred to the [Phase 10](phase-10-a11y-keyboard.md) pass.
231+
213232
## Plan revisions
214233

215234
- **2026-06-03 — parity model corrected; a byte-parity phase inserted after export.** Pre-Phase-5

‎src/pages/DayEditor/DevicesStep.jsx‎

Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import './DayEditor.scss';
3030
* `applyConfigurationForward`); when provided, the reconfiguration wizard is available.
3131
* @returns {JSX.Element}
3232
*/
33-
export default function DevicesStep({ animal, day, mergedDay, onFieldUpdate, animalDays, actions }) {
33+
export default function DevicesStep({ animal, day, mergedDay, onFieldUpdate, animalDays = undefined, actions = undefined }) {
3434
const electrodeGroups = animal.devices?.electrode_groups || [];
3535
const [wizardOpen, setWizardOpen] = useState(false);
3636

@@ -215,26 +215,34 @@ export default function DevicesStep({ animal, day, mergedDay, onFieldUpdate, ani
215215
<a href={`#/animal/${animal.id}/editor`}>Edit at Animal Level</a>
216216
</div>
217217

218-
{/* Configuration-version indicator + reconfiguration entry point */}
218+
{/* Configuration-version indicator + reconfiguration entry point. The wizard
219+
is a sibling of the bar (not nested inside the flex layout div) so the
220+
dialog is not a descendant of a layout container. */}
219221
{reconfig && (
220-
<div className="config-version-bar">
221-
<div className="config-version-info">
222-
<span className="config-version-label">
223-
Configuration version {reconfig.version}
224-
{reconfig.snapshot ? `: ${reconfig.snapshot.description} (${reconfig.snapshot.date})` : ''}
225-
</span>
226-
<span className="config-version-applied">
227-
Applied to {reconfig.appliedCount} {reconfig.appliedCount === 1 ? 'day' : 'days'}
228-
</span>
222+
<>
223+
<div className="config-version-bar">
224+
<div className="config-version-info">
225+
<span className="config-version-label">
226+
Configuration version {reconfig.version}
227+
{reconfig.snapshot
228+
? `: ${reconfig.snapshot.description || 'No description'} (${reconfig.snapshot.date || 'date unknown'})`
229+
: ''}
230+
</span>
231+
<span className="config-version-applied">
232+
Applied to {reconfig.appliedCount} {reconfig.appliedCount === 1 ? 'day' : 'days'}
233+
</span>
234+
</div>
235+
<button
236+
type="button"
237+
className="config-reconfig-button"
238+
onClick={() => setWizardOpen(true)}
239+
>
240+
Reconfigure devices…
241+
</button>
229242
</div>
230-
<button
231-
type="button"
232-
className="config-reconfig-button"
233-
onClick={() => setWizardOpen(true)}
234-
>
235-
Reconfigure devices…
236-
</button>
237243
<ReconfigWizard
244+
// Remount per day/version so reopening shows fresh form state.
245+
key={`${day.id}-${reconfig.version}`}
238246
isOpen={wizardOpen}
239247
onClose={() => setWizardOpen(false)}
240248
animal={animal}
@@ -243,7 +251,7 @@ export default function DevicesStep({ animal, day, mergedDay, onFieldUpdate, ani
243251
candidateDays={reconfig.candidateDays}
244252
actions={actions}
245253
/>
246-
</div>
254+
</>
247255
)}
248256

249257
{/* Electrode groups (accordion) */}
@@ -371,11 +379,12 @@ DevicesStep.propTypes = {
371379
}).isRequired,
372380
mergedDay: PropTypes.object.isRequired,
373381
onFieldUpdate: PropTypes.func.isRequired,
382+
// animalDays + actions are supplied together by DayEditorStepper to enable the
383+
// configuration-version indicator and reconfiguration wizard; omitting both (e.g.
384+
// in isolated unit renders) simply hides that section.
374385
animalDays: PropTypes.arrayOf(PropTypes.object),
375-
actions: PropTypes.object,
376-
};
377-
378-
DevicesStep.defaultProps = {
379-
animalDays: undefined,
380-
actions: undefined,
386+
actions: PropTypes.shape({
387+
addConfigurationSnapshot: PropTypes.func,
388+
applyConfigurationForward: PropTypes.func,
389+
}),
381390
};

‎src/pages/DayEditor/ReconfigWizard.jsx‎

Lines changed: 70 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,24 @@ import { resolveDayConfig } from '../../state/workspaceUtils';
55
import { diffProbeConfigs } from '../../state/configDiff';
66
import './ReconfigWizard.scss';
77

8+
/**
9+
* Human-readable labels for the electrode-group fields a diff may flag as changed,
10+
* so a scientist sees "brain region" rather than the raw `location` key. Unmapped
11+
* keys fall through to the raw name.
12+
*/
13+
const GROUP_FIELD_LABELS = {
14+
location: 'brain region',
15+
device_type: 'probe type',
16+
description: 'description',
17+
targeted_location: 'targeted region',
18+
targeted_x: 'ML coordinate',
19+
targeted_y: 'AP coordinate',
20+
targeted_z: 'DV coordinate',
21+
units: 'coordinate units',
22+
};
23+
24+
const labelField = (field) => GROUP_FIELD_LABELS[field] || field;
25+
826
/**
927
* Probe-reconfiguration wizard.
1028
*
@@ -20,17 +38,27 @@ import './ReconfigWizard.scss';
2038
*
2139
* @param {object} props
2240
* @param {boolean} props.isOpen - Whether the dialog is shown.
23-
* @param {Function} props.onClose - Called on cancel/ESC/overlay and after a successful apply.
41+
* @param {Function} props.onClose - Called (no args) on cancel/ESC/overlay and after a successful apply.
2442
* @param {object} props.animal - Animal whose live `devices` form the "next" config.
2543
* @param {object} props.day - The day being reconfigured (the default earliest applied day).
26-
* @param {object|null} props.prevDay - The chronologically previous day, or null.
44+
* @param {object|null} [props.prevDay] - The chronologically previous day, or null.
2745
* @param {object[]} props.candidateDays - This day and all chronologically later days (apply-forward set).
2846
* @param {object} props.actions - Store actions: `addConfigurationSnapshot`, `applyConfigurationForward`.
2947
* @returns {JSX.Element|null}
3048
*/
31-
export default function ReconfigWizard({ isOpen, onClose, animal, day, prevDay, candidateDays, actions }) {
49+
export default function ReconfigWizard({
50+
isOpen,
51+
onClose,
52+
animal,
53+
day,
54+
prevDay = null,
55+
candidateDays,
56+
actions,
57+
}) {
3258
const baseId = useId();
3359
const titleId = `${baseId}-title`;
60+
const summaryId = `${baseId}-summary`;
61+
const errorId = `${baseId}-error`;
3462

3563
const nextConfig = useMemo(
3664
() => ({
@@ -51,6 +79,7 @@ export default function ReconfigWizard({ isOpen, onClose, animal, day, prevDay,
5179
const [error, setError] = useState('');
5280

5381
const toggleDay = (id) => {
82+
if (error) setError('');
5483
setSelectedIds((prev) => {
5584
const next = new Set(prev);
5685
if (next.has(id)) next.delete(id);
@@ -71,7 +100,9 @@ export default function ReconfigWizard({ isOpen, onClose, animal, day, prevDay,
71100
}
72101

73102
// Version the change, then assign it forward. The new snapshot takes the next
74-
// sequential version (matching addConfigurationSnapshot's own numbering).
103+
// sequential version (matching addConfigurationSnapshot's own numbering). These
104+
// two calls are synchronous and read the current animal, so the version is
105+
// correct; see the cross-action atomicity follow-up in the v3 plan overview.
75106
const newVersion = (animal.configurationHistory?.length || 0) + 1;
76107
actions.addConfigurationSnapshot(animal.id, {
77108
date,
@@ -82,22 +113,35 @@ export default function ReconfigWizard({ isOpen, onClose, animal, day, prevDay,
82113
const orderedIds = candidateDays.map((d) => d.id).filter((id) => selectedIds.has(id));
83114
actions.applyConfigurationForward(animal.id, newVersion, orderedIds);
84115

85-
onClose({ applied: true, version: newVersion, dayIds: orderedIds });
116+
onClose();
86117
};
87118

88119
if (!isOpen) return null;
89120

90121
return (
91-
<Modal isOpen={isOpen} onClose={onClose} title="Reconfigure devices" titleId={titleId} className="reconfig-wizard">
122+
<Modal
123+
isOpen={isOpen}
124+
onClose={onClose}
125+
title="Reconfigure devices"
126+
titleId={titleId}
127+
// A consequential action (reassigns configuration across many recording days).
128+
role="alertdialog"
129+
describedById={diff.hasChanges ? summaryId : 'reconfig-no-change'}
130+
className="reconfig-wizard"
131+
>
92132
{!diff.hasChanges ? (
93-
<p className="reconfig-no-change" data-testid="reconfig-no-change">
133+
<p id="reconfig-no-change" className="reconfig-no-change" data-testid="reconfig-no-change">
94134
No configuration change detected between{' '}
95135
{prevDay ? `${prevDay.date}` : 'the baseline'} and the current animal
96136
configuration. Edit the electrode groups in the Animal Editor first, then
97137
reopen this wizard to version the change.
98138
</p>
99139
) : (
100140
<div className="reconfig-diff">
141+
<p id={summaryId} className="reconfig-summary">
142+
This versions the current device configuration and applies it to the days
143+
you select below. Days before this one are not affected. Review the changes:
144+
</p>
101145
<ConfigDiffView diff={diff} />
102146
</div>
103147
)}
@@ -114,9 +158,13 @@ export default function ReconfigWizard({ isOpen, onClose, animal, day, prevDay,
114158
<input
115159
type="text"
116160
value={description}
117-
onChange={(e) => setDescription(e.target.value)}
161+
onChange={(e) => {
162+
if (error) setError('');
163+
setDescription(e.target.value);
164+
}}
118165
placeholder="e.g. Lowered CA1 tetrodes by 40 µm"
119166
disabled={!diff.hasChanges}
167+
aria-describedby={error ? errorId : undefined}
120168
/>
121169
</label>
122170

@@ -132,6 +180,10 @@ export default function ReconfigWizard({ isOpen, onClose, animal, day, prevDay,
132180

133181
<fieldset className="reconfig-days" disabled={!diff.hasChanges}>
134182
<legend>Apply to days</legend>
183+
<p className="reconfig-days-hint">
184+
All days from {day.date} onward are selected by default. To change an
185+
earlier day, open its Day Editor individually.
186+
</p>
135187
{candidateDays.map((d) => (
136188
<label key={d.id} className="reconfig-day-option">
137189
<input
@@ -145,17 +197,22 @@ export default function ReconfigWizard({ isOpen, onClose, animal, day, prevDay,
145197
</fieldset>
146198

147199
{error && (
148-
<p className="reconfig-error" role="alert">
200+
<p id={errorId} className="reconfig-error" role="alert">
149201
{error}
150202
</p>
151203
)}
152204

153205
<div className="form-actions">
154-
<button type="button" className="btn-cancel" onClick={onClose}>
206+
<button
207+
type="button"
208+
className="btn-cancel"
209+
onClick={() => onClose()}
210+
aria-label="Cancel and close"
211+
>
155212
Cancel
156213
</button>
157214
<button type="submit" className="btn-primary" disabled={!diff.hasChanges}>
158-
Apply forward
215+
Apply to {selectedIds.size} {selectedIds.size === 1 ? 'day' : 'days'}
159216
</button>
160217
</div>
161218
</form>
@@ -176,15 +233,11 @@ ReconfigWizard.propTypes = {
176233
}).isRequired,
177234
};
178235

179-
ReconfigWizard.defaultProps = {
180-
prevDay: null,
181-
};
182-
183236
/**
184237
* Read-only rendering of a {@link diffProbeConfigs} result.
185238
*
186239
* @param {object} props
187-
* @param {object} props.diff - The structured diff.
240+
* @param {import('../../state/workspaceTypes').ProbeConfigDiff} props.diff - The structured diff.
188241
* @returns {JSX.Element}
189242
*/
190243
function ConfigDiffView({ diff }) {
@@ -208,7 +261,7 @@ function ConfigDiffView({ diff }) {
208261
))}
209262
{electrodeGroups.changed.map((c) => (
210263
<p key={`c-${c.id}`} className="diff-changed">
211-
~ Group {c.id} changed: {c.fields.join(', ')}
264+
~ Group {c.id} changed: {c.fields.map(labelField).join(', ')}
212265
</p>
213266
))}
214267
</section>

‎src/pages/DayEditor/ReconfigWizard.scss‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,17 @@
88
padding: 0.75rem 1rem;
99
}
1010

11+
.reconfig-summary {
12+
margin: 0 0 0.75rem;
13+
color: #333;
14+
}
15+
16+
.reconfig-days-hint {
17+
margin: 0 0 0.5rem;
18+
font-size: 0.8rem;
19+
color: #555;
20+
}
21+
1122
.config-diff {
1223
section {
1324
margin-bottom: 1rem;
@@ -37,7 +48,8 @@
3748
}
3849

3950
.diff-none {
40-
color: #777;
51+
/* #666 keeps AA contrast (≥4.5:1) for this small italic text on white. */
52+
color: #666;
4153
font-style: italic;
4254
font-family: inherit;
4355
}

‎src/pages/DayEditor/__tests__/ReconfigWizard.test.jsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ describe('ReconfigWizard [integration]', () => {
4444
expect(screen.getByText(/Added ntrode 3/)).toBeInTheDocument();
4545

4646
await user.type(screen.getByLabelText(/change description/i), 'Lowered CA1 tetrodes');
47-
await user.click(screen.getByRole('button', { name: /apply forward/i }));
47+
await user.click(screen.getByRole('button', { name: /apply to/i }));
4848

4949
// Creates the snapshot from the live (v2) config...
5050
expect(actions.addConfigurationSnapshot).toHaveBeenCalledTimes(1);
@@ -77,7 +77,7 @@ describe('ReconfigWizard [integration]', () => {
7777
);
7878

7979
expect(screen.getByTestId('reconfig-no-change')).toBeInTheDocument();
80-
expect(screen.getByRole('button', { name: /apply forward/i })).toBeDisabled();
80+
expect(screen.getByRole('button', { name: /apply to/i })).toBeDisabled();
8181
});
8282
});
8383

‎src/state/__tests__/applyConfigurationForward.test.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,21 @@ describe('applyConfigurationForward', () => {
101101
expect(JSON.stringify(result.current.model.workspace)).toBe(before);
102102
});
103103

104+
it('ignores day ids not in the workspace (no phantom entries in appliedToDays)', () => {
105+
const { result, animalId, dayIds } = renderStore();
106+
107+
act(() => {
108+
result.current.actions.applyConfigurationForward(animalId, 1, [dayIds.day3, 'no-such-day']);
109+
});
110+
111+
const v1 = snapshot(result, animalId, 1);
112+
expect(v1.appliedToDays).not.toContain('no-such-day');
113+
expect(v1.appliedToDays).toContain(dayIds.day3);
114+
expect(daysOf(result)[dayIds.day3].configurationVersion).toBe(1);
115+
// The phantom id created no day entry.
116+
expect(daysOf(result)['no-such-day']).toBeUndefined();
117+
});
118+
104119
it('throws on an unknown animal id', () => {
105120
const { result, dayIds } = renderStore();
106121

0 commit comments

Comments
 (0)