Skip to content

Commit 5740b0e

Browse files
Apply CodeRabbit review suggestions
- Extract process managers to PROCESS_MANAGERS constant for DRY code - Relax procfile path validation (remove shell metacharacter check) - Refactor tests to use around hook with ensure clause for proper cleanup - Close file descriptors properly to avoid leaks Co-authored-by: Abanoub Ghadban <AbanoubGhadban@users.noreply.github.com>
1 parent 153e965 commit 5740b0e

File tree

2 files changed

+25
-17
lines changed

2 files changed

+25
-17
lines changed

lib/react_on_rails/dev/process_manager.rb

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ class ProcessManager
88
# Timeout for version check operations to prevent hanging
99
VERSION_CHECK_TIMEOUT = 5
1010

11+
# Process managers in order of preference
12+
PROCESS_MANAGERS = %w[overmind foreman].freeze
13+
1114
class << self
1215
# Check if a process is available and usable in the current execution context
1316
# This accounts for bundler context where system commands might be intercepted
@@ -36,8 +39,9 @@ def run_with_process_manager(procfile)
3639
FileManager.cleanup_stale_files
3740

3841
# Try process managers in order of preference
39-
return if run_process_if_available("overmind", ["start", "-f", procfile])
40-
return if run_process_if_available("foreman", ["start", "-f", procfile])
42+
PROCESS_MANAGERS.each do |pm|
43+
return if run_process_if_available(pm, ["start", "-f", procfile])
44+
end
4145

4246
show_process_manager_installation_help
4347
exit 1
@@ -155,11 +159,9 @@ def show_process_manager_installation_help
155159
end
156160

157161
def valid_procfile_path?(procfile)
158-
# Reject paths with shell metacharacters
159-
return false if procfile.match?(/[;&|`$(){}\[\]<>]/)
160-
161-
# Ensure it's a readable file
162-
File.readable?(procfile)
162+
# system is invoked with args (no shell), so shell metacharacters are safe.
163+
# Ensure it's a readable regular file.
164+
File.file?(procfile) && File.readable?(procfile)
163165
rescue StandardError
164166
false
165167
end

spec/react_on_rails/dev/process_manager_spec.rb

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,22 @@
55

66
RSpec.describe ReactOnRails::Dev::ProcessManager do
77
# Suppress stdout/stderr during tests
8-
before(:all) do
9-
@original_stderr = $stderr
10-
@original_stdout = $stdout
11-
$stderr = File.open(File::NULL, "w")
12-
$stdout = File.open(File::NULL, "w")
13-
end
14-
15-
after(:all) do
16-
$stderr = @original_stderr
17-
$stdout = @original_stdout
8+
around do |example|
9+
original_stderr = $stderr
10+
original_stdout = $stdout
11+
null_stderr = File.open(File::NULL, "w")
12+
null_stdout = File.open(File::NULL, "w")
13+
14+
begin
15+
$stderr = null_stderr
16+
$stdout = null_stdout
17+
example.run
18+
ensure
19+
$stderr = original_stderr
20+
$stdout = original_stdout
21+
null_stderr.close
22+
null_stdout.close
23+
end
1824
end
1925

2026
describe ".installed?" do

0 commit comments

Comments
 (0)