[RHINENG-19549] fix: fixing severity not showing number of issues #15949
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
|
Hi @Jakub007d. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Jakub007d
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThe changes refactor the LabelComponent in the InsightsPopup component by replacing the ExternalLink-wrapped ChartLabel with an inline HTML anchor element. The href attribute is computed dynamically using optional chaining ( 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/packages/insights-plugin/src/components/InsightsPopup/index.tsx`:
- Around line 33-49: LabelComponent builds an href using clusterID and
riskSorting[props.datum?.id] which can produce NaN/broken URLs when clusterID or
props.datum?.id is missing; update LabelComponent to validate inputs before
wrapping ChartLabel in an anchor: check that clusterID exists and that
riskSorting[props.datum?.id] yields a finite number (e.g., Number.isFinite) and
only then construct the href and return the <a> wrapper; otherwise return
ChartLabel directly (preserving props and style) so the legend renders without a
broken link. Reference: LabelComponent, clusterID, props.datum?.id, riskSorting,
ChartLabel.
- Around line 33-49: The anchor in LabelComponent wrapping ChartLabel is not
keyboard-focusable because SVG anchors need an explicit tabIndex; update the
returned <a> element used in LabelComponent to include tabIndex={0} alongside
the existing href, target and rel attributes so the legend link becomes
reachable by keyboard and screen readers (keep ChartLabel and its props
unchanged).
| const LabelComponent = ({ clusterID, ...props }) => { | ||
| const href = `https://console.redhat.com/openshift/insights/advisor/clusters/${clusterID}?total_risk=${ | ||
| riskSorting[props.datum?.id] + 1 | ||
| }`; | ||
|
|
||
| return ( | ||
| <a href={href} target="_blank" rel="noopener noreferrer"> | ||
| <ChartLabel | ||
| {...props} | ||
| style={{ | ||
| ...props.style, | ||
| fill: 'var(--pf-t--global--text--color--link--default)', | ||
| cursor: 'pointer', | ||
| }} | ||
| /> | ||
| </a> | ||
| ); |
There was a problem hiding this comment.
Guard link generation when clusterID or datum.id is missing
If either value is absent, riskSorting[props.datum?.id] + 1 becomes NaN, producing a broken URL while the legend still renders. Consider falling back to a non-link label in those cases.
🔧 Suggested fix
const LabelComponent = ({ clusterID, ...props }) => {
- const href = `https://console.redhat.com/openshift/insights/advisor/clusters/${clusterID}?total_risk=${
- riskSorting[props.datum?.id] + 1
- }`;
+ const riskId = props.datum?.id;
+ const riskIndex = riskId != null ? riskSorting[riskId] : undefined;
+ const totalRisk = Number.isFinite(riskIndex) ? riskIndex + 1 : undefined;
+ const href =
+ clusterID && totalRisk != null
+ ? `https://console.redhat.com/openshift/insights/advisor/clusters/${clusterID}?total_risk=${totalRisk}`
+ : undefined;
- return (
- <a href={href} target="_blank" rel="noopener noreferrer">
- <ChartLabel
- {...props}
- style={{
- ...props.style,
- fill: 'var(--pf-t--global--text--color--link--default)',
- cursor: 'pointer',
- }}
- />
- </a>
- );
+ const label = (
+ <ChartLabel
+ {...props}
+ style={{
+ ...props.style,
+ fill: 'var(--pf-t--global--text--color--link--default)',
+ cursor: href ? 'pointer' : undefined,
+ }}
+ />
+ );
+
+ return href ? (
+ <a href={href} target="_blank" rel="noopener noreferrer">
+ {label}
+ </a>
+ ) : (
+ label
+ );
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const LabelComponent = ({ clusterID, ...props }) => { | |
| const href = `https://console.redhat.com/openshift/insights/advisor/clusters/${clusterID}?total_risk=${ | |
| riskSorting[props.datum?.id] + 1 | |
| }`; | |
| return ( | |
| <a href={href} target="_blank" rel="noopener noreferrer"> | |
| <ChartLabel | |
| {...props} | |
| style={{ | |
| ...props.style, | |
| fill: 'var(--pf-t--global--text--color--link--default)', | |
| cursor: 'pointer', | |
| }} | |
| /> | |
| </a> | |
| ); | |
| const LabelComponent = ({ clusterID, ...props }) => { | |
| const riskId = props.datum?.id; | |
| const riskIndex = riskId != null ? riskSorting[riskId] : undefined; | |
| const totalRisk = Number.isFinite(riskIndex) ? riskIndex + 1 : undefined; | |
| const href = | |
| clusterID && totalRisk != null | |
| ? `https://console.redhat.com/openshift/insights/advisor/clusters/${clusterID}?total_risk=${totalRisk}` | |
| : undefined; | |
| const label = ( | |
| <ChartLabel | |
| {...props} | |
| style={{ | |
| ...props.style, | |
| fill: 'var(--pf-t--global--text--color--link--default)', | |
| cursor: href ? 'pointer' : undefined, | |
| }} | |
| /> | |
| ); | |
| return href ? ( | |
| <a href={href} target="_blank" rel="noopener noreferrer"> | |
| {label} | |
| </a> | |
| ) : ( | |
| label | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In `@frontend/packages/insights-plugin/src/components/InsightsPopup/index.tsx`
around lines 33 - 49, LabelComponent builds an href using clusterID and
riskSorting[props.datum?.id] which can produce NaN/broken URLs when clusterID or
props.datum?.id is missing; update LabelComponent to validate inputs before
wrapping ChartLabel in an anchor: check that clusterID exists and that
riskSorting[props.datum?.id] yields a finite number (e.g., Number.isFinite) and
only then construct the href and return the <a> wrapper; otherwise return
ChartLabel directly (preserving props and style) so the legend renders without a
broken link. Reference: LabelComponent, clusterID, props.datum?.id, riskSorting,
ChartLabel.
🧩 Analysis chain
🌐 Web query:
PatternFly Victory ChartLegend labelComponent documentation
💡 Result:
In PatternFly React Charts, ChartLegend is a wrapper around Victory’s VictoryLegend, so labelComponent is the “pass-through” prop you use to customize how each legend label is rendered. (v4-archive.patternfly.org)
What labelComponent does (for ChartLegend):
- Type:
React.ReactElement<any> - Purpose: provide a custom component instance to render each legend label
- PatternFly will clone your element and supply it props like
x,y,style, andtext(you can override any of these via props on your component). (v4-archive.patternfly.org)
Where it’s documented:
- PatternFly charts docs (current site): see Charts → Patterns → ChartLegend (prop table includes
labelComponent). (patternfly.org) - PatternFly v4 archive page for
ChartLegend(includes the samelabelComponentdescription and notes that Victory pass-through props apply). (v4-archive.patternfly.org)
Citations:
- 1: https://v4-archive.patternfly.org/v4/documentation/react/charts/chartlegend?utm_source=openai
- 2: https://v4-archive.patternfly.org/v4/documentation/react/charts/chartlegend?utm_source=openai
- 3: https://www.patternfly.org/charts/legends?utm_source=openai
- 4: https://v4-archive.patternfly.org/v4/documentation/react/charts/chartlegend?utm_source=openai
🌐 Web query:
Victory ChartLegend custom label component anchor link SVG
💡 Result:
To make legend labels clickable links in Victory / PatternFly ChartLegend, pass a custom labelComponent that renders an SVG <a> wrapping a VictoryLabel (the legend will inject x, y, text, style, events, etc. into your component). PatternFly explicitly documents this approach as “Legend links … using a custom label component.” [1]. Victory’s label-component contract (props like x, y, text, style, events) is documented in Victory’s type/docs. [2][3]
import * as React from "react";
import { VictoryLabel } from "victory";
// PatternFly: import { ChartLegend } from "@patternfly/react-charts/victory";
const LegendLinkLabel: React.FC<any> = (props) => {
const { datum, text, ...labelProps } = props;
// Put the URL on each legend datum (e.g. legendData: [{ name, href }])
const href = datum?.href;
if (!href) return <VictoryLabel {...labelProps} text={text} />;
return (
<a
href={href}
target="_blank"
rel="noopener noreferrer"
tabIndex={0} // keyboard focus
>
<VictoryLabel {...labelProps} text={text} />
</a>
);
};
// VictoryLegend
// <VictoryLegend data={[{ name: "Cats", href: "/cats" }]} labelComponent={<LegendLinkLabel />} />
// PatternFly ChartLegend
// <ChartLegend data={[{ name: "Cats", href: "/cats" }]} labelComponent={<LegendLinkLabel />} />Notes:
- In SVG, anchors can be rendered as
<a href="…">…</a>(some environments/libraries historically usedxlinkHref, but modern SVG2 useshref). - You can also wire
onClick/onKeyDownvia theeventsprop Victory passes into the label component (included inlabelProps). [2][3]
Sources: PatternFly Legends (“Legend links” via custom label component) [1]; Victory label-component props contract [2][3].
[1] (patternfly.org)
[2] (unpkg.com)
[3] (unpkg.com)
Citations:
- 1: https://www.patternfly.org/charts/legends
- 2: https://unpkg.com/victory-docs%406.6.8/public/docs/victory-group/index.html?utm_source=openai
- 3: https://unpkg.com/victory-docs%406.6.8/public/docs/victory-label/index.html?utm_source=openai
🌐 Web query:
SVG foreignObject anchor tag href browser support accessibility
💡 Result:
Browser support (rendering)
<foreignObject>is supported in modern Chrome/Edge, Firefox, and Safari, but not supported in Internet Explorer (incl. IE11). [1][2]- “Supported” doesn’t mean bug-free: WebKit/Safari has long-running rendering/positioning quirks with
<foreignObject>in various scenarios. [4]
<a href> inside <foreignObject> (click/navigation)
- Inside
<foreignObject>, you’re effectively embedding HTML, so an HTML<a href>can work—but only when the SVG is in a “document/DOM” context (inline<svg>in HTML, or embedded via<object>/<iframe>/<embed>). [3] - If the SVG is used as an image (
<img>, CSSbackground-image, SVG<image>), browsers often apply “image-context” restrictions (notably JS disabled and other limitations), and interactivity is unreliable/limited compared to document embedding—so don’t depend on<a>inside a<foreignObject>there. [3]
Accessibility (screen readers / keyboard)
- SVG accessibility support has historically been inconsistent across browser + screen reader pairs; patterns that work in one combo may fail in another. [5][6]
- For reliable accessible link text, the most robust pattern is usually: keep the SVG decorative (
aria-hidden="true") and provide the link’s accessible name via visible text or a visually-hidden text node (rather than depending on SVG/foreignObjectinternals being announced consistently). [5][6]
Sources
[1] Can I use: SVG element foreignObject — https://caniuse.com/mdn-svg_elements_foreignobject
[2] MDN: <foreignObject> — https://developer.mozilla.org/en-US/docs/Web/SVG/Element/foreignObject
[3] MDN: SVG as an image (restrictions; document vs image context) — https://developer.mozilla.org/en-US/docs/Web/SVG/Guides/SVG_as_an_image
[4] Example of long-running WebKit/Safari <foreignObject> issues — https://stackoverflow.com/questions/61895287/wrong-position-of-video-element-inside-svg-foreignobject-on-safari
[5] TPGi: Using ARIA to enhance SVG accessibility (notes on limited support) — https://www.tpgi.com/using-aria-enhance-svg-accessibility/
[6] Orange accessibility guidelines: Accessible SVG (notes on testing + robust patterns) — https://a11y-guidelines.orange.com/en/articles/accessible-svg/
Add tabIndex={0} to the anchor for keyboard accessibility
The <a> pattern is officially supported by PatternFly/Victory for legend links. However, anchors nested in SVG require an explicit tabIndex to be keyboard-focusable and screen-reader navigable. Add tabIndex={0} alongside the existing href, target, and rel attributes:
Suggested fix:
return (
<a
href={href}
target="_blank"
rel="noopener noreferrer"
tabIndex={0}
>
<ChartLabel {...props} ... />
</a>
);Modern href (not xlinkHref) is correct; SVG link support is solid in evergreen browsers.
🤖 Prompt for AI Agents
In `@frontend/packages/insights-plugin/src/components/InsightsPopup/index.tsx`
around lines 33 - 49, The anchor in LabelComponent wrapping ChartLabel is not
keyboard-focusable because SVG anchors need an explicit tabIndex; update the
returned <a> element used in LabelComponent to include tabIndex={0} alongside
the existing href, target and rel attributes so the legend link becomes
reachable by keyboard and screen readers (keep ChartLabel and its props
unchanged).
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
/retest |
|
Looks like a platform issue |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jakub007d, jhadvig The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@Jakub007d: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
As described in RHINENG-19549 there is currently bug in the plugin where the severity numbers and links are not showing but the icons are.
As part of the investigation i have noticed that the main issue was with ExternalLink which did not render the text and the link correctly. Changin the ExternalLink to simple a link element seems to have fixed the problem.
Screenshot before fix:
Screenshot after fix:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.