chore: enable @eslint-react/eslint-plugin #62
Conversation
🦋 Changeset detectedLatest commit: b8f9e27 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update integrates the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant ESLint Config
participant React Component
Developer->>ESLint Config: Add @eslint-react/eslint-plugin
ESLint Config->>React Component: Enforce React-specific lint rules
React Component->>ESLint Config: Add disable comments/fix keys as needed
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR enables the @eslint-react/eslint-plugin by updating the ESLint configuration and adding inline disable comments in several React component files to work around newly enforced rules.
- Added inline ESLint disable comments for various React rules in multiple components.
- Refactored the Tabs component to remove the forwardRef wrapper.
- Updated eslint.config.js to include recommended React plugin configurations.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/runtime/components/_HeadingTitle.tsx | Added disable comment for no-children-to-array rule. |
| src/runtime/components/TermsTable.tsx | Added disable comment for no-array-index-key on list items. |
| src/runtime/components/Tabs.tsx | Changed Tabs component by removing forwardRef and added eslint-disable comments for children mapping and key usage. |
| src/runtime/components/OpenAPIRef.tsx | Updated list key from index to name in mapped items. |
| src/runtime/components/OpenAPIPath.tsx | Added disable comment for no-array-index-key and adjusted required badge rendering. |
| src/runtime/components/K8sCrd.tsx | Added disable comment for no-direct-set-state-in-use-effect in useEffect. |
| src/global/VersionsNav/index.tsx | Added disable comment for no-direct-set-state-in-use-effect when updating navMenu. |
| eslint.config.js | Imported and applied recommended configurations for the React ESLint plugin. |
Comments suppressed due to low confidence (2)
src/runtime/components/Tabs.tsx:9
- Removing forwardRef changes the component's ref forwarding behavior. If the component is expected to support ref forwarding, consider using React.forwardRef to maintain that contract.
export const Tabs = ({ ref, children, ...props }: TabsProps) => {
src/runtime/components/OpenAPIPath.tsx:57
- [nitpick] Using an array index as a key in list items may cause rendering issues when the list is dynamic. Consider using a unique identifier if one is available.
<X.li key={index}>
commit: |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/runtime/components/K8sCrd.tsx (1)
44-44: Consider refactoring instead of suppressing the ESLint rule.This is similar to a previous concern raised about directly setting state in useEffect. While the current implementation may work, the pattern can lead to synchronization issues and performance problems.
Consider these alternatives:
- Remove local state and use
expandAllprop directly- Use a different pattern like
useMemoor derived state-const [open, setOpen] = useState(expandAll) - -const onToggle = useCallback((open: boolean) => { - setOpen(open) -}, []) - -useEffect(() => { - // eslint-disable-next-line @eslint-react/hooks-extra/no-direct-set-state-in-use-effect - setOpen(expandAll) -}, [expandAll]) +const [manualOverride, setManualOverride] = useState<boolean | null>(null) +const open = manualOverride ?? expandAll + +const onToggle = useCallback((open: boolean) => { + setManualOverride(open) +}, [])
🧹 Nitpick comments (3)
src/runtime/components/TermsTable.tsx (1)
23-24: Consider using string content as key instead of array index.While the ESLint disable is acceptable for this static list, using the string content itself as the key would be more robust and align with React best practices.
- // eslint-disable-next-line @eslint-react/no-array-index-key - <X.li key={index}>{item}</X.li> + <X.li key={item}>{item}</X.li>This approach eliminates the need for the ESLint disable comment and provides better React reconciliation if the list order ever changes.
src/runtime/components/Tabs.tsx (1)
18-24: Consider improving the implementation rather than suppressing ESLint rules.While these suppressions may work for print mode, they represent patterns that could be improved:
For the array index key issue, consider using a more stable identifier:
- // eslint-disable-next-line @eslint-react/no-children-map Children.map(children, (child, index) => ( - // eslint-disable-next-line @eslint-react/no-array-index-key - <Tabs_ key={index} {...props}> + <Tabs_ key={`print-tab-${index}`} {...props}> {child} </Tabs_> ))Alternatively, if the children have stable identifiers, use those instead of array indices.
src/runtime/components/OpenAPIPath.tsx (1)
61-67: Replace explicit space fragments with CSS or non-breaking spaces
Inserting{' '}inside fragments for spacing is brittle. Consider using CSS margin/padding, non-breaking spaces (\u00A0), or template strings (" ") to handle inline spacing more cleanly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/slow-readers-work.md(1 hunks)eslint.config.js(2 hunks)src/global/VersionsNav/index.tsx(1 hunks)src/runtime/components/K8sCrd.tsx(1 hunks)src/runtime/components/OpenAPIPath.tsx(1 hunks)src/runtime/components/OpenAPIRef.tsx(1 hunks)src/runtime/components/Tabs.tsx(1 hunks)src/runtime/components/TermsTable.tsx(1 hunks)src/runtime/components/_HeadingTitle.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/runtime/components/OpenAPIPath.tsx (1)
src/runtime/components/_Markdown.tsx (1)
Markdown(6-21)
🔇 Additional comments (5)
eslint.config.js (1)
4-4: LGTM! Proper React ESLint plugin integration.The React ESLint plugin is correctly integrated following ESLint flat config best practices:
- Clean import statement
- Base configuration added to the main config array
- Type-checked React rules appropriately scoped to TypeScript files
Also applies to: 17-17, 21-25
src/runtime/components/_HeadingTitle.tsx (1)
28-29: ESLint disable comment is appropriately justified.The
@eslint-react/no-children-to-arrayrule is correctly disabled here sinceChildren.toArray()is being used for slug generation from heading content, not for rendering. This is a legitimate use case where converting children to an array is necessary for text extraction.src/global/VersionsNav/index.tsx (1)
123-124:Details
✅ Verification successful
Verify the useEffect logic to prevent potential infinite re-renders.
The ESLint disable comment is understandable given the DOM integration requirements, but the direct state setting in
useEffectcould be risky.Please verify that this useEffect implementation doesn't cause infinite re-renders or performance issues:
The current implementation appears to have guards in place, but consider if a
useCallbackoruseRefapproach might be safer for DOM integration.
🏁 Script executed:
#!/bin/bash # Description: Search for similar patterns and potential useEffect issues in the codebase echo "Searching for other direct setState calls in useEffect..." rg -A 3 -B 3 "useState.*setNavMenu|setNavMenu.*useState" echo -e "\nSearching for other mutation observer patterns..." rg -A 5 -B 5 "MutationObserver.*useEffect|useEffect.*MutationObserver" echo -e "\nSearching for similar DOM integration patterns..." rg -A 3 -B 3 "getNavMenu|querySelector.*nav.*menu"Length of output: 3362
Direct state update in useEffect is safe and won’t loop
I’ve verified that:
- The
useEffectonly runs whensiteData.basechanges.- You compare
newNavMenuagainstnavMenubefore callingsetNavMenu, so it only updates once per actual DOM change.- The
MutationObservercallback lives inside the same effect but only fires on DOM removals, not React updates.Given these guards, the disable comment for
@eslint-react/hooks-extra/no-direct-set-state-in-use-effectis justified here..changeset/slow-readers-work.md (1)
1-6: LGTM!The changeset correctly documents the patch-level update for enabling the React ESLint plugin and fixing related linting issues.
src/runtime/components/OpenAPIRef.tsx (1)
109-110: Excellent improvement!Great choice to use the property name as the key instead of the array index. This provides stable, unique keys that improve React's reconciliation performance and avoid potential rendering issues.
Summary by CodeRabbit