From a6326fdc5d9a0d5168810afc6f5c2949a3bfda91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sun, 2 Aug 2026 17:00:32 +0100 Subject: [PATCH] feat: unlink task pull requests (#2114) --- apps/backend/internal/events/types.go | 1 + .../gateway/websocket/task_notifications.go | 1 + .../websocket/task_notifications_test.go | 1 + apps/backend/internal/github/controller.go | 21 ++ .../internal/github/controller_test.go | 51 ++++ apps/backend/internal/github/models.go | 1 + .../internal/github/service_pr_watch.go | 29 ++- .../internal/github/service_task_pr_detach.go | 67 ++++++ .../github/service_task_pr_detach_test.go | 220 ++++++++++++++++++ apps/backend/internal/github/store.go | 101 ++++++-- .../github/store_task_pr_detach_test.go | 151 ++++++++++++ apps/backend/pkg/websocket/actions.go | 1 + .../components/github/multi-pr-ci-popover.tsx | 116 +++++++-- .../github/pr-ci-popover.automation.test.tsx | 117 +++++++++- .../components/github/pr-status-chip.test.tsx | 21 +- apps/web/components/github/pr-status-chip.tsx | 97 +++++--- .../components/github/pr-topbar-button.tsx | 44 +++- apps/web/e2e/pages/session-page.ts | 6 + .../e2e/tests/pr/mobile-pr-ci-chip.spec.ts | 103 ++++++++ .../web/e2e/tests/pr/pr-multi-popover.spec.ts | 54 ++++- .../hooks/domains/github/use-task-pr.test.tsx | 64 +++++ apps/web/hooks/domains/github/use-task-pr.ts | 16 +- apps/web/lib/api/domains/github-api.test.ts | 15 ++ apps/web/lib/api/domains/github-api.ts | 18 ++ .../state/slices/github/github-slice.test.ts | 27 +++ .../lib/state/slices/github/github-slice.ts | 15 +- apps/web/lib/state/slices/github/types.ts | 1 + apps/web/lib/types/backend.ts | 8 +- apps/web/lib/types/github.ts | 7 + apps/web/lib/ws/handlers/github.test.ts | 26 +++ apps/web/lib/ws/handlers/github.ts | 13 +- docs/plans/unlink-task-github-pr/plan.md | 187 +++++++++++++++ .../task-01-backend-unlink-contract.md | 70 ++++++ .../task-02-frontend-multi-pr-unlink.md | 75 ++++++ .../task-03-pr-unlink-e2e.md | 66 ++++++ .../task-04-public-documentation.md | 58 +++++ docs/public/sessions-and-review.md | 2 + .../tasks/link-existing-task-github-issue.md | 46 ++++ 38 files changed, 1808 insertions(+), 109 deletions(-) create mode 100644 apps/backend/internal/github/service_task_pr_detach.go create mode 100644 apps/backend/internal/github/service_task_pr_detach_test.go create mode 100644 apps/backend/internal/github/store_task_pr_detach_test.go create mode 100644 docs/plans/unlink-task-github-pr/plan.md create mode 100644 docs/plans/unlink-task-github-pr/task-01-backend-unlink-contract.md create mode 100644 docs/plans/unlink-task-github-pr/task-02-frontend-multi-pr-unlink.md create mode 100644 docs/plans/unlink-task-github-pr/task-03-pr-unlink-e2e.md create mode 100644 docs/plans/unlink-task-github-pr/task-04-public-documentation.md diff --git a/apps/backend/internal/events/types.go b/apps/backend/internal/events/types.go index 4563d07248..78638ec988 100644 --- a/apps/backend/internal/events/types.go +++ b/apps/backend/internal/events/types.go @@ -270,6 +270,7 @@ const ( GitHubNewReviewPR = "github.new_pr_to_review" // New PR found needing review GitHubNewIssue = "github.new_issue" // New issue found matching issue watch GitHubTaskPRUpdated = "github.task_pr.updated" // TaskPR record updated (for UI refresh) + GitHubTaskPRDeleted = "github.task_pr.deleted" // TaskPR association detached (for UI refresh) GitHubTaskCIOptionsUpdated = "github.task_ci_options.updated" // Task CI automation options updated GitHubWatchEvent = "github.watch.event" // Watch created/deleted GitHubRateLimitUpdated = "github.rate_limit.updated" // GitHub API rate-limit snapshot changed diff --git a/apps/backend/internal/gateway/websocket/task_notifications.go b/apps/backend/internal/gateway/websocket/task_notifications.go index d7e0d0c89e..d85b95c35d 100644 --- a/apps/backend/internal/gateway/websocket/task_notifications.go +++ b/apps/backend/internal/gateway/websocket/task_notifications.go @@ -83,6 +83,7 @@ func RegisterTaskNotifications(ctx context.Context, eventBus bus.EventBus, hub * b.subscribe(eventBus, events.TurnCompleted, ws.ActionSessionTurnCompleted) b.subscribe(eventBus, events.MessageQueueStatusChanged, ws.ActionMessageQueueStatusChanged) b.subscribe(eventBus, events.GitHubTaskPRUpdated, ws.ActionGitHubTaskPRUpdated) + b.subscribe(eventBus, events.GitHubTaskPRDeleted, ws.ActionGitHubTaskPRDeleted) b.subscribe(eventBus, events.GitHubTaskCIOptionsUpdated, ws.ActionGitHubTaskCIOptionsUpdated) b.subscribe(eventBus, events.GitHubRateLimitUpdated, ws.ActionGitHubRateLimitUpdated) b.subscribe(eventBus, events.GitLabTaskMRUpdated, ws.ActionGitLabTaskMRUpdated) diff --git a/apps/backend/internal/gateway/websocket/task_notifications_test.go b/apps/backend/internal/gateway/websocket/task_notifications_test.go index eb01f6dcb7..9654dee183 100644 --- a/apps/backend/internal/gateway/websocket/task_notifications_test.go +++ b/apps/backend/internal/gateway/websocket/task_notifications_test.go @@ -123,6 +123,7 @@ func TestTaskEventBroadcaster_NoDuplicateSubscriptions(t *testing.T) { events.TaskSessionStateChanged, events.TaskSessionActivityChanged, events.GitHubTaskPRUpdated, + events.GitHubTaskPRDeleted, events.GitLabTaskMRUpdated, } { subject := subject diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index fb02974863..f4d080aa31 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -71,6 +71,7 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) { api.GET("/task-prs", c.httpListTaskPRs) api.POST("/task-prs", c.httpCreateTaskPR) + api.DELETE("/task-prs/:associationId", c.httpDeleteTaskPR) api.GET("/task-prs/:taskId", c.httpGetTaskPR) api.GET("/task-issues", c.httpListTaskIssues) api.PUT("/tasks/:taskId/issue", c.httpLinkTaskIssue) @@ -249,6 +250,26 @@ func (c *Controller) httpCreateTaskPR(ctx *gin.Context) { ctx.JSON(http.StatusOK, tp) } +func (c *Controller) httpDeleteTaskPR(ctx *gin.Context) { + workspaceID := ctx.Query("workspace_id") + if workspaceID == "" { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "workspace_id query parameter required"}) + return + } + associationID := ctx.Param("associationId") + if _, err := c.service.DetachTaskPR(ctx.Request.Context(), workspaceID, associationID); err != nil { + if errors.Is(err, ErrTaskPRNotFound) || writeWatchWorkspaceError(ctx, "task PR", err) { + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "task PR not found"}) + } + return + } + ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + ctx.JSON(http.StatusOK, gin.H{"deleted": true}) +} + func (c *Controller) httpGetTaskPR(ctx *gin.Context) { taskID := ctx.Param("taskId") tp, err := c.service.GetTaskPR(ctx.Request.Context(), taskID) diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index 5d68986202..bad5a72a0f 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -312,6 +312,57 @@ func TestCredentialBrokerReadinessUsesExactResolveRoute(t *testing.T) { } } +func TestHttpDeleteTaskPRDetachesOnlyTheRequestedWorkspaceAssociation(t *testing.T) { + router, store := setupControllerStoreTest(t) + ctx := context.Background() + first := &TaskPR{ + WorkspaceID: "ws-1", TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "demo", + PRNumber: 10, PRURL: "https://github.com/acme/demo/pull/10", PRTitle: "old", HeadBranch: "old", + BaseBranch: "main", State: "merged", CreatedAt: time.Now().UTC(), + } + second := &TaskPR{ + WorkspaceID: "ws-1", TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "demo", + PRNumber: 11, PRURL: "https://github.com/acme/demo/pull/11", PRTitle: "new", HeadBranch: "new", + BaseBranch: "main", State: "open", CreatedAt: time.Now().UTC().Add(time.Second), + } + if err := store.CreateTaskPR(ctx, first); err != nil { + t.Fatalf("create first PR: %v", err) + } + if err := store.CreateTaskPR(ctx, second); err != nil { + t.Fatalf("create second PR: %v", err) + } + + request := httptest.NewRequest(http.MethodDelete, "/api/v1/github/task-prs/"+first.ID+"?workspace_id=ws-1", nil) + response := httptest.NewRecorder() + router.ServeHTTP(response, request) + if response.Code != http.StatusOK { + t.Fatalf("delete status = %d, body = %s", response.Code, response.Body.String()) + } + var body struct { + Deleted bool `json:"deleted"` + } + if err := json.NewDecoder(response.Body).Decode(&body); err != nil { + t.Fatalf("decode delete response: %v", err) + } + if !body.Deleted { + t.Fatal("delete response deleted=false, want true") + } + active, err := store.ListTaskPRsByTask(ctx, "task-1") + if err != nil { + t.Fatalf("list active PRs: %v", err) + } + if len(active) != 1 || active[0].ID != second.ID { + t.Fatalf("active PRs = %+v, want second association only", active) + } + + request = httptest.NewRequest(http.MethodDelete, "/api/v1/github/task-prs/"+second.ID+"?workspace_id=other", nil) + response = httptest.NewRecorder() + router.ServeHTTP(response, request) + if response.Code != http.StatusNotFound { + t.Fatalf("cross-workspace delete status = %d, want 404", response.Code) + } +} + func TestHttpListTaskIssues_WorkspaceScopedMetadataLinks(t *testing.T) { router, store := setupControllerStoreTest(t) rows := []struct { diff --git a/apps/backend/internal/github/models.go b/apps/backend/internal/github/models.go index 17fe7c8007..06b34fee59 100644 --- a/apps/backend/internal/github/models.go +++ b/apps/backend/internal/github/models.go @@ -432,6 +432,7 @@ type TaskPR struct { MergedAt *time.Time `json:"merged_at,omitempty" db:"merged_at"` ClosedAt *time.Time `json:"closed_at,omitempty" db:"closed_at"` LastSyncedAt *time.Time `json:"last_synced_at,omitempty" db:"last_synced_at"` + DetachedAt *time.Time `json:"-" db:"detached_at"` UpdatedAt time.Time `json:"updated_at" db:"updated_at"` } diff --git a/apps/backend/internal/github/service_pr_watch.go b/apps/backend/internal/github/service_pr_watch.go index 721e8dcf3e..8240985fcd 100644 --- a/apps/backend/internal/github/service_pr_watch.go +++ b/apps/backend/internal/github/service_pr_watch.go @@ -284,7 +284,7 @@ func (s *Service) ensurePRWatch( // callers MUST pass it — empty causes ReplaceTaskPR to wipe the entire task's // PR rows (legacy "delete all" branch), which is what older code relied on. func (s *Service) AssociatePRWithTask(ctx context.Context, taskID, repositoryID string, pr *PR) (*TaskPR, error) { - return s.associatePRWithTask(ctx, "", taskID, repositoryID, pr) + return s.associatePRWithTask(ctx, "", taskID, repositoryID, pr, false) } func (s *Service) AssociatePRWithTaskForWorkspace( @@ -293,11 +293,11 @@ func (s *Service) AssociatePRWithTaskForWorkspace( if strings.TrimSpace(workspaceID) == "" { return nil, ErrGitHubWorkspaceRequired } - return s.associatePRWithTask(ctx, workspaceID, taskID, repositoryID, pr) + return s.associatePRWithTask(ctx, workspaceID, taskID, repositoryID, pr, false) } func (s *Service) associatePRWithTask( - ctx context.Context, workspaceID, taskID, repositoryID string, pr *PR, + ctx context.Context, workspaceID, taskID, repositoryID string, pr *PR, restoreDetached bool, ) (*TaskPR, error) { // Multi-branch: scope the "already-current" short-circuit by exact // pr_number too. A task can hold multiple PR rows per (task, repo) on @@ -306,11 +306,24 @@ func (s *Service) associatePRWithTask( // secondary PR is already there (wrong PR number) and skip the insert // — or worse, fall through to ReplaceTaskPR which used to delete the // sibling row. - existing, err := s.store.GetTaskPRByRepoAndNumber(ctx, taskID, repositoryID, pr.Number) + existing, err := s.store.GetTaskPRByRepoAndNumberIncludingDetached(ctx, taskID, repositoryID, pr.Number) if err != nil { return nil, err } if existing != nil { + if existing.DetachedAt != nil && restoreDetached { + restored, restoreErr := s.store.RestoreTaskPR(ctx, taskID, repositoryID, pr) + if restoreErr != nil || restored == nil { + return restored, restoreErr + } + if s.eventBus != nil { + event := bus.NewEvent(events.GitHubTaskPRUpdated, "github", restored) + if err := s.eventBus.Publish(ctx, events.GitHubTaskPRUpdated, event); err != nil { + s.logger.Debug("failed to publish restored task PR event", zap.Error(err)) + } + } + return restored, nil + } return existing, nil } tp := &TaskPR{ @@ -380,7 +393,7 @@ func (s *Service) AssociateExistingPRByURL(ctx context.Context, taskID, reposito if err != nil { return nil, fmt.Errorf("fetch PR: %w", err) } - tp, err := s.AssociatePRWithTask(ctx, taskID, repositoryID, pr) + tp, err := s.associatePRWithTask(ctx, "", taskID, repositoryID, pr, true) if err != nil { return nil, fmt.Errorf("associate PR with task: %w", err) } @@ -405,7 +418,7 @@ func (s *Service) AssociateExistingPRByURLForWorkspace( if err != nil { return nil, fmt.Errorf("fetch PR: %w", err) } - tp, err := s.AssociatePRWithTaskForWorkspace(ctx, workspaceID, taskID, repositoryID, pr) + tp, err := s.associatePRWithTask(ctx, workspaceID, taskID, repositoryID, pr, true) if err != nil { return nil, fmt.Errorf("associate PR with task: %w", err) } @@ -445,7 +458,7 @@ func (s *Service) AssociatePRByURL(ctx context.Context, sessionID, taskID, repos } // Associate PR with task (persists + publishes WS event) - if _, assocErr := s.AssociatePRWithTask(ctx, taskID, repositoryID, pr); assocErr != nil { + if _, assocErr := s.associatePRWithTask(ctx, "", taskID, repositoryID, pr, true); assocErr != nil { s.logger.Error("failed to associate PR with task after creation", zap.String("task_id", taskID), zap.Error(assocErr)) } @@ -477,7 +490,7 @@ func (s *Service) AssociatePRByURLForWorkspace( ); err != nil { return fmt.Errorf("create PR watch: %w", err) } - if _, err := s.AssociatePRWithTaskForWorkspace(ctx, workspaceID, taskID, repositoryID, pr); err != nil { + if _, err := s.associatePRWithTask(ctx, workspaceID, taskID, repositoryID, pr, true); err != nil { return fmt.Errorf("associate PR with task: %w", err) } return nil diff --git a/apps/backend/internal/github/service_task_pr_detach.go b/apps/backend/internal/github/service_task_pr_detach.go new file mode 100644 index 0000000000..28b953ce66 --- /dev/null +++ b/apps/backend/internal/github/service_task_pr_detach.go @@ -0,0 +1,67 @@ +package github + +import ( + "context" + "errors" + "strings" + + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" + "go.uber.org/zap" +) + +var ErrTaskPRNotFound = errors.New("github task PR not found") + +// TaskPRDeletedEvent identifies one task-PR association removed from active +// task surfaces. The remote GitHub pull request is intentionally untouched. +type TaskPRDeletedEvent struct { + WorkspaceID string `json:"workspace_id"` + TaskID string `json:"task_id"` + AssociationID string `json:"association_id"` +} + +// GetWorkspaceID lets the websocket broadcaster route the typed event to the +// owning workspace instead of treating it as an instance-wide notification. +func (e TaskPRDeletedEvent) GetWorkspaceID() string { return e.WorkspaceID } + +// DetachTaskPR removes one association from active task views while retaining +// a durable tombstone that prevents background discovery from resurrecting it. +func (s *Service) DetachTaskPR(ctx context.Context, workspaceID, associationID string) (*TaskPR, error) { + if strings.TrimSpace(workspaceID) == "" || strings.TrimSpace(associationID) == "" { + return nil, ErrTaskPRNotFound + } + tp, err := s.store.GetTaskPRByID(ctx, associationID) + if err != nil { + return nil, err + } + if tp == nil || (tp.WorkspaceID != "" && tp.WorkspaceID != workspaceID) { + return nil, ErrTaskPRNotFound + } + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return nil, err + } + if tp.DetachedAt != nil { + return tp, nil + } + detached, transitioned, err := s.store.DetachTaskPR(ctx, associationID) + if err != nil { + return nil, err + } + if detached == nil { + return nil, ErrTaskPRNotFound + } + if !transitioned { + return detached, nil + } + if s.eventBus != nil { + event := bus.NewEvent(events.GitHubTaskPRDeleted, "github", &TaskPRDeletedEvent{ + WorkspaceID: workspaceID, + TaskID: detached.TaskID, + AssociationID: detached.ID, + }) + if err := s.eventBus.Publish(ctx, events.GitHubTaskPRDeleted, event); err != nil { + s.logger.Debug("failed to publish task PR deleted event", zap.Error(err)) + } + } + return detached, nil +} diff --git a/apps/backend/internal/github/service_task_pr_detach_test.go b/apps/backend/internal/github/service_task_pr_detach_test.go new file mode 100644 index 0000000000..990ffd6027 --- /dev/null +++ b/apps/backend/internal/github/service_task_pr_detach_test.go @@ -0,0 +1,220 @@ +package github + +import ( + "context" + "reflect" + "testing" + "time" +) + +func TestServiceDetachTaskPRAuthorizesWorkspaceAndPublishesRemoval(t *testing.T) { + svc, store, eventBus := setupSyncTest(t) + contract, ok := any(svc).(interface { + DetachTaskPR(context.Context, string, string) (*TaskPR, error) + }) + if !ok { + t.Fatal("Service does not implement DetachTaskPR") + } + ctx := context.Background() + first := &TaskPR{ + WorkspaceID: "ws-1", TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "demo", + PRNumber: 1, PRURL: "https://github.com/acme/demo/pull/1", PRTitle: "old", HeadBranch: "old", + BaseBranch: "main", State: "merged", CreatedAt: time.Now().UTC(), + } + second := &TaskPR{ + WorkspaceID: "ws-1", TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "demo", + PRNumber: 2, PRURL: "https://github.com/acme/demo/pull/2", PRTitle: "new", HeadBranch: "new", + BaseBranch: "main", State: "open", CreatedAt: time.Now().UTC().Add(time.Second), + } + if err := store.CreateTaskPR(ctx, first); err != nil { + t.Fatalf("create first PR: %v", err) + } + if err := store.CreateTaskPR(ctx, second); err != nil { + t.Fatalf("create second PR: %v", err) + } + var authorizedWorkspace string + svc.SetWorkspaceAuthorizer(func(_ context.Context, workspaceID string) error { + authorizedWorkspace = workspaceID + return nil + }) + + detached, err := contract.DetachTaskPR(ctx, "ws-1", first.ID) + if err != nil { + t.Fatalf("detach task PR: %v", err) + } + if detached == nil || detached.ID != first.ID || detached.DetachedAt == nil { + t.Fatalf("detached = %+v, want stamped first row", detached) + } + if authorizedWorkspace != "ws-1" { + t.Fatalf("authorized workspace = %q, want ws-1", authorizedWorkspace) + } + active, err := store.ListTaskPRsByTask(ctx, "task-1") + if err != nil { + t.Fatalf("list active task PRs: %v", err) + } + if len(active) != 1 || active[0].ID != second.ID { + t.Fatalf("active task PRs = %+v, want sibling only", active) + } + if eventBus.publishedCount() != 1 { + t.Fatalf("published events = %d, want one deletion event", eventBus.publishedCount()) + } + eventBus.mu.Lock() + event := eventBus.events[0] + eventBus.mu.Unlock() + if event.Type != "github.task_pr.deleted" { + t.Fatalf("event type = %q, want github.task_pr.deleted", event.Type) + } + payload := reflect.ValueOf(event.Data) + if payload.Kind() == reflect.Pointer { + payload = payload.Elem() + } + if !payload.IsValid() || payload.FieldByName("AssociationID").String() != first.ID || payload.FieldByName("TaskID").String() != first.TaskID || payload.FieldByName("WorkspaceID").String() != first.WorkspaceID { + t.Fatalf("event payload = %#v, want first association identity", event.Data) + } +} + +func TestServiceDetachTaskPRFailsClosedForWorkspaceMismatch(t *testing.T) { + svc, store, _ := setupSyncTest(t) + contract, ok := any(svc).(interface { + DetachTaskPR(context.Context, string, string) (*TaskPR, error) + }) + if !ok { + t.Fatal("Service does not implement DetachTaskPR") + } + pr := &TaskPR{WorkspaceID: "ws-1", TaskID: "task-1", Owner: "acme", Repo: "demo", PRNumber: 1, CreatedAt: time.Now().UTC()} + if err := store.CreateTaskPR(context.Background(), pr); err != nil { + t.Fatalf("create PR: %v", err) + } + + _, err := contract.DetachTaskPR(context.Background(), "ws-other", pr.ID) + if err == nil || err.Error() != "github task PR not found" { + t.Fatalf("error = %v, want github task PR not found", err) + } + active, err := store.ListTaskPRsByTask(context.Background(), pr.TaskID) + if err != nil { + t.Fatalf("list task PRs: %v", err) + } + if len(active) != 1 || active[0].ID != pr.ID { + t.Fatalf("active task PRs = %+v, want association unchanged", active) + } +} + +func TestServiceDetachTaskPRAllowsLegacyEmptyWorkspace(t *testing.T) { + svc, store, eventBus := setupSyncTest(t) + ctx := context.Background() + pr := &TaskPR{ + TaskID: "task-legacy", Owner: "acme", Repo: "demo", PRNumber: 7, + PRURL: "https://github.com/acme/demo/pull/7", CreatedAt: time.Now().UTC(), + } + if err := store.CreateTaskPR(ctx, pr); err != nil { + t.Fatalf("create legacy PR: %v", err) + } + var authorizedWorkspace string + svc.SetWorkspaceAuthorizer(func(_ context.Context, workspaceID string) error { + authorizedWorkspace = workspaceID + return nil + }) + + detached, err := svc.DetachTaskPR(ctx, "ws-legacy", pr.ID) + if err != nil { + t.Fatalf("detach legacy PR: %v", err) + } + if detached == nil || detached.DetachedAt == nil { + t.Fatalf("detached legacy PR = %+v, want stamped row", detached) + } + if authorizedWorkspace != "ws-legacy" { + t.Fatalf("authorized workspace = %q, want ws-legacy", authorizedWorkspace) + } + if eventBus.publishedCount() != 1 { + t.Fatalf("published events = %d, want one deletion event", eventBus.publishedCount()) + } + eventBus.mu.Lock() + event := eventBus.events[0] + eventBus.mu.Unlock() + payload := reflect.ValueOf(event.Data) + if payload.Kind() == reflect.Pointer { + payload = payload.Elem() + } + if payload.FieldByName("WorkspaceID").String() != "ws-legacy" { + t.Fatalf("event workspace = %q, want ws-legacy", payload.FieldByName("WorkspaceID").String()) + } +} + +func TestAssociatePRWithTaskDoesNotResurrectDetachedAssociation(t *testing.T) { + svc, store, _ := setupSyncTest(t) + ctx := context.Background() + row := &TaskPR{ + WorkspaceID: "ws-1", TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "demo", + PRNumber: 3, PRURL: "https://github.com/acme/demo/pull/3", PRTitle: "old", HeadBranch: "old", + BaseBranch: "main", State: "open", CreatedAt: time.Now().UTC(), + } + if err := store.CreateTaskPR(ctx, row); err != nil { + t.Fatalf("create PR: %v", err) + } + if _, _, err := store.DetachTaskPR(ctx, row.ID); err != nil { + t.Fatalf("detach PR: %v", err) + } + + associated, err := svc.AssociatePRWithTask(ctx, row.TaskID, row.RepositoryID, &PR{ + Number: 3, RepoOwner: "acme", RepoName: "demo", HTMLURL: row.PRURL, Title: "old", HeadBranch: "old", BaseBranch: "main", State: "open", + }) + if err != nil { + t.Fatalf("associate PR: %v", err) + } + if associated == nil || associated.DetachedAt == nil { + t.Fatalf("associated = %+v, want detached tombstone to remain", associated) + } + active, err := store.ListTaskPRsByTask(ctx, row.TaskID) + if err != nil { + t.Fatalf("list active PRs: %v", err) + } + if len(active) != 0 { + t.Fatalf("active PRs = %+v, want no resurrected row", active) + } +} + +func TestAssociatePRWithTaskExplicitLinkRestoresDetachedAssociation(t *testing.T) { + svc, store, eventBus := setupSyncTest(t) + ctx := context.Background() + row := &TaskPR{ + WorkspaceID: "ws-1", TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "demo", + PRNumber: 4, PRURL: "https://github.com/acme/demo/pull/4", PRTitle: "old", HeadBranch: "old", + BaseBranch: "main", State: "open", CreatedAt: time.Now().UTC(), + } + if err := store.CreateTaskPR(ctx, row); err != nil { + t.Fatalf("create PR: %v", err) + } + if _, _, err := store.DetachTaskPR(ctx, row.ID); err != nil { + t.Fatalf("detach PR: %v", err) + } + + freshPR := &PR{ + Number: 4, RepoOwner: "acme", RepoName: "demo", HTMLURL: "https://github.com/acme/demo/pull/4", + Title: "fresh title", HeadBranch: "fresh-branch", BaseBranch: "develop", AuthorLogin: "bob", + State: "closed", MergeableState: "dirty", Additions: 8, Deletions: 3, + } + associated, err := svc.associatePRWithTask(ctx, row.WorkspaceID, row.TaskID, row.RepositoryID, freshPR, true) + if err != nil { + t.Fatalf("explicitly associate PR: %v", err) + } + if associated == nil || associated.DetachedAt != nil { + t.Fatalf("associated = %+v, want active restored row", associated) + } + if associated.PRURL != freshPR.HTMLURL || associated.PRTitle != freshPR.Title || + associated.HeadBranch != freshPR.HeadBranch || associated.BaseBranch != freshPR.BaseBranch || + associated.AuthorLogin != freshPR.AuthorLogin || associated.State != freshPR.State || + associated.MergeableState != freshPR.MergeableState || associated.Additions != freshPR.Additions || + associated.Deletions != freshPR.Deletions { + t.Fatalf("restored PR = %+v, want fresh GitHub data", associated) + } + active, err := store.ListTaskPRsByTask(ctx, row.TaskID) + if err != nil { + t.Fatalf("list active PRs: %v", err) + } + if len(active) != 1 || active[0].ID != row.ID { + t.Fatalf("active PRs = %+v, want restored row", active) + } + if eventBus.publishedCount() != 1 { + t.Fatalf("published events = %d, want one restoration update", eventBus.publishedCount()) + } +} diff --git a/apps/backend/internal/github/store.go b/apps/backend/internal/github/store.go index bfd5644fcc..dace925909 100644 --- a/apps/backend/internal/github/store.go +++ b/apps/backend/internal/github/store.go @@ -134,6 +134,7 @@ const createTablesSQL = ` merged_at DATETIME, closed_at DATETIME, last_synced_at DATETIME, + detached_at DATETIME, updated_at DATETIME NOT NULL, UNIQUE(task_id, repository_id, pr_number) ); @@ -494,6 +495,7 @@ func (s *Store) applyIdempotentSchemaColumns() { _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN unresolved_review_threads INTEGER DEFAULT 0`) _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN checks_total INTEGER DEFAULT 0`) _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN checks_passing INTEGER DEFAULT 0`) + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN detached_at DATETIME`) // Per-watch cleanup policy for review/issue watches: controls whether the // poller deletes auto-created tasks when the underlying PR/issue reaches // a terminal state. Values: 'auto' (default — preserve only when user @@ -1140,6 +1142,7 @@ func (s *Store) migratePRTablesForMultiRepo() error { merged_at DATETIME, closed_at DATETIME, last_synced_at DATETIME, + detached_at DATETIME, updated_at DATETIME NOT NULL, UNIQUE(task_id, repository_id, pr_number) )`, @@ -1147,12 +1150,12 @@ func (s *Store) migratePRTablesForMultiRepo() error { id, workspace_id, task_id, repository_id, owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, state, review_state, checks_state, mergeable_state, review_count, pending_review_count, comment_count, - additions, deletions, created_at, merged_at, closed_at, last_synced_at, updated_at + additions, deletions, created_at, merged_at, closed_at, last_synced_at, detached_at, updated_at ) SELECT id, COALESCE(workspace_id, ''), task_id, COALESCE(repository_id, ''), owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, state, review_state, checks_state, mergeable_state, review_count, pending_review_count, comment_count, - additions, deletions, created_at, merged_at, closed_at, last_synced_at, updated_at + additions, deletions, created_at, merged_at, closed_at, last_synced_at, detached_at, updated_at FROM github_task_prs`, ) } @@ -1460,12 +1463,12 @@ func (s *Store) CreateTaskPR(ctx context.Context, tp *TaskPR) error { INSERT INTO github_task_prs (id, workspace_id, task_id, repository_id, owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, state, review_state, checks_state, mergeable_state, review_count, pending_review_count, required_reviews, comment_count, unresolved_review_threads, checks_total, checks_passing, additions, deletions, - created_at, merged_at, closed_at, last_synced_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + created_at, merged_at, closed_at, last_synced_at, detached_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, tp.ID, tp.WorkspaceID, tp.TaskID, tp.RepositoryID, tp.Owner, tp.Repo, tp.PRNumber, tp.PRURL, tp.PRTitle, tp.HeadBranch, tp.BaseBranch, tp.AuthorLogin, tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.RequiredReviews, tp.CommentCount, tp.UnresolvedReviewThreads, tp.ChecksTotal, tp.ChecksPassing, tp.Additions, tp.Deletions, - tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt) + tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.DetachedAt, tp.UpdatedAt) return err } @@ -1482,7 +1485,7 @@ const taskPRColumns = `id, workspace_id, task_id, repository_id, owner, repo, pr pr_title, head_branch, base_branch, author_login, state, review_state, checks_state, mergeable_state, review_count, pending_review_count, required_reviews, comment_count, unresolved_review_threads, checks_total, checks_passing, additions, deletions, - created_at, merged_at, closed_at, last_synced_at, updated_at` + created_at, merged_at, closed_at, last_synced_at, detached_at, updated_at` // taskPRColumnsQualified is taskPRColumns with each column qualified by the // `gtp` alias, for queries that join github_task_prs against another table. @@ -1491,13 +1494,26 @@ const taskPRColumnsQualified = `gtp.id, gtp.workspace_id, gtp.task_id, gtp.repos gtp.state, gtp.review_state, gtp.checks_state, gtp.mergeable_state, gtp.review_count, gtp.pending_review_count, gtp.required_reviews, gtp.comment_count, gtp.unresolved_review_threads, gtp.checks_total, gtp.checks_passing, gtp.additions, gtp.deletions, - gtp.created_at, gtp.merged_at, gtp.closed_at, gtp.last_synced_at, gtp.updated_at` + gtp.created_at, gtp.merged_at, gtp.closed_at, gtp.last_synced_at, gtp.detached_at, gtp.updated_at` // GetTaskPR returns the first PR association for a task. For multi-repo tasks // the result is non-deterministic across repos — use ListTaskPRsByTask instead. func (s *Store) GetTaskPR(ctx context.Context, taskID string) (*TaskPR, error) { var tp TaskPR - err := s.ro.GetContext(ctx, &tp, `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? LIMIT 1`, taskID) + err := s.ro.GetContext(ctx, &tp, `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? AND detached_at IS NULL LIMIT 1`, taskID) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + return &tp, err +} + +// GetTaskPRByID returns an association regardless of whether it was detached. +// Mutation paths use this exact lookup so detached rows remain durable +// tombstones and can be explicitly restored by a later link action. +func (s *Store) GetTaskPRByID(ctx context.Context, associationID string) (*TaskPR, error) { + var tp TaskPR + err := s.ro.GetContext(ctx, &tp, + `SELECT `+taskPRColumns+` FROM github_task_prs WHERE id = ? LIMIT 1`, associationID) if errors.Is(err, sql.ErrNoRows) { return nil, nil } @@ -1514,7 +1530,7 @@ func (s *Store) GetTaskPR(ctx context.Context, taskID string) (*TaskPR, error) { func (s *Store) GetTaskPRByRepository(ctx context.Context, taskID, repositoryID string) (*TaskPR, error) { var tp TaskPR err := s.ro.GetContext(ctx, &tp, - `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? AND repository_id = ? + `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? AND repository_id = ? AND detached_at IS NULL ORDER BY updated_at DESC LIMIT 1`, taskID, repositoryID) if errors.Is(err, sql.ErrNoRows) { @@ -1529,6 +1545,21 @@ func (s *Store) GetTaskPRByRepository(ctx context.Context, taskID, repositoryID // the same PR number, not a sibling PR that happens to be the first // row returned by the legacy by-repo query. func (s *Store) GetTaskPRByRepoAndNumber(ctx context.Context, taskID, repositoryID string, prNumber int) (*TaskPR, error) { + var tp TaskPR + err := s.ro.GetContext(ctx, &tp, + `SELECT `+taskPRColumns+` FROM github_task_prs + WHERE task_id = ? AND repository_id = ? AND pr_number = ? AND detached_at IS NULL LIMIT 1`, + taskID, repositoryID, prNumber) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + return &tp, err +} + +// GetTaskPRByRepoAndNumberIncludingDetached returns the exact association +// row, including a detached tombstone, for automatic-versus-explicit link +// decisions. +func (s *Store) GetTaskPRByRepoAndNumberIncludingDetached(ctx context.Context, taskID, repositoryID string, prNumber int) (*TaskPR, error) { var tp TaskPR err := s.ro.GetContext(ctx, &tp, `SELECT `+taskPRColumns+` FROM github_task_prs @@ -1545,7 +1576,7 @@ func (s *Store) GetTaskPRByRepoAndNumber(ctx context.Context, taskID, repository func (s *Store) ListTaskPRsByTask(ctx context.Context, taskID string) ([]*TaskPR, error) { var prs []TaskPR if err := s.ro.SelectContext(ctx, &prs, - `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? ORDER BY created_at ASC`, taskID); err != nil { + `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? AND detached_at IS NULL ORDER BY created_at ASC`, taskID); err != nil { return nil, err } out := make([]*TaskPR, 0, len(prs)) @@ -1555,6 +1586,44 @@ func (s *Store) ListTaskPRsByTask(ctx context.Context, taskID string) ([]*TaskPR return out, nil } +// DetachTaskPR marks an association as removed from active task surfaces while +// retaining the row so automatic PR discovery cannot silently resurrect it. +// The bool return reports whether this call performed the transition. +func (s *Store) DetachTaskPR(ctx context.Context, associationID string) (*TaskPR, bool, error) { + now := time.Now().UTC() + result, err := s.db.ExecContext(ctx, + `UPDATE github_task_prs SET detached_at = ?, updated_at = ? + WHERE id = ? AND detached_at IS NULL`, now, now, associationID) + if err != nil { + return nil, false, err + } + count, err := result.RowsAffected() + if err != nil { + return nil, false, err + } + tp, err := s.GetTaskPRByID(ctx, associationID) + return tp, count > 0, err +} + +// RestoreTaskPR clears a detached tombstone for an explicit link action and +// refreshes the persisted fields available from the fetched GitHub PR. +func (s *Store) RestoreTaskPR(ctx context.Context, taskID, repositoryID string, pr *PR) (*TaskPR, error) { + if pr == nil { + return nil, errors.New("restore task PR: missing PR data") + } + if _, err := s.db.ExecContext(ctx, + `UPDATE github_task_prs SET owner = ?, repo = ?, pr_url = ?, pr_title = ?, + head_branch = ?, base_branch = ?, author_login = ?, state = ?, mergeable_state = ?, + additions = ?, deletions = ?, merged_at = ?, closed_at = ?, detached_at = NULL, updated_at = ? + WHERE task_id = ? AND repository_id = ? AND pr_number = ?`, + pr.RepoOwner, pr.RepoName, pr.HTMLURL, pr.Title, pr.HeadBranch, pr.BaseBranch, pr.AuthorLogin, + pr.State, pr.MergeableState, pr.Additions, pr.Deletions, pr.MergedAt, pr.ClosedAt, time.Now().UTC(), + taskID, repositoryID, pr.Number); err != nil { + return nil, err + } + return s.GetTaskPRByRepoAndNumber(ctx, taskID, repositoryID, pr.Number) +} + // ListTaskPRsByTaskIDs returns PR associations for multiple tasks. Each task // may have multiple PRs (one per repository for multi-repo tasks); rows are // returned grouped by task_id, ordered by created_at ascending within a group. @@ -1563,7 +1632,7 @@ func (s *Store) ListTaskPRsByTaskIDs(ctx context.Context, taskIDs []string) (map return make(map[string][]*TaskPR), nil } query, args, err := sqlx.In( - `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id IN (?) ORDER BY created_at ASC`, + `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id IN (?) AND detached_at IS NULL ORDER BY created_at ASC`, taskIDs, ) if err != nil { @@ -1585,7 +1654,7 @@ func (s *Store) ListTaskPRsByWorkspaceID(ctx context.Context, workspaceID string if err := s.ro.SelectContext(ctx, &prs, `SELECT `+taskPRColumnsQualified+` FROM github_task_prs gtp INNER JOIN tasks t ON gtp.task_id = t.id - WHERE t.workspace_id = ? + WHERE t.workspace_id = ? AND gtp.detached_at IS NULL ORDER BY gtp.created_at ASC`, workspaceID); err != nil { return nil, err } @@ -1615,7 +1684,7 @@ func (s *Store) ListTaskIDsByPRNumber(ctx context.Context, workspaceID string, p if err := s.ro.SelectContext(ctx, &ids, `SELECT DISTINCT gtp.task_id FROM github_task_prs gtp INNER JOIN tasks t ON gtp.task_id = t.id - WHERE t.workspace_id = ? AND gtp.pr_number = ?`, workspaceID, prNumber); err != nil { + WHERE t.workspace_id = ? AND gtp.pr_number = ? AND gtp.detached_at IS NULL`, workspaceID, prNumber); err != nil { return nil, err } return ids, nil @@ -1673,12 +1742,12 @@ func (s *Store) ReplaceTaskPR(ctx context.Context, tp *TaskPR) error { INSERT INTO github_task_prs (id, workspace_id, task_id, repository_id, owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, state, review_state, checks_state, mergeable_state, review_count, pending_review_count, required_reviews, comment_count, unresolved_review_threads, checks_total, checks_passing, additions, deletions, - created_at, merged_at, closed_at, last_synced_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + created_at, merged_at, closed_at, last_synced_at, detached_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, tp.ID, tp.WorkspaceID, tp.TaskID, tp.RepositoryID, tp.Owner, tp.Repo, tp.PRNumber, tp.PRURL, tp.PRTitle, tp.HeadBranch, tp.BaseBranch, tp.AuthorLogin, tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.RequiredReviews, tp.CommentCount, tp.UnresolvedReviewThreads, tp.ChecksTotal, tp.ChecksPassing, tp.Additions, tp.Deletions, - tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt); err != nil { + tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.DetachedAt, tp.UpdatedAt); err != nil { return err } return tx.Commit() diff --git a/apps/backend/internal/github/store_task_pr_detach_test.go b/apps/backend/internal/github/store_task_pr_detach_test.go new file mode 100644 index 0000000000..e2c2345870 --- /dev/null +++ b/apps/backend/internal/github/store_task_pr_detach_test.go @@ -0,0 +1,151 @@ +package github + +import ( + "context" + "reflect" + "testing" + "time" +) + +func TestTaskPRDetachFiltersActiveRowsAndPersistsTombstone(t *testing.T) { + store := newTestStore(t) + detacher, ok := any(store).(interface { + DetachTaskPR(context.Context, string) (*TaskPR, bool, error) + }) + if !ok { + t.Fatal("Store does not implement DetachTaskPR") + } + ctx := context.Background() + now := time.Now().UTC() + first := &TaskPR{ + WorkspaceID: "ws-1", TaskID: "task-1", RepositoryID: "repo-1", + Owner: "acme", Repo: "demo", PRNumber: 1, PRURL: "https://github.com/acme/demo/pull/1", + PRTitle: "old", HeadBranch: "old", BaseBranch: "main", State: "merged", CreatedAt: now, + } + second := &TaskPR{ + WorkspaceID: "ws-1", TaskID: "task-1", RepositoryID: "repo-1", + Owner: "acme", Repo: "demo", PRNumber: 2, PRURL: "https://github.com/acme/demo/pull/2", + PRTitle: "new", HeadBranch: "new", BaseBranch: "main", State: "open", CreatedAt: now.Add(time.Second), + } + if err := store.CreateTaskPR(ctx, first); err != nil { + t.Fatalf("create first PR: %v", err) + } + if err := store.CreateTaskPR(ctx, second); err != nil { + t.Fatalf("create second PR: %v", err) + } + + detached, transitioned, err := detacher.DetachTaskPR(ctx, first.ID) + if err != nil { + t.Fatalf("detach first PR: %v", err) + } + if !transitioned { + t.Fatal("first detach did not report a transition") + } + detachedAt := reflect.Value{} + if detached != nil { + detachedAt = reflect.ValueOf(detached).Elem().FieldByName("DetachedAt") + } + if detached == nil || detached.ID != first.ID || !detachedAt.IsValid() || detachedAt.IsNil() { + t.Fatalf("detached row = %+v, want stamped row", detached) + } + + active, err := store.ListTaskPRsByTask(ctx, "task-1") + if err != nil { + t.Fatalf("list active PRs: %v", err) + } + if len(active) != 1 || active[0].ID != second.ID { + t.Fatalf("active PRs = %+v, want only second PR", active) + } + _, transitioned, err = detacher.DetachTaskPR(ctx, first.ID) + if err != nil { + t.Fatalf("repeat detach first PR: %v", err) + } + if transitioned { + t.Fatal("repeat detach reported a transition") + } + + reopened, err := NewStore(store.db, store.ro) + if err != nil { + t.Fatalf("reopen store: %v", err) + } + active, err = reopened.ListTaskPRsByTask(ctx, "task-1") + if err != nil { + t.Fatalf("list active PRs after reopen: %v", err) + } + if len(active) != 1 || active[0].ID != second.ID { + t.Fatalf("active PRs after reopen = %+v, want only second PR", active) + } +} + +func TestLegacyTaskPRRebuildPreservesDetachedAt(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + now := time.Now().UTC() + if _, err := store.db.ExecContext(ctx, ` + INSERT INTO workspaces (id) VALUES ('ws-legacy'); + INSERT INTO tasks (id, workspace_id) VALUES ('task-legacy', 'ws-legacy'); + DROP TABLE github_task_prs; + CREATE TABLE github_task_prs ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL DEFAULT '', + task_id TEXT NOT NULL, + owner TEXT NOT NULL, + repo TEXT NOT NULL, + pr_number INTEGER NOT NULL, + pr_url TEXT NOT NULL, + pr_title TEXT NOT NULL, + head_branch TEXT NOT NULL, + base_branch TEXT NOT NULL, + author_login TEXT NOT NULL, + state TEXT NOT NULL DEFAULT 'open', + review_state TEXT NOT NULL DEFAULT '', + checks_state TEXT NOT NULL DEFAULT '', + mergeable_state TEXT NOT NULL DEFAULT '', + review_count INTEGER DEFAULT 0, + pending_review_count INTEGER DEFAULT 0, + required_reviews INTEGER, + comment_count INTEGER DEFAULT 0, + unresolved_review_threads INTEGER DEFAULT 0, + checks_total INTEGER DEFAULT 0, + checks_passing INTEGER DEFAULT 0, + additions INTEGER DEFAULT 0, + deletions INTEGER DEFAULT 0, + created_at DATETIME NOT NULL, + merged_at DATETIME, + closed_at DATETIME, + last_synced_at DATETIME, + detached_at DATETIME, + updated_at DATETIME NOT NULL, + UNIQUE(task_id, pr_number) + ); + INSERT INTO github_task_prs ( + id, workspace_id, task_id, owner, repo, pr_number, pr_url, pr_title, + head_branch, base_branch, author_login, state, created_at, detached_at, updated_at + ) VALUES ( + 'legacy-detached', 'ws-legacy', 'task-legacy', 'acme', 'demo', 42, + 'https://github.com/acme/demo/pull/42', 'Legacy detached PR', 'feat/legacy', + 'main', 'octocat', 'merged', ?, ?, ? + ) + `, now, now, now); err != nil { + t.Fatalf("seed legacy task PR schema: %v", err) + } + + reopened, err := NewStore(store.db, store.ro) + if err != nil { + t.Fatalf("reopen store with legacy task PR schema: %v", err) + } + got, err := reopened.GetTaskPRByID(ctx, "legacy-detached") + if err != nil { + t.Fatalf("get migrated detached PR: %v", err) + } + if got == nil || got.DetachedAt == nil || !got.DetachedAt.Equal(now) { + t.Fatalf("migrated detached PR = %+v, want detached_at %s", got, now) + } + active, err := reopened.ListTaskPRsByTask(ctx, "task-legacy") + if err != nil { + t.Fatalf("list migrated active PRs: %v", err) + } + if len(active) != 0 { + t.Fatalf("migrated active PRs = %+v, want detached row filtered", active) + } +} diff --git a/apps/backend/pkg/websocket/actions.go b/apps/backend/pkg/websocket/actions.go index 2fa55d0f4f..de22928d66 100644 --- a/apps/backend/pkg/websocket/actions.go +++ b/apps/backend/pkg/websocket/actions.go @@ -448,6 +448,7 @@ const ( ActionGitHubPRFilesGet = "github.pr_files.get" ActionGitHubPRCommitsGet = "github.pr_commits.get" ActionGitHubTaskPRUpdated = "github.task_pr.updated" // Notification + ActionGitHubTaskPRDeleted = "github.task_pr.deleted" // Notification ActionGitHubTaskCIOptionsUpdated = "github.task_ci_options.updated" // Notification ActionGitHubRateLimitUpdated = "github.rate_limit.updated" // Notification ActionGitHubPRFeedbackNotify = "github.pr_feedback.notify" // Notification diff --git a/apps/web/components/github/multi-pr-ci-popover.tsx b/apps/web/components/github/multi-pr-ci-popover.tsx index e92575794b..c9daeada54 100644 --- a/apps/web/components/github/multi-pr-ci-popover.tsx +++ b/apps/web/components/github/multi-pr-ci-popover.tsx @@ -1,13 +1,14 @@ "use client"; import { useId, useState, type KeyboardEvent } from "react"; -import { IconGitPullRequest } from "@tabler/icons-react"; +import { IconGitPullRequest, IconLoader2, IconX } from "@tabler/icons-react"; import { cn } from "@/lib/utils"; import { PRCIPopover } from "@/components/github/pr-ci-popover"; import { getPRStatusColor, pickDefaultPR } from "@/components/github/pr-task-icon"; import { prIdentitySlug } from "@/components/github/pr-utils"; import { usePRFeedbackBackgroundSync } from "@/hooks/domains/github/use-pr-ci-popover"; import { useAppStore } from "@/components/state-provider"; +import { useToast } from "@/components/toast-provider"; import type { TaskPR } from "@/lib/types/github"; /** @@ -39,38 +40,66 @@ function PRTab({ panelId, active, onSelect, + canRemove, + removing, + onRemove, }: { pr: TaskPR; uid: string; panelId: string; active: boolean; onSelect: (pr: TaskPR) => void; + canRemove: boolean; + removing: boolean; + onRemove?: (pr: TaskPR) => void; }) { return ( - + {canRemove && ( + )} - > - - - {pr.repo} #{pr.pr_number} - - + ); } @@ -85,11 +114,15 @@ export function MultiPRCIPopover({ enabled, onOpenDetailPanel, refreshTaskPR, + onRemovePR, + onCollapseFocus, }: { prs: TaskPR[]; enabled: boolean; onOpenDetailPanel?: (pr: TaskPR) => void; refreshTaskPR?: () => void; + onRemovePR?: (pr: TaskPR) => Promise; + onCollapseFocus?: (remainingPR: TaskPR) => void; }) { // `overrideId` is only set when the user activates a tab. The displayed PR is // derived: honour the override while it still exists, otherwise fall back to @@ -101,9 +134,41 @@ export function MultiPRCIPopover({ // always references an element that exists in the DOM. const uid = useId(); const panelId = `${uid}-pr-ci-tabpanel`; + const [removingId, setRemovingId] = useState(null); + const { toast } = useToast(); const selected = prs.find((p) => p.id === overrideId) ?? pickDefaultPR(prs); if (!selected) return null; + const removePR = async (pr: TaskPR) => { + if (!onRemovePR || removingId) return; + setRemovingId(pr.id); + try { + await onRemovePR(pr); + if (selected.id === pr.id) { + const removedIndex = prs.findIndex((candidate) => candidate.id === pr.id); + const next = prs[removedIndex + 1] ?? prs[removedIndex - 1]; + if (next && prs.length > 2) { + setOverrideId(next.id); + // Keep keyboard focus on the deterministic adjacent tab while the + // multi-PR surface remains mounted. + document.getElementById(prTabDomId(uid, next))?.focus(); + } else if (next) { + // The parent collapses to its single-PR surface after a two-to-one + // removal, so let it restore focus to the surviving trigger. + onCollapseFocus?.(next); + } + } + } catch (error) { + toast({ + title: "Failed to unlink pull request", + description: error instanceof Error ? error.message : "The pull request is still linked.", + variant: "error", + }); + } finally { + setRemovingId(null); + } + }; + // ArrowLeft/ArrowRight move selection (and focus) within the tablist, // wrapping at the ends — selection follows focus, per the ARIA tabs pattern. const onTablistKeyDown = (e: KeyboardEvent) => { @@ -138,6 +203,9 @@ export function MultiPRCIPopover({ panelId={panelId} active={pr.id === selected.id} onSelect={(p) => setOverrideId(p.id)} + canRemove={prs.length > 1 && Boolean(onRemovePR)} + removing={removingId === pr.id} + onRemove={(p) => void removePR(p)} /> ))} diff --git a/apps/web/components/github/pr-ci-popover.automation.test.tsx b/apps/web/components/github/pr-ci-popover.automation.test.tsx index 52c9620a3e..7260bd60e8 100644 --- a/apps/web/components/github/pr-ci-popover.automation.test.tsx +++ b/apps/web/components/github/pr-ci-popover.automation.test.tsx @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useState } from "react"; import { cleanup, render, screen, fireEvent, waitFor } from "@testing-library/react"; import { TooltipProvider } from "@kandev/ui/tooltip"; import { StateProvider } from "@/components/state-provider"; @@ -54,6 +55,8 @@ const MERGED_PROMPT_LABEL = "PR merged"; const CLOSED_PROMPT_LABEL = "PR closed without merging"; const REVIEW_REQUEST_PROMPT_LABEL = "Your review is requested"; const REVIEW_FOLLOW_UP_TRIGGER = "ci-review-follow-up-trigger"; +const REMOVE_FIRST_PR_LABEL = "Remove r #1 from task"; +const BACKEND_UNAVAILABLE = "backend unavailable"; function makeOptions(overrides: Partial = {}): TaskCIAutomationOptions { return { @@ -290,6 +293,116 @@ describe("PRCIPopover automation status", () => { }); }); +describe("MultiPRCIPopover unlink", () => { + beforeEach(() => { + resetHookMocks(); + }); + + afterEach(() => { + cleanup(); + }); + + it("unlinks one multi-PR tab while keeping its sibling visible", async () => { + const onCollapseFocus = vi.fn(); + function RemovablePopover() { + const [prs, setPrs] = useState([ + makePR({ id: "a", pr_number: 1, pr_title: "First PR" }), + makePR({ id: "b", pr_number: 2, pr_title: "Second PR", checks_state: "success" }), + ]); + return ( + + setPrs((current) => current.filter((item) => item.id !== pr.id)) + } + onCollapseFocus={onCollapseFocus} + /> + ); + } + + render( + + + + + + + , + ); + + fireEvent.click(screen.getByRole("button", { name: REMOVE_FIRST_PR_LABEL })); + + await waitFor(() => + expect(screen.queryByRole("button", { name: REMOVE_FIRST_PR_LABEL })).toBeNull(), + ); + expect(screen.getByRole("tab", { name: "r #2" })).not.toBeNull(); + expect(onCollapseFocus).toHaveBeenCalledWith(expect.objectContaining({ id: "b" })); + }); + + it("keeps a failed unlink tab and reports the error", async () => { + const onRemovePR = vi.fn().mockRejectedValue(new Error(BACKEND_UNAVAILABLE)); + render( + + + + + + + , + ); + + fireEvent.click(screen.getByRole("button", { name: REMOVE_FIRST_PR_LABEL })); + + await waitFor(() => + expect(screen.getByTestId("toast-message").textContent).toContain(BACKEND_UNAVAILABLE), + ); + expect(screen.getByRole("button", { name: REMOVE_FIRST_PR_LABEL })).not.toBeNull(); + expect(onRemovePR).toHaveBeenCalledWith(expect.objectContaining({ id: "a" })); + }); + + it("focuses the next adjacent tab when a selected tab is removed from a larger set", async () => { + function RemovablePopover() { + const [prs, setPrs] = useState([ + makePR({ id: "a", pr_number: 1, pr_title: "First PR", checks_state: "success" }), + makePR({ id: "b", pr_number: 2, pr_title: "Second PR" }), + makePR({ id: "c", pr_number: 3, pr_title: "Third PR", checks_state: "success" }), + ]); + return ( + + setPrs((current) => current.filter((item) => item.id !== pr.id)) + } + /> + ); + } + + render( + + + + + + + , + ); + + expect(screen.getByRole("tab", { name: "r #2" }).getAttribute("aria-selected")).toBe("true"); + fireEvent.click(screen.getByRole("button", { name: "Remove r #2 from task" })); + + await waitFor(() => expect(screen.queryByRole("tab", { name: "r #2" })).toBeNull()); + const nextTab = screen.getByRole("tab", { name: "r #3" }); + expect(nextTab.getAttribute("aria-selected")).toBe("true"); + expect(document.activeElement).toBe(nextTab); + }); +}); + describe("PRCIPopover task prompts", () => { beforeEach(() => { resetHookMocks(); @@ -338,10 +451,10 @@ describe("PRCIPopover task prompts", () => { }); it("offers retry after CI automation options fail to load", () => { - hookMocks.error = "backend unavailable"; + hookMocks.error = BACKEND_UNAVAILABLE; renderPopover(); - expect(screen.getByText("backend unavailable")).not.toBeNull(); + expect(screen.getByText(BACKEND_UNAVAILABLE)).not.toBeNull(); fireEvent.click(screen.getByRole("button", { name: "Retry" })); expect(hookMocks.refreshMock).toHaveBeenCalledTimes(1); diff --git a/apps/web/components/github/pr-status-chip.test.tsx b/apps/web/components/github/pr-status-chip.test.tsx index 43ca54612a..c1363b510b 100644 --- a/apps/web/components/github/pr-status-chip.test.tsx +++ b/apps/web/components/github/pr-status-chip.test.tsx @@ -129,6 +129,7 @@ afterEach(() => { const CHIP_TESTID = "pr-status-chip"; const ATTR_PR_NUMBER = "data-pr-number"; +const ATTR_PR_COUNT = "data-pr-count"; const ATTR_STATUS = "data-status"; const ATTR_READY_TO_MERGE = "data-pr-ready-to-merge"; const DRAWER_SELECTOR = "[data-testid='pr-status-chip-drawer']"; @@ -636,8 +637,7 @@ describe("PRStatusChip — multiple PRs", () => { it("renders one aggregate chip with a PR count and worst-of status", () => { renderWithStore(multiState(TWO_OPEN), ); const chip = screen.getByTestId(CHIP_TESTID); - expect(chip.getAttribute("data-pr-count")).toBe("2"); - // PR #2 is failing, so the aggregate glyph is red. + expect(chip.getAttribute(ATTR_PR_COUNT)).toBe("2"); expect(chip.getAttribute(ATTR_STATUS)).toBe("failed"); }); @@ -646,21 +646,14 @@ describe("PRStatusChip — multiple PRs", () => { await expectDesktopHoverPopoverConstrained(); }); - it("stays visible while at least one PR is still open", () => { + it("keeps terminal siblings in the multi-PR unlink surface", () => { renderWithStore( multiState([makePR({ id: "a", state: "merged" }), makePR({ id: "b", pr_number: 2 })]), , ); - // Only one PR is open, so it renders as the single-PR chip (its number). - expect(screen.getByTestId(CHIP_TESTID).getAttribute(ATTR_PR_NUMBER)).toBe("2"); - }); - - it("returns null only when every PR is terminal", () => { - renderWithStore( - multiState([makePR({ id: "a", state: "merged" }), makePR({ id: "b", state: "closed" })]), - , - ); - expect(screen.queryByTestId(CHIP_TESTID)).toBeNull(); + const chip = screen.getByTestId(CHIP_TESTID); + expect(chip.getAttribute(ATTR_PR_COUNT)).toBe("2"); + expect(chip.getAttribute(ATTR_STATUS)).toBe("passed"); }); describe("mobile drawer", () => { @@ -676,8 +669,6 @@ describe("PRStatusChip — multiple PRs", () => { }); expect(document.querySelector(DRAWER_SELECTOR)).not.toBeNull(); expect(document.querySelector("[data-testid='pr-multi-popover']")).not.toBeNull(); - // One tab per PR (testid is owner+repo-scoped so same-number PRs on - // different repos, or repos sharing a name across owners, stay unique). expect(document.querySelector("[data-testid='pr-popover-tab-acme-demo-1']")).not.toBeNull(); expect(document.querySelector("[data-testid='pr-popover-tab-acme-demo-2']")).not.toBeNull(); }); diff --git a/apps/web/components/github/pr-status-chip.tsx b/apps/web/components/github/pr-status-chip.tsx index 2781394cfa..34d2a67e45 100644 --- a/apps/web/components/github/pr-status-chip.tsx +++ b/apps/web/components/github/pr-status-chip.tsx @@ -46,8 +46,8 @@ import type { TaskCIAutomationOptions, TaskPR } from "@/lib/types/github"; const HOVER_OPEN_DELAY_MS = 150; const HOVER_CLOSE_DELAY_MS = 150; -// Terminal states (merged / closed) never reach here — PRStatusChip returns -// null for them before rendering — so the chip status union omits them. +// Terminal states (merged / closed) are omitted from CI status aggregation, +// but remain in multi-PR surfaces so users can unlink old associations. type ChipStatus = | "passed" | "failed" @@ -63,17 +63,27 @@ type AutomationFlags = { autoMerge: boolean; autoFixRound: AutoFixRoundInfo | null; }; +type TriggerRef = { current: HTMLButtonElement | null }; type SingleChipProps = { pr: TaskPR; automation: AutomationFlags; refreshTaskPR: () => void; + triggerRef?: TriggerRef; }; type MultiChipProps = { prs: TaskPR[]; + statusPrs?: TaskPR[]; automation: AutomationFlags; refreshTaskPR: () => void; + onRemovePR?: (pr: TaskPR) => Promise; + triggerRef?: TriggerRef; }; +function focusAfterCollapse(triggerRef?: TriggerRef) { + if (!triggerRef) return; + setTimeout(() => triggerRef.current?.focus(), 0); +} + function chipStatus(pr: TaskPR): ChipStatus { if (pr.review_state === "changes_requested" || pr.checks_state === "failure") return "failed"; // Merge conflicts / behind-base block the merge even when CI is green — the @@ -132,8 +142,9 @@ const CHIP_BUTTON_CLASS = * via hover. Returns the trigger ref plus a memoised handler that reads the ref * lazily (inside the callback, never during render). */ -function useChipTriggerGuard() { - const ref = useRef(null); +function useChipTriggerGuard(externalRef?: TriggerRef) { + const fallbackRef = useRef(null); + const ref = externalRef ?? fallbackRef; const onPointerDownOutside = useCallback( (e: { target: EventTarget | null; preventDefault: () => void }) => { if (ref.current && ref.current.contains(e.target as Node)) { @@ -171,42 +182,46 @@ function useChipPopoverInteractions() { * Mobile: tapping opens the same popover content inside a bottom-sheet Drawer * — hover is unreachable on touch devices. * - * Returns null when the task has no PR yet, or once the PR reaches a terminal - * state (merged / closed) — the chat-input banner already conveys that, so the - * CI chip would be redundant. + * Returns null when the task has no PR yet, or when its only PR is terminal + * (merged / closed). With multiple associations, terminal PRs stay in the + * multi-PR surface so old links can still be removed while CI status continues + * to reflect open PRs only. */ export function PRStatusChip({ taskId }: { taskId: string | null }) { const workspaceId = useAppStore((state) => state.workspaces.activeId); - const { prs, refresh } = useTaskPR(taskId); + const { prs, refresh, unlink } = useTaskPR(taskId); const { options: automationOptions } = useTaskCIAutomationOptions(taskId); + const triggerRef = useRef(null); // Defensive Array.isArray: a partial hydration can briefly seed the store // with a non-array value (same guard as PRTaskIcon). - // Only open PRs are worth a CI chip — terminal PRs (merged/closed) are - // already conveyed by the chat-input banner. With multiple PRs the chip - // stays visible as long as at least one is still open. - const openPRs = Array.isArray(prs) - ? prs.filter((p) => p.state !== "merged" && p.state !== "closed") - : []; + const allPRs = Array.isArray(prs) ? prs : []; + // Terminal PRs are excluded from CI status and background warming, but are + // kept in the multi-PR association list so old links remain unlinkable. + const openPRs = allPRs.filter((p) => p.state !== "merged" && p.state !== "closed"); // Subscribe at the chip level so the cache warms even when the top-bar PR // button isn't mounted (e.g. small viewport that hides it). Warm the PR the // popover will actually open first (worst-status via pickDefaultPR — for a // single PR that's just the PR itself); the remaining PRs in a multi-PR // task warm when the popover opens. usePRFeedbackBackgroundSync(workspaceId, pickDefaultPR(openPRs)); - if (openPRs.length === 0) return null; - if (openPRs.length === 1) + if (allPRs.length === 0 || (allPRs.length === 1 && openPRs.length === 0)) return null; + if (allPRs.length === 1) return ( ); return ( unlink(pr.id)} + triggerRef={triggerRef} /> ); } @@ -328,9 +343,9 @@ function PRStatusChipInner(props: SingleChipProps) { return ; } -function PRStatusChipHoverCard({ pr, automation, refreshTaskPR }: SingleChipProps) { +function PRStatusChipHoverCard({ pr, automation, refreshTaskPR, triggerRef }: SingleChipProps) { const status = chipStatus(pr); - const { ref, onPointerDownOutside } = useChipTriggerGuard(); + const { ref, onPointerDownOutside } = useChipTriggerGuard(triggerRef); const { open, onOpenChange, onTriggerEnter, onTriggerLeave, onContentEnter, onContentLeave } = useChipPopoverInteractions(); return ( @@ -420,9 +435,16 @@ function MultiChipGlyph({ ); } -function PRStatusChipMultiHoverCard({ prs, automation, refreshTaskPR }: MultiChipProps) { - const status = aggregateChipStatus(prs); - const { ref, onPointerDownOutside } = useChipTriggerGuard(); +function PRStatusChipMultiHoverCard({ + prs, + statusPrs, + automation, + refreshTaskPR, + onRemovePR, + triggerRef, +}: MultiChipProps) { + const status = aggregateChipStatus(statusPrs ?? prs); + const { ref, onPointerDownOutside } = useChipTriggerGuard(triggerRef); const { open, onOpenChange, onTriggerEnter, onTriggerLeave, onContentEnter, onContentLeave } = useChipPopoverInteractions(); return ( @@ -457,18 +479,32 @@ function PRStatusChipMultiHoverCard({ prs, automation, refreshTaskPR }: MultiChi onPointerDownOutside={onPointerDownOutside} onOpenAutoFocus={(e) => e.preventDefault()} > - + focusAfterCollapse(triggerRef)} + /> ); } -function PRStatusChipMultiDrawer({ prs, automation, refreshTaskPR }: MultiChipProps) { - const status = aggregateChipStatus(prs); +function PRStatusChipMultiDrawer({ + prs, + statusPrs, + automation, + refreshTaskPR, + onRemovePR, + triggerRef, +}: MultiChipProps) { + const status = aggregateChipStatus(statusPrs ?? prs); const [open, setOpen] = useState(false); return (