From cf9c98ded007d91d539bdecf2c47bfcc19c4cc5d Mon Sep 17 00:00:00 2001 From: Henry Date: Thu, 17 Sep 2026 21:34:22 +0800 Subject: [PATCH] fix(office): routine create/save persists every field and can arm a cron schedule (#3759) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../[id]/coordinator-routine-hint.test.tsx | 91 +++ apps/web/app/office/agents/[id]/layout.tsx | 11 +- apps/web/app/office/lib/catch-up-max.test.ts | 32 + apps/web/app/office/lib/catch-up-max.ts | 10 + .../routines/[id]/cron-reconcile.test.ts | 221 +++++++ .../office/routines/[id]/cron-reconcile.ts | 92 +++ apps/web/app/office/routines/[id]/page.tsx | 14 +- .../[id]/routine-detail-view.test.tsx | 306 +++++++++- .../routines/[id]/routine-detail-view.tsx | 172 +++--- .../routines/create-routine-dialog.test.tsx | 79 ++- .../office/routines/create-routine-dialog.tsx | 104 ++-- .../app/office/routines/routine-row.test.ts | 34 ++ apps/web/app/office/routines/routine-row.tsx | 52 +- .../office/routines/routine-wire-scan.test.ts | 121 ++++ .../office/routines/routines-content.test.tsx | 301 ++++++++++ .../app/office/routines/routines-content.tsx | 97 +-- apps/web/e2e/helpers/office-api-client.ts | 19 +- .../office/routine-catch-up-policy-ui.spec.ts | 71 ++- apps/web/e2e/tests/office/routines-ui.spec.ts | 332 +++++++++++ apps/web/lib/api/domains/office-api.ts | 102 +--- .../api/domains/office-routine-api.test.ts | 124 ++++ .../web/lib/api/domains/office-routine-api.ts | 154 +++++ .../domains/office-routine-normalize.test.ts | 331 +++++++++++ .../api/domains/office-routine-normalize.ts | 182 ++++++ apps/web/lib/state/slices/office/types.ts | 50 +- apps/web/src/locales/en/office.json | 6 + apps/web/src/locales/pseudo/office.json | 6 + apps/web/src/locales/pt-pt/office.json | 6 + apps/web/src/locales/zh-cn/office.json | 6 + apps/web/src/locales/zh-hk/office.json | 6 + apps/web/src/locales/zh-tw/office.json | 6 + .../src/office-routine-client-routes.test.tsx | 94 +++ apps/web/src/office-routine-client-routes.tsx | 15 +- docs/plans/routine-wire-contract/plan.md | 116 ++++ .../task-01-wire-adapter.md | 157 +++++ .../requirements/routine-wire-contract.md | 339 +++++++++++ .../system-design/routine-wire-contract.md | 554 ++++++++++++++++++ 37 files changed, 4127 insertions(+), 286 deletions(-) create mode 100644 apps/web/app/office/agents/[id]/coordinator-routine-hint.test.tsx create mode 100644 apps/web/app/office/lib/catch-up-max.test.ts create mode 100644 apps/web/app/office/lib/catch-up-max.ts create mode 100644 apps/web/app/office/routines/[id]/cron-reconcile.test.ts create mode 100644 apps/web/app/office/routines/[id]/cron-reconcile.ts create mode 100644 apps/web/app/office/routines/routine-row.test.ts create mode 100644 apps/web/app/office/routines/routine-wire-scan.test.ts create mode 100644 apps/web/app/office/routines/routines-content.test.tsx create mode 100644 apps/web/lib/api/domains/office-routine-api.test.ts create mode 100644 apps/web/lib/api/domains/office-routine-api.ts create mode 100644 apps/web/lib/api/domains/office-routine-normalize.test.ts create mode 100644 apps/web/lib/api/domains/office-routine-normalize.ts create mode 100644 apps/web/src/office-routine-client-routes.test.tsx create mode 100644 docs/plans/routine-wire-contract/plan.md create mode 100644 docs/plans/routine-wire-contract/task-01-wire-adapter.md create mode 100644 docs/specs/office/requirements/routine-wire-contract.md create mode 100644 docs/specs/office/system-design/routine-wire-contract.md diff --git a/apps/web/app/office/agents/[id]/coordinator-routine-hint.test.tsx b/apps/web/app/office/agents/[id]/coordinator-routine-hint.test.tsx new file mode 100644 index 0000000000..eee277cf6e --- /dev/null +++ b/apps/web/app/office/agents/[id]/coordinator-routine-hint.test.tsx @@ -0,0 +1,91 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { cleanup, render, screen } from "@testing-library/react"; +import { TooltipProvider } from "@kandev/ui/tooltip"; +import { StateProvider } from "@/components/state-provider"; +import { defaultOfficeState } from "@/lib/state/slices/office/office-slice"; +import type { Routine } from "@/lib/state/slices/office/types"; +import { CoordinatorRoutineHint } from "./layout"; + +afterEach(() => { + cleanup(); +}); + +const WORKSPACE_ID = "ws-1"; +const AGENT_ID = "agent-1"; +const TIMESTAMP = "2026-05-04T00:00:00Z"; +const HINT_LABEL = "No scheduled wake-ups; manage routines"; + +function baseRoutine(overrides: Partial = {}): Routine { + return { + id: "routine-1", + workspaceId: WORKSPACE_ID, + name: "Daily standup", + taskTemplate: {}, + status: "active", + concurrencyPolicy: "coalesce_if_active", + assigneeAgentProfileId: AGENT_ID, + createdAt: TIMESTAMP, + updatedAt: TIMESTAMP, + ...overrides, + }; +} + +function renderHint(routines: Routine[], agentRole = "ceo") { + return render( + + + + + , + ); +} + +describe("CoordinatorRoutineHint", () => { + it("shows no hint for a coordinator with a routine whose status is active", () => { + renderHint([baseRoutine({ status: "active" })]); + expect(screen.queryByLabelText(HINT_LABEL)).toBeNull(); + }); + + it("shows no hint for a coordinator with a routine whose status is the empty string (no writer set one)", () => { + renderHint([baseRoutine({ status: "" })]); + expect(screen.queryByLabelText(HINT_LABEL)).toBeNull(); + }); + + it("shows the hint for a coordinator with a paused routine", () => { + renderHint([baseRoutine({ status: "paused" })]); + expect(screen.getByLabelText(HINT_LABEL)).toBeTruthy(); + }); + + it("shows the hint for a coordinator with no routines at all", () => { + renderHint([]); + expect(screen.getByLabelText(HINT_LABEL)).toBeTruthy(); + }); + + it("shows the hint when the only routine targeting this agent is archived", () => { + renderHint([baseRoutine({ status: "archived" })]); + expect(screen.getByLabelText(HINT_LABEL)).toBeTruthy(); + }); + + it("shows no hint when a routine is active but a different one targeting the agent is paused", () => { + renderHint([ + baseRoutine({ id: "routine-1", status: "paused" }), + baseRoutine({ id: "routine-2", status: "active" }), + ]); + expect(screen.queryByLabelText(HINT_LABEL)).toBeNull(); + }); + + it("ignores an active routine assigned to a different agent", () => { + renderHint([baseRoutine({ assigneeAgentProfileId: "agent-2", status: "active" })]); + expect(screen.getByLabelText(HINT_LABEL)).toBeTruthy(); + }); + + it("renders nothing for a non-coordinator role, regardless of routines", () => { + renderHint([], "worker"); + expect(screen.queryByLabelText(HINT_LABEL)).toBeNull(); + }); +}); diff --git a/apps/web/app/office/agents/[id]/layout.tsx b/apps/web/app/office/agents/[id]/layout.tsx index a22bd44389..706890188e 100644 --- a/apps/web/app/office/agents/[id]/layout.tsx +++ b/apps/web/app/office/agents/[id]/layout.tsx @@ -15,6 +15,7 @@ import { AgentRoleBadge } from "../components/agent-role-badge"; import { BudgetGauge } from "../components/budget-gauge"; import { AgentRouteStrip } from "./components/agent-route-strip"; import { AgentRecoveryControl } from "./components/agent-recovery-control"; +import { isRoutineFiring } from "../../lib/routine-status"; import { Trans, useTranslation } from "react-i18next"; type AgentDetailLayoutProps = { @@ -137,12 +138,18 @@ function activeSlugFromPath(pathname: string | null, agentId: string): string { * navigates to /office/routines to install one. Workers / specialists * don't get this hint since they only run on assignment, not schedule. */ -function CoordinatorRoutineHint({ agentId, agentRole }: { agentId: string; agentRole: string }) { +export function CoordinatorRoutineHint({ + agentId, + agentRole, +}: { + agentId: string; + agentRole: string; +}) { const { t } = useTranslation(); const routines = useAppStore((s) => s.office.routines); if (agentRole !== "ceo") return null; const hasActive = routines.some( - (r) => r.assigneeAgentProfileId === agentId && r.status === "active", + (r) => r.assigneeAgentProfileId === agentId && isRoutineFiring(r.status), ); if (hasActive) return null; return ( diff --git a/apps/web/app/office/lib/catch-up-max.test.ts b/apps/web/app/office/lib/catch-up-max.test.ts new file mode 100644 index 0000000000..2307fafe7d --- /dev/null +++ b/apps/web/app/office/lib/catch-up-max.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from "vitest"; +import { coerceCatchUpMax } from "./catch-up-max"; + +describe("coerceCatchUpMax", () => { + it("passes through a valid positive number", () => { + expect(coerceCatchUpMax(10)).toBe(10); + }); + + it("passes through a valid numeric string", () => { + expect(coerceCatchUpMax("10")).toBe(10); + }); + + it("coerces zero to 25", () => { + expect(coerceCatchUpMax(0)).toBe(25); + }); + + it("coerces a negative number to 25", () => { + expect(coerceCatchUpMax(-5)).toBe(25); + }); + + it("coerces an unparsable string to 25", () => { + expect(coerceCatchUpMax("abc")).toBe(25); + }); + + it("coerces undefined to 25", () => { + expect(coerceCatchUpMax(undefined)).toBe(25); + }); + + it("coerces an empty string to 25", () => { + expect(coerceCatchUpMax("")).toBe(25); + }); +}); diff --git a/apps/web/app/office/lib/catch-up-max.ts b/apps/web/app/office/lib/catch-up-max.ts new file mode 100644 index 0000000000..1a9e7df912 --- /dev/null +++ b/apps/web/app/office/lib/catch-up-max.ts @@ -0,0 +1,10 @@ +/** + * Coerces a routine catch-up max to a valid value for display or form submission. + * Any value below 1 (including NaN from an unparsable string) becomes the + * default of 25, mirroring the backend's own normalization + * (apps/backend/internal/office/models: NormaliseCatchUpMax). + */ +export function coerceCatchUpMax(value: number | string | undefined): number { + const numeric = typeof value === "number" ? value : Number(value); + return Number.isFinite(numeric) && numeric >= 1 ? numeric : 25; +} diff --git a/apps/web/app/office/routines/[id]/cron-reconcile.test.ts b/apps/web/app/office/routines/[id]/cron-reconcile.test.ts new file mode 100644 index 0000000000..711e1681c1 --- /dev/null +++ b/apps/web/app/office/routines/[id]/cron-reconcile.test.ts @@ -0,0 +1,221 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { RoutineTrigger } from "@/lib/state/slices/office/types"; +import { reconcileCronTrigger } from "./cron-reconcile"; +import { + createRoutineTrigger, + deleteRoutineTrigger, + listRoutineTriggers, +} from "@/lib/api/domains/office-api"; + +vi.mock("@/lib/api/domains/office-api", () => ({ + createRoutineTrigger: vi.fn(), + deleteRoutineTrigger: vi.fn(), + listRoutineTriggers: vi.fn(), +})); + +const createRoutineTriggerMock = vi.mocked(createRoutineTrigger); +const deleteRoutineTriggerMock = vi.mocked(deleteRoutineTrigger); +const listRoutineTriggersMock = vi.mocked(listRoutineTriggers); + +afterEach(() => { + vi.clearAllMocks(); +}); + +const TIMESTAMP = "2026-05-04T00:00:00Z"; +const CRON_EXPR = "*/5 * * * *"; +const NEW_TRIGGER_ID = "new-trigger"; +const DELETE_REJECTED_MESSAGE = "delete rejected"; + +function makeTrigger(overrides: Partial = {}): RoutineTrigger { + return { + id: "trigger-1", + routineId: "routine-1", + kind: "cron", + cronExpression: CRON_EXPR, + timezone: "UTC", + enabled: true, + createdAt: TIMESTAMP, + updatedAt: TIMESTAMP, + ...overrides, + }; +} + +describe("reconcileCronTrigger: no-op branches", () => { + it("does nothing for a webhook draft", async () => { + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "webhook", cronExpression: "", timezone: "UTC" }, + [], + ); + expect(outcome).toEqual({ kind: "unchanged" }); + expect(createRoutineTriggerMock).not.toHaveBeenCalled(); + }); + + it("does nothing when the cron expression is empty after trimming", async () => { + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: " ", timezone: "UTC" }, + [makeTrigger()], + ); + expect(outcome).toEqual({ kind: "unchanged" }); + expect(createRoutineTriggerMock).not.toHaveBeenCalled(); + expect(deleteRoutineTriggerMock).not.toHaveBeenCalled(); + }); + + it("does nothing when the expression and effective timezone are unchanged", async () => { + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: CRON_EXPR, timezone: "UTC" }, + [makeTrigger()], + ); + expect(outcome).toEqual({ kind: "unchanged" }); + expect(createRoutineTriggerMock).not.toHaveBeenCalled(); + }); + + it("treats an absent stored timezone and an explicit UTC draft as the same effective timezone", async () => { + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: CRON_EXPR, timezone: "UTC" }, + [makeTrigger({ timezone: undefined })], + ); + expect(outcome).toEqual({ kind: "unchanged" }); + }); + + it("treats a stored UTC timezone and an unset draft timezone as the same effective timezone", async () => { + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: CRON_EXPR, timezone: "" }, + [makeTrigger({ timezone: "UTC" })], + ); + expect(outcome).toEqual({ kind: "unchanged" }); + expect(createRoutineTriggerMock).not.toHaveBeenCalled(); + }); +}); + +describe("reconcileCronTrigger: create/delete orchestration", () => { + it("creates a trigger and skips delete when the routine has no cron trigger yet", async () => { + createRoutineTriggerMock.mockResolvedValue(makeTrigger({ id: NEW_TRIGGER_ID })); + listRoutineTriggersMock.mockResolvedValue({ triggers: [makeTrigger({ id: NEW_TRIGGER_ID })] }); + + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: CRON_EXPR, timezone: "UTC" }, + [], + ); + + expect(createRoutineTriggerMock).toHaveBeenCalledWith("routine-1", { + kind: "cron", + cronExpression: CRON_EXPR, + timezone: "UTC", + }); + expect(deleteRoutineTriggerMock).not.toHaveBeenCalled(); + expect(outcome).toEqual({ kind: "success", triggers: [makeTrigger({ id: NEW_TRIGGER_ID })] }); + }); + + it("creates the replacement before deleting the existing trigger, then re-lists", async () => { + const calls: string[] = []; + createRoutineTriggerMock.mockImplementation(async () => { + calls.push("create"); + return makeTrigger({ id: NEW_TRIGGER_ID }); + }); + deleteRoutineTriggerMock.mockImplementation(async () => { + calls.push("delete"); + }); + listRoutineTriggersMock.mockResolvedValue({ triggers: [makeTrigger({ id: NEW_TRIGGER_ID })] }); + + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: "0 * * * *", timezone: "UTC" }, + [makeTrigger()], + ); + + expect(calls).toEqual(["create", "delete"]); + expect(deleteRoutineTriggerMock).toHaveBeenCalledWith("trigger-1"); + expect(outcome).toEqual({ kind: "success", triggers: [makeTrigger({ id: NEW_TRIGGER_ID })] }); + }); + + it("leaves the existing trigger in place and issues no delete when create is rejected", async () => { + createRoutineTriggerMock.mockRejectedValue(new Error("bad cron")); + listRoutineTriggersMock.mockResolvedValue({ triggers: [makeTrigger()] }); + + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: "bad", timezone: "UTC" }, + [makeTrigger()], + ); + + expect(deleteRoutineTriggerMock).not.toHaveBeenCalled(); + expect(outcome).toEqual({ + kind: "create-failed", + message: "bad cron", + triggers: [makeTrigger()], + }); + }); + + it("reports create-failed with unknown triggers when the re-list after a create failure also fails", async () => { + createRoutineTriggerMock.mockRejectedValue(new Error("bad cron")); + listRoutineTriggersMock.mockRejectedValue(new Error("network down")); + + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: "bad", timezone: "UTC" }, + [makeTrigger()], + ); + + expect(outcome).toEqual({ kind: "create-failed", message: "bad cron", triggers: null }); + }); +}); + +describe("reconcileCronTrigger: delete-failure and stale re-list outcomes", () => { + it("reports delete-failed with the re-listed triggers when delete fails after a successful create", async () => { + createRoutineTriggerMock.mockResolvedValue(makeTrigger({ id: NEW_TRIGGER_ID })); + deleteRoutineTriggerMock.mockRejectedValue(new Error(DELETE_REJECTED_MESSAGE)); + listRoutineTriggersMock.mockResolvedValue({ + triggers: [makeTrigger(), makeTrigger({ id: NEW_TRIGGER_ID })], + }); + + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: "0 * * * *", timezone: "UTC" }, + [makeTrigger()], + ); + + expect(outcome).toEqual({ + kind: "delete-failed", + message: DELETE_REJECTED_MESSAGE, + triggers: [makeTrigger(), makeTrigger({ id: NEW_TRIGGER_ID })], + }); + }); + + it("reports delete-failed with unknown triggers when the re-list after a delete failure also fails", async () => { + createRoutineTriggerMock.mockResolvedValue(makeTrigger({ id: NEW_TRIGGER_ID })); + deleteRoutineTriggerMock.mockRejectedValue(new Error(DELETE_REJECTED_MESSAGE)); + listRoutineTriggersMock.mockRejectedValue(new Error("network down")); + + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: "0 * * * *", timezone: "UTC" }, + [makeTrigger()], + ); + + expect(outcome).toEqual({ + kind: "delete-failed", + message: DELETE_REJECTED_MESSAGE, + triggers: null, + }); + }); + + it("reports success with null triggers (stale) when the re-list fails after create and delete both succeed", async () => { + createRoutineTriggerMock.mockResolvedValue(makeTrigger({ id: NEW_TRIGGER_ID })); + deleteRoutineTriggerMock.mockResolvedValue(undefined); + listRoutineTriggersMock.mockRejectedValue(new Error("network down")); + + const outcome = await reconcileCronTrigger( + "routine-1", + { triggerKind: "cron", cronExpression: "0 * * * *", timezone: "UTC" }, + [makeTrigger()], + ); + + expect(outcome).toEqual({ kind: "success", triggers: null }); + }); +}); diff --git a/apps/web/app/office/routines/[id]/cron-reconcile.ts b/apps/web/app/office/routines/[id]/cron-reconcile.ts new file mode 100644 index 0000000000..254101a33c --- /dev/null +++ b/apps/web/app/office/routines/[id]/cron-reconcile.ts @@ -0,0 +1,92 @@ +import { + createRoutineTrigger, + deleteRoutineTrigger, + listRoutineTriggers, +} from "@/lib/api/domains/office-api"; +import type { RoutineTrigger } from "@/lib/state/slices/office/types"; + +export type CronDraft = { + triggerKind: string; + cronExpression: string; + timezone: string; +}; + +export type CronReconcileOutcome = + | { kind: "unchanged" } + | { kind: "success"; triggers: RoutineTrigger[] | null } + | { kind: "create-failed"; message: string; triggers: RoutineTrigger[] | null } + | { kind: "delete-failed"; message: string; triggers: RoutineTrigger[] | null }; + +function effectiveTimezone(timezone: string | undefined): string { + const trimmed = timezone?.trim(); + return trimmed ? trimmed : "UTC"; +} + +function cronMatches(existing: RoutineTrigger, cronExpression: string, timezone: string): boolean { + return ( + existing.cronExpression === cronExpression && + effectiveTimezone(existing.timezone) === effectiveTimezone(timezone) + ); +} + +async function relistOrNull(routineId: string): Promise { + try { + const res = await listRoutineTriggers(routineId); + return res.triggers; + } catch { + return null; + } +} + +function errorMessage(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} + +/** + * Reconciles the routine's cron trigger with the draft's expression, timezone, + * and kind. Creates the replacement trigger before deleting the old one, and + * always re-lists from the server afterward rather than splicing the local + * array, per docs/specs/office/system-design/routine-wire-contract.md. + */ +export async function reconcileCronTrigger( + routineId: string, + draft: CronDraft, + triggers: RoutineTrigger[], +): Promise { + if (draft.triggerKind !== "cron") return { kind: "unchanged" }; + const cronExpression = draft.cronExpression.trim(); + if (!cronExpression) return { kind: "unchanged" }; + + const existing = triggers.find((t) => t.kind === "cron") ?? null; + if (existing && cronMatches(existing, cronExpression, draft.timezone)) { + return { kind: "unchanged" }; + } + + try { + await createRoutineTrigger(routineId, { + kind: "cron", + cronExpression, + timezone: draft.timezone, + }); + } catch (err) { + return { + kind: "create-failed", + message: errorMessage(err), + triggers: await relistOrNull(routineId), + }; + } + + if (existing) { + try { + await deleteRoutineTrigger(existing.id); + } catch (err) { + return { + kind: "delete-failed", + message: errorMessage(err), + triggers: await relistOrNull(routineId), + }; + } + } + + return { kind: "success", triggers: await relistOrNull(routineId) }; +} diff --git a/apps/web/app/office/routines/[id]/page.tsx b/apps/web/app/office/routines/[id]/page.tsx index 09f0f227f7..b45d2d7bb3 100644 --- a/apps/web/app/office/routines/[id]/page.tsx +++ b/apps/web/app/office/routines/[id]/page.tsx @@ -16,15 +16,17 @@ export default async function RoutineDetailPage({ params }: Props) { let routine: Routine | null = null; let triggers: RoutineTrigger[] = []; try { + // A failed trigger list must not silently become "no trigger exists": + // the client's save-time reconciliation (cron-reconcile.ts) trusts this + // initial list to detect an already-armed cron trigger, so swallowing a + // failure here would make a later Save create a duplicate schedule + // instead of replacing the one that failed to load. Let it fail the + // whole page load instead, same as a `getRoutine` failure below. const [routineRes, triggersRes] = await Promise.all([ getRoutine(id, { cache: "no-store" }), - listRoutineTriggers(id, { cache: "no-store" }).catch(() => ({ - triggers: [] as RoutineTrigger[], - })), + listRoutineTriggers(id, { cache: "no-store" }), ]); - routine = - (routineRes as unknown as { routine?: Routine }).routine ?? - (routineRes as unknown as Routine); + routine = routineRes; triggers = triggersRes.triggers ?? []; } catch { routine = null; diff --git a/apps/web/app/office/routines/[id]/routine-detail-view.test.tsx b/apps/web/app/office/routines/[id]/routine-detail-view.test.tsx index a73ca91f60..e96c94ed3f 100644 --- a/apps/web/app/office/routines/[id]/routine-detail-view.test.tsx +++ b/apps/web/app/office/routines/[id]/routine-detail-view.test.tsx @@ -1,14 +1,23 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { cleanup, fireEvent, render, screen, within } from "@testing-library/react"; +import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; import { StateProvider } from "@/components/state-provider"; import { defaultOfficeState } from "@/lib/state/slices/office/office-slice"; import type { Routine, RoutineTrigger } from "@/lib/state/slices/office/types"; import { RoutineDetailView } from "./routine-detail-view"; +import { reconcileCronTrigger } from "./cron-reconcile"; +import { + OfficeTopbarChromeProvider, + useOfficeTopbarChrome, +} from "../../components/office-topbar-context"; + +const refreshMock = vi.hoisted(() => vi.fn()); vi.mock("@/lib/routing/client-router", () => ({ - useRouter: () => ({ refresh: vi.fn(), push: vi.fn(), replace: vi.fn() }), + useRouter: () => ({ refresh: refreshMock, push: vi.fn(), replace: vi.fn() }), })); +vi.mock("sonner", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + vi.mock("@/lib/api/domains/office-api", async () => { const actual = await vi.importActual( "@/lib/api/domains/office-api", @@ -20,6 +29,18 @@ vi.mock("@/lib/api/domains/office-api", async () => { }; }); +vi.mock("./cron-reconcile", async () => { + const actual = await vi.importActual("./cron-reconcile"); + return { + ...actual, + reconcileCronTrigger: vi.fn(), + }; +}); + +import { toast } from "sonner"; + +const reconcileCronTriggerMock = vi.mocked(reconcileCronTrigger); + afterEach(() => { cleanup(); vi.clearAllMocks(); @@ -51,8 +72,29 @@ const cronTrigger: RoutineTrigger = { updatedAt: TIMESTAMP, }; +const relistedCronTrigger: RoutineTrigger = { + id: "trigger-2", + routineId: "routine-1", + kind: "cron", + cronExpression: "*/10 * * * *", + timezone: "UTC", + nextRunAt: "2026-06-06T00:00:00Z", + enabled: true, + createdAt: TIMESTAMP, + updatedAt: TIMESTAMP, +}; + const NO_TRIGGERS: RoutineTrigger[] = []; +// Save/Run Now are contributed through useOfficeTopbar's actions slot (a +// context the office shell's topbar renders elsewhere), not RoutineDetailView's +// own return tree, so a direct render needs the provider plus a consumer that +// actually displays the slot for the buttons to be reachable at all. +function TopbarActions() { + const chrome = useOfficeTopbarChrome(); + return <>{chrome?.actions}; +} + function renderDetailView(routine: Routine, triggers: RoutineTrigger[]) { return render( - + + + + , ); } @@ -90,6 +135,12 @@ describe("RoutineDetailView next-fire display", () => { renderDetailView({ ...baseRoutine, status: "paused" }, [cronTrigger]); expect(screen.getByText("Next fire: -")).toBeTruthy(); }); + + it("shows no next-fire countdown for a routine loaded with an unrecognized status", () => { + renderDetailView({ ...baseRoutine, status: "draft" }, [cronTrigger]); + expect(screen.getByText("Next fire: -")).toBeTruthy(); + expect(screen.queryByText(/Next fire: 5\/5\/2026/)).toBeNull(); + }); }); // AC-OFFICE-ROUTINE-CATCHUP-003.8: the catch_up_max control renders under @@ -143,3 +194,252 @@ describe("RoutineDetailView catch-up policy labeling (AC-003.6)", () => { expect(option.textContent).toMatch(/once|single/i); }); }); + +describe("RoutineDetailView catch-up max editing", () => { + it("keeps the raw value while editing and coerces it at save time", async () => { + const { updateRoutine } = await import("@/lib/api/domains/office-api"); + reconcileCronTriggerMock.mockResolvedValue({ kind: "unchanged" }); + renderDetailView( + { ...baseRoutine, catchUpPolicy: "summarize_missed", catchUpMax: 25 }, + NO_TRIGGERS, + ); + + const input = screen.getByRole("spinbutton") as HTMLInputElement; + fireEvent.change(input, { target: { value: "" } }); + expect(input.value).toBe(""); + fireEvent.change(input, { target: { value: "7" } }); + expect(input.value).toBe("7"); + + fireEvent.click(screen.getByRole("button", { name: /save/i })); + + await waitFor(() => + expect(updateRoutine).toHaveBeenCalledWith( + "routine-1", + expect.objectContaining({ catchUpMax: 7 }), + ), + ); + }); +}); + +// AC-003.17: a stored status outside the three selectable options renders no +// selection (the control does not invent a value) and a save omits the field +// rather than sending an invented one. +describe("RoutineDetailView draft status normalization (AC-003.17)", () => { + it("shows no selected status option when the stored status is not one of the three known values", () => { + renderDetailView({ ...baseRoutine, status: "draft" }, NO_TRIGGERS); + const statusField = screen.getByText("Status").closest("div") as HTMLElement; + const statusCombobox = within(statusField).getByRole("combobox"); + expect(statusCombobox.textContent).toBe(""); + }); + + it("keeps the known status selected when it is one of the three options", () => { + renderDetailView({ ...baseRoutine, status: "paused" }, NO_TRIGGERS); + const statusField = screen.getByText("Status").closest("div") as HTMLElement; + expect(within(statusField).getByText("Paused")).toBeTruthy(); + }); + + it("omits status from the save patch when the stored status was unrecognized", async () => { + const { updateRoutine } = await import("@/lib/api/domains/office-api"); + reconcileCronTriggerMock.mockResolvedValue({ kind: "unchanged" }); + renderDetailView({ ...baseRoutine, status: "draft" }, NO_TRIGGERS); + + fireEvent.click(screen.getByRole("button", { name: /save/i })); + + await waitFor(() => { + expect(updateRoutine).toHaveBeenCalledWith( + "routine-1", + expect.objectContaining({ status: undefined }), + ); + }); + }); +}); + +// TS-005: a rejected updateRoutine must not fall through into a cron +// reconcile attempt against a routine whose own field save just failed, and +// must re-enable the Save button so the operator can retry. +describe("RoutineDetailView save failure (TS-005)", () => { + it("shows the server error and does not attempt a cron reconcile when updateRoutine rejects", async () => { + const { updateRoutine } = await import("@/lib/api/domains/office-api"); + vi.mocked(updateRoutine).mockRejectedValueOnce(new Error("name already exists")); + renderDetailView(baseRoutine, NO_TRIGGERS); + + const saveButton = screen.getByRole("button", { name: /save/i }); + fireEvent.click(saveButton); + + await waitFor(() => expect(toast.error).toHaveBeenCalledWith("name already exists")); + expect(reconcileCronTriggerMock).not.toHaveBeenCalled(); + expect(toast.success).not.toHaveBeenCalled(); + expect((saveButton as HTMLButtonElement).disabled).toBe(false); + expect(screen.getByRole("button", { name: /save/i }).textContent).not.toMatch(/saving/i); + }); + + it("shows an error toast and re-enables Save if reconcileCronTrigger throws instead of resolving with a failure outcome", async () => { + // reconcileCronTrigger is designed to always resolve (every internal call + // is its own try/catch), but nothing in its type enforces that. Save must + // still catch it, toast, and clear the saving flag if a future change + // breaks that invariant, rather than leaving an unhandled rejection and a + // permanently-disabled button. + reconcileCronTriggerMock.mockRejectedValueOnce(new Error("unexpected")); + renderDetailView(baseRoutine, NO_TRIGGERS); + + const saveButton = screen.getByRole("button", { name: /save/i }); + fireEvent.click(saveButton); + + await waitFor(() => expect(toast.error).toHaveBeenCalledWith("unexpected")); + expect((screen.getByRole("button", { name: /save/i }) as HTMLButtonElement).disabled).toBe( + false, + ); + }); +}); + +// AC-002.9 / SRF-34: describeCronOutcome (through handleSave) must map every +// CronReconcileOutcome branch to the correct toast kind, message, and +// refresh decision — including the SRF-34 uncovered cell (a failed trigger +// mutation whose re-list also fails), which must show the fate-unknown +// message rather than AC-002.9's "saved but schedule was not" text. +describe("RoutineDetailView stale schedule state", () => { + it("blocks another save after a successful schedule mutation has an unknown trigger state", async () => { + const { updateRoutine } = await import("@/lib/api/domains/office-api"); + reconcileCronTriggerMock.mockResolvedValueOnce({ kind: "success", triggers: null }); + renderDetailView(baseRoutine, [cronTrigger]); + + fireEvent.click(screen.getByRole("button", { name: /save/i })); + + await waitFor(() => + expect(toast.success).toHaveBeenCalledWith( + "The routine was saved. The displayed schedule may be stale until you reload the page.", + ), + ); + + fireEvent.click(screen.getByRole("button", { name: /save/i })); + + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + "The routine was saved, but the schedule's fate is unknown. Reload the page to see the current state.", + ), + ); + expect(reconcileCronTriggerMock).toHaveBeenCalledTimes(1); + expect(updateRoutine).toHaveBeenCalledTimes(1); + }); +}); + +describe("RoutineDetailView save outcome mapping (AC-002.9, SRF-34)", () => { + function clickSave() { + fireEvent.click(screen.getByRole("button", { name: /save/i })); + } + + it("shows the plain saved toast and refreshes when the cron draft is unchanged", async () => { + reconcileCronTriggerMock.mockResolvedValue({ kind: "unchanged" }); + renderDetailView(baseRoutine, NO_TRIGGERS); + + clickSave(); + + await waitFor(() => expect(toast.success).toHaveBeenCalledWith("Routine saved")); + expect(toast.error).not.toHaveBeenCalled(); + expect(refreshMock).toHaveBeenCalled(); + }); + + it("shows the plain saved toast and refreshes on a successful reconcile with a fresh trigger list", async () => { + reconcileCronTriggerMock.mockResolvedValue({ kind: "success", triggers: [] }); + renderDetailView(baseRoutine, NO_TRIGGERS); + + clickSave(); + + await waitFor(() => expect(toast.success).toHaveBeenCalledWith("Routine saved")); + expect(refreshMock).toHaveBeenCalled(); + }); + + it("shows the stale-schedule toast and does not refresh when a successful reconcile's re-list fails", async () => { + reconcileCronTriggerMock.mockResolvedValue({ kind: "success", triggers: null }); + renderDetailView(baseRoutine, NO_TRIGGERS); + + clickSave(); + + await waitFor(() => + expect(toast.success).toHaveBeenCalledWith( + "The routine was saved. The displayed schedule may be stale until you reload the page.", + ), + ); + expect(refreshMock).not.toHaveBeenCalled(); + }); + + it("shows the fate-unknown toast, not AC-002.9's saved-but-not text, when a failed create's re-list also fails (SRF-34)", async () => { + reconcileCronTriggerMock.mockResolvedValue({ + kind: "create-failed", + message: "invalid cron expression", + triggers: null, + }); + renderDetailView(baseRoutine, NO_TRIGGERS); + + clickSave(); + + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + "The routine was saved, but the schedule's fate is unknown. Reload the page to see the current state.", + ), + ); + expect(toast.success).not.toHaveBeenCalled(); + expect(refreshMock).not.toHaveBeenCalled(); + }); + + it("shows the fate-unknown toast when a failed delete's re-list also fails (SRF-34)", async () => { + reconcileCronTriggerMock.mockResolvedValue({ + kind: "delete-failed", + message: "server error", + triggers: null, + }); + renderDetailView(baseRoutine, NO_TRIGGERS); + + clickSave(); + + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + "The routine was saved, but the schedule's fate is unknown. Reload the page to see the current state.", + ), + ); + expect(toast.success).not.toHaveBeenCalled(); + }); + + it("names the create failure, does not refresh, and renders the re-listed schedule when the re-list succeeds", async () => { + reconcileCronTriggerMock.mockResolvedValue({ + kind: "create-failed", + message: "invalid cron expression", + triggers: [relistedCronTrigger], + }); + renderDetailView(baseRoutine, [cronTrigger]); + + clickSave(); + + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + "The routine was saved, but the schedule change failed: invalid cron expression", + ), + ); + expect(refreshMock).not.toHaveBeenCalled(); + // Proves setTriggers(outcome.triggers) actually reaches the DOM: the + // Schedule card must show the re-listed trigger's next fire, not the + // pre-save trigger it replaced. + expect(screen.getByText(/Next fire: 6\/6\/2026/)).toBeTruthy(); + expect(screen.queryByText(/Next fire: 5\/5\/2026/)).toBeNull(); + }); + + it("names the delete failure, the surviving cron count, and renders the re-listed schedule when the re-list succeeds", async () => { + reconcileCronTriggerMock.mockResolvedValue({ + kind: "delete-failed", + message: "server error", + triggers: [relistedCronTrigger], + }); + renderDetailView(baseRoutine, [cronTrigger]); + + clickSave(); + + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + "The routine was saved, but removing the old schedule failed: server error. 1 cron schedule is now active.", + ), + ); + expect(refreshMock).not.toHaveBeenCalled(); + expect(screen.getByText(/Next fire: 6\/6\/2026/)).toBeTruthy(); + expect(screen.queryByText(/Next fire: 5\/5\/2026/)).toBeNull(); + }); +}); diff --git a/apps/web/app/office/routines/[id]/routine-detail-view.tsx b/apps/web/app/office/routines/[id]/routine-detail-view.tsx index 03254c5693..a67c7f34e9 100644 --- a/apps/web/app/office/routines/[id]/routine-detail-view.tsx +++ b/apps/web/app/office/routines/[id]/routine-detail-view.tsx @@ -12,18 +12,21 @@ import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@ import { toast } from "@/lib/toast/sonner"; import { useAppStore } from "@/components/state-provider"; import { selectOfficeAgentProfiles } from "@/lib/state/slices/office/selectors"; -import { - updateRoutine, - runRoutine, - createRoutineTrigger, - deleteRoutineTrigger, -} from "@/lib/api/domains/office-api"; -import type { Routine, RoutineTrigger } from "@/lib/state/slices/office/types"; +import { updateRoutine, runRoutine } from "@/lib/api/domains/office-api"; +import type { + Routine, + RoutineStatus, + RoutineTrigger, + UpdateRoutinePatch, +} from "@/lib/state/slices/office/types"; import { timeAgo } from "@/lib/utils/time"; import { useOfficeTopbar } from "../../components/office-topbar-context"; import { isRoutineFiring } from "../../lib/routine-status"; import { routineNotFiringMessage } from "../../lib/routine-not-firing"; +import { coerceCatchUpMax } from "../../lib/catch-up-max"; import { useTranslation } from "react-i18next"; +import type { TFunction } from "i18next"; +import { reconcileCronTrigger, type CronReconcileOutcome } from "./cron-reconcile"; // Lift the form state out of the component so the file stays under the // 100-line per-function ceiling and the helpers can render typed slices @@ -31,16 +34,24 @@ import { useTranslation } from "react-i18next"; type DraftState = { name: string; description: string; - status: "active" | "paused" | "archived"; + // Undefined means the stored status is not one of the three selectable + // options; the control shows no selection and a save omits the field. + status: RoutineStatus | undefined; assigneeAgentProfileId: string; concurrencyPolicy: string; catchUpPolicy: string; - catchUpMax: number; + catchUpMax: string; triggerKind: "cron" | "webhook"; cronExpression: string; timezone: string; }; +const ROUTINE_STATUSES: RoutineStatus[] = ["active", "paused", "archived"]; + +function normalizeDraftStatus(status: string): RoutineStatus | undefined { + return (ROUTINE_STATUSES as string[]).includes(status) ? (status as RoutineStatus) : undefined; +} + function pickTriggerKind(triggers: RoutineTrigger[]): "cron" | "webhook" { const cron = triggers.find((t) => t.kind === "cron"); if (cron) return "cron"; @@ -55,17 +66,63 @@ function buildDraft(routine: Routine, triggers: RoutineTrigger[]): DraftState { return { name: routine.name, description: routine.description ?? "", - status: (routine.status as DraftState["status"]) ?? "active", + status: normalizeDraftStatus(routine.status), assigneeAgentProfileId: routine.assigneeAgentProfileId ?? "", concurrencyPolicy: routine.concurrencyPolicy ?? "coalesce_if_active", catchUpPolicy: routine.catchUpPolicy ?? "summarize_missed", - catchUpMax: routine.catchUpMax ?? 25, + catchUpMax: String(coerceCatchUpMax(routine.catchUpMax)), triggerKind, cronExpression: cron?.cronExpression ?? "", timezone: cron?.timezone ?? "UTC", }; } +function buildUpdatePatch(draft: DraftState): UpdateRoutinePatch { + return { + name: draft.name, + description: draft.description, + status: draft.status, + assigneeAgentProfileId: draft.assigneeAgentProfileId, + concurrencyPolicy: draft.concurrencyPolicy, + catchUpPolicy: draft.catchUpPolicy, + catchUpMax: coerceCatchUpMax(draft.catchUpMax), + }; +} + +function describeCronOutcome( + outcome: CronReconcileOutcome, + t: TFunction, +): { toastKind: "success" | "error"; message: string; refresh: boolean } { + if (outcome.kind === "unchanged") { + return { toastKind: "success", message: t("office:routineSaved"), refresh: true }; + } + if (outcome.kind === "success") { + return outcome.triggers === null + ? { + toastKind: "success", + message: t("office:routineSavedScheduleMayBeStale"), + refresh: false, + } + : { toastKind: "success", message: t("office:routineSaved"), refresh: true }; + } + if (outcome.triggers === null) { + return { toastKind: "error", message: t("office:routineScheduleFateUnknown"), refresh: false }; + } + if (outcome.kind === "create-failed") { + return { + toastKind: "error", + message: t("office:routineSavedScheduleChangeFailed", { error: outcome.message }), + refresh: false, + }; + } + const count = outcome.triggers.filter((trigger) => trigger.kind === "cron").length; + return { + toastKind: "error", + message: t("office:routineSavedScheduleDeleteFailed", { error: outcome.message, count }), + refresh: false, + }; +} + type RoutineDetailViewProps = { initialRoutine: Routine; initialTriggers: RoutineTrigger[]; @@ -76,7 +133,7 @@ export function RoutineDetailView({ initialRoutine, initialTriggers }: RoutineDe const router = useRouter(); const agents = useAppStore(selectOfficeAgentProfiles); const [routine] = useState(initialRoutine); - const [triggers, setTriggers] = useState(initialTriggers); + const [triggers, setTriggers] = useState(initialTriggers); const [draft, setDraft] = useState(buildDraft(initialRoutine, initialTriggers)); const [saving, setSaving] = useState(false); const update = useCallback( @@ -84,31 +141,44 @@ export function RoutineDetailView({ initialRoutine, initialTriggers }: RoutineDe [], ); - const cronTrigger = triggers.find((t) => t.kind === "cron"); + const cronTrigger = triggers?.find((t) => t.kind === "cron"); const lastFired = cronTrigger?.lastFiredAt ?? null; const handleSave = useCallback(async () => { + if (triggers === null) { + toast.error(t("office:routineScheduleFateUnknown")); + return; + } setSaving(true); try { - await updateRoutine(routine.id, { - name: draft.name, - description: draft.description, - status: draft.status, - assigneeAgentProfileId: draft.assigneeAgentProfileId, - concurrencyPolicy: draft.concurrencyPolicy, - catchUpPolicy: draft.catchUpPolicy, - catchUpMax: draft.catchUpMax, - } as Record); - const nextTriggers = await syncCronTrigger(routine.id, draft, triggers); - setTriggers(nextTriggers); - toast.success(t("office:routineSaved")); - router.refresh(); + await updateRoutine(routine.id, buildUpdatePatch(draft)); } catch (err) { toast.error(err instanceof Error ? err.message : t("office:failedToSaveRoutine")); + setSaving(false); + return; + } + + try { + const outcome = await reconcileCronTrigger(routine.id, draft, triggers); + if (outcome.kind !== "unchanged") setTriggers(outcome.triggers); + const result = describeCronOutcome(outcome, t); + if (result.toastKind === "success") { + toast.success(result.message); + } else { + toast.error(result.message); + } + if (result.refresh) router.refresh(); + } catch (err) { + // reconcileCronTrigger is designed to always resolve (every internal + // call is its own try/catch) rather than throw, but nothing enforces + // that contract. Catching here — not just the `finally` below — keeps + // a future violation from becoming an unhandled rejection out of an + // unawaited click handler on top of a stuck Save button. + toast.error(err instanceof Error ? err.message : t("office:failedToSaveRoutine")); } finally { setSaving(false); } - }, [routine.id, draft, triggers, router]); + }, [routine.id, draft, triggers, router, t]); const handleRunNow = useCallback(async () => { try { @@ -143,7 +213,9 @@ export function RoutineDetailView({ initialRoutine, initialTriggers }: RoutineDe ); @@ -174,7 +246,8 @@ function DetailGeneralCard({ type="number" min={1} value={draft.catchUpMax} - onChange={(e) => update({ catchUpMax: Number(e.target.value) || 25 })} + onChange={(e) => update({ catchUpMax: e.target.value })} + onBlur={(e) => update({ catchUpMax: String(coerceCatchUpMax(e.target.value)) })} /> )} @@ -221,7 +294,7 @@ function StatusAndAssigneeFields({