Address review findings on the component ignore

Four gaps the review bots found in the first commit:

- **The hook dropped the waivers when the design system was off.** Both
  `design_system_options` paths returned `HookScanOptions::default()` when
  `designSystem.enabled` is false, which left `ignore_selectors` empty, so an
  opted-out component kept re-firing on every edit in exactly the projects
  that have no DESIGN.md. Component ignores are not design-system state; they
  travel either way now.
- **The in-page and extension scans never saw the key.** `BrowserConfig`
  reads `ignoreSelectors`, but the two JS adapters that build that config
  (`browser-bundle/50-scan.js` `collectConfigJson`, `60-offscreen.js`
  `configJson`) listed their keys explicitly and dropped it, so the documented
  `window.__IMPECCABLE_CONFIG__.ignoreSelectors` path did nothing. Both
  forward it now, and the bundle is regenerated.
- **Visual-contrast findings skipped the stamp.** The URL engine's visual pass
  produces its findings outside `collect_browser_findings`, so a
  `low-contrast` hit on an opted-out component was reported rather than
  waived. Each candidate carries its own selector, so the pass now resolves
  that element against the same post-reveal snapshot and stamps what the
  config waives. Page-level results (`content-hidden-at-rest`, `script-error`)
  stay unstamped: they name no element.
- **One bad entry could discard the whole page config.** `ignoreSelectors`
  used strict deserialization, so a hand-edited `{}` or `null` in the array
  failed the parse of `BrowserConfig`, which the wasm entry points answer with
  `unwrap_or_default()` — losing the design system and every other setting.
  It now filters bad entries the way `disabledValues` does.

Also: a `files` glob no longer applies to a URL scan. Globs name repo paths,
and `index.html` reaching `https://example.com/index.html` would scope an
ignore to a page the entry never named. URL scans take the unscoped entries
only, which is what the docs already promised.

