Skip to content

fix(audit-sprint-4c): modal primitives (Phase 1) + 2 proof migrations #167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ejay-dev merged 1 commit into from
May 23, 2026
Merged

fix(audit-sprint-4c): modal primitives (Phase 1) + 2 proof migrations #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/app/layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { CommandPalette } from '@/components/command-palette/CommandPalette';
import { HelpAssistant } from '@/components/help/HelpAssistant';
import { AiAssistant } from '@/components/ai-assistant/AiAssistant';
import { NotificationToast } from '@/components/notifications/notification-toast';
import { Toaster } from '@/components/ui/toaster';
import { OnboardingWizard } from '@/components/onboarding/OnboardingWizard';
import { recoverUserWorkspace } from '@/lib/provisioning/workspace-recovery';
import { SecurityTrackingBootstrap } from '@/components/security/SecurityTrackingBootstrap';
Expand Down Expand Up @@ -278,6 +279,11 @@ export default async function AppLayout({
userId={systemState.user.id}
orgId={systemState.organization.id}
/>
{/* Audit 2026-05-23: Sprint 4c Phase 1 — shared sonner toast
surface mounted once at the root. New code should reach for
`toast()` from `@/components/ui/toaster` instead of rolling
another in-house implementation. */}
<Toaster />
<SecurityTrackingBootstrap />
<FeedbackWidget />
<RuntimeDebugIndicator />
Expand Down
50 changes: 47 additions & 3 deletions components/comments/comments-section.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ import {
Reply,
} from 'lucide-react';
import { z } from 'zod';
import {
AlertDialog,
AlertDialogAction,
AlertDialogCancel,
AlertDialogContent,
AlertDialogDescription,
AlertDialogFooter,
AlertDialogHeader,
AlertDialogTitle,
} from '@/components/ui/alert-dialog';

const commentSchema = z.object({
content: z.string().min(1, 'Comment cannot be empty').max(5000, 'Comment must be under 5000 characters'),
Expand Down Expand Up @@ -62,6 +72,10 @@ export default function Comments({
const [editingId, setEditingId] = useState<string | null>(null);
const [editContent, setEditContent] = useState('');
const [loading, setLoading] = useState(true);
// Audit 2026-05-23: replaced window.confirm() with AlertDialog. Holding
// the pending-delete id in state lets the dialog open + close cleanly
// and keeps the focus trap / aria semantics the browser confirm lacked.
const [pendingDeleteId, setPendingDeleteId] = useState<string | null>(null);

useEffect(() => {
fetchComments();
Expand Down Expand Up @@ -127,8 +141,6 @@ export default function Comments({
};

const handleDelete = async (commentId: string) => {
if (!confirm('Delete this comment?')) return;

try {
await fetch(`/api/comments/${commentId}`, {
method: 'DELETE',
Expand All @@ -137,6 +149,8 @@ export default function Comments({
await fetchComments();
} catch (error) {
console.error('Failed to delete comment:', error);
} finally {
setPendingDeleteId(null);
}
Comment on lines +152 to 154

@coderabbitai coderabbitai Bot May 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Guard pendingDeleteId cleanup to avoid clobbering a newer delete intent.

The unconditional reset in finally can close a newly opened confirmation if a previous delete request finishes later. Reset only when the same commentId is still pending.

Suggested fix 
-    } finally {
-      setPendingDeleteId(null);
-    }
+    } finally {
+      setPendingDeleteId((current) =>
+        current === commentId ? null : current
+      );
+    }
📝 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
} finally {
setPendingDeleteId(null);
}
} finally {
setPendingDeleteId((current) =>
current === commentId ? null : current
);
}
🤖 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 `@components/comments/comments-section.tsx` around lines 152 - 154, The
unconditional setPendingDeleteId(null) in the finally block can clobber a newer
delete intent; change the cleanup so you only clear the pending ID if it still
matches the commentId that started this request (i.e., replace the unconditional
call with a guarded check like if (pendingDeleteId === commentId)
setPendingDeleteId(null)), referencing the pendingDeleteId state and the
commentId/handler that initiated the delete so you don't close a newer
confirmation opened after this request began.

};

Expand Down Expand Up @@ -224,8 +238,9 @@ export default function Comments({
<Edit2 className="h-3 w-3 text-gray-500" />
</button>
<button
onClick={() => handleDelete(comment.id)}
onClick={() => setPendingDeleteId(comment.id)}
className="p-1 hover:bg-gray-200 rounded"
aria-label="Delete comment"
>
Comment on lines 240 to 244
<Trash2 className="h-3 w-3 text-gray-500" />
</button>
Expand Down Expand Up @@ -400,6 +415,35 @@ export default function Comments({
comments.map((comment) => renderComment(comment))
)}
</div>

<AlertDialog
open={pendingDeleteId !== null}
onOpenChange={(open) => {
if (!open) setPendingDeleteId(null);
}}
>
<AlertDialogContent>
<AlertDialogHeader>
<AlertDialogTitle>Delete this comment?</AlertDialogTitle>
<AlertDialogDescription>
The comment will be removed for everyone. This action is
recorded in the audit log.
</AlertDialogDescription>
</AlertDialogHeader>
<AlertDialogFooter>
<AlertDialogCancel>Cancel</AlertDialogCancel>
<AlertDialogAction
onClick={() => {
if (pendingDeleteId) {
void handleDelete(pendingDeleteId);
}
}}
>
Delete comment
</AlertDialogAction>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
</div>
);
}
132 changes: 74 additions & 58 deletions components/delete-button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,31 @@
import { useState } from "react"
import { useRouter } from "next/navigation"
import { createSupabaseClient } from "@/lib/supabase/client"
import { Trash2, Loader2, AlertTriangle, X } from "lucide-react"
import { Trash2, Loader2 } from "lucide-react"
import { logActivity } from "@/lib/actions/audit"
import { useComplianceAction, type NodeType } from "@/components/compliance-system"
import {
AlertDialog,
AlertDialogAction,
AlertDialogCancel,
AlertDialogContent,
AlertDialogDescription,
AlertDialogFooter,
AlertDialogHeader,
AlertDialogTitle,
AlertDialogTrigger,
} from "@/components/ui/alert-dialog"

/**
* =========================================================
* DELETE BUTTON
* Action: Removes node from compliance graph
* Shows system impact warning before deletion
* Action: Removes node from compliance graph.
*
* Audit 2026-05-23 (Sprint 4c Phase 1): migrated from inline
* confirm card to AlertDialog. The previous implementation had
* no focus trap, no ESC handler, no aria-modal, and rendered
* confirmation as a sibling element. AlertDialog gives all of
* those for free.
* =========================================================
*/

Expand All @@ -22,7 +38,6 @@ type DeleteButtonProps = {
onDelete?: () => void
}

// Map table names to node types
const tableToNodeType: Record<string, NodeType> = {
'org_policies': 'policy',
'org_registers': 'control',
Expand All @@ -37,88 +52,89 @@ const tableToLabel: Record<string, string> = {

export function DeleteButton({ id, tableName, itemTitle, onDelete }: DeleteButtonProps) {
const [loading, setLoading] = useState(false)
const [confirm, setConfirm] = useState(false)
const [open, setOpen] = useState(false)
const router = useRouter()
const { nodeDeleted, reportError } = useComplianceAction()

const nodeType = tableToNodeType[tableName] || 'entity'
const nodeLabel = tableToLabel[tableName] || 'Record'
const displayTitle = itemTitle || `${nodeLabel} ${id.slice(0, 8)}`

async function handleDelete() {
async function handleDelete(event: React.MouseEvent) {
// Stop the AlertDialog default close so the spinner has time to render.
event.preventDefault()
setLoading(true)

// 1. Execute the deletion

const supabase = createSupabaseClient()
const { error } = await supabase.from(tableName).delete().eq('id', id)

if (error) {
reportError({ title: "Delete failed", message: `Failed to delete ${nodeLabel.toLowerCase()}: ${error.message}` })
reportError({
title: "Delete failed",
message: `Failed to delete ${nodeLabel.toLowerCase()}: ${error.message}`,
})
setLoading(false)
setConfirm(false)
return
}

// 2. Trigger the Automated Audit Log
const recordType = tableName.split('_')[1].replace(/s$/, '').toUpperCase();


const recordType = tableName.split('_')[1].replace(/s$/, '').toUpperCase()

try {
await logActivity({
type: `${recordType}_DELETE`,
description: `User permanently deleted ${recordType.toLowerCase()} record: ${id.slice(0, 8)}`,
metadata: { record_id: id, source_table: tableName }
});
metadata: { record_id: id, source_table: tableName },
})
} catch (auditError) {
console.error("Audit log failed, but record was deleted:", auditError);
console.error("Audit log failed, but record was deleted:", auditError)
}

// 3. Report to compliance system
nodeDeleted(nodeType, itemTitle || `${nodeLabel} ${id.slice(0, 8)}`)
nodeDeleted(nodeType, displayTitle)

// 4. Update UI
setConfirm(false)
setLoading(false)
setOpen(false)
if (onDelete) onDelete()
router.refresh()
}

if (confirm) {
return (
<div className="flex items-center gap-2 bg-rose-500/10 px-3 py-1.5 rounded-lg border border-rose-500/30 animate-in fade-in slide-in-from-right-2 duration-200">
<AlertTriangle className="h-3.5 w-3.5 text-rose-400 shrink-0" />
<span className="text-xs font-medium text-rose-300 hidden sm:inline">
This will disconnect all linked nodes
</span>
<button
onClick={() => setConfirm(false)}
className="p-1 text-muted-foreground hover:text-foreground/90 hover:bg-glass-strong rounded transition-colors"
title="Cancel"
>
<X className="h-3.5 w-3.5" />
</button>
<div className="h-4 w-px bg-rose-500/30" />
<button
onClick={handleDelete}
disabled={loading}
className="flex items-center gap-1.5 px-2 py-1 text-xs font-bold uppercase text-rose-300 hover:text-white hover:bg-rose-500/20 rounded transition-all disabled:opacity-50"
return (
<AlertDialog open={open} onOpenChange={setOpen}>
<AlertDialogTrigger asChild>
<button
className="group p-2 text-muted-foreground hover:text-rose-400 hover:bg-rose-500/10 rounded-lg border border-transparent hover:border-rose-500/20 transition-all motion-safe:active:scale-95"
title={`Delete ${nodeLabel.toLowerCase()}`}
>
Comment on lines +103 to 106

@coderabbitai coderabbitai Bot May 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Set an explicit button type and accessible name on the dialog trigger.

This icon-only trigger can submit a parent form by default and lacks a reliable accessible name. Add type="button" and aria-label.

Suggested fix 
         <button
+          type="button"
+          aria-label={`Delete ${nodeLabel.toLowerCase()}`}
           className="group p-2 text-muted-foreground hover:text-rose-400 hover:bg-rose-500/10 rounded-lg border border-transparent hover:border-rose-500/20 transition-all motion-safe:active:scale-95"
           title={`Delete ${nodeLabel.toLowerCase()}`}
         >
🤖 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 `@components/delete-button.tsx` around lines 103 - 106, The icon-only trigger
button in delete-button.tsx needs an explicit type and accessible name to avoid
accidental form submission and to be screen-reader friendly; update the <button>
in the component (the button that currently uses title={`Delete
${nodeLabel.toLowerCase()}`}) to include type="button" and an aria-label that
uses nodeLabel (e.g., aria-label={`Delete ${nodeLabel}`}) so the trigger is
non-submitting and has a reliable accessible name.

{loading ? (
<Loader2 className="h-3 w-3 animate-spin" />
) : (
<Trash2 className="h-3 w-3" />
)}
Delete
<Trash2 className="h-4 w-4 group-hover:animate-pulse" />
</button>
Comment on lines +103 to 108
</div>
)
}

return (
<button
onClick={() => setConfirm(true)}
className="group p-2 text-muted-foreground hover:text-rose-400 hover:bg-rose-500/10 rounded-lg border border-transparent hover:border-rose-500/20 transition-all motion-safe:active:scale-95"
title={`Delete ${nodeLabel.toLowerCase()}`}
>
<Trash2 className="h-4 w-4 group-hover:animate-pulse" />
</button>
</AlertDialogTrigger>
<AlertDialogContent>
<AlertDialogHeader>
<AlertDialogTitle>Delete {nodeLabel.toLowerCase()}?</AlertDialogTitle>
<AlertDialogDescription>
<span className="block text-slate-300">{displayTitle}</span>
<span className="mt-2 block">
This will disconnect all linked nodes. The deletion is permanent
and will be recorded in the audit log.
</span>
</AlertDialogDescription>
</AlertDialogHeader>
<AlertDialogFooter>
<AlertDialogCancel disabled={loading}>Cancel</AlertDialogCancel>
<AlertDialogAction onClick={handleDelete} disabled={loading}>
{loading ? (
<>
<Loader2 className="mr-2 h-4 w-4 animate-spin" />
Deleting…
</>
) : (
<>
<Trash2 className="mr-2 h-4 w-4" />
Delete {nodeLabel.toLowerCase()}
</>
)}
</AlertDialogAction>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
)
}
Loading
Loading