JSON.pretty_generate reformats the entire package.json. This will normalize all whitespace, ordering, and trailing commas, creating a noisy diff and potentially losing any non-standard formatting the user intentionally has. The same issue applies to resolve_browserslist_conflict_after_shakapacker_install on line 669.

A safer approach would be to do a targeted string-level insertion (e.g. injecting "packageManager": "..." with a regex or a minimal JSON patch) so that only the required change appears in git diff, rather than a full reformat.

This regex has a meaningful false-positive risk. The pattern <\s*[A-Za-z][\w:-]*(\s|>|/) will match:

• HTML strings in test fixtures: assert_match "<div class=...", response.body
• HTML in comments: # returns <Component/>
• String templates: fetch("/<div>")
• Comparison operators that happen to align: x < abc > 0 (only if no space before <)

A false positive will silently switch the project from SWC (fast) to Babel (slower) without the user asking for it, which could be surprising in non-migration runs.

Consider restricting the scan to known JSX entry-point files (e.g. files that also contain import React or from 'react') to reduce false positives, or at minimum gate this only during initial migration generator invocations where --from-react-rails is set.

The return values of add_packages and install_js_dependencies are not checked here. If add_packages(["babel-loader"], dev: true) fails (returns false), install_js_dependencies is still called, and both failures are silently swallowed. At a minimum, it's worth surfacing a warning to the user when either step fails, so they know to manually add babel-loader.

dev_flag_for returns nil when the case doesn't match. While fallback_package_manager currently guarantees only %w[npm yarn pnpm bun], the else-less case means any future caller that passes an unguarded value would silently append nil to the install_args array passed to system(), causing a TypeError at runtime. Add an explicit else branch that raises or returns a sensible default:

else "--save-dev" # safe fallback for unknown managers

All StandardErrors are silently swallowed here with no log output. This makes it very hard to diagnose why fallback installs fail. Even a simple GeneratorMessages.add_warning("⚠️ Fallback install failed: #{e.message}") before returning false would help users debug issues like Errno::ENOENT (command not found) or permission errors.

Two separate package-manager detection mechanisms are now in play: detect_package_manager_from_lockfiles here (lockfile-based) and GeneratorMessages.detect_package_manager used by fallback_package_manager in js_dependency_manager.rb. These could return different values for the same project (e.g. the lockfile says yarn but GeneratorMessages.detect_package_manager returns npm if it uses a different heuristic). This could result in seed_package_manager_in_package_json_from_lockfile! writing [email protected] but the fallback installer then calling npm install.

Consider unifying both call sites to use the same detection method.

Add @babel/preset-react when switching SWC projects to Babel

This branch switches javascript_transpiler from SWC to Babel for JSX-in-.js, but it only adds babel-loader. In SWC-default installs, @babel/preset-react is not added on this path, yet the generated babel.config.js still references it, so compilation fails with a missing preset unless the app already had it installed. Please add the preset dependency when this compatibility switch runs.

Useful? React with 👍 / 👎.

Preserve versioned installs in fallback package filtering

The fallback installer treats a package as satisfied by name only, so versioned specs are dropped when a package already exists. In legacy setups where package_json is unavailable, requests like react-on-rails@<gem_version> or react@~19.0.4 are skipped if any react-on-rails/react entry exists, leaving stale versions in package.json and potentially mismatching required runtime versions.

Useful? React with 👍 / 👎.

@babel/preset-react not added when switching from SWC to Babel

ensure_jsx_in_js_compatibility switches the transpiler to Babel and adds babel-loader, but does not add @babel/preset-react (the BABEL_REACT_DEPENDENCIES constant). babel-loader is only the Webpack loader that invokes Babel; processing JSX also requires @babel/preset-react (or an equivalent preset) in the Babel config.

In the primary migration scenario (from react-rails) this is typically safe — the user already has @babel/preset-react in package.json from their existing Babel setup. However, an app that arrived at SWC + JSX-in-.js via another path (e.g. upgrading Shakapacker to 9.3+ on a project that already had .js JSX files) would get a broken transpiler setup after this generator runs.

Consider calling add_babel_react_dependencies alongside the babel-loader install so the method is self-contained:

def ensure_jsx_in_js_compatibility
  return if options[:pretend]
  return unless using_swc?
  return unless jsx_in_js_files_present?

  say "⚙️  Detected JSX in .js files; switching shakapacker javascript_transpiler to babel for compatibility",
      :yellow
  set_javascript_transpiler_to_babel
  add_packages(["babel-loader"], dev: true)
  add_babel_react_dependencies   # also adds @babel/preset-react
  install_js_dependencies
end

Stale @using_swc cache leaves unused SWC packages after Babel switch

using_swc? in GeneratorHelper is memoised into @using_swc. By the time ensure_jsx_in_js_compatibility is reached, setup_react_dependencies → setup_js_dependencies → add_transpiler_dependencies has already been called, which:

1. Evaluated using_swc? → true (memoised)
2. Added @swc/core and swc-loader to package.json
3. Skipped add_babel_react_dependencies

When ensure_jsx_in_js_compatibility subsequently changes shakapacker.yml to babel, @using_swc is still true. The SWC packages already added to package.json remain there, unused by Babel. Downstream code in the same generator instance that calls using_swc? would also get the stale value.

The fix is to invalidate the cache after the transpiler switch:

def set_javascript_transpiler_to_babel
  shakapacker_config_path = "config/shakapacker.yml"
  return unless File.exist?(shakapacker_config_path)

  gsub_file(
    shakapacker_config_path,
    /^(\s*javascript_transpiler:\s*)["']?swc["']?(\s*(?:#.*)?)$/,
    '\1"babel"\2'
  )

  # Invalidate the cached value so subsequent using_swc? calls reflect the change
  @using_swc = false
end

dev_flag_for missing else branch — returns nil silently

The case statement has no else clause, so Ruby returns nil for any package manager not explicitly listed. If that happens, build_install_args appends nil to the args array (e.g., ["npm", "install", nil]), which would cause Kernel#system to misbehave.

fallback_package_manager already constrains the value to the four known managers, so this cannot fire today. Still, an explicit guard prevents silent breakage if the allowed list ever changes:

⚠️ Potential issue | 🟠 Major

Don't treat versioned specs as already satisfied by name alone.

This filter drops the version/range before checking package.json, so react-on-rails@..., Pro package pins, and the RSC react@~19.0.4 pins are all skipped as soon as the bare package name exists. On legacy apps the fallback path can therefore return success while leaving an incompatible version in place.

 def filter_missing_packages(packages)
   existing = existing_package_names
   return packages if existing.empty?

   packages.reject do |package_spec|
     package_name = package_name_from_spec(package_spec)
-    package_name && existing.include?(package_name)
+    package_name == package_spec && existing.include?(package_name)
   end
 end

Please add a regression around a versioned spec such as react-on-rails@... or react@~19.0.4 so this does not silently regress.

Verify each finding against the current code and only fix it if needed.

In `@react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb` around
lines 545 - 552, The current filter_missing_packages method incorrectly treats
versioned specs like "[email protected]" or "react@~19.0.4" as satisfied if
the package name exists; update filter_missing_packages to detect when
package_spec includes a version/range (use package_name_from_spec to get the
name and check package_spec for a version token or regex) and, in that case,
compare the spec's version/range against the installed version (look up
installed versions via existing_package_names/extend or an
existing_package_versions helper); only reject the spec if the installed version
satisfies the spec range, otherwise keep it so the installer will add/upgrade
the package.

Fragile return-value check may suppress the fallback path

pj.manager.add(...) returns the parsed package.json Hash on success (always truthy/non-nil for a real write). But the check !result.nil? would return true even if the gem returned false for some reason—hiding a real failure and skipping the fallback install.