Assisted-by: Claude Code
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQBUunp8QttxZqihybNmtL
This commit is contained in:
Paul Bakaus
2026-09-11 12:42:24 -07:00
co-authored by Claude Fable 5.1
parent 1d214f1e48
commit b50098a477
10 changed files with 159 additions and 33 deletions
+5
View File
@@ -73,6 +73,11 @@ if (IS_BROWSER && !__impeccable) {
// applies them where the findings are assembled, because the overlay
// draws its markers from the collected findings.
disabledValues: Array.isArray(config.disabledValues) ? config.disabledValues : [],
// detector.ignoreSelectors: the project's component-level opt-outs,
// [{ rule, selector }]. The core waives a finding on any element the
// selector matches, and on its subtree, the way the
// data-impeccable-ignore attribute waives the element carrying it.
ignoreSelectors: Array.isArray(config.ignoreSelectors) ? config.ignoreSelectors : [],
designSystem: config.designSystem == null ? null : config.designSystem,
lineLengthMax: config.lineLengthMax == null ? null : config.lineLengthMax,
skipScan: config.skipScan === true,
+5
View File
@@ -111,6 +111,11 @@
extensionMode: true,
disabledRules: Array.isArray(config.disabledRules) ? config.disabledRules : [],
disabledValues: Array.isArray(config.disabledValues) ? config.disabledValues : [],
// detector.ignoreSelectors: the project's component-level opt-outs,
// [{ rule, selector }]. The core waives a finding on any element the
// selector matches, and on its subtree, the way the
// data-impeccable-ignore attribute waives the element carrying it.
ignoreSelectors: Array.isArray(config.ignoreSelectors) ? config.ignoreSelectors : [],
designSystem: config.designSystem == null ? null : config.designSystem,
lineLengthMax: config.lineLengthMax == null ? null : config.lineLengthMax,
skipScan: config.skipScan === true,
+41 -9
View File
@@ -548,7 +548,24 @@ fn scan_page_inner(
snapshot_engine::analyze_visual_contrast(page, &base, 12.0, true)
})
.map_err(cdp_err)?;
let visual = run_visual_contrast_fallback(page, &analyses, &serialized_groups, viewport, profile, url)?;
// The visual pass produces findings outside `collect_browser_findings`,
// so the component-level opt-outs are applied here against the same
// post-reveal snapshot, keyed on each candidate's own selector.
let waive = |selector: &str, rule: &str| -> String {
use impeccable_core::browser::Dom as _;
if config.ignore_selectors.is_empty() || selector.is_empty() {
return String::new();
}
let Ok(Some(el)) = base.query_one(None, selector) else {
return String::new();
};
impeccable_core::selector_ignores::waiving_selector(&config.ignore_selectors, rule, |sel| {
matches!(base.closest(el, sel), Ok(Some(_)))
})
.unwrap_or_default()
.to_string()
};
let visual = run_visual_contrast_fallback(page, &analyses, &serialized_groups, viewport, profile, url, &waive)?;
results.extend(visual);
Ok(results)
}
@@ -579,6 +596,7 @@ fn reveal_sweep(page: &mut Page<'_>) -> Result<(), CdpError> {
/// target)`: the JS post-processing of the analytic/canvas analyses
/// (`analyzeVisualContrast`, computed natively in [`snapshot_engine`]) plus the
/// screenshot pixel fallback for candidates the analyses left unresolved.
#[allow(clippy::too_many_arguments)]
fn run_visual_contrast_fallback(
page: &mut Page<'_>,
browser_analyses: &[Value],
@@ -586,6 +604,9 @@ fn run_visual_contrast_fallback(
viewport: Viewport,
profile: Option<&DetectorProfile>,
target: &str,
// `(candidate selector, rule id) -> the detector.ignoreSelectors selector
// that waives it, or empty`.
waive: &dyn Fn(&str, &str) -> String,
) -> Result<Vec<RawResult>, EngineError> {
let existing_low_contrast: Vec<String> = serialized_groups
.iter()
@@ -611,13 +632,18 @@ fn run_visual_contrast_fallback(
.iter()
.any(|s| Some(s.as_str()) == r.get("selector").and_then(Value::as_str))
})
.filter_map(|r| r.get("finding"))
.map(|f| RawResult {
id: js_str(f.get("id")),
snippet: js_str(f.get("snippet")),
ignore_value: String::new(),
ignored_by: String::new(),
severity: String::new(),
.map(|r| {
let selector = r.get("selector").and_then(Value::as_str).unwrap_or("");
let f = r.get("finding").expect("filtered on a truthy finding");
let id = js_str(f.get("id"));
let ignored_by = waive(selector, &id);
RawResult {
id,
snippet: js_str(f.get("snippet")),
ignore_value: String::new(),
ignored_by,
severity: String::new(),
}
})
.collect();
@@ -649,6 +675,11 @@ fn run_visual_contrast_fallback(
})
.collect();
for candidate in filtered {
let candidate_selector = candidate
.get("selector")
.and_then(Value::as_str)
.unwrap_or("")
.to_string();
let result = step_findings(profile, "visual-contrast", "pixel-diff", target, || {
let f = screenshot_contrast::capture_visual_contrast_candidate(
page,
@@ -658,11 +689,12 @@ fn run_visual_contrast_fallback(
.map_err(cdp_err)?;
Ok::<_, EngineError>(
f.map(|f| {
let ignored_by = waive(&candidate_selector, f.id);
vec![RawResult {
id: f.id.to_string(),
snippet: f.snippet,
ignore_value: String::new(),
ignored_by: String::new(),
ignored_by,
severity: String::new(),
}]
})
+16 -4
View File
@@ -1989,12 +1989,24 @@ mod tests {
)
.unwrap();
assert_eq!(cfg.ignore_selectors.len(), 1);
// Normalization is the constructor's job, not the parser's: the raw
// value round-trips and `covers_rule` folds case.
assert!(SelectorIgnore::new(&cfg.ignore_selectors[0].rule, ".ks-tag")
.covers_rule("undersized-ui-text"));
// The parser normalizes, so a page config written by hand still
// matches: the rule folds case, the selector keeps it.
assert_eq!(cfg.ignore_selectors[0].rule, "undersized-ui-text");
assert_eq!(cfg.ignore_selectors[0].selector, ".ks-tag");
let bare: BrowserConfig = serde_json::from_str("{}").unwrap();
assert!(bare.ignore_selectors.is_empty());
// A hand-edited entry of the wrong shape drops itself, never the whole
// config: `unwrap_or_default()` at the wasm boundary would otherwise
// lose the design system with it.
let junk: BrowserConfig = serde_json::from_str(
r#"{"lineLengthMax":90,"ignoreSelectors":[{},null,"nope",{"rule":"side-tab"},{"rule":"side-tab","selector":".x"}]}"#,
)
.unwrap();
assert_eq!(junk.ignore_selectors.len(), 1);
assert_eq!(junk.line_max(), 90.0);
let not_a_list: BrowserConfig =
serde_json::from_str(r#"{"ignoreSelectors":"nope"}"#).unwrap();
assert!(not_a_list.ignore_selectors.is_empty());
// A config without the key serializes without it.
assert!(!serde_json::to_string(&bare).unwrap().contains("ignoreSelectors"));
}
+7 -6
View File
@@ -10,7 +10,7 @@ use serde_json::Value;
use crate::config::{
filter_detection_findings_reported, read_detection_config, selector_ignores_for_target,
should_ignore_detection_file, DetectionConfig, IgnoredBySelector,
selector_ignores_for_url, should_ignore_detection_file, DetectionConfig, IgnoredBySelector,
};
use crate::design_system::{load_design_system_for_target, DesignSystemCache};
use crate::detect_text::{detect_text, TextOptions};
@@ -297,11 +297,12 @@ impl<'a> Ctx<'a> {
options
}
/// The URL scan's options: no local design system to resolve, but the
/// project's unscoped selector ignores still govern the page.
fn url_scan_options(&self, url: &str) -> ScanOptions {
/// The URL scan's options: no local design system to resolve, and only
/// the unscoped component ignores, since a `files` glob names repo paths
/// rather than URLs.
fn url_scan_options(&self) -> ScanOptions {
ScanOptions {
ignore_selectors: selector_ignores_for_target(&self.config, url),
ignore_selectors: selector_ignores_for_url(&self.config),
..self.base.clone()
}
}
@@ -752,7 +753,7 @@ fn scan_targets(
let local = file_url_to_local_path(target);
ctx.scan_options_for(local.as_deref())
} else {
ctx.url_scan_options(target)
ctx.url_scan_options()
};
let result = match (shared, ctx.engines.url) {
(Some(s), _) => s.detect_url(target, &url_options),
+32 -7
View File
@@ -811,20 +811,36 @@ pub fn merge_ignore_selectors(
map.into_iter().map(|(_, e)| e).collect()
}
/// The entries that govern one scan target, as the engines take them.
/// The entries that govern one local scan target, as the engines take them.
///
/// An entry with no `files` covers every target. An entry with `files` covers
/// the ones its globs match, tested the way a scoped `ignoreValues` entry is
/// (raw path, then each `/`-suffix of it), so a URL target is covered only by
/// the unscoped entries.
/// the paths its globs match, tested the way a scoped `ignoreValues` entry is
/// (raw path, then each `/`-suffix of it).
pub fn selector_ignores_for_target(
config: &DetectionConfig,
target: &str,
) -> Vec<SelectorIgnore> {
selector_ignores_filtered(config, |files| path_matches_scoped_globs(target, files))
}
/// The entries that govern a URL scan: the unscoped ones only.
///
/// `files` globs describe repo paths, and a URL is not one. Matching them
/// against the URL would let a glob like `index.html` reach
/// `https://example.com/index.html` by accident, scoping an ignore to a page
/// the entry never named.
pub fn selector_ignores_for_url(config: &DetectionConfig) -> Vec<SelectorIgnore> {
selector_ignores_filtered(config, |_| false)
}
fn selector_ignores_filtered(
config: &DetectionConfig,
covers: impl Fn(&[String]) -> bool,
) -> Vec<SelectorIgnore> {
normalize_ignore_selector_entries_typed(&config.ignore_selectors)
.into_iter()
.filter(|e| match &e.files {
Some(files) if !files.is_empty() => path_matches_scoped_globs(target, files),
Some(files) if !files.is_empty() => covers(files),
_ => true,
})
.map(|e| SelectorIgnore::new(&e.rule, &e.selector))
@@ -1414,9 +1430,18 @@ mod tests {
assert_eq!(everywhere[0].selector, ".ks-tag");
let scoped = selector_ignores_for_target(&config, "src/demo/playground.astro");
assert_eq!(scoped.len(), 2);
// A URL target is covered by the unscoped entries only.
// A URL scan is covered by the unscoped entries only: a `files` glob
// describes repo paths, and must not reach a URL path that happens to
// end the same way.
assert_eq!(selector_ignores_for_url(&config).len(), 1);
let url_globs = config_with_selectors(
r#"{"ignoreSelectors":[
{"rule":"glow-effect","selector":".demo","files":["index.html"]}
]}"#,
);
assert!(selector_ignores_for_url(&url_globs).is_empty());
assert_eq!(
selector_ignores_for_target(&config, "https://example.com/").len(),
selector_ignores_for_target(&url_globs, "src/index.html").len(),
1
);
// `--no-config` leaves the list empty, so nothing is waived.
+34 -1
View File
@@ -139,6 +139,35 @@ where
.collect())
}
/// The same tolerance for `ignoreSelectors`: an entry that is not an object
/// with both halves is dropped, rather than failing the parse of the whole
/// config (which the wasm entry points answer with `unwrap_or_default()`,
/// silently losing the design system and every other setting with it).
fn de_ignore_selectors<'de, D>(
de: D,
) -> Result<Vec<crate::selector_ignores::SelectorIgnore>, D::Error>
where
D: serde::Deserializer<'de>,
{
let raw = serde_json::Value::deserialize(de)?;
let Some(items) = raw.as_array() else {
return Ok(Vec::new());
};
Ok(items
.iter()
.filter_map(|entry| {
let obj = entry.as_object()?;
let text = |key: &str| match obj.get(key) {
Some(serde_json::Value::String(s)) => s.clone(),
_ => String::new(),
};
let parsed =
crate::selector_ignores::SelectorIgnore::new(text("rule"), text("selector"));
parsed.is_valid().then_some(parsed)
})
.collect())
}
/// What the bundle passes into `collectBrowserFindings`: extension mode and
/// the relevant slice of `window.__IMPECCABLE_CONFIG__`.
#[derive(Debug, Clone, Default, Deserialize, Serialize)]
@@ -165,7 +194,11 @@ pub struct BrowserConfig {
/// in every mode: unlike `disabledRules`, this list is the project's own
/// config rather than a browser-extension preference. Empty by default,
/// and skipped in serialization so a config without it is byte-identical.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
#[serde(
default,
deserialize_with = "de_ignore_selectors",
skip_serializing_if = "Vec::is_empty"
)]
pub ignore_selectors: Vec<crate::selector_ignores::SelectorIgnore>,
/// `window.__IMPECCABLE_CONFIG__?.skipScan === true` (only honored in
/// extension mode): the page is waived wholesale by detector.ignoreFiles,
+11 -3
View File
@@ -1655,12 +1655,19 @@ impl HookScanOptions {
/// JS: designSystemOptions(config, detector, projectCwd)
pub fn design_system_options(config: &HookConfig, project_cwd: &str) -> HookScanOptions {
// Component ignores are not design-system state: a project with
// `designSystem.enabled: false` still opted its components out, and the
// hook would otherwise re-report them on every edit.
let ignore_selectors = config.ignore_selectors.clone();
if !config.design_system_enabled {
return HookScanOptions::default();
return HookScanOptions {
design_system: None,
ignore_selectors,
};
}
HookScanOptions {
design_system: load_design_system_for_cwd(project_cwd).map(Rc::new),
ignore_selectors: config.ignore_selectors.clone(),
ignore_selectors,
}
}
@@ -1672,7 +1679,8 @@ pub fn design_system_options_for_file(
file_path: &str,
) -> HookScanOptions {
if !config.design_system_enabled {
return HookScanOptions::default();
// Same as above: the waivers travel even when no design system does.
return design_system_options(config, project_cwd);
}
let project = impeccable_context::context::resolve_project(
project_cwd,
File diff suppressed because one or more lines are too long
+1 -1
View File
@@ -287,7 +287,7 @@ Optional keys added later by engines (appended after the above): `ignoreValue` (
- `readDetectionConfig(root)`: start `{ignoreRules:[],ignoreFiles:[],ignoreValues:[],designSystem:{enabled:true}}`; for shared then local: apply legacy `raw.hook.*` section then `raw.detector.*`. Arrays are unioned (`uniqueStrings`, String-coerced); ignoreValues merged by key `rule\0value\0sortedFiles.join('\x1f')` (later wins); `designSystem.enabled` false only when literally `false`; `advisoryRules` copied only if `'include'|'exclude'`. Invalid JSON / non-object files are ignored silently. **No validation errors are ever raised by the CLI**; the only validation of ignore lists lives in `skill/scripts/lib/staleness-deep.mjs checkDetectorIgnores` (doctor): unknown `ignoreRules` ids vs live `ANTIPATTERNS` → finding `detector-ignore-rules-unknown` (severity `mention`); non-glob `ignoreFiles` entries that don't exist → `detector-ignore-files-missing`.
- `normalizeIgnoreValue(v)`: trim, strip one leading/trailing quote, `+`→space, collapse whitespace, lowercase. Rules lowercased/trimmed.
- `normalizeIgnoreValueEntries`: keeps `{rule, value, [files], [createdAt], [reason]}` in **that key order**; `file` (string) and `files` merged, trimmed, deduped.
- `ignoreSelectors` (component ignores): merged by key `rule\0selector\0sortedFiles.join('\x1f')` (later wins), normalized to `{rule, selector, [files], [createdAt], [reason]}` in **that key order**. The rule is lowercased/trimmed like every rule id; the **selector keeps its case** (CSS class names are case-sensitive) and only loses surrounding whitespace. An entry missing either half is dropped. `selectorIgnoresForTarget(config, target)` narrows the list per scan target: an entry without `files` covers every target, one with `files` covers the targets its globs match (raw path then each `/`-suffix, so a URL target gets the unscoped entries only). `--no-config` leaves the list empty. The key is **written only by a project that uses it**, so an existing config does not grow an empty `ignoreSelectors` on the next `ignores add-rule`. `doctor`'s `detector-ignore-rules-unknown` validates the `rule` of each entry alongside `ignoreRules`.
- `ignoreSelectors` (component ignores): merged by key `rule\0selector\0sortedFiles.join('\x1f')` (later wins), normalized to `{rule, selector, [files], [createdAt], [reason]}` in **that key order**. The rule is lowercased/trimmed like every rule id; the **selector keeps its case** (CSS class names are case-sensitive) and only loses surrounding whitespace. An entry missing either half is dropped. `selectorIgnoresForTarget(config, target)` narrows the list per local scan target: an entry without `files` covers every target, one with `files` covers the paths its globs match (raw path then each `/`-suffix). A URL scan uses `selectorIgnoresForUrl(config)`, the unscoped entries only: a `files` glob names repo paths, so it must not reach a URL that happens to end the same way. `--no-config` leaves the list empty. The key is **written only by a project that uses it**, so an existing config does not grow an empty `ignoreSelectors` on the next `ignores add-rule`. `doctor`'s `detector-ignore-rules-unknown` validates the `rule` of each entry alongside `ignoreRules`.
- Glob → regex: `**``.*` (swallowing a following `/`), `*``[^/]*`, `?``[^/]`, `{a,b}``(?:a|b)`, regex specials escaped; anchored `^...$`. `matchesAnyGlob` tests the `/`-normalized path and its basename.
- `shouldIgnoreDetectionFile(filePath, root, config)`: raw path, absolute path, and root-relative path (if inside root) tested against `ignoreFiles`.
- `filterDetectionFindings`: first drop every finding the engines stamped with `ignoredBy` (the component ignores above), counted by `{rule, selector, count}` in first-seen order; `filterDetectionFindingsReported` returns that tally beside the kept findings. Then drop when `ignoreRules` has the rule, or an `ignoreValues` entry matches: same rule; entry.value `*` (wildcard) OR extracted value equals (with color-key equality for `design-system-color`: rgb/hex/hsl parsed to `r,g,b,round(a*255)`); if entry has `files`, `finding.file` (or any `/`-suffix of it) must glob-match; a wildcard with no files never matches (unscoped `*` disallowed).