diff --git a/apps/backend/internal/github/client.go b/apps/backend/internal/github/client.go index bd0e554e71..1cd7e73ff7 100644 --- a/apps/backend/internal/github/client.go +++ b/apps/backend/internal/github/client.go @@ -73,6 +73,9 @@ type Client interface { // ListPRCommits lists commits in a pull request. ListPRCommits(ctx context.Context, owner, repo string, number int) ([]PRCommitInfo, error) + // GetPRCommitDetail retrieves one commit and all of its changed files. + GetPRCommitDetail(ctx context.Context, owner, repo, sha string) (PRCommitDetail, error) + // SubmitReview submits a review on a pull request. // event is one of "APPROVE", "COMMENT", "REQUEST_CHANGES". SubmitReview(ctx context.Context, owner, repo string, number int, event, body string) error diff --git a/apps/backend/internal/github/client_helpers.go b/apps/backend/internal/github/client_helpers.go index 724b05caf3..1893e3d01e 100644 --- a/apps/backend/internal/github/client_helpers.go +++ b/apps/backend/internal/github/client_helpers.go @@ -3,12 +3,22 @@ package github import ( "context" "encoding/json" + "errors" "fmt" "sort" "strings" "time" + + "github.com/kandev/kandev/internal/common/securityutil" ) +func validateGitHubCommitSHA(sha string) error { + if !securityutil.LooksLikeCommitSHA(sha) { + return fmt.Errorf("invalid commit SHA") + } + return nil +} + // buildReviewSearchQuery assembles the full GitHub search query. // When customQuery is non-empty, it is used verbatim as the entire query. // Otherwise a query is built from scope + optional filter qualifier. @@ -642,6 +652,28 @@ type ghPRCommit struct { } `json:"author"` } +// ghPRCommitDetail is the JSON shape returned by GitHub's individual commit +// endpoint. With gh api --paginate --slurp, the same shape is wrapped in an +// array, one element per page. +type ghPRCommitDetail struct { + SHA string `json:"sha"` + Commit struct { + Message string `json:"message"` + Author struct { + Name string `json:"name"` + Date string `json:"date"` + } `json:"author"` + } `json:"commit"` + Author *struct { + Login string `json:"login"` + } `json:"author"` + Stats struct { + Additions int `json:"additions"` + Deletions int `json:"deletions"` + } `json:"stats"` + Files []ghPRFile `json:"files"` +} + // parsePRFilesJSON parses the JSON response from the PR files API. func parsePRFilesJSON(data string) ([]PRFile, error) { var raw []ghPRFile @@ -667,6 +699,59 @@ func convertRawPRFiles(raw []ghPRFile) []PRFile { return files } +// parsePRCommitDetailJSON parses one or more pages from GitHub's individual +// commit endpoint. Metadata and aggregate stats come from the first page; +// file records are appended in provider order and duplicate filenames are +// ignored when a provider repeats a record across pages. +func parsePRCommitDetailJSON(data string) (PRCommitDetail, error) { + var pages []ghPRCommitDetail + trimmed := strings.TrimSpace(data) + if strings.HasPrefix(trimmed, "[") { + if err := json.Unmarshal([]byte(trimmed), &pages); err != nil { + return PRCommitDetail{}, fmt.Errorf("parse PR commit detail: %w", err) + } + } else { + var page ghPRCommitDetail + if err := json.Unmarshal([]byte(trimmed), &page); err != nil { + return PRCommitDetail{}, fmt.Errorf("parse PR commit detail: %w", err) + } + pages = []ghPRCommitDetail{page} + } + if len(pages) == 0 { + return PRCommitDetail{}, errors.New("parse PR commit detail: empty response") + } + + first := pages[0] + if first.SHA == "" { + return PRCommitDetail{}, errors.New("parse PR commit detail: missing commit SHA") + } + detail := PRCommitDetail{ + SHA: first.SHA, + Message: first.Commit.Message, + AuthorName: first.Commit.Author.Name, + AuthorDate: first.Commit.Author.Date, + Additions: first.Stats.Additions, + Deletions: first.Stats.Deletions, + Files: make([]PRFile, 0), + } + if first.Author != nil { + detail.AuthorLogin = first.Author.Login + } + seen := make(map[string]struct{}) + for _, page := range pages { + for _, file := range page.Files { + key := file.Filename + "\x00" + file.PreviousFilename + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + detail.Files = append(detail.Files, convertRawPRFiles([]ghPRFile{file})[0]) + } + } + detail.FilesChanged = len(detail.Files) + return detail, nil +} + // buildFullDiff wraps a GitHub patch fragment into a complete unified diff. func buildFullDiff(filename, status, patch, previousFilename string) string { if patch == "" { @@ -729,10 +814,11 @@ func convertRawPRCommits(raw []ghPRCommit) []PRCommitInfo { msg = msg[:idx] } commits[i] = PRCommitInfo{ - SHA: c.SHA, - Message: msg, - AuthorLogin: author, - AuthorDate: c.Commit.Author.Date, + SHA: c.SHA, + Message: msg, + AuthorLogin: author, + AuthorDate: c.Commit.Author.Date, + StatsAvailable: false, } } return commits diff --git a/apps/backend/internal/github/commit_detail_test.go b/apps/backend/internal/github/commit_detail_test.go new file mode 100644 index 0000000000..426220fda0 --- /dev/null +++ b/apps/backend/internal/github/commit_detail_test.go @@ -0,0 +1,181 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "strings" + "testing" + + ws "github.com/kandev/kandev/pkg/websocket" +) + +func TestPRCommitStatsAreMarkedUnavailable(t *testing.T) { + commits, err := parsePRCommitsJSON(`[ + { + "sha": "1111111111111111111111111111111111111111", + "commit": { + "message": "Remote commit", + "author": {"date": "2026-08-04T10:00:00Z"} + }, + "author": {"login": "octocat"} + } + ]`) + if err != nil { + t.Fatalf("parse PR commits: %v", err) + } + + encoded, err := json.Marshal(commits) + if err != nil { + t.Fatalf("marshal PR commits: %v", err) + } + if !strings.Contains(string(encoded), `"stats_available":false`) { + t.Fatalf("PR commit stats availability = %s, want explicit false", encoded) + } +} + +func TestParsePRCommitDetailJSONMergesPages(t *testing.T) { + detail, err := parsePRCommitDetailJSON(`[ + { + "sha": "2222222222222222222222222222222222222222", + "commit": { + "message": "Remote detail\nbody", + "author": {"name": "Octo Cat", "date": "2026-08-04T11:00:00Z"} + }, + "author": {"login": "octocat"}, + "stats": {"additions": 7, "deletions": 3, "total": 10}, + "files": [ + {"filename": "one.txt", "status": "modified", "additions": 4, "deletions": 1, "patch": "@@ -1 +1 @@\n-old\n+new"}, + {"filename": "duplicate.txt", "status": "added", "additions": 1, "deletions": 0} + ] + }, + { + "sha": "2222222222222222222222222222222222222222", + "commit": {"message": "ignored page metadata", "author": {"name": "Other", "date": "2026-08-05T11:00:00Z"}}, + "author": {"login": "other"}, + "stats": {"additions": 99, "deletions": 99, "total": 198}, + "files": [ + {"filename": "duplicate.txt", "status": "added", "additions": 1, "deletions": 0}, + {"filename": "two.txt", "status": "removed", "additions": 0, "deletions": 2, "patch": "@@ -1 +0,0 @@\n-gone"} + ] + } + ]`) + if err != nil { + t.Fatalf("parse PR commit detail: %v", err) + } + if detail.SHA != "2222222222222222222222222222222222222222" || detail.Message != "Remote detail\nbody" { + t.Fatalf("metadata = %#v", detail) + } + if detail.AuthorLogin != "octocat" || detail.AuthorName != "Octo Cat" || detail.AuthorDate != "2026-08-04T11:00:00Z" { + t.Fatalf("author = %#v", detail) + } + if detail.Additions != 7 || detail.Deletions != 3 || detail.FilesChanged != 3 { + t.Fatalf("stats = %#v", detail) + } + if len(detail.Files) != 3 { + t.Fatalf("files = %#v, want three provider-ordered unique files", detail.Files) + } + if detail.Files[0].Filename != "one.txt" || detail.Files[1].Filename != "duplicate.txt" || detail.Files[2].Filename != "two.txt" { + t.Fatalf("file order = %#v", detail.Files) + } + if !strings.Contains(detail.Files[0].Patch, "diff --git a/one.txt b/one.txt") { + t.Fatalf("file patch = %q, want complete diff", detail.Files[0].Patch) + } +} + +func TestParsePRCommitDetailJSONRejectsEmptyPayloads(t *testing.T) { + for _, payload := range []string{`null`, `[null]`, `{}`, `[{}]`} { + t.Run(payload, func(t *testing.T) { + if _, err := parsePRCommitDetailJSON(payload); err == nil { + t.Fatalf("parsePRCommitDetailJSON(%q) succeeded, want malformed payload error", payload) + } + }) + } +} + +func TestGetPRCommitDetailForWorkspaceUsesAuthorizedClient(t *testing.T) { + const sha = "3333333333333333333333333333333333333333" + client := NewMockClient() + client.AddRepos("test-user", []GitHubRepo{{FullName: "acme/widget", Owner: "acme", Name: "widget"}}) + client.AddPRCommitDetail("acme", "widget", sha, PRCommitDetail{ + SHA: sha, + Files: []PRFile{{Filename: "remote.txt", Status: "modified", Patch: "remote patch"}}, + }) + svc := newWorkspaceAuthenticatedTestService(t, client, nil, "workspace-1") + + detail, err := svc.GetPRCommitDetailForWorkspace(context.Background(), "workspace-1", "", "acme", "widget", sha) + if err != nil { + t.Fatalf("GetPRCommitDetailForWorkspace: %v", err) + } + if detail.SHA != sha || len(detail.Files) != 1 || detail.Files[0].Filename != "remote.txt" { + t.Fatalf("detail = %#v", detail) + } +} + +func TestGetPRCommitDetailForWorkspaceRejectsUnsafeSHA(t *testing.T) { + svc := &Service{workspaceAuthorizer: func(context.Context, string) error { + return errors.New("workspace lookup should not run") + }} + + _, err := svc.GetPRCommitDetailForWorkspace(context.Background(), "workspace-1", "", "acme", "widget", "main") + if err == nil || !strings.Contains(err.Error(), "invalid commit SHA") { + t.Fatalf("error = %v, want invalid commit SHA before provider access", err) + } +} + +func TestWSGetPRCommitDetailRoutesWorkspaceRequest(t *testing.T) { + const sha = "4444444444444444444444444444444444444444" + client := NewMockClient() + client.AddRepos("test-user", []GitHubRepo{{FullName: "acme/widget", Owner: "acme", Name: "widget"}}) + client.AddPRCommitDetail("acme", "widget", sha, PRCommitDetail{SHA: sha, Message: "remote commit"}) + svc := newWorkspaceAuthenticatedTestService(t, client, nil, "workspace-1") + message, err := ws.NewRequest("request-1", "github.pr_commit.get", map[string]any{ + "workspace_id": "workspace-1", "owner": "acme", "repo": "widget", "sha": sha, + }) + if err != nil { + t.Fatalf("create request: %v", err) + } + + response, err := wsGetPRCommitDetail(svc, testLogger(t))(context.Background(), message) + if err != nil { + t.Fatalf("handler error: %v", err) + } + if response.Type != ws.MessageTypeResponse { + t.Fatalf("response type = %q, want response", response.Type) + } + var body struct { + Commit PRCommitDetail `json:"commit"` + } + if err := json.Unmarshal(response.Payload, &body); err != nil { + t.Fatalf("decode response: %v", err) + } + if body.Commit.SHA != sha || body.Commit.Message != "remote commit" { + t.Fatalf("response body = %#v", body) + } +} + +func TestWSGetPRCommitDetailReturnsWorkspaceAuthorizationError(t *testing.T) { + svc := NewService(NewMockClient(), AuthMethodPAT, nil, nil, nil, testLogger(t)) + denied := errors.New("workspace not found") + svc.SetWorkspaceAuthorizer(func(_ context.Context, workspaceID string) error { + if workspaceID != "workspace-1" { + t.Fatalf("workspaceID = %q", workspaceID) + } + return denied + }) + message, err := ws.NewRequest("request-1", "github.pr_commit.get", map[string]any{ + "workspace_id": "workspace-1", "owner": "acme", "repo": "widget", + "sha": "5555555555555555555555555555555555555555", + }) + if err != nil { + t.Fatalf("create request: %v", err) + } + + response, err := wsGetPRCommitDetail(svc, testLogger(t))(context.Background(), message) + if err != nil { + t.Fatalf("handler error: %v", err) + } + if response.Type != ws.MessageTypeError || !strings.Contains(string(response.Payload), denied.Error()) { + t.Fatalf("response = %#v, want workspace authorization error", response) + } +} diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index bad5a72a0f..6ca49a4797 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -117,6 +117,9 @@ func (s *stubClient) ListPRFiles(context.Context, string, string, int) ([]PRFile func (s *stubClient) ListPRCommits(context.Context, string, string, int) ([]PRCommitInfo, error) { return nil, nil } +func (s *stubClient) GetPRCommitDetail(context.Context, string, string, string) (PRCommitDetail, error) { + return PRCommitDetail{}, nil +} func (s *stubClient) SubmitReview(context.Context, string, string, int, string, string) error { return nil } diff --git a/apps/backend/internal/github/gh_client.go b/apps/backend/internal/github/gh_client.go index 779f79d07e..abde40b95e 100644 --- a/apps/backend/internal/github/gh_client.go +++ b/apps/backend/internal/github/gh_client.go @@ -782,6 +782,23 @@ func (c *GHClient) ListPRCommits(ctx context.Context, owner, repo string, number return parsePRCommitsJSON(out) } +func (c *GHClient) GetPRCommitDetail(ctx context.Context, owner, repo, sha string) (PRCommitDetail, error) { + if err := validateGitHubCommitSHA(sha); err != nil { + return PRCommitDetail{}, err + } + out, err := c.run(ctx, "api", + fmt.Sprintf("repos/%s/%s/commits/%s?per_page=100", owner, repo, sha), + "--paginate", "--slurp") + if err != nil { + return PRCommitDetail{}, fmt.Errorf("get PR commit detail: %w", err) + } + detail, err := parsePRCommitDetailJSON(out) + if err != nil { + return PRCommitDetail{}, err + } + return detail, nil +} + func (c *GHClient) SubmitReview(ctx context.Context, owner, repo string, number int, event, body string) error { args := []string{"api", fmt.Sprintf("repos/%s/%s/pulls/%d/reviews", owner, repo, number), diff --git a/apps/backend/internal/github/gh_client_test.go b/apps/backend/internal/github/gh_client_test.go index 73f6fb3496..d678ac7cdd 100644 --- a/apps/backend/internal/github/gh_client_test.go +++ b/apps/backend/internal/github/gh_client_test.go @@ -234,6 +234,53 @@ esac } } +func TestGHClient_PRCommitDetailUsesExactSHAAndMergesPages(t *testing.T) { + binDir := t.TempDir() + logPath := filepath.Join(t.TempDir(), "gh-args.log") + ghPath := filepath.Join(binDir, "gh") + script := `#!/bin/sh +printf '%s\n' "$*" >> "$GH_ARGS_LOG" +case "$*" in + *commits/*) + printf '%s\n' '[{"sha":"2222222222222222222222222222222222222222","commit":{"message":"remote detail","author":{"name":"Octo Cat","date":"2026-08-04T11:00:00Z"}},"author":{"login":"octocat"},"stats":{"additions":7,"deletions":3},"files":[{"filename":"one.txt","status":"modified","additions":4,"deletions":1,"patch":"@@ -1 +1 @@\\n-old\\n+new"}]},{"sha":"2222222222222222222222222222222222222222","commit":{"message":"ignored","author":{"name":"Other","date":"2026-08-05T11:00:00Z"}},"stats":{"additions":99,"deletions":99},"files":[{"filename":"two.txt","status":"removed","additions":0,"deletions":2,"patch":"@@ -1 +0,0 @@\\n-gone"}]}]' + ;; + *) printf '%s\n' '[]' ;; +esac +` + if err := os.WriteFile(ghPath, []byte(script), 0o755); err != nil { + t.Fatalf("write fake gh: %v", err) + } + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("GH_ARGS_LOG", logPath) + + detail, err := NewGHClient().GetPRCommitDetail( + context.Background(), "acme", "widget", "2222222222222222222222222222222222222222", + ) + if err != nil { + t.Fatalf("GetPRCommitDetail: %v", err) + } + if detail.SHA != "2222222222222222222222222222222222222222" || detail.Additions != 7 || detail.Deletions != 3 { + t.Fatalf("detail = %#v", detail) + } + if len(detail.Files) != 2 { + t.Fatalf("files = %#v, want two merged files", detail.Files) + } + args, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read gh args: %v", err) + } + command := string(args) + for _, want := range []string{ + "api repos/acme/widget/commits/2222222222222222222222222222222222222222?per_page=100", + "--paginate", + "--slurp", + } { + if !strings.Contains(command, want) { + t.Fatalf("gh args = %q, want %q", command, want) + } + } +} + func TestGHClient_RequestReviewers_UsesGitHubReviewRequestEndpoint(t *testing.T) { binDir := t.TempDir() logPath := filepath.Join(t.TempDir(), "gh-args.log") diff --git a/apps/backend/internal/github/handlers.go b/apps/backend/internal/github/handlers.go index c2189a22f0..7331af0205 100644 --- a/apps/backend/internal/github/handlers.go +++ b/apps/backend/internal/github/handlers.go @@ -53,6 +53,7 @@ func registerWSHandlers(dispatcher *ws.Dispatcher, svc *Service, log *logger.Log dispatcher.RegisterFunc(ws.ActionGitHubPRWatchDelete, wsDeletePRWatch(svc, log)) dispatcher.RegisterFunc(ws.ActionGitHubPRFilesGet, wsGetPRFiles(svc, log)) dispatcher.RegisterFunc(ws.ActionGitHubPRCommitsGet, wsGetPRCommits(svc, log)) + dispatcher.RegisterFunc(ws.ActionGitHubPRCommitGet, wsGetPRCommitDetail(svc, log)) dispatcher.RegisterFunc(ws.ActionGitHubTaskPRSync, wsSyncTaskPR(svc, log)) dispatcher.RegisterFunc(ws.ActionGitHubStats, wsGetStats(svc, log)) @@ -464,6 +465,35 @@ func wsGetPRCommits(svc *Service, _ *logger.Logger) func(ctx context.Context, ms } } +func wsGetPRCommitDetail(svc *Service, _ *logger.Logger) func(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + return func(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + payload, parseErr := parseMap(msg) + if parseErr != nil { + resp, _ := ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, errMsgInvalidPayload, nil) + return resp, nil + } + workspaceID, _ := payload["workspace_id"].(string) + owner, _ := payload["owner"].(string) + repo, _ := payload["repo"].(string) + sha, _ := payload["sha"].(string) + if workspaceID == "" || owner == "" || repo == "" || sha == "" { + resp, _ := ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "workspace_id, owner, repo, sha required", nil) + return resp, nil + } + if err := validateGitHubCommitSHA(sha); err != nil { + resp, _ := ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, err.Error(), nil) + return resp, nil + } + detail, err := svc.GetPRCommitDetailForWorkspace( + ctx, workspaceID, githubUserID(ctx), owner, repo, sha, + ) + if err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, err.Error(), nil) + } + return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{"commit": detail}) + } +} + // --- Issue watch WS handlers --- func wsListIssueWatches(svc *Service, _ *logger.Logger) func(ctx context.Context, msg *ws.Message) (*ws.Message, error) { diff --git a/apps/backend/internal/github/mock_client.go b/apps/backend/internal/github/mock_client.go index 8b158acf24..a54181abeb 100644 --- a/apps/backend/internal/github/mock_client.go +++ b/apps/backend/internal/github/mock_client.go @@ -70,6 +70,12 @@ type repoKey struct { Repo string } +type commitDetailKey struct { + Owner string + Repo string + SHA string +} + // repoFileEntry is one seeded file for MockClient.ListRepoDirectory / // GetRepoFileContent. Ref "" is a wildcard that matches any requested ref; // a non-empty Ref only matches that exact ref. @@ -104,6 +110,7 @@ type MockClient struct { checks map[checkKey][]CheckRun files map[prKey][]PRFile commits map[prKey][]PRCommitInfo + commitDetails map[commitDetailKey]PRCommitDetail submittedReviews []submittedReview requestedReviews []requestedReviewers mergedPRs []mergedPR @@ -151,6 +158,7 @@ func NewMockClient() *MockClient { checks: make(map[checkKey][]CheckRun), files: make(map[prKey][]PRFile), commits: make(map[prKey][]PRCommitInfo), + commitDetails: make(map[commitDetailKey]PRCommitDetail), mergeMethods: make(map[repoKey]RepoMergeMethods), gists: make(map[string]mockGist), repoFiles: make(map[repoKey][]repoFileEntry), @@ -449,6 +457,17 @@ func (m *MockClient) ListPRCommits(_ context.Context, owner, repo string, number return m.commits[prKey{owner, repo, number}], nil } +func (m *MockClient) GetPRCommitDetail(_ context.Context, owner, repo, sha string) (PRCommitDetail, error) { + m.mu.RLock() + defer m.mu.RUnlock() + detail, ok := m.commitDetails[commitDetailKey{Owner: owner, Repo: repo, SHA: sha}] + if !ok { + return PRCommitDetail{}, fmt.Errorf("mock: commit %s/%s@%s not found", owner, repo, sha) + } + detail.Files = append([]PRFile(nil), detail.Files...) + return detail, nil +} + func (m *MockClient) ListRepoBranches(_ context.Context, owner, repo string) ([]RepoBranch, error) { m.mu.RLock() defer m.mu.RUnlock() @@ -855,6 +874,17 @@ func (m *MockClient) AddPRCommits(owner, repo string, number int, commits []PRCo m.commits[k] = append(m.commits[k], commits...) } +// AddPRCommitDetail seeds an individual GitHub commit response. +func (m *MockClient) AddPRCommitDetail(owner, repo, sha string, detail PRCommitDetail) { + m.mu.Lock() + defer m.mu.Unlock() + if detail.SHA == "" { + detail.SHA = sha + } + detail.Files = append([]PRFile(nil), detail.Files...) + m.commitDetails[commitDetailKey{Owner: owner, Repo: repo, SHA: sha}] = detail +} + // Reset clears all mock data and resets the user to the default. func (m *MockClient) Reset() { m.mu.Lock() @@ -874,6 +904,7 @@ func (m *MockClient) Reset() { m.checks = make(map[checkKey][]CheckRun) m.files = make(map[prKey][]PRFile) m.commits = make(map[prKey][]PRCommitInfo) + m.commitDetails = make(map[commitDetailKey]PRCommitDetail) m.submittedReviews = nil m.requestedReviews = nil m.mergedPRs = nil diff --git a/apps/backend/internal/github/mock_controller.go b/apps/backend/internal/github/mock_controller.go index f167c608c4..d3890ee290 100644 --- a/apps/backend/internal/github/mock_controller.go +++ b/apps/backend/internal/github/mock_controller.go @@ -50,6 +50,7 @@ func (c *MockController) RegisterRoutes(router *gin.Engine) { api.POST("/checks", c.addCheckRuns) api.POST("/files", c.addPRFiles) api.POST("/commits", c.addPRCommits) + api.POST("/commit-details", c.addPRCommitDetail) api.POST("/branches", c.addBranches) api.POST("/repo-files", c.addRepoFiles) api.POST("/task-prs", c.associateTaskPR) @@ -500,6 +501,25 @@ func (c *MockController) addPRCommits(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"added": len(req.Commits)}) } +func (c *MockController) addPRCommitDetail(ctx *gin.Context) { + var req struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + SHA string `json:"sha"` + Detail PRCommitDetail `json:"detail"` + } + if err := ctx.ShouldBindJSON(&req); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) + return + } + if req.Owner == "" || req.Repo == "" || req.SHA == "" { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "owner, repo, and sha are required"}) + return + } + c.mock.AddPRCommitDetail(req.Owner, req.Repo, req.SHA, req.Detail) + ctx.JSON(http.StatusOK, gin.H{"added": 1}) +} + func (c *MockController) addBranches(ctx *gin.Context) { var req struct { Owner string `json:"owner"` diff --git a/apps/backend/internal/github/models.go b/apps/backend/internal/github/models.go index 594950cdb5..e4efdc4ed8 100644 --- a/apps/backend/internal/github/models.go +++ b/apps/backend/internal/github/models.go @@ -820,13 +820,29 @@ type PRFile struct { // PRCommitInfo represents a commit in a pull request. type PRCommitInfo struct { - SHA string `json:"sha"` - Message string `json:"message"` - AuthorLogin string `json:"author_login"` - AuthorDate string `json:"author_date"` - Additions int `json:"additions"` - Deletions int `json:"deletions"` - FilesChanged int `json:"files_changed"` + SHA string `json:"sha"` + Message string `json:"message"` + AuthorLogin string `json:"author_login"` + AuthorDate string `json:"author_date"` + Additions int `json:"additions"` + Deletions int `json:"deletions"` + FilesChanged int `json:"files_changed"` + StatsAvailable bool `json:"stats_available"` +} + +// PRCommitDetail represents the metadata and changed files for one GitHub +// commit. Unlike PRCommitInfo, its statistics come from GitHub's individual +// commit endpoint and are always measured. +type PRCommitDetail struct { + SHA string `json:"sha"` + Message string `json:"message"` + AuthorLogin string `json:"author_login"` + AuthorName string `json:"author_name"` + AuthorDate string `json:"author_date"` + Additions int `json:"additions"` + Deletions int `json:"deletions"` + FilesChanged int `json:"files_changed"` + Files []PRFile `json:"files"` } // DailyCount holds a date and count for chart data. diff --git a/apps/backend/internal/github/noop_client.go b/apps/backend/internal/github/noop_client.go index 44919e2db9..a3c00848c8 100644 --- a/apps/backend/internal/github/noop_client.go +++ b/apps/backend/internal/github/noop_client.go @@ -89,6 +89,10 @@ func (c *NoopClient) ListPRCommits(context.Context, string, string, int) ([]PRCo return nil, ErrNoClient } +func (c *NoopClient) GetPRCommitDetail(context.Context, string, string, string) (PRCommitDetail, error) { + return PRCommitDetail{}, ErrNoClient +} + func (c *NoopClient) ListRepoBranches(context.Context, string, string) ([]RepoBranch, error) { return nil, ErrNoClient } diff --git a/apps/backend/internal/github/pat_client.go b/apps/backend/internal/github/pat_client.go index 8124fe94b2..b6c0d2f599 100644 --- a/apps/backend/internal/github/pat_client.go +++ b/apps/backend/internal/github/pat_client.go @@ -578,6 +578,28 @@ func (c *PATClient) ListPRCommits(ctx context.Context, owner, repo string, numbe return convertRawPRCommits(raw), nil } +func (c *PATClient) GetPRCommitDetail(ctx context.Context, owner, repo, sha string) (PRCommitDetail, error) { + if err := validateGitHubCommitSHA(sha); err != nil { + return PRCommitDetail{}, err + } + var pages []ghPRCommitDetail + endpoint := fmt.Sprintf("/repos/%s/%s/commits/%s?per_page=100", owner, repo, sha) + for endpoint != "" { + var page ghPRCommitDetail + next, err := c.getPaginated(ctx, endpoint, &page) + if err != nil { + return PRCommitDetail{}, fmt.Errorf("get PR commit detail: %w", err) + } + pages = append(pages, page) + endpoint = next + } + encoded, err := json.Marshal(pages) + if err != nil { + return PRCommitDetail{}, fmt.Errorf("encode PR commit detail pages: %w", err) + } + return parsePRCommitDetailJSON(string(encoded)) +} + func (c *PATClient) ListRepoBranches(ctx context.Context, owner, repo string) ([]RepoBranch, error) { var branches []RepoBranch endpoint := fmt.Sprintf("/repos/%s/%s/branches?per_page=100", owner, repo) diff --git a/apps/backend/internal/github/pat_client_test.go b/apps/backend/internal/github/pat_client_test.go index 86f1667543..dba015b76f 100644 --- a/apps/backend/internal/github/pat_client_test.go +++ b/apps/backend/internal/github/pat_client_test.go @@ -196,6 +196,42 @@ func TestPATClient_ListCheckRunsPaginatesCheckRuns(t *testing.T) { } } +func TestPATClient_PRCommitDetailUsesExactSHAAndMergesPages(t *testing.T) { + var requested []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requested = append(requested, r.URL.RequestURI()) + if r.URL.Path != "/repos/acme/widget/commits/2222222222222222222222222222222222222222" { + t.Errorf("path = %q", r.URL.Path) + } + if r.URL.Query().Get("per_page") != "100" { + t.Errorf("per_page = %q, want 100", r.URL.Query().Get("per_page")) + } + if r.URL.Query().Get("page") == "2" { + _, _ = w.Write([]byte(`{"sha":"2222222222222222222222222222222222222222","commit":{"message":"ignored","author":{"name":"Other","date":"2026-08-05T11:00:00Z"}},"stats":{"additions":99,"deletions":99},"files":[{"filename":"two.txt","status":"removed","additions":0,"deletions":2,"patch":"@@ -1 +0,0 @@\n-gone"}]}`)) + return + } + w.Header().Set("Link", `<`+githubAPIBase+`/repos/acme/widget/commits/2222222222222222222222222222222222222222?per_page=100&page=2>; rel="next"`) + _, _ = w.Write([]byte(`{"sha":"2222222222222222222222222222222222222222","commit":{"message":"remote detail","author":{"name":"Octo Cat","date":"2026-08-04T11:00:00Z"}},"author":{"login":"octocat"},"stats":{"additions":7,"deletions":3},"files":[{"filename":"one.txt","status":"modified","additions":4,"deletions":1,"patch":"@@ -1 +1 @@\n-old\n+new"}]}`)) + })) + t.Cleanup(srv.Close) + + detail, err := newPATClientPointingAt(t, srv.URL).GetPRCommitDetail( + context.Background(), "acme", "widget", "2222222222222222222222222222222222222222", + ) + if err != nil { + t.Fatalf("GetPRCommitDetail: %v", err) + } + if detail.AuthorLogin != "octocat" || detail.AuthorName != "Octo Cat" || detail.FilesChanged != 2 { + t.Fatalf("detail = %#v", detail) + } + if detail.Additions != 7 || detail.Deletions != 3 || len(detail.Files) != 2 { + t.Fatalf("detail stats/files = %#v", detail) + } + if len(requested) != 2 { + t.Fatalf("requests = %#v, want two pages", requested) + } +} + func TestConvertPatPR_Mergeable(t *testing.T) { mergeable := true raw := &patPR{ diff --git a/apps/backend/internal/github/service_pr.go b/apps/backend/internal/github/service_pr.go index 628cc32066..249fabb4c3 100644 --- a/apps/backend/internal/github/service_pr.go +++ b/apps/backend/internal/github/service_pr.go @@ -548,6 +548,24 @@ func (s *Service) GetPRCommitsForWorkspace( return resolved.Client.ListPRCommits(ctx, owner, repo, number) } +// GetPRCommitDetailForWorkspace fetches one commit through the workspace's +// authorized personal read client. It never consults a task worktree. +func (s *Service) GetPRCommitDetailForWorkspace( + ctx context.Context, workspaceID, userID, owner, repo, sha string, +) (PRCommitDetail, error) { + if err := validateGitHubCommitSHA(sha); err != nil { + return PRCommitDetail{}, err + } + if err := s.ensureRepositoryInWorkspaceScope(ctx, workspaceID, owner, repo); err != nil { + return PRCommitDetail{}, err + } + resolved, err := s.resolvePersonalReadClient(ctx, workspaceID, userID, owner, repo) + if err != nil { + return PRCommitDetail{}, err + } + return resolved.Client.GetPRCommitDetail(ctx, owner, repo, sha) +} + // timeEqual compares two nullable time pointers for equality. func timeEqual(a, b *time.Time) bool { if a == nil && b == nil { diff --git a/apps/backend/pkg/websocket/actions.go b/apps/backend/pkg/websocket/actions.go index 82fd44da89..13108d6596 100644 --- a/apps/backend/pkg/websocket/actions.go +++ b/apps/backend/pkg/websocket/actions.go @@ -457,6 +457,7 @@ const ( ActionGitHubPRWatchDelete = "github.pr_watches.delete" ActionGitHubPRFilesGet = "github.pr_files.get" ActionGitHubPRCommitsGet = "github.pr_commits.get" + ActionGitHubPRCommitGet = "github.pr_commit.get" ActionGitHubTaskPRUpdated = "github.task_pr.updated" // Notification ActionGitHubTaskPRDeleted = "github.task_pr.deleted" // Notification ActionGitHubTaskCIOptionsUpdated = "github.task_ci_options.updated" // Notification diff --git a/apps/web/components/task/changes-diff-target.ts b/apps/web/components/task/changes-diff-target.ts index 69ecf6b0b9..5c8f5b714f 100644 --- a/apps/web/components/task/changes-diff-target.ts +++ b/apps/web/components/task/changes-diff-target.ts @@ -1,5 +1,24 @@ export type DiffSource = "uncommitted" | "committed" | "pr"; +export type LocalCommitDetailTarget = { + source: "local"; + sha: string; + /** Multi-repo local repository subpath, omitted for the workspace root. */ + repo?: string; +}; + +export type GitHubCommitDetailTarget = { + source: "github"; + sha: string; + workspaceId: string; + owner: string; + repo: string; + /** Local display/group identity for the linked repository. */ + repositoryName?: string; +}; + +export type CommitDetailTarget = LocalCommitDetailTarget | GitHubCommitDetailTarget; + export type OpenDiffOptions = { source?: DiffSource; repositoryName?: string; @@ -15,4 +34,4 @@ export type DiffSheetMode = repositoryName?: string; prKey?: string; } - | { kind: "commit"; sha: string; repo?: string }; + | { kind: "commit"; target: CommitDetailTarget }; diff --git a/apps/web/components/task/changes-panel-data.tsx b/apps/web/components/task/changes-panel-data.tsx index d3cf850d67..e1e24f7d95 100644 --- a/apps/web/components/task/changes-panel-data.tsx +++ b/apps/web/components/task/changes-panel-data.tsx @@ -36,8 +36,9 @@ import { buildRepoNameById, selectPRFilesForReviewProgress, type ReviewProgressPRSource, + type PRCommitForMerge, } from "./changes-panel-helpers"; -import type { OpenDiffOptions } from "./changes-diff-target"; +import type { CommitDetailTarget, OpenDiffOptions } from "./changes-diff-target"; import type { PRDiffFile, TaskPR } from "@/lib/types/github"; import { getGitCredentialDisplay } from "./changes-git-credential-display"; @@ -95,14 +96,7 @@ export type ChangesPanelBodyProps = { unstagedFiles: ChangedFile[]; stagedFiles: ChangedFile[]; prFiles: PRChangedFile[]; - prCommits: { - sha: string; - message: string; - author_login: string; - author_date: string; - additions: number; - deletions: number; - }[]; + prCommits: PRCommitForMerge[]; commits: { commit_sha: string; commit_message: string; @@ -120,7 +114,7 @@ export type ChangesPanelBodyProps = { dialogs: DialogsType; onOpenDiffFile: (path: string, options?: OpenDiffOptions) => void; onEditFile: (path: string, repo?: string) => void; - onOpenCommitDetail?: (sha: string, repo?: string) => void; + onOpenCommitDetail?: (target: CommitDetailTarget) => void; onOpenReview?: () => void; onRevertCommit?: (sha: string, repo?: string) => void; onStageAll: () => void; @@ -256,6 +250,7 @@ function buildChangesPanelPRData({ function useChangesPanelPRData(repositoryNames: string[], sessionId: string | null) { const { prs, filesByPRKey } = useActiveTaskPRsWithFiles(); const activeTaskId = useAppStore((state) => state.tasks.activeTaskId); + const workspaceId = useAppStore((state) => state.workspaces.activeId); const { selectedPR: taskPR } = useReviewPRSelection(activeTaskId); const reposByWorkspace = useAppStore((s) => s.repositories.itemsByWorkspaceId); const repoNameById = useMemo(() => buildRepoNameById(reposByWorkspace), [reposByWorkspace]); @@ -275,6 +270,16 @@ function useChangesPanelPRData(repositoryNames: string[], sessionId: string | nu taskPR?.pr_number ?? null, refreshKey, ); + const prCommits = useMemo(() => { + if (!workspaceId?.trim() || !taskPR?.owner?.trim() || !taskPR.repo?.trim()) return []; + return prCommitsList.map((commit) => ({ + ...commit, + workspace_id: workspaceId, + owner: taskPR.owner, + repo: taskPR.repo, + repository_name: useRepositoryKeys ? selectedPRRepositoryName : undefined, + })); + }, [prCommitsList, workspaceId, taskPR?.owner, taskPR?.repo, selectedPRRepositoryName]); const { prFiles, prDiffFiles } = useMemo( () => buildChangesPanelPRData({ @@ -297,10 +302,10 @@ function useChangesPanelPRData(repositoryNames: string[], sessionId: string | nu ], ); const hasPRFiles = prFiles.length > 0; - const hasPRCommits = prCommitsList.length > 0; + const hasPRCommits = prCommits.length > 0; return { prDiffFiles, - prCommitsList, + prCommits, hasPRFiles, hasPRCommits, prFiles, @@ -389,7 +394,7 @@ export function useChangesPanelData() { type ChangesPanelCallbacks = { onOpenDiffFile: (path: string, options?: OpenDiffOptions) => void; onEditFile: (path: string, repo?: string) => void; - onOpenCommitDetail?: (sha: string, repo?: string) => void; + onOpenCommitDetail?: (target: CommitDetailTarget) => void; onOpenReview?: () => void; }; @@ -411,7 +416,7 @@ export function buildChangesPanelBodyProps( unstagedFiles: data.unstagedFiles, stagedFiles: data.stagedFiles, prFiles: data.prFiles, - prCommits: data.prCommitsList, + prCommits: data.prCommits, commits: git.commits, pendingStageFiles: git.pendingStageFiles, reviewedCount: data.reviewedCount, diff --git a/apps/web/components/task/changes-panel-helpers.ts b/apps/web/components/task/changes-panel-helpers.ts index b1614b3180..022975dddf 100644 --- a/apps/web/components/task/changes-panel-helpers.ts +++ b/apps/web/components/task/changes-panel-helpers.ts @@ -10,6 +10,7 @@ import type { FileInfo } from "@/lib/state/store"; import type { PRDiffFile } from "@/lib/types/github"; import { normalizeFileChangeStatus, type FileChangeStatus } from "@/lib/utils/file-change-status"; import type { PRChangedFile } from "./changes-panel-timeline"; +import type { CommitDetailTarget } from "./changes-diff-target"; export type ChangedFile = { path: string; @@ -266,17 +267,82 @@ export function firstVisibleSection(flags: { return null; } -type MergedCommit = { +export type MergedCommit = { commit_sha: string; commit_message: string; insertions: number; deletions: number; pushed: boolean; + statsAvailable: boolean; + detailTarget: CommitDetailTarget; /** Multi-repo: name of the repo this commit was made in. Empty for single-repo. */ repository_name?: string; committed_at?: string; }; +export type PRCommitForMerge = { + sha: string; + message: string; + additions: number; + deletions: number; + author_date?: string; + stats_available?: boolean; + workspace_id: string; + owner: string; + repo: string; + repository_name?: string; +}; + +function localCommitTarget(commit: { + commit_sha: string; + repository_name?: string; +}): CommitDetailTarget { + return { + source: "local", + sha: commit.commit_sha, + ...(commit.repository_name ? { repo: commit.repository_name } : {}), + }; +} + +function githubCommitTarget(commit: PRCommitForMerge): CommitDetailTarget | null { + if (!commit.workspace_id.trim() || !commit.owner.trim() || !commit.repo.trim()) return null; + return { + source: "github", + sha: commit.sha, + workspaceId: commit.workspace_id, + owner: commit.owner, + repo: commit.repo, + ...(commit.repository_name ? { repositoryName: commit.repository_name } : {}), + }; +} + +function prCommitKey(pr: PRCommitForMerge): string { + return `${pr.repository_name ?? ""}\0${pr.sha}`; +} + +function appendPROnlyCommits( + prCommits: PRCommitForMerge[], + matchedPRKeys: Set, + pushed: MergedCommit[], +): void { + for (const pr of prCommits) { + if (matchedPRKeys.has(prCommitKey(pr))) continue; + const detailTarget = githubCommitTarget(pr); + if (!detailTarget) continue; + pushed.push({ + commit_sha: pr.sha, + commit_message: pr.message, + insertions: pr.additions, + deletions: pr.deletions, + pushed: true, + statsAvailable: pr.stats_available === true, + detailTarget, + ...(pr.repository_name ? { repository_name: pr.repository_name } : {}), + committed_at: pr.author_date, + }); + } +} + /** * Merge local session commits and PR commits into a single list. A commit is * pushed when EITHER source confirms it: the backend's `pushed` field @@ -299,46 +365,46 @@ export function mergeCommits( pushed?: boolean; committed_at?: string; }[], - prCommits: { - sha: string; - message: string; - additions: number; - deletions: number; - author_date?: string; - }[], + prCommits: PRCommitForMerge[], ): MergedCommit[] { const shaMatches = (a: string, b: string) => a.startsWith(b) || b.startsWith(a); + const repositoryMatches = (a?: string, b?: string) => (a ?? "") === (b ?? ""); + const commitsMatch = ( + local: { commit_sha: string; repository_name?: string }, + pr: PRCommitForMerge, + ) => + shaMatches(pr.sha, local.commit_sha) && + repositoryMatches(local.repository_name, pr.repository_name); const unpushed: MergedCommit[] = []; const pushed: MergedCommit[] = []; - const matchedPRShas = new Set(); + const matchedPRKeys = new Set(); for (const c of localCommits) { - const matchesPR = prCommits.some((pr) => shaMatches(pr.sha, c.commit_sha)); + const matchesPR = prCommits.some((pr) => commitsMatch(c, pr)); const isPushed = c.pushed === true || matchesPR; if (isPushed) { - pushed.push({ ...c, pushed: true }); + pushed.push({ + ...c, + pushed: true, + statsAvailable: true, + detailTarget: localCommitTarget(c), + }); } else { - unpushed.push({ ...c, pushed: false }); + unpushed.push({ + ...c, + pushed: false, + statsAvailable: true, + detailTarget: localCommitTarget(c), + }); } if (matchesPR) { for (const pr of prCommits) { - if (shaMatches(pr.sha, c.commit_sha)) { - matchedPRShas.add(pr.sha); + if (commitsMatch(c, pr)) { + matchedPRKeys.add(prCommitKey(pr)); } } } } - for (const pr of prCommits) { - if (!matchedPRShas.has(pr.sha)) { - pushed.push({ - commit_sha: pr.sha, - commit_message: pr.message, - insertions: pr.additions, - deletions: pr.deletions, - pushed: true, - committed_at: pr.author_date, - }); - } - } + appendPROnlyCommits(prCommits, matchedPRKeys, pushed); return [...unpushed, ...pushed]; } diff --git a/apps/web/components/task/changes-panel-remote.test.ts b/apps/web/components/task/changes-panel-remote.test.ts new file mode 100644 index 0000000000..ce9f9dd4b6 --- /dev/null +++ b/apps/web/components/task/changes-panel-remote.test.ts @@ -0,0 +1,163 @@ +import { describe, expect, it } from "vitest"; +import { mergeCommits } from "./changes-panel"; + +const PR_DATE = "2026-03-29T00:00:00Z"; +const SHARED_SHA = "ccc3333"; +const WIDGET_REPO = "widget"; +const WORKSPACE_ID = "workspace-1"; + +function makeLocal(sha: string, message = "msg") { + return { commit_sha: sha, commit_message: message, insertions: 1, deletions: 0 }; +} + +function makePR(sha: string, message = "msg") { + return { + sha, + message, + author_login: "user", + author_date: PR_DATE, + additions: 5, + deletions: 2, + files_changed: 1, + }; +} + +describe("mergeCommits source provenance", () => { + it("marks PR-only commits as remote with explicitly unavailable stats", () => { + const pr = { + ...makePR(SHARED_SHA, "external fix"), + stats_available: false, + workspace_id: WORKSPACE_ID, + owner: "acme", + repo: WIDGET_REPO, + repository_name: WIDGET_REPO, + }; + + expect(mergeCommits([], [pr])).toMatchObject([ + { + statsAvailable: false, + detailTarget: { + source: "github", + sha: SHARED_SHA, + workspaceId: WORKSPACE_ID, + owner: "acme", + repo: WIDGET_REPO, + }, + }, + ]); + }); + + it("prefers the local source when a SHA exists in both feeds", () => { + const local = [{ ...makeLocal(SHARED_SHA, "local"), repository_name: WIDGET_REPO }]; + const pr = { + ...makePR(SHARED_SHA, "remote"), + stats_available: false, + workspace_id: WORKSPACE_ID, + owner: "acme", + repo: WIDGET_REPO, + repository_name: WIDGET_REPO, + }; + + expect(mergeCommits(local, [pr])).toMatchObject([ + { + commit_message: "local", + statsAvailable: true, + detailTarget: { source: "local", sha: SHARED_SHA, repo: WIDGET_REPO }, + }, + ]); + }); + + it("does not deduplicate equal SHAs from different repositories", () => { + const local = [{ ...makeLocal(SHARED_SHA, "local"), repository_name: "widget-a" }]; + const pr = { + ...makePR(SHARED_SHA, "remote"), + stats_available: false, + workspace_id: WORKSPACE_ID, + owner: "acme", + repo: "widget-b", + repository_name: "widget-b", + }; + + const result = mergeCommits(local, [pr]); + expect(result).toHaveLength(2); + expect(result[1]).toMatchObject({ + repository_name: "widget-b", + detailTarget: { source: "github", repo: "widget-b" }, + }); + }); + + it("keeps a same-SHA PR-only commit when another repository matches locally", () => { + const local = [{ ...makeLocal(SHARED_SHA, "local"), repository_name: "widget-a" }]; + const matchedPR = { + ...makePR(SHARED_SHA, "local PR"), + stats_available: false, + workspace_id: WORKSPACE_ID, + owner: "acme", + repo: "widget-a", + repository_name: "widget-a", + }; + const remotePR = { + ...makePR(SHARED_SHA, "remote PR"), + stats_available: false, + workspace_id: WORKSPACE_ID, + owner: "acme", + repo: "widget-b", + repository_name: "widget-b", + }; + + const result = mergeCommits(local, [matchedPR, remotePR]); + + expect(result).toHaveLength(2); + expect(result[0]).toMatchObject({ + commit_message: "local", + detailTarget: { source: "local", repo: "widget-a" }, + }); + expect(result[1]).toMatchObject({ + commit_message: "remote PR", + repository_name: "widget-b", + detailTarget: { source: "github", repo: "widget-b" }, + }); + }); +}); + +describe("mergeCommits repository identity", () => { + it("does not create a PR-only target without complete provenance", () => { + const pr = { + ...makePR(SHARED_SHA, "remote"), + stats_available: false, + workspace_id: "", + owner: "acme", + repo: WIDGET_REPO, + repository_name: WIDGET_REPO, + }; + + expect(mergeCommits([], [pr])).toEqual([]); + }); + + it("does not match a root local commit to a named repository PR", () => { + const local = [makeLocal(SHARED_SHA, "root local")]; + const pr = { + ...makePR(SHARED_SHA, "remote"), + stats_available: false, + workspace_id: WORKSPACE_ID, + owner: "acme", + repo: WIDGET_REPO, + repository_name: WIDGET_REPO, + }; + + const result = mergeCommits(local, [pr]); + + expect(result).toHaveLength(2); + expect(result[0]).toMatchObject({ + commit_message: "root local", + pushed: false, + detailTarget: { source: "local", sha: SHARED_SHA }, + }); + expect(result[1]).toMatchObject({ + commit_message: "remote", + pushed: true, + repository_name: WIDGET_REPO, + detailTarget: { source: "github", repo: WIDGET_REPO }, + }); + }); +}); diff --git a/apps/web/components/task/changes-panel-repo-groups.tsx b/apps/web/components/task/changes-panel-repo-groups.tsx index a217f78822..482fe4b88e 100644 --- a/apps/web/components/task/changes-panel-repo-groups.tsx +++ b/apps/web/components/task/changes-panel-repo-groups.tsx @@ -10,6 +10,7 @@ import { import { Button } from "@kandev/ui/button"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; import { CommitRow, type CommitItem } from "./commit-row"; +import type { CommitDetailTarget } from "./changes-diff-target"; import { groupByRepositoryName } from "@/lib/group-by-repo"; import type { ChangedFile } from "./changes-panel-helpers"; @@ -222,7 +223,7 @@ export function CommitsRepoGroup({ * workspaces use this — the action buttons (Push / PR) move up to the * section header so we don't render a redundant repo sub-header. */ showHeader?: boolean; - onOpenCommitDetail?: (sha: string, repo?: string) => void; + onOpenCommitDetail?: (target: CommitDetailTarget) => void; onAmendCommit?: (currentMessage: string, repo?: string) => void; onRevertCommit?: (sha: string, repo?: string) => void; onResetToCommit?: (sha: string, repo?: string) => void; diff --git a/apps/web/components/task/changes-panel-timeline-grouping.test.tsx b/apps/web/components/task/changes-panel-timeline-grouping.test.tsx index f08e89c55f..630daa1c98 100644 --- a/apps/web/components/task/changes-panel-timeline-grouping.test.tsx +++ b/apps/web/components/task/changes-panel-timeline-grouping.test.tsx @@ -71,6 +71,12 @@ function commit(sha: string, message: string, repo?: string): CommitProps["commi insertions: 1, deletions: 0, pushed: false, + statsAvailable: true, + detailTarget: { + source: "local", + sha, + ...(repo ? { repo } : {}), + }, repository_name: repo, }; } @@ -304,6 +310,8 @@ describe("section auto-expand (defaultCollapsed prop)", () => { insertions: 1, deletions: 0, pushed: false, + statsAvailable: true, + detailTarget: { source: "local", sha: "abc123" }, repository_name: undefined, }, ]} diff --git a/apps/web/components/task/changes-panel-timeline.tsx b/apps/web/components/task/changes-panel-timeline.tsx index 02279e43c9..2a7acb4c40 100644 --- a/apps/web/components/task/changes-panel-timeline.tsx +++ b/apps/web/components/task/changes-panel-timeline.tsx @@ -20,7 +20,7 @@ import { RepoGroupItem, } from "./changes-panel-repo-groups"; import { PRFilesGroupedList } from "./changes-panel-pr-files"; -import type { OpenDiffOptions } from "./changes-diff-target"; +import type { CommitDetailTarget, OpenDiffOptions } from "./changes-diff-target"; // --- Timeline visual components --- @@ -129,7 +129,7 @@ function TimelineSection({ type CommitsSectionProps = { commits: CommitItem[]; - onOpenCommitDetail?: (sha: string, repo?: string) => void; + onOpenCommitDetail?: (target: CommitDetailTarget) => void; // Handlers receive the commit's repository_name so amend/revert/reset land // in the right git repo. The empty string routes to the workspace root for // single-repo workspaces. diff --git a/apps/web/components/task/changes-panel.test.ts b/apps/web/components/task/changes-panel.test.ts index 88013d5659..0861761b7f 100644 --- a/apps/web/components/task/changes-panel.test.ts +++ b/apps/web/components/task/changes-panel.test.ts @@ -9,6 +9,10 @@ const makeLocal = (sha: string, message = "msg") => ({ }); const PR_DATE = "2026-03-29T00:00:00Z"; +const WORKSPACE_ID = "workspace-1"; +const PR_OWNER = "acme"; +const PR_REPO = "widget"; +const REMOTE_SHA = "ccc3333"; const makePR = (sha: string) => ({ sha, @@ -49,26 +53,27 @@ const makePRFull = (sha: string, message = "msg", author = "user") => ({ additions: 5, deletions: 2, files_changed: 1, + workspace_id: WORKSPACE_ID, + owner: PR_OWNER, + repo: PR_REPO, }); describe("mergeCommits", () => { it("marks all local commits as unpushed when no PR commits exist", () => { const local = [makeLocal("aaa1111", "first"), makeLocal("bbb2222", "second")]; const result = mergeCommits(local, []); - expect(result).toEqual([ + expect(result).toMatchObject([ { commit_sha: "aaa1111", - commit_message: "first", - insertions: 1, - deletions: 0, pushed: false, + statsAvailable: true, + detailTarget: { source: "local", sha: "aaa1111" }, }, { commit_sha: "bbb2222", - commit_message: "second", - insertions: 1, - deletions: 0, pushed: false, + statsAvailable: true, + detailTarget: { source: "local", sha: "bbb2222" }, }, ]); }); @@ -106,23 +111,34 @@ describe("mergeCommits", () => { const local = [makeLocal("aaa1111", "first")]; const pr = [makePRFull("aaa1111bbbccc", "first", "user")]; const result = mergeCommits(local, pr); - expect(result).toEqual([ - { commit_sha: "aaa1111", commit_message: "first", insertions: 1, deletions: 0, pushed: true }, + expect(result).toMatchObject([ + { + commit_sha: "aaa1111", + pushed: true, + statsAvailable: true, + detailTarget: { source: "local", sha: "aaa1111" }, + }, ]); }); it("includes PR-only commits (from other contributors) as pushed", () => { const local: ReturnType[] = []; - const pr = [makePRFull("ccc3333", "external fix", "other-dev")]; + const pr = [makePRFull(REMOTE_SHA, "external fix", "other-dev")]; const result = mergeCommits(local, pr); - expect(result).toEqual([ + expect(result).toMatchObject([ { - commit_sha: "ccc3333", + commit_sha: REMOTE_SHA, commit_message: "external fix", - insertions: 5, - deletions: 2, pushed: true, committed_at: PR_DATE, + statsAvailable: false, + detailTarget: { + source: "github", + sha: REMOTE_SHA, + workspaceId: WORKSPACE_ID, + owner: PR_OWNER, + repo: PR_REPO, + }, }, ]); }); diff --git a/apps/web/components/task/changes-panel.tsx b/apps/web/components/task/changes-panel.tsx index 14109df91d..d2b4162f58 100644 --- a/apps/web/components/task/changes-panel.tsx +++ b/apps/web/components/task/changes-panel.tsx @@ -7,7 +7,7 @@ import { ChangesPanelHeader } from "./changes-panel-header"; import { filterUnpushedCommits, mergeCommits } from "./changes-panel-helpers"; import { useChangesPanelData, buildChangesPanelBodyProps } from "./changes-panel-data"; import { ChangesPanelBody } from "./changes-panel-body"; -import type { OpenDiffOptions } from "./changes-diff-target"; +import type { CommitDetailTarget, OpenDiffOptions } from "./changes-diff-target"; import { useRequestChangesWalkthrough } from "@/hooks/domains/session/use-request-changes-walkthrough"; export { filterUnpushedCommits, mergeCommits }; @@ -15,7 +15,7 @@ export { filterUnpushedCommits, mergeCommits }; type ChangesPanelProps = { onOpenDiffFile: (path: string, options?: OpenDiffOptions) => void; onEditFile: (path: string, repo?: string) => void; - onOpenCommitDetail?: (sha: string, repo?: string) => void; + onOpenCommitDetail?: (target: CommitDetailTarget) => void; onOpenDiffAll?: () => void; onOpenReview?: () => void; }; diff --git a/apps/web/components/task/commit-detail-panel.test.tsx b/apps/web/components/task/commit-detail-panel.test.tsx new file mode 100644 index 0000000000..4a8efa93cd --- /dev/null +++ b/apps/web/components/task/commit-detail-panel.test.tsx @@ -0,0 +1,85 @@ +import { fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const mocks = vi.hoisted(() => ({ + refetch: vi.fn(), + useCommitDetail: vi.fn(() => ({ + files: null, + commit: null, + loading: false, + error: "Commit detail unavailable", + refetch: mocks.refetch, + })), +})); + +vi.mock("@/components/state-provider", () => ({ + useAppStore: (selector: (state: unknown) => unknown) => + selector({ tasks: { activeSessionId: null } }), +})); + +vi.mock("@/hooks/domains/session/use-session-commits", () => ({ + useSessionCommits: () => ({ commits: [] }), +})); + +vi.mock("@/hooks/domains/session/use-commit-detail", () => ({ + useCommitDetail: mocks.useCommitDetail, +})); + +vi.mock("@/hooks/use-panel-actions", () => ({ + usePanelActions: () => ({ openFile: vi.fn() }), +})); + +vi.mock("@/lib/layout/panel-portal-manager", () => ({ + setPanelTitle: vi.fn(), +})); + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock("./panel-primitives", () => ({ + PanelRoot: ({ children }: { children: React.ReactNode }) =>
{children}
, + PanelBody: ({ children }: { children: React.ReactNode }) =>
{children}
, +})); + +import { CommitDetailPanel, CommitDiffView } from "./commit-detail-panel"; + +afterEach(() => { + mocks.refetch.mockReset(); + mocks.useCommitDetail.mockClear(); +}); + +describe("CommitDiffView error state", () => { + it("does not present a protocol failure as an empty commit and offers retry", () => { + render( + , + ); + + expect(screen.getByRole("alert").textContent).toContain("Commit detail unavailable"); + expect(screen.queryByText("No files in this commit")).toBeNull(); + + fireEvent.click(screen.getByRole("button", { name: "system:featureTogglesRetry" })); + expect(mocks.refetch).toHaveBeenCalledOnce(); + }); +}); + +describe("CommitDetailPanel target validation", () => { + it("does not accept a partial GitHub target from serialized params", () => { + render( + , + ); + + expect(mocks.useCommitDetail).toHaveBeenCalledWith({ source: "local", sha: "" }); + }); +}); diff --git a/apps/web/components/task/commit-detail-panel.tsx b/apps/web/components/task/commit-detail-panel.tsx index b3320ca1ef..42d376c6df 100644 --- a/apps/web/components/task/commit-detail-panel.tsx +++ b/apps/web/components/task/commit-detail-panel.tsx @@ -1,17 +1,20 @@ "use client"; import { memo, useEffect, useMemo } from "react"; -import { IconLoader2 } from "@tabler/icons-react"; +import { IconAlertCircle, IconLoader2, IconRefresh } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { useTranslation } from "react-i18next"; import { PanelRoot, PanelBody } from "./panel-primitives"; import { FileDiffViewer } from "@/components/diff"; import { DEFAULT_DIFF_WORD_WRAP } from "@/components/diff/diff-defaults"; import { useAppStore } from "@/components/state-provider"; import { useSessionCommits } from "@/hooks/domains/session/use-session-commits"; -import { useCommitDiff } from "@/hooks/domains/session/use-commit-diff"; +import { useCommitDetail } from "@/hooks/domains/session/use-commit-detail"; import { usePanelActions } from "@/hooks/use-panel-actions"; import { setPanelTitle } from "@/lib/layout/panel-portal-manager"; import { formatRelativeTime } from "@/lib/utils"; import type { FileInfo } from "@/lib/state/store"; +import type { CommitDetailTarget } from "./changes-diff-target"; type CommitDetailPanelProps = { panelId: string; @@ -19,12 +22,44 @@ type CommitDetailPanelProps = { }; type CommitDiffViewProps = { - sha: string; - repo?: string; + target: CommitDetailTarget; onOpenFile?: (path: string) => void; wordWrap?: boolean; }; +function isCommitDetailTarget(value: unknown): value is CommitDetailTarget { + if (!value || typeof value !== "object") return false; + const target = value as { + source?: unknown; + sha?: unknown; + repo?: unknown; + workspaceId?: unknown; + owner?: unknown; + }; + if (typeof target.sha !== "string" || target.sha.length === 0) return false; + if (target.source === "local") { + return target.repo === undefined || typeof target.repo === "string"; + } + return ( + target.source === "github" && + typeof target.workspaceId === "string" && + target.workspaceId.length > 0 && + typeof target.owner === "string" && + target.owner.length > 0 && + typeof target.repo === "string" && + target.repo.length > 0 + ); +} + +function targetFromParams(params: Record): CommitDetailTarget { + if (isCommitDetailTarget(params.target)) return params.target; + return { + source: "local", + sha: typeof params.commitSha === "string" ? params.commitSha : "", + ...(typeof params.repo === "string" && params.repo ? { repo: params.repo } : {}), + }; +} + function getInitials(name: string): string { return name .split(/\s+/) @@ -42,21 +77,43 @@ function useSortedFileEntries(files: Record | null): [string, }, [files]); } -function useActiveCommit(commitSha: string) { +function useActiveCommit(target: CommitDetailTarget) { const activeSessionId = useAppStore((state) => state.tasks.activeSessionId); const { commits } = useSessionCommits(activeSessionId ?? null); - return useMemo(() => commits.find((c) => c.commit_sha === commitSha), [commits, commitSha]); + return useMemo(() => { + if (target.source !== "local") return undefined; + return commits.find( + (c) => + c.commit_sha === target.sha && + (!target.repo || !c.repository_name || c.repository_name === target.repo), + ); + }, [commits, target]); +} + +function headerCommit( + target: CommitDetailTarget, + localCommit: ReturnType, + remoteCommit: ReturnType["commit"], +) { + if (target.source === "github" && remoteCommit) { + return { + author_name: remoteCommit.author_name || remoteCommit.author_login, + commit_message: remoteCommit.message, + committed_at: remoteCommit.author_date, + }; + } + return localCommit; } /** Standalone commit diff viewer — no dockview dependencies. */ export const CommitDiffView = memo(function CommitDiffView({ - sha: commitSha, - repo, + target, onOpenFile, wordWrap = DEFAULT_DIFF_WORD_WRAP, }: CommitDiffViewProps) { - const commit = useActiveCommit(commitSha); - const { files, loading } = useCommitDiff(commitSha, repo); + const localCommit = useActiveCommit(target); + const { files, commit: remoteCommit, loading, error, refetch } = useCommitDetail(target); + const commit = headerCommit(target, localCommit, remoteCommit); const fileEntries = useSortedFileEntries(files); if (loading) { @@ -67,16 +124,18 @@ export const CommitDiffView = memo(function CommitDiffView({ ); } + if (error) return ; return (
-
{commit && }
+
{commit && }
{})} - baseRef={`${commitSha}^`} - repo={repo} + onOpenFile={target.source === "local" ? onOpenFile : undefined} + baseRef={target.source === "local" ? `${target.sha}^` : undefined} + repo={target.source === "local" ? target.repo : undefined} + remote={target.source === "github"} wordWrap={wordWrap} />
@@ -87,24 +146,24 @@ const CommitDetailPanel = memo(function CommitDetailPanel({ panelId, params, }: CommitDetailPanelProps) { - const commitSha = params.commitSha as string; - const repo = (params.repo as string | undefined) ?? undefined; + const target = targetFromParams(params); const { openFile } = usePanelActions(); - const commit = useActiveCommit(commitSha); - const { files, loading } = useCommitDiff(commitSha, repo); + const localCommit = useActiveCommit(target); + const { files, commit: remoteCommit, loading, error, refetch } = useCommitDetail(target); + const commit = headerCommit(target, localCommit, remoteCommit); const fileEntries = useSortedFileEntries(files); // Update tab title via dockview API stored in portal manager useEffect(() => { if (commit) { - const shortSha = commitSha.slice(0, 7); + const shortSha = target.sha.slice(0, 7); const msg = commit.commit_message.length > 30 ? commit.commit_message.slice(0, 30) + "..." : commit.commit_message; setPanelTitle(panelId, `${shortSha} ${msg}`); } - }, [commit, commitSha, panelId]); + }, [commit, panelId, target.sha]); if (loading) { return ( @@ -118,19 +177,29 @@ const CommitDetailPanel = memo(function CommitDetailPanel({ ); } + if (error) { + return ( + + + + + + ); + } return (
- {commit && } + {commit && }
@@ -168,6 +237,35 @@ function CommitHeader({ ); } +function CommitDetailErrorState({ + error, + onRetry, +}: { + error: string; + onRetry: () => Promise; +}) { + const { t } = useTranslation(); + return ( +
+ + {error} + +
+ ); +} + /** List of file diffs in a commit */ function CommitFileList({ fileEntries, @@ -175,13 +273,15 @@ function CommitFileList({ onOpenFile, baseRef, repo, + remote, wordWrap, }: { fileEntries: [string, FileInfo][]; loading: boolean; - onOpenFile: (path: string) => void; - baseRef: string; + onOpenFile?: (path: string) => void; + baseRef?: string; repo?: string; + remote: boolean; wordWrap?: boolean; }) { if (fileEntries.length === 0 && !loading) { @@ -200,9 +300,9 @@ function CommitFileList({ diff={file.diff} status={file.status} onOpenFile={onOpenFile} - enableExpansion={true} + enableExpansion={!remote} baseRef={baseRef} - repo={repo} + repo={remote ? undefined : repo} wordWrap={wordWrap} /> ) : ( diff --git a/apps/web/components/task/commit-detail-request.test.ts b/apps/web/components/task/commit-detail-request.test.ts new file mode 100644 index 0000000000..5951b63e59 --- /dev/null +++ b/apps/web/components/task/commit-detail-request.test.ts @@ -0,0 +1,195 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { PRCommitDetail } from "@/lib/types/github"; +import type { FileInfo } from "@/lib/state/store"; + +const mocks = vi.hoisted(() => ({ + request: vi.fn(), + requestCommitDiff: vi.fn(), +})); +let websocketClient: { request: typeof mocks.request } | null = { request: mocks.request }; + +vi.mock("@/lib/ws/connection", () => ({ + getWebSocketClient: () => websocketClient, +})); + +vi.mock("./commit-diff-request", () => ({ + requestCommitDiff: mocks.requestCommitDiff, +})); + +import { mapGitHubCommitFiles, requestCommitDetail } from "./commit-detail-request"; + +const WORKSPACE_ID = "workspace-1"; +const remoteTarget = { + source: "github" as const, + sha: "abc1234567890", + workspaceId: WORKSPACE_ID, + owner: "acme", + repo: "widget", +}; + +const detail: PRCommitDetail = { + sha: "abc1234567890", + message: "remote commit message", + author_login: "octocat", + author_name: "Octo Cat", + author_date: "2026-08-04T12:00:00Z", + additions: 3, + deletions: 1, + files_changed: 2, + files: [ + { + filename: "src/new.ts", + status: "added", + additions: 3, + deletions: 0, + patch: "@@ -0,0 +1 @@\n+new", + }, + { + filename: "assets/logo.bin", + status: "modified", + additions: 0, + deletions: 1, + patch: "", + }, + ], +}; + +afterEach(() => { + mocks.request.mockReset(); + mocks.requestCommitDiff.mockReset(); + websocketClient = { request: mocks.request }; +}); + +describe("requestCommitDetail", () => { + it("uses the workspace GitHub action for a remote target", async () => { + mocks.request.mockResolvedValue({ commit: detail }); + + const result = await requestCommitDetail({ + target: { + source: "github", + sha: detail.sha, + workspaceId: WORKSPACE_ID, + owner: "acme", + repo: "widget", + }, + local: { + sessionId: "session-1", + taskId: "task-1", + agentctlReady: true, + }, + }); + + expect(result).toMatchObject({ source: "github", success: true, commit: detail }); + expect(mocks.request).toHaveBeenCalledWith( + "github.pr_commit.get", + { + workspace_id: WORKSPACE_ID, + owner: "acme", + repo: "widget", + sha: detail.sha, + }, + 10000, + ); + expect(mocks.requestCommitDiff).not.toHaveBeenCalled(); + }); + + it("uses the local session action only for a local target", async () => { + const files: Record = { + "src/local.ts": { + path: "src/local.ts", + status: "modified", + staged: false, + diff: "@@ -1 +1 @@", + }, + }; + mocks.requestCommitDiff.mockResolvedValue({ success: true, files }); + + const result = await requestCommitDetail({ + target: { source: "local", sha: "local123", repo: "frontend" }, + local: { + sessionId: "session-1", + taskId: "task-1", + agentctlReady: true, + }, + }); + + expect(result).toEqual({ source: "local", success: true, files }); + expect(mocks.requestCommitDiff).toHaveBeenCalledWith({ + sessionId: "session-1", + taskId: "task-1", + commitSha: "local123", + agentctlReady: true, + repo: "frontend", + }); + expect(mocks.request).not.toHaveBeenCalled(); + }); + + it("does not fall back to the local action when GitHub fails", async () => { + mocks.request.mockRejectedValue(new Error("GitHub unavailable")); + + await expect( + requestCommitDetail({ + target: remoteTarget, + local: { + sessionId: "session-1", + taskId: "task-1", + agentctlReady: true, + }, + }), + ).rejects.toThrow("GitHub unavailable"); + expect(mocks.requestCommitDiff).not.toHaveBeenCalled(); + }); + + it("rejects when the WebSocket client is unavailable", async () => { + websocketClient = null; + + await expect( + requestCommitDetail({ + target: remoteTarget, + }), + ).rejects.toThrow(); + }); + + it("rejects a GitHub response without a commit payload", async () => { + mocks.request.mockResolvedValue({}); + + await expect( + requestCommitDetail({ + target: remoteTarget, + }), + ).rejects.toThrow(); + }); + + it("rejects a present but incomplete GitHub commit payload", async () => { + mocks.request.mockResolvedValue({ commit: { files: [] } }); + + await expect( + requestCommitDetail({ + target: remoteTarget, + }), + ).rejects.toThrow(); + }); +}); + +describe("mapGitHubCommitFiles", () => { + it("maps patch and patchless files without introducing a local base ref", () => { + expect(mapGitHubCommitFiles(detail)).toEqual({ + "src/new.ts": { + path: "src/new.ts", + status: "added", + staged: false, + additions: 3, + deletions: 0, + diff: "@@ -0,0 +1 @@\n+new", + }, + "assets/logo.bin": { + path: "assets/logo.bin", + status: "modified", + staged: false, + additions: 0, + deletions: 1, + diff: "", + }, + }); + }); +}); diff --git a/apps/web/components/task/commit-detail-request.ts b/apps/web/components/task/commit-detail-request.ts new file mode 100644 index 0000000000..68051694f4 --- /dev/null +++ b/apps/web/components/task/commit-detail-request.ts @@ -0,0 +1,131 @@ +"use client"; + +import type { FileInfo } from "@/lib/state/store"; +import type { PRCommitDetail } from "@/lib/types/github"; +import { normalizeFileChangeStatus } from "@/lib/utils/file-change-status"; +import { getWebSocketClient } from "@/lib/ws/connection"; +import type { CommitDetailTarget } from "./changes-diff-target"; +import { requestCommitDiff, type CommitDiffResponse } from "./commit-diff-request"; + +export type CommitDetailRequestResult = + | ({ source: "local" } & CommitDiffResponse) + | { + source: "github"; + success: boolean; + commit?: PRCommitDetail; + files?: Record; + }; + +export class CommitDetailProtocolError extends Error { + constructor(reason: "client_unavailable" | "invalid_response") { + super(reason); + this.name = "CommitDetailProtocolError"; + } +} + +function isNonEmptyString(value: unknown): value is string { + return typeof value === "string" && value.length > 0; +} + +function isInteger(value: unknown): value is number { + return typeof value === "number" && Number.isInteger(value); +} + +function isOptionalString(value: unknown): boolean { + return value === undefined || typeof value === "string"; +} + +function isCompleteGitHubFile(value: unknown): value is PRCommitDetail["files"][number] { + if (!value || typeof value !== "object") return false; + const file = value as Record; + return ( + isNonEmptyString(file.filename) && + isNonEmptyString(file.status) && + isInteger(file.additions) && + isInteger(file.deletions) && + isOptionalString(file.old_path) && + isOptionalString(file.patch) + ); +} + +function isCompleteGitHubCommitDetail(value: unknown): value is PRCommitDetail { + if (!value || typeof value !== "object") return false; + const commit = value as Record; + return ( + isNonEmptyString(commit.sha) && + typeof commit.message === "string" && + typeof commit.author_login === "string" && + isNonEmptyString(commit.author_name) && + isNonEmptyString(commit.author_date) && + isInteger(commit.additions) && + isInteger(commit.deletions) && + isInteger(commit.files_changed) && + Array.isArray(commit.files) && + commit.files.every(isCompleteGitHubFile) + ); +} + +function mapGitHubFile(file: PRCommitDetail["files"][number]): FileInfo { + return { + path: file.filename, + status: normalizeFileChangeStatus(file.status), + staged: false, + additions: file.additions, + deletions: file.deletions, + old_path: file.old_path, + diff: file.patch ?? "", + }; +} + +export function mapGitHubCommitFiles(detail: PRCommitDetail): Record { + return Object.fromEntries(detail.files.map((file) => [file.filename, mapGitHubFile(file)])); +} + +async function requestGitHubCommitDetail( + target: Extract, +): Promise { + const ws = getWebSocketClient(); + if (!ws) throw new CommitDetailProtocolError("client_unavailable"); + + const response = await ws.request<{ commit?: PRCommitDetail }>( + "github.pr_commit.get", + { + workspace_id: target.workspaceId, + owner: target.owner, + repo: target.repo, + sha: target.sha, + }, + 10000, + ); + if (!isCompleteGitHubCommitDetail(response?.commit)) { + throw new CommitDetailProtocolError("invalid_response"); + } + return { + source: "github", + success: true, + commit: response.commit, + files: mapGitHubCommitFiles(response.commit), + }; +} + +export async function requestCommitDetail(params: { + target: CommitDetailTarget; + local?: { + sessionId: string; + taskId: string | null; + agentctlReady: boolean; + }; +}): Promise { + if (params.target.source === "github") { + return requestGitHubCommitDetail(params.target); + } + + if (!params.local) throw new CommitDetailProtocolError("client_unavailable"); + const response = await requestCommitDiff({ + ...params.local, + commitSha: params.target.sha, + repo: params.target.repo, + }); + if (!response) throw new CommitDetailProtocolError("client_unavailable"); + return { source: "local", ...response }; +} diff --git a/apps/web/components/task/commit-row.test.tsx b/apps/web/components/task/commit-row.test.tsx new file mode 100644 index 0000000000..ad045a954c --- /dev/null +++ b/apps/web/components/task/commit-row.test.tsx @@ -0,0 +1,76 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { CommitRow, type CommitItem } from "./commit-row"; + +vi.mock("@kandev/ui/context-menu", () => ({ + ContextMenu: ({ children }: { children: React.ReactNode }) => <>{children}, + ContextMenuContent: ({ children }: { children: React.ReactNode }) =>
{children}
, + ContextMenuItem: ({ + children, + onSelect, + }: { + children: React.ReactNode; + onSelect: () => void; + }) => ( + + ), + ContextMenuTrigger: ({ children }: { children: React.ReactNode }) => <>{children}, +})); + +vi.mock("@kandev/ui/tooltip", () => ({ + Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: React.ReactNode }) => <>{children}, +})); + +afterEach(cleanup); + +function remoteCommit(): CommitItem { + return { + commit_sha: "remote123456", + commit_message: "Remote contribution", + insertions: 0, + deletions: 0, + pushed: true, + statsAvailable: false, + detailTarget: { + source: "github", + sha: "remote123456", + workspaceId: "workspace-1", + owner: "acme", + repo: "widget", + }, + }; +} + +describe("CommitRow", () => { + it("hides unknown statistics and local mutation actions for remote commits", () => { + render( + , + ); + + expect(screen.getByText("Remote contribution")).toBeTruthy(); + expect(screen.queryByText("+0")).toBeNull(); + expect(screen.queryByText("-0")).toBeNull(); + expect(screen.queryByText("Amend message")).toBeNull(); + expect(screen.queryByText("Revert commit")).toBeNull(); + expect(screen.queryByText("Reset to this commit")).toBeNull(); + }); + + it("passes the full source target when a remote row is opened", () => { + const onOpenCommitDetail = vi.fn(); + const commit = remoteCommit(); + render(); + + fireEvent.click(screen.getByTestId("commit-row-remote1")); + expect(onOpenCommitDetail).toHaveBeenCalledWith(commit.detailTarget); + }); +}); diff --git a/apps/web/components/task/commit-row.tsx b/apps/web/components/task/commit-row.tsx index 38cf9e8ed4..1bfeca7496 100644 --- a/apps/web/components/task/commit-row.tsx +++ b/apps/web/components/task/commit-row.tsx @@ -16,6 +16,7 @@ import { ContextMenuTrigger, } from "@kandev/ui/context-menu"; import { timeAgo } from "@/lib/utils/time"; +import type { CommitDetailTarget } from "./changes-diff-target"; export type CommitItem = { commit_sha: string; @@ -23,6 +24,8 @@ export type CommitItem = { insertions: number; deletions: number; pushed?: boolean; + statsAvailable: boolean; + detailTarget: CommitDetailTarget; /** Multi-repo: name of the repo this commit was made in. Empty for single-repo. */ repository_name?: string; committed_at?: string; @@ -47,7 +50,8 @@ function CommitContextMenu({ onRevertCommit?: (sha: string, repo?: string) => void; onResetToCommit?: (sha: string, repo?: string) => void; }) { - const hasActions = onAmendCommit || onRevertCommit || onResetToCommit; + const hasActions = + commit.detailTarget.source === "local" && (onAmendCommit || onRevertCommit || onResetToCommit); if (!hasActions) { return <>{children}; @@ -177,7 +181,7 @@ export function CommitRow({ // Multi-repo: opening the diff for a non-primary repo's commit needs the // repo subpath, otherwise the agentctl looks up the SHA at the workspace // root and finds nothing (each repo has its own commit graph). - onOpenCommitDetail?: (sha: string, repo?: string) => void; + onOpenCommitDetail?: (target: CommitDetailTarget) => void; // Multi-repo: handlers receive the commit's repository_name so the // amend/revert/reset op runs in the right git repo. Without it, ops hit // the workspace root which fails on multi-repo task workspaces. @@ -185,7 +189,9 @@ export function CommitRow({ onRevertCommit?: (sha: string, repo?: string) => void; onResetToCommit?: (sha: string, repo?: string) => void; }) { - const showActions = onResetToCommit || (isLatest && (onAmendCommit || onRevertCommit)); + const isLocalCommit = commit.detailTarget.source === "local"; + const showActions = + isLocalCommit && (onResetToCommit || (isLatest && (onAmendCommit || onRevertCommit))); return ( onOpenCommitDetail?.(commit.commit_sha, commit.repository_name)} + onClick={() => onOpenCommitDetail?.(commit.detailTarget)} onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); - onOpenCommitDetail?.(commit.commit_sha, commit.repository_name); + onOpenCommitDetail?.(commit.detailTarget); } }} > @@ -225,12 +231,14 @@ export function CommitRow({ {commit.commit_sha.slice(0, 7)} {commit.commit_message} - - +{commit.insertions}{" "} - -{commit.deletions} - + {commit.statsAvailable && ( + + +{commit.insertions}{" "} + -{commit.deletions} + + )} {commit.committed_at && ( {timeAgo(commit.committed_at)} diff --git a/apps/web/components/task/dockview-panel-content.tsx b/apps/web/components/task/dockview-panel-content.tsx index 750a0f2eee..02ffafb981 100644 --- a/apps/web/components/task/dockview-panel-content.tsx +++ b/apps/web/components/task/dockview-panel-content.tsx @@ -13,7 +13,7 @@ import { t } from "@/lib/i18n"; import { setPanelTitle } from "@/lib/layout/panel-portal-manager"; import { useDockviewStore } from "@/lib/state/dockview-store"; import { BrowserPanel } from "./browser-panel"; -import type { OpenDiffOptions } from "./changes-diff-target"; +import type { CommitDetailTarget, OpenDiffOptions } from "./changes-diff-target"; import { ChangesPanel } from "./changes-panel"; import { CommitDetailPanel } from "./commit-detail-panel"; import { FileEditorPanel } from "./file-editor-panel"; @@ -154,7 +154,7 @@ function ChangesContent({ panelId }: { panelId: string }) { [addFileDiffPanel], ); const handleOpenCommitDetail = useCallback( - (sha: string, repo?: string) => addCommitDetailPanel(sha, { repo }), + (target: CommitDetailTarget) => addCommitDetailPanel(target), [addCommitDetailPanel], ); const handleOpenDiffAll = useCallback(() => addDiffViewerPanel(), [addDiffViewerPanel]); diff --git a/apps/web/components/task/dockview-shared.tsx b/apps/web/components/task/dockview-shared.tsx index 889ad639ea..347273bac9 100644 --- a/apps/web/components/task/dockview-shared.tsx +++ b/apps/web/components/task/dockview-shared.tsx @@ -35,7 +35,7 @@ import { TerminalPanel } from "./terminal-panel"; import { BrowserPanel } from "./browser-panel"; import { VscodePanel } from "./vscode-panel"; import { CommitDetailPanel } from "./commit-detail-panel"; -import type { OpenDiffOptions } from "./changes-diff-target"; +import type { CommitDetailTarget, OpenDiffOptions } from "./changes-diff-target"; import { ReviewDetailPanelComponent } from "./review-detail-panel"; import { MRDetailPanelComponent } from "@/components/gitlab/mr-detail-panel"; import { PluginTaskPanel } from "./plugin-task-panel"; @@ -332,7 +332,7 @@ function ChangesContent({ panelId }: { panelId: string }) { [addFileDiffPanel], ); const handleOpenCommitDetail = useCallback( - (sha: string, repo?: string) => addCommitDetailPanel(sha, { repo }), + (target: CommitDetailTarget) => addCommitDetailPanel(target), [addCommitDetailPanel], ); const handleOpenDiffAll = useCallback(() => addDiffViewerPanel(), [addDiffViewerPanel]); diff --git a/apps/web/components/task/mobile/mobile-changes-panel.tsx b/apps/web/components/task/mobile/mobile-changes-panel.tsx index 37d46b103b..1758791851 100644 --- a/apps/web/components/task/mobile/mobile-changes-panel.tsx +++ b/apps/web/components/task/mobile/mobile-changes-panel.tsx @@ -86,8 +86,8 @@ export const MobileChangesPanel = memo(function MobileChangesPanel({ const bodyProps = buildChangesPanelBodyProps(data, { onOpenDiffFile: handleOpenDiffFile, onEditFile: onOpenFile ?? (() => {}), - onOpenCommitDetail: (sha, repo) => { - setDiffSheet({ kind: "commit", sha, repo }); + onOpenCommitDetail: (target) => { + setDiffSheet({ kind: "commit", target }); }, onOpenReview: handleOpenReview, }); diff --git a/apps/web/components/task/mobile/mobile-diff-sheet.tsx b/apps/web/components/task/mobile/mobile-diff-sheet.tsx index 3f8311bd50..4300a2c4cf 100644 --- a/apps/web/components/task/mobile/mobile-diff-sheet.tsx +++ b/apps/web/components/task/mobile/mobile-diff-sheet.tsx @@ -161,7 +161,7 @@ function renderPanel( ): React.ReactNode { if (!mode) return null; if (mode.kind === "commit") { - return ; + return ; } const panelMode = mode.kind; const filePath = mode.kind === "file" ? mode.path : undefined; diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index a122eafd96..06ebf40803 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -10,7 +10,11 @@ import type { } from "../../lib/types/http"; import type { Agent, AgentProfile } from "../../lib/types/http-agents"; import { normalizeAgentProfile } from "../../lib/api/domains/agent-profile-normalize"; -import type { TaskCIAutomationOptions, TaskCIAutomationPatch } from "../../lib/types/github"; +import type { + PRCommitDetail, + TaskCIAutomationOptions, + TaskCIAutomationPatch, +} from "../../lib/types/github"; import type { VoiceModeSettings } from "../../lib/types/http-voice"; import type { TaskStatusSummary } from "../../lib/types/task-status-summary"; import type { @@ -1319,6 +1323,7 @@ export class ApiClient { message: string; author_login: string; author_date: string; + stats_available?: boolean; }>, ): Promise { await this.request("POST", "/api/v1/github/mock/commits", { @@ -1339,6 +1344,28 @@ export class ApiClient { }); } + async mockGitHubAddPRCommitDetail( + owner: string, + repo: string, + sha: string, + detail: Omit & { sha?: string }, + ): Promise { + await this.request("POST", "/api/v1/github/mock/commit-details", { + owner, + repo, + sha, + detail: { ...detail, sha: detail.sha ?? sha }, + }); + await this.seedMockGitHubRepositoryAccess([{ repo_owner: owner, repo_name: repo }]); + + const workspaceId = await this.activeWorkspaceId(); + if (!workspaceId) return; + await this.mockGitHubSetWorkspaceConnection(workspaceId, { + source: "legacy_shared", + status: "active", + }); + } + async mockGitHubAddBranches( owner: string, repo: string, diff --git a/apps/web/e2e/tests/git/git-changes-panel.spec.ts b/apps/web/e2e/tests/git/git-changes-panel.spec.ts index a235808b9f..00ccff1613 100644 --- a/apps/web/e2e/tests/git/git-changes-panel.spec.ts +++ b/apps/web/e2e/tests/git/git-changes-panel.spec.ts @@ -401,6 +401,105 @@ test.describe("Git Changes Panel", () => { await expect(testPage.getByText("line 1")).toBeVisible({ timeout: 5_000 }); }); + test("PR-only commit uses GitHub details when local history is stale", async ({ + testPage, + apiClient, + seedData, + }) => { + const profile = await createStandardProfile(apiClient, "Git PR-only Detail Profile"); + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Git PR-only Detail Test", + profile.id, + { + description: "Testing PR-only commit details after a force-push", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + + const remoteSha = "d".repeat(40); + const remoteMessage = "Force-pushed remote commit"; + await apiClient.mockGitHubReset(); + await apiClient.mockGitHubSetUser("remote-author"); + await apiClient.mockGitHubAddPRs([ + { + number: 2253, + title: "Force-pushed PR", + state: "open", + head_branch: "feature/stale-worktree", + base_branch: "main", + author_login: "remote-author", + repo_owner: "testorg", + repo_name: "testrepo", + }, + ]); + await apiClient.mockGitHubAddPRCommits("testorg", "testrepo", 2253, [ + { + sha: remoteSha, + message: remoteMessage, + author_login: "remote-author", + author_date: "2026-08-04T12:00:00Z", + stats_available: false, + }, + ]); + await apiClient.mockGitHubAddPRCommitDetail("testorg", "testrepo", remoteSha, { + message: remoteMessage, + author_login: "remote-author", + author_name: "Remote Author", + author_date: "2026-08-04T12:00:00Z", + additions: 1, + deletions: 0, + files_changed: 1, + files: [ + { + filename: "pr-only-marker.ts", + status: "added", + additions: 1, + deletions: 0, + patch: "@@ -0,0 +1 @@\n+PR_ONLY_REMOTE_MARKER", + }, + ], + }); + await apiClient.mockGitHubAssociateTaskPR({ + task_id: task.id, + owner: "testorg", + repo: "testrepo", + pr_number: 2253, + pr_url: "https://github.com/testorg/testrepo/pull/2253", + pr_title: "Force-pushed PR", + head_branch: "feature/stale-worktree", + base_branch: "main", + author_login: "remote-author", + }); + + const session = await openTaskSession(testPage, "Git PR-only Detail Test"); + await session.clickTab("Changes"); + await expect(session.changes).toBeVisible({ timeout: 10_000 }); + await expect(testPage.getByTestId("commits-section")).toBeVisible({ timeout: 20_000 }); + await session.expandCommitsSection(); + + const row = testPage.getByTestId(`commit-row-${remoteSha.slice(0, 7)}`); + await expect(row).toBeVisible({ timeout: 20_000 }); + await expect(row.getByText("+0", { exact: true })).toHaveCount(0); + await expect(row.getByText("-0", { exact: true })).toHaveCount(0); + await row.hover(); + await expect(row.getByRole("button")).toHaveCount(0); + + await row.click(); + await expect(testPage.getByText(remoteMessage).last()).toBeVisible({ timeout: 15_000 }); + await expect(testPage.getByText("Remote Author")).toBeVisible({ timeout: 10_000 }); + await testPage.waitForFunction( + (marker: string) => + Array.from(document.querySelectorAll("diffs-container")).some((container) => + container.shadowRoot?.textContent?.includes(marker), + ), + "PR_ONLY_REMOTE_MARKER", + { timeout: 60_000 }, + ); + }); + /** * Verifies that reverting a commit undoes it (soft reset). * Note: The "Revert commit" action does `git reset --soft HEAD~1`, diff --git a/apps/web/e2e/tests/task/mobile-changes-panel.spec.ts b/apps/web/e2e/tests/task/mobile-changes-panel.spec.ts index 18d0d3abc6..6355ca267d 100644 --- a/apps/web/e2e/tests/task/mobile-changes-panel.spec.ts +++ b/apps/web/e2e/tests/task/mobile-changes-panel.spec.ts @@ -254,6 +254,106 @@ test.describe("Mobile changes panel", () => { await testPage.getByTestId("mobile-diff-sheet-close").tap(); }); + test("PR-only commit opens the remote commit sheet when local history is stale", async ({ + testPage, + apiClient, + seedData, + }) => { + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Mobile PR-only Commit Detail", + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + + const remoteSha = "e".repeat(40); + const remoteMessage = "Mobile force-pushed commit"; + await apiClient.mockGitHubReset(); + await apiClient.mockGitHubSetUser("mobile-remote-author"); + await apiClient.mockGitHubAddPRs([ + { + number: 2254, + title: "Mobile force-pushed PR", + state: "open", + head_branch: "feature/mobile-stale", + base_branch: "main", + author_login: "mobile-remote-author", + repo_owner: "testorg", + repo_name: "testrepo", + }, + ]); + await apiClient.mockGitHubAddPRCommits("testorg", "testrepo", 2254, [ + { + sha: remoteSha, + message: remoteMessage, + author_login: "mobile-remote-author", + author_date: "2026-08-04T12:00:00Z", + stats_available: false, + }, + ]); + await apiClient.mockGitHubAddPRCommitDetail("testorg", "testrepo", remoteSha, { + message: remoteMessage, + author_login: "mobile-remote-author", + author_name: "Mobile Remote Author", + author_date: "2026-08-04T12:00:00Z", + additions: 1, + deletions: 0, + files_changed: 1, + files: [ + { + filename: "mobile-pr-only.ts", + status: "added", + additions: 1, + deletions: 0, + patch: "@@ -0,0 +1 @@\n+MOBILE_PR_ONLY_REMOTE_MARKER", + }, + ], + }); + await apiClient.mockGitHubAssociateTaskPR({ + task_id: task.id, + owner: "testorg", + repo: "testrepo", + pr_number: 2254, + pr_url: "https://github.com/testorg/testrepo/pull/2254", + pr_title: "Mobile force-pushed PR", + head_branch: "feature/mobile-stale", + base_branch: "main", + author_login: "mobile-remote-author", + }); + + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.waitForChatIdle({ timeout: 45_000 }); + await openMobileChangesPanel(testPage); + await expandSection(testPage, "commits-section"); + + const row = testPage.getByTestId(`commit-row-${remoteSha.slice(0, 7)}`); + await expect(row).toBeVisible({ timeout: 20_000 }); + await expect(row.getByText("+0", { exact: true })).toHaveCount(0); + await expect(row.getByText("-0", { exact: true })).toHaveCount(0); + await row.tap(); + + await expect(testPage.getByText("Commit Changes")).toBeVisible({ timeout: 10_000 }); + await expect(testPage.getByLabel("Commit Changes").getByText(remoteMessage)).toBeVisible({ + timeout: 15_000, + }); + await expect(testPage.getByText("Mobile Remote Author")).toBeVisible({ timeout: 10_000 }); + await expectDiffText(testPage, "MOBILE_PR_ONLY_REMOTE_MARKER"); + await expect(testPage.getByTestId("mobile-diff-sheet-close")).toBeVisible(); + await expect(testPage.getByTestId("mobile-diff-sheet-close")).toHaveCount(1); + await expect(testPage.locator("body")).toHaveCSS("overflow-x", "hidden"); + await testPage.getByTestId("mobile-diff-sheet-close").tap(); + await expect(testPage.getByTestId("mobile-diff-sheet-close")).not.toBeVisible({ + timeout: 10_000, + }); + }); + test("tapping PR file row shows PR diff when same file also has local changes", async ({ testPage, apiClient, diff --git a/apps/web/hooks/domains/github/use-pr-commits.test.ts b/apps/web/hooks/domains/github/use-pr-commits.test.ts new file mode 100644 index 0000000000..350f3f06ae --- /dev/null +++ b/apps/web/hooks/domains/github/use-pr-commits.test.ts @@ -0,0 +1,93 @@ +import { act, renderHook, waitFor } from "@testing-library/react"; +import { createElement, type ReactNode } from "react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { StateProvider } from "@/components/state-provider"; +import type { AppState } from "@/lib/state/store"; +import type { PRCommitInfo } from "@/lib/types/github"; +import { resolvePRCommitsView, usePRCommits, type KeyedPRCommitsState } from "./use-pr-commits"; + +const requestMock = vi.hoisted(() => vi.fn()); +let websocketClient: { request: typeof requestMock } | null = { request: requestMock }; +vi.mock("@/lib/ws/connection", () => ({ + getWebSocketClient: () => websocketClient, +})); + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise; + }); + return { promise, resolve }; +} + +beforeEach(() => { + requestMock.mockReset(); + websocketClient = { request: requestMock }; +}); + +function wrapper({ children }: { children: ReactNode }) { + const initialState = { + workspaces: { activeId: "workspace-1" }, + } as unknown as Partial; + return createElement(StateProvider, { initialState, children }); +} + +function commit(sha: string): PRCommitInfo { + return { + sha, + message: sha, + author_login: "octocat", + author_date: "2026-08-04T12:00:00Z", + additions: 1, + deletions: 0, + files_changed: 1, + stats_available: false, + }; +} + +describe("usePRCommits request ownership", () => { + it("masks state unless the complete workspace and PR source key matches", () => { + const staleState: KeyedPRCommitsState = { + sourceKey: "workspace-1/acme/app/1/old-refresh", + commits: [commit("first-sha")], + loading: false, + error: null, + }; + + expect(resolvePRCommitsView(staleState, "workspace-1/acme/other-app/2/new-refresh")).toEqual({ + commits: [], + loading: true, + error: null, + }); + }); + + it("masks the previous PR while a rapid switch is loading", async () => { + const first = deferred<{ commits: PRCommitInfo[] }>(); + const second = deferred<{ commits: PRCommitInfo[] }>(); + requestMock.mockImplementation((_action: string, payload: { number: number }) => + payload.number === 1 ? first.promise : second.promise, + ); + + const { result, rerender } = renderHook( + ({ number }) => usePRCommits("acme", "app", number, "synced"), + { initialProps: { number: 1 }, wrapper }, + ); + + await waitFor(() => expect(requestMock).toHaveBeenCalledTimes(1)); + rerender({ number: 2 }); + + await waitFor(() => expect(requestMock).toHaveBeenCalledTimes(2)); + await act(async () => { + second.resolve({ commits: [commit("second-sha")] }); + await second.promise; + }); + await waitFor(() => expect(result.current.commits[0]?.sha).toBe("second-sha")); + + await act(async () => { + first.resolve({ commits: [commit("late-first-sha")] }); + await first.promise; + }); + + expect(result.current.commits[0]?.sha).toBe("second-sha"); + }); +}); diff --git a/apps/web/hooks/domains/github/use-pr-commits.ts b/apps/web/hooks/domains/github/use-pr-commits.ts index 52327e77a5..b971582397 100644 --- a/apps/web/hooks/domains/github/use-pr-commits.ts +++ b/apps/web/hooks/domains/github/use-pr-commits.ts @@ -10,25 +10,43 @@ type PRCommitsState = { loading: boolean; error: string | null; }; -type WorkspacePRCommitsState = { workspaceId: string | null; state: PRCommitsState }; +export type KeyedPRCommitsState = PRCommitsState & { sourceKey: string }; -const INITIAL_STATE: PRCommitsState = { +const INITIAL_STATE: KeyedPRCommitsState = { + sourceKey: "", commits: [], loading: false, error: null, }; +export function resolvePRCommitsView( + state: KeyedPRCommitsState, + requestedKey: string, +): PRCommitsState { + if (state.sourceKey === requestedKey) { + return { commits: state.commits, loading: state.loading, error: state.error }; + } + return { commits: [], loading: requestedKey !== "", error: null }; +} + +type PRCommitsRequest = { + workspaceId: string; + owner: string; + repo: string; + prNumber: number; + sourceKey: string; +}; + async function fetchPRCommits( - workspaceId: string, - owner: string, - repo: string, - prNumber: number, - setState: (s: PRCommitsState) => void, + { workspaceId, owner, repo, prNumber, sourceKey }: PRCommitsRequest, + setState: (s: KeyedPRCommitsState) => void, ) { const client = getWebSocketClient(); - if (!client) return; - - setState({ commits: [], loading: true, error: null }); + setState({ sourceKey, commits: [], loading: true, error: null }); + if (!client) { + setState({ sourceKey, commits: [], loading: false, error: null }); + return; + } try { const response = await client.request<{ commits?: PRCommitInfo[] }>("github.pr_commits.get", { workspace_id: workspaceId, @@ -36,9 +54,10 @@ async function fetchPRCommits( repo, number: prNumber, }); - setState({ commits: response?.commits ?? [], loading: false, error: null }); + setState({ sourceKey, commits: response?.commits ?? [], loading: false, error: null }); } catch (err) { setState({ + sourceKey, commits: [], loading: false, error: err instanceof Error ? err.message : "Failed to fetch PR commits", @@ -57,41 +76,36 @@ export function usePRCommits( refreshKey?: string | null, ) { const workspaceId = useAppStore((s) => s.workspaces.activeId); - const [result, setResult] = useState({ - workspaceId: null, - state: INITIAL_STATE, - }); const hasParams = !!workspaceId && !!owner && !!repo && !!prNumber; + const sourceKey = hasParams + ? `${workspaceId}/${owner}/${repo}/${prNumber}/${refreshKey ?? ""}` + : ""; + const [state, setState] = useState(INITIAL_STATE); const paramsKeyRef = useRef(""); const requestIdRef = useRef(0); const refresh = useCallback(() => { if (!workspaceId || !owner || !repo || !prNumber) return; const requestId = ++requestIdRef.current; - void fetchPRCommits(workspaceId, owner, repo, prNumber, (next) => { + void fetchPRCommits({ workspaceId, owner, repo, prNumber, sourceKey }, (next) => { if (requestId !== requestIdRef.current) return; - setResult({ workspaceId, state: next }); + setState(next); }); - }, [workspaceId, owner, repo, prNumber]); + }, [workspaceId, owner, repo, prNumber, sourceKey]); useEffect(() => { - const key = hasParams ? `${workspaceId}/${owner}/${repo}/${prNumber}/${refreshKey ?? ""}` : ""; - if (key === paramsKeyRef.current) return; - paramsKeyRef.current = key; + if (sourceKey === paramsKeyRef.current) return; + paramsKeyRef.current = sourceKey; if (!workspaceId || !owner || !repo || !prNumber) { requestIdRef.current++; // invalidate in-flight responses return; } const requestId = ++requestIdRef.current; - void fetchPRCommits(workspaceId, owner, repo, prNumber, (next) => { + void fetchPRCommits({ workspaceId, owner, repo, prNumber, sourceKey }, (next) => { if (requestId !== requestIdRef.current) return; - setResult({ workspaceId, state: next }); + setState(next); }); - }, [workspaceId, owner, repo, prNumber, hasParams, refreshKey]); + }, [workspaceId, owner, repo, prNumber, sourceKey]); - // Return initial state when params are null to clear stale data - if (!hasParams || result.workspaceId !== workspaceId) { - return { ...INITIAL_STATE, refresh }; - } - return { ...result.state, refresh }; + return { ...resolvePRCommitsView(state, sourceKey), refresh }; } diff --git a/apps/web/hooks/domains/session/use-commit-detail.test.ts b/apps/web/hooks/domains/session/use-commit-detail.test.ts new file mode 100644 index 0000000000..e51d1dc041 --- /dev/null +++ b/apps/web/hooks/domains/session/use-commit-detail.test.ts @@ -0,0 +1,209 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; +import type { FileInfo } from "@/lib/state/store"; + +const mocks = vi.hoisted(() => ({ + requestCommitDetail: vi.fn(), + toast: vi.fn(), + state: { + tasks: { activeSessionId: "session-1", activeTaskId: "task-1" }, + taskSessions: { items: { "session-1": { task_id: "task-1" } } }, + sessionAgentctl: { itemsBySessionId: { "session-1": { status: "ready" } } }, + }, +})); + +vi.mock("@/components/state-provider", () => ({ + useAppStore: (selector: (state: unknown) => unknown) => selector(mocks.state), +})); + +vi.mock("@/components/toast-provider", () => ({ + useToast: () => ({ toast: mocks.toast }), +})); + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock("@/components/task/commit-detail-request", () => ({ + CommitDetailProtocolError: class CommitDetailProtocolError extends Error {}, + requestCommitDetail: mocks.requestCommitDetail, +})); + +import { useCommitDetail } from "./use-commit-detail"; + +const remoteFiles: Record = { + "src/remote.ts": { + path: "src/remote.ts", + status: "modified", + staged: false, + diff: "remote patch", + }, +}; +const remoteTarget = { + source: "github" as const, + sha: "remote123", + workspaceId: "workspace-1", + owner: "acme", + repo: "widget", +}; +const nextRemoteTarget = { ...remoteTarget, sha: "remote456" }; + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise; + }); + return { promise, resolve }; +} + +afterEach(() => { + cleanup(); + mocks.requestCommitDetail.mockReset(); + mocks.toast.mockReset(); + mocks.state.sessionAgentctl.itemsBySessionId["session-1"].status = "ready"; +}); + +describe("useCommitDetail", () => { + it("requests a GitHub target without adding local session routing", async () => { + mocks.requestCommitDetail.mockResolvedValue({ + source: "github", + success: true, + files: remoteFiles, + commit: { + sha: "remote123", + message: "remote commit", + author_login: "octocat", + author_name: "Octo Cat", + author_date: "2026-08-04T12:00:00Z", + additions: 1, + deletions: 0, + files_changed: 1, + files: [], + }, + }); + + const { result } = renderHook(() => useCommitDetail(remoteTarget)); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(mocks.requestCommitDetail).toHaveBeenCalledWith({ target: remoteTarget }); + expect(result.current.files).toEqual(remoteFiles); + expect(result.current.commit?.message).toBe("remote commit"); + }); + + it("passes local session readiness only for a local target", async () => { + mocks.requestCommitDetail.mockResolvedValue({ + source: "local", + success: true, + files: remoteFiles, + }); + + const target = { source: "local" as const, sha: "local123", repo: "frontend" }; + const { result } = renderHook(() => useCommitDetail(target)); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(mocks.requestCommitDetail).toHaveBeenCalledWith({ + target, + local: { + sessionId: "session-1", + taskId: "task-1", + agentctlReady: true, + }, + }); + }); + + it("keeps a GitHub error on the remote path and exposes it to the panel", async () => { + mocks.requestCommitDetail.mockRejectedValue(new Error("GitHub unavailable")); + + const { result } = renderHook(() => useCommitDetail(remoteTarget)); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.files).toBeNull(); + expect(result.current.error).toBe("GitHub unavailable"); + expect(mocks.toast).toHaveBeenCalled(); + expect(mocks.requestCommitDetail).toHaveBeenCalledTimes(1); + }); + + it("turns a malformed remote response into a persistent error", async () => { + mocks.requestCommitDetail.mockResolvedValue({ source: "github", success: false }); + + const { result } = renderHook(() => useCommitDetail(remoteTarget)); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.error).toBe("unexpectedResponseFromTheServer"); + expect(result.current.files).toBeNull(); + expect(mocks.toast).toHaveBeenCalled(); + }); + + it("does not refetch a GitHub target when local readiness changes", async () => { + mocks.requestCommitDetail.mockResolvedValue({ + source: "github", + success: true, + files: remoteFiles, + }); + + const { result, rerender } = renderHook(() => useCommitDetail(remoteTarget)); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(mocks.requestCommitDetail).toHaveBeenCalledTimes(1); + + mocks.state.sessionAgentctl.itemsBySessionId["session-1"].status = "starting"; + rerender(); + await waitFor(() => expect(result.current.loading).toBe(false)); + + expect(mocks.requestCommitDetail).toHaveBeenCalledTimes(1); + }); + + it("refetches the same target on demand", async () => { + mocks.requestCommitDetail.mockResolvedValue({ source: "github", success: false }); + const { result } = renderHook(() => useCommitDetail(remoteTarget)); + await waitFor(() => expect(result.current.loading).toBe(false)); + + await act(async () => { + await result.current.refetch(); + }); + expect(mocks.requestCommitDetail).toHaveBeenCalledTimes(2); + }); +}); + +describe("useCommitDetail target switching", () => { + it("hides the previous detail while switching targets", async () => { + const first = deferred<{ + source: "github"; + success: true; + files: Record; + commit: { sha: string; message: string }; + }>(); + const second = deferred<{ + source: "github"; + success: true; + files: Record; + commit: { sha: string; message: string }; + }>(); + mocks.requestCommitDetail.mockImplementation(({ target }: { target: { sha: string } }) => + target.sha === remoteTarget.sha ? first.promise : second.promise, + ); + + const { result, rerender } = renderHook(({ target }) => useCommitDetail(target), { + initialProps: { target: remoteTarget }, + }); + + await waitFor(() => expect(mocks.requestCommitDetail).toHaveBeenCalledTimes(1)); + await act(async () => { + first.resolve({ + source: "github", + success: true, + files: remoteFiles, + commit: { sha: remoteTarget.sha, message: "first commit" }, + }); + await first.promise; + }); + await waitFor(() => expect(result.current.commit?.message).toBe("first commit")); + + rerender({ target: nextRemoteTarget }); + + expect(result.current.files).toBeNull(); + expect(result.current.commit).toBeNull(); + expect(result.current.error).toBeNull(); + expect(result.current.loading).toBe(true); + }); +}); diff --git a/apps/web/hooks/domains/session/use-commit-detail.ts b/apps/web/hooks/domains/session/use-commit-detail.ts new file mode 100644 index 0000000000..57c642a47a --- /dev/null +++ b/apps/web/hooks/domains/session/use-commit-detail.ts @@ -0,0 +1,135 @@ +"use client"; + +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { useAppStore } from "@/components/state-provider"; +import { useToast } from "@/components/toast-provider"; +import type { PRCommitDetail } from "@/lib/types/github"; +import type { FileInfo } from "@/lib/state/store"; +import type { CommitDetailTarget } from "@/components/task/changes-diff-target"; +import { + CommitDetailProtocolError, + requestCommitDetail, +} from "@/components/task/commit-detail-request"; + +export type UseCommitDetailResult = { + files: Record | null; + commit: PRCommitDetail | null; + loading: boolean; + error: string | null; + refetch: () => Promise; +}; + +function targetKey(target: CommitDetailTarget): string { + if (target.source === "local") return `local:${target.repo ?? ""}:${target.sha}`; + return `github:${target.workspaceId}:${target.owner}/${target.repo}:${target.sha}`; +} + +function buildLocalRequest( + target: CommitDetailTarget, + activeSessionId: string | null, + sessionTaskId: string | undefined, + activeTaskId: string | null, + agentctlReady: boolean, +) { + if (target.source !== "local" || !activeSessionId) return undefined; + return { + sessionId: activeSessionId, + taskId: sessionTaskId ?? activeTaskId ?? null, + agentctlReady, + }; +} + +function detailErrorMessage(error: unknown, unexpectedError: string): string { + if (error instanceof CommitDetailProtocolError) return unexpectedError; + if (error instanceof Error) return error.message; + return unexpectedError; +} + +/** + * Loads commit details from the source encoded in the target. GitHub targets + * never use the local session/worktree request, including after an error. + */ +export function useCommitDetail(target: CommitDetailTarget): UseCommitDetailResult { + const activeSessionId = useAppStore((state) => state.tasks.activeSessionId); + const activeTaskId = useAppStore((state) => state.tasks.activeTaskId); + const sessionTaskId = useAppStore((state) => + activeSessionId ? state.taskSessions.items[activeSessionId]?.task_id : undefined, + ); + const agentctlReady = useAppStore((state) => + activeSessionId + ? state.sessionAgentctl.itemsBySessionId[activeSessionId]?.status === "ready" + : false, + ); + const { toast } = useToast(); + const { t } = useTranslation(); + const unexpectedError = t("unexpectedResponseFromTheServer", { ns: "github" }); + const requestFailed = t("requestFailed"); + const [files, setFiles] = useState | null>(null); + const [commit, setCommit] = useState(null); + const [loading, setLoading] = useState(false); + const [error, setError] = useState(null); + const [loadedKey, setLoadedKey] = useState(null); + const requestSeqRef = useRef(0); + const key = targetKey(target); + const stableTarget = useMemo(() => target, [key]); + const localRequest = useMemo( + () => + stableTarget.source === "local" + ? buildLocalRequest( + stableTarget, + activeSessionId, + sessionTaskId, + activeTaskId, + agentctlReady, + ) + : undefined, + [stableTarget, activeSessionId, sessionTaskId, activeTaskId, agentctlReady], + ); + + const fetchDetail = useCallback(async () => { + const requestSeq = ++requestSeqRef.current; + setLoading(true); + setError(null); + try { + const response = await requestCommitDetail({ + target: stableTarget, + ...(localRequest ? { local: localRequest } : {}), + }); + if (requestSeq !== requestSeqRef.current) return; + if (stableTarget.source === "github" && !response.success) { + throw new CommitDetailProtocolError("invalid_response"); + } + setLoadedKey(key); + setFiles(response.success && response.files ? response.files : null); + setCommit(response.source === "github" ? (response.commit ?? null) : null); + } catch (err) { + if (requestSeq !== requestSeqRef.current) return; + const message = detailErrorMessage(err, unexpectedError); + setLoadedKey(key); + setFiles(null); + setCommit(null); + setError(message); + toast({ + title: requestFailed, + description: message, + variant: "error", + }); + } finally { + if (requestSeq === requestSeqRef.current) setLoading(false); + } + }, [key, localRequest, requestFailed, stableTarget, toast, unexpectedError]); + + useEffect(() => { + void fetchDetail(); + }, [fetchDetail]); + + const isCurrentTargetLoaded = loadedKey === key; + return { + files: isCurrentTargetLoaded ? files : null, + commit: isCurrentTargetLoaded ? commit : null, + loading: loading || !isCurrentTargetLoaded, + error: isCurrentTargetLoaded ? error : null, + refetch: fetchDetail, + }; +} diff --git a/apps/web/lib/state/dockview-panel-actions-extra.test.ts b/apps/web/lib/state/dockview-panel-actions-extra.test.ts new file mode 100644 index 0000000000..ff23da3a99 --- /dev/null +++ b/apps/web/lib/state/dockview-panel-actions-extra.test.ts @@ -0,0 +1,269 @@ +import { describe, expect, it } from "vitest"; +import type { DockviewApi } from "dockview-react"; +import { buildExtraPanelActions } from "./dockview-panel-actions"; +import { CENTER_GROUP } from "./layout-manager"; +import { makeApi, makeStore } from "./dockview-panel-actions.test-utils"; +import type { MockPanel } from "./dockview-panel-actions.test-utils"; + +describe("addPRPanel — dedup with legacy auto-shown panel", () => { + const PR_KEY = "testorg/testrepo/101"; + const OTHER_PR_KEY = "testorg/testrepo/202"; + const LEGACY_PR_ID = "pr-detail"; + const KEYED_PR_ID = `pr-detail|${PR_KEY}`; + const OTHER_KEYED_PR_ID = `pr-detail|${OTHER_PR_KEY}`; + + function buildExtra(api: DockviewApi) { + const store = makeStore(api); + return { api, actions: buildExtraPanelActions(store.get) }; + } + + /** Canonical PR Details panels retain the key currently shown for the task. */ + function seedLegacyPanel(api: DockviewApi, prKey: string): void { + api.addPanel({ + id: LEGACY_PR_ID, + component: "pr-detail", + title: "Pull Request", + params: { prKey }, + position: { referenceGroup: CENTER_GROUP }, + }); + } + + it("reuses the legacy panel when the requested PR is what it's already showing", () => { + const { api, actions } = buildExtra(makeApi()); + seedLegacyPanel(api, PR_KEY); + + actions.addPRPanel(PR_KEY); + + const prPanels = api.panels.filter( + (p) => p.id === LEGACY_PR_ID || p.id.startsWith(`${LEGACY_PR_ID}|`), + ); + expect(prPanels).toHaveLength(1); + expect(api.getPanel(KEYED_PR_ID)).toBeUndefined(); + expect((api.getPanel(LEGACY_PR_ID) as unknown as MockPanel).isActive).toBe(true); + }); + + it("opens a distinct tab for a different PR instead of overwriting the legacy tab", () => { + const { api, actions } = buildExtra(makeApi()); + seedLegacyPanel(api, PR_KEY); + + // Multi-repo "+" menu click for a PR other than the one the legacy tab + // is showing must NOT repurpose that tab — it must open its own tab. + actions.addPRPanel(OTHER_PR_KEY); + + const legacy = api.getPanel(LEGACY_PR_ID) as unknown as MockPanel; + expect(legacy.params.prKey).toBe(PR_KEY); + + const other = api.getPanel(OTHER_KEYED_PR_ID) as unknown as MockPanel; + expect(other).toBeDefined(); + expect(other.isActive).toBe(true); + + const prPanels = api.panels.filter( + (p) => p.id === LEGACY_PR_ID || p.id.startsWith(`${LEGACY_PR_ID}|`), + ); + expect(prPanels).toHaveLength(2); + }); + + it("creates a new keyed panel when no legacy panel exists", () => { + const { api, actions } = buildExtra(makeApi()); + + actions.addPRPanel(PR_KEY); + + expect(api.getPanel(KEYED_PR_ID)).toBeDefined(); + expect(api.getPanel(LEGACY_PR_ID)).toBeUndefined(); + }); + + it("focuses an existing keyed panel exactly once", () => { + const { api, actions } = buildExtra(makeApi()); + + actions.addPRPanel(PR_KEY); + actions.addPRPanel(PR_KEY); + + const prPanels = api.panels.filter( + (p) => p.id === LEGACY_PR_ID || p.id.startsWith(`${LEGACY_PR_ID}|`), + ); + expect(prPanels).toHaveLength(1); + const keyed = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; + expect(keyed.isActive).toBe(true); + }); +}); + +describe("addPRPanel — layout-owned group placement", () => { + const PR_KEY = "testorg/testrepo/202"; + const KEYED_PR_ID = `pr-detail|${PR_KEY}`; + const SESSION_ID = "s-1"; + const SESSION_PANEL_ID = `session:${SESSION_ID}`; + const SESSION_GROUP = "group-session-host"; + const PR_DETAILS_GROUP = "group-configured-pr-details"; + + function buildExtra(api: DockviewApi) { + const store = makeStore(api); + return { api, actions: buildExtraPanelActions(store.get) }; + } + + function seedSessionInGroup(api: DockviewApi, groupId: string): void { + api.addPanel({ + id: SESSION_PANEL_ID, + component: "chat", + title: "Session", + params: { sessionId: SESSION_ID }, + position: { referenceGroup: groupId }, + }); + } + + function seedCanonicalPRDetails(api: DockviewApi, groupId = PR_DETAILS_GROUP): void { + api.addPanel({ + id: "pr-detail", + component: "pr-detail", + title: "PR Details", + position: { referenceGroup: groupId }, + }); + } + + it("places a new PR tab in PR Details' configured group, not beside Agent", () => { + const { api, actions } = buildExtra(makeApi()); + seedSessionInGroup(api, SESSION_GROUP); + seedCanonicalPRDetails(api); + + actions.addPRPanel(PR_KEY); + + const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; + expect(pr).toBeDefined(); + expect(pr.group.id).toBe(PR_DETAILS_GROUP); + expect(pr.group.id).not.toBe(SESSION_GROUP); + }); + + it("falls back to centerGroupId when no PR Details panel exists", () => { + const { api, actions } = buildExtra(makeApi()); + actions.addPRPanel(PR_KEY); + + const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; + expect(pr).toBeDefined(); + expect(pr.group.id).toBe(CENTER_GROUP); + }); + + it("focuses an existing keyed PR tab without relocating it", () => { + const { api, actions } = buildExtra(makeApi()); + seedCanonicalPRDetails(api); + api.addPanel({ + id: KEYED_PR_ID, + component: "pr-detail", + title: "Pull Request", + params: { prKey: PR_KEY }, + position: { referenceGroup: SESSION_GROUP }, + }); + + actions.addPRPanel(PR_KEY); + + const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; + expect(pr.group.id).toBe(SESSION_GROUP); + expect(pr.isActive).toBe(true); + }); +}); + +describe("addMRPanel — layout-owned review tabs", () => { + const MR_KEY = "https://gitlab.example.test|platform/kandev|81"; + const OTHER_MR_KEY = "https://gitlab.example.test|platform/kandev|82"; + const LEGACY_MR_ID = "mr-detail"; + + function buildExtra(api: DockviewApi) { + const store = makeStore(api); + return { api, actions: buildExtraPanelActions(store.get) }; + } + + function seedLegacyPanel(api: DockviewApi, mrKey: string): void { + api.addPanel({ + id: LEGACY_MR_ID, + component: "mr-detail", + title: "Merge Request", + params: { mrKey }, + position: { referenceGroup: CENTER_GROUP }, + }); + } + + it("focuses a matching legacy panel in place", () => { + const { api, actions } = buildExtra(makeApi()); + seedLegacyPanel(api, MR_KEY); + + actions.addMRPanel(MR_KEY); + + expect(api.panels.filter((panel) => panel.id.startsWith(LEGACY_MR_ID))).toHaveLength(1); + expect(api.getPanel(`${LEGACY_MR_ID}|${MR_KEY}`)).toBeUndefined(); + expect((api.getPanel(LEGACY_MR_ID) as unknown as MockPanel).isActive).toBe(true); + }); + + it("opens a keyed panel in the canonical PR Details group", () => { + const { api, actions } = buildExtra(makeApi()); + seedLegacyPanel(api, MR_KEY); + api.addPanel({ + id: "pr-detail", + component: "pr-detail", + title: "PR Details", + position: { referenceGroup: "group-configured-pr-details" }, + }); + + actions.addMRPanel(OTHER_MR_KEY); + + const keyed = api.getPanel(`${LEGACY_MR_ID}|${OTHER_MR_KEY}`) as unknown as MockPanel; + expect(keyed).toBeDefined(); + expect(keyed.group.id).toBe("group-configured-pr-details"); + expect(api.panels.filter((panel) => panel.id.startsWith(LEGACY_MR_ID))).toHaveLength(2); + }); +}); + +const PLUGIN_ID = "kandev-plugin-notes"; +const PLUGIN_PANEL_KEY = "notes"; +const PLUGIN_PANEL_TITLE = "Notes"; +const PLUGIN_PANEL_ID = "plugin:kandev-plugin-notes:notes"; + +describe("addPluginPanel / closePluginPanels", () => { + function buildExtra(api: DockviewApi) { + const store = makeStore(api); + return { api, actions: buildExtraPanelActions(store.get) }; + } + + it("opens a plugin panel beside chat by default, with the shared plugin-panel component/tab", () => { + const { api, actions } = buildExtra(makeApi()); + + actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE); + + const panel = api.getPanel(PLUGIN_PANEL_ID) as unknown as MockPanel; + expect(panel).toBeDefined(); + expect(panel.title).toBe(PLUGIN_PANEL_TITLE); + expect(panel.params).toEqual({ pluginId: PLUGIN_ID, panelKey: PLUGIN_PANEL_KEY }); + }); + + it("focuses an already-open plugin panel instead of duplicating it", () => { + const { api, actions } = buildExtra(makeApi()); + + actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE); + (api.getPanel(PLUGIN_PANEL_ID) as unknown as MockPanel).isActive = false; + actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE); + + expect(api.panels.filter((p) => p.id === PLUGIN_PANEL_ID)).toHaveLength(1); + expect((api.getPanel(PLUGIN_PANEL_ID) as unknown as MockPanel).isActive).toBe(true); + }); + + it("respects inCenter and quiet options", () => { + const { api, actions } = buildExtra(makeApi()); + + actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE, { + inCenter: true, + quiet: true, + }); + + const panel = api.getPanel(PLUGIN_PANEL_ID) as unknown as MockPanel; + expect(panel.group.id).toBe(CENTER_GROUP); + expect(panel.isActive).toBe(false); + }); + + it("closePluginPanels removes only the target plugin's panels", () => { + const { api, actions } = buildExtra(makeApi()); + actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE); + actions.addPluginPanel("kandev-plugin-other", "widget", "Widget"); + + actions.closePluginPanels(PLUGIN_ID); + + expect(api.getPanel(PLUGIN_PANEL_ID)).toBeUndefined(); + expect(api.getPanel("plugin:kandev-plugin-other:widget")).toBeDefined(); + }); +}); diff --git a/apps/web/lib/state/dockview-panel-actions.test-utils.ts b/apps/web/lib/state/dockview-panel-actions.test-utils.ts new file mode 100644 index 0000000000..9fbe02e737 --- /dev/null +++ b/apps/web/lib/state/dockview-panel-actions.test-utils.ts @@ -0,0 +1,130 @@ +import type { AddPanelOptions, DockviewApi } from "dockview-react"; +import { buildPanelActions } from "./dockview-panel-actions"; +import { CENTER_GROUP } from "./layout-manager"; + +export type MockPanel = { + id: string; + title: string; + params: Record; + group: { id: string }; + isActive: boolean; + api: { + setActive: () => void; + updateParameters: (p: Record) => void; + moveTo: (opts: { group: { id: string } }) => void; + }; + setTitle: (t: string) => void; +}; + +export function makeApi( + options: { centerGroupId?: string; extraGroupIds?: string[] } = {}, +): DockviewApi { + const centerId = options.centerGroupId ?? CENTER_GROUP; + const panels: MockPanel[] = []; + const groups = [{ id: centerId }, ...(options.extraGroupIds ?? []).map((id) => ({ id }))]; + + function makePanel(add: AddPanelOptions & { id: string }): MockPanel { + const groupId = + (add.position as { referenceGroup?: string } | undefined)?.referenceGroup ?? centerId; + if (!groups.some((g) => g.id === groupId)) groups.push({ id: groupId }); + const panel: MockPanel = { + id: add.id, + title: (add.title as string) ?? "", + params: { ...(add.params ?? {}) }, + group: { id: groupId }, + isActive: false, + setTitle(t: string) { + this.title = t; + }, + api: { + setActive() { + for (const p of panels) p.isActive = false; + panel.isActive = true; + }, + updateParameters(p: Record) { + Object.assign(panel.params, p); + }, + moveTo({ group }: { group: { id: string } }) { + panel.group = { id: group.id }; + }, + }, + }; + return panel; + } + + const api = { + get groups() { + return groups; + }, + get panels() { + return panels; + }, + getPanel(id: string) { + return panels.find((p) => p.id === id); + }, + getGroup(id: string) { + return groups.find((g) => g.id === id); + }, + addPanel(opts: AddPanelOptions & { id: string }) { + const p = makePanel(opts); + panels.push(p); + if (!opts.inactive) p.api.setActive(); + return p; + }, + removePanel(panel: { id: string }) { + const i = panels.findIndex((p) => p.id === panel.id); + if (i >= 0) panels.splice(i, 1); + }, + get activePanel() { + return panels.find((p) => p.isActive); + }, + } as unknown as DockviewApi; + return api; +} + +type StoreShape = { + api: DockviewApi | null; + centerGroupId: string; + rightTopGroupId: string; + rightBottomGroupId: string; + selectedDiff: { path: string; content?: string } | null; +}; + +export function makeStore(api: DockviewApi) { + const state: StoreShape = { + api, + centerGroupId: CENTER_GROUP, + rightTopGroupId: "group-right-top", + rightBottomGroupId: "group-right-bottom", + selectedDiff: null, + }; + return { + get: () => state, + set: (partial: Partial) => Object.assign(state, partial), + state, + }; +} + +export function build(api: DockviewApi) { + const store = makeStore(api); + const actions = buildPanelActions(store.set, store.get); + return { api, actions, store }; +} + +export const PATH_A = "src/a.ts"; +export const PATH_B = "src/b.ts"; +export const PATH_NESTED_B = "src/nested/b.ts"; +export const SHARED_PATH = "README.md"; +export const NAME_A = "a.ts"; +export const NAME_B = "b.ts"; +export const SHARED_NAME = "README.md"; +export const PINNED_FILE_A_ID = "file:src/a.ts"; +export const PREVIEW_FILE_ID = "preview:file-editor"; +export const PREVIEW_DIFF_ID = "preview:file-diff"; +export const PREVIEW_COMMIT_ID = "preview:commit-detail"; +export const SHA_A = "abcdef1234567890"; +export const SHA_B = "fedcba0987654321"; +export const DIFF_FILE_PREFIX = "diff:file:"; +export const FILE_PREFIX_ID = "file:"; +export const COMMIT_PREFIX_ID = "commit:"; +export const TYPE_FILE_EDITOR = "file-editor" as const; diff --git a/apps/web/lib/state/dockview-panel-actions.test.ts b/apps/web/lib/state/dockview-panel-actions.test.ts index 18417ad419..a09a37894f 100644 --- a/apps/web/lib/state/dockview-panel-actions.test.ts +++ b/apps/web/lib/state/dockview-panel-actions.test.ts @@ -1,144 +1,28 @@ import { describe, it, expect, beforeEach } from "vitest"; -import type { DockviewApi, AddPanelOptions } from "dockview-react"; -import { buildPanelActions, buildExtraPanelActions } from "./dockview-panel-actions"; +import type { DockviewApi } from "dockview-react"; +import { buildPanelActions } from "./dockview-panel-actions"; import { CENTER_GROUP } from "./layout-manager"; - -// --------------------------------------------------------------------------- -// Minimal DockviewApi mock -// --------------------------------------------------------------------------- - -type MockPanel = { - id: string; - title: string; - params: Record; - group: { id: string }; - isActive: boolean; - api: { - setActive: () => void; - updateParameters: (p: Record) => void; - moveTo: (opts: { group: { id: string } }) => void; - }; - setTitle: (t: string) => void; -}; - -function makeApi(options: { centerGroupId?: string; extraGroupIds?: string[] } = {}): DockviewApi { - const centerId = options.centerGroupId ?? CENTER_GROUP; - const panels: MockPanel[] = []; - const groups = [{ id: centerId }, ...(options.extraGroupIds ?? []).map((id) => ({ id }))]; - - function makePanel(add: AddPanelOptions & { id: string }): MockPanel { - const groupId = - (add.position as { referenceGroup?: string } | undefined)?.referenceGroup ?? centerId; - if (!groups.some((g) => g.id === groupId)) groups.push({ id: groupId }); - const panel: MockPanel = { - id: add.id, - title: (add.title as string) ?? "", - params: { ...(add.params ?? {}) }, - group: { id: groupId }, - isActive: false, - setTitle(t: string) { - this.title = t; - }, - api: { - setActive() { - for (const p of panels) p.isActive = false; - panel.isActive = true; - }, - updateParameters(p: Record) { - Object.assign(panel.params, p); - }, - moveTo({ group }: { group: { id: string } }) { - panel.group = { id: group.id }; - }, - }, - }; - return panel; - } - - const api = { - get groups() { - return groups; - }, - get panels() { - return panels; - }, - getPanel(id: string) { - return panels.find((p) => p.id === id); - }, - getGroup(id: string) { - return groups.find((g) => g.id === id); - }, - addPanel(opts: AddPanelOptions & { id: string }) { - const p = makePanel(opts); - panels.push(p); - if (!opts.inactive) p.api.setActive(); - return p; - }, - removePanel(panel: { id: string }) { - const i = panels.findIndex((p) => p.id === panel.id); - if (i >= 0) panels.splice(i, 1); - }, - get activePanel() { - return panels.find((p) => p.isActive); - }, - } as unknown as DockviewApi; - return api; -} - -// --------------------------------------------------------------------------- -// Store adapter: panel actions take (set, get) closures -// --------------------------------------------------------------------------- - -type StoreShape = { - api: DockviewApi | null; - centerGroupId: string; - rightTopGroupId: string; - rightBottomGroupId: string; - selectedDiff: { path: string; content?: string } | null; -}; - -function makeStore(api: DockviewApi) { - const state: StoreShape = { - api, - centerGroupId: CENTER_GROUP, - rightTopGroupId: "group-right-top", - rightBottomGroupId: "group-right-bottom", - selectedDiff: null, - }; - return { - get: () => state, - set: (partial: Partial) => Object.assign(state, partial), - state, - }; -} - -function build(api: DockviewApi) { - const store = makeStore(api); - const actions = buildPanelActions(store.set, store.get); - return { api, actions, store }; -} - -// --------------------------------------------------------------------------- -// Test fixtures -// --------------------------------------------------------------------------- - -const PATH_A = "src/a.ts"; -const PATH_B = "src/b.ts"; -const PATH_NESTED_B = "src/nested/b.ts"; -const SHARED_PATH = "README.md"; -const NAME_A = "a.ts"; -const NAME_B = "b.ts"; -const SHARED_NAME = "README.md"; -const PINNED_FILE_A_ID = "file:src/a.ts"; -const PREVIEW_FILE_ID = "preview:file-editor"; -const PREVIEW_DIFF_ID = "preview:file-diff"; -const PREVIEW_COMMIT_ID = "preview:commit-detail"; -const SHA_A = "abcdef1234567890"; -const SHA_B = "fedcba0987654321"; -const DIFF_FILE_PREFIX = "diff:file:"; -const FILE_PREFIX_ID = "file:"; -const COMMIT_PREFIX_ID = "commit:"; -const TYPE_FILE_EDITOR = "file-editor" as const; +import { build, makeApi } from "./dockview-panel-actions.test-utils"; +import type { MockPanel } from "./dockview-panel-actions.test-utils"; +import { + COMMIT_PREFIX_ID, + DIFF_FILE_PREFIX, + FILE_PREFIX_ID, + NAME_A, + NAME_B, + PATH_A, + PATH_B, + PATH_NESTED_B, + PINNED_FILE_A_ID, + PREVIEW_COMMIT_ID, + PREVIEW_DIFF_ID, + PREVIEW_FILE_ID, + SHA_A, + SHA_B, + SHARED_NAME, + SHARED_PATH, + TYPE_FILE_EDITOR, +} from "./dockview-panel-actions.test-utils"; // --------------------------------------------------------------------------- // Tests @@ -456,6 +340,27 @@ describe("addCommitDetailPanel — preview behavior", () => { expect(preview.params.commitSha).toBe(SHA_B); expect(preview.params.promoted).toBeUndefined(); }); + + it("serializes remote provenance and keeps repositories with equal SHAs distinct", () => { + const first = { + source: "github" as const, + sha: SHA_A, + workspaceId: "workspace-1", + owner: "acme", + repo: "frontend", + }; + const second = { ...first, repo: "backend" }; + + actions.addCommitDetailPanel(first, { pin: true }); + actions.addCommitDetailPanel(second, { pin: true }); + + const firstPanel = api.getPanel(`commit:github:workspace-1:acme/frontend:${SHA_A}`); + const secondPanel = api.getPanel(`commit:github:workspace-1:acme/backend:${SHA_A}`); + expect(firstPanel).toBeDefined(); + expect(secondPanel).toBeDefined(); + expect((secondPanel as unknown as MockPanel).params.target).toEqual(second); + expect((secondPanel as unknown as MockPanel).params.commitSha).toBe(SHA_A); + }); }); describe("preview slots are independent across types", () => { @@ -471,266 +376,3 @@ describe("preview slots are independent across types", () => { expect(api.getPanel(PREVIEW_COMMIT_ID)).toBeDefined(); }); }); - -describe("addPRPanel — dedup with legacy auto-shown panel", () => { - const PR_KEY = "testorg/testrepo/101"; - const OTHER_PR_KEY = "testorg/testrepo/202"; - const LEGACY_PR_ID = "pr-detail"; - const KEYED_PR_ID = `pr-detail|${PR_KEY}`; - const OTHER_KEYED_PR_ID = `pr-detail|${OTHER_PR_KEY}`; - - function buildExtra(api: DockviewApi) { - const store = makeStore(api); - return { api, actions: buildExtraPanelActions(store.get) }; - } - - /** Canonical PR Details panels retain the key currently shown for the task. */ - function seedLegacyPanel(api: DockviewApi, prKey: string): void { - api.addPanel({ - id: LEGACY_PR_ID, - component: "pr-detail", - title: "Pull Request", - params: { prKey }, - position: { referenceGroup: CENTER_GROUP }, - }); - } - - it("reuses the legacy panel when the requested PR is what it's already showing", () => { - const { api, actions } = buildExtra(makeApi()); - seedLegacyPanel(api, PR_KEY); - - actions.addPRPanel(PR_KEY); - - const prPanels = api.panels.filter( - (p) => p.id === LEGACY_PR_ID || p.id.startsWith(`${LEGACY_PR_ID}|`), - ); - expect(prPanels).toHaveLength(1); - expect(api.getPanel(KEYED_PR_ID)).toBeUndefined(); - expect((api.getPanel(LEGACY_PR_ID) as unknown as MockPanel).isActive).toBe(true); - }); - - it("opens a distinct tab for a different PR instead of overwriting the legacy tab", () => { - const { api, actions } = buildExtra(makeApi()); - seedLegacyPanel(api, PR_KEY); - - // Multi-repo "+" menu click for a PR other than the one the legacy tab - // is showing must NOT repurpose that tab — it must open its own tab. - actions.addPRPanel(OTHER_PR_KEY); - - const legacy = api.getPanel(LEGACY_PR_ID) as unknown as MockPanel; - expect(legacy.params.prKey).toBe(PR_KEY); - - const other = api.getPanel(OTHER_KEYED_PR_ID) as unknown as MockPanel; - expect(other).toBeDefined(); - expect(other.isActive).toBe(true); - - const prPanels = api.panels.filter( - (p) => p.id === LEGACY_PR_ID || p.id.startsWith(`${LEGACY_PR_ID}|`), - ); - expect(prPanels).toHaveLength(2); - }); - - it("creates a new keyed panel when no legacy panel exists", () => { - const { api, actions } = buildExtra(makeApi()); - - actions.addPRPanel(PR_KEY); - - expect(api.getPanel(KEYED_PR_ID)).toBeDefined(); - expect(api.getPanel(LEGACY_PR_ID)).toBeUndefined(); - }); - - it("focuses an existing keyed panel exactly once", () => { - const { api, actions } = buildExtra(makeApi()); - - actions.addPRPanel(PR_KEY); - actions.addPRPanel(PR_KEY); - - const prPanels = api.panels.filter( - (p) => p.id === LEGACY_PR_ID || p.id.startsWith(`${LEGACY_PR_ID}|`), - ); - expect(prPanels).toHaveLength(1); - const keyed = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; - expect(keyed.isActive).toBe(true); - }); -}); - -describe("addPRPanel — layout-owned group placement", () => { - const PR_KEY = "testorg/testrepo/202"; - const KEYED_PR_ID = `pr-detail|${PR_KEY}`; - const SESSION_ID = "s-1"; - const SESSION_PANEL_ID = `session:${SESSION_ID}`; - const SESSION_GROUP = "group-session-host"; - const PR_DETAILS_GROUP = "group-configured-pr-details"; - - function buildExtra(api: DockviewApi) { - const store = makeStore(api); - return { api, actions: buildExtraPanelActions(store.get) }; - } - - function seedSessionInGroup(api: DockviewApi, groupId: string): void { - api.addPanel({ - id: SESSION_PANEL_ID, - component: "chat", - title: "Session", - params: { sessionId: SESSION_ID }, - position: { referenceGroup: groupId }, - }); - } - - function seedCanonicalPRDetails(api: DockviewApi, groupId = PR_DETAILS_GROUP): void { - api.addPanel({ - id: "pr-detail", - component: "pr-detail", - title: "PR Details", - position: { referenceGroup: groupId }, - }); - } - - it("places a new PR tab in PR Details' configured group, not beside Agent", () => { - const { api, actions } = buildExtra(makeApi()); - seedSessionInGroup(api, SESSION_GROUP); - seedCanonicalPRDetails(api); - - actions.addPRPanel(PR_KEY); - - const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; - expect(pr).toBeDefined(); - expect(pr.group.id).toBe(PR_DETAILS_GROUP); - expect(pr.group.id).not.toBe(SESSION_GROUP); - }); - - it("falls back to centerGroupId when no PR Details panel exists", () => { - const { api, actions } = buildExtra(makeApi()); - actions.addPRPanel(PR_KEY); - - const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; - expect(pr).toBeDefined(); - expect(pr.group.id).toBe(CENTER_GROUP); - }); - - it("focuses an existing keyed PR tab without relocating it", () => { - const { api, actions } = buildExtra(makeApi()); - seedCanonicalPRDetails(api); - api.addPanel({ - id: KEYED_PR_ID, - component: "pr-detail", - title: "Pull Request", - params: { prKey: PR_KEY }, - position: { referenceGroup: SESSION_GROUP }, - }); - - actions.addPRPanel(PR_KEY); - - const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; - expect(pr.group.id).toBe(SESSION_GROUP); - expect(pr.isActive).toBe(true); - }); -}); - -describe("addMRPanel — layout-owned review tabs", () => { - const MR_KEY = "https://gitlab.example.test|platform/kandev|81"; - const OTHER_MR_KEY = "https://gitlab.example.test|platform/kandev|82"; - const LEGACY_MR_ID = "mr-detail"; - - function buildExtra(api: DockviewApi) { - const store = makeStore(api); - return { api, actions: buildExtraPanelActions(store.get) }; - } - - function seedLegacyPanel(api: DockviewApi, mrKey: string): void { - api.addPanel({ - id: LEGACY_MR_ID, - component: "mr-detail", - title: "Merge Request", - params: { mrKey }, - position: { referenceGroup: CENTER_GROUP }, - }); - } - - it("focuses a matching legacy panel in place", () => { - const { api, actions } = buildExtra(makeApi()); - seedLegacyPanel(api, MR_KEY); - - actions.addMRPanel(MR_KEY); - - expect(api.panels.filter((panel) => panel.id.startsWith(LEGACY_MR_ID))).toHaveLength(1); - expect(api.getPanel(`${LEGACY_MR_ID}|${MR_KEY}`)).toBeUndefined(); - expect((api.getPanel(LEGACY_MR_ID) as unknown as MockPanel).isActive).toBe(true); - }); - - it("opens a keyed panel in the canonical PR Details group", () => { - const { api, actions } = buildExtra(makeApi()); - seedLegacyPanel(api, MR_KEY); - api.addPanel({ - id: "pr-detail", - component: "pr-detail", - title: "PR Details", - position: { referenceGroup: "group-configured-pr-details" }, - }); - - actions.addMRPanel(OTHER_MR_KEY); - - const keyed = api.getPanel(`${LEGACY_MR_ID}|${OTHER_MR_KEY}`) as unknown as MockPanel; - expect(keyed).toBeDefined(); - expect(keyed.group.id).toBe("group-configured-pr-details"); - expect(api.panels.filter((panel) => panel.id.startsWith(LEGACY_MR_ID))).toHaveLength(2); - }); -}); - -const PLUGIN_ID = "kandev-plugin-notes"; -const PLUGIN_PANEL_KEY = "notes"; -const PLUGIN_PANEL_TITLE = "Notes"; -const PLUGIN_PANEL_ID = "plugin:kandev-plugin-notes:notes"; - -describe("addPluginPanel / closePluginPanels", () => { - function buildExtra(api: DockviewApi) { - const store = makeStore(api); - return { api, actions: buildExtraPanelActions(store.get) }; - } - - it("opens a plugin panel beside chat by default, with the shared plugin-panel component/tab", () => { - const { api, actions } = buildExtra(makeApi()); - - actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE); - - const panel = api.getPanel(PLUGIN_PANEL_ID) as unknown as MockPanel; - expect(panel).toBeDefined(); - expect(panel.title).toBe(PLUGIN_PANEL_TITLE); - expect(panel.params).toEqual({ pluginId: PLUGIN_ID, panelKey: PLUGIN_PANEL_KEY }); - }); - - it("focuses an already-open plugin panel instead of duplicating it", () => { - const { api, actions } = buildExtra(makeApi()); - - actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE); - (api.getPanel(PLUGIN_PANEL_ID) as unknown as MockPanel).isActive = false; - actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE); - - expect(api.panels.filter((p) => p.id === PLUGIN_PANEL_ID)).toHaveLength(1); - expect((api.getPanel(PLUGIN_PANEL_ID) as unknown as MockPanel).isActive).toBe(true); - }); - - it("respects inCenter and quiet options", () => { - const { api, actions } = buildExtra(makeApi()); - - actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE, { - inCenter: true, - quiet: true, - }); - - const panel = api.getPanel(PLUGIN_PANEL_ID) as unknown as MockPanel; - expect(panel.group.id).toBe(CENTER_GROUP); - expect(panel.isActive).toBe(false); - }); - - it("closePluginPanels removes only the target plugin's panels", () => { - const { api, actions } = buildExtra(makeApi()); - actions.addPluginPanel(PLUGIN_ID, PLUGIN_PANEL_KEY, PLUGIN_PANEL_TITLE); - actions.addPluginPanel("kandev-plugin-other", "widget", "Widget"); - - actions.closePluginPanels(PLUGIN_ID); - - expect(api.getPanel(PLUGIN_PANEL_ID)).toBeUndefined(); - expect(api.getPanel("plugin:kandev-plugin-other:widget")).toBeDefined(); - }); -}); diff --git a/apps/web/lib/state/dockview-panel-actions.ts b/apps/web/lib/state/dockview-panel-actions.ts index 9491d52ff8..d501ceec80 100644 --- a/apps/web/lib/state/dockview-panel-actions.ts +++ b/apps/web/lib/state/dockview-panel-actions.ts @@ -1,4 +1,5 @@ import type { DockviewApi, DockviewGroupPanel } from "dockview-react"; +import type { CommitDetailTarget } from "@/components/task/changes-diff-target"; import { focusOrAddPanel } from "./dockview-layout-builders"; import { parsePluginPanelId, @@ -360,20 +361,46 @@ function buildFileDiffAction(get: StoreGet) { }; } +function buildCommitItemId( + requestedTarget: CommitDetailTarget | string, + target: CommitDetailTarget, + legacyRepo?: string, +): string { + if (typeof requestedTarget === "string") { + return legacyRepo ? `${legacyRepo}:${requestedTarget}` : requestedTarget; + } + if (target.source === "local") return `local:${target.repo ?? ""}:${target.sha}`; + return `github:${target.workspaceId}:${target.owner}/${target.repo}:${target.sha}`; +} + function buildCommitDetailAction(get: StoreGet) { - return (sha: string, opts?: OpenPanelOpts & { groupId?: string; repo?: string }) => { + return ( + requestedTarget: CommitDetailTarget | string, + opts?: OpenPanelOpts & { groupId?: string; repo?: string }, + ) => { const { api, centerGroupId } = get(); if (!api) return; - // Multi-repo: scope the panel id by repo so the same SHA from two repos - // (rare in practice, but cheap to be correct) doesn't collide and so the - // existing-tab dedup doesn't reuse the wrong-repo's panel. - const itemId = opts?.repo ? `${opts.repo}:${sha}` : sha; + const target: CommitDetailTarget = + typeof requestedTarget === "string" + ? { + source: "local", + sha: requestedTarget, + ...(opts?.repo ? { repo: opts.repo } : {}), + } + : requestedTarget; + // Preserve the legacy string-call identity for saved callers/tests; new + // discriminated targets include provenance and repository identity. + const itemId = buildCommitItemId(requestedTarget, target, opts?.repo); openOrReplacePreview({ api, type: "commit-detail", itemId, - title: sha.slice(0, 7), - params: { commitSha: sha, repo: opts?.repo }, + title: target.sha.slice(0, 7), + params: { + target, + commitSha: target.sha, + ...(target.source === "local" && target.repo ? { repo: target.repo } : {}), + }, groupId: opts?.groupId ?? centerGroupId, quiet: opts?.quiet, pin: opts?.pin, diff --git a/apps/web/lib/state/dockview-store.ts b/apps/web/lib/state/dockview-store.ts index d6ce82d7d4..55f0ae4ae1 100644 --- a/apps/web/lib/state/dockview-store.ts +++ b/apps/web/lib/state/dockview-store.ts @@ -33,6 +33,7 @@ import { materializeReusableChatPanel, } from "./layout-manager"; import type { BuiltInPreset, LayoutState, LayoutGroupIds } from "./layout-manager"; +import type { CommitDetailTarget } from "@/components/task/changes-diff-target"; import { performEnvSwitch, replaceStaleSessionPanels } from "./dockview-env-switch"; import { enforcePinnedTargets } from "./dockview-pinned-enforce"; import { @@ -149,7 +150,7 @@ type DockviewStore = { }, ) => void; addCommitDetailPanel: ( - sha: string, + target: CommitDetailTarget | string, opts?: OpenPanelOpts & { groupId?: string; repo?: string }, ) => void; addFileEditorPanel: (path: string, name: string, opts?: OpenPanelOpts) => void; diff --git a/apps/web/lib/types/github.ts b/apps/web/lib/types/github.ts index 75b9af20aa..24aeda3482 100644 --- a/apps/web/lib/types/github.ts +++ b/apps/web/lib/types/github.ts @@ -525,6 +525,21 @@ export type PRCommitInfo = { additions: number; deletions: number; files_changed: number; + /** False when the PR commit-list endpoint did not include exact stats. */ + stats_available?: boolean; +}; + +/** Exact metadata and files returned by the individual GitHub commit endpoint. */ +export type PRCommitDetail = { + sha: string; + message: string; + author_login: string; + author_name: string; + author_date: string; + additions: number; + deletions: number; + files_changed: number; + files: PRDiffFile[]; }; // GitHub Issue (separate from Pull Request) diff --git a/docs/plans/pr-only-commit-details/plan.md b/docs/plans/pr-only-commit-details/plan.md new file mode 100644 index 0000000000..79b9df81ff --- /dev/null +++ b/docs/plans/pr-only-commit-details/plan.md @@ -0,0 +1,219 @@ +--- +spec: docs/specs/pr-only-commit-details/spec.md +created: 2026-08-04 +status: implemented +--- + +# Implementation Plan: Repair PR-only commit details + +## Overview + +Make commit provenance a first-class part of the Changes panel. Preserve the +local session path for SHAs known to the worktree, but route PR-only SHAs to a +new workspace-authorized GitHub individual-commit action. Hide unmeasured row +statistics and local mutation controls for remote commits, then cover the +force-pushed/stale-worktree case on desktop and mobile. + +## Confirmed root cause + +For task `8531031a-9a98-439f-ab0a-c25b480158dc`, the stored session worktree +was created at local head `5c2873d`, while linked PR #2253 was later +force-pushed to six different commits ending at `df3c05e`. + +- `mergeCommits` in + `apps/web/components/task/changes-panel-helpers.ts` appends PR-only SHAs to + the local commit feed without preserving their source. +- GitHub's pull request commit-list response does not include commit stats. + `convertRawPRCommits` leaves Go integer fields at zero, so the frontend + presents unknown values as `+0 -0`. +- Every row click is reduced to a SHA and repository name, then sent through + `session.commit_diff`. The backend runs `git show` in the task worktree even + for PR-only SHAs. +- The shared row also exposes local reset/revert/amend callbacks for PR-only + commits, while `CommitDetailPanel` resolves metadata only from local session + commits. + +The repair invariant is: row statistics, detail data, metadata, and available +actions must all derive from the same explicit commit source. + +## Backend + +### Workspace-scoped GitHub commit detail + +- Add a `github.pr_commit.get` WebSocket action in + `apps/backend/pkg/websocket/actions.go` and register it in + `apps/backend/internal/github/handlers.go`. +- Add `StatsAvailable bool` with JSON field `stats_available` to `PRCommitInfo`. + Define `PRCommitDetail` in `apps/backend/internal/github/models.go` with SHA, + message, author login/name/date, exact additions/deletions, files changed, + and the merged `[]PRFile` returned by the individual GitHub commit endpoint. +- Extend the GitHub client interface and the `gh`, PAT, noop, and mock clients + with an exact-SHA commit-detail method. The CLI client should call + `gh api repos/{owner}/{repo}/commits/{sha}`; the PAT client should call the + equivalent REST path. Parse both through one helper so their response + semantics stay aligned. +- Request the maximum supported page size and merge every returned file page + for both clients (`gh api --paginate --slurp` for the CLI representation and + Link-header pagination for PAT). Preserve the first page's metadata/stats, + append file records in provider order without duplication, and test a + multi-page response. +- Add a service method that reuses `ensureRepositoryInWorkspaceScope` and the + existing personal read-client resolution. Validate that the SHA is a safe + exact object ID before interpolating it into either client path. +- Do not fetch, persist, or import the commit into the task worktree. + +### Unknown list statistics + +- Use `PRCommitInfo.stats_available` to explicitly represent whether additions, + deletions, and files-changed values are available. The normal GitHub list + parser must set it to false rather than relying on zero values. +- Keep exact integers mandatory on `PRCommitDetail`, where the individual + endpoint provides them. This lets a genuinely empty commit remain + distinguishable from an unmeasured row. + +## Frontend + +### Source-aware commit targets + +- Introduce a discriminated commit-detail target: local targets contain the + existing SHA/repository routing data; GitHub targets additionally contain + workspace, owner, and repository identity. +- Make `mergeCommits` return that target plus a statistics-availability flag. + A SHA found in both feeds remains one local row and retains its pushed state. + A PR-only SHA becomes a GitHub target and does not acquire zero statistics. +- Thread the full target through the Changes panel callbacks, desktop dockview + panel params/actions, and mobile diff target. Include source and repository + identity in dockview panel identity so equal SHAs from different repositories + cannot collide. + +### Unified detail loading and rendering + +- Add a source-aware commit-detail hook/request layer. Local targets continue + to use `session.commit_diff`; GitHub targets call `github.pr_commit.get` and + map `PRFile` records into the current diff-view model. +- Have `CommitDetailPanel` obtain metadata and files from that unified result, + fixing the missing PR-only header as well as the patch source. +- For GitHub targets, disable worktree-dependent file actions and local context + expansion. Preserve the current rendering for available, binary, empty, or + truncated GitHub patches; never retry through the local git path. +- Gate reset/revert/amend row actions on local provenance. Render additions and + deletions only when the row marks them as measured. + +## Desktop and mobile behavior + +The visual composition does not change. Desktop continues to open the existing +dockview commit panel. Mobile continues to use +`MobileChangesPanel` -> `MobileDiffSheet`, the current full-height drawer and +closest native-mobile exemplar. Both surfaces consume the same discriminated +target and detail hook. The mobile drawer keeps its existing single scroll +owner, close affordance, and touch behavior; only its data source and local +action eligibility change. + +## Tests + +- Backend parser tests prove list statistics are unavailable and individual + commit responses preserve exact metadata, stats, file status, and patch. +- `gh` and PAT client tests prove the exact repository/SHA path and shared + response parsing. +- Handler/service tests prove workspace/repository authorization, exact-SHA + validation, response routing, and error propagation. +- Frontend merge tests prove PR-only provenance, hidden unknown stats, + multi-repository identity, and local precedence for a SHA found in both + feeds. +- Request/hook tests prove mutually exclusive local and GitHub actions and no + local fallback after a GitHub error. +- Panel/row tests prove remote metadata is visible and reset/revert/amend, + worktree-file actions, and local expansion are absent for GitHub targets. +- Dockview and mobile target tests prove source/repository identity survives + serialization and selection. + +All production changes follow Red-Green-Refactor: add each focused regression, +record the expected failure against current behavior, add the smallest source +change, then rerun the focused and package-level checks. + +## E2E tests + +Extend `apps/web/e2e/tests/git/git-changes-panel.spec.ts` with a linked PR whose +mocked current commits are absent from the stale local worktree. Assert that the +row omits false `+0 -0`, its local mutation menu is absent, and opening it shows +the GitHub-specific commit header and patch marker. The scenario must fail if +the UI invokes `session.commit_diff` for that SHA. + +Extend `apps/web/e2e/tests/task/mobile-changes-panel.spec.ts` with the same +PR-only source. Assert that tapping the row opens the existing full-height +commit sheet, renders the remote marker patch, exposes no local action, closes +normally, and introduces no horizontal overflow. Add deterministic mock GitHub +individual-commit seeding to the existing mock controller/API helper rather +than depending on live GitHub. + +## Implementation waves and parallel candidates + +Wave 1 (sequential): + +- [x] [Task 01: GitHub commit detail contract](task-01-github-commit-detail.md) + — add the authenticated individual-commit backend contract and distinguish + unknown list statistics. + +Wave 2 (depends on Wave 1): + +- [x] [Task 02: Source-aware commit UI](task-02-source-aware-commit-ui.md) — + route local and GitHub targets correctly across desktop and mobile. + +Wave 3 (depends on Waves 1-2): + +- [x] [Task 03: Commit-source E2E coverage](task-03-commit-source-e2e.md) — + exercise stale local history against deterministic remote commit details. + +The tasks are intentionally sequential. Task 02 consumes the backend wire +contract from Task 01, and Task 03 validates the integrated behavior and adds +mock support for that settled contract. + +## Verification + +Run the focused commands recorded in each task, then finish with: + +```bash +cd apps/backend && go test ./internal/github +cd apps/web && pnpm run typecheck +cd apps && pnpm --filter @kandev/web lint +cd apps/web && pnpm run i18n:check && pnpm run i18n:ratchet +cd apps/web && pnpm e2e:run tests/git/git-changes-panel.spec.ts -- --grep "PR-only commit" +cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-changes-panel.spec.ts -- --grep "PR-only commit" +git diff --check +``` + +## Verification results + +- Backend: `go test ./internal/github` passed (1081 tests). +- Frontend: focused source/target suite passed (7 files, 72 tests), followed by + `pnpm run typecheck`, web lint, `i18n:check`, and `i18n:ratchet`. +- E2E: the desktop PR-only scenario passed once; the mobile scenario passed + once after narrowing an assertion to the opened sheet. Both scenarios cover + stale local history, hidden unknown statistics/actions, remote metadata, and + remote patch rendering. +- `git diff --check` passed. +- Review remediation: complete PR identity now gates visible commit-list + results; unavailable or malformed detail responses show a persistent retry + state; and remote detail loading is independent of local agent readiness. + The broader focused frontend suite passed (8 files, 69 tests), plus the + remediation request/hook/panel tests (4 files, 15 tests), typecheck, lint, + i18n checks, and ratchet. +- Review follow-up after rebasing onto `main`: root repository identity is now + matched exactly, invalid/null commit payloads are rejected at both backend + and frontend boundaries, target switches cannot expose prior detail state, + and serialized GitHub targets require complete identity. Backend GitHub tests + passed (1,086); focused frontend tests passed (8 files, 73 tests), with + typecheck, lint, i18n checks, ratchet, and diff checks green. + +## Risks and out of scope + +- Individual GitHub commit requests add a lazy network round trip and can be + rate-limited. The existing loading/error treatment must remain responsive, + and no eager N+1 list loading is allowed. +- GitHub can omit patches for binary or oversized changes. That state must be + presented honestly and must not trigger a local-data fallback. +- The source target becomes part of desktop panel identity and mobile + selection state. Tests must prevent stale serialized targets or + cross-repository SHA collisions. +- No database migration, public documentation change, worktree synchronization, + or non-GitHub provider is part of this repair. diff --git a/docs/plans/pr-only-commit-details/task-01-github-commit-detail.md b/docs/plans/pr-only-commit-details/task-01-github-commit-detail.md new file mode 100644 index 0000000000..c698a4abb5 --- /dev/null +++ b/docs/plans/pr-only-commit-details/task-01-github-commit-detail.md @@ -0,0 +1,63 @@ +--- +id: "01-github-commit-detail" +title: "GitHub commit detail contract" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/pr-only-commit-details/spec.md" +--- + +# Task 01: GitHub commit detail contract + +- **Acceptance:** The backend exposes one workspace-authorized + `github.pr_commit.get` action for an exact owner/repository/SHA; the response + contains GitHub commit metadata, exact aggregate statistics, and changed-file + patches merged across provider pages; `PRCommitInfo.stats_available` + distinguishes unavailable list statistics from known zeroes; unauthorized + repositories, unsafe SHAs, upstream errors, and missing commits return errors + without touching a task worktree. +- **Verification:** Follow strict TDD. First add parser, client, and + handler/service regressions and run + `cd apps/backend && go test -run 'Test.*(PRCommitDetail|CommitDetail|PRCommitStats)' ./internal/github`; + record failures showing the absent action/model and zero-value ambiguity. + After the minimal implementation, rerun that command, then run + `cd apps/backend && go test ./internal/github` and `git diff --check`. +- **Files likely touched:** + `apps/backend/pkg/websocket/actions.go`; + `apps/backend/internal/github/models.go`; + `apps/backend/internal/github/client.go`; + `apps/backend/internal/github/client_helpers.go`; + `apps/backend/internal/github/gh_client.go`; + `apps/backend/internal/github/pat_client.go`; + `apps/backend/internal/github/noop_client.go`; + `apps/backend/internal/github/mock_client.go`; + `apps/backend/internal/github/service_pr.go`; + `apps/backend/internal/github/handlers.go`; and focused tests alongside those + files, including authorization coverage. +- **Dependencies:** None. +- **Parallelism:** sequential — the model, client interface, authorization + service, and WebSocket handler form one wire contract and their RED/GREEN + changes overlap the same package. +- **Inputs:** repair spec API contract; current `PRCommitInfo`, `PRFile`, + `GetPRCommits`, `GetPRFiles`, `ensureRepositoryInWorkspaceScope`, personal + read-client resolution, and GitHub handler authorization test patterns. +- **Output contract:** Report the final request/response shape, exact files + changed, RED/GREEN and full-package results, pagination behavior, + authorization/error behavior, confirmation that no worktree or persistence + path was added, and synchronized task/plan status. + +## Results + +- RED: parser, client, authorization, and WebSocket regressions exposed the + missing individual-commit contract and the zero-value ambiguity for list + statistics. +- GREEN: added `github.pr_commit.get`, workspace/repository authorization, + exact-SHA validation, shared PAT/CLI parsing, paginated file merging with + provider-order deduplication, and mock/noop support. List commits now set + `stats_available: false`; individual details carry exact metadata, + statistics, and files. +- Verification: `go test ./internal/github` passed with 1081 tests; the + focused parser/client/service/handler tests and `git diff --check` passed. +- Individual commit details remain read-only and are never imported into a + task worktree or persisted. diff --git a/docs/plans/pr-only-commit-details/task-02-source-aware-commit-ui.md b/docs/plans/pr-only-commit-details/task-02-source-aware-commit-ui.md new file mode 100644 index 0000000000..2865158ea1 --- /dev/null +++ b/docs/plans/pr-only-commit-details/task-02-source-aware-commit-ui.md @@ -0,0 +1,84 @@ +--- +id: "02-source-aware-commit-ui" +title: "Source-aware commit UI" +status: done +wave: 2 +depends_on: ["01-github-commit-detail"] +plan: "plan.md" +spec: "../../specs/pr-only-commit-details/spec.md" +--- + +# Task 02: Source-aware commit UI + +- **Acceptance:** Merged commit rows carry a discriminated local/GitHub target; + matching SHAs prefer the local source; PR-only rows omit unknown stats and + local mutation controls; opening a local target calls only + `session.commit_diff`; opening a GitHub target calls only + `github.pr_commit.get`, renders remote metadata/files on desktop and mobile, + disables worktree-dependent actions/expansion, preserves repository identity, + and never falls back locally on a GitHub error. +- **Verification:** Follow strict TDD. Add focused merge, request/hook, row, + panel, dockview, and mobile-target regressions; run the smallest affected + Vitest files and record failures caused by the current SHA-only target and + local-only loader. Use + `cd apps/web && pnpm test -- components/task/changes-panel.test.ts components/task/commit-detail-request.test.ts components/task/commit-detail-panel.test.tsx lib/state/dockview-panel-actions.test.ts` + for the focused RED/GREEN cycle. After implementation, rerun it, then run + `cd apps/web && pnpm run typecheck`, + `cd apps && pnpm --filter @kandev/web lint`, + `cd apps && pnpm run i18n:check && pnpm run i18n:ratchet`, and + `git diff --check`. +- **Files likely touched:** + `apps/web/lib/types/github.ts`; + a source-aware target/request hook under + `apps/web/components/task` or `apps/web/hooks/domains`; + `apps/web/components/task/changes-panel-helpers.ts`; + `apps/web/components/task/changes-panel-data.tsx`; + `apps/web/components/task/changes-panel-body.tsx`; + `apps/web/components/task/commit-row.tsx`; + `apps/web/components/task/commit-detail-panel.tsx`; + `apps/web/components/task/changes-diff-target.ts`; + `apps/web/components/task/dockview-shared.tsx`; + `apps/web/components/task/dockview-panel-content.tsx`; + `apps/web/lib/state/dockview-store.ts`; + `apps/web/lib/state/dockview-panel-actions.ts`; + `apps/web/components/task/mobile/mobile-changes-panel.tsx`; + `apps/web/components/task/mobile/mobile-diff-sheet.tsx`; and colocated unit + tests for the changed helpers, state, hook, and components. +- **Dependencies:** Task 01's settled WebSocket action and response types. +- **Parallelism:** sequential — source identity crosses merge logic, serialized + desktop panel state, mobile selection, and one shared detail renderer; split + implementations would create an intermediate incompatible UI contract. +- **Inputs:** repair spec desired behavior; Task 01 output contract; current + `mergeCommits`, `CommitRow`, `requestCommitDiff`, `useCommitDiff`, + `CommitDetailPanel`, dockview commit panel actions, `MobileChangesPanel`, and + `MobileDiffSheet` patterns. +- **Output contract:** Report the discriminated target shape, route selection, + row/action behavior, desktop/mobile rendering behavior, exact files changed, + RED/GREEN and lint/type/i18n results, remaining patch limitations, and + synchronized task/plan status. + +## Results + +- RED: source-routing regressions demonstrated that SHA-only rows always used + `session.commit_diff`, including PR-only commits, and that remote rows could + inherit fabricated zero statistics and local actions. +- GREEN: added the discriminated local/GitHub target, repository-aware merge + precedence, source-aware request/hook loading, remote metadata/file mapping, + dockview serialization identity, and shared desktop/mobile rendering. Remote + rows omit unmeasured statistics and all worktree mutation/file/context + actions; GitHub errors do not fall back locally. +- Verification: focused Vitest coverage passed (7 files, 72 tests), followed + by web typecheck, lint, i18n check, i18n ratchet, and `git diff --check`. +- Review remediation: keyed PR commit results by the complete workspace, + repository, pull request, and refresh identity so rapid PR switches cannot + reuse stale rows. Unavailable clients and malformed commit responses now + remain errors with a retry state, and GitHub detail requests no longer + refetch when local agent readiness changes. The remediation suite passed + (8 files, 69 tests), with additional focused regressions covering the PR + switch, protocol failures, retry rendering, and remote dependency boundary. +- Review follow-up: normalized missing repository names to the workspace-root + identity, rejected malformed backend and frontend commit-detail payloads, + bound loaded detail state to the selected target, hardened serialized GitHub + target validation, and corrected the late-response regression. The rebased + verification passed with 1,086 backend GitHub tests and 73 focused frontend + tests. diff --git a/docs/plans/pr-only-commit-details/task-03-commit-source-e2e.md b/docs/plans/pr-only-commit-details/task-03-commit-source-e2e.md new file mode 100644 index 0000000000..975aa4b0be --- /dev/null +++ b/docs/plans/pr-only-commit-details/task-03-commit-source-e2e.md @@ -0,0 +1,60 @@ +--- +id: "03-commit-source-e2e" +title: "Commit-source E2E coverage" +status: done +wave: 3 +depends_on: ["01-github-commit-detail", "02-source-aware-commit-ui"] +plan: "plan.md" +spec: "../../specs/pr-only-commit-details/spec.md" +--- + +# Task 03: Commit-source E2E coverage + +- **Acceptance:** Deterministic GitHub mock support can seed an individual + commit detail independently of the local worktree; desktop E2E proves a + PR-only commit omits false zero stats and local mutation actions and renders + the remote header/patch; mobile E2E proves the same remote patch opens in the + existing full-height sheet, has no local action, closes normally, and causes + no horizontal overflow; the scenario cannot pass by serving a local + `session.commit_diff` response for the remote SHA. +- **Verification:** Follow strict TDD. Add mock-controller and Playwright + regressions first; record the backend/mock failure and browser failure against + the pre-task state. After the minimal mock and test wiring, run + `cd apps/backend && go test -run 'Test.*Mock.*Commit' ./internal/github`, + `cd apps/web && pnpm e2e:run tests/git/git-changes-panel.spec.ts -- --grep "PR-only commit"`, + `cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-changes-panel.spec.ts -- --grep "PR-only commit"`, + and `git diff --check`. +- **Files likely touched:** + `apps/backend/internal/github/mock_client.go`; + `apps/backend/internal/github/mock_controller.go`; + their focused tests; + `apps/web/e2e/helpers/api-client.ts`; + `apps/web/e2e/tests/git/git-changes-panel.spec.ts`; and + `apps/web/e2e/tests/task/mobile-changes-panel.spec.ts`. +- **Dependencies:** Tasks 01 and 02 complete with stable backend and frontend + source-routing contracts. +- **Parallelism:** sequential — mock seeding must match the settled response + contract, and the desktop/mobile tests jointly validate one integrated source + route. +- **Inputs:** repair spec regression scenarios; Task 01 individual-commit mock + model; Task 02 source-aware targets; existing `mockGitHubAddPRCommits`, + Git Changes panel fixtures, and mobile Changes panel full-height drawer tests. +- **Output contract:** Report the deterministic stale-worktree fixture, how the + test prevents local fallback, exact files changed, backend and desktop/mobile + RED/GREEN results, cleanup state, any quarantined instability, and + synchronized task/plan status. + +## Results + +- GREEN: added deterministic mock individual-commit seeding through the + existing mock GitHub controller and API helper. Desktop and mobile fixtures + use PR-only SHAs absent from the stale local worktree, so the rendered + metadata and patch require the GitHub detail response. +- Desktop verification passed with + `pnpm e2e:run tests/git/git-changes-panel.spec.ts -- --grep "PR-only commit"`. +- Mobile verification passed with + `pnpm e2e:run --project mobile-chrome tests/task/mobile-changes-panel.spec.ts -- --grep "PR-only commit"`. + The first mobile attempt only exposed a strict-locator ambiguity in the test + assertion; narrowing it to the sheet passed the same scenario. +- No local mutation action appeared, the remote patch marker rendered, the + mobile sheet closed normally, and the horizontal-overflow check passed. diff --git a/docs/specs/pr-only-commit-details/spec.md b/docs/specs/pr-only-commit-details/spec.md new file mode 100644 index 0000000000..143498c8f5 --- /dev/null +++ b/docs/specs/pr-only-commit-details/spec.md @@ -0,0 +1,111 @@ +--- +title: Repair PR-only commit details +status: done +created: 2026-08-04 +owner: kandev +--- + +# Repair PR-only commit details + +## Problem + +The Changes panel combines commits discovered in a task session's local +worktree with commits returned by the task's linked GitHub pull request. When a +pull request is force-pushed after the task worktree is created, those sources +can describe different histories. + +PR-only rows currently inherit zero-valued statistics from GitHub's pull +request commit-list response and are opened through the local +`session.commit_diff` action. The list response does not contain commit stats, +and the remote commit may not exist in the task worktree. As a result, Kandev +can show false `+0 -0` totals, an empty or failed detail view, metadata from the +wrong source, and local reset/revert/amend controls for a commit that is not in +the local branch. + +## Desired behavior + +- A commit row and its detail view use one explicit source of truth. +- A SHA present in the local session commit feed is a **local commit**, even + when the same SHA is also present in the linked pull request. It keeps the + current local `session.commit_diff` behavior and local commit actions. +- A SHA present only in the selected linked GitHub pull request is a + **GitHub commit**. Its source identity includes the workspace, owner, + repository, and SHA. +- A GitHub commit whose list response has no statistics does not display a + fabricated `+0 -0`. A known empty commit may display zero additions and zero + deletions only when those values came from commit-detail data. +- Opening a GitHub commit lazily loads its metadata, aggregate statistics, + changed files, and patches from GitHub's individual commit endpoint. It does + not depend on the commit object being present in the task worktree. +- Desktop and mobile commit details render the same source-aware data. +- GitHub-only commit details are read-only relative to the task worktree. They + do not expose reset, revert, amend, open-current-worktree-file, or local + context-expansion controls. +- A GitHub request failure produces the existing visible error treatment and + never silently falls back to local `git show` data. +- Viewing a GitHub commit never fetches, checks out, resets, or otherwise + mutates the task worktree. + +## API contract + +Add a workspace-scoped WebSocket request for an individual GitHub commit. The +request carries `workspace_id`, `owner`, `repo`, and an exact commit SHA. The +response carries commit metadata, exact additions/deletions/files-changed +statistics, and the existing GitHub-style file records needed by the diff +viewer. + +The backend must authorize the requested repository through the same +workspace-scoped credential and repository checks used by the existing GitHub +pull request actions. Pull request commit-list data must distinguish +unavailable statistics from known zero statistics; omitted data must not be +serialized or interpreted as measured zeroes. + +The individual-commit response must merge GitHub's paginated file records so a +commit is not presented as complete after only the first page. Provider limits +or unavailable patches remain explicit upstream limitations rather than a +reason to consult the local worktree. + +## Regression scenarios + +- **GIVEN** a task worktree at an obsolete pull request head and a linked pull + request that was force-pushed to new SHAs, **WHEN** the user opens a new + PR-only commit, **THEN** Kandev shows the GitHub commit's metadata, exact + statistics, files, and patches without invoking the local commit-diff path. +- **GIVEN** a PR-only commit returned by the pull request commit-list endpoint, + **WHEN** its row is rendered before detail data is loaded, **THEN** the row + omits statistics instead of displaying `+0 -0`. +- **GIVEN** the same SHA is present in both the local and pull request feeds, + **WHEN** the user opens it, **THEN** it is deduplicated as a local commit and + keeps the existing local detail path and local actions. +- **GIVEN** two linked repositories contain PR-only commits, **WHEN** either + commit is opened, **THEN** the request uses that row's workspace, owner, + repository, and SHA rather than the active worktree's repository. +- **GIVEN** a GitHub-only commit on mobile, **WHEN** the user taps the row, + **THEN** the existing full-height commit sheet renders the remote patch and + closes normally without exposing local commit actions. +- **GIVEN** GitHub rejects or cannot serve the individual commit request, + **WHEN** the user opens the row, **THEN** Kandev shows an error and does not + substitute local worktree content. + +## Constraints + +- Preserve the current local commit-detail flow and action semantics. +- Fetch individual GitHub commit details lazily; do not issue one request per + row while rendering the commit list. +- Reuse the current desktop panel and mobile full-height sheet rather than + adding a new detail composition. +- Keep GitHub patches in their existing patch representation, including the + current binary, unavailable, or truncated-patch treatment. +- Route any changed user-facing copy through i18n. No new explanatory copy is + required for this repair. + +## Out of scope + +- Automatically fetching or checking out the pull request head in the task + worktree. +- Resetting or rebasing a stale task branch. +- Adding a general stale-worktree warning or synchronization workflow. +- Eagerly enriching every pull request commit row with individual commit data. +- Parent selection or combined-diff controls for merge commits. +- Persistence or database schema changes. +- Adding equivalent remote-commit providers for non-GitHub integrations.