feat(editor): fretboard companion strip (view-modality P7 / VA.6) - #198
Conversation
A docked, toggleable mini-fretboard at the bottom of the timeline for the active FRETTED part (design V13.2, the Christian-endorsed mechanism): tuning / string count / capo drawn from the arrangement, the capo as a visual nut-bar (chart frets index right of it; sounding-open labels are tuning+capo aware). Selecting notes lights every playable same-pitch position from the P6 resolver's enumeration (_enumerateFrettedPositionsPure), annotated: bright inside the active anchor window (_activeAnchorAtPure over _rollAnchorList), dim outside, open-string candidates drawn SQUARE at the effective nut, stretch-cost digits (fret travel vs _prevNoteBefore's hand position). The note's own position renders filled and carries its fret_finger mark. - Click a candidate -> assign through the SAME command path as P5 cycling: a single-move MoveToStringCmd with pitchPreserving=true, so it passes the read-only-roll lock and CONFIRMS a suggested note (the command already clears the mark on exec and re-marks on rollback — pinned by a real-EditHistory round-trip test). - Right-click a current position -> cycle the fret_finger teaching mark (none -> 1..4 -> T -> none) via SetTeachingMarkCmd. - Render/hit-test share ONE geometry pure — including the open-candidate placement right of the capo bar (the first cut had the renderer moved and the hit-test stale; a test now pins them together). The render idiom (xLine/xNote/rowY, windowed frets, capped spacing, inlay dots, hollow-vs-filled dots) is ported from Virtuoso's Live fretboard strip (screen.js $13) with Christian's blessing — read for the port, never modified. The editor owns all candidate/annotation logic. Perf: a canvas SIDECAR, never in the main draw loop — repaints only when a memo key (editGen + selection + arr + size) changes, from the updateStatus refresh and its own interactions. Hidden on keys parts, drum mode, the parts overview and Tempo Map mode; 'Fret' toolbar toggle persists as an editor pref (never the pack). The one document/window-level listener (resize re-key) rides the teardown registry. Deferred to a follow-up (noted per prompt): chord-selection handshape- template lighting — the anchor-window home-box ships now. tests/fretboard_strip.test.mjs (11): annotation math (enumeration x window x stretch x capo), display window, geometry/hit-test sharing, finger cycle, and the two command round-trips through the real EditHistory. Suite 91/91, ESLint 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a toggleable fretboard companion strip for selected fretted notes, including candidate rendering, pitch-preserving position assignment, finger-mark cycling, editor lifecycle wiring, and Node-based tests. ChangesFretboard companion strip
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as Editor selection
participant Strip as fretboard strip
participant Canvas as fretboard canvas
participant History as EditHistory
Editor->>Strip: update selection and context
Strip->>Canvas: render candidate positions
Canvas->>Strip: send click or contextmenu
Strip->>History: execute MoveToStringCmd or SetTeachingMarkCmd
History->>Editor: update arrangement
Editor->>Strip: trigger refresh
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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)
tests/fretboard_strip.test.mjs (1)
30-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider Node's built-in test runner instead of a hand-rolled harness.
The custom
t()/pass/fail counter duplicates whatnode:test(already available givennode:assertis used) provides out of the box — structured output,describe/itnesting, and TAP-compatible reporting for CI.🤖 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/fretboard_strip.test.mjs` around lines 30 - 34, Replace the hand-rolled t() function and pass/fail counters in tests/fretboard_strip.test.mjs with Node’s built-in node:test runner, using describe/it or test and the existing node:assert assertions; remove manual console logging and summary handling so the runner produces structured TAP-compatible results.
🤖 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 `@src/fretboard-strip.js`:
- Around line 296-303: The current open-string note is drawn at the wrong
horizontal position when a capo is active. In the current-note rendering branch,
use the same shared open-note geometry as hit-testing—placing fret-0 notes at
the capo bar position plus the open-note offset (the equivalent of xLine(capo) +
9), while retaining the existing position for fretted notes.
---
Nitpick comments:
In `@tests/fretboard_strip.test.mjs`:
- Around line 30-34: Replace the hand-rolled t() function and pass/fail counters
in tests/fretboard_strip.test.mjs with Node’s built-in node:test runner, using
describe/it or test and the existing node:assert assertions; remove manual
console logging and summary handling so the runner produces structured
TAP-compatible results.
🪄 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: fa32aabb-0785-4214-b0aa-329efb7ba9c0
📒 Files selected for processing (5)
CHANGELOG.mdscreen.htmlsrc/fretboard-strip.jssrc/main.jstests/fretboard_strip.test.mjs
…d hit-test CodeRabbit: a selected (current) open-string note under a capo drew at xNote(capo) while the hit-test expected the effective-nut placement — right-click finger cycling missed the drawn dot. The placement rule now hoists once per annotation (open → just right of the capo bar, for current and candidate alike) and the renderer/hit-test share it; pinned by a current-open-under-capo hit test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
2 participants
What
The V13.2 ★ mechanism: a docked, toggleable mini-fretboard at the bottom of the timeline for the active fretted part. Tuning, string count, and capo come from the arrangement — the capo renders as a visual nut-bar, chart frets index right of it, and the open-string labels are tuning+capo aware.
Selection lights every playable same-pitch position (the P6 resolver's
_enumerateFrettedPositionsPure), annotated per the design: bright inside the active anchor window / dim outside, open-string candidates drawn square at the effective nut, and a stretch-cost digit (fret travel vs the previous note's hand position). The note's own position renders filled and shows its finger mark.MoveToStringCmdpath as P5 cycling — passes the roll's edit lock, and confirms a suggested note (the command clears the mark on exec and re-marks on rollback; pinned by a real-EditHistory round-trip test).fret_finger(1-4 · T · none) throughSetTeachingMarkCmd.Ported idiom
The render/hit-test grammar (xLine/xNote/rowY, windowed frets, capped fret spacing, inlay dots, hollow-vs-filled dots) is ported from Virtuoso's Live fretboard strip with Christian's blessing — read for the port, never modified. The editor owns all candidate/annotation logic (
src/position.jspures).Discipline
Frettoolbar toggle persists as an editor pref, never in the pack.Deferred (follow-ups)
Tests
tests/fretboard_strip.test.mjs(11, real-import): annotation math (enumeration × anchor window × stretch × capo), the display window, geometry/hit-test sharing, the finger cycle, and both command round-trips through the real EditHistory (assign clears + undo re-marks the suggested state). Full suite 91/91 · ESLint 0 errors · CHANGELOG updated.Screenshot shared with Christian during review.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit
New Features
Documentation
Tests