Skip to content

Commit a564936

Browse files
fix(Home): add AbortController cleanup to prevent memory leaks on unmount (#7)
* fix(Home): add AbortController cleanup to prevent memory leaks on unmount useEffect had no cleanup — if the component unmounted before fetches completed, React would attempt state updates on an unmounted component. AbortController cancels both fetches on cleanup. AbortError is silently ignored (not treated as a fetch failure) per Gemini review on PR #6. * test(Home): assert AbortError is silently ignored, not logged as error When a fetch is aborted (e.g. component unmount), the AbortError should be swallowed — it is not a real failure. Verifies console.error is NOT called when all fetches reject with an AbortError. * chore: add coverage to .gitignore, delete stale docs copy files coverage/ directories were untracked. Also removes accidental *copy.md files that were left over from a previous session. * fix(Home): check res.ok before parsing JSON on both fetch calls fetch() only rejects on network errors — HTTP 4xx/5xx responses resolve with a non-OK status. Without this check a 500 would try to parse an error body as JSON and throw a cryptic error. Addresses high-priority Gemini feedback on PR #7. * test(Home): cover res.ok HTTP error check and update mock responses Adds two tests that verify a non-OK HTTP response (4xx/5xx) is treated as an error and logged via console.error. Updates all existing mocks to include ok: true to match the new res.ok guard.
1 parent 8adac66 commit a564936

3 files changed

Lines changed: 69 additions & 8 deletions

File tree

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,4 @@ dist
33
.env
44
*.db
55
.turbo
6+
coverage

packages/web/src/pages/Home.test.tsx

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,9 @@ beforeEach(() => {
2727
consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
2828
global.fetch = vi.fn().mockImplementation((url: string) => {
2929
if (url.includes('/meta/genres')) {
30-
return Promise.resolve({ json: () => Promise.resolve(mockGenres) })
30+
return Promise.resolve({ ok: true, json: () => Promise.resolve(mockGenres) })
3131
}
32-
return Promise.resolve({ json: () => Promise.resolve(mockMovies) })
32+
return Promise.resolve({ ok: true, json: () => Promise.resolve(mockMovies) })
3333
})
3434
})
3535

@@ -92,7 +92,7 @@ describe('Home — search and genre filter', () => {
9292
const fetchError = new Error('Network error')
9393
global.fetch = vi.fn().mockImplementation((url: string) => {
9494
if (url.includes('/meta/genres')) {
95-
return Promise.resolve({ json: () => Promise.resolve(mockGenres) })
95+
return Promise.resolve({ ok: true, json: () => Promise.resolve(mockGenres) })
9696
}
9797
return Promise.reject(fetchError)
9898
})
@@ -107,10 +107,55 @@ describe('Home — search and genre filter', () => {
107107
if (url.includes('/meta/genres')) {
108108
return Promise.reject(fetchError)
109109
}
110-
return Promise.resolve({ json: () => Promise.resolve(mockMovies) })
110+
return Promise.resolve({ ok: true, json: () => Promise.resolve(mockMovies) })
111111
})
112112

113113
renderHome()
114114
await waitFor(() => expect(consoleErrorSpy).toHaveBeenCalledWith('Failed to fetch genres:', fetchError))
115115
})
116+
117+
it('does not log console.error when fetch is aborted (component unmount)', async () => {
118+
const abortError = Object.assign(new Error('Aborted'), { name: 'AbortError' })
119+
global.fetch = vi.fn().mockRejectedValue(abortError)
120+
121+
renderHome()
122+
// Wait for promises to settle
123+
await new Promise(r => setTimeout(r, 0))
124+
125+
expect(consoleErrorSpy).not.toHaveBeenCalled()
126+
})
127+
128+
it('logs console.error when movies fetch returns a non-OK HTTP status', async () => {
129+
global.fetch = vi.fn().mockImplementation((url: string) => {
130+
if (url.includes('/meta/genres')) {
131+
return Promise.resolve({ ok: true, json: () => Promise.resolve(mockGenres) })
132+
}
133+
return Promise.resolve({ ok: false, status: 500 })
134+
})
135+
136+
renderHome()
137+
await waitFor(() =>
138+
expect(consoleErrorSpy).toHaveBeenCalledWith(
139+
'Failed to fetch movies:',
140+
expect.objectContaining({ message: 'HTTP error! status: 500' })
141+
)
142+
)
143+
})
144+
145+
it('logs console.error when genres fetch returns a non-OK HTTP status', async () => {
146+
global.fetch = vi.fn().mockImplementation((url: string) => {
147+
if (url.includes('/meta/genres')) {
148+
return Promise.resolve({ ok: false, status: 404 })
149+
}
150+
return Promise.resolve({ ok: true, json: () => Promise.resolve(mockMovies) })
151+
})
152+
153+
renderHome()
154+
await waitFor(() =>
155+
expect(consoleErrorSpy).toHaveBeenCalledWith(
156+
'Failed to fetch genres:',
157+
expect.objectContaining({ message: 'HTTP error! status: 404' })
158+
)
159+
)
160+
})
116161
})

packages/web/src/pages/Home.tsx

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,23 +20,38 @@ function Home() {
2020
const [genres, setGenres] = useState<string[]>([]);
2121

2222
useEffect(() => {
23-
fetch('/api/movies')
24-
.then(res => res.json())
23+
const controller = new AbortController();
24+
const { signal } = controller;
25+
26+
fetch('/api/movies', { signal })
27+
.then(res => {
28+
if (!res.ok) throw new Error(`HTTP error! status: ${res.status}`);
29+
return res.json();
30+
})
2531
.then((data: Movie[]) => {
2632
setMovies(data);
2733
setLoading(false);
2834
})
2935
.catch((err: unknown) => {
36+
if (err instanceof Error && err.name === 'AbortError') return;
3037
console.error('Failed to fetch movies:', err);
3138
setLoading(false);
3239
});
3340

34-
fetch('/api/movies/meta/genres')
35-
.then(res => res.json())
41+
fetch('/api/movies/meta/genres', { signal })
42+
.then(res => {
43+
if (!res.ok) throw new Error(`HTTP error! status: ${res.status}`);
44+
return res.json();
45+
})
3646
.then((data: string[]) => setGenres(data))
3747
.catch((err: unknown) => {
48+
if (err instanceof Error && err.name === 'AbortError') return;
3849
console.error('Failed to fetch genres:', err);
3950
});
51+
52+
return () => {
53+
controller.abort();
54+
};
4055
}, []);
4156

4257
const filteredMovies = useMemo(() => {

0 commit comments

Comments
 (0)