feat: redirect and toast when focused task is auto-deleted (#1544)

This commit is contained in:
Carlos Florêncio
2026-06-29 19:10:08 +01:00
committed by GitHub
parent c53b8f4bea
commit 5ed975ddbf
24 changed files with 767 additions and 11 deletions
@@ -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
}
+8
View File
@@ -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.
@@ -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",
@@ -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)
}
}
+8
View File
@@ -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)
@@ -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
@@ -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)
}
}
@@ -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 {
@@ -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)))
@@ -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()
@@ -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;
}
@@ -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<string, unknown>) => 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);
});
});
+49
View File
@@ -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<TaskDeletionReason, string> = {
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<Set<string>>(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]);
}
+24 -1
View File
@@ -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);
});
});
+13
View File
@@ -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;
+10
View File
@@ -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);
}
+11
View File
@@ -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;
+15 -4
View File
@@ -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<UISlice, [["zustand/immer", never]], [], UISlice> = (
set,
get,
@@ -316,6 +330,7 @@ export const createUISlice: StateCreator<UISlice, [["zustand/immer", never]], []
...buildCollapsedSubtaskActions(set, get),
...buildSystemHealthActions(set),
...buildDismissedAgentErrors(set),
...buildNotificationActions(set),
setRightPanelActiveTab: (sessionId, tab) =>
set((draft) => {
draft.rightPanel.activeTabBySessionId[sessionId] = tab;
@@ -393,8 +408,4 @@ export const createUISlice: StateCreator<UISlice, [["zustand/immer", never]], []
});
if (renamed) setStoredQuickChatName(sessionId, name);
},
setSessionFailureNotification: (n) =>
set((draft) => {
draft.sessionFailureNotification = n;
}),
});
+2
View File
@@ -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;
+2
View File
@@ -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 = {
+3
View File
@@ -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;
+128
View File
@@ -55,6 +55,7 @@ function makeStore(initial: Partial<AppState> = {}) {
};
}),
removeTaskFromSidebarPrefs: vi.fn(),
setTaskDeletedNotification: vi.fn(),
...initial,
} as unknown as AppState;
@@ -107,6 +108,34 @@ function makeDeletedMessage(payload: Record<string, unknown>) {
} as Parameters<NonNullable<ReturnType<typeof registerTasksHandlers>["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<AppState>);
}
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<AppState>);
}
describe("task.updated primary-session focus follow", () => {
let store: ReturnType<typeof makeStore>;
let setActiveSessionAuto: ReturnType<typeof vi.fn>;
@@ -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<AppState>);
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");
});
});
+38
View File
@@ -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/<id>` or the compatibility
* `/tasks/<id>`), 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<AppState>): WsHandlers {
return {
"task.created": (message) => {
@@ -205,6 +219,8 @@ export function registerTasksHandlers(store: StoreApi<AppState>): 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<AppState>): 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/<id>` or `/tasks/<id>`) 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
+2
View File
@@ -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) {
<ToastProvider>
<SonnerToaster richColors position="top-right" />
<SessionFailureToastBridge />
<TaskDeletedToastBridge />
<SidebarViewsSyncBridge />
<LogBufferBridge />
<CommandRegistryProvider>