Publish the chart catalog as generated content - #2
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe catalog pipeline now generates a schema-v2 artifact from the Vite manifest, validates its module closure and hashes, uploads it from CI, and publishes it to ChangesCatalog artifact publication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConformanceCases
participant BuildArtifact
participant ArtifactValidator
participant ValidateWorkflow
participant CatalogDist
ConformanceCases->>BuildArtifact: provide case metadata and Vite manifest
BuildArtifact->>ArtifactValidator: construct catalog.json and asset closure
ArtifactValidator-->>ValidateWorkflow: return validated artifact
ValidateWorkflow->>CatalogDist: publish catalog.json and hashed assets
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
scripts/build-conformance-artifact.mjs (1)
115-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
validateCaseIdentitiesduplicatesvalidateCaseEntriesinscripts/catalog-artifact.mjs.Same id/order/regex rules, re-implemented with
throwinstead ofassert. The--checkpath and the build path can silently drift. ExportvalidateCaseEntriesfromcatalog-artifact.mjsand reuse it here.♻️ Proposed reuse
import { attachEmbedContract, createCatalogArtifact, serializeCatalogManifest, + validateCaseEntries, validateCatalogArtifactManifest, } from './catalog-artifact.mjs'if (checkOnly) { - validateCaseIdentities(cases) + validateCaseEntries(cases) console.log(`Validated ${cases.length} publishable catalog cases.`) process.exit(0) }Then delete
validateCaseIdentitiesand addexporttovalidateCaseEntriesinscripts/catalog-artifact.mjs.🤖 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/build-conformance-artifact.mjs` around lines 115 - 139, Export validateCaseEntries from scripts/catalog-artifact.mjs and import and reuse it in the build script’s validation flow. Remove validateCaseIdentities and its duplicated id, order, and regex checks so both --check and build paths share the same validation implementation and assertion behavior..github/workflows/chart-library-benchmarks.yml (1)
156-164: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a
concurrencygroup so overlapping pushes can't race oncatalog-dist.Two pushes to
mainin quick succession run this job twice; the second worktree is created from a staleorigin/catalog-dist, and the non-fast-forward push simply fails (or, worse, an older revision wins if the ordering flips). A serialized concurrency group makes publication deterministic.🛠️ Proposed change
publish-catalog: if: github.event_name == 'push' && github.ref == 'refs/heads/main' needs: - validate - conformance runs-on: ubuntu-latest timeout-minutes: 10 + concurrency: + group: publish-catalog + cancel-in-progress: false permissions: contents: write🤖 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 @.github/workflows/chart-library-benchmarks.yml around lines 156 - 164, Add a workflow-level concurrency group to the publish-catalog job, using a stable key shared by all runs that publish catalog-dist and preventing overlapping executions. Keep the existing publish-catalog conditions, dependencies, permissions, and timeout unchanged.scripts/check-conformance-artifact.mjs (1)
24-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpected catalog counts are duplicated across the two check scripts.
scripts/check-conformance-artifact.mjsasserts 100 cases and 68/21/11 renderer counts, andscripts/check-catalog-loading.mjsasserts the same numbers against the Vite graph. Adding or reclassifying a case requires editing both, and a partial update yields a confusing single-script failure.
scripts/check-conformance-artifact.mjs#L24-L33: import the expected counts from a shared module (e.g. exported constants inscripts/catalog-artifact.mjs) instead of inlining them.scripts/check-catalog-loading.mjs#L90-L96: consume the same shared constants for the tanstack/plot/recharts/echarts expectations.🤖 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-conformance-artifact.mjs` around lines 24 - 33, Centralize the expected catalog counts in shared exports from scripts/catalog-artifact.mjs, then update the assertions in check-conformance-artifact.mjs and check-catalog-loading.mjs to consume those constants instead of duplicating numeric literals. Preserve the existing case-count and renderer-specific expectations, including the tanstack, plot, recharts, and echarts checks.scripts/catalog-artifact.test.mjs (1)
171-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe unreferenced-asset case is never actually exercised.
validateCatalogArtifactManifestassertscomparison.visibility === 'debug'well before the closure reachability check, so the injectedassets/extra-GGGG7777.jsorphan is irrelevant here — the test would pass identically without it. Split into two tests so the "assets outside the implementation closure" assertion is covered.💚 Proposed split
- it('rejects unreferenced files and non-debug comparisons', async () => { + it('rejects assets outside the implementation closure', async () => { const artifact = await createArtifact() const catalog = attachEmbedContract(artifact.catalog, { protocol: { version: 1 }, }) catalog.assets['assets/extra-GGGG7777.js'] = { bytes: 1, sha256: 'b'.repeat(64), imports: [], dynamicImports: [], } - catalog.cases[0].modules.comparison.visibility = 'public' + + expect(() => validateCatalogArtifactManifest(catalog)).toThrow( + 'assets outside the implementation closure', + ) + }) + + it('rejects non-debug comparisons', async () => { + const artifact = await createArtifact() + const catalog = attachEmbedContract(artifact.catalog, { + protocol: { version: 1 }, + }) + catalog.cases[0].modules.comparison.visibility = 'public' expect(() => validateCatalogArtifactManifest(catalog)).toThrow( 'comparison must be debug-only', ) })🤖 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/catalog-artifact.test.mjs` around lines 171 - 187, Split the combined test around validateCatalogArtifactManifest into separate cases: keep the non-debug comparison test focused only on visibility, and add a distinct test that leaves comparison.visibility as debug while injecting assets/extra-GGGG7777.js, then asserts the unreferenced-asset validation error.scripts/check-catalog-loading.mjs (1)
16-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCross-check depends on two independent Vite builds producing identical content hashes.
inspectCatalogGraph()runs a fresh in-memory build and thenverifyPublishedGraphmatches artifact asset paths againstchunksByFileby hashed filename. Any environment difference between this build and the one behind.catalog-artifact(base path, mode, env vars, plugin ordering) surfaces as a confusingpublished artifact contains unknown chunk …. Also, a missing artifact fails with a bareENOENTrather than a hint to runpnpm catalog:build.♻️ Clearer failure for a missing artifact
-const artifact = JSON.parse( - await fs.readFile(path.join(artifactDirectory, 'catalog.json'), 'utf8'), -) +const catalogPath = path.join(artifactDirectory, 'catalog.json') +let artifactSource +try { + artifactSource = await fs.readFile(catalogPath, 'utf8') +} catch (error) { + throw new Error( + `Missing ${catalogPath}; run \`pnpm catalog:build\` first.`, + { cause: error }, + ) +} +const artifact = JSON.parse(artifactSource)🤖 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-catalog-loading.mjs` around lines 16 - 21, Update the check flow around inspectCatalogGraph and verifyPublishedGraph so graph inspection uses the same build inputs or metadata as the catalog artifact instead of performing an independent build with potentially different configuration. Also handle missing catalog.json before fs.readFile and report an actionable message directing the user to run pnpm catalog:build, while preserving validateCatalogArtifactManifest and verification for existing artifacts.
🤖 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.
Nitpick comments:
In @.github/workflows/chart-library-benchmarks.yml:
- Around line 156-164: Add a workflow-level concurrency group to the
publish-catalog job, using a stable key shared by all runs that publish
catalog-dist and preventing overlapping executions. Keep the existing
publish-catalog conditions, dependencies, permissions, and timeout unchanged.
In `@scripts/build-conformance-artifact.mjs`:
- Around line 115-139: Export validateCaseEntries from
scripts/catalog-artifact.mjs and import and reuse it in the build script’s
validation flow. Remove validateCaseIdentities and its duplicated id, order, and
regex checks so both --check and build paths share the same validation
implementation and assertion behavior.
In `@scripts/catalog-artifact.test.mjs`:
- Around line 171-187: Split the combined test around
validateCatalogArtifactManifest into separate cases: keep the non-debug
comparison test focused only on visibility, and add a distinct test that leaves
comparison.visibility as debug while injecting assets/extra-GGGG7777.js, then
asserts the unreferenced-asset validation error.
In `@scripts/check-catalog-loading.mjs`:
- Around line 16-21: Update the check flow around inspectCatalogGraph and
verifyPublishedGraph so graph inspection uses the same build inputs or metadata
as the catalog artifact instead of performing an independent build with
potentially different configuration. Also handle missing catalog.json before
fs.readFile and report an actionable message directing the user to run pnpm
catalog:build, while preserving validateCatalogArtifactManifest and verification
for existing artifacts.
In `@scripts/check-conformance-artifact.mjs`:
- Around line 24-33: Centralize the expected catalog counts in shared exports
from scripts/catalog-artifact.mjs, then update the assertions in
check-conformance-artifact.mjs and check-catalog-loading.mjs to consume those
constants instead of duplicating numeric literals. Preserve the existing
case-count and renderer-specific expectations, including the tanstack, plot,
recharts, and echarts checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7deeb63a-34a0-4b9d-aa83-dcca8935adb7
📒 Files selected for processing (20)
.github/workflows/chart-library-benchmarks.yml.gitignoreAPI-FRICTION.mdbenchmarks/conformance/README.mddeploy/catalog/_headersexamples/conformance/package.jsonexamples/conformance/vite.config.tspackage.jsonscripts/build-conformance-artifact.mjsscripts/build-conformance-site.mjsscripts/catalog-artifact.mjsscripts/catalog-artifact.test.mjsscripts/catalog-deployment.test.mjsscripts/check-catalog-deployment.mjsscripts/check-catalog-loading.mjsscripts/check-conformance-artifact.mjsscripts/check-conformance-site.mjsscripts/check-local-catalog-worker.mjsscripts/stage-conformance-deployment.mjswrangler.catalog.jsonc
💤 Files with no reviewable changes (8)
- wrangler.catalog.jsonc
- scripts/check-local-catalog-worker.mjs
- deploy/catalog/_headers
- scripts/build-conformance-site.mjs
- scripts/stage-conformance-deployment.mjs
- scripts/catalog-deployment.test.mjs
- scripts/check-catalog-deployment.mjs
- scripts/check-conformance-site.mjs
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)
.github/workflows/chart-library-benchmarks.yml (1)
156-164: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize
catalog-distpublication.Two pushes to
maincan run this job at the same time, both publish from the samecatalog-distbase, and one push will fail on a non-fast-forward update. Add a job-level concurrency group so publications queue instead of racing.Proposed fix
publish-catalog: if: github.event_name == 'push' && github.ref == 'refs/heads/main' needs: - validate - conformance runs-on: ubuntu-latest timeout-minutes: 10 + concurrency: + group: catalog-dist-publication + cancel-in-progress: false permissions: contents: write🤖 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 @.github/workflows/chart-library-benchmarks.yml around lines 156 - 164, Add a job-level concurrency configuration to publish-catalog so all catalog-dist publications share one group and queue rather than run concurrently. Keep the existing publish-catalog conditions, dependencies, permissions, and timeout unchanged.
🤖 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 @.github/workflows/chart-library-benchmarks.yml:
- Around line 25-27: Disable persisted checkout credentials for the
artifact-producing jobs by adding persist-credentials: false to the
actions/checkout steps in .github/workflows/chart-library-benchmarks.yml at
lines 25-27, 60-62, 87-89, and 123-125, corresponding to validate, compare,
conformance, and stress. Leave publish-catalog unchanged because it requires
push authentication.
---
Outside diff comments:
In @.github/workflows/chart-library-benchmarks.yml:
- Around line 156-164: Add a job-level concurrency configuration to
publish-catalog so all catalog-dist publications share one group and queue
rather than run concurrently. Keep the existing publish-catalog conditions,
dependencies, permissions, and timeout unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b8432f1-c1dc-429e-b1ff-a442d7231b37
📒 Files selected for processing (2)
.github/workflows/chart-library-benchmarks.ymlAPI-FRICTION.md
🚧 Files skipped from review as they are similar to previous changes (1)
- API-FRICTION.md
1 participant
Publishes the catalog as a versioned schema-v2 artifact on
catalog-distfor native rendering by tanstack.com.Summary by CodeRabbit