Skip to content

Commit 0d8552b

Browse files
justin808claude
andcommitted
Implement code review recommendations: improve thread-safety, tests, and deprecation detection
- Add comprehensive unit tests for ReactOnRails::Utils.normalize_immediate_hydration - Tests for Pro/non-Pro license scenarios - Tests for invalid value type validation - Tests for warning messages with component/store names - Add tests for deprecated config.immediate_hydration setter/getter - Tests for deprecation warnings on both setter and getter - Tests for warning suppression (only warns once) - Tests for setter/getter interaction scenarios - Fix thread-safety of deprecation warning flag - Implement Mutex-based synchronization for @immediate_hydration_warned flag - Ensures safe access in multi-threaded environments (Puma, Sidekiq, etc.) - Add doctor.rb detection for deprecated config.immediate_hydration - Helps users identify and remove deprecated configuration during migration - Provides clear guidance on automatic behavior for Pro users - Remove unnecessary blank line in controller.rb for cleaner code All tests passing. Zero RuboCop violations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 4c845d9 commit 0d8552b

File tree

5 files changed

+234
-9
lines changed

5 files changed

+234
-9
lines changed

lib/react_on_rails/configuration.rb

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,18 +66,25 @@ class Configuration
6666
:component_registry_timeout,
6767
:server_bundle_output_path, :enforce_private_server_bundles
6868

69-
# Class instance variable to track if deprecation warning has been shown
69+
# Class instance variable and mutex to track if deprecation warning has been shown
70+
# Using mutex to ensure thread-safety in multi-threaded environments
7071
@immediate_hydration_warned = false
72+
@immediate_hydration_mutex = Mutex.new
7173

7274
class << self
73-
attr_accessor :immediate_hydration_warned
75+
attr_accessor :immediate_hydration_warned, :immediate_hydration_mutex
7476
end
7577

7678
# Deprecated: immediate_hydration configuration has been removed
7779
def immediate_hydration=(value)
78-
return if self.class.immediate_hydration_warned
80+
warned = false
81+
self.class.immediate_hydration_mutex.synchronize do
82+
warned = self.class.immediate_hydration_warned
83+
self.class.immediate_hydration_warned = true unless warned
84+
end
85+
86+
return if warned
7987

80-
self.class.immediate_hydration_warned = true
8188
Rails.logger.warn <<~WARNING
8289
[REACT ON RAILS] The 'config.immediate_hydration' configuration option is deprecated and no longer used.
8390
Immediate hydration is now automatically enabled for React on Rails Pro users.
@@ -87,9 +94,14 @@ def immediate_hydration=(value)
8794
end
8895

8996
def immediate_hydration
90-
return nil if self.class.immediate_hydration_warned
97+
warned = false
98+
self.class.immediate_hydration_mutex.synchronize do
99+
warned = self.class.immediate_hydration_warned
100+
self.class.immediate_hydration_warned = true unless warned
101+
end
102+
103+
return nil if warned
91104

92-
self.class.immediate_hydration_warned = true
93105
Rails.logger.warn <<~WARNING
94106
[REACT ON RAILS] The 'config.immediate_hydration' configuration option is deprecated and no longer used.
95107
Immediate hydration is now automatically enabled for React on Rails Pro users.

lib/react_on_rails/controller.rb

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ module Controller
1616
# or else there will be no client side hydration of your stores.
1717
def redux_store(store_name, props: {}, immediate_hydration: nil)
1818
immediate_hydration = ReactOnRails::Utils.normalize_immediate_hydration(immediate_hydration, store_name, "Store")
19-
2019
redux_store_data = { store_name: store_name,
2120
props: props,
2221
immediate_hydration: immediate_hydration }

lib/react_on_rails/doctor.rb

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -701,7 +701,7 @@ def analyze_server_rendering_config(content)
701701
end
702702
# rubocop:enable Metrics/AbcSize
703703

704-
# rubocop:disable Metrics/CyclomaticComplexity
704+
# rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity
705705
def analyze_performance_config(content)
706706
checker.add_info("\n⚡ Performance & Loading:")
707707

@@ -732,13 +732,21 @@ def analyze_performance_config(content)
732732
auto_load_match = content.match(/config\.auto_load_bundle\s*=\s*([^\s\n,]+)/)
733733
checker.add_info(" auto_load_bundle: #{auto_load_match[1]}") if auto_load_match
734734

735+
# Deprecated immediate_hydration setting
736+
immediate_hydration_match = content.match(/config\.immediate_hydration\s*=\s*([^\s\n,]+)/)
737+
if immediate_hydration_match
738+
checker.add_warning(" ⚠️ immediate_hydration: #{immediate_hydration_match[1]} (DEPRECATED)")
739+
checker.add_info(" 💡 This setting is no longer used. Immediate hydration is now automatic for Pro users.")
740+
checker.add_info(" 💡 Remove this line from your config/initializers/react_on_rails.rb file.")
741+
end
742+
735743
# Component registry timeout
736744
timeout_match = content.match(/config\.component_registry_timeout\s*=\s*([^\s\n,]+)/)
737745
return unless timeout_match
738746

739747
checker.add_info(" component_registry_timeout: #{timeout_match[1]}ms")
740748
end
741-
# rubocop:enable Metrics/CyclomaticComplexity
749+
# rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity
742750

743751
# rubocop:disable Metrics/AbcSize
744752
def analyze_development_config(content)

spec/react_on_rails/configuration_spec.rb

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,118 @@ module ReactOnRails
366366
end
367367
end
368368

369+
describe ".immediate_hydration (deprecated)" do
370+
before do
371+
# Reset the warning flag before each test
372+
described_class.immediate_hydration_warned = false
373+
allow(Rails.logger).to receive(:warn)
374+
end
375+
376+
after do
377+
# Reset the warning flag after each test
378+
described_class.immediate_hydration_warned = false
379+
end
380+
381+
describe "setter" do
382+
it "logs a deprecation warning when setting to true" do
383+
ReactOnRails.configure do |config|
384+
config.immediate_hydration = true
385+
end
386+
387+
expect(Rails.logger).to have_received(:warn)
388+
.with(/immediate_hydration' configuration option is deprecated/)
389+
end
390+
391+
it "logs a deprecation warning when setting to false" do
392+
ReactOnRails.configure do |config|
393+
config.immediate_hydration = false
394+
end
395+
396+
expect(Rails.logger).to have_received(:warn)
397+
.with(/immediate_hydration' configuration option is deprecated/)
398+
end
399+
400+
it "mentions the value in the warning message" do
401+
ReactOnRails.configure do |config|
402+
config.immediate_hydration = true
403+
end
404+
405+
expect(Rails.logger).to have_received(:warn) do |message|
406+
expect(message).to include("config.immediate_hydration = true")
407+
end
408+
end
409+
410+
it "only logs the warning once even if called multiple times" do
411+
ReactOnRails.configure do |config|
412+
config.immediate_hydration = true
413+
config.immediate_hydration = false
414+
config.immediate_hydration = true
415+
end
416+
417+
expect(Rails.logger).to have_received(:warn).once
418+
end
419+
end
420+
421+
describe "getter" do
422+
it "logs a deprecation warning when accessed" do
423+
ReactOnRails.configure {} # rubocop:disable Lint/EmptyBlock
424+
425+
ReactOnRails.configuration.immediate_hydration
426+
427+
expect(Rails.logger).to have_received(:warn)
428+
.with(/immediate_hydration' configuration option is deprecated/)
429+
end
430+
431+
it "returns nil" do
432+
ReactOnRails.configure {} # rubocop:disable Lint/EmptyBlock
433+
434+
result = ReactOnRails.configuration.immediate_hydration
435+
436+
expect(result).to be_nil
437+
end
438+
439+
it "only logs the warning once even if called multiple times" do
440+
ReactOnRails.configure {} # rubocop:disable Lint/EmptyBlock
441+
442+
ReactOnRails.configuration.immediate_hydration
443+
ReactOnRails.configuration.immediate_hydration
444+
ReactOnRails.configuration.immediate_hydration
445+
446+
expect(Rails.logger).to have_received(:warn).once
447+
end
448+
end
449+
450+
describe "setter and getter interactions" do
451+
it "does not warn again on getter if setter already warned" do
452+
ReactOnRails.configure do |config|
453+
config.immediate_hydration = true
454+
end
455+
456+
expect(Rails.logger).to have_received(:warn).once
457+
458+
ReactOnRails.configuration.immediate_hydration
459+
460+
# Still only one warning total
461+
expect(Rails.logger).to have_received(:warn).once
462+
end
463+
464+
it "does not warn again on setter if getter already warned" do
465+
ReactOnRails.configure {} # rubocop:disable Lint/EmptyBlock
466+
467+
ReactOnRails.configuration.immediate_hydration
468+
469+
expect(Rails.logger).to have_received(:warn).once
470+
471+
ReactOnRails.configure do |config|
472+
config.immediate_hydration = true
473+
end
474+
475+
# Still only one warning total
476+
expect(Rails.logger).to have_received(:warn).once
477+
end
478+
end
479+
end
480+
369481
describe "enforce_private_server_bundles validation" do
370482
context "when enforce_private_server_bundles is true" do
371483
before do

spec/react_on_rails/utils_spec.rb

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -898,6 +898,100 @@ def self.configuration=(config)
898898
end
899899

900900
# RSC utility method tests moved to react_on_rails_pro/spec/react_on_rails_pro/utils_spec.rb
901+
902+
describe ".normalize_immediate_hydration" do
903+
context "with Pro license" do
904+
before do
905+
allow(described_class).to receive(:react_on_rails_pro?).and_return(true)
906+
end
907+
908+
it "returns true when value is explicitly true" do
909+
result = described_class.normalize_immediate_hydration(true, "TestComponent", "Component")
910+
expect(result).to be true
911+
end
912+
913+
it "returns false when value is explicitly false" do
914+
result = described_class.normalize_immediate_hydration(false, "TestComponent", "Component")
915+
expect(result).to be false
916+
end
917+
918+
it "returns true when value is nil (Pro default)" do
919+
result = described_class.normalize_immediate_hydration(nil, "TestComponent", "Component")
920+
expect(result).to be true
921+
end
922+
923+
it "does not log a warning for any valid value" do
924+
expect(Rails.logger).not_to receive(:warn)
925+
926+
described_class.normalize_immediate_hydration(true, "TestComponent", "Component")
927+
described_class.normalize_immediate_hydration(false, "TestComponent", "Component")
928+
described_class.normalize_immediate_hydration(nil, "TestComponent", "Component")
929+
end
930+
end
931+
932+
context "without Pro license" do
933+
before do
934+
allow(described_class).to receive(:react_on_rails_pro?).and_return(false)
935+
end
936+
937+
it "returns false and logs warning when value is explicitly true" do
938+
expect(Rails.logger).to receive(:warn)
939+
.with(/immediate_hydration: true requires a React on Rails Pro license/)
940+
941+
result = described_class.normalize_immediate_hydration(true, "TestComponent", "Component")
942+
expect(result).to be false
943+
end
944+
945+
it "returns false when value is explicitly false" do
946+
expect(Rails.logger).not_to receive(:warn)
947+
948+
result = described_class.normalize_immediate_hydration(false, "TestComponent", "Component")
949+
expect(result).to be false
950+
end
951+
952+
it "returns false when value is nil (non-Pro default)" do
953+
expect(Rails.logger).not_to receive(:warn)
954+
955+
result = described_class.normalize_immediate_hydration(nil, "TestComponent", "Component")
956+
expect(result).to be false
957+
end
958+
959+
it "includes component name and type in warning message" do
960+
expect(Rails.logger).to receive(:warn) do |message|
961+
expect(message).to include("TestStore")
962+
expect(message).to include("Store")
963+
end
964+
965+
described_class.normalize_immediate_hydration(true, "TestStore", "Store")
966+
end
967+
end
968+
969+
context "with invalid values" do
970+
it "raises ArgumentError for string values" do
971+
expect do
972+
described_class.normalize_immediate_hydration("yes", "TestComponent", "Component")
973+
end.to raise_error(ArgumentError, /immediate_hydration must be true, false, or nil/)
974+
end
975+
976+
it "raises ArgumentError for numeric values" do
977+
expect do
978+
described_class.normalize_immediate_hydration(1, "TestComponent", "Component")
979+
end.to raise_error(ArgumentError, /immediate_hydration must be true, false, or nil/)
980+
end
981+
982+
it "raises ArgumentError for hash values" do
983+
expect do
984+
described_class.normalize_immediate_hydration({}, "TestComponent", "Component")
985+
end.to raise_error(ArgumentError, /immediate_hydration must be true, false, or nil/)
986+
end
987+
988+
it "includes the invalid value in error message" do
989+
expect do
990+
described_class.normalize_immediate_hydration("invalid", "TestComponent", "Component")
991+
end.to raise_error(ArgumentError, /Got: "invalid" \(String\)/)
992+
end
993+
end
994+
end
901995
end
902996
end
903997
# rubocop:enable Metrics/ModuleLength, Metrics/BlockLength

0 commit comments

Comments
 (0)