chore(editor): add ESLint no-undef (typeof:true) to the src module graph - #159
Merged
Conversation
Closes #158. The module playbook says lint rides in each migrated repo; R2 step 9b produced the evidence for why. TWO BUGS motivated this. Both were green under `node --test` (86/86): 1. MIN_NOTE_W / NOTE_PAD moved to geometry.js and main.js kept using them unimported -> `NOTE_PAD is not defined` on every mousemove. Only the headless browser harnesses caught it. 2. `typeof _coverageEditGen === 'number' ? _coverageEditGen : 0` survived the counter moving to state.js. `typeof` on an undeclared name is legal and yields 'undefined', so the chord-at-cursor and drum-limb-lint memos keyed on a constant 0 and would never have invalidated on an edit. No error, no failing test, and no harness would have caught it either. `no-undef` with `{ typeof: true }` catches both. THE OPTION IS MANDATORY: plain `no-undef` deliberately ignores `typeof x`, which is exactly bug (2)'s shape. Verified against a deliberately-broken checkout — it flags MIN_NOTE_W, NOTE_PAD, editGen and bumpEditGen. The 32 pre-existing violations were all one shape: the editor's own `window.editorX = …` functions called BARE, working only because window properties are implicit globals. They are now `window.editorX(…)`. Patched from eslint's JSON output by line/column — note eslint reports columns in UTF-16 code units, so an emoji earlier on the line shifts them relative to a Python string index. `showScreen` (core's shell) and `alphaTab` (the tab-preview vendor bundle) really are host-provided, and are declared as globals rather than qualified. no-unused-vars is a WARNING (10 today), not an error: a handful of declarations are reachable only from tests that slice them out of the source text, so erroring would mean deleting code the suite covers. Ratchets down as those tests move to real imports — the same shape as core's max-lines warn ratchet. ZERO node_modules. ESLint runs through `npx` in its own `lint` CI job rather than as a devDependency, because feedback-desktop's `bundle-slopsmith.sh` `copy_plugin()` ships external plugins with `cp -R "$src/."` and strips only `.git` — a devDependency here would ship the whole dependency tree into the packaged app. That prune is still owed on the desktop side before anything adds real deps here. Deliberately out of scope: `import-x/no-cycle` needs a plugin, which needs real deps, which needs the desktop prune first. The graph is verified acyclic by hand each step. Verified: `npm run lint` 0 errors / 10 warnings. node --test 86/86, pytest 248/248. All five headless harnesses green. Every one of the 22 qualified names confirmed to be `window.X = ` assigned in main.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds ESLint tooling with a new flat config, CI lint job, npm lint script, and changelog note. Separately, ChangesESLint Setup
main.js window.* call-site updates
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds per-repo ESLint linting for the editor’s src/ ES-module graph (per the module playbook), wiring it into CI and updating main.js to remove reliance on implicit-global window.editorX call sites that no-undef would flag.
Changes:
- Introduces
eslint.config.mjswithno-undef: ['error', { typeof: true }]andno-unused-varsas warnings, plus explicit browser/host globals. - Adds a
lintscript and a dedicatedlintGitHub Actions job that runs ESLint vianpx(keeping the repo free ofnode_modules). - Qualifies previously-bare
editorX()calls aswindow.editorX()insrc/main.jsto eliminateno-undefviolations andtypeofblind spots.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.js | Rewrites bare editorX() calls to window.editorX() to satisfy no-undef and remove implicit-global reliance. |
| package.json | Adds a lint script running ESLint via npx. |
| eslint.config.mjs | Adds flat ESLint config for src/**/*.js with no-undef (typeof:true) and warning-only no-unused-vars. |
| CHANGELOG.md | Documents the new lint gate and motivation in the Unreleased section. |
| .github/workflows/ci.yml | Adds a standalone lint job to run ESLint in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+4
to
+6
| * earns its keep during the R2 split, and it exists because of two bugs that | ||
| * `node --test` (86/86 green) and the headless harnesses BOTH missed: | ||
| * |
Collaborator
Author
There was a problem hiding this comment.
Fixed — you are right, and it mattered. The harnesses did catch the loud NOTE_PAD is not defined bug; only the silent typeof case escaped everything. The entire argument for adding this lint rests on that distinction, so overstating it undercut the case. Header rewritten to say exactly which bug escaped what.
Comment on lines
+13
to
+18
| lint, added because two bugs during the R2 split slipped past `node --test` | ||
| (86/86 green) *and* the headless harnesses: `MIN_NOTE_W`/`NOTE_PAD` used after | ||
| they moved to `geometry.js` without an import, and — silently — | ||
| `typeof _coverageEditGen === 'number' ? _coverageEditGen : 0` surviving the | ||
| counter's move, so two memos keyed on a constant `0` and never invalidated. | ||
| `no-undef` with **`typeof: true`** catches both. That option is off by default: |
Collaborator
Author
There was a problem hiding this comment.
Fixed, same correction as the config header. The harnesses caught bug 1; nothing caught bug 2. CHANGELOG now says so.
Comment on lines
+25
to
+26
| - name: ESLint (src module graph) | ||
| run: npx --yes eslint@9.39.4 src |
Collaborator
Author
There was a problem hiding this comment.
Fixed — the job now runs npm run lint, so package.json is the single owner of the eslint version and args. (The script still shells out to npx, which is what keeps this repo at zero node_modules; I updated the comment above the job so it still describes what actually happens.)
![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 @.github/workflows/ci.yml:
- Around line 18-26: The lint job’s `actions/checkout@v4` step is persisting the
`GITHUB_TOKEN` unnecessarily; update the checkout configuration to disable
credential persistence by setting `persist-credentials: false` on the `checkout`
step in the `lint` job, since `src` linting only requires read access and does
not need git credentials retained.
🪄 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: 57bf861b-6713-421d-9771-b80d2a2923fd
📒 Files selected for processing (5)
.github/workflows/ci.ymlCHANGELOG.mdeslint.config.mjspackage.jsonsrc/main.js
Four findings, all correct. - [Copilot x2] My eslint.config.mjs header and the CHANGELOG both claimed the headless harnesses missed BOTH motivating bugs. They didn't: the harnesses caught the loud `NOTE_PAD is not defined` one. Only the silent typeof case escaped everything. Corrected in both places — the whole argument for this lint rests on that distinction, so getting it wrong undercut it. - [Copilot] ci.yml duplicated the eslint invocation and version. It now runs `npm run lint`; package.json owns the version and args. - [CodeRabbit] The lint job's checkout persisted GITHUB_TOKEN in the local git config. It only reads `src`, and npx pulls a dependency tree, so `persist-credentials: false`. npm run lint 0 errors / 10 warnings; node --test 86/86. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 @.github/workflows/ci.yml:
- Around line 18-32: The lint job still inherits broad default GITHUB_TOKEN
access; add an explicit permissions block on the lint job that grants only the
minimal read access needed for checkout and eslint, and ensure no write scopes
remain. Update the lint job definition in the workflow alongside the existing
checkout/setup-node steps so the job’s permissions are clearly restricted.
🪄 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: 51fe8c22-ea91-4099-a725-b5fc2a8480cf
📒 Files selected for processing (3)
.github/workflows/ci.ymlCHANGELOG.mdeslint.config.mjs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- eslint.config.mjs
Comment on lines
+18
to
+32
| lint: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| # This job only reads `src` to lint it. Don't leave GITHUB_TOKEN in the | ||
| # local git config while npx pulls a dependency tree. | ||
| persist-credentials: false | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 22 | ||
| - name: ESLint (src module graph) | ||
| # `npm run lint` rather than repeating the invocation — package.json owns | ||
| # the eslint version and args. | ||
| run: npm run lint |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add an explicit permissions: block to the lint job.
The lint job has no permissions key, so it inherits the repository's default GITHUB_TOKEN permissions (often broader read/write across scopes). The job only checks out and reads src to run eslint — it needs no write access anywhere. This was previously flagged by zizmor as excessive-permissions for this job alongside the persist-credentials finding; only the latter was addressed.
🔒 Proposed fix
lint:
runs-on: ubuntu-latest
+ permissions:
+ contents: read
steps:
- uses: actions/checkout@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lint: | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| # This job only reads `src` to lint it. Don't leave GITHUB_TOKEN in the | |
| # local git config while npx pulls a dependency tree. | |
| persist-credentials: false | |
| - uses: actions/setup-node@v4 | |
| with: | |
| node-version: 22 | |
| - name: ESLint (src module graph) | |
| # `npm run lint` rather than repeating the invocation — package.json owns | |
| # the eslint version and args. | |
| run: npm run lint | |
| lint: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| # This job only reads `src` to lint it. Don't leave GITHUB_TOKEN in the | |
| # local git config while npx pulls a dependency tree. | |
| persist-credentials: false | |
| - uses: actions/setup-node@v4 | |
| with: | |
| node-version: 22 | |
| - name: ESLint (src module graph) | |
| # `npm run lint` rather than repeating the invocation — package.json owns | |
| # the eslint version and args. | |
| run: npm run lint |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 18-33: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/ci.yml around lines 18 - 32, The lint job still inherits
broad default GITHUB_TOKEN access; add an explicit permissions block on the lint
job that grants only the minimal read access needed for checkout and eslint, and
ensure no write scopes remain. Update the lint job definition in the workflow
alongside the existing checkout/setup-node steps so the job’s permissions are
clearly restricted.
Source: Linters/SAST tools
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
…tep 10) (#160) * refactor(editor): extract hit testing, shortcuts and setStatus (R2, step 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> * fix(editor): sustain edge-drag used the fretted lane band in the piano 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> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Closes #158. The module playbook says lint rides in each migrated repo. Step 9b produced the evidence.
Two bugs motivated this
Both were green under
node --test(86/86).Loud.
MIN_NOTE_W/NOTE_PADmoved togeometry.jsandmain.jskept using them unimported →NOTE_PAD is not definedon every mousemove. Only the headless browser harnesses caught it.Silent. This survived the counter moving to
state.js:typeofon an undeclared name is legal and yields'undefined', so the chord-at-cursor and drum-limb-lint memos would have keyed on a constant0and never invalidated on an edit. No error, no failing test — and no harness would have caught it either.{ typeof: true }is mandatoryPlain
no-undefdeliberately ignorestypeof x, which is exactly bug 2's shape. Verified against a deliberately-broken checkout: with the option on, it flagsMIN_NOTE_W,NOTE_PAD,editGenandbumpEditGen. With it off, it misses the second.The 32 pre-existing violations were all one shape
The editor's own
window.editorX = …functions, called bare — working only because window properties are implicit globals. Nowwindow.editorX(…).Patched from ESLint's JSON output by line/column. Worth knowing: ESLint reports columns in UTF-16 code units, so an emoji earlier on the line (
row('🎵', …)) shifts them relative to a Python string index. My first patcher asserted itself into a clean stop rather than corrupting the file.Every one of the 22 names is confirmed
window.X =assigned inmain.js.showScreen(core's shell) andalphaTab(the tab-preview vendor bundle) really are host-provided, so they're declared as globals rather than qualified.no-unused-varsis a warning, not an error10 today. A few declarations are reachable only from tests that slice them out of the source text (
ResizeSustainCmd,_drumConflictIndexSetPure), so erroring would mean deleting code the suite covers. It ratchets down as those tests move to real imports — the same shape as core'smax-lineswarn ratchet. The others (renderSongPrompt,_makeTimeRemap,_populateCreateArrButtons,_tempoSyncInspectorState) look like genuinely dead code and are worth a separate look.Zero
node_modulesESLint runs through
npxin its ownlintCI job rather than as a devDependency, becausefeedback-desktop'sbundle-slopsmith.shcopy_plugin()ships external plugins withcp -R "$src/."and strips only.git. A devDependency here would ship the whole dependency tree into the packaged app. That prune is still owed on the desktop side before anything adds real deps to this repo.Which is also why
import-x/no-cycleis deliberately out of scope: it needs a plugin, which needs real deps, which needs the desktop prune. The graph stays verified acyclic by hand each step.Verification
npm run lint: 0 errors, 10 warnings.node --test86/86,pytest248/248, all five headless harnesses green.🤖 Generated with Claude Code
Summary by CodeRabbit