fix(react-form): address review issues from #2106 (extendForm / composition example)#2115
Open
devCluna wants to merge 7 commits intoTanStack:mainfrom
Open
fix(react-form): address review issues from #2106 (extendForm / composition example)#2115devCluna wants to merge 7 commits intoTanStack:mainfrom
devCluna wants to merge 7 commits intoTanStack:mainfrom
Conversation
…mposition example - docs: fix import casing (Weyland-Yutan-corp → weyland-yutan-corp) and use SubmitButton (a real parent component) in the collision example instead of the non-existent CustomSubmit - example: correct HTML title from "Simple" to "Composition Example App" - example: export extendForm from AppForm and use object shorthand for TextField - example: add missing onBlur handler to TextField so isTouched / onBlur validators update correctly - example: fix firstName label (was "last name") and capitalise both labels - example: fix async validator to return undefined instead of false when valid - createFormHook: block component names that collide with core runtime APIs (AppField, AppForm, Field, Subscribe, handleSubmit, ...) in extendForm - test: assert ThirdField survives second extendForm chain and use the doubly-extended withForm instead of the singly-extended one
📝 WalkthroughWalkthroughIntroduces an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (3)
packages/react-form/tests/createFormHook.test.tsx (1)
703-952: Add negative-path tests forextendFormruntime guards.Given the new runtime checks in
createFormHook.tsx, please also assert throw behavior for reserved keys (and duplicate base keys if added) so guardrails are regression-proof.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-form/tests/createFormHook.test.tsx` around lines 703 - 952, Add negative-path tests covering the new runtime guards in createFormHook by asserting that calling extendForm with reserved keys and with keys that duplicate base hook components throws: write tests that call createFormHook(...).extendForm({...}) passing a fieldComponents or formComponents object that includes a reserved key (e.g. keys used by the hook API like AppField/AppForm or other reserved identifiers your runtime check rejects) and assert it throws, and another test where extendForm is passed a key that already exists on the base hook (duplicate base key) and assert that also throws; use the same test patterns and helpers as the existing extendForm tests (referencing extendForm, createFormHook, useAppForm, withForm) so the guard behavior is covered and regression-proof.examples/react/composition/src/index.tsx (1)
66-68: Optional: avoid non-null assertion on root lookup.Line 66 can fail hard with a less-informative error if
#rootis missing; a small explicit guard improves diagnostics.Suggested tweak
-const rootElement = document.getElementById('root')! +const rootElement = document.getElementById('root') +if (!rootElement) { + throw new Error('Missing `#root` element') +}🤖 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 66 - 68, document.getElementById('root') is using a non-null assertion on rootElement which will throw a less-informative error if the element is missing; change the logic around the root lookup in index.tsx to explicitly guard the result of document.getElementById('root') (the rootElement variable) and either throw a clear Error (e.g., "Root element '#root' not found") or bail out gracefully before calling createRoot(...).render so createRoot and render are only invoked when rootElement is non-null.examples/react/composition/src/AppForm/FieldComponents/TextField.tsx (1)
16-19: Consider associating error text with the input for better a11y.Line 17 renders validation errors visually, but the input on Line 10 is not explicitly tied to that message (
aria-invalid/aria-describedby).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react/composition/src/AppForm/FieldComponents/TextField.tsx` around lines 16 - 19, The visual error message rendered from field.state.meta.errors should be programmatically associated with the input: when field.state.meta.isTouched && !field.state.meta.isValid render the error element with a stable id (e.g., `${field.input.name || field.input.id}-error`) and on the input element (in TextField component where the input is rendered) set aria-invalid={field.state.meta.isValid === false} and aria-describedby={field.state.meta.isValid ? undefined : `${field.input.name || field.input.id}-error`} so screen readers will announce the error; ensure the error element contains that id and remains hidden/omitted when there are no errors.
🤖 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 216-223: The primary example shows a TypeScript collision by
reusing SubmitButton; update the main snippet so ProfileForm.extendForm uses a
uniquely named component (e.g., formComponents: { TeamSubmitButton } or another
distinct identifier) and expose useAppForm from that result; then move the
collision example (reusing SubmitButton) into a separate "Gotcha" or "Type
collision" block with an explanatory note. Target the ProfileForm.extendForm
call, the exported useAppForm binding, and the component names CustomTextField
and SubmitButton when making the edits.
In `@examples/react/composition/.gitignore`:
- Around line 14-16: The .gitignore currently excludes pnpm-lock.yaml,
yarn.lock, and package-lock.json which prevents committing lockfiles; update the
.gitignore in examples/react/composition to stop ignoring the lockfile your
example uses (e.g., remove pnpm-lock.yaml if the project uses pnpm, or remove
yarn.lock/package-lock.json for their respective managers) so the appropriate
lockfile is committed for reproducible installs; ensure only irrelevant
lockfiles remain listed (or none) so the correct lockfile (pnpm-lock.yaml or
yarn.lock or package-lock.json) is tracked in Git.
In `@examples/react/composition/README.md`:
- Around line 3-6: Update the "To run this example:" instructions to explicitly
set the working directory before running npm commands: add a step like "cd
examples/react/composition" (or similar wording) before the existing `npm
install` and `npm run dev` lines so users know to change into the example
directory; ensure the "To run this example:" section in README.md includes that
cd step and that the subsequent commands remain in the same ordered list.
In `@packages/react-form/src/createFormHook.tsx`:
- Around line 641-651: The merge in createFormHook currently lets
extension.fieldComponents and extension.formComponents silently overwrite
existing keys at runtime; before creating the merged objects (the
fieldComponents and formComponents passed into createFormHook), iterate the keys
of extension.fieldComponents and extension.formComponents and check for
collisions against the base fieldComponents and formComponents respectively
(using Object.prototype.hasOwnProperty or similar), and if any duplicate key is
found, throw a clear error (or at minimum console.error and throw) naming the
duplicate key and whether it came from fieldComponents or formComponents so JS
consumers cannot accidentally overwrite base components at runtime.
---
Nitpick comments:
In `@examples/react/composition/src/AppForm/FieldComponents/TextField.tsx`:
- Around line 16-19: The visual error message rendered from
field.state.meta.errors should be programmatically associated with the input:
when field.state.meta.isTouched && !field.state.meta.isValid render the error
element with a stable id (e.g., `${field.input.name || field.input.id}-error`)
and on the input element (in TextField component where the input is rendered)
set aria-invalid={field.state.meta.isValid === false} and
aria-describedby={field.state.meta.isValid ? undefined : `${field.input.name ||
field.input.id}-error`} so screen readers will announce the error; ensure the
error element contains that id and remains hidden/omitted when there are no
errors.
In `@examples/react/composition/src/index.tsx`:
- Around line 66-68: document.getElementById('root') is using a non-null
assertion on rootElement which will throw a less-informative error if the
element is missing; change the logic around the root lookup in index.tsx to
explicitly guard the result of document.getElementById('root') (the rootElement
variable) and either throw a clear Error (e.g., "Root element '#root' not
found") or bail out gracefully before calling createRoot(...).render so
createRoot and render are only invoked when rootElement is non-null.
In `@packages/react-form/tests/createFormHook.test.tsx`:
- Around line 703-952: Add negative-path tests covering the new runtime guards
in createFormHook by asserting that calling extendForm with reserved keys and
with keys that duplicate base hook components throws: write tests that call
createFormHook(...).extendForm({...}) passing a fieldComponents or
formComponents object that includes a reserved key (e.g. keys used by the hook
API like AppField/AppForm or other reserved identifiers your runtime check
rejects) and assert it throws, and another test where extendForm is passed a key
that already exists on the base hook (duplicate base key) and assert that also
throws; use the same test patterns and helpers as the existing extendForm tests
(referencing extendForm, createFormHook, useAppForm, withForm) so the guard
behavior is covered and regression-proof.
🪄 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: 9864e5dd-df27-41da-8fa0-7d99e0e7d1bd
⛔ 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
Comment on lines
+216
to
+223
| export const { useAppForm } = ProfileForm.extendForm({ | ||
| fieldComponents: { CustomTextField }, | ||
| // Ts will error since the parent appForm already has a component called SubmitButton | ||
| formComponents: { SubmitButton }, | ||
| }) | ||
| ``` | ||
|
|
||
| This way you can add extra fields that are unique to your team without bloating the upstream AppForm. |
There was a problem hiding this comment.
Primary extension example currently demonstrates a failing snippet.
At Line 219 the sample intentionally triggers a TS collision, but this appears in the main “how to extend” flow and is followed by success-oriented text. Please make the primary snippet valid (unique component name), and move the collision case to a separate “gotcha” block.
Suggested patch
import ProfileForm from 'weyland-yutan-corp/forms'
import { CustomTextField } from './FieldComponents/CustomTextField'
-import { SubmitButton } from './FormComponents/SubmitButton'
+import { CustomSubmitButton } from './FormComponents/CustomSubmitButton'
export const { useAppForm } = ProfileForm.extendForm({
fieldComponents: { CustomTextField },
- // Ts will error since the parent appForm already has a component called SubmitButton
- formComponents: { SubmitButton },
+ formComponents: { CustomSubmitButton },
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/react/guides/form-composition.md` around lines 216 - 223, The
primary example shows a TypeScript collision by reusing SubmitButton; update the
main snippet so ProfileForm.extendForm uses a uniquely named component (e.g.,
formComponents: { TeamSubmitButton } or another distinct identifier) and expose
useAppForm from that result; then move the collision example (reusing
SubmitButton) into a separate "Gotcha" or "Type collision" block with an
explanatory note. Target the ProfileForm.extendForm call, the exported
useAppForm binding, and the component names CustomTextField and SubmitButton
when making the edits.
Comment on lines
+14
to
+16
| pnpm-lock.yaml | ||
| yarn.lock | ||
| package-lock.json |
There was a problem hiding this comment.
Lockfiles should be committed for reproducible builds.
Ignoring all package manager lockfiles (pnpm-lock.yaml, yarn.lock, package-lock.json) violates modern JavaScript best practices. Lockfiles ensure consistent dependency versions across development environments and deployments. Since this is an example project, it should demonstrate the recommended practice of committing the lockfile for whichever package manager is in use.
📦 Recommended fix
Keep only the lockfiles you don't use:
-pnpm-lock.yaml
-yarn.lock
-package-lock.json
+# Commit the lockfile for your package manager
+# If using pnpm, ignore these:
+yarn.lock
+package-lock.json
+
+# If using yarn, ignore these:
+# pnpm-lock.yaml
+# package-lock.json
+
+# If using npm, ignore these:
+# pnpm-lock.yaml
+# yarn.lock📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pnpm-lock.yaml | |
| yarn.lock | |
| package-lock.json | |
| # Commit the lockfile for your package manager | |
| # If using pnpm, ignore these: | |
| yarn.lock | |
| package-lock.json | |
| # If using yarn, ignore these: | |
| # pnpm-lock.yaml | |
| # package-lock.json | |
| # If using npm, ignore these: | |
| # pnpm-lock.yaml | |
| # yarn.lock |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/react/composition/.gitignore` around lines 14 - 16, The .gitignore
currently excludes pnpm-lock.yaml, yarn.lock, and package-lock.json which
prevents committing lockfiles; update the .gitignore in
examples/react/composition to stop ignoring the lockfile your example uses
(e.g., remove pnpm-lock.yaml if the project uses pnpm, or remove
yarn.lock/package-lock.json for their respective managers) so the appropriate
lockfile is committed for reproducible installs; ensure only irrelevant
lockfiles remain listed (or none) so the correct lockfile (pnpm-lock.yaml or
yarn.lock or package-lock.json) is tracked in Git.
Comment on lines
+3
to
+6
| To run this example: | ||
|
|
||
| - `npm install` | ||
| - `npm run dev` |
There was a problem hiding this comment.
Specify the working directory before run commands.
The commands are ambiguous as written. Add an explicit cd examples/react/composition step to avoid running them from repo root.
Suggested patch
To run this example:
+- `cd examples/react/composition`
- `npm install`
- `npm run dev`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| To run this example: | |
| - `npm install` | |
| - `npm run dev` | |
| To run this example: | |
| - `cd examples/react/composition` | |
| - `npm install` | |
| - `npm run dev` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/react/composition/README.md` around lines 3 - 6, Update the "To run
this example:" instructions to explicitly set the working directory before
running npm commands: add a step like "cd examples/react/composition" (or
similar wording) before the existing `npm install` and `npm run dev` lines so
users know to change into the example directory; ensure the "To run this
example:" section in README.md includes that cd step and that the subsequent
commands remain in the same ordered list.
Comment on lines
+641
to
+651
| return createFormHook({ | ||
| fieldContext, | ||
| formContext, | ||
| fieldComponents: { | ||
| ...fieldComponents, | ||
| ...extension.fieldComponents, | ||
| } as TComponents & TNewField, | ||
| formComponents: { | ||
| ...formComponents, | ||
| ...extension.formComponents, | ||
| } as TFormComponents & TNewForm, |
There was a problem hiding this comment.
Prevent silent overwrite of base components in runtime (extendForm).
At Line 645 and Line 649, extension keys can still overwrite base keys at runtime. Type checks catch this in TS, but JS consumers can bypass it and get silent behavior changes. Add explicit duplicate-key runtime checks against fieldComponents / formComponents before merging.
Suggested patch
+ const duplicateField = Object.keys(extension.fieldComponents ?? {}).find(
+ (k) => k in fieldComponents,
+ )
+ if (duplicateField) {
+ throw new Error(
+ `extendForm: "${duplicateField}" already exists in base field components.`,
+ )
+ }
+
+ const duplicateForm = Object.keys(extension.formComponents ?? {}).find(
+ (k) => k in formComponents,
+ )
+ if (duplicateForm) {
+ throw new Error(
+ `extendForm: "${duplicateForm}" already exists in base form components.`,
+ )
+ }
+
return createFormHook({
fieldContext,
formContext,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return createFormHook({ | |
| fieldContext, | |
| formContext, | |
| fieldComponents: { | |
| ...fieldComponents, | |
| ...extension.fieldComponents, | |
| } as TComponents & TNewField, | |
| formComponents: { | |
| ...formComponents, | |
| ...extension.formComponents, | |
| } as TFormComponents & TNewForm, | |
| const duplicateField = Object.keys(extension.fieldComponents ?? {}).find( | |
| (k) => k in fieldComponents, | |
| ) | |
| if (duplicateField) { | |
| throw new Error( | |
| `extendForm: "${duplicateField}" already exists in base field components.`, | |
| ) | |
| } | |
| const duplicateForm = Object.keys(extension.formComponents ?? {}).find( | |
| (k) => k in formComponents, | |
| ) | |
| if (duplicateForm) { | |
| throw new Error( | |
| `extendForm: "${duplicateForm}" already exists in base form components.`, | |
| ) | |
| } | |
| return createFormHook({ | |
| fieldContext, | |
| formContext, | |
| fieldComponents: { | |
| ...fieldComponents, | |
| ...extension.fieldComponents, | |
| } as TComponents & TNewField, | |
| formComponents: { | |
| ...formComponents, | |
| ...extension.formComponents, | |
| } as TFormComponents & TNewForm, |
🤖 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 641 - 651, The merge
in createFormHook currently lets extension.fieldComponents and
extension.formComponents silently overwrite existing keys at runtime; before
creating the merged objects (the fieldComponents and formComponents passed into
createFormHook), iterate the keys of extension.fieldComponents and
extension.formComponents and check for collisions against the base
fieldComponents and formComponents respectively (using
Object.prototype.hasOwnProperty or similar), and if any duplicate key is found,
throw a clear error (or at minimum console.error and throw) naming the duplicate
key and whether it came from fieldComponents or formComponents so JS consumers
cannot accidentally overwrite base components at runtime.
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
Fixes the unresolved CodeRabbit review comments left on #2106 (
feat(react-form): extend appform), which introducedextendForm()and the composition example project.Actionable bugs fixed
firstNamefield (examples/react/composition/src/index.tsx): the field was rendering"last name"instead of"First Name".onBluronTextFieldinput (src/AppForm/FieldComponents/TextField.tsx):field.handleBlurwas never wired up, soisTouchedandonBlurvalidators never fired.falseinstead ofundefinedwhen valid (src/index.tsx): short-circuit&&expression replaced with an explicit ternary returningundefined.docs/framework/react/guides/form-composition.md):Weyland-Yutan-corp/forms→weyland-yutan-corp/forms;CustomSubmit(doesn't exist in parent) →SubmitButton(the real collision).Nitpicks / improvements
extendFormnow validates against areservedNamesset (AppField,AppForm,Field,Subscribe,handleSubmit, etc.) and throws a clear error if a key would silently override a core runtime API.extendFormnow assertsThirdFieldsurvived the second extension and useswithDoublyExtendedForminstead of the singly-extendedwithExtendedForm."Simple Example App"to"Composition Example App".extendFormexported fromAppForm.tsxfor downstream composition.TextField: TextField→TextField(object shorthand).Test plan
pnpm testpasses inpackages/react-formcreateFormHook.test-d.tsx) pass including the doubly-extended chain assertiononBlurvalidation firingRelates to #2106
Summary by CodeRabbit
New Features
extendFormmethod enabling extension of form hooks with custom field and form components, with built-in validation to prevent naming conflicts.Documentation