fix: resolve enharmonic chord lookups, thread tuning through, serve SPA fallback - #23
Conversation
getChordVariations looked up chords-db by note name via a hand-written keyMap covering only single accidentals. Mode.triads spells keys like Eb minor and Ab minor with double accidentals (Cb, Fb, Bbb, Ebb, F##), so the lookup returned null for 21 of the 588 key/scale triads the UI can produce, and the render sites dereferenced that null. Index by pitch chroma instead, which resolves every enharmonic spelling. Also honour the tuning argument that three call sites were already passing and the function was silently dropping. Fingerings are re-fretted so they sound the same pitches in the target tuning: in Drop D a shape using the open low E now frets it at the 2nd fret. Barres survive only when every string they cover shifts equally; otherwise the strings no longer line up and we render individual dots. Shapes that would fall behind the nut or off the neck are dropped. Tunings without six strings fall back to the standard fingerings rather than emitting wrong shapes - convertToSVGuitarFormat hardcodes a six-string reversal, so seven-string support is a separate change. Adds the full 12 keys x 9 scales sweep as a regression test, plus enharmonic equivalence, pitch preservation across tunings, and a check that the midi we report matches the frets we draw.
Chord.svelte and the chord route both dereferenced getChordVariations
without a guard, so a single unmatched chord blanked the entire page
rather than the one card. Guard both and render a "No diagram available"
placeholder with the play button disabled.
Thread the selected tuning from the page through ScaleChords to Chord so
the tuning selector actually changes the shapes it draws.
Drop the .replace("E#","F").replace("B#","C") rewrite in ScaleChords: it
hand-patched two enharmonic cases, is now redundant, and relabelled
chords away from how the scale spells them.
Encode the chord name in the /chords/:chord link. Sharp chords like C#m
contain a '#', which truncates the URL at the fragment delimiter.
Removes three $effect blocks that existed only to console.log, plus two
more log statements. The one on the main route serialised the whole
fretData object on every selector change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
adapter-static writes an index.html fallback, but nothing on the hosting side was rewriting unmatched paths to it, so /chords/Am and /practice returned a hard 404 on direct load, refresh or share. Client-side navigation masked it - every chord diagram links to a URL that only worked if you were already in the app. Static files still win: Vercel checks the filesystem before rewrites, and _app/ is excluded so missing assets 404 properly instead of being served index.html. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1651071b74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bind:this={el} | ||
| data-chord={chordName} | ||
| href="/chords/{chordName}" | ||
| href="/chords/{encodeURIComponent(chordName)}" |
There was a problem hiding this comment.
When a non-standard tuning such as Drop D is selected, the main page now renders and plays retuned chord shapes, but this link omits the tuning query parameter. The chord route consequently defaults back to Standard, so clicking a displayed Drop D chord immediately shows different diagrams and playback; carry the selected tuning through the link.
Useful? React with 👍 / 👎.
| ? result.positions | ||
| .map((pos) => retunePosition(pos, offsets, tuning)) | ||
| .filter((pos) => pos !== null) | ||
| : result.positions; |
There was a problem hiding this comment.
When the user selects the available B Standard tuning, tuningOffsets returns null because it has seven strings, so this fallback passes the raw six-string database positions through the hard-coded 6 - i conversion. Chord.svelte nevertheless configures the chart with strings: tuning.length (7), leaving the seventh string unmapped and associating the shape and MIDI with standard tuning rather than the displayed B Standard tuning. Return an unsupported state or consistently render a six-string standard-tuning chart instead of mixing the two representations.
Useful? React with 👍 / 👎.
| // Choose a display window: sit at the nut when the shape fits there, | ||
| // otherwise start at the lowest fretted note. | ||
| const fretted = absolute.filter((fret) => fret > 0); | ||
| const highest = fretted.length ? Math.max(...fretted) : 0; | ||
| const lowest = fretted.length ? Math.min(...fretted) : 0; | ||
| const newBaseFret = highest <= 4 || !fretted.length ? 1 : lowest; |
There was a problem hiding this comment.
For alternate-tuning shapes whose absolute fret span exceeds four frets, choosing the lowest note as newBaseFret can still produce relative frets greater than 4, while Chord.svelte always configures a four-fret diagram. For example, the common open G shape retuned to Drop C spans absolute frets 2–7 and therefore emits a finger at relative fret 6, outside the rendered neck. Either discard such positions, choose/expand an adequate display window, or pass the required fret count to the chart.
Useful? React with 👍 / 👎.
…links Addresses review feedback on #23. Retuning could stretch a shape past the four frets Chord.svelte draws, so fingers rendered off the end of the neck. Uneven string shifts pull a grip apart, and a voicing that leaned on an open string high up the neck has to fret it once the tuning drops - chords-db's open G spans frets 2-7 in Drop C, and a C sus voicing spans 1-9 in Half Step Down. Those are unplayable as a single grip rather than merely badly framed, so drop them the way shapes running off the end of the neck are already dropped. Standard tuning is unaffected (0 of 2818 positions). Half Step Down loses 8.3%, Drop D 7.9%, Drop C 15.5% - all redundant alternates, leaving no chord in any tuning without a diagram. FRET_WINDOW is exported and Chord.svelte takes its frets default from it, so the window the chart draws cannot drift from the one shapes are fitted into. Chord links also dropped the selected tuning, so clicking a Drop D chord landed on the chord page in Standard and drew different shapes than the ones just played. Thread the tuning name through ScaleChords to Chord and encode it into the href. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
getChordFingeringslooked up chords-db through a hand-written name→key map covering only single accidentals.Mode.triadsspells keys like Eb minor and Ab minor with double accidentals (Cb,Fb,Bbb,Ebb,F##), so the lookup returnednullfor 21 of the 588 key/scale triads the UI can produce — and both render sites dereferenced that null, blanking the entire page rather than the one card. Reproduced live on production atfretwise.io/?key=Eb&scale=minor.Changes
fix(chords)— index chords-db by pitch chroma instead of note name, so every enharmonic spelling resolves.getChordVariationsalso has arity 2 now; it previously took one argument while three call sites passed a tuning as the second, silently dropped, so the tuning selector did nothing. Shapes are re-fretted to sound the same pitches in the target tuning (Drop D moves an open low E to the 2nd fret). Barres survive only when every string they cover shifts equally. Adds 606 regression tests.fix(ui)— guard both dereference sites and render a "No diagram available" placeholder with the play button disabled, so a single unmatched chord no longer takes the page down. Thread the selected tuning from the page throughScaleChordstoChord. Drop the.replace("E#","F").replace("B#","C")rewrite — it hand-patched two enharmonic cases, is now redundant, and relabelled chords away from how the scale spells them. Encode the chord name in/chords/:chordlinks, since a#truncates the URL at the fragment delimiter. Removes three$effectblocks that existed only toconsole.log; the one on the main route serialised the wholefretDataobject on every selector change.fix(deploy)— addvercel.jsonrewriting unmatched paths to the adapter-staticindex.htmlfallback./chords/Amand/practicereturned a hard 404 on direct load, refresh or share; client-side navigation masked it, so every chord diagram linked to a URL that only worked if you were already in the app. Static files still win — Vercel checks the filesystem before rewrites, and_app/is excluded so missing assets 404 properly.Verification
npm run test:run— 7 files, 701 tests passingnpm run build— clean; the one chunk-size warning (552 kB, chords-db) is pre-existing onmain41ebea0: retuned shapes are now kept inside the diagram window, and chord links carry the selected tuning. Standard tuning drops 0 of 2818 positions; Half Step Down 8.3%, Drop D 7.9%, Drop C 15.5%, all redundant alternates — no chord in any tuning is left without a diagram. Both behaviours are covered by new tests (703 total), verified to fail without the fixKnown gaps, deliberately left
convertToSVGuitarFormathardcodes a six-string reversal (6 - i), andtuningsinsrc/lib/index.jslists B Standard as 7 entries withB2aboveE2, where it should beB1 E2 A2 D3 G3 B3 E4.getChordVariationsdoes fall back to the standard six-string fingerings, butChord.sveltestill configures the chart withstrings: tuning.length, so a 7-string neck gets drawn with 6-string shapes and MIDI belonging to standard tuning. That mixed rendering is left as-is here and wants its own branch.vercel.jsonsetsoutputDirectory: "build"to match adapter-static. This is the one thing that could not be verified without deploying; if Vercel's SvelteKit framework preset overrides it, check that line first.🤖 Generated with Claude Code