refactor(editor): extract the string/lane model to src/lanes.js (R2, step 4)

[byrongamatos]

Step 4 of the editor's ES-module split (R2). The first functional module, and the first reassigned-scalar lift.

What moved

src/lanes.js (173 lines) — the string/lane model. main.js 21,036 → 20,800.

concern	moved
how many strings	_stringCountFor, lanes, _seedExtendedStringsFromTuning, isBassArr (module-private)
what they're called	laneLabels
string ⇄ display lane	strToLane, laneToStr
what colour they paint	colorForLane + STRING_LABEL_COLORS (module-private)

It reads S, touches no DOM, and mirrors lib/song.py:arrangement_string_count so the editor and the highway agree on string counts. Graph stays acyclic: lanes.js → state.js.

The LC container — why this step is different

Steps 2 and 3 were pure moves. This one hits the hazard I flagged: ES import bindings are read-only, and the per-frame lane cache was three module-scope lets —

let _lanesCacheActive = false;
let _lanesCacheValue  = 6;
let _laneLabelsCacheValue = null;

— written from two places that stay in main.js. draw() seeds the cache for the frame; onMouseMove() seeds it and restores the previous value in a finally around every hit test (hitNote → strToY → strToLane → lanes(), which is O(N) uncached).

So they move onto one exported container LC (.active / .value / .labels) — the shape the stems pilot used for its SH/ST lifts. Mechanical rename; main.js's only additions are the import block and that rename, everything else is deletion (verified by reconstructing the diff programmatically).

Tests: two more source-extractors die

• bass_string_count brace-counted _seedExtendedStringsFromTuning and _stringCountFor out of main.js and re-declared its own const MAX_LANES = 8. It now imports the real functions, so the clamp can't silently diverge from the source.
• strings_modal concatenated _stringCountFor's source text into a new Function sandbox. It now injects the real imported function as a parameter (the chord_at_cursor pattern from step 2).
• New tests/lanes.test.mjs drives lanes.js against the real S — the first test composing two modules. It covers what _stringCountFor's own suite doesn't: extended-range label rows (7/8-string guitar, 5/6-string bass with their ↓/↑ arrows), the string⇄lane involution, label-keyed colour (a bass low E is red exactly like a guitar's — the whole point of keying by label), the unknown-label grey fallback, and the LC cache contract itself.

Verification

• node --test 83/83, pytest 248/248.
• No shadowing of any imported symbol. I audited every import in main.js for real (non-comment) use — which is how Codex's one finding, an unused isBassArr import, got fixed: a naive grep -c counts comment mentions, and all five remaining isBassArr mentions were prose. It's now module-private.
• Served from local uvicorn on core@main (R0): src/lanes.js 200 as text/javascript.
• Three headless Chromium harnesses, all green, draw output byte-identical to the pre-change baseline:
  • draw path — loads a real chart; 6 lane colours through colorForLane; 39 (degree-label, colour) pairs all matching theory.js's own tables; out-of-key branch hit for 6/12 tonics.
  • state round-trip — S.snapIdx write → read-back through the DOM.
  • hit test (new, written for LC) — drives real mousemoves across the canvas, clicks a note → "1 note selected, string: 0"; clicks a note on a different lane → "string: 1". A lost finally restore would leave the cache hot with a stale count and resolve the second click to the wrong string.
• Codex preflight: NO ISSUES.

Note

This is the first step in this run with a genuinely behavioural surface (a hot-path cache used by drawing and hit-testing). The headless harnesses cover the guitar path end to end and the bass/extended-range logic is unit-tested, but an on-device pass is worth doing here in a way it wasn't for steps 2–3.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added improved lane labeling and color handling for guitar and bass arrangements, including extended-range instruments.
  • Lane counts now adapt more accurately to the instrument’s string range, with clearer labeling for extra strings.

• Bug Fixes

  • Improved consistency in lane-to-string mapping and mouse interaction behavior.
  • Updated lane caching so rendering and hover behavior stay in sync across frames.

• Tests

  • Expanded automated coverage for lane labels, string counts, color selection, and mapping behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

Extracts the string/lane model (lane counting, labeling, coloring, and index transforms) from src/main.js into a new src/lanes.js module with an exported LC cache container. Updates src/main.js to consume these utilities, migrates related tests to ESM importing real functions, adds a new lanes.test.mjs suite, and updates CHANGELOG.md.

Changes

Lane Model Extraction

Layer / File(s)	Summary
Lane model contract and logic src/lanes.js	New module defines MAX_LANES, exported LC cache container, colorForLane(), string-count inference (_seedExtendedStringsFromTuning, _stringCountFor), lanes()/laneLabels(), and strToLane/laneToStr transforms.
main.js wiring to LC cache src/main.js	Imports lane utilities and LC from lanes.js, removes duplicated local implementations, and rewires draw()/onMouseMove() cache handling to use LC.active/LC.value/LC.labels with proper reset in finally blocks.
Test migration and new coverage tests/bass_string_count.test.mjs, tests/strings_modal.test.mjs, tests/lanes.test.mjs, CHANGELOG.md	Converts existing tests from CommonJS source-extraction to ESM imports of real functions from src/lanes.js, adds a new tests/lanes.test.mjs suite covering labels, clamping, transforms, colors, and the LC cache contract, and documents the migration in CHANGELOG.md.

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

Sequence Diagram(s)

sequenceDiagram
  participant MainJS as main.js draw()
  participant LanesModule as lanes.js
  participant LC as LC cache

  MainJS->>LC: LC.active = false
  MainJS->>LanesModule: lanes()
  LanesModule->>LanesModule: _stringCountFor(arrangement)
  LanesModule-->>MainJS: laneCount
  MainJS->>LC: LC.value = laneCount, LC.active = true
  MainJS->>LanesModule: laneLabels()
  LanesModule-->>LC: LC.labels
  MainJS->>LanesModule: colorForLane(l)
  LanesModule->>LC: read LC.labels
  LanesModule-->>MainJS: color
  MainJS->>LC: LC.active = false, LC.labels = null

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 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 change: extracting the editor string/lane model into src/lanes.js.
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 refactor/es-module-split-lanes

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)

src/main.js

ast-grep timed out on this file

---

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

[Copilot]

Pull request overview

This PR continues the editor’s ES-module split by extracting the string/lane model from src/main.js into a new functional module src/lanes.js, and updates call sites and tests to use the extracted exports while preserving the per-frame lane cache behavior via an exported LC container.

Changes:

• Added src/lanes.js to host string-count, lane-label, string↔lane mapping, and lane-color logic (plus the exported LC cache container).
• Refactored src/main.js to import the extracted lane/string APIs and to seed/clear the LC cache in drawNow() and onMouseMove().
• Updated existing tests to import real lane functions instead of brace-counting source extraction; added a new integration-style unit test for the module.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/strings_modal.test.mjs	Switches to ESM imports and injects the real _stringCountFor from src/lanes.js into the sandbox harness.
tests/lanes.test.mjs	New test covering extended-range labels, string↔lane inversion, label-keyed colors, and LC cache behavior.
tests/bass_string_count.test.mjs	Replaces source extraction with direct imports of _seedExtendedStringsFromTuning and _stringCountFor from src/lanes.js.
src/main.js	Removes inlined lane/string model logic and replaces it with imports from src/lanes.js, including LC cache seeding/restoring.
src/lanes.js	New module implementing the extracted string/lane model and the exported LC cache container.
CHANGELOG.md	Documents the step-4 ES-module migration and associated test refactors/additions.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

🧹 Nitpick comments (1)

src/lanes.js (1)

85-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicate branch body in _seedExtendedStringsFromTuning.

Both branches compute the identical arr._extendedStrings = tuningLen - baseline;. Could collapse into a single condition for clarity, though behavior is unchanged from the verbatim move.

♻️ Optional consolidation

-        if (tuningLen > 6) {
-            arr._extendedStrings = tuningLen - baseline;
-        } else if (authoritativeLength && tuningLen > baseline
-                   && !(isBass && tuningLen === 6)) {
-            arr._extendedStrings = tuningLen - baseline;
-        }
+        const shouldSeed = tuningLen > 6 ||
+            (authoritativeLength && tuningLen > baseline && !(isBass && tuningLen === 6));
+        if (shouldSeed) arr._extendedStrings = tuningLen - baseline;

🤖 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/lanes.js` around lines 85 - 105, The `_seedExtendedStringsFromTuning`
function has duplicated branch logic that assigns the same `_extendedStrings`
value in both the `tuningLen > 6` path and the `authoritativeLength` path.
Refactor the conditional around `arrangements`/`arr._extendedStrings` so the
shared assignment happens once after the appropriate guard is selected, while
preserving the bass-specific exception and existing behavior.

🤖 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/lanes.js`:
- Around line 85-105: The `_seedExtendedStringsFromTuning` function has
duplicated branch logic that assigns the same `_extendedStrings` value in both
the `tuningLen > 6` path and the `authoritativeLength` path. Refactor the
conditional around `arrangements`/`arr._extendedStrings` so the shared
assignment happens once after the appropriate guard is selected, while
preserving the bass-specific exception and existing behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 67f1196d-d119-4a08-9a3a-a09cc1a9338a

📥 Commits

Reviewing files that changed from the base of the PR and between dc5eabf and 3fb9121.

📒 Files selected for processing (6)

• CHANGELOG.md
• src/lanes.js
• src/main.js
• tests/bass_string_count.test.mjs
• tests/lanes.test.mjs
• tests/strings_modal.test.mjs