feat(editor): tuning-aware Strings modal — either-end add, direct per-string tuning entry - #112
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Strings modal now supports per-string semitone tuning inputs, explicit low/high add/remove controls, updated end-eligibility logic, and expanded regression coverage for the new tuning and boundary behavior. ChangesStrings modal tuning and directional controls
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)screen.jsast-grep timed out on this file Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/strings_modal.test.js (2)
33-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBrace-counting class extraction is fragile to future syntax.
extractClasscounts{/}naively, so any future string, regex, or template literal insideAddStringCmdcontaining an unbalanced brace would silently mis-extract the class body. Not an issue with current code, just a latent trap for future edits toAddStringCmd.🤖 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 `@tests/strings_modal.test.js` around lines 33 - 43, The class extraction helper is using naive brace counting and can break on future `AddStringCmd` syntax changes. Update `extractClass` to use a more robust parsing approach that ignores braces inside strings, regexes, and template literals, or otherwise constrain it to extract the `AddStringCmd` class body safely without relying on raw character counts. Keep the helper and its use in `strings_modal.test.js` aligned with the `extractClass` and `AddStringCmd` symbols.
108-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest doesn't exercise
_historyEnsureArr's arrangement-switch path.
makeEnvdoesn't provide_historyEnsureArr/window.editorSelectArrangement, sodoUndo'stypeof _historyEnsureArr === 'function'guard is always false here (perEditHistory.doUndoin the provided context snippet). This test verifies thatSetStringTuningCmdtargets its constructor-capturedarrIdxregardless ofS.currentArr— which is real coverage — but it doesn't verify the UI-switch-on-undo behavior the PR description calls out ("undo works after arrangement switches"), since that mechanism is bypassed entirely in this harness.Not a correctness bug in this file, just worth being aware the arrangement-switch UI flow (
_historyEnsureArr) remains untested at this layer.🤖 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 `@tests/strings_modal.test.js` around lines 108 - 122, The undo test currently only covers the captured-arrangement behavior of SetStringTuningCmd and never exercises the arrangement-switch path in EditHistory.doUndo because makeEnv lacks _historyEnsureArr/window.editorSelectArrangement. Update the test harness so doUndo can take the _historyEnsureArr branch, or add a separate test that stubs _historyEnsureArr and verifies the undo action switches to the stored arrangement before applying the command; use EditHistory.doUndo, makeEnv, and SetStringTuningCmd to locate the relevant setup.screen.js (1)
11124-11140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive each tuning input an accessible name.
The
<input>is wrapped by a<label>whose only text is thestunit, and the identifyingString ${i} (${lbl})text lives in a separate, unlinked<span>. Screen-reader users hear the field announced only as “st spinbutton,” with no indication of which string it edits. Add anaria-label(or wire the left span viafor/id).♿ Proposed tweak
input.value = String(off); input.className = 'w-14 bg-dark-700 border border-gray-700 rounded px-1 py-0.5 text-xs text-gray-300 outline-none text-center'; + input.setAttribute('aria-label', `String ${i} (${lbl}) tuning offset in semitones`); input.title = 'Semitones from this lane’s standard pitch (e.g. -2 = whole-step down; a re-entrant drone can sit far above)';🤖 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 11124 - 11140, The tuning input in the string editor lacks an accessible name because the `String ${i} (${lbl})` text is not associated with the `input` created in this block. Update the tuning control in the `screen.js` string-tuning UI by giving the `input` an `aria-label` that includes the string identifier, or by wiring the existing left text span to the input with matching `id`/`for` semantics via the surrounding label markup, so screen readers announce which string the field edits.
🤖 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 `@screen.js`:
- Around line 11124-11140: The tuning input in the string editor lacks an
accessible name because the `String ${i} (${lbl})` text is not associated with
the `input` created in this block. Update the tuning control in the `screen.js`
string-tuning UI by giving the `input` an `aria-label` that includes the string
identifier, or by wiring the existing left text span to the input with matching
`id`/`for` semantics via the surrounding label markup, so screen readers
announce which string the field edits.
In `@tests/strings_modal.test.js`:
- Around line 33-43: The class extraction helper is using naive brace counting
and can break on future `AddStringCmd` syntax changes. Update `extractClass` to
use a more robust parsing approach that ignores braces inside strings, regexes,
and template literals, or otherwise constrain it to extract the `AddStringCmd`
class body safely without relying on raw character counts. Keep the helper and
its use in `strings_modal.test.js` aligned with the `extractClass` and
`AddStringCmd` symbols.
- Around line 108-122: The undo test currently only covers the
captured-arrangement behavior of SetStringTuningCmd and never exercises the
arrangement-switch path in EditHistory.doUndo because makeEnv lacks
_historyEnsureArr/window.editorSelectArrangement. Update the test harness so
doUndo can take the _historyEnsureArr branch, or add a separate test that stubs
_historyEnsureArr and verifies the undo action switches to the stored
arrangement before applying the command; use EditHistory.doUndo, makeEnv, and
SetStringTuningCmd to locate the relevant setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a16ffdaa-6d99-4c6d-a44f-2e1adb6fb835
📒 Files selected for processing (4)
CHANGELOG.mdscreen.htmlscreen.jstests/strings_modal.test.js
49ccf18 to
a3c09a7
Compare
Gate add/remove string ops to the end the pitch/label model can represent. The either-end/guitar-floor-4 design silently re-snapped the string count and re-interpreted note indices (guitar high-add, guitar remove-below-6, high-remove on a low-extended shape, bass high-add at 4). Add/remove now go through _addPositionPure/_removePositionPure; guitar floor restored to 6. Direct per-string tuning entry (SetStringTuningCmd) is unchanged and covers exotic tunings without changing the count. Handler-level regression tests drive the real _stringCountFor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Addressed the a11y nitpick: each per-string tuning input now carries an @coderabbitai review |
|
(=^･ω･^=) ✏️ Learnings added
✅ Action performedReview finished.
|
…-string entry Roadmap 4.18. The command layer (AddStringCmd/RemoveStringCmd) always supported both ends, but the modal hardcoded the position policy (high-side adds unreachable except the one 5->6 bass case), offered no way to edit a string tuning, and the guitar floor of 6 made 5-lane banjo charts unconstructible. - Add low / Add high + Remove low / Remove high buttons — the END is the user choice; removal refuses an end string carrying notes, per end independently (the other end stays removable). - Direct per-string offset entry (semitones from the lane standard pitch, clamped +/-36) through the new undoable SetStringTuningCmd, which captures its target arrangement index so undo survives an arrangement switch and pads short tuning arrays instead of writing holes. Re-entrant setups (banjo high drone 5th) are now typable. - Guitar-family range widens to 4-8 strings (bass stays 4-6) so 5-lane banjo-family charts are legal. - E9 pedal-steel preset deferred with the MAX_LANES=8 vs 10-string display decision (roadmap 4.19). Tests: tests/strings_modal.test.js (8 cases) drives the real AddStringCmd + SetStringTuningCmd + range policy — the floor, high-side add, and tuning entry assertions fail on main. Full JS suite 42 files green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Gate add/remove string ops to the end the pitch/label model can represent. The either-end/guitar-floor-4 design silently re-snapped the string count and re-interpreted note indices (guitar high-add, guitar remove-below-6, high-remove on a low-extended shape, bass high-add at 4). Add/remove now go through _addPositionPure/_removePositionPure; guitar floor restored to 6. Direct per-string tuning entry (SetStringTuningCmd) is unchanged and covers exotic tunings without changing the count. Handler-level regression tests drive the real _stringCountFor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The wrapping label's only text is the "st" unit, so a screen reader announced the field as "st spinbutton" with no string identity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
55d5191 to
bc77535
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/strings_modal.test.js (1)
209-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a test for the "ignore no-op edits" behavior mentioned in the PR summary.
The PR summary states the tuning-edit flow "ignores no-op edits," but that guard lives in
editorSetStringTuning(a different cohort layer, not extracted here), so this suite doesn't cover it. Worth a follow-up test once that handler is in scope, to confirm identical old/new offsets don't push a no-op command onto the undo stack.Also applies to: 221-229
🤖 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 `@tests/strings_modal.test.js` around lines 209 - 219, Add a follow-up test for the “ignore no-op edits” path in the tuning-edit flow: this suite currently covers padding in SetStringTuningCmd, but not the guard in editorSetStringTuning. When that handler is available in scope, verify that editing a string to the same old/new offset does not enqueue a command or change the undo history, using the existing tuning-edit test setup in strings_modal.test.js as the reference point.
🤖 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 `@tests/strings_modal.test.js`:
- Around line 209-219: Add a follow-up test for the “ignore no-op edits” path in
the tuning-edit flow: this suite currently covers padding in SetStringTuningCmd,
but not the guard in editorSetStringTuning. When that handler is available in
scope, verify that editing a string to the same old/new offset does not enqueue
a command or change the undo history, using the existing tuning-edit test setup
in strings_modal.test.js as the reference point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dfc82d0e-6e4e-44fd-ad59-1d7e29c800a2
📒 Files selected for processing (4)
CHANGELOG.mdscreen.htmlscreen.jstests/strings_modal.test.js
💤 Files with no reviewable changes (1)
- screen.js
🚧 Files skipped from review as they are similar to previous changes (1)
- screen.html
2 participants
Roadmap 4.18 (Phase 4 — instruments). The command layer (
AddStringCmd/RemoveStringCmd) always supported both ends; the modal was the bottleneck — it hardcoded the position policy, offered no tuning editing, and its guitar floor of 6 made 5-lane banjo charts unconstructible.What
+ Add low/+ Add highand− Remove low/− Remove highbuttons replace the policy-picked pair. High-side adds were previously unreachable except the one hardcoded 5→6 bass case. Removal still refuses an end string that carries notes — now checked per end independently, so notes on one end never block removing the other.SetStringTuningCmd. It captures its target arrangement index so undo lands on the right arrangement after a switch, pads short tuning arrays instead of writing holes, and skips no-op edits. Drop tunings, open tunings, and re-entrant setups — a banjo''s high drone 5th — are now typable instead of unconstructible.Tests
tests/strings_modal.test.js(8 cases) drives the realAddStringCmd+SetStringTuningCmd+ range policy: the 4-floor, high-side adds (note lanes untouched), low-add note renumbering, the banjo construction sequence end-to-end with exact undo, arrangement-switch undo targeting, array padding, and boundary clamps. The floor/high-add/tuning-entry assertions fail on main. Full JS suite: 42 files green;node --checkclean.Fresh region vs the open queue (#101–#108) — Strings modal + its command block only.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit
New Features
Bug Fixes
Tests