Fix upload-assets endpoint duplicating bundles across directories#2768
Fix upload-assets endpoint duplicating bundles across directories#2768AbanoubGhadban merged 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughParses multipart form keys into per-bundle Changes
Sequence Diagram(s)sequenceDiagram
participant Ruby as Ruby (rake/app)
participant Client as HTTP Client
participant Worker as Node Renderer Worker
participant FS as Filesystem
Ruby->>Client: form_with_assets_and_bundle (fields: bundle_<hash>=file, assets=files)
Client->>Worker: POST /upload-assets (multipart)
Worker->>Worker: extractBundlesAndAssets(req.body)
alt providedNewBundles exist
Worker->>Worker: handleNewBundlesProvided(taskDesc, providedNewBundles, assetsToCopy)
Worker->>FS: place each bundle into its matching bundle directory
Worker->>FS: copy shared assets into all target directories
else no bundles found
Worker-->>Client: 4xx validation error
end
Worker-->>Client: ResponseResult (success/failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review: Fix upload-assets endpoint duplicating bundles across directoriesOverall this is a solid fix — the root cause is correctly identified and the DRY refactor is clean. A few concerns worth addressing before merge: Breaking protocol change without a version bumpThe The codebase has a
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| ### [Unreleased] | ||
|
|
||
| #### Fixed |
There was a problem hiding this comment.
The PR link says PR 2767 but this PR is 2768. Please update the CHANGELOG entry.
| #### Fixed | |
| - **[Pro] Fixed bundle duplication in remote node renderer asset uploads**: When RSC support is enabled, running `rake react_on_rails_pro:copy_assets_to_remote_vm_renderer` no longer duplicates bundle JS files across bundle directories. Previously, both the server bundle and RSC bundle were copied into every target directory; now each bundle is placed only in its own directory while shared assets (manifests, stats) are correctly distributed to all. [PR 2768](https://github.com/shakacode/react_on_rails/pull/2768) by [AbanoubGhadban](https://github.com/AbanoubGhadban). Fixes [Issue 2766](https://github.com/shakacode/react_on_rails/issues/2766). |
|
|
||
| // There can be additional files that might be required at the runtime. | ||
| // Since the remote renderer doesn't contain any assets, they must be uploaded manually. | ||
| // Bundle files use the form key convention "bundle_<hash>" and are placed in |
There was a problem hiding this comment.
The Body type Record<string, Asset> is inaccurate — scalar fields like gemVersion, protocolVersion, railsEnv, and password are also present in the body. The runtime isAsset() filtering handles this correctly, but the type annotation is misleading.
| // Bundle files use the form key convention "bundle_<hash>" and are placed in | |
| Body: Record<string, unknown>; |
| const firstFailure = results.find((r): r is PromiseRejectedResult => r.status === 'rejected'); | ||
| if (firstFailure) { | ||
| throw firstFailure.reason; | ||
| try { |
There was a problem hiding this comment.
handleNewBundlesProvided's first parameter is named renderingRequest (a JSON blob in the render path). Here taskDescription is passed instead — a human-readable string. This is functional, but any error messages or log lines in handleNewBundlesProvided that reference "renderingRequest" will surface this task description string, which can be confusing when debugging. Consider renaming the parameter to something context-neutral (e.g. context or requestContext) in handleRenderRequest.ts.
| target_bundles = [pool.server_bundle_hash] | ||
|
|
||
| # Add RSC bundle if enabled | ||
| if ReactOnRailsPro.configuration.enable_rsc_support |
There was a problem hiding this comment.
This existence check is now redundant: form_with_assets_and_bundle → populate_form_with_bundle_and_assets(check_bundle: true) → add_bundle_to_form already raises ReactOnRailsPro::Error, "Bundle not found #{bundle_path}" when the file is missing. The early check provides a friendlier error message (worth keeping), but a brief comment would help future readers understand why the check is intentionally duplicated.
size-limit report 📦
|
![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: 50290afe01
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| perform_request("/upload-assets", form: form) | ||
| # The node renderer derives target directories from the bundle_<hash> | ||
| # form keys, so no separate targetBundles field is needed. | ||
| perform_request("/upload-assets", form: form_with_assets_and_bundle) |
There was a problem hiding this comment.
Dropping targetBundles here creates a wire-level incompatibility with older node-renderer versions that still advertise protocol 2.0.0 and reject /upload-assets without that field. Because production only warns on gem/package version mismatch (it does not block requests), mixed-version rollouts are a supported scenario; if the gem is upgraded before the remote renderer, upload_assets now fails with 400 and the deployment task cannot sync assets. Please keep sending targetBundles (at least as a compatibility fallback) or bump protocol version if this is an intentional breaking change.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes a bundle duplication bug in the Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Ruby as Ruby Client
participant Node as worker.ts
participant HNBP as handleNewBundlesProvided
participant FS as File System
Ruby->>Node: "POST /upload-assets (bundle_hashA, bundle_hashB, assets)"
Node->>Node: "extractBundlesAndAssets() splits bundles from assets"
Node->>HNBP: "handleNewBundlesProvided(bundles=[hashA,hashB], assets)"
par hashA bundle
HNBP->>FS: "lock(hashA) → move serverBundle → bundleDir/hashA/"
HNBP->>FS: "copy shared assets → bundleDir/hashA/"
HNBP->>FS: "unlock(hashA)"
and hashB bundle
HNBP->>FS: "lock(hashB) → move rscBundle → bundleDir/hashB/"
HNBP->>FS: "copy shared assets → bundleDir/hashB/"
HNBP->>FS: "unlock(hashB)"
end
HNBP-->>Node: "undefined (success)"
Node-->>Ruby: "200 OK"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| ### [Unreleased] | ||
|
|
||
| #### Fixed |
There was a problem hiding this comment.
The changelog entry links to PR 2767, but this PR is #2768. This will create a broken link in the published changelog.
| #### Fixed | |
| - **[Pro] Fixed bundle duplication in remote node renderer asset uploads**: When RSC support is enabled, running `rake react_on_rails_pro:copy_assets_to_remote_vm_renderer` no longer duplicates bundle JS files across bundle directories. Previously, both the server bundle and RSC bundle were copied into every target directory; now each bundle is placed only in its own directory while shared assets (manifests, stats) are correctly distributed to all. [PR 2768](https://github.com/shakacode/react_on_rails/pull/2768) by [AbanoubGhadban](https://github.com/AbanoubGhadban). Fixes [Issue 2766](https://github.com/shakacode/react_on_rails/issues/2766). |
| providedNewBundles.push({ timestamp: primaryBundleTimestamp, bundle: value }); | ||
| } else if (key.startsWith('bundle_')) { | ||
| assertAsset(value, key); | ||
| providedNewBundles.push({ timestamp: key.replace('bundle_', ''), bundle: value }); |
There was a problem hiding this comment.
replace only removes first prefix occurrence
String.prototype.replace(string, string) replaces only the first occurrence of the search string. For a key like bundle_bundle_abc, the derived timestamp would be bundle_abc instead of abc, which would cause the bundle to be placed in the wrong directory.
A more defensive approach is to strip the known prefix with slice (which is also slightly more efficient since no regex or search is needed):
| providedNewBundles.push({ timestamp: key.replace('bundle_', ''), bundle: value }); | |
| providedNewBundles.push({ timestamp: key.slice('bundle_'.length), bundle: value }); |
In practice the Ruby client strictly follows the bundle_<hash> format, so this won't trigger today, but the intent is clearer with slice.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts (1)
345-370: Assert the duplication regression directly.This proves the shared assets were copied, but it never checks that each bundle directory omits the other bundle's JS file—the specific bug this PR is fixing. A couple of negative assertions here would make the regression coverage much tighter.
🤖 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/tests/worker.test.ts` around lines 345 - 370, Add negative assertions to the "post /upload-assets with multiple bundles and assets" test to ensure each bundle directory does NOT contain the other bundle's JS file: after the existing positive expects for assetPath(...) and assetPathOther(...), assert that assetPath(testName, bundleHashOther) and assetPathOther(testName, bundleHashOther) are false for the first bundle's checks and that assetPath(testName, bundleHash) and assetPathOther(testName, bundleHash) are false for the second bundle's checks (use the same assetPath and assetPathOther helpers and bundleHash / bundleHashOther variables).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 29: The changelog entry currently references "[PR 2767]" but the comment
indicates this should point to PR 2768; update the PR link text and URL in the
CHANGELOG.md entry (the "[PR 2767](...)" token) to "[PR
2768](https://github.com/shakacode/react_on_rails/pull/2768)" so the entry links
to the correct pull request while leaving the rest of the paragraph (issue and
author references) unchanged.
In `@react_on_rails_pro/lib/react_on_rails_pro/request.rb`:
- Around line 62-64: The upload-assets wire format changed to derive bundle
targets from bundle_<hash> form keys (see perform_request("/upload-assets",
form: form_with_assets_and_bundle)), so bump the shared protocol version string
used by the gem and the node renderer (currently "2.0.0") to a new version (e.g.
"2.1.0") in both places where protocolVersion is declared so the worker can
detect the format change; update the Ruby protocol constant and the package.json
protocolVersion field to the new value and ensure any code that checks
protocolVersion is comparing against the new version.
---
Nitpick comments:
In `@packages/react-on-rails-pro-node-renderer/tests/worker.test.ts`:
- Around line 345-370: Add negative assertions to the "post /upload-assets with
multiple bundles and assets" test to ensure each bundle directory does NOT
contain the other bundle's JS file: after the existing positive expects for
assetPath(...) and assetPathOther(...), assert that assetPath(testName,
bundleHashOther) and assetPathOther(testName, bundleHashOther) are false for the
first bundle's checks and that assetPath(testName, bundleHash) and
assetPathOther(testName, bundleHash) are false for the second bundle's checks
(use the same assetPath and assetPathOther helpers and bundleHash /
bundleHashOther variables).
🪄 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: a6a8f93d-b10d-476b-b716-9402d9fcf3c9
📒 Files selected for processing (6)
CHANGELOG.mdpackages/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/uploadRaceCondition.test.tspackages/react-on-rails-pro-node-renderer/tests/worker.test.tsreact_on_rails_pro/lib/react_on_rails_pro/request.rb
| # The node renderer derives target directories from the bundle_<hash> | ||
| # form keys, so no separate targetBundles field is needed. | ||
| perform_request("/upload-assets", form: form_with_assets_and_bundle) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files in this PR that changed the /upload-assets contract:"
git diff --name-only origin/main...HEAD -- \
react_on_rails_pro/lib/react_on_rails_pro/request.rb \
packages/react-on-rails-pro-node-renderer/src/worker.ts \
packages/react-on-rails-pro-node-renderer/package.json
echo
echo "Current protocolVersion declarations/usages:"
rg -n 'protocolVersion' packages/react-on-rails-pro-node-renderer react_on_rails_pro
echo
echo "Protocol manifest diff in this PR:"
git diff --unified=0 origin/main...HEAD -- packages/react-on-rails-pro-node-renderer/package.jsonRepository: shakacode/react_on_rails
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check git status and available refs
echo "=== Git Status ==="
git status || echo "Not a git repo or status failed"
echo
echo "=== Available refs ==="
git branch -a 2>/dev/null || echo "No branches found"
echo
echo "=== Locating protocolVersion declarations ==="
find . -type f \( -name "*.json" -o -name "*.ts" -o -name "*.js" -o -name "*.rb" \) -exec grep -l 'protocolVersion' {} \; 2>/dev/null | head -20
echo
echo "=== protocolVersion in package.json files ==="
find packages -name "package.json" -exec grep -H 'protocolVersion' {} \; 2>/dev/null
echo
echo "=== protocolVersion in React on Rails Pro files ==="
rg -n 'protocolVersion' react_on_rails_pro 2>/dev/null || echo "No matches in react_on_rails_pro"Repository: shakacode/react_on_rails
Length of output: 1561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Gem PROTOCOL_VERSION constant ==="
grep -n 'PROTOCOL_VERSION' react_on_rails_pro/lib/react_on_rails_pro/utils.rb | head -5
echo
echo "=== Full context around PROTOCOL_VERSION in utils.rb ==="
grep -B 2 -A 2 'PROTOCOL_VERSION' react_on_rails_pro/lib/react_on_rails_pro/utils.rb | head -20
echo
echo "=== request.rb lines 50-75 (context around 62-64) ==="
sed -n '50,75p' react_on_rails_pro/lib/react_on_rails_pro/request.rb
echo
echo "=== Check for targetBundles in request.rb ==="
grep -n 'targetBundles' react_on_rails_pro/lib/react_on_rails_pro/request.rb || echo "No targetBundles found"
echo
echo "=== Git diff: compare HEAD with origin/main for request.rb ==="
git diff origin/main HEAD -- react_on_rails_pro/lib/react_on_rails_pro/request.rb | head -100Repository: shakacode/react_on_rails
Length of output: 3634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find PROTOCOL_VERSION constant definition ==="
find react_on_rails_pro -type f -name "*.rb" -exec grep -n "PROTOCOL_VERSION\s*=" {} +
echo
echo "=== Check for version file or constants ==="
ls -la react_on_rails_pro/lib/react_on_rails_pro/*.rb | head -20
echo
echo "=== Check config or constants file ==="
cat react_on_rails_pro/lib/react_on_rails_pro/version.rb 2>/dev/null || echo "No version.rb"
echo
echo "=== worker.ts handling of bundle_<hash> fields ==="
grep -n -A 5 'bundle_' packages/react-on-rails-pro-node-renderer/src/worker.ts | head -50
echo
echo "=== Compare versions: package.json vs origin/main ==="
git show origin/main:packages/react-on-rails-pro-node-renderer/package.json | grep -i version | head -5
echo
echo "=== Current package.json version ==="
cat packages/react-on-rails-pro-node-renderer/package.json | grep -i version | head -5Repository: shakacode/react_on_rails
Length of output: 3788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Git diff for version.rb (gem PROTOCOL_VERSION) ==="
git diff origin/main HEAD -- react_on_rails_pro/lib/react_on_rails_pro/version.rb
echo
echo "=== Git diff for node-renderer package.json (protocolVersion) ==="
git diff origin/main HEAD -- packages/react-on-rails-pro-node-renderer/package.json | head -50
echo
echo "=== Check when these protocol versions were introduced ==="
echo "Gem PROTOCOL_VERSION history:"
git log --oneline -p origin/main -- react_on_rails_pro/lib/react_on_rails_pro/version.rb | grep -A 2 "PROTOCOL_VERSION" | head -20
echo
echo "=== Check if targetBundles was removed on origin/main ==="
git show origin/main:react_on_rails_pro/lib/react_on_rails_pro/request.rb | grep -n "targetBundles" | head -5Repository: shakacode/react_on_rails
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Origin/main: gem PROTOCOL_VERSION ==="
git show origin/main:react_on_rails_pro/lib/react_on_rails_pro/version.rb 2>/dev/null | grep PROTOCOL_VERSION
echo
echo "=== Origin/main: node-renderer protocolVersion ==="
git show origin/main:packages/react-on-rails-pro-node-renderer/package.json 2>/dev/null | grep protocolVersion
echo
echo "=== Current HEAD: gem PROTOCOL_VERSION ==="
cat react_on_rails_pro/lib/react_on_rails_pro/version.rb | grep PROTOCOL_VERSION
echo
echo "=== Current HEAD: node-renderer protocolVersion ==="
cat packages/react-on-rails-pro-node-renderer/package.json | grep protocolVersion
echo
echo "=== Did targetBundles exist on origin/main in request.rb? ==="
git show origin/main:react_on_rails_pro/lib/react_on_rails_pro/request.rb 2>/dev/null | grep -c "targetBundles" || echo "0"
echo
echo "=== Current HEAD: does targetBundles still exist in request.rb? ==="
grep -c "targetBundles" react_on_rails_pro/lib/react_on_rails_pro/request.rb || echo "0 (outside upload_assets)"Repository: shakacode/react_on_rails
Length of output: 512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Did worker.ts change in this PR? Check for bundle_ parsing ==="
git show origin/main:packages/react-on-rails-pro-node-renderer/src/worker.ts 2>/dev/null | grep -n "bundle_" | head -10
echo
echo "=== Current HEAD worker.ts bundle_ parsing ==="
grep -n "bundle_" packages/react-on-rails-pro-node-renderer/src/worker.ts | head -10
echo
echo "=== Check if origin/main worker expected targetBundles ==="
git show origin/main:packages/react-on-rails-pro-node-renderer/src/worker.ts 2>/dev/null | grep -n "targetBundles" | head -5
echo
echo "=== Check if HEAD worker still references targetBundles ==="
grep -n "targetBundles" packages/react-on-rails-pro-node-renderer/src/worker.ts | head -5Repository: shakacode/react_on_rails
Length of output: 1618
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== HEAD worker.ts: /upload-assets endpoint full context (lines 380-415) ==="
sed -n '380,415p' packages/react-on-rails-pro-node-renderer/src/worker.ts
echo
echo "=== HEAD worker.ts: how targetBundles is used in /upload-assets ==="
sed -n '380,430p' packages/react-on-rails-pro-node-renderer/src/worker.ts | grep -A 50 "targetBundles" | head -40
echo
echo "=== Check if /upload-assets actually uses targetBundles or ignores it ==="
sed -n '334,380p' packages/react-on-rails-pro-node-renderer/src/worker.tsRepository: shakacode/react_on_rails
Length of output: 4991
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Origin/main /upload-assets endpoint (lines 317-360) ==="
git show origin/main:packages/react-on-rails-pro-node-renderer/src/worker.ts 2>/dev/null | sed -n '317,360p'
echo
echo "=== Verify: does new /upload-assets check for or use targetBundles? ==="
grep -A 30 "'/upload-assets'" packages/react-on-rails-pro-node-renderer/src/worker.ts | grep -i "targetBundles" || echo "Not found in /upload-assets"Repository: shakacode/react_on_rails
Length of output: 2354
Verify and bump the protocol version for the /upload-assets wire-format change.
This PR changes the /upload-assets endpoint to derive bundle targets from bundle_<hash> form keys instead of the explicit targetBundles field. The old node renderer requires targetBundles and will reject requests lacking it—if a new gem is deployed with an older node renderer, /upload-assets will fail with "No targetBundles provided."
Both react_on_rails_pro/lib/react_on_rails_pro/version.rb and packages/react-on-rails-pro-node-renderer/package.json still declare protocolVersion = "2.0.0", but the protocols are now incompatible. Increment the protocol version in both files to signal to the worker that the request format has changed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react_on_rails_pro/lib/react_on_rails_pro/request.rb` around lines 62 - 64,
The upload-assets wire format changed to derive bundle targets from
bundle_<hash> form keys (see perform_request("/upload-assets", form:
form_with_assets_and_bundle)), so bump the shared protocol version string used
by the gem and the node renderer (currently "2.0.0") to a new version (e.g.
"2.1.0") in both places where protocolVersion is declared so the worker can
detect the format change; update the Ruby protocol constant and the package.json
protocolVersion field to the new value and ensure any code that checks
protocolVersion is comparing against the new version.
|
|
||
| ### [Unreleased] | ||
|
|
||
| #### Fixed |
There was a problem hiding this comment.
The PR link here says PR 2767 but this PR is #2768.
| #### Fixed | |
| - **[Pro] Fixed bundle duplication in remote node renderer asset uploads**: When RSC support is enabled, running `rake react_on_rails_pro:copy_assets_to_remote_vm_renderer` no longer duplicates bundle JS files across bundle directories. Previously, both the server bundle and RSC bundle were copied into every target directory; now each bundle is placed only in its own directory while shared assets (manifests, stats) are correctly distributed to all. [PR 2768](https://github.com/shakacode/react_on_rails/pull/2768) by [AbanoubGhadban](https://github.com/AbanoubGhadban). Fixes [Issue 2766](https://github.com/shakacode/react_on_rails/issues/2766). |
| Body: Record<string, Asset>; | ||
| }>('/upload-assets', async (req, res) => { | ||
| if (!(await requestPrechecks(req, res))) { | ||
| return; |
There was a problem hiding this comment.
This is a breaking protocol change — the endpoint no longer accepts targetBundles and now requires bundle_<hash> fields. The protocol version in package.json is still 2.0.0, so a checkProtocolVersion call won't catch a gem/renderer version mismatch. If a user updates only the node renderer (not the gem), they'll get a confusing 400 "No bundle_ fields provided" instead of a clear protocol-version error.
Consider bumping the minor protocol version (e.g. 2.1.0) so that the existing version-check middleware can surface a clear "gem/renderer version mismatch" message.
| if ReactOnRailsPro.configuration.enable_rsc_support | ||
| rsc_bundle_path = ReactOnRailsPro::Utils.rsc_bundle_js_file_path | ||
| unless File.exist?(rsc_bundle_path) | ||
| raise ReactOnRailsPro::Error, "RSC bundle not found at #{rsc_bundle_path}. " \ | ||
| "Please build your bundles before uploading assets." | ||
| end | ||
| target_bundles << pool.rsc_bundle_hash | ||
| end |
There was a problem hiding this comment.
This block is now redundant. form_with_assets_and_bundle → populate_form_with_bundle_and_assets(form, check_bundle: true) → add_bundle_to_form(..., check_bundle: true) already raises ReactOnRailsPro::Error, "Bundle not found #{rsc_bundle_path}" if the RSC bundle is missing.
The only visible difference is the error message (this block produces a more descriptive message). If the better message is important, consider moving the check to add_bundle_to_form instead of duplicating it here. Otherwise this block can be removed entirely.
|
Review summary posted via inline comments. Top-level notes: (1) CHANGELOG references PR 2767 instead of 2768. (2) The /upload-assets protocol change (removing targetBundles, requiring bundle_hash fields) is breaking but the protocol version was not bumped from 2.0.0 — old gem + new renderer will get a confusing 400 rather than a version-mismatch error. Consider bumping to 2.1.0. (3) The RSC bundle existence check in upload_assets (request.rb lines 54-60) is now redundant since form_with_assets_and_bundle already checks it via check_bundle: true. (4) handleNewBundlesProvided runs handleNewBundleProvided concurrently for each bundle, all sharing the same assetsToCopy file paths. Safe today since copyUploadedAssets uses fs-extra copy (reads not moves), but worth a comment near Promise.allSettled. Overall the fix is correct: extractBundlesAndAssets cleanly extracts the bundle-vs-asset classification, moving copyUploadedAssets outside the EEXIST try/catch makes asset copying idempotent, and reusing handleNewBundlesProvided eliminates the duplicated lock logic in worker.ts. |
| providedNewBundles.push({ timestamp: primaryBundleTimestamp, bundle: value }); | ||
| } else if (key.startsWith('bundle_')) { | ||
| assertAsset(value, key); | ||
| providedNewBundles.push({ timestamp: key.slice('bundle_'.length), bundle: value }); |
There was a problem hiding this comment.
Edge case: empty timestamp from bundle_ with no suffix
If a form field is sent with the key "bundle_" (hash part omitted), key.slice('bundle_'.length) produces "". This empty string silently becomes the timestamp, which will likely create a bundle directory at an unexpected path or overwrite something unrelated.
Consider adding a guard:
| providedNewBundles.push({ timestamp: key.slice('bundle_'.length), bundle: value }); | |
| } else if (key.startsWith('bundle_') && key.length > 'bundle_'.length) { |
Or validate after the loop and return an error if any extracted timestamp is empty.
| } else if (key.startsWith('bundle_')) { | ||
| assertAsset(value, key); | ||
| providedNewBundles.push({ timestamp: key.slice('bundle_'.length), bundle: value }); | ||
| } else if (isAsset(value)) { |
There was a problem hiding this comment.
Naming collision risk with shared asset files
Any multipart field whose key starts with bundle_ is treated as a bundle, not a shared asset. If a user ever configures an assets_to_copy entry whose filename begins with bundle_ (e.g., a Webpack stats file named bundle_stats.json), it would be silently misclassified as a bundle — its content would be moved to a per-hash directory rather than copied to all bundle directories as an asset.
This is a non-obvious constraint that should be documented (e.g., in AGENTS.md or a JSDoc on extractBundlesAndAssets) so callers know asset filenames must not start with bundle_.
| // Always copy assets to the bundle directory — even if the bundle was | ||
| // already present (e.g., from a prior upload or another worker). | ||
| // copyUploadedAssets uses overwrite:true, so this is idempotent. | ||
| if (assetsToCopy) { | ||
| await copyUploadedAssets(assetsToCopy, bundleDirectory); | ||
| } |
There was a problem hiding this comment.
"Always copy" comment is slightly misleading for the lock-failure path
The comment says "Always copy assets … even if the bundle was already present", but assets are not copied when the lock cannot be acquired (line 102 returns an error before reaching this block). The "always" refers specifically to the case where the bundle file already exists on disk (file-exists catch branch at line 113), not to the lock-failure case.
Worth clarifying the comment so future readers don't mistakenly believe lock failure still copies assets:
| // Always copy assets to the bundle directory — even if the bundle was | |
| // already present (e.g., from a prior upload or another worker). | |
| // copyUploadedAssets uses overwrite:true, so this is idempotent. | |
| if (assetsToCopy) { | |
| await copyUploadedAssets(assetsToCopy, bundleDirectory); | |
| } | |
| // Copy assets to the bundle directory — even if the bundle was already | |
| // present on disk (e.g., from a prior upload or another worker). | |
| // NOTE: If lock acquisition failed above, we return early and assets are | |
| // not copied for that bundle. | |
| // copyUploadedAssets uses overwrite:true, so this is idempotent. | |
| if (assetsToCopy) { | |
| await copyUploadedAssets(assetsToCopy, bundleDirectory); | |
| } |
|
Overall this is a solid, well-motivated refactor. The root bug (all uploaded files being blindly copied into every bundle directory) is real, the fix is correct, and extracting Substantive issues 1. Empty-timestamp edge case in 2. 3. "Always copy" comment is misleading for the lock-failure path (see inline): The new comment in Minor observation
Positive highlights Moving |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
2 similar comments
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
) The /upload-assets endpoint treated all uploaded files as a flat list and copied every file into every target bundle directory. When RSC support sends both server and RSC bundles, each bundle JS was duplicated in the other's directory. - Extract shared `extractBundlesAndAssets` helper that parses form body into bundles (bundle_<hash> keys) and shared assets, used by both the render and /upload-assets endpoints - Rewrite /upload-assets to reuse `handleNewBundlesProvided` from the render path, ensuring each bundle goes only to its own directory while shared assets are distributed to all directories - Remove redundant `targetBundles` form field from /upload-assets (directories are now derived from bundle_<hash> keys) - Update Ruby side to stop sending targetBundles for upload_assets Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When handleNewBundleProvided encounters an existing bundle (EEXIST), it previously skipped copying assets. This caused the upload_asset_spec integration test to fail because a pre-existing bundle meant assets were never written. Since copyUploadedAssets uses overwrite:true, always copying is idempotent and safe. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix CHANGELOG PR link from 2767 to 2768 - Bump protocol version to 2.1.0 in both gem and node-renderer (wire-format change: /upload-assets no longer uses targetBundles) - Fix Body type to Record<string, unknown> (matches actual form contents) - Use slice() instead of replace() for bundle_ prefix stripping - Add comment explaining intentionally duplicated bundle existence checks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
A form field keyed exactly "bundle_" (no hash) could indicate a bug in the Ruby client returning an empty bundle hash. Log a warning and drop the field rather than creating a directory with an empty name. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The parameter is used for error message context, not as actual JS code. Renaming makes it accurate for both render (JS code) and upload-assets (task description) callers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| import packageJson from '../package.json'; | ||
| import worker, { disableHttp2 } from '../src/worker'; | ||
| import { resetForTest, serverBundleCachePath, getFixtureBundle } from './helper'; | ||
| import { resetForTest, serverBundleCachePath, getFixtureBundle, getFixtureSecondaryBundle } from './helper'; |
There was a problem hiding this comment.
Unused import
getFixtureSecondaryBundle is imported here but never referenced anywhere in this file (it's only used in worker.test.ts). This will produce a lint warning (no-unused-vars) and should be removed.
| import { resetForTest, serverBundleCachePath, getFixtureBundle, getFixtureSecondaryBundle } from './helper'; | |
| import { resetForTest, serverBundleCachePath, getFixtureBundle } from './helper'; |
ReviewThe core fix is correct and well-motivated: the old A few issues worth addressing before merge: Security (medium)Path traversal via Code quality (medium)
Test / minor
Minor observations
|
|
/run-skipped-ci |
|
🚀 Full CI Mode Enabled ✅ Triggered all workflows for full CI coverage Note: Added the To disable full CI mode, use the View progress in the Actions tab. |
…olve-2833 * origin/main: docs: fix profiling node renderer command (#2863) generators: point Pro install fallback to upgrade guide (#2868) Add RSC Flight payload optimization guide (Article 7) (#2827) Migrate from deprecated Async::Variable to Async::Promise (#2832) docs: turn pro quick start into a gateway (#2862) Fix upload-assets endpoint duplicating bundles across directories (#2768) docs: fix stale docs links and help URLs (#2850) docs: replace dead pro.reactonrails.com links (#2851) docs: refresh generator and helper URLs (#2852) Add standalone RSC upgrade guide for existing Pro apps (#2831) Raise docs version floor to 16.4.0 in install/demo guidance (#2610) # Conflicts: # CHANGELOG.md
…olve-2834 * origin/main: docs: fix profiling node renderer command (#2863) generators: point Pro install fallback to upgrade guide (#2868) Add RSC Flight payload optimization guide (Article 7) (#2827) Migrate from deprecated Async::Variable to Async::Promise (#2832) docs: turn pro quick start into a gateway (#2862) Fix upload-assets endpoint duplicating bundles across directories (#2768) docs: fix stale docs links and help URLs (#2850) docs: replace dead pro.reactonrails.com links (#2851) docs: refresh generator and helper URLs (#2852) # Conflicts: # CHANGELOG.md # react_on_rails/lib/react_on_rails/utils.rb
…olve-2849 * origin/main: docs: fix profiling node renderer command (#2863) generators: point Pro install fallback to upgrade guide (#2868) Add RSC Flight payload optimization guide (Article 7) (#2827) Migrate from deprecated Async::Variable to Async::Promise (#2832) docs: turn pro quick start into a gateway (#2862) Fix upload-assets endpoint duplicating bundles across directories (#2768) docs: fix stale docs links and help URLs (#2850) docs: replace dead pro.reactonrails.com links (#2851) docs: refresh generator and helper URLs (#2852) Add standalone RSC upgrade guide for existing Pro apps (#2831) Raise docs version floor to 16.4.0 in install/demo guidance (#2610) # Conflicts: # CHANGELOG.md
…olve-2835 * origin/main: (21 commits) docs: fix profiling node renderer command (#2863) generators: point Pro install fallback to upgrade guide (#2868) Add RSC Flight payload optimization guide (Article 7) (#2827) Migrate from deprecated Async::Variable to Async::Promise (#2832) docs: turn pro quick start into a gateway (#2862) Fix upload-assets endpoint duplicating bundles across directories (#2768) docs: fix stale docs links and help URLs (#2850) docs: replace dead pro.reactonrails.com links (#2851) docs: refresh generator and helper URLs (#2852) Add standalone RSC upgrade guide for existing Pro apps (#2831) Raise docs version floor to 16.4.0 in install/demo guidance (#2610) Fix release script: require changelog, fix RC version computation (#2848) Bump version to 16.5.0 Bump version to 16.5.0.rc.0 Update CHANGELOG.md for 16.5.0.rc.0 (#2847) Docs: add memory leak prevention guide for Node Renderer SSR (#2845) Docs: fix RSC migration gaps found during real-world migration (#2842) Add common mistakes sections to RSC migration guides (#2826) fix: preserve runtime env vars across Bundler.with_unbundled_env (#2836) Skip Pro CI workflows for Dependabot PRs (#2825) ... # Conflicts: # CHANGELOG.md
## Summary - Stamped `### [16.5.1] - 2026-03-27` with two Pro fixes (PRs #2872, #2768) - Removed the `### [16.5.0.rc.0]` section (was already released as 16.5.0 stable) - Consolidated the 16.5.0 section with full entries (previously just said "no changes from rc.0") - Updated diff links at bottom of file ## Skipped PRs (docs/internal only) #2856, #2860, #2857, #2859, #2864, #2870, #2863, #2868, #2827, #2862, #2850, #2851, #2852, #2831, #2610, #2848 ## Test plan - [ ] Verify CHANGELOG.md formatting and diff links are correct - [ ] Run `rake release` (no args) after merge to publish 16.5.1 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: changes are limited to `CHANGELOG.md` release notes and version/compare links with no runtime code modifications. > > **Overview** > Documents the `16.5.1` release by adding a new section under *Unreleased* with two **[Pro]** fixes (missing packaged rake tasks and avoiding duplicated bundles during remote renderer asset uploads). > > Cleans up the `16.5.0` entry by removing the `16.5.0.rc.0` section and updating the compare-link footer so `unreleased` now starts from `v16.5.1` and `16.5.0` compares from `v16.4.0`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 9a6b224. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated changelog to document version 16.5.1 release with consolidated version history and updated comparison links. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary - Stamped `### [16.5.1] - 2026-03-27` with two Pro fixes (PRs #2872, #2768) - Removed the `### [16.5.0.rc.0]` section (was already released as 16.5.0 stable) - Consolidated the 16.5.0 section with full entries (previously just said "no changes from rc.0") - Updated diff links at bottom of file ## Skipped PRs (docs/internal only) #2856, #2860, #2857, #2859, #2864, #2870, #2863, #2868, #2827, #2862, #2850, #2851, #2852, #2831, #2610, #2848 ## Test plan - [ ] Verify CHANGELOG.md formatting and diff links are correct - [ ] Run `rake release` (no args) after merge to publish 16.5.1 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: changes are limited to `CHANGELOG.md` release notes and version/compare links with no runtime code modifications. > > **Overview** > Documents the `16.5.1` release by adding a new section under *Unreleased* with two **[Pro]** fixes (missing packaged rake tasks and avoiding duplicated bundles during remote renderer asset uploads). > > Cleans up the `16.5.0` entry by removing the `16.5.0.rc.0` section and updating the compare-link footer so `unreleased` now starts from `v16.5.1` and `16.5.0` compares from `v16.4.0`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 9a6b224. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated changelog to document version 16.5.1 release with consolidated version history and updated comparison links. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ndpoint Three test fixes for API changes from main (PR #2768): 1. vm.test.ts: Replace direct `buildVM()` calls with `buildExecutionContext()` since `buildVM` is no longer exported (now a private function). 2. worker.test.ts: Update empty upload-assets test to expect 400 status since the endpoint now requires at least one `bundle_<hash>` field. 3. worker.test.ts: Update targetBundles directory test since the endpoint no longer creates directories for targetBundles (only bundle_<hash> fields determine directory placement). Also update buildVM spy to use buildExecutionContext which is the exported function. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ndpoint Three test fixes for API changes from main (PR #2768): 1. vm.test.ts: Replace direct `buildVM()` calls with `buildExecutionContext()` since `buildVM` is no longer exported (now a private function). 2. worker.test.ts: Update empty upload-assets test to expect 400 status since the endpoint now requires at least one `bundle_<hash>` field. 3. worker.test.ts: Update targetBundles directory test since the endpoint no longer creates directories for targetBundles (only bundle_<hash> fields determine directory placement). Also update buildVM spy to use buildExecutionContext which is the exported function. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ndpoint Three test fixes for API changes from main (PR #2768): 1. vm.test.ts: Replace direct `buildVM()` calls with `buildExecutionContext()` since `buildVM` is no longer exported (now a private function). 2. worker.test.ts: Update empty upload-assets test to expect 400 status since the endpoint now requires at least one `bundle_<hash>` field. 3. worker.test.ts: Update targetBundles directory test since the endpoint no longer creates directories for targetBundles (only bundle_<hash> fields determine directory placement). Also update buildVM spy to use buildExecutionContext which is the exported function. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ndpoint Three test fixes for API changes from main (PR #2768): 1. vm.test.ts: Replace direct `buildVM()` calls with `buildExecutionContext()` since `buildVM` is no longer exported (now a private function). 2. worker.test.ts: Update empty upload-assets test to expect 400 status since the endpoint now requires at least one `bundle_<hash>` field. 3. worker.test.ts: Update targetBundles directory test since the endpoint no longer creates directories for targetBundles (only bundle_<hash> fields determine directory placement). Also update buildVM spy to use buildExecutionContext which is the exported function. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
/upload-assetsendpoint blindly copying all uploaded files into every target bundle directory, which duplicated bundle JS files when RSC support is enabledextractBundlesAndAssets()helper used by both the render and/upload-assetsendpoints, DRYing up form body parsing/upload-assetsto reusehandleNewBundlesProvided()from the render path, ensuring each bundle goes only to its own directorytargetBundlesform field from/upload-assets— target directories are derived frombundle_<hash>form keysTest plan
worker.test.tstests pass (including updated/upload-assetstests)uploadRaceCondition.test.tstests pass (concurrent isolation, serialization, cross-endpoint locking)handleRenderRequest.test.tstests pass (no regressions in render path)Closes #2766
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
Tests