Add private_output_path configuration for SSR bundles

[justin808]

Summary

• Adds new private_output_path configuration option to shakapacker for server-side rendering bundles
• Provides a dedicated path for private server bundles that shouldn't be served publicly
• Supports React on Rails' improved bundle path resolution strategy

Motivation

This feature addresses the need for better separation between public client bundles and private server-side rendering bundles. Currently, React on Rails is working on improving bundle path resolution (see feature/improve-bundle-path-resolution branch), and this shakapacker feature provides the foundational configuration support.

Changes

1. Configuration class enhancement: Added private_output_path method to Shakapacker::Configuration that joins the configured path with the root path
2. Default template update: Added documented configuration option to lib/install/config/shakapacker.yml with clear explanation of its purpose
3. Test coverage: Added spec to verify the new configuration method returns the correct path

Usage

Users can now configure a separate output directory for SSR bundles in their shakapacker.yml:

default: &default
  # Location for private output, defaults to tmp/shakapacker
  # This setting is for generating bundles that are only used in server-side rendering
  # and should not be served publicly (alternative to public_output_path)
  private_output_path: ssr-generated

Testing

• ✅ All existing tests pass
• ✅ New test added for private_output_path configuration
• ✅ RuboCop checks pass

Related Work

This supports the ongoing work in React on Rails for improving bundle path resolution and SSR bundle handling.

Target Release

8.5.0

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added support for configuring a private output directory for server-side rendering bundles, separate from the public output path. Default behavior unchanged unless configured.

• Documentation

  • Sample configuration and README updated to document the private output path option (commented by default) with example usage for SSR setups.

• Tests

  • Added test to verify correct resolution of the private output path relative to the application root.

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 21 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between a1e051c and d32df0e.

📒 Files selected for processing (5)

• CHANGELOG.md (2 hunks)
• README.md (1 hunks)
• lib/install/config/shakapacker.yml (1 hunks)
• lib/shakapacker/configuration.rb (1 hunks)
• spec/shakapacker/configuration_spec.rb (1 hunks)

Walkthrough

Adds a commented private_output_path option to the install config, implements Configuration#private_output_path to resolve that path against root, and adds a spec verifying the resolved path. Documentation and changelog entries were updated.

Changes

Cohort / File(s)	Summary
Config template update lib/install/config/shakapacker.yml	Adds a commented private_output_path: ssr-generated example under defaults; no runtime effect while commented.
Configuration accessor lib/shakapacker/configuration.rb	Adds private_output_path instance method that returns root_path.join(fetch(:private_output_path)).
Spec for new accessor spec/shakapacker/configuration_spec.rb	Adds a test asserting config.private_output_path.to_s resolves to ./test_app/ssr-generated.
Docs & changelog README.md, CHANGELOG.md	Documents new private_output_path option and updates version/changelog entries for the beta release.

Sequence Diagram(s)

sequenceDiagram
    participant Dev as Developer / Config file
    participant App as Shakapacker::Configuration
    participant FS as Filesystem (root_path)

    Dev->>App: (option) set :private_output_path = "ssr-generated"
    App->>FS: root_path.join("ssr-generated")
    FS-->>App: "/path/to/project/ssr-generated"
    App-->>Dev: returns private_output_path (Pathname)

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

• G-Rath
• tomdracz

Poem

A rabbit pads through code at night,
"A private path!" it chants with delight.
Configs whisper, specs confirm the trail,
Bundles tucked where public eyes won't hail.
Hop, hop — safe output, neat and light. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly and concisely describes the primary change by stating the addition of the private_output_path configuration for SSR bundles. It directly reflects the main feature introduced in the pull request without extraneous wording or vague terminology. A reviewer scanning the history can immediately understand the purpose of the change from this title.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

lib/install/config/shakapacker.yml (1)

32-36: Set an explicit default for private_output_path to avoid nil errors.

Comment-only docs claim a default of tmp/shakapacker, but no default is actually set. This leads to nil being joined in Configuration#private_output_path.

Apply this diff to define the default and keep the example:

-  # Location for private output, defaults to tmp/shakapacker
-  # This setting is for generating bundles that are only used in server-side rendering
-  # and should not be served publicly (alternative to public_output_path)
-  # private_output_path: ssr-generated
+  # Location for private output; default is tmp/shakapacker
+  # This setting is for generating bundles that are only used in server-side rendering
+  # and should not be served publicly (alternative to public_output_path)
+  private_output_path: tmp/shakapacker
+  # Example custom value:
+  # private_output_path: ssr-generated

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b25aa1b and 01c8e69.

📒 Files selected for processing (3)

• lib/install/config/shakapacker.yml (1 hunks)
• lib/shakapacker/configuration.rb (1 hunks)
• spec/shakapacker/configuration_spec.rb (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2024-10-09T10:50:16.512Z

Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:104-116
Timestamp: 2024-10-09T10:50:16.512Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, `error_unless_package_manager_is_obvious` is not a test method, so extracting into a shared context may not be helpful.

Applied to files:

• spec/shakapacker/configuration_spec.rb

📚 Learning: 2024-10-09T10:47:17.620Z

Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:68-89
Timestamp: 2024-10-09T10:47:17.620Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, the code within the context `"when lockfile is in Rails.root, but pwd is different"` is not duplicated for each package manager.

Applied to files:

• spec/shakapacker/configuration_spec.rb

🧬 Code graph analysis (1)

spec/shakapacker/configuration_spec.rb (1)

lib/shakapacker/configuration.rb (1)

• private_output_path (71-73)

🪛 GitHub Actions: Ruby based checks

lib/shakapacker/configuration.rb

[error] 72-72: private_output_path method encountered nil while converting to String

spec/shakapacker/configuration_spec.rb

[error] 60-60: RSpec failure in Shakapacker::Configuration: TypeError - no implicit conversion of nil into String

⏰ 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). (21)

• GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.6.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.6.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.6.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.1.x)
• GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.6.1.x)

[coderabbitai]

⚠️ Potential issue

Fix nil handling in private_output_path (pipeline failure).

fetch(:private_output_path) can be nil (no default set), causing TypeError when joining.

Apply this diff to fall back to cache_path:

-  def private_output_path
-    root_path.join(fetch(:private_output_path))
-  end
+  def private_output_path
+    root_path.join(fetch(:private_output_path) || fetch(:cache_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.

Suggested change

	def private_output_path
	root_path.join(fetch(:private_output_path))
	end
	def private_output_path
	root_path.join(fetch(:private_output_path) || fetch(:cache_path))
	end

🧰 Tools

🪛 GitHub Actions: Ruby based checks

[error] 72-72: private_output_path method encountered nil while converting to String

🤖 Prompt for AI Agents

In lib/shakapacker/configuration.rb around lines 71 to 73, private_output_path
currently calls root_path.join(fetch(:private_output_path)) which raises a
TypeError when fetch(:private_output_path) is nil; update the method to fall
back to cache_path when the fetched value is nil by using the fetched value if
present, otherwise using cache_path before joining with root_path.

[coderabbitai]

⚠️ Potential issue

Align test with actual default or set a fixture.

Current test expects ssr-generated but config doesn’t set it, causing failure. Either change the expectation to the documented default (tmp/shakapacker), or set private_output_path in the test fixture YAML.

Apply this diff to test the default:

-    it "#private_output_path returns correct path" do
-      private_output_path = File.expand_path File.join(File.dirname(__FILE__), "./test_app/ssr-generated").to_s
-
-      expect(config.private_output_path.to_s).to eq private_output_path
-    end
+    it "#private_output_path returns default cache_path when unset" do
+      private_output_path = File.expand_path File.join(File.dirname(__FILE__), "./test_app/tmp/shakapacker").to_s
+      expect(config.private_output_path.to_s).to eq private_output_path
+    end

Optionally, add a separate example to assert a custom value by stubbing:

• allow(config).to receive(:fetch).with(:private_output_path).and_return("ssr-generated")

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

Suggested change

	it "#private_output_path returns correct path" do
	private_output_path = File.expand_path File.join(File.dirname(__FILE__), "./test_app/ssr-generated").to_s
	expect(config.private_output_path.to_s).to eq private_output_path
	end
	it "#private_output_path returns default cache_path when unset" do
	private_output_path = File.expand_path File.join(File.dirname(__FILE__), "./test_app/tmp/shakapacker").to_s
	expect(config.private_output_path.to_s).to eq private_output_path
	end

🧰 Tools

🪛 GitHub Actions: Ruby based checks

[error] 60-60: RSpec failure in Shakapacker::Configuration: TypeError - no implicit conversion of nil into String

🤖 Prompt for AI Agents

In spec/shakapacker/configuration_spec.rb around lines 57 to 61, the test
expects "ssr-generated" but the configuration default is different
(tmp/shakapacker), so update the test to either assert the documented default by
changing the expected path to File.expand_path(File.join(File.dirname(__FILE__),
"./test_app/tmp/shakapacker")) or modify the test fixture to set
private_output_path to "ssr-generated" in the YAML used by this spec;
alternatively add a separate example that stubs the fetch call (allow(config).to
receive(:fetch).with(:private_output_path).and_return("ssr-generated")) and
asserts the custom value.

[claude]

Code Review for PR #592: Add private_output_path configuration for SSR bundles

Thank you for this contribution! I have reviewed the changes and overall this is a well-implemented feature that provides valuable functionality for SSR bundle management. Here is my detailed feedback:

✅ Strengths

Code Quality

• Clean implementation: The code follows the existing patterns in the Configuration class consistently
• Minimal changes: Only necessary changes were made, no unnecessary modifications
• Follows conventions: Correctly uses fetch and root_path.join pattern like other path methods

Documentation

• Comprehensive updates: README and CHANGELOG properly updated
• Clear explanations: The purpose of private_output_path is well-documented in the config template
• Good examples: Shows practical usage scenarios for React on Rails

Testing

• Test coverage added: Includes a spec that verifies the method works correctly
• Follows existing patterns: Test structure matches other configuration path tests

🔍 Observations & Suggestions

1. Default Value Handling

The implementation relies on the defaults from lib/install/config/shakapacker.yml, but there is no explicit default value defined there. The config only has a commented example. This means:

• If private_output_path is not set in user config, fetch(:private_output_path) will return nil
• root_path.join(nil) would fail with an error

Recommendation: Either:

• Add a default value in the template (like cache_path: tmp/shakapacker has one), OR
• Add fallback handling in the method: root_path.join(fetch(:private_output_path) || "tmp/shakapacker")

2. Test Configuration

The test at spec/shakapacker/configuration_spec.rb:57-60 expects the path to be ./test_app/ssr-generated, but I do not see private_output_path: ssr-generated added to spec/shakapacker/test_app/config/shakapacker.yml. This test might be passing accidentally or relying on side effects.

Recommendation: Add private_output_path: ssr-generated to the test config file for explicit testing.

3. Security Considerations

✅ No security concerns identified. The path joining is done safely using root_path.join which prevents path traversal attacks.

4. Performance

✅ No performance impact. The method follows the same lazy-loading pattern as other configuration methods.

5. Backward Compatibility

✅ Fully backward compatible. This is an additive change that does not affect existing functionality.

📝 Minor Suggestions

1. Consider validation: You might want to validate that the private_output_path does not overlap with public_output_path to prevent accidental exposure of private bundles.

2. Gemfile.lock changes: The PR includes changes to spec/dummy/Gemfile.lock that seem unrelated (sqlite3 and mini_portile2). Consider reverting these if they are not necessary for this feature.

🎯 Overall Assessment

This is a solid implementation that adds valuable functionality for SSR use cases. The main concern is the missing default value handling which should be addressed before merging. Once that is fixed, this PR is ready to go!

Great work on maintaining code quality and following the project patterns! 👍

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

CHANGELOG.md (1)

442-444: Add compare links for v9.0.0.beta.2

Please extend the footer references so readers can click through to the new beta diff, and make sure the [Unreleased] anchor now compares against that tag.

-[Unreleased]: https://github.com/shakacode/shakapacker/compare/v8.4.0...main
+[Unreleased]: https://github.com/shakacode/shakapacker/compare/v9.0.0.beta.2...main
+[v9.0.0.beta.2]: https://github.com/shakacode/shakapacker/compare/v8.4.0...v9.0.0.beta.2

README.md (1)

934-942: Clarify the resolved output path

Because Configuration#private_output_path joins the value with Rails.root, saying it “outputs to ssr-generated/” can read as if it’s relative. A quick tweak keeps the docs precise.

-private_output_path: ssr-generated # outputs to => ssr-generated/
+private_output_path: ssr-generated # resolves to => <Rails.root>/ssr-generated

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 01c8e69 and a1e051c.

⛔ Files ignored due to path filters (1)

• spec/dummy/Gemfile.lock is excluded by !**/*.lock

📒 Files selected for processing (5)

• CHANGELOG.md (1 hunks)
• README.md (1 hunks)
• lib/install/config/shakapacker.yml (1 hunks)
• lib/shakapacker/configuration.rb (1 hunks)
• spec/shakapacker/configuration_spec.rb (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• spec/shakapacker/configuration_spec.rb
• lib/shakapacker/configuration.rb
• lib/install/config/shakapacker.yml

⏰ 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). (20)

• GitHub Check: Testing (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
• GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
• GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
• GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
• GitHub Check: claude-review

[claude]

Code Review for PR #592: Add private_output_path configuration for SSR bundles

Thank you for this contribution! I've reviewed the PR and have the following feedback:

✅ Strengths

• Clean, focused implementation that follows existing configuration patterns
• Proper null handling when private_output_path is not configured
• Good documentation in README and config template
• Follows Ruby conventions and existing code style

🔍 Issues Found

1. Test Failure - Missing Configuration in Test File 🐛

The test at spec/shakapacker/configuration_spec.rb:57-61 will fail because spec/shakapacker/test_app/config/shakapacker.yml doesn't include the private_output_path configuration. The test expects "ssr-generated" but the config file doesn't define it.

Fix needed: Add private_output_path: ssr-generated to the test config file at line 9 or in the default section.

2. Inconsistent Default Behavior ⚠️

The implementation has conflicting approaches to defaults:

• lib/shakapacker/configuration.rb:71-75 returns nil when not configured
• lib/install/config/shakapacker.yml:36 sets a default value of "ssr-generated"
• The test expects the path to exist and resolve to "./test_app/ssr-generated"

Recommendation: Either:

• Remove the default from the config template (comment it out) and update the test to check for nil, OR
• Ensure the configuration class properly handles the default value

3. Documentation Clarity 📝

The config template has redundant comments on lines 32-35 that say essentially the same thing. Consider consolidating to:

# Location for private server-side bundles (e.g., for SSR)
# These bundles are not served publicly, unlike public_output_path
# private_output_path: ssr-generated

💡 Suggestions

1. Consider validation: Add a check to ensure private_output_path and public_output_path are different to prevent accidental misconfigurations

2. Migration guide: Since this is targeted for v8.5.0, consider adding a note about how existing React on Rails users should migrate their configurations

3. Edge case handling: What happens if someone tries to use private_output_path with webpack-dev-server? Should there be a warning or specific behavior?

✅ Security & Performance

• No security concerns identified
• No performance impact - configuration is loaded once at startup
• Proper path sanitization through root_path.join()

📊 Test Coverage

The test coverage looks minimal but adequate for this feature. However, you might want to add:

• A test case for when private_output_path is not configured (should return nil)
• Integration test to verify the path is actually used correctly in the build process

Verdict

The implementation is solid but needs the test configuration issue fixed before merging. Once that's addressed, this will be a valuable addition for SSR use cases.

🤖 Generated with Claude Code