Code Review

# Code Review Checklist

Use this checklist when reviewing code changes. Proceed immediately — no confirmation prompt needed.

---

## IMPORTANT: Start Mutation Tests First

**ALWAYS start mutation tests in the background BEFORE reviewing anything else.**

Mutation tests take the longest to run. Start them immediately, then do the rest of the review while they execute:

```bash
# Start mutation tests in background FIRST
npm run test:mutation &

Or use Bash tool with run_in_background: true.

After completing ALL other review steps, check if mutation tests finished:

• If still running, wait in 60-second intervals
• Only proceed to Final Checks after mutation tests complete

---

Tech Debt Tracking

IMPORTANT: Tech debt issues are identified but NOT created during code review. They are created later during /pr-review <n> accept when the PR is approved. This prevents:

• Creating issues for things fixed before PR merge
• Creating issues for rejected PRs
• Noise from premature issue creation

Identifying Tech Debt

During review, track tech debt findings for later creation:

1. Record findings with enough detail to create issues later:

   • File and line number
   • Brief description of the issue
   • Suggested fix
   • Priority level

2. Priority levels:

   • High: Violates coding standards, security issues
   • Medium: ESLint warnings, performance issues
   • Low: Nice-to-have improvements

3. Unique ID format (for deduplication): <filename>-<issue-type>-<line-numbers>

   • Example: luarepl-any-types-26-93-101
   • Example: stryker-exclude-test-files

Output Format for Tech Debt

In the review summary, list tech debt items in a format that can be used by /pr-review accept:

### Tech Debt Identified

| # | File | Issue | Priority |
|---|------|-------|----------|
| 1 | LuaRepl.tsx:42 | Uses `any` type for callback | High |
| 2 | utils.ts:15-20 | Duplicate code with parser.ts | Medium |
| 3 | styles.css | Unused CSS class `.oldHeader` | Low |

**Note**: These will be converted to GitHub issues when the PR is accepted via `/pr-review <n> accept`.

Tech Debt Issue Creation (Deferred to /pr-review accept)

When /pr-review <n> accept is run, it will prompt to create issues for any tech debt items identified:

gh issue create --title "[Tech Debt] <file>: <brief description>" \
  --label "tech-debt" \
  --project "LuaInTheWeb" \
  --body "<!-- tech-debt-id: <unique-id> -->

## Description
<what needs to be fixed>

## Location
- [file.tsx:line](src/path/file.tsx#Lline)

## Suggested Fix
<how to fix it>

## Priority
High|Medium|Low - <reason>

## Found In
PR #<pr-number> - <pr-title>"

DRY Detection

For each new function or significant code block in the diff, use MCP tools to check for duplicates:

mcp__code-index-mcp__search_code_advanced("<function-name-or-key-logic-pattern>")

Flag any existing code that implements similar logic. Common patterns:

• Utility functions that already exist in src/hooks/ or src/utils/
• Duplicated validation logic across components
• Copy-pasted CSS that could use shared variables/modules

Complexity & Duplication Check

After DRY detection, check for complexity and duplication issues:

Complexity Warnings in Changed Files

# Run lint and grep for complexity warnings in changed files
npm run lint 2>&1 | grep -E "complexity|max-lines|max-params|max-depth"

Flag any new complexity warnings introduced by the PR. Existing warnings (244 baseline as of March 2026) are tech debt — only new ones are actionable.

Duplication Check

# Run cross-package duplication detection (threshold: 3%)
npm run duplicates

If this fails, duplication exceeds the 3% ceiling — the PR must reduce duplication before merging.

---

Review Criteria

Review the code against these criteria:

Architecture

• UI components contain NO business logic
• All logic is extracted into custom hooks
• Components are pure (props in, JSX out)
• Related files are co-located in component folders
• Proper separation of concerns

TypeScript

• No any types used
• Explicit return types on public functions
• Props interfaces defined for all components
• Interfaces preferred over type aliases for objects
• Types exported from index.ts

CSS & Styling

• CSS modules used (not inline styles or global CSS)
• Existing styles reused where possible
• CSS variables used for theming values
• No duplicate styles across modules

Testing

• Tests written BEFORE implementation (TDD)
• AAA pattern used (Arrange-Act-Assert)
• Descriptive test names (behavior, not implementation)
• Edge cases covered
• Error cases covered
• Mutation score > 80%
• Tests colocated with components

E2E Testing (for user-facing features)

• E2E tests cover core user flows
• Test page created at /test/<feature> (dev only)
• Tests wait for async operations (Lua engine, Monaco)
• Clear assertions on user-visible outcomes

File Organization

• Component folder structure followed
• types.ts for component-specific types
• index.ts with barrel exports
• Hooks in separate files with tests
• No orphaned files

Error Handling

• Errors handled explicitly
• Error states returned from hooks
• Error boundaries used where appropriate
• User-friendly error messages

Performance

• useMemo/useCallback used appropriately (not overused)
• No unnecessary re-renders
• Large lists virtualized if needed
• Lazy loading for heavy components

Accessibility

• Semantic HTML used
• Interactive elements keyboard accessible
• ARIA attributes added where needed
• Focus management handled

Security

• No user input rendered as HTML without sanitization
• No secrets in code
• External URLs validated
• Input validation present