feat(editor): point users at Tempo Map when an import drifts from the audio - #82
Merged
Merged
Conversation
… audio Auto-sync can only approximate a human performance between sync points, so an imported GP chart often starts aligned and then drifts -- and the #1 confusion in the field is not realizing the beatmap is editable at all. The Tempo Map editor (drag sync points, per-measure BPM + time signature, insert/delete, drum-vs-all ride scope) already exists, but nobody finds it ("surprised you can't edit the beatmap"). Make the fix discoverable at the moment it's needed: the post-import status now names it for both the per-bar 'warp' and the scalar 'offset' fallback (repeats/jumps, degenerate anchors), ending with "Drifting from the recording? Open Tempo Map to drag the beat grid onto the audio." The toolbar button tooltip now reads "...fix a chart drifting from the audio -- drag the beat grid, edit BPM & time signatures." No change to the import or the Tempo Map editor itself. The message is extracted to the pure _syncAppliedMessagePure so it's unit-tested. All 25 editor JS test files pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UR2Cr7GEu3yMY7SrfxH6c1
|
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 (1)
📝 WalkthroughWalkthroughIntroduces a pure helper for GP import sync status messaging, wires it into the import flow, updates the Tempo Map tooltip, and adds tests plus a changelog note. ChangesTempo Map guidance messaging
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ScreenJS as editorDoCreate
participant API as convert-gp API
participant Helper as _syncAppliedMessagePure
User->>ScreenJS: Import Guitar Pro file
ScreenJS->>API: POST /api/plugins/editor/convert-gp
API-->>ScreenJS: sync_applied, sync_reason
ScreenJS->>Helper: _syncAppliedMessagePure(sync_applied, sync_reason)
Helper-->>ScreenJS: status message or ''
ScreenJS-->>User: Display status (if non-empty)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
56-62: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winOrphaned/malformed changelog entry.
The new Tempo Map bullet at Line 56 ends cleanly with the test reference, but Line 57 immediately continues with unrelated, mid-sentence text —
"centre line), which hid the signal's actual shape and went blocky on zoom."— that clearly belongs to the previous waveform-envelope entry. This entry's own bullet title/opening appears to have been overwritten by the new insertion, leaving orphaned continuation text with no heading, which corrupts the rendered changelog.Suggested fix
### Changed -- **Point people at Tempo Map when an import drifts from the audio.** Auto-sync can only approximate a *human* performance between sync points, so an imported chart often starts aligned and then drifts (falls behind / gets ahead) — and the `#1` confusion in the field is not realizing the **beatmap is editable at all**. The post-import status now names the fix in both cases: per-bar `warp` and the scalar `offset` fallback (GP3/4/5 repeats/jumps, degenerate anchors) each end with *"Drifting from the recording? Open 🎵 Tempo Map to drag the beat grid onto the audio."*, and the toolbar button's tooltip now reads "…fix a chart drifting from the audio — drag the beat grid, edit BPM & time signatures." No behavior change to the import itself; the Tempo Map editor (drag sync points, per-measure BPM + time signature, insert/delete, drum-vs-all ride scope) already existed. Tests: `tests/tempo_map_guidance.test.js` (`_syncAppliedMessagePure`). - centre line), which hid the signal's actual shape and went blocky on zoom. - It now renders the true signed **min→max peak envelope** with a brighter - **RMS body** inside it (Audacity-style), built from a high-resolution - per-bin min/max/RMS cache (~3 ms/bin) and aggregated per pixel column so the - shape stays sharp at any zoom. Same lane, same seek behaviour — no layout or - API change. Pure helper `_buildWaveformPeaks` covered by tests. +- **Point people at Tempo Map when an import drifts from the audio.** Auto-sync can only approximate a *human* performance between sync points, so an imported chart often starts aligned and then drifts (falls behind / gets ahead) — and the `#1` confusion in the field is not realizing the **beatmap is editable at all**. The post-import status now names the fix in both cases: per-bar `warp` and the scalar `offset` fallback (GP3/4/5 repeats/jumps, degenerate anchors) each end with *"Drifting from the recording? Open 🎵 Tempo Map to drag the beat grid onto the audio."*, and the toolbar button's tooltip now reads "…fix a chart drifting from the audio — drag the beat grid, edit BPM & time signatures." No behavior change to the import itself; the Tempo Map editor (drag sync points, per-measure BPM + time signature, insert/delete, drum-vs-all ride scope) already existed. Tests: `tests/tempo_map_guidance.test.js` (`_syncAppliedMessagePure`). +- **Waveform lane now renders a true peak envelope instead of a flat + centre line**, which hid the signal's actual shape and went blocky on zoom. + It now renders the true signed **min→max peak envelope** with a brighter + **RMS body** inside it (Audacity-style), built from a high-resolution + per-bin min/max/RMS cache (~3 ms/bin) and aggregated per pixel column so the + shape stays sharp at any zoom. Same lane, same seek behaviour — no layout or + API change. Pure helper `_buildWaveformPeaks` covered by tests.🤖 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 `@CHANGELOG.md` around lines 56 - 62, The changelog entry is malformed because the new Tempo Map note in CHANGELOG.md is followed by leftover continuation text from the waveform-envelope entry, leaving an orphaned sentence with no heading. Reconstruct the two adjacent bullets so the Tempo Map item ends cleanly after the `_syncAppliedMessagePure` test reference, and move the waveform-related sentence back under its own `_buildWaveformPeaks` entry with its original heading. Keep the surrounding changelog text readable and ensure each entry is self-contained.
🤖 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.
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 56-62: The changelog entry is malformed because the new Tempo Map
note in CHANGELOG.md is followed by leftover continuation text from the
waveform-envelope entry, leaving an orphaned sentence with no heading.
Reconstruct the two adjacent bullets so the Tempo Map item ends cleanly after
the `_syncAppliedMessagePure` test reference, and move the waveform-related
sentence back under its own `_buildWaveformPeaks` entry with its original
heading. Keep the surrounding changelog text readable and ensure each entry is
self-contained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c62c61a9-55e4-44c0-b54f-7efe5e160605
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/tempo_map_guidance.test.js
The Tempo Map discoverability bullet was pasted over the 'real waveform' entry's header + first sentence. Restore the waveform entry and keep the Tempo Map note as its own bullet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Why
The community keeps hitting this: import a Guitar Pro tab + audio, and the chart starts synced but drifts (falls behind, catches up, gets ahead) — worst around section / time-signature changes. That's largely inherent: auto-sync interpolates between sync points, and a human performance never sits on one BPM. The editor already has the fix — an EOF-style Tempo Map editor (drag the measure sync points to fit the audio, set per-measure BPM + time signature, insert/delete points, choose whether all notes or just drums re-time). But nobody finds it — in the field the reaction is literally "surprised you can't edit the beatmap."
This is the discoverability fix, delivered at the moment it's needed.
What
warp: "Imported with per-bar audio sync. Drifting from the recording? Open 🎵 Tempo Map to drag the beat grid onto the audio."offsetfallback (GP3/4/5 repeats/jumps, degenerate anchors): the existing "applied start offset only" note + the same Tempo Map pointer._syncAppliedMessagePureso it's unit-tested.Not in scope (deliberately)
Whether there's a genuine time-signature bug on top of the inherent sparse-anchor drift (some users report 2/4-amid-4/4 bars throwing it off) — that needs a repro to confirm and is a separate change. This PR just makes the existing manual fix findable.
Tests
tests/tempo_map_guidance.test.js— 4 new cases (warp/offset-repeats /offset-other / no-sync). All 25 editor JS test files pass;node --checkclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01UR2Cr7GEu3yMY7SrfxH6c1
Summary by CodeRabbit