frontend-code-review

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.

Download .zip Raw Source

When agents use itUse 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.

Install

Copilot (VS Code, Visual Studio, Copilot CLI and github.com) reads skills from the repository — commit them so the whole team gets them.

npx skills add AGCO-Global/org-skills --skill frontend-code-review -a github-copilot
# or with the org installer (adds .github/skills/frontend-code-review):
npx -y github:AGCO-Global/org-skills add skill frontend-code-review

Uses the open skills CLI. Works with Claude Code, Codex, Cursor, Copilot, Gemini CLI, OpenCode, Windsurf and 60+ others — it asks which agent to install into.

npx skills add AGCO-Global/org-skills --skill frontend-code-review
# user-level instead of project-level:
npx skills add AGCO-Global/org-skills --skill frontend-code-review -g

Installs the frontend-shared-skills plugin, which bundles all Frontend / Shared skills and keeps them updated.

/plugin marketplace add AGCO-Global/org-skills
/plugin install frontend-shared-skills@org-skills
# or just this skill, in this repository:
npx skills add AGCO-Global/org-skills --skill frontend-code-review -a claude-code

Use Download .zip above, then upload it under Customize → Skills → Upload skill. Team and Enterprise admins can sync this repo as a plugin marketplace instead.

Installs into .agents/skills/, which Codex reads.

npx skills add AGCO-Global/org-skills --skill frontend-code-review -a codex

Installs into Cursor's skills folder.

npx skills add AGCO-Global/org-skills --skill frontend-code-review -a cursor

Installs into Gemini CLI's skills folder.

npx skills add AGCO-Global/org-skills --skill frontend-code-review -a gemini-cli

Any tool with rules, instructions or custom prompts: use Copy SKILL.md above and paste it in. It is plain Markdown.

Commands use your normal git sign-in to GitHub, so they work while the repository is private. Node.js 20+ required.

Skill contents

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.

---
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.