[WIP] Auto-detect server bundle path from shakapacker.yml #1808

justin808 · 2025-09-23T23:10:25Z

🚧 Work In Progress

This PR introduces automatic detection of server_bundle_output_path from shakapacker.yml configuration to standardize all webpack-related configuration in one place.

🎯 Goals

Eliminate Configuration Duplication: Remove the need to specify server bundle output path in both webpack config and React on Rails config
Single Source of Truth: All webpack-related configuration centralized in shakapacker.yml
Improved Developer Experience: Generator can auto-detect correct paths during installation
Version-Aware Integration: Graceful degradation for older Shakapacker versions

📋 Implementation Plan

Add semantic version checking for Shakapacker v8.5.0+
Implement auto-detection from shakapacker.yml when available
Update generator to read from shakapacker.yml during installation
Add fallback to explicit configuration for older versions
Update documentation and examples
Add comprehensive tests for version compatibility

🔗 Dependencies

Requires Shakapacker v8.5.0+ for automatic detection
Falls back to manual configuration for older versions
Related to bundle path resolution improvements in #[PR_NUMBER]

🎨 Benefits

Developers: Less configuration to maintain
Teams: Reduced chance of webpack/Rails config getting out of sync
Maintainers: Simplified configuration story

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

- 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 <[email protected]>

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 <[email protected]>

Co-authored-by: Copilot <[email protected]>

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 <[email protected]>

- 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 <[email protected]>

- 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 <[email protected]>

… 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 <[email protected]>

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 <[email protected]>

- 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 <[email protected]>

- 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 <[email protected]>

- 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 <[email protected]>

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 <[email protected]>

…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 <[email protected]>

- 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 <[email protected]>

- 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 <[email protected]>

- 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 <[email protected]>

…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 <[email protected]>

…_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 <[email protected]>

- 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.

… is enabled

- 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 <[email protected]>

…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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

coderabbitai · 2025-09-23T23:10:33Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feature/shakapacker-yml-integration

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

claude · 2025-09-23T23:13:06Z

🔍 Code Review: Auto-detect server bundle path from shakapacker.yml

Thank 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

Well-structured approach: The separation of server and client bundles with configurable paths is a solid architectural improvement
Backward compatibility: Good handling of legacy configurations with deprecation warnings
Security focus: The enforce_private_server_bundles option adds valuable security controls
Comprehensive test coverage: The spec files adequately test the new functionality

🚨 Critical Issues

1. Incomplete Shakapacker v8.5.0+ Integration

The PR description mentions "automatic detection from shakapacker.yml when available" for v8.5.0+, but the implementation appears incomplete:

No code for reading server_bundle_output_path from shakapacker.yml
Missing semantic version checking for Shakapacker v8.5.0+
No fallback mechanism for older versions

Recommendation: Implement the auto-detection logic or update the PR description to reflect the current scope.

2. Path Resolution Logic Complexity

The bundle_js_file_path_with_packer method in utils.rb has become complex with nested conditions:

# Lines 88-108 handle multiple scenarios with overlapping logic

Recommendation: Consider refactoring into smaller, focused methods for clarity.

⚠️ Potential Bugs

1. File Existence Check Inconsistency

In bundle_js_file_path_with_packer, when enforce_private_server_bundles is false:

if config.enforce_private_server_bundles || File.exist?(private_server_bundle_path)
  return private_server_bundle_path
end

This returns the path even if the file doesn't exist when enforcement is enabled, which could cause runtime errors.

2. Rails.root Nil Handling

root_path = Rails.root || "."

Using "." as fallback might not work correctly in all contexts. Consider a more robust fallback or raising an error.

🔧 Code Quality Improvements

Method Naming: The rename from generated_assets_full_path to public_bundles_full_path is excellent for clarity
Constants Organization: Consider extracting magic strings like "ssr-generated" into constants
Documentation: The configuration guide updates are helpful, but consider adding migration guide for users upgrading from older versions

🔒 Security Considerations

✅ Good security practices observed:

Private directory enforcement for server bundles
Path validation to prevent public directory exposure
Clear separation of client and server assets

Suggestion: Add logging when server bundles are loaded from fallback locations in non-enforced mode for security auditing.

📊 Performance Considerations

File.exist? calls: Multiple existence checks in hot paths could impact performance. Consider caching results or restructuring the logic.
Path resolution: The current implementation performs multiple path expansions. Consider memoizing expanded paths.

🧪 Test Coverage

✅ Good test coverage for:

Configuration validation
Path resolution logic
Backward compatibility

Missing tests for:

Shakapacker v8.5.0+ auto-detection (when implemented)
Edge cases with symlinks or relative paths
Concurrent access scenarios

📝 Minor Issues

Gitignore: Good addition of ssr-generated to .gitignore
Generator updates: Properly updated to handle new directory structure
CHANGELOG: Well-documented breaking changes and new features

🎯 Recommendations

Priority 1: Complete the Shakapacker v8.5.0+ auto-detection feature or update PR scope
Priority 2: Refactor complex path resolution logic for maintainability
Priority 3: Add missing test cases for edge scenarios
Priority 4: Consider adding debug logging for path resolution

💬 Questions

Is the plan to read server_bundle_output_path directly from shakapacker.yml in a follow-up PR?
Should we add a rake task to help users migrate from generated_assets_dirs to the new configuration?
Have you tested this with actual Shakapacker v8.5.0 when it's released?

Overall, this is a valuable improvement to the configuration system. The main concern is the incomplete Shakapacker v8.5.0 integration mentioned in the PR description. Once that's addressed (either by implementation or scope clarification), this will be a great addition to React on Rails!

Let me know if you'd like me to clarify any points or help with the implementation of the missing features.

🤖 Generated with Claude Code

justin808 and others added 30 commits September 23, 2025 14:07

Apply suggestions from code review

7aff6da

Co-authored-by: Copilot <[email protected]>

Update Gemfile.lock and CHANGELOG.md

b0c4963

update changelog.md

60d498d

Don't fall back to public directory if enforce_private_server_bundles…

ffa58ac

… is enabled

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[WIP] Auto-detect server bundle path from shakapacker.yml #1808

[WIP] Auto-detect server bundle path from shakapacker.yml #1808

Uh oh!

justin808 commented Sep 23, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Sep 23, 2025

Review skipped

Uh oh!

claude bot commented Sep 23, 2025

Uh oh!

Uh oh!

Uh oh!

[WIP] Auto-detect server bundle path from shakapacker.yml #1808

Are you sure you want to change the base?

[WIP] Auto-detect server bundle path from shakapacker.yml #1808

Uh oh!

Conversation

justin808 commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚧 Work In Progress

🎯 Goals

📋 Implementation Plan

🔗 Dependencies

🎨 Benefits

Uh oh!

coderabbitai bot commented Sep 23, 2025

Review skipped

Uh oh!

claude bot commented Sep 23, 2025

🔍 Code Review: Auto-detect server bundle path from shakapacker.yml

✅ Strengths

🚨 Critical Issues

1. Incomplete Shakapacker v8.5.0+ Integration

2. Path Resolution Logic Complexity

⚠️ Potential Bugs

1. File Existence Check Inconsistency

2. Rails.root Nil Handling

🔧 Code Quality Improvements

🔒 Security Considerations

📊 Performance Considerations

🧪 Test Coverage

📝 Minor Issues

🎯 Recommendations

💬 Questions

Uh oh!

Uh oh!

justin808 commented Sep 23, 2025 •

edited

Loading