feat(editor): pitched GM guide voices + per-kind instrument picker (DAW 1.2/1.5) - #217
Conversation
…AW 1.2/1.5) The guide can now sound the charted notes as a General-MIDI instrument instead of the clap. New src/gm-guide.js owns the voice model and loader; the scheduler integration rides the existing guide tick, bus, fader and limiter unchanged. - Transport menu gains a Guide voice radio (Clap / Instrument (GM)) and per-part-kind instrument rows (curated FluidR3_GM programs for guitar / bass / keys; drums keep their clap). Prefs are editor-side, never the pack. Kind inference mirrors the repo rule (KEYS_PATTERN start-anchored > /bass/i > guitar). - Pitches come from _rollMidiForNote — keys packing for keys parts, capo-aware sounding pitch for fretted — so the guide can never disagree with the roll. Chord buckets (the clap's 1 ms dedupe resolution) ring up to 4 distinct pitches, each with its own sustain (capped 1.6 s). - The clap is the permanent fallback: while a preset loads (or no source serves it) the guide claps, never goes silent; a failed chord bucket claps ONCE, not per note. - Assets lazy-load through a three-rung source chain: plugin-vendored (/api/plugins/editor/wafont/, a whitelisted route over assets/wafonts/; nothing vendored yet - the README pins the FluidR3-only provenance contract, mirroring Virtuoso's) -> org-hosted base URL (editor-pref editorGmVoiceBase) -> the upstream WebAudioFont CDN core's drum_highway_3d and the piano plugin already use. Pinnable via editorGmVoiceSource. - routes.py: the /wafont route + _safe_wafont_name (whitelist regex, \Z anchored so a trailing newline can't slip past $). tests/gm_guide.test.mjs (19, real-import ESM; suite crashes on main - module absent): naming grammar, source chain + URL builder, kind inference, pref validation (incl. the Number-vs-parseInt and Number(null)=0 coercion traps), window grouping/cap/sustain, menu-model rows. tests/test_wafont_route.py (4): whitelist accepts player+FluidR3 only, rejects traversal shapes, JCLive-class variants, and non-asset names.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds pitched General-MIDI guide voices with per-kind instrument selection, lazy-loaded WebAudioFont assets, roll-derived pitch scheduling, clap fallback, editor menu controls, and secure plugin asset serving. ChangesGM guide voice runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as Editor menu
participant Audio as Guide scheduler
participant GM as gm-guide.js
participant AssetRoute as WebAudioFont asset route
participant Player as WebAudioFontPlayer
Editor->>Audio: Select GM guide voice
Audio->>GM: Ensure selected preset
GM->>AssetRoute: Request whitelisted asset
AssetRoute-->>GM: Return JavaScript asset
GM->>Player: Load preset and queue waveform
Audio->>Audio: Schedule pitched events or clap fallback
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
tests/test_wafont_route.py (1)
12-63: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover the route's security behavior, not only the helper.
These tests never exercise
get_wafont_asset. Add route-level cases for invalid names → 400, valid-but-absent assets → 404, vendored files → JavaScript responses, and an in-tree symlink resolving outsideassets/wafonts→ rejection. The containment check is currently verified only by inspection.🤖 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 `@tests/test_wafont_route.py` around lines 12 - 63, The tests currently cover only _safe_wafont_name; add route-level tests exercising get_wafont_asset for invalid names returning 400, valid but missing assets returning 404, existing vendored files returning JavaScript responses, and an in-tree symlink resolving outside assets/wafonts being rejected. Use the route’s existing test client and temporary filesystem setup, and assert the containment check through the HTTP response rather than inspection alone.
🤖 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 `@routes.py`:
- Around line 82-83: Update _WAFONT_NAME_RE to whitelist only the documented
ASCII filename forms: four-digit melodic names with an optional underscore
suffix, and five-digit percussion names beginning with the 128NN pattern and its
suffix. Replace \d with [0-9] and reject unsupported five-digit melodic names
such as 99999_FluidR3_GM_sf2_file.js; keep the WebAudioFontPlayer.js alternative
intact.
In `@src/gm-guide.js`:
- Around line 211-218: Update the GM preset-loading flow centered on
ensureGmPreset and _presetLoading so each source attempt has a timeout that
clears the stuck loading state and allows the plugin→org→CDN fallback chain to
continue. Add per-program retry backoff after fast failures to prevent repeated
full-chain starts on every guide tick, and reset that backoff after a successful
load or an explicit voice/source change such as editorSetGmVoice.
---
Nitpick comments:
In `@tests/test_wafont_route.py`:
- Around line 12-63: The tests currently cover only _safe_wafont_name; add
route-level tests exercising get_wafont_asset for invalid names returning 400,
valid but missing assets returning 404, existing vendored files returning
JavaScript responses, and an in-tree symlink resolving outside assets/wafonts
being rejected. Use the route’s existing test client and temporary filesystem
setup, and assert the containment check through the HTTP response rather than
inspection alone.
🪄 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: 508b41d9-4412-4b60-b376-aa23ece12acf
📒 Files selected for processing (8)
CHANGELOG.mdassets/wafonts/README.mdroutes.pysrc/audio.jssrc/gm-guide.jssrc/menu-bar.jstests/gm_guide.test.mjstests/test_wafont_route.py
| _WAFONT_NAME_RE = re.compile( | ||
| r"^(?:WebAudioFontPlayer\.js|\d{4,5}(?:_\d+)?_FluidR3_GM_sf2_file\.js)\Z") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the surrounding code/docs.
git ls-files | rg '(^|/)(routes\.py|README(\.md)?|readme(\.md)?)$'
echo '--- routes.py outline ---'
ast-grep outline routes.py --view expanded || true
echo '--- routes.py relevant lines ---'
nl -ba routes.py | sed -n '1,180p'
echo '--- README mentions ---'
rg -n "WebAudioFontPlayer|FluidR3|128[0-9]{2}|melodic|percussion|NNNN|NNNNN" README.md . -g '!**/.git/**' || trueRepository: got-feedBack/feedBack-plugin-editor
Length of output: 5248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If routes.py is not at repo root, locate it and inspect the exact file.
fd -a '^routes\.py$' .Repository: got-feedBack/feedBack-plugin-editor
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the whitelist grammar and any tests for the route.
rg -n "_WAFONT_NAME_RE|_safe_wafont_name|webaudiofont|FluidR3_GM_sf2_file|WebAudioFontPlayer" . -g '!**/.git/**'Repository: got-feedBack/feedBack-plugin-editor
Length of output: 4522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any nearby tests or fixtures that mention the accepted names.
fd -a 'test|spec' . | sed -n '1,120p'Repository: got-feedBack/feedBack-plugin-editor
Length of output: 5935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
pat = re.compile(r"^(?:WebAudioFontPlayer\.js|\d{4,5}(?:_\d+)?_FluidR3_GM_sf2_file\.js)\Z")
tests = [
"1234_FluidR3_GM_sf2_file.js",
"12345_FluidR3_GM_sf2_file.js",
"99999_FluidR3_GM_sf2_file.js",
"12800_1_FluidR3_GM_sf2_file.js",
"١٢٣٤_FluidR3_GM_sf2_file.js",
]
for s in tests:
print(s, bool(pat.fullmatch(s)))
PY
echo '--- routes.py ---'
nl -ba routes.py | sed -n '70,95p'
echo '--- README matches ---'
rg -n "WebAudioFontPlayer|FluidR3_GM_sf2_file|melodic|percussion|128[0-9]{2}|NNNN" README.md . -g '!**/.git/**'Repository: got-feedBack/feedBack-plugin-editor
Length of output: 402
🌐 Web query:
WebAudioFont README melodic NNNN percussion 128NN_N FluidR3_GM_sf2_file.js
💡 Result:
In the WebAudioFont library, the file naming convention relates to the General MIDI (GM) standard, which separates instruments into melodic (programs 0–127) and percussion (often mapped as bank 128) [1][2]. The naming structure NNNN_Name_sf2_file.js typically breaks down as follows: 1. NNNN: Represents the MIDI program/patch number (e.g., 0000 for Acoustic Grand Piano) [3][4]. 2. Name: Indicates the source soundfont used (e.g., FluidR3_GM, GeneralUserGS, JCLive) [3][4][2]. 3. FluidR3_GM_sf2_file.js: This refers to a specific JavaScript-encoded wavetable file derived from the FluidR3_GM soundfont [3][5][1]. Regarding the percussion/melodic distinction and the 128NN_N pattern: - Melodic instruments are cataloged using programs 0–127 [1]. - Percussion is typically handled as a specific set of instruments [1]. In many WebAudioFont-compatible implementations, percussion kits are accessed via bank/program mappings where the bank 128 is used for drums [2]. The notation 128NN_N refers to these percussion drum notes (e.g., MIDI notes 27–81), where 128 signifies the percussion bank and NN_N represents the specific drum sound ID or MIDI note number [2]. The library provides a comprehensive catalog where developers can load these files to perform sample-based synthesis in the browser using the Web Audio API [1][6][7]. Instruments are stored in this standardized format to ensure compatibility across different web applications [6][8].
Citations:
- 1: https://surikov.github.io/webaudiofont/npm/src/docs/index.html
- 2: https://dokumentaristen.dk/rackwerk/
- 3: https://github.com/surikov/webaudiofont/
- 4: https://github.com/surikov/webaudiofont/blob/master/README.md
- 5: https://github.com/surikov/webaudiofontdata
- 6: https://surikov.github.io/webaudiofont/
- 7: https://github.com/surikov/webaudiofont/blob/fd16e95ffd1ef3c38a94afe757d8eaca3b9796bb/npm/src/player.ts
- 8: https://github.com/itsbrex/my-awesome-stars
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- routes.py around the whitelist ---'
sed -n '74,92p' /home/jailuser/git/routes.py | cat -n
echo '--- assets/wafonts/README.md relevant lines ---'
sed -n '1,40p' /home/jailuser/git/assets/wafonts/README.md | cat -n
echo '--- tests/test_wafont_route.py relevant lines ---'
sed -n '1,90p' /home/jailuser/git/tests/test_wafont_route.py | cat -nRepository: got-feedBack/feedBack-plugin-editor
Length of output: 6012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
pat = re.compile(r"^(?:WebAudioFontPlayer\.js|\d{4,5}(?:_\d+)?_FluidR3_GM_sf2_file\.js)\Z")
candidates = [
"0000_FluidR3_GM_sf2_file.js",
"0270_FluidR3_GM_sf2_file.js",
"1270_FluidR3_GM_sf2_file.js",
"12835_0_FluidR3_GM_sf2_file.js",
"12800_1_FluidR3_GM_sf2_file.js",
"99999_FluidR3_GM_sf2_file.js",
"1234_FluidR3_GM_sf2_file.js",
"١٢٣٤_FluidR3_GM_sf2_file.js",
]
for s in candidates:
print(f"{s}: {bool(pat.fullmatch(s))}")
PYRepository: got-feedBack/feedBack-plugin-editor
Length of output: 452
🌐 Web query:
surikov webaudiofont README file naming 12835_0_FluidR3_GM_sf2_file.js 0270_FluidR3_GM_sf2_file.js
💡 Result:
The file naming convention used in the WebAudioFont library follows a specific pattern designed to identify the MIDI instrument index, the source soundfont, and the file type [1][2]. A typical filename, such as 0270_FluidR3_GM_sf2_file.js, is structured as follows: 1. MIDI Instrument ID (e.g., 0270): The four-digit prefix corresponds to the instrument's MIDI index [1][2]. Note that while standard MIDI uses indices 0-127, WebAudioFont often uses a four-digit format where the first digit may represent a bank or variation [1][3]. 2. Soundfont Name (e.g., FluidR3_GM): This segment identifies the source soundfont library used to generate the wavetable (e.g., FluidR3_GM, GeneralUserGS, Aspirin, SoundBlasterOld) [1][4]. 3. File Type Suffix (e.g., _sf2_file.js): This indicates that the file contains data exported from an SF2 (SoundFont) file, structured as a JavaScript file for use by the WebAudioFont player [1][2]. The variable name inside each JavaScript file typically follows a similar pattern, prefixed with tone to avoid global namespace collisions (e.g., _tone_0270_FluidR3_GM_sf2_file) [1][5][2]. To use these files, you must load the.js file and refer to the specific variable name defined within it to initialize the instrument in your Web Audio API project [1][2].
Citations:
- 1: https://surikov.github.io/webaudiofont/
- 2: https://surikov.github.io/webaudiofont/npm/src/docs/index.html
- 3: https://unpkg.com/webaudiofont@3.0.4/README.md
- 4: https://surikov.github.io/webaudiofontdata/
- 5: https://surikov.github.io/webaudiofontdata/sound/0270_FluidR3_GM_sf2_file.html
Tighten the filename whitelist
_WAFONT_NAME_RE still admits 5-digit melodic names like 99999_FluidR3_GM_sf2_file.js and Unicode decimal digits via \d. The README and tests only cover ASCII NNNN_... melodic files and 128NN_N_... percussion files, so narrow the regex to those explicit forms or update the docs/tests if the broader set is intentional.
🤖 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 `@routes.py` around lines 82 - 83, Update _WAFONT_NAME_RE to whitelist only the
documented ASCII filename forms: four-digit melodic names with an optional
underscore suffix, and five-digit percussion names beginning with the 128NN
pattern and its suffix. Replace \d with [0-9] and reject unsupported five-digit
melodic names such as 99999_FluidR3_GM_sf2_file.js; keep the
WebAudioFontPlayer.js alternative intact.
| export function editorSetGmVoice(kind, gm) { | ||
| const program = _gmVoiceForKindPure(gm, kind); | ||
| if (program === null) return; | ||
| try { localStorage.setItem('editorGmVoice:' + kind, String(program)); } catch (_) {} | ||
| _presetError = ''; // a new choice deserves a fresh load attempt | ||
| const row = (GM_VOICE_CHOICES[kind] || []).find(c => c.gm === program); | ||
| setStatus(`Guide instrument (${kind}): ${row ? row.label : 'GM ' + program}`); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'src/gm-guide.js' 'src/audio.js' || true
printf '\n== gm-guide outline ==\n'
ast-grep outline src/gm-guide.js --view expanded || true
printf '\n== search for ensureGmPreset and preset loading ==\n'
rg -n "ensureGmPreset|_presetLoading|_presetError|load|error|setTimeout|25 ms|25ms|preset" src/gm-guide.js src/audio.js || true
printf '\n== relevant slices from gm-guide.js ==\n'
nl -ba src/gm-guide.js | sed -n '1,380p'
printf '\n== relevant slices from audio.js around ensureGmPreset references ==\n'
nl -ba src/audio.js | sed -n '920,1045p'Repository: got-feedBack/feedBack-plugin-editor
Length of output: 6816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate tests or mocks mentioning gm preset ==\n'
rg -n "gm preset|ensureGmPreset|presetLoading|presetError|editorSetGmVoice|GM_VOICE_CHOICES|plugin→org→CDN|CDN" . || trueRepository: got-feedBack/feedBack-plugin-editor
Length of output: 4930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== gm-guide.js 228-304 ==\n'
awk 'NR>=228 && NR<=304 {printf "%4d %s\n", NR, $0}' src/gm-guide.js
printf '\n== audio.js 950-995 ==\n'
awk 'NR>=950 && NR<=995 {printf "%4d %s\n", NR, $0}' src/audio.js
printf '\n== gm_guide tests around loading/retry behavior ==\n'
awk 'NR>=1 && NR<=320 {printf "%4d %s\n", NR, $0}' tests/gm_guide.test.mjsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 20316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== scheduler / guide timer interval ==\n'
rg -n "25 ms|25ms|setInterval|requestAnimationFrame|GUIDE_|guideTimer|tick" src/audio.js src/gm-guide.js
printf '\n== audio.js around the guide loop and timer setup ==\n'
awk 'NR>=520 && NR<=640 {printf "%4d %s\n", NR, $0}' src/audio.js
printf '\n== any retry/backoff state in gm-guide.js ==\n'
rg -n "_presetLoading|_presetError|retry|backoff|deadline|timeout|abort|hung|hang" src/gm-guide.js tests/gm_guide.test.mjsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 11299
Bound GM preset loads and add retry backoff.
A hung script leaves _presetLoading stuck, so the plugin→org→CDN chain never advances past that source. If a source fails fast, ensureGmPreset() is retried on every 25 ms guide tick during playback, which can restart the full chain up to 40 times per second. Add a per-attempt timeout and a per-program retry delay that resets on success or an explicit voice/source change.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 216-216: React's useState should not be directly called
Context: setStatus(Guide instrument (${kind}): ${row ? row.label : 'GM ' + program})
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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 `@src/gm-guide.js` around lines 211 - 218, Update the GM preset-loading flow
centered on ensureGmPreset and _presetLoading so each source attempt has a
timeout that clears the stuck loading state and allows the plugin→org→CDN
fallback chain to continue. Add per-program retry backoff after fast failures to
prevent repeated full-chain starts on every guide tick, and reset that backoff
after a successful load or an explicit voice/source change such as
editorSetGmVoice.
# Conflicts: # CHANGELOG.md # src/audio.js
2 participants
What
The guide can now play the charted notes as a General-MIDI instrument instead of the clap — charting-by-ear finally sounds like music, not just timing. DAW-workspace roadmap items 1.2 (pitched GM guide) and 1.5 (voice picker + defaults), scoped to the guide path.
Clap/Instrument (GM)radio, plus per-part-kind instrument rows (curated FluidR3_GM programs for guitar / bass / keys — drums keep their clap; the drum strip owns drum sounds). Prefs are editor-side, never the pack._rollMidiForNote— keys packing for keys parts, capo-aware sounding pitch for fretted — so the guide can never disagree with what the roll shows. Chord buckets (the clap's 1 ms dedupe resolution) ring up to 4 distinct pitches, each with its own sustain (capped at 1.6 s).Asset sourcing — a chain, not a single origin
Per Christian's call, all three sources are wired, walked in order (
editorGmVoiceSourcepref: auto / pinned):/api/plugins/editor/wafont/servingassets/wafonts/(the Virtuoso idiom: no runtime CDN dependency once assets are committed). Nothing is vendored in this PR — the route 404s cleanly and the chain moves on.assets/wafonts/README.mdpins the provenance contract (FluidR3_GM renders only, mirroring Virtuoso's cleared set and its JCLive removal rationale). Vendoring the actual preset subset is a follow-up — flagging it here for your take on repo/package size.editorGmVoiceBase) for when org-hosted WebAudioFont renders exist (e.g. a feedback-soundfonts release; today it hosts.sf2for the desktop, a different format).drum_highway_3dand the piano plugin already lazy-load from. Works out of the box.The
/wafontroute validates with a whitelist regex (player + FluidR3 grammar only,\Z-anchored) plus a resolved-path containment check — traversal shapes and undocumented-provenance variants never reach the filesystem.Tests
tests/gm_guide.test.mjs— 19 real-import ESM tests: the naming grammar, source order + URL builder (incl. org-base normalization and http(s)-only), kind inference (the start-anchored "Electric Piano is not keys" trap pinned), pref validation (theNumber(null)=0andparseInt('127.5')coercion traps — both caught by these tests during development), window grouping/chord-cap/per-voice sustain, and the Transport-menu rows.tests/test_wafont_route.py— 4 tests: whitelist accepts player + FluidR3 names only; rejects traversal shapes (incl. NUL and trailing-newline), JCLive-class variants, and non-asset names.Gates: full JS suite 108 files green, ESLint 0 errors (3 warnings = main's ratchet, unchanged), pytest 234 passed (routes.py touched).
Seams
New module + additive edits in
audio.js(guide tick) /menu-bar.js(Transport menu) /routes.py; the usual CHANGELOG keep-both. Independent of the open #210/#212/#213/#214/#215/#216.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit