Skip to content

Fix failing rspec test for non hydrated streamed page #1892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 16 commits into from
Closed

Fix failing rspec test for non hydrated streamed page #1892

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
986d952
tmp
AbanoubGhadban Oct 26, 2025
74ba35c
Revert "tmp"
AbanoubGhadban Oct 30, 2025
764788c
remove error log during rsc generation error handling
AbanoubGhadban Oct 30, 2025
a33dbaa
update test:non-rsc script
AbanoubGhadban Oct 31, 2025
f8d1dbc
revert this: increase ssr timeout
AbanoubGhadban Nov 1, 2025
ea67f66
revert this: prevent request retries at the dummy app
AbanoubGhadban Nov 1, 2025
9301803
revert this: don't make httpx retry the request
AbanoubGhadban Nov 2, 2025
a546fdf
revert this: put very big timeouts for httpx requests
AbanoubGhadban Nov 2, 2025
a94859d
Empty commit
AbanoubGhadban Nov 2, 2025
1374927
revert this: log error happen that causes retry
AbanoubGhadban Nov 2, 2025
3c1cd48
keep max retries to 1
AbanoubGhadban Nov 2, 2025
98e0075
remove retries httpx plugin
AbanoubGhadban Nov 2, 2025
982cf03
Revert "remove retries httpx plugin"
AbanoubGhadban Nov 2, 2025
01823f8
revert chanegs only keep the error logging block
AbanoubGhadban Nov 2, 2025
de5b78b
log retrials
AbanoubGhadban Nov 2, 2025
f33381d
revert this: increase timeouts at rails and node js sides
AbanoubGhadban Nov 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-on-rails-pro/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"build-watch": "yarn run clean && yarn run tsc --watch",
"clean": "rm -rf ./lib",
"test": "yarn test:non-rsc && yarn test:rsc",
"test:non-rsc": "jest tests --testPathIgnorePatterns=\".*(.rsc.test.).*\"",
"test:non-rsc": "jest tests --testPathIgnorePatterns=\".*\\.rsc\\.test\\..*\"",
"test:rsc": "NODE_CONDITIONS=react-server jest tests/*.rsc.test.*",
"type-check": "yarn run tsc --noEmit --noErrorTruncation",
"prepack": "nps build.prepack",
Expand Down
1 change: 0 additions & 1 deletion packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ const streamRenderRSCComponent = (
transformRenderStreamChunksToResultObject(renderState);

const reportError = (error: Error) => {
console.error('Error in RSC stream', error);
if (throwJsErrors) {
emitError(error);
}
Expand Down
20 changes: 17 additions & 3 deletions react_on_rails_pro/lib/react_on_rails_pro/request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,17 @@ def create_connection
# For persistent connections we want retries,
# so the requests don't just fail if the other side closes the connection
# https://honeyryderchuck.gitlab.io/httpx/wiki/Persistent
.plugin(:retries, max_retries: 1, retry_change_requests: true)
.plugin(
:retries,
max_retries: 1,
retry_change_requests: true,
retry_after: ->(req, res) do
Rails.logger.error("An error occured and retry is going to be made")
Rails.logger.error("Error: #{res.error}")
Rails.logger.error("Request Body: #{req.body}")
0
end,
Comment on lines 237 to 242
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid logging request body due to security and privacy risks.

The retry_after hook unconditionally logs the full request body, which may contain sensitive information such as rendering code, form data, PII, or secrets. This creates a compliance and security risk.

Additionally, the hook always returns 0 (immediate retry) regardless of the error type, which could cause inappropriate retry behavior for non-transient errors and lacks backoff strategy.

Minor: Line 235 has a typo—"occured" should be "occurred".

Consider these improvements:

           retry_after: ->(req, res) do
-            Rails.logger.error("An error occured and retry is going to be made")
+            Rails.logger.error("An error occurred and retry is going to be made")
             Rails.logger.error("Error: #{res.error}")
-            Rails.logger.error("Request Body: #{req.body}")
+            # Avoid logging request body due to potential sensitive data
+            Rails.logger.debug("Request path: #{req.path}") if Rails.env.development?
             0
           end,

Also, consider implementing conditional retry logic based on error type (e.g., only retry on transient errors like connection failures, not on 4xx client errors).

📝 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.

Suggested change
retry_after: ->(req, res) do
Rails.logger.error("An error occured and retry is going to be made")
Rails.logger.error("Error: #{res.error}")
Rails.logger.error("Request Body: #{req.body}")
0
end,
retry_after: ->(req, res) do
Rails.logger.error("An error occurred and retry is going to be made")
Rails.logger.error("Error: #{res.error}")
# Avoid logging request body due to potential sensitive data
Rails.logger.debug("Request path: #{req.path}") if Rails.env.development?
0
end,
🤖 Prompt for AI Agents 
In react_on_rails_pro/lib/react_on_rails_pro/request.rb around lines 234–239,
the retry_after hook currently logs the full request body (a security/privacy
risk), has a typo ("occured"), and always returns 0 (immediate retry) regardless
of error type; change it to stop logging req.body and instead log non-sensitive
metadata (e.g., request path, method, and sanitized headers) and the error
message/class, correct the typo to "occurred", and implement conditional
retry/backoff: inspect the response/error (status code or exception class) and
only return a retry delay for transient errors (e.g., network/5xx) — use an
exponential backoff strategy (or nil/false for no retry) for non-transient
errors such as 4xx client errors.

)
.plugin(:stream)
# See https://www.rubydoc.info/gems/httpx/1.3.3/HTTPX%2FOptions:initialize for the available options
.with(
Expand All @@ -244,8 +254,12 @@ def create_connection
# :operation_timeout
# :keep_alive_timeout
timeout: {
connect_timeout: ReactOnRailsPro.configuration.renderer_http_pool_timeout,
read_timeout: ReactOnRailsPro.configuration.ssr_timeout
connect_timeout: 100,
read_timeout: 100,
write_timeout: 100,
request_timeout: 100,
operation_timeout: 100,
keep_alive_timeout: 100,
}
)
rescue StandardError => e
Expand Down
5 changes: 3 additions & 2 deletions react_on_rails_pro/spec/dummy/config/initializers/react_on_rails_pro.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
# that the remote renderer works for CI.
config.renderer_use_fallback_exec_js = false

config.ssr_timeout = 10
config.renderer_http_pool_timeout = 20
config.ssr_timeout = 30

config.raise_non_shell_server_rendering_errors = false

Expand All @@ -35,7 +36,7 @@

# Retry request in case of time out on the node-renderer side
# 0 - no retry
config.renderer_request_retry_limit = 1
config.renderer_request_retry_limit = 0

# Array of globs to find any files for which changes should bust the fragment cache for
# cached_react_component and cached_react_component_hash. This should
Expand Down
Loading