feat(editor): guide claps — audible ticks for charted notes during playback - #90
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds a Guide claps transport toggle that sonifies charted events during playback. It includes keyboard and toolbar controls, AudioContext-based scheduling with lookahead timing, shared audio routing with limiter, seek/loop cancellation, persistence, and tests. ChangesGuide Claps Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)screen.jsast-grep timed out on this file 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)
screen.js (1)
5130-5142: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffAvoid rebuilding the full event-time array (and reading
localStorage) every tick.
_guideTickruns every 25 ms and calls_guideSourceTimes(), which.map()s the entire note/hit list into a fresh array each time (~40 allocations/sec over the whole chart; noticeable on dense charts).editorGuideClapEnabled()also hitslocalStorageon every tick. Consider caching the sorted times array (invalidated on edit/arrangement/mode change) and caching the enabled flag in memory, updating it only in_editorToggleGuideClap.🤖 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 5130 - 5142, _guideTick is doing too much work on every timer pulse by rebuilding the full source-time list via _guideSourceTimes() and re-reading editorGuideClapEnabled() from localStorage. Update _guideTick to use an in-memory cached sorted times array instead of recomputing the map each tick, and invalidate that cache only when the chart data/arrangement/mode changes. Also cache the guide-clap enabled state in memory and refresh it from _editorToggleGuideClap rather than polling localStorage in the hot path.
🤖 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 5086-5093: The arrangement branch in _guideSourceTimes returns
note times directly from notes(), but those n.time values can become unsorted
after edits, which breaks the binary search in _guideClapTimesInWindowPure.
Update _guideSourceTimes to return a sorted list of times for the arrangement
path (while leaving the S.drumEditMode/S.drumTab.hits path unchanged), or ensure
the same sorting happens whenever note times are mutated so the guide times are
always in ascending order.
---
Nitpick comments:
In `@screen.js`:
- Around line 5130-5142: _guideTick is doing too much work on every timer pulse
by rebuilding the full source-time list via _guideSourceTimes() and re-reading
editorGuideClapEnabled() from localStorage. Update _guideTick to use an
in-memory cached sorted times array instead of recomputing the map each tick,
and invalidate that cache only when the chart data/arrangement/mode changes.
Also cache the guide-clap enabled state in memory and refresh it from
_editorToggleGuideClap rather than polling localStorage in the hot path.
🪄 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: 3d3ce69b-a3c8-4e36-bb19-875d39614b30
📒 Files selected for processing (4)
CHANGELOG.mdscreen.htmlscreen.jstests/guide_clap.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
screen.js (1)
5209-5213: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCancel queued clap voices when disabling the toggle.
Turning claps off only syncs the timer, so voices already scheduled in the 120 ms lookahead window can still fire after the UI says “off”. Stop handling cancels voices separately; the off-toggle path should do the same.
Proposed fix
try { localStorage.setItem('editorGuideClap', next ? '1' : '0'); } catch (_) {} _refreshGuideBtn(); _guideTimerSync(); + if (!next) _guideCancelVoices(); setStatus(next🤖 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 5209 - 5213, The off-toggle path in _editorToggleGuideClap only updates storage and calls _guideTimerSync, so queued clap voices can still fire after the toggle is disabled. Update _editorToggleGuideClap to explicitly cancel any scheduled clap voices when next is false, using the same cancel logic already used elsewhere for stopping guide/clap playback, then keep the existing refresh and sync calls so the UI and timer state stay aligned.
🧹 Nitpick comments (1)
screen.js (1)
5068-5068: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftCache sanitized guide times outside the 25 ms scheduler tick.
_guideTick()now maps, filters, and sorts the full guide source every tick. That makes dense charts payO(n log n)about 40 times/sec, undermining the lookahead scheduler’s canvas-load goal. Prefer caching the sanitized/sorted times and invalidating on note/hit edits, arrangement changes, drum mode changes, or playback reset.Also applies to: 5108-5114, 5166-5166
🤖 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` at line 5068, _cache sanitized guide times instead of recomputing them inside _guideTick(); the current tick path is mapping, filtering, and sorting the full guide source every 25 ms, so move that sanitized/sorted result into a reusable cache and have _guideTick() consume it. Invalidate or rebuild the cache from the places that change guide data or timing, including note/hit edits, arrangement changes, drum mode changes, and playback reset, so the scheduler stays cheap while keeping GUIDE_TICK_MS behavior correct.
🤖 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 5077-5079: The editor guide clap toggle currently reads only from
localStorage in editorGuideClapEnabled(), so when storage access throws the UI
can switch to “on” but the feature still evaluates as off. Update the toggle
flow around editorGuideClapEnabled() and the related setter logic to maintain an
in-memory fallback state whenever localStorage.getItem() or setItem() fails, and
have the enabled check consult that fallback before returning false.
---
Outside diff comments:
In `@screen.js`:
- Around line 5209-5213: The off-toggle path in _editorToggleGuideClap only
updates storage and calls _guideTimerSync, so queued clap voices can still fire
after the toggle is disabled. Update _editorToggleGuideClap to explicitly cancel
any scheduled clap voices when next is false, using the same cancel logic
already used elsewhere for stopping guide/clap playback, then keep the existing
refresh and sync calls so the UI and timer state stay aligned.
---
Nitpick comments:
In `@screen.js`:
- Line 5068: _cache sanitized guide times instead of recomputing them inside
_guideTick(); the current tick path is mapping, filtering, and sorting the full
guide source every 25 ms, so move that sanitized/sorted result into a reusable
cache and have _guideTick() consume it. Invalidate or rebuild the cache from the
places that change guide data or timing, including note/hit edits, arrangement
changes, drum mode changes, and playback reset, so the scheduler stays cheap
while keeping GUIDE_TICK_MS behavior correct.
🪄 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: 5cf9ddc6-9916-4c68-a1a2-8486200368bc
📒 Files selected for processing (2)
screen.jstests/guide_clap.test.js
Comment on lines
+5077
to
+5079
| function editorGuideClapEnabled() { | ||
| try { return localStorage.getItem('editorGuideClap') === '1'; } | ||
| catch (_) { return false; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep an in-memory toggle state when localStorage is unavailable.
If localStorage.getItem() or setItem() throws, next becomes true and the status says claps are on, but editorGuideClapEnabled() still returns false, so the feature never enables.
Proposed fix
+let _editorGuideClapOn = (() => {
+ try { return localStorage.getItem('editorGuideClap') === '1'; }
+ catch (_) { return false; }
+})();
+
function editorGuideClapEnabled() {
- try { return localStorage.getItem('editorGuideClap') === '1'; }
- catch (_) { return false; }
+ return _editorGuideClapOn;
}
function _editorToggleGuideClap() {
const next = !editorGuideClapEnabled();
+ _editorGuideClapOn = next;
try { localStorage.setItem('editorGuideClap', next ? '1' : '0'); } catch (_) {}Also applies to: 5209-5216
🤖 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 5077 - 5079, The editor guide clap toggle currently
reads only from localStorage in editorGuideClapEnabled(), so when storage access
throws the UI can switch to “on” but the feature still evaluates as off. Update
the toggle flow around editorGuideClapEnabled() and the related setter logic to
maintain an in-memory fallback state whenever localStorage.getItem() or
setItem() fails, and have the enabled check consult that fallback before
returning false.
…ayback The editor had zero note sonification: charting by ear was impossible, especially for drum placement. Adds a "Claps" transport toggle (key C in both shortcut profiles) that ticks each charted event during playback — drum hits in drum-edit mode, the current arrangement's notes elsewhere. Engine (first slice of the DAW-workspace guide-playback design): - setInterval lookahead scheduler (25 ms cadence, 120 ms window) anchored on playStartWall/playStartTime — never the rAF draw loop, so clap timing survives heavy canvas load. - Every (re)start via _startAudioSourceAtCursor cancels queued voices and resets the window, so seeks and loop wraps can't fire ghost claps. - Chord stacks dedupe to one tick (1 ms buckets) so simultaneous notes can't sum into a louder transient; soft 3 ms attack / 45 ms decay voice. - New shared master bus: reference audio and claps each get a gain node, summed through a DynamicsCompressor limiter (threshold -1, ratio 20) — playback no longer connects raw to destination. - Toggle persists as an editor preference (localStorage), joins the command registry + both key profiles + the transport bar. Tests: tests/guide_clap.test.js (8 cases: window semantics, half-open boundaries, chord dedupe vs drum flams, dense-chart binary search, transport mapping). node --check clean; all 26 JS test files pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
…oop-end clamp Reviewer-confirmed fixes to the guide-clap scheduler: - Sanitize _guideSourceTimes: filter non-finite + sort ascending (new _guideSanitizeTimesPure) before the window query, matching every other time-array consumer. A NaN time reached osc.start(NaN) and threw inside the tick; an unsorted array made the early-terminating scan drop claps. - Keep the reference recording on a transparent path straight to destination; route only the guide voices through the limiter (guideGain -> limiter -> destination). The limiter no longer colors loud/brickwalled reference audio when claps are off. - Clamp the lookahead window end to the loop end while looping (new _guideWindowEndPure) so no clap is scheduled past the boundary before the rAF wrap cancels it (ghost claps). - Persist the last-fired 1 ms bucket key across ticks so a chord straddling a window boundary can't double-fire; reset on seek/wrap. - Extend guide_clap tests: unsorted+NaN sanitizing and loop-end clamp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/editor-guide-clap
branch
from
July 6, 2026 19:36
31866df to
db00209
Compare
This was referenced Jul 7, 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
2 participants
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.
Summary
The editor has zero note sonification — you cannot verify note placement by ear, which hurts most exactly where charting-by-ear matters (drum placement, dense passages). This PR adds guide claps: a
Clapstransport toggle (shortcutCin both the FeedBack and EOF profiles) that ticks each charted event during playback — drum hits in drum-edit mode, the current arrangement's notes everywhere else.First slice of the DAW-workspace guide-playback design (percussive clap now; pitched per-instrument guide voices ride the same scheduler/bus in a later PR).
Engine
setInterval(25 ms cadence, 120 ms window) anchored on the existing transport anchor (playStartWall/playStartTime) — deliberately not the rAF draw loop, so clap timing stays accurate while the canvas is busy._startAudioSourceAtCursor, which now cancels queued voices and resets the schedule watermark — no ghost claps after a loop wrap or scrub. Timer-stall recovery skips missed past events instead of machine-gunning them late.DynamicsCompressorlimiter (threshold −1, ratio 20) instead of connecting raw todestination.Verification
node --check screen.jscleantests/guide_clap.test.js— 8 cases via the@pure:guide-clapblock: window semantics, half-open boundaries (no double-fire across adjacent windows), chord-stack dedupe vs flam preservation, degenerate inputs, dense-chart binary search, transport-clock mapping🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit