code-review-checklist

Code Review Checklist

Apply each section systematically to every file changed in the review scope. Use severity levels: Critical (must fix), Warning (should fix), Suggestion (consider for improvement).

1. Security

• No hardcoded secrets, API keys, or credentials in code
• All user input validated server-side (Zod schemas on API routes)
• SQL injection prevented (Prisma parameterized queries only, no raw SQL)
• XSS prevented (no dangerouslySetInnerHTML without sanitization)
• CSRF protection on all state-changing endpoints
• Authentication checked on all protected API routes
• Authorization verified (users can only access their own data)
• Rate limiting considered for public-facing endpoints
• Sensitive data not logged or exposed in error messages
• File uploads validated (type, size) if applicable
• Financial/sensitive calculations done server-side (never trust client-side amounts)

2. Performance

• No N+1 database queries (use Prisma include / select appropriately)
• Large lists use pagination (cursor-based preferred for large datasets)
• Images use next/image with proper width/height and priority flags
• Heavy computations not in the render path
• Appropriate use of React.memo, useMemo, useCallback (not excessive)
• Server Components used where possible (minimize 'use client' directives)
• Database queries have appropriate indexes (check prisma/schema.prisma)
• No blocking operations in API route handlers
• Static data uses ISR or SSG where appropriate
• Bundle size impact considered (no unnecessary large dependencies)

3. TypeScript Standards

• No any types (use unknown if type is truly unknown)
• All function parameters and return types are explicit
• Shared types live in src/types/, not duplicated across files
• Discriminated unions used for state management (not boolean flags)
• Enums avoided in favor of const objects or literal union types
• Strict null checks handled (no non-null assertions ! without justification)
• Generic types used when appropriate for reusability

4. NextJS App Router Patterns

• File conventions followed: page.tsx, layout.tsx, route.ts, loading.tsx, error.tsx
• Loading and error states implemented for async pages
• metadata or generateMetadata exported for SEO on public pages
• Server Actions used appropriately (not for read operations)
• Dynamic vs static routes chosen correctly
• process.env only accessed in server-side code
• Route handlers use proper HTTP methods and status codes
• Middleware used sparingly and for cross-cutting concerns only

5. Accessibility

• All interactive elements are keyboard-navigable
• Images have meaningful alt text (descriptive, not generic)
• Form fields have associated <label> elements
• Color is not the only indicator of state (e.g., status indicators)
• ARIA attributes used correctly where needed
• Focus management works after dynamic content changes
• Dynamic content changes announced to screen readers (aria-live regions)

6. Error Handling

• API routes return consistent error shapes: { error: string, code: string }
• Frontend displays user-friendly error messages (no raw error dumps)
• Network errors handled gracefully (retry, fallback, or clear message)
• Loading states prevent duplicate submissions (especially for critical actions)
• Error boundaries catch component-level failures with fallback UI
• Failed database operations are rolled back properly
• Validation errors return 400 with specific field-level feedback

7. Test Coverage

• New features have corresponding unit tests
• API endpoints have integration tests (request → response validation)
• Edge cases tested: empty arrays, null values, boundary values
• Error paths tested: invalid input, unauthorized access, not found
• Core user flows have E2E coverage (critical paths through the application)
• Test names clearly describe the scenario being tested
• Mocks are minimal and realistic (not over-mocked)
• Test data uses factories/fixtures, not inline magic values

8. Domain Boundary Compliance

• Backend-dev only modified files in: src/app/api/, src/services/, src/lib/server/, prisma/
• Web-dev only modified files in: src/app/ (non-api), src/components/, src/hooks/, src/lib/client/, src/styles/
• Mobile-dev only modified files in: lib/, android/, ios/, test/, integration_test/, pubspec.yaml
• Test-worker only modified files in: tests/, __tests__/, **/*.test.*, **/*.spec.*
• Shared files (src/types/, package.json, tsconfig.json) only modified by team-lead
• No worker modified files outside their assigned domain

9. Mobile (Flutter/Dart) Standards

When reviewing mobile-dev branches:

• const constructors used for stateless widgets
• No widget building inside build() method (extract to methods/widgets)
• Riverpod providers use autoDispose when appropriate
• ref.watch used in build, ref.read used in callbacks (not reversed)
• GoRouter routes properly defined with type-safe parameters
• No setState for app-wide state (use Riverpod)
• Dart analysis passes (dart analyze clean)
• Platform-specific code properly isolated (no direct Platform checks in widgets)
• Responsive layout handling (LayoutBuilder/MediaQuery)
• Proper key usage in lists (ValueKey)

Review Output Template

## Review: [branch-name]

### Summary
[1-2 sentence overview]

### [filename]
- **Critical**: [issue with line reference]
- **Warning**: [concern with explanation]
- **Suggestion**: [improvement idea]

### Cross-Cutting Concerns
- API contract consistency: [pass/fail with details]
- Domain boundary compliance: [pass/fail with details]
- Test coverage: [adequate/gaps identified]

### Verdict: [APPROVE | REQUEST_CHANGES | NEEDS_DISCUSSION]
[Reasoning for verdict]