Review this React component: what bugs would you flag?
Code-review the component below. Identify the correctness bugs and the conventions to fix, and explain why each matters. Focus on the data-fetching effect, the data-shape handling, and the list rendering.
export const SpotList = (props) => {
const [spot, setSpot] = useState();
const [location, setLocation] = useState({});
useEffect(() => {
loadLocation(props.locationId).then((loc) => setLocation(loc));
});
const spotTimes = props.spots.map((s) => {
const slot = s.timeslots.find((t) => t.isAvailable);
return { [s.id]: slot };
});
return (
<>
<header>Spots for {location.name}</header>
{props.spots.map((s) => (
<div onClick={() => setSpot(s.id)} className={spot == s.id ? 'active' : ''}>
{s.id} at {spotTimes[s.id]}
</div>
))}
</>
);
};Diagnose the bugs.
Main bug: useEffect has no dependency array, so it refetches every render — and each setLocation triggers another render, an effective loop; give it [props.locationId]. spotTimes is an ARRAY of single-key objects but is read as spotTimes[s.id] — wrong shape; build one object or a Map keyed by id. The mapped <div> has no key, breaking reconciliation. Minor: use ===, and render a loading state for location.
- ✗Missing the no-dependency-array bug that causes the effect to refetch on every render
- ✗Not noticing
spotTimesis an array of objects but indexed as if it were a keyed map - ✗Overlooking the missing
keyon the mapped list elements
- →Why does an effect with no dependency array combined with
setStatecreate a refetch loop? - →How would you reshape
spotTimessospotTimes[s.id]reads correctly?
Solution
The main bugs are the dependency-free effect and the wrong shape of spotTimes.
export const SpotList = (props) => {
const [spot, setSpot] = useState();
const [location, setLocation] = useState(null);
useEffect(() => {
loadLocation(props.locationId).then(setLocation);
}, [props.locationId]); // dependency: fetch only when the id changes
const spotTimes = props.spots.reduce((acc, s) => {
acc[s.id] = s.timeslots.find((t) => t.isAvailable);
return acc;
}, {}); // one object, readable via spotTimes[s.id]
if (!location) return <p>Loading…</p>;
return (
<>
<header>Spots for {location.name}</header>
{props.spots.map((s) => (
<div
key={s.id}
onClick={() => setSpot(s.id)}
className={spot === s.id ? 'active' : ''}
>
{s.id} at {spotTimes[s.id]?.label}
</div>
))}
</>
);
};How it works
A useEffect with no second argument runs after EVERY render. Inside, it calls setLocation, which triggers a re-render, which runs the effect again — a cascade of requests. The dependency array [props.locationId] limits it to running when the id changes.
In the original, spotTimes is an array of { [id]: slot } objects but is then read as spotTimes[s.id], which is almost always undefined. The fix is to build a single object (or Map) keyed by id via reduce. Plus the small things: mapped elements need a stable key for reconciliation, === over ==, and a loading state while location is not yet fetched. </content>
