From 259254e4fb8cc37e794ebc84b71a4f1e906b0ab1 Mon Sep 17 00:00:00 2001 From: Michael B Reiser Date: Wed, 27 May 2026 19:57:22 -0400 Subject: [PATCH] =?UTF-8?q?fix(v3):=20Phase=204=20cleanup=20=E2=80=94=20li?= =?UTF-8?q?stener=20accumulation,=20selection=20drift,=20ruler=20ticks,=20?= =?UTF-8?q?convert=20=5FunknownKeys=20(v0.9)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six Codex-flagged bugs in landed Phase 4 work: - A1: #sequenceList drag/drop listeners moved to one-time startup wiring. Previously re-attached on every renderSequence(); a single drop after N renders fired N handlers, inserting N duplicate refs and pushing N undo entries. Surfaced within a normal editing session. - A2: onMoveSequenceEntry and onInsertSequenceRefFromLib now walk selection.index across the full displacement, not just the moved entry. Library drag-drop no longer steals selection focus — it shifts the existing selection through the insert (matches the +Add Ref button's intent of selecting the new entry only for explicit button clicks). - A3: Timeline ruler tick positions are computed piecewise by walking the layout array (rulerXForRealTime), so ticks stay aligned with clamped step widths. Flat pxPerSec drifted as soon as any short step clamped to the 60px / 40px minimum. - A4: isConvertibleBlock rejects entries with non-empty _unknownKeys. Block→ref convert was silently dropping forward-compat fields like retry_on_fail, undermining the Tier 1 unknown-passthrough guarantee. The user-facing error message now enumerates every blocker including forward-compat keys. - A5: _buildSequenceEntry mirrors the parser's positive-integer repetitions validation. Closed a doc/JS-mirror divergence path that D4/paste-import would hit (entry.repetitions = 0 → YAML '0' but mirror = 1 via `|| 1` default). - A6: onMoveCommand defensively early-returns on no-op / out-of-bounds, matching the onMoveSequenceEntry pattern. Avoids pushUndo pollution. Tests: 375/375 (was 369). Added 6 new builder-side validation cases to Suite 10b for A5. Verified in browser: - A1: 30 forced re-renders + 1 simulated library drop adds exactly 1 entry. - A4: Right-click on future_keys block shows full error listing all blockers including `forward-compat keys: retry_on_fail, abort_if`. - A3: Multi_block fixture (mixed durations exercising 60px clamping) shows "1m" tick at x=2447 of a 3516-px ruler — piecewise positioning. Footer bumped to v0.9 | 2026-05-27 19:54 ET. Co-Authored-By: Claude Opus 4.7 (1M context) --- experiment_designer_v3.html | 146 ++++++++++++++++++++-------- js/protocol-yaml-v3.js | 15 ++- tests/test-protocol-roundtrip-v3.js | 32 ++++++ 3 files changed, 151 insertions(+), 42 deletions(-) diff --git a/experiment_designer_v3.html b/experiment_designer_v3.html index 755d672..3b219eb 100644 --- a/experiment_designer_v3.html +++ b/experiment_designer_v3.html @@ -978,7 +978,7 @@

v3 Experiment Designer

Reiser Lab | - v3 Experiment Designer v0.8 | 2026-05-27 17:28 ET + v3 Experiment Designer v0.9 | 2026-05-27 19:54 ET
@@ -1473,15 +1473,55 @@

Import error

$('addRefBtn').addEventListener('click', onAddSeqRef); $('addBlockBtn').addEventListener('click', onAddSeqBlock); + // List-level drop target for the empty-list / append-past-end case. + // Wired ONCE at startup (not per renderSequence) — the #sequenceList + // element persists across renders, so re-attaching here would stack N + // handlers and cause N duplicate inserts per drop. + (function wireSequenceListDrop() { + const list = $('sequenceList'); + list.addEventListener('dragover', (e) => { + if (!e.dataTransfer.types.includes('text/x-library-cond')) return; + // Only highlight when we're not over an existing entry (entries handle their own) + if (e.target.closest('.seq-entry')) return; + e.preventDefault(); + e.dataTransfer.dropEffect = 'copy'; + list.classList.add('lib-drop-target'); + }); + list.addEventListener('dragleave', (e) => { + if (e.target === list) list.classList.remove('lib-drop-target'); + }); + list.addEventListener('drop', (e) => { + if (e.target.closest('.seq-entry')) return; // handled by entry's own drop + const libCond = e.dataTransfer.getData('text/x-library-cond'); + if (!libCond) return; + e.preventDefault(); + list.classList.remove('lib-drop-target'); + if (!experiment) return; + onInsertSequenceRefFromLib(libCond, experiment.sequence.length); + }); + })(); + function onMoveSequenceEntry(fromIdx, toIdx) { if (fromIdx === toIdx) return; pushUndo(); try { docMoveSequenceEntry(experiment, fromIdx, toIdx); setDirty(true); - // Update selection if it was on the moved entry - if (selection && (selection.kind === 'ref' || selection.kind === 'block') && selection.index === fromIdx) { - selection = { ...selection, index: toIdx }; + // Walk selection.index across the displacement. Three cases: + // 1. The moved entry was selected → follow it to toIdx. + // 2. Selection sits in the range that shifts left because the + // moved entry crossed it going down (fromIdx < sel <= toIdx). + // 3. Selection sits in the range that shifts right because the + // moved entry crossed it going up (toIdx <= sel < fromIdx). + if (selection && (selection.kind === 'ref' || selection.kind === 'block')) { + const sel = selection.index; + if (sel === fromIdx) { + selection = { ...selection, index: toIdx }; + } else if (fromIdx < sel && sel <= toIdx) { + selection = { ...selection, index: sel - 1 }; + } else if (toIdx <= sel && sel < fromIdx) { + selection = { ...selection, index: sel + 1 }; + } } renderAll(); } catch (err) { @@ -1494,8 +1534,14 @@

Import error

try { docInsertSequenceEntry(experiment, atIdx, { kind: 'ref', condition_name: condName }); setDirty(true); - const clamped = Math.max(0, Math.min(atIdx, experiment.sequence.length - 1)); - selection = { kind: 'ref', index: clamped }; + // Walk existing selection through the insert: any entry at or + // after atIdx shifts +1. Drag-drop from the library should not + // steal focus from whatever the user had selected, so we DON'T + // overwrite selection to point to the new entry (unlike the + // explicit "+ Add ref" button, which intentionally selects it). + if (selection && (selection.kind === 'ref' || selection.kind === 'block') && atIdx <= selection.index) { + selection = { ...selection, index: selection.index + 1 }; + } renderAll(); } catch (err) { showError('Add ref failed', err.message); @@ -1538,16 +1584,17 @@

Import error

// Right-click handler — convert a sequence entry between ref and // single-trial block. A block is "convertible" to a ref only when it - // has exactly 1 trial, 1 rep, no randomize, and no intertrial (i.e., - // it's semantically equivalent to a bare ref). Non-convertible blocks - // show a soft error instead. + // has exactly 1 trial, 1 rep, no randomize, no intertrial, AND no + // forward-compat unknown keys (which would be silently dropped by the + // replace path, undermining the Tier 1 unknown-passthrough guarantee). function isConvertibleBlock(entry) { return entry && entry.kind === 'block' && entry.trials.length === 1 && (entry.repetitions || 1) === 1 && !entry.randomize && - !entry.intertrial; + !entry.intertrial && + Object.keys(entry._unknownKeys || {}).length === 0; } function onConvertSequenceEntry(idx) { @@ -1571,12 +1618,17 @@

Import error

} } else if (entry.kind === 'block') { if (!isConvertibleBlock(entry)) { + const unknownKeys = Object.keys(entry._unknownKeys || {}); + const reasons = []; + if (entry.trials.length !== 1) reasons.push(entry.trials.length + ' trial(s)'); + if ((entry.repetitions || 1) !== 1) reasons.push((entry.repetitions || 1) + ' rep(s)'); + if (entry.randomize) reasons.push('randomize=on'); + if (entry.intertrial) reasons.push('intertrial=' + entry.intertrial); + if (unknownKeys.length > 0) reasons.push('forward-compat keys: ' + unknownKeys.join(', ')); showError( 'Cannot convert block to ref', - 'Only blocks with exactly 1 trial, 1 repetition, no randomize, and no intertrial can convert to a bare ref. ' + - 'This block has ' + entry.trials.length + ' trial(s), ' + - (entry.repetitions || 1) + ' rep(s), randomize=' + (entry.randomize ? 'on' : 'off') + ', ' + - (entry.intertrial ? 'intertrial=' + entry.intertrial : 'no intertrial') + '.' + 'Only blocks with exactly 1 trial, 1 repetition, no randomize, no intertrial, and no forward-compat keys can convert to a bare ref. ' + + 'This block has ' + reasons.join(', ') + '.' ); return; } @@ -1805,26 +1857,10 @@

Import error

}); }; - // List-level drop target for the empty-list / append-past-end case. - list.addEventListener('dragover', (e) => { - if (!e.dataTransfer.types.includes('text/x-library-cond')) return; - // Only highlight when we're not over an existing entry (entries handle their own) - if (e.target.closest('.seq-entry')) return; - e.preventDefault(); - e.dataTransfer.dropEffect = 'copy'; - list.classList.add('lib-drop-target'); - }); - list.addEventListener('dragleave', (e) => { - if (e.target === list) list.classList.remove('lib-drop-target'); - }); - list.addEventListener('drop', (e) => { - if (e.target.closest('.seq-entry')) return; // handled by entry's own drop - const libCond = e.dataTransfer.getData('text/x-library-cond'); - if (!libCond) return; - e.preventDefault(); - list.classList.remove('lib-drop-target'); - onInsertSequenceRefFromLib(libCond, experiment.sequence.length); - }); + // List-level drop target listeners live on #sequenceList itself — + // they're wired once at startup (see below), not per-render, because + // innerHTML='' clears children but leaves listeners on the list + // element. Re-binding here would stack N handlers per render. experiment.sequence.forEach((entry, idx) => { const isSelected = selection && ( @@ -2573,6 +2609,15 @@

