From 93d3c09671a46e892efbd237910e198542d4b808 Mon Sep 17 00:00:00 2001
From: Marty Pradere <martyp@labkey.com>
Date: Mon, 23 Feb 2026 05:27:16 -0800
Subject: [PATCH] frontend skills

---
 .agents/review-checklists/common.md           |  20 +
 .../review-checklists/jest/business-logic.md  |  58 +++
 .../review-checklists/jest/code-quality.md    | 208 ++++++++
 .agents/review-checklists/jest/performance.md | 105 ++++
 .../review-checklists/react/business-logic.md | 248 ++++++++++
 .../review-checklists/react/code-quality.md   | 462 ++++++++++++++++++
 .../review-checklists/react/performance.md    | 144 ++++++
 .claude/skills/code-review-jest/skill.md      | 123 +++++
 .claude/skills/code-review-react/skill.md     | 123 +++++
 9 files changed, 1491 insertions(+)
 create mode 100644 .agents/review-checklists/common.md
 create mode 100644 .agents/review-checklists/jest/business-logic.md
 create mode 100644 .agents/review-checklists/jest/code-quality.md
 create mode 100644 .agents/review-checklists/jest/performance.md
 create mode 100644 .agents/review-checklists/react/business-logic.md
 create mode 100644 .agents/review-checklists/react/code-quality.md
 create mode 100644 .agents/review-checklists/react/performance.md
 create mode 100644 .claude/skills/code-review-jest/skill.md
 create mode 100644 .claude/skills/code-review-react/skill.md

diff --git a/.agents/review-checklists/common.md b/.agents/review-checklists/common.md
new file mode 100644
index 0000000000..abb6b84237
--- /dev/null
+++ b/.agents/review-checklists/common.md
@@ -0,0 +1,20 @@
+# Common Review Guidelines
+
+## Reviewer Priority
+
+Agents must prioritize findings in this order and report them in this order when multiple issues are present:
+
+1. **Correctness** (behavioral bugs, warnings, broken contracts, stale state, invalid keys)
+2. **Maintainability** (dead code, duplication, unnecessary complexity, high-review-cost risks)
+3. **Style** (formatting/convention consistency that does not change behavior)
+
+Do not prioritize Style-only findings ahead of unresolved Correctness or Maintainability findings.
+
+## Standard Review Format
+
+For every rule below, agents should provide:
+
+1. **Evidence**: exact code snippet or file/line reference from the changed code
+2. **Category**: the rule's listed primary category (Correctness, Maintainability, or Style)
+3. **Confidence**: apply the rule's confidence threshold before escalating severity in review feedback
+4. **Exceptions**: check the rule's false-positive section before flagging
\ No newline at end of file
diff --git a/.agents/review-checklists/jest/business-logic.md b/.agents/review-checklists/jest/business-logic.md
new file mode 100644
index 0000000000..0759dabf17
--- /dev/null
+++ b/.agents/review-checklists/jest/business-logic.md
@@ -0,0 +1,58 @@
+# Business Logic
+
+> **Prerequisite:** Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.
+
+## Jest tests must assert on meaningful outcomes
+
+**Urgency:** urgent
+
+### Category
+
+Correctness
+
+### Confidence Threshold
+
+Flag as a high-severity finding when the test would still pass after removing the feature logic, or when assertions only validate setup/static existence. If the test is intentionally a smoke/no-crash render test, require that intent to be explicit in the test name.
+
+### Exceptions / False Positives
+
+- Allow explicit smoke tests when the purpose is only to verify the component/function does not throw on render or initialization.
+- Do not flag helper tests that intentionally validate test utilities/builders rather than product behavior, if the subject under test is the utility itself.
+- A simple existence assertion can be acceptable when the behavior under review is conditional presence/absence itself (for example, permission-gated rendering).
+
+### Detection heuristic
+
+Look for tests where all assertions target mock setup data, static strings, or DOM existence rather than computed/rendered output.
+
+## Rules
+
+1. **Assert on component output, not existence.** After `render()`, assert on visible text, element states, or DOM changes that result from the props/state you set up — never just `expect(container).toBeDefined()` or `expect(document.querySelector('.x')).not.toBeNull()`.
+
+2. **Assert on computed results, not test inputs.** If you pass `mockData` into a component, don't assert that `mockData` has the values you just wrote. Assert on what the component *did* with that data.
+
+3. **Every test must fail if the feature is removed.** Apply this litmus test: if you deleted the implementation code for the feature under test, would the test still pass? If yes, the test is worthless — rewrite it.
+
+4. **Interact before asserting (when applicable).** If testing behavior triggered by user action (click, submit, input), simulate that action with `fireEvent` or `userEvent`, then assert on the resulting DOM or state change.
+
+## Examples
+
+```js
+// ❌ BAD — asserts on render existence
+render(<MyForm valid={false} />);
+expect(document.querySelector('.form-container')).not.toBeNull();
+
+// ✅ GOOD — asserts on behavioral outcome of props
+render(<MyForm valid={false} />);
+expect(screen.getByRole('button', { name: /submit/i })).toBeDisabled();
+
+// ❌ BAD — asserts on mock input
+const data = [{ name: 'Alpha', value: 10 }];
+render(<Table rows={data} />);
+expect(data[0].name).toBe('Alpha'); // This tests your test, not your code
+
+// ✅ GOOD — asserts on rendered output from that input
+const data = [{ name: 'Alpha', value: 10 }];
+render(<Table rows={data} />);
+expect(screen.getByText('Alpha')).toBeInTheDocument();
+expect(screen.getByText('10')).toBeInTheDocument();
+```
diff --git a/.agents/review-checklists/jest/code-quality.md b/.agents/review-checklists/jest/code-quality.md
new file mode 100644
index 0000000000..bd6bee4574
--- /dev/null
+++ b/.agents/review-checklists/jest/code-quality.md
@@ -0,0 +1,208 @@
+# Code Quality
+
+> **Prerequisite:** Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.
+
+## Arrange / Act / Assert (AAA) structure
+
+**Urgency:** suggestion
+
+### Category
+
+Style
+
+### Confidence Threshold
+
+Flag only when the test is long enough or complex enough that structure affects readability (for example, multi-step setup/interactions or >10 lines). For short, self-evident tests, prefer a suggestion or no comment.
+
+### Exceptions / False Positives
+
+- Do not flag short tests (roughly 3-5 lines) where Arrange/Act/Assert is obvious without comments.
+- Do not require AAA comments in parameterized tests when added comments make the test harder to scan than the code itself.
+- Prefer suggestions over high-severity findings unless the repository explicitly enforces AAA comment structure in tests.
+
+### Rules
+
+1. **Each section must be preceded by a comment** — `// Arrange`, `// Act`, and `// Assert`.
+2. **The Assert comment must include a brief explanation** of how the described scenario is being verified. The explanation should summarize what the assertions prove about the behavior stated in the test name.
+   - Good: `// Assert - textarea shows hash subjects, participantId query param is ignored`
+   - Good: `// Assert - ID Search button is active`
+   - Bad: `// Assert` (missing explanation)
+   - Bad: `// Assert - check results` (too vague; does not connect to the scenario)
+3. **Arrange may be omitted** when there is no setup beyond what `beforeEach` already provides, but Act and Assert are always required.
+4. **Combined `// Act & Assert` is acceptable** only when the assertion must be wrapped inside a `waitFor` that is inseparable from the action (e.g., awaiting an async side-effect). In that case the comment must still include an explanation:
+   - Good: `// Act & Assert - empty state message is shown and error was logged to console`
+   - Bad: `// Act & Assert`
+5. **Multiple Act/Assert cycles in one test are allowed** for stateful interaction flows (e.g., click then verify, click again then verify). Each cycle must carry its own `// Act` and `// Assert - ...` comments.
+
+---
+
+## No unused variables, imports, or mocks
+
+**Urgency:** urgent
+
+### Category
+
+Maintainability
+
+### Confidence Threshold
+
+Flag when the symbol is clearly unused in the file or when a mock setup is not consumed by behavior under test. If a mock or import may be used implicitly (module mocking side effects, global setup conventions), verify before commenting.
+
+### Exceptions / False Positives
+
+- Do not flag side-effect imports (for example, polyfills or test environment setup) just because no identifier is referenced.
+- Do not flag module-level `jest.mock(...)` declarations that intentionally replace imports used elsewhere in the file.
+- If a placeholder parameter is required for function signature/position, allow underscore-prefixed names (for example, `_arg`).
+
+### Rules
+
+1. **Unused imports must be removed.** If a symbol is imported but never referenced in the file, delete the import. This includes named imports, default imports, and type-only imports.
+2. **Unused variables and constants must be removed.** If a `const`, `let`, or destructured binding is declared but never read, delete it. This applies to top-level declarations, inside `describe`/`beforeEach`/`test` blocks, and helper functions.
+3. **Unused mock declarations must be removed.** If `jest.fn()`, `jest.mock()`, `jest.spyOn()`, or a manual mock variable is set up but never referenced in an assertion or as a dependency, delete it. Mocks that are called implicitly (e.g., module-level `jest.mock('...')` that replaces an import used elsewhere) are considered used.
+4. **Unused mock return values must be removed.** If a mock is configured with `.mockReturnValue()`, `.mockResolvedValue()`, or `.mockImplementation()` but the return value is never consumed or asserted on, simplify or remove the configuration.
+5. **Unused helper functions and factory functions must be removed.** If a test utility, builder, or factory function defined in the file is never called, delete it.
+6. **Unused parameters in callbacks must be prefixed with `_`.** If a callback parameter (e.g., in `.mockImplementation((unusedArg) => ...)`) is required for positional reasons but not used, prefix it with `_` to signal intent (e.g., `_unusedArg`).
+7. **Unused `render` results must not be destructured.** If `render(<Component />)` is called and the return value is not used, do not destructure it. Write `render(<Component />);` instead of `const { container } = render(<Component />);` when `container` is never referenced.
+
+### Examples
+
+```tsx
+// ---- Unused imports ----
+
+// ❌ BAD — ApiResponse is imported but never used
+import { render, screen } from '@testing-library/react';
+import { ApiResponse, UserProfile } from '../models';
+
+const mockProfile: UserProfile = { name: 'Test' };
+
+// ✅ GOOD — only referenced imports remain
+import { render, screen } from '@testing-library/react';
+import { UserProfile } from '../models';
+
+const mockProfile: UserProfile = { name: 'Test' };
+
+
+// ---- Unused variables ----
+
+// ❌ BAD — mockHandler is declared but never used
+const mockHandler = jest.fn();
+const mockCallback = jest.fn();
+
+test('calls callback on click', () => {
+    render(<Button onClick={mockCallback} />);
+    fireEvent.click(screen.getByRole('button'));
+    expect(mockCallback).toHaveBeenCalledTimes(1);
+});
+
+// ✅ GOOD — only mockCallback remains
+const mockCallback = jest.fn();
+
+test('calls callback on click', () => {
+    render(<Button onClick={mockCallback} />);
+    fireEvent.click(screen.getByRole('button'));
+    expect(mockCallback).toHaveBeenCalledTimes(1);
+});
+
+
+// ---- Unused mock setup ----
+
+// ❌ BAD — jest.spyOn for console.warn is set up but never asserted or needed
+jest.spyOn(console, 'warn').mockImplementation(() => {});
+jest.spyOn(console, 'error').mockImplementation(() => {});
+
+test('renders error state', () => {
+    render(<ErrorBanner message="fail" />);
+    expect(screen.getByText('fail')).toBeInTheDocument();
+    expect(console.error).toHaveBeenCalled();
+    // console.warn is never checked
+});
+
+// ✅ GOOD — only the console.error spy remains
+jest.spyOn(console, 'error').mockImplementation(() => {});
+
+test('renders error state', () => {
+    render(<ErrorBanner message="fail" />);
+    expect(screen.getByText('fail')).toBeInTheDocument();
+    expect(console.error).toHaveBeenCalled();
+});
+
+
+// ---- Unused render destructuring ----
+
+// ❌ BAD — container is destructured but never referenced
+const { container } = render(<Header title="Hello" />);
+expect(screen.getByText('Hello')).toBeInTheDocument();
+
+// ✅ GOOD — render called without unused destructuring
+render(<Header title="Hello" />);
+expect(screen.getByText('Hello')).toBeInTheDocument();
+
+
+// ---- Unused callback parameter ----
+
+// ❌ BAD — `req` is not used but has no underscore prefix
+mockFetch.mockImplementation((req, options) => {
+    return Promise.resolve({ ok: true });
+});
+
+// ✅ GOOD — unused positional parameter prefixed with _
+mockFetch.mockImplementation((_req, options) => {
+    return Promise.resolve({ ok: true });
+});
+```
+
+---
+
+## Async React updates must be awaited (`act` warning prevention)
+
+**Urgency:** urgent
+
+### Category
+
+Correctness
+
+### Confidence Threshold
+
+Flag as a high-severity finding when the changed test triggers async React updates and asserts before the UI settles, or when `act(...)` warnings are present in test output. If the interaction and state updates are fully synchronous, do not force async patterns.
+
+### Exceptions / False Positives
+
+- Do not require `await` for purely synchronous interactions that do not trigger async state updates.
+- If the test uses a project helper that already waits for async UI stabilization, avoid duplicating `waitFor`/`findBy` calls.
+- When fake timers are used, accept explicit `act(...)` + timer advancement patterns that correctly flush updates.
+
+### Rules
+
+1. **If a component triggers async state updates after render, the test must await a UI-stable condition before assertions.** Look for `useEffect` with async work, fetch-on-mount patterns, or promise-based state transitions.
+2. **User interactions that trigger async updates must be awaited.** `userEvent.click`, `userEvent.type`, and similar calls should be `await`ed when the resulting handler performs async work.
+3. **Tests with async UI behavior must be marked `async`.** A test function that uses `await` must be declared `async`; a test whose component performs async work almost certainly needs both.
+4. **Presence of `act(...)` warnings in test output is a defect, even when tests pass.** Treat these warnings as test failures during review.
+
+### Heuristics for detection
+
+- `render(...)` followed by immediate assertions while the component has `useEffect(() => { fetch(...).then(...) }, [])` or similar async-on-mount logic.
+- `userEvent.click/type/...` followed by immediate assertions that depend on async updates.
+- Presence of `console.error` `act(...)` warnings in test output, even when tests pass.
+- Test functions not marked `async` despite the component performing async work.
+
+### Preferred fixes (in order of preference)
+
+1. **`await screen.findBy...(...)` — preferred.** Use for elements that appear after async work. Simpler and more idiomatic than `waitFor`.
+2. **`await waitFor(() => expect(...))`** — use when asserting on state-dependent conditions that `findBy` can't express (e.g., element attribute changes, disappearance).
+3. **Shared helpers** like `waitForComponentToLoad()` — call after render when available.
+4. **`await userEvent.click(...)`** — always await interactions that trigger async handlers.
+
+### Examples
+
+```tsx
+// ❌ BAD — immediate assertion after render; component fetches data on mount
+render(<ParticipantReports />);
+expect(screen.getByLabelText(/enter animal ids/i)).toBeInTheDocument();
+
+// ✅ GOOD — waits for async mount to settle before asserting
+render(<ParticipantReports />);
+await waitFor(() => {
+    expect(screen.getByText('General')).toBeVisible();
+});
+expect(screen.getByLabelText(/enter animal ids/i)).toBeInTheDocument();
+```
diff --git a/.agents/review-checklists/jest/performance.md b/.agents/review-checklists/jest/performance.md
new file mode 100644
index 0000000000..67cd2af28e
--- /dev/null
+++ b/.agents/review-checklists/jest/performance.md
@@ -0,0 +1,105 @@
+# Performance
+
+> **Prerequisite:** Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.
+
+## No redundant or near-duplicate tests
+
+**Urgency:** suggestion
+
+### Category
+
+Maintainability
+
+### Confidence Threshold
+
+Flag when two tests clearly exercise the same code path with identical setup/action and only trivial literal differences. If separate tests improve failure localization or document distinct branches, prefer a suggestion over a required merge.
+
+### Exceptions / False Positives
+
+- Do not merge tests that exercise different branches, error paths, permission states, or feature flags even if they look similar.
+- Keep separate tests when splitting assertions materially improves failure diagnostics or readability for a complex behavior.
+- Do not force `test.each` if parameterization would obscure the intent or make debugging harder than explicit tests.
+
+### Rules
+
+1. **Merge tests that assert the same behavior with different literal values.** If two or more tests render the same component, trigger the same interaction, and assert the same outcome shape — differing only in the data passed in — combine them into a single `test.each` or a single test with multiple representative cases.
+2. **Merge tests whose Arrange and Act steps are identical.** If two tests set up the same state and perform the same action but assert on different aspects of the result, combine them into one test with multiple assertions. A single test with several `expect` calls is preferable to duplicate setup/teardown overhead.
+3. **Remove tests that are strict subsets of another test.** If test A asserts that a button is rendered and test B asserts that clicking the same button fires a callback, test A is redundant — the click in test B already proves the button exists. **Exception:** keep the existence test if it tests a different conditional path than the behavioral test (e.g., the existence test renders with restricted permissions while the click test renders with full permissions).
+4. **Keep separate tests for genuinely distinct code paths.** Two tests that look similar but exercise different branches (e.g., an error path vs. a success path, an empty list vs. a populated list) are not duplicates and must remain separate.
+
+### Examples
+
+```tsx
+// ❌ BAD — three nearly identical tests differing only in input label
+test('renders label for first name', () => {
+    render(<FormField label="First Name" />);
+    expect(screen.getByText('First Name')).toBeInTheDocument();
+});
+
+test('renders label for last name', () => {
+    render(<FormField label="Last Name" />);
+    expect(screen.getByText('Last Name')).toBeInTheDocument();
+});
+
+test('renders label for email', () => {
+    render(<FormField label="Email" />);
+    expect(screen.getByText('Email')).toBeInTheDocument();
+});
+
+// ✅ GOOD — combined into a parameterized test
+test.each(['First Name', 'Last Name', 'Email'])('renders label "%s"', (label) => {
+    render(<FormField label={label} />);
+    expect(screen.getByText(label)).toBeInTheDocument();
+});
+
+
+// ❌ BAD — two tests with identical Arrange/Act, different Assert
+test('disables submit when form is invalid', () => {
+    render(<MyForm valid={false} />);
+    expect(screen.getByRole('button', { name: /submit/i })).toBeDisabled();
+});
+
+test('shows validation message when form is invalid', () => {
+    render(<MyForm valid={false} />);
+    expect(screen.getByText('Please fix errors above')).toBeInTheDocument();
+});
+
+// ✅ GOOD — combined into one test with both assertions
+test('shows validation state when form is invalid', () => {
+    render(<MyForm valid={false} />);
+    expect(screen.getByRole('button', { name: /submit/i })).toBeDisabled();
+    expect(screen.getByText('Please fix errors above')).toBeInTheDocument();
+});
+
+
+// ❌ BAD — subset test is redundant
+test('renders the delete button', () => {
+    render(<ItemRow item={mockItem} onDelete={mockDelete} />);
+    expect(screen.getByRole('button', { name: /delete/i })).toBeInTheDocument();
+});
+
+test('calls onDelete when delete button is clicked', () => {
+    render(<ItemRow item={mockItem} onDelete={mockDelete} />);
+    fireEvent.click(screen.getByRole('button', { name: /delete/i }));
+    expect(mockDelete).toHaveBeenCalledWith(mockItem.id);
+});
+
+// ✅ GOOD — only the behavioral test remains (it implicitly proves the button exists)
+test('calls onDelete when delete button is clicked', () => {
+    render(<ItemRow item={mockItem} onDelete={mockDelete} />);
+    fireEvent.click(screen.getByRole('button', { name: /delete/i }));
+    expect(mockDelete).toHaveBeenCalledWith(mockItem.id);
+});
+
+
+// ✅ OK — these look similar but test genuinely different code paths
+test('renders empty state when no items', () => {
+    render(<ItemList items={[]} />);
+    expect(screen.getByText('No items found')).toBeInTheDocument();
+});
+
+test('renders item rows when items are provided', () => {
+    render(<ItemList items={[mockItem]} />);
+    expect(screen.getByText(mockItem.name)).toBeInTheDocument();
+});
+```
diff --git a/.agents/review-checklists/react/business-logic.md b/.agents/review-checklists/react/business-logic.md
new file mode 100644
index 0000000000..0641b6e955
--- /dev/null
+++ b/.agents/review-checklists/react/business-logic.md
@@ -0,0 +1,248 @@
+# Rule Catalog — Business Logic
+
+> **Prerequisite:** Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.
+
+## Avoid using array index as React key
+
+**Urgency:** urgent
+
+### Category
+
+Correctness
+
+### Confidence Threshold
+
+Flag as a high-severity finding when the list can reorder, insert, delete, filter, or preserve item-local state. If the list is demonstrably static and append-only with no stateful children, leave at most a low-priority note.
+
+### Exceptions / False Positives
+
+- Allow index keys for truly static lists whose order and membership never change during the component lifecycle.
+- Do not flag when the rendered items are simple, stateless presentation nodes and the list is effectively constant data.
+- If a stable unique key is unavailable in the data model, note the risk and suggest data-model follow-up rather than inventing unstable keys in review.
+
+### Description
+
+Using array index as a `key` prop in React lists (e.g., `key={index}`, `key={`item-${index}`}`) can cause incorrect component behavior when items are reordered, inserted, or deleted. React uses keys to identify which items changed—index-based keys cause React to associate the wrong component instance with the wrong data, leading to:
+
+- State attached to the wrong list item
+- Input values appearing in the wrong row
+- Stale data displayed after list mutations
+
+### Suggested Fix
+
+Use a stable, unique identifier from the data itself:
+
+- **Before:** `items.map((item, index) => <Row key={index} ... />)`
+- **After:** `items.map(item => <Row key={item.id} ... />)`
+
+If no natural unique key exists, consider whether the data model should include one. As a last resort, generate stable IDs when the data is first loaded.
+
+---
+
+## Falsy value rendered by `&&` short-circuit
+
+**Urgency:** urgent
+
+### Category
+
+Correctness
+
+### Confidence Threshold
+
+Flag when the left side can be a number or string in the changed code path and React would render `0` or `""`. If the condition is already guaranteed boolean, do not flag.
+
+### Exceptions / False Positives
+
+- Do not flag when the left operand is explicitly boolean (for example, `isReady`, `Boolean(value)`, `!!value`).
+- Do not flag when rendering `0` is intentional behavior and is part of the UI requirement.
+- If TypeScript types or local guards prove the value cannot be a renderable falsy primitive, avoid speculative comments.
+
+### Description
+
+Using `&&` for conditional rendering with a numeric or string value can render `0` or `""` instead of nothing. JavaScript short-circuits to the left operand when it is falsy but not `null`/`undefined`/`false` — React renders `0` and `""` as text nodes.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Renders "0" when count is 0
+{count && <Badge count={count} />}
+
+// ❌ Renders "" when name is empty string
+{name && <Greeting name={name} />}
+```
+
+### Suggested Fix
+
+Convert the condition to a boolean explicitly:
+
+```tsx
+// ✅ Renders nothing when count is 0
+{count > 0 && <Badge count={count} />}
+
+// ✅ Or use a ternary
+{count ? <Badge count={count} /> : null}
+
+// ✅ Double negation for truthy check
+{!!name && <Greeting name={name} />}
+```
+
+---
+
+## Effect dependency array correctness
+
+**Urgency:** urgent
+
+### Category
+
+Correctness
+
+### Confidence Threshold
+
+Flag as a high-severity finding when a missing/unstable dependency can be shown to cause stale closures, skipped updates, or excessive reruns in the changed code. If the effect is intentionally one-time or dependency omission is documented with a valid reason, verify context before commenting.
+
+### Exceptions / False Positives
+
+- Do not flag values read through stable refs (`ref.current`) when the effect intentionally avoids rerunning on ref changes.
+- Allow justified `eslint-disable react-hooks/exhaustive-deps` comments when the code explains the invariant and the behavior is correct.
+- Do not require memoization solely to satisfy the rule if it adds complexity and the effect cost is trivial; prefer a contextual suggestion.
+
+### Description
+
+`useEffect`, `useMemo`, and `useCallback` dependency arrays must include every reactive value referenced inside the callback. Missing dependencies cause stale closures; extra dependencies cause unnecessary re-runs.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Missing dependency — handler captures stale `count`
+useEffect(() => {
+    document.addEventListener('click', () => console.log(count));
+}, []);
+
+// ❌ Object/array reference in deps without memoization — runs every render
+useEffect(() => {
+    fetchData(filters);
+}, [filters]); // if `filters` is created inline each render
+```
+
+### Suggested Fix
+
+```tsx
+// ✅ Include all referenced reactive values
+useEffect(() => {
+    const handler = () => console.log(count);
+    document.addEventListener('click', handler);
+    return () => document.removeEventListener('click', handler);
+}, [count]);
+
+// ✅ Derive a stable object from primitive values
+const stableFilters = useMemo(
+    () => ({ id: filters.id, status: filters.status }),
+    [filters.id, filters.status]
+);
+useEffect(() => {
+    fetchData(stableFilters);
+}, [stableFilters]);
+```
+
+### How to Detect
+
+1. For each `useEffect`/`useMemo`/`useCallback`, list variables from the enclosing scope referenced inside the callback.
+2. Compare against the dependency array. Flag any variable that is referenced but missing from deps.
+3. Flag objects or arrays in the deps array that are created inline each render (not memoized).
+
+---
+
+## Stale closure in async state updates
+
+**Urgency:** urgent
+
+### Category
+
+Correctness
+
+### Confidence Threshold
+
+Flag as a high-severity finding when async callbacks use captured state to compute next state and can overwrite newer updates. If the callback intentionally uses a snapshot value for comparison/logging (not state derivation), do not flag.
+
+### Exceptions / False Positives
+
+- Do not flag async callbacks that only read captured state for analytics, logging, or conditional branching without writing derived state back.
+- Do not flag when the update intentionally restores a specific captured value (snapshot semantics are the requirement).
+- If the setter does not derive from prior state (for example, sets a constant), the functional updater is not always necessary.
+
+### Description
+
+Using the current state value (rather than the updater function) inside `setTimeout`, `setInterval`, promises, or event listeners captures a stale snapshot. The update will overwrite intermediate changes.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Captures stale `count` — concurrent clicks lose updates
+const handleClick = () => {
+    setTimeout(() => {
+        setCount(count + 1);
+    }, 1000);
+};
+```
+
+### Suggested Fix
+
+Use the functional updater form:
+
+```tsx
+// ✅ Always reads the latest state
+const handleClick = () => {
+    setTimeout(() => {
+        setCount(prev => prev + 1);
+    }, 1000);
+};
+```
+
+---
+
+## Uncontrolled-to-controlled component switch
+
+**Urgency:** urgent
+
+### Category
+
+Correctness
+
+### Confidence Threshold
+
+Flag as a high-severity finding when a controlled form element `value`/`checked` can be `undefined` on initial render and later becomes defined in the same component path. If the element is intentionally uncontrolled (`defaultValue`/`defaultChecked`), do not apply this rule.
+
+### Exceptions / False Positives
+
+- Do not flag intentionally uncontrolled inputs that use `defaultValue` or `defaultChecked` rather than `value`/`checked`.
+- Do not flag when the component never renders the input until a defined value is available (for example, guarded render paths).
+- If a wrapper component normalizes `undefined` before passing props to the DOM input, verify that normalization before commenting.
+
+### Description
+
+Initializing a controlled input's value as `undefined` and later setting it to a string causes React to warn about switching from uncontrolled to controlled. This indicates a state initialization bug.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ value starts as undefined, then becomes a string
+const [input, setInput] = useState();
+// ...
+<input value={input} onChange={e => setInput(e.target.value)} />
+
+// ❌ Value from optional prop without fallback
+const [text, setText] = useState(props.initialText);
+// If props.initialText is undefined on first render, input is uncontrolled
+```
+
+### Suggested Fix
+
+Always initialize state with a value matching the controlled type:
+
+```tsx
+// ✅ Explicit empty string for text inputs
+const [input, setInput] = useState('');
+
+// ✅ Fallback for optional props
+const [text, setText] = useState(props.initialText ?? '');
+```
diff --git a/.agents/review-checklists/react/code-quality.md b/.agents/review-checklists/react/code-quality.md
new file mode 100644
index 0000000000..722b423e6d
--- /dev/null
+++ b/.agents/review-checklists/react/code-quality.md
@@ -0,0 +1,462 @@
+# Rule Catalog — Code Quality
+
+> **Prerequisite:** Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.
+
+## Props interface should be declared separately
+
+**Urgency:** suggestion
+
+### Category
+
+Style
+
+### Confidence Threshold
+
+Flag only when the component has multiple props or the inline type is large enough to reduce readability. For tiny local components with one simple prop, prefer no comment unless the file is being standardized.
+
+### Exceptions / False Positives
+
+- Do not flag tiny local components with a single simple prop where an inline type is shorter and clearer.
+- Do not require this pattern if the surrounding file consistently uses a different accepted convention and the PR is not refactoring style.
+- If the project avoids `FC` entirely and types props on the function parameter instead, apply the spirit of the rule (named props type) rather than the exact syntax.
+
+### Description
+
+Component props should be declared as a separate named interface rather than inline in the `FC<>` generic. This improves readability, enables JSDoc documentation on individual props, and maintains consistency across the codebase. Components with 2 or more props should always use a separate interface.
+
+Wrong:
+
+```tsx
+const MyComponent: FC<{ report: ReportConfig; tab: ExtReportTab }> = ({ report, tab }) => {
+    // ...
+};
+```
+
+### Suggested Fix
+
+```tsx
+interface MyComponentProps {
+    report: ReportConfig;
+    tab: ExtReportTab;
+}
+
+const MyComponent: FC<MyComponentProps> = ({ report, tab }) => {
+    // ...
+};
+```
+
+## Optional props that are always passed should be required
+
+**Urgency:** suggestion
+
+### Category
+
+Correctness
+
+### Confidence Threshold
+
+Flag when the changed PR includes the component and all visible call sites (or a small, searchable set of call sites) and every usage passes the prop. If external consumers or broad usage make this uncertain, leave a note.
+
+### Exceptions / False Positives
+
+- Do not require making a prop required when the component is exported for external/unknown consumers that are not visible in the current repo or PR.
+- Do not flag when the optional prop supports backward compatibility or gradual migration and that intent is documented.
+- If call sites are numerous or spread across the codebase, treat this as an audit suggestion rather than a high-severity review comment.
+
+### Description
+
+When reviewing a component, check if any optional props (marked with `?`) are actually passed in every single usage of the component. If a prop is always provided at every call site, it should be declared as required rather than optional. If the value can legitimately be `undefined`, use `T | undefined` as the type instead of `prop?: T`.
+
+This makes the component's contract explicit: callers know they must provide the prop, and the component implementation can rely on it being present (even if the value is `undefined`).
+
+Wrong (if `onClose` is passed at every call site):
+
+```tsx
+interface ModalProps {
+    title: string;
+    onClose?: () => void;  // Optional, but always passed in practice
+}
+
+// Every usage passes onClose:
+<Modal title="Confirm" onClose={handleClose} />
+<Modal title="Settings" onClose={() => setOpen(false)} />
+```
+
+### Suggested Fix
+
+```tsx
+interface ModalProps {
+    title: string;
+    onClose: (() => void) | undefined;  // Required prop, value may be undefined
+}
+
+// Or if it truly should never be undefined:
+interface ModalProps {
+    title: string;
+    onClose: () => void;  // Required prop, must have a value
+}
+```
+
+### How to Check
+
+1. Search for all usages of the component across the codebase
+2. For each optional prop, verify whether any call site omits it
+3. If all call sites provide the prop, flag it for conversion to required
+
+**Scope note:** For widely-used shared components (5+ call sites), this check can be deferred to a separate audit. Focus on components with 1–3 usages where the call sites are visible in the current PR.
+
+## No unused imports, variables, props, or exports
+
+**Urgency:** suggestion
+
+### Category
+
+Maintainability
+
+### Confidence Threshold
+
+Flag file-local unused imports/variables/props when unused status is clear in the changed file. For exported symbols, require evidence from a targeted search (preferably limited to changed/new exports in the PR) before flagging.
+
+### Exceptions / False Positives
+
+- Do not flag side-effect imports (for example, stylesheet imports, polyfills, module registration) that intentionally have no local references.
+- Do not flag exports that are consumed indirectly through framework conventions, dynamic imports, or generated wiring without checking project patterns first.
+- If verifying export usage requires a broad codebase audit outside the PR scope, lower confidence and leave a note.
+
+### Description
+
+Every import, variable, constant, destructured prop, and export in a file should be referenced or consumed. Unused symbols add noise, increase bundle size (for runtime imports), and signal incomplete refactors.
+
+**Important:** Exported types, interfaces, and constants must be verified as actually imported somewhere in the codebase—don't assume an export is used just because it exists.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Unused import
+import { useState, useEffect } from 'react';
+// only useState is used in the file — useEffect is dead
+
+// ❌ Unused variable / constant
+const label = 'hello';
+// label is never read
+
+// ❌ Unused destructured prop
+const MyComponent: FC<Props> = ({ name, age, email }) => {
+    return <span>{name}</span>;
+    // age and email are destructured but never used
+};
+
+// ❌ Unused `declare` statement
+declare const Ext4: any;
+// Ext4 is never referenced in the file
+
+// ❌ Unused exported type/interface/constant
+export type ReportType = 'js' | 'query' | 'report';
+// ReportType is exported but never imported anywhere in the codebase
+```
+
+### Suggested Fix
+
+Remove the unused symbol. If it is an import, remove just the unused specifier (keep others on the same line). If an entire import statement becomes empty, delete the line.
+
+```tsx
+// ✅ Only import what is used
+import { useState } from 'react';
+
+// ✅ Only destructure props that are used
+const MyComponent: FC<Props> = ({ name }) => {
+    return <span>{name}</span>;
+};
+
+// ✅ Only export what is consumed elsewhere
+// (remove exports that have no importers)
+```
+
+### How to Detect
+
+1. For each `import { A, B, C }` statement, search the rest of the file for references to `A`, `B`, and `C` (excluding the import line itself and type-only re-exports). Flag any specifier with zero references.
+2. For each top-level `const`, `let`, `var`, or `declare` statement, verify the declared name appears elsewhere in the file.
+3. For each destructured prop in a component signature, verify the name appears in the component body.
+4. **For each `export` (type, interface, const, function), grep the codebase to verify it is imported somewhere.** Do not assume exports are used—verify with a search.
+
+**Scope note:** Full codebase verification of exports is expensive during review. Focus on exports introduced or modified in the current PR. 
+
+## No redundant or historical comments
+
+**Urgency:** suggestion
+
+### Category
+
+Style
+
+### Confidence Threshold
+
+Flag comments only when they add no information beyond the code or document obsolete history without current relevance. If a comment explains a constraint, workaround, or non-obvious intent, do not flag it.
+
+### Exceptions / False Positives
+
+- Do not flag comments required by tooling or documentation generation (for example, public API docs or lint-enforced JSDoc).
+- Allow brief migration/history comments when they explain a current compatibility constraint and include actionable context for maintainers.
+- Do not flag comments that clarify business rules, framework quirks, or sequencing constraints not obvious from the code.
+
+### Description
+
+Comments should explain **why** something is done, not **what** is done. Remove comments that restate what the code already communicates through its naming, and comments that document the history or evolution of the code rather than its current purpose. These add noise without value.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Comment restates the interface/variable name
+/** Props for JSReportWrapper component */
+interface JSReportWrapperProps {
+
+// ❌ Comment restates what the variable name already says
+// The active tab ID
+const activeTabId = userSelectedTabId ?? defaultActive.tabId;
+
+// ❌ Comment describes code history or refactoring origin
+/**
+ * Extracted from the former ReportTab render-prop component.
+ */
+const ReportPanel: FC<ReportPanelProps> = (props) => {
+
+// ❌ Comment narrates what the next line of code does
+// Set the user selected category
+setUserSelectedCategory(category);
+
+// ❌ Comment explains an obvious return
+// Return null if no data
+if (!data) return null;
+```
+
+### When Comments Are Appropriate
+
+```tsx
+// ✅ Explains WHY — a non-obvious business rule or constraint
+// LABKEY.WebPart expects a string ID, not a DOM element
+const targetId = `report-target-${report.id}-${uniqueId}`;
+
+// ✅ Explains WHY — clarifies a workaround or unexpected pattern
+// partConfig requires an index signature because filter.getURLParameterName()
+// generates dynamic keys (e.g., "query.Id~eq") that can't be statically typed.
+const partConfig: Record<string, unknown> = { ... };
+
+// ✅ Documents a subtle gotcha that isn't obvious from the code
+// This effect must run after reports have loaded, not on mount
+useEffect(() => { ... }, [reports]);
+```
+
+### How to Detect
+
+1. Read each comment and the code it annotates. If deleting the comment loses no information that isn't already conveyed by the identifier names, types, or structure, flag it.
+2. Look for comments containing phrases like "extracted from", "formerly", "used to be", "was previously", "refactored from" — these describe history, not current behavior.
+3. Look for comments that mirror a pattern like `/** <Type> for <Name> */` directly above a type/interface/class declaration with a self-descriptive name.
+
+## Conditional class names use utility function
+
+**Urgency:** suggestion
+
+### Category
+
+Style
+
+### Confidence Threshold
+
+Flag only when the file/area already uses a class-name utility convention (for example, `classnames`, `clsx`, or local `cn`) and the inline conditional harms consistency/readability. For one-off simple cases in mixed-style files, prefer a suggestion.
+
+### Exceptions / False Positives
+
+- Do not require `classnames` specifically if the project uses another equivalent utility such as `clsx` or `cn`.
+- Do not flag simple static class strings or library APIs that require a computed string expression without a utility helper.
+- If introducing a utility import for a single trivial ternary would add more noise than value in a local file, prefer a suggestion.
+
+### Description
+
+Ensure conditional CSS is handled via the `classNames` utility (imported from `classnames`) instead of custom ternaries, string concatenation, or template strings directly in the className prop.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Direct ternary in className
+className={isActive ? 'active' : 'inactive'}
+className={condition ? 'class-a' : 'class-b'}
+
+// ❌ Template string with ternary
+className={`base ${isActive ? 'active' : ''}`}
+
+// ❌ String concatenation
+className={'btn ' + (isActive ? 'btn-active' : 'btn-inactive')}
+```
+
+### Correct Pattern
+
+```tsx
+import classNames from 'classnames';
+
+// ✅ Using classNames utility with object syntax
+className={classNames('btn', {
+    'btn-active': isActive,
+    'btn-inactive': !isActive,
+})}
+
+// ✅ Or with conditional classes
+className={classNames({
+    active: isActive,
+    inactive: !isActive,
+})}
+```
+
+### Exceptions
+
+- **Single static class swap:** A plain ternary choosing between two static string classes (`className={isX ? 'a' : 'b'}`) is acceptable when there is no combination logic. The `classNames` utility adds value primarily when combining multiple classes, not when selecting one of two.
+- **Third-party / framework classes:** Bootstrap or other framework classes used as-is do not need `classNames` wrapping (e.g., `className={isOpen ? 'panel-open' : 'panel-closed'}`).
+- **CSS Modules:** Ternaries over CSS Module references are idiomatic and do not require `classNames` (e.g., `className={isActive ? styles.active : styles.inactive}`).
+
+### How to Detect
+
+Search for patterns matching `className={...?...:...}` where the ternary is not wrapped in a `classNames()` or `cn()` call.
+
+## CSS class names should follow BEM naming convention
+
+**Urgency:** suggestion
+
+### Category
+
+Style
+
+### Confidence Threshold
+
+Flag only when the component/stylesheet area is clearly using BEM or the PR is introducing new classes into a BEM-styled file. If the file uses CSS Modules or another established naming scheme, do not apply this rule.
+
+### Exceptions / False Positives
+
+- Do not flag CSS Modules patterns or hashed class references where BEM is not the project convention.
+- Do not flag third-party library class names or attributes that must match external CSS/JS behavior.
+- In legacy non-BEM files, prefer localized consistency over forcing a partial BEM conversion in an unrelated PR.
+
+### Description
+
+CSS class names should follow the BEM (Block, Element, Modifier) convention for consistency and maintainability.
+
+- **Block:** A standalone component name in kebab-case (e.g., `search-form`)
+- **Element:** A part of a block, separated by double underscores `__` (e.g., `search-form__input`)
+- **Modifier:** A variant or state, separated by double hyphens `--` (e.g., `search-form--disabled`, `search-form__input--large`)
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Generic or meaningless class names
+className="container1"
+className="big-wrapper"
+
+// ❌ camelCase class names
+className="searchForm"
+className="navBarItem"
+
+// ❌ Deeply nested elements (more than one __ level)
+className="search-form__input__icon__svg"
+
+// ❌ Modifier used without the base class
+className="--disabled"
+className="__input"
+
+// ❌ Mixing conventions (BEM + camelCase or BEM + arbitrary nesting)
+className="searchForm__input--active"
+```
+
+### Correct Pattern
+
+```tsx
+// ✅ Block
+className="search-form"
+
+// ✅ Element
+className="search-form__input"
+className="search-form__submit-button"
+
+// ✅ Modifier on block
+className="search-form--disabled"
+className="search-form--compact"
+
+// ✅ Modifier on element
+className="search-form__input--large"
+className="search-form__input--error"
+
+// ✅ Combining base class with modifier using classNames utility
+className={classNames('search-form__input', {
+    'search-form__input--large': isLarge,
+    'search-form__input--error': hasError,
+})}
+```
+
+### How to Detect
+
+1. Look for `className` values that use camelCase instead of kebab-case — these should be converted to BEM blocks.
+2. Flag class names containing more than one `__` separator (e.g., `block__el1__el2`) — flatten to a single element level.
+3. Flag class names that start with `--` or `__` without a preceding block name — modifiers and elements must always include their block.
+4. Look for generic class names like `container`, `wrapper`, `item`, `box` without a block prefix — these should be scoped to a BEM block.
+
+### Exceptions
+
+- **Third-party / framework classes:** Bootstrap (`btn`, `btn-primary`, `col-xs-*`, `panel-body`, `form-control`, etc.), LabKey API classes (`labkey-*`), or other library-provided class names follow their own conventions and must not be flagged or renamed.
+- **CSS Modules:** When using CSS Modules (`.module.css` / `.module.scss`), camelCase class names are standard since they become JS property names (e.g., `styles.searchForm`, `styles.activeItem`). Do not flag these as BEM violations.
+- **Existing non-BEM code:** Class names in unchanged code that predate BEM adoption should not be flagged. Only apply BEM conventions to new or actively modified class names in the PR.
+
+## No orphaned or unused CSS/SCSS rules
+
+**Urgency:** suggestion
+
+### Category
+
+Maintainability
+
+### Confidence Threshold
+
+Flag when a changed selector has no usage in the changed component(s) and a targeted search finds no references. If selectors may be referenced dynamically or outside TSX/JSX, lower confidence and avoid a high-severity review comment without evidence.
+
+### Exceptions / False Positives
+
+- Do not flag selectors that are referenced dynamically (for example, string composition, server-rendered markup, or JS interop) without checking usage patterns.
+- Do not assume TSX/JSX-only usage; classes may be used in legacy templates, tests, docs, or external scripts tied to the component.
+- If full verification requires codebase-wide searching beyond the PR scope, mark as a follow-up suggestion rather than a high-severity finding.
+
+### Description
+
+Every CSS/SCSS selector in a stylesheet should correspond to a `className` actually used in the associated TSX/JSX component(s). Orphaned rules add dead code, increase bundle size, and mislead future developers into thinking a class is in use.
+
+### Anti-patterns to Flag
+
+```scss
+// ❌ Selector exists in SCSS but no component references it
+.filter-panel {
+    &__header { ... }
+    &__body { ... }
+    &__toggle-buttons { ... }  // No TSX file uses "filter-panel__toggle-buttons"
+}
+
+// ❌ Entire rule block with no matching className in any component
+.legacy-banner {
+    display: flex;
+    padding: 8px;
+}
+// No component renders className="legacy-banner"
+```
+
+### Correct Pattern
+
+```scss
+// ✅ Every selector maps to a className used in a component
+.filter-panel {
+    &__header { ... }   // <div className="filter-panel__header">
+    &__body { ... }      // <div className="filter-panel__body">
+}
+// Removed &__toggle-buttons because no component uses it
+```
+
+### How to Detect
+
+1. For each selector in a changed or reviewed SCSS/CSS file, search the codebase for a corresponding `className` usage (e.g., grep for the resolved class name in TSX/JSX files).
+2. Flag any selector with zero references — it is likely orphaned from a previous refactor.
+3. Pay special attention to nested BEM selectors (`&__element`, `&--modifier`) whose parent block exists but the specific element or modifier is no longer rendered.
+
+**Scope note:** Full codebase verification is expensive during manual review. Focus on CSS rules within files changed in the current PR.
\ No newline at end of file
diff --git a/.agents/review-checklists/react/performance.md b/.agents/review-checklists/react/performance.md
new file mode 100644
index 0000000000..be604c27ee
--- /dev/null
+++ b/.agents/review-checklists/react/performance.md
@@ -0,0 +1,144 @@
+# Rule Catalog — Performance
+
+> **Prerequisite:** Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.
+
+## Inline object/array literals in JSX props
+
+**Urgency:** urgent
+
+### Category
+
+Maintainability
+
+### Confidence Threshold
+
+Flag when referential identity matters in the changed code (for example, prop passed to a memoized child, dependency-sensitive hook, or effectful child) or when re-renders are already a known issue. If the child is not memoized and the object is tiny, prefer a suggestion or no comment.
+
+### Exceptions / False Positives
+
+- Do not flag inline literals passed to non-memoized children when there is no evidence that referential identity affects behavior or performance.
+- Do not require `useMemo` for values that are truly static and can simply be hoisted to module scope instead.
+- Avoid adding `useMemo` by default when it meaningfully harms readability and there is no demonstrated performance concern.
+
+### Description
+
+Do not pass inline object or array literals directly as JSX props. Each render creates a new reference, causing child components to re-render even when the values haven't changed. Extract the value into a `useMemo` (or a module-level constant if truly static) to preserve referential identity.
+
+Wrong:
+
+```tsx
+<HeavyComp
+    config={{
+        provider: ...,
+        detail: ...
+    }}
+/>
+```
+
+Right:
+
+```tsx
+const config = useMemo(() => ({
+    provider: ...,
+    detail: ...
+}), [provider, detail]);
+
+<HeavyComp
+    config={config}
+/>
+```
+
+---
+
+## Expensive computations in render should use `useMemo`
+
+**Urgency:** suggestion
+
+### Category
+
+Maintainability
+
+### Confidence Threshold
+
+Flag when the computation is non-trivial (for example, sorting/filtering large collections or repeated transformations) and can run frequently with unchanged inputs. For small arrays or infrequent renders, make this a suggestion only.
+
+### Exceptions / False Positives
+
+- Do not force `useMemo` for cheap computations over small data where memoization adds more complexity than benefit.
+- Do not flag one-time or rarely re-rendered components without evidence that render cost is material.
+- If dependencies are hard to express correctly and memoization risks stale data bugs, prefer a profiling-backed follow-up suggestion.
+
+### Description
+
+Operations that sort, filter, reduce, or transform large collections should be wrapped in `useMemo` so the work only runs when inputs change — regardless of whether the result is passed as a prop. This rule targets **computational cost**, not referential identity.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Re-sorts on every render even when `items` and `sortKey` haven't changed
+const SortedList: FC<{ items: Item[]; sortKey: string }> = ({ items, sortKey }) => {
+    const sorted = [...items].sort((a, b) => a[sortKey].localeCompare(b[sortKey]));
+    return <ul>{sorted.map(item => <li key={item.id}>{item.name}</li>)}</ul>;
+};
+```
+
+### Suggested Fix
+
+```tsx
+// ✅ Only recalculates when items or sortKey change
+const SortedList: FC<{ items: Item[]; sortKey: string }> = ({ items, sortKey }) => {
+    const sorted = useMemo(
+        () => [...items].sort((a, b) => a[sortKey].localeCompare(b[sortKey])),
+        [items, sortKey]
+    );
+    return <ul>{sorted.map(item => <li key={item.id}>{item.name}</li>)}</ul>;
+};
+```
+
+---
+
+## No component definitions inside other components
+
+**Urgency:** urgent
+
+### Category
+
+Correctness
+
+### Confidence Threshold
+
+Flag as a high-severity finding when a nested function is used as a React component type (capitalized and rendered with JSX) inside a component body, causing remounts and state loss. Do not confuse this with render helpers that return JSX but are not treated as component types.
+
+### Exceptions / False Positives
+
+- Do not flag plain helper functions inside components that return JSX and are called like normal functions (not rendered as `<Helper />`).
+- Do not flag render-prop callbacks or inline functions passed to APIs that expect functions rather than component types.
+- If the nested component is intentionally recreated and stateless for a localized pattern, prefer a note unless remount behavior is harmful.
+
+### Description
+
+Defining a component (function or arrow) inside another component's render body creates a new component type on every render. React unmounts and remounts the inner component each time, destroying all state and DOM, which causes flickering and performance problems.
+
+### Anti-patterns to Flag
+
+```tsx
+// ❌ Inner component re-created every render — loses state on each parent re-render
+const ParentComponent: FC = () => {
+    const InnerRow = ({ item }) => <tr><td>{item.name}</td></tr>;
+
+    return <table>{items.map(item => <InnerRow key={item.id} item={item} />)}</table>;
+};
+```
+
+### Suggested Fix
+
+Move the inner component to module scope:
+
+```tsx
+// ✅ Stable component identity — React can reconcile correctly
+const InnerRow: FC<{ item: Item }> = ({ item }) => <tr><td>{item.name}</td></tr>;
+
+const ParentComponent: FC = () => {
+    return <table>{items.map(item => <InnerRow key={item.id} item={item} />)}</table>;
+};
+```
\ No newline at end of file
diff --git a/.claude/skills/code-review-jest/skill.md b/.claude/skills/code-review-jest/skill.md
new file mode 100644
index 0000000000..568eb90ca0
--- /dev/null
+++ b/.claude/skills/code-review-jest/skill.md
@@ -0,0 +1,123 @@
+---
+name: code-review-jest
+description: "Trigger when the user requests a review of Jest test files (e.g., `.test.tsx`, `.spec.tsx`). Supports --full flag for complete file/directory review; defaults to staged changes when a path is provided."
+---
+
+# Jest Test Code Review
+
+## Intent
+Use this skill whenever the user asks to review Jest test code (especially `.test.tsx`, `.spec.tsx`, `.test.ts`, or `.spec.ts` files). Support three review modes:
+
+1. **Pending-change review** – bare invocation with no arguments; inspects staged/working-tree
+   files slated for commit across all repos.
+2. **Path review (default)** – a file or directory path is provided (no flag); reviews only the
+   git staged/working-tree changes for that path.
+3. **Full review** – a file or directory path is provided with `--full`; reviews the entire file
+   contents (or all matching files in a directory) regardless of git status.
+
+Stick to the checklist below for every applicable file and mode. Apply only the Jest checklist rules to test files — do not apply React component rules to component code that happens to be visible via imports in the test file.
+
+## Checklist
+See [.agents/review-checklists/common.md](../../../.agents/review-checklists/common.md) for reviewer priority and standard review format, and [.agents/review-checklists/jest/code-quality.md](../../../.agents/review-checklists/jest/code-quality.md), [.agents/review-checklists/jest/performance.md](../../../.agents/review-checklists/jest/performance.md), [.agents/review-checklists/jest/business-logic.md](../../../.agents/review-checklists/jest/business-logic.md) for the living checklist split by category—treat it as the canonical set of rules to follow.
+
+Use the rule's `Urgency` to place findings in the "urgent issues" vs "suggestions" sections.
+
+## Review Process
+
+**Argument parsing:**
+
+Parse the invocation arguments to extract:
+- An optional flag: `--full`
+- An optional path: a file path or directory path
+
+**File discovery:**
+
+- **No path, no flag (bare invocation):** Pending-change review. Discover nested repos by
+  running `find server/modules -maxdepth 2 -name ".git" -type d` from the workspace root,
+  then for each discovered repo (and the top-level root) run `git diff --cached --name-only`
+  and `git diff --name-only`. Aggregate all results, filtering to test file extensions
+  (`.test.tsx`, `.spec.tsx`, `.test.ts`, `.spec.ts`).
+
+- **Path provided (no `--full` flag — default):** Determine the git root via
+  `git -C <path> rev-parse --show-toplevel`.
+  - *File path:* Run `git diff --cached -- <file>` and `git diff -- <file>`. If there are
+    no changes, report "No staged or working-tree changes found for `<file>`." and stop.
+    If there are changes, proceed to review.
+  - *Directory path:* Run `git diff --cached --name-only -- <dir>` and
+    `git diff --name-only -- <dir>`. Filter to test file extensions. If no changed files
+    are found, report "No staged or working-tree changes found under `<dir>`." and stop.
+
+- **Path + `--full`:** Review the entire contents regardless of git status.
+  - *File path:* Use the file directly — no git discovery needed.
+  - *Directory path:* Find all files matching test file extensions under the directory
+    (using glob/find). Review every matching file.
+
+- **`--full` with no path:** Ask the user to provide a file or directory path.
+
+**Review scope when reviewing staged changes (default mode):**
+
+When reviewing staged changes, read the full file for context but **focus the review on changed
+lines and their immediate surroundings**. Use the diff output to identify which sections
+changed, then apply the checklist rules primarily to those sections. Still flag issues in
+unchanged code only if they directly interact with or are affected by the changes.
+
+**Multi-file grouping:** When reviewing multiple files, group all findings together by urgency section (urgent issues first, then suggestions), not per-file. Include the file path in each finding's `FilePath` field.
+
+1. Open the relevant test file(s). Gather all lines.
+2. For each applicable checklist rule, evaluate the code against the rule text, confidence threshold, and exceptions/false positives before deciding to flag it.
+3. For each confirmed violation, capture evidence (exact snippet and/or file/line), record the rule's primary category, and note confidence briefly.
+4. Compose the review section per the template below. Group findings by **Urgency** section first (urgent issues, then suggestions). Within each section, order findings by the checklist primary category priority: **Correctness**, then **Maintainability**, then **Style**.
+
+## Required output
+When invoked, the response must exactly follow one of the two templates:
+
+### Template A (any findings)
+```
+# Code review
+Found <N> urgent issues that need to be fixed:
+
+## 1 <brief description of bug>
+FilePath: <path> line <line>
+Evidence: <relevant code snippet or pointer>
+Category: <Correctness | Maintainability | Style>
+Confidence: <high | medium | low> - <brief justification>
+Exceptions checked: <none apply | brief exception note>
+
+
+### Suggested fix
+<brief description of suggested fix>
+
+---
+... (repeat for each urgent issue) ...
+
+Found <M> suggestions for improvement:
+
+## 1 <brief description of suggestion>
+FilePath: <path> line <line>
+Evidence: <relevant code snippet or pointer>
+Category: <Correctness | Maintainability | Style>
+Confidence: <high | medium | low> - <brief justification>
+Exceptions checked: <none apply | brief exception note>
+
+
+### Suggested fix
+<brief description of suggested fix>
+
+---
+
+... (repeat for each suggestion) ...
+```
+
+If there are no urgent issues, omit that section. If there are no suggestions, omit that section.
+
+Always list every urgent issue — never cap or truncate them. If there are more than 10 suggestions, summarize as "10+ suggestions" and output the first 10.
+
+Don't compress the blank lines between sections; keep them as-is for readability.
+
+If you use Template A (i.e., there are issues to fix) and at least one issue requires code changes, append a brief follow-up question after the structured output asking whether the user wants you to apply the suggested fix(es). For example: "Would you like me to apply the suggested fixes?"
+
+### Template B (no issues)
+```
+# Code review
+No issues found.
+```
diff --git a/.claude/skills/code-review-react/skill.md b/.claude/skills/code-review-react/skill.md
new file mode 100644
index 0000000000..3cb2a78a6f
--- /dev/null
+++ b/.claude/skills/code-review-react/skill.md
@@ -0,0 +1,123 @@
+---
+name: code-review-react
+description: "Trigger when the user requests a review of frontend files (e.g., `.tsx`, `.ts`, `.js`). Excludes test files. Supports --full flag for complete file/directory review; defaults to staged changes when a path is provided."
+---
+
+# Frontend Code Review
+
+## Intent
+Use this skill whenever the user asks to review frontend code (especially `.tsx`, `.ts`, `.scss`, `.css` or `.js` files). **Exclude test files** with extensions `.test.tsx`, `.test.ts`, `.spec.tsx`, or `.spec.ts` — those should be reviewed using the `code-review-jest` skill instead. Support three review modes:
+
+1. **Pending-change review** – bare invocation with no arguments; inspects staged/working-tree
+   files slated for commit across all repos.
+2. **Path review (default)** – a file or directory path is provided (no flag); reviews only the
+   git staged/working-tree changes for that path.
+3. **Full review** – a file or directory path is provided with `--full`; reviews the entire file
+   contents (or all matching files in a directory) regardless of git status.
+
+Stick to the checklist below for every applicable file and mode. Apply only the React/frontend checklist rules — do not apply Jest test rules to test code that may be co-located or visible in the same file.
+
+## Checklist
+See [.agents/review-checklists/common.md](../../../.agents/review-checklists/common.md) for reviewer priority and standard review format, and [.agents/review-checklists/react/code-quality.md](../../../.agents/review-checklists/react/code-quality.md), [.agents/review-checklists/react/performance.md](../../../.agents/review-checklists/react/performance.md), [.agents/review-checklists/react/business-logic.md](../../../.agents/review-checklists/react/business-logic.md) for the living checklist split by category—treat it as the canonical set of rules to follow.
+
+Use the rule's `Urgency` to place findings in the "urgent issues" vs "suggestions" sections.
+
+## Review Process
+
+**Argument parsing:**
+
+Parse the invocation arguments to extract:
+- An optional flag: `--full`
+- An optional path: a file path or directory path
+
+**File discovery:**
+
+- **No path, no flag (bare invocation):** Pending-change review. Discover nested repos by
+  running `find server/modules -maxdepth 2 -name ".git" -type d` from the workspace root,
+  then for each discovered repo (and the top-level root) run `git diff --cached --name-only`
+  and `git diff --name-only`. Aggregate all results, filtering to applicable frontend
+  extensions (`.tsx`, `.ts`, `.js`, `.scss`, `.css`) and excluding test files.
+
+- **Path provided (no `--full` flag — default):** Determine the git root via
+  `git -C <path> rev-parse --show-toplevel`.
+  - *File path:* Run `git diff --cached -- <file>` and `git diff -- <file>`. If there are
+    no changes, report "No staged or working-tree changes found for `<file>`." and stop.
+    If there are changes, proceed to review.
+  - *Directory path:* Run `git diff --cached --name-only -- <dir>` and
+    `git diff --name-only -- <dir>`. Filter to applicable extensions. If no changed files
+    are found, report "No staged or working-tree changes found under `<dir>`." and stop.
+
+- **Path + `--full`:** Review the entire contents regardless of git status.
+  - *File path:* Use the file directly — no git discovery needed.
+  - *Directory path:* Find all files matching applicable extensions under the directory
+    (using glob/find). Review every matching file.
+
+- **`--full` with no path:** Ask the user to provide a file or directory path.
+
+**Review scope when reviewing staged changes (default mode):**
+
+When reviewing staged changes, read the full file for context but **focus the review on changed
+lines and their immediate surroundings**. Use the diff output to identify which sections
+changed, then apply the checklist rules primarily to those sections. Still flag issues in
+unchanged code only if they directly interact with or are affected by the changes.
+
+**Multi-file grouping:** When reviewing multiple files, group all findings together by urgency section (urgent issues first, then suggestions), not per-file. Include the file path in each finding's `FilePath` field.
+
+1. Open the relevant component/module. Gather all lines.
+2. For each applicable checklist rule, evaluate the code against the rule text, confidence threshold, and exceptions/false positives before deciding to flag it.
+3. For each confirmed violation, capture evidence (exact snippet and/or file/line), record the rule's primary category, and note confidence briefly.
+4. Compose the review section per the template below. Group findings by **Urgency** section first (urgent issues, then suggestions). Within each section, order findings by the checklist primary category priority: **Correctness**, then **Maintainability**, then **Style**.
+
+## Required output
+When invoked, the response must exactly follow one of the two templates:
+
+### Template A (any findings)
+```
+# Code review
+Found <N> urgent issues that need to be fixed:
+
+## 1 <brief description of bug>
+FilePath: <path> line <line>
+Evidence: <relevant code snippet or pointer>
+Category: <Correctness | Maintainability | Style>
+Confidence: <high | medium | low> - <brief justification>
+Exceptions checked: <none apply | brief exception note>
+
+
+### Suggested fix
+<brief description of suggested fix>
+
+---
+... (repeat for each urgent issue) ...
+
+Found <M> suggestions for improvement:
+
+## 1 <brief description of suggestion>
+FilePath: <path> line <line>
+Evidence: <relevant code snippet or pointer>
+Category: <Correctness | Maintainability | Style>
+Confidence: <high | medium | low> - <brief justification>
+Exceptions checked: <none apply | brief exception note>
+
+
+### Suggested fix
+<brief description of suggested fix>
+
+---
+
+... (repeat for each suggestion) ...
+```
+
+If there are no urgent issues, omit that section. If there are no suggestions, omit that section.
+
+Always list every urgent issue — never cap or truncate them. If there are more than 10 suggestions, summarize as "10+ suggestions" and output the first 10.
+
+Don't compress the blank lines between sections; keep them as-is for readability.
+
+If you use Template A (i.e., there are issues to fix) and at least one issue requires code changes, append a brief follow-up question after the structured output asking whether the user wants you to apply the suggested fix(es). For example: "Would you like me to apply the suggested fixes?"
+
+### Template B (no issues)
+```
+# Code review
+No issues found.
+```