# Review current branch changes
/code-review

# Review specific files
/code-review src/components/CartSummary.tsx

# Review with specific instructions
/code-review focus on memoization issues in the dashboard widgets

// BAD: Business logic and network calls in render
export const CheckoutSummary = ({ cart }: { cart: LineItem[] }) => {
  const total = cart.reduce((sum, item) => sum + item.price * item.qty, 0);
  if (total > 1000) {
    analytics.track('whale'); // fires on every render
  }
  fetch('/api/checkout/fees'); // runs during render
  return <span>{formatCurrency(total)}</span>;
};

// BAD: Prop drilling through every layout layer
<App user={user}>
  <Layout user={user}>
    <Sidebar user={user}>
      <Nav user={user} />
    </Sidebar>
  </Layout>
</App>;

// GOOD: Extract domain logic into a hook
function useCheckoutTotal(cart: LineItem[]): number {
  return useMemo(
    () => cart.reduce((sum, item) => sum + item.price * item.qty, 0),
    [cart]
  );
}

export const CheckoutSummary = () => {
  const cart = useCartContext();
  const total = useCheckoutTotal(cart);
  useEffect(() => {
    if (total > 1000) analytics.track('whale');
  }, [total]);
  return <Amount value={total} />;
};

// GOOD: Replace prop drilling with context/provider
const UserProvider = ({ user, children }: PropsWithChildren<{ user: User }>) => (
  <UserContext.Provider value={user}>{children}</UserContext.Provider>
);

// BAD: any everywhere
export function buildPayload(data: any) {
  return data['value'] || 'default';
}

// BAD: Missing return type on exported hook
export const usePrice = (sku) => {
  const [price, setPrice] = useState(0);
  /* ... */
  return price;
};

// BAD: Magic values sprinkled inline
if (plan === 'enterprise') {
  setTimeout(startTrial, 45000);
}

// BAD: Barrel export cycle
// src/features/index.ts
export * from './FeatureProvider';
export * from './useFeature';
// src/features/useFeature.ts
import { FeatureProvider } from '.'; // circular import

// GOOD: Narrow types and explicit returns
export interface CheckoutPayload {
  sku: string;
  quantity: number;
}
export function buildPayload(data: CheckoutPayload): string {
  return JSON.stringify(data);
}

export const usePrice = (sku: string): number => {
  const [price, setPrice] = useState<number>(0);
  /* ... */
  return price;
};

// GOOD: Shared enums/constants and explicit exports
export enum PlanTier {
  Starter = 'starter',
  Enterprise = 'enterprise',
}
export const REQUEST_TIMEOUT_MS = 30_000;
export { FeatureProvider } from './FeatureProvider';
export { useFeature } from './useFeature';

// BAD: Copy/paste tests
it('renders primary button', () => {
  render(<Button variant="primary" />);
  expect(screen.getByText('Save')).toHaveClass('btn--primary');
});
it('renders secondary button', () => {
  render(<Button variant="secondary" />);
  expect(screen.getByText('Cancel')).toHaveClass('btn--secondary');
});

// BAD: Implementation detail assertion
const wrapper = shallow(<Toggle />);
expect(wrapper.find('.toggle__thumb').prop('data-state')).toBe('on');

// BAD: Missing await for async user interaction
userEvent.click(screen.getByRole('button', { name: /submit/i }));
expect(mockSubmit).toHaveBeenCalled(); // may run before promise resolves

// GOOD: Parameterized, user-facing assertions
it.each`
  variant        | label
  ${'primary'}   | ${'Save'}
  ${'secondary'} | ${'Cancel'}
`('renders $variant button', ({ variant, label }) => {
  render(<Button variant={variant}>{label}</Button>);
  expect(screen.getByRole('button', { name: label })).toBeEnabled();
});

// GOOD: Focus on behavior, await async APIs
await userEvent.click(screen.getByRole('button', { name: /submit/i }));
await waitFor(() => expect(mockSubmit).toHaveBeenCalled());

// GOOD: Cleanup timers/mocks
afterEach(() => {
  vi.useRealTimers();
  vi.restoreAllMocks();
});

// BAD: Unsanitized HTML injection
const Comment = ({ html }: { html: string }) => (
  <div dangerouslySetInnerHTML={{ __html: html }} />
);

// BAD: Shipping secrets
export const config = {
  mapsApiKey: 'pk_live_abc123',
};

// BAD: Storing sensitive tokens client-side
localStorage.setItem('refreshToken', token);

// BAD: Prototype pollution
const payload = { ...userInput };
Object.assign(globalTheme, payload);

// GOOD: Sanitize or render as text
const sanitized = DOMPurify.sanitize(html);
return <div dangerouslySetInnerHTML={{ __html: sanitized }} />;

// GOOD: Secrets fetched from server-only env vars
const { data } = await fetch('/api/config');

// GOOD: Keep sensitive data in httpOnly cookies; store opaque ids only
document.cookie = `session=${sessionId}; Secure; SameSite=Lax`;

// GOOD: Validate and whitelist keys before merging
const safe = pick(userInput, ['theme', 'density']);
const merged = { ...defaultTheme, ...safe };

// BAD: Inline functions + no memoization
export const ExpensiveList = ({ items }: { items: Item[] }) => (
  <div>
    {items.map((item) => (
      <Row key={item.id} item={item} onSelect={() => select(item.id)} />
    ))}
  </div>
);

// BAD: Giant list rendered all at once
{
  users.map((user) => <UserRow key={user.id} user={user} />);
}

// BAD: Unbounded imports
import _ from 'lodash';
const formatted = _.startCase(name);

// BAD: Missing cleanup
useEffect(() => {
  window.addEventListener('resize', onResize);
}, []); // never removed

// GOOD: Memo + stable callbacks
const Row = React.memo(({ item, onSelect }: RowProps) => {
  /* ... */
});
const handleSelect = useCallback((id: string) => select(id), [select]);

export const ExpensiveList = ({ items }: Props) => (
  <div>
    {items.map((item) => (
      <Row key={item.id} item={item} onSelect={handleSelect} />
    ))}
  </div>
);

// GOOD: Virtualization & focused imports
import startCase from 'lodash/startCase';
const rows = useVirtualizer({ count: users.length, estimateSize: () => 48 });

// GOOD: Cleanup effects
useEffect(() => {
  window.addEventListener('resize', onResize);
  return () => window.removeEventListener('resize', onResize);
}, [onResize]);

// BAD: Conditional hook
if (isAdmin) {
  const data = useDashboardData();
}

// BAD: Missing deps leading to stale closure
useEffect(() => {
  fetchData(filter);
}, []); // ignores filter updates

// BAD: Controlled/uncontrolled mix
<input defaultValue={value} value={value} onChange={setValue} />;

// BAD: Index as key
{
  items.map((item, index) => <Row key={index} item={item} />);
}

// GOOD: Hooks called unconditionally
const data = useDashboardData({ role: isAdmin ? 'admin' : 'user' });

// GOOD: Stable dependencies or refs
const stableFilter = useDeepCompareMemoize(filter);
useEffect(() => {
  fetchData(stableFilter);
}, [stableFilter]);

// GOOD: Controlled OR uncontrolled, not both
const [value, setValue] = useState('');
<input value={value} onChange={(e) => setValue(e.target.value)} />;

// GOOD: Deterministic keys & proper refs
{
  items.map((item) => <Row key={item.id} item={item} />);
}

const Input = forwardRef<HTMLInputElement, InputProps>((props, ref) => (
  <input ref={ref} {...props} />
));

<ErrorBoundary fallback={<CrashScreen />}>
  <CriticalPanel />
</ErrorBoundary>;

// BAD: Conflates duplicate purchase with missing customer
export async function activatePremium(
  orderId: string,
  customerId: string
): Promise<boolean> {
  const response = await fetch(`/api/customers/${customerId}/activate`, {
    method: 'POST',
    body: JSON.stringify({ orderId }),
  });
  if (!response.ok) {
    return false; // could be 404, 409, or 500
  }
  return true;
}

if (!(await activatePremium(orderId, user.id))) {
  // UI silently ignores: no refund, no error state
}

// GOOD: Distinguish each case explicitly
export type ActivationResult =
  | { status: 'success' }
  | { status: 'duplicate'; orderId: string }
  | { status: 'user-missing'; customerId: string }
  | { status: 'error'; message: string };

export async function activatePremium(
  orderId: string,
  customerId: string
): Promise<ActivationResult> {
  const response = await fetch(`/api/customers/${customerId}/activate`, {
    method: 'POST',
    body: JSON.stringify({ orderId }),
  });
  if (response.status === 409) return { status: 'duplicate', orderId };
  if (response.status === 404) return { status: 'user-missing', customerId };
  if (!response.ok) return { status: 'error', message: await response.text() };
  return { status: 'success' };
}

// BAD: setState after unmount
useEffect(() => {
  let active = true;
  fetchUser().then(user => {
    if (active) setUser(user);
  });
}, []); // never flips active, setState after unmount

// BAD: No abort controller
useEffect(() => {
  fetch(`/api/search?q=${query}`).then(setResults);
}, [query]);

// BAD: Concurrent updates using stale value
onScroll={() => setOffset(offset + 1)};

// GOOD: Abort + cleanup
useEffect(() => {
  const controller = new AbortController();
  fetch(`/api/search?q=${query}`, { signal: controller.signal })
    .then(resp => resp.json())
    .then(setResults)
    .catch(err => {
      if (err.name !== 'AbortError') throw err;
    });
  return () => controller.abort();
}, [query]);

// GOOD: Functional updates prevent stale closures
onScroll={() => setOffset(prev => prev + 1)};

// GOOD: Dexie liveQuery cleanup
useEffect(() => {
  const sub = liveQuery(() => db.todos.where({ status }).toArray()).subscribe(setTodos);
  return () => sub.unsubscribe();
}, [status]);

// BAD: Window listener leaks
useEffect(() => {
  window.addEventListener('resize', onResize);
}, []);

// BAD: Interval never cleared
useEffect(() => {
  const id = setInterval(poll, 1000);
}, []);

// BAD: Firestore subscription persists
useEffect(() => {
  firebase.firestore().collection('rooms').onSnapshot(setRooms);
}, []);

// GOOD: Cleanup everything
useEffect(() => {
  window.addEventListener('resize', onResize);
  return () => window.removeEventListener('resize', onResize);
}, [onResize]);

useEffect(() => {
  const id = setInterval(poll, 1000);
  return () => clearInterval(id);
}, [poll]);

useEffect(() => {
  const unsubscribe = firebase.firestore().collection('rooms').onSnapshot(setRooms);
  return () => unsubscribe();
}, []);

// BAD: Unbounded fetch
const promise = fetch('/api/report'); // never canceled if user navigates away

// BAD: Worker leak
const worker = new Worker(new URL('./heavy.ts', import.meta.url));
worker.postMessage(payload); // no termination

// BAD: BroadcastChannel leak
const channel = new BroadcastChannel('notifications');
channel.onmessage = handleMessage;

// GOOD: Abort fetch and clear references
const controller = new AbortController();
fetch('/api/report', { signal: controller.signal });
return () => controller.abort();

// GOOD: Terminate workers/broadcast channels
useEffect(() => {
  const worker = new Worker(new URL('./heavy.ts', import.meta.url));
  worker.postMessage(payload);
  return () => worker.terminate();
}, [payload]);

useEffect(() => {
  const channel = new BroadcastChannel('notifications');
  channel.onmessage = handleMessage;
  return () => channel.close();
}, []);

// GOOD: Guard service worker registration
navigator.serviceWorker.register('/sw.js').then((reg) => {
  reg.update();
});

// BAD: Unbounded query pulling entire table
const allLogs = await db.logs.toArray();

// BAD: Missing index and filtering client-side
const tasks = await db.tasks.toArray();
const open = tasks.filter((t) => t.status === 'open');

// BAD: Migration ignored
db.version(3).stores({ tasks: '++id' }); // new fields but no upgrade handling

// BAD: No quota handling
await db.files.add(blob); // QuotaExceededError crashes app

// GOOD: Bounded queries + indexes
db.version(4).stores({ tasks: '++id,status,assigneeId' });
const open = await db.tasks.where('status').equals('open').limit(100).toArray();

// GOOD: Upgrade hook
db.version(4)
  .stores({ tasks: '++id,status,assigneeId' })
  .upgrade((tx) =>
    tx
      .table('tasks')
      .toCollection()
      .modify((task) => (task.slug ??= slugify(task.title)))
  );

