Speed up and distribute CI with Nx Cloud - #62
Conversation
This commit sets up Nx Cloud for your Nx workspace, enabling distributed caching and the Nx Cloud GitHub integration for fast CI and improved developer experience. You can access your Nx Cloud workspace by going to https://cloud.nx.app/orgs/6412ca9d1c251d000efa21ba/workspaces/6a728964bb91bb09533eb006 > [!TIP] > Run `npx nx generate ci-workflow` if you don't have a CI script configured yet. **Note:** This commit attempts to maintain formatting of the nx.json file, however you may need to correct formatting by running an nx format command and committing the changes.
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR restructures CI around path classification, Nx caching, distributed execution, weighted benchmark sharding, baseline validation, catalog revision handling, and bounded concurrency for package and benchmark verification. ChangesCI execution and validation
Nx and catalog build graph
Benchmark validation and sharding
Parallel package and catalog verification
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant classifyCiChanges
participant Nx
participant BenchmarkJobs
GitHubActions->>classifyCiChanges: changed paths
classifyCiChanges->>GitHubActions: partition outputs
GitHubActions->>Nx: distributed static validation
GitHubActions->>BenchmarkJobs: enabled comparison and stress partitions
BenchmarkJobs->>GitHubActions: artifacts and results
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
View your CI Pipeline Execution ↗ for commit 7b1c67d
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/compare-plot-catalog.mjs (1)
1164-1178: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDeduplicate renderer pairs before concurrent builds.
pairedRenderers(entry)can generate the same[referenceRenderer, targetRenderer]pair whenreferenceRendererForCase(entry)equalstargetRenderer. For the target renderer,pairedRenderers(entry).map(...)creates two candidates with the sameid, whilerunWithConcurrencybuilds them with the sameoutputPath. Filter to a unique(case, renderer)set before building.🤖 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 `@scripts/compare-plot-catalog.mjs` around lines 1164 - 1178, The build flow around pairedRenderers(entry) must deduplicate renderer candidates before passing them to runWithConcurrency, especially when referenceRendererForCase(entry) equals targetRenderer. Filter by the unique case-and-renderer identity so each id and outputPath is built only once, while preserving distinct renderer pairs.
🧹 Nitpick comments (5)
scripts/check-packed-consumers.mjs (3)
148-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass an explicit encoding to
readFile.Line 150 calls
readFilewithout an encoding, soJSON.parsereceives aBuffer. This works, but the rest of the codebase passes'utf8'(for examplescripts/packed-react-native-consumers.mjsline 49). Use the same form here.♻️ Optional: align the read with the rest of the scripts
const manifest = JSON.parse( - await readFile(resolve(packageInfo.sourceDirectory, 'package.json')), + await readFile(resolve(packageInfo.sourceDirectory, 'package.json'), 'utf8'), )🤖 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 `@scripts/check-packed-consumers.mjs` around lines 148 - 158, Update readFile in loadPackageManifest to pass the explicit 'utf8' encoding before JSON.parse, matching the established pattern used by the other scripts.
1870-1942: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the bundle filename from
entry.filename, notentry.label.Line 1879 builds the output path from
entry.label.toLowerCase(). Labels contain spaces, so the fixture now holds files such ascore renderer.jsandcompact linear scale.js. Two entries with labels that differ only in case would also collide on oneoutfile, and concurrent workers would then bundle into the same path.entry.filenameis already unique and filesystem-safe.The indexed
results[index]assignment preserves report order correctly.♻️ Proposed change
const entryPath = resolve(fixtureDirectory, entry.filename) - const outfile = resolve( - bundleDirectory, - `${entry.label.toLowerCase()}.js`, - ) + const outfile = resolve( + bundleDirectory, + `${entry.filename.replace(/\.ts$/, '')}.js`, + )🤖 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 `@scripts/check-packed-consumers.mjs` around lines 1870 - 1942, Update the outfile construction in the runWithConcurrency callback to derive the bundle filename from entry.filename rather than entry.label.toLowerCase(). Keep the existing bundle directory and report-order results[index] assignment unchanged, relying on entry.filename as the unique filesystem-safe name.
89-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueManifest loading stays unbounded while builds are bounded.
Line 90 limits builds to
packageBuildConcurrency, but line 89 fans out every manifest read at once. The reads are small, so this is acceptable today. If you want one policy for all filesystem fan-out in this script, route line 89 throughrunWithConcurrencyas well.♻️ Optional: use one concurrency policy
- await Promise.all(packages.map(loadPackageManifest)) + await runWithConcurrency(packages, packageBuildConcurrency, loadPackageManifest) await runWithConcurrency(packages, packageBuildConcurrency, buildPackage)🤖 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 `@scripts/check-packed-consumers.mjs` around lines 89 - 90, Optionally apply the shared concurrency policy to manifest loading by replacing the unbounded Promise.all call around loadPackageManifest with runWithConcurrency using packages, packageBuildConcurrency, and loadPackageManifest, while preserving the existing bounded build flow.scripts/packed-react-native-consumers.mjs (1)
79-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe isolation gate reports ENOENT instead of the intended message.
If Metro does not create
.metro-cache,statrejects withENOENTat line 79. The assertion at lines 80-83 then never runs, and the failure reaches the caller as a raw filesystem error inside anAggregateError. Catch the missing path so the isolation failure names the consumer.♻️ Optional: report a missing cache directory clearly
- const cache = await stat(resolve(consumerRoot, '.metro-cache')) + const cache = await stat(resolve(consumerRoot, '.metro-cache')).catch( + () => null, + ) assert.ok( - cache.isDirectory(), + cache?.isDirectory(), `${config.name} Metro cache was not isolated`, )🤖 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 `@scripts/packed-react-native-consumers.mjs` around lines 79 - 83, Update the Metro cache validation around the cache lookup in the consumer isolation flow to handle a missing .metro-cache path without allowing stat’s ENOENT rejection to escape. Convert that case into the existing assertion failure using config.name so the reported message identifies the affected consumer, while preserving the directory check when the path exists.scripts/compare-plot-catalog.mjs (1)
1150-1153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the bundle concurrency configurable.
The concurrency limit is the literal
4.scripts/measure-bundles.mjsreads its limit fromBUNDLE_BUILD_CONCURRENCYthroughreadConcurrency. Both scripts run esbuild builds in CI. A shared, configurable limit lets operators tune both scripts for the runner size.🤖 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 `@scripts/compare-plot-catalog.mjs` around lines 1150 - 1153, Update the runWithConcurrency call in the candidate comparison flow to use the shared BUNDLE_BUILD_CONCURRENCY configuration via the existing readConcurrency utility, replacing the hardcoded limit of 4; preserve the current default behavior when the environment variable is unset.
🤖 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 `@API-FRICTION.md`:
- Around line 5094-5097: Add a permitted “Classification: tooling” field to each
of the F-206, F-207, and F-208 entries in API-FRICTION.md, placing it alongside
the existing Owner field while preserving Owner for responsibility.
In `@nx.json`:
- Around line 263-265: Update the catalog-loading-check target configuration in
nx.json to include catalogRevision alongside catalogSource in its inputs,
ensuring changes to CATALOG_SOURCE_REVISION or GITHUB_SHA invalidate the cached
check result.
In `@scripts/benchmark/filters.mjs`:
- Line 41: Update selectWeightedShard’s shardWeights allocation to cap its
length at values.length instead of the unbounded CLI-parsed shard.total, while
preserving valid weighting behavior. Add a regression test using a shard total
far larger than the input values to verify allocation remains bounded, and
update the public type if it exposes the affected size or configuration.
In `@scripts/compare-chart-libraries.mjs`:
- Around line 1224-1232: Normalize baseline.bundles once to an empty record when
it is missing or not a record, then use that normalized value in the expectedIds
construction and the measured-bundles access around line 1242. Preserve
bundleBaselineShapeFailures so malformed input remains reported as a validation
failure rather than causing Object.keys or property-access exceptions.
In `@scripts/compare-plot-catalog.mjs`:
- Around line 136-143: Update the empty-selection handling after
selectWeightedShard to distinguish whether caseFilter matched any entries before
sharding: report a filter-miss error only when the filtered input is empty, and
report a shard-miss error when matching cases exist but the selected shard is
empty. Preserve the existing successful selection flow.
- Around line 657-675: Normalize TypeScript diagnostic file names to
forward-slash paths when populating the byFile map in the diagnostic collection
flow. Update the corresponding auditTypes and auditTypeProtection lookups to
apply the same normalization before querying byFile, ensuring resolved paths
match diagnostic keys on all platforms.
In `@scripts/measure-bundles.mjs`:
- Around line 968-975: Update readConcurrency to trim the provided value before
numeric parsing, and return fallback when the trimmed value is empty, treating
an empty BUNDLE_BUILD_CONCURRENCY as unset. Preserve the existing
positive-safe-integer validation for non-empty values.
---
Outside diff comments:
In `@scripts/compare-plot-catalog.mjs`:
- Around line 1164-1178: The build flow around pairedRenderers(entry) must
deduplicate renderer candidates before passing them to runWithConcurrency,
especially when referenceRendererForCase(entry) equals targetRenderer. Filter by
the unique case-and-renderer identity so each id and outputPath is built only
once, while preserving distinct renderer pairs.
---
Nitpick comments:
In `@scripts/check-packed-consumers.mjs`:
- Around line 148-158: Update readFile in loadPackageManifest to pass the
explicit 'utf8' encoding before JSON.parse, matching the established pattern
used by the other scripts.
- Around line 1870-1942: Update the outfile construction in the
runWithConcurrency callback to derive the bundle filename from entry.filename
rather than entry.label.toLowerCase(). Keep the existing bundle directory and
report-order results[index] assignment unchanged, relying on entry.filename as
the unique filesystem-safe name.
- Around line 89-90: Optionally apply the shared concurrency policy to manifest
loading by replacing the unbounded Promise.all call around loadPackageManifest
with runWithConcurrency using packages, packageBuildConcurrency, and
loadPackageManifest, while preserving the existing bounded build flow.
In `@scripts/compare-plot-catalog.mjs`:
- Around line 1150-1153: Update the runWithConcurrency call in the candidate
comparison flow to use the shared BUNDLE_BUILD_CONCURRENCY configuration via the
existing readConcurrency utility, replacing the hardcoded limit of 4; preserve
the current default behavior when the environment variable is unset.
In `@scripts/packed-react-native-consumers.mjs`:
- Around line 79-83: Update the Metro cache validation around the cache lookup
in the consumer isolation flow to handle a missing .metro-cache path without
allowing stat’s ENOENT rejection to escape. Convert that case into the existing
assertion failure using config.name so the reported message identifies the
affected consumer, while preserving the directory check when the path exists.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be4e5510-cd6d-485d-8af6-5ba530a9fc9d
📒 Files selected for processing (29)
.github/actions/setup/action.yml.github/workflows/chart-library-benchmarks.yml.gitignore.nx/workflows/distribution.yamlAPI-FRICTION.mdbenchmarks/comparison/stress/workloads.jsonbenchmarks/comparison/stress/workloads.test.tsexamples/conformance/package.jsonnx.jsonpackage.jsonproject.jsonscripts/benchmark/bundle-baseline.mjsscripts/benchmark/bundle-baseline.test.mjsscripts/benchmark/conformance-sharding.mjsscripts/benchmark/conformance-sharding.test.mjsscripts/benchmark/filters.d.mtsscripts/benchmark/filters.mjsscripts/benchmark/filters.test.mjsscripts/build-conformance-artifact.mjsscripts/catalog-source-revision.mjsscripts/check-packed-consumers.mjsscripts/ci-workflow.test.mjsscripts/classify-ci-changes.mjsscripts/compare-chart-libraries.mjsscripts/compare-plot-catalog.mjsscripts/measure-bundles.mjsscripts/packed-react-native-consumers.mjsscripts/stress-chart-libraries.mjstsconfig.json
tannerlinsley
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/benchmark/bundle-baseline.mjs (1)
7-10: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNormalize the top-level baseline before validation.
If
JSON.parsereturnsnull,baseline.matrix?.chartTypesstill throws because optional chaining starts afterbaseline.matrix. The downstreamcheckBundleBaselineinscripts/compare-chart-libraries.mjs(Lines 1156-1271) also readsbaseline.schemaVersionbefore calling this helper. CI then crashes instead of returning a structured baseline-shape failure.Normalize the parsed baseline at the caller boundary and use the normalized record here. Add a regression test for a
nullroot baseline.Suggested normalization
) { const failures = [] + const baselineRecord = recordOrEmpty(baseline) if ( - JSON.stringify(baseline.matrix?.chartTypes) !== + JSON.stringify(baselineRecord.matrix?.chartTypes) !== JSON.stringify(chartTypes) || - JSON.stringify(baseline.matrix?.tiers) !== JSON.stringify(tiers) + JSON.stringify(baselineRecord.matrix?.tiers) !== JSON.stringify(tiers) ... - recordKeys(baseline.bundles), + recordKeys(baselineRecord.bundles),🤖 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 `@scripts/benchmark/bundle-baseline.mjs` around lines 7 - 10, Normalize the JSON-parsed baseline at the caller boundary before invoking checkBundleBaseline, ensuring a null root becomes an empty record so schemaVersion access and validation cannot throw. Update the chartTypes and tiers comparison in the baseline validation helper to use this normalized record, and add a regression test covering a null root baseline that verifies a structured baseline-shape failure.
🤖 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 `@scripts/benchmark/filters.test.mjs`:
- Line 77: Update the assertion in the weighted-shard coverage test to compare
the flattened shard IDs against the sorted input values, rather than using
expect.arrayContaining. Ensure the comparison verifies exact equality, rejecting
missing, extra, or duplicated values while preserving order normalization
through sorting.
---
Outside diff comments:
In `@scripts/benchmark/bundle-baseline.mjs`:
- Around line 7-10: Normalize the JSON-parsed baseline at the caller boundary
before invoking checkBundleBaseline, ensuring a null root becomes an empty
record so schemaVersion access and validation cannot throw. Update the
chartTypes and tiers comparison in the baseline validation helper to use this
normalized record, and add a regression test covering a null root baseline that
verifies a structured baseline-shape failure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01c36e1e-4ec0-4449-b431-8387505ca331
📒 Files selected for processing (12)
nx.jsonscripts/benchmark/bundle-baseline.mjsscripts/benchmark/bundle-baseline.test.mjsscripts/benchmark/filters.mjsscripts/benchmark/filters.test.mjsscripts/compare-chart-libraries.mjsscripts/compare-plot-catalog-helpers.mjsscripts/compare-plot-catalog-helpers.test.mjsscripts/compare-plot-catalog.mjsscripts/measure-bundles-options.mjsscripts/measure-bundles-options.test.mjsscripts/measure-bundles.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/compare-plot-catalog.mjs
- scripts/benchmark/filters.mjs
- scripts/compare-chart-libraries.mjs
- nx.json
1 participant
Summary
Validation
pnpm run validategit diff --checkNx Cloud workspace: https://cloud.nx.app/orgs/6412ca9d1c251d000efa21ba/workspaces/6a728964bb91bb09533eb006
Summary by CodeRabbit
New Features
Bug Fixes
Chores