From 41c78160674b1831b95086c2e5f06c9a8c57a543 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 1 Sep 2026 23:30:28 -0700 Subject: [PATCH 1/4] Add tests for routine target editing in the routine panel (CL-7358) --- apps/web/test/routine-panel.test.tsx | 87 +++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 2 deletions(-) diff --git a/apps/web/test/routine-panel.test.tsx b/apps/web/test/routine-panel.test.tsx index f8a0c167a..a4916bc00 100644 --- a/apps/web/test/routine-panel.test.tsx +++ b/apps/web/test/routine-panel.test.tsx @@ -87,6 +87,7 @@ let targets: TargetFixture[] = [ }, ]; let targetsRequestFails = false; +let patchTargetRejection: { status: number; message: string } | null = null; let chatWorkbenches: Record[] = []; let runsByRoutineId: Record[]> = {}; let topLevelRuns: Record[] = []; @@ -233,6 +234,21 @@ async function routeFetch( const patchMatch = url.match(/\/routines\/([^/?]+)$/); if (patchMatch && method === "PATCH") { const patch: Record = JSON.parse(String(init?.body)); + if ( + patch["definitionAssetId"] !== undefined && + patchTargetRejection !== null + ) { + updatedPatches.push(patch); + return new Response( + JSON.stringify({ + error: { userMessage: patchTargetRejection.message }, + }), + { + status: patchTargetRejection.status, + headers: { "content-type": "application/json" }, + }, + ); + } updatedPatches.push(patch); createdRoutine = { ...(createdRoutine ?? routineRecord()), ...patch }; routines = routines.map((r) => @@ -295,6 +311,7 @@ describe("RoutinePanel", () => { capabilitiesProbeFails = false; networkDelayMs = 0; targetsRequestFails = false; + patchTargetRejection = null; targets = [ { definitionAssetId: "asset_myra", @@ -697,16 +714,82 @@ describe("RoutinePanel", () => { expect(updatedPatches).toContainEqual({ enabled: true }); }); - test("existing-routine mode shows the current target's name, read-only", async () => { + test("existing-routine mode shows the current target already selected in the same editable picker as create mode (CL-7358)", async () => { createdRoutine = routineRecord({ definitionAssetId: "asset_digest" }); routines = [createdRoutine]; await renderPanel({ routineId: "rtn_1" }); await settle(); - expect(container.querySelector("#routine-panel-target")).toBeNull(); + const select = container.querySelector( + "#routine-panel-target", + ) as HTMLSelectElement; + expect(select).not.toBeNull(); + expect(select.value).toBe("asset_digest"); expect(container.textContent).toContain("Morning digest workflow"); }); + test("retargeting an existing routine sends a target-only PATCH — no other field rides along", async () => { + createdRoutine = routineRecord({ definitionAssetId: "asset_myra" }); + routines = [createdRoutine]; + await renderPanel({ routineId: "rtn_1" }); + await settle(); + + selectTarget("asset_digest"); + await settle(); + + expect(updatedPatches).toContainEqual({ + definitionAssetId: "asset_digest", + }); + expect(updatedPatches).toHaveLength(1); + }); + + test("a server rejection on retarget (409 unfrozen/undeployed) reverts the picker and shows the error, without losing other unsaved input", async () => { + createdRoutine = routineRecord({ definitionAssetId: "asset_myra" }); + routines = [createdRoutine]; + await renderPanel({ routineId: "rtn_1" }); + await settle(); + + const instruction = fieldByLabel( + "What should this routine do each time it runs?", + ) as HTMLTextAreaElement; + const textareaSetter = Object.getOwnPropertyDescriptor( + window.HTMLTextAreaElement.prototype, + "value", + )?.set as (this: HTMLTextAreaElement, v: string) => void; + act(() => { + textareaSetter.call(instruction, "not yet blurred"); + instruction.dispatchEvent(new Event("input", { bubbles: true })); + }); + + patchTargetRejection = { + status: 409, + message: "That target isn't deployed yet.", + }; + selectTarget("asset_digest"); + await settle(); + + const select = container.querySelector( + "#routine-panel-target", + ) as HTMLSelectElement; + expect(select.value).toBe("asset_myra"); + expect(container.textContent).toContain("That target isn't deployed yet."); + expect(instruction.value).toBe("not yet blurred"); + }); + + test("an unavailable current target disables Run now until a valid target is chosen", async () => { + createdRoutine = routineRecord({ definitionAssetId: "asset_gone" }); + routines = [createdRoutine]; + await renderPanel({ routineId: "rtn_1" }); + await settle(); + + expect(buttonWithText("Run now")?.hasAttribute("disabled")).toBe(true); + + selectTarget("asset_digest"); + await settle(); + + expect(buttonWithText("Run now")?.hasAttribute("disabled")).toBe(false); + }); + test("Test run is disabled until the routine is saved, then fires the run-once call", async () => { await renderPanel({ routineId: null, workbenchId: "ch_1" }); expect(buttonWithText("Run now")?.hasAttribute("disabled")).toBe(true); From 972cebf60bacd5d913cf60937db714d7001d8638 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 1 Sep 2026 23:30:36 -0700 Subject: [PATCH 2/4] Routine panel: let editing a routine retarget it (CL-7358) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse DefinitionTargetPicker for edit mode instead of a read-only display, so create and edit never duplicate target-display logic. Picking a new target on an existing routine fires a target-only PATCH through the panel's autosave queue; a server rejection reverts the picker to the last-saved target and surfaces through the existing inline error path without touching other unsaved fields. The picker now reports staleness via onStaleChange so the panel can disable Run now when the current target is no longer available. Also exposes definitionAssetId on the browser-safe UpdateRoutineInput client type — the server (packages/routines/src/store.ts) already accepted it. --- .../src/shell/definition-target-picker.tsx | 16 ++- apps/web/src/shell/routine-panel.tsx | 113 ++++++++---------- packages/routines/src/client.ts | 4 + 3 files changed, 69 insertions(+), 64 deletions(-) diff --git a/apps/web/src/shell/definition-target-picker.tsx b/apps/web/src/shell/definition-target-picker.tsx index fc66e4600..a2f0db568 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" }); @@ -78,6 +84,14 @@ export function DefinitionTargetPicker({ // eslint-disable-next-line react-hooks/exhaustive-deps -- see comment above }, [tenantId, retryTick]); + useEffect(() => { + if (state.kind !== "loaded") return; + const isStale = + value !== null && + !state.targets.some((t) => t.definitionAssetId === value); + onStaleChange?.(isStale); + }, [state, value]); + if (state.kind === "loading") { return (
( - 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) { @@ -541,6 +510,32 @@ function RoutineEditorPanel({ setNeedsTargetHint(false); }; + // Retargeting an existing routine (CL-7358) 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 pickTargetForEdit = (definitionAssetId: string) => { + const id = routineIdRef.current; + if (id === null) { + pickTarget(definitionAssetId); + return; + } + const previous = targetAssetIdRef.current; + targetAssetIdRef.current = definitionAssetId; + setTargetAssetId(definitionAssetId); + void runWrite(() => + updateRoutine(tenantId as string, id, { definitionAssetId }).catch( + (cause: unknown) => { + targetAssetIdRef.current = previous; + setTargetAssetId(previous); + throw cause; + }, + ), + ); + }; + const removeTrigger = () => { setTrigger(null); draftRef.current.trigger = null; @@ -649,7 +644,7 @@ function RoutineEditorPanel({ { if (tenantId === null || routineId === null) return; return runRoutineNow(tenantId, routineId).then(() => { @@ -677,30 +672,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} +