Skip to content

fix(scan): don't extract icon names from the middle of identifiers - #531

Merged
antfu merged 1 commit into
nuxt:mainfrom
giaBaoJS:fix/scan-icon-name-boundaries
Aug 25, 2026
Merged

fix(scan): don't extract icon names from the middle of identifiers#531
antfu merged 1 commit into
nuxt:mainfrom
giaBaoJS:fix/scan-icon-name-boundaries

Conversation

@giaBaoJS

@giaBaoJS giaBaoJS commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

🔗 Linked issue

Related to #410, though it does not close it. See the last paragraph.

📚 Description

createMatchRegex wraps the match in \b (src/core/scan.ts:82). A - is a word boundary, so \b puts no constraint at all on a hyphenated identifier, and any one of them that happens to contain a collection prefix is read as an icon usage:

import 'source-map-js'              ->  map:js
undici-vs-builtin-fetch.md          ->  vs:builtin-fetch
<div style="--gg-size: 4px" />      ->  gg:size

The name part is [a-z0-9-]+, which also allows a leading or trailing -. Fast-memory-efficient-Levenshtein extracts memory:efficient-: the trailing - survives because the uppercase L closes the word boundary. That is not a name Iconify can ever accept, so it can only ever be thrown away later.

47 of the 220 built-in prefixes are three characters or shorter and 24 are two (bi, ci, fa, gg, ix, la, ls, map, mi, ph, ri, si, vs, ...), which is why this happens as often as it does rather than being a curiosity.

The fix asks for two things the old pattern did not: the match may not start or end inside a longer identifier, and the name has to follow Iconify's own icon name grammar rather than "any run of lowercase, digits and dashes".

I deliberately did not go further and require the i- prefix for the dash form, which would have taken out bi-monthly-report and data-test="mdi-check" too. stringToIcon('mdi-home') resolves to { prefix: 'mdi', name: 'home' }, so a bare mdi-home is a real usage, and playgrounds/nuxt/app.vue:17 writes exactly that ('logos-nuxt-icon'). There is no way to tell those apart from prose by pattern alone, so they stay in.

Test

I measured this before and after over the 5739 .vue/.md/.mdc/.mdx/.yml/.yaml files under this repo's node_modules (about 42 MB), using the scanner's own default globInclude:

current:   69 names extracted
candidate: 50 names extracted
removed (19): ci:href, ci:src, foundation:as-an-incubation-project, ix:ltd, map:0,
  map:buffer, map:from-entries, map:index-generator, map:js, map:key,
  map:label-fg-color, map:options, map:support, memory:efficient-,
  token:code-point, vs:bash, vs:builtin-fetch, vs:type-arguments,
  vs:unicode-scalar-values
added (0):

Every one of the 19 comes from prose or an identifier, and nothing new is picked up. The playground client bundle is byte-identical before and after, still 27 icons with 18.40KB, and the existing extract icon usages snapshot is unchanged.

The three tests added to test/extract.test.ts cover the forms that must keep working (mdi:home, mdi-home, i-mdi-home, i-mdi:home, dark:i-mdi-home, mdi-light:home, uil:0-plus, logos-nuxt-icon), the identifier cases, and the grammar. Reverting src/core/scan.ts and keeping the tests fails the last two on the extraction itself:

FAIL  test/extract.test.ts > does not extract from the middle of a longer identifier
AssertionError: expected [ 'map:js' ] to deeply equal []

FAIL  test/extract.test.ts > never extracts a name that Iconify could not accept
AssertionError: expected [ 'memory:efficient-' ] to deeply equal []

pnpm test:unit is 27 passed, lint, vue-tsc and pnpm build are clean. pnpm test:playground fails on the committed fixtures.html snapshot both before and after, on a solar icon body that no longer matches, which looks like dependency drift and is unrelated to this.

On #410

I could not make this account for what #410 describes. The reporter sees roughly 8000 icons; 42 MB of real files produce 69 extracted names here, and a scanned name whose collection is not installed locally never reaches the bundle anyway, because markUnresolved (src/core/bundle.ts:130-143) keeps scanned icons best-effort by design. So this is a smaller, separate correctness problem in the same code, not that report. Happy to open it as its own issue if you would rather have the link the other way round.

On the red CI here: it is the mount fixtures snapshot, and main fails the same way at dc20973 (chore: release v2.5.1), before this branch existed. The only thing in the diff is the solar icon body, so it is not from this change.

The scan pattern used `\b` around the match, and a `-` is a word boundary, so
any hyphenated identifier containing a collection prefix was read as an icon
usage: `source-map-js` became `map:js` and the CSS custom property `--gg-size`
became `gg:size`. The name part also allowed a leading or trailing `-`, so
`Fast-memory-efficient-Levenshtein` produced `memory:efficient-`, which is not
a name Iconify can ever accept.

Require the match not to start or end inside a longer identifier, and follow
Iconify's icon name grammar for the name itself. Every documented way of
writing a usage still matches, including the bare `logos-nuxt-icon` form.
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 194620d8-c453-4a62-8f92-917a65deadcc

📥 Commits

Reviewing files that changed from the base of the PR and between dc20973 and 0d6496e.

📒 Files selected for processing (2)
  • src/core/scan.ts
  • test/extract.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The icon-reference regex now rejects matches embedded in longer identifiers and enforces lowercase alphanumeric segments separated by single hyphens. Tests cover supported icon syntaxes, separator normalization, variant prefixes, numeric names, playground references, identifier boundaries, URLs, imports, CSS custom properties, trailing hyphens, and repeated separators.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 0d649

This localized change tightens icon-name extraction and adds coverage for the affected identifier and grammar cases; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description clearly explains the scan fix, affected false positives, preserved syntax, tests, and known unrelated test failure.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing icon names from being extracted inside longer identifiers.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@antfu
antfu merged commit 729a575 into nuxt:main Aug 25, 2026
3 of 4 checks passed
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