Repository navigation
Promote virtuoso-dev → main (v0.2.2) — the 2026-07-14 batch - #4
Merged
Merged
Conversation
…-flare (v0.1.10)
Two grading-DISPLAY bugs; detection was always correct.
Card 0%/0-of-0 hero: shareCardAction preferred note_detect's
renderResultsCard, whose hero reads note_detect's own 0/0 last-session
counters (the contained verifier scored, not nd's UI) while Virtuoso's
real numbers only reached the stats[] override row. Retire the delegation
so Virtuoso draws its own verdict-led, skinned card (the ratified v0.1.4
"keep our renderer, don't consume note_detect's" decision); this also
restores the skins + mastery crest the delegation silently suppressed on
desktop. _ndCardApi / _resultsCardData / _virCardOverlay kept dormant for
a possible future host-level card API.
3D highway hit-flare invisible: highway_3d's feedBack#254 per-note
provider (getNoteState) is authoritative over the window-event marks and
culls a gem ~100ms past the strike line unless the provider returns a live
verdict that frame. Our provider only reported 'hit' after the async
contained verdict (~150-400ms late), so the gem was gone before green
could paint. Add a display-only { state:'active', live:true } keep-alive
from the live YIN ear (mirrors note_detect's own noteStateFor): pitch-gated
no looser than the credit floor, level-gated, +/-0.12s window; it renders
no judgment and drives no credit - credit stays verifier-only via
_ptScored. Contract confirmed present in highway_3d 3.30.0 (desktop bundle)
and 3.31.3 (core).
Verified on the installed desktop (real DI dogfood).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughVirtuoso is updated to version 0.2.2 with dedicated tapping ladders, instrument-aware generation, coach and preview UX, host-topbar parity, a classic 2D highway fallback, provisional pitch display states, and expanded deterministic Playwright smoke coverage. ChangesVirtuoso practice and interaction update
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant VirtuosoUI
participant HostBus
participant HostAPI
User->>VirtuosoUI: open header tuner, profile, or setup
VirtuosoUI->>HostAPI: read tunings, settings, or profile
HostAPI-->>VirtuosoUI: return host data
VirtuosoUI->>HostBus: subscribe to host changes
HostBus-->>VirtuosoUI: update header and tuning state
sequenceDiagram
participant User
participant ResultsModal
participant CoachPrescription
participant PathwaySelector
participant PreviewPlayback
User->>ResultsModal: inspect verdict
ResultsModal->>CoachPrescription: compute recommendation
User->>PathwaySelector: choose Practice next
PathwaySelector->>PreviewPlayback: focus recommended practice
User->>PreviewPlayback: select Hear it
PreviewPlayback-->>User: play target notes without scoring
🚥 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: 1
🧹 Nitpick comments (1)
screen.js (1)
23328-23340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the contradictory share-card roadmap.
ROADMAP.mdLines 19-24 still directs future work to consumenote_detect’s renderer and records delegation as built. Update it so a later change does not restore this retired, faulty path.🤖 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 `@screen.js` around lines 23328 - 23340, Update the share-card roadmap entries around the note_detect renderer to document that Virtuoso retains its own renderer and that delegation to note_detect is retired, correcting any statement that delegation is built or planned. Preserve the rationale that the local renderer supplies accurate contained-verifier metrics and the required skins/crest, so future work does not restore the deprecated path.
🤖 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 `@screen.js`:
- Around line 22500-22524: In the live keep-alive logic, missing recent level
samples must not count as valid level evidence. Update the `leveled` calculation
in the `_ptLive` block to require `ptHasSamplesIn(lo, now)` and
`ptPeakLevelIn(lo, now) >= PT_LVL_FLOOR_MIN` whenever `_lvlMode !== 'none'`;
only bypass this check when level mode is disabled.
---
Nitpick comments:
In `@screen.js`:
- Around line 23328-23340: Update the share-card roadmap entries around the
note_detect renderer to document that Virtuoso retains its own renderer and that
delegation to note_detect is retired, correcting any statement that delegation
is built or planned. Preserve the rationale that the local renderer supplies
accurate contained-verifier metrics and the required skins/crest, so future work
does not restore the deprecated path.
🪄 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: 625674b4-57d8-4d78-8115-b4a4892754ce
📒 Files selected for processing (2)
plugin.jsonscreen.js
Comment on lines
+22500
to
+22524
| // Live keep-alive for the borrowed highway (feedBack#254 provider): while this note | ||
| // straddles the strike line AND the live ear hears its target pitch NOW, return a | ||
| // display-only { state:'active', live:true } so the host gem stays lit until the async | ||
| // contained verdict credits it — the provider-cull would otherwise delete the gem | ||
| // ~100ms past the line, before our verdict lands (the "hits invisible on the 3D | ||
| // highway" bug). MIRRORS note_detect's own noteStateFor provisional glow: it renders | ||
| // NO judgment and drives NO credit ('hit'/'miss'/credit stay verifier-only, above); | ||
| // the 2D strip's string check ignores the object, so that surface stays credited-only. | ||
| // live:true routes it through the highway's non-latched 'hit-live' branch, so it | ||
| // extinguishes on mute / relights on re-strike and can never stick a false green. | ||
| if (_ptLive && _ptLive.c >= PT_CONF_INPUT) { | ||
| const age = now - _ptLive.t, om = _ptOpenMidis[note.s]; | ||
| if (age > -0.05 && age < 0.08 && om != null && now >= tn.t - 0.12 && now <= end + 0.12) { | ||
| const cents = Math.abs(1200 * Math.log2(_ptLive.f / midiToFreq(om + note.f))); | ||
| const tol = note.bn ? PT_BEND_CENTS : 50; // no looser than the credit floor | ||
| // level/onset evidence — the SAME host-mirror tap the credit gate trusts, so a | ||
| // confident YIN lock on an inaudible residual can't false-light the gem. | ||
| let leveled = true; | ||
| if (_lvlMode !== 'none') { const lo = now - PT_LVL_RECENT_WIN; leveled = !ptHasSamplesIn(lo, now) || ptPeakLevelIn(lo, now) >= PT_LVL_FLOOR_MIN; } | ||
| if (cents <= tol && leveled) { | ||
| const alpha = 0.55 + 0.4 * Math.max(0, Math.min(1, (_ptLive.c - PT_CONF_INPUT) / (1 - PT_CONF_INPUT))); | ||
| return { state: 'active', live: true, alpha }; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not treat missing level samples as positive evidence.
Line 22518 sets leveled to true when no recent samples exist. A confident residual can therefore light the gem without the required level evidence.
Proposed fix
- if (_lvlMode !== 'none') { const lo = now - PT_LVL_RECENT_WIN; leveled = !ptHasSamplesIn(lo, now) || ptPeakLevelIn(lo, now) >= PT_LVL_FLOOR_MIN; }
+ if (_lvlMode !== 'none') { const lo = now - PT_LVL_RECENT_WIN; leveled = ptHasSamplesIn(lo, now) && ptPeakLevelIn(lo, now) >= PT_LVL_FLOOR_MIN; }📝 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.
Suggested change
| // Live keep-alive for the borrowed highway (feedBack#254 provider): while this note | |
| // straddles the strike line AND the live ear hears its target pitch NOW, return a | |
| // display-only { state:'active', live:true } so the host gem stays lit until the async | |
| // contained verdict credits it — the provider-cull would otherwise delete the gem | |
| // ~100ms past the line, before our verdict lands (the "hits invisible on the 3D | |
| // highway" bug). MIRRORS note_detect's own noteStateFor provisional glow: it renders | |
| // NO judgment and drives NO credit ('hit'/'miss'/credit stay verifier-only, above); | |
| // the 2D strip's string check ignores the object, so that surface stays credited-only. | |
| // live:true routes it through the highway's non-latched 'hit-live' branch, so it | |
| // extinguishes on mute / relights on re-strike and can never stick a false green. | |
| if (_ptLive && _ptLive.c >= PT_CONF_INPUT) { | |
| const age = now - _ptLive.t, om = _ptOpenMidis[note.s]; | |
| if (age > -0.05 && age < 0.08 && om != null && now >= tn.t - 0.12 && now <= end + 0.12) { | |
| const cents = Math.abs(1200 * Math.log2(_ptLive.f / midiToFreq(om + note.f))); | |
| const tol = note.bn ? PT_BEND_CENTS : 50; // no looser than the credit floor | |
| // level/onset evidence — the SAME host-mirror tap the credit gate trusts, so a | |
| // confident YIN lock on an inaudible residual can't false-light the gem. | |
| let leveled = true; | |
| if (_lvlMode !== 'none') { const lo = now - PT_LVL_RECENT_WIN; leveled = !ptHasSamplesIn(lo, now) || ptPeakLevelIn(lo, now) >= PT_LVL_FLOOR_MIN; } | |
| if (cents <= tol && leveled) { | |
| const alpha = 0.55 + 0.4 * Math.max(0, Math.min(1, (_ptLive.c - PT_CONF_INPUT) / (1 - PT_CONF_INPUT))); | |
| return { state: 'active', live: true, alpha }; | |
| } | |
| } | |
| } | |
| // Live keep-alive for the borrowed highway (feedBack#254 provider): while this note | |
| // straddles the strike line AND the live ear hears its target pitch NOW, return a | |
| // display-only { state:'active', live:true } so the host gem stays lit until the async | |
| // contained verdict credits it — the provider-cull would otherwise delete the gem | |
| // ~100ms past the line, before our verdict lands (the "hits invisible on the 3D | |
| // highway" bug). MIRRORS note_detect's own noteStateFor provisional glow: it renders | |
| // NO judgment and drives NO credit ('hit'/'miss'/credit stay verifier-only, above); | |
| // the 2D strip's string check ignores the object, so that surface stays credited-only. | |
| // live:true routes it through the highway's non-latched 'hit-live' branch, so it | |
| // extinguishes on mute / relights on re-strike and can never stick a false green. | |
| if (_ptLive && _ptLive.c >= PT_CONF_INPUT) { | |
| const age = now - _ptLive.t, om = _ptOpenMidis[note.s]; | |
| if (age > -0.05 && age < 0.08 && om != null && now >= tn.t - 0.12 && now <= end + 0.12) { | |
| const cents = Math.abs(1200 * Math.log2(_ptLive.f / midiToFreq(om + note.f))); | |
| const tol = note.bn ? PT_BEND_CENTS : 50; // no looser than the credit floor | |
| // level/onset evidence — the SAME host-mirror tap the credit gate trusts, so a | |
| // confident YIN lock on an inaudible residual can't false-light the gem. | |
| let leveled = true; | |
| if (_lvlMode !== 'none') { const lo = now - PT_LVL_RECENT_WIN; leveled = ptHasSamplesIn(lo, now) && ptPeakLevelIn(lo, now) >= PT_LVL_FLOOR_MIN; } | |
| if (cents <= tol && leveled) { | |
| const alpha = 0.55 + 0.4 * Math.max(0, Math.min(1, (_ptLive.c - PT_CONF_INPUT) / (1 - PT_CONF_INPUT))); | |
| return { state: 'active', live: true, alpha }; | |
| } | |
| } | |
| } |
🤖 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 `@screen.js` around lines 22500 - 22524, In the live keep-alive logic, missing
recent level samples must not count as valid level evidence. Update the
`leveled` calculation in the `_ptLive` block to require `ptHasSamplesIn(lo,
now)` and `ptPeakLevelIn(lo, now) >= PT_LVL_FLOOR_MIN` whenever `_lvlMode !==
'none'`; only bypass this check when level mode is disabled.
…ion refs New 2026-07-10 STOPPED HERE: card 0/0 (retired the stale note_detect card delegation) + 3D highway flare (live-ear keep-alive vs the feedBack#254 provider cull), the Program-Files installed-app deploy gotcha, and the version corrections (highway_3d 3.30.0 desktop / 3.31.3 core; note_detect 1.28.0 desktop) that supersede the stale 3.26/1.19 refs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ROADMAP.md`:
- Around line 29-30: Mark the earlier note_detect results-card delegation plan
in the “Planned” section as superseded or remove it entirely. Update references
around the local renderer decision so future work consistently uses Virtuoso’s
own renderShareCardImage implementation and does not suggest consuming
_ndCardApi or related delegation APIs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
… settings sync (v0.1.11)
"fullscreen":true hides the host topbar on our screen, so the header now
mirrors its three badges in the host's exact order (compact 32px density
variants of the host cards). Spec: docs/topbar-host-parity.md.
HOST CHECK (feedback-compatibility, 2026-07-10, vs FeedBack 0.3.0-alpha.1):
- Tuner: BORROW launch (window.tuner.toggle, feature-detected) + MIRROR card
(live note via bus 'tuner:frame'). Flip: a host-embeddable badge component.
- Instrument: BORROW persistence (GET/POST /api/settings, GET /api/tunings,
workingTuning.setCurrentInstrument, 'instrument:changed') + MIRROR badge/
panel UI. Host = source of truth for guitar/bass; piano (gated) never POSTs.
Flip: host adding piano to STRING_COUNTS.
- Profile: MIRROR chip (avatar+streak+rank, /api/profile*) + LINK
(goScreen('v3-profile')); renders nothing when !onboarded. Never a second
profile/economy implementation.
Sync semantics: user panel changes funnel through instrumentStoreSave (the L1
declaration boundary) -> debounced POST + workingTuning + instrument:changed +
tuner-config push (gated on window.tuner: a blind POST 404s and trips the
smoke console-error guards). Boot reconcile: local L1 store wins and pushes
host-ward (pre-merge tunings live only locally; host default is
indistinguishable from untouched); fresh install adopts host; live external
'instrument:changed' always pulls. Per-rung overrides never write (anti-leak).
Interop maps through ABSOLUTE open-string midis only. note_detect reads
neither workingTuning nor /api/settings - the per-call verify ctx stays.
Also: progress chip hidden (pending the profile-merge design; P-sheet opens
via new gear-menu "Progress (P)" + the P hotkey), setup label now
instrument-strings-tuning, Floating-tuner courtesy button retired (the badge
is that affordance), Tune... relabeled "Tune to this tuning...", tuning
dropdown gains the host named-tuning group (stable built-in ids preserved -
deliberate deviation from spec's host-name-wins: id stability beats label
provenance), host-parity uniform standards added as offset:true presets
(C#/C std 6-str, Bb/A std 7-str, E std 8-str), and the latent
guitar_8_standard.tuning [2,0,...] metadata bug fixed to [0,...] (host
8-string base is F#; the old offset would detune string 0 a whole step on a
host round-trip).
Smoke: 16/19 - the 3 reds (backing-engine voice-leading, progress, variation)
fail identically on base (pre-existing).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
…ds (v0.1.12) Audit verdict: "8-string exercises ignore the extra strings" was DESIGN, not plumbing - string count/tuning reach every builder, but CAGED/Open anchor the top-six EADGBE subset by construction (correct for CAGED itself) and 'caged' was the universal beginner/session default, so most exercises never touched strings 7-8 (renderers draw all N lanes; the bottom two just sat empty). Fix (guitar-pedagogy + bass-pedagogy + metal-idiom panel): - defaultFretboardSystem(instrument, count): bass -> 'position' (bass never uses guitar shape systems; the pathway path already coded this - now Custom/beginner + sessions agree), guitar N>6 -> '3nps' (THE extended-range scale system - tiles every string, so 7/8-string runs actually reach the low B/F#), guitar 6 -> 'caged' (unchanged). Explicit advanced-mode picks and rung-coded systems always win; a rung that TEACHES a CAGED shape stays a CAGED lesson. Wired into readConfig + buildSegmentConfig. - Instrument-first guards (defense-in-depth) on every count-gated shape path: resolveCurrentShape, cagedShapeNotesForChord, templateFromShape, sweep wantShape. Kills a REAL latent pitch bug: a 6-string bass satisfies the old count-only >=6 guards, but the CAGED templates bake EADGBE's G->B major 3rd - on an all-4ths B-E-A-D-G-C bass the top two strings land a semitone flat. Only UI suppression protected it (hidden shape controls); programmatic configs (preset import, host sync) were exposed. Smoke: new smoke-strings rows (12) default routing (8-string default=3nps, reaches s=0/s=1, no-unison holds; 6-string stays caged; explicit caged preserved) + (13) bass never resolves a shape, forced 6-string-bass caged sweeps stay diatonic, 5-string scale plays the low-B string. Suite green; full run 16/19 with only the pre-existing base reds (backing-engine voice- leading row, progress, variation); contained-verifier + level-gate-async flaked under concurrency, green solo. Deferred by panel verdict (ROADMAP open thread): opt-in extended-range CAGED window-continuation (never a re-rooted box), bassRootGrip low-fifth downward reach, drop-tuning s0+s1+s2 barre primitive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
…own (v0.1.13) The in-tree 2D highway was only reachable as a silent fallback (builtin_2d is the Jumping Tab borrow slot; highway_3d falls back to it too). New first-class kind 'highway_2d' -> makeBuiltin2DRenderer, always available, any string count. Five stringed views + Piano Roll outgrew the segmented pill row, so the view switcher is now a dropdown (#virtuoso-view-select) in the same control family as the mode/settings selects. change -> onViewSwitch (same path a pill click took); syncViewSwitcher mirrors what's ACTUALLY rendered (saved-pref restore, >8-string force) by value-set only, so syncing never re-triggers a switch. Piano/stringed option visibility keeps the old CSS mechanism, retargeted from .virtuoso-view-btn[data-renderer] to option[value]. highway_2d is fretboard-strip capable; theme toggle stays Tab/Notation-only; hand-marks off-view stays 3D-only. Retired .virtuoso-view-btn/.virtuoso-view-tabs CSS kept (local shot-*.mjs scripts + skin overrides still reference it) with a RETIRED note; no live markup uses it. Harness: driver.mjs + all suites drive the dropdown (value + bubbled change); readiness waits retargeted to #virtuoso-view-select; smoke-renderers gains a highway_2d row (enforcePixels: true - it draws into the in-tree canvas); smoke-highway-settings cycles it through the attach/detach churn loop; driver.mjs screenshot/all-renderers accept highway_2d. Smoke: 16/19 - same 3 pre-existing base reds (backing-engine voice-leading, progress, variation). Screenshot-verified: dropdown renders, 2D highway attaches + draws lanes/gems. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
…ailable Clarified target: the 2D Highway view means Byron's Classic 2D Highway (host core), not Jumping Tab and not merely our in-tree renderer. HOST CHECK (2026-07-10): it is createHighway()'s private _defaultRenderer (static/highway.js), picker id 'default' (RESERVED), no feedBackViz factory, closure-fed + websocket-fed (no chart setter) -> NOT borrowable today; verdict MIRROR, flips to BORROW when the host ships a bundle-driven feedBackViz_highway_2d factory (asked in got-feedBack/feedBack#835). Wiring is borrow-FIRST via a factory-global probe only (vizFactoryFor - no speculative script fetch; the URL doesn't exist yet and a blind 404 would trip the smoke console-guards). The moment the host registers the factory, every install upgrades to the real Classic 2D Highway with zero plugin changes; until then the in-tree 2D highway renders the slot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
…ination) The v0.1.11 host-settings sync treats an empty localStorage as a fresh install and ADOPTS host config on boot. Smoke pages always boot with empty localStorage, and any suite whose panel drives dispatch real change events WRITES THROUGH to the persistent host config - so a later suite (or a later run) could boot into whatever instrument the previous one left (the panel flipped to bass mid-suite; smoke-strings rows 3/4 went red, and this is the likely mechanism behind the contained-verifier/level-gate-async flakes under the parallel run-all). Fix: every suite (and driver.mjs) seeds localStorage['virtuoso.instrument'] with the 6-string-standard default via addInitScript BEFORE page scripts run. The boot reconcile then takes the local-wins path - deterministic panel state regardless of suite ordering, and each boot heals the host config for the suites after it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
…rre (v0.1.14)
powerChordGrip hardcoded the standard-4ths offsets (+2 on s1/s2), so in every
tuning where the s0->s1 interval is a FIFTH (drop-D/C/B, drop-A, the bass
drops, DADGAD, Open D) the '5'/'5oct' grip sounded root + MAJOR 6TH instead of
root + 5th - a real wrong pitch on the strum_comp/pickStrumGrip path.
Fret offsets are now computed BY PITCH against the actual open intervals:
in a drop tuning the grip collapses to the iconic one-finger SAME-FRET barre
{s0:F, s1:F, s2:F} = root-5th-octave; standard tuning keeps {F, F+2, F+2}
unchanged. Guitar-pedagogy 2026-07-12: gate on the interval (DADGAD/Open D are
TRUE positives - the barre is the idiomatic voicing there), apply by default
(the barre is the only correct voicing in a drop tuning), F=0 is a valid open
voicing; templateFromPositions already fingers a same-fret row as a shared
index barre (fg 1) by construction. Negative-fret guard bails on hostile
custom tunings. (buildPedalRiffExercise's own s1/s2/s3 power chord was already
interval-correct - untouched.)
Smoke: smoke-strings row (14) - drop-D 5oct = same-fret barre sounding
root-5th-octave; standard grip unchanged. Full run 16/19 (the 3 pre-existing
base reds); the contained-verifier/level-gate flakes stayed green with the
suite seeding from the previous commit.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
bassRootGrip only reached UP (fifth s+1, octave s+2); the fifth BELOW the root - the reach the low string exists for, and the whole point of a 5-string's B - had no primitive (deferred from the string-count audit). bassRootGrip gains lowFifth: the P5 below the root on the string below, computed by pitch with an exact-midi check. Guards (bass-pedagogy spec 2026-07-12): root.s>=1 (a string exists below) AND root.f>=2 (at f0/f1 the pc-math octave-push wraps the note UP a 4th - the opposite of intent); null when unavailable. root_fifth_octave gains the opt-in rfoPattern='low5' variant: R-low5-5-8 (root on the downbeat, traverses both fifths + the octave); an unreachable low fifth degrades to the upper 5th so the pattern stays playable everywhere. The default R-5-8-5 box is deliberately untouched (the canonical pre-scales lesson) - guarded byte-identical by smoke. Scope per spec: root_fifth_octave ONLY (octave_groove stays pure R-8 per the soul-motown ruling; the right_hand_technique modes are pitch-invisible by design). No root-selection bias - nearest-to-prev stands. The Root-5th-Octave rung gains a final vary step teaching the reach (rfoPattern coded in base so it never leaks across the rung's own vary steps). Smoke: smoke-strings row (15) - low5 geometry (R, root-7 on the string below, 5th, octave), default-parity (field absent === 'r5o', byte-identical), 4-string null-degrade to the upper 5th. Full run 16/19 (the 3 pre-existing base reds). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
…nderer (v0.1.16)
The `highway_2d` view slot rendered our horizontal-lane tab scroller, not the
2D highway Christian meant: Byron's "Classic 2D Highway" — the perspective
falling-fret highway that is createHighway()'s private _defaultRenderer
(static/highway.js + static/js/highway-{geometry,draw,state-primitives}.js).
HOST CHECK 2026-07-10 verdict was MIRROR (core/closure-fed, no viz factory to
borrow; issue got-feedBack/feedBack#835 asks the host to expose one, which
would flip this to a pure borrow).
New makeClassic2DHighwayRenderer() — a faithful port reading our bundle, with
the host constants/geometry lifted from source: #080810 BG, the perspective
project() (VISIBLE_SECONDS 3.0 / Z_CAM 2.2 / Z_MAX 10.0, strike line at
y=0.82), the centered fretX() trapezoid, converging fret lines, the beat
sweep, the DEFAULT_STRING_COLORS/DIM/BRIGHT palette, the glowing now line,
falling perspective-scaled fret-number gems (rounded-rect body + dim glow +
white fret number, open-string wide bar, dead-note X, technique glyphs),
sustain trails, and the amber fret-number rail with the live anchor-window
highlight. Fret window follows the chart anchors via the host's smoothed
displayMaxFret (0.4 rate), falling back to the notes' max fret when a chart
carries no anchors. Lefty mirror + inverted string order honored.
resolveRendererFactory's highway_2d slot now falls back to this (was
makeBuiltin2DRenderer); the borrow-first vizFactoryFor('highway_2d') probe is
unchanged, so the host factory still wins when #835 ships. makeBuiltin2DRenderer
stays the Jumping-Tab (builtin_2d) fallback and the >8-string path, untouched.
Deliberate approximations (documented in the renderer header, not oversights):
the exotic host overlays our bundle doesn't feed — unison-bend connectors,
strum-group brackets, master-difficulty phrase filtering, lyrics, chord frames
— are omitted; and a CREDITED hit keeps Virtuoso's own green-gem grammar
(#22c55e + glow, the cross-surface "a hit is unmistakable" guarantee from the
2026-07-09 tester fix) rather than the host's string-bright lit gem, so a hit
reads as clearly here as on our Tab/Notation surfaces (consume-the-judge:
credit stays verifier-only; this is display).
Verification: screenshot-confirmed static (receding trapezoid, fret lines,
strike line, string palette, 0-8 amber rail) AND playing (numbered gems
falling with perspective scale + sustain trails, near gem at the strike line).
Full smoke 16/19 - only the 3 pre-existing base reds (backing-engine,
progress, variation); smoke-renderers' pixel-enforced highway_2d row green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
…, v0.1.11) New Concepts pack carved out of Legato (the lone leg_tapping rung re-homed as tap_articulation). Designed by a 6-agent panel: learning-design (chair), guitar/bass-pedagogy, metal-idiom, harmony-theory, rhythm-meter. Engine: split buildTappingExercise on a tapArp flag. Plain 'tapping' is now the single-string tap->pull articulation drill (replaces the off-idiom "scalar tap +12 across the neck"); tapArp:true routes to buildTappedArpeggioExercise / buildTapCascadePath — idiomatic tap->pull->hammer cascade cells reusing the sweepArpeggioPositions grid (guitar 3-note EVH cell, bass 2-note tap->pull; wide interval always routed to the RH tap; fret cap 22; no-unison seam pass for the dim7 symmetry). tapArp plumbed like sweepStrings (hidden field + readConfig + anti-leak default). Rungs: guitar 5 (tap_articulation -> tap_cell -> tap_cascade -> tap_sevenths -> tap_changes, descending ceilings, tap_cascade density-ramps triplet->sextuplet); bass 4 (btap_articulation -> btap_arpeggio -> btap_groove -> btap_apply, register-clamped frets 5-12). One concept_tapping band (bass rungs instrument-hidden). Skill-tree edges, form-cues, and screen.html options wired. Verified: node --check + probe-tapping-ladders (9/9 rungs) + smoke-generators 134/134, core-purity 40/40, strings, renderers 8/8. Generation-path (core) work — no HOST CHECK needed. Deferred to v2: bass two-hand independence, bass open-string cascades, guitar tapped-pedal-point. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
There was a problem hiding this comment.
♻️ Duplicate comments (1)
screen.js (1)
22748-22762: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing level samples still satisfy the
leveledgate — unresolved from prior review.
leveledis true whenever there are no recent samples (!ptHasSamplesIn(...)), which is the opposite of the intended guard described in the comment above it: a confident residual with zero level evidence still lights the keep-alive gem.Proposed fix
- if (_lvlMode !== 'none') { const lo = now - PT_LVL_RECENT_WIN; leveled = !ptHasSamplesIn(lo, now) || ptPeakLevelIn(lo, now) >= PT_LVL_FLOOR_MIN; } + if (_lvlMode !== 'none') { const lo = now - PT_LVL_RECENT_WIN; leveled = ptHasSamplesIn(lo, now) && ptPeakLevelIn(lo, now) >= PT_LVL_FLOOR_MIN; }🤖 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 `@screen.js` around lines 22748 - 22762, The level-evidence gate in the live-note activation block must reject missing recent samples: update the `leveled` assignment inside the `_lvlMode !== 'none'` branch so it is true only when recent samples exist and `ptPeakLevelIn(lo, now)` meets `PT_LVL_FLOOR_MIN`, while preserving the existing behavior when `_lvlMode` is `'none'`.
🤖 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.
Duplicate comments:
In `@screen.js`:
- Around line 22748-22762: The level-evidence gate in the live-note activation
block must reject missing recent samples: update the `leveled` assignment inside
the `_lvlMode !== 'none'` branch so it is true only when recent samples exist
and `ptPeakLevelIn(lo, now)` meets `PT_LVL_FLOOR_MIN`, while preserving the
existing behavior when `_lvlMode` is `'none'`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf39986d-d697-48cb-9dbc-508d53a829f5
📒 Files selected for processing (4)
ROADMAP.mdplugin.jsonscreen.htmlscreen.js
ChrisBeWithYou
changed the title
cb6cc0d pushed; PR #4 now carries both v0.1.10 grading fixes and v0.1.11 tapping ladders. Corrects the STOPPED-HERE handoff + NEXT. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ROADMAP.md (1)
40-40: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate the beta version to 0.1.11.
This instruction still says
0.1.10-beta.1, but line 34 states that PR#4combines both releases and skips a standalone 0.1.10 tag. Following this handoff would produce a stale beta label.🤖 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 `@ROADMAP.md` at line 40, Update the beta version referenced in the NEXT roadmap entry to 0.1.11, replacing the stale 0.1.10-beta.1 value while preserving the existing release command and handoff details.
🤖 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 `@ROADMAP.md`:
- Line 33: Update the deferred-work note in ROADMAP.md by replacing “Not gated
the ladder on these” with “The ladder was not gated on these.”
---
Outside diff comments:
In `@ROADMAP.md`:
- Line 40: Update the beta version referenced in the NEXT roadmap entry to
0.1.11, replacing the stale 0.1.10-beta.1 value while preserving the existing
release command and handoff details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| - **Engine:** `buildTappingExercise` split — plain `tapping` = the single-string tap→pull ARTICULATION drill (replaces the off-idiom "scalar tap +12 across the neck" metal-idiom flagged); new `tapArp` flag → `buildTappedArpeggioExercise`/`buildTapCascadePath` = idiomatic tap→pull→hammer CASCADE cells (reuses `sweepArpeggioPositions` grid; guitar = 3-note EVH cell, bass = 2-note tap→pull; wide interval always routed to the RH tap; fret cap 22; no-unison adjacency pass incl. the dim7-symmetry seam). `tapArp` plumbed like `sweepStrings` (hidden field + readConfig + anti-leak default). | ||
| - **Rungs:** guitar 5 (`tap_articulation`→`tap_cell`→`tap_cascade`→`tap_sevenths`→`tap_changes`, descending tempo ceilings, `tap_cascade` density-ramps triplet→sixteenth_triplet); bass 4 (`btap_articulation`→`btap_arpeggio`→`btap_groove`→`btap_apply`, register-clamped frets 5–12). Both in the one `concept_tapping` band (bass rungs instrument-hidden). Edges + FORM_CUE + `screen.html` options wired. | ||
| - **Verified:** `node --check` + `probe-tapping-ladders.mjs` (9/9 rungs: startup guards pass, tp/po/ho present, frets ≤22, no adjacent unison, bass on bass strings) + smoke-generators 134/134, core-purity 40/40, strings, renderers 8/8. Screenshots confirm the `17ᵀ→5ᵖ→9ʰ` cell renders in Tab. **No HOST CHECK needed — generation-path (core) work, our USP, not shell.** | ||
| - **DEFERRED (v2, engine gaps flagged by the panel):** bass two-hand independence (needs a two-voice engine), bass open-string cascades, guitar tapped-pedal-point color rung. Not gated the ladder on these. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the grammar in the deferred-work note.
Change “Not gated the ladder on these” to “The ladder was not gated on these.”
🤖 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 `@ROADMAP.md` at line 33, Update the deferred-work note in ROADMAP.md by
replacing “Not gated the ladder on these” with “The ladder was not gated on
these.”
Source: Linters/SAST tools
The results modal already NAMES a run's dominant fault (the miss-heatmap + lean-strip heroes); this adds the FIX — one prescriptive "Practice next" button that loads a real Ladder rung for that fault. Display-only, downstream-only: reads the verdicts already produced (heat.fgMiss/transMiss, leanMs, nearMiss, the bass felt verdict) and re-judges nothing. - coachRxFor(): pure fault->drill mapping (finger / string-crossing / timing lean / near-miss), instrument-aware, never the rung just run (sibling fallback). - Suppressed on a clean clear (the climb CTA is the prescription there) and when the mid-run downshift chip already fired. - Validated by guitar-pedagogy + bass-pedagogy + learning-design: crossing -> pick_alternate (not string-skip); bass timing -> bass_rh_pulse (above the sub-70 Hz detector floor); pinky copy matches the legato drill it routes to. - smoke-coach-rx.mjs unit-tests the pure mapping via the __virtuosoCoach hook — immune to the "smoke mocks the verifier" blind spot. gap-roadmap PR 1.3. No HOST CHECK: extends already-shipped shell display code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
A bass feltGate run showed only the verdict WORD (Dragging/Settling/Locked), which conflates three distinct pocket faults the felt engine already measures and that have DIFFERENT fixes. buildPocketDiagnosis names the dominant one in a sentence, bucketed the same way feltHoldAnalyze picks the verdict (drift -> lean -> jitter): - drift = a tempo trend across the phrase (slowing / speeding) — the fix splits by direction (slowing: keep the subdivision alive; speeding: don't chase it). - steady lean = a consistent offset (lean in earlier / relax and let it come). - jitter = loose-but-there (keep working it, no rush). locked = green win. Descriptive + a light feel cue; never a score. Bass-pedagogy validated 2026-07-14 (the direction-split drift fix was its one must-fix). Rendered as a .virtuoso-results-pocket line under the verdict word; smoke-coach-rx.mjs guards the mapping via the __virtuosoCoach hook. gap-roadmap PR 1.5. No HOST CHECK: extends already-shipped shell display. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
The timing prescription said "You were behind the click" off the run MEDIAN — diffuse. worstLeanBars() reads the per-bar lean strip the scorer already snapshots and names WHERE it broke down when the lean localizes to a region: "You dragged most in bars 5-6 — lock your pulse to the beat." A whole-run lean (a region spanning >=75% of the bars) stays generic, and a run with no clearly hot bars keeps the median framing. Pure selection over already-blessed data (ptLeanBarsSnapshot); no re-judging. gap-roadmap PR 1.4. Guarded by smoke-coach-rx.mjs (localized + global-lean cases). No HOST CHECK: extends already-shipped shell display. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
…1.20) Re-diagnosing from scratch every run could flip the prescribed focus finger->timing->crossing so the player never stays on one fix long enough to land it (learning-design 2026-07-14 flagged this as load-bearing for the gating). coachRxFor is refactored into coachRxCandidates (the full ranked, category-tagged fault list) + a thin top-of-list coachRxFor; applyCoachHysteresis keeps reinforcing last run's focus while it still trips a threshold — up to 2 consecutive runs — before a new top fault takes over. Focus persists per spec (mode + rung) in localStorage; the hysteresis decision is pure and smoke-tested. Behavior of the single prescription is unchanged when there's no history. gap-roadmap PR 1.3 refinement. Guarded by smoke-coach-rx.mjs. No HOST CHECK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
The first slice of Ear Mode (docs/ear-mode.md): a "Hear it" transport button that plays the current exercise's NOTES as audio, UNGRADED — no scorer, no session, no RAF clock. It schedules the notes-only voice through the same audio bus playback uses (schedulePreviewAudio standalone; the note helpers lazily build the bus, so it's self-contained), so the player can HEAR the phrase — the "call" a future Echo rung will have them reproduce. Notes only (no metronome/backing); an A-B loop bounds the phrase; a graded Play supersedes it; a second click stops. The enabler for 2.2/2.3 — BUILD-on-ours per the cleared HOST CHECK (docs/ear-mode.md): nothing host-borrowable, our own contained engine, zero detector dependency. Verified by shot-listen.mjs (toggles + schedules without throwing + Play supersedes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
main advanced to 0.2.0 via Byron's PR #11 (Gold improv rung — career passports, harmonic-comb-verified jam conformity) merged directly to main, while virtuoso-dev accumulated the grading-display fixes (0.1.10) + tapping ladders (0.1.11) on the 0.1.9 base. The two lines never crossed. This merges main into dev so dev is a superset — Byron's Gold rung + our patch work — and resets the version line to 0.2.1 (a patch above main's 0.2.0 minor), so the eventual dev→main promotion is monotonic and drops nothing. Only the two version tokens (plugin.json + VIRTUOSO_VERSION) conflicted; screen.html / ROADMAP / package.json (playwright bump) + smoke-gold.mjs auto-merged.
ChrisBeWithYou
changed the title
The host now serves the tuning catalog as exact integer midis alongside the reference-scaled frequencies (feedBack#829, merged). hostTuningEntries prefers those - no client-side log2 reconstruction, no rounding risk at non-440 reference pitches. The freq->midi fallback stays for hosts predating #829. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
This was referenced Jul 14, 2026
ChrisBeWithYou
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ROADMAP.md (1)
54-59: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReconcile the release state with this promotion.
These entries still say
mainis 0.1.9 and that PR#4will move it to 0.1.11, while this PR objective says the Gold rung is already on main at 0.2.0 and this promotion produces 0.2.1. Update the authoritative roadmap so future release and beta-cut steps do not use obsolete versions or merge status.Also applies to: 66-72
🤖 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 `@ROADMAP.md` around lines 54 - 59, The release-state roadmap entries around the tapping-ladders status and the related lines 66–72 use obsolete versions and merge assumptions. Update the authoritative roadmap to state that the Gold rung is already on main at 0.2.0, this promotion produces 0.2.1, and revise the PR/merge and beta-cut instructions accordingly so they no longer reference the superseded 0.1.9–0.1.11 sequence.
🧹 Nitpick comments (7)
docs/ear-mode.md (1)
51-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecify the fenced-block language.
Use
```textfor this ASCII layout so the documentation passes MD040.🤖 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/ear-mode.md` around lines 51 - 53, Update the fenced ASCII layout block in the documentation to declare the text language explicitly with the `text` fence marker, preserving the layout content unchanged.Source: Linters/SAST tools
.claude/skills/run-virtuoso/smoke-host-surface.mjs (1)
82-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated instrument-seed boilerplate.
Same block as driver.mjs and 4 other smoke files; see the consolidated comment.
🤖 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 @.claude/skills/run-virtuoso/smoke-host-surface.mjs around lines 82 - 88, Remove the duplicated inline instrument-store seeding from the smoke-host-surface setup and reuse the shared helper or centralized initialization used by driver.mjs and the other smoke files. Preserve the existing seeded guitar_6_standard behavior before page scripts execute..claude/skills/run-virtuoso/smoke-level-gate-async.mjs (1)
72-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated instrument-seed boilerplate.
Same block as driver.mjs and 4 other smoke files; see the consolidated comment.
🤖 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 @.claude/skills/run-virtuoso/smoke-level-gate-async.mjs around lines 72 - 78, Remove the duplicated localStorage instrument-seeding addInitScript from the smoke-level gate flow and reuse the shared helper or centralized setup used by driver.mjs and the other smoke files. Preserve the deterministic guitar_6_standard initialization before page scripts run..claude/skills/run-virtuoso/smoke-gems.mjs (1)
37-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated instrument-seed boilerplate.
Same block as driver.mjs and 4 other smoke files; see the consolidated comment.
🤖 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 @.claude/skills/run-virtuoso/smoke-gems.mjs around lines 37 - 43, Extract the repeated localStorage instrument-seeding init script from the smoke test into the shared helper used by driver.mjs and the other smoke files, then call that helper here before page scripts run. Preserve the existing guitar_6_standard seed and error-tolerant behavior while removing the duplicated boilerplate from this file..claude/skills/run-virtuoso/smoke-audioctx.mjs (1)
46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated instrument-seed boilerplate.
Identical to the block in driver.mjs and 4 other smoke files; see the consolidated comment for a shared-helper suggestion.
🤖 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 @.claude/skills/run-virtuoso/smoke-audioctx.mjs around lines 46 - 52, Replace the inline localStorage seeding block before page scripts run in the smoke-audioctx setup with the shared instrument-seeding helper used by driver.mjs and the other smoke files. Preserve the existing timing and deterministic guitar_6_standard initialization while removing the duplicated addInitScript boilerplate..claude/skills/run-virtuoso/smoke-backing-engine.mjs (1)
49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated instrument-seed boilerplate.
Same block as driver.mjs and 4 other smoke files; see the consolidated comment.
Also applies to: 67-67
🤖 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 @.claude/skills/run-virtuoso/smoke-backing-engine.mjs around lines 49 - 55, Remove the duplicated localStorage instrument-seeding block from the smoke test and reuse the shared setup introduced for driver.mjs and the other smoke files. Preserve the deterministic guitar_6_standard initialization by invoking the existing centralized helper at the same initialization point before page scripts run..claude/skills/run-virtuoso/smoke-contained-verifier.mjs (1)
85-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the instrument-seeding
addInitScriptinto a shared helper.The exact same 7-line block (identical comment text and identical
page.addInitScript(() => { try { localStorage.setItem("virtuoso.instrument", ...) } catch (_) {} })call) is pasted into 7 separate smoke files. If the seeded JSON shape (or the rationale/host-sync version referenced in the comment) ever needs to change, every copy has to be updated in lockstep or the suites silently drift out of sync.
.claude/skills/run-virtuoso/smoke-contained-verifier.mjs#L85-L91: replace the inline block with a call to a sharedseedInstrumentStore(page)helper..claude/skills/run-virtuoso/smoke-core-purity.mjs#L112-L118: same replacement..claude/skills/run-virtuoso/smoke-generators.mjs#L107-L113: same replacement..claude/skills/run-virtuoso/smoke-herta.mjs#L19-L25: same replacement..claude/skills/run-virtuoso/smoke-highway-settings.mjs#L41-L47: same replacement..claude/skills/run-virtuoso/smoke-strings.mjs#L28-L34: same replacement..claude/skills/run-virtuoso/smoke-variation.mjs#L182-L188: same replacement.♻️ Proposed shared helper
// e.g. .claude/skills/run-virtuoso/smoke-shared.mjs export async function seedInstrumentStore(page) { // Seed the L1 instrument store BEFORE page scripts run: the host-settings // sync (v0.1.11) treats an empty localStorage as a fresh install and ADOPTS // host config — which persists whatever instrument the PREVIOUS suite's panel // drives wrote through (cross-suite contamination; the panel flipped to bass // mid-suite). With the store seeded, the local-wins boot path holds the // deterministic 6-string default AND heals the host config for later suites. await page.addInitScript(() => { try { localStorage.setItem("virtuoso.instrument", JSON.stringify({ stringSetup: "guitar_6_standard", customOpenMidis: "" })); } catch (_) {} }); }Then each of the 7 call sites becomes:
- // Seed the L1 instrument store BEFORE page scripts run: ... - await page.addInitScript(() => { try { localStorage.setItem("virtuoso.instrument", JSON.stringify({ stringSetup: "guitar_6_standard", customOpenMidis: "" })); } catch (_) {} }); + await seedInstrumentStore(page);🤖 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 @.claude/skills/run-virtuoso/smoke-contained-verifier.mjs around lines 85 - 91, Extract the duplicated instrument-seeding addInitScript into a shared seedInstrumentStore(page) helper, preserving its existing JSON payload and initialization behavior. In .claude/skills/run-virtuoso/smoke-contained-verifier.mjs lines 85-91, .claude/skills/run-virtuoso/smoke-core-purity.mjs lines 112-118, .claude/skills/run-virtuoso/smoke-generators.mjs lines 107-113, .claude/skills/run-virtuoso/smoke-herta.mjs lines 19-25, .claude/skills/run-virtuoso/smoke-highway-settings.mjs lines 41-47, .claude/skills/run-virtuoso/smoke-strings.mjs lines 28-34, and .claude/skills/run-virtuoso/smoke-variation.mjs lines 182-188, replace each inline block with seedInstrumentStore(page) and import the shared helper.
🤖 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 @.claude/skills/run-virtuoso/smoke-highway-settings.mjs:
- Around line 56-66: Update the setView helper in smoke-highway-settings.mjs to
validate that `#virtuoso-view-select` exists and contains the requested option
before changing its value; throw a clear error such as “View option not found”
when either check fails. Preserve the existing change-event dispatch and
settling behavior for valid highway_3d selections, including those used in the
cycling loop.
In `@docs/topbar-host-parity.md`:
- Around line 263-267: Update the “Failure modes” section in
docs/topbar-host-parity.md to match the supplied screen.js behavior by removing
the promise of a one-shot console.warn, while preserving the documented fallback
behavior and silent write-through skipping.
- Around line 164-170: Update the tuner frame event documentation in the display
section to specify the actual payload shape as {note, freq, cents, hasSignal},
replacing hz and including hasSignal. Keep the surrounding rendering and
idle-state guidance unchanged.
- Around line 3-11: Update docs/topbar-host-parity.md to reflect the existing
host integrations in screen.js, removing the outdated “no code changes yet”
status and zero-reference claim. Refresh the host-check evidence and affected
sections around the status and implementation notes to match the current
implementation and verification state.
In `@ROADMAP.md`:
- Around line 57-59: Update the ROADMAP verification note to distinguish tapping
generation verification from grading-display verification: retain “No HOST CHECK
needed” only for the tapping path, and explicitly state that grading fixes still
require installed-desktop verification with the real detector before release.
Keep the existing note that smoke tests mock the contained verifier and grading
verification remains outstanding.
---
Outside diff comments:
In `@ROADMAP.md`:
- Around line 54-59: The release-state roadmap entries around the
tapping-ladders status and the related lines 66–72 use obsolete versions and
merge assumptions. Update the authoritative roadmap to state that the Gold rung
is already on main at 0.2.0, this promotion produces 0.2.1, and revise the
PR/merge and beta-cut instructions accordingly so they no longer reference the
superseded 0.1.9–0.1.11 sequence.
---
Nitpick comments:
In @.claude/skills/run-virtuoso/smoke-audioctx.mjs:
- Around line 46-52: Replace the inline localStorage seeding block before page
scripts run in the smoke-audioctx setup with the shared instrument-seeding
helper used by driver.mjs and the other smoke files. Preserve the existing
timing and deterministic guitar_6_standard initialization while removing the
duplicated addInitScript boilerplate.
In @.claude/skills/run-virtuoso/smoke-backing-engine.mjs:
- Around line 49-55: Remove the duplicated localStorage instrument-seeding block
from the smoke test and reuse the shared setup introduced for driver.mjs and the
other smoke files. Preserve the deterministic guitar_6_standard initialization
by invoking the existing centralized helper at the same initialization point
before page scripts run.
In @.claude/skills/run-virtuoso/smoke-contained-verifier.mjs:
- Around line 85-91: Extract the duplicated instrument-seeding addInitScript
into a shared seedInstrumentStore(page) helper, preserving its existing JSON
payload and initialization behavior. In
.claude/skills/run-virtuoso/smoke-contained-verifier.mjs lines 85-91,
.claude/skills/run-virtuoso/smoke-core-purity.mjs lines 112-118,
.claude/skills/run-virtuoso/smoke-generators.mjs lines 107-113,
.claude/skills/run-virtuoso/smoke-herta.mjs lines 19-25,
.claude/skills/run-virtuoso/smoke-highway-settings.mjs lines 41-47,
.claude/skills/run-virtuoso/smoke-strings.mjs lines 28-34, and
.claude/skills/run-virtuoso/smoke-variation.mjs lines 182-188, replace each
inline block with seedInstrumentStore(page) and import the shared helper.
In @.claude/skills/run-virtuoso/smoke-gems.mjs:
- Around line 37-43: Extract the repeated localStorage instrument-seeding init
script from the smoke test into the shared helper used by driver.mjs and the
other smoke files, then call that helper here before page scripts run. Preserve
the existing guitar_6_standard seed and error-tolerant behavior while removing
the duplicated boilerplate from this file.
In @.claude/skills/run-virtuoso/smoke-host-surface.mjs:
- Around line 82-88: Remove the duplicated inline instrument-store seeding from
the smoke-host-surface setup and reuse the shared helper or centralized
initialization used by driver.mjs and the other smoke files. Preserve the
existing seeded guitar_6_standard behavior before page scripts execute.
In @.claude/skills/run-virtuoso/smoke-level-gate-async.mjs:
- Around line 72-78: Remove the duplicated localStorage instrument-seeding
addInitScript from the smoke-level gate flow and reuse the shared helper or
centralized setup used by driver.mjs and the other smoke files. Preserve the
deterministic guitar_6_standard initialization before page scripts run.
In `@docs/ear-mode.md`:
- Around line 51-53: Update the fenced ASCII layout block in the documentation
to declare the text language explicitly with the `text` fence marker, preserving
the layout content unchanged.
🪄 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: 679ab708-62ef-4ddb-8b10-d2463763d078
📒 Files selected for processing (28)
.claude/skills/run-virtuoso/driver.mjs.claude/skills/run-virtuoso/package.json.claude/skills/run-virtuoso/smoke-audioctx.mjs.claude/skills/run-virtuoso/smoke-backing-engine.mjs.claude/skills/run-virtuoso/smoke-coach-rx.mjs.claude/skills/run-virtuoso/smoke-connect.mjs.claude/skills/run-virtuoso/smoke-contained-verifier.mjs.claude/skills/run-virtuoso/smoke-core-purity.mjs.claude/skills/run-virtuoso/smoke-gems.mjs.claude/skills/run-virtuoso/smoke-generators.mjs.claude/skills/run-virtuoso/smoke-herta.mjs.claude/skills/run-virtuoso/smoke-highway-settings.mjs.claude/skills/run-virtuoso/smoke-host-surface.mjs.claude/skills/run-virtuoso/smoke-level-gate-async.mjs.claude/skills/run-virtuoso/smoke-meter-subdiv.mjs.claude/skills/run-virtuoso/smoke-over-barline.mjs.claude/skills/run-virtuoso/smoke-progress.mjs.claude/skills/run-virtuoso/smoke-renderers.mjs.claude/skills/run-virtuoso/smoke-scoring-e2e.mjs.claude/skills/run-virtuoso/smoke-session-sync.mjs.claude/skills/run-virtuoso/smoke-strings.mjs.claude/skills/run-virtuoso/smoke-variation.mjsROADMAP.mddocs/ear-mode.mddocs/topbar-host-parity.mdplugin.jsonscreen.htmlscreen.js
Comment on lines
+56
to
+66
| await page.waitForSelector("#virtuoso-view-select", { timeout: 5000 }); | ||
|
|
||
| // Ensure the 3D highway is the active view, then settle. | ||
| const hwBtn = await page.$('.virtuoso-view-btn[data-renderer="highway_3d"]'); | ||
| if (hwBtn) { await hwBtn.click(); await page.waitForTimeout(600); } | ||
| // Ensure the 3D highway is the active view, then settle. (View selector is | ||
| // a dropdown since v0.1.13 — value + change, the user event path.) | ||
| const setView = (k) => page.evaluate((kind) => { | ||
| const sel = document.querySelector("#virtuoso-view-select"); | ||
| if (!sel) return; | ||
| sel.value = kind; | ||
| sel.dispatchEvent(new Event("change", { bubbles: true })); | ||
| }, k); | ||
| await setView("highway_3d"); await page.waitForTimeout(600); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
setView silently no-ops instead of failing when the option is missing.
Unlike switchRenderer in smoke-renderers.mjs, which checks the option exists and throws Renderer option not found if not, this setView just returns when sel (or the matching <option>) is absent. Since the test's entire premise hinges on highway_3d actually being the active view at the baseline snapshot (line 66) and through the cycling loop (line 85-87), a missing/renamed option would silently produce a false PASS instead of a loud failure — masking exactly the kind of regression this suite exists to catch.
🐛 Proposed fix — validate before switching
const setView = (k) => page.evaluate((kind) => {
const sel = document.querySelector("`#virtuoso-view-select`");
- if (!sel) return;
+ if (!sel || ![...sel.options].some((o) => o.value === kind)) return false;
sel.value = kind;
sel.dispatchEvent(new Event("change", { bubbles: true }));
+ return true;
}, k);
- await setView("highway_3d"); await page.waitForTimeout(600);
+ if (!(await setView("highway_3d"))) throw new Error("Renderer option not found: highway_3d");
+ await page.waitForTimeout(600); for (const kind of ["builtin_2d", "highway_2d", "tab_2d", "notation_2d", "highway_3d"]) {
- await setView(kind); await page.waitForTimeout(400);
+ if (!(await setView(kind))) throw new Error(`Renderer option not found: ${kind}`);
+ await page.waitForTimeout(400);
}Also applies to: 85-87
🤖 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 @.claude/skills/run-virtuoso/smoke-highway-settings.mjs around lines 56 - 66,
Update the setView helper in smoke-highway-settings.mjs to validate that
`#virtuoso-view-select` exists and contains the requested option before changing
its value; throw a clear error such as “View option not found” when either check
fails. Preserve the existing change-event dispatch and settling behavior for
valid highway_3d selections, including those used in the cycling loop.
Comment on lines
+3
to
+11
| > **Status: SPEC (2026-07-10, ux lane) — no code changes yet.** | ||
| > Why now: Virtuoso's `"fullscreen": true` opt-in hides the host topbar entirely on our | ||
| > screen, so the three host-standard topbar elements (Open Tuner, Instrument selector, | ||
| > Profile) vanish for the player while inside Virtuoso. Part 1 rule 1: match the host | ||
| > standard — bring them into our header, in the host's order, speaking the host's | ||
| > visual language, without inventing variants. | ||
| > | ||
| > Host recon verified 2026-07-10 against the local FeedBack clone (v0.3.0-alpha.1, | ||
| > `static/v3/shell.js` + `badges.js` + `profile.js`, `server.py`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Refresh the implementation status and host-check evidence.
This document still says “no code changes yet” and claims screen.js has zero host-integration references, but the supplied implementation already contains these integrations in screen.js, Lines 18495–18760. Leaving this stale snapshot in the release docs will mislead contributors and verification work.
Also applies to: 29-31
🧰 Tools
🪛 LanguageTool
[grammar] ~4-~4: Ensure spelling is correct
Context: ...ullscreen": true` opt-in hides the host topbar entirely on our > screen, so the three ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~5-~5: Ensure spelling is correct
Context: ...ur > screen, so the three host-standard topbar elements (Open Tuner, Instrument select...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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/topbar-host-parity.md` around lines 3 - 11, Update
docs/topbar-host-parity.md to reflect the existing host integrations in
screen.js, removing the outdated “no code changes yet” status and zero-reference
claim. Refresh the host-check evidence and affected sections around the status
and implementation notes to match the current implementation and verification
state.
Comment on lines
+164
to
+170
| - **Click = `window.tuner.toggle()`**, feature-detected exactly like the host badge; if | ||
| `window.tuner` is absent the badge does not render (no dead control — honesty rule). | ||
| - **Display** (compact mirror of the host card): a small vertical/segment meter glyph + | ||
| the live note name. Subscribe `hostBus().on('tuner:frame', cb)` → `{note, hz, cents}`; | ||
| when no frames are flowing (tuner closed), show the resting glyph + "Tuner" label — | ||
| matching the host card's idle state, at our scale. No hz sub-label at 32px (fold it | ||
| into the `title` tooltip: "Open Tuner — 440hz"). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the actual tuner event shape.
The current consumer handles note, cents, and hasSignal, while the supplied host contract exposes frequency as freq, not hz. Update this section to use {note, freq, cents, hasSignal} so future integrations do not read a nonexistent field.
🤖 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/topbar-host-parity.md` around lines 164 - 170, Update the tuner frame
event documentation in the display section to specify the actual payload shape
as {note, freq, cents, hasSignal}, replacing hz and including hasSignal. Keep
the surrounding rendering and idle-state guidance unchanged.
Comment on lines
+263
to
+267
| **Failure modes:** | ||
| - `/api/settings` or `/api/tunings` unreachable (older host, headless smoke oddity, | ||
| network) → panel behaves exactly as today: `STRING_SETUPS` + `TUNING_PRESETS` + | ||
| Virtuoso's own persistence. Write-throughs are skipped silently; a one-shot | ||
| console.warn, never a user-facing error. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align failure-mode documentation with the implementation.
The supplied screen.js paths catch /api/settings and /api/tunings failures silently, but this section promises a one-shot console.warn. Either implement deduplicated warning telemetry or remove that promise from the documentation.
🤖 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/topbar-host-parity.md` around lines 263 - 267, Update the “Failure
modes” section in docs/topbar-host-parity.md to match the supplied screen.js
behavior by removing the promise of a one-shot console.warn, while preserving
the documented fallback behavior and silent write-through skipping.
Comment on lines
+57
to
+59
| - **Verified:** `node --check` + `probe-tapping-ladders.mjs` (9/9 rungs: startup guards pass, tp/po/ho present, frets ≤22, no adjacent unison, bass on bass strings) + smoke-generators 134/134, core-purity 40/40, strings, renderers 8/8. Screenshots confirm the `17ᵀ→5ᵖ→9ʰ` cell renders in Tab. **No HOST CHECK needed — generation-path (core) work, our USP, not shell.** | ||
| - **DEFERRED (v2, engine gaps flagged by the panel):** bass two-hand independence (needs a two-voice engine), bass open-string cascades, guitar tapped-pedal-point color rung. Not gated the ladder on these. | ||
| - **NEXT:** review/merge **PR #4** (dev→main — now the combined grading+tapping PR, https://github.com/got-feedBack/feedBack-plugin-virtuoso/pull/4); on merge `main` goes 0.1.9→0.1.11 (skips a standalone 0.1.10 tag — both land together). Then re-cut beta (`node scripts/cut-beta.mjs --push`). Smoke still MOCKS the contained verifier, so the grading fixes remain real-desktop-DI-dogfood-verified only (standing owe). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Separate tapping verification from grading verification.
“No HOST CHECK needed” is valid only for the tapping generation path. This combined PR also ships grading-display fixes, whose installed-desktop/real-detector verification is a release requirement; the current note says that verification is still owed.
🧰 Tools
🪛 LanguageTool
[grammar] ~59-~59: Ensure spelling is correct
Context: ...g. Not gated the ladder on these. - NEXT: review/merge PR #4 (dev→main — no...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@ROADMAP.md` around lines 57 - 59, Update the ROADMAP verification note to
distinguish tapping generation verification from grading-display verification:
retain “No HOST CHECK needed” only for the tapping path, and explicitly state
that grading fixes still require installed-desktop verification with the real
detector before release. Keep the existing note that smoke tests mock the
contained verifier and grading verification remains outstanding.
This was referenced Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promote
virtuoso-dev(0.2.2) →main(0.2.0)The full 2026-07-14 batch.
virtuoso-devreconciled withmain(absorbed Byron's Gold-rung #11), then landed every open feature branch and re-versioned the line to 0.2.2 (patch above main's 0.2.0). Monotonic; nothing dropped.What ships to main (beyond the Gold rung already there)
docs/ear-mode.md).tuningMidisread (host #829), string-count-aware generation (+ the real 6-string-bass pitch-bug fix), view dropdown +highway_2dslot, interval-aware drop-tuning power-chord grip, bass low-fifth reach, and the Classic 2D Highway renderer.concept_tapping, guitar + bass) + grading-display fixes (copy-card accuracy + 3D hit-flare restore).How it was assembled
main → dev reconcile (
161801c, → 0.2.1); topbar stack merge (c022065); coach merge (8cc4076); ear-mode merge (4f9bf8e, → 0.2.2); cherry-pick of #5's latetuningMidiscommit (f0c78c6). Every merge conflicted only on the two version tokens — no code overlap between the features and Byron's Gold rung. Superseded PRs: #5–#10 closed (content landed here), #12 / #13 auto-merged.Verify
backing-engine,progress,variation) are the documented pre-existing base failures;smoke-gold(from Gold improv rung — harmonic-comb verified jam conformity (career passports) #11),smoke-coach-rx, and the stack's suites all pass.node --checkclean; version tokens consistent (plugin.json+VIRTUOSO_VERSION=0.2.2).🤖 Generated with Claude Code