Skip to content

Commit 20624c1

Browse files
alex-strukclaude
andcommitted
fix(workflow-builder): Try/Run refuse an unrunnable graph; honest preview copy
Three findings from the walkthrough, all the same shape — the canvas asserting one thing while the run does another. D-11 (14.8) — a graph with validation errors was still runnable. A deleted `dyn.*` lineage is diagnosed at author time (red Deleted badge, "not registered") and the run then dies at dynamicNode.resolveLineage. Try and Run now refuse while errorCount > 0. Warnings are advisory and deliberately do NOT block. D-16 (9.12) — with unsaved edits, Try ran the PREVIOUSLY SAVED graph and said nothing. Measured live: the rename landed on the canvas, the editor reported dirty, and after Try no version existed and the server still held the old label. Try and Run now refuse while dirty. Chose this over auto-saving because auto-save mints a version on every dirty Try, which changes what version history means. Both funnel through one `runBlockedReason`, which doubles as the tooltip — a button can never be disabled here without saying why. The existing "Save the workflow first" string is reused verbatim so its assertions elsewhere keep passing. D-12 (14.9) — a succeeded node with no cache row always reported `evicted` and offered "Re-run to repopulate". For a `@deterministic:false` dynamic node that is untrue twice: nothing expired (it is never cached, §3.3) and re-running can never repopulate it. New `not-cached` reason with its own copy, no Re-run button, naming the actual cause and the fix. No backend work was needed — the catalog already ships `nonCacheable`; it just was not being read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 8f26609 commit 20624c1

6 files changed

Lines changed: 278 additions & 24 deletions

File tree

apps/frontend/src/features/workflow-builder/WorkflowEditorV2Page.test.tsx

