feat: color output, per-command help, and 62 new tests#120
Conversation
WalkthroughAdds configurable color output ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Entry Point (bin/gtr)
participant Config as Config System (config.sh)
participant UI as UI Module (lib/ui.sh)
participant Cmd as Command Handler (lib/commands)
CLI->>CLI: Source core libs + hooks/provider/adapters/launch
CLI->>UI: _ui_enable_color (auto-detect)
CLI->>Config: Source config.sh (load .gtrconfig)
CLI->>UI: _ui_apply_color_config
Note over UI: Evaluate NO_COLOR / GTR_COLOR / auto
CLI->>CLI: Set _GTR_CURRENT_COMMAND
CLI->>Cmd: Invoke cmd_help with args
Cmd->>Cmd: Normalize alias -> canonical command
Cmd->>Cmd: Call _help_<command>()
Cmd->>UI: Emit help output (uses colored logging)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/cmd_copy.bats`:
- Around line 57-61: The test "cmd_copy for unknown branch warns but succeeds"
only checks exit status but doesn't verify the warning; update the test for
cmd_copy to assert the warning text emitted by the implementation (the log_warn
call in lib/commands/copy.sh that emits "No files copied (source and target may
be the same)") — e.g., after running run cmd_copy nonexistent -- ".env" add an
assertion that $output contains "No files copied" (or change the test name to
remove "warns" if you don't want to assert the warning).
🧹 Nitpick comments (5)
bin/gtr (1)
53-54: Consider using lowercase for the mutable context variable.
_GTR_CURRENT_COMMANDusesUPPER_CASE, but per coding guidelines,UPPER_CASEis reserved for constants. Since this value changes per command invocation,_gtr_current_command(lowercase) would be more idiomatic.♻️ Proposed change
- # Set for per-command help (used by show_command_help in ui.sh) - _GTR_CURRENT_COMMAND="$cmd" + # Set for per-command help (used by show_command_help in ui.sh) + _gtr_current_command="$cmd"Note: Would also require updating any references in
lib/ui.shor other files that read this variable.lib/ui.sh (2)
40-52: Phase 1 duplicates_ui_enable_colorlogic.The Phase 1 auto-detection block duplicates the ANSI code assignment from
_ui_enable_color(). Consider calling the helper functions instead to reduce duplication and ensure consistency.♻️ Suggested refactor to reduce duplication
# Phase 1: auto-detect color at source time (before config.sh is available) -if _ui_should_color 2; then - _UI_GREEN=$(printf '\033[0;32m') - _UI_YELLOW=$(printf '\033[0;33m') - _UI_RED=$(printf '\033[0;31m') - _UI_CYAN=$(printf '\033[0;36m') - _UI_BOLD=$(printf '\033[1m') - _UI_RESET=$(printf '\033[0m') -fi -if _ui_should_color 1; then - _UI_BOLD_STDOUT=$(printf '\033[1m') - _UI_RESET_STDOUT=$(printf '\033[0m') -fi +if _ui_should_color 2 && _ui_should_color 1; then + _ui_enable_color +elif _ui_should_color 2; then + # stderr only + _UI_GREEN=$(printf '\033[0;32m') + _UI_YELLOW=$(printf '\033[0;33m') + _UI_RED=$(printf '\033[0;31m') + _UI_CYAN=$(printf '\033[0;36m') + _UI_BOLD=$(printf '\033[1m') + _UI_RESET=$(printf '\033[0m') +elif _ui_should_color 1; then + # stdout only + _UI_BOLD_STDOUT=$(printf '\033[1m') + _UI_RESET_STDOUT=$(printf '\033[0m') +fi
142-143: Consider validatingvar_namebeforeeval.The
evalusage with a caller-supplied variable name is a mild code smell. While unlikely to be exploited in this context (internal function with controlled callers), consider validating thatvar_nameis a valid identifier to prevent unintended behavior.🛡️ Optional: Add variable name validation
prompt_input() { local question="$1" local var_name="$2" log_question "$question " read -r input if [ -n "$var_name" ]; then + # Validate var_name is a valid shell identifier + case "$var_name" in + [!a-zA-Z_]*|*[!a-zA-Z0-9_]*) + return 1 + ;; + esac eval "$var_name=\"\$input\"" else printf "%s" "$input" fi }tests/ui_color.bats (1)
19-24: Test title is slightly misleading.The test is titled "returns 1 when NO_COLOR is empty string (unset wins)" but it actually tests
GTR_COLOR=neverwithNO_COLORunset. The title suggests testing the edge case ofNO_COLOR=""(set but empty), which would behave differently. Consider clarifying the title.📝 Clarify test title
-@test "_ui_should_color returns 1 when NO_COLOR is empty string (unset wins)" { +@test "_ui_should_color returns 1 with GTR_COLOR=never when NO_COLOR is unset" { _source_ui unset NO_COLOR GTR_COLOR=never run _ui_should_color 2 [ "$status" -ne 0 ] }lib/commands/help.sh (1)
392-395: Consider usingdeclare -finstead oftypefor function check.Per coding guidelines,
declare -fis the recommended way to check function presence. Whiletypeworks,declare -fis more explicit for functions.♻️ Use declare -f for function check
# Dispatch to per-command help function local help_func="_help_${command//-/_}" - if type "$help_func" >/dev/null 2>&1; then + if declare -f "$help_func" >/dev/null 2>&1; then "$help_func" elseAs per coding guidelines: "Check function presence with
declare -f create_worktreeordeclare -f resolve_target"
Add GitHub Actions workflow that auto-updates the Homebrew formula in coderabbitai/homebrew-tap on each new release. Update README to promote `brew install git-gtr` as the primary macOS install method. Closes #92
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
319-340:⚠️ Potential issue | 🟠 MajorDocument the new
gtr.ui.colorconfig option in README.The PR adds a new configuration key, but README’s Configuration section doesn’t mention it. Please add an example (and brief description) under “Quick Setup” or “Configuration” so users can discover it. As per coding guidelines, "README.md: Update README when adding new commands, adapters, or configuration options".
- Rename ui_color test to match what it actually tests (GTR_COLOR=never, not NO_COLOR empty string) - Assert warning output in cmd_copy unknown branch test
Add color configuration to README Quick Setup and docs/configuration.md with new UI Settings section, precedence rules, and env vars.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/configuration.md`:
- Line 455: Update the prose for the GTR_PROVIDER entry so the provider name
uses the proper product branding (e.g., "GitHub" and "GitLab") while leaving the
config values lowercase; locate the table row referencing the `GTR_PROVIDER` key
and change the description from "github" or "gitlab" to use "GitHub" and
"GitLab" in the human-readable text while keeping the actual config token
examples lowercase in backticks.
1 participant
Summary by CodeRabbit
New Features
Improvements
Completions
Docs
Tests
Chores