Fix tuner auto-open flash: opt-in + persist (issue E, stage 1/3)#655
Merged
Conversation
The tuner self-closes on song:play; autoplay fires it right after a song switch, so an auto-opened tuner flashed shut ~1s later. An arrangement switch (which never arms autoplay) instead persisted — the opposite tester reports, and not the mic. - New opt-in setting autoOpenOnTuningChange (tuner Settings, default OFF) - An auto-opened tuner persists: it ignores the autoplay song:play, stray outside-clicks, and same-screen re-emits, closing only via the new in-panel x / Skip buttons or leaving the song. A manual open keeps the classic click-away / play-to-close behaviour. - Adds the panel's first in-box close (x + contextual Skip). - All in the tuner plugin; no core app.js changes. Default (opt-in vs opt-out) is teed up for Byron to flip one boolean. Staged follow-ups: E1.5 = instrument-coverage smart prompting + badge cue; E2 = holdAutoplay gate. Tests: tests/js/tuner_auto_open.test.js (opt-in gate, persist mode, play/click-proofing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF
This was referenced Jun 30, 2026
This was referenced Jul 1, 2026
Closed
Merged
byrongamatos
added a commit
that referenced
this pull request
Jul 1, 2026
…#655 review) (#681) Two review fixes for the auto-open opt-in+persist stage: - enable() wasn't transactional. The panel (with the ×/Skip buttons) is shown before `await _tunerAudio.start()`, and `_state.enabled` was only set after it. A ×/Skip dismiss during that await hit disable() with wasEnabled=false, then enable() completed and flipped enabled on — an enabled-but-hidden zombie. Guard the open with an `_openGen` token bumped on every enable()/disable(); after the audio-start await, bail if superseded instead of enabling. Closes #675. - Config wasn't fail-closed. routes.py normalized the opt-in with bool(data.get("autoOpenOnTuningChange", False)), so "false"/"0"/junk coerced to True. Accept only a real JSON boolean. Closes #676. Tests: tuner_auto_open.test.js (dismiss-mid-open stays disabled — fails without the token guard), test_config.py (auto-open default-false + fail-closed on non-bool). 34 JS + 24 config tests green. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 1, 2026
Closed
byrongamatos
pushed a commit
that referenced
this pull request
Jul 1, 2026
With the opt-in auto-open on, only prompt when the player's current physical tuning doesn't already cover the song. FeedBack is tune-to-song (the highway draws tab in the song's tuning), so the check aligns the song's open-string tuning string-for-string against the player's instrument: - An 8-string F# player gets no prompt for a 6-/7-string standard song (its top strings already match those tunings). - A song needing an open string the player lacks (e.g. a Drop-A 7-string's low A on an F# 8-string) still prompts. - A whole-instrument reference difference (A440 vs A432, or an octave-down centOffset, previously ignored) also prompts. Reads the player's instrument from core /api/settings (the v3 instrument selector, a stable physical reference); conservative fallback (prompt) when undeclared or unavailable, so a real retune is never silently skipped. v3-only. All in plugins/tuner/screen.js; no core changes. Stacked on #655. Follow-up E1.6: a passive badge cue that names the strings to retune, plus splitscreen / no-usable-input guards. Tests: tests/js/tuner_auto_open.test.js (covered/uncovered, the Drop-A case, reference mismatch, direct contiguous alignment). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF
byrongamatos
pushed a commit
that referenced
this pull request
Jul 1, 2026
With the opt-in auto-open on, only prompt when the player's current physical tuning doesn't already cover the song. FeedBack is tune-to-song (the highway draws tab in the song's tuning), so the check aligns the song's open-string tuning string-for-string against the player's instrument: - An 8-string F# player gets no prompt for a 6-/7-string standard song (its top strings already match those tunings). - A song needing an open string the player lacks (e.g. a Drop-A 7-string's low A on an F# 8-string) still prompts. - A whole-instrument reference difference (A440 vs A432, or an octave-down centOffset, previously ignored) also prompts. Reads the player's instrument from core /api/settings (the v3 instrument selector, a stable physical reference); conservative fallback (prompt) when undeclared or unavailable, so a real retune is never silently skipped. v3-only. All in plugins/tuner/screen.js; no core changes. Stacked on #655. Follow-up E1.6: a passive badge cue that names the strings to retune, plus splitscreen / no-usable-input guards. Tests: tests/js/tuner_auto_open.test.js (covered/uncovered, the Drop-A case, reference mismatch, direct contiguous alignment). Claude-Session: https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 1, 2026
…#655 review) (#681) Two review fixes for the auto-open opt-in+persist stage: - enable() wasn't transactional. The panel (with the ×/Skip buttons) is shown before `await _tunerAudio.start()`, and `_state.enabled` was only set after it. A ×/Skip dismiss during that await hit disable() with wasEnabled=false, then enable() completed and flipped enabled on — an enabled-but-hidden zombie. Guard the open with an `_openGen` token bumped on every enable()/disable(); after the audio-start await, bail if superseded instead of enabling. Closes #675. - Config wasn't fail-closed. routes.py normalized the opt-in with bool(data.get("autoOpenOnTuningChange", False)), so "false"/"0"/junk coerced to True. Accept only a real JSON boolean. Closes #676. Tests: tuner_auto_open.test.js (dismiss-mid-open stays disabled — fails without the token guard), test_config.py (auto-open default-false + fail-closed on non-bool). 34 JS + 24 config tests green. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 1, 2026
…E, stage 2.5/3) (#657) * Tuner: passive "different tuning" badge cue that names the retune Building on the coverage check: when you enter a song your current instrument doesn't cover, the topbar tuner badge gets an amber ring + a tooltip naming the change (e.g. "retune B->A", or "the reference pitch" for an A440 vs A432 mismatch). Advisory only -- it never auto-opens the panel; recomputed on song:ready, cleared on song-load / leaving the player. Refactors the coverage check into a structured report (window._tunerAutoOpen.coverageReport -> { covered, retune:[{from,to}], reference, cantCover }); the boolean gate now wraps it. The cue is CSS-free (inline ring + native tooltip, no Tailwind rebuild) and no-ops when the tuner plugin is absent. Touches static/v3/badges.js (cue) + plugins/tuner/screen.js (report). v3-only. Stacked on #656 (issue E stage 2.5/3). The splitscreen-suppress and no-usable-input guards move to E2 (the playback gate). Tests: tests/js/tuner_auto_open.test.js (report names the strings, reference mismatch, badge wiring). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF * feat(tuner): read/write the live per-instrument working tuning — both-directions retune prompt (working-tuning PR 3) (#660) The §4 coverage check compared each song against the player's fixed instrument-profile tuning, so the tuner only ever prompted *away* from a "home" tuning (E -> Drop C#) and stayed silent coming back (Drop C# -> E), even though the player had physically retuned. _playerTuning() now reads the host's live per-instrument working tuning (window.feedBack.workingTuning, keyed by the selected instrument from /api/settings) instead of re-deriving from the static settings tuning, so coverage is measured against what the instrument is ACTUALLY in and prompts both directions. On clearing an auto-opened tuner, _publishWorkingTuning() writes that song's tuning as the instrument's live working tuning ('assumed' — PR 4's explicit "I tuned / Skip" refines the write-point), so the next song is judged against where the player now is. Per-instrument (guitar vs bass tracked separately). Feature-detected: falls back to the static /api/settings tuning when the working-tuning capability is absent, so the 27 existing coverage tests are unchanged. Builds on PR 1 (host workingTuning) + PR 2 (instrument->chart routing). Tests: tests/js/tuner_auto_open.test.js — +2 (both-directions coverage via a live Drop-D working tuning; publish-on-clear targets the right instrument slot); 29 pass total. Claude-Session: https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(tuner): transactional open + fail-closed auto-open config (tuner-E #655 review) (#681) Two review fixes for the auto-open opt-in+persist stage: - enable() wasn't transactional. The panel (with the ×/Skip buttons) is shown before `await _tunerAudio.start()`, and `_state.enabled` was only set after it. A ×/Skip dismiss during that await hit disable() with wasEnabled=false, then enable() completed and flipped enabled on — an enabled-but-hidden zombie. Guard the open with an `_openGen` token bumped on every enable()/disable(); after the audio-start await, bail if superseded instead of enabling. Closes #675. - Config wasn't fail-closed. routes.py normalized the opt-in with bool(data.get("autoOpenOnTuningChange", False)), so "false"/"0"/junk coerced to True. Accept only a real JSON boolean. Closes #676. Tests: tuner_auto_open.test.js (dismiss-mid-open stays disabled — fails without the token guard), test_config.py (auto-open default-false + fail-closed on non-bool). 34 JS + 24 config tests green. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tuner): coverage stays conservative when the instrument is unknown (tuner-E #656 review) (#682) _playerTuning() is documented as conservative ("missing data → not covered → still prompt"), but when /api/settings carried no instrument identity (a fresh profile: _default_settings() omits instrument/string_count/tuning) it invented guitar/6/440/ standard, so an unconfigured player was treated as 6-string E-standard and coverage suppressed the auto-open (and badge cue) for matching songs. The post-#660 rewrite only returned null when the whole fetch failed (!s), not when settings existed but lacked an instrument. Now return null unless there's a confident identity — any of instrument/string_count/ tuning in settings, or live working-tuning offsets. A configured standard guitar still covers a standard song (no regression). Closes #677. Tests: tuner_auto_open.test.js — empty-settings → not covered (fails without the fix); configured standard guitar → still covered. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tuner): badge coverage cue staleness + unknown-as-warning + dedupe (tuner-E #657 review) (#683) Three review fixes for the passive "different tuning" badge cue stage: - Stale async cue (#678): _refreshCoverageCue awaited coverageReport then wrote the DOM unconditionally, so a slow /api/settings fetch could restore the previous song's amber ring after song:loading / leaving the player. Add a monotonic token bumped on every refresh and both clear paths; apply the awaited report only if the token still matches. - "Unknown" rendered as "needs retune" (#679): the plugin returns a conservative all-false report on a fetch hiccup; the cue painted that as an amber "retune the reference pitch" ring. Collapse a no-signal report (not covered, no reference / retune / cantCover) to null (no cue) via _meaningfulReport(). A genuine not-covered report always carries reference / retune / cantCover, so real cues are preserved. - Duplicate /api/settings fetch (#680): the auto-open gate and the badge cue both call coverageReport() per song:ready. Cache the coverage promise per song (keyed by session + tuning + centOffset) so they share one fetch; invalidate on song:loading, instrument:changed, and working-tuning-changed so it can't go stale within a song. Tests: tuner_auto_open.test.js — concurrent reports share one fetch, a new song refetches (fails without the cache). 34 JS tests green. Codex-reviewed. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Byron Gamatos <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.
Fixes the 0.3.0 tester reports where the tuner auto-opened on a tuning change, then vanished ~1s later (macOS + Windows).
Root cause
The tuner closes itself on
song:play(plugins/tuner/utils/ui.js— "don't tune while playing"). Autoplay (default ON) firessong:playright after a song switch → the just-auto-opened tuner snapped shut. An arrangement switch (which never arms autoplay) had nosong:play, so it persisted — which is exactly why two testers reported opposite behaviour. It wasn't the mic.This PR (stage 1 of 3)
autoOpenOnTuningChange, default OFF) — also defuses the arrangement-switch annoyance.song:play, stray outside-clicks, and same-screen re-emits, closing only via the new in-panel×/ "Skip" buttons or when you leave the song. A manually opened tuner keeps its classic click-away / play-to-close behaviour.×+ contextual Skip — it had none before).app.jschanges (a test enforces this).@byrongamatos — your call on the default
Ships opt-in (default OFF); flip the one boolean for opt-out. Heads-up from the design charrette: a tiered default is on the table — passive badge ON / auto-open opt-in / playback-gate opt-in — but those land in the follow-ups, so this PR only asks about the auto-open toggle's default.
Staged follow-ups (not in this PR)
holdAutoplay()hook (mirrors the existingholdAutoExit()), defer-autoplay rather than persist-through-play.Testing
tests/js/tuner_auto_open.test.js— opt-in gate,{ auto: true }persist mode, play/click-proofing (18/18).mainand unrelated.🤖 Generated with Claude Code
https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF