fix(router): prevent script tag duplication in SSR and client-side navigation #5095
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (2)
WalkthroughAdds an /inline-scripts route and navigation to React Start and Solid Start e2e apps, generates route tree and typings for both frameworks, introduces Playwright tests validating no duplication for external and inline scripts across SSR and client navigations, and updates Asset.Script in both React and Solid routers to deduplicate scripts and gate client injection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant App as App (SSR / Client)
participant Head as HeadCollector
participant Asset as Asset.Script
participant DOM as Document
rect rgb(245,245,255)
note over App,DOM: Server-side render (SSR)
U->>App: HTTP request
App->>Head: collect head() scripts
Head->>Asset: render Script (SSR)
Asset->>DOM: emit <script> (src or inline)
end
rect rgb(245,255,245)
note over App,DOM: Client hydration / navigation
U->>App: hydrate or navigate
App->>Head: render Scripts
Head->>Asset: Script (client)
Asset->>Asset: detect environment (router.isServer or onMount)
Note right of Asset: client render returns null\nuse effect/onMount to inject
Asset->>DOM: normalize src or compute inline signature
alt No existing matching script found
Asset->>DOM: create and append <script>
else Duplicate found
Asset-->>DOM: skip injection
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Poem
Tip You can disable the changed files summary in the walkthrough.Disable the ✨ Finishing Touches
🧪 Generate unit tests
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 |
|
View your CI Pipeline Execution ↗ for commit 22b268f ☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
packages/react-router/src/Asset.tsx (2)
49-55: Make src-dedup robust (relative vs absolute URLs, CSS escaping).Attribute-selector equality can miss matches (e.g., SSR absolute vs client relative) and can break if src contains characters needing CSS escaping. Compare normalized URLs from document.scripts instead.
- const existingScript = document.querySelector( - `script[src="${attrs.src}"]`, - ) - if (existingScript) { + const existingScript = Array.from(document.scripts).find((s) => { + const attr = s.getAttribute('src') + if (!attr) return false + try { + const a = new URL(attr, document.baseURI).href + const b = new URL(attrs.src!, document.baseURI).href + return a === b + } catch { + return attr === attrs.src + } + }) + if (existingScript) { return }
81-87: Inline-script dedup: consider minor normalization and scoping to head.textContent equality is fine but brittle to whitespace. Optional: trim or normalize and scope query to document.head to reduce scan cost.
- const existingScript = Array.from( - document.querySelectorAll('script:not([src])'), - ).find((script) => script.textContent === children) + const needle = children.trim() + const existingScript = Array.from( + (document.head ?? document).querySelectorAll('script:not([src])'), + ).find((script) => (script.textContent ?? '').trim() === needle)packages/solid-router/src/Asset.tsx (2)
41-47: Harden external-script dedup (normalize URL, avoid CSS selector pitfalls).Using
querySelector('script[src="..."]')can miss absolute/relative URL equivalence and needs escaping. Prefer normalized URL comparison.- const existingScript = document.querySelector( - `script[src="${attrs.src}"]`, - ) - if (existingScript) { + const existingScript = Array.from(document.scripts).find((s) => { + const attr = s.getAttribute('src') + if (!attr) return false + try { + const a = new URL(attr, document.baseURI).href + const b = new URL(attrs.src!, document.baseURI).href + return a === b + } catch { + return attr === attrs.src + } + }) + if (existingScript) { return }
67-73: Inline-script dedup: optional normalization and head scoping.Trim to avoid whitespace mismatches and scope to head to reduce scan surface.
- const existingScript = Array.from( - document.querySelectorAll('script:not([src])'), - ).find((script) => script.textContent === children) + const needle = children.trim() + const existingScript = Array.from( + (document.head ?? document).querySelectorAll('script:not([src])'), + ).find((script) => (script.textContent ?? '').trim() === needle)e2e/react-start/basic/src/routes/inline-scripts.tsx (1)
9-12: Nit: explicit type is redundant for classic scripts.
type: 'text/javascript'is default; can be omitted to reduce noise (keep if tests assert presence).- type: 'text/javascript', + // type: 'text/javascript', // optionale2e/solid-start/basic/tests/script-duplication.spec.ts (4)
10-17: Prefer locators and ends-with selector for robustness and auto-waitUsing page.locator with [src$="script.js"] is more resilient (absolute vs relative paths) and leverages Playwright’s auto-wait.
Apply:
- // Count script tags with src="script.js" - const scriptCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - - // Should have exactly one script tag - expect(scriptCount).toBe(1) + // Should have exactly one script tag (robust selector and auto-wait) + await expect(page.locator('script[src$="script.js"]')).toHaveCount(1)
32-37: Reduce flakiness by replacing manual counts with locator assertionsThese spots duplicate the same pattern; use toHaveCount for consistency and auto-wait.
Apply:
- const firstNavCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(firstNavCount).toBe(1) + await expect(page.locator('script[src$="script.js"]')).toHaveCount(1) - const secondNavCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(secondNavCount).toBe(1) + await expect(page.locator('script[src$="script.js"]')).toHaveCount(1) - const finalCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(finalCount).toBe(1) + await expect(page.locator('script[src$="script.js"]')).toHaveCount(1)Also applies to: 46-51, 77-81
95-113: Use locator filtering for inline scripts instead of string includes in page.evaluateLocator.hasText is simpler and less brittle against whitespace/minification differences.
Apply:
- // Count specific inline scripts - const script1Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_1 = true'), - ).length - }) - - const script2Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_2 = "test"'), - ).length - }) - - // Should have exactly one of each inline script - expect(script1Count).toBe(1) - expect(script2Count).toBe(1) + // Should have exactly one of each inline script + const inlineScripts = page.locator('script:not([src])') + await expect( + inlineScripts.filter({ hasText: 'window.INLINE_SCRIPT_1 = true' }), + ).toHaveCount(1) + await expect( + inlineScripts.filter({ hasText: 'window.INLINE_SCRIPT_2 = "test"' }), + ).toHaveCount(1)
129-145: Tighten selector to exact and use locators for inline script counts
- Use exact: true for "Inline Scripts" to mirror the "Scripts" fix.
- Use locator filters + toHaveCount for stable, auto-waiting assertions.
Apply:
- await page.getByRole('link', { name: 'Inline Scripts' }).click() + await page.getByRole('link', { name: 'Inline Scripts', exact: true }).click() @@ - const firstNavScript1Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_1 = true'), - ).length - }) - expect(firstNavScript1Count).toBe(1) + await expect( + page + .locator('script:not([src])') + .filter({ hasText: 'window.INLINE_SCRIPT_1 = true' }), + ).toHaveCount(1) @@ - await page.getByRole('link', { name: 'Inline Scripts' }).click() + await page.getByRole('link', { name: 'Inline Scripts', exact: true }).click() @@ - const secondNavScript1Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_1 = true'), - ).length - }) - expect(secondNavScript1Count).toBe(1) + const inlineScripts = page.locator('script:not([src])') + await expect( + inlineScripts.filter({ hasText: 'window.INLINE_SCRIPT_1 = true' }), + ).toHaveCount(1) + // Optional: also assert script 2 stays single across navs + await expect( + inlineScripts.filter({ hasText: 'window.INLINE_SCRIPT_2 = "test"' }), + ).toHaveCount(1)Also applies to: 150-165
e2e/solid-start/basic/src/routes/inline-scripts.tsx (1)
11-14: Normalize the scripttypeto avoid false negatives in de-dup checks.If the de-dup logic considers attributes, mixing an explicit
typewith an implicit default can cause mismatches. Prefer being consistent across both inline scripts.Apply one of the following diffs (pick one):
Remove the explicit
type:{ children: 'window.INLINE_SCRIPT_2 = "test"; console.log("Inline script 2 executed");', - type: 'text/javascript', },Or add it to the first one, too:
{ children: 'window.INLINE_SCRIPT_1 = true; console.log("Inline script 1 executed");', + type: 'text/javascript', },e2e/react-start/basic/tests/script-duplication.spec.ts (2)
11-13: Make assertions more robust and less timing‑sensitive by using locators with built-in retries.Direct
page.evaluatecounts are brittle (attribute equality and timing). Using locator queries withtoHaveCountimproves resiliency and avoids hard equality onsrc="script.js"vs/script.js.Apply this diff to replace querySelectorAll/evaluate with locators and add exact matching for the Inline Scripts link:
- const scriptCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(scriptCount).toBe(1) + const external = page.locator('script[src$="script.js"]') + await expect(external).toHaveCount(1) - const firstNavCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(firstNavCount).toBe(1) + const externalAfterFirstNav = page.locator('script[src$="script.js"]') + await expect(externalAfterFirstNav).toHaveCount(1) - const secondNavCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(secondNavCount).toBe(1) + const externalAfterSecondNav = page.locator('script[src$="script.js"]') + await expect(externalAfterSecondNav).toHaveCount(1) - const finalCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(finalCount).toBe(1) + const externalFinal = page.locator('script[src$="script.js"]') + await expect(externalFinal).toHaveCount(1) - const script1Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_1 = true'), - ).length - }) - const script2Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_2 = "test"'), - ).length - }) - expect(script1Count).toBe(1) - expect(script2Count).toBe(1) + const inline1 = page.locator('script:not([src])', { + hasText: 'window.INLINE_SCRIPT_1 = true', + }) + const inline2 = page.locator('script:not([src])', { + hasText: 'window.INLINE_SCRIPT_2 = "test"', + }) + await expect(inline1).toHaveCount(1) + await expect(inline2).toHaveCount(1) - await page.getByRole('link', { name: 'Inline Scripts' }).click() + await page.getByRole('link', { name: 'Inline Scripts', exact: true }).click() - const firstNavScript1Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_1 = true'), - ).length - }) - expect(firstNavScript1Count).toBe(1) + const inline1AfterFirstNav = page.locator('script:not([src])', { + hasText: 'window.INLINE_SCRIPT_1 = true', + }) + await expect(inline1AfterFirstNav).toHaveCount(1) - const secondNavScript1Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_1 = true'), - ).length - }) - expect(secondNavScript1Count).toBe(1) + const inline1AfterSecondNav = page.locator('script:not([src])', { + hasText: 'window.INLINE_SCRIPT_1 = true', + }) + await expect(inline1AfterSecondNav).toHaveCount(1)Optionally also assert
inline2on client nav for completeness.Also applies to: 32-36, 47-50, 78-81, 96-103, 105-112, 136-143, 156-163
56-85: Add a repeat-navigation check for inline scripts (parity with external).External scripts are exercised through multiple cycles; inline scripts are only checked once. Add a small loop to navigate away/back a few times and assert counts stay at 1.
Happy to push a follow-up diff that mirrors the “multiple navigation cycles” test for inline scripts.
Also applies to: 123-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e/react-start/basic/src/routeTree.gen.ts(13 hunks)e2e/react-start/basic/src/routes/__root.tsx(1 hunks)e2e/react-start/basic/src/routes/inline-scripts.tsx(1 hunks)e2e/react-start/basic/tests/navigation.spec.ts(1 hunks)e2e/react-start/basic/tests/script-duplication.spec.ts(1 hunks)e2e/solid-start/basic/src/routeTree.gen.ts(11 hunks)e2e/solid-start/basic/src/routes/__root.tsx(1 hunks)e2e/solid-start/basic/src/routes/inline-scripts.tsx(1 hunks)e2e/solid-start/basic/tests/navigation.spec.ts(1 hunks)e2e/solid-start/basic/tests/script-duplication.spec.ts(1 hunks)packages/react-router/src/Asset.tsx(3 hunks)packages/solid-router/src/Asset.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/react-start/basic/src/routes/inline-scripts.tsx (1)
e2e/solid-start/basic/src/routes/inline-scripts.tsx (1)
Route(3-18)
e2e/solid-start/basic/src/routes/inline-scripts.tsx (1)
e2e/react-start/basic/src/routes/inline-scripts.tsx (1)
Route(1-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (11)
packages/react-router/src/Asset.tsx (1)
118-121: Manually verify no hydration warnings: SSR renders the asset script but the client returnsnull, causing a DOM mismatch—ensure in React 18/19 (dev and prod) across your supported environments that no hydration warnings or dropped nodes occur.packages/solid-router/src/Asset.tsx (1)
98-101: Client-side null vs SSR script: verify Solid hydration behavior.Returning null on client while SSR emitted a script alters structure. Please verify Solid’s hydration doesn’t warn or discard the SSR node across dev/prod.
e2e/react-start/basic/src/routes/inline-scripts.tsx (1)
18-28: LGTM: route component for inline-script tests.Clear test ids and copy. Matches the e2e intent.
e2e/solid-start/basic/tests/navigation.spec.ts (1)
37-37: Good fix to disambiguate the "Scripts" linkUsing exact: true prevents accidental matches with "Inline Scripts" and reduces flakiness after adding the new nav item.
e2e/solid-start/basic/src/routes/__root.tsx (1)
106-113: Nav entry addition looks goodConsistent styling and placement with neighboring links. No concerns.
e2e/react-start/basic/tests/navigation.spec.ts (1)
37-37: Solid selector updateMatching the "Scripts" link with exact: true avoids ambiguity with "Inline Scripts". LGTM.
e2e/react-start/basic/src/routes/__root.tsx (1)
144-151: Inline Scripts link addition LGTMMatches existing pattern and active styling. No issues spotted.
e2e/solid-start/basic/src/routes/inline-scripts.tsx (2)
3-18: Route definition adds clear inline-script coverage — looks good.Mirrors the React test route and is appropriate for exercising inline-script de-duplication.
20-30: UI scaffold is fine for e2e anchoring.Solid’s
classusage anddata-testidare correct and minimal.e2e/solid-start/basic/src/routeTree.gen.ts (1)
19-19: Generated route tree updates for/inline-scriptslook consistent.Import added, route updated, and all relevant maps/unions include the new path. No manual edits needed.
Ensure the route tree was regenerated from the same commit that adds
routes/inline-scripts.tsxto avoid drift after merges.Also applies to: 78-82, 243-247, 275-279, 308-312, 345-346, 446-447, 866-867
e2e/react-start/basic/src/routeTree.gen.ts (1)
26-26: React route tree and module augmentations for/inline-scriptsare coherent.The route import/update, type unions, module declarations, and root children wiring are all in place.
Same note: confirm these artifacts are regenerated post-merge to keep them in sync with any route file moves/renames.
Also applies to: 94-98, 273-276, 306-309, 343-346, 384-385, 417-420, 453-454, 494-498, 568-574, 852-858, 1178-1194, 1905-1906
| export const Route = createFileRoute({ | ||
| head: () => ({ | ||
| scripts: [ | ||
| { | ||
| children: | ||
| 'window.INLINE_SCRIPT_1 = true; console.log("Inline script 1 executed");', | ||
| }, | ||
| { | ||
| children: | ||
| 'window.INLINE_SCRIPT_2 = "test"; console.log("Inline script 2 executed");', | ||
| type: 'text/javascript', | ||
| }, | ||
| ], | ||
| }), | ||
| component: InlineScriptsComponent, | ||
| }) |
There was a problem hiding this comment.
Fix createFileRoute usage: missing path argument.
React Start routes typically call createFileRoute('/inline-scripts'). The current call with an object will fail type-checking or at runtime.
-export const Route = createFileRoute({
+export const Route = createFileRoute('/inline-scripts')({📝 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.
| export const Route = createFileRoute({ | |
| head: () => ({ | |
| scripts: [ | |
| { | |
| children: | |
| 'window.INLINE_SCRIPT_1 = true; console.log("Inline script 1 executed");', | |
| }, | |
| { | |
| children: | |
| 'window.INLINE_SCRIPT_2 = "test"; console.log("Inline script 2 executed");', | |
| type: 'text/javascript', | |
| }, | |
| ], | |
| }), | |
| component: InlineScriptsComponent, | |
| }) | |
| export const Route = createFileRoute('/inline-scripts')({ | |
| head: () => ({ | |
| scripts: [ | |
| { | |
| children: | |
| 'window.INLINE_SCRIPT_1 = true; console.log("Inline script 1 executed");', | |
| }, | |
| { | |
| children: | |
| 'window.INLINE_SCRIPT_2 = "test"; console.log("Inline script 2 executed");', | |
| type: 'text/javascript', | |
| }, | |
| ], | |
| }), | |
| component: InlineScriptsComponent, | |
| }) |
🤖 Prompt for AI Agents
In e2e/react-start/basic/src/routes/inline-scripts.tsx around lines 1 to 16, the
createFileRoute call is missing the required path argument; change the
invocation to pass the route path as the first parameter (e.g.
'/inline-scripts') and pass the existing config object as the second parameter
so the function signature matches createFileRoute(path, config) and
TypeScript/runtime checks succeed.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/react-router/src/Asset.tsx (1)
119-119: Narrow effect deps to avoid teardown/re-execution of scripts.Depending on attrs object identity triggers unnecessary remove/re-add and re-exec of inline code.
- }, [attrs, children]) + }, [ + attrs?.src, + attrs?.type, + attrs?.crossOrigin, + attrs?.integrity, + attrs?.referrerPolicy, + (attrs as Record<string, unknown>)?.['nonce'], + children, + ])
🧹 Nitpick comments (1)
packages/react-router/src/Asset.tsx (1)
5-9: Make ScriptAttrs explicit for commonly used fields.Improves type-safety for deps and dedupe keys.
interface ScriptAttrs { [key: string]: string | boolean | undefined src?: string suppressHydrationWarning?: boolean + type?: string + nonce?: string + crossOrigin?: string + integrity?: string + referrerPolicy?: string }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/src/Asset.tsx(4 hunks)packages/solid-router/src/Asset.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/solid-router/src/Asset.tsx
🧰 Additional context used
🪛 ESLint
packages/react-router/src/Asset.tsx
[error] 3-3: ./useRouter import should occur before type import of @tanstack/router-core
(import/order)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (1)
packages/react-router/src/Asset.tsx (1)
121-124: Server-only render + client injection pattern looks good.Prevents hydration dupes while preserving SSR output.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
e2e/react-start/basic/tests/script-duplication.spec.ts (7)
9-13: Prefer locator-based counts with auto-retry to reduce flakiness.Use Playwright locators + toHaveCount for built-in waiting instead of page.evaluate + manual counts.
Apply:
- const scriptCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - - expect(scriptCount).toBe(1) + const externalScripts = page.locator('script[src*="script.js"]') + await expect(externalScripts).toHaveCount(1) @@ - const firstNavCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(firstNavCount).toBe(1) + const externalScripts1 = page.locator('script[src*="script.js"]') + await expect(externalScripts1).toHaveCount(1) @@ - const secondNavCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(secondNavCount).toBe(1) + const externalScripts2 = page.locator('script[src*="script.js"]') + await expect(externalScripts2).toHaveCount(1) @@ - const finalCount = await page.evaluate(() => { - return document.querySelectorAll('script[src="script.js"]').length - }) - expect(finalCount).toBe(1) + const externalScriptsFinal = page.locator('script[src*="script.js"]') + await expect(externalScriptsFinal).toHaveCount(1)Also applies to: 26-30, 37-41, 61-65
76-83: Inline script checks: use locator filtering with regex for resilience.Filtering text via JS can be brittle. Use locator.filter({ hasText: /.../ }) with whitespace-tolerant regex.
Apply:
- const script1Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_1 = true'), - ).length - }) + const inline1 = page + .locator('script:not([src])') + .filter({ hasText: /window\.INLINE_SCRIPT_1\s*=\s*true/ }) + await expect(inline1).toHaveCount(1) @@ - const script2Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_2 = "test"'), - ).length - }) + const inline2 = page + .locator('script:not([src])') + .filter({ hasText: /window\.INLINE_SCRIPT_2\s*=\s*["']test["']/ }) + await expect(inline2).toHaveCount(1) @@ - const firstNavScript1Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_1 = true'), - ).length - }) - expect(firstNavScript1Count).toBe(1) + const inline1First = page + .locator('script:not([src])') + .filter({ hasText: /window\.INLINE_SCRIPT_1\s*=\s*true/ }) + await expect(inline1First).toHaveCount(1) @@ - const secondNavScript1Count = await page.evaluate(() => { - const scripts = Array.from(document.querySelectorAll('script:not([src])')) - return scripts.filter( - (script) => - script.textContent && - script.textContent.includes('window.INLINE_SCRIPT_1 = true'), - ).length - }) - expect(secondNavScript1Count).toBe(1) + const inline1Second = page + .locator('script:not([src])') + .filter({ hasText: /window\.INLINE_SCRIPT_1\s*=\s*true/ }) + await expect(inline1Second).toHaveCount(1)Also applies to: 85-92, 111-118, 129-136
7-7: Use toBeVisible instead of toBeInViewport for stability.Visibility is sufficient and less flaky across CI viewport differences.
Apply:
- await expect(page.getByTestId('scripts-test-heading')).toBeInViewport() + await expect(page.getByTestId('scripts-test-heading')).toBeVisible() @@ - await expect(page.getByTestId('scripts-test-heading')).toBeInViewport() + await expect(page.getByTestId('scripts-test-heading')).toBeVisible() @@ - await expect(page.getByTestId('scripts-test-heading')).toBeInViewport() + await expect(page.getByTestId('scripts-test-heading')).toBeVisible() @@ - await expect(page.getByTestId('scripts-test-heading')).toBeInViewport() + await expect(page.getByTestId('scripts-test-heading')).toBeVisible() @@ - await expect(page.getByTestId('scripts-test-heading')).toBeInViewport() + await expect(page.getByTestId('scripts-test-heading')).toBeVisible() @@ - await expect( - page.getByTestId('inline-scripts-test-heading'), - ).toBeInViewport() + await expect(page.getByTestId('inline-scripts-test-heading')).toBeVisible() @@ - await expect( - page.getByTestId('inline-scripts-test-heading'), - ).toBeInViewport() + await expect(page.getByTestId('inline-scripts-test-heading')).toBeVisible() @@ - await expect( - page.getByTestId('inline-scripts-test-heading'), - ).toBeInViewport() + await expect(page.getByTestId('inline-scripts-test-heading')).toBeVisible()Also applies to: 24-24, 35-35, 53-53, 59-59, 72-75, 106-110, 124-128
15-16: Await script execution deterministically.Directly reading window props can race with async injection/loading. Wait for the condition.
Apply:
- expect(await page.evaluate('window.SCRIPT_1')).toBe(true) + await page.waitForFunction(() => (window as any).SCRIPT_1 === true) @@ - expect(await page.evaluate('window.SCRIPT_1')).toBe(true) + await page.waitForFunction(() => (window as any).SCRIPT_1 === true) @@ - expect(await page.evaluate('window.SCRIPT_1')).toBe(true) + await page.waitForFunction(() => (window as any).SCRIPT_1 === true) @@ - expect(await page.evaluate('window.INLINE_SCRIPT_1')).toBe(true) - expect(await page.evaluate('window.INLINE_SCRIPT_2')).toBe('test') + await page.waitForFunction(() => (window as any).INLINE_SCRIPT_1 === true) + await page.waitForFunction(() => (window as any).INLINE_SCRIPT_2 === 'test') @@ - expect(await page.evaluate('window.INLINE_SCRIPT_1')).toBe(true) - expect(await page.evaluate('window.INLINE_SCRIPT_2')).toBe('test') + await page.waitForFunction(() => (window as any).INLINE_SCRIPT_1 === true) + await page.waitForFunction(() => (window as any).INLINE_SCRIPT_2 === 'test')Also applies to: 42-42, 66-66, 97-99, 140-142
106-106: Use exact: true for “Inline Scripts” for selector consistency.Avoid accidental partial matches and align with “Scripts” links.
Apply:
- await page.getByRole('link', { name: 'Inline Scripts' }).click() + await page.getByRole('link', { name: 'Inline Scripts', exact: true }).click() @@ - await page.getByRole('link', { name: 'Inline Scripts' }).click() + await page.getByRole('link', { name: 'Inline Scripts', exact: true }).click()Also applies to: 124-124
143-143: Optional: add history back/forward scenario.Duplicates sometimes surface on bfcache/history restores. Consider adding:
test('no duplicates on back/forward restores', async ({ page }) => { await page.goto('/') await page.getByRole('link', { name: 'Scripts', exact: true }).click() await expect(page.getByTestId('scripts-test-heading')).toBeVisible() await page.goBack() await expect(page.getByRole('link', { name: 'Posts' })).toBeVisible() await page.goForward() await expect(page.getByTestId('scripts-test-heading')).toBeVisible() await expect(page.locator('script[src*="script.js"]')).toHaveCount(1) })
10-10: Verify script selector robustnessInjected scripts use a strict
src="script.js"match and have no stable identifiers. To avoid breakage when asset names include hashes or query params, either:
- Add a fixed
idordata-testidto the<script>inhead()- Or update the test selector to something like
querySelectorAll('script[src*="script.js"]')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/react-start/basic/tests/script-duplication.spec.ts(1 hunks)e2e/solid-start/basic/tests/script-duplication.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/solid-start/basic/tests/script-duplication.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (1)
e2e/react-start/basic/tests/script-duplication.spec.ts (1)
3-3: Good coverage and intent.Solid end-to-end coverage for SSR, CSR navigations, and inline/external scripts. Nice!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react-router/src/Asset.tsx (1)
139-139: Effect deps will cause teardown/re-exec; narrow to stable keys.
Depending onattrsobject identity will remove/recreate scripts on benign re-renders, re-executing inline code and reloading external scripts.- }, [attrs, children]) + }, [attrs?.src, attrs?.type, attrs?.nonce, children])If you also want re-runs on other attrs (
async,defer,crossOrigin,integrity,referrerPolicy), include them explicitly.
🧹 Nitpick comments (2)
packages/react-router/src/Asset.tsx (2)
52-66: Normalize against document.baseURI to honor tags.
Usedocument.baseURIas the base when resolving relativesrcso dedupe works with<base href>present.- return new URL(attrs.src, window.location.href).href + return new URL(attrs.src, document.baseURI).href
91-106: Harden inline-script matching with type guards for type/nonce.
attrs’ index signature allows booleans; ensure we only compare string values to avoid false mismatches.- const typeAttr = attrs?.type ?? 'text/javascript' - const nonceAttr = attrs?.nonce + const typeAttr = + typeof attrs?.type === 'string' ? attrs.type : 'text/javascript' + const nonceAttr = + typeof attrs?.nonce === 'string' ? attrs.nonce : undefined
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/src/Asset.tsx(4 hunks)packages/solid-router/src/Asset.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/solid-router/src/Asset.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-router/src/Asset.tsx (1)
packages/react-router/src/index.tsx (1)
useRouter(308-308)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (3)
packages/react-router/src/Asset.tsx (3)
2-2: Import order fix LGTM.
Value import before type-only import satisfies lint/CI.
48-49: Client/server gating hook usage looks correct.
UsinguseRouter()to branch SSR vs client behavior is appropriate here.
141-144: Verify hydration doesn’t double-execute scripts when returning null on client.
Returningnullon the client while SSR rendered a<script>can lead React to remove the SSR node before the effect runs, causing the effect to reinsert and re-execute. Please confirm with an SSR page whose inline script increments a counter that it runs exactly once on first load. If it double-executes, consider preserving the SSR node during the first client render (e.g., gatereturn nullbehind a “post-hydration” ref) or manage a registry to avoid reinsert on first mount.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/solid-router/src/Asset.tsx (2)
42-123: Make external-vs-inline injection mutually exclusiveIf both attrs.src and children are provided, the current logic can inject twice (external, then inline), defeating dedupe. Short-circuit after handling the external path; only run the inline path when no src.
onMount(() => { - if (attrs?.src) { + const hasSrc = typeof attrs?.src === 'string' && attrs.src.length > 0 + if (hasSrc) { const normSrc = (() => { try { - return new URL(attrs.src, window.location.href).href + const base = document.baseURI || window.location.href + return new URL(attrs!.src!, base).href } catch { - return attrs.src + return attrs!.src! } })() const existingScript = Array.from( document.querySelectorAll('script[src]'), ).find((el) => (el as HTMLScriptElement).src === normSrc) if (existingScript) { - return + return } const script = document.createElement('script') for (const [key, value] of Object.entries(attrs)) { if (value !== undefined && value !== false) { script.setAttribute( key, typeof value === 'boolean' ? '' : String(value), ) } } document.head.appendChild(script) onCleanup(() => { if (script.parentNode) { script.parentNode.removeChild(script) } }) - } + return + } - if (typeof children === 'string') { + if (!hasSrc && typeof children === 'string') { const typeAttr = typeof attrs?.type === 'string' ? attrs.type : 'text/javascript' const nonceAttr = typeof attrs?.nonce === 'string' ? attrs.nonce : undefined const existingScript = Array.from( document.querySelectorAll('script:not([src])'), ).find((el) => { if (!(el instanceof HTMLScriptElement)) return false const sType = el.getAttribute('type') ?? 'text/javascript' const sNonce = el.getAttribute('nonce') ?? undefined return ( el.textContent === children && sType === typeAttr && sNonce === nonceAttr ) }) if (existingScript) { return } const script = document.createElement('script') script.textContent = children if (attrs) { for (const [key, value] of Object.entries(attrs)) { + if (key === 'src') continue if (value !== undefined && value !== false) { script.setAttribute( key, typeof value === 'boolean' ? '' : String(value), ) } } } document.head.appendChild(script) onCleanup(() => { if (script.parentNode) { script.parentNode.removeChild(script) } }) } })
101-113: Inline path should not propagate src attributeWhen creating an inline script, copying attrs.src turns it into an external script, defeating the not([src]) dedupe and risking duplication. Skip src here (even if upstream passes it).
- if (attrs) { - for (const [key, value] of Object.entries(attrs)) { + if (attrs) { + for (const [key, value] of Object.entries(attrs)) { + if (key === 'src') continue if (value !== undefined && value !== false) { script.setAttribute( key, typeof value === 'boolean' ? '' : String(value), ) } } }
🧹 Nitpick comments (2)
packages/solid-router/src/Asset.tsx (2)
125-127: Client render guard: prefer compile-time isServer and confirm no hydration warningsUsing router.isServer couples this component to router context and may mask hydration mismatches if Asset ever renders inside a hydratable tree. Consider using isServer from solid-js/web (tree-shakable) and please confirm no hydration mismatch warnings in dev.
If you want, I can add a quick check to run the e2e/dev app and grep console output for hydration warnings.
61-68: Set nonce via property before insertion (CSP best practice)For CSP, browsers honor the nonce property; setting it pre-append is safest. This is additive and backward-compatible.
- for (const [key, value] of Object.entries(attrs)) { + for (const [key, value] of Object.entries(attrs)) { if (value !== undefined && value !== false) { - script.setAttribute( + if (key === 'nonce' && typeof value === 'string') { + // ensure CSP nonce is applied via property + (script as any).nonce = value + } + script.setAttribute( key, typeof value === 'boolean' ? '' : String(value), ) } }(Apply the same change in the inline branch.)
Also applies to: 105-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/src/Asset.tsx(4 hunks)packages/solid-router/src/Asset.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-router/src/Asset.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (1)
packages/solid-router/src/Asset.tsx (1)
51-57: LGTM on external dedupe approachAbsolute URL normalization + src equality check is a sound, low-cost dedupe.
packages/solid-router/src/Asset.tsx
Outdated
| const normSrc = (() => { | ||
| try { | ||
| return new URL(attrs.src, window.location.href).href | ||
| } catch { | ||
| return attrs.src | ||
| } | ||
| })() | ||
| const existingScript = Array.from( | ||
| document.querySelectorAll('script[src]'), | ||
| ).find((el) => (el as HTMLScriptElement).src === normSrc) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize src against document.baseURI to handle correctly
Using window.location.href ignores a document , which can break dedupe. Prefer document.baseURI (with a location fallback).
- const normSrc = (() => {
- try {
- return new URL(attrs.src, window.location.href).href
- } catch {
- return attrs.src
- }
- })()
+ const normSrc = (() => {
+ try {
+ const base = document.baseURI || window.location.href
+ return new URL(attrs.src, base).href
+ } catch {
+ return attrs.src
+ }
+ })()📝 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.
| const normSrc = (() => { | |
| try { | |
| return new URL(attrs.src, window.location.href).href | |
| } catch { | |
| return attrs.src | |
| } | |
| })() | |
| const existingScript = Array.from( | |
| document.querySelectorAll('script[src]'), | |
| ).find((el) => (el as HTMLScriptElement).src === normSrc) | |
| const normSrc = (() => { | |
| try { | |
| const base = document.baseURI || window.location.href | |
| return new URL(attrs.src, base).href | |
| } catch { | |
| return attrs.src | |
| } | |
| })() | |
| const existingScript = Array.from( | |
| document.querySelectorAll('script[src]'), | |
| ).find((el) => (el as HTMLScriptElement).src === normSrc) |
🤖 Prompt for AI Agents
In packages/solid-router/src/Asset.tsx around lines 44 to 54, the URL
normalization uses window.location.href which ignores a document <base> element
and can break script deduping; change the base used to document.baseURI with a
fallback to window.location.href (e.g. construct the URL with new URL(attrs.src,
document.baseURI || window.location.href).href inside the existing try/catch) so
normalization respects <base href> while preserving current fallback behavior.
fixes #4585
Summary by CodeRabbit
New Features
Bug Fixes
Tests