Add preview playback-speed control (1×/1.5×/2×/4×)#18
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPreview playback gains selectable 1×, 1.5×, 2×, and 4× rates through a toolbar button and J/K/L shortcuts. Timeline and media synchronization use the selected rate, while exports remain at 1×. Documentation describes the feature. ChangesPreview playback speed
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant btnSpeed
participant cyclePreviewRate
participant animationLoop
participant syncMedia
participant mediaElements
User->>btnSpeed: click or press J, K, or L
btnSpeed->>cyclePreviewRate: select preview rate
cyclePreviewRate->>animationLoop: update state.previewRate
animationLoop->>syncMedia: advance timeline and synchronize clips
syncMedia->>mediaElements: apply effective rate and currentTime
animationLoop->>animationLoop: use 1x during export
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app.js (2)
2765-2765: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse cached DOM element reference.
For consistency with other UI interactions in the app, consider using the
els.btnSpeedreference you already cached on line 168 instead of querying the DOM again here.💡 Proposed refactor
-$("btnSpeed").addEventListener("click", () => cyclePreviewRate(1)); +els.btnSpeed.addEventListener("click", () => cyclePreviewRate(1));🤖 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 `@app.js` at line 2765, Update the click listener registration for btnSpeed to use the existing cached els.btnSpeed reference instead of querying the DOM with $("btnSpeed"). Preserve the current cyclePreviewRate(1) handler behavior.
1576-1580: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClamp instead of wrap for directional keyboard shortcuts.
While wrapping around is the expected behavior for a single UI toggle button, it can be counter-intuitive when using discrete "faster" and "slower" keyboard commands (e.g., pressing "faster" at 4x shouldn't drop the speed down to 1x). Consider adding a
wrapparameter to enforce array bounds when using the hotkeys.
app.js#L1576-L1580: updatecyclePreviewRateto conditionally applyclampor modulo based on awrapflag.app.js#L2819-L2820: passfalsewhen invoking the function from the directional keyboard shortcuts.💡 Proposed refactor
app.js#L1576-L1580:-function cyclePreviewRate(dir) { - const i = PREVIEW_RATES.indexOf(state.previewRate); - const n = ((i < 0 ? 0 : i) + dir + PREVIEW_RATES.length) % PREVIEW_RATES.length; - setPreviewRate(PREVIEW_RATES[n]); +function cyclePreviewRate(dir, wrap = true) { + const i = Math.max(0, PREVIEW_RATES.indexOf(state.previewRate)); + const n = wrap + ? (i + dir + PREVIEW_RATES.length) % PREVIEW_RATES.length + : clamp(i + dir, 0, PREVIEW_RATES.length - 1); + setPreviewRate(PREVIEW_RATES[n]); }
app.js#L2819-L2820:- else if (k === ">") cyclePreviewRate(1); // Shift+. → faster preview - else if (k === "<") cyclePreviewRate(-1); // Shift+, → slower preview + else if (k === ">") cyclePreviewRate(1, false); // Shift+. → faster preview + else if (k === "<") cyclePreviewRate(-1, false); // Shift+, → slower preview🤖 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 `@app.js` around lines 1576 - 1580, Update app.js lines 1576-1580 in cyclePreviewRate to accept a wrap flag and use bounded clamping when wrapping is disabled, while preserving modulo wrapping by default for the toggle button. Update app.js lines 2819-2820 to pass false from the directional keyboard shortcut invocations so faster/slower commands stop at the first and last PREVIEW_RATES values.
🤖 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 `@app.js`:
- Line 2765: Update the click listener registration for btnSpeed to use the
existing cached els.btnSpeed reference instead of querying the DOM with
$("btnSpeed"). Preserve the current cyclePreviewRate(1) handler behavior.
- Around line 1576-1580: Update app.js lines 1576-1580 in cyclePreviewRate to
accept a wrap flag and use bounded clamping when wrapping is disabled, while
preserving modulo wrapping by default for the toggle button. Update app.js lines
2819-2820 to pass false from the directional keyboard shortcut invocations so
faster/slower commands stop at the first and last PREVIEW_RATES values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac116552-ba8a-4eb4-8035-969b20d7a11d
📒 Files selected for processing (4)
CHANGELOG.mdREADME.mdapp.jsindex.html
|
Maybe it would be better to implement the standard JKL shortcuts, with double click on L speeds up, double click on J speeds down, K acts as spacebar now and resets speed to x1 ? |
Monitor toolbar toggle cycles 1×/1.5×/2×/4×; J/K/L drive it like a video editor — L faster, J slower, K play and reset to 1× (per @PlkMarudny's suggestion on ronak-create#18). state.previewRate scales the play-loop clock and each active clip's playbackRate; playRate() forces 1 during export so fast renders and the MediaRecorder capture stay real-time. Toolbar click cycles (wraps), the J/L keys clamp at the ends. CHANGELOG [1.7.0], README and the help overlay updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSmZYU7BhVLcmybTWZwWAB
2073476 to
79dbd14
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@index.html`:
- Line 89: Update the btnSpeed tooltip and corresponding help text to accurately
document the J/K/L handlers: K toggles play/pause and resets speed to 1×, while
the first J or L press when paused only starts playback and later presses change
speed during playback. Keep the handler behavior unchanged unless necessary to
make the documented semantics explicit.
🪄 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
Run ID: 55e86b14-659b-472b-b141-279b1222592d
📒 Files selected for processing (4)
CHANGELOG.mdREADME.mdapp.jsindex.html
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
ronak-create
left a comment
ronak-create
left a comment
There was a problem hiding this comment.
Nice work adopting the J/K/L shuttle keys from the discussion — the implementation is clean: playRate() returning 1 during state.exporting covers both export engines, the drift tolerance scaling with the effective rate avoids re-seek fighting, and the clamp to the browser's playbackRate range is documented honestly. Two things before merge:
-
Rebase needed — #20 just landed and touches the same
syncMedialines (it adds anenabledterm to the play condition and rewrites the else branch to zero the gain node). When resolving, keep both:state.playing && enabled && activeAt(...)plus youreffrate. GitHub reports the branch as CONFLICTING against main now. -
CHANGELOG heading: the entry declares
[1.7.0] - 2026-07-14with a compare link — cutting a version is a release decision, and v1.7.0 doesn't exist yet. Please move the entry under an## [Unreleased]heading and drop the 1.7.0 compare link (the added[1.6.0]link is a genuine fix, keep that).
Minor, no action needed: the rate also applies playbackRate to audio elements — modern browsers preserve pitch by default, so this is fine; and keeping previewRate as session-only UI state (not in project.json) is the right call.
Verified node --check passes on the branch head.
|
I'd like to take this — the preview-only design is right (playRate() forced to 1 during export keeps renders real-time) and the JKL scheme is a good call. Two things before it can land:
Happy to merge once it's rebased. |
79dbd14 to
95f3e8a
Compare
Monitor toolbar toggle cycles 1×/1.5×/2×/4×; J/K/L drive it like a video editor — from a stop J/L start playback, then L steps faster and J slower, K toggles play/pause and resets to 1× (per @PlkMarudny's suggestion on ronak-create#18). state.previewRate scales the play-loop clock and each active clip's playbackRate; playRate() forces 1 during export so fast renders and the MediaRecorder capture stay real-time. Toolbar click cycles (wraps), the J/L keys clamp at the ends. CHANGELOG entry filed under [Unreleased]; README and the help overlay updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CHANGELOG.md`:
- Line 11: Update the “Preview playback speed” changelog entry to describe J and
L as slowing down or speeding up through the available rates with clamping,
rather than cycling all rates; describe K as toggling playback while resetting
the preview speed to 1×, and preserve the existing toolbar, per-clip speed, and
export behavior details.
🪄 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
Run ID: 7a340f62-a84d-4ded-bde8-09949fe41011
📒 Files selected for processing (4)
CHANGELOG.mdREADME.mdapp.jsindex.html
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- index.html
95f3e8a to
aaa2c3b
Compare
Monitor toolbar toggle cycles 1×/1.5×/2×/4×; J/K/L drive it like a video editor — from a stop J/L start playback, then L steps faster and J slower, K toggles play/pause and resets to 1× (per @PlkMarudny's suggestion on ronak-create#18). state.previewRate scales the play-loop clock and each active clip's playbackRate; playRate() forces 1 during export so fast renders and the MediaRecorder capture stay real-time. Toolbar click cycles (wraps), the J/L keys clamp at the ends. CHANGELOG entry filed under [Unreleased]; README and the help overlay updated. Co-Authored-By: Claude Opus 4.8 <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 (1)
app.js (1)
2036-2036: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEscape user-supplied text to prevent UI breakage and XSS.
The
p.textproperty is sourced from the project state, which can contain arbitrary user input. Inserting it directly intoinnerHTMLwithout escaping<and>allows a user (or a loaded malicious project file) to prematurely close the<textarea>by including</textarea>. This will break the inspector UI and enables Cross-Site Scripting (XSS) if followed by malicious HTML.🔒️ Proposed fix
- ${row("Content", `<textarea data-k="text">${p.text}</textarea>`, "", "text")} + ${row("Content", `<textarea data-k="text">${String(p.text || "").replace(/&/g, "&").replace(/</g, "<")}</textarea>`, "", "text")}(Using
String(p.text || "")also ensures it handlesundefinedgracefully without displaying the word "undefined").🤖 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 `@app.js` at line 2036, Update the Content textarea construction in the row call to HTML-escape the user-supplied p.text value, converting missing values to an empty string before escaping. Ensure both angle brackets and any other HTML-significant characters are escaped so arbitrary text cannot close the textarea or inject markup.
🧹 Nitpick comments (1)
app.js (1)
3725-3730: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrevent default browser actions for
JandLshortcuts.While
e.preventDefault()is correctly called forK, it is omitted forJandL. For consistency and to guarantee no unintended browser features are triggered (like Firefox's quick search), it is best to prevent the default action for all three shuttle keys.🛠️ Proposed fix
else if ((k === "l" || k === "L") && !e.ctrlKey && !e.metaKey && !e.altKey) { + e.preventDefault(); if (!state.playing) play(); else stepPreviewRate(1); // tap again = faster } else if ((k === "j" || k === "J") && !e.ctrlKey && !e.metaKey && !e.altKey) { + e.preventDefault(); if (!state.playing) play(); else stepPreviewRate(-1); // tap again = slower }🤖 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 `@app.js` around lines 3725 - 3730, Update the J/L keyboard shortcut branches in the key handler alongside the existing K handling to call e.preventDefault() before invoking play() or stepPreviewRate(). Preserve the current modifier checks and playback behavior while preventing browser default actions for both shuttle keys.
🤖 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 `@app.js`:
- Line 2036: Update the Content textarea construction in the row call to
HTML-escape the user-supplied p.text value, converting missing values to an
empty string before escaping. Ensure both angle brackets and any other
HTML-significant characters are escaped so arbitrary text cannot close the
textarea or inject markup.
---
Nitpick comments:
In `@app.js`:
- Around line 3725-3730: Update the J/L keyboard shortcut branches in the key
handler alongside the existing K handling to call e.preventDefault() before
invoking play() or stepPreviewRate(). Preserve the current modifier checks and
playback behavior while preventing browser default actions for both shuttle
keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba96381c-5a89-4a45-9374-4d80473bf4a3
📒 Files selected for processing (4)
CHANGELOG.mdREADME.mdapp.jsindex.html
🚧 Files skipped from review as they are similar to previous changes (3)
- index.html
- CHANGELOG.md
- README.md
Monitor toolbar toggle cycles 1×/1.5×/2×/4×; J/K/L drive it like a video editor — from a stop J/L start playback, then L steps faster and J slower, K toggles play/pause and resets to 1× (per @PlkMarudny's suggestion on ronak-create#18). state.previewRate scales the play-loop clock and each active clip's playbackRate; playRate() forces 1 during export so fast renders and the MediaRecorder capture stay real-time. Toolbar click cycles (wraps), the J/L keys clamp at the ends. CHANGELOG entry filed under [Unreleased]; README and the help overlay updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aaa2c3b to
71922fc
Compare
|
Both points addressed, head is now 1. Rebase — rebased onto In if (state.playing && enabled && activeAt(c, t)) {
const eff = clamp(sp * playRate(), 0.0625, 16);The else branch zeroing the gain node is untouched. 2. CHANGELOG — entry moved under Also picked up a CodeRabbit nit from this round: Skipped its other finding — unescaped
|
3 participants
Adds a preview-only playback-speed control so long timelines can be reviewed faster, without touching clip speed or the export.
What
1× → 1.5× → 2× → 4×.Shift+.faster,Shift+,slower.state.previewRatescales both the play-loop clock and each active clip'splaybackRate, so video and audio stay in sync (the drift tolerance scales with it too — no re-seek fighting).Preview only — export is unaffected
playRate()returns1wheneverstate.exportingis set, so the fast (ffmpeg) render and the realtime MediaRecorder capture both run at real time regardless of the toggle.Notes
node --checkpasses onapp.jsandserver.js.[1.6.0]+ README updated; the help overlay lists the shortcut.speed × previewRateexceeds 16 the video re-seeks slightly (the browser clampsplaybackRateto 16 while the clock keeps advancing). Preview-only cosmetic, and only reachable above the documented 4× clip-speed max.🤖 Generated with Claude Code
Summary by CodeRabbit