Lines changed: 134 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ const {
4040
capturedCreateDto,
4141
capturedPaletteProps,
4242
catalogEntriesRef,
43+
validationRef,
4344
capturedRunDrawerProps,
4445
capturedSettingsPanelProps,
4546
capturedValidationDrawerProps,
@@ -66,6 +67,16 @@ const {
6667
// The merged activity catalog, mutable so a test can model the entry
6768
// arriving AFTER a publish (which is when `onAddDynamicNode` fires).
6869
catalogEntriesRef: { current: [] as Array<Record<string, unknown>> },
70+
// D-11: mutable so a test can put the graph into an error state.
71+
validationRef: {
72+
current: {
73+
errorCount: 0,
74+
warningCount: 0,
75+
isPending: false,
76+
errorsByNode: new Map(),
77+
errors: [] as Array<Record<string, unknown>>,
78+
},
79+
},
6980
// US-148 — the Run drawer stub captures its props so the trigger
7081
// tests can verify `openMode` was set correctly by whichever
7182
// top-bar button opened the drawer.
@@ -243,13 +254,7 @@ vi.mock("./dynamic-nodes", () => ({
243254
}));
244255

245256
vi.mock("./validation/useGraphValidation", () => ({
246-
useGraphValidation: () => ({
247-
errorCount: 0,
248-
warningCount: 0,
249-
isPending: false,
250-
errorsByNode: new Map(),
251-
errors: [],
252-
}),
257+
useGraphValidation: () => validationRef.current,
253258
}));
254259

255260
// US-153 — the Run-history drawer body calls `useWorkflowRuns`, which
@@ -3253,3 +3258,125 @@ describe("WorkflowEditorV2Page — 14.8 dynamic node drops after a publish", ()
32533258
expect(added).toBeDefined();
32543259
});
32553260
});
3261+
3262+
/**
3263+
* D-11 + D-16 — Try and Run must refuse a graph the run would not honour.
3264+
*
3265+
* Both were found walking the plan and are the same bug wearing two hats: the
3266+
* canvas asserted one thing and the run did another.
3267+
*
3268+
* D-11 (14.8) — a workflow whose `dyn.*` lineage is deleted is diagnosed
3269+
* correctly (red Deleted badge, "not registered" error) and Try stayed
3270+
* enabled. The run then failed at `dynamicNode.resolveLineage` — knowable at
3271+
* author time, which is what "fail before the run" asks for.
3272+
*
3273+
* D-16 (9.12) — with unsaved edits, Try ran the PREVIOUSLY SAVED graph.
3274+
* Measured live: the rename landed on the canvas, the editor reported dirty,
3275+
* and after Try no version existed and the server still held the old label.
3276+
* The author watched badges light up for a graph they were not looking at.
3277+
*/
3278+
describe("WorkflowEditorV2Page — D-11/D-16 Try and Run refuse an unrunnable graph", () => {
3279+
beforeEach(() => {
3280+
capturedCanvasProps.current = null;
3281+
capturedPaletteProps.current = null;
3282+
validationRef.current = {
3283+
errorCount: 0,
3284+
warningCount: 0,
3285+
isPending: false,
3286+
errorsByNode: new Map(),
3287+
errors: [],
3288+
};
3289+
existingWorkflowRef.current = {
3290+
id: "wf-1",
3291+
name: "WF",
3292+
description: "",
3293+
slug: "wf",
3294+
version: 1,
3295+
workflowVersionId: "v-head",
3296+
config: buildTemplateConfig({ positions: "all" }),
3297+
};
3298+
});
3299+
3300+
afterEach(() => {
3301+
validationRef.current = {
3302+
errorCount: 0,
3303+
warningCount: 0,
3304+
isPending: false,
3305+
errorsByNode: new Map(),
3306+
errors: [],
3307+
};
3308+
existingWorkflowRef.current = null;
3309+
});
3310+
3311+
it("enables Try and Run on a clean, saved, valid graph", async () => {
3312+
renderEditPage("wf-1");
3313+
await waitFor(() => {
3314+
expect(capturedCanvasProps.current?.config).toBeDefined();
3315+
});
3316+
expect(screen.getByTestId("try-button")).toBeEnabled();
3317+
expect(screen.getByTestId("run-this-workflow-button")).toBeEnabled();
3318+
});
3319+
3320+
it("D-11 — disables Try and Run while the graph has validation errors", async () => {
3321+
validationRef.current = {
3322+
errorCount: 1,
3323+
warningCount: 0,
3324+
isPending: false,
3325+
errorsByNode: new Map(),
3326+
errors: [
3327+
{
3328+
path: "nodes.a.activityType",
3329+
message: 'Activity type "dyn.gone" is not registered',
3330+
severity: "error",
3331+
},
3332+
],
3333+
};
3334+
renderEditPage("wf-1");
3335+
await waitFor(() => {
3336+
expect(capturedCanvasProps.current?.config).toBeDefined();
3337+
});
3338+
expect(screen.getByTestId("try-button")).toBeDisabled();
3339+
expect(screen.getByTestId("run-this-workflow-button")).toBeDisabled();
3340+
});
3341+
3342+
it("D-11 — a warning alone does NOT disable them", async () => {
3343+
validationRef.current = {
3344+
errorCount: 0,
3345+
warningCount: 3,
3346+
isPending: false,
3347+
errorsByNode: new Map(),
3348+
errors: [],
3349+
};
3350+
renderEditPage("wf-1");
3351+
await waitFor(() => {
3352+
expect(capturedCanvasProps.current?.config).toBeDefined();
3353+
});
3354+
expect(screen.getByTestId("try-button")).toBeEnabled();
3355+
expect(screen.getByTestId("run-this-workflow-button")).toBeEnabled();
3356+
});
3357+
3358+
it("D-16 — disables Try and Run once there are unsaved edits", async () => {
3359+
renderEditPage("wf-1");
3360+
await waitFor(() => {
3361+
expect(capturedCanvasProps.current?.config).toBeDefined();
3362+
});
3363+
expect(screen.getByTestId("try-button")).toBeEnabled();
3364+
3365+
const onConfigChange = capturedCanvasProps.current?.onConfigChange as (
3366+
c: GraphWorkflowConfig,
3367+
) => void;
3368+
const live = capturedCanvasProps.current?.config as GraphWorkflowConfig;
3369+
act(() => {
3370+
onConfigChange({
3371+
...live,
3372+
nodes: {
3373+
...live.nodes,
3374+
b: { ...(live.nodes.b as ActivityNode), label: "EDITED-NOT-SAVED" },
3375+
},
3376+
});
3377+
});
3378+
3379+
expect(screen.getByTestId("try-button")).toBeDisabled();
3380+
expect(screen.getByTestId("run-this-workflow-button")).toBeDisabled();
3381+
});
3382+
});

