From f7b065d2362c3fa70a4978f2d9f200b78438d581 Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Thu, 16 Jul 2026 11:36:27 +0200 Subject: [PATCH] fix(plugins): restore window.esc for out-of-tree plugins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit app.js exported `esc` as an implicit global back when it was a classic script. a9fce29 made it an ES module and 14b4058 carved `esc` into js/dom.js; the window re-export list was rebuilt without it. Out-of-tree plugins load screen.js as a classic script and call `esc()` bare, so nothing in-tree catches the break: no-undef, a call-graph scan and a grep all pass while the plugin throws in the field. The MIDI plugin builds its device list with esc() inside the same try block that catches requestMIDIAccess() failures, so the ReferenceError surfaced to testers as "MIDI Access denied esc is not defined" — access had actually been granted. Pin the whole plugin-facing global surface by name, mirroring tests/test_plugin_context_contract.py. Verified both ways against a running app: without the fix the spec fails with "missing or not functions: esc". Co-Authored-By: Claude Opus 4.8 (1M context) --- static/app.js | 5 +- tests/browser/plugin-globals-contract.spec.ts | 63 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 tests/browser/plugin-globals-contract.spec.ts diff --git a/static/app.js b/static/app.js index 8d606083..d3062590 100644 --- a/static/app.js +++ b/static/app.js @@ -2319,11 +2319,14 @@ configureHost({ currentFilename: () => currentFilename, }); +// `esc` is here for out-of-tree plugins only: their screen.js loads as a classic +// script and called esc() back when app.js was one too and it was an implicit +// global. Nothing in core reads window.esc — import it from ./js/dom.js instead. Object.assign(window, { _confirmDialog, _getArrangementNamingMode, _libraryLocalFilename, _librarySongArtUrl, _librarySongId, _onHeaderClick, _onNamingModeChange, _trapFocusInModal, changeArrangement, checkPluginUpdates, clearLibFilters, clearLoop, - deleteSelectedLoop, exportDiagnostics, exportSettings, filterFavorites, + deleteSelectedLoop, esc, exportDiagnostics, exportSettings, filterFavorites, filterLibrary, fullRescanLibrary, goFavPage, handleSliderInput, hideScanBanner, importSettings, loadPlugins, loadSavedLoop, loadSettings, onSectionPracticeModeChange, openEditModal, persistSetting, diff --git a/tests/browser/plugin-globals-contract.spec.ts b/tests/browser/plugin-globals-contract.spec.ts new file mode 100644 index 00000000..0915016d --- /dev/null +++ b/tests/browser/plugin-globals-contract.spec.ts @@ -0,0 +1,63 @@ +// The window globals are a THIRD-PARTY CONTRACT. Pin them. +// +// Out-of-tree plugins load their screen.js as a CLASSIC script and call these +// as bare globals. Nothing in core reads most of them, so a call-graph scan, +// ESLint's no-undef, and a grep all come back clean while the plugin breaks in +// the field. This is the frontend twin of tests/test_plugin_context_contract.py +// — same reasoning, same literal-list rule. +// +// This guard is retroactive: `esc` was an implicit global back when app.js was +// a classic script, went module-scoped in a9fce29, and got carved into +// js/dom.js in 14b4058. The re-export list at the bottom of app.js was rebuilt +// without it, and the MIDI plugin's device list threw "esc is not defined" for +// testers — reported as "MIDI Access denied", because the ReferenceError landed +// in a try/catch meant for permission failures. +// +// WHY A LITERAL LIST AND NOT A DERIVED ONE. Deriving the expected set from +// app.js would assert the code equals itself. The point is that a human has to +// look at a diff and consciously agree to change the contract. + +import { test, expect } from '@playwright/test'; + +const PLUGIN_GLOBALS = [ + '_confirmDialog', '_getArrangementNamingMode', '_libraryLocalFilename', '_librarySongArtUrl', + '_librarySongId', '_onHeaderClick', '_onNamingModeChange', '_trapFocusInModal', + 'changeArrangement', 'checkPluginUpdates', 'clearLibFilters', 'clearLoop', + 'deleteSelectedLoop', 'esc', 'exportDiagnostics', 'exportSettings', 'filterFavorites', + 'filterLibrary', 'fullRescanLibrary', 'goFavPage', 'handleSliderInput', + 'hideScanBanner', 'importSettings', 'loadPlugins', 'loadSavedLoop', + 'loadSettings', 'onSectionPracticeModeChange', 'openEditModal', 'persistSetting', + 'pickDlcFolder', 'pinCurrentArrangementDefault', 'playSong', 'previewDiagnostics', + 'previewEditArt', 'renderGridCards', 'renderTreeInto', 'rescanLibrary', + 'retuneSong', 'saveCurrentLoop', 'saveSettings', 'seekBy', + 'setAvOffsetMs', 'setFavView', 'setInstrumentPathway', 'setLibView', + 'setLibraryProvider', 'setLoopEnd', 'setLoopStart', 'setMastery', + 'setSpeed', 'setViz', 'showScreen', 'sortFavorites', + 'sortLibrary', 'syncLibrarySong', 'toggleAllArtists', 'toggleAllFavoriteArtists', + 'toggleLibFilters', 'togglePlay', 'toggleSectionPracticePopover', 'uiPrompt', + 'updatePlugin', 'uploadSongs', + 'filterFavTreeLetter', 'filterTreeLetter', 'goFavTreePage', 'goTreePage', +]; + +test('plugin-facing window globals are all callable', async ({ page }) => { + await page.goto('/'); + await page.waitForSelector('.screen.active', { timeout: 10000 }); + + const missing = await page.evaluate( + (names) => names.filter((n) => typeof (window as any)[n] !== 'function'), + PLUGIN_GLOBALS, + ); + + expect(missing, `window globals plugins depend on are missing or not functions: ${missing.join(', ')}`).toEqual([]); +}); + +// The plugin call site that actually broke: esc() interpolated into a template +// string. A global that exists but doesn't escape is its own bug. +test('window.esc escapes HTML metacharacters', async ({ page }) => { + await page.goto('/'); + await page.waitForSelector('.screen.active', { timeout: 10000 }); + + const escaped = await page.evaluate(() => (window as any).esc('')); + expect(escaped).not.toContain('