Fix v3 Songs List View favorite heart staying dim until re-search by ChrisBeWithYou · Pull Request #654 · got-feedBack/feedBack

ChrisBeWithYou · 2026-06-30T00:34:10Z

Reported on macOS + Windows, open since 0.3.0 / 2026-06-25 (=Scr4tch=):

"When favoriting a song while in the list view the heart now sticks without needing to leave the view or re-search, but they do not appear red. Only after re-searching do they appear red."

Root cause

One shared wireCards() [data-fav] click handler in static/v3/songs.js serves both the grid card and the tree / List View row. But the two render sites use different idle colours:

Surface	favorited	idle (not favorited)
Grid card	`text-fb-accent`	`text-white`
Tree / List View	`text-fb-accent`	`text-fb-textDim`

The handler hardcoded the grid's idle class:

btn.classList.toggle('text-fb-accent', d.favorite);
btn.classList.toggle('text-white', !d.favorite);   // ← grid only

In List View it added text-fb-accent but never removed text-fb-textDim, so the heart carried both colour utilities and the dim one won by CSS source order — the glyph flipped ♡→♥ ("sticks") but the colour didn't change until a re-search re-rendered the row with only text-fb-accent.

Fix

Each heart declares its idle colour via a data-fav-idle attribute (text-white on the grid, text-fb-textDim in List View).
The handler reads it and toggles exactly that class, so only one colour class is ever present — context-agnostic, no CSS-source-order fragility.
The handler also writes song.favorite = d.favorite back onto the in-memory model, so a tree re-render / virtualized-grid recycle reflects the new state instead of reverting to a stale favorite=false.

Testing

New tests/js/v3_favorites_toggle.test.js (3 tests) pins the data-fav-idle declarations, the idle-class swap (and that the hardcoded text-white toggle is gone), and the model sync.
Full JS suite: 810 tests, 797 pass, 13 fail — the 13 are pre-existing on main (colour-facade / viz-attribution / loop-seek, unrelated) and unchanged. node --check static/v3/songs.js passes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF

Favoriting from the tree / "List View" flipped the glyph ♡→♥ but the heart stayed grey until a re-search — reported macOS+Windows, open since 0.3.0 / 2026-06-25. One shared wireCards() [data-fav] handler serves both the grid card and the List-View row, but they render with different idle colours (grid text-white, List View text-fb-textDim) and the handler only ever removed the grid's text-white. So in List View text-fb-textDim lingered next to the freshly-added text-fb-accent and won by CSS source order — the glyph changed but the colour didn't, until a re-search re-rendered the row. Each heart now declares its idle colour via a data-fav-idle attribute; the handler swaps exactly that class (so only one colour class is ever present) and writes the new state back onto the in-memory song model so a re-render / virtualized-grid recycle agrees instead of reverting. Tests: tests/js/v3_favorites_toggle.test.js. Full JS suite 810 tests; the 13 pre-existing unrelated failures are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF

…View) (#717) _patchCardFav (the Song Details drawer's like -> card heart sync) hardcoded `classList.toggle('text-white', !fav)`, so toggling the like from the drawer left List-View rows' `text-fb-textDim` idle class in place — the exact dim-heart bug #654 fixed for the on-card click handler, reintroduced on the drawer path. Read the per-heart `data-fav-idle` and swap that class instead, mirroring wireCards. +regression assertion in v3_favorites_toggle.test.js. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ving it (R3c) 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>

…ving 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>

Pausing the song dropped the venue, the crowd and the stage to ~10 fps — "everything around the highway drops fps by a lot". draw() caps paused frames to one per _PAUSED_FRAME_INTERVAL_MS (100ms), on an assumption stated plainly in highway-constants.js: a heavy WebGL renderer "does a full render every frame even while paused. That is pure waste." That was true when a paused chart was a still picture. The venue broke the assumption. Its video backdrop keeps playing and its crowd reacts on a clock of their own, and BOTH are drawn into the same canvas as the notes — so a throttle aimed at static notes throttled the entire room. The scene only got a texture upload 10 times a second while the transport sat paused. Renderers can now declare that their picture is not static while the chart clock is stopped: an optional needsContinuousFrames(). The throttle is skipped only when it returns exactly true, and the probe fails closed — a renderer that doesn't implement it, or one that throws, keeps the throttle unchanged. So the GPU saving that motivated #654 survives everywhere it was actually valid. highway_3d implements it and claims continuous frames ONLY while a crowd video is genuinely rolling (bound, unpaused, not ended, readyState >= 2). With no venue pack — the common case — the paused scene really is static, so it keeps the throttle and the GPU still idles. Tests extend tests/js/highway_pause_throttle.test.js, which guards this code path source-level (the draw loop owns the rAF + WebGL lifecycle and is deliberately not reproduced in a vm — see the file header). The new guards pin that the capability GATES the early return rather than merely being called near it, that the probe fails closed on absent/non-function/throwing/truthy-but-not- true, and that the 3D renderer keys off the real video elements and can still return false. All 3 fail against the pre-fix source. eslint 0 errors; JS 1202/1202; pytest 2597 passed.

…rtuoso, and the paused throttle starving the venue (#968) * fix(venue): don't replay the flyover on an arrangement switch; keep the venue off other screens Two bugs from a live career session. 1. CHANGING ARRANGEMENT REPLAYED THE ARRIVAL FLYOVER. changeArrangement() reloads the song through the normal load path, so highway.js re-emits `song:loaded` — same filename, new arrangement. The venue could not tell that from a fresh arrival, so it reset the machine and flew the camera in from the back of the room again, mid-set, every time the player switched lead -> rhythm. The player is already on stage. onSongLoaded now compares the filename. A repeat of the song already on stage keeps the video pipeline running and only re-syncs the mood: the performance restarts, so the loop follows the reset machine with a quiet crossfade, never the intro. A genuinely different song still gets the full teardown + flyover. 2. THE VENUE SHOWED UP ON THE VIRTUOSO HIGHWAY. The venue was gated purely on `isVenueViz()` — the selected visualization, which is a GLOBAL preference and says nothing about what is on screen. Virtuoso borrows the same highway_3d renderer for its practice charts, so with Venue selected it inherited the backdrop: the crowd and the stage behind a chromatic exercise. Selecting Venue is a preference for the PLAYER; it is not a licence to paint the venue over whatever else happens to be using the renderer. The venue is now gated on viz AND screen (`shouldBeActive`), and follows `screen:changed` — it tears down on leaving the player and rebuilds on return. Nothing else changes: stop() already unbinds the videos from the renderer, so deactivating is enough to clear the backdrop. Tests: both decisions exposed as pure predicates and pinned — arrangement switch vs new song (including the first load, and a malformed payload that must not suppress the flyover forever), and the venue's screen scope. The existing syncViz test encoded the OLD contract (activate regardless of screen), so it now states the new one and additionally asserts the venue does NOT activate on virtuoso. Includes a guard test: with Venue selected AND on the player, the venue IS active — without it, every "not active" assertion could pass vacuously. All 8 new/updated assertions fail against the pre-fix source. eslint clean; JS 1199/1199; pytest 2597 passed. * fix(highway): the paused-frame throttle was throttling the whole venue Pausing the song dropped the venue, the crowd and the stage to ~10 fps — "everything around the highway drops fps by a lot". draw() caps paused frames to one per _PAUSED_FRAME_INTERVAL_MS (100ms), on an assumption stated plainly in highway-constants.js: a heavy WebGL renderer "does a full render every frame even while paused. That is pure waste." That was true when a paused chart was a still picture. The venue broke the assumption. Its video backdrop keeps playing and its crowd reacts on a clock of their own, and BOTH are drawn into the same canvas as the notes — so a throttle aimed at static notes throttled the entire room. The scene only got a texture upload 10 times a second while the transport sat paused. Renderers can now declare that their picture is not static while the chart clock is stopped: an optional needsContinuousFrames(). The throttle is skipped only when it returns exactly true, and the probe fails closed — a renderer that doesn't implement it, or one that throws, keeps the throttle unchanged. So the GPU saving that motivated #654 survives everywhere it was actually valid. highway_3d implements it and claims continuous frames ONLY while a crowd video is genuinely rolling (bound, unpaused, not ended, readyState >= 2). With no venue pack — the common case — the paused scene really is static, so it keeps the throttle and the GPU still idles. Tests extend tests/js/highway_pause_throttle.test.js, which guards this code path source-level (the draw loop owns the rAF + WebGL lifecycle and is deliberately not reproduced in a vm — see the file header). The new guards pin that the capability GATES the early return rather than merely being called near it, that the probe fails closed on absent/non-function/throwing/truthy-but-not- true, and that the 3D renderer keys off the real video elements and can still return false. All 3 fail against the pre-fix source. eslint 0 errors; JS 1202/1202; pytest 2597 passed.

Starting a gig dropped the player onto the fallback 2D highway with no venue. startGig() calls setViz('venue'), which installs the 3D renderer — whose init is async — and then immediately starts its play queue. playSong() re-initialises that same renderer a tick later. A renderer mints a fresh readyPromise per init() and rejects the previous one with "superseded"; highway.js only checked that the RENDERER OBJECT was unchanged, which it is. So it treated a healthy, re-initialising renderer as a failed one, tore it down, and reverted to 2D: renderer async init failure: Error: superseded viz picker: reverted to default renderer (async-init-failure) The guard now also checks the PROMISE identity: a rejection from an init cycle the renderer has already moved on from is ignored. The renderer-identity guard stays (a rejection for a renderer since REPLACED is also not ours), and a genuine failure of the CURRENT cycle still reverts — both init() call sites go through _setRenderer, which re-wires the handler every time, so the new cycle is always watched. Reproduced and fixed against the real build: before: vizSelection=default viz-picker=default venue=inactive viz:reverted after: vizSelection=venue viz-picker=venue venue=ACTIVE (no revert) Also widens the paused-frame throttle's opt-out. The throttle fires whenever the CHART CLOCK is stalled — not only on a pause, but through a count-in and the credits/author overlay too. Its opt-out only asked "is a crowd video rolling", but the venue scene animates on a clock of its own with no pack at all (backdrop breathe, parallax, haze drift, warmth pulse — Math.sin(t) in the draw loop), so that motion was still being throttled. It now claims frames for both sources; a plain 3D highway with no venue reads motion mode 'off' and keeps the #654 GPU saving. HONEST CAVEAT on that second part: I could not get the throttle to fire in a reproduction. A control run on the shipped code showed 100 draws/sec while paused, not the ~10/sec a firing throttle would give — so the change is defensible on its own terms (a stalled clock is genuinely not a static picture) but it does NOT have a demonstrated symptom behind it. The viz fix above does. Tests: the superseded guard, and that the throttle opt-out covers both motion sources. All fail against the pre-fix source. eslint 0 errors; JS 1207/1207.

ChrisBeWithYou mentioned this pull request Jul 1, 2026

feat(v3): flag library songs by working-tuning match (working-tuning PR 6) #668

Merged

byrongamatos force-pushed the fix/v3-favorites-list-red branch from 5090920 to 507ef25 Compare July 2, 2026 11:53

byrongamatos merged commit 0d28886 into main Jul 2, 2026
0 of 3 checks passed

byrongamatos deleted the fix/v3-favorites-list-red branch July 2, 2026 11:58

byrongamatos mentioned this pull request Jul 2, 2026

fix(v3): drawer fav-sync honours data-fav-idle (no dim-heart on List View) #717

Merged

byrongamatos mentioned this pull request Jul 12, 2026

test(highway): the R3c perf gate — measure the render loop before carving it #910

Merged

Kilgannon2113 mentioned this pull request Jul 14, 2026

Highway jitter: the chart-clock interpolator manufactures the jitter it exists to prevent (backward frames, ~±40ms positional error) #957

Open

byrongamatos mentioned this pull request Jul 14, 2026

fix(venue/highway): flyover replay on arrangement switch, venue on Virtuoso, and the paused throttle starving the venue #968

Merged

byrongamatos mentioned this pull request Jul 14, 2026

fix(highway): a superseded renderer init is not a failed one (gig fell back to the 2D highway) #970

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix v3 Songs List View favorite heart staying dim until re-search#654

Fix v3 Songs List View favorite heart staying dim until re-search#654
byrongamatos merged 1 commit into
mainfrom
fix/v3-favorites-list-red

ChrisBeWithYou commented Jun 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ChrisBeWithYou commented Jun 30, 2026

Root cause

Fix

Testing

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants