fix(tuner): transactional open + fail-closed auto-open config (tuner-E #655 review)#681
Merged
byrongamatos merged 1 commit intoJul 1, 2026
Conversation
…#655 review) 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>
There was a problem hiding this comment.
Pull request overview
This PR hardens the tuner-E auto-open flow by making tuner.enable() transactional (preventing an enabled-but-hidden “zombie” state when the panel is dismissed while audio is still starting) and by making the autoOpenOnTuningChange config flag fail-closed (only a real JSON boolean can enable it).
Changes:
- Add an
_openGengeneration token to invalidate in-flightenable()calls whendisable()occurs mid-open, preventing post-dismiss re-enablement. - Update tuner config reading to accept
autoOpenOnTuningChangeonly when it’s a JSON boolean; otherwise default toFalse. - Add regression tests covering mid-open dismiss behavior and strict boolean handling for the auto-open config flag.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
plugins/tuner/screen.js |
Adds _openGen token guarding to prevent “zombie” enabled state when dismissed during async audio start. |
plugins/tuner/routes.py |
Makes autoOpenOnTuningChange fail-closed by requiring a boolean type when reading config. |
tests/js/tuner_auto_open.test.js |
Adds a regression test ensuring mid-audio-start dismiss keeps tuner disabled. |
tests/plugins/tuner/test_config.py |
Adds tests for default-false, accepting True, and rejecting non-bool values for auto-open config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Review fixes for the #655 stage (auto-open opt-in + persist) of the tuner-E stack. Targets
feat/v3-tuner-coverage-badge(where the stack + #660 already live).Closes #675 — enable() wasn't transactional. The panel (with ×/Skip) is shown before
await _tunerAudio.start(), and_state.enabledwas only set after it. A dismiss during that await hitdisable()withwasEnabled=false, then enable() completed and flippedenabledon → an enabled-but-hidden zombie. Now guarded with an_openGentoken (bumped on every enable()/disable()); after the audio-start await, enable() bails if superseded.Closes #676 — config wasn't fail-closed.
routes.pyusedbool(data.get("autoOpenOnTuningChange", False)), coercing "false"/"0"/junk toTrue. Now accepts only a real JSON boolean.Tests:
tuner_auto_open.test.js(dismiss-mid-open stays disabled — verified it fails without the token guard) +test_config.py(default-false + fail-closed on non-bool). 34 JS + 24 config tests green.Part of the tuner-E review-fix set (stages #655/#656/#657); see also the #656 and #657 fix PRs.
🤖 Generated with Claude Code