feat(editor): Create New Arrangement redesign (roster, staged import, MusicBrainz match, auto-preview, folder-browser load) - #45
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR unifies the editor create flow around Blank, Guitar Pro, and EOF XML modes, adds audio-less draft support, expands browse/import/metadata handling, auto-generates previews server-side, and updates the create-gate test coverage and related labels. ChangesUnified create flow
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ChrisBeWithYou
changed the title
ChrisBeWithYou
marked this pull request as ready for review
July 4, 2026 10:28
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
screen.js (1)
6742-6807: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead create-arr/string-count helpers
_populateCreateArrButtonsand_populateStringCountButtonsno longer have call sites, and the oldeditor-create-*DOM hooks are gone.editorShowCreateModalnow uses the roster palette, so these helpers and theirinitialArr/stringCountstate can be deleted.🤖 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 `@screen.js` around lines 6742 - 6807, Remove the now-dead create-arr/string-count UI helpers and related state from screen.js: `_populateCreateArrButtons` and `_populateStringCountButtons` are no longer referenced, and their `createState.initialArr` / `createState.stringCount` logic is obsolete. Update the create-modal flow in `editorShowCreateModal` and any `createState` initialization to rely only on the roster palette, and delete the unused `editor-create-*` DOM wiring these helpers target.
🤖 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 10-28: The changelog entry is stale and still describes an
intermediate Blank/GP/EOF mode-picker flow that was later removed. Update the
`CHANGELOG.md` “Fixed” note to match the final unified create modal and
`editorDoCreate()` behavior: all options are shown in one menu, routing is based
on the provided inputs, `_createGateOpen` uses the full 5-instrument roster
(Lead/Rhythm/Keys/Bass/Drums), and the gate does not require an artist field.
Also remove the outdated “tracked separately” caveat for Keys/Drums and mention
the start landing page if relevant to the shipped flow.
In `@routes.py`:
- Around line 4195-4199: `replace_audio` is calling `_make_preview_clip()`
synchronously, which can block the event loop while `ffprobe`/`ffmpeg` run.
Update the preview regeneration path in `replace_audio` to offload
`_make_preview_clip` via `run_in_executor()` the same way the upload flow does,
then keep the existing `_pv.exists()` and manifest update logic on the awaited
result.
---
Nitpick comments:
In `@screen.js`:
- Around line 6742-6807: Remove the now-dead create-arr/string-count UI helpers
and related state from screen.js: `_populateCreateArrButtons` and
`_populateStringCountButtons` are no longer referenced, and their
`createState.initialArr` / `createState.stringCount` logic is obsolete. Update
the create-modal flow in `editorShowCreateModal` and any `createState`
initialization to rely only on the roster palette, and delete the unused
`editor-create-*` DOM wiring these helpers target.
🪄 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: 81d09431-48aa-47a5-96c0-f06be5882481
📒 Files selected for processing (5)
CHANGELOG.mdroutes.pyscreen.htmlscreen.jstests/create_gate.test.js
Comment on lines
+4195
to
+4199
| # Regenerate the auto preview from the new master audio so a draft | ||
| # that had no audio at create (and thus no preview) gets one now. | ||
| _pv = _make_preview_clip(dest, source_dir) | ||
| if _pv and _pv.exists(): | ||
| manifest["preview"] = _pv.name |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and surrounding symbols first.
ast-grep outline routes.py --view expanded | sed -n '1,220p'
# Find the helper and the two audio paths.
rg -n "_make_preview_clip|replace_audio|upload_audio|run_in_executor|subprocess\.run" routes.py
# Read the relevant section around the reported lines.
sed -n '4140,4235p' routes.py
# Read the helper implementation if it is in the same file.
sed -n '1,260p' routes.py | sed -n '/def _make_preview_clip/,/^\s*def /p'Repository: got-feedBack/feedBack-plugin-editor
Length of output: 10969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the upload path that already offloads preview generation.
sed -n '3920,3975p' routes.py
# Inspect the helper implementation and its subprocess calls.
sed -n '5840,5905p' routes.py
# Check whether preview is conditionally preserved anywhere else in the file.
rg -n 'manifest\["preview"\]|preview"\s*=' routes.pyRepository: got-feedBack/feedBack-plugin-editor
Length of output: 5584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the save path that also writes manifest["preview"].
sed -n '6110,6165p' routes.py
# Search for any user-facing/manual preview upload or preservation logic.
rg -n "manual.*preview|Preview Clip|preview_path|manifest\\[\"preview\"\\]|preview_clip" routes.pyRepository: got-feedBack/feedBack-plugin-editor
Length of output: 4040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the endpoint that returns a preview_path.
sed -n '4018,4045p' routes.py
# Inspect the replace-audio request body handling around preview_path/audio_url.
sed -n '5758,5818p' routes.py
# Read any nearby comments that explain preview semantics.
sed -n '5848,5908p' routes.pyRepository: got-feedBack/feedBack-plugin-editor
Length of output: 7187
Offload preview generation in replace_audio
routes.py:4197 calls _make_preview_clip() directly; it shells out to ffprobe and ffmpeg, so this async handler can block the event loop for up to ~75s. Run it through run_in_executor() like the upload path.
🤖 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 `@routes.py` around lines 4195 - 4199, `replace_audio` is calling
`_make_preview_clip()` synchronously, which can block the event loop while
`ffprobe`/`ffmpeg` run. Update the preview regeneration path in `replace_audio`
to offload `_make_preview_clip` via `run_in_executor()` the same way the upload
flow does, then keep the existing `_pv.exists()` and manifest update logic on
the awaited result.
Source: Linters/SAST tools
There was a problem hiding this comment.
🧹 Nitpick comments (3)
routes.py (1)
4046-4057: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching "no cover" results to avoid repeated external fetches.
_caa_cachedonly persists successful fetches; a release with no art returnsNonewithout recording that, so every re-render of the cover grid re-hitscoverartarchive.org(10s timeout per tile) for the same art-less MBIDs. A small negative marker (e.g. an empty sentinel file or an in-memory set of known-empty release ids) would short-circuit repeat lookups.Note: the Ruff
S310/ast-grep SSRF hints on_caa_fetch_frontare false positives here —release_idis constrained by_CAA_ID_REat both callers, so no scheme/host injection is possible.🤖 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 `@routes.py` around lines 4046 - 4057, The _caa_cached helper only caches successful cover fetches, so repeated requests for art-less releases keep calling _caa_fetch_front and re-hitting coverartarchive.org. Update _caa_cached to remember negative results too, using a small sentinel/marker for the release_id so later calls can return None immediately without another network fetch; keep the existing success-path cache behavior intact and use the same _caa_cached/_caa_fetch_front symbols to locate the change.Source: Linters/SAST tools
screen.js (2)
6853-6869: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSurface pick failures to the user.
On
!resp.ok || !data || !data.art_paththe function returns silently (Line 6860) leaving the popup open with no feedback, while a thrown error falls through and does close the popup (Line 6868). Since the tile only renders when its image already loaded, a faileduse-caa-coveris unexpected and worth signaling. Consider a brief inline error and consistent popup handling across both failure paths.🤖 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 `@screen.js` around lines 6853 - 6869, The _editorPickCaaCover flow in screen.js silently returns on failed fetch/invalid response while leaving the popup open, unlike the thrown-error path that closes it. Update _editorPickCaaCover to surface a brief inline error or visible feedback when !resp.ok || !data || !data.art_path, and make popup cleanup in editor-art-popup consistent across both success and failure paths so users aren’t left without any indication of the failure.
6803-6830: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard against stale/overlapping searches.
_editorArtRunSearchisasyncand can be triggered repeatedly (Search click + Enter on either input). Because the render at Line 6816 happens afterawait, a slower earlier search can resolve after a newer one and wipe/replace its results, showing stale covers. Consider tagging each run and bailing if a newer run started.♻️ Example guard
async function _editorArtRunSearch(artist, title, grid) { grid.replaceChildren(); if (!artist && !title) { _editorArtMsg(grid, 'Enter an artist or title.'); return; } _editorArtMsg(grid, 'Searching for covers…'); + const token = (grid._artRunToken = (grid._artRunToken || 0) + 1); const params = new URLSearchParams(); ... } catch (_) { data = null; } + if (grid._artRunToken !== token) return; // a newer search superseded this one grid.replaceChildren();🤖 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 `@screen.js` around lines 6803 - 6830, _editorArtRunSearch can render stale results because overlapping async searches may resolve out of order and overwrite newer output. Add a per-run guard in _editorArtRunSearch (for example, a monotonic request token stored outside the function) and check it again after the fetch/await before calling grid.replaceChildren() and appending tiles. If a newer search has started, exit early so only the latest artist/title query updates the grid.
🤖 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 `@routes.py`:
- Around line 4046-4057: The _caa_cached helper only caches successful cover
fetches, so repeated requests for art-less releases keep calling
_caa_fetch_front and re-hitting coverartarchive.org. Update _caa_cached to
remember negative results too, using a small sentinel/marker for the release_id
so later calls can return None immediately without another network fetch; keep
the existing success-path cache behavior intact and use the same
_caa_cached/_caa_fetch_front symbols to locate the change.
In `@screen.js`:
- Around line 6853-6869: The _editorPickCaaCover flow in screen.js silently
returns on failed fetch/invalid response while leaving the popup open, unlike
the thrown-error path that closes it. Update _editorPickCaaCover to surface a
brief inline error or visible feedback when !resp.ok || !data || !data.art_path,
and make popup cleanup in editor-art-popup consistent across both success and
failure paths so users aren’t left without any indication of the failure.
- Around line 6803-6830: _editorArtRunSearch can render stale results because
overlapping async searches may resolve out of order and overwrite newer output.
Add a per-run guard in _editorArtRunSearch (for example, a monotonic request
token stored outside the function) and check it again after the fetch/await
before calling grid.replaceChildren() and appending tiles. If a newer search has
started, exit early so only the latest artist/title query updates the grid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebb27b43-5c82-4ba2-a140-1d4f3a024ea6
📒 Files selected for processing (3)
routes.pyscreen.htmlscreen.js
🚧 Files skipped from review as they are similar to previous changes (1)
- screen.html
There was a problem hiding this comment.
🧹 Nitpick comments (1)
routes.py (1)
4088-4142: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMusicBrainz rate limiting on
cover-search._mb_release_group_covershitsmusicbrainz.org/ws/2/release-groupon every call with no throttle. MusicBrainz enforces roughly one request per second per client and a descriptive User-Agent; a burst of keystroke-driven searches can draw503s or a temporary IP block. Consider debouncing on the client, or a small server-side cache/rate-limiter on this route (CAA fetches are already cached underSTORAGE_DIR, but the release-group search is not).Note: the Ruff
S310/ast-grep SSRF hint at Lines 4112-4113 is a false positive here — the request host is the hardcodedmusicbrainz.org; only the query string is derived from input, so the endpoint isn't attacker-redirectable.🤖 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 `@routes.py` around lines 4088 - 4142, The `_mb_release_group_covers` helper and `/api/plugins/editor/cover-search` endpoint call MusicBrainz on every request without any throttle or caching, which can trigger rate limits during rapid searches. Add a small server-side cache and/or rate limiter around `_mb_release_group_covers` (or the `cover_search` route) so repeated artist/query lookups reuse recent results instead of always hitting `musicbrainz.org`. Keep the hardcoded MusicBrainz host and `_CAA_UA` usage as-is; just reduce request frequency for the `cover_search` flow.Source: Linters/SAST tools
🤖 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 `@routes.py`:
- Around line 4088-4142: The `_mb_release_group_covers` helper and
`/api/plugins/editor/cover-search` endpoint call MusicBrainz on every request
without any throttle or caching, which can trigger rate limits during rapid
searches. Add a small server-side cache and/or rate limiter around
`_mb_release_group_covers` (or the `cover_search` route) so repeated
artist/query lookups reuse recent results instead of always hitting
`musicbrainz.org`. Keep the hardcoded MusicBrainz host and `_CAA_UA` usage
as-is; just reduce request frequency for the `cover_search` flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ded9c9ca-3343-4b62-8102-f92cb5d300b5
📒 Files selected for processing (2)
routes.pyscreen.js
🚧 Files skipped from review as they are similar to previous changes (1)
- screen.js
Collaborator
|
Review + fix pass (Claude Code). Deep-reviewed the create-flow rework; 1. YouTube URL not autosynced on GP import ( 2. Legacy Two findings left for you (not auto-fixed):
|
Collaborator
|
Follow-up: implemented the import-mode metadata persistence I'd flagged (was the larger of the deferred items, but turned out contained). All editor tests green (180 pytest + JS). What was wrong: the create modal collects album_artist / track / disc / genres / language / ISRC / MBID / authors for every flow, but only blank-create wrote them — a GP or EOF import kept just title/artist/album/year on the session, silently dropping the rest. What made it tractable: Fix (both import paths → session.metadata → Build):
|
Contributor
Author
|
Integration note — from a full-queue dry-run that merged every open org PR into a next-version test build.
Fix on merge: delete the old |
The create modal labelled its Guitar Pro / EOF XML inputs "optional" but hard-disabled Create unless one was picked, and its blank-chart options (initial arrangement + drum tab) were never wired up -- so an audio-only feedpak could only be made via a separate "New Sloppak" dialog. Add an explicit Blank / Guitar Pro / EOF XML mode picker (Blank default): - Blank shows the initial-arrangement toggle (Lead/Rhythm/Bass) + a drum-tab option, enables Create on audio + title + artist, routes to the existing create_sloppak backend, and opens the new feedpak in the editor. - GP / EOF keep their import flows; only the active mode's section shows. - The "New" chooser's entries now open this one unified modal (Blank / GP mode) instead of a parallel dialog. - Rename the toolbar's "Build Song" button to "Build feedpak". The Create-button enable logic is factored into a pure _createGateOpen() covered by tests/create_gate.test.js (per-mode enable). screen.js parses clean; all 13 JS suites pass. Deferred (backend work / follow-ups): Keys/Drums-as-arrangement + extended tunings; removing the now-unreferenced standalone New-Sloppak dialog; the editor-header trim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR-A of the onboarding charrette train. create_sloppak hard-capped the
initial arrangement's tuning to length 4|6, which feedpak-spec 5.2
forbids ("Readers MUST NOT hard-code length 6") -- so 7/8-string guitars
and 5/6-string basses could only be made via the save-as path, not at
the source, and imported extended-range tunings risked coercion.
Backend: range-check tuning length 4-8 (default 4 for Bass, 6 otherwise);
the writer already derives string count from the tuning array length.
Frontend: a Strings picker in Blank mode (guitar 6/7/8, bass 4/5/6) that
sends a standard tuning array of the chosen length.
Verified against the running backend: create_sloppak now accepts a
7-string Lead and 5-string Bass tuning (previously 400) and rejects
out-of-range (9) with the spec-cited error. screen.js parses; all
editor JS suites pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extends the create roster beyond fretted roles. Keys and Drums are name-recognized by the editor (KEYS_PATTERN / ^drums), so a "Keys" arrangement opens in piano-roll and a "Drums" arrangement in drum mode. Backend (create_sloppak): accept initial_arrangement Keys/Drums; branch the tuning/capo validation to fretted only (Keys/Drums carry no strings); Keys arrangements get the spec `type: piano`; a Drums pack always seeds its drum_tab. Verified against the running backend -- Keys/Drums accepted, Vocals rejected, extended-range fretted unchanged. Frontend: full roster (Lead/Rhythm, Bass, Keys, Drums) with the string- count picker hidden for Keys/Drums; Vocals shown but DISABLED with an honest tooltip -- the editor has no vocals edit mode yet, so offering it would create a pack you can't edit. Bass string-default bug: switching to Bass kept the guitar default of 6 (6 is a valid bass count, so the keep-if-valid reset didn't fire) -- now resets to the role default (Bass 4, guitar 6). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A ground-up rework of the "New…" create flow (was "Create New custom song"), built + tested iteratively on the :8000 testbed. Onboarding / entry - Entering the Song Editor with nothing loaded shows a Load / Create landing. - Load is now a file browser rooted at the DLC/library folder (new backend POST /browse, single directory level, containment-guarded) with recursive search; opens on the folder instead of a blank "type to search" prompt. Create New Arrangement modal (two-column, wider) - "What are you arranging?" — click-to-add + drag-to-reorder instrument roster (Vocals / Lead / Rhythm / Keys / Bass / Drums); no string counts (set in the editor). Multi-arrangement create; Vocals seeds a lyrics side-file. - Draft-now, audio-later: create with just a title (audio + artist optional); backend writes an audio-less draft the editor can open. - Content Import — one staged, role-capped file list (1 master audio, 1 chart, MIDI info row) that no longer drops one file when you add another; Guitar Pro audio auto-syncs to a staged master track in one click. - Spec-complete manifest metadata (album_artist, track, disc, genres, ISRC, MBID, language, authors) + non-destructive GP-metadata autofill. - MusicBrainz "Match" popup: structured Artist+Title query (no more junk single-field search), and re-ranks candidates by how close each length is to the staged master audio (studio vs live/extended) with a "≈ your audio" tag. - Album art promoted to a preview; preview clip auto-generated from the master audio (manual upload removed). - GP track picker: drums recolored from alarming red to a neutral role pill (Drums amber / Keys indigo / Bass sky / Guitar muted) — it's a role, not a warning. Backend (routes.py): create_sloppak multi-arrangement + full metadata + audio-less drafts + lyrics/drum seeds; upload-audio returns duration; auto preview generation (create + replace-audio); folder-browse endpoint. Tests: create_gate.test.js updated; full editor JS suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cover art lives in the Cover Art Archive (CAA), keyed by RELEASE MBID — which
the create modal's MusicBrainz "Match" already carries (candidate.release_id).
A "Find cover…" button next to the album-art file input opens a Plex-style grid
of covers for the song's releases; click one to bake it into the pack.
- routes.py: GET /caa-cover/{release_id} (server-side CAA front-500 fetch,
cached under STORAGE_DIR, FileResponse; 404 when a release has no art so the
grid can hide the tile — dodges browser CORS and gives us the bytes) and
POST /use-caa-cover → an art_path create_sloppak bakes like an upload.
release_id is regex-validated before URL interpolation.
- screen.*: "Find cover…" button; editorArtSearch popup — prefilled artist/title,
reuses /api/enrichment/search for candidate release MBIDs, one tile per unique
release (same-origin <img>, onerror-hidden), pick → art_path + preview.
Verified on :8000: real covers returned per release, 404s hidden, art_path baked.
The art picker reused the RECORDING search, so it inherited the same "dozens of comp/reissue versions" problem — random covers instead of the canonical album. Album art is a property of the ALBUM, so search MusicBrainz RELEASE-GROUPS, which (unlike recording releases) carry reliable Album vs Live/Compilation typing. - routes.py: GET /cover-search?artist=&query= → release-group search, studio album first (primary Album, no Live/Compilation secondary type), earliest. caa-cover/use-caa-cover gain a release-group mode (?group=1) since CAA also serves release-group front art. - screen.*: the picker's second field is now "Album (or song)", prefilled from the Album field (filled by a MusicBrainz Match) or the title; tiles come from cover-search (canonical first), with the recording-release covers as a fallback for non-title-track songs searched art-first. This fixes "art search shows random covers" and makes art-first work: verified AC/DC "Highway to Hell" returns the 1979 studio album cover as the first tile.
A non-title-track studio recording can sit well down MusicBrainz's flat list (dozens of comp/reissue takes score identically), so a small limit dropped it entirely and the duration re-rank had nothing to promote. Fetch the max (25) and pass the staged audio's duration. Marginal — MB's search order is non-deterministic, so text matching still can't reliably surface the canonical non-title-track recording; audio fingerprinting (AcoustID) is the real fix.
…gerprint) Add an "Identify from audio…" button beside Match: it re-POSTs the staged master audio to core /api/enrichment/identify and fills the exact recording, reusing the MB result rows + apply path (identify returns the same candidate shape). Track the staged File so the bytes can be re-uploaded (core can't read our stored copy). Self-serve: when AcoustID is off (412 needs_setup), the popup expands an inline "enable + paste your key" form (application-key guidance + a register link) that saves to core settings and re-runs — no separate settings screen. A saved-but- rejected key (503) offers "Change AcoustID key" so it never dead-ends. Autofill note reflects the real source (fingerprint vs MusicBrainz).
File master audio couples into the GP autosync path on selection (via _refreshGpAudioUI), but a pasted YouTube URL only resolves at Create time, so gp8AudioMode stayed 'none' and the chart was attached to the downloaded audio UNALIGNED (no autosync run). After resolving the URL, re-derive the GP audio UI and recompute the mode so the autosync path runs — matching file-audio behavior. Embedded-audio GPs are unaffected (that branch is guarded and still deliberately ignores a user URL). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The rework computed init_drums solely from whether the roster contains
'Drums', ignoring the legacy 'init_drum_tab' flag. A back-compat caller sending
{initial_arrangement: 'Lead', init_drum_tab: true} (default was True) no longer
got its drum tab. Translate the legacy flag into the roster (append 'Drums')
so the old single-arrangement + drum-tab contract is preserved; the new
roster-based path is unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The create modal collects album_artist / track / disc / genres / language / isrc / mbid / authors for every flow, but only blank-create wrote them — a Guitar Pro or EOF import silently dropped them (convert-gp / import-xml-project kept only title/artist/album/year on the session). _write_sloppak_pak already writes all these fields from its meta dict and /build merges session.metadata, so the only gap was routing them in. New lenient normalizer _extended_manifest_meta() coerces the modal's raw strings into the shapes the manifest writer expects (track/disc → int, genres/authors → deduped lists, isrc stripped+upper, mbid lower). convert-gp reads them from its JSON body; import-xml-project takes one extended_meta JSON form field; both merge the result into session.metadata so /build persists it. Frontend sends the fields via a shared _createExtendedMeta() helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/blank-project-onboarding
branch
from
July 5, 2026 12:18
51893af to
323e68e
Compare
…boarding # Conflicts: # routes.py # screen.js # tests/create_gate.test.js
This was referenced Jul 5, 2026
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
…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>
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
…tep 22) (#173) * refactor(editor): move the song-creation flow to src/create.js (R2, step 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> * docs(editor): disambiguate the main.js reduction percentage 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> * docs(editor): correct a stale comment and mark #45's unreachable create 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> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
…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>
This was referenced Jul 10, 2026
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
3 participants
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.
Ground-up rework of the "New…" create flow (was "Create New custom song"), built + tested iteratively on the :8000 testbed.
Entry
POST /browse, single directory level, containment-guarded) with recursive search — opens on the folder, not a blank "type to search".Create New Arrangement modal (two-column)
Backend (routes.py)
create_sloppakmulti-arrangement + full metadata + audio-less drafts + lyrics/drum seeds;upload-audioreturns duration; auto preview generation (create + replace-audio); folder-browse endpoint.Tests:
create_gate.test.jsupdated; full editor JS suite green. Draft — still iterating on the testbed.🤖 Generated with Claude Code
Summary by CodeRabbit