diff --git a/apps/web/src/shell/definition-target-picker.tsx b/apps/web/src/shell/definition-target-picker.tsx index fc66e460..9d80cfa2 100644 --- a/apps/web/src/shell/definition-target-picker.tsx +++ b/apps/web/src/shell/definition-target-picker.tsx @@ -21,17 +21,23 @@ type LoadState = /** `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. */ + * itself never guesses one on the person's behalf. `onStaleChange` reports + * whether `value` is currently absent from the loaded target list (CL-7358) + * — a caller that needs to gate another affordance (e.g. disabling + * "Run now" on an edited routine's picker) on that without refetching or + * re-deriving the target list itself. */ export function DefinitionTargetPicker({ tenantId, value, onChange, preselectedAssetId, + onStaleChange, }: { readonly tenantId: string | null; readonly value: string | null; readonly onChange: (definitionAssetId: string) => void; readonly preselectedAssetId?: string; + readonly onStaleChange?: (stale: boolean) => void; }) { const navigate = useNavigate(); const [state, setState] = useState({ kind: "loading" }); @@ -51,6 +57,13 @@ export function DefinitionTargetPicker({ if (tenantId === null) return; let cancelled = false; setState({ kind: "loading" }); + // A tenant/bench switch invalidates whatever the previous tenant's + // target list said about `value`'s staleness immediately, rather + // than leaving `onStaleChange`'s last answer (about a different + // tenant's targets) standing until this reload completes — a narrow + // window where "Run now" could otherwise stay wrongly enabled or + // disabled. + onStaleChange?.(false); void listAllRoutineTargets(tenantId).then( (targets) => { if (cancelled) return; @@ -78,6 +91,28 @@ export function DefinitionTargetPicker({ // eslint-disable-next-line react-hooks/exhaustive-deps -- see comment above }, [tenantId, retryTick]); + // The one place "is `value` stale" is computed — both the effect that + // reports it to `onStaleChange` and the render below (the `stale` + // local) read this, so a future change to the staleness rule can't + // drift between the two. + const isTargetStale = ( + loaded: Extract, + candidate: string | null, + ): boolean => + candidate !== null && + !loaded.targets.some((t) => t.definitionAssetId === candidate); + + useEffect(() => { + if (state.kind !== "loaded") return; + // `onStaleChange` is read here but deliberately not in the deps + // array — see this component's doc comment: the single caller + // passes a stable reference. A future caller passing an inline + // arrow would need `onStaleChange` added here (and would then + // re-run this effect on every render of that caller). + onStaleChange?.(isTargetStale(state, value)); + // eslint-disable-next-line react-hooks/exhaustive-deps -- onStaleChange must be stable; see comment above + }, [state, value]); + if (state.kind === "loading") { return (
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; + const stale = isTargetStale(state, value); return (
diff --git a/apps/web/src/shell/routine-panel.tsx b/apps/web/src/shell/routine-panel.tsx index 66981e54..0ed33ac3 100644 --- a/apps/web/src/shell/routine-panel.tsx +++ b/apps/web/src/shell/routine-panel.tsx @@ -48,7 +48,6 @@ import { } from "@corbits/react-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"; @@ -59,19 +58,13 @@ import { createRoutine, deleteRoutine, getRoutine, - listAllRoutineTargets, listRoutineRuns, routineCreatedToast, routineRunStartedToast, runRoutineNow, updateRoutine, } from "../routines-api"; -import type { - Routine, - RoutineRun, - RoutineTarget, - RoutineTrigger, -} from "../routines-api"; +import type { Routine, RoutineRun, RoutineTrigger } from "../routines-api"; import { RunsTable } from "../pages/routines-page"; import { createWebhookTrigger, @@ -229,9 +222,7 @@ function RoutineEditorPanel({ // 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, - ); + const [targetStale, setTargetStale] = useState(false); // Every write (create or update) this panel session makes runs through // this one chain — never two in flight at once. `routineIdRef` is the @@ -283,7 +274,7 @@ function RoutineEditorPanel({ setError(null); setRuns([]); setTargetAssetId(null); - setExistingTarget(null); + setTargetStale(false); }, [subject]); const loadRuns = (id: string) => { @@ -354,14 +345,10 @@ function RoutineEditorPanel({ // Loads an existing routine's real fields once the tenant resolves — // 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. + // existing-routine target is now owned by `DefinitionTargetPicker` + // itself (CL-7358: editing a routine can retarget it), which reports + // staleness back via `onStaleChange` — this effect no longer fetches + // the target list a second time. useEffect(() => { if (tenantId === null || subject.routineId == null) return; let cancelled = false; @@ -370,24 +357,6 @@ 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) { @@ -531,14 +500,46 @@ 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. + // Picking a target reads `routineIdRef.current` itself — the same "is + // there a routine yet" decision `runWrite`'s own `task` closures make — + // rather than the panel branching create-vs-edit at the picker's call + // site (`routineId === null ? pickTarget : pickTargetForEdit`, the + // shape this replaced). On a brand-new routine (no id yet) 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 so a blur landing in the very + // same tick already sees the pick, not a stale pre-render snapshot. On + // an existing routine (CL-7358), picking a target autosaves immediately, + // like every other field, as a target-only PATCH — no other draft field + // rides along. A server rejection (404/409/403 — cross-tenant, unfrozen/ + // undeployed, forbidden; see `routineTargetRejection`) surfaces through + // the panel's normal inline error path and reverts the picker to the + // last-saved target, without touching any other unsaved input. const pickTarget = (definitionAssetId: string) => { + const id = routineIdRef.current; + if (id === null) { + targetAssetIdRef.current = definitionAssetId; + setTargetAssetId(definitionAssetId); + setNeedsTargetHint(false); + return; + } + const previous = targetAssetIdRef.current; targetAssetIdRef.current = definitionAssetId; setTargetAssetId(definitionAssetId); - setNeedsTargetHint(false); + void runWrite(() => + // This catch reverts ONLY `targetAssetIdRef`/`targetAssetId` — the + // two fields this PATCH ever sends. Do not fold another field's + // revert into this same catch if a future change widens this PATCH + // beyond target-only; give it its own previous/revert pair instead, + // the way this one is scoped to just the target. + updateRoutine(tenantId as string, id, { definitionAssetId }).catch( + (cause: unknown) => { + targetAssetIdRef.current = previous; + setTargetAssetId(previous); + throw cause; + }, + ), + ); }; const removeTrigger = () => { @@ -649,7 +650,7 @@ function RoutineEditorPanel({ { if (tenantId === null || routineId === null) return; return runRoutineNow(tenantId, routineId).then(() => { @@ -677,30 +678,22 @@ 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 ?? "—"} -
-
- )} +
+ + {needsTargetHint && targetAssetId === null ? ( +

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

+ ) : null} +