fix(av-sync): make the A/V offset slider actually settable + un-clobberable#1046
Open
ahonnecke wants to merge 1 commit into
Open
fix(av-sync): make the A/V offset slider actually settable + un-clobberable#1046ahonnecke wants to merge 1 commit into
ahonnecke wants to merge 1 commit into
Conversation
…erable The A/V offset was practically unusable: ±1000 slider range (useful zone a few unclickable pixels), no exact entry, and note_detect's auto-calibrate silently overwrote any hand-set value on song-end — so it read as "always wrong". - Narrow both A/V sliders ±1000 → ±300 (settings + player popover) for real resolution. - Player popover: replace the read-only label with an exact-entry number box (arrows step ±1 ms), plus a directional hint (higher = notes reach the strike line sooner / visuals faster). - setAvOffsetMs(ms, skipPersist, source): a user control passes 'manual' and sets a sticky override; the load path passes 'load'; any other caller (the plugin's window.setAvOffsetMs auto-calibrate) is IGNORED while the override holds. Persist av_offset_manual (client + settings router + resettable keys) so a hand-set value survives reload and isn't re-clobbered. Core-only — no plugin change needed; the block happens host-side. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesThe A/V offset settings API now validates, persists, and resets A/V offset override flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant settings.js
participant SettingsAPI
Browser->>SettingsAPI: GET persisted A/V offset settings
SettingsAPI-->>settings.js: av_offset_ms and av_offset_manual
settings.js->>settings.js: apply loaded offset and override state
Browser->>settings.js: enter or nudge manual offset
settings.js->>SettingsAPI: POST updated offset and manual flag
SettingsAPI-->>settings.js: persist settings response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
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
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.
Problem
The A/V offset was practically unusable:
Changes (core-only, no plugin change)
setAvOffsetMs(ms, skipPersist, source): a user control passes'manual'and sets a sticky override; the load path passes'load'; any other caller (the plugin's untaggedwindow.setAvOffsetMsauto-calibrate) is ignored while the override holds. Persistav_offset_manual(client + settings router + resettable keys) so a hand-set value survives reload and isn't re-clobbered.The auto-calibrate block happens host-side via the
sourcearg, so no note_detect change is required.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes