refactor(editor): move the annotation lanes to src/annotation-lanes.js (R2, step 16) - #167
Merged
Merged
Conversation
…s (R2, step 16) src/main.js 17,756 -> 16,078. Nineteenth module; the graph stays acyclic. 1,678 lines — the tone lane, the anchor lane and the handshape lane, which turned out to be a contiguous tail of the IIFE. They move together because they lean on each other: the handshape lane positions itself off _anchorLaneTopY, and both it and the anchor lane share _currentAnchorArr. Split apart, those would be cross-module imports for no gain. main.js keeps the canvas event routing (deciding which strip is under the cursor) and forwards to the on*LaneMouse* handlers. Four main.js symbols travel back — draw, hideContextMenu, snapTime, _editorPromptText — and would close a cycle, so they arrive through setLaneHooks(). snapTime stays behind because its onset-snap path reaches _ensureOnsets and the onset cache; _editorPromptText stays because it owns a modal and the shared _editorPromptCancel handle. Neither is a lane concern. TONE_LANE_H moved to geometry.js, where ANCHOR_LANE_H and HS_LANE_H already live. The tones modal's three window.* handlers became exported functions that main.js re-attaches: a top-level `window.x =` throws when the module is imported under node. Two internal call sites that reached back through window.editorHideTonesModal() now call the module-local function (Codex). Also fixes a regression this refactor's own predecessors introduced. `draw` is reassigned near the bottom of main.js to a wrapper that refreshes seven toolbar buttons before repainting. setHistoryHooks() and setDrumHooks() were handed the bare identifier, so they captured the ORIGINAL function at wiring time; every undo, redo and drum-density toggle has been skipping those refreshes since #165/#166. The canvas repaints either way, which is exactly why it went unnoticed — the only visible symptom is the drum-density button keeping its "Rows: Full" label after the grid has collapsed to Compact. All three hook sites now take `_drawLive = (...args) => draw(...args)`, resolving the live binding at call time as the in-IIFE call sites always did. Found by Codex; verify_drum.py now asserts the label, and fails on the pre-fix code. Tests: anchor_authoring (CJS -> .mjs) and handshape_authoring stop brace-matching declarations out of main.js and import them. Verified beyond the unit tests, which cannot see hook wiring: verify_lanes.py clicks the anchor lane and asserts an anchor is added, the canvas repaints (_hooks.draw), the anchor lands on the grid rather than under the cursor (_hooks.snapTime), and the tones modal opens through the re-attached window.* handler. Comment out setLaneHooks() and all 88 unit tests still pass while three of its eight checks fail. node --test 88/88, pytest 248/248, npm run lint 0 errors, Codex clean, all 12 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExtracts tone, anchor, and handshape annotation lane logic into ChangesAnnotation lanes extraction and hook wiring fix
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MainJS as main.js
participant DrawLive as _drawLive
participant AnnotationLanes as annotation-lanes.js
MainJS->>DrawLive: wrap current draw reference
MainJS->>AnnotationLanes: setLaneHooks(_drawLive)
AnnotationLanes->>DrawLive: invoke hook after edit
DrawLive->>MainJS: call current draw() implementation
sequenceDiagram
participant User
participant AnchorLane as annotation-lanes.js
participant PromoteAnchorsCmd
participant Arrangement as arr.anchors_user
User->>AnchorLane: click or drag auto anchor
AnchorLane->>PromoteAnchorsCmd: exec(autoAnchors, clicked, extra)
PromoteAnchorsCmd->>Arrangement: seed full fallback set into anchors_user
Arrangement-->>AnchorLane: updated authored anchors
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Refactors the editor by extracting the tone/anchor/handshape “annotation lanes” out of the monolithic src/main.js into a dedicated module, while also fixing a subtle regression where hook consumers captured the pre-wrapper draw() implementation and therefore skipped toolbar refreshes during undo/redo and drum-density toggles.
Changes:
- Moved the tone lane, anchor lane, and handshape lane into
src/annotation-lanes.jsand wired the module back intomain.jsviasetLaneHooks()pluswindow.*re-attachments for modal handlers. - Fixed stale
drawcaptures by passing a live-binding thunk (_drawLive) tosetHistoryHooks(),setDrumHooks(), andsetLaneHooks(). - Updated lane geometry (
TONE_LANE_H), tests to import real module symbols, and the changelog entry for the regression/fix.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/annotation-lanes.js | New module containing tone/anchor/handshape lane drawing, commands, and mouse handlers, with cycle-breaking hooks. |
| src/main.js | Imports and wires annotation-lanes; fixes hook wiring to use a live draw binding; re-attaches window.* modal handlers. |
| src/geometry.js | Adds exported TONE_LANE_H constant alongside other lane height constants. |
| tests/anchor_authoring.test.mjs | Stops brace-matching from main.js; imports lane authoring code from annotation-lanes.js and seeds real S. |
| tests/handshape_authoring.test.mjs | Stops brace-matching from main.js; imports _handshapeSpanFrets from annotation-lanes.js. |
| CHANGELOG.md | Documents the draw hook capture regression fix and the annotation lanes extraction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ed code Copilot, on #167. The three comments travelled verbatim with the code and now describe an ES module: TONE_LANE_H comes from geometry.js rather than being declared at the top of the IIFE; _editorConfirmToneDefinitions is an ordinary export, not a plain function relying on the global -> IIFE fallback; and _handshapeSpanFrets is imported, not brace-matched out of main.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
All three are correct — the comments travelled verbatim with the code and were describing the IIFE they no longer live in. Fixed:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nineteenth module.
src/main.js17,756 → 16,078 — 1,678 lines, and the graph stays acyclic.What moved
The tone lane, the anchor lane and the handshape lane. They turned out to be a contiguous tail of the
main.jsIIFE, so the excision is a truncation.They travel together rather than as three modules because they lean on each other: the handshape lane positions itself off
_anchorLaneTopY, and both it and the anchor lane share_currentAnchorArr. Split apart, those become cross-module imports for no gain.main.jskeeps the canvas event routing — deciding which strip is under the cursor — and forwards to theon*LaneMouse*handlers.Four
main.jssymbols travel back and would close a cycle, so they arrive throughsetLaneHooks(). Two of them deliberately stay behind:snapTime— its onset-snap path reaches_ensureOnsetsand the onset cache._editorPromptText— it owns a modal and the shared_editorPromptCancelhandle.Neither is a lane concern.
TONE_LANE_Hmoved togeometry.js, joiningANCHOR_LANE_HandHS_LANE_Hwhere it always belonged.A regression this refactor’s own predecessors introduced
Codex caught it on review, and it is already on
main.drawis reassigned near the bottom ofmain.jsto a wrapper that refreshes seven toolbar buttons before repainting:setHistoryHooks({ draw, … })andsetDrumHooks({ draw, … })were handed the bare identifier, so they captured the original function at wiring time. Since #165/#166, every undo, redo and drum-density toggle has skipped those seven refreshes.The canvas repaints either way — which is precisely why nothing looked wrong, and why my own
verify_drum.pypassed. The one visible symptom: the drum-density button keeps itsRows: Fulllabel after the grid has already collapsed to Compact. All three hook sites now take_drawLive = (...args) => draw(...args), resolving the live binding at call time, as the in-IIFE call sites always did.verify_drum.pynow asserts the label. It fails on the pre-fix code.Tests
anchor_authoring(CJS →.mjs) andhandshape_authoringstop brace-matching declarations out ofmain.jsand import them instead.Why a headless harness, again
Only the anchor/handshape authoring helpers had unit coverage. The draw passes, the mouse handlers and all four hooks are invisible to
node --test.verify_lanes.pyclicks the anchor lane:_hooks.draw)_hooks.snapTime)The
snapTimecheck is the interesting one: its no-op default is the identity function, so a dead hook silently places the anchor exactly under the cursor instead of on the grid. Comment outsetLaneHooks(...)and all 88 unit tests still pass.Verification
node --test88/88 ·pytest248/248 ·npm run lint0 errors (9 warnings) · Codex clean · all 12 headless harnesses pass.🤖 Generated with Claude Code
Summary by CodeRabbit