Fix generator CI SSR regression on main#3110
Conversation
WalkthroughThis pull request refactors a default export into a named constant and enhances Rake tasks for example app generation by adding helpers to pin Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review SummaryOverall AssessmentThe changes are well-motivated and the approach is sound. The
|
Greptile SummaryThis PR fixes two related CI regressions on Confidence Score: 5/5Safe to merge — both changes are well-scoped, the TypeScript fix is backward-compatible, and the rake changes are CI/tooling only with no production-code impact. All findings are P2 style suggestions. No logic bugs, no security concerns, no API breakage — the default-export refactor is a drop-in rename and all consumers use the default import unchanged. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[install_example_node_dependencies] --> B[pin_shakapacker_npm_version]
A --> C[pin_react_on_rails_to_local_package]
C --> D{pinned_react_version?}
D -- yes --> E[npm install --legacy-peer-deps --install-links]
E --> F[install_local_react_on_rails_package legacy_peer_deps: true]
F --> G[bundle exec rake shakapacker:binstubs]
D -- no --> H[npm install --install-links]
H --> I[install_local_react_on_rails_package legacy_peer_deps: false]
Reviews (1): Last reviewed commit: "Stabilize generator CI against npm and w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails/rakelib/shakapacker_examples.rake (1)
157-175: Consider whether the double install ofreact-on-railsis necessary.The flow pins
react-on-railsinpackage.json(line 159), then runsnpm install(lines 164 or 172), and then explicitly installsreact-on-railsagain (lines 165 or 173). Since thefile:reference is already inpackage.jsonbefore the main install, the second install may be redundant.If this is intentional (e.g., to handle npm resolution edge cases with
--install-links), a brief comment explaining why would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/rakelib/shakapacker_examples.rake` around lines 157 - 175, The code in install_example_node_dependencies currently pins react-on-rails via pin_react_on_rails_to_local_package(example_type.dir) and then runs npm install, but immediately calls install_local_react_on_rails_package(...) which performs a second explicit install; either remove the redundant explicit install or make it conditional. Fix by ensuring pin_react_on_rails_to_local_package writes the file: dependency into package.json before the npm install so the main npm install suffices, and remove the subsequent install_local_react_on_rails_package(...) calls in both branches (or retain them only behind a clearly documented conditional like legacy_peer_deps edge-case), and add a short comment in install_example_node_dependencies explaining why a second install remains necessary if you choose to keep it; reference functions: install_example_node_dependencies, pin_react_on_rails_to_local_package, install_local_react_on_rails_package, and the npm install invocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/rakelib/shakapacker_examples.rake`:
- Around line 157-175: The code in install_example_node_dependencies currently
pins react-on-rails via pin_react_on_rails_to_local_package(example_type.dir)
and then runs npm install, but immediately calls
install_local_react_on_rails_package(...) which performs a second explicit
install; either remove the redundant explicit install or make it conditional.
Fix by ensuring pin_react_on_rails_to_local_package writes the file: dependency
into package.json before the npm install so the main npm install suffices, and
remove the subsequent install_local_react_on_rails_package(...) calls in both
branches (or retain them only behind a clearly documented conditional like
legacy_peer_deps edge-case), and add a short comment in
install_example_node_dependencies explaining why a second install remains
necessary if you choose to keep it; reference functions:
install_example_node_dependencies, pin_react_on_rails_to_local_package,
install_local_react_on_rails_package, and the npm install invocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b119759-cff9-4f17-bf7e-5152acc7c56d
📒 Files selected for processing (2)
packages/react-on-rails/src/scriptSanitizedVal.tsreact_on_rails/rakelib/shakapacker_examples.rake
The explicit re-install of react-on-rails after the initial npm install is required because npm install --install-links does not reliably copy a file: dependency that was just pinned in the same session. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Review comments addressedAddressed (1):
Declined with rationale (3):
All 4 review threads resolved. |
| raise | ||
| end | ||
|
|
||
| deps = package_json["dependencies"] |
There was a problem hiding this comment.
Only dependencies is checked here, while pin_shakapacker_npm_version checks both dependencies and devDependencies. In practice, example apps install react-on-rails as a runtime dependency so this is likely fine, but for robustness and consistency it may be worth adding a devDependencies fallback — or at least a comment explaining why only dependencies is intentional.
| deps = package_json["dependencies"] | |
| %w[dependencies devDependencies].each do |section| | |
| deps = package_json[section] | |
| next unless deps&.key?("react-on-rails") | |
| local_package_ref = local_react_on_rails_package_ref(dir) | |
| next if deps["react-on-rails"] == local_package_ref | |
| puts " Pinning react-on-rails npm dependency from #{deps['react-on-rails']} to #{local_package_ref}" | |
| deps["react-on-rails"] = local_package_ref | |
| end | |
| File.write(package_json_path, "#{JSON.pretty_generate(package_json)}\n") |
| puts " React: #{react_version}" | ||
| end | ||
|
|
||
| def install_local_react_on_rails_package(dir, legacy_peer_deps: false) |
There was a problem hiding this comment.
The trailing space embedded in "--legacy-peer-deps " is intentional — it separates the flag from --install-links when interpolated — but this is easy to misread as a typo. Consider using Array#join(" ") or a conditional separator to make the intent explicit:
| def install_local_react_on_rails_package(dir, legacy_peer_deps: false) | |
| def install_local_react_on_rails_package(dir, legacy_peer_deps: false) | |
| flags = ["--install-links"] | |
| flags.unshift("--legacy-peer-deps") if legacy_peer_deps | |
| sh_in_dir(dir, | |
| "npm install react-on-rails@#{local_react_on_rails_package_ref(dir)} #{flags.join(' ')}") | |
| end |
ReviewOverall this is a well-targeted fix. The webpack Observations
Double-install pattern — the comments explaining why an explicit re-install is needed after the initial
|
### Summary Fixes the generator CI break on main by avoiding webpack's `__WEBPACK_DEFAULT_EXPORT__` runtime issue and ensuring example-app CI uses the local monorepo `react-on-rails` package instead of stale published tarballs. Updated `scriptSanitizedVal` export shape and refactored `shakapacker_examples` dependency install flow to pin/install `react-on-rails` from `file:` refs for both latest and pinned matrices. ### Pull Request checklist - [x] ~Add/update test to cover these changes~ - [x] ~Update documentation~ - [x] ~Update CHANGELOG file~ ### Other Information Validated locally with `run_rspec:shakapacker_examples_latest` and `run_rspec:shakapacker_examples_pinned`, plus `rubocop`, `eslint`, and package build checks. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches generator/CI dependency installation logic and npm package pinning, which can break example app builds/SSR if the `file:` install behavior differs across npm versions. No production runtime or security-sensitive code paths are changed beyond a small export refactor. > > **Overview** > Fixes an SSR/webpack regression by refactoring `scriptSanitizedVal` to a named const default export (avoiding runtime issues with default export handling). > > Updates `shakapacker_examples` rake tasks to **pin `react-on-rails` to the local monorepo** via a `file:` dependency and centralizes node dependency installation, including explicit re-install of the local package and consistent `--install-links`/`--legacy-peer-deps` handling for pinned React matrices. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 8e32dc6. 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 * **Refactor** * Improved internal code organization for better maintainability. * **Chores** * Enhanced dependency management and installation process for local development environments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
### Summary Fixes the generator CI break on main by avoiding webpack's `__WEBPACK_DEFAULT_EXPORT__` runtime issue and ensuring example-app CI uses the local monorepo `react-on-rails` package instead of stale published tarballs. Updated `scriptSanitizedVal` export shape and refactored `shakapacker_examples` dependency install flow to pin/install `react-on-rails` from `file:` refs for both latest and pinned matrices. ### Pull Request checklist - [x] ~Add/update test to cover these changes~ - [x] ~Update documentation~ - [x] ~Update CHANGELOG file~ ### Other Information Validated locally with `run_rspec:shakapacker_examples_latest` and `run_rspec:shakapacker_examples_pinned`, plus `rubocop`, `eslint`, and package build checks. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches generator/CI dependency installation logic and npm package pinning, which can break example app builds/SSR if the `file:` install behavior differs across npm versions. No production runtime or security-sensitive code paths are changed beyond a small export refactor. > > **Overview** > Fixes an SSR/webpack regression by refactoring `scriptSanitizedVal` to a named const default export (avoiding runtime issues with default export handling). > > Updates `shakapacker_examples` rake tasks to **pin `react-on-rails` to the local monorepo** via a `file:` dependency and centralizes node dependency installation, including explicit re-install of the local package and consistent `--install-links`/`--legacy-peer-deps` handling for pinned React matrices. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 8e32dc6. 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 * **Refactor** * Improved internal code organization for better maintainability. * **Chores** * Enhanced dependency management and installation process for local development environments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
### Summary Fixes the generator CI break on main by avoiding webpack's `__WEBPACK_DEFAULT_EXPORT__` runtime issue and ensuring example-app CI uses the local monorepo `react-on-rails` package instead of stale published tarballs. Updated `scriptSanitizedVal` export shape and refactored `shakapacker_examples` dependency install flow to pin/install `react-on-rails` from `file:` refs for both latest and pinned matrices. ### Pull Request checklist - [x] ~Add/update test to cover these changes~ - [x] ~Update documentation~ - [x] ~Update CHANGELOG file~ ### Other Information Validated locally with `run_rspec:shakapacker_examples_latest` and `run_rspec:shakapacker_examples_pinned`, plus `rubocop`, `eslint`, and package build checks. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches generator/CI dependency installation logic and npm package pinning, which can break example app builds/SSR if the `file:` install behavior differs across npm versions. No production runtime or security-sensitive code paths are changed beyond a small export refactor. > > **Overview** > Fixes an SSR/webpack regression by refactoring `scriptSanitizedVal` to a named const default export (avoiding runtime issues with default export handling). > > Updates `shakapacker_examples` rake tasks to **pin `react-on-rails` to the local monorepo** via a `file:` dependency and centralizes node dependency installation, including explicit re-install of the local package and consistent `--install-links`/`--legacy-peer-deps` handling for pinned React matrices. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 8e32dc6. 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 * **Refactor** * Improved internal code organization for better maintainability. * **Chores** * Enhanced dependency management and installation process for local development environments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
…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
Fixes the generator CI break on main by avoiding webpack's
__WEBPACK_DEFAULT_EXPORT__runtime issue and ensuring example-app CI uses the local monoreporeact-on-railspackage instead of stale published tarballs. UpdatedscriptSanitizedValexport shape and refactoredshakapacker_examplesdependency install flow to pin/installreact-on-railsfromfile:refs for both latest and pinned matrices.Pull Request checklist
Add/update test to cover these changesUpdate documentationUpdate CHANGELOG fileOther Information
Validated locally with
run_rspec:shakapacker_examples_latestandrun_rspec:shakapacker_examples_pinned, plusrubocop,eslint, and package build checks.Note
Medium Risk
Touches generator/CI dependency installation logic and npm package pinning, which can break example app builds/SSR if the
file:install behavior differs across npm versions. No production runtime or security-sensitive code paths are changed beyond a small export refactor.Overview
Fixes an SSR/webpack regression by refactoring
scriptSanitizedValto a named const default export (avoiding runtime issues with default export handling).Updates
shakapacker_examplesrake tasks to pinreact-on-railsto the local monorepo via afile:dependency and centralizes node dependency installation, including explicit re-install of the local package and consistent--install-links/--legacy-peer-depshandling for pinned React matrices.Reviewed by Cursor Bugbot for commit 8e32dc6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Refactor
Chores