fix(library): tuning filter answers for your instrument, not always guitar - #1003
Conversation
…uitar The library indexed exactly one tuning per song, chosen guitar-first (lead > rhythm > combo, bass only as a last resort), and nothing consulted the player's instrument. A bassist filtering by tuning was shown the guitar chart's tuning, so playlists built by tuning contained songs needing a retune. Reported by a tester building bass practice sets; Covet "Shibuya" is the clean case, with a custom guitar tuning over a standard bass chart. Indexes each arrangement role's own tuning and makes the facet, filter, sort and labels answer for one perspective. `guitar-lead` reads the original unprefixed columns and adds no payload keys, so the default response is unchanged. The same defect existed inside guitar -- lead and rhythm charts can disagree -- so perspective is three-valued (guitar-lead, guitar-rhythm, bass) driven by one PERSPECTIVES table rather than parallel column families. Songs with no chart for the perspective fall back to the song-level tuning rather than vanishing (18 of 59 packs in the test library have no bass chart), but the fallback is marked inferred in the facet counts and on the row instead of being silently coalesced. "Only real charts" reuses the existing `arrangements_has` filter rather than adding one. Bass-specific handling, from measured content: - Bass tuning arrays are padded to six entries; charts never reference string index 4 or 5. Truncated to four before naming and grouping. - Grouping uses a canonical open-pitch key, so [-2,0,0,0] and [-2,0,0,0,0,0] are one facet row instead of two. - Offsets above +1 semitone are refused a name. Bassists tune down, near never up; one pack ships [5,5,5,5,4,4] (A-D-G-C, unplayable, and its own notes sit in the song's real key under standard tuning). Naming that would send a player to retune to a tuning that does not exist. Rhythm deliberately does not truncate -- padding is a bass finding, and cutting a seven-string array would invent a tuning the chart lacks. Adds an opt-in `tuning_match=playable` mode alongside exact match: a chart is offered when your lowest open pitch is at or below its lowest open pitch, so a five-string bass covers four-string standard and drop-D with no retune. Open strings only -- note range is not indexed and the scan stays manifest-only -- so it fails conservative: unknown low pitch is excluded, and the upper bound is unchecked and documented rather than guessed. Existing installs would otherwise never populate: the tree-signature fast path reports "unchanged" forever on a settled library. Rows with NULL marker columns re-extract, and the fast path is disabled until that backfill converges (writes use '' rather than NULL, so it self-clears). 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>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
✨ 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 |
The album-art badge now reads shownTuningName(), so the source-pattern guard no longer matched the inline `tuning_name || tuning` form and CI went red. Accept the helper, and pin the helper's own fallback in a companion test so the guard still fails if a guitar player's tuning label is ever dropped. Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
Finding un-split songs took five taps (cycle each stem pill to its "lacks" state) — and was quietly wrong even then: the drawer offered five of the canonical six stems, so a piano-only song lacked all five listed and matched a hand-built "not split" filter despite being split. The stems section gains a "Not split" toggle that sets stem_lacks to every instrument stem in one tap (the same lacks-ALL query Stem Splitter's missing-stems view runs, backend semantics unchanged), and piano joins the pill row (already in the backend's allowed set). (Rebuilt on current main after #1003/#810/#92e78be rewrote the drawer region — the original branch conflicted whole-file.) Signed-off-by: topkoa <topkoa@gmail.com>
Finding un-split songs took five taps (cycle each stem pill to its "lacks" state) — and was quietly wrong even then: the drawer offered five of the canonical six stems, so a piano-only song lacked all five listed and matched a hand-built "not split" filter despite being split. The stems section gains a "Not split" toggle that sets stem_lacks to every instrument stem in one tap (the same lacks-ALL query Stem Splitter's missing-stems view runs, backend semantics unchanged), and piano joins the pill row (already in the backend's allowed set). (Rebuilt on current main after #1003/#810/#92e78be rewrote the drawer region — the original branch conflicted whole-file.) Signed-off-by: topkoa <topkoa@gmail.com>
The report
The library indexed exactly one tuning per song, chosen guitar-first (
lead > rhythm > combo, bass only as a last resort), and nothing anywhere consulted the player's instrument. So a bassist filtering by tuning was reading the guitar chart's tuning, and playlists built that way contained songs that need a retune.Covet – "Shibuya" is the clean repro: custom guitar tuning
[-2,0,0,-1,-2,0], dead-standard bass chart. Today a bassist filtering Standard never sees it.What this does
Indexes each arrangement role's own tuning and makes the facet, filter, sort and labels answer for one perspective. Perspective is three-valued —
guitar-lead,guitar-rhythm,bass— because the same defect exists inside guitar: lead and rhythm charts can disagree on tuning. OnePERSPECTIVEStable inlib/tunings.pydrives extraction, the derived columns, the SQL, the migration list, the scanner's re-extract marker and the labels, rather than three near-identical column families.guitar-leadreads the original unprefixed columns and adds no payload keys — a test asserts default, explicit and legacy responses are identical, so nothing changes for existing users until they pick another perspective.The fallback is kept but made honest. 18 of 59 packs in the test library have no bass chart, so excluding them would hide a third of the library. Those fall back to the song-level tuning, but the fallback is marked inferred in facet counts and on the row rather than silently coalesced. The "only real charts" lever reuses the existing
arrangements_hasfilter rather than adding one.The perspective is labelled in the UI (
Tuning (bass)), including on the sort option — a silently-reordering list was the same discoverability failure as the original bug.Bass handling, from measured content
Measured across the 59-pack test library rather than assumed:
[-2,0,0,0]and[-2,0,0,0,0,0]are one facet row rather than two with split counts.[5,5,5,5,4,4]— that's A-D-G-C, which no one plays, and the chart's own notes sit in the song's real key under standard tuning. Naming it would send a player to retune to a tuning that doesn't exist.Playable-without-retuning mode
Opt-in
tuning_match=playablealongside exact match, which is untouched and still the default. A chart is offered when your lowest open pitch is at or below its lowest open pitch, so a 5-string bass covers 4-string standard and drop-D with no retune (the low D is just fretted on the B string).Honest scope: open strings only. Note range isn't indexed and the scan is deliberately manifest-only, so the upper bound is unchecked — documented, not overlooked. It fails conservative throughout: unknown low pitch is excluded rather than assumed playable, and a malformed current tuning applies no filter rather than claiming everything.
Why existing installs would otherwise get nothing
The tree-signature fast path (#979) reports "unchanged" forever on a settled library, so new scan columns would stay empty for everyone who already has a library. Rows with
NULLmarker columns re-extract, and the fast path is disabled until that backfill converges — writes use''rather thanNULL, so it self-clears instead of disabling the fast path permanently.Testing
tests/test_library_tuning_perspectives.py(25) +tests/test_library_tuning_instrument.py(53) + 4 intest_loosefolder.pymainorigin/maincontrol worktreeKnown gaps, not fixed here
feat/v3-playlist-tuning-check🤖 Generated with Claude Code
https://claude.ai/code/session_01SFDokqh2H6mEjk1Kgbi6JW