[7852] Consider embedded items in node selector validator#4816
Open
jvega190 wants to merge 12 commits intocraftercms:developfrom
Open
[7852] Consider embedded items in node selector validator#4816jvega190 wants to merge 12 commits intocraftercms:developfrom
jvega190 wants to merge 12 commits intocraftercms:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPropagates richer validator context ({ siteId, contentTypesById }) through FormsEngine field initialization and value-setting, adds a nodeSelectorValidator that validates embedded items using contentTypesById, and simplifies length display logic by removing min/range formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as FormsEngine (UI)
participant FU as formUtils.setFieldAtoms
participant VA as validators.nodeSelectorValidator
participant CT as contentTypesById (Lookup)
participant VF as validateFieldValue
UI->>FU: setFieldAtoms(fieldValue, validatorsData{siteId, contentTypesById})
FU->>VA: if field.type == "node-selector", call nodeSelectorValidator(currentValue, meta)
VA->>CT: lookup content type for each embedded item via meta.contentTypesById
VA->>VF: for each embedded field, call validateFieldValue(field, value, messages, meta)
VF-->>VA: per-field validation results
VA-->>FU: aggregated validity and messages
FU-->>UI: update field atoms with validation state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/app/src/components/FormsEngine/FormsEngine.tsx (1)
354-363:⚠️ Potential issue | 🟡 MinorStale closure risk: use
effectRefs.current.contentTypesByIdconsistentlyLines 362 and 510 use the outer-scope
contentTypesByIdclosure variable, while the create-mode branch (line 424) correctly readseffectRefs.current.contentTypesByIdso it reflects the latest value even if the reference changes after the effect runs. The subscription callback at line 510 fires asynchronously (after network I/O), widening the staleness window compared with the synchronous repeat-mode path.♻️ Proposed fix (repeat mode, line 362)
- { siteId, contentTypesById } + { siteId, contentTypesById: effectRefs.current.contentTypesById }♻️ Proposed fix (update mode subscription, line 510)
- { siteId, contentTypesById } + { siteId, contentTypesById: effectRefs.current.contentTypesById }Also applies to: 502-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/FormsEngine.tsx` around lines 354 - 363, The closure uses the stale outer-scope contentTypesById inside the atomValueCreator passed to createParsedValuesObject (and similarly in the async subscription callback) which can diverge from the latest state; update those call sites to read effectRefs.current.contentTypesById instead of the outer contentTypesById so setFieldAtoms and any other helpers always receive the up-to-date map (i.e., change the reference in the atomValueCreator used with createParsedValuesObject and the subscription callback where contentTypesById is referenced to effectRefs.current.contentTypesById).ui/app/src/components/FormsEngine/lib/formUtils.tsx (1)
843-848:⚠️ Potential issue | 🔴 Critical
prepareEmbeddedItemFormcallscreateFieldAtomswithoutvalidatorsData— causes crash when embedded stacked forms contain node-selector fieldsWhen an embedded stacked form is opened and contains a
node-selectorfield with embedded items, the validation at line 256 in validators.ts crashes:meta.contentTypesById[contentTypeId]throwsTypeError: Cannot read properties of undefined (reading 'contentTypeId')becausecreateFieldAtomsat line 846 receives novalidatorsData.Fix by plumbing
siteIdandcontentTypesByIdthroughprepareEmbeddedItemForm:🔧 Proposed fix
export function prepareEmbeddedItemForm(props: { username: string; contentType: ContentType; locked: boolean; lockError: ApiResponse; affectedPackages: PublishPackage[]; update: FormsEngineProps['update']; parentStackData: StableFormContextProps; stableFormContextRef: RefObject<StableFormContextProps>; parentPathInSite: string; + siteId?: string; + contentTypesById?: LookupTable<ContentType>; }): { atoms: FormsEngineAtoms; values: LookupTable<unknown>; itemMeta: FormsEngineItemMetaContextProps } { const { username, contentType, update, parentStackData, stableFormContextRef, parentPathInSite, locked, lockError, - affectedPackages + affectedPackages, + siteId, + contentTypesById } = props; ... - const [valueAtom, validityAtom] = createFieldAtoms(contentType.fields[fieldId], value, stableFormContextRef); + const [valueAtom, validityAtom] = createFieldAtoms( + contentType.fields[fieldId], + value, + stableFormContextRef, + siteId && contentTypesById ? { siteId, contentTypesById } : undefined + );Then update the call site at FormsEngine.tsx:400 to pass
siteIdandcontentTypesById: effectRefs.current.contentTypesById.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx` around lines 843 - 848, prepareEmbeddedItemForm calls createFieldAtoms without passing validatorsData, causing validators (e.g., in validators.ts) to access undefined meta.contentTypesById for node-selector fields; update prepareEmbeddedItemForm to accept validatorsData (or explicit siteId and contentTypesById) and pass that through to createFieldAtoms wherever it's called, and then update the call site in FormsEngine.tsx (around the create/prepare invocation currently at line ~400) to supply siteId and contentTypesById: effectRefs.current.contentTypesById so createFieldAtoms receives validatorsData and validators no longer crash when resolving content types.
🧹 Nitpick comments (3)
ui/app/src/components/FormsEngine/lib/formUtils.tsx (1)
184-187:createFieldAtomsvalidatorsDatais optional butsetFieldAtomsvalidatorsDatais required — asymmetric contract
setFieldAtoms(lines 496–499) now requiresvalidatorsData(no?), whilecreateFieldAtoms(line 184) keeps it optional. The intended callers that bypasssetFieldAtomsand callcreateFieldAtomsdirectly (e.g.,prepareEmbeddedItemForm) can silently omitvalidatorsData. Making the contract explicit oncreateFieldAtomsas well — or at minimum adding a JSDoc note — would prevent future callers from accidentally skipping it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx` around lines 184 - 187, The createFieldAtoms function currently declares validatorsData as optional while setFieldAtoms requires it, creating an asymmetric contract that lets callers like prepareEmbeddedItemForm omit validatorsData; update createFieldAtoms to require validatorsData (remove the optional ? from the validatorsData parameter/type) so its signature matches setFieldAtoms, or alternatively add a clear JSDoc on createFieldAtoms stating validatorsData is mandatory and callers must pass it (reference createFieldAtoms, setFieldAtoms, validatorsData, and prepareEmbeddedItemForm when making the change).ui/app/src/components/FormsEngine/lib/validators.ts (2)
267-277:invalidEmbeddedis redundant withisValidBoth
invalidEmbeddedandisValidare flipped in the same condition, soinvalidEmbeddedis always equivalent to!isValidafter the loop. The extra variable can be removed:♻️ Proposed simplification
- let invalidEmbedded = false; results.forEach((result) => { if (!result.isValid) { - invalidEmbedded = true; isValid = false; } }); - if (invalidEmbedded) { + if (!isValid) { messages.push(defineMessage({ defaultMessage: 'One or more embedded content items are invalid.' })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 267 - 277, The variable invalidEmbedded in the validation aggregation loop is redundant because it mirrors the same state as isValid; remove invalidEmbedded and instead, after awaiting Promise.all(validationPromises) and iterating results to set isValid = false when result.isValid is false, conditionally push the message using !isValid (e.g., if (!isValid) messages.push(defineMessage(...))). Update the loop and the subsequent conditional to reference only isValid and remove all uses/declarations of invalidEmbedded (symbols to edit: validationPromises, results, isValid, messages.push, defineMessage).
260-264:fieldparameter shadowed by loop variableThe outer
fieldparameter (the node-selector field, line 242) is shadowed by the innerfieldbinding in theforEachcallback. This will trigger anno-shadowESLint rule and is confusing when reading the code.♻️ Proposed fix
- Object.values(fields).forEach((field) => { - const value = component[field.id]; - const validationPromise = validateFieldValue(field, value, meta); + Object.values(fields).forEach((embeddedField) => { + const value = component[embeddedField.id]; + const validationPromise = validateFieldValue(embeddedField, value, meta); validationPromises.push(validationPromise); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 260 - 264, The loop shadows the outer parameter named field (the node-selector field) by reusing the identifier in Object.values(fields).forEach; rename the inner loop variable (e.g., to childField or subField) wherever it's used inside that callback so it no longer conflicts with the outer field parameter, and ensure calls using that variable (validateFieldValue(...), const value = component[...] and any references to field.id) are updated to the new name; keep validateFieldValue, validationPromises, fields, component, and meta usage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 254-257: The loop in embeddedContent.forEach accesses
meta.contentTypesById without guarding for undefined, causing a crash; update
the validator in validators.ts so that within the embeddedContent.forEach
callback you first check that meta?.contentTypesById exists (and optionally that
component['content-type'] is defined) and bail out or skip that embedded item
when it's missing, then only read meta.contentTypesById[contentTypeId] to get
contentType; this will prevent the TypeError until the root cause in
prepareEmbeddedItemForm/createFieldAtoms and ValidatorMetaData is fixed.
- Around line 241-279: nodeSelectorValidator can recurse indefinitely via
validateFieldValue for embedded node-selectors; add a simple runtime guard on
the meta object to prevent deep or circular recursion: inside
nodeSelectorValidator check or initialize a meta._validationDepth (number) or
meta._visitedContentTypeIds (Set<string>), increment depth (or add
contentTypeId) before calling validateFieldValue for embedded items and
decrement (or remove) after, and if the depth exceeds a small MAX_DEPTH (e.g.,
10) or the contentTypeId is already in the visited set, short-circuit by marking
the embedded item invalid and push a message into messages (or skip validation)
so recursion stops; reference nodeSelectorValidator, validateFieldValue,
messages, and ValidatorMetaData when making this change.
---
Outside diff comments:
In `@ui/app/src/components/FormsEngine/FormsEngine.tsx`:
- Around line 354-363: The closure uses the stale outer-scope contentTypesById
inside the atomValueCreator passed to createParsedValuesObject (and similarly in
the async subscription callback) which can diverge from the latest state; update
those call sites to read effectRefs.current.contentTypesById instead of the
outer contentTypesById so setFieldAtoms and any other helpers always receive the
up-to-date map (i.e., change the reference in the atomValueCreator used with
createParsedValuesObject and the subscription callback where contentTypesById is
referenced to effectRefs.current.contentTypesById).
In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx`:
- Around line 843-848: prepareEmbeddedItemForm calls createFieldAtoms without
passing validatorsData, causing validators (e.g., in validators.ts) to access
undefined meta.contentTypesById for node-selector fields; update
prepareEmbeddedItemForm to accept validatorsData (or explicit siteId and
contentTypesById) and pass that through to createFieldAtoms wherever it's
called, and then update the call site in FormsEngine.tsx (around the
create/prepare invocation currently at line ~400) to supply siteId and
contentTypesById: effectRefs.current.contentTypesById so createFieldAtoms
receives validatorsData and validators no longer crash when resolving content
types.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx`:
- Around line 184-187: The createFieldAtoms function currently declares
validatorsData as optional while setFieldAtoms requires it, creating an
asymmetric contract that lets callers like prepareEmbeddedItemForm omit
validatorsData; update createFieldAtoms to require validatorsData (remove the
optional ? from the validatorsData parameter/type) so its signature matches
setFieldAtoms, or alternatively add a clear JSDoc on createFieldAtoms stating
validatorsData is mandatory and callers must pass it (reference
createFieldAtoms, setFieldAtoms, validatorsData, and prepareEmbeddedItemForm
when making the change).
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 267-277: The variable invalidEmbedded in the validation
aggregation loop is redundant because it mirrors the same state as isValid;
remove invalidEmbedded and instead, after awaiting
Promise.all(validationPromises) and iterating results to set isValid = false
when result.isValid is false, conditionally push the message using !isValid
(e.g., if (!isValid) messages.push(defineMessage(...))). Update the loop and the
subsequent conditional to reference only isValid and remove all
uses/declarations of invalidEmbedded (symbols to edit: validationPromises,
results, isValid, messages.push, defineMessage).
- Around line 260-264: The loop shadows the outer parameter named field (the
node-selector field) by reusing the identifier in Object.values(fields).forEach;
rename the inner loop variable (e.g., to childField or subField) wherever it's
used inside that callback so it no longer conflicts with the outer field
parameter, and ensure calls using that variable (validateFieldValue(...), const
value = component[...] and any references to field.id) are updated to the new
name; keep validateFieldValue, validationPromises, fields, component, and meta
usage intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ui/app/src/components/FormsEngine/lib/formUtils.tsx (1)
184-187: Duplicated inlinevalidatorsDatatype — extract to a named typeThe same anonymous object type
{ siteId: string; contentTypesById: LookupTable<ContentType> }appears verbatim in bothcreateFieldAtomsandsetFieldAtoms. Consider extracting it (or reusingValidatorMetaDataonce its fields are made optional).♻️ Proposed refactor
In validators.ts, export the relevant subset:
-interface ValidatorMetaData { +export interface ValidatorMetaData { siteId?: string; ... contentTypesById?: LookupTable<ContentType>; } +export type ValidatorsData = Pick<ValidatorMetaData, 'siteId' | 'contentTypesById'>;Then in formUtils.tsx:
+import type { ValidatorsData } from './validators'; export function createFieldAtoms( field: ContentTypeField, initialValue: unknown, formContextRef: RefObject<...>, - validatorsData?: { - siteId: string; - contentTypesById: LookupTable<ContentType>; - } + validatorsData?: ValidatorsData ) export function setFieldAtoms( ... - validatorsData?: { - siteId: string; - contentTypesById: LookupTable<ContentType>; - } + validatorsData?: ValidatorsData )Also applies to: 496-499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx` around lines 184 - 187, The inline type used for validatorsData is duplicated in createFieldAtoms and setFieldAtoms; extract it to a single named export (e.g., ValidatorsData or make ValidatorMetaData's fields optional and reuse that) in validators.ts and import it into formUtils.tsx, then replace both anonymous object types in the createFieldAtoms and setFieldAtoms signatures with the new named type so both functions reference the same type definition.ui/app/src/components/FormsEngine/lib/validators.ts (2)
277-281: Innerfieldparameter shadows the outerfieldparameter ofnodeSelectorValidatorThe
forEachcallback on line 277 names its parameterfield, shadowing the outerfield: ContentTypeField(line 242). While not a runtime bug, it obscures which field is being validated and can confuse readers or future refactors.♻️ Proposed fix
- Object.values(fields).forEach((field) => { - const value = component[field.id]; - const validationPromise = validateFieldValue(field, value, meta); + Object.values(fields).forEach((embeddedField) => { + const value = component[embeddedField.id]; + const validationPromise = validateFieldValue(embeddedField, value, meta); validationPromises.push(validationPromise); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 277 - 281, The inner forEach callback in nodeSelectorValidator is shadowing the outer parameter named field (ContentTypeField); rename the callback parameter (e.g., to childField or fieldItem) and update its usages (component[childField.id], validateFieldValue(childField, ...), validationPromises.push(...)) so the outer field variable is not shadowed and the intent is clear in validateFieldValue calls and the component access.
33-38:siteIdandcontentTypesByIdshould be optional inValidatorMetaDatato match actual call sites
formUtils.tsxconstructs the meta object withvalidatorsData?.siteIdandvalidatorsData?.contentTypesById, both of which resolve toundefinedwhenvalidatorsDatais absent (e.g.,prepareEmbeddedItemFormline 846). WithstrictNullChecks, this is a type error; without it, the type contract is silently broken at runtime.fileNameValidatoralready guards withnou(siteId)(line 118), which acknowledgessiteIdcan be absent in practice.🛡️ Proposed fix
interface ValidatorMetaData { - siteId: string; + siteId?: string; fileName: string; itemMeta: FormsEngineItemMetaContextProps; - contentTypesById: LookupTable<ContentType>; + contentTypesById?: LookupTable<ContentType>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 33 - 38, ValidatorMetaData declares siteId and contentTypesById as required but call sites (e.g., prepareEmbeddedItemForm in formUtils.tsx) pass validatorsData?.siteId and validatorsData?.contentTypesById which may be undefined; update the ValidatorMetaData interface so siteId?: string and contentTypesById?: LookupTable<ContentType> to reflect optionality, leaving fileNameValidator (which already checks nou(siteId)) and any other validators intact so they continue to guard for missing values at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 248-263: nodeSelectorValidator currently assumes currentValue is
an array and crashes when currentValue is null/undefined; update
nodeSelectorValidator (used by validateFieldValue) to treat a null/undefined
currentValue as an empty array before accessing .filter or .length—e.g.,
normalize currentValue at the top (const current = Array.isArray(currentValue) ?
currentValue : []) then use current for embeddedContent, current.length and the
minSize/maxSize checks (referencing minSize, maxSize and embeddedContent) so
non-required node-selector fields no longer throw TypeError.
---
Duplicate comments:
In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx`:
- Around line 843-848: prepareEmbeddedItemForm is calling createFieldAtoms
without passing validatorsData, causing meta.contentTypesById to be undefined
when validation runs (e.g., nodeSelectorValidator). Update
prepareEmbeddedItemForm so that when you call
createFieldAtoms(contentType.fields[fieldId], value, stableFormContextRef) you
also pass the validatorsData object (the same validatorsData used elsewhere in
the form setup) so createFieldAtoms receives validatorsData and validators can
access meta.contentTypesById; ensure the passed variable matches the one used in
surrounding code so nodeSelectorValidator and other validators can dereference
meta.contentTypesById safely.
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 271-274: Add a null guard before accessing meta.contentTypesById
in the validators logic: check if meta.contentTypesById is falsy and return the
current validity (isValid) early to avoid a TypeError when contentTypesById is
undefined. Update the block around embeddedContent.forEach (referencing meta,
contentTypesById, and component) to short-circuit if meta.contentTypesById is
missing; this aligns with callers like prepareEmbeddedItemForm/createFieldAtoms
that may pass validatorsData-less meta.
- Around line 241-296: nodeSelectorValidator currently allows unbounded
recursion via validateFieldValue -> nodeSelectorValidator for embedded
components; add a recursion guard by threading a visitation or depth limit
through ValidatorMetaData and checking it before recursing. Concretely: extend
ValidatorMetaData with either a visitedContentTypeIds: Set<string> (or a
validationDepth number and maxValidationDepth) and update validateFieldValue and
nodeSelectorValidator to accept/use that meta; before validating an embedded
component in nodeSelectorValidator (where you read component['content-type'] and
call validateFieldValue), check/mark the contentTypeId in visitedContentTypeIds
(or increment depth) and skip or fail validation if revisiting or exceeding
maxValidationDepth, and ensure you unmark/decrement after the recursive call so
sibling branches are validated correctly. This prevents stack overflows from
deeply nested or circular node-selector graphs while keeping existing validation
logic intact.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx`:
- Around line 184-187: The inline type used for validatorsData is duplicated in
createFieldAtoms and setFieldAtoms; extract it to a single named export (e.g.,
ValidatorsData or make ValidatorMetaData's fields optional and reuse that) in
validators.ts and import it into formUtils.tsx, then replace both anonymous
object types in the createFieldAtoms and setFieldAtoms signatures with the new
named type so both functions reference the same type definition.
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 277-281: The inner forEach callback in nodeSelectorValidator is
shadowing the outer parameter named field (ContentTypeField); rename the
callback parameter (e.g., to childField or fieldItem) and update its usages
(component[childField.id], validateFieldValue(childField, ...),
validationPromises.push(...)) so the outer field variable is not shadowed and
the intent is clear in validateFieldValue calls and the component access.
- Around line 33-38: ValidatorMetaData declares siteId and contentTypesById as
required but call sites (e.g., prepareEmbeddedItemForm in formUtils.tsx) pass
validatorsData?.siteId and validatorsData?.contentTypesById which may be
undefined; update the ValidatorMetaData interface so siteId?: string and
contentTypesById?: LookupTable<ContentType> to reflect optionality, leaving
fileNameValidator (which already checks nou(siteId)) and any other validators
intact so they continue to guard for missing values at runtime.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
283-283: Variablefieldshadows the outer parameter on line 242The inner loop variable
fieldshadows thenodeSelectorValidatorparameter of the same name. While functional (the outerfieldisn't used after this point), it hurts readability. Consider renaming toembeddedField.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` at line 283, The inner forEach callback uses a parameter named `field` which shadows the outer `nodeSelectorValidator` parameter; rename the inner loop variable to something clearer like `embeddedField` in the Object.values(fields).forEach(...) callback to avoid shadowing and improve readability—update all usages inside that callback accordingly (reference: nodeSelectorValidator and the Object.values(fields).forEach(...) callback in validators.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 275-288: The current loop over embeddedContent incorrectly skips
sibling embedded items by using the local visited set keyed only by
contentTypeId; remove the per-contentTypeId early return (the
visited.has/contentTypeId logic) so every embedded item is validated inside the
embeddedContent.forEach block (keeps calling validateFieldValue for each
component instance), and to prevent infinite recursion propagate a shared
recursion guard through meta (e.g., add a meta.recursionVisited or meta.depth
counter and pass it into validateFieldValue and nodeSelectorValidator) so
recursive calls consult the shared guard instead of a local visited variable.
- Line 248: The Array.isArray guard around currentValue is correct and should be
kept to avoid TypeError on null/undefined; ensure the check "if
(!Array.isArray(currentValue)) return isValid;" remains in validators.ts so any
early-return preserves the existing isValid flow for the validator that reads
currentValue and isValid.
- Line 270: You added a guard "if (!meta.contentTypesById) return isValid;" to
prevent a TypeError when meta.contentTypesById is undefined; keep this guard in
the validators.ts function where meta and isValid are used (referencing
meta.contentTypesById and the surrounding validation logic) to short-circuit the
validator and avoid accessing contentTypesById when it's missing.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Line 283: The inner forEach callback uses a parameter named `field` which
shadows the outer `nodeSelectorValidator` parameter; rename the inner loop
variable to something clearer like `embeddedField` in the
Object.values(fields).forEach(...) callback to avoid shadowing and improve
readability—update all usages inside that callback accordingly (reference:
nodeSelectorValidator and the Object.values(fields).forEach(...) callback in
validators.ts).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
272-285:⚠️ Potential issue | 🟠 MajorVisited set skips all-but-first instances of the same content type at the same level, and fails to guard against cross-level recursion.
Two distinct problems share the same root cause:
Sibling-validation bug (new regression):
visitedis scoped to a singlenodeSelectorValidatorcall. Once the first item of content typeXis processed,visited.has(contentTypeId)istruefor every subsequent item of typeXin the same node-selector — those items are silently skipped. A node-selector containing three "Banner" components would only validate the first one.Recursion guard is ineffective (continuing concern from previous review): Because
visitedis anew Set()local to each invocation, recursive call chains (nodeSelectorValidator → validateFieldValue → nodeSelectorValidator) each create a fresh empty set. A content type that embeds itself (A → A) is never detected as circular and will recurse unboundedly until the call stack is exhausted.The fix should validate every sibling item individually and use a depth counter threaded via
metato bound recursion across call levels:🛡️ Suggested approach
Add
_embeddedValidationDepthtoValidatorMetaData:interface ValidatorMetaData { siteId: string; fileName: string; itemMeta: FormsEngineItemMetaContextProps; contentTypesById: LookupTable<ContentType>; + _embeddedValidationDepth?: number; }Then update
nodeSelectorValidator:export async function nodeSelectorValidator( field: ContentTypeField, currentValue: Array<NodeSelectorItem>, messages: FieldValidityState['messages'], meta: ValidatorMetaData ): Promise<boolean> { let isValid = true; if (!Array.isArray(currentValue)) return isValid; - // This set is used to keep track of visited content items during validation to prevent infinite loops in case of circular references. - const visited = new Set<string>(); const minSize: number = getPropertyValue(field.properties, 'minSize') as number; const maxSize: number = getPropertyValue(field.properties, 'maxSize') as number; const embeddedContent = currentValue.filter((item) => nnou(item.component)); // ...min/max checks... if (embeddedContent.length === 0) return isValid; if (!meta.contentTypesById) return isValid; + + const MAX_DEPTH = 5; + const capturedDepth = meta._embeddedValidationDepth ?? 0; + if (capturedDepth >= MAX_DEPTH) return isValid; + meta._embeddedValidationDepth = capturedDepth + 1; const validationPromises: Promise<FieldValidityState>[] = []; embeddedContent.forEach(({ component }) => { const contentTypeId = component['content-type'] as string; - if (visited.has(contentTypeId)) return; // prevent circular validation - visited.add(contentTypeId); const contentType = meta.contentTypesById[contentTypeId]; if (!contentType) return; const fields = contentType.fields; if (!fields) return; Object.values(fields).forEach((field) => { const value = component[field.id]; const validationPromise = validateFieldValue(field, value, meta); validationPromises.push(validationPromise); }); }); const results = await Promise.all(validationPromises); + meta._embeddedValidationDepth = capturedDepth; // ...rest unchanged... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 272 - 285, The visited Set in nodeSelectorValidator incorrectly skips sibling items of the same content type and fails to prevent cross-call recursion; add a numeric _embeddedValidationDepth field to ValidatorMetaData and thread it via meta to bound recursion across calls, remove the per-call visited check that prevents validating later siblings (instead validate every embedded component instance by iterating embeddedContent and calling validateFieldValue for each), and at the start of nodeSelectorValidator increment meta._embeddedValidationDepth, check it against a safe max (e.g., 10) to abort further descent if exceeded, then decrement it before returning; update any references to meta in validateFieldValue/nodeSelectorValidator to rely on meta._embeddedValidationDepth rather than the local visited Set to both validate all siblings and prevent infinite recursion.
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
33-38: Consider makingcontentTypesByIdoptional to align the type with runtime reality.The null guard added at line 267 (
if (!meta.contentTypesById) return isValid) proves thatcontentTypesByIdcan beundefinedat runtime (e.g., whenprepareEmbeddedItemFormcallscreateFieldAtomswithoutvalidatorsData). Declaring it as required contradicts this, and TypeScript will treat the guard at line 267 as always-false.♻️ Suggested change
interface ValidatorMetaData { siteId: string; fileName: string; itemMeta: FormsEngineItemMetaContextProps; - contentTypesById: LookupTable<ContentType>; + contentTypesById?: LookupTable<ContentType>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 33 - 38, The ValidatorMetaData interface currently requires contentTypesById but runtime paths (e.g., prepareEmbeddedItemForm → createFieldAtoms when validatorsData is absent) allow it to be undefined (see the null guard checking meta.contentTypesById), so update ValidatorMetaData to make contentTypesById optional (contentTypesById?: LookupTable<ContentType>) and then audit usages of ValidatorMetaData, createFieldAtoms, and any callers like prepareEmbeddedItemForm or validatorsData to ensure they handle an optional contentTypesById (or provide a safe default) rather than assuming it’s always present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 256-262: The user-facing validation messages in validators.ts
within the size-checking logic (the block that checks
nnou(minSize)/nnou(maxSize) and uses currentValue.length) incorrectly use
"items(s)"; update the defineMessage calls to use "item(s)" instead so the
messages read "At least {minSize} item(s) are required." and "No more than
{maxSize} item(s) are allowed."; adjust the two defineMessage strings where
messages?.push is called (the calls that reference defineMessage, minSize, and
maxSize) to correct the typo.
---
Duplicate comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 272-285: The visited Set in nodeSelectorValidator incorrectly
skips sibling items of the same content type and fails to prevent cross-call
recursion; add a numeric _embeddedValidationDepth field to ValidatorMetaData and
thread it via meta to bound recursion across calls, remove the per-call visited
check that prevents validating later siblings (instead validate every embedded
component instance by iterating embeddedContent and calling validateFieldValue
for each), and at the start of nodeSelectorValidator increment
meta._embeddedValidationDepth, check it against a safe max (e.g., 10) to abort
further descent if exceeded, then decrement it before returning; update any
references to meta in validateFieldValue/nodeSelectorValidator to rely on
meta._embeddedValidationDepth rather than the local visited Set to both validate
all siblings and prevent infinite recursion.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 33-38: The ValidatorMetaData interface currently requires
contentTypesById but runtime paths (e.g., prepareEmbeddedItemForm →
createFieldAtoms when validatorsData is absent) allow it to be undefined (see
the null guard checking meta.contentTypesById), so update ValidatorMetaData to
make contentTypesById optional (contentTypesById?: LookupTable<ContentType>) and
then audit usages of ValidatorMetaData, createFieldAtoms, and any callers like
prepareEmbeddedItemForm or validatorsData to ensure they handle an optional
contentTypesById (or provide a safe default) rather than assuming it’s always
present.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/app/src/components/FormsEngine/components/FormsEngineField.tsxui/app/src/components/FormsEngine/lib/validators.ts
…o bugfix/7852 # Conflicts: # ui/app/src/components/FormsEngine/lib/validators.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ui/app/src/components/FormsEngine/lib/validators.ts (2)
313-313:⚠️ Potential issue | 🟡 MinorTypo in user-facing validation message:
"items(s)"→"item(s)".Line 313 uses
items(s)while line 317 correctly usesitem(s). This should be consistent.🐛 Proposed fix
- messages?.push([defineMessage({ defaultMessage: 'At least {minSize} items(s) are required.' }), { minSize }]); + messages?.push([defineMessage({ defaultMessage: 'At least {minSize} item(s) are required.' }), { minSize }]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` at line 313, The user-facing validation message pushed via messages?.push([...defineMessage({ defaultMessage: 'At least {minSize} items(s) are required.' })...]) contains a typo "items(s)"; update the defaultMessage to "item(s)" to match the other message and ensure consistency (locate the messages?.push call and the defineMessage invocation in validators.ts that constructs the "At least {minSize} ..." message and change "items(s)" to "item(s)").
328-341:⚠️ Potential issue | 🟠 MajorCycle detection prevents validation of multiple same-type embedded items.
The
visitedset trackscontentTypeId, but this causes all embedded items after the first of each content type to be skipped entirely. If a node-selector contains 3 "article" embedded components, only the first article's fields are validated.The cycle detection should track individual component instances (e.g., by a unique key like
component.objectIdor path) rather than content type IDs, or the recursion guard should be implemented differently—perhaps by tracking depth or by checking for actual circular references in the component hierarchy rather than just repeated content types.🐛 Proposed fix using component identity instead of content type
export async function nodeSelectorValidator( field: ContentTypeField, currentValue: Array<NodeSelectorItem>, messages: FieldValidityState['messages'], meta: ValidatorMetaData ): Promise<boolean> { let isValid = true; if (!Array.isArray(currentValue)) return isValid; - // This set is used to keep track of visited content items during validation to prevent infinite loops in case of circular references. - const visited = new Set<string>(); const minSize: number = getPropertyValue(field.properties, 'minSize') as number; const maxSize: number = getPropertyValue(field.properties, 'maxSize') as number; const embeddedContent = currentValue.filter((item) => nnou(item.component)); // ... minSize/maxSize checks ... // If there are no embedded items, return validation result (items validation is not needed if there are no items) if (embeddedContent.length === 0) return isValid; if (!meta.contentTypesById) return isValid; const validationPromises: Promise<FieldValidityState>[] = []; // Validate fields of each embedded item embeddedContent.forEach(({ component }) => { const contentTypeId = component['content-type'] as string; - if (visited.has(contentTypeId)) return; // prevent circular validation - visited.add(contentTypeId); const contentType = meta.contentTypesById[contentTypeId]; if (!contentType) return; const fields = contentType.fields; if (!fields) return; Object.values(fields).forEach((field) => { const value = component[field.id]; const validationPromise = validateFieldValue(field, value, meta); validationPromises.push(validationPromise); }); });Note: If true cycle detection for deeply nested structures is needed, consider passing a
visitedComponentsset throughmetathat tracks component object IDs across recursive calls, rather than blocking by content type at the top level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 328 - 341, The cycle guard currently uses the contentTypeId (variable visited and contentTypeId) which skips validation for subsequent embedded components of the same type; change the guard to track individual component instances instead of content types — e.g., derive a unique component key (component.objectId or a path/id available on the component) and use that in the visited set, or accept/propagate a visitedComponents set on meta and store the component identity there before calling validateFieldValue; ensure you still detect real circular references by checking/adding the component identity (not contentTypeId) and removing it on return so legitimate multiple sibling components of the same type are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Line 313: The user-facing validation message pushed via
messages?.push([...defineMessage({ defaultMessage: 'At least {minSize} items(s)
are required.' })...]) contains a typo "items(s)"; update the defaultMessage to
"item(s)" to match the other message and ensure consistency (locate the
messages?.push call and the defineMessage invocation in validators.ts that
constructs the "At least {minSize} ..." message and change "items(s)" to
"item(s)").
- Around line 328-341: The cycle guard currently uses the contentTypeId
(variable visited and contentTypeId) which skips validation for subsequent
embedded components of the same type; change the guard to track individual
component instances instead of content types — e.g., derive a unique component
key (component.objectId or a path/id available on the component) and use that in
the visited set, or accept/propagate a visitedComponents set on meta and store
the component identity there before calling validateFieldValue; ensure you still
detect real circular references by checking/adding the component identity (not
contentTypeId) and removing it on return so legitimate multiple sibling
components of the same type are validated.
…o bugfix/7852 # Conflicts: # ui/app/src/components/FormsEngine/lib/validators.ts
…o bugfix/7852 # Conflicts: # ui/app/src/components/FormsEngine/lib/formUtils.tsx # ui/app/src/components/FormsEngine/lib/validators.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
craftercms/craftercms#7852
Summary by CodeRabbit
New Features
Improvements
Bug Fixes / UI