Skip to content

fix(playback): throttle legacy bridge-hit recording; emit loop-set for manual A/B - #811

Merged
byrongamatos merged 2 commits into
mainfrom
fix/loop-api-bridge-throttle
Jul 7, 2026
Merged

byrongamatos merged 2 commits into
mainfrom
fix/loop-api-bridge-throttle

Conversation

@OmikronApex

@OmikronApex OmikronApex commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Problem

The capability inspector showed playback.loop-api firing dozens of times per second — even with no song playing.

Root cause chain:

  • note_detect's HUD ticks every 33 ms and called window.slopsmith.getLoop() per tick (a legitimately cheap read when it was written).
  • Core instruments every window.feedBack.getLoop() call as a legacy bridge hit: compat-shim bookkeeping, a playback:bridge-hit event, and a diagnostics snapshot rebuild + JSON.stringify per call.
  • Result: ~30 snapshot serializations/sec on the main thread while idle, and a saturated hitCount drowning genuine legacy-surface signal.

Fix

  • _recordPlaybackBridge now throttles per bridgeId|surface (5 s window). First call records immediately; repeats inside the window are dropped. Bridge hits are a "this surface is still in use" signal, not a call counter.
  • setLoopEnd() (manual A/B buttons) now emits the same loop-set transport event as setLoop(), so plugins can go event-driven via playback:loop-set / playback:loop-cleared instead of polling getLoop() — polling was the only way to see button-armed loops before.

Companion PR in note_detect makes its drill sync event-driven with a 1 Hz fallback poll.

Testing

  • node --check static/app.js
  • All 924 JS tests pass (node --test tests/js/*.test.js)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Throttled repeated legacy playback bridge hits to reduce overhead by limiting repeat bridge polling per bridge/surface within a 5-second window.
    • Updated manual A/B loop end control so it emits the same transport loop-set event as other loop actions (including the active loop payload), enabling plugins to react without polling.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d79f1202-4b5a-4d29-8185-8a974771fcc3

📥 Commits

Reviewing files that changed from the base of the PR and between 37eeb9c and 8c23270.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • static/app.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • static/app.js

📝 Walkthrough

Walkthrough

The playback bridge recording function now throttles repeated hits per bridge/surface pair. The manual A/B loop control setLoopEnd() now emits a loop-set transport event, and the changelog records both changes.

Changes

Playback bridge and loop event fixes

Layer / File(s) Summary
Bridge hit throttling
static/app.js
_recordPlaybackBridge() now uses a Map to throttle recordBridgeHit calls per (bridgeId, legacySurface) pair to a 5s minimum interval.
Loop-set transport event emission
static/app.js, CHANGELOG.md
setLoopEnd() now dispatches playback.transportEvent('loop-set', ...) with active loopA/loopB state, and the changelog documents both changes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant AppJS as static/app.js
    participant Plugin

    User->>AppJS: setLoopEnd()
    AppJS->>AppJS: arm loopA/loopB
    AppJS->>Plugin: transportEvent('loop-set', loopA, loopB, loop payload)
    Plugin->>Plugin: react to playback:loop-set
Loading
🚥 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 summarizes the main changes: throttling legacy bridge-hit recording and emitting loop-set for manual A/B loops.
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 fix/loop-api-bridge-throttle

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

OmikronApex and others added 2 commits July 7, 2026 23:37
…r manual A/B

window.feedBack.getLoop() is a read surface plugins legitimately poll
(note_detect HUD ticked it at ~30 Hz), but every call recorded a
playback.loop-api bridge hit: compat-shim bookkeeping, a
playback:bridge-hit event, and a diagnostics snapshot rebuild +
stringify per call — real main-thread cost and a saturated hitCount in
the capability inspector, even with no song playing.

- _recordPlaybackBridge now throttles per bridgeId|surface (5 s window).
  Bridge hits are a 'surface still in use' signal, not a call counter.
- setLoopEnd() (manual A/B buttons) now emits the same loop-set
  transport event as setLoop(), so event-driven consumers no longer
  need to poll getLoop() to see button-armed loops.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@OmikronApex
OmikronApex force-pushed the fix/loop-api-bridge-throttle branch from 37eeb9c to 8c23270 Compare July 7, 2026 21:37

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
static/app.js (1)

5539-5563: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Throttle logic looks correct.

Per-key throttling with a 5s window is implemented correctly (check-then-set ordering avoids allowing bursts). One minor note: _bridgeRecordLast is a module-level Map with no eviction, unlike playback.js's own bridges map which caps size via MAX_BRIDGES. In practice the key space (bridgeId/legacySurface pairs) is likely small and bounded, so this is unlikely to be a real problem, but consider capping it defensively if bridge identifiers can ever be dynamic/unbounded.

🤖 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 `@static/app.js` around lines 5539 - 5563, The per-surface throttling in
_recordPlaybackBridge uses a module-level _bridgeRecordLast Map that never
evicts entries, so add a bounded cleanup strategy or size cap similar to
playback.js’s bridged state handling. Update _recordPlaybackBridge and
_bridgeRecordLast so old bridgeId|legacySurface keys are removed or the map is
capped when identifiers can grow unbounded, keeping the 5s throttle behavior
intact.
🤖 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 `@static/app.js`:
- Around line 5539-5563: The per-surface throttling in _recordPlaybackBridge
uses a module-level _bridgeRecordLast Map that never evicts entries, so add a
bounded cleanup strategy or size cap similar to playback.js’s bridged state
handling. Update _recordPlaybackBridge and _bridgeRecordLast so old
bridgeId|legacySurface keys are removed or the map is capped when identifiers
can grow unbounded, keeping the 5s throttle behavior intact.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e3ee0ae2-7f15-41dd-968f-78b0e7b1d0d2

📥 Commits

Reviewing files that changed from the base of the PR and between 115c352 and 37eeb9c.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • static/app.js

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.

2 participants