Conversation
…ckages - Replace git tag --list strategy with package.json-driven tag validation in all 16 publish workflows; use git rev-parse to verify the exact tag exists rather than guessing the latest repo-wide tag - Update error guidance to reflect feat/** → develop → master flow - Standardize dependabot to npm-only, grouped, monthly cadence across all 16 packages; remove github-actions ecosystem updates - Add missing dependabot.yml to AuthKit-UI, ChartKit-UI, HealthKit, HooksKit, paymentkit, StorageKit
* feat: add typed chart contracts and buildChartConfig utility with bar/line/area support and theme-based color fallback * feat: add BarChart component with typed props, stacking/horizontal support, and internal config builder * feat: add LineChart and AreaChart with smooth option, area fill, stacking, and shared config builder * test: add full test suite with mocked react-chartjs-2, config validation, and 80%+ coverage * chore: add README and changeset * fix: extract shared test fixtures to resolve SonarCloud duplication
There was a problem hiding this comment.
Pull request overview
This PR converts the existing React/TS library template into a typed Chart.js-based chart kit, adding reusable Bar/Line/Area chart components, a shared config builder, public type exports, and updated CI/publishing automation.
Changes:
- Added
buildChartConfigutility + chart type contracts (ChartDataset,ChartTheme, etc.). - Introduced
BarChart,LineChart, andAreaChartcomponents with Vitest + RTL coverage and shared chart test helpers. - Updated package metadata/docs and GitHub workflows for validation/release/publish.
Reviewed changes
Copilot reviewed 29 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/index.ts | Exposes the new chart config builder from the utils barrel. |
| src/utils/buildChartConfig.ts | Adds typed transformation from datasets/theme into Chart.js config. |
| src/utils/buildChartConfig.test.ts | Unit tests for config building across variants and theme behaviors. |
| src/types/index.ts | Adds a public types barrel export. |
| src/types/chart.types.ts | Introduces shared chart data/theme/config TypeScript types. |
| src/index.ts | Exports new public types entrypoint from the package root. |
| src/components/LineChart/LineChart.types.ts | Defines typed props contract for LineChart. |
| src/components/LineChart/LineChart.tsx | Implements LineChart using react-chartjs-2 + buildChartConfig. |
| src/components/LineChart/LineChart.test.tsx | Component tests for LineChart behavior (mocked canvas). |
| src/components/LineChart/index.ts | Barrel export for LineChart and its props type. |
| src/components/index.ts | Exposes new chart components from the components barrel. |
| src/components/BarChart/index.ts | Barrel export for BarChart and its props type. |
| src/components/BarChart/BarChart.types.ts | Defines typed props contract for BarChart. |
| src/components/BarChart/BarChart.tsx | Implements BarChart (stacked/horizontal options). |
| src/components/BarChart/BarChart.test.tsx | Component tests for BarChart behavior (mocked canvas). |
| src/components/AreaChart/index.ts | Barrel export for AreaChart and its props type. |
| src/components/AreaChart/AreaChart.types.ts | Defines typed props contract for AreaChart. |
| src/components/AreaChart/AreaChart.tsx | Implements AreaChart using area variant config + options. |
| src/components/AreaChart/AreaChart.test.tsx | Component tests for AreaChart behavior (mocked canvas). |
| src/tests/chart-test-utils.ts | Shared RTL test helpers and fixtures for chart components. |
| README.md | Replaces template README with package docs, types, and usage examples. |
| package.json | Renames package, adds Chart.js deps, updates metadata and scripts. |
| package-lock.json | Lockfile updates for new dependencies and package metadata. |
| ciscode-ui-chart-kit-0.1.0.tgz | Adds an npm pack artifact to the repo (should be removed). |
| ciscode-reactts-developerkit-1.0.0.tgz | Adds an npm pack artifact to the repo (should be removed). |
| .vscode/mcp.json | Adds VS Code MCP server config for GitHub integration. |
| .github/workflows/release-check.yml | Updates CI checks and SonarCloud scanning on PRs to master. |
| .github/workflows/publish.yml | Updates publish pipeline to validate version/tag and use npm provenance. |
| .github/workflows/pr-validation.yml | Adds PR validation workflow for the develop branch. |
| .github/dependabot.yml | Enables monthly npm dependency updates via Dependabot. |
| .github/CODEOWNERS | Adds code ownership rules for review routing. |
| .changeset/initial-release.md | Adds changeset describing the initial chart kit release. |
| function resolveColor(dataset: ChartDataset, index: number, theme: ChartTheme): string { | ||
| return dataset.color ?? theme.colors[index % theme.colors.length]; | ||
| } |
There was a problem hiding this comment.
theme.colors can legally be an empty array per the ChartTheme type, but resolveColor uses index % theme.colors.length. When colors.length is 0 this produces NaN and returns undefined, which then flows into backgroundColor/borderColor (e.g., 'undefined33' for area) at runtime. Consider enforcing a non-empty palette at the type level (e.g., tuple type) and/or throwing a clear error when theme.colors.length === 0.
| function applyAreaOpacity(hex: string): string { | ||
| return `${hex}33`; | ||
| } |
There was a problem hiding this comment.
applyAreaOpacity blindly appends "33" to whatever string is passed. If callers provide a non-#RRGGBB hex (e.g., #RGB, #RRGGBBAA) or a non-hex CSS color, this will create an invalid color value. Since the API is typed as string, consider validating/normalizing to 6-digit hex before appending alpha, or switching to an rgba(...) conversion approach.
| @@ -0,0 +1,93 @@ | |||
| import { describe, it, expect, vi } from 'vitest'; | |||
| import { render, screen } from '@testing-library/react'; | |||
There was a problem hiding this comment.
screen is imported but never used in this test file. With @typescript-eslint/no-unused-vars: 'warn' and --max-warnings=0, this will fail CI lint. Remove the unused screen import.
| import { render, screen } from '@testing-library/react'; | |
| import { render } from '@testing-library/react'; |
| @@ -0,0 +1,59 @@ | |||
| import { describe, it, expect, vi } from 'vitest'; | |||
| import { render, screen } from '@testing-library/react'; | |||
There was a problem hiding this comment.
screen is imported but never used in this test file. With @typescript-eslint/no-unused-vars: 'warn' and --max-warnings=0, this will fail CI lint. Remove the unused screen import.
| import { render, screen } from '@testing-library/react'; | |
| import { render } from '@testing-library/react'; |
| @@ -0,0 +1,95 @@ | |||
| import { describe, it, expect, vi } from 'vitest'; | |||
| import { render, screen } from '@testing-library/react'; | |||
There was a problem hiding this comment.
screen is imported but never used in this test file. With @typescript-eslint/no-unused-vars: 'warn' and --max-warnings=0, this will fail CI lint. Remove the unused screen import.
| import { render, screen } from '@testing-library/react'; | |
| import { render } from '@testing-library/react'; |
|
| const tickFont: Record<string, unknown> = {}; | ||
| if (theme.fontFamily) tickFont.family = theme.fontFamily; | ||
| if (theme.fontSize) tickFont.size = theme.fontSize; | ||
|
|
There was a problem hiding this comment.
theme.fontSize is checked with a truthy check, so a value of 0 (or other falsy numeric values) will be ignored. If the intent is “only skip when undefined”, prefer an explicit undefined check (same applies to fontFamily if empty string should be allowed).
| export const __utils_placeholder = true; | ||
|
|
||
| export * from './noop'; | ||
| export * from './buildChartConfig'; |
There was a problem hiding this comment.
This barrel still exports __utils_placeholder, which becomes part of the public API via src/index.ts. Now that real utilities are exported, the placeholder export should be removed to avoid shipping template artifacts to consumers.
| export * from './NoopButton'; | ||
| export * from './BarChart'; | ||
| export * from './LineChart'; | ||
| export * from './AreaChart'; |
There was a problem hiding this comment.
This barrel exports __components_placeholder, which becomes part of the public API. Since this package now has real components, remove the placeholder export to avoid exposing template-only symbols to consumers.
| } | ||
|
|
||
| export interface ChartTheme { | ||
| colors: string[]; |
There was a problem hiding this comment.
ChartTheme.colors is typed as string[], which allows an empty array. Since chart config generation requires at least one color (or a defined fallback), consider typing this as a non-empty tuple (e.g., [string, ...string[]]) and/or documenting/validating the requirement to prevent runtime invalid colors.
| colors: string[]; | |
| colors: [string, ...string[]]; |
| | `react-dom` | ≥ 18 | | ||
|
|
||
| Anything not exported from `src/index.ts` is considered private. | ||
| `chart.js` and `react-chartjs-2` are bundled — you do **not** need to install them separately. |
There was a problem hiding this comment.
The README says chart.js and react-chartjs-2 are “bundled”. In this PR they are added as runtime dependencies, and tsup.config.ts only externalizes react/react-dom (so these libs may also be bundled into dist). Align the documentation with the packaging strategy (bundled vs installed deps) to avoid confusing consumers.
| `chart.js` and `react-chartjs-2` are bundled — you do **not** need to install them separately. | |
| Only `react` and `react-dom` must be installed separately as peer dependencies when using this package. |
| "dependencies": { | ||
| "chart.js": "^4.5.1", | ||
| "react-chartjs-2": "^5.3.1" | ||
| } |
There was a problem hiding this comment.
chart.js and react-chartjs-2 are added as runtime dependencies, but the build config (tsup.config.ts) only externalizes react/react-dom, which means these packages are likely being bundled into dist as well. This can increase bundle size and potentially lead to duplicate Chart.js/react-chartjs-2 instances. Decide on one approach: either externalize them and keep as dependencies/peerDependencies, or remove them from dependencies if you truly want them bundled.
| - name: SonarCloud Scan | ||
| if: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.sonar == 'true' }} | ||
| uses: SonarSource/sonarqube-scan-action@v6 | ||
| env: | ||
| SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} | ||
| SONAR_HOST_URL: ${{ env.SONAR_HOST_URL }} | ||
| with: |
There was a problem hiding this comment.
SonarCloud scan will fail on pull requests where SONAR_TOKEN is not available (e.g., PRs from forks). Add a conditional to skip this job/step when the secret is missing or when github.event.pull_request.head.repo.fork is true, so CI stays green for external contributions.
| permissions: | ||
| contents: read | ||
| statuses: write | ||
|
|
There was a problem hiding this comment.
The report job requests statuses: write and posts a commit status. On pull_request workflows (especially from forks), token permissions can be read-only, which can make this job fail and mask the actual check results. Consider making this step conditional (e.g., only for non-fork PRs) or removing it in favor of standard job statuses/checks.
| npm-dependencies: | ||
| patterns: | ||
| - '*' | ||
| assignees: |
There was a problem hiding this comment.
Dependabot assignees typically expects GitHub usernames (not an org/team slug). If CISCODE-MA/cloud-devops is intended to be a team, use reviewers with org/team syntax instead, or replace with individual assignees; otherwise Dependabot may reject/ignore this config.
| assignees: | |
| reviewers: |



Summary
Why
Checklist
npm run lintpassesnpm run typecheckpassesnpm testpassesnpm run buildpassesnpx changeset) if this affects consumersNotes