Skip to content

Add routine target editing to the routine panel (CL-7358) #559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TheGreatAxios wants to merge 4 commits into from
Open

Add routine target editing to the routine panel (CL-7358) #559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41c7816
Add tests for routine target editing in the routine panel (CL-7358)
TheGreatAxios Sep 2, 2026
972cebf
Routine panel: let editing a routine retarget it (CL-7358)
TheGreatAxios Sep 2, 2026
6b37632
Address review findings (CL-7358)
TheGreatAxios Sep 2, 2026
19211a1
Fix CI after review pass (CL-7358)
TheGreatAxios Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 37 additions & 2 deletions apps/web/src/shell/definition-target-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<LoadState>({ kind: "loading" });
Expand All @@ -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;
Expand Down Expand Up @@ -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<LoadState, { kind: "loaded" }>,
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 (
<div
Expand Down Expand Up @@ -147,7 +182,7 @@ export function DefinitionTargetPicker({
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;
const stale = isTargetStale(state, value);

return (
<div className="flex flex-col gap-1.5" aria-live="polite">
Expand Down
129 changes: 61 additions & 68 deletions apps/web/src/shell/routine-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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,
Expand Down Expand Up @@ -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<RoutineTarget | null>(
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
Expand Down Expand Up @@ -283,7 +274,7 @@ function RoutineEditorPanel({
setError(null);
setRuns([]);
setTargetAssetId(null);
setExistingTarget(null);
setTargetStale(false);
}, [subject]);

const loadRuns = (id: string) => {
Expand Down Expand Up @@ -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;
Expand All @@ -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) {
Expand Down Expand Up @@ -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 = () => {
Expand Down Expand Up @@ -649,7 +650,7 @@ function RoutineEditorPanel({
<RunNowButton
variant="outline"
size="sm"
disabled={routineId === null}
disabled={routineId === null || targetStale}
onRun={() => {
if (tenantId === null || routineId === null) return;
return runRoutineNow(tenantId, routineId).then(() => {
Expand Down Expand Up @@ -677,30 +678,22 @@ function RoutineEditorPanel({
/>
</div>

{routineId === null ? (
<div className="flex flex-col gap-1.5">
<DefinitionTargetPicker
tenantId={tenantId}
value={targetAssetId}
onChange={pickTarget}
{...(subject.preselectedAssetId !== undefined
? { preselectedAssetId: subject.preselectedAssetId }
: {})}
/>
{needsTargetHint && targetAssetId === null ? (
<p className="text-xs text-[var(--ui-danger)]" role="alert">
Pick what this routine runs before the rest can save.
</p>
) : null}
</div>
) : (
<div className="flex flex-col gap-1.5">
<span className="text-xs font-medium">What this routine runs</span>
<div className="rounded-[var(--ui-radius-md)] border border-[var(--ui-border)] px-2.5 py-1.5 text-sm">
{existingTarget?.name ?? targetAssetId ?? "—"}
</div>
</div>
)}
<div className="flex flex-col gap-1.5">
<DefinitionTargetPicker
tenantId={tenantId}
value={targetAssetId}
onChange={pickTarget}
onStaleChange={setTargetStale}
{...(routineId === null && subject.preselectedAssetId !== undefined
? { preselectedAssetId: subject.preselectedAssetId }
: {})}
/>
{needsTargetHint && targetAssetId === null ? (
<p className="text-xs text-[var(--ui-danger)]" role="alert">
Pick what this routine runs before the rest can save.
</p>
) : null}
</div>

<div className="flex flex-col gap-1.5">
<label
Expand Down
Loading
Loading