Skip to content

refactor(editor): move Sync-Tempo to src/sync-tempo.js (R2, step 33) - #185

Merged
byrongamatos merged 1 commit into
mainfrom
refactor/r2-step33-sync-tempo
Jul 10, 2026
Merged

byrongamatos merged 1 commit into
mainfrom
refactor/r2-step33-sync-tempo

Conversation

@byrongamatos

@byrongamatos byrongamatos commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

What

Step 33 of the R2 module-extraction refactor: move the Sync-Tempo feature out of src/main.js into a new native ES module src/sync-tempo.js.

Detect the audio's BPM (onset autocorrelation), compare against the tab's grid BPM, and scale notes/beats/sections to match — reprojecting onto the warped grid when a sync point is locked.

How the seam is drawn

  • 4 window.editor* handlers (editorSyncTempo / editorSyncUpdateFactor / editorHideSyncDialog / editorApplySync) become named exports, re-attached by main.js.
  • getTabBPM exported back — main.js calls it at 2 sites.
  • The start-landing helpers (editorShowStartLanding / _editorMaybeShowStartLanding) that happened to share the banner are a separate feature and stayed in main.js — this was a function-level cut, not a banner range.
  • draw routes through host; no new host hook.
  • Removed the now-dead beatOf/timeOf main.js import (only the moved reproject used them).
  • tests/beat_lock.test.mjs retargeted to slice editorApplySync's section-scale loop from src/sync-tempo.js (byte-identical).

main.js drops ~218 lines. 34 modules.

Verification

  • 90/90 JS suites pass; ESLint gate clean (0 errors); strict no-undef clean on sync-tempo.js.
  • Codex preflight: 0 correctness issues.
  • New headless harness (verify_sync_tempo.py) drives the real detect→apply flow: editorSyncTempo detects the audio BPM by autocorrelation over the decoded buffer (90 tab / 120 audio on the diagnostic song) and populates the dialog; editorApplySync scales by the 1.3333× factor and reports it (proving host.draw resolved); the dialog closes; zero page errors. Negative-checked by breaking the host.draw call.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added a Sync Tempo workflow for matching editor beats and musical timing to an audio track.
    • Detects audio BPM and calculates tab BPM, with support for manual BPM and offset adjustments.
    • Preserves locked sync points while updating beats, notes, sustains, and section timing.
  • Bug Fixes

    • Improved timing alignment when applying tempo synchronization with locked points.
  • Tests

    • Updated beat-lock tests to validate the tempo synchronization workflow.

Extract the tempo-sync feature (detectAudioBPM onset-autocorrelation, getTabBPM,
and the sync dialog that scales notes/beats/sections to match — respecting locked
sync points) out of the src/main.js monolith into a new native ES module.

- src/sync-tempo.js: 4 window.editor* handlers (re-attached by main.js) + getTabBPM
  exported back (main.js calls it at 2 sites). The start-landing helpers
  (editorShowStartLanding / _editorMaybeShowStartLanding) that shared the banner
  are a separate feature and STAYED in main.js — this was a function-level cut.
- draw routes through host; no new host hook.
- Removed the now-dead beatOf/timeOf main.js import (only the moved reproject used
  them).
- tests/beat_lock.test.mjs retargeted to slice editorApplySync's section-scale
  loop from sync-tempo.js.

main.js drops ~218 lines. 34 modules.

Verified: 90/90 JS suites pass, ESLint gate clean (0 errors), strict no-undef
clean on sync-tempo.js, Codex preflight 0 issues. New headless harness drives the
REAL detect→apply flow: editorSyncTempo detects the audio BPM (autocorrelation
over the decoded buffer) and tab BPM, populates the dialog; editorApplySync scales
by the factor and reports it (proving host.draw); dialog closes; zero page errors.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Failed to post review comments.

GitHub was unavailable or timed out while CodeRabbit was posting the review. Please request a new review later if the pull request still needs one. Use @coderabbitai full review to retry the review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 493e6656-0263-42a3-9498-3afa6c2cef4a

📥 Commits

Reviewing files that changed from the base of the PR and between c69c425 and db39123.

📒 Files selected for processing (3)
  • src/main.js
  • src/sync-tempo.js
  • tests/beat_lock.test.mjs
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: CodeRabbit / Review
🧰 Additional context used
🪛 ast-grep (0.44.1)
src/sync-tempo.js

[error] 135-135: React's useState should not be directly called
Context: setStatus('Need audio and beats loaded for sync')
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.

(usestate-direct-usage)


[error] 139-139: React's useState should not be directly called
Context: setStatus('Detecting audio BPM...')
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.

(usestate-direct-usage)


[error] 155-155: React's useState should not be directly called
Context: setStatus('Ready')
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.

(usestate-direct-usage)


[error] 224-224: React's useState should not be directly called
Context: setStatus(Tempo synced: scaled ${factor.toFixed(4)}x + (offset ? , offset ${offset}s : ''))
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.

(usestate-direct-usage)

📝 Walkthrough

Walkthrough

Sync Tempo functionality moves from main.js into sync-tempo.js, which now handles BPM detection, dialog controls, locked-grid rescaling, note and section reprojection, and exported editor actions. Beat-lock tests now extract the section-scaling logic from the new module.

Changes

Sync Tempo modularization

Layer / File(s) Summary
Extract and wire Sync Tempo
src/main.js, src/sync-tempo.js
Tempo-sync logic is moved into sync-tempo.js; main.js imports the exports and re-attaches the expected window globals.
Detect tempo and manage sync dialog
src/sync-tempo.js
Audio BPM detection, tab BPM calculation, factor updates, and dialog visibility are implemented in the new module.
Apply locked grid synchronization
src/sync-tempo.js, tests/beat_lock.test.mjs
Beat, note, and section timing is rescaled with lock-aware respacing, and the beat-lock test harness extracts the section logic from sync-tempo.js.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant EditorUI
  participant sync-tempo.js
  participant S.audioBuffer
  participant S.beats
  participant host.draw
  EditorUI->>sync-tempo.js: editorSyncTempo()
  sync-tempo.js->>S.audioBuffer: detectAudioBPM()
  sync-tempo.js->>S.beats: getTabBPM()
  sync-tempo.js->>EditorUI: show BPM and factor dialog
  EditorUI->>sync-tempo.js: editorApplySync()
  sync-tempo.js->>S.beats: rescale beats, notes, and sections
  sync-tempo.js->>host.draw: redraw editor
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main refactor of moving Sync Tempo into a new module.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/r2-step33-sync-tempo

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant