Fix keyboard-arrangement misclassification and highway_3d chord-diagram mirroring

.gitignore

@@ -65,3 +65,5 @@ Thumbs.db
 *.swp
 .idea/
 plugins/support_creators
+/library
+/static/sloppak_cache

CHANGELOG.md

@@ -400,6 +400,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   engine (`app.js`, `highway.js`, `playSong`, `showScreen`, the capability registry).
 
 ### Fixed
+- **`convert_wem` no longer blocks the event loop inside `highway_ws`.** Both
+  call sites in `lib/routers/ws_highway.py` (loose-folder and archive audio
+  conversion) invoked `convert_wem` directly inside the `async def
+  highway_ws` handler; `convert_wem` shells out to vgmstream-cli/ffmpeg via
+  `subprocess.run` with up to a 120s timeout, so a single slow/large
+  conversion held the whole event loop and stalled every other concurrent
+  WebSocket connection on that worker for as long as it ran. Both sites now
+  run through `loop.run_in_executor()`, reusing the `contextvars.copy_context()`
+  snapshot already taken earlier in the function so the bound `ws_conn_id`
+  correlation ID still applies to log lines raised inside the executor
+  thread — same pattern this file already uses for `load_song`/
+  `sloppak_mod.load_song`.
+- **highway_3d chord diagram no longer mirrors on Invert.** The top-left chord
+  diagram overlay (`drawChordDiagram()`) was flipping its column order
+  (high-e/low-E swapped) whenever the highway's Invert toggle was on, passed
+  through as `inverted: _invertedCached` at both call sites. The diagram's
+  orientation should be fixed regardless of that toggle, so both call sites
+  now pass `inverted: false`. Note: `plugins/highway_3d/CLAUDE.md` had
+  documented the mirroring as this overlay's contract, but that line traces
+  only to a single squashed "Clean release snapshot" commit with no
+  surviving design rationale — treated here as an inaccurate description of
+  a bug, not a protected feature, and updated accordingly.
 - **Count-in follows the song's meter and its pickup measure.** The count-in
   (loop wrap, section practice, and the "Countdown before song" setting) always
   clicked exactly four beats, so a 3/4 song was counted in 4/4, and a song
@@ -411,6 +433,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   bar shorter than that meter shortens the count by its length: a 1-beat pickup
   in 4/4 counts "1 2 3" and the music enters on 4. Songs without beats — pre-chart,
   minigames, synthetic highways — still get four.
+
 - **GP8 asset resolution honours the directory the registry named.**
   `<EmbeddedFilePath>` is matched on filename stem so a format variant of the
   same recording can win (an `.ogg` beside the declared `.mp3` is copied out

lib/gp2rs_gpx.py

@@ -2531,12 +2531,29 @@ def _auto_select_gpx(tracks: list[dict]) -> tuple[list[int], dict[int, str]]:
 
         is_bass = (t['string_pitches'] and max(t['string_pitches']) <= 48) \
             or t['midi_program'] in BASS_PROGS
-        is_guitar = bool(t['string_pitches']) and not is_bass
+        # GP6+ often notates piano/keys parts on a fretted string template, so
+        # string_pitches alone can't distinguish a keyboard part from a real
+        # guitar (feedBack: "Combo" mislabeled piano arrangement). Gate only
+        # the midi_program heuristic on `not string_pitches` (to avoid
+        # grabbing real guitars); a name/arrangement keyword forces keys
+        # regardless, so a track explicitly named "Keys ..." isn't swept into
+        # the unhinted-guitar Lead/Rhythm/Combo bucket just because it has
+        # string tuning data. This mirrors convert_gpif's sibling is_keys rule
+        # above (line ~1682) exactly, including its accepted trade-off: a
+        # genuinely fretted guitar whose name merely *contains* "piano"/
+        # "keys"/"organ" as a substring (e.g. "Keys of the Kingdom") is a
+        # false positive here too, same as there — the name match is the
+        # only signal these two functions have for a fretted keyboard part,
+        # so it has to win. Check is_keys before is_guitar so the keys match
+        # actually takes effect.
         is_keys = (not t['string_pitches'] and t['midi_program'] in KEYS_PROGS) \
-            or any(kw in name_l for kw in ('piano', 'keys', 'organ'))
+            or any(kw in name_l for kw in ('piano', 'keys', 'keyboard', 'organ'))
+        is_guitar = bool(t['string_pitches']) and not is_bass and not is_keys
 
         if is_bass:
             selected.append((i, 'bass'))
+        elif is_keys:
+            selected.append((i, 'keys'))
         elif is_guitar:
             # Honor "lead"/"rhythm" in the GP track name so two guitars keep
             # the author's roles instead of being labelled by appearance order
@@ -2547,8 +2564,6 @@ def _auto_select_gpx(tracks: list[dict]) -> tuple[list[int], dict[int, str]]:
                 selected.append((i, 'guitar_rhythm'))
             else:
                 selected.append((i, 'guitar'))
-        elif is_keys:
-            selected.append((i, 'keys'))
 
     if not selected:
         for i, t in enumerate(tracks):

lib/routers/ws_highway.py

@@ -473,7 +473,15 @@ def _evict_audio_cache():
                     tmp_suffix = uuid.uuid4().hex[:8]
                     tmp_base = appstate.audio_cache_dir / f"audio_{audio_id}.{tmp_suffix}"
                     try:
-                        produced = convert_wem(str(wem_resolved), str(tmp_base))
+                        # convert_wem shells out to vgmstream-cli/ffmpeg via
+                        # subprocess.run (up to 120s timeout) — bare-calling it
+                        # here would block the whole event loop, stalling every
+                        # other concurrent connection's WebSocket traffic for
+                        # as long as the conversion takes.
+                        produced = await loop.run_in_executor(
+                            None,
+                            lambda: _ctx.run(convert_wem, str(wem_resolved), str(tmp_base)),
+                        )
                         ext = Path(produced).suffix
                         final_path = appstate.audio_cache_dir / f"audio_{audio_id}{ext}"
                         os.replace(produced, final_path)
@@ -495,7 +503,13 @@ def _evict_audio_cache():
                 audio_error = "No WEM audio files were found inside this archive."
             else:
                 try:
-                    audio_path = convert_wem(wem_files[0], os.path.join(tmp, "audio"))
+                    # Same reasoning as the loose-folder conversion above:
+                    # convert_wem is a blocking subprocess call and must not
+                    # run inline on the event loop.
+                    audio_path = await loop.run_in_executor(
+                        None,
+                        lambda: _ctx.run(convert_wem, wem_files[0], os.path.join(tmp, "audio")),
+                    )
                     ext = Path(audio_path).suffix
                     audio_dest = appstate.audio_cache_dir / f"audio_{audio_id}{ext}"
                     shutil.copy2(audio_path, audio_dest)
@@ -513,6 +527,11 @@ def _evict_audio_cache():
                 "name": a.name,
                 "smart_name": smart_names[i],
                 "notes": len(a.notes) + sum(len(c.notes) for c in a.chords),
+                # Manifest `type` (sloppak.py:942) — authoritative instrument
+                # classification, independent of the display name. Lets viz
+                # auto-selection (e.g. the piano viz's matchesArrangement)
+                # match on real type instead of name-sniffing.
+                "type": (a.type or "").strip().lower() if isinstance(a.type, str) else "",
             }
             for i, a in enumerate(song.arrangements)
         ]
@@ -525,6 +544,9 @@ def _evict_audio_cache():
             "arrangement": arr.name,
             "arrangement_smart_name": smart_names[best],
             "arrangement_index": best,
+            # Named distinctly from the top-level "type" (WS message
+            # discriminator, = "song_info") to avoid colliding with it.
+            "arrangement_type": (arr.type or "").strip().lower() if isinstance(arr.type, str) else "",
             # Echo the resolved naming mode so highway.js doesn't have to
             # re-read localStorage (which can be unavailable / disagree with
             # app.js's in-memory cache when storage writes fail).

lib/song.py

@@ -805,6 +805,14 @@ def _resolve(a: Arrangement) -> tuple[str | None, bool]:
             return "path_rhythm", bool(a.bonus_arr)
         if a.path_bass:
             return "path_bass", bool(a.bonus_arr)
+        # The manifest `type` field (sloppak.py:942) is authoritative when
+        # present — trust it over name-sniffing. A "keys"/"piano"/"vocals"/
+        # "drums" arrangement is never part of the Lead/Rhythm/Bass grouping,
+        # even if its display name happens to collide with the name-fallback
+        # table below (e.g. a keys arrangement literally named "Combo").
+        arr_type = (a.type or "").strip().lower() if isinstance(a.type, str) else ""
+        if arr_type in ("keys", "piano", "vocals", "drums"):
+            return None, bool(a.bonus_arr)
         name = a.name if isinstance(a.name, str) else ""
         entry = _NAME_FALLBACK.get(name.strip().lower())
         if entry is None:

plugins/highway_3d/CLAUDE.md

@@ -126,7 +126,7 @@ Each entry names the function or banner you should grep for, plus key sub-blocks
 
 ### Lyrics & overlays
 - **Lyrics overlay** → `drawLyrics()`. 2D canvas, top centre, semi-transparent rounded background, syllable-level highlighting (current syllable in white, played in muted, upcoming in dim).
-- **Chord diagram overlay** → `drawChordDiagram()` (see "Chords" above). 2D canvas, top-left, fades over the 1.1 s linger window. Respects `inverted` (column 0 is high-e when inverted, low-E otherwise).
+- **Chord diagram overlay** → `drawChordDiagram()` (see "Chords" above). 2D canvas, top-left, fades over the 1.1 s linger window. `drawChordDiagram()` still accepts an `inverted` param (column 0 is low-E when inverted, high-e otherwise), but the two call sites always pass `inverted: false` — the diagram's orientation is fixed and does not mirror when the highway's own Invert toggle is on.
 - **The `lyricsCanvas`** is created in `initScene()` with `z-index:1`, appended to `wrap` **after** `ren.domElement` — this is the empirically-correct stacking order for all browsers/contexts (including splitscreen panels with `position:relative; overflow:hidden`). Don't reorder; see Pitfall #5.
 
 ### Splitscreen

plugins/highway_3d/screen.js

@@ -1498,7 +1498,7 @@
      */
     const ARP_INFER_MIN_HAND_SHAPE_SPAN_S = 0.21;
     /**
      * In a **short** chart window, chord strums (same voicing, strings picked
      * within ~30–45 ms) barely exceed this total spread; real arpeggios in that
      * window are usually slower across strings OR have 4+ plucks.
      */
@@ -16561,7 +16561,9 @@
                                 ? Math.min(1.0, Math.max(0, (bundle.currentTime - _diagPrev.t) / DIAG_ENTRANCE_S))
                                 : 1.0,
                             canvasW: lyricsCanvas.width, canvasH: lyricsCanvas.height,
-                            inverted: _invertedCached,
+                            // Chord diagram orientation is fixed regardless of the
+                            // highway's own Invert toggle.
+                            inverted: false,
                             sizeSlider: chordDiagramSize, position: chordDiagramPosition,
                             nStr: _diagPrev.nStr ?? nStr,
                             lyricsBottom,
@@ -16575,7 +16577,9 @@
                             opacity: Math.max(0, 1 + (_diagChord.t - bundle.currentTime) / DIAG_LINGER_S),
                             entranceT: _diagEntranceT,
                             canvasW: lyricsCanvas.width, canvasH: lyricsCanvas.height,
-                            inverted: _invertedCached,
+                            // Chord diagram orientation is fixed regardless of the
+                            // highway's own Invert toggle.
+                            inverted: false,
                             sizeSlider: chordDiagramSize, position: chordDiagramPosition,
                             nStr: _diagChord.nStr ?? nStr,
                             lyricsBottom,
@@ -16679,7 +16683,13 @@
     // arrangements that merely contain these as substrings (e.g. a
     // "BasslineKeys" arrangement would otherwise match `bass`).
     window.feedBackViz_highway_3d.matchesArrangement = function (songInfo) {
-        const arr = (songInfo && songInfo.arrangement) || '';
+        // Manifest `type: keys`/`type: piano` is authoritative and independent
+        // of the display name — a keys/piano arrangement literally named
+        // "Combo" (GP import quirk) would otherwise match the /combo/
+        // keyword below and steal the song from the piano/keys viz. Yield
+        // whenever the active arrangement's real type says so.
+        if (songInfo && (songInfo.arrangement_type === 'keys' || songInfo.arrangement_type === 'piano')) return false;
+        const arr = songInfo?.arrangement || '';
         return /\b(?:lead|rhythm|bass|combo|guitar)\b/i.test(arr);
     };
 

static/app.js

@@ -1001,6 +1001,7 @@
     // migration step — playback genuinely continues, so don't emit song:play or
     // flip feedBack.isPlaying (the watcher keeps the canonical state itself).
     if (window._juceRerouteInProgress) return;
+    if (window._stemsRerouteInProgress) return;
     window.feedBack.isPlaying = true;
     const payload = _songEventPayload();
     window.feedBack.emit('song:play', payload);
@@ -1011,6 +1012,7 @@
     // Same as above: suppress the song:pause emitted by a reroute's deliberate
     // audio.pause() — the migration is transparent to plugin play-state.
     if (window._juceRerouteInProgress) return;
+    if (window._stemsRerouteInProgress) return;
     window.feedBack.isPlaying = false;
     window.feedBack.emit('song:pause', _songEventPayload());
 });
@@ -1496,7 +1498,7 @@
         'position:fixed', 'inset:0', 'z-index:200', 'display:flex',
         'align-items:center', 'justify-content:center',
         'background:rgba(0,0,0,0.6)',
         'font:14px/1.4 system-ui,-apple-system,"Segoe UI",Roboto,sans-serif',
     ].join(';');
 
     const card = document.createElement('div');

static/js/transport.js

@@ -358,6 +358,12 @@ export async function togglePlay() {
             // leave the button showing Play while the song keeps playing — the
             // "two clicks to pause on the first song after a fresh load" bug.
             if (window._juceRerouteInProgress) return;
+            // Same shape of race, HTML5 -> stems-plugin Web-Audio takeover
+            // (get-flashbacks/feedBack#39): the stems plugin deliberately
+            // pauses the core element while it builds its own multi-stem
+            // transport, then dispatches a synthetic 'play' once that
+            // transport actually starts. Don't stomp the button in between.
+            if (window._stemsRerouteInProgress) return;
             console.error('[app] audio.play() rejected:', err);
             S.isPlaying = false;
             setPlayButtonState(false);

tests/test_gp2rs_gpx.py

@@ -1310,3 +1310,43 @@ def test_auto_select_gpx_unhinted_does_not_steal_later_rhythm():
     assert names[indices[0]] == "Lead"
     assert names[indices[2]] == "Rhythm"
     assert names[indices[1]] == "Combo"
+
+
+# This PR's headline fix: GP6+ often notates a piano/keys part on a fretted
+# string template, so a keyboard track named "Keys ..." usually HAS
+# string_pitches too. is_keys's name match must win over that fret data —
+# gating it on `not string_pitches` (an earlier revision of this fix did,
+# briefly, before review caught that it silently undid this exact case) would
+# sweep a real keyboard part back into the unhinted-guitar Lead/Rhythm/Combo
+# bucket, the precise "Combo" mislabeling this PR exists to fix.
+_GPIF_FRETTED_KEYS_TRACK = """
+<GPIF>
+  <Score><Title>T</Title><Artist>A</Artist></Score>
+  <Tracks>
+    <Track id="0"><Name>Keys</Name>
+      <Property name="Tuning"><Pitches>40 45 50 55 59 64</Pitches></Property></Track>
+  </Tracks>
+  <MasterBars><MasterBar><Time>4/4</Time><Bars>0</Bars></MasterBar></MasterBars>
+  <Bars>
+    <Bar id="0"><Voices>0</Voices></Bar>
+  </Bars>
+  <Voices>
+    <Voice id="0"><Beats>0</Beats></Voice>
+  </Voices>
+  <Beats>
+    <Beat id="0"><Rhythm ref="r0"/><Notes>0</Notes></Beat>
+  </Beats>
+  <Notes>
+    <Note id="0"><Property name="String"><String>0</String></Property><Property name="Fret"><Fret>0</Fret></Property></Note>
+  </Notes>
+  <Rhythms><Rhythm id="r0"><NoteValue>Quarter</NoteValue></Rhythm></Rhythms>
+</GPIF>
+"""
+
+
+def test_auto_select_gpx_fretted_track_named_keys_is_still_classified_as_keys():
+    root = ET.fromstring(_GPIF_FRETTED_KEYS_TRACK)
+    tracks = gp2rs_gpx._gpif_tracks(root)
+    assert tracks[0]['string_pitches']  # sanity: real fret data is present
+    _indices, names = gp2rs_gpx._auto_select_gpx(tracks)
+    assert list(names.values()) == ["Keys"]

tests/test_song.py

@@ -1235,14 +1235,15 @@ def test_arrangement_is_bass_signal_safety():
 # ── compute_smart_names ───────────────────────────────────────────────────────
 
 def _sarr(path_lead=False, path_rhythm=False, path_bass=False,
-          bonus_arr=False, represent=0, name="Combo") -> Arrangement:
+          bonus_arr=False, represent=0, name="Combo", type="") -> Arrangement:
     return Arrangement(
         name=name,
         path_lead=path_lead,
         path_rhythm=path_rhythm,
         path_bass=path_bass,
         bonus_arr=bonus_arr,
         represent=represent,
+        type=type,
     )
 
 
@@ -1328,6 +1329,18 @@ def test_smart_names_combo_treated_as_lead():
     assert compute_smart_names(arrs) == ["Lead"]
 
 
+def test_smart_names_piano_typed_combo_not_treated_as_lead():
+    # Kilo Code Review finding on feedBack#42: the manifest `type` guard
+    # checked "keys"/"vocals"/"drums" but omitted "piano" (also a first-class
+    # Arrangement.type value, per song.py's own docstring and progression.py's
+    # arr_type in ("piano", "keys") check) — a piano-typed arrangement
+    # literally named "Combo" fell through to the name fallback and was
+    # grouped as Lead, the exact GP-import misclassification this guard
+    # exists to prevent.
+    arrs = [_sarr(name="Combo", type="piano")]
+    assert compute_smart_names(arrs) == [None]
+
+
 def test_smart_names_recognises_display_names_from_load_song():
     # load_song() synthesises display names like "Bonus Lead" / "Bass 2"
     # when manifest JSON is missing. compute_smart_names must classify them