mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-14 15:16:35 +03:00
fix: address second round of PR review bot findings
cursor[bot]: - style: directives with dynamic values now fall back to source-preview instead of being scaffolded as boolean condition props that falsified the style in the detached preview. - class: directives carry a className probe, so v2 hydration answers the condition from the live DOM instead of always defaulting to false. - The existing-wrapper remount path now checks the mount result; a failed remount keeps the error card instead of advancing to a CYCLING bar over a page where nothing rendered. greptile-apps[bot]: - The repo-root live pointer records every booted app (most recent first) and resolution prefers the app whose helper server is alive, so a helper run from the repo root of a two-app monorepo can no longer be redirected onto the wrong app's session store by the last boot. Legacy single-value pointers still read. This work was produced with AI assistance (Claude Code). Co-Authored-By: Claude Code <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Code
parent
2d66c9acf1
commit
a6f965e8bf
@@ -5453,6 +5453,13 @@
|
||||
if (entry.probe && entry.probe.tag) {
|
||||
const selector = entry.probe.tag + (entry.probe.classes || []).map((c) => '.' + cssEscapeIdent(c)).join('');
|
||||
try { values[entry.prop] = !!liveEl.querySelector(selector); } catch { /* keep default */ }
|
||||
} else if (entry.probe && entry.probe.className) {
|
||||
// class:name directive: the live DOM answers directly, either on
|
||||
// the picked element itself or on a descendant carrying the class.
|
||||
try {
|
||||
values[entry.prop] = liveEl.classList.contains(entry.probe.className)
|
||||
|| !!liveEl.querySelector('.' + cssEscapeIdent(entry.probe.className));
|
||||
} catch { /* keep default */ }
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -5731,7 +5738,14 @@
|
||||
arrivedVariants = availableVariants;
|
||||
expectedVariants = Number(manifest.count) || expectedVariants || arrivedVariants;
|
||||
visibleVariant = visibleVariant > 0 && visibleVariant <= arrivedVariants ? visibleVariant : 1;
|
||||
await mountSvelteComponentVariant(visibleVariant || 1);
|
||||
const remounted = await mountSvelteComponentVariant(visibleVariant || 1);
|
||||
if (!remounted) {
|
||||
// The mount already reported the failure and raised the card.
|
||||
// Advancing to CYCLING here would show a bar claiming variants are
|
||||
// ready over a page where nothing rendered.
|
||||
saveSession();
|
||||
return;
|
||||
}
|
||||
setLiveState('CYCLING');
|
||||
showOrUpdateCyclingBar();
|
||||
saveSession();
|
||||
|
||||
@@ -258,11 +258,45 @@ export function writeRootsManifest(manifest) {
|
||||
if (path.resolve(manifest.repoRoot) !== path.resolve(manifest.appRoot)) {
|
||||
const pointer = pointerFilePath(manifest.repoRoot);
|
||||
fs.mkdirSync(path.dirname(pointer), { recursive: true });
|
||||
fs.writeFileSync(pointer, JSON.stringify({ appRoot: manifest.appRoot }));
|
||||
// The pointer records EVERY app that has booted live in this repo, most
|
||||
// recent first. A single last-boot-wins value made a helper run from the
|
||||
// repo root silently target whichever app booted last, even while an
|
||||
// earlier app's session was the one still live.
|
||||
const entries = readPointerEntries(manifest.repoRoot)
|
||||
.filter((entry) => path.resolve(entry.appRoot) !== path.resolve(manifest.appRoot));
|
||||
entries.unshift({ appRoot: manifest.appRoot, bootedAt: new Date().toISOString() });
|
||||
fs.writeFileSync(pointer, JSON.stringify({ version: 2, appRoots: entries }));
|
||||
}
|
||||
return file;
|
||||
}
|
||||
|
||||
function readPointerEntries(repoRoot) {
|
||||
try {
|
||||
const raw = JSON.parse(fs.readFileSync(pointerFilePath(repoRoot), 'utf-8'));
|
||||
if (Array.isArray(raw?.appRoots)) {
|
||||
return raw.appRoots.filter((entry) => entry && typeof entry.appRoot === 'string');
|
||||
}
|
||||
// v1 shape: a single { appRoot } value.
|
||||
if (raw && typeof raw.appRoot === 'string') return [{ appRoot: raw.appRoot }];
|
||||
return [];
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
||||
/** True when the app's live helper server is recorded and its pid is alive. */
|
||||
function hasLiveServer(appRoot) {
|
||||
try {
|
||||
const info = JSON.parse(fs.readFileSync(path.join(appRoot, '.impeccable', 'live', 'server.json'), 'utf-8'));
|
||||
if (!info || typeof info.pid !== 'number') return false;
|
||||
process.kill(info.pid, 0);
|
||||
return true;
|
||||
} catch (err) {
|
||||
// EPERM: the process exists but is not signalable by this user.
|
||||
return err?.code === 'EPERM';
|
||||
}
|
||||
}
|
||||
|
||||
function readManifestAt(appRoot) {
|
||||
try {
|
||||
const raw = JSON.parse(fs.readFileSync(rootsFilePath(appRoot), 'utf-8'));
|
||||
@@ -297,13 +331,16 @@ export function resolveLiveRoots(cwd = process.cwd(), { targetPath = null } = {}
|
||||
|
||||
const gitRoot = findGitRoot(absCwd);
|
||||
if (gitRoot) {
|
||||
try {
|
||||
const pointer = JSON.parse(fs.readFileSync(pointerFilePath(gitRoot), 'utf-8'));
|
||||
if (pointer && typeof pointer.appRoot === 'string') {
|
||||
const viaPointer = readManifestAt(pointer.appRoot);
|
||||
if (viaPointer) return { manifest: viaPointer, source: 'pointer' };
|
||||
}
|
||||
} catch { /* no pointer */ }
|
||||
// Several apps in one repo may have booted live. Prefer the one whose
|
||||
// helper server is actually running; a stale pointer entry must not
|
||||
// redirect status/poll/accept onto the wrong app's session store.
|
||||
const candidates = readPointerEntries(gitRoot)
|
||||
.map((entry) => readManifestAt(entry.appRoot))
|
||||
.filter(Boolean);
|
||||
if (candidates.length > 0) {
|
||||
const live = candidates.find((manifest) => hasLiveServer(manifest.appRoot));
|
||||
return { manifest: live || candidates[0], source: 'pointer' };
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -374,16 +374,34 @@ function analyzeAttributes(node, analysis, scopes) {
|
||||
}
|
||||
break;
|
||||
}
|
||||
case 'ClassDirective':
|
||||
case 'StyleDirective': {
|
||||
case 'ClassDirective': {
|
||||
const expr = attr.expression;
|
||||
if (expr && isFree(expr, scopes)) {
|
||||
const text = exprText(analysis.source, expr);
|
||||
const entry = analysis.propFor(text, 'condition');
|
||||
// The directive's class name is literal, so the live DOM answers
|
||||
// the condition directly: the class is either present or not.
|
||||
const entry = analysis.propFor(text, 'condition', {
|
||||
probe: { className: attr.name },
|
||||
});
|
||||
analysis.replacements.push({ start: expr.start, end: expr.end, prop: entry.prop });
|
||||
}
|
||||
break;
|
||||
}
|
||||
case 'StyleDirective': {
|
||||
// Unlike ClassDirective, a style directive stores its value in
|
||||
// attribute shape: `true` for the shorthand, else an array of parts.
|
||||
const parts = attr.value === true ? [] : (Array.isArray(attr.value) ? attr.value : [attr.value]);
|
||||
const dynamic = parts.some((part) => part?.type === 'ExpressionTag' && isFree(part.expression, scopes));
|
||||
const shorthandFree = attr.value === true && isFree({ type: 'Identifier', name: attr.name }, scopes);
|
||||
if (dynamic || shorthandFree) {
|
||||
// style:opacity={x} carries a css VALUE, not a boolean, and the
|
||||
// computed value on the live element is not reliably recoverable in
|
||||
// the shape the expression produced. A falsified style is worse
|
||||
// than an HMR-resetting preview.
|
||||
analysis.fail(`style:${attr.name} with a dynamic value requires source-preview mode`);
|
||||
}
|
||||
break;
|
||||
}
|
||||
case 'BindDirective':
|
||||
analysis.fail(`bind:${attr.name} requires source-preview mode`);
|
||||
return;
|
||||
|
||||
@@ -196,3 +196,46 @@ describe('review regressions: walk bounds', () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('review regressions: multi-app pointer', () => {
|
||||
it('prefers the app whose live server is running over the last boot', () => {
|
||||
const repo = realpathSync(mkdtempSync(join(tmpdir(), 'impeccable-roots-multi-')));
|
||||
try {
|
||||
mkdirSync(join(repo, '.git'), { recursive: true });
|
||||
for (const name of ['siteA', 'siteB']) {
|
||||
write(repo, `${name}/vite.config.js`, 'export default {};');
|
||||
write(repo, `${name}/package.json`, `{"name":"${name}"}`);
|
||||
}
|
||||
const a = resolveRoots({ cwd: repo, targetPath: join(repo, 'siteA/vite.config.js') }).manifest;
|
||||
const b = resolveRoots({ cwd: repo, targetPath: join(repo, 'siteB/vite.config.js') }).manifest;
|
||||
writeRootsManifest(a);
|
||||
writeRootsManifest(b); // B booted last: a naive pointer now points at B
|
||||
|
||||
// A's helper server is the one alive (this test process's pid).
|
||||
write(repo, 'siteA/.impeccable/live/server.json', JSON.stringify({ pid: process.pid, port: 1, token: 't' }));
|
||||
write(repo, 'siteB/.impeccable/live/server.json', JSON.stringify({ pid: 999999999, port: 2, token: 't' }));
|
||||
|
||||
const resolved = resolveLiveRoots(repo);
|
||||
assert.equal(resolved.source, 'pointer');
|
||||
assert.equal(resolved.manifest.appRoot, join(repo, 'siteA'));
|
||||
} finally {
|
||||
rmSync(repo, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('reads a legacy single-value pointer', () => {
|
||||
const repo = realpathSync(mkdtempSync(join(tmpdir(), 'impeccable-roots-legacy-')));
|
||||
try {
|
||||
mkdirSync(join(repo, '.git'), { recursive: true });
|
||||
write(repo, 'app/vite.config.js', 'export default {};');
|
||||
const m = resolveRoots({ cwd: repo, targetPath: join(repo, 'app/vite.config.js') }).manifest;
|
||||
// Write the manifest, then downgrade the pointer to the v1 shape.
|
||||
writeRootsManifest(m);
|
||||
write(repo, '.impeccable/live/app-root.json', JSON.stringify({ appRoot: join(repo, 'app') }));
|
||||
const resolved = resolveLiveRoots(repo);
|
||||
assert.equal(resolved.manifest.appRoot, join(repo, 'app'));
|
||||
} finally {
|
||||
rmSync(repo, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -215,3 +215,24 @@ describe('review regressions: reserved prop names', () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('review regressions: directives', () => {
|
||||
it('class directives carry a className probe for live hydration', () => {
|
||||
const src = `<div class:active={isActive}><span>{label}</span></div>`;
|
||||
const res = analyzeSvelteMarkup(src, parse);
|
||||
assert.equal(res.ok, true, res.reason);
|
||||
const cond = res.contract.find((c) => c.expr === 'isActive');
|
||||
assert.deepEqual(cond.probe, { className: 'active' });
|
||||
});
|
||||
|
||||
it('style directives with dynamic values fall back to source-preview', () => {
|
||||
const res = analyzeSvelteMarkup(`<div style:opacity={fade}>x</div>`, parse);
|
||||
assert.equal(res.ok, false);
|
||||
assert.match(res.reason, /style:opacity/);
|
||||
});
|
||||
|
||||
it('style directives with static values stay supported', () => {
|
||||
const res = analyzeSvelteMarkup(`<div style:color="red">{note}</div>`, parse);
|
||||
assert.equal(res.ok, true, res.reason);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user