Walkthrough

This PR introduces dynamic path resolution for package.json and webpack configuration files within the ReactOnRails diagnostic system. New resolver methods are added to both Doctor and SystemChecker classes to discover configurations in non-root JS workspaces and custom Shakapacker directories, replacing hardcoded path references with resolved paths throughout existing checks.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 With whiskers high, the paths now dance,
No hardcoded stance!
From root to workspace, webpack does roam,
Each configuration finds its home,
Dynamic discovery—a bunny's delight! 🎉

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 @coderabbitai help to get the list of available commands and usage tips.}

Greptile Summary

This PR fixes false positives in react_on_rails:doctor for apps that use non-root JS workspaces (e.g., client/package.json) or custom webpack config locations. It resolves package.json from ReactOnRails.configuration.node_modules_location, broadens webpack config discovery to common custom paths and Shakapacker-configured directories, and downgrades missing webpack config from a hard error to a contextual warning.

• resolved_package_json_path now respects node_modules_location config in both Doctor and SystemChecker
• resolved_webpack_config_path searches multiple candidate paths including Shakapacker-configured directories and various extensions (.js, .ts, .cjs, .mjs)
• Missing webpack config in SystemChecker#check_webpack_configuration downgraded from add_error to add_warning
• Webpack config removed from the static file checklist in Doctor#check_key_configuration_files and replaced with dynamic resolution
• Regression specs added for both non-root JS workspace and custom webpack config discovery
• Issue: The candidate ordering in resolved_webpack_config_path differs between doctor.rb and system_checker.rb, which could lead to inconsistent results in edge cases
• Issue: check_shakapacker_configuration error message still hardcodes config/webpack/webpack.config.js despite the check now supporting custom paths

Confidence Score: 4/5

• This PR is safe to merge with minor issues — it reduces false positives in a diagnostic tool and does not affect core runtime behavior.
• The changes are well-scoped and address a real user pain point. The two issues found (inconsistent candidate ordering between Doctor and SystemChecker, and a stale error message) are unlikely to cause runtime failures but could lead to confusing diagnostics output. Good test coverage is included for the new behavior.
• Pay attention to react_on_rails/lib/react_on_rails/doctor.rb (candidate ordering inconsistency) and react_on_rails/lib/react_on_rails/system_checker.rb (stale error message in check_shakapacker_configuration).

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Doctor / SystemChecker needs package.json] --> B{node_modules_location configured?}
    B -->|empty / Rails.root| C[Use root package.json]
    B -->|custom path e.g. 'client'| D["Use client/package.json"]
    
    E[Doctor / SystemChecker needs webpack config] --> F[Build candidate list]
    F --> G["1. config/webpack/webpack.config.js"]
    F --> H["2. Shakapacker config directory paths"]
    F --> I["3. Dir.glob config/**/webpack.config.*"]
    G --> J{First candidate that exists?}
    H --> J
    I --> J
    J -->|Found| K[Return discovered path]
    J -->|None found| L["Return nil → warning instead of error"]

_{Last reviewed commit: eea9116}

Review

The direction here is solid — resolving package.json via node_modules_location and broadening webpack config discovery are good fixes for the reported false positives.

Code duplication between doctor.rb and system_checker.rb

resolved_package_json_path, resolved_webpack_config_path / webpack_config_candidates, and shakapacker_webpack_config_directory are implemented nearly identically in both files, but with a subtle inconsistency in candidate ordering (see inline comment). These helpers should live in a shared module (e.g. ReactOnRails::DiagnosticHelpers) so future changes do not silently diverge.

parse_package_json rescues only JSON::ParserError

File.read raises Errno::ENOENT when the file is absent — not JSON::ParserError. The method is safe today because check_react_dependencies guards with File.exist? beforehand, but parse_package_json is not independently safe to call. Consider adding Errno::ENOENT to the rescue clause or adding a guard inside the method itself.

Minor nits

• check_webpack_config_content uses resolved_webpack_config_path as a default-argument expression. This re-runs the filesystem search on every call that omits the argument, creating a subtle performance trap for future callers.
• The missing-webpack-config warning still ends with 'Run: rails generate react_on_rails:install', which is misleading for apps that intentionally use a custom config path.

Review

The approach here is solid. Resolving package.json from node_modules_location and broadening webpack config discovery are the right fixes for the false-positive problem. One blocking bug and a few smaller issues to address before merging.

Bug: shakapacker_webpack_config_directory returns a file path, not a directory

Shakapacker.config.assets_bundler_config_path returns the path to the webpack config FILE (e.g. /app/config/webpack/webpack.config.js). After stripping the Rails root the method still returns config/webpack/webpack.config.js — a file path. It is then used as a directory in File.join(shakapacker_config_dir, webpack.config.EXT), yielding bogus paths like config/webpack/webpack.config.js/webpack.config.ts that will never exist.

The Shakapacker-specific candidates are therefore silently dead; only the Dir.glob fallback actually finds configs. This masks the bug but means the Shakapacker path logic provides zero benefit. Fix: add File.dirname before stripping the Rails root (see inline comment for a code suggestion). The same fix is needed in both system_checker.rb and doctor.rb.

Code duplication

resolved_package_json_path, resolved_webpack_config_path, webpack_config_candidates, and shakapacker_webpack_config_directory are now copy-pasted between SystemChecker and Doctor. This is a maintenance hazard. Extracting these into a shared module (e.g. ReactOnRails::ConfigPathResolver) would be cleaner, but at minimum they must stay in sync.

Minor issues

• rescue StandardError in resolved_package_json_path is broader than needed and can silently swallow programming errors. Narrow to Errno::ENOENT, TypeError (or remove — File.join on a valid string cannot raise).
• The test for the custom webpack config path mocks Dir.glob, so it never exercises the Shakapacker-based candidate logic. A test that stubs Shakapacker.config.assets_bundler_config_path would catch the File.dirname regression.

Review Summary

The core logic — resolving package.json from node_modules_location and broadening webpack config discovery — is sound and the regression tests cover the new paths well. A few things worth addressing before merge:

Must Fix

'use client' directive syntax in docs (two inline comments)
The ('use client') parenthesised form is not a valid directive prologue in the ECMAScript spec. Some parsers/bundlers check for a bare StringLiteral AST node as the first statement; wrapping it in parens produces a (StringLiteral) grouped expression which may silently fail. The change appears to silence a no-unused-expressions linter in CI — the right fix is a linter suppression comment in the example or an eslint directive option, not rewriting the documented syntax.

Should Fix

Code duplication across doctor.rb and system_checker.rb
resolved_package_json_path, resolved_webpack_config_path / webpack_config_candidates, and shakapacker_webpack_config_directory are now defined identically in both files. Future bug fixes or extensions will need to be applied twice. Extracting these into a shared module (e.g. ReactOnRails::WebpackConfigResolver) would eliminate the duplication and allow the test cases to be written once as well.

Inconsistency between the two resolved_webpack_config_path implementations
system_checker.rb cleanly delegates to webpack_config_candidates; doctor.rb builds the list inline. They should be kept consistent — another argument for the shared module above.

Nits

• Dir.glob in doctor.rb:1195 already returns only existing paths, so the File.exist? check in the find block is redundant for those entries (harmless, but noisy).
• The suggest_webpack_inspection default parameter ("config/webpack/webpack.config.js") is unreachable — the method is only called when a path has already been found.

Positive notes

• Downgrading "missing webpack config" from error → warning is the right UX call given the variety of custom layouts.
• The Shakapacker config path extraction (shakapacker_webpack_config_directory) is well-guarded with rescue LoadError, StandardError.
• Test coverage for the new paths is thorough.

Review Summary

The direction here is correct - resolving package.json via node_modules_location and broadening webpack config discovery are clear improvements over hard-coding config/webpack/webpack.config.js. A few issues worth addressing before merge:

Code duplication across two files (structural)

resolved_package_json_path, resolved_webpack_config_path / webpack_config_candidates, and shakapacker_webpack_config_directory are copy-pasted verbatim into both system_checker.rb and doctor.rb. Any bug fix or enhancement has to be applied twice. These should live in a shared module (e.g. ReactOnRails::WebpackPathHelpers) included in both classes, or a small utility object both can delegate to. (See inline comments on system_checker.rb lines 710-744.)

resolved_package_json_path: relative path assumption

When node_modules_location is a relative string like client, the method returns client/package.json - a path relative to the process CWD. This works as long as the CWD is Rails.root, which is conventional but not guaranteed. The node_modules_location == Rails.root.to_s guard also misses variants like a trailing slash or a symlink-resolved path, which would slip through and produce a doubly-joined path. Normalising via Pathname to an absolute path makes the contract explicit. (Inline suggestion on line 712.)

Dir.glob in candidate list produces non-deterministic ordering

Dir.glob already returns only existing paths, so the subsequent File.exist? call is redundant for those entries. More subtly, glob result ordering is filesystem-dependent; .find will return whichever file the OS lists first if multiple matches exist. For a diagnostic tool this is low risk, but a comment explaining the intended fallback semantics would help.

Minor

• Downgrading check_webpack_configuration from add_error to add_warning is the right call for non-standard layouts.
• parse_package_json now rescues Errno::ENOENT in addition to JSON::ParserError - correct fix.
• The new specs cover the happy paths well; a negative test (e.g. confirming the method returns nil / falls back gracefully when neither the shakapacker-derived path nor the default exist) would round out the coverage.

Review: Fix #2606 - reduce doctor false positives for custom layouts

Overall the direction is right -- resolving paths from node_modules_location instead of hardcoding repo-root assumptions is the correct fix. A few issues worth addressing before merge:

Bug: mixed relative/absolute paths from resolved_package_json_path

The default branch returns the bare string "package.json" (relative), while the workspace branch returns an absolute path via Rails.root.join(...).to_s. The relative form works only when Dir.pwd == Rails.root, which is not guaranteed in Rake tasks or certain CI environments. VersionChecker::NodePackageVersion.package_json_path already solves this and always returns an absolute path -- worth either delegating to it or applying the same pattern. See inline comment on line 9 of config_path_resolver.rb.

Code duplication with version_checker.rb

VersionChecker::NodePackageVersion.package_json_path already resolves package.json using node_modules_location. The new resolved_package_json_path reimplements the same logic. Centralising would mean one place to maintain if configuration semantics change.

shakapacker_webpack_config_directory path-stripping edge case

start_with?("#{rails_root}/") requires a trailing slash, so it does not match when the webpack config sits directly in Rails.root (i.e. directory == rails_root). The method then returns an absolute path, mixing absolute and relative strings in the candidates list. Using Pathname#relative_path_from handles this cleanly -- see the inline suggestion on line 37.

Default webpack-config fallback only covers .js

Shakapacker-derived candidates cover js/ts/cjs/mjs, but the hardcoded fallback config/webpack/webpack.config.js only covers .js. Non-Shakapacker projects using a .ts or .mjs config at the default location will not be found. The inline suggestion on line 26 applies the same extension set to the default path.

Warning message is misleading

When no webpack config is found, the warning still names only config/webpack/webpack.config.js, even though ts/cjs/mjs variants and Shakapacker paths were also searched. This can confuse developers into thinking only one location was checked. See inline comment on doctor.rb line 630.

Pre-existing tests may silently break

Tests outside the new workspace blocks stub File.exist? with "package.json" (relative). Now that all package.json reads go through resolved_package_json_path, those stubs only fire if node_modules_location.to_s == Rails.root.to_s in the test environment -- implicit and fragile. Explicitly stubbing Rails.root and node_modules_location in those tests (as the new workspace tests already do) would make them robust.

@cursor review
@codex review

Codex Review: Didn't find any major issues. You're on a roll.

@cursor push f9dc52a