fix: scope alternate link dedupeKey by href when type is used#759
Conversation
📝 WalkthroughWalkthrough
ChangesLink Deduplication Key & Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/unhead/src/utils/dedupe.ts (1)
16-21: ⚡ Quick winConsider adding a regression test for the two-feed scenario.
No test file is visible in this PR. Without a test covering
rel="alternate"links with the sametypebut differenthrefvalues, this fix could silently regress. A minimal test asserting both keys are distinct (and thatdedupeKeyreturns different values for the two links) would lock in the behaviour.🤖 Prompt for 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. In `@packages/unhead/src/utils/dedupe.ts` around lines 16 - 21, Add a regression test that covers the two-feed scenario for rel="alternate" links: create two link entries with t === 'link', rel === 'alternate', the same props.type but different props.href (and one with props.hreflang absent), call the dedupe key function (dedupeKey) or run the dedupe pipeline that uses it, and assert that dedupeKey returns different strings for each link and that both links are preserved after deduplication; this ensures the branch in dedupe.ts that returns `alternate:${props.type}:${props.href || ''}` remains correct and prevents future regressions.
🤖 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.
Nitpick comments:
In `@packages/unhead/src/utils/dedupe.ts`:
- Around line 16-21: Add a regression test that covers the two-feed scenario for
rel="alternate" links: create two link entries with t === 'link', rel ===
'alternate', the same props.type but different props.href (and one with
props.hreflang absent), call the dedupe key function (dedupeKey) or run the
dedupe pipeline that uses it, and assert that dedupeKey returns different
strings for each link and that both links are preserved after deduplication;
this ensures the branch in dedupe.ts that returns
`alternate:${props.type}:${props.href || ''}` remains correct and prevents
future regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f0558ab-3d3d-4e34-8909-626a96969808
📒 Files selected for processing (1)
packages/unhead/src/utils/dedupe.ts
Reframes the existing 'dedupes RSS feeds with same type' test (which asserted the buggy collapse-on-type behaviour) to cover the correct rehydration case (same type + same href dedupes), and adds two new regression tests for issue unjs#758 — multiple RSS feeds with the same type but different hrefs must coexist, both within a single push and across separate head.push calls.
|
Added regression coverage in
I also rewrote the existing Full |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/unhead/test/unit/server/deduping.test.ts (1)
584-600: 💤 Low valueNew test duplicates existing coverage at lines 526–553.
dedupes RSS feeds with same type and same href on rehydrationis functionally identical to the pre-existingdedupes alternate links without hreflang using hreftest (lines 526–553): both push the sameapplication/rss+xml+feed.xmllink twice and assert exactly one tag survives. The existing test is stricter (usestoMatchInlineSnapshot). The new test also passes under the old broken logic (type-only key still deduplicates same-type+same-href entries), so it provides no regression guard for the actual fix.Consider removing this block and relying on the existing test at lines 526–553 for that scenario.
🤖 Prompt for 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. In `@packages/unhead/test/unit/server/deduping.test.ts` around lines 584 - 600, Remove the redundant test named "dedupes RSS feeds with same type and same href on rehydration" (the it(...) block that pushes two identical link entries with type 'application/rss+xml' and href 'https://example.com/feed.xml'), since it duplicates the existing "dedupes alternate links without hreflang using href" coverage; delete the entire it(...) block to avoid duplicate assertions and rely on the stricter existing test.
🤖 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.
Nitpick comments:
In `@packages/unhead/test/unit/server/deduping.test.ts`:
- Around line 584-600: Remove the redundant test named "dedupes RSS feeds with
same type and same href on rehydration" (the it(...) block that pushes two
identical link entries with type 'application/rss+xml' and href
'https://example.com/feed.xml'), since it duplicates the existing "dedupes
alternate links without hreflang using href" coverage; delete the entire it(...)
block to avoid duplicate assertions and rely on the stricter existing test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32ba04a0-0a75-4da3-80b0-302ffb7e2c5e
📒 Files selected for processing (1)
packages/unhead/test/unit/server/deduping.test.ts
|
Heads-up on the red
|
harlan-zw
changed the title
|
Thanks! |
Backport of #759 to v2. Multiple RSS/Atom alternate links sharing the same `type` but with different `href` values were being collapsed into one tag because the dedupe key was built from `type` alone. Include `href` in the key for type-based alternates so distinct feeds are preserved. `hreflang` precedence is unchanged. Fixes #758
2 participants
Problem
Multiple RSS/Atom alternate tags with the same ype but different hrefs are incorrectly deduplicated into a single tag.
Root cause
In dedupe.ts, the deduplication key for
el="alternate" links is built as:
s const altKey = props.hreflang || props.type if (altKey) returnalternate:${altKey}When hreflang is absent (e.g. RSS/Atom feeds), all feeds with the same ype (e.g. �pplication/rss+xml) share the same key — �lternate:application/rss+xml — regardless of their href. This collapses all but one of them.
Fix
Use hreflang alone as the key (language variants are uniquely identified by it). For type-based alternates (RSS/Atom), include href in the key so distinct feeds are not conflated:
s if (t === 'link' && props.rel === 'alternate') { if (props.hreflang) returnalternate:${props.hreflang}if (props.type) returnalternate:${props.type}:${props.href || ''}}Fixes #758
Summary by CodeRabbit