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.