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.
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)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
| all_required_files = [ | ||
| "manifest.json", | ||
| server_bundle_js_file, | ||
| rsc_bundle_js_file, | ||
| react_client_manifest_file, | ||
| react_server_client_manifest_file | ||
| ].compact_blank | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Confirm ActiveSupport availability for compact_blank (or require the extension)
Array#compact_blank is from ActiveSupport core extensions. If the gem supports older Rails/AS versions, this will break unless the extension is loaded.
Run to verify dependency floor and usage:
🏁 Script executed:
#!/bin/bash
# Check declared minimum Rails/AS and usages of compact_blank
fd -a '*.gemspec' | xargs -I{} rg -n 'add_(runtime_)?dependency.*(rails|activesupport)' {}
rg -n 'compact_blank' -g '!**/tmp/**'Length of output: 437
🏁 Script executed:
#!/bin/bash
# Correct search for gemspec dependencies on rails/activesupport
echo "Searching gemspec dependencies..."
fd --extension gemspec --exec rg -n "add_(runtime_)?dependency.*(rails|activesupport)" {}
echo "Searching for .compact_blank usage..."
rg -nP "\.compact_blank" -g "!**/tmp/**"
echo "Searching for explicit requires of ActiveSupport core extensions..."
rg -n "require.*active_support/core_ext" -g "!**/tmp/**"Length of output: 537
#!/bin/bash
echo "Locate `.compact_blank` in configuration:"
rg --no-ignore -nP "\.compact_blank" lib/react_on_rails/configuration.rb
echo -e "\nInspect top of configuration file for requires:"
sed -n '1,50p' lib/react_on_rails/configuration.rb
echo -e "\nSearch for any ActiveSupport core_ext requires across lib/:"
rg --no-ignore -n "require .*active_support/core_ext" -g "lib/**"Require ActiveSupport or replace compact_blank in configuration.rb
Ensure compact_blank is available on Rails <6.1 by adding
require 'active_support/core_ext/enumerable'at the top of lib/react_on_rails/configuration.rb—or replace .compact_blank with .reject(&:blank?) to maintain compatibility with Rails 5.2–6.0.
🤖 Prompt for AI Agents
In lib/react_on_rails/configuration.rb around lines 323 to 330 the call to
.compact_blank may not exist on Rails < 6.1; either add a require for
ActiveSupport by adding require 'active_support/core_ext/enumerable' at the top
of the file, or replace .compact_blank with .reject(&:blank?) on that
all_required_files array to restore compatibility with Rails 5.2–6.0.
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