mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-12 06:06:37 +03:00
fix(live): four HMR + React race bugs from Next 16 / Turbopack testing
Surfaced during hands-on testing against a real Next 16 + Turbopack app (EACManagement). All four compound to produce unusable live iteration for React users; fixed bottom-up because each one blocked testing the next. ## 1. Picker bar snaps to (0,0) on first variant arrival In startVariantObserver, `showVariantInDOM(sessionId, 1)` hides the original via display:none but we never re-pointed selectedElement. Next frame, getBoundingClientRect() on the hidden original returns a zero rect and the bar positions at (0,0). Clicking Next masked the bug because cycleVariant already calls updateSelectedElement. Fix: after showVariantInDOM, re-point selectedElement via pickVariantContent(wrapper, visibleVariant) — same call the no-HMR fallback and updateSelectedElement already use. ## 2. React NotFoundError on accept/discard (Next 16 / Turbopack) handleAccept and cleanup both called `wrapper.parentElement.replaceChild(...)` eagerly, before the agent's source rewrite had propagated through HMR. That yanks children out from under React's reconciler; when React later tries to remove/replace the wrapper, its fiber tree no longer matches the DOM and it throws. Fix, both paths: - cleanup (discard): `wrapper.style.display = 'none'` so variants disappear immediately, no structural DOM mutation. - handleAccept: skip the eager replaceChild entirely. The accepted variant is already the only visible child of the wrapper thanks to the display: contents pattern; HMR cleans up the wrapper itself. - Both paths schedule a 2s fallback replaceChild that runs only if HMR hasn't cleaned up — keeps static-server / no-HMR flows working. - Capture sessionId + visibleVariant in closure variables before the 1800ms cleanup timer zeros them, so the fallback still has context. ## 3. Server serves stale live.js forever loadBrowserScripts() read live-browser.js once at startup into a liveScript string. The /live.js handler served that cached string with no cache headers. Every edit to the browser script was invisible until a full server restart — silently broke the iteration loop on fixes #1 and #2 for the user. Fix: - loadBrowserScripts returns { detectScript, livePath } — existence check only, no caching. - /live.js handler re-reads livePath on every request and prepends __IMPECCABLE_TOKEN__ / __IMPECCABLE_PORT__ each time. - Response headers: Cache-Control: no-store, no-cache, must-revalidate, max-age=0 + Pragma: no-cache. detect.js stays cached — it rarely changes during a session. ## 4. Picker stuck in GENERATING when HMR doesn't fire The only 'done' fallback fired when arrivedVariants === 0 and called injectVariantsFromSource, which parses raw source via DOMParser. That can't work for TSX/JSX/Vue/Svelte — JSX expressions aren't valid HTML. If HMR flaked or was slow (500+ line inserts on Next 16 are prone to this), state stayed in GENERATING and the spinner ran forever. Fix: give HMR a 2s grace window, then `window.location.reload()`. resumeSession already counts variants off the rendered DOM on load and transitions straight to CYCLING — reload is the universal recovery path that works for any framework, HTML, static server, anything. injectVariantsFromSource is now dead code on the 'done' path. Kept for potential pure-HTML-no-HMR future use. ## Credit Precise repro + root-cause diagnosis from the other agent in the EACManagement session. #2 and #4 are the high-impact ones for Next 16 / Turbopack; #3 is the meta-fix that made iterating on #1 and #2 possible at all. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Paul Bakaus
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
89466e5d98
commit
c7ee722472
@@ -1357,6 +1357,11 @@
|
||||
if (visibleVariant === 0 && arrivedVariants > 0) {
|
||||
visibleVariant = 1;
|
||||
showVariantInDOM(sessionId, 1);
|
||||
// showVariantInDOM hid the original (display:none); if we were still
|
||||
// anchored to the original's content, its boundingRect is now zero
|
||||
// and the bar snaps to (0,0). Re-point at the visible variant instead.
|
||||
const visEl = pickVariantContent(wrapper, visibleVariant);
|
||||
if (visEl) selectedElement = visEl;
|
||||
}
|
||||
|
||||
const expected = parseInt(wrapper.dataset.impeccableVariantCount || '0');
|
||||
@@ -1427,16 +1432,26 @@
|
||||
if (state === 'IDLE') state = 'PICKING';
|
||||
break;
|
||||
case 'done':
|
||||
// Generate completion: handle no-HMR fallback
|
||||
if (arrivedVariants === 0 && expectedVariants > 0 && msg.file) {
|
||||
console.log('[impeccable] No HMR detected. Fetching variants from source file...');
|
||||
injectVariantsFromSource(msg.file, currentSessionId);
|
||||
return;
|
||||
}
|
||||
if (state === 'GENERATING') {
|
||||
state = 'CYCLING';
|
||||
updateBarContent('cycling');
|
||||
// Variants already arrived via HMR → normal transition.
|
||||
if (arrivedVariants >= expectedVariants && expectedVariants > 0) {
|
||||
if (state === 'GENERATING') {
|
||||
state = 'CYCLING';
|
||||
updateBarContent('cycling');
|
||||
}
|
||||
break;
|
||||
}
|
||||
// HMR didn't propagate in time. Give it a 2s grace window, then
|
||||
// reload the page. resumeSession counts variants off the rendered
|
||||
// DOM on load and transitions straight to CYCLING — reload is the
|
||||
// one universal recovery path: HTML, JSX/TSX, Vue, Svelte, static
|
||||
// servers, anything. We used to try DOMParser on the raw source,
|
||||
// but JSX expressions aren't valid HTML and the parse fails.
|
||||
setTimeout(() => {
|
||||
if (arrivedVariants >= expectedVariants && expectedVariants > 0) return;
|
||||
if (state !== 'GENERATING') return;
|
||||
saveSession();
|
||||
window.location.reload();
|
||||
}, 2000);
|
||||
break;
|
||||
case 'error':
|
||||
console.error('[impeccable] Error:', msg.message);
|
||||
@@ -2030,15 +2045,14 @@ void main() {
|
||||
sendEvent({ type: 'accept', id: currentSessionId, variantId: String(visibleVariant) });
|
||||
markSessionHandled();
|
||||
|
||||
// Instantly commit the accepted variant in the DOM (fire-and-forget)
|
||||
var wrapper = document.querySelector('[data-impeccable-variants="' + currentSessionId + '"]');
|
||||
if (wrapper) {
|
||||
var accepted = wrapper.querySelector('[data-impeccable-variant="' + visibleVariant + '"]');
|
||||
if (accepted && accepted.firstElementChild) {
|
||||
var parent = wrapper.parentElement;
|
||||
if (parent) parent.replaceChild(accepted.firstElementChild.cloneNode(true), wrapper);
|
||||
}
|
||||
}
|
||||
// The accepted variant is already the only visible child of the wrapper
|
||||
// (all other variants are display:none). HMR from the source rewrite will
|
||||
// replace the wrapper imminently. Don't eagerly replaceChild here — React
|
||||
// reconciliation races with our mutation and throws NotFoundError in Next
|
||||
// 16 / Turbopack. Schedule a fallback that runs the manual swap only if
|
||||
// HMR hasn't cleaned up by then (keeps static-server flows working).
|
||||
const acceptedSessionId = currentSessionId;
|
||||
const acceptedVariant = visibleVariant;
|
||||
|
||||
state = 'CONFIRMED';
|
||||
updateBarContent('confirmed');
|
||||
@@ -2053,6 +2067,19 @@ void main() {
|
||||
selectedAction = 'impeccable';
|
||||
state = 'PICKING';
|
||||
}, 1800);
|
||||
|
||||
// Static-server / no-HMR fallback: if the wrapper is still around 2s after
|
||||
// the cleanup above, swap it out manually. By now React has either moved
|
||||
// on or the app isn't React at all.
|
||||
setTimeout(function() {
|
||||
const wrapper = document.querySelector('[data-impeccable-variants="' + acceptedSessionId + '"]');
|
||||
if (!wrapper) return;
|
||||
const accepted = wrapper.querySelector('[data-impeccable-variant="' + acceptedVariant + '"]');
|
||||
if (accepted && accepted.firstElementChild) {
|
||||
const parent = wrapper.parentElement;
|
||||
if (parent) parent.replaceChild(accepted.firstElementChild.cloneNode(true), wrapper);
|
||||
}
|
||||
}, 2000);
|
||||
}
|
||||
|
||||
function handleDiscard() {
|
||||
@@ -2117,26 +2144,31 @@ void main() {
|
||||
}
|
||||
|
||||
function cleanup() {
|
||||
// Remove any leftover variant wrapper from the live DOM.
|
||||
// After discard, the agent cleans the source, but on dev servers without
|
||||
// HMR the DOM still has the old wrapper, which confuses the picker.
|
||||
if (currentSessionId) {
|
||||
const wrapper = document.querySelector('[data-impeccable-variants="' + currentSessionId + '"]');
|
||||
if (wrapper) {
|
||||
// Restore the original element into the DOM
|
||||
const orig = wrapper.querySelector('[data-impeccable-variant="original"]');
|
||||
if (orig) {
|
||||
const content = orig.firstElementChild;
|
||||
if (content) {
|
||||
wrapper.parentElement.replaceChild(content, wrapper);
|
||||
} else {
|
||||
wrapper.remove();
|
||||
}
|
||||
} else {
|
||||
wrapper.remove();
|
||||
// Hide the wrapper immediately so variants disappear. DON'T structurally
|
||||
// mutate the DOM yet — HMR from the agent's source rewrite is on its way,
|
||||
// and a manual replaceChild under React causes NotFoundError when the
|
||||
// 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 cleanupSessionId = currentSessionId;
|
||||
if (cleanupSessionId) {
|
||||
const wrapper = document.querySelector('[data-impeccable-variants="' + cleanupSessionId + '"]');
|
||||
if (wrapper) wrapper.style.display = 'none';
|
||||
}
|
||||
setTimeout(function() {
|
||||
if (!cleanupSessionId) return;
|
||||
const wrapper = document.querySelector('[data-impeccable-variants="' + cleanupSessionId + '"]');
|
||||
if (!wrapper) return;
|
||||
const orig = wrapper.querySelector('[data-impeccable-variant="original"]');
|
||||
if (orig) {
|
||||
const content = orig.firstElementChild;
|
||||
if (content) {
|
||||
wrapper.parentElement.replaceChild(content, wrapper);
|
||||
return;
|
||||
}
|
||||
}
|
||||
}
|
||||
wrapper.remove();
|
||||
}, 2000);
|
||||
hideBar();
|
||||
hideHighlight();
|
||||
stopScrollTracking();
|
||||
|
||||
@@ -84,7 +84,8 @@ function broadcast(msg) {
|
||||
|
||||
function loadBrowserScripts() {
|
||||
// Detection script: look relative to the skill scripts dir, then fall back
|
||||
// to the npm package location (src/detect-antipatterns-browser.js)
|
||||
// to the npm package location (src/detect-antipatterns-browser.js).
|
||||
// This one IS cached — detect.js rarely changes during a session.
|
||||
const detectPaths = [
|
||||
path.join(__dirname, '..', '..', '..', '..', 'src', 'detect-antipatterns-browser.js'),
|
||||
path.join(process.cwd(), 'node_modules', 'impeccable', 'src', 'detect-antipatterns-browser.js'),
|
||||
@@ -94,16 +95,16 @@ function loadBrowserScripts() {
|
||||
try { detectScript = fs.readFileSync(p, 'utf-8'); break; } catch { /* try next */ }
|
||||
}
|
||||
|
||||
// live-browser.js: DO NOT cache. Return the path so the /live.js handler
|
||||
// can re-read on every request. Editing the browser script during iteration
|
||||
// should land on the next tab reload, not require a server restart.
|
||||
const livePath = path.join(__dirname, 'live-browser.js');
|
||||
let liveScript = '';
|
||||
try {
|
||||
liveScript = fs.readFileSync(livePath, 'utf-8');
|
||||
} catch {
|
||||
if (!fs.existsSync(livePath)) {
|
||||
process.stderr.write('Error: live-browser.js not found at ' + livePath + '\n');
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
return { detectScript, liveScript };
|
||||
return { detectScript, livePath };
|
||||
}
|
||||
|
||||
function hasProjectContext() {
|
||||
@@ -163,7 +164,7 @@ function validateEvent(msg) {
|
||||
// HTTP request handler
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
function createRequestHandler({ detectScript, liveScriptWithToken }) {
|
||||
function createRequestHandler({ detectScript, livePath }) {
|
||||
return (req, res) => {
|
||||
const url = new URL(req.url, `http://localhost:${state.port}`);
|
||||
res.setHeader('Access-Control-Allow-Origin', '*');
|
||||
@@ -175,8 +176,28 @@ function createRequestHandler({ detectScript, liveScriptWithToken }) {
|
||||
|
||||
// --- Scripts ---
|
||||
if (p === '/live.js') {
|
||||
res.writeHead(200, { 'Content-Type': 'application/javascript' });
|
||||
res.end(liveScriptWithToken);
|
||||
// Re-read from disk each request so edits to live-browser.js land on
|
||||
// the next tab reload. No-store headers prevent browser caching across
|
||||
// sessions — during iteration, a cached old script silently breaks
|
||||
// every subsequent session.
|
||||
let liveScript;
|
||||
try {
|
||||
liveScript = fs.readFileSync(livePath, 'utf-8');
|
||||
} catch (err) {
|
||||
res.writeHead(500, { 'Content-Type': 'text/plain' });
|
||||
res.end('Error reading live-browser.js: ' + err.message);
|
||||
return;
|
||||
}
|
||||
const body =
|
||||
`window.__IMPECCABLE_TOKEN__ = '${state.token}';\n` +
|
||||
`window.__IMPECCABLE_PORT__ = ${state.port};\n` +
|
||||
liveScript;
|
||||
res.writeHead(200, {
|
||||
'Content-Type': 'application/javascript',
|
||||
'Cache-Control': 'no-store, no-cache, must-revalidate, max-age=0',
|
||||
'Pragma': 'no-cache',
|
||||
});
|
||||
res.end(body);
|
||||
return;
|
||||
}
|
||||
if (p === '/detect.js' || p === '/') {
|
||||
@@ -632,13 +653,8 @@ const annotRoot = path.join(process.cwd(), '.impeccable-live', 'annotations');
|
||||
fs.mkdirSync(annotRoot, { recursive: true });
|
||||
state.sessionDir = fs.mkdtempSync(path.join(annotRoot, 'session-'));
|
||||
|
||||
const { detectScript, liveScript } = loadBrowserScripts();
|
||||
const liveScriptWithToken =
|
||||
`window.__IMPECCABLE_TOKEN__ = '${state.token}';\n` +
|
||||
`window.__IMPECCABLE_PORT__ = ${state.port};\n` +
|
||||
liveScript;
|
||||
|
||||
httpServer = http.createServer(createRequestHandler({ detectScript, liveScriptWithToken }));
|
||||
const { detectScript, livePath } = loadBrowserScripts();
|
||||
httpServer = http.createServer(createRequestHandler({ detectScript, livePath }));
|
||||
|
||||
httpServer.listen(state.port, '127.0.0.1', () => {
|
||||
fs.writeFileSync(LIVE_PID_FILE, JSON.stringify({ pid: process.pid, port: state.port, token: state.token }));
|
||||
|
||||
Reference in New Issue
Block a user