Skip to content

review(cli): code review response for TUI defaults and positional path shortcuts#24

Closed
Copilot wants to merge 1 commit intocursor/tui-watch-path-handling-3694from
copilot/sub-pr-23
Closed

review(cli): code review response for TUI defaults and positional path shortcuts#24
Copilot wants to merge 1 commit intocursor/tui-watch-path-handling-3694from
copilot/sub-pr-23

Conversation

Copy link
Contributor

Copilot AI commented Mar 22, 2026

Responding to @copilot review on the PR adding TUI defaults and positional CLI shortcuts. All CI checks pass (Tests ✅, Lint ✅, 127 tests).

Review findings

No blocking issues. Changes are correct, well-tested, and backward-compatible.

What looks good

  • DEFAULT_METRICS_JSONL_FILENAME constant in metrics_stream.py is the right single source of truth for the worker↔TUI filename
  • inject_cli_positional_paths and its private helpers are pure functions, easily testable in isolation
  • sys.argv mutation in main() is properly guarded by finally restoration
  • TUI status line now shows the resolved absolute path, removing ambiguity for custom --watch values
  • Test coverage is solid across positional rewriting, dev e2e, TUI help output, and default-path resolution

Minor nits (non-blocking)

  • inject_worker_positional_paths docstring says "backward-compatible alias" but both functions were introduced together; "public alias" is more accurate
  • _QUICKSTART_EPILOG wording is slightly awkward — suggest condensing to:

    openrtc dev ./agents [./openrtc-metrics.jsonl] or use --agents-dir/--metrics-jsonl flags. start for production.

  • _inject_worker_start_dev_console — the mixed-mode edge case (explicit --agents-dir + a bare second token) falls through to Typer's "unexpected argument" error; a short code comment would clarify this is intentional

⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI changed the title [WIP] Fix TUI defaults and clearer --watch handling review(cli): code review response for TUI defaults and positional path shortcuts Mar 22, 2026
Copilot AI requested a review from mahimairaja March 22, 2026 18:57
@codecov
Copy link

codecov bot commented Mar 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (23224dc) to head (bb95383).
⚠️ Report is 3 commits behind head on cursor/tui-watch-path-handling-3694.

Additional details and impacted files

Impacted file tree graph

@@                         Coverage Diff                          @@
##           cursor/tui-watch-path-handling-3694      #24   +/-   ##
====================================================================
  Coverage                                90.14%   90.14%           
====================================================================
  Files                                       13       13           
  Lines                                     1420     1420           
====================================================================
  Hits                                      1280     1280           
  Misses                                     140      140           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23224dc...bb95383. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mahimairaja mahimairaja deleted the copilot/sub-pr-23 branch March 23, 2026 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants