Skip to content

Conversation

@cajieh
Copy link
Contributor

@cajieh cajieh commented Jan 19, 2026

Summary

Removes the withDashboardResources Higher-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 withDashboardResources HOC injected all capabilities into every component regardless of actual needs:

  • Performance: Unnecessary re-renders on unrelated data changes
  • Clarity: Unclear dependencies per component
  • Technical debt: Class-based HOC + Redux + Firehose coupling
  • Type safety: Complex HOC intersection types

Solution

Replace monolithic HOC with targeted hooks based on actual usage:

Screenshot 2026-01-22 at 1 56 02 PM

Key Change: useDynamicDashboardResources

Bridges 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

  extensions.forEach((ext, i) => {
    watchResource(key(i), ext.properties.k8sResource);
  });

Benefits

  • Performance: Components only watch what they need
  • Code quality: -271 lines, clearer intent
  • Type safety: Specific return types per hook
  • Modern React: Hooks over HOCs, functional components

Summary by CodeRabbit

  • Refactor
    • Modernized dashboard component data-fetching architecture across cluster and project dashboards, improving code organization and maintainability while preserving all existing functionality.

✏️ Tip: You can customize this high-level summary in your review settings.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 19, 2026
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 19, 2026

@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.

Details

In response to this:

NOT READY FOR READY

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
Copy link

coderabbitai bot commented Jan 19, 2026

📝 Walkthrough

Walkthrough

This pull request refactors the console dashboard resource management from a higher-order component pattern (withDashboardResources) to a hook-based architecture. A new useDynamicK8sWatchResources hook provides imperative APIs for dynamically registering and unregistering Kubernetes resources at runtime. The useDashboardResources hook is updated to accept an additional parameter for URL fetching. Multiple dashboard and plugin components are refactored to use these hooks directly instead of receiving resource data through props injected by the HOC. The withDashboardResources export and its associated types are removed. Component public signatures are simplified by removing dependency on DashboardItemProps and shifting data loading into components via hooks.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is truncated and incomplete, ending with an ellipsis without conveying the full scope of this major architectural refactor that replaces the withDashboardResources HOC with hooks across 17 components. Complete the title to reflect the full change: 'Refactor withDashboardResources HOC into hook-based patterns' or similar, ensuring it communicates the main transformation without requiring external context.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 19, 2026
@openshift-ci openshift-ci bot added component/core Related to console core functionality component/dashboard Related to dashboard approved Indicates a PR has been approved by an approver from all required OWNERS files. component/metal3 Related to metal3-plugin component/shared Related to console-shared labels Jan 19, 2026
@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch 2 times, most recently from 17a6b03 to d56e111 Compare January 19, 2026 21:07
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2026
@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch from d56e111 to faafcc4 Compare January 19, 2026 21:16
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2026
@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch from faafcc4 to 2fc0cfb Compare January 19, 2026 21:18
<RecentEvent />
<RecentEvents />
</ActivityBody>
<RecentEventFooter />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redundant!

@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch from 2fc0cfb to d11eec1 Compare January 19, 2026 21:33
<OngoingActivity projectName={projectName} />
<RecentEvent projectName={projectName} viewEvents={viewEvents} />
</ActivityBody>
<RecentEventFooter projectName={projectName} viewEvents={viewEvents} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redundant after the refactoring! The footer functionality is preserved with the same logic:
const shouldShowFooter = events?.loaded && events?.data && events.data.length > 50;

@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch 4 times, most recently from 46dbf18 to 0fa4c50 Compare January 21, 2026 14:18
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 21, 2026

@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.

Details

In 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 cajieh force-pushed the refactor-withDashboard-resources-component branch 2 times, most recently from ff8cacf to 0538151 Compare January 21, 2026 15:39
@cajieh
Copy link
Contributor Author

cajieh commented Jan 22, 2026

/retest

@cajieh cajieh changed the title [WIP] CONSOLE-5031: Refactor withDashboardResources in with-dashboard-resources.tsx into … CONSOLE-5029: Refactor withDashboardResources in with-dashboard-resources.tsx into … Jan 22, 2026
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2026
Comment on lines 58 to 65
const events = useMemo(
() => ({
data: eventsData,
loaded: eventsLoaded,
loadError: eventsLoadError,
}),
[eventsData, eventsLoaded, eventsLoadError],
);
Copy link
Member

@logonoff logonoff Jan 26, 2026

Choose a reason for hiding this comment

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

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 cajieh force-pushed the refactor-withDashboard-resources-component branch from 26f594d to 6e30068 Compare January 27, 2026 13:34
@openshift-ci openshift-ci bot added component/sdk Related to console-plugin-sdk plugin-api-changed Categorizes a PR as containing plugin API changes labels Jan 27, 2026
@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch from 6e30068 to e757ba3 Compare January 27, 2026 16:03
@cajieh
Copy link
Contributor Author

cajieh commented Jan 27, 2026

/retest

@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch from e757ba3 to a62f4ae Compare January 27, 2026 20:17
@cajieh
Copy link
Contributor Author

cajieh commented Jan 28, 2026

/test e2e-gcp-console

