Open
Conversation
Replace VirtualTable with the shared DataTable system (@unkey/ui) for the audit logs page. Converts cursor-based infinite scroll to page-based pagination with PaginationFooter and prefetching. Moves feature-specific code to components/audit-logs-table/ following the established pattern.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR migrates the audit logs feature from cursor-based infinite-scroll pagination to explicit page-based pagination. It extracts reusable audit-logs components into a shared library at Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ReactUI as React UI<br/>(DataTable)
participant Hook as useAuditLogsQuery<br/>(Paginated)
participant tRPC as tRPC Router<br/>(audit.logs)
participant DB as Database
User->>ReactUI: Navigate to page/change filters
ReactUI->>Hook: Call hook (pageSize=50)
Hook->>Hook: Derive filtersKey from filters
Hook->>Hook: Reset page=1 if filters changed
Hook->>Hook: Build AuditLogsQueryPayload
Hook->>tRPC: useQuery with {page, limit, ...filters}
tRPC->>DB: buildWhereConditions (workspace/bucket/event/actor/time)
DB->>DB: Execute COUNT() for total
DB->>DB: Fetch rows with LIMIT+OFFSET
DB-->>tRPC: Return {logs[], total}
tRPC-->>Hook: Receive logs + totalCount
Hook->>Hook: Compute totalPages
Hook->>Hook: Clamp page if out of range
Hook->>tRPC: Prefetch pages+1 and +2
Hook-->>ReactUI: Return {auditLogs, page, pageSize, totalPages, isLoading, onPageChange}
ReactUI->>ReactUI: Render DataTable + PaginationFooter
User->>ReactUI: Click next page
ReactUI->>Hook: onPageChange(newPage)
Hook->>Hook: Update URL query param
loop Re-executes for new page
Hook->>tRPC: useQuery with new {page, limit, ...filters}
tRPC->>DB: Execute with offset=(page-1)*limit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
MichaelUnkey
requested review from
Flo4604,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/audit/components/table/logs-table.tsx:
- Around line 22-23: The selectedLog is not cleared when pagination or filters
change, so add logic to reset it whenever the currently loaded auditLogs no
longer contain the selectedLog.auditLog.id: in the component that calls
useAuditLogsQuery(), create/modify a useEffect (or equivalent state-sync) that
watches [auditLogs, selectedLog] and calls setSelectedLog(null) (or the
component's deselect function) when selectedLog is defined but
auditLogs.some(log => log.id === selectedLog.auditLog.id) is false; apply the
same fix for the other selection-check block around lines 34-45 so selection is
cleared any time the current page's row set does not include the previously
selected log.
In
`@web/apps/dashboard/components/audit-logs-table/components/cells/actor-cell.tsx`:
- Around line 10-19: The Actor cell can be empty when user.firstName and
user.lastName are missing; update the isUser rendering in actor-cell.tsx to show
a fallback label that first tries `${user.firstName ?? ""} ${user.lastName ??
""}` but if that is blank uses `user.username`, and if that is also missing uses
`log.auditLog.actor.id` (preserving truncation/whitespace handling and the same
span/className), so the displayed value always falls back to username then actor
id.
In
`@web/apps/dashboard/components/audit-logs-table/hooks/use-audit-logs-query.ts`:
- Around line 20-25: The effect that resets page uses setPage(1) but the
component render still builds queryParams with the old page causing a stale
query; fix by deriving an effectivePage variable that equals 1 when
prevFiltersKeyRef.current !== filtersKey (and otherwise equals page) and use
effectivePage when constructing queryParams and calling useQuery/prefetch, then
let the existing useEffect call setPage(1) to sync the URL/state after render;
update references in this file to use effectivePage wherever queryParams, the
useQuery hook, and the prefetch logic (the code around prevFiltersKeyRef,
filtersKey, setPage, queryParams, and useQuery) are built so no stale request is
emitted.
In `@web/apps/dashboard/lib/trpc/routers/audit/fetch.ts`:
- Around line 49-56: The current offset pagination orders only by time DESC in
db.query.auditLog.findMany (orderBy: (table, { desc }) => desc(table.time)),
which is non-deterministic when multiple rows share the same timestamp; modify
the orderBy to include a deterministic tiebreaker by adding a secondary unique
sort key (e.g., id) in descending order as well so results are stable across
pages (ensure orderBy uses both table.time and table.id consistently).
- Around line 38-40: Clamp both params.limit and params.page to be at least 1
before computing pageSize and offset: compute a clampedLimit = Math.max(1,
params.limit ?? LIMIT), then compute pageSize = Math.min(clampedLimit,
MAX_LIMIT), compute clampedPage = Math.max(1, params.page ?? 1), and finally
offset = (clampedPage - 1) * pageSize; update the references to
params.limit/params.page in fetch.ts (the pageSize, page and offset
calculations) so negative or zero values cannot produce a negative offset or
negative pageSize.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7005be8d-b186-4f63-9730-dbf7f45badb7
📒 Files selected for processing (16)
web/apps/dashboard/app/(app)/[workspaceSlug]/audit/components/table/hooks/use-logs-query.tsweb/apps/dashboard/app/(app)/[workspaceSlug]/audit/components/table/log-details/components/log-header.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/audit/components/table/logs-table.tsxweb/apps/dashboard/components/audit-logs-table/columns/create-audit-log-columns.tsxweb/apps/dashboard/components/audit-logs-table/components/cells/actor-cell.tsxweb/apps/dashboard/components/audit-logs-table/components/cells/audit-action-badge-cell.tsxweb/apps/dashboard/components/audit-logs-table/components/cells/mono-text-cell.tsxweb/apps/dashboard/components/audit-logs-table/components/empty-audit-logs.tsxweb/apps/dashboard/components/audit-logs-table/components/skeletons/render-audit-log-skeleton-row.tsxweb/apps/dashboard/components/audit-logs-table/hooks/use-audit-logs-query.tsweb/apps/dashboard/components/audit-logs-table/index.tsweb/apps/dashboard/components/audit-logs-table/schema/audit-logs.schema.tsweb/apps/dashboard/components/audit-logs-table/utils/get-row-class.tsweb/apps/dashboard/lib/trpc/routers/audit/fetch.tsweb/apps/dashboard/lib/trpc/routers/audit/schema.tsweb/apps/dashboard/lib/trpc/routers/audit/utils.ts
💤 Files with no reviewable changes (1)
- web/apps/dashboard/app/(app)/[workspaceSlug]/audit/components/table/hooks/use-logs-query.ts
web/apps/dashboard/app/(app)/[workspaceSlug]/audit/components/table/logs-table.tsx
Show resolved
Hide resolved
web/apps/dashboard/components/audit-logs-table/components/cells/actor-cell.tsx
Show resolved
Hide resolved
web/apps/dashboard/components/audit-logs-table/hooks/use-audit-logs-query.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/apps/dashboard/lib/trpc/routers/audit/fetch.ts (1)
38-40:⚠️ Potential issue | 🟠 MajorClamp
pageandlimitbefore building the offset.Line 38 still accepts negative
limit, and Line 39 still acceptspage <= 0, so Line 40 can produce a negativelimit/offset. Clamp both values at the RPC boundary before they reach Drizzle.Suggested fix
- const pageSize = Math.min(params.limit, MAX_LIMIT) || LIMIT; - const page = params.page ?? 1; + const pageSize = Math.min(Math.max(params.limit ?? LIMIT, 1), MAX_LIMIT); + const page = Math.max(params.page ?? 1, 1); const offset = (page - 1) * pageSize;As per coding guidelines, "Make illegal states unrepresentable by modeling domain with discriminated unions and parsing inputs at boundaries into typed structures".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/lib/trpc/routers/audit/fetch.ts` around lines 38 - 40, Clamp both params.limit and params.page before computing pageSize and offset: ensure you derive a sanitizedLimit = Math.max(1, Math.min(params.limit ?? LIMIT, MAX_LIMIT)) and a sanitizedPage = Math.max(1, params.page ?? 1) (or equivalent), then use sanitizedLimit for pageSize and sanitizedPage for page so offset = (sanitizedPage - 1) * sanitizedLimit cannot be negative; update the logic around the variables pageSize, page, offset in fetch.ts to use these sanitized values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/apps/dashboard/lib/trpc/routers/audit/fetch.ts`:
- Around line 44-58: The current LIMIT/OFFSET pagination
(db.query.auditLog.findMany with orderBy (table, { desc }) => [desc(table.time),
desc(table.id)], pageSize, offset and the separate count() query) can drift as
new rows are inserted; fix by switching to cursor-based paging or by pinning an
upper-bound snapshot from the first page and applying it to all subsequent
reads: capture the last row's (time,id) from the first page and add a persistent
where clause (e.g., time < upperTime OR (time = upperTime AND id <= upperId)) to
both db.query.auditLog.findMany and the count() select so all pages and the
total share the same snapshot; update the API to accept a cursor (upperTime,
upperId and/or nextCursor) instead of relying on offset to avoid
duplicates/missing rows.
---
Duplicate comments:
In `@web/apps/dashboard/lib/trpc/routers/audit/fetch.ts`:
- Around line 38-40: Clamp both params.limit and params.page before computing
pageSize and offset: ensure you derive a sanitizedLimit = Math.max(1,
Math.min(params.limit ?? LIMIT, MAX_LIMIT)) and a sanitizedPage = Math.max(1,
params.page ?? 1) (or equivalent), then use sanitizedLimit for pageSize and
sanitizedPage for page so offset = (sanitizedPage - 1) * sanitizedLimit cannot
be negative; update the logic around the variables pageSize, page, offset in
fetch.ts to use these sanitized values.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0fee9742-a993-4a0d-9033-e63d46b3f079
📒 Files selected for processing (4)
web/apps/dashboard/app/(app)/[workspaceSlug]/audit/components/table/logs-table.tsxweb/apps/dashboard/components/audit-logs-table/components/cells/actor-cell.tsxweb/apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsxweb/apps/dashboard/lib/trpc/routers/audit/fetch.ts
✅ Files skipped from review due to trivial changes (1)
- web/apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/apps/dashboard/components/audit-logs-table/components/cells/actor-cell.tsx
- web/apps/dashboard/app/(app)/[workspaceSlug]/audit/components/table/logs-table.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/apps/dashboard/lib/trpc/routers/audit/fetch.ts`:
- Around line 38-40: The code computes page and offset from params but only
lower-bounds page, allowing huge offsets to be used in queries; update the RPC
input/parsing or add a guard in the fetch handler to enforce a maximum
page/offset: introduce a MAX_PAGE or MAX_OFFSET constant and either clamp page
(e.g., page = Math.min(Math.max(1, params.page ?? 1), MAX_PAGE)) before
computing offset, or compute offset and throw a TRPCError when offset >
MAX_OFFSET; apply this change around the pageSize/page/offset logic (pageSize,
page, offset, LIMIT, MAX_LIMIT, params) so oversized requests are rejected or
clamped at the API boundary.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 677c9122-9172-438c-bb40-ba60d00dc029
📒 Files selected for processing (1)
web/apps/dashboard/lib/trpc/routers/audit/fetch.ts
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes # (issue)
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated