fix: surface orphaned tasks in pipeline view after step deletion (#1809)
This commit is contained in:
@@ -880,7 +880,7 @@ func registerSecondaryRoutes(
|
||||
planService *taskservice.PlanService,
|
||||
handoffSvc *taskservice.HandoffService,
|
||||
) {
|
||||
workflowhandlers.RegisterRoutes(p.router, p.gateway.Dispatcher, workflowCtrl, p.log)
|
||||
workflowhandlers.RegisterRoutes(p.router, p.gateway.Dispatcher, workflowCtrl, p.eventBus, p.log)
|
||||
p.log.Info("Registered Workflow handlers (HTTP + WebSocket)")
|
||||
|
||||
agentsettingshandlers.RegisterRoutes(p.router, p.agentSettingsController, p.gateway.Hub, p.log)
|
||||
|
||||
@@ -119,7 +119,7 @@ func NewTestServer(t *testing.T) *TestServer {
|
||||
taskhandlers.RegisterRepositoryRoutes(router, gateway.Dispatcher, taskSvc, log)
|
||||
taskhandlers.RegisterExecutorRoutes(router, gateway.Dispatcher, taskSvc, log)
|
||||
taskhandlers.RegisterEnvironmentRoutes(router, gateway.Dispatcher, taskSvc, log)
|
||||
workflowhandlers.RegisterRoutes(router, gateway.Dispatcher, workflowCtrl, log)
|
||||
workflowhandlers.RegisterRoutes(router, gateway.Dispatcher, workflowCtrl, eventBus, log)
|
||||
|
||||
// Create test server
|
||||
server := httptest.NewServer(router)
|
||||
|
||||
@@ -348,7 +348,7 @@ func NewOrchestratorTestServer(t *testing.T) *OrchestratorTestServer {
|
||||
taskhandlers.RegisterRepositoryRoutes(router, gateway.Dispatcher, taskSvc, log)
|
||||
taskhandlers.RegisterExecutorRoutes(router, gateway.Dispatcher, taskSvc, log)
|
||||
taskhandlers.RegisterEnvironmentRoutes(router, gateway.Dispatcher, taskSvc, log)
|
||||
workflowhandlers.RegisterRoutes(router, gateway.Dispatcher, workflowCtrl, log)
|
||||
workflowhandlers.RegisterRoutes(router, gateway.Dispatcher, workflowCtrl, eventBus, log)
|
||||
|
||||
// Create test server
|
||||
server := httptest.NewServer(router)
|
||||
|
||||
@@ -12,6 +12,7 @@ import (
|
||||
"github.com/kandev/kandev/internal/task/dto"
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
"github.com/kandev/kandev/internal/task/service"
|
||||
workflowmodels "github.com/kandev/kandev/internal/workflow/models"
|
||||
v1 "github.com/kandev/kandev/pkg/api/v1"
|
||||
ws "github.com/kandev/kandev/pkg/websocket"
|
||||
"github.com/stretchr/testify/assert"
|
||||
@@ -619,6 +620,188 @@ func TestHandleMoveTask_ActiveSessionWithoutPrompt_DefersMove(t *testing.T) {
|
||||
assert.Equal(t, "step-work", task.WorkflowStepID, "deferred move must not apply immediately")
|
||||
}
|
||||
|
||||
// seedRunningTask creates a workspace, workflow, task, and a RUNNING session so
|
||||
// handleMoveTask takes the deferred path. Returns the task ID.
|
||||
func seedRunningTask(t *testing.T, repo interface {
|
||||
CreateWorkspace(context.Context, *models.Workspace) error
|
||||
CreateWorkflow(context.Context, *models.Workflow) error
|
||||
CreateTask(context.Context, *models.Task) error
|
||||
CreateTaskSession(context.Context, *models.TaskSession) error
|
||||
}, wsID, wfID, taskID, sessionID, currentStepID string) {
|
||||
t.Helper()
|
||||
ctx := context.Background()
|
||||
now := time.Now().UTC()
|
||||
require.NoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: wsID, Name: wsID, CreatedAt: now, UpdatedAt: now}))
|
||||
require.NoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ID: wfID, WorkspaceID: wsID, Name: wfID, CreatedAt: now, UpdatedAt: now}))
|
||||
require.NoError(t, repo.CreateTask(ctx, &models.Task{
|
||||
ID: taskID, WorkspaceID: wsID, WorkflowID: wfID, WorkflowStepID: currentStepID,
|
||||
Title: "task", State: v1.TaskStateInProgress, CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
require.NoError(t, repo.CreateTaskSession(ctx, &models.TaskSession{
|
||||
ID: sessionID, TaskID: taskID, State: models.TaskSessionStateRunning,
|
||||
IsPrimary: true, StartedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
}
|
||||
|
||||
// TestDeferMoveTask_RejectsNonExistentStep ensures deferMoveTask returns a
|
||||
// validation error when the requested workflow_step_id does not exist in the
|
||||
// workflow service, preventing a stale step ID from being stored as a pending move.
|
||||
func TestDeferMoveTask_RejectsNonExistentStep(t *testing.T) {
|
||||
svc, repo, wfCtrl, wfRepo := newTestTaskServiceWithWorkflow(t)
|
||||
ctx := context.Background()
|
||||
seedRunningTask(t, repo, "ws-defer1", "wf-defer1", "task-defer1", "sess-defer1", "src-step")
|
||||
|
||||
// Only insert the source step — target "ghost-step" does not exist.
|
||||
now := time.Now().UTC()
|
||||
require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{
|
||||
ID: "src-step", WorkflowID: "wf-defer1", Name: "Source", Position: 0, CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
|
||||
queue := &pendingMoveRecordingQueuer{}
|
||||
h := &Handlers{
|
||||
taskSvc: svc,
|
||||
workflowCtrl: wfCtrl,
|
||||
messageQueue: queue,
|
||||
logger: testLogger(t).WithFields(),
|
||||
}
|
||||
|
||||
msg := makeWSMessage(t, ws.ActionMCPMoveTask, map[string]interface{}{
|
||||
"task_id": "task-defer1",
|
||||
"workflow_id": "wf-defer1",
|
||||
"workflow_step_id": "ghost-step", // does not exist
|
||||
"position": 0,
|
||||
})
|
||||
|
||||
resp, err := h.handleMoveTask(ctx, msg)
|
||||
require.NoError(t, err)
|
||||
assertWSError(t, resp, ws.ErrorCodeValidation)
|
||||
assert.Empty(t, queue.pendingMoves, "pending move must not be recorded for a non-existent step")
|
||||
}
|
||||
|
||||
// TestDeferMoveTask_RejectsStepFromDifferentWorkflow ensures deferMoveTask
|
||||
// returns a validation error when the step exists but belongs to another workflow.
|
||||
func TestDeferMoveTask_RejectsStepFromDifferentWorkflow(t *testing.T) {
|
||||
svc, repo, wfCtrl, wfRepo := newTestTaskServiceWithWorkflow(t)
|
||||
ctx := context.Background()
|
||||
seedRunningTask(t, repo, "ws-defer2", "wf-defer2", "task-defer2", "sess-defer2", "src-step2")
|
||||
|
||||
now := time.Now().UTC()
|
||||
// source step (belongs to the task's workflow)
|
||||
require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{
|
||||
ID: "src-step2", WorkflowID: "wf-defer2", Name: "Source", Position: 0, CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
// A step that belongs to a different workflow — not wf-defer2
|
||||
require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{
|
||||
ID: "other-wf-step", WorkflowID: "wf-other", Name: "Other", Position: 0, CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
|
||||
queue := &pendingMoveRecordingQueuer{}
|
||||
h := &Handlers{
|
||||
taskSvc: svc,
|
||||
workflowCtrl: wfCtrl,
|
||||
messageQueue: queue,
|
||||
logger: testLogger(t).WithFields(),
|
||||
}
|
||||
|
||||
msg := makeWSMessage(t, ws.ActionMCPMoveTask, map[string]interface{}{
|
||||
"task_id": "task-defer2",
|
||||
"workflow_id": "wf-defer2",
|
||||
"workflow_step_id": "other-wf-step", // exists but in another workflow
|
||||
"position": 0,
|
||||
})
|
||||
|
||||
resp, err := h.handleMoveTask(ctx, msg)
|
||||
require.NoError(t, err)
|
||||
assertWSError(t, resp, ws.ErrorCodeValidation)
|
||||
assert.Empty(t, queue.pendingMoves, "pending move must not be recorded for a step from another workflow")
|
||||
}
|
||||
|
||||
// TestDeferMoveTask_RejectsWorkspaceMismatch ensures deferMoveTask returns a
|
||||
// validation error when the target step's workflow exists and matches
|
||||
// req.WorkflowID, but that workflow lives in a different workspace than the
|
||||
// task being moved. The deferred path bypasses task.Service.MoveTask (see
|
||||
// applyPendingMove's doc comment) so it must validate workspace ownership
|
||||
// independently, mirroring the immediate-apply path's validateTaskMove check.
|
||||
func TestDeferMoveTask_RejectsWorkspaceMismatch(t *testing.T) {
|
||||
svc, repo, wfCtrl, wfRepo := newTestTaskServiceWithWorkflow(t)
|
||||
ctx := context.Background()
|
||||
seedRunningTask(t, repo, "ws-defer4", "wf-defer4", "task-defer4", "sess-defer4", "src-step4")
|
||||
|
||||
now := time.Now().UTC()
|
||||
require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{
|
||||
ID: "src-step4", WorkflowID: "wf-defer4", Name: "Source", Position: 0, CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
|
||||
// A separate workspace with its own workflow/step — the step exists and
|
||||
// req.WorkflowID matches it, but its workspace differs from the task's.
|
||||
require.NoError(t, repo.CreateWorkspace(ctx, &models.Workspace{
|
||||
ID: "ws-defer4-other", Name: "Other", CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
require.NoError(t, repo.CreateWorkflow(ctx, &models.Workflow{
|
||||
ID: "wf-defer4-other", WorkspaceID: "ws-defer4-other", Name: "Other Board", CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{
|
||||
ID: "dst-step4-other", WorkflowID: "wf-defer4-other", Name: "Dest", Position: 0, CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
|
||||
queue := &pendingMoveRecordingQueuer{}
|
||||
h := &Handlers{
|
||||
taskSvc: svc,
|
||||
workflowCtrl: wfCtrl,
|
||||
messageQueue: queue,
|
||||
logger: testLogger(t).WithFields(),
|
||||
}
|
||||
|
||||
msg := makeWSMessage(t, ws.ActionMCPMoveTask, map[string]interface{}{
|
||||
"task_id": "task-defer4",
|
||||
"workflow_id": "wf-defer4-other",
|
||||
"workflow_step_id": "dst-step4-other",
|
||||
"position": 0,
|
||||
})
|
||||
|
||||
resp, err := h.handleMoveTask(ctx, msg)
|
||||
require.NoError(t, err)
|
||||
assertWSError(t, resp, ws.ErrorCodeValidation)
|
||||
assert.Empty(t, queue.pendingMoves, "pending move must not be recorded for a cross-workspace target")
|
||||
}
|
||||
|
||||
// TestDeferMoveTask_AcceptsValidStep ensures deferMoveTask succeeds when the
|
||||
// target step exists and belongs to the requested workflow.
|
||||
func TestDeferMoveTask_AcceptsValidStep(t *testing.T) {
|
||||
svc, repo, wfCtrl, wfRepo := newTestTaskServiceWithWorkflow(t)
|
||||
ctx := context.Background()
|
||||
seedRunningTask(t, repo, "ws-defer3", "wf-defer3", "task-defer3", "sess-defer3", "src-step3")
|
||||
|
||||
now := time.Now().UTC()
|
||||
require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{
|
||||
ID: "src-step3", WorkflowID: "wf-defer3", Name: "Source", Position: 0, CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{
|
||||
ID: "dst-step3", WorkflowID: "wf-defer3", Name: "Dest", Position: 1, CreatedAt: now, UpdatedAt: now,
|
||||
}))
|
||||
|
||||
queue := &pendingMoveRecordingQueuer{}
|
||||
h := &Handlers{
|
||||
taskSvc: svc,
|
||||
workflowCtrl: wfCtrl,
|
||||
messageQueue: queue,
|
||||
logger: testLogger(t).WithFields(),
|
||||
}
|
||||
|
||||
msg := makeWSMessage(t, ws.ActionMCPMoveTask, map[string]interface{}{
|
||||
"task_id": "task-defer3",
|
||||
"workflow_id": "wf-defer3",
|
||||
"workflow_step_id": "dst-step3",
|
||||
"position": 0,
|
||||
})
|
||||
|
||||
resp, err := h.handleMoveTask(ctx, msg)
|
||||
require.NoError(t, err)
|
||||
assert.NotEqual(t, ws.MessageTypeError, resp.Type, "valid deferred move must succeed")
|
||||
require.Len(t, queue.pendingMoves, 1)
|
||||
assert.Equal(t, "dst-step3", queue.pendingMoves[0].WorkflowStepID)
|
||||
}
|
||||
|
||||
func TestMoveTaskErrorMessage_SanitizesClassifiedErrors(t *testing.T) {
|
||||
assert.Equal(t,
|
||||
"Move task conflicts with the current task or workflow state",
|
||||
|
||||
@@ -89,6 +89,65 @@ func (h *Handlers) deferMoveTask(
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError,
|
||||
"move_task requires message queue support while the source session is active", nil)
|
||||
}
|
||||
|
||||
// Validate the target step exists and belongs to the requested workflow
|
||||
// before committing the deferred move. Without this check a stale or
|
||||
// foreign step_id would be stored and silently fail at turn-end, leaving
|
||||
// the task orphaned on the board.
|
||||
if h.workflowCtrl != nil {
|
||||
stepResp, err := h.workflowCtrl.GetStep(ctx, req.WorkflowStepID)
|
||||
if err != nil || stepResp == nil || stepResp.Step == nil {
|
||||
h.logger.Error("move_task: target step not found",
|
||||
zap.String("task_id", req.TaskID),
|
||||
zap.String("workflow_step_id", req.WorkflowStepID),
|
||||
zap.Error(err))
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation,
|
||||
"target workflow_step_id does not exist", nil)
|
||||
}
|
||||
if stepResp.Step.WorkflowID != req.WorkflowID {
|
||||
h.logger.Error("move_task: target step belongs to a different workflow",
|
||||
zap.String("task_id", req.TaskID),
|
||||
zap.String("workflow_step_id", req.WorkflowStepID),
|
||||
zap.String("step_workflow_id", stepResp.Step.WorkflowID),
|
||||
zap.String("requested_workflow_id", req.WorkflowID))
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation,
|
||||
"target workflow_step_id does not belong to the requested workflow_id", nil)
|
||||
}
|
||||
}
|
||||
|
||||
// Validate the target workflow lives in the same workspace as the task.
|
||||
// The immediate-apply path (task.Service.MoveTask) already enforces this
|
||||
// via validateTaskMove; the deferred path bypasses that service entirely
|
||||
// (see applyPendingMove's doc comment), so it must check independently or
|
||||
// a cross-workspace move_task_kandev call would silently succeed.
|
||||
if h.taskSvc != nil {
|
||||
task, err := h.taskSvc.GetTask(ctx, req.TaskID)
|
||||
if err != nil {
|
||||
h.logger.Error("move_task: failed to load task for workspace validation",
|
||||
zap.String("task_id", req.TaskID), zap.Error(err))
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError,
|
||||
"failed to load task for move validation", nil)
|
||||
}
|
||||
targetWorkflow, err := h.taskSvc.GetWorkflow(ctx, req.WorkflowID)
|
||||
if err != nil {
|
||||
h.logger.Error("move_task: target workflow not found",
|
||||
zap.String("task_id", req.TaskID),
|
||||
zap.String("workflow_id", req.WorkflowID),
|
||||
zap.Error(err))
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation,
|
||||
"target workflow_id does not exist", nil)
|
||||
}
|
||||
if targetWorkflow.WorkspaceID != task.WorkspaceID {
|
||||
h.logger.Error("move_task: target workflow is in a different workspace",
|
||||
zap.String("task_id", req.TaskID),
|
||||
zap.String("workflow_id", req.WorkflowID),
|
||||
zap.String("task_workspace_id", task.WorkspaceID),
|
||||
zap.String("target_workspace_id", targetWorkflow.WorkspaceID))
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation,
|
||||
"target workflow is in a different workspace", nil)
|
||||
}
|
||||
}
|
||||
|
||||
if req.Prompt != "" {
|
||||
wrapped := "You were moved to this step with the following message: " + req.Prompt
|
||||
if err := h.queueMoveTaskPrompt(ctx, req.TaskID, session.ID, wrapped); err != nil {
|
||||
|
||||
@@ -117,6 +117,50 @@ func TestPendingMove_OutOfTerminalStepReopensCompletedTask(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestPendingMove_DropsForeignWorkflowStepWithoutMovingTask(t *testing.T) {
|
||||
sc := buildPendingMoveScenario(t)
|
||||
sc.stepGetter.steps["foreign-step"] = &wfmodels.WorkflowStep{
|
||||
ID: "foreign-step",
|
||||
WorkflowID: "wf-other",
|
||||
Name: "Foreign",
|
||||
Position: 1,
|
||||
}
|
||||
|
||||
session, err := sc.repo.GetTaskSession(sc.ctx, sc.reviewSessionID)
|
||||
if err != nil {
|
||||
t.Fatalf("load review session: %v", err)
|
||||
}
|
||||
sc.svc.applyPendingMove(sc.ctx, "task-1", sc.reviewSessionID, session, &messagequeue.PendingMove{
|
||||
TaskID: "task-1",
|
||||
WorkflowID: "wf1",
|
||||
WorkflowStepID: "foreign-step",
|
||||
})
|
||||
|
||||
task, err := sc.repo.GetTask(sc.ctx, "task-1")
|
||||
if err != nil {
|
||||
t.Fatalf("load task: %v", err)
|
||||
}
|
||||
if task.WorkflowStepID != stepInReviewID {
|
||||
t.Fatalf("workflow_step_id = %q, want unchanged %q", task.WorkflowStepID, stepInReviewID)
|
||||
}
|
||||
session, err = sc.repo.GetTaskSession(sc.ctx, sc.reviewSessionID)
|
||||
if err != nil {
|
||||
t.Fatalf("reload review session: %v", err)
|
||||
}
|
||||
if session.State != models.TaskSessionStateRunning {
|
||||
t.Fatalf("session state = %q, want unchanged RUNNING", session.State)
|
||||
}
|
||||
|
||||
// Regression: the workflow-mismatch drop must clean up any hand-off prompt
|
||||
// queued by handleMoveTask before the deferred move was applied. Without
|
||||
// this cleanup, the queued prompt (authored for the foreign-workflow
|
||||
// target step) would still be sitting in the queue and could be
|
||||
// misdelivered to the review session's agent on a future turn.
|
||||
if status := sc.svc.messageQueue.GetStatus(sc.ctx, sc.reviewSessionID); status.Count != 0 {
|
||||
t.Fatalf("queued message count = %d, want 0 after workflow-mismatch drop", status.Count)
|
||||
}
|
||||
}
|
||||
|
||||
// --- Pending-move scenario builder & assertions ---
|
||||
|
||||
const (
|
||||
|
||||
@@ -1067,6 +1067,29 @@ func (s *Service) applyPendingMove(ctx context.Context, taskID, sessionID string
|
||||
reinsertPendingMove()
|
||||
return
|
||||
}
|
||||
if targetStep.WorkflowID != move.WorkflowID {
|
||||
s.logger.Error("pending move target step belongs to a different workflow; dropping move",
|
||||
zap.String("task_id", taskID),
|
||||
zap.String("target_step_id", move.WorkflowStepID),
|
||||
zap.String("step_workflow_id", targetStep.WorkflowID),
|
||||
zap.String("move_workflow_id", move.WorkflowID))
|
||||
// Do NOT reinsert: the move is invalid and retrying would keep failing.
|
||||
// The hand-off prompt was queued by handleMoveTask before the move was
|
||||
// applied. Since the move is being dropped, the on_enter path that would
|
||||
// have drained the queue won't run. Drop the orphan so it can't be
|
||||
// misdelivered to the source step's agent on a future turn (it was
|
||||
// authored for the move's *target* step) — mirrors the cleanup done on
|
||||
// the ApplyTransition failure path below.
|
||||
if s.messageQueue != nil {
|
||||
if _, ok := s.messageQueue.TakeQueued(ctx, sessionID); ok {
|
||||
s.publishQueueStatusEvent(ctx, sessionID)
|
||||
s.logger.Warn("dropped hand-off prompt after pending-move workflow mismatch",
|
||||
zap.String("task_id", taskID),
|
||||
zap.String("session_id", sessionID))
|
||||
}
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
// Mark the session WAITING_FOR_INPUT before processOnEnter runs. The agent
|
||||
// just finished its turn; the active-turn guard in processOnEnter would
|
||||
|
||||
@@ -109,7 +109,7 @@ type TurnService interface {
|
||||
// metadata, parent_id). The orchestrator does not construct task.updated
|
||||
// payloads itself.
|
||||
type TaskEventPublisher interface {
|
||||
PublishTaskUpdated(ctx context.Context, task *models.Task)
|
||||
PublishTaskUpdated(ctx context.Context, task *models.Task, oldWorkflowIDs ...string)
|
||||
PublishTaskStateChanged(ctx context.Context, task *models.Task, oldState v1.TaskState)
|
||||
}
|
||||
|
||||
@@ -690,12 +690,14 @@ func (s *Service) SetTaskEventPublisher(publisher TaskEventPublisher) {
|
||||
|
||||
// publishTaskUpdated forwards to the configured TaskEventPublisher.
|
||||
// No-op when the publisher isn't wired (tests, or before SetTaskEventPublisher
|
||||
// has been called during startup).
|
||||
func (s *Service) publishTaskUpdated(ctx context.Context, task *models.Task) {
|
||||
// has been called during startup). Pass the pre-move workflow ID when the
|
||||
// task's workflow changed (e.g. a cross-workflow transition) so the event
|
||||
// carries old_workflow_id.
|
||||
func (s *Service) publishTaskUpdated(ctx context.Context, task *models.Task, oldWorkflowIDs ...string) {
|
||||
if s.taskEvents == nil || task == nil {
|
||||
return
|
||||
}
|
||||
s.taskEvents.PublishTaskUpdated(ctx, task)
|
||||
s.taskEvents.PublishTaskUpdated(ctx, task, oldWorkflowIDs...)
|
||||
}
|
||||
|
||||
func (s *Service) publishTaskStateChanged(ctx context.Context, task *models.Task, oldState v1.TaskState) {
|
||||
|
||||
@@ -19,7 +19,7 @@ import (
|
||||
// taskUpdatedPublisher is the minimal hook the workflow store needs to emit
|
||||
// task.updated events. The orchestrator Service binds this to its shared
|
||||
// publishTaskUpdated helper so the publisher wiring stays in one place.
|
||||
type taskUpdatedPublisher func(ctx context.Context, task *models.Task)
|
||||
type taskUpdatedPublisher func(ctx context.Context, task *models.Task, oldWorkflowIDs ...string)
|
||||
|
||||
type taskMovedPublisher func(ctx context.Context, task *models.Task, fromWorkflowID, fromStepID, toStepID, sessionID string)
|
||||
|
||||
@@ -131,13 +131,26 @@ func (s *workflowStore) ApplyTransition(ctx context.Context, taskID, sessionID,
|
||||
return err
|
||||
}
|
||||
|
||||
// Keep WorkflowID in sync with the target step's owning workflow. Most
|
||||
// callers transition within the same workflow (targetStep.WorkflowID ==
|
||||
// task.WorkflowID already), but applyPendingMove uses this path for
|
||||
// cross-workflow move_task_kandev hand-offs too — without this, the task
|
||||
// would end up with a step ID from a workflow its WorkflowID doesn't match.
|
||||
oldWorkflowID := task.WorkflowID
|
||||
if targetStep != nil {
|
||||
task.WorkflowID = targetStep.WorkflowID
|
||||
}
|
||||
task.WorkflowStepID = toStepID
|
||||
task.UpdatedAt = time.Now().UTC()
|
||||
if err := s.updateTransitionTask(ctx, task, targetStep); err != nil {
|
||||
return fmt.Errorf("update task workflow step: %w", err)
|
||||
}
|
||||
|
||||
s.publishTaskUpdated(ctx, task)
|
||||
// Pass the pre-move workflow ID through so cross-workflow transitions
|
||||
// carry old_workflow_id on the task.updated payload — the frontend uses
|
||||
// that field to remove the task from its previous workflow's snapshot
|
||||
// instead of leaving a stale duplicate until reload.
|
||||
s.publishTaskUpdated(ctx, task, oldWorkflowID)
|
||||
|
||||
if err := s.repo.UpdateSessionReviewStatus(ctx, sessionID, ""); err != nil {
|
||||
s.logger.Warn("failed to clear session review status",
|
||||
|
||||
@@ -11,7 +11,23 @@ import (
|
||||
|
||||
// noopPublisher satisfies the taskUpdatedPublisher contract without touching
|
||||
// an event bus. Workflow-store unit tests don't exercise the event path.
|
||||
func noopPublisher(_ context.Context, _ *models.Task) {}
|
||||
func noopPublisher(_ context.Context, _ *models.Task, _ ...string) {}
|
||||
|
||||
// capturingPublisher records each publishTaskUpdated call's task and
|
||||
// old-workflow-ID argument so tests can assert on what ApplyTransition
|
||||
// passed through.
|
||||
type capturingPublisher struct {
|
||||
calls []capturedTaskUpdate
|
||||
}
|
||||
|
||||
type capturedTaskUpdate struct {
|
||||
task *models.Task
|
||||
oldWorkflowIDs []string
|
||||
}
|
||||
|
||||
func (c *capturingPublisher) publish(_ context.Context, task *models.Task, oldWorkflowIDs ...string) {
|
||||
c.calls = append(c.calls, capturedTaskUpdate{task: task, oldWorkflowIDs: oldWorkflowIDs})
|
||||
}
|
||||
|
||||
func TestWorkflowStore_LoadState(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
@@ -143,6 +159,86 @@ func TestWorkflowStore_ApplyTransition(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestWorkflowStore_ApplyTransitionSyncsWorkflowIDAcrossWorkflows(t *testing.T) {
|
||||
// Regression test: applyPendingMove (deferred cross-workflow move_task_kandev
|
||||
// hand-off for tasks with an active/starting session) calls ApplyTransition
|
||||
// directly, bypassing task.Service.MoveTask. Without syncing WorkflowID here,
|
||||
// a cross-workflow move would leave the task pointing at a step ID that
|
||||
// belongs to a different workflow than task.WorkflowID records.
|
||||
ctx := context.Background()
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step1") // seedSession puts the task in wf1
|
||||
|
||||
// Create a second workflow the task is moving into.
|
||||
if err := repo.CreateWorkflow(ctx, &models.Workflow{
|
||||
ID: "wf2", WorkspaceID: "ws1", Name: "Other Workflow",
|
||||
}); err != nil {
|
||||
t.Fatalf("CreateWorkflow wf2: %v", err)
|
||||
}
|
||||
|
||||
stepGetter := newMockStepGetter()
|
||||
stepGetter.steps["step-wf2"] = &wfmodels.WorkflowStep{
|
||||
ID: "step-wf2", WorkflowID: "wf2", Name: "Target", Position: 0,
|
||||
}
|
||||
store := newWorkflowStore(repo, stepGetter, nil, noopPublisher, testLogger())
|
||||
|
||||
if err := store.ApplyTransition(ctx, "t1", "s1", "step1", "step-wf2", "manual_move"); err != nil {
|
||||
t.Fatalf("ApplyTransition: %v", err)
|
||||
}
|
||||
|
||||
task, err := repo.GetTask(ctx, "t1")
|
||||
if err != nil {
|
||||
t.Fatalf("failed to get task: %v", err)
|
||||
}
|
||||
if task.WorkflowStepID != "step-wf2" {
|
||||
t.Errorf("expected task WorkflowStepID %q, got %q", "step-wf2", task.WorkflowStepID)
|
||||
}
|
||||
if task.WorkflowID != "wf2" {
|
||||
t.Errorf("expected task WorkflowID to sync to the target step's workflow %q, got %q",
|
||||
"wf2", task.WorkflowID)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWorkflowStore_ApplyTransitionPublishesOldWorkflowIDOnCrossWorkflowMove(t *testing.T) {
|
||||
// Regression test for PR review feedback: ApplyTransition syncs
|
||||
// task.WorkflowID to the target step's workflow, but the task.updated
|
||||
// event must also carry the pre-move workflow ID (old_workflow_id) —
|
||||
// same as the normal task.Service.MoveTask path — so the frontend can
|
||||
// remove the task from its previous workflow's snapshot instead of
|
||||
// leaving a stale duplicate until reload.
|
||||
ctx := context.Background()
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step1") // seedSession puts the task in wf1
|
||||
|
||||
if err := repo.CreateWorkflow(ctx, &models.Workflow{
|
||||
ID: "wf2", WorkspaceID: "ws1", Name: "Other Workflow",
|
||||
}); err != nil {
|
||||
t.Fatalf("CreateWorkflow wf2: %v", err)
|
||||
}
|
||||
|
||||
stepGetter := newMockStepGetter()
|
||||
stepGetter.steps["step-wf2"] = &wfmodels.WorkflowStep{
|
||||
ID: "step-wf2", WorkflowID: "wf2", Name: "Target", Position: 0,
|
||||
}
|
||||
pub := &capturingPublisher{}
|
||||
store := newWorkflowStore(repo, stepGetter, nil, pub.publish, testLogger())
|
||||
|
||||
if err := store.ApplyTransition(ctx, "t1", "s1", "step1", "step-wf2", "manual_move"); err != nil {
|
||||
t.Fatalf("ApplyTransition: %v", err)
|
||||
}
|
||||
|
||||
if len(pub.calls) != 1 {
|
||||
t.Fatalf("expected exactly 1 publishTaskUpdated call, got %d", len(pub.calls))
|
||||
}
|
||||
call := pub.calls[0]
|
||||
if call.task.WorkflowID != "wf2" {
|
||||
t.Errorf("expected published task WorkflowID %q, got %q", "wf2", call.task.WorkflowID)
|
||||
}
|
||||
if len(call.oldWorkflowIDs) != 1 || call.oldWorkflowIDs[0] != "wf1" {
|
||||
t.Errorf("expected old_workflow_id %q, got %v", "wf1", call.oldWorkflowIDs)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWorkflowStore_ApplyTransitionRejectsFullWIPLimitedTarget(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
repo := setupTestRepo(t)
|
||||
|
||||
@@ -981,3 +981,13 @@ func (r *Repository) backfillTaskEnvironmentRepos() error {
|
||||
}
|
||||
return tx.Commit()
|
||||
}
|
||||
|
||||
// Startup healing of orphaned workflow_step_id values was removed: a raw SQL
|
||||
// UPDATE reassigning tasks to a workflow's start step bypasses every
|
||||
// domain-level invariant the task service enforces on a move (WIP limits,
|
||||
// task-state sync, position bookkeeping, session/on_exit/on_enter handling,
|
||||
// transition history, and event publication). The Kanban/Pipeline "Needs
|
||||
// Reassignment" fallback column now keeps orphaned tasks visible without any
|
||||
// automatic mutation, and any real repair should go through
|
||||
// task.Service.MoveTask (or a dedicated, explicit reassignment operation)
|
||||
// rather than a migration-time SQL statement.
|
||||
|
||||
@@ -618,7 +618,7 @@ type fakeEventPublisher struct {
|
||||
archived []bool // archivedAt nil/non-nil per updated entry
|
||||
}
|
||||
|
||||
func (f *fakeEventPublisher) PublishTaskUpdated(_ context.Context, task *models.Task) {
|
||||
func (f *fakeEventPublisher) PublishTaskUpdated(_ context.Context, task *models.Task, _ ...string) {
|
||||
f.mu.Lock()
|
||||
defer f.mu.Unlock()
|
||||
f.updated = append(f.updated, task.ID)
|
||||
|
||||
@@ -198,7 +198,7 @@ type HandoffService struct {
|
||||
// Optional — when nil the cascade still completes, it just won't emit
|
||||
// WS events (matches the pre-handoff fallback in the HTTP handler).
|
||||
type TaskEventPublisher interface {
|
||||
PublishTaskUpdated(ctx context.Context, task *models.Task)
|
||||
PublishTaskUpdated(ctx context.Context, task *models.Task, oldWorkflowIDs ...string)
|
||||
PublishTaskDeleted(ctx context.Context, task *models.Task)
|
||||
}
|
||||
|
||||
|
||||
@@ -16,9 +16,13 @@ import (
|
||||
|
||||
// PublishTaskUpdated publishes a task.updated event for the given task.
|
||||
// Used when task metadata changes (e.g., primary session assignment) that
|
||||
// don't go through the normal UpdateTask path.
|
||||
func (s *Service) PublishTaskUpdated(ctx context.Context, task *models.Task) {
|
||||
s.publishTaskEvent(ctx, events.TaskUpdated, task, nil)
|
||||
// don't go through the normal UpdateTask path. Callers that changed the
|
||||
// task's workflow (e.g. a cross-workflow transition) should pass the
|
||||
// pre-move workflow ID so the payload carries old_workflow_id — the
|
||||
// frontend uses that field to remove the task from its previous
|
||||
// workflow's snapshot instead of leaving a stale duplicate until reload.
|
||||
func (s *Service) PublishTaskUpdated(ctx context.Context, task *models.Task, oldWorkflowIDs ...string) {
|
||||
s.publishTaskEvent(ctx, events.TaskUpdated, task, nil, oldWorkflowIDs...)
|
||||
}
|
||||
|
||||
// PublishTaskStateChanged publishes a task.state_changed event for callers
|
||||
|
||||
@@ -12,6 +12,8 @@ import (
|
||||
"gopkg.in/yaml.v3"
|
||||
|
||||
"github.com/kandev/kandev/internal/common/logger"
|
||||
"github.com/kandev/kandev/internal/events"
|
||||
"github.com/kandev/kandev/internal/events/bus"
|
||||
"github.com/kandev/kandev/internal/workflow/controller"
|
||||
"github.com/kandev/kandev/internal/workflow/models"
|
||||
"github.com/kandev/kandev/internal/workflow/service"
|
||||
@@ -21,20 +23,22 @@ import (
|
||||
// Handlers manages workflow HTTP and WebSocket handlers
|
||||
type Handlers struct {
|
||||
controller *controller.Controller
|
||||
eventBus bus.EventBus
|
||||
logger *logger.Logger
|
||||
}
|
||||
|
||||
// NewHandlers creates new workflow handlers
|
||||
func NewHandlers(ctrl *controller.Controller, log *logger.Logger) *Handlers {
|
||||
func NewHandlers(ctrl *controller.Controller, eventBus bus.EventBus, log *logger.Logger) *Handlers {
|
||||
return &Handlers{
|
||||
controller: ctrl,
|
||||
eventBus: eventBus,
|
||||
logger: log.WithFields(zap.String("component", "workflow-handlers")),
|
||||
}
|
||||
}
|
||||
|
||||
// RegisterRoutes registers workflow HTTP and WebSocket handlers
|
||||
func RegisterRoutes(router *gin.Engine, dispatcher *ws.Dispatcher, ctrl *controller.Controller, log *logger.Logger) {
|
||||
handlers := NewHandlers(ctrl, log)
|
||||
func RegisterRoutes(router *gin.Engine, dispatcher *ws.Dispatcher, ctrl *controller.Controller, eventBus bus.EventBus, log *logger.Logger) {
|
||||
handlers := NewHandlers(ctrl, eventBus, log)
|
||||
handlers.registerHTTP(router)
|
||||
handlers.registerWS(dispatcher)
|
||||
}
|
||||
@@ -210,14 +214,59 @@ func isStepValidationError(msg string) bool {
|
||||
}
|
||||
|
||||
func (h *Handlers) httpDeleteStep(c *gin.Context) {
|
||||
if err := h.controller.DeleteStep(c.Request.Context(), c.Param("id")); err != nil {
|
||||
ctx := c.Request.Context()
|
||||
stepID := c.Param("id")
|
||||
stepResp, getErr := h.controller.GetStep(ctx, stepID)
|
||||
if getErr != nil {
|
||||
h.logger.Warn("failed to fetch step before delete; workflow_step.deleted event will not be published",
|
||||
zap.String("step_id", stepID), zap.Error(getErr))
|
||||
}
|
||||
|
||||
if err := h.controller.DeleteStep(ctx, stepID); err != nil {
|
||||
h.logger.Error("failed to delete step", zap.Error(err))
|
||||
h.writeStepMutationError(c, err)
|
||||
return
|
||||
}
|
||||
if stepResp != nil {
|
||||
h.publishWorkflowStepEvent(ctx, events.WorkflowStepDeleted, stepResp.Step)
|
||||
}
|
||||
c.JSON(http.StatusOK, gin.H{"success": true})
|
||||
}
|
||||
|
||||
func (h *Handlers) publishWorkflowStepEvent(ctx context.Context, eventType string, step *models.WorkflowStep) {
|
||||
if step == nil {
|
||||
return
|
||||
}
|
||||
data := map[string]interface{}{
|
||||
"step": map[string]interface{}{
|
||||
"id": step.ID,
|
||||
"workflow_id": step.WorkflowID,
|
||||
"name": step.Name,
|
||||
"position": step.Position,
|
||||
"color": step.Color,
|
||||
"prompt": step.Prompt,
|
||||
"events": step.Events,
|
||||
"show_in_command_panel": step.ShowInCommandPanel,
|
||||
"allow_manual_move": step.AllowManualMove,
|
||||
"is_start_step": step.IsStartStep,
|
||||
"auto_archive_after_hours": step.AutoArchiveAfterHours,
|
||||
"wip_limit": step.WIPLimit,
|
||||
"pull_from_step_id": step.PullFromStepID,
|
||||
"agent_profile_id": step.AgentProfileID,
|
||||
"stage_type": string(step.StageType),
|
||||
"auto_advance_requires_signal": step.AutoAdvanceRequiresSignal,
|
||||
"created_at": step.CreatedAt,
|
||||
"updated_at": step.UpdatedAt,
|
||||
},
|
||||
}
|
||||
if err := h.eventBus.Publish(ctx, eventType, bus.NewEvent(eventType, "workflow-handlers", data)); err != nil {
|
||||
h.logger.Error("failed to publish workflow step event",
|
||||
zap.String("event_type", eventType),
|
||||
zap.String("step_id", step.ID),
|
||||
zap.Error(err))
|
||||
}
|
||||
}
|
||||
|
||||
type httpReorderStepsRequest struct {
|
||||
StepIDs []string `json:"step_ids"`
|
||||
}
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
package handlers
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/kandev/kandev/internal/common/logger"
|
||||
"github.com/kandev/kandev/internal/events"
|
||||
"github.com/kandev/kandev/internal/events/bus"
|
||||
"github.com/kandev/kandev/internal/workflow/models"
|
||||
)
|
||||
|
||||
type recordingWorkflowEventBus struct {
|
||||
subject string
|
||||
event *bus.Event
|
||||
}
|
||||
|
||||
func (b *recordingWorkflowEventBus) Publish(_ context.Context, subject string, event *bus.Event) error {
|
||||
b.subject = subject
|
||||
b.event = event
|
||||
return nil
|
||||
}
|
||||
|
||||
func (b *recordingWorkflowEventBus) Subscribe(string, bus.EventHandler) (bus.Subscription, error) {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func (b *recordingWorkflowEventBus) QueueSubscribe(string, string, bus.EventHandler) (bus.Subscription, error) {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func (b *recordingWorkflowEventBus) Request(context.Context, string, *bus.Event, time.Duration) (*bus.Event, error) {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func (b *recordingWorkflowEventBus) Close() {}
|
||||
|
||||
func (b *recordingWorkflowEventBus) IsConnected() bool { return true }
|
||||
|
||||
func TestPublishWorkflowStepEventPublishesDeletedStepPayload(t *testing.T) {
|
||||
eventBus := &recordingWorkflowEventBus{}
|
||||
h := NewHandlers(nil, eventBus, logger.Default())
|
||||
|
||||
h.publishWorkflowStepEvent(context.Background(), events.WorkflowStepDeleted, &models.WorkflowStep{
|
||||
ID: "step-deleted",
|
||||
WorkflowID: "workflow-1",
|
||||
Name: "Deleted",
|
||||
Position: 2,
|
||||
})
|
||||
|
||||
if eventBus.subject != events.WorkflowStepDeleted {
|
||||
t.Fatalf("subject = %q, want %q", eventBus.subject, events.WorkflowStepDeleted)
|
||||
}
|
||||
if eventBus.event == nil {
|
||||
t.Fatal("expected event to be published")
|
||||
}
|
||||
if eventBus.event.Source != "workflow-handlers" {
|
||||
t.Fatalf("source = %q, want workflow-handlers", eventBus.event.Source)
|
||||
}
|
||||
data, ok := eventBus.event.Data.(map[string]interface{})
|
||||
if !ok {
|
||||
t.Fatalf("event data type = %T, want map", eventBus.event.Data)
|
||||
}
|
||||
step, ok := data["step"].(map[string]interface{})
|
||||
if !ok {
|
||||
t.Fatalf("step data type = %T, want map", data["step"])
|
||||
}
|
||||
if step["id"] != "step-deleted" || step["workflow_id"] != "workflow-1" {
|
||||
t.Fatalf("unexpected step payload: %#v", step)
|
||||
}
|
||||
}
|
||||
@@ -311,6 +311,13 @@ func (s *Service) UpdateStepWithStartStepUpdates(ctx context.Context, step *mode
|
||||
}
|
||||
|
||||
// DeleteStep deletes a workflow step and clears any references to it from other steps.
|
||||
// Tasks currently on the step are NOT auto-reassigned: doing so through the normal
|
||||
// move path would run on_exit/on_enter (auto-start agents, session hand-offs) as a
|
||||
// side effect of an administrative delete, and any failed move would be silently
|
||||
// ignored while the step is deleted anyway (a partial, non-atomic cascade). Instead,
|
||||
// affected tasks keep their now-dangling workflow_step_id and are surfaced via the
|
||||
// Kanban/Pipeline "Needs Reassignment" fallback column for explicit manual
|
||||
// reassignment by a user (which goes through the normal, fully-validated move path).
|
||||
func (s *Service) DeleteStep(ctx context.Context, stepID string) error {
|
||||
// First, get the step to find its workflow ID
|
||||
step, err := s.repo.GetStep(ctx, stepID)
|
||||
|
||||
@@ -881,3 +881,99 @@ func TestImportWorkflows(t *testing.T) {
|
||||
assert.Empty(t, steps[0].AgentProfileID, "no matcher means no profile ID set")
|
||||
})
|
||||
}
|
||||
|
||||
// --- DeleteStep tests ---
|
||||
//
|
||||
// DeleteStep intentionally does NOT auto-reassign tasks off the deleted step: doing
|
||||
// so through the normal move path would trigger on_exit/on_enter side effects
|
||||
// (auto-start agents, session hand-offs) as a byproduct of an administrative delete,
|
||||
// and any failed move would silently leave the step deleted anyway (a partial,
|
||||
// non-atomic cascade). Affected tasks keep their dangling workflow_step_id and are
|
||||
// surfaced via the Kanban/Pipeline "Needs Reassignment" fallback for explicit manual
|
||||
// reassignment through the normal, fully-validated move path.
|
||||
|
||||
func TestDeleteStep_RemovesStepWithoutReassigningTasks(t *testing.T) {
|
||||
svc, db := setupTestService(t)
|
||||
ctx := context.Background()
|
||||
insertWorkflow(t, db, "wf-1", "Test")
|
||||
|
||||
createStep(t, svc, &models.WorkflowStep{ID: "start", WorkflowID: "wf-1", Name: "Start", Position: 0, IsStartStep: true})
|
||||
createStep(t, svc, &models.WorkflowStep{ID: "mid", WorkflowID: "wf-1", Name: "Mid", Position: 1})
|
||||
createStep(t, svc, &models.WorkflowStep{ID: "end", WorkflowID: "wf-1", Name: "End", Position: 2})
|
||||
|
||||
require.NoError(t, svc.DeleteStep(ctx, "mid"))
|
||||
|
||||
// Deleted step must not exist anymore.
|
||||
steps, err := svc.repo.ListStepsByWorkflow(ctx, "wf-1")
|
||||
require.NoError(t, err)
|
||||
require.Len(t, steps, 2)
|
||||
for _, s := range steps {
|
||||
assert.NotEqual(t, "mid", s.ID, "deleted step must be removed")
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeleteStep_SucceedsWhenDeletingOnlyStep(t *testing.T) {
|
||||
svc, db := setupTestService(t)
|
||||
ctx := context.Background()
|
||||
insertWorkflow(t, db, "wf-3", "Test")
|
||||
|
||||
createStep(t, svc, &models.WorkflowStep{ID: "only", WorkflowID: "wf-3", Name: "Only", Position: 0})
|
||||
|
||||
require.NoError(t, svc.DeleteStep(ctx, "only"))
|
||||
|
||||
steps, err := svc.repo.ListStepsByWorkflow(ctx, "wf-3")
|
||||
require.NoError(t, err)
|
||||
assert.Empty(t, steps)
|
||||
}
|
||||
|
||||
// TestDeleteStep_WIPLimitedAutoStartTargetIsUnaffected is a regression test
|
||||
// requested during maintainer review: deleting a step must not run tasks
|
||||
// through the normal move path (which could trip a neighboring step's WIP
|
||||
// limit or trigger its auto-start-agent on_enter action as a side effect of
|
||||
// an administrative delete). Since DeleteStep no longer reassigns tasks at
|
||||
// all, this asserts the delete succeeds cleanly and is fully independent of
|
||||
// what limits/automations are configured on the surviving steps.
|
||||
func TestDeleteStep_WIPLimitedAutoStartTargetIsUnaffected(t *testing.T) {
|
||||
svc, db := setupTestService(t)
|
||||
ctx := context.Background()
|
||||
insertWorkflow(t, db, "wf-5", "Test")
|
||||
|
||||
createStep(t, svc, &models.WorkflowStep{ID: "start", WorkflowID: "wf-5", Name: "Start", Position: 0, IsStartStep: true})
|
||||
createStep(t, svc, &models.WorkflowStep{ID: "mid", WorkflowID: "wf-5", Name: "Mid", Position: 1})
|
||||
createStep(t, svc, &models.WorkflowStep{
|
||||
ID: "auto-start-limited", WorkflowID: "wf-5", Name: "Auto Start", Position: 2,
|
||||
WIPLimit: 1,
|
||||
Events: models.StepEvents{
|
||||
OnEnter: []models.OnEnterAction{{Type: models.OnEnterAutoStartAgent}},
|
||||
},
|
||||
})
|
||||
|
||||
require.NoError(t, svc.DeleteStep(ctx, "mid"))
|
||||
|
||||
steps, err := svc.repo.ListStepsByWorkflow(ctx, "wf-5")
|
||||
require.NoError(t, err)
|
||||
require.Len(t, steps, 2)
|
||||
for _, s := range steps {
|
||||
assert.NotEqual(t, "mid", s.ID, "deleted step must be removed")
|
||||
if s.ID == "auto-start-limited" {
|
||||
// The WIP-limited/auto-start step itself must remain untouched.
|
||||
assert.Equal(t, 1, s.WIPLimit)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeleteStep_ClearsStepReferences(t *testing.T) {
|
||||
svc, db := setupTestService(t)
|
||||
ctx := context.Background()
|
||||
insertWorkflow(t, db, "wf-4", "Test")
|
||||
|
||||
createStep(t, svc, &models.WorkflowStep{ID: "s1", WorkflowID: "wf-4", Name: "S1", Position: 0})
|
||||
createStep(t, svc, &models.WorkflowStep{ID: "s2", WorkflowID: "wf-4", Name: "S2", Position: 1})
|
||||
|
||||
require.NoError(t, svc.DeleteStep(ctx, "s1"))
|
||||
|
||||
steps, err := svc.repo.ListStepsByWorkflow(ctx, "wf-4")
|
||||
require.NoError(t, err)
|
||||
require.Len(t, steps, 1)
|
||||
assert.Equal(t, "s2", steps[0].ID)
|
||||
}
|
||||
|
||||
@@ -123,6 +123,7 @@ function MobileLayout({
|
||||
/>
|
||||
<SwipeableColumns
|
||||
steps={steps}
|
||||
moveTargetSteps={steps}
|
||||
tasks={tasks}
|
||||
activeIndex={activeColumnIndex}
|
||||
onIndexChange={setActiveColumnIndex}
|
||||
|
||||
@@ -16,6 +16,7 @@ import { needsAction } from "@/lib/utils/needs-action";
|
||||
import { useAppStore } from "@/components/state-provider";
|
||||
import { Graph2StepNode } from "./graph2-step-node";
|
||||
import { Graph2Connector } from "./graph2-connector";
|
||||
import { isOrphanMoveTarget } from "./swimlane-kanban-content";
|
||||
import type { Task } from "@/components/kanban-card";
|
||||
import type { WorkflowStep } from "@/components/kanban-column";
|
||||
|
||||
@@ -68,6 +69,31 @@ function getConnectorType(
|
||||
return "transition";
|
||||
}
|
||||
|
||||
export type StepAdjacency = {
|
||||
hasPrev: boolean;
|
||||
prevStepId?: string;
|
||||
hasNext: boolean;
|
||||
nextStepId?: string;
|
||||
};
|
||||
|
||||
/**
|
||||
* Computes the prev/next move targets for the node at `index`. The synthetic
|
||||
* "Needs Reassignment" node is display-only: it marks where an orphaned task
|
||||
* currently sits, but is never itself a valid move destination (there is no
|
||||
* backing workflow step to move into), so it is excluded as an adjacency
|
||||
* target in either direction.
|
||||
*/
|
||||
export function getStepAdjacency(steps: WorkflowStep[], index: number): StepAdjacency {
|
||||
const hasPrev = index > 0 && !isOrphanMoveTarget(steps[index - 1].id);
|
||||
const hasNext = index < steps.length - 1 && !isOrphanMoveTarget(steps[index + 1].id);
|
||||
return {
|
||||
hasPrev,
|
||||
prevStepId: hasPrev ? steps[index - 1].id : undefined,
|
||||
hasNext,
|
||||
nextStepId: hasNext ? steps[index + 1].id : undefined,
|
||||
};
|
||||
}
|
||||
|
||||
function PipelineStepNodes({
|
||||
steps,
|
||||
currentStepIndex,
|
||||
@@ -92,16 +118,18 @@ function PipelineStepNodes({
|
||||
? getConnectorType(phase, getStepPhase(index + 1, currentStepIndex))
|
||||
: null;
|
||||
|
||||
const { hasPrev, prevStepId, hasNext, nextStepId } = getStepAdjacency(steps, index);
|
||||
|
||||
return (
|
||||
<div key={step.id} className="flex items-center">
|
||||
<Graph2StepNode
|
||||
step={step}
|
||||
phase={phase}
|
||||
task={task}
|
||||
hasPrev={index > 0}
|
||||
hasNext={index < steps.length - 1}
|
||||
prevStepId={index > 0 ? steps[index - 1].id : undefined}
|
||||
nextStepId={index < steps.length - 1 ? steps[index + 1].id : undefined}
|
||||
hasPrev={hasPrev}
|
||||
hasNext={hasNext}
|
||||
prevStepId={prevStepId}
|
||||
nextStepId={nextStepId}
|
||||
onMoveTask={onMoveTask}
|
||||
onPreviewTask={onPreviewTask}
|
||||
isMoving={isMoving}
|
||||
|
||||
@@ -0,0 +1,55 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { ORPHAN_STEP_ID, isOrphanMoveTarget } from "./swimlane-kanban-content";
|
||||
import { getStepAdjacency } from "./graph2-task-pipeline";
|
||||
import type { WorkflowStep } from "@/components/kanban-column";
|
||||
|
||||
describe("isOrphanMoveTarget", () => {
|
||||
it("identifies the synthetic Needs Reassignment sentinel", () => {
|
||||
expect(isOrphanMoveTarget(ORPHAN_STEP_ID)).toBe(true);
|
||||
});
|
||||
|
||||
it("treats real step ids as valid move targets", () => {
|
||||
expect(isOrphanMoveTarget("todo")).toBe(false);
|
||||
expect(isOrphanMoveTarget("")).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("getStepAdjacency", () => {
|
||||
const realSteps: WorkflowStep[] = [
|
||||
{ id: "todo", title: "Todo", color: "#64748b" },
|
||||
{ id: "in-progress", title: "In Progress", color: "#3b82f6" },
|
||||
{ id: "done", title: "Done", color: "#22c55e" },
|
||||
];
|
||||
const stepsWithOrphan: WorkflowStep[] = [
|
||||
...realSteps,
|
||||
{ id: ORPHAN_STEP_ID, title: "Needs Reassignment", color: "#f59e0b" },
|
||||
];
|
||||
|
||||
it("offers both neighbors for a middle real step with no orphan present", () => {
|
||||
const adjacency = getStepAdjacency(realSteps, 1);
|
||||
expect(adjacency).toEqual({
|
||||
hasPrev: true,
|
||||
prevStepId: "todo",
|
||||
hasNext: true,
|
||||
nextStepId: "done",
|
||||
});
|
||||
});
|
||||
|
||||
it("does not offer the orphan sentinel as the next move target", () => {
|
||||
// "done" (index 2) is immediately followed by the orphan node (index 3).
|
||||
const adjacency = getStepAdjacency(stepsWithOrphan, 2);
|
||||
expect(adjacency.hasNext).toBe(false);
|
||||
expect(adjacency.nextStepId).toBeUndefined();
|
||||
// The real, backward neighbor is still a valid target.
|
||||
expect(adjacency.hasPrev).toBe(true);
|
||||
expect(adjacency.prevStepId).toBe("in-progress");
|
||||
});
|
||||
|
||||
it("still allows an orphaned task to move back onto the last real step", () => {
|
||||
// The orphan node itself (index 3) — its prev neighbor is a real step.
|
||||
const adjacency = getStepAdjacency(stepsWithOrphan, 3);
|
||||
expect(adjacency.hasPrev).toBe(true);
|
||||
expect(adjacency.prevStepId).toBe("done");
|
||||
expect(adjacency.hasNext).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -3,7 +3,22 @@
|
||||
import { useMemo, useState } from "react";
|
||||
import { useSwimlaneMove } from "@/hooks/domains/kanban/use-swimlane-move";
|
||||
import { Graph2TaskPipeline } from "./graph2-task-pipeline";
|
||||
import { ORPHAN_STEP, ORPHAN_STEP_ID, remapOrphanTasks } from "./swimlane-kanban-content";
|
||||
import type { ViewContentProps } from "@/lib/kanban/view-registry";
|
||||
import type { Task } from "@/components/kanban-card";
|
||||
import type { WorkflowStep } from "@/components/kanban-column";
|
||||
|
||||
export function getGraph2DisplayState(
|
||||
tasks: Task[],
|
||||
steps: WorkflowStep[],
|
||||
): { displayTasks: Task[]; displaySteps: WorkflowStep[] } {
|
||||
const stepIds = new Set(steps.map((step) => step.id));
|
||||
const { tasks: displayTasks, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_STEP_ID);
|
||||
return {
|
||||
displayTasks,
|
||||
displaySteps: hasOrphans ? [...steps, ORPHAN_STEP] : steps,
|
||||
};
|
||||
}
|
||||
|
||||
export function SwimlaneGraph2Content({
|
||||
workflowId,
|
||||
@@ -24,16 +39,20 @@ export function SwimlaneGraph2Content({
|
||||
onMoveError,
|
||||
});
|
||||
const [movingTaskId, setMovingTaskId] = useState<string | null>(null);
|
||||
const { displayTasks, displaySteps } = useMemo(
|
||||
() => getGraph2DisplayState(tasks, steps),
|
||||
[tasks, steps],
|
||||
);
|
||||
|
||||
const sortedTasks = useMemo(
|
||||
() =>
|
||||
[...tasks].sort((a, b) => {
|
||||
const aStepIdx = steps.findIndex((c) => c.id === a.workflowStepId);
|
||||
const bStepIdx = steps.findIndex((c) => c.id === b.workflowStepId);
|
||||
[...displayTasks].sort((a, b) => {
|
||||
const aStepIdx = displaySteps.findIndex((c) => c.id === a.workflowStepId);
|
||||
const bStepIdx = displaySteps.findIndex((c) => c.id === b.workflowStepId);
|
||||
if (aStepIdx !== bStepIdx) return aStepIdx - bStepIdx;
|
||||
return (a.position ?? 0) - (b.position ?? 0);
|
||||
}),
|
||||
[tasks, steps],
|
||||
[displayTasks, displaySteps],
|
||||
);
|
||||
|
||||
const handleMoveTask = async (task: (typeof tasks)[number], targetStepId: string) => {
|
||||
@@ -45,7 +64,7 @@ export function SwimlaneGraph2Content({
|
||||
}
|
||||
};
|
||||
|
||||
if (tasks.length === 0) {
|
||||
if (displayTasks.length === 0) {
|
||||
return (
|
||||
<div className="px-3 pb-3">
|
||||
<div className="text-xs text-muted-foreground text-center py-4">No tasks</div>
|
||||
@@ -60,7 +79,7 @@ export function SwimlaneGraph2Content({
|
||||
<Graph2TaskPipeline
|
||||
key={task.id}
|
||||
task={task}
|
||||
steps={steps}
|
||||
steps={displaySteps}
|
||||
onMoveTask={handleMoveTask}
|
||||
onPreviewTask={onPreviewTask}
|
||||
onOpenTask={onOpenTask}
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { ORPHAN_STEP_ID } from "./swimlane-kanban-content";
|
||||
import { getGraph2DisplayState } from "./swimlane-graph2-content";
|
||||
import type { Task } from "@/components/kanban-card";
|
||||
import type { WorkflowStep } from "@/components/kanban-column";
|
||||
|
||||
function makeTask(id: string, stepId: string, position = 0): Task {
|
||||
return {
|
||||
id,
|
||||
title: id,
|
||||
workflowStepId: stepId,
|
||||
position,
|
||||
} as Task;
|
||||
}
|
||||
|
||||
const steps: WorkflowStep[] = [
|
||||
{ id: "todo", title: "Todo", color: "#64748b" },
|
||||
{ id: "done", title: "Done", color: "#22c55e" },
|
||||
];
|
||||
|
||||
describe("getGraph2DisplayState", () => {
|
||||
it("adds a Needs Reassignment pipeline step for tasks on deleted workflow steps", () => {
|
||||
const { displayTasks, displaySteps } = getGraph2DisplayState(
|
||||
[makeTask("valid", "todo"), makeTask("orphan", "deleted-step")],
|
||||
steps,
|
||||
);
|
||||
|
||||
expect(displaySteps.map((step) => step.title)).toEqual(["Todo", "Done", "Needs Reassignment"]);
|
||||
expect(displayTasks.find((task) => task.id === "valid")?.workflowStepId).toBe("todo");
|
||||
expect(displayTasks.find((task) => task.id === "orphan")?.workflowStepId).toBe(ORPHAN_STEP_ID);
|
||||
});
|
||||
|
||||
it("keeps the pipeline unchanged when every task references a rendered step", () => {
|
||||
const { displayTasks, displaySteps } = getGraph2DisplayState(
|
||||
[makeTask("valid", "todo"), makeTask("finished", "done")],
|
||||
steps,
|
||||
);
|
||||
|
||||
expect(displaySteps).toBe(steps);
|
||||
expect(displayTasks.map((task) => task.workflowStepId)).toEqual(["todo", "done"]);
|
||||
});
|
||||
});
|
||||
@@ -31,6 +31,28 @@ import {
|
||||
useKanbanExternalLinkAvailability,
|
||||
} from "@/components/kanban-external-link-availability";
|
||||
|
||||
/**
|
||||
* Sentinel step ID used to collect tasks whose workflow_step_id no longer
|
||||
* matches any rendered column. Tasks remapped here are visible as a
|
||||
* "Needs Reassignment" fallback column so they are never silently hidden.
|
||||
*/
|
||||
export const ORPHAN_STEP_ID = "__kandev_orphan__";
|
||||
|
||||
export const ORPHAN_STEP: WorkflowStep = {
|
||||
id: ORPHAN_STEP_ID,
|
||||
title: "Needs Reassignment",
|
||||
color: "#f59e0b",
|
||||
};
|
||||
|
||||
/**
|
||||
* The "Needs Reassignment" column is a display-only fallback, not a real
|
||||
* workflow step — it must never be offered as a manual move destination
|
||||
* (drag-and-drop, "Move to" menus, or Pipeline navigation).
|
||||
*/
|
||||
export function isOrphanMoveTarget(targetStepId: string): boolean {
|
||||
return targetStepId === ORPHAN_STEP_ID;
|
||||
}
|
||||
|
||||
export type SwimlaneKanbanContentProps = {
|
||||
workflowId: string;
|
||||
steps: WorkflowStep[];
|
||||
@@ -82,7 +104,7 @@ function useSwimlaneKanbanDnd({ tasks, workflowId, onMoveError }: SwimlaneKanban
|
||||
const taskId = active.id as string;
|
||||
const targetStepId = over.id as string;
|
||||
const task = tasks.find((t) => t.id === taskId);
|
||||
if (!task || task.workflowStepId === targetStepId) return;
|
||||
if (!task || task.workflowStepId === targetStepId || isOrphanMoveTarget(targetStepId)) return;
|
||||
|
||||
const state = store.getState();
|
||||
const snapshot = state.kanbanMulti.snapshots[workflowId];
|
||||
@@ -176,6 +198,46 @@ function useMobileColumnIndex(workflowId: string, steps: WorkflowStep[], tasks:
|
||||
return { activeIndex, setActiveIndex };
|
||||
}
|
||||
|
||||
/**
|
||||
* remapOrphanTasks re-keys any task whose workflowStepId matches no step in
|
||||
* `stepIds` to `orphanStepId`. Returns the remapped task list and whether
|
||||
* any orphans were found. Pure function — safe to call outside React.
|
||||
*/
|
||||
export function remapOrphanTasks(
|
||||
tasks: Task[],
|
||||
stepIds: Set<string>,
|
||||
orphanStepId: string,
|
||||
): { tasks: Task[]; hasOrphans: boolean } {
|
||||
let hasOrphans = false;
|
||||
const remapped = tasks.map((t) => {
|
||||
if (!t.workflowStepId || stepIds.has(t.workflowStepId)) return t;
|
||||
hasOrphans = true;
|
||||
return { ...t, workflowStepId: orphanStepId };
|
||||
});
|
||||
return { tasks: remapped, hasOrphans };
|
||||
}
|
||||
|
||||
/**
|
||||
* useOrphanDisplay remaps tasks with an unknown workflowStepId to the sentinel
|
||||
* ORPHAN_STEP so they appear in a visible fallback column instead of being
|
||||
* silently dropped from the board.
|
||||
*
|
||||
* Returns:
|
||||
* displayTasks – all tasks, with orphaned ones keyed to ORPHAN_STEP_ID
|
||||
* displaySteps – original steps plus ORPHAN_STEP when orphans are present
|
||||
*/
|
||||
function useOrphanDisplay(
|
||||
tasks: Task[],
|
||||
steps: WorkflowStep[],
|
||||
): { displayTasks: Task[]; displaySteps: WorkflowStep[] } {
|
||||
return useMemo(() => {
|
||||
const stepIds = new Set(steps.map((s) => s.id));
|
||||
const { tasks: displayTasks, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_STEP_ID);
|
||||
const displaySteps = hasOrphans ? [...steps, ORPHAN_STEP] : steps;
|
||||
return { displayTasks, displaySteps };
|
||||
}, [tasks, steps]);
|
||||
}
|
||||
|
||||
function useTasksByStep(tasks: Task[]) {
|
||||
return useCallback(
|
||||
(stepId: string) =>
|
||||
@@ -186,6 +248,7 @@ function useTasksByStep(tasks: Task[]) {
|
||||
|
||||
function MobileKanbanLayout({
|
||||
steps,
|
||||
moveTargetSteps,
|
||||
tasks,
|
||||
activeIndex,
|
||||
onIndexChange,
|
||||
@@ -205,26 +268,10 @@ function MobileKanbanLayout({
|
||||
isMultiSelectMode,
|
||||
externalLinkAvailability,
|
||||
mobileWorkflowNavigation,
|
||||
}: {
|
||||
steps: WorkflowStep[];
|
||||
tasks: Task[];
|
||||
}: SharedKanbanLayoutProps & {
|
||||
activeIndex: number;
|
||||
onIndexChange: (index: number) => void;
|
||||
onPreviewTask: (task: Task) => void;
|
||||
onOpenTask: (task: Task) => void;
|
||||
onEditTask: (task: Task) => void;
|
||||
onDeleteTask: (task: Task) => void;
|
||||
onArchiveTask?: (task: Task) => void;
|
||||
moveTaskToStep: (task: Task, targetStepId: string) => Promise<void>;
|
||||
activeTask: Task | null;
|
||||
showMaximizeButton?: boolean;
|
||||
deletingTaskId?: string | null;
|
||||
archivingTaskId?: string | null;
|
||||
selectedIds?: Set<string>;
|
||||
onToggleSelect?: (taskId: string) => void;
|
||||
onSelectRange?: (taskId: string, orderedIds: string[]) => void;
|
||||
isMultiSelectMode?: boolean;
|
||||
externalLinkAvailability: KanbanExternalLinkAvailability;
|
||||
mobileWorkflowNavigation?: MobileWorkflowNavigation;
|
||||
}) {
|
||||
const taskCounts = useMemo(() => {
|
||||
@@ -261,6 +308,7 @@ function MobileKanbanLayout({
|
||||
) : (
|
||||
<SwipeableColumns
|
||||
steps={steps}
|
||||
moveTargetSteps={moveTargetSteps}
|
||||
tasks={tasks}
|
||||
activeIndex={activeIndex}
|
||||
onIndexChange={onIndexChange}
|
||||
@@ -280,30 +328,21 @@ function MobileKanbanLayout({
|
||||
externalLinkAvailability={externalLinkAvailability}
|
||||
/>
|
||||
)}
|
||||
<MobileDropTargets steps={steps} currentStepId={currentStepId} isDragging={!!activeTask} />
|
||||
<MobileDropTargets
|
||||
steps={moveTargetSteps}
|
||||
currentStepId={currentStepId}
|
||||
isDragging={!!activeTask}
|
||||
/>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
function TabletKanbanLayout({
|
||||
steps,
|
||||
tasks,
|
||||
onPreviewTask,
|
||||
onOpenTask,
|
||||
onEditTask,
|
||||
onDeleteTask,
|
||||
onArchiveTask,
|
||||
moveTaskToStep,
|
||||
showMaximizeButton,
|
||||
deletingTaskId,
|
||||
archivingTaskId,
|
||||
selectedIds,
|
||||
onToggleSelect,
|
||||
onSelectRange,
|
||||
isMultiSelectMode,
|
||||
externalLinkAvailability,
|
||||
}: {
|
||||
type SharedKanbanLayoutProps = {
|
||||
steps: WorkflowStep[];
|
||||
// Real workflow steps only (excludes the synthetic "Needs Reassignment"
|
||||
// sentinel) — used wherever a step is offered as a move destination
|
||||
// (move menus, drop targets), since that sentinel is display-only.
|
||||
moveTargetSteps: WorkflowStep[];
|
||||
tasks: Task[];
|
||||
onPreviewTask: (task: Task) => void;
|
||||
onOpenTask: (task: Task) => void;
|
||||
@@ -319,7 +358,27 @@ function TabletKanbanLayout({
|
||||
onSelectRange?: (taskId: string, orderedIds: string[]) => void;
|
||||
isMultiSelectMode?: boolean;
|
||||
externalLinkAvailability: KanbanExternalLinkAvailability;
|
||||
}) {
|
||||
};
|
||||
|
||||
function TabletKanbanLayout({
|
||||
steps,
|
||||
moveTargetSteps,
|
||||
tasks,
|
||||
onPreviewTask,
|
||||
onOpenTask,
|
||||
onEditTask,
|
||||
onDeleteTask,
|
||||
onArchiveTask,
|
||||
moveTaskToStep,
|
||||
showMaximizeButton,
|
||||
deletingTaskId,
|
||||
archivingTaskId,
|
||||
selectedIds,
|
||||
onToggleSelect,
|
||||
onSelectRange,
|
||||
isMultiSelectMode,
|
||||
externalLinkAvailability,
|
||||
}: SharedKanbanLayoutProps) {
|
||||
const getTasksForStep = useTasksByStep(tasks);
|
||||
|
||||
return (
|
||||
@@ -338,7 +397,7 @@ function TabletKanbanLayout({
|
||||
onDeleteTask={onDeleteTask}
|
||||
onArchiveTask={onArchiveTask}
|
||||
onMoveTask={moveTaskToStep}
|
||||
steps={steps}
|
||||
steps={moveTargetSteps}
|
||||
showMaximizeButton={showMaximizeButton}
|
||||
deletingTaskId={deletingTaskId}
|
||||
archivingTaskId={archivingTaskId}
|
||||
@@ -356,6 +415,7 @@ function TabletKanbanLayout({
|
||||
|
||||
function DesktopKanbanLayout({
|
||||
steps,
|
||||
moveTargetSteps,
|
||||
tasks,
|
||||
onPreviewTask,
|
||||
onOpenTask,
|
||||
@@ -372,24 +432,8 @@ function DesktopKanbanLayout({
|
||||
isMultiSelectMode,
|
||||
isCompactDesktop,
|
||||
externalLinkAvailability,
|
||||
}: {
|
||||
steps: WorkflowStep[];
|
||||
tasks: Task[];
|
||||
onPreviewTask: (task: Task) => void;
|
||||
onOpenTask: (task: Task) => void;
|
||||
onEditTask: (task: Task) => void;
|
||||
onDeleteTask: (task: Task) => void;
|
||||
onArchiveTask?: (task: Task) => void;
|
||||
moveTaskToStep: (task: Task, targetStepId: string) => Promise<void>;
|
||||
showMaximizeButton?: boolean;
|
||||
deletingTaskId?: string | null;
|
||||
archivingTaskId?: string | null;
|
||||
selectedIds?: Set<string>;
|
||||
onToggleSelect?: (taskId: string) => void;
|
||||
onSelectRange?: (taskId: string, orderedIds: string[]) => void;
|
||||
isMultiSelectMode?: boolean;
|
||||
}: SharedKanbanLayoutProps & {
|
||||
isCompactDesktop: boolean;
|
||||
externalLinkAvailability: KanbanExternalLinkAvailability;
|
||||
}) {
|
||||
const getTasksForStep = useTasksByStep(tasks);
|
||||
|
||||
@@ -412,7 +456,7 @@ function DesktopKanbanLayout({
|
||||
onDeleteTask={onDeleteTask}
|
||||
onArchiveTask={onArchiveTask}
|
||||
onMoveTask={moveTaskToStep}
|
||||
steps={steps}
|
||||
steps={moveTargetSteps}
|
||||
deletingTaskId={deletingTaskId}
|
||||
archivingTaskId={archivingTaskId}
|
||||
showMaximizeButton={showMaximizeButton}
|
||||
@@ -427,6 +471,50 @@ function DesktopKanbanLayout({
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Picks the responsive layout to render for the swimlane. Extracted so
|
||||
* `SwimlaneKanbanContent` stays under the max-lines-per-function limit.
|
||||
*/
|
||||
function renderKanbanLayout({
|
||||
isMobile,
|
||||
isTablet,
|
||||
isCompactDesktop,
|
||||
sharedProps,
|
||||
activeIndex,
|
||||
setActiveIndex,
|
||||
activeTask,
|
||||
mobileWorkflowNavigation,
|
||||
}: {
|
||||
isMobile: boolean;
|
||||
isTablet: boolean;
|
||||
isCompactDesktop: boolean;
|
||||
sharedProps: SharedKanbanLayoutProps;
|
||||
activeIndex: number;
|
||||
setActiveIndex: (index: number) => void;
|
||||
activeTask: Task | null;
|
||||
mobileWorkflowNavigation?: MobileWorkflowNavigation;
|
||||
}): React.ReactNode {
|
||||
if (isMobile) {
|
||||
return (
|
||||
<MobileKanbanLayout
|
||||
{...sharedProps}
|
||||
activeIndex={activeIndex}
|
||||
onIndexChange={setActiveIndex}
|
||||
activeTask={activeTask}
|
||||
mobileWorkflowNavigation={mobileWorkflowNavigation}
|
||||
/>
|
||||
);
|
||||
}
|
||||
if (isTablet) {
|
||||
return <TabletKanbanLayout {...sharedProps} />;
|
||||
}
|
||||
return (
|
||||
<div className="h-full overflow-x-auto">
|
||||
<DesktopKanbanLayout {...sharedProps} isCompactDesktop={isCompactDesktop} />
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
export function SwimlaneKanbanContent({
|
||||
workflowId,
|
||||
steps,
|
||||
@@ -449,17 +537,27 @@ export function SwimlaneKanbanContent({
|
||||
const { isMobile, isTablet, isCompactDesktop } = useResponsiveBreakpoint();
|
||||
const activeWorkspaceId = useAppStore((state) => state.workspaces.activeId);
|
||||
const externalLinkAvailability = useKanbanExternalLinkAvailability(activeWorkspaceId);
|
||||
const { activeIndex, setActiveIndex } = useMobileColumnIndex(workflowId, steps, tasks);
|
||||
|
||||
// Remap tasks with a dead workflowStepId to the orphan sentinel so they
|
||||
// always appear in a visible column rather than being silently dropped.
|
||||
const { displayTasks, displaySteps } = useOrphanDisplay(tasks, steps);
|
||||
|
||||
const { activeIndex, setActiveIndex } = useMobileColumnIndex(
|
||||
workflowId,
|
||||
displaySteps,
|
||||
displayTasks,
|
||||
);
|
||||
const { sensors, handleDragStart, handleDragEnd, handleDragCancel, moveTaskToStep, activeTask } =
|
||||
useSwimlaneKanbanDnd({ tasks, workflowId, onMoveError });
|
||||
useSwimlaneKanbanDnd({ tasks: displayTasks, workflowId, onMoveError });
|
||||
|
||||
// Memoized so the layout components don't re-render from a fresh props object
|
||||
// on every parent render. Declared before the early return to keep hook order
|
||||
// stable.
|
||||
const sharedProps = useMemo(
|
||||
() => ({
|
||||
steps,
|
||||
tasks,
|
||||
steps: displaySteps,
|
||||
moveTargetSteps: steps,
|
||||
tasks: displayTasks,
|
||||
onPreviewTask,
|
||||
onOpenTask,
|
||||
onEditTask,
|
||||
@@ -476,8 +574,9 @@ export function SwimlaneKanbanContent({
|
||||
externalLinkAvailability,
|
||||
}),
|
||||
[
|
||||
displaySteps,
|
||||
steps,
|
||||
tasks,
|
||||
displayTasks,
|
||||
onPreviewTask,
|
||||
onOpenTask,
|
||||
onEditTask,
|
||||
@@ -495,28 +594,18 @@ export function SwimlaneKanbanContent({
|
||||
],
|
||||
);
|
||||
|
||||
if (steps.length === 0 && !isMobile) return null;
|
||||
if (displaySteps.length === 0 && !isMobile) return null;
|
||||
|
||||
let layoutContent: React.ReactNode;
|
||||
if (isMobile) {
|
||||
layoutContent = (
|
||||
<MobileKanbanLayout
|
||||
{...sharedProps}
|
||||
activeIndex={activeIndex}
|
||||
onIndexChange={setActiveIndex}
|
||||
activeTask={activeTask}
|
||||
mobileWorkflowNavigation={mobileWorkflowNavigation}
|
||||
/>
|
||||
);
|
||||
} else if (isTablet) {
|
||||
layoutContent = <TabletKanbanLayout {...sharedProps} />;
|
||||
} else {
|
||||
layoutContent = (
|
||||
<div className="h-full overflow-x-auto">
|
||||
<DesktopKanbanLayout {...sharedProps} isCompactDesktop={isCompactDesktop} />
|
||||
</div>
|
||||
);
|
||||
}
|
||||
const layoutContent = renderKanbanLayout({
|
||||
isMobile,
|
||||
isTablet,
|
||||
isCompactDesktop,
|
||||
sharedProps,
|
||||
activeIndex,
|
||||
setActiveIndex,
|
||||
activeTask,
|
||||
mobileWorkflowNavigation,
|
||||
});
|
||||
|
||||
return (
|
||||
<DndContext
|
||||
|
||||
@@ -0,0 +1,60 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { remapOrphanTasks } from "./swimlane-kanban-content";
|
||||
|
||||
const ORPHAN_ID = "__kandev_orphan__";
|
||||
|
||||
function makeTask(id: string, stepId: string) {
|
||||
return { id, workflowStepId: stepId } as Parameters<typeof remapOrphanTasks>[0][number];
|
||||
}
|
||||
|
||||
describe("remapOrphanTasks", () => {
|
||||
it("leaves tasks on known steps unchanged", () => {
|
||||
const stepIds = new Set(["s1", "s2"]);
|
||||
const tasks = [makeTask("t1", "s1"), makeTask("t2", "s2")];
|
||||
const { tasks: out, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_ID);
|
||||
expect(hasOrphans).toBe(false);
|
||||
expect(out[0].workflowStepId).toBe("s1");
|
||||
expect(out[1].workflowStepId).toBe("s2");
|
||||
});
|
||||
|
||||
it("remaps a task with a dead step to the orphan sentinel", () => {
|
||||
const stepIds = new Set(["s1"]);
|
||||
const tasks = [makeTask("t-orphan", "deleted-step")];
|
||||
const { tasks: out, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_ID);
|
||||
expect(hasOrphans).toBe(true);
|
||||
expect(out[0].workflowStepId).toBe(ORPHAN_ID);
|
||||
});
|
||||
|
||||
it("does not mutate the original task object", () => {
|
||||
const stepIds = new Set(["s1"]);
|
||||
const original = makeTask("t-orig", "dead");
|
||||
const { tasks: out } = remapOrphanTasks([original], stepIds, ORPHAN_ID);
|
||||
// Original must not change.
|
||||
expect(original.workflowStepId).toBe("dead");
|
||||
// Returned task is a new object.
|
||||
expect(out[0]).not.toBe(original);
|
||||
});
|
||||
|
||||
it("handles a mix of valid and orphaned tasks", () => {
|
||||
const stepIds = new Set(["s1", "s2"]);
|
||||
const tasks = [makeTask("t-ok1", "s1"), makeTask("t-orphan", "ghost"), makeTask("t-ok2", "s2")];
|
||||
const { tasks: out, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_ID);
|
||||
expect(hasOrphans).toBe(true);
|
||||
expect(out[0].workflowStepId).toBe("s1");
|
||||
expect(out[1].workflowStepId).toBe(ORPHAN_ID);
|
||||
expect(out[2].workflowStepId).toBe("s2");
|
||||
});
|
||||
|
||||
it("does not remap tasks with an empty workflowStepId", () => {
|
||||
const stepIds = new Set(["s1"]);
|
||||
const tasks = [makeTask("t-empty", "")];
|
||||
const { tasks: out, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_ID);
|
||||
expect(hasOrphans).toBe(false);
|
||||
expect(out[0].workflowStepId).toBe("");
|
||||
});
|
||||
|
||||
it("returns hasOrphans=false for an empty task list", () => {
|
||||
const { hasOrphans } = remapOrphanTasks([], new Set(["s1"]), ORPHAN_ID);
|
||||
expect(hasOrphans).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -9,6 +9,9 @@ import type { KanbanExternalLinkAvailability } from "../kanban-external-link-ava
|
||||
|
||||
type SwipeableColumnsProps = {
|
||||
steps: WorkflowStep[];
|
||||
// Real workflow steps only (excludes the "Needs Reassignment" sentinel) —
|
||||
// passed to KanbanColumn purely for move-menu targets.
|
||||
moveTargetSteps: WorkflowStep[];
|
||||
tasks: Task[];
|
||||
activeIndex: number;
|
||||
onIndexChange: (index: number) => void;
|
||||
@@ -66,6 +69,7 @@ function useEmblaIndexSync(
|
||||
|
||||
export function SwipeableColumns({
|
||||
steps,
|
||||
moveTargetSteps,
|
||||
tasks,
|
||||
activeIndex,
|
||||
onIndexChange,
|
||||
@@ -126,7 +130,7 @@ export function SwipeableColumns({
|
||||
onDeleteTask={onDeleteTask}
|
||||
onArchiveTask={onArchiveTask}
|
||||
onMoveTask={onMoveTask}
|
||||
steps={steps}
|
||||
steps={moveTargetSteps}
|
||||
showMaximizeButton={showMaximizeButton}
|
||||
deletingTaskId={deletingTaskId}
|
||||
archivingTaskId={archivingTaskId}
|
||||
|
||||
Reference in New Issue
Block a user