Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughThis extensive update refactors various parts of the dashboard codebase. Several components have updated import paths and refactored constant definitions, while multiple new hooks, UI components, and utilities have been introduced to streamline timestamp formatting, filtering, sorting, and visualization in both logs and rate-limit sections. Deprecated modules, constants, and unused files have been removed. The TRPC routers have been reorganized to support new namespace search and ratelimit analytics features, and navigation components have been updated for improved namespace management. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant NN as NamespaceNavbar
participant NUD as NamespaceUpdateNameDialog
participant ND as DeleteNamespaceDialog
participant API as TRPC API
U->>NN: Click "Edit" or "Delete" option
alt Update Namespace
NN->>NUD: Open update dialog
U->>NUD: Enter new name and submit
NUD->>API: Call update namespace mutation
API-->>NUD: Return success confirmation
NUD->>U: Display success toast and refresh UI
else Delete Namespace
NN->>ND: Open delete dialog
U->>ND: Confirm deletion
ND->>API: Call delete namespace mutation
API-->>ND: Return success confirmation
ND->>U: Display success toast and navigate away
end
sequenceDiagram
participant U as User
participant LS as LogsSearch Component
participant TRPC as Logs Search Mutation
participant Toast as Toast Notification
U->>LS: Enter search query
LS->>TRPC: Execute search mutation with query
alt Successful Response
TRPC-->>LS: Return matching filter data
LS->>U: Update filters with search results
else Error Occurs
TRPC-->>LS: Return error response
LS->>Toast: Display error notification
end
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
internal/icons/src/index.ts (1)
12-13:⚠️ Potential issueRemove duplicate export of
circle-info-sparkle.The icon is exported twice, which could lead to confusion and unnecessarily increase the bundle size.
Apply this diff to remove the duplicate export:
export * from "./icons/circle-info-sparkle"; -export * from "./icons/circle-info-sparkle";
♻️ Duplicate comments (2)
apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts (2)
27-41: 🛠️ Refactor suggestionOptimize namespace lookup by using findFirst instead of findMany.
Since we're only interested in a single namespace and we already have the workspace context, we can simplify this query:
- const ratelimitNamespaces = await db.query.ratelimitNamespaces - .findMany({ + const namespace = await db.query.ratelimitNamespaces + .findFirst({ where: (table, { and, eq, isNull }) => and( eq(table.workspaceId, ctx.workspace.id), and(eq(table.id, input.namespaceId), isNull(table.deletedAt)), ), }) .catch((_err) => { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: - "Failed to retrieve ratelimit timeseries analytics due to a workspace error. If this issue persists, please contact support@unkey.dev with the time this occurred.", + "Failed to retrieve rate limit namespace. If this issue persists, please contact support@unkey.dev.", }); });
58-64: 🛠️ Refactor suggestionSimplify namespace ID access after refactoring.
After refactoring the namespace lookup, update the clickhouse query to use the simplified namespace access:
const result = await clickhouse.ratelimits.overview.logs({ ...transformedInputs, cursorRequestId: input.cursor?.requestId ?? null, cursorTime: input.cursor?.time ?? null, workspaceId: ctx.workspace.id, - namespaceId: ratelimitNamespaces[0].id, + namespaceId: namespace.id, });
🧹 Nitpick comments (10)
apps/dashboard/lib/trpc/routers/ratelimit/query-timeseries/index.ts (3)
11-18: Consider using a single-record query if only one namespace is expected.
db.query.ratelimitNamespaces.findManyalways returns an array (including an empty one). If you expect exactly one matching namespace, usingfindFirstorfindUniquemight be more direct.-const ratelimitNamespaces = await db.query.ratelimitNamespaces - .findMany({...}); +const ratelimitNamespace = await db.query.ratelimitNamespaces + .findFirst({...});
27-31: Redundant null check.
SincefindManywill never returnnull(it would return an empty array on no matches or throw on error), theif (!ratelimitNamespaces)check is redundant.-if (!ratelimitNamespaces) { - throw new TRPCError({ - code: "NOT_FOUND", - message: "Ratelimit namespaces not found, please contact support using support@unkey.dev.", - }); -}
34-39: Clarify separate cases for “no results.”
It's clear you handle an empty array separately, but the error messages might be merged or clarified if there's truly no functional difference between “namespaces not found” and “no results.”apps/dashboard/lib/trpc/routers/ratelimit/query-latency-timeseries/index.ts (3)
8-8: Track the TODO.
There is a//TODO: Refactor this endpoint once we move to AWS. Would you like to open a dedicated issue or PR for that refactor?
12-26: Align error message with actual failure point.
The message references “due to a workspace error,” but the real issue may be the namespace retrieval or another DB issue. Consider adjusting the wording.
28-33: Remove or rethink the!ratelimitNamespacescheck.
As in the other file,findManywon’t returnnull; an empty array is returned when no matches exist or an error is thrown in.catch().apps/dashboard/lib/trpc/routers/ratelimit/query-logs/index.ts (3)
27-34: PreferfindFirst/findUniquefor single-match scenarios.
If a single namespace is expected, consider switching tofindFirstorfindUnique, asfindManymight introduce edge cases if multiple namespaces match.
39-39: Revise wording to accurately reflect the cause of failure.
Referring to “a workspace error” may be confusing if the query is specifically for namespaces. Clarify the error message for quicker troubleshooting.
43-47: Check array length instead of null.
findManywill not yieldnull; this check can be simplified by removing the!ratelimitNamespacescondition and relying solely on the empty array check.apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts (1)
10-21: Add JSDoc comments to document the response schema.Consider adding JSDoc comments to document the purpose and structure of the response schema, especially for fields like
hasMoreandnextCursorthat are used for pagination.+/** + * Response schema for rate limit overview logs. + * @property ratelimitOverviewLogs - Array of rate limit overview log entries + * @property hasMore - Indicates if there are more logs available + * @property nextCursor - Cursor for pagination, contains time and requestId for the next page + */ const RatelimitOverviewLogsResponse = z.object({ ratelimitOverviewLogs: z.array(ratelimitOverviewLogs), hasMore: z.boolean(), nextCursor: z .object({ time: z.number().int(), requestId: z.string(), }) .optional(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/namespace-delete-dialog.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/namespace-update-dialog.tsx(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/namespace-search.ts(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-latency-timeseries/index.ts(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-logs/index.ts(2 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-timeseries/index.ts(3 hunks)apps/dashboard/lib/trpc/routers/ratelimit/updateNamespaceName.ts(1 hunks)internal/icons/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/namespace-update-dialog.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/namespace-delete-dialog.tsx
- apps/dashboard/lib/trpc/routers/ratelimit/namespace-search.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
internal/icons/src/index.ts (1)
41-49: LGTM! The new icon exports enhance the UI capabilities.The new icons follow the established pattern and naming convention. They appear to support rate limiting features with icons for actions like blocking, cloning, and showing progress/information.
apps/dashboard/lib/trpc/routers/ratelimit/query-timeseries/index.ts (1)
47-48: Handle multiple matched namespaces or confirm only one match is expected.
Currently, the code uses the first array element. If multiple namespaces match, only one is used. Confirm if that is desired or if an error should be thrown when multiple matches occur.apps/dashboard/lib/trpc/routers/ratelimit/query-logs/index.ts (2)
50-50: Confirm if multiple namespace matches need distinct handling.
WhenratelimitNamespaces.length === 0, the code throws an error. Consider similarly verifying if more than one matching namespace is unexpected.
63-64: Evaluate multi-namespace scenarios.
UsingratelimitNamespaces[0].idsilently discards additional matches. Verify that handling multiple potential matches is not required.apps/dashboard/lib/trpc/routers/ratelimit/updateNamespaceName.ts (1)
11-14: LGTM! Input schema simplified by removing redundant workspaceId.The removal of
workspaceIdfrom the input schema is a good improvement as it's already available inctx.workspace.id. This change reduces redundancy and potential inconsistencies.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/dashboard/components/logs/llm-search/index.tsx (2)
101-112: Enhance clear button accessibility and structure.The clear button implementation is good but could be improved.
Consider these improvements:
{searchText.length > 0 && !hideClear && ( <button - aria-label="Clear search" + type="button" + aria-label="Clear search input and reset filters" onClick={() => { setSearchText(""); onClear?.(); }} - type="button" > <XMark className="size-4 text-accent-9" /> </button> )}
149-149: Fix icon size inconsistency.The CaretRightOutline icon size is inconsistent with other icons in the component.
-<CaretRightOutline className="size-2 text-accent-9" /> +<CaretRightOutline className="size-[10px] text-accent-9" />apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx (3)
10-18: Consider extracting API endpoint and improving placeholder visibility.The example snippet is well-formatted, but consider these improvements:
- Extract the API endpoint to a constant or config to maintain DRY if used elsewhere
- Make the placeholder more obvious by using something like
YOUR_UNKEY_ROOT_KEY+const API_ENDPOINT = 'https://api.unkey.dev/v1/ratelimits.limit'; const EXAMPLE_SNIPPET = `curl -XPOST '${API_ENDPOINT}' \\ -H 'Content-Type: application/json' \\ - -H 'Authorization: Bearer <UNKEY_ROOT_KEY>' \\ + -H 'Authorization: Bearer YOUR_UNKEY_ROOT_KEY' \\ -d '{ "namespace": "demo_namespace", "identifier": "user_123", "limit": 10, "duration": 10000 }'`;
20-47: Consider extracting EmptyNamespaces to a separate file.The component is well-structured but could be moved to its own file to improve maintainability and reduce file size. This would follow the single responsibility principle and make the code more modular.
Consider moving it to:
apps/dashboard/app/(app)/ratelimits/_components/empty-namespaces.tsx
49-84: Consider extracting TypeScript interfaces for better type safety.The components are well-structured, but could benefit from dedicated TypeScript interfaces:
interface Namespace { id: string; name: string; } interface NamespaceGridProps { namespaces: Namespace[]; } interface RatelimitClientProps { ratelimitNamespaces: Namespace[]; }This would improve type reusability and make the code more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/ratelimits/_components/controls/components/logs-search/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx(1 hunks)apps/dashboard/components/logs/llm-search/index.tsx(2 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts(1 hunks)internal/clickhouse/src/ratelimits.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/app/(app)/ratelimits/_components/controls/components/logs-search/index.tsx
- apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/dashboard/components/logs/llm-search/index.tsx (3)
7-14: LGTM! Props type definition is well-structured.The new optional props are well-typed and follow clear naming conventions.
16-23: LGTM! Component signature is well-defined with sensible defaults.The default values maintain backward compatibility while providing flexibility for customization.
91-91: LGTM! Placeholder text is now configurable.The hardcoded placeholder text has been replaced with the configurable placeholder prop.
apps/dashboard/app/(app)/ratelimits/_components/ratelimit-client.tsx (2)
1-9: LGTM! Clean import structure and proper client directive.The imports are well-organized and the "use client" directive is correctly placed at the top of the file.
1-84: LGTM! Well-structured React components with room for minor improvements.The code is clean, follows React best practices, and implements a good user experience with proper empty states and responsive design. The suggested improvements are mostly about code organization and TypeScript enhancements, but the current implementation is solid.
internal/clickhouse/src/ratelimits.ts (5)
48-48: Looks good:stepSizeproperty addition.
Defining astepSizeproperty in theTimeIntervaltype is a clear and flexible way to represent multi-minute or multi-hour intervals without relying on dedicated time functions.
57-86: Great extension of available intervals.
The newly introduced intervals (fiveMinutes,fifteenMinutes,thirtyMinutes,twoHours,fourHours,sixHours) expand the granularity of the timeseries data. The table references andstepSizevalues are consistent with the existing logic.
105-113: Validate against unexpected interval keys.
Although the current code maps all known values ofinterval.step(e.g.,MINUTE,MINUTES,HOUR, etc.) to valid ClickHouse units, consider adding a fallback or assertion to handle any unexpectedinterval.stepvalue (e.g., due to a typo) to prevent silent misbehavior.Would you like a script to locate or detect all possible
interval.stepassignments in the codebase for additional safety checks?
127-129: Confirm time boundaries withWITH FILL.
UsingWITH FILLhelps ensure a complete timeseries, but confirm the exact boundary requirements (inclusive/exclusive) match the product’s needs—for example, whether you need timeslots beyond the final usage or exactly up toendTime.
594-789: Latency timeseries design is consistent.
TheratelimitLatencyTimeseriesParamsschema, interval definitions, and MergeTree aggregate functions foravg_latencyandp99_latencyfollow the existing pattern correctly. This ensures consistency across rate-limits and latency timeseries.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.test.ts (2)
75-84: Add test case for invalid namespace ID.The test suite covers invalid filter types but doesn't verify behavior with invalid namespace IDs. Consider adding a test case for this scenario.
Example test case:
it("handles invalid namespace ID", () => { const consoleMock = vi.spyOn(console, "error"); renderHook(() => useRatelimitLogsQuery({ namespaceId: "" })); expect(consoleMock).toHaveBeenCalled(); });
148-156: Enhance test coverage for namespace-specific behavior.While the test verifies realtime logs reset behavior, it would be valuable to add test cases that verify:
- Logs are filtered correctly by namespace ID
- Namespace changes trigger appropriate resets/refetches
Example test case:
it("filters logs by namespace ID", () => { const mockLogs = [ { request_id: "1", namespace_id: "test-namespace" }, { request_id: "2", namespace_id: "other-namespace" } ]; useInfiniteQuery.mockReturnValue({ data: { pages: [{ ratelimitLogs: mockLogs, nextCursor: null }], }, // ... other mock values }); const { result } = renderHook(() => useRatelimitLogsQuery({ namespaceId: "test-namespace" }) ); expect(result.current.historicalLogs).toHaveLength(1); expect(result.current.historicalLogs[0].request_id).toBe("1"); });apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/table-action-popover.tsx (5)
41-43: Consider using a constant for filter fields.The filter fields are hardcoded strings that could be moved to constants to prevent typos and improve maintainability.
+const TIME_FILTER_FIELDS = ['startTime', 'endTime', 'since'] as const; + const timeFilters = filters.filter((f) => - ["startTime", "endTime", "since"].includes(f.field) + TIME_FILTER_FIELDS.includes(f.field) );
141-144: Consider consolidating keyboard event handlers.The keyboard event handling for 'Enter', 'ArrowRight', 'l', and 'Space' could be simplified using a Set for better performance and maintainability.
+const ACTIVATION_KEYS = new Set(['Enter', 'ArrowRight', 'l', ' ']); + -case "Enter": -case "ArrowRight": -case "l": -case " ": +case ACTIVATION_KEYS.has(e.key):
41-64: Consider using URLSearchParams constructor for better security.The
getTimeParamsfunction could be simplified and made more secure by using the URLSearchParams constructor for all parameters.const getTimeParams = () => { - const params = new URLSearchParams({ - identifier: `contains:${identifier}`, - }); - - // Only add time parameters if they exist - const timeMap = { - startTime: timeFilters.find((f) => f.field === "startTime")?.value, - endTime: timeFilters.find((f) => f.field === "endTime")?.value, - since: timeFilters.find((f) => f.field === "since")?.value, - }; - - Object.entries(timeMap).forEach(([key, value]) => { - if (value) { - params.append(key, value.toString()); - } - }); + const params = new URLSearchParams(); + params.append('identifier', `contains:${identifier}`); + + timeFilters.forEach(filter => { + if (filter.value) { + params.append(filter.field, filter.value.toString()); + } + }); return params.toString(); };
103-152: Enhance keyboard navigation with ARIA attributes.The keyboard navigation implementation is good but could be enhanced with ARIA attributes for better screen reader support.
<div className="flex flex-col gap-2" role="menu" + aria-label="Identifier actions" onClick={(e) => e.stopPropagation()} onKeyDown={handleKeyDown} >
205-225: Remove redundant biome-ignore comment.The biome-ignore comment for a11y/useKeyWithClickEvents is unnecessary as it doesn't provide a meaningful explanation.
- {/* biome-ignore lint/a11y/useKeyWithClickEvents: <explanation> */}apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/identifier-dialog.tsx (3)
31-47: Consider using constants for validation limits.The validation schema contains magic numbers that could be moved to constants for better maintainability and documentation.
+const VALIDATION_LIMITS = { + IDENTIFIER_MIN_LENGTH: 2, + IDENTIFIER_MAX_LENGTH: 250, + LIMIT_MIN: 1, + LIMIT_MAX: 10_000, + DURATION_MIN: 1_000, + DURATION_MAX: 24 * 60 * 60 * 1000, +} as const; + const overrideValidationSchema = z.object({ identifier: z .string() .trim() - .min(2, "Name is required and should be at least 2 characters") - .max(250), + .min(VALIDATION_LIMITS.IDENTIFIER_MIN_LENGTH, + `Name is required and should be at least ${VALIDATION_LIMITS.IDENTIFIER_MIN_LENGTH} characters`) + .max(VALIDATION_LIMITS.IDENTIFIER_MAX_LENGTH), limit: z.coerce .number() .int() - .min(1, "Limit must be at least 1") - .max(10_000, "Limit cannot exceed 10,000"), + .min(VALIDATION_LIMITS.LIMIT_MIN, `Limit must be at least ${VALIDATION_LIMITS.LIMIT_MIN}`) + .max(VALIDATION_LIMITS.LIMIT_MAX, `Limit cannot exceed ${VALIDATION_LIMITS.LIMIT_MAX}`),
31-48: Consider adding custom error messages for all validation rules.The validation schema has inconsistent error messaging. Some rules have custom messages while others use default messages.
const overrideValidationSchema = z.object({ identifier: z .string() .trim() - .min(2, "Name is required and should be at least 2 characters") - .max(250), + .min(2, "Name is required and should be at least 2 characters") + .max(250, "Name cannot exceed 250 characters"), limit: z.coerce .number() .int() .min(1, "Limit must be at least 1") .max(10_000, "Limit cannot exceed 10,000"), duration: z.coerce .number() .int() .min(1_000, "Duration must be at least 1 second (1000ms)") .max(24 * 60 * 60 * 1000, "Duration cannot exceed 24 hours"), - async: z.enum(["unset", "sync", "async"]), + async: z.enum(["unset", "sync", "async"], { + errorMap: () => ({ message: "Invalid override type selected" }) + }), });
198-204: Consider using disabled attribute alone for readonly fields.The input field uses both
readOnlyanddisabledprops, which is redundant.<Input {...register("identifier")} - readOnly disabled className="border-gray-4 focus:border-gray-5 px-3 py-1" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/identifier-dialog.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/table-action-popover.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.test.ts(5 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/index.tsx
- apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: autofix
🔇 Additional comments (4)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/identifier-dialog.tsx (4)
60-61: Add keyboard accessibility to form field.The form field uses
onClickto stop event propagation but lacks keyboard accessibility.
148-175: Improve error handling in form submission.The error handling could be improved.
60-61: Add keyboard accessibility to form field.The form field uses
onClickto stop event propagation but lacks keyboard accessibility.
148-175: Improve error handling in form submission.The error handling could be improved.
...hboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.test.ts
Show resolved
Hide resolved
...ceId]/_overview/components/table/components/logs-actions/components/table-action-popover.tsx
Show resolved
Hide resolved
...ceId]/_overview/components/table/components/logs-actions/components/table-action-popover.tsx
Show resolved
Hide resolved
...spaceId]/_overview/components/table/components/logs-actions/components/identifier-dialog.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.test.ts (1)
92-166: Add test coverage forREALTIME_DATA_LIMIT.Based on the related changes mentioned in the summary, the hook has introduced a new constant
REALTIME_DATA_LIMITset to 100 for limiting realtime logs. Consider adding test cases to verify:
- The limit is enforced when realtime logs exceed 100 entries
- The oldest entry (by timestamp) is removed when the limit is reached
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/table-action-popover.tsx (5)
8-8: Consider using absolute imports for better maintainability.The deep relative import path (
../../../../../hooks/use-filters) could be problematic for maintenance. Consider using absolute imports with path aliases.-import { useFilters } from "../../../../../hooks/use-filters"; +import { useFilters } from "@/hooks/use-filters";
12-16: Add JSDoc documentation for the Props type.Adding documentation for the Props type would improve code maintainability and help other developers understand the purpose of each prop.
+/** + * Props for the TableActionPopover component + * @property {string} identifier - The identifier to be displayed and acted upon + * @property {string} namespaceId - The namespace ID for routing + * @property {OverrideDetails | null} [overrideDetails] - Optional details for overriding the identifier + */ type Props = { identifier: string; namespaceId: string; overrideDetails?: OverrideDetails | null; };
33-52: Consider memoizing getTimeParams for performance.The getTimeParams function rebuilds the URL parameters on every render. Consider using useMemo to optimize performance.
+const getTimeParams = useMemo(() => { -const getTimeParams = () => { const params = new URLSearchParams({ identifier: `contains:${identifier}`, }); const timeMap = { startTime: timeFilters.find((f) => f.field === "startTime")?.value, endTime: timeFilters.find((f) => f.field === "endTime")?.value, since: timeFilters.find((f) => f.field === "since")?.value, }; Object.entries(timeMap).forEach(([key, value]) => { if (value) { params.append(key, value.toString()); } }); return params.toString(); -}; +}, [identifier, timeFilters]);
91-138: Extract magic number and enhance keyboard navigation.The keyboard navigation logic uses the magic number '3' multiple times. Consider extracting it as a constant and adding aria-keyshortcuts attribute for better accessibility.
+const MENU_ITEMS_COUNT = 3; + const handleKeyDown = (e: KeyboardEvent) => { e.stopPropagation(); const activeElement = document.activeElement; const currentIndex = menuItems.current.findIndex((item) => item === activeElement); switch (e.key) { case "Tab": e.preventDefault(); if (!e.shiftKey) { - setFocusIndex((currentIndex + 1) % 3); - menuItems.current[(currentIndex + 1) % 3]?.focus(); + setFocusIndex((currentIndex + 1) % MENU_ITEMS_COUNT); + menuItems.current[(currentIndex + 1) % MENU_ITEMS_COUNT]?.focus(); } else { - setFocusIndex((currentIndex - 1 + 3) % 3); - menuItems.current[(currentIndex - 1 + 3) % 3]?.focus(); + setFocusIndex((currentIndex - 1 + MENU_ITEMS_COUNT) % MENU_ITEMS_COUNT); + menuItems.current[(currentIndex - 1 + MENU_ITEMS_COUNT) % MENU_ITEMS_COUNT]?.focus(); } break;
163-168: Enhance accessibility with descriptive labels.Add aria-labels to interactive elements for better screen reader support.
<div className="flex flex-col gap-2" role="menu" + aria-label="Identifier actions menu" onClick={(e) => e.stopPropagation()} onKeyDown={handleKeyDown} > <div ref={(el) => { if (el) { menuItems.current[0] = el; } }} role="menuitem" + aria-label="View logs for this identifier" tabIndex={focusIndex === 0 ? 0 : -1} className="flex w-full items-center px-2 py-1.5 gap-3 rounded-lg group cursor-pointer hover:bg-gray-3 data-[state=open]:bg-gray-3 focus:outline-none focus:bg-gray-3" >Also applies to: 174-187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/identifier-dialog.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/table-action-popover.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.test.ts(5 hunks)apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/index.tsx
- apps/dashboard/lib/trpc/routers/ratelimit/query-overview-logs/index.ts
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/identifier-dialog.tsx
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.test.ts (1)
142-150: LGTM! Well-structured test with clear parameter organization.The test case properly includes the
namespaceIdparameter alongside polling parameters, and effectively verifies the reset functionality for realtime logs.apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/components/table-action-popover.tsx (1)
18-30: LGTM! Well-structured state management.The component setup demonstrates good practices:
- Proper TypeScript usage with PropsWithChildren
- Clear state management for modal and focus control
- Efficient use of useRef for menu items tracking
...hboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.test.ts
Show resolved
Hide resolved
...ceId]/_overview/components/table/components/logs-actions/components/table-action-popover.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
256-267: Consider minor improvements to the documentation button.The documentation button implementation looks good and follows security best practices. Consider these minor improvements:
- Move the URL to a constants file for better maintainability
- Add an aria-label to the button for better accessibility
+const DOCS_URL = "https://www.unkey.com/docs/introduction"; <Empty.Actions className="mt-4 justify-start"> <a - href="https://www.unkey.com/docs/introduction" + href={DOCS_URL} target="_blank" rel="noopener noreferrer" > - <Button> + <Button aria-label="View documentation"> <BookBookmark /> Documentation </Button> </a> </Empty.Actions>apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-actions/components/table-action-popover.tsx (1)
98-99: Inconsistent navigation implementation between keyboard and click handlers.The keyboard handler uses
pushfor navigation while the click handler uses aLinkcomponent. While both work, using different approaches could lead to inconsistent behavior. Consider unifying the implementation to use eitherLinkorpushconsistently.Here's how to unify using the
Linkcomponent approach:- } else if (activeElement === menuItems.current[1]) { - push(`/ratelimits/${namespaceId}/overrides?identifier=${identifier}`); + } else if (activeElement === menuItems.current[1]) { + const link = menuItems.current[1]?.querySelector('a'); + link?.click();Or alternatively, unify using the
pushapproach:- <Link - href={`/ratelimits/${namespaceId}/overrides?identifier=${identifier}`} - tabIndex={-1} - onClick={(e) => e.stopPropagation()} - > <div ref={(el) => { if (el) { menuItems.current[1] = el; } }} role="menuitem" tabIndex={focusIndex === 1 ? 0 : -1} className="flex w-full items-center px-2 py-1.5 justify-between rounded-lg group cursor-pointer hover:bg-gray-3 data-[state=open]:bg-gray-3 focus:outline-none focus:bg-gray-3" + onClick={(e) => { + e.stopPropagation(); + push(`/ratelimits/${namespaceId}/overrides?identifier=${identifier}`); + }} > <span className="text-[13px] text-accent-12 font-medium">Override</span> <PenWriting3 /> </div> - </Link>Also applies to: 152-171
internal/clickhouse/src/ratelimits.ts (1)
449-450: Consider removing commented fields if not needed.The fields
avg_latencyandp99_latencyare commented out in theratelimitOverviewLogsschema. If these are not planned for immediate use, consider removing them to avoid confusion.- // avg_latency: z.number().int(), - // p99_latency: z.number().int(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-actions/components/table-action-popover.tsx(3 hunks)internal/clickhouse/src/ratelimits.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/clickhouse/src/ratelimits.ts (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2896
File: internal/clickhouse/src/ratelimits.ts:468-592
Timestamp: 2025-02-21T11:15:16.185Z
Learning: The cursor logic in getRatelimitOverviewLogs query uses (time, request_id) < (cursorTime, cursorRequestId) comparison which works well with descending order but may need adjustment for ascending sorts based on real usage patterns.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (2)
9-10: LGTM!The new imports for the documentation button are correctly added and follow the project's conventions.
1-273: Note: Discrepancy between code changes and AI summary.The AI summary mentions changes to handle undefined values in log entries and updates to the empty state description, but these changes are not present in the provided code.
Likely an incorrect or invalid review comment.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-actions/components/table-action-popover.tsx (3)
7-7: LGTM: Router import and usage follows Next.js best practices.The addition of
useRouterand destructuring ofpushfollows Next.js recommended patterns for client-side navigation.Also applies to: 17-17
58-105: LGTM: Comprehensive keyboard navigation implementation.The keyboard navigation implementation is thorough and well-structured:
- Handles both arrow keys and vim-style navigation (j/k)
- Maintains proper focus management
- Prevents default browser behavior where appropriate
- Implements circular navigation through menu items
129-134: LGTM: Strong accessibility implementation.The component demonstrates good accessibility practices:
- Proper ARIA roles for menu and menu items
- Managed focus states
- Keyboard navigation support
- Click event handling with proper event propagation control
Also applies to: 136-151, 172-187
internal/clickhouse/src/ratelimits.ts (4)
45-101: LGTM! Enhanced time interval handling.The changes to
TimeIntervaltype andINTERVALSobject improve flexibility by:
- Using
stepSizefor more precise interval control- Adding new granularities (5min, 15min, 30min, 2h, 4h, 6h)
104-130: LGTM! Improved query generation logic.The refactored
createTimeseriesQueryfunction:
- Uses stepSize for flexible interval handling
- Has clear interval unit mapping
- Maintains consistent SQL structure
534-547: LGTM! Improved cursor logic for sorting.The code now properly handles ascending and descending sorts by:
- Explicitly checking for time sort direction
- Maintaining consistent sort order for time and request_id
- Ensuring proper cursor behavior
595-789: LGTM! Well-structured latency tracking implementation.The new latency tracking functionality:
- Follows consistent patterns with rate limit tracking
- Properly handles different time granularities
- Uses specialized tables for latency statistics
2 participants
What does this PR do?
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
Summary by CodeRabbit
New Features
LogsChartErrorcomponent for displaying error messages related to log retrieval.LogsChartLoadingto visualize loading states with mock data in charts.NamespaceNavbarfor better navigation within the rate limit context.FilterOperatorInputfor structured filtering options and inputs.LogsTimeseriesBarChartfor visualizing timeseries data in a bar chart format.RatelimitOverviewLogsTableto display a virtual table of rate limit logs with enhanced sorting and filtering.Refactor
queryRatelimitTimeseriesfunction to focus on ratelimit namespaces, improving error handling and data retrieval logic.LogsClientcomponent to encapsulate namespace display logic, enhancing modularity.VirtualTablecomponent with interactive sorting capabilities in table headers.