Skip to content

fix(matcher): coalesce mutation observer snapshots per callback batch - #992

Open
mdesrosiers wants to merge 1 commit into
salesforce:masterfrom
mdesrosiers:fix/matcher-mutation-observer-duplicate-snapshots
Open

mdesrosiers wants to merge 1 commit into
salesforce:masterfrom
mdesrosiers:fix/matcher-mutation-observer-duplicate-snapshots

Conversation

@mdesrosiers

@mdesrosiers mdesrosiers commented Jul 24, 2026

Copy link
Copy Markdown

Summary

mutationObserverCallback in @sa11y/matcher's automatic.ts pushes one DOM snapshot onto mutatedNodes for every addedNodes entry across all MutationRecords in a single callback invocation:

for (const mutation of mutations) {
    mutation.addedNodes.forEach((node) => {
        if (node?.parentElement?.innerHTML) {
            mutatedNodes.push(node.parentElement.innerHTML);
        } else if ((node as Element)?.outerHTML) {
            mutatedNodes.push((node as Element).outerHTML);
        }
    });
}

A MutationObserver already batches every synchronous DOM change into a single callback call — document.body reflects the exact same settled state for every record in that batch. Looping over addedNodes therefore queues many duplicate/ancestor-redundant snapshots for what is really one DOM settle event (e.g. a single framework re-render that inserts several nested nodes).

Each queued snapshot is later replayed through axe.run() in runAutomaticCheck's for await (const mutated of mutatedNodes) loop, so this duplication multiplies accessibility-check cost. In one of our own test suites (runDOMMutationObserver: true enabled), the slowest test file went from 75 axe.run() calls down to 4 after this fix, for the same set of childList-addition-driven re-renders.

Fix

Capture document.body.innerHTML once per callback invocation instead of once per added node:

export function mutationObserverCallback(mutations: MutationRecord[]) {
    if (mutations.length > 0) mutatedNodes.push(document.body.innerHTML);
}

Behavior change beyond deduping — please review carefully

This fix does more than dedupe. Two side effects are worth calling out explicitly, since they affect what gets checked, not just how many times:

  1. Coverage now also includes attribute/characterData/pure-removal mutations. observerOptions (subtree, childList, attributes, characterData) has always configured the observer to watch attribute and text mutations, but the old callback only ever queued a snapshot when a record's addedNodes was non-empty — so attribute-only changes (e.g. aria-hidden, disabled, class toggles), characterData edits, and pure node removals never triggered a replay check at all. I confirmed this with real MutationRecords captured from an actual observer for each of those three cases: addedNodes.length === 0 in all of them, and I verified the old callback made 0 pushes for such records. The new callback pushes once per callback invocation regardless of mutation content, so these previously-unchecked mutation types are now checked. This is arguably fixing a second, latent bug (the observer was configured to watch things the callback then ignored), but it does mean consumers relying on runDOMMutationObserver: true may see new axe.run() invocations — and potentially newly-surfaced a11y violations — for DOM changes that were silently skipped before. It is not simply "fewer duplicate checks of the same coverage."
  2. Snapshot content changed from a subtree to the full body. The old code captured node.parentElement.innerHTML (or node.outerHTML) — a snippet scoped to the mutated node's ancestor chain — and replayed it by assigning it wholesale to document.body.innerHTML. That means the old replay was already checking a partial/lossy view of the DOM (discarding content outside that subtree). The new code captures document.body.innerHTML in full, so each replay now checks the entire settled body. This is likely more correct, but it's a second behavior change bundled with the dedup fix.

I don't believe either of these invalidates the fix — if anything, both make the check more correct and more consistent with observerOptions's stated intent — but I want to flag them explicitly rather than characterize this as a pure performance/dedup change with "no change in coverage," since a consuming test suite could see new (valid) failures after upgrading.

Test plan

  • Added two unit tests directly exercising mutationObserverCallback:
    • A batch of 5 MutationRecords (all reporting the same added node, mimicking one childList-based re-render) now queues exactly 1 snapshot per callback call instead of 5.
    • An empty batch queues no snapshot.
  • Verified the new coalescing test fails against the pre-fix code (11 axe.run() calls observed vs. the expected 3), confirming it actually pins the childList-duplication bug rather than trivially passing.
  • Manually verified (outside the committed test suite) that real attribute-only/characterData-only/removal-only MutationRecords produce 0 replay pushes under the old callback and 1 push per batch under the new callback, to characterize the coverage-expansion side effect described above.
  • yarn lint — clean (pre-existing warnings only, unrelated to this change).
  • yarn test — all 16 suites / 217 tests pass (2 pre-existing skips), no regressions.

mutationObserverCallback previously pushed one document snapshot per
addedNode across all MutationRecords in a callback invocation. Since
a MutationObserver already batches every synchronous DOM change into a
single callback call, document.body reflects the same settled state
for every record in that batch. Looping over addedNodes therefore
queued many duplicate or ancestor-redundant snapshots for what was
really one DOM settle event.

Each queued snapshot is replayed through axe.run() in
runAutomaticCheck, so the duplication multiplies accessibility check
cost for no additional coverage: the same final/settled DOM states
still get checked, just once per batch instead of once per added
node.

Capture document.body.innerHTML once per callback invocation instead.
@mdesrosiers
mdesrosiers requested a review from a team as a code owner July 24, 2026 18:14
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.21%. Comparing base (94bc67b) to head (71c3517).
⚠️ Report is 233 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
+ Coverage   94.88%   95.21%   +0.32%     
==========================================
  Files          27       32       +5     
  Lines         626      690      +64     
  Branches      137      136       -1     
==========================================
+ Hits          594      657      +63     
- Misses         32       33       +1     
Files with missing lines Coverage Δ
packages/matcher/src/automatic.ts 98.63% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant