refactor(app): give the <audio> element a module of its own (R3a)#886
Conversation
📝 WalkthroughWalkthroughThe audio element lookup now lives in ChangesAudio binding modularization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. 🔧 ESLint
static/js/host.jsOops! Something went wrong! :( ESLint: 9.39.4 No files matching the pattern "static/js/host.js" were found. static/js/section-practice.jsOops! Something went wrong! :( ESLint: 9.39.4 No files matching the pattern "static/js/section-practice.js" were found. tests/js/host_contract.test.jsOops! Something went wrong! :( ESLint: 9.39.4 No files matching the pattern "tests/js/host_contract.test.js" were found. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/js/tuner_auto_open.test.js (1)
17-30: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAttach
tuning-display.jshelpers intests/js/tuner_auto_open.test.js:17-30
loadTuningHelpers()returnssandbox.window.feedBack, butstatic/js/tuning-display.jsonly exports functions; it doesn’t populatewindow.feedBack. That leaves the sandbox empty here, soplugins/tuner/screen.jsfalls back fromsongTuningContext/isBassArrangement/effectiveStringCountinstead of using the real helpers. Wire those functions ontowindow.feedBack(or loadstatic/app.jsfirst).🤖 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/js/tuner_auto_open.test.js` around lines 17 - 30, Update loadTuningHelpers so the evaluated tuning-display.js exports are attached to sandbox.window.feedBack before returning it, ensuring songTuningContext, isBassArrangement, and effectiveStringCount resolve to the real helpers instead of screen.js fallbacks.
🤖 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 `@static/js/plugin-loader.js`:
- Around line 822-824: Validate the fetch response in the plugin update-loading
flow before parsing it, treating any non-2xx response as a failure rather than
defaulting to an empty updates object. Preserve the existing updates handling
for successful responses and route failed responses through the flow’s
established error handling.
- Around line 847-862: Update updatePlugin so btn.disabled is set back to false
in both the data.ok === false branch and the catch block, while preserving the
disabled state after a successful update.
- Around line 834-836: Replace the innerHTML construction in the plugin row
rendering with DOM element creation and textContent assignments for all
API-derived values, including u.name, u.behind, u.local, and u.remote. Create
the Update button without interpolating id into HTML or JavaScript, and attach
its handler via addEventListener while preserving the existing updatePlugin(id,
button) behavior and styling.
In `@static/js/settings-io.js`:
- Around line 50-55: Update the localStorageData accumulator in the export loop
to use a null-prototype object, ensuring an own property named __proto__ is
stored and included in the exported bundle. Preserve the existing key and value
filtering and assignment behavior.
---
Outside diff comments:
In `@tests/js/tuner_auto_open.test.js`:
- Around line 17-30: Update loadTuningHelpers so the evaluated tuning-display.js
exports are attached to sandbox.window.feedBack before returning it, ensuring
songTuningContext, isBassArrangement, and effectiveStringCount resolve to the
real helpers instead of screen.js fallbacks.
🪄 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: 6af827b3-2951-4863-ba25-43310ad7a711
📒 Files selected for processing (9)
static/app.jsstatic/js/audio-el.jsstatic/js/plugin-loader.jsstatic/js/settings-io.jsstatic/js/tuning-display.jstests/js/tuner_auto_open.test.jstests/js/tuning_display.test.jstests/js/tuning_targets.test.jstests/js/v3_songs_tuning.test.js
| const resp = await fetch('/api/plugins/updates'); | ||
| const data = await resp.json(); | ||
| const updates = data.updates || {}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle non-2xx responses as failures.
fetch() does not reject on HTTP errors. A backend or demo-mode error response without updates is currently interpreted as {}, causing the UI to falsely report that all plugins are up to date.
const resp = await fetch('/api/plugins/updates');
+ if (!resp.ok) {
+ throw new Error(`Update check failed: ${resp.status}`);
+ }
const data = await resp.json();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resp = await fetch('/api/plugins/updates'); | |
| const data = await resp.json(); | |
| const updates = data.updates || {}; | |
| const resp = await fetch('/api/plugins/updates'); | |
| if (!resp.ok) { | |
| throw new Error(`Update check failed: ${resp.status}`); | |
| } | |
| const data = await resp.json(); | |
| const updates = data.updates || {}; |
🤖 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/js/plugin-loader.js` around lines 822 - 824, Validate the fetch
response in the plugin update-loading flow before parsing it, treating any
non-2xx response as a failure rather than defaulting to an empty updates object.
Preserve the existing updates handling for successful responses and route failed
responses through the flow’s established error handling.
| row.innerHTML = ` | ||
| <span class="text-sm text-gray-300 flex-1">${u.name} <span class="text-xs text-gray-500">(${u.behind} commit${u.behind > 1 ? 's' : ''} behind — ${u.local} → ${u.remote})</span></span> | ||
| <button onclick="updatePlugin('${id}', this)" class="bg-accent/20 hover:bg-accent/30 text-accent-light px-3 py-1 rounded-lg text-xs transition">Update</button>`; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent XSS when rendering plugin metadata.
API values are interpolated into innerHTML and an inline onclick handler. A plugin name, ref, or ID containing markup or quotes can execute JavaScript in the Settings page. Build these nodes with textContent and attach the click handler with addEventListener; do not interpolate API data into HTML/JavaScript contexts.
Proposed fix
- row.innerHTML = `
- <span class="text-sm text-gray-300 flex-1">${u.name} <span class="text-xs text-gray-500">(${u.behind} commit${u.behind > 1 ? 's' : ''} behind — ${u.local} → ${u.remote})</span></span>
- <button onclick="updatePlugin('${id}', this)" class="bg-accent/20 hover:bg-accent/30 text-accent-light px-3 py-1 rounded-lg text-xs transition">Update</button>`;
+ const label = document.createElement('span');
+ label.className = 'text-sm text-gray-300 flex-1';
+ const details = document.createElement('span');
+ details.className = 'text-xs text-gray-500';
+ details.textContent = `(${u.behind} commit${u.behind > 1 ? 's' : ''} behind — ${u.local} → ${u.remote})`;
+ label.append(document.createTextNode(`${u.name} `), details);
+
+ const updateButton = document.createElement('button');
+ updateButton.type = 'button';
+ updateButton.className = 'bg-accent/20 hover:bg-accent/30 text-accent-light px-3 py-1 rounded-lg text-xs transition';
+ updateButton.textContent = 'Update';
+ updateButton.addEventListener('click', () => updatePlugin(id, updateButton));
+ row.append(label, updateButton);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| row.innerHTML = ` | |
| <span class="text-sm text-gray-300 flex-1">${u.name} <span class="text-xs text-gray-500">(${u.behind} commit${u.behind > 1 ? 's' : ''} behind — ${u.local} → ${u.remote})</span></span> | |
| <button onclick="updatePlugin('${id}', this)" class="bg-accent/20 hover:bg-accent/30 text-accent-light px-3 py-1 rounded-lg text-xs transition">Update</button>`; | |
| const label = document.createElement('span'); | |
| label.className = 'text-sm text-gray-300 flex-1'; | |
| const details = document.createElement('span'); | |
| details.className = 'text-xs text-gray-500'; | |
| details.textContent = `(${u.behind} commit${u.behind > 1 ? 's' : ''} behind — ${u.local} → ${u.remote})`; | |
| label.append(document.createTextNode(`${u.name} `), details); | |
| const updateButton = document.createElement('button'); | |
| updateButton.type = 'button'; | |
| updateButton.className = 'bg-accent/20 hover:bg-accent/30 text-accent-light px-3 py-1 rounded-lg text-xs transition'; | |
| updateButton.textContent = 'Update'; | |
| updateButton.addEventListener('click', () => updatePlugin(id, updateButton)); | |
| row.append(label, updateButton); |
🤖 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/js/plugin-loader.js` around lines 834 - 836, Replace the innerHTML
construction in the plugin row rendering with DOM element creation and
textContent assignments for all API-derived values, including u.name, u.behind,
u.local, and u.remote. Create the Update button without interpolating id into
HTML or JavaScript, and attach its handler via addEventListener while preserving
the existing updatePlugin(id, button) behavior and styling.
Source: Linters/SAST tools
| export async function updatePlugin(pluginId, btn) { | ||
| btn.disabled = true; | ||
| btn.textContent = 'Updating...'; | ||
| try { | ||
| const resp = await fetch(`/api/plugins/${pluginId}/update`, { method: 'POST' }); | ||
| const data = await resp.json(); | ||
| if (data.ok) { | ||
| btn.textContent = 'Updated — restart to apply'; | ||
| btn.className = 'bg-green-900/30 text-green-400 px-3 py-1 rounded-lg text-xs'; | ||
| } else { | ||
| btn.textContent = 'Failed'; | ||
| btn.title = data.error || ''; | ||
| } | ||
| } catch (e) { | ||
| btn.textContent = 'Error'; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Re-enable the update button after failures.
The button remains disabled when the server returns data.ok === false or when the request throws, so users cannot retry without reloading the page. Re-enable it in both failure paths while keeping it disabled after a successful update.
} else {
btn.textContent = 'Failed';
btn.title = data.error || '';
+ btn.disabled = false;
}
} catch (e) {
btn.textContent = 'Error';
+ btn.disabled = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function updatePlugin(pluginId, btn) { | |
| btn.disabled = true; | |
| btn.textContent = 'Updating...'; | |
| try { | |
| const resp = await fetch(`/api/plugins/${pluginId}/update`, { method: 'POST' }); | |
| const data = await resp.json(); | |
| if (data.ok) { | |
| btn.textContent = 'Updated — restart to apply'; | |
| btn.className = 'bg-green-900/30 text-green-400 px-3 py-1 rounded-lg text-xs'; | |
| } else { | |
| btn.textContent = 'Failed'; | |
| btn.title = data.error || ''; | |
| } | |
| } catch (e) { | |
| btn.textContent = 'Error'; | |
| } | |
| export async function updatePlugin(pluginId, btn) { | |
| btn.disabled = true; | |
| btn.textContent = 'Updating...'; | |
| try { | |
| const resp = await fetch(`/api/plugins/${pluginId}/update`, { method: 'POST' }); | |
| const data = await resp.json(); | |
| if (data.ok) { | |
| btn.textContent = 'Updated — restart to apply'; | |
| btn.className = 'bg-green-900/30 text-green-400 px-3 py-1 rounded-lg text-xs'; | |
| } else { | |
| btn.textContent = 'Failed'; | |
| btn.title = data.error || ''; | |
| btn.disabled = false; | |
| } | |
| } catch (e) { | |
| btn.textContent = 'Error'; | |
| btn.disabled = false; | |
| } |
🤖 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/js/plugin-loader.js` around lines 847 - 862, Update updatePlugin so
btn.disabled is set back to false in both the data.ok === false branch and the
catch block, while preserving the disabled state after a successful update.
| const localStorageData = {}; | ||
| for (let i = 0; i < localStorage.length; i++) { | ||
| const key = localStorage.key(i); | ||
| if (key === null) continue; | ||
| const value = localStorage.getItem(key); | ||
| if (value !== null) localStorageData[key] = value; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve __proto__ localStorage keys during export.
Because localStorageData is a normal object, assigning a valid key named __proto__ invokes the inherited setter instead of creating an own property, so that key is omitted from the exported bundle. Use a null-prototype object to preserve round-trip fidelity.
Proposed fix
- const localStorageData = {};
+ const localStorageData = Object.create(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const localStorageData = {}; | |
| for (let i = 0; i < localStorage.length; i++) { | |
| const key = localStorage.key(i); | |
| if (key === null) continue; | |
| const value = localStorage.getItem(key); | |
| if (value !== null) localStorageData[key] = value; | |
| const localStorageData = Object.create(null); | |
| for (let i = 0; i < localStorage.length; i++) { | |
| const key = localStorage.key(i); | |
| if (key === null) continue; | |
| const value = localStorage.getItem(key); | |
| if (value !== null) localStorageData[key] = value; |
🤖 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/js/settings-io.js` around lines 50 - 55, Update the localStorageData
accumulator in the export loop to use a null-prototype object, ensuring an own
property named __proto__ is stored and included in the exported bundle. Preserve
the existing key and value filtering and assignment behavior.
static/js/audio-el.js — one exported const. app.js's diff is 5 lines.
This is a HINGE, not a carve: nothing shrinks, but almost everything left in
app.js is blocked behind it.
WHY. `audio` is `document.getElementById('audio')` with 162 references in app.js
and 173 outside any one cluster. Every remaining cluster measured — settings,
app-updates, count-in (208 fns), exit-confirm (212), library-render (220) — lists
`audio` among its inbound symbols, because they all touch playback and playback
reaches for the element directly. A module that needs it cannot import app.js to
get it (that closes a cycle and fails import-x/no-cycle), so today the only way to
carve any of them would be a host seam — the exact thing #878 had to build and
#880 had to tear out.
WHY IT'S SAFE. `audio` is a `const` and is NEVER reassigned anywhere in core, so a
read-only import binding is exactly right and no state container is needed. The
162 call sites are untouched — the binding keeps its name, it is just imported
instead of declared. (Contrast the reassigned scalars — isPlaying, _avOffsetMs —
which CANNOT be shared this way: an imported binding cannot be written to. Those
still need containers, and that is the next problem, not this one.)
TIMING. app.js is <script type="module">, so it evaluates after the HTML is parsed
and its imports evaluate just before its body — the same moment app.js used to run
this exact lookup. If the element had not been in the document, `audio` would be
null and app.js's top-level `audio.addEventListener(...)` calls would throw and
kill the module. They don't.
VERIFIED WITH REAL PLAYBACK, not a boot check. A/B against origin/main in two
browsers: app alive with zero page errors (which is itself the proof the import
resolved), #audio is an AUDIO element, togglePlay/seekBy live, and playSong() on a
real library song sets audio.src and the element reports a duration — IDENTICAL on
both sides.
pytest 2396, node 1038/1038, ESLint 0 (no-cycle clean), tailwind clean, Codex 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0c9e7ff to
4bf4ec0
Compare
1 participant
static/js/audio-el.js— one exported const. app.js's diff is 5 lines.This is a hinge, not a carve. Nothing shrinks. But almost everything left in app.js is blocked behind it.
Why
audioisdocument.getElementById('audio')— 162 references in app.js, 173 outside any single cluster.Every remaining cluster I measured lists
audioamong its inbound symbols:audioaudioaudioaudioThey all touch playback, and playback reaches for the element directly. A carved module can't import app.js to get it — that closes a cycle and fails
import-x/no-cycle. So today the only way to carve any of them is a host seam: exactly the thing #878 had to build and #880 had to tear out.Giving
audioa home removes that from every one of them at once.Why it's safe
audiois aconst, and it is never reassigned anywhere in core. So a read-only import binding is precisely right — no state container needed. The 162 call sites are untouched: the binding keeps its name, it's just imported instead of declared.Contrast the reassigned scalars (
isPlaying,_avOffsetMs, …). Those cannot be shared this way, because an imported binding cannot be written to. They still need containers — that's the next problem, and deliberately not this one.Timing
app.js is
<script type="module">, so it evaluates after the HTML is parsed, and its imports evaluate just before its body — the same moment app.js used to run this exact lookup itself.If the element weren't in the document yet,
audiowould benulland app.js's top-levelaudio.addEventListener(...)calls would throw and kill the whole module. They don't — which is itself the proof.Verified with real playback
A/B against
origin/mainin two browsers:#audioelementAUDIOtogglePlay/seekByfunctionplaySong()on a real library song →audio.srcsetpytest 2396 · node 1038/1038 · ESLint 0 (no-cycle clean) · tailwind clean · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit