Add TypeScript support to React on Rails install generator #1786
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an optional --typescript/-T generator flag, TypeScript-aware templates and typings (components, Redux), tsconfig and CSS module d.ts, package-manager-aware dependency install fallbacks, ESLint ignores for generator templates, a component_extension helper, and expanded generator specs for TypeScript paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant IG as InstallGenerator
participant GH as GeneratorHelper
participant GM as GeneratorMessages
participant PM as PackageManager
Dev->>IG: rails g react_on_rails:install [--typescript|-T] [--redux]
alt typescript flag set
IG->>IG: create_typescript_config()
IG->>IG: create_css_module_types()
IG->>GH: add_npm_dependencies([typescript,@types/react,@types/react-dom,@babel/preset-typescript])
alt helper install failed
IG->>GM: detect_package_manager()
IG->>PM: run install command (npm/yarn/pnpm/bun) via build_install_args
alt install failed
IG->>Dev: warn with manual install command
end
end
end
IG->>IG: invoke appropriate generator (--redux? react_with_redux : react_no_redux) passing typescript flag
IG-->>Dev: generation complete
sequenceDiagram
autonumber
participant Rails as Rails View
participant App as HelloWorldApp.client.tsx
participant Store as configureStore
participant Provider as Provider
participant UI as HelloWorldContainer
Rails->>App: render with props
App->>Store: configureStore(props)
Store-->>App: store
App->>Provider: <Provider store={store}>
Provider->>UI: render connected component
UI-->>Rails: DOM/SSR output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Code Review: TypeScript Support for React on Rails Install GeneratorThank you for this valuable contribution! The TypeScript support addition is well-implemented overall. Here's my detailed review: ✅ Strengths
🔍 Issues to Address1. Critical: Dead Code
2. Inconsistent Package Manager Usage
3. Incomplete TypeScript Coverage for Redux
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
189-191: cli_exists? is not Windows-safewhich does not exist on Windows, causing false negatives (affects missing_package_manager? and detect_package_manager fallback).
- def cli_exists?(command) - system("which #{command} > /dev/null 2>&1") - end + def cli_exists?(command) + if ReactOnRails::Utils.running_on_windows? + system("where #{command} > NUL 2>&1") + else + system("command -v #{command} >/dev/null 2>&1") + end + end
🧹 Nitpick comments (10)
eslint.config.ts (1)
47-48: Global ignore makes template-specific ESLint overrides unreachable.Adding '/templates//' to globalIgnores prevents later rules for 'lib/generators/react_on_rails/templates/**/' (Lines 151-160) from ever applying. Drop the global ignore or narrow it.
Apply this diff:
- // generator templates - '**/templates/**/*',lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx (1)
1-30: Minor TS ergonomics for onChange.Prefer
e.currentTarget.value(better typings) and aChangeEventHandleralias.- onChange={(e: React.ChangeEvent<HTMLInputElement>) => updateName(e.target.value)} + onChange={(e) => updateName(e.currentTarget.value)}Optionally:
// above component import type { ChangeEventHandler } from 'react'; const onNameChange: ChangeEventHandler<HTMLInputElement> = (e) => updateName(e.currentTarget.value);lib/generators/react_on_rails/react_with_redux_generator.rb (1)
38-41: No-op gsub: safe to remove.Search and replace string equals replacement; drop these lines or fix the pattern if something else was intended.
- ror_client_file = "app/javascript/src/HelloWorldApp/ror_components/HelloWorldApp.client.#{extension}" - gsub_file(ror_client_file, "../store/helloWorldStore", "../store/helloWorldStore") - gsub_file(ror_client_file, "../containers/HelloWorldContainer", - "../containers/HelloWorldContainer") + # Template already uses the correct relative imports; no path adjustments required.lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx (1)
1-30: Minor TS/React nits.
- Prefer
e.currentTarget.valuefor stricter typings.useState(props.name)infersstring; generic arg is optional.- const [name, setName] = useState<string>(props.name); + const [name, setName] = useState(props.name); @@ - onChange={(e: React.ChangeEvent<HTMLInputElement>) => setName(e.target.value)} + onChange={(e) => setName(e.currentTarget.value)}lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx (1)
1-1: Use type-only React import with react-jsx runtimeNo runtime React usage; keep React.FC typing via a type-only import to avoid an unnecessary bundle import.
-import React from 'react'; +import type React from 'react';spec/react_on_rails/generators/install_generator_spec.rb (1)
1-5: Require JSON before parsing tsconfig contentAvoid NameError by requiring json in this spec file.
# frozen_string_literal: true +require "json" require_relative "../support/generator_spec_helper" require_relative "../support/version_test_helpers"lib/generators/react_on_rails/install_generator.rb (4)
360-367: Broaden package-manager detection and align fallback behaviorAlso detect Bun by CLI (not only lockfile) to match other PMs, improving first-run ergonomics.
def detect_package_manager return "yarn" if File.exist?("yarn.lock") return "pnpm" if File.exist?("pnpm-lock.yaml") - return "bun" if File.exist?("bun.lockb") - return "npm" if File.exist?("package-lock.json") || cli_exists?("npm") + return "bun" if File.exist?("bun.lockb") || cli_exists?("bun") + return "npm" if File.exist?("package-lock.json") || cli_exists?("npm") nil end
360-367: Deduplicate detect_package_manager logicA similar detect method exists elsewhere (generator_messages.rb). Centralize this in a shared helper to prevent drift.
400-432: configure_babel_for_typescript is unusedEither remove to reduce surface area or invoke it; you already install @babel/preset-typescript in install_typescript_dependencies.
- def configure_babel_for_typescript - # Install Babel TypeScript preset - package_manager = detect_package_manager - return unless package_manager - babel_typescript_package = "@babel/preset-typescript" - install_command = case package_manager - when "npm" then "npm install --save-dev #{babel_typescript_package}" - when "yarn" then "yarn add --dev #{babel_typescript_package}" - when "pnpm" then "pnpm add --save-dev #{babel_typescript_package}" - when "bun" then "bun add --dev #{babel_typescript_package}" - end - puts Rainbow("📝 Installing Babel TypeScript preset...").yellow - success = system(install_command) - return if success - warning = <<~MSG.strip - ⚠️ Failed to install Babel TypeScript preset automatically. - Please run manually: - #{install_command} - TypeScript compilation may not work without this preset. - MSG - GeneratorMessages.add_warning(warning) - nil - end + # def configure_babel_for_typescript; end # intentionally removed (handled in install_typescript_dependencies)
369-387: TSConfig defaults are fine; consider TS5-friendly tweaks (optional)If targeting Node 18+ and modern bundlers, consider:
- target: ES2022
- lib: ["dom", "dom.iterable", "ES2022"]
- moduleResolution: "bundler" (or "node16") for improved ESM/node-res semantics.
- "target" => "es2018", - "lib" => ["dom", "dom.iterable", "es6"], + "target" => "ES2022", + "lib" => ["dom", "dom.iterable", "ES2022"], - "moduleResolution" => "node", + "moduleResolution" => "bundler",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
eslint.config.ts(1 hunks)lib/generators/react_on_rails/install_generator.rb(4 hunks)lib/generators/react_on_rails/react_no_redux_generator.rb(1 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(3 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsx(1 hunks)package.json(2 hunks)spec/react_on_rails/generators/install_generator_spec.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsxeslint.config.tslib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsxlib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsxlib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsxlib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
spec/react_on_rails/generators/install_generator_spec.rblib/generators/react_on_rails/react_no_redux_generator.rblib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/react_no_redux_generator.rblib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
🧠 Learnings (3)
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsxlib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx
📚 Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to {coverage,tmp,gen-examples,node_package/lib,node_modules,spec/dummy/node_modules,spec/dummy/tmp,spec/dummy/app/assets/webpack,spec/dummy/log}/** : Exclude generated/build/vendor directories from IDE indexing to avoid slowdowns
Applied to files:
eslint.config.ts
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rb
🧬 Code graph analysis (5)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsx (1)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.jsx (1)
HelloWorldApp(10-14)
spec/react_on_rails/generators/install_generator_spec.rb (3)
spec/react_on_rails/support/generator_spec_helper.rb (1)
run_generator_test_with_args(62-72)lib/generators/react_on_rails/install_generator.rb (1)
include(11-437)lib/generators/react_on_rails/react_no_redux_generator.rb (1)
include(8-45)
lib/generators/react_on_rails/react_no_redux_generator.rb (2)
lib/generators/react_on_rails/react_with_redux_generator.rb (1)
copy_base_files(25-42)lib/generators/react_on_rails/base_generator.rb (1)
copy_base_files(34-46)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx (3)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.jsx (1)
HelloWorldApp(10-14)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.js (1)
configureStore(4-4)spec/dummy/client/app/components/HelloWorldContainer.jsx (1)
HelloWorldContainer(10-12)
lib/generators/react_on_rails/install_generator.rb (1)
lib/generators/react_on_rails/generator_messages.rb (2)
detect_package_manager(183-193)add_warning(13-15)
⏰ 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). (9)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build
- GitHub Check: claude-review
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
🔇 Additional comments (10)
lib/generators/react_on_rails/react_no_redux_generator.rb (2)
14-18: Option looks good; confirm predicate access style.Ensure
options.typescript?is supported in your generator stack; if not, useoptions[:typescript].
21-33: LGTM: TS/JSX switching and copy list are correct.Source/destination paths align with the new TS templates, including the CSS module in ror_components.
lib/generators/react_on_rails/react_with_redux_generator.rb (3)
11-15: TS option: same predicate access check as other generator.Verify
options.typescript?works; otherwise useoptions[:typescript].
27-33: LGTM: extension switching and TSX/JSX file copies.Paths line up with the new Redux TS templates.
46-61: LGTM: keep Redux infra in .js; component toggles TSX/JSX.Reasonable default; TS can be layered in later without blocking generation.
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsx (1)
1-5: LGTM: server entry re-exports client.Matches the established SSR alias pattern; fine for templates.
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx (1)
1-5: LGTM: server alias mirrors client component.Keeps SSR path simple and consistent with Redux template.
package.json (2)
125-125: ```shell
#!/bin/bash
set -euo pipefailecho "=== install_generator.rb (around detect_package_manager) ==="
sed -n '300,460p' lib/generators/react_on_rails/install_generator.rb || trueecho
echo "=== generator_messages.rb (around detect_package_manager) ==="
sed -n '160,220p' lib/generators/react_on_rails/generator_messages.rb || trueecho
echo "=== base_generator.rb (top -> package-manager usage) ==="
sed -n '1,260p' lib/generators/react_on_rails/base_generator.rb || trueecho
echo "=== Search for packageManager / package.json occurrences ==="
rg -n --hidden --no-ignore-vcs -S '"packageManager"|packageManager|package.json' || trueecho
echo "=== Search for explicit lockfile checks (yarn/npm/pnpm) ==="
rg -n -S 'yarn.lock|package-lock.json|pnpm-lock.yaml' || trueecho
echo "=== Search for reading/parsing package.json in ruby files ==="
rg -n -S "JSON.parse\(|File.read\([^)]*package.json|read\([^)]*package.json" || true--- `37-37`: ```shell #!/bin/bash set -euo pipefail echo "PWD: $(pwd)" echo echo "---- package.json (first 160 lines) ----" if [ -f package.json ]; then sed -n '1,160p' package.json || true else echo "package.json not found at repo root" fi echo echo "---- ripgrep: searching for @babel/preset-typescript or preset-typescript (including hidden, ignoring .gitignore disabled) ----" rg -n --hidden -uu "@babel/preset-typescript|preset-typescript" || true echo echo "---- Check common Babel config files ----" for f in .babelrc .babelrc.js .babelrc.cjs .babelrc.json babel.config.js babel.config.cjs babel.config.mjs; do if [ -f "$f" ]; then echo "=== $f ===" sed -n '1,240p' "$f" || true fi done echo echo "---- package.json babel field and scripts summary (python) ----" python - <<'PY' import json,sys try: j=json.load(open('package.json')) except Exception as e: print('package.json parse error or not found:', e) sys.exit(0) if 'babel' in j: import pprint print('package.json babel field:') pprint.pprint(j['babel']) else: print('no babel field in package.json') print('\npackage.json scripts (name: command)') for k in sorted(j.get('scripts',{}).keys()): print(k + ':', j['scripts'][k][:400]) print('\nDependencies keys (devDependencies and dependencies)') for field in ('dependencies','devDependencies'): d=j.get(field,{}) if d: print('\n' + field + ':') for k in sorted(d.keys()): print(' -', k, d[k]) else: print('\n' + field + ': none') PY echo echo "---- Search for Babel/tooling mentions in repo (excluding node_modules/dist) ----" rg -n --hidden -uu "babel-loader|@babel/core|@babel/preset-typescript|preset-typescript|babel-jest|babel-register|babel-plugin|\\bbabel\\b" --glob '!node_modules/**' --glob '!dist/**' || true echo echo "---- Search for TypeScript compilation / tooling (tsc, ts-loader, tsup, swc, esbuild) ----" rg -n --hidden -uu "\\btsc\\b|ts-loader|tsup|swc|@swc/core|esbuild|@babel/preset-typescript" --glob '!node_modules/**' --glob '!dist/**' || true echo echo "---- Done ----"lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx (1)
1-20: Template path inconsistent with TS+Redux spec — verify generator destinationFound template at lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx but specs/tests expect generated files under app/javascript/src/HelloWorldApp/ror_components; confirm the generator copies/uses this TS Redux template to that destination or relocate/rename the template and update the generator mappings to match tests.
🔍 Code Review for TypeScript Support PRGreat implementation of TypeScript support for the React on Rails install generator! This is a valuable addition that maintains backward compatibility while providing modern TypeScript capabilities. Here's my comprehensive review: ✅ Strengths
🎯 Code Quality ObservationsBest Practices
Architecture
🔧 Suggestions for Improvement
🛡️ Security & Performance
📋 Test Coverage
🐛 Potential Issues
✨ Overall AssessmentThis is a well-implemented feature that adds significant value to React on Rails. The code is clean, maintains consistency with the existing codebase, and provides a smooth TypeScript adoption path for developers. The implementation correctly leverages Shakapacker's automatic preset detection, making the integration seamless. Recommendation: ✅ Approved with minor suggestions The PR is ready to merge after considering the minor improvements suggested above, particularly the CSS module type definitions and the tsconfig.json overwrite protection. Great work on this implementation! The TypeScript support will be a valuable addition for the React on Rails community. 🎉 |
| "pnpm add --save-dev #{typescript_packages.join(' ')}" | ||
| when "bun" | ||
| "bun add --dev #{typescript_packages.join(' ')}" | ||
| end |
There was a problem hiding this comment.
Use https://github.com/shakacode/package_json to clean up code like this.
There was a problem hiding this comment.
It should use GeneratorHelper
package_json).
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive TypeScript support for the React on Rails install generator, enabling developers to create TypeScript-based React components and applications with proper type safety and modern tooling configuration.
- Added
--typescript/-Tflag to the install generator for TypeScript file generation - Implemented automatic TypeScript dependency management with package manager detection
- Created TypeScript templates for both base and Redux configurations with proper typing
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/react_on_rails/generators/install_generator_spec.rb | Added comprehensive test coverage for TypeScript generator functionality |
| package.json | Added @babel/preset-typescript dev dependency and cleaned up formatting |
| lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsx | New TypeScript Redux server component template |
| lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx | New TypeScript Redux client component template with proper interfaces |
| lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx | New TypeScript Redux component template with typed props and event handlers |
| lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx | New TypeScript base server component template |
| lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx | New TypeScript base client component template with proper typing |
| lib/generators/react_on_rails/react_with_redux_generator.rb | Modified to support TypeScript file generation and dependency installation |
| lib/generators/react_on_rails/react_no_redux_generator.rb | Modified to support TypeScript component generation |
| lib/generators/react_on_rails/install_generator.rb | Added TypeScript flag, dependency installation, and tsconfig.json generation |
| eslint.config.ts | Added templates directory to ESLint ignore patterns |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if options.typescript? | ||
| run "npm install redux react-redux @types/react-redux" | ||
| else | ||
| run "npm install redux react-redux" | ||
| end |
There was a problem hiding this comment.
This hardcodes npm as the package manager, but the install generator has logic to detect different package managers (yarn, pnpm, bun). Consider using the same package manager detection logic here for consistency.
| if options.typescript? | |
| run "npm install redux react-redux @types/react-redux" | |
| else | |
| run "npm install redux react-redux" | |
| end | |
| require_relative "package_manager_detector" | |
| package_manager = PackageManagerDetector.detect | |
| packages = if options.typescript? | |
| "redux react-redux @types/react-redux" | |
| else | |
| "redux react-redux" | |
| end | |
| install_command = | |
| case package_manager | |
| when :yarn | |
| "yarn add #{packages}" | |
| when :pnpm | |
| "pnpm add #{packages}" | |
| when :bun | |
| "bun add #{packages}" | |
| else | |
| "npm install #{packages}" | |
| end | |
| run install_command |
There was a problem hiding this comment.
Worse suggestion than CodeRabbit.
| def configure_babel_for_typescript | ||
| # Install Babel TypeScript preset | ||
| package_manager = detect_package_manager | ||
| return unless package_manager | ||
|
|
||
| babel_typescript_package = "@babel/preset-typescript" | ||
|
|
||
| install_command = case package_manager | ||
| when "npm" | ||
| "npm install --save-dev #{babel_typescript_package}" | ||
| when "yarn" | ||
| "yarn add --dev #{babel_typescript_package}" | ||
| when "pnpm" | ||
| "pnpm add --save-dev #{babel_typescript_package}" | ||
| when "bun" | ||
| "bun add --dev #{babel_typescript_package}" | ||
| end | ||
|
|
||
| puts Rainbow("📝 Installing Babel TypeScript preset...").yellow | ||
| success = system(install_command) | ||
| return if success | ||
|
|
||
| warning = <<~MSG.strip | ||
| ⚠️ Failed to install Babel TypeScript preset automatically. | ||
| Please run manually: | ||
| #{install_command} | ||
| TypeScript compilation may not work without this preset. | ||
| MSG | ||
| GeneratorMessages.add_warning(warning) | ||
| nil | ||
| end | ||
|
|
There was a problem hiding this comment.
The configure_babel_for_typescript method duplicates the package installation logic that already exists in install_typescript_dependencies. Since @babel/preset-typescript is already included in the typescript_packages array (line 326), this entire method appears to be redundant and unused.
| def configure_babel_for_typescript | |
| # Install Babel TypeScript preset | |
| package_manager = detect_package_manager | |
| return unless package_manager | |
| babel_typescript_package = "@babel/preset-typescript" | |
| install_command = case package_manager | |
| when "npm" | |
| "npm install --save-dev #{babel_typescript_package}" | |
| when "yarn" | |
| "yarn add --dev #{babel_typescript_package}" | |
| when "pnpm" | |
| "pnpm add --save-dev #{babel_typescript_package}" | |
| when "bun" | |
| "bun add --dev #{babel_typescript_package}" | |
| end | |
| puts Rainbow("📝 Installing Babel TypeScript preset...").yellow | |
| success = system(install_command) | |
| return if success | |
| warning = <<~MSG.strip | |
| ⚠️ Failed to install Babel TypeScript preset automatically. | |
| Please run manually: | |
| #{install_command} | |
| TypeScript compilation may not work without this preset. | |
| MSG | |
| GeneratorMessages.add_warning(warning) | |
| nil | |
| end |
There was a problem hiding this comment.
@justin808 Copilot is right.
|
@justin808 This shouldn't add a huge |
...rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx
Outdated
Show resolved
Hide resolved
...rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx
Outdated
Show resolved
Hide resolved
...ct_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx
Outdated
Show resolved
Hide resolved
| def detect_package_manager | ||
| return "yarn" if File.exist?("yarn.lock") | ||
| return "pnpm" if File.exist?("pnpm-lock.yaml") | ||
| return "bun" if File.exist?("bun.lockb") | ||
| return "npm" if File.exist?("package-lock.json") || cli_exists?("npm") | ||
|
|
||
| nil | ||
| end |
There was a problem hiding this comment.
This logic should be in GeneratorHelper too (trying to detect from package.json first).
There was a problem hiding this comment.
Or actually, in package_json: shakacode/package_json#22.
| "esModuleInterop" => true, | ||
| "allowSyntheticDefaultImports" => true, | ||
| "strict" => true, | ||
| "forceConsistentCasingInFileNames" => true, |
| "module" => "esnext", | ||
| "moduleResolution" => "node", | ||
| "resolveJsonModule" => true, | ||
| "isolatedModules" => true, |
There was a problem hiding this comment.
Not desirable in my opinion, it simply makes some TS code not compile which the user may want to compile.
| def configure_babel_for_typescript | ||
| # Install Babel TypeScript preset | ||
| package_manager = detect_package_manager | ||
| return unless package_manager | ||
|
|
||
| babel_typescript_package = "@babel/preset-typescript" | ||
|
|
||
| install_command = case package_manager | ||
| when "npm" | ||
| "npm install --save-dev #{babel_typescript_package}" | ||
| when "yarn" | ||
| "yarn add --dev #{babel_typescript_package}" | ||
| when "pnpm" | ||
| "pnpm add --save-dev #{babel_typescript_package}" | ||
| when "bun" | ||
| "bun add --dev #{babel_typescript_package}" | ||
| end | ||
|
|
||
| puts Rainbow("📝 Installing Babel TypeScript preset...").yellow | ||
| success = system(install_command) | ||
| return if success | ||
|
|
||
| warning = <<~MSG.strip | ||
| ⚠️ Failed to install Babel TypeScript preset automatically. | ||
| Please run manually: | ||
| #{install_command} | ||
| TypeScript compilation may not work without this preset. | ||
| MSG | ||
| GeneratorMessages.add_warning(warning) | ||
| nil | ||
| end | ||
|
|
There was a problem hiding this comment.
@justin808 Copilot is right.
| app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx | ||
| app/javascript/src/HelloWorld/ror_components/HelloWorld.module.css] | ||
| base_files.each { |file| copy_file("#{base_js_path}/#{file}", file) } | ||
| extension = options.typescript? ? "tsx" : "jsx" |
There was a problem hiding this comment.
Put in GeneratorHelper as def component_extension(options)?
| if options.typescript? | ||
| run "npm install redux react-redux @types/react-redux" | ||
| else | ||
| run "npm install redux react-redux" | ||
| end |
There was a problem hiding this comment.
Worse suggestion than CodeRabbit.
| "@babel/eslint-parser": "^7.26.10", | ||
| "@babel/preset-env": "^7.20.2", | ||
| "@babel/preset-react": "^7.26.3", | ||
| "@babel/preset-typescript": "^7.27.1", |
There was a problem hiding this comment.
Do we need this in our own devDependencies?
Code Review: TypeScript Support ImplementationI've completed a comprehensive review of this PR. Overall, this is a well-implemented feature that adds valuable TypeScript support to React on Rails. The implementation score: 8/10. ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
lib/generators/react_on_rails/install_generator.rb (3)
314-358: Deduplicate package install logic; reuseGeneratorHelper#add_npm_dependenciesand standard fallback.This re-implements package-manager detection and install commands. We already have helper flows using
package_jsonand consistent failure handling. Please switch to the established pattern.Apply this diff to simplify and align behavior:
def install_typescript_dependencies - puts Rainbow("📝 Installing TypeScript dependencies...").yellow - - # Determine the package manager to use - package_manager = detect_package_manager - return unless package_manager - - # Install TypeScript and React type definitions - typescript_packages = %w[ - typescript - @types/react - @types/react-dom - @babel/preset-typescript - ] - - install_command = case package_manager - when "npm" - "npm install --save-dev #{typescript_packages.join(' ')}" - when "yarn" - "yarn add --dev #{typescript_packages.join(' ')}" - when "pnpm" - "pnpm add --save-dev #{typescript_packages.join(' ')}" - when "bun" - "bun add --dev #{typescript_packages.join(' ')}" - end - - success = system(install_command) - unless success - warning = <<~MSG.strip - ⚠️ Failed to install TypeScript dependencies automatically. - - Please run manually: - #{install_command} - - TypeScript files will still be generated. - MSG - GeneratorMessages.add_warning(warning) - end + puts Rainbow("📝 Installing TypeScript dependencies...").yellow + typescript_packages = %w[typescript @types/react @types/react-dom @babel/preset-typescript] + # Try helper (auto-detects PM and edits package.json via package_json). If it succeeds, we're done. + return if add_npm_dependencies(typescript_packages, dev: true) + # Fallback to npm (consistent with BaseGenerator pattern). + success = run "npm install --save-dev #{typescript_packages.join(' ')}" + handle_npm_failure("TypeScript dependencies", typescript_packages) unless success
360-367: Remove duplicateddetect_package_manager; centralize detection.We already have detection (see
GeneratorHelper/GeneratorMessages). Keeping another copy risks drift (e.g., bun support here but not elsewhere). Prefer one source of truth.- def detect_package_manager - return "yarn" if File.exist?("yarn.lock") - return "pnpm" if File.exist?("pnpm-lock.yaml") - return "bun" if File.exist?("bun.lockb") - return "npm" if File.exist?("package-lock.json") || cli_exists?("npm") - - nil - end + # Package manager detection is provided by existing helpers. Remove this method.
369-398: Don’t overwrite existing tsconfig; update defaults (moduleResolution, lib/target) and strengthen strictness.
- Guard: skip writing if
tsconfig.jsonexists, or writetsconfig.react-on-rails.jsonand instruct users toextendsit.- Use
moduleResolution: "bundler"for modern bundlers.- Drop explicit
libunless you need to deviate; current mix (target: es2018,lib: es6) is inconsistent.- Add
noUncheckedIndexedAccess: true(good safety that’s not covered bystrict).Proposed change:
def create_typescript_config - tsconfig_content = { + if File.exist?("tsconfig.json") + puts Rainbow("ℹ️ tsconfig.json exists; not overwriting. Please ensure jsx:'react-jsx' and moduleResolution:'bundler'.").blue + return + end + + tsconfig_content = { "compilerOptions" => { - "target" => "es2018", - "lib" => ["dom", "dom.iterable", "es6"], + "target" => "ES2022", "allowJs" => true, "skipLibCheck" => true, "esModuleInterop" => true, "allowSyntheticDefaultImports" => true, "strict" => true, + "noUncheckedIndexedAccess" => true, "forceConsistentCasingInFileNames" => true, "noFallthroughCasesInSwitch" => true, "module" => "esnext", - "moduleResolution" => "node", + "moduleResolution" => "bundler", "resolveJsonModule" => true, "isolatedModules" => true, "noEmit" => true, "jsx" => "react-jsx" }, "include" => [ "app/javascript/**/*" ], - "exclude" => [ - "node_modules" - ] + "exclude" => ["node_modules"] }lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx (1)
22-22: Inline handler can be simplified (duplicate of prior review).Apply:
- onChange={(e: React.ChangeEvent<HTMLInputElement>) => updateName(e.target.value)} + onChange={e => updateName(e.target.value)}lib/generators/react_on_rails/react_with_redux_generator.rb (2)
104-132: De‑duplicate PM fallback logic; centralize in GeneratorHelper.This file re‑implements install fallback and command builders. Consolidate into GeneratorHelper (single source of truth), then delete install_packages_with_fallback/build_install_command here.
Apply this diff after moving fallback into GeneratorHelper:
- private - - def install_packages_with_fallback(packages, dev:, package_manager:) - packages_str = packages.join(" ") - install_command = build_install_command(package_manager, dev, packages_str) - - success = system(install_command) - return if success - - warning = <<~MSG.strip - ⚠️ Failed to install Redux dependencies automatically. - - Please run manually: - #{install_command} - MSG - GeneratorMessages.add_warning(warning) - end - - def build_install_command(package_manager, dev, packages_str) - commands = { - "npm" => { dev: "npm install --save-dev", prod: "npm install" }, - "yarn" => { dev: "yarn add --dev", prod: "yarn add" }, - "pnpm" => { dev: "pnpm add --save-dev", prod: "pnpm add" }, - "bun" => { dev: "bun add --dev", prod: "bun add" } - } - - command_type = dev ? :dev : :prod - "#{commands[package_manager][command_type]} #{packages_str}" - end + privateIf you prefer to keep local fallback, ensure a single place builds commands for all generators to avoid drift.
91-102: Undefined detect_package_manager here; inline detection or move to helper.This class doesn’t define detect_package_manager, and it’s not in GeneratorHelper (per snippet). Calling it will raise at runtime.
Apply this minimal inline detection (avoids cross‑generator coupling):
- package_manager = detect_package_manager - return unless package_manager + package_manager = + if File.exist?("yarn.lock") + "yarn" + elsif File.exist?("pnpm-lock.yaml") + "pnpm" + elsif File.exist?("bun.lockb") + "bun" + else + "npm" + endAlternatively, move all fallback logic into GeneratorHelper and call only add_npm_dependencies from here. See next comment.
🧹 Nitpick comments (9)
eslint.config.ts (1)
47-49: Templates ignore looks fine; consider also ignoring .d.ts and verify overlap with the templates override block.Adding TS/TSX under templates to global ignores prevents type-aware linting and bypasses the templates-specific override below. If that's intended, LGTM. If you still want basic linting without type-checking, move these paths into a dedicated override and disable the type-checked config there instead of globally ignoring.
Add .d.ts to avoid stray typedefs being linted:
// generator templates - exclude TypeScript templates that need tsconfig.json '**/templates/**/*.tsx', '**/templates/**/*.ts', + '**/templates/**/*.d.ts',lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx (2)
1-1: Prefer type-only React import with the automatic JSX runtime.Avoid default React import when using the automatic runtime.
-import React from 'react'; +import { useMemo, type FC } from 'react';
13-17: Memoize the store to avoid re-creating it on re-renders.-const HelloWorldApp: React.FC<HelloWorldAppProps> = (props) => ( - <Provider store={configureStore(props)}> - <HelloWorldContainer /> - </Provider> -); +const HelloWorldApp: FC<HelloWorldAppProps> = (props) => { + const store = useMemo(() => configureStore(props), [props]); + return ( + <Provider store={store}> + <HelloWorldContainer /> + </Provider> + ); +};lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts (2)
5-8: Tighten Rails props typing; avoidany.Model initial state precisely and keep extras typed as
unknown.Apply:
-export interface RailsProps { - name: string; - [key: string]: any; // Allow additional props from Rails -} +export type RailsProps = PreloadedState<HelloWorldState> & Record<string, unknown>;
13-14: Optional: prefer Redux Toolkit for TS ergonomics.
configureStoregives better types/devtools by default.Example:
// import { configureStore } from '@reduxjs/toolkit'; // const configureStore = (railsProps: RailsProps) => // configureStore({ reducer: helloWorldReducer, preloadedState: railsProps }); // export type HelloWorldStore = ReturnType<typeof configureStore>;lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts (1)
19-19: Use stable docs URL.Avoid
masterlinks that can drift.Apply:
-// See https://github.com/reactjs/react-redux/blob/master/docs/api.md#examples +// See https://react-redux.js.org/api/connectlib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx (1)
15-15: Prevent form submit refresh.Stops accidental page reload on Enter.
Apply:
- <form> + <form onSubmit={e => e.preventDefault()}>lib/generators/react_on_rails/react_with_redux_generator.rb (2)
41-45: No‑op gsubs; either remove or fix target/replacement.Both gsub_file calls replace a string with itself. If paths don’t need updates, drop these lines; otherwise, correct the replacement.
Apply this diff to remove the dead code:
- ror_client_file = "app/javascript/src/HelloWorldApp/ror_components/HelloWorldApp.client.#{extension}" - gsub_file(ror_client_file, "../store/helloWorldStore", "../store/helloWorldStore") - gsub_file(ror_client_file, "../containers/HelloWorldContainer", - "../containers/HelloWorldContainer") + # Client imports already resolve correctly relative to ror_components.Please confirm the imports inside the template already point to ../store/helloWorldStore and ../containers/HelloWorldContainer for both TSX/JSX.
106-121: Avoid hard failure when warning; ensure GeneratorMessages is loaded and improve text.On failure, this path may raise NameError if GeneratorMessages wasn’t required yet. Also, the message should list the actual packages.
Apply this diff:
- success = system(install_command) + success = system(install_command) return if success - warning = <<~MSG.strip - ⚠️ Failed to install Redux dependencies automatically. - - Please run manually: - #{install_command} - MSG + require_relative "generator_messages" unless defined?(GeneratorMessages) + warning = <<~MSG.strip + ⚠️ Failed to install npm packages automatically: #{packages.join(' ')} + + Please run manually: + #{install_command} + MSG GeneratorMessages.add_warning(warning)Optionally prefer Thor’s run(...) over system(...) for consistent logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
eslint.config.ts(1 hunks)lib/generators/react_on_rails/install_generator.rb(4 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(3 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/constants/helloWorldConstants.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts(1 hunks)spec/react_on_rails/generators/install_generator_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/react_on_rails/generators/install_generator_spec.rb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/constants/helloWorldConstants.tslib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.tslib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.tslib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.tseslint.config.tslib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.tslib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsxlib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
🧠 Learnings (5)
📚 Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use ESLint for all JavaScript/TypeScript code
Applied to files:
eslint.config.ts
📚 Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to {coverage,tmp,gen-examples,node_package/lib,node_modules,spec/dummy/node_modules,spec/dummy/tmp,spec/dummy/app/assets/webpack,spec/dummy/log}/** : Exclude generated/build/vendor directories from IDE indexing to avoid slowdowns
Applied to files:
eslint.config.ts
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rb
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx
🧬 Code graph analysis (8)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts (2)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts (1)
HelloWorldState(6-8)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.js (1)
configureStore(4-4)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts (2)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.js (1)
mapStateToProps(8-8)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts (1)
HelloWorldState(6-8)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.ts (1)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/constants/helloWorldConstants.ts (1)
HELLO_WORLD_NAME_UPDATE(3-3)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts (3)
spec/dummy/client/app/reducers/HelloWorldReducer.jsx (1)
action(12-12)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.ts (1)
HelloWorldAction(12-12)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/constants/helloWorldConstants.ts (1)
HELLO_WORLD_NAME_UPDATE(3-3)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx (2)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts (1)
PropsFromRedux(15-15)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.ts (1)
updateName(15-18)
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
lib/generators/react_on_rails/install_generator.rb (1)
detect_package_manager(360-367)lib/generators/react_on_rails/generator_helper.rb (1)
add_npm_dependencies(23-39)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx (4)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts (1)
RailsProps(5-8)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.jsx (1)
HelloWorldApp(10-14)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.js (1)
configureStore(4-4)spec/dummy/client/app/components/HelloWorldContainer.jsx (1)
HelloWorldContainer(10-12)
lib/generators/react_on_rails/install_generator.rb (1)
lib/generators/react_on_rails/generator_messages.rb (2)
detect_package_manager(183-193)add_warning(13-15)
⏰ 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). (10)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
🔇 Additional comments (15)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/constants/helloWorldConstants.ts (1)
1-6: LGTM: literal action type withas constis correct.Constant and derived type are clean and align with TS best practices for Redux action types.
lib/generators/react_on_rails/install_generator.rb (2)
24-30: Nice addition:--typescript/-Toption.Flag definition, default, and help text look good. Please ensure docs/USAGE mention the
-Talias and specs cover both long and short forms.
67-75: Invocation flow looks right.Installing TS deps before invoking sub-generators and threading
typescript: options.typescript?through is the correct order.lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts (1)
1-23: LGTM: typed reducer and state shape are clear.Action narrowing via constant and
combineReducers<HelloWorldState>usage look solid.lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.ts (1)
5-18: Looks good — clear, typed action creator.lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx (2)
16-16: Update CSS module reference after switching to default import.Apply:
- <label className={style.bright} htmlFor="name"> + <label className={styles.bright} htmlFor="name">
2-2: Import CSS modules via default export, not namespace.Namespace import often fails with CSS module typings.
Apply:
-import * as style from './HelloWorld.module.css'; +import styles from './HelloWorld.module.css';And update usage accordingly (see below).
⛔ Skipped due to learnings
Learnt from: justin808 PR: shakacode/react_on_rails#1770 File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2 Timestamp: 2025-09-16T08:01:11.146Z Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.lib/generators/react_on_rails/react_with_redux_generator.rb (8)
4-4: Good: pulls in shared generator helpers.
9-10: Good: mixes in GeneratorHelper for PM‑agnostic installs.
30-30: Deriving extension from option looks good.
33-36: Copying TSX/JSX startup files to ror_components looks correct.
49-51: Per‑file extensions for component vs Redux infra are handled cleanly.
53-60: Bulk copy of Redux infra files looks right and extension‑aware.
63-64: Component copy path/extension looks correct.
122-132: Command map looks correct across npm/yarn/pnpm/bun.
...ct_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx
Outdated
Show resolved
Hide resolved
...ils/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts
Outdated
Show resolved
Hide resolved
...act_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/generators/react_on_rails/install_generator.rb (2)
335-364: Harden and modernize tsconfig: don’t overwrite, use bundler resolution, avoid overriding defaults, add noUncheckedIndexedAccess.
- Skip creation if
tsconfig.jsonexists (don’t clobber user config).- Use
"moduleResolution": "bundler"(TS 5+ recommendation for bundlers).- Drop
"lib"override andexclude: ["node_modules"](defaults suffice).- Add
"noUncheckedIndexedAccess": true.Apply this diff:
def create_typescript_config - tsconfig_content = { + # Do not overwrite an existing config; ask user to merge + if File.exist?("tsconfig.json") + GeneratorMessages.add_warning("⚠️ tsconfig.json already exists; skipping creation. Merge React on Rails settings manually if desired.") + return + end + + tsconfig_content = { "compilerOptions" => { - "target" => "es2018", - "lib" => ["dom", "dom.iterable", "es6"], + "target" => "es2018", "allowJs" => true, "skipLibCheck" => true, "esModuleInterop" => true, "allowSyntheticDefaultImports" => true, - "strict" => true, + "strict" => true, + "noUncheckedIndexedAccess" => true, "forceConsistentCasingInFileNames" => true, "noFallthroughCasesInSwitch" => true, "module" => "esnext", - "moduleResolution" => "node", + "moduleResolution" => "bundler", "resolveJsonModule" => true, "isolatedModules" => true, "noEmit" => true, "jsx" => "react-jsx" }, "include" => [ "app/javascript/**/*" - ], - "exclude" => [ - "node_modules" ] } File.write("tsconfig.json", JSON.pretty_generate(tsconfig_content)) puts Rainbow("✅ Created tsconfig.json").green end
314-333: Fix NameError and redundant PM detection in install_typescript_dependencies.CI failure shows
detect_package_manageris undefined here; and the variable isn’t used anyway. Rely onadd_npm_dependencies(package_json) with npm fallback and markdev: truein the failure hint.Apply this diff:
def install_typescript_dependencies puts Rainbow("📝 Installing TypeScript dependencies...").yellow - - # Determine the package manager to use - package_manager = detect_package_manager - return unless package_manager - # Install TypeScript and React type definitions typescript_packages = %w[ typescript @types/react @types/react-dom @babel/preset-typescript ] - return if add_npm_dependencies(typescript_packages, dev: true) + # Prefer package_json (via GeneratorHelper); fall back to npm if unavailable + return if add_npm_dependencies(typescript_packages, dev: true) - success = run "npm install --save-dev #{typescript_packages.join(' ')}" - handle_npm_failure("TypeScript dependencies", typescript_packages) unless success + success = run("npm install --save-dev #{typescript_packages.join(' ')}") + handle_npm_failure("TypeScript dependencies", typescript_packages, dev: true) unless success endRun to confirm no remaining references:
#!/bin/bash rg -n -C2 '\bdetect_package_manager\b' rg -n -C2 '\bcreate_typescript_config\b'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/generators/react_on_rails/install_generator.rb(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/generators/react_on_rails/install_generator.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/install_generator.rb
🧠 Learnings (1)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/install_generator.rb
🧬 Code graph analysis (1)
lib/generators/react_on_rails/install_generator.rb (3)
lib/generators/react_on_rails/generator_messages.rb (1)
detect_package_manager(183-193)lib/generators/react_on_rails/generator_helper.rb (1)
add_npm_dependencies(23-39)lib/generators/react_on_rails/base_generator.rb (1)
handle_npm_failure(232-244)
🪛 GitHub Actions: Rspec test for gem
lib/generators/react_on_rails/install_generator.rb
[error] 318-318: NameError: undefined local variable or method detect_package_manager in InstallGenerator#install_typescript_dependencies (called during rspec tests).
⏰ 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: dummy-app-integration-tests (3.4, 22)
- GitHub Check: examples (3.4, latest)
🔇 Additional comments (2)
lib/generators/react_on_rails/install_generator.rb (2)
4-4: Require JSON is appropriate.Needed for JSON.pretty_generate used below.
24-30: Flag API LGTM.Clear
--typescript/-Toption with sensible defaults.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
372-401: Don’t overwrite existing tsconfig; prefer modern resolver; simplify libs.
- Skip creation if tsconfig.json already exists (avoid clobbering user config).
- Use
"moduleResolution": "bundler"for TS 5+ with webpack/shakapacker.- Omit explicit
"lib"; defaults match target and avoid downgrading to"es6".- Optional: enable
noUncheckedIndexedAccessfor safer types.Apply this diff:
def create_typescript_config - tsconfig_content = { + # Do not overwrite an existing user tsconfig + if File.exist?("tsconfig.json") + puts Rainbow("ℹ️ tsconfig.json already exists; skipping creation").cyan + return + end + + tsconfig_content = { "compilerOptions" => { - "target" => "es2018", - "lib" => ["dom", "dom.iterable", "es6"], + "target" => "es2018", "allowJs" => true, "skipLibCheck" => true, "esModuleInterop" => true, "allowSyntheticDefaultImports" => true, "strict" => true, + "noUncheckedIndexedAccess" => true, "forceConsistentCasingInFileNames" => true, "noFallthroughCasesInSwitch" => true, "module" => "esnext", - "moduleResolution" => "node", + "moduleResolution" => "bundler", "resolveJsonModule" => true, "isolatedModules" => true, "noEmit" => true, "jsx" => "react-jsx" },
🧹 Nitpick comments (2)
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
41-46: Remove no‑op gsub_file calls.Both replacements use identical from/to strings, so they do nothing. Safer to delete to avoid confusion.
Apply this diff:
- # Update import paths in client component - ror_client_file = "app/javascript/src/HelloWorldApp/ror_components/HelloWorldApp.client.#{extension}" - gsub_file(ror_client_file, "../store/helloWorldStore", "../store/helloWorldStore") - gsub_file(ror_client_file, "../containers/HelloWorldContainer", - "../containers/HelloWorldContainer") + # Client imports in the template are already correct; no path rewrites needed.
113-129: Builds safe, explicit PM commands.Validated PM allowlist and explicit dev/prod maps look good. Consider moving these helpers into
GeneratorHelperto DRY with other generators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/generators/react_on_rails/install_generator.rb(4 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(3 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
🧠 Learnings (2)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rb
🧬 Code graph analysis (2)
lib/generators/react_on_rails/react_with_redux_generator.rb (3)
lib/generators/react_on_rails/install_generator.rb (1)
include(11-406)lib/generators/react_on_rails/generator_helper.rb (1)
add_npm_dependencies(23-39)lib/generators/react_on_rails/generator_messages.rb (1)
detect_package_manager(183-193)
lib/generators/react_on_rails/install_generator.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
add_npm_dependencies(23-39)lib/generators/react_on_rails/generator_messages.rb (1)
add_warning(13-15)
🪛 GitHub Actions: Lint JS and Ruby
lib/generators/react_on_rails/install_generator.rb
[error] 333-333: RuboCop: Style/GuardClause offense at lib/generators/react_on_rails/install_generator.rb:333:9 (autocorrectable). Use a guard clause (return if success) instead of wrapping the code inside a conditional expression. Command: bundle exec rubocop.
⏰ 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). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: claude-review
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
🔇 Additional comments (4)
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
79-93: Good: PM‑agnostic install with dev types avoided.Installs only
reduxandreact-reduxand relies on helper + PM fallback. No unnecessary@types/react-redux. LGTM.If any older code paths still attempt to install
@types/react-redux, please remove them; React‑Redux ships its own types.
50-66: Do not change HelloWorldContainer to .tsx — it's a pure connector with no JSX.
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts exports connector(HelloWorld) and contains no JSX, so using .ts is correct.Likely an incorrect or invalid review comment.
lib/generators/react_on_rails/install_generator.rb (2)
24-30: CLI option design: -T alias included.Matches PR promise and mirrors other flags. LGTM.
344-371: CSS module typings: solid default coverage.Covers css/scss/sass module patterns and writes to app/javascript/types. LGTM.
| def install_typescript_dependencies | ||
| puts Rainbow("📝 Installing TypeScript dependencies...").yellow | ||
|
|
||
| # Install TypeScript and React type definitions | ||
| typescript_packages = %w[ | ||
| typescript | ||
| @types/react | ||
| @types/react-dom | ||
| @babel/preset-typescript | ||
| ] | ||
|
|
||
| # Try using GeneratorHelper first (package manager agnostic) | ||
| return if add_npm_dependencies(typescript_packages, dev: true) | ||
|
|
||
| # Fallback to npm if GeneratorHelper fails | ||
| success = run "npm install --save-dev #{typescript_packages.join(' ')}" | ||
| unless success | ||
| warning = <<~MSG.strip | ||
| ⚠️ Failed to install TypeScript dependencies automatically. | ||
| Please run manually: | ||
| npm install --save-dev #{typescript_packages.join(' ')} | ||
| MSG | ||
| GeneratorMessages.add_warning(warning) | ||
| end | ||
| end |
There was a problem hiding this comment.
Use PM‑aware fallback and fix RuboCop guard‑clause offense.
Current fallback hardcodes npm and trips Style/GuardClause at Line 333. Detect PM like other generators and use a guard clause.
Apply this diff:
# Try using GeneratorHelper first (package manager agnostic)
return if add_npm_dependencies(typescript_packages, dev: true)
- # Fallback to npm if GeneratorHelper fails
- success = run "npm install --save-dev #{typescript_packages.join(' ')}"
- unless success
- warning = <<~MSG.strip
- ⚠️ Failed to install TypeScript dependencies automatically.
-
- Please run manually:
- npm install --save-dev #{typescript_packages.join(' ')}
- MSG
- GeneratorMessages.add_warning(warning)
- end
+ # Fallback: detect package manager and install
+ pm =
+ if File.exist?("yarn.lock")
+ "yarn"
+ elsif File.exist?("pnpm-lock.yaml")
+ "pnpm"
+ elsif File.exist?("bun.lockb")
+ "bun"
+ else
+ "npm"
+ end
+ cmds = {
+ "npm" => "npm install --save-dev",
+ "yarn" => "yarn add --dev",
+ "pnpm" => "pnpm add --save-dev",
+ "bun" => "bun add --dev"
+ }
+ install_cmd = "#{cmds[pm]} #{typescript_packages.join(' ')}"
+ success = run install_cmd
+ return if success
+
+ warning = <<~MSG.strip
+ ⚠️ Failed to install TypeScript dependencies automatically.
+
+ Please run manually:
+ #{install_cmd}
+ MSG
+ GeneratorMessages.add_warning(warning)📝 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.
| def install_typescript_dependencies | |
| puts Rainbow("📝 Installing TypeScript dependencies...").yellow | |
| # Install TypeScript and React type definitions | |
| typescript_packages = %w[ | |
| typescript | |
| @types/react | |
| @types/react-dom | |
| @babel/preset-typescript | |
| ] | |
| # Try using GeneratorHelper first (package manager agnostic) | |
| return if add_npm_dependencies(typescript_packages, dev: true) | |
| # Fallback to npm if GeneratorHelper fails | |
| success = run "npm install --save-dev #{typescript_packages.join(' ')}" | |
| unless success | |
| warning = <<~MSG.strip | |
| ⚠️ Failed to install TypeScript dependencies automatically. | |
| Please run manually: | |
| npm install --save-dev #{typescript_packages.join(' ')} | |
| MSG | |
| GeneratorMessages.add_warning(warning) | |
| end | |
| end | |
| def install_typescript_dependencies | |
| puts Rainbow("📝 Installing TypeScript dependencies...").yellow | |
| # Install TypeScript and React type definitions | |
| typescript_packages = %w[ | |
| typescript | |
| @types/react | |
| @types/react-dom | |
| @babel/preset-typescript | |
| ] | |
| # Try using GeneratorHelper first (package manager agnostic) | |
| return if add_npm_dependencies(typescript_packages, dev: true) | |
| # Fallback: detect package manager and install | |
| pm = | |
| if File.exist?("yarn.lock") | |
| "yarn" | |
| elsif File.exist?("pnpm-lock.yaml") | |
| "pnpm" | |
| elsif File.exist?("bun.lockb") | |
| "bun" | |
| else | |
| "npm" | |
| end | |
| cmds = { | |
| "npm" => "npm install --save-dev", | |
| "yarn" => "yarn add --dev", | |
| "pnpm" => "pnpm add --save-dev", | |
| "bun" => "bun add --dev" | |
| } | |
| install_cmd = "#{cmds[pm]} #{typescript_packages.join(' ')}" | |
| success = run install_cmd | |
| return if success | |
| warning = <<~MSG.strip | |
| ⚠️ Failed to install TypeScript dependencies automatically. | |
| Please run manually: | |
| #{install_cmd} | |
| MSG | |
| GeneratorMessages.add_warning(warning) | |
| end |
🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[error] 333-333: RuboCop: Style/GuardClause offense at lib/generators/react_on_rails/install_generator.rb:333:9 (autocorrectable). Use a guard clause (return if success) instead of wrapping the code inside a conditional expression. Command: bundle exec rubocop.
🤖 Prompt for AI Agents
In lib/generators/react_on_rails/install_generator.rb around lines 317 to 342,
the fallback install hardcodes npm and uses an unless block which triggers
RuboCop Style/GuardClause; change it to detect the package manager (like other
generators do), build the correct install command for yarn/pnpm/npm (e.g. yarn
add --dev, pnpm add -D, npm install --save-dev) and run it, using a guard clause
(return if run command succeeds); if the command fails, then add the existing
warning message to GeneratorMessages.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| puts Rainbow("✅ Created CSS module type definitions").green | ||
| end | ||
|
|
||
| def create_typescript_config |
There was a problem hiding this comment.
The create_typescript_config method is defined but never called. This means TypeScript projects won't get a tsconfig.json file, which is essential for TypeScript compilation.
| "homepage": "https://github.com/shakacode/react_on_rails#readme", | ||
| "packageManager": "yarn@1.22.22+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e", | ||
| "dependencies": {} | ||
| "packageManager": "yarn@1.22.22+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e" |
There was a problem hiding this comment.
[nitpick] The removal of the empty dependencies object and trailing comma creates inconsistent JSON formatting. Consider keeping consistent formatting patterns.
Code Review for PR #1786: Add TypeScript support to React on Rails install generatorI've conducted a comprehensive review of this pull request and am pleased to report that this is a high-quality implementation of TypeScript support for React on Rails. Here's my detailed assessment: ✅ Strengths1. Excellent Code Quality
2. Robust TypeScript Implementation
3. Smart Dependency Management
4. Excellent Test Coverage
5. Developer Experience
🔍 Security Assessment
⚡ Performance Considerations
💡 Minor Suggestions for Future Improvements
📝 Best Practices Observed
🎯 ConclusionThis is a well-implemented, production-ready feature that successfully adds TypeScript support to React on Rails. The implementation is clean, secure, performant, and maintains excellent backward compatibility. The test coverage is comprehensive, and the developer experience is thoughtfully designed. Recommendation: APPROVE ✅ Great work on this implementation! The TypeScript support will be a valuable addition for teams wanting type safety in their React on Rails applications. Reviewed focusing on: code quality, type safety, security, performance, test coverage, and adherence to project conventions as specified in CLAUDE.md |
justin808
force-pushed
the
justin808-ts-generator
branch
from
9158565 to
0e982be
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts (1)
1-4: Type-only imports: LGTM; fixes Babel TS preset issue.Splitting value vs type imports avoids runtime type imports with Babel TS preset.
lib/generators/react_on_rails/react_no_redux_generator.rb (1)
21-22: Centralize extension logic in GeneratorHelperUse a shared helper to avoid repeated ternaries across generators.
- extension = options.typescript? ? "tsx" : "jsx" + extension = component_extension(options)
🧹 Nitpick comments (12)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts (3)
7-10: Prefer unknown over any for RailsProps.Safer, satisfies common ESLint configs without weakening types.
Apply:
export interface RailsProps { name: string; - [key: string]: any; // Allow additional props from Rails + [key: string]: unknown; // Allow additional props from Rails }
15-16: Avoid casting railsProps; sanitize preloaded state.Only pass known reducer keys to preloaded state; removes the need for an unsafe cast and prevents stray keys persisting in state.
-const configureStore = (railsProps: RailsProps): HelloWorldStore => - createStore(helloWorldReducer, railsProps as PreloadedState<HelloWorldState>); +const configureStore = (railsProps: RailsProps): HelloWorldStore => { + const preloaded: PreloadedState<HelloWorldState> = { name: railsProps.name }; + return createStore(helloWorldReducer, preloaded); +};
15-16: Optional: consider Redux Toolkit for better TS DX.RTK’s
configureStoreimproves typing, devtools, and future-proofing. Fine to keepreduxhere if minimizing deps.eslint.config.ts (1)
47-49: Tighten ignore scope for templates and include .d.tsLimit ignores to generator templates to avoid unintentionally skipping other repo “templates” dirs; also ignore .d.ts if added later.
- // generator templates - exclude TypeScript templates that need tsconfig.json - '**/templates/**/*.tsx', - '**/templates/**/*.ts', + // generator templates - exclude TS files that require a tsconfig.json + 'lib/generators/**/templates/**/*.tsx', + 'lib/generators/**/templates/**/*.ts', + 'lib/generators/**/templates/**/*.d.ts',lib/generators/react_on_rails/react_no_redux_generator.rb (2)
14-17: Add -T alias for consistency with other generatorsExpose the documented shorthand here as well.
- class_option :typescript, - type: :boolean, - default: false, - desc: "Generate TypeScript files" + class_option :typescript, + type: :boolean, + default: false, + desc: "Generate TypeScript files", + aliases: "-T"
23-32: Avoid double‑copying HelloWorld.module.cssBaseGenerator already copies this CSS for the non‑Redux path; this duplicates work and can force overwrites when not using --force.
component_files = [ "app/javascript/src/HelloWorld/ror_components/HelloWorld.client.#{extension}", "app/javascript/src/HelloWorld/ror_components/HelloWorld.server.#{extension}", - "app/javascript/src/HelloWorld/ror_components/HelloWorld.module.css" ]spec/react_on_rails/generators/install_generator_spec.rb (3)
42-49: Assert CSS module type declarations are createdThis validates the documented CSS modules typings step.
it "creates tsconfig.json file" do assert_file "tsconfig.json" do |content| config = JSON.parse(content) expect(config["compilerOptions"]["jsx"]).to eq("react-jsx") expect(config["compilerOptions"]["strict"]).to be true expect(config["include"]).to include("app/javascript/**/*") end end + + it "creates css-modules type declarations" do + assert_file "app/javascript/types/css-modules.d.ts" + end
60-70: Also verify tsconfig.json under -T aliasConfirms the alias triggers the full TS setup.
context "with -T" do before(:all) { run_generator_test_with_args(%w[-T], package_json: true) } include_examples "base_generator_common", application_js: true include_examples "no_redux_generator" it "creates TypeScript component files with .tsx extension" do assert_file "app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx" assert_file "app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx" end + + it "creates tsconfig.json file" do + assert_file "tsconfig.json" + end end
72-119: Increase assertions: TS deps present and no @types/react-reduxCovers the dependency install side and guards against adding deprecated typings.
context "with --redux --typescript" do before(:all) { run_generator_test_with_args(%w[--redux --typescript], package_json: true) } @@ it "TypeScript Redux App includes proper typing" do assert_file "app/javascript/src/HelloWorldApp/ror_components/HelloWorldApp.client.tsx" do |content| expect(content).to match(/interface HelloWorldAppProps/) expect(content).to match(/React\.FC<HelloWorldAppProps>/) end end + + it "adds TS dependencies (and omits @types/react-redux)" do + assert_file "package.json" do |content| + pkg = JSON.parse(content) + dev = pkg.fetch("devDependencies", {}) + expect(dev).to include("typescript", "@types/react", "@types/react-dom", "@babel/preset-typescript") + expect(dev).not_to include("@types/react-redux") + end + end endlib/generators/react_on_rails/react_with_redux_generator.rb (3)
42-46: Remove no‑op gsubsBoth source and target strings are identical; these calls do nothing.
- ror_client_file = "app/javascript/src/HelloWorldApp/ror_components/HelloWorldApp.client.#{extension}" - gsub_file(ror_client_file, "../store/helloWorldStore", "../store/helloWorldStore") - gsub_file(ror_client_file, "../containers/HelloWorldContainer", - "../containers/HelloWorldContainer") + # No import path rewrites needed for auto-registration structure
95-112: Prefer Thor’s run over system and surface failure outputrun(...) integrates with Rails generators and preserves output; keep the warning as-is.
- def install_packages_with_fallback(packages, dev:, package_manager:) + def install_packages_with_fallback(packages, dev:, package_manager:) packages_str = packages.join(" ") install_command = build_install_command(package_manager, dev, packages_str) - success = system(install_command) + success = run(install_command, verbose: true) return if success @@ - GeneratorMessages.add_warning(warning) + GeneratorMessages.add_warning(warning) end
113-129: Minor: unify fallback into GeneratorHelper to avoid duplicationConsider moving build_install_command/install_packages_with_fallback into GeneratorHelper so both Redux and future generators share one code path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
CLAUDE.md(1 hunks)eslint.config.ts(1 hunks)lib/generators/react_on_rails/install_generator.rb(4 hunks)lib/generators/react_on_rails/react_no_redux_generator.rb(1 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(3 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/constants/helloWorldConstants.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts(1 hunks)package.json(2 hunks)spec/react_on_rails/generators/install_generator_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (11)
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx
- package.json
- lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.ts
- lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts
- lib/generators/react_on_rails/install_generator.rb
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/constants/helloWorldConstants.ts
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
spec/react_on_rails/generators/install_generator_spec.rblib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/react_no_redux_generator.rb
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
eslint.config.tslib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/react_no_redux_generator.rb
🧠 Learnings (4)
📚 Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to {coverage,tmp,gen-examples,node_package/lib,node_modules,spec/dummy/node_modules,spec/dummy/tmp,spec/dummy/app/assets/webpack,spec/dummy/log}/** : Exclude generated/build/vendor directories from IDE indexing to avoid slowdowns
Applied to files:
eslint.config.ts
📚 Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use ESLint for all JavaScript/TypeScript code
Applied to files:
eslint.config.ts
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rb
🧬 Code graph analysis (4)
spec/react_on_rails/generators/install_generator_spec.rb (5)
spec/react_on_rails/support/generator_spec_helper.rb (1)
run_generator_test_with_args(62-72)lib/generators/react_on_rails/install_generator.rb (1)
include(11-408)lib/generators/react_on_rails/react_with_redux_generator.rb (1)
include(8-139)lib/generators/react_on_rails/react_no_redux_generator.rb (1)
include(8-45)lib/generators/react_on_rails/base_generator.rb (1)
include(9-387)
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
add_npm_dependencies(23-39)lib/generators/react_on_rails/generator_messages.rb (1)
detect_package_manager(183-193)
lib/generators/react_on_rails/react_no_redux_generator.rb (2)
lib/generators/react_on_rails/react_with_redux_generator.rb (1)
copy_base_files(29-46)lib/generators/react_on_rails/base_generator.rb (1)
copy_base_files(34-46)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts (2)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts (1)
HelloWorldState(6-8)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.js (1)
configureStore(4-4)
⏰ 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). (8)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: claude-review
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
🔇 Additional comments (3)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts (1)
12-13: Store typing is clear and correct.Alias reads well and matches reducer state.
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
14-19: Good: TS option with -T aliasMatches the CLI contract and other generators.
Please ensure all generators expose the same alias; I flagged the no‑redux generator separately.
79-93: GeneratorMessages defines detect_package_manager — require it only if not already loadedlib/generators/react_on_rails/generator_messages.rb defines GeneratorMessages and def detect_package_manager; add the require to lib/generators/react_on_rails/react_with_redux_generator.rb if that file doesn't already load it.
-require_relative "generator_helper" +require_relative "generator_helper" +require_relative "generator_messages"
Code Review for TypeScript Support PR #1786Thank you for this comprehensive TypeScript implementation! I've reviewed the changes and have the following feedback: ✅ Strengths
🔍 Areas for Improvement1. Error Handling & User Experience
2. TypeScript Configuration
3. Redux TypeScript Support
4. Missing CSS Module Types
🚨 Potential Issues
🔒 Security Considerations
💡 Suggestions for Enhancement
📊 Performance Considerations
Overall AssessmentThis is a solid implementation that thoughtfully adds TypeScript support while maintaining backward compatibility. The main areas for improvement are around Redux TypeScript patterns and handling of edge cases. Recommendation: Approve with minor changes suggested above, particularly the CSS modules type declaration and Redux typing improvements. Great work on this feature! 🎉 |
Code Review for TypeScript Support PRThank you for this comprehensive implementation of TypeScript support! This is a significant enhancement that will benefit many developers. Strengths1. Excellent Implementation Quality
2. Redux TypeScript Integration
3. Developer Experience
4. Test Coverage
Suggestions for Improvement1. Type Safety EnhancementConsider making the Rails props interface more strict in helloWorldStore.ts. Instead of using [key: string]: any, consider adding specific optional props. 2. tsconfig.json ImprovementsConsider adding compiler options: noUnusedLocals, noUnusedParameters, noImplicitReturns for better type safety. 3. Error Handling in Dependency InstallationConsider checking if npm is available first before attempting the fallback in install_typescript_dependencies method. 4. Documentation SuggestionConsider adding JSDoc comments to the TypeScript templates to help new users understand the patterns. Minor Issues
Security Review
Performance Considerations
Overall AssessmentThis is a high-quality implementation that maintains backward compatibility while adding powerful TypeScript support. The code is clean, well-tested, and follows React on Rails conventions. Recommendation: APPROVED The minor suggestions above are optional improvements that could be addressed in future PRs. This implementation is ready for merge and will provide immediate value to users wanting TypeScript support. Great work on this feature! |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 22 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| id="name" | ||
| type="text" | ||
| value={name} | ||
| onChange={e => setName(e.target.value)} |
There was a problem hiding this comment.
The event handler should include proper TypeScript typing for the event parameter. Use (e: React.ChangeEvent<HTMLInputElement>) => setName(e.target.value) to match the typing shown in the test expectations.
| onChange={e => setName(e.target.value)} | |
| onChange={(e: React.ChangeEvent<HTMLInputElement>) => setName(e.target.value)} |
| id="name" | ||
| type="text" | ||
| value={name} | ||
| onChange={e => updateName(e.target.value)} |
There was a problem hiding this comment.
The event handler should include proper TypeScript typing for the event parameter. Use (e: React.ChangeEvent<HTMLInputElement>) => updateName(e.target.value) to provide explicit type safety.
| onChange={e => updateName(e.target.value)} | |
| onChange={(e: React.ChangeEvent<HTMLInputElement>) => updateName(e.target.value)} |
Code Review: TypeScript Support for React on Rails Install GeneratorStrengths:
Suggestions for Improvement:
Potential Issues:
Security Considerations:
Test Coverage:Excellent coverage including TypeScript file generation, proper extension handling, type checking, Redux + TypeScript combination, and tsconfig.json validation Overall Assessment:Strong implementation that adds valuable TypeScript support to React on Rails. Well-structured, thoroughly tested, maintains excellent backwards compatibility. Recommendation: Approve with minor suggestions Great work on bringing TypeScript support to React on Rails! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
193-195:cli_exists?fails on Windows; usewhereorcommand -v.Current implementation uses
which, which isn’t available on Windows and will cause false negatives.Apply this diff:
- def cli_exists?(command) - system("which #{command} > /dev/null 2>&1") - end + def cli_exists?(command) + if ReactOnRails::Utils.running_on_windows? + system("where #{command} > NUL 2>&1") + else + system("command -v #{command} >/dev/null 2>&1") + end + end
♻️ Duplicate comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
318-343: Fallback should respect the project’s package manager (avoid creating an unexpected package-lock.json).Current fallback hardcodes npm. Detect the PM via lockfiles and build the correct install command. This also aligns with the PR claim of auto‑detecting npm/yarn/pnpm/bun and addresses prior reviewer guidance.
Apply this diff:
# Try using GeneratorHelper first (package manager agnostic) return if add_npm_dependencies(typescript_packages, dev: true) - # Fallback to npm if GeneratorHelper fails - success = run "npm install --save-dev #{typescript_packages.join(' ')}" - return if success - - warning = <<~MSG.strip - ⚠️ Failed to install TypeScript dependencies automatically. - - Please run manually: - npm install --save-dev #{typescript_packages.join(' ')} - MSG - GeneratorMessages.add_warning(warning) + # Fallback: detect package manager by lockfile and install + pm = + if File.exist?("yarn.lock") + "yarn" + elsif File.exist?("pnpm-lock.yaml") + "pnpm" + elsif File.exist?("bun.lockb") + "bun" + else + "npm" + end + cmds = { + "npm" => "npm install --save-dev", + "yarn" => "yarn add --dev", + "pnpm" => "pnpm add -D", + "bun" => "bun add --dev" + } + install_cmd = "#{cmds[pm]} #{typescript_packages.join(' ')}" + return if run(install_cmd) + + warning = <<~MSG.strip + ⚠️ Failed to install TypeScript dependencies automatically. + + Please run manually: + #{install_cmd} + MSG + GeneratorMessages.add_warning(warning)
🧹 Nitpick comments (4)
docs/javascript/troubleshooting-build-errors.md (2)
19-23: Add a language to the fenced code block (markdownlint MD040).Specify a language to satisfy linting and improve readability.
Apply this diff:
-``` +```text Cannot read properties of undefined (reading 'module') TypeError: Cannot read properties of undefined (reading 'module') at ProvidedDependencyTemplate.apply--- `139-149`: **Add a language to the fenced code block (markdownlint MD040).** Same lint warning here; use a neutral language. Apply this diff: ```diff -``` +```text MONGODB | Error checking localhost:27017: Connection refused</blockquote></details> <details> <summary>lib/generators/react_on_rails/generator_helper.rb (1)</summary><blockquote> `95-97`: **Helper looks good; consider aligning with existing `options` pattern.** Minor consistency nit: other helpers read `options` directly; you can drop the param and call `options.typescript?` inside. Apply this diff if you prefer: ```diff -def component_extension(options) - options.typescript? ? "tsx" : "jsx" +def component_extension(options) + options.typescript? ? "tsx" : "jsx" endOr:
-def component_extension(options) - options.typescript? ? "tsx" : "jsx" +def component_extension + options.typescript? ? "tsx" : "jsx" endNote: the second variant requires updating call sites.
lib/generators/react_on_rails/install_generator.rb (1)
373-402: tsconfig.json: add paths for$app/*alias so TS resolves webpack aliases.Without this, TS can’t resolve
$app/...imports even though webpack can. Also optional: raise target to ES2020 to match Node 18+.Apply this diff:
tsconfig_content = { "compilerOptions" => { "target" => "es2018", "allowJs" => true, "skipLibCheck" => true, "strict" => true, "noUncheckedIndexedAccess" => true, "forceConsistentCasingInFileNames" => true, "noFallthroughCasesInSwitch" => true, "module" => "esnext", "moduleResolution" => "bundler", "resolveJsonModule" => true, "isolatedModules" => true, "noEmit" => true, - "jsx" => "react-jsx" + "jsx" => "react-jsx", + "baseUrl" => ".", + "paths" => { + "$app/*" => ["app/javascript/*"] + } }, "include" => [ "app/javascript/**/*" ] }Optional tweak:
- "target" => "es2018", + "target" => "es2020",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/contributor-info/coding-agents-guide.md(6 hunks)docs/guides/upgrading-react-on-rails.md(1 hunks)docs/javascript/troubleshooting-build-errors.md(7 hunks)lib/generators/react_on_rails/generator_helper.rb(1 hunks)lib/generators/react_on_rails/install_generator.rb(4 hunks)lib/generators/react_on_rails/react_no_redux_generator.rb(1 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(3 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/guides/upgrading-react-on-rails.md
- docs/contributor-info/coding-agents-guide.md
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx
- lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx
- lib/generators/react_on_rails/react_no_redux_generator.rb
- lib/generators/react_on_rails/react_with_redux_generator.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/generators/react_on_rails/generator_helper.rblib/generators/react_on_rails/install_generator.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/generator_helper.rblib/generators/react_on_rails/install_generator.rb
🧠 Learnings (3)
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
docs/javascript/troubleshooting-build-errors.md
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
docs/javascript/troubleshooting-build-errors.md
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/install_generator.rb
🧬 Code graph analysis (1)
lib/generators/react_on_rails/install_generator.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
add_npm_dependencies(23-39)lib/generators/react_on_rails/generator_messages.rb (1)
add_warning(13-15)
🪛 markdownlint-cli2 (0.17.2)
docs/javascript/troubleshooting-build-errors.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (9)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: claude-review
🔇 Additional comments (3)
docs/javascript/troubleshooting-build-errors.md (1)
65-69: Clarify alias usage for ProvidePlugin snippet (TS path mapping may be needed).If code imports via
$app/..., TypeScript will fail to resolve without tsconfig paths. Cross‑reference with tsconfig generation to add$app/*mapping.Do you plan to include
$app/*in tsconfig paths (see suggested change in install_generator.rb)?Also applies to: 76-80
lib/generators/react_on_rails/install_generator.rb (2)
69-73: Good: TS flow invoked in the right order.Ensures deps, CSS module types, and tsconfig are created before invoking sub‑generators. ✅
Also applies to: 75-79
112-131: Nice: clear Node.js version guidance with soft warning.Good UX and non‑blocking behavior. ✅
| # frozen_string_literal: true | ||
|
|
||
| require "rails/generators" | ||
| require "json" |
There was a problem hiding this comment.
Require FileUtils before using it.
create_css_module_types calls FileUtils.mkdir_p but fileutils isn’t required in this file.
Apply this diff:
require "rails/generators"
require "json"
+require "fileutils"📝 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.
| require "json" | |
| require "rails/generators" | |
| require "json" | |
| require "fileutils" |
🤖 Prompt for AI Agents
In lib/generators/react_on_rails/install_generator.rb around line 4, FileUtils
is used in create_css_module_types but not required; add require "fileutils"
(for example immediately after the existing require "json") so FileUtils.mkdir_p
calls succeed.
| puts Rainbow("📝 Creating CSS module type definitions...").yellow | ||
|
|
||
| # Ensure the types directory exists | ||
| FileUtils.mkdir_p("app/javascript/types") | ||
|
|
||
| css_module_types_content = <<~TS.strip | ||
| // TypeScript definitions for CSS modules | ||
| declare module "*.module.css" { | ||
| const classes: { [key: string]: string }; | ||
| export default classes; | ||
| } | ||
| declare module "*.module.scss" { | ||
| const classes: { [key: string]: string }; | ||
| export default classes; | ||
| } | ||
| declare module "*.module.sass" { | ||
| const classes: { [key: string]: string }; | ||
| export default classes; | ||
| } | ||
| TS | ||
|
|
||
| File.write("app/javascript/types/css-modules.d.ts", css_module_types_content) | ||
| puts Rainbow("✅ Created CSS module type definitions").green | ||
| end |
There was a problem hiding this comment.
CSS Modules d.ts mismatches namedExports=true (project convention).
Per project learning, CSS Modules use named exports with import * as style from './file.module.css'. The current typings export a default, which will mislead TS. Use export = classes to support namespace imports.
Apply this diff:
- css_module_types_content = <<~TS.strip
- // TypeScript definitions for CSS modules
- declare module "*.module.css" {
- const classes: { [key: string]: string };
- export default classes;
- }
-
- declare module "*.module.scss" {
- const classes: { [key: string]: string };
- export default classes;
- }
-
- declare module "*.module.sass" {
- const classes: { [key: string]: string };
- export default classes;
- }
- TS
+ css_module_types_content = <<~TS.strip
+ // TypeScript definitions for CSS modules (supports `import * as styles from ...`)
+ declare module "*.module.css" {
+ const classes: { readonly [key: string]: string };
+ export = classes;
+ }
+
+ declare module "*.module.scss" {
+ const classes: { readonly [key: string]: string };
+ export = classes;
+ }
+
+ declare module "*.module.sass" {
+ const classes: { readonly [key: string]: string };
+ export = classes;
+ }
+ TSNote: If you prefer strict per-file typings, consider adding a typed‑CSS‑modules generator later.
📝 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.
| puts Rainbow("📝 Creating CSS module type definitions...").yellow | |
| # Ensure the types directory exists | |
| FileUtils.mkdir_p("app/javascript/types") | |
| css_module_types_content = <<~TS.strip | |
| // TypeScript definitions for CSS modules | |
| declare module "*.module.css" { | |
| const classes: { [key: string]: string }; | |
| export default classes; | |
| } | |
| declare module "*.module.scss" { | |
| const classes: { [key: string]: string }; | |
| export default classes; | |
| } | |
| declare module "*.module.sass" { | |
| const classes: { [key: string]: string }; | |
| export default classes; | |
| } | |
| TS | |
| File.write("app/javascript/types/css-modules.d.ts", css_module_types_content) | |
| puts Rainbow("✅ Created CSS module type definitions").green | |
| end | |
| puts Rainbow("📝 Creating CSS module type definitions...").yellow | |
| # Ensure the types directory exists | |
| FileUtils.mkdir_p("app/javascript/types") | |
| css_module_types_content = <<~TS.strip | |
| // TypeScript definitions for CSS modules (supports `import * as styles from ...`) | |
| declare module "*.module.css" { | |
| const classes: { readonly [key: string]: string }; | |
| export = classes; | |
| } | |
| declare module "*.module.scss" { | |
| const classes: { readonly [key: string]: string }; | |
| export = classes; | |
| } | |
| declare module "*.module.sass" { | |
| const classes: { readonly [key: string]: string }; | |
| export = classes; | |
| } | |
| TS | |
| File.write("app/javascript/types/css-modules.d.ts", css_module_types_content) | |
| puts Rainbow("✅ Created CSS module type definitions").green | |
| end |
🤖 Prompt for AI Agents
In lib/generators/react_on_rails/install_generator.rb around lines 346 to 371,
the generated CSS module type file currently uses "export default classes" which
conflicts with the project's namedExports=true convention; update the generated
declarations to use CommonJS-style namespace exports so namespace imports work.
Replace each "export default classes;" with "export = classes;" (keep the const
classes: { [key: string]: string }; declaration) for all "*.module.css",
"*.module.scss", and "*.module.sass" blocks and write that updated content to
app/javascript/types/css-modules.d.ts.
🔍 Code Review for TypeScript Support PR #1786Great work on implementing TypeScript support for the React on Rails install generator! This is a significant enhancement that will benefit many teams adopting TypeScript. Here's my comprehensive review: ✅ Strengths
🎯 Code Quality Observations
|
This commit implements comprehensive TypeScript support for the React on Rails install generator, allowing developers to generate TypeScript-based React components with proper type safety and tooling support. ## Key Features Added ### Generator Options - Added `--typescript` or `-T` flag to the install generator - Enables TypeScript file generation and dependency installation - Maintains backward compatibility with JavaScript (default behavior) ### TypeScript Dependencies - Automatically installs required TypeScript packages: - `typescript` - TypeScript compiler - `@types/react` - React type definitions - `@types/react-dom` - React DOM type definitions - `@babel/preset-typescript` - Babel preset for TypeScript compilation - Supports all package managers: npm, yarn, pnpm, and bun ### TypeScript Templates - Created `.tsx` versions of all React component templates - Proper TypeScript interfaces for component props - Full type annotations for state and event handlers - Server and client component templates with type safety ### Configuration - Generates `tsconfig.json` with React-optimized settings - Automatic Babel configuration through Shakapacker detection - Proper JSX handling with React 18+ automatic runtime ### Testing Integration - Updated generator specs to test TypeScript functionality - Validates proper file generation and dependency installation - Ensures TypeScript compilation works correctly ## Technical Details The implementation leverages Shakapacker's automatic TypeScript preset detection. When `@babel/preset-typescript` is installed, Shakapacker automatically includes it in the Babel configuration, enabling seamless TypeScript compilation without manual Babel configuration changes. ## Usage ```bash # Generate TypeScript React on Rails app rails generate react_on_rails:install --typescript # Short form rails generate react_on_rails:install -T ``` This addresses the growing demand for TypeScript support in React on Rails applications and provides a smooth migration path for developers wanting to adopt TypeScript. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Alexey Romanov <alexey.romanov+ci5@shakacode.com>
Type-only import fixes: - Use type-only imports for HelloWorldState in HelloWorldContainer.ts - Separate value/type imports in helloWorldStore.ts with PreloadedState typing - Fix PropsFromRedux import to type-only in HelloWorld.tsx Package management fixes: - Remove unnecessary @types/react-redux dependency (react-redux now bundles types) - Add -T alias to ReactWithReduxGenerator for --typescript option - Fix TypeScript dependency installation to use GeneratorHelper properly Security improvements: - Add package manager validation to prevent command injection - Use allowlist of valid package managers (npm, yarn, pnpm, bun) Type definition enhancements: - Add CSS module type definitions for .module.css/.scss/.sass files - Create app/javascript/types/css-modules.d.ts automatically when TypeScript is enabled All changes maintain backward compatibility and improve type safety. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix RuboCop guard clause violation in install_generator.rb - Add missing create_typescript_config method call for --typescript flag - Fix indentation and syntax issues from RuboCop auto-correct - Change TypeScript Redux template to use interface instead of type alias to match test expectations All generator tests now pass and lint is clean. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Move create_css_module_types and create_typescript_config back into class - Fix indentation and formatting issues - Ensure proper class structure and method visibility 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update docs files that were updated from master branch rebase - Fix formatting in CLAUDE.md, coding-agents-guide.md, upgrading-react-on-rails.md, troubleshooting-build-errors.md - Ensure CI formatting checks pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Alexey Romanov <alexey.romanov+ci5@shakacode.com>
Co-authored-by: Alexey Romanov <alexey.romanov+ci5@shakacode.com>
Co-authored-by: Alexey Romanov <alexey.romanov+ci5@shakacode.com>
Co-authored-by: Alexey Romanov <alexey.romanov+ci5@shakacode.com>
…pt/src/HelloWorld/ror_components/HelloWorld.client.tsx Co-authored-by: Alexey Romanov <alexey.romanov+ci5@shakacode.com>
Co-authored-by: Alexey Romanov <alexey.romanov+ci5@shakacode.com>
- Remove unnecessary type annotations from TypeScript templates (useState<string>, React.ChangeEvent) - Improve tsconfig.json settings: - Change moduleResolution from 'node' to 'bundler' - Remove unnecessary lib array (use defaults) - Remove exclude array (node_modules excluded by default) - Remove esModuleInterop and allowSyntheticDefaultImports - Add noUncheckedIndexedAccess for better type safety - Add check for existing tsconfig.json before creating - Add component_extension helper method to GeneratorHelper - Update generators to use component_extension helper consistently - Improve Redux generator to use %W syntax for file arrays - Fix line length issues in generator files Addresses all unresolved suggestions from alexeyr-ci2's code review. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Redux Generator: - Move GeneratorMessages require to top of file to fix load order - Fix detect_package_manager calls to use proper module method - Remove duplicate require statement in add_redux_specific_messages React Component Improvements: - Use modern JSX transform (remove React import) - Add useMemo to prevent store recreation on re-renders - Use type-only imports for better tree-shaking - Switch from React.FC to FC import Addresses additional code review feedback for performance and correctness. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Format HelloWorld.client.tsx templates with proper line breaks - Ensure all code follows Prettier style guidelines - Fix input element formatting for consistency 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Updated test expectations to match the improved TypeScript patterns that rely on inference instead of explicit type annotations, per alexeyr-ci2's code review suggestions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
justin808-ts-generator
branch
from
7d2fd5a to
b037c6a
Compare
Pull Request Review: TypeScript Support ImplementationExecutive SummaryWell-structured TypeScript support implementation with excellent developer experience and backward compatibility. Security and error handling improvements needed before merging. ✅ Strengths
🔧 Required Improvements1. Security IssuesHIGH PRIORITY - Command Injection Risk (lib/generators/react_on_rails/install_generator.rb:342): 2. Error HandlingTypeScript dependency installation failures only show warnings. Consider making failures blocking or more prominent to prevent broken setups. 3. Type SafetyReplace 💡 Suggestions
📊 Code Quality: B+Solid engineering with good separation of concerns and modern TypeScript patterns. Main improvements needed in security hardening and error handling. 🎯 VerdictValuable feature ready to merge after addressing security fixes and error handling improvements. Excellent work on template quality and maintaining backward compatibility! 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/generators/react_on_rails/react_with_redux_generator.rb (1)
128-134: Don’t clear GeneratorMessages.output; preserves earlier warnings.Calling output.clear removes warnings (e.g., failed installs and manual command). Append the Redux message without clearing.
- GeneratorMessages.output.clear GeneratorMessages.add_info( GeneratorMessages.helpful_message_after_installation(component_name: "HelloWorldApp", route: "hello_world") )
🧹 Nitpick comments (5)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx (2)
1-5: Optional: re-export typed props for DXExpose the component props type from the server entry for better editor hints without coupling to a named type export.
import HelloWorld from './HelloWorld.client'; +import type { ComponentProps } from 'react'; // This could be specialized for server rendering // For example, if using React Router, we'd have the SSR setup here. +export type HelloWorldProps = ComponentProps<typeof HelloWorld>; export default HelloWorld;
2-4: Optional: include a commented SSR wrapper exampleA brief, commented example helps users adapt this file for common SSR setups (e.g., React Router) without changing behavior.
// This could be specialized for server rendering // For example, if using React Router, we'd have the SSR setup here. +// Example: +// import { StaticRouter } from 'react-router-dom/server'; +// const HelloWorldServer = (props: HelloWorldProps) => ( +// <StaticRouter location={props.location}> +// <HelloWorld {...props} /> +// </StaticRouter> +// ); +// export default HelloWorldServer;lib/generators/react_on_rails/react_with_redux_generator.rb (3)
43-47: Remove no‑op gsubs.Both gsub_file calls replace the same string with itself; they don’t change the file and add noise. Drop these lines.
- ror_client_file = "app/javascript/src/HelloWorldApp/ror_components/HelloWorldApp.client.#{ext}" - gsub_file(ror_client_file, "../store/helloWorldStore", "../store/helloWorldStore") - gsub_file(ror_client_file, "../containers/HelloWorldContainer", - "../containers/HelloWorldContainer") + # imports inside HelloWorldApp.client already use extensionless paths that are correct
76-90: PM‑agnostic install path: good; add bun detection to the detector.Fallback is solid, but GeneratorMessages.detect_package_manager (in the other file) doesn’t currently detect bun.lockb, so bun will never be selected here.
# lib/generators/react_on_rails/generator_messages.rb - if File.exist?("yarn.lock") + if File.exist?("yarn.lock") "yarn" elsif File.exist?("pnpm-lock.yaml") "pnpm" + elsif File.exist?("bun.lockb") + "bun" else "npm" end
94-109: Prefer Thor/Rails generator run() for pretend mode and consistent logging.system(...) bypasses Thor’s pretend/dry‑run and status output. Consider using run(install_command) and checking its return to keep generator UX consistent. If you keep system, at least say_status(:run, install_command) before executing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
eslint.config.ts(1 hunks)lib/generators/react_on_rails/generator_helper.rb(1 hunks)lib/generators/react_on_rails/install_generator.rb(4 hunks)lib/generators/react_on_rails/react_no_redux_generator.rb(1 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(3 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/constants/helloWorldConstants.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts(1 hunks)package.json(2 hunks)spec/react_on_rails/generators/install_generator_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/store/helloWorldStore.ts
- package.json
- lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.ts
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.client.tsx
- spec/react_on_rails/generators/install_generator_spec.rb
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.tsx
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.ts
- lib/generators/react_on_rails/generator_helper.rb
- eslint.config.ts
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.ts
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.tsx
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/constants/helloWorldConstants.ts
- lib/generators/react_on_rails/react_no_redux_generator.rb
- lib/generators/react_on_rails/install_generator.rb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/generators/react_on_rails/react_with_redux_generator.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/react_with_redux_generator.rb
🧠 Learnings (2)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rb
🧬 Code graph analysis (1)
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
lib/generators/react_on_rails/generator_helper.rb (2)
component_extension(95-97)add_npm_dependencies(23-39)lib/generators/react_on_rails/generator_messages.rb (2)
detect_package_manager(183-193)add_warning(13-15)
⏰ 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). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: build
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: claude-review
🔇 Additional comments (8)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.tsx (2)
1-5: LGTM: simple server mirror is fineRe-exporting the client component for SSR is acceptable for the base template.
1-1: Default export present for HelloWorld.clientBoth templates contain
export default HelloWorld;: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx (line 21) and lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.tsx (line 25).lib/generators/react_on_rails/react_with_redux_generator.rb (6)
4-5: Top‑level requires: good load‑order fix.Loading GeneratorHelper and GeneratorMessages up front avoids runtime constant/method resolution issues.
10-10: Including GeneratorHelper is correct.Enables component_extension and package helpers in this generator.
15-20: -T alias added as promised.Matches PR description; ensure docs/README usage examples mention -T.
32-38: Extension‑aware template copies: LGTM.Using component_extension(options) to drive .tsx/.jsx selection is clean and consistent.
51-59: Verify TS/JS template extensions for Redux files. Automated search returned no matching TypeScript templates under lib/generators/react_on_rails/templates; cannot confirm if HelloWorldContainer contains JSX. If the container template includes JSX, emit .tsx instead of .ts; otherwise .ts is fine. Check lib/generators/react_on_rails/templates/**/HelloWorldContainer.ts* and adjust accordingly.
110-126: Dev flags compatible — keep long--devform.
Yarn (v1–v4) and Bun accept both short (-D/-d) and long (--dev); current entries already use--devand are version-compatible.
Replace unsafe string interpolation with array-based system calls to prevent command injection attacks in npm/yarn/pnpm/bun commands. Security fixes: - install_generator.rb: Fix TypeScript package installation - base_generator.rb: Fix all package manager install commands - react_with_redux_generator.rb: Refactor to use safe array-based calls Risk: HIGH - Command injection could allow arbitrary code execution on developer machines during generator usage. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/generators/react_on_rails/base_generator.rb (2)
105-118: Add Bun support in install_js_dependencies detection.The PR promises npm/yarn/pnpm/bun detection. This method doesn’t handle Bun lockfiles.
Apply this diff:
success = if File.exist?(File.join(destination_root, "yarn.lock")) - system("yarn", "install") + system("yarn", "install") elsif File.exist?(File.join(destination_root, "pnpm-lock.yaml")) - system("pnpm", "install") + system("pnpm", "install") + elsif File.exist?(File.join(destination_root, "bun.lockb")) + system("bun", "install") elsif File.exist?(File.join(destination_root, "package-lock.json")) || File.exist?(File.join(destination_root, "package.json")) # Use npm for package-lock.json or as default fallback - system("npm", "install") + system("npm", "install") else true # No package manager detected, skip end
180-195: Move Babel tooling to devDependencies (avoid bloating production deps).
@babel/preset-react,babel-plugin-transform-react-remove-prop-types, andbabel-plugin-macrosshould be devDeps. Keeping them in deps inflates lockfiles and prod installs.Apply these diffs:
react_deps = %w[ react react-dom - @babel/preset-react prop-types - babel-plugin-transform-react-remove-prop-types - babel-plugin-macros ]dev_deps = %w[ @pmmmwh/react-refresh-webpack-plugin react-refresh + @babel/preset-react + babel-plugin-transform-react-remove-prop-types + babel-plugin-macros ]Also applies to: 210-221
♻️ Duplicate comments (4)
lib/generators/react_on_rails/base_generator.rb (1)
172-179: Avoid hardcoded npm fallbacks; use PM-aware fallback consistently.These blocks fall back to npm only. Prefer a PM-aware fallback (yarn/pnpm/bun) for symmetry with detection and to prevent lockfile churn.
Apply these diffs:
- puts "Using direct npm commands as fallback" - success = system("npm", "install", *react_on_rails_pkg) + puts "Using direct package-manager commands as fallback" + success = install_with_pm(react_on_rails_pkg) handle_npm_failure("react-on-rails package", react_on_rails_pkg) unless success- success = system("npm", "install", *react_deps) + success = install_with_pm(react_deps) handle_npm_failure("React dependencies", react_deps) unless success- success = system("npm", "install", *css_deps) + success = install_with_pm(css_deps) handle_npm_failure("CSS dependencies", css_deps) unless success- success = system("npm", "install", "--save-dev", *dev_deps) + success = install_with_pm(dev_deps, dev: true) handle_npm_failure("development dependencies", dev_deps, dev: true) unless successAdd this helper (outside the shown ranges; place under
private):def install_with_pm(packages, dev: false) pm = if File.exist?(File.join(destination_root, "yarn.lock")) "yarn" elsif File.exist?(File.join(destination_root, "pnpm-lock.yaml")) "pnpm" elsif File.exist?(File.join(destination_root, "bun.lockb")) "bun" else "npm" end case pm when "yarn" then system("yarn", "add", ("--dev" if dev), *packages.compact) when "pnpm" then system("pnpm", "add", ("--save-dev" if dev), *packages.compact) when "bun" then system("bun", "add", ("--dev" if dev), *packages.compact) else system("npm", "install", ("--save-dev" if dev), *packages.compact) end endAlso applies to: 180-195, 196-209, 210-221
lib/generators/react_on_rails/install_generator.rb (3)
327-352: Package-manager-aware fallback for TypeScript deps (including Bun).Avoid hardcoding npm; detect yarn/pnpm/bun/npm and pass the correct dev flag. Keeps lockfiles consistent with the chosen PM.
Apply this diff:
- # Fallback to npm if GeneratorHelper fails - success = system("npm", "install", "--save-dev", *typescript_packages) - return if success - - warning = <<~MSG.strip - ⚠️ Failed to install TypeScript dependencies automatically. - - Please run manually: - npm install --save-dev #{typescript_packages.join(' ')} - MSG - GeneratorMessages.add_warning(warning) + # Fallback: detect package manager and install with proper dev flag + pm = + if File.exist?("yarn.lock") + "yarn" + elsif File.exist?("pnpm-lock.yaml") + "pnpm" + elsif File.exist?("bun.lockb") + "bun" + else + "npm" + end + args = case pm + when "yarn" then ["yarn", "add", "--dev", *typescript_packages] + when "pnpm" then ["pnpm", "add", "--save-dev", *typescript_packages] + when "bun" then ["bun", "add", "--dev", *typescript_packages] + else ["npm", "install", "--save-dev", *typescript_packages] + end + return if system(*args) + + GeneratorMessages.add_warning(<<~MSG.strip) + ⚠️ Failed to install TypeScript dependencies automatically. + + Please run manually: + #{args.join(' ')} + MSG
4-4: Require FileUtils before use.
create_css_module_typescalls FileUtils.mkdir_p but FileUtils isn’t required here. This will crash when--typescriptis used.Apply this diff:
require "rails/generators" require "json" +require "fileutils"
360-376: Align CSS Modules d.ts with namedExports=true (use export = classes).Project convention uses namespace imports (
import * as styles ...). Default-export typings will mislead TS and break autocomplete.Apply this diff:
- // TypeScript definitions for CSS modules + // TypeScript definitions for CSS modules (supports `import * as styles from ...`) declare module "*.module.css" { - const classes: { [key: string]: string }; - export default classes; + const classes: { readonly [key: string]: string }; + export = classes; } declare module "*.module.scss" { - const classes: { [key: string]: string }; - export default classes; + const classes: { readonly [key: string]: string }; + export = classes; } declare module "*.module.sass" { - const classes: { [key: string]: string }; - export default classes; + const classes: { readonly [key: string]: string }; + export = classes; }
🧹 Nitpick comments (2)
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
43-47: Remove no-op gsub_file calls.Both replacements replace the same string with itself; they have no effect and add noise.
Apply this diff:
- ror_client_file = "app/javascript/src/HelloWorldApp/ror_components/HelloWorldApp.client.#{ext}" - gsub_file(ror_client_file, "../store/helloWorldStore", "../store/helloWorldStore") - gsub_file(ror_client_file, "../containers/HelloWorldContainer", - "../containers/HelloWorldContainer") + # No import path rewrites needed; templates already use correct paths.
76-90: PM-aware fallback looks good; ensure Bun is detected upstream.This flow is correct, but GeneratorMessages.detect_package_manager (upstream) doesn’t currently detect Bun. Either extend detection to check bun.lockb or fall back locally.
Minimal local tweak (if you can’t update the detector now):
- package_manager = GeneratorMessages.detect_package_manager + package_manager = GeneratorMessages.detect_package_manager + package_manager = "bun" if File.exist?("bun.lockb")Prefer fixing detect_package_manager to include Bun so all generators benefit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/generators/react_on_rails/base_generator.rb(5 hunks)lib/generators/react_on_rails/install_generator.rb(4 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/base_generator.rblib/generators/react_on_rails/install_generator.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/base_generator.rblib/generators/react_on_rails/install_generator.rb
🧠 Learnings (3)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
lib/generators/react_on_rails/base_generator.rblib/generators/react_on_rails/install_generator.rb
🧬 Code graph analysis (2)
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
lib/generators/react_on_rails/generator_helper.rb (2)
component_extension(95-97)add_npm_dependencies(23-39)lib/generators/react_on_rails/generator_messages.rb (2)
detect_package_manager(183-193)add_warning(13-15)
lib/generators/react_on_rails/install_generator.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
add_npm_dependencies(23-39)lib/generators/react_on_rails/generator_messages.rb (1)
add_warning(13-15)
⏰ 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). (10)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: claude-review
- GitHub Check: build
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
🔇 Additional comments (2)
lib/generators/react_on_rails/install_generator.rb (1)
388-411: tsconfig.json looks solid; minor nits OK to keep.Good defaults (react-jsx, isolatedModules, bundler resolution). Nice adds: strict and noUncheckedIndexedAccess.
If you want, I can add “lib” to rely on TS defaults instead of explicit target. Otherwise, ship it.
lib/generators/react_on_rails/react_with_redux_generator.rb (1)
110-134: Nice: safe argv usage and injection-resistant command construction.Validation against an allowlist and passing args to system as an array is the right call.
- Add DEFAULT_ROUTE variable in generated bin/dev template - Automatically append --route hello_world when no route specified - Remove confusing fallback code from generator template - Keep fallback in spec/dummy/bin/dev with clear comments for gem development - Improve developer experience: ./bin/dev now goes directly to generated component 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/generators/react_on_rails/templates/base/base/bin/dev (2)
32-34: Fix route flag check and RuboCop offense (Style/IfUnlessModifier).Current check can falsely match flags like
--routerand misses-r. Also fails RuboCop’s modifier style. Use a stricter match and one‑liner.-unless argv_with_defaults.any? { |arg| arg.start_with?("--route") } - argv_with_defaults.push("--route", DEFAULT_ROUTE) -end +argv_with_defaults.push("--route", DEFAULT_ROUTE) unless argv_with_defaults.any? { |arg| arg == "-r" || arg.match?(/\A--route(?:=|\z)/) }
21-23: Add a friendly fallback if react_on_rails/dev isn’t installed yet.Improves DX when users run
bin/devbeforebundle install.require "bundler/setup" -require "react_on_rails/dev" +begin + require "react_on_rails/dev" +rescue LoadError => e + warn "react_on_rails/dev not found. Run `bundle install`. (#{e.message})" + exit 1 +end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/generators/react_on_rails/templates/base/base/bin/dev(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/templates/base/base/bin/dev
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/templates/base/base/bin/dev
🪛 GitHub Actions: Lint JS and Ruby
lib/generators/react_on_rails/templates/base/base/bin/dev
[error] 32-32: RuboCop: Style/IfUnlessModifier: Favor modifier unless usage for a single-line body. Another good alternative is the usage of control flow &&/||.
⏰ 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). (9)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: claude-review
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
🔇 Additional comments (2)
lib/generators/react_on_rails/templates/base/base/bin/dev (2)
36-36: LGTM: argument pass‑through via ServerManager.
24-27: LGTM — default route "hello_world" verified across generator templates.
rg output confirms occurrences in generators (install_generator/base_generator), controllers, views, and HelloWorld JS bundles.
- Convert multi-line unless to modifier form - Add required trailing newline - All linting now passes clean 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Updated test expectation to work with the new bin/dev behavior that adds --route hello_world by default. Simplified the expectation to avoid RSpec version compatibility issues. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Document comprehensive improvements since 16.0.0 including: - Modern TypeScript patterns with better type inference - Security fixes for command injection vulnerabilities - Enhanced developer experience with smart bin/dev defaults - Optimized tsconfig.json and Redux integration 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Follow project format by including PR #1786 link and justin808 attribution for TypeScript generator improvements. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
This PR implements comprehensive TypeScript support for the React on Rails install generator, enabling developers to generate TypeScript-based React components with full type safety and modern tooling support.
Key Features
🎯 Generator Options
--typescriptor-Tflag for the install generator📦 Automatic Dependency Management
typescript- TypeScript compiler@types/react- React type definitions@types/react-dom- React DOM type definitions@babel/preset-typescript- Babel preset for TypeScript compilation🔧 TypeScript Templates
.tsxtemplate suite for all React components⚙️ Configuration
tsconfig.jsonwith React 18+ settingsTechnical Implementation
The implementation leverages Shakapacker's automatic TypeScript preset detection. When
@babel/preset-typescriptis installed, Shakapacker automatically includes it in the Babel configuration, enabling seamless TypeScript compilation without manual configuration.Usage Examples
Generated TypeScript Files
Component with Props Interface
Type-Safe Event Handlers
Breaking Changes
None - This is a purely additive feature that maintains full backward compatibility.
Testing
Generator Tests
Manual Testing
Addresses
This addresses the growing demand for TypeScript support in React on Rails applications and provides a smooth adoption path for teams wanting to migrate to TypeScript.
Dependencies
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
This change is
Summary by CodeRabbit
New Features
Tests
Chores