Share GlobalHealthBar via Module Federation#4875
Conversation
…t in a self-contained PlatformGlobalHealthBarFederated component that initialises the Prometheus client and provides the required context providers (MetricsTimeSpan, StartTime, IntlProvider). Expose the component in rspack
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| duration: durationSeconds, | ||
| interval: frequencySeconds, | ||
| frequency: frequencySeconds, // StartTimeProvider reads this deprecated field | ||
| }; |
There was a problem hiding this comment.
The timeSpan object is recreated on every render, which gives MetricsTimeSpanContext.Provider a new reference each time and forces all context consumers (including the charts inside PlatformGlobalHealthBar) to re-render unnecessarily.
Memoize it:
```suggestion
const timeSpan = useMemo(() => ({
query: '',
label: '',
duration: durationSeconds,
interval: frequencySeconds,
frequency: frequencySeconds, // StartTimeProvider reads this deprecated field
}), [durationSeconds, frequencySeconds]);
<br>And add `useMemo` to the React import on line 3.<br><br>— Claude Code
|
| duration: durationSeconds, | ||
| interval: frequencySeconds, | ||
| frequency: frequencySeconds, // StartTimeProvider reads this deprecated field | ||
| }; |
There was a problem hiding this comment.
timeSpan is recreated on every render, producing a new object reference each time. Since it is passed as the value to MetricsTimeSpanContext.Provider, every parent re-render will push a new context value and trigger re-renders in all consumers (StartTimeProvider, PlatformGlobalHealthBar, useMetricsTimeSpan).
Wrap it in useMemo:suggestion const timeSpan = useMemo(() => ({ query: '', label: '', duration: durationSeconds, interval: frequencySeconds, frequency: frequencySeconds, // StartTimeProvider reads this deprecated field }), [durationSeconds, frequencySeconds]);
and add useMemo to the import on line 3.
— Claude Code
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
| initializePrometheus(prometheusUrl); | ||
| setHeaders({ Authorization: `Bearer ${token}` }); | ||
| } | ||
| }, [prometheusUrl, token]); |
There was a problem hiding this comment.
Race condition: initializePrometheus and setHeaders run in useEffect (after paint), but PlatformGlobalHealthBar fires its useQuery during the same render — before the effect executes. Since prometheusApiClient is still null, queryPrometheusRange returns undefined, React Query treats it as a successful response with no data, and the health bar stays empty until refetchInterval fires (60 s).
Fix by tracking initialization state so the child only renders once Prometheus is ready, and gate the render: if (!token || !isInitialized) return null;
— Claude Code
| label: '', | ||
| duration: durationSeconds, | ||
| interval: frequencySeconds, | ||
| frequency: frequencySeconds, // StartTimeProvider reads this deprecated field |
There was a problem hiding this comment.
Stale comment: StartTimeProvider was changed in this PR to read interval instead of frequency (see StartTimeProvider.tsx line 25). This field is no longer read by StartTimeProvider. Consider updating the comment to reflect the actual reason this field is kept (e.g., backward compatibility with the MetricsTimeSpanContext type).
— Claude Code
Review by Claude Code |
use iterval in StartTimeProvider instead of deprecated frequency
JeanMarcMilletScality
force-pushed
the
improvement/MK8S-219-share-health-bar
branch
from
0a9a831 to
3fc94ab
Compare
Review by Claude Code |
|
LGTM |
| useLayoutEffect(() => { | ||
| if (token) { | ||
| initializePrometheus(prometheusUrl); | ||
| setHeaders({ Authorization: `Bearer ${token}` }); | ||
| } | ||
| }, [prometheusUrl, token]); |
There was a problem hiding this comment.
- Since this component displays null when no token is available we should probably make token a property of the component and do conditional rendering, that would allow us to dodge some ifs.
- Why is it a useLayoutEffect and not a useEffect ?
- I don't see initializePrometheus defined anywhere in the code, there is function that looks like it with a different name tho.
- From where will prometheusUrl come from ? Does it need to initialise the api so deep into the code ? This part can probably be bubled up or we will forget it and reinitialise it at different places causing more redraws.
There was a problem hiding this comment.
initializePrometheus is an alias for initialize from prometheus api:
import { initialize as initializePrometheus, setHeaders } from '../services/prometheus/api';
The component will be used in artesca, if the prometheus is not initialized the query will throw an error
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: The following reviewers are expecting changes from the author, or must review again: |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
| @@ -0,0 +1,66 @@ | |||
| import { MetricsTimeSpanContext } from '@scality/core-ui/dist/components/charts/MetricsTimeSpanProvider'; | |||
| import { useShellHooks } from '@scality/module-federation'; | |||
| import { useEffect, useLayoutEffect, useMemo } from 'react'; | |||
There was a problem hiding this comment.
Unused import. useLayoutEffect is imported but never used.
```suggestion
import { useEffect, useMemo } from 'react';
| [durationSeconds, frequencySeconds], | ||
| ); | ||
|
|
||
| if (!token) return null; |
There was a problem hiding this comment.
Race condition: useEffect runs after paint, so on the first render where token is available, PlatformGlobalHealthBar mounts and useQuery fires before initializePrometheus executes. queryPrometheusRange finds prometheusApiClient still null and silently returns undefined.
Consider adding an initialization guard — e.g. an isInitialized state set to true inside the effect, and gate rendering on !token || !isInitialized.
(The unused useLayoutEffect import on line 3 suggests this was partially considered.)
— Claude Code
|
/approve |
Review by Claude Code |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: The following options are set: approve |
| <MetricsTimeSpanContext.Provider value={timeSpan}> | ||
| <StartTimeProvider> | ||
| <FederatedIntlProvider> | ||
| <PlatformGlobalHealthBar title={title} /> |
There was a problem hiding this comment.
PlatformGlobalHealthBar calls useHighestSeverityAlerts() and useAlertLibrary(), which both rely on useShellAlerts(). The other federated component (AlertNavbarUpdaterComponent) explicitly wraps its children with AlertProvider to ensure alert hooks work. This component omits it — is the host app guaranteed to have AlertsProvider in its tree above the mount point for this federated component? If not, these hooks may fail at runtime.
— Claude Code
|
|
||
| /** | ||
| * Initialize the Prometheus client and set the authorization header if the token is available | ||
| * The initialization of Prometheus client is neeeded here as it is shared with Module Federation to another ui |
There was a problem hiding this comment.
Typo: "neeeded" should be "needed".
— Claude Code
| const HealthBarContainer = styled.div` | ||
| display: flex; | ||
| flex-direction: column; | ||
| `; |
There was a problem hiding this comment.
The original HealthBarContainer had width: 90%; margin: 0 auto; which is now removed. This changes the dashboard layout — the health bar will render at full parent width instead of centered at 90%. Intentional?
— Claude Code
Review by Claude Code |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
/status |
StatusStatus report is not available. The following options are set: approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: The following options are set: approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue MK8S-219. Goodbye jeanmarcmilletscality. |
4 participants
Context
The new overview page in artesca has a Platform status section showing the global health bar
Summary
Extract the global health bar from the dashboard and move it in its own component
Use the new component in dashboard
Wrap the new component with its providers before sharing it via module federation