fix(react-form): subscribe to full meta object#1981
Open
ws-rush wants to merge 22 commits intoTanStack:mainfrom
Open
fix(react-form): subscribe to full meta object#1981ws-rush wants to merge 22 commits intoTanStack:mainfrom
ws-rush wants to merge 22 commits intoTanStack:mainfrom
Conversation
… custom meta properties This commit modifies `useField` to subscribe to the entire `state.meta` object instead of granular properties. This ensures that custom meta properties (like `hidden`) added by users trigger re-renders when updated. To mitigate performance regressions (specifically array fields re-rendering on every child update due to `isDirty`/`isTouched` refresh), `FormApi.setFieldValue` is optimized to skip `setFieldMeta` if the meta state is already correct. This ensures `meta` object reference stability when relevant state hasn't changed. Added regression test `tests/issue-1980.test.tsx`. Updated `tests/useField.test.tsx` to accommodate the slight behavior change (initial re-render due to `isDefaultValue` change).
🦋 Changeset detectedLatest commit: c8a45f1 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 |
… custom meta properties This commit modifies `useField` to subscribe to the entire `state.meta` object instead of granular properties. This ensures that custom meta properties (like `hidden`) added by users trigger re-renders when updated. To mitigate performance regressions (specifically array fields re-rendering on every child update due to `isDirty`/`isTouched` refresh), `FormApi.setFieldValue` is optimized to skip `setFieldMeta` if the meta state is already correct. This ensures `meta` object reference stability when relevant state hasn't changed. Added regression test `tests/issue-1980.test.tsx`. Updated `tests/useField.test.tsx` to accommodate the slight behavior change (initial re-render due to `isDefaultValue` change).
…' of https://github.com/wusabyRush/custom-field-meta into issue-1980-fix-custom-meta-updates-18083788771914964547
…' of https://github.com/wusabyRush/custom-field-meta into issue-1980-fix-custom-meta-updates-18083788771914964547
ws-rush
force-pushed
the
issue-1980-fix-custom-meta-updates-18083788771914964547
branch
from
60c8c31 to
effb050
Compare
Merge Upstream Changes
merge upstream edits
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates meta subscription in Changes
Sequence Diagram(s)sequenceDiagram
participant ReactComponent as React Component
participant useField as useField Hook
participant FormCore as Form Core (FormApi / FieldApi)
participant DOM as DOM / Renderer
ReactComponent->>useField: mount Field component
useField->>FormCore: subscribe to `field.state.meta` (single selector)
useField->>FormCore: read other field value (e.g., color)
useField->>FormCore: call setFieldMeta / setFieldValue (if needed)
FormCore-->>useField: emit meta update notification
useField->>DOM: update reactive meta -> re-render Field (show/hide)
ReactComponent->>useField: user changes other field
FormCore-->>useField: notify meta/state change for dependent field
useField->>DOM: update DOM according to new meta
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
🤖 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/FormApi.ts`:
- Around line 2309-2327: getFieldMeta(field) can return undefined and the
current checks dereference meta (meta.isDirty / meta.errorMap) before falling
back, causing runtime errors; guard the lookup by assigning a safe fallback
(e.g., defaultFieldMeta) or check for meta existence before accessing
properties. Update the block around getFieldMeta(field) to use a local const
(e.g., const meta = this.getFieldMeta(field) ?? defaultFieldMeta) or change the
conditional to short-circuit when meta is falsy, and then call
setFieldMeta(field, prev => ...) as before; reference getFieldMeta,
meta.isDirty, meta.errorMap, and setFieldMeta in your change.
🪄 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: 847a0459-ef08-4954-a800-2325dd7e2cbd
📒 Files selected for processing (5)
.changeset/issue-1980-fix.mdpackages/form-core/src/FormApi.tspackages/react-form/src/useField.tsxpackages/react-form/tests/issue-1980.test.tsxpackages/react-form/tests/useField.test.tsx
Comment on lines
2309
to
+2327
| if (!dontUpdateMeta) { | ||
| this.setFieldMeta(field, (prev) => ({ | ||
| ...prev, | ||
| isTouched: true, | ||
| isDirty: true, | ||
| errorMap: { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| ...prev?.errorMap, | ||
| onMount: undefined, | ||
| }, | ||
| })) | ||
| const meta = this.getFieldMeta(field) | ||
|
|
||
| if ( | ||
| !meta?.isTouched || | ||
| !meta.isDirty || | ||
| meta.errorMap.onMount !== undefined | ||
| ) { | ||
| this.setFieldMeta(field, (prev) => ({ | ||
| ...prev, | ||
| isTouched: true, | ||
| isDirty: true, | ||
| errorMap: { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| ...prev?.errorMap, | ||
| onMount: undefined, | ||
| }, | ||
| })) | ||
| } |
There was a problem hiding this comment.
Guard the metadata lookup before dereferencing it.
getFieldMeta(field) can be undefined for fields that haven't been materialized yet, so meta.isDirty / meta.errorMap can throw before the fallback path runs. Default to defaultFieldMeta or add an explicit guard.
🔧 Suggested fix
- const meta = this.getFieldMeta(field)
+ const meta = this.getFieldMeta(field) ?? defaultFieldMeta🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/form-core/src/FormApi.ts` around lines 2309 - 2327,
getFieldMeta(field) can return undefined and the current checks dereference meta
(meta.isDirty / meta.errorMap) before falling back, causing runtime errors;
guard the lookup by assigning a safe fallback (e.g., defaultFieldMeta) or check
for meta existence before accessing properties. Update the block around
getFieldMeta(field) to use a local const (e.g., const meta =
this.getFieldMeta(field) ?? defaultFieldMeta) or change the conditional to
short-circuit when meta is falsy, and then call setFieldMeta(field, prev => ...)
as before; reference getFieldMeta, meta.isDirty, meta.errorMap, and setFieldMeta
in your change.
Author
|
support custom meta typing added as experiment, Remaining TODOs:
useForm({
defaultMeta: { disabled: false },
})
<form.Field
name="countryId"
defaultMeta={{ dataSource: [] as Country[] }}
> there is a lot of AI here so sure all thing need review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-form/src/useForm.tsx (1)
22-45: Consider centralizingWidenMetain one internal type utility.
WidenMetanow exists in bothpackages/react-form/src/useForm.tsxandpackages/react-form/src/useField.tsx; extracting it reduces future type drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-form/src/useForm.tsx` around lines 22 - 45, WidenMeta is duplicated across useForm.tsx and useField.tsx—extract it into a single internal type utility (e.g., create a new internal types module) and replace the local definitions in both files with an import of that shared WidenMeta; update usages in FormApiWithCustomMeta and getFieldMeta to reference the centralized WidenMeta, ensure the new module exports the type (or marks it internal) and run type-check to fix any import/visibility issues.
🤖 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/react-form/src/useForm.tsx`:
- Around line 22-45: WidenMeta is duplicated across useForm.tsx and
useField.tsx—extract it into a single internal type utility (e.g., create a new
internal types module) and replace the local definitions in both files with an
import of that shared WidenMeta; update usages in FormApiWithCustomMeta and
getFieldMeta to reference the centralized WidenMeta, ensure the new module
exports the type (or marks it internal) and run type-check to fix any
import/visibility issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b31e1a6-9cdc-46ee-b2d0-0b6c10b22c66
📒 Files selected for processing (7)
.changeset/custom-field-meta.mdpackages/form-core/src/FieldApi.tspackages/form-core/src/FormApi.tspackages/react-form/src/createFormHook.tsxpackages/react-form/src/useField.tsxpackages/react-form/src/useForm.tsxpackages/react-form/tests/useField.test-d.tsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/custom-field-meta.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-form/src/useForm.tsx (1)
418-420: Pragmatic type cast, consider documenting the reason.The
as anycast bypasses complex generic type checking forFieldComponent's 24+ type parameters. While this suppresses type errors at the spread site, call-site type checking is preserved sinceAPIField's signature inherits from the typedFieldComponent.Consider adding a brief comment explaining why the cast is necessary to help future maintainers.
📝 Suggested documentation
extendedApi.Field = function APIField(props) { + // Cast needed: FieldComponent's complex generic constraints can't be fully + // expressed here; call-site type checking remains intact via the typed signature return <Field {...(props as any)} form={formApi} /> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-form/src/useForm.tsx` around lines 418 - 420, Add a brief inline comment above the cast in extendedApi.Field explaining why the pragmatic `as any` is required: note that FieldComponent has a very large generic signature (24+ type parameters) which causes excessive TypeScript complexity at the spread site, and that the cast only suppresses the spread-site generic inference while preserving call-site type checking via APIField's signature; reference the symbols extendedApi.Field, APIField, Field, and formApi in the comment so future maintainers understand the trade-off and where to revisit if type simplification or helper overloads are added.
🤖 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/react-form/src/useForm.tsx`:
- Around line 418-420: Add a brief inline comment above the cast in
extendedApi.Field explaining why the pragmatic `as any` is required: note that
FieldComponent has a very large generic signature (24+ type parameters) which
causes excessive TypeScript complexity at the spread site, and that the cast
only suppresses the spread-site generic inference while preserving call-site
type checking via APIField's signature; reference the symbols extendedApi.Field,
APIField, Field, and formApi in the comment so future maintainers understand the
trade-off and where to revisit if type simplification or helper overloads are
added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c34af65e-842b-40ef-b773-98a87a48b35c
📒 Files selected for processing (2)
packages/react-form/src/useForm.tsxpackages/react-form/tests/useField.test-d.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-form/tests/useField.test-d.tsx
Thread form-level custom meta through useAppForm, AppFieldExtendedReactFormApi,
and FieldComponent so metadata inferred from formOptions({ defaultMeta }) is
available in field render callbacks.
Add regression coverage for useAppForm with formOptions and array-valued
defaultMeta.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react-form/src/createFormHook.tsx (2)
320-371: AlignuseAppFormoverloads withuseFormto remove the double assertion.At Line 357,
as unknown as ReactFormExtendedApi<...>bypasses compile-time validation. The mismatch arises becauseuseAppFormmarksdefaultMetaas optional (defaultMeta?: TFormMeta), whileuseForm's first overload requires it to be non-optional (defaultMeta: TFormMeta). The double assertion hides this contract drift.Proposed refactor
Use function overloads to distinguish between when
defaultMetais explicitly provided vs. omitted, mirroringuseForm's pattern:- function useAppForm<..., const TFormMeta extends object = {}>( - props: Omit<FormOptions<..., TFormMeta>, 'defaultMeta'> & { - defaultMeta?: TFormMeta - }, - ): AppFieldExtendedReactFormApi<..., TFormMeta> { - const form = useForm(props) as unknown as ReactFormExtendedApi<..., TFormMeta> + // overload 1: defaultMeta provided => infer/preserve TFormMeta + function useAppForm<..., const TFormMeta extends object>( + props: Omit<FormOptions<..., TFormMeta>, 'defaultMeta'> & { + defaultMeta: TFormMeta + }, + ): AppFieldExtendedReactFormApi<..., TFormMeta> + + // overload 2: defaultMeta omitted => fallback meta type + function useAppForm<...>( + props: FormOptions<..., TSubmitMeta>, + ): AppFieldExtendedReactFormApi<..., {}> + + function useAppForm(props: any): any { + const form = useForm(props)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-form/src/createFormHook.tsx` around lines 320 - 371, The double-cast on the result of useForm in useAppForm hides a type-contract mismatch caused by marking defaultMeta optional in useAppForm's props while useForm's overload requires defaultMeta to be present; adjust useAppForm to provide matching function overloads (mirror useForm's overloads) that distinguish the case where defaultMeta is supplied vs omitted so the inferred return type is ReactFormExtendedApi<...> without using "as unknown as ReactFormExtendedApi"; update the signatures for useAppForm (and its props generic) so callers that omit defaultMeta get the appropriate overload and the implementation can call useForm without unsafe assertions.
389-389: Replaceas neverat the children boundary with an explicit merged type.At line 389,
as neversuppresses type checks exactly wherefieldandfieldComponentsare merged and passed to user code. This weakens safety and makes regressions harder to catch.Proposed refactor
- {children(Object.assign(field, fieldComponents) as never)} + {children( + Object.assign(field, fieldComponents) as typeof field & + NoInfer<TComponents> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-form/src/createFormHook.tsx` at line 389, The cast "as never" at the children boundary suppresses type checking for the merged props (children(Object.assign(field, fieldComponents) as never)); replace it with an explicit merged type so callers get correct typings: define a merged interface/type (e.g., MergedFieldProps = FieldProps & typeof fieldComponents or a generic Merge<FieldProps, FieldComponentTypes>) that represents the intersection of the runtime `field` shape and `fieldComponents`, and cast the Object.assign result to that type before passing to `children`; also update the `children` prop signature to accept this merged type (referencing the `children`, `field`, and `fieldComponents` symbols in createFormHook.tsx).
🤖 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/react-form/src/createFormHook.tsx`:
- Around line 320-371: The double-cast on the result of useForm in useAppForm
hides a type-contract mismatch caused by marking defaultMeta optional in
useAppForm's props while useForm's overload requires defaultMeta to be present;
adjust useAppForm to provide matching function overloads (mirror useForm's
overloads) that distinguish the case where defaultMeta is supplied vs omitted so
the inferred return type is ReactFormExtendedApi<...> without using "as unknown
as ReactFormExtendedApi"; update the signatures for useAppForm (and its props
generic) so callers that omit defaultMeta get the appropriate overload and the
implementation can call useForm without unsafe assertions.
- Line 389: The cast "as never" at the children boundary suppresses type
checking for the merged props (children(Object.assign(field, fieldComponents) as
never)); replace it with an explicit merged type so callers get correct typings:
define a merged interface/type (e.g., MergedFieldProps = FieldProps & typeof
fieldComponents or a generic Merge<FieldProps, FieldComponentTypes>) that
represents the intersection of the runtime `field` shape and `fieldComponents`,
and cast the Object.assign result to that type before passing to `children`;
also update the `children` prop signature to accept this merged type
(referencing the `children`, `field`, and `fieldComponents` symbols in
createFormHook.tsx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b579600-3809-4b8f-b352-432822630127
📒 Files selected for processing (2)
packages/react-form/src/createFormHook.tsxpackages/react-form/tests/createFormHook.test-d.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/react-form/tests/createFormHook.test-d.tsx
…pForm useAppForm was not forwarding TFormMeta to useForm, causing custom meta properties (e.g. dataSource) to resolve as unknown in form.AppField render props, getFieldMeta(), and state.fieldMeta. - Pass TFormMeta explicitly when calling useForm inside useAppForm - Intersect FormApiCustomMeta into AppFieldExtendedReactFormApi so getFieldMeta and state.fieldMeta carry the widened custom meta types - Cast return to AppFieldExtendedReactFormApi to satisfy the widened fieldMeta signature without breaking FormApi structural compatibility - Add type tests for form-level and field-level defaultMeta inference via both formOptions and direct props, using form.AppField
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/react-form/src/createFormHook.tsx`:
- Around line 185-201: The custom TFormMeta generic is being dropped by helper
types—update the helper signatures WithFormProps, withForm, and
useTypedAppFormContext to include a trailing generic parameter TFormMeta = {}
and thread that parameter into every instantiation of
AppFieldExtendedReactFormApi used in those helpers (i.e., add the trailing meta
type argument to AppFieldExtendedReactFormApi in the WithFormProps, withForm,
and useTypedAppFormContext definitions) so getFieldMeta/state.fieldMeta retain
the custom meta typing from createFormHook.
🪄 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: d1b7e613-6bb7-4f2f-baa6-ce0cc411a871
📒 Files selected for processing (2)
packages/react-form/src/createFormHook.tsxpackages/react-form/tests/createFormHook.test-d.tsx
…field validators - Thread TFormMeta through WithFormProps so forms with inferred defaultMeta can be passed to withForm components without type errors - Field validators now receive a fieldApi whose getMeta() includes custom meta inferred from the form's defaultMeta - Previously the validator's fieldApi was typed with the core FieldApi which has no awareness of custom meta, causing properties like dataSource to be typed as unknown
…field validators - Thread TFormMeta through WithFormProps so forms with inferred defaultMeta can be passed to withForm components without type errors - Field validators now receive a fieldApi whose getMeta() includes custom meta inferred from the form's defaultMeta - Previously the validator's fieldApi was typed with the core FieldApi which has no awareness of custom meta, causing properties like dataSource to be typed as unknown
…' of https://github.com/wusabyRush/custom-field-meta into issue-1980-fix-custom-meta-updates-18083788771914964547 # Conflicts: # packages/react-form/src/useField.tsx # packages/react-form/tests/createFormHook.test-d.tsx
… for same keys Use Omit<TFormMeta, keyof TFieldMeta> & TFieldMeta instead of simple intersection so that when both form and field define the same key (e.g. dataSource), the field-level type takes precedence. Also apply the merged meta type to field listeners, not just validators and render props.
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
1 participant
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.
Fixes issue #1980 where custom field meta properties were not triggering updates in
useField.Replaced granular meta subscriptions with a full
state.metasubscription inuseFieldhook.Optimized
FormApi.setFieldValuein@tanstack/form-coreto avoid unnecessary meta updates, preserving performance optimizations for array fields.Added regression test and updated existing tests.
PR created automatically by Jules for task 18083788771914964547 started by @ws-rush
Summary by CodeRabbit
New Features
Bug Fixes
Tests