Skip to content

Commit 3c18116

Browse files
AbanoubGhadbanclaude
authored andcommitted
Fix ci-changes-detector skipping pro jobs for node renderer changes (#2475)
## Summary - Add `PRO_NODE_RENDERER_CHANGED` to the `RUN_PRO_LINT` and `RUN_PRO_TESTS` conditions in `script/ci-changes-detector` so that PRs touching only `packages/react-on-rails-pro-node-renderer/` correctly trigger pro lint, pro package tests, and pro integration tests - Add `PRO_NODE_RENDERER_CHANGED=true` to the CI infrastructure catch-all for consistency - Add `run_pro_node_renderer_tests:false` to the docs-only output path for consistency ## Test plan - [x] Verified bash syntax passes (`bash -n`) - [x] Simulated node-renderer-only change locally — `RUN_PRO_LINT`, `RUN_PRO_TESTS`, and `RUN_PRO_NODE_RENDERER_TESTS` all correctly set to `true` - [ ] CI workflows on this PR should demonstrate correct detection (this PR changes `script/ci-changes-detector` which is CI infrastructure, so all jobs trigger) Fixes #2471 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * CI now detects changes to the Pro Node Renderer and exposes a dedicated change flag. * CI decision logic was extended so Pro Node Renderer changes gate and trigger the appropriate Pro Node Renderer test runs. * Test run decisions and summaries now include the Pro Node Renderer flag and appear in CI outputs and final summaries (including JSON/GitHub outputs). * Change-detection categories and reporting were expanded to cover Node Renderer updates. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 11998fb commit 3c18116

File tree

6 files changed

+108
-14
lines changed

6 files changed

+108
-14
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ Changes since the last non-beta release.
3131

3232
##### Fixed
3333

34+
- **Fixed ScoutApm instrumentation depending on Gemfile ordering**. ScoutApm instrumentation for `react_component`, `react_component_hash`, and `exec_server_render_js` was previously installed at gem load time using `defined?(ScoutApm)` guards, which meant it was silently skipped if `scout_apm` appeared after `react_on_rails` in the Gemfile, and produced noisy INFO log messages if it appeared before (since ScoutApm wasn't yet initialized). Moved instrumentation into an initializer that runs after `scout_apm.start`, ensuring it works regardless of gem ordering and only after ScoutApm is fully configured. [PR 2442](https://github.com/shakacode/react_on_rails/pull/2442) by [tonyta](https://github.com/tonyta).
3435
- **Fixed node renderer upload race condition causing ENOENT errors and asset corruption during concurrent requests**. Concurrent multipart uploads (e.g., during pod rollovers) all wrote to a single shared path (`uploads/<filename>`), causing file overwrites, `ENOENT` errors, and cross-contamination between requests. Each request now gets its own isolated upload directory (`uploads/<uuid>/`), eliminating all shared-path collisions. [PR 2456](https://github.com/shakacode/react_on_rails/pull/2456) by [AbanoubGhadban](https://github.com/AbanoubGhadban).
3536
- **Fixed node renderer race condition between `/upload-assets` and render requests writing to the same bundle directory**. The `/upload-assets` endpoint used a global lock while render requests used per-bundle locks, so both could write to the same bundle directory concurrently, risking asset corruption. Now both endpoints share the same per-bundle lock key. Also switched parallel bundle processing from `Promise.all` to `Promise.allSettled` to prevent the `onResponse` cleanup hook from deleting uploaded files while in-flight copies are still reading from them. [PR 2464](https://github.com/shakacode/react_on_rails/pull/2464) by [AbanoubGhadban](https://github.com/AbanoubGhadban).
3637

react_on_rails/lib/react_on_rails/engine.rb

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,23 @@ def self.package_json_missing?
8383
!File.exist?(VersionChecker::NodePackageVersion.package_json_path)
8484
end
8585

86+
# Install ScoutApm instrumentation after ScoutApm is configured via "scout_apm.start" initializer.
87+
# https://github.com/scoutapp/scout_apm_ruby/blob/v6.1.0/lib/scout_apm.rb#L221
88+
initializer "react_on_rails.scout_apm_instrumentation", after: "scout_apm.start" do
89+
next unless defined?(ScoutApm)
90+
91+
ReactOnRails::Helper.class_eval do
92+
include ScoutApm::Tracer
93+
instrument_method :react_component, type: "ReactOnRails", name: "react_component"
94+
instrument_method :react_component_hash, type: "ReactOnRails", name: "react_component_hash"
95+
end
96+
97+
ReactOnRails::ServerRenderingPool::RubyEmbeddedJavaScript.singleton_class.class_eval do
98+
include ScoutApm::Tracer
99+
instrument_method :exec_server_render_js, type: "ReactOnRails", name: "ExecJs React Server Rendering"
100+
end
101+
end
102+
86103
config.to_prepare do
87104
ReactOnRails::ServerRenderingPool.reset_pool
88105
end

react_on_rails/lib/react_on_rails/helper.rb

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -714,13 +714,6 @@ def in_mailer?
714714
controller.is_a?(ActionMailer::Base)
715715
end
716716

717-
if defined?(ScoutApm)
718-
include ScoutApm::Tracer
719-
720-
instrument_method :react_component, type: "ReactOnRails", name: "react_component"
721-
instrument_method :react_component_hash, type: "ReactOnRails", name: "react_component_hash"
722-
end
723-
724717
def raise_missing_autoloaded_bundle(react_component_name)
725718
raise ReactOnRails::SmartError.new(
726719
error_type: :missing_auto_loaded_bundle,

react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -211,12 +211,6 @@ def console_polyfill
211211
JS
212212
end
213213

214-
if defined?(ScoutApm)
215-
include ScoutApm::Tracer
216-
217-
instrument_method :exec_server_render_js, type: "ReactOnRails", name: "ExecJs React Server Rendering"
218-
end
219-
220214
private
221215

222216
def file_url_to_string(url)

react_on_rails/spec/react_on_rails/engine_spec.rb

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,93 @@ module ReactOnRails
166166
end
167167
end
168168

169+
describe "ScoutApm instrumentation initializer" do
170+
subject(:initializer) { described_class.initializers.find { |i| i.name.include?("scout_apm") } }
171+
172+
it "defines a named Rails initializer to run after scout_apm.start" do
173+
expect(initializer.name).to eq "react_on_rails.scout_apm_instrumentation"
174+
expect(initializer.after).to eq "scout_apm.start"
175+
end
176+
177+
describe "react_on_rails.scout_apm_instrumentation" do
178+
let(:mock_scout_tracer) do
179+
#
180+
# Simplified mock of ScoutApm::Tracer that mirrors its real implementation.
181+
# https://github.com/scoutapp/scout_apm_ruby/blob/v6.1.0/lib/scout_apm/tracer.rb#L47-L70
182+
#
183+
Module.new do
184+
def self.included(base)
185+
base.define_singleton_method(:instrument_method) do |method_name, **|
186+
raise "method does not exist: #{method_name}" unless method_defined?(method_name)
187+
188+
instrumented_name = :"#{method_name}_with_test_instrument"
189+
uninstrumented_name = :"#{method_name}_without_test_instrument"
190+
191+
define_method(instrumented_name) do |*args, **kwargs, &block|
192+
send(uninstrumented_name, *args, **kwargs, &block)
193+
end
194+
195+
alias_method uninstrumented_name, method_name
196+
alias_method method_name, instrumented_name
197+
end
198+
end
199+
end
200+
end
201+
202+
let(:mock_helper) { Module.new.include(ReactOnRails::Helper) }
203+
let(:mock_rb_embedded_js) { Class.new(ReactOnRails::ServerRenderingPool::RubyEmbeddedJavaScript) }
204+
205+
before do
206+
stub_const("ReactOnRails::Helper", mock_helper)
207+
stub_const("ReactOnRails::ServerRenderingPool::RubyEmbeddedJavaScript", mock_rb_embedded_js)
208+
end
209+
210+
context "when ScoutApm is not defined" do
211+
before { hide_const("ScoutApm") }
212+
213+
it "does not instrument Helper#react_component" do
214+
initializer.run
215+
expect(mock_helper.instance_methods(false)).not_to include(:react_component_with_test_instrument)
216+
expect(mock_helper.instance_methods(false)).not_to include(:react_component_without_test_instrument)
217+
end
218+
219+
it "does not instrument Helper#react_component_hash" do
220+
initializer.run
221+
expect(mock_helper.instance_methods(false)).not_to include(:react_component_hash_with_test_instrument)
222+
expect(mock_helper.instance_methods(false)).not_to include(:react_component_hash_without_test_instrument)
223+
end
224+
225+
it "does not instrument RubyEmbeddedJavaScript.exec_server_render_js" do
226+
initializer.run
227+
expect(mock_rb_embedded_js.methods(false)).not_to include(:exec_server_render_js_with_test_instrument)
228+
expect(mock_rb_embedded_js.methods(false)).not_to include(:exec_server_render_js_without_test_instrument)
229+
end
230+
end
231+
232+
context "when ScoutApm is defined" do
233+
before { stub_const("ScoutApm::Tracer", mock_scout_tracer) }
234+
235+
it "instruments Helper#react_component" do
236+
initializer.run
237+
expect(mock_helper.instance_methods(false)).to include(:react_component_with_test_instrument)
238+
expect(mock_helper.instance_methods(false)).to include(:react_component_without_test_instrument)
239+
end
240+
241+
it "instruments Helper#react_component_hash" do
242+
initializer.run
243+
expect(mock_helper.instance_methods(false)).to include(:react_component_hash_with_test_instrument)
244+
expect(mock_helper.instance_methods(false)).to include(:react_component_hash_without_test_instrument)
245+
end
246+
247+
it "instruments RubyEmbeddedJavaScript.exec_server_render_js" do
248+
initializer.run
249+
expect(mock_rb_embedded_js.methods(false)).to include(:exec_server_render_js_with_test_instrument)
250+
expect(mock_rb_embedded_js.methods(false)).to include(:exec_server_render_js_without_test_instrument)
251+
end
252+
end
253+
end
254+
end
255+
169256
describe "automatic rake task loading" do
170257
# Rails::Engine automatically loads all .rake files from lib/tasks/
171258
# This test verifies that our rake tasks are loaded without needing

script/ci-changes-detector

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ while IFS= read -r file; do
158158
PRO_RUBY_CORE_CHANGED=true
159159
PRO_JS_CHANGED=true
160160
PRO_DUMMY_CHANGED=true
161+
PRO_NODE_RENDERER_CHANGED=true
161162
;;
162163

163164
# Catch-all: any file not explicitly categorized above
@@ -188,6 +189,7 @@ if [ "$DOCS_ONLY" = true ]; then
188189
"run_pro_lint:false"
189190
"run_pro_tests:false"
190191
"run_pro_dummy_tests:false"
192+
"run_pro_node_renderer_tests:false"
191193
)
192194

193195
# Output to GITHUB_OUTPUT if in GitHub Actions
@@ -281,7 +283,7 @@ if [ "$PRO_RUBY_CORE_CHANGED" = true ] || [ "$PRO_RSPEC_CHANGED" = true ] || [ "
281283
RUN_PRO_TESTS=true
282284
fi
283285

284-
if [ "$PRO_DUMMY_CHANGED" = true ] || [ "$PRO_RUBY_CORE_CHANGED" = true ] || [ "$PRO_JS_CHANGED" = true ] || [ "$RUBY_CORE_CHANGED" = true ]; then
286+
if [ "$PRO_DUMMY_CHANGED" = true ] || [ "$PRO_RUBY_CORE_CHANGED" = true ] || [ "$PRO_JS_CHANGED" = true ] || [ "$PRO_NODE_RENDERER_CHANGED" = true ] || [ "$RUBY_CORE_CHANGED" = true ]; then
285287
RUN_PRO_DUMMY_TESTS=true
286288
fi
287289

0 commit comments

Comments
 (0)