feat: approvals page #310
Open
feat: approvals page #310
Conversation
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
WalkthroughA new dedicated approvals dashboard page is introduced to handle device approval/rejection workflows. The main dashboard is simplified by removing its inline pending approval panel. The layout header gains a notification badge displaying pending approvals count, linking to the new approvals page. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApprovalsPage
participant QueryClient
participant useGetDevices
participant useApproveDevice
participant useRevokeDevice
participant API
User->>ApprovalsPage: Visit /approvals
ApprovalsPage->>useGetDevices: Fetch unapproved devices (refetch: 5s)
useGetDevices->>API: GET /devices?approved=false
API-->>useGetDevices: Return device list
useGetDevices-->>ApprovalsPage: Display devices in list
User->>ApprovalsPage: Click Approve button
ApprovalsPage->>ApprovalsPage: Set processing indicator
ApprovalsPage->>useApproveDevice: Trigger approval
useApproveDevice->>API: POST /devices/{id}/approve
API-->>useApproveDevice: Success response
useApproveDevice-->>ApprovalsPage: Approval complete
ApprovalsPage->>QueryClient: Invalidate unapproved devices query
QueryClient->>useGetDevices: Refetch devices
useGetDevices->>API: GET /devices?approved=false
API-->>useGetDevices: Updated device list
ApprovalsPage->>ApprovalsPage: Show success toast (3s auto-dismiss)
User->>ApprovalsPage: Click Reject button
ApprovalsPage->>ApprovalsPage: Show confirmation prompt
User->>ApprovalsPage: Confirm rejection
ApprovalsPage->>ApprovalsPage: Set processing indicator
ApprovalsPage->>useRevokeDevice: Trigger rejection
useRevokeDevice->>API: POST /devices/{id}/revoke
API-->>useRevokeDevice: Success response
useRevokeDevice-->>ApprovalsPage: Rejection complete
ApprovalsPage->>QueryClient: Invalidate unapproved devices query
QueryClient->>useGetDevices: Refetch devices
useGetDevices->>API: GET /devices?approved=false
API-->>useGetDevices: Updated device list
ApprovalsPage->>ApprovalsPage: Show success toast (3s auto-dismiss)
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@dashboard/app/`(private)/approvals/page.tsx:
- Around line 63-91: The handleApprove function currently calls
approveDeviceHook.mutateAsync without error handling; wrap the mutateAsync call
in a try/catch/finally so network/server errors don’t cause unhandled promise
rejections and so processing state is always cleaned up. Specifically, in
handleApprove: add try { const success = await approveDeviceHook.mutateAsync({
deviceId }); ... } catch (err) { setToast({ message: `Failed to approve
${deviceName}: ${err?.message || "unknown error"}`, type: "error" }); /*
optionally log error */ } finally { remove deviceId from processingDevices (use
the existing setProcessingDevices logic) } and ensure
queryClient.invalidateQueries and the success toast remain inside the try when
success is truthy.
- Around line 93-129: The handleReject function must mirror handleApprove's
error handling: wrap the async revokeDeviceHook.mutateAsync call and subsequent
logic in try/catch/finally so errors are caught and a failure toast is shown in
the catch, while queryClient.invalidateQueries({ queryKey:
unapprovedDevices.queryKey }) and the success toast remain inside the try when
mutateAsync returns success; always remove the deviceId from
setProcessingDevices in finally to avoid stuck processing state. Reference:
handleReject, revokeDeviceHook.mutateAsync, setProcessingDevices,
queryClient.invalidateQueries, unapprovedDevices.queryKey.
🧹 Nitpick comments (1)
dashboard/app/(private)/approvals/page.tsx (1)
46-57: Duplicate helper function — consider extracting to a shared utility.
formatTimeAgois duplicated fromdashboard/page.tsx(lines 60-71). Extract this to a shared utility file (e.g.,@/app/utils/date.ts) to avoid drift and reduce maintenance burden.
Comment on lines
+63
to
+91
| const handleApprove = async (deviceId: number, e: React.MouseEvent) => { | ||
| e.stopPropagation(); | ||
|
|
||
| const device = unapprovedDevices.data?.find((d) => d.id === deviceId); | ||
| const deviceName = device?.serial_number || "Device"; | ||
|
|
||
| setProcessingDevices((prev) => new Set(prev).add(deviceId)); | ||
|
|
||
| const success = await approveDeviceHook.mutateAsync({ deviceId }); | ||
|
|
||
| if (success) { | ||
| queryClient.invalidateQueries({ queryKey: unapprovedDevices.queryKey }); | ||
| setToast({ | ||
| message: `${deviceName} approved successfully`, | ||
| type: "success", | ||
| }); | ||
| } else { | ||
| setToast({ | ||
| message: `Failed to approve ${deviceName}`, | ||
| type: "error", | ||
| }); | ||
| } | ||
|
|
||
| setProcessingDevices((prev) => { | ||
| const newSet = new Set(prev); | ||
| newSet.delete(deviceId); | ||
| return newSet; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Missing error handling around mutateAsync — unhandled promise rejection on network/server errors.
mutateAsync throws when the mutation fails (network error, server error, etc.). Without a try/catch, this will result in an unhandled promise rejection, leaving processingDevices in an inconsistent state and potentially crashing the component.
🐛 Proposed fix: wrap in try/catch
const handleApprove = async (deviceId: number, e: React.MouseEvent) => {
e.stopPropagation();
const device = unapprovedDevices.data?.find((d) => d.id === deviceId);
const deviceName = device?.serial_number || "Device";
setProcessingDevices((prev) => new Set(prev).add(deviceId));
+ try {
const success = await approveDeviceHook.mutateAsync({ deviceId });
if (success) {
queryClient.invalidateQueries({ queryKey: unapprovedDevices.queryKey });
setToast({
message: `${deviceName} approved successfully`,
type: "success",
});
} else {
setToast({
message: `Failed to approve ${deviceName}`,
type: "error",
});
}
+ } catch (error) {
+ setToast({
+ message: `Failed to approve ${deviceName}`,
+ type: "error",
+ });
+ } finally {
+ setProcessingDevices((prev) => {
+ const newSet = new Set(prev);
+ newSet.delete(deviceId);
+ return newSet;
+ });
+ }
-
- setProcessingDevices((prev) => {
- const newSet = new Set(prev);
- newSet.delete(deviceId);
- return newSet;
- });
};📝 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
| const handleApprove = async (deviceId: number, e: React.MouseEvent) => { | |
| e.stopPropagation(); | |
| const device = unapprovedDevices.data?.find((d) => d.id === deviceId); | |
| const deviceName = device?.serial_number || "Device"; | |
| setProcessingDevices((prev) => new Set(prev).add(deviceId)); | |
| const success = await approveDeviceHook.mutateAsync({ deviceId }); | |
| if (success) { | |
| queryClient.invalidateQueries({ queryKey: unapprovedDevices.queryKey }); | |
| setToast({ | |
| message: `${deviceName} approved successfully`, | |
| type: "success", | |
| }); | |
| } else { | |
| setToast({ | |
| message: `Failed to approve ${deviceName}`, | |
| type: "error", | |
| }); | |
| } | |
| setProcessingDevices((prev) => { | |
| const newSet = new Set(prev); | |
| newSet.delete(deviceId); | |
| return newSet; | |
| }); | |
| }; | |
| const handleApprove = async (deviceId: number, e: React.MouseEvent) => { | |
| e.stopPropagation(); | |
| const device = unapprovedDevices.data?.find((d) => d.id === deviceId); | |
| const deviceName = device?.serial_number || "Device"; | |
| setProcessingDevices((prev) => new Set(prev).add(deviceId)); | |
| try { | |
| const success = await approveDeviceHook.mutateAsync({ deviceId }); | |
| if (success) { | |
| queryClient.invalidateQueries({ queryKey: unapprovedDevices.queryKey }); | |
| setToast({ | |
| message: `${deviceName} approved successfully`, | |
| type: "success", | |
| }); | |
| } else { | |
| setToast({ | |
| message: `Failed to approve ${deviceName}`, | |
| type: "error", | |
| }); | |
| } | |
| } catch (error) { | |
| setToast({ | |
| message: `Failed to approve ${deviceName}`, | |
| type: "error", | |
| }); | |
| } finally { | |
| setProcessingDevices((prev) => { | |
| const newSet = new Set(prev); | |
| newSet.delete(deviceId); | |
| return newSet; | |
| }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@dashboard/app/`(private)/approvals/page.tsx around lines 63 - 91, The
handleApprove function currently calls approveDeviceHook.mutateAsync without
error handling; wrap the mutateAsync call in a try/catch/finally so
network/server errors don’t cause unhandled promise rejections and so processing
state is always cleaned up. Specifically, in handleApprove: add try { const
success = await approveDeviceHook.mutateAsync({ deviceId }); ... } catch (err) {
setToast({ message: `Failed to approve ${deviceName}: ${err?.message || "unknown
error"}`, type: "error" }); /* optionally log error */ } finally { remove
deviceId from processingDevices (use the existing setProcessingDevices logic) }
and ensure queryClient.invalidateQueries and the success toast remain inside the
try when success is truthy.
Comment on lines
+93
to
+129
| const handleReject = async (deviceId: number, e: React.MouseEvent) => { | ||
| e.stopPropagation(); | ||
|
|
||
| const device = unapprovedDevices.data?.find((d) => d.id === deviceId); | ||
| const deviceName = device?.serial_number || "Device"; | ||
|
|
||
| if ( | ||
| !confirm( | ||
| `Are you sure you want to reject ${deviceName}? This will archive it.`, | ||
| ) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| setProcessingDevices((prev) => new Set(prev).add(deviceId)); | ||
|
|
||
| const success = await revokeDeviceHook.mutateAsync({ deviceId }); | ||
|
|
||
| if (success) { | ||
| queryClient.invalidateQueries({ queryKey: unapprovedDevices.queryKey }); | ||
| setToast({ | ||
| message: `${deviceName} rejected and archived`, | ||
| type: "success", | ||
| }); | ||
| } else { | ||
| setToast({ | ||
| message: `Failed to reject ${deviceName}`, | ||
| type: "error", | ||
| }); | ||
| } | ||
|
|
||
| setProcessingDevices((prev) => { | ||
| const newSet = new Set(prev); | ||
| newSet.delete(deviceId); | ||
| return newSet; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Same error handling issue as handleApprove — wrap in try/catch/finally.
Apply the same fix pattern here. Additionally, consider replacing the native confirm() with a custom modal component for consistency with the app's design system, though this is a lower priority.
🐛 Proposed fix
const handleReject = async (deviceId: number, e: React.MouseEvent) => {
e.stopPropagation();
const device = unapprovedDevices.data?.find((d) => d.id === deviceId);
const deviceName = device?.serial_number || "Device";
if (
!confirm(
`Are you sure you want to reject ${deviceName}? This will archive it.`,
)
) {
return;
}
setProcessingDevices((prev) => new Set(prev).add(deviceId));
+ try {
const success = await revokeDeviceHook.mutateAsync({ deviceId });
if (success) {
queryClient.invalidateQueries({ queryKey: unapprovedDevices.queryKey });
setToast({
message: `${deviceName} rejected and archived`,
type: "success",
});
} else {
setToast({
message: `Failed to reject ${deviceName}`,
type: "error",
});
}
+ } catch (error) {
+ setToast({
+ message: `Failed to reject ${deviceName}`,
+ type: "error",
+ });
+ } finally {
+ setProcessingDevices((prev) => {
+ const newSet = new Set(prev);
+ newSet.delete(deviceId);
+ return newSet;
+ });
+ }
-
- setProcessingDevices((prev) => {
- const newSet = new Set(prev);
- newSet.delete(deviceId);
- return newSet;
- });
};📝 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
| const handleReject = async (deviceId: number, e: React.MouseEvent) => { | |
| e.stopPropagation(); | |
| const device = unapprovedDevices.data?.find((d) => d.id === deviceId); | |
| const deviceName = device?.serial_number || "Device"; | |
| if ( | |
| !confirm( | |
| `Are you sure you want to reject ${deviceName}? This will archive it.`, | |
| ) | |
| ) { | |
| return; | |
| } | |
| setProcessingDevices((prev) => new Set(prev).add(deviceId)); | |
| const success = await revokeDeviceHook.mutateAsync({ deviceId }); | |
| if (success) { | |
| queryClient.invalidateQueries({ queryKey: unapprovedDevices.queryKey }); | |
| setToast({ | |
| message: `${deviceName} rejected and archived`, | |
| type: "success", | |
| }); | |
| } else { | |
| setToast({ | |
| message: `Failed to reject ${deviceName}`, | |
| type: "error", | |
| }); | |
| } | |
| setProcessingDevices((prev) => { | |
| const newSet = new Set(prev); | |
| newSet.delete(deviceId); | |
| return newSet; | |
| }); | |
| }; | |
| const handleReject = async (deviceId: number, e: React.MouseEvent) => { | |
| e.stopPropagation(); | |
| const device = unapprovedDevices.data?.find((d) => d.id === deviceId); | |
| const deviceName = device?.serial_number || "Device"; | |
| if ( | |
| !confirm( | |
| `Are you sure you want to reject ${deviceName}? This will archive it.`, | |
| ) | |
| ) { | |
| return; | |
| } | |
| setProcessingDevices((prev) => new Set(prev).add(deviceId)); | |
| try { | |
| const success = await revokeDeviceHook.mutateAsync({ deviceId }); | |
| if (success) { | |
| queryClient.invalidateQueries({ queryKey: unapprovedDevices.queryKey }); | |
| setToast({ | |
| message: `${deviceName} rejected and archived`, | |
| type: "success", | |
| }); | |
| } else { | |
| setToast({ | |
| message: `Failed to reject ${deviceName}`, | |
| type: "error", | |
| }); | |
| } | |
| } catch (error) { | |
| setToast({ | |
| message: `Failed to reject ${deviceName}`, | |
| type: "error", | |
| }); | |
| } finally { | |
| setProcessingDevices((prev) => { | |
| const newSet = new Set(prev); | |
| newSet.delete(deviceId); | |
| return newSet; | |
| }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@dashboard/app/`(private)/approvals/page.tsx around lines 93 - 129, The
handleReject function must mirror handleApprove's error handling: wrap the async
revokeDeviceHook.mutateAsync call and subsequent logic in try/catch/finally so
errors are caught and a failure toast is shown in the catch, while
queryClient.invalidateQueries({ queryKey: unapprovedDevices.queryKey }) and the
success toast remain inside the try when mutateAsync returns success; always
remove the deviceId from setProcessingDevices in finally to avoid stuck
processing state. Reference: handleReject, revokeDeviceHook.mutateAsync,
setProcessingDevices, queryClient.invalidateQueries, unapprovedDevices.queryKey.
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.
No description provided.