fix(editor): an auto-sync import keeps the front of the chart#357
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughGP imports now convert source files at offset zero, then apply scalar audio offsets after parsing. The change preserves lead-in notes, retimes notation sidecars, and adds regression tests for negative, positive, and zero offsets. ChangesGP import offset handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A Guitar Pro import aligned to a recording with a lead-in used to lose its opening notes. The scalar-offset path baked the audio offset into the converted XML, so the pre-roll landed at a negative time that core's parse_arrangement slices off at t>=0 — silently dropping the front of the chart (one report lost the first 16 notes). If auto-sync wasn't used it imported fine, which pinned the cause to the offset application. Guitar Pro is now always converted at offset 0.0 and the shift is applied to the parsed arrangement in memory via the new _apply_gp_import_offset (built on the existing, tested _apply_chart_offset, which preserves negative times) — the same in-place retime the per-bar warp path already used and was immune because of. This repairs the scalar-offset fallback the warp path hands off to (GP5 with repeats, degenerate/absent sync points, or an older core). Notes, beats, sections and the keys notation sidecars all ride the shift. Tests: tests/test_gp_import_offset.py (front notes survive at negative time; sidecars retimed by the same offset; zero-offset no-op). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
0f969b4 to
e0d5112
Compare
The bug
Fixes got-feedBack/feedBack#246 — a Guitar Pro import aligned to a recording with a lead-in (the audio starts before the tab's bar 1) silently loses its opening notes. One report lost the first 16. It only happens with audio auto-sync; a plain import is fine — which pins the cause to the offset application.
Root cause
The scalar-offset import path baked the audio offset into the converted XML (
convert_file(audio_offset=…)). For a lead-in the resolved offset is negative, so the front notes land at a negative XML time — and core'sparse_arrangementslices each level withbisect_left(times, 0.0), discarding everything beforet = 0. No warning; the notes are just gone before the frontend ever sees them.The newer per-bar warp path was already immune: it converts at offset 0 and retimes the parsed
Songin memory afterward. This bug lived only in the scalar-offset fallback the warp path hands off to (GP5 with repeats, degenerate/absent sync points, or an older core).The fix
Mirror what warp already does — never bake the offset into the XML:
convert-gpnow always converts ataudio_offset=0.0.Songin memory via the new_apply_gp_import_offset, built on the existing, tested_apply_chart_offset(which rewrites absolute times only and preserves negatives)._attach_gp_notationstays consistent.Notes, beats, sections and keys notation all ride the shift; nothing off the front is dropped. Zero spec change; the warp path is untouched.
Tests
tests/test_gp_import_offset.py— front notes survive at negative time (nothing dropped), sidecars retimed by the same offset, zero-offset no-op.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests