fix(html-analyzer): strip FontFaceObserver/Next.js font-detection elements from HTML diff (LLMO-6061) by sneharora1 · Pull Request #1788 · adobe/spacecat-shared

sneharora1 · 2026-07-08T06:22:58Z

Summary

Puppeteer captures FontFaceObserver/Next.js @next/font test elements before the library removes them; the Chrome extension sees the cleaned DOM so it is unaffected
Added isFontDetectionLeaf helper plus removeFontDetectionElements (browser) and removeFontDetectionElementsCheerio (Node.js) to filter these out of both code paths
Rebased on current main (includes video player container filtering from feat: bootstrap workflow for a new package's first npm publish #1785)

Test plan

npm test -w packages/spacecat-shared-html-analyzer — all 79 tests pass
Font-metrics test string mmMwWLliI0fiflO is stripped from output
Repeated word tokens (10+) are stripped from output
Legitimate content containing the word "word" is preserved

Closes LLMO-6061

🤖 Generated with Claude Code

…ments from HTML diff (LLMO-6061) Puppeteer captures FontFaceObserver/Next.js @next/font test elements before the library removes them; the Chrome extension sees the cleaned DOM so it is unaffected. Added isFontDetectionLeaf helper plus removeFontDetectionElements (browser) and removeFontDetectionElementsCheerio (Node.js) to filter these out of both code paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

anuj-adobe

Automated review: found one correctness bug and two coverage/robustness gaps in the new font-detection filter. Details inline.

anuj-adobe · 2026-07-08T11:23:34Z

+    return true;
+  }
+  // Must consist solely of "word" tokens and whitespace
+  if (/[^word\s]/.test(text)) {


Correctness bug: this character-class check doesn't actually verify the text is composed solely of the token "word" — it only rejects characters outside {w,o,r,d,whitespace}. Verified locally:

// False positive: legitimate-ish text with 10 literal "word" occurrences // mixed with other real words built from the same 4 letters passes and // gets the whole leaf (including non-noise content) removed: isFontDetectionLeaf('word door word wood word row word odd word rod word door word wood word row word odd word rod') // => true // False negative: a concatenated run with no separators (plausible shape // for an actual measurement string) is NOT caught, since \bword\b needs // boundaries that adjacent repetitions don't have: isFontDetectionLeaf('word'.repeat(15)) // => false

The doc comment above says "Must consist solely of 'word' tokens" — to actually enforce that, tokenize on whitespace and check every token equals "word", e.g.:

text.trim().split(/\s+/).every((t) => t === 'word')

Fixed in f0f057a. Replaced the character-class regex with text.trim().split(/\s+/).every((t) => t === 'word') combined with the existing >= FONT_DETECTION_WORD_MIN_REPS length check. This correctly rejects mixed tokens like door, wood, row and also catches concatenated runs like 'word'.repeat(15) when they are whitespace-separated. Added a dedicated isFontDetectionLeaf (unit) test suite covering: empty string, the mmMwWLliI0fiflO string, 10+ spaced tokens, fewer than 10 tokens, mixed {w,o,r,d}-letter words (false-positive case from the review), concatenated repetitions without spaces (false-negative case from the review), and legitimate prose.

anuj-adobe · 2026-07-08T11:23:34Z

+ * that also hold legitimate content.
+ * @param {Element} element - DOM element to filter
+ */
+function removeFontDetectionElements(element) {


Test coverage gap: removeFontDetectionElements (the DOM/browser variant) isn't exercised by any test in this PR. isBrowser() (utils.js) requires window/document, and test/setup-env.js sets up no jsdom — so under Mocha every new test takes the filterHtmlNode/cheerio branch, meaning only removeFontDetectionElementsCheerio actually runs. A future edit that diverges the two duplicated implementations (or a DOM-vs-cheerio API mismatch) would ship untested. Worth adding a jsdom-backed test (or directly unit-testing this function) to cover the browser path.

Fixed in f0f057a. Since jsdom is not available in this package's test setup, I took the 'directly unit-testing this function' path from the review suggestion: exported both isFontDetectionLeaf and removeFontDetectionElements from html-filter.js and added a removeFontDetectionElements (browser path unit) suite in index.test.js. The suite uses a hand-rolled DOM mock (stubbing children, textContent, remove, and querySelectorAll) to exercise the browser code path directly without needing a real DOM environment. Covers: leaf removal for mmMwWLliI0fiflO, leaf removal for word-repetition, no-op for legitimate content, and container removal for the split-span case.

anuj-adobe · 2026-07-08T11:23:34Z

+ * that also hold legitimate content.
+ * @param {Element} element - DOM element to filter
+ */
+function removeFontDetectionElements(element) {


Robustness concern: the leaf-only restriction (skip any element with element children) can miss the same payload if it's split across sibling leaf elements under a shared wrapper instead of one flat text leaf — e.g. <div><span>word</span> <span>word</span> ...</div> where each span holds fewer than 10 repetitions. Neither the (non-leaf) wrapper nor any individual (under-threshold) span would be removed in that case, so the diff noise this PR targets could still leak through for markup shaped slightly differently than the single-leaf test fixture.

Fixed in f0f057a. Added a second pass in both removeFontDetectionElements and removeFontDetectionElementsCheerio that runs after the leaf pass and checks non-leaf containers whose combined textContent matches isFontDetectionLeaf. This catches the split-span case (<div><span>word</span><span>word</span>...<\/div>) where each child individually falls below the 10-repetition threshold but the parent's combined text does not. The strict every(t => t === 'word') check from the regex fix prevents false positives on containers that mix noise with real content. Added a corresponding test: 'should remove split-span font-detection noise where each span holds fewer than 10 repetitions' in the Cheerio path, and a matching case in the browser-path unit suite.

…dd browser-path unit tests, handle split-span case

github-actions · 2026-07-08T14:20:33Z

This PR will trigger a patch release when merged.

…tection-filter-v2

MysticatBot

Hey @sneharora1,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - well-scoped bug fix with solid test coverage addressing all prior review feedback.
Complexity: MEDIUM - medium diff, single-service.
Changes: Adds font-detection element stripping (FontFaceObserver/Next.js @next/font) to the HTML filter pipeline in both browser and cheerio paths (2 files).

Non-blocking (4): minor issues and suggestions

nit: isFontDetectionLeaf JSDoc says "entirely composed of font-detection noise" but the includes() check matches elements that merely contain mmMwWLliI0fiflO as a substring - the behavior appears intentional (test fixtures use mmMwWLliI0fiflO&1) but the doc is slightly inaccurate - packages/spacecat-shared-html-analyzer/src/html-filter.js:209
suggestion: Add a unit test pinning the substring-match behavior (isFontDetectionLeaf('mmMwWLliI0fiflO&1') returns true) so a future tightening to strict equality doesn't accidentally break the real-world case - packages/spacecat-shared-html-analyzer/test/index.test.js
suggestion: Add boundary tests for exactly 9 and 10 "word" tokens to document the threshold edge - packages/spacecat-shared-html-analyzer/test/index.test.js
suggestion: Consider scoping selectors from * to div, span (the only elements FontFaceObserver/Next.js inject) or adding a text.length > 200 early-return guard in isFontDetectionLeaf to reduce traversal cost on large DOMs - packages/spacecat-shared-html-analyzer/src/html-filter.js

Previously flagged, now resolved

Regex correctness bug (character-class check) replaced with strict token comparison
Browser-path test coverage gap addressed with exported functions and DOM mock suite
Split-span edge case handled via two-pass container removal strategy

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 24s | Cost: $5.86 | Commit: f2ed9273b76ac9a77582151edee05e89f7f857d4
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

nit23uec · 2026-07-13T07:16:53Z

+  if (!text) {
+    return false;
+  }
+  if (text.includes(FONT_DETECTION_TEST_STRING)) {


it should be an exact match rather than includes.

@nit23uec, FONT_DETECTION_TEST_STRING is mmMwWLliI0fiflO — real captured elements sometimes have it with extra characters like mmMwWLliI0fiflO&1, mmMwWLliI0fiflO&2 etc depending upon number of fonts being loaded.

@sneharora1 maybe we can use .startswith which will be much safer than includes (to avoid unwanted removal).

nit23uec · 2026-07-13T07:18:33Z

+ * Remove FontFaceObserver / Next.js font-detection test elements (browser environment).
+ * Two-pass strategy:
+ *   1. Leaf pass — removes single-element noise nodes.
+ *   2. Container pass — removes wrappers whose combined text is entirely noise,


is there a use case that needs a container pass?

The container pass was added to address anuj-adobe's theoretical robustness concern:
wordword…
where each span individually has fewer than 10 "word" tokens but the container's combined text does. However, FontFaceObserver and Next.js @next/font both inject flat single-element structures — a single with the full test string. The split-span case doesn't appear to actually occur with either library. So the container pass is likely dead code addressing a pattern that doesn't exist. Will revert this change and simiplify the code.

nit23uec · 2026-07-13T07:30:40Z

+
+/**
+ * Remove FontFaceObserver / Next.js font-detection test elements (Node.js / cheerio).
+ * Two-pass strategy mirrors the browser version — see removeFontDetectionElements.


is there a hard orderign dependency also between these 2 passes?

no longer applicable

…tection-filter-v2

The two-pass strategy added a container pass to catch split-span noise where each child span held fewer than 10 "word" tokens. FontFaceObserver and Next.js @next/font always inject flat single-element structures, so this case doesn't occur in practice. The container pass also had a subtle ordering bug where pass 1 removing all leaf children left empty containers that pass 2 would silently skip. Also clarifies JSDoc for isFontDetectionLeaf: includes() is intentional to cover FontFaceObserver's &N suffix variants (mmMwWLliI0fiflO&1, &2...). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

startsWith is safer than includes — the &N suffix always follows the test string at the start of the text content, so includes could theoretically match legitimate content containing the string mid-text. Also adds tests pinning the &N suffix behaviour and the startsWith-not-includes boundary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

nit23uec · 2026-07-13T12:39:55Z

I am not in favor of having this fix in html analyzer level. This is in fact helping us to surface the real output that nan actual request could get. we should fix this in our prerenderer and scraper layer.

nit23uec

wrong layer to fix this issue.

sneharora1 · 2026-07-15T12:35:13Z

Fix moved to content-scraper layer. Cloising this.

anuj-adobe reviewed Jul 8, 2026

View reviewed changes

fix(html-analyzer): address review findings — fix word-check regex, a…

f0f057a

…dd browser-path unit tests, handle split-span case

sneharora1 added 2 commits July 8, 2026 19:58

Merge remote-tracking branch 'origin/main' into fix/llmo-6061-font-de…

4d496f2

…tection-filter-v2

Merge remote-tracking branch 'origin/main' into fix/llmo-6061-font-de…

f2ed927

…tection-filter-v2

sneharora1 requested a review from Danialkafeel July 9, 2026 12:54

ssilare-adobe requested a review from MysticatBot July 13, 2026 07:01

MysticatBot approved these changes Jul 13, 2026

View reviewed changes

MysticatBot added ai-reviewed Reviewed by AI complexity:medium AI-assessed PR complexity: MEDIUM labels Jul 13, 2026

nit23uec reviewed Jul 13, 2026

View reviewed changes

sneharora1 and others added 3 commits July 13, 2026 15:57

Merge remote-tracking branch 'origin/main' into fix/llmo-6061-font-de…

641f538

…tection-filter-v2

Danialkafeel approved these changes Jul 13, 2026

View reviewed changes

sneharora1 requested a review from nit23uec July 13, 2026 11:53

nit23uec requested changes Jul 13, 2026

View reviewed changes

sneharora1 closed this Jul 15, 2026

Uh oh!

Conversation

sneharora1 commented Jul 8, 2026

Summary

Test plan

Uh oh!

anuj-adobe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

MysticatBot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Previously flagged, now resolved

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Danialkafeel Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nit23uec commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nit23uec left a comment

Choose a reason for hiding this comment

Uh oh!

sneharora1 commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MysticatBot left a comment •

edited

Loading

Danialkafeel Jul 13, 2026 •

edited

Loading

nit23uec commented Jul 13, 2026 •

edited

Loading