Integrate Shakapacker 9.0+ private_output_path for server bundle validation #1967
Conversation
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>
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (17)
WalkthroughAdds explanatory comments to generator templates about the server bundle output path and implements Doctor checks that read the webpack server config to validate the webpack output.path matches the Rails Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Doctor as ReactOnRails::Doctor
participant FS as FileSystem
participant Logger as Logger
Doctor->>FS: read "config/webpack/serverWebpackConfig.js"
alt file missing
FS-->>Doctor: file not found
Doctor->>Logger: info "serverWebpackConfig.js not present — skipping validation"
else file present
FS-->>Doctor: webpack_content
Doctor->>Doctor: extract_webpack_output_path(webpack_content)
alt hardcoded path extracted
Doctor->>Doctor: normalize_path(rails_config_path) & normalize_path(webpack_path)
alt paths match
Doctor->>Logger: success "Rails and Webpack server bundle paths in sync"
else mismatch
Doctor->>Logger: warn "Mismatch — show Rails path, Webpack path, and remediation steps"
end
else pattern not extractable
Doctor->>Logger: info "Could not parse output.path — manual verification needed"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
PR Review: Add server bundle path validation to doctor and improve generator documentationSummaryThis PR adds automated validation to prevent configuration mismatches between webpack and React on Rails server bundle paths. Overall, this is a well-designed and valuable addition that will help prevent hard-to-debug runtime errors. The implementation is mostly solid with good error handling. ✅ Strengths1. Excellent Developer Experience Improvements
2. Good Error Handling
3. Well-Documented Templates
🔍 Issues & Concerns1. Critical: Regex Pattern May Not Match Real-World Configs
|
![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)
lib/react_on_rails/doctor.rb (1)
1163-1211: Robust validation with helpful error messages.The validation logic is well-structured with graceful error handling and clear user feedback. The regex-based parsing is appropriate for this best-effort validation scenario.
Consider normalizing paths before comparison to avoid false positives from trailing slashes:
webpack_bundle_path = path_match[1] + + # Normalize paths by removing trailing slashes for comparison + webpack_bundle_path = webpack_bundle_path.sub(%r{/+$}, '') + rails_bundle_path = rails_bundle_path.sub(%r{/+$}, '') # Compare the pathsThis would ensure that
ssr-generatedmatchesssr-generated/in either configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt(1 hunks)lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt(1 hunks)lib/react_on_rails/doctor.rb(4 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.ttlib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.ttlib/react_on_rails/doctor.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 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/templates/base/base/config/webpack/serverWebpackConfig.js.ttlib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 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/templates/base/base/config/webpack/serverWebpackConfig.js.ttlib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.ttlib/react_on_rails/doctor.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.ttlib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.ttlib/react_on_rails/doctor.rb
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.ttlib/react_on_rails/doctor.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
lib/react_on_rails/doctor.rb
🧬 Code graph analysis (1)
lib/react_on_rails/doctor.rb (1)
lib/react_on_rails/system_checker.rb (3)
add_info(28-30)add_success(24-26)add_warning(20-22)
⏰ 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). (8)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: claude-review
🔇 Additional comments (3)
lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt (1)
47-64: Excellent documentation for server bundle path synchronization!The added comments clearly explain the critical requirement that webpack's
output.pathmust match Rails'server_bundle_output_path, and provide security guidance about keeping server bundles private. This will help developers avoid common SSR configuration issues.lib/react_on_rails/doctor.rb (1)
679-689: Good addition of server bundle path validation!The integration into
analyze_server_rendering_configis clean, and calling the validation helper maintains good separation of concerns.lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt (1)
46-59: Clear and consistent documentation!The comments effectively mirror the webpack config documentation, establishing a clear two-way reference between the Rails and webpack configurations. The security guidance about keeping server bundles outside the
public/directory is valuable.
…sive tests
Addresses critical issues identified in code review:
**Issue 1: Regex Pattern Too Restrictive**
- Original regex only matched hardcoded path.resolve() pattern
- Real dummy app uses config.outputPath, which wasn't matched at all
- Validation would silently skip without detecting actual configs
**Fix:**
- Support hardcoded path: require('path').resolve(__dirname, '../../path')
- Detect config.outputPath usage (can't validate, inform user)
- Detect variable usage (can't validate, inform user)
- Clear messaging for each pattern type
**Issue 2: Missing Path Normalization**
- String equality comparison failed on equivalent paths
- ./ssr-generated vs ssr-generated would be false mismatch
- Trailing slashes caused false positives
**Fix:**
- normalize_path() method strips leading ./ and /
- Removes trailing slashes
- Handles whitespace
- Graceful handling of nil/non-string values
**Issue 3: No Test Coverage**
- New validation methods had zero tests
- High risk of regressions
**Fix:**
- Added 20+ comprehensive test cases covering:
- File not found scenarios
- Hardcoded paths (matching and mismatched)
- config.outputPath detection
- Variable detection
- Error handling
- Path normalization edge cases
- All return nil vs success vs warning paths
**Testing Results:**
- All 20 new tests passing
- 30/31 total specs passing (1 pre-existing failure unrelated)
- Covers real-world patterns including dummy app config
**Technical Details:**
- extract_webpack_output_path() now pattern-aware
- Better user messaging for unvalidatable configs
- Maintains backward compatibility
- No breaking changes
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
✅ Critical Issues FixedThank you for the excellent code review! All three critical issues have been addressed: 1. ✅ Regex Pattern Now Supports Real-World ConfigsProblem: Original regex only matched hardcoded Fix:
2. ✅ Path Normalization AddedProblem: String equality failed on equivalent paths like Fix:
3. ✅ Comprehensive Test Coverage AddedProblem: No tests for the new validation methods Fix:
Test ResultsTechnical Changeslib/react_on_rails/doctor.rb:
spec/lib/react_on_rails/doctor_spec.rb:
Ready for re-review! 🚀 |
Code Review - PR #1967I've reviewed the pull request "Add server bundle path validation to doctor and improve generator documentation." Overall, this is a well-implemented feature that addresses a real pain point. Here's my detailed feedback: ✅ Strengths1. Excellent Problem Identification & Solution
2. Robust Implementation
3. Outstanding Test Coverage
4. Documentation
🔍 Issues & Suggestions1. Regex Pattern Limitation (Minor)Location: The hardcoded path pattern only matches hardcoded_pattern = %r{path:\s*require\(['"]path['"]\)\.resolve\(__dirname,\s*['"]\.\./ \.\./([^'"]+)['"]\)}Issue: Won't match other relative paths like:
Suggestion: Make the pattern more flexible: hardcoded_pattern = %r{path:\s*require\(['"]path['"]\)\.resolve\(__dirname,\s*['"](\.[\./ ]+[^'"]+)['"]\)}Then normalize the extracted path to be relative to Rails.root. For example:
This would handle more project structures. 2. Path Normalization Doesn't Handle Relative Paths (Minor)Location: Current normalized = normalized.sub(%r{^\.?/}, "") # Remove leading ./ or /
normalized.sub(%r{/$}, "") # Remove trailing /Issue: Doesn't resolve Impact: If someone uses a different webpack directory structure, the path comparison will fail even when configs are actually in sync. Suggestion: Add path resolution logic: def normalize_path(path)
return path unless path.is_a?(String)
normalized = path.strip
# Resolve relative paths like ../../ssr-generated
normalized = Pathname.new(normalized).cleanpath.to_s if normalized.include?('..')
# Remove leading ./ or /
normalized = normalized.sub(%r{^\.?/}, "")
# Remove trailing /
normalized.sub(%r{/$}, "")
end3. Unused Parameter (Very Minor)Location: def extract_webpack_output_path(webpack_content, _webpack_config_path)The Suggestion: Remove it unless you're planning to use it in error messages: def extract_webpack_output_path(webpack_content)Update the caller at line 1175: webpack_bundle_path = extract_webpack_output_path(webpack_content)4. Consider Testing Integration (Enhancement)The tests mock all File I/O, which is good for unit testing. Consider adding an integration test that:
This would catch issues like:
Example location: 5. Variable Pattern Might Be Too Broad (Minor)Location: if webpack_content.match?(/path:\s*[a-zA-Z_]\w*/)Issue: This will match ANY identifier after Suggestion: Be more specific: if webpack_content.match?(/^\s*path:\s*[a-zA-Z_]\w*[,}]/, Regexp::MULTILINE)Or check that it's within an output object context. 🛡️ Security Considerations✅ No security concerns identified
⚡ Performance Considerations✅ Performance is good
📋 Best Practices✅ Follows repository conventions
🧪 Test Coverage AssessmentCoverage: Excellent (95%+) ✅ Covered:
❓ Could add (nice-to-have):
📝 Documentation Quality✅ Very good
🎯 RecommendationsMust Fix (Before Merge)None - the code is production-ready as-is. Should Fix (Improvements)
Nice to Have (Future Enhancement)
✨ Final VerdictApproval: ✅ Approved with minor suggestions This is a high-quality PR that solves a real problem. The implementation is solid, tests are comprehensive, and it follows the repository's standards. The suggested improvements are minor and can be addressed in follow-up PRs if desired. The PR is ready to merge as-is, with the optional improvements noted above for consideration. Great work! 🎉 Review performed by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/doctor.rb(4 hunks)spec/lib/react_on_rails/doctor_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/react_on_rails/doctor.rbspec/lib/react_on_rails/doctor_spec.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
lib/react_on_rails/doctor.rbspec/lib/react_on_rails/doctor_spec.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 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/react_on_rails/doctor.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 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/react_on_rails/doctor.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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:
lib/react_on_rails/doctor.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/doctor.rb
🧬 Code graph analysis (1)
lib/react_on_rails/doctor.rb (1)
lib/react_on_rails/system_checker.rb (3)
add_info(28-30)add_success(24-26)add_warning(20-22)
⏰ 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). (8)
- GitHub Check: dummy-app-integration-tests (3.2, 20, minimum)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: claude-review
🔇 Additional comments (3)
lib/react_on_rails/doctor.rb (2)
1160-1207: Well-designed validation with clear user guidance.The implementation is resilient and user-friendly:
- Gracefully handles missing webpack configs
- Provides detailed, actionable fix instructions when mismatches are detected
- Normalizes paths before comparison to avoid false positives
- Error handling prevents validation failures from disrupting the doctor run
1209-1235: No action required—regex limitation to../../is intentional.The hardcoded regex pattern matching only
../../is by design. Test coverage (spec/lib/react_on_rails/doctor_spec.rb, lines 311-352) confirms this is the supported convention for React on Rails webpack configs, where the webpack config sits atconfig/webpack/and output atpublic/, requiring exactly two parent directory traversals.All other relative path formats are handled gracefully:
config.outputPathreturns nil with info message- Variable references return nil with info message
- Other patterns trigger "Could not parse" fallback
The method's fallback logic ensures validation doesn't break when encountering unsupported patterns.
spec/lib/react_on_rails/doctor_spec.rb (1)
191-387: Excellent test coverage with comprehensive edge case handling.The test suite thoroughly covers:
- All validation scenarios (missing config, matching paths, mismatches)
- Different webpack config patterns (hardcoded, config.outputPath, variables)
- Error handling (read failures)
- Path normalization edge cases (nil, non-string, whitespace, slashes)
- Proper message verification (success, warning, info)
The mocking strategy is appropriate and doesn't introduce brittleness.
Updated Code Review - PR #1967I've reviewed the latest changes, and I'm happy to report that all previous concerns have been addressed! All Critical Issues Resolved1. Regex Pattern Now Supports Real-World ConfigsThe updated implementation now intelligently detects:
2. Path Normalization AddedThe new normalize_path helper properly handles leading/trailing slashes, whitespace, and graceful nil handling. 3. Comprehensive Test CoverageOutstanding test additions with 20+ test cases covering all scenarios, edge cases, and error handling. Code Quality AssessmentStrengths:
Potential Improvements (Optional/Future):
Security & Performance
Repository Standards ComplianceFollows all CLAUDE.md requirements: RuboCop compliant, comprehensive tests, clear naming Final RecommendationStatus: APPROVED - Ready to Merge This PR successfully addresses a real pain point with a well-designed, thoroughly tested solution. The implementation is robust, handles edge cases gracefully, and provides excellent developer experience. The minor improvements noted above are truly optional and could be addressed in future PRs if the need arises. Excellent work addressing all the feedback from the initial review! Reviewed with Claude Code following repository conventions from CLAUDE.md |
Code Review - PR #1967I've reviewed this pull request and I'm impressed with the implementation! This is a well-thought-out solution to a real developer pain point. Here's my detailed feedback: ✅ Strengths1. Excellent Problem-Solution AlignmentThe PR clearly identifies a real issue (config sync between webpack and Rails) and provides a comprehensive solution with documentation + validation + testing. The approach is pragmatic and developer-friendly. 2. Comprehensive Test CoverageThe test suite (
3. Robust Error HandlingThe implementation gracefully handles:
4. Clear DocumentationThe warning comments in both
5. User-Friendly MessagingThe doctor output provides:
🔍 Code Quality Observations1. Path Normalization Logic ✅The
2. Regex Pattern Matching ✅The
3. Method Organization ✅Good separation of concerns:
🎯 Potential Improvements (Minor)1. Method PlacementThe new methods are added at line 1160+ in 2. Complexity Warning
3. Webpack Config Parsing LimitationsThe current regex-based approach is pragmatic but has inherent limitations (can't handle runtime values). The PR handles this well by:
Consider: Adding a comment in the code explaining why regex parsing is used instead of evaluating the JS (security, complexity, dependencies). 4. Test File LocationThe tests are added to the existing 🔒 Security Considerations ✅
🚀 Performance Considerations ✅
🐛 Potential BugsMinor: Line 678-688 Ordering IssueIn the diff, I notice the new code is inserted at lines 678-688, but the original RSC bundle check at line 679 should come AFTER the new server_bundle_output_path section. Looking at the actual implementation, this appears correct in the final version, so no issue here. No bugs identified - the implementation is solid! 📋 Testing RecommendationsManual Testing Checklist:
These all appear to be covered by your test suite. Excellent work! 📝 Documentation Quality ✅The PR description is excellent:
✨ SummaryThis is a high-quality PR that I recommend merging after addressing any feedback you'd like to incorporate. The code is:
Minor Suggestions (Optional):
Required Before Merge:
Great work! This will genuinely improve the developer experience for React on Rails users. Review generated by Claude Code with human oversight |
Code Review FeedbackOverall, this is a well-designed PR that addresses a real pain point for developers. The validation logic and enhanced documentation will help prevent configuration mismatches. Strengths
Code Quality ObservationsDoctor.rb Implementation - Positive:
Suggestions:
Generator Templates - Positive:
Potential Issues
Security and Performance
Test CoverageExcellent coverage of edge cases. Consider adding: test for malformed webpack files to ensure the parser doesn't crash. RecommendationsHigh Priority:
Medium Priority: Low Priority: Overall AssessmentThis is a high-quality PR that will meaningfully improve the developer experience. The implementation is solid, test coverage is comprehensive, and the documentation is clear. The suggestions above are refinements rather than blockers. Recommendation: Approve with minor suggestions The PR is ready to merge as-is, but would benefit from addressing the Windows path handling and adding the malformed config test case for robustness. |
Code ReviewOverviewThis PR adds automated validation for server bundle path configuration sync between webpack and Rails, along with enhanced documentation. The implementation is well-thought-out and addresses a real pain point for developers. ✅ Strengths1. Excellent Problem-Solution Fit
2. Robust Implementation
3. Strong Test Coverage
4. Good Documentation
🔍 Code Quality Observations1. Method Complexity (Minor)The
Suggestion: def analyze_server_rendering_config(content)
checker.add_info("\n🖥️ Server Rendering:")
# ... existing code ...
rails_bundle_path = extract_rails_bundle_path(content)
checker.add_info(" server_bundle_output_path: #{rails_bundle_path}")
validate_server_bundle_path_sync(rails_bundle_path)
# ... rest of analysis ...
end
def extract_rails_bundle_path(content)
server_bundle_path_match = content.match(/config\.server_bundle_output_path\s*=\s*["']([^"']+)["']/)
server_bundle_path_match ? server_bundle_path_match[1] : "ssr-generated"
end2. Regex Pattern Robustness (Minor)The webpack path extraction uses a specific regex pattern: hardcoded_pattern = %r{path:\s*require\(['"]path['"]\)\.resolve\(__dirname,\s*['"]\.\.?/\.\.?/([^'"]+)['"]\)}Potential issues:
Current behavior is acceptable since:
Optional enhancement: Add a comment explaining why certain patterns aren't supported. 3. Path Normalization Edge CasesThe def normalize_path(path)
return path unless path.is_a?(String)
normalized = path.strip
normalized = normalized.sub(%r{^\.\.?/}, "") # Only removes one leading ./
normalized.sub(%r{/$}, "")
endEdge case: Paths like Impact: Minimal - such paths are unlikely in practice Optional improvement: normalized = normalized.gsub(%r{^\.?/}, '') # Remove all leading ./ or /4. Error Message Formatting (Minor - Nitpick)The multiline warning message is excellent, but there's inconsistent indentation:
🔒 Security Considerations✅ No security concerns identified
🧪 Testing RecommendationsAlready Covered ✅
Additional Test Suggestions (Optional)
📊 Performance Considerations✅ No performance concerns
🎯 Best Practices AlignmentFollows Repository Guidelines ✅
React on Rails Conventions ✅
🐛 Potential Bugs1. Regex Pattern for Webpack Path (Low Risk)The pattern hardcoded_pattern = %r{path:\s*require\(['"]path['"]\)\.resolve\(__dirname,\s*['"]\.\.?/\.\.?/([^'"]+)['"]\)}Issue: This assumes webpack config is always in Current behavior: Template is in Risk: If users move webpack config, validation will fail silently (return nil) Mitigation: Already handled - returns nil and logs info message ✅ Not a bug - working as designed 2. Missing Test for Mixed SlashesThe tests don't cover paths with mixed slashes:
Impact: Verification needed: expect(doctor.send(:normalize_path, "./ssr-generated/")).to eq("ssr-generated")
expect(doctor.send(:normalize_path, "/ssr-generated/")).to eq("ssr-generated")Looking at the tests: Actually, this IS tested at line 195: it "handles paths with both leading and trailing slashes" do
expect(doctor.send(:normalize_path, "./ssr-generated/")).to eq("ssr-generated")
end✅ Already covered 💡 Suggestions for Future Enhancements
📝 DocumentationGenerator Templates ✅
Code Comments
🎓 Learning & MaintainabilityExcellent maintainability:
Potential confusion:
✅ Final VerdictLGTM - Ready to merge with minor suggestions This is a high-quality PR that:
Minor Improvements (Optional)
Recommendation: Approve and merge. The suggestions are truly optional and don't block merging. Great work! This will save developers significant debugging time. 🎉 Review generated with assistance from Claude Code |
Code Review - PR #1967: Add server bundle path validation to doctorOverall Assessment✅ Excellent PR! This is a well-thought-out solution to a real developer pain point. The implementation is solid with comprehensive test coverage and clear documentation. Strengths1. Clear Problem & Solution ✨
2. Comprehensive Test Coverage 🧪
3. Excellent Error Messages 💬
4. Robust Implementation 🛡️
Issues & Suggestions🔴 Critical Issues1. Missing RuboCop disable in line 667 In # rubocop:disable Metrics/CyclomaticComplexity
def analyze_server_rendering_config(content)But the ending comment on line 714 disables BOTH: # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexityThis is asymmetric and may cause RuboCop violations. According to CLAUDE.md:
Fix: Update line 667 to match: # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexityVerification needed: bundle exec rubocop lib/react_on_rails/doctor.rb🟡 Medium Priority Issues2. Incomplete implementation in Looking at the diff, the new code adds: server_bundle_path_match = content.match(/config\.server_bundle_output_path\s*=\s*["']([^"']+)["']/)
rails_bundle_path = server_bundle_path_match ? server_bundle_path_match[1] : "ssr-generated"
checker.add_info(" server_bundle_output_path: #{rails_bundle_path}")
enforce_private_match = content.match(/config\.enforce_private_server_bundles\s*=\s*([^\s\n,]+)/)
checker.add_info(" enforce_private_server_bundles: #{enforce_private_match[1]}") if enforce_private_match
validate_server_bundle_path_sync(rails_bundle_path)But then continues with the RSC bundle code. The original
Suggestion: The logical grouping is good, but consider if the validation should run AFTER all config is displayed, not in the middle. This might make the output flow better. Not a blocker, but consider: # Display all config first
checker.add_info(" server_bundle_js_file: ...")
checker.add_info(" server_bundle_output_path: ...")
checker.add_info(" enforce_private_server_bundles: ...")
# ... other configs ...
# Then validate at the end
validate_server_bundle_path_sync(rails_bundle_path)3. Regex pattern could be more specific In hardcoded_pattern = %r{path:\s*require\(['"]path['"]\)\.resolve\(__dirname,\s*['"]\.\.[\/\.\.]+([^'"]+)['"]\)}The pattern path: require('path').resolve(__dirname, '../../ssr-generated')Current pattern: This looks correct but hard to read. Consider simplifying to: hardcoded_pattern = %r{path:\s*require\(['"]path['"]\)\.resolve\(__dirname,\s*['"]\.\.[\/]\.\.[\/]([^'"]+)['"]\)}This explicitly matches 4. Missing test for trailing newlines According to CLAUDE.md:
The spec file should verify this (though git hooks should catch it). Not critical but worth checking: tail -c 1 lib/react_on_rails/doctor.rb | od -An -tx1🔵 Minor Suggestions5. Documentation could mention The generator templates have great inline comments, but might also benefit from mentioning: # Run 'rails react_on_rails:doctor' to verify these configs are in sync
config.server_bundle_output_path = "ssr-generated"This helps discoverability of the validation feature. 6. Consider extracting magic strings The path SERVER_WEBPACK_CONFIG_PATH = "config/webpack/serverWebpackConfig.js".freeze
def validate_server_bundle_path_sync(rails_bundle_path)
unless File.exist?(SERVER_WEBPACK_CONFIG_PATH)
# ...
end
end7. Test coverage: consider adding integration test The unit tests are comprehensive, but an integration test that actually runs Security Review ✅No security concerns identified:
Performance Review ✅Negligible performance impact:
Alignment with CLAUDE.md ✅
RecommendationsBefore Merge:
Optional Improvements (can be follow-up PRs):
ConclusionThis is a high-quality PR that solves a real problem elegantly. The only blocking issue is the RuboCop asymmetry which should be a quick fix. Once that's resolved and RuboCop passes, this is ready to merge! 🚀 Great work on the comprehensive test coverage and thoughtful error messages. This will definitely improve the developer experience. Review by: Claude Code |
…improve docs
Addresses feedback from detailed code review:
**1. Critical: Fix Asymmetric RuboCop Directives**
- Issue: Line 667 only disabled Metrics/CyclomaticComplexity
- But line 702 enabled BOTH Metrics/AbcSize and Metrics/CyclomaticComplexity
- This asymmetry could cause violations
- Fix: Changed line 667 to disable both cops symmetrically
- Result: RuboCop reports method doesn't actually need AbcSize disabled, auto-fixed
**2. Simplify Regex Pattern for Clarity**
- Issue: Pattern `['"]\.\.[\/\.\.]+` was hard to read
- Fix: Changed to explicit `['"]\.\.\/\.\.\/` to clearly match ../../path
- Makes intent obvious: we match exactly two parent directory traversals
- RuboCop auto-removed redundant escapes inside %r{}
**3. Improve Documentation Discoverability**
- Issue: Users might not know about the validation feature
- Fix: Added "Run 'rails react_on_rails:doctor' to verify these configs are in sync"
- Placement: In generator template right next to server_bundle_output_path config
- Helps users discover the validation tool when they need it most
**Testing:**
- ✅ All 20 validation tests still passing
- ✅ RuboCop passes with zero offenses
- ✅ Regex pattern matches test cases correctly
- ✅ 30/31 total specs passing (1 pre-existing failure unrelated)
**Other Suggestions Considered:**
- Reordering validation (decided current flow is good - validates immediately after config display)
- Magic string constant (not needed - single use in method, clear context)
- Trailing newline check (git hooks already enforce this automatically)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
✅ Second Round of Feedback AddressedThank you for the thorough second review! All issues have been fixed: 1. ✅ Critical: Fixed Asymmetric RuboCop DirectivesProblem: Line 667 only disabled Fix:
2. ✅ Simplified Regex PatternProblem: Pattern Fix:
3. ✅ Improved Documentation DiscoverabilityProblem: Users might not know the validation feature exists Fix:
VerificationOther Suggestions ConsideredConfig Ordering (Issue #2):
Magic String Constant (Issue #6):
Trailing Newlines (Issue #4):
All changes pushed and ready for final review! 🚀 |
Code Review - PR #1967OverviewThis PR adds automated validation to detect configuration mismatches between webpack and Rails server bundle paths. The implementation is well-thought-out and addresses a real developer pain point. ✅ Strengths
🔍 Code Quality ObservationsPositive
Suggestions for Improvement
🔒 Security Considerations✅ No security concerns identified. The code:
⚡ Performance Considerations✅ Performance impact is negligible:
📚 Documentation Quality✅ Excellent:
🧪 Test Coverage✅ Comprehensive test coverage:
The test suite demonstrates professional quality and should catch regressions effectively. 📋 Changelog ComplianceNote: According to
🎯 Final RecommendationAPPROVE with minor suggestions This is a high-quality PR that:
The suggestions above are minor refinements that could be addressed in a follow-up PR if desired. The current implementation is production-ready. Minor Action Items (Optional)
Great work on improving the developer experience! 🎉 Review generated with Claude Code |
Code Review - PR 1967OverviewThis PR adds automated validation to prevent configuration mismatches between webpack and React on Rails server bundle paths. The implementation is well-designed and addresses a real pain point for developers. Strengths1. Excellent Problem Statement and Documentation
2. Robust Implementation
3. Comprehensive Test Coverage
4. User Experience
Code Quality Observationsdoctor.rb validate_server_bundle_path_sync method:
doctor.rb extract_webpack_output_path method:
doctor.rb normalize_path method:
Test CoverageExemplary test suite covering:
Performance and SecurityPerformance: Minimal overhead validation only runs during doctor command with no impact on build or runtime Security: No issues identified. Regex patterns are safe file operations are local only and validation is read-only. Actually improves security by reinforcing private server bundle best practices. Checklist per CLAUDE.md
Final AssessmentOverall Rating: Excellent This is a high-quality PR that solves a real developer pain point with comprehensive tests excellent documentation and graceful edge case handling. Recommendation: Approve with one request Please add a CHANGELOG.md entry for this user-visible feature per the projects CLAUDE.md guidelines. The format should be: PR 1967 by justin808 - Added automated validation to rails react_on_rails:doctor command to detect server bundle path mismatches between webpack and Rails configs Once CHANGELOG is updated this is ready to merge. Generated with Claude Code - https://claude.com/claude-code |
Code Review FeedbackThank you for this PR! This is a valuable enhancement that addresses a real pain point for developers. Overall, the implementation is solid and well-tested. Here are my observations: ✅ Strengths
🐛 Potential Issues1. Method Placement in
|
Code Review: Server Bundle Path ValidationI've reviewed PR #1967 and found this to be a well-crafted improvement that addresses a real pain point in React on Rails configuration. Here's my detailed feedback: ✅ Strengths1. Excellent Problem-Solution Fit
2. Comprehensive Test Coverage
3. Robust Implementation
4. Documentation Excellence
5. Code Quality
💡 Minor Observations1. RuboCop Directive Placement (line 667)The code correctly disables # rubocop:disable Metrics/CyclomaticComplexity
def analyze_server_rendering_config(content)
# ... method body ...
validate_server_bundle_path_sync(rails_bundle_path) # Adds complexity
end
# rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexityNote: The enable directive includes both 2. Regex Pattern ClarityThe hardcoded pattern regex is clear with the added comment: # Explicitly match ../../path pattern for clarity
hardcoded_pattern = %r{path:\s*require\(['"]path['"]\)\.resolve\(__dirname,\s*['"]\.\./ \.\./ ([^'"]+)['"]\)}This explicitly shows it matches exactly two parent directory traversals ( 3. Security ContextThe PR correctly maintains the security-first approach established in PRs #1798 and #1815:
🎯 Specific Technical PointsPath Normalization Logic (doctor.rb:1233-1240)def normalize_path(path)
return path unless path.is_a?(String)
normalized = path.strip
normalized = normalized.sub(%r{^\. ?/}, "") # Remove leading ./ or /
normalized.sub(%r{/$}, "") # Remove trailing /
end✅ Good:
Pattern Detection Logic (doctor.rb:1212-1229)The three-tier detection approach is solid:
This graceful degradation is excellent UX. Test Coverage AnalysisFrom
Coverage assessment: Excellent - all critical paths tested. 🔒 Security Considerations✅ No security concerns identified:
🚀 Performance Considerations✅ No performance concerns:
📋 Testing AssessmentTest execution from PR description:
Test structure:
🎨 Code Style & Best Practices✅ Adheres to React on Rails conventions:
✅ Ruby best practices:
📝 Documentation QualityGenerator Templates✅ Excellent improvements:
Error Messages✅ User-friendly and actionable:
🔄 Commit HistoryThe three commits show good iterative improvement:
This progression demonstrates responsiveness to feedback and commitment to quality. ✨ Overall AssessmentRating: Excellent ⭐⭐⭐⭐⭐ This PR demonstrates:
Recommendation: Approve and merge ✅ The implementation is production-ready and adds valuable DX improvements without introducing risks. The validation feature will help developers catch configuration issues early, reducing debugging time and support burden. 🙏 Minor Suggestions (Optional)
But these are truly minor and don't block this PR. Great work! This is exactly the kind of developer experience improvement that makes a framework pleasant to use. 🎉 |
Code Review for PR #1967Thank you for this well-thought-out PR! The approach to prevent configuration mismatches is excellent. ✅ Strengths1. Excellent Problem/Solution Fit
2. Well-Structured Implementation
3. Great Developer Experience
4. Documentation Quality
🔍 Observations & Suggestions1. Path Extraction Regex Pattern The regex only matches
2. Path Normalization Excellent defensive programming with nil and non-string handling! Minor consideration: Windows paths with backslashes (though likely not a concern for this project). 3. Test Coverage Outstanding test coverage! One additional test to consider for webpack config syntax errors (malformed JS). 🛡️ Security & PerformanceSecurity: No concerns. Only reads files, uses regex (no eval), handles errors gracefully. Performance: Minimal impact. Only runs during manual 🎯 Final RecommendationAPPROVE with minor suggestions This is a high-quality PR that:
The suggestions above are minor enhancements, not blockers. Ready to merge! Great work! 🎉 Reviewer: Claude Code |
Code Review - PR #1967Excellent work on this PR! The addition of automated validation for server bundle path synchronization is a valuable improvement. ✅ Strengths1. Well-Documented Problem & Solution
2. Comprehensive Test Coverage
3. Robust Error Handling
4. Smart Pattern Matching
🎯 Code QualityRuboCop Directives ✅ - Symmetrically disabled/enabled, addressing previous review feedback Path Normalization ✅ - Handles nil, removes leading/trailing slashes, prevents false mismatches Validation Integration ✅ - Well-placed in analyze_server_rendering_config method 🔒 Security & PerformanceSecurity ✅
Performance ✅
📝 Documentation QualityGenerator Templates ✅ - Clear warnings with cross-references and validation tool pointer Error Messages ✅ - Actionable warnings with both values shown and fix steps provided 💡 Minor SuggestionThe regex pattern assumes exactly ../../ (two levels up). If someone uses ../../../ it won't match. This is acceptable since the generator creates ../../, and it falls through to a Could not parse message gracefully. 🎯 Final Verdict✅ APPROVED - Ready to Merge This PR demonstrates thoughtful problem-solving, excellent test coverage, robust error handling, and clear documentation. All feedback from previous reviews has been addressed. Impact:
Recommendation: ✅ Merge when ready (after CI passes) Great work on addressing all previous feedback! Generated with Claude Code |
Addresses code review feedback about fragile regex matching by implementing a proper integration with Shakapacker 9.0+ private_output_path. **Key Changes:** **1. Auto-Detection from Shakapacker (configuration.rb)** - New `auto_detect_server_bundle_path_from_shakapacker` method - Automatically reads `private_output_path` from Shakapacker 9.0+ config - Only applies if user hasn't explicitly set `server_bundle_output_path` - Gracefully falls back to default if detection fails - Logs info message when auto-detection succeeds **2. Removed Fragile Regex Validation (doctor.rb)** - Removed `validate_server_bundle_path_sync` method (regex matching) - Removed `extract_webpack_output_path` method (pattern detection) - Removed `normalize_path` method (no longer needed) - Replaced with `check_shakapacker_private_output_path` method **3. Recommendation-Based Doctor Checks (doctor.rb)** - Detects Shakapacker version and capabilities - Pre-9.0: Recommends upgrading for better DX - 9.0+ without config: Shows how to configure private_output_path - 9.0+ with config matching: Success message - 9.0+ with config mismatch: Warning with fix instructions - No Shakapacker: Informs about manual configuration **4. Updated Generator Templates** - **React on Rails initializer**: Documents Shakapacker 9.0+ approach first - **Webpack config**: Shows config.privateOutputPath pattern - Both templates emphasize single source of truth in shakapacker.yml - Clear migration path for older Shakapacker versions **5. Comprehensive Test Coverage (8 new tests)** - Shakapacker not defined scenario - Pre-9.0 Shakapacker (no private_output_path support) - 9.0+ with matching config - 9.0+ with mismatched config - 9.0+ without config - Error handling - All tests passing **Benefits:** - No fragile regex parsing of webpack configs - Single source of truth in shakapacker.yml - Automatic configuration for Shakapacker 9.0+ users - Backward compatible with older Shakapacker versions - Clear upgrade path and recommendations - Robust error handling **Breaking Changes:** None - Existing configurations continue to work - Auto-detection only applies to default values - Explicit user configuration always takes precedence Addresses: @justin's feedback on PR #1967 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🎉 Replaced Fragile Regex with Proper Shakapacker IntegrationThank you for the excellent feedback! You were absolutely right - the regex matching approach was too fragile. I've completely replaced it with proper Shakapacker 9.0+ integration. ✅ What Changed1. Auto-Detection from Shakapacker (NEW) # lib/react_on_rails/configuration.rb
def auto_detect_server_bundle_path_from_shakapacker
# Reads Shakapacker.config.private_output_path (9.0+)
# Only applies if user hasn't explicitly configured
# Falls back gracefully if detection fails
end2. Removed ALL Fragile Regex Validation
3. Recommendation-Based Doctor
4. Updated Generator Templates
🎯 How It WorksFor Shakapacker 9.0+ Users:
React on Rails automatically detects and uses the value from shakapacker.yml. Single source of truth! For Older Shakapacker or Custom Setups:
📊 Test CoverageAll 8 new tests passing:
🚫 No More Regex!Before (Fragile): # Tried to parse webpack config with regex
webpack_content.match(%r{path:\s*require...})
# Failed on: config.outputPath, variables, etc.After (Robust): # Reads from Shakapacker's actual runtime config
::Shakapacker.config.private_output_path
# Always accurate, no parsing needed!###Thanks for pushing me in the right direction! This is much cleaner and more maintainable. 🙏 |
…tion Documents the recommended approach of using Shakapacker 9.0+ private_output_path for server bundle configuration, providing a single source of truth. **Documentation Updates:** **1. Configuration API Reference (docs/api-reference/configuration.md)** - Added prominent recommendation for Shakapacker 9.0+ approach - Documents shakapacker.yml private_output_path configuration - Explains auto-detection behavior - Preserves documentation for older versions **2. Webpack Configuration Guide (docs/core-concepts/webpack-configuration.md)** - New section: "Server Bundle Configuration (Shakapacker 9.0+)" - Complete example with shakapacker.yml and webpack config - Lists benefits of the new approach: - Single source of truth - Automatic synchronization - No configuration duplication - Better maintainability - Notes compatibility with older versions **Key Points:** - Shakapacker 9.0+ users get automatic configuration - Backward compatible with manual configuration - Generator templates already show both approaches - Doctor command guides users to upgrade **Related Changes:** - Generator templates already updated in previous commit - Auto-detection implemented in configuration.rb - Doctor provides version-aware recommendations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
📚 Documentation UpdatedAdded comprehensive documentation for the Shakapacker 9.0+ integration: Updated Files1. Configuration API Reference (
2. Webpack Configuration Guide (
Generator TemplatesNo changes needed! The generator templates already:
This way:
Complete Package✅ Auto-detection in The PR is complete and ready for final review! 🎉 |
Adds commented-out private_output_path configuration to the generated shakapacker.yml, making it easy for users to enable the Shakapacker 9.0+ integration feature. **Changes:** - Added private_output_path: ssr-generated (commented out) - Clear documentation that it's a Shakapacker 9.0+ feature - Notes that React on Rails automatically detects it - Positioned logically after public_output_path configuration **User Experience:** Users can now simply uncomment one line in shakapacker.yml to enable automatic configuration instead of needing to manually add it. **Complete Integration:** - ✅ Auto-detection in configuration.rb - ✅ Doctor recommendations - ✅ Generator templates (webpack + initializer) - ✅ Shakapacker.yml template (NEW) - ✅ Documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
✅ Added private_output_path to shakapacker.yml GeneratorGreat catch! I've added the What Was AddedIn # Location for private server-side bundles (e.g., for SSR)
# These bundles are not served publicly, unlike public_output_path
# Shakapacker 9.0+ feature - automatically detected by React on Rails
# Uncomment to enable (requires Shakapacker 9.0+):
# private_output_path: ssr-generatedBenefitsEasy Discovery: New users see the option right in the generated file Complete Generator CoverageNow ALL generator templates guide users to the Shakapacker 9.0+ approach:
Users can enable the full integration by:
Perfect DX! 🎉 |
justin808
changed the title
…ration
This commit addresses code review feedback and enhances the Shakapacker 9.0+
integration with several key improvements:
## Key Improvements
1. **Extract path normalization to shared helper**
- Added Utils.normalize_to_relative_path method
- Eliminates code duplication between configuration.rb and doctor.rb
- Handles edge cases: Pathname objects, special characters, trailing slashes
- Uses proper regex escaping with Regexp.escape
2. **Add comprehensive test coverage**
- Added 17 new test cases for path normalization
- Covers absolute/relative paths, nil handling, special characters
- Tests paths with spaces, dots, hyphens, and substring matching
- All 81 utils tests passing
3. **Make generator version-aware**
- Added shakapacker_version_9_or_higher? helper method
- serverWebpackConfig.js.tt now generates optimal code based on version:
* Shakapacker 9.0+: Uses config.privateOutputPath
* Shakapacker < 9.0: Uses hardcoded path with upgrade notes
- shakapacker.yml.tt automatically enables private_output_path for 9.0+
- Provides clear upgrade path for older versions
4. **Improve error handling**
- Added safe navigation operator for Rails.logger calls
- Graceful fallbacks throughout auto-detection code
- Proper exception handling in generator helper
5. **Code quality improvements**
- All RuboCop violations fixed (0 offenses)
- Added complexity disable directive for well-structured method
- Clear documentation and comments throughout
## Benefits
- New installations get optimal configuration automatically
- No manual config needed for Shakapacker 9.0+ users
- Single source of truth in shakapacker.yml
- Better maintainability through DRY principles
- Comprehensive test coverage ensures reliability
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Resolved conflicts between Shakapacker 9.0+ integration and async validation: - Combined Shakapacker private_output_path auto-detection with async checks - Merged documentation for both features - Integrated test suites for both feature sets All conflicts resolved while preserving both sets of functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Addresses code review feedback about fragile regex matching by implementing a proper integration with Shakapacker 9.0+ private_output_path. **Key Changes:** **1. Auto-Detection from Shakapacker (configuration.rb)** - New `auto_detect_server_bundle_path_from_shakapacker` method - Automatically reads `private_output_path` from Shakapacker 9.0+ config - Only applies if user hasn't explicitly set `server_bundle_output_path` - Gracefully falls back to default if detection fails - Logs info message when auto-detection succeeds **2. Removed Fragile Regex Validation (doctor.rb)** - Removed `validate_server_bundle_path_sync` method (regex matching) - Removed `extract_webpack_output_path` method (pattern detection) - Removed `normalize_path` method (no longer needed) - Replaced with `check_shakapacker_private_output_path` method **3. Recommendation-Based Doctor Checks (doctor.rb)** - Detects Shakapacker version and capabilities - Pre-9.0: Recommends upgrading for better DX - 9.0+ without config: Shows how to configure private_output_path - 9.0+ with config matching: Success message - 9.0+ with config mismatch: Warning with fix instructions - No Shakapacker: Informs about manual configuration **4. Updated Generator Templates** - **React on Rails initializer**: Documents Shakapacker 9.0+ approach first - **Webpack config**: Shows config.privateOutputPath pattern - Both templates emphasize single source of truth in shakapacker.yml - Clear migration path for older Shakapacker versions **5. Comprehensive Test Coverage (8 new tests)** - Shakapacker not defined scenario - Pre-9.0 Shakapacker (no private_output_path support) - 9.0+ with matching config - 9.0+ with mismatched config - 9.0+ without config - Error handling - All tests passing **Benefits:** - No fragile regex parsing of webpack configs - Single source of truth in shakapacker.yml - Automatic configuration for Shakapacker 9.0+ users - Backward compatible with older Shakapacker versions - Clear upgrade path and recommendations - Robust error handling **Breaking Changes:** None - Existing configurations continue to work - Auto-detection only applies to default values - Explicit user configuration always takes precedence Addresses: @justin's feedback on PR #1967 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Closing in favor of #2028 which has a cleaner rebased history without the merge commit and unrelated changes from master. |
Addresses code review feedback about fragile regex matching by implementing a proper integration with Shakapacker 9.0+ private_output_path. **Key Changes:** **1. Auto-Detection from Shakapacker (configuration.rb)** - New `auto_detect_server_bundle_path_from_shakapacker` method - Automatically reads `private_output_path` from Shakapacker 9.0+ config - Only applies if user hasn't explicitly set `server_bundle_output_path` - Gracefully falls back to default if detection fails - Logs info message when auto-detection succeeds **2. Removed Fragile Regex Validation (doctor.rb)** - Removed `validate_server_bundle_path_sync` method (regex matching) - Removed `extract_webpack_output_path` method (pattern detection) - Removed `normalize_path` method (no longer needed) - Replaced with `check_shakapacker_private_output_path` method **3. Recommendation-Based Doctor Checks (doctor.rb)** - Detects Shakapacker version and capabilities - Pre-9.0: Recommends upgrading for better DX - 9.0+ without config: Shows how to configure private_output_path - 9.0+ with config matching: Success message - 9.0+ with config mismatch: Warning with fix instructions - No Shakapacker: Informs about manual configuration **4. Updated Generator Templates** - **React on Rails initializer**: Documents Shakapacker 9.0+ approach first - **Webpack config**: Shows config.privateOutputPath pattern - Both templates emphasize single source of truth in shakapacker.yml - Clear migration path for older Shakapacker versions **5. Comprehensive Test Coverage (8 new tests)** - Shakapacker not defined scenario - Pre-9.0 Shakapacker (no private_output_path support) - 9.0+ with matching config - 9.0+ with mismatched config - 9.0+ without config - Error handling - All tests passing **Benefits:** - No fragile regex parsing of webpack configs - Single source of truth in shakapacker.yml - Automatic configuration for Shakapacker 9.0+ users - Backward compatible with older Shakapacker versions - Clear upgrade path and recommendations - Robust error handling **Breaking Changes:** None - Existing configurations continue to work - Auto-detection only applies to default values - Explicit user configuration always takes precedence Addresses: @justin's feedback on PR #1967 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Addresses code review feedback about fragile regex matching by implementing a proper integration with Shakapacker 9.0+ private_output_path. **Key Changes:** **1. Auto-Detection from Shakapacker (configuration.rb)** - New `auto_detect_server_bundle_path_from_shakapacker` method - Automatically reads `private_output_path` from Shakapacker 9.0+ config - Only applies if user hasn't explicitly set `server_bundle_output_path` - Gracefully falls back to default if detection fails - Logs info message when auto-detection succeeds **2. Removed Fragile Regex Validation (doctor.rb)** - Removed `validate_server_bundle_path_sync` method (regex matching) - Removed `extract_webpack_output_path` method (pattern detection) - Removed `normalize_path` method (no longer needed) - Replaced with `check_shakapacker_private_output_path` method **3. Recommendation-Based Doctor Checks (doctor.rb)** - Detects Shakapacker version and capabilities - Pre-9.0: Recommends upgrading for better DX - 9.0+ without config: Shows how to configure private_output_path - 9.0+ with config matching: Success message - 9.0+ with config mismatch: Warning with fix instructions - No Shakapacker: Informs about manual configuration **4. Updated Generator Templates** - **React on Rails initializer**: Documents Shakapacker 9.0+ approach first - **Webpack config**: Shows config.privateOutputPath pattern - Both templates emphasize single source of truth in shakapacker.yml - Clear migration path for older Shakapacker versions **5. Comprehensive Test Coverage (8 new tests)** - Shakapacker not defined scenario - Pre-9.0 Shakapacker (no private_output_path support) - 9.0+ with matching config - 9.0+ with mismatched config - 9.0+ without config - Error handling - All tests passing **Benefits:** - No fragile regex parsing of webpack configs - Single source of truth in shakapacker.yml - Automatic configuration for Shakapacker 9.0+ users - Backward compatible with older Shakapacker versions - Clear upgrade path and recommendations - Robust error handling **Breaking Changes:** None - Existing configurations continue to work - Auto-detection only applies to default values - Explicit user configuration always takes precedence Addresses: @justin's feedback on PR #1967 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Addresses code review feedback about fragile regex matching by implementing a proper integration with Shakapacker 9.0+ private_output_path. **Key Changes:** **1. Auto-Detection from Shakapacker (configuration.rb)** - New `auto_detect_server_bundle_path_from_shakapacker` method - Automatically reads `private_output_path` from Shakapacker 9.0+ config - Only applies if user hasn't explicitly set `server_bundle_output_path` - Gracefully falls back to default if detection fails - Logs info message when auto-detection succeeds **2. Removed Fragile Regex Validation (doctor.rb)** - Removed `validate_server_bundle_path_sync` method (regex matching) - Removed `extract_webpack_output_path` method (pattern detection) - Removed `normalize_path` method (no longer needed) - Replaced with `check_shakapacker_private_output_path` method **3. Recommendation-Based Doctor Checks (doctor.rb)** - Detects Shakapacker version and capabilities - Pre-9.0: Recommends upgrading for better DX - 9.0+ without config: Shows how to configure private_output_path - 9.0+ with config matching: Success message - 9.0+ with config mismatch: Warning with fix instructions - No Shakapacker: Informs about manual configuration **4. Updated Generator Templates** - **React on Rails initializer**: Documents Shakapacker 9.0+ approach first - **Webpack config**: Shows config.privateOutputPath pattern - Both templates emphasize single source of truth in shakapacker.yml - Clear migration path for older Shakapacker versions **5. Comprehensive Test Coverage (8 new tests)** - Shakapacker not defined scenario - Pre-9.0 Shakapacker (no private_output_path support) - 9.0+ with matching config - 9.0+ with mismatched config - 9.0+ without config - Error handling - All tests passing **Benefits:** - No fragile regex parsing of webpack configs - Single source of truth in shakapacker.yml - Automatic configuration for Shakapacker 9.0+ users - Backward compatible with older Shakapacker versions - Clear upgrade path and recommendations - Robust error handling **Breaking Changes:** None - Existing configurations continue to work - Auto-detection only applies to default values - Explicit user configuration always takes precedence Addresses: @justin's feedback on PR #1967 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Addresses code review feedback about fragile regex matching by implementing a proper integration with Shakapacker 9.0+ private_output_path. **Key Changes:** **1. Auto-Detection from Shakapacker (configuration.rb)** - New `auto_detect_server_bundle_path_from_shakapacker` method - Automatically reads `private_output_path` from Shakapacker 9.0+ config - Only applies if user hasn't explicitly set `server_bundle_output_path` - Gracefully falls back to default if detection fails - Logs info message when auto-detection succeeds **2. Removed Fragile Regex Validation (doctor.rb)** - Removed `validate_server_bundle_path_sync` method (regex matching) - Removed `extract_webpack_output_path` method (pattern detection) - Removed `normalize_path` method (no longer needed) - Replaced with `check_shakapacker_private_output_path` method **3. Recommendation-Based Doctor Checks (doctor.rb)** - Detects Shakapacker version and capabilities - Pre-9.0: Recommends upgrading for better DX - 9.0+ without config: Shows how to configure private_output_path - 9.0+ with config matching: Success message - 9.0+ with config mismatch: Warning with fix instructions - No Shakapacker: Informs about manual configuration **4. Updated Generator Templates** - **React on Rails initializer**: Documents Shakapacker 9.0+ approach first - **Webpack config**: Shows config.privateOutputPath pattern - Both templates emphasize single source of truth in shakapacker.yml - Clear migration path for older Shakapacker versions **5. Comprehensive Test Coverage (8 new tests)** - Shakapacker not defined scenario - Pre-9.0 Shakapacker (no private_output_path support) - 9.0+ with matching config - 9.0+ with mismatched config - 9.0+ without config - Error handling - All tests passing **Benefits:** - No fragile regex parsing of webpack configs - Single source of truth in shakapacker.yml - Automatic configuration for Shakapacker 9.0+ users - Backward compatible with older Shakapacker versions - Clear upgrade path and recommendations - Robust error handling **Breaking Changes:** None - Existing configurations continue to work - Auto-detection only applies to default values - Explicit user configuration always takes precedence Addresses: @justin's feedback on PR #1967 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Addresses code review feedback about fragile regex matching by implementing a proper integration with Shakapacker 9.0+ private_output_path. **Key Changes:** **1. Auto-Detection from Shakapacker (configuration.rb)** - New `auto_detect_server_bundle_path_from_shakapacker` method - Automatically reads `private_output_path` from Shakapacker 9.0+ config - Only applies if user hasn't explicitly set `server_bundle_output_path` - Gracefully falls back to default if detection fails - Logs info message when auto-detection succeeds **2. Removed Fragile Regex Validation (doctor.rb)** - Removed `validate_server_bundle_path_sync` method (regex matching) - Removed `extract_webpack_output_path` method (pattern detection) - Removed `normalize_path` method (no longer needed) - Replaced with `check_shakapacker_private_output_path` method **3. Recommendation-Based Doctor Checks (doctor.rb)** - Detects Shakapacker version and capabilities - Pre-9.0: Recommends upgrading for better DX - 9.0+ without config: Shows how to configure private_output_path - 9.0+ with config matching: Success message - 9.0+ with config mismatch: Warning with fix instructions - No Shakapacker: Informs about manual configuration **4. Updated Generator Templates** - **React on Rails initializer**: Documents Shakapacker 9.0+ approach first - **Webpack config**: Shows config.privateOutputPath pattern - Both templates emphasize single source of truth in shakapacker.yml - Clear migration path for older Shakapacker versions **5. Comprehensive Test Coverage (8 new tests)** - Shakapacker not defined scenario - Pre-9.0 Shakapacker (no private_output_path support) - 9.0+ with matching config - 9.0+ with mismatched config - 9.0+ without config - Error handling - All tests passing **Benefits:** - No fragile regex parsing of webpack configs - Single source of truth in shakapacker.yml - Automatic configuration for Shakapacker 9.0+ users - Backward compatible with older Shakapacker versions - Clear upgrade path and recommendations - Robust error handling **Breaking Changes:** None - Existing configurations continue to work - Auto-detection only applies to default values - Explicit user configuration always takes precedence Addresses: @justin's feedback on PR #1967 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…es (#2028) ## Summary This PR integrates Shakapacker 9.0+ `private_output_path` configuration with React on Rails, eliminating the need for manual `server_bundle_output_path` configuration and providing automatic synchronization between webpack and Rails configs. ## Key Changes ### Auto-Detection - **New**: Automatically detects `private_output_path` from `shakapacker.yml` (Shakapacker 9.0+) - Falls back to default `"ssr-generated"` if not configured - Gracefully handles older Shakapacker versions ### Generator Updates - **Templates**: Updated to use `private_output_path` for Shakapacker 9.0+ - **Version-aware**: Detects Shakapacker version and generates appropriate config - **Backward compatible**: Generates hardcoded paths for older versions ### Doctor Enhancements - **Detection**: Identifies Shakapacker version and `private_output_path` support - **Validation**: Checks for configuration mismatches - **Recommendations**: Provides upgrade guidance and migration steps - **Clear messaging**: Success/warning/info messages for all scenarios ### Documentation - Updated configuration docs with Shakapacker 9.0+ examples - Added benefits section explaining advantages - Clear migration path from older versions ## Benefits 1. **Single Source of Truth**: Server bundle path configured once in `shakapacker.yml` 2. **Automatic Sync**: No manual coordination between webpack and Rails configs 3. **Better Maintainability**: Reduces configuration duplication 4. **Graceful Degradation**: Works with older Shakapacker versions 5. **Clear Diagnostics**: Doctor command validates configuration ## Testing - ✅ Comprehensive RSpec tests for auto-detection logic - ✅ Tests for path normalization edge cases - ✅ Doctor diagnostic tests for all scenarios - ✅ Generator tests for version-aware templates - ✅ All existing tests pass ## Migration Path For users on Shakapacker 9.0+: 1. Add `private_output_path: ssr-generated` to `config/shakapacker.yml` 2. Remove `config.server_bundle_output_path` from React on Rails initializer (optional - auto-detected) 3. Run `rails react_on_rails:doctor` to verify For users on older Shakapacker: - No changes required - continues to work as before ## Related Supersedes #1967 (rebased for cleaner history) 🤖 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/2028) <!-- Reviewable:end --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Auto-detection/validation of server bundle locations with new configuration options for private server bundles and SSR behavior. * **Documentation** * Major expansion of configuration docs and guides for server rendering, bundle organization, Shakapacker integration, and migration examples. * **Tools & Diagnostics** * Enhanced doctor diagnostics with targeted guidance when bundle paths or private-output settings mismatch. * **Tests** * Added comprehensive tests for Shakapacker integration and path-normalization scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Integrates Shakapacker 9.0+'s new
private_output_pathconfiguration to automatically validate and ensure server bundle path consistency between webpack and React on Rails.Problem
Developers need to keep two configurations in sync manually:
When these get out of sync, server-side rendering fails with hard-to-debug errors.
Solution
This PR leverages Shakapacker 9.0+'s new
private_output_pathfeature to eliminate manual configuration:1. Shakapacker Configuration Integration
In
shakapacker.yml:This single source of truth is:
2. Enhanced Generator Documentation
In
serverWebpackConfig.js.tt:In
react_on_rails.rb.tt:3. Automated Doctor Validation
The
rails react_on_rails:doctorcommand now:private_output_pathfrom shakapacker.ymlserver_bundle_output_pathfrom Rails configExample output when configs match:
Example output when mismatched:
4. Comprehensive Documentation
Added detailed documentation in:
Technical Details
Files changed:
config/shakapacker.yml- Added private_output_path configurationlib/react_on_rails/configuration.rb- Added private_output_path reader methodlib/react_on_rails/doctor.rb- Added Shakapacker integration validationlib/generators/.../serverWebpackConfig.js.tt- Updated to use private_output_pathlib/generators/.../react_on_rails.rb.tt- Enhanced documentationdocs/api-reference/configuration.md- New configuration documentationdocs/core-concepts/webpack-configuration.md- Updated with private_output_path usagespec/lib/react_on_rails/doctor_spec.rb- Comprehensive validation testsImplementation highlights:
Benefits
Testing
Related
Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
This change is