refactor(cli): split run() into smaller, focused functions - #36
Merged
Conversation
run() had grown to ~350 lines handling model resolution, interactive file selection, suggestion generation, message editing, and applying commits all in one function, making it hard to reason about or test in isolation. Split it along its existing natural phases: _handle_suggest_only_mode, _select_files, _warn_if_partial_staging_risk, _suggest_for_group, _determine_commit_mode, _build_commit_plan, _print_ai_fallback_warning, _render_plan, _maybe_edit_plan, _confirm_apply, and _apply_plan. run() is now a thin orchestrator. Behavior is unchanged: existing tests pass unmodified, and the full interactive flow (env creation, file selection, AI fallback warning, plan rendering, edit prompt, apply) was smoke-tested manually. Closes #20
📝 WalkthroughWalkthrough
ChangesCLI workflow decomposition
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
The lint job's `pip install ruff` had no version pin, decoupled from pyproject.toml's own ruff>=0.8.0. A newer ruff release enables new default-on lint rules, breaking CI on pre-existing code unrelated to this change. Pin to the last known-good version in both places.
# Conflicts: # git_explain/cli.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
git_explain/cli.py (1)
752-779: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo indication of partial success when a multi-commit plan fails midway.
In "split" mode,
plancan contain several entries; each is committed in turn viaapply_commands. If a later entry raises, earlier entries have already been committed to the repo, but the error output only reports the failing command — the user isn't told which commits (if any) already succeeded, which can be confusing when diagnosing repo state afterward.Consider including the successfully applied commit names in the error output before exiting, e.g. tracking
applied = []and printing it in the except blocks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@git_explain/cli.py` around lines 752 - 779, The _apply_plan function does not report commits that succeeded before a later plan entry failed. Track each completed name in an applied list after apply_commands returns, and include the successfully applied commit names in both subprocess.CalledProcessError and RuntimeError output before raising typer.Exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@git_explain/cli.py`:
- Around line 791-808: Remove the second redundant
_load_ai_env_from_dotenv(repo_env) call in the flow around
_resolve_project_ai_model, keeping the initial environment load after repo_env
is computed and preserving the existing empty-changes early return and model
resolution behavior.
- Around line 536-590: Reuse the existing infer_diff result in
_suggest_for_group when building the with_diff payload, avoiding the second
get_diff_for_paths call; preserve the current behavior by appending the diff
section only when the fetched diff is non-empty.
---
Nitpick comments:
In `@git_explain/cli.py`:
- Around line 752-779: The _apply_plan function does not report commits that
succeeded before a later plan entry failed. Track each completed name in an
applied list after apply_commands returns, and include the successfully applied
commit names in both subprocess.CalledProcessError and RuntimeError output
before raising typer.Exit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+536
to
+590
| def _suggest_for_group( | ||
| change_items: list[tuple[str, str]], | ||
| title: str, | ||
| *, | ||
| repo_root: Path, | ||
| has_commits: bool | None, | ||
| ai_model: str | None, | ||
| with_diff: bool, | ||
| mode: str, | ||
| ) -> tuple[Suggestion, str | None]: | ||
| """Return (suggestion, ai_fallback_reason).""" | ||
| paths_for_infer = [p for _, p in change_items] | ||
| infer_diff: str | None = None | ||
| if paths_for_infer: | ||
| raw_d = get_diff_for_paths(paths_for_infer, cwd=repo_root) | ||
| if raw_d.strip(): | ||
| infer_diff = ( | ||
| raw_d[:_DIFF_INFER_MAX_CHARS] | ||
| if len(raw_d) > _DIFF_INFER_MAX_CHARS | ||
| else raw_d | ||
| ) | ||
| else: | ||
| mode_input = ( | ||
| typer.prompt("Commit mode: one or split", default="one").strip().lower() | ||
|
|
||
| if ai_model: | ||
| payload = _render_combined(has_commits, change_items, title=title) | ||
| if with_diff: | ||
| paths_for_diff = [p for _, p in change_items] | ||
| diff_text = get_diff_for_paths(paths_for_diff, cwd=repo_root) | ||
| if diff_text: | ||
| payload = payload + "\n\n## Diff\n" + diff_text | ||
| try: | ||
| fb_label = title if mode == "split" else None | ||
| sug, _raw = suggest_commands( | ||
| payload, | ||
| model=ai_model, | ||
| with_diff=with_diff, | ||
| unified_diff_for_infer=infer_diff, | ||
| fallback_notifier=_gemini_fallback_notifier(fb_label), | ||
| ) | ||
| if mode_input in ("one", "split"): | ||
| mode = mode_input | ||
| if sug is None: | ||
| raise RuntimeError("Could not parse AI suggestion.") | ||
| return sug, None | ||
| except _AI_CALL_ERRORS as e: | ||
| h = suggest_from_changes( | ||
| changes=change_items, | ||
| has_commits=has_commits, | ||
| diff_text=infer_diff, | ||
| ) | ||
| return h, str(e) | ||
| h = suggest_from_changes( | ||
| changes=change_items, | ||
| has_commits=has_commits, | ||
| diff_text=infer_diff, | ||
| ) | ||
| return h, None | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Redundant get_diff_for_paths call when with_diff is enabled.
infer_diff is computed at Line 550 via get_diff_for_paths(paths_for_infer, ...), and when with_diff is true, diff_text is computed again at Line 562 with the identical paths list via the same function. Both calls run the same git diff --cached/git diff subprocess pair for identical paths, doubling git subprocess invocations per group (and per split-mode group) for no behavioral benefit.
♻️ Reuse the already-fetched diff instead of refetching
paths_for_infer = [p for _, p in change_items]
infer_diff: str | None = None
+ raw_d: str = ""
if paths_for_infer:
raw_d = get_diff_for_paths(paths_for_infer, cwd=repo_root)
if raw_d.strip():
infer_diff = (
raw_d[:_DIFF_INFER_MAX_CHARS]
if len(raw_d) > _DIFF_INFER_MAX_CHARS
else raw_d
)
if ai_model:
payload = _render_combined(has_commits, change_items, title=title)
if with_diff:
- paths_for_diff = [p for _, p in change_items]
- diff_text = get_diff_for_paths(paths_for_diff, cwd=repo_root)
- if diff_text:
- payload = payload + "\n\n## Diff\n" + diff_text
+ if raw_d:
+ payload = payload + "\n\n## Diff\n" + raw_d📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _suggest_for_group( | |
| change_items: list[tuple[str, str]], | |
| title: str, | |
| *, | |
| repo_root: Path, | |
| has_commits: bool | None, | |
| ai_model: str | None, | |
| with_diff: bool, | |
| mode: str, | |
| ) -> tuple[Suggestion, str | None]: | |
| """Return (suggestion, ai_fallback_reason).""" | |
| paths_for_infer = [p for _, p in change_items] | |
| infer_diff: str | None = None | |
| if paths_for_infer: | |
| raw_d = get_diff_for_paths(paths_for_infer, cwd=repo_root) | |
| if raw_d.strip(): | |
| infer_diff = ( | |
| raw_d[:_DIFF_INFER_MAX_CHARS] | |
| if len(raw_d) > _DIFF_INFER_MAX_CHARS | |
| else raw_d | |
| ) | |
| else: | |
| mode_input = ( | |
| typer.prompt("Commit mode: one or split", default="one").strip().lower() | |
| if ai_model: | |
| payload = _render_combined(has_commits, change_items, title=title) | |
| if with_diff: | |
| paths_for_diff = [p for _, p in change_items] | |
| diff_text = get_diff_for_paths(paths_for_diff, cwd=repo_root) | |
| if diff_text: | |
| payload = payload + "\n\n## Diff\n" + diff_text | |
| try: | |
| fb_label = title if mode == "split" else None | |
| sug, _raw = suggest_commands( | |
| payload, | |
| model=ai_model, | |
| with_diff=with_diff, | |
| unified_diff_for_infer=infer_diff, | |
| fallback_notifier=_gemini_fallback_notifier(fb_label), | |
| ) | |
| if mode_input in ("one", "split"): | |
| mode = mode_input | |
| if sug is None: | |
| raise RuntimeError("Could not parse AI suggestion.") | |
| return sug, None | |
| except _AI_CALL_ERRORS as e: | |
| h = suggest_from_changes( | |
| changes=change_items, | |
| has_commits=has_commits, | |
| diff_text=infer_diff, | |
| ) | |
| return h, str(e) | |
| h = suggest_from_changes( | |
| changes=change_items, | |
| has_commits=has_commits, | |
| diff_text=infer_diff, | |
| ) | |
| return h, None | |
| def _suggest_for_group( | |
| change_items: list[tuple[str, str]], | |
| title: str, | |
| *, | |
| repo_root: Path, | |
| has_commits: bool | None, | |
| ai_model: str | None, | |
| with_diff: bool, | |
| mode: str, | |
| ) -> tuple[Suggestion, str | None]: | |
| """Return (suggestion, ai_fallback_reason).""" | |
| paths_for_infer = [p for _, p in change_items] | |
| infer_diff: str | None = None | |
| raw_d: str = "" | |
| if paths_for_infer: | |
| raw_d = get_diff_for_paths(paths_for_infer, cwd=repo_root) | |
| if raw_d.strip(): | |
| infer_diff = ( | |
| raw_d[:_DIFF_INFER_MAX_CHARS] | |
| if len(raw_d) > _DIFF_INFER_MAX_CHARS | |
| else raw_d | |
| ) | |
| if ai_model: | |
| payload = _render_combined(has_commits, change_items, title=title) | |
| if with_diff: | |
| if raw_d: | |
| payload = payload + "\n\n## Diff\n" + raw_d | |
| try: | |
| fb_label = title if mode == "split" else None | |
| sug, _raw = suggest_commands( | |
| payload, | |
| model=ai_model, | |
| with_diff=with_diff, | |
| unified_diff_for_infer=infer_diff, | |
| fallback_notifier=_gemini_fallback_notifier(fb_label), | |
| ) | |
| if sug is None: | |
| raise RuntimeError("Could not parse AI suggestion.") | |
| return sug, None | |
| except _AI_CALL_ERRORS as e: | |
| h = suggest_from_changes( | |
| changes=change_items, | |
| has_commits=has_commits, | |
| diff_text=infer_diff, | |
| ) | |
| return h, str(e) | |
| h = suggest_from_changes( | |
| changes=change_items, | |
| has_commits=has_commits, | |
| diff_text=infer_diff, | |
| ) | |
| return h, None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@git_explain/cli.py` around lines 536 - 590, Reuse the existing infer_diff
result in _suggest_for_group when building the with_diff payload, avoiding the
second get_diff_for_paths call; preserve the current behavior by appending the
diff section only when the fetched diff is non-empty.
Comment on lines
+791
to
+808
| try: | ||
| combined, repo_root = get_combined_diff(cwd=cwd) | ||
| except RuntimeError as e: | ||
| console.print(f"[red]Error:[/red] {e}") | ||
| raise typer.Exit(1) | ||
|
|
||
| repo_env = repo_root / ".env" | ||
| if repo_env.is_file(): | ||
| _load_ai_env_from_dotenv(repo_env) | ||
|
|
||
| if not combined.strip(): | ||
| console.print("[yellow]No staged, unstaged, or untracked changes.[/yellow]") | ||
| return | ||
| ai_model = _resolve_project_ai_model(repo_env, model) | ||
| if repo_env.is_file(): | ||
| _load_ai_env_from_dotenv(repo_env) | ||
| has_commits, changes = _parse_combined(combined) | ||
| console.print(Panel(combined, title="Changed files", border_style="dim")) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Duplicate .env reload.
_load_ai_env_from_dotenv(repo_env) is called twice: once at Line 798-799 right after computing repo_env, and again at Line 805-806 immediately after resolving ai_model, with nothing in between (aside from the early-return empty-changes check) that would require reloading. This looks like a leftover from consolidating the extracted flow and does unnecessary duplicate work each run.
🧹 Remove the redundant reload
repo_env = repo_root / ".env"
if repo_env.is_file():
_load_ai_env_from_dotenv(repo_env)
if not combined.strip():
console.print("[yellow]No staged, unstaged, or untracked changes.[/yellow]")
return
ai_model = _resolve_project_ai_model(repo_env, model)
- if repo_env.is_file():
- _load_ai_env_from_dotenv(repo_env)
has_commits, changes = _parse_combined(combined)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| combined, repo_root = get_combined_diff(cwd=cwd) | |
| except RuntimeError as e: | |
| console.print(f"[red]Error:[/red] {e}") | |
| raise typer.Exit(1) | |
| repo_env = repo_root / ".env" | |
| if repo_env.is_file(): | |
| _load_ai_env_from_dotenv(repo_env) | |
| if not combined.strip(): | |
| console.print("[yellow]No staged, unstaged, or untracked changes.[/yellow]") | |
| return | |
| ai_model = _resolve_project_ai_model(repo_env, model) | |
| if repo_env.is_file(): | |
| _load_ai_env_from_dotenv(repo_env) | |
| has_commits, changes = _parse_combined(combined) | |
| console.print(Panel(combined, title="Changed files", border_style="dim")) | |
| try: | |
| combined, repo_root = get_combined_diff(cwd=cwd) | |
| except RuntimeError as e: | |
| console.print(f"[red]Error:[/red] {e}") | |
| raise typer.Exit(1) | |
| repo_env = repo_root / ".env" | |
| if repo_env.is_file(): | |
| _load_ai_env_from_dotenv(repo_env) | |
| if not combined.strip(): | |
| console.print("[yellow]No staged, unstaged, or untracked changes.[/yellow]") | |
| return | |
| ai_model = _resolve_project_ai_model(repo_env, model) | |
| has_commits, changes = _parse_combined(combined) | |
| console.print(Panel(combined, title="Changed files", border_style="dim")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@git_explain/cli.py` around lines 791 - 808, Remove the second redundant
_load_ai_env_from_dotenv(repo_env) call in the flow around
_resolve_project_ai_model, keeping the initial environment load after repo_env
is computed and preserving the existing empty-changes early return and model
resolution behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
run()incli.pyhad grown to ~350 lines handling model resolution, interactive file selection, suggestion generation (single/split-commit), message editing, and applying commits all in one function._handle_suggest_only_mode,_select_files,_warn_if_partial_staging_risk,_suggest_for_group,_determine_commit_mode,_build_commit_plan,_print_ai_fallback_warning,_render_plan,_maybe_edit_plan,_confirm_apply,_apply_plan).run()is now a thin orchestrator that calls these in sequence.Test plan
pytest -q(unmodified)ruff check ./ruff format --check ..envcreation prompt, file selection, AI-unavailable fallback warning, suggested-commands panel, edit prompt, apply prompt) — output and resulting commit message matched expected behaviorCloses #20
(Re-opened as a new PR: the original PR #28 was auto-closed by GitHub when
main's history was rewritten to correct a contributor's commit attribution — this branch is unchanged.)Summary by CodeRabbit
New Features
Bug Fixes