fix: node-renderer diagnostic improvements#3086
Conversation
- Add `credentials` to sensitive-key filtering test coverage - Exclude `renderingRequest` from diagnostic bodyKeys output (already reported via the "Received type:" line) - Document client-side vs server-side keep-alive timeout relationship Fixes #3075 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThree related improvements to the node-renderer package: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR makes three focused diagnostic improvements to the node renderer: it excludes the Confidence Score: 5/5Safe to merge — all changes are correct, well-tested, and narrowly scoped to diagnostic output and documentation. No P0 or P1 findings. The logic change is a simple, correct filter addition; the test additions close a real coverage gap; the Ruby change is documentation-only. All three files look good. No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/react-on-rails-pro-node-renderer/src/worker.ts | Adds key !== 'renderingRequest' to the bodyKeys filter so the renderingRequest field (already surfaced via "Received type:") is not redundantly listed in the diagnostic output. |
| packages/react-on-rails-pro-node-renderer/tests/worker.test.ts | Adds a regex assertion confirming renderingRequest is absent from bodyKeys, and adds Credentials (mixed-case) to the sensitive-key test to close the one gap in coverage of SENSITIVE_REQUEST_BODY_KEYS. |
| react_on_rails_pro/lib/react_on_rails_pro/configuration.rb | Documentation-only addition explaining that renderer_http_keep_alive_timeout should be set shorter than the node renderer's own idle timeout to avoid stale-connection errors. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /bundles/:ts/render/:digest] --> B{renderingRequest valid?}
B -- yes --> C[Execute SSR & return 200]
B -- no --> D[invalidRenderingRequestMessage]
D --> E["Report: Received type: type"]
D --> F["Filter body keys"]
F --> G{key === 'renderingRequest'?}
G -- yes --> H[Exclude — already in type line]
G -- no --> I{key in SENSITIVE_REQUEST_BODY_KEYS?}
I -- yes --> J[Exclude — sensitive]
I -- no --> K[Include in bodyKeys list]
E --> L[Return 400 with diagnostic message]
H --> L
J --> L
K --> L
Reviews (1): Last reviewed commit: "fix: node-renderer diagnostic improvemen..." | Re-trigger Greptile
Review: fix: node-renderer diagnostic improvementsOverall, this is a clean, focused, and low-risk PR. The changes are correct and well-tested. A few observations: worker.ts —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| const bodyKeys = Object.keys(body).filter((key) => !SENSITIVE_REQUEST_BODY_KEYS.has(key.toLowerCase())); | ||
| const bodyKeys = Object.keys(body).filter( | ||
| (key) => key !== 'renderingRequest' && !SENSITIVE_REQUEST_BODY_KEYS.has(key.toLowerCase()), |
There was a problem hiding this comment.
The exact-case match on 'renderingRequest' is correct for this internal protocol, but juxtaposed with the case-insensitive sensitive-key filter it can look like an inconsistency. A short inline comment would clarify the intent:
| (key) => key !== 'renderingRequest' && !SENSITIVE_REQUEST_BODY_KEYS.has(key.toLowerCase()), | |
| (key) => key !== 'renderingRequest' && !SENSITIVE_REQUEST_BODY_KEYS.has(key.toLowerCase()), |
Consider something like:
// 'renderingRequest' is already described by the 'Received type:' line; exclude it to avoid
// redundancy. Exact-case match is safe: this key is fixed by the internal protocol.
(key) => key !== 'renderingRequest' && !SENSITIVE_REQUEST_BODY_KEYS.has(key.toLowerCase()),(Up to you whether a comment adds more than it costs; the logic itself is correct.)
| expect(res.statusCode).toBe(400); | ||
| expect(res.payload).toContain('Invalid "renderingRequest" field in render request.'); | ||
| expect(res.payload).toContain('Received type: array.'); | ||
| expect(res.payload).not.toMatch(/Received body keys:.*renderingRequest/); |
There was a problem hiding this comment.
The assertion is correct — bodyKeys is joined with ', ' so they all land on one line. But .* doesn't cross newlines, so if the format ever changes to spread keys across lines this assertion would silently stop testing what it claims. Using [^\n]* instead of .* makes the single-line assumption explicit:
| expect(res.payload).not.toMatch(/Received body keys:.*renderingRequest/); | |
| expect(res.payload).not.toMatch(/Received body keys:[^\n]*renderingRequest/); |
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
## Summary - Add `credentials` to the sensitive-key filtering test (every other key in `SENSITIVE_REQUEST_BODY_KEYS` already had coverage) - Exclude `renderingRequest` from diagnostic `bodyKeys` output — it's already reported via the `Received type:` line, so showing it again is redundant - Document that `renderer_http_keep_alive_timeout` should be set shorter than the node renderer's server-side idle timeout to prevent stale-connection errors Fixes #3075 ## Test plan - [x] Node renderer `worker.test.ts` passes (verified locally) - [x] RuboCop passes on `configuration.rb` - [ ] CI green 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: changes are limited to error-message diagnostics/test coverage in the Node renderer and comment-only documentation for a Rails config option. > > **Overview** > **Node renderer diagnostics are tightened for malformed `renderingRequest` requests.** The invalid-request message now omits the `renderingRequest` key from the reported `bodyKeys` list (it’s already described via `Received type:`), and tests assert this behavior. > > **Sensitive key filtering coverage is expanded.** Tests add `Credentials` to ensure case-insensitive filtering matches `SENSITIVE_REQUEST_BODY_KEYS`. > > **Rails config docs are clarified.** `renderer_http_keep_alive_timeout` is now documented to be set slightly shorter than the node renderer’s server-side idle timeout to avoid stale-connection reuse errors (and to use HTTPX defaults when `nil`). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f01cd0f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error message clarity for invalid render requests by excluding internal keys from diagnostic output. * **Documentation** * Enhanced configuration documentation for HTTP keep-alive timeout settings with recommended values and risk mitigation guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary - Add `credentials` to the sensitive-key filtering test (every other key in `SENSITIVE_REQUEST_BODY_KEYS` already had coverage) - Exclude `renderingRequest` from diagnostic `bodyKeys` output — it's already reported via the `Received type:` line, so showing it again is redundant - Document that `renderer_http_keep_alive_timeout` should be set shorter than the node renderer's server-side idle timeout to prevent stale-connection errors Fixes #3075 ## Test plan - [x] Node renderer `worker.test.ts` passes (verified locally) - [x] RuboCop passes on `configuration.rb` - [ ] CI green 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: changes are limited to error-message diagnostics/test coverage in the Node renderer and comment-only documentation for a Rails config option. > > **Overview** > **Node renderer diagnostics are tightened for malformed `renderingRequest` requests.** The invalid-request message now omits the `renderingRequest` key from the reported `bodyKeys` list (it’s already described via `Received type:`), and tests assert this behavior. > > **Sensitive key filtering coverage is expanded.** Tests add `Credentials` to ensure case-insensitive filtering matches `SENSITIVE_REQUEST_BODY_KEYS`. > > **Rails config docs are clarified.** `renderer_http_keep_alive_timeout` is now documented to be set slightly shorter than the node renderer’s server-side idle timeout to avoid stale-connection reuse errors (and to use HTTPX defaults when `nil`). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f01cd0f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error message clarity for invalid render requests by excluding internal keys from diagnostic output. * **Documentation** * Enhanced configuration documentation for HTTP keep-alive timeout settings with recommended values and risk mitigation guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary - Add `credentials` to the sensitive-key filtering test (every other key in `SENSITIVE_REQUEST_BODY_KEYS` already had coverage) - Exclude `renderingRequest` from diagnostic `bodyKeys` output — it's already reported via the `Received type:` line, so showing it again is redundant - Document that `renderer_http_keep_alive_timeout` should be set shorter than the node renderer's server-side idle timeout to prevent stale-connection errors Fixes #3075 ## Test plan - [x] Node renderer `worker.test.ts` passes (verified locally) - [x] RuboCop passes on `configuration.rb` - [ ] CI green 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: changes are limited to error-message diagnostics/test coverage in the Node renderer and comment-only documentation for a Rails config option. > > **Overview** > **Node renderer diagnostics are tightened for malformed `renderingRequest` requests.** The invalid-request message now omits the `renderingRequest` key from the reported `bodyKeys` list (it’s already described via `Received type:`), and tests assert this behavior. > > **Sensitive key filtering coverage is expanded.** Tests add `Credentials` to ensure case-insensitive filtering matches `SENSITIVE_REQUEST_BODY_KEYS`. > > **Rails config docs are clarified.** `renderer_http_keep_alive_timeout` is now documented to be set slightly shorter than the node renderer’s server-side idle timeout to avoid stale-connection reuse errors (and to use HTTPX defaults when `nil`). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f01cd0f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error message clarity for invalid render requests by excluding internal keys from diagnostic output. * **Documentation** * Enhanced configuration documentation for HTTP keep-alive timeout settings with recommended values and risk mitigation guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ages * origin/main: (44 commits) Consolidate CSP nonce sanitization into shared module (#2828) Add comprehensive --rsc-pro generator tests (#3098) fix: cross-env validation and docs for renderer password (#3090) Improve package metadata and Pro upgrade CTAs (#3112) docs: standardize warning syntax to GFM alert format (#3115) docs: improve react-intl documentation for React Server Components (#3085) Fix generator CI SSR regression on main (#3110) Refocus GitHub README on docs navigation (#3113) Add manual dev environment testing checklist for coding agents (#3074) Bump version to 16.6.0 Update CHANGELOG.md for 16.6.0 (#3078) fix: node-renderer diagnostic improvements (#3086) fix: pin third-party npm deps in generator to prevent peer dep conflicts (#3083) chore(deps): bump lodash from 4.17.23 to 4.18.1 in the npm-security group across 1 directory (#2920) fix: refactor formatExceptionMessage to accept generic request context (#2877) Bump version to 16.6.0.rc.1 Update CHANGELOG.md for 16.6.0.rc.1 (#3079) Update CHANGELOG.md unreleased section (#3077) Fix Content-Length mismatch and null renderingRequest errors in node renderer (#3069) Improve memory debugging docs with simpler heap snapshot approach (#3072) ... # Conflicts: # docs/pro/home-pro.md # docs/pro/react-on-rails-pro.md # docs/sidebars.ts
1 participant
Summary
credentialsto the sensitive-key filtering test (every other key inSENSITIVE_REQUEST_BODY_KEYSalready had coverage)renderingRequestfrom diagnosticbodyKeysoutput — it's already reported via theReceived type:line, so showing it again is redundantrenderer_http_keep_alive_timeoutshould be set shorter than the node renderer's server-side idle timeout to prevent stale-connection errorsFixes #3075
Test plan
worker.test.tspasses (verified locally)configuration.rb🤖 Generated with Claude Code
Note
Low Risk
Low risk: changes are limited to error-message diagnostics/test coverage in the Node renderer and comment-only documentation for a Rails config option.
Overview
Node renderer diagnostics are tightened for malformed
renderingRequestrequests. The invalid-request message now omits therenderingRequestkey from the reportedbodyKeyslist (it’s already described viaReceived type:), and tests assert this behavior.Sensitive key filtering coverage is expanded. Tests add
Credentialsto ensure case-insensitive filtering matchesSENSITIVE_REQUEST_BODY_KEYS.Rails config docs are clarified.
renderer_http_keep_alive_timeoutis now documented to be set slightly shorter than the node renderer’s server-side idle timeout to avoid stale-connection reuse errors (and to use HTTPX defaults whennil).Reviewed by Cursor Bugbot for commit f01cd0f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Documentation