feat(editor): advisory drum limb-lint (physically-impossible hits) - #129
Conversation
DAW roadmap 4.7 (F5.4 — the drum sibling of the fretted playability-lint posture). Flags hits that can't be played by a two-hand, two-foot drummer, without ever blocking or auto-fixing. - @pure:drum-limb-lint: `_drumLimbConflictsPure(hits, epsilon)` groups hits into near-simultaneous clusters (within ~12 ms, bounded from the cluster start so a fast roll never chains) and flags two conditions: * 'hands' — 3+ stick-struck pieces at one instant (feet = kick + hh_pedal never count; two hands + two feet is the playable ceiling). * 'hihat' — hh_open with hh_closed, or hh_open with hh_pedal (the foot can't be up-for-open and down-on-the-pedal at once). `_drumConflictIndexSetPure` flattens the conflicts to the hit indices the renderer marks. Read-only — never mutates a hit; skips NaN-time hits. - _drumEditorDraw: computes the conflict set once per draw over the already- sorted hits (single O(n) pass, drum-editor mode only), draws a small amber warning triangle above each conflicted hit, and adds an amber advisory line to the HUD ("N playability hints … nothing is blocked"). Advisory only, matching the fretted-lint / drum-lint posture in the DAW doc: no gate, no auto-move. Tests: tests/drum_limb_lint.test.js (15) — both rules, feet-don't-count, duplicate-piece dedupe, epsilon clustering + no-runaway-chain, adversarial (non-array / NaN times), read-only guarantee, index flattening. All fail on main. Full JS suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzMBtxWnLGHYkMMXtK38Bg
|
Warning Review limit reached
Next review available in: 31 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)
📝 WalkthroughWalkthroughAdds an advisory-only drum playability lint that detects physically impossible near-simultaneous drum hits, renders amber warnings and a HUD count in the editor, and documents and tests the behavior. ChangesDrum playability lint
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DrumEditorDraw
participant DrumLimbConflicts as _drumLimbConflicts
participant ConflictIndexSet as _drumConflictIndexSetPure
participant HUD
DrumEditorDraw->>DrumLimbConflicts: analyze current hits
DrumLimbConflicts-->>DrumEditorDraw: conflict records
DrumEditorDraw->>ConflictIndexSet: flatten conflict records
ConflictIndexSet-->>DrumEditorDraw: conflicted hit indices
DrumEditorDraw->>DrumEditorDraw: draw amber warning triangle per conflicted hit
DrumEditorDraw->>HUD: report playability hint count
HUD-->>DrumEditorDraw: render amber advisory line
🚥 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)
screen.js (1)
14705-14712: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate flattening logic vs.
_drumConflictIndexSetPure.This block re-implements the index-flattening already provided by
_drumConflictIndexSetPure(Line 14343-14349), just inline to avoid a second_drumLimbConflictsPurepass. Consider adding a small overload/parameter to_drumConflictIndexSetPurethat accepts a precomputed conflicts array, so both call sites share one implementation.♻️ Optional refactor
-function _drumConflictIndexSetPure(hits, epsilon) { +function _drumConflictIndexSetPure(hits, epsilon, conflicts) { const set = new Set(); - for (const c of _drumLimbConflictsPure(hits, epsilon)) { + for (const c of (conflicts || _drumLimbConflictsPure(hits, epsilon))) { for (const idx of c.indices) set.add(idx); } return set; }const _lintConflicts = _drumLimbConflictsPure(hits, DRUM_LIMB_EPSILON); - const _conflictIdx = new Set(); - for (const c of _lintConflicts) for (const idx of c.indices) _conflictIdx.add(idx); + const _conflictIdx = _drumConflictIndexSetPure(hits, DRUM_LIMB_EPSILON, _lintConflicts);🤖 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 14705 - 14712, The playability lint block is duplicating the same conflict-index flattening logic already centralized in _drumConflictIndexSetPure. Update _drumConflictIndexSetPure to accept an optional precomputed conflicts array, and reuse it here instead of rebuilding _conflictIdx inline after calling _drumLimbConflictsPure. Keep the existing behavior in the draw path, but make both call sites share the same index-set implementation.
🤖 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 `@screen.js`:
- Around line 14705-14712: The playability lint block is duplicating the same
conflict-index flattening logic already centralized in
_drumConflictIndexSetPure. Update _drumConflictIndexSetPure to accept an
optional precomputed conflicts array, and reuse it here instead of rebuilding
_conflictIdx inline after calling _drumLimbConflictsPure. Keep the existing
behavior in the draw path, but make both call sites share the same index-set
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d79476cb-e3ee-4675-b271-4f656e8021e7
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/drum_limb_lint.test.js
Memoize the advisory limb-lint behind the shared edit-generation counter (_coverageEditGen) plus hits array identity/length so the O(n) cluster pass no longer runs on every repaint, and pause it during a live drum-move drag — mid-drag hit times are mutated in place and only re-sorted on drop, so the sorted-input clusterer could flash spurious/missed markers on a transiently unsorted array. Adds stateful-wrapper regression tests (drag bypass + memo invalidation + draw-path source locks) that fail on pre-fix code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/drum_limb_lint.test.js (3)
244-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFunction-body extraction relies on top-level
\nfunctionboundary.
indexOf('\nfunction ', fnStart + 1)only terminates the body at another unindentedfunctiondeclaration. If_drumEditorDrawever grows an inner helper declared withfunctionstarting at column 0 within its body (unlikely but possible with certain formatting), the extractedbodywould be truncated before or including unrelated code, silently weakening this source-lock assertion. Low risk given current formatting conventions, but worth a brace-matching approach (as used inbody()intests/key_highlight_hoist.test.js) for robustness if this pattern is reused frequently.🤖 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/drum_limb_lint.test.js` around lines 244 - 253, The body extraction for _drumEditorDraw is too fragile because it relies on finding the next top-level “function” token instead of matching the function’s braces. Update the test in drum_limb_lint.test.js to use a brace-aware body extraction approach like the helper used in key_highlight_hoist.test.js, so the assertion still targets _drumEditorDraw even if formatting or nested helpers change.
218-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
setGen(1)call.At line 238,
W.setGen(1)isn't needed to force recomputation:sortedis a distinct array reference fromunsorted, so thehitsRef !== hitscheck alone already invalidates the memo per the wrapper's key logic (Context snippet 2). Not incorrect, just slightly muddies the test's intent by implying the gen bump is what triggers the recompute.Simplify by dropping the redundant gen bump
// Drag done → lint resumes (and this array, once dropped, is re-sorted). W.setDrag(null); - W.setGen(1); assert.deepStrictEqual(W.fn(sorted), [], 'after drop, sorted hits lint clean');🤖 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/drum_limb_lint.test.js` around lines 218 - 240, Remove the redundant gen bump in the drum-move pause test so the intent stays focused on array identity invalidation. In the `LIVE drum-move drag pauses the lint` test, drop the `W.setGen(1)` call and keep the assertion that `W.fn(sorted)` recomputes cleanly via the `hitsRef !== hits` path in the wrapper logic. This should leave the `buildWrapper` / `W.fn` behavior under test without implying the generation counter is required for the refresh.
255-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLoose proximity regex for the memo-key source lock.
/_coverageEditGen[\s\S]*?_drumLintCache|_drumLintCache[\s\S]*?_coverageEditGen/only confirms both identifiers appear somewhere insrcnear each other — it doesn't confirm_coverageEditGenactually participates in cache-key construction inside_drumLimbConflicts. This is a secondary sanity check though; the behavioral test at Line 208-216 already exercises the real invalidation contract, so the risk of this being misleading is low.🤖 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/drum_limb_lint.test.js` around lines 255 - 258, The memo-key assertion in the drum lint test is too loose because it only checks that _coverageEditGen and _drumLintCache appear near each other in src. Tighten the check in tests/drum_limb_lint.test.js so it verifies _coverageEditGen is actually part of the cache-key construction used by _drumLimbConflicts, rather than just nearby text, while keeping the existing invalidation behavior covered by the behavioral test.
🤖 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 `@tests/drum_limb_lint.test.js`:
- Around line 244-253: The body extraction for _drumEditorDraw is too fragile
because it relies on finding the next top-level “function” token instead of
matching the function’s braces. Update the test in drum_limb_lint.test.js to use
a brace-aware body extraction approach like the helper used in
key_highlight_hoist.test.js, so the assertion still targets _drumEditorDraw even
if formatting or nested helpers change.
- Around line 218-240: Remove the redundant gen bump in the drum-move pause test
so the intent stays focused on array identity invalidation. In the `LIVE
drum-move drag pauses the lint` test, drop the `W.setGen(1)` call and keep the
assertion that `W.fn(sorted)` recomputes cleanly via the `hitsRef !== hits` path
in the wrapper logic. This should leave the `buildWrapper` / `W.fn` behavior
under test without implying the generation counter is required for the refresh.
- Around line 255-258: The memo-key assertion in the drum lint test is too loose
because it only checks that _coverageEditGen and _drumLintCache appear near each
other in src. Tighten the check in tests/drum_limb_lint.test.js so it verifies
_coverageEditGen is actually part of the cache-key construction used by
_drumLimbConflicts, rather than just nearby text, while keeping the existing
invalidation behavior covered by the behavioral test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7852bd28-1b57-41d3-a2f5-a39127f27862
📒 Files selected for processing (2)
screen.jstests/drum_limb_lint.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- screen.js
Brace-match the _drumEditorDraw body extraction (robust to inner function declarations), assert _coverageEditGen participates in the memo key inside _drumLimbConflicts rather than mere proximity, and drop a redundant edit-gen bump in the drum-move pause test (the fresh array reference already invalidates via hitsRef). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed all three nitpicks in e5da08e (test-only): brace-matched the @coderabbitai review |
|
✅ Action performedReview finished.
|
…-lint # Conflicts: # CHANGELOG.md
2 participants
DAW roadmap 4.7 (§F5.4 — the drum sibling of the fretted playability-lint posture). Editor-only.
What
The drum grid now flags hits a two-hand, two-foot drummer physically can't play — with a small amber warning triangle on the offending hits and an advisory line in the editor HUD. It never blocks, never auto-fixes, never mutates a hit.
Two rules, per near-simultaneous cluster:
hands— 3+ stick-struck pieces at one instant. A drummer has two hands, so three simultaneous non-foot pieces needs three hands. Feet (kick,hh_pedal) don't count toward the limit — two hands + two feet is the playable ceiling.hihat— a contradictory hi-hat state:hh_opentogether withhh_closed(can't be both) or withhh_pedal(the foot can't be up-for-open and down-on-the-pedal at once).hh_closed+hh_pedalis fine.Clustering is tight (~12 ms, bounded from the cluster's start), so a fast roll or a flam pair (~30 ms) never merges into a false positive.
Implementation
@pure:drum-limb-lint:_drumLimbConflictsPure(hits, epsilon)→ one entry per conflicted cluster{time, indices, pieces, reasons};_drumConflictIndexSetPureflattens to the marked indices. Read-only; skips NaN-time hits._drumEditorDraw: computes the conflict set once per draw over the already-sorted hits (single O(n) pass, drum-editor mode only — never touches the guitar/keys path), marks conflicted hits, and adds the amber HUD line.Tests —
tests/drum_limb_lint.test.js(15, all fail on main)Both rules; feet-don't-count; duplicate-piece dedupe (2 snares = one hand piece); epsilon clustering + the no-runaway-chaining bound; a fast roll staying clear; adversarial (non-array, NaN times); the read-only guarantee; and the index flattener. Full JS suite green.
Scope / fresh region
New pure block after
@pure:drum-density, plus the marker + HUD line in_drumEditorDraw. No core, no spec, no overlap with the open PRs (#112/#125/#126/#127/#128).Summary by CodeRabbit