Add @eslint-react and fix issues #535
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes in this pull request primarily involve updates to ESLint configurations and enhancements to various React components. A new ESLint plugin for React has been added, along with specific linting rules for TypeScript files. Several components have been modified to improve their functionality, including updates to props handling, state management, and the introduction of a new custom hook, Changes
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
ivan-dalmet
force-pushed
the
fix/improve-react-code
branch
from
60adc16 to
252a12c
Compare
ivan-dalmet
force-pushed
the
fix/improve-react-code
branch
from
252a12c to
b257f0a
Compare
Add some TODO for some legacy code
ivan-dalmet
force-pushed
the
fix/improve-react-code
branch
from
b257f0a to
54824f0
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (21)
src/hooks/useValueHasChanged.ts (2)
1-9: Good implementation of a custom hook for change detection.The
useValueHasChangedhook is well-implemented and serves its purpose of detecting changes in a value between renders. Here are some observations and suggestions:
The use of
useRefis appropriate for persisting the previous value across renders without causing re-renders itself.The hook is flexible, accepting any type of value (
unknown), which is good for reusability.The strict inequality check (
!==) works well for primitive types but may not accurately detect changes in objects or arrays.Consider the following improvements:
- For better type safety, you could use a generic type parameter:
export const useValueHasChanged = <T>(value: T): boolean => { const valueRef = useRef<T>(value); // ... rest of the implementation }
- To handle object and array comparisons, you might want to add an optional deep comparison:
import { isEqual } from 'lodash'; export const useValueHasChanged = <T>(value: T, deep = false): boolean => { const valueRef = useRef<T>(value); const valueHasChanged = deep ? !isEqual(valueRef.current, value) : valueRef.current !== value; valueRef.current = value; return valueHasChanged; };
- To optimize performance in components that use this hook, consider memoizing the result:
import { useMemo } from 'react'; export const useValueHasChanged = <T>(value: T): boolean => { const valueRef = useRef<T>(value); return useMemo(() => { const valueHasChanged = valueRef.current !== value; valueRef.current = value; return valueHasChanged; }, [value]); };These suggestions can enhance the hook's type safety, flexibility, and performance. Choose the one that best fits your project's needs.
1-9: Usage considerations foruseValueHasChangedhookThis hook provides a simple way to detect value changes between renders, which can be useful in various scenarios. However, keep the following in mind when using it:
It's most reliable with primitive values. For objects or arrays, consider implementing deep comparison as suggested earlier.
Use this hook judiciously. While it's useful for optimizing re-renders or triggering side effects, overuse might complicate your component logic.
Remember that this hook will trigger on every render where the value changes, which might not always be desirable. Consider combining it with
useMemooruseCallbackin the parent component if you need more control over when the change detection occurs.If you're using this hook to trigger side effects, consider whether
useEffectwith proper dependencies might be a more idiomatic React approach in some cases.By keeping these points in mind, you can effectively leverage this hook to improve your component's performance and reactivity.
src/components/MonthPicker/MonthPicker.tsx (2)
16-16: Approve removal of default values from props destructuringRemoving default values from props destructuring is a good practice. It allows for more explicit control from the parent component and aligns with making the component more flexible.
Consider adding type guards or default values where these props are used within the component to ensure type safety and prevent potential undefined errors.
Also applies to: 20-20
23-26: Approve inline fallback for year propThe inline fallback
year={year ?? new Date().getFullYear()}is a good practice. It ensures that a valid year is always provided, maintaining the previous behavior while allowing for more flexibility in prop passing.Consider memoizing the fallback value to avoid unnecessary recalculations on re-renders:
const defaultYear = useMemo(() => new Date().getFullYear(), []); // ... <YearProvider year={year ?? defaultYear} onYearChange={onYearChange}>src/components/ResponsiveIconButton/index.tsx (1)
25-30: ImproveduseBreakpointValueimplementation with a minor suggestion.The use of the nullish coalescing operator for providing a default value is a good improvement. It ensures that the default breakpoints are only applied when
hideTextBreakpointsis undefined or null, making the logic more explicit and aligned with modern JavaScript practices.However, to improve TypeScript compatibility and avoid potential narrowing issues, consider using a type assertion:
const responsiveStates = useBreakpointValue( (hideTextBreakpoints ?? { base: true, md: false, }) as ResponsiveValue<boolean> );This type assertion ensures that the inferred type matches the expected input type for
useBreakpointValue, preventing any potential TypeScript errors.src/components/SearchInput/docs.stories.tsx (1)
22-24: LGTM: Button addition enhances component demonstration.The new button effectively demonstrates how the SearchInput's value can be controlled externally. This is a valuable addition to the story.
Consider using a more meaningful value update instead of a random string. For example:
- <Button onClick={() => setValue(Math.random().toString())}> + <Button onClick={() => setValue(`Updated at ${new Date().toLocaleTimeString()}`)}>This change would make the external updates more intuitive for users viewing the story.
src/components/Icons/docs.stories.tsx (1)
Line range hint
28-44: Consider alternative approaches to avoid using React.cloneElementThe
CustomIconcomponent usesReact.cloneElementto add anonClickhandler to the passed child element. While this approach works, it's flagged by the ESLint rule because it can lead to unexpected behavior and make the component harder to understand and maintain.Consider these alternatives:
Use a wrapper component:
const CustomIcon = ({ children, name }: { children: ReactElement; name: string }) => { const { hasCopied, onCopy } = useClipboard(name); return ( <Tooltip label={hasCopied ? 'Copied!' : name} closeOnClick={false}> <span onClick={onCopy}>{children}</span> </Tooltip> ); };Pass the
onClickhandler as a prop to the child component:const CustomIcon = ({ children, name }: { children: ReactElement; name: string }) => { const { hasCopied, onCopy } = useClipboard(name); return ( <Tooltip label={hasCopied ? 'Copied!' : name} closeOnClick={false}> {React.isValidElement(children) ? React.cloneElement(children, { onClick: onCopy }) : children} </Tooltip> ); };Both alternatives avoid using
React.cloneElementdirectly, which resolves the ESLint warning. The first option is simpler but adds an extra DOM element, while the second maintains the current behavior but with a type check for valid React elements.src/components/MonthPicker/docs.stories.tsx (1)
21-29: LGTM: New story effectively demonstrates external year control.The
UpdateYearFromOutsidestory is a valuable addition, showcasing how to control the MonthPicker's year prop externally. It uses proper React patterns and effectively demonstrates the component's flexibility.Consider adding a text display of the current year for better visibility:
export const UpdateYearFromOutside = () => { const [year, setYear] = useState(1994); return ( <Stack> + <Box>Current Year: {year}</Box> <MonthPicker year={year} onYearChange={(y) => setYear(y)} /> <Button onClick={() => setYear((y) => y + 1)}>Increment Year</Button> </Stack> ); };src/components/DateAgo/index.tsx (1)
25-28: Consider adding test cases for undefined date propThe changes to the
DateAgocomponent, particularly making thedateprop optional and providing a fallback, introduce a new scenario that should be covered by tests.Consider adding the following test cases:
- Rendering the component with an undefined
dateprop.- Verifying the component's behavior when switching from a defined date to undefined.
- Checking the tooltip content when the date is undefined.
These tests will ensure the component behaves correctly in all scenarios and prevent potential regressions in future updates.
Would you like assistance in generating these test cases?
src/components/ConfirmModal/index.tsx (3)
46-47: Address TODO comment and track technical debt.The TODO comment indicates that there's an ESLint rule that needs to be addressed. While disabling the rule might be necessary for now, it's important to track this technical debt and plan for a proper resolution.
Would you like me to create a GitHub issue to track this TODO and ensure it's addressed in the future?
48-51: Approve changes with a minor suggestion for clarity.The implementation for handling the case when
isEnabledis false looks good. It effectively bypasses the modal and directly triggers the confirmation action.For improved clarity, consider adding a brief comment explaining the purpose of this block:
if (!isEnabled) { // Bypass modal and directly trigger confirmation when disabled // eslint-disable-next-line @eslint-react/no-clone-element const childrenWithOnClick = React.cloneElement(children, { onClick: onConfirm, }); return <>{childrenWithOnClick}</>; }
54-58: Consider refactoring to reduce code duplication.The implementation for handling the enabled state is correct. However, there's a repeated pattern with the TODO comment and ESLint disable directive.
Consider refactoring to reduce code duplication and centralize the handling of the ESLint rule:
// TODO: Address @eslint-react/no-clone-element rule const cloneElementWithHandler = (element: ReactElement, handler: () => void) => { // eslint-disable-next-line @eslint-react/no-clone-element return React.cloneElement(element, { onClick: handler }); }; // Usage const childrenWithHandler = isEnabled ? cloneElementWithHandler(children, confirmModal.onOpen) : cloneElementWithHandler(children, onConfirm); return <>{childrenWithHandler}</>;This refactoring centralizes the ESLint comment and reduces code duplication, making it easier to manage the TODO and potential future changes.
src/components/SearchInput/index.tsx (1)
69-72: Improved logic for handling external value changes.The new implementation using
useValueHasChangedis more efficient and avoids unnecessary re-renders. It's a good replacement for the previoususeEffectapproach.Consider adding a brief comment explaining the purpose of this logic block for better code readability:
+ // Sync search state with external value changes const externalValueHasChanged = useValueHasChanged(externalValue); if (externalValueHasChanged && externalValue !== searchRef.current) { setSearch(externalValue); }src/components/Form/FieldOtp/index.tsx (3)
105-105: Consider alternatives to using array index as keyWhile disabling the ESLint rule is acceptable in this case, using array indices as keys can potentially lead to issues with component identity and state persistence during re-renders. Although the current implementation is unlikely to cause problems due to the fixed nature of the array, consider the following alternatives for improved robustness:
- Use a combination of a prefix and index:
key={pin-${index}}.- If the OTP digits have a specific meaning or position, use that information to create unique keys.
- // eslint-disable-next-line @eslint-react/no-array-index-key - key={index} + key={`pin-${index}`}
Line range hint
67-82: Consider extracting auto-submit logic into a custom hookThe auto-submit feature is a good addition, but it might benefit from being extracted into a custom hook. This would improve reusability, testability, and separation of concerns.
Consider creating a custom hook like this:
const useAutoSubmit = (shouldAutoSubmit: boolean, isSubmitted: boolean) => { const formRef = useRef<HTMLFormElement>(null); const triggerSubmit = useCallback(() => { if (shouldAutoSubmit && !isSubmitted) { formRef.current?.requestSubmit(); } }, [shouldAutoSubmit, isSubmitted]); return { formRef, triggerSubmit }; };Then use it in your component:
const { formRef, triggerSubmit } = useAutoSubmit(props.autoSubmit, formState.isSubmitted); // In your JSX <form ref={formRef}> {/* ... */} <PinInput // ... onComplete={(v) => { props.onComplete?.(v); triggerSubmit(); }} > {/* ... */} </PinInput> </form>This approach eliminates the need for DOM manipulation and provides a cleaner, more React-like solution.
Line range hint
52-66: Improve focus management logicThe current focus management logic using a hidden input field is a bit hacky. Consider using Chakra UI's
useFocusManagerhook or React'suseImperativeHandlefor a more robust solution.Here's an example using
useImperativeHandle:import React, { useRef, useImperativeHandle, forwardRef } from 'react'; const FieldOtp = forwardRef((props, ref) => { const inputRef = useRef(null); useImperativeHandle(ref, () => ({ focus: () => { inputRef.current?.focus(); }, })); return ( <PinInput ref={inputRef} {...props}> {/* ... */} </PinInput> ); });This approach provides a cleaner way to manage focus without relying on DOM manipulation.
src/components/Nav/index.tsx (2)
49-52: Good use ofuseMemofor context value optimization.The introduction of
contextValueusinguseMemois a good practice for optimizing context updates. This approach can prevent unnecessary re-renders of child components that consume this context.Consider adding a custom comparison function to
useMemofor fine-grained control over when the context value should be recalculated:const contextValue = useMemo( () => ({ active, setActive, isMenu: !!isMenu }), [active, isMenu], (prevDeps, nextDeps) => prevDeps[0] === nextDeps[0] && prevDeps[1] === nextDeps[1] );This ensures that the context value is only recalculated when
activeorisMenuactually change, potentially further optimizing performance.Also applies to: 54-54
Line range hint
93-98: Good use ofuseMemoforitemContent, butuseEffectneeds adjustment.The memoization of
itemContentis a good optimization that can prevent unnecessary re-renders. TheuseEffecthook is also a good addition to update theactivestate whenisActivechanges.However, the
useEffectdependency array is missingsetActive, which could lead to stale closures. Please update theuseEffecthook as follows:useEffect(() => { if (isActive) { setActive(itemContent); } }, [isActive, setActive, itemContent]);This ensures that the effect will re-run if
setActivechanges, preventing potential bugs due to stale closures.Also applies to: 117-121
🧰 Tools
🪛 Biome
[error] 48-48: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
package.json (1)
111-111: LGTM! Consider using caret (^) for version flexibility.The addition of
@eslint-react/eslint-pluginaligns with the PR objectives and enhances the project's linting capabilities for React. The chosen version (1.14.3) is appropriate.Consider using the caret (^) for version flexibility:
- "@eslint-react/eslint-plugin": "1.14.3", + "@eslint-react/eslint-plugin": "^1.14.3",This allows for compatible updates within the same major version, which can include bug fixes and non-breaking changes.
src/features/admin/AdminNavBar.tsx (1)
325-325: LGTM! Great improvements to accessibility and user experience.The changes to the
AdminNavBarAccountMenuVersioncomponent are well-implemented:
- Adding
type="button"improves accessibility and semantics.- The new copy functionality enhances user experience.
- Conditional rendering provides clear feedback on the copy action.
- Using the
useClipboardhook is an efficient way to handle clipboard operations.Consider adding an
aria-labelto the button to further improve accessibility:<Flex role="group" as="button" type="button" + aria-label={t('admin:layout.accountMenu.version.copyLabel')} position="relative" w="full" // ... other props >Don't forget to add the corresponding translation key.
Also applies to: 325-385
src/components/DayPicker/hooks/useDayPickerInputManagement.ts (1)
Line range hint
61-63: Translate code comments to EnglishThe inline comments are in French, which may be difficult for non-French-speaking team members to understand. For better collaboration and code maintainability, consider translating the comments to English.
Apply this diff to update the comments:
- // Pour éviter le problème de non sélection quand : - // * L'input est focus avec une valeur déjà sélectionnée - // * On clique directement sur une nouvelle date + // To avoid the issue of non-selection when: + // * The input is focused with an already selected value + // * We directly click on a new date
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
- .eslintrc.json (2 hunks)
- package.json (1 hunks)
- src/components/ConfirmMenuItem/index.tsx (1 hunks)
- src/components/ConfirmModal/index.tsx (1 hunks)
- src/components/ConfirmPopover/index.tsx (1 hunks)
- src/components/DateAgo/index.tsx (1 hunks)
- src/components/DayPicker/_partials/DayPickerContent.tsx (1 hunks)
- src/components/DayPicker/hooks/useDayPickerInputManagement.ts (2 hunks)
- src/components/DayPicker/index.tsx (3 hunks)
- src/components/Form/FieldOtp/index.tsx (1 hunks)
- src/components/Form/form-test-utils.tsx (1 hunks)
- src/components/Icons/docs.stories.tsx (1 hunks)
- src/components/InputNumber/index.tsx (1 hunks)
- src/components/MonthPicker/MonthPicker.tsx (1 hunks)
- src/components/MonthPicker/MonthPickerContext.tsx (2 hunks)
- src/components/MonthPicker/YearContext.tsx (2 hunks)
- src/components/MonthPicker/docs.stories.tsx (2 hunks)
- src/components/Nav/index.tsx (1 hunks)
- src/components/Pagination/index.tsx (2 hunks)
- src/components/ResponsiveIconButton/index.tsx (1 hunks)
- src/components/SearchInput/docs.stories.tsx (2 hunks)
- src/components/SearchInput/index.tsx (2 hunks)
- src/components/Sort/index.tsx (3 hunks)
- src/features/admin/AdminNavBar.tsx (1 hunks)
- src/features/auth/LoginForm.tsx (1 hunks)
- src/hooks/useIsHydrated.ts (1 hunks)
- src/hooks/useValueHasChanged.ts (1 hunks)
- src/lib/i18n/useLocale.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/hooks/useIsHydrated.ts
🧰 Additional context used
🔇 Additional comments (36)
src/lib/i18n/useLocale.ts (1)
15-16:⚠️ Potential issueAddress the TODO comment and consider alternatives to disabling the ESLint rule.
The added TODO comment and ESLint directive raise some concerns:
- The TODO comment suggests this is a temporary solution. It's important to address this in the near future to maintain code quality.
- Disabling the '@eslint-react/hooks-extra/no-direct-set-state-in-use-effect' rule can potentially lead to race conditions or unnecessary re-renders in other scenarios.
Consider the following alternatives:
- Use the
useCallbackhook to memoize thesetLocalefunction.- Move the state update logic to a separate function outside of
useEffect.- If the current implementation is indeed the best approach for this specific case, add a comment explaining why the rule is disabled to provide context for future developers.
To ensure this ESLint rule isn't disabled elsewhere, run the following script:
Would you like assistance in implementing one of the suggested alternatives?
✅ Verification successful
ESLint rule disable instances are limited and verified
- The rule
@eslint-react/hooks-extra/no-direct-set-state-in-use-effectis disabled in:
src/hooks/useIsHydrated.tssrc/lib/i18n/useLocale.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of this ESLint rule being disabled rg "eslint-disable.*@eslint-react/hooks-extra/no-direct-set-state-in-use-effect" --type tsLength of output: 329
src/components/MonthPicker/MonthPickerContext.tsx (1)
15-22: Excellent performance optimization using useMemo!The introduction of
useMemoto memoize the context value is a great improvement. This optimization prevents unnecessary re-renders of child components that consume this context, especially when the parent component re-renders for reasons unrelated to the context value.The dependency array
[onMonthClick, onTodayButtonClick, selectedMonths]is correctly implemented, ensuring that the memoized value is only recalculated when these props change.This change aligns with React best practices for optimizing context providers and should lead to better performance in complex component trees.
Also applies to: 24-24
src/components/Form/form-test-utils.tsx (1)
17-17: Approve change, but verify impact and add clarification.The removal of the default value for
useFormOptionsis a good change as it aligns with the TypeScript type definition and allows for more flexible usage. However, please consider the following:
- Verify that this change doesn't break any existing tests or components using
FormMocked.- Add a comment explaining why
useFormOptionsdoesn't have a default value, to prevent future developers from mistakenly adding it back.To verify the impact of this change, run the following script:
Consider adding a comment above the
useFormOptionsparameter:// Intentionally not defaulted to {} to allow undefined to be passed explicitly useFormOptions,src/components/MonthPicker/MonthPicker.tsx (1)
30-30: Approve inline fallback for selectedMonths propThe inline fallback
selectedMonths={selectedMonths ?? []}is a good practice. It ensures that a valid array is always provided, maintaining the previous behavior while allowing for more flexibility in prop passing..eslintrc.json (2)
22-25: LGTM! Consider clarifying the use of 'legacy' configuration.The addition of a TypeScript-specific override is a good practice. It ensures that React-specific linting rules are applied to TypeScript files.
Could you please clarify why the "recommended-legacy" configuration was chosen over the standard "recommended" configuration? This choice might impact future compatibility.
To help with the decision, let's check the available configurations:
#!/bin/bash # Description: List available @eslint-react configurations # Test: Check available configurations in the plugin if [ -d "node_modules/@eslint-react" ]; then grep -r "module.exports = {" node_modules/@eslint-react/eslint-plugin/lib/configs --include="*.js" | sed 's/.*configs\/\(.*\)\.js.*/\1/' else echo "Plugin is not installed. Unable to list configurations." fi
Line range hint
1-43: Overall, good improvements to ESLint configuration.The changes in this file successfully integrate the @eslint-react plugin and add TypeScript-specific configurations. These improvements align well with the PR objectives and should enhance the project's linting capabilities, particularly for React components.
To ensure these changes are effective, consider running the linter on your TypeScript files and checking for any new issues or improvements:
#!/bin/bash # Description: Run ESLint on TypeScript files # Test: Run ESLint on TypeScript files npx eslint "**/*.{ts,tsx}" --max-warnings=0 # Note: This script assumes that ESLint is installed and configured in your project. # If you encounter any issues, make sure to check your ESLint installation and configuration.src/components/ResponsiveIconButton/index.tsx (2)
22-22: Improved prop handling forhideTextBreakpoints.The removal of the default value for
hideTextBreakpointsin the props destructuring is a good change. This allows for more flexible handling of the prop and aligns well with the subsequent changes in theuseBreakpointValuehook usage.
Line range hint
1-30: Overall assessment: Positive improvements to the ResponsiveIconButton component.The changes made to this component enhance its flexibility in handling the
hideTextBreakpointsprop. The removal of the default value in the props destructuring and the use of the nullish coalescing operator inuseBreakpointValueare good practices that align with modern JavaScript and React patterns.These modifications contribute to the project's goal of improving React components, as mentioned in the PR objectives. The core logic of the component remains intact, ensuring that existing functionality is preserved while enhancing its implementation.
src/components/SearchInput/docs.stories.tsx (1)
3-3: LGTM: Import statement updated correctly.The import statement has been properly updated to include the
Buttoncomponent from '@chakra-ui/react', which is used in the Controlled story.src/components/MonthPicker/docs.stories.tsx (2)
3-3: LGTM: New imports added for the new story.The addition of
ButtonandStackimports from '@chakra-ui/react' is consistent with their usage in the newUpdateYearFromOutsidestory.
Line range hint
1-29: Summary: Valuable addition to MonthPicker storiesThe changes in this file enhance the documentation and testing of the MonthPicker component by adding a new story that demonstrates external year control. This addition is consistent with the PR objectives of improving React components and aligns well with the ESLint-related changes mentioned in the PR description.
The new story provides a clear example of how to use the MonthPicker in a more dynamic context, which will be helpful for developers integrating this component into their applications. It also indirectly validates the changes made to the MonthPicker component itself, ensuring that the year prop can be controlled externally.
Overall, these changes contribute positively to the component's documentation and usability.
src/components/DateAgo/index.tsx (2)
25-25: LGTM: Improved prop destructuringThe function signature has been updated to explicitly destructure
dateandformatprops, which enhances readability and type safety. The use of rest props (...rest) maintains flexibility for passing additional props to the component.
28-28: Verify intended behavior with undefined date propThe initialization of
dayjsDatenow includes a fallback tonew Date()whendateis not provided. This change improves robustness by ensuringdayjsDatealways has a valid value.Please confirm if defaulting to the current date when
dateis undefined is the intended behavior. If not, consider the following alternatives:
- Return null earlier in the component if
dateis undefined.- Use a more specific default value that indicates no date was provided.
To verify the current usage of the
DateAgocomponent, you can run the following script:✅ Verification successful
Verified:
dayjsDateinitialization functions as intendedAll instances of the
DateAgocomponent provide adateprop, ensuring the fallback tonew Date()is never triggered.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of DateAgo component to verify how it's being used with undefined date prop # Search for DateAgo component usage echo "DateAgo component usage:" rg -n '<DateAgo' --type tsx # Search for DateAgo component imports echo "\nDateAgo component imports:" rg -n 'import.*DateAgo' --type tsxLength of output: 248
Script:
#!/bin/bash # Description: Find usages of DateAgo component to verify how it's being used with undefined date prop # Search for DateAgo component usage in .tsx files echo "DateAgo component usage:" rg -n '<DateAgo' -g '*.tsx' # Search for DateAgo component imports in .tsx files echo "\nDateAgo component imports:" rg -n 'import.*DateAgo' -g '*.tsx'Length of output: 819
src/features/auth/LoginForm.tsx (3)
31-31: ImprovedonSuccesscallback signatureThe modification to the
onSuccessprop type enhances the component's flexibility and utility. By providing access to login data and variables, consumers of this component can now perform more sophisticated actions upon successful login.This change:
- Maintains backward compatibility by keeping the prop optional.
- Aligns with best practices for callback props in React components.
- Improves the overall API of the
LoginFormcomponent.
31-31: Summary of changesThe modification to the
LoginFormcomponent enhances its API by providing a more informativeonSuccesscallback. This change:
- Improves flexibility for consumers of the component.
- Maintains backward compatibility.
- Aligns with React best practices.
Overall, this is a positive change that contributes to the PR's objective of improving React components. Ensure to verify the component's usage across the codebase to maintain consistency.
31-31: Verify usage ofLoginFormcomponentThe change to the
onSuccessprop type is an improvement, but it's important to ensure that all usages of theLoginFormcomponent across the codebase are compatible with this change.Please run the following script to check for any potential issues:
Review the output to ensure that all
onSuccessprop usages are compatible with the new type signature. Update any instances that may need modification to work with the new callback structure.✅ Verification successful
Verify usage of
LoginFormcomponentPlease run the following script to check for any potential issues:
LoginFormcomponent usage is compatibleAll usages of the
LoginFormcomponent have been verified and are compatible with the updatedonSuccessprop type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of LoginForm component and verify onSuccess prop usage # Search for LoginForm usages echo "Searching for LoginForm usages:" rg -n "<LoginForm" --type tsx # Search for onSuccess prop specifically echo "\nSearching for onSuccess prop usage:" rg -n "onSuccess=\{" --type tsx | rg "LoginForm"Length of output: 294
Script:
#!/bin/bash # Description: Find all usages of LoginForm component and verify onSuccess prop usage # Search for LoginForm usages echo "Searching for LoginForm usages:" rg -n "<LoginForm" -g "*.tsx" # Search for onSuccess prop specifically within LoginForm echo "\nSearching for onSuccess prop usage:" rg -n "onSuccess=\{" -g "*.tsx" | rg "LoginForm"Length of output: 404
src/components/ConfirmPopover/index.tsx (1)
Line range hint
53-58: Approve with suggestions: Consider alternatives and address ESLint concernsThe implementation looks good overall and provides a fallback behavior when
isEnabledis false, improving user experience. However, there are a few points to consider:
Cloning elements in React can sometimes lead to unexpected behavior. Consider investigating alternatives, such as using a wrapper component or a render prop pattern.
The TODO comment and ESLint disable directive suggest that there might be a better way to handle this. Please investigate the ESLint rule
@eslint-react/no-clone-elementand see if there's a recommended alternative approach.If you decide to keep this implementation, add a comment explaining the rationale behind cloning the element and why it's necessary in this case. This will help future maintainers understand the decision.
To ensure consistency across the codebase, let's check if similar patterns are used elsewhere:
Consider refactoring to avoid cloning elements if possible. Here's a potential alternative using a wrapper component:
const ChildWrapper: React.FC<{ isEnabled: boolean; onConfirm: () => void; children: ReactElement }> = ({ isEnabled, onConfirm, children }) => { if (!isEnabled) { return <div onClick={onConfirm}>{children}</div>; } return children; }; // Usage in render return ( <ChildWrapper isEnabled={isEnabled} onConfirm={onConfirm}> {children} </ChildWrapper> );This approach avoids cloning elements while achieving the same functionality.
✅ Verification successful
Approve with ESLint Concerns Addressed
The implementation is consistent with existing patterns in the codebase and provides the necessary fallback behavior. However, please address the ESLint rule
@eslint-react/no-clone-elementby either finding an alternative approach or justifying the need to disable the rule. This will help maintain code quality and consistency across the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar patterns of cloning elements with onClick handlers # Test: Search for similar patterns in other files rg -A 5 'React\.cloneElement.*onClick:'Length of output: 447
src/components/SearchInput/index.tsx (2)
17-17: LGTM: New hook import added correctly.The import statement for the new
useValueHasChangedhook is correctly added and follows React naming conventions for custom hooks.
Line range hint
1-118: Summary: Improved efficiency in handling external value changes.The changes in this file effectively improve the
SearchInputcomponent by:
- Introducing the
useValueHasChangedhook for more efficient state updates.- Replacing the previous
useEffectapproach with a more targeted update mechanism.These modifications align well with the PR objectives of fixing ESLint issues and enhancing code quality. The overall structure and functionality of the component remain intact while achieving better performance.
src/components/Form/FieldOtp/index.tsx (1)
Line range hint
1-116: Overall component reviewThe
FieldOtpcomponent is well-structured and makes good use of Chakra UI and react-hook-form. The implementation is generally solid, with a few areas for potential improvement:
- The ESLint disable comment for array index keys has been addressed.
- The auto-submit feature could be extracted into a custom hook for better reusability and testability.
- The focus management logic could be improved using React's built-in features or Chakra UI's hooks.
These suggestions aim to enhance the component's robustness and maintainability without significantly altering its current functionality.
src/components/Sort/index.tsx (6)
42-47: Improved prop handling enhances component flexibility.The removal of default values for
sort,options,onChange,ascIcon, anddescIconprops is a positive change. This modification allows for more explicit control over these props from the parent component, improving the overall flexibility of theSortcomponent. It also aligns well with the ESLint fixes mentioned in the PR objectives.
55-55: Enhanced safety with optional chaining foronChange.The use of optional chaining (
onChange?.) is a good practice. It prevents potential runtime errors when theonChangeprop is not provided, making the component more robust. This change aligns well with the overall improvements in prop handling across the project.
59-59: Consistent use of optional chaining foronChange.The implementation of optional chaining for
onChangeinhandleOrderChangeis consistent with the change inhandleByChange. This maintains a uniform approach to handling the optionalonChangeprop throughout the component, enhancing its overall reliability.
80-82: Improved icon rendering logic with nullish coalescing.The use of the nullish coalescing operator (
??) for renderingascIconanddescIconis an excellent improvement. This change allows for more precise control over when default icons are used, distinguishing betweenundefined/nulland other falsy values. It aligns with modern JavaScript best practices and enhances the component's flexibility.
91-91: Enhanced safety in options handling.The addition of the optional chaining operator when accessing
options(options?.find) is a valuable safety improvement. This change prevents potential runtime errors that could occur if theoptionsprop is undefined. It's consistent with the overall enhancements in prop handling throughout the component and contributes to its robustness.
Line range hint
1-146: Overall assessment: Significant improvements in component robustness and flexibility.The changes made to the
Sortcomponent demonstrate a consistent approach to enhancing prop handling, improving type safety, and increasing the component's overall flexibility. The removal of default values for props, the introduction of optional chaining for callbacks, and the use of nullish coalescing for icon rendering all contribute to a more robust and maintainable component. These modifications align well with the PR objectives of addressing ESLint issues and improving the codebase. The changes are also consistent with improvements seen in other components, as mentioned in the AI-generated summary.src/components/DayPicker/index.tsx (6)
51-51: Improved prop flexibility with optional chainingThe changes to
onClose,inputProps, andonMonthChangeprops, making them optional, enhance the component's flexibility. This allows users to omit these props when not needed, improving the component's usability across different scenarios.Also applies to: 55-55, 58-58
72-72: Safe callback invocation with optional chainingThe use of optional chaining (
onClose?.(value)) ensures that theonClosecallback is only invoked if it's defined. This prevents potential runtime errors and aligns with the prop being optional, enhancing the component's robustness.
115-115: Consistent use of optional chaining for callbacksThe
onMonthChangecallback is now safely invoked using optional chaining, consistent with the earlieronClosechange. This maintains a uniform approach to handling optional callback props throughout the component.
121-121: Consistent callback handling in month selectionThe
onMonthChangecallback in thehandleSelectMonthfunction now uses optional chaining, maintaining consistency with its usage elsewhere in the component. This uniform approach to handling optional callbacks enhances code readability and reliability.
129-129: Safe prop access with optional chainingThe use of optional chaining for
inputProps?.widthensures that thewidthprop is safely accessed, even ifinputPropsis undefined. This change aligns withinputPropsbeing optional and prevents potential runtime errors, enhancing the component's stability.
Line range hint
1-170: Overall improvement in component flexibility and robustnessThe changes in this file consistently enhance the
DayPickercomponent by:
- Making several props optional, increasing flexibility in component usage.
- Implementing optional chaining for callback invocations and prop access, preventing potential runtime errors.
- Maintaining a uniform approach to handling optional props and callbacks throughout the component.
These modifications align with React best practices and significantly improve the component's reliability and ease of use.
src/components/DayPicker/_partials/DayPickerContent.tsx (1)
137-138: 🛠️ Refactor suggestionConsider extracting the Caption component and addressing the ESLint rule.
The TODO comment indicates an outstanding issue with an @eslint-react rule. Consider creating a separate task to address this ESLint rule comprehensively across the project.
The inline definition of the Caption component, while functional, may lead to unnecessary re-renders and goes against best practices for component organization. Consider extracting this into a separate component:
const CustomCaption = (props: CaptionProps) => ( <Caption {...props} onCaptionLabelClick={toggleMode} setMonth={(date) => { handleChangeMonth(date); }} /> );Then, use it in the components prop:
components={{ Caption: CustomCaption, Day, }}
- Once extracted, you can remove the ESLint disable comment, improving code quality and maintainability.
To ensure this change doesn't affect other parts of the codebase, run the following script:
src/components/InputNumber/index.tsx (1)
60-60:⚠️ Potential issueConsider updating type definition to match
onChangeprop changeThe removal of the default value for
onChangeeffectively makes it a required prop. This change enforces better prop hygiene and aligns with React's philosophy of making side effects explicit. However, there are a few points to consider:
- This change might break existing usage of the component where
onChangewas not explicitly provided.- The type definition
InputNumberCustomPropsstill showsonChangeas a required prop, which is now redundant.To maintain consistency and improve type safety, consider updating the
InputNumberCustomPropstype definition:export type InputNumberCustomProps = { value: number | null; defaultValue?: number | null; - onChange(value: number | null): void; + onChange: (value: number | null) => void; // ... other props };This change removes the method signature, making it consistent with the component implementation.
To ensure this change doesn't break existing code, please run the following script to check for any usage of
InputNumberwithout anonChangeprop:If this script returns any results, those instances will need to be updated to provide an
onChangeprop.src/components/Pagination/index.tsx (1)
205-242: Excellent performance optimization!The changes in the
Paginationcomponent implementation are well-thought-out and align with React best practices:
- The destructuring of
getPaginationInforesult improves code readability.- The introduction of
useMemofor the context value is a great optimization. It prevents unnecessary recalculations, which is particularly beneficial in larger or deeply nested component trees.- The dependency array for
useMemois comprehensive, ensuring the memoized value updates when any relevant data changes.- The memoized
contextValueis correctly used in thePaginationContext.Provider.These changes should noticeably improve the performance of the Pagination component, especially in larger applications, without altering its functionality.
src/components/MonthPicker/YearContext.tsx (1)
43-49: Good use ofuseMemoto optimize context valueMemoizing the context value with
useMemoprevents unnecessary re-renders of context consumers by ensuring the context value only changes when theyearchanges. The dependency array[year]is correctly specified to recalculate the memoized value wheneveryearupdates.
| @@ -5,6 +5,7 @@ | |||
| "next/core-web-vitals", | |||
| "plugin:storybook/recommended" | |||
| ], | |||
| "plugins": ["@eslint-react/eslint-plugin"], | |||
There was a problem hiding this comment.
💡 Codebase verification
Plugin "@eslint-react/eslint-plugin" is listed in package.json but not installed in node_modules.
- Please install the plugin by running
npm installor the appropriate package manager command.
🔗 Analysis chain
LGTM! Verify plugin installation.
The addition of the "@eslint-react/eslint-plugin" aligns with the PR objectives and is a good step towards improving React-specific linting.
Please run the following script to verify the plugin installation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify @eslint-react/eslint-plugin installation
# Test: Check if the plugin is listed in package.json
if jq -e '.devDependencies["@eslint-react/eslint-plugin"]' package.json > /dev/null; then
echo "Plugin is listed in package.json"
else
echo "Plugin is not listed in package.json"
fi
# Test: Check if the plugin is installed in node_modules
if [ -d "node_modules/@eslint-react" ]; then
echo "Plugin is installed in node_modules"
else
echo "Plugin is not installed in node_modules"
fi
Length of output: 392
Script:
#!/bin/bash
# Description: Verify @eslint-react/eslint-plugin installation
# Test: Check if the plugin is listed in package.json
if grep -q '"@eslint-react/eslint-plugin"' package.json; then
echo "Plugin is listed in package.json"
else
echo "Plugin is not listed in package.json"
fi
# Test: Check if the plugin is installed in node_modules
if [ -d "node_modules/@eslint-react/eslint-plugin" ]; then
echo "Plugin is installed in node_modules"
else
echo "Plugin is not installed in node_modules"
fi
Length of output: 272
| // TODO @eslint-react rule | ||
| // eslint-disable-next-line @eslint-react/no-clone-element |
There was a problem hiding this comment.
💡 Codebase verification
ESLint rule @eslint-react/no-clone-element is being disabled in multiple files.
Consider refactoring the usage of React.cloneElement to comply with the ESLint rule or updating the ESLint configuration if the rule needs to be relaxed globally.
- Affected Files:
src/components/Icons/docs.stories.tsxsrc/components/ConfirmModal/index.tsxsrc/components/ConfirmMenuItem/index.tsxsrc/components/ConfirmPopover/index.tsx
🔗 Analysis chain
Consider addressing the ESLint rule instead of disabling it.
The added comment indicates that there's a TODO item related to the @eslint-react rule, and the rule @eslint-react/no-clone-element is being disabled for the next line. While this allows the code to pass linting, it might be better to address the underlying issue that the ESLint rule is flagging.
Let's verify if this ESLint rule is disabled globally or if it's a localized exception:
Depending on the results, consider refactoring the CustomIcon component to avoid using React.cloneElement if possible, or provide a clear explanation in the comment about why this rule needs to be disabled here.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for global ESLint configuration and usage of the @eslint-react/no-clone-element rule
# Test 1: Check if there's a global ESLint config file
echo "Checking for global ESLint config:"
fd -e js -e json -e yaml -e yml '\.eslintrc' -x cat {}
# Test 2: Search for other occurrences of this specific ESLint rule being disabled
echo "Searching for other occurrences of @eslint-react/no-clone-element rule being disabled:"
rg -i "@eslint-react/no-clone-element"
# Test 3: Check if React.cloneElement is used elsewhere without the rule being disabled
echo "Searching for other uses of React.cloneElement:"
rg "React\.cloneElement"
Length of output: 1519
| ? // TODO @eslint-react rule | ||
| // eslint-disable-next-line @eslint-react/no-clone-element | ||
| React.cloneElement(icon, { color: 'transparent' }) |
There was a problem hiding this comment.
💡 Codebase verification
Multiple Instances of React.cloneElement Detected
The use of React.cloneElement has been identified in several files:
src/components/Icons/docs.stories.tsxsrc/components/ConfirmPopover/index.tsxsrc/components/ConfirmModal/index.tsxsrc/components/ConfirmMenuItem/index.tsx
This recurring pattern suggests the need for a more sustainable solution to comply with ESLint rules.
🔗 Analysis chain
Address the ESLint rule violation for React.cloneElement
The added comment and ESLint disable directive acknowledge an issue with the use of React.cloneElement. While this is a temporary fix to suppress the ESLint warning, it's important to address this in the future.
-
Consider investigating alternative approaches to avoid using
React.cloneElement. For example, you might be able to pass the necessary props directly to the icon component instead of cloning it. -
Update the TODO comment with more specific information, such as the reason for using
cloneElementhere and potential alternatives to explore. This will help future developers (including yourself) understand the context and requirements when addressing this issue.
Example of a more informative TODO comment:
// TODO: Investigate alternatives to React.cloneElement for icon color override
// Current approach used due to [specific reason]. Consider:
// 1. Passing color prop directly to icon component
// 2. Using a wrapper component for icon styling
// Issue: [link to relevant issue or documentation]To help identify other instances of this pattern in the codebase, you can run the following command:
This will help in addressing similar cases consistently across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of React.cloneElement in the codebase
rg "React\.cloneElement" --type tsx --type ts
Length of output: 75
Script:
#!/bin/bash
# Search for other instances of React.cloneElement in .ts and .tsx files
rg "React\.cloneElement" -g "*.ts" -g "*.tsx"
Length of output: 559
| if (yearHasChanged && yearProp !== year) { | ||
| setYear(yearProp); | ||
| }, [yearProp]); | ||
| } |
There was a problem hiding this comment.
Avoid setting state during rendering to prevent unexpected behavior
Updating state (setYear) directly within the render phase can lead to infinite render loops or other unintended side effects because each state update triggers a new render. In React, state updates should be performed within useEffect hooks or event handlers to ensure they occur after rendering.
To address this, move the state update inside a useEffect hook:
- if (yearHasChanged && yearProp !== year) {
- setYear(yearProp);
- }
+ useEffect(() => {
+ if (yearHasChanged && yearProp !== year) {
+ setYear(yearProp);
+ }
+ }, [yearHasChanged, yearProp, year]);This ensures that setYear is called after the component has rendered, avoiding potential rendering issues.
📝 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.
| if (yearHasChanged && yearProp !== year) { | |
| setYear(yearProp); | |
| }, [yearProp]); | |
| } | |
| useEffect(() => { | |
| if (yearHasChanged && yearProp !== year) { | |
| setYear(yearProp); | |
| } | |
| }, [yearHasChanged, yearProp, year]); |
| if (dateFormatHasChanged || dateValueHasChanged) { | ||
| setInputValue(dateValue ? dayjs(dateValue).format(dateFormat) : ''); | ||
| } |
There was a problem hiding this comment.
Avoid setting state during render
Calling setInputValue directly during render can cause infinite re-renders because updating the state triggers a re-render, which then updates the state again. To prevent this, use the useEffect hook to update inputValue when dateFormat or dateValue changes.
Apply this diff to fix the issue:
+ import { useEffect } from 'react';
...
- if (dateFormatHasChanged || dateValueHasChanged) {
- setInputValue(dateValue ? dayjs(dateValue).format(dateFormat) : '');
- }
+ useEffect(() => {
+ setInputValue(dateValue ? dayjs(dateValue).format(dateFormat) : '');
+ }, [dateFormat, dateValue]);Committable suggestion was skipped due to low confidence.
Describe your changes
Based on this tweet https://x.com/tkdodo/status/1841014700877795699
We added @eslint-react and fixed most of the errors.
Some error was ignored and a // TODO was added. Feel free to contribute to this PR :)
Summary by CodeRabbit
Release Notes
New Features
ConfirmModalandConfirmPopovercomponents to allow direct confirmation actions without displaying the modal when disabled.Captioncomponent in theDayPickerContentfor improved month navigation.MonthPickerto demonstrate dynamic year updates.Improvements
InputNumberto allow explicit passing of theonChangeprop.SortandResponsiveIconButtoncomponents for better flexibility.NavandPaginationcomponents using memoization.Bug Fixes
Documentation