fix(kanban): stop stuck spinner by writing task.state=REVIEW on engine on_turn_complete (#1001)
* fix(web): stop kanban spinner when primary session is terminal The kanban card spinner was gated only on `task.state` being IN_PROGRESS or SCHEDULING. Workflows leave a task in the IN_PROGRESS column after the agent's session reaches a terminal state (COMPLETED/FAILED/CANCELLED) so the user can review and move it manually, which made the loading icon spin indefinitely. The frontend already receives `primary_session_state` over SSR and WS; gate the spinner on that not being terminal. Fixes #985. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(orchestrator): write task.state=REVIEW after engine on_turn_complete transition The non-engine path (setSessionWaitingForInput → writeTaskReviewState) has always written tasks.state = REVIEW when the agent's session paused for input. The workflow-engine on_turn_complete path forgot the task-state write: it persisted the new workflow step + flipped the session to WAITING_FOR_INPUT but left tasks.state stale at IN_PROGRESS. That left the kanban card stuck in IN_PROGRESS — with a spinning loader — even after the workflow had moved it into the "Review" column. Mirror the non-engine path: call writeTaskReviewState inside the RUNNING/STARTING branch of applyEngineTransition. Add a regression test that asserts tasks.state = REVIEW after an engine-driven on_turn_complete transition. Also tighten the frontend gate added in the previous commit to suppress the spinner for any non-active primary-session state (WAITING_FOR_INPUT, IDLE in addition to COMPLETED/FAILED/CANCELLED). The backend fix is the real correction; the broader frontend gate stays as defense-in-depth against future state-sync drift. Refs #985. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(orchestrator): drop redundant time.Sleep in engine on_turn_complete test writeTaskReviewState and ApplyTransition both run synchronously inside applyEngineTransition (before processOnEnter's goroutine), so the task-state and step assertions need no async wait. assertSessionState already has its own 500ms polling loop for the WAITING_FOR_INPUT flip. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
João Salavisa
@@ -1828,9 +1828,20 @@ func (s *Service) applyEngineTransition(
|
||||
// Flip to WAITING_FOR_INPUT so that autoStartStepPrompt in processOnEnter sends
|
||||
// the prompt directly instead of queueing it — the queue would never be drained
|
||||
// because handleAgentReady already returned.
|
||||
//
|
||||
// Mirror setSessionWaitingForInput's task-state side effect: write
|
||||
// tasks.state = REVIEW so the kanban card drops out of IN_PROGRESS. Without
|
||||
// this, an engine-driven on_turn_complete transition would persist the
|
||||
// new workflow step + flip the session but leave tasks.state stale at
|
||||
// IN_PROGRESS, leaving the spinner spinning in the new column even though
|
||||
// the agent has paused. If the target step's on_enter starts another agent,
|
||||
// setSessionRunning will flip tasks.state back to IN_PROGRESS — the
|
||||
// REVIEW write is a safe intermediate that any active-running follow-up
|
||||
// will overwrite.
|
||||
if session.State == models.TaskSessionStateRunning || session.State == models.TaskSessionStateStarting {
|
||||
s.updateTaskSessionState(ctx, taskID, session.ID, models.TaskSessionStateWaitingForInput, "", false, session)
|
||||
session.State = models.TaskSessionStateWaitingForInput
|
||||
s.writeTaskReviewState(ctx, taskID)
|
||||
}
|
||||
|
||||
// Launch processOnEnter asynchronously to avoid blocking the stream reader goroutine.
|
||||
|
||||
@@ -8,6 +8,8 @@ import (
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
v1 "github.com/kandev/kandev/pkg/api/v1"
|
||||
|
||||
"github.com/kandev/kandev/internal/orchestrator/executor"
|
||||
"github.com/kandev/kandev/internal/orchestrator/messagequeue"
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
@@ -408,3 +410,55 @@ func assertSessionState(t *testing.T, ctx context.Context, repo sessionExecutorS
|
||||
time.Sleep(10 * time.Millisecond)
|
||||
}
|
||||
}
|
||||
|
||||
// Regression for #985: an on_turn_complete engine transition flips the
|
||||
// session to WAITING_FOR_INPUT and moves the task to the next step, but the
|
||||
// pre-fix code path forgot to write tasks.state = REVIEW. That left the
|
||||
// kanban card stuck in IN_PROGRESS — with a spinning loader — even though
|
||||
// the agent had paused and the card had already moved into the "Review"
|
||||
// column. Mirrors what setSessionWaitingForInput (the non-engine path)
|
||||
// has always done via writeTaskReviewState.
|
||||
//
|
||||
// The chosen transition is "New Step" → "Done": "Done" has no on_enter, so
|
||||
// no follow-up action can mask the missing write (e.g. auto_start_agent
|
||||
// would otherwise overwrite tasks.state back to IN_PROGRESS via
|
||||
// setSessionRunning).
|
||||
func TestProcessOnTurnCompleteViaEngine_WritesTaskStateReview(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
sg, nameToID := buildWorkflowFromJSON(t, developmentWorkflowJSON)
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", nameToID["New Step"])
|
||||
setSessionExecID(t, repo, "s1", "exec-1")
|
||||
setSessionState(t, ctx, repo, "s1", models.TaskSessionStateRunning)
|
||||
|
||||
agentMgr := &mockAgentManager{repoForExecutionLookup: repo}
|
||||
taskRepo := newMockTaskRepo()
|
||||
svc := createEngineService(t, repo, sg, agentMgr)
|
||||
svc.taskRepo = taskRepo
|
||||
|
||||
session, err := repo.GetTaskSession(ctx, "s1")
|
||||
if err != nil {
|
||||
t.Fatalf("failed to load session: %v", err)
|
||||
}
|
||||
|
||||
transitioned := svc.processOnTurnCompleteViaEngine(ctx, "t1", session)
|
||||
if !transitioned {
|
||||
t.Fatalf("expected a transition from New Step → Done, got none")
|
||||
}
|
||||
|
||||
// writeTaskReviewState and ApplyTransition both run synchronously inside
|
||||
// applyEngineTransition (before processOnEnter's goroutine), so the
|
||||
// task-state and step assertions need no async wait. assertSessionState
|
||||
// has its own polling loop for the WAITING_FOR_INPUT flip.
|
||||
assertStepByName(t, ctx, repo, "s1", "Done", nameToID)
|
||||
assertSessionState(t, ctx, repo, "s1", models.TaskSessionStateWaitingForInput)
|
||||
|
||||
got, ok := taskRepo.updatedStates["t1"]
|
||||
if !ok {
|
||||
t.Fatalf("expected tasks.state to be updated, but UpdateTaskState was never called")
|
||||
}
|
||||
if got != v1.TaskStateReview {
|
||||
t.Errorf("tasks.state = %q, want %q (the kanban card would still show the spinner)", got, v1.TaskStateReview)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -17,7 +17,11 @@ import {
|
||||
import { useAppStore } from "@/components/state-provider";
|
||||
import { RemoteCloudTooltip } from "@/components/task/remote-cloud-tooltip";
|
||||
import { useTaskPendingClarification } from "@/hooks/use-task-pending-clarification";
|
||||
import { getTaskStateIcon, shouldUseQuestionTaskIcon } from "@/lib/ui/state-icons";
|
||||
import {
|
||||
getTaskStateIcon,
|
||||
shouldShowTaskRunningSpinner,
|
||||
shouldUseQuestionTaskIcon,
|
||||
} from "@/lib/ui/state-icons";
|
||||
import { cn } from "@/lib/utils";
|
||||
import { needsAction } from "@/lib/utils/needs-action";
|
||||
import type { Task } from "@/components/kanban-card";
|
||||
@@ -188,14 +192,14 @@ function KanbanCardActions({
|
||||
const effectiveMenuOpen = menuOpen || Boolean(isDeleting) || Boolean(isArchiving);
|
||||
const hasPendingClarificationRequest = useTaskPendingClarification(task.primarySessionId);
|
||||
const showQuestionIcon = shouldUseQuestionTaskIcon(task.state, hasPendingClarificationRequest);
|
||||
const showRunningSpinner = shouldShowTaskRunningSpinner(task.state, task.primarySessionState);
|
||||
const statusIcon = getTaskStateIcon(task.state, "h-4 w-4", hasPendingClarificationRequest);
|
||||
const hasKnownSession =
|
||||
Boolean(task.primarySessionId) || Boolean(task.sessionCount && task.sessionCount > 0);
|
||||
|
||||
return (
|
||||
<div className="flex items-center gap-2">
|
||||
{(task.state === "IN_PROGRESS" || task.state === "SCHEDULING" || showQuestionIcon) &&
|
||||
statusIcon}
|
||||
{(showRunningSpinner || showQuestionIcon) && statusIcon}
|
||||
{showMaximizeButton && onOpenFullPage && hasKnownSession && (
|
||||
<button
|
||||
type="button"
|
||||
|
||||
@@ -27,6 +27,12 @@ export interface Task {
|
||||
repositories?: Array<{ id: string; repository_id: string; position: number }>;
|
||||
sessionCount?: number | null;
|
||||
primarySessionId?: string | null;
|
||||
/**
|
||||
* Primary session's runtime state. Decoupled from `state` (the workflow
|
||||
* column). Used to suppress the running-spinner when the agent has already
|
||||
* finished — the workflow may leave the task in IN_PROGRESS for review.
|
||||
*/
|
||||
primarySessionState?: string | null;
|
||||
reviewStatus?: "pending" | "approved" | "changes_requested" | "rejected" | null;
|
||||
primaryExecutorId?: string | null;
|
||||
primaryExecutorType?: string | null;
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { isValidElement, type ReactNode } from "react";
|
||||
import { IconCheck, IconMessageQuestion } from "@tabler/icons-react";
|
||||
import { getTaskStateIcon } from "./state-icons";
|
||||
import { getTaskStateIcon, shouldShowTaskRunningSpinner } from "./state-icons";
|
||||
|
||||
function iconType(node: ReactNode) {
|
||||
if (!isValidElement(node)) throw new Error("Expected React element");
|
||||
@@ -21,3 +21,48 @@ describe("getTaskStateIcon", () => {
|
||||
expect(iconType(getTaskStateIcon("REVIEW", undefined, false))).toBe(IconCheck);
|
||||
});
|
||||
});
|
||||
|
||||
describe("shouldShowTaskRunningSpinner", () => {
|
||||
it("returns false for non-loading task states regardless of session state", () => {
|
||||
expect(shouldShowTaskRunningSpinner("COMPLETED")).toBe(false);
|
||||
expect(shouldShowTaskRunningSpinner("FAILED")).toBe(false);
|
||||
expect(shouldShowTaskRunningSpinner("CANCELLED")).toBe(false);
|
||||
expect(shouldShowTaskRunningSpinner("REVIEW")).toBe(false);
|
||||
expect(shouldShowTaskRunningSpinner("TODO")).toBe(false);
|
||||
expect(shouldShowTaskRunningSpinner("REVIEW", "RUNNING")).toBe(false);
|
||||
});
|
||||
|
||||
it("returns true for SCHEDULING with no primary session yet", () => {
|
||||
expect(shouldShowTaskRunningSpinner("SCHEDULING")).toBe(true);
|
||||
expect(shouldShowTaskRunningSpinner("SCHEDULING", null)).toBe(true);
|
||||
expect(shouldShowTaskRunningSpinner("SCHEDULING", undefined)).toBe(true);
|
||||
});
|
||||
|
||||
it("returns true for IN_PROGRESS when the primary session is actively running", () => {
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "RUNNING")).toBe(true);
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "STARTING")).toBe(true);
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "CREATED")).toBe(true);
|
||||
});
|
||||
|
||||
it("returns true for IN_PROGRESS when no primary session is attached yet", () => {
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", undefined)).toBe(true);
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", null)).toBe(true);
|
||||
});
|
||||
|
||||
it("suppresses the spinner when the primary session is terminal", () => {
|
||||
// Repro from issue #985: agent finishes (session → COMPLETED) but the
|
||||
// workflow leaves the task in IN_PROGRESS for review/manual move. The
|
||||
// spinner must not keep spinning forever.
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "COMPLETED")).toBe(false);
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "FAILED")).toBe(false);
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "CANCELLED")).toBe(false);
|
||||
expect(shouldShowTaskRunningSpinner("SCHEDULING", "COMPLETED")).toBe(false);
|
||||
});
|
||||
|
||||
it("suppresses the spinner when the primary session is paused (waiting/idle)", () => {
|
||||
// Same desync class, paused branch: agent stopped to wait for input or
|
||||
// was torn down (office IDLE). The spinner is misleading.
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "WAITING_FOR_INPUT")).toBe(false);
|
||||
expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "IDLE")).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -72,6 +72,36 @@ export function shouldUseQuestionTaskIcon(
|
||||
return isWaitingForInputState(state) || hasPendingClarification;
|
||||
}
|
||||
|
||||
// Session states where the agent is actively running work. Anything outside
|
||||
// this set (WAITING_FOR_INPUT, IDLE, COMPLETED, FAILED, CANCELLED) is paused
|
||||
// or terminal and must not drive the spinner — even when the task is still
|
||||
// in the IN_PROGRESS workflow column.
|
||||
const ACTIVE_SESSION_STATES: ReadonlySet<string> = new Set<TaskSessionState>([
|
||||
"CREATED",
|
||||
"STARTING",
|
||||
"RUNNING",
|
||||
]);
|
||||
|
||||
/**
|
||||
* Returns true when the kanban card should show the spinning loader for an
|
||||
* in-progress task. The task workflow state and the primary session's runtime
|
||||
* state are decoupled — the workflow can keep a task in `IN_PROGRESS` after
|
||||
* the agent has finished, errored, or paused waiting for input — so we gate
|
||||
* the spinner on the primary session being actively running.
|
||||
*
|
||||
* When no primary session is attached yet (task just created / scheduling),
|
||||
* we still show the spinner so users see the imminent work; otherwise we
|
||||
* require an active session state.
|
||||
*/
|
||||
export function shouldShowTaskRunningSpinner(
|
||||
taskState?: TaskState,
|
||||
primarySessionState?: string | null,
|
||||
): boolean {
|
||||
if (taskState !== "IN_PROGRESS" && taskState !== "SCHEDULING") return false;
|
||||
if (!primarySessionState) return true;
|
||||
return ACTIVE_SESSION_STATES.has(primarySessionState);
|
||||
}
|
||||
|
||||
export function shouldUsePermissionTaskIcon(hasPendingPermission = false): boolean {
|
||||
return hasPendingPermission;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user