fix: prevent initialFormState from overriding client values during merge#2096
fix: prevent initialFormState from overriding client values during merge#2096Mishra-coder wants to merge 3 commits intoTanStack:mainfrom
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 d6a7af9
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2096 +/- ##
==========================================
- Coverage 90.35% 0.00% -90.36%
==========================================
Files 38 6 -32
Lines 1752 50 -1702
Branches 444 18 -426
==========================================
- Hits 1583 0 -1583
+ Misses 149 42 -107
+ Partials 20 8 -12 ☔ 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: 2
🤖 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-nextjs/tests/createServerValidate.spec.tsx`:
- Around line 17-18: Add a descriptive reason to the existing TypeScript
suppression above the mutateMergeDeep(clientState, initialFormState) call:
replace the bare "// `@ts-ignore`" with a comment that includes the TS error code
and a short justification (e.g., indicating this is intentional test-only
mismatch of types), or alternatively use "// eslint-disable-next-line
`@typescript-eslint/ban-ts-comment` -- <reason>" style; ensure the comment
directly precedes the mutateMergeDeep invocation so ESLint recognizes the
description.
In `@packages/react-form-remix/tests/createServerValidate.spec.tsx`:
- Around line 17-18: Replace the bare "// `@ts-ignore`" above the call to
mutateMergeDeep(clientState, initialFormState) with a documented suppression or
preferred directive: either use "// `@ts-expect-error` -- <reason why types are
incompatible at runtime>" to ensure the suppression fails if types become
compatible, or keep "// `@ts-ignore` -- <explicit reason>" explaining why the type
error is acceptable here; make sure the comment references the mutateMergeDeep
call and why clientState/initialFormState type mismatch is intentionally
ignored.
🪄 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: 8398ee9b-ed6b-43a1-8cd2-af200314aca2
📒 Files selected for processing (2)
packages/react-form-nextjs/tests/createServerValidate.spec.tsxpackages/react-form-remix/tests/createServerValidate.spec.tsx
| // @ts-ignore | ||
| mutateMergeDeep(clientState, initialFormState) |
There was a problem hiding this comment.
Add description to @ts-ignore directive.
Same issue as in the Remix test file. ESLint requires a description for the type suppression.
Proposed fix
- // `@ts-ignore`
+ // `@ts-expect-error` - Testing with simplified mock state that doesn't match full ServerFormState type
mutateMergeDeep(clientState, initialFormState)📝 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.
| // @ts-ignore | |
| mutateMergeDeep(clientState, initialFormState) | |
| // `@ts-expect-error` - Testing with simplified mock state that doesn't match full ServerFormState type | |
| mutateMergeDeep(clientState, initialFormState) |
🧰 Tools
🪛 ESLint
[error] 17-17: Include a description after the "@ts-ignore" directive to explain why the @ts-ignore is necessary. The description must be 3 characters or longer.
(@typescript-eslint/ban-ts-comment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-form-nextjs/tests/createServerValidate.spec.tsx` around lines
17 - 18, Add a descriptive reason to the existing TypeScript suppression above
the mutateMergeDeep(clientState, initialFormState) call: replace the bare "//
`@ts-ignore`" with a comment that includes the TS error code and a short
justification (e.g., indicating this is intentional test-only mismatch of
types), or alternatively use "// eslint-disable-next-line
`@typescript-eslint/ban-ts-comment` -- <reason>" style; ensure the comment
directly precedes the mutateMergeDeep invocation so ESLint recognizes the
description.
| // @ts-ignore | ||
| mutateMergeDeep(clientState, initialFormState) |
There was a problem hiding this comment.
Add description to @ts-ignore directive.
ESLint requires a description explaining why the type suppression is necessary. Consider using @ts-expect-error which is preferred as it will error if the suppression becomes unnecessary.
Proposed fix
- // `@ts-ignore`
+ // `@ts-expect-error` - Testing with simplified mock state that doesn't match full ServerFormState type
mutateMergeDeep(clientState, initialFormState)📝 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.
| // @ts-ignore | |
| mutateMergeDeep(clientState, initialFormState) | |
| // `@ts-expect-error` - Testing with simplified mock state that doesn't match full ServerFormState type | |
| mutateMergeDeep(clientState, initialFormState) |
🧰 Tools
🪛 ESLint
[error] 17-17: Include a description after the "@ts-ignore" directive to explain why the @ts-ignore is necessary. The description must be 3 characters or longer.
(@typescript-eslint/ban-ts-comment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-form-remix/tests/createServerValidate.spec.tsx` around lines
17 - 18, Replace the bare "// `@ts-ignore`" above the call to
mutateMergeDeep(clientState, initialFormState) with a documented suppression or
preferred directive: either use "// `@ts-expect-error` -- <reason why types are
incompatible at runtime>" to ensure the suppression fails if types become
compatible, or keep "// `@ts-ignore` -- <explicit reason>" explaining why the type
error is acceptable here; make sure the comment references the mutateMergeDeep
call and why clientState/initialFormState type mismatch is intentionally
ignored.
1 participant
The
initialFormStateexported by the Next.js and Remix adapters contained avalues: undefinedproperty. During the server-client state merge process (specifically inmutateMergeDeep), this undefined value was being merged into the client's form state, causing existing field values to be reset toundefinedafter initial mount or during route transitions.This fix removes the
valuesproperty from theinitialFormStateobject in both adapters. By omitting the property entirely, the merge logic no longer overwrites existing client values when no server-side values are provided.This approach is safe because:
values: undefinedexplicitly overrides client statevaluespreserves existing client valuesvalues(e.g. after validation) will still correctly override client state as expectedTo maintain type compatibility, a TypeScript cast is used:
as unknown as ServerFormState<any, undefined>This resolves the issue where forms using server actions would have their fields set to
undefinedafter initial render or during client-side navigation.Closes #2089
Summary by CodeRabbit