2026-09-05 · 7 min read

React code review exercise: the timer that stops at one

I have seen this bug in more than one codebase, and it shows up roughly the same way every time: a "time on page" metric silently reports one second for every single user session, and nobody notices until someone squints at the analytics dashboard weeks later. The component has a setInterval inside a useEffect with [] deps, and the callback does setCount(count + 1). It renders "1s" and stays there. The interval is firing, React is re-rendering, everything looks alive. But the callback closed over count from the very first render, so every tick computes 0 + 1, forever. Weeks of engagement data, quietly corrupted, because the code looked like it worked and even sort of did work if you only glanced at it.

That bug is the subject of this React and JavaScript code review exercise. It is a worked example with the full solution: the fourteen-line component, the stale closure walkthrough, the one-line fix, and a test that fails on the broken version and passes on the fixed one. If you review AI-generated React code, or you just want to keep your code-reading instincts from atrophying while Copilot writes your components, this is a good ten minutes. Stale closures are one of the most documented React hooks bugs, the exhaustive-deps lint rule warns about them, and yet they keep shipping, because the code that triggers them looks exactly like the code you are supposed to write. An effect with an empty dependency array, an interval created once, a cleanup that clears it on unmount. That is the textbook pattern for "run something once at mount." The problem is that the callback inside the interval is not the effect. It is a separate function, created once, that closes over whatever state existed at creation time. And nobody re-creates it, because that is exactly what the empty array prevents.

The requirements

The task is a small "time on page" widget for an article view:

Show a counter that starts at 0 and increases by 1 every second.
Use a single interval started once when the component mounts.
Clear the interval when the component unmounts.

Straightforward. One useEffect, one setInterval, one cleanup. Here is the PR that came back.

The PR

ai-agent wants to merge · src/components/SecondsCounter.jsx +14

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

3+ export function SecondsCounter() {

4+ const [count, setCount] = useState(0);

6+ useEffect(() => {

7+ const id = setInterval(() => {

8+ setCount(count + 1);

9+ }, 1000);

10+ return () => clearInterval(id);

11+ }, []);

12+

13+ return <p>{count}s</p>;

14+ }

Fourteen lines. Clean imports, no unnecessary state, cleanup on unmount. If you are skimming PRs between meetings, this looks done. The empty dependency array means the interval starts once, exactly as required. The cleanup function clears it. The component renders the count. What is wrong with it?

Try it yourself

The same PR is a live exercise. Review it cold before you read the answer.

Leave inline comments, pick your verdict, then see the canonical review. Free, no signup.

Review this PR →

Walkthrough: spoilers below

Fourteen lines, what could go wrong

Focus on line 8: setCount(count + 1). Now look at line 11: }, []). The empty dependency array tells React to run this effect exactly once, on mount. That means the arrow function passed to setInterval on line 7 is created once, right now, during the first render. And during the first render, count is 0.

JavaScript closures capture variables by reference to the scope they were created in, not by some live binding to React state. The interval callback was born in a world where count is 0, and it will live in that world until the interval is cleared. Every single tick, it runs setCount(0 + 1). The component re-renders, shows "1s", and then a second later it runs setCount(0 + 1) again. And again. And again. The counter jumps to 1 and stays there, forever.

The insidious part is that the interval is genuinely firing. React is genuinely re-rendering (it does not bail out because setCount(1) when state is already 1 still triggers a render in React 18+ the first time, and after that React may bail out but the interval keeps ticking). The cleanup will genuinely clear the interval on unmount. Everything about this component works except the one thing it was supposed to do.

The requirement it breaks

Requirement 1: "increases by 1 every second." After the first tick, the counter reads 1 and never changes. Five seconds in, it should read 5. It reads 1. A "time on page" widget that always says one second is worse than no widget at all, because someone downstream will use that number to make decisions, and those decisions will be based on the fiction that every user leaves after one second.

The obvious fix is wrong

The first instinct most people have is: "the linter is right, count is missing from the dependency array, just add it." If you add count to the deps, the effect now re-runs on every render where count changes. That means it clears the old interval and creates a new one, every single second. The counter works, but you have violated requirement 2: "use a single interval started once when the component mounts." You are now creating and destroying an interval every second, which is both wasteful and subtly different in timing behavior. It also means that if the component re-renders for any other reason (a parent state change, a context update), the interval resets. That is not what anyone asked for.

This is what makes the bug a good review exercise. The naive fix satisfies one requirement by breaking another, and the correct fix requires understanding why the closure is stale in the first place.

The comment you would leave

6+ useEffect(() => {

7+ const id = setInterval(() => {

8+ setCount(count + 1);

Yyou commented

count here is captured from the first render and never updates. Every tick computes setCount(0 + 1), so the counter sticks at 1. Use the functional updater instead: setCount(c => c + 1). That reads the latest state without capturing it, so the empty dependency array stays correct and the interval stays singular.

9+ }, 1000);

10+ return () => clearInterval(id);

11+ }, []);

The verdict

Request changes. The counter does not count. That is a blocker, not a nit. The shape of the component is right, the cleanup is right, and the fix is a one-line change inside the callback, so this should come back quickly.

The fix

Replace line 8 with the functional form of setCount:

8- setCount(count + 1);

8+ setCount((c) => c + 1);

The functional updater setCount(c => c + 1) receives the current state as its argument instead of closing over a stale variable. The callback no longer needs to know what count is, so the empty dependency array is now genuinely correct. The interval is still created once, still cleaned up on unmount, and the counter actually counts: 0, 1, 2, 3, 4, as many seconds as the component stays mounted.

A check that fails on the old code and passes on the fix

You do not need a test framework to see this. Open a browser console, render the component, and wait three seconds. The old code shows "1s" after one second and stays there. The fixed code shows "1s", then "2s", then "3s". If you want something more mechanical, here is a minimal test with React Testing Library and fake timers:

1+ import { render, screen, act } from '@testing-library/react';

2+ import { SecondsCounter } from './SecondsCounter';

4+ beforeEach(() => jest.useFakeTimers());

5+ afterEach(() => jest.useRealTimers());

7+ test('counter increments past 1', () => {

8+ render(<SecondsCounter />);

9+ act(() => jest.advanceTimersByTime(3000));

10+ expect(screen.getByText('3s')).toBeInTheDocument();

11+ });

With the buggy code, the screen shows "1s" after three seconds and the assertion fails. With the fix, it shows "3s" and the test passes. The test is simple enough that the assertion itself is obviously correct, which is important: a test that is as complicated as the code it checks is just another place for bugs to hide.

The question that would have caught this in ten seconds

Every time you see a callback inside useEffect with an empty dependency array that reads component state, ask one question: "which render's value is this capturing?" If the answer is "the first one, and only the first one," and the callback is supposed to work with current state, you have found a stale closure. This applies to setInterval, setTimeout, event listeners attached in effects, WebSocket message handlers, anything that creates a long-lived function that outlives the render it was born in.

The functional updater is the fix when you are computing new state from old state. When you need to read state without updating it (say, to conditionally skip some work), a ref is the right tool. And when you need both current state and current props inside a long-lived callback, React's useEffectEvent (stable since React 19) is built exactly for this. But the review habit is the same regardless of which fix applies: if a callback outlives its render, check what it captured.