feat(editor): fretted playability lint, advisory (view-modality P9 / VA.8) - #200
Merged
Conversation
…VA.8) A pure lint pass over the active fretted part (design V13.5/V14), the fretted sibling of the drum limb-lint and under the same constitution: ADVISORY ONLY — names the physical question, never blocks, never auto-fixes (guitar has real outliers: big hands, benders, altered technique). Rules (one @pure:playability-lint block, thresholds as named consts with tuning invited from the pedagogy seat): - stretch: simultaneous fretted notes spanning more than the active anchor window (+1 fret tolerance); open strings never count toward the span (open-position idioms are not stretches). - overlap: two notes on ONE string overlapping in time, sustain-aware (one string sounds one note), incl. the same-instant case. - open-bend: a bend authored on an open string (needs a bender). ADAPTED from the prompt's 'open-string pitch vs tuning' check: notes carry {string, fret} only — pitch is always derived — so there is no independent pitch to cross-check; the rule pairs with bad-fret (fret outside 0-24 / non-integer), the data-bug-catcher half. - legato-jump: an HO/PO arrival or a pitched slide's reach whose fret gap exceeds the active anchor window (same-string chains only). Anchors resolve authored-first (anchors_user, falling back to computed anchors — the dual-list precedence the tempo remap uses), through _activeAnchorAtPure; no anchors → a default 4-fret window. Surface: flagged notes wear a 2px amber underline in BOTH String view and the roll (drawNotes hoists one memoized Set — the lint recomputes only when editGen / the arrangement changes, the draw-coalesce dirty path, never per frame); a count chip by the note-count readout opens a popover listing the issues; clicking one seeks + selects. The popover's click-away listener rides the teardown registry. tests/playability_lint.test.mjs (9): a flag/no-flag boundary per rule, anchor-width participation, cross-string and open-string exemptions, sustain-aware overlap, the junk/empty/no-anchors degradations, dual anchor-list precedence, and full-pass ordering. 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 (4)
📝 WalkthroughWalkthroughAdds an advisory fretted playability lint system with four pure rule passes, memoized results, count-chip and popover UI, amber note underlines, editor initialization wiring, and Node-based rule tests. ChangesFretted playability lint
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant playabilityLint
participant ArrangementState
participant drawNotes
participant LintPopover
Editor->>playabilityLint: initPlayabilityLint()
playabilityLint->>ArrangementState: read active arrangement and notes
playabilityLint-->>drawNotes: provide flagged note indices
drawNotes-->>Editor: render amber note underlines
Editor->>LintPopover: editorToggleLintPopover()
LintPopover->>playabilityLint: request issue rows
playabilityLint-->>LintPopover: render issues with seek data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
🤖 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 `@CHANGELOG.md`:
- Line 20: Update the flagged-notes description in CHANGELOG.md to replace
“yellow” with “amber,” matching the UI terminology and PR objective.
- Around line 26-28: Remove internal references such as “the prompt’s” and “the
charter” from the changelog entry, and rewrite the affected sentence as a
direct, user-facing explanation of the open-bend and bad-fret behavior without
mentioning development or validation processes.
In `@screen.html`:
- Around line 151-152: The playability-lint chip and popover lack accessible
disclosure and focus management. Update the chip trigger to reference
`editor-lint-pop` via `aria-controls` and keep `aria-expanded` synchronized with
its open state; add an accessible label to the popover. When opening, focus the
first issue row, and when closing—including Escape handling—return focus to the
chip.
In `@src/playability-lint.js`:
- Around line 106-113: Update the overlap detection loop in the byString
processing to compare each note against all earlier sustaining notes that remain
active, rather than only list[k - 1]. Preserve same-instant detection, report
every sustain-aware conflict, and add a regression test covering one long
sustain followed by at least two later attacks.
- Around line 246-250: renderPopover() binds each row to mutable array index k,
allowing changed lint ordering to select the wrong issue. Assign each issue a
stable identifier derived from its affected note/range, store that key in the
row’s data attribute, and update the click handler to resolve the current lint
result by that stable key before seeking and selecting; apply the same change to
the related handler around the additional referenced lines.
🪄 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: 7a665e73-9205-4ed7-b2a1-d5c7a9216a3b
📒 Files selected for processing (7)
CHANGELOG.mdassets/v3-theme.cssscreen.htmlsrc/draw.jssrc/main.jssrc/playability-lint.jstests/playability_lint.test.mjs
Comment on lines
+151
to
+152
| <!-- Playability-lint popover (P9) — anchored bottom-right of the timeline. --> | ||
| <div id="editor-lint-pop" class="hidden absolute right-3 bottom-2 z-30 editor-lint-pop"></div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add accessible disclosure and focus management.
The chip should expose aria-controls and synchronized aria-expanded, while the popover should have an accessible label. Opening it should focus the first issue row, and closing it should restore focus to the chip (including Escape handling); otherwise keyboard and screen-reader users may not be able to review the issues reliably.
Based on the PR objective that the count chip opens an issue popover for review.
Also applies to: 250-250
🤖 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.html` around lines 151 - 152, The playability-lint chip and popover
lack accessible disclosure and focus management. Update the chip trigger to
reference `editor-lint-pop` via `aria-controls` and keep `aria-expanded`
synchronized with its open state; add an accessible label to the popover. When
opening, focus the first issue row, and when closing—including Escape
handling—return focus to the chip.
- Overlap rule: scan EVERY still-active predecessor on the string, not just the immediate one — a long sustain (t=1..6) overlaps attacks at t=2 AND t=3 and both conflicts are named now. Regression test added. - Popover rows bind to a rendered SNAPSHOT instead of indices into fresh lint results, and an open popover re-renders when the lint changes under it — a mid-review edit can no longer remap clicks. - Accessible disclosure: aria-controls/aria-expanded on the chip, role=dialog + label on the popover, focus lands on the first row on open and returns to the chip on close, Escape closes. - CHANGELOG wording: amber (the shipped color), product language instead of internal planning terms. 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>
This was referenced Jul 12, 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
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.
What
The fretted sibling of the drum limb-lint, same constitution (F5.4): advisory only — a pure lint pass that names the physical question and lets the charter answer it. Never blocks, never auto-fixes.
Rules (one
@pure:playability-lintblock, thresholds as named consts){string, fret}only, pitch is always derived — so the rule ships as this physical check plus bad-fret (outside 0–24 / non-integer), the data-bug-catcher half.Anchors resolve authored-first (
anchors_user→ computedanchors, the dual-list precedence the tempo remap uses); no anchors degrades to a 4-fret default window.Surface
Flagged notes wear a 2px amber underline in both String view and the roll —
drawNoteshoists one memoized Set, recomputed only wheneditGen/the arrangement changes (the draw-coalesce dirty path), never per frame. A count chip by the note-count readout opens a popover listing every issue; clicking a row seeks + selects the notes. Click-away rides the teardown registry. Hidden on keys parts and drum mode.Tests
tests/playability_lint.test.mjs(9, real-import): a flag/no-flag boundary per rule, anchor-width participation, cross-string/open-string exemptions, sustain-aware overlap, junk/empty/no-anchors degradations, dual-list precedence, and full-pass ordering. Full suite 91/91 · ESLint 0 errors · CHANGELOG updated.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit