diff --git a/apps/web/src/shell/definition-target-picker.tsx b/apps/web/src/shell/definition-target-picker.tsx new file mode 100644 index 000000000..fc66e4600 --- /dev/null +++ b/apps/web/src/shell/definition-target-picker.tsx @@ -0,0 +1,210 @@ +// An explicit picker over `GET /api/tenants/:tenantId/workflows/targets` +// (CL-7355): the routine panel used to infer a create target from the +// conversation's own agent (`listWorkbenchAgents(...)[0]`) — silently wrong +// the moment a workbench hosted more than one agent, or none. This picker +// instead lists every deployed, frozen definition the signed-in principal +// may target, grouped presentationally by `kind` (Agents / Workflows), and +// leaves the choice to the person — it never auto-selects the first item. +import { useEffect, useState } from "react"; +import { Button, EmptyState, Select, Skeleton } from "@corbits/react-ui"; +import { Robot } from "@corbits/icons"; + +import { useNavigate } from "../navigation"; +import { listAllRoutineTargets } from "../routines-api"; +import type { RoutineTarget } from "../routines-api"; + +type LoadState = + | { readonly kind: "loading" } + | { readonly kind: "error"; readonly message: string } + | { readonly kind: "loaded"; readonly targets: readonly RoutineTarget[] }; + +/** `value`/`onChange` carry a `definitionAssetId` — the stable identity a + * routine stores. `preselectedAssetId` lets a caller that already knows + * which target it wants seed the initial selection explicitly; the picker + * itself never guesses one on the person's behalf. */ +export function DefinitionTargetPicker({ + tenantId, + value, + onChange, + preselectedAssetId, +}: { + readonly tenantId: string | null; + readonly value: string | null; + readonly onChange: (definitionAssetId: string) => void; + readonly preselectedAssetId?: string; +}) { + const navigate = useNavigate(); + const [state, setState] = useState({ kind: "loading" }); + // Bumping this re-runs the load effect below without depending on + // `tenantId` changing — the retry button's only job. + const [retryTick, setRetryTick] = useState(0); + + // Deps are `[tenantId, retryTick]` only, not `[tenantId, value, onChange, + // preselectedAssetId]` — this picker has exactly one caller today + // (`RoutinePanel`), which remounts on subject change and passes a stable + // `onChange` (`useState`+ref, per its own comment), so a stale closure + // over those three can't happen in practice. If a second caller ever + // reuses this component without remounting on `preselectedAssetId` + // changes, add it to this array (and accept the extra re-fetch that + // implies) rather than relying on this invariant silently. + useEffect(() => { + if (tenantId === null) return; + let cancelled = false; + setState({ kind: "loading" }); + void listAllRoutineTargets(tenantId).then( + (targets) => { + if (cancelled) return; + setState({ kind: "loaded", targets }); + if ( + preselectedAssetId !== undefined && + value === null && + targets.some((t) => t.definitionAssetId === preselectedAssetId) + ) { + onChange(preselectedAssetId); + } + }, + (cause: unknown) => { + if (!cancelled) { + setState({ + kind: "error", + message: cause instanceof Error ? cause.message : String(cause), + }); + } + }, + ); + return () => { + cancelled = true; + }; + // eslint-disable-next-line react-hooks/exhaustive-deps -- see comment above + }, [tenantId, retryTick]); + + if (state.kind === "loading") { + return ( +
+ + What should this routine run? + + +
+ ); + } + + if (state.kind === "error") { + return ( +
+ + What should this routine run? + +

+ {state.message} +

+ +
+ ); + } + + const { targets } = state; + + if (targets.length === 0) { + return ( + } + title="No deployable workflows yet — author or install one" + action={ + + } + /> + ); + } + + const nameCounts = new Map(); + for (const target of targets) { + nameCounts.set(target.name, (nameCounts.get(target.name) ?? 0) + 1); + } + const labelFor = (target: RoutineTarget): string => + (nameCounts.get(target.name) ?? 0) > 1 + ? `${target.name} (${target.assetName})` + : target.name; + + const agents = targets.filter((t) => t.kind === "agent"); + const workflows = targets.filter((t) => t.kind === "workflow"); + const selected = targets.find((t) => t.definitionAssetId === value) ?? null; + const stale = value !== null && selected === null; + + return ( +
+ + + {stale ? ( +

+ This routine's target is no longer available — pick a new one. +

+ ) : selected?.description !== null && + selected?.description !== undefined ? ( +

+ {selected.description} +

+ ) : null} +
+ ); +} diff --git a/apps/web/src/shell/routine-panel.tsx b/apps/web/src/shell/routine-panel.tsx index 20a3dc100..e48785faa 100644 --- a/apps/web/src/shell/routine-panel.tsx +++ b/apps/web/src/shell/routine-panel.tsx @@ -20,16 +20,15 @@ // picking a schedule while another field's save is in flight keeps the // in-progress values. // -// A routine created from this panel always targets the conversation it -// was opened beside: every workbench's host participant is Myra, so "this -// workbench's own agent" resolves to that workbench's host agent -// (`listWorkbenchAgents`), and the routine delivers back into that same -// workbench — never a new one. A panel opened with no workbench in scope (a -// deliberate `/routines` visit) falls back to the workbench's own default -// Myra workbench (`ensureMyraWorkbench`, the one deliberate find-or-create -// path in the product) rather than the old tenant-wide "assistant" -// definition lookup, which had no workbench to deliver into at all and -// silently minted a fresh one server-side whenever delivery was required. +// The routine's delivery destination is the conversation this panel was +// opened beside — its own id is where the routine delivers back into. A +// panel opened with no workbench in scope (a deliberate `/routines` visit) +// falls back to the workbench's own default Myra workbench +// (`ensureMyraWorkbench`, the one deliberate find-or-create path in the +// product). What the routine *runs* is a separate, explicit choice (CL-7355): +// the panel no longer infers it from the conversation's own agent — a +// person picks a target from `DefinitionTargetPicker`, backed by +// `GET /api/tenants/:tenantId/workflows/targets`. import { useEffect, useRef, useState } from "react"; import type { ChangeEvent } from "react"; import { useQueryClient } from "@tanstack/react-query"; @@ -47,25 +46,32 @@ import { Textarea, toast, } from "@corbits/react-ui"; -import { listWorkbenchAgents } from "@corbits/chat-ui"; import { Clock, X } from "@corbits/icons"; +import { reportError } from "@corbits/error-sink"; import { useBench } from "../bench-context"; import { useNavigate } from "../navigation"; import { ensureMyraWorkbench } from "../myra-workbench"; import { routineScheduleSentence } from "@corbits/routines/client"; import { ScheduleEditor } from "../routine-schedule"; +import { DefinitionTargetPicker } from "./definition-target-picker"; import { createRoutine, deleteRoutine, getRoutine, + listAllRoutineTargets, listRoutineRuns, routineCreatedToast, routineRunStartedToast, runRoutineNow, updateRoutine, } from "../routines-api"; -import type { Routine, RoutineRun, RoutineTrigger } from "../routines-api"; +import type { + Routine, + RoutineRun, + RoutineTarget, + RoutineTrigger, +} from "../routines-api"; import { RunsTable } from "../pages/routines-page"; import { createWebhookTrigger, @@ -177,11 +183,6 @@ export function RoutinePanel() { type SaveState = "idle" | "saving" | "saved" | "error"; -type CreateTarget = { - readonly definitionAssetId: string; - readonly deliveryWorkbenchId: string; -}; - /** The editor view: create/edit one routine. Self-fetching — handed only * the subject, loads the rest itself, exactly like `ProfileCanvasPane`. */ function RoutineEditorPanel({ @@ -222,6 +223,15 @@ function RoutineEditorPanel({ const [saveState, setSaveState] = useState("idle"); const [error, setError] = useState(null); const [runs, setRuns] = useState([]); + const [targetAssetId, setTargetAssetId] = useState(null); + // Set when a field blur bails out because no target is picked yet on a + // brand-new routine — a silent `return` there gives the person no + // feedback that their edit didn't save, so this drives an inline hint + // under the picker instead. + const [needsTargetHint, setNeedsTargetHint] = useState(false); + const [existingTarget, setExistingTarget] = useState( + null, + ); // Every write (create or update) this panel session makes runs through // this one chain — never two in flight at once. `routineIdRef` is the @@ -229,6 +239,8 @@ function RoutineEditorPanel({ // resolves, so a second commit queued before the first finished sees the // *post*-first-write id, not a stale snapshot from before either ran. const routineIdRef = useRef(routineId); + const targetAssetIdRef = useRef(targetAssetId); + targetAssetIdRef.current = targetAssetId; const writeChainRef = useRef>(Promise.resolve()); const draftRef = useRef({ name: "", @@ -270,6 +282,8 @@ function RoutineEditorPanel({ setSaveState("idle"); setError(null); setRuns([]); + setTargetAssetId(null); + setExistingTarget(null); }, [subject]); const loadRuns = (id: string) => { @@ -292,6 +306,7 @@ function RoutineEditorPanel({ setTrigger(routine.trigger); setSavedTrigger(routine.trigger); setEnabled(routine.enabled); + setTargetAssetId(routine.definitionAssetId); draftRef.current = { name: routine.name, savedName: routine.name, @@ -338,7 +353,15 @@ function RoutineEditorPanel({ }; // Loads an existing routine's real fields once the tenant resolves — - // mirrors `ProfileCanvasPane`'s own "fetch once open" effect. + // mirrors `ProfileCanvasPane`'s own "fetch once open" effect. The + // existing-routine target's display name (read-only for this issue — + // editing it is CL-7358; a stale target, no longer in the tenant's + // deployable list, is shown honestly as its raw id rather than hidden) + // is fetched here too, sequenced after the routine itself resolves and + // keyed on the just-loaded `routine.definitionAssetId` directly, not + // component state — a separate effect keyed on `targetAssetId` would + // both re-fire an identical fetch right after `hydrateRoutine` sets it + // (no new information) and race it if the two fetches ran in parallel. useEffect(() => { if (tenantId === null || subject.routineId == null) return; let cancelled = false; @@ -347,6 +370,24 @@ function RoutineEditorPanel({ if (cancelled) return; hydrateRoutine(routine); loadRuns(routine.id); + void listAllRoutineTargets(tenantId).then( + (targets) => { + if (cancelled) return; + setExistingTarget( + targets.find( + (t) => t.definitionAssetId === routine.definitionAssetId, + ) ?? null, + ); + }, + (cause: unknown) => { + if (cancelled) return; + reportError(cause, { + operation: "routine-panel.load_existing_target", + tenantId, + }); + setExistingTarget(null); + }, + ); }, (cause: unknown) => { if (!cancelled) { @@ -359,37 +400,20 @@ function RoutineEditorPanel({ }; }, [tenantId, subject.routineId]); - /** This routine's own agent + delivery workbench: the conversation the - * panel was opened beside (its host participant — every workbench's - * host is Myra), or, with no conversation in scope, this workbench's - * own default Myra workbench. Never mints a new workbench — `ensureMyraWorkbench` - * finds-or-creates the one singleton Myra conversation this tenant - * already has. */ - const resolveCreateTarget = async (): Promise => { + /** This routine's delivery workbench: the conversation the panel was + * opened beside (its own id), or, with no conversation in scope, this + * workbench's own default Myra workbench. Never mints a new workbench — + * `ensureMyraWorkbench` finds-or-creates the one singleton Myra + * conversation this tenant already has. Delivery is independent of what + * the routine runs (CL-7355) — that's `targetAssetId`, picked explicitly. */ + const resolveDeliveryWorkbenchId = async (): Promise => { if (tenantId === null) { throw new Error("No workbench to create this in yet"); } - if (subject.workbenchId !== undefined) { - const workbenchId = subject.workbenchId; - const agents = await listWorkbenchAgents(tenantId, workbenchId); - const definitionAssetId = agents[0]?.definitionAssetId; - if (definitionAssetId === undefined) { - throw new Error( - "This conversation has no agent to run this routine yet.", - ); - } - return { definitionAssetId, deliveryWorkbenchId: workbenchId }; - } + if (subject.workbenchId !== undefined) return subject.workbenchId; const result = await ensureMyraWorkbench(tenantId); if (result.kind === "error") throw new Error(result.message); - const agents = await listWorkbenchAgents(tenantId, result.workbenchId); - const definitionAssetId = agents[0]?.definitionAssetId; - if (definitionAssetId === undefined) { - throw new Error( - "This workbench has no assistant to run this routine yet.", - ); - } - return { definitionAssetId, deliveryWorkbenchId: result.workbenchId }; + return result.workbenchId; }; /** Every create/update this panel makes funnels through this one chain — @@ -421,11 +445,15 @@ function RoutineEditorPanel({ readonly instruction: string; readonly trigger: RoutineTrigger; }): Promise => { - const target = await resolveCreateTarget(); + const definitionAssetId = targetAssetIdRef.current; + if (definitionAssetId === null) { + throw new Error("Pick what this routine runs before saving."); + } + const deliveryWorkbenchId = await resolveDeliveryWorkbenchId(); const routine = await createRoutine(tenantId as string, { name: fields.name, - definitionAssetId: target.definitionAssetId, - deliveryWorkbenchId: target.deliveryWorkbenchId, + definitionAssetId, + deliveryWorkbenchId, scope: "personal", trigger: fields.trigger, runOnceNow: false, @@ -447,10 +475,17 @@ function RoutineEditorPanel({ // write's result, so it correctly updates instead. The bail-outs below // (blank name, unchanged value) are safe to read eagerly — being a tick // stale there costs at most one redundant PATCH, never a second POST. + // A brand-new routine requires a picked target before its first write can + // fire at all (CL-7355) — a blur with no target selected yet is not a + // failed save, it's simply not ready to submit. const commitName = () => { const trimmed = name.trim(); if (trimmed === "") return; if (routineIdRef.current !== null && trimmed === savedName) return; + if (routineIdRef.current === null && targetAssetIdRef.current === null) { + setNeedsTargetHint(true); + return; + } void runWrite((id) => { if (id === null) return doCreate({ name: trimmed, instruction, trigger }); return updateRoutine(tenantId as string, id, { name: trimmed }); @@ -461,6 +496,10 @@ function RoutineEditorPanel({ const trimmed = instruction.trim(); if (routineIdRef.current === null && name.trim() === "") return; if (routineIdRef.current !== null && trimmed === savedInstruction) return; + if (routineIdRef.current === null && targetAssetIdRef.current === null) { + setNeedsTargetHint(true); + return; + } void runWrite((id) => { if (id === null) { return doCreate({ name: name.trim(), instruction, trigger }); @@ -476,6 +515,10 @@ function RoutineEditorPanel({ draftRef.current.trigger = next; setTriggerSourceLabel(null); setAddingSchedule(false); + if (routineIdRef.current === null && targetAssetIdRef.current === null) { + setNeedsTargetHint(true); + return; + } void runWrite((id) => { if (id === null) { return doCreate({ @@ -488,6 +531,16 @@ function RoutineEditorPanel({ }); }; + // Picking a target never itself submits — it only clears the "no target + // yet" bail-out so the next field blur (Name/Instruction) can create. + // `targetAssetIdRef` is set synchronously here so a blur landing in the + // very same tick already sees the pick, not a stale pre-render snapshot. + const pickTarget = (definitionAssetId: string) => { + targetAssetIdRef.current = definitionAssetId; + setTargetAssetId(definitionAssetId); + setNeedsTargetHint(false); + }; + const removeTrigger = () => { setTrigger(null); draftRef.current.trigger = null; @@ -508,23 +561,19 @@ function RoutineEditorPanel({ let targetRoutineId = id; let definitionAssetId: string; if (targetRoutineId === null) { - const target = await resolveCreateTarget(); - const created = await createRoutine(tenantId, { + const created = await doCreate({ name: name.trim() || "Untitled routine", - definitionAssetId: target.definitionAssetId, - deliveryWorkbenchId: target.deliveryWorkbenchId, - scope: "personal", + instruction, trigger: null, - runOnceNow: false, - ...(instruction.trim() !== "" - ? { input: { instruction: instruction.trim() } } - : {}), }); targetRoutineId = created.id; definitionAssetId = created.definitionAssetId; - toast(routineCreatedToast(created.name)); } else { - definitionAssetId = (await resolveCreateTarget()).definitionAssetId; + const existing = targetAssetIdRef.current; + if (existing === null) { + throw new Error("Pick what this routine runs before saving."); + } + definitionAssetId = existing; } const binding = await createWebhookTrigger(tenantId, { name: `${name.trim() || "Untitled routine"} — ${sourceLabel}`, @@ -628,6 +677,28 @@ function RoutineEditorPanel({ /> + {routineId === null ? ( +
+ + {needsTargetHint && targetAssetId === null ? ( +

+ Pick what this routine runs before the rest can save. +

+ ) : null} +
+ ) : ( +
+ What this routine runs +
+ {existingTarget?.name ?? targetAssetId ?? "—"} +
+
+ )} +