Refactor use devices hook mutations to support info banner system#3574
Conversation
…port-InfoBanner-system
…ity across components
|
Warning Review limit reached
More reviews will be available in 18 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors device/shipping mutation hooks to accept optional onSuccess/onError callbacks, centralizes clipboard copying in a new useClipboard hook, and updates multiple UI components (devices, grids, cohorts, shipping) to show banner-based feedback instead of ReusableToast. ChangesBanner System Migration for Device Mutations and Clipboard
Sequence Diagram(s)sequenceDiagram
participant PrepareModal as PrepareShippingModal
participant ShippingPage as ShippingBatchPage
participant PrepareHook as usePrepareBulkDevicesForShipping
participant LabelsHook as useGenerateShippingLabels
PrepareModal->>PrepareHook: mutate(importData)
PrepareHook-->>PrepareModal: onSuccess / onError (banner)
ShippingPage->>LabelsHook: mutate(selectedDeviceNames)
LabelsHook-->>ShippingPage: onSuccess (banner + open print modal) / onError (banner)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
|
New azure docs changes available for preview here |
|
New azure vertex changes available for preview here |
|
New azure analytics_platform changes available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/vertex/components/features/devices/device-activity-item.tsx (1)
6-6: ⚡ Quick winConsider using the
useClipboardhook to reduce duplication.This component reimplements the same clipboard-copy-with-banner-feedback pattern that was extracted to the
useClipboardhook (introduced in this PR).device-measurements-api-card.tsxalready uses the hook (line 12), and the PR summary mentions addinguseClipboardto reduce duplication.♻️ Refactor to use useClipboard
-import { useBanner } from '`@/context/banner-context`'; +import { useClipboard } from '`@/core/hooks/useClipboard`';- const { showBanner } = useBanner(); + const { handleCopy } = useClipboard();<ReusableButton variant="text" - onClick={async () => { - try { - await navigator.clipboard.writeText(previousSiteId); - showBanner({ severity: 'success', message: 'Copied', scoped: false }); - } catch { - showBanner({ severity: 'error', message: 'Failed to copy', scoped: false }); - } - }} + onClick={() => handleCopy(previousSiteId)} className="p-1" Icon={AqCopy01} aria-label="Copy site ID" />Also applies to: 31-31, 109-123
🤖 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 `@src/vertex/components/features/devices/device-activity-item.tsx` at line 6, This component duplicates clipboard-copy + banner feedback logic; refactor DeviceActivityItem to use the existing useClipboard hook instead of manually calling navigator.clipboard and useBanner: import and call useClipboard (the hook already used in device-measurements-api-card) inside the DeviceActivityItem component, replace the manual copy handlers (the duplicated logic around lines 31 and 109-123) with the hook's copy function and use its success/failure callbacks to trigger banners as needed, and remove the now-unused navigator.clipboard and banner-specific code (and any unused useBanner import) so the component delegates copying and feedback to useClipboard.
🤖 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 `@src/vertex/core/hooks/useDevices.ts`:
- Around line 484-501: The hook useUpdateDeviceGroup dropped query invalidation;
restore it by injecting React Query's QueryClient (useQueryClient) and calling
queryClient.invalidateQueries(...) in the onSuccess handler (in addition to
options?.onSuccess?.()), invalidating the same query keys that
useUpdateDeviceBulk invalidates (e.g., device list and affected device detail
queries) so callers of useUpdateDeviceGroup no longer need to manually refresh;
keep mutationFn as devices.bulkUpdateDeviceDetails([...]) and ensure
options?.onError?.(error) remains unchanged.
---
Nitpick comments:
In `@src/vertex/components/features/devices/device-activity-item.tsx`:
- Line 6: This component duplicates clipboard-copy + banner feedback logic;
refactor DeviceActivityItem to use the existing useClipboard hook instead of
manually calling navigator.clipboard and useBanner: import and call useClipboard
(the hook already used in device-measurements-api-card) inside the
DeviceActivityItem component, replace the manual copy handlers (the duplicated
logic around lines 31 and 109-123) with the hook's copy function and use its
success/failure callbacks to trigger banners as needed, and remove the
now-unused navigator.clipboard and banner-specific code (and any unused
useBanner import) so the component delegates copying and feedback to
useClipboard.
🪄 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: Pro
Run ID: 9728e814-6b56-416d-bafa-791b89573bef
📒 Files selected for processing (8)
src/vertex/app/(authenticated)/admin/shipping/[batchId]/page.tsxsrc/vertex/components/features/devices/bulk-edit-device-details-modal.tsxsrc/vertex/components/features/devices/device-activity-item.tsxsrc/vertex/components/features/devices/device-measurements-api-card.tsxsrc/vertex/components/features/devices/orphaned-devices-alert.tsxsrc/vertex/components/features/shipping/PrepareShippingModal.tsxsrc/vertex/core/hooks/useClipboard.tssrc/vertex/core/hooks/useDevices.ts
|
New azure docs changes available for preview here |
|
New azure analytics_platform changes available for preview here |
|
New azure vertex changes available for preview here |
|
@copilot review |
|
New azure docs changes available for preview here |
|
New azure analytics_platform changes available for preview here |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codebmk
left a comment
Codebmk
left a comment
There was a problem hiding this comment.
kindly update changelog accordingly. @BwanikaRobert
|
New azure docs changes available for preview here |
|
New azure analytics_platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/vertex/components/features/grids/grid-details-card.tsx`:
- Around line 18-23: The hook call order error is caused by calling useClipboard
after an early return when loading is true; always call hooks unconditionally.
Move the useClipboard call (const { handleCopy } = useClipboard(...)) to the top
of the GridDetailsCard component (before checking the loading flag) so that
useClipboard is invoked on every render, then keep the early return UI for
loading unchanged.
In `@src/vertex/components/features/grids/grid-measurements-api-card.tsx`:
- Around line 16-21: The hook useClipboard is conditionally skipped when the
component returns early for loading; move the call to useClipboard (the const {
handleCopy } = useClipboard({...}) line) so it executes unconditionally before
the if (loading) return branch to preserve hook order across renders and then
keep the existing early return that renders the Loader2 Card when loading is
true.
🪄 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: Pro
Run ID: 9414b08b-f7cd-4e5a-ada6-1dcac660d55f
📒 Files selected for processing (8)
src/vertex/app/changelog.mdsrc/vertex/components/features/cohorts/cohort-measurements-api-card.tsxsrc/vertex/components/features/devices/device-activity-item.tsxsrc/vertex/components/features/grids/admin-levels-modal.tsxsrc/vertex/components/features/grids/grid-details-card.tsxsrc/vertex/components/features/grids/grid-measurements-api-card.tsxsrc/vertex/core/hooks/useClipboard.tssrc/vertex/core/hooks/useDevices.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/vertex/core/hooks/useDevices.ts
|
New azure docs changes available for preview here |
|
New azure analytics_platform changes available for preview here |
|
New azure vertex changes available for preview here |
Codebmk
left a comment
Codebmk
left a comment
There was a problem hiding this comment.
Amazing, thanks @BwanikaRobert
3 participants
Summary of Changes (What does this PR do?)
Decouples all remaining
ReusableToastcalls from mutation hooks inuseDevices.tsand delegates notification responsibility to the calling UI layer via theuseBanner/useBannerWithDelaysystem.Hooks decoupled (removed ReusableToast, added optional onSuccess/onError interfaces):
useClaimDevice,useBulkClaimDevices,useUnassignDeviceFromOrganization,useUpdateDeviceBulk,useUpdateDeviceGroup,usePrepareDeviceForShipping,usePrepareBulkDevicesForShipping,useGenerateShippingLabelsComponents updated in this PR:
bulk-edit-device-details-modal.tsx— wired hook-level callbacks; error usesscoped: trueinline, success usesshowBannerWithDelay(scoped: false) after dialog closes.PrepareShippingModal.tsx— replaced all 8ReusableToastcalls; file/import validation errors usescoped: true, bulk preparation success usesshowBannerWithDelay(scoped: false).shipping/[batchId]/page.tsx— removedReusableToast; wireduseGenerateShippingLabelswith hook-level callbacks and replaced inline validation toasts withshowBanner(scoped: false).Components already migrated in PR feat(vertex/devices): migrate device management toasts to InfoBanner … #3503 (no changes needed):
create-device-modal.tsx✓import-device-modal.tsx✓deploy-device-component.tsx✓recall-device-dialog.tsx✓add-maintenance-log-modal.tsx✓Not implemented because files in the task spec do not exist:
claim-devices-dialog.tsx,claim-single-device.tsx— actual file isclaim-device-modal.tsx, which uses an internal multi-step state machine (steptransitions +setError) for feedback. Adding banners would duplicate already-handled UX. Hook is decoupled; banner wiring can be added if a simpler claim component is built.create-device-dialog.tsx,import-devices-dialog.tsx,recall-device-component.tsx,maintenance-log-dialog.tsx— none of these paths exist; their counterparts were already migrated in PR feat(vertex/devices): migrate device management toasts to InfoBanner … #3503.Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation