chore: enable eslint-plugin-react-hooks and @eslint-react/eslint-plugin #60
Conversation
🦋 Changeset detectedLatest commit: 23dd206 The changes in this PR will be included in the next version bump. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 change enables and configures React and React Hooks ESLint plugins, updating the codebase to address all resulting lint issues. It adjusts dependency arrays in React hooks, improves memoization, and refactors several components for better lint compliance. The ESLint configuration and dependencies are updated accordingly, with no changes to public APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant ESLint as ESLint Config
participant Code as Codebase
Dev->>ESLint: Add React and Hooks plugins
Dev->>Code: Update hook dependency arrays and memoization
ESLint->>Code: Lint with new rules
Code-->>Dev: Reports issues
Dev->>Code: Fixes all reported lint issues
Code-->>ESLint: Passes linting
Possibly related PRs
Suggested labels
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 (
|
JounQin
changed the title
commit: |
Signed-off-by: JounQin <admin@1stg.me>
There was a problem hiding this comment.
Pull Request Overview
This PR integrates eslint-plugin-react-hooks and addresses hook dependency issues across the codebase.
- Added and configured
eslint-plugin-react-hookswith recommended rules. - Updated dependency arrays in various React hooks (
useEffect,useMemo,useCallback). - Minor JSX adjustments for list keys and ref forwarding.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added eslint-plugin-react-hooks to dependencies |
| eslint.config.js | Enabled React Hooks lint rules |
| src/runtime/hooks/useSiteOverrides.ts | Added missing dependencies to useEffect |
| src/runtime/components/Tabs.tsx | Refactored Tabs component; addressed ref forwarding |
| src/runtime/components/TermsTable.tsx | List items keyed by index; consider unique keys |
| src/runtime/components/OpenAPIPath.tsx | Removed unused dependency from hook dependency array |
Comments suppressed due to low confidence (3)
src/runtime/components/Tabs.tsx:9
- Using
refas a prop in a functional component does not forward React refs. You should wrap the component inforwardRefor accept a prop likeinnerRefand pass it to the DOM node.
export const Tabs = ({ ref, children, ...props }: TabsProps) => {
src/runtime/components/TermsTable.tsx:21
- [nitpick] Using the array index as a key can cause rendering issues when the list changes. Consider using a unique identifier from
itemor the item value itself as the key.
// eslint-disable-next-line @eslint-react/no-array-index-key
src/runtime/components/OpenAPIPath.tsx:207
- [nitpick] The
page.routePathdependency isn’t used inside this memoized function. Removing it will avoid unnecessary recalculations.
}, [openapiPath_, page.routePath, path])
![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 (1)
src/runtime/components/K8sCrd.tsx (1)
45-46: Consider alternatives to direct state setting in useEffect.While the ESLint disable comment shows awareness of the issue, direct state setting in useEffect can cause unnecessary re-renders. Consider using a ref or restructuring the logic to avoid this pattern.
Alternative approach:
- useEffect(() => { - // eslint-disable-next-line @eslint-react/hooks-extra/no-direct-set-state-in-use-effect - setOpen(expandAll) - }, [expandAll]) + useEffect(() => { + setOpen((prev) => expandAll !== prev ? expandAll : prev) + }, [expandAll])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
.changeset/quick-zoos-allow.md(1 hunks)eslint.config.js(2 hunks)package.json(1 hunks)src/global/SiteOverrides/index.tsx(1 hunks)src/global/VersionsNav/index.tsx(2 hunks)src/runtime/components/ExternalSiteLink.tsx(1 hunks)src/runtime/components/K8sCrd.tsx(5 hunks)src/runtime/components/K8sPermissionTable.tsx(1 hunks)src/runtime/components/Mermaid.tsx(1 hunks)src/runtime/components/OpenAPIPath.tsx(2 hunks)src/runtime/components/OpenAPIRef.tsx(3 hunks)src/runtime/components/Overview.tsx(4 hunks)src/runtime/components/Tabs.tsx(2 hunks)src/runtime/components/TermsTable.tsx(1 hunks)src/runtime/components/_ExternalSiteBase.tsx(1 hunks)src/runtime/components/_HeadingTitle.tsx(2 hunks)src/runtime/hooks/useIsPrint.ts(1 hunks)src/runtime/hooks/useSiteOverrides.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/runtime/components/_ExternalSiteBase.tsx (1)
src/runtime/hooks/useTranslation.ts (1)
useLang(7-7)
src/runtime/components/K8sCrd.tsx (1)
src/runtime/components/Badge.ts (2)
Badge(1-1)Badge(1-1)
src/runtime/components/OpenAPIPath.tsx (2)
src/runtime/components/_X.tsx (1)
X(3-3)src/runtime/components/_Markdown.tsx (1)
Markdown(7-16)
src/runtime/components/OpenAPIRef.tsx (1)
src/runtime/components/_X.tsx (1)
X(3-3)
src/runtime/components/Tabs.tsx (1)
src/runtime/hooks/useIsPrint.ts (1)
useIsPrint(40-44)
src/runtime/components/K8sPermissionTable.tsx (1)
src/plugins/permission/types.ts (1)
FunctionResource(3-8)
🪛 ESLint
src/runtime/components/Tabs.tsx
[error] 2-2: Unable to resolve path to module 'react'.
(import-x/no-unresolved)
src/runtime/components/Overview.tsx
[error] 14-14: Unable to resolve path to module 'react'.
(import-x/no-unresolved)
🔇 Additional comments (36)
package.json (1)
124-124: LGTM! Correctly adds React Hooks ESLint plugin.The addition of
eslint-plugin-react-hooksaligns perfectly with the PR objective to enable React Hooks linting rules.src/runtime/components/K8sCrd.tsx (2)
77-88: Good improvement to JSX spacing consistency.The explicit space fragments ensure consistent visual spacing between elements, improving UI consistency.
184-184: Critical fix: Corrected useMemo dependency array.This is an important correction - the memoized value depends on both
crdPathandname, but the dependency array was empty. This fix prevents stale closures and ensures proper memoization behavior.src/runtime/components/ExternalSiteLink.tsx (1)
34-37: Excellent fix: Corrected missing useMemo dependency.The memoized
sitevalue depends on thenameprop, so including it in the dependency array ensures the memoization works correctly whennamechanges. This prevents stale values and potential bugs.src/runtime/components/TermsTable.tsx (1)
21-22: Verify if a better unique key is available.While using array indices as keys is generally discouraged, it may be acceptable here if the
listis static and order doesn't change. However, consider if theitemstring itself could serve as a unique key, or if there's other identifying information available.Alternative approach if items are unique:
- // eslint-disable-next-line @eslint-react/no-array-index-key - <X.li key={index}>{item}</X.li> + <X.li key={item}>{item}</X.li>src/runtime/hooks/useIsPrint.ts (1)
35-35: LGTM: Correct dependency array fix for exhaustive-deps compliance.The addition of
onChangeto the dependency array is correct since the effect uses this callback. This ensures the event listener is properly updated if theonChangefunction identity changes, aligning with React hooks best practices..changeset/quick-zoos-allow.md (1)
1-6: LGTM: Well-documented changeset.The changeset properly documents the patch update for enabling React hooks ESLint plugins and related fixes.
src/global/SiteOverrides/index.tsx (1)
74-81: LGTM: Comprehensive dependency array fix.The expanded dependency array correctly includes all variables used within the effect (
articleTitle,frontmatter.title,frontmatter.titleSuffix,pageType,siteData,title). This ensures the document title updates properly when any of these values change, which is the intended behavior.src/runtime/hooks/useSiteOverrides.ts (1)
128-128:Details
✅ Verification successful
Verify the behavioral impact of this dependency array change.
While adding
acpSiteandsiteData.baseto the dependency array aligns with exhaustive-deps rules, this changes the effect from running once on mount to re-running whenever these values change. Given the global state management withnormalizedSiteOverridesandpromise, please verify this doesn't cause unintended side effects like multiple concurrent fetches.The early return check
if (normalizedSiteOverrides)should prevent redundant fetches, but please test that the global state behaves correctly when the effect re-runs.
🏁 Script executed:
#!/bin/bash # Description: Check if there are other places that rely on the one-time fetch behavior # Search for usages of normalizedSiteOverrides and promise variables rg -A 3 -B 3 "normalizedSiteOverrides|promise.*=" src/ # Look for other effects or functions that might expect single initialization ast-grep --pattern 'useEffect($_, [])'Length of output: 2826
Effect dependencies update verified
- A search found no other
useEffect(..., [])hooks that would rely on a strictly one-time fetch.- The global
normalizedSiteOverridesandpromiseguard ensures the fetch only ever runs once, even if the effect re-fires.- Adding
[acpSite, siteData.base]to the dependency array does not introduce redundant or concurrent fetches.No further changes required.
src/global/VersionsNav/index.tsx (2)
109-109: Correct dependency array fix for React Hooks ESLint rule.Adding
versionandversionsBaseto the dependency array is correct since both variables are used within the effect logic.
123-123: ESLint disable comment is appropriately used.The disable comment for
@eslint-react/hooks-extra/no-direct-set-state-in-use-effectis justified here since you're performing a direct comparison and conditional state update, which is a valid pattern in this mutation observer context.src/runtime/components/Mermaid.tsx (1)
35-35: Correct dependency array fix for React Hooks ESLint rule.Adding
idto the dependency array is correct since theidvariable is used in the mermaid.render call within the effect (line 25). This ensures the diagram re-renders when the component ID changes.src/runtime/components/_ExternalSiteBase.tsx (2)
97-100: Correct dependency array fix for useMemo.Adding
nameto the dependency array is correct since thenamevariable is used in the find operation to locate the site.
106-106: Correct dependency array fix for useMemo.The dependency array
[lang, name, site?.displayName]correctly includes all variables used in the computation:langfor language selection,namefor the fallback uppercase transformation, andsite?.displayNamefor the primary display name source.src/runtime/components/_HeadingTitle.tsx (3)
24-24: Correct optimization of useMemo dependency array.Removing
Xfrom the dependency array is correct sinceHeadingComponentsis a static array andXis a constant import that never changes.
28-28: Appropriate ESLint disable comment.The disable comment for
@eslint-react/no-children-to-arrayis justified here sinceChildren.toArrayis necessary to extract string content from the children for slug generation.
39-39: Correct dependency array fix for useMemo.Adding
sluggerto the dependency array is correct since it's used in the computation viaslugger?.slug(slugFromChildren)on line 36.src/runtime/components/OpenAPIPath.tsx (2)
56-69: Good fix for React Hooks dependency array, but consider the spacing pattern.The useMemo dependency array fix on line 207 is excellent - it properly includes all dependencies that could change (
openapiPath_,page.routePath,path), preventing stale closures.The explicit fragment wrapping for spacing around
<Badge>and<Markdown>components is functional but creates verbose JSX. Consider if CSS margins would be cleaner.
207-207: Critical React Hooks dependency fix applied correctly.This change fixes a significant React Hooks issue where the
useMemohook was missing dependencies, which could lead to stale closures and incorrect memoization behavior.eslint.config.js (3)
4-4: ESLint plugin imports are correct.Proper imports for both React and React Hooks ESLint plugins.
Also applies to: 7-7
17-18: Good choice of React ESLint configurations.Using
react.configs.recommendedandreactHooks.configs['recommended-latest']follows current best practices. The "recommended-latest" config for React Hooks includes the most up-to-date rules.
21-25: Proper TypeScript-specific React configuration.Adding
react.configs['recommended-type-checked']for TypeScript files provides enhanced type-aware React linting, which is valuable for catching React-specific TypeScript issues.src/runtime/components/OpenAPIRef.tsx (3)
108-109: Excellent improvement to React key usage.Changing from array index to property name as the React key is a significant improvement. Using property names provides stable, unique keys that won't cause React reconciliation issues when the order changes.
175-175: Critical useMemo dependency array fix.This fixes a serious React Hooks issue where the memoization was missing dependencies, potentially causing stale closures and incorrect behavior when
openapiPath_orschemachanged.
178-178: Smart defensive programming.Adding the
openapicheck prevents the refs collection from running when the OpenAPI document isn't available, avoiding potential runtime errors.src/runtime/components/Tabs.tsx (3)
6-7: Clean TypeScript typing for the refactored component.The
TabsPropstype alias properly combines the original component props withRefAttributes<HTMLDivElement>, maintaining type safety after removingforwardRef.
9-9: Valid forwardRef to direct ref refactoring.The change from
forwardRefto direct ref prop handling is correct and simpler. The ref is properly passed to the wrappingdivelement.Also applies to: 12-12
14-17: Appropriate ESLint disables for print mode rendering.The ESLint disables are justified here:
@eslint-react/no-children-map: UsingChildren.mapis necessary for the print mode transformation@eslint-react/no-array-index-key: Using array index as key is acceptable in print mode where the list is static and won't be reorderedsrc/runtime/components/Overview.tsx (6)
32-42: Excellent extraction of pure function.Moving
getChildLinkoutside the component scope prevents unnecessary recreations on each render and improves performance. The function is pure and doesn't depend on any component state or props.
60-67: Correct memoization of filter function.The
useCallbackwrapper with[routePath]dependency is appropriate sincesubFilterdepends onroutePathfor its closure behavior.
71-74: Proper memoization of computed array.The
useMemowith dependencies[pages, subFilter]correctly identifies the values that affect the computation. This prevents unnecessary re-filtering when other component state changes.
89-126: Well-structured memoization with comprehensive dependencies.The
useCallbackwrapper correctly identifies all dependencies:overviewModules,props.overviewHeaders, androutePath. The function signature and logic remain unchanged while optimizing for re-renders.
133-184: Appropriate callback memoization for complex function.The dependency array
[frontmatter, normalizeSidebarItem, subFilter]correctly captures all the values used within the function. This memoization will prevent unnecessary re-computations when other component dependencies change.
186-201: Effective memoization of expensive computation.The
useMemowith[getGroup, overviewSidebarGroups]dependencies is well-placed for this complex computation that involves multiple iterations and filtering operations.src/runtime/components/K8sPermissionTable.tsx (2)
87-96: Excellent performance optimization by extracting expensive computation.Moving the
allFunctionResourcescomputation to module level prevents this expensive reduce operation from running on every component render. The logic remains identical while significantly improving performance.
99-111: Proper memoization with justified ESLint disable.The
useMemocorrectly depends only onfunctionssinceallFunctionResourcesis now a module-level constant. The ESLint disable comment forexhaustive-depsis justified because includingallFunctionResourcesin the dependency array would be unnecessary and could cause ESLint confusion since it's a constant.
Summary by CodeRabbit
Chores
Refactor
Style
Bug Fixes