fix: Preserve string errors when removing nested field keys in setIn#527
Conversation
Fixes #271 Problem: When setIn tries to remove a nested field key from a parent that contains a string error message (e.g., 'Invalid range' on 'range' when removing 'range.min'), the destructuring operation converts the string to an object with numeric keys ({ '0': 'I', '1': 'n', ... }). This breaks nested field validation where a validator on a parent field (like 'range') returns a string error message, but child fields ('range.min', 'range.max') also exist. Root Cause: Line 61 in setIn.ts: `const { [key]: _removed, ...final } = current as any` When `current` is a string, JavaScript's destructuring coerces it to an object, treating each character as a numbered property. Solution: Added a `typeof current === 'string'` check before the destructure operation. Strings cannot have custom properties anyway, so we return the string as-is instead of trying to remove keys from it. Changes: - src/structure/setIn.ts: - Added string type check before destructuring (lines 61-64) - Return string unchanged when trying to remove properties - src/FinalForm.validating.test.ts: - Added test case for nested field-level validation errors - Verifies 'Invalid range' stays a string, not converted to object Impact: ✅ Nested field validators work correctly ✅ String error messages preserved on parent fields ✅ No breaking changes ✅ All existing tests pass
erikras-richard-agent
left a comment
erikras-richard-agent
left a comment
There was a problem hiding this comment.
LGTM! Simple, targeted fix. The typeof current === 'string' guard prevents destructuring a string error into an empty object.
The test is excellent — demonstrates the exact bug (parent field range with string error, child fields range.min/range.max causing the error to be destroyed) and verifies the fix preserves the error string. ✅
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR addresses a bug where validators applied to nested fields were incorrectly converting string error messages to objects. A test case is added to verify nested field validation behavior, and a guard clause is introduced in the setIn function to preserve string values during nested path resolution. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Published in |
3 participants
Fixes #271
Problem
When setIn tries to remove a nested field key from a parent that contains a string error message (e.g.,
'Invalid range'on'range'when removing'range.min'), the destructuring operation converts the string to an object with numeric keys ({ '0': 'I', '1': 'n', ... }).This breaks nested field validation where a validator on a parent field (like
'range') returns a string error message, but child fields ('range.min','range.max') also exist.Root Cause
Line 61 in setIn.ts:
const { [key]: _removed, ...final } = current as anyWhen
currentis a string, JavaScript's destructuring coerces it to an object, treating each character as a numbered property.Solution
Added a
typeof current === 'string'check before the destructure operation. Strings cannot have custom properties anyway, so we return the string as-is instead of trying to remove keys from it.Changes
src/structure/setIn.ts:src/FinalForm.validating.test.ts:'Invalid range'stays a string, not converted to objectTest Case
The test creates a form with:
rangefield validator that checksmin < maxrange.minandrange.maxmin=10,max=5(invalid)errors = { range: 'Invalid range' }✅Without the fix, the error becomes
{ range: { '0': 'I', '1': 'n', ... } }❌Impact
✅ Nested field validators work correctly
✅ String error messages preserved on parent fields
✅ No breaking changes
✅ All existing tests pass (342 passed)
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests