chore(docs): add adoption metrics generation and service#156
Open
coryrylan wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
scope(internals)
scope(docs)
dependencies
|
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:
📝 WalkthroughWalkthroughAdds public adoption metrics generation, caching, and dashboard rendering, and changes the metrics page release charts to weekly aggregation. ChangesAdoption metrics pipeline
Adoption dashboard
Weekly release metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. 🔧 Biome (2.5.1)projects/internals/metadata/static/adoption.jsonFile contains syntax errors that prevent linting: Line 1: String values must be double quoted.; Line 1: String values must be double quoted.; Line 1: End of file expected; Line 2: End of file expected; Line 2: String values must be double quoted.; Line 3: String values must be double quoted.; Line 3: End of file expected projects/internals/metadata/static/releases.jsonFile contains syntax errors that prevent linting: Line 1: String values must be double quoted.; Line 1: String values must be double quoted.; Line 1: End of file expected; Line 2: End of file expected; Line 2: String values must be double quoted.; Line 3: String values must be double quoted.; Line 3: End of file expected 🔧 OpenGrep (1.23.0)projects/internals/metadata/vitest.config.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.29][ERROR]: unable to find a config; path projects/site/src/docs/metrics/adoption-data.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.15][ERROR]: unable to find a config; path projects/site/package.json┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.30][ERROR]: unable to find a config; path
🔧 markdownlint-cli2 (0.22.1)projects/internals/metadata/README.mdmarkdownlint-cli2 wrapper config was not available before execution 🔧 ESLint
projects/internals/metadata/src/services/adoption.service.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. projects/internals/metadata/src/services/releases.service.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. projects/internals/metadata/src/services/wireit.service.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'.
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@projects/internals/metadata/README.md`:
- Line 41: The README entry for AdoptionService overstates runtime behavior by
implying a CDN/fallback path that the actual AdoptionService implementation does
not provide. Update the documentation to accurately reflect the current
local-only loading from static/adoption.json, or implement the described
dual-loading/CDN fallback in adoption.service.ts and then document that behavior
consistently. Use the AdoptionService symbol and its adoption.json loading
behavior as the source of truth.
In `@projects/internals/metadata/src/services/adoption.service.test.ts`:
- Line 15: The test in AdoptionService.getData is asserting shared object
identity with toBe(adoption), which ties the test to the service returning the
same cached reference. Update the expectation to verify equivalent contents
instead of the exact object instance, so the test still passes if
AdoptionService.getData returns a defensive clone or frozen copy.
In `@projects/internals/metadata/src/services/adoption.service.ts`:
- Around line 36-42: The getData method in AdoptionService is returning the
cached `#adoption` object by reference, which lets callers mutate shared state.
Update getData so it returns a defensive copy or an immutable/frozen payload
before exposing it, while keeping the internal `#adoption` cache private and
unchanged for later reads. Use the AdoptionService.#adoption field and getData
method as the entry points for the fix.
In `@projects/internals/metadata/src/tasks/adoption.ts`:
- Around line 7-9: The adoption export in getAdoptionData/writeFileSync is using
a cwd-dependent path, so resolve the output location relative to this module
instead of "./static/adoption.json". Update the adoption task to derive the
static file path from the current file/module location and write the JSON there
so it always lands where the service reads it, regardless of where the task is
run from.
In `@projects/internals/metadata/src/tasks/adoption.utils.ts`:
- Around line 358-369: Cap the GitHub stargazer page fetch burst in the adoption
task by replacing the unbounded Promise.all fan-out in the stargazer pagination
logic with a bounded/concurrent approach. Update the pagination fetch flow
around parseGitHubPaginationTotal and fetchPublicJson so only a limited number
of page requests run at once, while still collecting all pages into stargazers.
- Around line 387-415: The adoption summary branch in `buildAdoptionStatus` is
discarding successful npm/CDN data whenever `parseNpmRegistry` returns null.
Change the `!registry` path so it still uses
`parseNpmDownloads(downloadsResult.data)` and
`parseJsDelivrStats(cdnResult.data, ...)` when those results are OK, and only
fall back to `createEmptyNpmDownloads()` / `createEmptyCdnStats()` for the
sources that actually failed; keep `errors` and status handling based on
`registryResult`, `downloadsResult`, and `cdnResult`.
In `@projects/internals/metadata/static/adoption.json`:
- Around line 1-3: The checked-in adoption data artifact is a Git LFS pointer
instead of valid JSON, so `AdoptionService.getData()` cannot parse it in
environments without LFS materialization. Replace the pointer contents with the
actual JSON payload (or change `AdoptionService.getData()` to load from a
non-LFS source) so the imported `adoption.json` is valid at runtime and can be
parsed reliably.
In `@projects/site/src/docs/metrics/adoption-data.test.ts`:
- Around line 101-154: The adoption-data tests are missing coverage for the
untested helper paths, so add a case that exercises getReleaseAdoptionTimeline
and a CDN share case with unsorted versions. Update the adoptionData test suite
to assert marker alignment and leading-version selection through
getReleaseAdoptionTimeline and getCdnVersionShareRows, using the existing
adoptionFixture-style helpers so regressions in sorting or latest-version
detection are caught.
In `@projects/site/src/docs/metrics/adoption-data.ts`:
- Around line 117-121: The helper getTopNonLatestVersionData is selecting the
first non-latest version instead of the true leading one, so the adoption row
can report the wrong CDN version and totals. Update this function to choose the
non-latest entry with the highest share/request count from
packageData.cdn.versions, rather than using find. Keep the latestVersion
exclusion, but compare the remaining versions by the same metric used elsewhere
in the adoption data model so the returned AdoptionCdnVersionLike is the actual
leader.
In `@projects/site/src/docs/metrics/index.11ty.js`:
- Around line 19-21: The adoption status line is reusing the releases timestamp
instead of the adoption data timestamp, so the metrics page can show the wrong
freshness for adoption totals/charts. Update the metrics template in the docs
page to use the adoption-specific formatted date variable created from
adoption.created (the adoptionReportDate symbol) wherever the adoption section’s
status line is rendered, and keep releasesReportDate only for release data.
In `@projects/site/src/docs/metrics/release-data.test.ts`:
- Around line 15-45: The current `getWeeklyReleaseMilestones` test only covers
straightforward same-month dates, so add a regression case in
`release-data.test.ts` that exercises a week boundary with releases on a
Sunday/Monday rollover and across a month or year change. Extend the
`describe('release chart data')` coverage with a new `it(...)` that asserts the
correct `weekKey`, `formattedDate`, `releasesThisWeek`, and `cumulativeTotal`
for `getWeeklyReleaseMilestones`, using `releaseFixture`-style inputs that prove
Monday-based bucketing stays correct across boundaries.
In `@projects/site/src/docs/metrics/release-data.ts`:
- Around line 23-27: The weekly date label formatter in WEEK_START_FORMATTER
drops the year, causing repeated labels across calendar years. Update the
formatting used by formattedDate in release-data.ts so week labels include the
year for both the chart axis and tooltip title, and apply the same change
anywhere the shared weekly label format is reused.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: 94968d36-e81c-4234-bffc-3d761c9ac3cd
📒 Files selected for processing (23)
projects/internals/metadata/README.mdprojects/internals/metadata/package.jsonprojects/internals/metadata/src/index.tsprojects/internals/metadata/src/services/adoption.service.test.tsprojects/internals/metadata/src/services/adoption.service.tsprojects/internals/metadata/src/services/releases.service.test.tsprojects/internals/metadata/src/tasks/adoption.tsprojects/internals/metadata/src/tasks/adoption.utils.test.tsprojects/internals/metadata/src/tasks/adoption.utils.tsprojects/internals/metadata/src/types.tsprojects/internals/metadata/static/adoption.jsonprojects/internals/metadata/static/releases.jsonprojects/internals/metadata/vitest.config.tsprojects/site/package.jsonprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/metrics/adoption-data.test.tsprojects/site/src/docs/metrics/adoption-data.tsprojects/site/src/docs/metrics/adoption.tsprojects/site/src/docs/metrics/bundle-explorer.11ty.jsprojects/site/src/docs/metrics/index.11ty.jsprojects/site/src/docs/metrics/index.tsprojects/site/src/docs/metrics/release-data.test.tsprojects/site/src/docs/metrics/release-data.ts
Comment on lines
+358
to
+369
| const totalPages = parseGitHubPaginationTotal(firstPage.headers.get('link'), 1); | ||
| const remainingPages = await Promise.all( | ||
| Array.from({ length: Math.max(totalPages - 1, 0) }, (_, index) => | ||
| fetchPublicJson( | ||
| `https://api.github.com/repos/${repository}/stargazers?per_page=100&page=${index + 2}`, | ||
| 'github', | ||
| { | ||
| ...headers, | ||
| Accept: 'application/vnd.github.star+json' | ||
| } | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cap GitHub stargazer pagination concurrency.
Promise.all fires one request per remaining page immediately. As the repo grows, that burst can hit GitHub secondary rate limits and leave stargazers partial even though the task keeps running.
Suggested fix
- const remainingPages = await Promise.all(
- Array.from({ length: Math.max(totalPages - 1, 0) }, (_, index) =>
- fetchPublicJson(
- `https://api.github.com/repos/${repository}/stargazers?per_page=100&page=${index + 2}`,
- 'github',
- {
- ...headers,
- Accept: 'application/vnd.github.star+json'
- }
- )
- )
- );
+ const remainingPages: PublicJsonResult[] = [];
+ for (let page = 2; page <= totalPages; page += 1) {
+ remainingPages.push(
+ await fetchPublicJson(`https://api.github.com/repos/${repository}/stargazers?per_page=100&page=${page}`, 'github', {
+ ...headers,
+ Accept: 'application/vnd.github.star+json'
+ })
+ );
+ }📝 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.
Suggested change
| const totalPages = parseGitHubPaginationTotal(firstPage.headers.get('link'), 1); | |
| const remainingPages = await Promise.all( | |
| Array.from({ length: Math.max(totalPages - 1, 0) }, (_, index) => | |
| fetchPublicJson( | |
| `https://api.github.com/repos/${repository}/stargazers?per_page=100&page=${index + 2}`, | |
| 'github', | |
| { | |
| ...headers, | |
| Accept: 'application/vnd.github.star+json' | |
| } | |
| ) | |
| ) | |
| const totalPages = parseGitHubPaginationTotal(firstPage.headers.get('link'), 1); | |
| const remainingPages: PublicJsonResult[] = []; | |
| for (let page = 2; page <= totalPages; page += 1) { | |
| remainingPages.push( | |
| await fetchPublicJson(`https://api.github.com/repos/${repository}/stargazers?per_page=100&page=${page}`, 'github', { | |
| ...headers, | |
| Accept: 'application/vnd.github.star+json' | |
| }) | |
| ); | |
| } |
🤖 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 `@projects/internals/metadata/src/tasks/adoption.utils.ts` around lines 358 -
369, Cap the GitHub stargazer page fetch burst in the adoption task by replacing
the unbounded Promise.all fan-out in the stargazer pagination logic with a
bounded/concurrent approach. Update the pagination fetch flow around
parseGitHubPaginationTotal and fetchPublicJson so only a limited number of page
requests run at once, while still collecting all pages into stargazers.
Comment on lines
+117
to
+121
| function getTopNonLatestVersionData( | ||
| packageData: AdoptionPackageLike, | ||
| latestVersion: string | ||
| ): AdoptionCdnVersionLike | undefined { | ||
| return packageData.cdn.versions.find(version => version.version !== latestVersion); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pick the actual leading non-latest CDN version.
This returns the first non-latest entry, not the highest-share one. If packageData.cdn.versions is not already sorted, the CDN adoption row will report the wrong version and request totals.
Suggested fix
function getTopNonLatestVersionData(
packageData: AdoptionPackageLike,
latestVersion: string
): AdoptionCdnVersionLike | undefined {
- return packageData.cdn.versions.find(version => version.version !== latestVersion);
+ return packageData.cdn.versions
+ .filter(version => version.version !== latestVersion)
+ .sort((a, b) => b.share - a.share || b.total - a.total)[0];
}📝 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.
Suggested change
| function getTopNonLatestVersionData( | |
| packageData: AdoptionPackageLike, | |
| latestVersion: string | |
| ): AdoptionCdnVersionLike | undefined { | |
| return packageData.cdn.versions.find(version => version.version !== latestVersion); | |
| function getTopNonLatestVersionData( | |
| packageData: AdoptionPackageLike, | |
| latestVersion: string | |
| ): AdoptionCdnVersionLike | undefined { | |
| return packageData.cdn.versions | |
| .filter(version => version.version !== latestVersion) | |
| .sort((a, b) => b.share - a.share || b.total - a.total)[0]; | |
| } |
🤖 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 `@projects/site/src/docs/metrics/adoption-data.ts` around lines 117 - 121, The
helper getTopNonLatestVersionData is selecting the first non-latest version
instead of the true leading one, so the adoption row can report the wrong CDN
version and totals. Update this function to choose the non-latest entry with the
highest share/request count from packageData.cdn.versions, rather than using
find. Keep the latestVersion exclusion, but compare the remaining versions by
the same metric used elsewhere in the adoption data model so the returned
AdoptionCdnVersionLike is the actual leader.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
projects/site/src/docs/metrics/adoption-data.test.ts (1)
4-154: 📐 Maintainability & Code Quality | 🟠 MajorAdd a case with unsorted CDN versions and release markers.
This suite still passes even with the
getTopNonLatestVersionData()bug because the fixture already puts the leading non-latest version first, andgetReleaseAdoptionTimeline()is still unexercised. Add one unsortedcdn.versionscase and one release-marker case so those paths stop regressing silently.🤖 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 `@projects/site/src/docs/metrics/adoption-data.test.ts` around lines 4 - 154, The adoption-data tests do not cover the buggy ordering and release-marker paths, so add coverage in adoption-data.test for getTopNonLatestVersionData and getReleaseAdoptionTimeline. Extend the existing adoptionFixture with one package whose cdn.versions are deliberately unsorted so the top non-latest version must be derived correctly, and add a separate case with publishDates/release markers to exercise the timeline logic. Keep the expectations aligned with getCdnVersionShareRows, getPackageDownloadTrend, and getChannelMixPoints so regressions in those helper paths are caught.projects/site/src/docs/metrics/adoption-data.ts (1)
117-121: 🎯 Functional Correctness | 🟠 MajorPick the actual leading non-latest CDN version.
This still returns the first non-latest entry, not the non-latest entry with the highest adoption.
getCdnVersionShareRow()then reports the wrong version name, share, and request totals wheneverpackageData.cdn.versionsis unsorted.Suggested fix
function getTopNonLatestVersionData( packageData: AdoptionPackageLike, latestVersion: string ): AdoptionCdnVersionLike | undefined { - return packageData.cdn.versions.find(version => version.version !== latestVersion); + return packageData.cdn.versions + .filter(version => version.version !== latestVersion) + .sort((a, b) => b.share - a.share || b.total - a.total)[0]; }🤖 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 `@projects/site/src/docs/metrics/adoption-data.ts` around lines 117 - 121, The getTopNonLatestVersionData helper currently returns the first non-latest entry from packageData.cdn.versions, which can make getCdnVersionShareRow report the wrong version when the list is unsorted. Update getTopNonLatestVersionData to select the non-latest AdoptionCdnVersionLike with the highest adoption/share instead of using find, and keep the existing latestVersion filter so the downstream version name, share, and request totals come from the true leading non-latest CDN version.projects/site/src/docs/metrics/index.11ty.js (1)
14-21: 🎯 Functional Correctness | 🟡 MinorUse
adoptionReportDatein the page or drop it.The new adoption timestamp is still only computed here. Because the template status line continues to render
releasesReportDate, the adoption charts/totals advertise the wrong freshness while this variable stays dead.🤖 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 `@projects/site/src/docs/metrics/index.11ty.js` around lines 14 - 21, The adoption freshness timestamp is computed in the metrics page but never used, while the status line still reflects the releases timestamp. Update the docs template in the metrics page so the rendered freshness/status text uses adoptionReportDate where the adoption data is being presented, or remove adoptionReportDate entirely if it is not needed. Use the metrics page’s existing data-loading and formatting logic around AdoptionService.getData and the report date variables to locate the fix.
🤖 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 `@projects/internals/metadata/src/services/releases.service.test.ts`:
- Around line 13-14: The test in releases.service.test.ts is pinned to a
specific generated release tag, which makes it brittle against normal metadata
refreshes. Update the assertion in the releases service test to validate the
deserialized release record shape and expected fields through the releases
service output, rather than checking for a hardcoded package name in
releases.data.some. Keep the length check if needed, but replace the
version-specific name assertion with generic shape-based expectations tied to
releases.data.
In `@projects/internals/metadata/src/tasks/adoption.utils.ts`:
- Around line 178-209: fetchPublicJson currently awaits fetch without any
deadline, so a stalled public API request can block adoption updates
indefinitely. Add a timeout around the network read in fetchPublicJson, using an
AbortController or equivalent, and ensure the request is aborted and reported as
a failed source with a clear timeout message. Keep the existing ok/error shape
and preserve the current handling for non-ok responses and caught network
errors.
- Around line 146-164: The catch in getPackageFile is swallowing all failures
and treating malformed or unreadable package.json files as absent packages.
Update getPackageFile so it only returns null for a missing manifest (ENOENT)
when reading the package.json URL, and rethrow any parse or other I/O errors.
Keep the existing package filtering logic in getPackageFile, but make the error
handling precise so real `@nvidia-elements/`* workspaces are not silently dropped.
---
Duplicate comments:
In `@projects/site/src/docs/metrics/adoption-data.test.ts`:
- Around line 4-154: The adoption-data tests do not cover the buggy ordering and
release-marker paths, so add coverage in adoption-data.test for
getTopNonLatestVersionData and getReleaseAdoptionTimeline. Extend the existing
adoptionFixture with one package whose cdn.versions are deliberately unsorted so
the top non-latest version must be derived correctly, and add a separate case
with publishDates/release markers to exercise the timeline logic. Keep the
expectations aligned with getCdnVersionShareRows, getPackageDownloadTrend, and
getChannelMixPoints so regressions in those helper paths are caught.
In `@projects/site/src/docs/metrics/adoption-data.ts`:
- Around line 117-121: The getTopNonLatestVersionData helper currently returns
the first non-latest entry from packageData.cdn.versions, which can make
getCdnVersionShareRow report the wrong version when the list is unsorted. Update
getTopNonLatestVersionData to select the non-latest AdoptionCdnVersionLike with
the highest adoption/share instead of using find, and keep the existing
latestVersion filter so the downstream version name, share, and request totals
come from the true leading non-latest CDN version.
In `@projects/site/src/docs/metrics/index.11ty.js`:
- Around line 14-21: The adoption freshness timestamp is computed in the metrics
page but never used, while the status line still reflects the releases
timestamp. Update the docs template in the metrics page so the rendered
freshness/status text uses adoptionReportDate where the adoption data is being
presented, or remove adoptionReportDate entirely if it is not needed. Use the
metrics page’s existing data-loading and formatting logic around
AdoptionService.getData and the report date variables to locate the fix.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: 747b1c9f-5296-4bce-b07b-affa57aedd3a
📒 Files selected for processing (23)
projects/internals/metadata/README.mdprojects/internals/metadata/package.jsonprojects/internals/metadata/src/index.tsprojects/internals/metadata/src/services/adoption.service.test.tsprojects/internals/metadata/src/services/adoption.service.tsprojects/internals/metadata/src/services/releases.service.test.tsprojects/internals/metadata/src/tasks/adoption.tsprojects/internals/metadata/src/tasks/adoption.utils.test.tsprojects/internals/metadata/src/tasks/adoption.utils.tsprojects/internals/metadata/src/types.tsprojects/internals/metadata/static/adoption.jsonprojects/internals/metadata/static/releases.jsonprojects/internals/metadata/vitest.config.tsprojects/site/package.jsonprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/metrics/adoption-data.test.tsprojects/site/src/docs/metrics/adoption-data.tsprojects/site/src/docs/metrics/adoption.tsprojects/site/src/docs/metrics/bundle-explorer.11ty.jsprojects/site/src/docs/metrics/index.11ty.jsprojects/site/src/docs/metrics/index.tsprojects/site/src/docs/metrics/release-data.test.tsprojects/site/src/docs/metrics/release-data.ts
coryrylan
force-pushed
the
topic-metrics
branch
2 times, most recently
from
ca8213c to
1017188
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@projects/internals/metadata/src/tasks/adoption.utils.ts`:
- Around line 466-472: The totals logic in adoption.utils.ts is counting partial
packages as published because the publishedPackages aggregate uses a
non-unavailable check. Update the totals calculation in the adoption summary to
count only packages whose status is exactly published, and if the broader
aggregate is still needed, add it under a separate field with a clearer name.
Use the existing packages.reduce/filter block and the packageData.status checks
as the main place to fix, and add a regression test covering a partial status so
adoption.json does not overstate published totals.
In `@projects/site/package.json`:
- Line 204: The vitest command in the package.json script includes the same test
target twice, so remove the duplicate src/examples/index.test.ts entry from the
command list. Keep the rest of the vitest run targets unchanged and make sure
the script still references the existing suite names consistently.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: de089de4-23d9-4980-bc31-95efa978a0f3
📒 Files selected for processing (23)
projects/internals/metadata/README.mdprojects/internals/metadata/package.jsonprojects/internals/metadata/src/index.tsprojects/internals/metadata/src/services/adoption.service.test.tsprojects/internals/metadata/src/services/adoption.service.tsprojects/internals/metadata/src/services/releases.service.test.tsprojects/internals/metadata/src/tasks/adoption.tsprojects/internals/metadata/src/tasks/adoption.utils.test.tsprojects/internals/metadata/src/tasks/adoption.utils.tsprojects/internals/metadata/src/types.tsprojects/internals/metadata/static/adoption.jsonprojects/internals/metadata/static/releases.jsonprojects/internals/metadata/vitest.config.tsprojects/site/package.jsonprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/metrics/adoption-data.test.tsprojects/site/src/docs/metrics/adoption-data.tsprojects/site/src/docs/metrics/adoption.tsprojects/site/src/docs/metrics/bundle-explorer.11ty.jsprojects/site/src/docs/metrics/index.11ty.jsprojects/site/src/docs/metrics/index.tsprojects/site/src/docs/metrics/release-data.test.tsprojects/site/src/docs/metrics/release-data.ts
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@projects/internals/metadata/package.json`:
- Around line 330-340: The generate:adoption task only lists local files, so
remote npm/jsDelivr/GitHub data changes won’t invalidate static/adoption.json.
Update the generate:adoption target in package.json and the adoption task flow
(node ./src/tasks/adoption.ts, adoption.utils.ts) to include a remote-aware
invalidation signal or mark it always-run so fetched data is refreshed even when
local files are unchanged.
In `@projects/internals/metadata/src/services/wireit.service.test.ts`:
- Around line 11-13: The test in WireitService is asserting reference identity
with getData(), which bakes in cache behavior that is not part of the public
contract. Update the assertion in wireit.service.test.ts to validate graph
contents instead, using toEqual(graph) or equivalent field-level checks, and
keep the existing graph.nodes and graph.links assertions aligned with the
WireitService.getData() result.
In `@projects/internals/metadata/src/tasks/adoption.utils.test.ts`:
- Around line 316-364: The adoption totals regression test only covers published
and unavailable packages, so it still allows `partial` to be counted as
published. Update the `createAdoptionSummary` test in `adoption.utils.test.ts`
to include a fixture with `status: 'partial'` alongside the existing package
cases, and assert `summary.totals.publishedPackages` excludes it while totals
still reflect the full package count. Keep the check centered on
`createAdoptionSummary` and `summary.totals` so the contract around `partial`
status is covered.
In `@projects/site/src/docs/metrics/adoption.ts`:
- Around line 226-230: ChannelMixPoint.status currently treats only unavailable
specially, so partial packages are rendered with the same color as fully
published. Update the backgroundColor mapping in adoption.ts to handle the
partial status explicitly, using a distinct token from the unavailable and fully
published cases. Keep the change localized to the channelMixPoints.map logic so
the chart visually differentiates partial packages.
In `@projects/site/src/docs/metrics/index.11ty.js`:
- Around line 241-243: The footer summary in the adoption metrics view is
missing the `partial` package count, so the totals shown by the
`getAdoptionStatus()`-driven table and the `nve-grid-footer` summary can
diverge. Update the footer rendering in `index.11ty.js` to include
`adoption.totals.partialPackages` alongside the existing published and
unavailable totals, and keep the wording consistent with the three-state status
model used by `getAdoptionStatus()`.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: 1a5f1a40-aa5f-46b3-9899-7873d01f5e07
📒 Files selected for processing (24)
projects/internals/metadata/README.mdprojects/internals/metadata/package.jsonprojects/internals/metadata/src/index.tsprojects/internals/metadata/src/services/adoption.service.test.tsprojects/internals/metadata/src/services/adoption.service.tsprojects/internals/metadata/src/services/releases.service.test.tsprojects/internals/metadata/src/services/wireit.service.test.tsprojects/internals/metadata/src/tasks/adoption.tsprojects/internals/metadata/src/tasks/adoption.utils.test.tsprojects/internals/metadata/src/tasks/adoption.utils.tsprojects/internals/metadata/src/types.tsprojects/internals/metadata/static/adoption.jsonprojects/internals/metadata/static/releases.jsonprojects/internals/metadata/vitest.config.tsprojects/site/package.jsonprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/metrics/adoption-data.test.tsprojects/site/src/docs/metrics/adoption-data.tsprojects/site/src/docs/metrics/adoption.tsprojects/site/src/docs/metrics/bundle-explorer.11ty.jsprojects/site/src/docs/metrics/index.11ty.jsprojects/site/src/docs/metrics/index.tsprojects/site/src/docs/metrics/release-data.test.tsprojects/site/src/docs/metrics/release-data.ts
- Introduced `AdoptionService` to fetch and cache public adoption metrics from npm, jsDelivr, and GitHub. - Added `adoption.ts` task to generate adoption metrics and save them to `static/adoption.json`. - Implemented utility functions for parsing adoption data from various sources. - Updated `package.json` scripts to include a new `generate:adoption` command. - Enhanced documentation to reflect the new adoption metrics functionality. Signed-off-by: Cory Rylan <crylan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
projects/internals/metadata/README.md (1)
41-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
AdoptionServiceas local-only.
projects/internals/metadata/src/services/adoption.service.ts:37-44only lazy-loads../../static/adoption.json, so listing it in this fallback-oriented services section still implies a CDN recovery path it doesn’t implement. Either qualify this bullet as a cached local snapshot or narrow the surrounding fallback claim.🤖 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 `@projects/internals/metadata/README.md` at line 41, The README entry for AdoptionService is too broad and implies a CDN fallback path that the service does not provide. Update the bullet for AdoptionService in the metadata services section, or adjust the surrounding fallback wording, to clearly state that adoption.service.ts only lazy-loads the local static adoption.json snapshot and is local-only/cached rather than a recovery fallback.projects/internals/metadata/src/tasks/adoption.utils.ts (4)
390-395: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t treat registry outages as package absence.
Line 394 maps every failed npm registry lookup to
unavailable. A 5xx/timeout will therefore move a published package into the unavailable bucket even when downloads/CDN data succeeded, skewingpartialPackagesandunavailablePackages. Reserveunavailablefor a confirmed missing package, and classify transient registry failures aspartial.Proposed fix
const errors = [registryResult, downloadsResult, cdnResult].flatMap(result => (result.ok ? [] : [result.error])); const registry = registryResult.ok ? parseNpmRegistry(registryResult.data) : null; + const registryMissing = !registryResult.ok && registryResult.error.status === 404; if (!registry) { return { name: workspacePackage.name, workspaceVersion: workspacePackage.workspaceVersion, - status: registryResult.ok ? 'partial' : 'unavailable', + status: registryMissing ? 'unavailable' : 'partial', latestVersion: null, publishedAt: null, versionCount: 0,🤖 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 `@projects/internals/metadata/src/tasks/adoption.utils.ts` around lines 390 - 395, In adoption.utils.ts, the status mapping in the registry-miss branch of the package result logic is treating any failed registry lookup as unavailable, which conflates transient registry outages with a truly missing package. Update the logic around the registryResult check in this branch so that only confirmed “package not found” cases map to unavailable, and any other registry failure (for example 5xx/timeout) maps to partial; keep using the existing workspacePackage and registryResult symbols to locate the fix.
358-370: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap GitHub stargazer pagination concurrency.
Promise.allfires every remaining page immediately. As the repo grows, that burst can hit GitHub secondary rate limits and leavestargazerspartial.Proposed fix
- const remainingPages = await Promise.all( - Array.from({ length: Math.max(totalPages - 1, 0) }, (_, index) => - fetchPublicJson( - `https://api.github.com/repos/${repository}/stargazers?per_page=100&page=${index + 2}`, - 'github', - { - ...headers, - Accept: 'application/vnd.github.star+json' - } - ) - ) - ); + const remainingPages: PublicJsonResult[] = []; + for (let page = 2; page <= totalPages; page += 1) { + remainingPages.push( + await fetchPublicJson( + `https://api.github.com/repos/${repository}/stargazers?per_page=100&page=${page}`, + 'github', + { + ...headers, + Accept: 'application/vnd.github.star+json' + } + ) + ); + }🤖 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 `@projects/internals/metadata/src/tasks/adoption.utils.ts` around lines 358 - 370, The stargazer fetch logic in adoption.utils.ts is launching all remaining GitHub pages at once via Promise.all, which can trigger rate limits for large repositories. Update the pagination loop around parseGitHubPaginationTotal/fetchPublicJson to fetch pages with a bounded concurrency limit or sequential batching instead of firing every request immediately. Keep the existing totalPages calculation, but replace the unbounded Promise.all over remainingPages with a throttled approach so stargazers are fully loaded without bursting GitHub.
178-209: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a deadline to public API reads.
A stalled npm/jsDelivr/GitHub socket can hang the whole adoption generator indefinitely because
fetch()has no timeout here.Proposed fix
export async function fetchPublicJson( url: string, source: AdoptionSource, headers: Record<string, string> = {} ): Promise<PublicJsonResult> { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 15_000); + try { - const response = await fetch(url, { headers }); + const response = await fetch(url, { + headers, + signal: controller.signal + }); @@ } catch (error) { // Network endpoints can fail independently of the generator process. return { ok: false, - error: getSourceError(source, null, error instanceof Error ? error.message : 'unknown network error') + error: getSourceError( + source, + null, + error instanceof Error && error.name === 'AbortError' + ? 'request timed out' + : error instanceof Error + ? error.message + : 'unknown network error' + ) }; + } finally { + clearTimeout(timeoutId); } }🤖 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 `@projects/internals/metadata/src/tasks/adoption.utils.ts` around lines 178 - 209, fetchPublicJson currently calls fetch() without any deadline, so a stalled network read can block the adoption generator indefinitely. Update fetchPublicJson to enforce a timeout/deadline for the request, ideally by using an AbortController (or equivalent) around the existing fetch(url, { headers }) call, and make sure timeout-triggered aborts are returned through the existing getSourceError path with a clear timeout-specific message.
146-164: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOnly swallow missing manifests here.
This
catchturns malformed or unreadablepackage.jsonfiles into “not a package”, so a real@nvidia-elements/*workspace can silently disappear from the snapshot and undercounttotals.packages.Proposed fix
- } catch { - // Not every projects directory entry is an npm package. - return null; + } catch (error) { + if ( + typeof error === 'object' && + error !== null && + 'code' in error && + error.code === 'ENOENT' + ) { + // Not every projects directory entry is an npm package. + return null; + } + + throw error; } }🤖 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 `@projects/internals/metadata/src/tasks/adoption.utils.ts` around lines 146 - 164, The getPackageFile helper is swallowing all errors and treating malformed or unreadable package.json files as missing packages. Update the catch in getPackageFile to only ignore the “file not found” case for package.json, and rethrow or surface any other read/parse errors so real `@nvidia-elements/`* workspaces are not silently excluded from the snapshot and totals.packages count.
🤖 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 `@projects/internals/metadata/src/services/adoption.service.ts`:
- Around line 37-40: The dynamic JSON import in AdoptionService.getData() uses a
source-relative path that breaks once the package is published from the dist
layout. Update the import in getData() so it resolves the built runtime artifact
correctly, or ensure the build copies adoption.json into the emitted dist/static
location that AdoptionService expects. Use the existing AdoptionService and
getData() symbols to locate the runtime import and make the published export
load the JSON successfully.
In `@projects/site/src/docs/metrics/index.ts`:
- Line 85: The weekly “Total Releases” series in the metrics docs is omitting
`chore` items, so the plotted total is lower than the tooltip. Update the
`weeklyTypeData.map(...)` expression in the metrics dataset to include
`week.chore` alongside `feat`, `fix`, and `breaking`, keeping the total series
aligned with the tooltip and the rest of `weeklyTypeData`.
---
Duplicate comments:
In `@projects/internals/metadata/README.md`:
- Line 41: The README entry for AdoptionService is too broad and implies a CDN
fallback path that the service does not provide. Update the bullet for
AdoptionService in the metadata services section, or adjust the surrounding
fallback wording, to clearly state that adoption.service.ts only lazy-loads the
local static adoption.json snapshot and is local-only/cached rather than a
recovery fallback.
In `@projects/internals/metadata/src/tasks/adoption.utils.ts`:
- Around line 390-395: In adoption.utils.ts, the status mapping in the
registry-miss branch of the package result logic is treating any failed registry
lookup as unavailable, which conflates transient registry outages with a truly
missing package. Update the logic around the registryResult check in this branch
so that only confirmed “package not found” cases map to unavailable, and any
other registry failure (for example 5xx/timeout) maps to partial; keep using the
existing workspacePackage and registryResult symbols to locate the fix.
- Around line 358-370: The stargazer fetch logic in adoption.utils.ts is
launching all remaining GitHub pages at once via Promise.all, which can trigger
rate limits for large repositories. Update the pagination loop around
parseGitHubPaginationTotal/fetchPublicJson to fetch pages with a bounded
concurrency limit or sequential batching instead of firing every request
immediately. Keep the existing totalPages calculation, but replace the unbounded
Promise.all over remainingPages with a throttled approach so stargazers are
fully loaded without bursting GitHub.
- Around line 178-209: fetchPublicJson currently calls fetch() without any
deadline, so a stalled network read can block the adoption generator
indefinitely. Update fetchPublicJson to enforce a timeout/deadline for the
request, ideally by using an AbortController (or equivalent) around the existing
fetch(url, { headers }) call, and make sure timeout-triggered aborts are
returned through the existing getSourceError path with a clear timeout-specific
message.
- Around line 146-164: The getPackageFile helper is swallowing all errors and
treating malformed or unreadable package.json files as missing packages. Update
the catch in getPackageFile to only ignore the “file not found” case for
package.json, and rethrow or surface any other read/parse errors so real
`@nvidia-elements/`* workspaces are not silently excluded from the snapshot and
totals.packages count.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: 695dca46-c269-481f-b392-f81f428ee080
📒 Files selected for processing (24)
projects/internals/metadata/README.mdprojects/internals/metadata/package.jsonprojects/internals/metadata/src/index.tsprojects/internals/metadata/src/services/adoption.service.test.tsprojects/internals/metadata/src/services/adoption.service.tsprojects/internals/metadata/src/services/releases.service.test.tsprojects/internals/metadata/src/services/wireit.service.test.tsprojects/internals/metadata/src/tasks/adoption.tsprojects/internals/metadata/src/tasks/adoption.utils.test.tsprojects/internals/metadata/src/tasks/adoption.utils.tsprojects/internals/metadata/src/types.tsprojects/internals/metadata/static/adoption.jsonprojects/internals/metadata/static/releases.jsonprojects/internals/metadata/vitest.config.tsprojects/site/package.jsonprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/metrics/adoption-data.test.tsprojects/site/src/docs/metrics/adoption-data.tsprojects/site/src/docs/metrics/adoption.tsprojects/site/src/docs/metrics/bundle-explorer.11ty.jsprojects/site/src/docs/metrics/index.11ty.jsprojects/site/src/docs/metrics/index.tsprojects/site/src/docs/metrics/release-data.test.tsprojects/site/src/docs/metrics/release-data.ts
Comment on lines
+37
to
+40
| static async getData(): Promise<AdoptionSummary> { | ||
| if (AdoptionService.#adoption.created === '') { | ||
| AdoptionService.#adoption = (await import('../../static/adoption.json', { with: { type: 'json' } })) | ||
| .default as AdoptionSummary; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The JSON import path does not survive the published dist layout.
getData() loads ../../static/adoption.json, which is correct from src/services, but this package exports the built runtime from ./dist/src/services/adoption.service.js. From that emitted location the same relative path points at dist/static/adoption.json, while this PR generates the artifact at static/adoption.json. Unless the build also copies the file into dist/static, the public AdoptionService export will throw at runtime.
🤖 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 `@projects/internals/metadata/src/services/adoption.service.ts` around lines 37
- 40, The dynamic JSON import in AdoptionService.getData() uses a
source-relative path that breaks once the package is published from the dist
layout. Update the import in getData() so it resolves the built runtime artifact
correctly, or ensure the build copies adoption.json into the emitted dist/static
location that AdoptionService expects. Use the existing AdoptionService and
getData() symbols to locate the runtime import and make the published export
load the JSON successfully.
| { | ||
| label: 'Total', | ||
| data: monthlyTypeData.map(m => m.feat + m.fix + m.breaking), | ||
| data: weeklyTypeData.map(week => week.feat + week.fix + week.breaking), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include chore releases in the weekly total series.
weeklyTypeData includes chore, and the tooltip total already counts it, but this dataset drops it. Weeks with chores will plot a lower "Total Releases" value than the tooltip reports.
Suggested fix
- data: weeklyTypeData.map(week => week.feat + week.fix + week.breaking),
+ data: weeklyTypeData.map(week => week.feat + week.fix + week.breaking + week.chore),📝 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.
Suggested change
| data: weeklyTypeData.map(week => week.feat + week.fix + week.breaking), | |
| data: weeklyTypeData.map(week => week.feat + week.fix + week.breaking + week.chore), |
🤖 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 `@projects/site/src/docs/metrics/index.ts` at line 85, The weekly “Total
Releases” series in the metrics docs is omitting `chore` items, so the plotted
total is lower than the tooltip. Update the `weeklyTypeData.map(...)` expression
in the metrics dataset to include `week.chore` alongside `feat`, `fix`, and
`breaking`, keeping the total series aligned with the tooltip and the rest of
`weeklyTypeData`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AdoptionServiceto fetch and cache public adoption metrics from npm, jsDelivr, and GitHub.adoption.tstask to generate adoption metrics and save them tostatic/adoption.json.package.jsonscripts to include a newgenerate:adoptioncommand.Summary by CodeRabbit