CONSOLE-5029: Refactor withDashboardResources in with-dashboard-resources.tsx into … #15927
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@cajieh: This pull request references CONSOLE-5031 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request refactors the console dashboard resource management from a higher-order component pattern (withDashboardResources) to a hook-based architecture. A new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh 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
component/dashboard
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
2 times, most recently
from
17a6b03 to
d56e111
Compare
openshift-merge-robot
added
the
needs-rebase
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
from
d56e111 to
faafcc4
Compare
openshift-merge-robot
removed
the
needs-rebase
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
from
faafcc4 to
2fc0cfb
Compare
| <RecentEvent /> | ||
| <RecentEvents /> | ||
| </ActivityBody> | ||
| <RecentEventFooter /> |
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
from
2fc0cfb to
d11eec1
Compare
| <OngoingActivity projectName={projectName} /> | ||
| <RecentEvent projectName={projectName} viewEvents={viewEvents} /> | ||
| </ActivityBody> | ||
| <RecentEventFooter projectName={projectName} viewEvents={viewEvents} /> |
There was a problem hiding this comment.
Redundant after the refactoring! The footer functionality is preserved with the same logic:
const shouldShowFooter = events?.loaded && events?.data && events.data.length > 50;
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
4 times, most recently
from
46dbf18 to
0fa4c50
Compare
|
@cajieh: This pull request references CONSOLE-5031 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
2 times, most recently
from
ff8cacf to
0538151
Compare
|
/retest |
cajieh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@cajieh: This pull request references CONSOLE-5029 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@cajieh: This pull request references CONSOLE-5029 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@cajieh: This pull request references CONSOLE-5029 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@cajieh: This pull request references CONSOLE-5029 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@cajieh: This pull request references CONSOLE-5029 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@cajieh: This pull request references CONSOLE-5029 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@cajieh: This pull request references CONSOLE-5029 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@cajieh: This pull request references CONSOLE-5029 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
from
0538151 to
0fd3120
Compare
|
@cajieh: This pull request references CONSOLE-5029 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@frontend/public/components/dashboard/dashboards-page/cluster-dashboard/activity-card.tsx`:
- Around line 130-148: The computed allPrometheusActivities currently calls
a.properties.isActivity without null-checking (isActivity is optional) and
recalculates identical queryResults in both filter and map; fix by iterating
prometheusActivities once inside the useMemo, for each activity compute
queryResults once from a.properties.queries using
prometheusResults.getIn([...,'data']) and then only call
a.properties.isActivity?.(queryResults) (or check typeof isActivity ===
'function') before including the activity in the result; return objects with
component: a.properties.component and results: queryResults for activities that
pass the check.
In
`@frontend/public/components/dashboard/dashboards-page/cluster-dashboard/details-card.tsx`:
- Around line 120-133: The fetch flow in the useEffect (fetchK8sVersion) is
storing the raw Response instead of parsed data and doesn't check response.ok;
update fetchK8sVersion to await fetch('version'), verify response.ok and throw
an error with status/text if not ok, then await response.json() (or
response.text() if the endpoint returns plain text) and pass the parsed value
into setK8sVersion; on errors call setK8sVersionError with a meaningful Error
(include status/message); keep references to flagPending(openshiftFlag) and the
dependency on openshiftFlag as-is.
- Around line 108-109: The state k8sVersion is incorrectly typed as Response and
therefore never receives parsed JSON; change the useState for
k8sVersion/setK8sVersion to the correct data shape (e.g., the parsed version
object/interface expected by getK8sGitVersion, not Response) and type
k8sVersionError appropriately (e.g., string | Error | undefined). In the fetch
logic that calls getK8sGitVersion, await response.json() and pass that parsed
object into setK8sVersion (or adjust getK8sGitVersion to return parsed JSON) so
components read the actual gitVersion field; update the related type/interface
used by k8sVersion to match the JSON shape.
In
`@frontend/public/components/dashboard/dashboards-page/cluster-dashboard/health-item.tsx`:
- Around line 231-236: The fallback hardcoded message in the healthState
assignment is not internationalized; update the expression that sets
SubsystemHealth (the healthState const) so the fallback message uses the
translation function t(), e.g. replace the raw string 'Health handler not
available' with t('health.handlerNotAvailable') or t('Health handler not
available') consistent with project i18n keys; ensure
subsystem.healthHandler?.(...) ?? { state: HealthState.NOT_AVAILABLE, message:
t(...) } so it matches how URLHealthItem uses t().
🧹 Nitpick comments (5)
frontend/packages/console-shared/src/components/dashboard/utilization-card/TopConsumerPopover.tsx (3)
116-158: Consider a more declarative approach fortop5Datacomputation.The imperative
forloop with mutation works, but a functional approach would be cleaner and more consistent with React idioms. Also,top5Datais recomputed on every render since it's not memoized.♻️ Suggested functional refactor
- const top5Data = []; - - const [data, error] = getPrometheusQueryResponse(prometheusResults, query); - const bodyData = getInstantVectorStats(data, metric); - - if (k8sResource && consumerLoaded && !consumersLoadError) { - for (const d of bodyData) { - const consumerExists = consumerData.some( - (consumer) => - getName(consumer) === d.metric[metric] && - (model.namespaced ? getNamespace(consumer) === d.metric.namespace : true), - ); - if (consumerExists) { - top5Data.push({ ...d, y: humanize(d.y).string }); - } - if (top5Data.length === 5) { - break; - } - } - } + const [data, error] = getPrometheusQueryResponse(prometheusResults, query); + const bodyData = getInstantVectorStats(data, metric); + + const top5Data = useMemo(() => { + if (!k8sResource || !consumerLoaded || consumersLoadError) { + return []; + } + return bodyData + .filter((d) => + consumerData.some( + (consumer) => + getName(consumer) === d.metric[metric] && + (model.namespaced ? getNamespace(consumer) === d.metric.namespace : true), + ), + ) + .slice(0, 5) + .map((d) => ({ ...d, y: humanize(d.y).string })); + }, [k8sResource, consumerLoaded, consumersLoadError, bodyData, consumerData, model, metric, humanize]);
129-133: Trailing comma in dependency array may cause issues in some build configurations.While valid in modern JS/TS, the trailing comma after
isOpenin the dependency array can occasionally cause issues with certain bundler configurations.const prometheusQueries = useMemo(() => (isOpen ? [{ query, namespace }] : []), [ query, namespace, isOpen, ]);
288-292: Unused props inPopoverBodyPropstype.The
topConsumersanderrorprops are defined in the type but are not used in thePopoverBodycomponent signature (line 117). These appear to be remnants from the previous HOC-based implementation.🧹 Clean up unused type properties
type PopoverBodyProps = PopoverProps & { - topConsumers?: DataPoint[][]; - error?: boolean; isOpen: boolean; };frontend/public/components/dashboard/dashboards-page/cluster-dashboard/health-item.tsx (1)
95-151:healthStatusescomputation should be memoized.The
healthStatusesarray is computed on every render. With multiple operator subsystems, this could be expensive. Consider wrapping inuseMemo.♻️ Memoize healthStatuses
- const healthStatuses = operatorSubsystems.map((o, index) => { + const healthStatuses = useMemo(() => operatorSubsystems.map((o, index) => { const operatorResources = o.resources.reduce((acc, r) => { acc[r.prop] = resources[uniqueResource(r, index).prop] || {}; return acc; }, {}); if (Object.keys(operatorResources).some((resource) => operatorResources[resource].loadError)) { return { health: HealthState.NOT_AVAILABLE }; } if (Object.keys(operatorResources).some((resource) => !operatorResources[resource].loaded)) { return { health: HealthState.LOADING }; } const operatorStatuses = o.getOperatorsWithStatuses(operatorResources); if (!operatorStatuses.length) { return { health: HealthState.OK }; } const importantStatuses = getMostImportantStatuses(operatorStatuses); return { health: importantStatuses[0].status.health, count: importantStatuses.length, }; - }); + }), [operatorSubsystems, resources]);frontend/public/components/dashboard/dashboards-page/cluster-dashboard/inventory-card.tsx (1)
39-56: Mapper loading error is swallowed silently.The catch block only logs to console. Consider setting an error state to surface mapper load failures in the UI, or at minimum, ensure
resolvedMapperis used as fallback (which it is on line 88).
| const allPrometheusActivities = useMemo( | ||
| () => | ||
| prometheusActivities | ||
| .filter((a) => { | ||
| const queryResults = a.properties.queries.map( | ||
| (q) => prometheusResults.getIn([q, 'data']) as PrometheusResponse, | ||
| ); | ||
| }; | ||
| }, [ | ||
| watchK8sResource, | ||
| stopWatchK8sResource, | ||
| watchPrometheus, | ||
| stopWatchPrometheusQuery, | ||
| resourceActivities, | ||
| prometheusActivities, | ||
| ]); | ||
|
|
||
| const allResourceActivities = useMemo( | ||
| () => | ||
| _.flatten( | ||
| resourceActivities.map((a, index) => { | ||
| const k8sResources = _.get( | ||
| resources, | ||
| [uniqueResource(a.properties.k8sResource, index).prop, 'data'], | ||
| [], | ||
| ) as FirehoseResult['data']; | ||
| return k8sResources | ||
| .filter((r) => (a.properties.isActivity ? a.properties.isActivity(r) : true)) | ||
| .map((r) => ({ | ||
| resource: r, | ||
| timestamp: a.properties.getTimestamp ? a.properties.getTimestamp(r) : null, | ||
| component: a.properties.component, | ||
| })); | ||
| }), | ||
| ), | ||
| [resourceActivities, resources], | ||
| ); | ||
|
|
||
| const allPrometheusActivities = useMemo( | ||
| () => | ||
| prometheusActivities | ||
| .filter((a) => { | ||
| const queryResults = a.properties.queries.map( | ||
| (q) => prometheusResults.getIn([q, 'data']) as PrometheusResponse, | ||
| ); | ||
| return a.properties.isActivity(queryResults); | ||
| }) | ||
| .map((a) => { | ||
| const queryResults = a.properties.queries.map( | ||
| (q) => prometheusResults.getIn([q, 'data']) as PrometheusResponse, | ||
| ); | ||
| return { | ||
| component: a.properties.component, | ||
| results: queryResults, | ||
| }; | ||
| }), | ||
| [prometheusActivities, prometheusResults], | ||
| ); | ||
|
|
||
| const resourcesLoaded = useMemo( | ||
| () => | ||
| resourceActivities.every((a, index) => { | ||
| const uniqueProp = uniqueResource(a.properties.k8sResource, index).prop; | ||
| return resources[uniqueProp]?.loaded || resources[uniqueProp]?.loadError; | ||
| }), | ||
| [resourceActivities, resources], | ||
| ); | ||
|
|
||
| const queriesLoaded = useMemo( | ||
| () => | ||
| prometheusActivities.every((a) => | ||
| a.properties.queries.every( | ||
| (q) => | ||
| prometheusResults.getIn([q, 'data']) || prometheusResults.getIn([q, 'loadError']), | ||
| ), | ||
| ), | ||
| [prometheusActivities, prometheusResults], | ||
| ); | ||
|
|
||
| return ( | ||
| <OngoingActivityBody | ||
| loaded={resourcesLoaded && queriesLoaded} | ||
| resourceActivities={allResourceActivities} | ||
| prometheusActivities={allPrometheusActivities} | ||
| /> | ||
| ); | ||
| }, | ||
| ), | ||
| ); | ||
|
|
||
| const RecentEventFooter = withDashboardResources( | ||
| ({ watchK8sResource, stopWatchK8sResource, resources }) => { | ||
| const { t } = useTranslation(); | ||
| useEffect(() => { | ||
| watchK8sResource(eventsResource); | ||
| return () => { | ||
| stopWatchK8sResource(eventsResource); | ||
| }; | ||
| }, [watchK8sResource, stopWatchK8sResource]); | ||
|
|
||
| const events = resources.events as FirehoseResult<EventKind[]>; | ||
| const shouldShowFooter = events?.loaded && events?.data && events.data.length > 50; | ||
|
|
||
| if (!shouldShowFooter) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| <Divider /> | ||
| <CardFooter> | ||
| <Link to={viewEvents} data-test="events-view-all-link"> | ||
| {t('public~View all events')} | ||
| </Link> | ||
| </CardFooter> | ||
| </> | ||
| ); | ||
| }, | ||
| ); | ||
| return a.properties.isActivity(queryResults); | ||
| }) | ||
| .map((a) => { | ||
| const queryResults = a.properties.queries.map( | ||
| (q) => prometheusResults.getIn([q, 'data']) as PrometheusResponse, | ||
| ); | ||
| return { | ||
| component: a.properties.component, | ||
| results: queryResults, | ||
| }; | ||
| }), | ||
| [prometheusActivities, prometheusResults], |
There was a problem hiding this comment.
Potential null reference and duplicate computation in allPrometheusActivities.
Two issues:
a.properties.isActivity(queryResults)(line 137) is called without checking ifisActivityis defined, but the extension type shows it's optional (perDashboardsOverviewPrometheusActivitydefinition).queryResultsis computed identically in both filter and map callbacks.
🐛 Fix null safety and optimize
const allPrometheusActivities = useMemo(
() =>
- prometheusActivities
- .filter((a) => {
- const queryResults = a.properties.queries.map(
- (q) => prometheusResults.getIn([q, 'data']) as PrometheusResponse,
- );
- return a.properties.isActivity(queryResults);
- })
- .map((a) => {
- const queryResults = a.properties.queries.map(
- (q) => prometheusResults.getIn([q, 'data']) as PrometheusResponse,
- );
- return {
- component: a.properties.component,
- results: queryResults,
- };
- }),
+ prometheusActivities
+ .map((a) => {
+ const queryResults = a.properties.queries.map(
+ (q) => prometheusResults.getIn([q, 'data']) as PrometheusResponse,
+ );
+ return { activity: a, queryResults };
+ })
+ .filter(({ activity, queryResults }) =>
+ activity.properties.isActivity ? activity.properties.isActivity(queryResults) : true,
+ )
+ .map(({ activity, queryResults }) => ({
+ component: activity.properties.component,
+ results: queryResults,
+ })),
[prometheusActivities, prometheusResults],
);🤖 Prompt for AI Agents
In
`@frontend/public/components/dashboard/dashboards-page/cluster-dashboard/activity-card.tsx`
around lines 130 - 148, The computed allPrometheusActivities currently calls
a.properties.isActivity without null-checking (isActivity is optional) and
recalculates identical queryResults in both filter and map; fix by iterating
prometheusActivities once inside the useMemo, for each activity compute
queryResults once from a.properties.queries using
prometheusResults.getIn([...,'data']) and then only call
a.properties.isActivity?.(queryResults) (or check typeof isActivity ===
'function') before including the activity in the result; return objects with
component: a.properties.component and results: queryResults for activities that
pass the check.
| const [k8sVersion, setK8sVersion] = useState<Response>(); | ||
| const [k8sVersionError, setK8sVersionError] = useState(); |
There was a problem hiding this comment.
Type mismatch: k8sVersion should be the parsed JSON, not Response.
The state is typed as Response, but getK8sGitVersion (line 143) expects parsed JSON data containing version information, not a raw Response object. This will cause the version to never display correctly.
🐛 Fix fetch and type
- const [k8sVersion, setK8sVersion] = useState<Response>();
+ const [k8sVersion, setK8sVersion] = useState<any>();And update the fetch logic:
const fetchK8sVersion = async () => {
try {
- const version = await fetch('version');
- setK8sVersion(version);
+ const response = await fetch('version');
+ if (!response.ok) {
+ throw new Error(`HTTP error: ${response.status}`);
+ }
+ const version = await response.json();
+ setK8sVersion(version);
} catch (error) {
setK8sVersionError(error);
}
};📝 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 [k8sVersion, setK8sVersion] = useState<Response>(); | |
| const [k8sVersionError, setK8sVersionError] = useState(); | |
| const [k8sVersion, setK8sVersion] = useState<any>(); | |
| const [k8sVersionError, setK8sVersionError] = useState(); |
🤖 Prompt for AI Agents
In
`@frontend/public/components/dashboard/dashboards-page/cluster-dashboard/details-card.tsx`
around lines 108 - 109, The state k8sVersion is incorrectly typed as Response
and therefore never receives parsed JSON; change the useState for
k8sVersion/setK8sVersion to the correct data shape (e.g., the parsed version
object/interface expected by getK8sGitVersion, not Response) and type
k8sVersionError appropriately (e.g., string | Error | undefined). In the fetch
logic that calls getK8sGitVersion, await response.json() and pass that parsed
object into setK8sVersion (or adjust getK8sGitVersion to return parsed JSON) so
components read the actual gitVersion field; update the related type/interface
used by k8sVersion to match the JSON shape.
There was a problem hiding this comment.
These CodeRabbitAI suggestions could be considered later, as they are beyond the scope of refactoring the withDashboardResources and Firehose components.
There was a problem hiding this comment.
@cajieh — Understood! Keeping this PR focused on the withDashboardResources and Firehose refactoring is the right call for maintainability and reviewability.
Would you like me to open a GitHub issue to track the k8sVersion type mismatch fix as a follow-up? That way it won't be lost, and someone can pick it up in a future PR. Just let me know!
| useEffect(() => { | ||
| if (flagPending(openshiftFlag)) { | ||
| return; | ||
| } | ||
| const fetchK8sVersion = async () => { | ||
| try { | ||
| const version = await fetch('version'); | ||
| setK8sVersion(version); | ||
| } catch (error) { | ||
| setK8sVersionError(error); | ||
| } | ||
| const fetchK8sVersion = async () => { | ||
| try { | ||
| const version = await fetch('version'); | ||
| setK8sVersion(version); | ||
| } catch (error) { | ||
| setK8sVersionError(error); | ||
| } | ||
| }; | ||
| fetchK8sVersion(); | ||
| }, [openshiftFlag, watchK8sResource, stopWatchK8sResource]); | ||
| const serviceLevelTitle = useServiceLevelTitle(); | ||
| }; | ||
| fetchK8sVersion(); | ||
| }, [openshiftFlag]); |
There was a problem hiding this comment.
Missing response status check and JSON parsing.
The fetch call doesn't check response.ok before using the response, and critically, doesn't parse the JSON body. The Response object is stored directly instead of the parsed data.
This is related to the type issue above - the entire fetch flow needs correction.
🤖 Prompt for AI Agents
In
`@frontend/public/components/dashboard/dashboards-page/cluster-dashboard/details-card.tsx`
around lines 120 - 133, The fetch flow in the useEffect (fetchK8sVersion) is
storing the raw Response instead of parsed data and doesn't check response.ok;
update fetchK8sVersion to await fetch('version'), verify response.ok and throw
an error with status/text if not ok, then await response.json() (or
response.text() if the endpoint returns plain text) and pass the parsed value
into setK8sVersion; on errors call setK8sVersionError with a meaningful Error
(include status/message); keep references to flagPending(openshiftFlag) and the
dependency on openshiftFlag as-is.
| const healthState: SubsystemHealth = subsystem.healthHandler?.( | ||
| queryResults, | ||
| t, | ||
| k8sResult, | ||
| infrastructure, | ||
| ) ?? { state: HealthState.NOT_AVAILABLE, message: 'Health handler not available' }; |
There was a problem hiding this comment.
Hardcoded English string violates i18n requirements.
The fallback message 'Health handler not available' is not wrapped in t() for translation, unlike the equivalent in URLHealthItem (line 176-177).
🌐 Apply i18n translation
const healthState: SubsystemHealth = subsystem.healthHandler?.(
queryResults,
t,
k8sResult,
infrastructure,
- ) ?? { state: HealthState.NOT_AVAILABLE, message: 'Health handler not available' };
+ ) ?? { state: HealthState.NOT_AVAILABLE, message: healthStateMessage(HealthState.NOT_AVAILABLE, t) };🤖 Prompt for AI Agents
In
`@frontend/public/components/dashboard/dashboards-page/cluster-dashboard/health-item.tsx`
around lines 231 - 236, The fallback hardcoded message in the healthState
assignment is not internationalized; update the expression that sets
SubsystemHealth (the healthState const) so the fallback message uses the
translation function t(), e.g. replace the raw string 'Health handler not
available' with t('health.handlerNotAvailable') or t('Health handler not
available') consistent with project i18n keys; ensure
subsystem.healthHandler?.(...) ?? { state: HealthState.NOT_AVAILABLE, message:
t(...) } so it matches how URLHealthItem uses t().
fc73419 to
26f594d
Compare
| const events = useMemo( | ||
| () => ({ | ||
| data: eventsData, | ||
| loaded: eventsLoaded, | ||
| loadError: eventsLoadError, | ||
| }), | ||
| [eventsData, eventsLoaded, eventsLoadError], | ||
| ); |
There was a problem hiding this comment.
Looks like we need to adapt k8sWatchResource to FirehoseResult here and in a few other places.
Can we instead modify RecentEventsBody to directly accept these 3 props instead? This would also remove the FirehoseResult type from that component
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
from
26f594d to
6e30068
Compare
openshift-ci
bot
added
component/sdk
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
from
6e30068 to
e757ba3
Compare
|
/retest |
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
from
e757ba3 to
a62f4ae
Compare
|
/test e2e-gcp-console |
cajieh
force-pushed
the
refactor-withDashboard-resources-component
branch
from
a62f4ae to
62151c4
Compare
|
/test e2e-gcp-console |
|
@cajieh: 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. |
7 participants
Summary
Removes the
withDashboardResourcesHigher-Order Component in favor of targeted React hooks, modernizing 17 components across 10 files as part of the "Remove and Update Firehose" epic.Problem
The
withDashboardResourcesHOC injected all capabilities into every component regardless of actual needs:Solution
Replace monolithic HOC with targeted hooks based on actual usage:
Key Change:
useDynamicDashboardResourcesBridges declarative hooks with imperative plugin requirements:. Enables runtime resource watching without violating Rules of Hooks
const { watchResource, stopWatchResource, results } = useDynamicDashboardResources();Plugin extensions can register 0-N resources at runtime
Benefits
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.