fix(flow-runner): honor a screen field's visibleWhen — render and validation (framework#3528)

.changeset/screen-field-visible-when.md

@@ -0,0 +1,42 @@
+---
+"@object-ui/app-shell": patch
+---
+
+fix(flow-runner): honor a screen field's `visibleWhen` — in rendering AND in required-enforcement (framework#3528)
+
+A paused screen-flow rendered every declared field regardless of its
+`visibleWhen` predicate, while still enforcing `required` over the full list.
+Where a field is optional-by-design but required *when shown*, that combination
+dead-ends the run: Submit blocks on an input the user was never shown, issues
+**zero network requests**, and the flow sits paused forever.
+
+Reproduced in Chromium against a real HotCRM dev server — on both the console
+shipped with `@objectstack/*` 16.1.0 and current `main`:
+
+```
+→ POST /api/v1/automation/lead_conversion/trigger   200 {status: paused, screen}
+   rendered: ["Create Opportunity? *", "Opportunity Name *", "Opportunity Amount"]
+   click Submit (checkbox untouched)
+→ (nothing)   resume calls: 0   toasts: none   dialog: still open
+```
+
+The predicate never reached the client — the framework declared `visibleWhen` on
+the screen node's designer form but dropped it when building the paused payload
+(fixed in objectstack#3771). This is the consumer half.
+
+- **`visibleScreenFields(screen, values)`** is the single source of truth for
+  what is on screen. `ScreenView` renders from it and `FlowRunner.submit()`
+  validates from it, so the two can never disagree — splitting them is the bug.
+- Predicates are **bare CEL over the screen's own field names**
+  (`createOpportunity == true`), evaluated through the canonical
+  `@objectstack/formula` engine, the same verdict the server reaches for field
+  rules. Values bind both bare and under `record.`.
+- **Declared fields are seeded before evaluation.** An untouched checkbox holds
+  `undefined`, which CEL treats as an unknown identifier — the evaluation errors
+  and falls open, leaving the dependent field on screen in exactly the state
+  where it should be hidden. Booleans seed `false`, everything else `null`.
+- **Fail-open is preserved for genuinely broken predicates** (syntax error, or a
+  name that is not a field on this screen), matching `resolveFieldRuleState`:
+  hiding an input on a typo would silently drop data the flow is waiting for.
+
+Screens with no `visibleWhen` behave exactly as before.

packages/app-shell/src/views/FlowRunner.tsx

@@ -25,7 +25,7 @@ import {
   Button,
 } from '@object-ui/components';
 import { toast } from 'sonner';
-import { ScreenView, isObjectFormScreen, initialScreenValues, screenFields, type ScreenSpec } from './ScreenView';
+import { ScreenView, isObjectFormScreen, initialScreenValues, visibleScreenFields, type ScreenSpec } from './ScreenView';
 
 export type { ScreenSpec, ScreenFieldSpec } from './ScreenView';
 
@@ -120,7 +120,15 @@ export function FlowRunner({ state, authFetch, baseUrl, onClose, onComplete, dat
   };
 
   const submit = async () => {
-    const missing = screenFields(screen).filter(
+    // Enforce `required` over the fields ACTUALLY ON SCREEN, not the whole
+    // declared list. A `visibleWhen` field that is required *when shown* is not
+    // required while hidden — the user was never asked for it, and the flow is
+    // not waiting on it. Validating the full list here is what dead-ended
+    // #3528: HotCRM's lead conversion declares `opportunityName` required with
+    // `visibleWhen: createOpportunity == true`, so leaving the checkbox
+    // unticked blocked Submit on an input that was not on screen, and the run
+    // sat paused with no resume request ever issued.
+    const missing = visibleScreenFields(screen, values).filter(
       (f) => f.required && (values[f.name] === undefined || values[f.name] === '' || values[f.name] === null),
     );
     if (missing.length) {

packages/app-shell/src/views/ScreenView.tsx

@@ -28,6 +28,7 @@ import {
   cn,
 } from '@object-ui/components';
 import { ObjectForm } from '@object-ui/plugin-form';
+import { evalFieldPredicate } from '@object-ui/core';
 
 export interface ScreenFieldSpec {
   name: string;
@@ -37,6 +38,14 @@ export interface ScreenFieldSpec {
   options?: Array<{ value: unknown; label: string }>;
   defaultValue?: unknown;
   placeholder?: string;
+  /**
+   * Conditional-visibility predicate — bare CEL over the screen's own field
+   * names (`createOpportunity == true`), re-evaluated against the values
+   * collected so far. Omit = always visible. Read it through
+   * {@link visibleScreenFields}, never field-by-field, so rendering and
+   * `required` enforcement can never disagree (#3528).
+   */
+  visibleWhen?: string;
 }
 export interface ScreenSpec {
   nodeId: string;
@@ -73,6 +82,61 @@ export function screenFields(screen: ScreenSpec): ScreenFieldSpec[] {
   return Array.isArray(screen.fields) ? screen.fields : [];
 }
 
+/**
+ * The fields actually on screen for the values collected so far — i.e. every
+ * field whose `visibleWhen` predicate holds (or that declares none).
+ *
+ * This is the list callers must use for BOTH rendering and `required`
+ * enforcement. Splitting them is the #3528 dead-end: validate the full list
+ * while rendering a subset and Submit blocks on a field the user was never
+ * shown, with no resume request ever issued.
+ *
+ * The predicate is bare CEL over the screen's own field names
+ * (`createOpportunity == true`), evaluated through the canonical
+ * `@objectstack/formula` engine — the same verdict the server reaches for
+ * field rules. Values are bound both bare and under `record.`, so either
+ * spelling resolves. A broken predicate **fails open** (field stays visible),
+ * matching `resolveFieldRuleState`: hiding an input on a typo would silently
+ * drop data the flow is waiting for.
+ */
+export function visibleScreenFields(
+  screen: ScreenSpec,
+  values: Record<string, unknown>,
+): ScreenFieldSpec[] {
+  const scope = screenPredicateScope(screen, values);
+  return screenFields(screen).filter((f) =>
+    f.visibleWhen ? evalFieldPredicate(f.visibleWhen, scope, true, undefined, scope) : true,
+  );
+}
+
+/**
+ * The scope a screen's `visibleWhen` predicates evaluate against: every
+ * DECLARED field bound to its collected value, or to a known-empty one when
+ * the user has not touched it yet.
+ *
+ * Seeding matters. An untouched checkbox holds `undefined`, and to CEL an
+ * unbound name is an *unknown identifier* — the evaluation errors and
+ * `evalFieldPredicate` falls open, leaving `createOpportunity == true` fields
+ * on screen precisely in the state where they should be hidden. Binding the
+ * declared names makes the predicate resolve to a real `false` instead.
+ *
+ * Fail-open is still the behaviour we want for a genuinely broken predicate —
+ * a syntax error, or a name that is not a field on this screen. Seeding only
+ * the declared names keeps that split intact.
+ */
+function screenPredicateScope(
+  screen: ScreenSpec,
+  values: Record<string, unknown>,
+): Record<string, unknown> {
+  const scope: Record<string, unknown> = {};
+  for (const f of screenFields(screen)) {
+    const t = (f.type || 'text').toLowerCase();
+    scope[f.name] = t === 'boolean' || t === 'checkbox' ? false : null;
+  }
+  for (const [k, v] of Object.entries(values)) if (v !== undefined) scope[k] = v;
+  return scope;
+}
+
 /** Seed flat-field values from each field's `defaultValue`. */
 export function initialScreenValues(screen: ScreenSpec): Record<string, unknown> {
   const v: Record<string, unknown> = {};
@@ -154,7 +218,7 @@ export function ScreenView({ screen, values, onValueChange, dataSource, objects,
 
   return (
     <div className={cn('space-y-4 py-2', className)}>
-      {screenFields(screen).map((f) => (
+      {visibleScreenFields(screen, values).map((f) => (
         <div key={f.name} className="space-y-1.5">
           <Label htmlFor={`ff-${f.name}`} className="text-sm">
             {f.label || f.name}

packages/app-shell/src/views/__tests__/FlowRunner.visibleWhen.test.tsx

@@ -0,0 +1,158 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * `visibleWhen` on a screen field — rendering AND `required` enforcement.
+ *
+ * These must agree. #3528: the framework declared `visibleWhen` on the screen
+ * node's designer form but never put it on the wire, so every field rendered
+ * unconditionally while `required` was still enforced. HotCRM's lead conversion
+ * is the shape that made that fatal — an `opportunityName` that is required
+ * only *when shown*. Leave the checkbox unticked and Submit blocked on an input
+ * the user was never shown, issuing zero network requests, with the run left
+ * paused forever.
+ *
+ * The server half now forwards the predicate (objectstack#3771). This is the
+ * client half: honour it in both places, from one helper, so they cannot drift.
+ */
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { render, screen, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { FlowRunner, type ScreenFlowState } from '../FlowRunner';
+
+/** HotCRM's `lead_conversion` screen, verbatim in shape. */
+const CONVERSION: ScreenFlowState = {
+  flowName: 'lead_conversion',
+  runId: 'run-1',
+  screen: {
+    nodeId: 'screen_1',
+    title: 'Conversion Details',
+    fields: [
+      { name: 'createOpportunity', label: 'Create Opportunity?', type: 'boolean', required: true },
+      {
+        name: 'opportunityName',
+        label: 'Opportunity Name',
+        type: 'text',
+        required: true,
+        visibleWhen: 'createOpportunity == true',
+      },
+      {
+        name: 'opportunityAmount',
+        label: 'Opportunity Amount',
+        type: 'currency',
+        visibleWhen: 'createOpportunity == true',
+      },
+    ],
+  },
+};
+
+function jsonResponse(body: unknown, status = 200): Response {
+  return new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } });
+}
+
+function setup(state: ScreenFlowState, authFetch: (url: string, init?: RequestInit) => Promise<Response>) {
+  const onClose = vi.fn();
+  const onComplete = vi.fn();
+  render(<FlowRunner state={state} authFetch={authFetch} baseUrl="" onClose={onClose} onComplete={onComplete} />);
+  return { onClose, onComplete };
+}
+
+const okResume = () => vi.fn(async () => jsonResponse({ success: true, data: { success: true } }));
+
+beforeEach(() => vi.restoreAllMocks());
+
+describe('screen field visibleWhen (#3528)', () => {
+  it('hides a field whose predicate is false', () => {
+    setup(CONVERSION, okResume());
+    expect(screen.getByText('Create Opportunity?')).toBeInTheDocument();
+    expect(screen.queryByText('Opportunity Name')).not.toBeInTheDocument();
+    expect(screen.queryByText('Opportunity Amount')).not.toBeInTheDocument();
+  });
+
+  it('reveals the dependent fields once the predicate holds', async () => {
+    const user = userEvent.setup();
+    setup(CONVERSION, okResume());
+    await user.click(screen.getByRole('checkbox'));
+    await waitFor(() => expect(screen.getByText('Opportunity Name')).toBeInTheDocument());
+    expect(screen.getByText('Opportunity Amount')).toBeInTheDocument();
+  });
+
+  /**
+   * The regression. A hidden `required` field must not block Submit — the whole
+   * reported defect is that it did, silently.
+   */
+  it('RESUMES with a hidden required field left empty', async () => {
+    const user = userEvent.setup();
+    const authFetch = okResume();
+    const { onComplete } = setup(CONVERSION, authFetch);
+
+    // Tick the required boolean; `opportunityName` stays hidden and empty.
+    await user.click(screen.getByRole('checkbox'));
+    await waitFor(() => expect(screen.getByText('Opportunity Name')).toBeInTheDocument());
+    await user.click(screen.getByRole('checkbox')); // untick → hidden again
+    await waitFor(() => expect(screen.queryByText('Opportunity Name')).not.toBeInTheDocument());
+
+    await user.click(screen.getByRole('button', { name: 'Submit' }));
+
+    await waitFor(() => expect(authFetch).toHaveBeenCalledTimes(1));
+    expect(authFetch.mock.calls[0][0]).toBe('/api/v1/automation/lead_conversion/runs/run-1/resume');
+    expect(onComplete).toHaveBeenCalled();
+  });
+
+  it('still blocks Submit on a VISIBLE required field left empty', async () => {
+    const user = userEvent.setup();
+    const authFetch = okResume();
+    setup(CONVERSION, authFetch);
+
+    // Reveal `opportunityName` (required) and submit without filling it.
+    await user.click(screen.getByRole('checkbox'));
+    await waitFor(() => expect(screen.getByText('Opportunity Name')).toBeInTheDocument());
+    await user.click(screen.getByRole('button', { name: 'Submit' }));
+
+    // No resume — the field is genuinely on screen and genuinely required.
+    await new Promise((r) => setTimeout(r, 50));
+    expect(authFetch).not.toHaveBeenCalled();
+  });
+
+  /**
+   * Fail-open, matching `resolveFieldRuleState`: hiding an input because its
+   * predicate has a typo would silently drop data the flow is waiting for.
+   */
+  it('keeps a field visible when its predicate is broken', () => {
+    setup(
+      {
+        flowName: 'f',
+        runId: 'r',
+        screen: {
+          nodeId: 'n',
+          title: 'Broken',
+          fields: [{ name: 'note', label: 'Note', type: 'text', visibleWhen: 'this is not CEL (((' }],
+        },
+      },
+      okResume(),
+    );
+    expect(screen.getByText('Note')).toBeInTheDocument();
+  });
+
+  it('leaves a screen with no predicates completely unchanged', async () => {
+    const user = userEvent.setup();
+    const authFetch = okResume();
+    setup(
+      {
+        flowName: 'reassign',
+        runId: 'run-9',
+        screen: {
+          nodeId: 'collect',
+          title: 'New Assignee',
+          fields: [{ name: 'new_assignee', label: 'Assignee', type: 'text', required: true }],
+        },
+      },
+      authFetch,
+    );
+    await user.type(screen.getAllByRole('textbox')[0], 'ada@example.com');
+    await user.click(screen.getByRole('button', { name: 'Submit' }));
+    await waitFor(() => expect(authFetch).toHaveBeenCalledTimes(1));
+    expect(JSON.parse(String(authFetch.mock.calls[0][1]?.body))).toMatchObject({
+      inputs: { new_assignee: 'ada@example.com' },
+    });
+  });
+});