refactor(editor): extract canvas geometry to src/geometry.js (R2, step 5)

[byrongamatos]

Step 5 of the editor's ES-module split (R2). The tier every draw and hit-test path sits on.

What moved

src/geometry.js (82 lines) — main.js 20,799 → 20,764.

• time ⇄ x: timeToX, xToTime
• string ⇄ lane ⇄ y: laneToY, yToLane, strToY, yToStr
• lane metrics: WAVEFORM_H, LANE_H, BEAT_H, LABEL_W, ANCHOR_LANE_H, HS_LANE_H
• scroll bounds: _editorViewportDurationPure, _editorMaxScrollXPure, _editorClampScrollXPure, EDITOR_SCROLL_TAIL_SECONDS

Reads S and the lane model, no DOM. Graph stays acyclic: geometry → lanes → state.

No renames this time — and the reason is worth stating

Step 4 needed the LC container because the lane cache is reassigned and ES import bindings are read-only. These three lane metrics are reassigned too, on every resize. But their only writer — the lane-sizing arithmetic inside resizeCanvas() — could move with them:

export let WAVEFORM_H = 70, LANE_H = 44, BEAT_H = 24;
export function setLaneMetrics(canvasHeightPx) { … }   // the sole writer

ES import bindings are live. So main.js's ~100 read sites keep reading the current value with no rename at all, and none of them can write it — an assignment from an importer throws TypeError. That's strictly better than a container: same ergonomics, and the read-only guarantee is enforced by the language instead of by convention.

LC needed a container only because its writers (draw(), onMouseMove()) have to stay in main.js. The distinction is "can the writer move?", not "is it reassigned?".

main.js's entire diff is the deletions, the import block, and three assignments in resizeCanvas collapsing to one setLaneMetrics(h) call.

Tests

• scroll_bounds — the last @pure-block slicer among the coordinate helpers — now imports the real module.
• New tests/geometry.test.mjs pins the mappings (round-trip inversion while scrolled, out-of-range yToStr clamping instead of a phantom string, gutter offset, lane height dividing by the current string count so a 4-string bass gets taller lanes) and the live-binding contract itself: setLaneMetrics updates what importers see, an importer assigning throws TypeError, and laneToY tracks a resize rather than capturing boot-time metrics.

Verification

• node --test 84/84, pytest 248/248.

• Grepped for any surviving assignment to the three metrics anywhere in main.js — there is none; one would now throw at runtime. (The three assignments in geometry.js are all inside setLaneMetrics.)

• No unused import, no shadowing. Served from local uvicorn on core@main (R0): src/geometry.js 200 as text/javascript.

• Four headless Chromium harnesses, all green — draw path, state round-trip, hit test, plus a new resize harness written for this change:

  check	result
  canvas height shrunk	663px → 283px
  fret-label baseline moved	−276px → draw() sees the new metrics
  click at the new coords selects the note	hit-test agrees with draw
  no phantom hit at the stale position

  A read site that captured a metric at module-eval time would leave the baseline delta at 0 and fail the second row. Both draw and hit-test read the same bindings, so the test is that they agree at the new size and that the geometry actually moved.

• Codex preflight: NO ISSUES.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Improved canvas positioning and scrolling behavior for the arrangement editor, including more accurate time, lane, and string coordinate mapping.
  • Canvas lane sizing now updates more consistently when the editor is resized.

• Tests

  • Added coverage for coordinate conversions, resize-driven metric updates, and scroll boundary calculations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4446a590-42cf-48fc-a48b-ff5164f80ce5

📥 Commits

Reviewing files that changed from the base of the PR and between e9b8230 and 6db9c8d.

📒 Files selected for processing (5)

• CHANGELOG.md
• src/geometry.js
• src/main.js
• tests/geometry.test.mjs
• tests/scroll_bounds.test.mjs

---

📝 Walkthrough

Walkthrough

Canvas geometry logic (lane metrics, time/coordinate mapping, scroll-bound calculations) is extracted from src/main.js into a new src/geometry.js ES module with live mutable bindings. src/main.js is updated to import these instead of defining them locally. Tests are added/updated to validate the module, and CHANGELOG.md documents the migration.

Changes

Canvas geometry module extraction

Layer / File(s)	Summary
New geometry module src/geometry.js	Exports layout constants, live lane-metric bindings (WAVEFORM_H, LANE_H, BEAT_H), setLaneMetrics(), time/x and lane/string/y mapping functions, and scroll-bound pure helpers.
main.js delegation src/main.js	Removes locally defined metrics and mapping helpers, imports equivalents from geometry.js, and replaces inline resize computation with a single setLaneMetrics(h) call.
Tests and changelog tests/geometry.test.mjs, tests/scroll_bounds.test.mjs, CHANGELOG.md	Adds a new test suite validating mapping round-trips, live-binding read-only contract, and resize behavior; converts scroll-bounds test to import the real module; documents the migration in the changelog.

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

Sequence Diagram(s)

sequenceDiagram
  participant MainJS as main.js
  participant Geometry as geometry.js
  participant State as S (state)
  MainJS->>Geometry: setLaneMetrics(canvasHeightPx)
  Geometry->>State: read lane count
  Geometry->>Geometry: recompute WAVEFORM_H, LANE_H, BEAT_H
  MainJS->>Geometry: timeToX(t) / laneToY(l)
  Geometry-->>MainJS: mapped coordinate

Loading

🚥 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 matches the main change: extracting editor canvas geometry into src/geometry.js for R2 step 5.
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 refactor/es-module-split-geometry

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

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

[Copilot]

Pull request overview

This PR continues the editor’s ES-module split (R2, step 5) by extracting the shared canvas geometry layer (time/x mapping, lane/y mapping, lane sizing metrics, and scroll-bound math) into a dedicated src/geometry.js module, keeping the dependency graph clean (geometry → lanes → state) and avoiding DOM coupling.

Changes:

• Added src/geometry.js to centralize coordinate mapping helpers, lane metrics, and scroll-bound arithmetic.
• Updated src/main.js to import geometry helpers/metrics and to delegate resize lane sizing to setLaneMetrics(h).
• Updated and added tests to import the real module (tests/scroll_bounds.test.mjs) and to pin geometry behavior + the live-binding contract (tests/geometry.test.mjs).

Reviewed changes

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

Show a summary per file

File	Description
tests/scroll_bounds.test.mjs	Switches from source-slicing the @pure block to importing scroll-bound helpers from src/geometry.js.
tests/geometry.test.mjs	Adds coverage for geometry mappings, clamping behavior, scroll tail export, and live export let metric updates.
src/main.js	Removes inline geometry/metric definitions; imports from geometry.js and uses setLaneMetrics() in resizeCanvas().
src/geometry.js	New module containing time/x, string/lane/y mappings, lane metrics + writer (setLaneMetrics), and scroll-bound pure helpers.
CHANGELOG.md	Documents the ES-module migration step and the rationale for live exported metric bindings.

---

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