feat: XYNE-206 added a chats overview for admins #1129
Conversation
Chinmay67
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
Summary of ChangesHello @Chinmay67, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the administrative capabilities of the application by providing a comprehensive overview of all chat interactions. Administrators can now easily monitor, search, filter, and sort chat conversations, and delve into the details of individual chats, improving system oversight and debugging processes. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an admin chat management UI and route, server-side pagination/filtering/sorting for admin chats, a new API endpoint to fetch user queries for a chat, and a DB helper to return user-sent messages for a chat. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant Frontend
participant API
participant DB
Admin->>Frontend: Open Chat Overview / Dashboard → All/User Chats
Frontend->>API: GET /admin/chats?page&offset&search&filterType&sortBy
API->>DB: Query chats with filters, aggregates, pagination
DB-->>API: Paginated AdminChat rows + aggregations
API-->>Frontend: AdminChat objects + pagination meta
Frontend->>Frontend: Render AdminChatsTable
Admin->>Frontend: Click "View" on a chat
Frontend->>API: GET /chat/queries/:chatId
API->>DB: fetchUserQueriesForChat(chatId[, workspace])
DB-->>API: Array<string> (user queries)
API-->>Frontend: Messages
Frontend->>Frontend: Open ChatViewDialog (display messages)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for administrators to view and filter chats. The implementation includes a new AdminChatsTable component, a dedicated route for the chat overview, and backend API enhancements for fetching chat data. My review focuses on improving performance, fixing a few bugs related to filtering and pagination, and enhancing code maintainability. Key suggestions include implementing server-side pagination and sorting correctly, memoizing expensive computations on the frontend, and addressing code duplication. Overall, this is a great addition, and with these changes, it will be more robust and performant.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/Dashboard.tsx (2)
1629-1633: Bug: mutating theusersprop while computingoriginalRank.
users.sort(...)mutates the prop array on every render; this can cause subtle UI bugs and unnecessary re-renders.Apply this non‑mutating, cached rank computation:
@@ - const sortedUsers = filteredUsers.sort( - (a, b) => b.messageCount - a.messageCount, - ) + const sortedUsers = [...filteredUsers].sort( + (a, b) => b.messageCount - a.messageCount, + ) + + // Precompute original ranks once per `users` change + const rankByUserId = React.useMemo(() => { + const arr = [...users].sort((a, b) => b.messageCount - a.messageCount) + return new Map(arr.map((u, idx) => [u.userId, idx + 1])) + }, [users]) @@ - const originalRank = - users - .sort((a, b) => b.messageCount - a.messageCount) - .findIndex((u) => u.userId === user.userId) + 1 + const originalRank = rankByUserId.get(user.userId) ?? index + 1
3340-3368: Inconsistent time bucketing for “3m”.
processChatsDatauses weekly buckets for "3m", butprocessAdminChatsDatauses daily buckets. This yields mismatched charts across tabs.Unify “3m” to weekly buckets in
processAdminChatsData:case "3m": - d.setHours(0, 0, 0, 0) - return { - key: d.toISOString(), - display: d.toLocaleDateString("en-US", { - month: "short", - day: "numeric", - }), - } + const startOfWeek = new Date(d) + const day = startOfWeek.getDay() + startOfWeek.setDate(startOfWeek.getDate() - day) + startOfWeek.setHours(0, 0, 0, 0) + return { + key: startOfWeek.toISOString(), + display: `Week of ${startOfWeek.toLocaleDateString("en-US", { + month: "short", + day: "numeric", + })}`, + }And increment by weeks in the filler:
case "3m": - current.setDate(current.getDate() + 1) + current.setDate(current.getDate() + 7)Also applies to: 3017-3030
🧹 Nitpick comments (13)
server/db/message.ts (1)
345-363: Add optional limit and workspace scoping to prevent large responses and cross-tenant leaks.Current query returns all user-authored messages for a chat without any cap. Consider:
- Accepting an optional limit (e.g., default 50/100).
- Optionally scoping by workspaceExternalId when the caller can supply it.
Apply this diff to make the API safer and more flexible:
-export const fetchUserQueriesForChat = async ( - trx: TxnOrClient, - chatExternalId: string, -): Promise<string[]> => { - const queries = await trx +export const fetchUserQueriesForChat = async ( + trx: TxnOrClient, + chatExternalId: string, + opts?: { limit?: number; workspaceExternalId?: string }, +): Promise<string[]> => { + const q = trx .select({ content: messages.message, }) .from(messages) - .where( + .where( and( eq(messages.chatExternalId, chatExternalId), eq(messages.messageRole, MessageRole.User), isNull(messages.deletedAt), - ), - ) - .orderBy(asc(messages.createdAt)) - return queries.map((q) => q.content) + ...(opts?.workspaceExternalId + ? [eq(messages.workspaceExternalId, opts.workspaceExternalId)] + : []), + ), + ) + .orderBy(asc(messages.createdAt)) + + const queries = opts?.limit ? await q.limit(opts.limit) : await q + return queries.map((q) => q.content) }server/server.ts (2)
1321-1321: Validate route params with zod to harden the admin endpoint.Add a param validator to ensure chatId is present/non-empty and block malformed requests.
- .get("/chat/queries/:chatId", GetChatQueriesApi) + .get( + "/chat/queries/:chatId", + zValidator( + "param", + z.object({ chatId: z.string().min(1, "chatId is required") }), + ), + GetChatQueriesApi, + )
1443-1471: Minor: use Object.entries for clarity and to avoid repeated lookups.This is optional, but simplifies the loop.
- const queryParams = c.req.query() - Object.keys(queryParams).forEach((key) => { - if (queryParams[key]) { - urlObj.searchParams.set(key, queryParams[key]) - } - }) + const queryParams = c.req.query() + for (const [key, value] of Object.entries(queryParams)) { + if (value) urlObj.searchParams.set(key, value) + }server/api/admin.ts (2)
169-187: Use z.coerce.number with defaults; rename “offset” to “limit” for clarity.z.coerce.number() reduces manual parsing. Also, “offset” currently means page size; renaming avoids confusion.
- page: z - .string() - .optional() - .refine((val) => !val || (!isNaN(Number(val)) && Number(val) > 0), { - message: "Page must be a positive number", - }) - .transform((val) => (val ? Number(val) : 1)), - offset: z - .string() - .optional() - .refine((val) => !val || (!isNaN(Number(val)) && Number(val) >= 0), { - message: "Offset must be a non-negative number", - }) - .transform((val) => (val ? Number(val) : 20)), + page: z.coerce.number().int().positive().default(1), + limit: z.coerce.number().int().min(1).default(20), search: z .string() .optional() .transform((val) => (val?.trim() ? val.trim() : undefined)),Follow-up: Adjust downstream code and frontend to send/expect “limit” instead of “offset”.
2363-2394: Admin-only OK, but add param validation and consider a max items cap.
- Validate chatId with zod (server.ts change suggested separately).
- Optionally accept a “limit” query param and pass to fetchUserQueriesForChat to avoid very large payloads.
- const queries = await fetchUserQueriesForChat(db, chatId) + const limit = + Number(c.req.query("limit")) > 0 ? Number(c.req.query("limit")) : 100 + const queries = await fetchUserQueriesForChat(db, chatId, { limit })frontend/src/routes/_authenticated/admin/chat-overview.tsx (3)
69-73: Type search via Zod to avoid “never” and improve resilience; default pageSize/currentPage from search if provided.Optional, but helps keep URL and state in sync.
-// Reset to page 1 when userId filter changes +// Reset to page 1 when userId filter changes useEffect(() => { setCurrentPage(1) }, [search.userId]) @@ - }, [search.userId, currentPage, pageSize]) + }, [search.userId, search.search, currentPage, pageSize]) @@ - const getPageInfo = () => { + const getPageInfo = () => { if (search.userId && search.userName) { return { title: `Chats for ${search.userName}`, description: `All chat conversations for user: ${search.userName}`, } } return { title: "All Chats Overview", description: "Complete overview of all chat conversations across the platform", } } @@ - {!loading && ( + {!loading && ( <div className="text-xs text-muted-foreground mb-4 p-2 bg-muted/20 rounded"> Debug: userId={search.userId}, userName={search.userName}, chats= {adminChats.length}, page={currentPage}, pageSize={pageSize} </div> )}If you want URL-driven pagination later, add page/limit to search and derive state from it.
Also applies to: 138-140, 146-158, 200-205
101-127: Defensive mapping: ensure numbers and booleans are correctly shaped for AdminChatsTable.Coerce numeric fields and fallback safely.
- setAdminChats( - adminChats.map((chat: any) => ({ + setAdminChats( + adminChats.map((chat: any) => ({ externalId: chat.externalId, title: chat.title || "Untitled Chat", createdAt: chat.createdAt, - userId: chat.userId || chat.user?.id, + userId: Number(chat.userId ?? chat.user?.id ?? 0), userName: chat.userName || chat.user?.name || "Unknown User", userEmail: chat.userEmail || chat.user?.email || "", agentId: chat.agentId, agentName: chat.agentName || (chat.agentId ? adminAgents.find((a: any) => a.externalId === chat.agentId) ?.name : null), - messageCount: chat.messageCount || chat.messages?.length || 0, - totalCost: chat.totalCost || 0, - totalTokens: chat.totalTokens || 0, - likes: chat.likes || 0, - dislikes: chat.dislikes || 0, - isBookmarked: chat.isBookmarked || false, + messageCount: Number(chat.messageCount ?? chat.messages?.length ?? 0), + totalCost: Number(chat.totalCost ?? 0), + totalTokens: Number(chat.totalTokens ?? 0), + likes: Number(chat.likes ?? 0), + dislikes: Number(chat.dislikes ?? 0), + isBookmarked: Boolean(chat.isBookmarked ?? false), })), )
217-268: Next page button enablement may mislead when result length equals pageSize.Consider consuming a totalCount from the API or returning “hasNextPage” to drive the disabled state. As-is, “length < pageSize” may disable Next on exact multiples.
No diff since API changes are required. Suggest API to return { data, page, limit, total }. Then use total to compute last page reliably.
frontend/src/components/Dashboard.tsx (2)
4040-4046: Navigation: avoid full page reload; use router navigation.Using
window.location.hrefbreaks SPA flow and state. Prefer your app router (e.g., react-routeruseNavigateor TanStack Router).Example with react-router:
- onAllChatsClick={() => { - window.location.href = "/admin/chat-overview" - }} + const navigate = useNavigate() + onAllChatsClick={() => navigate("/admin/chat-overview")} @@ - onUserChatsClick={(userId: number, userName: string) => { - window.location.href = `/admin/chat-overview?userId=${userId}&userName=${encodeURIComponent(userName)}` - }} + onUserChatsClick={(userId: number, userName: string) => + navigate(`/admin/chat-overview?userId=${userId}&userName=${encodeURIComponent(userName)}`) + }
3569-3583: Inefficient/fragile sort ofrecentActivity.Sorting values while recomputing keys per item is O(n²) and brittle. Build a sorted entries list by key once.
- let recentActivity: TimeSeriesData[] = Array.from( - timeSeriesMap.values(), - ).sort((a, b) => { - const aTime = timeSeriesMap.keys() - const bTime = timeSeriesMap.keys() - return ( - new Date( - Array.from(aTime).find((k) => timeSeriesMap.get(k) === a) || "", - ).getTime() - - new Date( - Array.from(bTime).find((k) => timeSeriesMap.get(k) === b) || "", - ).getTime() - ) - }) + let recentActivity: TimeSeriesData[] = Array.from(timeSeriesMap.entries()) + .sort((a, b) => new Date(a[0]).getTime() - new Date(b[0]).getTime()) + .map(([_, v]) => v)frontend/src/components/AdminChatsTable.tsx (3)
123-124: Dialog control: don’t close on open.Passing
onClosedirectly toonOpenChangewill also fire when opening, causing flicker. Only handle the “close” transition.- <Dialog open={isOpen} onOpenChange={onClose}> + <Dialog + open={isOpen} + onOpenChange={(open) => { + if (!open) onClose() + }} + >
289-311: Data ownership: fetching users inside the table couples UI and data.Prefer lifting users fetch to the parent (Dashboard) and pass as a prop, so the Admin overview can reuse cached users and avoid duplicate calls when the table re-mounts.
331-365: Perf: repeated filter/sort on every render.For large datasets, memoize filtered and sorted lists.
Example:
- const filteredChats = chats.filter((chat) => { ... }) - const sortedChats = [...filteredChats].sort((a, b) => { ... }) + const filteredChats = React.useMemo(() => { + return chats.filter((chat) => { /* existing logic */ }) + }, [chats, searchQuery, filterType, userFilter]) + const sortedChats = React.useMemo(() => { + return [...filteredChats].sort((a, b) => { /* existing logic */ }) + }, [filteredChats, sortBy])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/components/AdminChatsTable.tsx(1 hunks)frontend/src/components/Dashboard.tsx(10 hunks)frontend/src/routes/_authenticated/admin/chat-overview.tsx(1 hunks)server/api/admin.ts(10 hunks)server/db/message.ts(2 hunks)server/server.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
server/server.ts (2)
server/api/ingestion.ts (3)
cancelIngestionSchema(32-34)pauseIngestionSchema(41-43)resumeIngestionSchema(36-38)server/api/admin.ts (1)
GetChatQueriesApi(2363-2394)
server/db/message.ts (2)
server/types.ts (1)
TxnOrClient(319-321)server/db/schema/messages.ts (1)
messages(31-78)
frontend/src/components/AdminChatsTable.tsx (4)
frontend/src/api.ts (1)
api(5-5)server/db/schema/users.ts (1)
users(27-57)server/db/schema/chats.ts (1)
chats(37-70)frontend/src/lib/utils.ts (1)
formatCostInINR(90-94)
server/api/admin.ts (6)
server/db/client.ts (1)
db(16-16)server/db/ingestion.ts (2)
hasActiveIngestion(127-144)createIngestion(15-24)server/db/schema/chats.ts (1)
chats(37-70)server/db/schema/users.ts (1)
users(27-57)server/db/message.ts (1)
fetchUserQueriesForChat(345-363)server/utils.ts (1)
getErrorMessage(103-106)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (4)
frontend/src/components/error.tsx (1)
errorComponent(1-6)frontend/src/components/AdminChatsTable.tsx (2)
AdminChat(36-51)AdminChatsTable(260-621)frontend/src/api.ts (1)
api(5-5)frontend/src/components/Sidebar.tsx (1)
Sidebar(40-390)
frontend/src/components/Dashboard.tsx (1)
frontend/src/components/AdminChatsTable.tsx (2)
AdminChat(36-51)AdminChatsTable(260-621)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/components/AdminChatsTable.tsx
[error] 13-13: TS6133: 'Clock' is declared but its value is never read.
[error] 14-14: TS6133: 'Hash' is declared but its value is never read.
[error] 29-29: TS6133: 'DialogDescription' is declared but its value is never read.
frontend/src/routes/_authenticated/admin/chat-overview.tsx
[error] 26-26: TS2345: Argument of type '"/_authenticated/admin/chat-overview"' is not assignable to parameter of type 'keyof FileRoutesByPath'.
[error] 62-62: TS2322: Type '"/_authenticated/admin/chat-overview"' is not assignable to type 'ConstrainLiteral<"/_authenticated/admin/chat-overview", ...>'.
[error] 72-72: TS2339: Property 'userId' does not exist on type 'never'.
[error] 85-85: TS2339: Property 'userId' does not exist on type 'never'.
[error] 86-86: TS2339: Property 'userId' does not exist on type 'never'.
[error] 88-88: TS2339: Property 'search' does not exist on type 'never'.
[error] 89-89: TS2339: Property 'search' does not exist on type 'never'.
[error] 139-139: TS2339: Property 'userId' does not exist on type 'never'.
[error] 147-147: TS2339: Property 'userId' does not exist on type 'never'.
[error] 147-147: TS2339: Property 'userName' does not exist on type 'never'.
[error] 149-149: TS2339: Property 'userName' does not exist on type 'never'.
[error] 150-150: TS2339: Property 'userName' does not exist on type 'never'.
[error] 202-202: TS2339: Property 'userId' does not exist on type 'never'.
[error] 202-202: TS2339: Property 'userName' does not exist on type 'never'.
🔇 Additional comments (3)
frontend/src/components/Dashboard.tsx (1)
1512-1520: Good addition: “All Chats” and per-user “View Chats” affordances.
The callbacks are optional and guarded; event propagation is handled.Confirm the new route
/admin/chat-overviewexists and acceptsuserIdanduserNamequery params (created on 2025-10-22 per PR). If not, I can patch navigation. Based on learnings.Also applies to: 1715-1733
frontend/src/components/AdminChatsTable.tsx (2)
552-566: Currency display is clear.
formatCostInINR(chat.totalCost)reads well alongside tokens/messages.
83-101: The endpoint path is correctly configured; no changes needed.Verification confirms that
api.admin.chat.queries[chatId].$get()matches the server route at/api/v1/admin/chat/queries/:chatId. The backend applies.basePath("/admin")at line 1207, and the frontend hono-rpc client correctly maps the nested object chain to this path structure. The endpoint path matches exactly as described in the review comment's own example.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (2)
258-260: Remove leftover debug log.Avoid console noise in production.
- onChatView={(chat: AdminChat) => { - console.log("Viewing chat:", chat.externalId) - // You can implement chat viewing functionality here if needed - }} + onChatView={() => { + /* no-op: hook available for parent wiring */ + }}
1-7: Fix TanStack Router typing: use Route.useSearch and silence path typing once.Build is red due to incorrect useSearch and path typing. Remove useSearch import, switch to Route.useSearch(), and add a one-line ts-expect-error above createFileRoute path literal.
import { createFileRoute, redirect, useRouterState, useNavigate, - useSearch, } from "@tanstack/react-router" @@ -export const Route = createFileRoute("/_authenticated/admin/chat-overview")({ +// @ts-expect-error: Path literal is validated by the file-based router plugin at build time. +export const Route = createFileRoute("/_authenticated/admin/chat-overview")({ @@ - const search = useSearch({ from: "/_authenticated/admin/chat-overview" }) + const search = Route.useSearch()Also applies to: 25-25, 61-61
🧹 Nitpick comments (4)
frontend/src/components/AdminChatsTable.tsx (2)
120-133: Prevent unintended close when Dialog opens; add a11y label.onOpenChange={onClose} may close on open events. Only close when open becomes false; add aria-label to the close button.
- <Dialog open={isOpen} onOpenChange={onClose}> + <Dialog open={isOpen} onOpenChange={(open) => !open && onClose()}> ... - <button + <button + aria-label="Close dialog" onClick={onClose} className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2" >
387-406: Add accessible labels to icon-only controls.Improve keyboard/screen-reader UX for search controls.
- {searchInput && ( + {searchInput && ( <button type="button" onClick={onClearSearch} + aria-label="Clear search" className="p-1 hover:bg-muted rounded-sm transition-colors" title="Clear search" > <X className="h-3 w-3 text-muted-foreground hover:text-foreground" /> </button> )} <button type="button" onClick={onSearch} + aria-label="Search" className="p-1 hover:bg-muted rounded-sm transition-colors" title="Search" >frontend/src/routes/_authenticated/admin/chat-overview.tsx (1)
269-284: Clarify pagination labels.“Total: {adminChats.length}” is page count, not total rows. Consider “per page • {adminChats.length} on this page”, or surface a totalCount from API.
frontend/src/components/Dashboard.tsx (1)
2906-2928: Delete duplicated admin chat mapping (now unused).This block only fed the removed adminChats state and duplicated logic from the route.
- // Set the raw admin chats data for the table - setAdminChats( - adminChats.map((chat: any) => ({ - externalId: chat.externalId, - title: chat.title || "Untitled Chat", - createdAt: chat.createdAt, - userId: chat.userId || chat.user?.id, - userName: chat.userName || chat.user?.name || "Unknown User", - userEmail: chat.userEmail || chat.user?.email || "", - agentId: chat.agentId, - agentName: - chat.agentName || - (chat.agentId - ? adminAgents.find((a: any) => a.externalId === chat.agentId) - ?.name - : null), - messageCount: chat.messageCount || chat.messages?.length || 0, - totalCost: chat.totalCost || 0, - totalTokens: chat.totalTokens || 0, - likes: chat.likes || 0, - dislikes: chat.dislikes || 0, - isBookmarked: chat.isBookmarked || false, - })), - )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/AdminChatsTable.tsx(1 hunks)frontend/src/components/Dashboard.tsx(11 hunks)frontend/src/routes/_authenticated/admin/chat-overview.tsx(1 hunks)server/api/admin.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T08:44:08.412Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#719
File: server/api/chat/chat.ts:4023-4029
Timestamp: 2025-08-11T08:44:08.412Z
Learning: In server/api/chat/chat.ts and similar files, MayankBansal2004 prefers not to add explicit type definitions for metadata.usage structures when the receiving array is already typed and default values ensure type safety through the || 0 pattern.
Applied to files:
frontend/src/components/Dashboard.tsx
🧬 Code graph analysis (4)
frontend/src/components/AdminChatsTable.tsx (5)
server/db/schema/messages.ts (1)
messages(31-78)frontend/src/api.ts (1)
api(5-5)server/db/schema/users.ts (1)
users(27-57)server/db/schema/chats.ts (1)
chats(37-70)frontend/src/lib/utils.ts (1)
formatCostInINR(90-94)
server/api/admin.ts (8)
server/db/client.ts (1)
db(16-16)server/db/ingestion.ts (2)
hasActiveIngestion(127-144)createIngestion(15-24)server/db/user.ts (1)
getUserByEmail(148-157)server/db/schema/messages.ts (1)
messages(31-78)server/db/schema/users.ts (1)
users(27-57)server/db/schema/chats.ts (1)
chats(37-70)server/db/message.ts (1)
fetchUserQueriesForChat(345-363)server/utils.ts (1)
getErrorMessage(103-106)
frontend/src/components/Dashboard.tsx (2)
frontend/src/components/ui/card.tsx (2)
CardTitle(76-76)CardDescription(76-76)frontend/src/components/AdminChatsTable.tsx (1)
AdminChat(33-48)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (3)
frontend/src/components/error.tsx (1)
errorComponent(1-6)frontend/src/components/AdminChatsTable.tsx (2)
AdminChat(33-48)AdminChatsTable(271-635)frontend/src/api.ts (1)
api(5-5)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/components/Dashboard.tsx
[error] 41-41: bunx tsc -b: TS6133: 'AdminChatsTable' is declared but its value is never read.
[error] 2707-2707: bunx tsc -b: TS6133: 'adminChats' is declared but its value is never read.
[error] 4043-4043: bunx tsc -b: TS2322: Type '"/admin/chat-overview"' is not assignable to type '"/call" | "/auth" | "/oauth/success" | "/" | "." | ".." | "/agent" | "/chat" | "/dashboard" | "/knowledgeManagement" | "/search" | "/tuning" | "/workflow" | "/admin/userManagement" | ... 13 more ... | "/admin/integrations"'.
[error] 4048-4048: bunx tsc -b: TS2322: Type '"/admin/chat-overview"' is not assignable to type '"/call" | "/auth" | "/oauth/success" | "/" | "." | ".." | "/agent" | "/chat" | "/dashboard" | "/knowledgeManagement" | "/search" | "/tuning" | "/workflow" | "/admin/userManagement" | ... 13 more ... | "/admin/integrations"'.
[error] 4049-4049: bunx tsc -b: TS2353: Object literal may only specify known properties, and 'userName' does not exist in type 'ParamsReducerFn<Router<RootRoute<undefined, {}, AnyContext, AnyContext, {}, undefined, RootRouteChildren, FileRouteTypes>, ... 4 more ..., Record<...>>, "SEARCH", string, "/admin/chat-overview">'.
frontend/src/routes/_authenticated/admin/chat-overview.tsx
[error] 25-25: bunx tsc -b: TS2345: Argument of type '"/_authenticated/admin/chat-overview"' is not assignable to parameter of type 'keyof FileRoutesByPath'.
[error] 61-61: bunx tsc -b: TS2322: Type '"/_authenticated/admin/chat-overview"' is not assignable to type 'ConstrainLiteral<"/_authenticated/admin/chat-overview", "root" | "/call" | "/_authenticated" | "/auth" | "/_authenticated/agent" | "/_authenticated/chat" | "/_authenticated/dashboard" | ... 20 more ... | "/_authenticated/admin/integrations/">'.
[error] 108-108: bunx tsc -b: TS2339: Property 'search' does not exist on type 'never'.
[error] 109-109: bunx tsc -b: TS2339: Property 'search' does not exist on type 'never'.
[error] 174-174: bunx tsc -b: TS2339: Property 'search' does not exist on type 'never'.
[error] 189-189: bunx tsc -b: TS2339: Property 'userName' does not exist on type 'never'.
[error] 191-191: bunx tsc -b: TS2339: Property 'userName' does not exist on type 'never'.
[error] 192-192: bunx tsc -b: TS2339: Property 'userName' does not exist on type 'never'.
[error] 256-256: bunx tsc -b: TS2339: Property 'userName' does not exist on type 'never'.
🔇 Additional comments (5)
server/api/admin.ts (3)
144-144: LGTM!The import is correctly placed and used by the new
GetChatQueriesApiendpoint.
1611-1669: LGTM!The structured metadata approach with
websocketDataandingestionStateproperly supports resumability. The 409 conflict response prevents concurrent ingestions, and all necessary state is captured for resuming interrupted operations.
1767-1804: Excellent implementation of SQL-based pagination and deleted message handling.The aggregations correctly exclude deleted messages in all calculations (likes, dislikes, cost, tokens), and pagination is now performed in SQL rather than in JavaScript. This addresses the concerns from previous reviews and will perform well with large datasets.
frontend/src/components/Dashboard.tsx (1)
41-42: Remove unused importAdminChatsTable(build break TS6133).Not referenced in this file.
-import { AdminChatsTable } from "@/components/AdminChatsTable" -import type { AdminChat } from "@/components/AdminChatsTable"Likely an incorrect or invalid review comment.
frontend/src/routes/_authenticated/admin/chat-overview.tsx (1)
101-122: No issue found—pagination logic is correct despite confusing parameter naming.The backend handler (GetAdminChats, lines 1781-1782) correctly calculates pagination:
- Uses the
offsetquery parameter as thelimit(page size)- Uses the
pageparameter with the formula(page - 1) * limitto compute the actual SQLOFFSETResult: Frontend sends
page=2, offset=20→ Backend computesLIMIT 20 OFFSET 20, which correctly returns the second page of 20 items.The parameter naming is unconventional (sending
offsetto mean "limit"), but the SQL pagination is mathematically correct.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
server/api/admin.ts (3)
176-182: Parameter naming remains confusing:offsetis actuallylimit/pageSize.This concern was previously raised in past reviews. The parameter named
offsetrepresents items per page (used aslimiton line 1795), while the actual SQL offset is calculated separately on line 1796. This terminology mismatch can confuse developers maintaining pagination logic.
1700-1723: UnusedisSuperAdminvariable and disabled SuperAdmin cross-workspace access.The
isSuperAdmincheck on line 1707 is computed but never used. Lines 1721-1723 show commented-out logic that would allow SuperAdmins to view chats across all workspaces. Currently, line 1722 enforces workspace filtering for all users, including SuperAdmins.If SuperAdmin cross-workspace access is not needed, remove the unused variable. Otherwise, uncomment lines 1721 and 1723 to enable the feature.
2394-2458: Verify chat ownership before fetching queries to prevent information disclosure.While the endpoint correctly checks for Admin/SuperAdmin role (lines 2420-2430) and scopes message queries to the workspace (line 2435), it doesn't verify that the chat itself belongs to
currentWorkspaceId. An admin could potentially probe for chats in other workspaces:
- If the chat exists in another workspace, the query returns
[](leaking that the chatId exists)- If the chat doesn't exist, the query also returns
[](same response)Additionally, SuperAdmin users may need cross-workspace access but are currently restricted.
Consider adding explicit chat ownership verification:
export const GetChatQueriesApi = async (c: Context) => { try { const chatId = c.req.param("chatId") if (!chatId) { return c.json( { success: false, message: "chatId is required", }, 400, ) } // Get current user and check if they are admin/superadmin const { workspaceId: currentWorkspaceId, sub } = c.get(JwtPayloadKey) const currentUserRes = await getUserByEmail(db, sub) if (!currentUserRes || !currentUserRes.length) { loggerWithChild({ email: sub }).error( { sub }, "No user found for sub in GetChatQueriesApi", ) throw new HTTPException(403, { message: "User not found" }) } const currentUser = currentUserRes[0] // Check if user is admin or superadmin const isAuthorized = currentUser.role === "Admin" || currentUser.role === "SuperAdmin" if (!isAuthorized) { loggerWithChild({ email: sub }).warn( { userEmail: sub, userRole: currentUser.role }, "Unauthorized access attempt to chat queries - not admin", ) throw new HTTPException(403, { message: "Access denied. Admin privileges required.", }) } + + // Verify the chat exists and belongs to the workspace + const chatResult = await db + .select({ + workspaceExternalId: chats.workspaceExternalId, + userId: chats.userId + }) + .from(chats) + .where(eq(chats.externalId, chatId)) + .limit(1) + + if (chatResult.length === 0) { + return c.json( + { + success: false, + message: "Chat not found", + }, + 404, + ) + } + + const isSuperAdmin = currentUser.role === "SuperAdmin" + if (!isSuperAdmin && chatResult[0].workspaceExternalId !== currentWorkspaceId) { + loggerWithChild({ email: sub }).warn( + { chatId, userWorkspace: currentWorkspaceId, chatWorkspace: chatResult[0].workspaceExternalId }, + "Unauthorized cross-workspace chat access attempt", + ) + throw new HTTPException(403, { + message: "Access denied. Chat not found in your workspace.", + }) + } - const queries = await fetchUserQueriesForChat( - db, - chatId, - currentWorkspaceId, - ) + // For SuperAdmin, don't scope to workspace; for Admin, scope to their workspace + const queries = await fetchUserQueriesForChat( + db, + chatId, + isSuperAdmin ? undefined : currentWorkspaceId, + ) return c.json( { success: true, data: queries, }, 200, ) } catch (error) { Logger.error(error, "Error fetching chat queries") if (error instanceof HTTPException) { throw error } return c.json( { success: false, message: getErrorMessage(error), }, 500, ) } }frontend/src/components/Dashboard.tsx (3)
2706-2706: Remove dead adminChats state (+ unused type import).State value is discarded and never read; mapping below only wastes CPU and memory. Also drop the now-unused AdminChat import.
-import type { AdminChat } from "@/components/AdminChatsTable" ... - const [_, setAdminChats] = useState<AdminChat[]>([])Also applies to: 41-41
2904-2927: Delete duplicate/unreferenced admin chat mapping.This duplicates mapping already handled in the admin chat-overview route and sets state that isn't used. Remove to avoid drift and unnecessary work. If you must keep it, coerce types (Number(...) + nullish coalescing).
- // Set the raw admin chats data for the table - setAdminChats( - adminChats.map((chat: any) => ({ - externalId: chat.externalId, - title: chat.title || "Untitled Chat", - createdAt: chat.createdAt, - userId: chat.userId || chat.user?.id, - userName: chat.userName || chat.user?.name || "Unknown User", - userEmail: chat.userEmail || chat.user?.email || "", - agentId: chat.agentId, - agentName: - chat.agentName || - (chat.agentId - ? adminAgents.find((a: any) => a.externalId === chat.agentId) - ?.name - : null), - messageCount: chat.messageCount || chat.messages?.length || 0, - totalCost: chat.totalCost || 0, - totalTokens: chat.totalTokens || 0, - likes: chat.likes || 0, - dislikes: chat.dislikes || 0, - isBookmarked: chat.isBookmarked || false, - })), - )
4041-4050: Fix route path and search type to resolve TS2322/TS2353.Use the authenticated route and pass only supported search keys (userId as string).
Apply:
- onAllChatsClick={() => { - // Navigate to all chats view - navigate({ to: "/admin/chat-overview" }) - }} - onUserChatsClick={(userId: number, userName: string) => { - // Navigate to user-specific chats view - navigate({ - to: "/admin/chat-overview", - search: { userName: userName }, - }) - }} + onAllChatsClick={() => { + navigate({ to: "/_authenticated/admin/chat-overview" }) + }} + onUserChatsClick={(userId: number, _userName: string) => { + navigate({ + to: "/_authenticated/admin/chat-overview", + search: { userId: String(userId) }, + }) + }}Run to confirm the route and its search schema:
#!/bin/bash # Find the chat overview route file and its search schema fd -a 'chat-overview.tsx' frontend/src/routes | sed -n '1p' | while read f; do echo "Route file: $f" rg -n 'createFileRoute|search|validateSearch|searchSchema|path' "$f" -n -C2 done
🧹 Nitpick comments (2)
frontend/src/components/Dashboard.tsx (2)
1544-1547: Row “View Chats” handler is fine; minor nit.Add explicit button type to avoid accidental form submits if nested in a form.
- const handleViewChatsClick = (e: React.MouseEvent, user: AdminUserUsage) => { + const handleViewChatsClick = (e: React.MouseEvent<HTMLButtonElement>, user: AdminUserUsage) => { e.stopPropagation() onUserChatsClick?.(user.userId, user.userName) }And on the button:
-<button +<button type="button"
1714-1733: Per-row CTAs LGTM; minor performance note.Rendering is fine. Consider precomputing a userId→rank map once to avoid sorting per row (Lines 1629–1633).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/Dashboard.tsx(11 hunks)server/api/admin.ts(9 hunks)server/db/message.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/db/message.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T08:44:08.412Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#719
File: server/api/chat/chat.ts:4023-4029
Timestamp: 2025-08-11T08:44:08.412Z
Learning: In server/api/chat/chat.ts and similar files, MayankBansal2004 prefers not to add explicit type definitions for metadata.usage structures when the receiving array is already typed and default values ensure type safety through the || 0 pattern.
Applied to files:
frontend/src/components/Dashboard.tsx
🧬 Code graph analysis (2)
server/api/admin.ts (9)
server/db/client.ts (1)
db(16-16)server/db/ingestion.ts (2)
hasActiveIngestion(127-144)createIngestion(15-24)server/db/user.ts (1)
getUserByEmail(148-157)server/db/schema/messages.ts (1)
messages(31-78)server/db/schema/users.ts (1)
users(27-57)server/db/schema/chats.ts (1)
chats(37-70)server/search/vespa.ts (1)
GetDocument(22-22)server/db/message.ts (1)
fetchUserQueriesForChat(345-369)server/utils.ts (1)
getErrorMessage(103-106)
frontend/src/components/Dashboard.tsx (1)
frontend/src/components/AdminChatsTable.tsx (1)
AdminChat(33-48)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/components/Dashboard.tsx
[error] 4042-4042: bunx tsc -b: TS2322: Type '"/admin/chat-overview"' is not assignable to type '"/call" | "/auth" | "/oauth/success" | "/" | "." | ".." | "/agent" | "/chat" | "/dashboard" | "/knowledgeManagement" | "/search" | "/tuning" | "/workflow" | "/admin/userManagement" | ... 13 more ... | "/admin/integrations"'.
[error] 4047-4047: bunx tsc -b: TS2322: Type '"/admin/chat-overview"' is not assignable to type '"/call" | "/auth" | "/oauth/success" | "/" | "." | ".." | "/agent" | "/chat" | "/dashboard" | "/knowledgeManagement" | "/search" | "/tuning" | "/workflow" | "/admin/userManagement" | ... 13 more ... | "/admin/integrations"'.
[error] 4048-4048: bunx tsc -b: TS2353: Object literal may only specify known properties, and 'userName' does not exist in type 'ParamsReducerFn<Router<RootRoute<undefined, {}, AnyContext, AnyContext, {}, undefined, RootRouteChildren, FileRouteTypes>, ... 4 more ..., Record<...>>, "SEARCH", string, "/admin/chat-overview">'.
🔇 Additional comments (5)
server/api/admin.ts (2)
1735-1751: Good implementation of server-side filtering with parameterized queries.The filter logic correctly handles agent/normal chat types (lines 1736-1740) and search functionality (lines 1743-1751) using parameterized SQL queries to prevent SQL injection. This is a solid implementation.
1767-1770: Excellent: SQL-based pagination with proper deleted message exclusion.The aggregations (lines 1767-1770), sorting (lines 1776-1792), and pagination (lines 1799-1804) are now properly implemented at the SQL level. Deleted messages are correctly excluded from all calculations using
CASE WHEN ${messages.deletedAt} IS NULL, which ensures accurate metrics and prevents loading unnecessary data into memory.Also applies to: 1776-1792, 1799-1804
frontend/src/components/Dashboard.tsx (3)
1512-1520: Prop additions for admin navigation look good.
Wiring onAllChatsClick/onUserChatsClick is clear and optional.
1593-1610: Header CTA integration LGTM.
“All Chats” button is gated on handler presence; good.
2-2: useNavigate import/usage LGTM.
Matches SPA navigation best practices.Also applies to: 2642-2642
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server/api/admin.ts (1)
192-198: RenameoffsettolimitorpageSizefor clarity.The parameter named
offsetis actually the page size (items per page), not a positional offset. In standard pagination terminology, "offset" refers to the starting position (e.g.,(page - 1) * limit), which can confuse developers. This concern was raised in previous reviews and remains unresolved.Apply this diff to improve clarity:
- offset: z + limit: z .string() .optional() .refine((val) => !val || (!isNaN(Number(val)) && Number(val) >= 0), { - message: "Offset must be a non-negative number", + message: "Limit must be a non-negative number", }) .transform((val) => (val ? Number(val) : 20)),Then update the usage in
GetAdminChatsat lines 1725, 1811:- const offset = query.offset ? Number(query.offset) : 20 + const limit = query.limit ? Number(query.limit) : 20 // ... later ... - const limit = offset || 20 + const pageSize = limit || 20
🧹 Nitpick comments (1)
server/api/admin.ts (1)
2410-2450: LGTM! Properly workspace-scoped chat queries endpoint.The
GetChatQueriesApiendpoint correctly:
- Validates the
chatIdparameter (lines 2413-2421)- Extracts
currentWorkspaceIdfrom the JWT payload (line 2422)- Passes workspace ID to
fetchUserQueriesForChatfor workspace scoping (lines 2424-2428)- Handles errors appropriately with proper status codes (lines 2437-2449)
The
fetchUserQueriesForChatfunction (from server/db/message.ts) ensures workspace isolation by filtering messages witheq(messages.workspaceExternalId, workspaceExternalId)and also excludes deleted messages withisNull(messages.deletedAt).Optional enhancement: Consider explicit chat ownership verification.
Currently, if a
chatIdfrom another workspace is provided, the query returns an empty array (no messages match). While this is safe, you could improve UX by explicitly checking if the chat exists and belongs to the workspace first:// After line 2422, before calling fetchUserQueriesForChat: const chatResult = await db .select({ id: chats.id }) .from(chats) .where( and( eq(chats.externalId, chatId), eq(chats.workspaceExternalId, currentWorkspaceId) ) ) .limit(1) if (chatResult.length === 0) { return c.json( { success: false, message: "Chat not found or access denied", }, 404, ) }This provides clearer error feedback when an invalid
chatIdis provided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api/admin.ts(10 hunks)server/server.ts(6 hunks)server/types.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/api/admin.ts (8)
server/db/client.ts (1)
db(16-16)server/db/ingestion.ts (2)
hasActiveIngestion(127-144)createIngestion(15-24)server/db/schema/messages.ts (1)
messages(31-78)server/db/schema/users.ts (1)
users(27-57)server/db/schema/chats.ts (1)
chats(37-70)server/search/vespa.ts (1)
GetDocument(22-22)server/db/message.ts (1)
fetchUserQueriesForChat(345-369)server/utils.ts (1)
getErrorMessage(103-106)
server/server.ts (3)
server/api/ingestion.ts (3)
cancelIngestionSchema(32-34)pauseIngestionSchema(41-43)resumeIngestionSchema(36-38)server/types.ts (1)
chatIdParamSchema(245-247)server/api/admin.ts (1)
GetChatQueriesApi(2410-2450)
🔇 Additional comments (8)
server/types.ts (1)
245-247: LGTM! Clean parameter validation schema.The
chatIdParamSchemaprovides appropriate validation for the new chat queries endpoint with a clear error message for non-empty string requirements.server/server.ts (3)
48-48: LGTM! Proper imports for new chat queries endpoint.The imports correctly support the new
/admin/chat/queries/:chatIdroute added at lines 1322-1326.Also applies to: 83-83
1322-1326: LGTM! Properly secured admin endpoint.The new chat queries endpoint correctly:
- Applies
AdminRoleMiddlewarefor role-based access- Validates
chatIdparameter withzValidatorandchatIdParamSchema- Delegates to
GetChatQueriesApiwhich includes workspace scoping (confirmed at line 2422 in admin.ts)
1448-1506: LGTM! Improved proxy function with GET request support.The refactored
proxyToSyncServerfunction correctly:
- Adds method parameter to support both GET and POST requests
- Properly handles query parameters for GET requests
- Conditionally includes request body only for non-GET requests
- Maintains authentication token forwarding
server/api/admin.ts (4)
29-45: LGTM! Necessary imports for server-side query operations.The expanded drizzle-orm imports properly support the server-side pagination, filtering, and sorting implementation in
GetAdminChats.
160-160: LGTM! Proper import for chat queries functionality.The
fetchUserQueriesForChatimport correctly supports the newGetChatQueriesApiendpoint with workspace-scoped query retrieval.
1627-1672: LGTM! Well-structured resumable ingestion implementation.The changes properly support resumable Slack channel ingestion:
- Dynamic import of ingestion helpers is appropriate for this code path
- 409 Conflict status correctly prevents concurrent ingestions
- Comprehensive metadata structure includes both progress tracking (
websocketData) and resumability state (ingestionState)- Passing
ingestion.idenables proper progress trackingAlso applies to: 1685-1685
1714-1840: Excellent server-side pagination and filtering implementation!The refactored
GetAdminChatscorrectly implements:
- SQL-based pagination with
LIMITandOFFSET(lines 1819-1820)- Deleted message exclusion in both conditions and aggregations (lines 1734, 1783-1786)
- Workspace scoping (line 1738)
- Case-insensitive search using
ilike()(lines 1761-1766)- Dynamic sorting based on
sortByparameter (lines 1792-1808)- Proper aggregation with
count()andsum()that excludes deleted messagesThis addresses the concerns from previous reviews about JavaScript-based filtering and pagination.
However, clarify the commented-out SuperAdmin check (lines 1736-1739).
The code contains a commented-out block for SuperAdmin cross-workspace access:
// if (!isSuperAdmin) { conditions.push(eq(users.workspaceExternalId, currentWorkspaceId)) // }This means workspace filtering is always applied, preventing SuperAdmins from viewing chats across workspaces. Is this intentional, or should SuperAdmins have cross-workspace visibility?
If cross-workspace access for SuperAdmins is needed, uncomment and implement the check:
+ const { sub } = c.get(JwtPayloadKey) + const currentUserRes = await getUserByEmail(db, sub) + if (!currentUserRes || !currentUserRes.length) { + throw new HTTPException(403, { message: "User not found" }) + } + const currentUser = currentUserRes[0] + const isSuperAdmin = currentUser.role === "SuperAdmin" + - // if (!isSuperAdmin) { + if (!isSuperAdmin) { conditions.push(eq(users.workspaceExternalId, currentWorkspaceId)) - // } + }
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (3)
1-7: Fix TanStack Router imports to resolve TypeScript build errors.The pipeline shows TS2345 and TS2322 errors caused by the route path typing and
useSearchusage pattern. Remove theuseSearchimport here and useRoute.useSearch()in the component instead (see also Line 63-65).Apply this diff:
import { createFileRoute, redirect, useRouterState, useNavigate, - useSearch, } from "@tanstack/react-router"Also applies to: 27-27, 63-65
27-27: Add ts-expect-error to silence path inference error.The pipeline shows a TS2345 error because TypeScript cannot infer the route path in
FileRoutesByPath. Add a single-line comment to silence this typing error while preserving runtime behavior.Apply this diff:
+// @ts-expect-error: Path literal is validated by the file-based router plugin at build time. export const Route = createFileRoute("/_authenticated/admin/chat-overview")({
63-65: Replace useSearch with Route.useSearch() to fix TypeScript error.The pipeline shows a TS2322 error at Line 64. Use
Route.useSearch()which provides the correct typing without needing the "from" parameter or type assertion.Apply this diff:
- const search = useSearch({ - from: "/_authenticated/admin/chat-overview", - }) as ChatOverviewSearch + const search = Route.useSearch()
🧹 Nitpick comments (3)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (3)
51-55: Consider replacinganytypes with proper interfaces.The
userandworkspaceprops are typed asany, which removes type safety. Consider defining proper interfaces or importing existing types for better type checking and IDE support.
106-109: Type the query object properly instead of usingany.Consider defining a proper type or interface for the query parameters to improve type safety.
- const query: any = { + const query: Record<string, string> = { page: currentPage.toString(), offset: pageSize.toString(), }
141-164: Consider extracting the mapping logic to a helper function.The inline transformation from API response to
AdminChattype is quite large. Extracting it to a named function would improve readability and make the mapping logic reusable and testable.Example:
const mapToAdminChat = (chat: any, adminAgents: any[]): AdminChat => ({ externalId: chat.externalId, title: chat.title || "Untitled Chat", createdAt: chat.createdAt, userId: chat.userId ?? chat.user?.id ?? 0, userName: (chat.userName ?? chat.user?.name ?? "Unknown User") as string, userEmail: (chat.userEmail ?? chat.user?.email ?? "") as string, agentId: chat.agentId, agentName: chat.agentName || (chat.agentId ? adminAgents.find((a: any) => a.externalId === chat.agentId)?.name : null), messageCount: chat.messageCount || chat.messages?.length || 0, totalCost: chat.totalCost || 0, totalTokens: chat.totalTokens || 0, likes: chat.likes || 0, dislikes: chat.dislikes || 0, isBookmarked: chat.isBookmarked || false, }) // Then use: setAdminChats(adminChats.map(chat => mapToAdminChat(chat, adminAgents)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/routes/_authenticated/admin/chat-overview.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (4)
frontend/src/components/error.tsx (1)
errorComponent(1-6)frontend/src/components/AdminChatsTable.tsx (2)
AdminChat(33-48)AdminChatsTable(271-635)frontend/src/api.ts (1)
api(5-5)frontend/src/components/Sidebar.tsx (1)
Sidebar(40-390)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/routes/_authenticated/admin/chat-overview.tsx
[error] 27-27: bunx tsc -b: TS2345: Argument of type '"/_authenticated/admin/chat-overview"' is not assignable to parameter of type 'keyof FileRoutesByPath'. (src/routes/_authenticated/admin/chat-overview.tsx:27)
[error] 64-64: bunx tsc -b: TS2322: Type '"/_authenticated/admin/chat-overview"' is not assignable to type 'ConstrainLiteral<"/_authenticated/admin/chat-overview", "root" | "/call" | "/_authenticated" | "/auth" | "/_authenticated/agent" | "/_authenticated/chat" | "/_authenticated/dashboard" | ... 20 more ... | "/_authenticated/admin/integrations/">'. (src/routes/_authenticated/admin/chat-overview.tsx:64)
🔇 Additional comments (6)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (6)
66-82: LGTM - well-organized state management.The state is properly typed and the filter state is correctly lifted to the parent component to support server-side pagination, addressing the previous review feedback.
84-86: LGTM - correct pagination reset logic.Resetting to page 1 when filters change is the expected behavior for server-side pagination.
89-97: LGTM - clean search handlers.The separation of
searchInput(UI state) andsearchQuery(API state) provides good UX by avoiding API calls on every keystroke.
128-135: LGTM - efficient parallel data fetching.Using
Promise.allto fetch admin chats and agents in parallel is a good practice that improves performance.
187-204: LGTM - clean helper functions.Both navigation and page info helpers are straightforward and provide good UX with context-aware titles.
208-243: LGTM - well-structured layout and error handling.The UI composition is clean with appropriate error state display and context-aware page titles.
server/api/admin.ts
Outdated
| messageCount: count(messages.id), | ||
| likes: sql<number>`COUNT(CASE WHEN ${messages.feedback}->>'type' = 'like' AND ${messages.deletedAt} IS NULL THEN 1 END)::int`, | ||
| dislikes: sql<number>`COUNT(CASE WHEN ${messages.feedback}->>'type' = 'dislike' AND ${messages.deletedAt} IS NULL THEN 1 END)::int`, | ||
| totalCost: sql<number>`COALESCE(SUM(CASE WHEN ${messages.deletedAt} IS NULL THEN ${messages.cost} ELSE 0 END), 0)::numeric`, | ||
| totalTokens: sql<number>`COALESCE(SUM(CASE WHEN ${messages.deletedAt} IS NULL THEN ${messages.tokensUsed} ELSE 0 END), 0)::bigint`, |
There was a problem hiding this comment.
cant be completely converted to orm function because orm functions dont provide access to jsonb fields
| let orderByClause | ||
| switch (sortBy) { | ||
| case "messages": | ||
| orderByClause = sql`COUNT(CASE WHEN ${messages.deletedAt} IS NULL THEN ${messages.id} END) DESC` | ||
| break | ||
| case "cost": | ||
| orderByClause = sql`COALESCE(SUM(CASE WHEN ${messages.deletedAt} IS NULL THEN ${messages.cost} ELSE 0 END), 0) DESC` | ||
| break | ||
| case "tokens": | ||
| orderByClause = sql`COALESCE(SUM(CASE WHEN ${messages.deletedAt} IS NULL THEN ${messages.tokensUsed} ELSE 0 END), 0) DESC` | ||
| break | ||
| case "created": | ||
| default: | ||
| orderByClause = desc(chats.createdAt) | ||
| break | ||
| } |
There was a problem hiding this comment.
converted to ORM functions wherever possible, used sql only where orm functions did not suffice
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
frontend/src/components/Dashboard.tsx (2)
2706-2706: Remove dead state and duplicate mapping; centralize mapping in the route.
setAdminChats(...)writes to an unused state (you intentionally dropped the reader). Also, mapping logic duplicates the route’s mapper. Delete the state, its import, and the mapping block.Apply:
- import type { AdminChat } from "@/components/AdminChatsTable" + // (removed) AdminChat no longer needed @@ - const [_, setAdminChats] = useState<AdminChat[]>([]) + // Removed unused adminChats state @@ - // Set the raw admin chats data for the table - setAdminChats( - chatsArray.map((chat: any) => ({ - externalId: chat.externalId, - title: chat.title || "Untitled Chat", - createdAt: chat.createdAt, - userId: chat.userId || chat.user?.id, - userName: chat.userName || chat.user?.name || "Unknown User", - userEmail: chat.userEmail || chat.user?.email || "", - agentId: chat.agentId, - agentName: - chat.agentName || - (chat.agentId - ? adminAgents.find((a: any) => a.externalId === chat.agentId) - ?.name - : null), - messageCount: chat.messageCount || chat.messages?.length || 0, - totalCost: chat.totalCost || 0, - totalTokens: chat.totalTokens || 0, - likes: chat.likes || 0, - dislikes: chat.dislikes || 0, - isBookmarked: chat.isBookmarked || false, - })), - ) + // Mapping handled in route; no local state neededAlso applies to: 2921-2944, 41-41
4057-4067: Fix router path for admin chat-overview navigation to use authenticated route.The route path must include the
/_authenticatedprefix to match the TanStack Router configuration. Additionally, update the unused parameter naming and simplify the search param syntax.onAllChatsClick={() => { - // Navigate to all chats view - navigate({ to: "/admin/chat-overview" }) + navigate({ to: "/_authenticated/admin/chat-overview" }) }} onUserChatsClick={(userId: number, userName: string) => { - // Navigate to user-specific chats view navigate({ - to: "/admin/chat-overview", + to: "/_authenticated/admin/chat-overview", - search: { userName: userName }, + search: { userName }, }) }}frontend/src/routes/_authenticated/admin/chat-overview.tsx (1)
299-302: Remove leftover console.log in production.- onChatView={(chat: AdminChat) => { - console.log("Viewing chat:", chat.externalId) - // You can implement chat viewing functionality here if needed - }} + onChatView={(_chat: AdminChat) => { + // Hook up a view handler here if needed + }}
🧹 Nitpick comments (5)
server/api/admin.ts (2)
192-198: Renameoffsettolimitfor standard pagination terminology.The parameter named
offsetrepresents the page size (items per page), not a positional offset. In standard pagination, "offset" refers to the starting position(page - 1) * limit, which creates confusion. This was flagged in previous reviews but remains unaddressed.Apply this diff to improve clarity:
- offset: z + limit: z .string() .optional() .refine((val) => !val || (!isNaN(Number(val)) && Number(val) >= 0), { - message: "Offset must be a non-negative number", + message: "Limit must be a non-negative number", }) .transform((val) => (val ? Number(val) : 20)),Then update usage throughout GetAdminChats (lines 1725, 1844, 1863, 1872) to use
limitinstead ofoffset.
1737-1739: Remove commented-out SuperAdmin check or implement cross-workspace access.The commented-out lines suggest incomplete implementation of SuperAdmin cross-workspace access. Based on past review comments, this was supposed to be addressed but remains commented out.
Option 1: If cross-workspace access isn't needed, remove the comments:
// Build the conditions array const conditions = [] // Always exclude deleted messages conditions.push(isNull(messages.deletedAt)) - // Add workspace filtering unless user is SuperAdmin - // if (!isSuperAdmin) { conditions.push(eq(users.workspaceExternalId, currentWorkspaceId)) - // }Option 2: If SuperAdmins should see cross-workspace chats, implement it:
+ // Get current user to check role + const { sub } = c.get(JwtPayloadKey) + const currentUserRes = await getUserByEmail(db, sub) + if (!currentUserRes || !currentUserRes.length) { + throw new HTTPException(403, { message: "User not found" }) + } + const isSuperAdmin = currentUserRes[0].role === "SuperAdmin" + // Build the conditions array const conditions = [] // Always exclude deleted messages conditions.push(isNull(messages.deletedAt)) // Add workspace filtering unless user is SuperAdmin - // if (!isSuperAdmin) { + if (!isSuperAdmin) { conditions.push(eq(users.workspaceExternalId, currentWorkspaceId)) - // } + }frontend/src/components/Dashboard.tsx (1)
1629-1633: Don’t mutate props when computing rank.
users.sort(...)mutates the prop array. Use a copy.- const originalRank = - users - .sort((a, b) => b.messageCount - a.messageCount) - .findIndex((u) => u.userId === user.userId) + 1 + const originalRank = + [...users] + .sort((a, b) => b.messageCount - a.messageCount) + .findIndex((u) => u.userId === user.userId) + 1frontend/src/routes/_authenticated/admin/chat-overview.tsx (2)
73-76: Search state: initialize from URL; prefer single source of truth.Good initializing from URL. Consider removing the extra
search.searchcheck to avoid ambiguity and just rely onsearchQuery.- if (search.search) { - query.search = search.search - } if (searchQuery.trim()) { query.search = searchQuery.trim() }Also applies to: 112-118
178-202: Type normalization in chat mapper (optional hardening).Coerce numeric fields to numbers to avoid accidental strings from API.
- userId: chat.userId ?? chat.user?.id ?? 0, + userId: Number(chat.userId ?? chat.user?.id ?? 0) || 0, @@ - messageCount: chat.messageCount || chat.messages?.length || 0, - totalCost: chat.totalCost || 0, - totalTokens: chat.totalTokens || 0, + messageCount: Number(chat.messageCount ?? chat.messages?.length ?? 0) || 0, + totalCost: Number(chat.totalCost ?? 0) || 0, + totalTokens: Number(chat.totalTokens ?? 0) || 0,(Based on learnings)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/Dashboard.tsx(10 hunks)frontend/src/routes/_authenticated/admin/chat-overview.tsx(1 hunks)server/api/admin.ts(11 hunks)server/types.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/types.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T08:44:08.412Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#719
File: server/api/chat/chat.ts:4023-4029
Timestamp: 2025-08-11T08:44:08.412Z
Learning: In server/api/chat/chat.ts and similar files, MayankBansal2004 prefers not to add explicit type definitions for metadata.usage structures when the receiving array is already typed and default values ensure type safety through the || 0 pattern.
Applied to files:
frontend/src/components/Dashboard.tsx
🧬 Code graph analysis (3)
server/api/admin.ts (7)
server/db/client.ts (1)
db(16-16)server/db/ingestion.ts (2)
hasActiveIngestion(127-144)createIngestion(15-24)server/db/schema/messages.ts (1)
messages(31-78)server/db/schema/users.ts (1)
users(27-57)server/db/schema/chats.ts (1)
chats(37-70)server/db/message.ts (1)
fetchUserQueriesForChat(345-369)server/utils.ts (1)
getErrorMessage(103-106)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (4)
frontend/src/components/error.tsx (1)
errorComponent(1-6)frontend/src/components/AdminChatsTable.tsx (2)
AdminChat(33-48)AdminChatsTable(271-635)frontend/src/api.ts (1)
api(5-5)frontend/src/components/Sidebar.tsx (1)
Sidebar(40-390)
frontend/src/components/Dashboard.tsx (1)
frontend/src/components/AdminChatsTable.tsx (1)
AdminChat(33-48)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/routes/_authenticated/admin/chat-overview.tsx
[error] 23-23: Argument of type '"/_authenticated/admin/chat-overview"' is not assignable to parameter of type 'keyof FileRoutesByPath'.
[error] 74-74: Property 'search' does not exist on the type of the route query options.
[error] 113-113: Property 'search' does not exist on the route query options type.
[error] 114-114: Property 'search' does not exist on the route query options type.
[error] 216-216: Property 'search' does not exist on the route query options type.
[error] 231-231: Property 'userName' does not exist on the route query options type.
[error] 233-233: Property 'userName' does not exist on the route query options type.
[error] 234-234: Property 'userName' does not exist on the route query options type.
[error] 298-298: Property 'userName' does not exist on the route query options type.
frontend/src/components/Dashboard.tsx
[error] 4059-4059: Type '"/admin/chat-overview"' is not assignable to the union of allowed router paths in the component props.
[error] 4064-4064: Type '"/admin/chat-overview"' is not assignable to the union of allowed router paths in the component props.
[error] 4065-4065: Object literal may only specify known properties, and 'userName' does not exist in the expected props type.
🔇 Additional comments (12)
server/api/admin.ts (7)
29-45: LGTM! Import additions support new features.The added Drizzle ORM operators and
fetchUserQueriesForChatimport are appropriately used for the new pagination, filtering, sorting, and chat queries functionality.
1627-1685: LGTM! Proper concurrency handling for ingestions.The transaction-based check prevents race conditions, and the 409 Conflict status appropriately signals concurrent ingestion attempts. The ingestion metadata structure supports resumability.
1716-1729: Consider using validated query parameters.The query parameters are parsed manually using
c.req.query()instead of leveraging the validated query fromc.req.valid("query")with theadminQuerySchema. While the schema is defined, it doesn't appear to be used here via validation middleware.If the
adminQuerySchemavalidation is applied via middleware elsewhere in the routing, the manual parsing here is redundant. Otherwise, consider using:// @ts-ignore const { from, to, userId, page, offset, search, filterType, sortBy } = c.req.valid("query")This ensures consistent validation and leverages the schema transforms (trimming, defaults, etc.).
1770-1780: Note: Count query approach for paginated aggregations.The count query groups by
chats.idand then counts the result length. While this may seem inefficient, it's necessary to get an accurate count when usingGROUP BYwith joins, as a simpleCOUNT(*)would count rows before grouping.This approach is acceptable but be aware that for very large datasets (100k+ chats), performance may degrade. Consider adding an index on commonly filtered columns if not already present.
1796-1807: Excellent handling of deleted messages in aggregations.The use of
CASEstatements to exclude deleted messages from counts and cost/token sums is correct and prevents data leakage from soft-deleted records.
2459-2499: LGTM! Authorization handled via workspace validation.The endpoint properly authorizes access by passing
currentWorkspaceIdtofetchUserQueriesForChat, which validates that the chat belongs to the user's workspace. This prevents unauthorized cross-workspace access while maintaining a clean implementation.Error handling appropriately logs errors and returns structured responses.
2408-2408: Function name change verified as correct.GetDocument is properly exported from server/search/vespa.ts, and the parameters match the call site. This is an internal refactoring within
GetKbVespaContent—the public API remains unchanged sinceGetKbVespaContentis still exported and used by the route handler.frontend/src/components/Dashboard.tsx (1)
2893-2912: Nice: backward-compat handling for new/old admin chats response.If the backend settles on metadata shape, add a type to avoid
anyand narrow keys. I can wire this quickly if you share the final schema.frontend/src/routes/_authenticated/admin/chat-overview.tsx (4)
325-327: Avoid misleading “Total” when total is unknown.This already guards on undefined. With the change above (optional totalCount), UI remains correct. No further action if you adopt that change.
129-137: Parallel fetch + guards look good.Fetching chats and agents together with proper error checks. LGTM.
231-241: Page title from userName query: good UX.Works once validateSearch typing is fixed.
106-124: Param names verified — no mispagination issue found.The server correctly expects
pageandoffsetparameters (lines 1724–1725 ofserver/api/admin.ts). The pagination logic properly calculates database offset as(page - 1) * pageSize(line 1845) and applies it correctly (line 1853). The frontend and backend param names align, and pagination functions as intended.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (6)
79-87: URL search params and local state remain out of sync.This is the same issue flagged in a previous review. Line 79 initializes
searchInputfromsearch.search, butsearchQuery(which drives the API call) is initialized to an empty string. When a user navigates with?search=fooin the URL, the input field will show "foo" but the API won't receive the search term until the user manually triggers the search.The previous review suggested initializing both from URL params:
- const [searchInput, setSearchInput] = useState<string>(search.search || "") - const [searchQuery, setSearchQuery] = useState<string>("") + const [searchInput, setSearchInput] = useState<string>(search.search || "") + const [searchQuery, setSearchQuery] = useState<string>(search.search || "")Alternatively, remove the duplicate
search.searchcheck at lines 118-120 and rely solely onsearchQuery.
118-123: Remove duplicate search filter logic.Lines 118-120 check
search.searchand lines 121-123 checksearchQuery.trim(), potentially applying the same filter twice or causing confusion about which takes precedence.As noted in the previous review, this duplication should be removed. Since
searchQueryis the canonical state for the search term, remove thesearch.searchcheck:// Add server-side filters - if (search.search) { - query.search = search.search - } if (searchQuery.trim()) { query.search = searchQuery.trim() }
167-175: Fallback pagination metadata is misleading and breaks on exact multiples of pageSize.The fallback code sets
totalCounttoadminChatsData.length, which is only the current page count, not the total across all pages. This makes the UI label "Total: X" misleading to users. Additionally, thehasNextPagelogic (adminChatsData.length >= pageSize) fails when total items are an exact multiple ofpageSize, allowing navigation to an empty page.This issue was flagged in previous reviews. The recommended fix is to request
pageSize + 1items and use the extra item as a probe for whether more pages exist:const query: any = { page: currentPage.toString(), - offset: pageSize.toString(), + offset: (pageSize + 1).toString(), }Then update the fallback logic:
} else if (Array.isArray(adminChatsData)) { // Old format - direct array - chatsArray = adminChatsData - // Fallback: use length-based logic for hasNextPage + const hasExtra = adminChatsData.length > pageSize + chatsArray = hasExtra ? adminChatsData.slice(0, pageSize) : adminChatsData paginationMeta = { - totalCount: adminChatsData.length, // This is just the current page count - hasNextPage: adminChatsData.length >= pageSize, // Old logic as fallback + // Don't include totalCount when we can't determine it accurately + hasNextPage: hasExtra, hasPreviousPage: currentPage > 1, }Also update the state type to make
totalCountoptional:const [paginationMetadata, setPaginationMetadata] = useState<{ - totalCount: number + totalCount?: number hasNextPage: boolean hasPreviousPage: boolean } | null>(null)
304-307: Remove debug console.log before merging.The
console.logstatement is leftover debug code and should be removed from production.onChatView={(chat: AdminChat) => { - console.log("Viewing chat:", chat.externalId) // You can implement chat viewing functionality here if needed }}
328-333: Misleading "Total" count display.The label shows
paginationMetadata.totalCount, which in the fallback mode is just the current page count, not the total across all pages. This is confusing for users.As noted in previous reviews, either remove the count display when using the fallback, or change the label to clarify:
<span className="text-sm text-muted-foreground"> per page - {paginationMetadata?.totalCount !== undefined && ( - <> (Total: {paginationMetadata.totalCount})</> + {paginationMetadata?.totalCount !== undefined && ( + <> (Showing: {adminChats.length} of {paginationMetadata.totalCount})</> )} </span>Or only show total when it's truly the total count (not in fallback mode):
<span className="text-sm text-muted-foreground"> - per page - {paginationMetadata?.totalCount !== undefined && ( - <> (Total: {paginationMetadata.totalCount})</> - )} + per page (Showing: {adminChats.length}) </span>
17-24: Schema definition is correct but route typing integration is broken.The Zod schema is properly defined, but TanStack Router isn't inferring the search type correctly (as evidenced by the 9 TypeScript errors). The
validateSearchfunction needs to be structured so thatRoute.useSearch()returns the parsedChatOverviewSearchtype.The previous review suggested adding
ts-expect-errorand adjustingvalidateSearch, but a more robust fix ensures full type safety. Apply this approach:const chatOverviewSearchSchema = z.object({ userName: z.string().optional(), page: z.string().optional(), offset: z.string().optional(), search: z.string().optional(), }) type ChatOverviewSearch = z.infer<typeof chatOverviewSearchSchema> +// @ts-expect-error: TanStack Router path literal is validated at build time export const Route = createFileRoute("/_authenticated/admin/chat-overview")({ beforeLoad: ({ context }) => { if ( !(context.user.role === "Admin" || context.user.role === "SuperAdmin") ) { throw redirect({ to: "/" }) } }, - validateSearch: (search: Record<string, unknown>): ChatOverviewSearch => { - return chatOverviewSearchSchema.parse(search) - }, + validateSearch: chatOverviewSearchSchema.parse, component: () => { const matches = useRouterState({ select: (s) => s.matches }) const { user, workspace, agentWhiteList } = matches[matches.length - 1].context return ( <ChatOverviewPage user={user} workspace={workspace} agentWhiteList={agentWhiteList} /> ) }, errorComponent: errorComponent, })This ensures
Route.useSearch()returns the correctly typedChatOverviewSearchobject, eliminating all 9 TypeScript errors.frontend/src/components/Dashboard.tsx (2)
41-41: Remove unused AdminChat state and duplicate mapping (wasted work + future drift).State is never read; the heavy mapping runs each admin fetch doing nothing and duplicates logic already in the admin route. Delete the import, state, and mapping block.
Apply:
- import type { AdminChat } from "@/components/AdminChatsTable" + // (removed) AdminChat mapping no longer needed here @@ - const [_, setAdminChats] = useState<AdminChat[]>([]) + // removed unused admin chats state @@ - // Set the raw admin chats data for the table - setAdminChats( - chatsArray.map((chat: any) => ({ - externalId: chat.externalId, - title: chat.title || "Untitled Chat", - createdAt: chat.createdAt, - userId: chat.userId || chat.user?.id, - userName: chat.userName || chat.user?.name || "Unknown User", - userEmail: chat.userEmail || chat.user?.email || "", - agentId: chat.agentId, - agentName: - chat.agentName || - (chat.agentId - ? adminAgents.find((a: any) => a.externalId === chat.agentId) - ?.name - : null), - messageCount: chat.messageCount || chat.messages?.length || 0, - totalCost: chat.totalCost || 0, - totalTokens: chat.totalTokens || 0, - likes: chat.likes || 0, - dislikes: chat.dislikes || 0, - isBookmarked: chat.isBookmarked || false, - })), - ) + // removed duplicate mapping; route-level overview handles table dataIf you must keep it, harden types as below (next comment).
Also applies to: 2706-2706, 2922-2944
2922-2944: Type safety: possible undefined userId and unnormalized fields in AdminChat mapping.Ensure userId is numeric and name/email are strings; coerce numeric totals.
Apply:
- setAdminChats( - chatsArray.map((chat: any) => ({ + setAdminChats( + chatsArray.map((chat: any) => ({ externalId: chat.externalId, title: chat.title || "Untitled Chat", createdAt: chat.createdAt, - userId: chat.userId || chat.user?.id, - userName: chat.userName || chat.user?.name || "Unknown User", - userEmail: chat.userEmail || chat.user?.email || "", + userId: Number(chat.userId ?? chat.user?.id ?? 0) || 0, + userName: String(chat.userName ?? chat.user?.name ?? "Unknown User"), + userEmail: String(chat.userEmail ?? chat.user?.email ?? ""), agentId: chat.agentId, agentName: - chat.agentName || - (chat.agentId - ? adminAgents.find((a: any) => a.externalId === chat.agentId) - ?.name - : null), - messageCount: chat.messageCount || chat.messages?.length || 0, - totalCost: chat.totalCost || 0, - totalTokens: chat.totalTokens || 0, - likes: chat.likes || 0, - dislikes: chat.dislikes || 0, - isBookmarked: chat.isBookmarked || false, + chat.agentName ?? + (chat.agentId + ? adminAgents.find((a: any) => a.externalId === chat.agentId)?.name ?? null + : null), + messageCount: Number(chat.messageCount ?? chat.messages?.length ?? 0) || 0, + totalCost: Number(chat.totalCost ?? 0) || 0, + totalTokens: Number(chat.totalTokens ?? 0) || 0, + likes: Number(chat.likes ?? 0) || 0, + dislikes: Number(chat.dislikes ?? 0) || 0, + isBookmarked: Boolean(chat.isBookmarked), })), )Based on learnings.
🧹 Nitpick comments (1)
frontend/src/components/Dashboard.tsx (1)
2893-2912: Harden back-compat shape check for adminChats.Also ensure data is an array before use; otherwise error messages are noisy.
Apply:
- if ( - adminChatsData && - typeof adminChatsData === "object" && - "data" in adminChatsData && - "pagination" in adminChatsData - ) { - // New format with pagination metadata - chatsArray = adminChatsData.data - } else if (Array.isArray(adminChatsData)) { + if ( + adminChatsData && + typeof adminChatsData === "object" && + "data" in adminChatsData && + Array.isArray((adminChatsData as any).data) + ) { + chatsArray = (adminChatsData as any).data + } else if (Array.isArray(adminChatsData)) { // Old format - direct array chatsArray = adminChatsData } else { throw new Error("Invalid response format from admin chats API") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Dashboard.tsx(10 hunks)frontend/src/routes/_authenticated/admin/chat-overview.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T08:44:08.412Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#719
File: server/api/chat/chat.ts:4023-4029
Timestamp: 2025-08-11T08:44:08.412Z
Learning: In server/api/chat/chat.ts and similar files, MayankBansal2004 prefers not to add explicit type definitions for metadata.usage structures when the receiving array is already typed and default values ensure type safety through the || 0 pattern.
Applied to files:
frontend/src/components/Dashboard.tsx
🧬 Code graph analysis (2)
frontend/src/routes/_authenticated/admin/chat-overview.tsx (4)
frontend/src/components/error.tsx (1)
errorComponent(1-6)frontend/src/components/AdminChatsTable.tsx (2)
AdminChat(33-48)AdminChatsTable(271-635)frontend/src/api.ts (1)
api(5-5)frontend/src/components/Sidebar.tsx (1)
Sidebar(40-390)
frontend/src/components/Dashboard.tsx (1)
frontend/src/components/AdminChatsTable.tsx (1)
AdminChat(33-48)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/routes/_authenticated/admin/chat-overview.tsx
[error] 26-26: Argument of type '"/_authenticated/admin/chat-overview"' is not assignable to parameter of type 'keyof FileRoutesByPath'.
[error] 79-79: Property 'search' does not exist on type '{ app?: Apps | undefined; entity?: string | undefined; offset?: number | undefined; page?: number | undefined; query?: string | undefined; lastUpdated?: string | undefined; debug?: boolean | undefined; } | { ...; } | {} | { ...; }'.
[error] 118-118: Property 'search' does not exist on type '{ app?: Apps | undefined; entity?: string | undefined; offset?: number | undefined; page?: number | undefined; query?: string | undefined; lastUpdated?: string | undefined; debug?: boolean | undefined; } | { ...; } | {} | { ...; }'.
[error] 119-119: Property 'search' does not exist on type '{ app?: Apps | undefined; entity?: string | undefined; offset?: number | undefined; page?: number | undefined; query?: string | undefined; lastUpdated?: string | undefined; debug?: boolean | undefined; } | { ...; } | {} | { ...; }'.
[error] 221-221: Property 'search' does not exist on type '{ app?: Apps | undefined; entity?: string | undefined; offset?: number | undefined; page?: number | undefined; query?: string | undefined; lastUpdated?: string | undefined; debug?: boolean | undefined; } | { ...; } | {} | { ...; }'.
[error] 236-236: Property 'userName' does not exist on type '{ app?: Apps | undefined; entity?: string | undefined; offset?: number | undefined; page?: number | undefined; query?: string | undefined; lastUpdated?: string | undefined; debug?: boolean | undefined; } | { ...; } | {} | { ...; }'.
[error] 238-238: Property 'userName' does not exist on type '{ app?: Apps | undefined; entity?: string | undefined; offset?: number | undefined; page?: number | undefined; query?: string | undefined; lastUpdated?: string | undefined; debug?: boolean | undefined; } | { ...; } | {} | { ...; }'.
[error] 239-239: Property 'userName' does not exist on type '{ app?: Apps | undefined; entity?: string | undefined; offset?: number | undefined; page?: number | undefined; query?: string | undefined; lastUpdated?: string | undefined; debug?: boolean | undefined; } | { ...; } | {} | { ...; }'.
[error] 303-303: Property 'userName' does not exist on type '{ app?: Apps | undefined; entity?: string | undefined; offset?: number | undefined; page?: number | undefined; query?: string | undefined; lastUpdated?: string | undefined; debug?: boolean | undefined; } | { ...; } | {} | { ...; }'.
[error] Process completed with exit code 2.
frontend/src/components/Dashboard.tsx
[error] 4059-4059: Type '"/admin/chat-overview"' is not assignable to type '"/call" | "/auth" | "/oauth/success" | "/" | "." | ".." | "/agent" | "/chat" | "/dashboard" | "/knowledgeManagement" | "/search" | "/tuning" | "/workflow" | "/admin/userManagement" | ... 13 more ... | "/admin/integrations"'.
[error] 4064-4064: Type '"/admin/chat-overview"' is not assignable to type '"/call" | "/auth" | "/oauth/success" | "/" | "." | ".." | "/agent" | "/chat" | "/dashboard" | "/knowledgeManagement" | "/search" | "/tuning" | "/workflow" | "/admin/userManagement" | ... 13 more ... | "/admin/integrations"'.
[error] 4065-4065: Object literal may only specify known properties, and 'userName' does not exist in type 'ParamsReducerFn<Router<RootRoute<undefined, {}, AnyContext, AnyContext, {}, undefined, RootRouteChildren, FileRouteTypes>, ... 4 more ..., Record<...>>, "SEARCH", string, "/admin/chat-overview">'.
[error] Process completed with exit code 2.
🔇 Additional comments (7)
frontend/src/components/Dashboard.tsx (7)
1512-1520: Props extension is sound.Optional callbacks are correctly typed and guarded at call sites.
1544-1547: Good event handling.stopPropagation prevents row click from hijacking per-user actions.
1593-1611: Nice UX: “All Chats” action is conditionally rendered.Clear affordance and safe optional handling.
1715-1733: Row actions are well-scoped.Buttons gated by props/feedback presence; concise styling.
2642-2642: useNavigate import/use is appropriate.Sets up SPA navigation for added actions.
2-2: Import looks good.Router hook is only used inside component; no issues.
4057-4067: Now let me verify the actual Dashboard.tsx code to see the correct implementation:
The route and search key are already correct. The route
/_authenticated/admin/chat-overviewexists and its schema explicitly supportsuserNameas an optional search parameter. The suggested diff breaks this by changinguserNametouserId, which is not in the schema and will cause a runtime validation error. Keep the original code as-is withuserName.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/routeTree.gen.ts (1)
179-184: Route definition is correct, but consider path naming consistency.The route constant is properly defined and follows the established pattern. However, note that the path uses kebab-case (
chat-overview) while other admin routes use camelCase (userManagement). For better consistency across the application, consider aligning route naming conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/routeTree.gen.ts(13 hunks)
🔇 Additional comments (3)
frontend/src/routeTree.gen.ts (3)
35-35: LGTM!The import statement is correctly formatted and follows the established pattern for admin routes.
316-322: LGTM!All type definitions and interface additions are correctly generated. The route is properly integrated into:
FileRoutesByPathAuthenticatedRouteChildren(interface and object)FileRoutesByFullPathFileRoutesByToFileRoutesByIdFileRouteTypesunion typesThe alphabetical ordering is maintained throughout.
Also applies to: 453-453, 479-479, 519-519, 549-549, 581-581, 614-614, 643-643, 673-673
733-733: LGTM!The route manifest entries are correctly generated and properly positioned. The manifest correctly references the source file at
_authenticated/admin/chat-overview.tsxwith the appropriate parent route.Also applies to: 794-797
2 participants
Description
Testing
Summary by CodeRabbit
New Features
UX