feat(editor): song key/scale + in-key highlight for the piano roll

[ChrisBeWithYou]

Summary

A broad-stroke pass on the key-awareness thread — directly answering Christian's ask for "a toggle for notes that are in key only, account for the key of the song." When a keys arrangement is active, the toolbar shows a Key control (tonic + scale/mode) and an In-key toggle. With the highlight on:

• out-of-key rows get a neutral desaturating wash in the piano roll (drawPianoLanes);
• out-of-key notes are dimmed (_drawPianoNote).

Nothing is hidden or flagged as wrong — no red, and chromatic notes stay fully visible and editable (barbershop, prog modal interchange, and atonal passages are correctly chromatic, per the harmony charrette seat).

• SCALE_INTERVALS ships 13 tonic-relative pitch-class sets: major, the seven diatonic modes, harmonic/melodic minor, major/minor pentatonic, blues, and chromatic (shades nothing) for atonal regions.
• _pcInScalePure(pc, tonic, scale) is octave-invariant; unknown scale / non-finite inputs default to in-key so a bad state never paints the whole roll out-of-key.
• The key is a per-song editor preference (localStorage keyed by S.filename, never the feedpak — a view aid, not chart data); the highlight is a global preference. Lazily loaded per song in the control refresh (no song-load-path edit). Registry command + toolbar button + command palette; no key binding (avoids collision).

Deliberately the broad-stroke infrastructure: a later pass reads authored key regions (keys.json), adds enharmonic key-signature row spelling, the chord-tone emphasis tier, and guitar-lane scale-degree tinting.

Verification — held to the testing habits

Display feature, so the habits that bite are adversarial inputs and proving the helper uses all its arguments.

• tests/scale_membership.test.js (9 cases) drive the real _pcInScalePure: C-major naturals vs accidentals; membership proven to use both the tonic (F# out of C major, in of G major) and the scale (Bb out of C major, in of C mixolydian/minor); octave-invariance; wrap-around tonic (A minor); pentatonic/blues subsets; chromatic = all-in; adversarial unknown-scale/NaN → in-key; and every declared scale is a valid, deduped, 0..11, tonic-rooted set.
• node --check clean; all 26 JS test files pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu

Summary by CodeRabbit

• New Features
  • Added piano-roll Key / scale controls (tonic + scale/mode) and an In-key toggle.
  • Highlighting is a view preference (tonic/scale selection) and includes multiple major/minor modes, pentatonics, blues, and chromatic.
• Bug Fixes
  • Out-of-key notes are no longer hidden; they remain editable and are dimmed via a neutral desaturation wash (chromatic notes stay fully visible).
• Tests
  • Added scale membership tests, including safety for unknown/invalid inputs.
  • Added a regression test to ensure highlighting is computed efficiently during rendering.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e21ab21f-7ce5-44af-97ac-a2006c48fb95

📥 Commits

Reviewing files that changed from the base of the PR and between 6a11cda and cafd771.

📒 Files selected for processing (2)

• CHANGELOG.md
• screen.js

---

📝 Walkthrough

Walkthrough

Adds piano-roll key/scale controls, persisted key preferences, rendering changes that dim out-of-key notes, shortcut wiring, scale-membership tests, and an unreleased changelog entry.

Changes

Piano-roll in-key highlighting feature

Layer / File(s)	Summary
Scale data and membership helper screen.js	Adds scale interval sets, scale labels, and a pure pitch-class membership helper that treats invalid or unknown scale inputs as in-key.
Key controls and highlight state screen.html, screen.js	Adds hidden tonic/scale controls, per-song editor key state in localStorage, global setters and toggle wiring, UI refresh, shortcut dispatch, redraw integration, and a small coverage guard.
Rendering and validation screen.js, tests/scale_membership.test.js, tests/key_highlight_hoist.test.js	Dims out-of-key rows and notes when highlighting is enabled, and adds Node tests covering scale membership behavior, interval-set structure, and highlight hoisting.
Changelog entry CHANGELOG.md	Documents the new key/scale controls, In-key highlighting, supported scales, and preference behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main editor change: song key/scale support and in-key highlighting for the piano roll.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/editor-key-highlight

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.44.1)

screen.js

ast-grep timed out on this file

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

screen.js (1)

1863-1869: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Hoist _activeKeyHighlight() out of the per-note render path.

drawPianoLanes computes _activeKeyHighlight() once per call (Line 1584), but _drawPianoNote recomputes it for every note (Line 1865), which re-runs localStorage.getItem (via editorKeyHighlightEnabled()) and re-validates S.editorKey on every note draw. If _drawPianoNote is invoked in a per-note loop during frequent redraws, this adds avoidable synchronous localStorage reads to a hot rendering path.

Consider computing the active highlight once per frame (e.g., in the caller that iterates notes, or cache it alongside the lane pass) and passing it into _drawPianoNote, mirroring the pattern already used in drawPianoLanes.

♻️ Suggested direction

-function _drawPianoNote(n, selected) {
+function _drawPianoNote(n, selected, hl) {
     const midi = noteToMidi(n.string, n.fret);
     if (midi < pianoRange.lo || midi > pianoRange.hi) return;
     ...
-    const hl = _activeKeyHighlight();
     const outOfKey = !!hl && !_pcInScalePure(midi % 12, hl.tonic, hl.scale);

Then compute hl once in the note-iteration loop and pass it through.

🤖 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 1863 - 1869, Hoist the active key highlight lookup
out of the per-note render path so _drawPianoNote no longer calls
_activeKeyHighlight() for every note. Compute the highlight once per frame or
per note-iteration in the caller that draws notes, then pass it into
_drawPianoNote, following the same pattern already used by drawPianoLanes. Keep
the outOfKey alpha logic in _drawPianoNote, but use the precomputed hl value
instead of recalculating it.

🤖 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 1863-1869: Hoist the active key highlight lookup out of the
per-note render path so _drawPianoNote no longer calls _activeKeyHighlight() for
every note. Compute the highlight once per frame or per note-iteration in the
caller that draws notes, then pass it into _drawPianoNote, following the same
pattern already used by drawPianoLanes. Keep the outOfKey alpha logic in
_drawPianoNote, but use the precomputed hl value instead of recalculating it.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e427a733-3c51-44d7-a26b-5c92c8fc05a7

📥 Commits

Reviewing files that changed from the base of the PR and between 57d154a and c416942.

📒 Files selected for processing (4)

• CHANGELOG.md
• screen.html
• screen.js
• tests/scale_membership.test.js

[coderabbitai]

🧹 Nitpick comments (1)

tests/key_highlight_hoist.test.js (1)

24-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Comment-stripping in body() misses block comments.

Only // line comments are stripped (line 38) before the localStorage/_activeKeyHighlight( checks run. A future block comment (/* ... */) inside either function mentioning those tokens would cause a false test failure even though the code is compliant.

♻️ Strip block comments too

             if (depth === 0) {
-                return src.slice(start, i + 1).replace(/\/\/[^\n]*/g, '');
+                return src.slice(start, i + 1)
+                    .replace(/\/\*[\s\S]*?\*\//g, '')
+                    .replace(/\/\/[^\n]*/g, '');
             }

🤖 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/key_highlight_hoist.test.js` around lines 24 - 43, The body() helper in
the key highlight hoist test only removes line comments, so block comments can
still trigger false positives in the code-shape assertions. Update the
comment-stripping logic in body() to remove both // and /* ... */ comments
before checking for localStorage and _activeKeyHighlight so the function scan
stays robust.

🤖 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/key_highlight_hoist.test.js`:
- Around line 24-43: The body() helper in the key highlight hoist test only
removes line comments, so block comments can still trigger false positives in
the code-shape assertions. Update the comment-stripping logic in body() to
remove both // and /* ... */ comments before checking for localStorage and
_activeKeyHighlight so the function scan stays robust.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 09160ce5-1b1c-40e4-9abf-a931b46b8286

📥 Commits

Reviewing files that changed from the base of the PR and between 3d6736f and 6a11cda.

📒 Files selected for processing (2)

• screen.js
• tests/key_highlight_hoist.test.js

🚧 Files skipped from review as they are similar to previous changes (1)

• screen.js