-
Notifications
You must be signed in to change notification settings - Fork 670
CONSOLE-5029: Refactor withDashboardResources in with-dashboard-resources.tsx into … #15927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
CONSOLE-5029: Refactor withDashboardResources in with-dashboard-resources.tsx into … #15927
Conversation
|
@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 |
17a6b03 to
d56e111
Compare
d56e111 to
faafcc4
Compare
faafcc4 to
2fc0cfb
Compare
| <RecentEvent /> | ||
| <RecentEvents /> | ||
| </ActivityBody> | ||
| <RecentEventFooter /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant!
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.
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;
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. |
ff8cacf to
0538151
Compare
|
/retest |
| const events = useMemo( | ||
| () => ({ | ||
| data: eventsData, | ||
| loaded: eventsLoaded, | ||
| loadError: eventsLoadError, | ||
| }), | ||
| [eventsData, eventsLoaded, eventsLoadError], | ||
| ); |
There was a problem hiding this comment.
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
26f594d to
6e30068
Compare
6e30068 to
e757ba3
Compare
|
/retest |
e757ba3 to
a62f4ae
Compare
|
/test e2e-gcp-console |
a62f4ae to
62151c4
Compare
|
/test e2e-gcp-console |
62151c4 to
7aceae2
Compare
| const [k8sVersion, setK8sVersion] = useState<Response>(); | ||
| const [k8sVersionError, setK8sVersionError] = useState(); |
There was a problem hiding this comment.
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[]>; |
There was a problem hiding this comment.
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.
| const [k8sVersion, setK8sVersion] = useState<Response>(); | ||
| const [k8sVersionError, setK8sVersionError] = useState(); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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> = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
| export const PrometheusUtilizationItem: React.FC<PrometheusUtilizationItemProps> = ({ | |
| export const PrometheusUtilizationItem: FC<PrometheusUtilizationItemProps> = ({ |
| ); | ||
| }; | ||
|
|
||
| export const PrometheusMultilineUtilizationItem: React.FC<PrometheusMultilineUtilizationItemProps> = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export const PrometheusMultilineUtilizationItem: React.FC<PrometheusMultilineUtilizationItemProps> = ({ | |
| export const PrometheusMultilineUtilizationItem: FC<PrometheusMultilineUtilizationItemProps> = ({ |
| const resourceKey = uniqueResource(a.properties.k8sResource, index).prop; | ||
| stopWatchResource(resourceKey); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- resourceActivities is recomputed (lines 78-85) and filters out the disabled extension
- The effect's cleanup function runs (lines 98-103), stopping all watches from the previous resourceActivities list - including "bar"
- 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
7aceae2 to
e0134ce
Compare
| export * from './useCopyLoginCommands'; | ||
| export * from './useQuickStartContext'; | ||
| export * from './useUser'; | ||
| export * from './useDynamicK8sWatchResources'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
e0134ce to
0165351
Compare
|
@cajieh: all tests passed! 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. |
logonoff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold QE Approver |
|
/assign @XiyunZhao |
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.