mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-12 19:59:35 +00:00
refactor(app): the host seam + carve section practice out of app.js (R3a) (#887)
static/js/host.js (99) + static/js/section-practice.js (1,214).
app.js 9,461 → 8,409.
THE FIRST SLICE OUT OF THE STRONGLY-CONNECTED CORE. What is left in app.js is not
a tree, it is a cycle: seeding a dependency closure from section-practice, from
loops, from count-in, or from the JUCE seek shim all return the SAME 178-function
set, and setLoop() and practiceSection() call each other directly. No closure-based
carve can cut it at any seed. So it is cut BY NAME, and the calls back into app.js
go through a host seam.
61 functions + its own 24 _sectionPractice*/_sectionParents* scalars (read nowhere
else) move out. 11 hooks come back in. 4 of those are read-only GETTERS —
loopA/loopB/_audioSeekGen/_loopMutationGen are only ever READ here, never written,
so app.js keeps owning them and NO state container is needed (a 977-site lift
avoided).
app.js used to reach IN and reset the module's state by hand (clearLoop() zeroed the
selection; changeArrangement() invalidated the parent count). It cannot now — an
imported binding is read-only — so those are exported as resetSelection() and
invalidateParentCount(). Strictly better: the module owns its own invariants instead
of trusting two callers on the far side of the file to zero the right three fields.
═══ THE SILENT-NO-OP PROBLEM, SOLVED ═══
The obvious host seam is an object of no-op defaults. That is a TRAP and we walked
into it once: the plugin loader's seam defaulted populateVizPicker to `() => {}`, so
a dropped wiring line would have left the viz picker quietly not refreshing with NO
test, boot check, or bot noticing. Two layers stop it here:
1. RUNTIME — host.js is a Proxy with NO defaults and NO stubs. Reading an unwired
hook THROWS. An unwired hook cannot degrade into a no-op because there is
nothing to degrade INTO. configureHost() also rejects a non-function at WIRE
time, and refuses to run twice.
2. STATIC — tests/js/host_contract.test.js asserts the hooks the modules USE are
exactly the hooks app.js WIRES. This is the layer that matters: a runtime throw
only fires if the broken path executes, and the whole danger of a seam is the
paths that never run in a smoke test. VERIFIED TO BITE in all three drift
directions: drop a hook from configureHost -> fails; rename host.setLoop in the
module -> fails; wire a hook nobody uses -> fails.
Writing that guard took three tries and each failure is instructive: (a) the
configureHost regex anchored `});` at column 0, ran past the indented close, and
swallowed app.js's 66-name window contract — 77 "hooks"; (b) an import-stripping
regex with `[\s\S]*?` ate 14,000 characters INCLUDING the drift the bite test was
meant to catch — a guard with a hole is worse than no guard, because you trust it;
(c) `host.js'` in the import path backtracked from `js` to a "hook" called `j`.
The bite tests are what surfaced all three.
CODEX FOUND A REAL RACE [P2]. configureHost() was inside the async boot function,
after several awaits — but the window handlers (onPhraseNext, …) go live during
app.js's SYNCHRONOUS module evaluation. A user clicking one in that window would hit
"[host] … was read before configureHost() ran". It is now a bare top-level statement
sitting immediately before the window contract, so the seam is always wired before a
handler can be reached. Verified live: invoking a handler 1.2s in — well before the
boot awaits settle — works.
VERIFIED. A/B against origin/main in two browsers with a REAL song loaded: popover
toggle, practice-mode change, phrase-next, and clearLoop (all of which cross the seam
— setLoop/clearLoop/_audioTime/loopA/loopB) — IDENTICAL, zero page errors. Since an
unwired hook throws, a live app is itself proof the seam is wired.
Harnesses: section_practice_dismiss retargeted; loop_api's clearLoop sandbox gains a
resetSelection SPY (not a stub) and ASSERTS it fires — the guarantee is still tested,
just through the seam.
pytest 2396, node 1040/1040, ESLint 0 (no-cycle clean), tailwind clean, Codex 0.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
f53d566dbc
commit
64f04565e2
+63
-1103
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,99 @@
|
|||||||
|
// The host seam — how a carved-out module calls back into app.js.
|
||||||
|
//
|
||||||
|
// WHY THIS EXISTS. What is left in app.js is not a tree, it is a cycle: seeding a
|
||||||
|
// dependency closure from count-in, from loops, from section-practice, or from the
|
||||||
|
// JUCE seek shim all return the SAME 178-function set, and setLoop() and
|
||||||
|
// practiceSection() call each other directly. So a module carved out of that
|
||||||
|
// component will always need to call back into app.js — and it cannot `import`
|
||||||
|
// app.js to do it, because app.js imports the module, and that closes a cycle the
|
||||||
|
// import-x/no-cycle gate (rightly) rejects.
|
||||||
|
//
|
||||||
|
// So app.js hands its functions DOWN, once, at boot: `configureHost({ playSong, … })`.
|
||||||
|
//
|
||||||
|
// ─── THE FAILURE MODE THIS IS BUILT TO PREVENT ───────────────────────────────
|
||||||
|
//
|
||||||
|
// The obvious way to write this is a plain object with no-op defaults. That is a
|
||||||
|
// TRAP, and we walked into it once already: the plugin loader's host seam defaulted
|
||||||
|
// `populateVizPicker` to `() => {}`, which means that if the wiring call in app.js
|
||||||
|
// is ever dropped, renamed, or drifts, the loader keeps running, the viz picker
|
||||||
|
// silently stops refreshing, and NOTHING — no test, no boot check, no bot — says a
|
||||||
|
// word. A feature just quietly stops existing.
|
||||||
|
//
|
||||||
|
// Two layers stop that here, and the second is the one that actually closes it:
|
||||||
|
//
|
||||||
|
// 1. RUNTIME — reading an unwired hook THROWS. There are no defaults and no
|
||||||
|
// stubs. `host.playSong` either is the real function or it is a loud error.
|
||||||
|
// An unwired hook cannot degrade into a no-op, because there is nothing for
|
||||||
|
// it to degrade INTO.
|
||||||
|
//
|
||||||
|
// 2. STATIC — tests/js/host_contract.test.js asserts that the set of hooks the
|
||||||
|
// modules USE is exactly the set app.js WIRES. This is the important one:
|
||||||
|
// layer 1 only fires if the broken path actually executes, and the whole
|
||||||
|
// danger of this seam is paths that don't run in a smoke test. The static
|
||||||
|
// check catches a drifted or misspelled hook in CI, on a path nobody ran.
|
||||||
|
//
|
||||||
|
// Consequence for anyone adding a hook: add it to the configureHost({…}) call in
|
||||||
|
// app.js *and* use it as `host.<name>`. The contract test fails on either alone —
|
||||||
|
// deliberately. A hook wired but never used is dead weight; a hook used but never
|
||||||
|
// wired is a bug that would otherwise hide.
|
||||||
|
|
||||||
|
const _hooks = Object.create(null);
|
||||||
|
let _configured = false;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Called ONCE by app.js at boot, before any carved module runs. Every value must
|
||||||
|
* be a function — a hook that is accidentally `undefined` (a typo, a renamed
|
||||||
|
* export, a dropped line) fails HERE, at startup, rather than silently much later.
|
||||||
|
*/
|
||||||
|
export function configureHost(hooks) {
|
||||||
|
if (_configured) {
|
||||||
|
throw new Error('[host] configureHost() called twice — it must be wired exactly once, at boot.');
|
||||||
|
}
|
||||||
|
const bad = Object.entries(hooks || {})
|
||||||
|
.filter(([, v]) => typeof v !== 'function')
|
||||||
|
.map(([k]) => k);
|
||||||
|
if (bad.length) {
|
||||||
|
throw new Error(
|
||||||
|
`[host] these hooks are not functions: ${bad.join(', ')}. `
|
||||||
|
+ 'A hook is usually undefined because it was renamed or its line was dropped.',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
Object.assign(_hooks, hooks);
|
||||||
|
_configured = true;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The seam itself. Reading a hook that was never wired THROWS — it never returns
|
||||||
|
* undefined and never returns a silent no-op. See the note at the top: a no-op
|
||||||
|
* default is precisely the bug this module exists to make impossible.
|
||||||
|
*/
|
||||||
|
export const host = new Proxy(Object.create(null), {
|
||||||
|
get(_target, name) {
|
||||||
|
if (typeof name === 'symbol') return undefined; // let JS probe it freely
|
||||||
|
if (!_configured) {
|
||||||
|
throw new Error(
|
||||||
|
`[host] host.${name} was read before configureHost() ran. `
|
||||||
|
+ 'app.js must call configureHost() at boot, before any carved module executes.',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
const fn = _hooks[name];
|
||||||
|
if (typeof fn !== 'function') {
|
||||||
|
throw new Error(
|
||||||
|
`[host] host.${name} is not wired. Add it to the configureHost({ … }) `
|
||||||
|
+ 'call in app.js. (tests/js/host_contract.test.js should have caught this in CI.)',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
return fn;
|
||||||
|
},
|
||||||
|
// Keep the object honest for anything that introspects it.
|
||||||
|
has(_target, name) { return name in _hooks; },
|
||||||
|
ownKeys() { return Object.keys(_hooks); },
|
||||||
|
getOwnPropertyDescriptor(_target, name) {
|
||||||
|
return name in _hooks
|
||||||
|
? { value: _hooks[name], enumerable: true, configurable: true, writable: false }
|
||||||
|
: undefined;
|
||||||
|
},
|
||||||
|
set(_target, name) {
|
||||||
|
throw new Error(`[host] host.${String(name)} is read-only — hooks are wired only via configureHost().`);
|
||||||
|
},
|
||||||
|
});
|
||||||
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,113 @@
|
|||||||
|
// The host-seam contract: the hooks the modules USE must be exactly the hooks
|
||||||
|
// app.js WIRES.
|
||||||
|
//
|
||||||
|
// This is the test that makes the seam safe. static/js/host.js already throws at
|
||||||
|
// runtime when an unwired hook is read — but a runtime throw only fires if the
|
||||||
|
// broken path actually executes, and the entire danger of a host seam is the paths
|
||||||
|
// that DON'T run in a smoke test. That is not hypothetical: the plugin loader's
|
||||||
|
// seam defaulted a hook to `() => {}`, and a dropped wiring line would have left
|
||||||
|
// the viz picker silently not refreshing with no test, boot check, or bot noticing.
|
||||||
|
//
|
||||||
|
// So this closes it statically. Rename a hook in app.js, drop a line from the
|
||||||
|
// configureHost({…}) call, or typo a `host.foo` in a module, and CI fails — on a
|
||||||
|
// path nobody ever ran.
|
||||||
|
//
|
||||||
|
// It is deliberately symmetric:
|
||||||
|
// * used but not wired -> a latent crash (host.js would throw at runtime)
|
||||||
|
// * wired but not used -> dead weight, and usually the fossil of a rename
|
||||||
|
// Both fail.
|
||||||
|
|
||||||
|
const { test } = require('node:test');
|
||||||
|
const assert = require('node:assert/strict');
|
||||||
|
const fs = require('node:fs');
|
||||||
|
const path = require('node:path');
|
||||||
|
|
||||||
|
const ROOT = path.join(__dirname, '..', '..');
|
||||||
|
const APP_JS = path.join(ROOT, 'static', 'app.js');
|
||||||
|
const JS_DIR = path.join(ROOT, 'static', 'js');
|
||||||
|
|
||||||
|
// Strip comments, so prose about `host.foo` in a header block is not read as a call
|
||||||
|
// site.
|
||||||
|
//
|
||||||
|
// NOTHING ELSE. An earlier version also tried to strip import statements (to stop
|
||||||
|
// `from './host.js'` reading as a hook called `js`) and its `[\s\S]*?` spanned lines
|
||||||
|
// and silently ate 14,000 characters of the file — including, in the bite test, the
|
||||||
|
// very drift it was supposed to catch. A guard with a hole in it is worse than no
|
||||||
|
// guard, because you trust it. The `host.js` path is excluded far more cheaply,
|
||||||
|
// below, by refusing a match followed by a quote.
|
||||||
|
function scrub(src) {
|
||||||
|
return src
|
||||||
|
.replace(/\/\*[\s\S]*?\*\//g, '')
|
||||||
|
.replace(/^\s*\/\/[^\n]*$/gm, '');
|
||||||
|
}
|
||||||
|
|
||||||
|
// `host.<name>` — but not `host.js'` from the `from './host.js'` import path, which is
|
||||||
|
// the one string in these files that looks like a hook and isn't.
|
||||||
|
//
|
||||||
|
// The trailing class must forbid a WORD character as well as a quote. With only
|
||||||
|
// `(?!['"])`, `host.js'` fails on `js` (a quote follows), then BACKTRACKS to `j` —
|
||||||
|
// where the next char is `s`, not a quote — and happily reports a hook called `j`.
|
||||||
|
// Forbidding `[\w$]` too leaves it nowhere to backtrack to.
|
||||||
|
const HOOK_RE = /(?<![\w$.])host\.([A-Za-z_$][\w$]*)(?![\w$'"])/g;
|
||||||
|
|
||||||
|
/** Every `host.<name>` referenced by a carved module. */
|
||||||
|
function hooksUsed() {
|
||||||
|
const used = new Map(); // name -> [files]
|
||||||
|
for (const file of fs.readdirSync(JS_DIR)) {
|
||||||
|
if (!file.endsWith('.js') || file === 'host.js') continue;
|
||||||
|
const raw = fs.readFileSync(path.join(JS_DIR, file), 'utf8');
|
||||||
|
if (!/from\s+'\.\/host\.js'/.test(raw)) continue;
|
||||||
|
for (const m of scrub(raw).matchAll(HOOK_RE)) {
|
||||||
|
if (!used.has(m[1])) used.set(m[1], []);
|
||||||
|
used.get(m[1]).push(file);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return used;
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Every hook app.js passes to configureHost({ … }). */
|
||||||
|
function hooksWired() {
|
||||||
|
const src = scrub(fs.readFileSync(APP_JS, 'utf8'));
|
||||||
|
// NB the closing brace is INDENTED (the call sits inside the boot function), so
|
||||||
|
// anchoring on `\n});` at column 0 runs straight past it and swallows the next
|
||||||
|
// object literal in the file — which is how this first read 77 "hooks", most of
|
||||||
|
// them app.js's window contract.
|
||||||
|
const call = src.match(/configureHost\(\{([\s\S]*?)\n\s*\}\);/);
|
||||||
|
if (!call) return null; // no seam wired yet — fine until there is one
|
||||||
|
const wired = new Set();
|
||||||
|
for (const m of call[1].matchAll(/(?:^|,)\s*([A-Za-z_$][\w$]*)\s*(?=[,:}]|$)/gm)) {
|
||||||
|
wired.add(m[1]);
|
||||||
|
}
|
||||||
|
return wired;
|
||||||
|
}
|
||||||
|
|
||||||
|
test('every host.<hook> a module uses is wired by app.js', () => {
|
||||||
|
const used = hooksUsed();
|
||||||
|
if (used.size === 0) return; // no consumers yet
|
||||||
|
const wired = hooksWired();
|
||||||
|
assert.ok(wired, 'modules import ./host.js but app.js never calls configureHost({ … })');
|
||||||
|
|
||||||
|
const missing = [...used.keys()]
|
||||||
|
.filter((h) => !wired.has(h))
|
||||||
|
.map((h) => `${h} (used in ${used.get(h).join(', ')})`);
|
||||||
|
|
||||||
|
assert.deepEqual(
|
||||||
|
missing, [],
|
||||||
|
'these hooks are read by a module but never wired by app.js — they would throw at runtime, '
|
||||||
|
+ 'on whatever path happens to reach them',
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('every hook app.js wires is actually used by a module', () => {
|
||||||
|
const wired = hooksWired();
|
||||||
|
if (!wired || wired.size === 0) return;
|
||||||
|
const used = hooksUsed();
|
||||||
|
|
||||||
|
const unused = [...wired].filter((h) => !used.has(h));
|
||||||
|
|
||||||
|
assert.deepEqual(
|
||||||
|
unused, [],
|
||||||
|
'these hooks are wired by app.js but no module reads them — dead weight, and usually '
|
||||||
|
+ 'the fossil of a rename that left the other half behind',
|
||||||
|
);
|
||||||
|
});
|
||||||
@@ -44,10 +44,18 @@ function buildSandbox() {
|
|||||||
const seekCalls = [];
|
const seekCalls = [];
|
||||||
const sectionPracticeModeCalls = [];
|
const sectionPracticeModeCalls = [];
|
||||||
const transportEvents = [];
|
const transportEvents = [];
|
||||||
|
// clearLoop() used to zero section-practice's three selection scalars by hand.
|
||||||
|
// They now live in static/js/section-practice.js, which owns them, so clearLoop
|
||||||
|
// calls its exported resetSelection() instead. This is a SPY, not a stub — the
|
||||||
|
// test below still asserts the reset happens, it just asserts it through the
|
||||||
|
// seam rather than by reaching into someone else's state.
|
||||||
|
const resetSelectionCalls = [];
|
||||||
const sandbox = {
|
const sandbox = {
|
||||||
seekCalls,
|
seekCalls,
|
||||||
sectionPracticeModeCalls,
|
sectionPracticeModeCalls,
|
||||||
transportEvents,
|
transportEvents,
|
||||||
|
resetSelectionCalls,
|
||||||
|
resetSelection: () => resetSelectionCalls.push(true),
|
||||||
// Mutable state (declared as `var` in eval prelude so it lives on
|
// Mutable state (declared as `var` in eval prelude so it lives on
|
||||||
// the sandbox global and the extracted functions can read/write).
|
// the sandbox global and the extracted functions can read/write).
|
||||||
// The actual values are set below.
|
// The actual values are set below.
|
||||||
@@ -201,7 +209,7 @@ test('setLoop rejects b <= a', async () => {
|
|||||||
await assert.rejects(() => sandbox.__setLoop(10, 5), /b > a/);
|
await assert.rejects(() => sandbox.__setLoop(10, 5), /b > a/);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('clearLoop resets loopA/loopB to null', async () => {
|
test('clearLoop resets loopA/loopB to null (and asks section-practice to drop its selection)', async () => {
|
||||||
const src = fs.readFileSync(APP_JS, 'utf8');
|
const src = fs.readFileSync(APP_JS, 'utf8');
|
||||||
const sandbox = buildSandbox();
|
const sandbox = buildSandbox();
|
||||||
loadFunctions(sandbox, src);
|
loadFunctions(sandbox, src);
|
||||||
@@ -211,6 +219,11 @@ test('clearLoop resets loopA/loopB to null', async () => {
|
|||||||
const { loopA, loopB } = sandbox.__getLoop();
|
const { loopA, loopB } = sandbox.__getLoop();
|
||||||
assert.equal(loopA, null);
|
assert.equal(loopA, null);
|
||||||
assert.equal(loopB, null);
|
assert.equal(loopB, null);
|
||||||
|
assert.equal(
|
||||||
|
sandbox.resetSelectionCalls.length, 1,
|
||||||
|
'clearLoop must ask section-practice to drop its selection (it used to zero the '
|
||||||
|
+ 'scalars by hand; the module owns them now)',
|
||||||
|
);
|
||||||
assert.equal(sandbox.sectionPracticeModeCalls.length, 1);
|
assert.equal(sandbox.sectionPracticeModeCalls.length, 1);
|
||||||
assert.equal(sandbox.sectionPracticeModeCalls[0].on, false);
|
assert.equal(sandbox.sectionPracticeModeCalls[0].on, false);
|
||||||
// Field-wise: vm-context objects break deepStrictEqual across realms.
|
// Field-wise: vm-context objects break deepStrictEqual across realms.
|
||||||
|
|||||||
@@ -15,9 +15,10 @@ const assert = require('node:assert/strict');
|
|||||||
const fs = require('node:fs');
|
const fs = require('node:fs');
|
||||||
const path = require('node:path');
|
const path = require('node:path');
|
||||||
|
|
||||||
const src = fs.readFileSync(path.join(__dirname, '..', '..', 'static', 'app.js'), 'utf8');
|
// _installSectionPracticeDismiss was carved out of app.js into its own module (R3a).
|
||||||
|
const src = fs.readFileSync(path.join(__dirname, '..', '..', 'static', 'js', 'section-practice.js'), 'utf8');
|
||||||
const m = src.match(/function _installSectionPracticeDismiss\s*\(\)\s*\{[\s\S]*?\n\}/);
|
const m = src.match(/function _installSectionPracticeDismiss\s*\(\)\s*\{[\s\S]*?\n\}/);
|
||||||
assert.ok(m, '_installSectionPracticeDismiss() not found in static/app.js');
|
assert.ok(m, '_installSectionPracticeDismiss() not found in static/js/section-practice.js');
|
||||||
const body = m[0];
|
const body = m[0];
|
||||||
|
|
||||||
test('the outside-click dismiss binds in the CAPTURE phase', () => {
|
test('the outside-click dismiss binds in the CAPTURE phase', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user