diff --git a/.agents/review-checklists/react/code-quality.md b/.agents/review-checklists/react/code-quality.md index 82aea80b49..2c3533cac2 100644 --- a/.agents/review-checklists/react/code-quality.md +++ b/.agents/review-checklists/react/code-quality.md @@ -43,6 +43,184 @@ const MyComponent: FC = ({ report, tab }) => { }; ``` +## React components and hooks should have unit tests + +**Urgency:** suggestion + +### Category + +Maintainability + +### Confidence Threshold + +Flag new or significantly modified components/hooks lacking Jest tests when they contain logic (state, effects, event handlers, conditional rendering). Simple presentational components rendering static JSX may defer testing. Existing untested code not modified in the PR should not be flagged. + +### Exceptions / False Positives + +- Do not flag demo-only, internal layout wrappers, or placeholder components. +- Do not flag when tests exist in a different location (e.g., shared test suite) if verifiable. +- Do not flag existing untested code outside the PR scope. + +### Description + +Components with state, effects, event handlers, or conditional logic need Jest tests. Test what should always be covered: `useState`/`useReducer`, event handlers, conditional rendering, custom hooks with logic, permission-gated rendering. Layout-only components are optional. + +### Anti-patterns to Flag + +```tsx +// ❌ Stateful component with events but no tests +const Counter: FC = () => { + const [count, setCount] = useState(0); + return ( +
+

Count: {count}

+ +
+ ); +}; + +// ❌ Custom hook with async logic but no tests +const useCustomFetch = (url: string) => { + const [data, setData] = useState(null); + useEffect(() => { + fetch(url).then(res => res.json()).then(setData); + }, [url]); + return data; +}; +``` + +### Suggested Fix + +Create `Component.test.tsx` with meaningful assertions per [`jest/business-logic.md`](../jest/business-logic.md): + +```tsx +test('increments count when button clicked', async () => { + render(); + await userEvent.click(screen.getByRole('button', { name: /increment/i })); + expect(screen.getByText('Count: 1')).toBeInTheDocument(); +}); +``` + +### How to Detect + +1. Check for `.test.tsx`/`.test.ts` file corresponding to modified component. +2. If missing, flag if component has: hooks, event handlers, or conditional logic. +3. If tests exist, verify they assert on behavior (not just render existence). + +## Large JSX/TSX blocks should be extracted into separate components + +**Urgency:** suggestion + +### Category + +Maintainability + +### Confidence Threshold + +Flag JSX return statements >50 lines or those with multiple conditional branches (3+ `{condition && <...>}` or ternary operators). Simple single-element returns or small layout wrappers are exempt. + +### Exceptions / False Positives + +- Do not flag simple, single-element returns or small layout wrappers. +- Do not flag when extraction would require excessive prop-drilling that makes code less readable. + +### Description + +Large JSX blocks reduce readability, increase cognitive load, and make testing harder. Red flags: multiple conditional rendering branches, repeated patterns (like tab buttons with similar structure), deeply nested element hierarchies, and components managing multiple independent concerns. + +### Anti-patterns to Flag + +```tsx +// ❌ Return block with multiple conditional branches and repeated patterns +const DesignerPanel: FC = ({ data, showWarnings }) => { + const [rightTab, setRightTab] = useState<'properties' | 'warnings'>('properties'); + + return ( +
+ {showWarnings && ( +
+ + +
+ )} + {(!showWarnings || rightTab === 'properties') && ( +
+ +
+ )} + {showWarnings && rightTab === 'warnings' && ( +
+ +
+ )} +
+ ); +}; +``` + +**Red flags in this example:** +- 3 separate conditional render blocks +- Repeated tab button structure with duplicated accessibility attrs +- Multiple independent concerns (tabs, properties, warnings) +- Mixed conditional logic (both &&-chaining and ternaries) + +### Suggested Fix + +Extract the tab control into a reusable component: + +```tsx +interface TabPanelProps { + active: 'properties' | 'warnings'; + onChange: (tab: 'properties' | 'warnings') => void; +} + +const TabPanel: FC = ({ active, onChange }) => ( +
+ {['properties', 'warnings'].map(tab => ( + + ))} +
+); + +const DesignerPanel: FC = ({ data, showWarnings }) => { + const [rightTab, setRightTab] = useState<'properties' | 'warnings'>('properties'); + + return ( +
+ {showWarnings && } + {(!showWarnings || rightTab === 'properties') && } + {showWarnings && rightTab === 'warnings' && } +
+ ); +}; +``` + +### How to Detect + +1. **Multiple conditional branches:** Count `{condition && <...>}` and ternary operators. 3+ suggests extraction. +2. **Repeated patterns:** Similar button/input groups with duplicated props (e.g., `role="tab"`, `aria-selected`, `onClick`) → extract into component. +3. **Multiple state sections:** Component with separate state for tabs/panels (e.g., `const [tab, setTab]` and `const [panel, setPanel]`) often signals multiple concerns. +4. **Line count + nesting:** Return >50 lines OR deeply nested (3+ levels) conditional JSX → consider extraction. + ## Optional props that are always passed should be required **Urgency:** suggestion