fix: require compatible withFieldGroup field types#2255
Conversation
📝 WalkthroughWalkthroughField-group deep-key and mapping utilities now evaluate recursive value compatibility. React, Preact, and Solid ChangesField-group typing
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/form-core/src/util-types.ts (1)
230-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented asymmetry:
NonNullable<TDeepValue>applied in one helper but not the other.
DeepKeysOfFieldGroupTypewraps the deep value inNonNullablebefore the compatibility check, whileDeepKeysOfFieldGroupFieldTypedoes not. This looks intentional (stripping the container's own optionality when matching a whole-object shape viafields="key", while preserving nullability for exact leaf-value matching via mapped fields, e.g. sostring | undefineddeep values can still matchstring | undefinedfield-group leaves), but it's non-obvious and could easily be "fixed" incorrectly by a future refactor unifying the two helpers.A short comment explaining why one strips
NonNullableand the other doesn't would prevent regressions.📝 Suggested doc comment
/** * The keys of an object or array, deeply nested and compatible with a mapped field group value. + * Unlike `DeepKeysOfFieldGroupType`, this does not strip `NonNullable` from the deep value, + * since leaf-level field mappings must preserve exact nullability (e.g. `string | undefined` + * parent fields should still match `string | undefined` field-group fields). */ export type DeepKeysOfFieldGroupFieldType<TData, TValue> = unknown extends TData🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/form-core/src/util-types.ts` around lines 230 - 252, Add a concise explanatory comment near DeepKeysOfFieldGroupType and DeepKeysOfFieldGroupFieldType documenting their intentional asymmetry: the former applies NonNullable to match whole-object field-group containers, while the latter preserves deep-value nullability for exact mapped-field leaf matching. Do not change either type’s behavior.packages/solid-form/src/createFormHook.tsx (1)
494-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChange looks correct; consider parity test coverage for solid-form.
Same constraint fix as react-form/preact-form. The PR only adds regression tests to the React and Preact
test-d.tsxfiles — solid-form gets the identical type-level fix but no equivalentwithFieldGroupnullable-field regression test. Worth adding for parity/future regression safety, though not blocking since the underlyingIsFieldGroupCompatiblelogic is already covered by the other two adapters' tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solid-form/src/createFormHook.tsx` around lines 494 - 498, Add a Solid Form type-level regression test covering withFieldGroup when a field is nullable, matching the equivalent React and Preact test cases. Place it alongside the existing Solid Form test-d.tsx coverage and verify the corrected generic constraint accepts the intended nullable-field configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/form-core/src/util-types.ts`:
- Around line 230-252: Add a concise explanatory comment near
DeepKeysOfFieldGroupType and DeepKeysOfFieldGroupFieldType documenting their
intentional asymmetry: the former applies NonNullable to match whole-object
field-group containers, while the latter preserves deep-value nullability for
exact mapped-field leaf matching. Do not change either type’s behavior.
In `@packages/solid-form/src/createFormHook.tsx`:
- Around line 494-498: Add a Solid Form type-level regression test covering
withFieldGroup when a field is nullable, matching the equivalent React and
Preact test cases. Place it alongside the existing Solid Form test-d.tsx
coverage and verify the corrected generic constraint accepts the intended
nullable-field configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15dd92d1-c9ab-4ad8-afca-3dce67ee44bf
📒 Files selected for processing (6)
packages/form-core/src/util-types.tspackages/preact-form/src/createFormHook.tsxpackages/preact-form/tests/createFormHook.test-d.tsxpackages/react-form/src/createFormHook.tsxpackages/react-form/tests/createFormHook.test-d.tsxpackages/solid-form/src/createFormHook.tsx
Fixes #1855
Summary
withFieldGroupcomponentfieldsin React, Preact, and Solidnullinto string-only parent fieldsTests
pnpm prettier --check packages/form-core/src/util-types.ts packages/react-form/src/createFormHook.tsx packages/react-form/tests/createFormHook.test-d.tsx packages/preact-form/src/createFormHook.tsx packages/preact-form/tests/createFormHook.test-d.tsx packages/solid-form/src/createFormHook.tsxpnpm --filter @tanstack/react-form test:typespnpm --filter @tanstack/preact-form test:typespnpm --filter @tanstack/solid-form test:typesSummary by CodeRabbit
Enhancements
Tests