2026-09-05 · 8 min read

React code review exercise: would you approve this search component?

I have seen this bug report on more than one team and it plays out roughly the same way every time: reviewers flag a "race condition" in a search component that isn't there. The component uses an AbortController cleanup in a useEffect, which is exactly the standard fix for that race condition, but nobody traces the code far enough to see the guard after the await. They see async plus state updates, pattern-match to "race condition," and request changes. The author, not yet sure of their footing, doesn't push back. Days of calendar time, a couple rounds of review comments, zero bugs found, because there were zero bugs to find.

That experience is what this exercise is about. It is a React code review walkthrough with a worked solution, but the twist is that the code is correct: a search component with debounce, AbortController, and a signal guard that handles every edge case in the spec. The exercise is arriving at a confident approve and being able to explain why, requirement by requirement. Most code review practice trains you to reject. Knowing when to approve, with reasons, is a separate skill, and if you are preparing for a code review interview or trying to stop rubber-stamping PRs in one direction or the other, it is arguably the harder one.

The cost of a false reject is quieter than the cost of a missed bug, but it compounds. A 2023 study on code review speed found that review latency is one of the strongest predictors of developer satisfaction and team throughput. Every unnecessary round-trip is a context switch for the author and a credibility loss for the reviewer. Teams that cry "race condition" on correct code stop getting listened to when they flag real problems later. Precision matters on both sides of the verdict.

The requirements

The task: add live search to a product catalog. Here is what the spec asks for:

As the user types, query the search API and render the results.
Debounce requests by 300 ms. No request fires while the user is still typing.
A stale response must never overwrite the results of a newer query, regardless of network timing.
An empty or whitespace-only search box shows no results and makes no request.
While the user is typing a new query or its request is loading, keep the previous results visible. Replace them when the new response arrives.
Handle loading and error states, announced to assistive technology. The API function searchProducts(query, { signal }) resolves with an array of products with unique, stable ids, supports cancellation via AbortSignal, and rejects when cancelled.

The PR

This PR was generated by Claude Fable 5. One file, one component, about 50 lines:

ai-agent wants to merge · src/components/ProductSearch.jsx +50

1+ import { useEffect, useState } from 'react';

2+ import { searchProducts } from '../api/search';

4+ const DEBOUNCE_MS = 300;

6+ function ProductSearch() {

7+ const [query, setQuery] = useState('');

8+ const [results, setResults] = useState([]);

9+ const [status, setStatus] = useState('idle');

10+

11+ useEffect(() => {

12+ const trimmed = query.trim();

13+ if (trimmed === '') {

14+ setResults([]);

15+ setStatus('idle');

16+ return undefined;

17+ }

18+ const controller = new AbortController();

19+ const timer = setTimeout(async () => {

20+ setStatus('loading');

21+ try {

22+ const found = await searchProducts(trimmed, { signal: controller.signal });

23+ if (controller.signal.aborted) return;

24+ setResults(found);

25+ setStatus('done');

26+ } catch {

27+ if (controller.signal.aborted) return;

28+ setStatus('error');

29+ }

30+ }, DEBOUNCE_MS);

31+ return () => {

32+ clearTimeout(timer);

33+ controller.abort();

34+ };

35+ }, [query]);

36+

37+ return (

38+ <div className="product-search">

39+ <input

40+ type="search"

41+ value={query}

42+ onChange={(event) => setQuery(event.target.value)}

43+ aria-label="Search products"

44+ placeholder="Search products"

45+ />

46+ {status === 'loading' && <p className="status" role="status">Searching…</p>}

47+ {status === 'error' && <p role="alert">Search failed. Try again.</p>}

48+ {status === 'done' && results.length === 0 && (

49+ <p className="empty" role="status">No products found.</p>

50+ )}

51+ <ul>

52+ {results.map((product) => (

53+ <li key={product.id}>{product.name}</li>

54+ ))}

55+ </ul>

56+ </div>

57+ );

58+ }

59+

60+ export default ProductSearch;

Fifty lines. Debounce inside a useEffect, an AbortController, an explicit signal check after the await, cleanup on unmount, accessible status announcements. It reads clean. But "reads clean" is exactly what the buggy version of this component also does, which is why this exercise exists.

Before you read my review

Try it cold

The same PR is a live exercise. Review it before I walk through the answer.

Read the requirements, read the diff, leave your comments, pick your verdict. You get the canonical review after you submit.

Review this PR →

Below here, I walk through every requirement and explain my verdict. If you want to form your own opinion first, now is the time.

The walkthrough: checking every door

The instinct on a PR like this is to scan for the bug, not find one, and hit approve with a vague sense that it "looked fine." That's rubber-stamping in the other direction. A proper approve is one where you can explain, requirement by requirement, why the code is correct. So let's do that.

The debounce: does typing actually suppress requests?

The request lives inside a setTimeout callback with a 300 ms delay (line 19). Every time query changes, the effect re-runs. The cleanup function on line 31-34 fires before the new effect body runs, and it calls clearTimeout(timer). So if the user types "a", then "al", then "ali" within 300 ms, the first two timers get cleared before they fire. Only the last one survives to make a request. That is a correct debounce.

A reviewer might object: "Why not use a debounce library?" That is a style preference, not a defect. The implementation is eight lines and does exactly what the spec asks. Requesting changes for "use lodash.debounce instead" when the hand-rolled version is correct and short is the kind of comment that burns reviewer credibility for zero safety gain.

The race condition that isn't

This is the line most likely to trigger a false reject:

22+ const found = await searchProducts(trimmed, { signal: controller.signal });

23+ if (controller.signal.aborted) return;

A reviewer who has seen race conditions in search components before might stop at line 22 and flag it. And they would be wrong, because the defense is on line 23.

Here is the scenario that line 23 handles. The user types "al", waits 300 ms, the request fires. Then they type "alice" and a new effect runs. The cleanup aborts the "al" controller and clears its timer. Now there are two outcomes for the in-flight "al" request:

The abort reaches the network layer before the response. The searchProducts promise rejects, the catch block runs, controller.signal.aborted is true, so it returns without touching state. Correct.
The response for "al" was already in flight and resolves before the abort signal propagates. This is the tricky edge. The await on line 22 resolves successfully with stale data. But line 23 checks controller.signal.aborted and returns. The stale results never reach setResults. Correct.

Both doors are closed. The cleanup aborts the controller, and the post-await guard catches the edge case where the response won the race against the abort. This is the textbook correct implementation, and the fact that it looks identical to the buggy version (which lacks line 23) is exactly what makes this a useful review exercise.

Empty and whitespace input

Lines 12-17 handle this. query.trim() collapses whitespace-only input to an empty string. When that happens, results are cleared, status goes to 'idle', and the function returns undefined (no cleanup needed because no timer or controller was created). No request fires. The spec says "An empty (or whitespace-only) box shows no results and makes no request." Check.

Returning undefined explicitly instead of just return is a style choice. React's useEffect expects the cleanup to be either a function or undefined. Both bare return and return undefined satisfy that contract. Not a defect.

Previous results stay visible during loading

This one is subtle enough that a reviewer might miss it in either direction. The spec says previous results should remain visible while a new query is loading. Look at what happens when the timer fires: line 20 sets status to 'loading', but nothing touches results until line 24, which only runs after the new response arrives. During the loading window, results still holds the previous array. The <ul> on line 51 renders results unconditionally, so the old items stay on screen while the loading indicator shows. When the new data lands, setResults(found) replaces them. Exactly what the spec asks.

Loading, error, and accessibility

Three states rendered: 'loading' shows "Searching..." with role="status" (line 46), 'error' shows "Search failed. Try again." with role="alert" (line 47), and the empty-results case gets role="status" (line 49). The role="alert" on error is appropriate because it is an urgent interruption. The role="status" on loading and empty-results is appropriate because those are polite status updates. The input has aria-label="Search products" (line 43). The spec says "announced to assistive technology." Check.

Unmount cleanup

If the component unmounts while a request is in flight, the same cleanup function (lines 31-34) runs: the timer is cleared and the controller is aborted. The post-await guard on line 23 (and the catch guard on line 27) prevents any state update on an unmounted component. No memory leak, no "can't perform a React state update on an unmounted component" warning. Check.

Keys

Line 53 uses product.id as the list key. The spec says the API returns products with "unique, stable ids." Using the stable ID as the key is correct. Using the array index would be wrong here because the list content changes between searches. Not a defect.

The verdict

Yyou commented