Import error

} function onMoveCommand(condIdx, fromIdx, toIdx) { + // Defensive: docMoveCommand silently no-ops on out-of-bounds / + // identity moves. Without this early-return, pushUndo() would + // record a snapshot for a move that never happened. The up/down + // arrow buttons disable at the edges so this is mostly belt-and- + // suspenders, but matches the onMoveSequenceEntry pattern. + if (fromIdx === toIdx) return; + const cond = experiment.conditions[condIdx]; + if (!cond || fromIdx < 0 || fromIdx >= cond.commands.length || + toIdx < 0 || toIdx >= cond.commands.length) return; pushUndo(); try { docMoveCommand(experiment, condIdx, fromIdx, toIdx); @@ -3180,23 +3225,42 @@

Import error

} const totalWidthPx = cumulativeWidth; - // The ruler maps real time → pixel position using the *actual* step - // strip width (so ticks stay aligned even when min-width clamping - // stretches short steps). 10s tick interval; label every minute - // for <15min totals, every 5min for longer. - const realTotalSec = steps.reduce((s, x) => s + (x.dur || 0), 0) || 1; - const pxPerSecRuler = totalWidthPx / realTotalSec; + // The ruler maps real time → pixel position by walking the layout + // array piecewise. A flat pxPerSec rate would drift as soon as any + // step's pixel width clamped to the minimum, since cumulative pixel + // position would no longer be a linear function of cumulative real + // time. 10s tick interval; label every minute for <15min totals, + // every 5min for longer. + const realTotalSec = layout.reduce((s, x) => s + (x.step.dur || 0), 0); const tickIntervalSec = 10; const labelIntervalSec = realTotalSec < 15 * 60 ? 60 : 5 * 60; const rulerHeight = 18; + // Map real-time t → pixel position. Find the layout entry whose + // real-time range contains t and interpolate proportionally within + // that entry's pixel range. Steps with dur === 0 use their start. + const rulerXForRealTime = (t) => { + let acc = 0; + for (let i = 0; i < layout.length; i++) { + const lay = layout[i]; + const stepDur = lay.step.dur || 0; + const isLast = i === layout.length - 1; + if (t <= acc + stepDur || isLast) { + const within = stepDur > 0 ? (t - acc) / stepDur : 0; + return lay.startPx + Math.max(0, Math.min(1, within)) * (lay.endPx - lay.startPx); + } + acc += stepDur; + } + return totalWidthPx; + }; + const NS = 'http://www.w3.org/2000/svg'; const ruler = document.createElementNS(NS, 'svg'); ruler.setAttribute('class', 'timeline-ruler'); ruler.setAttribute('width', String(totalWidthPx)); ruler.setAttribute('height', String(rulerHeight)); for (let t = 0; t <= realTotalSec; t += tickIntervalSec) { - const x = t * pxPerSecRuler; + const x = rulerXForRealTime(t); const isMajor = (t % labelIntervalSec) === 0 && t > 0; const tick = document.createElementNS(NS, 'line'); tick.setAttribute('class', 'tick' + (isMajor ? ' major' : '')); diff --git a/js/protocol-yaml-v3.js b/js/protocol-yaml-v3.js index c2f9293..1108029 100644 --- a/js/protocol-yaml-v3.js +++ b/js/protocol-yaml-v3.js @@ -1019,7 +1019,20 @@ function _buildSequenceEntry(doc, entry) { } const blockShape = { trials: entry.trials.slice() }; if (typeof entry.name === 'string' && entry.name) blockShape.name = entry.name; - if (typeof entry.repetitions === 'number') blockShape.repetitions = entry.repetitions; + if (entry.repetitions !== undefined) { + // Mirror the parser's validation: positive integer only. The + // parser at lines ~335-349 already rejects 0/negatives/decimals; + // without this guard, programmatic build paths (D4, paste-import) + // would emit invalid YAML and the doc/JS-mirror could diverge + // (entry.repetitions = 0 → YAML `0` but mirror = 1 via `|| 1`). + if (typeof entry.repetitions !== 'number' || !Number.isInteger(entry.repetitions) || entry.repetitions < 1) { + throw new V3ParseError( + '_buildSequenceEntry: invalid repetitions (must be positive integer): ' + JSON.stringify(entry.repetitions), + 'INVALID_SCHEMA' + ); + } + blockShape.repetitions = entry.repetitions; + } if (entry.randomize === true) blockShape.randomize = true; if (typeof entry.intertrial === 'string' && entry.intertrial) blockShape.intertrial = entry.intertrial; return { diff --git a/tests/test-protocol-roundtrip-v3.js b/tests/test-protocol-roundtrip-v3.js index 97d85f7..9c27012 100644 --- a/tests/test-protocol-roundtrip-v3.js +++ b/tests/test-protocol-roundtrip-v3.js @@ -800,6 +800,38 @@ checkThrows( check('reps validation: omitted defaults to 1', exp.sequence[0].repetitions, 1); } +// _buildSequenceEntry (builder path used by D4 / paste-import) must mirror the +// parser's positive-integer validation, so doc/JS-mirror can't diverge. +{ + const exp = parseV3Protocol(v3WithReps('1')); + checkThrows( + 'reps validation: builder rejects 0', + () => docInsertSequenceEntry(exp, 0, { kind: 'block', trials: ['c'], repetitions: 0 }), + 'INVALID_SCHEMA' + ); + checkThrows( + 'reps validation: builder rejects -2', + () => docInsertSequenceEntry(exp, 0, { kind: 'block', trials: ['c'], repetitions: -2 }), + 'INVALID_SCHEMA' + ); + checkThrows( + 'reps validation: builder rejects 1.5', + () => docInsertSequenceEntry(exp, 0, { kind: 'block', trials: ['c'], repetitions: 1.5 }), + 'INVALID_SCHEMA' + ); + checkThrows( + 'reps validation: builder rejects non-number', + () => docInsertSequenceEntry(exp, 0, { kind: 'block', trials: ['c'], repetitions: 'two' }), + 'INVALID_SCHEMA' + ); + // Positive integer accepted on the builder path + docInsertSequenceEntry(exp, 0, { kind: 'block', trials: ['c'], repetitions: 4 }); + check('reps validation: builder accepts 4', exp.sequence[0].repetitions, 4); + // Omitted accepted on the builder path (becomes default 1 on mirror) + docInsertSequenceEntry(exp, 0, { kind: 'block', trials: ['c'] }); + check('reps validation: builder accepts omitted', exp.sequence[0].repetitions, 1); +} + // ─── Test Suite 11: docInsertCommand / docMoveCommand / delete-command ──── console.log('\n--- Suite 11: command add / move / delete ---');