Isolate Pro runtime and node-renderer behavior changes#2895
Conversation
WalkthroughThis PR removes production-like password validation and fail-closed behavior, refactors bundle/asset upload endpoints to explicitly require Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -56,7 +57,7 @@ export interface Config { | |||
| // Number of workers that will be forked to serve rendering requests. | |||
There was a problem hiding this comment.
Security regression: This doc-comment change reflects the removal of validatePasswordForProduction below. Previously the renderer refused to start in production-like environments without a password, providing a fail-closed security guarantee. Now it silently runs unauthenticated.
If this relaxation is intentional (e.g. delegating enforcement to infrastructure), please add a prominent warning here and in the docs so operators don't accidentally run the renderer unauthenticated in production.
| // Number of workers that will be forked to serve rendering requests. | |
| // If no password is set, no authentication will be required. | |
| // WARNING: Without a password, any client that can reach this renderer can trigger | |
| // server-side rendering. Always protect the renderer with network-level access controls | |
| // (firewall rules, VPN, etc.) when running without a password in non-development environments. |
|
|
||
| // Default to true in development, otherwise it is set to false. | ||
| replayServerAsyncOperationLogs: defaultReplayServerAsyncOperationLogs(), | ||
| // default to true in development, otherwise it is set to false |
There was a problem hiding this comment.
Module-load-time evaluation of NODE_ENV: NODE_ENV is captured once at module load (const NODE_ENV = env.NODE_ENV || 'production'), and defaultConfig is evaluated at module load too. The old code explicitly re-evaluated defaultReplayServerAsyncOperationLogs() inside buildConfig to handle env vars set after import.
The previous test suite verified "does not throw when password is set after module import" — now that env vars are baked in at load time, this scenario silently uses stale values. This is likely fine for production but can cause confusing behaviour in test setups that mutate process.env between require() and buildConfig().
| // default to true in development, otherwise it is set to false | |
| // default to true in development, otherwise it is set to false. | |
| // NOTE: evaluated at module load time; mutating NODE_ENV after import has no effect. | |
| replayServerAsyncOperationLogs: truthy( | |
| env.REPLAY_SERVER_ASYNC_OPERATION_LOGS ?? NODE_ENV === 'development', | |
| ), |
| password: aConfig.password != null ? '<MASKED>' : defaultValue, | ||
| allWorkersRestartInterval: aConfig.allWorkersRestartInterval || defaultValue, | ||
| delayBetweenIndividualWorkerRestarts: aConfig.delayBetweenIndividualWorkerRestarts || defaultValue, | ||
| gracefulWorkerRestartTimeout: aConfig.gracefulWorkerRestartTimeout || defaultValue, |
There was a problem hiding this comment.
Diagnostic regression: The old code distinguished undefined, '', and a real password in logs (<NOT PROVIDED>, <EMPTY STRING>, <MASKED>). The new code collapses undefined and '' together since '' != null is true — an empty string now shows as <MASKED> instead of <EMPTY STRING>. This makes it harder to spot misconfigured-but-blank passwords during debugging.
| gracefulWorkerRestartTimeout: aConfig.gracefulWorkerRestartTimeout || defaultValue, | |
| password: aConfig.password === undefined ? defaultValue : aConfig.password === '' ? '<EMPTY STRING>' : '<MASKED>', |
| ), | ||
| 'Default settings': defaultConfig, | ||
| 'ENV values used for settings (use "RENDERER_" prefix)': envValuesUsed(), | ||
| 'Customized values for settings from config object (overrides ENV)': sanitizedSettings(userConfig), |
There was a problem hiding this comment.
Default settings logged unsanitized: defaultConfig is logged directly now. As long as defaultConfig.password is undefined this is fine, but if the field ever references env.RENDERER_PASSWORD at module load the password would appear in plain text in logs. Worth keeping sanitizedSettings(defaultConfig, ...) here for defence in depth.
| if (key === 'bundle') { | ||
| assertAsset(value, key); | ||
| providedNewBundles.push({ timestamp: bundleTimestamp, bundle: value }); | ||
| } else if (key.startsWith('bundle_')) { |
There was a problem hiding this comment.
Silent empty timestamp for bundle_ field: The old extractBundlesAndAssets warned when key === 'bundle_' (no hash suffix). The replacement silently uses key.replace('bundle_', '') which yields '' for that degenerate key, creating a bundle entry with an empty timestamp string. This could cause subtle filesystem issues (a bundle directory named ''). Consider at least logging a warning:
| } else if (key.startsWith('bundle_')) { | |
| } else if (key.startsWith('bundle_')) { | |
| const timestamp = key.slice('bundle_'.length); | |
| if (!timestamp) { | |
| log.warn('Received form field "bundle_" with no hash suffix — ignoring (possible Ruby client bug)'); | |
| } else { | |
| assertAsset(value, key); | |
| providedNewBundles.push({ timestamp, bundle: value }); | |
| } |
| "@fastify/formbody": "^7.4.0 || ^8.0.2", | ||
| "@fastify/multipart": "^8.3.1 || ^9.0.3", | ||
| "fastify": "^5.8.3", | ||
| "fastify": "^5.8.1", |
There was a problem hiding this comment.
Dependency downgrade: fastify is being lowered from ^5.8.3 to ^5.8.1. ^ ranges allow any compatible version at install time, so npm/pnpm could now resolve 5.8.1 or 5.8.2 — versions that were previously excluded. If this was intentional (e.g. to avoid a regression introduced in 5.8.2/5.8.3), please add a comment explaining why. Otherwise this looks like it should stay at ^5.8.3.
| import { normalizeSearch } from './utils.ts'; | ||
|
|
||
| /** | ||
| * Enables TanStack Router's internal SSR mode and verifies the flag is writable. |
There was a problem hiding this comment.
Compatibility check has a logic gap: routerWithSsrFlag.ssr = true then if (!routerWithSsrFlag.ssr) — if the router normalises the value internally (e.g. stores { manifest: undefined } when given true), the truthiness check passes but the actual stored value differs from what you intended. You'd want to verify the flag was accepted as true, not just truthy:
| * Enables TanStack Router's internal SSR mode and verifies the flag is writable. | |
| function enableRouterSsrMode(router: TanStackRouter): void { | |
| const routerWithSsrFlag = router; | |
| routerWithSsrFlag.ssr = true; | |
| if (routerWithSsrFlag.ssr !== true) { | |
| throw new Error( | |
| 'react-on-rails-pro/tanstack-router: Expected router.ssr to accept a boolean flag. ' + | |
| 'Please check that your @tanstack/react-router version is compatible.', | |
| ); | |
| } | |
| } |
| # BEFORE the response is committed, enabling a proper HTTP redirect. | ||
| # Do NOT re-raise here: the caller owns the error now. | ||
| rescue StandardError | ||
| # Variable was already resolved — the first chunk was returned successfully. | ||
| # This is a post-first-chunk error. Re-raise so barrier.wait propagates it | ||
| # (the response is already committed at that point, so only JS redirect is possible). | ||
| raise e | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
raise e loses the original backtrace: Using raise e re-raises the exception but wraps it with a new backtrace anchored at this rescue block, hiding the original call site. Use bare raise to preserve the original backtrace:
| # BEFORE the response is committed, enabling a proper HTTP redirect. | |
| # Do NOT re-raise here: the caller owns the error now. | |
| rescue StandardError | |
| # Variable was already resolved — the first chunk was returned successfully. | |
| # This is a post-first-chunk error. Re-raise so barrier.wait propagates it | |
| # (the response is already committed at that point, so only JS redirect is possible). | |
| raise e | |
| end | |
| end | |
| rescue StandardError | |
| # Variable was already resolved — the first chunk was returned successfully. | |
| # This is a post-first-chunk error. Re-raise so barrier.wait propagates it | |
| # (the response is already committed at that point, so only JS redirect is possible). | |
| raise |
| result = first_chunk_var.wait | ||
|
|
||
| # If the async task stored an exception (pre-first-chunk error), raise it now. | ||
| # This happens BEFORE response.stream.write(template_string) in |
There was a problem hiding this comment.
Async::Variable raises RuntimeError on double-write: The pattern here relies on the gem raising a StandardError subclass when the variable is already resolved so the inner rescue StandardError can detect the "already resolved" case. This works today because Async::Variable raises RuntimeError (< StandardError). However this is an undocumented internal behaviour — if the gem ever changes to raise a non-StandardError (e.g. Async::Stop) the inner rescue would silently swallow the original error instead of re-raising it. Consider asserting the variable is not yet resolved rather than relying on the exception type, or at least add a comment pinning the gem version dependency.
| @@ -411,7 +411,7 @@ def mock_request_and_response(mock_chunks = chunks, count: 1) | |||
| expect(initial_result).to include(react_component_div_with_initial_chunk) | |||
|
|
|||
| # Wait for async task to complete | |||
There was a problem hiding this comment.
Removed Async::Task.current.with_timeout: The earlier with_timeout(5) prevented the test from hanging indefinitely if the async task never completes. Removing it means a bug in the streaming path could cause this test to hang forever in CI. Consider restoring the timeout or using RSpec's built-in timeout helpers.
| @@ -626,26 +626,6 @@ def setup_stream_test(component_count: 2) | |||
| end | |||
There was a problem hiding this comment.
Removed pre-first-chunk error propagation test: The deleted test "does not commit the response when render_to_string raises" verified that a shell-level error before the first chunk reaches the caller before the response is committed — enabling a proper HTTP redirect. This is one of the trickiest code paths in the streaming implementation, and the new Async::Variable-based approach changes the error propagation mechanism. Please add an equivalent test for the Async::Variable path to verify:
- A pre-first-chunk exception is raised synchronously in
consumer_stream_async response.stream.writehas NOT been called at that point
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Default config logged with plaintext password
logSanitizedConfignow logsdefaultConfigthroughsanitizedSettings(..., '<NOT PROVIDED AT MODULE LOAD>')so passwords are masked again.
- ✅ Fixed: Assets not copied when bundle move fails
copyUploadedAssetswas moved to run after the bundle move try/catch so assets are copied even when move failure is handled as an existing-bundle race.
Or push these changes by commenting:
@cursor push f8f98d0b63
Preview (f8f98d0b63)
diff --git a/packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts b/packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts
--- a/packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts
+++ b/packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts
@@ -241,7 +241,7 @@
log.info({
'Node Renderer version': packageJson.version,
'Protocol version': packageJson.protocolVersion,
- 'Default settings': defaultConfig,
+ 'Default settings': sanitizedSettings(defaultConfig, '<NOT PROVIDED AT MODULE LOAD>'),
'ENV values used for settings (use "RENDERER_" prefix)': envValuesUsed(),
'Customized values for settings from config object (overrides ENV)': sanitizedSettings(userConfig),
'Final renderer settings': sanitizedSettings(config, '<NOT PROVIDED>'),
diff --git a/packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts b/packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts
--- a/packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts
+++ b/packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts
@@ -107,10 +107,6 @@
`Moving uploaded file ${providedNewBundle.bundle.savedFilePath} to ${bundleFilePathPerTimestamp}`,
);
await moveUploadedAsset(providedNewBundle.bundle, bundleFilePathPerTimestamp);
- if (assetsToCopy) {
- await copyUploadedAssets(assetsToCopy, bundleDirectory);
- }
-
log.info(
`Completed moving uploaded file ${providedNewBundle.bundle.savedFilePath} to ${bundleFilePathPerTimestamp}`,
);
@@ -131,6 +127,9 @@
bundleFilePathPerTimestamp,
);
}
+ if (assetsToCopy) {
+ await copyUploadedAssets(assetsToCopy, bundleDirectory);
+ }
return undefined;
} finally {You can send follow-ups to this agent here.
| defaultConfig, | ||
| '<NOT PROVIDED AT MODULE LOAD>', | ||
| ), | ||
| 'Default settings': defaultConfig, |
There was a problem hiding this comment.
Default config logged with plaintext password
High Severity
logSanitizedConfig now logs defaultConfig directly instead of passing it through sanitizedSettings(). Since defaultConfig.password holds the raw env.RENDERER_PASSWORD value, the plaintext password will appear in logs. The old code used sanitizedSettings(defaultConfig, '<NOT PROVIDED AT MODULE LOAD>') to mask it.
Additional Locations (1)
| await moveUploadedAsset(providedNewBundle.bundle, bundleFilePathPerTimestamp); | ||
| if (assetsToCopy) { | ||
| await copyUploadedAssets(assetsToCopy, bundleDirectory); | ||
| } |
There was a problem hiding this comment.
Assets not copied when bundle move fails
Medium Severity
copyUploadedAssets was moved inside the try block that wraps moveUploadedAsset. If the move throws (e.g., because another worker already wrote the bundle), the catch block handles it gracefully but assets are never copied. The old code intentionally placed asset copying after this try/catch so assets were always copied, even when the bundle already existed.
|
Critical: Security regression. validatePasswordForProduction (TS) and validate_renderer_password_for_production (Ruby) are deleted. The renderer now starts without a password in any environment. The old behavior was fail-closed; the new behavior is fail-open. If intentional, add: a prominent doc comment explaining the changed security model and required mitigations (firewall, VPN, mTLS), a CHANGELOG entry flagging the breaking security change, and a runtime warning log when starting without a password in a non-development NODE_ENV. Medium: Protocol breaking change. The /upload-assets endpoint now requires targetBundles and drops the bundle_hash form key convention. Document the protocol 2.0.0 bump in the upgrade guide so users running the Node renderer and Rails gem at different versions get a clear error rather than a silent asset-copy failure. Medium: Critical test coverage removed. The pre-first-chunk error propagation tests are deleted with no equivalent test for the Async::Variable path. This is one of the trickiest code paths in the codebase. See inline comment on stream_spec.rb. Medium: Backtrace lost on re-raise. The inner rescue block in consumer_stream_async uses raise e instead of bare raise, which replaces the original exception backtrace. See inline comment on react_on_rails_pro_helper.rb. Minor items (see inline comments for details): fastify downgraded from 5.8.3 to 5.8.1 range, needs comment if intentional; bundle_ key without hash silently creates empty-timestamp bundle entry; sanitizedSettings no longer distinguishes empty-string from real passwords; router.ssr check uses falsy rather than strict equality; with_timeout removed from specs leaving bare @async_barrier.wait that can hang CI. |
Greptile SummaryThis PR isolates two categories of changes from a larger migration branch: (1) the node-renderer protocol update that replaces Key changes:
Confidence Score: 4/5Mostly safe to merge once the password-plaintext logging regression in logSanitizedConfig is addressed; the other findings are behavioral trade-offs or test gaps that can be followed up. One P1 finding remains: logSanitizedConfig now logs defaultConfig directly instead of through sanitizedSettings(), which exposes RENDERER_PASSWORD in plaintext whenever the startup config dump runs. The remaining P2 items (asset-copy skip on race-condition path, deleted error-path tests, removal of production password guards) are lower-risk behavioral trade-offs or test debt, none of which break the primary render path. packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts (plaintext password logging); packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts (asset-copy skip on race path); react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb and react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb (deleted error-path tests). Important Files Changed
Sequence DiagramsequenceDiagram
participant Rails as Rails Client
participant NR as Node Renderer Worker
participant FS as File System
Note over Rails,FS: Render request (bundle + assets)
Rails->>NR: POST /:bundleTimestamp (multipart: bundle file + assets)
NR->>FS: moveUploadedAsset(bundle → bundleDirectory)
NR->>FS: copyUploadedAssets(assets → bundleDirectory)
NR-->>Rails: render result
Note over Rails,FS: Upload-assets request (NEW protocol — assets only)
Rails->>NR: POST /upload-assets (targetBundles: [hash1, hash2], asset files)
loop per bundleTimestamp in targetBundles
NR->>FS: mkdir(bundleDirectory)
NR->>NR: lock(bundleFilePath)
NR->>FS: copyUploadedAssets(assets → bundleDirectory)
NR->>NR: unlock
end
NR-->>Rails: 200 OK
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 'Default settings': defaultConfig, | ||
| 'ENV values used for settings (use "RENDERER_" prefix)': envValuesUsed(), | ||
| 'Customized values for settings from config object (overrides ENV)': sanitizedSettings(userConfig), | ||
| 'Final renderer settings': sanitizedSettings(config, '<NOT PROVIDED>'), | ||
| }); |
There was a problem hiding this comment.
defaultConfig
defaultConfig.password holds the raw value of env.RENDERER_PASSWORD (set at module load, line 171). By logging defaultConfig directly — without wrapping it in sanitizedSettings() — the password is now emitted in plaintext in the 'Default settings' log entry every time logSanitizedConfig() is called.
The previous code explicitly masked it:
'Default settings at module load (env-backed values may lag current runtime)': sanitizedSettings(
defaultConfig,
'<NOT PROVIDED AT MODULE LOAD>',
),Apply the same sanitizedSettings wrapper here:
| 'Default settings': defaultConfig, | |
| 'ENV values used for settings (use "RENDERER_" prefix)': envValuesUsed(), | |
| 'Customized values for settings from config object (overrides ENV)': sanitizedSettings(userConfig), | |
| 'Final renderer settings': sanitizedSettings(config, '<NOT PROVIDED>'), | |
| }); | |
| 'Default settings': sanitizedSettings(defaultConfig, '<NOT PROVIDED>'), |
| await moveUploadedAsset(providedNewBundle.bundle, bundleFilePathPerTimestamp); | ||
| if (assetsToCopy) { | ||
| await copyUploadedAssets(assetsToCopy, bundleDirectory); | ||
| } | ||
|
|
There was a problem hiding this comment.
Moving copyUploadedAssets inside the try block means it is skipped when moveUploadedAsset throws because the destination file already exists (the race-condition catch path). The removed code deliberately placed this call after the catch and included the comment:
Always copy assets to the bundle directory — even if the bundle was already present (e.g., from a prior upload or another worker).
copyUploadedAssetsusesoverwrite:true, so this is idempotent.
With this change, if two workers race to upload the same bundle and Worker B's move fails because Worker A already wrote the file, Worker B's assets are never copied to the bundle directory.
Consider restoring the unconditional copy inside the catch path:
} catch (error) {
const fileExists = await fileExistsAsync(bundleFilePathPerTimestamp);
if (!fileExists) {
return errorResponseResult(msg);
}
log.info('File exists when trying to overwrite bundle. Assuming bundle written by other thread');
if (assetsToCopy) {
await copyUploadedAssets(assetsToCopy, bundleDirectory);
}
}| first_chunk_var.value = e | ||
| # Variable accepted the error — this is a pre-first-chunk failure (e.g., shell error). | ||
| # The caller will detect the Exception value and raise it synchronously, | ||
| # BEFORE the response is committed, enabling a proper HTTP redirect. | ||
| # Do NOT re-raise here: the caller owns the error now. | ||
| rescue StandardError | ||
| # Variable was already resolved — the first chunk was returned successfully. | ||
| # This is a post-first-chunk error. Re-raise so barrier.wait propagates it | ||
| # (the response is already committed at that point, so only JS redirect is possible). | ||
| raise e |
There was a problem hiding this comment.
Async::Variable assignment exception type
When the variable is already resolved and first_chunk_var.value = e raises, the rescue StandardError catches it and re-raises e. This works as long as Async::Variable raises a StandardError subclass on a second assignment. If a future version of the async gem raises a non-StandardError, the double-assignment exception would propagate unhandled.
A more explicit approach using a settled flag avoids coupling to the gem's exception type:
settled = false
@async_barrier.async do
stream = yield
settled = true
fully_consumed = process_stream_chunks(stream, first_chunk_var, all_chunks)
on_complete&.call(all_chunks) if fully_consumed
rescue StandardError => e
if settled
raise e
else
first_chunk_var.value = e
end
end![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7ca7a5ad4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| defaultConfig, | ||
| '<NOT PROVIDED AT MODULE LOAD>', | ||
| ), | ||
| 'Default settings': defaultConfig, |
There was a problem hiding this comment.
logSanitizedConfig now logs defaultConfig directly, but defaultConfig.password is populated from RENDERER_PASSWORD. In clustered mode (master.ts calls this on startup), this writes the raw renderer shared secret to logs whenever password auth is configured, which can leak credentials to log files and external log aggregation systems. The default settings payload should be sanitized/redacted before logging.
Useful? React with 👍 / 👎.
| if (assetsToCopy) { | ||
| await copyUploadedAssets(assetsToCopy, bundleDirectory); | ||
| } |
There was a problem hiding this comment.
copyUploadedAssets is now inside the moveUploadedAsset success path only. If another worker/request already wrote the same bundle, moveUploadedAsset can throw, the catch path accepts that race (File exists...), and the function returns without copying this request's assets. In concurrent upload/render scenarios, that can leave bundle directories with missing or stale auxiliary assets when the winning writer did not provide the same asset set/version.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb (1)
149-155: Use precise wording for this example description.Line 149 says “blank,” but the assertion on Line 154 specifically expects
nil. Consider renaming to “is nil if not provided in the URL” to avoid implying empty-string acceptance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb` around lines 149 - 155, Update the example description string to accurately reflect the expectation: change the spec description that currently reads "is blank if not provided in the URL" to "is nil if not provided in the URL" so it matches the assertion on ReactOnRailsPro.configuration.renderer_password (and the example using ReactOnRailsPro.configure / config.renderer_url).react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb (1)
414-415: Consider retaining timeout protection for test reliability.Removing the
with_timeout(5)wrapper means tests could hang indefinitely if a regression causes the barrier to never complete. While the current implementation should be correct, a timeout provides a safety net that converts hangs into clear failures.That said, this is a low-risk change given the controlled test environment with finite mocked chunks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb` around lines 414 - 415, Reintroduce the timeout guard around the async barrier wait and queue close to avoid test hangs: wrap the calls to `@async_barrier.wait` and `@main_output_queue.close` in with_timeout(5) so the test fails fast on regressions; locate the test block that currently calls `@async_barrier.wait` and `@main_output_queue.close` and restore the with_timeout(5) wrapper (using the existing with_timeout helper) to enforce a 5-second timeout.react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb (1)
628-656: Consider adding test coverage for pre-first-chunk error propagation.Based on the AI summary, a spec that verified pre-first-chunk errors (e.g., shell errors from
render_to_string) don't result in stream writes was removed. The remaining exception handling tests (lines 629-655) only cover post-first-chunk scenarios (writer exceptions during streaming).The
Async::Variable-based error propagation logic inconsumer_stream_async(lines 452-463 in the helper) is critical for ensuring HTTP redirects work for shell errors. Consider adding a test that:
- Stubs
render_to_stringto raise before any chunks are produced- Verifies the exception propagates synchronously
- Confirms
stream.writeis never calledWould you like me to help draft a test case that covers pre-first-chunk error propagation?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb` around lines 628 - 656, Add a new spec that ensures pre-first-chunk errors from the rendering phase propagate synchronously and never trigger any stream writes: use setup_stream_test to get queues, controller, and stream; stub the renderer (e.g., render_to_string or the helper that produces the first chunk used by consumer_stream_async) to raise an exception before any chunks are enqueued; assert that running the stream raises that error immediately (not deadlocked) and verify stream.write is never called (use allow(stream).to receive(:write) and expect not_to have_received(:write)); this exercises the Async::Variable-based error propagation in consumer_stream_async so HTTP redirects/shell errors are handled prior to the first chunk.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-on-rails-pro-node-renderer/package.json`:
- Line 32: package.json now lists "fastify": "^5.8.1" but the pnpm-lock.yaml is
out of sync (it still references ^5.8.3); run pnpm install (without
--frozen-lockfile) to regenerate pnpm-lock.yaml and commit the updated lockfile
so CI matches package.json, or if you intended to pin a specific release change
the dependency in package.json to the exact version instead of "^5.8.1".
---
Nitpick comments:
In
`@react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb`:
- Around line 414-415: Reintroduce the timeout guard around the async barrier
wait and queue close to avoid test hangs: wrap the calls to `@async_barrier.wait`
and `@main_output_queue.close` in with_timeout(5) so the test fails fast on
regressions; locate the test block that currently calls `@async_barrier.wait` and
`@main_output_queue.close` and restore the with_timeout(5) wrapper (using the
existing with_timeout helper) to enforce a 5-second timeout.
In `@react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb`:
- Around line 149-155: Update the example description string to accurately
reflect the expectation: change the spec description that currently reads "is
blank if not provided in the URL" to "is nil if not provided in the URL" so it
matches the assertion on ReactOnRailsPro.configuration.renderer_password (and
the example using ReactOnRailsPro.configure / config.renderer_url).
In `@react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb`:
- Around line 628-656: Add a new spec that ensures pre-first-chunk errors from
the rendering phase propagate synchronously and never trigger any stream writes:
use setup_stream_test to get queues, controller, and stream; stub the renderer
(e.g., render_to_string or the helper that produces the first chunk used by
consumer_stream_async) to raise an exception before any chunks are enqueued;
assert that running the stream raises that error immediately (not deadlocked)
and verify stream.write is never called (use allow(stream).to receive(:write)
and expect not_to have_received(:write)); this exercises the
Async::Variable-based error propagation in consumer_stream_async so HTTP
redirects/shell errors are handled prior to the first chunk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e6b2f7e-b2c7-438c-8383-78dfdbbc8545
📒 Files selected for processing (19)
packages/react-on-rails-pro-node-renderer/package.jsonpackages/react-on-rails-pro-node-renderer/src/shared/configBuilder.tspackages/react-on-rails-pro-node-renderer/src/worker.tspackages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.tspackages/react-on-rails-pro-node-renderer/tests/configBuilder.test.tspackages/react-on-rails-pro-node-renderer/tests/uploadRaceCondition.test.tspackages/react-on-rails-pro-node-renderer/tests/worker.test.tspackages/react-on-rails-pro/src/tanstack-router/clientHydrate.tspackages/react-on-rails-pro/src/tanstack-router/serverRender.tspackages/react-on-rails-pro/src/tanstack-router/types.tspackages/react-on-rails-pro/tests/tanstackRouter.test.tsreact_on_rails/lib/react_on_rails/helper.rbreact_on_rails_pro/app/helpers/react_on_rails_pro_helper.rbreact_on_rails_pro/lib/react_on_rails_pro/configuration.rbreact_on_rails_pro/lib/react_on_rails_pro/request.rbreact_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/stream_integration_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb
💤 Files with no reviewable changes (1)
- react_on_rails_pro/lib/react_on_rails_pro/configuration.rb
| "@fastify/formbody": "^7.4.0 || ^8.0.2", | ||
| "@fastify/multipart": "^8.3.1 || ^9.0.3", | ||
| "fastify": "^5.8.3", | ||
| "fastify": "^5.8.1", |
There was a problem hiding this comment.
Lockfile is out of sync — regenerate with pnpm install.
The pipeline failures indicate pnpm-lock.yaml still specifies fastify: ^5.8.3 while package.json now declares ^5.8.1. Run pnpm install (without --frozen-lockfile) and commit the updated lockfile to fix CI.
Also, note that ^5.8.1 is a broader range that includes 5.8.3, so this change doesn't prevent installing newer patch versions — it just lowers the floor. If the intent was to pin a specific version, consider using an exact version instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-on-rails-pro-node-renderer/package.json` at line 32,
package.json now lists "fastify": "^5.8.1" but the pnpm-lock.yaml is out of sync
(it still references ^5.8.3); run pnpm install (without --frozen-lockfile) to
regenerate pnpm-lock.yaml and commit the updated lockfile so CI matches
package.json, or if you intended to pin a specific release change the dependency
in package.json to the exact version instead of "^5.8.1".
|
start over |
1 participant
Summary
Scope
packages/react-on-rails-pro-node-renderer/**packages/react-on-rails-pro/src/tanstack-router/**packages/react-on-rails-pro/tests/tanstackRouter.test.tsreact_on_rails/lib/react_on_rails/helper.rbreact_on_rails_pro/{app/helpers,lib,spec}/**Testing
pnpm --filter react-on-rails-pro test✅pnpm --filter react-on-rails-pro-node-renderer exec jest tests/configBuilder.test.ts tests/uploadRaceCondition.test.ts tests/worker.test.ts --runInBand✅bundle exec rubocop(via hooks and targeted run) ✅bundle exec rspec react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb react_on_rails_pro/spec/react_on_rails_pro/stream_integration_spec.rb react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rbasyncgem (LoadError: cannot load such file -- async)Note
Medium Risk
Changes Node renderer request semantics (
/upload-assetsnow requirestargetBundles) and relaxes renderer password enforcement, which can affect deployment security and compatibility with older clients. Also adjusts TanStack Router SSR flags and streaming async primitives, touching hydration and response streaming behavior.Overview
Node renderer upload/render handling is updated for protocol
2.0.0./upload-assetsnow requires atargetBundlesfield and copies uploaded assets into each target bundle directory under per-bundle locks, usingPromise.allSettledto avoid races with request upload-dir cleanup; related tests are updated to stop sendingbundle_<hash>fields for uploads.Renderer configuration and auth defaults are loosened/simplified.
configBuilderdrops production-like password enforcement/validation and simplifies env/default resolution and log sanitization (password is masked when present; no special empty-string handling), andreplayServerAsyncOperationLogsdefaulting is now based onNODE_ENV.TanStack Router SSR integration is adjusted. Both server render and client hydration now set
router.ssr = true(with a server-side compatibility check) to reduce hydration mismatches, and legacy logic that temporarily set/cleared an object-shapedssrflag is removed; tests are updated accordingly.Ruby Pro streaming and config behavior are updated. Streaming switches from
Async::PromisetoAsync::Variablefor first-chunk handoff and error propagation, renderer password production validation is removed from Pro configuration, and helper/request code and specs are aligned; a couple of documentation links are updated.Written by Cursor Bugbot for commit e7ca7a5. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
Refactor
targetBundlesparameter for bundle targeting