mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-11 03:09:57 +00:00
4a1b055afdba03140bd8e7bab84a812e659d22f2
435
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 topkoa
|
67e6b25c43 |
docs(panes): best practices for plugin authors
Every rule here is something that has already gone wrong on this feature — mostly in core's own code, twice in the two plugins that adopted it first. They are cheap to get right up front and miserable to diagnose later, because a broken pane almost always LOOKS perfect. The traps, and why each one is easy to walk into: - Your code still runs in the main window. That is exactly why moving the element works at all — and exactly why `document.body.appendChild(tooltip)` inside a popped-out panel lands in the window the user is NOT looking at. - Don't hide your own panel when it pops out. Core hides it and leaves a stub. A plugin that also hides it hides the node that just moved — which is precisely how core's own chip shipped a blank pop-out window. - Use `hidden` or a class, not inline `display`, for show/hide. `.fb-paned` forces the panel visible while it is out; when it docks and that class is removed, an inline `display:none` reasserts itself and the panel returns invisible. - `isConnected` does not mean "docked". A panel in a pane window IS connected, just not to this document. The test you meant is `el.ownerDocument === document`. - `element` is a function so it can be resolved late: return the LIVE node, and re-attach the chip if you rebuild your panel (Camera Director rebuilds on every mode change). - rAF is throttled while the main window is backgrounded — which it is, whenever the user is looking at your pane. Event-driven panels don't care; continuously animating ones will stutter exactly when they are the only thing on screen. - Don't synchronise anything. One realm, one panel. Writing sync code means you have misunderstood the model. Also states what core guarantees back, including the one that cost the most to learn: the element is evacuated BEFORE the pane window's document is destroyed, so it comes home alive rather than as a photograph of a panel with every listener in its subtree silently gone. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
K. O. A.andGitHub
|
503716acbf |
Merge pull request #928 from got-feedBack/feat/panes-core
feat(panes): detachable panes — pop a plugin's real panel out into its own window |
||
|
topkoa
|
41bb4482fe |
docs(panes): say which repo the desktop half lives in
Two comments pointed at `main.ts` and `pane-hosts.ts` as though they were in this repo. They are not — they are in got-feedback/feedBack-desktop, and a contributor reading only this codebase would go looking for files that do not exist. Named the repo and the paths, and said the part that actually matters: nothing here depends on that code. In a plain browser a pane window is simply a pop-up; the desktop side only upgrades it. And the frame-name prefix is a contract across two repos with no build-time link between them, so the comment IS the link — worth saying out loud. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
0955f0b6f2 |
fix(panes): keep a pane window in step with theme + interface scale
The pane window got a ONE-TIME snapshot of the app's theme classes and the interface-scale custom property. The app changes both at runtime — Interface size emits `scale:changed`, the theme emits `theme:changed` / `v3:cosmetics-applied` — so an already-open pane went on rendering at the old scale, in the old palette, the moment the user touched either. "Looks identical" has to keep being true, not merely start out true. A pane window now follows those three events for as long as it is open, and stops on unplace(). The inline style is assigned wholesale rather than merged: unlike the class lists (where pane.html's own `fb-pane-window` must survive), there is nothing in the pane document's inline style to preserve — and concatenating on every change would grow the attribute without bound as the user dragged the scale slider. Also: the dock's focus() always smooth-scrolled, ignoring prefers-reduced-motion — which panes.css already honours for the card's flash animation. A smooth scroll is motion too, and someone who asked for less of it meant this as well. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
3a50e593bf |
fix(panes): fail fast when the pane window is unreachable; validate opts.header
Two from review. 1. _whenReady's own comment said a SecurityError means the pop-out is not reachable from this realm and "no amount of waiting will fix it" — and then it waited the full 10s deadline anyway. Ten seconds of a detached panel and a half-popped-out UI, for a condition we had already diagnosed as fatal. It now gives up after a 1s grace instead. Not instantly, deliberately: a throw *during* the navigation from about:blank to /pane would otherwise take down a pop-out that was about to work perfectly. A second is far more than that transition needs and far less than a user should spend staring at a detached panel. 2. attachChip() took opts.header on trust. It's a public plugin API, and a truthy non-Element header (a selector string, a wrapper object, a ref) is an easy mistake — one that surfaced as a confusing DOM exception from deep inside core instead of a TypeError naming the offending pane. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
5049be0523 |
fix(panes): the dock is born empty, so say so
panes.css hides an empty dock (.fb-pane-dock.is-empty { display: none }), but
the element was created without the class — so between creation and the first
card it was a visible-to-CSS, announced-to-screen-readers role="region"
landmark containing nothing.
Harmless in practice today (the dock is created lazily, on the same tick as the
card that prompted it), but the CSS contract should hold from first paint rather
than from the first _syncEmpty(), and any future caller of dock() gets the right
thing for free.
Found by CodeRabbit on #928.
Signed-off-by: topkoa <topkoa@gmail.com>
|
||
|
topkoa
|
de2a42bd35 |
fix(panes): coerce plugin-supplied pane sizes to numbers
`spec.width` / `spec.height` are plugin-controlled, and the window host builds
window.open()'s feature string by concatenation:
'popup,width=' + spec.width + ',height=' + spec.height
`spec.width || 380` passed anything truthy straight through. So a width of
'300,menubar=1' would not merely be an invalid size — it would inject window
features. Less dramatically, any non-numeric value produced a malformed feature
string and a pane that failed to open for no visible reason.
They now go through _size(): Number, round, reject anything not finite and
positive, clamp to 120..4000. A hostile or careless value falls back to the
default instead of reaching window.open() at all.
Verified against the obvious inputs: '300,menubar=1' -> 380 (default), '300' ->
300, 0/-50/NaN/{}/'abc' -> 380, 1e9 -> 4000, 5 -> 120.
Signed-off-by: topkoa <topkoa@gmail.com>
|
||
|
topkoa
|
671aba950c |
chore(panes): drop the leftover adoption diagnostics
A ~15-line console.info dumping computed styles, sizes, child counts and the element's inline style on every single pop-out. It was instrumentation written to chase the "panel comes home dead" bug, and it should have gone out with the rest of the debugging — it survived the cleanup. Removed rather than downgraded to console.debug: nothing here is worth keeping even behind a flag. The failures it was built to diagnose are all handled and commented now, and the paths that can still go wrong (window never loads, adopt throws) already log a console.error that says what happened. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
95d6d8a46e |
fix(panes): keep panes.css last in the pane window's cascade
_copyStyles appended the app's stylesheets to the pane document — which already links panes.css — so they landed AFTER it. In the app document panes.css loads last, after tailwind/style/v3, and its rules win ties. In the pane window that order was silently inverted, letting core styles override the pane chrome and the .fb-paned placement rules. Cascade order is not a detail here. "Looks identical" has to include the order things are said in, or the same markup with the same sheets can still render differently. The clones now go in BEFORE pane.html's own link, preserving their relative order among themselves and leaving panes.css last, exactly as in the app. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
f43779c99e |
fix(panes): detach the element when the pop-out starts, not when it lands
The window host's place() is asynchronous — it opens the window, waits for /pane to load, and only then adopts the element in. But the manager emits `panes:opened` as soon as place() returns, and the chip reacts by putting its "popped out" stub where the element used to be. So for that gap the user saw BOTH: the real panel still sitting in its original spot, and a stub next to it claiming the panel had left. On a window that never loads, that lasts the full 10s readiness timeout. Detach the element as soon as we commit to moving it. That is not destructive: the node keeps its owner document, its listeners and its closures — it is simply out of the tree, waiting for a document to be adopted into. And if the window never loads, closePane() puts it straight back at its home, which is exactly what the failure path already does. The dock host has no such gap; its place() moves the element synchronously. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
82aa8a757e |
fix(panes): harden the persisted host map against unsafe pane ids
A pane id is plugin-controlled, and it becomes a KEY in the persisted
{ paneId: hostId } map. `__proto__` and friends are not ids, they are booby
traps:
- `map['__proto__'] = 'window'` on a plain object corrupts the map, and
can reach Object.prototype.
- `map[id]` on a polluted (or hand-edited) object can return a value straight
off the prototype chain for a pane that was never remembered at all — so a
pane could be "restored" to a host nobody ever put it in.
Three layers, because each is a one-liner:
- Reject `__proto__` / `constructor` / `prototype` as pane ids at
registration, so they never reach storage.
- Re-key whatever comes out of localStorage onto a null-prototype object, so
a corrupt or hand-edited value cannot smuggle a prototype in.
- Read with an own-property check.
Also from the same review:
- Removed `window.__fbPaneWindows`. It was exposed for pane-desktop.js back
when that file needed to reach the window handles; the rewrite dropped that
need and nothing has referenced it since. Dead global, and its comment
described a collaborator that no longer exists.
- Corrected the /pane cache comment. It claimed a stale page would leave the
window blank, which stopped being true when the readiness check gained a
`doc.body` fallback — it would still work, just without the pane window's
own layout. A comment that describes a failure mode the code no longer has
is worse than no comment.
Signed-off-by: topkoa <topkoa@gmail.com>
|
||
|
topkoa
|
cb425ed48d |
fix(panes): don't hide a docked pane; don't force display; restore visibility
Six more findings from CodeRabbit on #928. Three are real bugs. 1. THE CHIP HID DOCKED PANES. `_onOpened` decided "did the pane take my element?" from `ownerDocument !== document`. That is true for a pane in a pop-out window — and false for a pane moved into the DOCK, which lives in this very document. So docking a pane stamped `.fb-pane-detached` (display:none !important) onto the panel the user was looking at, and put the stub next to it instead of at its home. The element cannot answer this question — `isConnected` is true in a pane window, `ownerDocument` is this one in the dock. Both were live bugs. Ask the manager, which knows exactly what it handed to the host: `panes.elementOf(id)`. That holds for every host, and for reconciling after the fact (detail == null), which is what a plugin rebuilding its panel mid-pop-out triggers. 2. `.fb-paned` FORCED `display: block !important`. A panel that is `display:flex` or `grid` would be silently re-laid-out while detached — the exact opposite of "placement only", and precisely the kind of surprise this feature exists to avoid. Removed. Making a hidden panel visible is a separate job, and it now belongs to the manager, which does it without touching the panel's display MODE: clear `hidden`, and clear an inline `display:none` if that is how the panel hides. 3. VISIBILITY IS NOW RESTORED. The hosts used to set `el.hidden = false` and never put it back, so the docs' "core only changes placement" was a lie and a panel's hidden state was quietly lost. The manager stashes both `hidden` and the inline `display` on open and restores them on dock: a panel that was closed when you opened its pane from the tray goes back to being closed; one that was open stays open. Plus: - The launcher rebuilt its whole list on every panes:opened/closed — including the one fired by clicking a button in that list — destroying the button under the user's finger and dropping focus to <body>. It now restores focus to the toggled pane's button. - `_copyStyles` cloned every stylesheet link, including the panes.css that pane.html already loads. Skip sheets the pane document already has. - Docs: the chip may route to the DOCK, not always a window (it goes through detach() → the host router). `header` precedence was documented backwards — an explicit `header` wins. And the visibility contract above is now written down rather than being a surprise. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
b74a364857 |
docs(panes): re-attach the chip when the panel is rebuilt
A plugin that rebuilds its panel (Camera Director does, on every mode change) takes the chip with it. attachChip() returns a detach(); call it before re-attaching, and again in teardown, or you leave a stub pointing at DOM that no longer exists. Found by CodeRabbit on #928. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
859b0036e5 |
fix(panes): review fixes — stranded elements, duplicate listener, class clobber
Three real findings from CodeRabbit on #928, all in current code. 1. closePane() adopted the element out of the pane window ONLY when its original home was still connected. If the panel never had a parent (a plugin that builds it lazily and hands it straight over) or its container was torn down while the pane was out (a screen change), the whole block was skipped — leaving the element inside a window we then close, which strips every listener in its subtree. That is exactly the "comes home dead" failure this ordering exists to prevent; the guard just moved it from the common path to the rare one, where it is far harder to spot. Adopting and re-homing are two different jobs and only one of them is allowed to fail. Adopt UNCONDITIONALLY — that is what rescues the element — and insert only when there is somewhere to insert it. With no home the element ends up owned by this document but not in it: detached, intact, listeners alive, ready for the plugin to re-insert. 2. The pane window's `beforeunload` handler was registered TWICE, comment block and all — a bad scripted edit on my part. Harmless (the handler is idempotent via panes.isOpen) but dead duplicate code. Also fixed the stale comment further down that still claimed there was no beforeunload listener at all. 3. _copyStyles ASSIGNED className on the pane document's <html> and <body> instead of merging. pane.html sets `class="fb-pane-window"` on <html>, and panes.css hangs the pane window's own chrome off exactly that — so copying the app's classes over it silently took the pane window's own layout with them. Merge both class lists, and append the interface-scale inline style rather than replacing the attribute. Also guarded the docs' integration example behind a feedBack.panes check: the doc says the API is optional, and then showed an example that would throw on a host without it. Not applicable (reviewed against |
||
|
topkoa
|
a7348052ae |
fix(panes): give the pop-out stub a focus ring
.fb-pane-stub is a <button>, and both of its sibling controls (.fb-pane-chip, .fb-pane-card-btn) have an explicit :focus-visible outline. It didn't, so keyboard focus fell back to the UA default and looked inconsistent next to them. Found by CodeRabbit on #928. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
1e5282e27e |
fix(panes): get the element out before the pane window's document dies
Docking a popped-out panel brought it home DEAD. It rendered perfectly —
right markup, right size, right place — and every control in it was inert:
the close button, the sliders, the presets, even the pop-out chip. A
photograph of a panel.
Closing a pane window tears down its document, and the panel was still
inside it. The node itself survives (the manager holds a reference), but
every event listener in its subtree goes with the document that hosted
them. Two paths did this:
1. closePane() called the host's unplace() — which closes the window —
BEFORE adopting the element back. Order is now reversed, and the
comment says why so nobody helpfully "tidies" it back.
2. The user closing the pane window themselves was only noticed by the
`closed` poll, which by definition runs AFTER the document is gone.
The window now gets a `beforeunload` listener that brings the element
home while its document is still alive.
That listener has to be attached AFTER /pane loads: window.open() hands
back a throwaway about:blank document, and anything registered on it is
discarded when the real page replaces it. This is the same trap that made
the pane window blank in the first place — adopt into about:blank and the
panel is destroyed a moment later — and it is now handled in both places.
The `closed` poll stays, but only as a last-resort net for a CRASHED pane
window, where nothing can be saved.
Also fixed while chasing this:
- The chip stamped `.fb-pane-detached` (display:none !important) onto the
element to hide it in the main window — and that element is the one we
move, so the class travelled with it and blanked the pane window. The
chip now only hides an element the pane did NOT take, and marks the hole
with its stub otherwise. "Did not take" is an ownerDocument test, not
isConnected: a panel sitting in a pane window IS connected, just not
here, and a plugin that rebuilds its panel (Camera Director does, on
every mode change) re-runs attachChip while popped out.
- The stub was inserted "before the element", which is nowhere — the
element has left the document. The manager now hands over the element's
recorded home, and the stub goes there.
- GET /pane sent no cache headers. A stale copy is especially nasty here:
the opener waits for an element inside that page before adopting, so an
old cached version means the pane window just sits there blank.
Verified in the desktop app: pop out, use the controls in the pane window,
dock back, use them again. Panel comes home alive.
Signed-off-by: topkoa <topkoa@gmail.com>
|
||
|
OmikronApexandGitHub
|
d364529919 |
Merge pull request #930 from got-feedBack/fix/accuracy-floor-not-round
ship-ci / ci (push) Waiting to run
fix(v3): floor accuracy percentages so 100% means all notes hit |
||
|
OmikronApexandClaude Fable 5
|
81ef11d855 |
fix(v3): floor accuracy percentages so 100% means all notes hit
Math.round let 431/433 (99.54%) display as 100%. Floor at every accuracy display site (HUD, library badges, dashboard, lessons, profile, playlists, calibration overlay); stored fractions and mastery thresholds unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
topkoa
|
9e9f0fdac6 |
fix(panes): adopt into the real pane document, not about:blank
Both pop-outs opened blank. window.open() returns immediately, and the window it hands back already has a document — an `about:blank` one, whose readyState is 'complete'. So the host cheerfully adopted the panel into THAT, it worked for a few milliseconds, and then /pane finished loading, replaced the document, and took the panel with it. Blank window, vanished element. Waiting for 'load' is no better: it may already have fired for about:blank before we could listen. So don't trust readyState and don't trust 'load' — wait for the one thing that exists only in the document we actually want: pane.html's #fb-pane-root. Poll for it (guarding the cross-document window while it is mid-swap), give up after 10s, and on failure bring the element home rather than stranding it in a window that never loaded. Also drop the popup's 'beforeunload' listener: it was registered on the about:blank window and discarded along with it, so it never fired. The `closed` poll is what notices a user shutting a pane window — as it must be anyway, since a crashed renderer never says goodbye either. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
188bdaa837 |
feat(panes)!: move the real element, instead of rebuilding it
The first cut of this got the model wrong. A pane was a SECOND
implementation of the plugin's panel — its own sliders, its own styling,
driven over a cross-realm bridge (ctx, a state store, capability RPC,
mirrorGlobal, a stream sampler). Popping out gave you something that
resembled the panel you popped, and every feature it did not reimplement
(presets, tabs, EQ, language) was simply gone.
What a user wants from "pop this out" is the thing they popped out.
So: MOVE THE REAL ELEMENT. Same-origin windows can adopt each other's
nodes, and an adopted node keeps its event listeners and its closures.
The panel goes on running the plugin's own code, against the plugin's own
state, in the plugin's own realm — it is merely being DISPLAYED in another
window. Copy the app's stylesheets into that window and it looks identical
too, because it is identical.
The plugin's side collapses to two lines:
feedBack.panes.register({ id, title, element: () => panelEl });
feedBack.panes.attachChip(panelEl, id);
and everything comes along: the CSS, the listeners, the presets, the
state. Nothing to keep in step, because there is no second copy.
Deleted, all of it now pointless: pane-bridge (ctx + transports), pane-hub
(the cross-realm server), pane-runtime (the pane realm's boot), pane-streams
(the rAF sampler that existed because an AnalyserNode can't cross a window),
pane-mirror (mirrorGlobal), pane-plugins + the manifest `panes[]` key and its
server-side validation, panes.state(), and both built-in demo panes. ~1200
lines. None of it was wrong — it was all correct machinery for the wrong
problem.
Consequences worth knowing:
- The window MUST be opened by the renderer with window.open(), not by the
desktop's main process: a window we did not open gives this realm no handle
to its document, and without the handle there is nothing to adopt into.
Electron turns the same-origin window.open() into a real BrowserWindow
anyway (setWindowOpenHandler → 'allow'), so we get the OS window AND the
live DOM link. The desktop side finds it by frame name.
- `.fb-paned` neutralises PLACEMENT only (position/inset/width/z-index/shadow).
A plugin panel is nearly always a fixed overlay pinned to a corner of the
app; alone in a 380px window that positioning is nonsense. Colours, borders,
padding, fonts and the panel's own internal layout are untouched — the whole
promise is that what you popped out is what you get.
- The element is returned to its EXACT home on dock: same parent, same position
among its siblings.
- The plugin's code still runs in the main window. So a document.body
.appendChild() inside a panel (a tooltip, a popover) lands in the main
window, not the pane — anchor to the panel instead. And a continuously
animating panel may run slowly while the main window is backgrounded, since
its rAF lives there. Both documented.
Signed-off-by: topkoa <topkoa@gmail.com>
|
||
|
topkoa
|
330995588c |
feat(panes): panes.state(id) — let a plugin apply its own pane's values
mirrorGlobal covers the case where a pane drives a plain global that some renderer reads each frame. It does not cover the far more common one: a plugin whose MAIN-realm code is the authority — it clamps, it persists, it emits events, it owns the audio graph or the camera rig — and which must therefore APPLY the pane's values itself rather than have core splat them somewhere. Camera Director is the case that forced this. Its brain is the sole writer of the camera store, the sole broadcaster on splitscreen's channel, and the only thing that clamps an axis to its legal range. A pane cannot write window.__h3dCamCtl behind its back without desynchronising its presets, its persistence, and the panel's own sliders — and running the brain inside the pane realm would make it a SECOND store writer and a second broadcaster, racing the real one. So: `panes.state(id)` hands the main realm the open pane's store (get/set/all/subscribe). A plugin seeds it on `panes:opened`, subscribes, and applies what comes back through its own API. The pane stays realm-agnostic — it only ever touches ctx.state — and the plugin stays the single source of truth. For that to work, the hub now broadcasts EVERY change to the store, not just the ones a pane asked for: it subscribes to the store on connect rather than echoing pane-originated writes by hand. A value the plugin clamps or corrects therefore reaches the pane window immediately, and there is exactly one path by which state arrives in a pane — so it cannot drift. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
254e26bb3a |
feat(panes): mirrorGlobal, manifest-declared panes, and the plugin docs
Three things a plugin needs before it can actually use panes.
## mirrorGlobal — the camera-director problem
The 3D highways read their free camera from a plain global,
`window.__h3dCamCtl` (highway_3d/FREECAM_BRIDGE.md), once per frame in
_resolveFreeCam(). A camera panel in the main window just writes that
object and the camera moves. A panel in a POP-OUT window cannot:
window.__h3dCamCtl there is a different object in a different realm, and
writing it moves nothing.
So a pane declares one field — `mirrorGlobal: '__h3dCamCtl'` — and
pane-mirror.js (main realm, where the renderers live) copies that pane's
state onto the global whenever it changes. highway_3d, keys_highway_3d
and drum_highway_3d are NOT modified and do not know panes exist.
The rule that makes it work: MUTATE THE OBJECT, NEVER REPLACE IT. A
renderer may be holding the reference, and swapping in a new object would
leave it reading an orphan. Keys the pane doesn't set are left alone
rather than deleted — the global may carry a renderer's own bookkeeping.
Closing the pane deliberately leaves the global as-is: closing the camera
panel should not snap the camera back to a default, which is exactly what
happens today (nobody clears __h3dCamCtl).
## Manifest-declared panes
"panes": [{ "id": "camera_director", "title": "Camera Director",
"script": "panes/camera.js", "mirrorGlobal": "__h3dCamCtl" }]
Declaring a pane beats calling panes.register() from screen.js because it
becomes openable FROM THE RAIL OR THE TRAY WITHOUT THE PLUGIN'S SCREEN
EVER HAVING BEEN VISITED — core registers a stub from the manifest and
fetches the script only when the user opens it. A pane you can only reach
by first navigating to the screen it was meant to replace is not much of a
pane.
The script sets `window.feedBackPane_<id> = { mount, unmount }`, mirroring
the existing window.feedBackViz_<id> convention, and the SAME file is what
a pop-out window loads in its own realm.
`script` is validated as a relpath under the plugin's src/ and served
through the sandboxed /api/plugins/<id>/src/ route — the containment rule
`styles` already has for assets/. Traversal, absolute paths, drive letters,
backslashes and non-.js are rejected; a bad entry is dropped with a warning
rather than failing the whole plugin, because one malformed pane should not
cost the user a working plugin.
Note the projection is written TWICE — _nav_entry() and the /api/plugins
route re-project independently — so panes had to be added to both, plus the
pending branch (a pane can be opened while its plugin is still installing
deps; the script is fetched on open, not at discovery).
## docs/plugin-panes.md
The contract, and the one rule it all hangs on: mount(root, ctx) runs in a
realm that may not have the app in it. Everything comes through ctx, or the
pane works docked and silently dies popped out.
Verified: manifest validation rejects ../.., C:\, non-.js, dupes and
missing fields while passing a good entry; /api/plugins projects panes[] for
all 20 plugins. mirrorGlobal mutates the global IN PLACE — a reference held
the way _resolveFreeCam holds it sees the change, and a renderer's own field
on that object survives — both for a local write and for a write arriving
over the channel from a pop-out realm.
pytest: 2401 passed, 8 failed — all 8 reproduce on a clean main (including
the one in tests/test_plugins.py) and are unrelated.
Signed-off-by: topkoa <topkoa@gmail.com>
|
||
|
topkoa
|
d508380532 |
feat(panes): desktop host — real windows and the system tray
Registers a `desktop` pane host at priority 20, above the browser pop-up host (10) and the dock (0), whenever the Electron bridge exposes feedBackDesktop.panes. A popped-out pane then gets a real BrowserWindow: it remembers where you put it, can float above everything, minimizes to the system tray, and appears in the tray's menu. In a plain browser — or on an older desktop build that predates the bridge — this file registers nothing and the browser pop-up host handles detach exactly as before. Nothing else in the pane system changes. That is what the host registry is for. Two things only this realm can decide, so it owns them: - The user closed a pane window (or it crashed). Close the pane, or the dialog its pop-out chip hid never comes back and the user is left with no way to reach their own UI. - The tray asked to toggle a pane it has no window for. Main cannot know what opening one means — the pane might belong in the dock — so it asks. Unlike a browser pop-up, this host needs no user gesture, so it sets autoRestore: true — a pane you left popped out comes back popped out, where you left it, on the next launch. Pairs with got-feedback/feedBack-desktop#103. Signed-off-by: topkoa <topkoa@gmail.com> |
||
|
topkoa
|
fefb9051a4 |
feat(panes): pop-out windows — the pane realm, hub, and remote transport
A pane can now leave the main window entirely. Same `mount(root, ctx)`,
same file, different JS realm — which is what the ctx-only contract in the
previous commit was for.
## A purpose-built document, not the app shell with a flag on it
`GET /pane` serves static/panes/pane.html: the bridge, the runtime, and
the pane's own script. No highway, no library, no v3 shell, no <audio>,
no Tailwind.
The splitscreen follower takes the other road — it reloads the whole app
at `/?ssFollower=1` and hides what it doesn't want — and pays for it with
an anti-flash block that must run before any script parses (index.html),
bail-outs in app.js and shell.js, and ~40 lines of CSS hiding core
elements by id. It loads the entire app to throw it away. A pane window
has nothing to throw away, so it boots in milliseconds and there is
nothing to flash.
The cost is that `window.feedBack` in a pane realm is a deliberate,
documented SUBSET. The runtime installs exactly what a pane is promised —
`panes.register`, and the no-op chip/dock calls a shared script may make
at load — so a pane reaching for something it was never given fails
loudly at authoring time instead of subtly at runtime.
## The channel
BroadcastChannel('feedback-panes'), same origin. This works only because
Electron's setWindowOpenHandler returns `action: 'allow'` for same-origin
URLs: `deny` would push the window to the system browser, a different
Chromium instance, where BroadcastChannel cannot reach it and the pane
would silently never sync. That flag is load-bearing.
hello -> snapshot resync-on-open, always. The snapshot is the only way
the pane realm learns anything.
state main is authoritative. A pane's write is a REQUEST;
main applies it and echoes to every realm, so a
losing write self-corrects instead of splitting brain.
rpc / rpc:reply ctx.call() -> the capability bus, with a 10s deadline.
Without one, a main window that died mid-call leaves
the pane's promise pending forever.
event allowlisted bus events, JSON-safe. A CustomEvent
carrying a DOM node (highway:canvas-replaced does)
would throw on postMessage and take the channel down
for everyone, so detail is round-tripped through JSON.
stream one coalesced message per pane per frame, OVERWRITING
anything not yet flushed. Queueing would build a
backlog: Chromium throttles a backgrounded window, and
the main window is exactly what's backgrounded while
the user looks at the pane.
sub / unsub refcounts the main-realm sampler.
bye both directions.
## The follower clock
The pane extrapolates between broadcasts: anchor + observedRate * elapsed,
capped at 2s. observedRate is learned from the broadcasts themselves
(dt/dwall) so it tracks the speed slider without being told about it, and
seeks/pauses are excluded from the fit — a jump is not a tempo. Capping it
means a dead main window decays into a frozen clock rather than one that
confidently runs away. This is splitscreen's hard-won trick, generalized:
panes just call ctx.playhead().
## Failure modes, all of them
- Main window closes -> `bye {main-closed}` and the pane says so plainly,
rather than showing a frozen playhead that looks live. The host also
closes its windows outright; a pane that cannot be fed should not be on
screen.
- Pane window X'd or crashed -> a `closed` poll reaps it (a crashed
renderer never sends `bye`), the pane closes, and the chip's dialog comes
back. Without this the user's dialog stays hidden with no way back.
- Popup blocked -> a toast, and we bail BEFORE the manager records
anything, so the caller's dialog stays exactly where it was.
- Nobody answers `hello` in 5s -> the window says so instead of spinning.
- A pane with no `script` is a closure in this realm and cannot honestly
cross a window boundary. The window host declines it (canHost) and the
router falls back to the dock.
- A browser blocks window.open() outside a user gesture, so a popped-out
pane cannot be auto-restored on page load — it would only ever produce a
"blocked" toast. Such a pane comes back in the DOCK, and the chip pops it
out again on the next click. (autoRestore: false. The desktop host will
set it true.)
Hosts may now declare `remote: true`, meaning the pane's mount() runs in
another realm: the manager then owns only the authoritative state store and
never calls mount() itself. That is the seam the Electron BrowserWindow +
tray host drops into next, with no change here.
Verified: popped Now Playing and Mixer into real windows. The pane realm has
no window.highway, no capability bus and no <audio>, yet the Mixer renders
its faders via ctx.call('audio-mix','list-faders') across the channel — and
dragging that fader IN THE PANE WINDOW moved the main window's song volume
to 55 and persisted it. Closing the pane window un-hid the mixer dialog,
removed the stub and restored the chip, while the other pane window stayed
open.
Signed-off-by: topkoa <topkoa@gmail.com>
|
||
|
Byron GamatosandGitHub
|
e2215df753 | feat(career): bundle dive bar venue pack (#927) | ||
|
topkoa
|
e5cbea2e9f |
feat(panes): core detachable pane system + pop-out chip
The option-heavy player UIs (mixer, camera director, viz, audio routing)
all live in the rail popovers, which are exclusive: openPopFor() closes
the last one before opening the next. You cannot watch the mixer while
riding the camera, and both vanish the moment you look at the highway.
Add `window.feedBack.panes` — a core registry for live UI that is
authored once as `mount(root, ctx)` and hosted anywhere. Panes are
non-exclusive, and they survive song switches structurally: the dock is a
body child outside every .screen, so the per-song teardown never sees it.
The adoption cost for a plugin is two calls:
feedBack.panes.register({ id, title, icon, mount, unmount });
feedBack.panes.attachChip(myExistingDialogEl, id);
attachChip injects THE standard pop-out chip. Clicking it opens the pane
and hides the plugin's dialog, leaving a stub to bring it back. Core owns
the hide/restore, so every plugin's pop-out looks and behaves the same —
which is the point. It hides via a dedicated .fb-pane-detached class, not
.hidden/[hidden], because the dialogs we attach to already toggle those.
Everything a pane may touch arrives through `ctx` — never a global. That
is what will let the same mount() run inside a pop-out window, a separate
JS realm with no window.feedBack, no window.highway and no audio graph:
ctx.call(domain, cmd, payload) -> the capability bus
ctx.on(event, fn) -> the feedBack bus (allowlisted)
ctx.subscribe(stream, fn) -> playhead / meters
ctx.state.get/set -> persisted, main realm is the only writer
ctx.playhead(), ctx.song(), ctx.toast(), ctx.close()
ctx tracks every subscription it hands out and drops them on unmount, so
a pane cannot leak listeners across a dock/undock cycle.
Streams exist because an AnalyserNode cannot cross a window boundary:
levels are reduced to numbers in the realm that owns the audio graph.
One shared rAF loop, refcounted against live subscriptions, dirty-checked
before fan-out, and stopped dead when the last pane closes.
Hosts register themselves with the manager rather than being imported by
it — the dock lands at priority 0 (the floor, always available), so the
OS pane window can drop in later without this code changing.
Ships two built-in panes: Now Playing (the reference pane — reads the bus,
a stream, and levels, and touches no globals) and Mixer (the same faders
as the rail, via ctx.call('audio-mix', ...), with the chip attached to the
real #mixer-control). Plus a "Panes" rail popover to open panes that have
no dialog of their own; the system tray will mirror that list.
Note the dock sits at z-index 110, not on the docs/plugin-v3-ui.md ladder
(transport 20, rail 30, popovers 40) — those live INSIDE #player's
stacking context, and #player is itself fixed at z-index 100. A dock below
100 is invisible on the one screen panes exist for. Body-level ladder:
#player 100 < dock 110 < toasts 120 < modals 200.
Pop-out windows, the system tray, manifest-declared panes and mirrorGlobal
(the window.__h3dCamCtl proxy the camera director needs) follow.
Signed-off-by: topkoa <topkoa@gmail.com>
|
||
|
|
ea0ca94742 |
feat(career): career plugin — stars, venue tiers, pack downloads (career mode 2/3) (#907)
* feat(career): career plugin — stars from song_stats, venue tiers, pack downloads (career mode PR2) Bundled plugin: per-song stars from best_accuracy (60/75/85% → 1/2/3★), cumulative stars unlock bar → club → arena (data-driven venues.json). Venue packs (UE-rendered crowd loops) download on demand to CONFIG_DIR/plugin_uploads/career/ on a background thread with sha256 + zip-slip validation, served via FileResponse. Career screen (promoted sidebar entry) shows progress and pushes the active venue's manifest into the crowd video layer (v3VenueCrowd, PR1) — degrades cleanly when either side is absent. Pack URLs land in venues.json in PR3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(career): keep manifest cleanup path alive on delete; badge only for installed venues Codex preflight: nulling _appliedManifestVenue on delete skipped pushCrowdManifest's setManifest(null) cleanup, leaving the crowd layer on a deleted pack; and the 'playing here' badge showed for an override venue whose pack was removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(career): generation-guard in-flight manifest fetches Codex preflight: a manifest fetch resolving after a newer refresh (pack deleted, venue switched) could re-apply a stale pack over the user's newer selection — fetches now carry a generation token and bail when superseded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(career): exclude orphaned song_stats from star totals Codex preflight: scans hide rather than delete stats of removed songs, so stars now apply the same existing-song filter other stats surfaces use (filename IN (SELECT filename FROM songs)). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(career): 50/150 star thresholds + star collection overview Byron's progression tuning: club at 50★, arena at 150★. /state now returns star_detail rows (title/artist joined from the library, stars, best accuracy, next-star threshold) sorted closest-to-next-star first, and the career screen renders a collection panel: tier summary plus a per-song list with a 'N% to next star' practice hint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(career): venue select/unselect UX, intro manifest support, fullmatch guards - 'Play here' now also defaults the visualization to Venue (remembering the prior viz); active venues show 'Leave venue' which restores it and sets the '__none__' override so no installed venue silently reapplies. - Pack manifests may ship an intro block (flyover video + ambience mp3); files validate like loops/stingers, .mp3 added to the serving whitelist. - Codex preflight: whitelist regexes use fullmatch (trailing-newline names could validate but 500 on serving). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(career): let pushCrowdManifest clear the manifest on Leave venue Codex preflight: nulling _appliedManifestVenue before refresh skipped the setManifest(null) cleanup branch, leaving the crowd playing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(career): refresh tailwind output --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
e779c72396 |
feat(venue): reactive crowd video layer behind the 3D highway (career mode 1/3) (#905)
* feat(venue): reactive crowd video layer behind the 3D highway (career mode PR1) Two crossfading video backdrop planes in the highway_3d venue background style, driven by a new venue-crowd.js state machine that maps v3:live-performance-state to crowd states (bored/neutral/engaged/ecstatic) with 3s stability + 8s dwell hysteresis, plus one-shot reaction stingers on streak milestones and end-of-song accuracy. Inert without a venue pack manifest (career plugin, PR2) or the feedBack-venue-crowd-dev flag — the static bg plate behaves exactly as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): retry renderer binding + preserve mid-stinger transitions Codex preflight P2s: (1) videos created before highway_3d registered its globals never reached the backdrop planes — binding is now idempotent and retried from start/perf-event/re-activation paths; (2) a crowd-state switch committing while a stinger played was dropped because the machine had already advanced — it is now deferred and played when the stinger ends. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): per-video load tokens + unbind renderer on stop Codex preflight round 2: (1) the global load token let a stinger cancel a committed loop load on the other layer — tokens are now per-element, and a stinger preempting an in-flight loop on its own layer requeues that loop for when the stinger ends; (2) setManifest(null)/deactivate left the last crowd frame bound and visible over the static plate — stop() now unbinds both layers from the renderer and zeroes the mix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): flush deferred loop on stinger failure, source accuracy from perf events Codex preflight round 3: (1) a failed/timed-out stinger left a deferred loop switch queued forever; the failure path now flushes it. (2) stats:recorded only carries {filename, arrangement}, so the end-of-song reaction now uses the accuracyPct from the song's last v3:live-performance-state event (a real percentage) instead of a field that never existed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): requeue mid-crossfade loops preempted by stingers; hard-stop on manifest swap Codex preflight round 4: (1) idleLayer() still points at the fading-in layer during a crossfade, so a stinger firing mid-fade overwrote the new loop with nothing requeued — the fading loop is now tracked and requeued like an in-flight load; (2) swapping venue packs while active now goes through stop() so _stopGen invalidates the old manifest's in-flight loads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): generation-gate stinger handlers; recrop on video size change Codex preflight round 5: (1) an ended/timeout handler orphaned by stop() could fire into a later stinger's lifecycle on the reused element — handlers now detach unconditionally and carry a generation token; (2) the renderer only re-applied cover-crop on camera aspect changes, so a src swap with a different intrinsic size kept stale repeat/offset — it now recrops when videoWidth/Height change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): bail loop-fade completion when a stinger preempted the layer Codex preflight round 6: the loop crossfade's completion callback could still run between a stinger's start and its canplaythrough, promoting the stinger's layer to active and pausing the real loop — it now bails when the fading loop was preempted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): keep rear video layer opaque during crossfades Two half-transparent layers let the static bg plate bleed through (~25% at mid-fade) — visible as a flash of the old still image on every state transition. The crossfade is now always the front layer fading over an opaque rear layer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): reset active layer with mix on stop Codex preflight: stop() zeroed the mix but left _activeLayer at 1, so a restart flashed layer 0's stale frame until the new loop loaded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): reset crowd mood to neutral on song load Codex preflight: a song ending in ecstatic/bored left the next song's crowd stuck in that mood until the hysteresis window passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): cancel in-flight fade when a stinger preempts it Codex preflight: the orphaned ramp kept pushing the mix toward the layer whose src the stinger had just replaced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): don't let null accuracy resets wipe the end-of-song value Codex preflight: Number(null) is 0, so idle HUD resets overwrote _lastAccuracyPct before stats:recorded consumed it, suppressing the end-of-song stinger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): abort stale stinger state on song load Codex preflight: a stinger straddling a song change could fade back into the previous song's layer or flush its pending loop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): always detach load listeners, gate only the callback Codex preflight: superseded loads left canplaythrough/error listeners attached to the persistent video elements — unbounded growth over a session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(venue-crowd): flyover intro with crowd-ambience ducking On song:loaded, an optional pack intro plays once: a camera flyover video (idle layer, one-shot) with bar-crowd ambience audio that ducks out on song:play, near the flyover's landing, or at handoff — whichever first. Machine commits and stingers defer during the intro; stop()/song-change abort it. Packs without an intro behave as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(venue-crowd): fall back to the loop when the intro fails to load Codex preflight: a failed/timed-out intro left the song with no crowd loop at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
ChrisBeWithYouandClaude Fable 5
|
ffc52f13ce |
Harden freqs_to_midis against NaN/Inf; badges read exact tuningMidis
Follow-ups to #829 (CodeRabbit's review nit + the consumer adoption the PR body promised): - freqs_to_midis: reject non-finite frequencies (NaN/Infinity) — a provider handing one through would otherwise raise inside int(round(...)) and 500 GET /api/tunings. Tests cover nan/inf/-inf alongside the existing garbage cases. - v3 instrument badge: TUNING_NOTE now prefers the exact integer midis the server serves (tuningMidis) over reconstructing the note from the lowest string's frequency via log2 against a hardcoded 440 — which can land a semitone off at non-440 reference pitches. Frequency path kept as the fallback for older cached responses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> |
||
|
|
8d3db5f42c |
fix(nav): nobody may monkey-patch window.showScreen — add screen:changing, make the shell listen (#924) (#925)
ship-ci / ci (push) Waiting to run
* fix(nav): the library sometimes showed the legacy screen — map 'home' inside showScreen
Testers: "randomly, when moving to the library from another menu option, the library shows the
old interface — never when a song ends."
━━━ WHAT WAS ACTUALLY HAPPENING ━━━
#home is the PRE-V3 library screen. The v3 shell replaced it with #v3-songs, and the mapping DID
exist — but only inside WRAPPERS on window.showScreen, and only for callers that go through
`window`. THREE independent parties monkey-patch it, each capturing whatever happens to be there
at the time:
app.js publishes the raw function
-> shell.js wraps it, adding the home -> v3-songs mapping
-> the stems plugin wraps it AGAIN (src/main.js:1029), capturing the current value
Plugins load ASYNCHRONOUSLY. The chain links up in whatever order the race settles, and any
capture taken before shell.js installs — or any re-assignment after it — silently drops the
mapping. Hence "randomly".
AND THE INTERNAL CALLERS NEVER TOUCHED window.showScreen AT ALL. closeCurrentSong and the
Esc-from-settings shortcut call the IMPORTED showScreen, which no wrapper ever sees. Reproduced
in a browser: the unwrapped function with 'home' lands on the dead legacy screen EVERY time.
"Never when a song ends" is the tell, and it is what identified the mechanism: closeCurrentSong
resolves its target through _resolvePlayerOrigin(), which ALREADY applies this mapping. That one
path was fine — which is exactly why the bug looked random rather than total.
PRE-EXISTING, not a regression from the module carve: the onclick="showScreen('home')" links and
the wrapper-only mapping both date to 2026-06-22.
━━━ THE FIX ━━━
The guard lives inside showScreen now: ONE place, in the function every caller routes through,
instead of a chain of monkey-patches that must each remember. Wrapper order stops mattering, and
the module-internal callers are covered for the first time.
Verified in a browser: the raw, unwrapped showScreen('home') — which reproduced as #home — now
lands on #v3-songs, and cannot be undone by any wrapper order.
━━━ AND A [P1] I INTRODUCED, WHICH CODEX CAUGHT ━━━
My first cut mapped BOTH 'home' and 'v3-home', copied straight from _resolvePlayerOrigin.
That is correct THERE and wrong HERE. _resolvePlayerOrigin computes where to RETURN TO after a
song, and landing on the Songs list from the dashboard is the right behaviour. But #v3-home is
the v3 DASHBOARD — a real screen that the shell's Home nav, the onboarding tour and the dashboard
re-render listener all target. Redirecting it would have made Home unreachable.
A LEGACY ALIAS IS NOT THE SAME THING AS A RETURN TARGET. Only 'home' is mapped now, and a test
pins that: re-adding 'v3-home' to the guard fails it.
4 tests, bite-tested both ways.
node 1049, pytest 2425, ESLint 0, Codex 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(nav): nobody may monkey-patch window.showScreen — add screen:changing, make the shell listen (#924)
window.showScreen was wrapped by THREE independent parties, each capturing whatever happened to be
there at the time:
app.js publishes the raw function
-> static/v3/shell.js wrapped it (to call syncActive, and to map home -> v3-songs)
-> the stems plugin wrapped it AGAIN (to tear down on leaving the player)
Plugins load ASYNCHRONOUSLY, so the chain linked up in whatever order the race settled. A capture
taken before shell.js installed silently dropped the mapping it carried — and the library opened on
the dead legacy #home screen. Testers saw that as "randomly, the library shows the old interface"
(#923).
#923 fixed the symptom by moving the mapping inside showScreen. This removes the CAUSE: neither
wrapper ever needed to be one.
━━━ TWO EVENTS, AND THE DISTINCTION IS THE WHOLE POINT ━━━
screen:changing emitted BEFORE anything happens. "I am leaving `from`." Teardown/cancel here.
screen:changed emitted after the DOM and data settle. "I am on `id`." Now carries `from`.
screen:changing is new, and it exists because Codex caught me collapsing the two. The stems plugin
tore down its audio graph BEFORE showScreen did anything; screen:changed fires at the very END,
after core awaits library and provider loads — so moving the plugin onto it would have delayed
teardown behind a slow fetch, or skipped it entirely if that fetch threw, and stems would keep
playing on a non-player screen. A test pins the ordering: screen:changing must precede the first
await.
shell.js is a plain screen:changed listener now, like app.js, audio-mixer.js and tour-engine.js
already were. window.showScreen is an unwrapped function again, and tests/js/
no_showscreen_monkeypatch.test.js fails CI if anything in static/ ever assigns to it again — so the
hazard is structurally impossible rather than merely avoided.
━━━ AND A FALLBACK THAT COULD NEVER FIRE ━━━
My retry-if-the-bus-is-late path listened for `slopsmith:capabilities:ready`. Core dispatches
`feedBack:capabilities:ready` (capabilities.js:1536) — the slopsmith: name is the PRE-DMCA event
and nothing has emitted it since the rename. Codex caught it. A guard that cannot fire is worse
than no guard: it reads as protection and is decoration.
(The same dead-event bug turned out to be sitting in THREE of the stems plugin's fallbacks, where
it has silently disabled its lifecycle wiring whenever the bus was late. Fixed in
feedback-plugin-stems#38.)
VERIFIED. A/B against origin/main: the nav highlight and topbar title follow IDENTICALLY with
shell.js as a listener; screen:changing -> screen:changed fire in order with the right {id, from};
window.showScreen is unwrapped; and showScreen('home') still lands on v3-songs.
node 1053, pytest 2425, ESLint 0, Codex 0.
Closes #924
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
f27d4f623c |
fix(nav): the library sometimes showed the legacy screen — map 'home' inside showScreen (#923)
Testers: "randomly, when moving to the library from another menu option, the library shows the
old interface — never when a song ends."
━━━ WHAT WAS ACTUALLY HAPPENING ━━━
#home is the PRE-V3 library screen. The v3 shell replaced it with #v3-songs, and the mapping DID
exist — but only inside WRAPPERS on window.showScreen, and only for callers that go through
`window`. THREE independent parties monkey-patch it, each capturing whatever happens to be there
at the time:
app.js publishes the raw function
-> shell.js wraps it, adding the home -> v3-songs mapping
-> the stems plugin wraps it AGAIN (src/main.js:1029), capturing the current value
Plugins load ASYNCHRONOUSLY. The chain links up in whatever order the race settles, and any
capture taken before shell.js installs — or any re-assignment after it — silently drops the
mapping. Hence "randomly".
AND THE INTERNAL CALLERS NEVER TOUCHED window.showScreen AT ALL. closeCurrentSong and the
Esc-from-settings shortcut call the IMPORTED showScreen, which no wrapper ever sees. Reproduced
in a browser: the unwrapped function with 'home' lands on the dead legacy screen EVERY time.
"Never when a song ends" is the tell, and it is what identified the mechanism: closeCurrentSong
resolves its target through _resolvePlayerOrigin(), which ALREADY applies this mapping. That one
path was fine — which is exactly why the bug looked random rather than total.
PRE-EXISTING, not a regression from the module carve: the onclick="showScreen('home')" links and
the wrapper-only mapping both date to 2026-06-22.
━━━ THE FIX ━━━
The guard lives inside showScreen now: ONE place, in the function every caller routes through,
instead of a chain of monkey-patches that must each remember. Wrapper order stops mattering, and
the module-internal callers are covered for the first time.
Verified in a browser: the raw, unwrapped showScreen('home') — which reproduced as #home — now
lands on #v3-songs, and cannot be undone by any wrapper order.
━━━ AND A [P1] I INTRODUCED, WHICH CODEX CAUGHT ━━━
My first cut mapped BOTH 'home' and 'v3-home', copied straight from _resolvePlayerOrigin.
That is correct THERE and wrong HERE. _resolvePlayerOrigin computes where to RETURN TO after a
song, and landing on the Songs list from the dashboard is the right behaviour. But #v3-home is
the v3 DASHBOARD — a real screen that the shell's Home nav, the onboarding tour and the dashboard
re-render listener all target. Redirecting it would have made Home unreachable.
A LEGACY ALIAS IS NOT THE SAME THING AS A RETURN TARGET. Only 'home' is mapped now, and a test
pins that: re-adding 'v3-home' to the guard fails it.
4 tests, bite-tested both ways.
node 1049, pytest 2425, ESLint 0, Codex 0.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
57e7db5c2a |
refactor(app): carve the keyboard-shortcuts subsystem into static/js/shortcuts.js (R3d) (#922)
19 declarations + 23 TOP-LEVEL STATEMENTS. 922 lines. app.js 3,243 -> 2,325 (-28%). The panel registry, both global keydown dispatchers, the library arrow-nav, and the whole plugin-facing shortcut API. ━━━ MOST OF THIS SUBSYSTEM WAS NOT DECLARATIONS ━━━ A declaration-seeded dependency closure reports this cluster as 10 names, 246 lines. It is 42 statements and 922. window.registerShortcut, createShortcutPanel, getAllShortcuts, unregisterShortcut, clearWindowShortcuts, the panel registry, and BOTH global keydown dispatchers are bare TOP-LEVEL STATEMENTS at app.js's top level. A call-graph scan sees NONE of them. That blind spot has now cost three times: * it nearly shipped a dead library A-Z rail (#896) — 43 of library.js's exports were referenced only from app.js's window contract; * it threw "Assignment to constant variable" in the session carve (#921), where the autoplay gate's top-level statements wrote state that had just become a read-only import; * and here it under-reported the slice by 3x. The extractor takes them by construction now — any top-level statement that TOUCHES a moved binding comes along — and the SEED is closed to a FIXED POINT, because those statements have their own dependencies (_modifiersMatch, _isShortcutActive, _handleLibArrowNav, _gridColumns…) that the declaration closure never walked. Seed -> pull the statements -> the statements need more names -> re-seed. Iterate until it stops growing. ━━━ syncLibrarySong GOES ACROSS THE SEAM, NOT THROUGH AN IMPORT ━━━ The library arrow-nav calls it on Enter. It cannot be imported: syncLibrarySong reaches showScreen/playSong, and a module importing app.js closes a cycle. It is the ONE name here that had to stay behind, so it comes across the host seam — which is exactly what the seam is for. host.js throws loudly if the wiring is ever dropped, and tests/js/host_contract.test.js fails in CI if the hook drifts. VERIFIED. A/B against origin/main in two browsers, IDENTICAL, zero page errors — and driven for real, not merely present: the plugin API (register / unregister / getAll / panels), THE GLOBAL KEYDOWN DISPATCHER actually firing a registered shortcut, that same shortcut correctly SUPPRESSED while typing in a text input, and `?` opening the help modal. node 1045, pytest 2425, ESLint 0 (no-cycle clean), host contract 2/2, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
545e569ad6 |
refactor(app): carve the song session out of app.js — playSong, showScreen, closeCurrentSong (R3d) (#921)
36 declarations + the 4 autoplay/auto-exit gate statements. 359 lines. app.js 3,772 -> 3,242. Bodies VERBATIM. ━━━ THIS WAS "THE UNCUTTABLE HEART", AND IT IS 359 LINES ━━━ At the start of this epic, seeding a dependency closure from count-in, from loops, from section-practice, or from the JUCE seek shim all returned the SAME 178-function, 3,360-line set. playSong and showScreen called each other; everything called them; nothing could be cut anywhere. The conclusion — correct at the time — was that NO closure-based carve could touch it at any seed, and the answer was a host seam. That was true THEN. Every slice taken out since (transport, loops, count-in, section-practice, the library, the edit modal, settings) removed edges, and the strongly-connected component DISSOLVED. This closure is 36 declarations with an interface width of FOUR. The lesson is not that the seam was wrong — the seam is what MADE this possible, by letting the carves proceed against a cyclic core instead of stalling on it. The lesson is to RE-MEASURE. An SCC is a fact about a graph at a moment, not a property of the code. ━━━ THE BUG NO SCAN COULD SEE, AND THE A/B DID ━━━ First cut passed every gate — no-undef clean, no-cycle clean, 1045/1045, pytest green — and THREW IN THE BROWSER: "Assignment to constant variable." window.feedBack.holdAutoplay / holdAutoExit and their two event handlers are TOP-LEVEL STATEMENTS, not declarations. They WRITE this cluster's state (_autoplayHeld, _autoExitTimer, …), and an imported binding is READ-ONLY — so left behind in app.js, every one threw the instant the module existed. A dependency scan that walks DECLARATIONS cannot see them. Mine didn't. This is the same blind spot that nearly shipped a dead library A-Z rail (#896): app.js keeps its public API in top-level statements, and a call-graph is blind to every one of them. The extractor now finds them by construction — any top-level statement that WRITES a moved binding comes with the carve — and the gate statements live beside the machinery they drive, which is where they belonged anyway. ━━━ ZERO OUTSIDE WRITES, BY MOVING THE BOUNDARY RATHER THAN BUILDING MACHINERY ━━━ The autoplay scalars and the wake-lock state were written from outside the cluster, which would have forced a setter or a state container. But the writers — _releaseAutoplay, _acquireWakeLock — plainly belong here. Pulling them in left ZERO outside writes, so every export is a plain import. Same move as settings (#920): measure the writers before you reach for a container. VERIFIED. A/B against origin/main in two browsers, IDENTICAL, zero page errors — including the autoplay gate driven end to end: a plugin HOLDS autoplay, the song loads but does not start, the RELEASE fires it, and a stale release is a no-op. That is the exact machinery that was throwing. node 1045, pytest 2425, ESLint 0 (no-cycle clean), host contract 2/2, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
84fe29688c |
refactor(app): carve settings into static/js/settings.js (R3d) (#920)
22 declarations, 446 lines. app.js 4,218 -> 3,772. Bodies VERBATIM. Settings load/save, the AV-offset nudge, the default-arrangement pin, the instrument pathway, and the app-update channel. ━━━ INTERFACE WIDTH 1, AND IT GOT THERE BY DRAWING THE BOUNDARY IN THE RIGHT PLACE ━━━ app.js calls loadSettings() and nothing else. The first cut was NOT clean: _defaultArrangement was written from OUTSIDE the cluster, and an imported binding is READ-ONLY, so that one write would have forced a setter or a state container — as it did for the player (player-state.js) and the library (library-state.js). But the writers were saveSettings and pinCurrentArrangementDefault, which ARE settings functions. Widening the slice to include them left ZERO outside writes. Every export is now a plain read-only import and no container is needed. Worth naming, because I reached for a container twice before: the fix for "this binding is written from outside" is sometimes a container, and sometimes it just means the boundary is in the wrong place. Measure the writers before you build machinery. ━━━ handleSliderInput STAYS A HOST HOOK, DELIBERATELY ━━━ It lives in settings now (it is a settings control), but player-controls.js must NOT import it: this module already imports player-controls (_applyMastery, _autoplayExitEnabled, …), so a direct back-import would close a cycle. player-controls keeps reading it through the host seam, and app.js — the root, which imports both — wires it. That is exactly what the seam is for, and the contract test proves the wiring survived. VERIFIED. A/B against origin/main in two browsers: the window contract, the settings screen rendering, the AV-offset and default-arrangement controls present, and a real `input` event dispatched on a slider — which is the path that goes through the host seam. IDENTICAL, zero page errors. node 1045, pytest 2425, ESLint 0 (no-cycle clean), host contract 2/2, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
69aac32278 |
refactor(app): carve the edit-song modal into static/js/edit-modal.js (R3d) (#919)
4 functions, 234 lines. app.js 4,452 -> 4,218. Bodies VERBATIM. INTERFACE WIDTH ZERO — nothing in app.js calls into this cluster. app.js needs only the names on the window contract, so the markup's onclick= handlers resolve. That is what makes it the cleanest slice left. AND IT ONLY BECAME CLEAN BECAUSE THE LIBRARY CAME OUT FIRST (#896). Every dependency the modal has is a module now: it reads six bindings out of ./library.js (loadLibrary, loadFavorites, loadTreeView, _removeLibCardsForFilename, libView, _lastLibSelected) plus dom.js and the L container. Before that carve, extracting this would have dragged the whole library with it. Checked, and it matters: the modal never WRITES any of those six. An imported binding is READ-ONLY, so a single write would have forced a setter or a state container. Every use is a read, so plain imports suffice. Acyclic: edit-modal -> { dom, library-state, library }, and library imports none of them back. VERIFIED. A/B against origin/main in two browsers: the window contract, the modal actually OPENING off a real library row, its title and year fields rendering, and the data-edit-save wiring (rather than an inline onclick embedding the filename — the fix this cluster's harness exists to guard). IDENTICAL, no new page errors. node 1045, pytest 2425, ESLint 0 (no-cycle clean), host contract 2/2, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
0a6e0309e5 |
fix(tailwind): stop the dev server rewriting a tracked file (#911) (#918)
The runtime stylesheet moves to CONFIG_DIR. static/tailwind.min.css is never written again.
━━━ TWO DIFFERENT THINGS WERE SHARING ONE PATH ━━━
static/tailwind.min.css a BUILD ARTEFACT. Committed, image-baked, generated by scanning
the in-tree plugins only. CI's tailwind-fresh check verifies it.
the RUNTIME sheet PER-INSTALL STATE. Additionally scans whatever the user installed
into FEEDBACK_PLUGINS_DIR, so it differs machine to machine.
Writing the second over the first meant that MERELY RUNNING THE DEV SERVER from a git checkout
silently modified a tracked file. `git add -A` then swept a 100KB reshuffle of minified CSS
into the commit and ci/tailwind-fresh went red with a diff that explains nothing — on a PR
whose real change touched no Tailwind classes at all. It also wrote app state into the app
directory, which is read-only in some deploys.
A new route serves the runtime sheet when there is one and falls back to the committed one
otherwise. It is registered BEFORE the /static mount, which would otherwise swallow the path.
━━━ A PERSISTED SHEET MUST NOT OUTLIVE ITS REASON (Codex [P2] x2) ━━━
1. THE USER REMOVES THEIR PLUGINS. Startup only rebuilds when user plugins exist, so nothing
would ever overwrite the stale sheet — and it still carries classes for plugins that are
gone. With no user plugins the COMMITTED sheet is complete by definition. Guarded.
2. THE APP IS UPGRADED, and my first guard for this was WRONG. I compared mtimes. Codex: that
is not a freshness signal across install methods — archives and container images routinely
PRESERVE SOURCE MTIMES, so a just-shipped stylesheet can carry an OLDER timestamp than a
runtime sheet a user built days ago. The mtime check then calls the stale one FRESH and it
masks the new core CSS indefinitely — permanently, if no Tailwind toolchain is present to
trigger a rebuild.
Freshness is decided by CONTENT now. Each runtime build stamps a sidecar with the sha256 of
the committed sheet it was made from. Core ships new CSS -> that file changes -> the hash
changes -> the runtime sheet is correctly judged stale. Timestamps only gesture at the
question that hashing answers.
Falling back to the committed sheet is always safe: at worst it lacks a just-installed plugin's
classes for the seconds until the async rebuild lands.
VERIFIED END TO END. Ran the real dev server with 3 plugins installed: it rebuilt Tailwind over
them (123,291 bytes), wrote the sheet + sidecar to CONFIG_DIR, still served /static/
tailwind.min.css at 200 — and `git diff` on the tracked file came back CLEAN.
8 tests. Bite-tested: reverting to the shared path fails 3, dropping the staleness guards fails
2 more.
pytest 2425, pyflakes 0, Codex 0.
Closes #911
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
36cf77dc44 |
refactor(highway): carve the 2D drawing layer into highway-draw.js (R3c) (#917)
18 functions, 1,245 lines. highway.js 3,972 -> 2,727 (-31%). The biggest R3c slice: notes,
sustains, chords, strum groups, unison bends and lyrics — everything the default renderer
paints each frame.
━━━ MUTABILITY, NOT LOCATION, DECIDES WHERE A THING BELONGS ━━━
Three per-instance caches came out with this slice, and they are why it needed care:
_frameMismatchWarned a warn-once Set of chord ids (feedBack#88)
_chordRenderInfo a WeakMap of chord -> chain info
_lyricMeasureCache Map<fontSize, Map<text, width>>
All three are MUTATED. Left at module scope they would be SHARED ACROSS PANELS — one
highway's lyric widths and chord chains stomping another's, silently, with nothing throwing.
createHighway() is a factory (the constitution publishes window.createHighway so a plugin can
build a second highway), so they are lifted onto hwState, which is exactly what hwState is for.
The shimmer LUT went the OTHER way — to MODULE scope in highway-geometry.js. It is a
deterministic xorshift table, byte-for-byte identical for every instance, so sharing it is not
merely safe but BETTER: built once for the page rather than once per panel.
Same slice, opposite directions, decided entirely by whether the thing mutates.
━━━ MY SCRIPT WAS WRONG TWICE. THE GATES CAUGHT BOTH. ━━━
1. HAND-LISTED THE MOVE SET. I listed 10 functions and missed six that drawChords needs
(_ensureChordRenderCache, bsearchChords, getChordTemplateInfo, _computeChordBox,
_updateFretLinePreview, _drawFretLineChordPreview). The no-undef gate named every one. The
set is now DERIVED from the dependency closure — 18, not 10.
2. JUDGED PURITY TOO EARLY, and this one is subtle. I classified _computeChordBox as pure
because its ORIGINAL body never mentions hwState. Then the call-site rewriter injected
`fretX(hwState, …)` INTO it — fretX takes hwState now (#916) — leaving a function that
references an hwState it was never given. Purity has to be judged from the body AS IT WILL
BE, so the classifier iterates to a fixed point: a function needs hwState if it mentions it,
OR calls anything that now takes it. That moved _computeChordBox to the stateful side.
VERIFIED. A/B against origin/main: IDENTICAL, zero page errors. The PLUGIN BUNDLE contract is
byte-identical (b.fretX arity 3, b.getNoteState arity 2, both stable references, both correct
under the old calling convention). PERF GATE PASSES AT 1.92ms against its 12ms budget — and
this is the slice that could really have cost something: the ENTIRE per-frame drawing path is
now cross-module. It costs nothing measurable.
TESTS. highway_teaching_marks follows strumGroupBuckets to the new module. The two source-shape
harnesses now read highway.js AND every static/js/highway-*.js, rather than being re-pinned at
whichever file currently holds a function — re-pinning breaks again next time, and a shape
assertion that silently stops finding its target is indistinguishable from one that passes.
node 1045, pytest 2416, ESLint 0, no-undef 0, Codex 0.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
12eb73aee9 |
refactor(highway): carve the STATEFUL primitives, threading hwState explicitly (R3c) (#916)
fretX, fillTextReadable, _noteState, _paintGemGlow -> static/js/highway-state-primitives.js.
50 call sites rewritten. highway.js 4,105 -> 3,965.
The first slice that changes signatures. Each of these four gains hwState as an explicit
FIRST PARAMETER.
━━━ hwState IS A PARAMETER, NOT AN IMPORT ━━━
createHighway() is a FACTORY. The constitution publishes window.createHighway so a plugin can
build a SECOND highway for its own panel, and highway.js says so itself. Import hwState as a
module singleton and two panels silently share one clock, one render scale, one string
palette — each driving the other. Nothing throws. The picture is just wrong, in a way no test
would catch.
(The exact opposite of the app.js carve, where player-state.js and library-state.js ARE
module singletons — correctly, because there is exactly one app. Same epic, same language,
opposite answer, decided entirely by whether the thing is a factory.)
━━━ THE PLUGIN BUNDLE NEARLY BROKE, SILENTLY ━━━
The renderer bundle hands two of these STRAIGHT TO PLUGINS:
b.fretX = fretX;
b.getNoteState = _noteState; // stable reference
highway_3d calls both EVERY FRAME, with the old arity. Handing out the new 3-arg versions
would have passed `note` where hwState belongs — no throw, no error, just wrong geometry and
wrong judgment state INSIDE A PLUGIN, which no core test would ever see. Green CI, broken 3D
highway.
So hwState is bound ONCE per instance, in the factory, and the bundle hands out those views.
A per-frame arrow would have fixed the arity and reintroduced exactly the per-frame allocation
the bundle's stable-reference contract (feedBack#254) exists to prevent. b.project needs none
of this — project() is pure and its arity never changed.
VERIFIED IN A BROWSER, against the real bundle, on both builds:
fretX arity 3 3 (NOT 4 — the bound view preserves it)
getNoteState arity 2 2
fretX(5,1,800) in 0..800 True True
getNoteState null w/o provider True True
getNoteState honours provider True True
fretX is a stable reference True True
IDENTICAL. Without the bound views fretX would have reported arity 4 and computed garbage.
Also caught on the way: my generated module imported STRING_BRIGHT_FALLBACK, a name
highway-constants.js does not export. ESLint does not flag that — but importing a name a
module does not export is a runtime SyntaxError that kills the WHOLE module. These four need
no constants at all; the import is gone.
TESTS. highway_note_state pins the signature AND the stable-reference contract — it caught the
bundle break. Retargeted at the module and the new arity; both contracts still asserted, and
the "no fresh arrow per frame" rule is now asserted explicitly rather than implied by
`getNoteState: _noteState`.
PERF GATE PASSES AT 1.94ms against its 12ms budget — fretX and _noteState are now CROSS-MODULE
calls, per note, per frame. It costs nothing measurable.
node 1045, pytest 2416, ESLint 0, Codex 0.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
1a386c272d |
refactor(highway): carve the PURE geometry primitives into highway-geometry.js (R3c) (#915)
6 functions, 53 lines. highway.js 4,158 -> 4,105. NOT ONE CALL SITE CHANGES. project, roundRect, bnvNormalizedPoints, teachingFingerLabel, teachingDegreeLabel, chordHarmonyLabels — the shared primitives every drawing function leans on. ━━━ PURITY IS THE WHOLE POINT OF THIS SLICE ━━━ Every one of these is a pure function of its arguments. None touches hwState. None closes over the canvas context — roundRect() already took `ctx` explicitly, and the rest need nothing but numbers. project() reads only the module-level constants from #914. That matters because createHighway() is a FACTORY: a plugin can build a second highway for its own panel, so anything holding per-instance state must be PASSED hwState rather than importing it, or two panels silently share one clock and palette. These six hold no state at all, so they move VERBATIM — the module boundary is invisible to every caller. The asserts are mechanical and in the extractor: it REFUSES to move a function whose body mentions hwState, or that references `ctx` without taking it as a parameter. Purity is checked, not assumed. ━━━ WHAT IS DELIBERATELY LEFT BEHIND ━━━ The four primitives that DO need hwState — fretX, fillTextReadable, _noteState, _paintGemGlow — stay in the factory for now. They need an explicit hwState parameter threaded through 53 call sites, which is a real behavioural change and belongs in its own commit rather than smuggled in beside a provably-identical move. Separating the provable from the risky is the whole discipline of this epic. TESTS. Three harnesses brace-match these functions out of the source and run them in a sandbox; they now read static/js/highway-geometry.js. `export function x` still contains `function x`, so the extractor needed no change — only the path. VERIFIED. A/B against origin/main: 15 probes IDENTICAL, zero page errors. PERF GATE PASSES AT 1.91ms against its 12ms budget — and this is the one that could plausibly have cost something: project() runs for every visible note on every frame and is now a CROSS-MODULE call. It costs nothing measurable. That is the answer #910 was built to give. node 1045, pytest 2416, ESLint 0, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
8e89b39ad3 |
refactor(highway): carve the constants into static/js/highway-constants.js (R3c) (#914)
29 constants, 190 lines. highway.js 4,267 -> 4,158. The first real slice, and the one that
every later one imports.
━━━ WHY ONLY THE CONSTANTS MAY LIVE AT MODULE SCOPE ━━━
createHighway() is a FACTORY, not a singleton. The constitution publishes
window.createHighway precisely so a plugin can build a SECOND highway for its own panel, and
highway.js already says so at the top of the closure:
// R3c: per-instance mutable state in one object, so extracted renderer/ws
// modules can close over it as a factory arg without cross-panel sharing.
So hwState — all 79 mutable properties — must NEVER become a module-level singleton: two
highways would silently share it, and one panel would drive the other's clock, scale and
colour tables. Extracted functions will take it as an ARGUMENT.
That is the OPPOSITE of the app.js carve, where a single state container (player-state.js,
library-state.js) was exactly right, because there is exactly one app. Same epic, same
language, opposite answer — because one is a singleton and the other is a factory.
These 29 are pure literals: numbers, strings and colour tables, never reassigned, never
mutated. Sharing them across instances is not merely safe, it is what you want — one copy of
the shimmer LUT bounds and the string palettes rather than one per panel. Anything with a
runtime dependency (document, window, performance, localStorage) stays in the factory;
checked, and none of these has one.
ESLint now knows static/highway.js is a module. It could not have known before this commit:
the flip (#913) changed the SCRIPT TAG, but the file had no import/export yet, so it still
parsed as a script and lint stayed green. The first `import` is what makes the config wrong.
TESTS. Four source-shape harnesses asserted `const _AUTO_SCALE_MIN = …` etc. lived in
highway.js. They now read highway.js AND every static/js/highway-*.js — deliberately, rather
than being re-pinned at whichever file currently holds a constant. Re-pinning just breaks
again on the next carve, and a source-shape assertion that silently stops finding its target
is indistinguishable from one that passes. Bite-tested: renaming two constants away fails
them.
VERIFIED. A/B against origin/main: 15 probes IDENTICAL, zero page errors. AND THE PERF GATE
PASSES AT 1.97ms against its 12ms budget — which is the point of having built it (#910)
first: these constants moved from closure scope to module scope, and V8 does not treat those
identically. It does here. Now I know rather than hope.
node 1045, pytest 2416, ESLint 0, Codex 0.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
c6963fdf30 |
refactor(highway): flip highway.js to an ES module (R3c) (#913)
Two lines. index.html: defer -> type="module". highway.js: one explicit assignment.
highway.js can now `import`, which is the whole point — the carve can begin.
━━━ THE ONE THING THE FLIP ACTUALLY BREAKS: window.createHighway ━━━
A top-level `function createHighway()` in a CLASSIC script IMPLICITLY becomes
window.createHighway. In a module it does not — module declarations are module-scoped, and
the name vanishes from the global object the instant the tag grows type="module".
The constitution names window.createHighway as PUBLIC EXTENSION CONTRACT (alongside
window.playSong / showScreen / feedBack). NOTHING IN-TREE CALLS IT. That is exactly why this
would have shipped: the only consumers are third-party plugins rendering their own highway
panel, and I cannot grep those. Green CI, green tests, and a broken plugin API.
Verified by removing the assignment and reloading:
flip WITHOUT an explicit assignment: window.createHighway === undefined <-- gone
flip WITH it: window.createHighway === function
So it is assigned explicitly now — same object, same behaviour, no longer an accident of how
the file happens to be loaded.
━━━ AND A CORRECTION TO #912 ━━━
#912 (merged) rewrote 73 bare `highway.x` -> `window.highway.x` on the stated grounds that
the flip would turn every one of them into a ReferenceError. HAVING NOW ACTUALLY FLIPPED IT,
THAT WAS WRONG. highway.js already did `window.highway = highway`, which puts the name on the
GLOBAL OBJECT — and bare-identifier resolution falls back to the global object whether or not
a lexical global binding exists. Measured on both builds: bare `highway` resolves either way.
#912 is defensible as hygiene and it does not hurt, but it was not a precondition and it fixed
no latent bug. A correction is posted on the PR so its commit message does not mislead. The
real hazard was the factory, not the instance — same class of breakage, wrong name.
ORDERING is unchanged: classic-defer and non-async type="module" share ONE post-parse
execution queue, in document order, so highway.js keeps its position at index.html:1244.
VERIFIED. A/B against origin/main: 15 probes IDENTICAL, zero page errors — window.highway,
window.createHighway, the full API surface, a real song playing, the chart clock advancing,
and the seek->setTime sync. THE PERF GATE PASSES at 1.85ms against its 12ms budget (module
evaluation costs nothing at render time), which is exactly what #910 was built to tell me.
node 1045, pytest 2416, ESLint 0, Codex 0.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
d9fa6d3f55 |
refactor(highway): make the highway global explicit before the module flip (R3c) (#912)
73 bare `highway.x` references -> `window.highway.x`, across app.js and 10 other files. Provably a NO-OP today. It is the precondition for flipping highway.js to a module. ━━━ WHY THIS HAS TO LAND FIRST ━━━ highway.js is a CLASSIC script. Its top-level `const highway = createHighway()` therefore creates a GLOBAL LEXICAL BINDING — visible as a bare name to every other classic script AND to every ES module. 73 call sites quietly rely on that. The moment highway.js becomes a module, that binding is gone. `const` in a module is module-scoped, not global. Every one of those 73 sites becomes a ReferenceError, and the flip is impossible until they say what they mean. `window.highway = highway` is already set, to the same object, on the same line. So this is an identity rewrite — verified in the browser below. ━━━ THE REWRITE BIT ME THREE TIMES. REGEX IS NOT ENOUGH FOR THIS. ━━━ 1. A SHADOWED LOCAL. capabilities/note-detection.js does `const highway = window.highway`. Its 9 bare uses are LOCAL and already correct; a blind rewrite would have emitted `const window.highway = window.highway`. Excluded. 2. HALF-CONVERTED GUARDS — the dangerous one. Six sites read `typeof highway !== 'undefined' && highway && typeof highway.setTime === 'function'`. The regex converted the CONSEQUENT and left the TEST, which is WORSE than not touching them: after the flip `typeof highway` is 'undefined', so each guard is PERMANENTLY FALSE and the code behind it silently never runs. transport.js's was the seek->setTime sync: the chart clock would have quietly desynced after every seek, with nothing failing. All six now test window.highway. 3. TWO MORE BARE REFERENCES, found by Codex [P2] and confirmed by an AST scan: app.js:3114 and :3176 use `highway && typeof window.highway.getSections === 'function'`. My grep searched for `typeof highway`, not `highway &&`. After the flip these throw, the catch swallows it, and the editor silently falls back to a ±4s edit window and arrangement 0. Regex missed a shadow, a half-conversion, and two bare reads. The final check is an AST pass that resolves scopes and reports every `highway` identifier not bound locally. It now reports ZERO. VERIFIED. A/B against origin/main in two browsers, 15 probes, IDENTICAL, zero page errors: window.highway is the same object as the bare global, the whole API surface resolves, a real song plays, the chart clock advances, getPerf().drawMs > 0 — and `seek syncs chart` passes, which is the exact guard I nearly broke in (2). node 1045, pytest 2416, ESLint 0, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
23ecddc721 |
test(highway): the R3c perf gate — measure the render loop before carving it (R3c) (#910)
highway.getPerf() (additive) + tests/browser/highway-perf-baseline.spec.ts. No behaviour change. This lands BEFORE highway.js is touched, because a perf-gated refactor without a perf gate is just a refactor. ━━━ FRAME RATE IS THE WRONG THING TO MEASURE ━━━ The highway AUTO-SCALES. When the smoothed draw cost passes _DRAW_BUDGET_HI_MS (12ms) it LOWERS THE RENDER RESOLUTION to protect the frame rate (#654). Exactly right for players — and it means a real perf regression does NOT show up as dropped frames. It shows up as a BLURRIER PICTURE at a perfectly healthy 60fps. Benchmark fps and you measure the feedback loop, not the renderer, and conclude nothing changed while the image quietly degrades. So the gate pins the scale (setRenderScale(1) + setMinRenderScale(1), which clamps autoScale to [1,1]) and measures drawMs — the renderer's own cost. None of that was reachable before: neither drawMs nor the effective scale escaped the closure. Hence getPerf(). The threshold is the app's OWN: _DRAW_BUDGET_HI_MS is the cost at which the highway itself starts sacrificing resolution in production. Exceeding it is not an arbitrary benchmark line — it is the renderer failing its own budget. Current cost ~2.2ms, so ~5x headroom: far more than headless-CI variance, far less than any regression worth shipping. ━━━ I WROTE THIS GATE WRONG THREE TIMES. EACH TIME IT PASSED. ━━━ 1. VACUOUS ASSERTION. First cut asserted "the auto-scaler wasn't forced to intervene", i.e. effectiveScale == 1. I injected a 10x regression (drawMs 2.4 -> 22.4ms, nearly DOUBLE the budget) and it PASSED. Of course it did: setMinRenderScale(1) sets the scaler's FLOOR to 1, so effectiveScale CANNOT drop below it. The very pinning that stops the scaler hiding a regression also stops it ever reporting one. A guard that cannot fail. 2. MEASURING AN IDLE RENDERER (Codex [P2]). playSong() takes ~3-4s to actually start — it is fetching and decoding stems. My "if not playing after 2s, togglePlay()" fired BEFORE autoplay, started playback, and then the app's own autoplay toggled it straight back to PAUSED. The renderer idled through the entire measurement. Now it WAITS for playback rather than racing it, and asserts the chart clock advanced DURING the sampling window — not merely at some point beforehand, which the first fix would have accepted. 3. UNENCODED FILENAME (Codex [P2]). playSong() decodes its argument before building the /ws/highway path, so every real caller passes encodeURIComponent(filename) (app.js:2879, 4137). Raw, a name containing # ? % or / yields an invalid WebSocket URL, the song never loads — and on those libraries the gate would have silently measured an idle renderer instead of failing. Every one of those bugs made the gate PASS. That is the whole hazard of a perf test: it fails safe in the wrong direction. BITE-TESTED, and this is the only reason I trust it: a 10x regression injected into the draw path FAILS the gate under live playback (22.0ms vs the 12ms budget) and the clean build passes at ~2.1ms with the chart clock advancing 5.6s across the sample. node 1045, pytest 2412, ESLint 0, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
79825af28e |
fix(demo): the janitor re-entry guard actually works now (#902) (#909)
The guard in startup_events() read:
if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
and not _DEMO_JANITOR_STARTED:
`and` binds tighter than `or`, so that is `A or (B and C)`. The not-already-started half
never ran when the env var was truthy — the only case that reaches it at all. A second
startup started a SECOND janitor thread, overwrote the handle, and shutdown then joined
only the last: the first leaked and kept firing registered hooks hourly, forever.
The guard now lives INSIDE start_janitor(). A caller cannot get operator precedence wrong
if there is nothing left for it to get wrong.
━━━ THREE WAYS TO WRITE THIS GUARD WRONG. I HIT ALL THREE. ━━━
1. NO GUARD — the original bug. Double-start, orphaned thread.
2. GUARD ON THE FLAG (`if _DEMO_JANITOR_STARTED: return`). Codex [P2]. stop_janitor()
DELIBERATELY leaves that flag True when a hook outruns its join timeout, so that a later
startup cannot spawn a janitor beside a live one. But the hook usually finishes a moment
later: the thread exits and the flag is stale. A flag-keyed guard then refuses to start a
replacement for the rest of the process — demo cleanup silently dead. (The original bug
accidentally MASKED this by always starting.)
3. GUARD ON LIVENESS ALONE (`if thread.is_alive(): return`). Codex [P2], second pass. A
timed-out stop leaves the old thread ALIVE BUT DOOMED — its stop event is set and it
exits as soon as its current hook returns. Treating that as a running janitor skips the
replacement, and we are back at (2) a second later.
So: a janitor counts as running only if its thread is alive AND it has not been told to stop.
━━━ AND EACH JANITOR NOW OWNS ITS STOP EVENT ━━━
start_janitor() used to `_DEMO_JANITOR_STOP.clear()` a single SHARED Event. Start a
replacement while a doomed thread is still finishing a hook and that clear RESURRECTS it: it
loops back to wait(), sees the flag cleared, and carries on. Two janitors — the exact bug we
started from. A fresh Event per janitor makes it impossible; the old thread waits on its own
event, which stays set, so it can only exit.
Env semantics UNCHANGED, verified across every value ("", "1", "0", "true", "false", "off"):
the old expression and demo_mode_enabled() agree on all of them. The only behavioural change
is the idempotency fix.
FOUR tests, and each of the three wrong guards fails a different subset:
no guard -> 2 fail (double start; orphaned thread)
guard on the flag -> 2 fail (never restarts after a timed-out stop)
liveness alone -> 1 fail (no replacement for a doomed janitor)
liveness + not-stopping -> all pass
pytest 2416, pyflakes 0, Codex 0.
Closes #902
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
OmikronApexandGitHub
|
db3ca34fcb |
Merge pull request #906 from got-feedBack/fix/loopback-raw-audio
ship-ci / ci (push) Waiting to run
fix(static): raw stereo loopback capture — no voice-call DSP (tin-can fix) |
||
|
OmikronApexandClaude Fable 5
|
34215fbd32 |
fix(static): request raw stereo audio for the loopback capture track
Chromium treats a getDisplayMedia audio track as a voice call by default: echo cancellation, noise suppression, auto gain control and mono downmix. Music through that pipeline is the tester-reported "tin can" sound on ASIO/exclusive outputs. Request the raw path explicitly (EC/NS/AGC off, stereo, 48 kHz) — all constraints are best-effort so unsupported ones degrade silently instead of failing the capture. The [asio-diag] loopback line now dumps track.getSettings() so logs prove which processing actually applied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
f5d448af5c |
refactor(server): carve demo mode into lib/demo_mode.py (R3b) (#903)
lib/demo_mode.py (342). server.py 1,870 -> 1,649.
The read-only request guard (its 96-entry blocked-route table + the middleware) and the
hourly session janitor (registry, hook runner, thread). Bodies VERBATIM.
THE MIDDLEWARE NEEDS `app`, SO THE MODULE TAKES IT. _demo_mode_guard is an
@app.middleware("http") and cannot exist without an app object. Rather than have a module
under lib/ reach for a global, it exposes install(app) and server.py — which owns the app —
hands it over. The janitor is symmetrical: start_janitor() / stop_janitor(), called from
server.py's startup and shutdown hooks, where the process lifecycle actually lives.
register_demo_janitor_hook IS PART OF THE PLUGIN CONTRACT. It is a key in plugin_context,
so plugins hold it as a LIVE REFERENCE from setup(). server.py imports this exact object
and puts it in the dict unchanged — identity preserved, and
tests/test_plugin_context_contract.py (#898, merged) fails if that ever stops being true.
This is the first carve that guard has actually protected.
━━━ stop_janitor()'s ORDER IS LOAD-BEARING ━━━
The obvious way to write it — clear the "started" flag, then join — is WRONG, and I wrote
it that way first. server.py's original deliberately returns EARLY, leaving
_DEMO_JANITOR_STARTED True and the thread handle intact, when the thread outlives the join:
# Leave _DEMO_JANITOR_STARTED True so a new janitor is not
# spawned by a subsequent startup while the old one is alive.
Clearing the flag first quietly reintroduces exactly the double-janitor leak the flag
exists to prevent. Preserved byte-for-byte, and the reason is now written down at the
function rather than only at its single call site.
━━━ A BUG MOVED VERBATIM, ON PURPOSE ━━━
if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
and not _DEMO_JANITOR_STARTED:
`and` binds tighter than `or`, so this is `A or (B and C)` — the not-already-started
re-entry guard is DEAD whenever the env var is truthy, which is the only case that runs.
A second startup leaks a janitor thread (the handle is overwritten, so shutdown joins only
the last). Verified. Preserved exactly and filed as issue #902: a carve whose whole value
is being provably behaviour-neutral is not the place to change behaviour.
pyflakes caught three more missing imports on the way in (uuid, warnings x2). Five carves,
ten missing imports, every one a NameError on a live path.
pytest 2399, pyflakes 0, Codex 0.
Refs #48
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
8f014e6a30 |
refactor(server): carve the library scanner into lib/scan.py (R3b) (#901)
lib/scan.py (326). server.py 2,098 -> 1,870.
The background scan, its spawn ProcessPoolExecutor, and the kick/runner plumbing that
serialises passes. Bodies VERBATIM except the seam reads.
Everything shared is read LATE off appstate — the same contract every module in
lib/routers/ uses, and it is not cosmetic: tests monkeypatch CONFIG_DIR and swap meta_db,
so a value captured at import time pins the wrong one for the life of the process.
CONFIG_DIR -> appstate.config_dir
meta_db -> appstate.meta_db
_default_settings -> appstate.default_settings()
_stat_for_cache -> appstate.stat_for_cache()
━━━ THE SCAN STATUS IS REBOUND, NOT MUTATED ━━━
_background_scan does `global _scan_status; _scan_status = {**INIT, ...}` at every stage
transition. It REPLACES the dict; it never updates it in place. So nothing may hold that
dict by value — a reference captured once goes permanently stale at the first stage change
and would report "listing" forever while the scan ran to completion.
Hence `scan.status()`, a getter, and hence appstate publishes scan_status as a CALLABLE.
appstate.py already said so in a comment; this is the code that makes it true. (Same for
the plugin_context entry, which was already `lambda: dict(_scan_status)` — late-bound, so
it survives the move unchanged. The contract test from #898 covers it.)
━━━ appstate.server_root: A TRAP CLOSED PERMANENTLY ━━━
_background_scan seeds the builtin content, which needs the directory holding server.py.
`Path(__file__).resolve().parent` is correct in server.py and silently WRONG anywhere under
lib/ — it yields lib/, which holds no docs/ or data/ — and it fails by finding NOTHING
rather than by raising, so the seeds would just quietly never run.
lib/builtin_content.py (#900) closed that by taking the root as a parameter. This adds the
other half: server.py publishes it ONCE as appstate.server_root, so no module under lib/
ever has a reason to derive it. Documented at the slot.
pyflakes caught two more missing imports on the way in (loosefolder_mod, enrichment) —
each a NameError on a live scan path, and the suite would have handed them over one failure
at a time. It stays part of every server.py slice.
TESTS. The two scan fixtures (test_settings_api::scan_module,
test_feedpak_extension::scan_server) patched server._make_scan_executor to swap the spawn
pool for an in-process ThreadPool; they now patch it on lib/scan.py. Worth noting WHY that
still works: the fixtures re-import `server` per test, but `scan` stays cached in
sys.modules — and it picks up the fresh CONFIG_DIR anyway, because the appstate reads are
late-bound. The seam is doing exactly the job it was built for.
━━━ TEST ISOLATION: A REGRESSION THE CARVE ITSELF CREATED (Codex [P2]) ━━━
background_scan() deliberately NEVER sets running=False — ownership of that flag lives in
_scan_runner, so a kick_scan() racing the terminal write cannot observe a stale False and
start a second runner. Correct in production.
But the scan fixtures call background_scan() DIRECTLY, skipping the runner. That was
harmless while the state lived on `server`, which the fixtures RE-IMPORT per test. It is
NOT harmless now: `scan` stays cached in sys.modules across sys.modules.pop("server"), so
the status dict OUTLIVES the test. One direct call leaves the shared scanner marked
"running" forever, and every later scan or rescan returns "already in progress" and quietly
does nothing.
Verified: after a direct call, kick_scan() returns False and starts no scan at all.
The suite passed anyway, on ordering luck — which is exactly how this class of bug ships.
tests/conftest.py::reset_scan_state now snapshots and restores lib/scan.py's module state
around the two fixtures that drive it directly.
pytest 2398, pyflakes 0, Codex 0.
Refs #48
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
6b8f79dd9a |
fix(server): a raising tuning provider no longer takes down get_merged() for everyone (#899) (#904)
One word. server.py's TuningProviderRegistry.get_merged():
except Exception:
- logger.exception("tuning provider %r raised during get_merged()", provider_id)
+ log.exception("tuning provider %r raised during get_merged()", provider_id)
There is no `logger` in server.py — the module logger is `log`. So the handler written to
swallow-and-report a bad provider instead raised NameError from inside the except, and that
NameError propagated out of get_merged().
The effect was the exact OPPOSITE of what the handler is for: one misbehaving plugin took
the whole merged-tunings call down for every other provider, AND the traceback named the
wrong problem ("name 'logger' is not defined" rather than the provider that actually blew
up). Doubly silent: nothing was ever logged either, because the logging call was the thing
that crashed.
Found by pyflakes while carving server.py (R3b). It survived because NOTHING exercised the
failure path — no test ever had a provider raise. That is the whole reason this class of
bug is invisible: it lives only on error paths, so the suite is green and the feature is
broken exactly when it matters.
tests/test_tuning_provider_isolation.py is that path:
* a raising provider must not lose the HEALTHY providers' tunings, nor the defaults
* and the failure must actually be LOGGED — swallowing is only acceptable if it reports
Bite-tested: restoring `logger` fails both.
(The log assertion attaches caplog's handler to the feedBack logger directly. It sets
propagate=False, so pytest's root capture sees nothing from it — test_plugins.py has a
capture_logger() for this, but it is not importable here: pyproject pins pythonpath to
[".", "lib"], so `tests` is not a package. Three lines beat churning 21 call sites in an
unrelated file to convert that helper into a fixture.)
pytest 2410, pyflakes 0 undefined names in server.py, Codex 0.
Closes #899
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
70dbe45e27 |
refactor(server): carve builtin-content seeding into lib/builtin_content.py (R3b) (#900)
lib/builtin_content.py (321 lines moved). server.py 2,418 -> 2,098.
The calibration/diagnostic sloppaks and the starter library: _copy_builtin_packs,
_write_builtin_pack, the two seed helpers, their source tables, and the seed marker.
━━━ THE ONE SIGNATURE CHANGE, AND WHY THE CARVE IS UNSAFE WITHOUT IT ━━━
server.py has:
def _feedBack_server_root() -> Path:
return Path(__file__).resolve().parent
That is correct IN server.py: the repo root in dev, resources/feedBack when bundled — the
tree that actually holds docs/ and data/.
Move that body into lib/ unchanged and it keeps working, silently, and returns lib/. There
is no docs/diagnostics under lib/, so every seed would find nothing, log "source missing"
at debug, and return. Nothing raises. Nothing fails. The starter library simply never
appears, and the calibration sloppak is never seeded — on a fresh install, in the field.
A verbatim move whose MEANING changed because __file__ did.
So this module cannot compute a root: `server_root` is a PARAMETER, and server.py — the
only place that legitimately knows where it lives — passes it in. The trap is now
structurally impossible rather than merely avoided. (_copy_builtin_packs already took the
root that way; the two seed helpers now do too.)
Everything else is byte-identical. CONFIG_DIR is read late as appstate.config_dir and the
DLC root through dlc_paths._get_dlc_dir — the same seam every router in lib/routers/ uses,
late-bound because tests monkeypatch it.
━━━ PYFLAKES FOUND THREE MISSING IMPORTS THE TESTS WOULD HAVE FOUND ONE AT A TIME ━━━
The moved code uses `secrets`, `stat` and `tempfile`; none was in my import block. Each is
a NameError on a live path. `python3 -m pyflakes` names all three in one shot — this is the
Python twin of the no-undef gate that guarded every frontend carve, and it should run on
every server.py slice from here.
It also flagged a PRE-EXISTING one I deliberately did not touch: server.py's
TuningProviderRegistry.get_merged() calls `logger.exception(...)` in an except handler and
there is no `logger` in the module (it is `log`). So a raising tuning provider takes down
the merged-tunings call for everyone, with a NameError naming the wrong problem. Filed as
issue #899 rather than smuggled into a carve whose whole value is being behaviour-neutral.
The constants lost their underscore prefix: they cross a module boundary now (the seed
tests read them), so `_BUILTIN_STARTER_SOURCES` was a lie.
pytest 2397, pyflakes 0, Codex 0. Guarded by the plugin_context contract test (#898).
Refs #48
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|