Conversation
WalkthroughModified download URL resolution to precompute tags for valid versions and defer GitHub API calls when possible, reducing unnecessary requests. Added comprehensive test suite validating optimization paths and dynamic version resolution scenarios. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the download URL generation process by avoiding unnecessary GitHub API calls when a specific version number is provided. Instead of always fetching all tags from the API, the code now directly constructs the download URL for exact version strings (e.g., "1.0.0"), only falling back to the API for dynamic versions like "latest", "canary", or semver ranges.
Changes:
- Added
validateStrictimport to check if a version string is an exact semantic version - Modified
getSemverDownloadUrlto skip API calls when an exact version is provided - Added comprehensive test coverage for both optimized (no API call) and dynamic (API lookup) scenarios
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/download-url.ts | Implements optimization to construct download URLs directly for exact versions, avoiding API calls |
| tests/download-url.spec.ts | Adds test suite verifying API call avoidance for exact versions and proper API usage for dynamic versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/download-url.spec.ts`:
- Around line 92-104: Add a test that covers the case where version is undefined
or an empty string so the branch in getDownloadUrl that treats "latest" ||
!version is exercised; in tests/download-url.spec.ts add an it() that calls
getDownloadUrl({ version: undefined, os: "linux", arch: "x64" }) (and another
variant for "" if desired), asserts the same expected URL as the "latest" test,
and verifies requestSpy was called once to ensure the API lookup path is used.
- Around line 132-141: The test calling getDownloadUrl uses
expect(...).rejects.toThrow without awaiting the returned promise; update the
test for the "should throw error if semver range matches nothing" case to await
the rejects assertion (i.e., await
expect(getDownloadUrl({...})).rejects.toThrow(...)) so the rejection is actually
verified before asserting requestSpy call count; ensure the requestSpy assertion
(requestSpy.toHaveBeenCalledTimes(1)) remains after the awaited assertion to
avoid race conditions.
| describe("API Lookup (Dynamic Versions)", () => { | ||
| it("should call API and resolve 'latest' to the newest version", async () => { | ||
| const url = await getDownloadUrl({ | ||
| version: "latest", | ||
| os: "linux", | ||
| arch: "x64", | ||
| }); | ||
|
|
||
| expect(url).toBe( | ||
| "https://github.com/oven-sh/bun/releases/download/bun-v1.1.0/bun-linux-x64.zip", | ||
| ); | ||
| expect(requestSpy).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for undefined/empty version.
The implementation handles version === "latest" || !version (line 50 in download-url.ts), but there's no test explicitly covering when version is undefined or an empty string.
💡 Example test case
it("should call API and resolve undefined version to latest", async () => {
const url = await getDownloadUrl({
version: undefined,
os: "linux",
arch: "x64",
});
expect(url).toBe(
"https://github.com/oven-sh/bun/releases/download/bun-v1.1.0/bun-linux-x64.zip",
);
expect(requestSpy).toHaveBeenCalledTimes(1);
});🤖 Prompt for AI Agents
In `@tests/download-url.spec.ts` around lines 92 - 104, Add a test that covers the
case where version is undefined or an empty string so the branch in
getDownloadUrl that treats "latest" || !version is exercised; in
tests/download-url.spec.ts add an it() that calls getDownloadUrl({ version:
undefined, os: "linux", arch: "x64" }) (and another variant for "" if desired),
asserts the same expected URL as the "latest" test, and verifies requestSpy was
called once to ensure the API lookup path is used.
| it("should throw error if semver range matches nothing", async () => { | ||
| expect( | ||
| getDownloadUrl({ | ||
| version: "^2.0.0", | ||
| os: "linux", | ||
| arch: "x64", | ||
| }), | ||
| ).rejects.toThrow("No Bun release found matching version '^2.0.0'"); | ||
| expect(requestSpy).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
Missing await for rejects assertion may cause unreliable test behavior.
The rejects.toThrow() assertion returns a promise that must be awaited. Without await, the test might pass even if the promise doesn't reject, and the requestSpy assertion on line 140 could execute before the rejection is verified.
🐛 Proposed fix
it("should throw error if semver range matches nothing", async () => {
- expect(
+ await expect(
getDownloadUrl({
version: "^2.0.0",
os: "linux",
arch: "x64",
}),
).rejects.toThrow("No Bun release found matching version '^2.0.0'");
expect(requestSpy).toHaveBeenCalledTimes(1);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should throw error if semver range matches nothing", async () => { | |
| expect( | |
| getDownloadUrl({ | |
| version: "^2.0.0", | |
| os: "linux", | |
| arch: "x64", | |
| }), | |
| ).rejects.toThrow("No Bun release found matching version '^2.0.0'"); | |
| expect(requestSpy).toHaveBeenCalledTimes(1); | |
| }); | |
| it("should throw error if semver range matches nothing", async () => { | |
| await expect( | |
| getDownloadUrl({ | |
| version: "^2.0.0", | |
| os: "linux", | |
| arch: "x64", | |
| }), | |
| ).rejects.toThrow("No Bun release found matching version '^2.0.0'"); | |
| expect(requestSpy).toHaveBeenCalledTimes(1); | |
| }); |
🤖 Prompt for AI Agents
In `@tests/download-url.spec.ts` around lines 132 - 141, The test calling
getDownloadUrl uses expect(...).rejects.toThrow without awaiting the returned
promise; update the test for the "should throw error if semver range matches
nothing" case to await the rejects assertion (i.e., await
expect(getDownloadUrl({...})).rejects.toThrow(...)) so the rejection is actually
verified before asserting requestSpy call count; ensure the requestSpy assertion
(requestSpy.toHaveBeenCalledTimes(1)) remains after the awaited assertion to
avoid race conditions.
1 participant
closes #159