feat: per-bar sync warp on GP import + YouTube source for auto-sync#71
Merged
Conversation
…sync Auto-sync computed per-bar sync points (DTW) but convert-gp only ever applied the scalar bar-1 audio_offset, so any tempo difference between the recording and the tab's authored tempo accumulated audibly over the song. Now: - convert-gp accepts the sync_points payload back from the client and warps the whole converted Song (notes, sustains, beats, sections, handshapes, phrase levels) onto the recording's timeline via core's new piecewise warp helpers (lib.gp_autosync.bar_start_times/build_warp_anchors/warp_time/ warp_song_times). Falls back to the scalar offset — with sync_applied: 'offset' in the response — for GP3/4/5 files whose repeat expansion the as-written sync points can't map, when anchors degenerate, or when core lacks the new helpers (older core: ImportError is caught). - the create flow auto-runs the refine pass (onset phase sweep) right after the coarse DTW sync, so imports get per-bar accuracy by default; the manual refine row remains for re-running at a different density. Both refine calls now send gp_path so refinement uses exact per-bar score times. - the auto-sync section accepts a YouTube URL (reusing /youtube-audio) as the alignment/import audio source, next to the existing file upload. - _parse_sync_points_payload: the sync-point validation that lived inline in refine-sync is now a module-level helper shared with convert-gp, with tests. Requires got-feedback/feedback PR adding the warp helpers + refine_sync to lib/gp_autosync.py (the refine-sync endpoint has imported lib.gp_autosync .refine_sync since the snapshot, but the function never existed in core — the Refine button 500'd; that PR fixes it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds per-bar warp sync-point alignment to GP import/conversion, YouTube-based autosync audio input, a refine-sync pass using per-bar timing, and fallback scalar-offset reporting when warp alignment is not used. ChangesAuto-sync warp alignment and YouTube input
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves Guitar Pro import sync fidelity by applying per-bar warping (instead of only a scalar offset), automatically refining coarse autosync results, and allowing YouTube audio as the autosync source in the manual flow.
Changes:
- Backend: validate
sync_points, apply per-bar warp duringconvert-gp, and retime GP keys notation sidecars to avoid desync. - Backend:
refine-syncnow reuses sharedsync_pointsvalidation and can take a validatedgp_pathfor exact score-time reconstruction. - Frontend: add YouTube URL fetch as an autosync audio source and auto-run refine immediately after coarse DTW sync.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sync_points_payload.py | Adds unit tests for the shared sync_points payload validator. |
| screen.js | Adds YouTube autosync source, consolidates autosync source state handling, and auto-runs refine before convert. |
| screen.html | Adds YouTube URL input + Fetch button to the autosync audio UI. |
| routes.py | Introduces shared sync_points validation, adds notation-sidecar warping, and applies per-bar warp + offset sign fix in convert-gp. |
| CHANGELOG.md | Documents the new per-bar warp behavior, auto-refine, and YouTube autosync source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
screen.html (1)
313-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd disabled-state styling to the Fetch button.
editorAutoSyncYtFetch()setsbtn.disabled = truewhile downloading, but the button has nodisabled:variant classes, so users get no visual cue it's busy (unlike other buttons in this modal, e.g.editor-create-go).💄 Proposed fix
<button onclick="editorAutoSyncYtFetch()" id="editor-autosync-yt-btn" - class="px-2 py-0.5 bg-dark-600 hover:bg-dark-500 rounded text-xs text-gray-300 transition">Fetch</button> + class="px-2 py-0.5 bg-dark-600 hover:bg-dark-500 rounded text-xs text-gray-300 transition disabled:opacity-50 disabled:cursor-not-allowed">Fetch</button>🤖 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.html` around lines 313 - 314, The Fetch button in the editor autosync modal is missing disabled-state styling, so it does not visually reflect when editorAutoSyncYtFetch() sets the button to disabled during download. Update the button markup for editor-autosync-yt-btn to include disabled: variant classes similar to the other modal buttons (for example, matching the disabled styling used by editor-create-go) so the busy state is clearly indicated.screen.js (2)
8580-8604: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
_requestRefineSyncassumescreateState.lastSyncis already set.
createState.lastSync.audio_offset/.sync_pointsare read without a null check. Both current callers happen to setlastSyncbefore invoking this, so it's not exploitable today, but the helper itself has no precondition guard against future callers.🛡️ Optional guard
async function _requestRefineSync(barsPerPoint) { + if (!createState.lastSync) return null; try {🤖 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 8580 - 8604, The `_requestRefineSync` helper assumes `createState.lastSync` is always present, but it reads `audio_offset` and `sync_points` directly with no guard. Update `_requestRefineSync` to safely handle a missing `createState.lastSync` by adding an early return or defaulting the request payload fields before the `fetch` call, so future callers cannot trigger a null access.
8609-8637: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on the YouTube download fetch.
If the
/youtube-audiorequest stalls (slow/hung network to the external YouTube fetch on the backend), the promise never resolves, the button stays disabled indefinitely, and the user gets no feedback or way to retry.⏱️ Proposed fix using AbortSignal.timeout
try { const resp = await fetch('/api/plugins/editor/youtube-audio', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ url }), + signal: AbortSignal.timeout(60000), }); const data = await resp.json(); if (data.error) { if (status) status.textContent = 'Download failed: ' + data.error; return; } _setAutoSyncSource(data.audio_url, data.title || 'YouTube audio'); } catch (e) { - if (status) status.textContent = 'Download failed: ' + e.message; + if (status) status.textContent = e.name === 'TimeoutError' + ? 'Download timed out — try again.' + : 'Download failed: ' + e.message; } finally { if (btn) btn.disabled = false; }🤖 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 8609 - 8637, The YouTube audio request in editorAutoSyncYtFetch can hang indefinitely because fetch has no timeout, leaving the button disabled and the user stuck. Add an AbortSignal-based timeout to the fetch call in editorAutoSyncYtFetch, and handle the abort case in the catch block by showing a clear timeout message in the editor-autosync-status element. Keep the existing finally block so editor-autosync-yt-btn is always re-enabled, and make sure the logic still works for normal success and server-side error responses.
🤖 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.html`:
- Around line 313-314: The Fetch button in the editor autosync modal is missing
disabled-state styling, so it does not visually reflect when
editorAutoSyncYtFetch() sets the button to disabled during download. Update the
button markup for editor-autosync-yt-btn to include disabled: variant classes
similar to the other modal buttons (for example, matching the disabled styling
used by editor-create-go) so the busy state is clearly indicated.
In `@screen.js`:
- Around line 8580-8604: The `_requestRefineSync` helper assumes
`createState.lastSync` is always present, but it reads `audio_offset` and
`sync_points` directly with no guard. Update `_requestRefineSync` to safely
handle a missing `createState.lastSync` by adding an early return or defaulting
the request payload fields before the `fetch` call, so future callers cannot
trigger a null access.
- Around line 8609-8637: The YouTube audio request in editorAutoSyncYtFetch can
hang indefinitely because fetch has no timeout, leaving the button disabled and
the user stuck. Add an AbortSignal-based timeout to the fetch call in
editorAutoSyncYtFetch, and handle the abort case in the catch block by showing a
clear timeout message in the editor-autosync-status element. Keep the existing
finally block so editor-autosync-yt-btn is always re-enabled, and make sure the
logic still works for normal success and server-side error responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ace5ee8-fdca-4750-9ab2-5f4dbef4b2ea
📒 Files selected for processing (5)
CHANGELOG.mdroutes.pyscreen.htmlscreen.jstests/test_sync_points_payload.py
…rtError fallback (sync_reason: unavailable) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 (2)
routes.py (2)
95-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject fractional and boolean bar values instead of truncating them.
int(sp["bar"])accepts1.9as bar1andTrueas bar1, so malformed sync points can silently warp the wrong bar.Proposed fix
try: - bar = int(sp["bar"]) + raw_bar = sp["bar"] + if isinstance(raw_bar, bool): + raise ValueError + bar_f = float(raw_bar) + if not math.isfinite(bar_f) or not bar_f.is_integer(): + raise ValueError + bar = int(bar_f) tsec = float(sp["time_secs"]) mbpm = float(sp["modified_bpm"]) obpm = float(sp["original_bpm"])🤖 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 `@routes.py` around lines 95 - 108, The sync point parsing in routes.py currently uses int(sp["bar"]) inside the validation block, which can silently accept fractional or boolean values as valid bar numbers. Update the sync point parser so the bar field is only accepted when it is a true non-negative integer value, rejecting inputs like 1.9 and True before constructing the point entry. Keep the existing validation flow in the sync-point handling logic and return the same error path from the parser when bar is not an exact integer.
5165-5170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
sync_pointswhen the key is present, even if the value is falsy.
data.get("sync_points") or []lets malformed provided values like{}or""bypass_parse_sync_points_payload, despite the shared helper rejecting non-list payloads.Proposed fix
- _sync_points_payload = data.get("sync_points") or [] - _warp_points = None - if _sync_points_payload: + _sync_points_payload = data.get("sync_points", []) + _warp_points = None + if "sync_points" in (data or {}): _warp_points, _sync_err = _parse_sync_points_payload(_sync_points_payload) if _sync_err: return JSONResponse({"error": _sync_err}, 400)🤖 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 `@routes.py` around lines 5165 - 5170, The sync_points handling currently skips validation for provided falsy values because it uses data.get("sync_points") or [], which can hide malformed inputs like {} or "". Update the sync_points parsing logic in the request handler around _parse_sync_points_payload so that it checks whether the key is present in data and validates the raw value whenever it is supplied, even if falsy. Keep the existing _parse_sync_points_payload helper as the source of truth, and ensure the branch only bypasses parsing when sync_points is truly absent.
🤖 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 `@routes.py`:
- Around line 95-108: The sync point parsing in routes.py currently uses
int(sp["bar"]) inside the validation block, which can silently accept fractional
or boolean values as valid bar numbers. Update the sync point parser so the bar
field is only accepted when it is a true non-negative integer value, rejecting
inputs like 1.9 and True before constructing the point entry. Keep the existing
validation flow in the sync-point handling logic and return the same error path
from the parser when bar is not an exact integer.
- Around line 5165-5170: The sync_points handling currently skips validation for
provided falsy values because it uses data.get("sync_points") or [], which can
hide malformed inputs like {} or "". Update the sync_points parsing logic in the
request handler around _parse_sync_points_payload so that it checks whether the
key is present in data and validates the raw value whenever it is supplied, even
if falsy. Keep the existing _parse_sync_points_payload helper as the source of
truth, and ensure the branch only bypasses parsing when sync_points is truly
absent.
This was referenced Jul 5, 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
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.
What
Auto-sync computed per-bar sync points but
convert-gpapplied only the scalar bar-1 offset, so recordings that drift from the tab's authored tempo went audibly out of sync over the song — and the offset it did apply had an inverted sign (same bug the embedded-GP8 path fixed earlier: a 5s intro shifted the chart 5s the wrong way). This PR makes GP + audio imports land Songsterr-tight.Depends on got-feedBack/feedBack#787 (warp helpers +
refine_syncinlib/gp_autosync.py). Against an older core this degrades gracefully: the ImportError is caught and the import falls back to offset-only sync.Changes
sync_pointspayload back from the client, converts at offset 0, and warps the whole Song (notes, sustains, beats, sections, handshapes, phrase levels) plus the on-disk keys notation sidecars onto the recording's timeline. Respondssync_applied: "warp", or"offset"+sync_reason(repeats/degenerate/error) when falling back — GP3/4/5 files with repeats/voltas/directions can't be mapped from as-written sync points.gp_pathso refinement uses exact per-bar score times (odd meters, tempo changes); a provided-but-invalidgp_pathis now a hard 400 instead of a silent 4/4 fallback. The refine endpoint's inline validation moved to module-level_parse_sync_points_payload(shared with convert-gp, unit-tested)./youtube-audio) beside the file upload, for the manual path; the redesigned create modal's master-audio YouTube field already couples into the same warp via Fork A.Tests
tests/test_sync_points_payload.py(7 new); full suite green: 197 pytest + 24 node tests (rebased onto the #45 create-modal redesign; auto-refine integrated with the coupled/manual fork).Known follow-ups (unchanged behavior)
+Keys/+Drumsadditions don't inherit the warp (they never inherited the offset either) — needs a session-level warp record.Review note
Codex preflight was unavailable (usage limit until Jul 7); an 8-angle multi-agent Claude review ran instead — findings (offset sign, notation sidecar desync, misleading fallback message, refine gp_path 400, shared-helper dedups, CHANGELOG) are fixed here.
🤖 Generated with Claude Code
Summary by CodeRabbit