---
name: frontend-code-review
description: Perform a staff-engineer-level code review of frontend changes in any stack (React, React Native, Angular, Salesforce LWC, plain TypeScript) — correctness, architecture boundaries, accessibility, performance, security, testing, and maintainability — and deliver findings ordered by severity with concrete fixes. Use this whenever the user asks to review a PR, diff, component, or file; asks "is this good", "what's wrong with this", "any issues here", or pastes frontend code and asks for feedback. Also use it for self-review before opening a PR and for writing review comments in a constructive tone.
metadata:
  technology: Frontend (general)
  type: code-review
---

# Frontend Code Review

A review's job is to catch what tests and linters can't: wrong abstractions, missing edge cases, accessibility gaps, and future maintenance pain. Be specific, cite the line, propose the fix, and rank by severity so the author knows what blocks merge.

## 1. Before reading the diff

- Read the PR description / ticket. If there isn't one, ask for the intent in one sentence — you cannot review correctness without knowing the goal.
- Identify the stack and pull in the matching skill (`react-architecture`, `angular-architecture`, `lwc-development`, …) for stack-specific rules. This skill covers what's common.
- Check the diff size. > 400 changed lines of non-generated code → recommend splitting before detailed review; review only the riskiest file if the author can't split.

## 2. Severity scale (use these labels verbatim)

| Label | Meaning | Blocks merge? |
|---|---|---|
| **[blocker]** | Bug, security issue, data loss, broken a11y for a primary flow, violates a documented architecture boundary | Yes |
| **[major]** | Likely bug under realistic conditions, missing error/loading state, performance regression, missing test for new behaviour | Yes, unless explicitly deferred with a ticket |
| **[minor]** | Readability, naming, small duplication, non-idiomatic usage | No |
| **[nit]** | Style, formatting not caught by tooling | No |
| **[question]** | Need the author's intent before judging | — |
| **[praise]** | Something done well that others should copy | — |

Every finding gets one label. Include at least one `[praise]` when deserved — it calibrates the rest.

## 3. Checklist — run all sections, report only what you find

### Correctness
- Null/undefined paths: optional chaining hiding real bugs? Empty arrays, `0`, `''` handled as valid values?
- Async: unhandled rejections, race conditions (stale response overwriting newer), missing cancellation on unmount/navigation.
- Dates/times: timezone assumptions, `Date` mutation, locale-dependent parsing.
- Numbers/money: floating point for currency, missing `Intl` formatting.
- Conditional rendering: every branch has a designed state (loading, empty, error, partial).
- Forms: validation runs on submit *and* surfaces errors accessibly; disabled-while-submitting; double-submit prevented.
- i18n: hardcoded user-facing strings; string concatenation that breaks in other word orders; pluralisation.

### Architecture & boundaries
- Does the change live in the right layer (feature vs shared vs core)? Cross-feature imports of internals?
- New abstraction justified by ≥ 2 real call sites? Premature generalisation is as costly as duplication.
- Business logic inside UI components that should be in `model/`/services (untestable without rendering).
- Public API of a component/module grew — is it documented, typed, and is the old surface still consistent?
- Dependencies added: size, maintenance status, licence, does the platform already provide it?

### Types
- `any`, `as unknown as`, `!` non-null assertions — each needs a comment or a fix.
- Types derived from the source of truth (API schema, `typeof`, `keyof`) rather than duplicated.
- Discriminated unions instead of optional-everything shapes.

### Accessibility (WCAG 2.2 AA is the bar)
- Interactive elements are real `<button>`/`<a>`/inputs or have role + keyboard handling + focus styles.
- Every input has a label; every icon-only control has an accessible name; images have `alt` (empty for decorative).
- Focus management on route change, modal open/close, and after async actions (move focus to result or error).
- Colour is not the only carrier of meaning; contrast ≥ 4.5:1 for text.
- Live regions (`role="status"`/`alert`) for async feedback; no auto-playing motion without `prefers-reduced-motion` respect.
- Touch targets ≥ 24×24 CSS px (44 recommended on mobile).

### Performance
- Work in render paths that should be memoised or moved (formatting, sorting, `new Date()`, `JSON.parse`).
- Lists: stable keys, virtualisation for large data, no index keys on reorderable data.
- Bundle: full-library imports, heavy dependency for a small task, missing lazy loading of a heavy route/widget.
- Network: waterfalls (sequential awaits), refetch storms, missing caching/dedup, unbounded polling.
- Images/fonts without dimensions or optimisation.
- Subscriptions/timers/listeners without cleanup.

### Security
- User-controlled HTML rendered (`dangerouslySetInnerHTML`, `[innerHTML]`, `lwc:dom="manual"`) without sanitisation.
- Secrets, tokens, or internal URLs in client code or committed env files.
- Open redirects from query params; `target="_blank"` without `rel="noopener"`.
- Authorisation decided only on the client (hidden button ≠ protected action).
- Third-party scripts loaded without integrity/CSP consideration; `eval`/`new Function`.
- Logging PII to console/analytics.

### Testing
- New behaviour has a test at the right layer (see the stack's testing skill). Bug fixes include a regression test that fails without the fix.
- Tests assert user-visible outcomes, not implementation (no asserting on state setters or private methods).
- Mocks are at the network/boundary, not of your own modules.
- Flaky patterns: `setTimeout`, `waitFor` with huge timeouts, index-based queries.

### Maintainability
- Names reveal intent (`isSubmitDisabled` not `flag2`); booleans read as predicates; files/components under ~200 lines or split.
- Comments explain *why*, not *what*; TODOs have a ticket.
- Dead code, commented-out blocks, console logs, leftover feature-flag branches removed.
- Consistent with existing conventions in the repo — even when the reviewer would personally choose otherwise. Flag convention debates as `[question]`, not `[minor]`.
- Error handling: errors are surfaced to users meaningfully and reported to monitoring; not swallowed with empty `catch`.

## 4. Writing the comment

For each finding:

```
[label] <file>:<line> — <one-sentence problem>
Why it matters: <impact in one sentence>
Suggest:
<code block with the fix, or a precise description>
```

Tone rules:
- Address the code, not the person ("this handler re-subscribes each render" not "you forgot").
- Prefer questions when uncertain about intent; prefer statements when the issue is objective.
- One fix per comment; don't stack unrelated issues.
- Don't restate what a linter/formatter will catch — say "run the formatter" once.

## 5. Review summary (always end with this)

```
## Summary
Verdict: Approve | Approve with minor changes | Request changes
Blockers: <n>  Majors: <n>  Minors: <n>

Top 3 things to address:
1. ...
2. ...
3. ...

What's good: <one or two lines of genuine praise>
```

Verdict rules: any `[blocker]` → Request changes; `[major]` without an agreed follow-up ticket → Request changes; otherwise Approve (with minor changes if any `[minor]`).

## 6. Self-review mode

When the user asks to review *their own* code before a PR, run the same checklist but additionally produce:
- A suggested PR description (intent, approach, testing done, screenshots needed, risks).
- A list of the reviewer questions they should pre-empt in the description.

## Reviewer anti-patterns to avoid

- Nitpicking style while missing the race condition.
- "Consider using X" with no reason — always say why.
- Requesting a rewrite to your preferred pattern when the current one matches repo conventions.
- Reviewing a 2 000-line PR line by line instead of asking for a split.
- Approving with "LGTM" on code with no tests and new behaviour.