Approve. The debounce suppresses requests while typing, the AbortController cleanup cancels stale requests on re-render and unmount, and the signal.aborted guard after the await catches the edge case where the response resolves before the abort propagates. Empty and whitespace input is handled before any timer or controller is created. Previous results stay visible during loading because setResults is only called on success. All async states are rendered with appropriate ARIA roles. Clean implementation, nothing to block on.

Notice what that approve comment does: it names the specific mechanism that prevents the race condition (the post-await guard), confirms the edge case it covers (response resolving before abort), and checks the requirements that are easy to overlook (whitespace, previous results, accessibility). An approve that says "LGTM" communicates nothing. An approve that traces the async flow proves the reviewer actually read the code.

The comments you should not leave

There are several things a reviewer might flag on this PR that would be wrong to request changes for. It is worth naming them explicitly, because the ability to think "I noticed this and decided it is fine" is the skill this exercise trains.

"The empty catch hides errors." It does not. Aborted requests return early on line 27. Real failures (network errors, server 500s) fall through to setStatus('error') on line 28. The catch handles both cases correctly.

"setStatus('loading') inside the setTimeout is too late." It is not too late. It is precisely when loading begins. The loading state starts when the request is about to fire, not when the user starts typing. During the debounce window, nothing is loading, the component is just waiting for the user to stop typing. Showing a loading indicator during the debounce window would be incorrect.

"Use a custom hook or a library for this." That is a codebase architecture discussion, not a correctness issue. The implementation is 50 lines, it works, it handles all the edge cases, and it is self-contained. Blocking the PR to refactor it into a custom hook adds a review cycle and changes no behavior. File a follow-up if you feel strongly about it.

"The async callback inside setTimeout is unusual." It is unusual compared to some patterns, but it is not wrong. setTimeout ignores the return value of its callback, so the returned promise from the async function is discarded. That is fine here because all error handling is inside the try/catch, and the only way out of the function without setting state is through the abort guards. There is no unhandled promise rejection path.

Running the scenarios

To have full confidence in the approve, here are the scenarios worth tracing mentally (or in a test):

Fast typing: User types "a", "al", "ali", "alic", "alice" within 300 ms. Four timers are cleared by cleanup. Only the "alice" timer fires. One request. Correct.

Reversed response order: User types "al", waits 300 ms (request fires), then types "alice", waits 300 ms (second request fires). The "al" response arrives after the "alice" response. The "al" controller was aborted by the cleanup, so either the promise rejected (caught and guarded on line 27) or it resolved and is guarded on line 23. The "alice" results are the ones on screen. Correct.

Response already resolved before abort: The "al" promise has already settled with data sitting in the microtask queue. The cleanup aborts the controller. When the microtask runs, found contains the stale "al" data, but controller.signal.aborted is true, so line 23 returns. No stale state update. Correct.

Unmount during request: Component unmounts, cleanup runs, timer cleared, controller aborted. Any pending or resolving promise hits the abort guard. No state update on an unmounted component. Correct.

Whitespace input: User types three spaces. query.trim() is '', results cleared, status set to 'idle', early return before any timer or controller. No request. Correct.

API error: Server returns 500, searchProducts rejects. Catch block runs. Signal is not aborted (this was a real error, not a cancellation). setStatus('error') fires. User sees "Search failed. Try again." with role="alert". Correct.

The skill: defending your approve

Most code review advice is about finding problems. Lists of things to check, patterns to watch for, red flags to escalate. That advice is valuable and incomplete. Half of all PRs you review (roughly) are fine, and for those you need a different skill: the ability to check every requirement, trace every suspicious path, and then say "this is correct and here is why" instead of inventing a problem because you feel like you should find one.

A defended approve is not the same as a rubber stamp. A rubber stamp is "LGTM" after 30 seconds. A defended approve is "I traced the async flow through both resolution paths, confirmed the abort guard covers the microtask edge case, verified that previous results persist during loading, and checked the ARIA roles. Approve." The first one communicates nothing. The second one communicates that you actually reviewed the code, and if a bug surfaces later, your comment is evidence of due diligence rather than evidence of neglect.

Next time you are about to approve a PR and you catch yourself hesitating because you feel like you "should" find something, stop. Trace the requirements. Check the edge cases. If they all pass, write the approve comment that explains why. That is the review.