apps/frontend/src/features/workflow-builder/WorkflowEditorV2Page.tsx

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -762,6 +762,33 @@ function WorkflowEditorV2PageBody({ mode }: WorkflowEditorV2PageProps) {
762762
// session. Reuses the dirty signal above — no second source of truth.
763763
useUnsavedGuard({ isDirty, isDirtyNow: hasUnsavedChanges });
764764

765+
/**
766+
* Why Try / Run are unavailable, or `null` when they are. Doubles as the
767+
* tooltip, so the button can never be disabled without saying why.
768+
*
769+
* D-11 and D-16 are the same defect wearing two hats — the canvas asserting
770+
* one thing while the run does another:
771+
*
772+
* - **D-11**: a graph with validation errors was still runnable. A deleted
773+
* `dyn.*` lineage is diagnosed at author time (red Deleted badge,
774+
* "not registered") and the run then dies at
775+
* `dynamicNode.resolveLineage`. The plan's own invariant is "a state the
776+
* runtime cannot satisfy is reported at author time" — so refuse here.
777+
* Warnings are advisory and deliberately do NOT block.
778+
* - **D-16**: with unsaved edits, Try ran the PREVIOUSLY SAVED graph and
779+
* said nothing. Refusing while dirty keeps version history meaningful
780+
* (the alternative, auto-saving, mints a version on every Try) and makes
781+
* "what you see is what runs" true again.
782+
*/
783+
const runBlockedReason: string | null =
784+
!isEditMode || !workflowId
785+
? "Save the workflow first"
786+
: validation.errorCount > 0
787+
? `Fix ${validation.errorCount} validation ${validation.errorCount === 1 ? "error" : "errors"} first — this graph cannot run as it stands`
788+
: isDirty
789+
? "Save your changes first — a run always executes the saved graph, not the canvas"
790+
: null;
791+
765792
// Hydrate state when the workflow loads in edit mode.
766793
// Run auto-layout when the loaded config carries no node positions — e.g.
767794
// seeded workflows (docs-md/workflows/templates/*.json) and any
@@ -1414,18 +1441,15 @@ function WorkflowEditorV2PageBody({ mode }: WorkflowEditorV2PageProps) {
14141441
Save
14151442
</Button>
14161443
{tryButtonVisible && (
1417-
<Tooltip
1418-
label="Save the workflow first"
1419-
disabled={isEditMode && !!workflowId}
1420-
>
1444+
<Tooltip label={runBlockedReason ?? "Run this graph now"}>
14211445
<Button
14221446
variant="filled"
14231447
color="blue"
14241448
leftSection={<IconBolt size={14} />}
14251449
onClick={() => setRunDrawerMode("try")}
14261450
size="xs"
14271451
data-testid="try-button"
1428-
disabled={!isEditMode || !workflowId}
1452+
disabled={runBlockedReason !== null}
14291453
>
14301454
Try
14311455
</Button>
@@ -1437,11 +1461,9 @@ function WorkflowEditorV2PageBody({ mode }: WorkflowEditorV2PageProps) {
14371461
onClick={() => setRunDrawerMode("run")}
14381462
size="xs"
14391463
data-testid="run-this-workflow-button"
1440-
disabled={!isEditMode || !workflowId}
1464+
disabled={runBlockedReason !== null}
14411465
title={
1442-
!isEditMode || !workflowId
1443-
? "Save the workflow first to enable Run."
1444-
: "Open the run-trigger panel for this workflow"
1466+
runBlockedReason ?? "Open the run-trigger panel for this workflow"
14451467
}
14461468
>
14471469
Run this workflow

apps/frontend/src/features/workflow-builder/canvas/WorkflowEditorCanvas.tsx

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -666,10 +666,14 @@ const ActivityNodeRenderer = memo(
666666
// lineage was soft-deleted) render a red "Deleted" pill instead.
667667
const isDynamic = data.activityType.startsWith("dyn.");
668668
const catalog = useActivityCatalog();
669+
const catalogEntry = catalog.entries.find(
670+
(e) => e.activityType === data.activityType,
671+
);
669672
const isMissingFromCatalog =
670-
isDynamic &&
671-
!catalog.isLoading &&
672-
!catalog.entries.some((e) => e.activityType === data.activityType);
673+
isDynamic && !catalog.isLoading && catalogEntry === undefined;
674+
// D-12: a `@deterministic:false` dynamic node is never written to the
675+
// output cache, so its empty preview is "not cached", not "evicted".
676+
const neverCached = catalogEntry?.nonCacheable === true;
673677
// Node-level flow handles: the unnamed left target + the `id="out"`
674678
// right source keep today's connect gesture AND anchor everything the
675679
// wire projection doesn't route to a per-port dot. Source handles are
@@ -821,7 +825,11 @@ const ActivityNodeRenderer = memo(
821825
onOutputHandleEnter={data.onOutputHandleEnter}
822826
onOutputHandleLeave={data.onOutputHandleLeave}
823827
/>
824-
<NodePreviewOverlay nodeId={id} outputs={data.previewOutputs} />
828+
<NodePreviewOverlay
829+
nodeId={id}
830+
outputs={data.previewOutputs}
831+
neverCached={neverCached}
832+
/>
825833
</div>
826834
);
827835
},

apps/frontend/src/features/workflow-builder/preview/PreviewWidget.tsx

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,14 @@ export interface PreviewWidgetProps {
102102
* so they stay silent instead of showing a misleading "cache evicted" alert.
103103
*/
104104
producesOutput?: boolean;
105+
/**
106+
* D-12 — true when this node's output is never cached at all (a dynamic node
107+
* whose script is `@deterministic:false`, surfaced by the catalog as
108+
* `nonCacheable`). Without it a green run reported "cache evicted — re-run to
109+
* repopulate", blaming a TTL that never applied and offering a recovery that
110+
* cannot work.
111+
*/
112+
neverCached?: boolean;
105113
}
106114

107115
/**
@@ -120,6 +128,7 @@ export function PreviewWidget({
120128
outputs,
121129
nodeStatus,
122130
producesOutput = true,
131+
neverCached = false,
123132
}: PreviewWidgetProps): ReactNode {
124133
const { data, isLoading, error } = useActivityOutputPreview(
125134
workflowId,
@@ -170,6 +179,7 @@ export function PreviewWidget({
170179
runFinished: isReplay && hasRun,
171180
producesOutput,
172181
hasActiveRun: hasRun,
182+
neverCached,
173183
});
174184
const copy = describeNoOutput(reason);
175185

@@ -271,6 +281,14 @@ export interface NodePreviewOverlayProps {
271281
* output-cache row). Defaults to `true`.
272282
*/
273283
producesOutput?: boolean;
284+
/**
285+
* D-12 — true when this node's output is never cached at all (a dynamic node
286+
* whose script is `@deterministic:false`, surfaced by the catalog as
287+
* `nonCacheable`). Without it a green run reported "cache evicted — re-run to
288+
* repopulate", blaming a TTL that never applied and offering a recovery that
289+
* cannot work.
290+
*/
291+
neverCached?: boolean;
274292
}
275293

276294
/**
@@ -284,6 +302,7 @@ export function NodePreviewOverlay({
284302
nodeId,
285303
outputs,
286304
producesOutput = true,
305+
neverCached = false,
287306
}: NodePreviewOverlayProps): ReactNode {
288307
const ctx = useOptionalRunState();
289308
if (!ctx) {
@@ -316,6 +335,7 @@ export function NodePreviewOverlay({
316335
outputs={outputs}
317336
nodeStatus={ctx.nodeStatuses[nodeId]?.status}
318337
producesOutput={producesOutput}
338+
neverCached={neverCached}
319339
/>
320340
);
321341
}

apps/frontend/src/features/workflow-builder/preview/no-output-state.test.ts

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,3 +112,55 @@ describe("describeNoOutput", () => {
112112
).toThrowError(/unhandled variant/);
113113
});
114114
});
115+
116+
/**
117+
* D-12 — a succeeded node with no cache row was always reported as `evicted`,
118+
* offering a Re-run to "repopulate" it. For a `@deterministic:false` dynamic
119+
* node that is untrue twice over: nothing was evicted (it was never cached),
120+
* and re-running can never repopulate it, because §3.3 says such scripts must
121+
* re-execute every run and are deliberately not cached.
122+
*
123+
* Measured live on the Part-14 demo before it was tagged deterministic: a green
124+
* run, and the widget reading "Preview unavailable — cache evicted. Re-run to
125+
* repopulate."
126+
*/
127+
describe("noOutputReasonForNode — D-12 never-cached vs evicted", () => {
128+
const base = {
129+
runFinished: true,
130+
producesOutput: true,
131+
hasActiveRun: true,
132+
} as const;
133+
134+
it("reports `not-cached` for a succeeded node that is never cached", () => {
135+
expect(
136+
noOutputReasonForNode({
137+
...base,
138+
status: "succeeded",
139+
neverCached: true,
140+
}),
141+
).toBe("not-cached");
142+
});
143+
144+
it("still reports `evicted` for a succeeded node that IS cacheable", () => {
145+
expect(
146+
noOutputReasonForNode({
147+
...base,
148+
status: "succeeded",
149+
neverCached: false,
150+
}),
151+
).toBe("evicted");
152+
});
153+
154+
it("never-cached does not mask a failure", () => {
155+
expect(
156+
noOutputReasonForNode({ ...base, status: "failed", neverCached: true }),
157+
).toBe("failed");
158+
});
159+
160+
it("offers no Re-run for `not-cached`, and says why rather than blaming an eviction", () => {
161+
const copy = describeNoOutput("not-cached");
162+
expect(copy.offersRerun).toBe(false);
163+
expect(copy.message).toMatch(/non-deterministic/i);
164+
expect(copy.message).not.toMatch(/evict/i);
165+
});
166+
});

0 commit comments

Comments
 (0)