Fix Version Checker's hard-coded path for package.json #1657
Conversation
|
Warning Rate limit exceeded@Judahmeek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 16 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. WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
lib/react_on_rails/version_checker.rb (1)
77-77: LGTM! Consider making the path configurable in the future.The change correctly aligns with Shakapacker v8 requirements by looking for package.json in the root directory. However, as mentioned in the PR objectives, consider making this path configurable to support different project structures.
Consider adding a configuration option in the future:
def self.package_json_path Rails.root.join(ReactOnRails.configuration.package_json_path || "package.json") endCHANGELOG.md (1)
21-24: LGTM with a minor suggestion.The changelog entry correctly documents the package.json path change. Consider adding a note about why this change was necessary (to match Shakapacker v8 requirements) to provide more context for future readers.
- Changed the path of the package.json that the ReactOnRails' version checker attempts to use to be `/` instead of `/client`. [PR 1657](https://github.com/shakacode/react_on_rails/pull/1657) by [judahmeek](https://github.com/judahmeek). + Changed the path of the package.json that the ReactOnRails' version checker attempts to use to be `/` instead of `/client` to match Shakapacker v8 requirements. [PR 1657](https://github.com/shakacode/react_on_rails/pull/1657) by [judahmeek](https://github.com/judahmeek).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)lib/react_on_rails/version_checker.rb(1 hunks)
🔇 Additional comments (4)
lib/react_on_rails/version_checker.rb (2)
77-77: Verify version checking still works with relative paths
The change preserves compatibility with relative paths through the relative_path? check. However, we should verify that version checking still works correctly with the new path.
✅ Verification successful
Let me check the test coverage to verify the version checking functionality.
Version checking is properly handled with relative paths
The test coverage and implementation confirm that version checking works correctly with the new path. Specifically:
- The
relative_path?check explicitly handles both relative paths (../) and file URLs - Test cases cover relative path scenarios, verifying that version checking is skipped appropriately
- The change to use
Rails.root.joindoesn't affect the version checking logic as the path resolution remains consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test coverage of version checking
echo "Checking for version checker tests..."
rg -l "describe.*VersionChecker|test.*VersionChecker" --type ruby
# Look for any existing relative path usage
echo "Checking for relative path usage in package.json..."
rg -l "file://|\.\./" package.json || true
Length of output: 353
Script:
#!/bin/bash
# Check the version checker test content
echo "Examining version checker test cases..."
cat spec/react_on_rails/version_checker_spec.rb
# Check the actual version checker implementation
echo "Examining version checker implementation..."
cat lib/react_on_rails/version_checker.rb
Length of output: 10907
77-77: Verify impact on existing projects and update documentation
This change could break existing projects that still have package.json in the client/ directory. We should verify the impact and ensure documentation is updated.
CHANGELOG.md (2)
Line range hint 25-29: LGTM!
The "Added" section is well-structured with clear descriptions of new features, proper PR references, and author attributions.
Line range hint 1-20: LGTM!
The changelog follows the recommended format from keepachangelog.com and maintains consistent structure throughout. Version history and links are properly maintained.
| @@ -74,7 +74,7 @@ def self.build | |||
| end | |||
|
|
|||
| def self.package_json_path | |||
| Rails.root.join("client", "package.json") | |||
| Rails.root.join("package.json") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit error handling for missing package.json
The current implementation will raise a generic file not found error if package.json is missing. Consider adding explicit error handling to guide users about the new path requirement.
def self.package_json_path
- Rails.root.join("package.json")
+ path = Rails.root.join("package.json")
+ unless File.exist?(path)
+ raise ReactOnRails::Error,
+ "Could not find package.json in Rails root directory. " \
+ "As per Shakapacker v8 requirements, package.json should be located at #{path}"
+ end
+ path
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Rails.root.join("package.json") | |
| def self.package_json_path | |
| path = Rails.root.join("package.json") | |
| unless File.exist?(path) | |
| raise ReactOnRails::Error, | |
| "Could not find package.json in Rails root directory. " \ | |
| "As per Shakapacker v8 requirements, package.json should be located at #{path}" | |
| end | |
| path | |
| end |
There was a problem hiding this comment.
is there a way this code could be DRY'd up with Shakapacker someday?
There was a problem hiding this comment.
are we making the one in the client directory not work?
There was a problem hiding this comment.
@justin808, with this change, the version checker will no longer look for client/package.json and will instead check for package.json in the Rails root directory. This means that if package.json exists only in the client directory, the version checker will not find it, which could cause issues for setups relying on the previous path.
To address this, we might consider modifying the code to support both locations for backward compatibility or make the path configurable. Currently, this change aligns with Shakapacker v8 requirements, which expect package.json to be located at the root of the project.
There was a problem hiding this comment.
@justin808, that's a great suggestion. Refactoring this code to share functionality with Shakapacker could help reduce duplication and improve maintainability. Would you like me to open a GitHub issue to track this potential enhancement?
Judahmeek
changed the title
|
@Judahmeek can you update this:
|
Judahmeek
force-pushed
the
judahmeek/package-path
branch
from
19812b4 to
a8cb13f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
lib/react_on_rails/version_checker.rb (1)
48-48: Consider improving warning message clarityThe warning messages are quite verbose and repeat similar information. Consider:
- Moving the common version information to a single line
- Making the warning more actionable
- Separating the v15 warning into a separate method since it's a deprecation notice
def common_error_msg <<-MSG.strip_heredoc - Detected: #{node_package_version.raw} - gem: #{gem_version} + Version mismatch - node: #{node_package_version.raw}, gem: #{gem_version} Ensure the installed version of the gem is the same as the version of your installed node package. Do not use >= or ~> in your Gemfile for react_on_rails. Do not use ^ or ~ in your package.json for react-on-rails. Run `yarn add react-on-rails --exact` in the directory containing folder node_modules. - ***This warning will become a fatal error in ReactOnRails v15*** MSG end +def deprecation_warning + "***This warning will become a fatal error in ReactOnRails v15***" +end def raise_differing_versions_warning msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}" + msg += "\n#{deprecation_warning}" Rails.logger.warn(msg) endAlso applies to: 53-54, 58-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)lib/react_on_rails/engine.rb(1 hunks)lib/react_on_rails/version_checker.rb(5 hunks)
🔇 Additional comments (4)
lib/react_on_rails/engine.rb (1)
8-9: Verify VersionChecker's behavior
The method name raise_if_gem_and_node_package_versions_differ suggests it raises an error, which contradicts the requirement for silent failure.
Let's verify the VersionChecker implementation:
lib/react_on_rails/version_checker.rb (1)
79-80: Verify backward compatibility with existing setups
Let's verify if any existing projects rely on the client/package.json path.
✅ Verification successful
Let me check the specific references to understand the impact better.
Let me check one more thing to understand if this is a breaking change by looking at the version checker implementation.
Change is safe but needs documentation update
The implementation maintains backward compatibility:
- The engine.rb checks both
package.jsonandclient/package.jsonlocations - The version checker accepts a relative path parameter, defaulting to "package.json"
- Documentation actively references
client/package.jsonas a valid setup
However, the documentation should be updated to reflect this configurable path feature.
docs/additional-details/recommended-project-structure.md: Update to mention the configurable package.json pathdocs/additional-details/manual-installation-overview.md: Add note about configurable package.json locationdocs/guides/upgrading-react-on-rails.md: Include information about the flexible package.json path support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of client/package.json in existing projects
# Search for references to client/package.json
echo "Searching for references to client/package.json..."
rg -l "client/package.json"
# Search for package.json files in client directories
echo "Looking for package.json files in client directories..."
fd -p "client/package.json$"
Length of output: 505
Script:
#!/bin/bash
# Check the context of client/package.json references
echo "Checking CHANGELOG.md..."
rg -C 3 "client/package.json" CHANGELOG.md
echo "\nChecking engine.rb..."
rg -C 3 "client/package.json" lib/react_on_rails/engine.rb
echo "\nChecking documentation references..."
rg -C 3 "client/package.json" docs/guides/upgrading-react-on-rails.md docs/additional-details/manual-installation-overview.md docs/additional-details/recommended-project-structure.md docs/guides/i18n.md
Length of output: 7070
Script:
#!/bin/bash
# Check the version checker implementation
echo "Checking version_checker.rb implementation..."
cat lib/react_on_rails/version_checker.rb
Length of output: 3958
CHANGELOG.md (2)
21-24: LGTM! Changelog entry accurately reflects the version checker path changes.
The changelog entry properly documents the modification to check package.jsons in both root and client directories, which aligns with the PR objectives to fix the hard-coded path issue for Shakapacker v8 compatibility.
Line range hint 25-38: Skipping review of other recent changes
These changelog entries document other features and fixes that are not related to the version checker path changes in this PR.
lib/react_on_rails/engine.rb
Outdated
| if VersionChecker.new("package.json").raise_if_gem_and_node_package_versions_differ && | ||
| VersionChecker.new("client/package.json").raise_if_gem_and_node_package_versions_differ | ||
| Rails.logger.warn("No 'react-on-rails' entry found in 'dependencies' in package.json or client/package.json.") |
There was a problem hiding this comment.
Fix the logical operator in version checking
The current implementation using && requires both version checks to fail before showing the warning. This is incorrect as we want the version check to succeed if either package.json is valid. Additionally, the warning message is only appropriate when both checks fail.
Consider this implementation:
- if VersionChecker.new("package.json").raise_if_gem_and_node_package_versions_differ &&
- VersionChecker.new("client/package.json").raise_if_gem_and_node_package_versions_differ
- Rails.logger.warn("No 'react-on-rails' entry found in 'dependencies' in package.json or client/package.json.")
+ root_check = VersionChecker.new("package.json").raise_if_gem_and_node_package_versions_differ
+ client_check = VersionChecker.new("client/package.json").raise_if_gem_and_node_package_versions_differ
+
+ if !root_check && !client_check
+ Rails.logger.debug("No 'react-on-rails' entry found in 'dependencies' in package.json or client/package.json.")
+ endThe changes:
- Split checks for better readability
- Changed to logical OR behavior (if neither check succeeds)
- Changed to debug level logging for silent failure
📝 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.
| if VersionChecker.new("package.json").raise_if_gem_and_node_package_versions_differ && | |
| VersionChecker.new("client/package.json").raise_if_gem_and_node_package_versions_differ | |
| Rails.logger.warn("No 'react-on-rails' entry found in 'dependencies' in package.json or client/package.json.") | |
| root_check = VersionChecker.new("package.json").raise_if_gem_and_node_package_versions_differ | |
| client_check = VersionChecker.new("client/package.json").raise_if_gem_and_node_package_versions_differ | |
| if !root_check && !client_check | |
| Rails.logger.debug("No 'react-on-rails' entry found in 'dependencies' in package.json or client/package.json.") | |
| end |
| return nil unless File.exist?(@package_json) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for file operations
The file operations could benefit from more robust error handling to catch potential issues like permission problems or invalid JSON.
def raw
return nil unless File.exist?(@package_json)
- parsed_package_contents = JSON.parse(package_json_contents)
+ begin
+ parsed_package_contents = JSON.parse(package_json_contents)
+ rescue JSON::ParserError => e
+ raise ReactOnRails::Error, "Invalid JSON in #{@package_json}: #{e.message}"
+ end
if parsed_package_contents.key?("dependencies") &&
parsed_package_contents["dependencies"].key?("react-on-rails")
parsed_package_contents["dependencies"]["react-on-rails"]
end
end
def package_json_contents
- @package_json_contents ||= File.read(@package_json)
+ @package_json_contents ||= begin
+ File.read(@package_json)
+ rescue Errno::EACCES
+ raise ReactOnRails::Error, "Permission denied reading #{@package_json}"
+ rescue Errno::ENOENT
+ raise ReactOnRails::Error, "Could not find #{@package_json}"
+ end
endAlso applies to: 119-119
| def self.package_json_path(relative_path = "package.json") | ||
| Rails.root.join(relative_path) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add path validation for package.json
The method accepts any relative path without validation. Consider adding checks to:
- Ensure the path ends with 'package.json'
- Validate that the path doesn't escape the Rails root directory
def self.package_json_path(relative_path = "package.json")
+ unless relative_path.end_with?("package.json")
+ raise ReactOnRails::Error, "Path must end with package.json"
+ end
+ path = Rails.root.join(relative_path)
+ unless path.to_s.start_with?(Rails.root.to_s)
+ raise ReactOnRails::Error, "Path must be within Rails root directory"
+ end
- Rails.root.join(relative_path)
+ path
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def self.package_json_path(relative_path = "package.json") | |
| Rails.root.join(relative_path) | |
| def self.package_json_path(relative_path = "package.json") | |
| unless relative_path.end_with?("package.json") | |
| raise ReactOnRails::Error, "Path must end with package.json" | |
| end | |
| path = Rails.root.join(relative_path) | |
| unless path.to_s.start_with?(Rails.root.to_s) | |
| raise ReactOnRails::Error, "Path must be within Rails root directory" | |
| end | |
| path |
| return true unless node_package_version.raw | ||
| return if node_package_version.relative_path? | ||
|
|
||
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | ||
|
|
||
| node_major_minor_patch = node_package_version.major_minor_patch | ||
| gem_major_minor_patch = gem_major_minor_patch_version | ||
| versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] && | ||
| node_major_minor_patch[1] == gem_major_minor_patch[1] && | ||
| node_major_minor_patch[2] == gem_major_minor_patch[2] | ||
|
|
||
| raise_differing_versions_warning unless versions_match | ||
|
|
||
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | ||
| false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming method to better reflect its behavior
The method name raise_if_gem_and_node_package_versions_differ suggests it will raise an exception, but it now logs warnings and returns a boolean. Consider renaming to something like check_gem_and_node_package_versions or verify_version_match to better reflect its current behavior.
Additionally, the false return value on line 35 seems arbitrary and its purpose is unclear.
-def raise_if_gem_and_node_package_versions_differ
+def check_gem_and_node_package_versions
return true unless node_package_version.raw
return if node_package_version.relative_path?
raise_node_semver_version_warning if node_package_version.semver_wildcard?
node_major_minor_patch = node_package_version.major_minor_patch
gem_major_minor_patch = gem_major_minor_patch_version
versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] &&
node_major_minor_patch[1] == gem_major_minor_patch[1] &&
node_major_minor_patch[2] == gem_major_minor_patch[2]
raise_differing_versions_warning unless versions_match
- false
+ versions_match
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return true unless node_package_version.raw | |
| return if node_package_version.relative_path? | |
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | |
| node_major_minor_patch = node_package_version.major_minor_patch | |
| gem_major_minor_patch = gem_major_minor_patch_version | |
| versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] && | |
| node_major_minor_patch[1] == gem_major_minor_patch[1] && | |
| node_major_minor_patch[2] == gem_major_minor_patch[2] | |
| raise_differing_versions_warning unless versions_match | |
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | |
| false | |
| def check_gem_and_node_package_versions | |
| return true unless node_package_version.raw | |
| return if node_package_version.relative_path? | |
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | |
| node_major_minor_patch = node_package_version.major_minor_patch | |
| gem_major_minor_patch = gem_major_minor_patch_version | |
| versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] && | |
| node_major_minor_patch[1] == gem_major_minor_patch[1] && | |
| node_major_minor_patch[2] == gem_major_minor_patch[2] | |
| raise_differing_versions_warning unless versions_match | |
| versions_match | |
| end |
Judahmeek
force-pushed
the
judahmeek/package-path
branch
from
a8cb13f to
ceb80a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
lib/react_on_rails/version_checker.rb (1)
88-89: Consider adding debug logging for missing package.jsonWhile the silent failure aligns with the requested behavior, adding debug logging would help troubleshoot issues without disrupting normal operation.
def raw - return nil unless File.exist?(package_json) + unless File.exist?(package_json) + Rails.logger.debug("ReactOnRails: package.json not found at #{package_json}") + return nil + endspec/react_on_rails/version_checker_spec.rb (1)
204-219: Add documentation for yalc package handling.The test coverage for yalc package paths is well-structured, but it would be helpful to add a comment explaining what yalc is and why this test case is important.
Add a descriptive comment before the context block:
+ # Yalc is a tool for local package development that creates symlinks. + # This test ensures we properly handle package.json files that reference + # local packages via yalc's file:.yalc/* syntax. context "with node version of 'file:.yalc/react-on-rails'" do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)lib/react_on_rails/engine.rb(1 hunks)lib/react_on_rails/version_checker.rb(4 hunks)spec/react_on_rails/fixtures/yalc_package.json(1 hunks)spec/react_on_rails/version_checker_spec.rb(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- spec/react_on_rails/fixtures/yalc_package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- lib/react_on_rails/engine.rb
🔇 Additional comments (2)
lib/react_on_rails/version_checker.rb (2)
23-35: Method behavior doesn't match its name
The method name suggests it raises an error, but it now returns boolean values and logs warnings instead.
79-80:
Add path validation and error handling
The current implementation accepts any relative path without validation, which could lead to:
- Path traversal vulnerabilities
- Silent failures if the path is invalid
- Inconsistent behavior with different path formats
Apply this diff to add proper validation:
def self.package_json_path(relative_path = "package.json")
+ unless relative_path.end_with?("package.json")
+ raise ReactOnRails::Error, "Path must end with package.json"
+ end
+ path = Rails.root.join(relative_path)
+ unless path.to_s.start_with?(Rails.root.to_s)
+ raise ReactOnRails::Error, "Path must be within Rails root directory"
+ end
- Rails.root.join(relative_path)
+ path
endLikely invalid or redundant comment.
| @@ -43,18 +45,19 @@ def common_error_msg | |||
| your installed node package. Do not use >= or ~> in your Gemfile for react_on_rails. | |||
| Do not use ^ or ~ in your package.json for react-on-rails. | |||
| Run `yarn add react-on-rails --exact` in the directory containing folder node_modules. | |||
| ***This warning will become a fatal error in ReactOnRails v15*** | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review warning message implementation
The current implementation:
- Always displays verbose warning messages
- Includes a note about fatal errors in v15, which might cause unnecessary concern
- Logs warnings for both version mismatches and semver wildcards
Consider:
- Making warning messages configurable or less verbose
- Removing the v15 warning if it's not immediately relevant
- Adding a debug log level for detailed information
Also applies to: 53-54, 58-60
| @@ -96,7 +99,7 @@ | |||
| end | |||
|
|
|||
| def relative_path? | |||
| raw.match(%r{(\.\.|\Afile:///)}).present? | |||
| raw.match(/(\.\.|\Afile:)/).present? | |||
There was a problem hiding this comment.
Security: Strengthen relative path detection
The current regex allows potentially dangerous path patterns:
..could allow directory traversalfile:protocol could allow access to arbitrary files
Consider using a more restrictive approach:
- raw.match(/(\.\.|\Afile:)/).present?
+ raw.start_with?('file:./') || raw.start_with?('file:src/')Committable suggestion skipped: line range outside the PR's diff.
| it "logs" do | ||
| allow(Rails.logger).to receive(:warn) | ||
| message = /ReactOnRails: Your node package version for react-on-rails contains a \^ or ~/ | ||
| # expect { check_version(node_package_version) }.to raise_error(message) | ||
| expect(Rails.logger).to have_received(:warn).with(message) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up and improve test expectations.
- Remove commented-out expectations instead of leaving them in the code.
- Consider DRYing up the repeated test pattern using shared examples.
- The logging expectations could be more specific about the complete message content.
Here's a suggested refactor to make the tests more maintainable:
- # expect { check_version(node_package_version) }.to raise_error(message)
- expect(Rails.logger).to have_received(:warn).with(message)
+ shared_examples "version mismatch logging" do |version_type|
+ it "logs version mismatch warning" do
+ allow(Rails.logger).to receive(:warn)
+ check_version(node_package_version)
+ expect(Rails.logger).to have_received(:warn).with(
+ a_string_matching(/#{expected_message}/)
+ )
+ end
+ end
+ context "when major versions differ" do
+ let(:expected_message) { "ReactOnRails: ReactOnRails gem and node package versions do not match" }
+ include_examples "version mismatch logging", "major"
+ endAlso applies to: 53-57, 68-72, 83-87
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
lib/react_on_rails/version_checker.rb (1)
15-17: LGTM! Consider adding path validationThe new
instancemethod provides good flexibility for configurable paths. However, consider adding path validation to ensure the provided path is valid and safe.def self.instance(package_json_path) + unless package_json_path.to_s.end_with?('package.json') + raise ReactOnRails::Error, "Path must end with package.json" + end new(NodePackageVersion.new(package_json_path)) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/react_on_rails/engine.rb(1 hunks)lib/react_on_rails/version_checker.rb(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/engine.rb
🔇 Additional comments (4)
lib/react_on_rails/version_checker.rb (4)
106-106:
Strengthen relative path detection
The current regex allows potentially dangerous path patterns that could enable directory traversal or access to arbitrary files.
- raw.match(/(\.\.|\Afile:)/).present?
+ return false unless raw.is_a?(String)
+ raw.start_with?('file:./') || raw.start_with?('file:src/')Likely invalid or redundant comment.
83-84:
Add path validation for security
The current implementation accepts any relative path without validation, which could allow directory traversal attacks.
def self.package_json_path(relative_path = "package.json")
+ path = Rails.root.join(relative_path)
+ unless path.to_s.start_with?(Rails.root.to_s)
+ raise ReactOnRails::Error, "Path must be within Rails root directory"
+ end
- Rails.root.join(relative_path)
+ path
endLikely invalid or redundant comment.
92-93: 🛠️ Refactor suggestion
Add comprehensive error handling
While the file existence check implements silent failure, other potential issues like permission problems should be handled gracefully.
def raw
- return nil unless File.exist?(package_json)
+ begin
+ return nil unless File.exist?(package_json)
- parsed_package_contents = JSON.parse(package_json_contents)
+ parsed_package_contents = JSON.parse(package_json_contents)
+ rescue Errno::EACCES
+ Rails.logger.warn("Permission denied reading #{package_json}")
+ return nil
+ rescue JSON::ParserError
+ Rails.logger.warn("Invalid JSON in #{package_json}")
+ return nil
+ endLikely invalid or redundant comment.
27-39: 🛠️ Refactor suggestion
Method name and behavior mismatch
The method name raise_if_gem_and_node_package_versions_differ suggests it raises an error, but it now returns boolean values and logs warnings. The false return value on line 39 seems arbitrary.
-def raise_if_gem_and_node_package_versions_differ
+def check_gem_and_node_package_versions
return true unless node_package_version.raw
return if node_package_version.relative_path?
raise_node_semver_version_warning if node_package_version.semver_wildcard?
node_major_minor_patch = node_package_version.major_minor_patch
gem_major_minor_patch = gem_major_minor_patch_version
versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] &&
node_major_minor_patch[1] == gem_major_minor_patch[1] &&
node_major_minor_patch[2] == gem_major_minor_patch[2]
raise_differing_versions_warning unless versions_match
- false
+ versions_match
endLikely invalid or redundant comment.
| ***This warning will become a fatal error in ReactOnRails v15*** | ||
| MSG | ||
| end | ||
|
|
||
| def raise_differing_versions_warning | ||
| msg = "**ERROR** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}" | ||
| raise ReactOnRails::Error, msg | ||
| msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}" | ||
| Rails.logger.warn(msg) | ||
| end | ||
|
|
||
| def raise_node_semver_version_warning | ||
| msg = "**ERROR** ReactOnRails: Your node package version for react-on-rails contains a " \ | ||
| msg = "**WARNING** ReactOnRails: Your node package version for react-on-rails contains a " \ | ||
| "^ or ~\n#{common_error_msg}" | ||
| raise ReactOnRails::Error, msg | ||
| Rails.logger.warn(msg) |
There was a problem hiding this comment.
Simplify warning messages per feedback
The current warning messages are still verbose and the v15 warning may cause unnecessary concern. Per justin808's feedback, these should be more concise.
def common_error_msg
<<-MSG.strip_heredoc
Detected: #{node_package_version.raw}
gem: #{gem_version}
- Ensure the installed version of the gem is the same as the version of
- your installed node package. Do not use >= or ~> in your Gemfile for react_on_rails.
- Do not use ^ or ~ in your package.json for react-on-rails.
- Run `yarn add react-on-rails --exact` in the directory containing folder node_modules.
- ***This warning will become a fatal error in ReactOnRails v15***
+ To fix: Run `yarn add react-on-rails@#{gem_version} --exact`
MSG
end
def raise_differing_versions_warning
- msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}"
+ msg = "Version mismatch between gem and node package\n#{common_error_msg}"
Rails.logger.warn(msg)
end
def raise_node_semver_version_warning
- msg = "**WARNING** ReactOnRails: Your node package version for react-on-rails contains a " \
- "^ or ~\n#{common_error_msg}"
+ msg = "Semver wildcards not supported in package.json\n#{common_error_msg}"
Rails.logger.warn(msg)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ***This warning will become a fatal error in ReactOnRails v15*** | |
| MSG | |
| end | |
| def raise_differing_versions_warning | |
| msg = "**ERROR** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}" | |
| raise ReactOnRails::Error, msg | |
| msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}" | |
| Rails.logger.warn(msg) | |
| end | |
| def raise_node_semver_version_warning | |
| msg = "**ERROR** ReactOnRails: Your node package version for react-on-rails contains a " \ | |
| msg = "**WARNING** ReactOnRails: Your node package version for react-on-rails contains a " \ | |
| "^ or ~\n#{common_error_msg}" | |
| raise ReactOnRails::Error, msg | |
| Rails.logger.warn(msg) | |
| def common_error_msg | |
| <<-MSG.strip_heredoc | |
| Detected: #{node_package_version.raw} | |
| gem: #{gem_version} | |
| To fix: Run `yarn add react-on-rails@#{gem_version} --exact` | |
| MSG | |
| end | |
| def raise_differing_versions_warning | |
| msg = "Version mismatch between gem and node package\n#{common_error_msg}" | |
| Rails.logger.warn(msg) | |
| end | |
| def raise_node_semver_version_warning | |
| msg = "Semver wildcards not supported in package.json\n#{common_error_msg}" | |
| Rails.logger.warn(msg) | |
| end |
lib/react_on_rails/engine.rb
Outdated
| VersionChecker.build.raise_if_gem_and_node_package_versions_differ | ||
| if VersionChecker.instance("package.json").raise_if_gem_and_node_package_versions_differ && | ||
| VersionChecker.instance("client/package.json").raise_if_gem_and_node_package_versions_differ | ||
| Rails.logger.warn("No 'react-on-rails' entry found in 'dependencies' in package.json or client/package.json.") |
There was a problem hiding this comment.
- Isn't this a breaking change?
- Semantically, having methods with side effect of
raisedoes not seem right for anif
There was a problem hiding this comment.
all this consistency checking logic should go to one method, that logs warnings for now, and will throw for the next major version bump
| @@ -43,18 +49,19 @@ def common_error_msg | |||
| your installed node package. Do not use >= or ~> in your Gemfile for react_on_rails. | |||
| Do not use ^ or ~ in your package.json for react-on-rails. | |||
| Run `yarn add react-on-rails --exact` in the directory containing folder node_modules. | |||
| ***This warning will become a fatal error in ReactOnRails v15*** | |||
| MSG | |||
| end | |||
|
|
|||
| def raise_differing_versions_warning | |||
There was a problem hiding this comment.
method names can't be raise, but mean log
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/react_on_rails/engine.rb(1 hunks)lib/react_on_rails/version_checker.rb(5 hunks)spec/react_on_rails/version_checker_spec.rb(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/engine.rb
🔇 Additional comments (6)
lib/react_on_rails/version_checker.rb (3)
15-17: LGTM! Good addition for configuration flexibility.
The new instance method allows for custom package.json path configuration, which aligns with the PR objective of making the path configurable.
66-66: 🛠️ Refactor suggestion
Simplify warning messages per feedback.
The warning messages are verbose and the v15 warning may cause unnecessary concern. Consider simplifying the messages and removing the v15 warning.
Also applies to: 81-90
108-109:
Add path validation and strengthen relative path detection.
The current implementation has potential security concerns:
- No validation that the path ends with 'package.json'
- The relative path regex could allow directory traversal
Also applies to: 131-131
spec/react_on_rails/version_checker_spec.rb (3)
44-54: Clean up and improve test structure.
The test file has several repeated patterns that could be DRYed up using shared examples. Also, some commented-out expectations should be removed.
Also applies to: 65-75, 86-96, 107-117
139-150: LGTM! Good test coverage for package.json existence.
The new test cases properly verify the behavior when package.json doesn't exist, which is crucial for the PR's objective of handling missing files gracefully.
260-274: LGTM! Good test coverage for yalc paths.
The new test cases properly verify the behavior with yalc paths, which is a common development scenario.
| return true unless node_package_version.raw | ||
| return if node_package_version.relative_path? | ||
|
|
||
| node_major_minor_patch = node_package_version.major_minor_patch | ||
| gem_major_minor_patch = gem_major_minor_patch_version | ||
| versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] && | ||
| node_major_minor_patch[1] == gem_major_minor_patch[1] && | ||
| node_major_minor_patch[2] == gem_major_minor_patch[2] | ||
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | ||
|
|
||
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | ||
|
|
||
| raise_differing_versions_warning unless versions_match | ||
| false | ||
| end | ||
|
|
||
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | ||
| def log_if_gem_and_node_package_versions_differ | ||
| return true unless node_package_version.raw | ||
| return if node_package_version.relative_path? | ||
|
|
||
| log_node_semver_version_warning if node_package_version.semver_wildcard? | ||
|
|
||
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | ||
|
|
||
| log_differing_versions_warning unless versions_match | ||
| false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce duplication and improve naming.
The raise_if_gem_and_node_package_versions_differ and log_if_gem_and_node_package_versions_differ methods have identical logic but different actions. Consider:
- Extracting the common logic into a private method
- Renaming methods to better reflect their behavior (they return boolean values)
-def raise_if_gem_and_node_package_versions_differ
- return true unless node_package_version.raw
- return if node_package_version.relative_path?
-
- raise_node_semver_version_warning if node_package_version.semver_wildcard?
-
- versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version)
-
- raise_differing_versions_warning unless versions_match
- false
-end
-
-def log_if_gem_and_node_package_versions_differ
- return true unless node_package_version.raw
- return if node_package_version.relative_path?
-
- log_node_semver_version_warning if node_package_version.semver_wildcard?
-
- versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version)
-
- log_differing_versions_warning unless versions_match
- false
-end
+def check_versions(action = :log)
+ return true unless node_package_version.raw
+ return if node_package_version.relative_path?
+
+ if node_package_version.semver_wildcard?
+ action == :log ? log_node_semver_version_warning : raise_node_semver_version_warning
+ end
+
+ versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version)
+
+ unless versions_match
+ action == :log ? log_differing_versions_warning : raise_differing_versions_warning
+ end
+ false
+end
+
+def check_versions_with_raise
+ check_versions(:raise)
+end
+
+def check_versions_with_log
+ check_versions(:log)
+end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return true unless node_package_version.raw | |
| return if node_package_version.relative_path? | |
| node_major_minor_patch = node_package_version.major_minor_patch | |
| gem_major_minor_patch = gem_major_minor_patch_version | |
| versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] && | |
| node_major_minor_patch[1] == gem_major_minor_patch[1] && | |
| node_major_minor_patch[2] == gem_major_minor_patch[2] | |
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | |
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | |
| raise_differing_versions_warning unless versions_match | |
| false | |
| end | |
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | |
| def log_if_gem_and_node_package_versions_differ | |
| return true unless node_package_version.raw | |
| return if node_package_version.relative_path? | |
| log_node_semver_version_warning if node_package_version.semver_wildcard? | |
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | |
| log_differing_versions_warning unless versions_match | |
| false | |
| def check_versions(action = :log) | |
| return true unless node_package_version.raw | |
| return if node_package_version.relative_path? | |
| if node_package_version.semver_wildcard? | |
| action == :log ? log_node_semver_version_warning : raise_node_semver_version_warning | |
| end | |
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | |
| unless versions_match | |
| action == :log ? log_differing_versions_warning : raise_differing_versions_warning | |
| end | |
| false | |
| end | |
| def check_versions_with_raise | |
| check_versions(:raise) | |
| end | |
| def check_versions_with_log | |
| check_versions(:log) | |
| end |
There was a problem hiding this comment.
@Judahmeek please commit this this suggestion.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -20,21 +24,37 @@ def initialize(node_package_version) | |||
| # unless the user really knows what they're doing. So we will give a | |||
| # warning if they do not. | |||
| def raise_if_gem_and_node_package_versions_differ | |||
There was a problem hiding this comment.
is anybody code still calling this one?
we'll need a reminder for the next big update? Maybe leave a PR pending?
There was a problem hiding this comment.
We can create a follow-up PR and leave it pending.
lib/react_on_rails/engine.rb
Outdated
| VersionChecker.build.raise_if_gem_and_node_package_versions_differ | ||
| if VersionChecker.instance("package.json").raise_if_gem_and_node_package_versions_differ && | ||
| VersionChecker.instance("client/package.json").raise_if_gem_and_node_package_versions_differ | ||
| Rails.logger.warn("No 'react-on-rails' entry found in 'dependencies' in package.json or client/package.json.") |
There was a problem hiding this comment.
all this consistency checking logic should go to one method, that logs warnings for now, and will throw for the next major version bump
CHANGELOG.md
Outdated
| #### Added(https://github.com/AbanoubGhadban). | ||
| #### Fixed | ||
|
|
||
| - Changed the ReactOnRails' version checker to check package.jsons in both root and client directories. [PR 1657](https://github.com/shakacode/react_on_rails/pull/1657) by [judahmeek](https://github.com/judahmeek). |
There was a problem hiding this comment.
Should be using node_modules_location.
https://github.com/search?q=repo%3Ashakacode%2Freact_on_rails%20node_modules_location&type=code
| return true unless node_package_version.raw | ||
| return if node_package_version.relative_path? | ||
|
|
||
| node_major_minor_patch = node_package_version.major_minor_patch | ||
| gem_major_minor_patch = gem_major_minor_patch_version | ||
| versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] && | ||
| node_major_minor_patch[1] == gem_major_minor_patch[1] && | ||
| node_major_minor_patch[2] == gem_major_minor_patch[2] | ||
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | ||
|
|
||
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | ||
|
|
||
| raise_differing_versions_warning unless versions_match | ||
| false | ||
| end | ||
|
|
||
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | ||
| def log_if_gem_and_node_package_versions_differ | ||
| return true unless node_package_version.raw | ||
| return if node_package_version.relative_path? | ||
|
|
||
| log_node_semver_version_warning if node_package_version.semver_wildcard? | ||
|
|
||
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | ||
|
|
||
| log_differing_versions_warning unless versions_match | ||
| false |
There was a problem hiding this comment.
@Judahmeek please commit this this suggestion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
lib/react_on_rails/version_checker.rb (2)
52-60: Rename warning methods to match their behaviorThe method names starting with "log_" are more accurate than the previous "raise_" prefixes, but they could be even clearer.
Consider renaming for clarity:
-def log_differing_versions_warning +def warn_version_mismatch msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}" Rails.logger.warn(msg) end -def log_node_semver_version_warning +def warn_semver_wildcard msg = "**WARNING** ReactOnRails: Your node package version for react-on-rails contains a " \ "^ or ~\n#{common_error_msg}" Rails.logger.warn(msg) end
80-80: Consider caching the package.json pathThe path is computed each time but could be cached for better performance.
def self.package_json_path - Rails.root.join(ReactOnRails.configuration.node_modules_location, "package.json") + @package_json_path ||= Rails.root.join(ReactOnRails.configuration.node_modules_location, "package.json") endspec/react_on_rails/version_checker_spec.rb (3)
26-29: DRY up version mismatch test cases using shared examplesMultiple test cases follow the same pattern of setting up logger expectations and verifying warnings.
Extract shared behavior:
+shared_examples "version check logging" do |scenario, expected_warning| + it "handles #{scenario}" do + allow(Rails.logger).to receive(:warn) + check_version_and_log(node_package_version) + if expected_warning + expect(Rails.logger).to have_received(:warn).with(/#{expected_warning}/) + else + expect(Rails.logger).not_to have_received(:warn) + end + end +end context "when gem and node package major and minor versions are equal" do let(:node_package_version) do double_package_version(raw: "2.2.5-beta.2", major_minor_patch: %w[2 2 5]) end before { stub_gem_version("2.2.5.beta.2") } - it "does not log" do - allow(Rails.logger).to receive(:warn) - check_version_and_log(node_package_version) - expect(Rails.logger).not_to have_received(:warn) - end + include_examples "version check logging", "matching versions", nil endAlso applies to: 40-44, 55-59, 70-74, 85-89
140-149: Improve test description and error message verificationThe test description and error message verification could be more precise.
describe "#build" do - it "initializes NodePackageVersion with ReactOnRails.configuration.node_modules_location" do + it "raises error when react-on-rails is missing from package.json dependencies" do allow(ReactOnRails).to receive_message_chain(:configuration, :node_modules_location).and_return("spec/dummy") root_package_json_path = File.expand_path("../../package.json", __dir__) allow(Rails).to receive_message_chain(:root, :join).and_return(root_package_json_path) - message = "No 'react-on-rails' entry in the dependencies of #{root_package_json_path}, which is " \ - "the expected location according to ReactOnRails.configuration.node_modules_location" - expect { described_class.build.raw }.to raise_error(message) + expect { described_class.build.raw }.to raise_error(ReactOnRails::Error) + .with_message(/No 'react-on-rails' entry in the dependencies/) end end
251-257: Add more edge cases for package.json handlingThe test coverage for non-existing package.json could be expanded.
Add test cases for:
- Invalid JSON content
- Empty package.json
- Missing dependencies section
- Permissions issues
context "with invalid package.json" do let(:package_json) { File.expand_path("fixtures/invalid_package.json", __dir__) } describe "#raw" do it "raises error for invalid JSON" do expect { node_package_version.raw }.to raise_error(ReactOnRails::Error, /Invalid JSON/) end end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/react_on_rails/engine.rb(1 hunks)lib/react_on_rails/version_checker.rb(4 hunks)spec/react_on_rails/version_checker_spec.rb(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/engine.rb
🔇 Additional comments (4)
lib/react_on_rails/version_checker.rb (4)
106-106:
Strengthen relative path detection
The current regex allows potentially dangerous path patterns.
- raw.match(/(\.\.|\Afile:)/).present?
+ raw.start_with?('file:./') || raw.start_with?('file:src/') || raw.include?('..')Likely invalid or redundant comment.
88-98: 🛠️ Refactor suggestion
Enhance error handling for package.json parsing
The current implementation could raise unclear errors if the JSON is invalid.
def raw
return nil unless File.exist?(package_json)
- parsed_package_contents = JSON.parse(package_json_contents)
+ begin
+ parsed_package_contents = JSON.parse(package_json_contents)
+ rescue JSON::ParserError => e
+ raise ReactOnRails::Error, "Invalid JSON in #{package_json}: #{e.message}"
+ end
if parsed_package_contents.key?("dependencies") &&
parsed_package_contents["dependencies"].key?("react-on-rails")
parsed_package_contents["dependencies"]["react-on-rails"]
else
raise ReactOnRails::Error,
"No 'react-on-rails' entry in the dependencies of #{NodePackageVersion.package_json_path}, " \
"which is the expected location according to ReactOnRails.configuration.node_modules_location"
end
endLikely invalid or redundant comment.
15-17:
Consider adding parameter validation for package_json_path
The new instance method accepts any path without validation. This could lead to security issues or errors if invalid paths are provided.
Add path validation:
def self.instance(package_json_path)
+ unless package_json_path.end_with?("package.json")
+ raise ReactOnRails::Error, "Path must end with package.json"
+ end
+ path = Pathname.new(package_json_path)
+ unless path.absolute? || !path.each_filename.to_a.include?("..")
+ raise ReactOnRails::Error, "Path must be absolute or not contain '..'"
+ end
new(NodePackageVersion.new(package_json_path))
endLikely invalid or redundant comment.
26-36: 🛠️ Refactor suggestion
Improve return value consistency
The method implicitly returns nil in some cases and has no explicit return value in others. This makes the behavior unclear and could lead to bugs.
Consider making the return value explicit and consistent:
def log_if_gem_and_node_package_versions_differ
- return if node_package_version.raw.nil? || node_package_version.relative_path?
- return log_node_semver_version_warning if node_package_version.semver_wildcard?
+ return true if node_package_version.raw.nil? || node_package_version.relative_path?
+ return false if node_package_version.semver_wildcard? && log_node_semver_version_warning
node_major_minor_patch = node_package_version.major_minor_patch
gem_major_minor_patch = gem_major_minor_patch_version
versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] &&
node_major_minor_patch[1] == gem_major_minor_patch[1] &&
node_major_minor_patch[2] == gem_major_minor_patch[2]
log_differing_versions_warning unless versions_match
+ versions_match
endLikely invalid or redundant comment.
Judahmeek
force-pushed
the
judahmeek/package-path
branch
from
72c5f60 to
a64c1b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
lib/react_on_rails/version_checker.rb (1)
26-36: Improve return value consistencyThe method implicitly returns the result of
log_differing_versions_warningwhen versions don't match, but has no explicit return value when versions do match.def log_if_gem_and_node_package_versions_differ return if node_package_version.raw.nil? || node_package_version.relative_path? return log_node_semver_version_warning if node_package_version.semver_wildcard? node_major_minor_patch = node_package_version.major_minor_patch gem_major_minor_patch = gem_major_minor_patch_version versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] && node_major_minor_patch[1] == gem_major_minor_patch[1] && node_major_minor_patch[2] == gem_major_minor_patch[2] log_differing_versions_warning unless versions_match + versions_match endspec/react_on_rails/version_checker_spec.rb (2)
26-29: Consider using shared examples for common test patternsMultiple test blocks share similar structure for testing logging behavior. This could be DRYed up using RSpec shared examples.
+shared_examples "version check logging" do |scenario, expected_message| + it "handles #{scenario}" do + allow(Rails.logger).to receive(:warn) + check_version_and_log(node_package_version) + if expected_message + expect(Rails.logger).to have_received(:warn).with(expected_message) + else + expect(Rails.logger).not_to have_received(:warn) + end + end +end context "when gem and node package major and minor versions are equal" do let(:node_package_version) do double_package_version(raw: "2.2.5-beta.2", major_minor_patch: %w[2 2 5]) end before { stub_gem_version("2.2.5.beta.2") } - it "does not log" do - allow(Rails.logger).to receive(:warn) - check_version_and_log(node_package_version) - expect(Rails.logger).not_to have_received(:warn) - end + include_examples "version check logging", "matching versions", nil endAlso applies to: 40-44, 55-59, 70-74, 85-89
107-113: Add test coverage for error casesThe test coverage for non-existent package.json is minimal. Consider adding tests for file permission errors and invalid JSON.
+context "when package.json has invalid JSON" do + let(:node_package_version) do + instance_double(VersionChecker::NodePackageVersion, + package_json: "invalid.json", + package_json_contents: "invalid json") + end + + it "returns nil and logs error" do + allow(Rails.logger).to receive(:error) + expect(check_version_and_log(node_package_version)).to be_nil + expect(Rails.logger).to have_received(:error).with(/Failed to parse/) + end +end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/react_on_rails/engine.rb(1 hunks)lib/react_on_rails/version_checker.rb(4 hunks)spec/react_on_rails/version_checker_spec.rb(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/engine.rb
🔇 Additional comments (6)
lib/react_on_rails/version_checker.rb (4)
52-60: Consider renaming warning methods for clarity
The method names starting with "log_" better reflect their behavior than the previous "raise_" prefix.
80-80: Verify node_modules_location configuration
The code assumes ReactOnRails.configuration.node_modules_location is always set. Consider adding a fallback value.
106-106:
Security: Strengthen relative path detection
The current regex allows potentially dangerous path patterns that could lead to directory traversal.
- raw.match(/(\.\.|\Afile:)/).present?
+ return true if raw.nil?
+ normalized_path = File.expand_path(raw.sub(/\Afile:/, ''), Rails.root.to_s)
+ !normalized_path.start_with?(Rails.root.to_s)Likely invalid or redundant comment.
88-98: 🛠️ Refactor suggestion
Improve error handling for package.json parsing
The JSON parsing could fail with a JSON::ParserError, but this isn't handled explicitly.
def raw
return nil unless File.exist?(package_json)
- parsed_package_contents = JSON.parse(package_json_contents)
+ begin
+ parsed_package_contents = JSON.parse(package_json_contents)
+ rescue JSON::ParserError => e
+ Rails.logger.error("Failed to parse #{package_json}: #{e.message}")
+ return nil
+ end
if parsed_package_contents.key?("dependencies") &&
parsed_package_contents["dependencies"].key?("react-on-rails")
parsed_package_contents["dependencies"]["react-on-rails"]
else
msg = "No 'react-on-rails' entry in the dependencies of #{NodePackageVersion.package_json_path}, " \
"which is the expected location according to ReactOnRails.configuration.node_modules_location"
Rails.logger.warn(msg)
end
endLikely invalid or redundant comment.
spec/react_on_rails/version_checker_spec.rb (2)
237-251: LGTM: Comprehensive test coverage for yalc paths
The test cases for yalc package paths are well-structured and cover all necessary scenarios.
253-259: LGTM: Good error handling test
The test case properly verifies the graceful handling of non-existent package.json files.
| def self.instance(package_json_path) | ||
| new(NodePackageVersion.new(package_json_path)) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding parameter validation for package_json_path
The new instance method accepts a path parameter without validation. This could lead to issues if invalid paths are provided.
def self.instance(package_json_path)
+ raise ReactOnRails::Error, "package_json_path cannot be nil" if package_json_path.nil?
+ raise ReactOnRails::Error, "package_json_path must be a string" unless package_json_path.is_a?(String)
new(NodePackageVersion.new(package_json_path))
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def self.instance(package_json_path) | |
| new(NodePackageVersion.new(package_json_path)) | |
| end | |
| def self.instance(package_json_path) | |
| raise ReactOnRails::Error, "package_json_path cannot be nil" if package_json_path.nil? | |
| raise ReactOnRails::Error, "package_json_path must be a string" unless package_json_path.is_a?(String) | |
| new(NodePackageVersion.new(package_json_path)) | |
| end |
| @@ -74,20 +77,24 @@ def self.build | |||
| end | |||
|
|
|||
| def self.package_json_path | |||
| Rails.root.join("client", "package.json") | |||
| Rails.root.join(ReactOnRails.configuration.node_modules_location, "package.json") | |||
There was a problem hiding this comment.
could ReactOnRails.configuration.node_modules_location be nil and if so, what would happened?
There was a problem hiding this comment.
The default is an empty string, so it would have to be manually configured to nil.
If it is nil, that will cause a fatal error.
| def raise_if_gem_and_node_package_versions_differ | ||
| return if node_package_version.relative_path? | ||
| def log_if_gem_and_node_package_versions_differ | ||
| return if node_package_version.raw.nil? || node_package_version.relative_path? |
There was a problem hiding this comment.
return if node_package_version.raw.nil? || node_package_version.relative_path?
why?
There was a problem hiding this comment.
Because raw is nil if the package.json file doesn't exist.
Do you want us to create a warning for that?
There was a problem hiding this comment.
I just modified the code so that raw is nil if package.json or the react-on-rails dependency in the package.json is missing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
lib/react_on_rails/version_checker.rb (2)
26-36: Improve early return conditions readability.The early return conditions could be more explicit and the method could benefit from better documentation.
+# Checks if the gem and node package versions match +# Returns nil if: +# - package.json doesn't exist or react-on-rails is not in dependencies +# - the package uses a relative path def log_if_gem_and_node_package_versions_differ - return if node_package_version.raw.nil? || node_package_version.relative_path? + return if node_package_version.raw.nil? + return if node_package_version.relative_path? return log_node_semver_version_warning if node_package_version.semver_wildcard?
52-60: Consider renaming warning methods.The method names starting with "log_" are more accurate than the previous "raise_" prefixes, but they could be even more descriptive.
-def log_differing_versions_warning +def warn_version_mismatch msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}" Rails.logger.warn(msg) end -def log_node_semver_version_warning +def warn_semver_wildcard_usage msg = "**WARNING** ReactOnRails: Your node package version for react-on-rails contains a " \ "^ or ~\n#{common_error_msg}" Rails.logger.warn(msg) endspec/react_on_rails/version_checker_spec.rb (3)
26-29: DRY up repeated test patterns.The test cases follow a similar pattern and could benefit from shared examples.
+shared_examples "version check logging" do |context_description, expected_message, should_log| + it "handles logging appropriately" do + allow(Rails.logger).to receive(:warn) + check_version_and_log(node_package_version) + if should_log + expect(Rails.logger).to have_received(:warn).with(expected_message) + else + expect(Rails.logger).not_to have_received(:warn) + end + end +end context "when gem and node package major and minor versions are equal" do let(:node_package_version) do double_package_version(raw: "2.2.5-beta.2", major_minor_patch: %w[2 2 5]) end before { stub_gem_version("2.2.5.beta.2") } - it "does not log" do - allow(Rails.logger).to receive(:warn) - check_version_and_log(node_package_version) - expect(Rails.logger).not_to have_received(:warn) - end + include_examples "version check logging", + "matching versions", + nil, + false endAlso applies to: 40-44, 55-59, 70-74, 85-89
140-150: Improve test readability for build method.The test setup is complex and could be simplified for better readability.
describe "#build" do + let(:node_modules_location) { "spec/dummy" } + let(:root_package_json_path) { File.expand_path("../../package.json", __dir__) } + let(:expected_message) do + "No 'react-on-rails' entry in the dependencies of #{root_package_json_path}, " \ + "which is the expected location according to ReactOnRails.configuration.node_modules_location" + end + it "initializes NodePackageVersion with ReactOnRails.configuration.node_modules_location" do - allow(ReactOnRails).to receive_message_chain(:configuration, :node_modules_location).and_return("spec/dummy") - root_package_json_path = File.expand_path("../../package.json", __dir__) + allow(ReactOnRails).to receive_message_chain(:configuration, :node_modules_location).and_return(node_modules_location) allow(Rails).to receive_message_chain(:root, :join).and_return(root_package_json_path) - message = "No 'react-on-rails' entry in the dependencies of #{root_package_json_path}, which is " \ - "the expected location according to ReactOnRails.configuration.node_modules_location" allow(Rails.logger).to receive(:warn) described_class.build.raw - expect(Rails.logger).to have_received(:warn).with(message) + expect(Rails.logger).to have_received(:warn).with(expected_message) end end
237-251: Add test coverage for Yalc package paths.Good addition of test coverage for Yalc package paths, but consider adding edge cases.
Add tests for:
- Invalid Yalc paths
- Nested Yalc paths
- Absolute Yalc paths
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/react_on_rails/version_checker.rb(3 hunks)spec/react_on_rails/version_checker_spec.rb(9 hunks)
🔇 Additional comments (4)
lib/react_on_rails/version_checker.rb (4)
80-80:
Add nil check for node_modules_location configuration.
The code assumes ReactOnRails.configuration.node_modules_location is not nil, but this should be validated.
def self.package_json_path
+ location = ReactOnRails.configuration.node_modules_location
+ raise ReactOnRails::Error, "node_modules_location configuration cannot be nil" if location.nil?
- Rails.root.join(ReactOnRails.configuration.node_modules_location, "package.json")
+ Rails.root.join(location, "package.json")
endLikely invalid or redundant comment.
15-17: 🛠️ Refactor suggestion
Consider adding parameter validation.
The instance method accepts a path parameter without validation, which could lead to runtime errors if nil or invalid paths are provided.
def self.instance(package_json_path)
+ raise ReactOnRails::Error, "package_json_path cannot be nil" if package_json_path.nil?
+ raise ReactOnRails::Error, "package_json_path must be a string" unless package_json_path.is_a?(String)
new(NodePackageVersion.new(package_json_path))
endLikely invalid or redundant comment.
106-106:
Strengthen relative path detection.
The current regex allows potentially dangerous path patterns. Consider using a more restrictive approach.
- raw.match(/(\.\.|\Afile:)/).present?
+ return true if raw.nil?
+ raw.start_with?('file:./') || raw.start_with?('file:src/') || raw.include?('..')Likely invalid or redundant comment.
88-98: 🛠️ Refactor suggestion
Enhance error handling for JSON parsing.
The code should handle JSON parsing errors explicitly and provide more informative error messages.
def raw
if File.exist?(package_json)
- parsed_package_contents = JSON.parse(package_json_contents)
+ begin
+ parsed_package_contents = JSON.parse(package_json_contents)
+ rescue JSON::ParserError => e
+ msg = "Invalid JSON in #{package_json}: #{e.message}"
+ Rails.logger.error(msg)
+ return nil
+ end
if parsed_package_contents.key?("dependencies") &&
parsed_package_contents["dependencies"].key?("react-on-rails")
return parsed_package_contents["dependencies"]["react-on-rails"]
end
end
msg = "No 'react-on-rails' entry in the dependencies of #{NodePackageVersion.package_json_path}, " \
"which is the expected location according to ReactOnRails.configuration.node_modules_location"
Rails.logger.warn(msg)
nil
endLikely invalid or redundant comment.
| @@ -12,25 +12,28 @@ def self.build | |||
| new(NodePackageVersion.build) | |||
| end | |||
|
|
|||
| def self.instance(package_json_path) | |||
There was a problem hiding this comment.
do we need the arg?
can we just use node_modules_location ?
Judahmeek
force-pushed
the
judahmeek/package-path
branch
from
8eb2d4c to
9f4a3c1
Compare
Judahmeek
force-pushed
the
judahmeek/package-path
branch
from
9f4a3c1 to
45cb6df
Compare
Summary
Currently, the version checker is hard-coded to use
client/package.json. Ifclient/package.jsondoesn't exist, then the version checker silently fails.This PR changes the version checker to match Shakapacker v8's requirements, which are that the
package.jsonexists at the root of the project directory.Alternately, we'll need to make the package.json path configurable.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
New Features
Bug Fixes
ReactOnRails.registerStoremethod for clarity.Chores
package.jsonfiles.