feat(scorecard): implement blueprint architecture for grouped layouts#3282
feat(scorecard): implement blueprint architecture for grouped layouts#3282Eswaraiahsapram wants to merge 5 commits into
Conversation
Eswaraiahsapram
requested review from
a team,
christoph-jerolimov and
its-mitesh-kumar
as code owners
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
ReviewFindingsCritical
High
Medium
Low
Info
Previous runReviewFindingsLow
Info
Previous run (2)ReviewFindingsHigh
Medium
Low
Info
Previous run (3)ReviewReason: stale-head The review agent reviewed commit |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| * extensions (grid + list) into the Catalog entity pages. | ||
| * | ||
| * The plugin ships both layouts; platform engineers enable/disable individual | ||
| * layouts via app-config.yaml. No separate module registration is needed |
There was a problem hiding this comment.
[medium] behavioral-contract-change
scorecardCatalogModule now bundles layout extensions that change the default UI for existing consumers. When layouts are registered, the entity tab renders ScorecardLayoutSwitcher instead of the legacy EntityScorecardContent.
Suggested fix: Ship layout extensions in a separate opt-in module or ensure they are disabled by default.
| success: '#e8f5e9', | ||
| warning: '#fff3e0', | ||
| error: '#fce4ec', | ||
| }; |
There was a problem hiding this comment.
[medium] hardcoded-colors
statusTileBg and statusTileText use hardcoded hex colors that will not adapt to dark mode or custom themes. Rest of codebase uses resolveStatusColor() through the theme.
Suggested fix: Derive colors from MUI theme palette or use alpha(resolvedColor, 0.12) for background tints.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3282 +/- ##
==========================================
- Coverage 53.95% 53.92% -0.03%
==========================================
Files 2379 2384 +5
Lines 86136 86235 +99
Branches 23882 23918 +36
==========================================
+ Hits 46471 46500 +29
- Misses 38073 38143 +70
Partials 1592 1592
*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:
|
|
|
||
| /** @alpha */ | ||
| export interface ScorecardLayoutItem { | ||
| title: string; |
There was a problem hiding this comment.
[low] edge-case
The fallback layouts[selectedIndex] ?? layouts[0] masks a potential stale-index state. In practice layouts are static extension inputs resolved at boot time, but adding a useEffect to clamp selectedIndex would make the component more robust.
Suggested fix: Add useEffect(() => { if (selectedIndex >= layouts.length) setSelectedIndex(0); }, [layouts.length, selectedIndex]);
| return ( | ||
| <Box> | ||
| {layouts.length > 1 && ( | ||
| <Box display="flex" justifyContent="flex-end" mb={2}> |
There was a problem hiding this comment.
[low] error-handling
Toggle buttons use layout.title as React key. Duplicate titles would cause reconciliation issues. Currently mitigated by hardcoded unique titles.
fullsend-ai-review
Bot
added
the
ready-for-merge
Eswaraiahsapram
force-pushed
the
group-metrics-RHIDP-13955
branch
from
58c014f to
97f9226
Compare
|
| gap={2.5} | ||
| > | ||
| {/* show groups if they are configured for entity tab */} | ||
| {false && |
There was a problem hiding this comment.
[critical] logic-error
The grouped-card rendering path is gated behind a hardcoded {false && ...} condition, making the entire GroupCard/MetricTile rendering dead code. The component computes groupedMetrics and ungroupedMetrics maps but never uses either. This defeats the PR's stated purpose of implementing grouped scorecard layouts. The PR ships ~100 lines of dead code including GroupCard, MetricTile, statusTileBg/statusTileText maps, and getEvaluation helper.
Suggested fix: Either remove the false && guard and enable the grouped rendering, or remove the dead GroupCard/MetricTile code entirely and defer to a follow-up PR. If intentionally staged, remove the ready-for-merge label.
| ); | ||
| })} | ||
|
|
||
| {scorecards.length > 0 && |
There was a problem hiding this comment.
[high] logic-error
Even if the false && guard were removed, the flat rendering block iterates over all scorecards rather than ungroupedMetrics. Every metric assigned to a group would appear both inside its GroupCard and again as an individual Scorecard tile, causing duplicate rendering. The ungroupedMetrics array is computed but never consumed.
Suggested fix: Change scorecards.map(...) to ungroupedMetrics.map(...) in the second rendering block so only metrics not assigned to any group render as individual cards.
| * limitations under the License. | ||
| */ | ||
|
|
||
| export const ScorecardEntityListView = () => { |
There was a problem hiding this comment.
[medium] api-contract
ScorecardEntityListView does not accept ScorecardLayoutProps (the groups prop). The blueprint factory always passes { groups: config.groups } to loaded components. The list layout silently discards its configuration, violating the blueprint's contract.
Suggested fix: Accept ScorecardLayoutProps as the component's props type, matching the blueprint's expectations.
|
|
||
| export const ScorecardLayoutSwitcher = ({ | ||
| layouts, | ||
| }: { |
There was a problem hiding this comment.
[low] edge-case
If the layouts prop changes dynamically, stale selectedIndex state could reference a removed layout. The fallback silently resets rendering but leaves state inconsistent for subsequent toggles.
Suggested fix: Add a useEffect that clamps selectedIndex to layouts.length - 1 when the layouts array changes.
| resolveStatusColor, | ||
| resolveMetricTranslation, | ||
| } from '../../utils'; | ||
| import { useTranslation } from '../../hooks/useTranslation'; |
There was a problem hiding this comment.
[low] pattern-inconsistency
statusTileBg and statusTileText use hardcoded hex color strings rather than theme-aware values. Existing components consistently use useTheme() and resolveStatusColor() for dark-mode compatibility.
Suggested fix: Replace hardcoded hex maps with theme-aware color derivation.
| import { EntityScorecardContent } from './EntityScorecardContent'; | ||
|
|
||
| export { EntityScorecardContent }; | ||
| export { ScorecardEntityContentGridView } from './ScorecardEntityContentGridView'; |
There was a problem hiding this comment.
[low] pattern-inconsistency
New exports use direct re-export syntax while the existing pattern in this file uses import-then-re-export.
Suggested fix: Match the existing import-then-re-export pattern.
fullsend-ai-review
Bot
removed
the
ready-for-merge
2 participants
Hey, I just made a Pull Request!
Fix - https://redhat.atlassian.net/browse/RHIDP-13955
Description:
Add blueprint pattern for scorecard metric grouping with dynamic layout extensions discoverable through Backstage NFS.
Changes:
Screenshot
How to Test
Config to Enable Both Layouts
✔️ Checklist