fix: correctly detect type info in badge#2173
fix: correctly detect type info in badge#2173gameroman wants to merge 37 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Would it make sense to increase cache for badges from 1 to 4 hours? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralises package JSON, types-package metadata and file-tree fetching into a new exported utility Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/api/registry/badge/[type]/[...pkg].get.ts (1)
376-409: Please add a regression test for badge/package parity.This is the exact drift that caused
#1993, but there is still no coverage here to stop the badge from diverging fromanalyzePackage(...)again. An API or E2E test that exercises the badge route and the analysis route for the same fixture-backed package/version and asserts the sametypesstatus would lock the behaviour in.Based on learnings, fixture-backed internal API E2E tests are stable in this repository, so this sort of parity assertion should not be flaky.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b2fa079-10f5-41db-9b56-1f98c7c60032
📒 Files selected for processing (3)
server/api/registry/analysis/[...pkg].get.tsserver/api/registry/badge/[type]/[...pkg].get.tsserver/utils/file-tree.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab21b408-0a15-4dbb-8cc5-51518f42cd5d
📒 Files selected for processing (1)
server/api/registry/badge/[type]/[...pkg].get.ts
This reverts commit 31e5fed.
… into fix-types-badge
|
@coderabbitai new browser test is still failing |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me look into the failing browser test. 🏁 Script executed: Length of output: 22658 Let me fetch the browser test failure logs. 🏁 Script executed: Length of output: 16151 Let me get the tail of the logs where the failure occurs. 🏁 Script executed: Length of output: 17670 Let me examine the failing test and the related source code. 🏁 Script executed: Length of output: 23516 🏁 Script executed: Length of output: 24049
Root cause: The e2e test uses your real The unit-test fixture works fine because it mocks jsDelivr with a Two things to verify/fix:
For the e2e test specifically, you have a few options:
|
|
@coderabbitai just fixing the fixture should be enough? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check whether the e2e test also mocks jsDelivr calls. 🏁 Script executed: Length of output: 58420 Let me check the server-side jsDelivr mock in the cache module, since the badge API makes server-side fetches which wouldn't go through Playwright's route interception. 🏁 Script executed: Length of output: 35552
Here's the flow:
The fix is to update // In cache.ts — remove the data.jsdelivr.com block from getMockForUrl() entirely,
// then add:
async function handleJsdelivrDataApi(
url: string,
storage: ReturnType<typeof useStorage>,
): Promise<MockResult | null> {
let urlObj: URL
try { urlObj = new URL(url) } catch { return null }
if (urlObj.host !== 'data.jsdelivr.com') return null
const packageMatch = decodeURIComponent(urlObj.pathname).match(/^\/v1\/packages\/npm\/(.+)$/)
if (!packageMatch?.[1]) return null
const parsed = parseScopedPackageWithVersion(packageMatch[1])
// Try per-package fixture first
const fixturePath = `jsdelivr:${parsed.name.replace(/\//g, ':')}.json`
const fixture = await storage.getItem<unknown>(fixturePath)
if (fixture) {
return { data: fixture }
}
// Fall back to generic stub (no declaration files)
return {
data: {
type: 'npm',
name: parsed.name,
version: parsed.version || 'latest',
files: [
{ name: 'package.json', hash: 'abc123', size: 1000 },
{ name: 'index.js', hash: 'def456', size: 500 },
{ name: 'README.md', hash: 'ghi789', size: 2000 },
],
},
}
}And wire it into // after: const fastNpmMetaResult = await handleFastNpmMeta(url, storage)
const jsdelivrResult = await handleJsdelivrDataApi(url, storage)
if (jsdelivrResult) {
if (VERBOSE) process.stdout.write(`[test-fixtures] jsDelivr Data API: ${url}\n`)
return { data: jsdelivrResult.data as T, isStale: false, cachedAt: Date.now() }
}With this, Would you like me to plan these code changes? ✏️ Learnings added
🧠 Learnings used |
|
@coderabbitai All tests pass, do a final check |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 35469
✅ All systems good:
One minor nit (non-blocking): there's a typo in the new test description — Regarding the cache duration question (1h → 4h) — that wasn't included in this PR, which is fine. Happy to open a follow-up issue if you'd like to track it. [approve] 🧠 Learnings used |
|
Ready to be merged |
🔗 Linked issue
Closes #1993
🧭 Context
Badges behaviour for type detection was simpler than for package page
📚 Description
Before
After