From f240348cc55755f059c14e2133a38ac7689fed8a Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Fri, 4 Sep 2026 00:31:35 -0700 Subject: [PATCH] Live: every active-session wrapper lookup goes through the resolver Cursor Bugbot on #720: findVariantsWrapper alone was not enough. resolveBarAnchor, the visible-variant element, mountedParameterCount, readVisibleVariantFromDOM, showVariantInDOM, the source injection, and the whole accept path still took the first [data-impeccable-variants] match, so in the relocated-wrapper case Tune never bound and the bar kept anchoring to the empty scaffold even after the resume reached CYCLING. Thirteen call sites now resolve through findVariantsWrapper. The resolver split in two so a missing id cannot silently widen the lookup to any session: findVariantsWrapper(sessionId) returns null without an id, and findAnyVariantsWrapper() is the entry point for the two resume paths that have no id yet. Both share pickPopulatedVariantsWrapper, which is the old querySelector whenever there are fewer than two matches. Discard cleanup now hides every duplicate wrapper rather than the first, since a target inside a `.map()` renders one per item and hiding one left the rest of the discarded variants on screen. What still takes a raw first match is deliberate: bare existence checks, selector strings for stylesheets and observers (which want to cover every match), querySelectorAll sweeps, the parsed source document, and the Svelte component wrapper, which holds no variant children at all. The source-shape test pins that exact set by name, so a new raw lookup fails until it is either routed through the resolver or justified there. Co-Authored-By: Claude Code Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY --- skill/scripts/live-browser.js | 66 +++++++++++++++++---------- tests/live-browser-source.test.mjs | 73 +++++++++++++++++++++++++++--- 2 files changed, 109 insertions(+), 30 deletions(-) diff --git a/skill/scripts/live-browser.js b/skill/scripts/live-browser.js index cbbf3a7ce..dc4e751e2 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -2060,7 +2060,7 @@ if (anchor) return anchor; } if (currentSessionId && (state === 'GENERATING' || state === 'CYCLING')) { - const wrapper = document.querySelector('[data-impeccable-variants="' + currentSessionId + '"]'); + const wrapper = findVariantsWrapper(currentSessionId); if (wrapper) { const variantCount = wrapper.querySelectorAll('[data-impeccable-variant]:not([data-impeccable-variant="original"])').length; if (variantCount > 0 && visibleVariant > 0) { @@ -2131,14 +2131,14 @@ function isInsertGeneratingSession() { if (state !== 'GENERATING' || !currentSessionId) return false; - const wrapper = document.querySelector('[data-impeccable-variants="' + currentSessionId + '"]'); + const wrapper = findVariantsWrapper(currentSessionId); return !!wrapper && wrapper.dataset.impeccableMode === 'insert'; } /** Recreate the dotted placeholder if Astro/Vite HMR removed it mid-generation. */ function ensureInsertPlaceholder() { if (!isInsertGeneratingSession()) return placeholderElement; - const wrapper = document.querySelector('[data-impeccable-variants="' + currentSessionId + '"]'); + const wrapper = findVariantsWrapper(currentSessionId); const variantCount = wrapper.querySelectorAll('[data-impeccable-variant]:not([data-impeccable-variant="original"])').length; if (variantCount > 0) return placeholderElement; if (placeholderElement && document.body.contains(placeholderElement)) return placeholderElement; @@ -3156,7 +3156,7 @@ || svelteComponentSession.wrapperEl || null; } - const wrapper = document.querySelector('[data-impeccable-variants="' + currentSessionId + '"]'); + const wrapper = findVariantsWrapper(currentSessionId); if (!wrapper) return null; return wrapper.querySelector('[data-impeccable-variant="' + visibleVariant + '"]'); } @@ -4900,7 +4900,7 @@ return Object.values(svelteComponentSession.paramsByVariant || {}) .reduce((total, params) => total + (Array.isArray(params) ? params.length : 0), 0); } - const wrapper = document.querySelector('[data-impeccable-variants="' + currentSessionId + '"]'); + const wrapper = findVariantsWrapper(currentSessionId); if (!wrapper) return 0; return [...wrapper.querySelectorAll('[data-impeccable-variant]:not([data-impeccable-variant="original"])')] .reduce((total, variant) => total + parseVariantParams(variant).length, 0); @@ -5004,7 +5004,7 @@ scheduleCyclingBarSync(sessionId, num); return true; } - const wrapper = document.querySelector('[data-impeccable-variants="' + sessionId + '"]'); + const wrapper = findVariantsWrapper(sessionId); if (!wrapper) return false; updateVariantStateStylesheet(sessionId, num); // Unconditional refresh - covers first-reveal (no-op if state isn't @@ -6362,7 +6362,7 @@ } rememberSessionFileMeta({ file: filePath }); if (isJsxSourceFile(filePath)) { - const liveWrapper = document.querySelector('[data-impeccable-variants="' + sessionId + '"]'); + const liveWrapper = findVariantsWrapper(sessionId); if (liveWrapper && liveWrapper.querySelector('[data-impeccable-variant]:not([data-impeccable-variant="original"])')) { completeSourceInjection(liveWrapper, sessionId, { ...opts, filePath }); return; @@ -6434,7 +6434,7 @@ return; } - const existingWrapper = document.querySelector('[data-impeccable-variants="' + sessionId + '"]'); + const existingWrapper = findVariantsWrapper(sessionId); if (existingWrapper) { const wrapper = srcWrapper.cloneNode(true); existingWrapper.parentElement.replaceChild(wrapper, existingWrapper); @@ -6591,7 +6591,7 @@ if (anchor && !anchor.__impeccableFrozenAnchor) selectedElement = anchor; return; } - const wrapper = document.querySelector('[data-impeccable-variants="' + currentSessionId + '"]'); + const wrapper = findVariantsWrapper(currentSessionId); if (!wrapper) return; const visEl = pickVariantContent(wrapper, visibleVariant); if (visEl) selectedElement = visEl; @@ -6601,7 +6601,7 @@ if (svelteComponentSession?.sessionId === sessionId && svelteComponentSession.mountedVariant > 0) { return svelteComponentSession.mountedVariant; } - const wrapper = document.querySelector('[data-impeccable-variants="' + sessionId + '"]'); + const wrapper = findVariantsWrapper(sessionId); if (!wrapper) return 0; const variants = wrapper.querySelectorAll('[data-impeccable-variant]:not([data-impeccable-variant="original"])'); for (const variant of variants) { @@ -6913,12 +6913,16 @@ // agent may have relocated the wrapper out of the shared primitive live-wrap // scaffolded into. A plain first match can then pin an empty scaffold while // the real variants sit in a later wrapper, which strands the session at - // 0/N. Prefer a wrapper that actually holds variants. With zero or one match - // this is exactly the querySelector it replaces. - function findVariantsWrapper(sessionId) { - const selector = sessionId - ? '[data-impeccable-variants="' + sessionId + '"]' - : '[data-impeccable-variants]'; + // 0/N and leaves the bar, the params panel, and accept all reading the + // wrong element. Prefer a wrapper that actually holds variants. With zero + // or one match this is exactly the querySelector it replaces. + // + // Every lookup of the ACTIVE session's wrapper goes through here. The + // remaining raw `[data-impeccable-variants=...]` uses are deliberate: bare + // existence checks, selector strings for stylesheets and observers (which + // want to cover every match), `querySelectorAll` sweeps, and the parsed + // source document, which is not this document. + function pickPopulatedVariantsWrapper(selector) { const matches = document.querySelectorAll(selector); if (matches.length < 2) return matches[0] || null; for (const candidate of matches) { @@ -6929,6 +6933,17 @@ return matches[0]; } + /** The wrapper holding `sessionId`'s variants, or null without an id. */ + function findVariantsWrapper(sessionId) { + if (!sessionId) return null; + return pickPopulatedVariantsWrapper('[data-impeccable-variants="' + sessionId + '"]'); + } + + /** Any live variant wrapper, for the resume paths that have no id yet. */ + function findAnyVariantsWrapper() { + return pickPopulatedVariantsWrapper('[data-impeccable-variants]'); + } + function startVariantObserver(sessionId) { let updating = false; // re-entrancy guard @@ -8675,7 +8690,7 @@ void main() { clientSentAt: Date.now(), }; if (!currentSessionId || arrivedVariants === 0) return; - const acceptWrapper = document.querySelector('[data-impeccable-variants="' + currentSessionId + '"]'); + const acceptWrapper = findVariantsWrapper(currentSessionId); if (Object.keys(paramsCurrentValues).length > 0) { acceptPayload.paramValues = { ...paramsCurrentValues }; } @@ -8770,7 +8785,7 @@ void main() { } function snapshotAcceptedVariantDom(sessionId, variantId) { - const wrapper = document.querySelector('[data-impeccable-variants="' + sessionId + '"]'); + const wrapper = findVariantsWrapper(sessionId); const accepted = wrapper?.querySelector?.('[data-impeccable-variant="' + variantId + '"]'); const root = accepted?.firstElementChild || null; return { @@ -8897,7 +8912,7 @@ void main() { } function commitAcceptedVariantToDom(sessionId, variantId) { - const wrapper = document.querySelector('[data-impeccable-variants="' + sessionId + '"]'); + const wrapper = findVariantsWrapper(sessionId); if (!wrapper) return false; const accepted = wrapper.querySelector('[data-impeccable-variant="' + variantId + '"]'); if (!accepted || !accepted.firstElementChild) return false; @@ -9125,7 +9140,7 @@ void main() { } function restoreFromActiveSessions(activeSessions, reason) { - const wrapper = findVariantsWrapper(null); + const wrapper = findAnyVariantsWrapper(); if (wrapper && !isFrameworkComponentPreviewMode(wrapper.dataset.impeccablePreview)) return false; if (svelteComponentSession?.sessionId === currentSessionId) return false; return restoreSessionWithoutWrapper(reason || 'sse_connected', activeSessions); @@ -9238,10 +9253,13 @@ void main() { // reconciler later tries to remove a wrapper we already removed. // Schedule a 2s fallback that does the manual swap only if HMR hasn't // replaced the wrapper by then (keeps static-server / no-HMR flows alive). - const wrapper = document.querySelector('[data-impeccable-variants="' + cleanupSessionId + '"]'); - if (wrapper) { + // 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 + '"]')]; + if (discardWrappers.length > 0) { if (restoreOriginal) showOriginalDuringDiscard(cleanupSessionId); - else wrapper.style.display = 'none'; + else for (const discardWrapper of discardWrappers) discardWrapper.style.display = 'none'; } setTimeout(function() { const recoverySuperseded = deferredRecoverySuperseded(cleanupSessionId, cleanupRevision); @@ -9472,7 +9490,7 @@ void main() { // used to log the same `browser_resumed`, which made issue #719 take a // DOM reconstruction to diagnose. const resumeReason = opts.reason || 'browser_resumed'; - const wrapper = findVariantsWrapper(null); + const wrapper = findAnyVariantsWrapper(); const runtimeWrapper = wrapper || document.querySelector('[data-impeccable-carbonize]'); if (restoreSessionSupersedingHandledWrapper(runtimeWrapper)) return true; if (scheduleHandledRuntimeWrapperReload(runtimeWrapper, recoveryRevision)) return false; diff --git a/tests/live-browser-source.test.mjs b/tests/live-browser-source.test.mjs index c87db08f6..14feecafd 100644 --- a/tests/live-browser-source.test.mjs +++ b/tests/live-browser-source.test.mjs @@ -630,8 +630,8 @@ describe('live-browser source contracts', () => { // that relocates the wrapper out of the shared primitive live-wrap // scaffolded leaves an empty one behind. First match can then pin a // scaffold with no variants and strand the session at 0/N. - const start = SOURCE.indexOf('function findVariantsWrapper(sessionId)'); - assert.ok(start > 0, 'findVariantsWrapper must exist'); + const start = SOURCE.indexOf('function pickPopulatedVariantsWrapper(selector)'); + assert.ok(start > 0, 'pickPopulatedVariantsWrapper must exist'); const helper = SOURCE.slice(start, SOURCE.indexOf('\n function startVariantObserver(', start)); assert.match(helper, /if \(matches\.length < 2\) return matches\[0\] \|\| null;/); assert.match( @@ -640,12 +640,73 @@ describe('live-browser source contracts', () => { 'the preferred wrapper is the one holding non-original variants', ); assert.match(helper, /return matches\[0\];/, 'with no populated wrapper the old first match still wins'); - for (const caller of [ - 'const wrapper = findVariantsWrapper(sessionId);', - 'const wrapper = findVariantsWrapper(null);', + assert.match( + helper, + /function findVariantsWrapper\(sessionId\) \{\n if \(!sessionId\) return null;/, + 'a missing id must not silently widen the lookup to any session', + ); + assert.match( + helper, + /function findAnyVariantsWrapper\(\) \{[\s\S]{0,120}?'\[data-impeccable-variants\]'/, + 'the resume paths that have no id yet need their own entry point', + ); + }); + + it('routes every active-session wrapper lookup through the resolver (#719)', () => { + // Bugbot on #720: findVariantsWrapper alone is not enough while the bar + // anchor, the visible-variant element, the params count, and accept still + // take the first match, because in the relocated-wrapper case Tune never + // binds and the bar keeps anchoring to the empty scaffold. + for (const fn of [ + 'function resolveBarAnchor()', + 'function isInsertGeneratingSession()', + 'function ensureInsertPlaceholder()', + 'function mountedParameterCount()', + 'function readVisibleVariantFromDOM(sessionId)', + 'function snapshotAcceptedVariantDom(sessionId, variantId)', + 'function commitAcceptedVariantToDom(sessionId, variantId)', ]) { - assert.ok(SOURCE.includes(caller), `${caller} should be how the resolvers look a wrapper up`); + const at = SOURCE.indexOf(fn); + assert.ok(at > 0, `${fn} should exist`); + const body = SOURCE.slice(at, SOURCE.indexOf('\n }', at)); + assert.doesNotMatch( + body, + /document\.querySelector\('\[data-impeccable-variants="'/, + `${fn} must resolve the session wrapper through findVariantsWrapper`, + ); } + + // Anything still taking a raw first match is a deliberate existence check + // or a cleanup sweep. Pinning the exact set means a new raw lookup has to + // justify itself here rather than quietly reintroducing the bug. + const rawSites = [...SOURCE.matchAll( + /(?:const (\w+) = (?:!!)?|(if) \()[^\n]{0,40}?document\.querySelector\('\[data-impeccable-variants="' \+ [\w.?]+ \+ '"\]'\)/g, + )].map((m) => m[1] || m[2]); + assert.deepEqual( + [...rawSites].sort(), + [ + // existence only, inside the variant-anchor retry observer + 'wrapperLanded', + // svelte component republish: an identity check next to + // svelteComponentSession, and a component wrapper holds no variants + 'existingWrapper', + // orphan removal in abortSvelteComponentInjection + 'orphan', + // orphan removal in resetSvelteComponentSession + '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', + ); + assert.equal( + rawSites.length, + SOURCE.split(`document.querySelector('[data-impeccable-variants="'`).length - 1, + 'every raw session-wrapper lookup must be shaped so this guard can see it', + ); }); it('invalidates nullable deferred recovery as soon as a replacement edit starts configuring', () => {