feat(form-core): Add options to reset/replace/delete all fields [FieldGroupApi]#1962
feat(form-core): Add options to reset/replace/delete all fields [FieldGroupApi]#1962kusiewicz wants to merge 1 commit intoTanStack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: bf03c6c The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit 0780249
☁️ 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 #1962 +/- ##
==========================================
+ Coverage 90.35% 90.42% +0.06%
==========================================
Files 38 49 +11
Lines 1752 2078 +326
Branches 444 539 +95
==========================================
+ Hits 1583 1879 +296
- Misses 149 179 +30
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kusiewicz
changed the title
kusiewicz
force-pushed
the
fieldGroups
branch
2 times, most recently
from
1ce55b0 to
bf03c6c
Compare
|
Not sure what codecov is on about, so I'll ignore that bit for the review. Thanks for tackling this! |
|
Just a heads up, there's some reworks of how state works in TSF, so it would conflict with this PR. The changes shouldn't be drastic, but it'll likely cause a merge conflict once done. |
Sure, please ping me here when the change you are referring to is implemented, I will make the corrections. |
|
The new store is implemented! Names / types may be different. See #2035 for details. |
📝 WalkthroughWalkthroughAdds three bulk operations to FieldGroupApi— ChangesFieldGroupApi bulk operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Rebased :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/form-core/tests/FieldGroupApi.spec.ts (1)
515-729: Consider adding a test for path-prefix collisions.The string-field-group variant of
deleteAllFields/resetAllFieldsusesstartsWith(fieldStr), which also matches sibling fields whose registered name starts with the same substring (e.g., groupfields: 'people'would also matchpeopleCount.name). A targeted test would pin this behavior and surface the bug flagged inFieldGroupApi.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/form-core/tests/FieldGroupApi.spec.ts` around lines 515 - 729, The tests lack a case for path-prefix collisions where FieldGroupApi methods (deleteAllFields, resetAllFields) use a startsWith check and incorrectly match sibling field names; add a unit test that mounts a form with fields like 'people' and 'peopleCount.name' (or similar), create a FieldGroupApi with fields: 'people', call deleteAllFields/resetAllFields and assert that only the exact 'people' path is affected while 'peopleCount.name' remains unchanged; update expectations to catch the incorrect startsWith behavior so the failing test highlights the need to change the matching logic in FieldGroupApi (reference: FieldGroupApi.deleteAllFields, FieldGroupApi.resetAllFields and the startsWith check).packages/form-core/src/FieldGroupApi.ts (1)
398-419: Duplicate prefix-filter logic acrossdeleteAllFieldsandresetAllFields.The
Object.keys(this.form.fieldInfo).filter(...)block is copy-pasted. Extract a private helper (e.g.,#getSubFieldNames()) that returns the list of sub-field names for the string-group case, so the boundary fix only needs to land in one place.Also applies to: 558-577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/form-core/src/FieldGroupApi.ts` around lines 398 - 419, Extract the duplicated prefix-filter logic into a single private helper (e.g., `#getSubFieldNames`) that takes the string form of this.fieldsMap and returns Object.keys(this.form.fieldInfo).filter(f => f !== fieldStr && f.startsWith(fieldStr)); then replace the inline filter in deleteAllFields and the matching block in resetAllFields with calls to `#getSubFieldNames`(), and use its return value for deletion/reset; ensure the helper is used when typeof this.fieldsMap === 'string' and keeps the exact-exclusion behavior (exclude the exact fieldStr).
🤖 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/FieldGroupApi.ts`:
- Around line 486-495: The current replaceAllFields implementation iterates keys
and calls setFieldValue per key, which leaves any existing keys not present in
the incoming fields object untouched; clarify or change behavior: update the
JSDoc for replaceAllFields to state whether it performs a partial overwrite
(only provided keys are updated) or a full replacement (missing keys are
cleared), and if full replace is intended then for the string-group path call
this.form.setFieldValue(this.fieldsMap, fields) once to replace the whole
subtree instead of per-key writes; keep the mapped-case behavior as-is (keys
outside fieldsMap are out of scope) and reference replaceAllFields,
setFieldValue, this.form.setFieldValue, and this.fieldsMap in the change.
- Around line 561-577: resetAllFields repeats the same string-prefix matching
bug as deleteAllFields: using f.startsWith(fieldStr) can match siblings (e.g.,
"people" matching "peopleCount") and it only iterates form.fieldInfo so values
without mounted FieldApi are skipped; fix by extracting the prefix-match into a
shared helper used by resetAllFields and deleteAllFields that checks the
boundary after the prefix (accept only when next char is '.' or '[' or exact
match), and change the string-branch to collect keys from both form.fieldInfo
and the form's values (or form.getValues()) so you reset all matching entries
via this.form.resetField(f) rather than relying solely on fieldInfo.
- Around line 401-419: The string-branch of deleteAllFields is overly broad and
only iterates registered keys; update deleteAllFields so when this.fieldsMap is
a string you (1) compute the group's path string once (fieldsMap.toString()) and
filter targets using a path-boundary check (match only names where the prefix is
followed by '.' or '[') instead of plain startsWith to avoid deleting siblings
like "peopleCount"; and (2) instead of relying solely on
Object.keys(this.form.fieldInfo), remove the entire subtree from the underlying
form state (e.g., call this.form.deleteField(this.fieldsMap) and then set the
group's values to {} or undefined per desired semantics) so unmounted/raw values
under the group's path are also cleared; use the existing symbols
deleteAllFields, fieldsMap, form.fieldInfo and form.deleteField to locate and
change the logic.
---
Nitpick comments:
In `@packages/form-core/src/FieldGroupApi.ts`:
- Around line 398-419: Extract the duplicated prefix-filter logic into a single
private helper (e.g., `#getSubFieldNames`) that takes the string form of
this.fieldsMap and returns Object.keys(this.form.fieldInfo).filter(f => f !==
fieldStr && f.startsWith(fieldStr)); then replace the inline filter in
deleteAllFields and the matching block in resetAllFields with calls to
`#getSubFieldNames`(), and use its return value for deletion/reset; ensure the
helper is used when typeof this.fieldsMap === 'string' and keeps the
exact-exclusion behavior (exclude the exact fieldStr).
In `@packages/form-core/tests/FieldGroupApi.spec.ts`:
- Around line 515-729: The tests lack a case for path-prefix collisions where
FieldGroupApi methods (deleteAllFields, resetAllFields) use a startsWith check
and incorrectly match sibling field names; add a unit test that mounts a form
with fields like 'people' and 'peopleCount.name' (or similar), create a
FieldGroupApi with fields: 'people', call deleteAllFields/resetAllFields and
assert that only the exact 'people' path is affected while 'peopleCount.name'
remains unchanged; update expectations to catch the incorrect startsWith
behavior so the failing test highlights the need to change the matching logic in
FieldGroupApi (reference: FieldGroupApi.deleteAllFields,
FieldGroupApi.resetAllFields and the startsWith check).
🪄 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: 20f30030-8cc7-4638-bac9-a3e0f01abe3f
📒 Files selected for processing (3)
.changeset/upset-lemons-spend.mdpackages/form-core/src/FieldGroupApi.tspackages/form-core/tests/FieldGroupApi.spec.ts
| replaceAllFields = (fields: TFieldGroupData) => { | ||
| for (const fieldName of Object.keys( | ||
| fields as object, | ||
| ) as (keyof TFieldGroupData)[]) { | ||
| this.setFieldValue( | ||
| fieldName as unknown as DeepKeys<TFieldGroupData>, | ||
| fields[fieldName] as never, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
replaceAllFields does a shallow per-key write; surplus existing keys remain.
Because each provided key is written via setFieldValue, any keys that exist in the current form subtree but aren't present on the incoming fields object are not cleared. The name "replaceAllFields" implies a full replacement. For the string-group case, a single this.form.setFieldValue(this.fieldsMap, fields) would give true replace semantics; for the mapped case, keys outside fieldsMap are inherently out of scope, which is fine.
At minimum, please clarify the intended semantics in the JSDoc (partial overwrite vs. full replace) so callers don't get surprised. If full replace is intended, switch the string-group path to a single setFieldValue on the group root.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/form-core/src/FieldGroupApi.ts` around lines 486 - 495, The current
replaceAllFields implementation iterates keys and calls setFieldValue per key,
which leaves any existing keys not present in the incoming fields object
untouched; clarify or change behavior: update the JSDoc for replaceAllFields to
state whether it performs a partial overwrite (only provided keys are updated)
or a full replacement (missing keys are cleared), and if full replace is
intended then for the string-group path call
this.form.setFieldValue(this.fieldsMap, fields) once to replace the whole
subtree instead of per-key writes; keep the mapped-case behavior as-is (keys
outside fieldsMap are out of scope) and reference replaceAllFields,
setFieldValue, this.form.setFieldValue, and this.fieldsMap in the change.
There was a problem hiding this comment.
This one sounds like a miss. At least I can't make much sense of it. Focus on the other errors for now.
There was a problem hiding this comment.
@LeCarbonator, understood — I'll set this aside and focus on the other issues in the PR. Thanks for the clarification!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/FieldGroupApi.ts`:
- Around line 586-593: In resetAllFields, avoid unconditionally writing
undefined to the group by checking this.form.options.defaultValues before
calling setFieldValue: if defaultValues is undefined, do not call
this.form.setFieldValue(groupPath, ...), otherwise compute resetValue =
getBy(this.form.options.defaultValues, groupPath) and call
this.form.setFieldValue(groupPath, resetValue, { dontUpdateMeta: true }); ensure
this matches FormApi.resetField semantics (leave prev.values unchanged when no
defaultValues) and, if you must pass an empty/typed value similar to
deleteAllFields, use the same emptyValue as never cast pattern used in
deleteAllFields to satisfy the type system.
🪄 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: c73ee959-ac20-4b01-8087-9aba30e72e09
📒 Files selected for processing (3)
.changeset/upset-lemons-spend.mdpackages/form-core/src/FieldGroupApi.tspackages/form-core/tests/FieldGroupApi.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/upset-lemons-spend.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/form-core/tests/FieldGroupApi.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/form-core/tests/FieldGroupApi.spec.ts (2)
702-902: ⚡ Quick winAdd a mapped no-child
resetAllFieldsregression.The string-backed
resetAllFieldscases already cover the “no mounted children” path, but the mapped branch still only runs with mounted fields. Since those code paths differ, one unmounted mapped case would make this much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/form-core/tests/FieldGroupApi.spec.ts` around lines 702 - 902, Add a new test that covers the mapped FieldGroupApi path when no child FieldApi instances are mounted: create a FormApi with mapped defaultValues (e.g., { a: 'A', b: 'B' }), mount the form, mutate the form values (form.setFieldValue('a', 'X'), form.setFieldValue('b', 'Y')), create and mount a FieldGroupApi with fields mapping (e.g., { firstName: 'a', lastName: 'b' }) and its own defaultValues, then call group.resetAllFields() and assert form.state.values resets to the form's defaultValues and that metas for 'a' and 'b' reflect untouched/clean; reference FieldGroupApi, resetAllFields, form.setFieldValue and FormApi to locate the behavior to test.
515-639: ⚡ Quick winAdd an array-backed
deleteAllFieldsregression.This segment only exercises object-shaped string groups. The implementation also branches on
Array.isArray(currentValue), so a small array-backed case would cover the[]path and make that behavior harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/form-core/tests/FieldGroupApi.spec.ts` around lines 515 - 639, Add a test to FieldGroupApi.spec.ts covering the Array.isArray branch for deleteAllFields: create a form with defaultValues where the target group path is an array (e.g., nested: { field: [{ id: 1 }] }), mount a FormApi and a FieldGroupApi for fields: 'nested.field' (with appropriate defaultValues for an array), call group.deleteAllFields() and assert that form.state.values.nested.field becomes an empty array ([]) — reference the FieldGroupApi.deleteAllFields path and reuse FormApi/FieldApi setup patterns from the existing spec to locate where to add the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/form-core/tests/FieldGroupApi.spec.ts`:
- Around line 702-902: Add a new test that covers the mapped FieldGroupApi path
when no child FieldApi instances are mounted: create a FormApi with mapped
defaultValues (e.g., { a: 'A', b: 'B' }), mount the form, mutate the form values
(form.setFieldValue('a', 'X'), form.setFieldValue('b', 'Y')), create and mount a
FieldGroupApi with fields mapping (e.g., { firstName: 'a', lastName: 'b' }) and
its own defaultValues, then call group.resetAllFields() and assert
form.state.values resets to the form's defaultValues and that metas for 'a' and
'b' reflect untouched/clean; reference FieldGroupApi, resetAllFields,
form.setFieldValue and FormApi to locate the behavior to test.
- Around line 515-639: Add a test to FieldGroupApi.spec.ts covering the
Array.isArray branch for deleteAllFields: create a form with defaultValues where
the target group path is an array (e.g., nested: { field: [{ id: 1 }] }), mount
a FormApi and a FieldGroupApi for fields: 'nested.field' (with appropriate
defaultValues for an array), call group.deleteAllFields() and assert that
form.state.values.nested.field becomes an empty array ([]) — reference the
FieldGroupApi.deleteAllFields path and reuse FormApi/FieldApi setup patterns
from the existing spec to locate where to add the new case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f7e8a61-b2b1-48a7-987b-294c66c355c5
📒 Files selected for processing (3)
.changeset/upset-lemons-spend.mdpackages/form-core/src/FieldGroupApi.tspackages/form-core/tests/FieldGroupApi.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/upset-lemons-spend.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/form-core/src/FieldGroupApi.ts
|
@LeCarbonator adjusted |
|
Sounds good! I'll be reviewing it soon, then. Hopefully, coderabbit wasn't being too nitpicky, seems like it had a lot to say on this PR. |
2 participants
🎯 Changes
Regarding: #1684
This PR adds functionalities to the Field Group API:
resetAllFields- reset all group fields to their default valuesdeleteAllFields- delete all fields from the current group instancereplaceAllFields- replace all group fields with new provided valuesWhen accepted, I will prepare documentation update.
✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Tests