From f90e640892235f87cdf38fa132ece5ad6c2e9b3f Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Tue, 17 Mar 2026 15:54:00 -0700 Subject: [PATCH] Fix nested card detection, drop identical-card-grid - Nested cards: fixed dedup to use WeakSet on actual elements instead of tag-name key, so all nested card instances are found (not just the first div-in-div pair). Now catches all 4+ nesting examples. - Dropped identical-card-grid: too many legitimate uses (data displays, pricing cards, navigation tiles) make false positives unavoidable. - Removed from CLI, browser script, tests, and ANTIPATTERNS registry. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../critique/scripts/detect-antipatterns.mjs | 53 ++----------------- public/js/detect-antipatterns-browser.js | 19 ------- .../critique/scripts/detect-antipatterns.mjs | 53 ++----------------- tests/detect-antipatterns.test.js | 16 ++---- 4 files changed, 12 insertions(+), 129 deletions(-) diff --git a/.claude/skills/critique/scripts/detect-antipatterns.mjs b/.claude/skills/critique/scripts/detect-antipatterns.mjs index 4414637e4..745796ef2 100644 --- a/.claude/skills/critique/scripts/detect-antipatterns.mjs +++ b/.claude/skills/critique/scripts/detect-antipatterns.mjs @@ -119,12 +119,6 @@ const ANTIPATTERNS = [ description: 'Cards inside cards create visual noise and excessive depth. Flatten the hierarchy — use spacing, typography, and dividers instead of nesting containers.', }, - { - id: 'identical-card-grid', - name: 'Identical card grid', - description: - 'Same-sized cards with identical icon + heading + text structure, repeated endlessly. Vary card sizes, layouts, or content structure to create visual interest.', - }, { id: 'monotonous-spacing', name: 'Monotonous spacing', @@ -602,71 +596,32 @@ function checkPageLayout(document, window) { // --- Nested cards --- const allEls = document.querySelectorAll('*'); - const flaggedNested = new Set(); + const flaggedEls = new WeakSet(); for (const el of allEls) { if (!isCardLike(el, window)) continue; + if (flaggedEls.has(el)) continue; const tag = el.tagName.toLowerCase(); const cls = el.getAttribute?.('class') || ''; const rawStyle = el.getAttribute?.('style') || ''; - // Exclude elements that look like non-card components if (['pre', 'code'].includes(tag)) continue; - // Exclude absolutely/fixed positioned elements (dropdowns, modals, tooltips) if (/\b(?:absolute|fixed)\b/.test(cls) || /position\s*:\s*(?:absolute|fixed)/i.test(rawStyle)) continue; - // Exclude small elements (badges, chips, icons) — text < 20 chars if ((el.textContent?.trim().length || 0) < 20) continue; - // Exclude form elements that happen to match card heuristics if (/\b(?:dropdown|popover|tooltip|menu|modal|dialog)\b/i.test(cls)) continue; // Walk up to find card-like ancestor let parent = el.parentElement; while (parent) { if (isCardLike(parent, window)) { - const key = `${parent.tagName}:${el.tagName}`; - if (!flaggedNested.has(key)) { - flaggedNested.add(key); - findings.push({ id: 'nested-cards', snippet: `Card inside card (${tag} in ${parent.tagName.toLowerCase()})` }); - } + flaggedEls.add(el); + findings.push({ id: 'nested-cards', snippet: `Card inside card (${tag} in ${parent.tagName.toLowerCase()})` }); break; } parent = parent.parentElement; } } - // --- Identical card grid --- - const gridParents = document.querySelectorAll('[class*="grid"], [style*="display: grid"], [style*="display: flex"]'); - for (const grid of gridParents) { - const children = [...grid.children].filter(c => { - const tag = c.tagName.toLowerCase(); - return tag !== 'script' && tag !== 'style'; - }); - if (children.length < 3) continue; - - // Compare structural fingerprint of each child - function fingerprint(el) { - const childTags = [...el.children].map(c => c.tagName.toLowerCase()); - // Check for icon-like element (svg, img, or div with fixed size classes) - const hasIcon = childTags.includes('svg') || childTags.includes('img') || - [...el.children].some(c => { - const cls = c.getAttribute?.('class') || ''; - return /\bw-\d+\b.*\bh-\d+\b/.test(cls) && /\brounded/.test(cls); - }); - const hasHeading = childTags.some(t => /^h[1-6]$/.test(t)); - const hasParagraph = childTags.includes('p'); - return `icon:${hasIcon}|h:${hasHeading}|p:${hasParagraph}|children:${childTags.length}`; - } - - const fps = children.map(fingerprint); - const allSame = fps.every(f => f === fps[0]); - // Only flag if structure includes icon + heading + paragraph (the template pattern) - if (allSame && fps[0].includes('icon:true') && fps[0].includes('h:true') && fps[0].includes('p:true')) { - findings.push({ - id: 'identical-card-grid', - snippet: `${children.length} identical cards (icon + heading + text)`, - }); - } - } // --- Monotonous spacing --- // Regex on raw HTML — jsdom doesn't compute inline px spacing reliably diff --git a/public/js/detect-antipatterns-browser.js b/public/js/detect-antipatterns-browser.js index db2f8210e..64c729973 100644 --- a/public/js/detect-antipatterns-browser.js +++ b/public/js/detect-antipatterns-browser.js @@ -319,24 +319,6 @@ } } - // --- Identical card grid --- - for (const grid of document.querySelectorAll('[class*="grid"], [style*="display: grid"], [style*="display: flex"]')) { - const children = [...grid.children].filter(c => !['script','style'].includes(c.tagName.toLowerCase())); - if (children.length < 3) continue; - - function fingerprint(el) { - const tags = [...el.children].map(c => c.tagName.toLowerCase()); - const hasIcon = tags.includes('svg') || tags.includes('img') || - [...el.children].some(c => /\bw-\d+\b.*\bh-\d+\b/.test(c.getAttribute('class') || '') && /\brounded/.test(c.getAttribute('class') || '')); - return `icon:${hasIcon}|h:${tags.some(t => /^h[1-6]$/.test(t))}|p:${tags.includes('p')}|n:${tags.length}`; - } - - const fps = children.map(fingerprint); - if (fps.every(f => f === fps[0]) && fps[0].includes('icon:true') && fps[0].includes('h:true') && fps[0].includes('p:true')) { - findings.push({ type: 'identical-card-grid', detail: `${children.length} identical cards`, el: grid }); - } - } - return findings; } @@ -357,7 +339,6 @@ 'gradient-text': 'gradient text', 'ai-color-palette': 'AI palette', 'nested-cards': 'nested cards', - 'identical-card-grid': 'identical grid', }; function highlight(el, findings) { diff --git a/source/skills/critique/scripts/detect-antipatterns.mjs b/source/skills/critique/scripts/detect-antipatterns.mjs index 4414637e4..745796ef2 100644 --- a/source/skills/critique/scripts/detect-antipatterns.mjs +++ b/source/skills/critique/scripts/detect-antipatterns.mjs @@ -119,12 +119,6 @@ const ANTIPATTERNS = [ description: 'Cards inside cards create visual noise and excessive depth. Flatten the hierarchy — use spacing, typography, and dividers instead of nesting containers.', }, - { - id: 'identical-card-grid', - name: 'Identical card grid', - description: - 'Same-sized cards with identical icon + heading + text structure, repeated endlessly. Vary card sizes, layouts, or content structure to create visual interest.', - }, { id: 'monotonous-spacing', name: 'Monotonous spacing', @@ -602,71 +596,32 @@ function checkPageLayout(document, window) { // --- Nested cards --- const allEls = document.querySelectorAll('*'); - const flaggedNested = new Set(); + const flaggedEls = new WeakSet(); for (const el of allEls) { if (!isCardLike(el, window)) continue; + if (flaggedEls.has(el)) continue; const tag = el.tagName.toLowerCase(); const cls = el.getAttribute?.('class') || ''; const rawStyle = el.getAttribute?.('style') || ''; - // Exclude elements that look like non-card components if (['pre', 'code'].includes(tag)) continue; - // Exclude absolutely/fixed positioned elements (dropdowns, modals, tooltips) if (/\b(?:absolute|fixed)\b/.test(cls) || /position\s*:\s*(?:absolute|fixed)/i.test(rawStyle)) continue; - // Exclude small elements (badges, chips, icons) — text < 20 chars if ((el.textContent?.trim().length || 0) < 20) continue; - // Exclude form elements that happen to match card heuristics if (/\b(?:dropdown|popover|tooltip|menu|modal|dialog)\b/i.test(cls)) continue; // Walk up to find card-like ancestor let parent = el.parentElement; while (parent) { if (isCardLike(parent, window)) { - const key = `${parent.tagName}:${el.tagName}`; - if (!flaggedNested.has(key)) { - flaggedNested.add(key); - findings.push({ id: 'nested-cards', snippet: `Card inside card (${tag} in ${parent.tagName.toLowerCase()})` }); - } + flaggedEls.add(el); + findings.push({ id: 'nested-cards', snippet: `Card inside card (${tag} in ${parent.tagName.toLowerCase()})` }); break; } parent = parent.parentElement; } } - // --- Identical card grid --- - const gridParents = document.querySelectorAll('[class*="grid"], [style*="display: grid"], [style*="display: flex"]'); - for (const grid of gridParents) { - const children = [...grid.children].filter(c => { - const tag = c.tagName.toLowerCase(); - return tag !== 'script' && tag !== 'style'; - }); - if (children.length < 3) continue; - - // Compare structural fingerprint of each child - function fingerprint(el) { - const childTags = [...el.children].map(c => c.tagName.toLowerCase()); - // Check for icon-like element (svg, img, or div with fixed size classes) - const hasIcon = childTags.includes('svg') || childTags.includes('img') || - [...el.children].some(c => { - const cls = c.getAttribute?.('class') || ''; - return /\bw-\d+\b.*\bh-\d+\b/.test(cls) && /\brounded/.test(cls); - }); - const hasHeading = childTags.some(t => /^h[1-6]$/.test(t)); - const hasParagraph = childTags.includes('p'); - return `icon:${hasIcon}|h:${hasHeading}|p:${hasParagraph}|children:${childTags.length}`; - } - - const fps = children.map(fingerprint); - const allSame = fps.every(f => f === fps[0]); - // Only flag if structure includes icon + heading + paragraph (the template pattern) - if (allSame && fps[0].includes('icon:true') && fps[0].includes('h:true') && fps[0].includes('p:true')) { - findings.push({ - id: 'identical-card-grid', - snippet: `${children.length} identical cards (icon + heading + text)`, - }); - } - } // --- Monotonous spacing --- // Regex on raw HTML — jsdom doesn't compute inline px spacing reliably diff --git a/tests/detect-antipatterns.test.js b/tests/detect-antipatterns.test.js index 68b4f66c6..126666c71 100644 --- a/tests/detect-antipatterns.test.js +++ b/tests/detect-antipatterns.test.js @@ -282,14 +282,11 @@ describe('partials skip page-level checks', () => { // --------------------------------------------------------------------------- describe('detectHtml — layout', () => { - test('layout-should-flag: detects nested cards', async () => { + test('layout-should-flag: detects all nested cards', async () => { const f = await detectHtml(path.join(FIXTURES, 'layout-should-flag.html')); - expect(f.some(r => r.antipattern === 'nested-cards')).toBe(true); - }); - - test('layout-should-flag: detects identical card grid', async () => { - const f = await detectHtml(path.join(FIXTURES, 'layout-should-flag.html')); - expect(f.some(r => r.antipattern === 'identical-card-grid')).toBe(true); + const nested = f.filter(r => r.antipattern === 'nested-cards'); + // Classic, 3-level (2 inner cards), CSS, shadcn = at least 5 nested card findings + expect(nested.length).toBeGreaterThanOrEqual(4); }); test('detects monotonous spacing via regex', () => { @@ -320,11 +317,6 @@ describe('detectHtml — layout', () => { expect(f.filter(r => r.antipattern === 'nested-cards')).toHaveLength(0); }); - test('layout-should-pass: no identical-card-grid false positives', async () => { - const f = await detectHtml(path.join(FIXTURES, 'layout-should-pass.html')); - expect(f.filter(r => r.antipattern === 'identical-card-grid')).toHaveLength(0); - }); - test('layout-should-pass: no monotonous-spacing false positives', async () => { const f = await detectHtml(path.join(FIXTURES, 'layout-should-pass.html')); expect(f.filter(r => r.antipattern === 'monotonous-spacing')).toHaveLength(0);