feat(audit): add centralized audit logging system - #603
Merged
Conversation
- Add auditLogsTable schema with proper indexes - Create auditLog() helper with typed actions enum - Integrate logging for moderation actions (warn, block, dismiss) - Integrate logging for content actions (delete, edit, solve, pin) - Add audit log viewer page for administrators - Fire-and-forget logging pattern to prevent request failures
Fix TypeScript error by using NextRequest instead of Request for API route testing.
|
CodeAnt AI is reviewing your PR. |
|
@Rashi1404 is attempting to deploy a commit to the Karan Mani Tripathi 's projects Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
gssoc'26
|
@coderabbitai review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Technical Review
Hi @Rashi1404! Thank you for your contribution to DoubtDesk.
The code changes look good. Before we can complete the technical review, approve, and merge this pull request, we have one final requirement for all contributors: Please star the DoubtDesk repository.
Once you have starred the repository, please drop a comment here saying "done" (or we will automatically detect it) and we will proceed with approving and merging your PR. Thank you.
There was a problem hiding this comment.
Technical Review
Hi @Rashi1404! Thank you for your contribution to DoubtDesk.
The code changes look good. Before we can complete the technical review, approve, and merge this pull request, we have one final requirement for all contributors: Please star the DoubtDesk repository.
Once you have starred the repository, please drop a comment here saying "done" (or we will automatically detect it) and we will proceed with approving and merging your PR. Thank you.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR implements a comprehensive audit logging system for DoubtDesk by adding a database table to record administrative and moderation actions, creating a core library for audit event emission, building an admin interface to query logs, and instrumenting multiple API routes to emit audit events during moderation, doubt, and reply operations. ChangesAudit Logging System
Sequence DiagramsequenceDiagram
participant Admin
participant AdminPage as Admin Page<br>(Client)
participant AuditAPI as /api/admin/audit
participant Database
participant AuditTable
Admin->>AdminPage: navigate to audit logs
AdminPage->>AuditAPI: GET ?page=1&limit=20
AuditAPI->>Database: SELECT audit_logs<br/>JOIN users<br/>ORDER BY created_at DESC
Database-->>AuditAPI: paginated logs + pagination meta
AuditAPI-->>AdminPage: { logs, pagination }
AdminPage->>AuditTable: render logs
AuditTable-->>Admin: display table with actions/metadata
Note over AuditAPI: On moderation POST<br/>admin emits auditLog()
Note over AuditAPI: On doubt/reply changes<br/>actor emits auditLog()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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 |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+12
to
+16
| CREATE INDEX "audit_actor_idx" | ||
| ON "audit_logs" ("actor_email"); | ||
|
|
||
| CREATE INDEX "audit_action_idx" | ||
| ON "audit_logs" ("action"); No newline at end of file |
There was a problem hiding this comment.
Suggestion: The migration adds indexes for actor and action but not for created_at, while audit listing queries are ordered by newest first with pagination. As row count grows, this forces expensive sort/scan work on every admin page request; add an index on created_at (or composite index matching query pattern) to avoid degraded performance. [performance]
Severity Level: Major ⚠️
- ⚠️ Admin audit endpoint slows as audit_logs row count grows.
- ⚠️ Admin audit UI becomes sluggish for later pagination pages.Steps of Reproduction ✅
1. Apply migration `drizzle/0008_audit_logs.sql` which creates `audit_logs` and only the
`audit_actor_idx` and `audit_action_idx` indexes (lines 1–16).
2. Generate a large number of audit entries via normal flows that call `auditLog()` in
`src/lib/audit.ts:32-41` (for example, reply edit/delete in
`src/app/api/replies/action/[id]/route.ts:80-92`, doubt actions in
`src/app/api/doubts/action/[id]/route.ts:184-90`, pin/unpin in
`src/app/api/doubts/[id]/pin/route.ts:52-73`, moderation actions in
`src/app/api/admin/moderation/action/route.ts:10-46`), populating tens of thousands of
rows in `audit_logs`.
3. As an admin, open the audit UI at `/admin/audit`, which triggers `AdminAuditPage` in
`src/app/admin/audit/page.tsx:8-18` to fetch `/api/admin/audit?page=1&limit=20`.
4. The handler `GET` in `src/app/api/admin/audit/route.ts:7-35` executes a query ordering
by `desc(auditLogsTable.createdAt)` with `LIMIT/OFFSET` but there is no index on
`created_at` in either `drizzle/0008_audit_logs.sql:1-16` or `src/configs/schema.ts:8-20`,
so PostgreSQL must sort or scan the full `audit_logs` table, causing response latency to
grow significantly as row count increases (observable via `EXPLAIN ANALYZE` on this
query).Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** drizzle/0008_audit_logs.sql
**Line:** 12:16
**Comment:**
*Performance: The migration adds indexes for actor and action but not for `created_at`, while audit listing queries are ordered by newest first with pagination. As row count grows, this forces expensive sort/scan work on every admin page request; add an index on `created_at` (or composite index matching query pattern) to avoid degraded performance.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
Author
|
kindly add level advanced |
Comment on lines
+33
to
+39
| void auditLog({ | ||
| actorEmail: adminEmail, | ||
| targetEmail: userEmail, | ||
| action: AUDIT_ACTIONS.MODERATION_DISMISSED, | ||
| resourceType: "moderation_log", | ||
| resourceId: logId, | ||
| }); |
There was a problem hiding this comment.
Suggestion: The dismissed-log audit record uses userEmail directly from request input instead of deriving the target from the moderation log identified by logId, so an admin/client can produce incorrect audit attribution by sending mismatched values. Build the audit target from the fetched moderation log (or validate logId belongs to userEmail) before writing the audit entry. [security]
Severity Level: Major ⚠️
- ❌ Audit logs misattribute dismissed moderation logs to wrong user.
- ⚠️ Admins reviewing audit page see incorrect moderation targets.Steps of Reproduction ✅
1. Log in as an admin and open the moderation UI, which renders `ModerationTable`
(`src/components/admin/ModerationTable.tsx:4-7`) and calls `handleAction()` to POST to
`/api/admin/moderation/action` (`ModerationTable.tsx:8-15`).
2. Using browser devtools or a custom client, issue a POST to
`/api/admin/moderation/action` with a JSON body like `{ "logId": 123, "userEmail":
"other-user@example.com", "action": "dismiss" }` where `logId=123` belongs to a different
user in `moderation_logs.userEmail` (`src/configs/schema.ts:49-56`), but
`other-user@example.com` is any existing user.
3. On the server, the `POST` handler in
`src/app/api/admin/moderation/action/route.ts:10-32` parses `{ logId, userEmail, action
}`, verifies only that a `usersTable` row exists for `userEmail` (`route.ts:23-26`),
updates `moderationLogsTable` by `id = logId` (`route.ts:29-31`), and never validates that
this log's `userEmail` matches the supplied `userEmail`.
4. The same handler then calls `auditLog` at `route.ts:33-39`, passing `targetEmail:
userEmail` and `resourceId: logId`, so the audit row in `audit_logs`
(`src/configs/schema.ts:68-80`) incorrectly attributes dismissing log `logId=123` to
`targetEmail="other-user@example.com"`. When an admin views the Audit Logs page
(`src/app/admin/audit/page.tsx:14-31` displaying `AuditLogTable` from
`src/components/admin/AuditLogTable.tsx:14-31`), they see a `MODERATION_DISMISSED` event
showing the wrong target user.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/admin/moderation/action/route.ts
**Line:** 33:39
**Comment:**
*Security: The dismissed-log audit record uses `userEmail` directly from request input instead of deriving the target from the moderation log identified by `logId`, so an admin/client can produce incorrect audit attribution by sending mismatched values. Build the audit target from the fetched moderation log (or validate `logId` belongs to `userEmail`) before writing the audit entry.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+189
to
+201
| void auditLog({ | ||
| actorEmail: email || "unknown", | ||
| targetEmail: doubt.userEmail, | ||
| action: AUDIT_ACTIONS.DOUBT_SOLVED, | ||
| resourceType: "doubt", | ||
| resourceId: doubtId, | ||
| metadata: { | ||
| previousStatus: doubt.isSolved, | ||
| newStatus, | ||
| replyId: newSolvedReplyId, | ||
| solvedReplyId: newSolvedReplyId, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Suggestion: The audit event type is hard-coded to DOUBT_SOLVED even when the computed newStatus can be unsolved or in-progress, so unsolve/status-change operations will be recorded as solved events. Use an action that matches the resulting status (or introduce separate actions for solved vs unsolved/status-changed) to avoid incorrect audit history. [logic error]
Severity Level: Major ⚠️
- ❌ Unsolve operations recorded as DOUBT_SOLVED in audit trail.
- ⚠️ Admins cannot trust status changes in audit history.Steps of Reproduction ✅
1. From the doubt detail UI, open the replies modal and click "Mark as solution" on an
already-solved reply, which triggers `handleMarkAsSolution()` in
`src/components/DoubtRepliesModal.tsx:15-23`. This sends a PATCH request to
`/api/doubts/action/{doubt.id}` with body `{ action: "solve", userName, replyId }`.
2. The PATCH handler in `src/app/api/doubts/action/[id]/route.ts:14-33` loads the doubt
and permissions, then enters the `if (action === "solve")` block at `route.ts:117-204`.
3. Because the clicked reply is already the solved one, the code in `route.ts:166-173`
executes the branch `if (replyId && doubt.solvedReplyId === replyId)`, setting `newStatus
= DOUBT_STATUS.UNSOLVED` (`route.ts:169`) and `newSolvedReplyId = null` (`route.ts:170`),
so the doubt is being explicitly unsolved.
4. The handler updates the database with `isSolved: newStatus` and `solvedReplyId:
newSolvedReplyId` at `route.ts:181-187`, correctly persisting the unsolved state, but then
immediately calls `auditLog` at `route.ts:189-201` with `action:
AUDIT_ACTIONS.DOUBT_SOLVED` regardless of `newStatus`. As a result, this unsolve operation
is recorded in `audit_logs` (`src/configs/schema.ts:68-80`) as a `DOUBT_SOLVED` action.
When admins review the Audit Logs UI (`src/app/admin/audit/page.tsx:14-31` and
`src/components/admin/AuditLogTable.tsx:31-45`), the badge and action label show "DOUBT
SOLVED" even though the final status in `metadata.newStatus` is `unsolved`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/doubts/action/[id]/route.ts
**Line:** 189:201
**Comment:**
*Logic Error: The audit event type is hard-coded to `DOUBT_SOLVED` even when the computed `newStatus` can be `unsolved` or `in-progress`, so unsolve/status-change operations will be recorded as solved events. Use an action that matches the resulting status (or introduce separate actions for solved vs unsolved/status-changed) to avoid incorrect audit history.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+19
to
+31
| if (res.redirected && res.url.includes('/403')) { | ||
| window.location.href = '/403'; | ||
| return; | ||
| } | ||
| if (!res.ok) { | ||
| if (res.status === 403) { | ||
| window.location.href = '/403'; | ||
| return; | ||
| } | ||
| throw new Error("Failed to fetch audit logs"); | ||
| } | ||
| const json = await res.json(); | ||
| setData(json); |
There was a problem hiding this comment.
Suggestion: The client only handles redirects to /403, but requireAdmin() can also redirect to /sign-in; in that case this code proceeds to parse redirected HTML as JSON and falls into the error state instead of navigating to login. Handle redirected /sign-in (or any redirected non-API URL) before res.json(). [logic error]
Severity Level: Major ⚠️
- ⚠️ Unauthenticated users see generic error instead of login.
- ⚠️ Admin audit page UX degrades when session expires.
- ⚠️ Inconsistent handling of `/403` vs `/sign-in` redirects.Steps of Reproduction ✅
1. Ensure you are logged out (no Clerk session) and navigate in the browser to
`/admin/audit`, which renders `AdminAuditPage` from `src/app/admin/audit/page.tsx:8`.
2. On mount, `useEffect` at `src/app/admin/audit/page.tsx:40-42` calls `fetchData()`,
which issues a GET to `/api/admin/audit` at `src/app/admin/audit/page.tsx:17`.
3. In the API route `GET` handler at `src/app/api/admin/audit/route.ts:7-51`,
`requireAdmin()` at `src/lib/auth/requireAdmin.ts:13-23` sees no current user and calls
`redirect("/sign-in")`, which Next turns into an HTTP redirect response to `/sign-in`.
4. The client-side `fetchData()` receives this redirect response: `res.redirected ===
true` and `res.url` contains `/sign-in`, but the code only handles `/403` redirects at
`src/app/admin/audit/page.tsx:19-22`; it then falls through to `if (!res.ok)` at line 23,
throws `new Error("Failed to fetch audit logs")` at line 28, and the catch block at line
33 sets `error`, so the user sees the generic error UI instead of being navigated to
`/sign-in`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/admin/audit/page.tsx
**Line:** 19:31
**Comment:**
*Logic Error: The client only handles redirects to `/403`, but `requireAdmin()` can also redirect to `/sign-in`; in that case this code proceeds to parse redirected HTML as JSON and falls into the error state instead of navigating to login. Handle redirected `/sign-in` (or any redirected non-API URL) before `res.json()`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+11
to
+14
| const { searchParams } = new URL(request.url); | ||
| const page = parseInt(searchParams.get("page") || "1"); | ||
| const limit = parseInt(searchParams.get("limit") || "20"); | ||
| const offset = (page - 1) * limit; |
There was a problem hiding this comment.
Suggestion: page/limit are parsed but never validated; values like page=0, negative numbers, or non-numeric input can produce invalid offsets/limits and trigger runtime query errors or incorrect pagination behavior. Validate and clamp these query params to safe positive integers before using them in SQL. [type error]
Severity Level: Major ⚠️
- ⚠️ Malformed `page`/`limit` yield 500s on admin API.
- ⚠️ Database may be hit with invalid LIMIT/OFFSET values.
- ⚠️ Lack of validation complicates debugging client mistakes.Steps of Reproduction ✅
1. Call the admin audit API directly with an invalid `page` from any HTTP client, e.g.
`GET /api/admin/audit?page=foo&limit=20`, which routes to `GET` in
`src/app/api/admin/audit/route.ts:7-51`.
2. Inside the handler, `page` is parsed via `parseInt(searchParams.get("page") || "1")` at
`src/app/api/admin/audit/route.ts:12`; with `page=foo`, `parseInt("foo")` returns `NaN`,
and `limit` is parsed similarly at line 13.
3. `offset` is computed as `(page - 1) * limit` at `src/app/api/admin/audit/route.ts:14`,
resulting in `NaN` (or a negative number if `page=0` or `page<0`), and this `page`,
`limit`, and `offset` are passed into the Drizzle queries at
`src/app/api/admin/audit/route.ts:16-34`, producing a SQL query with invalid
`LIMIT`/`OFFSET` semantics for the underlying database driver.
4. When the database rejects this malformed pagination, the thrown error is caught by the
`catch` block at `src/app/api/admin/audit/route.ts:44-49`, logged as `"Error fetching
audit logs"`, and the route returns `HTTP 500` with `{ error: "Internal Server Error" }`
instead of a clean `400` validation error.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/admin/audit/route.ts
**Line:** 11:14
**Comment:**
*Type Error: `page`/`limit` are parsed but never validated; values like `page=0`, negative numbers, or non-numeric input can produce invalid offsets/limits and trigger runtime query errors or incorrect pagination behavior. Validate and clamp these query params to safe positive integers before using them in SQL.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+336
to
+339
| }, (table) => ({ | ||
| actorEmailIndex: index("audit_actor_idx").on(table.actorEmail), | ||
| actionIndex: index("audit_action_idx").on(table.action), | ||
| })); |
There was a problem hiding this comment.
Suggestion: The audit log listing endpoint orders by createdAt DESC, but this table only adds indexes on actorEmail and action; without an index covering createdAt, pagination will degrade into expensive sorts/scans as the audit table grows. Add a createdAt (or composite pagination) index to support this query pattern. [performance]
Severity Level: Major ⚠️
- ⚠️ Audit listing queries become slower as table grows.
- ⚠️ Admin UI pagination latency increases under real workloads.
- ⚠️ Database CPU usage rises for audit log reads.Steps of Reproduction ✅
1. Review the `audit_logs` migration at `drizzle/0008_audit_logs.sql:1-16`, which creates
indexes `audit_actor_idx` on `"actor_email"` and `audit_action_idx` on `"action"`, but no
index on `"created_at"`.
2. Confirm that the Drizzle schema at `src/configs/schema.ts:327-339` mirrors this by
defining `auditLogsTable` with indexes only on `actorEmail` and `action` via
`actorEmailIndex` and `actionIndex` at `src/configs/schema.ts:337-338`; no index is
declared on `createdAt`.
3. The admin listing API at `src/app/api/admin/audit/route.ts:19-34` selects from
`auditLogsTable` and orders results by `desc(auditLogsTable.createdAt)` (line 32), with
`limit` and `offset` pagination on that sort column.
4. As `audit_logs` grows (it records every moderation/doubt action via `auditLog` in
`src/lib/audit.ts:32-41` and its callers), the database must perform a full table sort on
`"created_at"` for each paginated request because there is no supporting index, leading to
increasingly slow responses and higher load for the `/api/admin/audit` endpoint.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/configs/schema.ts
**Line:** 336:339
**Comment:**
*Performance: The audit log listing endpoint orders by `createdAt DESC`, but this table only adds indexes on `actorEmail` and `action`; without an index covering `createdAt`, pagination will degrade into expensive sorts/scans as the audit table grows. Add a `createdAt` (or composite pagination) index to support this query pattern.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| imageUrl: z.union([safeUrl, z.literal('')]).optional().nullable().transform(e => e === '' ? null : e), | ||
| userName: trimmedString.max(255).optional(), | ||
| replyId: positiveInt.optional().nullable(), | ||
| status: z.enum(["unsolved", "in-progress", "solved"]).optional().nullable(), |
There was a problem hiding this comment.
Suggestion: status is declared as optional().nullable(), but the solve route treats any defined value (including null) as an explicit status and rejects it; this creates a schema/handler contract mismatch where a payload accepted by validation is then rejected by business logic. Remove nullable() (or transform null to undefined) so validation aligns with route behavior. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Clients sending `status: null` get confusing 400 errors.
- ⚠️ Validation schemas do not reflect route semantics.
- ⚠️ Harder to reason about acceptable payload shapes for solve.Steps of Reproduction ✅
1. Observe that `updateDoubtActionSchema` in `src/lib/validations/doubt.ts:17-25` defines
`status` as `z.enum(["unsolved", "in-progress", "solved"]).optional().nullable()`, so a
request body with `"status": null` passes Zod validation.
2. The `PATCH` handler for doubt actions at
`src/app/api/doubts/action/[id]/route.ts:14-204` uses `parseAndValidateRequest(req,
updateDoubtActionSchema)` at lines 16-17, so `data.status` can be `null` when validation
succeeds.
3. In the `"solve"` branch at `src/app/api/doubts/action/[id]/route.ts:117-148`, the code
determines `newStatus`: `if (status !== undefined) { ... } else { ... }` at lines 138-145;
when `status === null`, this condition is true, and it calls `isValidDoubtStatus(status)`
at line 140.
4. `isValidDoubtStatus` in `src/lib/doubtStatus.ts:37-42` only returns `true` for
`"unsolved"`, `"in-progress"`, or `"solved"`, so `isValidDoubtStatus(null)` is `false`;
the handler then returns `HTTP 400` with `{ error: "Invalid status value" }` at
`src/app/api/doubts/action/[id]/route.ts:141`, despite the payload having already passed
schema validation, creating a contract mismatch between validation and business logic.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/validations/doubt.ts
**Line:** 24:24
**Comment:**
*Api Mismatch: `status` is declared as `optional().nullable()`, but the solve route treats any defined value (including `null`) as an explicit status and rejects it; this creates a schema/handler contract mismatch where a payload accepted by validation is then rejected by business logic. Remove `nullable()` (or transform `null` to `undefined`) so validation aligns with route behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (6)
src/app/api/admin/audit/route.ts (1)
7-51: 🏗️ Heavy liftConsider adding filtering capabilities for admin investigations.
The endpoint currently supports only pagination. Administrators investigating incidents would benefit from filtering by:
- Action type (e.g., show only USER_BLOCKED events)
- Date range (e.g., last 7 days)
- Actor or target email
- Resource type/ID
This would significantly improve the utility of the audit log viewer.
💡 Example filter implementation
const action = searchParams.get("action"); const actorEmail = searchParams.get("actorEmail"); const startDate = searchParams.get("startDate"); const endDate = searchParams.get("endDate"); let query = db.select({...}).from(auditLogsTable).leftJoin(...); if (action) { query = query.where(eq(auditLogsTable.action, action)); } if (actorEmail) { query = query.where(eq(auditLogsTable.actorEmail, actorEmail)); } // Add date range filters, etc. const logs = await query.orderBy(desc(auditLogsTable.createdAt)) .limit(limit) .offset(offset);🤖 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/app/api/admin/audit/route.ts` around lines 7 - 51, The GET handler currently only supports pagination; extend it to accept filter query params (e.g., action, actorEmail, targetEmail, resourceType, resourceId, startDate, endDate) from the URL searchParams and apply them to the DB query before ordering/limiting; specifically parse params in GET, build the base query that selects from auditLogsTable and leftJoin usersTable (as in the existing db.select(...) call) and conditionally add .where(eq(auditLogsTable.action, action)) / .where(eq(auditLogsTable.actorEmail, actorEmail)) / range filters for createdAt using startDate/endDate (and partial matches for targetEmail/resourceId if desired), then run orderBy(desc(auditLogsTable.createdAt)).limit(limit).offset(offset) to return filtered logs and pagination metadata.src/components/admin/AuditLogTable.tsx (1)
106-108: ⚖️ Poor tradeoffConsider truncating long metadata for better mobile UX.
The
max-w-xs(max-width: 20rem) on the metadata<pre>tag may still be too wide on mobile devices, especially when nested JSON is pretty-printed. Consider adding a collapsible detail/summary or modal for full metadata viewing.Based on coding guidelines: TSX files should consider responsive design.
📱 Example: Collapsible metadata
{log.metadata ? ( <details className="text-xs"> <summary className="cursor-pointer text-primary hover:underline"> View metadata </summary> <pre className="mt-1 bg-muted p-2 rounded overflow-x-auto"> {JSON.stringify(JSON.parse(log.metadata), null, 2)} </pre> </details> ) : ( <span className="text-muted-foreground">-</span> )}🤖 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/components/admin/AuditLogTable.tsx` around lines 106 - 108, AuditLogTable's metadata <pre> currently renders JSON via JSON.parse(log.metadata) with a fixed max-w-xs, which can overflow on mobile; replace the direct <pre> with a responsive, collapsible viewer (e.g., a <details>/<summary> or button that opens a modal) that shows a short "View metadata" summary and only expands to the pretty-printed JSON when opened, remove or avoid the hard max-w-xs class, and ensure you guard parsing (log.metadata) with a safe check/try-catch or conditional so malformed or empty metadata renders a fallback (e.g., '-' or "invalid metadata").Source: Coding guidelines
src/configs/schema.ts (1)
333-333: ⚡ Quick winConsider using
text()instead ofvarchar({ length: 255 })forresourceId.The current varchar(255) constraint may be limiting if resourceId needs to store composite keys, UUIDs, or longer identifiers in the future. Since resourceId is already nullable and variable-length, using
text()provides more flexibility without schema migrations.♻️ Proposed change
- resourceId: varchar({ length: 255 }), + resourceId: text(),Note: This would also require updating the SQL migration.
🤖 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/configs/schema.ts` at line 333, Change the resourceId column definition in the schema from varchar({ length: 255 }) to text() to allow arbitrarily long identifiers: locate the resourceId field in the table/schema definition (the line containing resourceId: varchar({ length: 255 })) and replace it with resourceId: text(); then update the corresponding SQL migration to alter the column type (or recreate the column) so the DB matches the new schema and run/verify migrations.src/app/admin/audit/page.tsx (1)
76-98: ⚡ Quick winPagination controls may overflow on mobile devices.
The pagination section uses a horizontal flex layout without responsive breakpoints. On narrow screens, the text and buttons might wrap awkwardly or overflow.
Based on coding guidelines: TSX files should consider responsive design.
📱 Add responsive classes
-<div className="flex items-center justify-end space-x-2 py-4"> +<div className="flex flex-col sm:flex-row items-center justify-end gap-2 py-4"> <button onClick={() => setPage(p => Math.max(1, p - 1))} disabled={page === 1} aria-label="Previous page" - className="inline-flex items-center justify-center whitespace-nowrap rounded-md text-sm font-medium transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 border border-input bg-background shadow-sm hover:bg-accent hover:text-accent-foreground h-9 px-4 py-2" + className="w-full sm:w-auto inline-flex items-center justify-center whitespace-nowrap rounded-md text-sm font-medium transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 border border-input bg-background shadow-sm hover:bg-accent hover:text-accent-foreground h-9 px-4 py-2" > Previous </button> - <div className="text-sm text-muted-foreground"> + <div className="text-sm text-muted-foreground order-first sm:order-none"> Page {page} of {Math.ceil(data.pagination.total / data.pagination.limit)} </div> <button onClick={() => setPage(p => p + 1)} disabled={page >= Math.ceil(data.pagination.total / data.pagination.limit)} aria-label="Next page" - className="inline-flex items-center justify-center whitespace-nowrap rounded-md text-sm font-medium transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 border border-input bg-background shadow-sm hover:bg-accent hover:text-accent-foreground h-9 px-4 py-2" + className="w-full sm:w-auto inline-flex items-center justify-center whitespace-nowrap rounded-md text-sm font-medium transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 border border-input bg-background shadow-sm hover:bg-accent hover:text-accent-foreground h-9 px-4 py-2" > Next </button> </div>🤖 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/app/admin/audit/page.tsx` around lines 76 - 98, The pagination controls can overflow on narrow screens; update the container around the buttons and page text (the div that renders when data.pagination.total > data.pagination.limit) to be responsive (e.g., use flex-col on small screens and flex-row on larger: "flex flex-col sm:flex-row items-center sm:justify-end space-y-2 sm:space-y-0 sm:space-x-2 py-4" or similar) and make the Previous/Next buttons responsive so they can shrink/wrap (e.g., add classes like "w-full sm:w-auto" or "flex-1 sm:flex-none" to the button elements used with setPage and page) and ensure the page label remains centered/stacked correctly; this uses the existing symbols data.pagination, setPage, and page so you can locate and update the correct JSX.Source: Coding guidelines
drizzle/0008_audit_logs.sql (1)
1-10: ⚡ Quick winConsider adding indexes for common query patterns.
While actor and action indexes are present, consider adding indexes for resource-based lookups. Administrators may want to find all audit entries for a specific resource (e.g., "show me all actions on doubt
#123").📊 Suggested additional indexes
-- For resource-specific queries CREATE INDEX "audit_resource_idx" ON "audit_logs" ("resource_type", "resource_id"); -- For target-based queries (e.g., all actions affecting a user) CREATE INDEX "audit_target_idx" ON "audit_logs" ("target_email");🤖 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 `@drizzle/0008_audit_logs.sql` around lines 1 - 10, Add indexes to speed up resource- and target-based lookups on the audit_logs table: in the same migration that creates "audit_logs" add CREATE INDEX statements for ("resource_type","resource_id") and for ("target_email") — e.g., create indexes named audit_resource_idx on columns resource_type, resource_id and audit_target_idx on column target_email so queries like “all actions on a given resource” or “all actions affecting a user” are efficient.src/app/api/doubts/action/[id]/route.ts (1)
195-199: 💤 Low valueConsider removing redundant metadata field.
Both
replyIdandsolvedReplyIdstore the same value (newSolvedReplyId). This redundancy doesn't break functionality but adds confusion. Consider keeping onlysolvedReplyIdto match the domain model.♻️ Simplify metadata
metadata: { previousStatus: doubt.isSolved, newStatus, - replyId: newSolvedReplyId, solvedReplyId: newSolvedReplyId, },🤖 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/app/api/doubts/action/`[id]/route.ts around lines 195 - 199, The metadata object is duplicating the same value under replyId and solvedReplyId; remove the redundant replyId key and keep only solvedReplyId (set to newSolvedReplyId) in the metadata payload. Update any code that reads metadata.replyId to use metadata.solvedReplyId instead (search for uses of metadata.replyId in handlers, loggers, or types) and adjust types/interfaces if necessary so only solvedReplyId is defined. Ensure the created metadata still includes previousStatus and newStatus unchanged.
🤖 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 `@drizzle/0008_audit_logs.sql`:
- Around line 12-16: Add an index on the audit_logs.created_at column to support
the admin API's ORDER BY auditLogsTable.createdAt DESC; update the migration
adding a new index (e.g., audit_created_at_idx) on "created_at" (consider DESC)
so queries ordered by created_at are fast; optionally replace or add a composite
index named audit_actor_created_idx on ("actor_email", "created_at" DESC) if you
also want optimized actor/time-range lookups referenced by the admin audit
route.
In `@src/app/admin/audit/page.tsx`:
- Line 9: The state variable data currently uses the unsafe any type; define a
proper interface (e.g., AuditLogResponse with logs array and pagination shape
and inner log fields like id, actorEmail, targetEmail, action, resourceType,
resourceId, metadata, createdAt, actorName) and update the useState call to use
useState<AuditLogResponse | null>(null) and adjust setData usages to respect the
new type; ensure any mapping/consumption of data (e.g., rendering logs) uses the
new typed properties.
- Line 46: The error container div (the element with className "flex flex-col
items-center justify-center h-64 text-destructive") needs ARIA attributes and
the decorative icon must be hidden from assistive tech: add role="alert" and
aria-live="assertive" (and aria-atomic="true") to the error container to ensure
screen readers announce the message, and mark the decorative icon element (the
SVG or Icon component next to the message) with aria-hidden="true" (or
role="presentation") so it is ignored by screen readers; also consider using a
semantic element (e.g., <section> or <aside>) or aria-labelledby pointing to the
error text if there are multiple landmarks.
- Around line 47-48: Extract all hardcoded user-facing strings in
src/app/admin/audit/page.tsx into a single constant object (e.g.,
AUDIT_PAGE_STRINGS) and replace in-component literals with references to that
constant; specifically replace "Error loading audit logs" used in the error
block, the h1 title near the FileText icon ("Audit Logs"), the page description
("Track administrative and moderation actions for accountability."), the section
heading ("Recent Actions"), and pagination labels ("Previous", "Next", "Page
{page} of {total}") with properties on AUDIT_PAGE_STRINGS; keep the object as a
const (as const) so it’s easy to reuse for i18n and import/ export if needed.
- Around line 40-42: The effect that calls fetchData when page changes can race
because multiple requests may complete out of order; modify the useEffect and
fetchData to use an AbortController: create a controller inside useEffect, pass
controller.signal into fetchData (or the underlying fetch call) so the in-flight
request can be cancelled, handle the AbortError in fetchData by ignoring aborted
responses, and call controller.abort() in the effect cleanup to cancel previous
requests when page changes. Ensure fetchData (or its internal fetch) accepts a
signal parameter and does not call setState when the request was aborted.
- Around line 65-68: The current conditional rendering only shows the Skeleton
when (loading && !data), causing the skeleton to disappear on subsequent loads;
update the render logic around the Skeleton/AuditLogTable so the skeleton or a
semi-opaque loading overlay is shown whenever loading is true (not just when
data is absent). Concretely, modify the JSX that currently checks loading &&
!data (the block rendering Skeleton) to either always render the Skeleton when
loading or wrap the AuditLogTable (AuditLogTable component and its parent div)
in a relative container and render an absolute overlay when loading is true so
pagination/refresh loads show a visible indicator while preserving existing
data.
In `@src/app/api/admin/audit/route.ts`:
- Around line 11-14: The pagination parsing currently assigns page/limit
directly from parseInt and computes offset, which allows NaN, zero/negative or
huge values; update the code around parseInt(searchParams.get(...)) to validate
and sanitize inputs: parse the values, treat NaN by falling back to defaults
(page=1, limit=20), enforce page = Math.max(1, page) and clamp limit to a safe
range like 1..100 (e.g., limit = Math.min(100, Math.max(1, limit))), then
recompute offset = (page - 1) * limit; optionally return a 400 error if inputs
are wildly malformed instead of silently coercing. Ensure you change the
variables page, limit and offset where they are declared so downstream logic
uses the sanitized values.
In `@src/app/api/admin/moderation/action/route.ts`:
- Around line 12-13: The current fallback to the literal "admin" erases
auditability; update the route handler so that after calling currentUser() you
verify adminUser and adminUser.primaryEmailAddress?.emailAddress exist and, if
not, fail the request immediately (e.g., throw/return an
authentication/validation error) instead of assigning "admin" to
adminEmail—locate the adminUser and adminEmail variables in route.ts and replace
the fallback assignment with a check that rejects the request when the actor is
unknown.
In `@src/app/api/doubts/action/`[id]/route.ts:
- Around line 322-332: The actorEmail fallback "unknown" weakens audit
integrity; in the auditLog call replace actorEmail: email || "unknown" with
actorEmail: email (or otherwise assert/throw if email is absent) so the real
authenticated email is always recorded; locate the auditLog invocation in
route.ts (the block that builds metadata with subject/classroomId for
AUDIT_ACTIONS.DOUBT_DELETED) and remove the "unknown" fallback, relying on the
earlier permission checks that guarantee email is present.
- Around line 260-275: The audit log currently uses a fallback string "unknown"
for actorEmail which breaks accountability; update the auditLog call in route.ts
(the call that passes actorEmail, AUDIT_ACTIONS.DOUBT_EDITED, resourceId
doubtId, etc.) to use the authenticated email directly (remove the "unknown"
fallback) and, if needed for TypeScript, assert non-null (e.g., use email or
email! in the actorEmail field) since the earlier ownership/authentication check
guarantees email is present.
- Around line 189-201: The audit entry currently uses a fallback string
"unknown" for actorEmail which breaks accountability; update the auditLog call
in route.ts to use the authenticated email directly (remove the "unknown"
fallback) and, to be safe, add an explicit guard or assert earlier in the
handler to ensure email exists before reaching the auditLog invocation.
Specifically, adjust the auditLog invocation that sets actorEmail to use email
(not email || "unknown") and ensure the surrounding permission/auth checks that
set email (used with AUDIT_ACTIONS.DOUBT_SOLVED, doubtId, newSolvedReplyId, and
doubt.userEmail) enforce/throw if email is missing so the audit always records a
real actor.
In `@src/components/admin/AuditLogTable.tsx`:
- Around line 52-53: Extract all user-facing hardcoded strings from the
AuditLogTable component into a single constant object (e.g.,
AUDIT_TABLE_STRINGS) and replace inline literals with references to that object;
specifically move "No audit logs found" and "No actions have been logged yet."
(used in the <h3> and <p>), and all table header labels (e.g., "Actor",
"Action", "Resource", "Target", "Metadata", "Date") and any UI prefixes like
"ID: " into the AUDIT_TABLE_STRINGS object, then update usages in AuditLogTable
(e.g., the <h3>, <p>, TableHead, and any render locations) to read from
AUDIT_TABLE_STRINGS instead of hardcoded strings.
- Line 51: The decorative icon usages in AuditLogTable.tsx (e.g., the <FileText
... /> JSX instance and the other icon JSX occurrences present later in the
file) should include aria-hidden="true" so screen readers ignore them; update
each decorative icon component instance (FileText and the other icon JSX usages
noted in the review) to add the aria-hidden="true" prop while leaving any
informative/interactive icons unchanged.
- Line 107: The render currently calls JSON.parse(log.metadata) directly in
AuditLogTable which will throw on malformed JSON; wrap the parse in a try-catch
(or use a safeParse helper) when rendering each log entry (i.e., around
JSON.parse(log.metadata) in the AuditLogTable render) and fall back to a safe
string (e.g., original log.metadata or an "invalid JSON" message) so the
component doesn't crash; ensure any caught error is optionally logged
(console.warn or a logger) for debugging.
- Around line 60-70: Add a visually hidden caption for screen readers to the
audit log table to describe its purpose: inside the AuditLogTable component,
locate the JSX where <Table> and <TableHeader> are rendered (the
Table/TableHeader/TableRow block) and insert a <caption> element (using the
existing sr-only utility class) that succinctly describes the table (e.g.,
"Audit log of user actions"); ensure the caption appears as the first child of
<Table> so screen readers announce it before the table content.
In `@src/configs/schema.ts`:
- Around line 327-339: The schema for auditLogsTable is missing an index on
createdAt; update the table definition (auditLogsTable) to add a createdAt index
in the third-argument indexes object (similar to actorEmailIndex/actionIndex) by
adding something like createdAtIndex:
index("audit_created_at_idx").on(table.createdAt) so queries that ORDER BY
createdAt use the index for better performance.
---
Nitpick comments:
In `@drizzle/0008_audit_logs.sql`:
- Around line 1-10: Add indexes to speed up resource- and target-based lookups
on the audit_logs table: in the same migration that creates "audit_logs" add
CREATE INDEX statements for ("resource_type","resource_id") and for
("target_email") — e.g., create indexes named audit_resource_idx on columns
resource_type, resource_id and audit_target_idx on column target_email so
queries like “all actions on a given resource” or “all actions affecting a user”
are efficient.
In `@src/app/admin/audit/page.tsx`:
- Around line 76-98: The pagination controls can overflow on narrow screens;
update the container around the buttons and page text (the div that renders when
data.pagination.total > data.pagination.limit) to be responsive (e.g., use
flex-col on small screens and flex-row on larger: "flex flex-col sm:flex-row
items-center sm:justify-end space-y-2 sm:space-y-0 sm:space-x-2 py-4" or
similar) and make the Previous/Next buttons responsive so they can shrink/wrap
(e.g., add classes like "w-full sm:w-auto" or "flex-1 sm:flex-none" to the
button elements used with setPage and page) and ensure the page label remains
centered/stacked correctly; this uses the existing symbols data.pagination,
setPage, and page so you can locate and update the correct JSX.
In `@src/app/api/admin/audit/route.ts`:
- Around line 7-51: The GET handler currently only supports pagination; extend
it to accept filter query params (e.g., action, actorEmail, targetEmail,
resourceType, resourceId, startDate, endDate) from the URL searchParams and
apply them to the DB query before ordering/limiting; specifically parse params
in GET, build the base query that selects from auditLogsTable and leftJoin
usersTable (as in the existing db.select(...) call) and conditionally add
.where(eq(auditLogsTable.action, action)) / .where(eq(auditLogsTable.actorEmail,
actorEmail)) / range filters for createdAt using startDate/endDate (and partial
matches for targetEmail/resourceId if desired), then run
orderBy(desc(auditLogsTable.createdAt)).limit(limit).offset(offset) to return
filtered logs and pagination metadata.
In `@src/app/api/doubts/action/`[id]/route.ts:
- Around line 195-199: The metadata object is duplicating the same value under
replyId and solvedReplyId; remove the redundant replyId key and keep only
solvedReplyId (set to newSolvedReplyId) in the metadata payload. Update any code
that reads metadata.replyId to use metadata.solvedReplyId instead (search for
uses of metadata.replyId in handlers, loggers, or types) and adjust
types/interfaces if necessary so only solvedReplyId is defined. Ensure the
created metadata still includes previousStatus and newStatus unchanged.
In `@src/components/admin/AuditLogTable.tsx`:
- Around line 106-108: AuditLogTable's metadata <pre> currently renders JSON via
JSON.parse(log.metadata) with a fixed max-w-xs, which can overflow on mobile;
replace the direct <pre> with a responsive, collapsible viewer (e.g., a
<details>/<summary> or button that opens a modal) that shows a short "View
metadata" summary and only expands to the pretty-printed JSON when opened,
remove or avoid the hard max-w-xs class, and ensure you guard parsing
(log.metadata) with a safe check/try-catch or conditional so malformed or empty
metadata renders a fallback (e.g., '-' or "invalid metadata").
In `@src/configs/schema.ts`:
- Line 333: Change the resourceId column definition in the schema from varchar({
length: 255 }) to text() to allow arbitrarily long identifiers: locate the
resourceId field in the table/schema definition (the line containing resourceId:
varchar({ length: 255 })) and replace it with resourceId: text(); then update
the corresponding SQL migration to alter the column type (or recreate the
column) so the DB matches the new schema and run/verify migrations.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9909bb12-ee3a-4fd7-aa79-8749f56043d8
📒 Files selected for processing (12)
drizzle/0008_audit_logs.sqlsrc/__tests__/api/replies-action.test.tssrc/app/admin/audit/page.tsxsrc/app/api/admin/audit/route.tssrc/app/api/admin/moderation/action/route.tssrc/app/api/doubts/[id]/pin/route.tssrc/app/api/doubts/action/[id]/route.tssrc/app/api/replies/action/[id]/route.tssrc/components/admin/AuditLogTable.tsxsrc/configs/schema.tssrc/lib/audit.tssrc/lib/validations/doubt.ts
Comment on lines
+12
to
+16
| CREATE INDEX "audit_actor_idx" | ||
| ON "audit_logs" ("actor_email"); | ||
|
|
||
| CREATE INDEX "audit_action_idx" | ||
| ON "audit_logs" ("action"); No newline at end of file |
Contributor
There was a problem hiding this comment.
Add index on created_at for query performance.
The admin API endpoint (src/app/api/admin/audit/route.ts:32) orders results by created_at DESC, but there's no index on this column. As the audit log grows, queries will become slower.
The API orders by auditLogsTable.createdAt descending, so an index here will significantly improve performance.
📈 Recommended index addition
CREATE INDEX "audit_action_idx"
ON "audit_logs" ("action");
+
+CREATE INDEX "audit_created_at_idx"
+ON "audit_logs" ("created_at" DESC);Alternatively, consider a composite index for actor-based time-range queries:
CREATE INDEX "audit_actor_created_idx"
ON "audit_logs" ("actor_email", "created_at" DESC);🤖 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 `@drizzle/0008_audit_logs.sql` around lines 12 - 16, Add an index on the
audit_logs.created_at column to support the admin API's ORDER BY
auditLogsTable.createdAt DESC; update the migration adding a new index (e.g.,
audit_created_at_idx) on "created_at" (consider DESC) so queries ordered by
created_at are fast; optionally replace or add a composite index named
audit_actor_created_idx on ("actor_email", "created_at" DESC) if you also want
optimized actor/time-range lookups referenced by the admin audit route.
| import { FileText } from "lucide-react"; | ||
|
|
||
| export default function AdminAuditPage() { | ||
| const [data, setData] = useState<any>(null); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace any type with proper interface.
The data state uses any, which eliminates type safety. Define a proper interface matching the API response shape.
🔧 Define proper types
interface AuditLogResponse {
logs: Array<{
id: number;
actorEmail: string;
targetEmail: string | null;
action: string;
resourceType: string;
resourceId: string | null;
metadata: string | null;
createdAt: string;
actorName: string | null;
}>;
pagination: {
page: number;
limit: number;
total: number;
};
}
const [data, setData] = useState<AuditLogResponse | null>(null);🤖 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/app/admin/audit/page.tsx` at line 9, The state variable data currently
uses the unsafe any type; define a proper interface (e.g., AuditLogResponse with
logs array and pagination shape and inner log fields like id, actorEmail,
targetEmail, action, resourceType, resourceId, metadata, createdAt, actorName)
and update the useState call to use useState<AuditLogResponse | null>(null) and
adjust setData usages to respect the new type; ensure any mapping/consumption of
data (e.g., rendering logs) uses the new typed properties.
Comment on lines
+40
to
+42
| useEffect(() => { | ||
| fetchData(); | ||
| }, [page]); |
Contributor
There was a problem hiding this comment.
Prevent race conditions from rapid pagination clicks.
If a user clicks pagination buttons quickly, multiple fetch requests can be in flight simultaneously. The last request to complete will set the data, which may not correspond to the current page state, showing incorrect results.
🔄 Add AbortController
useEffect(() => {
+ const controller = new AbortController();
+
+ const fetchDataWithAbort = async () => {
+ setLoading(true);
+ try {
+ const res = await fetch(`/api/admin/audit?page=${page}&limit=20`, {
+ signal: controller.signal
+ });
+ // ... rest of fetch logic
+ } catch (err: any) {
+ if (err.name === 'AbortError') return; // Ignore aborted requests
+ setError(err.message);
+ } finally {
+ setLoading(false);
+ }
+ };
+
+ fetchDataWithAbort();
- fetchData();
+ return () => controller.abort();
}, [page]);🤖 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/app/admin/audit/page.tsx` around lines 40 - 42, The effect that calls
fetchData when page changes can race because multiple requests may complete out
of order; modify the useEffect and fetchData to use an AbortController: create a
controller inside useEffect, pass controller.signal into fetchData (or the
underlying fetch call) so the in-flight request can be cancelled, handle the
AbortError in fetchData by ignoring aborted responses, and call
controller.abort() in the effect cleanup to cancel previous requests when page
changes. Ensure fetchData (or its internal fetch) accepts a signal parameter and
does not call setState when the request was aborted.
|
|
||
| if (error) { | ||
| return ( | ||
| <div className="flex flex-col items-center justify-center h-64 text-destructive"> |
Contributor
There was a problem hiding this comment.
Add accessibility attributes for better screen reader support.
The error container and decorative icon are missing proper ARIA attributes for accessibility.
Based on coding guidelines: TSX files should be reviewed for accessibility, including aria labels and proper semantic markup.
♿ Add ARIA attributes
-<div className="flex flex-col items-center justify-center h-64 text-destructive">
+<div className="flex flex-col items-center justify-center h-64 text-destructive" role="alert">
<p className="text-lg font-semibold">Error loading audit logs</p>
<p>{error}</p>
</div>
-<FileText className="w-8 h-8 text-primary" />
+<FileText className="w-8 h-8 text-primary" aria-hidden="true" />Also applies to: 57-57
🤖 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/app/admin/audit/page.tsx` at line 46, The error container div (the
element with className "flex flex-col items-center justify-center h-64
text-destructive") needs ARIA attributes and the decorative icon must be hidden
from assistive tech: add role="alert" and aria-live="assertive" (and
aria-atomic="true") to the error container to ensure screen readers announce the
message, and mark the decorative icon element (the SVG or Icon component next to
the message) with aria-hidden="true" (or role="presentation") so it is ignored
by screen readers; also consider using a semantic element (e.g., <section> or
<aside>) or aria-labelledby pointing to the error text if there are multiple
landmarks.
Source: Coding guidelines
Comment on lines
+47
to
+48
| <p className="text-lg font-semibold">Error loading audit logs</p> | ||
| <p>{error}</p> |
Contributor
There was a problem hiding this comment.
Extract hardcoded strings to constants.
As per coding guidelines for TSX files, hardcoded strings should be extracted to constants. This file contains multiple user-facing strings that should be centralized for maintainability and potential i18n support.
♻️ Suggested refactor
const AUDIT_PAGE_STRINGS = {
ERROR_TITLE: "Error loading audit logs",
PAGE_TITLE: "Audit Logs",
PAGE_DESCRIPTION: "Track administrative and moderation actions for accountability.",
SECTION_TITLE: "Recent Actions",
PAGINATION_PREVIOUS: "Previous",
PAGINATION_NEXT: "Next",
PAGINATION_PAGE_OF: "Page {page} of {total}",
} as const;
// Then use throughout the component:
<p className="text-lg font-semibold">{AUDIT_PAGE_STRINGS.ERROR_TITLE}</p>
<h1 className="text-3xl font-bold flex items-center gap-2">
<FileText className="w-8 h-8 text-primary" />
{AUDIT_PAGE_STRINGS.PAGE_TITLE}
</h1>
// ... etcBased on coding guidelines: TSX files should not have hardcoded strings; use constants instead.
Also applies to: 56-62, 73-73, 84-87, 95-95
🤖 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/app/admin/audit/page.tsx` around lines 47 - 48, Extract all hardcoded
user-facing strings in src/app/admin/audit/page.tsx into a single constant
object (e.g., AUDIT_PAGE_STRINGS) and replace in-component literals with
references to that constant; specifically replace "Error loading audit logs"
used in the error block, the h1 title near the FileText icon ("Audit Logs"), the
page description ("Track administrative and moderation actions for
accountability."), the section heading ("Recent Actions"), and pagination labels
("Previous", "Next", "Page {page} of {total}") with properties on
AUDIT_PAGE_STRINGS; keep the object as a const (as const) so it’s easy to reuse
for i18n and import/ export if needed.
Source: Coding guidelines
| if (logs.length === 0) { | ||
| return ( | ||
| <div className="flex flex-col items-center justify-center p-8 border rounded-lg bg-card text-center"> | ||
| <FileText className="h-12 w-12 text-muted-foreground mb-4" /> |
Contributor
There was a problem hiding this comment.
Add aria-hidden="true" to decorative icons.
Icons used for visual decoration should have aria-hidden="true" to prevent screen readers from announcing them, as they don't convey additional semantic meaning beyond the adjacent text.
Based on coding guidelines: TSX files should be reviewed for accessibility, including proper ARIA attributes.
♿ Add aria-hidden
-<FileText className="h-12 w-12 text-muted-foreground mb-4" />
+<FileText className="h-12 w-12 text-muted-foreground mb-4" aria-hidden="true" />
-<Shield className="h-4 w-4 text-muted-foreground" />
+<Shield className="h-4 w-4 text-muted-foreground" aria-hidden="true" />
-<User className="h-3 w-3" />
+<User className="h-3 w-3" aria-hidden="true" />Also applies to: 76-76, 97-97
🤖 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/components/admin/AuditLogTable.tsx` at line 51, The decorative icon
usages in AuditLogTable.tsx (e.g., the <FileText ... /> JSX instance and the
other icon JSX occurrences present later in the file) should include
aria-hidden="true" so screen readers ignore them; update each decorative icon
component instance (FileText and the other icon JSX usages noted in the review)
to add the aria-hidden="true" prop while leaving any informative/interactive
icons unchanged.
Source: Coding guidelines
Comment on lines
+52
to
+53
| <h3 className="text-lg font-medium">No audit logs found</h3> | ||
| <p className="text-sm text-muted-foreground">No actions have been logged yet.</p> |
Contributor
There was a problem hiding this comment.
Extract hardcoded strings to constants.
As per coding guidelines for TSX files, user-facing strings should not be hardcoded. This component contains multiple strings that should be centralized.
♻️ Suggested constants
const AUDIT_TABLE_STRINGS = {
EMPTY_STATE_TITLE: "No audit logs found",
EMPTY_STATE_MESSAGE: "No actions have been logged yet.",
HEADER_ACTOR: "Actor",
HEADER_ACTION: "Action",
HEADER_RESOURCE: "Resource",
HEADER_TARGET: "Target",
HEADER_METADATA: "Metadata",
HEADER_DATE: "Date",
RESOURCE_ID_PREFIX: "ID: ",
} as const;
// Then use throughout:
<h3 className="text-lg font-medium">{AUDIT_TABLE_STRINGS.EMPTY_STATE_TITLE}</h3>
<TableHead>{AUDIT_TABLE_STRINGS.HEADER_ACTOR}</TableHead>
// ... etcBased on coding guidelines: TSX files should not have hardcoded strings; use constants instead.
Also applies to: 63-68
🤖 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/components/admin/AuditLogTable.tsx` around lines 52 - 53, Extract all
user-facing hardcoded strings from the AuditLogTable component into a single
constant object (e.g., AUDIT_TABLE_STRINGS) and replace inline literals with
references to that object; specifically move "No audit logs found" and "No
actions have been logged yet." (used in the <h3> and <p>), and all table header
labels (e.g., "Actor", "Action", "Resource", "Target", "Metadata", "Date") and
any UI prefixes like "ID: " into the AUDIT_TABLE_STRINGS object, then update
usages in AuditLogTable (e.g., the <h3>, <p>, TableHead, and any render
locations) to read from AUDIT_TABLE_STRINGS instead of hardcoded strings.
Source: Coding guidelines
Comment on lines
+60
to
+70
| <Table> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHead>Actor</TableHead> | ||
| <TableHead>Action</TableHead> | ||
| <TableHead>Resource</TableHead> | ||
| <TableHead>Target</TableHead> | ||
| <TableHead>Metadata</TableHead> | ||
| <TableHead>Date</TableHead> | ||
| </TableRow> | ||
| </TableHeader> |
Contributor
There was a problem hiding this comment.
Add table caption for screen reader accessibility.
The table lacks a <caption> element describing its purpose. Screen reader users rely on captions to understand table content before navigating through it.
Based on coding guidelines: TSX files should be reviewed for accessibility.
♿ Add table caption
<Table>
+ <caption className="sr-only">
+ Audit log entries showing administrative and moderation actions
+ </caption>
<TableHeader>
<TableRow>The sr-only class hides the caption visually while keeping it accessible to screen readers.
📝 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
| <Table> | |
| <TableHeader> | |
| <TableRow> | |
| <TableHead>Actor</TableHead> | |
| <TableHead>Action</TableHead> | |
| <TableHead>Resource</TableHead> | |
| <TableHead>Target</TableHead> | |
| <TableHead>Metadata</TableHead> | |
| <TableHead>Date</TableHead> | |
| </TableRow> | |
| </TableHeader> | |
| <Table> | |
| <caption className="sr-only"> | |
| Audit log entries showing administrative and moderation actions | |
| </caption> | |
| <TableHeader> | |
| <TableRow> | |
| <TableHead>Actor</TableHead> | |
| <TableHead>Action</TableHead> | |
| <TableHead>Resource</TableHead> | |
| <TableHead>Target</TableHead> | |
| <TableHead>Metadata</TableHead> | |
| <TableHead>Date</TableHead> | |
| </TableRow> | |
| </TableHeader> |
🤖 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/components/admin/AuditLogTable.tsx` around lines 60 - 70, Add a visually
hidden caption for screen readers to the audit log table to describe its
purpose: inside the AuditLogTable component, locate the JSX where <Table> and
<TableHeader> are rendered (the Table/TableHeader/TableRow block) and insert a
<caption> element (using the existing sr-only utility class) that succinctly
describes the table (e.g., "Audit log of user actions"); ensure the caption
appears as the first child of <Table> so screen readers announce it before the
table content.
Source: Coding guidelines
| <TableCell> | ||
| {log.metadata ? ( | ||
| <pre className="text-xs bg-muted p-2 rounded max-w-xs overflow-x-auto"> | ||
| {JSON.stringify(JSON.parse(log.metadata), null, 2)} |
Contributor
There was a problem hiding this comment.
Add error handling for JSON.parse to prevent component crashes.
The JSON.parse(log.metadata) call on line 107 will throw if the metadata string is malformed. Since metadata is user-controlled (via audit log writes), malformed JSON could crash the entire table component.
🛡️ Add try-catch around JSON.parse
<TableCell>
{log.metadata ? (
- <pre className="text-xs bg-muted p-2 rounded max-w-xs overflow-x-auto">
- {JSON.stringify(JSON.parse(log.metadata), null, 2)}
- </pre>
+ (() => {
+ try {
+ return (
+ <pre className="text-xs bg-muted p-2 rounded max-w-xs overflow-x-auto">
+ {JSON.stringify(JSON.parse(log.metadata), null, 2)}
+ </pre>
+ );
+ } catch {
+ return (
+ <span className="text-xs text-destructive">Invalid metadata</span>
+ );
+ }
+ })()
) : (
<span className="text-muted-foreground">-</span>
)}
</TableCell>📝 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
| {JSON.stringify(JSON.parse(log.metadata), null, 2)} | |
| <TableCell> | |
| {log.metadata ? ( | |
| (() => { | |
| try { | |
| return ( | |
| <pre className="text-xs bg-muted p-2 rounded max-w-xs overflow-x-auto"> | |
| {JSON.stringify(JSON.parse(log.metadata), null, 2)} | |
| </pre> | |
| ); | |
| } catch { | |
| return ( | |
| <span className="text-xs text-destructive">Invalid metadata</span> | |
| ); | |
| } | |
| })() | |
| ) : ( | |
| <span className="text-muted-foreground">-</span> | |
| )} | |
| </TableCell> |
🤖 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/components/admin/AuditLogTable.tsx` at line 107, The render currently
calls JSON.parse(log.metadata) directly in AuditLogTable which will throw on
malformed JSON; wrap the parse in a try-catch (or use a safeParse helper) when
rendering each log entry (i.e., around JSON.parse(log.metadata) in the
AuditLogTable render) and fall back to a safe string (e.g., original
log.metadata or an "invalid JSON" message) so the component doesn't crash;
ensure any caught error is optionally logged (console.warn or a logger) for
debugging.
Comment on lines
+327
to
+339
| export const auditLogsTable = pgTable("audit_logs", { | ||
| id: integer().primaryKey().generatedAlwaysAsIdentity(), | ||
| actorEmail: varchar({ length: 255 }).notNull(), | ||
| targetEmail: varchar({ length: 255 }), | ||
| action: varchar({ length: 100 }).notNull(), | ||
| resourceType: varchar({ length: 50 }).notNull(), | ||
| resourceId: varchar({ length: 255 }), | ||
| metadata: text(), | ||
| createdAt: timestamp().defaultNow().notNull(), | ||
| }, (table) => ({ | ||
| actorEmailIndex: index("audit_actor_idx").on(table.actorEmail), | ||
| actionIndex: index("audit_action_idx").on(table.action), | ||
| })); |
Contributor
There was a problem hiding this comment.
Add index on createdAt to match query patterns.
The schema mirrors the SQL migration but is missing an index on createdAt. The admin API route orders by this field, and without an index, performance will degrade as the table grows.
🚀 Add createdAt index
}, (table) => ({
actorEmailIndex: index("audit_actor_idx").on(table.actorEmail),
actionIndex: index("audit_action_idx").on(table.action),
+ createdAtIndex: index("audit_created_at_idx").on(table.createdAt),
}));📝 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
| export const auditLogsTable = pgTable("audit_logs", { | |
| id: integer().primaryKey().generatedAlwaysAsIdentity(), | |
| actorEmail: varchar({ length: 255 }).notNull(), | |
| targetEmail: varchar({ length: 255 }), | |
| action: varchar({ length: 100 }).notNull(), | |
| resourceType: varchar({ length: 50 }).notNull(), | |
| resourceId: varchar({ length: 255 }), | |
| metadata: text(), | |
| createdAt: timestamp().defaultNow().notNull(), | |
| }, (table) => ({ | |
| actorEmailIndex: index("audit_actor_idx").on(table.actorEmail), | |
| actionIndex: index("audit_action_idx").on(table.action), | |
| })); | |
| export const auditLogsTable = pgTable("audit_logs", { | |
| id: integer().primaryKey().generatedAlwaysAsIdentity(), | |
| actorEmail: varchar({ length: 255 }).notNull(), | |
| targetEmail: varchar({ length: 255 }), | |
| action: varchar({ length: 100 }).notNull(), | |
| resourceType: varchar({ length: 50 }).notNull(), | |
| resourceId: varchar({ length: 255 }), | |
| metadata: text(), | |
| createdAt: timestamp().defaultNow().notNull(), | |
| }, (table) => ({ | |
| actorEmailIndex: index("audit_actor_idx").on(table.actorEmail), | |
| actionIndex: index("audit_action_idx").on(table.action), | |
| createdAtIndex: index("audit_created_at_idx").on(table.createdAt), | |
| })); |
🤖 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/configs/schema.ts` around lines 327 - 339, The schema for auditLogsTable
is missing an index on createdAt; update the table definition (auditLogsTable)
to add a createdAt index in the third-argument indexes object (similar to
actorEmailIndex/actionIndex) by adding something like createdAtIndex:
index("audit_created_at_idx").on(table.createdAt) so queries that ORDER BY
createdAt use the index for better performance.
Owner
|
Automated Detailed Review
Please address the above items and request a review again. |
knoxiboy
added
level:advanced
Owner
|
Hi! This PR currently has merge conflicts with the main branch. Please rebase/merge main and resolve conflicts so we can review and merge it. Thank you! |
Owner
|
Hi @Rashi1404! Thanks for submitting this PR. Here is a detailed review of your changes: 🤖 Bot Review Feedback to ResolveCodeRabbit and CodeAnt have highlighted some important issues that need to be addressed. Please prioritize fixing these:
⚙️ CI/CD PipelineYour PR currently has some failing checks:
Note: We have recently fixed database configuration and This is an automated review assessment. Please address the feedback above to move your PR forward. |
Owner
|
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
Owner
|
@Rashi1404 Look into failing test and try to fix the issue |
github-actions
Bot
added
gssoc:approved
This was referenced Jul 7, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
User description
Problem
Administrative and moderation actions were not consistently tracked, making accountability and issue investigation difficult.
Root Cause
The project had
moderationLogsTablefor user violations but lacked a centralized audit logging system for sensitive actions across the platform.Solution
Implemented a centralized audit logging system:
Added
auditLogsTablewith relevant indexesCreated reusable
auditLog()helper with typed actionsAdded non-blocking audit logging for:
Testing
Issue closed #539
Summary by CodeRabbit
Release Notes
CodeAnt-AI Description
Add an admin audit log page and record key moderation and content actions
What Changed
Impact
✅ Clearer moderation review✅ Easier investigation of user reports✅ Fewer missing action records💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.