fix(shared): InputController relies on FormContext being provided #17511
Conversation
RunarVestmann
added
the
deprecated:automerge
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThe pull request modifies the Changes
Suggested reviewers
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 9 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/shared/form-fields/src/lib/InputController/InputController.tsx (2)
136-138: Consider extracting the repeated clearOnChange logic.The same conditional logic for clearing inputs is duplicated across four different onChange handlers. This violates the DRY principle and makes maintenance more difficult.
Consider extracting this into a shared helper function:
+ const handleClearOnChange = ( + e: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement> + ) => { + if (onInputChange) { + onInputChange(e) + } + if (clearOnChange && formContext?.setValue) { + clearInputsOnChange(clearOnChange, formContext.setValue) + } + } // Then use it in all onChange handlers: onChange={(e) => { - if (onInputChange) { - onInputChange(e) - } - if (clearOnChange && formContext?.setValue) { - clearInputsOnChange(clearOnChange, formContext.setValue) - } + handleClearOnChange(e) }}Also applies to: 179-181, 222-224, 264-266
Line range hint
1-290: Consider documenting the optional FormContext behavior.Since this component now works with or without FormContext, it would be helpful to document this behavior in the component's JSDoc or README.
Add a JSDoc comment above the component:
/** * A form input controller that works both with and without FormContext. * When FormContext is not provided, it falls back to standard input behavior. * * @example * // With FormContext * <FormProvider> * <InputController id="myInput" /> * </FormProvider> * * // Without FormContext * <InputController id="myInput" /> */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/shared/form-fields/src/lib/InputController/InputController.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/shared/form-fields/src/lib/InputController/InputController.tsx (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/shared/form-fields/src/lib/InputController/InputController.tsx (2)
102-102: LGTM! Defensive programming approach.Storing the entire form context instead of destructuring
setValuedirectly makes the code more resilient to undefined context scenarios.
Line range hint
1-290: Verify the impact on existing form implementations.The changes to FormContext handling might affect existing implementations that rely on the previous behavior.
Let's verify the usage patterns:
✅ Verification successful
The FormContext changes are safe and backward compatible
The changes to FormContext handling in InputController are verified to be safe because:
- The FormContext is only used for the optional clearOnChange functionality
- Proper null checks prevent runtime errors
- The pattern is consistently implemented across all form field controllers
- Core form functionality remains unchanged
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing InputController usage patterns # Find all files using InputController echo "Files using InputController:" rg -l "InputController" # Check for FormProvider usage patterns echo "\nFormProvider usage patterns:" rg -A 2 "FormProvider.*>.*InputController"Length of output: 12854
Script:
#!/bin/bash # Search for clearOnChange prop usage in InputController implementations echo "Searching for clearOnChange usage:" rg -A 2 "clearOnChange['\"]?:" # Search for clearInputsOnChange utility function usage echo "\nSearching for clearInputsOnChange utility usage:" rg -A 2 "clearInputsOnChange\("Length of output: 7562
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17511 +/- ##
=======================================
Coverage 35.61% 35.61%
=======================================
Files 7015 7015
Lines 150344 150344
Branches 42983 42983
=======================================
Hits 53543 53543
Misses 96801 96801
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
kodiakhq
bot
deleted the
fix/shared-input-controller-should-not-rely-on-context-being-provided
branch
InputController relies on FormContext being provided
What
This is causing issues in numerous places since the FormProvider isn't always wrapped around the InputController.
Why
Instead of having to manually wrap FormProvider around InputControllers in numerous places in the codebase I suggest we instead make it so that the InputController functions even though the FormContext isn't provided (like it used to do).
Checklist:
Summary by CodeRabbit