fix(highway_3d): Venue desync, bind race, and a11y for the player background control - #1018
Merged
Merged
Conversation
When the Venue visualization override is active, the entire Background control group (style dropdown and intensity/reactive knobs) is now disabled since the venue scene owns rendering. The dropdown, intensity, and reactive controls are greyed out with a tooltip explaining the state. Also fixes a cold-load bug where the screen hook subscription could fail to bind if the event bus wasn't ready during renderer init, by re-attempting binding on each retry tick. Includes test coverage for both the Venue override greyout behavior and the cold-load screen hook binding fix.
…ntrol Refactor the player chrome background control to use an explicit `_bgReadGlobal()` function instead of relying on the implicit behavior of `_bgReadSetting(null, ...)`. The control is a single shared instance across splitscreen panels and must always read/write the global slot. Add accessibility improvements: - aria-pressed on toggle buttons to expose state to screen readers - aria-label on select and intensity controls - aria-describedby pointing disabled controls to a visually-hidden reason span - The reason span carries dynamic explanatory text for why a control is greyed out Add comprehensive tests verifying the new `_bgReadGlobal` helper ignores per-panel overrides and that all accessibility attributes are set and updated correctly.
Add explicit check for `window.feedBack.uiVersion === 'v3'` in _pcSlot() per docs/plugin-v3-ui.md. This prevents the plugin from attempting to mount player controls on non-v3 hosts (e.g., legacy v2 shell). Complements the existing `playerControlSlot` typeof check and improves compatibility robustness. Updated test mocks to include `uiVersion: 'v3'` and added test case verifying that mounting is skipped when uiVersion is not v3, including a guard to ensure the retry loop terminates properly.
Document that the style controls group also greyes out when the Venue scene override is active, since the controls don't apply in that mode.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe 3D Highway background controls now read global settings, become inert under the Venue scene override, expose updated accessibility state, retry late initialization, and include expanded tests. The changelog and plugin version were updated to reflect the release. Changes3D Highway background controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PlayerBackgroundControls
participant GlobalBackgroundSettings
participant VenueScene
User->>PlayerBackgroundControls: change background control
PlayerBackgroundControls->>GlobalBackgroundSettings: read or write global setting
VenueScene-->>PlayerBackgroundControls: publish venueScene change
PlayerBackgroundControls->>PlayerBackgroundControls: disable group and update ARIA state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)plugins/highway_3d/screen.jsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
Author
|
@coderabbitai review |
Contributor
Action performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@plugins/highway_3d/screen.js`:
- Around line 4184-4225: Update the _pcSync() logic for _pcSel so its title is
restored to the mount-time base tooltip, “Background style”, whenever Venue mode
is inactive, while retaining why as the title during Venue mode. Match the
existing title behavior used by the intensity control and preserve the
aria-disabled and visual state handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7a32a46-6486-4731-896e-eb50e4e84e25
📒 Files selected for processing (4)
CHANGELOG.mdplugins/highway_3d/plugin.jsonplugins/highway_3d/screen.jsplugins/highway_3d/tests/background_control.test.js
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
The style dropdown's tooltip was cleared whenever the Venue override was inactive, permanently discarding the "Background style" hint set at mount time. Since the sync runs on every settings change, the tooltip was lost on the first sync and never returned. This brings the dropdown in line with the intensity slider and reactive toggle, which already restore their base tooltip when they're re-enabled. Includes a test asserting the tooltip returns after the Venue override exits.
_pcAcquire only runs once the renderer is viable inside the v3 player chrome, and player-chrome.js sets uiVersion synchronously as it builds that chrome — so a missing 'v3' at acquire means v2, not a not-yet-ready v3. Bail before scheduling the retry loop instead of spinning it out to the ~3s budget for a slot that will never appear. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: byrongamatos <xasiklas@gmail.com>
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
2 participants
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.
What
Follow-up to #1008 (mid-song background controls in the player chrome), addressing post-merge review feedback:
_bgMountStyleresolves tovenueregardless of what the dropdown shows, so picking a style did nothing and looked broken. The whole Background group (dropdown, Reactive, Intensity) now goes inert with a reason on hover while Venue is active, and resyncs correctly when it toggles off.screen:changedre-mount hook only tried to bind once; on a cold load where the event bus wasn't ready yet, the control could mount but stay permanently deaf to player-chrome rebuilds for the session. It now retries the bind alongside the mount retry._bgReadSetting(null, key)(which worked by relying on a'h3d_bg_null_<key>'storage key never existing) with an explicit_bgReadGlobal(key)helper, so the shared control's intent to always read the global slot is stated directly rather than implied.aria-pressedon the Reactive toggle,aria-describedbypointing disabled controls at a visually-hidden reason (previously title-only, which screen readers announce unreliably and touch doesn't surface at all), andaria-labelon the style select and intensity slider._pcSlot()now explicitly checkswindow.feedBack.uiVersion === 'v3'per docs/plugin-v3-ui.md, matching the pattern the tuner plugin already follows.feedpak surface
Checklist
CHANGELOG.md[Unreleased]updated (user-visible changes)git commit -s)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests