Skip to content

fix: correctly detect type info in badge#2173

Open
gameroman wants to merge 37 commits intonpmx-dev:mainfrom
gameroman:fix-types-badge
Open

fix: correctly detect type info in badge#2173
gameroman wants to merge 37 commits intonpmx-dev:mainfrom
gameroman:fix-types-badge

Conversation

@gameroman
Copy link
Contributor

@gameroman gameroman commented Mar 20, 2026

🔗 Linked issue

Closes #1993

🧭 Context

Badges behaviour for type detection was simpler than for package page

📚 Description

Before

After

@vercel
Copy link

vercel bot commented Mar 20, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Mar 20, 2026 10:53pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Mar 20, 2026 10:53pm
npmx-lunaria Ignored Ignored Mar 20, 2026 10:53pm

Request Review

@codecov
Copy link

codecov bot commented Mar 20, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
shared/utils/package-analysis.ts 0.00% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@gameroman
Copy link
Contributor Author

Would it make sense to increase cache for badges from 1 to 4 hours?

@gameroman gameroman marked this pull request as ready for review March 20, 2026 17:19
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 20, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Centralises package JSON, types-package metadata and file-tree fetching into a new exported utility fetchPackageWithTypesAndFiles(packageName, version?) in server/utils/file-tree.ts and adds AnalysisPackageJson. The analysis route (server/api/registry/analysis/[...pkg].get.ts) now calls this utility and uses its returned { pkg, typesPackage, files } when invoking analyzePackage. The types badge strategy (server/api/registry/badge/[type]/[...pkg].get.ts) accepts an optional requestedVersion, short-circuits when a version has built-in types, otherwise calls fetchPackageWithTypesAndFiles and classifies the badge via detectTypesStatus, returning included, @types (with optional deprecated marker) or missing.

Possibly related PRs

Suggested reviewers

  • danielroe
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description clearly relates to the changeset, addressing badge type detection behaviour and linking to issue #1993.
Linked Issues check ✅ Passed The PR successfully implements the objective from issue #1993 by aligning badge type detection logic with package page logic through the new fetchPackageWithTypesAndFiles function and detectTypesStatus integration.
Out of Scope Changes check ✅ Passed All changes are directly related to resolving the type detection logic across badge and analysis endpoints, with no unrelated modifications present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 from analyzePackage(...) 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 same types status 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4ad0679 and 45e01cd.

📒 Files selected for processing (3)
  • server/api/registry/analysis/[...pkg].get.ts
  • server/api/registry/badge/[type]/[...pkg].get.ts
  • server/utils/file-tree.ts

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ab21b408-0a15-4dbb-8cc5-51518f42cd5d

📥 Commits

Reviewing files that changed from the base of the PR and between 45e01cd and cfbc5e1.

📒 Files selected for processing (1)
  • server/api/registry/badge/[type]/[...pkg].get.ts

@gameroman
Copy link
Contributor Author

@coderabbitai new browser test is still failing

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 20, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me look into the failing browser test.


🏁 Script executed:

#!/bin/bash
# Check PR status and CI checks
gh pr view 2173 --repo npmx-dev/npmx.dev --json statusCheckRollup,commits | jq '.'

# Also check for any recent check runs
gh pr checks 2173 --repo npmx-dev/npmx.dev 2>&1 | head -80

Length of output: 22658


Let me fetch the browser test failure logs.


🏁 Script executed:

#!/bin/bash
# Get the detailed logs for the failing browser test job
gh api repos/npmx-dev/npmx.dev/actions/jobs/67974789287/logs 2>&1 | head -200

Length of output: 16151


Let me get the tail of the logs where the failure occurs.


🏁 Script executed:

#!/bin/bash
# Get the logs and look for the failure section
gh api repos/npmx-dev/npmx.dev/actions/jobs/67974789287/logs 2>&1 | tail -200

Length of output: 17670


Let me examine the failing test and the related source code.


🏁 Script executed:

#!/bin/bash
# Look at the failing test
fd "badge.spec.ts" --exec cat {}

echo "---"

# Look at hasBuiltInTypes and detectTypesStatus
fd "package-analysis.ts" --exec cat {}

echo "---"

# Look at the file-tree utility
fd "file-tree.ts" --exec cat {}

Length of output: 23516


🏁 Script executed:

#!/bin/bash
# Look at the badge API handler
fd "\[...pkg\].get.ts" --search-path server/api/registry/badge --exec cat {}

echo "=== FIXTURE FILES ==="
# Look for nano-stringify-object fixture
fd "nano-stringify-object" --exec cat {}

Length of output: 24049


@gameroman: I found the root cause. Here's what's happening:

Root cause: The e2e test uses your real nano-stringify-object@0.0.0 package, but when the badge API calls jsDelivr to fetch the file tree (https://data.jsdelivr.com/v1/packages/npm/nano-stringify-object@0.0.0), jsDelivr either hasn't indexed this package or the published tarball doesn't include .d.ts / .d.mts files in dist/. Because Promise.allSettled silently swallows the failure, files ends up as undefined, and detectTypesStatus falls through to kind: 'none'"missing".

The unit-test fixture works fine because it mocks jsDelivr with a dist/index.d.ts file that satisfies getDeclCandidates('./dist/index.mjs'). But the e2e test hits the real server + real jsDelivr.

Two things to verify/fix:

  1. Does the real published tarball include a .d.mts or .d.ts file? Run:

    curl https://data.jsdelivr.com/v1/packages/npm/nano-stringify-object@0.0.0

    If jsDelivr returns a 404 or the files list doesn't contain dist/index.d.mts or dist/index.d.ts, that's the problem. You may need to publish a new version of the package that actually ships the compiled declaration files.

  2. There's also a mismatch in the fixture format. The fixture uses a flat array of paths:

    { "name": "dist/index.d.ts", ... }

    But getPackageFileTreeconvertToFileTree expects a nested jsDelivr tree structure where dist is a directory node with files children. This would cause the fixture to not work correctly for any tests that actually exercise convertToFileTree with it.

    The fixture should look like:

    {
      "files": [
        {
          "type": "directory",
          "name": "dist",
          "files": [
            { "type": "file", "name": "index.d.ts", "hash": "ghi", "size": 1200 },
            { "type": "file", "name": "index.js", "hash": "def", "size": 5700 }
          ]
        },
        { "type": "file", "name": "package.json", "hash": "abc", "size": 600 }
      ]
    }

For the e2e test specifically, you have a few options:

  • Publish a new version of nano-stringify-object that actually emits .d.mts declarations (since your build tool is tsdown, add dts: true to the config), then wait for jsDelivr to index it.
  • Or switch to a well-known public package that already exhibits this pattern (ships implicit types without an explicit types field) and is guaranteed to be on jsDelivr.
  • Or mock the jsDelivr network call in the Playwright global setup using page.route().

@gameroman
Copy link
Contributor Author

@coderabbitai just fixing the fixture should be enough?

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 20, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me check whether the e2e test also mocks jsDelivr calls.


🏁 Script executed:

#!/bin/bash
# Check mock-routes.cjs for jsDelivr mocking
fd "mock-routes.cjs" --exec cat {}

echo "=== badge.spec.ts ==="
fd "badge.spec.ts" --exec cat {}

echo "=== any other mock/fixture files referencing jsdelivr ==="
rg -i "jsdelivr" --type=js --type=ts -n -C3

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:

#!/bin/bash
# Read the full jsDelivr section of modules/runtime/server/cache.ts
fd "cache.ts" --search-path modules --exec cat {}

echo "=== Check if nano-stringify-object fixture exists ==="
fd "nano-stringify-object" test/

echo "=== Check the nano-stringify-object packument fixture ==="
fd "nano-stringify-object.json" --exec cat {}

Length of output: 35552


@gameroman: Not quite — and actually, the fixture already exists and is correct. The real problem is in modules/runtime/server/cache.ts.

Here's the flow:

  1. The badge API route is server-side, so its fetchFileTree call to data.jsdelivr.com goes through the globalThis.fetch override in cache.ts, not through Playwright's mock-routes.cjs.
  2. cache.ts's getMockForUrl() handles data.jsdelivr.com with a hardcoded stub that always returns package.json, index.js, README.md — never any .d.ts files:
    files: [
      { name: 'package.json', hash: 'abc123', size: 1000 },
      { name: 'index.js', hash: 'def456', size: 500 },
      { name: 'README.md', hash: 'ghi789', size: 2000 },
    ]
  3. Because getMockForUrl returns a non-null value, fetchFromFixtures short-circuits and never reads test/fixtures/jsdelivr/nano-stringify-object.json (which already has the correct dist/index.d.mts entry).

The fix is to update cache.ts to mirror the pattern already used for handleFastNpmMeta: remove the jsDelivr hardcode from the synchronous getMockForUrl, and add an async handleJsdelivrDataApi that reads from storage first (falling back to the stub if no fixture exists). Here's what that would look like:

// 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 fetchFromFixtures right after the fast-npm-meta check:

// 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, test/fixtures/jsdelivr/nano-stringify-object.json will finally be served to the badge API, flattenFileTree will include dist/index.d.mts, and detectTypesStatus will return kind: 'included' as expected.

Would you like me to plan these code changes?


✏️ Learnings added
Learnt from: gameroman
URL: https://github.com/npmx-dev/npmx.dev/pull/2173

Timestamp: 2026-03-20T22:32:30.435Z
Learning: In the npmx.dev repository, the server-side test fixture interception for `data.jsdelivr.com` (jsDelivr Data API) in `modules/runtime/server/cache.ts` (`getMockForUrl`) always returns a hardcoded stub (only `package.json`, `index.js`, `README.md`) and does NOT read from `test/fixtures/jsdelivr/*.json` fixture files. Per-package jsDelivr fixture files (used by `mock-routes.cjs` for Playwright browser-level interception) are therefore ignored for server-side API routes. Fixing the badge types e2e test requires adding an async `handleJsdelivrDataApi` function in `cache.ts` that reads from storage (similar to `handleFastNpmMeta`), and wiring it into `fetchFromFixtures` so server-side fetches to `data.jsdelivr.com` also benefit from per-package fixtures.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: gameroman
Repo: npmx-dev/npmx.dev PR: 2173
File: server/api/registry/badge/[type]/[...pkg].get.ts:376-376
Timestamp: 2026-03-20T21:28:36.181Z
Learning: In the npmx.dev repository, `fetchPackageWithTypesAndFiles` in `server/utils/file-tree.ts` requests `${NPM_REGISTRY}/${packageName}/latest` from the npm registry. The E2E test mock (`matchNpmRegistry` in `test/fixtures/mock-routes.cjs`) strips the version suffix and returns the full packument fixture. Because a packument's version-specific fields (like `exports`, `type`, `main`) live under `versions["x.y.z"]` and not at the packument root, `pkg.exports` will be `undefined` when the mock response is used. For file-based types detection tests that exercise `fetchPackageWithTypesAndFiles`, the packument fixture must hoist `exports`, `type`, `version`, and `main` to the root level of the JSON file.

Learnt from: gameroman
Repo: npmx-dev/npmx.dev PR: 2135
File: test/e2e/interactions.spec.ts:89-90
Timestamp: 2026-03-19T13:44:54.800Z
Learning: In the npmx.dev repository, E2E tests under `test/e2e/` that hit internal API routes (e.g. `/api/registry/analysis/`, `/api/registry/install-size/`) are backed by static JSON fixtures in `test/fixtures/npm-registry/packuments/`. The `nuxt/kit` packument fixture pins `latest` to `4.3.0` with `"type": "module"` and `.mjs` exports, so asserting `moduleFormat === 'esm'` for `nuxt/kit` in E2E tests is stable and not flaky. Do not suggest weakening such assertions on the grounds of live-data brittleness.

@gameroman
Copy link
Contributor Author

@coderabbitai All tests pass, do a final check

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 20, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR diff and changed files
gh pr view 2173 --repo npmx-dev/npmx.dev --json files,statusCheckRollup | jq '.files[].path, .statusCheckRollup[].conclusion'

echo "=== Full diff ==="
gh pr diff 2173 --repo npmx-dev/npmx.dev

Length of output: 35469


@gameroman: All checks are green and the implementation looks correct. Here's a summary of the final state:

✅ All systems good:

Area Status
cache.ts jsDelivr fix Hardcoded stub removed; handleJsdelivrDataApi correctly reads per-package fixtures from storage with generic fallback
file-tree.ts extraction fetchPackageWithTypesAndFiles cleanly consolidates the npm fetch + @types lookup + jsDelivr file-tree into one reusable function
analysis/[...pkg].get.ts Properly delegates to shared helper; no duplicated logic
Badge types strategy Correct multi-state flow: fast-path hasBuiltInTypes → full fetchPackageWithTypesAndFiles + detectTypesStatusincluded / @types / @types (deprecated) / missing
detectTypesStatus simplification hasBuiltInTypes subsumes the old inline exports/analyzeExports check — no regression
Fixtures nano-stringify-object.json has root-level exports/type/version hoisted; jsDelivr fixture has proper nested tree with dist/index.d.mts; @types/is-odd packument is present
E2E tests Two new badge type assertions cover the @types and implicit-types-included paths
mock-routes.cjs Browser-level jsDelivr interception now also reads per-package fixtures

One minor nit (non-blocking): there's a typo in the new test description — "explictly" should be "explicitly".

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
Learnt from: gameroman
Repo: npmx-dev/npmx.dev PR: 0
File: :0-0
Timestamp: 2026-03-20T22:32:30.435Z
Learning: In the npmx.dev repository, the server-side test fixture interception for `data.jsdelivr.com` (jsDelivr Data API) in `modules/runtime/server/cache.ts` (`getMockForUrl`) always returns a hardcoded stub (only `package.json`, `index.js`, `README.md`) and does NOT read from `test/fixtures/jsdelivr/*.json` fixture files. Per-package jsDelivr fixture files (used by `mock-routes.cjs` for Playwright browser-level interception) are therefore ignored for server-side API routes. Fixing the badge types e2e test requires adding an async `handleJsdelivrDataApi` function in `cache.ts` that reads from storage (similar to `handleFastNpmMeta`), and wiring it into `fetchFromFixtures` so server-side fetches to `data.jsdelivr.com` also benefit from per-package fixtures.

Learnt from: gameroman
Repo: npmx-dev/npmx.dev PR: 2135
File: test/e2e/interactions.spec.ts:89-90
Timestamp: 2026-03-19T13:44:54.800Z
Learning: In the npmx.dev repository, E2E tests under `test/e2e/` that hit internal API routes (e.g. `/api/registry/analysis/`, `/api/registry/install-size/`) are backed by static JSON fixtures in `test/fixtures/npm-registry/packuments/`. The `nuxt/kit` packument fixture pins `latest` to `4.3.0` with `"type": "module"` and `.mjs` exports, so asserting `moduleFormat === 'esm'` for `nuxt/kit` in E2E tests is stable and not flaky. Do not suggest weakening such assertions on the grounds of live-data brittleness.

Learnt from: gameroman
Repo: npmx-dev/npmx.dev PR: 2173
File: server/api/registry/badge/[type]/[...pkg].get.ts:376-376
Timestamp: 2026-03-20T21:28:36.181Z
Learning: In the npmx.dev repository, `fetchPackageWithTypesAndFiles` in `server/utils/file-tree.ts` requests `${NPM_REGISTRY}/${packageName}/latest` from the npm registry. The E2E test mock (`matchNpmRegistry` in `test/fixtures/mock-routes.cjs`) strips the version suffix and returns the full packument fixture. Because a packument's version-specific fields (like `exports`, `type`, `main`) live under `versions["x.y.z"]` and not at the packument root, `pkg.exports` will be `undefined` when the mock response is used. For file-based types detection tests that exercise `fetchPackageWithTypesAndFiles`, the packument fixture must hoist `exports`, `type`, `version`, and `main` to the root level of the JSON file.

@gameroman
Copy link
Contributor Author

Ready to be merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

types badge shows incorrect info

1 participant