Rename bundlePath to serverBundleCachePath in node renderer

[justin808]

Summary

This PR renames the bundlePath configuration option to serverBundleCachePath in the node renderer to better describe its purpose and avoid confusion with Shakapacker's public bundle path.

Changes

• Config interface: Added serverBundleCachePath as the new property, deprecated bundlePath
• Deprecation handling: Added warning messages when the old name is used (both config and env var)
• Environment variables: Support both RENDERER_SERVER_BUNDLE_CACHE_PATH (new) and RENDERER_BUNDLE_PATH (deprecated)
• Internal code: Updated all internal code to use serverBundleCachePath
• Documentation: Updated all docs and examples to use the new name
• Tests: Updated test files and helper functions to use the new name
• Backwards compatibility: The old bundlePath option continues to work with deprecation warnings

Why this change?

The name bundlePath was confusing because:

1. It could be confused with Shakapacker's public bundle path (where client-side assets are served)
2. It didn't clearly indicate that this is a cache directory for uploaded server bundles
3. The new name serverBundleCachePath makes the purpose explicit

Migration guide

Users can update their configuration:

Before:

const config = {
  bundlePath: path.resolve(__dirname, './.node-renderer-bundles'),
};

After:

const config = {
  serverBundleCachePath: path.resolve(__dirname, './.node-renderer-bundles'),
};

Environment variable:

• Old: RENDERER_BUNDLE_PATH
• New: RENDERER_SERVER_BUNDLE_CACHE_PATH

Both the old config option and env var will continue to work with deprecation warnings.

Test plan

• ESLint passes
• RuboCop passes
• All documentation updated
• Backwards compatibility maintained

🤖 Generated with Claude Code

---

This change is 

[github-actions]

👋 Thanks for your contribution!

This PR will run CI tests based on the files you changed. If some tests are skipped and you want to run the full test suite (including minimum dependency tests), you can use these commands:

CI Control Commands

• /run-skipped-ci - Runs all skipped CI checks and enables full CI mode for this PR

  • Adds the full-ci label to ensure future commits also run the full test suite
  • Useful when you want comprehensive testing across all configurations

• /stop-run-skipped-ci - Disables full CI mode and returns to standard CI

  • Removes the full-ci label
  • Future commits will only run tests for changed files

Note:

• These commands require write access to the repository
• The full-ci label is preserved on merged PRs as a historical record

View CI progress in the Actions tab.

[github-actions]

👋 Thanks for opening this PR!

🚀 Running Full CI Suite

By default, PRs run a subset of CI jobs for faster feedback (latest Ruby/Node versions only).

To run the complete CI suite including all dependency combinations and skipped jobs, comment:

/run-skipped-ci

This will trigger:

• ✅ Minimum supported versions (Ruby 3.2, Node 20)
• ✅ All example app tests
• ✅ Pro package integration tests
• ✅ All test matrices

The full CI suite takes longer but ensures compatibility across all supported versions before merging.

[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 2 minutes and 43 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 d94259f and 9713fcf.

📒 Files selected for processing (12)

• react_on_rails_pro/CHANGELOG.md (7 hunks)
• react_on_rails_pro/docs/installation.md (7 hunks)
• react_on_rails_pro/docs/node-renderer/basics.md (2 hunks)
• react_on_rails_pro/docs/node-renderer/js-configuration.md (2 hunks)
• react_on_rails_pro/packages/node-renderer/src/shared/configBuilder.ts (5 hunks)
• react_on_rails_pro/packages/node-renderer/src/shared/utils.ts (1 hunks)
• react_on_rails_pro/packages/node-renderer/src/worker.ts (2 hunks)
• react_on_rails_pro/packages/node-renderer/src/worker/vm.ts (3 hunks)
• react_on_rails_pro/packages/node-renderer/tests/helper.ts (4 hunks)
• react_on_rails_pro/packages/node-renderer/tests/testingNodeRendererConfigs.js (1 hunks)
• react_on_rails_pro/packages/node-renderer/tests/worker.test.ts (18 hunks)
• react_on_rails_pro/spec/dummy/client/node-renderer.js (1 hunks)

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch justin808/rename-bundle-path

---

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

[claude]

Code Review

This PR renames bundlePath to serverBundleCachePath to better describe its purpose. Overall, this is a well-executed refactoring with good backwards compatibility. However, I found one critical bug that needs to be fixed before merging.

---

🐛 Critical Bug

worker.ts still uses the old property name

File: packages/node-renderer/src/worker.ts:121

The code is destructuring bundlePath from getConfig():

const { bundlePath, logHttpLevel, port, fastifyServerOptions, workersCount } = getConfig();

This should be changed to serverBundleCachePath:

const { serverBundleCachePath, logHttpLevel, port, fastifyServerOptions, workersCount } = getConfig();

And the usage on line 154 should also be updated:

const destinationPath = path.join(serverBundleCachePath, 'uploads', part.filename);

Impact: This will break at runtime since bundlePath is now deprecated and optional in the Config interface. The code will receive undefined when users only set serverBundleCachePath.

---

✅ Strengths

1. Excellent backwards compatibility:

   • Deprecation warnings are clear and helpful
   • Old config option continues to work
   • Old environment variable RENDERER_BUNDLE_PATH falls back gracefully
   • Fallback logic in configBuilder.ts:252-265 is well-implemented

2. Good deprecation messaging:

   • Messages clearly explain what to use instead
   • Helpful context about the difference from Shakapacker's bundle path

3. Comprehensive documentation updates:

   • All docs and examples updated to use new name
   • Migration guide provided in PR description

4. Thorough test updates:

   • All test files updated to use serverBundleCachePath
   • Test helpers renamed consistently

5. Smart environment variable handling:

   • Priority: RENDERER_SERVER_BUNDLE_CACHE_PATH > RENDERER_BUNDLE_PATH > default
   • Deprecation warning only shown when old var is used without new one

---

💡 Minor Suggestions

1. Add test coverage for deprecation warnings

Consider adding tests to verify:

• Warning is logged when bundlePath config is used
• Warning is logged when RENDERER_BUNDLE_PATH env var is used (without RENDERER_SERVER_BUNDLE_CACHE_PATH)
• serverBundleCachePath takes precedence when both are set

Example test:

test('logs deprecation warning when bundlePath is used', () => {
  const warnSpy = jest.spyOn(log, 'warn');
  buildConfig({ bundlePath: '/tmp/test' });
  expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('bundlePath is deprecated'));
});

2. Consider the deprecation timeline

The Config interface marks bundlePath as @deprecated, which is good. Consider:

• Adding a comment about when it will be removed (e.g., "Will be removed in v5.0")
• Updating CHANGELOG.md with this deprecation (if not already done)

3. Variable naming in vm.ts

The bundlePath parameter names in vm.ts are fine since they're local variables, not config properties. However, for consistency, you might consider renaming:

• hasVMContextForBundle(bundlePath: string) → hasVMContextForBundle(bundleFilePath: string)
• getVMContext(bundlePath: string) → getVMContext(bundleFilePath: string)
• removeVM(bundlePath: string) → removeVM(bundleFilePath: string)

This would make it clearer they refer to a file path, not the cache directory. This is optional and not required for this PR.

---

🔍 Security Considerations

No security concerns identified. The change is purely a refactoring of configuration naming.

---

🎯 Performance Considerations

No performance impact. The change is nominal with minimal overhead from deprecation warnings.

---

📋 Test Coverage

Test coverage is good - all test files have been updated. The only gap is the lack of explicit tests for the deprecation warnings themselves (see suggestion #1 above).

---

✨ Summary

This is a well-planned refactoring that improves API clarity. Fix the critical bug in worker.ts, and this will be ready to merge. The backwards compatibility handling is excellent.

Action items:

1. ✅ Fix worker.ts:121 and worker.ts:154 to use serverBundleCachePath
2. (Optional) Add tests for deprecation warnings
3. (Optional) Specify removal version in deprecation comments

---

🤖 Generated with Claude Code

[claude]

Code Review

Summary

This PR successfully renames bundlePath to serverBundleCachePath to better describe its purpose as a cache directory for server bundles, not to be confused with Shakapacker's public bundle path. The implementation demonstrates excellent attention to backwards compatibility and user experience.

---

✅ Strengths

1. Excellent Backwards Compatibility

• The deprecation handling in configBuilder.ts:252-271 is well-implemented with clear warning messages
• Both old config option (bundlePath) and environment variable (RENDERER_BUNDLE_PATH) continue to work
• Fallback logic correctly prioritizes the new name over the deprecated one
• Helpful warning messages explain WHY the name changed and HOW to migrate

2. Comprehensive Documentation Updates

• All docs updated consistently across installation.md, basics.md, and js-configuration.md
• Migration guide provided in PR description
• Code examples updated to use the new naming

3. Thorough Test Coverage

• Test helper functions updated (helper.ts:38-108)
• All test files using the renamed function (serverBundleCachePath instead of bundlePath)
• Config tests cover the new naming

4. Clear Naming Improvement
The new name serverBundleCachePath is significantly better because:

• Explicitly indicates it's a cache directory
• Clarifies it's for server bundles
• Reduces confusion with Shakapacker's bundle paths

5. Consistent Implementation

• All internal references updated (utils.ts, worker.ts)
• TypeScript interface properly marked old property as deprecated with @deprecated tag
• ENV variable naming follows the same pattern (RENDERER_SERVER_BUNDLE_CACHE_PATH)

---

🔍 Observations & Minor Suggestions

1. Deprecation Logic (configBuilder.ts:252-265)
The current logic is solid but could be slightly more defensive:

// Current implementation
if (
  \!config.serverBundleCachePath ||
  config.serverBundleCachePath === defaultConfig.serverBundleCachePath
) {
  config.serverBundleCachePath = userConfig.bundlePath;
}

Suggestion: Consider what happens if a user provides BOTH bundlePath and serverBundleCachePath. Currently, serverBundleCachePath would win (which is correct), but you might want to add a warning if both are provided:

if ('bundlePath' in userConfig) {
  log.warn('bundlePath is deprecated...');
  
  // Warn if both are provided
  if ('serverBundleCachePath' in userConfig) {
    log.warn('Both bundlePath and serverBundleCachePath provided. Using serverBundleCachePath.');
  } else if (\!config.serverBundleCachePath || ...) {
    config.serverBundleCachePath = userConfig.bundlePath;
  }
}

However, this is a very minor edge case and the current implementation is acceptable.

2. Environment Variable Handling (configBuilder.ts:266-271)
The ENV deprecation warning only fires when RENDERER_BUNDLE_PATH is set but RENDERER_SERVER_BUNDLE_CACHE_PATH is not. This is correct behavior. ✅

3. Function Rename (configBuilder.ts:108)
Good job renaming defaultBundlePath() to defaultServerBundleCachePath() for consistency. ✅

4. Type Safety
The TypeScript interface correctly marks bundlePath? as optional with @deprecated tag, while serverBundleCachePath is required. This provides good IDE hints for users. ✅

---

🧪 Test Coverage Assessment

Based on the diff, test coverage appears comprehensive:

• ✅ Helper functions updated
• ✅ Config builder tests implicitly covered (through existing tests using setConfig)
• ✅ Worker tests updated to use new naming
• ✅ All fixture paths updated

Suggestion for additional test: Consider adding an explicit test case for the deprecation warning to ensure it fires correctly:

test('warns when using deprecated bundlePath', () => {
  const consoleSpy = jest.spyOn(log, 'warn');
  buildConfig({ bundlePath: '/some/path' });
  expect(consoleSpy).toHaveBeenCalledWith(
    expect.stringContaining('bundlePath is deprecated')
  );
});

Again, this is nice-to-have, not critical, as the deprecation logic is straightforward.

---

🔒 Security Considerations

No security concerns identified. This is purely a refactoring/renaming change. ✅

---

⚡ Performance Considerations

No performance impact. The deprecation checks are minimal (string comparisons) and only run during config initialization. ✅

---

📝 Code Quality

Style & Conventions:

• Follows TypeScript best practices ✅
• JSDoc comments updated appropriately ✅
• Consistent with repository's existing patterns ✅

Maintainability:

• Clear deprecation path for future removal ✅
• Well-documented reasoning in PR description ✅
• Type safety maintained ✅

---

🎯 Recommendations

Before Merge:

1. ✅ MUST: Run bundle exec rubocop (per CLAUDE.md requirements)
2. ✅ MUST: Ensure all files end with newlines
3. ✅ MUST: Verify ESLint passes

Nice-to-Have (Optional):

1. Consider adding explicit deprecation test case (as mentioned above)
2. Consider warning if both old and new config options are provided simultaneously (edge case)

---

🏆 Overall Assessment

Quality Score: 9/10

This is a high-quality PR with:

• ✅ Clear motivation and benefits
• ✅ Excellent backwards compatibility
• ✅ Comprehensive documentation updates
• ✅ Thorough test coverage
• ✅ No breaking changes
• ✅ Follows repository conventions

The implementation is production-ready. The minor suggestions above are optional enhancements, not blockers.

Recommendation: ✅ APPROVE (pending mandatory lint checks)

---

Great work on this refactoring! The new naming will definitely reduce confusion for users. 🎉