Skip to content

feat(playlists): flag songs that are not in your current tuning - #1005

Closed
ChrisBeWithYou wants to merge 4 commits into
feat/v3-tuning-filter-instrumentfrom
feat/v3-playlist-tuning-check
Closed

feat(playlists): flag songs that are not in your current tuning#1005
ChrisBeWithYou wants to merge 4 commits into
feat/v3-tuning-filter-instrumentfrom
feat/v3-playlist-tuning-check

Conversation

@ChrisBeWithYou

Copy link
Copy Markdown
Contributor

Why

Making the library's tuning filter instrument-aware (#1003) does not repair playlists already built under the old guitar-first behaviour. Those keep their wrong-tuning songs, so a player still hits a surprise retune mid-practice and reasonably concludes nothing was fixed.

This matters more on bass than it sounds: heavy strings take minutes to settle after a retune, and detuning far on standard gauges goes floppy and unusable. "Group songs so I don't retune" isn't a convenience — it's the difference between practising and doing setup work.

What this does

A per-playlist check against the player's current tuning:

  • Each row is marked match / mismatch / unknown
  • A summary bar: "3 of 24 songs aren't in your tuning"
  • A Show only these filter
  • An explicit Remove them… that lists every affected song by title in the confirm and states they stay in the library

Flagging is the feature. Nothing is ever removed without being asked for — silent auto-repair of curated user data was considered and rejected. Removal loops the existing per-song DELETE /api/playlists/{pid}/songs/{fn} rather than adding a bulk destructive endpoint, and targets mismatches only, never unknowns.

Reuses the tuner capability's coverage report plus window.feedBack.workingTuning — the same pair the library cards already score against in decorateTuningChips() — rather than introducing another source of truth.

Two deliberate departures

1. unknown is derived, not reported. The coverage report returns the same "not covered" shape both for a real mismatch and for a bail-out it couldn't evaluate. Only a report carrying an actual reason (named string changes, a reference-pitch gap, too few strings) counts as a mismatch; the rest render as unknown. This differs from the library grid, which paints every not-covered song amber — acceptable on a grid, not on a hand-curated playlist where a false ⚠ costs trust in the whole feature.

2. No perspective → no claim. Without workingTuning or tuner coverage the view renders exactly as before, rather than defaulting to guitar and reproducing the original bug in a new place.

States carry text markers (⚠ / ?) as well as colour.

Backend

Playlist rows now carry tuning_offsets and bass_only. A tuning name can't be scored — two "Custom Tuning" rows are different tunings — and the check needs to know whether to measure against bass or guitar base pitches. _resolve_album_orphan now reports the resolved chart's tuning rather than the dead pin's.

Merge order

Based on main and works standalone, but it is only fully correct on top of #1003, which adds the per-instrument tuning columns. That dependency is confined to a single function — rowTuningForCheck() in static/v3/playlists.js, marked ── SEAM ── — which today reads the one guitar-derived tuning main indexes, i.e. literally the data source behind the bug report. After #1003 lands, that one function picks the bass tuning for a bass player and the chips, summary, filter and remove-list all follow with no other change.

Please merge #1003 first.

Testing

  • tests/js/v3_playlist_tuning_check.test.js — 20 tests driving the real functions lifted from source via extractBlock + vm, no re-implementation. Coverage is the one stubbed boundary, and a contract test pins its report shape against plugins/tuner/screen.js so fixtures can't drift.
  • 4 pytest tests including adversarial arrangement data ("", None, "Bassoon", "BASS").
  • Fail-on-main verified: the backend tests fail with KeyError: 'tuning_offsets' / KeyError: 'bass_only' against unmodified main.
  • Full JS suite 1044 passing; full pytest shows only the 10 known pre-existing Windows failures, none added.

One env note: eslint can't run in a fresh worktree (dev dependencies aren't installed there), so the JS is unlinted — pre-existing, but flagging it since style wasn't machine-verified.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SFDokqh2H6mEjk1Kgbi6JW

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3b331f54-38f2-4947-b3bb-7d40c987f2c6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/v3-playlist-tuning-check

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

ChrisBeWithYou and others added 3 commits July 18, 2026 23:01
Making the library's tuning filter instrument-aware does not repair playlists
already built under the old guitar-first behaviour. Those keep their
wrong-tuning songs, so a player still hits a surprise retune mid-practice and
reasonably concludes nothing was fixed.

Adds a per-playlist check: each row is marked against the player's current
tuning, with a summary ("3 of 24 songs are not in your tuning"), a filter to
show only those, and an explicit removal that lists every affected song by
title and states they stay in the library. Flagging is the feature -- nothing
is ever removed without being asked for, and removal reuses the existing
per-song DELETE rather than adding a bulk destructive endpoint.

Reuses the tuner capability's coverage report and `window.feedBack
.workingTuning`, the same pair the library cards already score against,
rather than introducing another source of truth.

Two deliberate departures:
- A coverage report reads "not covered" both for a real mismatch and for a
  bail-out it could not evaluate. Only a report carrying an actual reason
  counts as a mismatch; the rest render as unknown. This differs from the
  library grid, which paints every not-covered song amber -- acceptable on a
  grid, not on a hand-curated playlist where a false warning costs trust.
- With no tuning perspective available it makes no claim at all, rather than
  defaulting to guitar and reproducing the original bug in a new place.

Playlist rows carry `tuning_offsets` and `bass_only`; a tuning *name* cannot
be scored, since two "Custom Tuning" rows are different tunings.

Fully correct once the instrument-aware tuning filter lands. That dependency
is confined to `rowTuningForCheck()` in static/v3/playlists.js, marked SEAM.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFDokqh2H6mEjk1Kgbi6JW
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
CI's tailwind-fresh gate rebuilds static/tailwind.min.css and hard-fails if
the committed file differs. The new chip/summary/filter markup introduces
classes the previous build never saw.

Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
Reverts the regenerated static/tailwind.min.css and reworks the tuning-check
markup to use only classes already in the committed sheet.

Regenerating that file is not reproducible off CI: nothing pins tailwindcss,
autoprefixer or caniuse-lite, so a local `npx -y tailwindcss@3.4.19` resolves
different browser data and rewrites unrelated bytes -- a clean checkout of
main rebuilds with the -webkit-backdrop-filter prefixes dropped. Committing
that output fails the tailwind-fresh gate no matter how many times it is
regenerated.

Six utilities were new: bg-fb-good/10, border-fb-accent/50,
hover:bg-fb-accent/10, list-disc, list-inside, max-h-48, plus gap-x-3/gap-y-2.
Substituted bg-fb-good/30, the amber border already used by the mismatch
state, hover:bg-fb-card, a literal bullet in a div, max-h-32 and gap-3. Visual
intent is unchanged.

The removal-confirm test pinned the <li> markup; it now accepts either
wrapper, since what it guards is that every song is named and escaped ahead
of any DELETE, not which element wraps it.

Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
@ChrisBeWithYou
ChrisBeWithYou force-pushed the feat/v3-playlist-tuning-check branch from 3180a41 to b4edec2 Compare July 19, 2026 04:03
@ChrisBeWithYou
ChrisBeWithYou changed the base branch from main to feat/v3-tuning-filter-instrument July 19, 2026 04:03
@ChrisBeWithYou

Copy link
Copy Markdown
Contributor Author

Superseded by #1009 — same branch, rebased onto main.

This PR was auto-closed by GitHub when its base branch feat/v3-tuning-filter-instrument was deleted as part of merging #1003, and GitHub refuses to reopen it now that the head has been rebased off that base. Reopening was attempted (including restoring the deleted base branch) before opening the replacement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant