From a4a076005b896e0f7966e339c85ddf65be269615 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Sat, 25 Jul 2026 18:24:02 -0700 Subject: [PATCH] Carry source lines on pseudo-stripe findings and skip commented-out rules Review bots caught two real gaps in the pseudo-stripe wiring: findings had no source line (so line-scoped impeccable-disable directives could not match them), and the scanner read commented-out CSS as live rules. scanCssTextForPseudoStripe now blanks comment bodies byte-for-byte (preserving offsets) and returns each rule's selector offset; the three regex-engine call sites convert that to a real line, including the whole-file line for component style blocks and CSS-in-JS templates. The HTML path ignores the new field. Tests now assert every finding's line against the selector's actual position and cover a commented-out stripe. Prepared with AI assistance (Claude Code), directed by @pbakaus. Co-Authored-By: Claude Code --- cli/engine/detect-antipatterns-browser.js | 12 ++++++++- cli/engine/engines/regex/detect-text.mjs | 22 +++++++++------ cli/engine/rules/checks.mjs | 12 ++++++++- tests/detect-antipatterns-fixtures.test.mjs | 27 ++++++++++++++++++- tests/fixtures/antipatterns/pseudo-stripe.css | 10 +++++++ 5 files changed, 72 insertions(+), 11 deletions(-) diff --git a/cli/engine/detect-antipatterns-browser.js b/cli/engine/detect-antipatterns-browser.js index d07caa7ab..a728d4f9e 100644 --- a/cli/engine/detect-antipatterns-browser.js +++ b/cli/engine/detect-antipatterns-browser.js @@ -1625,7 +1625,13 @@ function isZeroOffset(value) { // never see it — pseudo-elements aren't part of the DOM the cascade walks — // so this scans stylesheet text directly, mirroring the border rule's // gates: >= 3px thick, chromatic fill, full height against a side edge. -function scanCssTextForPseudoStripe(content) { +function scanCssTextForPseudoStripe(rawContent) { + // Blank comment bodies byte-for-byte so commented-out rules are not + // scanned as live CSS and every rule keeps its source offset (each + // finding carries `index` so line-based callers can attribute it and + // line-scoped inline ignores can match). + const content = String(rawContent || '').replace(/\/\*[\s\S]*?\*\//g, + (block) => block.replace(/[^\n]/g, ' ')); const customProps = collectCssCustomProps(content); const findings = []; const seen = new Set(); @@ -1734,9 +1740,13 @@ function scanCssTextForPseudoStripe(content) { if (seen.has(selector)) continue; seen.add(selector); + // The selector group absorbs whitespace trailing the previous rule; + // advance past it so `index` points at the selector itself. + const selectorStart = m.index + (m[1].length - m[1].trimStart().length); findings.push({ id: 'side-tab', snippet: `${selector} — absolute ${thicknessPx}px pseudo-element stripe (${edge}: 0)`, + index: selectorStart, }); } return findings; diff --git a/cli/engine/engines/regex/detect-text.mjs b/cli/engine/engines/regex/detect-text.mjs index 8c5db8fbe..c664f70b6 100644 --- a/cli/engine/engines/regex/detect-text.mjs +++ b/cli/engine/engines/regex/detect-text.mjs @@ -653,14 +653,20 @@ function detectText(content, filePath, options = {}) { profile, phase: 'source', })); + // Pseudo-element stripes (::before/::after absolute bars) carry the same + // side-tab silhouette without any border token, so the line matchers can't + // see them (issue #394). The shared scanner already runs on full HTML pages + // via checkHtmlPatterns; give standalone stylesheets, component style + // blocks, and CSS-in-JS templates the same coverage. Each hit carries the + // rule's source offset, so the finding gets a real line and line-scoped + // inline ignores keep working. + const pseudoStripeFindings = (text, lineOffset) => + scanCssTextForPseudoStripe(text).map(hit => + finding(hit.id, filePath, hit.snippet, lineOffset + text.slice(0, hit.index).split('\n').length)); + if (cssLike.has(ext)) { findings.push(...scanInsetStripeCss(content, filePath)); - // Pseudo-element stripes (::before/::after absolute bars) carry the same - // side-tab silhouette without any border token, so the line matchers - // can't see them (issue #394). The shared scanner already runs on full - // HTML pages via checkHtmlPatterns; give standalone stylesheets the - // same coverage. - findings.push(...scanCssTextForPseudoStripe(content).map(hit => finding(hit.id, filePath, hit.snippet))); + findings.push(...pseudoStripeFindings(content, 0)); } // Block-level CSS checks that need multiple declarations must run over the @@ -698,7 +704,7 @@ function detectText(content, filePath, options = {}) { // reported every selector one line low. runRegexMatchers keeps startLine - 1 // because it indexes its split lines from zero. findings.push(...scanInsetStripeCss(block.content, filePath, block.startLine - 2)); - findings.push(...scanCssTextForPseudoStripe(block.content).map(hit => finding(hit.id, filePath, hit.snippet))); + findings.push(...pseudoStripeFindings(block.content, block.startLine - 2)); } // Extract and scan CSS-in-JS template literals @@ -717,7 +723,7 @@ function detectText(content, filePath, options = {}) { phase: 'css-in-js', })); findings.push(...scanInsetStripeCss(block.content, filePath, block.startLine - 1)); - findings.push(...scanCssTextForPseudoStripe(block.content).map(hit => finding(hit.id, filePath, hit.snippet))); + findings.push(...pseudoStripeFindings(block.content, block.startLine - 1)); } if (options?.designSystem) { diff --git a/cli/engine/rules/checks.mjs b/cli/engine/rules/checks.mjs index 418aa8f45..8431c1448 100644 --- a/cli/engine/rules/checks.mjs +++ b/cli/engine/rules/checks.mjs @@ -823,7 +823,13 @@ function isZeroOffset(value) { // never see it — pseudo-elements aren't part of the DOM the cascade walks — // so this scans stylesheet text directly, mirroring the border rule's // gates: >= 3px thick, chromatic fill, full height against a side edge. -function scanCssTextForPseudoStripe(content) { +function scanCssTextForPseudoStripe(rawContent) { + // Blank comment bodies byte-for-byte so commented-out rules are not + // scanned as live CSS and every rule keeps its source offset (each + // finding carries `index` so line-based callers can attribute it and + // line-scoped inline ignores can match). + const content = String(rawContent || '').replace(/\/\*[\s\S]*?\*\//g, + (block) => block.replace(/[^\n]/g, ' ')); const customProps = collectCssCustomProps(content); const findings = []; const seen = new Set(); @@ -932,9 +938,13 @@ function scanCssTextForPseudoStripe(content) { if (seen.has(selector)) continue; seen.add(selector); + // The selector group absorbs whitespace trailing the previous rule; + // advance past it so `index` points at the selector itself. + const selectorStart = m.index + (m[1].length - m[1].trimStart().length); findings.push({ id: 'side-tab', snippet: `${selector} — absolute ${thicknessPx}px pseudo-element stripe (${edge}: 0)`, + index: selectorStart, }); } return findings; diff --git a/tests/detect-antipatterns-fixtures.test.mjs b/tests/detect-antipatterns-fixtures.test.mjs index 66b72f3cd..7024ea8fc 100644 --- a/tests/detect-antipatterns-fixtures.test.mjs +++ b/tests/detect-antipatterns-fixtures.test.mjs @@ -104,8 +104,18 @@ describe('detectText — pseudo-element stripe fixtures (issue #394)', () => { 'Hairline Divider', 'Hover Underline', 'Floating Badge', + // Commented-out CSS is not a live rule. + 'Commented Out Stripe', ]; + // The 1-based line a case's selector sits on in a fixture file, so the + // reported finding line can be checked against the actual source. + const selectorLine = (source, caseName) => { + const idx = source.split('\n').findIndex(l => l.includes(`data-case="${caseName}"`)); + assert.notEqual(idx, -1, `fixture is missing case "${caseName}"`); + return idx + 1; + }; + it('standalone .css files flag chromatic pseudo-element stripes only', () => { const filePath = path.join(FIXTURES, 'pseudo-stripe.css'); const source = fs.readFileSync(filePath, 'utf8'); @@ -117,15 +127,30 @@ describe('detectText — pseudo-element stripe fixtures (issue #394)', () => { for (const heading of SHOULD_PASS) { assert.doesNotMatch(snippets, new RegExp(`data-case=${JSON.stringify(heading)}`), `"${heading}" should pass`); } + // Every finding must carry the selector's real source line, so + // line-scoped inline ignores (impeccable-disable-line and + // impeccable-disable-next-line) can match it. + for (const f of findings) { + const caseName = (f.snippet.match(/data-case="([^"]+)"/) || [])[1]; + assert.equal( + f.line, selectorLine(source, caseName), + `finding for "${caseName}" reports line ${f.line}, selector sits on line ${selectorLine(source, caseName)}`, + ); + } }); - it('component style blocks flag pseudo-element stripes', () => { + it('component style blocks flag pseudo-element stripes at their source line', () => { const filePath = path.join(FIXTURES, 'pseudo-stripe.vue'); const source = fs.readFileSync(filePath, 'utf8'); const findings = detectText(source, filePath).filter(r => r.antipattern === 'side-tab'); const snippets = findings.map(r => r.snippet || '').join(' | '); assert.match(snippets, /data-case="Component Left Edge"/, 'expected the component stripe to flag'); assert.doesNotMatch(snippets, /data-case="Component Neutral Divider"/, 'neutral divider should pass'); + const stripe = findings.find(r => /data-case="Component Left Edge"/.test(r.snippet || '')); + assert.equal( + stripe.line, selectorLine(source, 'Component Left Edge'), + 'style-block finding must map back to the whole-file line, not the block-local one', + ); }); }); diff --git a/tests/fixtures/antipatterns/pseudo-stripe.css b/tests/fixtures/antipatterns/pseudo-stripe.css index ccdd4470a..91286b50d 100644 --- a/tests/fixtures/antipatterns/pseudo-stripe.css +++ b/tests/fixtures/antipatterns/pseudo-stripe.css @@ -108,3 +108,13 @@ height: 12px; background: #7c3aed; } + +/* ── PASS: commented-out CSS is not a live rule ── +.card[data-case="Commented Out Stripe"]::before { + content: ""; + position: absolute; + inset: 0 auto 0 0; + width: 4px; + background: #7c3aed; +} +*/