feat(editor): Suggest fret-hand fingers (auto-fingering) - #237
Merged
Conversation
The gap audit's flagship finding: every piece of fret_finger plumbing already existed — the teaching mark, its Guitar-Pro/XML round-trip, the fretboard strip that DISPLAYS fingers — but nothing ever PROPOSED a finger. It was manual-only. Note ▸ Suggest fret-hand fingers now assigns a fingering to every fretted note from its fret relative to the hand anchor covering its time: index (1) at the anchor fret, one finger per fret across the four-fret span, clamped 1..4; open strings → none; notes outside a reachable hand position → left untouched (a different anchor owns them). Fingers the selection when there is one, else the whole track, in ONE undoable step. - Pures in src/position.js: `_suggestFingerForFretPure` (fret+anchor → finger, reusing the existing `_activeAnchorAtPure` window model) and `_suggestFingersPure`. - New `SetTeachingMarksCmd` (src/commands.js): per-note teaching-mark assignment in one undo step — the plural of the existing single-value SetTeachingMarkCmd. - Verb `editorSuggestFingers` (src/anchor-resolve.js, beside editorResolveAnchorWindow); registry entry + Note-menu item; dispatched through `_editorRunEofCommand`. `tests/auto_fingering.test.mjs` (6: the finger pure incl. open/refuse/clamp, the map-and-omit, and the SetTeachingMarksCmd per-note exec→undo→redo). 129 JS suites green (menu_model incl.), lint 0-err (3 pre-existing warnings). routes.py untouched. Verified live on AC/DC — Back In Black: Note ▸ Suggest fret-hand fingers → "Suggested fret-hand fingers for 1328 notes in the arrangement", one undoable command, no errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
Warning Review limit reached
Next review available in: 11 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 (2)
📝 WalkthroughWalkthroughChangesFret-hand finger suggestions
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant AnchorResolver
participant PositionRules
participant TeachingMarkCommand
Editor->>AnchorResolver: invoke editorSuggestFingers
AnchorResolver->>AnchorResolver: resolve active anchors
AnchorResolver->>PositionRules: calculate finger assignments
AnchorResolver->>TeachingMarkCommand: apply fret_finger values
TeachingMarkCommand-->>Editor: update notes and history
Editor-->>Editor: redraw and show status
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.
🧹 Nitpick comments (1)
src/anchor-resolve.js (1)
259-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider integration-level test coverage for
editorSuggestFingers.Current tests cover
_suggestFingerForFretPure,_suggestFingersPure, andSetTeachingMarksCmdin isolation, but not this orchestration function itself (selection branching, anchor resolution wiring, status text). A seeded-state test similar to the existingSetTeachingMarksCmdround-trip test (usingtests/_history_env.mjs) would catch regressions in how these pieces are wired together.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/anchor-resolve.js` around lines 259 - 287, Add an integration-level test for editorSuggestFingers using the seeded-state setup from tests/_history_env.mjs. Exercise both selected-note and whole-arrangement paths, including anchor resolution wiring, SetTeachingMarksCmd history updates, and the resulting status text, while preserving the existing isolated tests for _suggestFingerForFretPure, _suggestFingersPure, and SetTeachingMarksCmd.
🤖 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.
Nitpick comments:
In `@src/anchor-resolve.js`:
- Around line 259-287: Add an integration-level test for editorSuggestFingers
using the seeded-state setup from tests/_history_env.mjs. Exercise both
selected-note and whole-arrangement paths, including anchor resolution wiring,
SetTeachingMarksCmd history updates, and the resulting status text, while
preserving the existing isolated tests for _suggestFingerForFretPure,
_suggestFingersPure, and SetTeachingMarksCmd.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8334929-72bd-423f-a04d-6fe6f2c79cdb
📒 Files selected for processing (8)
CHANGELOG.mdsrc/anchor-resolve.jssrc/commands.jssrc/input.jssrc/menu-bar.jssrc/position.jssrc/shortcuts.jstests/auto_fingering.test.mjs
# Conflicts: # CHANGELOG.md # src/input.js
byrongamatos
added a commit
that referenced
this pull request
Jul 13, 2026
* feat(editor): lint finger conflicts in a grip Auto-fingering (#237) and the coherent-grip chord resolve (#245) assign fret-hand fingers, but a grip can still ask ONE finger to hold two different frets at the same instant — a shape no hand can play. Add a finger-conflict rule to the advisory playability lint. _lintFingerConflictPure (playability-lint.js): within a simultaneous cluster (the shared LINT_CLUSTER_EPSILON), group fretted notes by their fret_finger (1-4 only; open strings and the -1 unset sentinel carry no fretting finger), and flag any finger used on more than one distinct fret. A BARRE — the same finger across strings at the SAME fret — is one fret, so it never flags. Wired into _playabilityLintPure and the RULE_LABELS map, so it rides the existing surface: a yellow underline on the flagged notes, the count chip, and a popover row that seeks + selects. Advisory only — never blocks, never auto-fixes (the charter answers the physical question), like every rule. Tests: tests/finger_conflict_lint.test.mjs (10) — conflict vs barre vs different-fingers vs not-simultaneous, open/unset/out-of-range fingers ignored, barre-plus-stray, two conflicts in one cluster, aggregation into the full pass, junk-input degradation. Live-verified in-app: the conflict is named in the real _lintResults memo and both notes land in the flagged set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q * fix(editor): count the thumb as a fretting digit in the finger-conflict lint The rule guarded on `fret_finger` 1-4, so it never saw the THUMB. Per spec §6.2.2 (and routes.py's `_fret_finger_attr`) the fret-hand digit range is 0-4, where 0 is the thumb — the fretboard strip's `T`, cycled by right-click and clamped to -1..4 by the inspector. A thumb-over grip frets the low E, so a thumb assigned to two different frets at one instant is exactly as impossible as finger 1 in two places, and the lint went blind to it. Widen the guard to 0-4 and render the thumb as `T` in the popover detail (the strip's FINGER_LABELS encoding, inlined rather than importing that UI module into the @pure block). A thumb barre across E+A at one fret stays legal, as does a normal thumb-over grip — same physics as the other digits. Also drop the redundant `list.length >= 2` (implied by `frets.length >= 2`). --------- Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
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
2 participants
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.
The gap audit's flagship finding: every piece of
fret_fingerplumbing already existed — the teaching mark, its Guitar-Pro/XML round-trip, the fretboard strip that displays fingers — but nothing ever proposed a finger. It was manual-only. This fills that gap.What it does
Note ▸ Suggest fret-hand fingers assigns a fingering to every fretted note from its fret relative to the hand anchor covering its time:
It fingers the selection when you have one, otherwise the whole track — in one undoable step.
Implementation
src/position.js:_suggestFingerForFretPure(fret + anchor → finger, reusing the existing_activeAnchorAtPurewindow model) and_suggestFingersPure.SetTeachingMarksCmd(src/commands.js): per-note teaching-mark assignment as one undo step — the plural of the existing single-valueSetTeachingMarkCmd.editorSuggestFingers(src/anchor-resolve.js, besideeditorResolveAnchorWindow); a registry entry + Note-menu item, dispatched through the shared_editorRunEofCommand.Tests / gates
tests/auto_fingering.test.mjs(6: the finger pure incl. open/refuse/clamp, the map-and-omit, and theSetTeachingMarksCmdper-noteexec → undo → redo). 129 JS suites green (menu_model included — the registry/menu wiring is consistent), lint 0 errors (3 pre-existing warnings).routes.pyuntouched.Verified live
On AC/DC — Back In Black: Note ▸ Suggest fret-hand fingers → "Suggested fret-hand fingers for 1328 notes in the arrangement", one undoable command, no page errors. The assigned fingers display on the fretboard strip (Fret toggle).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests