Skip to content

Commit 0fc24b0

Browse files
justin808claude
andcommitted
Address code review feedback for verbose flag implementation
Based on thorough code review feedback, this commit addresses several improvements to make the implementation more robust and explicit: 1. **RBS Type Signatures**: Updated type signatures to include verbose parameter in run_pack_generation and run_via_bundle_exec methods. 2. **Explicit Parameter Passing**: Made verbose parameter explicit in verbose branch (line 41) for consistency and clarity. 3. **Environment Variable Propagation**: Made env var propagation explicit in bundle exec path using system's env hash parameter, ensuring the variable is properly passed to subprocess even when ENV context differs. 4. **Comprehensive Test Coverage**: Added 3 new tests to verify: - REACT_ON_RAILS_VERBOSE env var is set to "true" in verbose mode - REACT_ON_RAILS_VERBOSE env var is set to "false" in quiet mode - Environment variable cleanup happens even on failure - All bundle exec calls include the env var in the system call This ensures the verbose mode propagation is reliable across all execution paths (direct rake task and bundle exec). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 58b5f82 commit 0fc24b0

File tree

3 files changed

+65
-13
lines changed

3 files changed

+65
-13
lines changed

lib/react_on_rails/dev/pack_generator.rb

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ def generate(verbose: false)
3838

3939
if verbose
4040
puts "📦 Generating React on Rails packs..."
41-
success = run_pack_generation(verbose: true)
41+
success = run_pack_generation(silent: false, verbose: true)
4242
else
4343
print "📦 Generating packs... "
4444
success = run_pack_generation(silent: true, verbose: false)
@@ -67,7 +67,7 @@ def run_pack_generation(silent: false, verbose: false)
6767
if should_run_directly?
6868
run_rake_task_directly(silent: silent)
6969
else
70-
run_via_bundle_exec(silent: silent)
70+
run_via_bundle_exec(silent: silent, verbose: verbose)
7171
end
7272
ensure
7373
# Clean up environment variable
@@ -151,17 +151,22 @@ def handle_rake_error(error, _silent)
151151
# rubocop:enable Style/StderrPuts, Style/GlobalStdStream
152152
end
153153

154-
def run_via_bundle_exec(silent: false)
154+
def run_via_bundle_exec(silent: false, verbose: false)
155+
# Environment variable is already set in run_pack_generation, but we make it explicit here
156+
# for clarity and to ensure it's passed to the subprocess
157+
env = { "REACT_ON_RAILS_VERBOSE" => verbose ? "true" : "false" }
158+
155159
# Need to unbundle to prevent Bundler from intercepting our bundle exec call
156160
# when already running inside a Bundler context (e.g., from bin/dev)
157161
with_unbundled_context do
158162
if silent
159163
system(
164+
env,
160165
"bundle", "exec", "rake", "react_on_rails:generate_packs",
161166
out: File::NULL, err: File::NULL
162167
)
163168
else
164-
system("bundle", "exec", "rake", "react_on_rails:generate_packs")
169+
system(env, "bundle", "exec", "rake", "react_on_rails:generate_packs")
165170
end
166171
end
167172
end

sig/react_on_rails/dev/pack_generator.rbs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,15 @@ module ReactOnRails
55

66
private
77

8-
def self.run_pack_generation: (?silent: bool) -> bool
8+
def self.run_pack_generation: (?silent: bool, ?verbose: bool) -> bool
99
def self.should_run_directly?: () -> bool
1010
def self.rails_available?: () -> bool
1111
def self.run_rake_task_directly: (?silent: bool) -> bool
1212
def self.load_rake_tasks: () -> void
1313
def self.prepare_rake_task: () -> untyped
1414
def self.capture_output: (bool) { () -> bool } -> bool
1515
def self.handle_rake_error: (Exception, bool) -> void
16-
def self.run_via_bundle_exec: (?silent: bool) -> (bool | nil)
16+
def self.run_via_bundle_exec: (?silent: bool, ?verbose: bool) -> (bool | nil)
1717
end
1818
end
1919
end

spec/react_on_rails/dev/pack_generator_spec.rb

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,46 @@
177177
expect { described_class.generate(verbose: false) }
178178
.not_to output(/This should be suppressed/).to_stdout_from_any_process
179179
end
180+
181+
it "sets REACT_ON_RAILS_VERBOSE environment variable when verbose is true" do
182+
env_value = nil
183+
allow(mock_task).to receive(:invoke) do
184+
env_value = ENV.fetch("REACT_ON_RAILS_VERBOSE", nil)
185+
end
186+
187+
described_class.generate(verbose: true)
188+
189+
expect(env_value).to eq("true")
190+
# Ensure cleanup happened
191+
expect(ENV.fetch("REACT_ON_RAILS_VERBOSE", nil)).to be_nil
192+
end
193+
194+
it "sets REACT_ON_RAILS_VERBOSE environment variable to false when verbose is false" do
195+
env_value = nil
196+
allow(mock_task).to receive(:invoke) do
197+
env_value = ENV.fetch("REACT_ON_RAILS_VERBOSE", nil)
198+
end
199+
200+
described_class.generate(verbose: false)
201+
202+
expect(env_value).to eq("false")
203+
# Ensure cleanup happened
204+
expect(ENV.fetch("REACT_ON_RAILS_VERBOSE", nil)).to be_nil
205+
end
206+
207+
it "cleans up REACT_ON_RAILS_VERBOSE environment variable even when task fails" do
208+
allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Task failed"))
209+
210+
# Mock STDERR.puts to suppress error output
211+
# rubocop:disable Style/GlobalStdStream
212+
allow(STDERR).to receive(:puts)
213+
# rubocop:enable Style/GlobalStdStream
214+
215+
expect { described_class.generate(verbose: true) }.to raise_error(SystemExit)
216+
217+
# Ensure cleanup happened even on failure
218+
expect(ENV.fetch("REACT_ON_RAILS_VERBOSE", nil)).to be_nil
219+
end
180220
end
181221

182222
context "when not in Bundler context" do
@@ -187,33 +227,38 @@
187227

188228
it "runs pack generation successfully in verbose mode using bundle exec" do
189229
allow(described_class).to receive(:system)
190-
.with("bundle", "exec", "rake", "react_on_rails:generate_packs")
230+
.with({ "REACT_ON_RAILS_VERBOSE" => "true" },
231+
"bundle", "exec", "rake", "react_on_rails:generate_packs")
191232
.and_return(true)
192233

193234
expect { described_class.generate(verbose: true) }
194235
.to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process
195236

196237
expect(described_class).to have_received(:system)
197-
.with("bundle", "exec", "rake", "react_on_rails:generate_packs")
238+
.with({ "REACT_ON_RAILS_VERBOSE" => "true" },
239+
"bundle", "exec", "rake", "react_on_rails:generate_packs")
198240
end
199241

200242
it "runs pack generation successfully in quiet mode using bundle exec" do
201243
allow(described_class).to receive(:system)
202-
.with("bundle", "exec", "rake", "react_on_rails:generate_packs",
244+
.with({ "REACT_ON_RAILS_VERBOSE" => "false" },
245+
"bundle", "exec", "rake", "react_on_rails:generate_packs",
203246
out: File::NULL, err: File::NULL)
204247
.and_return(true)
205248

206249
expect { described_class.generate(verbose: false) }
207250
.to output(/📦 Generating packs\.\.\. ✅/).to_stdout_from_any_process
208251

209252
expect(described_class).to have_received(:system)
210-
.with("bundle", "exec", "rake", "react_on_rails:generate_packs",
253+
.with({ "REACT_ON_RAILS_VERBOSE" => "false" },
254+
"bundle", "exec", "rake", "react_on_rails:generate_packs",
211255
out: File::NULL, err: File::NULL)
212256
end
213257

214258
it "exits with error when pack generation fails" do
215259
allow(described_class).to receive(:system)
216-
.with("bundle", "exec", "rake", "react_on_rails:generate_packs",
260+
.with({ "REACT_ON_RAILS_VERBOSE" => "false" },
261+
"bundle", "exec", "rake", "react_on_rails:generate_packs",
217262
out: File::NULL, err: File::NULL)
218263
.and_return(false)
219264

@@ -233,14 +278,16 @@
233278

234279
it "falls back to bundle exec when Rails is not defined" do
235280
allow(described_class).to receive(:system)
236-
.with("bundle", "exec", "rake", "react_on_rails:generate_packs")
281+
.with({ "REACT_ON_RAILS_VERBOSE" => "true" },
282+
"bundle", "exec", "rake", "react_on_rails:generate_packs")
237283
.and_return(true)
238284

239285
expect { described_class.generate(verbose: true) }
240286
.to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process
241287

242288
expect(described_class).to have_received(:system)
243-
.with("bundle", "exec", "rake", "react_on_rails:generate_packs")
289+
.with({ "REACT_ON_RAILS_VERBOSE" => "true" },
290+
"bundle", "exec", "rake", "react_on_rails:generate_packs")
244291
end
245292
end
246293

0 commit comments

Comments
 (0)