fix: resolve CSP violations blocking Tone.js, Monaco, and Umami; defer AudioContext creation - #188
Conversation
…r AudioContext creation - Add blob: to script-src and connect-src for Tone.js v15 AudioWorklet - Add gateway.umami.is to connect-src for Umami Cloud analytics - Add cdn.jsdelivr.net to style-src for Monaco editor CDN CSS - Replace static Tone.js import with lazy dynamic import to prevent eager AudioContext creation at module load time
✅ Deploy Preview for dev-bayanflow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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:
📝 WalkthroughWalkthroughUpdates Content-Security-Policy header values, changes a Cloudflare preview comment heading, and switches Tone.js access in audio utilities to cached dynamic imports with async call sites. ChangesCSP and preview config
Tone lazy loading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Preview for Bayan Flow Staging ready!
Preview alias |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/soundManager.js (1)
177-192: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMaking
arpeggiateasync breaks the temporary-volume window.
arpeggiateis invoked through the synchronouswithTemporaryVolume(synth, volumeDb, playFn)(lines 143-153), which sets the boosted volume, callsplayFn(), then immediately restores the previous volume. Now thatarpeggiateisasync,playFn()returns at the firstawait getTone()and the volume is reset before anytriggerAttackReleaseruns (the body resumes on a later microtask). All arpeggiated milestone/accent sounds (playSorted,playPassComplete,playPathFound,playTargetFound,playNoResult,playCycle,playComponentComplete) therefore play at base volume instead of the adjusted volume. The returned promise is also never awaited, so any rejection is unhandled.Since
enable()(line 112) and_buildInstruments()(line 58) bothawait getTone()before any playback path runs,_Toneis guaranteed loaded by the timearpeggiateexecutes — keep it synchronous and read_Tonedirectly to preserve the volume window.🐛 Keep arpeggiate synchronous
- async arpeggiate( + arpeggiate( synth, notes, { ascending = true, noteDuration = MILESTONE_NOTE_DURATION } = {} ) { - const Tone = await getTone(); const ordered = ascending ? notes : [...notes].reverse(); - const now = Tone.now(); + const now = _Tone.now();🤖 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 `@src/utils/soundManager.js` around lines 177 - 192, The async change in arpeggiate breaks withTemporaryVolume because playFn returns before the notes are scheduled, so restore-volume runs too early and any rejection becomes unhandled. Keep arpeggiate synchronous by using the already-initialized _Tone directly instead of awaiting getTone(), since enable and _buildInstruments guarantee Tone is loaded before the playback methods call it. Make the fix in arpeggiate and verify the callers like playSorted, playPassComplete, and playTargetFound still benefit from the temporary volume boost.
🧹 Nitpick comments (1)
src/utils/masterChain.js (1)
13-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate lazy-loader across modules.
_Tone+getTone()are defined identically here and insrc/utils/soundManager.js(lines 18-25). Each module gets its own cache, so Tone may be resolved twice and the logic is duplicated. SincesoundManager.jsalready imports from this module, consider exportinggetTonehere and reusing it.♻️ Share a single loader
let _Tone = null; -async function getTone() { +export async function getTone() { if (!_Tone) { _Tone = await import('tone'); } return _Tone; }Then in
soundManager.js, import it instead of redeclaring:-let _Tone = null; - -async function getTone() { - if (!_Tone) { - _Tone = await import('tone'); - } - return _Tone; -} +import { createMasterChain, getTone } from './masterChain.js';🤖 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 `@src/utils/masterChain.js` around lines 13 - 20, Duplicate lazy-loading logic for Tone is defined in both masterChain.js and soundManager.js, which creates separate caches and unnecessary duplication. Move the shared loader to the getTone function in masterChain.js by exporting it there, then update soundManager.js to import and reuse that single helper instead of redeclaring its own _Tone cache. Keep the existing lazy import behavior intact while ensuring both modules share the same resolved Tone instance.
🤖 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 `@netlify.toml`:
- Line 21: The Content-Security-Policy in netlify.toml is still narrower than
the policy in public/_headers, so update the CSP string to match the same
allowed sources. In particular, adjust the script-src and connect-src entries in
the netlify.toml CSP so they include the missing Cloudflare Insights and
jsDelivr endpoints referenced by the deployed app, keeping the policy consistent
with the header-based configuration.
---
Outside diff comments:
In `@src/utils/soundManager.js`:
- Around line 177-192: The async change in arpeggiate breaks withTemporaryVolume
because playFn returns before the notes are scheduled, so restore-volume runs
too early and any rejection becomes unhandled. Keep arpeggiate synchronous by
using the already-initialized _Tone directly instead of awaiting getTone(),
since enable and _buildInstruments guarantee Tone is loaded before the playback
methods call it. Make the fix in arpeggiate and verify the callers like
playSorted, playPassComplete, and playTargetFound still benefit from the
temporary volume boost.
---
Nitpick comments:
In `@src/utils/masterChain.js`:
- Around line 13-20: Duplicate lazy-loading logic for Tone is defined in both
masterChain.js and soundManager.js, which creates separate caches and
unnecessary duplication. Move the shared loader to the getTone function in
masterChain.js by exporting it there, then update soundManager.js to import and
reuse that single helper instead of redeclaring its own _Tone cache. Keep the
existing lazy import behavior intact while ensuring both modules share the same
resolved Tone instance.
🪄 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: d8ceb916-31d5-48de-a8c7-8fdfbb845330
📒 Files selected for processing (4)
netlify.tomlpublic/_headerssrc/utils/masterChain.jssrc/utils/soundManager.js
Add missing https://static.cloudflareinsights.com and https://cdn.jsdelivr.net to script-src, and https://cloudflareinsights.com to connect-src in netlify.toml CSP to match the header-based policy.
Use pre-loaded _Tone directly instead of async getTone(), since _buildInstruments guarantees Tone is loaded before playback callbacks fire.
Move getTone() and _Tone cache into masterChain.js with named exports so both modules resolve the same Tone instance. Update soundManager.test.js mock to provide getTone and _Tone.
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)
src/utils/soundManager.js (1)
111-120: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset the manager state when lazy init fails.
isEnabledis set beforeensureInstrumentsAsync()succeeds, and a rejection leavesinitPromisecached forever. Becausesrc/pages/VisualizerApp.jsx:255-278skips reattaching resume listeners whensoundManager.getIsEnabled()is already true, one failed Tone/init path now wedges sound until reload; laterrunWhenReady()calls also keep chaining onto the same rejected promise.Suggested fix
async enable() { const Tone = await getTone(); if (Tone.context.state !== 'running') { await Tone.start(); } - this.isEnabled = true; this.microEventCounters = {}; this.melodicStepCounter = 0; - await this.ensureInstrumentsAsync(); + try { + await this.ensureInstrumentsAsync(); + this.isEnabled = true; + } catch (error) { + this.isEnabled = false; + this.initPromise = null; + throw error; + } }🤖 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 `@src/utils/soundManager.js` around lines 111 - 120, The SoundManager enable flow leaves the manager marked as enabled before ensureInstrumentsAsync() succeeds, so a failed lazy init can permanently cache a rejected initPromise and block later retries. Update enable() in SoundManager to only set isEnabled and reset counters after ensureInstrumentsAsync() completes, and on any rejection clear or reset the initPromise/internal state so subsequent runWhenReady() calls can retry cleanly. Also ensure the VisualizerApp resume-listener path that checks getIsEnabled() can recover after a failed init instead of treating the manager as already ready.
🤖 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 `@src/utils/soundManager.js`:
- Around line 111-120: The SoundManager enable flow leaves the manager marked as
enabled before ensureInstrumentsAsync() succeeds, so a failed lazy init can
permanently cache a rejected initPromise and block later retries. Update
enable() in SoundManager to only set isEnabled and reset counters after
ensureInstrumentsAsync() completes, and on any rejection clear or reset the
initPromise/internal state so subsequent runWhenReady() calls can retry cleanly.
Also ensure the VisualizerApp resume-listener path that checks getIsEnabled()
can recover after a failed init instead of treating the manager as already
ready.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 466d5f1c-76c5-4e23-8559-74feb12eb2ac
📒 Files selected for processing (1)
src/utils/soundManager.js
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 `@src/utils/soundManager.test.js`:
- Around line 24-25: The mock in soundManager.test.js is splitting getTone() and
_Tone across different instances, so update the Tone stub used by the test to
keep both symbols on the same cached mock module. Adjust the test setup around
getTone and _Tone so masterChain.js exercises the shared-instance contract
consistently and can catch bugs related to reusing the same Tone object.
🪄 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: cd17e58b-6e5a-4a66-8798-12387b06c45a
📒 Files selected for processing (3)
src/utils/masterChain.jssrc/utils/soundManager.jssrc/utils/soundManager.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/masterChain.js
- src/utils/soundManager.js
enable() no longer sets isEnabled=true before ensureInstrumentsAsync() completes. On failure, initPromise and instruments are reset so retries work cleanly instead of being permanently blocked by a cached rejection.
Use a shared mutable object that getTone() populates via Object.assign from the global Tone mock, so _Tone and getTone() return the same instance — matching the real masterChain.js contract.
…gress bar - Add 'wasm-unsafe-eval' to script-src CSP in public/_headers & netlify.toml - Split timeouts: 60s for Pyodide init, 10s for code execution - Worker sends heartbeat during loadPyodide() to prevent false timeouts - Replace loading spinner with indeterminate motion progress bar - Run button shows 'Loading Python...' vs 'Running...' distinctly - Clear stale timeouts on re-run (fixes double-click bug)
1 participant
Description
Three CSP directives were missing entries required by production dependencies, causing Tone.js AudioWorklet initialization to fail (AbortError), the Monaco editor code panel to render unstyled, and Umami analytics to drop events. Additionally, Tone.js eagerly created an AudioContext at module-import time, producing autoplay policy warnings.
Type of Change
Changes Made
public/_headers— Addedblob:toscript-srcandconnect-srcfor Tone.js v15 AudioWorklet modules; addedhttps://gateway.umami.istoconnect-srcfor Umami Cloud data ingestion; addedhttps://cdn.jsdelivr.nettostyle-srcfor Monaco editor CDN CSS.netlify.toml— Same CSP updates for the legacy Netlify deployment.src/utils/masterChain.js— Replaced staticimport * as Tone from "tone"with lazy asyncgetTone()using dynamicimport("tone")so no AudioContext is created at module-load time.src/utils/soundManager.js— Same lazy import pattern;Tone.*references in_buildInstruments(),enable(), andarpeggiate()now resolve viagetTone()after user gesture.Testing
pnpm test:run) — 119 files, 1563 testspnpm lint)pnpm format:check)pnpm build)dist/_headersverified for correct CSP directivesTest Results
Checklist
developRelated Issues
Fixes CSP violations that broke: Tone.js sound system, Monaco editor code panel styling, and Umami analytics tracking on the deployed environment.
Summary by CodeRabbit
script-srcandconnect-srcsources (includingblob:and analytics endpoints) to prevent blocked scripts/connections.