From ba3a90d8fd8913d95cca522f732696a89b3b3d37 Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Fri, 14 Feb 2025 11:38:38 +0100 Subject: [PATCH] Alerting: Fix loading states (#100641) --- .../features/alerting/unified/RuleViewer.tsx | 4 +-- .../alerting/unified/hooks/useCombinedRule.ts | 36 ++++++++++++------- .../unified/hooks/useIsRuleEditable.ts | 33 +++++++++++------ .../rule-editor/ExistingRuleEditor.tsx | 17 +++++---- 4 files changed, 59 insertions(+), 31 deletions(-) diff --git a/public/app/features/alerting/unified/RuleViewer.tsx b/public/app/features/alerting/unified/RuleViewer.tsx index 9df84a41b18..43c38b699d0 100644 --- a/public/app/features/alerting/unified/RuleViewer.tsx +++ b/public/app/features/alerting/unified/RuleViewer.tsx @@ -36,7 +36,7 @@ const RuleViewer = (): JSX.Element => { }, [id]); // we then fetch the rule from the correct API endpoint(s) - const { loading, error, result: rule } = useCombinedRule({ ruleIdentifier: identifier, limitAlerts }); + const { loading, error, result: rule, uninitialized } = useCombinedRule({ ruleIdentifier: identifier, limitAlerts }); if (error) { return ( @@ -46,7 +46,7 @@ const RuleViewer = (): JSX.Element => { ); } - if (loading) { + if (loading || uninitialized) { return ( <> diff --git a/public/app/features/alerting/unified/hooks/useCombinedRule.ts b/public/app/features/alerting/unified/hooks/useCombinedRule.ts index 0841b794fb5..5d45c65137c 100644 --- a/public/app/features/alerting/unified/hooks/useCombinedRule.ts +++ b/public/app/features/alerting/unified/hooks/useCombinedRule.ts @@ -80,6 +80,7 @@ interface RequestState { result?: T; loading: boolean; error?: unknown; + uninitialized: boolean; } interface Props { @@ -99,6 +100,7 @@ export function useCombinedRule({ ruleIdentifier, limitAlerts }: Props): Request loading: isLoadingRuleLocation, error: ruleLocationError, result: ruleLocation, + uninitialized, } = useRuleLocation(ruleIdentifier); const { @@ -125,7 +127,12 @@ export function useCombinedRule({ ruleIdentifier, limitAlerts }: Props): Request const [ fetchRulerRuleGroup, - { currentData: rulerRuleGroup, isLoading: isLoadingRulerGroup, error: rulerRuleGroupError }, + { + currentData: rulerRuleGroup, + isLoading: isLoadingRulerGroup, + error: rulerRuleGroupError, + isUninitialized: ruleGroupUninitialized, + }, ] = alertRuleApi.endpoints.getRuleGroupForNamespace.useLazyQuery(); useEffect(() => { @@ -158,9 +165,10 @@ export function useCombinedRule({ ruleIdentifier, limitAlerts }: Props): Request }, [ruleIdentifier, ruleSourceName, promRuleNs, rulerRuleGroup, ruleSource, ruleLocation, namespaceName]); return { - loading: isLoadingDsFeatures || isLoadingPromRules || isLoadingRulerGroup, + loading: isLoadingDsFeatures || isLoadingPromRules || isLoadingRulerGroup || ruleGroupUninitialized, error: ruleLocationError ?? promRuleNsError ?? rulerRuleGroupError, result: rule, + uninitialized, }; } @@ -187,17 +195,19 @@ export function useRuleLocation(ruleIdentifier: RuleIdentifier): RequestState { @@ -297,9 +306,10 @@ export function useRuleWithLocation({ }, [ruleIdentifier, rulerRuleGroup, ruleSource, ruleLocation]); return { - loading: isLoadingRuleLocation || isLoadingDsFeatures || isLoadingRulerGroup || isUninitializedRulerGroup, + loading: isLoadingRuleLocation || isLoadingDsFeatures || isLoadingRulerGroup, error: ruleLocationError ?? rulerRuleGroupError, result: ruleWithLocation, + uninitialized, }; } diff --git a/public/app/features/alerting/unified/hooks/useIsRuleEditable.ts b/public/app/features/alerting/unified/hooks/useIsRuleEditable.ts index 50ca91b8188..bc5d8a182a2 100644 --- a/public/app/features/alerting/unified/hooks/useIsRuleEditable.ts +++ b/public/app/features/alerting/unified/hooks/useIsRuleEditable.ts @@ -16,19 +16,22 @@ interface ResultBag { } export function useIsRuleEditable(rulesSourceName: string, rule?: RulerRuleDTO): ResultBag { - const { currentData: dsFeatures, isLoading } = featureDiscoveryApi.endpoints.discoverDsFeatures.useQuery({ - uid: getDatasourceAPIUid(rulesSourceName), - }); + const { currentData: dsFeatures, isLoading: loadingDataSourceFeatures } = + featureDiscoveryApi.endpoints.discoverDsFeatures.useQuery({ + uid: getDatasourceAPIUid(rulesSourceName), + }); const folderUID = rule && isGrafanaRulerRule(rule) ? rule.grafana_alert.namespace_uid : undefined; - const rulePermission = getRulesPermissions(rulesSourceName); - const { folder, loading } = useFolder(folderUID); + + const { folder, loading: loadingFolder } = useFolder(folderUID); if (!rule) { return { isEditable: false, isRemovable: false, loading: false }; } + const loading = loadingFolder || loadingDataSourceFeatures; + // Grafana rules can be edited if user can edit the folder they're in // When RBAC is disabled access to a folder is the only requirement for managing rules // When RBAC is enabled the appropriate alerting permissions need to be met @@ -39,13 +42,23 @@ export function useIsRuleEditable(rulesSourceName: string, rule?: RulerRuleDTO): ); } - if (!folder) { - // Loading or invalid folder UID + // loading folder information + if (loadingFolder) { return { isRulerAvailable: true, isEditable: false, isRemovable: false, - loading, + loading: true, + }; + } + + // invalid folder UID + if (!folder) { + return { + isRulerAvailable: true, + isEditable: false, + isRemovable: false, + loading: false, }; } @@ -56,7 +69,7 @@ export function useIsRuleEditable(rulesSourceName: string, rule?: RulerRuleDTO): isRulerAvailable: true, isEditable: canEditGrafanaRules, isRemovable: canRemoveGrafanaRules, - loading: loading || isLoading, + loading: loading, }; } @@ -69,6 +82,6 @@ export function useIsRuleEditable(rulesSourceName: string, rule?: RulerRuleDTO): isRulerAvailable, isEditable: canEditCloudRules && isRulerAvailable, isRemovable: canRemoveCloudRules && isRulerAvailable, - loading: isLoading, + loading: loading, }; } diff --git a/public/app/features/alerting/unified/rule-editor/ExistingRuleEditor.tsx b/public/app/features/alerting/unified/rule-editor/ExistingRuleEditor.tsx index 7d1b9d92fcf..8bc21533774 100644 --- a/public/app/features/alerting/unified/rule-editor/ExistingRuleEditor.tsx +++ b/public/app/features/alerting/unified/rule-editor/ExistingRuleEditor.tsx @@ -1,4 +1,5 @@ import { Alert, LoadingPlaceholder } from '@grafana/ui'; +import { EntityNotFound } from 'app/core/components/PageNotFound/EntityNotFound'; import { RuleIdentifier } from 'app/types/unified-alerting'; import { AlertWarning } from '../AlertWarning'; @@ -13,17 +14,21 @@ interface ExistingRuleEditorProps { } export function ExistingRuleEditor({ identifier }: ExistingRuleEditorProps) { + const ruleSourceName = ruleId.ruleIdentifierToRuleSourceName(identifier); + const { loading: loadingAlertRule, result: ruleWithLocation, error, + uninitialized, } = useRuleWithLocation({ ruleIdentifier: identifier }); - const ruleSourceName = ruleId.ruleIdentifierToRuleSourceName(identifier); - const { isEditable, loading: loadingEditable } = useIsRuleEditable(ruleSourceName, ruleWithLocation?.rule); - const loading = loadingAlertRule || loadingEditable; + // the loading of the editable state only happens once we've got a rule with location loaded, so we set it to true by default here + const loadingEditableState = Boolean(ruleWithLocation) ? loadingEditable : true; + const loading = loadingAlertRule || loadingEditableState || uninitialized; + const ruleNotFound = !Boolean(ruleWithLocation); if (loading) { return ; @@ -37,11 +42,11 @@ export function ExistingRuleEditor({ identifier }: ExistingRuleEditorProps) { ); } - if (!ruleWithLocation) { - return Sorry! This rule does not exist.; + if (ruleNotFound) { + return ; } - if (isEditable === false) { + if (isEditable === false && !loadingEditable) { return Sorry! You do not have permission to edit this rule.; }