ENG-3522: Add client-side conditional validation for privacy center custom fields

[jpople]

Ticket ENG-3522

Description Of Changes

Adds client-side support for display_condition on custom privacy center form fields. When a field has a display_condition, it is evaluated against the current form values and shown/hidden reactively. Validation and submission are adjusted to exclude fields hidden by their conditions.

This mirrors the backend's condition evaluation logic (ENG-3516) on the frontend, supporting the 5 display operators (eq, neq, exists, not_exists, list_contains), condition groups with and/or logic, and cascading dependencies via fixed-point iteration.

Code Changes

• Added condition types (ConditionLeaf, ConditionGroup, Condition, DisplayOperator) and display_condition to ICustomField in types/config.ts
• Created lib/condition-evaluator.ts — pure condition evaluator with evaluateCondition() and resolveApplicableFields() (fixed-point iteration for cascading deps)
• Created hooks/useApplicableFields.ts — React hook wrapping the evaluator with memoization (only recomputes when watched form values change)
• Updated hooks/useCustomFieldsForm.ts — extracted buildCustomFieldsValidationSchema() that accepts an optional applicableFields set
• Updated usePrivacyRequestForm.ts — switched to Formik validate function for dynamic schema; added value clearing on hide; filters gated-off fields from submission
• Updated PrivacyRequestForm.tsx — filters rendered fields by applicable set
• Updated useConsentRequestForm.ts and ConsentRequestForm.tsx — same pattern for consent flow
• Added 35 unit tests for the condition evaluator covering all operators, groups, nesting, cascading deps, and edge cases

Steps to Confirm

1. Configure a privacy center action with display_condition on a custom field (e.g., show "State" only when "Country" equals "US")
2. Open the privacy request form — the conditional field should be hidden when the condition is false
3. Change the controlling field's value to satisfy the condition — the conditional field should appear
4. Submit the form with a hidden conditional field — it should not be included in the request payload
5. Verify a required conditional field does not cause validation errors when hidden

Pre-Merge Checklist

• Issue requirements met
• All CI pipelines succeeded
• CHANGELOG.md updated
  • Updates unreleased work already in Changelog, no new entry necessary
• UX feedback:
  • No UX review needed
• Followup issues:
  • No followup issues
• Database migrations:
  • No migrations
• Documentation:
  • Documentation issue created in fidesdocs

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ignored	Preview	May 21, 2026 5:10pm
fides-privacy-center	Ignored		May 21, 2026 5:10pm

[github-actions]

Title	Lines	Statements	Branches	Functions
admin-ui		8.54% (4053/47445)	7.59% (2094/27561)	5.82% (812/13944)
fides-js		79.17% (1977/2497)	66.25% (1249/1885)	73.31% (349/476)
privacy-center		86.59% (517/597)	82.33% (247/300)	81.08% (90/111)

[rayharnett]

Code Review: ENG-3522 — Client-Side Conditional Validation for Privacy Center Custom Fields

PR: #8163
Branch: jpople/eng-3522/conditional-validation → main
Files changed: 15 files, +1,193 / −300 lines

---

1. 🚨 Critical Issues (Must Fix)

1.1 evaluateCondition swallows all errors silently

File: lib/condition-evaluator.ts, lines 57–72

export const evaluateCondition = (
  condition: Condition,
  data: Record<string, unknown>,
): boolean => {
  try {
    // ...evaluation logic...
  } catch {
    return false; // <── swallows everything: type errors, null derefs, etc.
  }
};

Problem: The catch block silently returns false for any error, including programming mistakes (e.g., accessing .logical_operator on a malformed object). This makes bugs impossible to detect in production and produces unpredictable show/hide behavior that's hard to debug.

Suggestion: Log the error (at minimum console.error) before returning false, so developers see stack traces in browser console:

} catch (err) {
  console.error("[condition-evaluator] Failed to evaluate condition:", err);
  return false;
}

Why: Silent failure is fine for user-facing behavior (don't crash the form), but developers need visibility into configuration errors.

---

1.2 resolveApplicableFields cycle guard returns silently incomplete result

File: lib/condition-evaluator.ts, lines 134–139

if (iterations > maxIterations) {
  return applicable; // <── returns whatever happened to be last, no warning
}

Problem: If the backend's acyclic-graph guarantee is ever bypassed (manual config edit, migration bug), this silently returns a partial result. The caller has no way to distinguish "converged normally" from "hit cycle guard."

Suggestion: Log a warning when the guard triggers:

if (iterations > maxIterations) {
  console.warn(
    `[condition-evaluator] resolveApplicableFields exceeded max iterations (${maxIterations}). ` +
    `Possible circular dependency in display_condition config. Applicable fields: ${Array.from(applicable).join(", ")}`,
  );
  return applicable;
}

---

1.3 hasValue treats false and 0 differently from backend behavior

File: lib/condition-evaluator.ts, lines 12–20

const hasValue = (val: unknown): boolean => {
  if (val === null || val === undefined) return false;
  if (val === "") return false;
  if (Array.isArray(val) && val.length === 0) return false;
  return true; // <── false, 0, NaN all pass — matches JS truthiness
};

Problem: The comment says "matching backend behavior," but hasValue(false) and hasValue(0) both return true because they pass all guards. This is probably correct for the backend (which likely distinguishes "absent" from "present" using None/null), but it's worth verifying that the backend also treats boolean false and numeric 0 as "present." If it doesn't, the frontend/backend will disagree on visibility for these values.

Action: Confirm backend behavior for exists/not_exists with false, 0, and NaN values.

---

2. ⚠️ Warnings (Should Fix)

2.1 useApplicabilitySync ref update precedes value clearing — race window

File: hooks/useConditionalValidation.ts, lines 78–79

export const useApplicabilitySync = ({...}) => {
  const applicableFields = useApplicableFields(
    customPrivacyRequestFields,
    formValues,
  );
  applicableFieldsRef.current = applicableFields; // <── ref updated first

Problem: The ref is set synchronously at the top of the hook, but useEffect clears field values later. If something reads applicableFieldsRef.current between the ref update and the effect (e.g., a custom submit handler that fires on render), it will see the new applicable set while old values are still in Formik state — leading to inconsistent submission.

Suggestion: Move the ref update into the useEffect alongside the clearing logic, so both happen atomically:

const applicableFields = useApplicableFields(
  customPrivacyRequestFields,
  formValues,
);

// eslint-disable-next-line no-param-reassign
useEffect(() => {
  applicableFieldsRef.current = applicableFields; // <── move here
  const prev = prevApplicable.current;
  prevApplicable.current = applicableFields;

  prev.forEach((key) => { /* ... */ });
}, [applicableFields]); // eslint-disable-line react-hooks/exhaustive-deps

---

2.2 set-utils.ts uses a flag instead of early return

File: lib/set-utils.ts, lines 1–8

export const setsEqual = (a: Set<string>, b: Set<string>): boolean => {
  if (a.size !== b.size) return false;
  let equal = true;
  a.forEach((item) => {
    if (!b.has(item)) equal = false; // <── flag, no early exit per iteration
  });
  return equal;
};

Problem: While not a bug (Set iteration is fast for small sets), this doesn't short-circuit on the first mismatch. Given that resolveApplicableFields calls this every render, and fields can number dozens in a privacy center, a straightforward early exit is cleaner:

for (const item of a) {
  if (!b.has(item)) return false;
}
return true;

---

2.3 JSON.stringify on form values in useApplicableFields — no circular reference protection

File: hooks/useApplicableFields.ts, line 30

return JSON.stringify(entries); // <── formValues can contain arrays, objects...

Problem: If a multiselect field or any future custom field type stores nested objects, JSON.stringify will throw on circular references (e.g., React state objects). The form values are typed as Record<string, string | string[]>, but there's no runtime enforcement.

Suggestion: Add a try/catch around the stringify or use a safer serializer. At minimum, add a comment noting that form values must be JSON-safe primitives.

---

2.4 useConditionalValidate schema cache can be stale across field reconfigurations

File: hooks/useConditionalValidation.ts, lines 26–31

const schemaCache = useRef<{
  applicableKey: string;
  schema: Yup.AnyObjectSchema;
} | null>(null);

const validate = (values: FormValues) => {
  // ...
  if (schemaCache.current?.applicableKey === applicableKey) {
    combinedSchema = schemaCache.current.schema; // <── cached across field reconfigurations

Problem: The cache key is only the sorted applicable keys string. If a field's display_condition changes (e.g., "State" becomes conditional on "Country == US"), but the applicable keys string is identical ("country,state"), the stale schema from before the config change would be served.

Mitigation: In practice, this is unlikely because display_condition changes usually alter the applicable set. But if a field is re-conditionalized to depend on different fields while remaining in the applicable set, the cache would be stale.

Suggestion: Add a configVersion or field-hash to the cache key, or invalidate the cache when customPrivacyRequestFields changes:

const configSignature = useMemo(
  () => JSON.stringify(Object.keys(customPrivacyRequestFields).sort()),
  [customPrivacyRequestFields],
);

// In validate:
if (schemaCache.current?.applicableKey === applicableKey && 
    schemaCache.current?.configSignature === configSignature) {

---

3. 💡 Suggestions (Nitpicks/Improvements)

3.1 Consider making hasValue explicit about what counts as "absent"

The current implementation treats empty strings, null, undefined, and empty arrays as absent. This matches the backend (per comment), but it's worth documenting why in a JSDoc block or extracting named constants:

/**
 * Values considered "absent" for exists/not_exists evaluation.
 * Matches backend _is_absent() behavior: null, undefined, "", and [].
 */
const ABSENT_VALUES = new Set([null, undefined, ""]);

3.2 buildCustomFieldsValidationSchema filters by applicable fields but the function signature is awkward

The optional applicableFields?: Set<string> parameter forces callers to always pass it (or not) depending on context. Consider an overload or a builder pattern:

export const buildCustomFieldsValidationSchema = (fields: Record<string, CustomConfigField>) => {
  const builder = Yup.object({ /* base rules */ });
  return {
    withApplicable: (applicableFields: Set<string>) => 
      builder.shape(/* filtered */),
  };
};

This is a style preference — the current approach works fine.

3.3 Test coverage gap: no integration test for useApplicableFields or useConditionalValidate

The 480-line unit test suite for condition-evaluator.ts is excellent, but the React hooks (useApplicableFields, useConditionalValidate, useApplicabilitySync) have no tests. These hooks tie the pure evaluator to React state and Formik — a layer where bugs like stale closures, incorrect dependency arrays, or race conditions can creep in.

Suggestion: At minimum, add a test for useApplicabilitySync that verifies field values are cleared when their condition becomes false.

3.4 visibility.ts deletion — check for external imports

The old common/visibility.ts (and its isFieldVisible + VisibilityCondition) was deleted. Ensure no other packages or entry points import from it (the diff shows 46 deletions, so this was the last user). A quick grep confirms no remaining references — good cleanup.

3.5 Console error in useConsentRequestForm submit handler

File: components/modals/consent-request-modal/useConsentRequestForm.ts, line ~130

} catch (error) {
  console.error(error); // <── bare catch-all from before, not addressed by PR
  handleError({ title: "An unhandled exception occurred." });
}

This pre-exists the PR but is worth noting — it should use a proper error type.

---

4. ✅ Good Practices

4.1 Pure evaluator function is an excellent design choice

evaluateCondition() and resolveApplicableFields() are pure functions with no side effects, making them trivially testable. The 480-line test suite covering all 5 operators, AND/OR groups, nested groups, cascading dependencies, and edge cases (empty conditions arrays) is exemplary.

4.2 Fixed-point iteration with cycle guard

The resolveApplicableFields algorithm correctly mirrors the backend's _resolve_applicable, converging in at most N iterations for N fields. The cycle guard (maxIterations = keys.length + 1) is a pragmatic safety net.

4.3 React hook memoization strategy

useApplicableFields uses a watched-keys snapshot (JSON.stringify(entries)) as the memoization key, ensuring it only recomputes when actually relevant form values change. This avoids unnecessary re-evaluations on every keystroke for unrelated fields.

4.4 Schema caching in useConditionalValidate

Caching Yup schemas by applicable-key avoids rebuilding the validation object on every keystroke, which is a smart performance optimization for forms with many conditional fields.

4.5 Conditional field values cleared on hide

useApplicabilitySync clears form values when fields become non-applicable, preventing stale data from persisting in the form state. The check for "unchanged" values (comparing to initial) prevents unnecessary re-set operations that could trigger UI flicker.

4.6 Old visibility.ts properly removed

The legacy visible_when system was cleanly replaced — no dangling references. The new system is strictly more capable (5 operators vs 4, supports groups with AND/OR).

4.7 Type safety throughout

The ConditionLeaf, ConditionGroup, and DisplayOperator types are well-structured discriminated unions that TypeScript can narrow correctly. The generic <T extends { display_condition?: Condition | null }> on resolveApplicableFields provides compile-time safety.

---

Summary

Category	Count
🚨 Critical (must fix)	3
⚠️ Warning (should fix)	5
💡 Suggestions (nits)	5
✅ Good practices noted	7

Overall assessment: This is a well-engineered feature. The core evaluator is pure, thoroughly tested, and mirrors backend behavior faithfully. The React integration layer has a few race conditions and edge-case concerns that should be addressed before merge, but nothing that fundamentally undermines the design.

Recommendation: Address issues 1.1, 1.2 (add console logging), and 2.1 (ref update timing) before merge. The remaining warnings are low-risk but worth fixing in a follow-up.

[jpople]

@rayharnett Updated. The changes:

Addressed

1.1 — Silent error swallowing in evaluateCondition
Removed try/catch from evaluateCondition and isFieldApplicable. Errors now propagate to useApplicableFields, which catches them and sets a conditionError flag that surfaces via
the existing ("Something went wrong…") in both form components.

1.2 — Silent cycle guard in resolveApplicableFields
Cycle guard now throws instead of silently returning a partial result. Caught by the same error path as 1.1.

1.3 — hasValue behavior for false/0
Verified against backend. Backend exists operator checks only is not None, but _submitted_has_value() also treats "" and [] as absent — matching our frontend behavior. false and 0 don't occur as form values. Updated the comment to document the nuance accurately (removed misleading "matching backend behavior" claim).

2.1 — Ref update race window
Moved applicableFieldsRef.current assignment into the useEffect alongside value clearing, so both happen atomically.

2.2 — setsEqual no early exit
Replaced forEach + flag with for...of + early return.

2.3 — JSON.stringify circular ref risk
Added comment noting formValues are typed string | string[] — no circular ref risk with current types.

2.4 — Stale schema cache
Added configSignature (sorted field keys) to the cache key so it invalidates when field config changes.

Skipped (no action needed)

• 3.1 (Document hasValue) — Already addressed as part of 1.3.
• 3.2 (Builder pattern for buildCustomFieldsValidationSchema) — Style preference per the review; current approach works fine.
• 3.3 (Hook integration tests) — Agree these would be valuable but out of scope for this PR.
• 3.4 (visibility.ts deletion check) — Already confirmed clean in the review.

[rayharnett]

Good to go.