Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 178 additions & 0 deletions .agents/review-checklists/react/code-quality.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,184 @@ 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 useCustomFetch = (url: string) => {
const [data, setData] = useState(null);
useEffect(() => {
fetch(url).then(res => res.json()).then(setData);
}, [url]);
return data;
};
Comment on lines +82 to +89
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude is flagging this as bad code example because it's missing error handling and cleanup, which could encourage an LLM to use this. Here's the recommended code.

// ❌ Custom hook with async logic but no tests
  const useCustomFetch = <T,>(url: string): T | null => {
      const [data, setData] = useState<T | null>(null);
      useEffect(() => {
          const controller = new AbortController();
          fetch(url, { signal: controller.signal })
              .then(res => res.json())
              .then(setData)
              .catch(() => { /* ignore aborts/errors for brevity */ });
          return () => controller.abort();
      }, [url]);
      return data;
  };

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. It IS a bad code example, though its intent is to be bad in a different way (lacking tests, not bad error handling).

Do we need our examples of bad code to be good in ways other than the specific problem they're intending to call out? The downside is overly verbose examples and token bloat.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how useful these code examples are because we cannot use fetch anywhere, and as a result cannot use AbortControllers. We'd need to release a 2.x version of @labkey/api for this to be relevant to us. This is something we should do, and I was just talking with Nick about it the other day because I want to use AbortControllers, but there isn't really a path forward for us to use either because all of our API requests are done via Ajax.request

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a low priority call out by claude, but I suspect what you might get is an LLM adds tests to the bad example and thinks it is now good code to use as a reference. Even though it has other issues. Maybe something to research further.

```

### 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 >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<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 }) => (
<div className="tabs" role="tablist">
{['properties', 'warnings'].map(tab => (
<button
key={tab}
role="tab"
aria-selected={active === tab}
onClick={() => onChange(tab as 'properties' | 'warnings')}
>
{tab.charAt(0).toUpperCase() + tab.slice(1)}
</button>
))}
</div>
);

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>
);
};
```

### 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
Expand Down
Loading