fix: route PR-only commits to GitHub details (#2268)
This commit is contained in:
Carlos Florêncio
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"`
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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{
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 };
|
||||
|
||||
@@ -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<PRCommitForMerge[]>(() => {
|
||||
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,
|
||||
|
||||
@@ -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<string>,
|
||||
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<string>();
|
||||
const matchedPRKeys = new Set<string>();
|
||||
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];
|
||||
}
|
||||
|
||||
|
||||
@@ -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 },
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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;
|
||||
|
||||
@@ -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,
|
||||
},
|
||||
]}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<typeof makeLocal>[] = [];
|
||||
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,
|
||||
},
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
|
||||
@@ -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 }) => <div>{children}</div>,
|
||||
PanelBody: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
|
||||
}));
|
||||
|
||||
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(
|
||||
<CommitDiffView
|
||||
target={{
|
||||
source: "github",
|
||||
sha: "remote123",
|
||||
workspaceId: "workspace-1",
|
||||
owner: "acme",
|
||||
repo: "widget",
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
|
||||
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(
|
||||
<CommitDetailPanel
|
||||
panelId="commit-detail"
|
||||
params={{ target: { source: "github", sha: "partial" } }}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(mocks.useCommitDetail).toHaveBeenCalledWith({ source: "local", sha: "" });
|
||||
});
|
||||
});
|
||||
@@ -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<string, unknown>): 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<string, FileInfo> | 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<typeof useActiveCommit>,
|
||||
remoteCommit: ReturnType<typeof useCommitDetail>["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({
|
||||
</div>
|
||||
);
|
||||
}
|
||||
if (error) return <CommitDetailErrorState error={error} onRetry={refetch} />;
|
||||
|
||||
return (
|
||||
<div className="overflow-y-auto">
|
||||
<div className="p-3">{commit && <CommitHeader commit={commit} commitSha={commitSha} />}</div>
|
||||
<div className="p-3">{commit && <CommitHeader commit={commit} commitSha={target.sha} />}</div>
|
||||
<CommitFileList
|
||||
fileEntries={fileEntries}
|
||||
loading={loading}
|
||||
onOpenFile={onOpenFile ?? (() => {})}
|
||||
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}
|
||||
/>
|
||||
</div>
|
||||
@@ -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({
|
||||
</PanelRoot>
|
||||
);
|
||||
}
|
||||
if (error) {
|
||||
return (
|
||||
<PanelRoot>
|
||||
<PanelBody>
|
||||
<CommitDetailErrorState error={error} onRetry={refetch} />
|
||||
</PanelBody>
|
||||
</PanelRoot>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<PanelRoot>
|
||||
<PanelBody padding={false} scroll>
|
||||
<div className="p-3">
|
||||
{commit && <CommitHeader commit={commit} commitSha={commitSha} />}
|
||||
{commit && <CommitHeader commit={commit} commitSha={target.sha} />}
|
||||
</div>
|
||||
<CommitFileList
|
||||
fileEntries={fileEntries}
|
||||
loading={loading}
|
||||
onOpenFile={openFile}
|
||||
baseRef={`${commitSha}^`}
|
||||
repo={repo}
|
||||
onOpenFile={target.source === "local" ? openFile : undefined}
|
||||
baseRef={target.source === "local" ? `${target.sha}^` : undefined}
|
||||
repo={target.source === "local" ? target.repo : undefined}
|
||||
remote={target.source === "github"}
|
||||
/>
|
||||
</PanelBody>
|
||||
</PanelRoot>
|
||||
@@ -168,6 +237,35 @@ function CommitHeader({
|
||||
);
|
||||
}
|
||||
|
||||
function CommitDetailErrorState({
|
||||
error,
|
||||
onRetry,
|
||||
}: {
|
||||
error: string;
|
||||
onRetry: () => Promise<void>;
|
||||
}) {
|
||||
const { t } = useTranslation();
|
||||
return (
|
||||
<div
|
||||
role="alert"
|
||||
className="flex h-full flex-col items-center justify-center gap-3 px-6 text-center text-sm text-muted-foreground"
|
||||
>
|
||||
<IconAlertCircle className="h-5 w-5 text-destructive" />
|
||||
<span>{error}</span>
|
||||
<Button
|
||||
type="button"
|
||||
className="min-h-11 cursor-pointer"
|
||||
variant="outline"
|
||||
size="sm"
|
||||
onClick={() => void onRetry()}
|
||||
>
|
||||
<IconRefresh className="h-4 w-4" />
|
||||
{t("system:featureTogglesRetry")}
|
||||
</Button>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
/** 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}
|
||||
/>
|
||||
) : (
|
||||
|
||||
@@ -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<string, FileInfo> = {
|
||||
"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: "",
|
||||
},
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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<string, FileInfo>;
|
||||
};
|
||||
|
||||
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<string, unknown>;
|
||||
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<string, unknown>;
|
||||
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<string, FileInfo> {
|
||||
return Object.fromEntries(detail.files.map((file) => [file.filename, mapGitHubFile(file)]));
|
||||
}
|
||||
|
||||
async function requestGitHubCommitDetail(
|
||||
target: Extract<CommitDetailTarget, { source: "github" }>,
|
||||
): Promise<CommitDetailRequestResult> {
|
||||
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<CommitDetailRequestResult> {
|
||||
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 };
|
||||
}
|
||||
@@ -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 }) => <div>{children}</div>,
|
||||
ContextMenuItem: ({
|
||||
children,
|
||||
onSelect,
|
||||
}: {
|
||||
children: React.ReactNode;
|
||||
onSelect: () => void;
|
||||
}) => (
|
||||
<button type="button" onClick={onSelect}>
|
||||
{children}
|
||||
</button>
|
||||
),
|
||||
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(
|
||||
<CommitRow
|
||||
commit={remoteCommit()}
|
||||
isLatest
|
||||
onAmendCommit={vi.fn()}
|
||||
onRevertCommit={vi.fn()}
|
||||
onResetToCommit={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
|
||||
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(<CommitRow commit={commit} isLatest onOpenCommitDetail={onOpenCommitDetail} />);
|
||||
|
||||
fireEvent.click(screen.getByTestId("commit-row-remote1"));
|
||||
expect(onOpenCommitDetail).toHaveBeenCalledWith(commit.detailTarget);
|
||||
});
|
||||
});
|
||||
@@ -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 (
|
||||
<CommitContextMenu
|
||||
@@ -200,11 +206,11 @@ export function CommitRow({
|
||||
tabIndex={0}
|
||||
data-testid={`commit-row-${commit.commit_sha.slice(0, 7)}`}
|
||||
className="group relative flex items-center gap-2 text-xs rounded-md px-1 py-1 -mx-1 hover:bg-muted/60 cursor-pointer"
|
||||
onClick={() => 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)}
|
||||
</code>
|
||||
<span className="flex-1 min-w-0 truncate text-foreground">{commit.commit_message}</span>
|
||||
<span
|
||||
className={`shrink-0 text-[11px] flex items-center gap-1 mr-1 ${commit.committed_at ? "group-hover:hidden" : ""}`}
|
||||
>
|
||||
<span className="text-emerald-500">+{commit.insertions}</span>{" "}
|
||||
<span className="text-rose-500">-{commit.deletions}</span>
|
||||
</span>
|
||||
{commit.statsAvailable && (
|
||||
<span
|
||||
className={`shrink-0 text-[11px] flex items-center gap-1 mr-1 ${commit.committed_at ? "group-hover:hidden" : ""}`}
|
||||
>
|
||||
<span className="text-emerald-500">+{commit.insertions}</span>{" "}
|
||||
<span className="text-rose-500">-{commit.deletions}</span>
|
||||
</span>
|
||||
)}
|
||||
{commit.committed_at && (
|
||||
<span className="hidden group-hover:flex shrink-0 text-[11px] items-center gap-1 mr-1 text-muted-foreground">
|
||||
{timeAgo(commit.committed_at)}
|
||||
|
||||
@@ -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]);
|
||||
|
||||
@@ -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]);
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
|
||||
@@ -161,7 +161,7 @@ function renderPanel(
|
||||
): React.ReactNode {
|
||||
if (!mode) return null;
|
||||
if (mode.kind === "commit") {
|
||||
return <CommitDiffView sha={mode.sha} repo={mode.repo} onOpenFile={onOpenFile} wordWrap />;
|
||||
return <CommitDiffView target={mode.target} onOpenFile={onOpenFile} wordWrap />;
|
||||
}
|
||||
const panelMode = mode.kind;
|
||||
const filePath = mode.kind === "file" ? mode.path : undefined;
|
||||
|
||||
@@ -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<void> {
|
||||
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<PRCommitDetail, "sha"> & { sha?: string },
|
||||
): Promise<void> {
|
||||
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,
|
||||
|
||||
@@ -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`,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<T>() {
|
||||
let resolve!: (value: T) => void;
|
||||
const promise = new Promise<T>((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<AppState>;
|
||||
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");
|
||||
});
|
||||
});
|
||||
@@ -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<WorkspacePRCommitsState>({
|
||||
workspaceId: null,
|
||||
state: INITIAL_STATE,
|
||||
});
|
||||
const hasParams = !!workspaceId && !!owner && !!repo && !!prNumber;
|
||||
const sourceKey = hasParams
|
||||
? `${workspaceId}/${owner}/${repo}/${prNumber}/${refreshKey ?? ""}`
|
||||
: "";
|
||||
const [state, setState] = useState<KeyedPRCommitsState>(INITIAL_STATE);
|
||||
const paramsKeyRef = useRef<string>("");
|
||||
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 };
|
||||
}
|
||||
|
||||
@@ -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<string, FileInfo> = {
|
||||
"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<T>() {
|
||||
let resolve!: (value: T) => void;
|
||||
const promise = new Promise<T>((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<string, FileInfo>;
|
||||
commit: { sha: string; message: string };
|
||||
}>();
|
||||
const second = deferred<{
|
||||
source: "github";
|
||||
success: true;
|
||||
files: Record<string, FileInfo>;
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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<string, FileInfo> | null;
|
||||
commit: PRCommitDetail | null;
|
||||
loading: boolean;
|
||||
error: string | null;
|
||||
refetch: () => Promise<void>;
|
||||
};
|
||||
|
||||
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<Record<string, FileInfo> | null>(null);
|
||||
const [commit, setCommit] = useState<PRCommitDetail | null>(null);
|
||||
const [loading, setLoading] = useState(false);
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
const [loadedKey, setLoadedKey] = useState<string | null>(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,
|
||||
};
|
||||
}
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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<string, unknown>;
|
||||
group: { id: string };
|
||||
isActive: boolean;
|
||||
api: {
|
||||
setActive: () => void;
|
||||
updateParameters: (p: Record<string, unknown>) => 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<string, unknown>) {
|
||||
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<StoreShape>) => 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;
|
||||
@@ -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<string, unknown>;
|
||||
group: { id: string };
|
||||
isActive: boolean;
|
||||
api: {
|
||||
setActive: () => void;
|
||||
updateParameters: (p: Record<string, unknown>) => 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<string, unknown>) {
|
||||
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<StoreShape>) => 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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user