feat: gateway logs - dashboard - merge after #4001#3993
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughConsolidates per-feature log detail drawers into a shared client-side, animated Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant Page as Gateway Logs Page
participant Ctrl as Controls
participant Filters as URL Filters (useGatewayLogsFilters)
participant Chart as GatewayLogsChart / useGatewayLogsTimeseries
participant TableHook as useGatewayLogsQuery
participant TRPC as TRPC routers
participant CH as ClickHouse
U->>Page: Open /projects/[id]/gateway-logs
Page->>Ctrl: render controls
Ctrl->>Filters: read/update URL filters
Page->>Chart: mount (onMount -> distanceToTop)
Chart->>TRPC: queryTimeseries(filters + excludeHosts)
TRPC->>CH: build timeseries SQL (apply excludeHosts)
CH-->>TRPC: timeseries data
TRPC-->>Chart: timeseries
Page->>TableHook: mount with filters + live flag
TableHook->>TRPC: queryLogs (historical) / poll (realtime)
TRPC->>CH: build logs SQL (apply excludeHosts)
CH-->>TRPC: logs
TRPC-->>TableHook: logs
TableHook-->>Page: render table (historical + realtime)
U->>Page: select log row
Page->>Page: open LogDetails (shared component, animated)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Before you guys pull out your torches and pitchfork I have to admit this PR is a little bigger than I expected, but since this is mostly client side(Gateway logs are mostly carbon copy of regular logs) this should be super easy to review |
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/table/components/log-details/index.tsx (1)
23-45: Only toast on real errors; remove premature “Log Data Unavailable” branch
The component fires “Log Data Unavailable” as soon aslogis undefined (including initial loading). Drop theelse if (!log)block so only actualerrortriggers a toast. To preserve “Unavailable” messaging later, extenduseFetchRequestDetailsto return anisLoading/isFetchedflag and gate that toast on a completed fetch.apps/dashboard/components/logs/details/log-details/components/log-footer.tsx (1)
86-104: Null-safe permissions to prevent runtime crash
content: extractResponseField(log, "permissions")may be null, butdescriptionmaps over it. Use a safe default.- content: extractResponseField(log, "permissions"), + content: extractResponseField(log, "permissions") ?? [],Also applies to: 101-101
🧹 Nitpick comments (46)
apps/dashboard/components/logs/details/log-details/components/log-meta.tsx (3)
5-5: Remove conflicting flex utilities.
justify-betweenwithflex-colcreates unexpected vertical spacing. Prefer a simple column layout.- <div className="flex justify-between pt-2.5 flex-col gap-1"> + <div className="flex flex-col gap-1 pt-2.5">
9-9: Prevent horizontal overflow and drop the trailing space.Long meta blobs can overflow; also there’s an extra space after the content.
- <pre className="text-accent-12">{content ?? "<EMPTY>"} </pre> + <pre className="text-accent-12 overflow-x-auto whitespace-pre-wrap break-words"> + {content ?? "<EMPTY>"} + </pre>
3-3: Align prop typing with nullish handling.Prop is typed as
stringbut nullish-coalescing is used. Either relax the type or remove??.-export const LogMetaSection = ({ content }: { content: string }) => { +export const LogMetaSection = ({ content }: { content: string | null | undefined }) => {- value={content} + value={content ?? ""}Also applies to: 11-11
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/types.ts (1)
1-1: Derive status type from the single source of truth.Avoid duplicating literals. Derive from
STATUSESto keep types and constants in sync.-export type ResponseStatus = 200 | 400 | 500; +import { STATUSES } from "./constants"; +export type ResponseStatus = (typeof STATUSES)[number];apps/dashboard/app/(app)/logs/page.tsx (1)
7-14: Optional: add skeleton/suspense for better UX on client fetch.Since this now renders entirely on the client, consider a lightweight loading state around .
apps/dashboard/app/(app)/projects/[projectId]/navigations/project-sub-navigation.tsx (1)
48-49: Avoid drift between validTabs and tabs[].Compute
validTabsfromtabsor share a typed union to prevent mismatches.Example:
- const validTabs = ["overview", "deployments", "gateway-logs", "settings"]; + type TabId = "overview" | "deployments" | "gateway-logs" | "settings"; + const validTabs: TabId[] = ["overview", "deployments", "gateway-logs", "settings"];Or derive:
const validTabs = tabs.map(t => t.id);apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-refresh.tsx (1)
11-15: Await cache invalidations to sync UI state.Return a promise from onRefresh so the button can reflect in-flight state; invalidate in parallel.
- const handleRefresh = () => { - refreshQueryTime(); - logs.queryLogs.invalidate(); - logs.queryTimeseries.invalidate(); - }; + const handleRefresh = async () => { + refreshQueryTime(); + await Promise.all([logs.queryLogs.invalidate(), logs.queryTimeseries.invalidate()]); + };- <RefreshButton onRefresh={handleRefresh} isEnabled isLive={isLive} toggleLive={toggleLive} /> + <RefreshButton onRefresh={handleRefresh} isEnabled isLive={isLive} toggleLive={toggleLive} />Also applies to: 18-18
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/constants.ts (2)
1-5: Tighten literal types and export helpers.Mark state arrays as const for literal inference; consider exporting derived types for reuse.
-export const YELLOW_STATES = ["RATE_LIMITED", "EXPIRED", "USAGE_EXCEEDED"]; -export const RED_STATES = ["DISABLED", "FORBIDDEN", "INSUFFICIENT_PERMISSIONS"]; +export const YELLOW_STATES = ["RATE_LIMITED", "EXPIRED", "USAGE_EXCEEDED"] as const; +export const RED_STATES = ["DISABLED", "FORBIDDEN", "INSUFFICIENT_PERMISSIONS"] as const; export const METHODS = ["GET", "POST", "PUT", "DELETE", "PATCH"] as const; export const STATUSES = [200, 400, 500] as const;Optionally:
export type GatewayState = (typeof YELLOW_STATES)[number] | (typeof RED_STATES)[number]; export type HttpMethod = (typeof METHODS)[number]; export type ResponseStatus = (typeof STATUSES)[number];
5-5: Confirm the status set is complete.Are 201/204/404/429/502/503 needed for charts/filters? If yes, extend STATUSES now to avoid follow-up migrations.
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-search/index.tsx (2)
26-29: Polish error message formatting.Avoid surrounding quotes around message; keep copy succinct.
- const errorMessage = `Unable to process your search request${ - error.message ? `' ${error.message} '` : "." - } Please try again or refine your search criteria.`; + const errorMessage = `Unable to process your search request${ + error.message ? `: ${error.message}` : "." + } Please try again or refine your search criteria.`;
51-55: Avoid unhandled promise rejections in event handlers.Use
voidwhen callingmutateAsyncwithout await; onError already handles failures.- onSearch={(query) => - queryLLMForStructuredOutput.mutateAsync({ - query, - timestamp: Date.now(), - }) - } + onSearch={(query) => + void queryLLMForStructuredOutput.mutateAsync({ + query, + timestamp: Date.now(), + }) + }apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/query-gateway-logs.schema.ts (2)
49-57: Constrain status to allowed values.If only 200/400/500 are supported, validate them explicitly to catch typos.
- status: z - .object({ - filters: z.array( - z.object({ - operator: z.literal("is"), - value: z.number(), - }), - ), - }) + status: z + .object({ + filters: z.array( + z.object({ + operator: z.literal("is"), + value: z.union([z.literal(200), z.literal(400), z.literal(500)]), + }), + ), + })
9-58: Make filter groups optional (not just nullable).Current schema requires keys present with null; allow omission to reduce payload noise.
- path: z.object({ ... }).nullable(), + path: z.object({ ... }).nullable().optional(), - host: z.object({ ... }).nullable(), + host: z.object({ ... }).nullable().optional(), - method: z.object({ ... }).nullable(), + method: z.object({ ... }).nullable().optional(), - requestId: z.object({ ... }).nullable(), + requestId: z.object({ ... }).nullable().optional(), - status: z.object({ ... }).nullable(), + status: z.object({ ... }).nullable().optional(),apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/utils.ts (2)
38-59: Header parsing is brittle; split once and compare names case‑insensitively.Current
startsWithmatcher can fail with extra spaces and requires scanning/allocating lowercased strings. Split once and compare normalized names; also guardlogitself.-export const getRequestHeader = (log: Log | RatelimitLog, headerName: string): string | null => { - if (!headerName.trim()) { +export const getRequestHeader = (log: Log | RatelimitLog, headerName: string): string | null => { + if (!headerName.trim()) { console.error("Invalid header name provided"); return null; } - if (!Array.isArray(log.request_headers)) { + if (!log || !Array.isArray(log.request_headers)) { console.error("request_headers is not an array"); return null; } - const lowerHeaderName = headerName.toLowerCase(); - const header = log.request_headers.find((h) => h.toLowerCase().startsWith(`${lowerHeaderName}:`)); + const target = headerName.toLowerCase(); + const entry = log.request_headers + .map((h) => h.split(":", 2)) + .find(([name]) => name.trim().toLowerCase() === target); - if (!header) { + if (!entry) { console.warn(`Header "${headerName}" not found in request headers`); return null; } - const [, value] = header.split(":", 2); - return value ? value.trim() : null; + const [, value] = entry; + return value ? value.trim() : null; };
4-18: Consider runtime validation ofResponseBody.Static casting trusts input. If feasible, define a zod schema for
ResponseBodyand narrow aftersafeParse. This prevents shape drift from breaking UI silently.apps/dashboard/components/logs/details/log-details/components/log-header.tsx (1)
24-33: Add a style for 3xx responses (redirects).Currently only 2xx/4xx/5xx are styled. Add a neutral/info style for 3xx for clearer status semantics.
<Badge className={cn("px-[6px] rounded-md font-mono text-xs", { "bg-success-3 text-success-11 hover:bg-success-4": log.response_status >= 200 && log.response_status < 300, + "bg-accent-3 text-accent-11 hover:bg-accent-4": + log.response_status >= 300 && log.response_status < 400, "bg-warning-3 text-warning-11 hover:bg-warning-4": log.response_status >= 400 && log.response_status < 500, "bg-error-3 text-error-11 hover:bg-error-4": log.response_status >= 500, })} >apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/page.tsx (2)
2-2: Use a consistentcnhelper import.Elsewhere we import
cnfrom@/lib/utils. Importing from@unkey/ui/src/lib/utilscouples us to package internals and can bloat bundles.-import { cn } from "@unkey/ui/src/lib/utils"; +import { cn } from "@/lib/utils";
21-25: Magic widths: consider centralizing constants.
616pxand256pxare repeated across pages; extracting to layout constants improves consistency and future theming.apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-filters/components/gateway-logs-paths-filter.tsx (1)
16-35: Keep FilterOperatorInput in sync with the active “paths” filterWhen the active filter changes, FilterOperatorInput’s internal state won’t update. Remount it using a stable key derived from the active filter.
return ( - <FilterOperatorInput + <FilterOperatorInput + key={`paths-${activePathFilter?.operator ?? "is"}-${activePathFilter?.value ?? ""}`} label="Path" options={options} defaultOption={activePathFilter?.operator} defaultText={activePathFilter?.value as string}apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-filters/components/gateway-logs-status-filter.tsx (4)
1-3: Use central color mapping to avoid driftLeverage gatewayLogsFilterFieldConfig.status.getColorClass instead of hardcoding colors.
-import { FilterCheckbox } from "@/components/logs/checkbox/filter-checkbox"; -import { useGatewayLogsFilters } from "../../../../../hooks/use-gateway-logs-filters"; -import type { ResponseStatus } from "../../../../../types"; +import { FilterCheckbox } from "@/components/logs/checkbox/filter-checkbox"; +import { useGatewayLogsFilters } from "../../../../../hooks/use-gateway-logs-filters"; +import { gatewayLogsFilterFieldConfig } from "../../../../../gateway-logs-filters.schema"; +import type { ResponseStatus } from "../../../../../types";
5-12: Drop redundant color from option typeColor can be computed; remove it from the option type.
type StatusOption = { id: number; status: ResponseStatus; display: string; label: string; - color: string; checked: boolean; };
14-39: Build options without hardcoded color fieldsCompute colors where rendered/used.
const options: StatusOption[] = [ { id: 1, status: 200, display: "2xx", label: "Success", - color: "bg-success-9", checked: false, }, { id: 2, status: 400, display: "4xx", label: "Warning", - color: "bg-warning-8", checked: false, }, { id: 3, status: 500, display: "5xx", label: "Error", - color: "bg-error-9", checked: false, }, ];
48-66: Compute color class from config for UI and metadataSingle source of truth for colors and grouping.
renderOptionContent={(checkbox) => ( <> - <div className={`size-2 ${checkbox.color} rounded-[2px]`} /> + <div + className={`size-2 ${gatewayLogsFilterFieldConfig.status.getColorClass(checkbox.status)} rounded-[2px]`} + /> <span className="text-accent-9 text-xs">{checkbox.display}</span> <span className="text-accent-12 text-xs">{checkbox.label}</span> </> )} createFilterValue={(option) => ({ value: option.status, metadata: { - colorClass: - option.status >= 500 - ? "bg-error-9" - : option.status >= 400 - ? "bg-warning-8" - : "bg-success-9", + colorClass: gatewayLogsFilterFieldConfig.status.getColorClass(option.status), }, })}apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-filters/components/gateway-logs-methods-filter.tsx (1)
10-16: Avoid duplicating HTTP method lists; derive from configUse gatewayLogsFilterFieldConfig.methods.validValues to stay in sync and drop the readonly tuple cast that can clash with mutable array typing.
-const options: MethodOption[] = [ - { id: 1, method: "GET", checked: false }, - { id: 2, method: "POST", checked: false }, - { id: 3, method: "PUT", checked: false }, - { id: 4, method: "DELETE", checked: false }, - { id: 5, method: "PATCH", checked: false }, -] as const; +import { gatewayLogsFilterFieldConfig } from "../../../../../gateway-logs-filters.schema"; + +const options: MethodOption[] = gatewayLogsFilterFieldConfig.methods.validValues.map( + (m, i) => ({ id: i + 1, method: m, checked: false }), +);apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/charts/hooks/use-gateway-logs-timeseries.ts (1)
107-109: Polling is effectively disabled by defaultrefetchInterval is keyed off endTime presence, but endTime is always set, so it never polls. Base it on explicit time filters (startTime/endTime) or since.
- const { data, isLoading, isError } = trpc.logs.queryTimeseries.useQuery(queryParams, { - refetchInterval: queryParams.endTime ? false : 10_000, - }); + const hasExplicitRange = filters.some((f) => f.field === "startTime" || f.field === "endTime"); + const hasSince = filters.some((f) => f.field === "since"); + const shouldPoll = hasSince || !hasExplicitRange; + + const { data, isLoading, isError } = trpc.logs.queryTimeseries.useQuery(queryParams, { + refetchInterval: shouldPoll ? 10_000 : false, + });apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/context/gateway-logs-provider.tsx (1)
15-36: Stabilize callbacks to reduce re-renders in consumersWrap toggleLive and the setSelectedLog wrapper in useCallback; export the memoized setter to the context.
-import { type PropsWithChildren, createContext, useContext, useState } from "react"; +import { type PropsWithChildren, createContext, useContext, useState, useCallback } from "react"; @@ - const toggleLive = (value?: boolean) => { - setIsLive((prev) => (typeof value !== "undefined" ? value : !prev)); - }; + const toggleLive = useCallback((value?: boolean) => { + setIsLive((prev) => (typeof value !== "undefined" ? value : !prev)); + }, []); @@ - setSelectedLog: (log) => { - if (log) { - setIsDetailsOpen(false); - } - setSelectedLog(log); - }, + setSelectedLog: useCallback( + (log: Log | null) => { + if (log) { + setIsDetailsOpen(false); + } + setSelectedLog(log); + }, + [setIsDetailsOpen], + ),apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx (1)
6-6: Consider centralizing the animation delay.ANIMATION_DELAY is duplicated across modules; export a single constant (e.g., from "@/components/logs/details/constants") to keep animations consistent.
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/control-cloud/index.tsx (2)
25-43: Guard status-family formatting by field to avoid mislabeling other numeric strings.As written, any numeric string (even from future filters) would be rendered as “Nxx”. Restrict to field === "status".
Apply this diff:
-const formatValue = (value: string | number, field: string): string => { - if (typeof value === "string" && /^\d+$/.test(value)) { +const formatValue = (value: string | number, field: string): string => { + if (field === "status" && typeof value === "string" && /^\d+$/.test(value)) { const statusFamily = Math.floor(Number.parseInt(value) / 100); switch (statusFamily) {
6-23: Optional: derive labels from field config for consistency.formatFieldName hardcodes labels; consider reusing gatewayLogsFilterFieldConfig (if accessible) to avoid drift and unify i18n.
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-datetime/index.tsx (2)
70-87: Open-state styling likely not applied due to wrapper hierarchy.DatetimePopover’s Trigger wraps children with its own div, so the Button’s group-data-[state=open] class won’t see the trigger’s data-state. Either pass Button as the direct Trigger element (adjust DatetimePopover) or switch to data-[state=open] on the actual trigger.
Apply this local simplification (still won’t fix upstream hierarchy, but reduces extra wrapper and aligns with FiltersPopover pattern):
- > - <div className="group"> - <Button + > + <Button variant="ghost" size="md" className={cn( - "group-data-[state=open]:bg-gray-4 px-2 rounded-lg", + "px-2 rounded-lg", title ? "" : "opacity-50", title !== "Last 12 hours" ? "bg-gray-4" : "", )} aria-label="Filter logs by time" aria-haspopup="true" title="Press 'T' to toggle filters" disabled={!title} > <Calendar className="text-gray-9 size-4" /> <span className="text-gray-12 font-medium text-[13px]">{title ?? "Loading..."}</span> </Button> - </div>If you want me to update DatetimePopover to forward the Trigger directly to its child (so Button receives data-[state]), I can draft that patch.
Also applies to: 74-78
12-17: Initialize title once with useState default to avoid a mount-time disabled flicker.Set the default in useState instead of useEffect.
Apply this diff:
- const [title, setTitle] = useState<string | null>(null); + const [title, setTitle] = useState<string | null>("Last 12 hours"); - useEffect(() => { - if (!title) { - setTitle("Last 12 hours"); - } - }, [title]); + // no effect neededapps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-filters/index.tsx (1)
38-59: Make Button the FiltersPopover trigger (fix a11y & open-state).Button must be the direct child of FiltersPopover so aria props and data-[state=open] styling apply — replace "group-data-[state=open]" with "data-[state=open]" on the Button.
Apply this diff:
- <FiltersPopover items={FILTER_ITEMS} activeFilters={filters}> - <div className="group"> - <Button + <FiltersPopover items={FILTER_ITEMS} activeFilters={filters}> + <Button variant="ghost" size="md" className={cn( - "group-data-[state=open]:bg-gray-4 px-2 rounded-lg", + "data-[state=open]:bg-gray-4 px-2 rounded-lg", filters.length > 0 ? "bg-gray-4" : "", )} aria-label="Filter logs" aria-haspopup="true" title="Press 'F' to toggle filters" > <BarsFilter className="text-accent-9 size-4" /> <span className="text-accent-12 font-medium text-[13px]">Filter</span> {filters.length > 0 && ( <div className="bg-gray-7 rounded h-4 px-1 text-[11px] font-medium text-accent-12 text-center flex items-center justify-center"> {filters.length} </div> )} </Button> - </div> </FiltersPopover>Deep-import scan (ran the suggested script) found 46 files importing "@unkey/ui/src/lib/utils" — update those to the public entry (e.g. import { cn } from "@unkey/ui"):
apps/dashboard/components/list-search-input.tsx
apps/dashboard/components/logs/checkbox/filter-item.tsx
apps/dashboard/components/logs/filter-operator-input/index.tsx
apps/dashboard/components/logs/overview-charts/overview-area-chart-error.tsx
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx
apps/dashboard/components/logs/overview-charts/overview-area-chart-loader.tsx
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/utils.ts
apps/dashboard/components/logs/live-switch-button/index.tsx
apps/dashboard/components/logs/table-action.popover.tsx
apps/dashboard/components/navbar-popover.tsx
apps/dashboard/components/navigation/navbar.tsx
apps/dashboard/components/confirmation-popover.tsx
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx
apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/index.tsx
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/ratelimit-setup.tsx
apps/dashboard/app/(app)/audit/components/table/log-details/components/log-header.tsx
apps/dashboard/app/(app)/audit/components/table/logs-table.tsx
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/controls/components/logs-filters/index.tsx
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/index.tsx
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/controls/components/logs-filters/index.tsx
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/skeletons.tsx
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/filter-button.tsx
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-filters/index.tsx
apps/dashboard/app/(app)/authorization/roles/components/controls/components/logs-filters/index.tsx
apps/dashboard/app/(app)/authorization/permissions/components/controls/components/logs-filters/index.tsx
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx
apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/index.tsx
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx
apps/dashboard/app/(app)/logs/components/controls/components/logs-display/index.tsx
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/controls/components/logs-filters/index.tsx
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-filters/index.tsx
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/deployment-status-badge.tsx
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/skeletons.tsx
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-filters/index.tsx
apps/dashboard/app/(app)/projects/[projectId]/deployments/page.tsx
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/index.tsx
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/skeleton.tsx
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/page.tsx
apps/dashboard/app/(app)/projects/[projectId]/page.tsx
apps/engineering/app/components/render.tsx
apps/dashboard/app/(app)/projects/[projectId]/details/card.tsxapps/dashboard/components/logs/details/log-details/components/log-footer.tsx (1)
12-13: Consider centralizing status-state constants
YELLOW_STATESandRED_STATESare redefined here. To avoid drift with other log views, consider a shared constants module and re-export locally.apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/gateway-logs-table.tsx (3)
177-188: Avoid double-applying selected styles
getRowClassNamealready addsstyle.selectedwhen selected, andselectedClassNamereturns it too. This duplicates classes.- const isSelected = selectedLog?.request_id === log.request_id; - return cn( style.base, style.hover, "group rounded-md", "focus:outline-none focus:ring-1 focus:ring-opacity-40", style.focusRing, - isSelected && style.selected,Rely on
selectedClassNamefor the selected styling.Also applies to: 200-213
217-223: Displayed count should include realtime rowsFooter shows only
historicalLogs.length, undercounting when realtime items exist.-import { VirtualTable } from "@/components/virtual-table/index"; +import { VirtualTable } from "@/components/virtual-table/index"; +import { useMemo } from "react"; @@ const { realtimeLogs, historicalLogs, isLoading, isLoadingMore, loadMore, hasMore, total } = useGatewayLogsQuery({ startPolling: isLive, pollIntervalMs: 2000, }); + + const displayedCount = useMemo(() => { + const ids = new Set<string>(); + for (const l of historicalLogs) ids.add(l.request_id); + for (const l of realtimeLogs) ids.add(l.request_id); + return ids.size; + }, [historicalLogs, realtimeLogs]); @@ - <span>Showing</span> <span className="text-accent-12">{historicalLogs.length}</span> + <span>Showing</span> <span className="text-accent-12">{displayedCount}</span>Also applies to: 169-176
124-126: Reduce row-level JSON parse/log noise for code extraction
extractResponseField(log, "code")is invoked in cell render and logs errors whenresponse_bodyis missing, which can spam devtools. Provide a silent variant or pre-parse once per row.- {extractResponseField(log, "code") ? `| ${extractResponseField(log, "code")}` : ""} + {(() => { + const code = extractResponseField(log, "code"); // consider a silent util + return code ? `| ${code}` : ""; + })()}Optionally introduce
extractResponseField(log, "code", { silent: true }).apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/hooks/use-gateway-logs-filters.ts (1)
55-64: Remove ts-expect-error by typing value for colorClassSafely coerce status to number and guard NaN; use string for others.
- metadata: gatewayLogsFilterFieldConfig[field].getColorClass - ? { - colorClass: gatewayLogsFilterFieldConfig[field].getColorClass( - //TODO: Handle this later - //@ts-expect-error will fix it - field === "status" ? Number(item.value) : item.value, - ), - } - : undefined, + metadata: gatewayLogsFilterFieldConfig[field].getColorClass + ? (() => { + const getColor = gatewayLogsFilterFieldConfig[field].getColorClass!; + if (field === "status") { + const n = Number(item.value); + return Number.isNaN(n) ? undefined : { colorClass: getColor(n as never) }; + } + return { colorClass: getColor(item.value as never) }; + })() + : undefined,apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (3)
24-37: Toast + early return is fine; ensure single-fire behavior across consecutive missing-details selectionsState resets only when
logbecomes null. If users select multiple logs missingkey_detailsconsecutively, only the first emits a toast. If you want one toast per selection, reseterrorShownwhenlog?.request_idchanges.- }, [log, errorShown]); + }, [log?.request_id, errorShown]);Also applies to: 43-49
12-13: Use named constants for animation timings
ANIMATION_DELAYexists butstartDelay/staggerDelayuse literals. Prefer named constants for consistency and easier tuning.-const ANIMATION_DELAY = 350; +const ANIMATION_DELAY = 350; +const SECTIONS_START_DELAY = 150; +const SECTIONS_STAGGER_DELAY = 50; @@ - <LogDetails.CustomSections startDelay={150} staggerDelay={50}> + <LogDetails.CustomSections startDelay={SECTIONS_START_DELAY} staggerDelay={SECTIONS_STAGGER_DELAY}>Also applies to: 103-113
70-81: Minor: Link semantics—avoid block-level div inside LinkWrap plain text in Link or use a span for consistency; unnecessary div can affect inline layout.
- > - <div className="font-mono font-medium truncate">{log.key_id}</div> - </Link> + > + <span className="font-mono font-medium truncate">{log.key_id}</span> + </Link>apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/hooks/use-gateway-logs-query.ts (3)
246-248: AvoidtoSortedto prevent polyfill/runtime support issues.Use a stable copy+sort to work across older browsers/Node without Transform/Ponyfill.
-const sortLogs = (logs: Log[]) => { - return logs.toSorted((a, b) => b.time - a.time); -}; +const sortLogs = (logs: Log[]) => [...logs].sort((a, b) => b.time - a.time);
203-208: Kick off the first poll immediately when enabling live mode.Prevents waiting one interval before seeing new logs.
useEffect(() => { if (startPolling) { - const interval = setInterval(pollForNewLogs, pollIntervalMs); + void pollForNewLogs(); + const interval = setInterval(pollForNewLogs, pollIntervalMs); return () => clearInterval(interval); } }, [startPolling, pollForNewLogs, pollIntervalMs]);
177-184: Comment/code mismatch on realtime size cap.Comment says “size limit 100” but code caps at
min(limit, REALTIME_DATA_LIMIT). Clarify comment or adjust logic.- // Remove oldest entries when exceeding the size limit `100` + // Remove oldest entries when exceeding the size cap (min(limit, REALTIME_DATA_LIMIT))apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/gateway-logs-filters.schema.ts (1)
35-55: Status validation range—intentional exclusion of 1xx/3xx?Current validation only allows 200–599. If 1xx or 3xx should be filterable, widen the range; otherwise, add a brief comment to document the intent.
- validate: (value) => value >= 200 && value <= 599, + // Accept only 2xx,4xx,5xx statuses for gateway logs + validate: (value) => value >= 200 && value <= 599,apps/dashboard/components/logs/details/log-details/index.tsx (2)
47-52: Avoid double JSON parsing of bodies.Parse once to reduce work and noisy error logs.
- content: - JSON.stringify(safeParseJson(log.request_body), null, 2) === "null" - ? EMPTY_TEXT - : JSON.stringify(safeParseJson(log.request_body), null, 2), + content: (() => { + const parsed = safeParseJson(log.request_body); + const s = JSON.stringify(parsed, null, 2); + return s === "null" ? EMPTY_TEXT : s; + })(), @@ - content: - JSON.stringify(safeParseJson(log.response_body), null, 2) === "null" - ? EMPTY_TEXT - : JSON.stringify(safeParseJson(log.response_body), null, 2), + content: (() => { + const parsed = safeParseJson(log.response_body); + const s = JSON.stringify(parsed, null, 2); + return s === "null" ? EMPTY_TEXT : s; + })(),Also applies to: 60-63
2-2: Consider moving utils to a shared path.Importing from
@/app/(app)/logs/utilscouples this generic component to the regular logs route. A shared@/components/logs/utilswould avoid cross-app coupling.
...ard/app/(app)/projects/[projectId]/gateway-logs/components/charts/query-timeseries.schema.ts
Outdated
Show resolved
Hide resolved
...jects/[projectId]/gateway-logs/components/controls/components/gateway-logs-filters/index.tsx
Show resolved
Hide resolved
...rojects/[projectId]/gateway-logs/components/controls/components/gateway-logs-live-switch.tsx
Show resolved
Hide resolved
...app/(app)/projects/[projectId]/gateway-logs/components/table/hooks/use-gateway-logs-query.ts
Show resolved
Hide resolved
...rd/app/(app)/projects/[projectId]/gateway-logs/components/table/query-gateway-logs.schema.ts
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/hooks/use-gateway-logs-filters.ts
Show resolved
Hide resolved
apps/dashboard/components/logs/details/log-details/components/log-footer.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (50)
apps/dashboard/app/(app)/projects/[projectId]/navigations/project-sub-navigation.tsx (2)
52-53: Guard against missing projectId before using it.getCurrentTab uses projectId earlier; throw first, then compute activeTab.
- const activeTab = getCurrentTab(); + if (!projectId) { + throw new Error("ProjectSubNavigation requires a valid project ID"); + } + const activeTab = getCurrentTab(); @@ - if (!projectId) { - throw new Error("ProjectSubNavigation requires a valid project ID"); - } + // (moved guard above)Also applies to: 79-81
45-50: Remove non-existent "settings" tab; optional legacy /logs normalization
- apps/dashboard/app/(app)/projects/[projectId]/gateway-logs exists; no internal "/logs" references found — normalizing "logs"→"gateway-logs" is optional (defensive).
- validTabs includes "settings" but there's no Settings route under projects — remove it to avoid an un-highlighted state.
Location: apps/dashboard/app/(app)/projects/[projectId]/navigations/project-sub-navigation.tsx (around lines 45–50)
- const validTabs = ["overview", "deployments", "gateway-logs", "settings"] as const; + const validTabs = ["overview", "deployments", "gateway-logs"] as const;apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/types.ts (1)
1-1: Derive the status type from a single source of truth (constants) to avoid drift.Avoid duplicating literal unions. Reuse STATUSES so additions (e.g., 404, 429) don’t require hunting multiple files.
Apply this diff:
-export type ResponseStatus = 200 | 400 | 500; +import { STATUSES } from "./constants"; +export type ResponseStatus = (typeof STATUSES)[number];apps/dashboard/components/logs/details/log-details/components/log-meta.tsx (1)
9-9: Remove trailing space inside.Tiny rendering nit; prevents accidental copy of an extra whitespace.
- <pre className="text-accent-12">{content ?? "<EMPTY>"} </pre> + <pre className="text-accent-12">{content ?? "<EMPTY>"}</pre>apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-refresh.tsx (1)
11-15: Await invalidations so the RefreshButton can reflect completion correctly.Make the handler async and await cache invalidations; many buttons rely on a returned Promise to manage loading states.
- const handleRefresh = () => { - refreshQueryTime(); - logs.queryLogs.invalidate(); - logs.queryTimeseries.invalidate(); - }; + const handleRefresh = async () => { + refreshQueryTime(); + await Promise.all([ + logs.queryLogs.invalidate(), + logs.queryTimeseries.invalidate(), + ]); + };apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-filters/components/gateway-logs-methods-filter.tsx (2)
1-3: Import METHODS from the shared constants.Prevents divergence between UI options and allowed values.
-import { FilterCheckbox } from "@/components/logs/checkbox/filter-checkbox"; +import { FilterCheckbox } from "@/components/logs/checkbox/filter-checkbox"; +import { METHODS } from "../../../../../constants";
10-16: Generate options from METHODS to keep a single source of truth.-const options: MethodOption[] = [ - { id: 1, method: "GET", checked: false }, - { id: 2, method: "POST", checked: false }, - { id: 3, method: "PUT", checked: false }, - { id: 4, method: "DELETE", checked: false }, - { id: 5, method: "PATCH", checked: false }, -] as const; +const options: MethodOption[] = METHODS.map((m, idx) => ({ + id: idx + 1, + method: m, + checked: false, +}));apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/constants.ts (1)
1-5: Export typed unions from these consts for reuse across the module.This lets downstream code import types without re-declaring unions elsewhere.
export const YELLOW_STATES = ["RATE_LIMITED", "EXPIRED", "USAGE_EXCEEDED"]; export const RED_STATES = ["DISABLED", "FORBIDDEN", "INSUFFICIENT_PERMISSIONS"]; export const METHODS = ["GET", "POST", "PUT", "DELETE", "PATCH"] as const; export const STATUSES = [200, 400, 500] as const; + +export type Method = (typeof METHODS)[number]; +export type ResponseStatus = (typeof STATUSES)[number];apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-search/index.tsx (2)
8-23: Harden success guard for optional shapes.Safer check reads clearer and handles undefined filters.
- onSuccess(data) { - if (data?.filters.length === 0 || !data) { + onSuccess(data) { + if (!data || (Array.isArray(data.filters) && data.filters.length === 0)) { toast.error(
50-55: Return a Promise from onSearch so the control can await it.Keeps loading states precise and prevents double-submits.
- onSearch={(query) => - queryLLMForStructuredOutput.mutateAsync({ - query, - timestamp: Date.now(), - }) - } + onSearch={async (query) => { + await queryLLMForStructuredOutput.mutateAsync({ + query, + timestamp: Date.now(), + }); + }}apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-live-switch.tsx (1)
10-31: Consider clearing time bounds when switching back to live.You add start/endTime when leaving live. On returning to live, stale time filters may continue constraining queries unless upstream ignores them. If that’s not guaranteed, clear them.
const handleSwitch = () => { toggleLive(); // To able to refetch historic data again we have to update the endTime if (isLive) { const timestamp = Date.now(); const activeFilters = filters.filter((f) => !["endTime", "startTime"].includes(f.field)); updateFilters([ ...activeFilters, { field: "endTime", value: timestamp, id: crypto.randomUUID(), operator: "is", }, { field: "startTime", value: timestamp - HISTORICAL_DATA_WINDOW, id: crypto.randomUUID(), operator: "is", }, ]); + } else { + // Optionally drop explicit time bounds to resume true live mode + const activeFilters = filters.filter((f) => !["endTime", "startTime"].includes(f.field)); + updateFilters(activeFilters); } };apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/gateway-logs-filters.schema.ts (3)
49-49: Validate HTTP status codes more preciselyThe status validation allows values from 200-599, but HTTP status codes are defined as 100-599 (with 100-199 being informational). Consider updating the validation range for completeness.
- validate: (value) => value >= 200 && value <= 599, + validate: (value) => value >= 100 && value <= 599,
40-48: Consider extracting status color mapping to a constantThe
getColorClassfunction contains hardcoded status ranges and color classes. For better maintainability and reusability across the codebase (e.g., in LogHeader component), consider extracting these mappings to a shared constant.+const STATUS_COLOR_RANGES = { + ERROR: { min: 500, className: "bg-error-9" }, + WARNING: { min: 400, className: "bg-warning-8" }, + SUCCESS: { min: 200, className: "bg-success-9" }, +} as const; export const gatewayLogsFilterFieldConfig: FilterFieldConfigs = { status: { type: "number", operators: ["is"], getColorClass: (value) => { - if (value >= 500) { - return "bg-error-9"; - } - if (value >= 400) { - return "bg-warning-8"; - } - return "bg-success-9"; + if (value >= STATUS_COLOR_RANGES.ERROR.min) { + return STATUS_COLOR_RANGES.ERROR.className; + } + if (value >= STATUS_COLOR_RANGES.WARNING.min) { + return STATUS_COLOR_RANGES.WARNING.className; + } + return STATUS_COLOR_RANGES.SUCCESS.className; },
68-79: Consider using date types for time fieldsThe
startTime,endTimefields are configured as numbers, but they appear to represent timestamps. Consider adding validation to ensure these are valid Unix timestamps.startTime: { type: "number", operators: ["is"], + validate: (value) => value > 0 && value < Date.now() + 86400000, // Allow future dates up to 1 day }, endTime: { type: "number", operators: ["is"], + validate: (value) => value > 0 && value < Date.now() + 86400000, },apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/query-gateway-logs.schema.ts (3)
59-59: Simplify redundant nullable chainingThe
cursorfield has redundant nullable specifications. The.optional()already makes the field optional, so the second.nullable()is unnecessary.- cursor: z.number().nullable().optional().nullable(), + cursor: z.number().nullable().optional(),
5-8: Add validation constraints for time and limit fieldsThe schema should validate that time fields are positive integers and that the limit is within reasonable bounds to prevent potential performance issues.
- limit: z.number().int(), - startTime: z.number().int(), - endTime: z.number().int(), + limit: z.number().int().min(1).max(1000), + startTime: z.number().int().positive(), + endTime: z.number().int().positive(),
5-8: Add validation for logical time rangeConsider adding a refinement to ensure
endTimeis greater than or equal tostartTimeto prevent invalid time ranges.-export const queryLogsPayload = z.object({ +export const queryLogsPayload = z.object({ limit: z.number().int(), startTime: z.number().int(), endTime: z.number().int(), since: z.string(), // ... rest of the fields -}); +}).refine((data) => data.endTime >= data.startTime, { + message: "endTime must be greater than or equal to startTime", + path: ["endTime"], +});apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/page.tsx (1)
23-25: Extract magic numbers to constantsThe hardcoded pixel values for width calculations should be extracted to named constants for better maintainability.
+const LAYOUT_WIDTHS = { + DETAILS_PANEL: 616, + SIDEBAR: 256, +} as const; export default function Page() { const { isDetailsOpen } = useProjectLayout(); const [tableDistanceToTop, setTableDistanceToTop] = useState(0); // ... rest of the code return ( <div className={cn( "flex flex-col transition-all duration-300 ease-in-out", - isDetailsOpen ? "w-[calc(100vw-616px)]" : "w-[calc(100vw-256px)]", + isDetailsOpen + ? `w-[calc(100vw-${LAYOUT_WIDTHS.DETAILS_PANEL}px)]` + : `w-[calc(100vw-${LAYOUT_WIDTHS.SIDEBAR}px)]`, )} >apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-filters/index.tsx (2)
4-4: Avoid deep import for cn; use the shared util path for consistency.Standardize on "@/lib/utils" (used elsewhere in this PR) instead of "@unkey/ui/src/lib/utils".
-import { cn } from "@unkey/ui/src/lib/utils"; +import { cn } from "@/lib/utils";
34-56: Filter badge counts time filters; scope count to visible filter fields only.The badge and button highlight use filters.length, which includes time filters (since/startTime/endTime). Show count/highlight only for status/methods/paths to match the popover contents.
export const GatewayLogsFilters = () => { const { filters } = useGatewayLogsFilters(); + const visibleFiltersCount = filters.filter((f) => + ["status", "methods", "paths"].includes(f.field), + ).length; return ( <FiltersPopover items={FILTER_ITEMS} activeFilters={filters}> <div className="group"> <Button variant="ghost" size="md" className={cn( "group-data-[state=open]:bg-gray-4 px-2 rounded-lg", - filters.length > 0 ? "bg-gray-4" : "", + visibleFiltersCount > 0 ? "bg-gray-4" : "", )} aria-label="Filter logs" aria-haspopup="true" title="Press 'F' to toggle filters" > <BarsFilter className="text-accent-9 size-4" /> <span className="text-accent-12 font-medium text-[13px]">Filter</span> - {filters.length > 0 && ( + {visibleFiltersCount > 0 && ( <div className="bg-gray-7 rounded h-4 px-1 text-[11px] font-medium text-accent-12 text-center flex items-center justify-center"> - {filters.length} + {visibleFiltersCount} </div> )} </Button> </div> </FiltersPopover> ); }apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/context/gateway-logs-provider.tsx (2)
2-2: Avoid deep type import; prefer the package’s public surface.Importing from “@unkey/clickhouse/src/logs” couples to internal paths. If available, use the top-level export.
-import type { Log } from "@unkey/clickhouse/src/logs"; +import type { Log } from "@unkey/clickhouse";
24-39: Stabilize context value to reduce unnecessary re-renders.Wrap toggleLive in useCallback and the provider value in useMemo.
-import { type PropsWithChildren, createContext, useContext, useState } from "react"; +import { type PropsWithChildren, createContext, useContext, useMemo, useCallback, useState } from "react"; @@ - const toggleLive = (value?: boolean) => { - setIsLive((prev) => (typeof value !== "undefined" ? value : !prev)); - }; + const toggleLive = useCallback((value?: boolean) => { + setIsLive((prev) => (typeof value !== "undefined" ? value : !prev)); + }, []); @@ - return ( - <GatewayLogsContext.Provider - value={{ - isLive, - toggleLive, - selectedLog, - setSelectedLog: (log) => { - if (log) { - setIsDetailsOpen(false); - } - setSelectedLog(log); - }, - }} - > + const value = useMemo( + () => ({ + isLive, + toggleLive, + selectedLog, + setSelectedLog: (log: Log | null) => { + if (log) { + setIsDetailsOpen(false); + } + setSelectedLog(log); + }, + }), + [isLive, toggleLive, selectedLog, setIsDetailsOpen], + ); + + return ( + <GatewayLogsContext.Provider value={value}> {children} </GatewayLogsContext.Provider> );apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-datetime/index.tsx (3)
9-16: Initialize title directly; avoid effect for default state.Inline the default ("Last 12 hours") and reuse a constant to prevent string drift.
-export const GatewayLogsDateTime = () => { - const [title, setTitle] = useState<string | null>(null); +const DEFAULT_TITLE = "Last 12 hours"; +export const GatewayLogsDateTime = () => { + const [title, setTitle] = useState<string>(DEFAULT_TITLE); @@ - useEffect(() => { - if (!title) { - setTitle("Last 12 hours"); - } - }, [title]); + // title updates via DatetimePopover.onSuggestionChange
18-27: Type timeValues to the expected shape.Prevents accidental type widening and clarifies intent.
- const timeValues = filters - .filter((f) => ["startTime", "endTime", "since"].includes(f.field)) - .reduce( - (acc, f) => ({ - // biome-ignore lint/performance/noAccumulatingSpread: it's safe to spread - ...acc, - [f.field]: f.value, - }), - {}, - ); + const timeValues: { startTime?: number; endTime?: number; since?: string } = filters + .filter((f) => ["startTime", "endTime", "since"].includes(f.field)) + .reduce((acc, f) => { + if (f.field === "startTime" || f.field === "endTime") { + acc[f.field] = Number(f.value); + } else { + acc.since = String(f.value); + } + return acc; + }, {} as { startTime?: number; endTime?: number; since?: string });
71-83: Use the constant for highlight and remove redundant disabled.title is always defined now; rely on DEFAULT_TITLE for styling.
- title ? "" : "opacity-50", - title !== "Last 12 hours" ? "bg-gray-4" : "", + title !== DEFAULT_TITLE ? "bg-gray-4" : "", )} @@ - disabled={!title} + /* title is always defined */apps/dashboard/app/(app)/logs/components/table/log-details/index.tsx (1)
4-30: Shared LogDetails adoption looks clean.Simplifies the drawer composition and keeps the API intact. The Spacer delay is explicit and readable.
If multiple drawers use the same 350ms, consider centralizing ANIMATION_DELAY in the shared component to avoid drift.
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/control-cloud/index.tsx (2)
18-19: Handle the "since" field appropriatelyThe "since" field returns an empty string, which might not display well in the UI. Consider returning a more descriptive label or handling this field differently.
case "since": - return ""; + return "Since";
25-43: Consider extracting HTTP status family logic to a shared utilityThe status family determination logic could be useful in other parts of the codebase. Consider extracting it to a shared utility function for reusability.
Create a shared utility:
// in a shared utils file export const getHttpStatusFamily = (status: number): string => { const statusFamily = Math.floor(status / 100); switch (statusFamily) { case 5: return "5xx (Error)"; case 4: return "4xx (Warning)"; case 2: return "2xx (Success)"; default: return `${statusFamily}xx`; } };Then use it here:
- if (typeof value === "string" && /^\d+$/.test(value)) { - const statusFamily = Math.floor(Number.parseInt(value) / 100); - switch (statusFamily) { - case 5: - return "5xx (Error)"; - case 4: - return "4xx (Warning)"; - case 2: - return "2xx (Success)"; - default: - return `${statusFamily}xx`; - } - } + if (typeof value === "string" && /^\d+$/.test(value)) { + return getHttpStatusFamily(Number.parseInt(value)); + }apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/hooks/use-gateway-logs-query.ts (3)
49-50: Typo in commentMinor typo in the comment.
- // "memo" required for preventing double trpc call during initial render + // memo required for preventing double trpc call during initial render
178-184: Potential performance issue with oldest entry removalThe logic to find and remove the oldest entry creates an array from the Map entries and then reduces over it for every removal operation. This could be inefficient when handling high-frequency log updates.
Consider maintaining an ordered data structure or tracking the oldest entry more efficiently:
- // Remove oldest entries when exceeding the size limit `100` - if (newMap.size > Math.min(limit, REALTIME_DATA_LIMIT)) { - const entries = Array.from(newMap.entries()); - const oldestEntry = entries.reduce((oldest, current) => { - return oldest[1].time < current[1].time ? oldest : current; - }); - newMap.delete(oldestEntry[0]); - } + // Remove oldest entries when exceeding the size limit + const maxSize = Math.min(limit, REALTIME_DATA_LIMIT); + if (newMap.size > maxSize) { + // Convert to array once and sort by time + const sortedEntries = Array.from(newMap.entries()) + .sort((a, b) => b[1].time - a[1].time); + // Keep only the newest entries + newMap.clear(); + sortedEntries.slice(0, maxSize).forEach(([id, log]) => { + newMap.set(id, log); + }); + }
71-72: Consider consolidating error handlingThe error handling for invalid filter values uses
console.errorin multiple places. Consider extracting this to a centralized error handler or using a more robust error reporting mechanism.const handleFilterError = (field: string, expectedType: string, value: unknown) => { const errorMessage = `${field} filter value must be a ${expectedType}, got ${typeof value}`; console.error(errorMessage); // Could also report to error tracking service };Then use it throughout:
- console.error("Status filter value must be a valid number"); + handleFilterError("Status", "valid number", filter.value);Also applies to: 85-86, 100-101, 115-116, 123-124
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/table/components/log-details/index.tsx (1)
55-57: Consider providing user feedback when error occursWhen an error occurs or log is unavailable, the component returns null without any visual feedback to the user. Consider showing a placeholder or error state in the drawer.
if (error || !log) { - return null; + return ( + <LogDetails distanceToTop={distanceToTop} log={selectedLog} onClose={handleClose} animated> + <div className="flex items-center justify-center h-full p-8 text-gray-11"> + <div className="text-center"> + <p className="text-sm font-medium mb-2">Unable to load log details</p> + <p className="text-xs text-gray-9">Please try again later</p> + </div> + </div> + </LogDetails> + ); }apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/charts/index.tsx (1)
72-73: Minor JSX nit.
enableSelection={true}can be shortened toenableSelection.- enableSelection={true} + enableSelectionapps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/charts/hooks/use-gateway-logs-timeseries.ts (1)
35-50: Input validation logging: downgrade severity.These user-controlled filter parse errors are expected edge cases; prefer
console.warnto reduce noise.- console.error("Status filter value must be a valid number"); + console.warn("Status filter value must be a valid number");apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (1)
53-60: Format “Created” consistently with TimestampInfo.If
meta.createdAtis a date/ISO/epoch, render it withTimestampInfofor parity with “Last Used.”- const usage = { - Created: metaData?.createdAt || "N/A", + const usage = { + Created: metaData?.createdAt ? ( + <TimestampInfo + value={new Date(metaData.createdAt)} + className="font-mono underline decoration-dotted" + /> + ) : ( + "N/A" + ),apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/hooks/use-gateway-logs-filters.ts (2)
55-63: Remove ts-expect-error by narrowing to the status case.Only
statususesgetColorClass. Narrowing removes the need for@ts-expect-error.- metadata: gatewayLogsFilterFieldConfig[field].getColorClass - ? { - colorClass: gatewayLogsFilterFieldConfig[field].getColorClass( - //TODO: Handle this later - //@ts-expect-error will fix it - field === "status" ? Number(item.value) : item.value, - ), - } - : undefined, + metadata: + field === "status" && gatewayLogsFilterFieldConfig.status.getColorClass + ? { + colorClass: gatewayLogsFilterFieldConfig.status.getColorClass( + Number(item.value), + ), + } + : undefined,
27-37: Restrict status operators to “is”.
statusshould not acceptcontains/startsWith/endsWithvia URL. Parse it with only “is” to avoid unsupported states.export const queryParamsPayload = { - status: parseAsFilterValArray, + // status supports only `is` + status: parseAsFilterValueArray<GatewayLogsFilterOperator>(["is"]), methods: parseAsFilterValArray, paths: parseAsFilterValArray, host: parseAsFilterValArray, requestId: parseAsFilterValArray, startTime: parseAsInteger, endTime: parseAsInteger, since: parseAsRelativeTime, } as const;apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/utils.ts (3)
29-36: Normalize missing fields to null.If the parsed body lacks
fieldName, you currently returnundefined(notnullas typed).- try { - const parsedBody = JSON.parse(log.response_body) as ResponseBody; - - return parsedBody[fieldName]; - } catch { - return null; - } + try { + const parsedBody = JSON.parse(log.response_body) as Partial<ResponseBody>; + return (parsedBody[fieldName] ?? null) as ResponseBody[K] | null; + } catch { + return null; + }
69-71: Avoid logging full JSON payloads (possible PII).Log a generic error instead of echoing the raw JSON string.
- console.error("Cannot parse JSON:", jsonString); + console.error("Cannot parse JSON");
44-59: Optional: support string-form request headers.If
request_headerssometimes arrives as a newline‑delimited string, add a fallback to parse it into an array before searching.apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/gateway-logs-table.tsx (6)
81-91: Warning icon shows for 3xx statuses (likely unintended).3xx responses currently render a visible gray warning icon. Hide the icon for <400 to match "warn/error only".
const WarningIcon = ({ status }: { status: number }) => ( <TriangleWarning2 size="md-regular" className={cn( WARNING_ICON_STYLES.base, - status < 300 && "invisible", + status < 400 && "invisible", status >= 400 && status < 500 && WARNING_ICON_STYLES.warning, status >= 500 && WARNING_ICON_STYLES.error, )} /> );
115-128: Avoid double JSON parse for status code.
extractResponseFieldparses JSON; it’s called twice per cell. Cache once.render: (log) => { const style = getStatusStyle(log.response_status); + const code = extractResponseField(log, "code"); return ( <Badge className={cn( "uppercase px-[6px] rounded-md font-mono whitespace-nowrap", style.badge.default, )} > - {log.response_status}{" "} - {extractResponseField(log, "code") ? `| ${extractResponseField(log, "code")}` : ""} + {log.response_status} {code ? `| ${code}` : ""} </Badge> ); },
169-176: Make live-mode opacity check O(1).
Array.prototype.someis O(n) and runs for each row. Precompute a Set of realtime IDs.export const GatewayLogsTable = () => { const { setSelectedLog, selectedLog, isLive } = useGatewayLogsContext(); const { realtimeLogs, historicalLogs, isLoading, isLoadingMore, loadMore, hasMore, total } = useGatewayLogsQuery({ startPolling: isLive, pollIntervalMs: 2000, }); + + const realtimeIds = React.useMemo( + () => new Set(realtimeLogs.map((l) => l.request_id)), + [realtimeLogs], + ); @@ isLive && - !realtimeLogs.some((realtime) => realtime.request_id === log.request_id) && [ + !realtimeIds.has(log.request_id) && [ "opacity-50", "hover:opacity-100", ],Also applies to: 188-192
181-188: Remove duplicate selected styling source.Row classes already add
style.selectedwhen selected and you also passselectedClassName. Keep one to avoid brittle CSS overrides. Suggest relying solely onselectedClassName.return cn( style.base, style.hover, "group rounded-md", "focus:outline-none focus:ring-1 focus:ring-opacity-40", style.focusRing, - isSelected && style.selected,Optionally drop the
selectedClassNameprop instead; keep one mechanism.Also applies to: 212-212
146-150: Prevent path overflow.Long paths can stretch the cell. Truncate with ellipsis.
- render: (log) => <div className="font-mono pr-4">{log.path}</div>, + render: (log) => ( + <div className="font-mono pr-4 truncate min-w-0 max-w-full">{log.path}</div> + ),
3-11: Minor: consolidate response parsing utilities.This file uses
extractResponseFieldfrom gateway-logs utils, whereas LogDetails imports the logs/utils variant. Consider a single shared util to avoid drift.apps/dashboard/components/logs/details/log-details/index.tsx (4)
40-64: De-duplicate JSON parsing and improve fallback rendering.
safeParseJsonis called twice per body, and when it returns an error string you end up showing quoted JSON. Parse once and render strings raw.// Helper functions for standard logs -const createLogSections = (log: Log | RatelimitLog) => [ +const parseOrEmpty = (json?: string | null) => { + const parsed = safeParseJson(json); + if (parsed === null) return EMPTY_TEXT; + return typeof parsed === "string" ? parsed : JSON.stringify(parsed, null, 2); +}; + +const createLogSections = (log: Log | RatelimitLog) => [ { title: "Request Header", content: log.request_headers.length ? log.request_headers : EMPTY_TEXT, }, { title: "Request Body", - content: - JSON.stringify(safeParseJson(log.request_body), null, 2) === "null" - ? EMPTY_TEXT - : JSON.stringify(safeParseJson(log.request_body), null, 2), + content: parseOrEmpty(log.request_body), }, { title: "Response Header", content: log.response_headers.length ? log.response_headers : EMPTY_TEXT, }, { title: "Response Body", - content: - JSON.stringify(safeParseJson(log.response_body), null, 2) === "null" - ? EMPTY_TEXT - : JSON.stringify(safeParseJson(log.response_body), null, 2), + content: parseOrEmpty(log.response_body), }, ];
66-84: Meta content: preserve raw strings and add safer fallback.Show raw meta if it’s a string; on parse failure for KeysOverviewLog, fall back to the original meta instead of
<EMPTY>.const createMetaContent = (log: SupportedLogTypes) => { // Handle KeysOverviewLog meta differently if ("key_details" in log && (log.key_details as { meta: string })?.meta) { try { const parsedMeta = JSON.parse((log.key_details as { meta: string })?.meta); return JSON.stringify(parsedMeta, null, 2); } catch { - return EMPTY_TEXT; + return (log.key_details as { meta: string }).meta ?? EMPTY_TEXT; } } // Standard log meta handling if ("request_body" in log || "response_body" in log) { const meta = extractResponseField(log as Log | RatelimitLog, "meta"); - return JSON.stringify(meta, null, 2) === "null" ? EMPTY_TEXT : JSON.stringify(meta, null, 2); + if (meta == null) return EMPTY_TEXT; + return typeof meta === "string" ? meta : JSON.stringify(meta, null, 2); } return EMPTY_TEXT; };
32-39: Make context usage safe outside provider.Current default value masks misuse. Throw if hook is used without a Provider (matches your GatewayLogsContext pattern).
-const LogDetailsContext = createContext<LogDetailsContextValue>({ - animated: false, - isOpen: true, - log: {} as SupportedLogTypes, -}); +const LogDetailsContext = createContext<LogDetailsContextValue | null>(null); -const useLogDetailsContext = () => useContext(LogDetailsContext); +const useLogDetailsContext = () => { + const ctx = useContext(LogDetailsContext); + if (!ctx) { + throw new Error("useLogDetailsContext must be used within a LogDetails.Provider"); + } + return ctx; +};
2-2: Consolidate utils import path.This imports from
/logs/utils, while the table uses/gateway-logs/utils. Prefer a shared utils module to avoid drift.
...pp)/projects/[projectId]/gateway-logs/components/charts/hooks/use-gateway-logs-timeseries.ts
Show resolved
Hide resolved
...ard/app/(app)/projects/[projectId]/gateway-logs/components/charts/query-timeseries.schema.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-live-switch.tsx (1)
9-14: Re-verify: leaving live should clear any relative “since” in filters.Previous feedback noted stale
sincepersisting in URL when turning live off. IftoggleLiveor the provider doesn’t clear it, filters may remain relative. Please confirm provider behavior.#!/bin/bash # 1) Locate toggleLive implementation rg -nP -C3 '\btoggleLive\b' apps/dashboard # 2) Inspect gateway-logs codepaths that set/clear relative time (since) rg -nP -C3 '\bsince\b' apps/dashboard/app/\(app\)/projects/.*/gateway-logs -g '!**/node_modules/**' # 3) Check if updateFilters/setFilters clear 'since' on exit from live rg -nP -C3 '\b(updateFilters|setFilters)\b' apps/dashboard/app/\(app\)/projects/.*/gateway-logsExpected: on toggling live off, any relative
sinceis removed and absolute start/end (or an anchored endTime via refreshQueryTime) are applied.
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-live-switch.tsx (2)
1-1: Memoize handler and use pre-toggle flag to avoid stale captures.Stabilize onToggle and make intent explicit.
Apply:
+import { useCallback } from "react"; import { LiveSwitchButton } from "@/components/logs/live-switch-button"; import { useQueryTime } from "@/providers/query-time-provider"; import { useGatewayLogsContext } from "../../../context/gateway-logs-provider"; export const GatewayLogsLiveSwitch = () => { const { toggleLive, isLive } = useGatewayLogsContext(); const { refreshQueryTime } = useQueryTime(); - const handleSwitch = () => { - toggleLive(); - if (isLive) { - refreshQueryTime(); - } - }; + const handleSwitch = useCallback(() => { + const wasLive = isLive; + toggleLive(); + if (wasLive) { + refreshQueryTime(); + } + }, [isLive, toggleLive, refreshQueryTime]);Also applies to: 9-14
3-3: Optional: use path alias instead of deep relative import.Reduces fragility during moves; align with other
@/…imports if paths are configured.Example:
-import { useGatewayLogsContext } from "../../../context/gateway-logs-provider"; +import { useGatewayLogsContext } from "@/app/(app)/projects/[projectId]/gateway-logs/context/gateway-logs-provider";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-live-switch.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-live-switch.tsx (3)
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/context/gateway-logs-provider.tsx (1)
useGatewayLogsContext(43-49)apps/dashboard/providers/query-time-provider.tsx (2)
useQueryTime(74-80)refreshQueryTime(8-12)apps/dashboard/components/logs/live-switch-button/index.tsx (1)
LiveSwitchButton(11-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (1)
apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/controls/components/gateway-logs-live-switch.tsx (1)
5-16: LGTM — simple, readable integration.Toggle logic and refresh on exit-from-live make sense.
ogzhanolguncu
changed the title
|
This PR needs a slight update. We have to prefilter gateway logs by |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx (2)
34-38: Rename boolean for clarityMinor readability: name reflects both actions.
- const canRollbackAndRollback = + const canDeployActions = liveDeployment && environment?.slug === "production" && selectedDeployment.status === "ready" && selectedDeployment.id !== liveDeployment.id; @@ - disabled: !canRollbackAndRollback, + disabled: !canDeployActions, @@ - liveDeployment && canRollbackAndRollback + liveDeployment && canDeployActions @@ - disabled: !canRollbackAndRollback, + disabled: !canDeployActions, @@ - liveDeployment && canRollbackAndRollback + liveDeployment && canDeployActionsAlso applies to: 45-48, 61-64
88-94: Add projectId to deps to avoid stale route paramPrevents stale push target if only projectId changes.
}, [ selectedDeployment.id, + selectedDeployment.projectId, selectedDeployment.status, liveDeployment?.id, environment?.slug, data, ]);apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/env-status-badge.tsx (5)
45-48: Handle null/undefined variant safely; avoid incorrect cast.If
variantisnull(orundefinedbefore defaulting),tooltipContent[variant]becomesundefinedat runtime. The cast only hides the type error. Fall back to "live" for the lookup.Apply this diff:
- content={tooltipContent[variant as Exclude<typeof variant, null>]} + content={tooltipContent[(variant ?? "live") as keyof typeof tooltipContent]}
24-30: Type tooltipContent against the variant keys to prevent drift.Tie the mapping to
statusBadgeVariantsso adding/removing a variant forces a compile-time update here.Apply this diff:
-const tooltipContent = { +const tooltipContent = { enabled: "This environment is enabled and ready to receive deployments.", disabled: "This environment is disabled and cannot receive deployments.", live: "This environment is currently receiving live traffic.", rolledBack: "This environment was previously live but has been rolled back.", -} as const; +} as const satisfies Record< + Exclude<VariantProps<typeof statusBadgeVariants>["variant"], null | undefined>, + string +>;
38-38: Avoid duplicated defaults.
statusBadgeVariantsalready defaults to "live" and the tooltip now falls back as well. Remove the param default to keep a single source of truth.-export const EnvStatusBadge = ({ - variant = "live", +export const EnvStatusBadge = ({ + variant,
45-54: Optional: Avoid extra wrapper if supported.If
InfoTooltipsupports anasChildprop, consider using it to avoid adding an extra DOM layer around the badge, reducing CSS specificity/hover quirks.
31-35: Prop typing nit: prefer ComponentPropsWithoutRef<"div">.This better mirrors intrinsic props and plays nicer with refs if you later
forwardRef.Example:
import type { ComponentPropsWithoutRef, ReactNode } from "react"; type EnvStatusBadgeProps = ComponentPropsWithoutRef<"div"> & { variant?: VariantProps<typeof statusBadgeVariants>["variant"]; icon?: ReactNode; text: string; };apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx (3)
60-69: Remove ts-expect-error “refetch” hacks; rely on typed invalidationThe try/catch with
// @ts-expect-errorandcollection.*.utils.refetch()is brittle and bypasses types. You already callutils.invalidate(), which should be sufficient. Prefer explicit TRPC invalidations if needed; avoid suppressing types.Apply this diff to drop the hack:
- // hack to revalidate - try { - // @ts-expect-error Their docs say it's here - collection.projects.utils.refetch(); - // @ts-expect-error Their docs say it's here - collection.deployments.utils.refetch(); - // @ts-expect-error Their docs say it's here - collection.domains.utils.refetch(); - } catch (error) { - console.error("Refetch error:", error); - }If additional revalidation is required beyond
utils.invalidate(), consider exposing a typed helper from@/lib/collections(e.g.,revalidateAllCollections()), rather than sprinkling untyped calls here.
47-51: Scope domains to the relevant environment to avoid noiseFiltering by
sticky IN ('environment','live')may include environment-sticky domains from all environments in the project. Typically, rollback should display domains impacted by this deployment (e.g., live domains and environment-sticky domains for the current environment).Consider refining the where-clause to:
- always include
sticky === 'live'- include
sticky === 'environment'only whendomain.environmentId === liveDeployment.environmentIdCan you confirm
domainCollectionis already environment-scoped inuseProjectLayout()? If not, add the extra filter.
118-123: Avoid repeating the “Domain” header for each itemThe header row is rendered inside the map, duplicating it for every domain. Move the header outside the loop for cleaner UI.
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/promotion-dialog.tsx (5)
118-133: Guard against undefined data from useLiveQueryIf
domains.datais undefined while loading,.mapwill throw. Use a fallback.Apply this diff:
- {domains.data.map((domain) => ( + {(domains.data ?? []).map((domain) => ( <div className="space-y-2" key={domain.id}> <div className="flex items-center gap-2"> <h3 className="text-[13px] text-grayA-11">Domain</h3> <CircleInfo size="sm-regular" className="text-gray-9" /> </div> <div className="bg-white dark:bg-black border border-grayA-5 rounded-lg p-4 relative"> <div className="flex items-center"> <Link4 className="text-gray-9" size="sm-medium" /> - <div className="text-gray-12 font-medium text-xs ml-3 mr-2">{domain.domain}</div> + <div className="text-gray-12 font-medium text-xs ml-3 mr-2">{domain.domain}</div> <div className="ml-3" /> </div> </div> </div> ))}Optional: if multiple domains are expected, consider rendering the "Domain" header once outside the map for better UX.
125-129: Make domains clickable and accessibleLink the domain to open in a new tab and add accessible labeling.
Apply this diff:
- <div className="text-gray-12 font-medium text-xs ml-3 mr-2">{domain.domain}</div> + <a + href={/^https?:\/\//.test(domain.domain) ? domain.domain : `https://${domain.domain}`} + target="_blank" + rel="noopener noreferrer" + className="text-gray-12 font-medium text-xs ml-3 mr-2 hover:underline" + aria-label={`Open ${domain.domain}`} + > + {domain.domain} + </a>
78-86: Avoid double error handling with mutateAsync and onErrorYou’re both awaiting
mutateAsyncand handling errors viaonError. Prefer one path to prevent duplicate logging/toasts. Either rely ononError, or wrap in try/catch and removeonError.Apply this diff to rely solely on
onError:const handlePromotion = async () => { - await promote - .mutateAsync({ - targetDeploymentId: targetDeployment.id, - }) - .catch((error) => { - console.error("Promotion error:", error); - }); + await promote.mutateAsync({ + targetDeploymentId: targetDeployment.id, + }); };
156-161: Verify deployment.status renders a user-friendly labelIf
Deployment.statusis an enum (numeric) from proto, rendering it directly may show a number. Confirm it’s a readable string; otherwise map to a human label (e.g., “Pending”, “Active”, “Failed”).
57-67: “Refetch hack” can be replaced with targeted invalidations when availableYou already call
utils.invalidate(). The extracollection.*.utils.refetch()calls (behind@ts-expect-errorand try/catch) add noise and potential runtime errors. Prefer explicit invalidations of affected queries or optional chaining to avoid exceptions until a proper API is available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/promotion-dialog.tsx(4 hunks)apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx(5 hunks)apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx(2 hunks)apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/env-status-badge.tsx(2 hunks)apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-12T17:57:18.337Z
Learnt from: mcstepp
PR: unkeyed/unkey#3952
File: apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx:70-79
Timestamp: 2025-09-12T17:57:18.337Z
Learning: In the deployment rollback functionality, self-rollback scenarios are prevented at the UI level in the actions menu through the `canRollback` condition which includes `!("active" in deployment && deployment.active)`, making additional checks in the dialog component redundant.
Applied to files:
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
PR: unkeyed/unkey#3662
File: apps/dashboard/lib/trpc/routers/deployment/list.ts:11-11
Timestamp: 2025-07-25T19:09:43.284Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/list.ts, the listDeployments procedure intentionally queries the versions table rather than a deployments table. The user mcstepp indicated that renaming the table would require a database migration, which was deferred for the current PR focused on UI features.
Applied to files:
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx
📚 Learning: 2025-09-12T17:57:18.337Z
Learnt from: mcstepp
PR: unkeyed/unkey#3952
File: apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx:70-79
Timestamp: 2025-09-12T17:57:18.337Z
Learning: In the rollback functionality, the actions menu prevents self-rollback scenarios by including `!deployment.active` in the `canRollback` condition, ensuring only inactive deployments show the rollback option.
Applied to files:
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx
🧬 Code graph analysis (3)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx (2)
apps/dashboard/app/(app)/projects/[projectId]/layout-provider.tsx (1)
useProjectLayout(13-19)internal/db/src/schema/domains.ts (1)
domains(4-36)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx (4)
apps/dashboard/app/(app)/projects/[projectId]/layout-provider.tsx (1)
useProjectLayout(13-19)apps/dashboard/components/logs/table-action.popover.tsx (2)
MenuItem(12-22)TableActionPopover(29-148)apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx (1)
RollbackDialog(36-137)apps/dashboard/app/(app)/projects/[projectId]/deployments/components/promotion-dialog.tsx (1)
PromotionDialog(35-138)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/promotion-dialog.tsx (2)
internal/proto/generated/ctrl/v1/deployment_pb.ts (1)
Deployment(172-301)internal/icons/src/icons/link-4.tsx (1)
Link4(16-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Vercel Agent Review
🔇 Additional comments (8)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx (4)
40-72: LGTM: Action gating and dialog wiringConditions align with prior rollout/rollback patterns and prevent self-rollback; ActionComponent usage is clean.
24-29: Use liveDeployment.id and restrict to environment/live domains to satisfy gateway-logs contractFilter domains by sticky in ["environment","live"] and prefer liveDeployment.id; fallback to selectedDeployment.id.
- const { data } = useLiveQuery((q) => - q - .from({ domain: collections.domains }) - .where(({ domain }) => eq(domain.deploymentId, selectedDeployment.id)) - .select(({ domain }) => ({ host: domain.domain })), - ); + const { data } = useLiveQuery((q) => + q + .from({ domain: collections.domains }) + .where(({ domain }) => inArray(domain.sticky, ["environment", "live"])) + .where(({ domain }) => + liveDeployment?.id + ? eq(domain.deploymentId, liveDeployment.id) + : eq(domain.deploymentId, selectedDeployment.id), + ) + .select(({ domain }) => ({ host: domain.domain })), + );
5-7: Import inArray for the sticky filterNeeded for the above query change.
-import { eq, useLiveQuery } from "@tanstack/react-db"; +import { eq, inArray, useLiveQuery } from "@tanstack/react-db";
74-86: Guard undefined data and build encoded, length-safer query; disable until hosts loadPrevents click-time crash and avoids oversized, unencoded CSV query strings.
{ id: "gateway-logs", label: "Go to Gateway Logs...", icon: <Layers3 size="md-regular" />, - onClick: () => { - //INFO: This will produce a long query, but once we start using `contains` instead of `is` this will be a shorter query. - router.push( - `/projects/${selectedDeployment.projectId}/gateway-logs?host=${data - .map((item) => `is:${item.host}`) - .join(",")}`, - ); - }, + disabled: !(data && data.length), + onClick: () => { + const hosts = (data ?? []).map((item) => `is:${item.host}`); + if (!hosts.length) { + router.push(`/projects/${selectedDeployment.projectId}/gateway-logs`); + return; + } + const params = new URLSearchParams(); + hosts.forEach((h) => params.append("host", h)); + router.push( + `/projects/${selectedDeployment.projectId}/gateway-logs?${params.toString()}`, + ); + }, },Optional (prefer once #4001 lands): push a short param and resolve hosts server-side.
router.push( `/projects/${selectedDeployment.projectId}/gateway-logs?liveDeploymentId=${ liveDeployment?.id ?? selectedDeployment.id }`, );apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/env-status-badge.tsx (1)
2-2: Mark EnvStatusBadge as a client component or confirm InfoTooltip is serverEnvStatusBadge imports InfoTooltip from "@unkey/ui" but has no "use client"; its only importer (apps/dashboard/.../deployments/components/table/deployments-list.tsx) is already "use client". If InfoTooltip is a client component, add "use client" to apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/env-status-badge.tsx to avoid RSC build errors — otherwise confirm InfoTooltip is a server component.
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx (1)
71-71: LGTM: Close dialog after successful rollbackCalling
onClose()on success is correct and pairs well with the button’s loading state.apps/dashboard/app/(app)/projects/[projectId]/deployments/components/promotion-dialog.tsx (2)
69-70: LGTM: close dialog after successful promotionClosing on success is appropriate and matches user expectation.
30-33: Prop rename to onClose — verify all call sites and external usagesThis is a breaking public-prop rename; confirm no callers still pass
onOpenChange(search the repo foronOpenChangeand<PromotionDialogand update any stale call sites).
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/promotion-dialog.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx
Show resolved
Hide resolved
The details automatically are shown each time I visit the deployments, shouldn't it be hidden first and only be shown when want it?
|
Yeah we can turn that off just before the demo if we want to |
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (09/24/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
|
Folks don’t merge this. I’ll merge this when I get home. |
|
@ogzhanolguncu lgtm, merge whenever you want |
coderabbitai
bot
added
app:dashboard
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/table/components/log-details/index.tsx (1)
23-39: Prevent false “Log Data Unavailable” toasts while data is loadingOn first render with a new
selectedLog,useFetchRequestDetailsstill needs to fetch the payload, sologis temporarilyundefined. Because we immediately treat!logas a fatal condition, users see the “Log Data Unavailable” error toast on every selection—even when the fetch succeeds a moment later. Please gate this branch behind a loading state (or otherwise ensure the hook distinguishes “still loading” from “no data”), and only surface the toast once the fetch has settled without data.
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx (1)
6-6: Consider centralizing the animation delay constant.
ANIMATION_DELAYis also defined inapps/dashboard/app/(app)/logs/components/table/log-details/index.tsx. Pulling this value from a shared export (e.g., the sharedLogDetailsmodule or a nearby constants file) would prevent the two implementations from drifting if we tweak the timing later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/table/components/log-details/index.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-12-03T14:23:07.189Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx:37-49
Timestamp: 2024-12-03T14:23:07.189Z
Learning: In `apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx`, the resize handler is already debounced.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsxapps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsxapps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/table/components/log-details/index.tsx
📚 Learning: 2024-12-03T14:17:08.016Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx
📚 Learning: 2025-08-25T13:46:34.441Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx:4-4
Timestamp: 2025-08-25T13:46:34.441Z
Learning: The namespace list refresh component (apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx) intentionally uses the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than a namespace-specific hook. This cross-coupling between namespace list components and overview hooks is an architectural design decision.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx
📚 Learning: 2025-08-25T13:46:08.303Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx:1-1
Timestamp: 2025-08-25T13:46:08.303Z
Learning: The NamespaceListDateTime component in apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx is intentionally designed to use the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than the namespace list hook, as clarified by ogzhanolguncu. This coupling is by design, not an architectural issue.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx
🧬 Code graph analysis (3)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx (1)
apps/dashboard/app/(app)/logs/components/table/log-details/index.tsx (1)
LogDetails(12-32)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (5)
internal/clickhouse/src/keys/keys.ts (1)
KeysOverviewLog(123-123)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/components/log-section.tsx (1)
LogSection(5-123)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/components/log-outcome-distribution-section.tsx (1)
OutcomeDistributionSection(6-62)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/components/roles-permissions.tsx (2)
RolesSection(12-60)PermissionsSection(71-136)apps/dashboard/components/logs/details/log-details/index.tsx (1)
LogDetails(100-170)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/table/components/log-details/index.tsx (1)
apps/dashboard/components/logs/details/log-details/index.tsx (1)
LogDetails(100-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
- GitHub Check: Vercel Agent Review
🔇 Additional comments (1)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx (1)
24-30: Shared LogDetails integration looks spot on.The ratelimit drawer now follows the same composition pattern as
/logs, which keeps the close handling and animated sections consistent across surfaces.
...dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx
Show resolved
Hide resolved
|
@chronark you can force merge this |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| }, [ | ||
| queryParams, | ||
| queryClient, | ||
| limit, | ||
| pollIntervalMs, | ||
| historicalLogsMap, | ||
| realtimeLogs, | ||
| historicalLogs, | ||
| ]); |
There was a problem hiding this comment.
The pollForNewLogs callback has realtimeLogs and historicalLogs in its dependency array, but these are derived from state that the callback itself modifies, creating an infinite loop.
View Details
📝 Patch Details
diff --git a/apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/hooks/use-gateway-logs-query.ts b/apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/hooks/use-gateway-logs-query.ts
index 336ca822b..a4892e93a 100644
--- a/apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/hooks/use-gateway-logs-query.ts
+++ b/apps/dashboard/app/(app)/projects/[projectId]/gateway-logs/components/table/hooks/use-gateway-logs-query.ts
@@ -150,7 +150,13 @@ export function useGatewayLogsQuery({
// Query for new logs (polling)
const pollForNewLogs = useCallback(async () => {
try {
- const latestTime = realtimeLogs[0]?.time ?? historicalLogs[0]?.time;
+ const realtimeLatest = realtimeLogsMap.size > 0
+ ? Math.max(...Array.from(realtimeLogsMap.values()).map(log => log.time))
+ : undefined;
+ const historicalLatest = historicalLogsMap.size > 0
+ ? Math.max(...Array.from(historicalLogsMap.values()).map(log => log.time))
+ : undefined;
+ const latestTime = realtimeLatest ?? historicalLatest;
const result = await queryClient.logs.queryLogs.fetch({
...queryParams,
startTime: latestTime ?? Date.now() - pollIntervalMs,
@@ -195,8 +201,7 @@ export function useGatewayLogsQuery({
limit,
pollIntervalMs,
historicalLogsMap,
- realtimeLogs,
- historicalLogs,
+ realtimeLogsMap,
]);
// Set up polling effect
Analysis
Infinite loop in pollForNewLogs callback due to derived state in dependency array
What fails: pollForNewLogs useCallback in use-gateway-logs-query.ts includes realtimeLogs and historicalLogs derived values in dependency array, causing callback recreation on every execution
How to reproduce:
- Start polling with
startPolling: true pollForNewLogsexecutes and callssetRealtimeLogsMaprealtimeLogsMapchange triggersrealtimeLogsrecalculation viauseMemorealtimeLogschange recreatespollForNewLogscallback- Polling interval restarts, cycle repeats infinitely
Result: Excessive API calls and constant interval restarts instead of stable 5-second polling
Expected: Callback should remain stable between executions, with consistent polling interval
Fix applied: Replaced derived array dependencies with underlying state maps and calculated latest timestamps directly from maps instead of sorted arrays
Per React useCallback docs, dependencies should avoid values that change as a result of the callback's own execution.
3 participants
What does this PR do?
This PR adds GatewayLogs to
/projects/gateway-logs. It's mostly copied from regular logs, but with some modifications.Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated