Fix RSpec helper optimization gap with private SSR directories #1838
Conversation
When server bundles are configured for private directories but webpack_generated_files is set to default ['manifest.json'], the server bundle was not being monitored for staleness during RSpec test runs. This change enhances ensure_webpack_generated_files_exists to automatically include server bundles and other configured bundle files in the monitoring list. Fixes #1822
Tests cover various scenarios including: - Auto-inclusion with default manifest.json configuration - Empty configuration population - Duplicate prevention - Custom file preservation - Edge cases with nil values - RSC bundle support - Server bundle monitoring for RSpec optimization These tests prevent regression of the optimization gap fix and ensure the Smart Auto-Inclusion logic works correctly across different configuration scenarios.
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 50 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 (1)
WalkthroughRefactors configuration to always compute required webpack-generated files and to populate or extend the webpack_generated_files list accordingly. Adds a comprehensive spec suite covering empty/prepopulated lists, optional bundles/manifests, nil handling, and RSpec server-bundle monitoring across directory variants. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test/Runtime as Caller
participant Config as Configuration
Note over Caller,Config: Ensuring webpack-generated files list
Caller->>Config: ensure_webpack_generated_files_exists()
Config->>Config: Build all_required_files (server bundle, RSC, manifests)
alt webpack_generated_files empty
Config->>Config: Set webpack_generated_files = all_required_files
else prepopulated
Config->>Config: Compute missing_files = all_required_files - existing
alt missing_files present
Config->>Config: Append missing_files (no duplicates)
else none missing
Config->>Config: No changes
end
end
Config-->>Caller: Updated webpack_generated_files
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Code Review for PR #1838: Fix RSpec helper optimization gap with private SSR directoriesSummaryThis PR successfully addresses a critical gap in the RSpec helper optimization that emerged after PR #1798's security improvements. The fix ensures server bundles in private directories are properly monitored during test runs, preventing tests from running with stale server-side code. ✅ Strengths
🎯 Code QualityThe implementation follows Ruby best practices:
✅ Security ConsiderationsThe fix properly maintains the security improvements from PR #1798:
📊 Performance
🔍 Potential Enhancements (Optional for future PRs)
✅ Testing VerificationThe test suite thoroughly validates:
🎖️ Overall AssessmentAPPROVED - This is a well-crafted fix that elegantly solves the optimization gap without introducing complexity or breaking changes. The comprehensive test coverage ensures reliability, and the implementation maintains the security improvements from PR #1798. The PR demonstrates excellent engineering practices:
Great work on identifying and fixing this subtle but important issue! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/react_on_rails/configuration.rb (1)
323-336: Make list-building nil-safe and dedupe via set-union; also simplify control flowCurrent code raises if
webpack_generated_filesisnil, and duplicates persist if pre-seeded by users. Prefer treatingnilas empty, removing duplicates, and appending missing in one step.Apply:
- all_required_files = [ - "manifest.json", - server_bundle_js_file, - rsc_bundle_js_file, - react_client_manifest_file, - react_server_client_manifest_file - ].compact_blank - - if webpack_generated_files.empty? - self.webpack_generated_files = all_required_files - else - missing_files = all_required_files.reject { |file| webpack_generated_files.include?(file) } - self.webpack_generated_files += missing_files if missing_files.any? - end + required = [ + "manifest.json", + server_bundle_js_file, + rsc_bundle_js_file, + react_client_manifest_file, + react_server_client_manifest_file + ].compact_blank + + current = Array(webpack_generated_files) + self.webpack_generated_files = + if current.blank? + required + else + current | required + endspec/react_on_rails/configuration_spec.rb (1)
528-673: Strong coverage of required-file population; add two edge testsThe scenarios look solid and align with the new behavior. Consider adding:
- A case where
webpack_generated_filesisnil(ensure nil-safety).- A case where
webpack_generated_filescontains duplicates (ensure de-dup).Example additions:
+ context "when webpack_generated_files is nil" do + it "treats nil as empty and populates required files" do + config.webpack_generated_files = nil + config.server_bundle_js_file = "server-bundle.js" + config.send(:ensure_webpack_generated_files_exists) + expect(config.webpack_generated_files).to eq(%w[ + manifest.json + server-bundle.js + react-client-manifest.json + react-server-client-manifest.json + ]) + end + end + + context "when webpack_generated_files has duplicates" do + it "does not keep duplicate entries after ensuring required files" do + config.webpack_generated_files = %w[manifest.json manifest.json server-bundle.js] + config.send(:ensure_webpack_generated_files_exists) + expect(config.webpack_generated_files.count("manifest.json")).to eq(1) + end + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/configuration.rb(1 hunks)spec/react_on_rails/configuration_spec.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
spec/react_on_rails/configuration_spec.rblib/react_on_rails/configuration.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: justin808
PR: shakacode/react_on_rails#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.
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#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/configuration.rb
🧬 Code graph analysis (1)
spec/react_on_rails/configuration_spec.rb (1)
lib/react_on_rails/test_helper/webpack_assets_status_checker.rb (1)
include(11-101)
⏰ 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, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: claude-review
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
- GitHub Check: build-and-test
|
@ihabadham I'm finishing this one up. |
The compact_blank method used in ensure_webpack_generated_files_exists was introduced in Rails 6.1. By requiring active_support/core_ext/enumerable, we ensure this method is available in Rails 5.2-6.0 environments. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #1838Thank you for working on this important optimization fix! The PR addresses a real gap in server bundle staleness detection. However, I've identified a critical bug that will cause the code to fail. 🔴 Critical Issue: Undefined MethodsLocation: The rsc_bundle_js_file,
react_client_manifest_file,
react_server_client_manifest_fileThese methods are only defined in Impact: This will cause a Test Evidence: The tests in config.rsc_bundle_js_file = nil
config.react_client_manifest_file = "react-client-manifest.json"
config.react_server_client_manifest_file = "react-server-client-manifest.json"This works in tests because RSpec allows setting arbitrary attributes, but will fail in production. Recommended FixAdd these attributes to the open-source # In lib/react_on_rails/configuration.rb
class Configuration
attr_accessor :node_modules_location, :server_bundle_js_file, :prerender, :replay_console,
# ... existing attributes ...
:server_bundle_output_path, :enforce_private_server_bundles,
:rsc_bundle_js_file, :react_client_manifest_file, # ADD THESE
:react_server_client_manifest_file # ADD THESEAnd initialize them in the ✅ Positive Aspects
📋 Other ObservationsCode Quality
Testing
Documentation
🔧 Required Changes Before Merge
🔒 Security & Performance
SummaryThis is a valuable fix for an important optimization gap, but requires the critical bug fix mentioned above before it can be safely merged. Once the missing attributes are added to the open-source Configuration class, this will be ready to go. Let me know if you need any clarification on the required changes! 🤖 Generated with Claude Code |
Summary
Fixes an optimization gap in the RSpec helper where server bundle staleness was not detected when using private SSR directories (introduced in PR #1798).
webpack_generated_files: ['manifest.json']configuration only monitored client assets, missing server bundle changes in privatessr-generated/directoryensure_webpack_generated_files_existsto automatically include server bundles and other critical files in monitoring listRoot Cause
PR #1798 moved server bundles to private directories for security, but the RSpec helper optimization continued monitoring only
manifest.jsonby default. This created a gap where:ssr-generated/server-bundle.jspublic/packs-test/manifest.jsonWhy this worked before: Prior to PR #1798, both client and server bundles were co-located in the same
public/packs/directory, so the RSpec helper's fallback logic would detect server bundle staleness even when only monitoringmanifest.json.Fix Details
Before:
After:
Testing
Test plan
public/, server inssr-generated/)Fixes #1822
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Bug Fixes
Tests