@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch from a62f4ae to 62151c4 Compare January 28, 2026 17:26
@cajieh
Copy link
Contributor Author

cajieh commented Jan 28, 2026

/test e2e-gcp-console

@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch from 62151c4 to 7aceae2 Compare January 29, 2026 12:46
Comment on lines 108 to 109
const [k8sVersion, setK8sVersion] = useState<Response>();
const [k8sVersionError, setK8sVersionError] = useState();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create a jira ticket to track these issues.

}>;

export type RecentEventsBodyProps = {
events: FirehoseResult<EventKind[]>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't have any impact on the public SDK since it is internal.

Comment on lines 108 to 109
const [k8sVersion, setK8sVersion] = useState<Response>();
const [k8sVersionError, setK8sVersionError] = useState();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.

);
};

const OngoingActivity = connect(mapStateToProps)(OngoingActivityComponent);
Copy link
Member

Choose a reason for hiding this comment

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

As some extra refactoring you can also replace the connect HOC with the useSelector hook. Not required though!

let request: PrometheusResponse, requestError: any;
let isLoading = false;
const { duration } = useUtilizationDuration();
export const PrometheusUtilizationItem: React.FC<PrometheusUtilizationItemProps> = ({
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
export const PrometheusUtilizationItem: React.FC<PrometheusUtilizationItemProps> = ({
export const PrometheusUtilizationItem: FC<PrometheusUtilizationItemProps> = ({

);
};

export const PrometheusMultilineUtilizationItem: React.FC<PrometheusMultilineUtilizationItemProps> = ({
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const PrometheusMultilineUtilizationItem: React.FC<PrometheusMultilineUtilizationItemProps> = ({
export const PrometheusMultilineUtilizationItem: FC<PrometheusMultilineUtilizationItemProps> = ({

Comment on lines +100 to +101
const resourceKey = uniqueResource(a.properties.k8sResource, index).prop;
stopWatchResource(resourceKey);
Copy link
Member

Choose a reason for hiding this comment

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

If some extension which we're watching is removed from resourceActivity, e.g., due to a flag changing, does this hook ensure that it is cleaned up properly?

e.g.: foo plugin provides bar DashboardOverviewResource extension, and we watch that resource. baz feature flag is turned off and disables bar extension. does this cleanup function handle this edge case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@logonoff Yes. The dynamic extension changes are correctly handled including cleanup. The useDynamicK8sWatchResources hook provides the stopWatchResource callback, and the resourceActivities dependency ensures it's called at the right time.

The resourceActivities dependency ensures proper cleanup:

When a feature flag disables an extension (like "bar"), the effect at lines 87-104 handles it automatically:

  1. resourceActivities is recomputed (lines 78-85) and filters out the disabled extension
  2. The effect's cleanup function runs (lines 98-103), stopping all watches from the previous resourceActivities list - including "bar"
  3. The effect re-runs and starts watches only for the current resourceActivities - excluding "bar"

Example flow:

  • Initial: resourceActivities = [foo, bar] → watches both
  • Flag disables bar: resourceActivities = [foo] → effect dependency changes
  • Cleanup runs: stops foo and bar
  • Effect runs: starts foo only
  • Result: bar is no longer watched

Why it works:
The effect cleanup function captures the previous resourceActivities value in its closure from when the effect was created, so it correctly stops the watch for "bar" even though "bar" is no longer in the current resourceActivities.
Let me know if you want to discuss this further.

);
};

const OngoingActivity = connect(mapStateToProps)(OngoingActivityComponent);
Copy link
Member

Choose a reason for hiding this comment

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

Can be refactored to use useSelector/useConsoleSelector if you are interested in more refactoring work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is out of scope for the current task, as these lines weren't directly modified but rather moved around by Git.

return () => {
stopWatchResource(resource.prop);
if (additionalResources) {
additionalResources.forEach((r) => stopWatchResource(r.prop));
Copy link
Member

Choose a reason for hiding this comment

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

same question for re. proper cleanup but for additionalResources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same cleanup pattern mentioned earlier applies here.

@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch from 7aceae2 to e0134ce Compare January 29, 2026 16:44
export * from './useCopyLoginCommands';
export * from './useQuickStartContext';
export * from './useUser';
export * from './useDynamicK8sWatchResources';
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid introducing more exports in the console-shared barrel as it's been a big cause of import cycles and overall build slowness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Good catch! 👍
Actually, this is redundant because all the imports are already pointing directly to the file path (@console/shared/src/hooks/useDynamicK8sWatchResources). Removed!

@cajieh cajieh force-pushed the refactor-withDashboard-resources-component branch from e0134ce to 0165351 Compare January 29, 2026 18:40
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 30, 2026

@cajieh: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copy link
Member

@logonoff logonoff left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2026
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 30, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cajieh, logonoff

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cajieh
Copy link
Contributor Author

cajieh commented Jan 30, 2026

/hold
Holding until we get a second LGTM.

QE Approver
/assign @yapei

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2026
@XiyunZhao
Copy link
Contributor

/assign @XiyunZhao

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dashboard Related to dashboard component/metal3 Related to metal3-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. plugin-api-changed Categorizes a PR as containing plugin API changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants