Add React guidance for unit tests, JSX structure, and memoization

.agents/review-checklists/react/code-quality.md

@@ -43,6 +43,269 @@ const MyComponent: FC<MyComponentProps> = ({ 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 (
+        <div>
+            <p>Count: {count}</p>
+            <button onClick={() => setCount(count + 1)}>Increment</button>
+        </div>
+    );
+};
+
+// ❌ Custom hook with async logic but no tests
+const useItemData = (id: string) => {
+    const [data, setData] = useState(null);
+    useEffect(() => {
+        // Error handling omitted for brevity
+        async function load() {
+            const result = await loadItemById(id);
+            setData(result);
+        }
+        load();
+    }, [id]);
+    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(<Counter />);
+    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 >25 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<Props> = ({ data, showWarnings }) => {
+    const [rightTab, setRightTab] = useState<'properties' | 'warnings'>('properties');
+
+    return (
+        <div className="designer">
+            {showWarnings && (
+                <div className="tabs" role="tablist">
+                    <button
+                        role="tab"
+                        aria-selected={rightTab === 'properties'}
+                        onClick={() => setRightTab('properties')}
+                    >
+                        Properties
+                    </button>
+                    <button
+                        role="tab"
+                        aria-selected={rightTab === 'warnings'}
+                        onClick={() => setRightTab('warnings')}
+                    >
+                        Warnings
+                    </button>
+                </div>
+            )}
+            {(!showWarnings || rightTab === 'properties') && (
+                <div role="tabpanel">
+                    <PropertiesPanel data={data} />
+                </div>
+            )}
+            {showWarnings && rightTab === 'warnings' && (
+                <div role="tabpanel">
+                    <WarningsPanel data={data} />
+                </div>
+            )}
+        </div>
+    );
+};
+```
+
+**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<TabPanelProps> = ({ active, onChange }) => {
+    const handlePropertiesClick = useCallback(() => onChange('properties'), [onChange]);
+    const handleWarningsClick = useCallback(() => onChange('warnings'), [onChange]);
+
+    return (
+        <div className="tabs" role="tablist">
+            <button
+                role="tab"
+                aria-selected={active === 'properties'}
+                onClick={handlePropertiesClick}
+            >
+                Properties
+            </button>
+            <button
+                role="tab"
+                aria-selected={active === 'warnings'}
+                onClick={handleWarningsClick}
+            >
+                Warnings
+            </button>
+        </div>
+    );
+};
+TabPanel.displayName = 'TabPanel';
+
+const DesignerPanel: FC<Props> = ({ data, showWarnings }) => {
+    const [rightTab, setRightTab] = useState<'properties' | 'warnings'>('properties');
+
+    return (
+        <div className="designer">
+            {showWarnings && <TabPanel active={rightTab} onChange={setRightTab} />}
+            {(!showWarnings || rightTab === 'properties') && <PropertiesPanel data={data} />}
+            {showWarnings && rightTab === 'warnings' && <WarningsPanel data={data} />}
+        </div>
+    );
+};
+DesignerPanel.displayName = 'DesignerPanel';
+```
+
+### 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 >25 lines OR deeply nested (3+ levels) conditional JSX → consider extraction.
+
+## 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 (
+        <div>
+            <p>Count: {count}</p>
+            <button onClick={() => setCount(count + 1)}>Increment</button>
+        </div>
+    );
+};
+
+// ❌ Custom hook with async logic but no tests
+const useItemData = (id: string) => {
+    const [data, setData] = useState(null);
+    useEffect(() => {
+        // Error handling omitted for brevity
+        async function load() {
+            const result = await loadItemById(id);
+            setData(result);
+        }
+        load();
+    }, [id]);
+    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(<Counter />);
+    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).
+
 ## Optional props that are always passed should be required
 
 **Urgency:** suggestion

.agents/review-checklists/react/performance.md

@@ -2,6 +2,56 @@
 
 > **Prerequisite:** Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.
 
+## Event handlers should be memoized with `useCallback`
+
+**Urgency:** urgent
+
+### Category
+
+Performance
+
+### Confidence Threshold
+
+Flag any event handler prop (`onClick`, `onChange`, `onSubmit`, etc.) that is an inline arrow function or a named function in the component body without `useCallback`. Do not flag handlers defined at module scope — they are already stable references.
+
+### Exceptions / False Positives
+
+- Do not flag event handlers defined at module scope.
+- Do not suggest `useCallback` for handlers with no component-scope dependencies — hoisting to module scope is preferable (a true constant reference with no Hook overhead).
+
+### Description
+
+All event handlers in a component body should be wrapped with `useCallback`, including those on native DOM elements. The overhead is negligible; omitting it has caused real performance issues and creates audit work whenever a future refactor forwards the handler to a memoized child.
+
+Watch for the factory anti-pattern: `useCallback` wrapping a function that itself returns a function. The outer reference is stable, but invoking it still produces a new function reference on every render.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Inline arrow — new reference every render
+<button onClick={() => handleDelete(item.id)}>Delete</button>
+
+// ❌ Factory pattern — getHandler(id) returns a new fn each render despite useCallback
+const getHandler = useCallback((id: string) => () => handleClick(id), [handleClick]);
+<Item onClick={getHandler(item.id)} />
+```
+
+### Suggested Fix
+
+```tsx
+const handleDelete = useCallback(() => {
+    doDelete(item.id);
+}, [item.id]);
+<button onClick={handleDelete}>Delete</button>
+```
+
+### How to Detect
+
+1. Search for `onClick={`, `onChange={`, etc. where the value is an inline arrow or an identifier not assigned via `useCallback(...)`.
+2. For existing `useCallback` calls, check if the wrapped function returns another function — if so, verify that returned function is not being passed directly as a prop.
+
+---
+
 ## Inline object/array literals in JSX props
 
 **Urgency:** urgent

CLAUDE.md

@@ -83,6 +83,13 @@ We keep local copies of the `@labkey` packages in the `clientAPIs/` directory. Y
 
 The local copies of the packages may contain changes related to the current branches in any of the modules that have an NPM build. For example there may be changes to the `@labkey/components` package that affect the package in the `server/modules/platform/core` module. These packages are not required to be present to build the project, so they may not be available. If they are not present, you can assume that there are no changes in those packages relevant to the current branch. If you suspect an issue is with one of the packages, but it is not present you may prompt the user to check out a local copy of the relevant package.
 
+### React Conventions
+- We are using React 18
+- **Component typing:** Use `React.FC<Props>` (or `React.FC` for no-prop components) for proper components used as `<Component />`. Use `: React.ReactNode` as the return type for render helpers — functions that return JSX but are not used as components (typically named with a lowercase letter or a `render` prefix). Do **not** annotate with `: JSX.Element` or `: React.ReactElement`.
+- **displayName:** Set `ComponentName.displayName = 'ComponentName'` immediately after each `React.FC` definition so it appears correctly in React DevTools and error boundaries.
+- **Props interfaces:** Declare a separate named `interface ComponentNameProps` for any component with two or more props. Do not inline the type in the `FC<>` generic.
+- **Event handlers:** Wrap all event handlers defined in a component body with `useCallback`, including those passed to native DOM elements. Do not use inline arrow functions in JSX (e.g., `onClick={() => doX()}`); extract and memoize them instead. Be aware that `useCallback` on a factory function (one that itself returns a new function) does not memoize the result — each invocation still produces a new reference.
+
 ### Distributions
 
 The `distributions/` directory defines 60+ distribution configurations that select which modules to package. Distributions inherit from each other (most inherit from `:distributions:base`). Distribution directory names must not collide with module names.