Skip to content

Commit a4e3882

Browse files
boneskullboneskull
committed
fix(github): address PR review comments
- Fix redundant PR number in link text (was '[PR #1 #1]', now '[#1]') - Add GraphQL error handling to GetPRTitles - Add comprehensive tests for GetPRTitles - Update test to expect correct fallback format
1 parent 7e5cc61 commit a4e3882

4 files changed

Lines changed: 151 additions & 10 deletions

File tree

‎internal/github/comments.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,11 @@ func GenerateStackComment(root *tree.Node, currentBranch, trunk, repoURL string,
3737
parentPR := currentNode.Parent.PR
3838
if parentPR > 0 {
3939
parentURL := fmt.Sprintf("%s/pull/%d", repoURL, parentPR)
40-
parentTitle := fmt.Sprintf("PR #%d", parentPR) // fallback
40+
linkText := fmt.Sprintf("#%d", parentPR)
4141
if info, ok := prInfo[parentPR]; ok && info.Title != "" {
42-
parentTitle = info.Title
42+
linkText = fmt.Sprintf("%s #%d", info.Title, parentPR)
4343
}
44-
sb.WriteString(fmt.Sprintf("> **DO NOT MERGE** until [%s #%d](%s) is merged into `%s`.\n\n", parentTitle, parentPR, parentURL, trunk))
44+
sb.WriteString(fmt.Sprintf("> **DO NOT MERGE** until [%s](%s) is merged into `%s`.\n\n", linkText, parentURL, trunk))
4545
} else {
4646
sb.WriteString(fmt.Sprintf("> **DO NOT MERGE** until the parent branch is merged into `%s`.\n\n", trunk))
4747
}
@@ -85,16 +85,16 @@ func renderTree(sb *strings.Builder, node *tree.Node, currentBranch, repoURL str
8585
// Format: "[Title #N](url) - branch: `name`" or just branch name if no PR
8686
if node.PR > 0 {
8787
prURL := fmt.Sprintf("%s/pull/%d", repoURL, node.PR)
88-
title := fmt.Sprintf("PR #%d", node.PR) // fallback if title not available
88+
linkText := fmt.Sprintf("#%d", node.PR)
8989
if info, ok := prInfo[node.PR]; ok && info.Title != "" {
90-
title = info.Title
90+
linkText = fmt.Sprintf("%s #%d", info.Title, node.PR)
9191
}
9292

9393
if isCurrent {
9494
// Bold the link for current PR
95-
fmt.Fprintf(sb, "%s**[%s #%d](%s)** - branch: `%s` *(this PR)*", prefix, title, node.PR, prURL, node.Name)
95+
fmt.Fprintf(sb, "%s**[%s](%s)** - branch: `%s` *(this PR)*", prefix, linkText, prURL, node.Name)
9696
} else {
97-
fmt.Fprintf(sb, "%s[%s #%d](%s) - branch: `%s`", prefix, title, node.PR, prURL, node.Name)
97+
fmt.Fprintf(sb, "%s[%s](%s) - branch: `%s`", prefix, linkText, prURL, node.Name)
9898
}
9999
} else {
100100
// No PR - just show branch name (e.g., trunk)

‎internal/github/comments_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,9 @@ func TestGenerateStackComment(t *testing.T) {
136136
// No PR info provided
137137
comment := GenerateStackComment(root, "feature-auth-tests", "main", testRepoURL, nil)
138138

139-
// Should fallback to "PR #N" format
140-
if !strings.Contains(comment, "[PR #1 #1]") {
141-
t.Error("should fallback to 'PR #N' when title not available")
139+
// Should fallback to just "#N" format (no title)
140+
if !strings.Contains(comment, "[#1](https://github.com/test/repo/pull/1)") {
141+
t.Error("should fallback to '#N' when title not available")
142142
}
143143
})
144144
}

‎internal/github/github.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,16 @@ func (c *Client) GetPRTitles(prNumbers []int) (map[int]PRInfo, error) {
7070
return nil, fmt.Errorf("GraphQL request failed: %w", err)
7171
}
7272

73+
// Check for GraphQL errors (can occur even with HTTP 200)
74+
if errorsRaw, hasErrors := rawResponse["errors"]; hasErrors {
75+
var gqlErrors []struct {
76+
Message string `json:"message"`
77+
}
78+
if err := json.Unmarshal(errorsRaw, &gqlErrors); err == nil && len(gqlErrors) > 0 {
79+
return nil, fmt.Errorf("GraphQL error: %s", gqlErrors[0].Message)
80+
}
81+
}
82+
7383
// Parse the response
7484
result := make(map[int]PRInfo)
7585

‎internal/github/github_test.go‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"encoding/json"
66
"errors"
77
"io"
8+
"strings"
89
"testing"
910
)
1011

@@ -296,6 +297,136 @@ func TestClient_UpdatePRBase(t *testing.T) {
296297
}
297298
}
298299

300+
func TestClient_GetPRTitles(t *testing.T) {
301+
t.Run("success with multiple PRs", func(t *testing.T) {
302+
mock := &mockREST{
303+
postFn: func(path string, body io.Reader, response any) error {
304+
if path != "graphql" {
305+
t.Errorf("expected path %q, got %q", "graphql", path)
306+
}
307+
308+
// Return mock GraphQL response
309+
if raw, ok := response.(*map[string]json.RawMessage); ok {
310+
*raw = map[string]json.RawMessage{
311+
"data": json.RawMessage(`{
312+
"repository": {
313+
"pr1": {"number": 1, "title": "First PR"},
314+
"pr2": {"number": 2, "title": "Second PR"}
315+
}
316+
}`),
317+
}
318+
}
319+
return nil
320+
},
321+
}
322+
323+
client := NewClientWithREST(mock, "owner", "repo")
324+
result, err := client.GetPRTitles([]int{1, 2})
325+
326+
if err != nil {
327+
t.Fatalf("unexpected error: %v", err)
328+
}
329+
if len(result) != 2 {
330+
t.Errorf("expected 2 results, got %d", len(result))
331+
}
332+
if result[1].Title != "First PR" {
333+
t.Errorf("expected title %q, got %q", "First PR", result[1].Title)
334+
}
335+
if result[2].Title != "Second PR" {
336+
t.Errorf("expected title %q, got %q", "Second PR", result[2].Title)
337+
}
338+
})
339+
340+
t.Run("empty input", func(t *testing.T) {
341+
mock := &mockREST{
342+
postFn: func(path string, body io.Reader, response any) error {
343+
t.Error("should not make API call for empty input")
344+
return nil
345+
},
346+
}
347+
348+
client := NewClientWithREST(mock, "owner", "repo")
349+
result, err := client.GetPRTitles([]int{})
350+
351+
if err != nil {
352+
t.Fatalf("unexpected error: %v", err)
353+
}
354+
if len(result) != 0 {
355+
t.Errorf("expected empty result, got %d items", len(result))
356+
}
357+
})
358+
359+
t.Run("GraphQL error in response", func(t *testing.T) {
360+
mock := &mockREST{
361+
postFn: func(path string, body io.Reader, response any) error {
362+
if raw, ok := response.(*map[string]json.RawMessage); ok {
363+
*raw = map[string]json.RawMessage{
364+
"errors": json.RawMessage(`[{"message": "Repository not found"}]`),
365+
}
366+
}
367+
return nil
368+
},
369+
}
370+
371+
client := NewClientWithREST(mock, "owner", "repo")
372+
_, err := client.GetPRTitles([]int{1})
373+
374+
if err == nil {
375+
t.Fatal("expected error, got nil")
376+
}
377+
if !strings.Contains(err.Error(), "Repository not found") {
378+
t.Errorf("expected error to contain 'Repository not found', got %q", err.Error())
379+
}
380+
})
381+
382+
t.Run("API error", func(t *testing.T) {
383+
mock := &mockREST{
384+
postFn: func(path string, body io.Reader, response any) error {
385+
return errors.New("network error")
386+
},
387+
}
388+
389+
client := NewClientWithREST(mock, "owner", "repo")
390+
_, err := client.GetPRTitles([]int{1})
391+
392+
if err == nil {
393+
t.Fatal("expected error, got nil")
394+
}
395+
})
396+
397+
t.Run("some PRs not found", func(t *testing.T) {
398+
mock := &mockREST{
399+
postFn: func(path string, body io.Reader, response any) error {
400+
// PR 2 doesn't exist (null in response)
401+
if raw, ok := response.(*map[string]json.RawMessage); ok {
402+
*raw = map[string]json.RawMessage{
403+
"data": json.RawMessage(`{
404+
"repository": {
405+
"pr1": {"number": 1, "title": "First PR"},
406+
"pr2": null
407+
}
408+
}`),
409+
}
410+
}
411+
return nil
412+
},
413+
}
414+
415+
client := NewClientWithREST(mock, "owner", "repo")
416+
result, err := client.GetPRTitles([]int{1, 2})
417+
418+
if err != nil {
419+
t.Fatalf("unexpected error: %v", err)
420+
}
421+
if len(result) != 1 {
422+
t.Errorf("expected 1 result (PR 2 not found), got %d", len(result))
423+
}
424+
if _, ok := result[1]; !ok {
425+
t.Error("expected PR 1 to be in result")
426+
}
427+
})
428+
}
429+
299430
func TestClient_FindPRByHead(t *testing.T) {
300431
mock := &mockREST{
301432
getFn: func(path string, response any) error {

0 commit comments

Comments
 (0)