✨ Quality: Incorrect JSON detection for Content-Type with charset causes wrong response parsing#2774
Conversation
…charset causes wrong response parsing `getResponseBody()` checks `Content-Type` using strict equality to `'application/json'`. Real APIs commonly return `'application/json; charset=utf-8'`. In that case, the function falls back to `response.text()` and returns a string instead of parsed JSON, causing downstream runtime failures when callers access object properties. Affected files: utils.ts Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the frontend API response parsing helper so JSON responses are detected even when the Content-Type header includes parameters (e.g., charset=utf-8), preventing callers from accidentally receiving a raw string instead of parsed JSON.
Changes:
- Cache
Content-Typeonce per response ingetResponseBody. - Switch JSON detection from strict equality to substring detection.
- Reuse the cached
Content-Typefor file-type detection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const contentType = response.headers.get('Content-Type') || ''; | ||
|
|
||
| if (contentType.includes('application/json')) { | ||
| return response.json(); |
There was a problem hiding this comment.
contentType.includes('application/json') is case-sensitive and can yield false positives (e.g. application/jsonp) while still missing valid JSON media types like application/merge-patch+json / application/*+json. Consider normalizing once (trim + lowercase) and comparing the parsed media type (e.g., split on ;) against application/json or endsWith('+json').
| const fileType = ['application/pdf', 'application/zip']; | ||
| if (fileType.includes(response.headers.get('Content-Type') || '')) { | ||
| if (fileType.includes(contentType)) { | ||
| return new Promise((resolve) => resolve(response)); |
There was a problem hiding this comment.
fileType.includes(contentType) still won’t match when the Content-Type header includes parameters (e.g. application/pdf; charset=binary). If you parse/normalize the media type (e.g. contentType.split(';')[0]), reuse that value for both JSON and file detection to avoid similar mis-parsing for downloads.
| export function getResponseBody(response: Response) { | ||
| if (response.headers.get('Content-Type') === 'application/json') { | ||
| const contentType = response.headers.get('Content-Type') || ''; | ||
|
|
||
| if (contentType.includes('application/json')) { | ||
| return response.json(); | ||
| } | ||
|
|
||
| const fileType = ['application/pdf', 'application/zip']; | ||
| if (fileType.includes(response.headers.get('Content-Type') || '')) { | ||
| if (fileType.includes(contentType)) { | ||
| return new Promise((resolve) => resolve(response)); | ||
| } | ||
|
|
||
| return response.text(); |
There was a problem hiding this comment.
There’s no unit test covering the new Content-Type parsing behavior (e.g. application/json; charset=utf-8 should go through response.json(), and ideally application/*+json too). Adding a focused test for getResponseBody would prevent regressions and validate the fix for #2773.
✨ Code Quality
Problem
getResponseBody()checksContent-Typeusing strict equality to'application/json'. Real APIs commonly return'application/json; charset=utf-8'. In that case, the function falls back toresponse.text()and returns a string instead of parsed JSON, causing downstream runtime failures when callers access object properties.Severity:
highFile:
src/frontend/js/api/utils.tsSolution
Parse media type using prefix/inclusion instead of exact equality:
Changes
src/frontend/js/api/utils.ts(modified)Purpose
Description...
Proposal
Description...
🤖 About this PR
This pull request was generated by ContribAI, an AI agent
that helps improve open source projects. The change was:
If you have questions or feedback about this PR, please comment below.
We appreciate your time reviewing this contribution!
Closes #2773