Repository navigation
Generate keys-first chord arrangements - #17
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 17 high |
| Security | 2 critical 14 high |
🟢 Metrics 239 complexity · 15 duplication
Metric Results Complexity 239 Duplication 15
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…s_karaoke Adds a third arrangement-generation path alongside chart chords and audio chord detection: generateAccompanimentFromLyrics(filename, options) fetches lyrics_karaoke's canonical /playback payload, takes the primary voice's pitched tokens, and harmonizes the melody into a chord sequence — a Krumhansl-Schmuckler-style key estimate from the melody's pitch-class durations, then a windowed best-fit diatonic triad per chord change — which feeds into the existing generateChordArrangement (keys or guitar). Covers a song whose only harmonic information is its vocal line: no chord chart, no full-mix audio worth running chord detection on. Returns null when there's nothing to harmonize from (route unavailable, lyrics-only track, unrecognized schema_version). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XMb6KvcVj8KxYytezviPkJ
|
Extended this PR's scope with a third generation path: Where
Returns Added This is melody harmonization, not chord detection — it invents a plausible backing, not a recovered progression, and is documented as such. Generated by Claude Code |
Signed-off-by: Carolina <79524656+carochacs@users.noreply.github.com>
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Five issues worth fixing before merge — a duplicated manifest key that silently voids the version bump and the description, a guitar path that emits the wrong chord for 5-string bass arrangements, a duration option that means something other than what the README says, an undocumented wire-format conversion on the keys payload, and a shape search that blocks the browser thread for tens of seconds. The melody harmonizer itself holds up well — I probed it against several ordinary progressions and the chord choices are reasonable.
Reviewed changes
Keys-first arrangement generation and everything downstream of it, across chordr/screen.js, two new test files, and the docs.
- Chord-symbol parsing —
parseChordNameturns text symbols into root/bass/quality, with alias handling (Δ,°,m7♭5) and slash-bass support. Verified all 216 root×quality names the Python detector can emit round-trip correctly. - Keys generation —
generateKeysArrangementpicks a right-hand voicing by centre-pull plus motion from the previous chord, and a left-hand bass note honouring slash chords; register clamps andsusderive from onset spacing. - Guitar generation —
_bestGuitarShapebrute-forces per-string fret combinations against the chord's pitch classes with a span/bass/muted-string score;_normaliseHarmonyEventsand_eventDurationare shared by both paths. - Audio path —
generateArrangementFromAudiowires the existingdetectChordsFromAudiooutput into the generator. - Melody path —
generateAccompanimentFromLyricsfetches lyrics_karaoke's/playback, estimates a key with a Krumhansl-Schmuckler correlation, then picks a best-fit diatonic triad per time window. I checked the lyrics_karaoke contract against itsroutes.py, schema module and contract doc: route path,filename/arrangementparams, integerschema_version: 1,voices[].primary, and optional-miditokens all match. - Docs — README API block plus prose for the three entry points, a CHANGELOG section, and a
plugin.jsonversion/description change.
ℹ️ Nitpicks
CHANGELOG.md: the new## 0.5.0heading is inserted above the "All notable changes to this project will be documented in this file" preamble, so the preamble now reads as part of the 0.5.0 section, and every entry that actually shipped since the last release is still sitting under## [Unreleased]— which is whatCLAUDE.md's Versioning section says to update.CLAUDE.md: the Testing block still claims# 81 passfornode --test tests/*.test.js; with the two new suites it is 95. The README's Tests block was updated in this PR,CLAUDE.md's was not.README.md:parseChordName,generateKeysArrangementandgenerateGuitarArrangementare all exported onwindow.chordrbut absent from thewindow.chordrAPI block — the two instrument backends are the ones a consumer needs to know about.tests/generate_arrangement.test.js:assert.ok(result.notes.length >= 4)— a keys triad is exactly onelhplus threerhnotes, so an exact count is available and>=would let extra notes leak through unnoticed.
big-pickle (free) ｜ 𝕏
|
Addressed all six review threads in Task list (11/14 completed)
|
- plugin.json: fold the merge's duplicate version/description keys into one pair, and bump to 0.6.0 (0.5.0 was a downgrade from main's 0.5.4, and the shadowed key meant the host never saw either edit) - _eventDuration: rename the absolute-end `duration` to `lengthSeconds`, the arrangement's total length, floored by `defaultDuration` so a value below the last onset extends nothing instead of vanishing - _bestGuitarShape: search against the base the host decodes with, so isBass shapes read back as the requested chord rather than a transposition of it; memoize shapes per generation call and prune the fret-product search on the partial score (5.2s -> 0.9ms per chord at 8 strings x 15 frets, with the winning shape unchanged) - README: document the keys payload's MIDI-bucket conversion to the wire note format, what becomes of `hand`, and the three new exports - tests: pin right-hand motion, bass shape decoding and the search cost; assert an exact note count
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Fix these issues in Kilo Cloud Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Previous Review Summaries (2 snapshots, latest commit 743ea63)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 743ea63)Status: 7 Issues Found | Recommendation: Address before merge Fix these issues in Kilo Cloud Overview
All six findings from the previous review round (duplicate Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Verification note: tests were not executed (read-only review); JS/Python pass counts were verified statically against the test files present. The guitar shape-search pruning and memoization were independently re-derived and confirmed score-preserving, and the Krumhansl rotation direction, diatonic triad tables and windowing math in the melody path were verified correct. Previous reviewThis review did not finish. The model reached its output limit before it Reviewed by free · Input: 209.4K · Output: 6K · Cached: 116.6K |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| function parseChordName(value) { | ||
| const text = String(value || "").trim(); | ||
| if (!text || /^(n\.?c\.?|no\s*chord)$/i.test(text)) return null; | ||
| const match = text.match(/^([A-Ga-g])([#b]?)([^/]*)?(?:\/([A-Ga-g])([#b]?))?$/); |
There was a problem hiding this comment.
WARNING: Unicode accidentals (♭/♯) fail this regex, silently dropping the entire chord event
The accidental group is ASCII-only ([#b]?), so B♭, C♯m and C/B♭ all fail the match and parseChordName returns null — _normaliseHarmonyEvents then quietly omits the whole harmony change. The alias map a few lines below already shows the intent to accept Unicode symbols (Δ, °, °7, m7♭5), but the most common Unicode symbols in authored charts are not honored, and README claims the generators work with authored charts. Normalize before the match (♭→b, ♯→# — arguably also ∆/△→Δ and ø→m7b5) to close the gap.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "°": "dim", "°7": "dim7", "+": "aug", | ||
| }; | ||
| suffix = Object.prototype.hasOwnProperty.call(aliases, suffix) ? aliases[suffix] : suffix; | ||
| const intervals = CHORD_INTERVALS_BY_SUFFIX.get(suffix.toLowerCase()); |
There was a problem hiding this comment.
WARNING: Case-sensitive alias check: CM7 resolves through the m7 table — a wrong chord, silently
The aliases map is consulted with exact-case hasOwnProperty, so the jazz shorthand CM7 (major 7) misses the M alias entry, which only covers the bare suffix. The fallback here then lowercases M7 to m7 and returns intervals [0,3,7,10] — a minor-7 voicing — while quality still reports M7, so generation emits the wrong chord with no signal (CM9 → m9, CM6 → m6). Either add the maj-sh aliases (M7 → maj7, M9 → maj9, M6 → 6) or resolve aliases case-insensitively before this lookup. The audio detector's ASCII output round-trips fine; this reaches the generator through authored chordTemplates names and the now-public parseChordName.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (shapeCache && shapeCache.has(cacheKey)) return shapeCache.get(cacheKey); | ||
| const choices = []; | ||
| for (let s = 0; s < stringCount; s++) { | ||
| const open = (base[s] ?? base.at(-1)) + Number(tuning[s] || 0); |
There was a problem hiding this comment.
WARNING: capo is documented as a guitar-shape option but is never folded into the search, so emitted shapes sound transposed when the host decodes with a capo
The search matches pitch classes against base + tuning + f only, but the decode these notes must round-trip through (pitchFromBase, reached via identifyChord) computes base + tuning + capo + fret, and README's generateChordArrangement options list explicitly documents capo for guitar shapes. A caller forwarding a song's { tuning, capo, stringCount } ctx gets frets that decode one capo's worth transposed — the same search-vs-decode divergence the isBass fix in this commit addressed (capo shifts all strings uniformly, but it still changes which frets produce which pitch class). Either fold capo into open/sounding here and into the cache key, or drop capo from the documented option list; the capo-relative framing in this comment's rationale only round-trips when capo is 0.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // memoizes the winner per chord + instrument setup: this search dominates | ||
| // the guitar path's cost, and a song repeats each chord symbol many times. | ||
| function _bestGuitarShape(chord, options, shapeCache) { | ||
| const stringCount = Number(options.stringCount) || 6; |
There was a problem hiding this comment.
SUGGESTION: Default stringCount diverges from the host decoder when only tuning is given
identifyChord (line 178) infers a missing stringCount from tuning.length before falling back to 6; this search defaults straight to 6. A 7-string tuning passed without stringCount is searched against the 6-string base, so the emitted {s,f} notes decode against the 7-string base on read-back and identify as a transposition. Mirroring the decode side's fallback keeps search and decode consistent:
| const stringCount = Number(options.stringCount) || 6; | |
| const stringCount = Number(options.stringCount) || (options.tuning && options.tuning.length) || 6; |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 22c7de5 (stringCount fallback included).
|
|
||
| // Diatonic triads by scale degree (root offset in semitones from tonic, | ||
| // quality suffix matching CHORD_QUALITIES). Degree seven is the | ||
| // half-diminished/diminished triad in both modes' natural form. |
There was a problem hiding this comment.
SUGGESTION: This comment is wrong for the minor mode — natural-minor degree 7 is a major triad, and the table below already encodes it correctly
The comment says degree seven is the half-diminished/diminished triad in both modes' natural form, but in the natural minor scale degree 7 is a major triad on the flattened seventh (VII), which the minor table's offset: 10, suffix: empty-string row correctly encodes; the natural-minor diminished triad is on degree 2. (Half-diminished is also a seventh-chord quality — a triad can only be diminished.) The tables themselves check out; only this sentence misleads.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (!payload || payload.schema_version !== 1 || !Array.isArray(payload.voices)) return null; | ||
| const voice = payload.voices.find((v) => v && v.primary) || payload.voices[0]; | ||
| const tokens = (voice && Array.isArray(voice.tokens) ? voice.tokens : []) | ||
| .filter((t) => Number.isFinite(t.midi) && Number.isFinite(t.start) && Number.isFinite(t.duration) && t.duration >= 0) |
There was a problem hiding this comment.
SUGGESTION: A null/undefined entry in voice.tokens throws here, breaking the documented fail-soft contract
The filter dereferences t.midi/t.start/t.duration unconditionally, while everything around it defends against an untrusted payload: voices are guarded with v && v.primary above, voice && Array.isArray(voice.tokens) wraps this, and fetch/JSON failures are caught below. One null token element — evaluated outside the try/catch — makes generateAccompanimentFromLyrics reject instead of returning null, contradicting the doc comment above and README's returns-null contract. A truthy guard restores it:
| .filter((t) => Number.isFinite(t.midi) && Number.isFinite(t.start) && Number.isFinite(t.duration) && t.duration >= 0) | |
| .filter((t) => t && Number.isFinite(t.midi) && Number.isFinite(t.start) && Number.isFinite(t.duration) && t.duration >= 0) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| to harmonize from: the lyrics_karaoke route is unavailable or 404s, the | ||
| track is lyrics-only (no `midi` on any token), or the response's | ||
| `schema_version` isn't the one this function understands. | ||
| `groupChordEvents` returns a `{ parentIndex, continuation }` entry for every |
There was a problem hiding this comment.
SUGGESTION: Merge collision — the generateAccompanimentFromLyrics paragraph above runs straight into this groupChordEvents paragraph
Line 155 ends with ...isn't the one this function understands. and this line follows with no blank line between them, so rendered Markdown fuses two unrelated API descriptions into one paragraph — a merge-resolution collision from 724fa63 that 743ea63 did not fix. Insert a blank line between the two paragraphs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Important
The new capo support is half-wired: it shifts the fret-candidate search but not the pitch classes the winner is validated against, so generateGuitarArrangement returns an empty arrangement for every chord whenever capo is set. One-token fix, and I confirmed it.
Reviewed changes
Five fix(review) commits since 743ea63 — chord-name aliases, guitar capo support, a comment correction, a null-token guard, and a README paragraph break.
- Chord-name aliases — added
M7/M9/M6toparseChordName's alias table, mapping to themaj7/maj9/6suffixesCHORD_QUALITIESalready carries. Verified:CM7→{0,4,7,11},CM9→{0,2,4,7,11},CM6→{0,4,7,9}, and slash-bass (CM7/Ab) resolves the bass correctly. Purely additive, no existing alias changed. - Guitar
caposupport —_bestGuitarShapenow readsoptions.capo, folds it into each string'sopenpitch, and includes it in theshapeCachekey (which is now complete: every input that affects the result is in it). This is the one that needs the fix below. stringCountinference — now falls back tooptions.tuning.lengthinstead of a hardcoded6. A caller passing an 8-stringtuningwith nostringCountgets the right base table instead of 6 strings against 8 tuning offsets — and it stops generating notes the host'spitchFromBasewould drop as out-of-range.- Minor-mode comment — the degree-7 comment was wrong; the new text is right.
MINOR_DEGREE_TRIADSis natural minor, sodimcorrectly sits on degree 2 and the major triad on degree 7 (VII). - Null-token guard — the token filter now rejects a falsy
tbefore dereferencingt.midi. Verified anullentry invoices[].tokensnow yields an arrangement instead of aTypeError. - README — blank line restored between the
generateAccompanimentFromLyricsparagraph and thegroupChordEventsone.
Everything from the first review is genuinely closed out: the manifest has a single version/description pair at 0.6.0, isBass reaches baseOpenStringMidis, duration is now lengthSeconds, the keys→wire bucket conversion is documented with a worked snippet, the shape search is memoized and pruned (24 chords at 8×15 in single-digit ms), and the voice-leading test now fails if the motion term is removed. Suite is 99 JS tests, all green.
big-pickle (free) ｜ 𝕏
|
Addressed the one open thread from review 5422766473 — the Fix ( Verification
Thread replied to and resolved. No other threads were open on this PR. Task list (7/7 completed)
|
The shape search folded `capo` into each string's open pitch but the leaf
that validates a candidate recomputed `base + tuning + f` without it, so
every non-multiple-of-12 capo made the coverage check reject the shapes
the candidate list had just built and `generateGuitarArrangement` returned
`{notes: [], shapes: []}` silently. Both sites now read one `opens` array,
so the shift cannot be applied in one and forgotten in the other.
| function parseChordName(value) { | ||
| const text = String(value || "").trim(); | ||
| if (!text || /^(n\.?c\.?|no\s*chord)$/i.test(text)) return null; | ||
| const match = text.match(/^([A-Ga-g])([#b]?)([^/]*)?(?:\/([A-Ga-g])([#b]?))?$/); |
There was a problem hiding this comment.
WARNING: Unicode accidentals (♭/♯) fail this regex, so B♭/C♯m/C/B♭ events are silently dropped entirely
The pattern /^([A-Ga-g])([#b]?)([^/]*)?(?:\/([A-Ga-g])([#b]?))?$/ only accepts ASCII # and b. Unicode flat (U+266D) and sharp (U+266F) are not matched, causing the whole chord parse to return null and the event to be discarded.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Summary
Testing