// GOOD: Graceful quota handling
try {
  await db.files.add(blob);
} catch (error) {
  if ((error as DOMException).name === 'QuotaExceededError') {
    await evictOldFiles();
    throw new Error('Storage full, try again after clearing space');
  }
  throw error;
}

// BAD: Mixing local and UTC
const createdAt = new Date(); // local
const expiresAt = new Date(Date.now() + 24 * 60 * 60 * 1000); // treated as UTC elsewhere

// BAD: Hardcoded timezone adjustments
const est = utcDate.getTime() - 4 * 60 * 60 * 1000;

// BAD: Direct Date.now in business logic
if (Date.now() > subscription.expiresAt) {
  cancel();
}

// GOOD: Always use ISO strings + DateTime libraries
const now = Temporal.Now.instant();
const expiresAt = now.add({ days: 1 });

// GOOD: Convert with Intl/Temporal APIs
const nyTime = now.toZonedDateTimeISO('America/New_York');

// GOOD: Inject clock during tests
const clock = useClock();
if (clock.now() > subscription.expiresAt) {
  cancel();
}

// BAD: Breaking prop rename without alias
export interface ButtonProps {
  onSubmit: () => void; // renamed from onClick without backwards compat
}

// BAD: Required prop treated as optional
const Avatar = ({ src }: { src?: string }) => <img src={src} />; // crashes when undefined

// BAD: No error boundary
<RemoteWidget />;

// BAD: Console noise in prod
console.log('Render metrics', data);

// GOOD: Provide shims/deprecations
export interface ButtonProps {
  onClick: () => void;
  /** @deprecated use onClick */
  onSubmit?: () => void;
}

// GOOD: Validate props via runtime guards or defaults
const Avatar: FC<{ src: string }> = ({ src, ...rest }) => (
  <img src={src ?? fallbackAvatar} {...rest} />
);

// GOOD: Wrap third-party components
<ErrorBoundary fallback={<WidgetCrashed />}>
  <RemoteWidget />
</ErrorBoundary>;

// GOOD: Structured monitoring
console.debug = noop;
reportErrorToSentry(error, context);

// BAD: Exported hook undocumented
export function usePaymentsGateway() {
  /* ... */
}

// GOOD: TSDoc explains behavior and edge cases
/**
 * usePaymentsGateway pulls the current PSP config, refreshes every 5 minutes,
 * and throws when credentials are missing. Consumers must catch errors.
 */
export function usePaymentsGateway(): PaymentsGatewayState {
  /* ... */
}

.md/issues/{issue-number}/code-review.md
.md/issues/{issue-number}/code-review-round-2.md  # subsequent rounds

.md/standalone/code-review-{branch-name}.md
.md/standalone/code-review-{date}.md  # if no branch context

# Code Review

**Context**: [Branch: feature/xyz] OR [Issue #123: Description] OR [Brief description of changes]  
**Date**: YYYY-MM-DD HH:MM

---

## Critical Issues

[Security vulnerabilities, data loss risks, crash scenarios]

## Component Architecture

[Hook discipline, prop boundaries, component responsibilities]

## React Performance & Async State

[Memoization, virtualization, fetch cancellation, concurrency issues]

## React Lifecycle & Resources

[Effect cleanup, timers, workers, subscriptions, browser resources]

## Return Value Semantics

[Ambiguous returns, silent failures, conflated error cases]

## Client Data Access

[IndexedDB/Dexie usage, schema, migrations, quota handling]

## Props API & Monitoring

[Contract issues, monitoring gaps, error boundaries, logging]

## Code Quality & Tests

[Type safety, code smells, Jest/RTL issues]

## Documentation Gaps

[Missing updates to README, Storybook, MDX, ADRs]

## Verdict

[APPROVE / REQUEST CHANGES / BLOCK MERGE]

---

**Model**: [e.g., claude-sonnet-4-5, claude-opus-4-6]
**Persona**: [e.g., Sisyphus-Junior, Oracle]

# After making changes for issue #123
/code-review

# The agent will:
# 1. git diff master...HEAD to see all changes
# 2. Detect issue context (from branch name feat/123-description or commits)
# 3. Read modified files (components, hooks, tests, Dexie stores, workers)
# 4. Analyze against React/TypeScript best practices (including memoization, hook rules, Dexie bounds)
# 5. Write review to .md/issues/123/code-review.md