Fix JSON parse race condition in immediate_hydration#2290
Fix JSON parse race condition in immediate_hydration#2290AbanoubGhadban merged 20 commits intomasterfrom
Conversation
WalkthroughThis PR addresses a non-deterministic JSON parsing race condition in React on Rails Pro's immediate hydration during HTML streaming. It introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
size-limit report 📦
|
Greptile OverviewGreptile SummaryThis PR fixes a critical race condition in React on Rails Pro's The ProblemWhen The SolutionThe fix filters immediate hydration elements to only process those with <script class="js-react-on-rails-component">{"props":...}</script>
<script>ReactOnRails.reactOnRailsComponentLoaded('id');</script>If Implementation QualityThe fix is elegant and minimal:
Test Coverage
The fix correctly handles edge cases including multiple components, delayed registration, and double-hydration prevention through the existing Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Browser
participant Server
participant JSBundle as JS Bundle
participant DOM
Note over Server,Browser: HTML Streaming with Race Condition
Server->>Browser: Chunk 1: <head><script src="bundle.js" async>
Browser->>Browser: Start downloading bundle.js
Server->>Browser: Chunk 2: <body><script class="js-react-on-rails-component">{"props":[
Browser->>DOM: Create props script element (opening tag parsed)
Note over DOM: element.nextSibling = null<br/>(closing tag not yet received)
JSBundle->>JSBundle: bundle.js download completes
JSBundle->>JSBundle: Execute: renderOrHydrateImmediateHydratedComponents()
JSBundle->>DOM: querySelectorAll('[data-immediate-hydration="true"]')
DOM->>JSBundle: Returns props script element
JSBundle->>JSBundle: Filter: el.nextSibling !== null
Note over JSBundle: nextSibling is null → SKIP<br/>(FIX: prevents reading incomplete JSON)
Server->>Browser: Chunk 3: ...more JSON...]}</script>
Browser->>DOM: Complete props script element
Note over DOM: Closing tag parsed
Server->>Browser: Chunk 4: <script>ReactOnRails.reactOnRailsComponentLoaded('id')</script>
Browser->>DOM: Create immediate hydration script
Note over DOM: Now element.nextSibling exists
Browser->>JSBundle: Execute inline script
JSBundle->>JSBundle: reactOnRailsComponentLoaded('id')
JSBundle->>DOM: Read el.textContent (now complete)
JSBundle->>JSBundle: JSON.parse(textContent) ✓ Success
JSBundle->>Browser: React hydration succeeds
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @analysis/json-race-condition-demo/demo.html:
- Around line 46-59: The retry loop in tryParseProps currently calls
setTimeout(tryParseProps, 5) with no limit and can poll forever; modify
tryParseProps to bound retries by either tracking an attempts counter or
comparing performance.now() against a max deadline (e.g., startTime + MAX_MS),
stop retrying when the limit is reached, and update resultEl with a clear
failure/time-out message; reference the tryParseProps function, the
scriptEl/resultEl checks and the existing setTimeout call to implement the
bounded retry and ensure you clear any pending retries when success or timeout
occurs.
In @analysis/json-race-condition-demo/minimal-demo.rb:
- Around line 18-33: The header-draining loop currently calls client.gets twice
per iteration and never handles nil, so replace the loop with a single-read loop
that assigns each line to a variable (e.g., line = client.gets), breaks when
line is nil or line == "\r\n", and handles early disconnects gracefully; also
move the send_chunk method definition out of the per-connection loop (define
send_chunk(client, data) at file scope) so it isn't redefined for every
connection and RuboCop offenses are avoided.
In @analysis/json-race-condition-demo/README.md:
- Around line 13-16: The fenced code blocks in README.md lack language
identifiers (causing markdownlint MD040); update the blocks that show the JSON
error message, the "Server streams HTML..." snippet, and the bash command
example by adding appropriate language tags (e.g., use "text" for plain
output/error blocks and "bash" for the shell command block) so all fenced blocks
(including those around the JSON SyntaxError, the chunked HTML example, and the
pnpm command shown in the diff and the sections noted at lines ~58-78 and
~83-86) include the language specifier.
- Around line 5-6: Replace the bare URL in the README heading line (“**Issue:**
https://github.com/shakacode/react_on_rails/issues/2283”) with a markdown link
to satisfy MD034, e.g. change it to something like "**Issue:**
[shakacode/react_on_rails#2283](https://github.com/shakacode/react_on_rails/issues/2283)"
so the URL is not bare but still points to the same issue.
In @react_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.ts:
- Line 262: The test uses a different data-testid selector
('[data-testid="status"]') than the main suite ('[data-testid="test-status"]'),
causing inconsistency; decide which element is intended and make them
consistent: if both should check the same render-complete element, change the
selector in large_props_stress_test.spec.ts from '[data-testid="status"]' to
'[data-testid="test-status"]' where await page.waitForSelector is called, or if
they are intentionally different, document the difference with a comment and
keep both selectors as-is to avoid accidental test coupling.
- Around line 251-255: Add an inline comment above the page.on('console')
handler explaining that filtering out messages containing "immediate_hydration"
is intentional: these errors stem from a known race condition (issue #2283) that
occurs during rapid sequential HTML streaming and are expected side-effects of
the stress test rather than actual failures; mention the specific handler
(page.on('console', (msg: ConsoleMessage) => { ... })) and the filter check
(!msg.text().includes('immediate_hydration')) so future readers know why those
errors are ignored.
In @react_on_rails_pro/spec/dummy/package.json:
- Line 91: The @webpack-cli/serve dependency in package.json is pinned to an
older 1.x line incompatible with webpack-cli 6.x; update the package.json
dependency entry for "@webpack-cli/serve" to use the 3.x range (e.g., "^3.0.0")
so it matches webpack-cli 6.0.1 requirements and rerun npm/yarn install to
verify compatibility.
🧹 Nitpick comments (15)
analysis/json-race-condition-demo/server.rb (1)
1-111: Demo file serves as educational tool but has limitations.This demo places the parsing script before the JSON script tag (lines 53-90), which is a different scenario from the actual bug. The real race condition occurs when:
- An async bundle in
<head>loads and executes- The JSON script tag in
<body>is still streamingSince WEBrick sends the full response synchronously (no chunked streaming), this demo may not reliably reproduce the actual race condition. The
server-v2.rbwith chunked transfer encoding is more accurate.Consider adding a note in the header comment clarifying this limitation and pointing to
server-v2.rbfor a more accurate reproduction.react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb (1)
101-109: Remove or gate debugputsstatements.These
putsstatements on lines 101 and 109 will output to stdout in test environments. Consider removing them or gating behind a debug flag/environment variable to keep test output clean.♻️ Suggested fix
- puts "Before props part #{props_script[0...split_point]}" # Send first half (contains opening tag + truncated JSON) yield before + props_script[0...split_point] # DELAY - This is where the race condition happens! # JS bundle can load and execute during this window, reading incomplete props sleep RACE_CONDITION_DELAY - puts "After props part #{props_script[split_point..]}" # Send second half (rest of JSON + closing tag) + any content afteranalysis/json-race-condition-demo/demo.html (2)
1-32: Add basic HTML metadata for easier standalone use (charset/viewport).
This is a demo page, but adding<meta charset>/<meta name="viewport">makes it more robust when opened directly and on mobile.Proposed diff
<head> + <meta charset="utf-8"> + <meta name="viewport" content="width=device-width, initial-scale=1"> <title>JSON Race Condition Demo (Static)</title> <style>
60-88: Avoid logging full props payloads; render errors withoutinnerHTMLinterpolation.
On real “large props” cases,console.log('Full textContent', ...)can overwhelm DevTools, and interpolatinge.messageintoinnerHTMLis avoidable.Proposed diff
} catch (e) { // THIS IS THE BUG! resultEl.innerHTML = '<span class="error">✗ JSON PARSE ERROR</span><br>' + - '<strong>Error:</strong> ' + e.message + '<br>' + + '<strong>Error:</strong> ' + escapeHtml(String(e && e.message)) + '<br>' + '<strong>textContent length:</strong> ' + textContent.length + ' bytes<br>' + '<strong>Time:</strong> ' + elapsed + 'ms<br><br>' + '<strong>Last 200 characters:</strong><br>' + '<pre>' + escapeHtml(textContent.slice(-200)) + '</pre>' + '<p><em>Notice how the JSON is truncated mid-content!</em></p>'; console.error('JSON Parse Error:', e); - console.log('Full textContent:', textContent); + console.log('Last 500 chars of textContent:', textContent.slice(-500)); } } function escapeHtml(text) { - return text.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>'); + return text + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); }react_on_rails_pro/spec/dummy/config/application.rb (1)
21-27: Make “after Rack::Deflater” ordering explicit; consider env gating.
Usinginsert_afteravoids accidental reordering later, and gating keeps the dummy middleware stack clean outside dev/test.
As per coding guidelines, runbundle exec rubocopto ensure zero offenses.Proposed diff
config.load_defaults 7.0 config.middleware.use Rack::Deflater # StreamingRaceSimulator must be added AFTER Rack::Deflater so it processes # the uncompressed response. Activated by adding ?simulate_race=true to URLs. - config.middleware.use StreamingRaceSimulator + if Rails.env.development? || Rails.env.test? + config.middleware.insert_after Rack::Deflater, StreamingRaceSimulator + endreact_on_rails_pro/spec/dummy/config/routes.rb (1)
80-83: Consider scoping the stress-test route to dev/test only.
If this is purely for reproduction/E2E, gating avoids accidental use in other dummy runs.
As per coding guidelines, runbundle exec rubocopto ensure zero offenses.Proposed diff
# Large props stress test for reproducing JSON parsing race condition # https://github.com/shakacode/react_on_rails/issues/2283 - get "large_props_stress_test" => "pages#large_props_stress_test" + get "large_props_stress_test" => "pages#large_props_stress_test" if Rails.env.development? || Rails.env.test?analysis/json-race-condition-demo/README.md (1)
88-95: Align “The Fix” section with the implemented heuristic (closing tag parsed / sibling present).
Since this PR’s fix is essentially “only parse when the JSON script element is complete,” it’d help to explicitly mention the sibling-based completeness check here.analysis/json-race-condition-demo/minimal-demo.rb (1)
12-16: Prefer binding to localhost for a local repro server.
Binding to0.0.0.0exposes the demo on the network;127.0.0.1is typically enough.Proposed diff
-server = TCPServer.new('0.0.0.0', PORT) +server = TCPServer.new('127.0.0.1', PORT) puts "Minimal DOM streaming demo: http://localhost:#{PORT}"react_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsx (2)
51-53: Consider memoizing the expensive JSON.stringify calculation.Computing
propsSizeviaJSON.stringify(largeData)on every render is expensive for ~200KB payloads. SincelargeDatais a prop and unlikely to change reference frequently, consider usinguseMemo:♻️ Memoize propsSize calculation
+import React, { useState, useEffect, useMemo } from 'react'; -import React, { useState, useEffect } from 'react'; import PropTypes from 'prop-types'; const DelayedLargePropsComponent = ({ largeData, componentId, loadTime, registrationDelay }) => { const [renderTime] = useState(() => new Date().toISOString()); const [propsValid, setPropsValid] = useState(false); // ... useEffect validation ... - // Calculate props size - const propsSize = largeData ? JSON.stringify(largeData).length : 0; + // Calculate props size (memoized) + const propsSize = useMemo(() => (largeData ? JSON.stringify(largeData).length : 0), [largeData]); const itemCount = largeData?.items?.length || 0;
1-84: Significant code duplication with LargePropsComponent.
DelayedLargePropsComponentandLargePropsComponentshare ~95% identical logic—only the border style and test IDs differ. For test infrastructure this may be acceptable, but consider whether a shared base component with configuration props would improve maintainability.react_on_rails_pro/spec/dummy/client/app/components/LargePropsComponent.jsx (1)
43-45: Consider memoizing the expensive JSON.stringify calculation.Computing
propsSizeviaJSON.stringify(largeData)on every render is expensive for ~200KB payloads. SincelargeDatais a prop and unlikely to change reference frequently, consider usinguseMemo:♻️ Memoize propsSize calculation
+import React, { useState, useEffect, useMemo } from 'react'; -import React, { useState, useEffect } from 'react'; import PropTypes from 'prop-types'; const LargePropsComponent = ({ largeData, componentId, loadTime, registrationDelay }) => { const [renderTime] = useState(() => new Date().toISOString()); const [propsValid, setPropsValid] = useState(false); // ... useEffect validation ... - // Calculate props size - const propsSize = largeData ? JSON.stringify(largeData).length : 0; + // Calculate props size (memoized) + const propsSize = useMemo(() => (largeData ? JSON.stringify(largeData).length : 0), [largeData]); const itemCount = largeData?.items?.length || 0;react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb (1)
226-276: Consider performance implications of generating 500 items per request.Each call to
generate_large_propscreates 500 items with nested structures. For test/demo infrastructure this is acceptable, but be aware this could add ~100-200ms latency per request depending on Ruby version and server resources.react_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erb (2)
14-43: Consider resource cleanup and configurability for the JSON.parse monkey-patch.The monkey-patch implementation has a few considerations:
No restoration mechanism: The original
JSON.parseis stored but never restored. If this page is used in a SPA or navigated away from, the monkey-patch persists globally.Hardcoded threshold: The 10KB threshold (line 28) for error logging is hardcoded. Consider making it configurable via a Rails variable similar to
@registration_delay.Error details exposure: Logging
firstCharsandlastCharscould potentially expose sensitive data. In a test environment this is acceptable, but document this assumption.♻️ Optional improvements
Add cleanup and make threshold configurable:
<script> // Configure the delay for component registration (simulates async bundle loading) window.__DELAYED_COMPONENT_REGISTRATION_DELAY__ = <%= @registration_delay %>; + window.__JSON_PARSE_ERROR_THRESHOLD__ = <%= @json_parse_threshold || 10000 %>; // Track any JSON parse errors window.__JSON_PARSE_ERRORS__ = []; // Monkey-patch JSON.parse to detect errors (function() { const originalParse = JSON.parse; + window.__ORIGINAL_JSON_PARSE__ = originalParse; JSON.parse = function(text, reviver) { try { return originalParse.call(this, text, reviver); } catch (e) { - if (text && text.length > 10000) { + if (text && text.length > window.__JSON_PARSE_ERROR_THRESHOLD__) { const errorInfo = { error: e.message, textLength: text.length, firstChars: text.slice(0, 200), lastChars: text.slice(-200), timestamp: new Date().toISOString() }; window.__JSON_PARSE_ERRORS__.push(errorInfo); console.error('[JSON Parse Error Detected]', errorInfo); } throw e; } }; + + // Restore on page unload + window.addEventListener('beforeunload', function() { + if (window.__ORIGINAL_JSON_PARSE__) { + JSON.parse = window.__ORIGINAL_JSON_PARSE__; + } + }); })(); </script>
47-47: Add nil checks for instance variables.The view assumes
@large_props_first(line 47) and@total_props_size(line 93) are set by the controller. If these are nil, the view will raise an error. Consider adding defensive nil checks or documenting the required controller setup.♻️ Suggested defensive nil handling
<p> - Testing JSON parsing with large props (~<%= (@large_props_first.to_json.length / 1024.0).round(2) %> KB per component) + Testing JSON parsing with large props (~<%= @large_props_first ? (@large_props_first.to_json.length / 1024.0).round(2) : 0 %> KB per component) </p><pre id="test-info"> -Total props size: <%= (@total_props_size / 1024.0).round(2) %> KB +Total props size: <%= @total_props_size ? (@total_props_size / 1024.0).round(2) : 0 %> KB Components: 5 (1 immediate, 1 delayed, 3 multi) Registration delay: <%= @registration_delay %>ms Timestamp: <%= Time.now.iso8601 %> </pre>Also applies to: 93-93
react_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.ts (1)
90-94: Consider runtime validation for JSON parse errors structure.The type assertion at line 92 assumes
window.__JSON_PARSE_ERRORS__matches the expected structure. While this is acceptable for test code that controls the page being tested, adding runtime validation would make debugging easier if the monkey-patch implementation changes.♻️ Add optional runtime validation
// Check for JSON parse errors captured by our monkey-patch const jsonErrors = await page.evaluate( () => - (window as unknown as { __JSON_PARSE_ERRORS__?: LoadResult['jsonErrors'] }).__JSON_PARSE_ERRORS__ || - [], + { + const errors = (window as unknown as { __JSON_PARSE_ERRORS__?: unknown }).__JSON_PARSE_ERRORS__; + if (!errors) return []; + if (!Array.isArray(errors)) { + console.warn('__JSON_PARSE_ERRORS__ is not an array:', errors); + return []; + } + return errors; + } );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
react_on_rails_pro/spec/dummy/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
analysis/json-race-condition-demo/README.mdanalysis/json-race-condition-demo/browser-proof.htmlanalysis/json-race-condition-demo/demo.htmlanalysis/json-race-condition-demo/minimal-demo.rbanalysis/json-race-condition-demo/server-v2.rbanalysis/json-race-condition-demo/server.rbanalysis/react-on-rails-pro+16.2.0-rc.0.patchpackages/react-on-rails-pro/src/ClientSideRenderer.tsreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erbreact_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsxreact_on_rails_pro/spec/dummy/client/app/components/LargePropsComponent.jsxreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.jsreact_on_rails_pro/spec/dummy/config/application.rbreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/config/shakapacker.ymlreact_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.tsreact_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.tsreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/package.json
💤 Files with no reviewable changes (1)
- react_on_rails_pro/spec/dummy/config/shakapacker.yml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rb: Runbundle exec rubocopand fix ALL violations before every commit/push
Runbundle exec rubocop(MANDATORY) before every commit to ensure zero offenses
Files:
react_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/config/application.rbreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbanalysis/json-race-condition-demo/minimal-demo.rbanalysis/json-race-condition-demo/server.rbanalysis/json-race-condition-demo/server-v2.rb
🧠 Learnings (25)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.spec.js : Use Playwright E2E tests in `react_on_rails/spec/dummy/e2e/playwright/` for React component integration testing. Tests automatically start Rails server on port 5017 before running
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsxreact_on_rails_pro/spec/dummy/client/app/components/LargePropsComponent.jsxanalysis/json-race-condition-demo/README.mdpackages/react-on-rails-pro/src/ClientSideRenderer.tsreact_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.tsreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erbreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.jsanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
react_on_rails_pro/spec/dummy/package.json
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsxreact_on_rails_pro/spec/dummy/client/app/components/LargePropsComponent.jsxpackages/react-on-rails-pro/src/ClientSideRenderer.tsreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erbreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.jsanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsxreact_on_rails_pro/spec/dummy/config/application.rbreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.jsanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Applied to files:
react_on_rails_pro/spec/dummy/package.json
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsxreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.jsanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsxanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.spec.js : Use Playwright E2E tests in `react_on_rails/spec/dummy/e2e/playwright/` for React component integration testing. Tests automatically start Rails server on port 5017 before running
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsxanalysis/json-race-condition-demo/README.mdreact_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.tsreact_on_rails_pro/spec/dummy/config/application.rbreact_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.tsreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erbreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.jsanalysis/json-race-condition-demo/server.rbanalysis/json-race-condition-demo/server-v2.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: In IDE configuration, exclude these directories to prevent slowdowns: /coverage, /tmp, /gen-examples, /packages/react-on-rails/lib, /node_modules, /react_on_rails/spec/dummy/node_modules, /react_on_rails/spec/dummy/tmp, /react_on_rails/spec/dummy/app/assets/webpack, /react_on_rails/spec/dummy/log, /react_on_rails/spec/dummy/e2e/playwright-report, /react_on_rails/spec/dummy/test-results
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsxreact_on_rails_pro/spec/dummy/config/application.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Shakapacker configuration should be tested by creating debug scripts in dummy app root (debug-webpack-rules.js, debug-webpack-with-config.js) to inspect webpack rules before committing configuration changes
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.js
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/app_commands/**/*.rb : Create custom Rails app commands in `e2e/playwright/app_commands/` directory with .rb files for reusable helpers in Playwright tests
Applied to files:
react_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.tsreact_on_rails_pro/spec/dummy/config/application.rbreact_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.tsreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erbreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.js
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Applied to files:
react_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/config/application.rbreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.js
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbanalysis/json-race-condition-demo/README.mdanalysis/json-race-condition-demo/demo.htmlreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbanalysis/json-race-condition-demo/server.rbanalysis/json-race-condition-demo/server-v2.rbanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erbanalysis/json-race-condition-demo/server-v2.rbanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsxreact_on_rails_pro/spec/dummy/client/app/components/LargePropsComponent.jsxanalysis/json-race-condition-demo/README.mdpackages/react-on-rails-pro/src/ClientSideRenderer.tsanalysis/json-race-condition-demo/demo.htmlreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erbreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.jsanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.spec.js : Use `app('clean')`, `appEval(code)`, `appFactories(options)`, and `appScenario(name)` helpers from cypress-on-rails in Playwright tests for Rails integration
Applied to files:
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.tsreact_on_rails_pro/spec/dummy/config/application.rbreact_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
react_on_rails_pro/spec/dummy/client/app/components/LargePropsComponent.jsxanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
react_on_rails_pro/spec/dummy/client/app/components/LargePropsComponent.jsxanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
analysis/json-race-condition-demo/README.mdreact_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.jsanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
analysis/json-race-condition-demo/README.md
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
analysis/json-race-condition-demo/README.mdreact_on_rails_pro/spec/dummy/config/application.rbreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Install Playwright browsers with `cd react_on_rails/spec/dummy && pnpm playwright install --with-deps` before running E2E tests
Applied to files:
react_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erbanalysis/react-on-rails-pro+16.2.0-rc.0.patch
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
analysis/react-on-rails-pro+16.2.0-rc.0.patch
🧬 Code graph analysis (7)
react_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsx (2)
react_on_rails_pro/spec/dummy/client/app/components/LargePropsComponent.jsx (2)
renderTime(14-14)propsValid(15-15)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/SetTimeoutLoggingApp.client.jsx (1)
div(5-7)
react_on_rails_pro/spec/dummy/client/app/components/LargePropsComponent.jsx (1)
react_on_rails_pro/spec/dummy/client/app/components/DelayedLargePropsComponent.jsx (4)
renderTime(22-22)propsValid(23-23)propsSize(52-52)itemCount(53-53)
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb (1)
react_on_rails_pro/lib/react_on_rails_pro/request.rb (1)
new(8-332)
react_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.js (1)
packages/react-on-rails/src/types/index.ts (1)
ReactOnRails(280-348)
analysis/json-race-condition-demo/minimal-demo.rb (1)
analysis/json-race-condition-demo/server-v2.rb (1)
send_chunk(121-124)
analysis/json-race-condition-demo/server.rb (1)
analysis/json-race-condition-demo/server-v2.rb (1)
generate_large_json(18-31)
analysis/json-race-condition-demo/server-v2.rb (4)
analysis/json-race-condition-demo/server.rb (1)
generate_large_json(10-23)react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb (1)
close(123-125)analysis/json-race-condition-demo/minimal-demo.rb (1)
send_chunk(30-33)react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb (1)
data(209-224)
🪛 markdownlint-cli2 (0.18.1)
analysis/json-race-condition-demo/README.md
5-5: Bare URL used
(MD034, no-bare-urls)
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (11)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: Greptile Review
- GitHub Check: examples (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: precompile-check
- GitHub Check: check-bundle-size
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: markdown-link-check
🔇 Additional comments (20)
packages/react-on-rails-pro/src/ClientSideRenderer.ts (3)
275-295: Well-documented fix for the streaming race condition.The
forAllCompleteElementsAsynchelper elegantly addresses the race condition by checkingel.nextSibling !== null. The comment clearly explains the rationale: presence of a nextSibling (the immediate hydration script) indicates the props script's closing tag was parsed and content is complete.This is a solid approach that avoids the race without adding complex timing logic or polling.
297-301: LGTM!Correctly uses
forAllCompleteElementsAsyncto ensure only fully-parsed component elements are processed during immediate hydration.
335-339: LGTM!Consistent application of the fix to store hydration, ensuring both components and stores are protected from the streaming race condition.
analysis/json-race-condition-demo/browser-proof.html (1)
1-126: Useful educational demo for understanding the race condition.The page effectively explains the streaming race condition concept. Test 1 with
document.writein an iframe is a good approach to demonstrate incremental DOM parsing.Note that Test 2 (lines 64-90) doesn't actually demonstrate streaming since
innerHTMLassignment is atomic—the comment on lines 76-78 acknowledges this limitation appropriately.react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb (1)
56-68: LGTM - Well-designed test infrastructure.The
DelayedPropsBodyclass effectively simulates the streaming race condition by:
- Buffering until the complete props script is found
- Splitting the JSON content in the middle
- Introducing a delay between halves
The regex pattern and 1-second delay are appropriate for test purposes. Good defensive coding with the
closemethod handling.analysis/json-race-condition-demo/server-v2.rb (2)
1-211: Accurate reproduction of the race condition with chunked streaming.This demo correctly models the React on Rails architecture:
- Async script in
<head>(line 141)- JSON props streaming in chunked pieces (lines 174-181)
- Realistic timing with 150ms delays between chunks
The use of raw TCP sockets with chunked transfer encoding (
Transfer-Encoding: chunked) provides an accurate simulation that doesn't depend on network throttling.
17-31: Minor duplication with server.rb is acceptable for demo files.The
generate_large_jsonfunction is similar toserver.rbbut with slightly different string content. For standalone demo files inanalysis/, this duplication is fine—it keeps each demo self-contained and independently runnable.analysis/react-on-rails-pro+16.2.0-rc.0.patch (1)
1-108: This is an analysis/reference document—the patch has not been applied to the codebase.The patch in
analysis/documents the diagnostic investigation approach and is correctly placed for reference. The comprehensive diagnostic function captures useful debugging signals: content slices around error position, truncation detection, HTML entity detection, and null byte detection.However, if this diagnostic approach is ever implemented in production or merged as an actual fix, be aware that the logging implementation outputs large content samples and full diagnostics objects. This could leak sensitive props data in logs. Consider:
- Gating verbose content logging behind a debug flag
- Redacting or truncating sensitive fields before logging
- Limiting diagnostic detail in production logs while preserving error tracking capability
analysis/json-race-condition-demo/demo.html (1)
100-113: Inline JSON sample looks fine for the demo.
The<script type="application/json">sample content is readable and aligns with the reproduction narrative.react_on_rails_pro/spec/dummy/config/application.rb (1)
11-13: Require is fine; ensure this stays dummy/test-only.
Given this is underspec/dummy, just ensure nothing outside dummy loads this file in non-test contexts.react_on_rails_pro/spec/dummy/client/app/packs/large-props-bundle.js (1)
19-21: LGTM! Window global access is safe in this browser-only bundle.The window global access with fallback (
|| 100) is appropriate for this test bundle that only runs client-side. The pattern allows test pages to configure the delay while providing a sensible default.react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb (2)
245-260: XSS test payload included—ensure this is intentional.Line 257 includes
<script>alert('xss')</script>in thespecial_charsfield, which is appropriate for testing escaping and XSS prevention. The React on Rails escapeScript function (per learnings) should handle this correctly when rendering props.Based on learnings, RSC payloads are already escaped by the escapeScript function which handles script tags and special HTML markers.
174-188: LGTM! Well-structured stress test action.The
large_props_stress_testaction is clearly organized and generates appropriate test data for reproducing the race condition with ~200KB payloads.As per coding guidelines, please ensure
bundle exec rubocoppasses with zero offenses before committing:#!/bin/bash # Verify Ruby code style compliance cd react_on_rails_pro bundle exec rubocop spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts (3)
31-79: Excellent event listener cleanup pattern.The
waitForHydrationOrErrorimplementation properly manages event listeners with acleanupfunction (lines 36-39) to prevent memory leaks, and includes a sensible 10-second timeout fallback.
85-101: Good interactivity verification approach.The
verifyComponentIsInteractivefunction validates actual hydration by testing user interaction (typing updates the heading), which is more reliable than checking for the presence of event handlers or internal React state.
103-137: Comprehensive race condition test coverage.Both test cases properly validate:
- Successful hydration even under simulated race conditions (line 113)
- Absence of JSON parse errors (lines 114, 131)
- Component interactivity post-hydration (lines 117-118, 134-135)
This provides deterministic verification of the fix described in issue #2283.
Based on learnings, Playwright E2E tests in
spec/dummy/e2e/automatically start Rails server on port 5017 before running.react_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.ts (3)
193-209: LGTM - Appropriate resource management for stress testing.The concurrent load function properly creates isolated browser contexts for each load and ensures cleanup via
.finally(). While creating many contexts (up to 250 in the full stress test) is resource-intensive, this is appropriate for a stress test designed to reproduce race conditions.
177-187: Correct test strictness for JSON parse errors.The test correctly distinguishes between general test flakiness and the specific JSON parse error bug. While the comment at line 177 mentions allowing some general flakiness, the assertion at line 187 correctly requires zero JSON parse errors, since those are the exact bug this PR fixes.
68-68: Thedelayquery parameter is already properly handled by the controller.The controller correctly reads
params[:delay]and sets@registration_delay(line 178:@registration_delay = (params[:delay] || 100).to_i), which is then used throughout the view. However, theload_idparameter passed by the test (line 68) is not used anywhere in the controller or view—verify whether this parameter should be implemented for tracking purposes or removed from the test if it's no longer needed.Likely an incorrect or invalid review comment.
react_on_rails_pro/spec/dummy/app/views/pages/large_props_stress_test.html.erb (1)
103-104: Selectors are correctly implemented and match component output.The React components render
data-testidattributes that precisely match the selector patterns in the stress test. LargePropsComponent rendersdata-testid="large-props-component-${componentId}"anddata-testid="status", while DelayedLargePropsComponent rendersdata-testid="delayed-large-props-component-${componentId}"anddata-testid="delayed-status". The polling logic correctly expects 5 status elements (1 immediate, 1 delayed, 3 multi-component) and will find them.
| <script> | ||
| (function() { | ||
| const startTime = performance.now(); | ||
|
|
||
| function tryParseProps() { | ||
| const scriptEl = document.getElementById('js-react-on-rails-component-MyComponent-0'); | ||
| const resultEl = document.getElementById('result'); | ||
|
|
||
| if (!scriptEl) { | ||
| resultEl.innerHTML = '<span class="warning">⏳ JSON script tag not found yet...</span>'; | ||
| setTimeout(tryParseProps, 5); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Bound the 5ms retry loop to avoid infinite polling.
As written, setTimeout(tryParseProps, 5) can run forever if the element never appears (or appears much later), which is noisy in demos and can skew timings.
Proposed diff
<script>
(function() {
const startTime = performance.now();
+ const maxWaitMs = 5000;
function tryParseProps() {
const scriptEl = document.getElementById('js-react-on-rails-component-MyComponent-0');
const resultEl = document.getElementById('result');
if (!scriptEl) {
+ if (performance.now() - startTime > maxWaitMs) {
+ resultEl.innerHTML = '<span class="error">✗ Timed out waiting for JSON script tag</span>';
+ return;
+ }
resultEl.innerHTML = '<span class="warning">⏳ JSON script tag not found yet...</span>';
setTimeout(tryParseProps, 5);
return;
}📝 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.
| <script> | |
| (function() { | |
| const startTime = performance.now(); | |
| function tryParseProps() { | |
| const scriptEl = document.getElementById('js-react-on-rails-component-MyComponent-0'); | |
| const resultEl = document.getElementById('result'); | |
| if (!scriptEl) { | |
| resultEl.innerHTML = '<span class="warning">⏳ JSON script tag not found yet...</span>'; | |
| setTimeout(tryParseProps, 5); | |
| return; | |
| } | |
| <script> | |
| (function() { | |
| const startTime = performance.now(); | |
| const maxWaitMs = 5000; | |
| function tryParseProps() { | |
| const scriptEl = document.getElementById('js-react-on-rails-component-MyComponent-0'); | |
| const resultEl = document.getElementById('result'); | |
| if (!scriptEl) { | |
| if (performance.now() - startTime > maxWaitMs) { | |
| resultEl.innerHTML = '<span class="error">✗ Timed out waiting for JSON script tag</span>'; | |
| return; | |
| } | |
| resultEl.innerHTML = '<span class="warning">⏳ JSON script tag not found yet...</span>'; | |
| setTimeout(tryParseProps, 5); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In @analysis/json-race-condition-demo/demo.html around lines 46 - 59, The retry
loop in tryParseProps currently calls setTimeout(tryParseProps, 5) with no limit
and can poll forever; modify tryParseProps to bound retries by either tracking
an attempts counter or comparing performance.now() against a max deadline (e.g.,
startTime + MAX_MS), stop retrying when the limit is reached, and update
resultEl with a clear failure/time-out message; reference the tryParseProps
function, the scriptEl/resultEl checks and the existing setTimeout call to
implement the bounded retry and ensure you clear any pending retries when
success or timeout occurs.
There was a problem hiding this comment.
This file has been removed in commit 869a22e. The analysis/json-race-condition-demo/ directory contained investigation files that are no longer needed now that the fix is complete.
There was a problem hiding this comment.
@AbanoubGhadban, thank you for the clarification! Since the investigation files in analysis/json-race-condition-demo/ have been removed, my review comment on this file is no longer relevant. Makes sense to clean up the analysis artifacts once the fix is in place. 👍
🧠 Learnings used
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
| **Issue:** https://github.com/shakacode/react_on_rails/issues/2283 | ||
|
|
There was a problem hiding this comment.
Fix markdownlint MD034: replace bare URL with a link.
Proposed diff
-**Issue:** https://github.com/shakacode/react_on_rails/issues/2283
+**Issue:** [#2283](https://github.com/shakacode/react_on_rails/issues/2283)📝 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.
| **Issue:** https://github.com/shakacode/react_on_rails/issues/2283 | |
| **Issue:** [#2283](https://github.com/shakacode/react_on_rails/issues/2283) | |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
5-5: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In @analysis/json-race-condition-demo/README.md around lines 5 - 6, Replace the
bare URL in the README heading line (“**Issue:**
https://github.com/shakacode/react_on_rails/issues/2283”) with a markdown link
to satisfy MD034, e.g. change it to something like "**Issue:**
[shakacode/react_on_rails#2283](https://github.com/shakacode/react_on_rails/issues/2283)"
so the URL is not bare but still points to the same issue.
There was a problem hiding this comment.
This file has been removed in commit 869a22e. The investigation files are no longer needed.
| ``` | ||
| SyntaxError: Unterminated string in JSON at position XXXXX | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Fix markdownlint MD040: add languages to fenced code blocks.
This improves readability and satisfies the lint hints you already have.
Proposed diff
-```
+```text
SyntaxError: Unterminated string in JSON at position XXXXX
```diff
-```
+```text
Server streams HTML via chunked transfer encoding:
...
```diff
-```bash
+```bash
cd react_on_rails_pro/spec/dummy
pnpm e2e-test e2e-tests/large_props_stress_test.spec.ts -g "EXTREME"
</details>
Also applies to: 58-78, 83-86
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @analysis/json-race-condition-demo/README.md around lines 13 - 16, The fenced
code blocks in README.md lack language identifiers (causing markdownlint MD040);
update the blocks that show the JSON error message, the "Server streams HTML..."
snippet, and the bash command example by adding appropriate language tags (e.g.,
use "text" for plain output/error blocks and "bash" for the shell command block)
so all fenced blocks (including those around the JSON SyntaxError, the chunked
HTML example, and the pnpm command shown in the diff and the sections noted at
lines ~58-78 and ~83-86) include the language specifier.
</details>
<!-- This is an auto-generated reply by CodeRabbit -->
There was a problem hiding this comment.
This file has been removed in commit 869a22e. The investigation files are no longer needed.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| page.on('console', (msg: ConsoleMessage) => { | ||
| if (msg.type() === 'error' && !msg.text().includes('immediate_hydration')) { | ||
| errors.push(msg.text()); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for documentation or comments about immediate_hydration errors
echo "=== Checking for immediate_hydration references in test files ==="
rg -n 'immediate_hydration' react_on_rails_pro/spec/dummy/e2e-tests/ -A 3 -B 3
echo -e "\n=== Checking for immediate_hydration in source code ==="
rg -n 'immediate_hydration' react_on_rails_pro/node_package/ -A 2 -B 2
echo -e "\n=== Checking for error handling comments around line 251-255 ==="
sed -n '240,260p' react_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.tsRepository: shakacode/react_on_rails
Length of output: 2496
🏁 Script executed:
#!/bin/bash
# Get more context about json_parse_race_condition test and the issue
echo "=== Full json_parse_race_condition.spec.ts test file ==="
cat react_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts
echo -e "\n=== Search for issue 2283 references ==="
rg -n '2283' react_on_rails_pro/
echo -e "\n=== Search for comments around the error filtering in large_props_stress_test ==="
sed -n '235,270p' react_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.tsRepository: shakacode/react_on_rails
Length of output: 7362
Add a comment explaining the immediate_hydration error filter.
The filter at line 252 is intentional—it excludes errors related to issue #2283 (a known race condition during rapid sequential HTML streaming). However, this context is not documented in the test code itself. Add a comment above the page.on('console') handler explaining why immediate_hydration errors are filtered out (they are expected side-effects of rapid sequential loads, not indicative of test failure).
🤖 Prompt for AI Agents
In @react_on_rails_pro/spec/dummy/e2e-tests/large_props_stress_test.spec.ts
around lines 251 - 255, Add an inline comment above the page.on('console')
handler explaining that filtering out messages containing "immediate_hydration"
is intentional: these errors stem from a known race condition (issue #2283) that
occurs during rapid sequential HTML streaming and are expected side-effects of
the stress test rather than actual failures; mention the specific handler
(page.on('console', (msg: ConsoleMessage) => { ... })) and the filter check
(!msg.text().includes('immediate_hydration')) so future readers know why those
errors are ignored.
There was a problem hiding this comment.
This file has been removed in commit 869a22e. The stress test was replaced with a simpler json_parse_race_condition.spec.ts test.
There was a problem hiding this comment.
@AbanoubGhadban Thank you for the clarification! Since the file has been removed and replaced with the simpler json_parse_race_condition.spec.ts test, my review comment is no longer applicable. I appreciate the update on the test suite refactoring.
🧠 Learnings used
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.spec.js : Use Playwright E2E tests in `react_on_rails/spec/dummy/e2e/playwright/` for React component integration testing. Tests automatically start Rails server on port 5017 before running
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Code Review: Fix JSON parse race condition in immediate_hydrationSummaryExcellent fix for a subtle race condition bug! The solution is elegant, well-tested, and properly documented. The use of ✅ Strengths1. Minimal, Surgical FixThe core fix in
2. Comprehensive Testing InfrastructureOutstanding test setup that not only validates the fix but helps future maintainers understand the problem:
3. Excellent DocumentationThe
4. Code Quality
|
Code Review: JSON Parse Race Condition FixThank you for this excellent PR! This is a well-thought-out solution to a subtle but critical race condition. Here's my detailed review: ✅ Strengths1. Elegant Core SolutionThe fix is brilliantly simple and minimal:
The logic is sound: if the immediate hydration script (which comes right after the props script) exists as a sibling, the closing 2. Comprehensive Testing StrategyThe E2E tests are thorough:
3. Excellent Documentation
4. Robust Test InfrastructureThe
🔍 Potential Issues & Questions1. Middleware Placement Assumption
# StreamingRaceSimulator must be added AFTER Rack::Deflater so it processes
# the uncompressed response. Activated by adding ?simulate_race=true to URLs.
config.middleware.use StreamingRaceSimulatorQuestion: Does this assumption hold in all environments? What happens if a user adds compression middleware later in the stack? Suggestion: Consider adding runtime validation: config.after_initialize do
deflater_index = Rails.application.middleware.middlewares.index(Rack::Deflater)
simulator_index = Rails.application.middleware.middlewares.index(StreamingRaceSimulator)
if deflater_index && simulator_index && simulator_index < deflater_index
warn "[StreamingRaceSimulator] WARNING: Must be placed after Rack::Deflater"
end
end2. Edge Case: Multiple Siblings
const completeEls = Array.from(els).filter((el) => el.nextSibling !== null);Question: What if there are multiple script elements in a row, and the immediate hydration script hasn't arrived yet but other scripts have? Could Analysis: Looking at the server-side rendering code, the immediate hydration script is always rendered immediately after the props script, so this should be safe. But worth confirming. 3. Performance: Multiple Array Conversions
The original Impact: Negligible for typical use (few components), but could matter if there are hundreds of immediate hydration components on a page. Suggestion (optional optimization): async function forAllCompleteElementsAsync(
selector: string,
callback: (el: Element) => Promise<void>,
): Promise<void> {
const els = document.querySelectorAll(selector);
const promises: Promise<void>[] = [];
for (const el of els) {
if (el.nextSibling !== null) {
promises.push(callback(el));
}
}
await Promise.all(promises);
}4. Testing: Missing View Template
def large_props_stress_test
# ...
endI don't see a corresponding view file in the PR diff. Did you create Action needed: Verify the view file exists or add it to the PR. 5. Hardcoded Constants in Test Code
RACE_CONDITION_DELAY = 1
const REGISTRATION_DELAY = window.__DELAYED_COMPONENT_REGISTRATION_DELAY__ || 100;Question: Are these delays (1 second and 100ms) tuned for CI environments? Do they need adjustment for different network speeds or CI parallelization? Suggestion: Consider making them configurable via environment variables for CI tuning. 6. Security: XSS Test Payload
special_chars: "Special: <script>alert('xss')</script> & \"quotes\" 'apostrophe' \u2028\u2029",Good: You're testing XSS payloads in the props. Question: Are these properly escaped when rendered to JSON? Worth adding an assertion in the test to verify the XSS payload doesn't execute. 🐛 Potential Bugs1. Middleware: Incomplete Buffer Handling
# If props script was never found, yield whatever we buffered
yield buffer unless buffer.empty? || props_processedBug: If the response doesn't contain a props script (e.g., error page, API response), the entire response gets buffered in memory until the end, then yielded all at once. Impact:
Fix: Add a buffer size limit or early yield: MAX_BUFFER_SIZE = 100_000 # 100KB
@source.each do |chunk|
if props_processed
yield chunk
next
end
buffer << chunk
# Early yield if buffer gets too large (props script not found yet)
if buffer.size > MAX_BUFFER_SIZE
yield buffer
buffer = +""
end
if (match = buffer.match(PROPS_SCRIPT_PATTERN))
# ... existing logic
end
end2. E2E Test: Race Condition in Race Condition Test
The test sets up event handlers after starting navigation: async function waitForHydrationOrError(page: Page): Promise<HydrationResult> {
const errors: string[] = [];
let hydrated = false;
return new Promise((resolve) => {
// ... setup handlers
page.on('console', onConsole);
page.on('pageerror', onPageError);
// ...
});
}Potential issue: If the hydration happens before the event handlers are registered, the test might miss it. Fix: Set up handlers before navigation: test('should hydrate successfully', async ({ page }) => {
const resultPromise = waitForHydrationOrError(page);
await page.goto('/server_side_hello_world_hooks?simulate_race=true');
const result = await resultPromise;
// ...
});Wait, looking at line 138-142, you actually do this correctly! Good catch. 📝 Code Style & Best Practices1. RuboCop Compliance
These new methods are well-factored to avoid RuboCop complexity warnings. Good job extracting helpers like 2. TypeScript Type SafetyThe E2E tests have some verbose type assertions: (window as unknown as { __capturedLogs?: string[] }).__capturedLogsSuggestion: Define a proper interface: interface WindowWithTestHelpers extends Window {
__capturedLogs?: string[];
__timing?: { hydration: number; domContentLoaded: number };
__DELAYED_COMPONENT_REGISTRATION_DELAY__?: number;
}
declare let window: WindowWithTestHelpers;3. Magic Strings
const HYDRATION_SUCCESS_PATTERN = /HYDRATED HelloWorldHooks in dom node with id/;This is duplicated across test files. Consider extracting to a shared test helper. 🎯 RecommendationsHigh Priority:
Medium Priority:
Low Priority (nice-to-have):
🏆 Overall AssessmentRating: Excellent (9/10) This PR demonstrates:
The core fix is production-ready. The main concerns are around the test infrastructure ( Recommendation:
📚 Questions for Discussion
Great work on this fix! 🎉 |
Code Review - PR #2290: Fix JSON parse race condition in immediate_hydrationSummaryThis PR addresses a critical race condition bug in the ✅ Strengths1. Core Fix is Simple and Effective
2. Excellent Testing Infrastructure
3. Comprehensive E2E Tests
4. Code Quality
🔍 Observations & Minor Suggestions1. Test Component Duplication
This is minor and doesn't block the PR - the duplication is acceptable for test code. 2. Middleware Safety def call(env)
return @app.call(env) if Rails.env.production?
return @app.call(env) unless env["QUERY_STRING"]&.include?("simulate_race=true")
# ...
end3. Magic Numbers
4. Error Context try {
const props = JSON.parse(el.textContent);
} catch (e) {
console.error('[React on Rails] JSON parse failed. Content length:', el.textContent?.length);
console.error('[React on Rails] First 100 chars:', el.textContent?.substring(0, 100));
throw e;
}This would help diagnose future issues. (Optional enhancement) 🔒 Security ConsiderationsXSS Payload in Test Data: The controller includes XSS test payloads (
📊 Performance ConsiderationsPositive:
Potential Concern:
✅ Test CoverageThe E2E tests are thorough:
Missing (optional):
🎯 VerdictAPPROVE ✅ This is a well-crafted fix for a critical bug:
The minor suggestions above are optional enhancements that don't block merging. 📋 Pre-Merge ChecklistBased on CLAUDE.md requirements:
Great work on this fix! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @react_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts:
- Around line 207-214: The hydration timestamp is set via an unawaited
page.evaluate inside the page.on('console', ...) handler, so performance.now()
runs later and skews measurements; fix by awaiting the evaluate call (i.e.,
change the handler to await page.evaluate(...) so (window as
any).__timing.hydration is set synchronously before continuing) or,
alternatively, inject a hydration hook via the existing addInitScript to log or
write performance.now() into window.__timing at the actual hydration moment and
read that value in the test (refer to HYDRATION_SUCCESS_PATTERN,
page.on('console', ...), page.evaluate(...), and the __timing.hydration field).
In @react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb:
- Line 24: The comment saying "then DELAYS 300ms" is out of sync with the actual
RACE_CONDITION_DELAY constant (RACE_CONDITION_DELAY = 1); update the comment in
streaming_race_simulator.rb to reflect the real delay (e.g., "then DELAYS 1s
(1000ms)" or "then DELAYS 1000ms") so the doc matches the RACE_CONDITION_DELAY
value.
🧹 Nitpick comments (2)
react_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts (1)
35-78: Consider clearing the timeout when resolved early.The
setTimeoutfallback at line 74 isn't cleared when the promise resolves viaonConsoleoronPageError, potentially leaving dangling timers.🛠️ Suggested improvement
return new Promise((resolve) => { + let timeoutId: ReturnType<typeof setTimeout>; + const cleanup = () => { page.off('console', onConsole); page.off('pageerror', onPageError); + clearTimeout(timeoutId); }; // ... existing handlers ... page.on('console', onConsole); page.on('pageerror', onPageError); // Timeout fallback (10 seconds) - setTimeout(() => { + timeoutId = setTimeout(() => { cleanup(); resolve({ hydrated, errors }); }, 10000); });react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb (1)
117-123: Potential double-close of source.The
ensureblock ineach(line 118) and theclosemethod (line 122) both call@source.close. In Rack,closeis called aftereachcompletes, leading to@source.closebeing invoked twice. While many sources handle this gracefully, consider using a flag to prevent double-close.🛠️ Suggested improvement
class DelayedPropsBody + PROPS_SCRIPT_PATTERN = /<script[^>]*class="[^"]*js-react-on-rails-component[^"]*"[^>]*>.*?<\/script>/m + RACE_CONDITION_DELAY = 1 + def initialize(source) @source = source + @closed = false end def each # ... existing implementation ... ensure - @source.close if @source.respond_to?(:close) + close end def close - @source.close if @source.respond_to?(:close) + return if @closed + @closed = true + @source.close if @source.respond_to?(:close) end end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.tsreact_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rb: Runbundle exec rubocopand fix ALL violations before every commit/push
Runbundle exec rubocop(MANDATORY) before every commit to ensure zero offenses
Files:
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb
🧠 Learnings (12)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.spec.js : Use Playwright E2E tests in `react_on_rails/spec/dummy/e2e/playwright/` for React component integration testing. Tests automatically start Rails server on port 5017 before running
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
Applied to files:
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.spec.js : Use Playwright E2E tests in `react_on_rails/spec/dummy/e2e/playwright/` for React component integration testing. Tests automatically start Rails server on port 5017 before running
Applied to files:
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Applied to files:
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Applied to files:
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rbreact_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.spec.js : Use `app('clean')`, `appEval(code)`, `appFactories(options)`, and `appScenario(name)` helpers from cypress-on-rails in Playwright tests for Rails integration
Applied to files:
react_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/app_commands/**/*.rb : Create custom Rails app commands in `e2e/playwright/app_commands/` directory with .rb files for reusable helpers in Playwright tests
Applied to files:
react_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Install Playwright browsers with `cd react_on_rails/spec/dummy && pnpm playwright install --with-deps` before running E2E tests
Applied to files:
react_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.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). (7)
- GitHub Check: dummy-app-integration-tests (3.2, 20, minimum)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: pro-lint-js-and-ruby
🔇 Additional comments (7)
react_on_rails_pro/spec/dummy/e2e-tests/json_parse_race_condition.spec.ts (5)
1-25: LGTM!Clear documentation explaining the test purpose with issue reference. The constants and interface definition are well-structured for capturing hydration results.
85-100: LGTM!Good defensive approach checking for already-captured logs before waiting for the console event, avoiding race conditions where hydration completes before the listener is attached.
106-122: LGTM!Clean verification of React hydration by testing state updates through user interaction.
124-158: LGTM!Comprehensive test coverage for both race-condition and normal scenarios. Good pattern of concurrent navigation + hydration monitoring followed by interactivity verification.
239-275: LGTM!Good test for verifying the fallback hydration path works correctly when the initial immediate hydration is skipped due to incomplete props.
react_on_rails_pro/spec/dummy/lib/streaming_race_simulator.rb (2)
36-54: LGTM!Clean Rack middleware implementation. Correctly activates only on
simulate_race=truequery param and properly removesContent-Lengthsince response timing is modified.
70-116: LGTM!Well-implemented buffering and splitting logic. The approach of finding complete props script, splitting at JSON midpoint, and introducing delay accurately simulates the race condition. Good handling of the fallback case at line 116 when no match is found.
Review Feedback AddressedThank you for the thorough reviews! Here's a summary of the changes made in response: ✅ Files Removed (commits 869a22e, f9ad72b)
✅ Code Issues Fixed (commit ec15e5b)
✅ Debug Statements Removed (commit 869a22e)
Files Remaining in PRThe PR now contains only the essential fix and test infrastructure:
|
|
Code Review - PR 2290: Fix JSON parse race condition in immediate_hydration Summary: This PR addresses a critical race condition bug in React on Rails Pro immediate_hydration feature where JSON parsing fails during HTML streaming on slow networks. The fix is elegant, well-tested, and thoroughly documented. STRENGTHS:
ISSUES AND CONCERNS:
CODE QUALITY ASSESSMENT:
MINOR SUGGESTIONS:
FINAL RECOMMENDATION: APPROVE with required changes
IMPACT ASSESSMENT: This is a high-value fix that addresses a real production issue with minimal risk. Great work! |
Code Review - PR #2290: Fix JSON parse race condition in immediate_hydrationI've reviewed this PR and have the following feedback: ✅ Overall Assessment: Excellent WorkThis is a well-crafted fix for a subtle race condition. The solution is elegant, the testing is comprehensive, and the documentation is thorough. 🎯 Core Fix AnalysisThe Solution (ClientSideRenderer.ts:288-295): const completeEls = Array.from(els).filter((el) => el.nextSibling !== null);Why This Works:
Strengths: 🧪 Testing ApproachStreamingRaceSimulator Middleware (streaming_race_simulator.rb): Excellent design:
E2E Tests (json_parse_race_condition.spec.ts):
Comprehensive coverage of:
📝 Code QualityDocumentation:
Code Organization:
|
Code ReviewThis is an excellent fix for the JSON parse race condition! The solution is elegant, well-tested, and thoroughly documented. Here's my detailed review: ✅ Strengths1. Brilliant Solution DesignThe
2. Exceptional Test CoverageThe
3. Documentation Excellence
🔍 Code Quality ReviewTypeScript Changes (
|
Code Review: JSON Parse Race Condition FixThank you for this thorough fix! The solution is elegant and well-tested. Here's my detailed review: ✅ Strengths1. Root Cause Analysis is ExcellentThe PR description clearly explains the race condition:
2. Solution is Elegant and MinimalThe fix in // Filter elements to only include those with a nextSibling
const completeEls = Array.from(els).filter((el) => el.nextSibling !== null);Why this works: The immediate hydration script is always rendered right after the props script. If 3. Testing Infrastructure is Comprehensive
4. Code Quality
🔍 Observations & Questions1. Middleware Placement (Minor)In config.middleware.use StreamingRaceSimulatorComment says it must be AFTER # Only add in test/development to make intent clearer
config.middleware.use StreamingRaceSimulator unless Rails.env.production?Though since it only activates with 2. Edge Case: What if nextSibling is a Text Node?The fix filters for <script class="js-react-on-rails-component">{"props":...}</script>
<script>ReactOnRails.reactOnRailsComponentLoaded('App');</script>If there's whitespace between the two scripts,
Still, it might be worth a comment clarifying this. Not critical, but adds clarity. 3. Performance ImpactFiltering on 4. Fallback BehaviorThe comment states:
This is correct - components will hydrate via the fallback path. Good defensive design! 🛡️ Security ConsiderationsNo security concerns. The middleware only activates with an explicit query parameter ( ⚡ Performance ConsiderationsPositive impact! This fix prevents JSON parse errors that would otherwise:
The 🧪 Test CoverageExcellent! The E2E test covers:
Suggestion: Consider adding a test comment explaining why both tests are needed (to verify the fix doesn't break normal operation). 📝 Minor Suggestions (Non-Blocking)
🎯 Final Assessment
✅ Recommendation: APPROVEThis is a well-researched, elegantly implemented fix with comprehensive testing. The solution is minimal, performant, and solves a real production issue affecting Pro users. No blocking issues found. All suggestions above are minor improvements for future consideration. Great work, @AbanoubGhadban! 🎉 📚 References
|
This commit moves test infrastructure for reproducing the non-deterministic JSON parsing error (issue #2283) to the Pro dummy app where the immediate hydration feature is enabled by default. Test infrastructure includes: - LargePropsComponent: Component receiving ~200KB props - DelayedLargePropsComponent: Component with delayed registration - large-props-bundle.js: Bundle simulating async component loading - large_props_stress_test view: Test page with multiple large-prop components - Playwright stress test: Concurrent load testing with error monitoring The test simulates the conditions reported: - Large props (~200KB JSON per component) - Immediate hydration enabled (Pro default) - Async component registration via configurable delay - Multiple concurrent page loads Related: #2283 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
This patch-package file can be shared with users experiencing the non-deterministic JSON parsing error to capture diagnostic info in Sentry. The patch wraps JSON.parse calls with enhanced error reporting that captures: - Content length and first/last 200 chars - Error position and surrounding content - Element connection status and document ready state - Detection of HTML entities, null bytes, and likely truncation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit documents the investigation into the non-deterministic JSON parsing error in react-on-rails-pro 16.2.0.beta.20. Root Cause Analysis: - immediate_hydration feature executes hydration JS immediately when async bundle loads, without waiting for DOMContentLoaded - During HTML streaming, DOM elements exist before their content is complete - el.textContent returns partial/truncated content during streaming - JSON.parse() fails on incomplete JSON Investigation includes: - Stress test infrastructure in Pro dummy app - Playwright e2e tests for concurrent page loads - Standalone demo files proving the browser behavior - Diagnostic patch for production debugging This is WIP investigation code - will be reverted after documentation. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Keep analysis/json-race-condition-demo/ with standalone demos. Revert dummy app changes to prepare for proper fix implementation. Co-Authored-By: Claude Opus 4.5 <[email protected]>
…dition This middleware reproduces the race condition bug in immediate_hydration (#2283) by: 1. Intercepting responses when ?simulate_race=true is in the URL 2. Finding the props <script> tag and splitting its JSON content 3. Sending the first half, delaying, then sending the second half This creates a window where JS can execute and read incomplete JSON, causing "SyntaxError: Unterminated string in JSON" errors. For testing purposes only - should never be used in production. Co-Authored-By: Claude Opus 4.5 <[email protected]>
This test uses the StreamingRaceSimulator middleware to reproduce the race condition bug in immediate_hydration. Test 1: Verifies that ?simulate_race=true triggers JSON parse errors Test 2: Verifies normal operation without the simulation Run with: pnpm e2e-test e2e-tests/json_parse_race_condition.spec.ts Issue: #2283 Co-Authored-By: Claude Opus 4.5 <[email protected]>
When HTML streams on slow networks, the props script element can exist in the DOM before its content is fully received. If the JS bundle loads and executes during this window, JSON.parse() fails with: "SyntaxError: Unterminated string in JSON at position X" The fix filters immediate hydration elements to only process those with a nextSibling (the immediate hydration script), which guarantees the props script's closing tag was parsed and content is complete. Elements without a nextSibling are skipped and will be hydrated when their immediate hydration script executes reactOnRailsComponentLoaded(). Fixes #2283 Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove analysis/json-race-condition-demo/ investigation files - Remove large_props_stress_test view and spec files - Remove debug puts statements from streaming_race_simulator.rb Keep only the actual fix: - E2E test for JSON parse race condition - Updated streaming_race_simulator middleware Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove analysis/react-on-rails-pro+16.2.0-rc.0.patch - investigation file for issue #2283 no longer needed. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Address CodeRabbit feedback: @webpack-cli/serve ^1.6.0 was incompatible with webpack-cli 6.0.1 which requires ^3.0.1. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The E2E test uses /server_side_hello_world_hooks (existing page), not the large props page. Remove: - LargePropsComponent.jsx - DelayedLargePropsComponent.jsx - large-props-bundle.js - large_props_stress_test route and controller action Co-Authored-By: Claude Opus 4.5 <[email protected]>
Added new v16.2.0.rc.1 version section with two bug fixes: - Fixed JSON parse race condition in immediate_hydration (PR #2290) - Fixed webpack-cli compatibility with webpack-dev-server v5 (PR #2291) Updated version links to reflect new rc.1 release. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
### Summary Updated CHANGELOG.md to document v16.2.0.rc.1 release with two bug fixes that address streaming and build compatibility issues since rc.0. ### Pull Request checklist - [x] Update CHANGELOG file ### Changes - **JSON Parse Race Condition Fix** (PR #2290): Fixed race condition in `immediate_hydration` that could cause JSON parse errors when HTML streams on slow networks. - **webpack-cli Compatibility Fix** (PR #2291): Fixed compatibility issue with webpack-dev-server v5 by upgrading webpack-cli and removing deprecated configurations. Co-authored-by: Claude Haiku 4.5 <[email protected]>
2 participants
Problem
When
immediate_hydrationis enabled and HTML streams on slow networks, a race condition can cause JSON parse errors:Root cause: The props
<script>element exists in the DOM before its content is fully received. If the JS bundle executes during this window,el.textContentreturns truncated JSON.Solution
Filter immediate hydration elements to only process those with a
nextSibling. The immediate hydration script is always rendered right after the props script:If
nextSiblingexists, the props script's</script>was parsed → content is complete.If
nextSiblingis null, skip the element → it will hydrate when its inline script executes.Testing
Added
StreamingRaceSimulatormiddleware that splits props script content with a delay, reliably reproducing the race condition. E2E test verifies hydration succeeds even under race condition.Fixes #2283
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.