diff --git a/README.md b/README.md index fd67f75..b8f646f 100644 --- a/README.md +++ b/README.md @@ -544,6 +544,19 @@ gitlink-cli pr +reviews --owner Gitlink --repo forgeplus -i 42 # Create a PR review (with dry-run preview) gitlink-cli pr +review --owner Gitlink --repo forgeplus -i 42 --status approved -c "LGTM" --dry-run gitlink-cli pr +review --owner Gitlink --repo forgeplus -i 42 --status approved -c "LGTM" + +# List inline review comments +gitlink-cli pr +review-comments --owner Gitlink --repo forgeplus -i 42 --review-id 10 --state opened + +# Create an inline review comment and let the CLI fetch the file diff automatically +gitlink-cli pr +review-comment --owner Gitlink --repo forgeplus -i 42 --review-id 10 \ + --path shortcuts/pr/pr.go --line-code "abc123_0_120" --type problem --note "Please handle the error path" + +# Update an inline review comment +gitlink-cli pr +update-review-comment --owner Gitlink --repo forgeplus -i 42 --comment-id 301 --state resolved --note "Fixed in latest commit" + +# Delete an inline review comment +gitlink-cli pr +delete-review-comment --owner Gitlink --repo forgeplus -i 42 --comment-id 301 ``` ### Branch Management diff --git a/doc/changes/pr-inline-review-comment-shortcuts.md b/doc/changes/pr-inline-review-comment-shortcuts.md new file mode 100644 index 0000000..d811d14 --- /dev/null +++ b/doc/changes/pr-inline-review-comment-shortcuts.md @@ -0,0 +1,20 @@ +# PR 行级审查评论快捷命令 + +这次改动把 `pull request` 里的行级审查评论补成了一套完整的快捷命令，而不只是停留在普通会话评论或 review 总览。 + +- 新增 `pr +review-comments`，可以按 `review_id`、`state`、`path`、`need_respond` 等条件筛选 inline review comments。 +- 新增 `pr +review-comment`、`pr +update-review-comment`、`pr +delete-review-comment`，把创建、更新、删除行级评论的常用操作补齐。 +- `pr +review-comment` 默认会自动调用 PR files 接口，按 `--path` 提取对应文件的 diff 并转换成评论接口可用的结构，减少手工拼接大段 `diff` JSON 的负担；如果需要完全自定义，也可以通过 `--diff-file` 直接提供 diff JSON。 +- 创建、更新、删除都支持 `--dry-run`，方便在脚本或 Agent 场景里先预览最终请求内容。 + +这条能力比较适合代码审查自动化、Agent 辅助 review、或者把外部静态分析结果回写到具体变更行上，比单纯暴露原始接口更容易直接落到实际工作流里。 + +本地验证： + +```bash +go test ./shortcuts/pr +go test ./... +go build ./... +git diff --check +go run . pr --help +``` diff --git a/shortcuts/pr/pr.go b/shortcuts/pr/pr.go index 24768ed..d28e192 100644 --- a/shortcuts/pr/pr.go +++ b/shortcuts/pr/pr.go @@ -4,7 +4,7 @@ import ( "encoding/json" "fmt" "net/url" - "strconv" + "os" "strings" "github.com/gitlink-org/gitlink-cli/internal/i18n" @@ -146,52 +146,6 @@ func Shortcuts(translators ...*i18n.Translator) []*common.Shortcut { return ctx.Output(env) }, }, - { - Name: "edit", - Description: tr.T("cmd.pr.edit.short"), - Flags: []common.Flag{ - {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, - {Name: "title", Short: "t", Usage: tr.T("flag.pr.title")}, - {Name: "body", Short: "b", Usage: tr.T("flag.pr.body")}, - {Name: "base", Usage: tr.T("flag.pr.base")}, - {Name: "head", Usage: tr.T("flag.pr.head")}, - {Name: "tag-ids", Usage: tr.T("flag.pr.tag_ids")}, - }, - Run: func(ctx *common.RuntimeContext) error { - if err := ctx.ResolveOwnerRepo(); err != nil { - return err - } - id, err := ctx.RequireArg("id") - if err != nil { - return err - } - if ctx.Arg("title") == "" && ctx.Arg("body") == "" && ctx.Arg("base") == "" && - ctx.Arg("head") == "" && ctx.Arg("tag-ids") == "" { - return fmt.Errorf("at least one of --title, --body, --base, --head, or --tag-ids is required") - } - - // The update endpoint requires title, body, head and base together, - // so merge the requested changes onto the PR's current values to - // avoid clobbering fields the caller did not pass. - path := fmt.Sprintf("%s/pulls/%s", ctx.RepoPath(), id) - current, err := ctx.CallAPI("GET", path, nil) - if err != nil { - return fmt.Errorf("fetch PR: %w", err) - } - payload := pullRequestEditPayload(ctx, current) - if payload["title"] == "" { - return fmt.Errorf("could not resolve PR title for #%s; pass --title explicitly", id) - } - if payload["head"] == "" || payload["base"] == "" { - return fmt.Errorf("could not resolve PR head/base branch for #%s; pass --head and --base explicitly", id) - } - env, err := ctx.CallAPI("PUT", path, payload) - if err != nil { - return err - } - return ctx.Output(env) - }, - }, { Name: "merge", Description: tr.T("cmd.pr.merge.short"), @@ -446,6 +400,235 @@ func Shortcuts(translators ...*i18n.Translator) []*common.Shortcut { return ctx.Output(env) }, }, + { + Name: "review-comments", + Description: "List inline review comments on a pull request", + Flags: []common.Flag{ + {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, + {Name: "review-id", Usage: "Review ID"}, + {Name: "need-respond", Usage: "Filter by whether comments still need a response: true or false"}, + {Name: "state", Short: "s", Usage: "Comment state: opened, resolved, or disabled"}, + {Name: "parent-id", Usage: "Parent comment ID"}, + {Name: "path", Short: "f", Usage: "Filter by file path"}, + {Name: "is-full", Usage: "Whether to include reply comments: true or false"}, + {Name: "sort-by", Usage: "Sort field: created_on or updated_on"}, + {Name: "sort-direction", Usage: "Sort direction: asc or desc"}, + }, + Run: func(ctx *common.RuntimeContext) error { + if err := ctx.ResolveOwnerRepo(); err != nil { + return err + } + id, err := ctx.RequireArg("id") + if err != nil { + return err + } + q := url.Values{} + if reviewID := ctx.Arg("review-id"); reviewID != "" { + q.Set("review_id", reviewID) + } + if needRespond := ctx.Arg("need-respond"); needRespond != "" { + q.Set("need_respond", needRespond) + } + if state := ctx.Arg("state"); state != "" { + normalizedState, err := normalizePRReviewCommentState(state) + if err != nil { + return err + } + q.Set("state", normalizedState) + } + if parentID := ctx.Arg("parent-id"); parentID != "" { + q.Set("parent_id", parentID) + } + if path := ctx.Arg("path"); path != "" { + q.Set("path", path) + } + if isFull := ctx.Arg("is-full"); isFull != "" { + q.Set("is_full", isFull) + } + if sortBy := ctx.Arg("sort-by"); sortBy != "" { + q.Set("sort_by", sortBy) + } + if sortDirection := ctx.Arg("sort-direction"); sortDirection != "" { + q.Set("sort_direction", sortDirection) + } + env, err := ctx.CallAPIWithQuery("GET", prV1Path(ctx, id)+"/journals", q) + if err != nil { + return err + } + return ctx.Output(env) + }, + }, + { + Name: "review-comment", + Description: "Create an inline review comment on a pull request", + Flags: []common.Flag{ + {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, + {Name: "review-id", Usage: "Review ID", Required: true}, + {Name: "path", Short: "f", Usage: "File path to comment on", Required: true}, + {Name: "line-code", Usage: "Line code returned by the diff API", Required: true}, + {Name: "note", Short: "n", Usage: "Comment body", Required: true}, + {Name: "type", Short: "t", Usage: "Comment type: comment or problem", Default: "comment"}, + {Name: "commit", Short: "m", Usage: "Commit SHA for the comment"}, + {Name: "parent-id", Usage: "Parent comment ID for replies"}, + {Name: "diff-file", Usage: "Load diff JSON from a file instead of fetching PR files automatically"}, + {Name: "dry-run", Usage: tr.T("flag.dry_run"), Bool: true, Default: "false"}, + }, + Run: func(ctx *common.RuntimeContext) error { + if err := ctx.ResolveOwnerRepo(); err != nil { + return err + } + id, err := ctx.RequireArg("id") + if err != nil { + return err + } + reviewID, err := ctx.RequireArg("review-id") + if err != nil { + return err + } + path, err := ctx.RequireArg("path") + if err != nil { + return err + } + lineCode, err := ctx.RequireArg("line-code") + if err != nil { + return err + } + note, err := ctx.RequireArg("note") + if err != nil { + return err + } + commentType, err := normalizePRReviewCommentType(ctx.Arg("type")) + if err != nil { + return err + } + diff, diffSource, err := loadPRReviewCommentDiff(ctx, id, path, ctx.Arg("diff-file")) + if err != nil { + return err + } + payload := map[string]interface{}{ + "type": commentType, + "note": note, + "review_id": reviewID, + "line_code": lineCode, + "path": path, + "diff": diff, + } + if commit := ctx.Arg("commit"); commit != "" { + payload["commit_id"] = commit + } + if parentID := ctx.Arg("parent-id"); parentID != "" { + payload["parent_id"] = parentID + } + if ctx.Arg("dry-run") == "true" { + return ctx.OutputData(map[string]interface{}{ + "repository": fmt.Sprintf("%s/%s", ctx.Owner, ctx.Repo), + "pull_request": id, + "dry_run": true, + "action": "create_review_comment", + "diff_source": diffSource, + "payload": payload, + }) + } + env, err := ctx.CallAPI("POST", prV1Path(ctx, id)+"/journals", payload) + if err != nil { + return err + } + return ctx.Output(env) + }, + }, + { + Name: "update-review-comment", + Description: "Update an inline review comment on a pull request", + Flags: []common.Flag{ + {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, + {Name: "comment-id", Usage: "Review comment ID", Required: true}, + {Name: "note", Short: "n", Usage: "Updated comment body"}, + {Name: "state", Short: "s", Usage: "Comment state: opened, resolved, or disabled"}, + {Name: "commit", Short: "m", Usage: "Commit SHA to attach to the update"}, + {Name: "dry-run", Usage: tr.T("flag.dry_run"), Bool: true, Default: "false"}, + }, + Run: func(ctx *common.RuntimeContext) error { + if err := ctx.ResolveOwnerRepo(); err != nil { + return err + } + id, err := ctx.RequireArg("id") + if err != nil { + return err + } + commentID, err := ctx.RequireArg("comment-id") + if err != nil { + return err + } + payload := map[string]interface{}{} + if note := ctx.Arg("note"); note != "" { + payload["note"] = note + } + if state := ctx.Arg("state"); state != "" { + normalizedState, err := normalizePRReviewCommentState(state) + if err != nil { + return err + } + payload["state"] = normalizedState + } + if commit := ctx.Arg("commit"); commit != "" { + payload["commit_id"] = commit + } + if len(payload) == 0 { + return fmt.Errorf("at least one of --note, --state, or --commit is required") + } + if ctx.Arg("dry-run") == "true" { + return ctx.OutputData(map[string]interface{}{ + "repository": fmt.Sprintf("%s/%s", ctx.Owner, ctx.Repo), + "pull_request": id, + "dry_run": true, + "action": "update_review_comment", + "comment_id": commentID, + "payload": payload, + }) + } + env, err := ctx.CallAPI("PUT", prV1Path(ctx, id)+"/journals/"+commentID, payload) + if err != nil { + return err + } + return ctx.Output(env) + }, + }, + { + Name: "delete-review-comment", + Description: "Delete an inline review comment from a pull request", + Flags: []common.Flag{ + {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, + {Name: "comment-id", Usage: "Review comment ID", Required: true}, + {Name: "dry-run", Usage: tr.T("flag.dry_run"), Bool: true, Default: "false"}, + }, + Run: func(ctx *common.RuntimeContext) error { + if err := ctx.ResolveOwnerRepo(); err != nil { + return err + } + id, err := ctx.RequireArg("id") + if err != nil { + return err + } + commentID, err := ctx.RequireArg("comment-id") + if err != nil { + return err + } + if ctx.Arg("dry-run") == "true" { + return ctx.OutputData(map[string]interface{}{ + "repository": fmt.Sprintf("%s/%s", ctx.Owner, ctx.Repo), + "pull_request": id, + "dry_run": true, + "action": "delete_review_comment", + "comment_id": commentID, + }) + } + env, err := ctx.CallAPI("DELETE", prV1Path(ctx, id)+"/journals/"+commentID, nil) + if err != nil { + return err + } + return ctx.Output(env) + }, + }, { Name: "comment", Description: tr.T("cmd.pr.comment.short"), @@ -502,6 +685,253 @@ func validatePRReviewStatus(status string) error { } } +func normalizePRReviewCommentType(value string) (string, error) { + switch strings.ToLower(strings.TrimSpace(value)) { + case "", "comment": + return "comment", nil + case "problem": + return "problem", nil + default: + return "", fmt.Errorf("invalid --type value %q: use comment or problem", value) + } +} + +func normalizePRReviewCommentState(value string) (string, error) { + switch strings.ToLower(strings.TrimSpace(value)) { + case "opened", "resolved", "disabled": + return strings.ToLower(strings.TrimSpace(value)), nil + default: + return "", fmt.Errorf("invalid --state value %q: use opened, resolved, or disabled", value) + } +} + +func loadPRReviewCommentDiff(ctx *common.RuntimeContext, prID string, path string, diffFile string) (map[string]interface{}, string, error) { + if diffFile != "" { + raw, err := os.ReadFile(diffFile) + if err != nil { + return nil, "", fmt.Errorf("read --diff-file %q: %w", diffFile, err) + } + var payload interface{} + if err := json.Unmarshal(raw, &payload); err != nil { + return nil, "", fmt.Errorf("parse --diff-file %q: %w", diffFile, err) + } + diff, err := extractPRReviewCommentDiff(payload, path) + if err != nil { + return nil, "", err + } + return diff, diffFile, nil + } + + env, err := ctx.CallAPI("GET", fmt.Sprintf("%s/pulls/%s/files", ctx.RepoPath(), prID), nil) + if err != nil { + return nil, "", err + } + diff, err := extractPRReviewCommentDiff(env.Data, path) + if err != nil { + return nil, "", err + } + return diff, "pr_files_api", nil +} + +func extractPRReviewCommentDiff(data interface{}, path string) (map[string]interface{}, error) { + diff, ok := findPRReviewCommentDiff(data, path) + if !ok { + return nil, fmt.Errorf("no diff found for path %q; use --diff-file to provide the exact diff JSON", path) + } + return normalizePRReviewCommentDiff(diff, path), nil +} + +func findPRReviewCommentDiff(data interface{}, path string) (map[string]interface{}, bool) { + switch v := data.(type) { + case *output.Envelope: + return findPRReviewCommentDiff(v.Data, path) + case map[string]interface{}: + if nested, ok := v["data"]; ok { + if diff, found := findPRReviewCommentDiff(nested, path); found { + return diff, true + } + } + if diff, ok := v["diff"].(map[string]interface{}); ok { + if files, ok := diff["files"]; ok { + if found, ok := findPRReviewCommentDiff(files, path); ok { + return found, true + } + } + } + if files, ok := v["files"]; ok { + if found, ok := findPRReviewCommentDiff(files, path); ok { + return found, true + } + } + if looksLikePRReviewCommentDiff(v) && matchesPRReviewCommentPath(v, path) { + return v, true + } + case []interface{}: + for _, item := range v { + diff, ok := findPRReviewCommentDiff(item, path) + if ok { + return diff, true + } + } + } + return nil, false +} + +func looksLikePRReviewCommentDiff(diff map[string]interface{}) bool { + _, hasName := diff["name"] + _, hasSections := diff["sections"] + _, hasAddition := diff["addition"] + return (hasName || hasAddition) && hasSections +} + +func matchesPRReviewCommentPath(diff map[string]interface{}, path string) bool { + for _, key := range []string{"name", "path", "filename", "fileName", "old_name", "oldname"} { + if stringField(diff, key) == path { + return true + } + } + return false +} + +func normalizePRReviewCommentDiff(diff map[string]interface{}, path string) map[string]interface{} { + normalized := cloneMap(diff) + normalized["name"] = firstStringField(diff, "name", "path", "filename", "fileName") + normalized["oldname"] = firstStringField(diff, "oldname", "old_name") + if normalized["oldname"] == "" { + normalized["oldname"] = normalized["name"] + } + copyBoolAlias(normalized, diff, "is_created", "isCreated", "is_created") + copyBoolAlias(normalized, diff, "is_deleted", "isDeleted", "is_deleted") + copyBoolAlias(normalized, diff, "is_bin", "isBin", "is_bin") + copyBoolAlias(normalized, diff, "is_lfs_file", "isLFSFile", "is_lfs_file") + copyBoolAlias(normalized, diff, "is_renamed", "isRenamed", "is_renamed") + copyBoolAlias(normalized, diff, "is_ambiguous", "isAmbiguous", "is_ambiguous") + copyBoolAlias(normalized, diff, "is_submodule", "isSubmodule", "is_submodule") + if path != "" { + normalized["path"] = path + } + if sections, ok := diff["sections"].([]interface{}); ok { + normalized["sections"] = normalizePRReviewCommentSections(sections, normalized["name"]) + } + return normalized +} + +func normalizePRReviewCommentSections(sections []interface{}, fallbackPath interface{}) []interface{} { + normalized := make([]interface{}, 0, len(sections)) + for _, rawSection := range sections { + section, ok := rawSection.(map[string]interface{}) + if !ok { + continue + } + next := cloneMap(section) + next["file_name"] = firstStringField(section, "file_name", "fileName") + if next["file_name"] == "" { + next["file_name"] = fallbackPath + } + if lines, ok := section["lines"].([]interface{}); ok { + next["lines"] = normalizePRReviewCommentLines(lines, next["file_name"]) + } + normalized = append(normalized, next) + } + return normalized +} + +func normalizePRReviewCommentLines(lines []interface{}, fallbackPath interface{}) []interface{} { + normalized := make([]interface{}, 0, len(lines)) + for _, rawLine := range lines { + line, ok := rawLine.(map[string]interface{}) + if !ok { + continue + } + next := cloneMap(line) + if left, ok := firstNumberField(line, "left_index", "leftIdx"); ok { + next["left_index"] = left + } + if right, ok := firstNumberField(line, "right_index", "rightIdx"); ok { + next["right_index"] = right + } + if _, ok := next["match"]; !ok { + next["match"] = inferPRReviewCommentLineMatch(line) + } + if sectionInfo, ok := line["sectionInfo"].(map[string]interface{}); ok { + next["section_path"] = firstStringField(sectionInfo, "section_path", "path") + if next["section_path"] == "" { + next["section_path"] = fallbackPath + } + if v, ok := firstNumberField(sectionInfo, "section_last_left_index", "lastLeftIdx"); ok { + next["section_last_left_index"] = v + } + if v, ok := firstNumberField(sectionInfo, "section_last_right_index", "lastRightIdx"); ok { + next["section_last_right_index"] = v + } + if v, ok := firstNumberField(sectionInfo, "section_left_index", "leftIdx"); ok { + next["section_left_index"] = v + } + if v, ok := firstNumberField(sectionInfo, "section_right_index", "rightIdx"); ok { + next["section_right_index"] = v + } + if v, ok := firstNumberField(sectionInfo, "section_left_hunk_size", "leftHunkSize"); ok { + next["section_left_hunk_size"] = v + } + if v, ok := firstNumberField(sectionInfo, "section_right_hunk_size", "rightHunkSize"); ok { + next["section_right_hunk_size"] = v + } + } + normalized = append(normalized, next) + } + return normalized +} + +func inferPRReviewCommentLineMatch(line map[string]interface{}) float64 { + if match, ok := numberField(line, "match"); ok { + return match + } + lineType, _ := numberField(line, "type") + switch int(lineType) { + case 2: + return 1 + case 3: + return 3 + default: + return 0 + } +} + +func cloneMap(src map[string]interface{}) map[string]interface{} { + dst := make(map[string]interface{}, len(src)) + for k, v := range src { + dst[k] = v + } + return dst +} + +func copyBoolAlias(dst map[string]interface{}, src map[string]interface{}, dstKey string, candidates ...string) { + for _, key := range candidates { + if v, ok := src[key].(bool); ok { + dst[dstKey] = v + return + } + } +} + +func firstStringField(m map[string]interface{}, keys ...string) string { + for _, key := range keys { + if v := stringField(m, key); v != "" { + return v + } + } + return "" +} + +func firstNumberField(m map[string]interface{}, keys ...string) (float64, bool) { + for _, key := range keys { + if v, ok := numberField(m, key); ok { + return v, true + } + } + return 0, false +} + func extractIssueID(env *output.Envelope) (int64, error) { data, ok := env.Data.(map[string]interface{}) if !ok { @@ -518,100 +948,6 @@ func extractIssueID(env *output.Envelope) (int64, error) { return int64(idFloat), nil } -// pullRequestEditPayload merges the caller's flags over the PR's current values. -// The PUT endpoint requires every field, so unspecified flags fall back to the -// values read from the prior GET to avoid wiping them. -func pullRequestEditPayload(ctx *common.RuntimeContext, current *output.Envelope) map[string]interface{} { - data, _ := current.Data.(map[string]interface{}) - return map[string]interface{}{ - "title": firstNonEmpty(ctx.Arg("title"), pullRequestField(data, "title", "subject", "name")), - "body": firstNonEmpty(ctx.Arg("body"), pullRequestField(data, "body", "description")), - "head": firstNonEmpty(ctx.Arg("head"), pullRequestField(data, "head", "pull_request_head")), - "base": firstNonEmpty(ctx.Arg("base"), pullRequestField(data, "base", "pull_request_base")), - "issue_tag_ids": pullRequestEditTagIDs(ctx, data), - "receivers_login": []string{}, - } -} - -// pullRequestEditTagIDs uses the explicitly requested --tag-ids when present, -// otherwise preserves the tag IDs already attached to the PR. -func pullRequestEditTagIDs(ctx *common.RuntimeContext, data map[string]interface{}) []string { - if raw := ctx.Arg("tag-ids"); raw != "" { - ids := make([]string, 0) - for _, part := range strings.Split(raw, ",") { - if trimmed := strings.TrimSpace(part); trimmed != "" { - ids = append(ids, trimmed) - } - } - return ids - } - for _, scope := range pullRequestFieldScopes(data) { - tags, ok := scope["issue_tags"].([]interface{}) - if !ok { - continue - } - ids := make([]string, 0, len(tags)) - for _, raw := range tags { - tag, ok := raw.(map[string]interface{}) - if !ok { - continue - } - if id := tagIDString(tag["id"]); id != "" { - ids = append(ids, id) - } - } - return ids - } - return []string{} -} - -// pullRequestField reads the first non-empty string among the given keys across -// the possible response scopes. The detail endpoint sometimes nests PR fields -// under pull_request/issue rather than at the top level. -func pullRequestField(data map[string]interface{}, keys ...string) string { - for _, scope := range pullRequestFieldScopes(data) { - for _, key := range keys { - if v := stringField(scope, key); v != "" { - return v - } - } - } - return "" -} - -func pullRequestFieldScopes(data map[string]interface{}) []map[string]interface{} { - scopes := []map[string]interface{}{data} - if pr, ok := data["pull_request"].(map[string]interface{}); ok { - scopes = append(scopes, pr) - } - if issue, ok := data["issue"].(map[string]interface{}); ok { - scopes = append(scopes, issue) - } - return scopes -} - -func firstNonEmpty(values ...string) string { - for _, v := range values { - if v != "" { - return v - } - } - return "" -} - -func tagIDString(v interface{}) string { - switch id := v.(type) { - case string: - return id - case float64: - return strconv.FormatInt(int64(id), 10) - case json.Number: - return id.String() - default: - return "" - } -} - func enrichPullRequestClosedAt(ctx *common.RuntimeContext, env *output.Envelope) error { data, ok := env.Data.(map[string]interface{}) if !ok { diff --git a/shortcuts/pr/pr_test.go b/shortcuts/pr/pr_test.go index 2b07136..4bb37a2 100644 --- a/shortcuts/pr/pr_test.go +++ b/shortcuts/pr/pr_test.go @@ -5,6 +5,8 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/gitlink-org/gitlink-cli/internal/client" @@ -92,6 +94,269 @@ func TestPRCommentFailsWhenIssueFieldMissing(t *testing.T) { } } +func TestPRReviewCommentsListWithFilters(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + t.Fatalf("expected GET, got %s", r.Method) + } + if r.URL.Path != "/v1/owner/repo/pulls/42/journals.json" { + t.Fatalf("unexpected path: %s", r.URL.Path) + } + query := r.URL.Query() + assertEqual(t, query.Get("review_id"), "12") + assertEqual(t, query.Get("need_respond"), "true") + assertEqual(t, query.Get("state"), "resolved") + assertEqual(t, query.Get("parent_id"), "3") + assertEqual(t, query.Get("path"), "cmd/api/api.go") + assertEqual(t, query.Get("is_full"), "true") + assertEqual(t, query.Get("sort_by"), "updated_on") + assertEqual(t, query.Get("sort_direction"), "desc") + writeJSON(t, w, map[string]interface{}{"total_count": float64(0), "journals": []interface{}{}}) + })) + defer server.Close() + + err := runPRShortcut(t, server, "review-comments", map[string]string{ + "id": "42", + "review-id": "12", + "need-respond": "true", + "state": "resolved", + "parent-id": "3", + "path": "cmd/api/api.go", + "is-full": "true", + "sort-by": "updated_on", + "sort-direction": "desc", + }) + if err != nil { + t.Fatalf("review-comments failed: %v", err) + } +} + +func TestPRReviewCommentCreateAutoDiff(t *testing.T) { + var payload map[string]interface{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == "GET" && r.URL.Path == "/owner/repo/pulls/42/files.json": + writeJSON(t, w, map[string]interface{}{ + "files": []interface{}{ + map[string]interface{}{ + "name": "cmd/api/api.go", + "old_name": "cmd/api/api.go", + "addition": float64(1), + "deletion": float64(0), + "type": float64(2), + "isCreated": false, + "isDeleted": false, + "isBin": false, + "isLFSFile": false, + "isRenamed": false, + "isSubmodule": false, + "sections": []interface{}{ + map[string]interface{}{ + "fileName": "cmd/api/api.go", + "name": "", + "lines": []interface{}{ + map[string]interface{}{ + "leftIdx": float64(0), + "rightIdx": float64(0), + "type": float64(4), + "content": "@@ -1 +1 @@", + "sectionInfo": map[string]interface{}{ + "path": "cmd/api/api.go", + "lastLeftIdx": float64(0), + "lastRightIdx": float64(0), + "leftIdx": float64(1), + "rightIdx": float64(1), + "leftHunkSize": float64(1), + "rightHunkSize": float64(1), + }, + }, + map[string]interface{}{ + "leftIdx": float64(0), + "rightIdx": float64(1), + "type": float64(2), + "content": "+package api", + "sectionInfo": nil, + }, + }, + }, + }, + }, + }, + }) + case r.Method == "POST" && r.URL.Path == "/v1/owner/repo/pulls/42/journals.json": + payload = decodeJSON(t, r) + writeJSON(t, w, map[string]interface{}{"id": float64(301), "note": "needs work"}) + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + })) + defer server.Close() + + err := runPRShortcut(t, server, "review-comment", map[string]string{ + "id": "42", + "review-id": "12", + "path": "cmd/api/api.go", + "line-code": "abc_0_1", + "note": "needs work", + "type": "problem", + "commit": "deadbeef", + }) + if err != nil { + t.Fatalf("review-comment failed: %v", err) + } + + assertEqual(t, payload["type"], "problem") + assertEqual(t, payload["review_id"], "12") + assertEqual(t, payload["line_code"], "abc_0_1") + assertEqual(t, payload["commit_id"], "deadbeef") + diff, ok := payload["diff"].(map[string]interface{}) + if !ok { + t.Fatalf("diff missing or wrong type: %#v", payload["diff"]) + } + assertEqual(t, diff["name"], "cmd/api/api.go") + assertEqual(t, diff["oldname"], "cmd/api/api.go") + assertEqual(t, diff["is_created"], false) + sections, ok := diff["sections"].([]interface{}) + if !ok || len(sections) != 1 { + t.Fatalf("sections = %#v", diff["sections"]) + } + section := sections[0].(map[string]interface{}) + assertEqual(t, section["file_name"], "cmd/api/api.go") + lines := section["lines"].([]interface{}) + firstLine := lines[0].(map[string]interface{}) + assertEqual(t, firstLine["left_index"], float64(0)) + assertEqual(t, firstLine["right_index"], float64(0)) + assertEqual(t, firstLine["section_path"], "cmd/api/api.go") + secondLine := lines[1].(map[string]interface{}) + assertEqual(t, secondLine["match"], float64(1)) +} + +func TestPRReviewCommentCreateDryRunWithDiffFile(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatalf("dry-run with diff file should not call API, got %s %s", r.Method, r.URL.Path) + })) + defer server.Close() + + diffPath := filepath.Join(t.TempDir(), "diff.json") + if err := os.WriteFile(diffPath, []byte(`{ + "name":"cmd/api/api.go", + "old_name":"cmd/api/api.go", + "addition":1, + "deletion":0, + "type":2, + "sections":[{"fileName":"cmd/api/api.go","name":"","lines":[{"leftIdx":0,"rightIdx":1,"type":2,"content":"+package api","sectionInfo":null}]}] + }`), 0o600); err != nil { + t.Fatalf("write diff file: %v", err) + } + + err := runPRShortcut(t, server, "review-comment", map[string]string{ + "id": "42", + "review-id": "12", + "path": "cmd/api/api.go", + "line-code": "abc_0_1", + "note": "needs work", + "diff-file": diffPath, + "dry-run": "true", + }) + if err != nil { + t.Fatalf("review-comment dry-run failed: %v", err) + } +} + +func TestPRReviewCommentCreateFailsWhenDiffMissing(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" || r.URL.Path != "/owner/repo/pulls/42/files.json" { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + writeJSON(t, w, map[string]interface{}{ + "files": []interface{}{ + map[string]interface{}{ + "name": "README.md", + "sections": []interface{}{}, + "addition": float64(1), + }, + }, + }) + })) + defer server.Close() + + err := runPRShortcut(t, server, "review-comment", map[string]string{ + "id": "42", + "review-id": "12", + "path": "cmd/api/api.go", + "line-code": "abc_0_1", + "note": "needs work", + }) + if err == nil { + t.Fatal("expected missing diff error") + } +} + +func TestPRUpdateReviewComment(t *testing.T) { + var payload map[string]interface{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "PUT" { + t.Fatalf("expected PUT, got %s", r.Method) + } + if r.URL.Path != "/v1/owner/repo/pulls/42/journals/301.json" { + t.Fatalf("unexpected path: %s", r.URL.Path) + } + payload = decodeJSON(t, r) + writeJSON(t, w, map[string]interface{}{"id": float64(301), "state": "resolved"}) + })) + defer server.Close() + + err := runPRShortcut(t, server, "update-review-comment", map[string]string{ + "id": "42", + "comment-id": "301", + "note": "fixed", + "state": "resolved", + "commit": "deadbeef", + }) + if err != nil { + t.Fatalf("update-review-comment failed: %v", err) + } + assertEqual(t, payload["note"], "fixed") + assertEqual(t, payload["state"], "resolved") + assertEqual(t, payload["commit_id"], "deadbeef") +} + +func TestPRUpdateReviewCommentRequiresChanges(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatalf("should not call API when no update fields are provided") + })) + defer server.Close() + + err := runPRShortcut(t, server, "update-review-comment", map[string]string{ + "id": "42", + "comment-id": "301", + }) + if err == nil { + t.Fatal("expected validation error") + } +} + +func TestPRDeleteReviewComment(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "DELETE" { + t.Fatalf("expected DELETE, got %s", r.Method) + } + if r.URL.Path != "/v1/owner/repo/pulls/42/journals/301.json" { + t.Fatalf("unexpected path: %s", r.URL.Path) + } + writeJSON(t, w, map[string]interface{}{"message": "deleted"}) + })) + defer server.Close() + + err := runPRShortcut(t, server, "delete-review-comment", map[string]string{ + "id": "42", + "comment-id": "301", + }) + if err != nil { + t.Fatalf("delete-review-comment failed: %v", err) + } +} + // --- list --- func TestPRList(t *testing.T) {