fix: fall back to unpkg for oversized packages - #3161
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughPackage file access now supports jsDelivr and unpkg fallback, bounded response reads, validated metadata, and shared URL construction. Registry, file-tree, skill, and package-code routes use these utilities. ChangesPackage file retrieval
Sequence Diagram(s)sequenceDiagram
participant PackageCodePage
participant RegistryRoute
participant fetchPackageFile
participant jsDelivr
participant unpkg
PackageCodePage->>RegistryRoute: request package file
RegistryRoute->>fetchPackageFile: fetch package file
fetchPackageFile->>jsDelivr: request primary URL
jsDelivr-->>fetchPackageFile: 403 response
fetchPackageFile->>unpkg: request fallback URL
unpkg-->>fetchPackageFile: bounded package response
fetchPackageFile-->>RegistryRoute: provider and response
RegistryRoute-->>PackageCodePage: file content or error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Hello! Thank you for opening your first PR to npmx, @LubuSeb! 🚀 Here’s what will happen next:
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
server/utils/skills.ts (1)
122-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or expand this comment.
The comment only repeats the function purpose. It does not explain complex or non-obvious logic. Remove it, or document the provider fallback and bounded-read behaviour.
As per coding guidelines, “Add comments only to explain complex logic or non-obvious implementations.”
🤖 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 `@server/utils/skills.ts` around lines 122 - 123, Update the comment above the package-content fetch function by either removing it or expanding it to document the provider fallback and bounded-read behavior; do not retain the current generic purpose-only wording.Source: Coding guidelines
server/api/registry/compare/[...pkg].get.ts (1)
17-26: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe catch block hides aborts and size errors.
Line 24 converts every failure into
null. The caller cannot distinguish a missingpackage.jsonfrom a timeout or from an oversized response. The fallback adds a second sequential request, so aborts are now more likely on this path. Re-throwAbortErrorand let the caller report the timeout.🤖 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 `@server/api/registry/compare/`[...pkg].get.ts around lines 17 - 26, Update the catch block surrounding fetchPackageFile and readPackageResponseText to re-throw AbortError instances, while continuing to return null for other failures. Preserve the existing response and JSON parsing behavior so missing or invalid package metadata still uses the null fallback.test/unit/server/utils/package-files.spec.ts (1)
125-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the null-body branch.
readPackageResponseTexthas a separate branch forresponse.body === nullat server/utils/package-files.ts Lines 35-40. The current tests never reach it, because every fixture has a body. Add one case with a null body and an oversized text payload to lock theBuffer.byteLengthcheck.🤖 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 `@test/unit/server/utils/package-files.spec.ts` around lines 125 - 147, Add a test in the readPackageResponseText suite using a Response fixture whose body is null and whose text payload exceeds the byte limit, then assert rejection with PackageResponseTooLargeError and the expected Buffer.byteLength-based sizeBytes value. Ensure this exercises the response.body === null branch without changing the existing tests.server/utils/package-files.ts (2)
72-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the provider identifiers instead of hardcoding them.
fetchWithFallbackreceives generic URLs but returns the literals'jsdelivr'and'unpkg'. The returned provider and the supplied URL can drift if a caller ever changes the primary provider. Accept the two providers as parameters and derive the URLs inside the helper.🤖 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 `@server/utils/package-files.ts` around lines 72 - 88, Update fetchWithFallback to accept the primary and fallback provider identifiers instead of generic URLs, derive each provider URL inside the helper, and return those parameters in the provider fields. Replace the hardcoded 'jsdelivr' and 'unpkg' values while preserving the existing 403 fallback and response-body cancellation behavior.
72-88: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a default timeout for the provider requests.
fetchWithFallbackmakes up to two sequentialfetchcalls. Thesignalparameter is optional.server/api/registry/file/[...pkg].get.tscallsfetchPackageFilewithout a signal at Line 46 and Line 62, so a slow CDN can hold the request until the platform limit. The fallback path doubles the worst-case latency compared with the previous single jsDelivr request.Add a default timeout when the caller supplies no signal, or require a signal from every caller.
♻️ Example: apply a default timeout
+const DEFAULT_PACKAGE_FETCH_TIMEOUT_MS = 10_000 + async function fetchWithFallback( primaryUrl: string, fallbackUrl: string, signal?: AbortSignal, ): Promise<PackageFetchResult> { - const primary = await fetch(primaryUrl, { signal }) + const requestSignal = signal ?? AbortSignal.timeout(DEFAULT_PACKAGE_FETCH_TIMEOUT_MS) + const primary = await fetch(primaryUrl, { signal: requestSignal }) if (primary.status !== 403) { if (!primary.ok) await cancelResponseBody(primary) return { provider: 'jsdelivr', response: primary } } await cancelResponseBody(primary) - const fallback = await fetch(fallbackUrl, { signal }) + const fallback = await fetch(fallbackUrl, { signal: requestSignal }) if (!fallback.ok) await cancelResponseBody(fallback) return { provider: 'unpkg', response: fallback } }🤖 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 `@server/utils/package-files.ts` around lines 72 - 88, Update fetchWithFallback so provider fetches cannot run indefinitely when signal is omitted: create and use a default timeout signal for both sequential fetch calls, while preserving the caller-provided signal when present. Ensure the timeout resource is cleaned up after the primary/fallback flow completes.test/unit/server/utils/file-tree.spec.ts (1)
136-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the global cleanup into
afterEach.Every new test wraps its assertions in
try/finallyonly to callvi.unstubAllGlobals(). A singleafterEach(() => vi.unstubAllGlobals())in this describe block removes the repetition and keeps each test focused on its assertions.test/unit/server/utils/package-files.spec.tsalready uses that pattern at Lines 9-11.Also applies to: 207-375
🤖 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 `@test/unit/server/utils/file-tree.spec.ts` around lines 136 - 160, Move vi.unstubAllGlobals() cleanup from the repeated try/finally blocks in the getPackageFileTree tests into a single afterEach hook within the describe block. Remove the per-test finally wrappers while preserving each test’s existing assertions and fetchMock checks.server/api/registry/file/[...pkg].get.ts (1)
44-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo identical
fetchPackageJsonhelpers now exist. Both routes define the same helper with the same 2 MiB constant and the samecatch { return null }. The duplication grew with this PR, because both now call the shared fetcher and reader.
server/api/registry/file/[...pkg].get.ts#L44-L52: move this helper andMAX_PACKAGE_JSON_SIZEintoserver/utils/package-files.tsand import it here.server/api/registry/compare/[...pkg].get.ts#L17-L26: import the shared helper and delete the local copy and constant.🤖 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 `@server/api/registry/file/`[...pkg].get.ts around lines 44 - 52, Move fetchPackageJson and MAX_PACKAGE_JSON_SIZE from server/api/registry/file/[...pkg].get.ts:44-52 into server/utils/package-files.ts, export them, and import the shared helper here. In server/api/registry/compare/[...pkg].get.ts:17-26, import the shared helper and remove its local fetchPackageJson implementation and constant.
🤖 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.
Inline comments:
In `@server/api/registry/file/`[...pkg].get.ts:
- Around line 80-85: Update the error message in the
PackageResponseTooLargeError handler to report both the actual file size and
MAX_FILE_SIZE using the same unit, preserving the existing formatting and 413
response.
In `@server/utils/file-tree.ts`:
- Around line 185-190: Update the non-OK response handling to map a 404 from
either the jsDelivr or fallback provider to the existing “Package or version not
found” 404 error. Preserve invalidFileListError() for all other response
statuses.
- Around line 114-121: Normalize the hash assigned in the PackageFileTree
construction within the file-tree builder so UNPKG SRI values are converted from
base64 to the same hex SHA-256 encoding returned by jsDelivr before exposing
hash. Preserve existing duplicate-path handling and file metadata, and ensure
equivalent digests compare identically across providers.
- Around line 45-60: The Unpkg metadata validation and conversion flow must
tolerate directory entries and unsupported integrity prefixes. Update the
metadata handling used by getPackageFileTree to filter out non-file/listing
entries before safeParse, and update convertUnpkgToFileTree to skip or
explicitly reject entries whose integrity does not use the supported prefix
instead of blindly slicing sha256-; preserve valid file conversion and
oversized-package handling.
In `@shared/utils/package-files.ts`:
- Around line 15-35: Validate packageName at the start of both getPackageFileUrl
and getPackageMetadataUrl before constructing packageSpec, rejecting values
containing path traversal or URL-delimiter characters such as .., ?, or #.
Ensure invalid names cannot reach PACKAGE_FILE_BASE_URLS or the metadata CDN
URLs, while preserving valid package-name behavior.
---
Nitpick comments:
In `@server/api/registry/compare/`[...pkg].get.ts:
- Around line 17-26: Update the catch block surrounding fetchPackageFile and
readPackageResponseText to re-throw AbortError instances, while continuing to
return null for other failures. Preserve the existing response and JSON parsing
behavior so missing or invalid package metadata still uses the null fallback.
In `@server/api/registry/file/`[...pkg].get.ts:
- Around line 44-52: Move fetchPackageJson and MAX_PACKAGE_JSON_SIZE from
server/api/registry/file/[...pkg].get.ts:44-52 into
server/utils/package-files.ts, export them, and import the shared helper here.
In server/api/registry/compare/[...pkg].get.ts:17-26, import the shared helper
and remove its local fetchPackageJson implementation and constant.
In `@server/utils/package-files.ts`:
- Around line 72-88: Update fetchWithFallback to accept the primary and fallback
provider identifiers instead of generic URLs, derive each provider URL inside
the helper, and return those parameters in the provider fields. Replace the
hardcoded 'jsdelivr' and 'unpkg' values while preserving the existing 403
fallback and response-body cancellation behavior.
- Around line 72-88: Update fetchWithFallback so provider fetches cannot run
indefinitely when signal is omitted: create and use a default timeout signal for
both sequential fetch calls, while preserving the caller-provided signal when
present. Ensure the timeout resource is cleaned up after the primary/fallback
flow completes.
In `@server/utils/skills.ts`:
- Around line 122-123: Update the comment above the package-content fetch
function by either removing it or expanding it to document the provider fallback
and bounded-read behavior; do not retain the current generic purpose-only
wording.
In `@test/unit/server/utils/file-tree.spec.ts`:
- Around line 136-160: Move vi.unstubAllGlobals() cleanup from the repeated
try/finally blocks in the getPackageFileTree tests into a single afterEach hook
within the describe block. Remove the per-test finally wrappers while preserving
each test’s existing assertions and fetchMock checks.
In `@test/unit/server/utils/package-files.spec.ts`:
- Around line 125-147: Add a test in the readPackageResponseText suite using a
Response fixture whose body is null and whose text payload exceeds the byte
limit, then assert rejection with PackageResponseTooLargeError and the expected
Buffer.byteLength-based sizeBytes value. Ensure this exercises the response.body
=== null branch without changing the existing tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7d7fe28-d9ec-4dbb-925c-ef12ed13a4f4
📒 Files selected for processing (18)
app/components/Code/Header.vueapp/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vuemodules/runtime/server/cache.tsserver/api/registry/compare-file/[...pkg].get.tsserver/api/registry/compare/[...pkg].get.tsserver/api/registry/file/[...pkg].get.tsserver/api/registry/files/[...pkg].get.tsserver/utils/file-tree.tsserver/utils/package-files.tsserver/utils/skills.tsshared/types/npm-registry.tsshared/utils/package-files.tstest/fixtures/mock-routes.cjstest/nuxt/pages/PackageCodePage.spec.tstest/unit/server/utils/file-tree.spec.tstest/unit/server/utils/package-files.spec.tstest/unit/server/utils/skills.spec.tstest/unit/shared/utils/package-files.spec.ts
908530c to
098a59b
Compare
serhalp
added
needs review
2 participants
🔗 Linked issue
Fixes #2899
🧭 Context
next@16.2.9is larger than jsDelivr's 150 MB package limit. Its metadata and file requests return 403, which npmx currently surfaces as 502.I kept jsDelivr as the primary provider. When it returns 403, npmx now retries the same package metadata or file request through UNPKG. Other statuses keep their existing behavior.
📚 Description
UNPKG returns a flat metadata list, so the fallback validates and converts it into the file tree npmx already expects. The shared provider logic covers file trees, individual files, comparisons and skills processing.
The fallback is bounded rather than open-ended:
Package versions and file path segments are encoded before they are added to provider URLs. Raw-file actions use UNPKG's file viewer so they still work when jsDelivr rejects the whole package. The current API response does not expose the selected provider, so the viewer link cannot switch dynamically without widening that response.
What I checked:
next@16.2.9file tree: 502 before, 200 after with 8,076 filesnext@16.2.9/package.json: 502 before, 200 aftervue@3.5.28control: 200 through jsDelivr with no fallbackBefore
After