Skip to content

fix: require compatible withFieldGroup field types#2255

Open
xianjianlf2 wants to merge 1 commit into
TanStack:mainfrom
xianjianlf2:fix/with-field-group-exact-fields-1855
Open

fix: require compatible withFieldGroup field types#2255
xianjianlf2 wants to merge 1 commit into
TanStack:mainfrom
xianjianlf2:fix/with-field-group-exact-fields-1855

Conversation

@xianjianlf2

@xianjianlf2 xianjianlf2 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Fixes #1855

Summary

  • add field-group-specific deep key helpers that allow parent objects to have extra fields while requiring exposed field values to be compatible
  • use the stricter helper for withFieldGroup component fields in React, Preact, and Solid
  • add React and Preact type regression coverage for reusable groups that can write null into string-only parent fields

Tests

  • 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.tsx
  • pnpm --filter @tanstack/react-form test:types
  • pnpm --filter @tanstack/preact-form test:types
  • pnpm --filter @tanstack/solid-form test:types

Summary by CodeRabbit

  • Enhancements

    • Improved TypeScript type checking for field-group field mappings across React, Preact, and Solid integrations.
    • Supports compatible nested object and array field types when defining field-group mappings.
    • Prevents mappings where the parent field cannot safely accept all values produced by the field group.
  • Tests

    • Added coverage verifying valid mappings are accepted and incompatible nullable/non-nullable mappings are rejected.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Field-group deep-key and mapping utilities now evaluate recursive value compatibility. React, Preact, and Solid withFieldGroup constraints use these utilities, with type tests covering compatible and incompatible nullable field mappings.

Changes

Field-group typing

Layer / File(s) Summary
Compatibility key and mapping utilities
packages/form-core/src/util-types.ts
Adds recursive field-group compatibility predicates, compatible deep-key selectors, and FieldGroupFieldsMap.
Adapter withFieldGroup constraints
packages/*-form/src/createFormHook.tsx
Updates Preact, React, and Solid field-group typing to use the new compatibility helpers.
Exact field mapping type tests
packages/*-form/tests/createFormHook.test-d.tsx
Accepts matching string | null mappings and rejects string-only parent mappings in direct and object forms.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: crutchcorn

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description summarizes the change, but it does not follow the repository template sections for Changes, Checklist, and Release Impact. Reformat the PR description to include the required Changes, Checklist, and Release Impact sections, and fill in the checklist items.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: stricter withFieldGroup field-type compatibility.
Linked Issues check ✅ Passed The code implements the stricter withFieldGroup field-map typing requested in #1855 and adds regression tests for the unsafe null case.
Out of Scope Changes check ✅ Passed The changes stay focused on field-group type compatibility and related tests across the affected form packages.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
packages/form-core/src/util-types.ts (1)

230-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Undocumented asymmetry: NonNullable<TDeepValue> applied in one helper but not the other.

DeepKeysOfFieldGroupType wraps the deep value in NonNullable before the compatibility check, while DeepKeysOfFieldGroupFieldType does not. This looks intentional (stripping the container's own optionality when matching a whole-object shape via fields="key", while preserving nullability for exact leaf-value matching via mapped fields, e.g. so string | undefined deep values can still match string | undefined field-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 NonNullable and 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 win

Change 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.tsx files — solid-form gets the identical type-level fix but no equivalent withFieldGroup nullable-field regression test. Worth adding for parity/future regression safety, though not blocking since the underlying IsFieldGroupCompatible logic 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5d11281 and 9bdf5e3.

📒 Files selected for processing (6)
  • packages/form-core/src/util-types.ts
  • packages/preact-form/src/createFormHook.tsx
  • packages/preact-form/tests/createFormHook.test-d.tsx
  • packages/react-form/src/createFormHook.tsx
  • packages/react-form/tests/createFormHook.test-d.tsx
  • packages/solid-form/src/createFormHook.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

withFieldGroup field map type is too wide

1 participant