react-router-code-review

# React Router Code Review

## Quick Reference

| Issue Type | Reference |
|------------|-----------|
| useEffect for data, missing loaders, params | [references/data-loading.md](references/data-loading.md) |
| Form vs useFetcher, action patterns | [references/mutations.md](references/mutations.md) |
| Missing error boundaries, errorElement | [references/error-handling.md](references/error-handling.md) |
| navigate() vs Link, pending states | [references/navigation.md](references/navigation.md) |

## Review Checklist

- [ ] Data loaded via `loader` not `useEffect`
- [ ] Route params accessed type-safely with validation
- [ ] Using `defer()` for parallel data fetching when appropriate
- [ ] Mutations use `<Form>` or `useFetcher` not manual fetch
- [ ] Actions handle both success and error cases
- [ ] Error boundaries with `errorElement` on routes
- [ ] Using `isRouteErrorResponse()` to check error types
- [ ] Navigation uses `<Link>` over `navigate()` where possible
- [ ] Pending states shown via `useNavigation()` or `fetcher.state`
- [ ] No navigation in render (only in effects or handlers)

## Valid Patterns (Do NOT Flag)

These patterns are correct React Router usage - do not report as issues:

- **useEffect for client-only data** - Loaders run server-side; localStorage, window dimensions, and browser APIs must use useEffect
- **navigate() in event handlers** - Link is for declarative navigation; navigate() is correct for imperative navigation in callbacks/handlers
- **Type annotation on loader data** - `useLoaderData<typeof loader>()` is a type annotation, not a type assertion
- **Empty errorElement at route level** - Route may intentionally rely on parent error boundary
- **Form without action prop** - Posts to current URL by convention; explicit action is optional
- **loader returning null** - Valid when data may not exist; null is a legitimate loader return value
- **Using fetcher.data without checking fetcher.state** - May be intentional when stale data is acceptable during revalidation

## Context-Sensitive Rules

Only flag these issues when the specific context applies:

| Issue | Flag ONLY IF |
|-------|--------------|
| Missing loader | Data is available server-side (not client-only) |
| useEffect for data fetching | Data is NOT client-only (localStorage, browser APIs, window size) |
| Missing errorElement | No parent route in the hierarchy has an error boundary |
| navigate() instead of Link | Navigation is NOT triggered by an event handler or conditional logic |

## When to Load References

- Reviewing data fetching code → data-loading.md
- Reviewing forms or mutations → mutations.md
- Reviewing error handling → error-handling.md
- Reviewing navigation logic → navigation.md

## Review Questions

1. Is data loaded in loaders instead of effects?
2. Are mutations using Form/action patterns?
3. Are there error boundaries at appropriate route levels?
4. Is navigation declarative with Link components?
5. Are pending states properly handled?

## Before Submitting Findings

Load and follow [review-verification-protocol](../review-verification-protocol/SKILL.md) before reporting any issue.