Skip to content

EDM-4049: Do now show sync status of RS without repositories #661

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.

Sign up for GitHub

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

Open
celdrake wants to merge 1 commit into
base: main
Choose a base branch
from
Open

EDM-4049: Do now show sync status of RS without repositories #661

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions libs/ui-components/src/components/ResourceSync/ResourceSyncImportStatus.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';
import { Alert, AlertActionCloseButton, PageSection, Stack, StackItem } from '@patternfly/react-core';
import { Trans } from 'react-i18next';

import { ConditionType, ResourceSync, ResourceSyncList } from '@flightctl/types';
import { ConditionType, RepositoryList, ResourceSync, ResourceSyncList } from '@flightctl/types';

import { useTranslation } from '../../hooks/useTranslation';
import { useFetchPeriodically } from '../../hooks/useFetchPeriodically';
Expand Down Expand Up @@ -147,6 +147,9 @@ const getVisibleResourceSyncs = (rsList: ResourceSync[]) => {
return { pendingRs, errorRs };
};

const removeRsWithMissingRepositories = (rsList: ResourceSync[], repoList: RepositoryList) =>
rsList.filter((rs) => repoList.items.some((repo) => repo.metadata.name === rs.spec.repository));

const ResourceSyncImport = ({ type }: { type: 'fleet' | 'catalog' }) => {
const params = new URLSearchParams();
params.set('fieldSelector', type === 'fleet' ? 'spec.type!=catalog' : 'spec.type==catalog');
Expand All @@ -155,7 +158,26 @@ const ResourceSyncImport = ({ type }: { type: 'fleet' | 'catalog' }) => {
});

// TODO Remove the client-side filtering once the API filter is available
const { pendingRs, errorRs } = React.useMemo(() => getVisibleResourceSyncs(rsList?.items || []), [rsList]);
const { pendingRs: allPendingRs, errorRs: allErrorRs } = React.useMemo(
() => getVisibleResourceSyncs(rsList?.items || []),
[rsList],
);

const shouldFetchRepositories = allPendingRs.length > 0 || allErrorRs.length > 0;
const [repoList, isLoadingRepos] = useFetchPeriodically<RepositoryList>({
endpoint: shouldFetchRepositories ? 'repositories' : '',
});

const { pendingRs, errorRs } = React.useMemo(() => {
if (isLoadingRepos || !repoList) {
return { pendingRs: [], errorRs: [] };
}
// If the repository no longer exists, the RS will eventually error, but it's not useful to show it to the user
return {
pendingRs: removeRsWithMissingRepositories(allPendingRs, repoList),
errorRs: removeRsWithMissingRepositories(allErrorRs, repoList),
};
}, [allPendingRs, allErrorRs, isLoadingRepos, repoList]);
Comment on lines +167 to +180

@coderabbitai coderabbitai Bot Jun 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't hide all ResourceSync alerts when the repository query fails.

This branch treats repoList === undefined as "show nothing". With useFetchPeriodically, an initial repositories fetch error also leaves repoList undefined, so a transient network failure or missing repository-list permission suppresses every pending/error alert, not just ones pointing at deleted repositories.

Suggested fallback 
-  const [repoList, isLoadingRepos] = useFetchPeriodically<RepositoryList>({
+  const [repoList, isLoadingRepos, repoListError] = useFetchPeriodically<RepositoryList>({
     endpoint: shouldFetchRepositories ? 'repositories' : '',
   });

   const { pendingRs, errorRs } = React.useMemo(() => {
-    if (isLoadingRepos || !repoList) {
+    if (isLoadingRepos && !repoList) {
       return { pendingRs: [], errorRs: [] };
     }
+    if (repoListError || !repoList) {
+      return { pendingRs: allPendingRs, errorRs: allErrorRs };
+    }
     // If the repository no longer exists, the RS will eventually error, but it's not useful to show it to the user
     return {
       pendingRs: removeRsWithMissingRepositories(allPendingRs, repoList),
       errorRs: removeRsWithMissingRepositories(allErrorRs, repoList),
     };
-  }, [allPendingRs, allErrorRs, isLoadingRepos, repoList]);
+  }, [allPendingRs, allErrorRs, isLoadingRepos, repoList, repoListError]);
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@libs/ui-components/src/components/ResourceSync/ResourceSyncImportStatus.tsx`
around lines 167 - 180, The current memo treats repoList being undefined as
“hide all alerts”; change the logic so you only filter out RS entries when you
actually have a valid repoList—otherwise preserve the original lists. In the
React.useMemo for pendingRs/errorRs (referencing repoList, isLoadingRepos,
allPendingRs, allErrorRs, removeRsWithMissingRepositories), return { pendingRs:
allPendingRs, errorRs: allErrorRs } when repoList is undefined or loading, and
only call removeRsWithMissingRepositories when repoList is present
(non-undefined) to avoid suppressing alerts on transient fetch failures.


if (pendingRs.length === 0 && errorRs.length === 0) {
return null;
Expand Down
Loading