EDM-2060 Backup and restore devices#347
Conversation
WalkthroughReplaces AppContext provider alias with a component wrapping SystemRestoreProvider; adds SystemRestore context and banners, resume UI (single/mass), types/i18n for resume/events, RBAC and helpers; generalizes POST/PUT fetch helpers with request/response generics; status/chart color support. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Page / Action / Banner
participant Modal as Resume Modal
participant Fetch as useFetch / postData
participant API as Backend
User->>UI: Click "Resume" (single/mass)
UI->>Modal: Open ResumeDevicesModal / MassResumeDevicesModal
Modal->>Fetch: postData("deviceactions/resume", selector)
Fetch->>API: HTTP POST { selector }
API-->>Fetch: 200 { resumedDevices }
Fetch-->>Modal: response
Modal-->>UI: onClose(hasResumed=true)
UI->>UI: onResumeComplete -> refetch data
UI-->>User: Show success/warning/error
sequenceDiagram
autonumber
participant App as App Root
participant SR as SystemRestoreProvider
participant RBAC as useAccessReview
participant DS as useDevicesSummary
participant Consumers as Banners/Pages
App->>SR: Mount provider
SR->>RBAC: Check device listing permission
alt allowed
SR->>DS: Fetch devices summary
DS-->>SR: summaryStatus
SR->>Consumers: Provide summaryStatus, counts, isLoading:false
else denied
SR->>Consumers: Provide isLoading:false, zero counts
end
Consumers->>Consumers: Conditionally render banners/alerts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| fetch: { checkPermissions }, | ||
| } = useAppContext(); | ||
| React.useEffect(() => { | ||
| let isMounted = true; |
There was a problem hiding this comment.
There was an error in the console due to the hook not cleaning-up properly when it was unmounted.
celdrake
force-pushed
the
EDM-2060-backup-and-restore-devices
branch
2 times, most recently
from
a124f05 to
02c660e
Compare
- Show the device count also
celdrake
force-pushed
the
EDM-2060-backup-and-restore-devices
branch
from
02c660e to
98b53ee
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 (14)
libs/ui-components/src/components/form/validations.ts (1)
706-724: Consider simplifying the lazy validation with a direct conditional approach.The
Yup.lazy()wrapper adds complexity that may not be necessary here. Consider using a direct.when()approach for better readability and maintainability.Apply this diff to simplify the labels validation:
- labels: Yup.lazy(() => - Yup.mixed().test('labels-validation', function (labels) { - const parent = this.parent as { mode?: string }; - if (parent.mode !== 'labels') { - return true; - } - if (!Array.isArray(labels) || labels.length === 0) { - return this.createError({ message: t('At least one label is required') }); - } - try { - validLabelsSchema(t).validateSync(labels); - return true; - } catch (error: unknown) { - const errorMessage = error instanceof Yup.ValidationError ? error.message : t('Invalid labels provided'); - return this.createError({ message: errorMessage }); - } - }), - ), + labels: Yup.array().when('mode', ([mode]) => { + if (mode === 'labels') { + return validLabelsSchema(t).min(1, t('At least one label is required')); + } + return Yup.array(); + }),libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/ResumeAllDevicesConfirmationDialog.tsx (1)
25-25: Consider using primary variant instead of danger for resume action.The resume action uses
variant="danger"which typically indicates destructive actions. Since resuming devices is a recovery action rather than destructive, consider usingvariant="primary"instead.- <Button key="confirm" variant="danger" onClick={() => onClose(true)}> + <Button key="confirm" variant="primary" onClick={() => onClose(true)}>libs/ui-components/src/components/ListPage/ListPageActions.tsx (2)
14-14: Consider more descriptive type name for clarity.The type name
DeviceOnlyResourceTypeis somewhat ambiguous. Since this type specifically represents a 'Device' literal, consider renaming it toDeviceResourceLiteralor simply using the literal type directly where needed for better clarity.
122-128: Consider simplifying the onClose logic.The conditional call to
onResumeCompletecould be simplified using optional chaining more consistently.const onClose = (hasResumed?: boolean) => { setDeviceId(undefined); setDeviceName(undefined); - if (hasResumed) { - onResumeComplete?.(); - } + hasResumed && onResumeComplete?.(); };libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx (2)
20-20: Remove default value in destructuring since it's redundant.The
expectedCount = 1default in the destructuring is unnecessary sinceexpectedCountis a required prop in the interface.-const ResumeDevicesModal = ({ mode, title, selector, expectedCount = 1, onClose }: ResumeDeviceModalProps) => { +const ResumeDevicesModal = ({ mode, title, selector, expectedCount, onClose }: ResumeDeviceModalProps) => {
26-26: Consider more explicit boolean conversion.The
hasResumedcalculation could be more explicit about the conditions being checked.- const hasResumed = Boolean(resumedCount && resumedCount > 0); + const hasResumed = resumedCount !== undefined && resumedCount > 0;libs/ui-components/src/utils/status/devices.ts (1)
122-124: Consider adding type guard for better type safety.The type assertion could be avoided with a more explicit type guard.
const orderedPriorityItems = priorityStatuses .map((status) => priorityItems.find((item) => item.id === status)) - .filter(Boolean) as StatusItem<DeviceSummaryStatusType>[]; + .filter((item): item is StatusItem<DeviceSummaryStatusType> => item !== undefined);libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx (4)
86-87: Improve condition readability.The boolean logic could be clearer about what constitutes success.
- const hasResumedAtLeastOne = Boolean(resumedCount !== undefined && resumedCount > 0); - const hasResumedAllExpected = hasResumedAtLeastOne && resumedCount === deviceCountNum; + const hasResumedAtLeastOne = resumedCount !== undefined && resumedCount > 0; + const hasResumedAllExpected = resumedCount === deviceCountNum && deviceCountNum > 0;
138-140: Add clarifying comment for API behavior.The TODO comment indicates this is a temporary workaround. Consider tracking this technical debt properly.
The comment indicates the API should have an explicit way to resume all suspended devices. Would you like me to create an issue to track this API enhancement requirement?
310-312: Consider extracting device selector formatting logic.The device selector string formatting logic is duplicated here and could be extracted to a utility function for reusability.
+// In a utils file +export const formatDeviceSelector = (matchLabels: Record<string, string> = {}): string => { + return Object.entries(matchLabels) + .map(([key, value]) => (value ? `${key}=${value}` : key)) + .join(','); +}; // Then use it here: - const deviceSelectorStr = Object.entries(fleet.spec?.selector?.matchLabels || {}) - .map(([key, value]) => (value ? `${key}=${value}` : key)) - .join(','); + const deviceSelectorStr = formatDeviceSelector(fleet.spec?.selector?.matchLabels);
451-454: Remove empty onSubmit handler.The empty
onSubmithandler in Formik suggests the form submission is handled elsewhere. Consider adding a comment explaining why this is intentionally empty.onSubmit={() => { - // This will be handled by the inner component + // Form submission is handled by performResume() in the inner component + // This empty handler satisfies Formik's required prop }}libs/ui-components/src/components/DetailsPage/DetailsPageActions.tsx (1)
96-98: Consider extracting selector creation to a utility.The device selector creation logic could be extracted to maintain consistency across the codebase.
+// In a utils file +export const createDeviceSelector = (deviceId: string) => ({ + fieldSelector: `metadata.name=${deviceId}`, +}); // Then use it here: - const resumeSelector = { - fieldSelector: `metadata.name=${deviceId}`, - }; + const resumeSelector = createDeviceSelector(deviceId);libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx (1)
43-43: Consider using classnames utility for conditional CSS classes.The manual string concatenation for CSS classes could be simplified using a utility like
classnamesfor better maintainability.If the
classnamespackage is available in your project, consider:+import classNames from 'classnames'; // ... return ( - <div className={`fctl-system-restore-banners ${className}`}> + <div className={classNames('fctl-system-restore-banners', className)}>libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx (1)
101-107: Consider extracting device summary logic to a utility function.The device summary status calculation could be extracted into a reusable utility function for better maintainability and reuse across other components.
Consider creating a utility function:
// In a utils file export const getDeviceSummaryStatus = (device: Device) => { const status = device?.status?.summary.status; return { [DeviceSummaryStatusType.DeviceSummaryStatusConflictPaused]: status === DeviceSummaryStatusType.DeviceSummaryStatusConflictPaused ? 1 : 0, [DeviceSummaryStatusType.DeviceSummaryStatusAwaitingReconnect]: status === DeviceSummaryStatusType.DeviceSummaryStatusAwaitingReconnect ? 1 : 0, }; }; // Then use it here: const deviceSummary = device ? getDeviceSummaryStatus(device) : undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
apps/ocp-plugin/src/components/AppContext/AppContext.tsx(2 hunks)apps/ocp-plugin/src/hooks/useFetch.ts(1 hunks)apps/ocp-plugin/src/utils/apiCalls.ts(2 hunks)apps/standalone/src/app/hooks/useFetch.ts(1 hunks)apps/standalone/src/app/index.tsx(2 hunks)apps/standalone/src/app/utils/apiCalls.ts(2 hunks)libs/ansible/src/utils/apiCalls.ts(2 hunks)libs/i18n/locales/en/translation.json(8 hunks)libs/types/index.ts(2 hunks)libs/types/models/DeviceResumeRequest.ts(1 hunks)libs/types/models/DeviceResumeResponse.ts(1 hunks)libs/types/models/Event.ts(2 hunks)libs/types/models/EventDetails.ts(1 hunks)libs/types/models/InternalTaskPermanentlyFailedDetails.ts(1 hunks)libs/types/models/PatchRequest.ts(1 hunks)libs/ui-components/src/components/DetailsPage/DetailsPage.tsx(3 hunks)libs/ui-components/src/components/DetailsPage/DetailsPageActions.tsx(3 hunks)libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx(7 hunks)libs/ui-components/src/components/Device/DevicesPage/DevicesPage.tsx(1 hunks)libs/ui-components/src/components/Device/DevicesPage/EnrolledDeviceTableRow.tsx(4 hunks)libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx(8 hunks)libs/ui-components/src/components/Events/useEvents.ts(1 hunks)libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx(3 hunks)libs/ui-components/src/components/Fleet/FleetDetails/FleetDevicesCharts.tsx(3 hunks)libs/ui-components/src/components/Fleet/FleetDetails/FleetRestoreBanner.tsx(1 hunks)libs/ui-components/src/components/Fleet/FleetsPage.tsx(2 hunks)libs/ui-components/src/components/ListPage/ListPageActions.tsx(4 hunks)libs/ui-components/src/components/OverviewPage/Cards/Status/DeviceStatusChart.tsx(2 hunks)libs/ui-components/src/components/OverviewPage/Overview.tsx(1 hunks)libs/ui-components/src/components/Status/StatusDisplay.tsx(5 hunks)libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx(1 hunks)libs/ui-components/src/components/SystemRestore/SuspendedDevicesAlert.tsx(1 hunks)libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.css(1 hunks)libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx(1 hunks)libs/ui-components/src/components/charts/utils.ts(1 hunks)libs/ui-components/src/components/form/FilterSelect.css(1 hunks)libs/ui-components/src/components/form/validations.ts(1 hunks)libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx(1 hunks)libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx(1 hunks)libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/ResumeAllDevicesConfirmationDialog.tsx(1 hunks)libs/ui-components/src/hooks/useAccessReview.ts(1 hunks)libs/ui-components/src/hooks/useAppContext.tsx(1 hunks)libs/ui-components/src/hooks/useSystemRestoreContext.tsx(1 hunks)libs/ui-components/src/types/rbac.ts(1 hunks)libs/ui-components/src/utils/devices.ts(2 hunks)libs/ui-components/src/utils/query.ts(2 hunks)libs/ui-components/src/utils/status/common.ts(1 hunks)libs/ui-components/src/utils/status/devices.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-11T10:20:50.496Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#208
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx:30-44
Timestamp: 2025-02-11T10:20:50.496Z
Learning: In the FlightCtl UI, error handling for device decommissioning is implemented in the modal component created by `useDecommissionAction`. Failed PUT requests show an error in the modal with retry/close options.
Applied to files:
libs/ui-components/src/components/Device/DevicesPage/EnrolledDeviceTableRow.tsxlibs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsxlibs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx
📚 Learning: 2025-02-11T10:24:15.684Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#208
File: libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx:142-150
Timestamp: 2025-02-11T10:24:15.684Z
Learning: In the FlightCtl UI, device tables (`EnrolledDevicesTable` and `DecommissionedDevicesTable`) are swapped based on the `onlyDecommissioned` state, so their respective switches can have hardcoded values (`false` and `true`).
Applied to files:
libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx
📚 Learning: 2025-03-19T08:55:03.335Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:55:03.335Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by validation logic even though TypeScript type definitions might not explicitly show this distinction.
Applied to files:
libs/ui-components/src/utils/query.ts
🧬 Code graph analysis (34)
libs/types/models/DeviceResumeResponse.ts (1)
libs/types/index.ts (1)
DeviceResumeResponse(54-54)
libs/ui-components/src/components/Fleet/FleetsPage.tsx (1)
libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx (1)
GlobalSystemRestoreBanners(61-82)
libs/types/models/DeviceResumeRequest.ts (1)
libs/types/index.ts (1)
DeviceResumeRequest(53-53)
libs/ui-components/src/components/Fleet/FleetDetails/FleetRestoreBanner.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx (1)
SystemRestoreBanners(25-56)libs/ui-components/src/utils/labels.ts (2)
fromAPILabel(7-12)labelToExactApiMatchString(25-25)
libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx (2)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)
libs/ui-components/src/hooks/useSystemRestoreContext.tsx (2)
libs/ui-components/src/hooks/useAccessReview.ts (1)
useAccessReview(8-49)libs/ui-components/src/components/Device/DevicesPage/useDevices.ts (1)
useDevicesSummary(90-103)
libs/types/models/InternalTaskPermanentlyFailedDetails.ts (1)
libs/types/models/Event.ts (1)
Event(12-41)
libs/ui-components/src/components/Device/DevicesPage/EnrolledDeviceTableRow.tsx (3)
libs/ui-components/src/components/ListPage/types.ts (1)
ListAction(3-3)libs/ui-components/src/utils/tooltip.ts (1)
getDisabledTooltipProps(1-2)libs/ui-components/src/utils/devices.ts (1)
getResumeDisabledReason(33-38)
libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)apps/standalone/src/app/hooks/useFetch.ts (1)
useFetch(5-46)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)
libs/types/models/EventDetails.ts (2)
libs/types/models/InternalTaskFailedDetails.ts (1)
InternalTaskFailedDetails(6-20)libs/types/models/InternalTaskPermanentlyFailedDetails.ts (1)
InternalTaskPermanentlyFailedDetails(6-20)
apps/ocp-plugin/src/components/AppContext/AppContext.tsx (2)
libs/ui-components/src/hooks/useAppContext.tsx (2)
AppContextProps(48-86)AppContext(88-120)libs/ui-components/src/hooks/useSystemRestoreContext.tsx (1)
SystemRestoreProvider(20-44)
libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx (9)
libs/ui-components/src/types/extraTypes.ts (1)
FlightCtlLabel(15-19)libs/ui-components/src/utils/labels.ts (2)
fromAPILabel(7-12)labelToExactApiMatchString(25-25)apps/ocp-plugin/src/hooks/useFetch.ts (1)
useFetch(39-93)apps/standalone/src/app/hooks/useFetch.ts (1)
useFetch(5-46)libs/ui-components/src/components/Fleet/useFleets.ts (1)
useFleets(87-104)libs/ui-components/src/utils/query.ts (1)
commonQueries(35-99)libs/ui-components/src/utils/api.ts (1)
getApiListCount(45-45)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/components/form/validations.ts (1)
createMassResumeValidationSchema(699-724)
libs/ui-components/src/components/DetailsPage/DetailsPageActions.tsx (2)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/tooltip.ts (1)
getDisabledTooltipProps(1-2)
libs/ui-components/src/utils/status/devices.ts (2)
libs/types/index.ts (1)
DeviceSummaryStatusType(59-59)libs/ui-components/src/utils/status/common.ts (1)
StatusItem(18-24)
libs/ansible/src/utils/apiCalls.ts (2)
apps/ocp-plugin/src/utils/apiCalls.ts (2)
postData(97-98)putData(100-101)apps/standalone/src/app/utils/apiCalls.ts (2)
postData(128-129)putData(131-132)
libs/ui-components/src/components/OverviewPage/Overview.tsx (3)
libs/ui-components/src/hooks/useAlertsEnabled.ts (1)
useAlertsEnabled(10-50)libs/ui-components/src/hooks/useAccessReview.ts (1)
useAccessReview(8-49)libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx (1)
GlobalSystemRestoreBanners(61-82)
libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx (3)
libs/ui-components/src/components/ListPage/ListPageActions.tsx (1)
useResumeListAction(105-145)libs/ui-components/src/hooks/useAccessReview.ts (1)
useAccessReview(8-49)libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx (1)
GlobalSystemRestoreBanners(61-82)
libs/ui-components/src/components/charts/utils.ts (1)
libs/ui-components/src/utils/status/common.ts (1)
getDefaultStatusColor(49-72)
libs/ui-components/src/components/ListPage/ListPageActions.tsx (3)
libs/ui-components/src/components/ListPage/types.ts (3)
ListActionProps(10-13)ListActionResult(5-8)ListAction(3-3)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/tooltip.ts (1)
getDisabledTooltipProps(1-2)
apps/standalone/src/app/utils/apiCalls.ts (2)
apps/ocp-plugin/src/utils/apiCalls.ts (2)
postData(97-98)putData(100-101)libs/ansible/src/utils/apiCalls.ts (2)
postData(44-49)putData(51-56)
apps/standalone/src/app/index.tsx (1)
libs/ui-components/src/hooks/useSystemRestoreContext.tsx (1)
SystemRestoreProvider(20-44)
apps/standalone/src/app/hooks/useFetch.ts (2)
apps/ocp-plugin/src/utils/apiCalls.ts (2)
postData(97-98)putData(100-101)apps/standalone/src/app/utils/apiCalls.ts (2)
postData(128-129)putData(131-132)
libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx (3)
libs/ui-components/src/components/SystemRestore/SuspendedDevicesAlert.tsx (1)
ResumeMode(10-10)libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx (1)
PendingSyncDevicesAlert(13-36)libs/ui-components/src/hooks/useSystemRestoreContext.tsx (1)
useSystemRestore(46-54)
libs/ui-components/src/hooks/useAccessReview.ts (1)
libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)
libs/ui-components/src/components/OverviewPage/Cards/Status/DeviceStatusChart.tsx (2)
libs/types/index.ts (1)
DeviceSummaryStatusType(59-59)libs/ui-components/src/utils/status/devices.ts (1)
getOverviewDeviceStatusItems(104-127)
libs/ui-components/src/components/Fleet/FleetDetails/FleetDevicesCharts.tsx (1)
libs/ui-components/src/utils/status/devices.ts (1)
getOverviewDeviceStatusItems(104-127)
libs/ui-components/src/components/SystemRestore/SuspendedDevicesAlert.tsx (1)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)
libs/ui-components/src/utils/devices.ts (1)
libs/types/index.ts (1)
DeviceSummaryStatusType(59-59)
libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/ResumeAllDevicesConfirmationDialog.tsx (1)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)
apps/ocp-plugin/src/utils/apiCalls.ts (2)
apps/standalone/src/app/utils/apiCalls.ts (2)
postData(128-129)putData(131-132)libs/ansible/src/utils/apiCalls.ts (2)
postData(44-49)putData(51-56)
libs/ui-components/src/utils/query.ts (3)
libs/ui-components/src/types/extraTypes.ts (1)
FlightCtlLabel(15-19)libs/ui-components/src/utils/labels.ts (1)
labelToExactApiMatchString(25-25)libs/types/index.ts (1)
DeviceSummaryStatusType(59-59)
libs/ui-components/src/components/Status/StatusDisplay.tsx (1)
libs/ui-components/src/utils/status/common.ts (2)
getDefaultStatusIcon(26-47)getDefaultStatusColor(49-72)
apps/ocp-plugin/src/hooks/useFetch.ts (2)
apps/ocp-plugin/src/utils/apiCalls.ts (4)
postData(97-98)putData(100-101)deleteData(103-116)patchData(118-135)libs/types/models/PatchRequest.ts (1)
PatchRequest(5-18)
libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx (5)
libs/ui-components/src/utils/devices.ts (3)
isDeviceEnrolled(11-11)getResumeDisabledReason(33-38)getEditDisabledReason(13-21)libs/ui-components/src/hooks/useAccessReview.ts (1)
useAccessReview(8-49)libs/ui-components/src/components/DetailsPage/DetailsPageActions.tsx (1)
useResumeAction(85-119)libs/ui-components/src/utils/tooltip.ts (1)
getDisabledTooltipProps(1-2)libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx (1)
SystemRestoreBanners(25-56)
🪛 Biome (2.1.2)
libs/ui-components/src/components/form/validations.ts
[error] 704-704: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (71)
apps/standalone/src/app/index.tsx (2)
3-3: LGTM! Clean import addition.The import is correctly placed and follows the established import organization pattern in the file.
23-25: Well-structured context provider integration.The SystemRestoreProvider is properly positioned in the component hierarchy, nested within AppContext and AuthContext as expected. This ensures the SystemRestore context has access to both app configuration and authentication state, which aligns with the RBAC checks shown in the provider implementation.
The provider will now make system restore status data (pending sync and suspended device counts) available to all components throughout the application, enabling the display of system restore banners and alerts as mentioned in the PR summary.
libs/ui-components/src/hooks/useAccessReview.ts (4)
17-17: LGTM: Proper component cleanup implementation.The
isMountedref initialization correctly addresses the cleanup issue mentioned in the past review comment.
20-20: LGTM: Early return prevents unnecessary async operations.The early return check prevents the async operation from proceeding if the component has already unmounted, which is a good optimization.
25-27: LGTM: Comprehensive state update protection.All state updates are properly guarded with
isMountedchecks, preventing React warnings about setting state on unmounted components. The implementation correctly handles all three state variables (setIsAllowed,setError,setIsLoading) in both success and error paths.Also applies to: 29-32, 34-36
42-46: LGTM: Proper cleanup function implementation.The cleanup function correctly sets
isMountedtofalseto prevent any pending state updates after component unmount. This addresses the console error mentioned in the past review comments.libs/ui-components/src/components/form/FilterSelect.css (2)
10-10: LGTM: Minor formatting improvements.The indentation adjustments for
.fctl-toggle-content__badgeand the trailing closing brace improve code consistency.Also applies to: 17-17
2-2: Verify dropdown behavior after removing fixed height from .fctl-filter-select__group..fctl-filter-select__group now only sets overflow: auto in libs/ui-components/src/components/form/FilterSelect.css and is used by libs/ui-components/src/components/form/FilterSelect.tsx (and by libs/ui-components/src/components/Device/DevicesPage/DeviceFilterSelect.tsx) — test the dropdowns (e.g., DevicesPage) near viewport edges and in constrained containers; if it grows beyond the viewport or breaks layout, restore a max-height (e.g., max-height: 18rem) with overflow:auto.
libs/ui-components/src/components/charts/utils.ts (1)
30-30: LGTM! Clean implementation of custom color support.The fallback logic correctly prioritizes
customColorwhen available, maintaining backward compatibility while enabling per-item color customization.libs/ui-components/src/hooks/useAppContext.tsx (1)
74-79: LGTM! Generic signature update aligns with API evolution.The updated generic signatures for
post<TRequest, TResponse = TRequest>andput<TRequest>enable better type safety by allowing distinct request/response types for POST operations while maintaining uniform input typing for PUT operations.libs/ansible/src/utils/apiCalls.ts (1)
20-26: LGTM! Consistent generic signature updates.The changes align with the broader API evolution across the codebase:
putOrPostData<TRequest, TResponse = TRequest>enables distinct request/response typingpostData<TRequest, TResponse = TRequest>supports flexible response typesputData<TRequest>maintains uniform input typingThe implementation correctly delegates to the shared
putOrPostDatafunction with appropriate generic parameters.Also applies to: 44-49, 51-56
libs/i18n/locales/en/translation.json (1)
93-94: LGTM! Comprehensive translation support for resume functionality.The translation keys appropriately cover:
- Single and bulk device resume actions with proper pluralization
- Status messages (pending sync, suspended, conflict resolved)
- Validation messages for resume criteria
- Warning messages for configuration review
- Success/failure feedback with device count details
The translations are well-structured and provide clear user guidance throughout the resume workflow.
Also applies to: 127-127, 361-361, 365-365, 468-470, 569-571, 575-575, 631-672, 786-794, 828-828, 839-841
libs/ui-components/src/utils/status/common.ts (1)
23-23: LGTM! Clean addition of customColor property.The optional
customColorproperty extends the StatusItem interface without breaking existing implementations, enabling per-item color customization as demonstrated in the chart utility changes.libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.css (1)
1-6: LGTM! Appropriate layout styles for system restore banners.The CSS class provides proper spacing and flexbox layout for banner components with consistent gap between items.
libs/ui-components/src/components/OverviewPage/Overview.tsx (1)
8-8: LGTM! Clean integration of system restore banners.The changes integrate the GlobalSystemRestoreBanners component properly:
- Import is correctly added
alertsEnabledinitialization moved before access review (good practice)- Banner is rendered with appropriate padding override
The integration maintains the existing component structure while adding the new system restore functionality.
Also applies to: 15-15, 21-21
libs/types/models/PatchRequest.ts (1)
17-17: LGTM! JSON Patch 'test' operation addition is correct.The 'test' operation is a standard part of RFC 6902 (JSON Patch) specification that "tests that a value at the target location is equal to a specified value". The expanded union type now includes all six standard JSON Patch operations: 'add', 'remove', 'replace', 'move', 'copy', and 'test'.
libs/ui-components/src/components/Events/useEvents.ts (1)
81-82: LGTM! Event reason mappings correctly added for system restore functionality.The new event reason mappings align with the system restore feature being implemented:
DEVICE_CONFLICT_RESOLVEDmaps to "Device conflict has been resolved" which supports the device resume workflowINTERNAL_TASK_PERMANENTLY_FAILEDmaps to "Internal task permanently failed" which provides visibility into task failures during system restore operationsThe localization follows the existing pattern and integrates properly with the broader system restore implementation.
Also applies to: 87-87
libs/ui-components/src/types/rbac.ts (1)
16-16: LGTM! RBAC resource addition follows established pattern.The
DEVICE_RESUME = 'devices/resume'resource follows the same naming convention as existing device-specific resources likeDEVICE_CONSOLEandDEVICE_DECOMMISSION. This enables proper permission checks for the device resume functionality being implemented across the UI.libs/ui-components/src/components/Fleet/FleetsPage.tsx (1)
40-40: LGTM! System restore banner integration is properly implemented.The
GlobalSystemRestoreBannerscomponent is correctly imported and rendered within theListPageBodywith theonResumeComplete={refetch}callback. This ensures that when resume operations are completed through the banner, the fleet data is refreshed to reflect the updated state.Also applies to: 126-126
libs/types/models/EventDetails.ts (1)
15-15: LGTM! EventDetails union type correctly extended.The import and union type addition for
InternalTaskPermanentlyFailedDetailsis consistent with the existing pattern used for other event detail types. This enables proper typing for events related to permanently failed internal tasks during system restore operations.Also applies to: 22-22
libs/ui-components/src/components/Device/DevicesPage/DevicesPage.tsx (1)
105-105: LGTM! Device refetch callback properly wired.The
refetchDevices={refetch}prop correctly passes the data refresh function toEnrolledDevicesTable, enabling the component to trigger device list updates when resume operations complete. This integration aligns with the system restore functionality being implemented.libs/ui-components/src/components/DetailsPage/DetailsPage.tsx (1)
34-34: LGTM! Banner prop addition enhances DetailsPage flexibility.The optional
banner?: React.ReactNodeprop and its rendering between the header and navigation sections provides a clean way to inject system restore banners into detail pages. The implementation follows React best practices and maintains backward compatibility.Also applies to: 49-49, 91-91
libs/types/models/DeviceResumeResponse.ts (1)
1-15: LGTM! Auto-generated type definition is properly structured.The
DeviceResumeResponsetype provides a clear interface for resume operation responses with aresumedDevices: numberfield to track the count of successfully resumed devices. The auto-generated file headers and structure follow the established pattern used throughout the types package.libs/ui-components/src/components/Device/DevicesPage/EnrolledDeviceTableRow.tsx (4)
7-7: LGTM! Resume functionality imported correctly.The import of
getResumeDisabledReasoncorrectly aligns with the new resume functionality pattern, following the same approach as the existing decommission functionality.
25-26: LGTM! Resume props added to interface.The addition of
canResumeboolean andresumeActionListAction props follows the established pattern used for decommission functionality.
37-38: LGTM! Resume parameters and disabled reason handling implemented correctly.The implementation follows the established pattern for similar actions like decommission, with proper parameter destructuring and disabled reason calculation.
Also applies to: 46-46
92-100: LGTM! Resume action rendering follows established pattern.The conditional rendering of the resume action matches the existing decommission pattern perfectly, with proper parameter passing including
disabledReasonfor tooltip functionality.libs/types/models/DeviceResumeRequest.ts (1)
1-21: LGTM! Well-documented type definition for device resume functionality.The type follows TypeScript discriminated union best practices and includes clear JSDoc comments explaining the Kubernetes-style selector syntax. The optional nature of both fields allows flexibility while maintaining type safety.
libs/types/index.ts (1)
53-54: LGTM! New types properly exported from index.The three new exports (
DeviceResumeRequest,DeviceResumeResponse, andInternalTaskPermanentlyFailedDetails) are correctly added to the public API, maintaining alphabetical order and following the established export pattern.Also applies to: 114-114
libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx (2)
18-18: LGTM! Banner integration follows proper component composition pattern.The addition of
FleetRestoreBannerto theDetailsPagevia thebannerprop follows React composition principles correctly, passing the necessaryfleetandrefetchprops for functionality.Also applies to: 49-49
103-103: LGTM! Filename prop updated for YAML editor.The change from
fleet.metadata.nametofleetIdfor the filename prop is logical and maintains consistency with the component's routing structure.libs/types/models/Event.ts (1)
70-70: LGTM! New event reasons added for system restore functionality.The addition of
DEVICE_CONFLICT_RESOLVEDandINTERNAL_TASK_PERMANENTLY_FAILEDenum values properly extends the event system to support the new restore functionality, maintaining the established naming convention.Also applies to: 83-83
libs/ui-components/src/components/Fleet/FleetDetails/FleetDevicesCharts.tsx (3)
4-4: LGTM! Updated imports for enhanced status handling.The addition of
DeviceSummaryStatusTypeand update fromgetDeviceStatusItemstogetOverviewDeviceStatusItemscorrectly aligns with the enhanced status utilities for system restore functionality.Also applies to: 6-6
22-25: LGTM! System restore statuses properly defined.The
systemRestoreStatusesconstant correctly identifies the statuses related to system restore functionality (AwaitingReconnectandConflictPaused) for exclusion logic.
86-87: LGTM! Dynamic status exclusion logic implemented correctly.The filtering logic to exclude system restore statuses when they have no device count (falsy values) is well-implemented, and the use of
getOverviewDeviceStatusItemswithexcludeStatusesmaintains chart relevance.libs/types/models/InternalTaskPermanentlyFailedDetails.ts (1)
6-20: LGTM! Well-structured discriminated union type for task failure details.The type properly implements a discriminated union pattern with the
detailTypediscriminator property, includes comprehensive JSDoc documentation, and provides all necessary fields (errorMessage,retryCount,originalEvent) for handling permanently failed internal tasks.apps/ocp-plugin/src/hooks/useFetch.ts (3)
45-50: LGTM! Enhanced generics for distinct request/response types.The update to support separate
TRequestandTResponsegenerics withTResponse = TRequestdefault provides flexibility for API calls where request and response types differ while maintaining backward compatibility.
52-54: LGTM! PUT method maintains expected behavior.The PUT method correctly maintains the same type for request and response (
TRequest), which is typical for PUT operations that return the updated resource.
62-63: LGTM! Parameter rename improves clarity.The change from
objtopatchesfor the PATCH method parameter better reflects the actual data type (PatchRequest) and improves code readability.libs/ui-components/src/components/OverviewPage/Cards/Status/DeviceStatusChart.tsx (3)
8-8: LGTM! Import updated to use the enhanced overview status helper.The import change from
getDeviceStatusItemstogetOverviewDeviceStatusItemscorrectly aligns with the enhanced status item generation that supports filtering.
13-16: LGTM! System restore statuses properly defined.The hardcoded array correctly defines the system restore statuses that need special handling in the overview chart.
29-30: LGTM! Filtering logic correctly excludes unavailable statuses.The filtering logic properly excludes system restore statuses that are not present in the current device status data, ensuring the chart only shows relevant status items.
libs/ui-components/src/utils/query.ts (3)
3-3: LGTM! Import correctly added for device status type.The import of
DeviceSummaryStatusTypeis correctly added to support the new suspended device queries.
78-88: LGTM! Query builder correctly filters suspended devices by labels.The function properly constructs a query to count suspended devices filtered by specific labels using exact label matching and the correct suspended status.
89-98: LGTM! Query builder correctly gets all suspended devices count.The function properly constructs a query to count all suspended devices using the correct field selector for the suspended status.
libs/ui-components/src/utils/devices.ts (3)
1-1: LGTM! Import correctly updated to include device status type.The import is properly updated to include
DeviceSummaryStatusTypeneeded for the resume functionality.
30-31: LGTM! Resume eligibility check correctly implemented.The function correctly determines if a device is resumable by checking for the specific suspended status.
33-38: LGTM! Disabled reason logic correctly implemented.The function properly provides a localized reason when a device cannot be resumed.
apps/ocp-plugin/src/components/AppContext/AppContext.tsx (2)
9-9: LGTM! Import correctly added for SystemRestoreProvider.The import is properly added to support the system restore context integration.
29-42: LGTM! Component correctly wraps context with SystemRestoreProvider.The component structure correctly wraps the AppContext.Provider with SystemRestoreProvider to provide system restore functionality to the OCP plugin.
libs/ui-components/src/hooks/useSystemRestoreContext.tsx (3)
7-12: LGTM! Interface correctly defines the context data structure.The interface properly defines all the required fields for system restore context data with appropriate types.
20-44: LGTM! Provider correctly fetches and provides system restore data.The provider properly:
- Checks user permissions before fetching data
- Fetches unfiltered device summary for global restore status
- Computes pending sync and suspended device counts
- Provides appropriate values based on user permissions
46-54: LGTM! Hook correctly validates context usage.The hook properly validates that it's used within the SystemRestoreProvider and provides a helpful error message if not.
libs/ui-components/src/components/Fleet/FleetDetails/FleetRestoreBanner.tsx (2)
34-38: LGTM! Label selector correctly constructed from fleet match labels.The code properly converts the fleet's matchLabels to a label selector string using the exact API match format, which will correctly scope the resume action to devices belonging to this fleet.
29-32: Pluralization verified — translation keys presenten translation file has matching _one/_other keys for this message (libs/i18n/locales/en/translation.json lines 469–470), so Trans with the count prop will pluralize correctly.
libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx (2)
11-11: LGTM! Brand helper correctly handles RHEM vs Flight Control branding.The helper function properly determines the brand name based on the RHEM setting.
17-29: LGTM! Message logic correctly adapts for single vs multi-device scenarios.The function properly provides different messaging based on whether it's for a single device or multiple devices, and correctly uses brand-specific wording.
libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/ResumeAllDevicesConfirmationDialog.tsx (1)
37-39: Pluralization verified — keys and usage are correct.
en translations include _one/_other entries for "You are about to resume all <1>{deviceCount}</1> suspended devices" (lines ~649–650) and ResumeAllDevicesConfirmationDialog passes count={deviceCountNum} (number), so Trans pluralization will work.libs/ui-components/src/components/ListPage/ListPageActions.tsx (1)
105-145: Good implementation of the resume action flow.The
useResumeListActionhook follows the established pattern of the other action hooks, properly manages state, handles cleanup, and integrates well with the translation system.libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx (1)
75-79: Well-structured user messaging.The modal provides clear, context-appropriate messaging for the irreversible resume action, properly warning users about the implications.
libs/ui-components/src/utils/status/devices.ts (2)
84-97: Good implementation of the status updates.The changes to replace "Awaiting reconnect" and "Updates paused" with "Pending sync" and "Suspended" are well-implemented, with appropriate icons and color tokens for visual differentiation.
100-127: Well-designed prioritization logic.The
getOverviewDeviceStatusItemsfunction correctly implements the prioritization of system restore statuses, maintaining their relative order while allowing for flexible exclusion.libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx (1)
59-65: Add null check before accessing fleet properties.The function correctly throws an error if the fleet is not found, which is good defensive programming.
libs/ui-components/src/components/DetailsPage/DetailsPageActions.tsx (1)
85-119: Clean implementation of resume action.The
useResumeActionhook follows the established patterns well, properly manages state, and correctly handles the completion callback.apps/standalone/src/app/utils/apiCalls.ts (1)
108-126: LGTM! Clean implementation of distinct request/response type support.The generic parameter changes allow for proper typing when API responses differ from request payloads, which is a common pattern for operations like device resume where the request and response have different structures.
apps/standalone/src/app/hooks/useFetch.ts (1)
11-20: LGTM! Consistent API surface with the underlying utils.The hook properly delegates to the updated
postDataandputDatafunctions with appropriate generic parameters.apps/ocp-plugin/src/utils/apiCalls.ts (1)
75-101: LGTM! Maintains consistency across both deployment modes.The implementation correctly mirrors the standalone version's generic type support, ensuring consistent behavior across OCP plugin and standalone deployments.
libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx (2)
133-134: Good placement of the system restore banners.The banners are appropriately placed at the top of the device table and correctly wired to refresh the device list when resume operations complete.
206-206: Efficient modal rendering logic.Using the logical OR operator to render either modal is clean and prevents unnecessary React reconciliation.
libs/ui-components/src/components/Status/StatusDisplay.tsx (1)
28-39: Well-structured custom color support implementation.The implementation correctly handles custom colors with proper fallback logic and conditional status override. The use of CSS custom properties for dynamic color application is a clean approach.
libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx (1)
129-139: Clean integration of system restore banners.The banner is properly conditionally rendered and correctly configured with device-specific resume action and summary status.
celdrake
force-pushed
the
EDM-2060-backup-and-restore-devices
branch
2 times, most recently
from
a92cfb3 to
36a2e2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx (1)
206-206: Avoid hard-prioritizing one modal via logical OR.Using decommissionDeviceModal || resumeDeviceModal will always prefer the left one if both are truthy, regardless of which action triggered last. Consider explicit mutual exclusivity or rendering whichever was opened last.
Example minimal tweak to render the last opened modal (store lastOpened in state when opening either modal), or if mutual exclusivity is guaranteed, leave as-is. If you prefer to render whichever is set without priority, render both conditionally:
- {decommissionDeviceModal || resumeDeviceModal} + {decommissionDeviceModal} + {resumeDeviceModal}libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx (1)
94-126: Prevent stale updates/race conditions in count loader (add abort + cleanup).Rapid mode/selection changes can race multiple fetches; late arrivals may clobber state after a newer selection or after unmount. Use AbortController and cleanup.
Apply:
@@ - const loadMatchingDevicesCount = React.useCallback( + const countAbortRef = React.useRef<AbortController | null>(null); + + // Abort any in-flight count request on unmount + React.useEffect(() => { + return () => { + countAbortRef.current?.abort(); + }; + }, []); + + const loadMatchingDevicesCount = React.useCallback( async (criteria: { fleetId?: string; labels?: FlightCtlLabel[]; all?: boolean }) => { setIsCountLoading(true); setCountError(null); setDeviceCountNum(0); try { + // Cancel previous pending request (if any) + countAbortRef.current?.abort(); + const ac = new AbortController(); + countAbortRef.current = ac; + let queryEndpoint: string; @@ - const deviceResult = await get<DeviceList>(queryEndpoint); + const deviceResult = await get<DeviceList>(queryEndpoint, ac.signal); await showSpinnerBriefly(); setDeviceCountNum(getApiListCount(deviceResult) || 0); } catch (error) { await showSpinnerBriefly(); setCountError(t('Failed to obtain the number of matching devices')); } finally { setIsCountLoading(false); } }, - [get, t, fleets], + [get, t, fleets], );libs/i18n/locales/en/translation.json (3)
469-471: Trans placeholders: use sequential indices for components.The indices 1 and 4 are unusual and may not align with the component order when using . Prefer 0 and 1 (or 0 and 2) consistently to avoid rendering issues if/when i18nKey is used.
Suggested:
- "You are about to resume all<1>{suspendedDevicesCount}</1> suspended devices in <4>{fleetId}</4>_one": "You are about to resume <1>{suspendedDevicesCount}</1> suspended device in <4>{fleetId}</4>", - "You are about to resume all<1>{suspendedDevicesCount}</1> suspended devices in <4>{fleetId}</4>_other": "You are about to resume all<1>{suspendedDevicesCount}</1> suspended devices in <4>{fleetId}</4>", + "You are about to resume all<1>{suspendedDevicesCount}</1> suspended devices in <3>{fleetId}</3>_one": "You are about to resume <1>{suspendedDevicesCount}</1> suspended device in <3>{fleetId}</3>", + "You are about to resume all<1>{suspendedDevicesCount}</1> suspended devices in <3>{fleetId}</3>_other": "You are about to resume all<1>{suspendedDevicesCount}</1> suspended devices in <3>{fleetId}</3>",Or, better: provide explicit i18nKey in code and align indices with actual components.
646-647: Trans placeholders likely mismatched with component count.These keys use indices 0 and 3; the corresponding JSX uses two components, which typically map to 0 and 1. Misaligned indices can break markup when using i18nKey.
Proposed:
- "<0>{deviceCount}</0> suspended devices are currently associated with fleet <3>{values.fleetId}</3>.": "<0>{deviceCount}</0> suspended devices are currently associated with fleet <3>{values.fleetId}</3>.", + "<0>{deviceCount}</0> suspended devices are currently associated with fleet <1>{values.fleetId}</1>.": "<0>{deviceCount}</0> suspended devices are currently associated with fleet <1>{values.fleetId}</1>.",
574-575: Duplicate-ish keys for same message increase maintenance.“You are about to resume device …” exists with different variable names. Consider consolidating to a single key to avoid drift.
libs/ansible/src/hooks/useFetch.ts (1)
26-35: Remove the unnecessary cast on post() in useFetchpostData in libs/ansible/src/utils/apiCalls.ts already returns Promise, so the
as Promise<TResponse>in libs/ansible/src/hooks/useFetch.ts is redundant and inconsistent with put; replace the call withpostData<TRequest, TResponse>(kind, data, serviceUrl, applyHeaders)or drop the explicit generic and cast.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/ocp-plugin/src/components/AppContext/AppContext.tsx(2 hunks)apps/ocp-plugin/src/hooks/useFetch.ts(1 hunks)libs/ansible/src/hooks/useFetch.ts(1 hunks)libs/ansible/src/utils/apiCalls.ts(2 hunks)libs/i18n/locales/en/translation.json(8 hunks)libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx(8 hunks)libs/ui-components/src/components/Fleet/FleetsPage.tsx(2 hunks)libs/ui-components/src/components/SystemRestore/SuspendedDevicesAlert.tsx(1 hunks)libs/ui-components/src/components/form/validations.ts(1 hunks)libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx(1 hunks)libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx(1 hunks)libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/ResumeAllDevicesConfirmationDialog.tsx(1 hunks)libs/ui-components/src/utils/query.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/ui-components/src/utils/query.ts
- apps/ocp-plugin/src/components/AppContext/AppContext.tsx
- libs/ui-components/src/components/form/validations.ts
- libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/ResumeAllDevicesConfirmationDialog.tsx
- apps/ocp-plugin/src/hooks/useFetch.ts
- libs/ui-components/src/components/Fleet/FleetsPage.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-11T10:20:50.496Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#208
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx:30-44
Timestamp: 2025-02-11T10:20:50.496Z
Learning: In the FlightCtl UI, error handling for device decommissioning is implemented in the modal component created by `useDecommissionAction`. Failed PUT requests show an error in the modal with retry/close options.
Applied to files:
libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx
📚 Learning: 2025-02-11T10:24:15.684Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#208
File: libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx:142-150
Timestamp: 2025-02-11T10:24:15.684Z
Learning: In the FlightCtl UI, device tables (`EnrolledDevicesTable` and `DecommissionedDevicesTable`) are swapped based on the `onlyDecommissioned` state, so their respective switches can have hardcoded values (`false` and `true`).
Applied to files:
libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx
🧬 Code graph analysis (6)
libs/ansible/src/utils/apiCalls.ts (3)
libs/types/scripts/openapi-typescript.js (1)
data(23-23)apps/ocp-plugin/src/utils/apiCalls.ts (2)
postData(97-98)putData(100-101)apps/standalone/src/app/utils/apiCalls.ts (2)
postData(128-129)putData(131-132)
libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx (3)
libs/ui-components/src/components/ListPage/ListPageActions.tsx (1)
useResumeListAction(105-145)libs/ui-components/src/hooks/useAccessReview.ts (1)
useAccessReview(8-49)libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx (1)
GlobalSystemRestoreBanners(61-82)
libs/ui-components/src/components/SystemRestore/SuspendedDevicesAlert.tsx (1)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)
libs/ansible/src/hooks/useFetch.ts (1)
libs/ansible/src/utils/apiCalls.ts (2)
postData(44-49)putData(51-56)
libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx (8)
libs/ui-components/src/types/extraTypes.ts (1)
FlightCtlLabel(15-19)libs/ui-components/src/utils/labels.ts (2)
fromAPILabel(7-12)labelToExactApiMatchString(25-25)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)apps/standalone/src/app/hooks/useFetch.ts (1)
useFetch(5-46)libs/ui-components/src/components/Fleet/useFleets.ts (1)
useFleets(87-104)libs/ui-components/src/utils/query.ts (1)
commonQueries(35-99)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/components/form/validations.ts (1)
createMassResumeValidationSchema(699-711)
libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)apps/standalone/src/app/hooks/useFetch.ts (1)
useFetch(5-46)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)
🔇 Additional comments (14)
libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx (5)
103-104: Nice integration of resume action with refetch.Wiring useResumeListAction(refetchDevices) ensures list data is refreshed post‑resume.
133-134: Banners hook-up looks good.GlobalSystemRestoreBanners with onResumeComplete={refetchDevices} aligns with the resume flow.
196-198: Forwarding resume capabilities to rows is correct.Passing canResume and resumeAction keeps row logic simple and consistent with other actions.
116-117: RBAC check for DEVICE_RESUME: constant and client wiring present — confirm backend enforcement
RESOURCE.DEVICE_RESUME is defined (libs/ui-components/src/types/rbac.ts) and consumed via useAccessReview → fetch.checkPermissions (libs/ui-components/src/hooks/useAccessReview.ts, libs/ui-components/src/hooks/useAppContext.tsx); verify the server/API exposes and enforces a 'devices/resume' permission (mapped for VERB.UPDATE) so the resume control isn't incorrectly enabled/disabled.
47-49: Prop API change — refetchDevices already passedDevicesPage passes refetch as refetchDevices to EnrolledDevicesTable (libs/ui-components/src/components/Device/DevicesPage/DevicesPage.tsx:91); no action required.
libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx (1)
210-230: Good UX: primary action gated on validity, count load, and submission.The isSubmitEnabled composition avoids premature submits and double-submits; the confirmation path for “ALL” is a nice touch.
libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx (1)
44-63: Modal UX/readiness is solid.
- Pluralization via count works.
- Primary disabled after completion avoids accidental replays.
- Error surfaced inline with getErrorMessage.
libs/ui-components/src/components/SystemRestore/SuspendedDevicesAlert.tsx (1)
93-100: Action wiring looks good.Primary and extra actions route to the correct modal types and consolidate close handling via onResumeComplete.
libs/ansible/src/utils/apiCalls.ts (4)
20-26: LGTM! Enhanced generics for better type safety.The addition of separate
TRequestandTResponsegeneric parameters allows for different request and response types, which is particularly useful for backup/restore operations where the request payload may differ from the response structure.
44-49: LGTM! Consistent generic pattern implementation.The
postDatafunction now correctly uses the enhanced generic signature with separate request and response types, maintaining consistency with the updatedputOrPostDatafunction.
51-56: LGTM! Appropriate generic constraint for PUT operations.The
putDatafunction correctly uses a single generic parameter since PUT operations typically expect the same type for both request and response, which aligns with REST conventions.
20-56: Verify consumers handle the new generic signaturesImplementations updated in libs/ansible/src/utils/apiCalls.ts, apps/standalone/src/app/utils/apiCalls.ts, and apps/ocp-plugin/src/utils/apiCalls.ts — all define putOrPostData<TRequest, TResponse = TRequest> and matching postData/putData; grep found no residual "" usages. Search the repo for postData( and putData( callers and update any that relied on the old signatures or that must now supply serviceUrl/applyOptions (libs/ansible).
libs/ansible/src/hooks/useFetch.ts (2)
26-30: LGTM! Enhanced post method with proper generic delegation.The updated
postmethod now supports separate request and response types, which is essential for backup/restore operations. The explicit type casting ensures type safety while maintaining backward compatibility.
32-35: LGTM! Consistent PUT method implementation.The
putmethod correctly uses a single generic parameter and delegates to the updatedputDatafunction, maintaining consistency with REST conventions where PUT operations typically return the same type as the request.
celdrake
force-pushed
the
EDM-2060-backup-and-restore-devices
branch
from
36a2e2f to
11c28fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
libs/ansible/src/utils/apiCalls.ts (4)
6-12: Handle 204 No Content to avoid JSON parse errorsDELETE/PUT/POST may return 204;
response.json()will throw. Short‑circuit 204 responses.const handleApiJSONResponse = async <R>(response: Response): Promise<R> => { if (response.ok) { + if (response.status === 204) { + // No Content + return undefined as R; + } const data = (await response.json()) as R; return data; }
27-35: Do not ignore applyOptions return; also include abortSignal in optionsIf
applyOptionsreturns a new object (immutable pattern), ignoring it can drop auth headers. Also wire theabortSignal.- const options: RequestInit = { + let options: RequestInit = { headers: { 'Content-Type': 'application/json', }, method, body: JSON.stringify(data), - }; - applyOptions(options); + signal: abortSignal, + }; + options = applyOptions(options);
64-69: Use applyOptions return value (immutable pattern support) in DELETEAvoid losing auth headers when
applyOptionsreturns a new object.- const options: RequestInit = { + let options: RequestInit = { method: 'DELETE', signal: abortSignal, }; - applyOptions(options); + options = applyOptions(options);
85-94: Use applyOptions return value (immutable pattern support) in PATCHSame reasoning as DELETE/PUT/POST.
- const options: RequestInit = { + let options: RequestInit = { headers: { 'Content-Type': 'application/json-patch+json', }, method: 'PATCH', body: JSON.stringify(data), signal: abortSignal, }; - applyOptions(options); + options = applyOptions(options);
🧹 Nitpick comments (8)
libs/ansible/src/utils/apiCalls.ts (3)
20-26: Thread abortSignal through putOrPostDataParity with GET/DELETE/PATCH improves cancellation and avoids hanging requests.
-const putOrPostData = async <TRequest, TResponse = TRequest>( +const putOrPostData = async <TRequest, TResponse = TRequest>( kind: string, data: TRequest, serviceUrl: string, applyOptions: (options: RequestInit) => RequestInit, - method: 'PUT' | 'POST', -): Promise<TResponse> => { + method: 'PUT' | 'POST', + abortSignal?: AbortSignal, +): Promise<TResponse> => {
44-49: Propagate abortSignal in postDataExpose cancellation to callers and pass it through.
-export const postData = async <TRequest, TResponse = TRequest>( +export const postData = async <TRequest, TResponse = TRequest>( kind: string, data: TRequest, serviceUrl: string, applyOptions: (options: RequestInit) => RequestInit, -): Promise<TResponse> => putOrPostData<TRequest, TResponse>(kind, data, serviceUrl, applyOptions, 'POST'); + abortSignal?: AbortSignal, +): Promise<TResponse> => + putOrPostData<TRequest, TResponse>(kind, data, serviceUrl, applyOptions, 'POST', abortSignal);
51-56: Propagate abortSignal in putDataMirror POST for consistency and cancellation.
-export const putData = async <TRequest>( +export const putData = async <TRequest>( kind: string, data: TRequest, serviceUrl: string, applyOptions: (options: RequestInit) => RequestInit, -): Promise<TRequest> => putOrPostData<TRequest, TRequest>(kind, data, serviceUrl, applyOptions, 'PUT'); + abortSignal?: AbortSignal, +): Promise<TRequest> => + putOrPostData<TRequest, TRequest>(kind, data, serviceUrl, applyOptions, 'PUT', abortSignal);libs/ui-components/src/utils/query.ts (2)
78-88: DRY the repeated suspended status field selector.Extract a shared constant and reuse in both functions.
Apply within these blocks:
- searchParams.set( - 'fieldSelector', - `status.summary.status=${DeviceSummaryStatusType.DeviceSummaryStatusConflictPaused}`, - ); + searchParams.set('fieldSelector', SUSPENDED_FIELD_SELECTOR);Add this once near the imports:
const SUSPENDED_FIELD_SELECTOR = `status.summary.status=${DeviceSummaryStatusType.DeviceSummaryStatusConflictPaused}`;Also applies to: 89-98
78-88: Optional: align with existing builder options pattern.Most builders accept
options?: CommonQueryOptions. Consider supporting it here (defaulting to 1) for consistency and testability.- getSuspendedDeviceCountByLabels: (labels: FlightCtlLabel[]) => { - const searchParams = new URLSearchParams({ - limit: '1', - }); + getSuspendedDeviceCountByLabels: (labels: FlightCtlLabel[], options?: CommonQueryOptions) => { + const searchParams = new URLSearchParams(); + searchParams.set('limit', String(options?.limit ?? 1)); @@ - getAllSuspendedDevicesCount: () => { - const searchParams = new URLSearchParams({ - limit: '1', - }); + getAllSuspendedDevicesCount: (options?: CommonQueryOptions) => { + const searchParams = new URLSearchParams(); + searchParams.set('limit', String(options?.limit ?? 1));Also applies to: 89-98
libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/ResumeAllDevicesConfirmationDialog.tsx (1)
7-13: Remove or use the unusedisSubmittingprop.Currently unused. Either drop it or disable buttons/spinner when provided.
Option A — remove:
-interface ResumeAllDevicesConfirmationModalProps { +interface ResumeAllDevicesConfirmationModalProps { deviceCountNum: number; onClose: (doConfirm: boolean) => void; - isSubmitting?: boolean; } -const ResumeAllDevicesConfirmationModal = ({ deviceCountNum, onClose }: ResumeAllDevicesConfirmationModalProps) => { +const ResumeAllDevicesConfirmationModal = ({ deviceCountNum, onClose }: ResumeAllDevicesConfirmationModalProps) => {Option B — use it:
-const ResumeAllDevicesConfirmationModal = ({ deviceCountNum, onClose }: ResumeAllDevicesConfirmationModalProps) => { +const ResumeAllDevicesConfirmationModal = ({ deviceCountNum, onClose, isSubmitting }: ResumeAllDevicesConfirmationModalProps) => { ... - <Button key="confirm" variant="primary" onClick={() => onClose(true)}> + <Button key="confirm" variant="primary" onClick={() => onClose(true)} isLoading={isSubmitting} isDisabled={isSubmitting}> {t('Resume all devices')} </Button>, - <Button key="cancel" variant="link" onClick={() => onClose(false)}> + <Button key="cancel" variant="link" onClick={() => onClose(false)} isDisabled={isSubmitting}> {t('Cancel')} </Button>,libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx (2)
35-39: Use nullish coalescing to keep 0 values intact.Avoid treating 0 as falsy when assigning.
- setResumedCount(resumeResponse.resumedDevices || 0); + setResumedCount(resumeResponse.resumedDevices ?? 0);
44-63: Returnundefinedon dismiss before submission.The prop type allows
undefined. Distinguish “canceled” from “closed after submitting”.- onClose={() => onClose(hasResumed)} + onClose={() => onClose(resumedCount === undefined ? undefined : hasResumed)} ... - <Button key="cancel" variant="link" onClick={() => onClose(hasResumed)} isDisabled={isSubmitting}> + <Button + key="cancel" + variant="link" + onClick={() => onClose(resumedCount === undefined ? undefined : hasResumed)} + isDisabled={isSubmitting} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/ocp-plugin/src/components/AppContext/AppContext.tsx(2 hunks)apps/ocp-plugin/src/hooks/useFetch.ts(1 hunks)libs/ansible/src/hooks/useFetch.ts(1 hunks)libs/ansible/src/utils/apiCalls.ts(2 hunks)libs/i18n/locales/en/translation.json(8 hunks)libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx(8 hunks)libs/ui-components/src/components/Fleet/FleetsPage.tsx(2 hunks)libs/ui-components/src/components/SystemRestore/SuspendedDevicesAlert.tsx(1 hunks)libs/ui-components/src/components/form/validations.ts(1 hunks)libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx(1 hunks)libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx(1 hunks)libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/ResumeAllDevicesConfirmationDialog.tsx(1 hunks)libs/ui-components/src/utils/query.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/ui-components/src/components/Fleet/FleetsPage.tsx
- apps/ocp-plugin/src/components/AppContext/AppContext.tsx
- libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx
- libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx
- libs/ui-components/src/components/SystemRestore/SuspendedDevicesAlert.tsx
- libs/ansible/src/hooks/useFetch.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-19T08:55:03.335Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:55:03.335Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by validation logic even though TypeScript type definitions might not explicitly show this distinction.
Applied to files:
libs/ui-components/src/utils/query.ts
🧬 Code graph analysis (5)
libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/ResumeAllDevicesConfirmationDialog.tsx (1)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)
libs/ui-components/src/utils/query.ts (3)
libs/ui-components/src/types/extraTypes.ts (1)
FlightCtlLabel(15-19)libs/ui-components/src/utils/labels.ts (1)
labelToExactApiMatchString(25-25)libs/types/index.ts (1)
DeviceSummaryStatusType(59-59)
libs/ansible/src/utils/apiCalls.ts (2)
apps/standalone/src/app/utils/apiCalls.ts (2)
postData(128-129)putData(131-132)apps/ocp-plugin/src/utils/apiCalls.ts (2)
postData(97-98)putData(100-101)
apps/ocp-plugin/src/hooks/useFetch.ts (2)
apps/ocp-plugin/src/utils/apiCalls.ts (3)
postData(97-98)putData(100-101)patchData(118-135)libs/types/models/PatchRequest.ts (1)
PatchRequest(5-18)
libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx (2)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Lint
- GitHub Check: Build ocp plugin
🔇 Additional comments (14)
apps/ocp-plugin/src/hooks/useFetch.ts (3)
45-50: LGTM! Enhanced generics improve type safety for request/response patterns.The updated signature with
TRequestandTResponse = TRequestgenerics provides better type safety for cases where the request and response types differ, while maintaining backward compatibility through the default generic parameter.
52-54: LGTM! Consistent generic typing for PUT operations.The signature change from
<R>to<TRequest>aligns with the updatedputDatafunction signature and maintains type consistency across the codebase.
62-63: LGTM! Parameter rename improves clarity.Renaming the parameter from
objtopatchesbetter reflects the nature of the JSON Patch operations being performed, making the code more self-documenting.libs/ansible/src/utils/apiCalls.ts (1)
44-56: Approve: generics refactor for POST/PUTReturn typing (TResponse = TRequest) matches the repo pattern; inspected call sites (apps/ocp-plugin, apps/standalone, libs/ansible) are compatible — no mismatched expectations found.
libs/ui-components/src/utils/query.ts (3)
3-3: DeviceSummaryStatusType is a string enum — no change required. DeviceSummaryStatusType is declared with string values (e.g. DeviceSummaryStatusConflictPaused = 'ConflictPaused' in libs/types/models/DeviceSummaryStatusType.ts), so the fieldSelector usage in libs/ui-components/src/utils/query.ts will send the expected string token.
89-98: 'Suspended' == DeviceSummaryStatusConflictPaused — keep single-value selectorOnly DeviceSummaryStatusType.DeviceSummaryStatusConflictPaused is used to represent "Suspended" in this codebase (see enum and usages), and UI/actions (isDeviceResumable, SystemRestoreBanners, MassResumeDevicesModal) expect/resume ConflictPaused devices, so the existing fieldSelector=status.summary.status=DeviceSummaryStatusConflictPaused is correct. See: libs/types/models/DeviceSummaryStatusType.ts, libs/ui-components/src/utils/devices.ts, libs/ui-components/src/components/SystemRestore/SystemRestoreBanners.tsx, libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx, and libs/ui-components/src/utils/query.ts.
78-88: Confirmed — limit=1 is compatible with how totals are readgetApiListCount (libs/ui-components/src/utils/api.ts) computes totals from response.metadata.remainingItemCount; query builders in libs/ui-components/src/utils/query.ts set limit=1 and callers (e.g., libs/ui-components/src/components/modals/massModals/ResumeDevicesModal/MassResumeDevicesModal.tsx and libs/ui-components/src/hooks/usePendingEnrollmentRequestsCount.ts) use getApiListCount — no header-based total extraction found.
libs/i18n/locales/en/translation.json (5)
630-647: LGTM: resume flow count/status messages.Keys cover the mass-resume UX (counts, loading states, and errors) and match component usage.
648-671: LGTM: pluralization keys for confirmation/success/warning.use of
_one/_otheraligns with i18next pluralization; values read correctly.Verify that components using these strings pass
counttot/Transand provide the needed components array for the indexed tags (e.g.,<1>).
569-571: LGTM: validation error messages added.These align with the new Yup schema in validations.ts.
93-95: LGTM: device-level resume prompts.Wording and variable placeholders are consistent.
Also applies to: 127-127, 574-574
842-844: Confirm status label mapping.Ensure these labels map to actual status enums/values used in UI filters and chips.
libs/ui-components/src/components/form/validations.ts (1)
699-711: LGTM: Yup conditional schema for mass resume.
- Correct function-form
.when()usage.- Proper conditional requirements for
fleetIdandlabels.libs/ui-components/src/components/modals/ResumeDevicesModal/ResumeDevicesModal.tsx (1)
28-28: Verify intentional plural forcing for fleet mode.Using
pluralCount = 2for fleets guarantees plural text even whenexpectedCount === 1. Confirm this is desired.
2 participants
Backup and restore devices working both for RHEM and RHEM for ACM.
We're missing some design decisions, so there may be related enhancements in the future.
For example, we need to know what is displayed for users that don't have permission to resume devices.
Summary by CodeRabbit
New Features
Enhancements
Localization