feat: instruments as plugins — extract hardcoded instrument knowledge into plugin-based registry#997
feat: instruments as plugins — extract hardcoded instrument knowledge into plugin-based registry#997j0b333 wants to merge 76 commits into
Conversation
Replace ~15 scattered hardcoded guitar/bass checks across Python and JS with a unified InstrumentRegistry. Guitar, Bass, Drums, and Keys now ship as bundled instrument plugins under plugins/instrument_<name>/, each defining its own tunings, roles, arrangement mappings, string/key counts, detection strategy, and icon. Backend: - lib/instruments.py — registry with schema validation and offset-to-MIDI - lib/routers/instruments.py — GET /api/instruments endpoint - Plugin loader recognizes type:instrument and registers definitions - lib/tunings.py is registry-aware (profiles, tuning validation, presets) - Arrangement routing uses role flags/names from registry - Progression instrument_for_arrangement accepts registry, returns None for unknown - Settings API validates against registered instrument IDs - /api/tunings populates both tuningMidis AND tunings (fixes tuner sync) - Non-stringed instruments skip string_count/tuning validation Frontend: - badges.js: dynamic instrument pills, key count selector (keyboard), per-instrument icons from plugin assets, tuner grays out for non-pitched - songs.js: auto-filters library by current instrument's arrangement roles, filter options derived from registry - instruments-settings.js: editable instrument cards with custom tunings, arrangement names, string counts. Persisted via instrument_overrides - index.html: new Instruments tab, default_arrangement moved to selector - working-tuning.js: registry-aware instrument normalization
The display name from tuning_name() already returns E Standard for all-zero offsets, but the internal key was Standard which was inconsistent. Renamed in plugin.json tunings, profile defaults, and fallback values. Removed the cross-key tuning name rejection that prevented E Standard from being valid for guitar-6 (it already existed for guitar-8 as a separate tuning).
guitar-6 and bass-4 standard tuning IS E Standard. But 7-string, 8-string, and 5/6-string bass standard tunings are NOT E Standard (guitar-7 standard = B Standard, guitar-8 standard = F# Standard, bass-5/6 standard = B Standard). Only rename Standard->E Standard for guitar-6 and bass-4 in plugin.json and TUNING_PRESET_MIDIS. Keep Standard for the other keys.
guitar-6 → E Standard guitar-7 → B Standard guitar-8 → F# Standard bass-4 → E Standard bass-5 → B Standard bass-6 → B Standard Registry validation no longer hardcodes 'E Standard' — just requires at least one tuning per string count.
tuning_offsets_from_midis, tuning_midis_from_offsets, and tuning_preset_offsets now check _build_standard_midis() and _build_preset_midis() (registry) first, falling back to the hardcoded STANDARD_OPEN_MIDIS/TUNING_PRESET_MIDIS only when the registry is unavailable.
…ment - Tuning dropdown change now re-renders the instrument selector card so the button label/title updates immediately. - Instrument switch now reads the target profile's saved tuning from instrument_profiles, so switching back to an instrument restores the last-used tuning instead of matching the current one.
- Instrument selector now shows role pills (Lead/Rhythm for guitar) when the instrument has multiple roles. - Clicking a role switches the active instrument profile and saves default_role, so switching instruments remembers the last role. - ws_highway.py reads the active profile's role label as the preferred arrangement, overriding legacy default_arrangement.
- Tuner badge now shows the tuning name (e.g. 'E Standard', 'Drop D')
below the note letter, next to the Hz readout.
- Instrument pill now shows the current role label ('Lead'/'Rhythm')
below the icon when the instrument has multiple roles, replacing
the blank space when no working tuning is active.
…ncation - Role selector moved above Strings/Keys in the instrument dropdown - Instrument pill shows role label (Lead/Rhythm) first; working tuning shown only when no multi-role instrument is selected - Tuner badge tuning name no longer truncates, fits at 0.4375rem
- All instruments now show their role (Lead, Rhythm, Bass, Drums, Keys) on the pill, not just multi-role instruments. - Tuner badge tuning name now uses same font size (0.5625rem) and style (truncate, font-semibold) as the instrument pill text.
…es, instrument icons
- Added GET /api/stats/best-by-arrangement endpoint returning
{filename: {arr_index: accuracy}}
- accuracyBadge now shows accuracy for the arrangement matching the
current instrument role (Lead, Rhythm, Bass, etc.), not song-wide max
- On hover over album art: overlay shows all arrangements with
per-arrangement accuracy (-- for unplayed)
- Mastery icon replaced with instrument+role abbreviation:
single-role instruments show first letter (B, D, K),
multi-role show G^L / G^R (guitar icon + Lead/Rhythm letter)
…ngs, badge shows even unplayed
… null-safe pct - Badge now renders even when no accuracy data exists, showing '—' % and a gray badge, so the hover overlay with arrangement list appears. - Hover overlay uses inline styles instead of potentially unscanned Tailwind classes (bg-black/85, rounded-b-lg, z-20, pointer-events-none). - All pct/acc references null-guarded for unplayed songs.
- badges.js exposes sm._activeInstrumentProfile on settings save - _matchingArrangementIndex now resolves the active role label from the profile (e.g. 'Rhythm' when guitar-rhythm is active) and returns that arrangement's index for accuracy lookup. - Falls back to first matching arrangement if role can't be resolved.
When clicking a card (not an arrangement chip), playSong gets undefined for arrangement. The song:loading event fires with arrangement=null BEFORE the WebSocket resolves it via instrument routing. Now song:ready reads the actual arrangement_index from highway.getSongInfo() and updates cur.arrangement so scores are attributed to the correct arrangement.
…ment change - Instruments settings tab now has a 'Preferred highway' dropdown listing all registered visualization plugins. - Selection persisted to instrument_overrides.preferred_highway. - On instrument change, reads preferred highway from settings and stores it in localStorage.vizSelection so the next song uses it. - 'Auto (match arrangement)' is the default option.
Hey, good suggestions and sort of the direction I was already thinking moving next. Once this gets merged I was thinking on working fleshing out the multiplayer a bit.
|
|
@byrongamatos |
… detection, chart-transform)
…rue for bass players
…d bass arrangement names
…duplicate icon in bass" This reverts commit 026a608.
…es, and merge fixes
|
@byrongamatos The content decisions when merging into upstream main were: songs.js — built on upstream's version Registry as source of truth for instruments Per-role accuracy with hover overlay — kept mine Auto-filter toggle — kept mine Dynamic arrangement filter pills — kept mine Tuning chip position — kept mine Tuning offset detection — combined both Bass chip matching — fixed upstream's bug Double reload on instrument switch — fixed Chip logic made registry-aware — replaced hardcoded bass/lead Bass pick/fingered arrangement names — added Missing function, encoding corruption — fixed |
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
lib/routers/tunings.py (1)
55-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate custom tuning overrides before resolving them.
Malformed
instrument_overridesentries can crash/api/tunings:int(sc_key),named_offsets.items(), ands + oall assume trusted types. Skip invalid keys/structures and require numeric offset arrays before computing MIDI values; ideally enforce the same schema when loading settings.Suggested validation
for sc_key, named_offsets in custom_tunings.items(): - key = instrument_key(inst_id, int(sc_key)) - std_midis = std.get(sc_key) + try: + string_count = int(sc_key) + except (TypeError, ValueError): + continue + if not isinstance(named_offsets, dict): + continue + key = instrument_key(inst_id, string_count) + std_midis = std.get(str(string_count)) if not std_midis: continue for t_name, offsets in named_offsets.items(): - if isinstance(offsets, list) and len(offsets) == len(std_midis): + if ( + isinstance(offsets, list) + and len(offsets) == len(std_midis) + and all(isinstance(o, int) and not isinstance(o, bool) for o in offsets) + ): midis = [int(s + o) for s, o in zip(std_midis, offsets)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/routers/tunings.py` around lines 55 - 64, Harden the custom tuning loop around custom_tunings before resolving overrides: validate that each sc_key is an integer-compatible scale key, named_offsets is a mapping, each offsets value is a list of numeric offsets matching std_midis, and each offset can safely participate in MIDI arithmetic. Skip malformed entries without raising, while preserving the existing range check and preset_midis population for valid overrides; apply the same schema validation when loading instrument_overrides if that path is available.lib/routers/settings.py (1)
269-278: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
key_count(andauto_filter_instrument) don't get the same treatmentfret_countgot in this same diff.
fret_countwas added with full parity: profile defaults inlib/tunings.py, an entry in_RESETTABLE_SETTINGS_KEYS(line 401), and a range check in_validate_server_config_types(499-502).key_countonly got the POST/api/settingsrange check (269-278) — it's absent from_RESETTABLE_SETTINGS_KEYS, so it can't be cleared via the reset endpoint, and absent from_validate_server_config_types, so a hand-edited/corrupted settings-bundle import bypasses the 1–127 range check that docstring at line 447-462 explicitly claims to mirror.auto_filter_instrumentis likewise missing from_RESETTABLE_SETTINGS_KEYS(its sibling booleansachievements_enabled/use_amp_simsare present there).🐛 Proposed fix
_RESETTABLE_SETTINGS_KEYS = frozenset({ "default_arrangement", "demucs_server_url", "master_difficulty", "av_offset_ms", "countdown_before_song", "miss_penalty", "fail_behavior", "reference_pitch", "instrument", "string_count", "fret_count", "tuning", "pathway", "instrument_profiles", "active_instrument_profile", - "achievements_enabled", "use_amp_sims", + "achievements_enabled", "use_amp_sims", "key_count", "auto_filter_instrument", })if "string_count" in cfg: v = cfg["string_count"] if v is not None and (isinstance(v, bool) or not isinstance(v, int) or not (4 <= v <= 8)): return "server_config.string_count must be an integer between 4 and 8" + if "key_count" in cfg: + v = cfg["key_count"] + if v is not None and (isinstance(v, bool) or not isinstance(v, int) or not (1 <= v <= 127)): + return "server_config.key_count must be an integer between 1 and 127"Also applies to: 398-404, 493-502
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/routers/settings.py` around lines 269 - 278, Bring key_count and auto_filter_instrument to parity with the existing settings handling: add both keys to _RESETTABLE_SETTINGS_KEYS, and add key_count’s integer conversion and 1–127 range validation to _validate_server_config_types. Preserve the established validation behavior and reset semantics used by fret_count and the sibling boolean settings.lib/tunings.py (2)
70-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegistry-derived profile defaults hardcode
"tuning": "E Standard"even for non-stringed instruments.
_build_profile_defaultssets"tuning": "E Standard"unconditionally for every role of every registered instrument. For the bundled Drums/Piano plugins (kind!="stringed"), this means the default (never-explicitly-configured) profile reports a musically meaninglesstuning: "E Standard", contradicting the "non-stringed instruments have no tuning" convention this same diff establishes inprofile_from_legacy_settings(tuning = ""for non-stringed) and innormalize_instrument_profile'selsebranch. Becausenormalize_instrument_profileonly clearstuningwhenraw is not None, an unconfigured Drums/Piano profile (raw is None→return base, None) leaks this bad default straight through toGET /api/settings.It's also latent for stringed instruments whose
default_string_countkey differs from guitar-6/bass-4 (e.g. a community plugin defaulting to 7-string) —"E Standard"isn't a valid preset for those keys, and later validation of that same default via_valid_tuning_for_keywould reject it as a misapplied cross-key name.🐛 Proposed fix
result[profile_id] = { "id": profile_id, "label": f"{role['label']} {inst['label']}", "instrument": inst["id"], "role": role["id"], "string_count": inst.get("default_string_count", 0), "fret_count": inst.get("default_fret_count", 0), - "tuning": "E Standard", + "tuning": _default_tuning_for_instrument(inst) if inst.get("kind") == "stringed" else "", "reference_pitch": inst.get("reference_pitch", DEFAULT_REFERENCE_PITCH), "pathway": "songs", "default_role": r_default, }where
_default_tuning_for_instrumentmirrors the "look up the name whose MIDI array equals the key's standard" pattern already used inprofile_from_legacy_settings/apply_flat_instrument_patch_to_profiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/tunings.py` around lines 70 - 100, Update _build_profile_defaults to derive each profile’s tuning via _default_tuning_for_instrument using the instrument’s kind and default_string_count, rather than hardcoding “E Standard”; this must produce an empty tuning for non-stringed instruments and the matching valid standard preset for each string-count key.
420-440: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
fret_countvalidation is more lenient thanstring_count's, unlike the top-level POST check.In
normalize_instrument_profile, a badstring_countreturns a hard error (return None, "...must be valid for the instrument"), but a badfret_countsilently falls back tobase.get("fret_count", 0)and the profile is still accepted. This path is reachable directly fromPOST /api/settings {"instrument_profiles": {...}}(settings.py callsnormalize_instrument_profileper-profile), where — unlike the flatfret_countkey, which is strictly_as_int+range-validated before ever reaching here — a malformed nestedfret_countis silently masked instead of surfaced as a structured error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/tunings.py` around lines 420 - 440, Update normalize_instrument_profile so invalid stringed-profile fret_count values return a structured validation error, matching string_count, instead of falling back to the base value. Preserve the existing defaulting behavior only when fret_count is absent, and use the profile’s instrument_profiles.{profile_id}.fret_count context in the error.lib/metadata_db.py (2)
4491-4529: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_attach_display_chartsdrops rhythm-tuning fields that the main row payload carries.The member-chart SELECT and the resulting
display_chartdict includebass_tuning_name/bass_tuning_offsetsbut notrhythm_tuning_name/rhythm_tuning_offsets, whilequery_page's own row payload (built a few dozen lines earlier) includes both perspectives. When grouping is on, an intrinsic filter admits a work through a non-representative member, and the active perspective isguitar-rhythm, the swapped-indisplay_chartwon't carry that chart's own rhythm tuning — the client falls back to the representative's (possibly different) rhythm tuning fields for a chart it isn't actually displaying.🐛 Proposed fix
rows = self.conn.execute( "SELECT mw.effective_work_key, m.filename, m.title, m.duration, m.tuning, " "m.arrangements, m.has_lyrics, m.mtime, m.format, m.stem_count, m.stem_ids, " - "m.tuning_name, m.tuning_offsets, m.bass_tuning_name, m.bass_tuning_offsets " + "m.tuning_name, m.tuning_offsets, m.bass_tuning_name, m.bass_tuning_offsets, " + "m.rhythm_tuning_name, m.rhythm_tuning_offsets " "FROM songs m JOIN work_display mw ON mw.filename = m.filename " ... "tuning_name": m[11] or "", "tuning_offsets": m[12] or "", "bass_tuning_name": m[13] or "", "bass_tuning_offsets": m[14] or "", + "rhythm_tuning_name": m[15] or "", "rhythm_tuning_offsets": m[16] or "", }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/metadata_db.py` around lines 4491 - 4529, Add rhythm_tuning_name and rhythm_tuning_offsets to the member-chart SELECT in _attach_display_charts, preserving their column order, then map those values into the display_chart dictionary alongside the existing guitar and bass tuning fields. Ensure swapped-in charts retain their own rhythm tuning metadata.
3048-3080: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve per-perspective low-pitch values in
MetadataDB.get().
_put_perspective_value()storeslow_pitchasint | None, but the cache hit loop converts falsyNone/0values to""along with other non-marker / non-_sort_keycolumns. Keep the low-pitch fields intact soNonestaysNoneand numeric pitch values keep their type.🐛 Proposed fix
for i, col in enumerate(self._PERSPECTIVE_COLS, start=16): val = row[i] if col in self._EXTRACTION_MARKER_COLS: out[col] = val # NULL preserved — drives re-extraction elif col.endswith("_sort_key"): out[col] = int(val or 0) + elif col.endswith("_low_pitch"): + out[col] = val # int or None — never string-coerce a pitch else: out[col] = val or ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/metadata_db.py` around lines 3048 - 3080, Update the cache-hit loop in MetadataDB.get() to preserve per-perspective low_pitch values: when the column is a low-pitch field, return its stored value unchanged so None remains None and numeric values remain integers. Keep the existing extraction-marker and sort-key handling unchanged, and continue defaulting other perspective fields to an empty string.
🧹 Nitpick comments (1)
lib/routers/settings.py (1)
259-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated int-range validation.
string_count,key_count, andfret_countare now three near-identical_as_int+ range-check + structured-error blocks. A tiny helper (_validate_int_range(data, key, lo, hi)) would remove the duplication and make future numeric settings less error-prone to add correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/routers/settings.py` around lines 259 - 288, The validation blocks for string_count, key_count, and fret_count duplicate the same integer conversion and range-check logic. Extract a small _validate_int_range helper that accepts data, key, and bounds, returns the validated value or the existing structured error, then update each setting’s validation in the surrounding settings flow to reuse it while preserving its current ranges and error messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/instrument-plugins.md`:
- Around line 99-106: Update the community-plugin template JSON examples in the
instrument plugin guide so their bundled field is omitted or set to false; do
not leave bundled set to true in either template.
In `@static/v3/songs.js`:
- Around line 3864-3874: Update the initial fetch batch in the surrounding
initialization flow to request and store per-arrangement accuracy in
state.arrangementAccuracy, alongside the existing /api/stats/best request.
Ensure the initial card render has this data available to accuracyBadge(), while
preserving the current state.accuracy and other Promise.all results.
---
Outside diff comments:
In `@lib/metadata_db.py`:
- Around line 4491-4529: Add rhythm_tuning_name and rhythm_tuning_offsets to the
member-chart SELECT in _attach_display_charts, preserving their column order,
then map those values into the display_chart dictionary alongside the existing
guitar and bass tuning fields. Ensure swapped-in charts retain their own rhythm
tuning metadata.
- Around line 3048-3080: Update the cache-hit loop in MetadataDB.get() to
preserve per-perspective low_pitch values: when the column is a low-pitch field,
return its stored value unchanged so None remains None and numeric values remain
integers. Keep the existing extraction-marker and sort-key handling unchanged,
and continue defaulting other perspective fields to an empty string.
In `@lib/routers/settings.py`:
- Around line 269-278: Bring key_count and auto_filter_instrument to parity with
the existing settings handling: add both keys to _RESETTABLE_SETTINGS_KEYS, and
add key_count’s integer conversion and 1–127 range validation to
_validate_server_config_types. Preserve the established validation behavior and
reset semantics used by fret_count and the sibling boolean settings.
In `@lib/routers/tunings.py`:
- Around line 55-64: Harden the custom tuning loop around custom_tunings before
resolving overrides: validate that each sc_key is an integer-compatible scale
key, named_offsets is a mapping, each offsets value is a list of numeric offsets
matching std_midis, and each offset can safely participate in MIDI arithmetic.
Skip malformed entries without raising, while preserving the existing range
check and preset_midis population for valid overrides; apply the same schema
validation when loading instrument_overrides if that path is available.
In `@lib/tunings.py`:
- Around line 70-100: Update _build_profile_defaults to derive each profile’s
tuning via _default_tuning_for_instrument using the instrument’s kind and
default_string_count, rather than hardcoding “E Standard”; this must produce an
empty tuning for non-stringed instruments and the matching valid standard preset
for each string-count key.
- Around line 420-440: Update normalize_instrument_profile so invalid
stringed-profile fret_count values return a structured validation error,
matching string_count, instead of falling back to the base value. Preserve the
existing defaulting behavior only when fret_count is absent, and use the
profile’s instrument_profiles.{profile_id}.fret_count context in the error.
---
Nitpick comments:
In `@lib/routers/settings.py`:
- Around line 259-288: The validation blocks for string_count, key_count, and
fret_count duplicate the same integer conversion and range-check logic. Extract
a small _validate_int_range helper that accepts data, key, and bounds, returns
the validated value or the existing structured error, then update each setting’s
validation in the surrounding settings flow to reuse it while preserving its
current ranges and error messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e1ea5ea8-d0f9-47d7-8895-17c097b0f0d0
⛔ Files ignored due to path filters (1)
plugins/instrument_vocals/assets/icon.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
CHANGELOG.mddocs/instrument-plugins.mdlib/instruments.pylib/metadata_db.pylib/routers/settings.pylib/routers/stats.pylib/routers/tunings.pylib/routers/ws_highway.pylib/tunings.pyplugins/instrument_bass/plugin.jsonplugins/instrument_guitar/plugin.jsonplugins/instrument_vocals/plugin.jsonstatic/app.jsstatic/js/settings.jsstatic/v3/badges.jsstatic/v3/index.htmlstatic/v3/instruments-settings.jsstatic/v3/songs.jstests/test_tunings.py
🚧 Files skipped from review as they are similar to previous changes (9)
- plugins/instrument_bass/plugin.json
- plugins/instrument_guitar/plugin.json
- static/app.js
- static/v3/index.html
- lib/routers/ws_highway.py
- tests/test_tunings.py
- static/v3/instruments-settings.js
- lib/instruments.py
- static/v3/badges.js
| ```jsonc | ||
| { | ||
| "id": "instrument_<id>", | ||
| "name": "<Display Name>", | ||
| "version": "1.0.0", | ||
| "type": "instrument", | ||
| "bundled": true, | ||
| "icon": "assets/icon.svg", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not mark community-plugin templates as bundled.
The guide defines bundled: true as “not user-installed,” but both new-plugin templates set it to true. Omit it or set it to false for community plugins.
Proposed fix
- "bundled": true,
+ "bundled": false,Also applies to: 137-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/instrument-plugins.md` around lines 99 - 106, Update the
community-plugin template JSON examples in the instrument plugin guide so their
bundled field is omitted or set to false; do not leave bundled set to true in
either template.
| const det = (sd.stage === 'scanning' && sd.total) ? ' ' + sd.done + '/' + sd.total : ''; | ||
| btn.textContent = '⟳ Scanning' + det + '…'; | ||
| btn.disabled = true; | ||
| btn.classList.add('opacity-70'); | ||
| const pct = sd.total ? Math.round((sd.done / sd.total) * 100) + '% · ' : ''; | ||
| btn.title = 'Scanning new/changed songs… ' + pct + (sd.current || ''); | ||
| } else { | ||
| btn.textContent = '⟳ Refresh'; | ||
| btn.disabled = false; | ||
| btn.classList.remove('opacity-70'); | ||
| btn.title = |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Load per-arrangement accuracy during the initial render.
accuracyBadge() now relies on state.arrangementAccuracy, but this initial batch fetches only /api/stats/best. Initial cards therefore show no per-role score until a later dirty-score refresh calls applyScoreRefresh().
Proposed fix
- const [, tn] = await Promise.all([
+ const [, bestByArrangement, tn] = await Promise.all([
(async () => { state.accuracy = (await jget('/api/stats/best')) || {}; })(),
+ jget('/api/stats/best-by-arrangement'),
jget('/api/library/tuning-names?provider=' + enc(state.provider)
+ '&instrument=' + enc(libInstrument())),
loadArtistCatalog(),
refreshArtistPageGates(),
]);
+ state.arrangementAccuracy = bestByArrangement || {};
state.tuningNames = (tn && tn.tunings) || [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const det = (sd.stage === 'scanning' && sd.total) ? ' ' + sd.done + '/' + sd.total : ''; | |
| btn.textContent = '⟳ Scanning' + det + '…'; | |
| btn.disabled = true; | |
| btn.classList.add('opacity-70'); | |
| const pct = sd.total ? Math.round((sd.done / sd.total) * 100) + '% · ' : ''; | |
| btn.title = 'Scanning new/changed songs… ' + pct + (sd.current || ''); | |
| } else { | |
| btn.textContent = '⟳ Refresh'; | |
| btn.disabled = false; | |
| btn.classList.remove('opacity-70'); | |
| btn.title = | |
| const [, bestByArrangement, tn] = await Promise.all([ | |
| (async () => { state.accuracy = (await jget('/api/stats/best')) || {}; })(), | |
| jget('/api/stats/best-by-arrangement'), | |
| jget('/api/library/tuning-names?provider=' + enc(state.provider) | |
| '&instrument=' + enc(libInstrument())), | |
| loadArtistCatalog(), | |
| // Artist-page gates (PR-B) ride the initial fetch batch so the | |
| // first card paint already knows whether artist lines are links. | |
| refreshArtistPageGates(), | |
| ]); | |
| state.arrangementAccuracy = bestByArrangement || {}; | |
| state.tuningNames = (tn && tn.tunings) || []; | |
| _lastRenderInstrument = libInstrument(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@static/v3/songs.js` around lines 3864 - 3874, Update the initial fetch batch
in the surrounding initialization flow to request and store per-arrangement
accuracy in state.arrangementAccuracy, alongside the existing /api/stats/best
request. Ensure the initial card render has this data available to
accuracyBadge(), while preserving the current state.accuracy and other
Promise.all results.
gionnibgud
left a comment
gionnibgud
left a comment
There was a problem hiding this comment.
Reviewing from the keys/instrument-foundations side, since this lands right on
that surface. Short version: the direction is right and most of this I'd want to
keep. One design question I think needs a maintainer call before it merges, one
coordination flag, two concrete registry bugs, and one packaging ask.
Thanks for taking this on — the three-disagreeing-guess-tables problem is real
and has bitten several of us.
The design question: name-based routing as a plugin contract
roles[].arrangement_names makes the arrangement's display name the routing
key, and puts it in plugin.json — a public, community-facing contract.
That collides with feedpak-spec#56,
which is open (ON HOLD, prototyping editor-side first) and exists specifically to
decouple instrument routing from arrangement name. Its problem statement is the
same three tables this PR consolidates — so we agree on the diagnosis; the
question is the cure. #56 proposes a per-arrangement roles list in the pack;
this PR keeps the name as the key and moves the guessing into manifests.
Consolidating three tables into one is a clear improvement either way. My concern
is specifically the manifest surface: once community instrument plugins ship
arrangement_names, moving to a spec-driven key later breaks them. That makes
this the expensive half to get wrong.
Concretely, the normative and the cosmetic currently share one namespace
(lib/progression.py):
if name in role.get("arrangement_names", []):
return inst["id"]
if arr_type in role.get("arrangement_names", []):
return inst["id"]type is a spec field (§5.2, and type: "drums" is normative as of 1.17.0);
"Drum Kit" and "Synth Lead" are display strings. Matching both against one
list works today only because the type tokens happen to appear in the name lists.
If a future spec minor adds a normative type value, routing silently misses it
until every plugin manifest adds it as a "name".
Two options that would both address this without shrinking the PR much:
- Separate the keys:
arrangement_types(matched against spectype, exact)
fromarrangement_names(matched against display name, fuzzy/fallback). Types
win. This is additive and leaves room for #56 to land without a breaking
manifest change. - Or land the registry as-is but mark
arrangement_namesprovisional in
docs/instrument-plugins.md— explicitly "subject to change pending #56, do
not depend on it in third-party plugins yet."
I'd prefer (1), but either resolves my objection. Flagging for a maintainer call
since it's a spec-adjacent contract, not purely a core decision.
Coordination flag: instrument_vocals
Vocals-as-a-typed-part is currently fenced to a coordinated instrument-pathways
FEP by an explicit agreement between the keys-foundations and song-editor lanes,
and there are vocal sidecar PRs in flight separately (#1031, #1037, #1038). This
PR adds a third, independent vocal typing (detect_strategy: "pitch", vocal role
names).
Nothing wrong with the definition itself — it's the same shape as the others. But
I'd suggest dropping instrument_vocals from this PR and letting vocals
enter through the coordinated FEP, so we don't end up with three definitions of
what a vocal part is. The registry is the right home for it eventually; it's
the timing that's awkward.
Two registry bugs
1. No cross-instrument name-collision detection. Duplicate role ids within
one instrument raise ValueError, but two instruments claiming the same
arrangement name coexist silently, and the winner is decided by dict insertion
order:
def instrument_id_for_arrangement(self, arr_name, arr_flags=None):
for inst in self._instruments.values(): # insertion orderRegistration order is plugin load order, which CLAUDE.md documents as
alphabetical by directory. So a plugin at plugins/instrument_aardvark/ that
claims "lead" silently outranks instrument_guitar for every Lead arrangement
in the library, with no warning anywhere. Suggest rejecting (or warning + first-
wins on) a name already claimed by another instrument.
2. Silent overwrite of an existing instrument id.
self._instruments[inst_id] = normalizedA community plugin declaring instrument.id = "keys" replaces the bundled keys
definition outright — no warning, no first-wins. Given instrument definitions
now drive routing, tuning validation and detection strategy, that's a footgun
worth closing. First-wins plus a log.warning would match how the loader treats
other plugin-supplied registrations.
Packaging ask: split the reindentation
static/v3/songs.js is +4682/−4455 in a single hunk. Diffing it with
whitespace normalized, 3448 of the added lines are identical after stripping —
roughly three-quarters of the diff is reindentation, with only ~180 genuinely
new lines inside it. The real change isn't reviewable in that form, and it'll
make every future git blame on that file dead-end here.
Could you split it into a whitespace-only commit and a functional one? Same
final tree, and it makes the substantive part readable. Same goes, to a lesser
extent, for the features bundled in beyond instrument extraction (per-role
mastery %, hover overlay, auto-filter toggle, score attribution, the new
/api/stats/best-by-arrangement) — those are each defensible, but they widen
the blast radius of a PR that's already large.
Not blocking, minor
plugins/instrument_piano/declaresinstrument.id: "keys"with label
"Keys" — the id is right (core useskeysthroughout:keys_highway_3d,
data/progression/paths/keys.json), so consider renaming the directory to
instrument_keysfor consistency.- Heads-up that this touches
lib/routers/ws_highway.pyin the arrangement-
routing region, where there's rig/tone-binding work in flight — happy to
handle the merge on my side, just flagging it.
|
@gionnibgud Let me also see what I can do splitting some of the features in their won PR. Used to working with small teams where taking on bigger changes was fine. But that does not work with 10s of PRs per week haha. I will try and make my changes more manageable in the future. |
…cals - Rename plugins/instrument_piano/ to plugins/instrument_keys/ for consistency with the 'keys' instrument id used everywhere (data/progression/paths/keys.json, keys_highway_3d, etc.) - Remove instrument_vocals plugin — defer to the coordinated instrument-pathways FEP with its sidecar PRs - Update docs/instrument-plugins.md bundled instruments table - Remove vocal-specific routing from the hardcoded fallback in lib/progression.py
…ment roles Separate the normative spec type routing (arrangement_types, exact match against the arrangement's type field) from cosmetic display name routing (arrangement_names, fuzzy fallback). Types win on collision. Changes: - Add arrangement_types field to role schema (optional list of lowercase type tokens, e.g. lead/rhythm/bass/drums/keys) - Update all 4 bundled instrument manifests to declare arrangement_types - InstrumentRegistry.register(): validate arrangement_types, add cross-instrument collision detection warnings - InstrumentRegistry.instrument_id_for_arrangement(): check types first, then names, then flags (backward-compatible — existing plugins without arrangement_types continue to match via arrangement_names) - InstrumentRegistry.find_role_by_arrangement(): same priority order - progression.instrument_for_arrangement(): same types-first logic - Change duplicate-id from ValueError to first-wins + warning (registry) - Update docs/instrument-plugins.md schema and role mapping section
3f7ddde to
2b95f4c
Compare
…ing perspective, auto-filter, grid nav
review fixes: arrangement_types, registry, songs.js reindent
|
Hey @gionnibgud Design question — arrangement_names vs spec types. I went with your option 1. Added arrangement_types alongside arrangement_names as a separate field. Types match exactly against the spec type value, names are the fuzzy display-name fallback. Types win on collision. Updated all four bundled manifests, the registry matching logic, progression, and docs. It's fully additive — existing plugins without arrangement_types keep routing through names. This leaves room for feedpak-spec#56 without a breaking manifest change. I also display the types read-only in the settings panel's role cards (e.g., "Lead (lead) (default)"). Coordination — dropped instrument_vocals. Removed the plugin directory entirely and reverted the vocal routing line from the hardcoded fallback in progression.py. Vocals still work via the pre-existing fallback. Will let the FEP land it properly when ready. Minor naming — instrument_piano → instrument_keys. Renamed the directory to match the "keys" id used everywhere in core. Registry bug 1 — cross-instrument collision. InstrumentRegistry.register() now iterates all already-registered instruments when a new one comes in and warns if any arrangement names or types overlap, identifying both the new and conflicting plugin IDs. First-wins remains the policy. Registry bug 2 — duplicate id. Changed from raising ValueError to first-wins + log.warning with both plugin IDs, so community plugins can't silently displace bundled instruments. Packaging — songs.js split. Split into two commits on the fix branch (PR #2 targeting this feature branch):
Both commits cancel to a net-zero diff against the feature branch — same final content, just layered so reviewers can step through formatting vs. logic separately. |
4 participants
What
Extracts all hardcoded instrument knowledge (guitar, bass, drums, keys) from ~15 scattered Python and JS files into a plugin-based registry. Guitar, Bass, Drums, and Keys now ship as bundled instrument plugins (
plugins/instrument_<name>/plugin.json) each defining their own tunings, roles, arrangement mappings, string/key counts, detection strategy, and icon. Community instruments drop in as newtype: "instrument"plugins with zero code changes.Backend
lib/instruments.py— registry class with schema validation, offset-to-MIDI computationGET /api/instruments— serves all registered instrument definitionstype: "instrument"and auto-registers definitionslib/tunings.py— profiles, tuning validation, preset catalogs from registryws_highway.py) uses role flags/names from registryinstrument_for_arrangementaccepts registry, returns None for unknown/api/tuningspopulates both tuningMidis and tunings (fixes tuner sync)Frontend
song:readyresolves arrangement indexfeedpak surface
Checklist
node --checkpasses on all modified JSprint()calls in modified Pythonlogging.getLogger("feedBack.*")conventionFull changelog in
CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes