feat(editor): "Bar 1 here" re-anchor + Lead-in region + import nudge (tempo PR 6) - #233
Merged
Conversation
…(tempo PR 6) Tempo-track PR 6 (charrette UX P4 / trans P4 / rhythm G2). Three related moves for lining a chart up to a recording that doesn't start at 0:00: - "Bar 1 here": an inspector button and a bar-1 pole right-click item (listed above the pickup item) shift the grid, every part's notes/chords/anchors/ drums, and the sections so bar 1's downbeat lands at the playhead. The audio never moves — it is a chart re-anchor riding the SAME undoable TempoOffsetCmd as a manual offset nudge (S.appliedOffset accrues; Ctrl+Z restores exactly). - Lead-in region: the space before bar 1 now draws as a labelled hatched wash, mirroring the Unmapped tail's treatment. - Import nudge (SUGGEST only, never auto-shift): when an import lands bar 1 at ~0 but the first detected onset is clearly later, the status line points the user at Tempo Map ▸ "Bar 1 here". - The pickup right-click item is relabelled "(partial first bar — for music that starts before beat 1)" so it reads distinctly from the new re-anchor. New pures in src/tempo.js: _firstDownbeatTimePure, _tempoBar1ShiftPure, _importBar1NudgePure (+ the _tempoSetBar1Here verb). tests/bar1_here.test.mjs (14): the pures, the nudge gating, and the command round-trip across a 2-arrangement song + drums + sections with exec/undo/redo. 122 JS suites green, lint 0-err (3 pre-existing ratchet warnings). routes.py untouched. Verified live on AC/DC — Back In Black: entered Tempo Map, clicked "Bar 1 here" at the playhead → "Bar 1 → 6.57s — chart and notes shifted; audio unchanged", the whole grid re-anchored, and the Lead-in wash rendered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Tempo Map controls to align bar 1 with the playhead, shifts musical timing through an undoable command, renders lead-in space, and appends import-time guidance when the first onset starts after the inferred grid. ChangesBar 1 tempo-grid anchoring
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant _tempoSetBar1Here
participant TempoOffsetCmd
participant EditorState
User->>_tempoSetBar1Here: Select Bar 1 here
_tempoSetBar1Here->>TempoOffsetCmd: Apply bar 1 shift
TempoOffsetCmd->>EditorState: Shift tempo grid and musical parts
EditorState-->>User: Update alignment and status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (2)
tests/bar1_here.test.mjs (1)
91-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRound-trip test never asserts anchors actually shift forward.
timesSnapshot()capturesanchorsand the undo-restore check (line 147) confirms the snapshot returns tobefore, but no assertion checks thatS.arrangements[X].anchors[Y].timeactually moved bydeltaafter_tempoSetBar1Here()(unlike notes, chords, handshapes, drums, and sections, which are all explicitly checked at lines 134-139). If a regression stopped anchors from being reprojected, this suite would still pass, since the undo-restore comparison only proves round-trip consistency of whatever was touched, not that anchors were touched at all — yet the command's own docstring (and PR objectives) explicitly claim anchors are shifted.✅ Suggested addition
assert.ok(near(S.drumTab.hits[0].t, 4.0), 'drum hit +delta'); assert.ok(near(S.sections[0].start_time, 3.0), 'section +delta'); + assert.ok(near(S.arrangements[0].anchors[0].time, 3.0), 'anchor +delta');🤖 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/bar1_here.test.mjs` around lines 91 - 153, Add an explicit post-_tempoSetBar1Here assertion for an anchor in seedMultiPart, such as arrangements[1].anchors[0].time, verifying it advances by the applied +2.0 delta. Keep the existing timesSnapshot undo/redo checks unchanged.src/tempo.js (1)
243-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLead-in wash duplicates the Unmapped-tail wash almost verbatim.
Both blocks compute clamped x0/x1, fill the same wash color, call
_tempoHatchRect, and conditionally draw a bold label — differing only in the time range and label text/threshold. Worth extracting a shared helper to avoid drift if the hatch styling ever changes.♻️ Suggested extraction
+function _tempoDrawHatchedSpan(t0, t1, w, gridBottom, label, minWidthForLabel) { + const x0 = Math.max(LABEL_W, timeToX(t0)); + const x1 = Math.min(w, timeToX(t1)); + const top = (TIMELINE_TOP + WAVEFORM_H); + if (x1 <= x0 + 2) return; + ctx.fillStyle = 'rgba(100,116,139,0.06)'; + ctx.fillRect(x0, top, x1 - x0, gridBottom - top); + _tempoHatchRect(x0, top, x1 - x0, gridBottom - top, '`#64748b`', 8, 0.10); + if (x1 - x0 > minWidthForLabel) { + ctx.fillStyle = '`#64748b`'; + ctx.font = 'bold 10px monospace'; + ctx.textAlign = 'left'; + ctx.textBaseline = 'top'; + ctx.fillText(label, x0 + 6, top + 6); + } +}Then both the "Unmapped tail" and "Lead-in" blocks collapse to a single call each, e.g.
_tempoDrawHatchedSpan(_lastDbTime, tailEndT, w, gridBottom, 'Unmapped', 66)and_tempoDrawHatchedSpan(0, _bar1T, w, gridBottom, 'Lead-in', 56).🤖 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/tempo.js` around lines 243 - 294, Extract the duplicated hatched-region rendering from the Unmapped tail and Lead-in blocks into a shared _tempoDrawHatchedSpan helper that handles clamped coordinates, wash fill, hatch drawing, and conditional labeling. Replace both inline rendering blocks with calls using their existing time ranges, labels, and width thresholds, preserving the current guards and visual 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.
Inline comments:
In `@src/create.js`:
- Around line 2052-2063: Update the status update at the end of the sync-message
flow around _syncAppliedMessagePure and the _msg nudge handling to call
setStatus unconditionally, including when _msg is an empty string. Preserve the
existing function-type guard, but remove the truthiness check so stale editor
status text is cleared.
---
Nitpick comments:
In `@src/tempo.js`:
- Around line 243-294: Extract the duplicated hatched-region rendering from the
Unmapped tail and Lead-in blocks into a shared _tempoDrawHatchedSpan helper that
handles clamped coordinates, wash fill, hatch drawing, and conditional labeling.
Replace both inline rendering blocks with calls using their existing time
ranges, labels, and width thresholds, preserving the current guards and visual
behavior.
In `@tests/bar1_here.test.mjs`:
- Around line 91-153: Add an explicit post-_tempoSetBar1Here assertion for an
anchor in seedMultiPart, such as arrangements[1].anchors[0].time, verifying it
advances by the applied +2.0 delta. Keep the existing timesSnapshot undo/redo
checks unchanged.
🪄 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: 7788b348-8366-45c6-8189-e33852d2b51e
📒 Files selected for processing (4)
CHANGELOG.mdsrc/create.jssrc/tempo.jstests/bar1_here.test.mjs
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 6 of the assisted-tempo charrette (UX P4 / trans P4 / rhythm G2). Everything you need to line a chart up to a recording that doesn't start at 0:00.
What's new
TempoOffsetCmdas a manual offset nudge (S.appliedOffsetaccrues; Ctrl+Z restores exactly). Status: "Bar 1 → 6.57s — chart and notes shifted; audio unchanged."warpimports (already bar-by-bar aligned).Design authority
docs/TEMPO-MAPPING-DESIGN.md+ the charrette. Re-anchoring goes through PR 1's offset command (#218); the region treatment mirrors PR 2 (#220). Both deps are onmain.Implementation
New pures in
src/tempo.js:_firstDownbeatTimePure,_tempoBar1ShiftPure,_importBar1NudgePure, plus the_tempoSetBar1Hereverb. Wired into the tempo-map inspector strip, the context menu, and the_tempoMapDrawpass.src/create.jsreads the first onset (_ensureOnsets, ready after the awaitedloadAudio) for the import nudge.Tests / gates
tests/bar1_here.test.mjs(14): the three pures, the nudge gating (fires only when bar 1 ≈ 0 and the onset is clearly + meaningfully later), and the command round-trip across a 2-arrangement song + drums + sections withexec → undo → redodeep-equality. 122 JS suites green, lint 0 errors (3 pre-existing ratchet warnings).routes.pyuntouched → no pytest.Verified live
On AC/DC — Back In Black: entered Tempo Map, moved the playhead, clicked Bar 1 here → grid re-anchored to 6.57s, the Lead-in wash rendered before bar 1, offset field synced to 6.5666, status read as designed. No page errors from the change.
🤖 Generated with Claude Code
Summary by CodeRabbit