-
-
Notifications
You must be signed in to change notification settings - Fork 638
[WIP] Auto-detect server bundle path from shakapacker.yml #1808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Fix fallback logic when manifest lookup fails to check multiple locations - Try environment-specific path, standard location (public/packs), then generated assets path - Return first path where bundle file actually exists - Maintains backward compatibility with original behavior as final fallback - Add comprehensive test cases to prevent regression Fixes issue where server rendering failed in test environments when bundles were in standard Shakapacker location but manifest lookup pointed to environment-specific directory. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Major improvements to bundle_js_file_path logic: **Security & Architecture:** - Server bundles (SSR/RSC) now try secure non-public locations first: - ssr-generated/ (primary) - generated/server-bundles/ (secondary) - Client bundles continue using manifest lookup as primary approach - Prevents exposure of server-side logic in public directories **Priority Order:** - SERVER BUNDLES: secure locations → manifest → legacy public locations - CLIENT BUNDLES: manifest → fallback locations (original behavior) - Fixed priority order (normal case first, edge cases second) **Code Quality:** - Extracted complex method into smaller, focused private methods - Reduced cyclomatic complexity and improved maintainability - Added comprehensive test coverage for all scenarios - Added ssr-generated/ to .gitignore **Backwards Compatibility:** - Legacy public locations still work as fallbacks - Existing client bundle behavior unchanged - Gradual migration path for server bundles This addresses the core architectural issue where server bundles were unnecessarily exposed in public directories while maintaining full compatibility with existing setups. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Introduces two new configuration options to enhance server bundle path resolution and security: - server_bundle_output_path: Configurable directory for server bundle output (defaults to "ssr-generated") - enforce_secure_server_bundles: Optional security enforcement for server bundle loading (defaults to false for backward compatibility) These options work in conjunction with the existing bundle path resolution improvements to provide better organization and security for server-side rendering assets. Features: - Secure server bundle location configuration - Backward compatibility maintained with sensible defaults - Comprehensive documentation added to configuration guide - Full parameter support in Configuration class initialization 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Changes default from "ssr-generated" to nil to avoid breaking changes - Updates documentation to reflect nil default and show example usage - Feature remains opt-in for backward compatibility - Can be changed to a sensible default in next major release 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Restores using_packer? check to handle test scenarios where packer is disabled - Combines manifest.json check with packer availability in cleaner conditional - Reverts one inline Packer assignment that breaks when Shakapacker isn't loaded - Maintains original functionality while keeping code improvements All failing tests now pass: - webpack_assets_status_checker_spec.rb (all scenarios) - utils_spec.rb "without a packer enabled" scenarios 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
… handling Since Shakapacker is required by gemspec, the using_packer? check in bundle_js_file_path is unnecessary for production code. However, tests mock this scenario for validation. Changes: - Remove using_packer? check from main bundle_js_file_path logic - Add guard check in bundle_js_file_path_with_packer for test scenarios - Maintains clean production code while handling test mocking properly - All tests pass including "without packer" scenarios This is the correct approach: main logic assumes Shakapacker is available (as guaranteed by gemspec), while method implementation handles edge cases for comprehensive test coverage. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Major Features: - Implement enforce_secure_server_bundles configuration logic - When enabled, server bundles MUST be found in private locations - Skip manifest fallback for server bundles when enforcement is active - Use configured server_bundle_output_path as additional private location Code Quality Improvements: - Extract duplicated file existence checking into find_first_existing_path helper - Use inline private_class_method declarations for better readability - Rename "secure" to "private" locations for clearer terminology - Handle Rails.root being nil in test environments - Use File.join consistently instead of Rails.root.join for compatibility Test Infrastructure: - Add comprehensive mocking for new configuration options - Fix test contexts that override mock_bundle_configs helper - Ensure all test scenarios properly mock new configuration keys - All previously failing tests now pass Security & Performance: - Private server bundle locations checked first (ssr-generated, generated/server-bundles) - Configurable server_bundle_output_path included in private location search - Enforcement prevents fallback to public manifest locations when enabled - Maintains backward compatibility with enforcement disabled by default 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Set 'ssr-generated' as default value for server_bundle_output_path - Remove hard-coded paths from try_private_server_locations - Update handle_missing_manifest_entry to respect configuration - Maintain backwards compatibility with 'generated/server-bundles' fallback - Honor enforce_secure_server_bundles flag to prevent public fallbacks Co-authored-by: Abanoub Ghadban <AbanoubGhadban@users.noreply.github.com>
- Remove enforce_secure_server_bundles from bundle resolution logic - Simplify utils.rb by removing hard-coded paths - Use server_bundle_output_path configuration directly - Add validation to ensure enforce_secure_server_bundles and server_bundle_output_path are compatible - Set server_bundle_output_path default to 'ssr-generated' - Update generator templates to use secure server bundle configuration - Update tests to match new implementation Co-authored-by: Abanoub Ghadban <AbanoubGhadban@users.noreply.github.com>
- Fix RuboCop violations: - Use string interpolation instead of concatenation in base_generator.rb - Apply guard clause pattern in configuration.rb - Remove extra blank line in utils.rb - Fix configuration validation tests: - Add Rails.root availability check to prevent nil errors in tests - Mock Rails.root in test specs for path validation - Fix utils tests: - Use default 'ssr-generated' path instead of nil in mock_bundle_configs - Update test expectations to match new server bundle path behavior - Remove outdated test expecting server bundles in public/packs Co-authored-by: Abanoub Ghadban <AbanoubGhadban@users.noreply.github.com>
Set server_bundle_output_path to nil and enforce_secure_server_bundles to false in the dummy app's React on Rails configuration. This ensures the dummy app uses the standard webpack output location (public/webpack/test) where bundles are actually built during tests, resolving the CI failures. Co-authored-by: Abanoub Ghadban <AbanoubGhadban@users.noreply.github.com>
…t existence check When server_bundle_output_path is configured, the bundle path resolution now returns the configured path immediately without checking if the file exists. This ensures predictable behavior and allows the server rendering process to handle missing files appropriately. Updates: - Modified bundle_js_file_path_with_packer to return configured path directly - Removed try_server_bundle_output_path method (no longer needed) - Updated tests to reflect new behavior without File.exist? checks Co-authored-by: Abanoub Ghadban <AbanoubGhadban@users.noreply.github.com>
- Fix RuboCop Layout/EmptyLines violation in lib/react_on_rails/utils.rb - Fix 6 failing tests that expect manifest paths for server bundles - Tests now properly clear server_bundle_output_path when testing manifest fallback behavior Co-authored-by: Abanoub Ghadban <AbanoubGhadban@users.noreply.github.com>
- Renamed enforce_secure_server_bundles to enforce_private_server_bundles - Updated validation method name to validate_enforce_private_server_bundles - Updated all comments, test descriptions, and documentation to use 'private' instead of 'secure' - Updated configuration files, templates, and specs to reflect new terminology This change provides clearer terminology - 'private' better describes non-public directories vs 'secure' which has broader security implications Co-authored-by: Justin Gordon <justin808@users.noreply.github.com>
- Remove call to undefined ReactOnRails::PackerUtils.using_packer? method - Update test mocks to use Shakapacker directly instead of packer abstraction - Fix Shakapacker::Manifest::MissingEntryError reference in tests The using_packer? method was removed in a recent refactor that eliminated the unnecessary packer abstraction layer. This commit aligns the code with those changes. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ets_full_path - Add new `public_assets_full_path` method to be explicit about public asset paths - Keep `generated_assets_full_path` as backwards-compatible deprecated alias - Update internal references to use clearer `public_assets_full_path` method - Addresses naming ambiguity concern about whether paths are public or private - Prepares for future evolution toward simplified public-only asset resolution This improvement clarifies the distinction between: - Public asset paths (client bundles, manifests) → public_assets_full_path - Private asset paths (server bundles with enforcement) → server_bundle_private_path 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…_assets_full_path - Rename public_assets_full_path → public_bundles_full_path for specificity - "public_bundles" is clearer than "public_assets" in Rails context - Avoids confusion with general Rails public assets (images, stylesheets, etc.) - Method specifically handles webpack bundles, not general assets - Update all internal references to use the more specific naming - Maintain backwards compatibility through generated_assets_full_path alias This creates clear semantic distinction: - public_bundles_full_path → webpack bundles in public directories - server_bundle_private_path → server bundles in private directories 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace all instances of generated_assets_full_path with public_bundles_full_path in utils_spec.rb - Ensures consistency in method naming across test cases - Aligns with recent changes for clearer asset path handling This change enhances clarity and maintains alignment with the updated method naming conventions.
…or server and client bundles - Added detailed comments in configuration.md to explain server bundle output path and security measures for loading server bundles. - Introduced examples for organizing client and server assets to improve clarity for users. - Updated comments in base_generator.rb and react_with_redux_generator.rb to reflect changes from 'auto-registration' to 'auto-bundling' terminology for consistency. These changes aim to improve the understanding of asset management and security practices within the React on Rails framework.
…hancements - Added a breaking change note regarding the removal of `generated_assets_dirs` configuration, emphasizing the transition to Shakapacker for asset path management. - Updated documentation in configuration.md to clarify the purpose of `components_subdirectory` for automatic component registration. - Refactored version constants for Shakapacker compatibility, ensuring consistent terminology for auto-bundling features. These changes improve clarity and guide users through the updated configuration requirements.
- Updated the logic for determining server bundle paths to include a fallback mechanism that checks for the existence of paths based on the `enforce_private_server_bundles` configuration. - This change ensures that if the enforcement is not active, the system will attempt to return the first valid path from a list of candidate paths, improving robustness in asset management. This update aligns with recent changes to clarify the handling of server bundles and enhances the overall functionality of the asset resolution process.
- Modified the candidate paths logic to include the public output path from PackerUtils when `enforce_private_server_bundles` is not active. - This change enhances the flexibility of server bundle path resolution, ensuring that the system can effectively locate bundles in the public directory when appropriate. This update builds on previous enhancements to improve asset management and aligns with the ongoing refactoring efforts in the project.
- Fixed Rails.root nil issue in configuration tests by adding mock - Updated all remaining ReactOnRails::PackerUtils.packer references to use Shakapacker directly - Fixed packer method calls in utils_spec.rb, packer_utils_spec.rb, and webpack_assets_status_checker_spec.rb - Replaced dynamic constant assignment with stub_const for better test isolation This completes the migration from the removed packer method to direct Shakapacker usage in tests. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ndles - Fix configuration test path inconsistencies by using consistent "public/packs" path - Add missing enforce_private_server_bundles mock to all server bundle tests - Update tests expecting no file existence check to set enforce_private_server_bundles=true - Fix RSC bundle tests to include required configuration mocks This resolves CI failures caused by recent commits that added fallback logic and enforce_private_server_bundles configuration option. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Update tests to properly reflect the new server bundle path resolution logic: - When enforce_private_server_bundles=false, both private and public paths are checked - Manifest lookup is attempted first, then fallback to candidate paths logic - Mock both File.exist? calls (private and public paths) in fallback scenarios - Use proper mock_missing_manifest_entry for consistent test behavior This follows the correct approach (#2) of updating test expectations to match the actual code behavior rather than forcing different execution paths. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Add explanatory comments about server bundle path configuration options: - Shakapacker v8.5.0+ can auto-detect from shakapacker.yml - Fallback to explicit config for older versions - Note about potential deprecation in v17 TODO: Implement automatic detection from shakapacker.yml when available See: https://github.com/shakacode/shakapacker/issues/xyz 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
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: Auto-detect server bundle path from shakapacker.ymlThank you for this PR! I've thoroughly reviewed the changes for automatic detection of server bundle paths from shakapacker.yml configuration. Here's my comprehensive feedback: ✅ Strengths
🚨 Critical Issues1. Incomplete Shakapacker v8.5.0+ IntegrationThe PR description mentions "automatic detection from shakapacker.yml when available" for v8.5.0+, but the implementation appears incomplete:
Recommendation: Implement the auto-detection logic or update the PR description to reflect the current scope. 2. Path Resolution Logic ComplexityThe # Lines 88-108 handle multiple scenarios with overlapping logicRecommendation: Consider refactoring into smaller, focused methods for clarity.
|
|
Closing WIP PR (opened September 23, 6+ weeks old). This was marked as WIP and hasn't been updated. If auto-detection of server bundle path from shakapacker.yml is still needed, please:
Closing to keep PR list clean. Feel free to reopen or create a new PR if this work should continue. |
✅ Closing this PR - Core Features Already MergedAfter thorough analysis, I'm closing this PR because the substantive changes have already been merged to master through PRs #1798 and #1815. What's Already on Master✅ Merged via PR #1798 (Sept 24, 2025): "Fix bundle path resolution and improve server bundle security"
✅ Merged via PR #1815 (Sept 24, 2025): "Improve server bundle security test coverage"
What's Missing (And Why)❌ Shakapacker.yml auto-detection - This feature was listed in the PR description but never actually implemented. Moreover, after reviewing Shakapacker's configuration structure, this feature may not be feasible as originally conceived:
The Real SituationThis PR branch contains the entire React on Rails history (1812 commits from "First Commit" forward), making it unrebaseable onto current master. The valuable code changes were already cherry-picked and merged through the PRs mentioned above. What About the TODO Comment?Master currently has this TODO: # TODO: Add automatic detection of server_bundle_output_path from shakapacker.yml
# See feature/shakapacker-yml-integration branch for implementation
# Requires Shakapacker v8.5.0+ and semantic version checkingRecommendation: Remove this TODO since:
Next Steps
The server bundle security feature is working great on master! 🎉 🤖 Generated with Claude Code |
The TODO referenced automatic detection of server_bundle_output_path from shakapacker.yml, but this feature was never implemented and may not be feasible as originally conceived. Why this TODO is being removed: 1. The referenced feature/shakapacker-yml-integration branch did not contain the implementation - it only had the foundational work which was already merged via PRs #1798 and #1815 2. Shakapacker.yml's public_output_path serves a different purpose than React on Rails' server_bundle_output_path (public webpack output vs private server bundle security) 3. The current configuration approach is working well The server bundle security features (server_bundle_output_path and enforce_private_server_bundles) are fully implemented and tested on master. Closes #1808 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## Summary Removes the TODO comment about automatic detection of `server_bundle_output_path` from shakapacker.yml, as this feature was never implemented and may not be feasible. ## Background PR #1808 was titled "Auto-detect server bundle path from shakapacker.yml" and contained a TODO comment referencing this feature. However: 1. **The feature was never actually implemented** - the PR branch contained the foundational work (server bundle security features), which was already merged via PRs #1798 and #1815 2. **The concept may not be feasible** - Shakapacker's `public_output_path` serves a different purpose than React on Rails' `server_bundle_output_path`: - `public_output_path`: Where webpack outputs ALL bundles (public directory) - `server_bundle_output_path`: Private server bundles OUTSIDE public directory for security ## What's Already on Master The server bundle security features are fully implemented and working: - ✅ `server_bundle_output_path` configuration option - ✅ `enforce_private_server_bundles` security enforcement - ✅ Private server bundle path resolution logic - ✅ Comprehensive test coverage ## Changes - Remove misleading TODO comment from `lib/react_on_rails/configuration.rb` - Closes #1808 (which has been separately closed with detailed explanation) ## Testing - ✅ RuboCop passes - ✅ No functional changes - only removing a comment --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- Reviewable:start --> - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/shakacode/react_on_rails/1962) <!-- Reviewable:end --> Co-authored-by: Claude <noreply@anthropic.com>
Adds clear documentation and automated validation to prevent configuration mismatches between webpack and React on Rails configurations. **Documentation Improvements:** - Add prominent warnings in both webpack config and Rails initializer templates - Clarify that both server_bundle_output_path settings must match - Explain the security rationale (private directories prevent code exposure) - Update generator templates with cross-reference comments **Doctor Validation:** - Add automated check that compares webpack serverWebpackConfig.js output.path with React on Rails server_bundle_output_path configuration - Display success message when configs are in sync - Provide detailed warning with fix instructions when mismatch detected - Gracefully handles missing config files or parsing errors **Why This Matters:** Webpack (build-time) and React on Rails (runtime) need to agree on where server bundles are located. Misconfiguration causes SSR failures that can be hard to debug. This change makes the requirement explicit and adds automated validation via the doctor command. **Technical Notes:** - Validation runs as part of `rails react_on_rails:doctor` analysis - Parses webpack config to extract output.path value - Compares paths relative to Rails.root - No breaking changes - pure documentation and tooling improvement Related to closed PR #1808 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Adds clear documentation and automated validation to prevent configuration mismatches between webpack and React on Rails configurations. **Documentation Improvements:** - Add prominent warnings in both webpack config and Rails initializer templates - Clarify that both server_bundle_output_path settings must match - Explain the security rationale (private directories prevent code exposure) - Update generator templates with cross-reference comments **Doctor Validation:** - Add automated check that compares webpack serverWebpackConfig.js output.path with React on Rails server_bundle_output_path configuration - Display success message when configs are in sync - Provide detailed warning with fix instructions when mismatch detected - Gracefully handles missing config files or parsing errors **Why This Matters:** Webpack (build-time) and React on Rails (runtime) need to agree on where server bundles are located. Misconfiguration causes SSR failures that can be hard to debug. This change makes the requirement explicit and adds automated validation via the doctor command. **Technical Notes:** - Validation runs as part of `rails react_on_rails:doctor` analysis - Parses webpack config to extract output.path value - Compares paths relative to Rails.root - No breaking changes - pure documentation and tooling improvement Related to closed PR #1808 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🚧 Work In Progress
This PR introduces automatic detection of
server_bundle_output_pathfrom shakapacker.yml configuration to standardize all webpack-related configuration in one place.🎯 Goals
📋 Implementation Plan
🔗 Dependencies
🎨 Benefits
Note: This enhancement builds on the secure bundle path resolution features. The core functionality works independently of this enhancement.
🤖 Generated with Claude Code
This change is