refactor(editor): move the song-creation flow to src/create.js (R2, step 22) - #173
Conversation
…tep 22) src/main.js 12,738 -> 10,380. Twenty-third module; the graph stays acyclic. main.js is now 51% of the 21,176 lines this refactor started from. 2,380 lines: the format picker, the sloppak-create modal, the roster, the MusicBrainz metadata match, the album-art picker, and the `createState` object every step of the flow reads. They travel together because they ARE one flow over one object. main.js keeps the load/audio pipeline (loadCDLC, loadAudio), the library rescan and the transport readouts — six new host hooks — plus the entry landing, which is screen-entry UI that merely opens two of these dialogs. The 22 window.editor* handlers the HTML calls are exported as plain functions and re-attached by main.js. The module's one import-time side effect, a global 'input' listener, became an exported initCreate() called from init(): a module must do no work when it is loaded, or its tests cannot import it without a DOM. FOUND A REAL BUG, and did not fix it here. main.js contained TWO `async function _editorDoBlankCreate` definitions. Inside the IIFE that is legal: function declarations hoist, and the LAST one in source order silently wins. So the version added by the Create-New redesign (#45, Jul 5) never ran. The one that has been executing came from "restore audio-only project creation" (3f66bec, Jul 4), and the two differ in behaviour: running (kept): artist REQUIRED, audio REQUIRED, no roster validation dead (removed): audio optional (draft-now, audio-later), roster validated The collision only surfaced because create.js is a module, where a duplicate declaration is a SyntaxError rather than a silent overwrite. This PR preserves runtime behaviour exactly: the dead definition is deleted, NOT promoted. Restoring the redesign's intent is a product decision and belongs in its own PR. A comment in create.js records all of this at the call site. Verified beyond the unit tests, which cannot see the wiring: verify_create.py opens the modal and types a title. That one keystroke exercises all three things that can only fail in a browser — the window.* re-attach, initCreate(), and host.addGlobalListener — because nothing else is listening. Comment out initCreate() and all 89 unit tests still pass while the Create button never un-greys. node --test 89/89, pytest 248/248, npm run lint 0 errors (6 warnings), Codex clean, all 15 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe create-gate test now imports ChangesCreate flow updates
Host callback registry
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 `@CHANGELOG.md`:
- Around line 27-30: Correct the CHANGELOG entry to reflect that reducing
main.js from 12,738 to 10,380 lines leaves it at approximately 81.5% of its
original size, representing an approximately 18.5% reduction rather than 51%.
🪄 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: 9cbb2783-725c-4602-95ec-90b52e8a0eb0
📒 Files selected for processing (5)
CHANGELOG.mdsrc/create.jssrc/host.jssrc/main.jstests/create_gate.test.mjs
CodeRabbit, on #173. The sentence said main.js is '51% of what it was', which reads as 51% of the previous step's 12,738 and is wrong either way. 10,380 / 21,176 = 49% of the ORIGINAL, i.e. a 51% reduction from where this refactor started. Against the previous step it is 81%. Both numbers are now stated explicitly rather than left to the reader. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The wording was wrong, though not by the arithmetic you did — and that ambiguity is exactly the problem, so it is fixed. The
Your 81.5% is the step-over-step figure, and the sentence as written ("51% of what it was") reads as though it meant that. Both numbers are now stated explicitly instead of leaving the baseline to the reader. |
There was a problem hiding this comment.
Pull request overview
This PR continues the editor refactor by extracting the entire song-creation flow out of src/main.js into a new src/create.js module, while preserving the existing runtime behavior and keeping Node-importability for unit tests (no import-time DOM side effects).
Changes:
- Moved the “New…” / create-modal flow (format picker, modal state, roster, MusicBrainz match, album art picker, build/import plumbing) into
src/create.js, and re-attached thewindow.editor*handlers frommain.js. - Expanded the
hosthook surface to include the load/audio pipeline and a teardown-tracked global listener registration, enablingcreate.jsto call back intomain.jswithout cycles. - Updated the create-gate unit test to directly import
_createGateOpenfromsrc/create.js(no longer brace-extracting frommain.js).
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/create_gate.test.mjs | Switches to ESM and imports _createGateOpen directly from src/create.js. |
| src/main.js | Imports create-flow exports, wires new host hooks, and re-attaches window.editor* handlers; calls initCreate() during init(). |
| src/host.js | Adds new host hook placeholders for create/load/audio pipeline and global listener registration. |
| src/create.js | New module containing the extracted create flow and exported handlers/state. |
| CHANGELOG.md | Documents the extraction and the historical duplicate _editorDoBlankCreate definition. |
Comments suppressed due to low confidence (1)
tests/create_gate.test.mjs:7
- The header comment says from-scratch draft create “needs only a title”, but
_createGateOpen(and the tests below) require a title and at least one instrument in the roster (default roster contains Lead, which makes it feel title-only). This comment is likely to mislead future edits to the gate.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…te UI Copilot, on #173. Three findings, all pre-existing code this move surfaced, all from the same commit. - initCreate()'s comment claimed the input listener invalidates a cached upload URL. It does not, and never did here: that happens in the audio file/URL change handlers, which clear createState.audioUrl directly. Comment corrected. - _populateCreateArrButtons and _populateStringCountButtons are UNREACHABLE. The first is called only by itself (a re-render on click); the second only by the first. Nothing outside the pair enters them, which is why lint has warned about the first for weeks. - They read and write createState.initialArr. Nothing else does — the create payload sends createState.initialArrangement. Were this UI ever wired up, the arrangement the user picked would not reach the server. Not renamed. Making dead code consistent would change the behaviour of code nobody has decided to run, and the right field name is a product question. Both functions arrived with the Create-New redesign (977ec65, #45) — the same commit whose _editorDoBlankCreate never ran either, for an unrelated reason. That redesign is half-wired. Recorded at the call site; left exactly as found. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
All three are real, all three are pre-existing, and they turn out to be the same finding.
I have not renamed the field. Making dead code self-consistent would change the behaviour of code nobody has decided to run, and which name is correct is a product question, not a refactor's. Both functions are left exactly as found, with the whole story recorded at the call site. The |
…nal (#174) Typing a title enabled the Create button. Clicking it said "Artist is required." Supplying an artist then said "Audio is required for an audio-only project", which made the advertised draft-now-audio-later flow impossible. The roster chips were ignored — every draft came out as a lone Lead arrangement — and the eight extended-metadata fields the modal collects were never sent. Root cause is the duplicate `_editorDoBlankCreate` found while extracting src/create.js (#173): main.js defined it twice, function declarations hoist, and the LAST one in source order silently won. So PR #45's redesign never ran, and the older definition kept sending the server's documented BACK-COMPAT payload (`initial_arrangement` + `init_drum_tab`) instead of the roster it asks for. Everything else already agreed, which is what makes this unambiguous rather than a product call: _createGateOpen enables Create on a title + one instrument (and is unit-tested) screen.html marks only Title "(required)" editorDoCreate comments, right above the call: "only a title is required; audio + artist are optional" create_sloppak "title required"; "Artist is OPTIONAL for a draft"; "Draft-now, audio-later: audio is OPTIONAL"; and it calls initial_arrangement/init_drum_tab the "Legacy shape" Only the handler disagreed. It now validates a title and a non-empty instrument roster, treats artist and audio as optional, and sends `arrangements` plus the spec-complete metadata via the same _createExtendedMeta() helper the Guitar Pro and EOF paths already use. Kept from the old function: the art-upload retry (art normally uploads on selection; this covers a selection whose upload failed). Added: the roster check re-enables the button on failure, which the redesign's version forgot. Removed createState.initialArrangement and createState.initDrumTab — nothing reads them now. The separate, older editorShowCreateSloppakModal dialog still sends the legacy shape with its own drum checkbox, and the server still accepts it; untouched. Codex flagged one apparent regression: that the old payload defaulted init_drum_tab to true, so a default create used to seed Drums as well. It reads that way, but the line below the default overwrites it with a lookup for `#editor-create-drum-tab` — an element the same redesign deleted from screen.html. Verified against the live DOM: the element does not exist, the expression is false, and the server appended nothing. The default roster was ['Lead'] before this change and is ['Lead'] after it. verify_blank_create.py drives the real modal and reads the real POST body: a title alone POSTs, no artist and no audio are sent or demanded, the roster chips arrive as `arrangements`, no back-compat keys are sent, and the extended fields travel. It fails on 6 of 8 checks against the pre-fix code. End to end the server writes an audio-less draft (`stems: []`) with arrangements/lead.json, arrangements/drums.json and drum_tab.json when Drums is chosen. node --test 89/89, pytest 248/248, npm run lint 0 errors, all 16 headless harnesses pass. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Twenty-third module.
src/main.js12,738 → 10,380 — now 51% of the 21,176 lines this refactor started from.2,380 lines: the format picker, the sloppak-create modal, the roster, the MusicBrainz match, the album-art picker, and the
createStateobject every step reads. They travel together because they are one flow over one object.Found a real bug — and deliberately did not fix it here
main.jscontained twoasync function _editorDoBlankCreatedefinitions.Inside the file’s IIFE that is legal: function declarations hoist, and the last one in source order silently wins. So the version added by the Create-New redesign (#45, Jul 5) never ran. The one that has actually been executing came from “restore audio-only project creation” (
3f66bec, Jul 4) — and the two differ in behaviour:The collision surfaced only because
create.jsis a module, where a duplicate declaration is aSyntaxErrorrather than a silent overwrite. It has been sitting inmain.jssince 5 July.This PR preserves runtime behaviour exactly: the dead definition is deleted, not promoted. Restoring the redesign’s intent is a product decision — audio optional, roster validated — and belongs in its own PR. A comment in
create.jsrecords the whole story at the call site.@ChrisBeWithYou, this is your #45 work that never took effect.
Structure
main.jskeeps the load/audio pipeline (loadCDLC,loadAudio), the library rescan and the transport readouts — six new host hooks — plus the entry landing, which is screen-entry UI that merely opens two of these dialogs.The 22
window.editor*handlers the HTML calls are exported as plain functions and re-attached bymain.js. The module’s one import-time side effect — a globalinputlistener — became an exportedinitCreate()called frominit(). A module must do no work when it is loaded, or its tests cannot import it without a DOM.Why a headless harness, again
verify_create.pyopens the modal and types a title. That single keystroke exercises all three things that can only fail in a browser, because nothing else is listening:initCreate()commented outwindow.editor*handler is boundComment out
initCreate()and all 89 unit tests still pass while the Create button never un-greys.Verification
node --test89/89 ·pytest248/248 ·npm run lint0 errors (6 warnings) · Codex clean — it independently confirmed which_editorDoBlankCreatewas the runtime binding, that the arrow→function conversions rely on nothis/arguments, and thatcreateState’s live binding tracks reassignment whilemain.jslegally mutates.audioUrl· all 15 headless harnesses pass.🤖 Generated with Claude Code
Summary by CodeRabbit