fix(sharing): extract producers array from wrapped API response in fetchDrinkData - #286
Conversation
…tchDrinkData
The data API returns {"producers": [...], "timestamp": ...} but
fetchDrinkData was returning the whole object. findDrink then called
for...of on a plain object, which throws TypeError: not iterable —
silently falling back to the unmodified SPA for all crawlers.
Fix: extract data.producers when the response is an object, fall back
to treating the response as an array for forward-compatibility.
Add 5 missing unit tests for fetchDrinkData covering the wrapped format,
bare-array format, 404 responses, missing producers key, and URL construction.
https://claude.ai/code/session_014n8NH3HkZQUkGGjK2geKYg
There was a problem hiding this comment.
Pull request overview
Fixes Cloudflare Pages “drink preview” sharing for crawlers by making fetchDrinkData return an iterable producers array when the data API wraps the response, preventing findDrink from throwing and falling back to the SPA for crawlers.
Changes:
- Update
fetchDrinkDatato unwrap{ producers: [...] }responses while still supporting bare-array responses. - Add unit tests covering wrapped vs bare-array responses, non-OK responses, missing
producers, and URL construction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| functions/_lib/drink-preview.js | Unwraps the API response so fetchDrinkData yields producer arrays for findDrink. |
| functions/test/drink-preview.test.js | Adds vitest coverage for fetchDrinkData response shapes and URL construction. |
| if (!response.ok) return null; | ||
| return response.json(); | ||
| const data = await response.json(); | ||
| return Array.isArray(data) ? data : (data.producers ?? null); |
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it('returns producers array from wrapped API response', async () => { | ||
| const producers = [{ id: 'adnams', name: 'Adnams', products: [] }]; | ||
| global.fetch = vi.fn().mockResolvedValue({ | ||
| ok: true, | ||
| json: () => Promise.resolve({ producers, timestamp: '2025-01-01' }), | ||
| }); | ||
| const result = await fetchDrinkData('cbf2025', 'beer'); | ||
| expect(result).toEqual(producers); | ||
| }); | ||
|
|
||
| it('returns array directly when API response is already an array', async () => { | ||
| const producers = [{ id: 'adnams', name: 'Adnams', products: [] }]; | ||
| global.fetch = vi.fn().mockResolvedValue({ | ||
| ok: true, | ||
| json: () => Promise.resolve(producers), | ||
| }); | ||
| const result = await fetchDrinkData('cbf2025', 'beer'); | ||
| expect(result).toEqual(producers); | ||
| }); | ||
|
|
||
| it('returns null when response is not ok', async () => { | ||
| global.fetch = vi.fn().mockResolvedValue({ ok: false, status: 404 }); | ||
| const result = await fetchDrinkData('cbf2025', 'beer'); | ||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it('returns null when wrapped response has no producers key', async () => { | ||
| global.fetch = vi.fn().mockResolvedValue({ | ||
| ok: true, | ||
| json: () => Promise.resolve({ timestamp: '2025-01-01' }), | ||
| }); | ||
| const result = await fetchDrinkData('cbf2025', 'beer'); | ||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it('fetches the correct URL for a given festival and category', async () => { | ||
| global.fetch = vi.fn().mockResolvedValue({ | ||
| ok: true, | ||
| json: () => Promise.resolve({ producers: [] }), | ||
| }); |
…ction Tests the full onRequest pipeline that was previously untested: - Non-crawler passthrough (ASSETS.fetch called, no data fetch, no OG tags) - Crawler OG injection (og:title, og:url, correct festival/category URL) - Fallback paths (non-ok API, network error, drink not found) Uses a minimal MockHTMLRewriter that appends to </head> without requiring the Cloudflare Workers runtime or any new dependencies. https://claude.ai/code/session_014n8NH3HkZQUkGGjK2geKYg
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ock leak fetchDrinkData now guards against data.producers being non-array and data being null, returning null in both cases rather than returning an uninitable value that would make findDrink throw. Test hygiene: replace global.fetch/HTMLRewriter direct assignment with vi.stubGlobal/vi.unstubAllGlobals so stubs are properly restored between test files without relying on vi.restoreAllMocks (which doesn't restore globalThis properties set by direct assignment). Two new fetchDrinkData cases: null JSON body and non-array producers field. https://claude.ai/code/session_014n8NH3HkZQUkGGjK2geKYg
|
@copilot re-review |
Re-reviewed: the previously flagged issues are addressed in 8acc8db. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
The data API returns {"producers": [...], "timestamp": ...} but
fetchDrinkData was returning the whole object. findDrink then called
for...of on a plain object, which throws TypeError: not iterable —
silently falling back to the unmodified SPA for all crawlers.
Fix: extract data.producers when the response is an object, fall back
to treating the response as an array for forward-compatibility.
Add 5 missing unit tests for fetchDrinkData covering the wrapped format,
bare-array format, 404 responses, missing producers key, and URL construction.
https://claude.ai/code/session_014n8NH3HkZQUkGGjK2geKYg