refactor(editor): extract hit testing, shortcuts and setStatus (R2, step 10) - #160
Merged
Merged
Conversation
…tep 10) src/main.js 19,339 -> 18,852. Three modules, and the first step with the new ESLint gate running. src/hit-test.js (64) — hitNote / hitNoteEdge and the EDGE_GRAB sustain-resize zone. Pure geometry over S; no DOM, no canvas. By the time the earlier tiers had landed it had ZERO calls left into main.js: everything it needs (timeToX, strToY, midiToY, isKeysMode, _rollPitchCtx, _rollMidiForNote, notes) is already a module. src/shortcuts.js (466) — the two profiles (FeedBack native / EOF legacy), their key->command maps, the right-click behaviour that rides on the profile, the localStorage persistence and the shortcut-panel renderer. Its ONLY dependency outside main.js was setStatus, which is why: src/ui.js (11) — setStatus. Four lines, ~180 call sites. It gets its own module rather than being dragged into whichever consumer happened to need it first; every future extraction that wants to talk to the user now has somewhere to import from. LIVE BINDINGS again: editorShortcutProfile and editorRightClickBehavior are reassigned, but every writer moved with them, so main.js's read sites (in the global keydown handler) are untouched and cannot write. `editorWaveformVisible` stayed behind — its writer is a view toggle in another section. The two `window.editorSet*` handlers become plain exported functions; main.js keeps the `window.*` surface that screen.html's inline handlers call (§V). That also keeps shortcuts.js importable under node — a top-level `window.x = …` would have thrown on import, which is exactly what happened on the first attempt. Tests: eof_shortcuts becomes a pure real-import suite; bookmarks becomes a hybrid (still slices @pure:bookmarks and @pure:shortcut-panel-hint, both of which stay in main.js). Verified: npm run lint 0 errors / 10 warnings — the gate added in #159 confirmed no missing import this time, and caught MIN_NOTE_W/NOTE_PAD becoming unused in main.js once hit-testing moved. node --test 87/87, pytest 248/248. No dead export, no cycle (shortcuts -> ui only; hit-test -> geometry/keys/notes/state). SIXTH headless harness, written for this step: nothing else presses a key. `toggleWaveform` is bound to W under the FeedBack profile and F5 under EOF, and reports through setStatus, so the profile switch is directly observable: feedback: W toggles, F5 inert. eof: F5 toggles, W inert. Both directions are asserted — checking only that W works under `feedback` would pass even with a dead profile switch, which is precisely what a broken live binding would look like. All five existing harnesses green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR extracts hit-testing, shortcut/right-click logic, and status updates into new ES modules, rewires ChangesModule extraction and rewiring
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.jsast-grep timed out on this file Comment |
There was a problem hiding this comment.
Pull request overview
This PR continues the editor ES-module refactor by extracting hit-testing logic, shortcut/right-click profile handling, and the shared status-line helper out of src/main.js into dedicated modules, while keeping the existing window.* API surface for HTML inline handlers.
Changes:
- Extracted pointer hit testing into
src/hit-test.jsand wiredmain.jsto importhitNote/hitNoteEdge. - Extracted shortcut profiles, key→command mapping, right-click behavior, persistence, and shortcut-panel rendering into
src/shortcuts.js. - Extracted
setStatusinto a small shared modulesrc/ui.jsand updated tests to import shortcut logic directly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/main.js |
Replaces in-file shortcut/hit-test/status helpers with imports; preserves window.editorSet* surface. |
src/hit-test.js |
New module containing hitNote/hitNoteEdge and the sustain-resize edge grab zone. |
src/shortcuts.js |
New module for shortcut profiles, command mapping, right-click behavior, persistence, and shortcut panel rendering. |
src/ui.js |
New shared setStatus helper module. |
tests/eof_shortcuts.test.mjs |
Refactors the test to import src/shortcuts.js directly instead of slicing main.js. |
tests/bookmarks.test.mjs |
Updates imports to use real shortcut-profile exports while still slicing the remaining @pure blocks from main.js. |
CHANGELOG.md |
Adds an entry documenting migration step 10 and the module extractions. |
Comments suppressed due to low confidence (1)
tests/eof_shortcuts.test.mjs:2
- The header comment still says these tests are for
src/main.js, but the suite now imports and testssrc/shortcuts.js. Updating the comment will keep the test description accurate.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/hit-test.js`:
- Around line 51-64: `hitNoteEdge` is using the wrong vertical geometry in
non-strum mode, so the sustain resize grab area does not line up with the note
body. Update `hitNoteEdge` to follow the same `isKeysMode()` branching logic
used in `hitNote`, using the keys-mode row positioning when `isKeysMode()` is
true and the current `strToY()`/`LANE_H` path otherwise, so the edge-drag hit
test matches the rendered note rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 09b3ec96-fbc1-42b9-9e67-962533a09607
📒 Files selected for processing (7)
CHANGELOG.mdsrc/hit-test.jssrc/main.jssrc/shortcuts.jssrc/ui.jstests/bookmarks.test.mjstests/eof_shortcuts.test.mjs
…o roll Two review findings on #160. [CodeRabbit, real bug — pre-existing] `hitNote` branches on isKeysMode() and resolves a note to its sounding-pitch row via midiToY. `hitNoteEdge` did not: it always computed `y` from `strToY(n.string)`, the fretted lane band. So the sustain-resize grab zone sat on rows the roll never draws on — even though the call site in main.js explicitly documents that edge-drag resize "applies directly even in the read-only fretted roll (V4)", because a duration edit is pitch-preserving and passes the roll's edit lock. The cursor hint had the same bug from the other side: it was gated on `y >= WAVEFORM_H && y < WAVEFORM_H + L * LANE_H`, the fretted band, so `ew-resize` never appeared in the roll. It now uses `_beatBarTopY()`, already computed two lines above, which is the bottom of the note area in BOTH views (it accounts for pianoLaneCount * PIANO_LANE_H in the roll). Root cause, not symptom: two functions computed the same note rectangle two different ways, so only one of them grew the keys-mode branch. They now share one `_noteRect(n, keysMode, rctx)`, which also returns null for an unresolvable pitch so neither hit-tests a wrong row. New tests/hit_test.test.mjs drives the real S + keys model. The two roll cases were verified to FAIL against the unfixed code, and to fail in BOTH directions: the edge is not grabbable on the roll row, and IS grabbable at the stale fretted-lane row. A guard that only asserts the first would pass on a function that hit-tests nothing at all. [Copilot] Stray leading space before an `if` in shortcuts.js — pre-existing, moved verbatim from main.js. npm run lint 0 errors / 10 warnings. node --test 88/88, pytest 248/248. All six headless harnesses green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/hit_test.test.mjs (1)
44-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the sentinel filename cache-bust here.
keys._viewPrefs()is an internal cache keyed onS.filename, so this seed depends on a private implementation detail and a magic'\u0000reset'value. A small reset/testing helper insrc/keys.jswould make the fixture less brittle.🤖 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/hit_test.test.mjs` around lines 44 - 46, The test is relying on a sentinel S.filename value to bust the cache in keys._viewPrefs(), which couples the fixture to a private implementation detail. Add a small reset/testing helper in src/keys.js for clearing the _viewPrefs cache, then update the hit_test fixture to use that helper instead of assigning '\u0000reset' to S.filename. Keep the change localized around keys._viewPrefs and the related S.filename setup in the test.
🤖 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/hit_test.test.mjs`:
- Around line 44-46: The test is relying on a sentinel S.filename value to bust
the cache in keys._viewPrefs(), which couples the fixture to a private
implementation detail. Add a small reset/testing helper in src/keys.js for
clearing the _viewPrefs cache, then update the hit_test fixture to use that
helper instead of assigning '\u0000reset' to S.filename. Keep the change
localized around keys._viewPrefs and the related S.filename setup in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6934ffe-bf96-453b-8a67-559e9f98d93d
📒 Files selected for processing (5)
CHANGELOG.mdsrc/hit-test.jssrc/main.jssrc/shortcuts.jstests/hit_test.test.mjs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hit-test.js
- src/shortcuts.js
This was referenced Jul 10, 2026
feat(editor): multi-track MIDI playback — the chart plays as a band, mixed by the Tracks strips
#280
Merged
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.
Step 10.
main.js19,339 → 18,852. Three modules, and the first step with the ESLint gate from #159 actually running.What moved
src/hit-test.js(64) —hitNote/hitNoteEdgeand theEDGE_GRABsustain-resize zone. Pure geometry overS; no DOM, no canvas.Worth noting: by the time the earlier tiers had landed it had zero calls left into
main.js. Everything it needs —timeToX,strToY,midiToY,isKeysMode,_rollPitchCtx,_rollMidiForNote,notes— was already a module. The 44-line section that looked awkward at step 9 fell out for free.src/shortcuts.js(466) — the two profiles (FeedBack native / EOF legacy), their key→command maps, the right-click behaviour that rides on the profile, localStorage persistence, and the shortcut-panel renderer. Its only dependency outsidemain.jswassetStatus. Which is why:src/ui.js(11) —setStatus. Four lines, ~180 call sites. It gets its own module rather than being dragged into whichever consumer happened to need it first; every future extraction that wants to talk to the user now has somewhere to import from.Live bindings, and a lesson
editorShortcutProfileandeditorRightClickBehaviorare reassigned, but every writer moved with them, so they'reexport letandmain.js's read sites (in the global keydown handler) are untouched and can't write.editorWaveformVisiblestayed behind — its writer is a view toggle in a different section.The two
window.editorSet*handlers become plain exported functions, withmain.jskeeping thewindow.*surfacescreen.html's inline handlers call (§V). That also keepsshortcuts.jsimportable under node — a top-levelwindow.x = …throws on import, which is exactly what my first attempt did.Verification
npm run lint: 0 errors, 10 warnings. The gate from chore(editor): add ESLint no-undef (typeof:true) to the src module graph #159 confirmed no missing import this time, and caughtMIN_NOTE_W/NOTE_PADgoing unused inmain.jsonce hit-testing took them. That is the check earning its keep on the very next step.node --test87/87,pytest248/248. No dead export, no cycle (shortcuts → ui;hit-test → geometry/keys/notes/state).eof_shortcutsbecomes a pure real-import suite;bookmarksa hybrid (still slices@pure:bookmarksand@pure:shortcut-panel-hint, both of which stay inmain.js).A sixth harness, because nothing else presses a key
toggleWaveformis bound toWunder the FeedBack profile andF5under EOF, and it reports throughsetStatus— so the profile switch is directly observable:WF5Both directions are asserted. Checking only that
Wworks underfeedbackwould pass even with a completely dead profile switch — which is precisely what a broken live binding would look like. All five existing harnesses green.Codex preflight: NO ISSUES.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests