code-review-web

# Code Review for Web

Review and debug web application code with a focus on the patterns that actually break production. Stack-agnostic principles in SKILL.md. Stack-specific patterns in references.

---

## When to use

- Reviewing a pull request before merging
- Debugging a production issue
- Investigating a build failure
- Auditing security or performance of existing code
- Investigating environment variable or configuration issues
- Triaging a "the site is broken" report

## When NOT to use

- Writing a new feature spec (use `pm-spec-writing`)
- Pre-launch QA against the running site (use `qa-testing`)
- Performance deep-dive on Core Web Vitals (use `performance-optimization`)
- Deep accessibility compliance review (use `accessibility-audit`)

---

## Required inputs

- The code, PR, error message, or symptom under review
- Access to logs (build logs, function logs, server logs) if debugging
- The stack (framework, hosting, database) - even at high level

If just a symptom is provided ("the site is broken"), the workflow's first step is gathering enough context to investigate.

---

## The framework: 5 review dimensions

Every code review covers five dimensions. Pick the depth based on the situation.

### 1. Correctness

Does the code do what it claims to do?

- Logic matches the intent stated in the spec or PR description
- Edge cases handled (empty states, error states, network failures)
- Off-by-one errors, null/undefined handling, async race conditions
- Tests exist for the change, or there's a reason they don't
- The change does not break existing functionality (regression risk)

### 2. Security

Does the code expose anything sensitive or open an attack surface?

- **Secrets handling.** No secrets, API keys, or service-role credentials in client-side code or version control
- **Auth checks.** Every mutation endpoint validates the caller before acting
- **Input validation.** User input sanitized before use in queries, file paths, or HTML
- **External requests.** Outbound URLs validated; no SSRF on user-controlled inputs
- **CSRF protection.** State-changing requests require a token or same-origin policy
- **Rate limiting.** Public-facing mutation endpoints have rate limits
- **HTTPS-only.** No HTTP in production code paths
- **Cookies.** Session cookies have `Secure`, `HttpOnly`, `SameSite` attributes set
- **Environment variables.** Server-only secrets are not prefixed with anything that exposes them to the client bundle

### 3. Performance

Will this code scale and stay fast?

- **Database queries.** No N+1 patterns. Joins or batch fetches preferred over loops with queries.
- **Pagination.** Large result sets paginated, never loaded entirely.
- **Caching.** Appropriate cache strategy for the data freshness needs.
- **Bundle size.** Client-side dependencies justified. Tree-shaking working.
- **Image handling.** Modern formats, lazy loading, explicit dimensions.
- **Background work.** Slow operations moved off the request path.
- **Cold start sensitivity.** Cold paths optimized if frequently triggered.

### 4. Reliability

What happens when this fails?

- **Error handling.** Caught and handled, not swallowed. Errors logged with context.
- **Retries.** Network calls have retry logic for transient failures.
- **Timeouts.** External calls have explicit timeouts (no infinite waits).
- **Graceful degradation.** Failure of non-critical paths does not crash the page.
- **Idempotency.** Mutations that might be retried are safe to retry.
- **Logging.** Enough context in logs to diagnose without reproducing.

### 5. Maintainability

Will the next person (or future you) understand this in six months?

- **Naming.** Functions and variables named for what they do, not how.
- **Comments.** Explain why, not what. The code says what.
- **Complexity.** Functions do one thing. If a function takes 200 lines, it's doing too much.
- **Duplication.** Same logic in multiple places gets extracted.
- **Dependencies.** New dependencies justified. Each one is a maintenance burden.
- **Magic values.** No literal `60000` in code. Use named constants.

---

## Common bug patterns (stack-agnostic)

Patterns that recur across stacks and are worth checking on every review.

### Build and deploy

- **Build-time data fetches that timeout.** Routes that query a database during static generation can fail at scale. Mark them as runtime-rendered if the data must be fresh.
- **Environment variables not propagating.** A var that works locally but breaks in production usually means it was not added to the production environment.
- **Mismatched env between preview and production.** Deploys that work in preview but break on the production domain often have stack-specific URLs hardcoded.

### URL and domain issues

- **Canonical pointing at staging or preview URL.** Caused by client-exposed environment variables that pick up the wrong domain. Canonical domain should come from a server-only environment variable.
- **API URL pointing at the main domain that loops back.** After a DNS cutover, the main domain may now serve a different application. APIs should live on dedicated subdomains.
- **HTTPS upgrade issues.** Mixed content (HTTP resources loaded into HTTPS pages) breaks browsers' security model.

### Cache invalidation

- **Stale content after deploy.** Either the cache was not invalidated, or the invalidation requires a manual trigger that did not run.
- **CDN serving old asset under same filename.** Always use a new filename or cache-bust query string when replacing assets.
- **Cache headers too aggressive.** Long max-age on resources that change frequently leads to users seeing stale content for hours or days.

### Database and data

- **N+1 queries.** Loop with a query inside the loop. Replace with batch fetch or join.
- **Missing limits on table scans.** Forgetting `LIMIT` on queries that hit large tables.
- **Connection pool exhaustion.** Too many concurrent connections, often from build-time fetches in parallel