Add audit logs for roadmaps #86
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
WalkthroughCentralizes audit logging in a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as App UI
participant Logic as Feature Logic
participant Auth as useUserData
participant SB as Supabase
participant Audit as createAuditLog
User->>UI: Trigger create/update/delete/move
UI->>Auth: Get { supabase, user }
UI->>Logic: Execute operation
Logic->>SB: Perform DB change
alt success
Logic->>Audit: createAuditLog(supabase, { page_id, actor_id: user.id, action, changes })
Audit->>SB: insert into page_audit_logs
SB-->>Audit: ok
else failure
Logic-->>UI: surface error (no audit)
end
sequenceDiagram
autonumber
actor User
participant Board as RoadmapBoard
participant DnD as useRoadmapDragDrop
participant Auth as useUserData
participant Audit as createAuditLog
participant SB as Supabase
User->>Board: Drag item and drop
Board->>DnD: onDrop({ itemsByColumn, board })
DnD->>DnD: Determine from_column/to_column and action
DnD->>Auth: Get { supabase, user }
alt user present
DnD->>Audit: createAuditLog(supabase, { page_id: board.page_id, actor_id: user.id, action, changes })
Audit->>SB: insert(page_audit_logs)
SB-->>Audit: ok
else no user
DnD-->>DnD: Skip audit
end
sequenceDiagram
autonumber
actor User
participant Hook as useRoadmapItems
participant Auth as useUserData
participant SB as Supabase
participant Audit as createAuditLog
User->>Hook: Create/Update/Delete item
Hook->>SB: Apply DB/state change
Hook->>Auth: Get { supabase, user }
opt user present
Hook->>Audit: createAuditLog(...{ action: "Created/Updated/Deleted Roadmap Item: ...", changes })
Audit->>SB: insert(page_audit_logs)
SB-->>Audit: ok
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![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
🧹 Nitpick comments (15)
apps/web/pages/api/posts/index.ts (1)
77-82: Trim audit payload to essentials to keep logs lean.Storing full post bodies in audit logs can bloat the table and complicate redactions. Prefer a minimal delta (ids and key fields).
Apply this diff:
await createAuditLog(supabase, { page_id, actor_id: user.id, action: `Created Post: ${post.title}`, - changes: postPayload, + changes: { id: post.id, title: post.title, status: post.status }, });apps/web/components/post/post.tsx (1)
175-181: Fix stale closure: add missing useCallback deps.
doDeletePostcapturespage.idanduser.idbut they’re not in the dependency array, risking incorrect actor/page in audit entries.Apply this diff:
- [supabase, fetchPosts] + [supabase, fetchPosts, page.id, user?.id]Also applies to: 190-190
apps/web/pages/pages/[page_id]/roadmap/new.tsx (1)
140-147: Don’t block UX on audit logging.Since
createAuditLogalready swallows errors, you can make it fire-and-forget to reduce latency on navigation.Apply this diff:
- if (user) { - await createAuditLog(supabase, { + if (user) { + void createAuditLog(supabase, { page_id: page_id, actor_id: user.id, action: `Created Roadmap Board: ${board.title}`, changes: { board }, }); }apps/web/pages/pages/[page_id]/[post_id].tsx (1)
71-76: Make audit non-blocking and log only key fields.Keeps updates snappy and avoids storing full post payloads.
Apply this diff:
- await createAuditLog(supabase, { + void createAuditLog(supabase, { page_id: String(page_id), actor_id: user.id, action: `Updated Post: ${newPost.title}`, - changes: newPost, + changes: { id: post_id, title: newPost.title, status: newPost.status }, });apps/web/utils/hooks/usePageSettings.ts (1)
25-31: Guard missing user and avoid awaiting audit.Prevents possible undefined access and keeps the UI responsive.
Apply this diff:
- await createAuditLog(supabase, { - page_id: pageId, - actor_id: user.id, - action: "Updated Page Settings", - changes: payload, - }); + if (user?.id) { + void createAuditLog(supabase, { + page_id: pageId, + actor_id: user.id, + action: "Updated Page Settings", + changes: payload, + }); + }apps/web/utils/auditLog.ts (2)
3-8: Export the AuditLogEntry type for reuse.Helps callers type their payloads consistently across the app.
Apply this diff:
-interface AuditLogEntry { +export interface AuditLogEntry { page_id: string; actor_id: string; action: string; changes?: Record<string, any>; }
14-23: Avoid throw-then-catch pattern; it double-logs and adds noise.Either let errors bubble or log-and-return. Given current callers shouldn’t fail on audit issues, prefer log-and-return.
Apply this diff:
try { const { error } = await supabase.from("page_audit_logs").insert(entry); if (error) { console.error("Failed to create audit log:", error); - throw error; + return; } } catch (error) { console.error("Error creating audit log:", error); }apps/web/pages/pages/[page_id]/roadmap/[board_id]/settings.tsx (1)
239-259: Make all audit calls non-blocking to keep settings UI snappy.You don’t need to await audit writes; switching to fire-and-forget reduces perceived latency and avoids coupling UX to logging.
Example diff (apply similarly to other audit calls in this file):
- await createAuditLog(supabase, { + void createAuditLog(supabase, { page_id: page_id, actor_id: user.id, action: `Updated Roadmap Board: ${boardForm.title}`, changes: { old: { /* ... */ }, new: { /* ... */ } }, });Also applies to: 292-299, 339-350, 383-395, 425-431, 460-471, 509-517, 568-578
apps/web/components/roadmap/hooks/useRoadmapDragDrop.ts (5)
54-57: Avoid unsafe cast incontainscheck.Guard
relatedTargetinstead of forcing aNodecast.- if (!e.currentTarget.contains(e.relatedTarget as Node)) { + if (!e.relatedTarget || !e.currentTarget.contains(e.relatedTarget as Node)) { setDragOverColumn(null); setDragOverPosition(null); }
98-116: Audit log message is ambiguous and payload is heavy.
- The description “from column to column” lacks identifiers.
- Storing the full
draggedItembloats logs; prefer minimal fields.- const action = sourceColumnId === targetColumnId ? "Reordered" : "Moved"; - const description = sourceColumnId === targetColumnId - ? `${action} item within column` - : `${action} item from column to column`; + const action = sourceColumnId === targetColumnId ? "Reordered" : "Moved"; + const description = + sourceColumnId === targetColumnId + ? `Reordered item within column ${targetColumnId}` + : `Moved item from ${sourceColumnId} to ${targetColumnId}`; await createAuditLog(supabase, { page_id: board.page_id, actor_id: user.id, - action: `Updated Roadmap Item: ${draggedItem.title}`, - changes: { - action: description, - from_column: sourceColumnId, - to_column: targetColumnId, - item: draggedItem - }, + action: `Updated Roadmap Item: ${draggedItem.title}`, + changes: { + action: description, + from_column: sourceColumnId, + to_column: targetColumnId, + item_id: draggedItem.id, + item_title: draggedItem.title + }, });Optional: only log when something actually changed (see next comment on returning a boolean).
125-147: Log only when a move/reorder actually occurred.
handleSameColumnReordercan early-return (no-op), but you still log a “Reordered” action. Have helpers return a boolean and use it to gate logging.-const handleSameColumnReorder = async (...) => { +const handleSameColumnReorder = async (...): Promise<boolean> => { if (!currentDragOverPosition || !draggedItem) { - return; + return false; } ... if (draggedIndex === -1 || targetIndex === -1 || draggedIndex === targetIndex) { - return; + return false; } ... - await Promise.all(finalUpdates); + await Promise.all(finalUpdates); + return true; }; -const handleCrossColumnMove = async (...) => { +const handleCrossColumnMove = async (...): Promise<boolean> => { ... - setBoardItems((prev) => ... - ); + setBoardItems((prev) => ...); + return true; }; - if (sourceColumnId === targetColumnId) { - await handleSameColumnReorder(...); - } else { - await handleCrossColumnMove(...); - } + const didChange = sourceColumnId === targetColumnId + ? await handleSameColumnReorder(...) + : await handleCrossColumnMove(...); + if (!didChange) return;Also applies to: 85-96
199-205: Cross-column moves don’t reindex the source column (positions can retain gaps).After moving an item out, decrement positions of items below it in the source column, and reflect that in state. Minimal change by passing
sourceColumnItems.- } else { - await handleCrossColumnMove( - targetColumnItems, - targetColumnId, - currentDragOverPosition - ); + } else { + await handleCrossColumnMove( + sourceColumnItems, + targetColumnItems, + targetColumnId, + currentDragOverPosition + ); }-const handleCrossColumnMove = async ( - targetColumnItems: RoadmapItemWithRelations[], - targetColumnId: string, - currentDragOverPosition: DragOverPosition | null -) => { +const handleCrossColumnMove = async ( + sourceColumnItems: RoadmapItemWithRelations[], + targetColumnItems: RoadmapItemWithRelations[], + targetColumnId: string, + currentDragOverPosition: DragOverPosition | null +) => {- setBoardItems((prev) => + setBoardItems((prev) => prev.map((item) => { if (item.id === draggedItem.id) { return { ...item, column_id: targetColumnId, position: newPosition, }; } + // Close gap in source column + if (item.column_id === draggedItem.column_id && item.position > (draggedItem.position || 0)) { + return { ...item, position: item.position - 1 }; + } if (item.column_id === targetColumnId && currentDragOverPosition) { const targetItem = targetColumnItems.find( (ti) => ti.id === currentDragOverPosition.itemId ); if (targetItem) { if ( currentDragOverPosition.position === "before" && item.position >= targetItem.position ) { return { ...item, position: item.position + 1 }; } if ( currentDragOverPosition.position === "after" && item.position > targetItem.position ) { return { ...item, position: item.position + 1 }; } } } return item; }) ); + + // Decrement positions in source column in DB to close the gap + const sourceItemsToShift = sourceColumnItems.filter( + (i) => (i.position || 0) > (draggedItem.position || 0) + ); + if (sourceItemsToShift.length) { + // Descending to honor unique constraints if any + const sorted = sourceItemsToShift.sort((a, b) => (b.position || 0) - (a.position || 0)); + for (const it of sorted) { + await supabase.from("roadmap_items").update({ position: (it.position || 0) - 1 }).eq("id", it.id); + } + }Note: For true atomicity and fewer round trips, prefer a DB-side function/transaction to shift blocks.
Also applies to: 91-96, 268-298
229-233: Performance: sequential updates per row.Both shifting loops issue N roundtrips. Consider a server-side function/SQL update to shift positions in one statement, executed in a transaction.
Also applies to: 246-250
apps/web/components/roadmap/hooks/useRoadmapItems.ts (2)
195-196: Optional: keep client state sorted after insert.If the consumer expects items ordered by
position, sort when appending.- setBoardItems((prev) => [...prev, data]); + setBoardItems((prev) => + [...prev, data].sort((a, b) => (a.position || 0) - (b.position || 0)) + );
151-159: Optional: trim audit payloads.To keep
page_audit_logslean, prefer IDs/titles over full objects forchanges.Also applies to: 200-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/components/post/post.tsx(2 hunks)apps/web/components/roadmap/RoadmapBoard.tsx(1 hunks)apps/web/components/roadmap/hooks/useRoadmapDragDrop.ts(2 hunks)apps/web/components/roadmap/hooks/useRoadmapItems.ts(5 hunks)apps/web/pages/api/posts/index.ts(2 hunks)apps/web/pages/pages/[page_id]/[post_id].tsx(2 hunks)apps/web/pages/pages/[page_id]/roadmap/[board_id]/settings.tsx(9 hunks)apps/web/pages/pages/[page_id]/roadmap/new.tsx(3 hunks)apps/web/utils/auditLog.ts(1 hunks)apps/web/utils/hooks/usePageSettings.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
apps/web/pages/pages/[page_id]/roadmap/[board_id]/settings.tsx (1)
apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
apps/web/pages/pages/[page_id]/roadmap/new.tsx (2)
apps/web/utils/useUser.tsx (1)
useUserData(135-141)apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
apps/web/pages/api/posts/index.ts (1)
apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
apps/web/components/post/post.tsx (1)
apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
apps/web/components/roadmap/hooks/useRoadmapItems.ts (2)
apps/web/utils/useUser.tsx (1)
useUserData(135-141)apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
apps/web/pages/pages/[page_id]/[post_id].tsx (1)
apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
apps/web/utils/hooks/usePageSettings.ts (1)
apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
apps/web/components/roadmap/hooks/useRoadmapDragDrop.ts (4)
apps/web/components/roadmap/types.ts (2)
ItemsByColumn(12-14)RoadmapItemWithRelations(7-10)packages/supabase/types/page.ts (1)
IRoadmapBoard(11-11)apps/web/utils/useUser.tsx (1)
useUserData(135-141)apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
🔇 Additional comments (9)
apps/web/pages/api/posts/index.ts (1)
6-6: Centralized audit logging import looks good.apps/web/components/post/post.tsx (1)
26-26: Import is correct; centralization aligns with the PR direction.apps/web/components/roadmap/RoadmapBoard.tsx (1)
36-40: Do not remove setBoardItems — hook still expects ituseRoadmapDragDrop's signature still includes setBoardItems (apps/web/components/roadmap/hooks/useRoadmapDragDrop.ts, lines 11–14), so keep passing setBoardItems from RoadmapBoard.tsx.
Likely an incorrect or invalid review comment.
apps/web/pages/pages/[page_id]/roadmap/new.tsx (1)
6-6: Import is fine.apps/web/pages/pages/[page_id]/[post_id].tsx (1)
17-17: Import OK; consistent with centralized logging.apps/web/utils/hooks/usePageSettings.ts (1)
9-9: Good move to centralize page settings audit logging.apps/web/components/roadmap/hooks/useRoadmapDragDrop.ts (1)
14-19: Signature change verified — all call sites updatedRepo search found a single consumer: apps/web/components/roadmap/RoadmapBoard.tsx calls useRoadmapDragDrop({ itemsByColumn, setBoardItems, board }), so no further changes required.
apps/web/components/roadmap/hooks/useRoadmapItems.ts (2)
149-160: Update audit logging looks good.Captures old/new payloads and awaits the insert. Nice.
197-205: Create audit logging looks good.Logs actor, page, action, and created item; awaited. ✅
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/components/roadmap/hooks/useRoadmapItems.ts (1)
69-88: Fix: move delete audit out of setState and await it (avoid duplicate/indeterminate logs).Side effects inside React state updaters can run multiple times (StrictMode) and your audit call isn’t awaited, risking duplicate or missing logs. Capture the item before updating state, then update state, then await the audit log.
Apply:
try { + // Capture the item deterministically before state mutation + const itemToDelete = + Object.values(itemsByColumn).flat().find((it) => it.id === itemId); const { error } = await supabase .from("roadmap_items") .delete() .eq("id", itemId); if (error) throw error; - setBoardItems((prev) => { - const deletedItem = prev.find((item) => item.id === itemId); - if (deletedItem && user) { - createAuditLog(supabase, { - page_id: board.page_id, - actor_id: user.id, - action: `Deleted Roadmap Item: ${deletedItem.title}`, - changes: { item: deletedItem }, - }); - } - return prev.filter((item) => item.id !== itemId); - }); + setBoardItems((prev) => prev.filter((item) => item.id !== itemId)); + if (itemToDelete && user) { + await createAuditLog(supabase, { + page_id: board.page_id, + actor_id: user.id, + action: `Deleted Roadmap Item: ${itemToDelete.title}`, + changes: { item_id: itemToDelete.id, item_title: itemToDelete.title }, + }); + }
🧹 Nitpick comments (2)
apps/web/components/roadmap/hooks/useRoadmapItems.ts (2)
143-154: Optional: slim payload and standardize change shape in update logs.To reduce JSON size/PII and improve queryability, log ids/titles instead of full objects and use before/after keys.
- if (user) { - await createAuditLog(supabase, { + if (user) { + await createAuditLog(supabase, { page_id: board.page_id, actor_id: user.id, action: `Updated Roadmap Item: ${data.title}`, - changes: { - old: editingItem, - new: data, - }, + changes: { + before: { id: editingItem.id, title: editingItem.title }, + after: { id: data.id, title: data.title }, + }, }); }
191-199: Optional: align create log payload with slimmer schema.Consistent, minimal payloads keep the audit table lean.
- if (user) { - await createAuditLog(supabase, { + if (user) { + await createAuditLog(supabase, { page_id: board.page_id, actor_id: user.id, action: `Created Roadmap Item: ${data.title}`, - changes: { item: data }, + changes: { item_id: data.id, item_title: data.title }, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/components/roadmap/hooks/useRoadmapItems.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/components/roadmap/hooks/useRoadmapItems.ts (2)
apps/web/utils/useUser.tsx (1)
useUserData(135-141)apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
🔇 Additional comments (2)
apps/web/components/roadmap/hooks/useRoadmapItems.ts (2)
6-6: LGTM: centralized audit importImport path and usage look correct.
24-24: LGTM: user + client acquisitionDestructuring supabase and user from useUserData is appropriate here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/web/components/roadmap/hooks/useRoadmapItems.ts (3)
83-90: Enrich and normalize delete audit payload; also consider surfacing failures.
- Add board_id/column_id for better forensics and lighter querying.
- createAuditLog swallows errors; awaiting it won’t propagate failures. Consider returning a boolean/result or letting errors bubble (and handling locally) if audit guarantees matter.
- if (itemToDelete && user) { - await createAuditLog(supabase, { - page_id: board.page_id, - actor_id: user.id, - action: `Deleted Roadmap Item: ${itemToDelete.title}`, - changes: { item_id: itemToDelete.id, item_title: itemToDelete.title }, - }); - } + if (itemToDelete && user) { + await createAuditLog(supabase, { + page_id: board.page_id, + actor_id: user.id, + action: `Deleted Roadmap Item: ${itemToDelete.title}`, + changes: { + item_id: itemToDelete.id, + item_title: itemToDelete.title, + board_id: board.id, + column_id: itemToDelete.column_id, + }, + }); + }
145-156: Log minimal “old/new” shapes to keep audit rows small and focused.Avoid storing full related objects; capture just the essentials you’ll query.
- if (user) { - await createAuditLog(supabase, { - page_id: board.page_id, - actor_id: user.id, - action: `Updated Roadmap Item: ${data.title}`, - changes: { - old: editingItem, - new: data, - }, - }); - } + if (user) { + await createAuditLog(supabase, { + page_id: board.page_id, + actor_id: user.id, + action: `Updated Roadmap Item: ${data.title}`, + changes: { + old: { + id: editingItem.id, + title: editingItem.title, + column_id: editingItem.column_id, + category_id: editingItem.category_id, + }, + new: { + id: data.id, + title: data.title, + column_id: data.column_id, + category_id: data.category_id, + }, + }, + }); + }
193-201: Normalize creation audit payload for consistency with delete/update.Prefer explicit fields over entire records.
- if (user) { - await createAuditLog(supabase, { - page_id: board.page_id, - actor_id: user.id, - action: `Created Roadmap Item: ${data.title}`, - changes: { item: data }, - }); - } + if (user) { + await createAuditLog(supabase, { + page_id: board.page_id, + actor_id: user.id, + action: `Created Roadmap Item: ${data.title}`, + changes: { + item_id: data.id, + item_title: data.title, + board_id: board.id, + column_id: data.column_id, + category_id: data.category_id, + }, + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/components/roadmap/hooks/useRoadmapItems.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/components/roadmap/hooks/useRoadmapItems.ts (2)
apps/web/utils/useUser.tsx (1)
useUserData(135-141)apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
🔇 Additional comments (3)
apps/web/components/roadmap/hooks/useRoadmapItems.ts (3)
6-6: Centralized audit import looks good.Importing from a single utility promotes consistency and reuse.
24-24: Verify hook is always under UserContextProvider.useUserData throws if context is undefined. Ensure every consumer of useRoadmapItems is wrapped by UserContextProvider (pages/layouts).
70-73: Pre-capturing the item before deletion resolves prior bug.Deterministic capture from itemsByColumn and no side-effects in the setter. Nice.
Summary by CodeRabbit
New Features
Refactor