The previous code simply fell through to true after a successful call. A clearer approach:

Or, since the gem either succeeds (returns something non-nil/non-false) or raises, the original true pattern was safer. At minimum the intent should be result (truthy check) rather than !result.nil?.

JSX detection regex has significant false-positive risk

The pattern %r{<\s*[A-Za-z][\w:-]*(\s|>|/)} will match things that aren't JSX:

• TypeScript-style generic annotations in .js: new Array<Item>() matches as <Item>
• Comparison operators: if (x < ErrorHandler ...) — the space after the identifier satisfies \s
• HTML in string literals or comments: // returns <div>
• Multi-word <= won't match, but < SomeVar will

A false positive silently switches the entire project from SWC to Babel—a meaningful, hard-to-reverse build change. Consider:

1. Narrowing the heuristic by also requiring the match to be outside comments/strings (hard in regex alone).
2. Requiring the pattern to appear at the beginning of an expression (e.g., preceded by return, (, =, ,, or newline) to reduce noise.
3. Asking for user confirmation before switching transpilers, since this is a consequential irreversible change.

At minimum the current heuristic should be documented so future maintainers understand why false positives can occur.

JSON.pretty_generate silently reformats the entire package.json

JSON.pretty_generate always uses 2-space indentation. Many projects (particularly older ones being migrated) use 4-space indent, tabs, or have formatter-enforced styles. This will silently reformat package.json everywhere—creating a large noisy diff of unrelated whitespace changes in the user's repo.

The same issue occurs at line 676 in resolve_browserslist_conflict_after_shakapacker_install.

Consider detecting and preserving the original indentation before re-serialising:

original = File.read("package.json")
indent_str = original.match(/\n(\s+)/)&.[](1) || "  "
File.write("package.json", "#{JSON.generate(package_json_content, indent: indent_str)}\n")

Or use a narrowly-targeted string substitution that only adds/removes the one key rather than re-serialising the whole file.

system() can return nil (not false) when the command isn't found

Kernel#system returns nil when the command cannot be executed (e.g., the package manager binary isn't on PATH). nil is falsy, so callers treat it as failure—which is correct—but no warning is generated for this case since rescue StandardError won't catch it.

Consider normalising the return value and emitting a warning:

Return false when package_json add reports failure

add_npm_dependencies now returns !result.nil?, which treats false as success. In the failure mode where pj.manager.add returns false, callers will skip fallback installation and continue as if dependencies were added, leaving required packages missing for subsequent build/generator steps. This should return the real success boolean (not non-nil) so fallback logic is actually triggered.

Useful? React with 👍 / 👎.

Verify detected fallback manager is installed

The fallback path chooses a package manager purely from lockfiles/env and only checks whether the name is supported, not whether the executable exists. In legacy installs (where package_json is unavailable), a stale lockfile can select yarn/pnpm even when only npm is installed, causing all fallback system(...) installs to fail even though a working manager is present. Add a CLI availability check and fall back to an installed manager.

Useful? React with 👍 / 👎.

Narrow JSX detection to actual JSX syntax

This pattern also matches normal comparison code such as a < b && ..., because it accepts < followed by an identifier and whitespace. That can incorrectly trigger the SWC→Babel switch in projects that do not use JSX in .js files, unexpectedly changing transpiler behavior and dependency installation. The detector should only match JSX-specific tokens like opening/closing tags or fragments.

Useful? React with 👍 / 👎.

Bug: system() can return nil, which silently propagates as a falsy non-false value.

Kernel#system returns true, false, or nil (when the command cannot be executed — e.g. the binary is missing or a permissions error). Returning nil here means no warning is shown to the user about what went wrong, unlike the false case which shows a clear message in install_js_dependencies.

Or explicitly handle nil:

case system(*install_args)
when true then true
when false
  GeneratorMessages.add_warning("⚠️  Fallback install command failed (non-zero exit).")
  false
else # nil
  GeneratorMessages.add_warning("⚠️  Could not execute #{install_args.first} — is it on PATH?")
  false
end

JSON.pretty_generate will reformat the entire package.json, creating noisy git diffs.

JSON.pretty_generate normalizes indentation (2 spaces) and key ordering, so even a file that previously used 4-space indentation, tabs, or different key order will be rewritten in full. This will produce a large diff in the user's working directory that has nothing to do with adding packageManager.

Consider a targeted string injection (e.g. inserting a line after "private": true) or using a JSON serializer that preserves original formatting. At a minimum, document the side effect in a comment so users aren't surprised by the reformatting:

# NOTE: JSON.pretty_generate reformats the entire file; this is intentional to
# ensure a canonical format before shakapacker:install.
File.write("package.json", "#{JSON.pretty_generate(package_json_content)}\n")

The same issue exists at line 675 in resolve_browserslist_conflict_after_shakapacker_install.

JSX detection regex has false positive risk that could unnecessarily switch projects from SWC to Babel.

The pattern <\s*[A-Za-z][\w:-]*(\s|>|/) matches:

• Regular HTML strings in template literals or JS strings: `<div class="foo">`
• HTML comments in JS: // <Component example>
• Comparison operators mixed with capitalized identifiers in some edge cases

A developer with SWC set up, .js files containing HTML in strings or comments, but no actual JSX syntax would have their transpiler silently downgraded. This could mask build issues rather than surfacing them.

Consider making this opt-in via a generator flag (e.g., --auto-detect-jsx) rather than running it unconditionally, or at minimum logging what file and what match triggered the switch so users can audit it:

Dir.glob("app/javascript/**/*.js").each do |path|
  content = File.read(path)
  if content.match?(%r{<\s*[A-Za-z][\w:-]*(\s|>|/)}) || content.match?(/<\s*>/)
    say "  → #{path}", :yellow  # show which file triggered the detection
    return true
  end
  false
rescue StandardError
  false
end

Direct @using_swc assignment bypasses the memoization pattern used elsewhere.

@using_swc = false sets the raw ivar directly, but the caching/initialization convention (e.g. @using_swc.nil? ? ... : @using_swc) elsewhere means false and "not yet computed" are distinguishable. Directly assigning false here is intentional, but it tightly couples this method to the implementation details of using_swc?.

If using_swc? is ever refactored to use ||= or similar memoization, this manual reset could silently stop working. Consider using a dedicated reset method or a comment explaining the coupling:

# Reset the memoized SWC flag so subsequent using_swc? calls reflect the YAML change.
@using_swc = false

result ? true : false is equivalent to !!result. Both are valid, but either !!result or just returning result (since the caller already coerces to boolean) would be more idiomatic Ruby.

→ !!result or simply return result if the callers all treat it as truthy/falsy.

detect_package_manager_from_lockfiles is now a public class method but is called internally as an implementation detail of detect_package_manager.

Exposing it publicly means external callers could call it directly and get nil (when no lockfile exists) rather than the "npm" default that detect_package_manager provides. This nil return is silently consumed in seed_package_manager_in_package_json_from_lockfile! (which correctly returns early on nil), but a caller who forgets to handle nil could get unexpected behavior.

Consider either:

1. Keeping it private and delegating from install_generator.rb through detect_package_manager (which already provides the nil-safe default), or
2. Documenting the nil return contract clearly so callers know to guard against it.

Fallback install silently returns nil on system failure

Medium Severity

install_packages_with_fallback returns the raw result of system(*install_args), which can be nil (command not found) or false (non-zero exit) without emitting any warning. This contrasts with install_js_dependencies, which explicitly checks the system return, emits a warning on failure, and returns false. The nil return also violates the documented @return [Boolean] contract of add_packages. When the detected package manager isn't installed, the fallback silently fails with no user-facing message about which command was attempted.