Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdds an extendable form hook API ( Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Downstream Dev
participant Base as createFormHook()
participant Extend as extendForm()
participant Hook as Extended Hook
participant App as App (uses hook)
Dev->>Base: createFormHook({ fieldComponents, formComponents })
Base-->>Dev: baseHook (useAppForm, contexts, component maps)
Dev->>Extend: baseHook.extendForm({ fieldComponents?, formComponents? })
Note over Extend: Merge base + extension\nCompile-time uniqueness checks for overlapping keys
Extend-->>Dev: extendedHook (reused contexts, merged component maps)
App->>Hook: useExtendedHook() / render
Hook-->>App: AppField / form components (parent + extended components available)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
|
View your CI Pipeline Execution ↗ for commit f6733e4
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview2 package(s) bumped directly, 11 bumped as dependents. 🟨 Minor bumps
🟩 Patch bumps
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2106 +/- ##
==========================================
- 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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/react-form/tests/createFormHook.test-d.tsx (1)
993-1021: The chaining type test never provesThirdFieldsurvived the second extension.This case creates
ThirdField, but the assertions still only coverTestandExtendedField, then stop atexpectTypeOf(doublyExtendedForm.AppField).toBeFunction(). A regression where the secondextendFormdrops the new key would still pass.Suggested fix
- const { useAppForm: useDoublyExtended } = baseHook + const { + useAppForm: useDoublyExtended, + withForm: withDoublyExtendedForm, + } = baseHook .extendForm({ fieldComponents: { ExtendedField } }) .extendForm({ fieldComponents: { ThirdField } }) - withExtendedForm({ + withDoublyExtendedForm({ defaultValues: { name: '' }, render: ({ form }) => { return ( <form.AppField name="name"> {(field) => { expectTypeOf(field.Test).toBeFunction() expectTypeOf(field.ExtendedField).toBeFunction() + expectTypeOf(field.ThirdField).toBeFunction() return null }} </form.AppField> ) }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-form/tests/createFormHook.test-d.tsx` around lines 993 - 1021, The test never asserts that ThirdField survived the second extendForm call; update the test around useDoublyExtended/withExtendedForm to assert ThirdField is present — add expectTypeOf(field.ThirdField).toBeFunction() inside the render callback (alongside the existing Test and ExtendedField checks) and also assert the doublyExtendedForm exposes the extended field (e.g., an assertion that the useDoublyExtended result exposes ThirdField on AppField or the equivalent exported shape) so the chaining regression would fail if the second extension drops the key.examples/react/composition/index.html (1)
9-9: Title doesn't match the example name.The title says "Simple Example App" but this is the composition example. Consider updating to reflect the actual example purpose.
- <title>TanStack Form React Simple Example App</title> + <title>TanStack Form React Composition Example App</title>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react/composition/index.html` at line 9, Update the HTML <title> element to reflect this is the "Composition" example rather than "Simple Example App": change the title text content inside the <title> tag (currently "TanStack Form React Simple Example App") to something like "TanStack Form React Composition Example" so it accurately describes this example page.examples/react/composition/src/index.tsx (2)
43-48: Async validator returnsfalseinstead ofundefinedwhen valid.When
value.includes('error')is false, the&&short-circuit returnsfalse. While TanStack Form may handle this, explicitly returningundefinedfor valid state is clearer and more conventional.onChangeAsync: async ({ value }) => { await new Promise((resolve) => setTimeout(resolve, 1000)) - return ( - value.includes('error') && 'No "error" allowed in first name' - ) + return value.includes('error') + ? 'No "error" allowed in first name' + : undefined },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react/composition/src/index.tsx` around lines 43 - 48, The async validator onChangeAsync currently returns the string error when value.includes('error') is true but returns false when valid due to the && expression; update the onChangeAsync implementation (the async validator function) to explicitly return undefined for valid input instead of false—e.g., replace the short-circuit expression with a conditional (ternary or if) that returns 'No "error" allowed in first name' when value.includes('error') is true and undefined otherwise.
54-56: Consider using a distinct label for the lastName field.Both fields currently display "last name". The second one is correct, but you may want to capitalize it consistently (e.g., "Last Name").
- {(f) => <f.TextField label="last name" />} + {(f) => <f.TextField label="Last Name" />}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react/composition/src/index.tsx` around lines 54 - 56, The lastName field's visible label is not distinct/capitalized; update the form.AppField for name="lastName" to use a proper, distinct label (e.g., change the f.TextField label prop to "Last Name") and verify the other field (form.AppField name="firstName") uses "First Name" so both fields are correctly labeled and consistently capitalized; look for usages of form.AppField and the f.TextField label prop to make this change.examples/react/composition/src/AppForm/AppForm.tsx (1)
16-21: Consider exportingextendFormfor downstream composition.Given this PR introduces form composition capabilities via
extendForm, you might want to export it alongsideuseAppFormso downstream consumers can extend this form with additional components.-const { useAppForm } = createFormHook({ +const { useAppForm, extendForm } = createFormHook({ fieldContext, formContext, fieldComponents: { TextField: TextField }, formComponents: { SubmitButton }, }) + +export { extendForm }Also, minor:
TextField: TextFieldcan use object shorthand:TextField.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react/composition/src/AppForm/AppForm.tsx` around lines 16 - 21, The export currently only exposes useAppForm from createFormHook; update the destructuring returned by createFormHook to also export extendForm so downstream modules can compose/extend the form (referencing createFormHook, useAppForm, extendForm), and simplify the fieldComponents object by using shorthand for TextField (replace TextField: TextField with TextField) while keeping SubmitButton in formComponents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/framework/react/guides/form-composition.md`:
- Around line 209-220: The example is incorrect: the package import casing was
changed and the example claims a name collision with CustomSubmit even though
ProfileForm defines SubmitButton; revert the import to the original casing
('weyland-yutan-corp/forms') and demonstrate a real collision by using the same
symbol name that ProfileForm actually exposes (e.g., use SubmitButton instead of
CustomSubmit) so TypeScript will error; update the export line that calls
ProfileForm.extendForm (reference: ProfileForm, useAppForm, CustomTextField,
CustomSubmit, SubmitButton) to import from the correctly-cased package and to
pass a formComponents object that actually collides with the parent
(SubmitButton) rather than CustomSubmit.
In `@examples/react/composition/src/AppForm/FieldComponents/TextField.tsx`:
- Around line 9-12: The TextField input is missing an onBlur handler so
field.state.meta.isTouched and onBlur validators won't update; update the input
rendered in the TextField component to add an onBlur that calls the form field's
blur handler (e.g., invoke field.handleBlur or the appropriate blur method on
the field object) so the field's touched state and onBlur validation run
correctly.
In `@examples/react/composition/src/index.tsx`:
- Around line 50-52: The firstName field in the form is using the wrong label;
update the AppField rendering for the firstName field (the instance that calls
form.AppField and renders {(f) => <f.TextField label="last name" />} ) to use
the correct label "first name" (i.e., change the label prop on f.TextField to
"first name" for the firstName AppField).
In `@packages/react-form/src/createFormHook.tsx`:
- Around line 599-617: extendForm currently only prevents extending keys that
exist in the parent maps but doesn't block names that collide with core runtime
APIs (e.g., AppForm, AppField, Field, Subscribe, handleChange) and thus can
silently override core behavior; modify extendForm to declare a reservedNames
set (include core exported API and runtime member names) and validate
extension.fieldComponents and extension.formComponents against that set,
rejecting/throwing with a clear error if any key from TNewField or TNewForm
appears in reservedNames; reference the extendForm function and the
fieldComponents/formComponents merge locations so the checks run before calling
createFormHook and before the cast to TComponents & TNewField / TFormComponents
& TNewForm.
---
Nitpick comments:
In `@examples/react/composition/index.html`:
- Line 9: Update the HTML <title> element to reflect this is the "Composition"
example rather than "Simple Example App": change the title text content inside
the <title> tag (currently "TanStack Form React Simple Example App") to
something like "TanStack Form React Composition Example" so it accurately
describes this example page.
In `@examples/react/composition/src/AppForm/AppForm.tsx`:
- Around line 16-21: The export currently only exposes useAppForm from
createFormHook; update the destructuring returned by createFormHook to also
export extendForm so downstream modules can compose/extend the form (referencing
createFormHook, useAppForm, extendForm), and simplify the fieldComponents object
by using shorthand for TextField (replace TextField: TextField with TextField)
while keeping SubmitButton in formComponents.
In `@examples/react/composition/src/index.tsx`:
- Around line 43-48: The async validator onChangeAsync currently returns the
string error when value.includes('error') is true but returns false when valid
due to the && expression; update the onChangeAsync implementation (the async
validator function) to explicitly return undefined for valid input instead of
false—e.g., replace the short-circuit expression with a conditional (ternary or
if) that returns 'No "error" allowed in first name' when value.includes('error')
is true and undefined otherwise.
- Around line 54-56: The lastName field's visible label is not
distinct/capitalized; update the form.AppField for name="lastName" to use a
proper, distinct label (e.g., change the f.TextField label prop to "Last Name")
and verify the other field (form.AppField name="firstName") uses "First Name" so
both fields are correctly labeled and consistently capitalized; look for usages
of form.AppField and the f.TextField label prop to make this change.
In `@packages/react-form/tests/createFormHook.test-d.tsx`:
- Around line 993-1021: The test never asserts that ThirdField survived the
second extendForm call; update the test around
useDoublyExtended/withExtendedForm to assert ThirdField is present — add
expectTypeOf(field.ThirdField).toBeFunction() inside the render callback
(alongside the existing Test and ExtendedField checks) and also assert the
doublyExtendedForm exposes the extended field (e.g., an assertion that the
useDoublyExtended result exposes ThirdField on AppField or the equivalent
exported shape) so the chaining regression would fail if the second extension
drops the key.
🪄 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: 89488856-d987-469f-8a56-da5c672e2719
⛔ Files ignored due to path filters (2)
examples/react/composition/public/emblem-light.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
docs/framework/react/guides/form-composition.mdexamples/react/composition/.eslintrc.cjsexamples/react/composition/.gitignoreexamples/react/composition/README.mdexamples/react/composition/index.htmlexamples/react/composition/package.jsonexamples/react/composition/src/AppForm/AppForm.tsxexamples/react/composition/src/AppForm/FieldComponents/TextField.tsxexamples/react/composition/src/AppForm/FormComponents/SubmitButton.tsxexamples/react/composition/src/index.tsxexamples/react/composition/tsconfig.jsonpackages/react-form/src/createFormHook.tsxpackages/react-form/tests/createFormHook.test-d.tsxpackages/react-form/tests/createFormHook.test.tsx
3 tasks
harry-whorlow
force-pushed
the
extend-appform
branch
2 times, most recently
from
fb94ae3 to
8b89094
Compare
Contributor
LeCarbonator
left a comment
LeCarbonator
left a comment
There was a problem hiding this comment.
Essentially done! Once the threads are resolved / answered, this'll be ready to merge.
harry-whorlow
force-pushed
the
extend-appform
branch
from
e628103 to
8b89094
Compare
harry-whorlow
force-pushed
the
extend-appform
branch
from
44898c5 to
02b63cc
Compare
harry-whorlow
force-pushed
the
extend-appform
branch
from
3bc5c2d to
11f51c2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react-form/src/createFormHook.tsx (1)
599-617:⚠️ Potential issue | 🟠 MajorPrevent
extendFormfrom shadowing built-in form members.
extendFormcurrently blocks only parent-map key collisions, but Line 614–617 can still inject names likeAppField/AppFormthat overwrite core members when merged at Line 388–392 (...formComponentsis applied last). This can silently change runtime behavior.Suggested fix
+ type ReservedFieldComponentNames = keyof AnyFieldApi + type ReservedFormComponentNames = keyof AnyFormApi | 'AppField' | 'AppForm' + function extendForm< const TNewField extends Record<string, ComponentType<any>> & { - [K in keyof TComponents]?: 'Error: field component names must be unique — this key already exists in the base form' + [K in keyof TComponents | ReservedFieldComponentNames]?: 'Error: field component names must be unique and must not shadow field API members' }, const TNewForm extends Record<string, ComponentType<any>> & { - [K in keyof TFormComponents]?: 'Error: form component names must be unique — this key already exists in the base form' + [K in keyof TFormComponents | ReservedFormComponentNames]?: 'Error: form component names must be unique and must not shadow form API members' }, >(extension: { fieldComponents?: TNewField; formComponents?: TNewForm }) { + // Runtime guard for JS/any-casts + const reservedFormNames = new Set(['AppField', 'AppForm']) + for (const key of Object.keys(extension.formComponents ?? {})) { + if (reservedFormNames.has(key)) { + throw new Error(`extendForm: "${key}" is a reserved form component name`) + } + } + return createFormHook({ fieldContext, formContext,#!/bin/bash set -euo pipefail file="$(fd -a createFormHook.tsx | head -n1)" echo "== Merge order on extended form ==" nl -ba "$file" | sed -n '387,393p' echo echo "== extendForm generic constraints ==" nl -ba "$file" | sed -n '599,617p'Expected verification result:
...formComponentsappears afterAppField/AppForm.extendFormconstraints only reference base component maps and do not reserve core names.🤖 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 599 - 617, extendForm currently allows extensions to define keys that collide with core built-in members (e.g., AppField/AppForm) and thus can shadow them when merged into formComponents/fieldComponents; update the extendForm generic constraints (TNewField and TNewForm) to explicitly exclude the core built-in names (e.g., AppField, AppForm) so extension.fieldComponents/extension.formComponents cannot declare those keys, and keep the createFormHook call using the same fieldComponents/formComponents identifiers; reference extendForm, TNewField, TNewForm, fieldComponents, formComponents, AppField, AppForm, and createFormHook when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react-form/src/createFormHook.tsx`:
- Around line 599-617: extendForm currently allows extensions to define keys
that collide with core built-in members (e.g., AppField/AppForm) and thus can
shadow them when merged into formComponents/fieldComponents; update the
extendForm generic constraints (TNewField and TNewForm) to explicitly exclude
the core built-in names (e.g., AppField, AppForm) so
extension.fieldComponents/extension.formComponents cannot declare those keys,
and keep the createFormHook call using the same fieldComponents/formComponents
identifiers; reference extendForm, TNewField, TNewForm, fieldComponents,
formComponents, AppField, AppForm, and createFormHook when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ec3e59b-2e28-48f5-8ac4-6a737eeb78a0
📒 Files selected for processing (4)
docs/framework/react/guides/form-composition.mdexamples/react/composition/src/AppForm/FieldComponents/TextField.tsxexamples/react/composition/src/index.tsxpackages/react-form/src/createFormHook.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/react/composition/src/AppForm/FieldComponents/TextField.tsx
- docs/framework/react/guides/form-composition.md
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.
Summary by CodeRabbit
New Features
Examples
Documentation
Tests