refactor(editor): extract the painters to src/draw.js (R2, step 9b)

[byrongamatos]

Step 9b. main.js 19,963 → 19,340. And the harnesses earned their keep: they caught two real bugs while node --test stayed 86/86 green.

What moved

src/draw.js (684 lines) — the lane and grid backgrounds, beat bar, section-coverage strip, the fretted and piano-roll note painters, the cursor and marquee, the song-key highlight state those painters consult, and canvasH.

No DI seam, as predicted. drawNow(), the per-frame orchestrator, stays in main.js and calls into draw.js — so the edge is one-way, and its ~14 calls into other sections (tempo map, drum editor, parts view, the lane overlays) never cross the boundary.

drawWaveform stays with it. It paints the onset-strip and bookmark overlays behind typeof guards, and those guards exist for sliced test sandboxes, not because the overlays are optional — moving it would have silently stopped drawing them.

Three things ride along, each to where it actually belongs:

• the suggested-position mark WeakSet → notes.js (it's note metadata, and _drawNote reads it)
• MIN_NOTE_W / NOTE_PAD → geometry.js (note-body geometry, shared by painters and hit-testing)
• _coverageEditGen → state.js, renamed editGen. It was never a drawing concern. Three memos key on it — the coverage strip, the chord-at-cursor readout and the drum-limb lint — because an in-place note-time move keeps the notes array's identity and length, so a cheap cache key can't see it. A counter can't be written across a module boundary, so EditHistory._afterEdit() calls the exported bumpEditGen().

Two real bugs, caught by the harnesses

node --test was 86/86 green for both.

1. A loud one. main.js still used MIN_NOTE_W/NOTE_PAD in hit-testing after they moved, and I hadn't imported them: NOTE_PAD is not defined on every mousemove. verify_hittest and verify_resize went red immediately.

2. A silent one — and no harness would have caught it either. Two reads survived in main.js:

const gen = typeof _coverageEditGen === 'number' ? _coverageEditGen : 0;

With the counter moved away and not imported, typeof yields 'undefined', so the chord-display and drum-lint memos would have keyed on a constant 0 and never invalidated on an edit. Stale readouts, no error, no failing test. I found it by scanning main.js for free identifiers that are neither declared nor imported — which is the check this whole class of refactor actually needs, and which I'll now run every step.

That scan is also what made me look at where the counter belongs, rather than just re-exporting it from draw.js.

Tests

Five more off the slicer path.

• section_coverage imports the real pure helper — draw.js loads under node, which is exactly what the typeof window guard on DPR bought in 9a. Its two source-shape cases now read draw.js and state.js respectively.
• suggest_position_persist joins move/wiring in injecting the real WeakSet + accessors. Each env builds fresh note objects, so an identity-keyed module-shared WeakSet cannot leak marks between cases.
• key_highlight_hoist keeps its source-shape assertions and reads src/draw.js.

Verification

• node --test 86/86, pytest 248/248. No unused import, no dead export, no dangling @pure marker, no cycle.
• All five headless harnesses green: draw path, state round-trip, hit test, resize, piano roll.
• Codex preflight: NO ISSUES (asked it specifically to hunt for a third missing import).

Where R2 stands

main.js 21,176 → 19,340 across ten merged PRs. Ten modules: state, snap, theory, lanes, geometry, keys, notes, chords, canvas, draw. 29 real-import suites vs 57 slicers, from 0/82.

Next up: Hit-testing (44 lines) and Mouse interactions (1,046) now sit directly on geometry + lanes + draw.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Improved note and piano-roll rendering with clearer lane, grid, section, beat, cursor, and selection visuals.
  • Added smarter in-key highlighting that remembers per-song settings and dims out-of-scale notes more subtly.

• Bug Fixes

  • Fixed editor overlays so they refresh more reliably after edits and live moves.
  • Improved handling of suggested note positions so they stay consistent across saves and reloads.

• Refactor

  • Moved rendering and shared editor state into cleaner modules for better maintainability and consistency.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR migrates rendering/painter logic from src/main.js into a new src/draw.js module, moves note-body geometry constants into geometry.js, relocates suggested-position WeakSet tracking to notes.js, and renames the coverage cache-invalidation counter to editGen/bumpEditGen in state.js. main.js and dependent tests are updated accordingly, with a changelog entry documenting the migration.

Changes

Painter extraction, state rename, and suggested-marks migration

Layer / File(s)	Summary
Shared edit-generation counter and geometry constants src/state.js, src/geometry.js	Adds editGen/bumpEditGen() to state.js and MIN_NOTE_W/NOTE_PAD constants to geometry.js as shared primitives.
New draw.js rendering module src/draw.js	Adds painter functions for lanes/grid, editor key highlighting, section coverage (_sectionCoveragePure/_sectionCoverage), sections/labels, beat bar/selection band, notes (fretted and piano), cursor, and selection rectangle.
Suggested-position WeakSet moved to notes.js src/notes.js	Adds _suggestedNotes WeakSet and _markSuggested/_clearSuggested/_isSuggested helpers.
main.js rewiring src/main.js	Removes local painter/geometry/canvasH/suggested-marks implementations, imports them instead, and switches memoization keys from _coverageEditGen to editGen.
Test updates and changelog tests/drum_limb_lint.test.js, tests/key_highlight_hoist.test.js, tests/section_coverage.test.mjs, tests/suggest_position_*.test.mjs, CHANGELOG.md	Updates tests to reference draw.js, editGen, and notes.js helpers via ESM imports, and records the migration in the changelog.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• got-feedBack/feedBack-plugin-editor#107: Both implement/test _sectionCoveragePure/memoized _sectionCoverage with edit-generation invalidation used by drawSections.
• got-feedBack/feedBack-plugin-editor#129: Shares the _coverageEditGen → editGen/bumpEditGen rename affecting drum-limb-lint memo-key logic.
• got-feedBack/feedBack-plugin-editor#143: Both migrate suggested-position marking to shared WeakSet-based helpers in src/notes.js.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 32.50% 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	Clear and specific: it accurately summarizes the main refactor of extracting painter logic into src/draw.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-draw

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

Refactors the editor render pipeline by extracting all canvas “painter” functions out of src/main.js into a dedicated src/draw.js module, while relocating related shared state/constants to the modules where they logically belong. This continues the ES-module migration by shrinking main.js and converting additional tests from slicer/eval-style to real imports.

Changes:

• Add src/draw.js and move canvas painting responsibilities (lanes/grid/notes/sections/beat bar/cursor/marquee + key-highlight state) from main.js into it.
• Move shared edit invalidation state to src/state.js (editGen + bumpEditGen()), and move suggested-position mark tracking to src/notes.js (WeakSet + accessors); move note-body constants to src/geometry.js.
• Update tests to import real functions and inject the real suggested-mark API where sandboxes previously extracted @pure:suggest-marks.

Reviewed changes

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

Show a summary per file

File	Description
tests/suggest_position_wiring.test.mjs	Injects real suggested-mark API from src/notes.js instead of extracting a pure block.
tests/suggest_position_persist.test.mjs	Migrates to ESM imports/URLs and injects real suggested-mark API from src/notes.js.
tests/suggest_position_move.test.mjs	Injects real suggested-mark API for the move sandbox; removes extracted suggest-marks block dependency.
tests/section_coverage.test.mjs	Imports _sectionCoveragePure directly from src/draw.js; updates shape assertions to read draw.js/state.js.
tests/key_highlight_hoist.test.js	Updates source-shape assertions to read src/draw.js after painter extraction.
tests/drum_limb_lint.test.js	Updates extracted-wrapper sandbox to use editGen instead of _coverageEditGen.
src/state.js	Introduces shared editGen and bumpEditGen() for cross-module memo invalidation.
src/notes.js	Adds suggested-position WeakSet + accessors (_markSuggested, _clearSuggested, _isSuggested, _suggestedNotes).
src/main.js	Removes inlined painter implementations; imports painters from draw.js; switches memo invalidation to bumpEditGen()/editGen; imports geometry/constants and suggested-mark API.
src/geometry.js	Moves MIN_NOTE_W / NOTE_PAD into shared geometry module.
src/draw.js	New module containing all extracted painters and _sectionCoveragePure, plus memoization keyed on shared editGen.
CHANGELOG.md	Documents step 9b refactor and related module/test moves.

---

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

[coderabbitai]

🧹 Nitpick comments (1)

tests/section_coverage.test.mjs (1)

158-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Loose ordering-dependent regex for the coverage-memo key check.

/editGen[\s\S]*?_covCache/ only proves editGen appears somewhere before _covCache in the file — it doesn't confirm the cache key construction in draw.js actually uses editGen. A future refactor could satisfy this regex while breaking the real dependency (or vice versa, reordering could produce a false failure).

♻️ Tighter shape check

-    assert.ok(/editGen[\s\S]*?_covCache/.test(drawSrc),
-        'the coverage memo must key on the edit generation counter');
+    const covFnStart = drawSrc.indexOf('function _sectionCoverage');
+    assert.ok(covFnStart >= 0, '_sectionCoverage must exist in draw.js');
+    const covFnSlice = drawSrc.slice(covFnStart, covFnStart + 400);
+    assert.ok(/editGen/.test(covFnSlice) && /_covCache/.test(covFnSlice),
+        'the coverage memo must key on the edit generation counter');

🤖 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/section_coverage.test.mjs` around lines 158 - 159, The coverage-memo
test in section_coverage.test.mjs uses a loose regex that only checks ordering,
so tighten the assertion to verify the actual cache-key construction in draw.js
rather than just matching editGen before _covCache. Update the check near the
editGen/_covCache assertion to target the relevant symbol or expression in
drawSrc more precisely, so the test fails only when the memo key no longer
depends on editGen.

🤖 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/section_coverage.test.mjs`:
- Around line 158-159: The coverage-memo test in section_coverage.test.mjs uses
a loose regex that only checks ordering, so tighten the assertion to verify the
actual cache-key construction in draw.js rather than just matching editGen
before _covCache. Update the check near the editGen/_covCache assertion to
target the relevant symbol or expression in drawSrc more precisely, so the test
fails only when the memo key no longer depends on editGen.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4acbb407-097b-4440-8003-549188ce1ab0

📥 Commits

Reviewing files that changed from the base of the PR and between c0bf8fc and 99416b1.

📒 Files selected for processing (12)

• CHANGELOG.md
• src/draw.js
• src/geometry.js
• src/main.js
• src/notes.js
• src/state.js
• tests/drum_limb_lint.test.js
• tests/key_highlight_hoist.test.js
• tests/section_coverage.test.mjs
• tests/suggest_position_move.test.mjs
• tests/suggest_position_persist.test.mjs
• tests/suggest_position_wiring.test.mjs