fix(highway): a superseded renderer init is not a failed one (gig fell back to the 2D highway)#970
Merged
Merged
Conversation
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.
Contributor
📝 WalkthroughWalkthroughHighway continuous-frame detection now accounts for venue motion modes in addition to rolling crowd videos. Renderer initialization failure handling now ignores rejections from superseded promises while preserving current-cycle failure recovery, with tests covering both behaviors. ChangesHighway rendering behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
static/highway.js (1)
1004-1004: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winResolve-path lacks the same staleness guard.
The reject handler now ignores a rejection when
_installedRenderer.readyPromise !== rp(Line 1004), but the adjacent resolve handler (Lines 1035-1038) only checkshwState._renderer === _installedRenderer— not whetherrpis still the renderer's currentreadyPromise. This currently works only because the documented contract says a superseded promise always rejects, never resolves. If that invariant is ever violated (e.g. a future renderer change resolves an old promise instead of rejecting it),_emitVizReady()could fire for a stale init cycle. Mirroring the same identity check in the resolve branch would close this gap defensively.♻️ Proposed symmetry fix
rp.then( - () => { if (hwState._renderer === _installedRenderer) _emitVizReady(); }, + () => { if (hwState._renderer === _installedRenderer && _installedRenderer.readyPromise === rp) _emitVizReady(); }, _handleAsyncInitFailure );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/highway.js` at line 1004, Update the adjacent resolve handler for the renderer ready promise to also require _installedRenderer.readyPromise === rp, matching the existing rejection guard. Preserve the hwState._renderer === _installedRenderer check and prevent _emitVizReady() from running when the promise belongs to a superseded initialization cycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@static/highway.js`:
- Line 1004: Update the adjacent resolve handler for the renderer ready promise
to also require _installedRenderer.readyPromise === rp, matching the existing
rejection guard. Preserve the hwState._renderer === _installedRenderer check and
prevent _emitVizReady() from running when the promise belongs to a superseded
initialization cycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e4c815f-ba3d-4818-8832-d631cd7502c8
📒 Files selected for processing (3)
plugins/highway_3d/screen.jsstatic/highway.jstests/js/highway_pause_throttle.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Starting a gig dropped the player onto the fallback 2D highway with no venue.
startGig()callssetViz('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 freshreadyPromiseperinit()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:The guard now also checks the promise identity. The renderer-identity guard stays, and a genuine failure of the current cycle still reverts — both
init()call sites go through_setRenderer, which re-wires the handler every time.Reproduced and fixed against the real build:
defaultdefaultviz:revertedvenuevenueAlso: the paused-frame throttle's opt-out was too narrow
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). It now claims frames for both sources; a plain 3D highway reads motion modeoffand 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. Happy to drop it if you'd rather not ship on a theory.
Tests fail against the pre-fix source. eslint 0 errors; JS 1207/1207.
Summary by CodeRabbit