Fix bin/dev failing with 'Unknown argument' when using --route flag

[ihabadham]

Summary

Fixes bin/dev failing with "Unknown argument: hello_world" immediately after running rails generate react_on_rails:install.

Problem

The generator creates a bin/dev script that passes ["--route", "hello_world"] to ServerManager.run_from_command_line. The early command extraction (added in PR #2092) was incorrectly picking up "hello_world" as the command instead of recognizing it as the value for --route.

$ ./bin/dev
Unknown argument: hello_world
Run 'dev help' for usage information

Root Cause

PR #2092 moved command extraction before OptionParser to enable skipping precompile hooks for kill/help commands:

# This finds "hello_world" as the "command" because it doesn't start with "-"
command = args.find { |arg| !arg.start_with?("--") && !arg.start_with?("-") }

Solution

Added extract_command_from_args method that properly skips values following flags like --route and --rails-env:

def extract_command_from_args(args)
  skip_next = false
  args.each do |arg|
    if skip_next
      skip_next = false
      next
    end
    if FLAGS_WITH_VALUES.include?(arg)
      skip_next = true
      next
    end
    next if arg.start_with?("-")
    return arg
  end
  nil
end

Test Plan

• All 35 existing server_manager_spec.rb tests pass
• Manual testing: Created fresh Rails app, ran generator, ./bin/dev works
• Verified bin/dev help, bin/dev kill, bin/dev static still work correctly

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor

  • Improved development-server CLI parsing so flags that take values (e.g., route or rails environment) are ignored when determining the command, preventing flag values from being misinterpreted as commands.

• Tests

  • Added comprehensive tests for flag/value forms, flag ordering, verbosity flags, default behavior, and unknown-command handling to validate parsing.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds a FLAGS_WITH_VALUES constant and an extract_command_from_args(args) helper in ReactOnRails::Dev::ServerManager to skip flags and their values (e.g., --route, --rails-env) when determining the CLI command; tests added covering many CLI flag permutations.

Changes

Cohort / File(s)	Summary
ServerManager argument parsing react_on_rails/lib/react_on_rails/dev/server_manager.rb	Add FLAGS_WITH_VALUES = %w[--route --rails-env].freeze; replace inline arg scan with extract_command_from_args(args); implement private helper to skip flags and their values and return the first non-flag argument.
Tests for CLI parsing react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb	Add extensive specs for --route (both --route value and --route=value), --rails-env, --verbose/-v, multiple flag orders, no-arg default, and unknown-command behavior. (Note: test suite appears duplicated in the diff.)

Sequence Diagram(s)

(omitted — changes are local argument-parsing logic without multi-component sequential interactions)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• 🚀 Enhance bin/dev with configurable routes and improved production asset handling #1790 — Related to ServerManager CLI parsing and handling of --route/--rails-env.
• Fix bin/dev route argument format #2309 — Addresses passing --route=<value> from bin scripts and parsing behavior.
• bin/dev is not tested in CI - bugs ship to users undetected #2274 — Targets same goal of treating certain flags as consuming the next argument.

Poem

🐰 I hop through args both short and long,
Nibble at flags and leave values be,
A tiny constant, a helper so strong,
Now commands peek out — clear for me to see! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the main change: fixing the bin/dev script's failure when using the --route flag, which directly matches the PR's core objective of improving argument parsing.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 ihabadham/fix/bin-dev-route-parsing

---

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.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

react_on_rails/lib/react_on_rails/dev/server_manager.rb (1)

151-152: LGTM! The constant correctly identifies flags that consume values.

The constant is complete for the current OptionParser configuration (--route and --rails-env are the only flags taking values).

Optional: Improve maintainability

Consider adding a comment to remind future developers to update this constant when adding new flags with values:

 # Flags that take a value as the next argument (not using = syntax)
+# NOTE: When adding new flags to OptionParser that take values, update this list
 FLAGS_WITH_VALUES = %w[--route --rails-env].freeze

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3efa19d and 7384cf5.

📒 Files selected for processing (1)

• react_on_rails/lib/react_on_rails/dev/server_manager.rb

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rb

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rb: Run bundle exec rubocop and fix ALL violations before every commit/push
Run bundle exec rubocop (MANDATORY) before every commit to ensure zero offenses

Files:

• react_on_rails/lib/react_on_rails/dev/server_manager.rb

🧠 Learnings (5)

📓 Common learnings

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)

📚 Learning: 2025-12-19T18:57:59.314Z

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages

Applied to files:

• react_on_rails/lib/react_on_rails/dev/server_manager.rb

📚 Learning: 2025-12-19T18:57:59.314Z

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`

Applied to files:

• react_on_rails/lib/react_on_rails/dev/server_manager.rb

📚 Learning: 2025-12-19T18:57:59.314Z

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/app_commands/**/*.rb : Create custom Rails app commands in `e2e/playwright/app_commands/` directory with .rb files for reusable helpers in Playwright tests

Applied to files:

• react_on_rails/lib/react_on_rails/dev/server_manager.rb

📚 Learning: 2025-12-19T18:57:59.314Z

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)

Applied to files:

• react_on_rails/lib/react_on_rails/dev/server_manager.rb

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
• GitHub Check: rspec-package-tests (3.4, latest)
• GitHub Check: build
• GitHub Check: claude-review

🔇 Additional comments (2)

react_on_rails/lib/react_on_rails/dev/server_manager.rb (2)

160-161: Excellent fix! This correctly delegates to the new extraction logic.

The updated comment clearly explains the problem being solved, and delegating to extract_command_from_args properly handles flag values.

---

222-246: Logic is sound, but RuboCop compliance verification required.

The extract_command_from_args method correctly:

• Skips argument values that follow flags in FLAGS_WITH_VALUES (currently --route and --rails-env)
• Ignores all flags starting with - or --
• Returns the first non-flag, non-value argument as the command
• Returns nil if no command is found

The implementation properly handles the documented examples. Before merging, verify this file passes RuboCop without violations, as required by your coding guidelines.

[claude]

Code Review - PR #2273

Overview

This is a well-executed bug fix addressing a regression from PR #2092. The implementation is clean and surgical.

Strengths

• Clear root cause analysis of command extraction issue
• Clean implementation with extract_command_from_args method
• Proper state machine with skip_next boolean
• Good code comments
• 35 existing tests pass (no regression)
• Manual testing verified

Critical Issue: Missing Automated Test Coverage

The fix lacks regression tests. Add tests for:

• [--route, hello_world] should return nil (no command)
• [static, --route, hello_world] should return static
• [prod, --rails-env, staging] should return prod
• Multiple flags with values
• Flags without values

Without tests, future refactors could re-introduce this bug.

Additional Recommendations

1. Add comment linking FLAGS_WITH_VALUES to OptionParser definition
2. Test edge case: --route=hello_world (equals syntax)
3. Update CHANGELOG.md per project guidelines
4. Verify: bundle exec rubocop

Security and Performance

• No security concerns (only parses internal CLI args)
• Negligible performance impact

Final Assessment

Code Quality: 4/5 stars
Test Coverage: 3/5 stars (missing regression tests)
Overall: LGTM with test additions recommended

Great fix that solves a real user-facing issue! Please add regression tests either in this PR or as immediate follow-up.

[claude]

Code Review - PR #2273

This PR fixes a critical bug where bin/dev fails after running rails generate react_on_rails:install due to incorrect command extraction when using the --route flag.

Strengths

1. Excellent Problem Analysis - Clear identification of root cause from PR Add automatic precompile hook coordination in bin/dev #2092
2. Comprehensive Test Coverage - 156 new lines covering all critical scenarios
3. Clean Implementation - Dedicated private method with FLAGS_WITH_VALUES constant
4. Backward Compatibility - All 35 existing tests pass

Code Quality

Architecture: Follows Single Responsibility Principle with extract_command_from_args method.

Security: No concerns - only parses structure, validation exists elsewhere (lines 458-464).

Performance: Negligible - O(n) with early return, runs once at startup.

Suggestions

1. Consider documenting that --route=value syntax does not need FLAGS_WITH_VALUES entry
2. Document short flag handling if added in future (e.g., -r value)
3. Optional tests: empty route value, multiple same flags (nice-to-haves)

Recommendation

APPROVE with minor suggestions

This is a high-quality fix that solves a critical user-facing bug with excellent test coverage.

Before Merging:

• Run bundle exec rubocop and fix violations
• Verify trailing newlines
• Consider changelog entry (user-visible bug fix)

Great work identifying this regression and providing a clean fix!

Review completed by Claude Code

[justin808]

@claude is this handled by #2309

[claude]

Claude finished @justin808's task in 1m 2s —— View job

---

Unable to Access PR #2309

I cannot access PR #2309 directly due to tool limitations (GitHub CLI and web fetch require additional permissions).

What I can confirm about THIS PR (#2273):

This PR is currently on branch ihabadham/fix/bin-dev-route-parsing and contains:

1. The fix in server_manager.rb:

   • FLAGS_WITH_VALUES constant (lines 151-152)
   • extract_command_from_args method (lines 222-246)
   • Proper command extraction that skips flag values (line 161)

2. Comprehensive tests in server_manager_spec.rb:

   • 156 lines of new test coverage
   • Tests for --route, --rails-env, --verbose flags
   • Tests for flag ordering and edge cases

To answer your question about PR #2309:

Could you clarify what PR #2309 addresses? Specifically:

• Does it fix the same "Unknown argument" issue with --route?
• Is it a broader refactor that includes this fix?
• Or does it address a different bin/dev issue?

If you can share the PR #2309 URL or description, I can better assess whether they're duplicates or if one supersedes the other.

---

[ihabadham]

@justin808 @claude PR #2309 fixes the default case (the generated bin/dev template now uses --route=hello_world).

However, the parser bug still exists. Per https://docs.ruby-lang.org/en/3.0/OptionParser.html, both --route=value and --route value are valid formats, but only the first works currently:

bin/dev --route=hello_world  # works
bin/dev --route hello_world  # fails with "Unknown argument: hello_world"

This PR fixes the early command extraction to handle both formats correctly, and adds unit tests for argument parsing.

[claude]

Code Review - PR #2273

Summary

This PR fixes a critical bug where bin/dev fails immediately after running rails generate react_on_rails:install when the generator passes --route flag values as separate arguments. The fix properly handles CLI argument parsing to distinguish between flags, their values, and actual commands.

---

✅ Strengths

1. Problem Identification & Root Cause Analysis

• Excellent root cause analysis tracing the issue back to PR Add automatic precompile hook coordination in bin/dev #2092
• Clear reproduction steps and error message in the description
• Well-documented why the early command extraction was needed (for kill/help commands)

2. Implementation Quality

• Clean, readable solution with extract_command_from_args method
• Properly handles both --route value and --route=value syntaxes
• Good separation of concerns with FLAGS_WITH_VALUES constant
• Implementation follows Ruby conventions and is maintainable

3. Test Coverage

• Comprehensive test suite covering all edge cases:
  • Both flag syntaxes (--route value and --route=value)
  • Command before/after flags
  • Multiple flags in any order
  • Unknown commands
  • Default behavior
• Tests follow Rails CLI testing patterns (references rails/rails repo)
• Tests include helpful comments explaining what they validate

4. Documentation

• Clear inline comments in both implementation and tests
• PR description includes test plan with manual verification
• References related PR (Add automatic precompile hook coordination in bin/dev #2092) for context

---

🔍 Observations & Suggestions

1. Generator Template Discrepancy
The current bin/dev template (line 32) uses --route=\#{DEFAULT_ROUTE} syntax:

argv_with_defaults.push("--route=\#{DEFAULT_ROUTE}")

However, the PR description states the generator passes ["--route", "hello_world"] as separate arguments. This suggests either:

• The template was recently updated (possibly in this PR or a related one)
• There are multiple generators/templates that need checking
• The bug exists in older generated bin/dev files that users may still have

Recommendation: Verify all generator templates use the --route=value syntax consistently, or document that this fix supports both syntaxes for backward compatibility.

2. Edge Case: What if a flag value looks like a command?
Consider this scenario:

bin/dev --route static

Current behavior: Would correctly treat static as the route value, not the command.
This is handled correctly by your implementation! Just worth noting as a success case.

3. Edge Case: Equals sign in separate arg syntax

bin/dev --route "my=route"

This should work fine, but worth adding a test if this is a valid use case.

4. FLAGS_WITH_VALUES Maintainability
The FLAGS_WITH_VALUES constant is defined but not dynamically derived from the OptionParser definition. This means future developers adding new flags must remember to update both places.

Potential enhancement (not blocking):

# Could add a comment reminder
# Flags that take a value as the next argument (not using = syntax)
# IMPORTANT: When adding new flags to OptionParser, update this list if they take values
FLAGS_WITH_VALUES = %w[--route --rails-env].freeze

5. Security: No injection risks
✅ All user input is properly handled
✅ No command injection risks (args are parsed, not executed directly)
✅ No SQL injection concerns (this is CLI parsing only)

---

📋 Testing Recommendations

Before Merge:

• ✅ Verify all 35 existing tests still pass
• ✅ Manual testing completed per PR description
• ⚠️ Recommended: Test with an actual older bin/dev file (if different from current template)
• ⚠️ Recommended: Run bundle exec rubocop lib/react_on_rails/dev/server_manager.rb to ensure no lint violations

After Merge:

• Monitor for any user reports of bin/dev issues
• Consider adding integration test that runs the actual generator + bin/dev

---

🎯 Performance Considerations

• ✅ Minimal performance impact (single array iteration)
• ✅ Early exit when command is found
• ✅ No unnecessary allocations

---

🏆 Best Practices Adherence

Per CLAUDE.md:

• ✅ Avoids over-engineering - minimal solution to the problem
• ✅ Good test coverage with clear, focused tests
• ✅ No security vulnerabilities introduced
• ✅ Clear comments explaining the "why" not just the "what"
• ✅ Follows Ruby conventions and project patterns

---

✨ Final Recommendation

APPROVE with minor suggestions

This is a well-implemented fix for a critical bug. The code is clean, well-tested, and solves the problem elegantly. The suggestions above are mostly for documentation and future maintainability, not blockers.

Action Items (Optional):

1. Add comment to FLAGS_WITH_VALUES reminding developers to update when adding new flags
2. Verify generator templates consistency (or document that both syntaxes are supported)
3. Run RuboCop to confirm no lint violations

Great work on the thorough testing and clear problem analysis! 🎉