chore: update React on Rails RC and Shakapacker 10.1.0#740
Conversation
WalkthroughThis PR bumps react_on_rails_pro to 16.7.0.rc.3 and shakapacker to 10.1.0 in Gemfile, package.json, and README, and refactors the RSC request spec to parse application/x-ndjson as length-prefixed chunks with validation and an added error test. ChangesDependency Upgrades and RSC Spec Parser
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
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 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 |
Review app commands
For setup details, comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "react-on-rails-pro": "16.7.0-rc.0", | ||
| "react-on-rails-pro-node-renderer": "16.7.0-rc.0", |
There was a problem hiding this comment.
The yarn.lock file was intentionally left at 16.6.0 for these packages, which means the committed lockfile is inconsistent with package.json. Any CI step using --frozen-lockfile (e.g. yarn install --frozen-lockfile) will fail because the lockfile doesn't satisfy the new 16.7.0-rc.0 requirement. The lockfile should be regenerated by running yarn install locally and committing the result—even for RC versions—so the repo is in a coherent, reproducible state.
There was a problem hiding this comment.
Already addressed by the current branch state. yarn.lock now resolves the React on Rails Pro packages at 16.7.0-rc.3, and bin/conductor-exec yarn install --frozen-lockfile passes.
| psych (>= 4.0.0) | ||
| tsort | ||
| react_on_rails (16.6.0) | ||
| react_on_rails (16.7.0.rc.0) |
There was a problem hiding this comment.
The PR description notes that Gemfile.lock was manually edited (only version strings were updated, not via bundle update react_on_rails_pro). Modern Bundler versions store per-gem checksums in Gemfile.lock; if those weren't updated to match the new gem tarballs, bundle install will fail with a checksum mismatch error. Please regenerate this file with bundle update react_on_rails react_on_rails_pro and commit the result.
There was a problem hiding this comment.
Already addressed by the current branch state. The lockfile has been regenerated for react_on_rails_pro 16.7.0.rc.3, and bin/conductor-exec bundle check passes.
![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 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Stale yarn lock blocks installs
- Updated yarn.lock to lock react-on-rails-pro, react-on-rails-pro-node-renderer, and the nested react-on-rails dependency at 16.7.0-rc.0 so frozen installs now succeed.
Or push these changes by commenting:
@cursor push 06cf0b472a
Preview (06cf0b472a)
diff --git a/yarn.lock b/yarn.lock
--- a/yarn.lock
+++ b/yarn.lock
@@ -8783,10 +8783,10 @@
resolved "https://registry.npmjs.org/react-is/-/react-is-18.3.1.tgz"
integrity sha512-/LLMVyas0ljjAtoYiPqYiL8VWXzUUdThrmU5+n20DZv+a+ClRoevUzw5JxU+Ieh5/c87ytoTBV9G1FiKfNJdmg==
-react-on-rails-pro-node-renderer@16.6.0:
- version "16.6.0"
- resolved "https://registry.npmjs.org/react-on-rails-pro-node-renderer/-/react-on-rails-pro-node-renderer-16.6.0.tgz#c13ca0f156566531d7c6e005759459b0f19472a8"
- integrity sha512-fBZ0lKRaEe8LyVTdUsXx364zQfL6hGJuE+2qQsKo+bXm0aTVq2RtO49gzq0m7Y4xuhBTVmnPQUP0O1v1cGRzLg==
+react-on-rails-pro-node-renderer@16.7.0-rc.0:
+ version "16.7.0-rc.0"
+ resolved "https://registry.yarnpkg.com/react-on-rails-pro-node-renderer/-/react-on-rails-pro-node-renderer-16.7.0-rc.0.tgz#fcd6b23bddf81a3805f38d2f23561a6aeaa16ef2"
+ integrity sha512-kgm8iuoyT1I2DPID0AWTaSjIuVbiW39vTBBouEZ2adxzX2+YKVpuBGQgTaRedTUPcdCkuVTnSNPIgcmkY9FNmw==
dependencies:
"@fastify/formbody" "^7.4.0 || ^8.0.2"
"@fastify/multipart" "^8.3.1 || ^9.0.3"
@@ -8796,12 +8796,12 @@
lockfile "^1.0.4"
pino "^9.0.0"
-react-on-rails-pro@16.6.0:
- version "16.6.0"
- resolved "https://registry.npmjs.org/react-on-rails-pro/-/react-on-rails-pro-16.6.0.tgz#19a5ea99d7b397dd56f14cff1f31955211b4d0a2"
- integrity sha512-Uc8o3gdHyIETvY5J9wVUyONKOhnkw9kGJDREMHQb/IuXoB5/Vo51UK487Rcep2Z+Dzz/bEvNoF+GuZohORZ7Zw==
+react-on-rails-pro@16.7.0-rc.0:
+ version "16.7.0-rc.0"
+ resolved "https://registry.yarnpkg.com/react-on-rails-pro/-/react-on-rails-pro-16.7.0-rc.0.tgz#2b1b6f1a65dc1780e77f7c2cb3e78916bf7b9eba"
+ integrity sha512-gy7PgXJZokaDepBVnqPIzINAhxJiBYbkGN0BHu/chRjhW2L7CJO/PC5pLnWnaV6CBu8AJipM+oNBiUpEXUyA6A==
dependencies:
- react-on-rails "16.6.0"
+ react-on-rails "16.7.0-rc.0"
react-on-rails-rsc@19.0.4:
version "19.0.4"
@@ -8812,10 +8812,10 @@
neo-async "^2.6.1"
webpack-sources "^3.2.0"
-react-on-rails@16.6.0:
- version "16.6.0"
- resolved "https://registry.npmjs.org/react-on-rails/-/react-on-rails-16.6.0.tgz#da7f117fec14f420f7f6ffe6bdb34b7fc2e01b3a"
- integrity sha512-LqLi7A0n0Tv5c3yMYlwS9s6rE82gvXNMj3sscmK2LOgIJ+mLQlOX65n0Jq5ZJ4Nsl9SRgxEOOQmcrfvDeB9F1g==
+react-on-rails@16.7.0-rc.0:
+ version "16.7.0-rc.0"
+ resolved "https://registry.yarnpkg.com/react-on-rails/-/react-on-rails-16.7.0-rc.0.tgz#4fb47203ab9f65528605233c3b6cddba69536107"
+ integrity sha512-81Lu9ToSlunduiPQm+R+C+DJ9irdPDFEN3Jvq8dyI6r5uKc9kgfsTiJkyeULjk613RBAT0apkr08GOyhaSe94A==
react-proxy@^1.1.7:
version "1.1.8"You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit e26bec4. Configure here.
Code Review: chore: bump react_on_rails to 16.7.0.rc.0OverviewThis PR bumps Blocking Issues1. yarn.lock is stale (inline comment on package.json) 2. Gemfile.lock was manually edited, not regenerated (inline comment on Gemfile.lock) Non-blocking Observations
SummaryThe intent of the PR is correct, but the two lockfile issues need to be resolved before merge to avoid broken CI and broken |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…#3366) (#3368) ## Summary Fixes [#3366](#3366) — a 16.7.0-rc.0 regression where the RSC client manifest (`react-client-manifest.json`) is silently dropped from the Pro Node Renderer upload, breaking server-component rendering with `ENOENT` on the renderer side. - Add a side-effect import of `react-on-rails-rsc/client.browser` at the top of `wrapServerComponentRenderer/client.tsx`. RSCWebpackPlugin scans every parsed module's resource path for an exact match against `require.resolve("../client.browser.js")` and only emits `react-client-manifest.json` when it finds one. Previously the client runtime was reachable only through a three-level transitive chain (`wrapServerComponentRenderer/client` → `getReactServerComponent.client` → `react-on-rails-rsc/client.browser`); any tooling that severed a link in that chain silently dropped the manifest and produced the misleading `Client runtime at react-on-rails-rsc/client was not found` warning. The direct import keeps the runtime resource in the client module graph regardless of how downstream tooling handles transitive default imports. - Add a structural regression test that fails if the side-effect import is removed from either the source or the compiled lib output. - Add a webpack-level regression test that compiles `wrapServerComponentRenderer/client.tsx` with the old transitive `getReactServerComponent.client` edge replaced by a stub, then asserts `RSCWebpackPlugin` still emits `react-client-manifest.json`. This proves the direct import itself preserves the manifest. ## Why Issue #3366 reports that the warning + missing manifest first appears when the consuming app upgrades to `react_on_rails_pro` / `react-on-rails-pro` / `react-on-rails-pro-node-renderer` `16.7.0-rc.0` from `16.6.0`, even with `react-on-rails-rsc@19.0.4` and `shakapacker@10.0.0` unchanged. The actual chain in source/lib is intact between both versions, and the issue does not reproduce on the in-tree dummy app or a clean clone of the tutorial PR — but the user's environment clearly does see `clientFileNameFound = false` from the upstream RSC plugin. The defensive direct import removes that fragility: even if a future transpiler/tree-shaker decides `getReactServerComponent.client` is "unused" or rewrites the path, the side-effect import keeps `client.browser` in the graph and the plugin always emits the manifest. ## Evaluation Confirmed the fix addresses the bug mechanism. The RSC plugin decides whether to write `react-client-manifest.json` by seeing the `react-on-rails-rsc/client.browser` module in the client compilation. The direct side-effect import is an appropriate fix because it changes build graph visibility without changing renderer runtime behavior. I verified the new behavior test is meaningful by temporarily removing the direct import: with the old transitive helper path stubbed out, webpack emitted the expected `Client runtime at react-on-rails-rsc/client was not found` warning and the test failed. Restoring the import made the same test pass and emit `react-client-manifest.json`. The Pro dummy build also emits both `react-client-manifest.json` and `react-server-client-manifest.json` successfully. ## Test plan - [x] `pnpm --filter react-on-rails-pro exec jest tests/wrapServerComponentRenderer.client.manifest.test.js --runInBand` passes - [x] Red check: temporarily removing `import 'react-on-rails-rsc/client.browser';` makes the new manifest test fail with the upstream missing-client-runtime warning - [x] `pnpm --filter react-on-rails-pro exec jest tests/wrapServerComponentRenderer.client.manifest.test.js tests/wrapServerComponentRenderer.client.imports.test.ts --runInBand` passes - [x] `pnpm --filter react-on-rails-pro exec jest tests/createReactOnRailsPro.test.ts --runInBand` passes - [x] `pnpm --filter react-on-rails-pro run test:non-rsc` passes: 7 suites, 58 tests - [x] `pnpm --filter react-on-rails-pro run type-check` passes - [x] `pnpm run lint` passes - [x] `(cd react_on_rails && bundle exec rubocop)` passes - [x] `(cd react_on_rails_pro && bundle exec rubocop --ignore-parent-exclusion)` passes; RuboCop prints deprecation warnings from dependencies but reports no offenses - [x] `pnpm exec prettier --check packages/react-on-rails-pro/tests/wrapServerComponentRenderer.client.manifest.test.js packages/react-on-rails-pro/tests/fixtures/rsc-manifest/getReactServerComponent.client.stub.ts` passes - [x] `RAILS_ENV=test NODE_ENV=development bundle exec bin/shakapacker` from `react_on_rails_pro/spec/dummy` passes and emits `react-client-manifest.json` plus `react-server-client-manifest.json` - [x] `git diff --check` passes - [x] Pre-commit and pre-push hooks passed while committing and pushing `3013f9d90` - [ ] Full-repo `pnpm start format.listDifferent` is blocked by an existing ignored `.context/plans/issue-3366-reproduction-plan.md` formatting issue; the PR files pass direct Prettier checks - [ ] Verify CI passes on this PR - [ ] Verify [shakacode/react-webpack-rails-tutorial#740](shakacode/react-webpack-rails-tutorial#740) unblocks once a new RC build with this fix is published 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches Pro RSC client entrypoint bundling to force inclusion of the RSC client runtime, which can affect build output and hydration behavior if misconfigured. Added tests reduce regression risk, but changes are in a critical RSC packaging path. > > **Overview** > Restores reliable React Server Components hydration for Pro by adding a **top-level side-effect import** of `react-on-rails-rsc/client.browser` in `wrapServerComponentRenderer/client.tsx`, ensuring `RSCWebpackPlugin` consistently emits `react-client-manifest.json` even when transitive imports are disrupted. > > Adds regression coverage: a structural test that enforces the presence of the side-effect import (source and built `lib` when available) and a webpack-level test that stubs `getReactServerComponent.client` to prove manifest emission still occurs. Updates `knip.ts` to ignore dynamically referenced test fixtures and records the fix in `CHANGELOG.md`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1bb24cc. 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** * Restored inclusion of the client runtime so the client manifest is emitted, preventing React Server Components hydration failures in certain build setups. * **Tests** * Added regression tests to ensure the client runtime import is preserved and the client manifest is emitted during bundling. * **Documentation** * Updated CHANGELOG with the fix. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/shakacode/react_on_rails/pull/3368?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#3366) (#3368) ## Summary Fixes [#3366](#3366) — a 16.7.0-rc.0 regression where the RSC client manifest (`react-client-manifest.json`) is silently dropped from the Pro Node Renderer upload, breaking server-component rendering with `ENOENT` on the renderer side. - Add a side-effect import of `react-on-rails-rsc/client.browser` at the top of `wrapServerComponentRenderer/client.tsx`. RSCWebpackPlugin scans every parsed module's resource path for an exact match against `require.resolve("../client.browser.js")` and only emits `react-client-manifest.json` when it finds one. Previously the client runtime was reachable only through a three-level transitive chain (`wrapServerComponentRenderer/client` → `getReactServerComponent.client` → `react-on-rails-rsc/client.browser`); any tooling that severed a link in that chain silently dropped the manifest and produced the misleading `Client runtime at react-on-rails-rsc/client was not found` warning. The direct import keeps the runtime resource in the client module graph regardless of how downstream tooling handles transitive default imports. - Add a structural regression test that fails if the side-effect import is removed from either the source or the compiled lib output. - Add a webpack-level regression test that compiles `wrapServerComponentRenderer/client.tsx` with the old transitive `getReactServerComponent.client` edge replaced by a stub, then asserts `RSCWebpackPlugin` still emits `react-client-manifest.json`. This proves the direct import itself preserves the manifest. ## Why Issue #3366 reports that the warning + missing manifest first appears when the consuming app upgrades to `react_on_rails_pro` / `react-on-rails-pro` / `react-on-rails-pro-node-renderer` `16.7.0-rc.0` from `16.6.0`, even with `react-on-rails-rsc@19.0.4` and `shakapacker@10.0.0` unchanged. The actual chain in source/lib is intact between both versions, and the issue does not reproduce on the in-tree dummy app or a clean clone of the tutorial PR — but the user's environment clearly does see `clientFileNameFound = false` from the upstream RSC plugin. The defensive direct import removes that fragility: even if a future transpiler/tree-shaker decides `getReactServerComponent.client` is "unused" or rewrites the path, the side-effect import keeps `client.browser` in the graph and the plugin always emits the manifest. ## Evaluation Confirmed the fix addresses the bug mechanism. The RSC plugin decides whether to write `react-client-manifest.json` by seeing the `react-on-rails-rsc/client.browser` module in the client compilation. The direct side-effect import is an appropriate fix because it changes build graph visibility without changing renderer runtime behavior. I verified the new behavior test is meaningful by temporarily removing the direct import: with the old transitive helper path stubbed out, webpack emitted the expected `Client runtime at react-on-rails-rsc/client was not found` warning and the test failed. Restoring the import made the same test pass and emit `react-client-manifest.json`. The Pro dummy build also emits both `react-client-manifest.json` and `react-server-client-manifest.json` successfully. ## Test plan - [x] `pnpm --filter react-on-rails-pro exec jest tests/wrapServerComponentRenderer.client.manifest.test.js --runInBand` passes - [x] Red check: temporarily removing `import 'react-on-rails-rsc/client.browser';` makes the new manifest test fail with the upstream missing-client-runtime warning - [x] `pnpm --filter react-on-rails-pro exec jest tests/wrapServerComponentRenderer.client.manifest.test.js tests/wrapServerComponentRenderer.client.imports.test.ts --runInBand` passes - [x] `pnpm --filter react-on-rails-pro exec jest tests/createReactOnRailsPro.test.ts --runInBand` passes - [x] `pnpm --filter react-on-rails-pro run test:non-rsc` passes: 7 suites, 58 tests - [x] `pnpm --filter react-on-rails-pro run type-check` passes - [x] `pnpm run lint` passes - [x] `(cd react_on_rails && bundle exec rubocop)` passes - [x] `(cd react_on_rails_pro && bundle exec rubocop --ignore-parent-exclusion)` passes; RuboCop prints deprecation warnings from dependencies but reports no offenses - [x] `pnpm exec prettier --check packages/react-on-rails-pro/tests/wrapServerComponentRenderer.client.manifest.test.js packages/react-on-rails-pro/tests/fixtures/rsc-manifest/getReactServerComponent.client.stub.ts` passes - [x] `RAILS_ENV=test NODE_ENV=development bundle exec bin/shakapacker` from `react_on_rails_pro/spec/dummy` passes and emits `react-client-manifest.json` plus `react-server-client-manifest.json` - [x] `git diff --check` passes - [x] Pre-commit and pre-push hooks passed while committing and pushing `3013f9d90` - [ ] Full-repo `pnpm start format.listDifferent` is blocked by an existing ignored `.context/plans/issue-3366-reproduction-plan.md` formatting issue; the PR files pass direct Prettier checks - [ ] Verify CI passes on this PR - [ ] Verify [shakacode/react-webpack-rails-tutorial#740](shakacode/react-webpack-rails-tutorial#740) unblocks once a new RC build with this fix is published 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches Pro RSC client entrypoint bundling to force inclusion of the RSC client runtime, which can affect build output and hydration behavior if misconfigured. Added tests reduce regression risk, but changes are in a critical RSC packaging path. > > **Overview** > Restores reliable React Server Components hydration for Pro by adding a **top-level side-effect import** of `react-on-rails-rsc/client.browser` in `wrapServerComponentRenderer/client.tsx`, ensuring `RSCWebpackPlugin` consistently emits `react-client-manifest.json` even when transitive imports are disrupted. > > Adds regression coverage: a structural test that enforces the presence of the side-effect import (source and built `lib` when available) and a webpack-level test that stubs `getReactServerComponent.client` to prove manifest emission still occurs. Updates `knip.ts` to ignore dynamically referenced test fixtures and records the fix in `CHANGELOG.md`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1bb24cc. 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** * Restored inclusion of the client runtime so the client manifest is emitted, preventing React Server Components hydration failures in certain build setups. * **Tests** * Added regression tests to ensure the client runtime import is preserved and the client manifest is emitted during bundling. * **Documentation** * Updated CHANGELOG with the fix. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/shakacode/react_on_rails/pull/3368?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
justin808
changed the title
Code Review — chore: update React on Rails and Shakapacker RC test matrixOverviewThis PR bumps Discrepancy: Shakapacker not updatedThe PR description dedicates a full section to "Shakapacker 10.1.0 RC" and states:
However the actual diff shows:
This makes the PR description misleading for reviewers. If the Shakapacker update is intentionally deferred (which the description's blocking notes suggest), that section should be reworded to describe what was evaluated but not yet applied rather than framing it as a done item. JWT major-version bump (2.10.2 → 3.2.0) — needs verification
RSC Spec Parser — correctness notesThe new binary parser in
Minor / Non-blocking
SummaryThe dependency upgrades are mechanically straightforward. The main asks before merge:
|
| end | ||
|
|
||
| def discard_blank_frame_lines(body) | ||
| body = body.byteslice(1..) || "".b while body.start_with?("\n") |
There was a problem hiding this comment.
The while modifier with an assignment on the same line is valid Ruby but uncommon enough to cause a double-take. An explicit loop block is easier to scan:
| body = body.byteslice(1..) || "".b while body.start_with?("\n") | |
| def discard_blank_frame_lines(body) | |
| while body.start_with?("\n") | |
| body = body.byteslice(1..) || "".b | |
| end | |
| body | |
| end |
There was a problem hiding this comment.
Leaving this one as-is. The one-line loop is local to the spec helper and is readability/style feedback rather than a correctness issue; the selected code change here was the invalid-hex error path.
| raise "Malformed length-prefixed RSC payload: missing tab separator" unless content_length_hex | ||
|
|
||
| content_start = header_end + 1 | ||
| content_length = Integer(content_length_hex, 16) |
There was a problem hiding this comment.
Integer(content_length_hex, 16) raises ArgumentError on invalid hex input, which bypasses the explicit raise "Malformed …" pattern used on every other error path. Consider wrapping it for a consistent failure message:
| content_length = Integer(content_length_hex, 16) | |
| content_length = Integer(content_length_hex.strip, 16) |
Or wrap with rescue:
content_length = Integer(content_length_hex, 16)
rescue ArgumentError
raise "Malformed length-prefixed RSC payload: invalid hex length '#{content_length_hex}'"There was a problem hiding this comment.
Handled in 39e46b9. Invalid hex content lengths now go through parse_content_length and raise the explicit malformed-payload error, with a regression example covering the zz frame length case.
|
|
||
| [ | ||
| parse_payload_metadata(metadata_json, content), | ||
| body.byteslice(content_start + content_length, body.bytesize) || "".b |
There was a problem hiding this comment.
byteslice(offset, length) takes a length as its second argument, not an end index. Passing body.bytesize works (it's large enough to capture everything remaining) but reads as if it were an end index. The endless-range form is both correct and self-documenting:
| body.byteslice(content_start + content_length, body.bytesize) || "".b | |
| body.byteslice(content_start + content_length..) || "".b |
There was a problem hiding this comment.
Leaving this as-is. The second argument is intentionally a length large enough to consume the remaining buffer, and the current behavior is correct; I kept the change focused on the malformed-hex error behavior.
| def parse_payload_metadata(metadata_json, content) | ||
| metadata = JSON.parse(metadata_json.force_encoding(Encoding::UTF_8)) | ||
| content.force_encoding(Encoding::UTF_8) | ||
| metadata["html"] = metadata.delete("payloadType") == "object" ? JSON.parse(content) : content |
There was a problem hiding this comment.
The metadata.delete("payloadType") call is doing double duty — removing the key and returning its value for the comparison. This is idiomatic Ruby but non-obvious at first read. A brief comment clarifies the intent:
| metadata["html"] = metadata.delete("payloadType") == "object" ? JSON.parse(content) : content | |
| # Remove payloadType from the hash and use its value to decide how to decode content. | |
| metadata["html"] = metadata.delete("payloadType") == "object" ? JSON.parse(content) : content |
There was a problem hiding this comment.
Leaving this as-is. metadata.delete("payloadType") is confined to a small parser helper and the mutation is intentional; adding a comment here would mostly restate the expression.
justin808
changed the title
|
Addressed the selected review items:
Local validation run:
Local request-spec execution is blocked in this workspace by the missing PostgreSQL test socket, so GitHub Actions should be the source of truth for the full request spec path. |
Code Review: chore: update React on Rails RC and Shakapacker 10.1.0OverviewRoutine dependency upgrade PR that bumps Dependency Changes ✅
Spec Refactor (
|
|
|
||
| def parse_payload_metadata(metadata_json, content) | ||
| metadata = JSON.parse(metadata_json.force_encoding(Encoding::UTF_8)) | ||
| content.force_encoding(Encoding::UTF_8) |
There was a problem hiding this comment.
force_encoding re-labels the bytes as UTF-8 without actually validating or transcoding them. If the server ever returns metadata or content with invalid UTF-8 bytes (e.g. a binary blob accidentally included in a frame), JSON.parse will raise Encoding::InvalidByteSequenceError rather than a protocol-level error, making failures harder to diagnose in CI.
For a spec helper where you control both ends this is low risk, but encode('UTF-8') (or encode('UTF-8', invalid: :replace) for maximum resilience) would catch encoding problems closer to the source:
| content.force_encoding(Encoding::UTF_8) | |
| metadata = JSON.parse(metadata_json.encode(Encoding::UTF_8)) | |
| content.encode(Encoding::UTF_8) |
|
|
||
| [ | ||
| parse_payload_metadata(metadata_json, content), | ||
| body.byteslice(content_start + content_length, body.bytesize) || "".b |
There was a problem hiding this comment.
The second argument to byteslice is a length, not an end index. Passing body.bytesize as the length always overshoots (the method caps at the end of the string), which works correctly but reads like an end-index to anyone unfamiliar with the API. An endless range is more expressive and self-documenting:
| body.byteslice(content_start + content_length, body.bytesize) || "".b | |
| body.byteslice((content_start + content_length)..) || "".b |
Greptile SummaryThis PR bumps
Confidence Score: 4/5Safe to merge; the dependency updates are well-scoped and the spec parser rewrite correctly implements the length-prefixed frame protocol. All dependency bumps are straightforward maintenance and the Gemfile.lock resolves cleanly. The spec-side parser logic handles frame boundaries, truncation, and invalid hex correctly; the one edge case (binary encoding in the error message interpolation) cannot cause test failures under normal inputs. Full integration coverage depends on the CI PostgreSQL path running green after this push. spec/requests/server_components_spec.rb — the new frame parser; the binary-encoding edge case in the error message is minor but worth a quick look. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[response.body.b binary string] --> B[parsed_chunks loop]
B --> C{body empty?}
C -- yes --> D[return chunks]
C -- no --> E[discard_blank_frame_lines]
E --> F{body empty?}
F -- yes --> D
F -- no --> G[parse_length_prefixed_chunk]
G --> H[find newline header_end]
H --> I{found?}
I -- no --> J[raise: missing header newline]
I -- yes --> K[split at tab: metadata_json + hex]
K --> L{tab found?}
L -- no --> M[raise: missing tab separator]
L -- yes --> N[parse_content_length Integer hex 16]
N --> O{valid hex?}
O -- no --> P[raise: invalid hex length]
O -- yes --> Q[byteslice content_length bytes]
Q --> R{bytesize matches?}
R -- no --> S[raise: truncated content]
R -- yes --> T[parse_payload_metadata force UTF-8]
T --> U[return chunk + remaining body]
U --> B
Reviews (1): Last reviewed commit: "Handle invalid RSC frame lengths" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def parse_content_length(content_length_hex) | ||
| Integer(content_length_hex, 16) | ||
| rescue ArgumentError | ||
| raise "Malformed length-prefixed RSC payload: invalid hex length '#{content_length_hex}'" |
There was a problem hiding this comment.
content_length_hex is ASCII-8BIT-encoded (inherited from body.b). Interpolating it into a UTF-8 string literal raises Encoding::CompatibilityError whenever the length field contains bytes above 0x7F, replacing the intended RuntimeError with an unexpected encoding crash. Forcing to UTF-8 before interpolation avoids this.
| raise "Malformed length-prefixed RSC payload: invalid hex length '#{content_length_hex}'" | |
| raise "Malformed length-prefixed RSC payload: invalid hex length '#{content_length_hex.force_encoding(Encoding::UTF_8)}'" |
Updates react_on_rails_pro gem and react-on-rails-pro, react-on-rails-pro-node-renderer npm packages from RC.0 to RC.2.
justin808
force-pushed
the
jg-codex/rc-react-on-rails-16-7-0-rc-0
branch
from
39e46b9 to
02fd3d6
Compare
|
+review-app-deploy |
Review: chore: update React on Rails RC and Shakapacker 10.1.0OverviewSolid dependency-bump PR. The lockfiles are consistent with the manifests. The most substantive change is in Positive notes
Issues1. Transitive Rails 8.1.2 → 8.1.3 bump is undocumented 2. Test coverage for the new frame parser 3. body.byteslice(content_start + content_length..) || "".b4. |
|
|
||
| [ | ||
| parse_payload_metadata(metadata_json, content), | ||
| body.byteslice(content_start + content_length, body.bytesize) || "".b |
There was a problem hiding this comment.
The second argument to byteslice(start, length) is a length, not an end index. Passing body.bytesize is technically correct (it requests more bytes than exist, so Ruby returns what's available), but it reads like an off-by-one error. The endless-range form is unambiguous and idiomatic:
| body.byteslice(content_start + content_length, body.bytesize) || "".b | |
| body.byteslice(content_start + content_length..) || "".b |
| def parse_payload_metadata(metadata_json, content) | ||
| metadata = JSON.parse(metadata_json.force_encoding(Encoding::UTF_8)) | ||
| content.force_encoding(Encoding::UTF_8) | ||
| metadata["html"] = metadata.delete("payloadType") == "object" ? JSON.parse(content) : content |
There was a problem hiding this comment.
When payloadType == "object", metadata["html"] is set to a parsed Ruby Hash/Array — not HTML text. The key name html is inherited from the expect_valid_rsc_payload assertion (chunk.key?("html")), so changing it would require updating the assertion too. A brief inline comment would prevent future readers from being surprised by the dual types:
| metadata["html"] = metadata.delete("payloadType") == "object" ? JSON.parse(content) : content | |
| # "html" holds a String for payloadType "string" and a parsed Hash/Array for payloadType "object" | |
| metadata["html"] = metadata.delete("payloadType") == "object" ? JSON.parse(content) : content |
|
|
||
| expect { parse_length_prefixed_chunk("#{payload}\tzz\ncontent".b) } | ||
| .to raise_error(RuntimeError, "Malformed length-prefixed RSC payload: invalid hex length 'zz'") | ||
| end |
There was a problem hiding this comment.
Good test for the hex-parsing error path. The other three raise paths in parse_length_prefixed_chunk (missing header newline, missing tab separator, truncated content) currently rely on the PostgreSQL-backed integration specs to exercise them indirectly. Consider adding unit tests for those paths here so they're validated without the DB:
it "raises a malformed payload error when the header newline is missing" do
expect { parse_length_prefixed_chunk("no-newline-at-all".b) }
.to raise_error(RuntimeError, "Malformed length-prefixed RSC payload: missing header newline")
end
it "raises a malformed payload error when the tab separator is missing" do
expect { parse_length_prefixed_chunk("{}\ncontent".b) }
.to raise_error(RuntimeError, "Malformed length-prefixed RSC payload: missing tab separator")
end
it "raises a malformed payload error when content is truncated" do
expect { parse_length_prefixed_chunk("{}\tff\nshort".b) }
.to raise_error(RuntimeError, "Malformed length-prefixed RSC payload: truncated content")
end
Code ReviewOverviewThis PR bumps React on Rails Pro and its npm siblings to Dependency changes
Spec changes (
|
| end | ||
|
|
||
| def discard_blank_frame_lines(body) | ||
| body = body.byteslice(1..) || "".b while body.start_with?("\n") |
There was a problem hiding this comment.
The || "".b fallback is unreachable dead code. String#byteslice(1..) returns "" (not nil) when the string is exactly one byte ("\n"), and start_with?("\n") guarantees at least one byte exists before the loop body runs. Consider removing the fallback:
| body = body.byteslice(1..) || "".b while body.start_with?("\n") | |
| body = body.byteslice(1..) while body.start_with?("\n") |
| it "raises a malformed payload error for invalid hex content lengths" do | ||
| payload = { payloadType: "string" }.to_json | ||
|
|
||
| expect { parse_length_prefixed_chunk("#{payload}\tzz\ncontent".b) } | ||
| .to raise_error(RuntimeError, "Malformed length-prefixed RSC payload: invalid hex length 'zz'") | ||
| end |
There was a problem hiding this comment.
The new unit test covers the invalid hex length path, but the other three error branches added in this PR have no direct tests:
"missing header newline"— e.g.parse_length_prefixed_chunk("no-newline-here".b)"missing tab separator"— e.g.parse_length_prefixed_chunk("no-tab\n".b)"truncated content"— e.g.parse_length_prefixed_chunk("{}\tff\nshort".b)(hex says 255 bytes, body only has 5)
The integration tests below exercise the happy path, but these guard clauses can silently regress. Adding three more one-liner expect { ... }.to raise_error cases here would give full coverage of the parser's error surface.
🎉 Deploy Complete!Open Review AppDeployment successful for PR #740, commit 02fd3d6 🎮 Control Plane Console |
](https://app.coderabbit.ai/change-stack/shakacode/react_on_rails/pull/3368?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
✅ Review App DeletedReview app for PR #740 is deleted |
1 participant
Refs shakacode/react_on_rails#3360
Summary
Updates the demo app to the current React on Rails Pro RC package set and Shakapacker 10.1.0.
react_on_rails_proto16.7.0.rc.3.react-on-rails-proandreact-on-rails-pro-node-rendererto16.7.0-rc.3.shakapackergem and npm package to10.1.0.Version Targets
react-on-rails@16.7.0-rc.3react-on-rails-pro@16.7.0-rc.3react-on-rails-pro-node-renderer@16.7.0-rc.3react_on_rails 16.7.0.rc.3,react_on_rails_pro 16.7.0.rc.3shakapacker 10.1.0Notes
react-on-rails-rscremains React-versioned and is not part of the React on Rails product-version bump.JWT.decode/JWT.encodeusage. The resolvedreact_on_rails_pro 16.7.0.rc.3dependency metadata allowsjwt >= 2.5, < 4, and Bundler resolvesjwt 3.2.0.assets_bundler: rspackstill depends on RSC/Rspack compatibility work tracked in feat: RSCRspackPlugin — rspack-native manifest emitter with client-reference injection react_on_rails_rsc#29.Validation
bin/conductor-exec bundle checkbin/conductor-exec yarn install --frozen-lockfilebin/conductor-exec bundle exec rubocop spec/requests/server_components_spec.rbbin/conductor-exec bundle exec ruby -c spec/requests/server_components_spec.rbbin/conductor-exec git diff --checkFull request spec execution requires the test PostgreSQL service; GitHub Actions will run that path after the latest push.
Summary by CodeRabbit
Note
Medium Risk
Release-candidate upgrades touch the SSR/RSC integration and bundler toolchain; the RSC spec change reflects a streaming protocol shift that could break payload validation if production responses diverge from the new frame format.
Overview
Aligns the demo app with React on Rails Pro
16.7.0.rc.3and Shakapacker10.1.0on both Ruby (Gemfile/ lock) and npm (package.json/yarn.lock), including the node renderer stack (e.g. Fastify / pino resolution changes). README version targets are updated to match.The only behavioral test change is in
spec/requests/server_components_spec.rb: RSC response parsing no longer treats each line as JSON and instead reads the length-prefixed streaming frame format (metadata JSON, tab, hex length, newline, then binary content), mapspayloadTypeto parsedhtml, and adds coverage for invalid hex frame lengths.Reviewed by Cursor Bugbot for commit 02fd3d6. Bugbot is set up for automated code reviews on this repo. Configure here.