Skip to content

feat(scorecard): add entity-page sparkline charts for time-series metrics - #4573

Open
Eswaraiahsapram wants to merge 7 commits into
redhat-developer:mainfrom
Eswaraiahsapram:feat/scorecard-entity-sparkline-cards-ui
Open

feat(scorecard): add entity-page sparkline charts for time-series metrics#4573
Eswaraiahsapram wants to merge 7 commits into
redhat-developer:mainfrom
Eswaraiahsapram:feat/scorecard-entity-sparkline-cards-ui

Conversation

@Eswaraiahsapram

@Eswaraiahsapram Eswaraiahsapram commented Sep 3, 2026

Copy link
Copy Markdown
Member

Hey, I just made a Pull Request!

Fix - https://redhat.atlassian.net/browse/RHIDP-15576

What

Adds sparkline (area chart) visualization support for entity-page scorecard metrics whose defaultVisualization is sparkline. This is the foundation PR — shared chart components and utilities are included here and will be reused by the homepage sparkline PR that follows.

What changed

New components

  • SparklineChart — Recharts-based area chart with gradient fill, error-dot markers, hover tooltip, and threshold legend
  • SparklineTooltip / SparklineLegend — supporting chart sub-components
  • EntitySparklineCard — entity-page card that fetches time-series data and renders a sparkline with a "View data sources" dialog for collector metadata
  • EntityMetricCard — routing component that renders EntitySparklineCard or the existing Scorecard card based on defaultVisualization

New API methods

  • getMetricTimeSeriesGET /metrics/catalog/:kind/:namespace/:name/time-series
  • getMetricCollectorsGET /metrics/:metricId/collectors

New hooks

  • useMetricTimeSeriesuseQuery-based hook for 30-day entity metric time series
  • useMetricCollectorsuseQuery-based hook for collector metadata (fetched only when the data-sources dialog is open)

New utilities

  • timeSeriesChartData — maps API points to chart-ready data with interpolation for error gaps
  • sparklineLegend — builds threshold legend items with color + line-style pairing
  • sparklineChartModel — shared view-model factory used by both entity and homepage cards
  • metricVisualizationisSparklineVisualization() helper
  • timeSeriesRange — computes the default 30-day ISO-8601 range

Refactors

  • DataSourcesDialog now accepts generic SourceRow[] instead of building rows internally
  • Extracted collectorSourceRows.ts (for sparkline metrics) and metricSourceRows.ts (for existing donut metrics) as separate row builders
  • All collector labels (GitHub, Jira, empty value --, unavailable status N/A) are now translated via i18n keys instead of hardcoded strings

Translations

  • Added 6 new dataSourcesDialog.* keys to ref.ts and all locale files (de, es, fr, it, ja)

Screen Recording

Screen.Recording.2026-09-07.at.3.18.23.PM.mov

How to test

  1. Configure a catalog entity with DORA metric providers (or any metric with defaultVisualization: sparkline)
  2. Navigate to the entity's Scorecard tab
  3. Verify the sparkline chart renders with a 30-day trend line
  4. Click the menu → "View data sources" and verify collectors are listed
  5. Error days should show red dots on the chart with tooltip messages

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

@rhdh-gh-app

rhdh-gh-app Bot commented Sep 3, 2026

Copy link
Copy Markdown

Important

This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-scorecard workspaces/scorecard/plugins/scorecard minor v4.2.0

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:36 PM UTC · Completed 7:44 PM UTC

Commit: b56fa91 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $2.16

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review — Approve

Scope: Adds sparkline (area chart) visualization for entity-page scorecard metrics whose defaultVisualization is sparkline. Includes new API client methods, hooks, chart components, shared utilities, a DataSourcesDialog refactor, i18n keys, and comprehensive tests.

This is a well-structured, cleanly decomposed feature PR. The code follows existing project patterns, has thorough input validation and error handling, and ships with excellent test coverage across all new and modified modules.

Correctness

  • API client (ScorecardApiClient): The three new methods (getMetricTimeSeries, getAggregationTimeSeries, getMetricCollectors) validate inputs, check response shapes, and surface clear error messages. Error handling follows the same try/catch pattern as existing methods. getMetricCollectors correctly uses encodeURIComponent for the metric ID in the URL path.
  • Hooks: useMetricTimeSeries properly gates the query on entity presence and non-empty metric ID; range computation happens inside queryFn while the cache key uses a constant (TIME_SERIES_DEFAULT_RANGE_DAYS), avoiding unnecessary refetches. useMetricCollectors cleanly suppresses loading/error state when enabled is false, preventing flash-of-loading when the dialog hasn't been opened yet.
  • DataSourcesDialog refactor: The dialog is now a pure presentation component accepting pre-built SourceRow[] and optional buckets. Row-building responsibility is properly separated into metricSourceRows.ts and collectorSourceRows.ts. The ThresholdLegend is conditionally rendered only when buckets is provided, which is correct for collector-mode usage where threshold filtering doesn't apply.
  • EntitySparklineCard: Correctly memoizes the chart model, lazily fetches collectors only when the dialog opens AND the metric has collectorIds, and handles loading/error/empty states.
  • Chart data utilities: interpolatePlotValue keeps the sparkline continuous across error gaps via linear interpolation — a sensible UX choice. getSparklineYDomain handles edge cases (empty data, single-value series) with appropriate fallback padding.
  • extractPluginName change: The regex update from .split('.') to .split(/[.:]/) is backward-compatible for existing dot-separated metric IDs and correctly handles colon-separated collector IDs like github:deploymentWorkflowRuns.
  • Test coverage: All new components, hooks, and utility functions have dedicated test suites covering happy paths, edge cases (null values, empty responses, disabled state), and error scenarios.

Security

No concerns. No authentication or authorization changes. All new API calls use the existing fetchApi plumbing. Translation strings are rendered via React (no dangerouslySetInnerHTML). Entity metadata values used in URL paths come from the Backstage catalog, not direct user input.

Intent & Coherence

The change matches its stated purpose and is appropriately scoped as a foundation PR. Shared chart components and utilities are designed for reuse by the homepage sparkline PR that follows. The changeset correctly selects a minor bump for a new user-visible feature.

Style & Conventions

  • All new files include the Red Hat license header.
  • Type imports use the import type form consistently.
  • JSDoc comments on public functions and hooks.
  • Test patterns match existing test suites in the scorecard plugin.
  • i18n keys follow the existing dataSourcesDialog.* namespace with translations provided for all five locales.

Documentation

The PR body is thorough with component descriptions, API endpoints, test instructions, and a checklist. In-code documentation (JSDoc, type comments) is present on all public-facing utilities. The checklist items for changeset, docs, tests, and screenshots are tracked but some are unchecked — the author should verify these before merge.

Observations

  • [provenance-warning] Prior review provenance: unverifiable-wrong-app. Prior review was created by a different GitHub App than expected; severity anchoring was skipped for this run.
  • [info] ScorecardEntityContentGridView grouping change: The guard if (metricsInOrder.length > 0) before groupedMetrics.set(...) was removed, meaning empty groups are now added to the map. This is functionally safe because the downstream .filter(Boolean) handles null returns, but it is a subtle behavioral change worth noting.
  • [info] Dev mock duplication: dev/legacy.tsx and dev/mocks.ts contain near-identical mock API implementations for the new methods. This is expected for dev server configuration but adds maintenance surface.

No medium or higher severity findings. The PR is safe to merge.

Previous run

Review

Findings

Medium

  • [error-handling-idiom] workspaces/scorecard/plugins/scorecard/src/hooks/useMetricCollectors.tsx — Every existing hook wraps queryFn in a try-catch that converts non-Error throwables to a translated message (e.g., t('errors.fetchError', { error: String(err) })). useMetricCollectors passes a bare queryFn with no error wrapping, breaking the established error-handling pattern. useMetricTimeSeries in the same PR correctly follows this pattern.

  • [Hook parameter convention] workspaces/scorecard/plugins/scorecard/src/hooks/useMetricCollectors.tsx — All existing hooks that accept an enabled flag use a destructured options-object pattern (e.g., useAggregatedScorecard({ aggregationId, enabled })). useMetricCollectors uses positional parameters (metricId, enabled), diverging from the established convention.

  • [stale-api-report] workspaces/scorecard/plugins/scorecard/report.api.md — The main report.api.md is missing 5 new translation keys (collectorStatusTooltip, collectorEmptyValue, collectorUnavailableStatus, pluginGithub, pluginJira) that were added to report-alpha.api.md and report-legacy.api.md. The report jumps directly from dataSourcesDialog.statusTooltip to dataSourcesDialog.columns.plugin.
    Remediation: Regenerate report.api.md by running the API Extractor.

  • [missing-doc] workspaces/scorecard/plugins/scorecard/README.md — The README Features section lists four features but does not mention sparkline chart visualization. This is a user-visible feature that administrators and integrators should know about.
    Remediation: Add a fifth bullet to the Features list describing sparkline chart support for time-series metrics.

