From d58d9d03bbf8b661cb760f08c7486bfd197c436e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Mon, 22 Jun 2026 15:16:43 +0100 Subject: [PATCH] fix: support issue links in remote task dialog (#1462) --- apps/backend/internal/github/client.go | 3 + apps/backend/internal/github/controller.go | 72 ++++++--- .../internal/github/controller_test.go | 144 ++++++++++++++++++ apps/backend/internal/github/gh_client.go | 76 +++++++++ apps/backend/internal/github/mock_client.go | 25 +++ .../internal/github/mock_client_test.go | 31 ++++ .../internal/github/mock_controller.go | 15 ++ .../internal/github/mock_controller_test.go | 58 +++++++ apps/backend/internal/github/noop_client.go | 4 + apps/backend/internal/github/pat_client.go | 59 +++++++ apps/backend/internal/github/service_pr.go | 11 +- apps/backend/internal/github/service_test.go | 4 +- ...sk-create-dialog-remote-repo-chip.test.tsx | 19 +++ .../task-create-dialog-remote-repo-chip.tsx | 11 +- .../task-create-dialog-state.test.ts | 103 ++++++++++++- .../components/task-create-dialog-state.ts | 16 +- apps/web/e2e/helpers/api-client.ts | 21 +++ .../task/create-task-remote-repo.spec.ts | 49 ++++++ .../mobile-create-task-remote-repo.spec.ts | 55 +++++++ ...task-create-prompt-autocomplete-qa.spec.ts | 2 + .../e2e/tests/terminal/terminal-agent.spec.ts | 11 +- .../github/use-branches-by-url.test.ts | 17 +++ .../domains/github/use-branches-by-url.ts | 7 +- .../domains/github/use-pr-info-by-url.test.ts | 86 ++++++++++- .../domains/github/use-pr-info-by-url.ts | 118 ++++++++++---- apps/web/lib/api/domains/github-api.test.ts | 15 ++ apps/web/lib/api/domains/github-api.ts | 14 ++ 27 files changed, 974 insertions(+), 72 deletions(-) create mode 100644 apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts diff --git a/apps/backend/internal/github/client.go b/apps/backend/internal/github/client.go index b108b9e0d5..2094c10afe 100644 --- a/apps/backend/internal/github/client.go +++ b/apps/backend/internal/github/client.go @@ -16,6 +16,9 @@ type Client interface { // GetPR retrieves a single pull request by number. GetPR(ctx context.Context, owner, repo string, number int) (*PR, error) + // GetIssue retrieves a single GitHub issue by number. + GetIssue(ctx context.Context, owner, repo string, number int) (*Issue, error) + // FindPRByBranch finds an open PR for the given head branch. FindPRByBranch(ctx context.Context, owner, repo, branch string) (*PR, error) diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index 9ebf4e3390..226d7330bf 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -50,6 +50,7 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) { api.POST("/prs/statuses", c.httpGetPRStatusesBatch) api.POST("/prs/:owner/:repo/:number/reviews", c.httpSubmitReview) api.PUT("/prs/:owner/:repo/:number/merge", c.httpMergePR) + api.GET("/issues/:owner/:repo/:number/info", c.httpGetIssueInfo) api.GET("/watches/pr", c.httpListPRWatches) api.DELETE("/watches/pr/:id", c.httpDeletePRWatch) @@ -354,32 +355,69 @@ func (c *Controller) httpGetPRStatusesBatch(ctx *gin.Context) { func (c *Controller) httpGetPRInfo(ctx *gin.Context) { owner := ctx.Param("owner") repo := ctx.Param("repo") - numberStr := ctx.Param("number") - number, err := strconv.Atoi(numberStr) - if err != nil { - ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid PR number"}) + number, ok := parseRouteNumber(ctx, "invalid PR number") + if !ok { return } pr, err := c.service.GetPR(ctx.Request.Context(), owner, repo, number) if err != nil { - status := http.StatusInternalServerError - var apiErr *GitHubAPIError - if errors.As(err, &apiErr) { - switch apiErr.StatusCode { - case http.StatusNotFound: - status = http.StatusNotFound - case http.StatusUnauthorized: - status = http.StatusUnauthorized - case http.StatusForbidden: - status = http.StatusForbidden - } - } - ctx.JSON(status, gin.H{"error": err.Error()}) + handleGitHubInfoError(ctx, err) return } ctx.JSON(http.StatusOK, pr) } +func (c *Controller) httpGetIssueInfo(ctx *gin.Context) { + owner := ctx.Param("owner") + repo := ctx.Param("repo") + number, ok := parseRouteNumber(ctx, "invalid issue number") + if !ok { + return + } + issue, err := c.service.GetIssue(ctx.Request.Context(), owner, repo, number) + if err != nil { + handleGitHubInfoError(ctx, err) + return + } + ctx.JSON(http.StatusOK, issue) +} + +func parseRouteNumber(ctx *gin.Context, invalidMessage string) (int, bool) { + number, err := strconv.Atoi(ctx.Param("number")) + if err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": invalidMessage}) + return 0, false + } + return number, true +} + +func handleGitHubInfoError(ctx *gin.Context, err error) { + if errors.Is(err, ErrNoClient) { + ctx.JSON(http.StatusServiceUnavailable, gin.H{ + "error": "GitHub is not configured. Connect GitHub in Settings > Integrations.", + "code": "github_not_configured", + }) + return + } + status := http.StatusInternalServerError + message := "failed to fetch GitHub info" + var apiErr *GitHubAPIError + if errors.As(err, &apiErr) { + switch apiErr.StatusCode { + case http.StatusNotFound: + status = http.StatusNotFound + case http.StatusUnauthorized: + status = http.StatusUnauthorized + case http.StatusForbidden: + status = http.StatusForbidden + } + } + if status != http.StatusInternalServerError { + message = err.Error() + } + ctx.JSON(status, gin.H{"error": message}) +} + func (c *Controller) httpSubmitReview(ctx *gin.Context) { owner := ctx.Param("owner") repo := ctx.Param("repo") diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index e916377585..55166f7773 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -23,6 +23,7 @@ import ( // stubClient implements Client with no-op defaults; override fields as needed. type stubClient struct { getPRFunc func(ctx context.Context, owner, repo string, number int) (*PR, error) + getIssueFunc func(ctx context.Context, owner, repo string, number int) (*Issue, error) mergePRFn func(ctx context.Context, owner, repo string, number int, mergeMethod string) error getRepoMergeMethodsFn func() (RepoMergeMethods, error) } @@ -37,6 +38,12 @@ func (s *stubClient) GetPR(ctx context.Context, owner, repo string, number int) } return nil, fmt.Errorf("not implemented") } +func (s *stubClient) GetIssue(ctx context.Context, owner, repo string, number int) (*Issue, error) { + if s.getIssueFunc != nil { + return s.getIssueFunc(ctx, owner, repo, number) + } + return nil, fmt.Errorf("not implemented") +} func (s *stubClient) FindPRByBranch(context.Context, string, string, string) (*PR, error) { return nil, nil } @@ -337,6 +344,143 @@ func TestHttpGetPRInfo_ServiceError(t *testing.T) { } } +func TestHttpGetPRInfo_NoClient(t *testing.T) { + router, _ := setupControllerTest(nil) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/prs/acme/widget/99/info", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503, got %d: %s", w.Code, w.Body.String()) + } + var got struct { + Code string `json:"code"` + } + if err := json.NewDecoder(w.Body).Decode(&got); err != nil { + t.Fatalf("decode response: %v", err) + } + if got.Code != "github_not_configured" { + t.Fatalf("expected github_not_configured code, got %q", got.Code) + } +} + +func TestHttpGetPRInfo_GitHubAPIErrorStatus(t *testing.T) { + sc := &stubClient{ + getPRFunc: func(context.Context, string, string, int) (*PR, error) { + return nil, &GitHubAPIError{ + StatusCode: http.StatusNotFound, + Endpoint: "/repos/acme/widget/pulls/99", + Body: "upstream error", + } + }, + } + router, _ := setupControllerTest(sc) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/prs/acme/widget/99/info", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d", w.Code) + } +} + +func TestHttpGetIssueInfo_Success(t *testing.T) { + sc := &stubClient{ + getIssueFunc: func(_ context.Context, owner, repo string, number int) (*Issue, error) { + if owner != "acme" || repo != "widget" || number != 1456 { + t.Errorf("unexpected params: %s/%s#%d", owner, repo, number) + } + return &Issue{Number: 1456, Title: "fix remote picker"}, nil + }, + } + router, _ := setupControllerTest(sc) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/issues/acme/widget/1456/info", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + var issue Issue + if err := json.NewDecoder(w.Body).Decode(&issue); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if issue.Number != 1456 { + t.Errorf("expected issue number 1456, got %d", issue.Number) + } + if issue.Title != "fix remote picker" { + t.Errorf("expected issue title, got %q", issue.Title) + } +} + +func TestHttpGetIssueInfo_InvalidNumber(t *testing.T) { + router, _ := setupControllerTest(&stubClient{}) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/issues/acme/widget/abc/info", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d", w.Code) + } +} + +func TestHttpGetIssueInfo_ServiceError(t *testing.T) { + sc := &stubClient{ + getIssueFunc: func(context.Context, string, string, int) (*Issue, error) { + return nil, fmt.Errorf("not found") + }, + } + router, _ := setupControllerTest(sc) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/issues/acme/widget/99/info", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusInternalServerError { + t.Fatalf("expected 500, got %d", w.Code) + } +} + +func TestHttpGetIssueInfo_GitHubAPIErrorStatus(t *testing.T) { + tests := []struct { + name string + apiStatus int + wantStatus int + }{ + {name: "not found", apiStatus: http.StatusNotFound, wantStatus: http.StatusNotFound}, + {name: "unauthorized", apiStatus: http.StatusUnauthorized, wantStatus: http.StatusUnauthorized}, + {name: "forbidden", apiStatus: http.StatusForbidden, wantStatus: http.StatusForbidden}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sc := &stubClient{ + getIssueFunc: func(context.Context, string, string, int) (*Issue, error) { + return nil, &GitHubAPIError{ + StatusCode: tt.apiStatus, + Endpoint: "/repos/acme/widget/issues/99", + Body: "upstream error", + } + }, + } + router, _ := setupControllerTest(sc) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/issues/acme/widget/99/info", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != tt.wantStatus { + t.Fatalf("expected %d, got %d", tt.wantStatus, w.Code) + } + }) + } +} + func TestHttpMergePR_Success(t *testing.T) { var called struct { owner string diff --git a/apps/backend/internal/github/gh_client.go b/apps/backend/internal/github/gh_client.go index 5c63e5437e..14b9a52503 100644 --- a/apps/backend/internal/github/gh_client.go +++ b/apps/backend/internal/github/gh_client.go @@ -204,11 +204,39 @@ type ghPR struct { } `json:"author"` } +type ghIssue struct { + Number int `json:"number"` + Title string `json:"title"` + URL string `json:"url"` + State string `json:"state"` + Body string `json:"body"` + CreatedAt time.Time `json:"createdAt"` + UpdatedAt time.Time `json:"updatedAt"` + // The gh CLI currently emits an empty string for open issues. + ClosedAt string `json:"closedAt"` + Author struct { + Login string `json:"login"` + } `json:"author"` + Labels []struct { + Name string `json:"name"` + } `json:"labels"` + Assignees []struct { + Login string `json:"login"` + } `json:"assignees"` +} + func (c *GHClient) GetPR(ctx context.Context, owner, repo string, number int) (*PR, error) { out, err := c.run(ctx, "pr", "view", fmt.Sprintf("%d", number), "--repo", fmt.Sprintf("%s/%s", owner, repo), "--json", "number,title,url,state,body,headRefName,headRefOid,baseRefName,author,isDraft,mergeable,mergeStateStatus,additions,deletions,createdAt,updatedAt,mergedAt,closedAt,reviewRequests") if err != nil { + if isNotFoundErr(err) { + return nil, &GitHubAPIError{ + StatusCode: http.StatusNotFound, + Endpoint: fmt.Sprintf("/repos/%s/%s/pulls/%d", owner, repo, number), + Body: err.Error(), + } + } return nil, fmt.Errorf("get PR #%d: %w", number, err) } var raw ghPR @@ -218,6 +246,27 @@ func (c *GHClient) GetPR(ctx context.Context, owner, repo string, number int) (* return convertGHPR(&raw, owner, repo), nil } +func (c *GHClient) GetIssue(ctx context.Context, owner, repo string, number int) (*Issue, error) { + out, err := c.run(ctx, "issue", "view", fmt.Sprintf("%d", number), + "--repo", fmt.Sprintf("%s/%s", owner, repo), + "--json", "number,title,url,state,body,author,labels,assignees,createdAt,updatedAt,closedAt") + if err != nil { + if isNotFoundErr(err) { + return nil, &GitHubAPIError{ + StatusCode: http.StatusNotFound, + Endpoint: fmt.Sprintf("/repos/%s/%s/issues/%d", owner, repo, number), + Body: err.Error(), + } + } + return nil, fmt.Errorf("get issue #%d: %w", number, err) + } + var raw ghIssue + if err := json.Unmarshal([]byte(out), &raw); err != nil { + return nil, fmt.Errorf("parse issue response: %w", err) + } + return convertGHIssue(&raw, owner, repo), nil +} + func (c *GHClient) FindPRByBranch(ctx context.Context, owner, repo, branch string) (*PR, error) { out, err := c.run(ctx, "pr", "list", "--repo", fmt.Sprintf("%s/%s", owner, repo), @@ -1053,6 +1102,33 @@ func convertGHPR(raw *ghPR, owner, repo string) *PR { return pr } +func convertGHIssue(raw *ghIssue, owner, repo string) *Issue { + labels := make([]string, len(raw.Labels)) + for i, label := range raw.Labels { + labels[i] = label.Name + } + assignees := make([]string, len(raw.Assignees)) + for i, assignee := range raw.Assignees { + assignees[i] = assignee.Login + } + return &Issue{ + Number: raw.Number, + Title: raw.Title, + URL: raw.URL, + HTMLURL: raw.URL, + State: strings.ToLower(raw.State), + Body: raw.Body, + AuthorLogin: raw.Author.Login, + RepoOwner: owner, + RepoName: repo, + Labels: labels, + Assignees: assignees, + CreatedAt: raw.CreatedAt, + UpdatedAt: raw.UpdatedAt, + ClosedAt: parseTimePtr(raw.ClosedAt), + } +} + func convertGHRequestedReviewers(raw []ghRequestedReviewer) []RequestedReviewer { reviewers := make([]RequestedReviewer, 0, len(raw)) for _, req := range raw { diff --git a/apps/backend/internal/github/mock_client.go b/apps/backend/internal/github/mock_client.go index a1c221b083..6eebe3590c 100644 --- a/apps/backend/internal/github/mock_client.go +++ b/apps/backend/internal/github/mock_client.go @@ -18,6 +18,12 @@ type prKey struct { Number int } +type issueKey struct { + Owner string + Repo string + Number int +} + // branchKey is a composite key for PR lookups by owner/repo/branch. type branchKey struct { Owner string @@ -70,6 +76,7 @@ type MockClient struct { // the whole mock client out of the wiring. reposUnavailable bool prs map[prKey]*PR + issues map[issueKey]*Issue prsByBranch map[branchKey]*PR orgs []GitHubOrg repos map[string][]GitHubRepo @@ -115,6 +122,7 @@ func NewMockClient() *MockClient { user: mockDefaultUser, authenticated: true, prs: make(map[prKey]*PR), + issues: make(map[issueKey]*Issue), prsByBranch: make(map[branchKey]*PR), repos: make(map[string][]GitHubRepo), branches: make(map[repoKey][]RepoBranch), @@ -152,6 +160,16 @@ func (m *MockClient) GetPR(_ context.Context, owner, repo string, number int) (* return pr, nil } +func (m *MockClient) GetIssue(_ context.Context, owner, repo string, number int) (*Issue, error) { + m.mu.RLock() + defer m.mu.RUnlock() + issue, ok := m.issues[issueKey{owner, repo, number}] + if !ok { + return nil, fmt.Errorf("mock: issue %s/%s#%d not found", owner, repo, number) + } + return issue, nil +} + func (m *MockClient) FindPRByBranch(_ context.Context, owner, repo, branch string) (*PR, error) { m.findPRByBranchCalls.Add(1) m.mu.RLock() @@ -523,6 +541,12 @@ func (m *MockClient) AddPR(pr *PR) { } } +func (m *MockClient) AddIssue(issue *Issue) { + m.mu.Lock() + defer m.mu.Unlock() + m.issues[issueKey{issue.RepoOwner, issue.RepoName, issue.Number}] = issue +} + // AddOrgs appends organizations to the mock data store. func (m *MockClient) AddOrgs(orgs []GitHubOrg) { m.mu.Lock() @@ -625,6 +649,7 @@ func (m *MockClient) Reset() { m.authError = "" m.reposUnavailable = false m.prs = make(map[prKey]*PR) + m.issues = make(map[issueKey]*Issue) m.prsByBranch = make(map[branchKey]*PR) m.orgs = nil m.repos = make(map[string][]GitHubRepo) diff --git a/apps/backend/internal/github/mock_client_test.go b/apps/backend/internal/github/mock_client_test.go index 7bf3435fca..2aba89b663 100644 --- a/apps/backend/internal/github/mock_client_test.go +++ b/apps/backend/internal/github/mock_client_test.go @@ -69,6 +69,37 @@ func TestMockClient_AddPR_GetPR(t *testing.T) { } } +func TestMockClient_AddIssueGetIssueAndReset(t *testing.T) { + m := NewMockClient() + ctx := context.Background() + + _, err := m.GetIssue(ctx, "owner", "repo", 1456) + if err == nil || err.Error() != "mock: issue owner/repo#1456 not found" { + t.Fatalf("expected missing issue error, got %v", err) + } + + m.AddIssue(&Issue{ + Number: 1456, + Title: "Test issue", + RepoOwner: "owner", + RepoName: "repo", + }) + + issue, err := m.GetIssue(ctx, "owner", "repo", 1456) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if issue.Title != "Test issue" { + t.Fatalf("expected title 'Test issue', got %q", issue.Title) + } + + m.Reset() + _, err = m.GetIssue(ctx, "owner", "repo", 1456) + if err == nil || err.Error() != "mock: issue owner/repo#1456 not found" { + t.Fatalf("expected reset issue error, got %v", err) + } +} + func TestMockClient_FindPRByBranch(t *testing.T) { m := NewMockClient() ctx := context.Background() diff --git a/apps/backend/internal/github/mock_controller.go b/apps/backend/internal/github/mock_controller.go index ab41dc885a..a1180c4120 100644 --- a/apps/backend/internal/github/mock_controller.go +++ b/apps/backend/internal/github/mock_controller.go @@ -35,6 +35,7 @@ func (c *MockController) RegisterRoutes(router *gin.Engine) { api := router.Group("/api/v1/github/mock") api.PUT("/user", c.setUser) api.POST("/prs", c.addPRs) + api.POST("/issues", c.addIssues) api.POST("/orgs", c.addOrgs) api.POST("/repos", c.addRepos) api.POST("/reviews", c.addReviews) @@ -111,6 +112,20 @@ func (c *MockController) addPRs(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"added": len(req.PRs)}) } +func (c *MockController) addIssues(ctx *gin.Context) { + var req struct { + Issues []Issue `json:"issues"` + } + if err := ctx.ShouldBindJSON(&req); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) + return + } + for i := range req.Issues { + c.mock.AddIssue(&req.Issues[i]) + } + ctx.JSON(http.StatusOK, gin.H{"added": len(req.Issues)}) +} + func (c *MockController) addOrgs(ctx *gin.Context) { var req struct { Orgs []GitHubOrg `json:"orgs"` diff --git a/apps/backend/internal/github/mock_controller_test.go b/apps/backend/internal/github/mock_controller_test.go index c36aafd655..68d6675d4a 100644 --- a/apps/backend/internal/github/mock_controller_test.go +++ b/apps/backend/internal/github/mock_controller_test.go @@ -1,11 +1,69 @@ package github import ( + "bytes" "context" + "encoding/json" + "net/http" + "net/http/httptest" "testing" "time" + + "github.com/gin-gonic/gin" ) +func setupMockControllerTestForAddIssues() (*gin.Engine, *MockClient) { + gin.SetMode(gin.TestMode) + mock := NewMockClient() + ctrl := NewMockController(mock, nil, nil, nil, newControllerTestLogger()) + router := gin.New() + ctrl.RegisterRoutes(router) + return router, mock +} + +func TestMockControllerAddIssues(t *testing.T) { + router, mock := setupMockControllerTestForAddIssues() + body := bytes.NewBufferString(`{"issues":[{"number":1456,"title":"Fix picker","repo_owner":"owner","repo_name":"repo"}]}`) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/github/mock/issues", body) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var got struct { + Added int `json:"added"` + } + if err := json.NewDecoder(w.Body).Decode(&got); err != nil { + t.Fatalf("decode response: %v", err) + } + if got.Added != 1 { + t.Fatalf("expected added=1, got %d", got.Added) + } + issue, err := mock.GetIssue(context.Background(), "owner", "repo", 1456) + if err != nil { + t.Fatalf("get seeded issue: %v", err) + } + if issue.Title != "Fix picker" { + t.Fatalf("expected seeded issue title, got %q", issue.Title) + } +} + +func TestMockControllerAddIssuesInvalidPayload(t *testing.T) { + router, _ := setupMockControllerTestForAddIssues() + + req := httptest.NewRequest(http.MethodPost, "/api/v1/github/mock/issues", bytes.NewBufferString(`{`)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String()) + } +} + func TestEnsureMockPRForRequestCopiesMergeableState(t *testing.T) { mock := NewMockClient() controller := &MockController{mock: mock} diff --git a/apps/backend/internal/github/noop_client.go b/apps/backend/internal/github/noop_client.go index eb5d47aae2..c2e2e8b17b 100644 --- a/apps/backend/internal/github/noop_client.go +++ b/apps/backend/internal/github/noop_client.go @@ -25,6 +25,10 @@ func (c *NoopClient) GetPR(context.Context, string, string, int) (*PR, error) { return nil, ErrNoClient } +func (c *NoopClient) GetIssue(context.Context, string, string, int) (*Issue, error) { + return nil, ErrNoClient +} + func (c *NoopClient) FindPRByBranch(context.Context, string, string, string) (*PR, error) { return nil, ErrNoClient } diff --git a/apps/backend/internal/github/pat_client.go b/apps/backend/internal/github/pat_client.go index da124acc1f..a260dce0aa 100644 --- a/apps/backend/internal/github/pat_client.go +++ b/apps/backend/internal/github/pat_client.go @@ -165,6 +165,15 @@ func (c *PATClient) GetPR(ctx context.Context, owner, repo string, number int) ( return convertPatPR(&raw, owner, repo), nil } +func (c *PATClient) GetIssue(ctx context.Context, owner, repo string, number int) (*Issue, error) { + var raw patIssue + endpoint := fmt.Sprintf("/repos/%s/%s/issues/%d", owner, repo, number) + if err := c.get(ctx, endpoint, &raw); err != nil { + return nil, fmt.Errorf("get issue #%d: %w", number, err) + } + return convertPatIssue(&raw, owner, repo), nil +} + func (c *PATClient) FindPRByBranch(ctx context.Context, owner, repo, branch string) (*PR, error) { statuses, err := runBatchedBranchQuery(ctx, c, []graphQLBranchRef{{ Owner: owner, @@ -905,6 +914,26 @@ type patPR struct { } `json:"base"` } +type patIssue struct { + Number int `json:"number"` + Title string `json:"title"` + HTMLURL string `json:"html_url"` + Body string `json:"body"` + State string `json:"state"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + ClosedAt *string `json:"closed_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Labels []struct { + Name string `json:"name"` + } `json:"labels"` + Assignees []struct { + Login string `json:"login"` + } `json:"assignees"` +} + type patSearchItem struct { Number int `json:"number"` Title string `json:"title"` @@ -958,6 +987,36 @@ func convertPatPR(raw *patPR, owner, repo string) *PR { return pr } +func convertPatIssue(raw *patIssue, owner, repo string) *Issue { + labels := make([]string, len(raw.Labels)) + for i, label := range raw.Labels { + labels[i] = label.Name + } + assignees := make([]string, len(raw.Assignees)) + for i, assignee := range raw.Assignees { + assignees[i] = assignee.Login + } + issue := &Issue{ + Number: raw.Number, + Title: raw.Title, + URL: raw.HTMLURL, + HTMLURL: raw.HTMLURL, + State: strings.ToLower(raw.State), + Body: raw.Body, + AuthorLogin: raw.User.Login, + RepoOwner: owner, + RepoName: repo, + Labels: labels, + Assignees: assignees, + CreatedAt: raw.CreatedAt, + UpdatedAt: raw.UpdatedAt, + } + if raw.ClosedAt != nil { + issue.ClosedAt = parseTimePtr(*raw.ClosedAt) + } + return issue +} + func convertPatRequestedReviewers(raw *patPR) []RequestedReviewer { reviewers := make([]RequestedReviewer, 0, len(raw.RequestedReviewers)+len(raw.RequestedTeams)) for _, reviewer := range raw.RequestedReviewers { diff --git a/apps/backend/internal/github/service_pr.go b/apps/backend/internal/github/service_pr.go index 3ef40e02d0..badd8e420b 100644 --- a/apps/backend/internal/github/service_pr.go +++ b/apps/backend/internal/github/service_pr.go @@ -102,11 +102,20 @@ func pickDefaultMergeMethod(m RepoMergeMethods) string { // GetPR fetches basic PR details from GitHub. func (s *Service) GetPR(ctx context.Context, owner, repo string, number int) (*PR, error) { if s.client == nil { - return nil, fmt.Errorf("github client not available") + return nil, ErrNoClient } return s.client.GetPR(ctx, owner, repo, number) } +// GetIssue fetches basic issue details from GitHub. The create-task dialog is +// currently the only caller and dedupes requests per URL on the frontend. +func (s *Service) GetIssue(ctx context.Context, owner, repo string, number int) (*Issue, error) { + if s.client == nil { + return nil, ErrNoClient + } + return s.client.GetIssue(ctx, owner, repo, number) +} + // GetPRFeedback fetches live PR feedback from GitHub. Cached briefly with // singleflight coalescing: the task page fires this for the same PR many times // in quick succession (chat-bar CI chip, detail panel, hover popover — each on diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index 6f32e8e078..9b69db1e4b 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -179,8 +179,8 @@ func TestGetPR_NilClient(t *testing.T) { if err == nil { t.Fatal("expected error when client is nil") } - if !strings.Contains(err.Error(), "github client not available") { - t.Errorf("unexpected error: %v", err) + if !errors.Is(err, ErrNoClient) { + t.Errorf("err = %v, want ErrNoClient", err) } } diff --git a/apps/web/components/task-create-dialog-remote-repo-chip.test.tsx b/apps/web/components/task-create-dialog-remote-repo-chip.test.tsx index a0981a0d30..2aa327919b 100644 --- a/apps/web/components/task-create-dialog-remote-repo-chip.test.tsx +++ b/apps/web/components/task-create-dialog-remote-repo-chip.test.tsx @@ -338,6 +338,25 @@ describe("RemoteRepoChip — picker loading state", () => { }); describe("RemoteRepoChip — popover content", () => { + it("constrains the inline popover to fit inside the create-task dialog body", () => { + renderInProvider( + , + ); + fireEvent.click(screen.getByTestId(TRIGGER_TID)); + const content = screen.getByTestId("remote-repo-popover-content"); + expect(content.className).toContain("max-w-[calc(100vw-2rem)]"); + expect(content.className).toContain("max-h-[min(420px,calc(100vh-12rem))]"); + expect(content.className).toContain("overflow-y-auto"); + }); + it("renders the 'Connect GitHub' banner when accessibleRepos.unavailable=true", () => { renderInProvider( {triggerLabel} - + { @@ -410,7 +415,7 @@ function PasteSection({ onPaste }: { onPaste: (value: string) => void }) { return (
void }) { commit(); } }} - placeholder="github.com/owner/repo or .../pull/123" + placeholder="github.com/owner/repo, .../pull/123, or .../issues/123" data-testid="remote-paste-url-input" className={cn( "h-8 rounded-md px-2 text-xs bg-muted/30 border border-border/60", diff --git a/apps/web/components/task-create-dialog-state.test.ts b/apps/web/components/task-create-dialog-state.test.ts index 49f48c4ace..5a0df9c2f5 100644 --- a/apps/web/components/task-create-dialog-state.test.ts +++ b/apps/web/components/task-create-dialog-state.test.ts @@ -18,11 +18,17 @@ vi.mock("@/hooks/domains/github/use-branches-by-url", () => ({ // `usePRInfoByURL` also touches the network on ensure(); stub it to a // per-test-controlled cache so the title-autofill effect can be exercised -// without an actual fetch. Each test that needs a specific PR-info value -// writes into `prInfoMap` before calling `setUseRemote(true)`. +// without an actual fetch. Each test that needs a specific GitHub URL info +// value writes into `prInfoMap` before calling `setUseRemote(true)`. const prInfoMap = new Map< string, - { prHeadBranch: string; prBaseBranch: string; prNumber: number; suggestedTitle: string } + { + prHeadBranch?: string; + prBaseBranch?: string; + prNumber?: number; + issueNumber?: number; + suggestedTitle: string; + } >(); vi.mock("@/hooks/domains/github/use-pr-info-by-url", async (importOriginal) => { const original = @@ -246,6 +252,9 @@ describe("buildRepositoriesPayload — remoteRepos rows", () => { const PR_URL_42 = "https://github.com/acme/site/pull/42"; const PR_TITLE_42 = "PR #42: Test PR"; +const USER_TYPED_TITLE = "my own title"; +const ISSUE_URL_1456 = "https://github.com/acme/site/issues/1456"; +const ISSUE_TITLE_1456 = "Issue #1456: Fix remote picker"; function seedPRInfo(url: string, prNumber: number, suggestedTitle: string) { prInfoMap.set(url, { @@ -256,7 +265,14 @@ function seedPRInfo(url: string, prNumber: number, suggestedTitle: string) { }); } -describe("useDialogFormState — title autofill from first row PR info", () => { +function seedIssueInfo(url: string, issueNumber: number, suggestedTitle: string) { + prInfoMap.set(url, { + issueNumber, + suggestedTitle, + }); +} + +describe("useDialogFormState — title autofill from first row GitHub URL info", () => { beforeEach(() => { prInfoMap.clear(); }); @@ -279,14 +295,14 @@ describe("useDialogFormState — title autofill from first row PR info", () => { seedPRInfo(PR_URL_42, 42, PR_TITLE_42); const { result } = renderHook(() => useDialogFormState(true, "ws-1", null)); act(() => { - result.current.setTaskName("my own title"); + result.current.setTaskName(USER_TYPED_TITLE); result.current.setUseRemote(true); }); const key = result.current.remoteRepos[0]?.key; act(() => { result.current.updateRemoteRepo(key!, { url: PR_URL_42 }); }); - expect(result.current.taskName).toBe("my own title"); + expect(result.current.taskName).toBe(USER_TYPED_TITLE); }); it("does NOT re-apply autofill after the user clears the title (user took ownership)", () => { @@ -362,3 +378,78 @@ describe("useDialogFormState — title autofill from first row PR info", () => { expect(result.current.taskName).toBe(""); }); }); + +describe("useDialogFormState — title autofill from first row GitHub issue info", () => { + beforeEach(() => { + prInfoMap.clear(); + }); + + it("seeds the task title from the first row's issue info when title is empty", () => { + seedIssueInfo(ISSUE_URL_1456, 1456, ISSUE_TITLE_1456); + const { result } = renderHook(() => useDialogFormState(true, "ws-1", null)); + act(() => { + result.current.setUseRemote(true); + }); + const key = result.current.remoteRepos[0]?.key; + act(() => { + result.current.updateRemoteRepo(key!, { url: ISSUE_URL_1456 }); + }); + expect(result.current.taskName).toBe(ISSUE_TITLE_1456); + expect(result.current.hasTitle).toBe(true); + }); + + it("does NOT overwrite a title the user typed themselves", () => { + seedIssueInfo(ISSUE_URL_1456, 1456, ISSUE_TITLE_1456); + const { result } = renderHook(() => useDialogFormState(true, "ws-1", null)); + act(() => { + result.current.setTaskName(USER_TYPED_TITLE); + result.current.setUseRemote(true); + }); + const key = result.current.remoteRepos[0]?.key; + act(() => { + result.current.updateRemoteRepo(key!, { url: ISSUE_URL_1456 }); + }); + expect(result.current.taskName).toBe(USER_TYPED_TITLE); + }); + + it("does NOT re-apply autofill after the user clears the title", () => { + seedIssueInfo(ISSUE_URL_1456, 1456, ISSUE_TITLE_1456); + const { result } = renderHook(() => useDialogFormState(true, "ws-1", null)); + act(() => { + result.current.setUseRemote(true); + }); + const key = result.current.remoteRepos[0]?.key; + act(() => { + result.current.updateRemoteRepo(key!, { url: ISSUE_URL_1456 }); + }); + expect(result.current.taskName).toBe(ISSUE_TITLE_1456); + act(() => { + result.current.setTaskName(""); + }); + expect(result.current.taskName).toBe(""); + }); + + it("re-applies autofill when the user switches to a different issue URL", () => { + const newIssueURL = "https://github.com/acme/site/issues/1457"; + seedIssueInfo(ISSUE_URL_1456, 1456, ISSUE_TITLE_1456); + seedIssueInfo(newIssueURL, 1457, "Issue #1457: Add issue URL paste"); + const { result } = renderHook(() => useDialogFormState(true, "ws-1", null)); + act(() => { + result.current.setUseRemote(true); + }); + const key = result.current.remoteRepos[0]?.key; + act(() => { + result.current.updateRemoteRepo(key!, { url: ISSUE_URL_1456 }); + }); + expect(result.current.taskName).toBe(ISSUE_TITLE_1456); + act(() => { + result.current.setTaskName(""); + }); + expect(result.current.taskName).toBe(""); + + act(() => { + result.current.updateRemoteRepo(key!, { url: newIssueURL }); + }); + expect(result.current.taskName).toBe("Issue #1457: Add issue URL paste"); + }); +}); diff --git a/apps/web/components/task-create-dialog-state.ts b/apps/web/components/task-create-dialog-state.ts index 6915a38384..82450ab658 100644 --- a/apps/web/components/task-create-dialog-state.ts +++ b/apps/web/components/task-create-dialog-state.ts @@ -396,7 +396,7 @@ export function useDialogFormState( // Title autofill: the first remote-repo row's PR info seeds the task title // when the user hasn't typed one. Keyed only off row 0 — adding more PR // rows later never overwrites the title. - useTitleAutofillFromFirstRowPRInfo({ + useTitleAutofillFromFirstRowGitHubInfo({ open: open && ghUrl.useRemote, firstRowUrl: remoteRepos.remoteRepos[0]?.url ?? "", prInfoByUrl, @@ -428,19 +428,19 @@ export function useDialogFormState( } /** - * Seeds the task title from the first Remote row's PR info (when present) + * Seeds the task title from the first Remote row's GitHub URL info (when present) * the first time it arrives, and only when the user hasn't typed a title. - * Subsequent PR-info changes for the same URL don't overwrite — the - * lastAutoFilledRef tracks our own writes so re-pasting a different PR URL - * still works while a user-edited title is preserved. + * Subsequent info changes for the same URL don't overwrite — the + * lastAutoFilledRef tracks our own writes so re-pasting a different GitHub + * URL still works while a user-edited title is preserved. */ /** Sentinel stored in `lastAutoFilledRef` once the user clears an * auto-filled title. Distinguishable from any real suggested title (which - * always begins with `"PR #"`) so the effect can refuse to re-apply autofill - * for the current URL until the URL itself changes. */ + * comes from the GitHub info loader) so the effect can refuse to re-apply + * autofill for the current URL until the URL itself changes. */ const USER_CLEARED_SENTINEL = "\0cleared"; -function useTitleAutofillFromFirstRowPRInfo(args: { +function useTitleAutofillFromFirstRowGitHubInfo(args: { open: boolean; firstRowUrl: string; prInfoByUrl: ReturnType; diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index feac77fbe2..1dd5f12420 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -41,6 +41,23 @@ export type MockPR = { requested_reviewers?: Array<{ login: string; type: string }>; }; +export type MockIssue = { + number: number; + title: string; + body?: string; + url?: string; + html_url?: string; + state?: string; + author_login?: string; + repo_owner: string; + repo_name: string; + labels?: string[]; + assignees?: string[]; + created_at?: string; + updated_at?: string; + closed_at?: string | null; +}; + export type MockOrg = { login: string; avatar_url?: string; @@ -902,6 +919,10 @@ export class ApiClient { await this.request("POST", "/api/v1/github/mock/prs", { prs }); } + async mockGitHubAddIssues(issues: MockIssue[]): Promise { + await this.request("POST", "/api/v1/github/mock/issues", { issues }); + } + async mockGitHubAddOrgs(orgs: MockOrg[]): Promise { await this.request("POST", "/api/v1/github/mock/orgs", { orgs }); } diff --git a/apps/web/e2e/tests/task/create-task-remote-repo.spec.ts b/apps/web/e2e/tests/task/create-task-remote-repo.spec.ts index 4666111ed8..25fd598285 100644 --- a/apps/web/e2e/tests/task/create-task-remote-repo.spec.ts +++ b/apps/web/e2e/tests/task/create-task-remote-repo.spec.ts @@ -96,6 +96,16 @@ async function pasteUrlInChip(testPage: Page, url: string, chipIndex = 0): Promi await pasteInput.press("Enter"); } +async function expectPopoverFitsDialog(testPage: Page): Promise { + const dialogBox = await testPage.getByTestId("create-task-dialog").boundingBox(); + const popoverBox = await testPage.getByTestId("remote-repo-popover-content").boundingBox(); + expect(dialogBox).not.toBeNull(); + expect(popoverBox).not.toBeNull(); + expect(popoverBox!.y + popoverBox!.height).toBeLessThanOrEqual( + dialogBox!.y + dialogBox!.height + 1, + ); +} + test.describe("Task creation from Remote tab (chip picker)", () => { test.describe.configure({ retries: 1 }); @@ -200,6 +210,45 @@ test.describe("Task creation from Remote tab (chip picker)", () => { expect(repoRow.provider_name).toBe("paste-repo"); }); + test("pasted GitHub issue URL fills the title and keeps the picker inside the dialog", async ({ + testPage, + apiClient, + }) => { + test.setTimeout(60_000); + await apiClient.mockGitHubAddBranches("issue-owner", "issue-repo", [{ name: "main" }]); + await apiClient.mockGitHubAddIssues([ + { + number: 1456, + title: "Fix remote repo picker clipping", + body: "The picker overlaps the dialog footer.", + state: "open", + author_login: "mock-user", + repo_owner: "issue-owner", + repo_name: "issue-repo", + html_url: "https://github.com/issue-owner/issue-repo/issues/1456", + }, + ]); + + const kanban = new KanbanPage(testPage); + await openCreateDialog(testPage, kanban); + await clickRemoteMode(testPage); + + await testPage.getByTestId("remote-repo-chip-trigger").first().click(); + await expectPopoverFitsDialog(testPage); + const pasteInput = testPage.getByTestId("remote-paste-url-input").last(); + await pasteInput.fill("https://github.com/issue-owner/issue-repo/issues/1456"); + await pasteInput.press("Enter"); + + await expect(testPage.getByTestId("task-title-input")).toHaveValue( + "Issue #1456: Fix remote repo picker clipping", + { timeout: 10_000 }, + ); + await expect(testPage.getByTestId("remote-repo-chip-trigger").first()).toContainText( + "issues/1456", + { timeout: 5_000 }, + ); + }); + test("scenario 3: three mixed rows (picker + paste-repo + paste-PR)", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts b/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts new file mode 100644 index 0000000000..44f5f1cc0e --- /dev/null +++ b/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts @@ -0,0 +1,55 @@ +import { test, expect } from "../../fixtures/test-base"; +import type { Page } from "@playwright/test"; +import { MobileKanbanPage } from "../../pages/mobile-kanban-page"; + +async function openRemotePicker(testPage: Page): Promise { + const mobile = new MobileKanbanPage(testPage); + await mobile.goto(); + await mobile.mobileFab.click(); + await expect(testPage.getByTestId("create-task-dialog")).toBeVisible(); + await testPage.getByTestId("source-mode-remote").click(); + await testPage.getByTestId("remote-repo-chip-trigger").first().click(); +} + +async function expectPopoverFitsViewport(testPage: Page): Promise { + const viewport = testPage.viewportSize(); + const box = await testPage.getByTestId("remote-repo-popover-content").boundingBox(); + expect(viewport).not.toBeNull(); + expect(box).not.toBeNull(); + expect(box!.x).toBeGreaterThanOrEqual(0); + expect(box!.x + box!.width).toBeLessThanOrEqual(viewport!.width); + expect(box!.y + box!.height).toBeLessThanOrEqual(viewport!.height); +} + +test.describe("Create task Remote repo picker on mobile", () => { + test.beforeEach(async ({ apiClient }) => { + await apiClient.mockGitHubReset(); + }); + + test("pastes a GitHub issue URL without clipping the picker", async ({ testPage, apiClient }) => { + await apiClient.mockGitHubAddBranches("issue-owner", "issue-repo", [{ name: "main" }]); + await apiClient.mockGitHubAddIssues([ + { + number: 1456, + title: "Fix remote repo picker clipping", + body: "The picker overlaps the dialog footer.", + state: "open", + author_login: "mock-user", + repo_owner: "issue-owner", + repo_name: "issue-repo", + html_url: "https://github.com/issue-owner/issue-repo/issues/1456", + }, + ]); + + await openRemotePicker(testPage); + await expectPopoverFitsViewport(testPage); + const pasteInput = testPage.getByTestId("remote-paste-url-input").last(); + await pasteInput.fill("https://github.com/issue-owner/issue-repo/issues/1456"); + await pasteInput.press("Enter"); + + await expect(testPage.getByTestId("task-title-input")).toHaveValue( + "Issue #1456: Fix remote repo picker clipping", + { timeout: 10_000 }, + ); + }); +}); diff --git a/apps/web/e2e/tests/task/task-create-prompt-autocomplete-qa.spec.ts b/apps/web/e2e/tests/task/task-create-prompt-autocomplete-qa.spec.ts index dcc66075e5..0ca30a2c5d 100644 --- a/apps/web/e2e/tests/task/task-create-prompt-autocomplete-qa.spec.ts +++ b/apps/web/e2e/tests/task/task-create-prompt-autocomplete-qa.spec.ts @@ -245,6 +245,8 @@ test.describe("@-mention autocomplete: adversarial QA", () => { await kanban.createTaskButton.first().click(); await expect(testPage.getByTestId("create-task-dialog")).toBeVisible(); + // Use scratch mode so submit does not depend on a pre-selected repository. + await testPage.getByTestId("source-mode-scratch").click(); await testPage.getByTestId("task-title-input").fill("qa-submit-task"); const textarea = testPage.getByTestId("task-description-input"); await textarea.click(); diff --git a/apps/web/e2e/tests/terminal/terminal-agent.spec.ts b/apps/web/e2e/tests/terminal/terminal-agent.spec.ts index 44ad2989dc..a34669e5bd 100644 --- a/apps/web/e2e/tests/terminal/terminal-agent.spec.ts +++ b/apps/web/e2e/tests/terminal/terminal-agent.spec.ts @@ -3,7 +3,7 @@ import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; import { KanbanPage } from "../../pages/kanban-page"; import { SessionPage } from "../../pages/session-page"; -import type { Page } from "@playwright/test"; +import { TimeoutError, type Page } from "@playwright/test"; // --------------------------------------------------------------------------- // Helpers shared across TUI passthrough tests @@ -108,8 +108,13 @@ test.describe("Terminal agent (TUI passthrough)", () => { const session = await openTaskSession(testPage, "TUI Test Task"); - // Loading overlay is visible while the WebSocket connects - await session.waitForPassthroughLoading(); + // The loading overlay is transient; on fast runs it may never be observable. + try { + await session.waitForPassthroughLoading(); + } catch (error) { + // Keep tolerance for "didn't appear in time", but preserve real failures. + if (!(error instanceof TimeoutError)) throw error; + } // Once connected, loading overlay disappears and terminal content is visible await session.waitForPassthroughLoaded(); diff --git a/apps/web/hooks/domains/github/use-branches-by-url.test.ts b/apps/web/hooks/domains/github/use-branches-by-url.test.ts index 3d9b90873b..e1626398a1 100644 --- a/apps/web/hooks/domains/github/use-branches-by-url.test.ts +++ b/apps/web/hooks/domains/github/use-branches-by-url.test.ts @@ -171,6 +171,23 @@ describe("useBranchesByURL — failure & invalidation", () => { expect(fetchRepoBranchesMock).toHaveBeenCalledTimes(1); }); + it("accepts an issue URL and fetches branches against the underlying repo", async () => { + fetchRepoBranchesMock.mockImplementation((owner: string, repo: string) => { + expect(owner).toBe("acme"); + expect(repo).toBe("site"); + return Promise.resolve({ branches: [{ name: "main" }, { name: "fix/issue" }] }); + }); + const { result } = renderHook(() => useBranchesByURL()); + const issueURL = "https://github.com/acme/site/issues/1456"; + + act(() => { + result.current.ensure(issueURL); + }); + + await waitFor(() => expect(result.current.branches(issueURL)).toHaveLength(2)); + expect(fetchRepoBranchesMock).toHaveBeenCalledTimes(1); + }); + it("clear(url) lets the next ensure() refetch a successfully loaded URL", async () => { fetchRepoBranchesMock.mockResolvedValue({ branches: [{ name: "main" }] }); diff --git a/apps/web/hooks/domains/github/use-branches-by-url.ts b/apps/web/hooks/domains/github/use-branches-by-url.ts index 6cbae04480..03543ca733 100644 --- a/apps/web/hooks/domains/github/use-branches-by-url.ts +++ b/apps/web/hooks/domains/github/use-branches-by-url.ts @@ -164,10 +164,9 @@ export function useBranchesByURL(): UseBranchesByURLResult { const url = rawUrl.trim(); if (!url) return; if (inFlightRef.current.has(url) || loadedRef.current.has(url)) return; - // Accept both plain repo URLs (`github.com/owner/repo`) and PR URLs - // (`github.com/owner/repo/pull/123`) — branches are listed against the - // repo in both cases, so we extract just `{ owner, repo }` and ignore - // the PR number. Using the repo-only parser here used to reject PR URLs + // Accept plain repo URLs plus PR/issue URLs — branches are listed against + // the repo in every case, so we extract just `{ owner, repo }` and ignore + // the item number. Using the repo-only parser here used to reject PR URLs // outright, leaving the branch picker permanently empty when the user // pasted a PR link into the Remote tab. const parsed = parseGitHubAnyUrl(url); diff --git a/apps/web/hooks/domains/github/use-pr-info-by-url.test.ts b/apps/web/hooks/domains/github/use-pr-info-by-url.test.ts index d6ce79bdd8..3b4bce3ecf 100644 --- a/apps/web/hooks/domains/github/use-pr-info-by-url.test.ts +++ b/apps/web/hooks/domains/github/use-pr-info-by-url.test.ts @@ -2,22 +2,26 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; const fetchPRInfoMock = vi.fn(); +const fetchIssueInfoMock = vi.fn(); vi.mock("@/lib/api/domains/github-api", () => ({ fetchPRInfo: (...args: unknown[]) => fetchPRInfoMock(...args), + fetchIssueInfo: (...args: unknown[]) => fetchIssueInfoMock(...args), })); // Import after mocks so the hook picks up the mocked module. -import { usePRInfoByURL, parseGitHubPrUrl } from "./use-pr-info-by-url"; +import { usePRInfoByURL, parseGitHubIssueUrl, parseGitHubPrUrl } from "./use-pr-info-by-url"; afterEach(() => { cleanup(); fetchPRInfoMock.mockReset(); + fetchIssueInfoMock.mockReset(); vi.useRealTimers(); }); const PR_URL_A = "https://github.com/acme/site/pull/42"; const PR_URL_B = "https://github.com/acme/api/pull/7"; +const ISSUE_URL_A = "https://github.com/acme/site/issues/1456"; const REPO_URL = "https://github.com/acme/site"; function makePR(overrides: { number: number; title?: string; head?: string; base?: string }) { @@ -37,6 +41,25 @@ function makePR(overrides: { number: number; title?: string; head?: string; base }; } +function makeIssue(overrides: { number: number; title?: string; body?: string }) { + return { + number: overrides.number, + title: overrides.title ?? "Test issue", + body: overrides.body ?? "Issue body", + url: "", + html_url: "", + state: "open" as const, + author_login: "octocat", + repo_owner: "acme", + repo_name: "site", + labels: [], + assignees: [], + created_at: "", + updated_at: "", + closed_at: null, + }; +} + describe("parseGitHubPrUrl", () => { it("returns owner/repo/prNumber for a canonical PR URL", () => { expect(parseGitHubPrUrl(PR_URL_A)).toEqual({ @@ -59,6 +82,31 @@ describe("parseGitHubPrUrl", () => { }); }); +describe("parseGitHubIssueUrl", () => { + it("returns owner/repo/issueNumber for a canonical issue URL", () => { + expect(parseGitHubIssueUrl(ISSUE_URL_A)).toEqual({ + owner: "acme", + repo: "site", + issueNumber: 1456, + }); + }); + + it("tolerates trailing path/hash", () => { + expect(parseGitHubIssueUrl(`${ISSUE_URL_A}#issuecomment-1`)).toEqual({ + owner: "acme", + repo: "site", + issueNumber: 1456, + }); + }); + + it("returns null for PR URLs, plain repo URLs, invalid input, and empty input", () => { + expect(parseGitHubIssueUrl(PR_URL_A)).toBeNull(); + expect(parseGitHubIssueUrl(REPO_URL)).toBeNull(); + expect(parseGitHubIssueUrl("not a url")).toBeNull(); + expect(parseGitHubIssueUrl("")).toBeNull(); + }); +}); + describe("usePRInfoByURL", () => { it("fetches PR info once per unique PR URL when ensure() is called", async () => { fetchPRInfoMock.mockImplementation((_o: string, _r: string, n: number) => { @@ -129,7 +177,9 @@ describe("usePRInfoByURL", () => { await waitFor(() => expect(result.current.loading(PR_URL_A)).toBe(false)); expect(result.current.info(PR_URL_A)).toBeDefined(); }); +}); +describe("usePRInfoByURL — non-PR and issue URLs", () => { it("no-ops (no fetch, no cached info) for a non-PR repo URL", async () => { const { result } = renderHook(() => usePRInfoByURL()); @@ -149,6 +199,40 @@ describe("usePRInfoByURL", () => { expect(fetchPRInfoMock).not.toHaveBeenCalled(); }); + it("clear() is a no-op after ensure() records a non-GitHub URL as loaded", () => { + const url = "https://example.com/not-github"; + const { result } = renderHook(() => usePRInfoByURL()); + + act(() => { + result.current.ensure(url); + result.current.clear(url); + }); + + expect(fetchPRInfoMock).not.toHaveBeenCalled(); + expect(fetchIssueInfoMock).not.toHaveBeenCalled(); + expect(result.current.info(url)).toBeUndefined(); + expect(result.current.loading(url)).toBe(false); + }); + + it("fetches issue info and exposes a suggested title for GitHub issue URLs", async () => { + fetchIssueInfoMock.mockResolvedValue(makeIssue({ number: 1456, title: "Fix remote picker" })); + + const { result } = renderHook(() => usePRInfoByURL()); + + act(() => { + result.current.ensure(ISSUE_URL_A); + }); + + await waitFor(() => expect(result.current.info(ISSUE_URL_A)).toBeDefined()); + expect(fetchIssueInfoMock).toHaveBeenCalledTimes(1); + expect(fetchIssueInfoMock).toHaveBeenCalledWith("acme", "site", 1456, expect.any(Object)); + expect(fetchPRInfoMock).not.toHaveBeenCalled(); + expect(result.current.info(ISSUE_URL_A)).toMatchObject({ + issueNumber: 1456, + suggestedTitle: "Issue #1456: Fix remote picker", + }); + }); + it("ignores ensure() with empty string", () => { const { result } = renderHook(() => usePRInfoByURL()); act(() => { diff --git a/apps/web/hooks/domains/github/use-pr-info-by-url.ts b/apps/web/hooks/domains/github/use-pr-info-by-url.ts index fb582e1349..beae798148 100644 --- a/apps/web/hooks/domains/github/use-pr-info-by-url.ts +++ b/apps/web/hooks/domains/github/use-pr-info-by-url.ts @@ -1,7 +1,7 @@ "use client"; import { useCallback, useEffect, useRef, useState } from "react"; -import { fetchPRInfo } from "@/lib/api/domains/github-api"; +import { fetchIssueInfo, fetchPRInfo } from "@/lib/api/domains/github-api"; import { parseGitHubRepoUrl } from "@/lib/github/parse-url"; /** @@ -27,9 +27,10 @@ import { parseGitHubRepoUrl } from "@/lib/github/parse-url"; */ export type PRInfo = { - prHeadBranch: string; - prBaseBranch: string; - prNumber: number; + prHeadBranch?: string; + prBaseBranch?: string; + prNumber?: number; + issueNumber?: number; suggestedTitle: string; }; @@ -60,14 +61,30 @@ export function parseGitHubPrUrl( return { owner: prMatch[1], repo: prMatch[2], prNumber: parseInt(prMatch[3], 10) }; } -/** Parse a GitHub URL as either a PR URL or a plain repo URL. Re-exported +/** Parse a GitHub URL and return the owner/repo/issueNumber when it's an + * issue URL. Returns null for PR URLs, plain repos, invalid input. */ +export function parseGitHubIssueUrl( + url: string, +): { owner: string; repo: string; issueNumber: number } | null { + const trimmed = url.trim(); + if (!trimmed) return null; + const issueMatch = trimmed.match( + /^(?:https?:\/\/)?(?:www\.)?github\.com\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/issues\/(\d+)(?:[/?#].*)?$/, + ); + if (!issueMatch) return null; + return { owner: issueMatch[1], repo: issueMatch[2], issueNumber: parseInt(issueMatch[3], 10) }; +} + +/** Parse a GitHub URL as a PR URL, issue URL, or plain repo URL. Re-exported * so the legacy `parseGitHubUrl` shape (used elsewhere in the dialog code) * has a single canonical implementation. */ export function parseGitHubAnyUrl( url: string, -): { owner: string; repo: string; prNumber?: number } | null { +): { owner: string; repo: string; prNumber?: number; issueNumber?: number } | null { const pr = parseGitHubPrUrl(url); if (pr) return pr; + const issue = parseGitHubIssueUrl(url); + if (issue) return issue; return parseGitHubRepoUrl(url); } @@ -84,6 +101,15 @@ type Refs = { type SetState = React.Dispatch>>; +type SuccessArgs = { + refs: Refs; + setState: SetState; + url: string; + seq: number; + value: T; + buildInfo: (value: T) => PRInfo; +}; + /** Marks the entry as loading=true (preserving any prior info) and bumps the * per-URL sequence counter. */ function initRequest( @@ -103,24 +129,13 @@ function initRequest( return { seq, signal: controller.signal }; } -/** Writes the successful PR info when the request is still current. */ -function handleSuccess( - refs: Refs, - setState: SetState, - url: string, - seq: number, - pr: Awaited>, -): void { +/** Writes successful GitHub URL info when the request is still current. */ +function handleSuccess(args: SuccessArgs): void { + const { refs, setState, url, seq, value, buildInfo } = args; if (!refs.mountedRef.current) return; if (refs.seqRef.current.get(url) !== seq) return; refs.loadedRef.current.add(url); - const info: PRInfo = { - prHeadBranch: pr.head_branch, - prBaseBranch: pr.base_branch, - prNumber: pr.number, - suggestedTitle: `PR #${pr.number}: ${pr.title}`, - }; - setState((prev) => ({ ...prev, [url]: { info, loading: false } })); + setState((prev) => ({ ...prev, [url]: { info: buildInfo(value), loading: false } })); } /** Marks loaded on failure (we don't want to retry in a tight loop) and @@ -142,6 +157,57 @@ function finalizeRequest(refs: Refs, url: string, seq: number): void { refs.abortersRef.current.delete(url); } +function runGitHubInfoRequest(args: { + refs: Refs; + setState: SetState; + url: string; + seq: number; + signal: AbortSignal; + pr: NonNullable> | null; + issue: ReturnType; +}): void { + const { refs, setState, url, seq, signal, pr, issue } = args; + let request: Promise; + if (pr) { + request = fetchPRInfo(pr.owner, pr.repo, pr.prNumber, { init: { signal } }).then((res) => + handleSuccess({ + refs, + setState, + url, + seq, + value: res, + buildInfo: (value) => ({ + prHeadBranch: value.head_branch, + prBaseBranch: value.base_branch, + prNumber: value.number, + suggestedTitle: `PR #${value.number}: ${value.title}`, + }), + }), + ); + } else if (issue) { + request = fetchIssueInfo(issue.owner, issue.repo, issue.issueNumber, { + init: { signal }, + }).then((res) => + handleSuccess({ + refs, + setState, + url, + seq, + value: res, + buildInfo: (value) => ({ + issueNumber: value.number, + suggestedTitle: `Issue #${value.number}: ${value.title}`, + }), + }), + ); + } else { + return; + } + request + .catch(() => handleFailure(refs, setState, url, seq)) + .finally(() => finalizeRequest(refs, url, seq)); +} + export function usePRInfoByURL(): UsePRInfoByURLResult { const [state, setState] = useState>({}); const inFlightRef = useRef>(new Set()); @@ -184,8 +250,9 @@ export function usePRInfoByURL(): UsePRInfoByURLResult { const url = rawUrl.trim(); if (!url) return; if (inFlightRef.current.has(url) || loadedRef.current.has(url)) return; - const parsed = parseGitHubPrUrl(url); - if (!parsed) { + const pr = parseGitHubPrUrl(url); + const issue = pr ? null : parseGitHubIssueUrl(url); + if (!pr && !issue) { // Non-PR URLs (plain repo, invalid) are recorded as "loaded with no // info" so subsequent ensure() calls for the same URL no-op instead // of re-parsing on every call. @@ -194,10 +261,7 @@ export function usePRInfoByURL(): UsePRInfoByURLResult { } const refs = refsRef.current; const { seq, signal } = initRequest(refs, setState, url); - fetchPRInfo(parsed.owner, parsed.repo, parsed.prNumber, { init: { signal } }) - .then((pr) => handleSuccess(refs, setState, url, seq, pr)) - .catch(() => handleFailure(refs, setState, url, seq)) - .finally(() => finalizeRequest(refs, url, seq)); + runGitHubInfoRequest({ refs, setState, url, seq, signal, pr, issue }); }, []); const info = useCallback( diff --git a/apps/web/lib/api/domains/github-api.test.ts b/apps/web/lib/api/domains/github-api.test.ts index 8e0f43c36b..764157b8aa 100644 --- a/apps/web/lib/api/domains/github-api.test.ts +++ b/apps/web/lib/api/domains/github-api.test.ts @@ -6,6 +6,7 @@ vi.mock("@/lib/config", () => ({ import { fetchAccessibleRepos, + fetchIssueInfo, getTaskCIAutomationOptions, GitHubUnavailableError, updateTaskCIAutomationOptions, @@ -113,6 +114,20 @@ describe("fetchAccessibleRepos — URL & parsing", () => { }); }); +describe("fetchIssueInfo", () => { + it("builds the encoded issue info endpoint and forwards options", async () => { + fetchSpy.mockResolvedValueOnce(jsonResponse({ number: 1456, title: "Fix picker" })); + + await fetchIssueInfo("acme org", "site/repo", 1456, { cache: "no-store" }); + + const call = fetchSpy.mock.calls.at(-1); + expect(String(call?.[0])).toBe( + "http://api.test/api/v1/github/issues/acme%20org/site%2Frepo/1456/info", + ); + expect(call?.[1]).toMatchObject({ cache: "no-store" }); + }); +}); + describe("fetchAccessibleRepos — errors & signal", () => { it("throws GitHubUnavailableError on 503 with code: github_not_configured", async () => { fetchSpy.mockResolvedValueOnce( diff --git a/apps/web/lib/api/domains/github-api.ts b/apps/web/lib/api/domains/github-api.ts index 9f0438145f..1d1ff92195 100644 --- a/apps/web/lib/api/domains/github-api.ts +++ b/apps/web/lib/api/domains/github-api.ts @@ -19,6 +19,7 @@ import type { CreateIssueWatchRequest, UpdateIssueWatchRequest, TriggerIssueResponse, + GitHubIssue, SearchPRsResponse, SearchIssuesResponse, GitHubPRStatus, @@ -397,6 +398,19 @@ export async function fetchPRInfo( ); } +// Issue info (lightweight) +export async function fetchIssueInfo( + owner: string, + repo: string, + number: number, + options?: ApiRequestOptions, +) { + return fetchJson( + `/api/v1/github/issues/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/info`, + options, + ); +} + // Remote repo branches export async function fetchRepoBranches(owner: string, repo: string, options?: ApiRequestOptions) { return fetchJson<{ branches: { name: string }[] }>(