EDM-3715 vulnerability management#636
Open
celdrake wants to merge 2 commits intoflightctl:mainfrom
Open
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Summary by CodeRabbit
WalkthroughIntroduces vulnerabilities: adds types and semver, API routing/handlers, hooks for lists/summary/impact, security overview UI with tables/drawers, device/fleet integration and filters, routing (standalone/OCP), table typing refactor, i18n strings, drawer abstraction, and minor date/status updates. ChangesSecurity overview and vulnerabilities
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes |
celdrake
force-pushed
the
EDM-3715-vulnerability-management
branch
from
95d6323 to
026d26c
Compare
celdrake
commented
May 7, 2026
| font-weight: var(--pf-t--global--font--weight--body--bold); | ||
| } | ||
|
|
||
| .fctl-security-overview-summary-box.critical { |
Collaborator
Author
There was a problem hiding this comment.
The "severity" Patternfly tokens are not really integrated with other elements.
celdrake
force-pushed
the
EDM-3715-vulnerability-management
branch
3 times, most recently
from
47e8455 to
4597c67
Compare
celdrake
force-pushed
the
EDM-3715-vulnerability-management
branch
from
4597c67 to
26b4900
Compare
celdrake
force-pushed
the
EDM-3715-vulnerability-management
branch
from
26b4900 to
272ed24
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
libs/ui-components/src/types/rbac.ts (1)
16-17: Confirm RBAC resource path before release.The "CELIA WIP" comment indicates the
vulnerabilitiesresource path needs backend RBAC confirmation. This coordination task should be tracked to ensure RBAC alignment before release.Would you like me to create a tracking issue for confirming the
vulnerabilitiesresource path with the backend RBAC implementation?🤖 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/types/rbac.ts` around lines 16 - 17, The WIP comment indicates the RBAC resource path for the enum member VULNERABILITY = 'vulnerabilities' needs backend confirmation; create a tracking issue in the repo/board to confirm the backend RBAC resource path, then replace the inline "CELIA-WIP" comment with a concise TODO that references the new issue ID (or link) and notes to verify/align the 'vulnerabilities' resource string with backend RBAC before release so the enum member (VULNERABILITY) can be finalized.libs/ui-components/src/hooks/useServicesEnabled.ts (1)
38-46: ⚡ Quick winDocument the error format contract to avoid silent breakage.
The error handling assumes
error.messageis a numeric string for status codes 500, 501, and 401—an implicit contract created by the apiCalls handlers in both apps/ocp-plugin and apps/standalone. This tight coupling works today because these specific status codes are intentionally thrown as pure numeric strings (e.g.,throw new Error(\${response.status}`)) with a comment "Return the status code to correctly detect that alerts are disabled." However, this contract is undocumented inuseServicesEnabled.ts`.If the error format changes or other error paths are used (e.g., formatted messages like
"Error 500: Internal Server Error"),Number(errorMessage)silently returnsNaN, and the fallback toisEnabled = truemasks the issue. Adding explicit documentation or a helper function to validate and extract status codes would prevent future maintainers from inadvertently breaking this coupling.🤖 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/hooks/useServicesEnabled.ts` around lines 38 - 46, The error handling in useServicesEnabled assumes error.message contains a pure numeric status code; add explicit documentation and a small validator/helper to extract and validate numeric status codes instead of trusting Number(error.message). Update the block referencing disabledStatusCodes and setIsEnabled (and abortController.signal.aborted) to call the helper (e.g., parseStatusCodeFromError) which returns a number or null, and only compare against disabledStatusCodes when a valid number is returned; also add a short comment above useServicesEnabled explaining the expected error.message contract so future callers/handlers know to throw plain numeric status strings.libs/ui-components/src/components/Device/DevicesPage/DeviceNameOnlyToolbarFilter.tsx (1)
23-25: 💤 Low valueMinor: Redundant dependency in useEffect.
setTextFilteris already captured in the memoizeddebouncedSetNameOrAliascallback, so including it directly in this effect's dependency array is unnecessary. This doesn't cause bugs but adds slight noise.Suggested cleanup
React.useEffect(() => { debouncedSetNameOrAlias(typingText); - }, [typingText, setTextFilter, debouncedSetNameOrAlias]); + }, [typingText, debouncedSetNameOrAlias]);🤖 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/Device/DevicesPage/DeviceNameOnlyToolbarFilter.tsx` around lines 23 - 25, The useEffect callback currently lists an unnecessary dependency; remove setTextFilter from the dependency array of the React.useEffect that calls debouncedSetNameOrAlias so the effect depends only on typingText and debouncedSetNameOrAlias (debouncedSetNameOrAlias already closes over setTextFilter), e.g., update the React.useEffect containing debouncedSetNameOrAlias(typingText) to only include typingText and debouncedSetNameOrAlias.libs/ui-components/src/components/SecurityOverview/VulnerabilityImpact.tsx (2)
180-180: ⚡ Quick winInconsistent query parameter key: hardcoded
cveIdinstead ofFilterSearchParams.CveId.Similar to above, use the constant for consistency with the rest of the codebase.
♻️ Suggested fix
- <Link to={ROUTE.DEVICES} query={new URLSearchParams({ cveId }).toString()}> + <Link to={ROUTE.DEVICES} query={new URLSearchParams({ [FilterSearchParams.CveId]: cveId }).toString()}>🤖 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/SecurityOverview/VulnerabilityImpact.tsx` at line 180, The Link to ROUTE.DEVICES is building the query with a hardcoded key "cveId" — change it to use the shared constant FilterSearchParams.CveId for consistency; update the URLSearchParams construction in the Link (the element using ROUTE.DEVICES) to pass new URLSearchParams({ [FilterSearchParams.CveId]: cveId }).toString() so the query key matches the rest of the codebase.
253-253: ⚡ Quick winInconsistent query parameter naming:
fleetIdvsFilterSearchParams.Fleet.The link uses hardcoded
fleetIdin the query string, but elsewhereFilterSearchParams.Fleetis used (which equals'fleetId'). For consistency and maintainability, use the constant.♻️ Suggested fix
- <Link to={ROUTE.DEVICES} query={new URLSearchParams({ cveId, fleetId: fleetName }).toString()}> + <Link to={ROUTE.DEVICES} query={new URLSearchParams({ [FilterSearchParams.CveId]: cveId, [FilterSearchParams.Fleet]: fleetName }).toString()}>🤖 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/SecurityOverview/VulnerabilityImpact.tsx` at line 253, Link query uses hardcoded "fleetId" instead of the shared constant; update the Link in VulnerabilityImpact (the <Link to={ROUTE.DEVICES} ...> call) to build the query using FilterSearchParams.Fleet (which equals 'fleetId') instead of the literal string so naming is consistent and maintainable; replace new URLSearchParams({ cveId, fleetId: fleetName }).toString() with a construction that uses FilterSearchParams.Fleet as the key for fleetName.libs/ui-components/src/components/SecurityOverview/VulnerabilityDetailsDrawer.tsx (1)
77-85: 💤 Low valuePotential runtime error when accessing
findings[0]on an empty array.If
vulnerability.findingsis an empty array, accessingvulnerability.findings[0]will returnundefined, causingadvisoryIdanddescriptionto silently becomeundefined. This may be intentional, but if theVulnerabilityGrouptype guarantees at least one finding, consider adding a runtime guard or documenting this assumption.🤖 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/SecurityOverview/VulnerabilityDetailsDrawer.tsx` around lines 77 - 85, The branch in VulnerabilityDetailsDrawer.tsx that runs when isGrouped reads vulnerability.findings[0].advisoryId and .description without guarding for an empty findings array; add a runtime guard so advisoryId and description are only read when vulnerability.findings?.length > 0 (or fallback to a sensible default like vulnerability.advisoryId/description or an empty string) and ensure publishedAt still uses vulnerability.maxPublishedAt for grouped items; alternatively, assert/document that the VulnerabilityGroup type always contains at least one finding and add a runtime check that throws or logs if findings is empty to make the assumption explicit.libs/ui-components/src/components/Device/DevicesPage/DeviceToolbarFilters.tsx (1)
333-352: 💤 Low valueDebounced function may not cancel properly on dependency changes.
When
setTextFilterortchanges, a new debounced function is created, but the old one's pending invocation isn't cancelled. The cleanup in the useEffect (line 363) only cancels the current debounced function, not the previous one.This is unlikely to cause visible issues in practice since
setTextFilteris typically stable (from useCallback), but consider usinguseReffor the debounce function or ensuring the debounce is cancelled when dependencies change.🤖 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/Device/DevicesPage/DeviceToolbarFilters.tsx` around lines 333 - 352, The debouncedSetTextFilter created with debounce(...) can leave pending invocations when its dependencies (setTextFilter or t) change; replace the useMemo pattern with a ref-stored debounced function so you can cancel the previous instance when recreating: create a React.useRef for the debounced function (debouncedSetTextFilterRef), initialize or recreate it inside a useEffect that depends on [setTextFilter, t], and call debouncedSetTextFilterRef.current(...) where needed; ensure the effect cancels the previous debounced instance (call .cancel()) in its cleanup to avoid stray invocations and also cancel on unmount. Use the existing identifiers debouncedSetTextFilter (or replace with debouncedSetTextFilterRef), debounce, setTextFilter, and the CVE validation logic unchanged.libs/ui-components/src/components/Device/DevicesPage/useDevices.ts (1)
126-136: ⚡ Quick winRemove unused
nameOrAliasparameter fromuseDevicessignature.The
nameOrAliasparameter is not passed by any callers. The function already derives it fromtextFilters(line 44), making the parameter redundant.♻️ Suggested fix
export const useDevices = (args: { textFilters?: Partial<Record<DeviceTextFilterKey, string>>; - nameOrAlias?: string; ownerFleets?: string[]; activeStatuses?: FilterStatusMap; labels?: FlightCtlLabel[]; onlyDecommissioned: boolean; nextContinue?: string; onlyFleetless?: boolean; onPageFetched?: (data: DeviceList) => void; }): DevicesResult => {🤖 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/Device/DevicesPage/useDevices.ts` around lines 126 - 136, Remove the redundant nameOrAlias parameter from the useDevices function signature and related type for its args: delete the nameOrAlias?: string entry from the args object in useDevices and remove any code that references that parameter inside the function (the hook already reads the value from textFilters using DeviceTextFilterKey). Ensure the exported useDevices signature and any local references only rely on textFilters (and other existing keys), and run TypeScript to confirm no callers require nameOrAlias (update any tests or types that referenced the args shape if necessary).libs/ui-components/src/components/SecurityOverview/VulnerabilitiesTable.tsx (1)
4-5: ⚡ Quick winUse indexed-access typing with
import typefor maintainability.The current namespace-style access (
Vulnerability.severity) couples this component to the generated model's namespace structure. Switching to indexed-access typing with type-only imports removes this dependency and clarifies type-only intent.Proposed change
-import { Vulnerability, VulnerabilityGroup, VulnerabilityGroupList, VulnerabilityList } from '@flightctl/types/alpha'; +import type { Vulnerability, VulnerabilityGroup, VulnerabilityGroupList, VulnerabilityList } from '@flightctl/types/alpha'; -type VulnerabilitySeverity = Vulnerability.severity; +type VulnerabilitySeverity = NonNullable<Vulnerability['severity']>;🤖 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/SecurityOverview/VulnerabilitiesTable.tsx` around lines 4 - 5, Change the runtime namespace-style imports to type-only imports and use indexed-access types where the code references model members: replace the current import with an import type of Vulnerability, VulnerabilityGroup, VulnerabilityGroupList, VulnerabilityList from '@flightctl/types/alpha', and update any uses like Vulnerability.severity (or other property-access on the model types) to the indexed-access form Vulnerability['severity'] (and similarly for other properties) so the component depends only on types rather than the generated namespace at runtime.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@libs/ui-components/src/components/common/FlightCtlPageDrawer.tsx`:
- Around line 13-24: getPageContentTop() reads document and is invoked eagerly
by usePageContentTop's useState initializer, causing SSR crashes; change the
initialization so no DOM access happens during render (either make
getPageContentTop return a safe default when typeof document === 'undefined' or
initialize topOffset with a constant like 60 and then call getPageContentTop()
inside a useEffect to setTopOffset). Update usePageContentTop (and keep
getPageContentTop) so DOM querying only runs inside useEffect or when document
is present, referencing the existing getPageContentTop and setTopOffset symbols
to locate and apply the fix.
In `@libs/ui-components/src/components/SecurityOverview/SecurityOverviewPage.tsx`:
- Around line 72-75: The UI currently shows the "not enabled" Alert while
useVulnerabilitiesEnabled() is still resolving; update the render guard in
SecurityOverviewPage to check isLoading first (e.g., only show the Alert when
isLoading is false and vulnerabilitiesEnabled is false) by changing the
condition that references vulnerabilitiesEnabled to include isLoading (for
example: if (!isLoading && !vulnerabilitiesEnabled) ...) or otherwise render a
loading state while isLoading is true so the transient incorrect Alert cannot
appear.
- Around line 36-40: The empty-state branch currently returns
SecurityOverviewEmptyState when itemCount === 0 regardless of request errors,
which masks backend failures; update the condition to only return that empty
state when there is no error (add a !error guard) so keep the existing check
(itemCount === 0 && !hasFiltersEnabled && !isLoading && !isUpdating) but require
!error as well; locate the conditional around SecurityOverviewEmptyState in
SecurityOverviewPage.tsx and add the error check so ListPageBody can render
errors when present.
In `@libs/ui-components/src/hooks/useVulnerabilitySummary.ts`:
- Around line 13-47: The hook useVulnerabilitySummary currently swallows fetch
failures and always returns fallback counts; update
UseVulnerabilitySummaryResult to include an error?: Error | null field and
modify useVulnerabilitySummary to track an error state (e.g., const [error,
setError] = useState<Error | null>(null)), set error to null before loading,
setError(err) in the catch when the request fails (but do not set for aborts),
and return error alongside counts and isLoading so consumers can distinguish API
failures from legitimately empty results; ensure the abort handling still
prevents setting error after abort and preserve the existing isLoading
semantics.
In `@libs/ui-components/src/utils/dates.ts`:
- Around line 30-36: The getDateNoTimeDisplay function currently calls
dateFormatter().format(new Date(timestamp)) without validating the parsed date,
which throws a RangeError for malformed timestamps; update getDateNoTimeDisplay
to parse the timestamp into a Date object, check its validity (e.g., const d =
new Date(timestamp); if (isNaN(d.getTime())) return 'N/A';), and only then call
dateFormatter().format(d) so invalid or missing timestamps both return 'N/A'
(references: getDateNoTimeDisplay and dateFormatter).
---
Nitpick comments:
In
`@libs/ui-components/src/components/Device/DevicesPage/DeviceNameOnlyToolbarFilter.tsx`:
- Around line 23-25: The useEffect callback currently lists an unnecessary
dependency; remove setTextFilter from the dependency array of the
React.useEffect that calls debouncedSetNameOrAlias so the effect depends only on
typingText and debouncedSetNameOrAlias (debouncedSetNameOrAlias already closes
over setTextFilter), e.g., update the React.useEffect containing
debouncedSetNameOrAlias(typingText) to only include typingText and
debouncedSetNameOrAlias.
In
`@libs/ui-components/src/components/Device/DevicesPage/DeviceToolbarFilters.tsx`:
- Around line 333-352: The debouncedSetTextFilter created with debounce(...) can
leave pending invocations when its dependencies (setTextFilter or t) change;
replace the useMemo pattern with a ref-stored debounced function so you can
cancel the previous instance when recreating: create a React.useRef for the
debounced function (debouncedSetTextFilterRef), initialize or recreate it inside
a useEffect that depends on [setTextFilter, t], and call
debouncedSetTextFilterRef.current(...) where needed; ensure the effect cancels
the previous debounced instance (call .cancel()) in its cleanup to avoid stray
invocations and also cancel on unmount. Use the existing identifiers
debouncedSetTextFilter (or replace with debouncedSetTextFilterRef), debounce,
setTextFilter, and the CVE validation logic unchanged.
In `@libs/ui-components/src/components/Device/DevicesPage/useDevices.ts`:
- Around line 126-136: Remove the redundant nameOrAlias parameter from the
useDevices function signature and related type for its args: delete the
nameOrAlias?: string entry from the args object in useDevices and remove any
code that references that parameter inside the function (the hook already reads
the value from textFilters using DeviceTextFilterKey). Ensure the exported
useDevices signature and any local references only rely on textFilters (and
other existing keys), and run TypeScript to confirm no callers require
nameOrAlias (update any tests or types that referenced the args shape if
necessary).
In `@libs/ui-components/src/components/SecurityOverview/VulnerabilitiesTable.tsx`:
- Around line 4-5: Change the runtime namespace-style imports to type-only
imports and use indexed-access types where the code references model members:
replace the current import with an import type of Vulnerability,
VulnerabilityGroup, VulnerabilityGroupList, VulnerabilityList from
'@flightctl/types/alpha', and update any uses like Vulnerability.severity (or
other property-access on the model types) to the indexed-access form
Vulnerability['severity'] (and similarly for other properties) so the component
depends only on types rather than the generated namespace at runtime.
In
`@libs/ui-components/src/components/SecurityOverview/VulnerabilityDetailsDrawer.tsx`:
- Around line 77-85: The branch in VulnerabilityDetailsDrawer.tsx that runs when
isGrouped reads vulnerability.findings[0].advisoryId and .description without
guarding for an empty findings array; add a runtime guard so advisoryId and
description are only read when vulnerability.findings?.length > 0 (or fallback
to a sensible default like vulnerability.advisoryId/description or an empty
string) and ensure publishedAt still uses vulnerability.maxPublishedAt for
grouped items; alternatively, assert/document that the VulnerabilityGroup type
always contains at least one finding and add a runtime check that throws or logs
if findings is empty to make the assumption explicit.
In `@libs/ui-components/src/components/SecurityOverview/VulnerabilityImpact.tsx`:
- Line 180: The Link to ROUTE.DEVICES is building the query with a hardcoded key
"cveId" — change it to use the shared constant FilterSearchParams.CveId for
consistency; update the URLSearchParams construction in the Link (the element
using ROUTE.DEVICES) to pass new URLSearchParams({ [FilterSearchParams.CveId]:
cveId }).toString() so the query key matches the rest of the codebase.
- Line 253: Link query uses hardcoded "fleetId" instead of the shared constant;
update the Link in VulnerabilityImpact (the <Link to={ROUTE.DEVICES} ...> call)
to build the query using FilterSearchParams.Fleet (which equals 'fleetId')
instead of the literal string so naming is consistent and maintainable; replace
new URLSearchParams({ cveId, fleetId: fleetName }).toString() with a
construction that uses FilterSearchParams.Fleet as the key for fleetName.
In `@libs/ui-components/src/hooks/useServicesEnabled.ts`:
- Around line 38-46: The error handling in useServicesEnabled assumes
error.message contains a pure numeric status code; add explicit documentation
and a small validator/helper to extract and validate numeric status codes
instead of trusting Number(error.message). Update the block referencing
disabledStatusCodes and setIsEnabled (and abortController.signal.aborted) to
call the helper (e.g., parseStatusCodeFromError) which returns a number or null,
and only compare against disabledStatusCodes when a valid number is returned;
also add a short comment above useServicesEnabled explaining the expected
error.message contract so future callers/handlers know to throw plain numeric
status strings.
In `@libs/ui-components/src/types/rbac.ts`:
- Around line 16-17: The WIP comment indicates the RBAC resource path for the
enum member VULNERABILITY = 'vulnerabilities' needs backend confirmation; create
a tracking issue in the repo/board to confirm the backend RBAC resource path,
then replace the inline "CELIA-WIP" comment with a concise TODO that references
the new issue ID (or link) and notes to verify/align the 'vulnerabilities'
resource string with backend RBAC before release so the enum member
(VULNERABILITY) can be finalized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 14ca6724-36a8-4fd0-b442-a8a1ac0f958f
📒 Files selected for processing (85)
apps/ocp-plugin/console-extensions.jsonapps/ocp-plugin/package.jsonapps/ocp-plugin/src/components/AppContext/AppContext.tsxapps/ocp-plugin/src/components/SecurityOverview/SecurityOverviewPage.tsxapps/ocp-plugin/src/utils/apiCalls.tsapps/standalone/src/app/routes.tsxapps/standalone/src/app/utils/apiCalls.tseslint.config.jslibs/i18n/locales/en/translation.jsonlibs/types/alpha/index.tslibs/types/alpha/models/AffectedFleet.tslibs/types/alpha/models/CatalogItemVersion.tslibs/types/alpha/models/CveCountsBySeverity.tslibs/types/alpha/models/DeviceCountsBySeverity.tslibs/types/alpha/models/DeviceVulnerabilitySummaryResponse.tslibs/types/alpha/models/FleetVulnerabilitySummary.tslibs/types/alpha/models/FleetVulnerabilitySummaryResponse.tslibs/types/alpha/models/SemVer.tslibs/types/alpha/models/SemVerRange.tslibs/types/alpha/models/Vulnerability.tslibs/types/alpha/models/VulnerabilityGroup.tslibs/types/alpha/models/VulnerabilityGroupItem.tslibs/types/alpha/models/VulnerabilityGroupList.tslibs/types/alpha/models/VulnerabilityImageRef.tslibs/types/alpha/models/VulnerabilityImpact.tslibs/types/alpha/models/VulnerabilityList.tslibs/types/alpha/models/VulnerabilitySeveritySummary.tslibs/types/alpha/models/VulnerabilitySummaryResponse.tslibs/types/imagebuilder/models/ImageBuildConditionReason.tslibs/types/models/K8sProviderSpec.tslibs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsxlibs/ui-components/src/components/Catalog/CatalogItemDetails.tsxlibs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsxlibs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsTab.tsxlibs/ui-components/src/components/Device/DeviceDetails/DeviceVulnerabilities.tsxlibs/ui-components/src/components/Device/DevicesPage/DecommissionedDevicesTable.tsxlibs/ui-components/src/components/Device/DevicesPage/DeviceNameOnlyToolbarFilter.tsxlibs/ui-components/src/components/Device/DevicesPage/DeviceTableToolbar.tsxlibs/ui-components/src/components/Device/DevicesPage/DeviceToolbarFilters.tsxlibs/ui-components/src/components/Device/DevicesPage/DevicesPage.tsxlibs/ui-components/src/components/Device/DevicesPage/EnrolledDeviceTableRow.tsxlibs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsxlibs/ui-components/src/components/Device/DevicesPage/useDeviceBackendFilters.tslibs/ui-components/src/components/Device/DevicesPage/useDevices.tslibs/ui-components/src/components/EnrollmentRequest/EnrollmentRequestList.tsxlibs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsContent.tsxlibs/ui-components/src/components/Fleet/FleetDetails/FleetVulnerabilities.tsxlibs/ui-components/src/components/Fleet/FleetsPage.tsxlibs/ui-components/src/components/ImageBuilds/ImageBuildAndExportStatus.tsxlibs/ui-components/src/components/ImageBuilds/ImageBuildsPage.tsxlibs/ui-components/src/components/OverviewPage/Cards/SecurityOverview/SecurityOverviewCard.tsxlibs/ui-components/src/components/OverviewPage/Cards/Tasks/TasksCard.tsxlibs/ui-components/src/components/OverviewPage/Overview.tsxlibs/ui-components/src/components/OverviewPage/OverviewPage.tsxlibs/ui-components/src/components/Repository/RepositoryList.tsxlibs/ui-components/src/components/ResourceSync/RepositoryResourceSyncList.tsxlibs/ui-components/src/components/SecurityOverview/SecurityOverviewPage.tsxlibs/ui-components/src/components/SecurityOverview/SecurityOverviewSummary.csslibs/ui-components/src/components/SecurityOverview/SecurityOverviewSummary.tsxlibs/ui-components/src/components/SecurityOverview/VulnerabilitiesEmptyState.tsxlibs/ui-components/src/components/SecurityOverview/VulnerabilitiesTable.tsxlibs/ui-components/src/components/SecurityOverview/VulnerabilitiesTableRow.tsxlibs/ui-components/src/components/SecurityOverview/VulnerabilityAffectedImages.tsxlibs/ui-components/src/components/SecurityOverview/VulnerabilityDescription.tsxlibs/ui-components/src/components/SecurityOverview/VulnerabilityDetailsDrawer.tsxlibs/ui-components/src/components/SecurityOverview/VulnerabilityImpact.tsxlibs/ui-components/src/components/Status/VulnerabilitySeverityStatus.tsxlibs/ui-components/src/components/Table/Table.tsxlibs/ui-components/src/components/common/FlightCtlPageDrawer.tsxlibs/ui-components/src/constants.tslibs/ui-components/src/hooks/useAffectedImagesExpand.tslibs/ui-components/src/hooks/useAlertsEnabled.tslibs/ui-components/src/hooks/useAppContext.tsxlibs/ui-components/src/hooks/useNavigate.tsxlibs/ui-components/src/hooks/useServicesEnabled.tslibs/ui-components/src/hooks/useVulnerabilities.tslibs/ui-components/src/hooks/useVulnerabilityImpact.tslibs/ui-components/src/hooks/useVulnerabilitySummary.tslibs/ui-components/src/types/rbac.tslibs/ui-components/src/utils/api.tslibs/ui-components/src/utils/dates.tslibs/ui-components/src/utils/imageBuilds.tslibs/ui-components/src/utils/status/devices.tslibs/ui-components/src/utils/status/vulnerabilities.tslibs/ui-components/src/utils/vulnerabilities.ts
💤 Files with no reviewable changes (1)
- libs/ui-components/src/hooks/useAlertsEnabled.ts
Comment on lines
+36
to
+40
| if (itemCount === 0 && !hasFiltersEnabled && !isLoading && !isUpdating) { | ||
| // The link to this page is only shown when some vulnerabilities are present. | ||
| // If the user navigates to it directly, we show the empty state assuming there are some devices | ||
| return <SecurityOverviewEmptyState hasDevices />; | ||
| } |
There was a problem hiding this comment.
Don’t mask backend failures with the empty state.
When itemCount === 0, this branch returns before ListPageBody can render error, so request failures can appear as a normal empty state. Add an !error guard here.
Suggested patch
- if (itemCount === 0 && !hasFiltersEnabled && !isLoading && !isUpdating) {
+ if (!error && itemCount === 0 && !hasFiltersEnabled && !isLoading && !isUpdating) {
// The link to this page is only shown when some vulnerabilities are present.
// If the user navigates to it directly, we show the empty state assuming there are some devices
return <SecurityOverviewEmptyState hasDevices />;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (itemCount === 0 && !hasFiltersEnabled && !isLoading && !isUpdating) { | |
| // The link to this page is only shown when some vulnerabilities are present. | |
| // If the user navigates to it directly, we show the empty state assuming there are some devices | |
| return <SecurityOverviewEmptyState hasDevices />; | |
| } | |
| if (!error && itemCount === 0 && !hasFiltersEnabled && !isLoading && !isUpdating) { | |
| // The link to this page is only shown when some vulnerabilities are present. | |
| // If the user navigates to it directly, we show the empty state assuming there are some devices | |
| return <SecurityOverviewEmptyState hasDevices />; | |
| } |
🤖 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/SecurityOverview/SecurityOverviewPage.tsx`
around lines 36 - 40, The empty-state branch currently returns
SecurityOverviewEmptyState when itemCount === 0 regardless of request errors,
which masks backend failures; update the condition to only return that empty
state when there is no error (add a !error guard) so keep the existing check
(itemCount === 0 && !hasFiltersEnabled && !isLoading && !isUpdating) but require
!error as well; locate the conditional around SecurityOverviewEmptyState in
SecurityOverviewPage.tsx and add the error check so ListPageBody can render
errors when present.
Comment on lines
+72
to
+75
| const [vulnerabilitiesEnabled, canListVulnerabilities, isLoading] = useVulnerabilitiesEnabled(); | ||
| if (!vulnerabilitiesEnabled) { | ||
| return <Alert isInline variant="info" title={t('Vulnerability reporting is not enabled in this environment.')} />; | ||
| } |
There was a problem hiding this comment.
Check loading before rendering the “not enabled” alert.
This branch can show a misleading disabled message while useVulnerabilitiesEnabled() is still resolving. Gate on isLoading first (or include it in the condition) to avoid transient incorrect UI state.
🤖 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/SecurityOverview/SecurityOverviewPage.tsx`
around lines 72 - 75, The UI currently shows the "not enabled" Alert while
useVulnerabilitiesEnabled() is still resolving; update the render guard in
SecurityOverviewPage to check isLoading first (e.g., only show the Alert when
isLoading is false and vulnerabilitiesEnabled is false) by changing the
condition that references vulnerabilitiesEnabled to include isLoading (for
example: if (!isLoading && !vulnerabilitiesEnabled) ...) or otherwise render a
loading state while isLoading is true so the transient incorrect Alert cannot
appear.
Comment on lines
+13
to
+47
| export const useVulnerabilitySummary = (): UseVulnerabilitySummaryResult => { | ||
| const { get } = useFetch(); | ||
| const [counts, setCounts] = React.useState<CveCountsBySeverity | null>(null); | ||
| const isLoading = counts === null; | ||
|
|
||
| React.useEffect(() => { | ||
| const abortController = new AbortController(); | ||
|
|
||
| const load = async () => { | ||
| setCounts(null); | ||
| try { | ||
| const summaryRes = await get<VulnerabilitySummaryResponse>('vulnerabilities/summary', abortController.signal); | ||
| if (abortController.signal.aborted) { | ||
| return; | ||
| } | ||
| setCounts(summaryRes.cvesBySeverity); | ||
| } catch { | ||
| if (!abortController.signal.aborted) { | ||
| setCounts(emptySeverityCounts); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| void load(); | ||
|
|
||
| return () => { | ||
| abortController.abort(); | ||
| }; | ||
| }, [get]); | ||
|
|
||
| return { | ||
| counts: counts ?? emptySeverityCounts, | ||
| isLoading, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Missing error state exposure could mislead users.
The hook silently falls back to zero counts on fetch failure without exposing an error state. Consuming components cannot distinguish between "no vulnerabilities" and "API failure", which could display misleading security information.
Impact:
- Security dashboards show "0 vulnerabilities" when the API is unavailable
- Users have no visibility into data freshness or availability
- Monitoring and alerting cannot detect fetch failures
Recommendation:
Add an error field to UseVulnerabilitySummaryResult and expose fetch failures to the consuming component.
🔧 Proposed fix to expose error state
export type UseVulnerabilitySummaryResult = {
counts: CveCountsBySeverity;
isLoading: boolean;
+ error: Error | null;
};
const emptySeverityCounts = { total: 0, critical: 0, high: 0, medium: 0, low: 0, none: 0, unknown: 0 };
export const useVulnerabilitySummary = (): UseVulnerabilitySummaryResult => {
const { get } = useFetch();
const [counts, setCounts] = React.useState<CveCountsBySeverity | null>(null);
+ const [error, setError] = React.useState<Error | null>(null);
const isLoading = counts === null;
React.useEffect(() => {
const abortController = new AbortController();
const load = async () => {
setCounts(null);
+ setError(null);
try {
const summaryRes = await get<VulnerabilitySummaryResponse>('vulnerabilities/summary', abortController.signal);
if (abortController.signal.aborted) {
return;
}
setCounts(summaryRes.cvesBySeverity);
- } catch {
+ } catch (e) {
if (!abortController.signal.aborted) {
- setCounts(emptySeverityCounts);
+ setError(e instanceof Error ? e : new Error('Failed to fetch vulnerability summary'));
+ setCounts(emptySeverityCounts);
}
}
};
void load();
return () => {
abortController.abort();
};
}, [get]);
return {
counts: counts ?? emptySeverityCounts,
isLoading,
+ error,
};
};The consuming component can then show an error state when error is not null.
🤖 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/hooks/useVulnerabilitySummary.ts` around lines 13 -
47, The hook useVulnerabilitySummary currently swallows fetch failures and
always returns fallback counts; update UseVulnerabilitySummaryResult to include
an error?: Error | null field and modify useVulnerabilitySummary to track an
error state (e.g., const [error, setError] = useState<Error | null>(null)), set
error to null before loading, setError(err) in the catch when the request fails
(but do not set for aborts), and return error alongside counts and isLoading so
consumers can distinguish API failures from legitimately empty results; ensure
the abort handling still prevents setting error after abort and preserve the
existing isLoading semantics.
celdrake
commented
May 8, 2026
| FLEET = 'fleets', | ||
| DEVICE = 'devices', | ||
| // CELIA-WIP: confirm vulnerabilities resource path with backend RBAC before release. | ||
| VULNERABILITY = 'vulnerabilities', |
Collaborator
Author
There was a problem hiding this comment.
I believe due to the automated mapping in the API this is the correct RBAC resource, but it's not properly configured in the system so I wasn't able to test this yet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New functionality to display the Vulnerabilities present in the system.
