AI front end code review guidelines and claude skills (#1292)

- .agents/review-checklists/jest code review checks
- .agents/review-checklists/react code review checks
- .agents/review-checkslists/common.md general code review guidelines
- `.claude/skill/<skill name>/skill.md` define the claude skill using these guidelines 
---------

Co-authored-by: Alan Vezina <alanv@labkey.com>

Author: labkey-martyp

.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

.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.
+- 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} />);
+const row = screen.getByRole('row', { name: /alpha\s+10/i });
+expect(within(row).getByRole('cell', { name: 'Alpha' })).toBeInTheDocument();
+expect(within(row).getByRole('cell', { name: '10' })).toBeInTheDocument();
+```

.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();
+```

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