feat(editor): "Song Fit" — one home for shift / fit-tempo / set-constant (tempo PR 9) - #234
Merged
Conversation
…ant (tempo PR 9)
Tempo-track PR 9 (charrette UX P1). The editor already has three ways to line a
chart up to a recording — an Offset nudge, audio Sync, and the BPM flatten with
conform/rebuild — but they're scattered toolbar controls most users never
connect. Song Fit is one discoverable front door over them:
- A **Song Fit…** button in the Tempo Map inspector opens a small menu with the
three operations, each labelled by a shared "consequence badge" stating what
it does to audio / grid / notes ("audio stays · …" — the audio never moves):
**Shift everything…** (offset nudge, keeps the ±10ms arrows in a compact
modal), **Fit tempo to recording…** (dispatches to the existing Sync verb),
and **Set constant tempo…** (prompt a BPM → the conform/rebuild flatten).
- The inline **Offset / Sync / BPM** controls are LEFT ALONE — Song Fit routes
through the same undoable verbs; nothing about direct editing changes (the
conservative scope: a new front door, not a behaviour swap).
- editorSetBPM's flatten flow (PR 3 / #225) is factored into a shared
`_editorFlattenSongToBpm(newBPM, opts)` so "Set constant tempo" reaches it
inside Tempo Map mode, where the inline BPM box doesn't offer flatten. The
post-await session re-validation moved into the helper (the flatten source
guard follows it); editorSetBPM keeps the variable-map GATE and delegates.
New module `src/song-fit.js` (`_consequenceBadgePure`, `_songFitChoicesPure`,
`_editorSongFit`, `_editorShiftEverything`). `tests/song_fit.test.mjs` (6:
badges, choices-are-badges single-source, extraction source guards);
`flatten_choice.test.mjs` guard retargeted to the extracted helper. 125 JS
suites green, lint 0-err (3 pre-existing ratchet warnings). routes.py untouched.
Verified live on AC/DC — Back In Black: Song Fit menu shows all three badges;
Shift everything's +10ms moved the offset 0→+20ms (toolbar in step); Set
constant tempo → 100 BPM → Conform → "Whole song conformed to a constant 100.00
BPM — notes moved with the grid" (the extracted helper working from Tempo Map
mode). No page errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
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)
📝 WalkthroughWalkthroughAdds a Song Fit control to the Tempo Map inspector with shift, recording-fit, and constant-tempo actions. Extracts shared constant-tempo flattening logic, exposes new entry points, routes actions through existing editor verbs, and adds source-level tests. ChangesSong Fit tempo editing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TempoInspector
participant _editorSongFit
participant editorSyncTempo
participant editorNudgeOffset
participant editorFlattenSongToBpm
User->>TempoInspector: click Song Fit
TempoInspector->>_editorSongFit: open choices
_editorSongFit->>editorSyncTempo: fit tempo to recording
_editorSongFit->>editorNudgeOffset: shift everything
_editorSongFit->>editorFlattenSongToBpm: set constant tempo
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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 `@src/song-fit.js`:
- Around line 42-50: Update the Song Fit flow around the awaited choice prompt
to snapshot S.sessionId before prompting and revalidate it afterward before
dispatching any choice. Thread the same session token through
_editorShiftEverything and _songFitSetConstant, validating after
_editorPromptText and before each Shift modal action so commands are not applied
to a replacement session; follow the existing protection pattern used by
_editorFlattenSongToBpm.
In `@tests/song_fit.test.mjs`:
- Around line 65-67: Update the editorSetBPM extraction in the test to end at
the next function assignment rather than a fixed 900-character offset. Use the
existing editorSetBPM marker and the following function-assignment boundary so
the complete function body is scanned, while preserving the assertions for
delegating to _editorFlattenSongToBpm and excluding inline TempoGridCmd usage.
🪄 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: 0669398d-fe9d-4230-8393-27c626bda64f
📒 Files selected for processing (6)
CHANGELOG.mdsrc/main.jssrc/song-fit.jssrc/tempo.jstests/flatten_choice.test.mjstests/song_fit.test.mjs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/song-fit.js (1)
91-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the shift-value input an accessible name.
The numeric input only carries a
titleattribute; unlike_editorPromptTextinsrc/ui.js, which setsaria-labelon the input when there's no visible<label>, this input has no accessible name for screen readers beyond the unreliabletitle.♿ Proposed fix
- + '<input type="number" id="editor-shift-value" step="0.01" class="w-24 bg-dark-700 border border-gray-600 rounded px-2 py-1 text-sm text-center outline-none" title="Total shift in seconds (negative = earlier)">' + + '<input type="number" id="editor-shift-value" step="0.01" class="w-24 bg-dark-700 border border-gray-600 rounded px-2 py-1 text-sm text-center outline-none" title="Total shift in seconds (negative = earlier)" aria-label="Total shift in seconds (negative = earlier)">'🤖 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 `@src/song-fit.js` around lines 91 - 100, Update the input with id editor-shift-value in the shift editor markup to include an explicit aria-label describing the total chart shift in seconds, while retaining its existing title and behavior.
🤖 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 `@src/song-fit.js`:
- Around line 91-100: Update the input with id editor-shift-value in the shift
editor markup to include an explicit aria-label describing the total chart shift
in seconds, while retaining its existing title and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b34f3d4f-3246-4a78-a031-ec94cd6db391
📒 Files selected for processing (2)
src/song-fit.jstests/song_fit.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/song_fit.test.mjs
This was referenced Jul 14, 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
2 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.
Tempo-track PR 9 — the surface pass that closes the assisted-tempo charrette (UX P1). The editor already has three ways to line a chart up to a recording, but they're scattered controls most people never connect. Song Fit gives them one discoverable home.
What's new
One refactor
editorSetBPM's flatten flow is factored into a shared_editorFlattenSongToBpm(newBPM, opts)so "Set constant tempo" works from inside Tempo Map mode, where the inline BPM box doesn't offer flatten.editorSetBPMkeeps its variable-map gate and delegates; the post-await session re-validation moved into the helper, soflatten_choice.test.mjs's source guard was retargeted to follow it (the variable-map re-check was relaxed to a session+grid guard so a constant song can still be re-flattened — the session-id check is the real protection).Tests / gates
New
src/song-fit.js;tests/song_fit.test.mjs(6: badge copy, choices-render-the-shared-badge single-source, extraction source guards). 125 JS suites green, lint 0 errors (3 pre-existing ratchet warnings).routes.pyuntouched.Verified live
On AC/DC — Back In Black: the menu shows all three badges; Shift everything's +10ms moved the offset 0 → +20ms with the toolbar in step; Set constant tempo → 100 BPM → Conform produced "Whole song conformed to a constant 100.00 BPM — notes moved with the grid" — confirming the extracted helper works from Tempo Map mode. No page errors.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests