fix: seek through the host funnel so far sections work off the <audio> element - #10
Merged
Conversation
…> element
The section map jumped only to sections near the playhead. It seeked by
poking `document.getElementById('audio').currentTime` directly, which used
to be the single source of truth for playback. It no longer is: a feedpak
full-mix plays through JUCE native output, whose seek is `jucePlayer.seek()`
(reached only via the host's canonical `_audioSeek` funnel) — writing the raw
element's currentTime never moves it. So a section click updated the muted
clock element but not the actual playback; far jumps were obviously wrong
while near ones looked close enough to pass.
Route every reposition (click + wheel) through `window.feedBack.seek()`
instead. It moves whichever backend is playing — JUCE native, or the
streaming stems worklet, which only reseeks on the `song:seek` event the
funnel emits — keeps the highway clock in sync, and lets notedetect suppress
detection across the seek transient. Read the current position from the host
clock (`highway.getTime()`) for the wheel nudge. The old raw-element path
stays only as a fallback for a host that predates the seek API.
Tests: click + wheel now assert the funnel is called with the right time
(raw element untouched); fallback path still covered; wheel reads the host
clock. 11/11 green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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 (2)
📝 WalkthroughWalkthroughSection map clicks and wheel seeks now use the host playback clock and a centralized seek funnel, with audio-element fallback behavior. Tests cover host routing, clamping, reason values, and fallback seeking. The plugin manifest version increases to 1.1.0. ChangesSection map seeking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SectionMap
participant screen_js
participant HostSeekAPI
participant AudioElement
SectionMap->>screen_js: click or wheel target time
screen_js->>HostSeekAPI: seek(time, reason) when available
HostSeekAPI-->>screen_js: host-managed seek
screen_js->>AudioElement: pause, seek, and resume when host API is unavailable
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 (1)
tests/screen.test.js (1)
119-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest title claims duration clamping but only asserts the lower bound.
The test name mentions clamping to
[0, duration], but the body only drives the backward/negative case. Consider adding a forward-direction case asserting the seek clamps at_smDuration.🤖 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/screen.test.js` around lines 119 - 132, Extend the _smOnWheel test to cover forward scrolling beyond the configured duration, asserting the seek target clamps to _smDuration (100) with reason sectionmap-wheel. Keep the existing backward case and prevented-default assertion unchanged.
🤖 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 `@screen.js`:
- Around line 74-88: Update _smSeek to clamp the seek time to the full valid
range [0, _smDuration], preserving its existing lower-bound handling. Use the
centrally clamped value for both host.seek and the legacy audio fallback so
_smOnClick and _smOnWheel cannot seek beyond the duration.
---
Nitpick comments:
In `@tests/screen.test.js`:
- Around line 119-132: Extend the _smOnWheel test to cover forward scrolling
beyond the configured duration, asserting the seek target clamps to _smDuration
(100) with reason sectionmap-wheel. Keep the existing backward case and
prevented-default assertion 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: 4538469d-370e-46a7-b430-3ffa655bc372
📒 Files selected for processing (3)
plugin.jsonscreen.jstests/screen.test.js
CodeRabbit: _smOnClick passed a raw pct*duration through, and _smSeek only clamped the lower bound — a pct just over 1 at the bar's right edge could seek past the end. Clamp to [0, _smDuration] centrally so click and wheel are consistent. 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
1 participant
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.
Symptom (tester)
The section-map bar at the top of the song only seeks to sections close to the playhead; clicking a distant section doesn't move playback.
Root cause
The plugin seeked by writing
document.getElementById('audio').currentTimedirectly. That worked when the<audio>element was playback. It no longer is: a feedpak full-mix now plays through JUCE native output, whose seek isjucePlayer.seek()— reachable only via the host's canonical_audioSeekfunnel (window.feedBack.seek). Writing the raw element'scurrentTimenever reaches JUCE, so a section click moved the muted clock element but not the actual audio. Far jumps were plainly wrong; near ones looked close enough to pass. (The tester's hunch — "audio streaming changes, stems don't all load at once" — was directionally right: playback moved off the raw<audio>element.)Fix
Route every reposition (click + wheel) through
window.feedBack.seek(time, reason):song:seekevent the funnel emits);_audioSeekcallshighway.setTime);song:seek).Wheel nudges read the current position from the host clock (
highway.getTime()) rather than the possibly-stale<audio>element. The old raw-element path (pause/seek/resume) stays only as a fallback for a host that predates the seek API.Tests
Click + wheel now assert the funnel is called with the correct time and the raw element is left untouched; a new test pins
_smNowreading the host clock; the legacy fallback (rawcurrentTime+ pause/resume) is still covered. 11/11 green.Note
Most impactful for the JUCE-native path (desktop feedpak full-mix — the common gig case). In stems mode both old and new code reach the stems
currentTimeshim, so behaviour there is unchanged. Wants an on-device confirm.🤖 Generated with Claude Code
Summary by CodeRabbit