Migrate to react on rails auto-registration #649
Conversation
WalkthroughSwitch to auto-generated React on Rails packs with auto-registration; add stores-registration; change dev startup (bin/dev, Procfiles); bump shakapacker/sass; add CI generate-packs steps; move asset tag injection points; update many import paths and add a ReScript component; ignore generated bundles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as bin/dev
participant Manager as ReactOnRails::Dev::ServerManager
CLI->>Manager: start(:hmr) (default / hmr)
CLI->>Manager: start(:production_like, "Procfile.dev-prod-assets")
CLI->>Manager: start(:static, "Procfile.dev-static-assets")
CLI->>Manager: kill_processes()
CLI->>Manager: show_help()
sequenceDiagram
autonumber
actor Browser as User
participant ReScript as RescriptShow.jsx
participant API as Actions.Fetch
participant Cable as ActionCable::CommentsChannel
Browser->>ReScript: mount
ReScript->>API: fetchComments()
API-->>ReScript: comments[] / error
alt success
ReScript->>ReScript: dispatch(SetComments(comments))
else error
ReScript->>ReScript: dispatch(FetchError)
end
ReScript->>Cable: subscribe()
Cable-->>ReScript: receive(newData)
ReScript->>ReScript: dispatch(SetComments([newData]))
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Gemfile (1)
48-52: Remove stale net-pop overrideRuby is pinned to 3.3.7 and Gemfile.lock shows net-pop coming from the Git override; no explicit require 'net/pop' was found — remove the Git override to avoid masking upstream fixes and supply-chain risk.
File: Gemfile (lines 48-52)
Apply:
-# Needed until Ruby 3.3.4 is released https://github.com/ruby/ruby/pull/11006 -# Related issue: https://github.com/ruby/net-pop/issues/26 -# TODO: When Ruby 3.3.4 is released, upgrade Ruby and remove this line -gem "net-pop", github: "ruby/net-pop"
🧹 Nitpick comments (10)
config/shakapacker.yml (1)
24-24: Typo in comment: Procfile.dev-static.Minor nit; helps future readers.
Apply:
- # This is false since we're running `bin/shakapacker -w` in Procfile.dev-setic + # This is false since we're running `bin/shakapacker -w` in Procfile.dev-static-assetsProcfile.dev-static-assets (1)
5-5: Avoid environment-specific Redis assumption in comments.“Redis is already running system-wide” may not hold for all devs/CI. Consider rewording to instruct enabling the line if needed.
Procfile.dev-prod-assets (1)
5-6: Ensure packs are precompiled before starting rails in prod-assets mode.This Procfile assumes precompiled assets exist. Document the step (e.g., bin/shakapacker -p and any Rails asset precompile) or add a guard in your dev workflow.
client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx (1)
62-67: Guard unsubscribe to avoid runtime error if subscription creation fails.If subscribeConsumer fails or shape changes, scription may be undefined or lack unsubscribe.
Apply this diff:
- React.useEffect((function () { - var scription = subscribeToCommentsChannel(); - return (function () { - scription.unsubscribe(); - }); - }), []); + React.useEffect((function () { + var scription = subscribeToCommentsChannel(); + return (function () { + if (scription && typeof scription.unsubscribe === "function") { + scription.unsubscribe(); + } + }); + }), []);bin/dev (2)
21-28: Improve fallback error handling when react_on_rails/dev isn’t available.If require_relative fails, the script currently crashes with a stack trace.
Apply this diff:
begin require "bundler/setup" require "react_on_rails/dev" rescue LoadError # Fallback for when gem is not yet installed puts "Loading ReactOnRails development tools..." - require_relative "../../lib/react_on_rails/dev" + begin + require_relative "../../lib/react_on_rails/dev" + rescue LoadError => e + warn "Could not load ReactOnRails dev tools. Ensure the gem is installed or lib/react_on_rails/dev exists. #{e.message}" + exit 1 + end end
43-45: Send unknown-argument errors to stderr and exit non‑zero.Minor polish for CLI ergonomics.
Apply this diff:
- puts "Unknown argument: #{ARGV[0]}" - puts "Run 'bin/dev help' for usage information" + warn "Unknown argument: #{ARGV[0]}" + warn "Run 'bin/dev help' for usage information" exit 1client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx (1)
11-19: Remove unused redirect/error scaffolding or wire it up.error and redirectLocation are never set, so the early-return branch is dead code. Either remove it or implement proper setting logic.
- let error; - let redirectLocation; - const { location } = railsContext; + const { location } = railsContext; - - if (error || redirectLocation) { - return { error, redirectLocation }; - }app/views/layouts/stimulus_layout.html.erb (1)
10-11: Stimulus assets appended in head: OK.If any views rendered with this layout use Redux stores, also append stores-registration here or ensure those pages append it explicitly.
<%= append_stylesheet_pack_tag('stimulus-bundle') %> <%= append_javascript_pack_tag('stimulus-bundle') %> + <%# Uncomment if pages under this layout use ReactOnRails stores %> + <%#= append_javascript_pack_tag('stores-registration') %>config/initializers/react_on_rails.rb (2)
5-7: Consider gating server‑side console replay/logging in production.With broader auto‑registration and SSR enabled elsewhere in this initializer, replaying server logs can leak props/PII into app logs. Suggest restricting to dev/test:
config.replay_console = Rails.env.development? || Rails.env.test? config.logging_on_server = Rails.env.development? || Rails.env.test?
5-7: Optional: simplify server bundle setup via generated entrypoint.If you want to avoid maintaining a handwritten
server-bundle.jsthat imports the generated file, enable:config.make_generated_server_bundle_the_entrypoint = trueIf you do this, adjust
config.server_bundle_js_fileto match the generated entrypoint as needed. See config docs. (shakacode.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
.gitignore(1 hunks).ruby-version(1 hunks)Gemfile(1 hunks)Procfile.dev-prod-assets(1 hunks)Procfile.dev-static-assets(1 hunks)app/views/layouts/application.html.erb(2 hunks)app/views/layouts/stimulus_layout.html.erb(2 hunks)bin/dev(1 hunks)client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx(1 hunks)client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx(1 hunks)client/app/bundles/comments/startup/App/ror_components/App.jsx(1 hunks)client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx(1 hunks)client/app/packs/client-bundle.js(0 hunks)client/app/packs/server-bundle.js(1 hunks)client/app/packs/stimulus-bundle.js(1 hunks)client/app/packs/stores-registration.js(1 hunks)config/initializers/react_on_rails.rb(1 hunks)config/shakapacker.yml(1 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- client/app/packs/client-bundle.js
🧰 Additional context used
🧬 Code graph analysis (1)
client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx (2)
client/app/bundles/comments/reducers/commentsReducer.js (1)
action(18-18)client/app/bundles/comments/components/CommentBox/CommentList/CommentList.jsx (1)
CommentList(17-87)
🪛 GitHub Actions: JS CI
Gemfile
[error] 1-1: Command 'bundle install' failed: Bundler exited with code 18 due to Ruby version mismatch. System Ruby is 3.3.4 but Gemfile specifies 3.3.7.
🔇 Additional comments (22)
.ruby-version (1)
1-1: LGTM: version bump to 3.3.7.Ensure CI picks this up (see Gemfile comment for workflow check).
Use the workflow scan script in the Gemfile comment to confirm all jobs use 3.3.7.
client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx (1)
8-10: Approve — import paths updated correctly; railsContext usage unaffected.No logic changes required; NavigationBar, NavigationBarContainer, and constants/paths imports verified.
client/app/bundles/comments/startup/App/ror_components/App.jsx (1)
5-5: Import path fix looks correct with v16 auto‑registration.NonRouterCommentsContainer.jsx exists at client/app/bundles/comments/containers/NonRouterCommentsContainer.jsx; ReactOnRails.getStore('commentsStore') is present in App.jsx (line 12) and NavigationBarApp.jsx (line 24). Explicit .jsx import removes the need to verify webpack extension resolution.
.gitignore (1)
56-58: Over‑broad ignore — narrow '/generated/' to ReactOnRails pack output.Current pattern in .gitignore (File: .gitignore, ~lines 56–58) can hide unrelated generated sources (GraphQL codegen, etc.). Scope it to the React on Rails pack output paths.
-**/generated** +# React on Rails generated packs only +/client/app/**/generated/** +/client/**/packs/**/generated/**Verification scripts returned no output; confirm whether any
generateddirectories exist in the repo before applying this change.config/shakapacker.yml (1)
10-10: nested_entries enabled — confirm manifest keys + server-bundle alignment
- config/shakapacker.yml: nested_entries: true.
- Found packs: client/app/packs/{stimulus-bundle.js, stores-registration.js, server-bundle.js}.
- Layouts reference packs via append_* helpers: app/views/layouts/application.html.erb, app/views/layouts/stimulus_layout.html.erb (use 'stimulus-bundle' and 'stores-registration').
- server-bundle imports ./../generated/server-bundle-generated.js (client/app/packs/server-bundle.js).
- No manifest.json found in the repo — cannot verify whether compiled manifest keys are nested (and therefore differ from the bare names used in the helpers).
Action: verify the compiled manifest.json on the build output (e.g., public/packs/manifest*.json) contains keys matching the helper arguments OR update the view helpers to use the nested manifest keys; also confirm generated/server-bundle-generated.js is emitted and referenced correctly in the manifest.
package.json (1)
81-81: react-on-rails 16 / shakapacker 8.4 — versions & auto-registration verified; confirm Webpack 5 + Node 22yarn.lock shows [email protected] and [email protected]; config/initializers/react_on_rails.rb has config.auto_load_bundle = true and build_production_command uses shakapacker.
- Confirm webpack 5 is present in package.json / yarn.lock and Node 22 is specified (package.json "engines", .nvmrc, or .node-version).
client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx (1)
13-15: Path updates OK — confirm client webpack CSS Modules for .module.scssImports and .module.scss usage look fine. serverWebpackConfig.js sets cssLoader.options.modules = { exportOnlyLocals: true }, but I did not find a client-side rule explicitly targeting /.module.(scss|sass|css)/; ensure the client webpack config (config/webpack/clientWebpackConfig.js or config/webpack/environment.js) enables css-loader modules (e.g., modules: { auto: /.module.\w+$/ } or equivalent).
Procfile.dev-prod-assets (1)
1-4: LGTM: clear header and intent.client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx (2)
12-25: Reducer treats any object action as success. Verify only SetComments objects dispatch here.If other object-shaped actions reach this reducer, they’ll be interpreted as CommentsFetched.
51-56: Confirm ActionCable payload shape.received wraps data in an array for SetComments. Ensure downstream expects an array-of-comment objects.
bin/dev (2)
31-41: Command mapping reads clean.Nice, intuitive aliases (prod/static/hmr/help) and clear Procfile routing.
33-35: Procfiles present — no action required.Procfile.dev, Procfile.dev-static-assets, and Procfile.dev-prod-assets are present in the repo.
client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx (2)
1-1: LGTM: doc comment sync with server side.
6-6: Verified: routes path and default export are correct.client/app/bundles/comments/routes/routes.jsx exists and exports a default JSX routes tree; import '../../../routes/routes.jsx' is valid.
import routes from '../../../routes/routes.jsx';Procfile.dev-static-assets (1)
3-4: LGTM — res:dev script verifiedpackage.json defines
res:devasyarn res:clean && rescript build -w.client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx (1)
6-6: Updated routes import path looks right.../../../routes/routes.jsx resolves correctly from ror_components to comments/routes.
client/app/packs/server-bundle.js (1)
1-5: Good separation: server bundle imports stores for SSR, generated code for auto‑registration.Ensure server-bundle.js is not included on the client to avoid duplicate store registrations.
app/views/layouts/stimulus_layout.html.erb (1)
22-23: Confirm append_ pattern with options under Shakapacker 8.4.*Using stylesheet_pack_tag/javascript_pack_tag with only options relies on previously appended packs. Verify tags render as expected in all envs.
app/views/layouts/application.html.erb (2)
23-25: Verify appended packs render and order is correct.Ensure stores-registration loads before components that access stores, and that these helpers output tags as intended across environments.
8-10: Head includes Stimulus and stores — OK.app/views/layouts/application.html.erb and app/views/layouts/stimulus_layout.html.erb include append_javascript_pack_tag('stores-registration') (and 'stimulus-bundle'); package.json contains @hotwired/stimulus — auto‑registration pack appears loaded, no action required.
client/app/packs/stores-registration.js (1)
6-9: Cannot verify store exports — confirm routerCommentsStore and commentsStore are default-exported store-generator functions and call-sites use exact namesrg output only matched reducers under client/app/bundles/comments/reducers and reported files were skipped; confirm the store file paths/exports and that callers use getStore('routerCommentsStore') and getStore('commentsStore').
client/app/packs/stimulus-bundle.js (1)
18-19: Confirmed: client auto-registration bundle is included at runtime.
app/views/layouts/application.html.erb and app/views/layouts/stimulus_layout.html.erb include append_javascript_pack_tag('stimulus-bundle'), so auto‑registered components will be available to react_component helpers.
| # Auto-registration configuration for v16 | ||
| config.components_subdirectory = "ror_components" | ||
| config.auto_load_bundle = true |
There was a problem hiding this comment.
🧩 Analysis chain
v16 auto‑registration config looks correct; verify ancillary setup.
components_subdirectory and auto_load_bundle align with the v13.1+ file‑system bundle generation and v16 auto‑registration flow. Default for auto_load_bundle is false; opting into true here is expected. (shakacode.com)
Run to sanity‑check the migration bits:
References on components_subdirectory and auto_load_bundle behaviors. (shakacode.com)
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Shakapacker nested entries must be on for generation
rg -nP '^\s*nested_entries:\s*true\b' config/shakapacker.yml || {
echo "Missing nested_entries: true in config/shakapacker.yml"; exit 1; }
# 2) Generated artifacts should be gitignored
rg -nP '\*\*/generated/\*\*' .gitignore || {
echo "Add **/generated/** to .gitignore"; exit 1; }
# 3) Layouts should not pass explicit packs anymore (auto-load handles it)
# Flag any javascript_pack_tag/stylesheet_pack_tag calls with args
rg -nP 'javascript_pack_tag\s*\((?!\s*\))' app 2>/dev/null || true
rg -nP 'stylesheet_pack_tag\s*\((?!\s*\))' app 2>/dev/null || true
# Flag legacy append_* helpers that should be removed
rg -n 'append_(javascript|stylesheet)_pack_tag' app 2>/dev/null || true
# 4) Server bundle should pull in the generated server bundle (if not using the "make_generated..." option)
fd -t f server-bundle.js client app/javascript 2>/dev/null | xargs -I{} rg -n 'server-bundle-generated' {} || true
# 5) Ensure components live under the configured ror_components dirs
fd -t d ror_components client app/javascript 2>/dev/null || {
echo "No ror_components dirs found; verify paths"; exit 1; }Length of output: 983
v16 auto‑registration config OK — remove legacy append_ pack helpers from layouts.*
- Confirmed: config/shakapacker.yml has
nested_entries: true;.gitignorecontains**/generated/**; server bundle imports./../generated/server-bundle-generated.js;ror_componentsdirs present underclient/app/bundles/.... - Required fix: remove legacy helpers so auto_load_bundle handles packs:
- app/views/layouts/application.html.erb (lines 8–10): remove
append_stylesheet_pack_tag('stimulus-bundle'),append_javascript_pack_tag('stimulus-bundle'),append_javascript_pack_tag('stores-registration') - app/views/layouts/stimulus_layout.html.erb (lines 10–11): remove
append_stylesheet_pack_tag('stimulus-bundle'),append_javascript_pack_tag('stimulus-bundle')
- app/views/layouts/application.html.erb (lines 8–10): remove
🤖 Prompt for AI Agents
In config/initializers/react_on_rails.rb around lines 5–7, v16 auto-registration
is enabled so remove the legacy append_* pack helper calls from layouts: in
app/views/layouts/application.html.erb (lines 8–10) delete the three calls
append_stylesheet_pack_tag('stimulus-bundle'),
append_javascript_pack_tag('stimulus-bundle'), and
append_javascript_pack_tag('stores-registration'); in
app/views/layouts/stimulus_layout.html.erb (lines 10–11) delete
append_stylesheet_pack_tag('stimulus-bundle') and
append_javascript_pack_tag('stimulus-bundle') so auto_load_bundle handles pack
inclusion.
| # Note, if using React on Rails localization you will need to run | ||
| # `bundle exec rake react_on_rails:locale` before you run bin/shakapacker | ||
| webpack: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w' | ||
| js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w' |
There was a problem hiding this comment.
❓ Verification inconclusive
Don’t swallow locale task failures; prefer shakapacker:clean over rm -rf.
The current chaining makes bin/shakapacker -w run even if react_on_rails:locale fails. Also avoid rm -rf in scripts; use the rake task.
Apply this diff:
-js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
+js: sh -c 'bundle exec rake react_on_rails:locale && (bundle exec rake shakapacker:clean || true) && bin/shakapacker -w'If you must keep rm, limit the “|| true” to that step only:
-js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
+js: sh -c 'bundle exec rake react_on_rails:locale && (rm -rf public/packs/* || true) && bin/shakapacker -w'Don’t swallow locale task failures; make the watcher conditional on the locale task
Procfile.dev-static-assets: line 12 — the current || true placement makes bin/shakapacker -w run unconditionally; apply one of these diffs:
-js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
+js: sh -c 'bundle exec rake react_on_rails:locale && (bundle exec rake shakapacker:clean || true) && bin/shakapacker -w'Or, if keeping rm:
-js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
+js: sh -c 'bundle exec rake react_on_rails:locale && (rm -rf public/packs/* || true) && bin/shakapacker -w'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w' | |
| js: sh -c 'bundle exec rake react_on_rails:locale && (bundle exec rake shakapacker:clean || true) && bin/shakapacker -w' |
| js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w' | |
| js: sh -c 'bundle exec rake react_on_rails:locale && (rm -rf public/packs/* || true) && bin/shakapacker -w' |
🤖 Prompt for AI Agents
In Procfile.dev-static-assets around line 12, the current command uses `|| true`
in a way that swallows failures from the locale rake task and causes
`bin/shakapacker -w` to run unconditionally; change the command so the watcher
only runs if the locale task succeeds — either remove `|| true` and chain with
`&&` so `bundle exec rake react_on_rails:locale && rm -rf public/packs/* &&
bin/shakapacker -w`, or if you want to ignore errors from `rm` but not from the
rake task, group/sequence so the rake task must succeed and only the `rm` is
tolerated, e.g. run the rake task, then `rm -rf public/packs/* || true`, then
`&& bin/shakapacker -w`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/js_test.yml (1)
47-49: Good addition; ensure task availability and consider upgrading GH Actions versions.
- Step placement is correct and aligns with v16 auto‑generated packs being .gitignored.
- Add a lightweight preflight to surface a clearer failure if the task isn’t available on older branches, and consider bumping Actions to v4.
- - name: Checkout code - uses: actions/checkout@v3 + - name: Checkout code + uses: actions/checkout@v4 - - uses: ruby/setup-ruby@v1 + - uses: ruby/setup-ruby@v1 with: ruby-version: ${{ matrix.ruby }} bundler-cache: true - - name: Use Node.js ${{ matrix.node }} - uses: actions/setup-node@v3 + - name: Use Node.js ${{ matrix.node }} + uses: actions/setup-node@v4 with: node-version: ${{ matrix.node }} cache: yarn + - name: Verify React on Rails generate_packs task exists + run: bundle exec rails -T | grep -q 'react_on_rails:generate_packs' + - name: Generate React on Rails packs run: bundle exec rails react_on_rails:generate_packs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/js_test.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (22.x, 3.3.4)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/lint_test.yml (1)
46-48: Packs generation step looks right; add debug flag and confirm DB independence.
- Append
--tracefor easier CI debugging.- This job has no DB service; confirm the rake task doesn’t touch the DB under
RAILS_ENV=test. If it does, override toRAILS_ENV=developmentfor this step only.- - name: Generate React on Rails packs - run: bundle exec rails react_on_rails:generate_packs + - name: Generate React on Rails packs + run: bundle exec rails react_on_rails:generate_packs --trace +# If the task requires no DB, keep test. If it does, explicitly switch env just for this step: +# env: +# RAILS_ENV: development.github/workflows/rspec_test.yml (1)
76-78: Good placement; consider ordering vs ReScript and add--trace.
- If generated packs import ReScript outputs, run this after
yarn res:build; otherwise current order is fine.- Add
--tracefor better failure diagnostics.- - name: Generate React on Rails packs - run: bundle exec rails react_on_rails:generate_packs + - name: Generate React on Rails packs + run: bundle exec rails react_on_rails:generate_packs --traceOptional reorder:
- - name: Generate React on Rails packs - run: bundle exec rails react_on_rails:generate_packs - - name: Build Rescript components run: yarn res:build + - name: Generate React on Rails packs + run: bundle exec rails react_on_rails:generate_packs --trace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/lint_test.yml(1 hunks).github/workflows/rspec_test.yml(1 hunks)client/app/bundles/comments/startup/serverRegistration.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (3)
client/app/bundles/comments/startup/serverRegistration.jsx (3)
8-9: Remove duplicate local store registration — stores are registered in client/app/packs/stores-registration.js.client/app/packs/stores-registration.js calls ReactOnRails.registerStore (includes routerCommentsStore); remove the local imports and ReactOnRails.registerStore call in client/app/bundles/comments/startup/serverRegistration.jsx (lines 8-9 and 20-23).
Apply:
-import routerCommentsStore from '../store/routerCommentsStore'; -import commentsStore from '../store/commentsStore'; +// Stores registered via packs/stores-registration.js in v16 migration. -ReactOnRails.registerStore({ - routerCommentsStore, - commentsStore, -}); +// No local store registration.
4-7: Drop manual component registration; verify ReactOnRails v16 auto-registration is enabledSanity check returned no matches for other manual registrations — verification inconclusive. If ReactOnRails v16 with auto_load_bundle is enabled and ror_components are auto-registered, remove the manual imports and the ReactOnRails.register block in client/app/bundles/comments/startup/serverRegistration.jsx (lines 4–18).
Apply:
import ReactOnRails from 'react-on-rails'; - -import App from './App/ror_components/App'; -import RouterApp from './RouterApp/ror_components/RouterApp.server'; -import SimpleCommentScreen from '../components/SimpleCommentScreen/ror_components/SimpleCommentScreen'; -import NavigationBarApp from './NavigationBarApp/ror_components/NavigationBarApp'; +// Components auto-registered by ReactOnRails v16 from ror_components. import routerCommentsStore from '../store/routerCommentsStore'; import commentsStore from ../store/commentsStore'; -import Footer from '../components/Footer/ror_components/Footer'; - -ReactOnRails.register({ - App, - RouterApp, - NavigationBarApp, - SimpleCommentScreen, - Footer, -}); +// No manual component registration needed.
5-5: No client bundles reference RouterApp.server — server-only import confirmedclient/app/bundles/comments/startup/serverRegistration.jsx imports RouterApp.server and that file is used only as the server webpack entry (config/webpack/server.js:23). No other references to serverRegistration.jsx or RouterApp.server were found; client counterpart is at client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx.
justin808
changed the title
|
rebase on master to get the rubocop fix |
ihabadham
force-pushed
the
migrate-to-react-on-rails-v16
branch
from
c775eba to
4383625
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
bin/dev (2)
30-41: Dispatcher looks good; consider a ‘dev’ alias for ergonomics.Optional: accept “dev” in addition to “hmr” and nil.
Apply this diff:
-when "hmr", nil +when "hmr", "dev", nil ReactOnRails::Dev::ServerManager.start(:development, "Procfile.dev")
43-46: Send errors to STDERR and show built-in help.Minor UX: use warn and call show_help.
Apply this diff:
- puts "Unknown argument: #{ARGV[0]}" - puts "Run 'bin/dev help' for usage information" - exit 1 + warn "Unknown argument: #{ARGV[0] || '(none)'}" + ReactOnRails::Dev::ServerManager.show_help + exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.github/workflows/js_test.yml(1 hunks).github/workflows/lint_test.yml(1 hunks).github/workflows/rspec_test.yml(1 hunks).gitignore(1 hunks)Gemfile(1 hunks)Procfile.dev-prod-assets(1 hunks)Procfile.dev-static-assets(1 hunks)app/views/layouts/application.html.erb(2 hunks)app/views/layouts/stimulus_layout.html.erb(2 hunks)bin/dev(1 hunks)client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx(1 hunks)client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx(1 hunks)client/app/bundles/comments/startup/App/ror_components/App.jsx(1 hunks)client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx(1 hunks)client/app/bundles/comments/startup/serverRegistration.jsx(1 hunks)client/app/packs/client-bundle.js(0 hunks)client/app/packs/server-bundle.js(1 hunks)client/app/packs/stimulus-bundle.js(1 hunks)client/app/packs/stores-registration.js(1 hunks)config/initializers/react_on_rails.rb(1 hunks)config/shakapacker.yml(1 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- client/app/packs/client-bundle.js
🚧 Files skipped from review as they are similar to previous changes (20)
- client/app/packs/server-bundle.js
- config/initializers/react_on_rails.rb
- client/app/bundles/comments/startup/App/ror_components/App.jsx
- Gemfile
- client/app/packs/stimulus-bundle.js
- client/app/bundles/comments/startup/serverRegistration.jsx
- client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx
- client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx
- client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx
- app/views/layouts/stimulus_layout.html.erb
- package.json
- client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx
- Procfile.dev-static-assets
- config/shakapacker.yml
- Procfile.dev-prod-assets
- .github/workflows/rspec_test.yml
- .gitignore
- .github/workflows/lint_test.yml
- .github/workflows/js_test.yml
- client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx
🧰 Additional context used
🪛 ESLint
client/app/packs/stores-registration.js
[error] 9-9: Insert ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (22.x, 3.3.4)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (4)
app/views/layouts/application.html.erb (2)
8-10: LGTM! Asset loading strategy aligned with auto-registration.The migration from
client-bundletostimulus-bundleand the addition ofstores-registrationpack correctly supports the new auto-registration approach. The use ofappend_*_pack_tagensures proper loading order.
23-24: Verify the generic pack tag implementation.stylesheet_pack_tag and javascript_pack_tag are used without pack names and likely won't load assets. Repository contains only app/assets/config/manifest.js (Sprockets) and no Webpacker/Shakapacker packs/manifest — confirm which asset system is intended and either:
- If using Webpacker/Shakapacker: specify pack names (e.g. stylesheet_pack_tag 'application'; javascript_pack_tag 'application') and ensure the packs and manifest (e.g. public/packs/manifest.json) exist.
- If using Sprockets: replace with stylesheet_link_tag 'application' and javascript_include_tag 'application'.
File: app/views/layouts/application.html.erb (lines 23–24)client/app/packs/stores-registration.js (1)
1-9: Approve — formatting fixed and store imports verifiedBoth ../bundles/comments/store/routerCommentsStore and ../bundles/comments/store/commentsStore exist and export default store factories, so ReactOnRails.registerStore({ routerCommentsStore, commentsStore }) is correct.
bin/dev (1)
4-20: Header docs are clear and accurate.Matches the dispatcher behavior below.
| begin | ||
| require "bundler/setup" | ||
| require "react_on_rails/dev" | ||
| rescue LoadError | ||
| # Fallback for when gem is not yet installed | ||
| puts "Loading ReactOnRails development tools..." | ||
| require_relative "../../lib/react_on_rails/dev" | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Fix fallback require path; '../../lib/…' escapes the repo and will fail. Also tighten LoadError handling.
From bin/dev, the project lib is ../lib, not ../../lib. Recommend nested rescue to show a clearer error if both gem and local copy are unavailable.
Apply this diff:
-begin
- require "bundler/setup"
- require "react_on_rails/dev"
-rescue LoadError
- # Fallback for when gem is not yet installed
- puts "Loading ReactOnRails development tools..."
- require_relative "../../lib/react_on_rails/dev"
-end
+begin
+ require "bundler/setup"
+ require "react_on_rails/dev"
+rescue LoadError => e
+ # Fallback for when the gem is not yet installed
+ warn "Loading ReactOnRails development tools... (#{e.message})" if ENV["DEBUG"]
+ begin
+ require_relative "../lib/react_on_rails/dev"
+ rescue LoadError
+ abort "ReactOnRails dev tools not found. Run `bundle install` or add lib/react_on_rails/dev."
+ end
+endRun to verify whether a local fallback exists (expected: it usually won’t in this repo; ensuring the gem is installed is the path forward):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for local ReactOnRails dev tools under lib/react_on_rails..."
if [ -f lib/react_on_rails/dev.rb ] || [ -d lib/react_on_rails/dev ]; then
echo "Found local fallback."
else
echo "No local fallback found. Ensure react_on_rails gem is installed so require 'react_on_rails/dev' succeeds."
fiLength of output: 175
Fix fallback require path; '../../lib/…' escapes the repo and will fail. Also tighten LoadError handling.
bin/dev (lines 21-28) — switch the fallback to ../lib and nest a second rescue that aborts if neither the gem nor a local copy is available. Verification: lib/react_on_rails/dev not found in repo (no local fallback); ensure the gem is installed or add lib/react_on_rails/dev.
-begin
- require "bundler/setup"
- require "react_on_rails/dev"
-rescue LoadError
- # Fallback for when gem is not yet installed
- puts "Loading ReactOnRails development tools..."
- require_relative "../../lib/react_on_rails/dev"
-end
+begin
+ require "bundler/setup"
+ require "react_on_rails/dev"
+rescue LoadError => e
+ # Fallback for when the gem is not yet installed
+ warn "Loading ReactOnRails development tools... (#{e.message})" if ENV["DEBUG"]
+ begin
+ require_relative "../lib/react_on_rails/dev"
+ rescue LoadError
+ abort "ReactOnRails dev tools not found. Run `bundle install` or add lib/react_on_rails/dev."
+ end
+end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| begin | |
| require "bundler/setup" | |
| require "react_on_rails/dev" | |
| rescue LoadError | |
| # Fallback for when gem is not yet installed | |
| puts "Loading ReactOnRails development tools..." | |
| require_relative "../../lib/react_on_rails/dev" | |
| end | |
| begin | |
| require "bundler/setup" | |
| require "react_on_rails/dev" | |
| rescue LoadError => e | |
| # Fallback for when the gem is not yet installed | |
| warn "Loading ReactOnRails development tools... (#{e.message})" if ENV["DEBUG"] | |
| begin | |
| require_relative "../lib/react_on_rails/dev" | |
| rescue LoadError | |
| abort "ReactOnRails dev tools not found. Run `bundle install` or add lib/react_on_rails/dev." | |
| end | |
| end |
🤖 Prompt for AI Agents
In bin/dev around lines 21 to 28 the fallback require uses
"../../lib/react_on_rails/dev" which escapes the repo and will fail; change it
to "../lib/react_on_rails/dev" and tighten LoadError handling by rescuing
LoadError for the initial gem require, then attempting the local require inside
its own begin/rescue and aborting with a clear error message if that also fails
(i.e., print an explanatory message and exit) so the script fails loudly when
neither the gem nor a local copy is available.
- Update react_on_rails gem from 14.1.0.rc.0 to 16.0.0 - Update shakapacker gem from 8.0.0 to 8.4.0 - Update react-on-rails npm package from 14.0.3 to 16.0.0 - Update shakapacker npm package from 8.0.0 to 8.4.0 - Update Ruby version from 3.3.4 to 3.3.7 - Add migration plan document
- Configure auto-registration with components_subdirectory and auto_load_bundle - Add nested_entries to shakapacker.yml for v16 compatibility - Restructure components into ComponentName/ror_components/ directories - Move App, RouterApp, NavigationBarApp, SimpleCommentScreen, Footer components - Handle RescriptShow component with compiled JS copy - Create stores-registration.js for manual store registration - Update server-bundle.js to import stores and generated bundle - Remove client-bundle.js (manual registration no longer needed) - Update .gitignore for generated packs - Successfully generate auto-registration packs
- Enable auto_load_bundle for automatic component discovery and loading - Update layouts to use append pack tag pattern for v16 compatibility - Remove manual component registration from stimulus-bundle.js - Maintain hybrid approach supporting both auto-registered React components and Stimulus controllers - Auto-registration now working: pages load only their specific component bundles 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Update bin/dev to use new ReactOnRails::Dev::ServerManager from PR #1790 - Add support for multiple development modes: * bin/dev (HMR development) - default mode * bin/dev static - static assets with auto-recompilation * bin/dev prod - production-optimized assets * bin/dev kill - clean process termination * bin/dev help - comprehensive help system - Rename Procfile.dev-static to Procfile.dev-static-assets for v16 convention - Add Procfile.dev-prod-assets for production-like asset development - Preserve project-specific customizations (ReScript, locale tasks) - Enhanced developer experience with better UI, error handling, and process management 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add bundle exec rails react_on_rails:generate_packs step to JS CI - Resolves missing server-bundle-generated.js error in CI builds - Generated files are in .gitignore but needed for webpack builds 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
… v16 - Update imports to use new ror_components directory structure - Fix paths for App, RouterApp, SimpleCommentScreen, NavigationBarApp, Footer - Use RouterApp.server for server-side registration - Resolves ESLint import/no-unresolved errors in CI 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add bundle exec rails react_on_rails:generate_packs step to Rspec CI - Resolves missing server-bundle-generated.js error in Rspec builds - Required for webpack to find generated component registration files 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add bundle exec rails react_on_rails:generate_packs step to Lint CI - Resolves ESLint import/no-unresolved error for server-bundle-generated.js - Required for linting to find generated component registration files 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
ihabadham
force-pushed
the
migrate-to-react-on-rails-v16
branch
from
52af588 to
631a699
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
bin/dev (1)
12-12: Update comment to match actual command pattern.Line 12 should reflect the actual command pattern shown in the implementation.
Apply this diff:
-- bin/dev static: Uses Procfile.dev-static-assets +# - bin/dev static: Uses Procfile.dev-static-assetsclient/app/packs/stores-registration.js (1)
2-3: Optional: consider using a webpack alias to avoid deep relative paths.If Shakapacker config supports it, alias
client/app(e.g.,@app) to write@app/bundles/comments/store/...and reduce fragile../hops.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.github/workflows/js_test.yml(1 hunks).github/workflows/lint_test.yml(1 hunks).github/workflows/rspec_test.yml(1 hunks).gitignore(1 hunks)Gemfile(1 hunks)Procfile.dev-prod-assets(1 hunks)Procfile.dev-static-assets(1 hunks)app/views/layouts/application.html.erb(2 hunks)app/views/layouts/stimulus_layout.html.erb(2 hunks)bin/dev(2 hunks)client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx(1 hunks)client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx(1 hunks)client/app/bundles/comments/startup/App/ror_components/App.jsx(1 hunks)client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx(1 hunks)client/app/bundles/comments/startup/serverRegistration.jsx(1 hunks)client/app/packs/client-bundle.js(0 hunks)client/app/packs/server-bundle.js(1 hunks)client/app/packs/stimulus-bundle.js(1 hunks)client/app/packs/stores-registration.js(1 hunks)config/initializers/react_on_rails.rb(1 hunks)config/shakapacker.yml(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- client/app/packs/client-bundle.js
🚧 Files skipped from review as they are similar to previous changes (21)
- client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx
- client/app/packs/stimulus-bundle.js
- client/app/bundles/comments/startup/serverRegistration.jsx
- Procfile.dev-static-assets
- config/initializers/react_on_rails.rb
- app/views/layouts/application.html.erb
- client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx
- .github/workflows/rspec_test.yml
- config/shakapacker.yml
- client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx
- client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx
- .github/workflows/js_test.yml
- .github/workflows/lint_test.yml
- Gemfile
- client/app/bundles/comments/startup/App/ror_components/App.jsx
- Procfile.dev-prod-assets
- app/views/layouts/stimulus_layout.html.erb
- .gitignore
- package.json
- client/app/packs/server-bundle.js
- client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx
🧰 Additional context used
🪛 ESLint
client/app/packs/stores-registration.js
[error] 9-9: Insert ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (4)
bin/dev (2)
21-28: Apply the previous fix for incorrect fallback path and poor error handling.This is the same issue flagged in the previous review. The fallback require path
../../lib/react_on_rails/devescapes the repository and will fail. The error handling also needs improvement.Apply this diff:
-begin - require "bundler/setup" - require "react_on_rails/dev" -rescue LoadError - # Fallback for when gem is not yet installed - puts "Loading ReactOnRails development tools..." - require_relative "../../lib/react_on_rails/dev" -end +begin + require "bundler/setup" + require "react_on_rails/dev" +rescue LoadError => e + # Fallback for when the gem is not yet installed + warn "Loading ReactOnRails development tools... (#{e.message})" if ENV["DEBUG"] + begin + require_relative "../lib/react_on_rails/dev" + rescue LoadError + abort "ReactOnRails dev tools not found. Run `bundle install` or add lib/react_on_rails/dev." + end +end
31-46: LGTM! Clean command dispatch pattern with proper error handling.The implementation correctly maps different command arguments to appropriate
ReactOnRails::Dev::ServerManagermethods and provides clear error messages for unknown arguments. The case-based dispatch is well-structured and makes the script's behavior transparent.client/app/packs/stores-registration.js (2)
1-3: No action needed — imports are store generator functions.Both routerCommentsStore.js and commentsStore.js default-export arrow functions (
export default (props, railsContext) => { ... }), so they satisfy ReactOnRails' store-generator requirement.
6-9: Correct ReactOnRails store registration — names match Rails usage. app/controllers/pages_controller.rb registers redux_store("routerCommentsStore", ...) and redux_store("commentsStore") (lines ~27 and ~33); layout includes redux_store_hydration_data.
Add trailing newlines to stores-registration.js and server-bundle.js to comply with code style standards. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Migrate to react_on_rails v16 and shakapacker v8.4 with auto-registration and enhanced dev tooling
This change is
Summary by CodeRabbit
New Features
Refactor
Chores