fix(scan): don't extract icon names from the middle of identifiers - #531
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe 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 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)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
🔗 Linked issue
Related to #410, though it does not close it. See the last paragraph.
📚 Description
createMatchRegexwraps the match in\b(src/core/scan.ts:82). A-is a word boundary, so\bputs 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:The name part is
[a-z0-9-]+, which also allows a leading or trailing-.Fast-memory-efficient-Levenshteinextractsmemory:efficient-: the trailing-survives because the uppercaseLcloses 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 outbi-monthly-reportanddata-test="mdi-check"too.stringToIcon('mdi-home')resolves to{ prefix: 'mdi', name: 'home' }, so a baremdi-homeis a real usage, andplaygrounds/nuxt/app.vue:17writes 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/.yamlfiles under this repo'snode_modules(about 42 MB), using the scanner's own defaultglobInclude: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 existingextract icon usagessnapshot is unchanged.The three tests added to
test/extract.test.tscover 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. Revertingsrc/core/scan.tsand keeping the tests fails the last two on the extraction itself:pnpm test:unitis 27 passed, lint,vue-tscandpnpm buildare clean.pnpm test:playgroundfails on the committedfixtures.htmlsnapshot both before and after, on asolaricon 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 fixturessnapshot, andmainfails the same way at dc20973 (chore: release v2.5.1), before this branch existed. The only thing in the diff is thesolaricon body, so it is not from this change.