From 0a1601469837ab33eea1c95d95aca8d280f73c1d Mon Sep 17 00:00:00 2001 From: gionnibgud Date: Thu, 9 Jul 2026 22:40:33 +0200 Subject: [PATCH] fix(keys_highway_3d): stop auto-connect clobbering the global MIDI device (#825) Opening the keys highway could silently switch the user's configured MIDI device. Two coupled defects in the plugin's MIDI selection: 1. _midiAutoConnect only consulted the plugin's own localStorage pick (keys3d_midi_pick); with none saved it fell straight through to "first non-loopback device", ignoring the core midi-input domain's global selection (Settings -> Input Setup, window.slopsmith.midiInput.getSelected()). 2. _midiConnect unconditionally persisted every connect to BOTH the local pick and the shared domain selection (mi.select). So the first-device guess got frozen locally and overwrote the global default that other consumers (drums, Input Setup) rely on. Make the domain-wide selection the source of truth: _pickMidiTarget now resolves global -> legacy local pick (fallback + name-recovery for stale ids) -> first device, and gates the "don't grab a random device" recovery guard on any configured preference. Gate persistence behind an explicit `persist` flag so only a deliberate device selection writes the local pick and the shared global; auto-connect and programmatic (audio-input) opens open the resolved device for the session without touching either store. mi.select() is not needed to open (open takes the logicalSourceKey directly), so dropping it from the auto path costs nothing. Interim step toward instrument-scoped selection in the midi-input domain itself (the input_setup wizard is already per-instrument, but the domain stores a single selection); tracked as a separate core follow-up. Pure decision logic extracted to _pickMidiTarget and covered by unit tests in data_layer.test.js. Signed-off-by: gionnibgud Co-authored-by: Claude Sonnet 5 --- plugins/keys_highway_3d/screen.js | 119 +++++++++++++----- .../keys_highway_3d/tests/data_layer.test.js | 96 ++++++++++++++ 2 files changed, 182 insertions(+), 33 deletions(-) diff --git a/plugins/keys_highway_3d/screen.js b/plugins/keys_highway_3d/screen.js index ff8e3f5..6277994 100644 --- a/plugins/keys_highway_3d/screen.js +++ b/plugins/keys_highway_3d/screen.js @@ -813,20 +813,36 @@ _writeStore(STORE_KEYS.midiPick, JSON.stringify({ id: id || '', name: name || '', key: key || '' })); } - function _midiAutoConnect(allowFallback) { - // Recovery (sources-changed after unplug) passes false: never switch to a - // fallback input, because _midiConnect persists the pick and that would - // overwrite the user's saved device on a transient multi-device unplug - // (the original returns on replug and reconnects then). - if (allowFallback === undefined) allowFallback = true; - const inputs = _midiSources(); - if (!inputs.length) return; - const saved = _readSavedPick(); - // Explicit "None" opt-out. - if (saved && saved.id === '' && saved.name === '') return; - // Prefer the globally-unique logicalSourceKey, then the legacy bare - // sourceId, then case-insensitive name (Chrome on Linux regenerates ids - // per page load), then first non-loopback. + // Pure decision logic (exported via __test): pick which device to + // auto-connect to from the current source list, the domain-wide selection + // (`globalKey`, from Settings → Input Setup), and this plugin's own legacy + // saved pick. Returns null for "connect to nothing" (explicit None opt-out, + // or the configured device currently absent during hotplug recovery). + // + // The domain-wide selection is the SOURCE OF TRUTH (checked first): a device + // configured globally must never be overridden by a stale plugin-local pick + // or an arbitrary first-device fallback — that override was the bug. The + // local pick is retained only as a fallback BELOW the global (and for + // name-recovery when the global's logicalSourceKey went stale, e.g. a + // browser that regenerates MIDI port ids across reloads). Auto-connect no + // longer writes the local pick, so it only ever holds a value an explicit + // selection put there (or a stale one from a pre-fix build — the global + // still wins over it). + function _pickMidiTarget(inputs, saved, globalKey, allowFallback) { + if (!inputs.length) return null; + const notBlocked = (i) => !!i && !_MIDI_BLOCKLIST_RE.test(i.name || ''); + // Explicit "None" opt-out (set only via the device-select API). + if (saved && saved.id === '' && saved.name === '') return null; + + // 1. Domain-wide selection (Settings → Input Setup) — source of truth. + if (globalKey) { + const g = inputs.find(i => i.key === globalKey); + if (notBlocked(g)) return g; + } + + // 2. Legacy plugin-local pick, as a fallback below the global. Prefer the + // globally-unique logicalSourceKey, then the legacy bare sourceId, then + // case-insensitive name (Chrome on Linux regenerates ids per page load). let target = null; if (saved && saved.key) target = inputs.find(i => i.key === saved.key) || null; if (!target && saved && saved.id) target = inputs.find(i => i.id === saved.id) || null; @@ -834,23 +850,50 @@ const n = saved.name.toLowerCase(); target = inputs.find(i => (i.name || '').toLowerCase() === n) || null; } - // Never honour a saved pick that's a loopback / "Midi Through" port — it - // carries no device input, so a stale pick silently eats every note. The - // saved-pick lookups above bypass the block-list; re-apply it here. - if (target && _MIDI_BLOCKLIST_RE.test(target.name || '')) target = null; - if (!target) { - // Skip the substitute ONLY when a saved pick exists but is currently - // absent (recovery: preserve it, don't clobber on a transient unplug). - // With no saved pick at all, a fallback is the intended first-hotplug - // auto-connect — allow it even in recovery. - const hasSavedPick = !!(saved && (saved.key || saved.id || saved.name)); - if (!allowFallback && hasSavedPick) return; - target = inputs.find(i => !_MIDI_BLOCKLIST_RE.test(i.name || '')) || inputs[0]; - } + // Never honour a saved pick that resolves to a loopback / "Midi Through" + // port — it carries no device input, so it silently eats every note. + if (target && !notBlocked(target)) target = null; + if (target) return target; + + // 3. Nothing configured resolved to a present device. In recovery + // (allowFallback=false) with a configured preference — a global pick or a + // saved pick — that's currently absent, preserve it rather than switching + // to an arbitrary device on a transient multi-device unplug. With no + // preference at all, a first-device grab is the intended first-hotplug + // auto-connect, allowed even in recovery. + const hasPreference = !!(globalKey || (saved && (saved.key || saved.id || saved.name))); + if (!allowFallback && hasPreference) return null; + // Connect to nothing rather than a loopback: if every present device is + // blocklisted, a first-device grab would attach to a "Midi Through"/IAC + // port that carries no input and silently eats every note. + return inputs.find(notBlocked) || null; + } + + function _midiAutoConnect(allowFallback) { + // Recovery (sources-changed after unplug) passes false: never switch to a + // fallback input on a transient multi-device unplug (the configured + // device returns on replug and reconnects then). Auto-connect is + // non-persisting (persist omitted → false): it opens the resolved device + // for this session WITHOUT writing the plugin-local pick or the shared + // domain selection, so opening this highway can't clobber the user's + // globally-configured device. + if (allowFallback === undefined) allowFallback = true; + const inputs = _midiSources(); + const saved = _readSavedPick(); + const mi = _mi(); + const globalKey = mi && typeof mi.getSelected === 'function' ? mi.getSelected() : null; + const target = _pickMidiTarget(inputs, saved, globalKey, allowFallback); + if (!target) return; _midiConnect(target.id, target.name, target.key); } - async function _midiConnect(id, name, key) { + // `persist` gates the two preference writes. Only an EXPLICIT device + // selection (the device-select API) persists: it writes the plugin-local + // pick AND the shared domain selection (`mi.select`, so the user's choice + // becomes the global default). Auto-connect and programmatic opens pass + // falsy — they open the resolved device for this session only, never + // touching either store, so they can't clobber a globally-configured device. + async function _midiConnect(id, name, key, persist) { // Capture our generation AFTER _midiDetach()'s own bump, so a later // detach (device removal / new connect / opt-out) reliably supersedes us. _midiDetach(); @@ -861,7 +904,7 @@ for (const inst of _instances) { if (inst && typeof inst._releaseAllHeld === 'function') inst._releaseAllHeld(); } - _writeSavedPick(id || '', name || '', key || ''); + if (persist) _writeSavedPick(id || '', name || '', key || ''); const mi = _mi(); if ((id || key) && mi) { // Prefer the globally-unique logicalSourceKey so two providers that @@ -874,13 +917,19 @@ const lkey = src.key || ('web-midi::' + src.id); _midiInput = { id: src.id, name: src.name, key: lkey }; _midiJustConnected = true; + // Only an explicit selection writes the shared global default; + // open takes the logicalSourceKey directly, so select() is not + // needed to open — it exists purely to set the global. Persist it + // BEFORE the no-instance early return so a settings-panel pick with + // no live renderer still updates the shared default (best-effort: + // a select hiccup must not abort the connect). + if (persist) { try { await mi.select(lkey); } catch (_) { /* best-effort */ } } // No live renderer to consume OR release a session — don't hold one // open (settings-only ensure-init, or the last instance was torn - // down during async discovery). The pick is saved; a later renderer - // mount re-runs auto-connect and opens for real, releasing on destroy. + // down during async discovery). A later renderer mount re-runs + // auto-connect and opens for real, releasing on destroy. if (_instances.size === 0) { _midiNotifyDeviceListChanged(); return; } try { - await mi.select(lkey); const res = await mi.open({ requester: PLUGIN_ID, logicalSourceKey: lkey }); // A newer _midiConnect (device switch / None / replug) ran while // we awaited open — discard this stale session so we don't wire a @@ -1039,10 +1088,11 @@ window.keysH3dGetMidiInputId = function () { return _midiInput ? _midiInput.id : ''; }; window.keysH3dSetMidiInput = function (id) { // `id` may be a logicalSourceKey (new host calls) or a legacy sourceId. + // Explicit user selection → persist (local pick + shared global default). const src = id ? (_midiSources().find(s => s.key === id) || _midiSources().find(s => s.id === id)) : null; - _midiConnect(src ? src.id : (id || ''), src ? src.name : '', src ? src.key : ''); + _midiConnect(src ? src.id : (id || ''), src ? src.name : '', src ? src.key : '', true); return true; }; window.keysH3dGetMidiChannel = function () { return _cfg.midiChannel; }; @@ -1547,6 +1597,8 @@ function _aiOpen(req) { // Opening a MIDI source connects the corresponding Web MIDI input. + // Programmatic open (audio-input source.open) — non-persisting: it must + // not rewrite the user's saved pick or the shared global default. const idx = _aiIndexFor(req && (req.sourceId || req.logicalSourceKey)); const inputs = _midiSources(); // carries .key (logicalSourceKey), unlike _midiListInputs() if (idx == null || idx >= inputs.length) { @@ -4068,6 +4120,7 @@ FX_DEFAULTS, FX_RANGES, _classifyTiming, + _pickMidiTarget, }; // Headless verification hook: lets Playwright drive synthetic note-ons diff --git a/plugins/keys_highway_3d/tests/data_layer.test.js b/plugins/keys_highway_3d/tests/data_layer.test.js index 1609a8d..a3a8b1f 100644 --- a/plugins/keys_highway_3d/tests/data_layer.test.js +++ b/plugins/keys_highway_3d/tests/data_layer.test.js @@ -188,3 +188,99 @@ test('measureMarkers extracts idx/t pairs', () => { [{ idx: 1, t: 0 }, { idx: 2, t: 2.5 }], ); }); + +test('_pickMidiTarget: no plugin-local pick defers to the domain-wide selection, not "first device"', () => { + const { _pickMidiTarget } = load(); + const inputs = [ + { id: 'a', name: 'Device A', key: 'web-midi::a' }, + { id: 'b', name: 'Device B', key: 'web-midi::b' }, + ]; + // Fresh install / never picked here — must use the Input Setup global, + // NOT fall through to inputs[0]. + const target = _pickMidiTarget(inputs, null, 'web-midi::b', true); + assert.equal(target.id, 'b'); +}); + +test('_pickMidiTarget: the domain-wide selection is the source of truth — it wins over a stale plugin-local pick', () => { + const { _pickMidiTarget } = load(); + const inputs = [ + { id: 'a', name: 'Device A', key: 'web-midi::a' }, + { id: 'b', name: 'Device B', key: 'web-midi::b' }, + ]; + // A stale local pick (e.g. left by a pre-fix build's auto-connect) must + // NOT override the device the user configured in Settings → Input Setup. + const target = _pickMidiTarget(inputs, { id: 'a', name: 'Device A', key: 'web-midi::a' }, 'web-midi::b', true); + assert.equal(target.id, 'b'); +}); + +test('_pickMidiTarget: local pick is used as a fallback when no global is configured', () => { + const { _pickMidiTarget } = load(); + const inputs = [ + { id: 'a', name: 'Device A', key: 'web-midi::a' }, + { id: 'b', name: 'Device B', key: 'web-midi::b' }, + ]; + const target = _pickMidiTarget(inputs, { id: 'a', name: 'Device A', key: 'web-midi::a' }, null, true); + assert.equal(target.id, 'a'); +}); + +test('_pickMidiTarget: local pick name-recovers when its logicalSourceKey went stale (id regeneration)', () => { + const { _pickMidiTarget } = load(); + // Same physical device, new id/key across a reload; the saved key/id miss + // but the name still matches. + const inputs = [{ id: 'a2', name: 'Device A', key: 'web-midi::a2' }]; + const target = _pickMidiTarget(inputs, { id: 'a1', name: 'Device A', key: 'web-midi::a1' }, null, true); + assert.equal(target.id, 'a2'); +}); + +test('_pickMidiTarget: domain-wide selection is ignored if it names a blocklisted loopback port', () => { + const { _pickMidiTarget } = load(); + const inputs = [ + { id: 'thru', name: 'IAC Driver Bus 1', key: 'web-midi::thru' }, + { id: 'b', name: 'Device B', key: 'web-midi::b' }, + ]; + const target = _pickMidiTarget(inputs, null, 'web-midi::thru', true); + assert.equal(target.id, 'b'); // falls through to the first non-loopback device +}); + +test('_pickMidiTarget: when every present device is a loopback, connect to nothing (never a dead port)', () => { + const { _pickMidiTarget } = load(); + const inputs = [ + { id: 'thru', name: 'MIDI Through Port-0', key: 'web-midi::thru' }, + { id: 'iac', name: 'IAC Driver Bus 1', key: 'web-midi::iac' }, + ]; + // No non-loopback device exists — must NOT fall back to inputs[0] (a port + // that carries no input and would silently eat every note). + const target = _pickMidiTarget(inputs, null, null, true); + assert.equal(target, null); +}); + +test('_pickMidiTarget: explicit "None" opt-out still wins over any global default', () => { + const { _pickMidiTarget } = load(); + const inputs = [{ id: 'a', name: 'Device A', key: 'web-midi::a' }]; + const target = _pickMidiTarget(inputs, { id: '', name: '' }, 'web-midi::a', true); + assert.equal(target, null); +}); + +test('_pickMidiTarget: a present global wins even during hotplug recovery', () => { + const { _pickMidiTarget } = load(); + const inputs = [{ id: 'b', name: 'Device B', key: 'web-midi::b' }]; + // The configured global device is present — reconnect to it, don't bail. + const target = _pickMidiTarget(inputs, null, 'web-midi::b', false); + assert.equal(target.id, 'b'); +}); + +test('_pickMidiTarget: recovery (allowFallback=false) preserves an absent configured device instead of grabbing a random one', () => { + const { _pickMidiTarget } = load(); + const inputs = [{ id: 'b', name: 'Device B', key: 'web-midi::b' }]; + // The configured device ('x', global) is currently unplugged; a transient + // recovery must NOT switch to the unrelated device that is present. + const target = _pickMidiTarget(inputs, null, 'web-midi::x', false); + assert.equal(target, null); +}); + +test('_pickMidiTarget: recovery with no preference at all still allows a first-hotplug grab', () => { + const { _pickMidiTarget } = load(); + const inputs = [{ id: 'b', name: 'Device B', key: 'web-midi::b' }]; + const target = _pickMidiTarget(inputs, null, null, false); + assert.equal(target.id, 'b'); +});