diff --git a/ui/app/(prowler)/_overview/graphs-tabs/findings-view/findings-view.ssr.tsx b/ui/app/(prowler)/_overview/graphs-tabs/findings-view/findings-view.ssr.tsx index 2d19a33b47..9a7a184a42 100644 --- a/ui/app/(prowler)/_overview/graphs-tabs/findings-view/findings-view.ssr.tsx +++ b/ui/app/(prowler)/_overview/graphs-tabs/findings-view/findings-view.ssr.tsx @@ -45,7 +45,8 @@ export async function FindingsViewSSR({ searchParams }: FindingsViewSSRProps) { const scan = scanDict[finding.relationships?.scan?.data?.id]; const resource = resourceDict[finding.relationships?.resources?.data?.[0]?.id]; - const provider = providerDict[scan?.relationships?.provider?.data?.id]; + const provider = + providerDict[scan?.relationships?.provider?.data?.id ?? ""]; return { ...finding, diff --git a/ui/components/compliance/compliance-accordion/client-accordion-content.test.ts b/ui/components/compliance/compliance-accordion/client-accordion-content.test.ts index 06f417a4db..a036fc0aa8 100644 --- a/ui/components/compliance/compliance-accordion/client-accordion-content.test.ts +++ b/ui/components/compliance/compliance-accordion/client-accordion-content.test.ts @@ -31,4 +31,12 @@ describe("client accordion content", () => { expect(source).not.toContain("useEffect"); expect(source).not.toContain("useRef"); }); + + it("gates the skeleton on the hook loading state and surfaces fetch errors", () => { + // A disabled fetch (e.g. "No findings" status) must not skeleton forever, + // and a failed fetch must offer a retry instead of hanging. + expect(source).toContain("isLoading && requirement.status"); + expect(source).not.toContain("findings === null"); + expect(source).toContain("Try again"); + }); }); diff --git a/ui/components/compliance/compliance-accordion/client-accordion-content.tsx b/ui/components/compliance/compliance-accordion/client-accordion-content.tsx index ef27301936..1ad22792f4 100644 --- a/ui/components/compliance/compliance-accordion/client-accordion-content.tsx +++ b/ui/components/compliance/compliance-accordion/client-accordion-content.tsx @@ -11,7 +11,7 @@ import { getStandaloneFindingColumns, SkeletonTableFindings, } from "@/components/findings/table"; -import { Alert, AlertDescription } from "@/components/shadcn"; +import { Alert, AlertDescription, Button } from "@/components/shadcn"; import { Accordion } from "@/components/ui/accordion/Accordion"; import { DataTable } from "@/components/ui/table"; import { FINDINGS_DEFAULT_SORT, MUTED_FILTER } from "@/lib"; @@ -50,20 +50,26 @@ export const ClientAccordionContent = ({ const checks = requirement.check_ids || []; - const { findings, expandedFindings, patchTriageUpdate, reload } = - useRequirementFindings({ - enabled: - !disableFindings && - checks.length > 0 && - requirement.status !== "No findings", - checkIds: checks, - scanId, - pageNumber, - pageSize, - sort, - region, - mutedFilter, - }); + const { + findings, + expandedFindings, + isLoading, + error, + patchTriageUpdate, + reload, + } = useRequirementFindings({ + enabled: + !disableFindings && + checks.length > 0 && + requirement.status !== "No findings", + checkIds: checks, + scanId, + pageNumber, + pageSize, + sort, + region, + mutedFilter, + }); const handleTriageUpdate = async (input: UpdateFindingTriageInput) => { await updateFindingTriage(input); @@ -125,7 +131,26 @@ export const ClientAccordionContent = ({ ]; const renderFindingsTable = () => { - if (findings === null && requirement.status !== "MANUAL") { + if (error) { + return ( + + + + {error} + + + + ); + } + + if (isLoading && requirement.status !== "MANUAL") { return ; } diff --git a/ui/components/compliance/compliance-accordion/use-requirement-findings.test.ts b/ui/components/compliance/compliance-accordion/use-requirement-findings.test.ts index 5e6ffb935b..87067fd020 100644 --- a/ui/components/compliance/compliance-accordion/use-requirement-findings.test.ts +++ b/ui/components/compliance/compliance-accordion/use-requirement-findings.test.ts @@ -122,6 +122,97 @@ describe("useRequirementFindings", () => { expect(findingsActionsMock.getFindings).not.toHaveBeenCalled(); }); + it("should not report loading when the fetch is disabled", async () => { + // Given / When + const disabled = renderHook(() => + useRequirementFindings(defaultOptions({ enabled: false })), + ); + const withoutChecks = renderHook(() => + useRequirementFindings(defaultOptions({ checkIds: [] })), + ); + await flushAsync(); + + // Then — a skipped fetch must not look like a pending one. + expect(disabled.result.current.isLoading).toBe(false); + expect(withoutChecks.result.current.isLoading).toBe(false); + }); + + it("should report loading until the fetch settles", async () => { + // Given + let resolveFetch: (value: unknown) => void = () => {}; + findingsActionsMock.getFindings.mockImplementationOnce( + () => new Promise((resolve) => (resolveFetch = resolve)), + ); + + // When + const { result } = renderHook(() => + useRequirementFindings(defaultOptions()), + ); + + // Then + expect(result.current.isLoading).toBe(true); + + // When + act(() => { + resolveFetch(makeFindingsResponse()); + }); + await flushAsync(); + + // Then + expect(result.current.isLoading).toBe(false); + }); + + it("should expose an error and stop loading when the fetch fails", async () => { + // Given + const consoleErrorSpy = vi + .spyOn(console, "error") + .mockImplementation(() => {}); + findingsActionsMock.getFindings.mockRejectedValue( + new Error("network down"), + ); + + // When + const { result } = renderHook(() => + useRequirementFindings(defaultOptions()), + ); + await flushAsync(); + + // Then — the caller can render an error state instead of a skeleton. + expect(result.current.error).toBe("Could not load findings."); + expect(result.current.isLoading).toBe(false); + expect(result.current.findings).toBeNull(); + + consoleErrorSpy.mockRestore(); + }); + + it("should clear the error and recover on reload", async () => { + // Given: first fetch fails, retry succeeds + const consoleErrorSpy = vi + .spyOn(console, "error") + .mockImplementation(() => {}); + findingsActionsMock.getFindings + .mockRejectedValueOnce(new Error("network down")) + .mockResolvedValueOnce(makeFindingsResponse()); + + const { result } = renderHook(() => + useRequirementFindings(defaultOptions()), + ); + await flushAsync(); + expect(result.current.error).toBe("Could not load findings."); + + // When + act(() => { + result.current.reload(); + }); + await flushAsync(); + + // Then + expect(result.current.error).toBeNull(); + expect(result.current.findings).not.toBeNull(); + + consoleErrorSpy.mockRestore(); + }); + it("should not refetch when only the checkIds array identity changes", async () => { // Given const { rerender } = renderHook((props) => useRequirementFindings(props), { diff --git a/ui/components/compliance/compliance-accordion/use-requirement-findings.ts b/ui/components/compliance/compliance-accordion/use-requirement-findings.ts index 308566f4a6..596069e877 100644 --- a/ui/components/compliance/compliance-accordion/use-requirement-findings.ts +++ b/ui/components/compliance/compliance-accordion/use-requirement-findings.ts @@ -22,10 +22,14 @@ interface UseRequirementFindingsOptions { interface UseRequirementFindingsReturn { findings: FindingsResponse | null; expandedFindings: FindingProps[]; + isLoading: boolean; + error: string | null; patchTriageUpdate: (input: UpdateFindingTriageInput) => void; reload: () => void; } +const FINDINGS_LOAD_ERROR = "Could not load findings."; + export function useRequirementFindings({ enabled, checkIds, @@ -38,20 +42,26 @@ export function useRequirementFindings({ }: UseRequirementFindingsOptions): UseRequirementFindingsReturn { const [findings, setFindings] = useState(null); const [expandedFindings, setExpandedFindings] = useState([]); + const [error, setError] = useState(null); const [reloadNonce, setReloadNonce] = useState(0); // Depend on the joined value, not the array: the requirement prop gets a // fresh identity on every parent render and must not retrigger the fetch. const checkIdsKey = checkIds.join(","); + const isFetchEnabled = enabled && checkIdsKey.length > 0; + // A skipped fetch is not a pending one; without this the caller would show + // a skeleton forever for requirements whose fetch never runs. + const isLoading = isFetchEnabled && findings === null && error === null; useEffect(() => { - if (!enabled || !checkIdsKey) { + if (!isFetchEnabled) { return; } let cancelled = false; const loadFindings = async () => { + setError(null); try { const findingsData = await getFindings({ filters: { @@ -80,7 +90,7 @@ export function useRequirementFindings({ const resource = resourceDict[finding.relationships?.resources?.data?.[0]?.id]; const provider = - providerDict[scan?.relationships?.provider?.data?.id]; + providerDict[scan?.relationships?.provider?.data?.id ?? ""]; return { ...finding, @@ -93,6 +103,7 @@ export function useRequirementFindings({ } catch (error) { if (!cancelled) { console.error("Error loading findings:", error); + setError(FINDINGS_LOAD_ERROR); } } }; @@ -103,7 +114,7 @@ export function useRequirementFindings({ cancelled = true; }; }, [ - enabled, + isFetchEnabled, checkIdsKey, scanId, pageNumber, @@ -124,5 +135,12 @@ export function useRequirementFindings({ setReloadNonce((value) => value + 1); }; - return { findings, expandedFindings, patchTriageUpdate, reload }; + return { + findings, + expandedFindings, + isLoading, + error, + patchTriageUpdate, + reload, + }; } diff --git a/ui/components/findings/table/finding-triage-cells.test.tsx b/ui/components/findings/table/finding-triage-cells.test.tsx index 3d0a417860..74fd48764c 100644 --- a/ui/components/findings/table/finding-triage-cells.test.tsx +++ b/ui/components/findings/table/finding-triage-cells.test.tsx @@ -373,7 +373,7 @@ describe("finding triage cells", () => { screen.getByRole("dialog", { name: "Add Triage Note" }), ).toBeVisible(); expect(screen.getByLabelText("Note text")).toBeDisabled(); - expect(screen.getByRole("button", { name: "Save changes" })).toBeDisabled(); + expect(screen.getByRole("button", { name: "Save" })).toBeDisabled(); expect( screen.getByRole("link", { name: "Available in Prowler Cloud" }), ).toHaveAttribute("href", "https://prowler.com/pricing"); diff --git a/ui/lib/finding-detail.ts b/ui/lib/finding-detail.ts index 5566b9f994..73bf2e33aa 100644 --- a/ui/lib/finding-detail.ts +++ b/ui/lib/finding-detail.ts @@ -23,12 +23,12 @@ export function expandFindingWithRelationships( const scan = scanDict[finding.relationships?.scan?.data?.id]; const resource = resourceDict[finding.relationships?.resources?.data?.[0]?.id]; - const provider = providerDict[scan?.relationships?.provider?.data?.id]; + const provider = providerDict[scan?.relationships?.provider?.data?.id ?? ""]; return { ...finding, relationships: { ...finding.relationships, scan, resource, provider }, - } as FindingProps; + } as unknown as FindingProps; } export function findingToFindingResourceRow( diff --git a/ui/lib/finding-triage.test.ts b/ui/lib/finding-triage.test.ts index ee351fe260..a08eb29804 100644 --- a/ui/lib/finding-triage.test.ts +++ b/ui/lib/finding-triage.test.ts @@ -126,6 +126,82 @@ describe("finding triage optimistic row updates", () => { expect(result[1]).toBe(otherFinding); }); + it("should preserve muted attributes when leaving a mutelist-shortcut status", () => { + // Given: a finding muted by a previous shortcut transition. The server + // never removes the mute rule when the status moves on, so the optimistic + // update must not unmute the row either. + const finding = makeFindingRow({ + triage: makeTriageSummary({ + status: FINDING_TRIAGE_STATUS.RISK_ACCEPTED, + label: "Risk Accepted", + isMuted: true, + }), + attributes: { + muted: true, + muted_reason: "Finding triage status changed to Risk Accepted.", + status: "FAIL", + }, + }); + + // When + const result = applyOptimisticFindingTriageRowUpdate(finding, { + findingId: "finding-1", + findingUid: "uid-1", + triageId: "triage-1", + notesCount: 0, + status: FINDING_TRIAGE_STATUS.REMEDIATING, + previousStatus: FINDING_TRIAGE_STATUS.RISK_ACCEPTED, + isMuted: true, + }); + + // Then + expect(result.triage).toEqual( + expect.objectContaining({ + status: FINDING_TRIAGE_STATUS.REMEDIATING, + label: "Remediating", + isMuted: true, + }), + ); + expect(result.attributes).toEqual( + expect.objectContaining({ + muted: true, + muted_reason: "Finding triage status changed to Risk Accepted.", + }), + ); + }); + + it("should not overwrite muted_reason when an already muted finding enters a shortcut status", () => { + // Given: muted through some other channel (e.g. a mutelist rule). + const finding = makeFindingRow({ + triage: makeTriageSummary({ isMuted: true }), + attributes: { + muted: true, + muted_reason: "Muted by mutelist rule.", + status: "FAIL", + }, + }); + + // When: no new mute rule is created for already muted findings, so the + // optimistic reason must keep the original one. + const result = applyOptimisticFindingTriageRowUpdate(finding, { + findingId: "finding-1", + findingUid: "uid-1", + triageId: "triage-1", + notesCount: 0, + status: FINDING_TRIAGE_STATUS.RISK_ACCEPTED, + previousStatus: FINDING_TRIAGE_STATUS.UNDER_REVIEW, + isMuted: true, + }); + + // Then + expect(result.attributes).toEqual( + expect.objectContaining({ + muted: true, + muted_reason: "Muted by mutelist rule.", + }), + ); + }); + it("should leave rows without triage unchanged", () => { // Given const finding = makeFindingRow({ triage: undefined }); diff --git a/ui/lib/utils.ts b/ui/lib/utils.ts index 36d7c080c9..7a37948ac7 100644 --- a/ui/lib/utils.ts +++ b/ui/lib/utils.ts @@ -29,10 +29,31 @@ export function getOptionalText(value: unknown): string | undefined { : undefined; } -// Helper function to create dictionaries by type -export function createDict(type: string, data: any) { +interface IncludedApiItemRelationshipRef { + id: string; +} + +interface IncludedApiItemRelationship { + data?: IncludedApiItemRelationshipRef; +} + +interface IncludedApiItem { + id: string; + type: string; + attributes?: Record; + relationships?: Record; +} + +// Indexes a JSON:API `included` array by id for the requested resource type. +export function createDict( + type: string, + data: { included?: unknown[] } | null | undefined, +): Record { const includedField = data?.included?.filter( - (item: { type: string }) => item.type === type, + (item): item is T => + typeof item === "object" && + item !== null && + (item as IncludedApiItem).type === type, ); if (!includedField || includedField.length === 0) { @@ -40,6 +61,6 @@ export function createDict(type: string, data: any) { } return Object.fromEntries( - includedField.map((item: { id: string }) => [item.id, item]), + includedField.map((item): [string, T] => [item.id, item]), ); }