bytefork

Code Review & AI-Generated Code

Roughly 13% of a typical loop — and 13% of the mock exam here.

The review method

A code-review round is a rubric of independent criteria — correctness, rendering, state, accessibility, UI/UX, performance hygiene, style, deliverable, instruction-following, edge cases — and each row is scored on its own evidence. A polished layout can still ship onClick={handleClick()}, which fires during render instead of on click, so never let a strong row pull up a weak one. For every finding, quote the exact offending line, then give the smallest rewrite that fixes it: a one-token change beats a rewritten component that hides which change was the fix. Read the complete code before scoring; the defect is usually in the last twenty lines.

State and rendering defects

Mutation: state.items.push(x); setState(state) changes the object React already holds, and Object.is sees the same reference, so React skips the re-render; the fix is a copy, setState({ ...state, items: [...state.items, x] }). Stale closures: setCount(count + 1) inside an interval reads the captured value; the rewrite is setCount(c => c + 1). Derived state: a useEffect that calls setFullName(first + ' ' + last) is redundant state plus a cascading render; compute it during render. Conditional hooks: a useState below an early return changes the call order between renders; hoist it above the guard. Effects without cleanup: setInterval and addEventListener must return the matching clearInterval or removeEventListener, and dev Strict Mode’s extra mount-unmount cycle surfaces the leak. Keys: key={index} on a reorderable list and key={Math.random()} both break identity, the second recreating every DOM node each render; use the data’s stable id. And the 0-leak:

// offending line
{items.length && <List items={items} />}
// minimal rewrite
{items.length > 0 && <List items={items} />}

0 is falsy but renderable, so the first version prints a literal 0 for an empty list.

Accessibility and UI defects

Rate the DOM the JSX produces, not the intent. <img src={url} /> without alt fails WCAG 1.1.1; decorative images take alt="". <input placeholder="Email" /> borrows its only name from a placeholder that vanishes on the first keystroke, so add <label htmlFor="email"> with a matching id. A <div onClick> is not focusable and has no role; the first rule of ARIA is to use the native element when one exists, so the rewrite is <button type="button">, type spelled out because a button inside a form defaults to submit. ARIA roles add no keyboard behaviour, so role="button" on a div without tabIndex and Enter/Space handling is a promise the element does not keep. Check that the focus ring is not erased with outline: none (2.4.7) and that state is not carried by colour alone (1.4.1): a red border on an invalid field needs a text message too. Narrow the viewport to 320 CSS px — fixed widths that force horizontal scroll fail 1.4.10 Reflow — and check pointer targets clear 24 by 24 CSS px (2.5.8).

Deliverable and instruction defects

When the brief asks for a runnable single-file component, the deliverable row is mechanical: one file, imports at the top, exactly one export default, no import of a ./components/Card that was never written. A response whose code is npm install react-router-dom followed by prose is not a deliverable. Faked output — a console log the code never emits, an // API call here where the fetch belongs — scores as missing, not partial. Diff the brief against the code: every omitted requirement is a finding, and every unrequested feature is one too. Edge cases get their own row: empty list, loading and error states, a fetch resolving after unmount, props arriving undefined.

Comparing two responses

A side-by-side is decided per criterion with evidence, not by overall preference. “A feels cleaner” is not a reason; “A returns cleanup from its useEffect at line 22, B leaks an interval” is. Correctness outranks style when the rubric is silent on weighting, and if both fail the same row, say so.

Writing the justification

One sentence per finding, carrying the line quote, the rule it breaks and the fix: “Line 14 setItems(items.push(x)) mutates state and stores a number; use setItems([...items, x]).” Keep the tone neutral — no “the model clearly”, no praise padding — and write nothing a second reviewer could not verify by opening the file. Order findings by severity, state each criterion’s score, and stop.

Sample questions

6 of the 30 questions this domain carries in practice mode — expand one to check yourself before drilling.

1. A generated TodoList click handler runs todos.push(next) and then setTodos(todos). The reviewer should flag this line as a rendering bug because:
  1. push returns the new length rather than the array, so the list becomes a number and the next map call throws
  2. arrays held in state are frozen by React in Strict Mode, so push throws a TypeError during development
  3. setTodos receives the same array reference, Object.is judges it unchanged, and React skips the re-render
  4. the mutation runs outside a transition, so React batches it away with the other updates from the click

Answer: C. React bails out when the next state is Object.is-equal to the current one; worse, the mutated array silently appears whenever an unrelated update re-renders. Fix: setTodos([...todos, next]).

2. A generated Inbox renders unread && <MessageList/> where unread is a number. QA reports a stray 0 on screen when the inbox is empty. The minimal rewrite is:
  1. make the left side a boolean, unread > 0 && <MessageList/>, so && yields false instead of 0
  2. wrap the count in a Fragment so React treats the number as a JSX hole rather than as a text node
  3. move the check into a useEffect that sets a showList flag in state before the list is allowed to mount
  4. switch to unread || <MessageList/> so a zero count falls through to the right-hand branch instead

Answer: A. JavaScript && returns its left operand when falsy, and React prints 0 as text; false, null, undefined and true are holes that render nothing.

3. A generated Timer uses useEffect(() => { setInterval(() => setSeconds(seconds + 1), 1000) }, []). It ticks to 1 and stops. The smallest fix that also satisfies the deps lint is:
  1. add seconds to the dependency array so the effect re-subscribes with a fresh closure every second
  2. switch to the updater form setSeconds(s => s + 1) so the interval never reads the stale seconds binding
  3. mirror seconds into a ref and read ref.current inside the callback, since a ref always holds the latest value
  4. suppress the exhaustive-deps warning, because an interval that starts once is exactly what [] means

Answer: B. The [] effect captures the initial seconds forever; an updater reads the live state instead. Also flag the missing clearInterval cleanup, which the Strict Mode double mount exposes.

4. A generated PriceSummary receives items as a prop, keeps total in useState, and syncs it with useEffect(() => setTotal(sum(items)), [items]). The reviewer should recommend:
  1. delete the state and the effect and compute const total = sum(items) during render
  2. keep the effect but switch to useLayoutEffect, which runs before render so total is already correct on the first pass
  3. initialise the state as useState(sum(items)), since the initializer re-runs whenever the items prop changes
  4. wrap the effect body in startTransition so the second render caused by setTotal is deprioritised

Answer: A. Derived data belongs in render, not state; the effect version paints a stale total and then cascades a second render. Reserve useEffect for syncing with external systems.

5. A parent renders a memo-wrapped Row as <Row style={{ padding: 8 }} label={label}/> and computes label with useMemo(() => first + ' ' + last, [first, last]). Row re-renders on every parent render. The reviewer's correct pair of ratings is:
  1. the useMemo is the defect because its dependency array rebuilds label on every render, while the inline style object is harmless because memo deep-compares object props by value
  2. both are correctness defects: memo needs every prop memoised, so the style object and the label string must each be wrapped in useMemo
  3. the inline style object is the rendering defect, since memo compares props with Object.is and a fresh object never matches; the useMemo on a string is merely unnecessary
  4. neither is a defect: memo bails out whenever the label string is unchanged, so the review note should be limited to naming the style constant

Answer: C. Strings are Object.is-equal by value, so the useMemo buys nothing; hoist the style to a module constant or useMemo it. Rate separately: a performance-hygiene miss and a readability note.

6. A generated UserCard effect: useEffect(() => { load(userId).then(setUser); logRef.current.push(userId) }, []) where logRef comes from useRef. The linter warns once. The reviewer's correct dependency verdict is:
  1. all three of userId, setUser and logRef must be listed, because every variable declared inside the component body counts as reactive by definition
  2. add userId and setUser; logRef stays out because reading a ref inside an effect is itself a bug the reviewer should flag first
  3. keep [] and add an eslint-disable comment, since a card should load its user once on mount and the warning is a false positive
  4. add userId only; setUser and the logRef object are stable identities the linter already exempts, so listing them would be harmless noise

Answer: D. Props and body variables are reactive; set functions, dispatch and ref objects are guaranteed stable. The ref's current value is mutable, so it can never be a dependency either.

Drill this domain in practice mode →

Independent community study resource — not affiliated with or endorsed by Meta Platforms, Inc. React is a trademark of Meta Platforms, Inc. All questions and study notes are original, written from the official React documentation. Everything runs in your browser; nothing you answer is stored or transmitted.