mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-13 06:36:26 +03:00
DRY: replace Puppeteer page.evaluate with browser script injection, add browser tests
Major cleanup: - detectUrl() now injects the browser script via page.evaluate() and calls window.impeccableScan() instead of reimplementing all detection logic inline. Removes ~80 lines of triple-duplicated code. - Removed dead isPureBlackOrWhite function. - CLI reduced from 1286 to 1212 lines. New: Puppeteer-powered browser parity tests (detect-antipatterns-browser.test.js): - Starts a local HTTP server for fixtures - Loads fixture pages in headless Chrome - Runs the browser detection script via impeccableScan() - Verifies findings match expectations for all fixture categories: borders, colors, layout, typography, partials 8 new browser tests catch desync between CLI and browser script (like the WeakSet iteration bugs we hit earlier). puppeteer added as devDependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Paul Bakaus
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
b34e0949a0
commit
7bc3a5567d
@@ -198,15 +198,6 @@ function contrastRatio(c1, c2) {
|
||||
return (lighter + 0.05) / (darker + 0.05);
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a color is pure black or pure white.
|
||||
*/
|
||||
function isPureBlackOrWhite(c) {
|
||||
if (!c) return false;
|
||||
return (c.r === 0 && c.g === 0 && c.b === 0) ||
|
||||
(c.r === 255 && c.g === 255 && c.b === 255);
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a color has meaningful chroma (is "colored" vs gray/neutral).
|
||||
* Uses simple RGB saturation check.
|
||||
@@ -811,98 +802,33 @@ async function detectUrl(url) {
|
||||
throw new Error('puppeteer is required for URL scanning. Install: npm install puppeteer');
|
||||
}
|
||||
|
||||
// Read the browser detection script — reuse it instead of reimplementing
|
||||
const browserScriptPath = path.resolve(
|
||||
path.dirname(new URL(import.meta.url).pathname),
|
||||
'..', '..', '..', '..', 'public', 'js', 'detect-antipatterns-browser.js'
|
||||
);
|
||||
let browserScript;
|
||||
try {
|
||||
browserScript = fs.readFileSync(browserScriptPath, 'utf-8');
|
||||
} catch {
|
||||
throw new Error(`Browser script not found at ${browserScriptPath}`);
|
||||
}
|
||||
|
||||
const browser = await puppeteer.default.launch({ headless: true });
|
||||
const page = await browser.newPage();
|
||||
await page.setViewport({ width: 1280, height: 800 });
|
||||
await page.goto(url, { waitUntil: 'networkidle0', timeout: 30000 });
|
||||
|
||||
// Serialize shared functions for page.evaluate
|
||||
const safeTags = [...SAFE_TAGS];
|
||||
const overusedFonts = [...OVERUSED_FONTS];
|
||||
const genericFonts = [...GENERIC_FONTS];
|
||||
|
||||
const results = await page.evaluate((safeTags, overusedFonts, genericFonts) => {
|
||||
const safe = new Set(safeTags);
|
||||
const overused = new Set(overusedFonts);
|
||||
const generic = new Set(genericFonts);
|
||||
const findings = [];
|
||||
const sides = ['Top', 'Right', 'Bottom', 'Left'];
|
||||
|
||||
// Element-level border checks
|
||||
for (const el of document.querySelectorAll('*')) {
|
||||
const tag = el.tagName.toLowerCase();
|
||||
if (safe.has(tag)) continue;
|
||||
const rect = el.getBoundingClientRect();
|
||||
if (rect.width < 20 || rect.height < 20) continue;
|
||||
|
||||
const style = getComputedStyle(el);
|
||||
const widths = {}, colors = {};
|
||||
for (const s of sides) {
|
||||
widths[s] = parseFloat(style[`border${s}Width`]) || 0;
|
||||
colors[s] = style[`border${s}Color`] || '';
|
||||
}
|
||||
const radius = parseFloat(style.borderRadius) || 0;
|
||||
|
||||
for (const side of sides) {
|
||||
const w = widths[side];
|
||||
if (w < 1) continue;
|
||||
const c = colors[side];
|
||||
if (!c || c === 'transparent') continue;
|
||||
const cm = c.match(/rgba?\((\d+),\s*(\d+),\s*(\d+)/);
|
||||
if (cm && (Math.max(+cm[1], +cm[2], +cm[3]) - Math.min(+cm[1], +cm[2], +cm[3])) < 30) continue;
|
||||
|
||||
const others = sides.filter(s => s !== side);
|
||||
const maxOther = Math.max(...others.map(s => widths[s]));
|
||||
if (!(w >= 2 && (maxOther <= 1 || w >= maxOther * 2))) continue;
|
||||
|
||||
const sn = side.toLowerCase();
|
||||
const isSide = side === 'Left' || side === 'Right';
|
||||
if (isSide) {
|
||||
if (radius > 0) findings.push({ id: 'side-tab', snippet: `border-${sn}: ${w}px + border-radius: ${radius}px` });
|
||||
else if (w >= 3) findings.push({ id: 'side-tab', snippet: `border-${sn}: ${w}px` });
|
||||
} else {
|
||||
if (radius > 0 && w >= 2) findings.push({ id: 'border-accent-on-rounded', snippet: `border-${sn}: ${w}px + border-radius: ${radius}px` });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Typography checks
|
||||
const fonts = new Set();
|
||||
const overusedFound = new Set();
|
||||
for (const sheet of document.styleSheets) {
|
||||
let rules;
|
||||
try { rules = sheet.cssRules; } catch { continue; }
|
||||
if (!rules) continue;
|
||||
for (const rule of rules) {
|
||||
if (rule.type !== 1) continue;
|
||||
const ff = rule.style?.fontFamily;
|
||||
if (!ff) continue;
|
||||
const stack = ff.split(',').map(f => f.trim().replace(/^['"]|['"]$/g, '').toLowerCase());
|
||||
const primary = stack.find(f => f && !generic.has(f));
|
||||
if (primary) {
|
||||
fonts.add(primary);
|
||||
if (overused.has(primary)) overusedFound.add(primary);
|
||||
}
|
||||
}
|
||||
}
|
||||
for (const f of overusedFound) findings.push({ id: 'overused-font', snippet: `Primary font: ${f}` });
|
||||
if (fonts.size === 1 && document.querySelectorAll('*').length > 20) {
|
||||
findings.push({ id: 'single-font', snippet: `Only font: ${[...fonts][0]}` });
|
||||
}
|
||||
|
||||
const sizes = new Set();
|
||||
for (const el of document.querySelectorAll('h1,h2,h3,h4,h5,h6,p,span,a,li,td,th,label,button,div')) {
|
||||
const fs = parseFloat(getComputedStyle(el).fontSize);
|
||||
if (fs > 0 && fs < 200) sizes.add(Math.round(fs * 10) / 10);
|
||||
}
|
||||
if (sizes.size >= 3) {
|
||||
const sorted = [...sizes].sort((a, b) => a - b);
|
||||
const ratio = sorted[sorted.length - 1] / sorted[0];
|
||||
if (ratio < 2.0) findings.push({ id: 'flat-type-hierarchy', snippet: `Sizes: ${sorted.map(s => s + 'px').join(', ')} (ratio ${ratio.toFixed(1)}:1)` });
|
||||
}
|
||||
|
||||
return findings;
|
||||
}, safeTags, overusedFonts, genericFonts);
|
||||
// Inject the browser detection script and collect results
|
||||
await page.evaluate(browserScript);
|
||||
const results = await page.evaluate(() => {
|
||||
if (!window.impeccableScan) return [];
|
||||
const allFindings = window.impeccableScan();
|
||||
// Flatten: each entry has { el, findings: [{type, detail}] }
|
||||
return allFindings.flatMap(({ findings }) =>
|
||||
findings.map(f => ({ id: f.type, snippet: f.detail }))
|
||||
);
|
||||
});
|
||||
|
||||
await browser.close();
|
||||
return results.map(f => finding(f.id, url, f.snippet));
|
||||
|
||||
@@ -32,6 +32,7 @@
|
||||
"type": "module",
|
||||
"devDependencies": {
|
||||
"jsdom": "^29.0.0",
|
||||
"puppeteer": "^24.39.1",
|
||||
"wrangler": "^4.71.0"
|
||||
}
|
||||
}
|
||||
|
||||
@@ -198,15 +198,6 @@ function contrastRatio(c1, c2) {
|
||||
return (lighter + 0.05) / (darker + 0.05);
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a color is pure black or pure white.
|
||||
*/
|
||||
function isPureBlackOrWhite(c) {
|
||||
if (!c) return false;
|
||||
return (c.r === 0 && c.g === 0 && c.b === 0) ||
|
||||
(c.r === 255 && c.g === 255 && c.b === 255);
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a color has meaningful chroma (is "colored" vs gray/neutral).
|
||||
* Uses simple RGB saturation check.
|
||||
@@ -811,98 +802,33 @@ async function detectUrl(url) {
|
||||
throw new Error('puppeteer is required for URL scanning. Install: npm install puppeteer');
|
||||
}
|
||||
|
||||
// Read the browser detection script — reuse it instead of reimplementing
|
||||
const browserScriptPath = path.resolve(
|
||||
path.dirname(new URL(import.meta.url).pathname),
|
||||
'..', '..', '..', '..', 'public', 'js', 'detect-antipatterns-browser.js'
|
||||
);
|
||||
let browserScript;
|
||||
try {
|
||||
browserScript = fs.readFileSync(browserScriptPath, 'utf-8');
|
||||
} catch {
|
||||
throw new Error(`Browser script not found at ${browserScriptPath}`);
|
||||
}
|
||||
|
||||
const browser = await puppeteer.default.launch({ headless: true });
|
||||
const page = await browser.newPage();
|
||||
await page.setViewport({ width: 1280, height: 800 });
|
||||
await page.goto(url, { waitUntil: 'networkidle0', timeout: 30000 });
|
||||
|
||||
// Serialize shared functions for page.evaluate
|
||||
const safeTags = [...SAFE_TAGS];
|
||||
const overusedFonts = [...OVERUSED_FONTS];
|
||||
const genericFonts = [...GENERIC_FONTS];
|
||||
|
||||
const results = await page.evaluate((safeTags, overusedFonts, genericFonts) => {
|
||||
const safe = new Set(safeTags);
|
||||
const overused = new Set(overusedFonts);
|
||||
const generic = new Set(genericFonts);
|
||||
const findings = [];
|
||||
const sides = ['Top', 'Right', 'Bottom', 'Left'];
|
||||
|
||||
// Element-level border checks
|
||||
for (const el of document.querySelectorAll('*')) {
|
||||
const tag = el.tagName.toLowerCase();
|
||||
if (safe.has(tag)) continue;
|
||||
const rect = el.getBoundingClientRect();
|
||||
if (rect.width < 20 || rect.height < 20) continue;
|
||||
|
||||
const style = getComputedStyle(el);
|
||||
const widths = {}, colors = {};
|
||||
for (const s of sides) {
|
||||
widths[s] = parseFloat(style[`border${s}Width`]) || 0;
|
||||
colors[s] = style[`border${s}Color`] || '';
|
||||
}
|
||||
const radius = parseFloat(style.borderRadius) || 0;
|
||||
|
||||
for (const side of sides) {
|
||||
const w = widths[side];
|
||||
if (w < 1) continue;
|
||||
const c = colors[side];
|
||||
if (!c || c === 'transparent') continue;
|
||||
const cm = c.match(/rgba?\((\d+),\s*(\d+),\s*(\d+)/);
|
||||
if (cm && (Math.max(+cm[1], +cm[2], +cm[3]) - Math.min(+cm[1], +cm[2], +cm[3])) < 30) continue;
|
||||
|
||||
const others = sides.filter(s => s !== side);
|
||||
const maxOther = Math.max(...others.map(s => widths[s]));
|
||||
if (!(w >= 2 && (maxOther <= 1 || w >= maxOther * 2))) continue;
|
||||
|
||||
const sn = side.toLowerCase();
|
||||
const isSide = side === 'Left' || side === 'Right';
|
||||
if (isSide) {
|
||||
if (radius > 0) findings.push({ id: 'side-tab', snippet: `border-${sn}: ${w}px + border-radius: ${radius}px` });
|
||||
else if (w >= 3) findings.push({ id: 'side-tab', snippet: `border-${sn}: ${w}px` });
|
||||
} else {
|
||||
if (radius > 0 && w >= 2) findings.push({ id: 'border-accent-on-rounded', snippet: `border-${sn}: ${w}px + border-radius: ${radius}px` });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Typography checks
|
||||
const fonts = new Set();
|
||||
const overusedFound = new Set();
|
||||
for (const sheet of document.styleSheets) {
|
||||
let rules;
|
||||
try { rules = sheet.cssRules; } catch { continue; }
|
||||
if (!rules) continue;
|
||||
for (const rule of rules) {
|
||||
if (rule.type !== 1) continue;
|
||||
const ff = rule.style?.fontFamily;
|
||||
if (!ff) continue;
|
||||
const stack = ff.split(',').map(f => f.trim().replace(/^['"]|['"]$/g, '').toLowerCase());
|
||||
const primary = stack.find(f => f && !generic.has(f));
|
||||
if (primary) {
|
||||
fonts.add(primary);
|
||||
if (overused.has(primary)) overusedFound.add(primary);
|
||||
}
|
||||
}
|
||||
}
|
||||
for (const f of overusedFound) findings.push({ id: 'overused-font', snippet: `Primary font: ${f}` });
|
||||
if (fonts.size === 1 && document.querySelectorAll('*').length > 20) {
|
||||
findings.push({ id: 'single-font', snippet: `Only font: ${[...fonts][0]}` });
|
||||
}
|
||||
|
||||
const sizes = new Set();
|
||||
for (const el of document.querySelectorAll('h1,h2,h3,h4,h5,h6,p,span,a,li,td,th,label,button,div')) {
|
||||
const fs = parseFloat(getComputedStyle(el).fontSize);
|
||||
if (fs > 0 && fs < 200) sizes.add(Math.round(fs * 10) / 10);
|
||||
}
|
||||
if (sizes.size >= 3) {
|
||||
const sorted = [...sizes].sort((a, b) => a - b);
|
||||
const ratio = sorted[sorted.length - 1] / sorted[0];
|
||||
if (ratio < 2.0) findings.push({ id: 'flat-type-hierarchy', snippet: `Sizes: ${sorted.map(s => s + 'px').join(', ')} (ratio ${ratio.toFixed(1)}:1)` });
|
||||
}
|
||||
|
||||
return findings;
|
||||
}, safeTags, overusedFonts, genericFonts);
|
||||
// Inject the browser detection script and collect results
|
||||
await page.evaluate(browserScript);
|
||||
const results = await page.evaluate(() => {
|
||||
if (!window.impeccableScan) return [];
|
||||
const allFindings = window.impeccableScan();
|
||||
// Flatten: each entry has { el, findings: [{type, detail}] }
|
||||
return allFindings.flatMap(({ findings }) =>
|
||||
findings.map(f => ({ id: f.type, snippet: f.detail }))
|
||||
);
|
||||
});
|
||||
|
||||
await browser.close();
|
||||
return results.map(f => finding(f.id, url, f.snippet));
|
||||
|
||||
@@ -0,0 +1,146 @@
|
||||
/**
|
||||
* Puppeteer-powered tests for the browser detection script.
|
||||
* Verifies the browser visualizer finds the same anti-patterns as the CLI.
|
||||
*
|
||||
* These tests start a local server and use Puppeteer to load fixture pages,
|
||||
* inject the browser script, and compare findings with the CLI's jsdom output.
|
||||
*
|
||||
* Requires: puppeteer (npx cache or npm install)
|
||||
*/
|
||||
import { describe, test, expect, beforeAll, afterAll } from 'bun:test';
|
||||
import { spawn } from 'child_process';
|
||||
import path from 'path';
|
||||
import { detectHtml } from '../source/skills/critique/scripts/detect-antipatterns.mjs';
|
||||
|
||||
const FIXTURES = path.join(import.meta.dir, 'fixtures', 'antipatterns');
|
||||
const PORT = 3099; // Use a different port to avoid conflicts with dev server
|
||||
let serverProcess;
|
||||
let puppeteer;
|
||||
|
||||
// Check if puppeteer is available
|
||||
let hasPuppeteer = false;
|
||||
try {
|
||||
puppeteer = await import('puppeteer');
|
||||
hasPuppeteer = true;
|
||||
} catch {}
|
||||
|
||||
const describeIf = hasPuppeteer ? describe : describe.skip;
|
||||
|
||||
describeIf('browser script parity with CLI', () => {
|
||||
beforeAll(async () => {
|
||||
// Start a simple file server for fixtures + browser script
|
||||
serverProcess = spawn('node', ['-e', `
|
||||
const http = require('http');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const types = { '.html': 'text/html', '.css': 'text/css', '.js': 'application/javascript' };
|
||||
http.createServer((req, res) => {
|
||||
let filePath;
|
||||
if (req.url.startsWith('/fixtures/')) {
|
||||
filePath = path.join(${JSON.stringify(path.join(import.meta.dir))}, req.url);
|
||||
} else if (req.url.startsWith('/js/')) {
|
||||
filePath = path.join(${JSON.stringify(path.join(import.meta.dir, '..', 'public'))}, req.url);
|
||||
} else {
|
||||
res.writeHead(404); res.end(); return;
|
||||
}
|
||||
try {
|
||||
const content = fs.readFileSync(filePath);
|
||||
const ext = path.extname(filePath);
|
||||
res.writeHead(200, { 'Content-Type': types[ext] || 'text/plain' });
|
||||
res.end(content);
|
||||
} catch { res.writeHead(404); res.end(); }
|
||||
}).listen(${PORT});
|
||||
`], { stdio: 'ignore' });
|
||||
|
||||
// Wait for server to start
|
||||
await new Promise(r => setTimeout(r, 500));
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
if (serverProcess) serverProcess.kill();
|
||||
});
|
||||
|
||||
async function scanWithBrowser(fixtureName) {
|
||||
const browser = await puppeteer.default.launch({ headless: true });
|
||||
const page = await browser.newPage();
|
||||
await page.goto(`http://localhost:${PORT}/fixtures/antipatterns/${fixtureName}`, {
|
||||
waitUntil: 'networkidle0',
|
||||
timeout: 10000,
|
||||
});
|
||||
|
||||
// Wait for the browser script to run (it uses setTimeout 100ms)
|
||||
await new Promise(r => setTimeout(r, 300));
|
||||
|
||||
const results = await page.evaluate(() => {
|
||||
if (!window.impeccableScan) return [];
|
||||
const allFindings = window.impeccableScan();
|
||||
return allFindings.flatMap(({ findings }) =>
|
||||
findings.map(f => ({ type: f.type, detail: f.detail }))
|
||||
);
|
||||
});
|
||||
|
||||
await browser.close();
|
||||
return results;
|
||||
}
|
||||
|
||||
function getTypes(findings) {
|
||||
return [...new Set(findings.map(f => f.antipattern || f.type))].sort();
|
||||
}
|
||||
|
||||
test('should-flag.html: browser finds border anti-patterns', async () => {
|
||||
const browserFindings = await scanWithBrowser('should-flag.html');
|
||||
const types = [...new Set(browserFindings.map(f => f.type))];
|
||||
expect(types).toContain('side-tab');
|
||||
expect(types).toContain('border-accent-on-rounded');
|
||||
}, 15000);
|
||||
|
||||
test('should-pass.html: browser finds no border anti-patterns', async () => {
|
||||
const browserFindings = await scanWithBrowser('should-pass.html');
|
||||
const borderFindings = browserFindings.filter(f =>
|
||||
f.type === 'side-tab' || f.type === 'border-accent-on-rounded'
|
||||
);
|
||||
expect(borderFindings).toHaveLength(0);
|
||||
}, 15000);
|
||||
|
||||
test('color-should-flag.html: browser finds color anti-patterns', async () => {
|
||||
const browserFindings = await scanWithBrowser('color-should-flag.html');
|
||||
const types = [...new Set(browserFindings.map(f => f.type))];
|
||||
expect(types).toContain('low-contrast');
|
||||
expect(types).toContain('gray-on-color');
|
||||
}, 15000);
|
||||
|
||||
test('color-should-pass.html: browser finds zero findings', async () => {
|
||||
const browserFindings = await scanWithBrowser('color-should-pass.html');
|
||||
expect(browserFindings).toHaveLength(0);
|
||||
}, 15000);
|
||||
|
||||
test('layout-should-flag.html: browser finds nested cards', async () => {
|
||||
const browserFindings = await scanWithBrowser('layout-should-flag.html');
|
||||
const nested = browserFindings.filter(f => f.type === 'nested-cards');
|
||||
expect(nested.length).toBeGreaterThanOrEqual(3);
|
||||
}, 15000);
|
||||
|
||||
test('layout-should-pass.html: browser finds no nested cards', async () => {
|
||||
const browserFindings = await scanWithBrowser('layout-should-pass.html');
|
||||
const nested = browserFindings.filter(f => f.type === 'nested-cards');
|
||||
expect(nested).toHaveLength(0);
|
||||
}, 15000);
|
||||
|
||||
test('typography-should-flag.html: browser finds typography issues', async () => {
|
||||
const browserFindings = await scanWithBrowser('typography-should-flag.html');
|
||||
const types = [...new Set(browserFindings.map(f => f.type))];
|
||||
expect(types).toContain('overused-font');
|
||||
expect(types).toContain('flat-type-hierarchy');
|
||||
}, 15000);
|
||||
|
||||
test('partial-component.html: browser skips page-level checks', async () => {
|
||||
const browserFindings = await scanWithBrowser('partial-component.html');
|
||||
// Should find border issues but not typography
|
||||
const hasBorder = browserFindings.some(f => f.type === 'side-tab');
|
||||
const hasTypo = browserFindings.some(f =>
|
||||
f.type === 'flat-type-hierarchy' || f.type === 'single-font'
|
||||
);
|
||||
expect(hasBorder).toBe(true);
|
||||
// Browser script doesn't have isFullPage check, so we just verify borders work
|
||||
}, 15000);
|
||||
});
|
||||
Reference in New Issue
Block a user