Skip to content

Commit b2a9aeb

Browse files
ihabadhamjustin808claude
authored
Fix RSpec helper optimization gap with private SSR directories (#1838)
## 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). - **Issue**: Default `webpack_generated_files: ['manifest.json']` configuration only monitored client assets, missing server bundle changes in private `ssr-generated/` directory - **Impact**: Tests could run with stale server-side code, leading to incorrect test results - **Solution**: Enhanced `ensure_webpack_generated_files_exists` to automatically include server bundles and other critical files in monitoring list ## Root Cause PR #1798 moved server bundles to private directories for security, but the RSpec helper optimization continued monitoring only `manifest.json` by default. This created a gap where: 1. Server bundle changes in `ssr-generated/server-bundle.js` 2. Client assets remain fresh in `public/packs-test/manifest.json` 3. RSpec helper reports "no rebuild needed" ❌ 4. Tests run with stale server-side code **Why 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 monitoring `manifest.json`. ## Fix Details **Before:** ```ruby def ensure_webpack_generated_files_exists return unless webpack_generated_files.empty? # ❌ Never runs with default config # ... end ``` **After:** ```ruby def ensure_webpack_generated_files_exists 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 end ``` ## Testing - ✅ Comprehensive test coverage added for all scenarios - ✅ Verified fix resolves gap with test apps - ✅ Backward compatibility maintained - ✅ Works across different directory configurations ## Test plan - [x] RSpec helper correctly detects server bundle staleness with default config - [x] Cross-directory monitoring works (client in `public/`, server in `ssr-generated/`) - [x] Existing configurations continue working unchanged - [x] Edge cases handled (nil values, custom configurations) Fixes #1822 🤖 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/1838) <!-- Reviewable:end --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - More reliable two-phase population of required Webpack assets, avoiding duplicates. - Preserves pre-existing custom files while adding only missing essentials. - Avoids adding nil/empty entries when server bundle or manifests are absent. - Includes RSC and additional Pro manifests when configured. - Improves server-bundle monitoring behavior for faster test runs. - Tests - Added comprehensive tests covering asset inclusion, duplication prevention, custom configs, RSC support, and multi-directory monitoring. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Justin Gordon <[email protected]> Co-authored-by: Claude <[email protected]>
1 parent 924fb00 commit b2a9aeb

File tree

2 files changed

+134
-7
lines changed

2 files changed

+134
-7
lines changed

lib/react_on_rails/configuration.rb

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -429,18 +429,23 @@ def configure_generated_assets_dirs_deprecation
429429
end
430430

431431
def ensure_webpack_generated_files_exists
432-
return unless webpack_generated_files.empty?
433-
434-
files = ["manifest.json", server_bundle_js_file]
432+
all_required_files = ["manifest.json", server_bundle_js_file]
435433

436434
if ReactOnRails::Utils.react_on_rails_pro?
437435
pro_config = ReactOnRailsPro.configuration
438-
files << pro_config.rsc_bundle_js_file
439-
files << pro_config.react_client_manifest_file
440-
files << pro_config.react_server_client_manifest_file
436+
all_required_files << pro_config.rsc_bundle_js_file
437+
all_required_files << pro_config.react_client_manifest_file
438+
all_required_files << pro_config.react_server_client_manifest_file
441439
end
442440

443-
self.webpack_generated_files = files.compact_blank
441+
all_required_files = all_required_files.compact_blank
442+
443+
if webpack_generated_files.empty?
444+
self.webpack_generated_files = all_required_files
445+
else
446+
missing_files = all_required_files.reject { |file| webpack_generated_files.include?(file) }
447+
self.webpack_generated_files += missing_files if missing_files.any?
448+
end
444449
end
445450

446451
def configure_skip_display_none_deprecation

spec/react_on_rails/configuration_spec.rb

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,17 @@ module ReactOnRails
287287
context "with Pro license" do
288288
before do
289289
allow(ReactOnRails::Utils).to receive(:react_on_rails_pro?).and_return(true)
290+
291+
# Mock Pro configuration to avoid dependency on Pro being installed
292+
pro_module = Module.new
293+
pro_config = double("ProConfiguration") # rubocop:disable RSpec/VerifiedDoubles
294+
allow(pro_config).to receive_messages(
295+
rsc_bundle_js_file: "",
296+
react_client_manifest_file: "react-client-manifest.json",
297+
react_server_client_manifest_file: "react-server-client-manifest.json"
298+
)
299+
pro_module.define_singleton_method(:configuration) { pro_config }
300+
stub_const("ReactOnRailsPro", pro_module)
290301
end
291302

292303
it "defaults to :async" do
@@ -626,6 +637,117 @@ module ReactOnRails
626637
end
627638
end
628639
end
640+
641+
describe "#ensure_webpack_generated_files_exists" do
642+
let(:config) { described_class.new }
643+
644+
before do
645+
# Reset to test defaults
646+
config.server_bundle_js_file = "server-bundle.js"
647+
allow(ReactOnRails::Utils).to receive(:react_on_rails_pro?).and_return(false)
648+
end
649+
650+
context "when webpack_generated_files has default manifest.json only" do
651+
it "automatically includes server bundle when configured" do
652+
config.webpack_generated_files = %w[manifest.json]
653+
654+
config.send(:ensure_webpack_generated_files_exists)
655+
656+
expect(config.webpack_generated_files).to eq(%w[manifest.json server-bundle.js])
657+
end
658+
659+
it "does not duplicate manifest.json" do
660+
config.webpack_generated_files = %w[manifest.json]
661+
662+
config.send(:ensure_webpack_generated_files_exists)
663+
664+
expect(config.webpack_generated_files.count("manifest.json")).to eq(1)
665+
end
666+
end
667+
668+
context "when webpack_generated_files is empty" do
669+
it "populates with all required files" do
670+
config.webpack_generated_files = []
671+
672+
config.send(:ensure_webpack_generated_files_exists)
673+
674+
expect(config.webpack_generated_files).to eq(%w[manifest.json server-bundle.js])
675+
end
676+
end
677+
678+
context "when server bundle already included" do
679+
it "does not duplicate entries" do
680+
config.webpack_generated_files = %w[manifest.json server-bundle.js]
681+
682+
config.send(:ensure_webpack_generated_files_exists)
683+
684+
expect(config.webpack_generated_files).to eq(%w[manifest.json server-bundle.js])
685+
expect(config.webpack_generated_files.count("server-bundle.js")).to eq(1)
686+
end
687+
end
688+
689+
context "when custom files are configured" do
690+
it "preserves custom files and adds missing critical files" do
691+
config.webpack_generated_files = %w[manifest.json custom-bundle.js]
692+
693+
config.send(:ensure_webpack_generated_files_exists)
694+
695+
expect(config.webpack_generated_files).to include("manifest.json")
696+
expect(config.webpack_generated_files).to include("custom-bundle.js")
697+
expect(config.webpack_generated_files).to include("server-bundle.js")
698+
end
699+
end
700+
701+
context "when server bundle is not configured" do
702+
it "does not add empty server bundle" do
703+
config.server_bundle_js_file = ""
704+
config.webpack_generated_files = %w[manifest.json]
705+
706+
config.send(:ensure_webpack_generated_files_exists)
707+
708+
expect(config.webpack_generated_files).not_to include("")
709+
expect(config.webpack_generated_files).to eq(%w[manifest.json])
710+
end
711+
712+
it "does not add nil server bundle" do
713+
config.server_bundle_js_file = nil
714+
config.webpack_generated_files = %w[manifest.json]
715+
716+
config.send(:ensure_webpack_generated_files_exists)
717+
718+
expect(config.webpack_generated_files).not_to include(nil)
719+
expect(config.webpack_generated_files).to eq(%w[manifest.json])
720+
end
721+
end
722+
723+
context "when ensuring server bundle monitoring for RSpec optimization" do
724+
it "ensures server bundle in private directory is monitored with default config" do
725+
# Simulate default generator configuration
726+
config.webpack_generated_files = %w[manifest.json]
727+
config.server_bundle_js_file = "server-bundle.js"
728+
config.server_bundle_output_path = "ssr-generated"
729+
730+
config.send(:ensure_webpack_generated_files_exists)
731+
732+
# Critical: server bundle must be included for RSpec helper optimization to work
733+
expect(config.webpack_generated_files).to include("server-bundle.js")
734+
end
735+
736+
it "handles all files being in different directories" do
737+
# Simulate cross-directory scenario
738+
config.webpack_generated_files = %w[manifest.json]
739+
config.server_bundle_js_file = "server-bundle.js"
740+
config.server_bundle_output_path = "ssr-generated"
741+
config.generated_assets_dir = "public/packs"
742+
743+
config.send(:ensure_webpack_generated_files_exists)
744+
745+
# All critical files should be monitored regardless of directory
746+
expect(config.webpack_generated_files).to include("manifest.json")
747+
expect(config.webpack_generated_files).to include("server-bundle.js")
748+
end
749+
end
750+
end
629751
end
630752
end
631753

0 commit comments

Comments
 (0)