-
Notifications
You must be signed in to change notification settings - Fork 114
chore: add React code review skill for agent-based guidance #687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| --- | ||
| name: review-react | ||
| description: > | ||
| React code review guidelines covering Rules of React, re-render optimization, | ||
| rendering performance, and advanced patterns. Activates when writing, reviewing, | ||
| or refactoring React components, hooks, or state management code. | ||
| --- | ||
|
|
||
| # React Code Review Guidelines | ||
|
|
||
| Performance optimization and correctness guide for React applications. Contains 23 rules across 4 categories, prioritized by impact. | ||
|
|
||
| ## When to Apply | ||
|
|
||
| Reference these guidelines when: | ||
| - Writing or reviewing React components and hooks | ||
| - Optimizing re-render performance | ||
| - Refactoring state management or effect logic | ||
| - Reviewing pull requests that touch React code | ||
|
|
||
| ## Rule Categories by Priority | ||
|
|
||
| | Priority | Category | Impact | Prefix | Rules | | ||
| |----------|----------|--------|--------|-------| | ||
| | 1 | Rules of React | CRITICAL | `react-rules-` | 3 | | ||
| | 2 | Re-render Optimization | MEDIUM | `rerender-` | 13 | | ||
| | 3 | Rendering Performance | MEDIUM | `rendering-` | 5 | | ||
| | 4 | Advanced Patterns | LOW | `advanced-` | 2 | | ||
|
|
||
| ## Quick Reference | ||
|
|
||
| ### 1. Rules of React (CRITICAL) | ||
|
|
||
| - `react-rules-purity` - Components and Hooks must be pure; no side effects during render | ||
| - `react-rules-hooks` - Only call Hooks at the top level and from React functions | ||
| - `react-rules-calling` - Never call components as functions or pass Hooks as values | ||
|
|
||
| ### 2. Re-render Optimization (MEDIUM) | ||
|
|
||
| - `rerender-no-inline-components` - Never define components inside other components | ||
| - `rerender-derived-state-no-effect` - Derive state during render, not in effects | ||
| - `rerender-memo` - Extract memoized child components to avoid re-renders | ||
| - `rerender-memo-with-default-value` - Hoist default non-primitive props outside memo | ||
| - `rerender-simple-expression-in-memo` - Don't useMemo for simple primitive expressions | ||
| - `rerender-defer-reads` - Don't subscribe to state only used in callbacks | ||
| - `rerender-dependencies` - Use primitive values in effect dependencies | ||
| - `rerender-derived-state` - Subscribe to derived booleans, not raw objects | ||
| - `rerender-functional-setstate` - Use functional setState for stable callbacks | ||
| - `rerender-lazy-state-init` - Pass initializer function to useState for expensive values | ||
| - `rerender-move-effect-to-event` - Move interaction logic from effects to event handlers | ||
| - `rerender-transitions` - Use startTransition for non-urgent state updates | ||
| - `rerender-use-ref-transient-values` - Use refs for frequently-changing transient values | ||
|
|
||
| ### 3. Rendering Performance (MEDIUM) | ||
|
|
||
| - `rendering-hoist-jsx` - Hoist static JSX outside component functions | ||
| - `rendering-conditional-render` - Use ternary operator instead of && for conditional rendering | ||
| - `rendering-usetransition-loading` - Prefer useTransition over manual loading state | ||
| - `rendering-content-visibility` - Use CSS content-visibility: auto for long lists | ||
| - `rendering-activity` - Use Activity component for preserving hidden UI state | ||
|
|
||
| ### 4. Advanced Patterns (LOW) | ||
|
|
||
| - `advanced-event-handler-refs` - Store latest event handlers in refs for stable callbacks | ||
| - `advanced-init-once` - Initialize app-level singletons once, not per mount | ||
|
|
||
| ## How to Use | ||
|
|
||
| Read individual rule files for detailed explanations and code examples: | ||
|
|
||
| ``` | ||
| rules/react-rules-purity.md | ||
| rules/rerender-no-inline-components.md | ||
| ``` | ||
|
|
||
| Each rule file contains: | ||
| - Brief explanation of why it matters | ||
| - Incorrect code example | ||
| - Correct code example | ||
| - Reference links |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # Sections | ||
|
|
||
| This file defines all sections, their ordering, impact levels, and descriptions. | ||
| The section ID (in parentheses) is the filename prefix used to group rules. | ||
|
|
||
| --- | ||
|
|
||
| ## 1. Rules of React (react-rules) | ||
|
|
||
| **Impact:** CRITICAL | ||
| **Description:** Fundamental rules from react.dev that ensure correctness. Components must be pure, Hooks must follow call rules, and components must not be called as functions. | ||
|
|
||
| ## 2. Re-render Optimization (rerender) | ||
|
|
||
| **Impact:** MEDIUM | ||
| **Description:** Patterns to minimize unnecessary re-renders: proper memoization, derived state, functional setState, and effect dependency management. | ||
|
|
||
| ## 3. Rendering Performance (rendering) | ||
|
|
||
| **Impact:** MEDIUM | ||
| **Description:** Techniques to optimize what and how React renders: hoisting static JSX, conditional rendering patterns, content-visibility, and transitions. | ||
|
|
||
| ## 4. Advanced Patterns (advanced) | ||
|
|
||
| **Impact:** LOW | ||
| **Description:** Specialized techniques for edge cases: storing handlers in refs for stable callbacks and one-time initialization patterns. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| --- | ||
| title: Rule Title Here | ||
| impact: MEDIUM | ||
| impactDescription: Optional description of impact (e.g., "20-50% improvement") | ||
| tags: tag1, tag2 | ||
| --- | ||
|
|
||
| ## Rule Title Here | ||
|
|
||
| **Impact: MEDIUM (optional impact description)** | ||
|
|
||
| Brief explanation of the rule and why it matters. This should be clear and concise, explaining the performance implications. | ||
|
|
||
| **Incorrect (description of what's wrong):** | ||
|
|
||
| ```typescript | ||
| // Bad code example here | ||
| const bad = example() | ||
| ``` | ||
|
|
||
| **Correct (description of what's right):** | ||
|
|
||
| ```typescript | ||
| // Good code example here | ||
| const good = example() | ||
| ``` | ||
|
|
||
| Reference: [Link to documentation or resource](https://example.com) |
49 changes: 49 additions & 0 deletions
49
.agents/skills/review-react/rules/advanced-event-handler-refs.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| --- | ||
| title: Store Event Handlers in Refs for Stable Callbacks | ||
| impact: LOW | ||
| impactDescription: provides stable function identity without stale closures | ||
| tags: advanced, refs, event-handlers, useRef, stable-callback | ||
| --- | ||
|
|
||
| ## Store Event Handlers in Refs for Stable Callbacks | ||
|
|
||
| **Impact: LOW (provides stable function identity without stale closures)** | ||
|
|
||
| When you need a callback that always calls the latest version of a function without changing identity, store the handler in a ref. This avoids both stale closures and unnecessary re-renders from changing callback props. | ||
|
|
||
| **Incorrect (callback changes identity, causing child re-renders):** | ||
|
|
||
| ```tsx | ||
| function Chat({ roomId }) { | ||
| const [message, setMessage] = useState('') | ||
|
|
||
| const handleSend = useCallback(() => { | ||
| sendMessage(roomId, message) | ||
| }, [roomId, message]) // Changes on every keystroke | ||
|
|
||
| return <SendButton onClick={handleSend} /> | ||
| } | ||
| ``` | ||
|
|
||
| **Correct (ref-based stable callback):** | ||
|
|
||
| ```tsx | ||
| function Chat({ roomId }) { | ||
| const [message, setMessage] = useState('') | ||
|
|
||
| const handleSendRef = useRef(() => {}) | ||
| handleSendRef.current = () => { | ||
| sendMessage(roomId, message) | ||
| } | ||
|
|
||
| const handleSend = useCallback(() => { | ||
| handleSendRef.current() | ||
| }, []) | ||
|
|
||
| return <SendButton onClick={handleSend} /> | ||
| } | ||
| ``` | ||
|
|
||
| This is the pattern behind `useEffectEvent` (experimental). When `useEffectEvent` stabilizes, prefer it over manual ref management. | ||
|
|
||
| Reference: [Separating Events from Effects](https://react.dev/learn/separating-events-from-effects) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| --- | ||
| title: Initialize App-Level Singletons Once | ||
| impact: LOW | ||
| impactDescription: prevents duplicate initialization in Strict Mode and re-mounts | ||
| tags: advanced, initialization, singleton, module-scope | ||
| --- | ||
|
|
||
| ## Initialize App-Level Singletons Once | ||
|
|
||
| **Impact: LOW (prevents duplicate initialization in Strict Mode and re-mounts)** | ||
|
|
||
| App-level setup (analytics, error tracking, SDK initialization) should run once per app load, not per component mount. Strict Mode double-invokes effects, causing duplicate initialization. | ||
|
|
||
| **Incorrect (initialization in effect runs twice in Strict Mode):** | ||
|
|
||
| ```tsx | ||
| function App() { | ||
| useEffect(() => { | ||
| analytics.init('key') // Runs twice in Strict Mode | ||
| errorTracker.setup() // May cause duplicate event listeners | ||
| }, []) | ||
|
|
||
| return <Router /> | ||
| } | ||
| ``` | ||
|
|
||
| **Correct (module-level initialization):** | ||
|
|
||
| ```tsx | ||
| // app-init.ts | ||
| let initialized = false | ||
|
|
||
| export function initApp() { | ||
| if (initialized) return | ||
| initialized = true | ||
| analytics.init('key') | ||
| errorTracker.setup() | ||
| } | ||
| ``` | ||
|
|
||
| ```tsx | ||
| // App.tsx | ||
| import { initApp } from './app-init' | ||
|
|
||
| initApp() // Runs once at module evaluation | ||
|
|
||
| function App() { | ||
| return <Router /> | ||
| } | ||
| ``` | ||
|
|
||
| **Also correct (top-level flag guard):** | ||
|
|
||
| ```tsx | ||
| let didInit = false | ||
|
|
||
| function App() { | ||
| useEffect(() => { | ||
| if (didInit) return | ||
| didInit = true | ||
| analytics.init('key') | ||
| }, []) | ||
|
|
||
| return <Router /> | ||
| } | ||
| ``` | ||
|
|
||
| Reference: [How to handle the Effect firing twice in development](https://react.dev/learn/synchronizing-with-effects#how-to-handle-the-effect-firing-twice-in-development) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| --- | ||
| title: React Calls Components and Hooks | ||
| impact: CRITICAL | ||
| impactDescription: breaks React's rendering model and optimization | ||
| tags: react-rules, components, hooks, calling-convention | ||
| --- | ||
|
|
||
| ## React Calls Components and Hooks | ||
|
|
||
| **Impact: CRITICAL (breaks React's rendering model and optimization)** | ||
|
|
||
| React must control when components render and hooks execute. Calling them directly bypasses reconciliation, state management, and optimization. | ||
|
|
||
| ### Rule 1: Never call component functions directly | ||
|
|
||
| **Incorrect (calling component as function):** | ||
|
|
||
| ```tsx | ||
| function Parent() { | ||
| // This bypasses React's rendering, no proper lifecycle or state isolation | ||
| return <div>{Profile({ name: 'Alice' })}</div> | ||
| } | ||
| ``` | ||
|
|
||
| **Correct (use JSX):** | ||
|
|
||
| ```tsx | ||
| function Parent() { | ||
| return <div><Profile name="Alice" /></div> | ||
| } | ||
| ``` | ||
|
|
||
| Calling a component as a function makes React treat it as part of the parent's render. This means: | ||
| - No separate fiber node for reconciliation | ||
| - State and effects are tied to the parent | ||
| - Keys and refs don't work as expected | ||
|
|
||
| ### Rule 2: Never pass Hooks as regular values | ||
|
|
||
| **Incorrect (passing hook as prop):** | ||
|
|
||
| ```tsx | ||
| function ChatRoom({ useStatus }) { | ||
| const status = useStatus() // Hook passed as value | ||
| return <p>{status}</p> | ||
| } | ||
|
|
||
| <ChatRoom useStatus={useOnlineStatus} /> | ||
| ``` | ||
|
|
||
| **Correct (call hook directly, pass result as prop):** | ||
|
|
||
| ```tsx | ||
| function ChatRoom({ status }) { | ||
| return <p>{status}</p> | ||
| } | ||
|
|
||
| function ChatRoomWrapper() { | ||
| const status = useOnlineStatus() | ||
| return <ChatRoom status={status} /> | ||
| } | ||
| ``` | ||
|
|
||
| Passing hooks as values makes them opaque to React's static analysis, breaks the Rules of Hooks, and prevents the compiler from optimizing correctly. | ||
|
|
||
| Reference: [React calls Components and Hooks](https://react.dev/reference/rules/react-calls-components-and-hooks) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.