Skip to content

Commit 20e1f3d

Browse files
authored
Fix auto-bundled component pack normalization (#3818)
## Summary - Normalize auto-bundled component pack loading through `RenderOptions#react_component_name`, so `react_component("component_name")` loads `generated/ComponentName` consistently with DOM/SSR lookup. - Camelize generated public component names for actual files under the configured auto-bundling component subdirectory, while preserving server-bundle and store filename behavior. - Preserve component/store generated-pack conflict protection for names that now differ only by case. Fixes #3809. ## Validation - `bundle exec rspec spec/helpers/react_on_rails_helper_spec.rb:63 spec/packs_generator_spec.rb:1414` from `react_on_rails/spec/dummy` -> 12 examples, 0 failures. - `bundle exec rspec spec/packs_generator_spec.rb` from `react_on_rails/spec/dummy` -> 157 examples, 0 failures. - `bundle exec rubocop lib/react_on_rails/helper.rb lib/react_on_rails/packs_generator.rb spec/dummy/spec/helpers/react_on_rails_helper_spec.rb spec/dummy/spec/packs_generator_spec.rb` from `react_on_rails` -> 4 files, no offenses. - `bundle exec rubocop` from `react_on_rails` -> 214 files, no offenses. - `git diff --check` -> passed. - `script/ci-changes-detector origin/main` -> Ruby core source code + dummy app; recommended broad CI set. - Pre-commit hook -> trailing newlines, autofix, RuboCop passed. - Pre-push hook -> branch Ruby lint and markdown-links passed. Known validation note: full `spec/helpers/react_on_rails_helper_spec.rb` reaches unrelated SSR examples that require `public/webpack/test/server-bundle.js`; the focused changed helper group passed. A `codex review --base origin/main` run was started, inspected the diff and Pro call sites, then was stopped after timing out without final findings; its attempted broad helper spec also hit sandbox-only Capybara port binding failures. ## Labels Labels: full-ci Reason: this is a small diff, but it affects runtime auto-bundled pack loading and generated pack naming, so path-based full CI is appropriate for generator/dummy-app coverage. No `benchmark` label recommended; the change does not affect performance-sensitive rendering or benchmark code paths. ## Release Mode Current live tracker read: #3570 appears to be the active central RC tracker, but it has no `Agent Release Mode` block. Per AGENTS.md, treating merge decisions as `strict-rc` and not auto-merging. ## Agent Merge Confidence Mode: strict-rc Score: 7/10 Auto-merge recommendation: no Affected areas: auto-bundling, generated pack loading, dummy app specs CI detector: `script/ci-changes-detector origin/main` -> Ruby core source code + dummy app; broad CI recommended Validation run: - `bundle exec rspec spec/helpers/react_on_rails_helper_spec.rb:63 spec/packs_generator_spec.rb:1414` -> passed, 12 examples - `bundle exec rspec spec/packs_generator_spec.rb` -> passed, 157 examples - `bundle exec rubocop` in `react_on_rails` -> passed, 214 files - focused RuboCop on touched files -> passed, 4 files - `git diff --check` -> passed Review/check gate: - Local self-review: complete - Codex review: timed out/stopped after exploratory output; no final findings emitted - GitHub checks: pending PR creation Known residual risk: full CI not yet complete; full helper spec needs generated SSR bundle assets and was not used as local pass/fail evidence Finalized by: not independently finalized <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes runtime generated pack paths and public component names for auto-bundled apps; mis-scoped camelization could break existing snake_case registrations until packs are regenerated. > > **Overview** > Aligns **auto-bundled** pack naming and loading so snake_case view names (e.g. `react_component("component_name")`) resolve to the same **camelized** public name as generated packs (`generated/ComponentName`). > > **`load_pack_for_generated_component`** now uses `RenderOptions#react_component_name` for dev existence checks and `append_*_pack_tag` paths instead of the raw helper argument, matching SSR/DOM registration. > > **`PacksGenerator#component_name`** camelizes basenames only for component files under the configured `components_subdirectory`; files outside that path keep the previous basename behavior. Component/store conflict detection is **case-insensitive**, with clearer error text when casing differs. > > Specs cover camelized pack tags, snake_case naming rules, and case-only component/store conflicts. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 481b108. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Component/store name conflict detection is now case-insensitive, catching more naming issues. * Component name normalization improved for snake_case and client/server filename variants for consistent runtime names. * **Tests** * Updated specs to reflect the refined naming behavior and generated pack expectations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 38835e9 commit 20e1f3d

4 files changed

Lines changed: 72 additions & 20 deletions

File tree

react_on_rails/lib/react_on_rails/helper.rb

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -394,22 +394,23 @@ def add_csp_nonce_to_context(result)
394394
result[:cspNonce] = nonce if nonce.present?
395395
end
396396

397-
def load_pack_for_generated_component(react_component_name, render_options)
397+
def load_pack_for_generated_component(_react_component_name, render_options)
398398
return unless render_options.auto_load_bundle
399399

400400
ReactOnRails::PackerUtils.raise_nested_entries_disabled unless ReactOnRails::PackerUtils.nested_entries?
401+
generated_component_name = render_options.react_component_name
401402
if Rails.env.development?
402-
is_component_pack_present = File.exist?(generated_components_pack_path(react_component_name))
403-
raise_missing_autoloaded_bundle(react_component_name) unless is_component_pack_present
403+
is_component_pack_present = File.exist?(generated_components_pack_path(generated_component_name))
404+
raise_missing_autoloaded_bundle(generated_component_name) unless is_component_pack_present
404405
end
405406

406407
options = { defer: ReactOnRails.configuration.generated_component_packs_loading_strategy == :defer }
407408
# Old versions of Shakapacker don't support async script tags.
408409
# ReactOnRails.configure already validates if async loading is supported by the installed Shakapacker version.
409410
# Therefore, we only need to pass the async option if the loading strategy is explicitly set to :async
410411
options[:async] = true if ReactOnRails.configuration.generated_component_packs_loading_strategy == :async
411-
append_javascript_pack_tag("generated/#{react_component_name}", **options)
412-
append_stylesheet_pack_tag("generated/#{react_component_name}")
412+
append_javascript_pack_tag("generated/#{generated_component_name}", **options)
413+
append_stylesheet_pack_tag("generated/#{generated_component_name}")
413414
end
414415

415416
def load_pack_for_generated_store(store_name, explicit_auto_load: false)

react_on_rails/lib/react_on_rails/packs_generator.rb

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,12 @@ def log_rsc_classification_summary
174174
def check_for_component_store_name_conflicts
175175
component_names = common_component_to_path.keys + client_component_to_path.keys
176176
store_names = store_to_path.keys
177-
conflicts = component_names & store_names
177+
conflicts = component_names.filter_map do |component_name|
178+
store_name = store_names.find { |name| component_name.casecmp?(name) }
179+
next unless store_name
180+
181+
component_name == store_name ? component_name : "#{component_name} (component) / #{store_name} (store)"
182+
end
178183

179184
return if conflicts.empty?
180185

@@ -715,8 +720,19 @@ def generated_pack_path(file_path)
715720

716721
def component_name(file_path)
717722
basename = File.basename(file_path, File.extname(file_path))
723+
derived_name = basename.sub(CONTAINS_CLIENT_OR_SERVER_REGEX, "")
718724

719-
basename.sub(CONTAINS_CLIENT_OR_SERVER_REGEX, "")
725+
return derived_name unless auto_bundled_component_file?(file_path)
726+
727+
derived_name.camelize
728+
end
729+
730+
def auto_bundled_component_file?(file_path)
731+
components_subdirectory = ReactOnRails.configuration.components_subdirectory
732+
return false if components_subdirectory.blank?
733+
return false unless file_path.match?(COMPONENT_EXTENSIONS)
734+
735+
Pathname(file_path).each_filename.include?(components_subdirectory)
720736
end
721737

722738
def component_name_to_path(paths)

react_on_rails/spec/dummy/spec/helpers/react_on_rails_helper_spec.rb

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -61,20 +61,23 @@ def self.pro_attribution_comment
6161
end
6262

6363
describe "#load_pack_for_generated_component" do
64+
let(:react_component_name) { "component_name" }
65+
let(:generated_component_name) { "ComponentName" }
6466
let(:render_options) do
65-
ReactOnRails::ReactComponent::RenderOptions.new(react_component_name: "component_name",
67+
ReactOnRails::ReactComponent::RenderOptions.new(react_component_name:,
6668
options: {})
6769
end
6870

6971
it "appends js/css pack tag" do
7072
allow(helper).to receive(:append_javascript_pack_tag)
7173
allow(helper).to receive(:append_stylesheet_pack_tag)
72-
expect { helper.load_pack_for_generated_component("component_name", render_options) }.not_to raise_error
74+
expect { helper.load_pack_for_generated_component(react_component_name, render_options) }.not_to raise_error
7375

7476
# Default loading strategy is now always :defer to prevent race conditions
7577
# between component registration and hydration, regardless of async support
76-
expect(helper).to have_received(:append_javascript_pack_tag).with("generated/component_name", { defer: true })
77-
expect(helper).to have_received(:append_stylesheet_pack_tag).with("generated/component_name")
78+
expect(helper).to have_received(:append_javascript_pack_tag).with("generated/#{generated_component_name}",
79+
{ defer: true })
80+
expect(helper).to have_received(:append_stylesheet_pack_tag).with("generated/#{generated_component_name}")
7881
end
7982

8083
context "when async loading is enabled" do
@@ -95,12 +98,12 @@ def helper.append_javascript_pack_tag(name, **options)
9598

9699
allow(helper).to receive(:append_javascript_pack_tag)
97100
allow(helper).to receive(:append_stylesheet_pack_tag)
98-
expect { helper.load_pack_for_generated_component("component_name", render_options) }.not_to raise_error
101+
expect { helper.load_pack_for_generated_component(react_component_name, render_options) }.not_to raise_error
99102
expect(helper).to have_received(:append_javascript_pack_tag).with(
100-
"generated/component_name",
103+
"generated/#{generated_component_name}",
101104
{ defer: false, async: true }
102105
)
103-
expect(helper).to have_received(:append_stylesheet_pack_tag).with("generated/component_name")
106+
expect(helper).to have_received(:append_stylesheet_pack_tag).with("generated/#{generated_component_name}")
104107
ensure
105108
helper.define_singleton_method(:append_javascript_pack_tag, original_append_javascript_pack_tag)
106109
end
@@ -115,15 +118,20 @@ def helper.append_javascript_pack_tag(name, **options)
115118
it "appends the defer attribute to the script tag" do
116119
allow(helper).to receive(:append_javascript_pack_tag)
117120
allow(helper).to receive(:append_stylesheet_pack_tag)
118-
expect { helper.load_pack_for_generated_component("component_name", render_options) }.not_to raise_error
119-
expect(helper).to have_received(:append_javascript_pack_tag).with("generated/component_name", { defer: true })
120-
expect(helper).to have_received(:append_stylesheet_pack_tag).with("generated/component_name")
121+
expect { helper.load_pack_for_generated_component(react_component_name, render_options) }.not_to raise_error
122+
expect(helper).to have_received(:append_javascript_pack_tag).with("generated/#{generated_component_name}",
123+
{ defer: true })
124+
expect(helper).to have_received(:append_stylesheet_pack_tag).with("generated/#{generated_component_name}")
121125
end
122126
end
123127

124128
it "throws an error in development if generated component isn't found" do
129+
missing_render_options = ReactOnRails::ReactComponent::RenderOptions.new(
130+
react_component_name: "nonexisting_component",
131+
options: {}
132+
)
125133
allow(Rails.env).to receive(:development?).and_return(true)
126-
expect { helper.load_pack_for_generated_component("nonexisting_component", render_options) }
134+
expect { helper.load_pack_for_generated_component("nonexisting_component", missing_render_options) }
127135
.to raise_error(ReactOnRails::SmartError, /Auto-loaded Bundle Missing/)
128136
end
129137
end

react_on_rails/spec/dummy/spec/packs_generator_spec.rb

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1162,9 +1162,12 @@ def create_new_component(name)
11621162
.and_return("#{stores_fixture_path}/StoreWithNameConflict")
11631163
end
11641164

1165-
it "raises an error for name conflict" do
1165+
it "raises an error for case-only name conflict" do
11661166
expect { described_class.instance.generate_packs_if_stale }
1167-
.to raise_error(ReactOnRails::Error, /names are used for both components and stores/)
1167+
.to raise_error(
1168+
ReactOnRails::Error,
1169+
%r{Conflicting \(component\) / conflicting \(store\)}
1170+
)
11681171
end
11691172
end
11701173
end
@@ -1420,18 +1423,42 @@ def stub_packer_source_path(packer_source_path:, component_name:)
14201423
it { is_expected.to eq "MyComponent" }
14211424
end
14221425

1426+
context "with snake_case component file" do
1427+
let(:file_path) { "/path/to/ror_components/my_component.jsx" }
1428+
1429+
it { is_expected.to eq "MyComponent" }
1430+
end
1431+
1432+
context "with snake_case component file outside components_subdirectory" do
1433+
let(:file_path) { "/path/to/other_dir/my_component.jsx" }
1434+
1435+
it { is_expected.to eq "my_component" }
1436+
end
1437+
14231438
context "with client component file" do
14241439
let(:file_path) { "/path/to/MyComponent.client.jsx" }
14251440

14261441
it { is_expected.to eq "MyComponent" }
14271442
end
14281443

1444+
context "with snake_case client component file" do
1445+
let(:file_path) { "/path/to/ror_components/my_component.client.jsx" }
1446+
1447+
it { is_expected.to eq "MyComponent" }
1448+
end
1449+
14291450
context "with server component file" do
14301451
let(:file_path) { "/path/to/MyComponent.server.jsx" }
14311452

14321453
it { is_expected.to eq "MyComponent" }
14331454
end
14341455

1456+
context "with snake_case server component file" do
1457+
let(:file_path) { "/path/to/ror_components/my_component.server.jsx" }
1458+
1459+
it { is_expected.to eq "MyComponent" }
1460+
end
1461+
14351462
context "with CSS module file" do
14361463
let(:file_path) { "/path/to/HeavyMarkdownEditor.module.css" }
14371464

0 commit comments

Comments
 (0)