feat(ui): delete affordance for every primary entity#7
Conversation
Reusable shell wrapping base-ui Dialog for entity deletes. Per-entity dialogs become thin wrappers that supply preview text and an onConfirm server-action callback. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Adds deleteSignal in lib/, server action, and a hover-trigger delete button on each signal row. Audit row written per delete. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Adds a trash-icon action to TaskActions, available regardless of task status (open / done / dismissed). Server action writes the audit row and revalidates list + company tabs. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Adds delete button to kanban cards (with optimistic local removal), list view, and the company-detail deals tab. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Person delete cascades to tasks/notes/meeting_attendees and nulls signals.person_id. Confirm dialog fetches counts before destruction; action writes per-child audit rows in addition to the parent. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Company delete cascades to deals, signals, meetings, notes, and tasks in a transaction; nullifies people.company_id. Detail-page header shows a Delete button; confirm dialog fetches and displays per-table counts before confirmation. Audit fans out one row per cascaded child plus the parent. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Per-row delete on the company-detail people tab, matching the global /people index. Tasks tab already uses TaskRow which handles delete. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
|
Working on this PR… |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
enkii · code review
Summary
This PR adds delete affordances (with audit logging) for every primary entity: signals, notes, tasks, meetings, deals, people, and companies. The implementation is thorough — shared DeleteConfirmDialog, per-entity action wrappers, cascade previews for company/person, audit fan-out for cascaded children, and 7 new test files (109 passing). Two P2 findings: (1) the kanban board delete path doesn't call router.refresh() unlike the existing drag-to-move flow, so the client-side state won't sync with server data after deletion; (2) meeting attendances are counted in the person-delete preview dialog but never collected or audit-logged during actual deletion. Safe to merge after addressing these.
Mergeability Score: 4/5
Likely mergeable after reviewing the flagged low-risk comments.
| {relativeTime(d.stageEnteredAt)} | ||
| </span> | ||
| </div> | ||
| </Link> |
There was a problem hiding this comment.
router.refresh(), unlike drag-to-move flow
When a deal is deleted from the kanban board, DeleteDealButton.onDeleted optimistically removes it from local state, but router.refresh() is never called afterward. This is inconsistent with moveDealStageAction (line 78), which calls router.refresh() after a successful move to re-sync the client-side useState(initialDeals) with the server. After a delete, any server-side changes (e.g. deals added or moved by another user) won't appear until the user manually refreshes the page. Fix by passing an onDeleted callback that also calls router.refresh(), matching the existing move pattern.
| export type Person = typeof people.$inferSelect; | ||
|
|
||
| export type PersonCascadeChildren = { | ||
| taskIds: string[]; |
There was a problem hiding this comment.
previewPersonDelete counts meeting attendances and the DeletePersonButton dialog shows them to the user ("2 meeting attendances"), but PersonCascadeChildren has no meetingAttendanceIds field, deletePerson never collects them, and deletePersonAction never writes audit rows for them. The FK cascade (meeting_attendees.person_id → ON DELETE CASCADE) silently removes them without audit trail. Either remove meetingAttendances from the preview/count type since they aren't individually auditable entities, or add them to PersonCascadeChildren and fan out audit rows with a stub before like { personId }.
There was a problem hiding this comment.
enkii · security review
Summary
This PR adds hard-delete affordances for 7 entity types (companies, people, deals, tasks, notes, meetings, signals) via Next.js server actions with confirmation dialogs. All delete paths enforce authentication via requireOrgSession(), validate IDs with Zod UUID parsing, scope queries by organizationId, and write audit log entries. No exploitable P0/P1 vulnerabilities found. Two P2 TOCTOU defense-in-depth notes in lib/companies.ts and lib/people.ts where the org-membership pre-check runs outside the delete transaction — not currently exploitable but worth tightening. The PR explicitly documents that per-role authorization and rate limiting are out of scope. Safe to merge after addressing or acknowledging the TOCTOU notes.
Mergeability Score: 4/5
Likely mergeable after reviewing the flagged low-risk comments.
| const before = await fetchCompany(db, organizationId, companyId); | ||
| if (!before) return null; | ||
|
|
||
| return db.transaction(async (tx) => { |
There was a problem hiding this comment.
fetchCompany (line 34–41) runs the org-membership check outside of the transaction that begins at line 75. If the entity's organizationId were somehow changed between the check and tx.delete, the delete could execute under a stale authorization decision. The same pattern exists in lib/people.ts:deletePerson. In practice, org reassignment is not a normal operation and the window is tiny, so this is not currently exploitable. Moving the org check inside the transaction (as a SELECT ... FOR UPDATE or by adding eq(organizationId) to the tx.delete WHERE clause) would close the gap.
| organizationId: string, | ||
| personId: string, | ||
| ): Promise<{ before: Person; children: PersonCascadeChildren } | null> { | ||
| const before = await fetchPerson(db, organizationId, personId); |
There was a problem hiding this comment.
fetchPerson verifies org membership before entering the transaction at line 65. Same defense-in-depth concern as lib/companies.ts:deleteCompany. Consider moving the existence-check into the transaction for consistency with the principle of checking authorization atomically with the destructive operation.
1. kanban: call router.refresh() after onDeleted so the local optimistic filter doesn't diverge from server state. 2. people: drop meetingAttendances from preview counts. The meeting_attendees join rows aren't in audit ENTITY_TYPES and weren't being collected or audited during the cascade, so surfacing a count the audit trail can't account for was misleading. 3. companies: move org-membership fetch inside the delete transaction to close the TOCTOU defense-in-depth gap. 4. people: same TOCTOU fix as companies. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
|
Working on this PR… |
There was a problem hiding this comment.
enkii · code review
Summary
This PR adds delete affordances (UI buttons + server actions + domain functions) for all seven primary entities (signals, notes, tasks, meetings, deals, people, companies), each backed by tests. The diff is large but mechanically consistent across entities. Two P2 issues: both deleteDealAction and deletePersonAction fail to revalidate /tasks after cascade-deleting child tasks via FK, which causes stale rows on the Tasks page. The four issues flagged in the prior review pass have been addressed in this commit (kanban router.refresh, meeting attendances gap, and both TOCTOU concerns). After fixing the two missing revalidatePath calls, the PR is safe to merge.
Mergeability Score: 4/5
Likely mergeable after reviewing the flagged low-risk comments.
| }); | ||
|
|
||
| revalidatePath("/deals"); | ||
| revalidatePath(`/companies/${result.before.companyId}/deals`); |
There was a problem hiding this comment.
revalidatePath("/tasks") after deleting a deal that cascade-removes tasks
deleteDealAction calls revalidatePath for /deals, the company deals tab, and the company detail page, but never revalidates /tasks. The tasks.dealId FK is ON DELETE CASCADE, so any tasks attached to the deleted deal are silently removed from the database. Navigating to /tasks after deleting a deal will show stale task rows that no longer exist. Add revalidatePath("/tasks") alongside the other revalidate calls.
| changes: { before: result.before }, | ||
| }); | ||
|
|
||
| revalidatePath("/people"); |
There was a problem hiding this comment.
revalidatePath("/tasks") after deleting a person that cascade-removes tasks
deletePersonAction revalidates /people and the company paths, but never calls revalidatePath("/tasks"). The tasks.personId FK is ON DELETE CASCADE, so every task assigned to the deleted person is removed from the database. After a person delete, the /tasks page continues to serve cached rows for those now-deleted tasks. Add revalidatePath("/tasks") to the final block, matching the pattern in deleteCompanyAction.
There was a problem hiding this comment.
enkii · security review
Summary
Security review of PR #7 — Delete affordance for every primary entity.
Surface reviewed: Seven new delete server actions (signal, note, task, meeting, deal, person, company), seven new lib/ domain functions, a shared DeleteConfirmDialog component, and per-entity UI buttons. Also reviewed the seven new test files for cross-org protection.
Key findings:
-
No P0 or P1 findings. All actions require
requireOrgSession()(authentication enforced). Everylib/function verifiesorganizationIdbefore deleting (cross-org IDOR prevented). All IDs are validated withz.string().uuid(). No SQL injection, no RCE, no auth bypass. -
P2 — TOCTOU in leaf entity deletes (5 files): The five leaf-entity delete functions (
lib/deals.ts,lib/signals.ts,lib/meetings.ts,lib/notes.ts,lib/tasks.ts) check org membership outside a database transaction, then DELETE without theorganizationIdin the WHERE clause. The cascade functions (deleteCompany,deletePerson) correctly usedb.transaction(). This is a defense-in-depth inconsistency; practical exploitability is very low since the app has no org-reassignment operation. -
Previously flagged issues resolved: The earlier-pass TOCTOU comments on
deleteCompanyanddeletePersonhave been addressed in this commit (both now wrap org check + delete indb.transaction()). The meeting-attendance audit-gap comment is also resolved (PersonCascadeCountsno longer includesmeetingAttendances).
Merge guidance: Safe to merge after the leaf-entity TOCTOU consistency fixes (wrap in db.transaction() or add organizationId to each DELETE WHERE). No blocker-level issues.
Mergeability Score: 4/5
Likely mergeable after reviewing the flagged low-risk comments.
| .select() | ||
| .from(deals) | ||
| .where(and(eq(deals.id, dealId), eq(deals.organizationId, organizationId))) | ||
| .limit(1); |
There was a problem hiding this comment.
All five leaf-entity delete functions (deleteDeal, deleteSignal, deleteMeeting, deleteNote, deleteTask) check org membership via a SELECT with organizationId filter, then execute the DELETE by id only — without wrapping these operations in a database transaction. This leaves a TOCTOU race window: if an entity's organizationId were changed between the SELECT and the DELETE, the delete would succeed for an entity no longer in the caller's org. The cascade delete functions (deleteCompany, deletePerson) do not have this gap — they correctly wrap both the org check and the DELETE inside db.transaction(). The practical exploitability is low (the app has no org-reassignment UI), but the inconsistency is a defense-in-depth gap. Fix by either wrapping each function in db.transaction() or adding eq(entity.organizationId, organizationId) to the DELETE WHERE clause.
| .select() | ||
| .from(signals) | ||
| .where(and(eq(signals.id, signalId), eq(signals.organizationId, organizationId))) | ||
| .limit(1); |
There was a problem hiding this comment.
Same defense-in-depth gap as lib/deals.ts. The SELECT verifies signals.organizationId matches the caller's org, but the subsequent db.delete(signals).where(eq(signals.id, signalId)) does not include the org condition and is not inside a transaction.
| .select() | ||
| .from(meetings) | ||
| .where(and(eq(meetings.id, meetingId), eq(meetings.organizationId, organizationId))) | ||
| .limit(1); |
There was a problem hiding this comment.
Same defense-in-depth gap as lib/deals.ts and lib/signals.ts. The org-membership SELECT and the DELETE are not atomic.
| .select() | ||
| .from(notes) | ||
| .where(and(eq(notes.id, noteId), eq(notes.organizationId, organizationId))) | ||
| .limit(1); |
There was a problem hiding this comment.
Same defense-in-depth gap as the other leaf-entity delete functions.
| .select() | ||
| .from(tasks) | ||
| .where(and(eq(tasks.id, taskId), eq(tasks.organizationId, organizationId))) | ||
| .limit(1); |
There was a problem hiding this comment.
Same defense-in-depth gap as the other leaf-entity delete functions.
1. leaf deletes: collapse SELECT-then-DELETE into a single atomic DELETE...WHERE id = ? AND organization_id = ? RETURNING *. Closes the TOCTOU defense-in-depth gap on signals, notes, tasks, meetings, and deals. 2. deals: revalidate /tasks after delete (FK cascade removes tasks). 3. people: revalidate /tasks after delete (same cascade). Co-authored-by: Claude <81847+claude@users.noreply.github.com>
|
Working on this PR… |
There was a problem hiding this comment.
enkii · code review
Summary
This PR adds delete affordances for all seven primary entities (signals, notes, tasks, meetings, deals, people, companies) with cascade previews for company/person, audit logging for every delete including child fan-out, and a shared DeleteConfirmDialog component. Most issues from the prior review round have been addressed (TOCTOU gaps in leaf deletes now use single DELETE ... RETURNING with orgId in the WHERE clause; meeting attendance counting removed from person cascade; deletePersonAction now revalidates /tasks; kanban delete now calls router.refresh()). Three P2 findings remain: (1) deleteDealAction still missing revalidatePath("/tasks"), (2) DeleteConfirmDialog has no error handling in startTransition and gets permanently stuck on thrown exceptions, and (3) the DealsView list view has no optimistic deletion callback, leaving stale rows after delete. Safe to merge after addressing these three items.
Mergeability Score: 4/5
Likely mergeable after reviewing the flagged low-risk comments.
| revalidatePath("/tasks"); | ||
| revalidatePath(`/companies/${result.before.companyId}/deals`); | ||
| revalidatePath(`/companies/${result.before.companyId}`); | ||
| revalidatePath("/companies"); |
There was a problem hiding this comment.
revalidatePath("/tasks") after deleting a deal
deleteDealAction calls revalidatePath for /deals, the company deals tab, the company detail page, and /companies, but never revalidates /tasks. Deals cascade-delete their tasks via ON DELETE CASCADE on tasks.dealId. After deleting a deal, visiting /tasks shows stale rows for tasks that no longer exist in the database. Add revalidatePath("/tasks") before return { ok: true } to match the pattern used in deleteCompanyAction and deletePersonAction.
| confirmLabel?: string; | ||
| onConfirm: () => Promise<DeleteResult>; | ||
| }) { | ||
| const [error, setError] = useState<string | null>(null); |
There was a problem hiding this comment.
startTransition locks the dialog in "Deleting…" forever
onConfirmClick passes an async function to startTransition but never wraps it in try/catch. If onConfirm() throws — for example, a network error when calling a server action — the exception propagates unhandled, isPending stays true, the Cancel button stays disabled, and the user has no way to dismiss the dialog short of a full page reload. Wrap the body in try/catch and call setError("Something went wrong. Please try again.") on failure.
| <TemperaturePill value={d.temperature} /> | ||
| </td> | ||
| <td className="px-3.5 py-2.5 align-middle"> | ||
| <DeleteDealButton dealId={d.id} dealName={d.name} /> |
There was a problem hiding this comment.
DealsView passes no onDeleted callback to DeleteDealButton in the table/list view, so the deleted deal row persists in useState until the component remounts. The kanban view in the same file correctly passes onDeleted that calls setDeals(cur => cur.filter(...)) and router.refresh(). After deleting from the list, the server action runs and revalidatePath triggers a parent re-render with fresh initialDeals, but useState ignores the updated prop. The row stays visible until the user navigates away and back. Pass an onDeleted that filters the row from local state and calls router.refresh(), matching the kanban pattern.
There was a problem hiding this comment.
enkii · security review
Summary
This PR adds hard-delete affordances for 7 entity types (companies, people, deals, tasks, notes, meetings, signals) with audit logging and cascade-delete previews. The security posture is solid: every action authenticates via requireOrgSession(), validates IDs with Zod UUID parsing, checks cross-org ownership, and uses Drizzle ORM parameterized queries (no SQL injection vectors). The prior enkii run's TOCTOU concerns have been addressed — leaf-entity deletes are now single DELETE … WHERE id=X AND organizationId=Y … RETURNING statements, and cascade deletes are wrapped in db.transaction(). Two new P2 findings: deleteCompanyAction and deletePersonAction both omit revalidatePath for the deleted entity's own detail page, risking stale cache. No P0 or P1 vulnerabilities. Safe to merge after addressing the two missing revalidation paths.
Mergeability Score: 4/5
Likely mergeable after reviewing the flagged low-risk comments.
| changes: { before: result.before }, | ||
| }); | ||
|
|
||
| revalidatePath("/companies"); |
There was a problem hiding this comment.
revalidatePath for deleted company's detail page
deleteCompanyAction calls revalidatePath("/companies") which in Next.js App Router defaults to page-level revalidation and only invalidates the /companies list page — it does not clear cached RSC payloads for child routes like /companies/${companyId}. After deletion, navigating to the deleted company's URL could serve a stale cached page. Add revalidatePath(\/companies/${companyId}`)or pass'layout'as the second argument torevalidatePath("/companies", "layout")` to ensure the detail-page cache is purged.
| changes: { before: result.before }, | ||
| }); | ||
|
|
||
| revalidatePath("/people"); |
There was a problem hiding this comment.
revalidatePath for deleted person's detail page
deletePersonAction revalidates /people (list page), /tasks, and company-scoped paths, but never calls revalidatePath(\/people/${personId}`). The person detail page at /people/[id]is not invalidated after deletion, so its cached RSC payload could be served stale. AddrevalidatePath(`/people/${personId}`)` alongside the existing revalidation calls.
1 participant
Summary
db/schema/handle dependent rows.company,person) the action fans out onedeleteaudit row per cascaded child (modelled ondeleteStageAction).components/delete-confirm-dialog.tsxwraps the existing base-ui Dialog; per-entity components are thin wrappers that supply preview text and anonConfirmserver action.Where the delete UI lives
/companies/[id]header (detail-page only; no kebab on the index)/peoplerow,/people/[id]header,/companies/[id]/peoplerow/dealslist row,/companies/[id]/dealsrowTaskActions(any list usingTaskRow)/companies/[id]/notesper-card/meetingsrow/companies/[id]/signalsper-rowCascade preview
Company and person delete dialogs run a server preview before destruction and show counts:
Domain layer
New per-entity
delete*(and where relevantpreview*Delete) functions live inlib/:lib/signals.ts,lib/notes.ts,lib/tasks.ts,lib/meetings.ts,lib/deals.ts— leaf entities.lib/people.ts,lib/companies.ts— cascade preview + transactional delete returning child IDs for audit fan-out.All functions take
(db, organizationId, id)and returnnullfor cross-org or missing rows — the action layer surfaces this as{ ok: false, message: "… not found" }.Audit
lib/audit.ts:ENTITY_TYPESalready covered every entity in scope — no audit-log schema change. Every delete writesaction: "delete"withchanges: { before }. Cascaded children get one row each withbefore: { companyId }/before: { personId }, consistent with the stage-delete precedent.Out of scope
Test plan
bun test— 109 pass, 0 fail (16 files; 7 new test files added:signals,notes,tasks,meetings,deals,people,companies).bun run lintbun run typecheckbun run buildperson_idnulled, redirect to/people./companies, audit shows parent + per-child rows.🤖 Generated with Claude Code