fix(cocktails): add timeout, UA header, clearer upstream errors and request logging

[devin-ai-integration]

Summary

Hardens GET /api/cocktails so that any upstream failure is surfaced as a descriptive JSON error to the client instead of leaving the browser with a generic TypeError: Failed to fetch. Motivated by a report that letters other than a failed with Failed to fetch in the UI — I could not reproduce locally (curl + Chromium both succeeded for every letter a–z against the deployed code), so this PR is defensive, not a confirmed root cause.

Changes:

• Upstream fetch robustness (src/routes/cocktails.js):
  • 10 s timeout via AbortSignal.timeout(FETCH_TIMEOUT_MS) — previously the request could hang forever, which browsers surface as Failed to fetch.
  • Explicit User-Agent: api00/... and Accept: application/json headers (some CDNs/firewalls reject requests with Node's default UA).
  • Separate handling for three failure modes, each with its own status + JSON body:
    • TimeoutError / AbortError → 504 Upstream thecocktaildb timed out after 10000ms.
    • Any other fetch rejection (DNS, ECONNRESET, …) → 502 Upstream thecocktaildb unreachable. + detail.
    • upstream.json() throwing (HTML rate-limit page, truncated body, …) → 502 …invalid JSON response.
  • Pre-flight guard: if the Node runtime has no global fetch (< 18), return 500 with a clear message instead of an opaque crash.
  • Each failure path also console.errors a one-line diagnostic on the server so local users can paste it into a bug report.
• Request logging middleware (src/app.js): logs METHOD URL -> STATUS MS on every finished response. Off when NODE_ENV=test (keeps test output quiet); createApp({ logRequests }) accepts an explicit override.
• Tests (tests/cocktails.test.js): 4 new cases covering the timeout (504), generic fetch rejection (502), invalid-JSON (502), and the UA/AbortSignal headers. Two existing assertions switched from toHaveBeenCalledWith(url) to mock.calls[0][0] since fetch now receives an options object. Cache-test letter changed to avoid collision with the new tests (the module-level cache is cleared in beforeEach, but the letters are kept distinct for clarity).

All 14 tests, npm run lint, and npm run format:check pass locally.

Review & Testing Checklist for Human

• Confirm this actually fixes the reported symptom. On the machine that saw the bug, pull this branch, npm install, npm run dev, reload /, and try letters that previously failed. Expected: either (a) they work, or (b) the UI shows a descriptive error (timed out…, unreachable…, etc.) instead of Failed to fetch. Share the server console line and the DevTools → Network response for the failing request so we can pinpoint the real root cause.
• Confirm node --version is ≥ 20 (per .nvmrc). AbortSignal.timeout is stable from Node 20; on older Node it will throw at request time. This PR adds a guard for missing fetch but not for missing AbortSignal.timeout.
• Sanity-check the 10 s timeout: acceptable for an upstream that occasionally takes a few seconds, but not so long that the UI feels frozen. Adjust FETCH_TIMEOUT_MS if needed.

Notes

• I could not reproduce Failed to fetch locally — curl and Chromium both succeeded for every letter a–z against the pre-fix code. If the bug persists after this PR, the most likely remaining causes are: (1) Node runtime missing AbortSignal.timeout, (2) the dev server crashing on their machine (the new request logger will make this obvious), or (3) thecocktaildb rate-limiting their IP after the first call (the new 502 + detail will expose this).
• createApp()'s signature is still zero-arg compatible; the new { logRequests } option has a sensible default.
• No change to the public API shape of successful responses, to caching behavior, or to the frontend.

Link to Devin session: https://app.devin.ai/sessions/38829c1a13124d54a5fdf114ce348ba9
Requested by: @ModulosPB

---

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[devin-ai-integration]

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

[devin-ai-integration]

🔴 Removal of outer try/catch + next(err) in Express 4 async handler causes unhandled promise rejections

The old code wrapped the entire route handler body in try { ... } catch (err) { return next(err); }, which is the standard pattern for safely handling errors in Express 4 async route handlers (Express 4 does not automatically catch rejected promises from async handlers). The new code removes this outer safety net and also removes the next parameter entirely. Any unexpected runtime error thrown outside the two specific try/catch blocks (lines 63-83 and 94-101) — such as in parseDrink at line 104 or res.json() at line 106 — will become an unhandled promise rejection instead of being routed to the errorHandler middleware (src/middleware/errorHandler.js).

A concrete scenario: if the upstream API returns { drinks: [null] }, drinks.map(parseDrink) will call parseDrink(null), which accesses null['strIngredient1'] at src/routes/cocktails.js:19, throwing a TypeError. With no outer catch, this crashes or leaves the request hanging.

Prompt for agents

The route handler at src/routes/cocktails.js:42 is an async function mounted on Express 4 (which does NOT auto-catch rejected promises from async handlers). The old code had try { ... } catch (err) { return next(err); } wrapping all logic, forwarding unexpected errors to the Express error handler middleware. The new code removed this pattern and also dropped the `next` parameter.

To fix: either (1) re-add the `next` parameter and wrap the handler body in a top-level try/catch that calls `next(err)`, or (2) use an async error-wrapping utility (e.g. express-async-errors or a custom wrapper). The specific try/catch blocks for fetch and upstream.json can remain, but an outer catch is needed for everything else — particularly lines 103-106 where parseDrink could throw on malformed upstream data (e.g. null elements in the drinks array).

---

Was this helpful? React with 👍 or 👎 to provide feedback.