Skip to main content

/code-review — Code Review

Code review for the codebase using diffs plus mandatory checks against .agents/rules/*.mdc, existing feature references, coding standards, and clear untwisted logic. Use whenever the user asks to review, check, or audit frontend changes — staged, a file, uncommitted work, or a PR. Triggers include "does this look right?", "any issues?", "review my changes", or React/Next/TS/Tailwind/a11y review in this repo.

Source: .agents/skills/code-review/SKILL.md

Metadata​

  • name: code-review

Content​

/code-review — Code Review

Reviews frontend code changes against the project's conventions and React/Next.js best practices.

Project rules, coding standards, and feature references are mandatory for this review. After loading the diff, load the relevant rules under .agents/rules/, coding standards under .agents/references/coding-standard/, and feature references under .agents/references/features/. Verify the changed code complies and that the implementation logic is clear, direct, and not unnecessarily twisted. When a finding conflicts with a rule file, the rule file wins—call that out as Critical or Warning depending on severity.

Usage​

/code-review [target]
  • No argument: review staged changes (git diff --cached)
  • all: all uncommitted changes (git diff HEAD)
  • file:<path>: a specific file
  • pr: current PR (gh pr diff)

Step 1: Get the diff​

Run the appropriate command based on the target argument, then read the full content of each changed file for context around the diff.

Step 2: Load and apply project rules (.agents/rules)​

  1. Discover rules: List or scan .agents/rules/**/*.mdc (all alwaysApply and topic-specific rule files live here).

  2. Select what applies to the touched files—for example:

    If the diff touches…Read at least
    Any TS/TSXclean-typescript.mdc, prefer-typescript-files.mdc, no-type-assertion-as.mdc, readable-variable-names.mdc, no-redundant-alias-variables.mdc
    React components / hooksclean-react.mdc, no-unnecessary-usecallback.mdc, no-functions-in-effect-deps.mdc, destructure-hook-results.mdc, react-one-fc-per-file.mdc, vercel-react-performance.mdc → .agents/references/coding-standard/vercel-react-best-practices/REPO-GUIDANCE.md and applicable rules/*.md
    public/locales or t(...) keysno-direct-locale-additions.mdc
    @/services/... or service-style callersapifox-api-source-of-truth.mdc, no-try-catch-service-api.mdc
    Imports / barrelsno-reexport-index-files.mdc
    New pages / shared placementpage-structure.mdc → follow .agents/references/coding-standard/page-structure.md
    Styling / JSX classNameno-inline-styles.mdc, use-classnames-for-conditional-classes.mdc
    Figma / design-spec UIclean-figma.mdc → follow references/coding-standard/figma-guidance.md
    Security-sensitive codeweb-security.mdc
    Scripts, package.json, install commandsbun-first.mdc

  3. Deep references: If clean-react.mdc points to effect guidance, use .agents/references/coding-standard/useeffect-guidance.md (or the path given in that rule). If Figma applies, use .agents/references/coding-standard/figma-guidance.md. If page-structure.mdc applies, use .agents/references/coding-standard/page-structure.md. For TS/TSX React changes, use vercel-react-best-practices/REPO-GUIDANCE.md and relevant rules/*.md.

  4. Rules pass: In the review output, add a short Rules compliance subsection: list rules you checked and either "No violations" or bullet findings tied to rule names (e.g. "Violates no-try-catch-service-api: try/catch around getFoo in Bar.tsx").

  5. Coding-standard pass: List coding standards checked from .agents/references/coding-standard/ and flag violations or omissions. If no coding standard applies, state Coding standards checked: none.

Step 3: Load feature context for domain changes​

If the diff touches a product domain, route family, shared service, hook, or utility, load .agents/skills/feature-context/SKILL.md and the relevant files under .agents/references/features/. Use those references to catch domain-specific regressions, but still review against the actual diff and source files.

Examples:

  • Checkout/payment changes -> checkout-and-payments.md
  • Community route/product-card changes -> community-public-pages.md and often products-and-commerce.md
  • Middleware/auth redirects -> auth-routing-and-middleware.md
  • Shared hooks/services/utilities -> shared-services-hooks-and-utilities.md

In Rules compliance, include a short Feature context checked line listing the references used. If no feature reference applies, state Feature context checked: none.

Step 4: Review logic clarity and complexity​

The review must check whether the implementation logic is straightforward and aligned with existing domain references. Flag code as Warning or Critical when logic is twisted enough to create behavioral risk.

Look for:

  • Branches or boolean conditions that are hard to reason about, duplicated, inverted, or inconsistent with the relevant feature reference.
  • State that is mirrored, synchronized with effects, or transformed through extra aliases instead of derived directly.
  • Service/API flows that obscure the &#123; data, error &#125; contract or make loading/error branches diverge.
  • Helpers that hide simple behavior, add fallback translation wrappers, or replace clear component boundaries with render functions.
  • New abstractions that do not remove real duplication or do not match nearby patterns.
  • File ownership changes that ignore existing feature context, route behavior, permissions, analytics, or coding-standard guidance.

Prefer findings that explain the simpler expected shape, not just "this is complex." A useful finding should say which condition, flow, or abstraction is twisted and what existing pattern/reference it should follow instead.

Part 1: Project Conventions Checklist​

Imports & Path Aliases​

  • Use project path aliases (@/components/*, @/hooks/*, @/contexts/*, @/utility/*, @/services/*, @/modules/*, @/pages/*, @/features/*, @/npl/*, @/middlewares/*, @/types/*) instead of deep relative paths
  • Import order: external packages → internal aliases → relative — sorted consistently
  • No default-exported anonymous components (makes debugging harder in React DevTools)
  • page-structure: new pages, page-local components, shared components, service/API contracts, and shared utilities are placed according to .agents/references/coding-standard/page-structure.md
  • page-structure: the diff shows reasonable reuse/duplication checks before new shared files or page-local helpers were introduced

TypeScript / JavaScript​

  • prefer-typescript-files / clean-typescript: new files use .tsx/.ts; only .js/.jsx where legacy requires it
  • clean-typescript: no any; use unknown and narrow. No unsafe as assertions (no-type-assertion-as.mdc) — fix types at the boundary instead
  • TypeScript interfaces have keys sorted alphabetically (typescript-sort-keys rule is warn) — fix warn-level violations in new code
  • No no-use-before-define violations
  • Unused variables removed (ESLint errors on no-unused-vars)

Localization​

  • no-direct-locale-additions: do not add/edit public/locales/*.json (or CSV locales) in the diff; keys and t('…', &#123; placeholders &#125;) in code must not be paired with unsolicited locale edits

Console & Logging​

  • No console.log — use console.warn, console.error, console.debug, console.info, console.trace, console.group, or console.groupEnd only
  • No sensitive data (tokens, user PII) in any console output

Browser Storage​

  • No direct localStorage / sessionStorage / indexedDB access — the no-storage/no-browser-storage rule is an error; use existing utility wrappers if they exist or justify the bypass clearly

Service layer (@/services/...)​

  • apifox-api-source-of-truth: API endpoints, request/response types, payloads, query params, and integration behavior were checked against Apifox project 8407323 in an authenticated session; if Apifox is inaccessible or missing the schema, flag the API integration as blocked
  • no-try-catch-service-api: calls use &#123; data, error &#125;; if (error) plus showErrorToast / state reset — no try/catch around service calls

React & Hooks​

  • Hooks rules followed: no conditional hooks, no hooks inside loops or nested functions
  • no-functions-in-effect-deps: useEffect dependency arrays include values the effect needs, not function identities from hooks/callbacks; if the linter demands a function in deps and the rule says otherwise, follow the rule and use the documented eslint-disable pattern when appropriate
  • Clean up subscriptions, timers, and event listeners in useEffect return functions when those effects exist
  • no-unnecessary-usecallback: do not add useCallback by default; only when justified for a memoized child or documented effect-deps behavior. Prefer plain handler functions (clean-react.mdc).
  • Prefer deriving state during render over effect-driven syncing when clean-react.mdc / useeffect-guidance applies
  • Keys in lists are stable and unique — never use array index as key in dynamic/reorderable lists

Next.js​

  • Page files end in .page.js/.page.tsx (project convention)
  • API routes are under src/pages/api/
  • NextImage used for images (LCP, CLS benefits) instead of raw <img>
  • Link used for internal navigation instead of <a href>
  • No direct window/document access outside useEffect or typeof window !== 'undefined' guards (SSR safety)

Styling​

  • no-inline-styles: Tailwind utilities (including arbitrary values like w-[60px]) for layout/spacing/color — avoid inline style=&#123;&#123;&#125;&#125; except when explicitly required by dynamic/third-party constraints
  • No hardcoded color hex values inline — use Tailwind tokens or CSS variables from the design system
  • use-classnames-for-conditional-classes: conditional className built with classnames (classNames(...)) not template-literal stitching

Accessibility (jsx-a11y)​

  • Interactive elements (div, span with onClick) have keyboard equivalents or are replaced with semantic elements (button, a)
  • Images have meaningful alt text (empty alt="" is valid for decorative images)
  • Form inputs are associated with labels
  • ARIA attributes are used correctly — role, aria-label, aria-expanded, etc.

Component Library​

  • Try to reuse existing npl components
  • Forward refs correctly when wrapping Radix components

React Hook Form​

  • Validation schemas use @hookform/resolvers (Zod/Yup) instead of inline validate functions for complex forms
  • formState.errors checked and displayed for each field
  • Controlled inputs use Controller or register — not both

Part 2: General React Best Practices​

Component Design​

  • Components have a single, clear responsibility — split if a component is doing too much
  • Props interface is not a dumping ground — if >8 props, consider whether composition or context makes more sense
  • No prop drilling more than 2 levels deep for shared state — use context or a state management pattern
  • children used for layout composition rather than passing render content as props
  • Logic is direct and readable: no inverted condition chains, duplicated branches, unnecessary aliases, or helpers that make behavior harder to trace than the existing local pattern
  • Domain behavior follows the relevant .agents/references/features/*.md file and does not invent a parallel flow for permissions, routing, pricing, analytics, localization, or loading/error handling

Performance​

  • Heavy computations: prefer inline derivation; use useMemo only when profiling or cost is proven (clean-react.mdc)
  • Stable handler references only when no-unnecessary-usecallback allows (memoized child / documented effect constraint) — do not demand useCallback by default
  • Large lists use virtualization (react-window, @hello-pangea/dnd handles its own) if rendering >100 items
  • Lazy-load heavy components or page sections with next/dynamic + ssr: false where appropriate

State Management​

  • Local UI state stays local — don't lift state higher than needed
  • Derived state is computed in the render, not stored in separate useState
  • Avoid stale closures: effects depend on values (no-functions-in-effect-deps); use useRef for latest values when handlers must not widen effect deps arbitrarily

Error Handling​

  • no-try-catch-service-api: for service APIs, errors flow through error checks, not exceptions
  • Non-service async (fetch, third-party SDKs): handle failures explicitly; never swallow errors silently where the user needs feedback
  • User-facing failures use showErrorToast (or equivalent) where the codebase does for service errors

Security​

  • web-security.mdc: XSS, injection, secrets, auth posture, CSP/logging — consistent with that rule file
  • No dangerouslySetInnerHTML without explicit sanitization
  • No secrets or API keys hardcoded — use environment variables (process.env.NEXT_PUBLIC_* for client, plain process.env.* for server)
  • User-supplied content rendered in HTML is sanitized
  • No eval() or dynamic code execution

Part 3: Code Quality​

  • No commented-out code blocks left in (use git history instead)
  • Magic numbers extracted to named constants
  • Logic is scoped to the request — no unrelated drive-by changes
  • Fix addresses root cause, not a symptom
  • Logic is not twisted: branches, derived values, data flow, and helper boundaries should be easy to explain from the surrounding code and relevant feature/coding-standard references

Output Format​

Start with a Rules compliance block (see Step 2): which .mdc rules were considered and whether the diff violates any.

Then categorize findings as:

Critical — Must fix before merge (security issue, runtime bug, broken rule, accessibility blocker)

Warning — Should fix (convention violation, performance risk, maintainability concern)

Suggestion — Nice to have (style, readability, minor improvement)

Also call out What's good — positive patterns worth noting so they get repeated.

## Rules compliance

- Checked: <rule-files>
- Coding standards checked: <coding-standard files or none>
- Feature context checked: <feature reference files or none>
- Violations: none | bullet list referencing rule filenames (e.g. no-try-catch-service-api)

## Critical
- file:line — issue and why it matters

## Warnings
- file:line — issue

## Suggestions
- file:line — suggestion

## What's Good
- pattern — why it's good

If there are no issues in a category, omit that section.

Verification commands​

npx tsc --noEmit                # Type check
npx eslint src/<changed-file>   # Lint check