Skip to content

fix(cli): score apple-touch-icon-precomposed at the 180px default - #3607

Merged
miguel-heygen merged 2 commits into
mainfrom
fix-apple-touch-icon-precomposed
Sep 3, 2026
Merged

fix(cli): score apple-touch-icon-precomposed at the 180px default#3607
miguel-heygen merged 2 commits into
mainfrom
fix-apple-touch-icon-precomposed

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What changed

The favicon ranker's declaredSize() matches the rel token apple-touch-icon
exactly. rel="apple-touch-icon-precomposed" is one token, not two, so it
fell through to a score of 0 instead of the 180px Apple default — even though
the selector (link[rel*="icon"]) already collects it. Same page, one
spelling apart, opposite winner inside tier 1: it never drops a candidate and
never beats the .ico tier, so it only mis-orders within the middle tier.

Fix: startsWith("apple-touch-icon") on the token instead of an exact match.
mask-icon keeps its own exact-token check (isMaskIcon), where a longer
rel really would be a different asset.

What I measured

Reverted the fix locally (exact-token match) and reran the new test:

FAIL  src/capture/faviconRanker.test.ts > rankIconCandidates > scores apple-touch-icon-precomposed at the same 180px default as apple-touch-icon
AssertionError: expected [ …(2) ] to deeply equal [ …(2) ]
- Expected
+ Received
  [
-   "https://x.test/apple-touch-icon-precomposed.png",
    "https://x.test/favicon-32.png",
+   "https://x.test/apple-touch-icon-precomposed.png",
  ]

With the fix restored:

✓ src/capture/faviconRanker.test.ts (12 tests) 3ms
Test Files  1 passed (1)
     Tests  12 passed (12)

bunx tsc --noEmit in packages/cli: same three pre-existing errors on both
origin/main and this branch (missing generated runtime files, unrelated to
this change) — no new errors introduced.

What I did NOT exercise

  • The live capture path end to end (no headless browser run against a real
    page declaring apple-touch-icon-precomposed). This is a pure ranking-unit
    change; the download loop and page-evaluate extraction it feeds into are
    untouched.
  • <link rel="manifest"> icons and data: URI hrefs, both called out as
    pre-existing gaps in the original PR's review and out of scope here.

declaredSize() matched the rel token "apple-touch-icon" exactly, so the
legacy "apple-touch-icon-precomposed" spelling (one token, not two) fell
through to 0 instead of the 180px Apple default, even though the selector
already collects it. Same page, one spelling apart, opposite winner inside
tier 1 — it never drops a candidate and never beats the .ico tier.

Switch to a startsWith check on the token. Exact-token matching stays for
mask-icon, where a longer rel really would be a different asset.

@jerrai-bot-heygen jerrai-bot-heygen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at exact head 4f8f102b11388bd059e0bc6119a5a9c5b71428a4.

The token-safe prefix match correctly gives unsized apple-touch-icon-precomposed the same 180px default as apple-touch-icon, without changing exact mask-icon handling or the mask-last tier. The new A/B fixture fails with the prior exact-token logic and preserves the other ranking tiers.

Completed checks are green; remaining CI jobs are pending and no check is red.

— Jerrai

@miguel-heygen
miguel-heygen merged commit 7dc31bd into main Sep 3, 2026
48 checks passed
@miguel-heygen
miguel-heygen deleted the fix-apple-touch-icon-precomposed branch September 3, 2026 04:21
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.

2 participants