From 524fb8c9500e16a00fa2d55b5ee2d8ca787e2b36 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Fri, 4 Sep 2026 00:50:54 -0700 Subject: [PATCH] Live: a discard releases every wrapper it hid Bugbot on #720: the non-restoreOriginal discard now hides every matching wrapper, but the delayed fallback still released only the first querySelector hit. A target inside a `.map()` renders one wrapper per item, so the rest stayed at display:none and their original content never came back on the static and missed-HMR flows that fallback exists for. The hide, the existence checks, and the release now all speak about the same set. discardedWrappers(sessionId) is the one place that collects it; releaseDiscardedStaticWrappers takes the stylesheet down once and releases each wrapper; releaseDiscardedStaticWrapper drops its sessionId argument and just unwinds the node it is given. The HMR-ownership decision still reads the first wrapper, which is fair: duplicates all render from one source element, so ownership is uniform across them. The reload branch is unchanged because a reload restores every original at once. Covered by a source-shape test rather than an e2e scenario: hasFrameworkHmrOwnership is true for every React, Vue, and Svelte runtime fixture, so all of them take the watcher path and none can reach the static release. The existing framework-ownership guards in the same file move to the new shape and keep their intent, including the one that says only non-discard cleanup may blank the wrapper while waiting for HMR. Co-Authored-By: Claude Code Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY --- skill/scripts/live-browser.js | 46 +++++++++++++++----- tests/live-browser-source.test.mjs | 67 ++++++++++++++++++++++++++---- 2 files changed, 95 insertions(+), 18 deletions(-) diff --git a/skill/scripts/live-browser.js b/skill/scripts/live-browser.js index dc4e751e2..277c60e2c 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -6716,8 +6716,17 @@ document.getElementById(discardStateStyleId(sessionId))?.remove(); } - function releaseDiscardedStaticWrapper(wrapper, sessionId) { - removeDiscardStateStylesheet(sessionId); + /** + * Every wrapper a discard has to unwind. A target inside a `.map()` renders + * one wrapper per item, so the hide, the release, and the existence checks + * all have to speak about the same set. + */ + function discardedWrappers(sessionId) { + if (!sessionId) return []; + return [...document.querySelectorAll('[data-impeccable-variants="' + sessionId + '"]')]; + } + + function releaseDiscardedStaticWrapper(wrapper) { if (!wrapper) return; const orig = wrapper.querySelector('[data-impeccable-variant="original"]'); const content = orig?.firstElementChild; @@ -6728,6 +6737,18 @@ wrapper.remove(); } + /** + * Undo the discard hide on every wrapper it covered. Releasing only the + * first match left the other mapped items sitting at display:none with + * their original content never restored, on exactly the static and + * missed-HMR flows this fallback exists for. + */ + function releaseDiscardedStaticWrappers(sessionId, wrappers) { + removeDiscardStateStylesheet(sessionId); + const set = wrappers && wrappers.length ? wrappers : discardedWrappers(sessionId); + for (const wrapper of set) releaseDiscardedStaticWrapper(wrapper); + } + function watchForDiscardedFrameworkWrapperRemoval(sessionId) { if (!sessionId || !document.body) return; if (discardedFrameworkWrapperWatchers.has(sessionId)) return; @@ -9256,7 +9277,7 @@ void main() { // Every match, not the first: a target inside a `.map()` renders one // wrapper per item, and hiding only one leaves the rest of the // discarded variants on screen. - const discardWrappers = [...document.querySelectorAll('[data-impeccable-variants="' + cleanupSessionId + '"]')]; + const discardWrappers = discardedWrappers(cleanupSessionId); if (discardWrappers.length > 0) { if (restoreOriginal) showOriginalDuringDiscard(cleanupSessionId); else for (const discardWrapper of discardWrappers) discardWrapper.style.display = 'none'; @@ -9267,16 +9288,19 @@ void main() { removeDiscardStateStylesheet(); return; } - const lateWrapper = document.querySelector('[data-impeccable-variants="' + cleanupSessionId + '"]'); - if (!lateWrapper) { + const lateWrappers = discardedWrappers(cleanupSessionId); + if (lateWrappers.length === 0) { removeDiscardStateStylesheet(cleanupSessionId); return; } + // Duplicates all render from one source element, so HMR ownership is + // uniform across them; the first is a fair witness for the set. + const lateWrapper = lateWrappers[0]; if (recoverySuperseded) { if (hasFrameworkHmrOwnership(lateWrapper)) { watchForDiscardedFrameworkWrapperRemoval(cleanupSessionId); } else { - releaseDiscardedStaticWrapper(lateWrapper, cleanupSessionId); + releaseDiscardedStaticWrappers(cleanupSessionId, lateWrappers); } return; } @@ -9285,18 +9309,20 @@ void main() { // the final source rewrite, reload once after a grace window so the // discarded source becomes authoritative without a reconciler race. setTimeout(function() { - const staleWrapper = document.querySelector('[data-impeccable-variants="' + cleanupSessionId + '"]'); + const staleWrappers = discardedWrappers(cleanupSessionId); if (deferredRecoverySuperseded(cleanupSessionId, cleanupRevision)) { - if (!staleWrapper) removeDiscardStateStylesheet(cleanupSessionId); + if (staleWrappers.length === 0) removeDiscardStateStylesheet(cleanupSessionId); else watchForDiscardedFrameworkWrapperRemoval(cleanupSessionId); return; } removeDiscardStateStylesheet(cleanupSessionId); - if (staleWrapper) location.reload(); + // A reload restores every wrapper's original at once, so there is + // nothing per-wrapper to do here. + if (staleWrappers.length > 0) location.reload(); }, 2000); return; } - releaseDiscardedStaticWrapper(lateWrapper, cleanupSessionId); + releaseDiscardedStaticWrappers(cleanupSessionId, lateWrappers); }, 2000); } hideBar(instantChrome); diff --git a/tests/live-browser-source.test.mjs b/tests/live-browser-source.test.mjs index 14feecafd..1148a5542 100644 --- a/tests/live-browser-source.test.mjs +++ b/tests/live-browser-source.test.mjs @@ -406,22 +406,22 @@ describe('live-browser source contracts', () => { ); assert.match( SOURCE, - /if \(hasFrameworkHmrOwnership\(lateWrapper\)\) \{[\s\S]{0,900}?location\.reload\(\);[\s\S]{0,100}?return;[\s\S]{0,150}?releaseDiscardedStaticWrapper\(lateWrapper, cleanupSessionId\)/, + /if \(hasFrameworkHmrOwnership\(lateWrapper\)\) \{[\s\S]{0,1100}?location\.reload\(\);[\s\S]{0,100}?return;[\s\S]{0,150}?releaseDiscardedStaticWrappers\(cleanupSessionId, lateWrappers\)/, 'discard cleanup must use a reload grace fallback before replacing a framework-owned wrapper', ); assert.match( SOURCE, - /function releaseDiscardedStaticWrapper\(wrapper, sessionId\)[\s\S]{0,400}?replaceChild\(content, wrapper\)/, + /function releaseDiscardedStaticWrapper\(wrapper\)[\s\S]{0,400}?replaceChild\(content, wrapper\)/, 'only the static-wrapper release helper may structurally restore discarded DOM', ); assert.match( SOURCE, - /if \(hasFrameworkHmrOwnership\(lateWrapper\)\) \{[\s\S]{0,700}?removeDiscardStateStylesheet\(cleanupSessionId\);[\s\S]{0,120}?location\.reload\(\);/, + /if \(hasFrameworkHmrOwnership\(lateWrapper\)\) \{[\s\S]{0,900}?removeDiscardStateStylesheet\(cleanupSessionId\);[\s\S]{0,250}?location\.reload\(\);/, 'discard must keep its original-visibility stylesheet until the HMR grace window ends', ); assert.match( SOURCE, - /const recoverySuperseded = deferredRecoverySuperseded\(cleanupSessionId, cleanupRevision\);[\s\S]{0,500}?if \(recoverySuperseded\) \{[\s\S]{0,250}?watchForDiscardedFrameworkWrapperRemoval\(cleanupSessionId\)[\s\S]{0,150}?releaseDiscardedStaticWrapper\(lateWrapper, cleanupSessionId\)[\s\S]{0,80}?return;/, + /const recoverySuperseded = deferredRecoverySuperseded\(cleanupSessionId, cleanupRevision\);[\s\S]{0,700}?if \(recoverySuperseded\) \{[\s\S]{0,250}?watchForDiscardedFrameworkWrapperRemoval\(cleanupSessionId\)[\s\S]{0,150}?releaseDiscardedStaticWrappers\(cleanupSessionId, lateWrappers\)[\s\S]{0,80}?return;/, 'discard cleanup and its reload grace callback must yield to a newer Live session', ); assert.match( @@ -436,7 +436,7 @@ describe('live-browser source contracts', () => { ); assert.match( SOURCE, - /setTimeout\(function\(\) \{[\s\S]{0,300}?const staleWrapper = document\.querySelector[\s\S]{0,250}?deferredRecoverySuperseded\(cleanupSessionId, cleanupRevision\)[\s\S]{0,250}?watchForDiscardedFrameworkWrapperRemoval\(cleanupSessionId\)[\s\S]{0,100}?return;[\s\S]{0,100}?removeDiscardStateStylesheet\(cleanupSessionId\);[\s\S]{0,100}?location\.reload\(\);/, + /setTimeout\(function\(\) \{[\s\S]{0,300}?const staleWrappers = discardedWrappers\(cleanupSessionId\);[\s\S]{0,250}?deferredRecoverySuperseded\(cleanupSessionId, cleanupRevision\)[\s\S]{0,250}?watchForDiscardedFrameworkWrapperRemoval\(cleanupSessionId\)[\s\S]{0,100}?return;[\s\S]{0,100}?removeDiscardStateStylesheet\(cleanupSessionId\);[\s\S]{0,250}?location\.reload\(\);/, 'framework discard recovery may observe safe HMR cleanup but must not reload replacement work', ); assert.match( @@ -564,6 +564,60 @@ describe('live-browser source contracts', () => { ); }); + it('unwinds every wrapper a discard hid, not just the first (#719)', () => { + // Bugbot on #720: the non-restoreOriginal discard hides every matching + // wrapper, so the delayed fallback has to release the same set. Releasing + // the first match left the other mapped items at display:none with their + // original content never restored, on exactly the static and missed-HMR + // flows the fallback exists for. The e2e fixtures cannot cover this: + // hasFrameworkHmrOwnership is true for every React, Vue, and Svelte + // fixture, so they all take the watcher path instead. + const cleanupAt = SOURCE.indexOf('function cleanup(options)'); + assert.ok(cleanupAt > 0, 'cleanup must exist'); + const cleanup = SOURCE.slice(cleanupAt, SOURCE.indexOf('\n //', cleanupAt)); + + assert.match( + cleanup, + /const discardWrappers = discardedWrappers\(cleanupSessionId\);[\s\S]{0,260}?for \(const discardWrapper of discardWrappers\) discardWrapper\.style\.display = 'none';/, + 'the hide must cover every wrapper for the session', + ); + assert.match( + cleanup, + /const lateWrappers = discardedWrappers\(cleanupSessionId\);[\s\S]{0,120}?if \(lateWrappers\.length === 0\)/, + 'the fallback must look at the same set the hide covered', + ); + assert.doesNotMatch( + cleanup, + /releaseDiscardedStaticWrapper\(/, + 'cleanup must go through the plural release so every hidden wrapper is unwound', + ); + for (const call of [...cleanup.matchAll(/releaseDiscardedStaticWrappers\([^)]*\)/g)].map((m) => m[0])) { + assert.match(call, /lateWrappers/, `${call} must release the captured set`); + } + assert.ok( + [...cleanup.matchAll(/releaseDiscardedStaticWrappers\(/g)].length === 2, + 'both the superseded and the plain static branch must release', + ); + + const pluralAt = SOURCE.indexOf('function releaseDiscardedStaticWrappers(sessionId, wrappers)'); + assert.ok(pluralAt > 0, 'releaseDiscardedStaticWrappers must exist'); + const plural = SOURCE.slice(pluralAt, SOURCE.indexOf('\n }', pluralAt)); + assert.match(plural, /removeDiscardStateStylesheet\(sessionId\);/, 'the stylesheet comes down once'); + assert.match( + plural, + /for \(const wrapper of set\) releaseDiscardedStaticWrapper\(wrapper\);/, + 'every wrapper in the set is released', + ); + + // Intent of main's original guard, kept: discard must not blank the + // original, and must not animate stale chrome while waiting for HMR. + assert.match( + cleanup, + /if \(restoreOriginal\) showOriginalDuringDiscard\(cleanupSessionId\);/, + 'only non-discard cleanup may blank the wrapper while waiting for HMR', + ); + }); + it('never leaves a shader behind when the teardown races its construction (#719)', () => { // showShaderOverlay appends its canvas, then awaits createImageBitmap and // the GL setup before it publishes shaderState. A teardown inside that @@ -696,9 +750,6 @@ describe('live-browser source contracts', () => { 'orphan', // pendingAcceptedSession existence guard 'if', - // discard cleanup, both bounded retries - 'lateWrapper', - 'staleWrapper', ].sort(), 'a new raw [data-impeccable-variants=...] first-match lookup appeared; route it through findVariantsWrapper, or add it here with the reason it may take the first match', );