Low

  • [edge-case] workspaces/scorecard/plugins/scorecard/src/utils/timeSeriesChartData.ts:136toAggregationSparklinePoints determines error labels based solely on point.status === 'error'. If a point has status: 'success' but value: null, no error label is set, causing the tooltip to display null. By contrast, toMetricSparklinePoints defensively checks point.value === null as a fallback.

  • [api-surface] workspaces/scorecard/plugins/scorecard/report-alpha.api.mdpluginGithub and pluginJira translation keys are hardcoded to two specific collector providers. Future providers would need new keys, though a fallback to extractPluginName exists for unknown prefixes.

  • [pattern-inconsistency] workspaces/scorecard/plugins/scorecard/src/api/index.tsgetMetricCollectors encodes metricId with encodeURIComponent, but getAggregationTimeSeries and getMetricTimeSeries do not encode their path segments. This is consistent with the pre-existing codebase pattern (most methods do not encode), making getMetricCollectors the outlier.

  • [naming-convention] workspaces/scorecard/plugins/scorecard/dev/mocks.ts — Type-only symbols (ScorecardApi, ScorecardOptions, etc.) imported with value import instead of import type. Sibling file legacy.tsx uses import type for the same symbols.

  • [interface-extension] workspaces/scorecard/plugins/scorecard/src/api/types.ts — Three new mandatory methods added to the ScorecardApi interface. Not part of the declared public API surface; any downstream consumer that deep-imports this internal interface will get clear compile errors at the missing methods. Minor version bump correctly signals additive changes.

  • [behavioral-change] workspaces/scorecard/plugins/scorecard/src/utils/translationUtils.tsextractPluginName regex changed from split('.') to split(/[.:]/) to handle colon-separated collector IDs. Internal function, backward-compatible for dot-separated IDs.

  • [internal-component-contract] workspaces/scorecard/plugins/scorecard/src/components/MetricGroupCard/DataSourcesDialog.tsxDataSourcesDialogProps interface refactored: metrics replaced with rows, isLoading, error, buckets. Internal component, not part of public API. Row computation now happens in callers (MetricGroupCard and EntitySparklineCard).


Labels: Feature PR adding new sparkline chart capability to the scorecard workspace.

Previous run (2)

Review

Verdict: comment

This PR adds sparkline (area chart) visualization support for entity-page scorecard metrics, including new chart components, API methods, hooks, utilities, and i18n strings. The architecture is well-structured: components follow the existing project patterns, hooks use the established useQuery + UseResponseData<T> pattern, and the code includes comprehensive test coverage across all new modules. The security posture is clean — all data rendering goes through React's auto-escaping, API calls use Backstage's authenticated fetch wrapper, and input validation is present on all new API methods.

Two medium-severity findings require attention before merge. Several low-severity items are noted for consideration.


Medium

1. Stale API report — report.api.md not regenerated

File: workspaces/scorecard/plugins/scorecard/report.api.md

The PR updates report-alpha.api.md and report-legacy.api.md with 5 new dataSourcesDialog.* translation keys (collectorStatusTooltip, collectorEmptyValue, collectorUnavailableStatus, pluginGithub, pluginJira), but report.api.md is not updated. This file exports the same scorecardTranslationRef and is now inconsistent with the other two reports.

Remediation: Run the API report generation command (e.g., yarn backstage-repo-tools api-reports) to regenerate report.api.md.

2. Breaking ScorecardApi interface — dev mocks will not compile

File: workspaces/scorecard/plugins/scorecard/src/api/types.ts (line ~105)

Three new required methods are added to the ScorecardApi interface (getAggregationTimeSeries, getMetricTimeSeries, getMetricCollectors). The MockScorecardApi classes in dev/mocks.ts (line 57) and dev/legacy.tsx (line 82) both implements ScorecardApi but are not updated with these methods, causing TypeScript compilation errors in the dev environment.

Remediation: Add stub implementations for the three new methods to both MockScorecardApi classes. Alternatively, consider making the new methods optional on the interface if downstream consumers implement ScorecardApi directly.


Low

3. URL construction inconsistency in getMetricCollectors

getMetricCollectors uses a template literal with explicit encodeURIComponent() while every other method in the class (including the two new time-series methods) uses new URL(). This creates a split pattern for URL construction within the same class.

4. Hardcoded 'DORA' in collectorStatusTooltip translation

The collectorStatusTooltip string hardcodes "DORA" but the tooltip displays for any metric with isCollector: true, not only DORA metrics. Consider using a generic term or a translation interpolation variable.

5. useMetricCollectors hook missing error-wrapping pattern

Unlike the sibling hooks (useMetricTimeSeries, useAggregatedScorecard), useMetricCollectors does not wrap its queryFn in a try/catch with translated error messages. The practical risk is low since the API client handles errors internally, but it breaks the convention.

6. getAggregationTimeSeries has no caller in this PR

The method is defined, tested, and added to the interface, but no hook or component in this PR calls it. The PR body notes a follow-up homepage sparkline PR will use it. Consider whether this uncalled method belongs in this PR or the follow-up.

7. Test gap — no test for collector fetch error

EntitySparklineCard.test.tsx covers loading state, time-series fetch errors, empty data, and successful rendering, but does not test the path where useMetricCollectors returns an error.

8. README not updated with sparkline feature

The README Features section does not mention sparkline time-series charts. The changeset describes the feature, but a README update would help users discover it.

9. extractPluginName regex change

The split regex changed from '.' to /[.:]/ to support colon-delimited collector IDs (e.g., github:deploymentWorkflowRuns). This is functionally correct and tested, but the behavioral change to an existing utility is not called out in the PR description.

10. Hardcoded pluginGithub/pluginJira translation keys

The pluginLabels map must be manually extended for each new collector integration. The fallback (extractPluginName) already capitalizes the first segment, so these keys only improve brand-name casing.

Previous run (3)

Review

Verdict: comment — medium-severity findings worth noting but none that should block.

Summary

This PR adds sparkline chart support to the scorecard entity page, enabling time-series visualization for DORA and other metrics alongside the existing score donut cards. The change spans 52 files (+4101/−315) across API client extensions, new React components, custom hooks, utility functions, translations, and comprehensive tests.

Architecture is clean: EntityMetricCard acts as a visualization router (sparkline vs. donut), EntitySparklineCard wires up data fetching and chart rendering, and SparklineChart is a reusable Recharts wrapper. The DataSourcesDialog refactoring from raw MetricResult[] to pre-built SourceRow[] improves separation of concerns and enables collector-based data source rows.

Test coverage is strong — ~20 new test files cover API client methods, hooks, utility functions, chart components, and the integration between EntitySparklineCard and data source dialogs.

Findings

1. Missing changeset [medium · process]

File: (repository root — no .changeset/*.md file present)

The PR adds a user-visible feature (feat prefix) but includes no changeset. Per CONTRIBUTING.md and .fullsend/AGENTS.md, a changeset with minor bump level is expected for new features. The PR checklist also shows all items unchecked.

Remediation: Add a changeset via npx changeset selecting the @red-hat-developer-hub/backstage-plugin-scorecard package with a minor bump.

2. Entity path segments not URL-encoded in getMetricTimeSeries [low · defense-in-depth]

File: workspaces/scorecard/plugins/scorecard/src/api/index.ts (new method getMetricTimeSeries)

The URL is built with entity kind, namespace, and name interpolated directly into the path:

const url = new URL(
  `${baseUrl}/metrics/catalog/${entity.kind}/${entity.metadata.namespace}/${entity.metadata.name}/time-series`,
);

While entity names from the Backstage catalog are typically safe, this is inconsistent with getMetricCollectors which properly uses encodeURIComponent(metricId). If an entity name contained / or other URL-special characters, the URL would be malformed.

Note: This pattern is consistent with existing methods in the same file (e.g., getScorecards), so it is pre-existing rather than introduced by this PR.

Remediation: Consider wrapping path segments with encodeURIComponent() for defense-in-depth:

`${baseUrl}/metrics/catalog/${encodeURIComponent(entity.kind)}/${encodeURIComponent(entity.metadata.namespace)}/${encodeURIComponent(entity.metadata.name)}/time-series`

3. Hardcoded plugin labels may not scale [low · maintainability]

File: workspaces/scorecard/plugins/scorecard/src/translations/ref.ts and collectorSourceRows.ts

Plugin labels for collectors (pluginGithub, pluginJira) are hardcoded as translation keys and passed via a pluginLabels map. When a new collector provider is added (e.g., PagerDuty, GitLab), a new translation key and mapping would need to be added manually. The fallback to extractPluginName handles unknown providers by capitalizing the prefix from the collector ID, which is reasonable, but the hardcoded map adds ongoing maintenance.

Remediation: Consider whether the extractPluginName fallback alone is sufficient, or document the pattern for adding new collector providers.

What looks good

  • Clean component extraction: EntityMetricCard centralizes the visualization-type routing, eliminating duplicated status/translation logic from both EntityScorecardContent and ScorecardEntityContentGridView.
  • Smart data fetching: useMetricCollectors is gated by enabled so collector data is only fetched when the data-sources dialog opens and the metric has collector IDs — no wasted requests.
  • Robust chart data handling: The interpolatePlotValue function linearly interpolates null/error points to keep the sparkline continuous, with proper edge-case handling (no prev, no next, both missing).
  • Comprehensive i18n: All 5 new translation keys are added across all 7 supported languages (en, de, es, fr, it, ja, ref).
  • Thorough test coverage: API client, hooks, utility functions, and component integration are all well-tested with edge cases.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Sep 3, 2026
@Eswaraiahsapram
Eswaraiahsapram force-pushed the feat/scorecard-entity-sparkline-cards-ui branch from b56fa91 to 3dd8c8b Compare September 4, 2026 08:51
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 8:53 AM UTC · Ended 9:09 AM UTC

Commit: 3dd8c8b · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Sep 4, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:53 AM UTC · Completed 9:09 AM UTC

Commit: 3dd8c8b · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $9.51

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:11 AM UTC · Completed 9:53 AM UTC

Commit: 46c6489 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $13.56

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment enhancement New feature or request and removed requires-manual-review Review requires human judgment labels Sep 4, 2026
@Eswaraiahsapram
Eswaraiahsapram force-pushed the feat/scorecard-entity-sparkline-cards-ui branch from 46c6489 to 159c8f3 Compare September 7, 2026 06:10
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 7, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 6:12 AM UTC · Ended 6:39 AM UTC

Commit: 159c8f3 · View workflow run →

@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.04665% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.70%. Comparing base (ada985e) to head (2dec824).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4573      +/-   ##
==========================================
+ Coverage   62.58%   62.70%   +0.11%     
==========================================
  Files        2634     2649      +15     
  Lines      105187   105619     +432     
  Branches    29506    29627     +121     
==========================================
+ Hits        65833    66228     +395     
- Misses      37535    37577      +42     
+ Partials     1819     1814       -5     
Flag Coverage Δ *Carryforward flag
adoption-insights 84.77% <ø> (ø) Carriedforward from ada985e
ai-integrations 78.80% <ø> (ø) Carriedforward from ada985e
app-defaults 53.07% <ø> (ø) Carriedforward from ada985e
augment 46.67% <ø> (ø) Carriedforward from ada985e
boost 82.94% <ø> (ø) Carriedforward from ada985e
bulk-import 73.12% <ø> (ø) Carriedforward from ada985e
cost-management 13.35% <ø> (ø) Carriedforward from ada985e
dcm 73.47% <ø> (ø) Carriedforward from ada985e
e2e-adoption-insights 60.00% <ø> (ø) Carriedforward from ada985e
e2e-extensions 62.31% <ø> (ø) Carriedforward from ada985e
e2e-global-header 50.35% <ø> (ø) Carriedforward from ada985e
e2e-homepage 61.11% <ø> (ø) Carriedforward from ada985e
e2e-intelligent-assistant 46.74% <ø> (ø) Carriedforward from ada985e
e2e-orchestrator 49.52% <ø> (ø) Carriedforward from ada985e
e2e-orchestrator-plugin 49.51% <ø> (ø) Carriedforward from ada985e
e2e-quickstart 55.21% <ø> (ø) Carriedforward from ada985e
e2e-scorecard 50.11% <ø> (-0.06%) ⬇️ Carriedforward from ada985e
e2e-theme 16.36% <ø> (ø) Carriedforward from ada985e
extensions 57.37% <ø> (ø) Carriedforward from ada985e
global-floating-action-button 71.18% <ø> (ø) Carriedforward from ada985e
global-header 68.09% <ø> (ø) Carriedforward from ada985e
homepage 48.39% <ø> (ø) Carriedforward from ada985e
install-dynamic-plugins 71.94% <ø> (ø) Carriedforward from ada985e
intelligent-assistant 76.51% <ø> (ø) Carriedforward from ada985e
konflux 91.98% <ø> (ø) Carriedforward from ada985e
lightspeed 69.02% <ø> (ø) Carriedforward from ada985e
mcp-integrations 84.46% <ø> (ø) Carriedforward from ada985e
orchestrator 71.13% <ø> (ø) Carriedforward from ada985e
quickstart 63.74% <ø> (ø) Carriedforward from ada985e
sandbox 79.56% <ø> (ø) Carriedforward from ada985e
scorecard 88.46% <89.04%> (+0.20%) ⬆️
theme 87.91% <ø> (ø) Carriedforward from ada985e
translations 5.12% <ø> (ø) Carriedforward from ada985e
x2a 77.18% <ø> (ø) Carriedforward from ada985e

*This pull request uses carry forward flags. Click here to find out more.


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ada985e...2dec824. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Sep 7, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:12 AM UTC · Completed 6:39 AM UTC

Commit: 159c8f3 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.78

@dzemanov dzemanov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the PR!

Tested for data with both successes and errors (both at the start / end or in the middle):

Screen.Recording.2026-09-08.at.12.44.36.mov

I am not sure if I have misconfigured something, but I see only the resulting threshold in legend.
I see we are not providing thresholds in time-series response, we might need to update that. Meanwhile you can skip fixing it or you can take if from metric result.

When I have only 1 data point:

Image What could help: The frontend should display data points for all 30 days, even if data is missing, indicating absence with "No data recorded for this date."

Only 2 data points:

Image

Translation:

Image

Collector description might need to also be provided:

Image

For development, I tried running:
yarn start in the plugins folder, but I did not see new sparkline cards:

Image This is not blocking, but worth fixing / adding different cases for future to avoid seeding database for some of the testing.

Group cards work nice:

Screen.Recording.2026-09-08.at.13.00.15.mov

There is . missing in median lead time for changes description, I will update it.

@ShiranHi, is View datasources description alright in this state?

This is description for group card:

Image

For individual dora cards, 'View datasources' shows:

Image Image

For individual "View Datasources" DORA cards, should we also include both the metric description and the collector description? It would make the metrics easier to understand, but it would mean repeating the same information on each row.
Incorporating the metric name and description directly into the collector description is not possible due to the collector's use across multiple metrics.

Current descriptions:
github:deployments: Collects GitHub deployments.
github:deploymentWorkflowRuns: Collects deployments from GitHub Actions.
github:deploymentPullRequests: Collects pull requests linked to deployments.
jira:incidents Collects Jira incidents.

Comment thread workspaces/scorecard/plugins/scorecard/src/api/index.ts Outdated
Comment thread workspaces/scorecard/plugins/scorecard/src/api/index.ts Outdated
Comment thread workspaces/scorecard/plugins/scorecard/src/hooks/useMetricTimeSeries.tsx Outdated

@ciiay ciiay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR — the shared chart stack, DataSourcesDialog refactor, and test coverage are in good shape. I agree with @dzemanov's manual testing feedback and am also requesting changes before merge.

Blocking

  1. Sparse / missing days in the 30-day window — The chart only plots API-returned points. With 1–2 data points the sparkline is misleading (flat line, no meaningful x-axis). Please fill the requested 30-day range on the frontend and mark missing days in the tooltip (e.g. "No data recorded for this date") rather than reusing errors.metricDataUnavailable.

  2. Threshold legendEntitySparklineCard currently passes only the matched rule (legendRules: matchedRule ? [matchedRule] : undefined), so users see a single band. Since the time-series response does not include thresholds yet, please derive the full legend from metric.result.thresholdResult.definition.rules while keeping line color/style tied to the current evaluation.

  3. Single-point UX — When there is only one tick, SparklineChart hides x-axis labels (xTicks.length <= 1). Please show at least one date label or an explicit insufficient-history state.

  4. i18ndataSourcesDialog.collectorStatusTooltip hardcodes "DORA" in all locales but applies to any collector-backed metric. Please generalize (interpolation or neutral wording). A native-speaker pass on the new German strings would also help.

Should address (non-blocking but recommended)

  • SonarCloud — Quality gate fails on ~3.9% duplication in new code; likely duplicated mock time-series code in dev/mocks.ts and dev/legacy.tsx — extract a shared fixture.
  • useMetricCollectors — Align error handling with useMetricTimeSeries (translated fetch errors in queryFn).
  • API response validation — Consider validating points[] entry shape, not just top-level object checks (see inline threads).
  • getAggregationTimeSeries — Fine to land here for the homepage follow-up PR, but worth a brief note in the PR description since it has no caller yet.
  • Data sources dialog content — Open question from @dzemanov on whether individual DORA cards should include metric description alongside collector description; needs product sign-off.

Happy to re-review once the sparse-data and legend items are addressed.

@Eswaraiahsapram

Copy link
Copy Markdown
Member Author

Thanks a lot @dzemanov , for reviewing this 🙏

I am not sure if I have misconfigured something, but I see only the resulting threshold in legend. I see we are not providing thresholds in time-series response, we might need to update that. Meanwhile you can skip fixing it or you can take if from metric result.

@dzemanov , this is intentional for the entity page. As per prototype, the legend shows only the matched / current threshold (e.g. Medium), and the line color/style follow that evaluation.

For homepage aggregation cards we will show all threshold bands in the legend. That is in the homepage follow-up PR.

When I have only 1 data point:

Image What could help: The frontend should display data points for all 30 days, even if data is missing, indicating absence with "No data recorded for this date."

Thanks @dzemanov, we can check with UX before changing it.

@ShiranHi, Right now, with a single sample we plot that point as-is (dot, no filled 30-day line). That matches the prototype

Filling all 30 days and showing “No data recorded for this date.” on the gaps would make the trend easier to read, but it is also a different look from the prototype (lots of empty/missing days vs one point).

@ShiranHi, which should we follow for the entity sparkline?

  1. Keep the prototype: only plot days the API returns (1–2 points stay as dots).
  2. Always show the 30-day window and mark missing days in the tooltip as “No data recorded for this date.”

@Eswaraiahsapram
Eswaraiahsapram force-pushed the feat/scorecard-entity-sparkline-cards-ui branch from 159c8f3 to 2d6f8e3 Compare September 10, 2026 09:27
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 9:28 AM UTC · Ended 9:30 AM UTC

Commit: 2d6f8e3 · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 9:28 AM UTC · Completed 9:30 AM UTC

Commit: 2d6f8e3 · View workflow run →

Runtime: claude · Model: opus → claude-opus-5

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 9:37 AM UTC · Ended 9:39 AM UTC

Commit: c1a7888 · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 9:37 AM UTC · Completed 9:39 AM UTC

Commit: c1a7888 · View workflow run →

Runtime: claude · Model: opus → claude-opus-5

@ShiranHi

Copy link
Copy Markdown

Thanks @dzemanov, we can check with UX before changing it.

@ShiranHi, Right now, with a single sample we plot that point as-is (dot, no filled 30-day line). That matches the prototype

Filling all 30 days and showing “No data recorded for this date.” on the gaps would make the trend easier to read, but it is also a different look from the prototype (lots of empty/missing days vs one point).

@ShiranHi, which should we follow for the entity sparkline?

  1. Keep the prototype: only plot days the API returns (1–2 points stay as dots).
  2. Always show the 30-day window and mark missing days in the tooltip as “No data recorded for this date.”

There are two topics here:

  1. Threshold legend: The legend should show all threshold ranges in all use cases. The current behavior, where only the current threshold is shown, is not intentional. I’ll update it so the full legend is displayed.
  2. Missing data: The prototype uses two states. When there are enough data points, the chart shows the 30-day trend, as in the Deployment Frequency card. When there is only one data point, it intentionally shows a single dot with its date and value, as in the Change Failure Rate card. I propose keeping this behavior for the entity page, since we don’t have enough information to draw a trend line for missing dates.

@Eswaraiahsapram
Eswaraiahsapram force-pushed the feat/scorecard-entity-sparkline-cards-ui branch from c1a7888 to 2dec824 Compare September 10, 2026 12:00
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 12:01 PM UTC · Ended 12:23 PM UTC

Commit: 2dec824 · View workflow run →

@sonarqubecloud

Copy link
Copy Markdown

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure (agent timed out after 20m0s without completing (timeout: 20m0s)) · Started 12:01 PM UTC · Completed 12:23 PM UTC

Commit: 2dec824 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6

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

Labels

enhancement New feature or request ready-for-merge All reviewers approved — ready to merge workspace/scorecard

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants