Add database check to bin/dev before starting server

[justin808]

Summary

• Adds database readiness check to bin/dev before starting development server
• Provides clear, actionable error messages when database is not set up
• Prevents confusing errors buried in combined Foreman/Overmind logs

Problem

When running bin/dev on a fresh checkout or after database cleanup, the script starts all services without checking if the database is set up. This leads to:

• Database connection errors hidden in combined logs
• Poor DX: developers see the server "running" but page loads fail
• Unclear failure mode with no indication that db:setup or db:migrate is needed

Solution

Added DatabaseChecker class that checks database status before starting processes. Handles three cases:

1. No database exists: suggests db:setup or db:create
2. Pending migrations: suggests db:migrate
3. Connection errors: shows troubleshooting steps

If Rails/ActiveRecord isn't available, or if an unexpected error occurs, the check is skipped to allow apps without databases to continue.

Test plan

• Unit tests for DatabaseChecker class pass
• All existing tests continue to pass (111 examples in dev/ module)
• RuboCop passes with no offenses
• Manual testing with dummy app on fresh checkout

Closes #2099

🤖 Generated with Claude Code

[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 19 minutes and 4 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 16b3908 and 22c3308.

📒 Files selected for processing (5)

• react_on_rails/Steepfile (1 hunks)
• react_on_rails/lib/react_on_rails/dev/database_checker.rb (1 hunks)
• react_on_rails/lib/react_on_rails/dev/server_manager.rb (4 hunks)
• react_on_rails/sig/react_on_rails/dev/database_checker.rbs (1 hunks)
• react_on_rails/spec/react_on_rails/dev/database_checker_spec.rb (1 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 jg/bindev-db-check

---

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 for PR #2133

✅ Overall Assessment

This is a well-crafted PR that adds a helpful developer experience feature. The code is clean, well-tested, and follows the project's conventions. I found a few areas for improvement.

---

🎯 Strengths

1. Excellent test coverage - 156 lines of comprehensive unit tests covering all error scenarios
2. Consistent with existing patterns - Mirrors ServiceChecker structure perfectly
3. Good error handling - Gracefully handles apps without databases via StandardError catch
4. Clear user feedback - Colored output with actionable next steps
5. Well-documented - Clear class-level documentation

---

🔴 Critical Issues

1. Missing RBS Type Signature

According to CLAUDE.md: When creating new Ruby files in lib/react_on_rails/, you must:

• Create RBS signature (sig/react_on_rails/dev/database_checker.rbs)
• Add to Steepfile: check "lib/react_on_rails/dev/database_checker.rb"

Action Required:

# react_on_rails/sig/react_on_rails/dev/database_checker.rbs
module ReactOnRails
  module Dev
    class DatabaseChecker
      def self.check_database: () -> bool

      private

      def self.rails_available?: () -> bool
      def self.check_and_report_database: () -> bool
      def self.database_ready?: () -> bool
      def self.print_check_header: () -> void
      def self.print_database_ok: () -> void
      def self.print_database_failed: () -> void
    end
  end
end

Then add to react_on_rails/Steepfile (alphabetically):

check "lib/react_on_rails/dev/database_checker.rb"

---

⚠️ Important Issues

2. Potential Security Issue: Command Injection in Error Message

Location: database_checker.rb:99

puts Rainbow("Error: #{@error_message}").red

Issue: User-controlled error messages from ActiveRecord exceptions are interpolated directly into output. While unlikely to be exploitable via terminal rendering, it's better practice to sanitize.

Recommendation:

puts Rainbow("Error: #{@error_message.inspect}").red

Or use parameterized formatting:

puts Rainbow("Error: %s" % @error_message.inspect).red

3. Instance Variable Usage in Class Methods

Locations: database_checker.rb:47-49, 57-58

@error_type = :no_database
@error_message = e.message

Issue: Using instance variables (@error_type, @error_message) in class methods is unconventional and can cause issues with thread safety and maintainability.

Better approach: Return a result object or pass data as parameters:

def database_ready?
  ActiveRecord::Base.connection.execute("SELECT 1")
  { ready: true }
rescue ActiveRecord::NoDatabaseError
  { ready: false, error_type: :no_database }
rescue ActiveRecord::PendingMigrationError
  { ready: false, error_type: :pending_migrations }
# ... etc
end

def check_and_report_database
  print_check_header
  result = database_ready?
  
  if result[:ready]
    print_database_ok
    true
  else
    print_database_failed(result[:error_type], result[:error_message])
    false
  end
end

---

💡 Suggestions

4. Test Coverage: Missing StandardError/DEBUG Scenario

The test at line 138 only checks that a warning is output to stderr when DEBUG=true, but doesn't verify the behavior when DEBUG is not set.

Add test:

it "does not output a warning when DEBUG is not enabled" do
  allow(ENV).to receive(:[]).with("DEBUG").and_return(nil)
  expect { described_class.check_database }.not_to output(/Database check warning/).to_stderr
end

5. Consistency: Check Both ENV Keys for DEBUG

ServiceChecker uses ENV["DEBUG"] for conditional warnings. Consider if you want to check both ENV["DEBUG"] and ENV["VERBOSE"] for consistency with Rails conventions.

6. Performance: Unnecessary Connection Test

Location: database_checker.rb:43

ActiveRecord::Base.connection.execute("SELECT 1")

Note: ActiveRecord::Base.connection already establishes a connection and will raise exceptions if it fails. The SELECT 1 is redundant. However, keeping it makes the intent explicit and handles edge cases, so this is fine.

7. Documentation: Edge Cases

Consider documenting what happens when:

• Database exists but is from a different Rails version
• Multiple databases are configured (Rails 6+)
• Database is read-only

---

🧪 Testing Recommendations

Before Merging:

1. Manual test in spec/dummy:

   cd spec/dummy
   # Drop database and test guidance
   bin/rails db:drop
   bin/dev  # Should show clear db:setup guidance
   
   # Test pending migrations scenario
   bin/rails db:create
   # Create a migration manually
   bin/dev  # Should show db:migrate guidance

2. Test without Rails/ActiveRecord:

   • The code currently uses defined?(Rails) && defined?(ActiveRecord::Base)
   • Ensure this works in non-Rails contexts

3. RBS validation:

   bundle exec rake rbs:validate
   bundle exec rake rbs:steep

---

📋 Checklist

• Add RBS signature file (sig/react_on_rails/dev/database_checker.rbs)
• Add to Steepfile
• Run bundle exec rake rbs:validate
• Fix instance variable usage in class methods (or document why it's safe)
• Consider sanitizing error messages in output
• Add test case for DEBUG=false scenario
• Manual testing with dummy app (db:drop, db:create, bin/dev)
• Run bundle exec rubocop to ensure no offenses

---

🎉 Summary

This PR significantly improves DX by catching database setup issues early with clear guidance. The implementation is solid and well-tested. The main action items are:

1. CRITICAL: Add RBS signatures (required by project standards)
2. IMPORTANT: Refactor instance variable usage for better thread safety
3. NICE-TO-HAVE: Enhanced test coverage and error message sanitization

Great work overall! 👍

[claude]

Code Review for PR #2133

This is a well-designed and thoughtfully implemented feature that significantly improves the developer experience. The code quality is high with excellent test coverage. Here's my detailed analysis:

---

✅ Strengths

1. Excellent Code Organization

• Clean separation of concerns with dedicated DatabaseChecker class
• Consistent with existing ServiceChecker pattern - good architectural consistency
• Private methods are well-named and single-purpose

2. Comprehensive Error Handling

• Handles all major database states gracefully:
  • No database (NoDatabaseError)
  • Pending migrations (PendingMigrationError)
  • Connection issues (ConnectionNotEstablished, StatementInvalid)
  • Unexpected errors (graceful degradation)
• Especially good: Returns true for unexpected errors to allow apps without databases to continue

3. Outstanding Test Coverage

• 156 lines of specs for 119 lines of code - excellent ratio
• Tests all error paths including edge cases
• Proper mocking of ActiveRecord components
• Tests both success and failure scenarios

4. Great User Experience

• Clear, actionable error messages with color-coded output
• Specific guidance for each error type (db:setup vs db:migrate vs connection troubleshooting)
• Friendly tips at the end of error messages
• Consistent UI/UX with existing ServiceChecker

---

🔍 Observations & Suggestions

1. Minor: Redundant Check in database_ready?

Location: database_checker.rb:43

The SELECT 1 query is fine but technically ActiveRecord::Base.connection alone will raise errors if the database isn't accessible. Consider:

def database_ready?
  # The .connection call itself will raise appropriate errors
  ActiveRecord::Base.connection.active?
  true
rescue # ...

However, your current approach with execute("SELECT 1") is more explicit and equally valid. This is a style preference, not a bug.

2. Enhancement: Consider Adding Database Name to Error Output

When connection fails, showing which database it tried to connect to could be helpful:

when :connection_error
  db_config = ActiveRecord::Base.connection_db_config rescue nil
  puts Rainbow("Could not connect to database: #{db_config&.database}").yellow

Priority: Low - nice-to-have for debugging

3. Enhancement: Parallel Checks with ServiceChecker

Currently runs sequentially:

exit 1 unless ServiceChecker.check_services
exit 1 unless DatabaseChecker.check_database

If both fail, user only sees the first failure. Could consider checking both and reporting all failures together. However, this might complicate the flow and current approach is acceptable.

4. Documentation: RBS Signatures

Per CLAUDE.md guidelines: When creating new Ruby files in lib/react_on_rails/, you should:

1. Create RBS signature: sig/react_on_rails/dev/database_checker.rbs
2. Add to Steepfile: check "lib/react_on_rails/dev/database_checker.rb"
3. Validate: bundle exec rake rbs:validate

Example RBS for this class:

# sig/react_on_rails/dev/database_checker.rbs
module ReactOnRails
  module Dev
    class DatabaseChecker
      def self.check_database: () -> bool

      private

      def self.rails_available?: () -> bool
      def self.check_and_report_database: () -> bool
      def self.database_ready?: () -> bool
      def self.print_check_header: () -> void
      def self.print_database_ok: () -> void
      def self.print_database_failed: () -> void
    end
  end
end

---

🔒 Security Considerations

✅ No security concerns identified

• No user input is processed
• No SQL injection risk (using ActiveRecord's execute with literal string)
• No shell command execution
• No sensitive data exposure

---

⚡ Performance Considerations

✅ Negligible performance impact

• Runs once at startup before server launches
• Single lightweight query (SELECT 1)
• Early return if Rails/ActiveRecord not available
• Graceful degradation on unexpected errors

---

🧪 Testing Recommendations

Already covered by tests ✅

• All error paths
• Rails/ActiveRecord availability check
• Each database state scenario

Additional manual testing (as noted in PR description):

• Test with dummy app on fresh checkout
• Verify error messages are readable in actual terminal
• Test behavior when .dev-services.yml also has failures

---

📋 Checklist Against CLAUDE.md Requirements

✅ Code quality requirements:

• RuboCop passes (as stated in PR)
• Tests pass (111 examples)
• No manual formatting (would be caught by pre-commit hooks)
• RBS signatures (see recommendation Add integration tests #4 above)

✅ Changelog:

• User-facing feature improvement ✓
• Should be added to CHANGELOG.md with appropriate entry

---

💡 Recommended Actions Before Merge

1. CRITICAL (per CLAUDE.md): Run manual testing with dummy app

   cd spec/dummy
   bin/rails db:drop
   bin/dev  # Should show database error
   bin/rails db:setup
   bin/dev  # Should succeed

2. STRONGLY RECOMMENDED: Add RBS type signatures (see Add integration tests #4 above)

3. OPTIONAL: Consider enhancements Make work with turbolinks and better helper #2 and Add linting and CI scripts #3 if time permits

---

🎯 Final Verdict

✅ APPROVED - This is high-quality code that solves a real DX problem elegantly.

Impact:

• Significantly improves onboarding experience for new developers
• Prevents frustrating "server appears running but fails" scenarios
• Maintains consistency with existing architecture patterns

Code Quality: 9/10

• Excellent structure, testing, and documentation
• Minor enhancements suggested but nothing blocking

Risk Level: Low

• Well-tested
• Graceful failure modes
• No breaking changes

Great work! This will make bin/dev much more pleasant to use. 🎉

[justin808]

Review Feedback Addressed

Thanks for the thorough code review! I've addressed all the critical and important items in commit 22c3308:

✅ CRITICAL: RBS Type Signatures (Fixed)

• Created sig/react_on_rails/dev/database_checker.rbs with full type signatures
• Added to Steepfile (alphabetically ordered)
• Validated with bundle exec rake rbs:validate

✅ IMPORTANT: Thread Safety (Fixed)

• Refactored database_ready? to return a result hash { ready: bool, error_type: Symbol?, error_message: String? } instead of using instance variables
• print_database_failed now takes error_type and error_message as parameters
• This makes the code thread-safe and improves maintainability

✅ Test Coverage (Fixed)

• Added test case for DEBUG=false scenario to verify no warning is output when DEBUG is not enabled
• All 9 tests pass

ℹ️ Notes on Other Suggestions

• Error message sanitization: The error messages come from ActiveRecord exceptions which are trusted internal sources, not user input. The current approach is standard Rails convention.
• Redundant SELECT 1 query: Kept intentionally as it makes the intent explicit and is consistent with database health check patterns.
• Parallel checks: Current sequential approach is simpler and failure-first is actually better UX (fix one thing at a time).

[justin808]

Closing b/c AI-off