Improve server bundle security test coverage and fix misleading comments #1815
Conversation
This commit addresses critical gaps in test coverage and terminology issues found in the server bundle security implementation: ## Test Coverage Improvements - **Added comprehensive tests for enforce_private_server_bundles=true**: - Tests for bundle_js_file_path() with enforcement enabled - Tests for server_bundle_js_file_path() with enforcement enabled - Tests for rsc_bundle_js_file_path() with enforcement enabled - Tests verify that public directories are never checked when enforcement is enabled - **Enhanced existing test scenarios**: - Added tests for all file existence combinations when enforcement is disabled - Added proper fallback behavior tests (private -> public -> configured path) - Improved test organization with clearer context descriptions ## Bug Fixes and Code Quality - **Fixed misleading terminology**: - Changed "auto-registration" to "auto-bundling" in comments and method names - Updated method name: update_gitignore_for_auto_registration -> update_gitignore_for_generated_bundles - **Improved test reliability**: - Added proper mocking for all configuration dependencies - Used parametrized tests for comprehensive file state coverage - Added clear expectations that File.exist? should not be called on public paths when enforcement is enabled ## Security Testing The new tests ensure that the critical security feature `enforce_private_server_bundles=true` works as intended: - Server bundles are never loaded from public directories when enforcement is enabled - RSC bundles follow the same security model as server bundles - Fallback behavior is properly disabled when security enforcement is active All tests pass with 0 failures. RuboCop linting passes with 0 violations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
WalkthroughPublic generator method renamed to update_gitignore_for_generated_bundles. Generator comments updated from “auto registration” to “auto-bundling.” Tests add broader coverage for bundle path resolution with enforce_private_server_bundles toggles, covering server and RSC bundles under various existence scenarios. One spec comment adjusted. No functional logic changes beyond the method rename. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review for PR #1815✅ Overall AssessmentThis PR makes important improvements to test coverage and code clarity. The changes are well-focused and address critical gaps in testing the security features introduced in PR #1798. 🎯 Strengths
🔍 Areas for Consideration
💡 Suggestions
✅ No Issues Found
📝 Minor Observations
✅ Approval RecommendationThis PR significantly improves the test coverage for a critical security feature and clarifies misleading terminology. The changes are backward compatible and well-tested. I recommend approving this PR after the minor considerations above are acknowledged. Great work on identifying and addressing these gaps in test coverage! The security feature is now properly validated. 🔒 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/react_on_rails/utils_spec.rb (1)
353-372: Clarify example description to match behaviorThe “without checking file existence” phrasing is misleading; the spec stubs File.exist? and expects the configured path when neither exists. Update the description.
- it "returns the configured path directly without checking file existence" do + it "returns the configured path when neither private nor public paths exist" do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/generators/react_on_rails/base_generator.rb(1 hunks)lib/generators/react_on_rails/react_no_redux_generator.rb(1 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(1 hunks)spec/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rb(1 hunks)spec/react_on_rails/utils_spec.rb(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}: Before every commit/push, runbundle exec rubocopand fix all violations in Ruby code
Let RuboCop handle all Ruby code formatting; never manually format Ruby files
Files:
lib/generators/react_on_rails/react_with_redux_generator.rbspec/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rblib/generators/react_on_rails/base_generator.rblib/generators/react_on_rails/react_no_redux_generator.rbspec/react_on_rails/utils_spec.rb
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
lib/generators/react_on_rails/react_with_redux_generator.rbspec/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rblib/generators/react_on_rails/base_generator.rblib/generators/react_on_rails/react_no_redux_generator.rbspec/react_on_rails/utils_spec.rb
🧠 Learnings (3)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/react_no_redux_generator.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/react_no_redux_generator.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
spec/react_on_rails/utils_spec.rb
🧬 Code graph analysis (1)
spec/react_on_rails/utils_spec.rb (2)
lib/react_on_rails/packer_utils.rb (1)
packer_public_output_path(100-102)lib/react_on_rails/utils.rb (3)
bundle_js_file_path(74-86)server_bundle_js_file_path(141-146)rsc_bundle_js_file_path(148-153)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
🔇 Additional comments (14)
lib/generators/react_on_rails/react_with_redux_generator.rb (1)
71-74: Terminology update LGTMComment aligns with “auto-bundling” terminology. No behavior changes.
spec/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rb (1)
5-5: Terminology update LGTMComment matches project-wide “auto-bundling” term.
lib/generators/react_on_rails/react_no_redux_generator.rb (1)
40-43: Terminology update LGTMComment-only change; behavior preserved.
spec/react_on_rails/utils_spec.rb (10)
149-176: Server bundle (enforcement off): path selection tests LGTMCovers private-preferred, public fallback, and configured-path return. Clear stubbing for both locations.
178-185: Server bundle: both-missing case LGTMCorrect expectation to return configured private path when neither exists.
187-220: Server bundle (enforcement on): exhaustive combinations LGTMGood parameterization to assert invariant preference for private path.
242-254: RSC bundle (enforcement off): setup LGTMMocks and config isolate RSC behavior cleanly.
256-263: RSC bundle: prefer private without checking public when present LGTMThe negative expectation on File.exist?(public_path) is valuable.
264-278: RSC bundle: public fallback and both-missing cases LGTMCovers key branches with clear expectations.
281-301: RSC bundle (enforcement on): enforce-private behavior LGTMEnsures no public checks and returns private path.
374-392: Server bundle (enforcement on): no public check LGTMGood negative expectation and assertion of private path.
456-466: RSC path (enforcement off): private path expectation LGTMMatches server-bundle parity.
468-487: RSC path (enforcement on): no public checks LGTMEnsures enforcement applies to RSC as well.
lib/generators/react_on_rails/base_generator.rb (1)
98-115: Public method rename: add deprecation alias and verify call sitesAdd a backward-compatible alias for the old public name that warns and forwards to the new method; verify no remaining references to the old name.
def update_gitignore_for_generated_bundles gitignore_path = File.join(destination_root, ".gitignore") return unless File.exist?(gitignore_path) @@ end end + +# Temporary backward-compat alias for the previous public API name. +# Remove in a future major release. +def update_gitignore_for_auto_registration + if defined?(ActiveSupport::Deprecation) + ActiveSupport::Deprecation.warn( + "update_gitignore_for_auto_registration is deprecated; " \ + "use update_gitignore_for_generated_bundles" + ) + else + warn("DEPRECATION: update_gitignore_for_auto_registration -> update_gitignore_for_generated_bundles") + end + update_gitignore_for_generated_bundles +endRun this to find any remaining callers (avoid the restrictive -g filters that skipped files earlier):
#!/bin/bash set -euo pipefail rg -n --hidden -uu '\bupdate_gitignore_for_auto_registration\b' || true rg -n --hidden -uu '\bupdate_gitignore_for_generated_bundles\b' || true
Add entries for changes since version 16.1.0 release: - Bug fix for React Server Components manifest file resolution (#1818) - Documentation for monorepo merger plan (#1817) - Improved server bundle security test coverage (#1815) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Summary
This PR addresses critical gaps in test coverage and terminology issues found in the server bundle security implementation from PR #1798. The changes focus on improving test quality and code maintainability without altering the actual functionality.
🧪 Test Coverage Improvements
enforce_private_server_bundles=true- the main security feature was previously untested🐛 Code Quality Fixes
update_gitignore_for_auto_registration→update_gitignore_for_generated_bundles🔒 Security Testing
The new tests ensure that the critical security feature
enforce_private_server_bundles=trueworks correctly:🚦 Test Results
📝 Files Changed
lib/generators/react_on_rails/base_generator.rb- Method name fixlib/generators/react_on_rails/react_*_generator.rb- Comment terminology fixesspec/react_on_rails/utils_spec.rb- Major test coverage expansionspec/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rb- Comment fixWhy This Matters
The original implementation in PR #1798 was solid, but the tests only covered
enforce_private_server_bundles=falsescenarios. This left the main security feature completely untested, which could have led to undetected regressions. These improvements ensure the server bundle security functionality is properly validated according to software engineering best practices.🤖 Generated with Claude Code
This change is
Summary by CodeRabbit