fix: throw on non-2xx HTTP status in fetchSpec and add unit tests#2104
fix: throw on non-2xx HTTP status in fetchSpec and add unit tests#2104Adi-204 wants to merge 2 commits into
Conversation
Adi-204
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
🦋 Changeset detectedLatest commit: 2f632fc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughfetchSpec was refactored to use async/await, validate response.ok, and throw descriptive errors on non-2xx HTTP responses; tests and a Changeset entry were added to verify success and multiple failure scenarios. ChangesfetchSpec HTTP Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/generator/lib/utils.js (1)
62-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the
@asynctag to the JSDoc.The function is now
async, but the JSDoc is missing the@asynctag. As per coding guidelines, public async functions should include the@asynctag.📝 Proposed fix to add `@async` tag
/** * Fetches an AsyncAPI document from the given URL and return its content as string * + * `@async` * `@param` {String} link URL where the AsyncAPI document is located. * `@returns` {Promise<String>} Content of fetched file. * `@throws` {Error} When the HTTP response is not successful (non-2xx status). */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/generator/lib/utils.js` around lines 62 - 68, Add the missing `@async` JSDoc tag to the docblock for the async function whose JSDoc begins "Fetches an AsyncAPI document from the given URL and return its content as string" (the async fetch function in utils.js), i.e., update that JSDoc to include an `@async` tag along with the existing `@param`, `@returns`, and `@throws` entries so the public async function follows the coding guidelines.
🧹 Nitpick comments (1)
apps/generator/lib/utils.js (1)
72-72: 💤 Low valueConsider simplifying the nested template literal.
The nested template literal reduces readability. You could extract the status text formatting to a variable for clarity.
♻️ Proposed refactor to improve readability
utils.fetchSpec = async (link) => { const res = await fetch(link); if (!res.ok) { - throw new Error(`Failed to fetch AsyncAPI document from ${link}: HTTP ${res.status}${res.statusText ? ` ${res.statusText}` : ''}`); + const statusText = res.statusText ? ` ${res.statusText}` : ''; + throw new Error(`Failed to fetch AsyncAPI document from ${link}: HTTP ${res.status}${statusText}`); } return res.text(); };As per coding guidelines, static analysis flagged this as a code smell for nested template literals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/generator/lib/utils.js` at line 72, Extract the status-text fragment into a local variable and use that variable in the Error message to avoid the nested template literal; specifically, in the throw new Error statement that builds the message with link, res.status and res.statusText, create a const (e.g. statusText or formattedStatusText) that computes res.statusText ? ` ${res.statusText}` : '' and then interpolate that variable into the final template literal so the throw new Error uses `Failed to fetch AsyncAPI document from ${link}: HTTP ${res.status}${statusText}` for improved readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/generator/lib/utils.js`:
- Around line 62-68: Add the missing `@async` JSDoc tag to the docblock for the
async function whose JSDoc begins "Fetches an AsyncAPI document from the given
URL and return its content as string" (the async fetch function in utils.js),
i.e., update that JSDoc to include an `@async` tag along with the existing `@param`,
`@returns`, and `@throws` entries so the public async function follows the coding
guidelines.
---
Nitpick comments:
In `@apps/generator/lib/utils.js`:
- Line 72: Extract the status-text fragment into a local variable and use that
variable in the Error message to avoid the nested template literal;
specifically, in the throw new Error statement that builds the message with
link, res.status and res.statusText, create a const (e.g. statusText or
formattedStatusText) that computes res.statusText ? ` ${res.statusText}` : ''
and then interpolate that variable into the final template literal so the throw
new Error uses `Failed to fetch AsyncAPI document from ${link}: HTTP
${res.status}${statusText}` for improved readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bded0fe7-0a49-412f-a325-fcc0e6033b0d
📒 Files selected for processing (3)
.changeset/fix-fetch-spec-http-error-status.mdapps/generator/lib/utils.jsapps/generator/test/utils.test.js
2 participants
Description
Fix
fetchSpecsilently resolving on non-2xx HTTP responses. Previously, fetching an AsyncAPI document from a URL that returned a 4xx or 5xx status would resolve with the error response body (e.g. an HTML page) instead of rejecting.fetchSpecnow throws a descriptive error including the URL and HTTP status code, so failures are surfaced immediately rather than propagating as invalid spec content.Related issue(s)
Fixes #1807
Summary by CodeRabbit
Bug Fixes
Tests