mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-11 11:19:24 +00:00
fix(plugins): don't treat transient absence from /api/plugins as uninstall (#741)
* fix(plugins): don't treat transient absence from /api/plugins as uninstall The backend clears its plugin registry at the start of load_plugins() and repopulates it incrementally while HTTP stays up, so every backend restart (desktop: Audio Quality soundfont switch, LAN toggle, update restart) serves a window of partial — even empty — /api/plugins responses. loadPlugins() treated absence from the current response as an uninstall, with three destructive consequences for still-loaded plugins: 1. Their settings-panel and screen DOM were wiped while their _loadedPluginScripts entry survived, so the NEXT refetch failed the DOM-existence check and re-evaluated the plugin's screen.js mid-session. For the desktop audio_engine plugin that re-ran init() against the surviving native audio chain and exactly duplicated every VST/NAM/IR stage (the alpha testers' "chain duplicates after leaving the Audio menu" / blown-out gain reports). 2. _reconcilePluginStyles dropped their stylesheet, leaving them visible but unstyled until they reappeared. 3. The stale-contribution sweep unmounted their UI contributions and unregistered their capability participant with no re-registration path (plugin scripts don't re-run thanks to the loadedScripts guard). Absence is now a non-signal everywhere in loadPlugins: the DOM wipe and style reconcile are scoped to plugins the response actually names, and the absence sweep is removed. Present plugins still fully re-sync via _registerLegacyPluginUiContributions each round; failed plugins are present in the response and still cleaned up; nav is rebuilt from the response so genuinely uninstalled plugins drop out of it, and their (un-unloadable) already-evaluated scripts keep their DOM until reload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: update idempotence contract to the absence-is-not-uninstall invariant The removed-plugin sweep contract pinned the old behavior this branch deletes; pin the new invariant instead (no absence sweep + respondedIds scoping on the DOM/style reconcilers). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
d2b2a7e9f7
commit
68e29a8b6e
+41
-21
@@ -10977,20 +10977,19 @@ async function loadPlugins() {
|
|||||||
const nameDelta = String(a.name || a.id || '').localeCompare(String(b.name || b.id || ''));
|
const nameDelta = String(a.name || a.id || '').localeCompare(String(b.name || b.id || ''));
|
||||||
return nameDelta || String(a.id || '').localeCompare(String(b.id || ''));
|
return nameDelta || String(a.id || '').localeCompare(String(b.id || ''));
|
||||||
});
|
});
|
||||||
const livePluginIds = new Set(plugins.map((plugin) => plugin.id));
|
// NOTE deliberately NO stale-contribution sweep for plugins absent
|
||||||
for (const [pluginId, contributions] of _pluginUiContributions) {
|
// from this response. Absent ≠ uninstalled: the backend clears its
|
||||||
if (livePluginIds.has(pluginId)) continue;
|
// plugin registry at the start of load_plugins() and repopulates it
|
||||||
const stalePlugin = { id: pluginId };
|
// incrementally while HTTP stays up, so every backend restart serves a
|
||||||
for (const contribution of contributions) {
|
// window of partial (even empty) responses. The old sweep unmounted UI
|
||||||
await _commandUiDomain(contribution.domain, 'unmount', stalePlugin, contribution);
|
// contributions and unregistered capability participants on mere
|
||||||
}
|
// absence, permanently breaking still-loaded plugins — their scripts
|
||||||
try {
|
// don't re-run (loadedScripts guard below), so nothing ever
|
||||||
window.feedBack?.capabilities?.unregisterParticipant?.(pluginId);
|
// re-registered. A genuine mid-session uninstall now leaves the
|
||||||
} catch (e) {
|
// (already-evaluated, un-unloadable) script's contributions in place
|
||||||
console.warn(`capability participant unregister failed for ${pluginId}:`, e);
|
// until reload; its nav entry still disappears because nav is rebuilt
|
||||||
}
|
// from the response each round. Same invariant as the settings/screen
|
||||||
_pluginUiContributions.delete(pluginId);
|
// DOM wipe and _reconcilePluginStyles below.
|
||||||
}
|
|
||||||
console.log('[feedBack] loadPlugins: got', plugins.length, 'plugins');
|
console.log('[feedBack] loadPlugins: got', plugins.length, 'plugins');
|
||||||
|
|
||||||
try {
|
try {
|
||||||
@@ -11132,17 +11131,23 @@ async function loadPlugins() {
|
|||||||
loadedStyles.set(plugin.id, wantedVersion);
|
loadedStyles.set(plugin.id, wantedVersion);
|
||||||
};
|
};
|
||||||
const _reconcilePluginStyles = (currentPlugins) => {
|
const _reconcilePluginStyles = (currentPlugins) => {
|
||||||
// Drop stylesheets for plugins that vanished from /api/plugins or are
|
// Drop stylesheets for plugins the response KNOWS about but that
|
||||||
// no longer ready+styled this round. _injectPluginStyles below only
|
// are no longer ready+styled this round. _injectPluginStyles below
|
||||||
// visits plugins still returned by the API, so an uninstalled or
|
// only visits plugins still returned by the API, so a newly-not-
|
||||||
// newly-not-ready plugin would otherwise keep its <link> applying.
|
// ready or unstyled plugin would otherwise keep its <link>
|
||||||
|
// applying. Plugins merely ABSENT from the response keep their
|
||||||
|
// stylesheet — a transient partial response during a backend
|
||||||
|
// restart is not an uninstall (same invariant as the screen/
|
||||||
|
// settings wipe below), and stripping the <link> would leave a
|
||||||
|
// still-loaded plugin visible but unstyled.
|
||||||
|
const responded = new Set(currentPlugins.map((p) => p.id));
|
||||||
const styled = new Set(
|
const styled = new Set(
|
||||||
currentPlugins
|
currentPlugins
|
||||||
.filter((p) => (p.status || 'ready') === 'ready' && p.has_styles && p.styles)
|
.filter((p) => (p.status || 'ready') === 'ready' && p.has_styles && p.styles)
|
||||||
.map((p) => p.id),
|
.map((p) => p.id),
|
||||||
);
|
);
|
||||||
for (const id of Array.from(loadedStyles.keys())) {
|
for (const id of Array.from(loadedStyles.keys())) {
|
||||||
if (!styled.has(id)) {
|
if (responded.has(id) && !styled.has(id)) {
|
||||||
_removePluginStyleTags(id);
|
_removePluginStyleTags(id);
|
||||||
loadedStyles.delete(id);
|
loadedStyles.delete(id);
|
||||||
}
|
}
|
||||||
@@ -11155,6 +11160,18 @@ async function loadPlugins() {
|
|||||||
if (pid) existingSettingsByPluginId.set(pid, child);
|
if (pid) existingSettingsByPluginId.set(pid, child);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// Plugins named in THIS response. A plugin can be transiently absent
|
||||||
|
// from /api/plugins — the backend clears its registry at the start of
|
||||||
|
// load_plugins() and repopulates it incrementally while HTTP stays up,
|
||||||
|
// so every backend restart serves a window of partial (even empty)
|
||||||
|
// responses. The wipe loops below must never treat that absence as an
|
||||||
|
// uninstall: stripping a still-loaded plugin's DOM while keeping its
|
||||||
|
// loadedScripts entry made the NEXT refetch fail the DOM check and
|
||||||
|
// re-evaluate its screen.js mid-session — which duplicated the desktop
|
||||||
|
// audio_engine's native signal chain (its init re-ran against the
|
||||||
|
// surviving engine chain). Absent plugins keep their DOM and script;
|
||||||
|
// they're re-reconciled when they reappear in a later response.
|
||||||
|
const respondedIds = new Set(plugins.map((p) => p.id));
|
||||||
const alreadyHydrated = new Set();
|
const alreadyHydrated = new Set();
|
||||||
for (const p of plugins) {
|
for (const p of plugins) {
|
||||||
if (!p.has_script) continue;
|
if (!p.has_script) continue;
|
||||||
@@ -11182,7 +11199,10 @@ async function loadPlugins() {
|
|||||||
for (const container of _pluginSettingsContainers()) {
|
for (const container of _pluginSettingsContainers()) {
|
||||||
[...container.children].forEach((el) => {
|
[...container.children].forEach((el) => {
|
||||||
const pid = el.dataset ? el.dataset.pluginId : null;
|
const pid = el.dataset ? el.dataset.pluginId : null;
|
||||||
if (!pid || !alreadyHydrated.has(pid)) el.remove();
|
// Remove junk (no plugin id) and plugins the response KNOWS
|
||||||
|
// about but that failed hydration; leave plugins absent from
|
||||||
|
// the response untouched (see respondedIds above).
|
||||||
|
if (!pid || (respondedIds.has(pid) && !alreadyHydrated.has(pid))) el.remove();
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
document.querySelectorAll('.screen[id^="plugin-"]').forEach((el) => {
|
document.querySelectorAll('.screen[id^="plugin-"]').forEach((el) => {
|
||||||
@@ -11191,7 +11211,7 @@ async function loadPlugins() {
|
|||||||
// change shipped — both forms strip a single leading "plugin-".
|
// change shipped — both forms strip a single leading "plugin-".
|
||||||
const pid = (el.dataset && el.dataset.pluginId)
|
const pid = (el.dataset && el.dataset.pluginId)
|
||||||
|| el.id.replace(/^plugin-/, '');
|
|| el.id.replace(/^plugin-/, '');
|
||||||
if (!alreadyHydrated.has(pid)) el.remove();
|
if (!pid || (respondedIds.has(pid) && !alreadyHydrated.has(pid))) el.remove();
|
||||||
});
|
});
|
||||||
|
|
||||||
// Plugin settings area hosts both "Plugin Updates" and per-plugin
|
// Plugin settings area hosts both "Plugin Updates" and per-plugin
|
||||||
|
|||||||
@@ -0,0 +1,115 @@
|
|||||||
|
// Verify loadPlugins' plugin-DOM wipe loops in static/app.js: a plugin that is
|
||||||
|
// merely ABSENT from the current /api/plugins response (transient partial
|
||||||
|
// response while the backend's plugin registry is repopulating after a
|
||||||
|
// restart) must keep its settings panel and screen DOM. Wiping it while its
|
||||||
|
// _loadedPluginScripts entry survives made the next refetch fail the
|
||||||
|
// DOM-existence check and re-evaluate the plugin's screen.js mid-session —
|
||||||
|
// which duplicated the desktop audio_engine's native signal chain. Plugins
|
||||||
|
// the response knows about but that failed hydration are still wiped, as is
|
||||||
|
// junk DOM carrying no plugin id.
|
||||||
|
|
||||||
|
const { test } = require('node:test');
|
||||||
|
const assert = require('node:assert/strict');
|
||||||
|
const fs = require('node:fs');
|
||||||
|
const path = require('node:path');
|
||||||
|
const vm = require('node:vm');
|
||||||
|
|
||||||
|
const APP_JS = path.join(__dirname, '..', '..', 'static', 'app.js');
|
||||||
|
|
||||||
|
// Slice the wipe block out of loadPlugins by its stable landmarks: from the
|
||||||
|
// nav reset that opens it to the comment introducing the next section.
|
||||||
|
function extractWipeBlock(src) {
|
||||||
|
const start = src.indexOf("navContainer.innerHTML = '';");
|
||||||
|
assert.ok(start !== -1, 'wipe block start (nav reset) not found');
|
||||||
|
const end = src.indexOf('// Plugin settings area hosts', start);
|
||||||
|
assert.ok(end !== -1, 'wipe block end marker not found');
|
||||||
|
return src.slice(start, end);
|
||||||
|
}
|
||||||
|
|
||||||
|
function makeEl(pluginId, id) {
|
||||||
|
return {
|
||||||
|
dataset: pluginId != null ? { pluginId } : {},
|
||||||
|
id: id || (pluginId != null ? `plugin-${pluginId}` : ''),
|
||||||
|
removed: false,
|
||||||
|
remove() {
|
||||||
|
this.removed = true;
|
||||||
|
const idx = this._parent ? this._parent.indexOf(this) : -1;
|
||||||
|
if (idx >= 0) this._parent.splice(idx, 1);
|
||||||
|
},
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
function runWipe({ respondedIds, alreadyHydrated, settingsChildren, screens }) {
|
||||||
|
const src = fs.readFileSync(APP_JS, 'utf8');
|
||||||
|
const block = extractWipeBlock(src);
|
||||||
|
settingsChildren.forEach((el) => { el._parent = settingsChildren; });
|
||||||
|
const container = { children: settingsChildren };
|
||||||
|
const sandbox = {
|
||||||
|
navContainer: { innerHTML: 'seed' },
|
||||||
|
mobileNavContainer: { innerHTML: 'seed' },
|
||||||
|
_pluginSettingsContainers: () => [container],
|
||||||
|
respondedIds,
|
||||||
|
alreadyHydrated,
|
||||||
|
document: {
|
||||||
|
querySelectorAll: (sel) => {
|
||||||
|
assert.equal(sel, '.screen[id^="plugin-"]');
|
||||||
|
return screens.slice();
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
vm.runInNewContext(block, sandbox, { filename: 'wipe-block.js' });
|
||||||
|
return sandbox;
|
||||||
|
}
|
||||||
|
|
||||||
|
test('plugin absent from the response keeps its settings + screen DOM', () => {
|
||||||
|
const settings = makeEl('audio_engine');
|
||||||
|
const screen = makeEl('audio_engine');
|
||||||
|
runWipe({
|
||||||
|
respondedIds: new Set(), // partial response: plugin missing
|
||||||
|
alreadyHydrated: new Set(), // scan loop never saw it either
|
||||||
|
settingsChildren: [settings],
|
||||||
|
screens: [screen],
|
||||||
|
});
|
||||||
|
assert.equal(settings.removed, false, 'settings panel must survive a partial response');
|
||||||
|
assert.equal(screen.removed, false, 'screen must survive a partial response');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('plugin present in the response but not hydrated is wiped', () => {
|
||||||
|
const settings = makeEl('stale_plugin');
|
||||||
|
const screen = makeEl('stale_plugin');
|
||||||
|
runWipe({
|
||||||
|
respondedIds: new Set(['stale_plugin']),
|
||||||
|
alreadyHydrated: new Set(),
|
||||||
|
settingsChildren: [settings],
|
||||||
|
screens: [screen],
|
||||||
|
});
|
||||||
|
assert.equal(settings.removed, true);
|
||||||
|
assert.equal(screen.removed, true);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('hydrated plugin present in the response is preserved', () => {
|
||||||
|
const settings = makeEl('audio_engine');
|
||||||
|
const screen = makeEl('audio_engine');
|
||||||
|
runWipe({
|
||||||
|
respondedIds: new Set(['audio_engine']),
|
||||||
|
alreadyHydrated: new Set(['audio_engine']),
|
||||||
|
settingsChildren: [settings],
|
||||||
|
screens: [screen],
|
||||||
|
});
|
||||||
|
assert.equal(settings.removed, false);
|
||||||
|
assert.equal(screen.removed, false);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('junk DOM without a plugin id is still removed', () => {
|
||||||
|
const junkSettings = makeEl(null);
|
||||||
|
// Screen whose id strips to '' (no dataset.pluginId, bare "plugin-" id).
|
||||||
|
const junkScreen = makeEl(null, 'plugin-');
|
||||||
|
runWipe({
|
||||||
|
respondedIds: new Set(['whatever']),
|
||||||
|
alreadyHydrated: new Set(),
|
||||||
|
settingsChildren: [junkSettings],
|
||||||
|
screens: [junkScreen],
|
||||||
|
});
|
||||||
|
assert.equal(junkSettings.removed, true);
|
||||||
|
assert.equal(junkScreen.removed, true);
|
||||||
|
});
|
||||||
@@ -204,15 +204,20 @@ test('does not collide tags across two different plugins', () => {
|
|||||||
assert.deepEqual(headLinks.map((l) => l.dataset.pluginId).sort(), ['a', 'b']);
|
assert.deepEqual(headLinks.map((l) => l.dataset.pluginId).sort(), ['a', 'b']);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('reconcile removes the <link> of a plugin that vanished from /api/plugins', () => {
|
test('reconcile keeps the <link> of a plugin absent from a partial response', () => {
|
||||||
const { inject, reconcile, headLinks } = setupSandbox();
|
const { inject, reconcile, headLinks } = setupSandbox();
|
||||||
inject(plug({ id: 'a' }));
|
inject(plug({ id: 'a' }));
|
||||||
inject(plug({ id: 'b' }));
|
inject(plug({ id: 'b' }));
|
||||||
assert.equal(headLinks.length, 2);
|
assert.equal(headLinks.length, 2);
|
||||||
// `a` is no longer returned (uninstalled) — its stylesheet must be dropped.
|
// `a` is missing from this response. That happens transiently during a
|
||||||
|
// backend restart (the plugin registry repopulates while HTTP stays up),
|
||||||
|
// so absence is NOT an uninstall signal — the still-loaded plugin must
|
||||||
|
// keep its stylesheet or it renders visible-but-unstyled until it
|
||||||
|
// reappears. Explicit removal still happens via the not-ready/unstyled
|
||||||
|
// paths (tests below).
|
||||||
reconcile([plug({ id: 'b' })]);
|
reconcile([plug({ id: 'b' })]);
|
||||||
assert.equal(headLinks.length, 1);
|
assert.equal(headLinks.length, 2);
|
||||||
assert.equal(headLinks[0].dataset.pluginId, 'b');
|
assert.deepEqual(headLinks.map((l) => l.dataset.pluginId).sort(), ['a', 'b']);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('reconcile removes the <link> of a plugin that is no longer ready', () => {
|
test('reconcile removes the <link> of a plugin that is no longer ready', () => {
|
||||||
|
|||||||
@@ -38,15 +38,25 @@ def test_plugin_loader_unmounts_previous_ui_contributions_before_reregistering()
|
|||||||
assert "await _commandUiDomain(contribution.domain, 'mount', plugin, contribution)" in source
|
assert "await _commandUiDomain(contribution.domain, 'mount', plugin, contribution)" in source
|
||||||
|
|
||||||
|
|
||||||
def test_plugin_loader_unmounts_contributions_for_removed_plugins():
|
def test_plugin_loader_does_not_treat_response_absence_as_uninstall():
|
||||||
|
# A plugin transiently absent from /api/plugins (the backend clears its
|
||||||
|
# registry at the start of load_plugins() and repopulates incrementally
|
||||||
|
# while HTTP stays up, so restarts serve partial responses) must NOT be
|
||||||
|
# torn down: the old absence sweep unmounted UI contributions and
|
||||||
|
# unregistered the capability participant with no re-registration path
|
||||||
|
# (plugin scripts don't re-run), and the DOM/style wipes forced a
|
||||||
|
# mid-session screen.js re-evaluation that duplicated the desktop
|
||||||
|
# audio_engine's native signal chain.
|
||||||
source = (ROOT / "static" / "app.js").read_text(encoding="utf-8")
|
source = (ROOT / "static" / "app.js").read_text(encoding="utf-8")
|
||||||
|
|
||||||
assert "const livePluginIds = new Set(plugins.map((plugin) => plugin.id))" in source
|
# The absence-triggered sweep is gone (rationale comment in its place)...
|
||||||
assert "for (const [pluginId, contributions] of _pluginUiContributions)" in source
|
assert "const livePluginIds" not in source
|
||||||
assert "const stalePlugin = { id: pluginId }" in source
|
assert "const stalePlugin = { id: pluginId }" not in source
|
||||||
assert "await _commandUiDomain(contribution.domain, 'unmount', stalePlugin, contribution)" in source
|
assert "deliberately NO stale-contribution sweep" in source
|
||||||
assert "window.feedBack?.capabilities?.unregisterParticipant?.(pluginId)" in source
|
# ...and the DOM/style reconcilers only act on plugins the response names.
|
||||||
assert "_pluginUiContributions.delete(pluginId)" in source
|
assert "const respondedIds = new Set(plugins.map((p) => p.id))" in source
|
||||||
|
assert "respondedIds.has(pid) && !alreadyHydrated.has(pid)" in source
|
||||||
|
assert "responded.has(id) && !styled.has(id)" in source
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user