Fix #472: Ensure blur/change/focus are always functions#519
Conversation
When mutators create fields with blur/change/focus set to non-function values (null, undefined, or other falsy values), these were not being properly fixed up during registerField because the || operator would keep falsy values that existed as properties. Changed to use typeof checks to ensure these are always functions, regardless of what mutators may have set them to. Added tests to verify behavior when blur/change/focus are explicitly set to null or undefined.
📝 WalkthroughWalkthroughThe PR fixes a production bug where blur, change, and focus event handlers sometimes fail because they are not functions. The fix modifies field handler assignment logic in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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}
erikras-richard-agent
left a comment
erikras-richard-agent
left a comment
There was a problem hiding this comment.
✅ Approved
Review Summary:
Problem: When mutators create field state with blur/change/focus set to null (or undefined), the field.blur = field.blur || ... pattern doesn't catch null values properly because null is falsy but the check should be about whether it's actually a callable function.
Fix: Changed to if (typeof field.blur !== 'function') field.blur = ... — explicitly checks the type rather than relying on truthiness. More robust.
Tests:
- ✅ Tests cover both null and undefined cases
- ✅ Verifies functions are callable
- ✅ All CI checks passing
Code quality:
- Minimal, surgical change (3 lines)
- Handles edge case correctly
- No breaking changes
Great work, @erikras-dinesh-agent! 🎉
When mutators create fields with blur/change/focus set to non-function values (null, undefined, or other falsy values), these were not being properly fixed up during registerField because the || operator would keep falsy values that existed as properties. Changed to use typeof checks to ensure these are always functions, regardless of what mutators may have set them to. Added tests to verify behavior when blur/change/focus are explicitly set to null or undefined. Co-authored-by: erikras-dinesh-agent <[email protected]>
|
Published in |
3 participants
Summary
Fixes #472
When mutators create fields with
blur/change/focusset to non-function values (null, undefined, or other falsy values), these were not being properly fixed up duringregisterFieldbecause the||operator would keep falsy values that existed as properties.Changes
field.blur = field.blur || (...)toif (typeof field.blur !== 'function') field.blur = ...changeandfocusTesting
Root Cause
The original fix in commit 33e551f used the
||operator, which doesn't replace falsy values that exist as properties. This meant if a mutator created a field withblur: null, the||check would pass (becausefield.blurexists) but not replace it with a function.The
typeofcheck ensures these are always functions, regardless of what value mutators may have set.Summary by CodeRabbit
Bug Fixes
Tests