From c4cc7fe24c4bdd83f1cb0b1d01b26b5ee54cc5bb Mon Sep 17 00:00:00 2001 From: Erwann Mest Date: Mon, 1 Sep 2025 22:54:22 +0100 Subject: [PATCH] feat(changelog, readme, script): add new flags and filters for improved functionality and UI updates --- CHANGELOG.md | 29 ++++++ README.md | 31 ++++++ gh-review-pull-request | 218 ++++++++++++++++++++++++++++++----------- 3 files changed, 223 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49092d0..1d7eff0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,20 @@ All notable changes to this project will be documented in this file. The format is based on Keep a Changelog, and this project adheres to Semantic Versioning. +## [Unreleased] + +### Added + +- `--json` flag to output parsed comments for scripting +- `--list` flag to print the UI list without launching fzf +- `GH_REVIEW_PR_JSON` env var to inject GraphQL JSON (path or raw) for offline workflows +- Filters: `-f/--file`, `--since`, `--until` for file/date-focused workflows + +### Changed + +- Dynamic dependency checks: only require `gh`/`fzf` when needed +- UI list is file-first (path โ€ข author โ€ข date) + ## [0.1.0] - 2025-09-01 ### Added @@ -15,3 +29,18 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve - Filters: --author, --all-comments, --include-outdated - Debug summary via --debug - Version flag (-v/--version) + +## [0.1.1] - 2025-09-01 + +### Changed + +- Safer temp dir via `mktemp -d` and consistent cleanup +- Stricter arg parsing: unknown flags now error out +- Author normalization now strips leading `@`; `copilot` maps to GitHub reviewer bot +- New `-R/--repo` to target a specific repository +- PATH includes `$HOME/.local/bin` and `$HOME/bin` for Linux setups +- More helpful message when no unresolved comments are found + +### Removed + +- Unused `chmod` and `bat` dependencies diff --git a/README.md b/README.md index 82c2a5d..1ca9283 100644 --- a/README.md +++ b/README.md @@ -23,14 +23,21 @@ gh extension install kud/gh-review-pull-request ``` gh review-pull-request # infer repo and current PR gh review-pull-request # browse specific PR number +gh review-pull-request -R owner/repo # target a specific repository ``` Options: - -a, --author Filter by author (can be repeated) +- -f, --file Filter by file path (repeatable) +- --since Filter comments created on/after date +- --until Filter comments created on/before date - --all, --all-comments Show all comments in threads (not just latest) - --include-outdated Include outdated comment threads +- --json Print parsed comments as JSON and exit +- --list Print UI list output and exit - --debug Write a small debug summary file +- -R, --repo Target a specific repository - -h, --help Show help - -v, --version Show version @@ -44,10 +51,34 @@ Keybindings inside fzf: - gh, jq, fzf - awk, sed, base64, wc, tr, nl, cat, cut, rev - Optional: glow or mdcat for nicer markdown rendering + - Clipboard: pbcopy (macOS) or xclip/xsel (Linux) for copy action + +## ๐Ÿงช Testing locally + +You can bypass live GitHub calls by providing a GraphQL result via the `GH_REVIEW_PR_JSON` env variable. This can be either a path to a file or the JSON string itself. + +Example with your own GraphQL response file: + +``` +GH_REVIEW_PR_JSON=/path/to/your_graphql.json \ + ./gh-review-pull-request 123 -R owner/repo --json +``` + +The above prints the parsed comments as JSON (use `--list` to print the UI list instead). + +## ๐Ÿ”Ž Filtering tips + +- File-first list: the UI list now starts with the file path for quick scanning. +- Filter by file: `-f src/app.py` can be repeated. Values are treated as regex by jqโ€™s `test(...)`, so you can use patterns like `-f '^src/.*\.py$'`. +- Filter by date: `--since 2024-01-01`, `--until 2024-01-31` (inclusive, UTC based on `createdAt`). +- Combine filters: `-f src -a @alice --since 2024-01-01 --include-outdated`. ## ๐Ÿ“ Notes - By default, only the latest non-outdated comment of each unresolved thread is shown. Use `--all` to see every comment, and `--include-outdated` to include outdated threads. +- `--author` accepts either `user` or `@user`. `copilot` is normalized to `copilot-pull-request-reviewer`. +- If no unresolved comments are found, try `--all` or `--include-outdated`. +- On Linux, ensure `$HOME/.local/bin` is in your `PATH` so `fzf`/`jq` can be found. ## ๐Ÿท๏ธ Versioning & Releases diff --git a/gh-review-pull-request b/gh-review-pull-request index 535436a..1537ac5 100755 --- a/gh-review-pull-request +++ b/gh-review-pull-request @@ -1,4 +1,5 @@ #!/bin/zsh +# shellcheck shell=bash # gh review-pull-request โ€” Browse PR review discussions with fzf # # Usage: @@ -7,9 +8,16 @@ # # Options: # -a, --author Filter by author (can be repeated) +# -f, --file Filter by file path (repeatable) +# --since Filter comments created on/after date +# --until Filter comments created on/before date # --all, --all-comments Show all comments in threads (not just latest) # --include-outdated Include outdated comment threads +# --json Print parsed comments as JSON and exit +# --list Print UI list output and exit # --debug Write a small debug summary file +# -R, --repo Target a specific repository +# -v, --version Show version and exit # -h, --help Show this help and exit set -uo pipefail @@ -18,20 +26,16 @@ err() { printf "%s\n" "$*" >&2; } need() { command -v "$1" >/dev/null 2>&1 || { err "Missing dependency: $1"; return 1; } } # Ensure a sane PATH when executed as a gh extension (minimal env) -export PATH="$PATH:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin" +export PATH="$PATH:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:$HOME/.local/bin:$HOME/bin" -# Required deps -for bin in gh jq fzf awk sed base64 wc tr nl cat chmod cut rev; do - need "$bin" || { err "Please install '$bin' and try again."; exit 1; } -done +# (moved dependency checks below after option parsing) # Optional markdown renderer and syntax highlighter GLOW_BIN=$(command -v glow 2>/dev/null || true) MDCAT_BIN=$(command -v mdcat 2>/dev/null || true) -BAT_BIN=$(command -v bat 2>/dev/null || true) # Resolve executables we will call explicitly later -FZF_BIN=$(command -v fzf) +FZF_BIN=$(command -v fzf 2>/dev/null || true) AWK_BIN=$(command -v awk) SED_BIN=$(command -v sed) BASE64_BIN=$(command -v base64) @@ -39,7 +43,6 @@ WC_BIN=$(command -v wc) TR_BIN=$(command -v tr) NL_BIN=$(command -v nl) CAT_BIN=$(command -v cat) -CHMOD_BIN=$(command -v chmod) CUT_BIN=$(command -v cut) REV_BIN=$(command -v rev) OPEN_BIN=$(command -v open 2>/dev/null || true) @@ -57,9 +60,15 @@ Usage: Options: -a, --author Filter by author (can be repeated) + -f, --file Filter by file path (repeatable) + --since Filter comments created on/after date + --until Filter comments created on/before date --all, --all-comments Show all comments in threads (not just latest) --include-outdated Include outdated comment threads + --json Print parsed comments as JSON and exit + --list Print UI list output and exit --debug Write a small debug summary file + -R, --repo Target a specific repository -h, --help Show this help and exit -v, --version Show version and exit USAGE @@ -217,14 +226,18 @@ sub_copy() { body_b64=$(get_field "$f" Body) decoded_body=$(decode_base64 "$body_b64") - { + if { printf "Review Comment\n" printf "Author: @%s\n" "$author" printf "File: %s\n" "$path" printf "Date: %s\n" "$date" printf "URL: %s\n\n" "$url" printf "%s\n" "$decoded_body" - } | copy_to_clipboard || true + } | copy_to_clipboard; then + copied_ok=1 + else + copied_ok=0 + fi # Optional friendly message to be printed by the parent after fzf exits if [ -n "$msgfile" ]; then @@ -235,12 +248,16 @@ sub_copy() { fi # Prepare in-interface toast for preview (once) - if printf "%s" "$url" | grep -q '#'; then - local anchor="${url##*#}" - anchor="${anchor#discussion_r}" - printf "โœ” Copied discussion %s" "$anchor" > "$tmpdir/toast" 2>/dev/null || true + if [ "$copied_ok" -eq 1 ]; then + if printf "%s" "$url" | grep -q '#'; then + local anchor="${url##*#}" + anchor="${anchor#discussion_r}" + printf "โœ” Copied discussion %s" "$anchor" > "$tmpdir/toast" 2>/dev/null || true + else + printf "โœ” Copied discussion" > "$tmpdir/toast" 2>/dev/null || true + fi else - printf "โœ” Copied discussion" > "$tmpdir/toast" 2>/dev/null || true + printf "โš  Could not copy to clipboard (install pbcopy, xclip, or xsel)" > "$tmpdir/toast" 2>/dev/null || true fi } @@ -273,16 +290,24 @@ esac # Parse args: optional PR number (positional) and author filter(s) PR_NUMBER="" AUTHORS="" +# Optional repo override +REPO_ARG="" # Display mode: latest non-outdated comment per unresolved thread by default SHOW_MODE="latest" # values: latest|all INCLUDE_OUTDATED=0 # 0: exclude outdated comments, 1: include DEBUG=0 # 1: print debug info about threads +OUTPUT_JSON=0 # 1: print parsed comments as JSON and exit +OUTPUT_LIST=0 # 1: print UI list and exit +FILES="" # comma-separated file path/regex filters +SINCE="" # ISO date YYYY-MM-DD +UNTIL="" # ISO date YYYY-MM-DD normalize_author() { local a="${1:-}" + a="${a#@}" case "$a" in - copilot|@copilot) printf "%s" "copilot-pull-request-reviewer" ;; - *) printf "%s" "$a" ;; + copilot) printf "%s" "copilot-pull-request-reviewer" ;; + *) printf "%s" "$a" ;; esac } @@ -294,42 +319,81 @@ while [ $# -gt 0 ]; do if [ -z "$AUTHORS" ]; then AUTHORS="$norm"; else AUTHORS="$AUTHORS,$norm"; fi shift 2 ;; + -f|--file) + [ -n "${2:-}" ] || { err "--file requires a value"; exit 1; } + if [ -z "$FILES" ]; then FILES="$2"; else FILES="$FILES,$2"; fi + shift 2 + ;; + --since) + [ -n "${2:-}" ] || { err "--since requires YYYY-MM-DD"; exit 1; } + SINCE="$2"; shift 2 ;; + --until) + [ -n "${2:-}" ] || { err "--until requires YYYY-MM-DD"; exit 1; } + UNTIL="$2"; shift 2 ;; --all|--all-comments) SHOW_MODE="all"; shift ;; --include-outdated) INCLUDE_OUTDATED=1; shift ;; --debug) DEBUG=1; shift ;; + --json) + OUTPUT_JSON=1; shift ;; + --list) + OUTPUT_LIST=1; shift ;; + -R|--repo) + [ -n "${2:-}" ] || { err "--repo requires "; exit 1; } + REPO_ARG="$2"; shift 2 ;; -v|--version) print_version_and_exit ;; -h|--help) usage; exit 0 ;; - ''|*[!0-9]*) - # Unknown/non-numeric arg โ€” ignore to stay simple - shift - ;; + --) + shift; break ;; + -*) + err "Unknown option: $1"; err "Run with --help for usage."; exit 1 ;; *) PR_NUMBER="$1"; shift ;; esac done +# Determine mode-specific dependencies now that flags are parsed +UI_MODE=1; [ "${OUTPUT_JSON:-0}" -eq 1 ] && UI_MODE=0; [ "${OUTPUT_LIST:-0}" -eq 1 ] && UI_MODE=0 +API_MODE=1; [ -n "${GH_REVIEW_PR_JSON:-}" ] && API_MODE=0 + +# Base requirements +for bin in jq awk sed base64 wc tr nl cat cut rev; do + need "$bin" || { err "Please install '$bin' and try again."; exit 1; } +done + +# Interactive UI requirement +if [ "$UI_MODE" -eq 1 ]; then + need fzf || { err "Please install 'fzf' and try again."; exit 1; } +fi + # Infer owner/repo OWNER=""; REPO="" -if info="$(gh repo view --json owner,name --jq '.owner.login+" " + .name' 2>/dev/null)"; then - OWNER="${info%% *}" - REPO="${info##* }" +if [ -n "$REPO_ARG" ]; then + case "$REPO_ARG" in + */*) OWNER="${REPO_ARG%%/*}"; REPO="${REPO_ARG#*/}" ;; + *) err "--repo must be in the form owner/repo"; exit 1 ;; + esac else - remote_url="$(git remote get-url origin 2>/dev/null || git remote get-url upstream 2>/dev/null || true)" - [ -n "${remote_url:-}" ] || { err "Cannot infer repository; ensure you're inside a GitHub repo or run gh repo set-default."; exit 1; } - if printf "%s" "$remote_url" | grep -q '^git@github.com:'; then - path_part="${remote_url#*:}"; path_part="${path_part%.git}" - elif printf "%s" "$remote_url" | grep -q '^https://github.com/'; then - path_part="${remote_url#https://github.com/}"; path_part="${path_part%.git}" + if info="$(gh repo view --json owner,name --jq '.owner.login+" " + .name' 2>/dev/null)"; then + OWNER="${info%% *}" + REPO="${info##* }" else - err "Remote is not a GitHub URL: $remote_url"; exit 1 + remote_url="$(git remote get-url origin 2>/dev/null || git remote get-url upstream 2>/dev/null || true)" + [ -n "${remote_url:-}" ] || { err "Cannot infer repository; ensure you're inside a GitHub repo or pass --repo."; exit 1; } + if printf "%s" "$remote_url" | grep -q '^git@github.com:'; then + path_part="${remote_url#*:}"; path_part="${path_part%.git}" + elif printf "%s" "$remote_url" | grep -q '^https://github.com/'; then + path_part="${remote_url#https://github.com/}"; path_part="${path_part%.git}" + else + err "Remote is not a GitHub URL: $remote_url"; exit 1 + fi + OWNER="${path_part%%/*}"; REPO="${path_part#*/}" fi - OWNER="${path_part%%/*}"; REPO="${path_part#*/}" fi # Infer PR number if not provided @@ -376,20 +440,33 @@ query($owner:String!, $name:String!, $number:Int!) { } GQL -# Fetch JSON with better error reporting -GH_ERR_FILE="$(mktemp -t gh-review-pr-gherr.XXXXXX)" -JSON="$(gh api graphql -f query="$QUERY" -f owner="$OWNER" -f name="$REPO" -F number="$PR_NUMBER" 2>"$GH_ERR_FILE")" || { - err "GitHub API failed:" - if [ -s "$GH_ERR_FILE" ]; then - # Show first few lines of the underlying error for context - head -n 10 "$GH_ERR_FILE" >&2 +# Create temp workspace early and ensure cleanup +TMPDIR="$(mktemp -d -t gh-review-pull-request.XXXXXX)" +cleanup() { command rm -rf "$TMPDIR" >/dev/null 2>&1 || true; } +trap cleanup EXIT INT TERM + +# Fetch JSON with better error reporting, allowing override via env +if [ -n "${GH_REVIEW_PR_JSON:-}" ]; then + if [ -f "$GH_REVIEW_PR_JSON" ]; then + JSON="$(cat "$GH_REVIEW_PR_JSON")" else - err "Are you authenticated? Try: gh auth status; gh auth login" + JSON="$GH_REVIEW_PR_JSON" fi +else + GH_ERR_FILE="$(mktemp -t gh-review-pr-gherr.XXXXXX)" + JSON="$(gh api graphql -f query="$QUERY" -f owner="$OWNER" -f name="$REPO" -F number="$PR_NUMBER" 2>"$GH_ERR_FILE")" || { + err "GitHub API failed:" + if [ -s "$GH_ERR_FILE" ]; then + # Show first few lines of the underlying error for context + head -n 10 "$GH_ERR_FILE" >&2 + else + err "Are you authenticated? Try: gh auth status; gh auth login" + fi + rm -f "$GH_ERR_FILE" >/dev/null 2>&1 || true + exit 1 + } rm -f "$GH_ERR_FILE" >/dev/null 2>&1 || true - exit 1 -} -rm -f "$GH_ERR_FILE" >/dev/null 2>&1 || true +fi # Validate/clean JSON printf '%s' "$JSON" | jq . >/dev/null 2>&1 || { @@ -409,12 +486,6 @@ if [ "$DEBUG" -eq 1 ]; then err "[debug] Wrote thread summary to: $DBG_FILE" fi -# Temp workspace -TMPDIR="/tmp/gh-review-pull-request-$$" -mkdir -p "$TMPDIR" -cleanup() { command rm -rf "$TMPDIR" >/dev/null 2>&1 || true; } -trap cleanup EXIT INT TERM - # Extract unresolved comments into TSV: # author, path, line, url, date, preview, diff_b64, body_b64 TSV="$TMPDIR/comments.tsv" @@ -422,8 +493,12 @@ TSV="$TMPDIR/comments.tsv" printf '%s' "$JSON" | jq -r \ --arg authors "$AUTHORS" \ --arg show_mode "$SHOW_MODE" \ + --arg files "$FILES" \ + --arg since "$SINCE" \ + --arg until "$UNTIL" \ --argjson include_outdated "$INCLUDE_OUTDATED" ' ($authors | split(",") | map(select(length>0))) as $A | + ($files | split(",") | map(select(length>0))) as $F | .data.repository.pullRequest.reviewThreads.nodes // [] | map(select(.isResolved == false)) | (if $include_outdated == 1 then . else map(select(.line != null)) end) | @@ -437,6 +512,12 @@ printf '%s' "$JSON" | jq -r \ ) ) | add // [] | + # File and date filters at comment level + ( if ($F|length) > 0 then map(select(any($F[]; ((.thread_path // .path // "") | test(.)) ))) else . end ) | + ( if ($since|length) > 0 then map(select((.createdAt|fromdateiso8601) >= ($since|fromdateiso8601))) else . end ) | + ( if ($until|length) > 0 then map(select((.createdAt|fromdateiso8601) <= ($until|fromdateiso8601))) else . end ) | + # Sort by file, then date desc + sort_by((.thread_path // .path // ""), -(.createdAt|fromdateiso8601)) | .[] | [ (.author.login // "unknown"), @@ -452,10 +533,11 @@ printf '%s' "$JSON" | jq -r \ if [ ! -s "$TSV" ]; then err "No unresolved review comments found for ${OWNER}/${REPO} PR #${PR_NUMBER}." + err "Tip: try --all or --include-outdated to broaden results." exit 0 fi -# Create per-comment files and display list +# Create per-comment files and display list (file-first) LIST="$TMPDIR/list" : > "$LIST" idx=0 @@ -484,16 +566,18 @@ while IFS=$'\t' read -r author path line url date preview diff_b64 body_b64; do truncated_path="$full_path" fi - # Format: Author (8 chars) + space + Path (up to 50 chars) + space + date - max_path_length=50 + # Format: Path (up to 56 chars) + space + Author (10 chars) + space + date + max_path_length=56 if [ ${#truncated_path} -gt $max_path_length ]; then truncated_path="$(echo "$truncated_path" | "$CUT_BIN" -c1-$((max_path_length-1)))โ€ฆ" fi - printf '\033[36m%-8s\033[0m \033[33m%-50s\033[0m \033[90m%s\033[0m\n' \ - "$display_author" \ + # Visible columns + hidden full path (tab-delimited for better fzf matching) + printf '\033[33m%-56s\033[0m \033[36m%-10s\033[0m \033[90m%s\033[0m\t%s\n' \ "$truncated_path" \ - "$date" >> "$LIST" + "$display_author" \ + "$date" \ + "$full_path" >> "$LIST" idx=$((idx+1)) done < "$TSV" @@ -503,6 +587,29 @@ Enter/Ctrl-Y: copy โ€ข Ctrl-O: open in browser " +# If requested, emit JSON or list and exit early (useful for CI/tests) +if [ "$OUTPUT_JSON" -eq 1 ]; then + # TSV columns: author, path, line, url, date, preview, diff_b64, body_b64 + "$CAT_BIN" "$TSV" | jq -R -s ' + split("\n")[:-1] | map(split("\t")) | + map({ + author: .[0], + path: .[1], + line: (.[2] | tonumber? // null), + url: .[3], + date: .[4], + preview: .[5], + diff_b64: .[6], + body_b64: .[7] + })' || true + exit 0 +fi + +if [ "$OUTPUT_LIST" -eq 1 ]; then + "$CAT_BIN" "$LIST" + exit 0 +fi + # Run fzf SCRIPT_PATH="$0" case "$SCRIPT_PATH" in @@ -516,6 +623,7 @@ MSG_FILE="$TMPDIR/copied-msg" --prompt='comments> ' \ --header="$header" \ --height=100% --border=rounded --layout=reverse \ + --delimiter='\t' --with-nth=2 \ --preview-window='right:60%:wrap' \ --preview "'$SCRIPT_PATH' __preview {} $TMPDIR ${OWNER}/${REPO} ${PR_NUMBER}" \ --bind "enter:execute-silent('$SCRIPT_PATH' __copy {} $TMPDIR $MSG_FILE)+refresh-preview" \