RHIDP-16658: Consume backend-provided color for scalar aggregation KPI cards - #4582
Conversation
Changed Packages
|
PR Summary by QodoReturn backend-calculated colors for scalar aggregation KPI cards
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
|
🤖 Finished Review · ✅ Success · Started 12:55 PM UTC · Completed 1:03 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.33 |
Code Review by Qodo
1.
|
rhdh-qodo-merge
Bot
added
documentation
ReviewVerdict: Comment — medium-severity findings worth noting, none blocking. This PR extracts No security or critical/high-severity findings. Medium findings1. Misleading JSDoc on default scalar thresholdsFile: The JSDoc comment on Suggestion: Change to "Lower value = better" or remove the directional hint. 2. Frontend scalar card does not consume the new backend-provided colorFile: The PR title says "Consume backend-provided color for scalar aggregation KPI cards" but 3. Backward-compatibility / semver concernFile: Two changes to
These are technically breaking under strict semver. In practice, the impact is mitigated — these types are primarily API response shapes constructed only within the backend workspace, so external consumers constructing them is unlikely. However, any consumer assigning Suggestion: Consider bumping 4. Documentation gaps in
|
| # | Category | File | Description |
|---|---|---|---|
| 1 | test adequacy | packages/app-legacy/e2e-tests/utils/scorecardResponseUtils.ts:285 |
E2E fixture has total: 0 with aggregationChartDisplayColor: '#6bb300' (non-null). After this PR, backend returns null when total is 0 — fixture is stale. |
| 2 | maintenance | ScalarAggregationStrategy.ts:70 |
Color uses DEFAULT_SCALAR_AGGREGATION_KPI_RESULT_THRESHOLDS while result.thresholds falls back to DEFAULT_NUMBER_THRESHOLDS. Rules are identical today but live in separate packages and could drift. |
| 3 | style | getAggregationChartDisplayColor.ts:33 |
Function instantiates new ThresholdEvaluator() on every call. The old WeightedStatusScoreAggregationStrategy method did the same, so this is not a regression, but the constructor-injection pattern used elsewhere is more testable. |
| 4 | code organization | getAggregationChartDisplayColor.ts:29 |
Logic partially duplicates classifyNumberAgainstThresholds (both call getFirstMatchingThreshold + rule lookup). Note: direct reuse would change behavior since classifyNumberAgainstThresholds applies withStandardThresholdDefaults. |
| 5 | docs | aggregation.md:272, thresholds.md:228,230 |
Three doc sections are slightly stale: weighted result description doesn't mention null, scalar thresholds section understates backend's active use of color, defaults bullet omits new constant. |
| 6 | test adequacy | isScalarAggregation.test.ts:29 |
Test fixture missing aggregationChartDisplayColor. Test still passes (type guard checks structural properties only) but fixture is incomplete. |
Previous run
Review — comment
PR: #4582 — RHIDP-16658: Consume backend-provided color for scalar aggregation KPI cards
Summary
This PR adds backend-computed aggregationChartDisplayColor to scalar aggregation results, matching the existing pattern in WeightedStatusScoreAggregationStrategy. The implementation is well-structured:
- Extracts
getAggregationChartDisplayColorinto a shared utility, eliminating duplication between the weighted-status-score and scalar strategies. - Introduces
DEFAULT_SCALAR_AGGREGATION_KPI_RESULT_THRESHOLDSwith explicit colors (unlike the oldDEFAULT_NUMBER_THRESHOLDSwhich was colorless), enabling meaningful color computation for the default path. - Adds a required
aggregationChartDisplayColor: stringfield to theScalarAggregationResulttype inscorecard-common. - Test coverage is solid across the new utility, strategy, mappers, and router tests.
The refactor is faithful — getAggregationChartDisplayColor is identical in logic to the private method it replaces in WeightedStatusScoreAggregationStrategy, and the error-throw guard on missing color is consistent with the weighted strategy.
Findings
1. Cross-package deep import from frontend test into backend src/ (medium — style/conventions)
ScorecardHomepageCard.test.tsx introduces:
import { DEFAULT_SCALAR_AGGREGATION_KPI_RESULT_THRESHOLDS }
from '@red-hat-developer-hub/backstage-plugin-scorecard-backend/src/constants/aggregationKPIs';This is the first cross-boundary deep import from the scorecard frontend plugin into the backend's internal src/ tree. It bypasses the backend's public API surface and creates a fragile test-time dependency — any restructuring of the backend's internal modules would break this test.
Remediation: Either re-export this constant from scorecard-common (since both backend and frontend need it), or define the threshold fixture inline in the test. Since the constant represents default backend behavior, re-exporting from scorecard-common alongside DEFAULT_NUMBER_THRESHOLDS is the cleaner approach.
2. Misleading JSDoc comment on new thresholds constant (low — documentation)
The comment on DEFAULT_SCALAR_AGGREGATION_KPI_RESULT_THRESHOLDS in aggregationKPIs.ts states:
Higher value = better. Evaluated in order; first match wins.
But the rules define <10 → success and >50 → error, which means lower values are better. The neighboring DEFAULT_WEIGHTED_STATUS_SCORE_KPI_RESULT_THRESHOLDS correctly says "Higher headline percentage = better" where >=80 is success. The new comment appears to be copy-pasted without updating the direction.
Remediation: Change "Higher value = better" to "Lower value = better" (or "High count indicates problems" to match the DEFAULT_NUMBER_THRESHOLDS JSDoc it replaces).
fullsend-ai-review
Bot
added
the
requires-manual-review
ef8296c to
4083f08
Compare
|
🤖 Review · Commit: |
…gregation response Signed-off-by: Ihor Mykhno imykhno@redhat.com Assisted-By: Cursor <cursoragent@cursor.com>
4083f08 to
2fba3b8
Compare
|
|
🤖 Review · ❌ Terminated · Started 5:34 PM UTC · Ended 5:53 PM UTC Commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4582 +/- ##
=======================================
Coverage 62.37% 62.38%
=======================================
Files 2607 2608 +1
Lines 104756 104775 +19
Branches 29430 29441 +11
=======================================
+ Hits 65346 65365 +19
Misses 38819 38819
Partials 591 591
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
workspaces/scorecard/plugins/scorecard-backend/src/constants/aggregationKPIs.ts(file-level): Line 37 · [medium] documentation error
JSDoc comment on DEFAULT_SCALAR_AGGREGATION_KPI_RESULT_THRESHOLDS states 'Higher value = better' but the threshold rules define '<10' as SUCCESS and '>50' as ERROR, meaning lower values are better. Copy-paste from weighted status score constant where higher is indeed better.
Suggested fix: Change the comment to 'Lower value = better' or remove the directional statement.
| /** | ||
| * @public | ||
| */ | ||
| export type ScalarAggregationResult = ScalarAggregatedMetric & { |
There was a problem hiding this comment.
[medium] backward-compatibility
Two type changes to @public exports marked as 'patch': (1) ScalarAggregationResult gains required field aggregationChartDisplayColor: string | null, (2) WeightedStatusScoreAggregationResult.aggregationChartDisplayColor widens from string to string | null. These are technically breaking under strict semver, though practical impact is mitigated since these are API response shapes constructed only within the backend workspace.
Suggested fix: Bump scorecard-common changeset to 'minor' to signal the API surface change, or make the new field optional (aggregationChartDisplayColor?: string | null).
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 5:34 PM UTC · Completed 5:53 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $6.82 |
1 participant
Hey, I just made a Pull Request!
In this PR, we added the
result.aggregationChartDisplayColorattribute to the scalar aggregation response. The color will be calculated on the backend, which now returns only the correct value.Dropping
result.thresholdswas not implemented in this PR, as we decided that the UI may still need threshold information for certain chartsThis PR is for:
✔️ Checklist