[8423] Add repeat group validator to check properties and fields of items#4804
Open
jvega190 wants to merge 5 commits intocraftercms:developfrom
Open
[8423] Add repeat group validator to check properties and fields of items#4804jvega190 wants to merge 5 commits intocraftercms:developfrom
jvega190 wants to merge 5 commits intocraftercms:developfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdded a new exported validator, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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
🤖 Fix all issues with 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 line 248-265: The checks for minOccurs and maxOccurs use truthiness
which treats 0 as unset; update the validation in the block around
getPropertyValue(field.properties, 'minOccurs') and 'maxOccurs' to explicitly
test for presence (e.g., minOccurs != null or typeof minOccurs === 'number' /
Number.isFinite(minOccurs)) before comparing currentValue.length, so a
configured 0 is enforced; keep pushing messages via messages?.push with
defineMessage and return false on violation as before.
- Line 247: The repeatGroupValidator currently assumes field.fields exists and
calls Object.values(fields) which can throw if fields is undefined; in the
repeatGroupValidator function (and where const fields = field.fields is
declared) add a defensive guard that returns true when fields is missing (e.g.
if (!fields) return true) so the validator safely handles repeat-group
descriptors without a fields array; alternatively, if you prefer stricter
typing, change the function parameter type from ContentTypeField to
ContentTypeRepeatField to require fields, but the quick fix is to add the guard
before iterating over Object.values(fields).
- Around line 269-288: The aggregation of validation results currently uses a
mutable boolean and a results.forEach loop; replace this with an idiomatic
short-circuiting check using Array.prototype.every: after awaiting
Promise.all(validationPromises) use `results.every(r => r.isValid)` (or return
that directly) to compute and return the final boolean. Update the code around
the variables `validationPromises`, `results`, and
`FieldValidityState`/`validateFieldValue` usage so the function returns the
`every` result instead of the `isValid` flag.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with 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 line 269-288: The aggregation of validation results currently uses a mutable boolean and a results.forEach loop; replace this with an idiomatic short-circuiting check using Array.prototype.every: after awaiting Promise.all(validationPromises) use `results.every(r => r.isValid)` (or return that directly) to compute and return the final boolean. Update the code around the variables `validationPromises`, `results`, and `FieldValidityState`/`validateFieldValue` usage so the function returns the `every` result instead of the `isValid` flag.ui/app/src/components/FormsEngine/lib/validators.ts (1)
269-288: Consider simplifying the validity aggregation.The
forEach+ mutable boolean can be replaced withevery, which is more idiomatic and short-circuits on the first invalid result.Suggested refactor
const results = await Promise.all(validationPromises); - let isValid = true; - results.forEach((result) => { - if (!result.isValid) { - isValid = false; - } - }); - return isValid; + return results.every((result) => result.isValid);🤖 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 269 - 288, The aggregation of validation results currently uses a mutable boolean and a results.forEach loop; replace this with an idiomatic short-circuiting check using Array.prototype.every: after awaiting Promise.all(validationPromises) use `results.every(r => r.isValid)` (or return that directly) to compute and return the final boolean. Update the code around the variables `validationPromises`, `results`, and `FieldValidityState`/`validateFieldValue` usage so the function returns the `every` result instead of the `isValid` flag.
There was a problem hiding this comment.
🤖 Fix all issues with 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 line 247-248: The explicit "as number" casts on minOccurs and maxOccurs
mask getPropertyValue's possible undefined/null return and contradict the nnou()
runtime guard; remove the unsafe casts and declare them with a nullable type
(e.g., number | undefined) or let TypeScript infer the correct type from
getPropertyValue, then use the existing nnou(minOccurs) / nnou(maxOccurs) checks
to narrow before any numeric usage; update any downstream usage sites in this
file to respect the nullable type until nnou() has validated them.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with 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 line 247-248: The explicit "as number" casts on minOccurs and maxOccurs mask getPropertyValue's possible undefined/null return and contradict the nnou() runtime guard; remove the unsafe casts and declare them with a nullable type (e.g., number | undefined) or let TypeScript infer the correct type from getPropertyValue, then use the existing nnou(minOccurs) / nnou(maxOccurs) checks to narrow before any numeric usage; update any downstream usage sites in this file to respect the nullable type until nnou() has validated them.ui/app/src/components/FormsEngine/lib/validators.ts (1)
247-248: Theas numbercast contradicts thennou()guard and masks the actual return type.
getPropertyValuemay returnundefined/null, so the declared type: numberis inaccurate. Althoughnnou()handles this at runtime, the cast silences any future compiler warnings if these variables are used unsafely elsewhere.Suggested fix
- const minOccurs: number = getPropertyValue(field.properties, 'minOccurs') as number; - const maxOccurs: number = getPropertyValue(field.properties, 'maxOccurs') as number; + const minOccurs = getPropertyValue(field.properties, 'minOccurs') as number | undefined; + const maxOccurs = getPropertyValue(field.properties, 'maxOccurs') as number | undefined;🤖 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 247 - 248, The explicit "as number" casts on minOccurs and maxOccurs mask getPropertyValue's possible undefined/null return and contradict the nnou() runtime guard; remove the unsafe casts and declare them with a nullable type (e.g., number | undefined) or let TypeScript infer the correct type from getPropertyValue, then use the existing nnou(minOccurs) / nnou(maxOccurs) checks to narrow before any numeric usage; update any downstream usage sites in this file to respect the nullable type until nnou() has validated them.
rart
reviewed
Feb 19, 2026
Member
rart
left a comment
rart
left a comment
There was a problem hiding this comment.
The validators can return multiple messages. Are we generally stopping after the first error? Are the validation errors able to render multiple errors for a single control?
Member
Author
|
@rart multiple error can render for a single control (is supported and other validators do return multiple errors). Updated the validator to avoid returning after first error. |
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
2 participants
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#8423
Summary by CodeRabbit