skills/pbakaus_impeccable
1
0
Fork 0
mirror of https://github.com/pbakaus/impeccable.git synced 2026-09-11 21:57:14 +03:00
Code Issues Packages Projects Releases Wiki Activity

Live: a discard releases every wrapper it hid

Browse Source 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY
This commit is contained in:
Paul Bakaus
2026-09-04 14:31:25 +05:00
committed by Abdul Wahab
co-authored by Claude Code
parent f240348cc5
commit 524fb8c950
2 changed files with 95 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
skill/scripts/live-browser.js
+36 -10
View File
@@ -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);
tests/live-browser-source.test.mjs
+59 -8
View File
@@ -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',
);