diff --git a/apps/backend/internal/backendapp/turn_adapters.go b/apps/backend/internal/backendapp/turn_adapters.go index 9c5a9daa9b..eb554ec4a3 100644 --- a/apps/backend/internal/backendapp/turn_adapters.go +++ b/apps/backend/internal/backendapp/turn_adapters.go @@ -98,7 +98,18 @@ type taskDeleterAdapter struct { } func (a *taskDeleterAdapter) DeleteTask(ctx context.Context, taskID string) error { - err := a.svc.DeleteTask(ctx, taskID) + return a.translateDeleteErr(a.svc.DeleteTask(ctx, taskID)) +} + +// DeleteTaskWithReason satisfies github.TaskDeleterWithReason so the review/issue +// cleanup paths can attach a deletion reason to the task.deleted event. +func (a *taskDeleterAdapter) DeleteTaskWithReason(ctx context.Context, taskID, reason string) error { + return a.translateDeleteErr(a.svc.DeleteTaskWithReason(ctx, taskID, reason)) +} + +// translateDeleteErr maps the task repository's ErrTaskNotFound sentinel to +// github.ErrTaskNotFound so cleanup can classify the "already gone" case. +func (a *taskDeleterAdapter) translateDeleteErr(err error) error { if err == nil { return nil } diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index b71d43c463..adf296fea0 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -50,6 +50,14 @@ type TaskDeleter interface { DeleteTask(ctx context.Context, taskID string) error } +// TaskDeleterWithReason is an optional extension of TaskDeleter that lets the +// cleanup path attach a machine-readable deletion reason (e.g. +// "pr_approved_by_user") to the published task.deleted event. When the wired +// deleter does not implement this, cleanup falls back to plain DeleteTask. +type TaskDeleterWithReason interface { + DeleteTaskWithReason(ctx context.Context, taskID, reason string) error +} + // isTaskNotFound reports whether an error from TaskDeleter signals the task // was already gone. Adapters wrap their underlying not-found error with // ErrTaskNotFound — see cmd/kandev/turn_adapters.go's taskDeleterAdapter. diff --git a/apps/backend/internal/github/service_cleanup.go b/apps/backend/internal/github/service_cleanup.go index 1ad1bc0cd4..4556997e57 100644 --- a/apps/backend/internal/github/service_cleanup.go +++ b/apps/backend/internal/github/service_cleanup.go @@ -123,6 +123,16 @@ func (s *Service) buildReviewWatchCaches(ctx context.Context, prTasks []*ReviewP return policy, enabled, unknown } +// deleteTaskWithReason deletes a task, threading the cleanup reason through to +// the task.deleted event when the wired deleter supports it (see +// TaskDeleterWithReason). Falls back to plain DeleteTask otherwise. +func (s *Service) deleteTaskWithReason(ctx context.Context, taskID, reason string) error { + if d, ok := s.taskDeleter.(TaskDeleterWithReason); ok { + return d.DeleteTaskWithReason(ctx, taskID, reason) + } + return s.taskDeleter.DeleteTask(ctx, taskID) +} + // cleanupReviewPRTaskBatch runs the deletion gate over a slice of dedup rows. // resolvePolicy returns the effective cleanup policy for each row; callers // supply it so per-watch and global-sweep paths can share this body. @@ -154,7 +164,7 @@ func (s *Service) cleanupReviewPRTaskBatch(ctx context.Context, prTasks []*Revie if !shouldDelete { continue } - if err := s.taskDeleter.DeleteTask(ctx, rpt.TaskID); err != nil { + if err := s.deleteTaskWithReason(ctx, rpt.TaskID, reason); err != nil { if isTaskNotFound(err) { // Task already deleted; clean up the orphaned dedup record. if err := s.store.DeleteReviewPRTask(ctx, rpt.ID); err != nil { @@ -393,7 +403,7 @@ func (s *Service) cleanupIssueTaskBatch(ctx context.Context, issueTasks []*Issue if !shouldDelete { continue } - if err := s.taskDeleter.DeleteTask(ctx, it.TaskID); err != nil { + if err := s.deleteTaskWithReason(ctx, it.TaskID, reason); err != nil { if isTaskNotFound(err) { if err := s.store.DeleteIssueWatchTask(ctx, it.ID); err != nil { s.logger.Warn("failed to delete orphan dedup row after task-not-found", diff --git a/apps/backend/internal/github/service_review_reason_test.go b/apps/backend/internal/github/service_review_reason_test.go new file mode 100644 index 0000000000..605c33d9af --- /dev/null +++ b/apps/backend/internal/github/service_review_reason_test.go @@ -0,0 +1,74 @@ +package github + +import ( + "context" + "testing" +) + +// recordingReasonDeleter implements both TaskDeleter and TaskDeleterWithReason +// so the cleanup path exercises the reason-threading branch (deleteTaskWithReason +// prefers DeleteTaskWithReason when the wired deleter supports it). +type recordingReasonDeleter struct { + taskID string + reason string +} + +func (r *recordingReasonDeleter) DeleteTask(_ context.Context, taskID string) error { + r.taskID = taskID + return nil +} + +func (r *recordingReasonDeleter) DeleteTaskWithReason(_ context.Context, taskID, reason string) error { + r.taskID = taskID + r.reason = reason + return nil +} + +// TestCleanupMergedReviewTasks_ThreadsApprovedReason verifies that when a PR is +// approved by the authenticated user, the cleanup path deletes the task with the +// "pr_approved_by_user" reason so the frontend can explain the disappearance. +func TestCleanupMergedReviewTasks_ThreadsApprovedReason(t *testing.T) { + _, svc, mockClient, store := setupPollerTest(t) + ctx := context.Background() + + watch := &ReviewWatch{WorkspaceID: "ws-1", Enabled: true} + if err := store.CreateReviewWatch(ctx, watch); err != nil { + t.Fatalf("CreateReviewWatch: %v", err) + } + + rpt := &ReviewPRTask{ + ReviewWatchID: watch.ID, + RepoOwner: "acme", + RepoName: "widget", + PRNumber: 42, + PRURL: "https://github.com/acme/widget/pull/42", + TaskID: "task-approved", + } + if err := store.CreateReviewPRTask(ctx, rpt); err != nil { + t.Fatalf("CreateReviewPRTask: %v", err) + } + + // PR is still open, but the authenticated user already approved it on + // GitHub, so shouldDeleteReviewTask returns reason pr_approved_by_user. + mockClient.AddPR(&PR{Number: 42, State: "open", RepoOwner: "acme", RepoName: "widget"}) + mockClient.AddReviews("acme", "widget", 42, []PRReview{ + {State: "APPROVED", Author: mockDefaultUser}, + }) + + rec := &recordingReasonDeleter{} + svc.SetTaskDeleter(rec) + + deleted, err := svc.CleanupMergedReviewTasks(ctx, watch) + if err != nil { + t.Fatalf("CleanupMergedReviewTasks: %v", err) + } + if deleted != 1 { + t.Fatalf("expected 1 deleted, got %d", deleted) + } + if rec.taskID != "task-approved" { + t.Errorf("deleted taskID=%q, want task-approved", rec.taskID) + } + if rec.reason != "pr_approved_by_user" { + t.Errorf("reason=%q, want pr_approved_by_user", rec.reason) + } +} diff --git a/apps/backend/internal/gitlab/service.go b/apps/backend/internal/gitlab/service.go index 07c686f096..5eff032fb5 100644 --- a/apps/backend/internal/gitlab/service.go +++ b/apps/backend/internal/gitlab/service.go @@ -43,6 +43,14 @@ type TaskDeleter interface { DeleteTask(ctx context.Context, taskID string) error } +// TaskDeleterWithReason is an optional extension of TaskDeleter that lets the +// cleanup path attach a machine-readable deletion reason (e.g. +// "pr_merged_or_closed") to the published task.deleted event. When the wired +// deleter does not implement this, cleanup falls back to plain DeleteTask. +type TaskDeleterWithReason interface { + DeleteTaskWithReason(ctx context.Context, taskID, reason string) error +} + // TaskSessionChecker checks whether the user genuinely engaged with a task. type TaskSessionChecker interface { HasUserAuthoredMessage(ctx context.Context, taskID string) (bool, error) diff --git a/apps/backend/internal/gitlab/service_cleanup.go b/apps/backend/internal/gitlab/service_cleanup.go index 233864a3d6..9275b640ea 100644 --- a/apps/backend/internal/gitlab/service_cleanup.go +++ b/apps/backend/internal/gitlab/service_cleanup.go @@ -7,6 +7,14 @@ import ( "go.uber.org/zap" ) +// Machine-readable deletion reasons attached to the task.deleted event so the +// frontend can explain why a focused auto-deleted task vanished. These mirror +// the GitHub cleanup reasons and the frontend TaskDeletionReason union. +const ( + reasonMRMergedOrClosed = "pr_merged_or_closed" + reasonIssueClosed = "issue_closed" +) + // CleanupAllReviewTasks deletes auto-created review tasks for merged/closed MRs, // respecting each watch's cleanup policy. Returns the count of tasks deleted. func (s *Service) CleanupAllReviewTasks(ctx context.Context) (int, error) { @@ -28,6 +36,16 @@ func (s *Service) CleanupAllReviewTasks(ctx context.Context) (int, error) { return s.sweepReviewMRTasks(ctx, tasks, deleter, checker), nil } +// deleteTaskWithReason deletes a task, threading the cleanup reason through to +// the task.deleted event when the wired deleter supports it (see +// TaskDeleterWithReason). Falls back to plain DeleteTask otherwise. +func deleteTaskWithReason(ctx context.Context, deleter TaskDeleter, taskID, reason string) error { + if d, ok := deleter.(TaskDeleterWithReason); ok { + return d.DeleteTaskWithReason(ctx, taskID, reason) + } + return deleter.DeleteTask(ctx, taskID) +} + func (s *Service) sweepReviewMRTasks(ctx context.Context, tasks []*ReviewMRTask, deleter TaskDeleter, checker TaskSessionChecker) int { client := s.Client() if client == nil { @@ -100,7 +118,7 @@ func (s *Service) deleteReviewMRTaskIfTerminal(ctx context.Context, t *ReviewMRT return false } } - if err := deleter.DeleteTask(ctx, t.TaskID); err != nil { + if err := deleteTaskWithReason(ctx, deleter, t.TaskID, reasonMRMergedOrClosed); err != nil { s.logger.Warn("delete review task during cleanup", zap.String("task_id", t.TaskID), zap.Error(err)) return false @@ -191,7 +209,7 @@ func (s *Service) deleteIssueWatchTaskIfTerminal(ctx context.Context, t *IssueWa return false } } - if err := deleter.DeleteTask(ctx, t.TaskID); err != nil { + if err := deleteTaskWithReason(ctx, deleter, t.TaskID, reasonIssueClosed); err != nil { s.logger.Warn("delete issue task during cleanup", zap.String("task_id", t.TaskID), zap.Error(err)) return false diff --git a/apps/backend/internal/gitlab/service_cleanup_reason_test.go b/apps/backend/internal/gitlab/service_cleanup_reason_test.go new file mode 100644 index 0000000000..42871f9340 --- /dev/null +++ b/apps/backend/internal/gitlab/service_cleanup_reason_test.go @@ -0,0 +1,173 @@ +package gitlab + +import ( + "context" + "testing" +) + +// recordingReasonDeleter implements both TaskDeleter and TaskDeleterWithReason +// so the cleanup path exercises the reason-threading branch (deleteTaskWithReason +// prefers DeleteTaskWithReason when the wired deleter supports it). +type recordingReasonDeleter struct { + taskID string + reason string +} + +func (r *recordingReasonDeleter) DeleteTask(_ context.Context, taskID string) error { + r.taskID = taskID + return nil +} + +func (r *recordingReasonDeleter) DeleteTaskWithReason(_ context.Context, taskID, reason string) error { + r.taskID = taskID + r.reason = reason + return nil +} + +// plainTaskDeleter implements only TaskDeleter (not TaskDeleterWithReason) so +// deleteTaskWithReason must take its documented fallback to DeleteTask. +type plainTaskDeleter struct{ taskID string } + +func (p *plainTaskDeleter) DeleteTask(_ context.Context, taskID string) error { + p.taskID = taskID + return nil +} + +// TestDeleteReviewMRTask_ThreadsMergedReason verifies that when a merged MR's +// review task is swept, the cleanup path deletes it with the +// "pr_merged_or_closed" reason so the frontend can explain the disappearance. +func TestDeleteReviewMRTask_ThreadsMergedReason(t *testing.T) { + svc := newServiceWithStore(t) + ctx := context.Background() + + const project = "team/repo" + mock := NewMockClient(svc.Host()) + mock.SeedMR(project, &MR{IID: 7, State: gitlabStateMerged}) + + rec := &recordingReasonDeleter{} + task := &ReviewMRTask{ID: "rmt-1", ProjectPath: project, MRIID: 7, TaskID: "task-merged"} + + if !svc.deleteReviewMRTaskIfTerminal(ctx, task, CleanupPolicyAlways, mock, rec, nil) { + t.Fatalf("expected deleteReviewMRTaskIfTerminal to delete the task") + } + if rec.taskID != "task-merged" { + t.Errorf("deleted taskID=%q, want task-merged", rec.taskID) + } + if rec.reason != reasonMRMergedOrClosed { + t.Errorf("reason=%q, want %q", rec.reason, reasonMRMergedOrClosed) + } +} + +// TestDeleteIssueWatchTask_ThreadsClosedReason mirrors the review case for the +// issue path: a closed issue's task is deleted with the "issue_closed" reason. +func TestDeleteIssueWatchTask_ThreadsClosedReason(t *testing.T) { + svc := newServiceWithStore(t) + ctx := context.Background() + + const project = "team/repo" + mock := NewMockClient(svc.Host()) + mock.SeedIssue(project, &Issue{IID: 5, State: gitlabStateClosed}) + + rec := &recordingReasonDeleter{} + task := &IssueWatchTask{ID: "iwt-1", ProjectPath: project, IssueIID: 5, TaskID: "task-closed"} + + if !svc.deleteIssueWatchTaskIfTerminal(ctx, task, CleanupPolicyAlways, mock, rec, nil) { + t.Fatalf("expected deleteIssueWatchTaskIfTerminal to delete the task") + } + if rec.taskID != "task-closed" { + t.Errorf("deleted taskID=%q, want task-closed", rec.taskID) + } + if rec.reason != reasonIssueClosed { + t.Errorf("reason=%q, want %q", rec.reason, reasonIssueClosed) + } +} + +// TestDeleteTaskWithReason_FallsBackWhenUnsupported covers the documented +// fallback: a deleter implementing only TaskDeleter is still invoked via +// DeleteTask (the reason is dropped) rather than panicking on the assertion. +func TestDeleteTaskWithReason_FallsBackWhenUnsupported(t *testing.T) { + rec := &plainTaskDeleter{} + if err := deleteTaskWithReason(context.Background(), rec, "task-x", reasonIssueClosed); err != nil { + t.Fatalf("deleteTaskWithReason: %v", err) + } + if rec.taskID != "task-x" { + t.Errorf("taskID=%q, want task-x (fallback DeleteTask must be called)", rec.taskID) + } +} + +// TestCleanupAllReviewTasks_ThreadsReason exercises the public wiring +// (SetTaskDeleter → CleanupAllReviewTasks → sweep) end-to-end, asserting the +// reason survives all the way to the deleter. +func TestCleanupAllReviewTasks_ThreadsReason(t *testing.T) { + svc := newServiceWithStore(t) + ctx := context.Background() + store := svc.store + + const project = "team/repo" + const iid = 7 + watch := &ReviewWatch{WorkspaceID: "ws-1", Enabled: true, CleanupPolicy: CleanupPolicyAlways} + if err := store.CreateReviewWatch(ctx, watch); err != nil { + t.Fatalf("CreateReviewWatch: %v", err) + } + if _, err := store.ReserveReviewMRTask(ctx, watch.ID, project, iid, "url"); err != nil { + t.Fatalf("ReserveReviewMRTask: %v", err) + } + if err := store.AssignReviewMRTaskID(ctx, watch.ID, project, iid, "task-merged"); err != nil { + t.Fatalf("AssignReviewMRTaskID: %v", err) + } + + mock := swapToMock(t, svc) + mock.SeedMR(project, &MR{IID: iid, State: gitlabStateMerged}) + + rec := &recordingReasonDeleter{} + svc.SetTaskDeleter(rec) + + deleted, err := svc.CleanupAllReviewTasks(ctx) + if err != nil { + t.Fatalf("CleanupAllReviewTasks: %v", err) + } + if deleted != 1 { + t.Fatalf("deleted=%d, want 1", deleted) + } + if rec.reason != reasonMRMergedOrClosed { + t.Errorf("reason=%q, want %q", rec.reason, reasonMRMergedOrClosed) + } +} + +// TestCleanupAllIssueTasks_ThreadsReason is the issue-path mirror of the +// review public-path test above. +func TestCleanupAllIssueTasks_ThreadsReason(t *testing.T) { + svc := newServiceWithStore(t) + ctx := context.Background() + store := svc.store + + const project = "team/repo" + const iid = 5 + watch := &IssueWatch{WorkspaceID: "ws-1", Enabled: true, CleanupPolicy: CleanupPolicyAlways} + if err := store.CreateIssueWatch(ctx, watch); err != nil { + t.Fatalf("CreateIssueWatch: %v", err) + } + if _, err := store.ReserveIssueWatchTask(ctx, watch.ID, project, iid, "url"); err != nil { + t.Fatalf("ReserveIssueWatchTask: %v", err) + } + if err := store.AssignIssueWatchTaskID(ctx, watch.ID, project, iid, "task-closed"); err != nil { + t.Fatalf("AssignIssueWatchTaskID: %v", err) + } + + mock := swapToMock(t, svc) + mock.SeedIssue(project, &Issue{IID: iid, State: gitlabStateClosed}) + + rec := &recordingReasonDeleter{} + svc.SetTaskDeleter(rec) + + deleted, err := svc.CleanupAllIssueTasks(ctx) + if err != nil { + t.Fatalf("CleanupAllIssueTasks: %v", err) + } + if deleted != 1 { + t.Fatalf("deleted=%d, want 1", deleted) + } + if rec.reason != reasonIssueClosed { + t.Errorf("reason=%q, want %q", rec.reason, reasonIssueClosed) + } +} diff --git a/apps/backend/internal/task/service/service_events.go b/apps/backend/internal/task/service/service_events.go index 225a1b474f..aa6dd23d1b 100644 --- a/apps/backend/internal/task/service/service_events.go +++ b/apps/backend/internal/task/service/service_events.go @@ -31,6 +31,14 @@ func (s *Service) PublishTaskDeleted(ctx context.Context, task *models.Task) { // publishTaskEvent publishes task events to the event bus func (s *Service) publishTaskEvent(ctx context.Context, eventType string, task *models.Task, oldState *v1.TaskState, oldWorkflowIDs ...string) { + s.publishTaskEventWithExtra(ctx, eventType, task, oldState, nil, oldWorkflowIDs...) +} + +// publishTaskEventWithExtra is publishTaskEvent with caller-supplied extra +// fields merged into the payload (e.g. a deletion reason on task.deleted). +// Caller-supplied keys must not shadow the standard task fields written below +// (task_id, title, workflow_id, etc.); colliding keys silently overwrite them. +func (s *Service) publishTaskEventWithExtra(ctx context.Context, eventType string, task *models.Task, oldState *v1.TaskState, extra map[string]interface{}, oldWorkflowIDs ...string) { if s.eventBus == nil { return } @@ -79,6 +87,9 @@ func (s *Service) publishTaskEvent(ctx context.Context, eventType string, task * if len(oldWorkflowIDs) > 0 && oldWorkflowIDs[0] != "" && oldWorkflowIDs[0] != task.WorkflowID { data["old_workflow_id"] = oldWorkflowIDs[0] } + for k, v := range extra { + data[k] = v + } event := bus.NewEvent(eventType, "task-service", data) if err := s.eventBus.Publish(ctx, eventType, event); err != nil { diff --git a/apps/backend/internal/task/service/service_tasks.go b/apps/backend/internal/task/service/service_tasks.go index 4978d717cf..e9bc9b18f7 100644 --- a/apps/backend/internal/task/service/service_tasks.go +++ b/apps/backend/internal/task/service/service_tasks.go @@ -837,6 +837,17 @@ func (s *Service) ArchiveTask(ctx context.Context, id string) error { // For fast UI response, the DB delete and event publish happen synchronously, // while agent stopping and worktree cleanup happen asynchronously. func (s *Service) DeleteTask(ctx context.Context, id string) error { + return s.deleteTaskWithReason(ctx, id, "") +} + +// DeleteTaskWithReason behaves like DeleteTask but attaches a machine-readable +// reason (e.g. "pr_approved_by_user") to the task.deleted event so the frontend +// can explain why a focused task vanished. +func (s *Service) DeleteTaskWithReason(ctx context.Context, id, reason string) error { + return s.deleteTaskWithReason(ctx, id, reason) +} + +func (s *Service) deleteTaskWithReason(ctx context.Context, id, reason string) error { start := time.Now() // 1. Get task (sync, fast) @@ -879,7 +890,11 @@ func (s *Service) DeleteTask(ctx context.Context, id string) error { } // 5. Publish event (sync, fast) - frontend removes task immediately - s.publishTaskEvent(ctx, events.TaskDeleted, task, nil) + var extra map[string]interface{} + if reason != "" { + extra = map[string]interface{}{"reason": reason} + } + s.publishTaskEventWithExtra(ctx, events.TaskDeleted, task, nil, extra) s.logger.Info("task deleted", zap.String("task_id", id), zap.Duration("duration", time.Since(start))) diff --git a/apps/backend/internal/task/service/service_test.go b/apps/backend/internal/task/service/service_test.go index e4d15d8235..2a8010b860 100644 --- a/apps/backend/internal/task/service/service_test.go +++ b/apps/backend/internal/task/service/service_test.go @@ -479,6 +479,61 @@ func TestService_DeleteTask(t *testing.T) { } } +func TestService_DeleteTaskWithReason_PublishesReason(t *testing.T) { + svc, eventBus, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "Workspace"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-123", WorkspaceID: "ws-1", Name: "Workflow"}) + _ = repo.CreateTask(ctx, &models.Task{ID: "task-123", WorkspaceID: "ws-1", WorkflowID: "wf-123", WorkflowStepID: "step-123", Title: "Test", Priority: "medium"}) + eventBus.ClearEvents() + + if err := svc.DeleteTaskWithReason(ctx, "task-123", "pr_approved_by_user"); err != nil { + t.Fatalf("DeleteTaskWithReason: %v", err) + } + + events := eventBus.GetPublishedEvents() + if len(events) != 1 { + t.Fatalf("expected 1 event, got %d", len(events)) + } + if events[0].Type != "task.deleted" { + t.Fatalf("expected event type 'task.deleted', got %s", events[0].Type) + } + data, ok := events[0].Data.(map[string]interface{}) + if !ok { + t.Fatalf("event data is not map[string]interface{}, got %T", events[0].Data) + } + if got := data["reason"]; got != "pr_approved_by_user" { + t.Errorf("expected reason 'pr_approved_by_user' in payload, got %v", got) + } +} + +func TestService_DeleteTask_OmitsReasonWhenUnset(t *testing.T) { + svc, eventBus, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "Workspace"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-123", WorkspaceID: "ws-1", Name: "Workflow"}) + _ = repo.CreateTask(ctx, &models.Task{ID: "task-123", WorkspaceID: "ws-1", WorkflowID: "wf-123", WorkflowStepID: "step-123", Title: "Test", Priority: "medium"}) + eventBus.ClearEvents() + + if err := svc.DeleteTask(ctx, "task-123"); err != nil { + t.Fatalf("DeleteTask: %v", err) + } + + events := eventBus.GetPublishedEvents() + if len(events) != 1 { + t.Fatalf("expected 1 event, got %d", len(events)) + } + data, ok := events[0].Data.(map[string]interface{}) + if !ok { + t.Fatalf("event data is not map[string]interface{}, got %T", events[0].Data) + } + if _, present := data["reason"]; present { + t.Errorf("expected no reason key when deleting without a reason, got %v", data["reason"]) + } +} + func TestService_DeleteTaskStopsExecutorRunningForTerminalSession(t *testing.T) { svc, _, repo := createTestService(t) ctx := context.Background() diff --git a/apps/web/components/task-deleted-toast-bridge.tsx b/apps/web/components/task-deleted-toast-bridge.tsx new file mode 100644 index 0000000000..eaf04ac153 --- /dev/null +++ b/apps/web/components/task-deleted-toast-bridge.tsx @@ -0,0 +1,9 @@ +"use client"; + +import { useTaskDeletedToast } from "@/hooks/use-task-deleted-toast"; + +/** Mounts the task-deleted toast hook inside the ToastProvider tree. */ +export function TaskDeletedToastBridge() { + useTaskDeletedToast(); + return null; +} diff --git a/apps/web/hooks/use-task-deleted-toast.test.ts b/apps/web/hooks/use-task-deleted-toast.test.ts new file mode 100644 index 0000000000..c9c15c28ff --- /dev/null +++ b/apps/web/hooks/use-task-deleted-toast.test.ts @@ -0,0 +1,72 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { renderHook } from "@testing-library/react"; +import type { TaskDeletedNotification } from "@/lib/state/slices/ui/types"; + +let mockNotification: TaskDeletedNotification | null = null; +const mockClearNotification = vi.fn(); +const mockToast = vi.fn(); + +vi.mock("@/components/state-provider", () => ({ + useAppStore: (selector: (state: Record) => unknown) => + selector({ + taskDeletedNotification: mockNotification, + setTaskDeletedNotification: mockClearNotification, + }), +})); + +vi.mock("@/components/toast-provider", () => ({ + useToast: () => ({ toast: mockToast }), +})); + +import { useTaskDeletedToast } from "./use-task-deleted-toast"; + +describe("useTaskDeletedToast", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockNotification = null; + }); + + it("shows a reason-specific toast for an approved PR", () => { + mockNotification = { taskId: "t-1", title: "Review PR #11259", reason: "pr_approved_by_user" }; + renderHook(() => useTaskDeletedToast()); + + expect(mockToast).toHaveBeenCalledWith({ + title: '"Review PR #11259" was closed', + description: "Its pull request was approved, so this review task was closed automatically.", + }); + expect(mockClearNotification).toHaveBeenCalledWith(null); + }); + + it("falls back to a generic message when no reason is provided", () => { + mockNotification = { taskId: "t-2" }; + renderHook(() => useTaskDeletedToast()); + + expect(mockToast).toHaveBeenCalledWith({ + title: "Task closed", + description: "This task was closed automatically.", + }); + }); + + it("does not show a toast when notification is null", () => { + mockNotification = null; + renderHook(() => useTaskDeletedToast()); + + expect(mockToast).not.toHaveBeenCalled(); + expect(mockClearNotification).not.toHaveBeenCalled(); + }); + + it("deduplicates toasts for the same taskId across rerenders", () => { + mockNotification = { taskId: "t-1", reason: "pr_approved_by_user" }; + const { rerender } = renderHook(() => useTaskDeletedToast()); + expect(mockToast).toHaveBeenCalledTimes(1); + + mockToast.mockClear(); + mockClearNotification.mockClear(); + + mockNotification = { taskId: "t-1", reason: "pr_approved_by_user" }; + rerender(); + + expect(mockToast).not.toHaveBeenCalled(); + expect(mockClearNotification).toHaveBeenCalledWith(null); + }); +}); diff --git a/apps/web/hooks/use-task-deleted-toast.ts b/apps/web/hooks/use-task-deleted-toast.ts new file mode 100644 index 0000000000..02c0d8d66b --- /dev/null +++ b/apps/web/hooks/use-task-deleted-toast.ts @@ -0,0 +1,49 @@ +"use client"; + +import { useEffect, useRef } from "react"; +import { useAppStore } from "@/components/state-provider"; +import { useToast } from "@/components/toast-provider"; +import type { TaskDeletionReason } from "@/lib/types/http"; + +// Exhaustive over TaskDeletionReason: adding or renaming a reason is a compile +// error here until a matching message is provided. +const REASON_MESSAGES: Record = { + pr_approved_by_user: + "Its pull request was approved, so this review task was closed automatically.", + pr_merged_or_closed: + "Its pull request was merged or closed, so this review task was closed automatically.", + issue_closed: "Its issue was closed, so this task was closed automatically.", +}; + +/** Maps a backend deletion reason (an untyped wire string) to an explanation. */ +function describeReason(reason: string | undefined): string { + return ( + (reason && REASON_MESSAGES[reason as TaskDeletionReason]) ?? + "This task was closed automatically." + ); +} + +/** + * Watches for task-deleted notifications and shows an explanatory toast when the + * focused task is removed out from under the user. Mount once inside ToastProvider. + */ +export function useTaskDeletedToast() { + const notification = useAppStore((s) => s.taskDeletedNotification); + const clearNotification = useAppStore((s) => s.setTaskDeletedNotification); + const { toast } = useToast(); + const shownRef = useRef>(new Set()); + + useEffect(() => { + if (!notification) return; + if (shownRef.current.has(notification.taskId)) { + clearNotification(null); + return; + } + shownRef.current.add(notification.taskId); + toast({ + title: notification.title ? `"${notification.title}" was closed` : "Task closed", + description: describeReason(notification.reason), + }); + clearNotification(null); + }, [notification, toast, clearNotification]); +} diff --git a/apps/web/lib/links.test.ts b/apps/web/lib/links.test.ts index 3d2ab38a4d..69e6e684bb 100644 --- a/apps/web/lib/links.test.ts +++ b/apps/web/lib/links.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { linkToTask, linkToTasks, replaceTaskUrl } from "./links"; +import { isTaskDetailPath, linkToTask, linkToTasks, replaceTaskUrl } from "./links"; describe("task links", () => { afterEach(() => { @@ -24,3 +24,26 @@ describe("task links", () => { expect(replaceState).toHaveBeenCalledWith({}, "", "/t/task-123"); }); }); + +describe("isTaskDetailPath", () => { + it("matches the canonical and compatibility detail routes", () => { + expect(isTaskDetailPath("/t/task-123", "task-123")).toBe(true); + expect(isTaskDetailPath("/tasks/task-123", "task-123")).toBe(true); + }); + + it("matches trailing-slash variants the SPA normalizes", () => { + expect(isTaskDetailPath("/t/task-123/", "task-123")).toBe(true); + expect(isTaskDetailPath("/tasks/task-123/", "task-123")).toBe(true); + }); + + it("does not match a different task id", () => { + expect(isTaskDetailPath("/t/other", "task-123")).toBe(false); + expect(isTaskDetailPath("/tasks/other", "task-123")).toBe(false); + }); + + it("does not match the task list route or unrelated paths", () => { + expect(isTaskDetailPath("/tasks", "task-123")).toBe(false); + expect(isTaskDetailPath("/", "task-123")).toBe(false); + expect(isTaskDetailPath("/t/task-123/extra", "task-123")).toBe(false); + }); +}); diff --git a/apps/web/lib/links.ts b/apps/web/lib/links.ts index 6bbde472f0..c04863d6e4 100644 --- a/apps/web/lib/links.ts +++ b/apps/web/lib/links.ts @@ -3,6 +3,19 @@ export function linkToTask(taskId: string, layout?: string): string { return layout ? `${base}?layout=${encodeURIComponent(layout)}` : base; } +/** Task-detail route prefixes the SPA serves: canonical and compatibility. */ +const TASK_DETAIL_PREFIXES = ["/t/", "/tasks/"]; + +/** + * True when `pathname` is a task-detail route for `taskId`. Matches both the + * canonical `/t/:id` and the compatibility `/tasks/:id` paths. + */ +export function isTaskDetailPath(pathname: string, taskId: string): boolean { + const normalized = + pathname.length > 1 && pathname.endsWith("/") ? pathname.slice(0, -1) : pathname; + return TASK_DETAIL_PREFIXES.some((prefix) => normalized === `${prefix}${taskId}`); +} + /** Replace the browser URL to reflect the active task (no navigation). */ export function replaceTaskUrl(taskId: string): void { if (typeof window === "undefined") return; diff --git a/apps/web/lib/routing/client-router.ts b/apps/web/lib/routing/client-router.ts index ccabadf07f..d16807edf9 100644 --- a/apps/web/lib/routing/client-router.ts +++ b/apps/web/lib/routing/client-router.ts @@ -57,6 +57,16 @@ function navigate(mode: "push" | "replace", href: string, options?: NavigateOpti dispatchLocationChange(); } +/** + * Imperative soft navigation usable outside React (e.g. WebSocket handlers). + * Mirrors what `useRouter().push/replace` does so a redirect triggered from a + * store mutation re-renders the SPA without a full reload. + */ +export function softNavigate(href: string, mode: "push" | "replace" = "push"): void { + if (typeof window === "undefined") return; + navigate(mode, href); +} + function useLocationKey(): string { return useSyncExternalStore(subscribeLocation, getLocationKey, getServerLocationKey); } diff --git a/apps/web/lib/state/slices/ui/types.ts b/apps/web/lib/state/slices/ui/types.ts index 42afca4163..873988828b 100644 --- a/apps/web/lib/state/slices/ui/types.ts +++ b/apps/web/lib/state/slices/ui/types.ts @@ -98,6 +98,14 @@ export type SessionFailureNotification = { message: string; }; +export type TaskDeletedNotification = { + taskId: string; + /** Task title, when known, so the toast can name it. */ + title?: string; + /** Backend deletion reason (e.g. "pr_approved_by_user"), when known. */ + reason?: string; +}; + export type BottomTerminalState = { isOpen: boolean; pendingCommand: string | null; @@ -146,6 +154,8 @@ export type UISliceState = { quickChat: QuickChatState; configChat: ConfigChatState; sessionFailureNotification: SessionFailureNotification | null; + /** Set when the focused task is deleted live, so a toast can explain why. */ + taskDeletedNotification: TaskDeletedNotification | null; bottomTerminal: BottomTerminalState; sidebarViews: SidebarSliceState; /** Parent task IDs whose subtasks are collapsed in the sidebar. Tab-scoped (sessionStorage). */ @@ -196,6 +206,7 @@ export type UISliceActions = { setActiveConfigChatSession: (sessionId: string) => void; renameConfigChatSession: (sessionId: string, name: string) => void; setSessionFailureNotification: (n: SessionFailureNotification | null) => void; + setTaskDeletedNotification: (n: TaskDeletedNotification | null) => void; toggleBottomTerminal: () => void; openBottomTerminalWithCommand: (command: string) => void; clearBottomTerminalCommand: () => void; diff --git a/apps/web/lib/state/slices/ui/ui-slice.ts b/apps/web/lib/state/slices/ui/ui-slice.ts index 4979b9abb8..45b6cf5ccc 100644 --- a/apps/web/lib/state/slices/ui/ui-slice.ts +++ b/apps/web/lib/state/slices/ui/ui-slice.ts @@ -91,6 +91,7 @@ export const defaultUIState: UISliceState = { quickChat: { isOpen: false, sessions: [], activeSessionId: null }, configChat: { isOpen: false, sessions: [], activeSessionId: null, workspaceId: null }, sessionFailureNotification: null, + taskDeletedNotification: null, bottomTerminal: { isOpen: false, pendingCommand: null }, sidebarViews: loadSidebarState(), collapsedSubtaskParents: [], @@ -292,6 +293,19 @@ function buildConfigChatActions(set: ImmerSet) { }; } +function buildNotificationActions(set: ImmerSet) { + return { + setSessionFailureNotification: (n: UISlice["sessionFailureNotification"]) => + set((draft) => { + draft.sessionFailureNotification = n; + }), + setTaskDeletedNotification: (n: UISlice["taskDeletedNotification"]) => + set((draft) => { + draft.taskDeletedNotification = n; + }), + }; +} + export const createUISlice: StateCreator = ( set, get, @@ -316,6 +330,7 @@ export const createUISlice: StateCreator set((draft) => { draft.rightPanel.activeTabBySessionId[sessionId] = tab; @@ -393,8 +408,4 @@ export const createUISlice: StateCreator - set((draft) => { - draft.sessionFailureNotification = n; - }), }); diff --git a/apps/web/lib/state/store.ts b/apps/web/lib/state/store.ts index 6951fe72bb..dbd439686a 100644 --- a/apps/web/lib/state/store.ts +++ b/apps/web/lib/state/store.ts @@ -228,6 +228,7 @@ export type AppState = { quickChat: (typeof defaultUIState)["quickChat"]; configChat: (typeof defaultUIState)["configChat"]; sessionFailureNotification: (typeof defaultUIState)["sessionFailureNotification"]; + taskDeletedNotification: (typeof defaultUIState)["taskDeletedNotification"]; bottomTerminal: (typeof defaultUIState)["bottomTerminal"]; sidebarViews: (typeof defaultUIState)["sidebarViews"]; collapsedSubtaskParents: (typeof defaultUIState)["collapsedSubtaskParents"]; @@ -380,6 +381,7 @@ export type AppState = { setActiveConfigChatSession: (sessionId: string) => void; renameConfigChatSession: (sessionId: string, name: string) => void; setSessionFailureNotification: (n: UISliceTypes.SessionFailureNotification | null) => void; + setTaskDeletedNotification: (n: UISliceTypes.TaskDeletedNotification | null) => void; toggleBottomTerminal: () => void; openBottomTerminalWithCommand: (command: string) => void; clearBottomTerminalCommand: () => void; diff --git a/apps/web/lib/types/backend.ts b/apps/web/lib/types/backend.ts index b3790b4aab..4147493b45 100644 --- a/apps/web/lib/types/backend.ts +++ b/apps/web/lib/types/backend.ts @@ -157,6 +157,8 @@ export type TaskEventPayload = { archived_at?: string | null; updated_at?: string; is_ephemeral: boolean; + /** Deletion reason on task.deleted (e.g. "pr_approved_by_user"). Absent otherwise. */ + reason?: string; }; export type AgentUpdatePayload = { diff --git a/apps/web/lib/types/http.ts b/apps/web/lib/types/http.ts index 9cb7e23b79..ccca35c9f1 100644 --- a/apps/web/lib/types/http.ts +++ b/apps/web/lib/types/http.ts @@ -51,6 +51,9 @@ export type TaskState = // Workflow Review Status export type WorkflowReviewStatus = "pending" | "approved" | "changes_requested" | "rejected"; +// Reasons the backend tags on an auto-deleted task.deleted event. +export type TaskDeletionReason = "pr_approved_by_user" | "pr_merged_or_closed" | "issue_closed"; + // Workflow Template - pre-defined workflow configurations export type WorkflowTemplate = { id: string; diff --git a/apps/web/lib/ws/handlers/tasks.test.ts b/apps/web/lib/ws/handlers/tasks.test.ts index 93102096b3..69c4a231f9 100644 --- a/apps/web/lib/ws/handlers/tasks.test.ts +++ b/apps/web/lib/ws/handlers/tasks.test.ts @@ -55,6 +55,7 @@ function makeStore(initial: Partial = {}) { }; }), removeTaskFromSidebarPrefs: vi.fn(), + setTaskDeletedNotification: vi.fn(), ...initial, } as unknown as AppState; @@ -107,6 +108,34 @@ function makeDeletedMessage(payload: Record) { } as Parameters["task.deleted"]>>[0]; } +const REVIEW_TITLE = "Review PR #11259"; + +function makeActiveStore() { + return makeStore({ + kanban: { workflowId: "wf1", steps: [], tasks: [{ id: "t1", workflowId: "wf1" }] }, + tasks: { + activeTaskId: "t1", + activeSessionId: null, + pinnedSessionId: null, + lastSessionByTaskId: {}, + }, + environmentIdBySessionId: {}, + } as unknown as Partial); +} + +function makeInactiveStore() { + return makeStore({ + kanban: { workflowId: "wf1", steps: [], tasks: [{ id: "t1", workflowId: "wf1" }] }, + tasks: { + activeTaskId: null, + activeSessionId: null, + pinnedSessionId: null, + lastSessionByTaskId: {}, + }, + environmentIdBySessionId: {}, + } as unknown as Partial); +} + describe("task.updated primary-session focus follow", () => { let store: ReturnType; let setActiveSessionAuto: ReturnType; @@ -553,3 +582,102 @@ describe("task.deleted cleanup", () => { expect(state.tasks.lastSessionByTaskId).toHaveProperty("t2", SESS_OTHER); }); }); + +describe("task.deleted live notification + redirect", () => { + it("sets a task-deleted notification (with title + reason) when the focused task is deleted", () => { + const store = makeActiveStore(); + const handlers = registerTasksHandlers(store); + + handlers["task.deleted"]!( + makeDeletedMessage({ + task_id: "t1", + workflow_id: "wf1", + title: REVIEW_TITLE, + reason: "pr_approved_by_user", + }), + ); + + expect(store.getState().setTaskDeletedNotification).toHaveBeenCalledWith({ + taskId: "t1", + title: REVIEW_TITLE, + reason: "pr_approved_by_user", + }); + }); + + it("does not notify when a non-focused task is deleted", () => { + const store = makeStore({ + kanban: { workflowId: "wf1", steps: [], tasks: [{ id: "t1", workflowId: "wf1" }] }, + tasks: { + activeTaskId: "t2", + activeSessionId: null, + pinnedSessionId: null, + lastSessionByTaskId: {}, + }, + environmentIdBySessionId: {}, + } as unknown as Partial); + const handlers = registerTasksHandlers(store); + + handlers["task.deleted"]!(makeDeletedMessage({ task_id: "t1", workflow_id: "wf1" })); + + expect(store.getState().setTaskDeletedNotification).not.toHaveBeenCalled(); + }); + + it("does not redirect or notify for a user-initiated delete (no reason) even on the task route", () => { + window.history.replaceState({}, "", "/t/t1"); + const store = makeActiveStore(); + const handlers = registerTasksHandlers(store); + + handlers["task.deleted"]!( + makeDeletedMessage({ task_id: "t1", workflow_id: "wf1", title: REVIEW_TITLE }), + ); + + // The local delete flow owns navigation for user-initiated deletes; the WS + // handler must not preempt it by redirecting. + expect(window.location.pathname).toBe("/t/t1"); + expect(store.getState().setTaskDeletedNotification).not.toHaveBeenCalled(); + }); + + // Covers both the canonical `/t/:id` and compatibility `/tasks/:id` routes, + // and the not-yet-hydrated case (activeTaskId still null) via makeInactiveStore. + it.each(["/t/t1", "/tasks/t1"])( + "redirects home and notifies when parked on %s before activeTaskId hydrates", + (path) => { + window.history.replaceState({}, "", path); + const store = makeInactiveStore(); + const handlers = registerTasksHandlers(store); + + handlers["task.deleted"]!( + makeDeletedMessage({ + task_id: "t1", + workflow_id: "wf1", + title: REVIEW_TITLE, + reason: "pr_approved_by_user", + }), + ); + + expect(window.location.pathname).toBe("/"); + expect(store.getState().setTaskDeletedNotification).toHaveBeenCalledWith({ + taskId: "t1", + title: REVIEW_TITLE, + reason: "pr_approved_by_user", + }); + }, + ); + + it("does not redirect an auto-deletion when viewing a different route", () => { + window.history.replaceState({}, "", "/t/other"); + const store = makeActiveStore(); + const handlers = registerTasksHandlers(store); + + handlers["task.deleted"]!( + makeDeletedMessage({ + task_id: "t1", + workflow_id: "wf1", + title: REVIEW_TITLE, + reason: "pr_approved_by_user", + }), + ); + + expect(window.location.pathname).toBe("/t/other"); + }); +}); diff --git a/apps/web/lib/ws/handlers/tasks.ts b/apps/web/lib/ws/handlers/tasks.ts index 69ac3264c7..ee5871bd4a 100644 --- a/apps/web/lib/ws/handlers/tasks.ts +++ b/apps/web/lib/ws/handlers/tasks.ts @@ -8,6 +8,8 @@ import { useContextFilesStore } from "@/lib/state/context-files-store"; import { toKanbanTask, type TaskLike } from "@/lib/kanban/map-task"; import { sessionId as toSessionId } from "@/lib/types/http"; import { mergeTaskRepositoryFields } from "@/lib/ws/handlers/task-repositories"; +import { softNavigate } from "@/lib/routing/client-router"; +import { isTaskDetailPath } from "@/lib/links"; import { clearPinnedSessionIfOverridden, shouldPreservePinnedSessionForTask, @@ -117,6 +119,18 @@ function removeTaskFromBothKanbans(state: AppState, wfId: string, taskId: string return next; } +/** + * Soft-redirect away from a deleted task's page. Only fires when the user is + * currently parked on that task's route (`/t/` or the compatibility + * `/tasks/`), so a background deletion of some other task never yanks the + * user elsewhere. + */ +function redirectAwayFromDeletedTask(deletedId: string): void { + if (typeof window === "undefined") return; + if (!isTaskDetailPath(window.location.pathname, deletedId)) return; + softNavigate("/", "replace"); +} + export function registerTasksHandlers(store: StoreApi): WsHandlers { return { "task.created": (message) => { @@ -205,6 +219,8 @@ export function registerTasksHandlers(store: StoreApi): WsHandlers { useContextFilesStore.getState().clearSession(sid); } + const wasActive = currentState.tasks.activeTaskId === deletedId; + store.setState((state) => { const isActive = state.tasks.activeTaskId === deletedId; let next = removeTaskFromBothKanbans(state, wfId, deletedId); @@ -217,6 +233,28 @@ export function registerTasksHandlers(store: StoreApi): WsHandlers { } return next; }); + + // Capture the route match before any redirect mutates the pathname. This + // covers a fresh load where the browser is parked on the task's route + // (`/t/` or `/tasks/`) but TaskPageContent hasn't hydrated + // `activeTaskId` yet, so `wasActive` is still false. + const onDeletedRoute = + typeof window !== "undefined" && isTaskDetailPath(window.location.pathname, deletedId); + + // Only react to genuine auto-deletions, which the backend tags with a + // reason (e.g. a review task whose PR was approved). User-initiated deletes + // carry no reason: their local delete flow (useTaskRemoval) owns + // navigation by switching to the next task, so redirecting here would + // preempt it and strand the user on the home route. For auto-deletions we + // move off the now-dead route (helper is route-guarded) and explain why. + if (message.payload.reason && (wasActive || onDeletedRoute)) { + redirectAwayFromDeletedTask(deletedId); + store.getState().setTaskDeletedNotification({ + taskId: deletedId, + title: message.payload.title, + reason: message.payload.reason, + }); + } }, "task.state_changed": (message) => { // Skip ephemeral tasks (e.g., quick chat) - they shouldn't appear on the Kanban board diff --git a/apps/web/src/app-shell.tsx b/apps/web/src/app-shell.tsx index ed6a71de2a..b92de3cb98 100644 --- a/apps/web/src/app-shell.tsx +++ b/apps/web/src/app-shell.tsx @@ -7,6 +7,7 @@ import { LogBufferBridge } from "@/components/log-buffer-bridge"; import { QuickChatProvider } from "@/components/quick-chat/quick-chat-provider"; import { RecentTaskSwitcher } from "@/components/task/recent-task-switcher"; import { SessionFailureToastBridge } from "@/components/session-failure-toast-bridge"; +import { TaskDeletedToastBridge } from "@/components/task-deleted-toast-bridge"; import { SidebarViewsSyncBridge } from "@/components/sidebar-views-sync-bridge"; import { ThemeProvider } from "@/components/theme-provider"; import { ToastProvider } from "@/components/toast-provider"; @@ -27,6 +28,7 @@ export function AppShell({ children }: AppShellProps) { +