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 6 commits intoTanStack:mainfrom
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 9m 45s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 4s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-07 14:47:23 UTC
jong-kyung
changed the title
|
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:
📝 WalkthroughWalkthroughAdds /canonical and /canonical/deep file routes and E2E tests for React, Solid, and Vue starters; refactors router head assembly to iterate matches deepest-first, deduplicate link tags by rel (so child canonical overrides parent), inject nonces, and produce deterministic ordering. 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.githubusercontent.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 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. |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
…lowercasing rel values.
jong-kyung
force-pushed
the
fix/heda-content-utils
branch
from
463260b to
c3af3f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/vue-router/src/headContentUtils.tsx (1)
89-96:⚠️ Potential issue | 🟡 MinorNormalize
relbefore dedupe matching (still needed).Line 89 uses a case-sensitive pre-check with raw
link.rel, soCanonical/canonicalcan skip dedupe and allow duplicate canonical tags.Proposed fix
- if (link.rel && relsToDedupe.has(link.rel)) { - const rel = link.rel?.toLowerCase() - if (rel && relsToDedupe.has(rel)) { - if (linksByRel.has(rel)) { - continue - } - linksByRel.add(rel) - } - } + const rel = + typeof link.rel === 'string' ? link.rel.trim().toLowerCase() : undefined + if (rel && relsToDedupe.has(rel)) { + if (linksByRel.has(rel)) { + continue + } + linksByRel.add(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 89 - 96, The dedupe pre-check uses the raw case/whitespace-sensitive link.rel which lets variants like "Canonical" or " canonical " bypass dedupe; normalize link.rel (trim and toLowerCase) before any membership checks so both the initial if (relsToDedupe.has(...)) and the subsequent rel variable use the same normalized value, then use that normalized rel for checks and for adding to linksByRel/relsToDedupe; update the block around linksByRel, relsToDedupe, and link.rel to compute a single normalizedRel and use it for all comparisons and adds.packages/react-router/src/headContentUtils.tsx (1)
104-111:⚠️ Potential issue | 🟡 MinorNormalize
relfirst; raw pre-check can miss canonical dedupe.Line 104 performs
relsToDedupe.has(link.rel)before normalization, so non-lowercase/trimmed canonical values can bypass dedupe and render duplicates.Proposed fix
- if (link.rel && relsToDedupe.has(link.rel)) { - const rel = link.rel?.toLowerCase() - if (rel && relsToDedupe.has(rel)) { - if (linksByRel.has(rel)) { - continue - } - linksByRel.add(rel) - } - } + const rel = + typeof link.rel === 'string' ? link.rel.trim().toLowerCase() : undefined + if (rel && relsToDedupe.has(rel)) { + if (linksByRel.has(rel)) { + continue + } + 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 - 111, The dedupe check uses the raw link.rel before normalization which lets non-lowercase/whitespace variants slip through; in the link-processing logic (headContentUtils.tsx) normalize link.rel first (e.g., const rel = link.rel?.toLowerCase().trim()) and then use rel for all checks against relsToDedupe and linksByRel and for adding to linksByRel, ensuring canonicalized values are consistently tested and stored to correctly dedupe links.
🤖 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/solid-router/src/headContentUtils.tsx`:
- Around line 105-112: Normalize link.rel before using it for the dedupe gate:
compute a normalizedRel = link.rel?.trim().toLowerCase() (or similar) at the
start of the block and use normalizedRel for relsToDedupe.has checks and for
adding/checking linksByRel instead of using the raw link.rel; update the
conditional that currently does relsToDedupe.has(link.rel) and subsequent checks
to use normalizedRel so values like "Canonical" or " canonical " are
deduplicated correctly.
---
Duplicate comments:
In `@packages/react-router/src/headContentUtils.tsx`:
- Around line 104-111: The dedupe check uses the raw link.rel before
normalization which lets non-lowercase/whitespace variants slip through; in the
link-processing logic (headContentUtils.tsx) normalize link.rel first (e.g.,
const rel = link.rel?.toLowerCase().trim()) and then use rel for all checks
against relsToDedupe and linksByRel and for adding to linksByRel, ensuring
canonicalized values are consistently tested and stored to correctly dedupe
links.
In `@packages/vue-router/src/headContentUtils.tsx`:
- Around line 89-96: The dedupe pre-check uses the raw case/whitespace-sensitive
link.rel which lets variants like "Canonical" or " canonical " bypass dedupe;
normalize link.rel (trim and toLowerCase) before any membership checks so both
the initial if (relsToDedupe.has(...)) and the subsequent rel variable use the
same normalized value, then use that normalized rel for checks and for adding to
linksByRel/relsToDedupe; update the block around linksByRel, relsToDedupe, and
link.rel to compute a single normalizedRel and use it for all comparisons and
adds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98830b12-786d-404d-bb19-58eae388eb77
📒 Files selected for processing (3)
packages/react-router/src/headContentUtils.tsxpackages/solid-router/src/headContentUtils.tsxpackages/vue-router/src/headContentUtils.tsx
| if (link.rel && relsToDedupe.has(link.rel)) { | ||
| const rel = link.rel?.toLowerCase() | ||
| if (rel && relsToDedupe.has(rel)) { | ||
| if (linksByRel.has(rel)) { | ||
| continue | ||
| } | ||
| linksByRel.add(rel) | ||
| } |
There was a problem hiding this comment.
Normalize rel before the dedupe gate.
Line 105 checks relsToDedupe.has(link.rel) before normalization. Values like Canonical or canonical bypass dedupe and can produce duplicate canonicals.
Proposed fix
- if (link.rel && relsToDedupe.has(link.rel)) {
- const rel = link.rel?.toLowerCase()
- if (rel && relsToDedupe.has(rel)) {
- if (linksByRel.has(rel)) {
- continue
- }
- linksByRel.add(rel)
- }
- }
+ const rel =
+ typeof link.rel === 'string' ? link.rel.trim().toLowerCase() : undefined
+ if (rel && relsToDedupe.has(rel)) {
+ if (linksByRel.has(rel)) {
+ continue
+ }
+ linksByRel.add(rel)
+ }📝 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)) { | |
| const rel = link.rel?.toLowerCase() | |
| if (rel && relsToDedupe.has(rel)) { | |
| if (linksByRel.has(rel)) { | |
| continue | |
| } | |
| linksByRel.add(rel) | |
| } | |
| const rel = | |
| typeof link.rel === 'string' ? link.rel.trim().toLowerCase() : undefined | |
| if (rel && relsToDedupe.has(rel)) { | |
| if (linksByRel.has(rel)) { | |
| continue | |
| } | |
| linksByRel.add(rel) | |
| } |
🤖 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 105 - 112,
Normalize link.rel before using it for the dedupe gate: compute a normalizedRel
= link.rel?.trim().toLowerCase() (or similar) at the start of the block and use
normalizedRel for relsToDedupe.has checks and for adding/checking linksByRel
instead of using the raw link.rel; update the conditional that currently does
relsToDedupe.has(link.rel) and subsequent checks to use normalizedRel so values
like "Canonical" or " canonical " are deduplicated correctly.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/solid-router/src/headContentUtils.tsx (1)
105-112:⚠️ Potential issue | 🟡 MinorNormalize
relbefore dedupe lookup.
toLowerCase()alone still lets values like' canonical 'bypass dedupe, so duplicate canonicals can slip through.Proposed fix
- if (link.rel) { - const rel = link.rel.toLowerCase() - if (relsToDedupe.has(rel)) { - if (linksByRel.has(rel)) { - continue - } - linksByRel.add(rel) - } - } + const rel = + typeof link.rel === 'string' ? link.rel.trim().toLowerCase() : undefined + if (rel && relsToDedupe.has(rel)) { + if (linksByRel.has(rel)) { + continue + } + linksByRel.add(rel) + }🤖 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 105 - 112, The dedupe logic uses link.rel.toLowerCase() which doesn't remove surrounding whitespace so values like " canonical " bypass relsToDedupe; update the normalization in headContentUtils.tsx to trim and lowercase the rel (e.g., use link.rel.trim().toLowerCase()) before checking relsToDedupe and before using linksByRel, and ensure you still guard for a non-empty rel value when computing the normalized rel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/solid-router/src/headContentUtils.tsx`:
- Around line 105-112: The dedupe logic uses link.rel.toLowerCase() which
doesn't remove surrounding whitespace so values like " canonical " bypass
relsToDedupe; update the normalization in headContentUtils.tsx to trim and
lowercase the rel (e.g., use link.rel.trim().toLowerCase()) before checking
relsToDedupe and before using linksByRel, and ensure you still guard for a
non-empty rel value when computing the normalized rel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34c57109-1654-4606-a85a-a3db0f717060
📒 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 (2)
- packages/vue-router/src/headContentUtils.tsx
- packages/react-router/src/headContentUtils.tsx
|
I'm leaving this PR open for review, but please note that the E2E tests for Here is what I found after digging deeper:
@schiller-manuel, could you please review my core deduplication logic? If the logic looks good, how should we handle the E2E tests? Any insights on why the |
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
Refactor