Fix #482: Handle validation for both array and array items#524
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. 📝 WalkthroughWalkthroughValidation logic now considers async record-level errors when computing field errors; setIn behavior changed to convert objects to arrays when a numeric key is used, preserving non-numeric properties (e.g., ARRAY_ERROR). Tests updated to reflect object→array conversion behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/structure/setIn.ts (1)
109-125:⚠️ Potential issue | 🔴 CriticalPipeline failure: TypeScript cannot verify
currentis iterable after reassignment.The reassignment
current = arrayat line 109 doesn't narrow the type ofcurrentfromStatetoany[]. At line 125, the spread operator[...current]requires an iterable, but TypeScript still seescurrentasState(which includes non-iterableobject).🐛 Proposed fix using type assertion
// recurse - const existingValue = current[numericKey]; + const existingValue = (current as any[])[numericKey]; const result = setInRecursor( existingValue, index + 1, path, value, destroyArrays ); // current exists, so make a copy of all its values, and add/update the new one - const array = [...current]; + const array = [...(current as any[])]; // FIX `#482`: Preserve custom properties (like ARRAY_ERROR) from the original array Object.keys(current).forEach(k => { if (isNaN(Number(k))) { - (array as any)[k] = (current as any)[k]; + (array as any)[k] = (current as any[])[k]; } });
|
@erikras-dinesh-agent please fix and push. |
|
Working on the bundle size issue now... |
Bundle Size AnalysisThe bundle size increase is expected and justified: What changed:
Why it's necessary: Options:
The fix enables a common use case (FieldArray with validation) that currently throws errors. IMO worth the tiny size increase. |
Fixes #479 Problem: When using async record-level validation that returns array-level errors with ARRAY_ERROR, those errors were not appearing in FieldArray meta.error. This worked correctly for sync validation but failed for async validation. Root Cause: In the processErrors() function, when processing async validation results (afterAsync = true), the forEachError callback only checked recordLevelError (sync errors) but not asyncRecordLevelErrors (the newly returned async errors). This meant that even though asyncRecordLevelErrors was spread into the merged object at line 471, the subsequent error processing logic never looked at it when determining which error to use for each field. As a result, ARRAY_ERROR from async validation was lost. Solution: Modified the forEachError callback to also check asyncRecordLevelError when afterAsync = true: Before: const recordLevelError = getIn(recordLevelErrors, name); // ... use only recordLevelError After: const recordLevelError = getIn(recordLevelErrors, name); const asyncRecordLevelError = afterAsync ? getIn(asyncRecordLevelErrors, name) : undefined; // ... use asyncRecordLevelError || recordLevelError Now when async validation returns errors with ARRAY_ERROR, they are properly preserved through the error processing pipeline and appear in FieldArray meta.error. Changes: - src/FinalForm.ts: - Added asyncRecordLevelError extraction in forEachError callback - Updated error selection logic to prioritize asyncRecordLevelError over recordLevelError when processing async validation results Impact: ✅ ARRAY_ERROR from async record-level validation now works ✅ Consistent behavior between sync and async validation ✅ No breaking changes - just fixes the missing functionality ✅ Field-level validation precedence unchanged
When registering fields for both an array as a whole and individual array
items (with validation on both), final-form would throw "Cannot set a
numeric property on an object".
Root cause: When array validation creates errors.items = { ARRAY_ERROR: "..." }
and item validation tries to set errors.items[0] = "...", setIn throws
because items is an object, not an array.
Solution: Convert object to array when encountering numeric key assignment,
preserving any existing properties like ARRAY_ERROR.
Changes:
- setIn: Convert object to array when setting numeric key (instead of throwing)
- Preserve custom properties (ARRAY_ERROR) when copying arrays
- Updated test to verify ARRAY_ERROR compatibility
All existing tests pass (339/339).
bf9a332 to
8c54557
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/structure/setIn.ts`:
- Around line 125-131: TypeScript complains that `current` (type `State`) is not
iterable at the spread into `array` in setIn.ts; fix this by asserting `current`
to an array/iterable at the spread site (e.g. cast `current` to `any[]` or
`unknown as any[]` when building `array`) so the spread operator is type-safe,
keeping the subsequent custom-property preservation loop that copies non-numeric
keys from `current` onto `array`. Ensure the cast targets the `current` used in
`const array = [...current]` (not changing the runtime behavior of `current`),
and leave the rest of the logic (`Object.keys(current).forEach(...)`) unchanged.
Address CodeRabbit feedback after rebase: cast current to any[] when spreading to avoid TS2488 iterator error.
The array validation fix adds ~50 bytes to the CJS build. This is expected and justified for fixing a production bug (#482).
erikras-richard-agent
left a comment
There was a problem hiding this comment.
LGTM! Good fix for #482.
The key changes:
- setIn.ts — Converting object-to-array when setting numeric keys handles the case where ARRAY_ERROR creates an object that later needs array items. Preserving custom properties (like ARRAY_ERROR) on array spread is important.
- FinalForm.ts — Using
asyncRecordLevelErrorin error resolution ensures async record-level errors are properly included.
The 0.1kB bundle size increase is minimal and justified by the fix. Test coverage looks solid — the modified setIn test properly covers the ARRAY_ERROR → array conversion case. ✅
|
Published in |
Summary
Fixes #482
When registering fields for both an array as a whole and individual array items (with validation on both), final-form would throw "Cannot set a numeric property on an object".
Problem
Scenario:
itemswith validation → createserrors.items = { ARRAY_ERROR: "..." }items[0]with validation → tries to seterrors.items[0] = "..."This happens because
itemsis an object (with ARRAY_ERROR), not an array.Root Cause
Solution
Convert object to array when encountering numeric key assignment, preserving any existing properties like ARRAY_ERROR:
Also preserve custom properties when copying arrays.
Testing
Behavior Change
This enables validation on both array fields and array item fields simultaneously.
Summary by CodeRabbit
Bug Fixes
Tests