Conversation
Contributor
📝 WalkthroughWalkthroughAdds hydration-aware handling for user scripts: server renders script markup, client preserves markup pre-hydration and imperatively injects scripts post-hydration; includes E2E fixtures and updated tests to validate script execution and avoid hydration mismatches. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant Client
participant Hydration
participant ScriptInjection as Script (useEffect)
participant DOM
rect rgba(200, 150, 255, 0.5)
Note over Server,DOM: Server renders script markup
Server->>DOM: Render <script> (src or inline)
end
rect rgba(150, 200, 255, 0.5)
Note over Client,Hydration: Client pre-hydration rendering
Client->>Hydration: check hydrated?
Hydration-->>Client: No
Client->>DOM: Render matching <script> node (prevents mismatch)
Hydration->>Client: Yes (hydration complete)
Client->>ScriptInjection: useEffect runs
ScriptInjection->>DOM: Imperatively inject script into head (avoid React tree)
ScriptInjection->>Client: Component returns null post-hydration
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 12m 27s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 37s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-02-16 18:31:35 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/react-start/basic/tests/script-duplication.spec.ts`:
- Around line 35-45: The current page.evaluate uses a substring selector
'script[src*="script.js"]' which incorrectly matches root layout scripts; update
the selector used in the scriptCount evaluation to a more specific attribute
match (for example use an ends-with selector like 'script[src$="/script.js"]' or
an exact match 'script[src="script.js"]') so only the route-specific /script.js
is counted; keep the surrounding logic (the waitForFunction on (window as
any).SCRIPT_1 and the expect on scriptCount) but replace the selector in the
page.evaluate call that defines scriptCount.
🧹 Nitpick comments (2)
packages/react-router/tests/Scripts.test.tsx (1)
206-208: Minor: Use block body to avoid implicit return in forEach callback.The arrow function
(s) => s.remove()implicitly returns the result ofremove()(which isundefined). While harmless, using a block body makes intent clearer.Suggested fix
// Clear head and any leftover body scripts between tests. document.head.innerHTML = '' - document.querySelectorAll('body script').forEach((s) => s.remove()) + document.querySelectorAll('body script').forEach((s) => { + s.remove() + })e2e/react-start/basic/tests/root-scripts.spec.ts (1)
44-60: Consider clarifying the test description.The test verifies that root layout scripts (which executed on the initial
/postsload) remain active after client-side navigation to Home. The current name "should execute on client-side navigation" could be misread as testing that scripts execute during navigation rather than persisting across navigation.A clearer name might be:
"root layout scripts should remain executed after client-side navigation".This is a minor documentation concern and doesn't affect test correctness.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/react-start/basic/tests/script-duplication.spec.ts (1)
35-47: The SSR test no longer validates script non-duplication.The change from an exact count assertion to
toBeGreaterThanOrEqual(1)means this test now only verifies that the script exists and executes, but no longer catches script duplication. The client-side navigation tests (lines 59-72) still asserttoBe(1)for the same purpose.If React 19 hoisting makes exact counts unreliable for SSR, consider asserting
toBeLessThanOrEqual(1)(like the inline scripts test at line 130) to at least ensure no duplication, while still relying on the execution check for "at least once":Suggested alternative
- expect(scriptCount).toBeGreaterThanOrEqual(1) + // Script executed (checked via waitForFunction above), now verify no duplication + expect(scriptCount).toBeLessThanOrEqual(1)Alternatively, if scripts can legitimately appear multiple times in the DOM due to React 19 hoisting, the test name "should not create duplicate scripts" may need updating to reflect the actual invariant being tested.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests