fix: ensure fieldName is passed to custom validation logic functions#2127
Open
mwg-ofx wants to merge 2 commits intoTanStack:mainfrom
Open
fix: ensure fieldName is passed to custom validation logic functions#2127mwg-ofx wants to merge 2 commits intoTanStack:mainfrom
mwg-ofx wants to merge 2 commits intoTanStack:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThreads Changes
Sequence Diagram(s)sequenceDiagram
participant FieldApi as FieldApi
participant Utils as getSync/AsyncValidatorArray
participant ValidationLogic as validationLogic
participant Validator as ValidatorFns
FieldApi->>Utils: build validator array (include fieldName)
Utils->>ValidationLogic: call validationLogic(event { type, async, fieldName })
ValidationLogic->>Validator: decide/append validators (may use fieldName)
Validator-->>ValidationLogic: return validators
ValidationLogic-->>Utils: provide final validator list
Utils-->>FieldApi: validators applied / validation results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
packages/form-core/tests/DynamicValidation.spec.ts (1)
238-333: Consider adding one async linked-field case to prevent fieldName regressions.A small async linked validation scenario asserting
props.event.fieldNamefor the linked field would lock in the async path as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/form-core/tests/DynamicValidation.spec.ts` around lines 238 - 333, Add an async linked-field scenario to the existing test to cover the async path that depends on props.event.fieldName: update the validationLogic used in the spec to include a case where a validator is async and references a linked field (so props.event.fieldName is set for that linked field) and then exercise that path by triggering the linked-field event (e.g., blur/change/submit on the linked field) and awaiting the validation; specifically modify the test that defines validationLogic, FieldApi instances and their validators ('onDynamic') to add a linked async validator and assertions that await and assert errorMap for the linked field to ensure the props.event.fieldName branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/form-core/src/FieldApi.ts`:
- Around line 1828-1832: Linked-field async validators are receiving the
triggering field's name (this.name) instead of the linked field's name; update
the linked-field validation loop where getAsyncValidatorArray is called (using
field.options, field.form, validationLogic) to set fieldName to the linked
field's identifier (e.g., linkedField.name or the loop variable representing the
linked field) rather than this.name so event.fieldName in the custom async
validationLogic reflects the linked field.
---
Nitpick comments:
In `@packages/form-core/tests/DynamicValidation.spec.ts`:
- Around line 238-333: Add an async linked-field scenario to the existing test
to cover the async path that depends on props.event.fieldName: update the
validationLogic used in the spec to include a case where a validator is async
and references a linked field (so props.event.fieldName is set for that linked
field) and then exercise that path by triggering the linked-field event (e.g.,
blur/change/submit on the linked field) and awaiting the validation;
specifically modify the test that defines validationLogic, FieldApi instances
and their validators ('onDynamic') to add a linked async validator and
assertions that await and assert errorMap for the linked field to ensure the
props.event.fieldName branch is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18dbf798-0f75-4e1b-8636-58dd478bdd92
📒 Files selected for processing (5)
.changeset/whole-views-wear.mdpackages/form-core/src/FieldApi.tspackages/form-core/src/ValidationLogic.tspackages/form-core/src/utils.tspackages/form-core/tests/DynamicValidation.spec.ts
This was always part of the types for custom validation functions, it just wasn't wired up. This update is pretty straightforward - it just wires up the field, and adds a test to prove that custom field-level dynamic validation is now possible. The example in the test mostly follows a basic version of what's outlined in TanStack#1861. Fixes TanStack#1861
mwg-ofx
force-pushed
the
dynamic-validation-fieldname
branch
from
ad2cd71 to
72c0b37
Compare
Member
|
Whoops! My bad for having this bug in the first place! Good catch. The code generally LGTM at a quick glance, but I want to have another maintainer take a closer look at the tests in particular since I'm deep in Form Group land and just got an email so I thought I'd take a look. I'll have it prioritized given the quality of the PR. Thanks! |
|
View your CI Pipeline Execution ↗ for commit 58473fe
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2127 +/- ##
==========================================
- Coverage 90.35% 90.25% -0.10%
==========================================
Files 38 49 +11
Lines 1752 2043 +291
Branches 444 532 +88
==========================================
+ Hits 1583 1844 +261
- Misses 149 179 +30
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
🎯 Changes
This was always part of the types for custom validation functions (see #1622), it just wasn't wired up. This update is pretty straightforward - it just wires up the field, and adds a test to prove that custom field-level dynamic validation is now possible. The example in the test mostly follows a basic version of what's outlined in #1861.
Fixes #1861
✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Tests