Enhance bin/dev kill to terminate processes on ports 3000 and 3001

[justin808]

Summary

Enhances bin/dev kill to also terminate any process listening on ports 3000 and 3001, ensuring a clean restart even when unexpected processes are blocking the development ports.

Previously, bin/dev kill only terminated processes by pattern matching (e.g., "rails", "puma", "webpack-dev-server"). This could miss processes that were holding ports 3000 or 3001 but didn't match the patterns.

Key Improvements

• Port-based killing: Now finds and terminates any process listening on ports 3000 and 3001 using lsof -ti:PORT
• Comprehensive coverage: Combines pattern-based and port-based process detection for complete cleanup
• Graceful error handling: Handles edge cases like lsof not being available or permission issues
• Default behavior: Integrated seamlessly into existing bin/dev kill command (no new flags needed)

Implementation Details

New Methods

1. kill_port_processes(ports) - Iterates through ports, finds PIDs, and terminates them
2. find_port_pids(port) - Uses lsof -ti:PORT to find PIDs listening on a specific port
   • Excludes current process PID to avoid self-termination
   • Returns empty array on errors (command not found, permission denied)

Updated Methods

• kill_processes - Now calls kill_port_processes([3000, 3001]) in addition to pattern-based killing

Tests Added

• ✅ Kills processes on ports 3000 and 3001
• ✅ Finds PIDs listening on specific ports
• ✅ Excludes current process PID
• ✅ Handles lsof command not found
• ✅ Handles permission denied errors
• ✅ Returns false when no processes found on ports

Test Plan

• All existing tests pass
• New tests added and passing (17 examples, 0 failures)
• RuboCop checks pass with no offenses
• Pre-commit hooks pass
• Manual testing: bin/dev kill successfully terminates processes on development ports

Files Changed

• lib/react_on_rails/dev/server_manager.rb - Added port-killing functionality
• spec/react_on_rails/dev/server_manager_spec.rb - Added comprehensive tests

Design Decision

Why default behavior instead of --force flag?

• Port-based killing is already targeted and precise (ports 3000/3001)
• Maintains consistency with current UX (no confirmation needed)
• Addresses the common use case: "Kill whatever is blocking my dev ports"
• Ports 3000/3001 are explicitly development ports, making this safe by default

🤖 Generated with Claude Code

---

This change is 

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 45 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 62d92e1 and cc0b116.

📒 Files selected for processing (2)

• lib/react_on_rails/dev/server_manager.rb (2 hunks)
• spec/react_on_rails/dev/server_manager_spec.rb (3 hunks)

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch update-bin-dev-kill-anything-using-port-3000-3001

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #1846

✅ Summary

This PR successfully enhances bin/dev kill to terminate processes listening on ports 3000 and 3001, addressing a common pain point where non-matching processes block development ports. The implementation is well-designed with good error handling and comprehensive test coverage.

---

🎯 Strengths

1. Clean Implementation

• The new methods (kill_port_processes and find_port_pids) follow the existing code patterns perfectly
• Graceful degradation when lsof is unavailable
• Proper integration into the existing kill chain: kill_running_processes || kill_port_processes([3000, 3001]) || cleanup_socket_files

2. Excellent Test Coverage ✅

All critical paths are tested:

• ✅ Finding PIDs on specific ports
• ✅ Excluding current process PID (prevents self-termination)
• ✅ Handling lsof command not found (Errno::ENOENT)
• ✅ Handling permission denied errors (Errno::EACCES)
• ✅ Multiple PIDs on single port (port 3001 test with newline-separated PIDs)
• ✅ Integration with main kill_processes method

3. Good Design Decisions

• Broad exception catching (rescue StandardError) in find_port_pids:93-98 is appropriate here - failures should be silent and graceful
• Process.pid exclusion at line 95 prevents accidental self-termination
• Port-based killing as default behavior - good UX decision since ports 3000/3001 are explicitly dev ports

4. Code Quality

• Follows project conventions (RuboCop compliant pattern)
• Clear, descriptive variable names
• Appropriate use of Ruby idioms (.map(&:to_i), .reject, etc.)

---

🔍 Minor Observations

1. Error Handling Consistency (Minor)

Location: lib/react_on_rails/dev/server_manager.rb:96

The find_port_pids method catches StandardError while find_process_pids:62-68 only catches Errno::ENOENT. This is actually fine - find_port_pids benefits from broader error handling since lsof can fail in more ways (permissions, system differences, etc.).

However, given this is a developer tool and silent failure is acceptable, the current approach is pragmatic. ✅

2. Port Hardcoding (Design Question)

Location: lib/react_on_rails/dev/server_manager.rb:30

Ports 3000 and 3001 are hardcoded in the kill_processes method. While appropriate for this gem's use case, consider whether this should be configurable if other projects using this code might use different ports.

Not a blocker - the current approach aligns with React on Rails conventions.

---

🛡️ Security Review

✅ No Security Concerns

1. No command injection risk - All parameters to Open3.capture2 are properly separated (not shell strings)
2. No privilege escalation - Uses TERM signal, which is standard
3. Self-protection - Excludes current process PID
4. Safe defaults - Returns empty arrays on errors rather than crashing

---

🚀 Performance Considerations

✅ Good Performance Characteristics

• lsof -ti :PORT is a lightweight operation
• Only checks 2 ports (3000, 3001) - minimal overhead
• Runs sequentially but appropriately (killing is inherently sequential)
• Early returns (next unless pids.any?) avoid unnecessary work

---

📊 Test Quality Assessment

✅ Comprehensive Test Coverage

The test suite covers:

1. ✅ Happy path (finding and killing processes)
2. ✅ Edge cases (empty results, current PID exclusion)
3. ✅ Error conditions (command not found, permission denied)
4. ✅ Integration (full kill_processes workflow)
5. ✅ Multiple PIDs per port (line 130: "3457\n3458")

Well done! The tests use proper RSpec mocking patterns and align with existing test conventions.

---

✅ Final Recommendation

APPROVE - This PR is ready to merge.

Summary

• ✅ Code quality: Excellent, follows project conventions
• ✅ Test coverage: Comprehensive and thorough
• ✅ Security: No concerns
• ✅ Performance: Efficient implementation
• ✅ Design: Well-integrated with existing code
• ✅ Documentation: Clear and helpful

Why This Is Good

1. Solves a real developer pain point
2. Backwards compatible (only adds functionality)
3. Safe by default (graceful error handling)
4. Well-tested with edge cases covered
5. Maintains consistency with existing code patterns

Pre-merge Checklist

According to CLAUDE.md:

• Run bundle exec rubocop - must pass with zero offenses
• Ensure all files end with newline character
• Tests pass

---

🤖 Generated with Claude Code