From 84173d3820c348037ed4d742211dffe6eb5d40f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 5 Mar 2026 23:17:10 +0000 Subject: [PATCH] feat(github): support PR URLs in task creation dialog (#379) --- .agents/skills/pr-comments/SKILL.md | 2 +- .../internal/agent/handlers/git_handlers.go | 12 ++ .../agent/handlers/git_handlers_test.go | 80 ++++++++++ apps/backend/internal/github/controller.go | 30 ++++ .../internal/github/controller_test.go | 146 ++++++++++++++++++ apps/backend/internal/github/service.go | 10 +- apps/backend/internal/github/service_test.go | 24 +++ .../components/task-create-dialog-effects.ts | 52 +++++-- .../components/task-create-dialog-state.ts | 8 + .../components/task-create-dialog-types.ts | 2 + apps/web/lib/api/domains/github-api.ts | 14 ++ 11 files changed, 368 insertions(+), 12 deletions(-) create mode 100644 apps/backend/internal/github/controller_test.go diff --git a/.agents/skills/pr-comments/SKILL.md b/.agents/skills/pr-comments/SKILL.md index 3f62a0060e..c3c9702726 100644 --- a/.agents/skills/pr-comments/SKILL.md +++ b/.agents/skills/pr-comments/SKILL.md @@ -10,7 +10,7 @@ Read and address review comments on a pull request, then push fixes. ## Context - Current branch: !`git branch --show-current` -- Current PR (if any): !`gh pr view --json number,url,title --jq '"\(.url) - \(.title)"' 2>/dev/null || echo "no PR found"` +- Current PR (if any): !`gh pr view --json number,url,title` ## Steps diff --git a/apps/backend/internal/agent/handlers/git_handlers.go b/apps/backend/internal/agent/handlers/git_handlers.go index a39021a06d..d54fe81995 100644 --- a/apps/backend/internal/agent/handlers/git_handlers.go +++ b/apps/backend/internal/agent/handlers/git_handlers.go @@ -647,6 +647,12 @@ func (h *GitHandlers) wsGitCommits(ctx context.Context, msg *ws.Message) (*ws.Me agentClient, err := h.getAgentCtlClient(req.SessionID) if err != nil { + if isSessionNotReadyError(err) { + return ws.NewResponse(msg.ID, msg.Action, map[string]any{ + "commits": []any{}, + "ready": false, + }) + } return nil, err } @@ -696,6 +702,12 @@ func (h *GitHandlers) wsCumulativeDiff(ctx context.Context, msg *ws.Message) (*w agentClient, err := h.getAgentCtlClient(req.SessionID) if err != nil { + if isSessionNotReadyError(err) { + return ws.NewResponse(msg.ID, msg.Action, map[string]any{ + "cumulative_diff": nil, + "ready": false, + }) + } return nil, err } diff --git a/apps/backend/internal/agent/handlers/git_handlers_test.go b/apps/backend/internal/agent/handlers/git_handlers_test.go index 54879dfc23..73fbb2dda6 100644 --- a/apps/backend/internal/agent/handlers/git_handlers_test.go +++ b/apps/backend/internal/agent/handlers/git_handlers_test.go @@ -3,6 +3,7 @@ package handlers import ( "context" "encoding/json" + "fmt" "sync" "testing" "time" @@ -331,3 +332,82 @@ func TestWsPush_NoExecution(t *testing.T) { t.Error("expected error when no execution found") } } + +func TestWsGitCommits_NotReady(t *testing.T) { + log := newTestLogger() + lookup := &mockExecutionLookup{} // no executions → "no agent running" error + h := NewGitHandlers(lookup, nil, log) + + msg, _ := ws.NewRequest("test-1", ws.ActionSessionGitCommits, GitCommitsRequest{SessionID: "session-1"}) + + resp, err := h.wsGitCommits(context.Background(), msg) + if err != nil { + t.Fatalf("expected graceful response, got error: %v", err) + } + if resp == nil { + t.Fatal("expected non-nil response") + } + + var payload map[string]any + if err := json.Unmarshal(resp.Payload, &payload); err != nil { + t.Fatalf("failed to parse payload: %v", err) + } + if ready, ok := payload["ready"].(bool); !ok || ready { + t.Errorf("expected ready=false, got %v", payload["ready"]) + } + commits, ok := payload["commits"].([]any) + if !ok { + t.Fatalf("expected commits array, got %T", payload["commits"]) + } + if len(commits) != 0 { + t.Errorf("expected empty commits, got %d", len(commits)) + } +} + +func TestWsCumulativeDiff_NotReady(t *testing.T) { + log := newTestLogger() + lookup := &mockExecutionLookup{} // no executions → "no agent running" error + h := NewGitHandlers(lookup, nil, log) + + msg, _ := ws.NewRequest("test-1", ws.ActionSessionCumulativeDiff, CumulativeDiffRequest{SessionID: "session-1"}) + + resp, err := h.wsCumulativeDiff(context.Background(), msg) + if err != nil { + t.Fatalf("expected graceful response, got error: %v", err) + } + if resp == nil { + t.Fatal("expected non-nil response") + } + + var payload map[string]any + if err := json.Unmarshal(resp.Payload, &payload); err != nil { + t.Fatalf("failed to parse payload: %v", err) + } + if ready, ok := payload["ready"].(bool); !ok || ready { + t.Errorf("expected ready=false, got %v", payload["ready"]) + } + if payload["cumulative_diff"] != nil { + t.Errorf("expected cumulative_diff=nil, got %v", payload["cumulative_diff"]) + } +} + +func TestIsSessionNotReadyError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"nil error", nil, false}, + {"no agent running", fmt.Errorf("no agent running for session abc"), true}, + {"agent client not available", fmt.Errorf("agent client not available for session abc"), true}, + {"unrelated error", fmt.Errorf("some other error"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isSessionNotReadyError(tt.err); got != tt.want { + t.Errorf("isSessionNotReadyError() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index 5f40621798..fed793ea45 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -33,6 +33,7 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) { api.GET("/task-prs/:taskId", c.httpGetTaskPR) api.GET("/prs/:owner/:repo/:number", c.httpGetPRFeedback) + api.GET("/prs/:owner/:repo/:number/info", c.httpGetPRInfo) api.POST("/prs/:owner/:repo/:number/reviews", c.httpSubmitReview) api.GET("/watches/pr", c.httpListPRWatches) @@ -107,6 +108,35 @@ func (c *Controller) httpGetPRFeedback(ctx *gin.Context) { ctx.JSON(http.StatusOK, feedback) } +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"}) + 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()}) + return + } + ctx.JSON(http.StatusOK, pr) +} + 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 new file mode 100644 index 0000000000..f5c5cb9faf --- /dev/null +++ b/apps/backend/internal/github/controller_test.go @@ -0,0 +1,146 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gin-gonic/gin" + + "github.com/kandev/kandev/internal/common/logger" +) + +// 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) +} + +func (s *stubClient) IsAuthenticated(context.Context) (bool, error) { return true, nil } +func (s *stubClient) GetAuthenticatedUser(context.Context) (string, error) { + return "test-user", nil +} +func (s *stubClient) GetPR(ctx context.Context, owner, repo string, number int) (*PR, error) { + if s.getPRFunc != nil { + return s.getPRFunc(ctx, owner, repo, number) + } + return nil, fmt.Errorf("not implemented") +} +func (s *stubClient) FindPRByBranch(context.Context, string, string, string) (*PR, error) { + return nil, nil +} +func (s *stubClient) ListAuthoredPRs(context.Context, string, string) ([]*PR, error) { + return nil, nil +} +func (s *stubClient) ListReviewRequestedPRs(context.Context, string, string, string) ([]*PR, error) { + return nil, nil +} +func (s *stubClient) ListUserOrgs(context.Context) ([]GitHubOrg, error) { return nil, nil } +func (s *stubClient) SearchOrgRepos(context.Context, string, string, int) ([]GitHubRepo, error) { + return nil, nil +} +func (s *stubClient) ListPRReviews(context.Context, string, string, int) ([]PRReview, error) { + return nil, nil +} +func (s *stubClient) ListPRComments(context.Context, string, string, int, *time.Time) ([]PRComment, error) { + return nil, nil +} +func (s *stubClient) ListCheckRuns(context.Context, string, string, string) ([]CheckRun, error) { + return nil, nil +} +func (s *stubClient) GetPRFeedback(context.Context, string, string, int) (*PRFeedback, error) { + return nil, nil +} +func (s *stubClient) ListPRFiles(context.Context, string, string, int) ([]PRFile, error) { + return nil, nil +} +func (s *stubClient) ListPRCommits(context.Context, string, string, int) ([]PRCommitInfo, error) { + return nil, nil +} +func (s *stubClient) SubmitReview(context.Context, string, string, int, string, string) error { + return nil +} +func (s *stubClient) ListRepoBranches(context.Context, string, string) ([]RepoBranch, error) { + return nil, nil +} + +func newControllerTestLogger() *logger.Logger { + log, _ := logger.NewLogger(logger.LoggingConfig{ + Level: "error", + Format: "json", + }) + return log +} + +func setupControllerTest(client Client) (*gin.Engine, *Controller) { + gin.SetMode(gin.TestMode) + log := newControllerTestLogger() + svc := NewService(client, "pat", nil, nil, nil, log) + ctrl := NewController(svc, log) + router := gin.New() + ctrl.RegisterHTTPRoutes(router) + return router, ctrl +} + +func TestHttpGetPRInfo_Success(t *testing.T) { + sc := &stubClient{ + getPRFunc: func(_ context.Context, owner, repo string, number int) (*PR, error) { + if owner != "acme" || repo != "widget" || number != 42 { + t.Errorf("unexpected params: %s/%s#%d", owner, repo, number) + } + return &PR{Number: 42, Title: "feat: add widget"}, nil + }, + } + router, _ := setupControllerTest(sc) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/prs/acme/widget/42/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 pr PR + if err := json.NewDecoder(w.Body).Decode(&pr); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if pr.Number != 42 { + t.Errorf("expected PR number 42, got %d", pr.Number) + } + if pr.Title != "feat: add widget" { + t.Errorf("expected title 'feat: add widget', got %q", pr.Title) + } +} + +func TestHttpGetPRInfo_InvalidNumber(t *testing.T) { + router, _ := setupControllerTest(&stubClient{}) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/prs/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 TestHttpGetPRInfo_ServiceError(t *testing.T) { + sc := &stubClient{ + getPRFunc: func(context.Context, string, string, int) (*PR, error) { + return nil, fmt.Errorf("not found") + }, + } + 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.StatusInternalServerError { + t.Fatalf("expected 500, got %d", w.Code) + } +} diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index e7e03d89cd..f2b7bd859b 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -399,7 +399,15 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, feedback *PRFee return nil } -// --- PR feedback (live) --- +// --- PR info and feedback (live) --- + +// 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 s.client.GetPR(ctx, owner, repo, number) +} // GetPRFeedback fetches live PR feedback from GitHub. func (s *Service) GetPRFeedback(ctx context.Context, owner, repo string, number int) (*PRFeedback, error) { diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index 4e1d2d235a..654ad182be 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -1,10 +1,34 @@ package github import ( + "context" + "strings" "testing" "time" ) +func TestGetPR_NilClient(t *testing.T) { + svc := &Service{client: nil} + _, err := svc.GetPR(context.Background(), "owner", "repo", 1) + 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) + } +} + +func TestGetPRFeedback_NilClient(t *testing.T) { + svc := &Service{client: nil} + _, err := svc.GetPRFeedback(context.Background(), "owner", "repo", 1) + 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) + } +} + func TestParsePRURL(t *testing.T) { tests := []struct { name string diff --git a/apps/web/components/task-create-dialog-effects.ts b/apps/web/components/task-create-dialog-effects.ts index d89d65e247..6cb2b80e1b 100644 --- a/apps/web/components/task-create-dialog-effects.ts +++ b/apps/web/components/task-create-dialog-effects.ts @@ -12,7 +12,7 @@ import { import { getLocalStorage } from "@/lib/local-storage"; import { STORAGE_KEYS } from "@/lib/settings/constants"; import { listWorkflowSteps } from "@/lib/api/domains/workflow-api"; -import { fetchRepoBranches } from "@/lib/api/domains/github-api"; +import { fetchRepoBranches, fetchPRInfo } from "@/lib/api/domains/github-api"; import type { DialogFormState, StoreSelections, @@ -222,8 +222,17 @@ export function useBranchAutoSelectEffect(fs: DialogFormState, branches: Branch[ if (repositoryId || localBranches.length === 0 || branch) return; autoSelectBranch(localBranches, setBranch); }, [branch, localBranches, repositoryId, setBranch]); + const { githubPrHeadBranch } = fs; useEffect(() => { if (!useGitHubUrl || githubBranches.length === 0 || branch) return; + // If a PR URL was detected, prefer the PR's head branch. + if (githubPrHeadBranch) { + const prBranch = githubBranches.find((b) => b.name === githubPrHeadBranch); + if (prBranch) { + setBranch(prBranch.name); + return; + } + } // GitHub URL branches are referenced by name only (no remote prefix). // selectPreferredBranch expects origin-prefixed remotes, so pick directly. const preferred = @@ -231,14 +240,21 @@ export function useBranchAutoSelectEffect(fs: DialogFormState, branches: Branch[ githubBranches.find((b) => b.name === "master") ?? githubBranches[0]; if (preferred) setBranch(preferred.name); - }, [branch, githubBranches, useGitHubUrl, setBranch]); + }, [branch, githubBranches, useGitHubUrl, setBranch, githubPrHeadBranch]); } -/** Parse a GitHub URL to extract owner and repo name. Returns null if invalid. */ -function parseGitHubUrl(url: string): { owner: string; repo: string } | null { +/** Parse a GitHub URL to extract owner, repo, and optional PR number. Returns null if invalid. */ +function parseGitHubUrl(url: string): { owner: string; repo: string; prNumber?: number } | null { const trimmed = url.trim(); if (!trimmed) return null; - // Match github.com/owner/repo with optional scheme, .git suffix, trailing slash + // Try PR URL first: github.com/owner/repo/pull/123 (with optional trailing path/hash like /files#diff-...) + const prMatch = trimmed.match( + /(?:https?:\/\/)?(?:www\.)?github\.com\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/pull\/(\d+)(?:[/?#].*)?$/, + ); + if (prMatch) { + return { owner: prMatch[1], repo: prMatch[2], prNumber: parseInt(prMatch[3], 10) }; + } + // Fall back to repo URL: github.com/owner/repo const match = trimmed.match( /(?:https?:\/\/)?(?:www\.)?github\.com\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+?)(?:\.git)?\/?$/, ); @@ -253,6 +269,7 @@ export function useGitHubUrlBranchesEffect(fs: DialogFormState, open: boolean) { setGitHubBranches, setGitHubBranchesLoading, setGitHubUrlError, + setGitHubPrHeadBranch, } = fs; useEffect(() => { if (!open || !useGitHubUrl) { @@ -269,21 +286,35 @@ export function useGitHubUrlBranchesEffect(fs: DialogFormState, open: boolean) { const parsed = parseGitHubUrl(githubUrl); if (!parsed) { setGitHubBranches([]); - setGitHubUrlError("Invalid GitHub URL — expected github.com/owner/repo"); + setGitHubPrHeadBranch(null); + setGitHubBranchesLoading(false); + setGitHubUrlError("Invalid GitHub URL — expected github.com/owner/repo or .../pull/123"); return; } let cancelled = false; setGitHubUrlError(null); setGitHubBranchesLoading(true); - fetchRepoBranches(parsed.owner, parsed.repo) - .then((res) => { + setGitHubPrHeadBranch(null); + + const branchesPromise = fetchRepoBranches(parsed.owner, parsed.repo); + const prPromise = parsed.prNumber + ? fetchPRInfo(parsed.owner, parsed.repo, parsed.prNumber).catch(() => null) + : Promise.resolve(null); + + Promise.all([branchesPromise, prPromise]) + .then(([branchesRes, prInfo]) => { if (cancelled) return; - setGitHubBranches(res.branches.map((b) => ({ name: b.name, type: "remote" as const }))); + setGitHubBranches( + branchesRes.branches.map((b) => ({ name: b.name, type: "remote" as const })), + ); setGitHubUrlError(null); + if (prInfo) { + setGitHubPrHeadBranch(prInfo.head_branch); + } }) .catch(() => { if (cancelled) return; - setGitHubUrlError(`Repository not found or not accessible`); + setGitHubUrlError("Repository not found or not accessible"); setGitHubBranches([]); }) .finally(() => { @@ -299,6 +330,7 @@ export function useGitHubUrlBranchesEffect(fs: DialogFormState, open: boolean) { setGitHubBranches, setGitHubBranchesLoading, setGitHubUrlError, + setGitHubPrHeadBranch, ]); } diff --git a/apps/web/components/task-create-dialog-state.ts b/apps/web/components/task-create-dialog-state.ts index db68be4794..1165a9d633 100644 --- a/apps/web/components/task-create-dialog-state.ts +++ b/apps/web/components/task-create-dialog-state.ts @@ -59,6 +59,7 @@ type FormResetters = { setGitHubUrl: (v: string) => void; setGitHubBranches: (v: Branch[]) => void; setGitHubUrlError: (v: string | null) => void; + setGitHubPrHeadBranch: (v: string | null) => void; }; function useFormResetEffects( @@ -104,6 +105,7 @@ function useFormResetEffects( resetters.setGitHubUrl(""); resetters.setGitHubBranches([]); resetters.setGitHubUrlError(null); + resetters.setGitHubPrHeadBranch(null); }); // eslint-disable-next-line react-hooks/exhaustive-deps }, [open, workspaceId]); @@ -115,6 +117,7 @@ function useGitHubUrlState() { const [githubBranches, setGitHubBranches] = useState([]); const [githubBranchesLoading, setGitHubBranchesLoading] = useState(false); const [githubUrlError, setGitHubUrlError] = useState(null); + const [githubPrHeadBranch, setGitHubPrHeadBranch] = useState(null); return { useGitHubUrl, setUseGitHubUrl, @@ -126,6 +129,8 @@ function useGitHubUrlState() { setGitHubBranchesLoading, githubUrlError, setGitHubUrlError, + githubPrHeadBranch, + setGitHubPrHeadBranch, }; } @@ -176,6 +181,7 @@ export function useDialogFormState( setGitHubUrl: ghUrl.setGitHubUrl, setGitHubBranches: ghUrl.setGitHubBranches, setGitHubUrlError: ghUrl.setGitHubUrlError, + setGitHubPrHeadBranch: ghUrl.setGitHubPrHeadBranch, }); return { taskName, @@ -307,6 +313,7 @@ export function useDialogHandlers(fs: DialogFormState, repositories: Repository[ fs.setGitHubUrl(""); fs.setGitHubBranches([]); fs.setGitHubUrlError(null); + fs.setGitHubPrHeadBranch(null); } fs.setBranch(""); }, [fs]); @@ -317,6 +324,7 @@ export function useDialogHandlers(fs: DialogFormState, repositories: Repository[ fs.setBranch(""); fs.setGitHubBranches([]); fs.setGitHubUrlError(null); + fs.setGitHubPrHeadBranch(null); }, [fs], ); diff --git a/apps/web/components/task-create-dialog-types.ts b/apps/web/components/task-create-dialog-types.ts index caebb8f564..4c14d1bf3a 100644 --- a/apps/web/components/task-create-dialog-types.ts +++ b/apps/web/components/task-create-dialog-types.ts @@ -130,4 +130,6 @@ export type DialogFormState = { setGitHubBranchesLoading: (v: boolean) => void; githubUrlError: string | null; setGitHubUrlError: (v: string | null) => void; + githubPrHeadBranch: string | null; + setGitHubPrHeadBranch: (v: string | null) => void; }; diff --git a/apps/web/lib/api/domains/github-api.ts b/apps/web/lib/api/domains/github-api.ts index 9e046f96a2..2b7afb8ab8 100644 --- a/apps/web/lib/api/domains/github-api.ts +++ b/apps/web/lib/api/domains/github-api.ts @@ -3,6 +3,7 @@ import type { GitHubStatusResponse, GitHubOrg, GitHubRepoInfo, + GitHubPR, TaskPRsResponse, TaskPR, PRFeedback, @@ -141,6 +142,19 @@ export async function searchOrgRepos(org: string, query?: string, options?: ApiR ); } +// PR info (lightweight) +export async function fetchPRInfo( + owner: string, + repo: string, + number: number, + options?: ApiRequestOptions, +) { + return fetchJson( + `/api/v1/github/prs/${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 }[] }>(