refactor(logo-bimi): read the record through bimi-url - #863
Conversation
The BIMI logic has no metascraper in it: a DNS lookup, a tag parser and an SVG-over-https check. It now lives in bimi-url, so anything that wants a domain logo can resolve one without pulling a scraper in. What stays here is the rule: mapping a scraped URL to its registrable domain and running the result through the `logo` validator. `createGetLogo`, `resolveLogoUrl` and `toLogoUrl` are re-exported by identity, so the surface published as 5.52.0 is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LDmGejCQnrpy5FDBjs6cyW
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe BIMI logo package now delegates logo discovery and URL resolution to ChangesBIMI resolver integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/metascraper-logo-bimi/README.md`:
- Around line 56-63: Add the missing KeyvRedis import from `@keyv/redis` to the
README usage example before it instantiates KeyvRedis, while preserving the
existing metascraper configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 225c92b1-05de-4c6d-8fa5-8926cfffc9e9
📒 Files selected for processing (11)
packages/metascraper-logo-bimi/README.mdpackages/metascraper-logo-bimi/package.jsonpackages/metascraper-logo-bimi/src/index.d.tspackages/metascraper-logo-bimi/src/index.jspackages/metascraper-logo-bimi/test/dns.jspackages/metascraper-logo-bimi/test/exports.jspackages/metascraper-logo-bimi/test/get-logo.jspackages/metascraper-logo-bimi/test/helpers.jspackages/metascraper-logo-bimi/test/record.jspackages/metascraper-logo-bimi/test/resolve-logo-url.jspnpm-workspace.yaml
💤 Files with no reviewable changes (4)
- packages/metascraper-logo-bimi/test/resolve-logo-url.js
- packages/metascraper-logo-bimi/test/dns.js
- packages/metascraper-logo-bimi/test/record.js
- packages/metascraper-logo-bimi/test/get-logo.js
The fixture declares no logo, so metascraper-logo-favicon resolves it by probing the live site. A probe that times out from the runner demotes to the Google favicon service, and the snapshot then mismatches on a value that says nothing about the fixture. It is not drift: no snapshot in the suite records a Google URL, so the fallback is only ever the symptom of a failed probe. Asserting the shape keeps the rest of the fixture pinned exactly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LDmGejCQnrpy5FDBjs6cyW
The assertion says what it does, and why it is not snapshotted is already in the commit that introduced it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LDmGejCQnrpy5FDBjs6cyW
Both identifiers in the snippet were undefined, so pasting it threw. Dropping the keyvOpts line takes the Redis import with it, leaving an example that shows the one option the paragraph is about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LDmGejCQnrpy5FDBjs6cyW
Moves the BIMI logic out to bimi-url and consumes it here. No behaviour change: 517 lines deleted, 36 added.
Why
Nothing in the resolver was metascraper-shaped. It is a DNS lookup, a tag parser and an SVG-over-https check, and the two
@metascraper/helperscalls it did make wereprotocol()andmimeExtension(). Keeping it here meant anything else wanting a domain logo (unavatar, a mail UI, a CLI) had to install a scraper to get one.What stays is the part that is a rule: mapping a scraped URL to its registrable domain, and running the result through the
logovalidator.Compatibility
The surface published as 5.52.0 is unchanged.
createGetLogo,resolveLogoUrlandtoLogoUrlare re-exported by identity, andtest/exports.jspins that witht.isso it cannot drift:src/index.d.tsstopped restating the options and derives them from the dependency, so the two can no longer disagree:The two helper calls
bimi-urlis a leaf package, so depending on@metascraper/helperswould have dragged jsdom, chrono-node and lodash into it for two checks. Both were replaced with equivalents, not approximations:protocol(url) === 'https'new URL(url).protocol === 'https:'in a try/catchmimeExtension(ct) === 'svg'image/svg+xmlcompare after stripping paramsThe mime table was checked before swapping:
image/svg+xmlis the only content type that maps tosvg(image/svgandapplication/svg+xmlboth returnnull), and it already strips;charset=utf-8, which the charset test covers.bimi-urlships 30 tests at 100% statements, branches, functions and lines.Its dependencies are
@keyvhq/memoizeandreachable-url, both of which this package already had.Tests
The resolver tests moved to
bimi-urlwith it. What remains here is what exercises the rule:test/index.js(precedence over markup, fallback to the next rule,nullwhen nothing resolves) plus the newtest/exports.js. All 5 pass, lint clean.Verified downstream too: microlink/api resolves the same logos through this package before and after.
Note on
pnpm-workspace.yamlminimumReleaseAgeis a pnpm 11 default and this repo runs withlockfile=false, so a clean install resolves fresh and would otherwise refuse a package this newly published. The entry can be dropped once 1.0.0 ages past the threshold.Note
Medium Risk
Large logic move into a new external package; public API is preserved via identity re-exports, but any behavioral drift in
bimi-urlwould affect logo resolution everywhere this rule runs.Overview
BIMI resolution is delegated to the new
bimi-urldependency instead of living in this package.metascraper-logo-biminow only wirescreateGetLogo(options)into thelogorule (registrable domain from the scraped URL) and re-exportscreateGetLogo,resolveLogoUrl, andtoLogoUrlby reference frombimi-url; TypeScript options areParameters<typeof import('bimi-url')>[0]so they stay in sync.Local DNS parsing, memoization, and SVG-over-HTTPS checks (~500 lines) are removed along with
@keyvhq/memoizeandreachable-url; resolver-focused tests are dropped in favor oftest/exports.jsand existing rule tests. The README points option docs at bimi-url.The blogger integration test still expects an
httpslogo but omitslogofrom the metadata snapshot.pnpm-workspace.yamlsetsminimumReleaseAge: 0so the newbimi-url@~1.0.0dependency can install under pnpm’s release-age policy.Reviewed by Cursor Bugbot for commit e7c193f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
bimi-urlimplementation.createGetLogo,resolveLogoUrl, andtoLogoUrlare now re-exported frombimi-url.metascraper-logo-bimiREADME and updated the customization example.bimi-urlre-exports.logofield.